RFC: UIO null parent for __uio_register_device and uio_device_name()

RFC: UIO null parent for __uio_register_device and uio_device_name()

[Earl Chew]

Hans,

I had a two UIO changes queued up from earlier which I
forgot to ask you about.

Would you consider these changes for inclusion in the mainline ?


o Allow a null parent during uio_register_device. We've had
  situations where there was no convenient parent. We could
  concoct a parent, but it seemed to make sense to allow
  for a null.

  The commentary for device_create() say:

 * device_create - creates a device and registers it with sysfs
 * @class: pointer to the struct class that this device should be registered to
 * @parent: pointer to the parent struct device of this new device, if any

 The "if any" suggests that a null parent is possibility that is catered for.

 Our implementation with a null parent seems to work fine.

o Introduce uio_device_name() to allow callers to query for the
  name of the created uio device. Without this, there doesn't seem
  to be a straightforward way for a client to figure out the name of the
  device.




--- /tmp/uio.c.orig     2011-01-18 17:38:17.157452875 -0800
+++ /tmp/uio.c  2011-01-18 19:46:48.367453578 -0800
@@ -808,6 +808,18 @@
 }

 /**
+ * uio_device_name - obtain the name of the registered device
+ * @info:      UIO device capabilities
+ *
+ * returns name of device.
+ */
+const char *uio_device_name(struct uio_info *info)
+{
+       return dev_name(info->uio_dev->dev);
+}
+EXPORT_SYMBOL_GPL(uio_device_name);
+
+/**
  * uio_register_device - register a new userspace IO device
  * @owner:     module that creates the new device
  * @parent:    parent device
@@ -822,7 +834,7 @@
        struct uio_device *idev;
        int ret = 0;

-       if (!parent || !info || !info->name || !info->version)
+       if (!info || !info->name || !info->version)
                return -EINVAL;

        info->uio_dev = NULL;

--- /tmp/uio.c.orig     2011-01-18 17:38:17.157452875 -0800
+++ /tmp/uio.c  2011-01-18 17:38:56.307452064 -0800
@@ -822,7 +822,7 @@
        struct uio_device *idev;
        int ret = 0;

-       if (!parent || !info || !info->name || !info->version)
+       if (!info || !info->name || !info->version)
                return -EINVAL;

        info->uio_dev = NULL;


--- /tmp/uio_driver.h.orig      2011-01-18 19:42:13.777452360 -0800
+++ /tmp/uio_driver.h   2011-01-18 19:45:03.507453695 -0800
@@ -105,6 +105,7 @@
 }
 extern void uio_unregister_device(struct uio_info *info);
 extern void uio_event_notify(struct uio_info *info);
+extern const char *uio_device_name(struct uio_device *info);

 /* defines for uio_info->irq */
 #define UIO_IRQ_CUSTOM -1

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Hans J. Koch]

On Tue, Jan 18, 2011 at 07:56:52PM -0800, Earl Chew wrote:
> Hans,

Hi Earl,

> 
> I had a two UIO changes queued up from earlier which I
> forgot to ask you about.
> 
> Would you consider these changes for inclusion in the mainline ?
> 
> 
> o Allow a null parent during uio_register_device. We've had
>   situations where there was no convenient parent. We could
>   concoct a parent, but it seemed to make sense to allow
>   for a null.

Hmm. I saw many UIO drivers over the years but we never had that
problem. In the probe() function of your UIO driver you get a
suitable device as parameter. Why don't you just use it?

> 
> o Introduce uio_device_name() to allow callers to query for the
>   name of the created uio device. Without this, there doesn't seem
>   to be a straightforward way for a client to figure out the name of the
>   device.

Yes, we intentionally defined struct uio_device in uio.c and not in
a header file to prevent driver authors from using it ;-)

> 
> --- /tmp/uio.c.orig     2011-01-18 17:38:17.157452875 -0800
> +++ /tmp/uio.c  2011-01-18 19:46:48.367453578 -0800
> @@ -808,6 +808,18 @@
>  }
> 
>  /**
> + * uio_device_name - obtain the name of the registered device
> + * @info:      UIO device capabilities
> + *
> + * returns name of device.
> + */
> +const char *uio_device_name(struct uio_info *info)
> +{
> +       return dev_name(info->uio_dev->dev);

No NULL checks? Note that info->uio_dev can be NULL if uio_register_device()
failed...

> +}
> +EXPORT_SYMBOL_GPL(uio_device_name);

The function above returns strings like "uio0", "uio1", and so on. What's
the value of that? To identify a certain driver or distinguish several
instances of it, the "name" member of struct uio_info should be used.

