Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240765 > unrolled thread

[PATCH v5 1/4] uio: add ioctl support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-06 19:20 +0200
    Re: [PATCH v5 1/4] uio: add ioctl support Stephen Hemminger <stephen@networkplumber.org> - 2015-10-06 23:40 +0200
      Re: [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:20 +0200
        Re: [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 10:20 +0200
    Re: [PATCH v5 1/4] uio: add ioctl support Greg KH <gregkh@linuxfoundation.org> - 2015-10-07 10:50 +0200
      Re: [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 11:00 +0200
        Re: [PATCH v5 1/4] uio: add ioctl support Greg KH <gregkh@linuxfoundation.org> - 2015-10-07 19:30 +0200
          Re: [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 19:30 +0200
            Re: [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-07 21:10 +0200
              Re: [PATCH v5 1/4] uio: add ioctl support Vlad Zolotarov <vladz@cloudius-systems.com> - 2015-10-08 14:50 +0200
                Re: [PATCH v5 1/4] uio: add ioctl support Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2015-10-08 17:40 +0200

#1240765 — [PATCH v5 1/4] uio: add ioctl support

Add the ability for underlying device drivers to register the ioctl
commands. This is useful when some interaction with the user space
beyond sysfs capabilities is required, e.g. query the interrupt mode
or bind eventfd to interrupt notifications (similarly to vfio ioctl
VFIO_DEVICE_SET_IRQS).

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 drivers/uio/uio.c          | 15 +++++++++++++++
 include/linux/uio_driver.h |  3 +++
 2 files changed, 18 insertions(+)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 8196581..714b0e5 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	}
 }
 
+static 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;
+
+	if (!idev->info)
+		return -EIO;
+
+	if (!idev->info->ioctl)
+		return -ENOTTY;
+
+	return idev->info->ioctl(idev->info, cmd, arg);
+}
+
 static const struct file_operations uio_fops = {
 	.owner		= THIS_MODULE,
 	.open		= uio_open,
@@ -712,6 +726,7 @@ static const struct file_operations uio_fops = {
 	.write		= uio_write,
 	.mmap		= uio_mmap,
 	.poll		= uio_poll,
+	.unlocked_ioctl	= uio_ioctl,
 	.fasync		= uio_fasync,
 	.llseek		= noop_llseek,
 };
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83..10d7833 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -89,6 +89,7 @@ struct uio_device {
  * @mmap:		mmap operation for this uio device
  * @open:		open operation for this uio device
  * @release:		release operation for this uio device
+ * @ioctl:		ioctl handler
  * @irqcontrol:		disable/enable irqs when 0/1 is written to /dev/uioX
  */
 struct uio_info {
@@ -105,6 +106,8 @@ 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);
+	int (*ioctl)(struct uio_info *info, unsigned int cmd,
+		     unsigned long arg);
 };
 
 extern int __must_check
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1241016

On Tue,  6 Oct 2015 20:17:36 +0300
Vlad Zolotarov <vladz@cloudius-systems.com> wrote:

> Add the ability for underlying device drivers to register the ioctl
> commands. This is useful when some interaction with the user space
> beyond sysfs capabilities is required, e.g. query the interrupt mode
> or bind eventfd to interrupt notifications (similarly to vfio ioctl
> VFIO_DEVICE_SET_IRQS).
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>

After discussions on and off list, the idea of an ioctl interface
is just not going to be accepted upstream because it can be abused.
Therefore another API will be necessary.

Ps: since I did most of this first, I am surprised you never gave
any attribution for the earlier work.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241224

On 10/07/15 00:33, Stephen Hemminger wrote:
> On Tue,  6 Oct 2015 20:17:36 +0300
> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>
>> Add the ability for underlying device drivers to register the ioctl
>> commands. This is useful when some interaction with the user space
>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>> VFIO_DEVICE_SET_IRQS).
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> After discussions on and off list, the idea of an ioctl interface
> is just not going to be accepted upstream because it can be abused.
> Therefore another API will be necessary.

I'm open for ideas. So far ioctl seems like the most appropriate 
candidate for the job... ;)

>
> Ps: since I did most of this first,

this particular patch and parts of SET_IRQ code - yes. But that would be 
all. Let's just be specific... ;)
This still doesn't mean that u don't deserve a proper credit for that. 
;) But there was a reason why I didn't do that. See below.
> I am surprised you never gave
> any attribution for the earlier work.

If memory serves me well I asked u about the "attribution" a few days 
ago of the dpdk-dev list but got no answer.
Therefore I took no steps in this regard since it left me under the 
impression that u just didn't want it. However it seems now
that that's not the case... ;)

Pls., let me know if u want me to mention that this patch and SET_IRQ 
ioctl code is based on your patches on dpdk-dev list and if yes, in which
form: just mentioning it in the patch description or putting your 
signed-off to the patch(es).

