[PATCH] Export device_add_attributes() so drivers can use it.

[PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

I find myself using the pattern of device_add_attributes() and
device_remove_attributes() frequently in my drivers.  Rather than
reinventing the wheel every time, I'm floating this patch to export
the symbols to see how it is received.  If this looks okay then I'll
rework my drivers and post additional patches to use these functions.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: linux-kernel@vger.kernel.org
CC: Greg Kroah-Hartman <gregkh@suse.de>
---

 drivers/base/core.c    |   10 ++++++----
 include/linux/device.h |    4 ++++
 2 files changed, 10 insertions(+), 4 deletions(-)


diff --git a/drivers/base/core.c b/drivers/base/core.c
index f3eae63..5393eb6 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -302,8 +302,8 @@ out:
 static struct device_attribute uevent_attr =
 	__ATTR(uevent, S_IRUGO | S_IWUSR, show_uevent, store_uevent);
 
-static int device_add_attributes(struct device *dev,
-				 struct device_attribute *attrs)
+int device_add_attributes(struct device *dev,
+			  struct device_attribute *attrs)
 {
 	int error = 0;
 	int i;
@@ -320,9 +320,10 @@ static int device_add_attributes(struct device *dev,
 	}
 	return error;
 }
+EXPORT_SYMBOL(device_add_attributes);
 
-static void device_remove_attributes(struct device *dev,
-				     struct device_attribute *attrs)
+void device_remove_attributes(struct device *dev,
+			      struct device_attribute *attrs)
 {
 	int i;
 
@@ -330,6 +331,7 @@ static void device_remove_attributes(struct device *dev,
 		for (i = 0; attr_name(attrs[i]); i++)
 			device_remove_file(dev, &attrs[i]);
 }
+EXPORT_SYMBOL(device_remove_attributes);
 
 static int device_add_groups(struct device *dev,
 			     struct attribute_group **groups)
diff --git a/include/linux/device.h b/include/linux/device.h
index 45e5b19..2a64d84 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -310,6 +310,10 @@ extern int __must_check device_create_file(struct device *device,
 					   struct device_attribute *entry);
 extern void device_remove_file(struct device *dev,
 			       struct device_attribute *attr);
+extern int device_add_attributes(struct device *dev,
+				 struct device_attribute *attrs);
+extern void device_remove_attributes(struct device *dev,
+				     struct device_attribute *attrs);
 extern int __must_check device_create_bin_file(struct device *dev,
 					       struct bin_attribute *attr);
 extern void device_remove_bin_file(struct device *dev,

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Greg KH]

On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> I find myself using the pattern of device_add_attributes() and
> device_remove_attributes() frequently in my drivers.  Rather than
> reinventing the wheel every time, I'm floating this patch to export
> the symbols to see how it is received.  If this looks okay then I'll
> rework my drivers and post additional patches to use these functions.

No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
like the rest of the driver core.  Is that ok with you?

thanks,

greg k-h

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> I find myself using the pattern of device_add_attributes() and
>> device_remove_attributes() frequently in my drivers.  Rather than
>> reinventing the wheel every time, I'm floating this patch to export
>> the symbols to see how it is received.  If this looks okay then I'll
>> rework my drivers and post additional patches to use these functions.
>
> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
> like the rest of the driver core.  Is that ok with you?

These functions used outside the core create attributes after the
uevent is sent, and userspace will not see these files at event time.
This is in most cases a pretty broken behavior. Is that the expected
behavior in your drivers?

Thanks,
Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

On Wed, Feb 18, 2009 at 8:29 AM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> I find myself using the pattern of device_add_attributes() and
>> device_remove_attributes() frequently in my drivers.  Rather than
>> reinventing the wheel every time, I'm floating this patch to export
>> the symbols to see how it is received.  If this looks okay then I'll
>> rework my drivers and post additional patches to use these functions.
>
> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
> like the rest of the driver core.  Is that ok with you?

Fine by me.  I'll rework the patch.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>
>>> I find myself using the pattern of device_add_attributes() and
>>> device_remove_attributes() frequently in my drivers.  Rather than
>>> reinventing the wheel every time, I'm floating this patch to export
>>> the symbols to see how it is received.  If this looks okay then I'll
>>> rework my drivers and post additional patches to use these functions.
>>
>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>> like the rest of the driver core.  Is that ok with you?
>
> These functions used outside the core create attributes after the
> uevent is sent, and userspace will not see these files at event time.
> This is in most cases a pretty broken behavior. Is that the expected
> behavior in your drivers?

??? I don't follow what you mean.

I'm using these functions to allow the driver to add device attribs;
primarily for debugging knobs and controls.  Userspace will see the
files after the driver is bound to the device.  The uevent doesn't
really come into play.

g.



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>>
>>>> I find myself using the pattern of device_add_attributes() and
>>>> device_remove_attributes() frequently in my drivers.  Rather than
>>>> reinventing the wheel every time, I'm floating this patch to export
>>>> the symbols to see how it is received.  If this looks okay then I'll
>>>> rework my drivers and post additional patches to use these functions.
>>>
>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>>> like the rest of the driver core.  Is that ok with you?
>>
>> These functions used outside the core create attributes after the
>> uevent is sent, and userspace will not see these files at event time.
>> This is in most cases a pretty broken behavior. Is that the expected
>> behavior in your drivers?
>
> ??? I don't follow what you mean.
>
> I'm using these functions to allow the driver to add device attribs;
> primarily for debugging knobs and controls.  Userspace will see the
> files after the driver is bound to the device.  The uevent doesn't
> really come into play.

Sure, they do. Many things expect all files which are visible at the
device to be readable also at event time. That's the whole way udev
and device property matching works. There are only a few exceptions
where creating files at a device later, after it is registered with
the core, is not a bug.

Thanks,
Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>>>
>>>>> I find myself using the pattern of device_add_attributes() and
>>>>> device_remove_attributes() frequently in my drivers.  Rather than
>>>>> reinventing the wheel every time, I'm floating this patch to export
>>>>> the symbols to see how it is received.  If this looks okay then I'll
>>>>> rework my drivers and post additional patches to use these functions.
>>>>
>>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>>>> like the rest of the driver core.  Is that ok with you?
>>>
>>> These functions used outside the core create attributes after the
>>> uevent is sent, and userspace will not see these files at event time.
>>> This is in most cases a pretty broken behavior. Is that the expected
>>> behavior in your drivers?
>>
>> ??? I don't follow what you mean.
>>
>> I'm using these functions to allow the driver to add device attribs;
>> primarily for debugging knobs and controls.  Userspace will see the
>> files after the driver is bound to the device.  The uevent doesn't
>> really come into play.
>
> Sure, they do. Many things expect all files which are visible at the
> device to be readable also at event time. That's the whole way udev
> and device property matching works. There are only a few exceptions
> where creating files at a device later, after it is registered with
> the core, is not a bug.

Let me make sure I understand you...

Is it a bug for a device driver to call
device_create_file()/device_remove_file() at probe time?  For example,
if I have a data capture device which is probed via the platform bus,
is it okay for the .probe() function for the driver to use
device_create_file() to add a 'rate_statistics' file which dumps out
some data rate statistics in ASCII form?

I was under the impression that
device_create_file()/device_remove_file() were okay to use at probe
time.  device_add_attributes()/device_remove_attributes() are only
wrappers around device_create_file()/device_remove_file() with error
checking and unwinding when things go wrong.

Am I incorrect here?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Wed, Feb 18, 2009 at 17:34, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>>>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>>>>
>>>>>> I find myself using the pattern of device_add_attributes() and
>>>>>> device_remove_attributes() frequently in my drivers.  Rather than
>>>>>> reinventing the wheel every time, I'm floating this patch to export
>>>>>> the symbols to see how it is received.  If this looks okay then I'll
>>>>>> rework my drivers and post additional patches to use these functions.
>>>>>
>>>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>>>>> like the rest of the driver core.  Is that ok with you?
>>>>
>>>> These functions used outside the core create attributes after the
>>>> uevent is sent, and userspace will not see these files at event time.
>>>> This is in most cases a pretty broken behavior. Is that the expected
>>>> behavior in your drivers?
>>>
>>> ??? I don't follow what you mean.
>>>
>>> I'm using these functions to allow the driver to add device attribs;
>>> primarily for debugging knobs and controls.  Userspace will see the
>>> files after the driver is bound to the device.  The uevent doesn't
>>> really come into play.
>>
>> Sure, they do. Many things expect all files which are visible at the
>> device to be readable also at event time. That's the whole way udev
>> and device property matching works. There are only a few exceptions
>> where creating files at a device later, after it is registered with
>> the core, is not a bug.
>
> Let me make sure I understand you...
>
> Is it a bug for a device driver to call
> device_create_file()/device_remove_file() at probe time?  For example,
> if I have a data capture device which is probed via the platform bus,
> is it okay for the .probe() function for the driver to use
> device_create_file() to add a 'rate_statistics' file which dumps out
> some data rate statistics in ASCII form?
>
> I was under the impression that
> device_create_file()/device_remove_file() were okay to use at probe
> time.  device_add_attributes()/device_remove_attributes() are only
> wrappers around device_create_file()/device_remove_file() with error
> checking and unwinding when things go wrong.
>
> Am I incorrect here?

You are probing an existing "struct device", and then create
attributes at this device when the probe succeeds? If yes, why don't
you create a new child "struct device" with your functionality and add
the attributes there?

The new child device can just add the attributes by its default
attributes, and all files will properly exist at event time, and the
device probing result will be properly advertised with an uevent of
the child device to userspace.

That would be the way the core and userspace expect device probes to
look like, if there are no special reasons that prevent this model
from working, which should be properly documented, so nobody just
copies that code without the need for such hacks.

Thanks,
Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

On Wed, Feb 18, 2009 at 9:49 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Wed, Feb 18, 2009 at 17:34, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>>>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>>>>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>>>>>
>>>>>>> I find myself using the pattern of device_add_attributes() and
>>>>>>> device_remove_attributes() frequently in my drivers.  Rather than
>>>>>>> reinventing the wheel every time, I'm floating this patch to export
>>>>>>> the symbols to see how it is received.  If this looks okay then I'll
>>>>>>> rework my drivers and post additional patches to use these functions.
>>>>>>
>>>>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>>>>>> like the rest of the driver core.  Is that ok with you?
>>>>>
>>>>> These functions used outside the core create attributes after the
>>>>> uevent is sent, and userspace will not see these files at event time.
>>>>> This is in most cases a pretty broken behavior. Is that the expected
>>>>> behavior in your drivers?
>>>>
>>>> ??? I don't follow what you mean.
>>>>
>>>> I'm using these functions to allow the driver to add device attribs;
>>>> primarily for debugging knobs and controls.  Userspace will see the
>>>> files after the driver is bound to the device.  The uevent doesn't
>>>> really come into play.
>>>
>>> Sure, they do. Many things expect all files which are visible at the
>>> device to be readable also at event time. That's the whole way udev
>>> and device property matching works. There are only a few exceptions
>>> where creating files at a device later, after it is registered with
>>> the core, is not a bug.
>>
>> Let me make sure I understand you...
>>
>> Is it a bug for a device driver to call
>> device_create_file()/device_remove_file() at probe time?  For example,
>> if I have a data capture device which is probed via the platform bus,
>> is it okay for the .probe() function for the driver to use
>> device_create_file() to add a 'rate_statistics' file which dumps out
>> some data rate statistics in ASCII form?
>>
>> I was under the impression that
>> device_create_file()/device_remove_file() were okay to use at probe
>> time.  device_add_attributes()/device_remove_attributes() are only
>> wrappers around device_create_file()/device_remove_file() with error
>> checking and unwinding when things go wrong.
>>
>> Am I incorrect here?
>
> You are probing an existing "struct device", and then create
> attributes at this device when the probe succeeds?

Yes

> If yes, why don't
> you create a new child "struct device" with your functionality and add
> the attributes there?

Mostly because I have no need another full struct device, and none of
the files that I'm adding have any bearing on udev.  They are debug
and statistical files for an embedded system that are used by the
developer.  I don't want application code depending on them and I'm
not interested in having them advertised.

> That would be the way the core and userspace expect device probes to
> look like, if there are no special reasons that prevent this model
> from working, which should be properly documented, so nobody just
> copies that code without the need for such hacks.

Proverbial cat and bag aside, I already see numerous examples of the
model I'm using in the drivers directory.  If this is a bad use case
then, yes, it really needs to be documented.  I see nothing that shows
me that it is bad practice, either in the code comments or in
Documentation (The only files which mention device_create_file() are
sysfs.txt and device.txt).  Nor do I see anything that shows it is
dangerous for a driver to add sysfs files at probe time to an existing
struct device.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Wed, Feb 18, 2009 at 18:51, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 18, 2009 at 9:49 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> On Wed, Feb 18, 2009 at 17:34, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>>> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>>>>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>>>>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>>>>>>> From: Grant Likely <grant.likely@secretlab.ca>
>>>>>>>>
>>>>>>>> I find myself using the pattern of device_add_attributes() and
>>>>>>>> device_remove_attributes() frequently in my drivers.  Rather than
>>>>>>>> reinventing the wheel every time, I'm floating this patch to export
>>>>>>>> the symbols to see how it is received.  If this looks okay then I'll
>>>>>>>> rework my drivers and post additional patches to use these functions.
>>>>>>>
>>>>>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>>>>>>> like the rest of the driver core.  Is that ok with you?
>>>>>>
>>>>>> These functions used outside the core create attributes after the
>>>>>> uevent is sent, and userspace will not see these files at event time.
>>>>>> This is in most cases a pretty broken behavior. Is that the expected
>>>>>> behavior in your drivers?
>>>>>
>>>>> ??? I don't follow what you mean.
>>>>>
>>>>> I'm using these functions to allow the driver to add device attribs;
>>>>> primarily for debugging knobs and controls.  Userspace will see the
>>>>> files after the driver is bound to the device.  The uevent doesn't
>>>>> really come into play.
>>>>
>>>> Sure, they do. Many things expect all files which are visible at the
>>>> device to be readable also at event time. That's the whole way udev
>>>> and device property matching works. There are only a few exceptions
>>>> where creating files at a device later, after it is registered with
>>>> the core, is not a bug.
>>>
>>> Let me make sure I understand you...
>>>
>>> Is it a bug for a device driver to call
>>> device_create_file()/device_remove_file() at probe time?  For example,
>>> if I have a data capture device which is probed via the platform bus,
>>> is it okay for the .probe() function for the driver to use
>>> device_create_file() to add a 'rate_statistics' file which dumps out
>>> some data rate statistics in ASCII form?
>>>
>>> I was under the impression that
>>> device_create_file()/device_remove_file() were okay to use at probe
>>> time.  device_add_attributes()/device_remove_attributes() are only
>>> wrappers around device_create_file()/device_remove_file() with error
>>> checking and unwinding when things go wrong.
>>>
>>> Am I incorrect here?
>>
>> You are probing an existing "struct device", and then create
>> attributes at this device when the probe succeeds?
>
> Yes
>
>> If yes, why don't
>> you create a new child "struct device" with your functionality and add
>> the attributes there?
>
> Mostly because I have no need another full struct device, and none of
> the files that I'm adding have any bearing on udev.  They are debug
> and statistical files for an embedded system that are used by the
> developer.  I don't want application code depending on them and I'm
> not interested in having them advertised.

That's what debugfs is for.

>> That would be the way the core and userspace expect device probes to
>> look like, if there are no special reasons that prevent this model
>> from working, which should be properly documented, so nobody just
>> copies that code without the need for such hacks.
>
> Proverbial cat and bag aside, I already see numerous examples of the
> model I'm using in the drivers directory.  If this is a bad use case
> then, yes, it really needs to be documented.  I see nothing that shows
> me that it is bad practice, either in the code comments or in
> Documentation (The only files which mention device_create_file() are
> sysfs.txt and device.txt).

A huge fraction of the code using the platform bus can be marked
entirely as "bad use case" be it staticall allocated objects, or
messing around with internal in bad ways to "optimize" stuff for no
good reason.

> Nor do I see anything that shows it is
> dangerous for a driver to add sysfs files at probe time to an existing
> struct device.

It's not about "danger" its about bad code, which is a different kind
of "danger". :)

Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Greg KH]

