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@xxxxxxxxxxxx> wrote:
> On Wed, Feb 18, 2009 at 9:49 AM, Kay Sievers <kay.sievers@xxxxxxxx> wrote:
>> On Wed, Feb 18, 2009 at 17:34, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
>>> On Wed, Feb 18, 2009 at 8:53 AM, Kay Sievers <kay.sievers@xxxxxxxx> wrote:
>>>> On Wed, Feb 18, 2009 at 16:48, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
>>>>> On Wed, Feb 18, 2009 at 8:45 AM, Kay Sievers <kay.sievers@xxxxxxxx> wrote:
>>>>>> On Wed, Feb 18, 2009 at 16:29, Greg KH <gregkh@xxxxxxx> wrote:
>>>>>>> On Wed, Feb 18, 2009 at 08:11:34AM -0700, Grant Likely wrote:
>>>>>>>> From: Grant Likely <grant.likely@xxxxxxxxxxxx>
>>>>>>>>
>>>>>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/