linux-kernel - Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220516073329.GJ4009@kadam>
Date:   Mon, 16 May 2022 10:33:29 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Yaşar Arabacı <yasar11732@...il.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

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.

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.

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[];
>  };

regards,
dan carpenter