On Wed, Feb 18, 2009 at 07:32:19PM +0100, Kay Sievers wrote:
> On Wed, Feb 18, 2009 at 18:51, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Wed, Feb 18, 2009 at 9:49 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> >> On Wed, Feb 18, 2009 at 17:34, Grant Likely <grant.likely@secretlab.ca> wrote:
> >>> On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> >>>> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
> >>>>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> >>>>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
> >>>>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
> >>>>>>>> From: Grant Likely <grant.likely@secretlab.ca>
> >>>>>>>>
> >>>>>>>> I find myself using the pattern of device_add_attributes() and
> >>>>>>>> device_remove_attributes() frequently in my drivers.  Rather than
> >>>>>>>> reinventing the wheel every time, I'm floating this patch to export
> >>>>>>>> the symbols to see how it is received.  If this looks okay then I'll
> >>>>>>>> rework my drivers and post additional patches to use these functions.
> >>>>>>>
> >>>>>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
> >>>>>>> like the rest of the driver core.  Is that ok with you?
> >>>>>>
> >>>>>> These functions used outside the core create attributes after the
> >>>>>> uevent is sent, and userspace will not see these files at event time.
> >>>>>> This is in most cases a pretty broken behavior. Is that the expected
> >>>>>> behavior in your drivers?
> >>>>>
> >>>>> ??? I don't follow what you mean.
> >>>>>
> >>>>> I'm using these functions to allow the driver to add device attribs;
> >>>>> primarily for debugging knobs and controls.  Userspace will see the
> >>>>> files after the driver is bound to the device.  The uevent doesn't
> >>>>> really come into play.
> >>>>
> >>>> Sure, they do. Many things expect all files which are visible at the
> >>>> device to be readable also at event time. That's the whole way udev
> >>>> and device property matching works. There are only a few exceptions
> >>>> where creating files at a device later, after it is registered with
> >>>> the core, is not a bug.
> >>>
> >>> Let me make sure I understand you...
> >>>
> >>> Is it a bug for a device driver to call
> >>> device_create_file()/device_remove_file() at probe time?  For example,
> >>> if I have a data capture device which is probed via the platform bus,
> >>> is it okay for the .probe() function for the driver to use
> >>> device_create_file() to add a 'rate_statistics' file which dumps out
> >>> some data rate statistics in ASCII form?
> >>>
> >>> I was under the impression that
> >>> device_create_file()/device_remove_file() were okay to use at probe
> >>> time.  device_add_attributes()/device_remove_attributes() are only
> >>> wrappers around device_create_file()/device_remove_file() with error
> >>> checking and unwinding when things go wrong.
> >>>
> >>> Am I incorrect here?
> >>
> >> You are probing an existing "struct device", and then create
> >> attributes at this device when the probe succeeds?
> >
> > Yes
> >
> >> If yes, why don't
> >> you create a new child "struct device" with your functionality and add
> >> the attributes there?
> >
> > Mostly because I have no need another full struct device, and none of
> > the files that I'm adding have any bearing on udev.  They are debug
> > and statistical files for an embedded system that are used by the
> > developer.  I don't want application code depending on them and I'm
> > not interested in having them advertised.
> 
> That's what debugfs is for.

