Using binary attributes for configuration sysfs entries

Using binary attributes for configuration sysfs entries

[Jay Aurabind]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

Hi,

Before I send this patch to actual mailing list, I'd appreciate if
someone could tell me if this is a bad idea!

The driver in staging for pi433 (a wireless transceiver) uses IOCTLs
at the moment. I wish to add a sysfs interface to it that control the
various transmission and reception parameters. In the ioctl interface,
it uses two structs that have about 40 parameters in total.

For the corresponding sysfs interface, since there are a lot of
parameters, would it be justified to use the same binary format though
sysfs_create_binary_file() ? The rationale is that it would be easier
to simply pack all the config options in the struct and send it in
once rather than individually write 40 files. This is what the
attached patch follows. Interface is added only for reception
parameters as of now.

Any others suggestions welcome!


-- 

Thanks and Regards,
Jay

[-- Attachment #2: 0001-staging-pi433-register-sysfs-i-f-for-rx-config.patch --]
[-- Type: text/x-patch, Size: 3340 bytes --]

From 926be3ade2b5769e035bf33ad6fb8ba2a228ce12 Mon Sep 17 00:00:00 2001
From: Aurabindo Jayamohanan <mail@aurabindo.in>
Date: Mon, 11 Mar 2019 16:12:48 +0530
Subject: [PATCH] staging: pi433: register sysfs i/f for rx config

Exposes a binary sysfs file for setting reception configuration
options for pi433. A binary interface is used since it would be
counterintuitive to manually set a dozen options individually.

Signed-off-by: Aurabindo Jayamohanan <mail@aurabindo.in>
---
 drivers/staging/pi433/pi433_if.c | 50 ++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index b2314636dc89..7f429b112a0a 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -25,6 +25,7 @@
 #include <linux/uaccess.h>
 #include <linux/fs.h>
 #include <linux/device.h>
+#include <linux/sysfs.h>
 #include <linux/cdev.h>
 #include <linux/err.h>
 #include <linux/kfifo.h>
@@ -1089,6 +1090,40 @@ static void pi433_free_minor(struct pi433_device *dev)
 	mutex_unlock(&minor_lock);
 }
 
+static ssize_t pi433_rx_cfg_read(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *bin_attr, char *buf,
+				loff_t off,
+				size_t count)
+{
+	struct pi433_device *pdev = bin_attr->private;
+	struct pi433_rx_cfg *src = pdev->rx_cfg;
+
+	if (count != bin_attr->size) {
+		return -EIO;
+	}
+
+	memcpy((void *)buf, (void *)src, bin_attr->size);
+
+	return bin_attr->size;
+}
+
+static ssize_t pi433_rx_cfg_write(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *bin_attr, char *buf,
+				loff_t off,
+				size_t count)
+{
+	struct pi433_device *pdev = bin_attr->private;
+	struct pi433_rx_cfg *dst = pdev->rx_cfg;
+
+	if (count != bin_attr->size) {
+		return -EIO;
+	}
+
+	memcpy((void *)dst, (void *)buf, bin_attr->size);
+
+	return bin_attr->size;
+}
+
 /*-------------------------------------------------------------------------*/
 
 static const struct file_operations pi433_fops = {
@@ -1107,6 +1142,8 @@ static const struct file_operations pi433_fops = {
 	.llseek =	no_llseek,
 };
 
+BIN_ATTR_RW(pi433_rx_cfg, sizeof(struct pi433_rx_cfg));
+
 /*-------------------------------------------------------------------------*/
 
 static int pi433_probe(struct spi_device *spi)
@@ -1243,6 +1280,15 @@ static int pi433_probe(struct spi_device *spi)
 		goto send_thread_failed;
 	}
 
