Date:   Mon, 16 May 2022 20:23:34 +0300
From:   Yaşar Arabacı <yasar11732@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     gregkh@...uxfoundation.org, paulo.miguel.almeida.rodenas@...il.com,
        alexandre.belloni@...tlin.com, realwakka@...il.com,
        u.kleine-koenig@...gutronix.de, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: pi433: Don't use ioctl for per-client
 configuration

Pzt, 2022-05-16 tarihinde 10:33 +0300 saatinde, Dan Carpenter yazdı:
> On Sun, May 15, 2022 at 01:47:11PM +0300, Yaşar Arabacı wrote:
> > Currently this driver uses ioctl for reading/writing per-device and
> > per-client configuration. Per-client configuration can be handled
> > by
> > usespace and sent to driver with each write() call. Doing so does
> > not
> > introduce extra overhead because we copy tx config to fifo for each
> > transmit anyway. This way, we don't have to introduce new ioctl's.
> > 
> > This has not been tested as I don't have access to hardware.
> > 
> > Signed-off-by: Yaşar Arabacı <yasar11732@...il.com>
> 
> This commit is confusing and does a number of unrelated things.  It's
> not explained well what the motivation is for the patch.
> 
> If I remember this correctly, the current API is broken.  It used a
> too small type or something?  People wanted fix it by making
> incompatible changes which would have broken user space.  I had said
> that the right thing would be to not using ioctls at all but instead
> use
> sysfs.
> 
> So I kind of remember that there was a motivation to get rid of the
> ioctl, but I don't remember what it was and it's not explained here.
> 

Motivation for this patch is also to get rid of ioctl in favor of more
suitable interfaces.

Currently, driver manages two different kinds of configurations. Per
device configuration is kept inside rx_cfg member of struct
pi433_device. These are parameters for recieving data. They are shared
by all clients using the device. Per-client configuration is kept under
private_data member of struct file. These parameters control
transmitting side of things. They are created for each call to open().

For reading and writing both kinds of configurations, ioctl commands
are used. It would be more fitting to use sysfs for per-device
configuration. On the other hand, using sysfs for per-client
congiguration feels wrong, as it is commonly used for global settings.
Therefore, user space is correct place to handle these in my opinion.
By using sysfs for per-device configuration, and letting user space
handle per-client configuration, we don't have to introduce new ioctl.
This patch does not touch per-device configuration case. It only
attempts to remove ioctl for per-client case.

Currently, when user calls write(), we write 3 things to tx queue.

- Per-client configuration (which is kept in kernel space)
- size of the payload (we get this from size of write call)
- payload itself (we get this from user space buffer)

My proposed solution is for user space to send all of these together
for each call to write() function. It should be trivial to implement
this behaviour as user space library function. This way, we don't have
to manage these on kernel, so it is one less thing to worry about.
Moreover, reading/writing configuration this way does not involve
seperate syscalls.

> I had imagined adding the sysfs configuration along side the ioctl to
> start with and then deleting the ioctl when userspace was updated. 
> If
> you're saying that we don't need any configuration at all then that's
> great but that has to come from someone who has tested the code.
> 

Configuration is still needed, just handled differently. I agree that
these should be tested. This patch could possibly be used as starting
point for anyone who has means/desire to test and experiment.

> What is this part of the commit for?
> 
> > --- a/drivers/staging/pi433/pi433_if.h
> > +++ b/drivers/staging/pi433/pi433_if.h
> > @@ -75,6 +75,8 @@ struct pi433_tx_cfg {
> >  
> >         __u8                    sync_pattern[8];
> >         __u8                    address_byte;
> > +       __u32                   payload_size;
> > +       __u8                    payload[];
> >  };
> 

Since my proposed implementation expects (config,payload_size,payload)
to be sent together with each write(), I added these here.

As a final note, replacing ioctl with something else would require
breaking changes at some point. If you think it is too much suffering
for too little to gain, I don't think it is absolutely necessary to do
so. But if people are interested I can make a better patch with less
unrelated changes and more explanations.

Best Regards,
Yaşar Arabacı

Powered by blists - more mailing lists