Kay is right, use debugfs for stuff like this.  Not sysfs.

So sorry, I'm not going to apply the patch, and you should change your
code to not open-code this as well.

thanks,

greg k-h

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Wed, Feb 18, 2009 at 19:39, Greg KH <gregkh@suse.de> wrote:
> On Wed, Feb 18, 2009 at 07:32:19PM +0100, Kay Sievers wrote:
>> On Wed, Feb 18, 2009 at 18:51, Grant Likely <grant.likely@secretlab.ca> wrote:
>> > On Wed, Feb 18, 2009 at 9:49 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> >> On Wed, Feb 18, 2009 at 17:34, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >>> On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> >>>> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@secretlab.ca> wrote:
>> >>>>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> >>>>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@suse.de> wrote:
>> >>>>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>> >>>>>>>> From: Grant Likely <grant.likely@secretlab.ca>
>> >>>>>>>>
>> >>>>>>>> I find myself using the pattern of device_add_attributes() and
>> >>>>>>>> device_remove_attributes() frequently in my drivers.  Rather than
>> >>>>>>>> reinventing the wheel every time, I'm floating this patch to export
>> >>>>>>>> the symbols to see how it is received.  If this looks okay then I'll
>> >>>>>>>> rework my drivers and post additional patches to use these functions.
>> >>>>>>>
>> >>>>>>> No objection from me, as long as the symbols are EXPORT_SYMBOL_GPL(),
>> >>>>>>> like the rest of the driver core.  Is that ok with you?
>> >>>>>>
>> >>>>>> These functions used outside the core create attributes after the
>> >>>>>> uevent is sent, and userspace will not see these files at event time.
>> >>>>>> This is in most cases a pretty broken behavior. Is that the expected
>> >>>>>> behavior in your drivers?
>> >>>>>
>> >>>>> ??? I don't follow what you mean.
>> >>>>>
>> >>>>> I'm using these functions to allow the driver to add device attribs;
>> >>>>> primarily for debugging knobs and controls.  Userspace will see the
>> >>>>> files after the driver is bound to the device.  The uevent doesn't
>> >>>>> really come into play.
>> >>>>
>> >>>> Sure, they do. Many things expect all files which are visible at the
>> >>>> device to be readable also at event time. That's the whole way udev
>> >>>> and device property matching works. There are only a few exceptions
>> >>>> where creating files at a device later, after it is registered with
>> >>>> the core, is not a bug.
>> >>>
>> >>> Let me make sure I understand you...
>> >>>
>> >>> Is it a bug for a device driver to call
>> >>> device_create_file()/device_remove_file() at probe time?  For example,
>> >>> if I have a data capture device which is probed via the platform bus,
>> >>> is it okay for the .probe() function for the driver to use
>> >>> device_create_file() to add a 'rate_statistics' file which dumps out
>> >>> some data rate statistics in ASCII form?
>> >>>
>> >>> I was under the impression that
>> >>> device_create_file()/device_remove_file() were okay to use at probe
>> >>> time.  device_add_attributes()/device_remove_attributes() are only
>> >>> wrappers around device_create_file()/device_remove_file() with error
>> >>> checking and unwinding when things go wrong.
>> >>>
>> >>> Am I incorrect here?
>> >>
>> >> You are probing an existing "struct device", and then create
>> >> attributes at this device when the probe succeeds?
>> >
>> > Yes
>> >
>> >> If yes, why don't
>> >> you create a new child "struct device" with your functionality and add
>> >> the attributes there?
>> >
>> > Mostly because I have no need another full struct device, and none of
>> > the files that I'm adding have any bearing on udev.  They are debug
>> > and statistical files for an embedded system that are used by the
>> > developer.  I don't want application code depending on them and I'm
>> > not interested in having them advertised.
>>
>> That's what debugfs is for.
>
> Kay is right, use debugfs for stuff like this.  Not sysfs.
>
> So sorry, I'm not going to apply the patch, and you should change your
> code to not open-code this as well.