+	/* create binary sysfs sentry for rx_cfg*/
+	bin_attr_pi433_rx_cfg.private = device;
+	retval = sysfs_create_bin_file(&device->dev->kobj,
+					&bin_attr_pi433_rx_cfg);
+	if (retval) {
+		dev_err(device->dev, "creation rx_cfg sysfs attribute failed");
+		goto sysfs_attr_failed;
+	}
+
 	/* create cdev */
 	device->cdev = cdev_alloc();
 	if (!device->cdev) {
@@ -1265,6 +1311,8 @@ static int pi433_probe(struct spi_device *spi)
 del_cdev:
 	cdev_del(device->cdev);
 cdev_failed:
+	sysfs_remove_bin_file(&device->dev->kobj, &bin_attr_pi433_rx_cfg);
+sysfs_attr_failed:
 	kthread_stop(device->tx_task_struct);
 send_thread_failed:
 	device_destroy(pi433_class, device->devt);
@@ -1284,6 +1332,8 @@ static int pi433_remove(struct spi_device *spi)
 {
 	struct pi433_device	*device = spi_get_drvdata(spi);
 
+	sysfs_remove_bin_file(&device->dev->kobj, &bin_attr_pi433_rx_cfg);
+
 	/* free GPIOs */
 	free_gpio(device);
 
-- 
2.19.1


[-- Attachment #3: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Using binary attributes for configuration sysfs entries

[valdis.kletnieks]

On Wed, 13 Mar 2019 12:02:02 +0530, Jay Aurabind said:

> For the corresponding sysfs interface, since there are a lot of
> parameters, would it be justified to use the same binary format though
> sysfs_create_binary_file() ? The rationale is that it would be easier
> to simply pack all the config options in the struct and send it in
> once rather than individually write 40 files. This is what the
> attached patch follows. Interface is added only for reception
> parameters as of now.

Sysfs has a rule of "one value per file" - and saying "the one value is the
config struct we read/write to the file" is stretching that quite a bit.   By
the time you're doing all the marshaling of values in and out of this struct,
there's no real benefit to do it via sysfs rather than the existing ioctl()
call.

Make sure you double-check that "the same binary format" means what you
think it does when considering 32/64 bit architectures. While you're there, double
check that the ioctl() works correctly for a 32-bit userspace program running
on a 64-bit kernel - we have historically had an incredible number of API botches
for that case.

Of course, it helps your case considerably if you can point at something else
in sysfs and say "The Foobar 9934 driver already does this".. :)

On the other hand, having 40 files is just a massive race condition waiting to
happen, especially if the device has hardware weirdness that imply that certain
sets of (say) 3 or 5 parameters have to be changed in unison.


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Using binary attributes for configuration sysfs entries

[Jay Aurabind]

Dear Valdis,

Thank you for the response. Please see my inline queries:

On Wed, 13 Mar 2019 at 12:26, <valdis.kletnieks@vt.edu> wrote:
>
> On Wed, 13 Mar 2019 12:02:02 +0530, Jay Aurabind said:
>
> > For the corresponding sysfs interface, since there are a lot of
> > parameters, would it be justified to use the same binary format though
> > sysfs_create_binary_file() ? The rationale is that it would be easier
> > to simply pack all the config options in the struct and send it in
> > once rather than individually write 40 files. This is what the
> > attached patch follows. Interface is added only for reception
> > parameters as of now.
>
> Sysfs has a rule of "one value per file" - and saying "the one value is the
> config struct we read/write to the file" is stretching that quite a bit.   By
> the time you're doing all the marshaling of values in and out of this struct,
> there's no real benefit to do it via sysfs rather than the existing ioctl()
> call.
>
> Make sure you double-check that "the same binary format" means what you
> think it does when considering 32/64 bit architectures. While you're there, double
> check that the ioctl() works correctly for a 32-bit userspace program running
> on a 64-bit kernel - we have historically had an incredible number of API botches
> for that case.

The driver already provides a compat_ioctl that handles 32-bit
processes. That should suffice? However the actual configuration
structure has lots of enums in it. It isnt explicitly aligned. What
would be an ideal alignment factor? I've seen some code explicitly do
8 byte align while some others do 16 byte alight using 'u8
reserved[x]'. I suppose __attribute__ ((aligned (x))) is a better
option. Does it need to be aligned at the first place?

But as long as the ioctl interface isnt removed, changing the
configuration structure's representation is a bad idea I suppose. So I
should probably go full sysfs and remove ioctl calls altogether?

>
> Of course, it helps your case considerably if you can point at something else
> in sysfs and say "The Foobar 9934 driver already does this".. :)
>

I suppose this driver will serve that purpose:
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sysfs.c#L1239
:)