But maybe I just miss the point. What's your use case?

Thanks,
Hans

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Earl Chew]

Hans,

Thanks for the quick response.

>> o Allow a null parent during uio_register_device. We've had
>>   situations where there was no convenient parent. We could
>>   concoct a parent, but it seemed to make sense to allow
>>   for a null.
> 
> Hmm. I saw many UIO drivers over the years but we never had that
> problem. In the probe() function of your UIO driver you get a
> suitable device as parameter. Why don't you just use it?

We build custom hardware, and UIO has served us well by providing
a way out of kernel space.

[ I'm calling the driver we write the "UIO client driver" to
  avoid confusion with the Linux UIO driver. ]

In some implementations, the HW binds directly to the CPU
physical address space. In particular, it is not accessed via
PCI space. Thus there is no probe() function required at the
UIO client driver.

The physical address assignments for the device are fixed
at system design time, and the UIO client driver (for the custom hardware)
has those addresses built into it.

No probing is required in this scenario.

> Yes, we intentionally defined struct uio_device in uio.c and not in
> a header file to prevent driver authors from using it ;-)

Yes. This is a good pattern. I like using it too  ;-)

>> +const char *uio_device_name(struct uio_info *info)
>> +{
>> +       return dev_name(info->uio_dev->dev);
> 
> No NULL checks? Note that info->uio_dev can be NULL if uio_register_device()
> failed...

Apologies. I've tightened up the code.  See below.

> The function above returns strings like "uio0", "uio1", and so on. What's
> the value of that? To identify a certain driver or distinguish several
> instances of it, the "name" member of struct uio_info should be used.
> 
> But maybe I just miss the point. What's your use case?

This is how our code currently works.

1. UIO client driver registers with Linux UIO driver.
2. UIO client driver advertises the name of the Linux UIO driver
   instance to which it is bound (eg "uio2") to our custom
   application
3. Our custom application queries for the name of the driver
   to use (eg "uio2")
4. Our custom application issues open("/dev/uio2") to access hardware.

Working on the implementation of our new system last night I realise
that our new configuration runs very lean, and I now do not have the
use of udev to populate /dev/uio[0-9].

This new configuration runs so lean that I don't have access to sysfs.
I've asked about including sysfs, but there are good reasons not to
include it. This means that /sys/class/uio[0-9]/... will not be available.

The result is that I need to figure out the major/minor device number in
order to access the Linux UIO device. I will advertise this through /proc/
entries specific to the UIO client driver. We can then use this information
to mknod /dev/uio[0-9].

I've added uio_device_chrdev() alongside uio_device_name() to allow
us to figure out the coordinates of the Linux UIO device.

Earl



--- /tmp/uio_driver.h.orig      2011-01-18 19:42:13.777452360 -0800
+++ /tmp/uio_driver.h   2011-01-19 07:26:58.037452472 -0800
@@ -105,6 +105,8 @@
 }
 extern void uio_unregister_device(struct uio_info *info);
 extern void uio_event_notify(struct uio_info *info);
+extern const char *uio_device_name(struct uio_device *info);
+extern dev_t uio_device_chrdev(struct uio_device *info);

 /* defines for uio_info->irq */
 #define UIO_IRQ_CUSTOM -1


