Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move ipts_fw_config.bin into the companion driver #11

Merged
merged 3 commits into from
Nov 4, 2019
Merged

Move ipts_fw_config.bin into the companion driver #11

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1531a7
ipts: Implement new companion interface
StollD Oct 20, 2019
e1cfd84
ipts: Move ipts_fw_config.bin into the companion
StollD Oct 20, 2019
636e23d
ipts: Address comments by qzed and grayhatter
StollD Oct 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/gpu/drm/i915_legacy/intel_ipts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#include <linux/kernel.h>
#include <linux/types.h>
#include <linux/module.h>
#include <linux/intel_ipts_if.h>
#include <linux/ipts-gfx.h>
#include <drm/drmP.h>

#include "intel_guc_submission.h"
Expand Down
2 changes: 1 addition & 1 deletion drivers/misc/ipts/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#

obj-$(CONFIG_INTEL_IPTS)+= intel-ipts.o
intel-ipts-objs += ipts-fw.o
intel-ipts-objs += ipts-companion.o
intel-ipts-objs += ipts-mei.o
intel-ipts-objs += ipts-hid.o
intel-ipts-objs += ipts-msg-handler.o
Expand Down
107 changes: 69 additions & 38 deletions drivers/misc/ipts/companion/ipts-surface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,69 @@
#include <linux/acpi.h>
#include <linux/firmware.h>
#include <linux/intel_ipts_fw.h>
#include <linux/intel_ipts_if.h>
#include <linux/ipts.h>
#include <linux/ipts-companion.h>
#include <linux/module.h>
#include <linux/platform_device.h>

#define IPTS_SURFACE_FW_PATH_FMT "intel/ipts/%s/%s"

#define __IPTS_SURFACE_FIRMWARE(X, Y) \
MODULE_FIRMWARE("intel/ipts/" X "/" Y)

#define IPTS_SURFACE_FIRMWARE(X) \
__IPTS_SURFACE_FIRMWARE(X, "config.bin"); \
__IPTS_SURFACE_FIRMWARE(X, "intel_desc.bin"); \
__IPTS_SURFACE_FIRMWARE(X, "intel_fw_config.bin"); \
__IPTS_SURFACE_FIRMWARE(X, "vendor_desc.bin"); \
__IPTS_SURFACE_FIRMWARE(X, "vendor_kernel.bin")

IPTS_SURFACE_FIRMWARE("MSHW0076");
IPTS_SURFACE_FIRMWARE("MSHW0078");
IPTS_SURFACE_FIRMWARE("MSHW0079");
IPTS_SURFACE_FIRMWARE("MSHW0101");
IPTS_SURFACE_FIRMWARE("MSHW0102");
IPTS_SURFACE_FIRMWARE("MSHW0103");
IPTS_SURFACE_FIRMWARE("MSHW0137");
#define IPTS_SURFACE_FIRMWARE(X) \
MODULE_FIRMWARE("intel/ipts/" X "/config.bin"); \
MODULE_FIRMWARE("intel/ipts/" X "/intel_desc.bin"); \
MODULE_FIRMWARE("intel/ipts/" X "/vendor_desc.bin"); \
MODULE_FIRMWARE("intel/ipts/" X "/vendor_kernel.bin"); \

int ipts_surface_request_firmware(const struct firmware **fw, const char *name,
struct device *device, void *data)
struct device *device, ipts_companion_t *companion)
{
char fw_path[MAX_IOCL_FILE_PATH_LEN];

if (data == NULL) {
if (companion == NULL || companion->data == NULL) {
return -ENOENT;
}

snprintf(fw_path, MAX_IOCL_FILE_PATH_LEN, IPTS_SURFACE_FW_PATH_FMT,
(const char *)data, name);
(const char *)companion->data, name);
return request_firmware(fw, fw_path, device);
}

static ipts_bin_fw_info_t ipts_surface_vendor_kernel = {
.fw_name = "vendor_kernel.bin",
.vendor_output = -1,
.num_of_data_files = 3,
.data_file = {
{
.io_buffer_type = IPTS_CONFIGURATION,
.flags = IPTS_DATA_FILE_FLAG_NONE,
.file_name = "config.bin",
},

// The following files are part of the config, but they don't
// exist, and the driver never requests them.
qzed marked this conversation as resolved.
Show resolved Hide resolved
{
.io_buffer_type = IPTS_CALIBRATION,
.flags = IPTS_DATA_FILE_FLAG_NONE,
.file_name = "calib.bin",
},
{
.io_buffer_type = IPTS_FEATURE,
.flags = IPTS_DATA_FILE_FLAG_SHARE,
.file_name = "feature.bin",
},
},
};

static ipts_bin_fw_info_t *ipts_surface_fw_config[] = {
&ipts_surface_vendor_kernel,
NULL,
};