> On the other hand, having 40 files is just a massive race condition waiting to
> happen, especially if the device has hardware weirdness that imply that certain
> sets of (say) 3 or 5 parameters have to be changed in unison.
>

The driver actually blocks upon a read (reception). So there does not
exist that possibility yet I think. But I should definitely check this
condition in sysfs_write so that I dont change the rx_config when
reception is active.


-- 

Thanks and Regards,
Jay

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Using binary attributes for configuration sysfs entries

[Greg KH]

On Wed, Mar 13, 2019 at 12:02:02PM +0530, Jay Aurabind wrote:
> Hi,
> 
> Before I send this patch to actual mailing list, I'd appreciate if
> someone could tell me if this is a bad idea!
> 
> The driver in staging for pi433 (a wireless transceiver) uses IOCTLs
> at the moment. I wish to add a sysfs interface to it that control the
> various transmission and reception parameters. In the ioctl interface,
> it uses two structs that have about 40 parameters in total.
> 
> For the corresponding sysfs interface, since there are a lot of
> parameters, would it be justified to use the same binary format though
> sysfs_create_binary_file() ? The rationale is that it would be easier
> to simply pack all the config options in the struct and send it in
> once rather than individually write 40 files. This is what the
> attached patch follows. Interface is added only for reception
> parameters as of now.

binary sysfs files are only allowed for "pass through" data, where the
kernel does not touch the information at all and only passes it from the
hardware, to userspace directly (or the other way around).  It can not
be used for data that the kernel actually knows about and modifies /
acts on.

An example of valid binary sysfs files are USB and PCI device
configuration information (read directly from the hardware), or firmware
files that are send from userspace directly to the hardware without the
kernel knowing what the data is.

You can't use a binary sysfs file for ioctl-like data, that's not
allowed, just use an ioctl for that.  Or better yet, use a common api
interface for it, to match the other types of devices.

hope this helps,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Using binary attributes for configuration sysfs entries

[Jay Aurabind]

On Wed, 13 Mar 2019 at 19:55, Greg KH <greg@kroah.com> wrote:
>
> On Wed, Mar 13, 2019 at 12:02:02PM +0530, Jay Aurabind wrote:
> > Hi,
> >
> > Before I send this patch to actual mailing list, I'd appreciate if
> > someone could tell me if this is a bad idea!
> >
> > The driver in staging for pi433 (a wireless transceiver) uses IOCTLs
> > at the moment. I wish to add a sysfs interface to it that control the
> > various transmission and reception parameters. In the ioctl interface,
> > it uses two structs that have about 40 parameters in total.
> >
> > For the corresponding sysfs interface, since there are a lot of
> > parameters, would it be justified to use the same binary format though
> > sysfs_create_binary_file() ? The rationale is that it would be easier
> > to simply pack all the config options in the struct and send it in
> > once rather than individually write 40 files. This is what the
> > attached patch follows. Interface is added only for reception
> > parameters as of now.
>
> binary sysfs files are only allowed for "pass through" data, where the
> kernel does not touch the information at all and only passes it from the
> hardware, to userspace directly (or the other way around).  It can not
> be used for data that the kernel actually knows about and modifies /
> acts on.
>
> An example of valid binary sysfs files are USB and PCI device
> configuration information (read directly from the hardware), or firmware
> files that are send from userspace directly to the hardware without the
> kernel knowing what the data is.
>
> You can't use a binary sysfs file for ioctl-like data, that's not
> allowed, just use an ioctl for that.  Or better yet, use a common api
> interface for it, to match the other types of devices.

Thank you for the explanation. What API would the most appropriate for
such a wireless transceiver ? If I could make it as a networking
driver, I could probably use setsockopt to pass in these
configuration. Is this recommended ?

>
> hope this helps,
>
> greg k-h


--

Thanks and Regards,
Jay

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Using binary attributes for configuration sysfs entries

[Greg KH]