You can probably use an attribute group instead, if you want to create
a bunch of attributes with error handling. If the group has a name,
the attributes end in a subdir, without a name, they will be created
directly.

Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

On Wed, Feb 18, 2009 at 11:39 AM, Greg KH <gregkh@suse.de> wrote:
> On Wed, Feb 18, 2009 at 07:32:19PM +0100, Kay Sievers wrote:
>> On Wed, Feb 18, 2009 at 18:51, Grant Likely <grant.likely@secretlab.ca> wrote:
>> > Mostly because I have no need another full struct device, and none of
>> > the files that I'm adding have any bearing on udev.  They are debug
>> > and statistical files for an embedded system that are used by the
>> > developer.  I don't want application code depending on them and I'm
>> > not interested in having them advertised.
>>
>> That's what debugfs is for.
>
> Kay is right, use debugfs for stuff like this.  Not sysfs.
>
> So sorry, I'm not going to apply the patch, and you should change your
> code to not open-code this as well.

Forgive my frustration, but I've been happily using sysfs for simple
exports of driver data (not just debug either; for tuning also) for a
while now with no indication that I've doing the wrong thing.  I
haven't found anything in Documentation or in LDDv3 that says files
should not be created at probe time.  The sysfs API is beautiful to
work with and requires less code than the equivalent debugfs
interface.  Often using it exactly matches the use case that I need;
small values, simple settings and safe, concise code.

