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]

Date:   Sun, 15 May 2022 12:54:35 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Yaşar Arabacı <yasar11732@...il.com>
Cc:     paulo.miguel.almeida.rodenas@...il.com, dan.carpenter@...cle.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>
> ---
>  drivers/staging/pi433/pi433_if.c | 63 ++++++--------------------------
>  drivers/staging/pi433/pi433_if.h |  7 +---
>  2 files changed, 13 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 941aaa7eab2e..07cd9054560a 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -109,10 +109,6 @@ struct pi433_device {
>  
>  struct pi433_instance {
>  	struct pi433_device	*device;
> -	struct pi433_tx_cfg	tx_cfg;
> -
> -	/* control flags */
> -	bool			tx_cfg_initialized;
>  };
>  
>  /*-------------------------------------------------------------------------*/
> @@ -574,12 +570,6 @@ static int pi433_tx_thread(void *data)
>  		if (kthread_should_stop())
>  			return 0;
>  
> -		/*
> -		 * get data from fifo in the following order:
> -		 * - tx_cfg
> -		 * - size of message
> -		 * - message
> -		 */
>  		retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
>  		if (retval != sizeof(tx_cfg)) {
>  			dev_dbg(device->dev,
> @@ -588,13 +578,7 @@ static int pi433_tx_thread(void *data)
>  			continue;
>  		}
>  
> -		retval = kfifo_out(&device->tx_fifo, &size, sizeof(size_t));
> -		if (retval != sizeof(size_t)) {
> -			dev_dbg(device->dev,
> -				"reading msg size from fifo failed: got %d, expected %d\n",
> -				retval, (unsigned int)sizeof(size_t));
> -			continue;
> -		}
> +		size = tx_cfg.payload_size;
>  
>  		/* use fixed message length, if requested */
>  		if (tx_cfg.fixed_message_length != 0)
> @@ -811,6 +795,7 @@ pi433_write(struct file *filp, const char __user *buf,
>  	    size_t count, loff_t *f_pos)
>  {
>  	struct pi433_instance	*instance;
> +	struct pi433_tx_cfg *tx_cfg;
>  	struct pi433_device	*device;
>  	int                     retval;
>  	unsigned int		required, available, copied;
> @@ -822,18 +807,16 @@ pi433_write(struct file *filp, const char __user *buf,
>  	 * check, whether internal buffer (tx thread) is big enough
>  	 * for requested size
>  	 */
> -	if (count > MAX_MSG_SIZE)
> +	if (unlikely(count > MAX_MSG_SIZE))
>  		return -EMSGSIZE;

Unless you can benchmark the difference, NEVER use likely/unlikely as
the compiler and CPU almost always do a better job than humans in
figuring this stuff out.

Also it's an unrelated change to what you said this commit was going to
do, please don't do that.

thanks,

greg k-h