lkml.org 
[lkml]   [2022]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
    From
    Date
    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@gmail.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ı

    \
     
     \ /
      Last update: 2022-05-16 19:24    [W:4.409 / U:1.388 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site