So, I re-ask my questions:
When is it legal and when is it not legal to call
device_create_file()?  If debug only stuff belongs in debugfs, then
fair enough.  But what about when tuning controls are needed?  If I
have to create a child 'struct device', then I will do so, but I'm
still not convinced on the benefit.

Even then, I'm confused.  The KOBJ_ADD uevent is raised at
device_register() time.  However, it appear that device_create_file()
often gets called after device_register() (pci_bus_add_child() for
example).  What is the cutoff point for calling device_create_file()
so that attributes exist before the uevent (as Kay stated earlier in
this thread)?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Thu, Feb 19, 2009 at 07:50, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 18, 2009 at 11:39 AM, Greg KH <gregkh@suse.de> wrote:
>> On Wed, Feb 18, 2009 at 07:32:19PM +0100, Kay Sievers wrote:
>>> On Wed, Feb 18, 2009 at 18:51, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> > Mostly because I have no need another full struct device, and none of
>>> > the files that I'm adding have any bearing on udev.  They are debug
>>> > and statistical files for an embedded system that are used by the
>>> > developer.  I don't want application code depending on them and I'm
>>> > not interested in having them advertised.
>>>
>>> That's what debugfs is for.
>>
>> Kay is right, use debugfs for stuff like this.  Not sysfs.
>>
>> So sorry, I'm not going to apply the patch, and you should change your
>> code to not open-code this as well.
>
> Forgive my frustration, but I've been happily using sysfs for simple
> exports of driver data (not just debug either; for tuning also) for a
> while now with no indication that I've doing the wrong thing.  I
> haven't found anything in Documentation or in LDDv3 that says files
> should not be created at probe time.  The sysfs API is beautiful to
> work with and requires less code than the equivalent debugfs
> interface.  Often using it exactly matches the use case that I need;
> small values, simple settings and safe, concise code.
>
> So, I re-ask my questions:
> When is it legal and when is it not legal to call
> device_create_file()?  If debug only stuff belongs in debugfs, then
> fair enough.  But what about when tuning controls are needed?  If I
> have to create a child 'struct device', then I will do so, but I'm
> still not convinced on the benefit.
>
> Even then, I'm confused.  The KOBJ_ADD uevent is raised at
> device_register() time.  However, it appear that device_create_file()
> often gets called after device_register() (pci_bus_add_child() for
> example).  What is the cutoff point for calling device_create_file()
> so that attributes exist before the uevent (as Kay stated earlier in
> this thread)?

