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

[Greg KH <gregkh@linuxfoundation.org>]

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>
> ---
>  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