thanks,
vlad

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241225

On 10/07/15 11:17, Vlad Zolotarov wrote:
>
>
> On 10/07/15 00:33, Stephen Hemminger wrote:
>> On Tue,  6 Oct 2015 20:17:36 +0300
>> Vlad Zolotarov <vladz@cloudius-systems.com> wrote:
>>
>>> Add the ability for underlying device drivers to register the ioctl
>>> commands. This is useful when some interaction with the user space
>>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>>> VFIO_DEVICE_SET_IRQS).
>>>
>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> After discussions on and off list, the idea of an ioctl interface
>> is just not going to be accepted upstream because it can be abused.
>> Therefore another API will be necessary.
>
> I'm open for ideas. So far ioctl seems like the most appropriate 
> candidate for the job... ;)
>
>>
>> Ps: since I did most of this first,
>
> this particular patch and parts of SET_IRQ code - yes. But that would 
> be all. Let's just be specific... ;)

Pardon - mapping bars was also snitched from your patches... ;)

> This still doesn't mean that u don't deserve a proper credit for that. 
> ;) But there was a reason why I didn't do that. See below.
>> I am surprised you never gave
>> any attribution for the earlier work.
>
> If memory serves me well I asked u about the "attribution" a few days 
> ago of the dpdk-dev list but got no answer.
> Therefore I took no steps in this regard since it left me under the 
> impression that u just didn't want it. However it seems now
> that that's not the case... ;)
>
> Pls., let me know if u want me to mention that this patch and SET_IRQ 
> ioctl code is based on your patches on dpdk-dev list and if yes, in which
> form: just mentioning it in the patch description or putting your 
> signed-off to the patch(es).
>
> thanks,
> vlad
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241238

On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
> Add the ability for underlying device drivers to register the ioctl
> commands. This is useful when some interaction with the user space
> beyond sysfs capabilities is required, e.g. query the interrupt mode
> or bind eventfd to interrupt notifications (similarly to vfio ioctl
> VFIO_DEVICE_SET_IRQS).
> 
> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> ---
>  drivers/uio/uio.c          | 15 +++++++++++++++
>  include/linux/uio_driver.h |  3 +++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 8196581..714b0e5 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>  	}
>  }
>  
> +static 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;
> +
> +	if (!idev->info)
> +		return -EIO;
> +
> +	if (!idev->info->ioctl)
> +		return -ENOTTY;
> +
> +	return idev->info->ioctl(idev->info, cmd, arg);
> +}

As Stephen said, I will not take this, sorry.  It opens up the ability
to add "new system calls" to a huge range of crappy drivers, it's
something that vendors have been trying to push for years and is
something that I will not allow.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241243

On 10/07/15 11:46, Greg KH wrote:
> On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
>> Add the ability for underlying device drivers to register the ioctl
>> commands. This is useful when some interaction with the user space
>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>> VFIO_DEVICE_SET_IRQS).
>>
>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>> ---
>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>   include/linux/uio_driver.h |  3 +++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> index 8196581..714b0e5 100644
>> --- a/drivers/uio/uio.c
>> +++ b/drivers/uio/uio.c
>> @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>>   	}
>>   }
>>   
>> +static 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;
>> +
>> +	if (!idev->info)
>> +		return -EIO;
>> +
>> +	if (!idev->info->ioctl)
>> +		return -ENOTTY;
>> +
>> +	return idev->info->ioctl(idev->info, cmd, arg);
>> +}
> As Stephen said, I will not take this, sorry.  It opens up the ability
> to add "new system calls" to a huge range of crappy drivers, it's
> something that vendors have been trying to push for years and is
> something that I will not allow.

Ok. Another alternative could be to add new sysfs attributes for the 
MSI-X functionality similarly to what is done with "maps".
Would it be acceptable?

>
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241701

On Wed, Oct 07, 2015 at 11:55:01AM +0300, Vlad Zolotarov wrote:
> 
> 
> On 10/07/15 11:46, Greg KH wrote:
> >On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
> >>Add the ability for underlying device drivers to register the ioctl
> >>commands. This is useful when some interaction with the user space
> >>beyond sysfs capabilities is required, e.g. query the interrupt mode
> >>or bind eventfd to interrupt notifications (similarly to vfio ioctl
> >>VFIO_DEVICE_SET_IRQS).
> >>
> >>Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> >>---
> >>  drivers/uio/uio.c          | 15 +++++++++++++++
> >>  include/linux/uio_driver.h |  3 +++
> >>  2 files changed, 18 insertions(+)
> >>
> >>diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >>index 8196581..714b0e5 100644
> >>--- a/drivers/uio/uio.c
> >>+++ b/drivers/uio/uio.c
> >>@@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
> >>  	}
> >>  }
> >>+static 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;
> >>+
> >>+	if (!idev->info)
> >>+		return -EIO;
> >>+
> >>+	if (!idev->info->ioctl)
> >>+		return -ENOTTY;
> >>+
> >>+	return idev->info->ioctl(idev->info, cmd, arg);
> >>+}
> >As Stephen said, I will not take this, sorry.  It opens up the ability
> >to add "new system calls" to a huge range of crappy drivers, it's
> >something that vendors have been trying to push for years and is
> >something that I will not allow.
> 
> Ok. Another alternative could be to add new sysfs attributes for the MSI-X
> functionality similarly to what is done with "maps".
> Would it be acceptable?

