[1/2] uio: add ioctl to uio

Commit Message

Guixin Liu Feb. 17, 2022, 2:29 a.m. UTC

In TCMU, if backstore holds its own userspace buffer, for read cmd, the
data needs to be copied from userspace buffer to tcmu data area first,
and then needs to be copied from tcmu data area to scsi sgl pages again.

To solve this problem, add ioctl to uio to let userspace backstore can
copy data between scsi sgl pages and its own buffer directly.

Reviewed-by: Xiaoguang Wang <[e-mail]>
Signed-off-by: Guixin Liu <[e-mail]>
---
 drivers/uio/uio.c          | 22 ++++++++++++++++++++++
 include/linux/uio_driver.h |  1 +
 2 files changed, 23 insertions(+)

Comments

Greg KH Feb. 17, 2022, 6:13 a.m. UTC | #1

On Thu, Feb 17, 2022 at 10:29:21AM +0800, Guixin Liu wrote:
> In TCMU, if backstore holds its own userspace buffer, for read cmd, the
> data needs to be copied from userspace buffer to tcmu data area first,
> and then needs to be copied from tcmu data area to scsi sgl pages again.
> 
> To solve this problem, add ioctl to uio to let userspace backstore can
> copy data between scsi sgl pages and its own buffer directly.
> 
> Reviewed-by: Xiaoguang Wang <[e-mail]>
> Signed-off-by: Guixin Liu <[e-mail]>
> ---
>  drivers/uio/uio.c          | 22 ++++++++++++++++++++++
>  include/linux/uio_driver.h |  1 +

No, sorry, thie uio driver will not be adding ioctls to them.  If you
need an ioctl, then you should not be using the UIO api but rather use a
custom character driver instead.

thanks,

greg k-h

kernel test robot Feb. 17, 2022, 6:55 a.m. UTC | #2

Hi Guixin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on mkp-scsi/for-next linux/master linus/master v5.17-rc4 next-20220216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Guixin-Liu/uio-add-ioctl-to-uio/20220217-103120
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git e6cb9c167eeb8f90ab924666c573e69e85e700a0
config: arc-randconfig-r035-20220217 (https://download.01.org/0day-ci/archive/20220217/[e-mail]/config)
compiler: arc-elf-gcc (GCC) 11.2.0
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
        # https://github.com/0day-ci/linux/commit/8786f129af888d844bc38ed78db7a6788e45655c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Guixin-Liu/uio-add-ioctl-to-uio/20220217-103120
        git checkout 8786f129af888d844bc38ed78db7a6788e45655c
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/uio/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/uio/uio.c:818:6: warning: no previous prototype for 'uio_ioctl' [-Wmissing-prototypes]
     818 | long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
         |      ^~~~~~~~~


vim +/uio_ioctl +818 drivers/uio/uio.c

   817	
 > 818	long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
   819	{
   820		struct uio_listener *listener = filep->private_data;
   821		struct uio_device *idev = listener->dev;
   822		long retval = 0;
   823	
   824		mutex_lock(&idev->info_lock);
   825		if (!idev->info || !idev->info->ioctl) {
   826			retval = -EINVAL;
   827			goto out;
   828		}
   829	
   830		retval = idev->info->ioctl(idev->info, cmd, arg);
   831	out:
   832		mutex_unlock(&idev->info_lock);
   833		return retval;
   834	}
   835	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Greg KH Feb. 17, 2022, 12:30 p.m. UTC | #3

On Thu, Feb 17, 2022 at 08:15:29PM +0800, Xiaoguang Wang wrote:
> hi,
> 
> > On Thu, Feb 17, 2022 at 10:29:21AM +0800, Guixin Liu wrote:
> > > In TCMU, if backstore holds its own userspace buffer, for read cmd, the
> > > data needs to be copied from userspace buffer to tcmu data area first,
> > > and then needs to be copied from tcmu data area to scsi sgl pages again.
> > > 
> > > To solve this problem, add ioctl to uio to let userspace backstore can
> > > copy data between scsi sgl pages and its own buffer directly.
> > > 
> > > Reviewed-by: Xiaoguang Wang<[e-mail]>
> > > Signed-off-by: Guixin Liu<[e-mail]>
> > > ---
> > >   drivers/uio/uio.c          | 22 ++++++++++++++++++++++
> > >   include/linux/uio_driver.h |  1 +
> > No, sorry, thie uio driver will not be adding ioctls to them.  If you
> > need an ioctl, then you should not be using the UIO api but rather use a
> > custom character driver instead.
> 
> I found that early in 2015, there was developer trying to add ioctl interface
> to uio framework:https://lore.kernel.org/lkml/[e-mail]/

See, I rejected your ioctl for the very same reasons :)

