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

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

[Yaşar Arabacı]

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;
 
-	/*
-	 * check if tx_cfg has been initialized otherwise we won't be able to
-	 * config the RF trasmitter correctly due to invalid settings
-	 */
-	if (!instance->tx_cfg_initialized) {
-		dev_notice_once(device->dev,
-				"write: failed due to unconfigured tx_cfg (see PI433_IOC_WR_TX_CFG)\n");
-		return -EINVAL;
-	}
+	if (unlikely(count < sizeof(struct pi433_tx_cfg)))
+		return -EMSGSIZE;
+
+	tx_cfg = (struct pi433_tx_cfg *)buf;
+
+	if (unlikely(count != sizeof(struct pi433_tx_cfg)) + tx_cfg->payload_size)
+		return -EMSGSIZE;
 
 	/*
 	 * write the following sequence into fifo:
@@ -843,24 +826,14 @@ pi433_write(struct file *filp, const char __user *buf,
 	 */
 	mutex_lock(&device->tx_fifo_lock);
 
-	required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
 	available = kfifo_avail(&device->tx_fifo);
-	if (required > available) {
+	if (count > available) {
 		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available\n",
 			required, available);
 		mutex_unlock(&device->tx_fifo_lock);
 		return -EAGAIN;
 	}
 
-	retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
-			  sizeof(instance->tx_cfg));
-	if (retval != sizeof(instance->tx_cfg))
-		goto abort;
-
-	retval = kfifo_in(&device->tx_fifo, &count, sizeof(size_t));
-	if (retval != sizeof(size_t))
-		goto abort;
-
 	retval = kfifo_from_user(&device->tx_fifo, buf, count, &copied);
 	if (retval || copied != count)
 		goto abort;
@@ -884,7 +857,6 @@ static long pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct pi433_instance	*instance;
 	struct pi433_device	*device;
-	struct pi433_tx_cfg	tx_cfg;
 	void __user *argp = (void __user *)arg;
 
 	/* Check type and command number */