People expect to be able to hook into device events and *match*
against certain device properties when the device shows up. Like you
can say: If a "usb_interface" with the "subclass" "video" shows up,
run this program, or do whatever else.

People expect to be able to hook into device events to *configure*
devices. They want to write to the tunables as soon as the device has
shown up, or the driver gets loaded. Like they want to enable timeouts
or power-management for specific devices.

Both *matching* and *configuring* devices at creation time are the key
to auto-setup and "just works" configurations.

If an attribute is not available at event time, nothing of this can
ever work. If you look a the device a second later, you will see the
proper looking files and values, and all the test programs will work,
but it will always fail at device creation.

You can hook into all that with a few simple instructions from udev,
and people can expect that device attributes they see at runtime can
be used in udev, means at creation time.

The same timing problem arises if a driver creates stuff at an already
existing device, userspace has no chance to ever get notified about
this. That's why the driver should usually create his own instance in
sysfs, that represents functionality, and will be a child device of
the hardware device it has bound to.
The child device that would be created, gets its "default attributes"
and they are always guaranteed to exist before the event is sent out.

That all might not be needed for your specific setup/driver/device and
may work fine for your need. But we don't want to encourage anybody
with another new API which creates the usual trouble we need to fix
later. And we need to fix things like this all the time.

Also the existing sysfs attribute groups API has the same problem if
the group is created later, but as far as I can see it should work
fine for your purpose. So it's better using that, instead of exporting
another different API.