--- /tmp/uio.c.orig     2011-01-18 17:38:17.157452875 -0800
+++ /tmp/uio.c  2011-01-19 07:39:48.457461684 -0800
@@ -808,6 +808,36 @@
 }

 /**
+ * uio_device_chrdev - obtain the name of the registered device
+ * @info:      UIO device capabilities
+ *
+ * returns device number or NULL if not available.
+ */
+dev_t uio_device_chrdev(struct uio_info *info)
+{
+       if (info && info->uio_dev)
+               return MKDEV(uio_major, info->uio_dev->minor);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(uio_device_chrdev);
+
+/**
+ * uio_device_name - obtain the name of the registered device
+ * @info:      UIO device capabilities
+ *
+ * returns name of device or NULL if not available.
+ */
+const char *uio_device_name(struct uio_info *info)
+{
+       if (info && info->uio_dev && info->uio_dev->dev)
+               return dev_name(info->uio_dev->dev);
+
+       return NULL;
+}
+EXPORT_SYMBOL_GPL(uio_device_name);
+
+/**
  * uio_register_device - register a new userspace IO device
  * @owner:     module that creates the new device
  * @parent:    parent device
@@ -822,7 +852,7 @@
        struct uio_device *idev;
        int ret = 0;

-       if (!parent || !info || !info->name || !info->version)
+       if (!info || !info->name || !info->version)
                return -EINVAL;

        info->uio_dev = NULL;

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Greg KH]

On Wed, Jan 19, 2011 at 07:42:12AM -0800, Earl Chew wrote:
> Hans,
> 
> Thanks for the quick response.
> 
> >> o Allow a null parent during uio_register_device. We've had
> >>   situations where there was no convenient parent. We could
> >>   concoct a parent, but it seemed to make sense to allow
> >>   for a null.
> > 
> > Hmm. I saw many UIO drivers over the years but we never had that
> > problem. In the probe() function of your UIO driver you get a
> > suitable device as parameter. Why don't you just use it?
> 
> We build custom hardware, and UIO has served us well by providing
> a way out of kernel space.
> 
> [ I'm calling the driver we write the "UIO client driver" to
>   avoid confusion with the Linux UIO driver. ]
> 
> In some implementations, the HW binds directly to the CPU
> physical address space. In particular, it is not accessed via
> PCI space. Thus there is no probe() function required at the
> UIO client driver.
> 
> The physical address assignments for the device are fixed
> at system design time, and the UIO client driver (for the custom hardware)
> has those addresses built into it.
> 
> No probing is required in this scenario.

Great, then create a platform device for your "hardware device" and you
should be fine, right?

> > Yes, we intentionally defined struct uio_device in uio.c and not in
> > a header file to prevent driver authors from using it ;-)
> 
> Yes. This is a good pattern. I like using it too  ;-)
> 
> >> +const char *uio_device_name(struct uio_info *info)
> >> +{
> >> +       return dev_name(info->uio_dev->dev);
> > 
> > No NULL checks? Note that info->uio_dev can be NULL if uio_register_device()
> > failed...
> 
> Apologies. I've tightened up the code.  See below.
> 
> > The function above returns strings like "uio0", "uio1", and so on. What's
> > the value of that? To identify a certain driver or distinguish several
> > instances of it, the "name" member of struct uio_info should be used.
> > 
> > But maybe I just miss the point. What's your use case?
> 
> This is how our code currently works.
> 
> 1. UIO client driver registers with Linux UIO driver.
> 2. UIO client driver advertises the name of the Linux UIO driver
>    instance to which it is bound (eg "uio2") to our custom
>    application
> 3. Our custom application queries for the name of the driver
>    to use (eg "uio2")
> 4. Our custom application issues open("/dev/uio2") to access hardware.
> 
> Working on the implementation of our new system last night I realise
> that our new configuration runs very lean, and I now do not have the
> use of udev to populate /dev/uio[0-9].

Then what populates your other device nodes?

> This new configuration runs so lean that I don't have access to sysfs.

I really find this hard to believe.  Why would you not configure sysfs
in your system?

> I've asked about including sysfs, but there are good reasons not to
> include it.

What are they?

> This means that /sys/class/uio[0-9]/... will not be available.

Nor will lots of other things that you will end up needing.

> The result is that I need to figure out the major/minor device number in
> order to access the Linux UIO device. I will advertise this through /proc/
> entries specific to the UIO client driver. We can then use this information
> to mknod /dev/uio[0-9].

Heh, you have /proc/ but not /sys/?  Someone needs to move out of the
90's :)

> I've added uio_device_chrdev() alongside uio_device_name() to allow
> us to figure out the coordinates of the Linux UIO device.

Ick, no, please use the sysfs files that are alredy present for such a
thing, don't reinvent the wheel, I will not accept such a change.

thanks,

greg k-h

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Earl Chew]

On 19/01/2011 8:30 AM, Greg KH wrote:
>> No probing is required in this scenario.
> 
> Great, then create a platform device for your "hardware device" and you
> should be fine, right?

Greg,

I suppose so. I don't understand the need to enforce !parent
since device_create() seems to be fine with a NULL parent.

Would you explain what is to be gained by requiring !parent
at the Linux UIO level ?

>> Working on the implementation of our new system last night I realise
>> that our new configuration runs very lean, and I now do not have the
>> use of udev to populate /dev/uio[0-9].
> 
> Then what populates your other device nodes?

I believe they are fixed in the root file system (ie the major numbers
are fixed). It is also possible that /etc/rcS script does the rest with
judicious mknod invocations.

>> This new configuration runs so lean that I don't have access to sysfs.
> 
> I really find this hard to believe.  Why would you not configure sysfs
> in your system?

The needs of our hardware and application are such that one of the key system
designers told me:

	For the record, sysfs affects interface scalability in two ways: adds a 
	significant memory overhead (IIRC 300-500 bytes per interface) and slows down 
	device registration and de-registration.

>> The result is that I need to figure out the major/minor device number in
>> order to access the Linux UIO device. I will advertise this through /proc/
>> entries specific to the UIO client driver. We can then use this information
>> to mknod /dev/uio[0-9].
> 
> Heh, you have /proc/ but not /sys/?  Someone needs to move out of the
> 90's :)

LOL  Yes, I see your point.

>> I've added uio_device_chrdev() alongside uio_device_name() to allow
>> us to figure out the coordinates of the Linux UIO device.
> 
> Ick, no, please use the sysfs files that are alredy present for such a
> thing, don't reinvent the wheel, I will not accept such a change.

I think you're telling me you won't accept either of:

a. uio_device_name()
b. uio_device_chrdev()


It is not my intention to reinvent the wheel. The problem I have
is that the wheel in question won't fit my application context.


I'm trying to figure out how to get this done without a kernel
change, but I don't have many options left.


I think I can do the following:

o Use device_create() to make a parent before calling uio_register_device.

o Have the application search through /proc/devices to yield the major number
  of the uio driver.


With no sysfs, I cannot think of a way to get hold of the minor number allocated
to this instance of the Linux UIO device, and without that I can't open() the
device driver.


Earl

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Earl Chew]

Greg,

I wrote:
> I think you're telling me you won't accept either of:
> 
> a. uio_device_name()
> b. uio_device_chrdev()
[ snip ]
> With no sysfs, I cannot think of a way to get hold of the minor number allocated
> to this instance of the Linux UIO device, and without that I can't open() the
> device driver.

Would you consider :

	extern int uio_device_minor(struct uio_info *info);

?

That would allow me to reconstitute the coordinates of the Linux UIO device
instance from a combination of /proc/devices and a uio_device_minor() query.

Earl

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Greg KH]

On Wed, Jan 19, 2011 at 09:11:04AM -0800, Earl Chew wrote:
> Greg,
> 
> I wrote:
> > I think you're telling me you won't accept either of:
> > 
> > a. uio_device_name()
> > b. uio_device_chrdev()
> [ snip ]
> > With no sysfs, I cannot think of a way to get hold of the minor number allocated
> > to this instance of the Linux UIO device, and without that I can't open() the
> > device driver.
> 
> Would you consider :
> 
> 	extern int uio_device_minor(struct uio_info *info);
> 
> ?
> 
> That would allow me to reconstitute the coordinates of the Linux UIO device
> instance from a combination of /proc/devices and a uio_device_minor() query.

Again, no, this is already present in userspace, and no kernel code
should ever need it.

sorry.

greg k-h

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Greg KH]

On Wed, Jan 19, 2011 at 09:07:01AM -0800, Earl Chew wrote:
> On 19/01/2011 8:30 AM, Greg KH wrote:
> >> No probing is required in this scenario.
> > 
> > Great, then create a platform device for your "hardware device" and you
> > should be fine, right?
> 
> Greg,
> 
> I suppose so. I don't understand the need to enforce !parent
> since device_create() seems to be fine with a NULL parent.
> 
> Would you explain what is to be gained by requiring !parent
> at the Linux UIO level ?

We want things to be "sane" and not have a uio device show up as a child
of a "virtual" device, as in reality, you have a real device so you
had better create one.

> >> Working on the implementation of our new system last night I realise
> >> that our new configuration runs very lean, and I now do not have the
> >> use of udev to populate /dev/uio[0-9].
> > 
> > Then what populates your other device nodes?
> 
> I believe they are fixed in the root file system (ie the major numbers
> are fixed). It is also possible that /etc/rcS script does the rest with
> judicious mknod invocations.

Then do the same for the uio devices and you should be fine, right?

> >> This new configuration runs so lean that I don't have access to sysfs.
> > 
> > I really find this hard to believe.  Why would you not configure sysfs
> > in your system?
> 
> The needs of our hardware and application are such that one of the key system
> designers told me:
> 
> 	For the record, sysfs affects interface scalability in two ways: adds a 
> 	significant memory overhead (IIRC 300-500 bytes per interface) and slows down 
> 	device registration and de-registration.

Really?  How many interfaces do you have that this is a slow-down, and
how is it affecting system speed (I have _never_ heard this complaint.
I've heard others, that have since been fixed, but never this one.)

> >> The result is that I need to figure out the major/minor device number in
> >> order to access the Linux UIO device. I will advertise this through /proc/
> >> entries specific to the UIO client driver. We can then use this information
> >> to mknod /dev/uio[0-9].
> > 
> > Heh, you have /proc/ but not /sys/?  Someone needs to move out of the
> > 90's :)
> 
> LOL  Yes, I see your point.
> 
> >> I've added uio_device_chrdev() alongside uio_device_name() to allow
> >> us to figure out the coordinates of the Linux UIO device.
> > 
> > Ick, no, please use the sysfs files that are alredy present for such a
> > thing, don't reinvent the wheel, I will not accept such a change.
> 
> I think you're telling me you won't accept either of:
> 
> a. uio_device_name()
> b. uio_device_chrdev()
> 
> 
> It is not my intention to reinvent the wheel. The problem I have
> is that the wheel in question won't fit my application context.
> 
> 
> I'm trying to figure out how to get this done without a kernel
> change, but I don't have many options left.

Yes you do, use the kernel interfaces that are already present to
provide this information (i.e. sysfs.)

> I think I can do the following:
> 
> o Use device_create() to make a parent before calling uio_register_device.

No, use a platform device, don't create you own "raw" struct device.

> o Have the application search through /proc/devices to yield the major number
>   of the uio driver.

Yes, you can do that, but again, sysfs is cleaner, nicer, and in the
end, easier to maintain.  It directly provides this information in the
/sys/dev/ files.  I will not accept duplicate kernel code sorry.

And I will be glad to work with anyone who feels that sysfs is somehow
an "unacceptable" overhead.  Look at what you are having to do right
now, to work around not having it.  It's probably caused more than
enough "overhead" and extra work and time and money than it would have
been to just enable it in the first place, instead of trying to work
around it not being present.

And my "move out of the 90's" comment really is true in a way.  To do
thing things that you are wanting to do here, you need to embrace the
way that Linux has changed over the years, as it is doing the things you
need it to do, if you allow it to.  Don't ignore the code already
written, debugged, supported, and used by everyone else in the world
just because it feels "different" than what your engineers are "used
to".

Otherwise, you might as well be using a BSD variant :)

good luck,

greg k-h

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Hans J. Koch]

On Wed, Jan 19, 2011 at 09:07:01AM -0800, Earl Chew wrote:
> 
> >> I've added uio_device_chrdev() alongside uio_device_name() to allow
> >> us to figure out the coordinates of the Linux UIO device.
> > 

There's a small library with helper functions. Among other things, they
allow searching the UIO device node by name (the name you gave your device
in uio_info->name). The library is here:

git://git.linutronix.de/projects/libUIO.git

or with gitweb:

http://git.linutronix.de/gitweb.cgi?p=projects/libUIO.git;a=summary

Thanks,
Hans

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Earl Chew]

Hans Koch wrote:
> There's a small library with helper functions. Among other things, they
> allow searching the UIO device node by name (the name you gave your device
> in uio_info->name). The library is here:
> 
> git://git.linutronix.de/projects/libUIO.git
> 
> or with gitweb:
> 
> http://git.linutronix.de/gitweb.cgi?p=projects/libUIO.git;a=summary

Hans, Greg,

I appreciate the chance to discuss this change with you. I do understand your
point of view, and I can see how libUIO navigates sysfs from userspace
to figure out what's there.

Unfortunately I am not in a position to mandate the inclusion of sysfs in our
deployment, and Linux UIO has a dependency on sysfs to be usable.

I had hoped to stay within the confines of the mainline, but for now
we'll have to tolerate a local patch.

Earl

Re: RFC: UIO null parent for __uio_register_device and uio_device_name()

[Greg KH]

On Wed, Jan 19, 2011 at 02:06:49PM -0800, Earl Chew wrote:
> Hans Koch wrote:
> > There's a small library with helper functions. Among other things, they
> > allow searching the UIO device node by name (the name you gave your device
> > in uio_info->name). The library is here:
> > 
> > git://git.linutronix.de/projects/libUIO.git
> > 
> > or with gitweb:
> > 
> > http://git.linutronix.de/gitweb.cgi?p=projects/libUIO.git;a=summary
> 
> Hans, Greg,
> 
> I appreciate the chance to discuss this change with you. I do understand your
> point of view, and I can see how libUIO navigates sysfs from userspace
> to figure out what's there.
> 
> Unfortunately I am not in a position to mandate the inclusion of sysfs in our
> deployment, and Linux UIO has a dependency on sysfs to be usable.

I still fail to see why sysfs has been deemed "unacceptable".

What is wrong with it that is keeping you from using it?  Technical
details please, otherwise there is no way for me to ever be able to fix
them.

thanks,

greg k-h