static ipts_companion_t ipts_surface_companion = {
.firmware_request = &ipts_surface_request_firmware,
.firmware_config = ipts_surface_fw_config,
.name = "ipts_surface",
};

static int ipts_surface_probe(struct platform_device *pdev)
{
int ret;
Expand All @@ -49,11 +74,11 @@ static int ipts_surface_probe(struct platform_device *pdev)
return -ENODEV;
}

ret = intel_ipts_add_fw_handler(&ipts_surface_request_firmware,
(void *)acpi_device_hid(adev));
ipts_surface_companion.data = (void *)acpi_device_hid(adev);
ret = ipts_add_companion(&ipts_surface_companion);
if (ret) {
dev_info(&pdev->dev, "Adding IPTS firmware handler failed, "
"error: %d\n", ret);
dev_info(&pdev->dev, "Adding IPTS companion failed, "
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this info and not error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, changed dev_info to dev_warn.

"error: %d\n", ret);
return ret;
}

Expand All @@ -62,25 +87,23 @@ static int ipts_surface_probe(struct platform_device *pdev)

static int ipts_surface_remove(struct platform_device *pdev)
{
int ret;

ret = intel_ipts_rm_fw_handler(&ipts_surface_request_firmware);
int ret = ipts_remove_companion(&ipts_surface_companion);
if (ret) {
dev_info(&pdev->dev, "Removing IPTS firmware handler failed, "
"error: %d\n", ret);
dev_info(&pdev->dev, "Removing IPTS companion failed, "
"error: %d\n", ret);
}

return 0;
}

static const struct acpi_device_id ipts_surface_acpi_match[] = {
{ "MSHW0076", 0 }, /* Surface Book 1 / Surface Studio */
{ "MSHW0078", 0 }, /* Surface Pro 4 */
{ "MSHW0079", 0 }, /* Surface Laptop 1 / 2 */
{ "MSHW0101", 0 }, /* Surface Book 2 15" */
{ "MSHW0102", 0 }, /* Surface Pro 2017 / 6 */
{ "MSHW0103", 0 }, /* unknown, but firmware exists */
{ "MSHW0137", 0 }, /* Surface Book 2 */
{ "MSHW0076", 0 }, // Surface Book 1 / Surface Studio
{ "MSHW0078", 0 }, // Surface Pro 4
{ "MSHW0079", 0 }, // Surface Laptop 1 / 2
{ "MSHW0101", 0 }, // Surface Book 2 15"
{ "MSHW0102", 0 }, // Surface Pro 2017 / 6
{ "MSHW0103", 0 }, // unknown, but firmware exists
{ "MSHW0137", 0 }, // Surface Book 2
{ },
};
MODULE_DEVICE_TABLE(acpi, ipts_surface_acpi_match);
Expand All @@ -98,3 +121,11 @@ module_platform_driver(ipts_surface_driver);
MODULE_AUTHOR("Dorian Stoll <dorian.stoll@tmsp.io>");
MODULE_DESCRIPTION("IPTS companion driver for Microsoft Surface");
MODULE_LICENSE("GPL v2");

IPTS_SURFACE_FIRMWARE("MSHW0076");
IPTS_SURFACE_FIRMWARE("MSHW0078");
IPTS_SURFACE_FIRMWARE("MSHW0079");
IPTS_SURFACE_FIRMWARE("MSHW0101");
IPTS_SURFACE_FIRMWARE("MSHW0102");
IPTS_SURFACE_FIRMWARE("MSHW0103");
IPTS_SURFACE_FIRMWARE("MSHW0137");
203 changes: 203 additions & 0 deletions drivers/misc/ipts/ipts-companion.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
#include <linux/firmware.h>
#include <linux/ipts.h>
#include <linux/ipts-binary.h>
#include <linux/ipts-companion.h>
#include <linux/mutex.h>

#include "ipts.h"
#include "ipts-companion.h"
#include "ipts-params.h"

#define IPTS_FW_PATH_FMT "intel/ipts/%s"
#define IPTS_FW_CONFIG_FILE "ipts_fw_config.bin"

ipts_companion_t *ipts_companion;
DEFINE_MUTEX(ipts_companion_lock);

bool ipts_companion_available(void)
{
bool ret;
mutex_lock(&ipts_companion_lock);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason you picked mutex over spin here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


ret = ipts_companion != NULL;

mutex_unlock(&ipts_companion_lock);
return ret;
}

/*
* General purpose API for adding or removing a companion driver
* A companion driver is a driver that implements hardware specific
* behaviour into IPTS, so it doesn't have to be hardcoded into the
* main driver. All requests to the companion driver should be wrapped,
* with a fallback in case a companion driver cannot be found.
*/

