Move ipts_fw_config.bin into the companion driver

[StollD]

As discussed on IRC, this modifies IPTS to have the companion driver provide the firmware config directly, instead of loading it from an obscure binary file that was provided by Intel ages ago.

To archive this the companion driver interface has been overhauled to be more easily extensible (just add the things you need to the ipts_companion struct and they will be usable from IPTS).

The public header files for IPTS under include/linux have been updated as well, since it was neccessary to add another one there (ipts-binary.h, formerly ipts-binary-spec.h in the driver).

[qzed]

CC @GrayHatter, if you want to look at it

[GrayHatter]

I like this, is look like a generally good change! A good portion of these are nitpicky things, as I'm still learning this code, and haven't had time to read/understand all of it.

[GrayHatter]

why is this info and not error?

[StollD]

Because it could happen that multiple companions get probed, or the user explicitly tells IPTS to ignore the companion. In that case, we didn't really want the companion to error.

[qzed]

I think multiple companions probing should be at least a warning, if not an error. I'd normally expect that only one companion driver gets loaded for a specific machine. Also, as far as I can tell, if a user sets the modparam to ignore the IPTS companion, ipts_add_companion does not return an error, so I think it's safe to change this to something like dev_warn(..., "IPTS companion driver already present, aborting\n").

[StollD]

Done, changed dev_info to dev_warn.

[GrayHatter]

is there a reason you picked mutex over spin here?

[StollD]

Because qzed told me so when writing the original implementation :P (in ipts-fw.c which was deleted).

I am not that familiar with synchronisation inside of the kernel, so I just kept it like that.

[qzed]

My reasoning was mostly: Mutex is the standard tool and it's initialization code, so we don't need the performance a spin-lock would give us here. Also the lock is being held during IO operations (request_firmware), so not really small critical sections.

[GrayHatter]

This looks awkard, can you rewrite it without the goto?

[StollD]

I could but then I would have to duplicate the mutex_unlock. I'd rather keep it that way, the kernel coding guidelines explicitly recommend goto in such cases.

[qzed]

I guess in this case it could be rewritten without gotos (as it's pretty small), but for longer functions that's the usual kernel style.

[StollD]

Done, used if / else instead of goto. The if condition looks a bit janky, but personally I'm fine with that.

[GrayHatter]

check companion != NULL before getting a mutex

[StollD]

Well, the mutex was added to make sure that every access to the companion driver is synchronised, so I am not sure if that wouldn't defeat the purpose of the mutex.

@qzed ?

[qzed]

Given that we are on x86, accessing ipts_companion outside of the critical section should be safe in this context, but I don't like relying on platform-specifics, especially if unmarked. Also can we guarantee that this will only run on x86? It's IPTS so probably.... Anyways, given that this function is not being called frequently (only during initialization), I don't think optimizations like these make much sense.

[GrayHatter]

shouldn't this function be in the header as well?

[StollD]

Both, ipts_add_companion and ipts_remove_companion are exported in the public header file (include/linux/ipts-companion.h) for other modules to use. The other functions are internal use by IPTS itself, so they are exported in the local header file inside driver/

[GrayHatter]

this whole function can be written better without any gotos

[StollD]

See my answer above: Not using gotos would yield duplicated mutex_unlock calls all over the place, and using goto in such a case is what the kernel coding guidelines advise.

[qzed]

I'm not necessarily for throwing out the gotos, but I think we can reduce the code beneath the mutex. I think actually only the code until (including) *cfg = ipts_alloc_fw_list(ipts_companion->firmware_config); needs to be synchronized. ipts_request_firmware does its own checks and synchronization, and the rest is stuff on data that we own at that point.

[qzed]

Also with that you'll probably get rid of one goto.

[StollD]

I looked into how to reduce the amount of gotos in that function, but I found no nice way that allows for all the fallback behaviour that is there, and that doesn't require duplicating the call to mutex_unlock.

If you want me to change it, I will look into it again ofc.

[qzed]

What I meant was something like this:

int ipts_request_firmware_config(ipts_info_t *ipts, ipts_bin_fw_list_t **cfg)
{
	int ret = 0;
	const struct firmware *config_fw = NULL;
	mutex_lock(&ipts_companion_lock);

	// Check if a companion was registered. If not, skip
	// forward and try to load the firmware config from a file
	if (ipts_modparams.ignore_companion || ipts_companion == NULL) {
		mutex_unlock(&ipts_companion_lock);
		goto config_fallback;
	}

	if (ipts_companion->firmware_config != NULL) {
		*cfg = ipts_alloc_fw_list(ipts_companion->firmware_config);
		mutex_unlock(&ipts_companion_lock);
		return 0;
	}

config_fallback:

	// If fallback loading for the firmware config was disabled, abort.
	// Return -ENOENT as no config file was found.
	if (ipts_modparams.ignore_config_fallback) {
		ret = -ENOENT;
		goto config_return;
	}

	// No firmware config was found by the companion driver,
	// try loading it from a file now
	ret = ipts_request_firmware(&config_fw, IPTS_FW_CONFIG_FILE,
			&ipts->cldev->dev);
	if (ret)
		return ret;

	*cfg = (ipts_bin_fw_list_t *)config_fw->data;
	release_firmware(config_fw);
	return 0;
}

You only need the mutex to guard the direct ipts_companion access, the rest doesn't need to be in the critical section (especially as ipts_request_firmware does all that itself). I'd still keep the goto for the fallback as I don't see a cleaner option.

[StollD]

Ok, yeah, that is definitly nicer. Updated the function, thank you.