On Fri, 10 Feb 2023 21:23:15 +0100 Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 2/10/23 21:15, Hans de Goede wrote:
Hi,
On 2/10/23 05:48, Orlando Chamberlain wrote:
Allow reading gmux ports from userspace. When the unsafe module parameter allow_user_writes is true, writing 1 byte values is also allowed.
For example:
cd /sys/bus/acpi/devices/APP000B:00/physical_node/ echo 4 > gmux_selected_port cat gmux_selected_port_data | xxd -p
Will show the gmux version information (00000005 in this case)
Please use debugfs for this and as part of the conversion drop the #ifdef-s (debugfs has stubs for when not enabled) and drop all the error checking of creating the files, debugfs is deliberately designed to not have any error checking in the setup / teardown code.
This also removes the need for the allow_user_writes parameter replacing it with the new kernel lockdown mechanism. debugfs will automatically block access to writable files when the kernel is in lockdown mode.
I'll change it to use debugfs instead of sysfs in v2.
Regards,
Hans
p.s.
I just realized I forgot my usual thank you for contributing to the kernel reply to the cover letter before diving into the review (oops).
So let me correct that: thank you very much for your work on this!
thank you for maintaining and reviewing!
Regards,
Hans
Signed-off-by: Orlando Chamberlain orlandoch.dev@gmail.com
drivers/platform/x86/apple-gmux.c | 129 ++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+)
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index c38d6ef0c15a..756059d48393 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -66,6 +66,11 @@ struct apple_gmux_data { enum vga_switcheroo_client_id switch_state_external; enum vga_switcheroo_state power_state; struct completion powerchange_done;
+#ifdef CONFIG_SYSFS
- /* sysfs data */
- int selected_port;
+#endif /* CONFIG_SYSFS */ };
static struct apple_gmux_data *apple_gmux_data; @@ -651,6 +656,121 @@ static void gmux_notify_handler(acpi_handle device, u32 value, void *context) complete(&gmux_data->powerchange_done); }
+/**
- DOC: Sysfs Interface
- gmux ports can be read from userspace as a sysfs interface.
For example:
- # echo 4 >
/sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port
- # cat
/sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port_data | xxd -p
- 00000005
- Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
- Single byte writes are also supported, however this must be
enabled with the
- unsafe allow_user_writes module parameter.
- */
+#ifdef CONFIG_SYSFS
+static bool allow_user_writes; +module_param_unsafe(allow_user_writes, bool, 0); +MODULE_PARM_DESC(allow_user_writes, "Allow userspace to write to gmux ports (default: false) (bool)"); + +static ssize_t gmux_selected_port_store(struct device *dev,
struct device_attribute *attr, const char*sysfsbuf, size_t count) +{
- struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
- u8 port;
- if (kstrtou8(sysfsbuf, 10, &port) < 0)
return -EINVAL;- /* On pio gmux's, make sure the user doesn't access too
high of a port. */
- if ((gmux_data->config == &apple_gmux_pio) &&
port > (gmux_data->iolen - 4))return -EINVAL;- gmux_data->selected_port = port;
- return count;
+}
+static ssize_t gmux_selected_port_show(struct device *dev,
struct device_attribute *attr, char *sysfsbuf)+{
- struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
- return sysfs_emit(sysfsbuf, "%d\n",
gmux_data->selected_port); +}
+DEVICE_ATTR_RW(gmux_selected_port);
+static ssize_t gmux_selected_port_data_store(struct device *dev,
struct device_attribute *attr, const char*sysfsbuf, size_t count) +{
- struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
- if (count == 1)
gmux_write8(gmux_data, gmux_data->selected_port,*sysfsbuf);
- else
return -EINVAL;- return count;
+}
+static ssize_t gmux_selected_port_data_show(struct device *dev,
struct device_attribute *attr, char *sysfsbuf)+{
- struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
- u32 data;
- data = gmux_read32(gmux_data, gmux_data->selected_port);
- memcpy(sysfsbuf, &data, sizeof(data));
- return sizeof(data);
+}
+struct device_attribute dev_attr_gmux_selected_port_data_rw = __ATTR_RW(gmux_selected_port_data); +struct device_attribute dev_attr_gmux_selected_port_data_ro = __ATTR_RO(gmux_selected_port_data); + +static int gmux_init_sysfs(struct pnp_dev *pnp) +{
- int ret;
- ret = device_create_file(&pnp->dev,
&dev_attr_gmux_selected_port);
- if (ret)
return ret;- if (allow_user_writes)
ret = device_create_file(&pnp->dev,&dev_attr_gmux_selected_port_data_rw);
- else
ret = device_create_file(&pnp->dev,&dev_attr_gmux_selected_port_data_ro);
- if (ret)
device_remove_file(&pnp->dev,&dev_attr_gmux_selected_port);
- return ret;
+}
+static void gmux_fini_sysfs(struct pnp_dev *pnp) +{
- device_remove_file(&pnp->dev,
&dev_attr_gmux_selected_port);
- if (allow_user_writes)
device_remove_file(&pnp->dev,&dev_attr_gmux_selected_port_data_rw);
- else
device_remove_file(&pnp->dev,&dev_attr_gmux_selected_port_data_ro); +}
+#else
+static int gmux_init_sysfs(struct pnp_dev *pnp) +{
- return 0;
+} +static void gmux_fini_sysfs(struct pnp_dev *pnp) +{ +}
+#endif /* CONFIG_SYSFS */
static int gmux_suspend(struct device *dev) { struct pnp_dev *pnp = to_pnp_dev(dev); @@ -846,8 +966,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) goto err_register_handler; }
- ret = gmux_init_sysfs(pnp);
- if (ret) {
pr_err("Failed to register gmux sysfs entries\n");goto err_sysfs;- }
- return 0;
+err_sysfs:
- vga_switcheroo_unregister_handler();
err_register_handler: gmux_disable_interrupts(gmux_data); apple_gmux_data = NULL; @@ -877,6 +1005,7 @@ static void gmux_remove(struct pnp_dev *pnp) { struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
- gmux_fini_sysfs(pnp); vga_switcheroo_unregister_handler(); gmux_disable_interrupts(gmux_data); if (gmux_data->gpe >= 0) {