int ipts_add_companion(ipts_companion_t *companion)
{
int ret = 0;
mutex_lock(&ipts_companion_lock);

if (ipts_companion != NULL) {
ret = -EBUSY;
goto add_companion_return;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

ipts_companion = companion;

add_companion_return:

mutex_unlock(&ipts_companion_lock);
return ret;
}
EXPORT_SYMBOL_GPL(ipts_add_companion);

int ipts_remove_companion(ipts_companion_t *companion)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

{
int ret = 0;
mutex_lock(&ipts_companion_lock);

if (ipts_companion == NULL || companion == NULL) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check companion != NULL before getting a mutex

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ret = 0;
goto remove_companion_return;
}

if (ipts_companion->name != companion->name) {
ret = -EPERM;
goto remove_companion_return;
}

ipts_companion = NULL;

remove_companion_return:

mutex_unlock(&ipts_companion_lock);
return ret;
}
EXPORT_SYMBOL_GPL(ipts_remove_companion);

/*
* Utility functions for IPTS. These functions replace codepaths in the IPTS
* driver, and redirect them to the companion driver, if one was found.
* Otherwise the legacy code gets executed as a fallback.
*/

int ipts_request_firmware(const struct firmware **fw, const char *name,
struct device *device)
{
int ret = 0;
char fw_path[MAX_IOCL_FILE_PATH_LEN];
mutex_lock(&ipts_companion_lock);

// Check if a companion was registered. If not, skip
// forward and try to load the firmware from the legacy path
if (ipts_companion == NULL || ipts_modparams.ignore_companion) {
goto request_firmware_fallback;
}

ret = ipts_companion->firmware_request(fw, name, device, ipts_companion);
if (!ret) {
goto request_firmware_return;
}

request_firmware_fallback:

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

// No firmware was found by the companion driver, try the generic path.
snprintf(fw_path, MAX_IOCL_FILE_PATH_LEN, IPTS_FW_PATH_FMT, name);
ret = request_firmware(fw, fw_path, device);

request_firmware_return:

mutex_unlock(&ipts_companion_lock);
return ret;
}

ipts_bin_fw_list_t *ipts_alloc_fw_list(ipts_bin_fw_info_t **fw)
qzed marked this conversation as resolved.
Show resolved Hide resolved
{
int size, len, i, j;
ipts_bin_fw_list_t *fw_list;
char *itr;

// Figure out the amount of firmware files inside of the array
len = 0;
while (fw[len] != NULL) {
len++;
}

// Determine the size that the final list will need in memory
size = sizeof(ipts_bin_fw_list_t);
for (i = 0; i < len; i++) {
size += sizeof(ipts_bin_fw_info_t);
size += sizeof(ipts_bin_data_file_info_t) *
fw[i]->num_of_data_files;
}

fw_list = kmalloc(size, GFP_KERNEL);
fw_list->num_of_fws = len;
itr = (char *)fw_list->fw_info;
for (i = 0; i < len; i++) {
*(ipts_bin_fw_info_t *)itr = *fw[i];
itr += sizeof(ipts_bin_fw_info_t);

for (j = 0; j < fw[i]->num_of_data_files; j++) {
*(ipts_bin_data_file_info_t *)itr = fw[i]->data_file[j];
itr += sizeof(ipts_bin_data_file_info_t);
}
}

return fw_list;
}

int ipts_request_firmware_config(ipts_info_t *ipts, ipts_bin_fw_list_t **cfg)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole function can be written better without any gotos

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
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) {
goto config_fallback;
}

if (ipts_companion->firmware_config != NULL) {
*cfg = ipts_alloc_fw_list(ipts_companion->firmware_config);
goto config_return;
}

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
mutex_unlock(&ipts_companion_lock);
ret = ipts_request_firmware(&config_fw, IPTS_FW_CONFIG_FILE,
&ipts->cldev->dev);
mutex_lock(&ipts_companion_lock);

if (!ret) {
*cfg = (ipts_bin_fw_list_t *)config_fw->data;
goto config_return;
}

release_firmware(config_fw);

config_return:

mutex_unlock(&ipts_companion_lock);
return ret;

}
15 changes: 15 additions & 0 deletions drivers/misc/ipts/ipts-companion.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#ifndef _IPTS_COMPANION_H_
#define _IPTS_COMPANION_H_

#include <linux/firmware.h>
#include <linux/ipts-binary.h>

#include "ipts.h"

bool ipts_companion_available(void);
int ipts_request_firmware(const struct firmware **fw, const char *name,
struct device *device);
int ipts_request_firmware_config(ipts_info_t *ipts,
ipts_bin_fw_list_t **firmware_config);

#endif // _IPTS_COMPANION_H_
Loading