Thanks,
Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

Hi Kay,

Thanks for the reply, it clarifies a lot.  It sounds like this really
should be documented (in Documentation/driver-model/device.txt?).  I'm
happy to add a blurb to that effect and send a patch, but I want to
make sure I really understand the model before I do so.  I've got a
couple more questions below:

On Thu, Feb 19, 2009 at 4:08 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
[...]
> If an attribute is not available at event time, nothing of this can
> ever work. If you look a the device a second later, you will see the
> proper looking files and values, and all the test programs will work,
> but it will always fail at device creation.
[...]
> That all might not be needed for your specific setup/driver/device and
> may work fine for your need. But we don't want to encourage anybody
> with another new API which creates the usual trouble we need to fix
> later.

Fair enough

> And we need to fix things like this all the time.

Can you point me at a 'textbook' example of one of these fixups?

> Also the existing sysfs attribute groups API has the same problem if
> the group is created later, but as far as I can see it should work
> fine for your purpose. So it's better using that, instead of exporting
> another different API.

Okay, I this option looks pretty straight forward too.

Thanks,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Greg KH]

On Fri, Feb 20, 2009 at 08:28:42AM -0700, Grant Likely wrote:
> Hi Kay,
> 
> Thanks for the reply, it clarifies a lot.  It sounds like this really
> should be documented (in Documentation/driver-model/device.txt?).  I'm
> happy to add a blurb to that effect and send a patch, but I want to
> make sure I really understand the model before I do so.

That would be wonderful to have, any documentation updates are gladly
accepted.

thanks,

greg k-h

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Kay Sievers]