It's good that I was consistent...

> Some of my customers use tcm_loop & tcmu to simulate block devices, it's tcmu
> driver that uses uio framework. There maybe more extra work if we tries to replace
> uio with a new character driver.

Why is tcmu using uio at all?

> Currently tcmu has performance bottleneck, Guixin's patch uses ioctl interface
> to bypass tcmu data area. I also have implemented a tcmu zero-copy feature that
> allows tcmu driver map io request sgl's pages to user space, which uses ioctl
> to do this mapping work, similar to network getsockopt(TCP_ZEROCOPY_RECEIVE).
> 
> I also understand your concerns about ioctl interface. Except that replacing
> uio with a new character driver in tcmu, are there any less complicated methods
> to complete our needs? Thanks.

I do not know what your needs are, nor what tcmu is, nor why you would
want to simulate a block device to userspace like this through the uio
api, sorry.

good luck!

greg k-h

Xiaoguang Wang Feb. 17, 2022, 12:30 p.m. UTC | #4

hi,

> On Thu, Feb 17, 2022 at 10:29:21AM +0800, Guixin Liu wrote:
>> In TCMU, if backstore holds its own userspace buffer, for read cmd, the
>> data needs to be copied from userspace buffer to tcmu data area first,
>> and then needs to be copied from tcmu data area to scsi sgl pages again.
>>
>> To solve this problem, add ioctl to uio to let userspace backstore can
>> copy data between scsi sgl pages and its own buffer directly.
>>
>> Reviewed-by: Xiaoguang Wang <[e-mail]>
>> Signed-off-by: Guixin Liu <[e-mail]>
>> ---
>>   drivers/uio/uio.c          | 22 ++++++++++++++++++++++
>>   include/linux/uio_driver.h |  1 +
> No, sorry, thie uio driver will not be adding ioctls to them.  If you
> need an ioctl, then you should not be using the UIO api but rather use a
> custom character driver instead.
I found that early in 2015, there was developer trying to add ioctl 
interface
to uio framework: 
https://lore.kernel.org/lkml/[e-mail]/

Some of my customers use tcm_loop & tcmu to simulate block devices, it's 
tcmu
driver that uses uio framework. There maybe more extra work if we tries 
to replace
uio with a new character driver.

Currently tcmu has performance bottleneck, Guixin's patch uses ioctl 
interface
to bypass tcmu data area. I also have implemented a tcmu zero-copy 
feature that
allows tcmu driver map io request sgl's pages to user space, which uses 
ioctl
to do this mapping work, similar to network 
getsockopt(TCP_ZEROCOPY_RECEIVE).

I also understand your concerns about ioctl interface. Except that replacing
uio with a new character driver in tcmu, are there any less complicated 
methods
to complete our needs? Thanks.

Regards,
Xiaoguang Wang
>
> thanks,
>
> greg k-h

Patch

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 43afbb7..0fb85a3 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -815,6 +815,24 @@  static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	return ret;
 }
 
+long uio_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
+{
+	struct uio_listener *listener = filep->private_data;
+	struct uio_device *idev = listener->dev;
+	long retval = 0;
+
+	mutex_lock(&idev->info_lock);
+	if (!idev->info || !idev->info->ioctl) {
+		retval = -EINVAL;
+		goto out;
+	}
+
+	retval = idev->info->ioctl(idev->info, cmd, arg);
+out:
+	mutex_unlock(&idev->info_lock);
+	return retval;
+}
+
 static const struct file_operations uio_fops = {
 	.owner		= THIS_MODULE,
 	.open		= uio_open,
@@ -825,6 +843,10 @@  static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	.poll		= uio_poll,
 	.fasync		= uio_fasync,
 	.llseek		= noop_llseek,
+	.unlocked_ioctl = uio_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl   = uio_ioctl,
+#endif
 };
 
 static int uio_major_init(void)
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 47c5962..971d172b 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -109,6 +109,7 @@  struct uio_info {
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+	long (*ioctl)(struct uio_info *info, unsigned int cmd, unsigned long arg);
 };
 
 extern int __must_check