@@ -898,19 +870,6 @@ static long pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		return -ESHUTDOWN;
 
 	switch (cmd) {
-	case PI433_IOC_RD_TX_CFG:
-		if (copy_to_user(argp, &instance->tx_cfg,
-				 sizeof(struct pi433_tx_cfg)))
-			return -EFAULT;
-		break;
-	case PI433_IOC_WR_TX_CFG:
-		if (copy_from_user(&tx_cfg, argp, sizeof(struct pi433_tx_cfg)))
-			return -EFAULT;
-		mutex_lock(&device->tx_fifo_lock);
-		memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
-		instance->tx_cfg_initialized = true;
-		mutex_unlock(&device->tx_fifo_lock);
-		break;
 	case PI433_IOC_RD_RX_CFG:
 		if (copy_to_user(argp, &device->rx_cfg,
 				 sizeof(struct pi433_rx_cfg)))
diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 25ee0b77a32c..5dd2a3c6e661 100644
--- 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[];
 };
 
 /**
@@ -135,11 +137,6 @@ struct pi433_rx_cfg {
 
 #define PI433_IOC_MAGIC	'r'
 
-#define PI433_IOC_RD_TX_CFG                                             \
-	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
-#define PI433_IOC_WR_TX_CFG                                             \
-	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
-
 #define PI433_IOC_RD_RX_CFG                                             \
 	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
 #define PI433_IOC_WR_RX_CFG                                             \
-- 
2.36.1

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

[Greg KH]

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

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

[Greg KH]

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.

Wait, you just changed the user api of the write/read call to the
driver?  That's dangerous, especially:

> This has not been tested as I don't have access to hardware.

That's not good.  You need to figure out what userspace tool(s) use this
api and ensure that you did not just break them for no good reason.

thanks,

greg k-h

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

[Dan Carpenter]

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.

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

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

[kernel test robot]

Hi "Yaşar,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Ya-ar-Arabac/Staging-pi433-Don-t-use-ioctl-for-per-client-configuration/20220515-185057
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git e41f7a5521d7f03dca99e3207633df71740569dd
config: riscv-randconfig-r036-20220516 (https://download.01.org/0day-ci/archive/20220516/202205161930.aGSjQp2u-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 853fa8ee225edf2d0de94b0dcbd31bea916e825e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/0cfbff215eb0e9e558af6b491d319fc736a927c6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ya-ar-Arabac/Staging-pi433-Don-t-use-ioctl-for-per-client-configuration/20220515-185057
        git checkout 0cfbff215eb0e9e558af6b491d319fc736a927c6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/staging/pi433/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/staging/pi433/pi433_if.c:832:4: warning: variable 'required' is uninitialized when used here [-Wuninitialized]
                           required, available);
                           ^~~~~~~~
   include/linux/dev_printk.h:155:39: note: expanded from macro 'dev_dbg'
           dynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)
                                                ^~~~~~~~~~~
   include/linux/dynamic_debug.h:167:19: note: expanded from macro 'dynamic_dev_dbg'
                              dev, fmt, ##__VA_ARGS__)
                                          ^~~~~~~~~~~
   include/linux/dynamic_debug.h:152:56: note: expanded from macro '_dynamic_func_call'
           __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
                                                                 ^~~~~~~~~~~
   include/linux/dynamic_debug.h:134:15: note: expanded from macro '__dynamic_func_call'
                   func(&id, ##__VA_ARGS__);               \
                               ^~~~~~~~~~~
   drivers/staging/pi433/pi433_if.c:801:24: note: initialize the variable 'required' to silence this warning
           unsigned int            required, available, copied;
                                           ^
                                            = 0
   1 warning generated.


vim +/required +832 drivers/staging/pi433/pi433_if.c

874bcba65f9a3a Marcus Wolf          2017-07-16  792  
874bcba65f9a3a Marcus Wolf          2017-07-16  793  static ssize_t
874bcba65f9a3a Marcus Wolf          2017-07-16  794  pi433_write(struct file *filp, const char __user *buf,
874bcba65f9a3a Marcus Wolf          2017-07-16  795  	    size_t count, loff_t *f_pos)
874bcba65f9a3a Marcus Wolf          2017-07-16  796  {
874bcba65f9a3a Marcus Wolf          2017-07-16  797  	struct pi433_instance	*instance;
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  798  	struct pi433_tx_cfg *tx_cfg;
874bcba65f9a3a Marcus Wolf          2017-07-16  799  	struct pi433_device	*device;
57f8965af417f9 Stefano Manni        2017-11-16  800  	int                     retval;
5451dab9b7f546 Valentin Vidic       2018-04-19  801  	unsigned int		required, available, copied;
874bcba65f9a3a Marcus Wolf          2017-07-16  802  
874bcba65f9a3a Marcus Wolf          2017-07-16  803  	instance = filp->private_data;
874bcba65f9a3a Marcus Wolf          2017-07-16  804  	device = instance->device;
874bcba65f9a3a Marcus Wolf          2017-07-16  805  
63688e61d5629c Sophie Matter        2018-07-11  806  	/*
63688e61d5629c Sophie Matter        2018-07-11  807  	 * check, whether internal buffer (tx thread) is big enough
63688e61d5629c Sophie Matter        2018-07-11  808  	 * for requested size
63688e61d5629c Sophie Matter        2018-07-11  809  	 */
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  810  	if (unlikely(count > MAX_MSG_SIZE))
874bcba65f9a3a Marcus Wolf          2017-07-16  811  		return -EMSGSIZE;
874bcba65f9a3a Marcus Wolf          2017-07-16  812  
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  813  	if (unlikely(count < sizeof(struct pi433_tx_cfg)))
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  814  		return -EMSGSIZE;
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  815  
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  816  	tx_cfg = (struct pi433_tx_cfg *)buf;
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  817  
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  818  	if (unlikely(count != sizeof(struct pi433_tx_cfg)) + tx_cfg->payload_size)
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  819  		return -EMSGSIZE;
ce514dadc61a53 Paulo Miguel Almeida 2022-01-15  820  
63688e61d5629c Sophie Matter        2018-07-11  821  	/*
63688e61d5629c Sophie Matter        2018-07-11  822  	 * write the following sequence into fifo:
056eeda2f9e637 Derek Robson         2017-07-22  823  	 * - tx_cfg
056eeda2f9e637 Derek Robson         2017-07-22  824  	 * - size of message
056eeda2f9e637 Derek Robson         2017-07-22  825  	 * - message
056eeda2f9e637 Derek Robson         2017-07-22  826  	 */
874bcba65f9a3a Marcus Wolf          2017-07-16  827  	mutex_lock(&device->tx_fifo_lock);
5451dab9b7f546 Valentin Vidic       2018-04-19  828  
5451dab9b7f546 Valentin Vidic       2018-04-19  829  	available = kfifo_avail(&device->tx_fifo);
0cfbff215eb0e9 Yaşar Arabacı        2022-05-15  830  	if (count > available) {
1b6a6147374eb3 Paulo Miguel Almeida 2022-02-07  831  		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available\n",
5451dab9b7f546 Valentin Vidic       2018-04-19 @832  			required, available);
5451dab9b7f546 Valentin Vidic       2018-04-19  833  		mutex_unlock(&device->tx_fifo_lock);
5451dab9b7f546 Valentin Vidic       2018-04-19  834  		return -EAGAIN;
5451dab9b7f546 Valentin Vidic       2018-04-19  835  	}
5451dab9b7f546 Valentin Vidic       2018-04-19  836  
874bcba65f9a3a Marcus Wolf          2017-07-16  837  	retval = kfifo_from_user(&device->tx_fifo, buf, count, &copied);
874bcba65f9a3a Marcus Wolf          2017-07-16  838  	if (retval || copied != count)
874bcba65f9a3a Marcus Wolf          2017-07-16  839  		goto abort;
874bcba65f9a3a Marcus Wolf          2017-07-16  840  
874bcba65f9a3a Marcus Wolf          2017-07-16  841  	mutex_unlock(&device->tx_fifo_lock);
874bcba65f9a3a Marcus Wolf          2017-07-16  842  
874bcba65f9a3a Marcus Wolf          2017-07-16  843  	/* start transfer */
874bcba65f9a3a Marcus Wolf          2017-07-16  844  	wake_up_interruptible(&device->tx_wait_queue);
1b6a6147374eb3 Paulo Miguel Almeida 2022-02-07  845  	dev_dbg(device->dev, "write: generated new msg with %d bytes.\n", copied);
874bcba65f9a3a Marcus Wolf          2017-07-16  846  
dd1114693bcc7d Oliver Graute        2017-12-19  847  	return copied;
874bcba65f9a3a Marcus Wolf          2017-07-16  848  
874bcba65f9a3a Marcus Wolf          2017-07-16  849  abort:
5451dab9b7f546 Valentin Vidic       2018-04-19  850  	dev_warn(device->dev,
1b6a6147374eb3 Paulo Miguel Almeida 2022-02-07  851  		 "write to fifo failed, non recoverable: 0x%x\n", retval);
874bcba65f9a3a Marcus Wolf          2017-07-16  852  	mutex_unlock(&device->tx_fifo_lock);
874bcba65f9a3a Marcus Wolf          2017-07-16  853  	return -EAGAIN;
874bcba65f9a3a Marcus Wolf          2017-07-16  854  }
874bcba65f9a3a Marcus Wolf          2017-07-16  855  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi "Yaşar,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/intel-lab-lkp/linux/commits/Ya-ar-Arabac/Staging-pi433-Don-t-use-ioctl-for-per-client-configuration/20220515-185057
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git e41f7a5521d7f03dca99e3207633df71740569dd
config: x86_64-randconfig-s022-20220516 (https://download.01.org/0day-ci/archive/20220516/202205162305.tBuUiz79-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/intel-lab-lkp/linux/commit/0cfbff215eb0e9e558af6b491d319fc736a927c6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ya-ar-Arabac/Staging-pi433-Don-t-use-ioctl-for-per-client-configuration/20220515-185057
        git checkout 0cfbff215eb0e9e558af6b491d319fc736a927c6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/staging/pi433/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/staging/pi433/pi433_if.c:816:19: sparse: sparse: cast removes address space '__user' of expression

vim +/__user +816 drivers/staging/pi433/pi433_if.c

   792	
   793	static ssize_t
   794	pi433_write(struct file *filp, const char __user *buf,
   795		    size_t count, loff_t *f_pos)
   796	{
   797		struct pi433_instance	*instance;
   798		struct pi433_tx_cfg *tx_cfg;
   799		struct pi433_device	*device;
   800		int                     retval;
   801		unsigned int		required, available, copied;
   802	
   803		instance = filp->private_data;
   804		device = instance->device;
   805	
   806		/*
   807		 * check, whether internal buffer (tx thread) is big enough
   808		 * for requested size
   809		 */
   810		if (unlikely(count > MAX_MSG_SIZE))
   811			return -EMSGSIZE;
   812	
   813		if (unlikely(count < sizeof(struct pi433_tx_cfg)))
   814			return -EMSGSIZE;
   815	
 > 816		tx_cfg = (struct pi433_tx_cfg *)buf;
   817	
   818		if (unlikely(count != sizeof(struct pi433_tx_cfg)) + tx_cfg->payload_size)
   819			return -EMSGSIZE;
   820	
   821		/*
   822		 * write the following sequence into fifo:
   823		 * - tx_cfg
   824		 * - size of message
   825		 * - message
   826		 */
   827		mutex_lock(&device->tx_fifo_lock);
   828	
   829		available = kfifo_avail(&device->tx_fifo);
   830		if (count > available) {
   831			dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available\n",
   832				required, available);
   833			mutex_unlock(&device->tx_fifo_lock);
   834			return -EAGAIN;
   835		}
   836	
   837		retval = kfifo_from_user(&device->tx_fifo, buf, count, &copied);
   838		if (retval || copied != count)
   839			goto abort;
   840	
   841		mutex_unlock(&device->tx_fifo_lock);
   842	
   843		/* start transfer */
   844		wake_up_interruptible(&device->tx_wait_queue);
   845		dev_dbg(device->dev, "write: generated new msg with %d bytes.\n", copied);
   846	
   847		return copied;
   848	
   849	abort:
   850		dev_warn(device->dev,
   851			 "write to fifo failed, non recoverable: 0x%x\n", retval);
   852		mutex_unlock(&device->tx_fifo_lock);
   853		return -EAGAIN;
   854	}
   855	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Yaşar Arabacı]

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ı