On Fri, Feb 20, 2009 at 16:28, Grant Likely <grant.likely@secretlab.ca> wrote:
> Thanks for the reply, it clarifies a lot.  It sounds like this really
> should be documented (in Documentation/driver-model/device.txt?).  I'm
> happy to add a blurb to that effect and send a patch, but I want to
> make sure I really understand the model before I do so.  I've got a
> couple more questions below:
>
> On Thu, Feb 19, 2009 at 4:08 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> [...]
>> If an attribute is not available at event time, nothing of this can
>> ever work. If you look a the device a second later, you will see the
>> proper looking files and values, and all the test programs will work,
>> but it will always fail at device creation.
> [...]
>> That all might not be needed for your specific setup/driver/device and
>> may work fine for your need. But we don't want to encourage anybody
>> with another new API which creates the usual trouble we need to fix
>> later.
>
> Fair enough
>
>> And we need to fix things like this all the time.
>
> Can you point me at a 'textbook' example of one of these fixups?

Here are a few cases of broken atribute timing, a quick git search has found:
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f7120a4f75168df3c02efacd10403a4ba0bcb29d
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8a89efd18aa15bb832778baa4e6eee3857ecada4
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3b23dd6f8a718e5339de4f7d86ce76a078b5f771
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=2e5f10e4f0a9649186d8a8c793822b2e0dae8373
  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=bfd129445f23c037d9a440ebfa4109e11c220301

Regarding the split of the driver device and the device to bind to:
the platform stuff is kind of "special", because it is often used for
devices/buses which can not be enumerated. With buses which need
enumeration, it's pretty obvious that the enumeration creates their
own devices, which then a driver can bind to, and where the driver
creates its own driver instance for it. Like nobody would expect a
network driver to add the netif properties directly to the pci device,
and so on.

Many platform devices are just static placeholders, and then it is not
obvious that userspace still prefers another device instance while
binding a driver, but in most cases it's worth the effort, because
then all devices, regardless which bus is backing them, can be handled
the same way with a "hotplug handler" like udev.

Thanks,
Kay

Re: [PATCH] Export device_add_attributes() so drivers can use it.

[Grant Likely]

Thanks, this is exactly what I want.  I'll read through and write up a
little blurb to describe the recommended model.

g.


On Thu, Feb 26, 2009 at 3:28 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
> On Fri, Feb 20, 2009 at 16:28, Grant Likely <grant.likely@secretlab.ca> wrote:
>> Thanks for the reply, it clarifies a lot.  It sounds like this really
>> should be documented (in Documentation/driver-model/device.txt?).  I'm
>> happy to add a blurb to that effect and send a patch, but I want to
>> make sure I really understand the model before I do so.  I've got a
>> couple more questions below:
>>
>> On Thu, Feb 19, 2009 at 4:08 PM, Kay Sievers <kay.sievers@vrfy.org> wrote:
>> [...]
>>> If an attribute is not available at event time, nothing of this can
>>> ever work. If you look a the device a second later, you will see the
>>> proper looking files and values, and all the test programs will work,
>>> but it will always fail at device creation.
>> [...]
>>> That all might not be needed for your specific setup/driver/device and
>>> may work fine for your need. But we don't want to encourage anybody
>>> with another new API which creates the usual trouble we need to fix
>>> later.
>>
>> Fair enough
>>
>>> And we need to fix things like this all the time.
>>
>> Can you point me at a 'textbook' example of one of these fixups?
>
> Here are a few cases of broken atribute timing, a quick git search has found:
>  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=f7120a4f75168df3c02efacd10403a4ba0bcb29d
>  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=8a89efd18aa15bb832778baa4e6eee3857ecada4
>  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=3b23dd6f8a718e5339de4f7d86ce76a078b5f771
>  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=2e5f10e4f0a9649186d8a8c793822b2e0dae8373
>  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=bfd129445f23c037d9a440ebfa4109e11c220301
>
> Regarding the split of the driver device and the device to bind to:
> the platform stuff is kind of "special", because it is often used for
> devices/buses which can not be enumerated. With buses which need
> enumeration, it's pretty obvious that the enumeration creates their
> own devices, which then a driver can bind to, and where the driver
> creates its own driver instance for it. Like nobody would expect a
> network driver to add the netif properties directly to the pci device,
> and so on.
>
> Many platform devices are just static placeholders, and then it is not
> obvious that userspace still prefers another device instance while
> binding a driver, but in most cases it's worth the effort, because
> then all devices, regardless which bus is backing them, can be handled
> the same way with a "hotplug handler" like udev.
>
> Thanks,
> Kay
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.