If you get everyone else here to agree that this is the interface you
all are going to be using, sure.  All I care is that you not add ioctl
to the UIO interface.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241702

On 10/07/15 20:21, Greg KH wrote:
> On Wed, Oct 07, 2015 at 11:55:01AM +0300, Vlad Zolotarov wrote:
>>
>> On 10/07/15 11:46, Greg KH wrote:
>>> On Tue, Oct 06, 2015 at 08:17:36PM +0300, Vlad Zolotarov wrote:
>>>> Add the ability for underlying device drivers to register the ioctl
>>>> commands. This is useful when some interaction with the user space
>>>> beyond sysfs capabilities is required, e.g. query the interrupt mode
>>>> or bind eventfd to interrupt notifications (similarly to vfio ioctl
>>>> VFIO_DEVICE_SET_IRQS).
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>   drivers/uio/uio.c          | 15 +++++++++++++++
>>>>   include/linux/uio_driver.h |  3 +++
>>>>   2 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>>>> index 8196581..714b0e5 100644
>>>> --- a/drivers/uio/uio.c
>>>> +++ b/drivers/uio/uio.c
>>>> @@ -704,6 +704,20 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
>>>>   	}
>>>>   }
>>>> +static 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;
>>>> +
>>>> +	if (!idev->info)
>>>> +		return -EIO;
>>>> +
>>>> +	if (!idev->info->ioctl)
>>>> +		return -ENOTTY;
>>>> +
>>>> +	return idev->info->ioctl(idev->info, cmd, arg);
>>>> +}
>>> As Stephen said, I will not take this, sorry.  It opens up the ability
>>> to add "new system calls" to a huge range of crappy drivers, it's
>>> something that vendors have been trying to push for years and is
>>> something that I will not allow.
>> Ok. Another alternative could be to add new sysfs attributes for the MSI-X
>> functionality similarly to what is done with "maps".
>> Would it be acceptable?
> If you get everyone else here to agree that this is the interface you
> all are going to be using, sure.  All I care is that you not add ioctl
> to the UIO interface.

Ok then. Thanks.

>
> greg k-h

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241735

On 10/07/15 21:05, Stephen Hemminger wrote:
>
> Thinking more of a custom filesystem like configfs
>

Why would configfs be better?
I imagine the final files layout as follows: uioY device that would have 
enabled MSI-X would have a separate uio/uioY/msix/irqZ for each IRQ[Z] 
MSI-X vector.
And this under the same roof with uio/uioY/maps/mapK files.

We then need to implement the write() method for irqZ files so that it 
would accept the eventfd and bind it (or unbind if the value is negative).
We may also make read() return not only the currently bond eventfd 
descriptor but also the corresponding MSI-X IRQ number.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242312

On 10/07/15 22:51, Stephen Hemminger wrote:
>
> I was thinking of not naming it uio at all
>

U mean doing something completely orthogonal to UIO? And how did u thing 
to access bars? With UIO or implement this functionality in the new 
configfs too?
If yes - then it will essentially be UIO with MSI-X support, won't it? 
;) It'll also be the third user-space drivers infrastructure and I'm not 
sure there is enough motivation add it.
Note that "some on this thread" are claiming that even two is too many... ;)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242539

On Thu, Oct 08, 2015 at 03:41:41PM +0300, Vlad Zolotarov wrote:
> 
> 
> On 10/07/15 22:51, Stephen Hemminger wrote:
> >
> >I was thinking of not naming it uio at all
> >
> 
> U mean doing something completely orthogonal to UIO? And how did u thing to
> access bars? With UIO or implement this functionality in the new configfs
> too?
> If yes - then it will essentially be UIO with MSI-X support, won't it? ;)
> It'll also be the third user-space drivers infrastructure and I'm not sure
> there is enough motivation add it.
> Note that "some on this thread" are claiming that even two is too many... ;)

Let's wait to see what happens here, I talked to Stephen and I think
that a non-UIO driver might be the correct solution here.  At the very
least, the proposed patches are not acceptable, so it can't be any worse
:)

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web