* [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
@ 2022-05-15 10:47 Yaşar Arabacı
2022-05-15 10:54 ` Greg KH
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Yaşar Arabacı @ 2022-05-15 10:47 UTC (permalink / raw)
To: gregkh
Cc: paulo.miguel.almeida.rodenas, dan.carpenter, alexandre.belloni,
realwakka, u.kleine-koenig, linux-staging, linux-kernel,
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
2022-05-15 10:47 [PATCH] Staging: pi433: Don't use ioctl for per-client configuration Yaşar Arabacı
@ 2022-05-15 10:54 ` Greg KH
2022-05-15 10:55 ` Greg KH
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2022-05-15 10:54 UTC (permalink / raw)
To: Yaşar Arabacı
Cc: paulo.miguel.almeida.rodenas, dan.carpenter, alexandre.belloni,
realwakka, u.kleine-koenig, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
2022-05-15 10:47 [PATCH] Staging: pi433: Don't use ioctl for per-client configuration Yaşar Arabacı
2022-05-15 10:54 ` Greg KH
@ 2022-05-15 10:55 ` Greg KH
2022-05-16 7:33 ` Dan Carpenter
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2022-05-15 10:55 UTC (permalink / raw)
To: Yaşar Arabacı
Cc: paulo.miguel.almeida.rodenas, dan.carpenter, alexandre.belloni,
realwakka, u.kleine-koenig, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
2022-05-15 10:47 [PATCH] Staging: pi433: Don't use ioctl for per-client configuration Yaşar Arabacı
2022-05-15 10:54 ` Greg KH
2022-05-15 10:55 ` Greg KH
@ 2022-05-16 7:33 ` Dan Carpenter
2022-05-16 17:23 ` Yaşar Arabacı
2022-05-16 11:23 ` kernel test robot
2022-05-16 15:30 ` kernel test robot
4 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2022-05-16 7:33 UTC (permalink / raw)
To: Yaşar Arabacı
Cc: gregkh, paulo.miguel.almeida.rodenas, alexandre.belloni,
realwakka, u.kleine-koenig, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
2022-05-15 10:47 [PATCH] Staging: pi433: Don't use ioctl for per-client configuration Yaşar Arabacı
` (2 preceding siblings ...)
2022-05-16 7:33 ` Dan Carpenter
@ 2022-05-16 11:23 ` kernel test robot
2022-05-16 15:30 ` kernel test robot
4 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-05-16 11:23 UTC (permalink / raw)
To: Yaşar Arabacı, gregkh
Cc: llvm, kbuild-all, paulo.miguel.almeida.rodenas, dan.carpenter,
alexandre.belloni, realwakka, u.kleine-koenig, linux-staging,
linux-kernel, Yaşar Arabacı
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
2022-05-15 10:47 [PATCH] Staging: pi433: Don't use ioctl for per-client configuration Yaşar Arabacı
` (3 preceding siblings ...)
2022-05-16 11:23 ` kernel test robot
@ 2022-05-16 15:30 ` kernel test robot
4 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2022-05-16 15:30 UTC (permalink / raw)
To: Yaşar Arabacı, gregkh
Cc: kbuild-all, paulo.miguel.almeida.rodenas, dan.carpenter,
alexandre.belloni, realwakka, u.kleine-koenig, linux-staging,
linux-kernel, Yaşar Arabacı
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Staging: pi433: Don't use ioctl for per-client configuration
2022-05-16 7:33 ` Dan Carpenter
@ 2022-05-16 17:23 ` Yaşar Arabacı
0 siblings, 0 replies; 7+ messages in thread
From: Yaşar Arabacı @ 2022-05-16 17:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh, paulo.miguel.almeida.rodenas, alexandre.belloni,
realwakka, u.kleine-koenig, linux-staging, linux-kernel
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ı
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-16 17:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-15 10:47 [PATCH] Staging: pi433: Don't use ioctl for per-client configuration Yaşar Arabacı
2022-05-15 10:54 ` Greg KH
2022-05-15 10:55 ` Greg KH
2022-05-16 7:33 ` Dan Carpenter
2022-05-16 17:23 ` Yaşar Arabacı
2022-05-16 11:23 ` kernel test robot
2022-05-16 15:30 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.