On Thu, Mar 14, 2019 at 04:17:13PM +0530, Jay Aurabind wrote:
> On Wed, 13 Mar 2019 at 19:55, Greg KH <greg@kroah.com> wrote:
> >
> > On Wed, Mar 13, 2019 at 12:02:02PM +0530, Jay Aurabind wrote:
> > > Hi,
> > >
> > > Before I send this patch to actual mailing list, I'd appreciate if
> > > someone could tell me if this is a bad idea!
> > >
> > > The driver in staging for pi433 (a wireless transceiver) uses IOCTLs
> > > at the moment. I wish to add a sysfs interface to it that control the
> > > various transmission and reception parameters. In the ioctl interface,
> > > it uses two structs that have about 40 parameters in total.
> > >
> > > For the corresponding sysfs interface, since there are a lot of
> > > parameters, would it be justified to use the same binary format though
> > > sysfs_create_binary_file() ? The rationale is that it would be easier
> > > to simply pack all the config options in the struct and send it in
> > > once rather than individually write 40 files. This is what the
> > > attached patch follows. Interface is added only for reception
> > > parameters as of now.
> >
> > binary sysfs files are only allowed for "pass through" data, where the
> > kernel does not touch the information at all and only passes it from the
> > hardware, to userspace directly (or the other way around).  It can not
> > be used for data that the kernel actually knows about and modifies /
> > acts on.
> >
> > An example of valid binary sysfs files are USB and PCI device
> > configuration information (read directly from the hardware), or firmware
> > files that are send from userspace directly to the hardware without the
> > kernel knowing what the data is.
> >
> > You can't use a binary sysfs file for ioctl-like data, that's not
> > allowed, just use an ioctl for that.  Or better yet, use a common api
> > interface for it, to match the other types of devices.
> 
> Thank you for the explanation. What API would the most appropriate for
> such a wireless transceiver ? If I could make it as a networking
> driver, I could probably use setsockopt to pass in these
> configuration. Is this recommended ?

No, try looking at how v4l2 works, odds are they already have the
correct api for this type of device there for you to use.

thanks,

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Using binary attributes for configuration sysfs entries

[Jay Aurabind]

On Thu, 14 Mar 2019 at 20:27, Greg KH <greg@kroah.com> wrote:
>
> On Thu, Mar 14, 2019 at 04:17:13PM +0530, Jay Aurabind wrote:
> > On Wed, 13 Mar 2019 at 19:55, Greg KH <greg@kroah.com> wrote:
> > >
> > > On Wed, Mar 13, 2019 at 12:02:02PM +0530, Jay Aurabind wrote:
> > > > Hi,
> > > >
> > > > Before I send this patch to actual mailing list, I'd appreciate if
> > > > someone could tell me if this is a bad idea!
> > > >
> > > > The driver in staging for pi433 (a wireless transceiver) uses IOCTLs
> > > > at the moment. I wish to add a sysfs interface to it that control the
> > > > various transmission and reception parameters. In the ioctl interface,
> > > > it uses two structs that have about 40 parameters in total.
> > > >
> > > > For the corresponding sysfs interface, since there are a lot of
> > > > parameters, would it be justified to use the same binary format though
> > > > sysfs_create_binary_file() ? The rationale is that it would be easier
> > > > to simply pack all the config options in the struct and send it in
> > > > once rather than individually write 40 files. This is what the
> > > > attached patch follows. Interface is added only for reception
> > > > parameters as of now.
> > >
> > > binary sysfs files are only allowed for "pass through" data, where the
> > > kernel does not touch the information at all and only passes it from the
> > > hardware, to userspace directly (or the other way around).  It can not
> > > be used for data that the kernel actually knows about and modifies /
> > > acts on.
> > >
> > > An example of valid binary sysfs files are USB and PCI device
> > > configuration information (read directly from the hardware), or firmware
> > > files that are send from userspace directly to the hardware without the
> > > kernel knowing what the data is.
> > >
> > > You can't use a binary sysfs file for ioctl-like data, that's not
> > > allowed, just use an ioctl for that.  Or better yet, use a common api
> > > interface for it, to match the other types of devices.
> >
> > Thank you for the explanation. What API would the most appropriate for
> > such a wireless transceiver ? If I could make it as a networking
> > driver, I could probably use setsockopt to pass in these
> > configuration. Is this recommended ?
>
> No, try looking at how v4l2 works, odds are they already have the
> correct api for this type of device there for you to use.

Thank you very much for the information!
>
> thanks,
>
> greg k-h



-- 

Thanks and Regards,
Jay

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies