Sysfs - export sysfs_create_subdir?

Sysfs - export sysfs_create_subdir?

[Jonathan Cameron]

Hi All,

I have a couple of cases in IIO where under some conditions
we end up using dummy attr_groups (no elements) to initialize
a sub directory before dynamically creating all of its attributes.

Now having dummy groups in the IIO core is fine, but it does seem
a little messy.

The obvious choice would be to put together a small wrapper function
for sysfs_create_subdir that also does sysfs_get on the result.

Anyone have any strong feelings on whether this is worthwhile or not?

It'll save me about 16 lines of code, so not exactly a big point,
but nice to get it right none the less!

If there is a way of doing this cleanly that I'm missing, then sorry
for the noise and please tell me what it is!

Jonathan

Re: Sysfs - export sysfs_create_subdir?

[Greg KH]

On Fri, Jul 22, 2011 at 06:43:13PM +0100, Jonathan Cameron wrote:
> Hi All,
> 
> I have a couple of cases in IIO where under some conditions
> we end up using dummy attr_groups (no elements) to initialize
> a sub directory before dynamically creating all of its attributes.

Ick, you really shouldn't do that.

> Now having dummy groups in the IIO core is fine, but it does seem
> a little messy.
> 
> The obvious choice would be to put together a small wrapper function
> for sysfs_create_subdir that also does sysfs_get on the result.

That's why that function is not exported.  Creating subdirectories in
sysfs is not a good idea, udev doesn't like it, and you don't get
userspace notification for it very well either.

What specifically are you trying to do in sysfs that you are needing
this?

thanks,

greg k-h

Re: Sysfs - export sysfs_create_subdir?

[Jonathan Cameron]

On 07/24/11 04:18, Greg KH wrote:
> On Fri, Jul 22, 2011 at 06:43:13PM +0100, Jonathan Cameron wrote:
>> Hi All,
>>
>> I have a couple of cases in IIO where under some conditions
>> we end up using dummy attr_groups (no elements) to initialize
>> a sub directory before dynamically creating all of its attributes.
> 
> Ick, you really shouldn't do that.
Agreed :)
> 
>> Now having dummy groups in the IIO core is fine, but it does seem
>> a little messy.
>>
>> The obvious choice would be to put together a small wrapper function
>> for sysfs_create_subdir that also does sysfs_get on the result.
> 

The sysfs_get issue still exists if we are creating all the elements in
the base directory (which we are in a few drivers - most have something
that isn't handled by the channel spec though).

> That's why that function is not exported.  Creating subdirectories in
> sysfs is not a good idea, udev doesn't like it, and you don't get
> userspace notification for it very well either.
> 
> What specifically are you trying to do in sysfs that you are needing
> this?
We are using it purely for organisation.  Note that when I said dynamically
creating all the attributes I meant at probe of the device.  We aren't
talking about creating subdirectories under any other conditions.

The primary reason is that the attributes in IIO fall into a couple of nice
clean groups and there can be an awful lot of them.

Hence taking an adis16400 as an example (from memory). Note the naming is based
on some more abi tweaks that are in progress.  I've also for reference added
the events side of things which this driver doesn't currently have (last patches
for this have long since bit rotted beyond being useful!).  Note this is
a moderately complex example.  There are devices with more channels, but this
has the greatest mix of anything currently in tree.

We have the following structure currently

iio:deviceX/  // top level directory containing the basic read parameters
              // for the channels + conversion factors (offsets calibration stuff
              // etc) + stuff that effects everything such as sampling frequency.

In this device that contains the following created from the chan_spec
structures.

in_voltage0_supply_raw
in_voltage0_supply_scale
in_accel_x_raw
in_accel_y_raw
in_accel_z_raw
in_accel_scale
in_gyro_x_raw
in_gyro_y_raw
in_gyro_z_raw
in_gyro_scale
in_magn_x_raw
in_magn_y_raw
in_magn_z_raw
in_magn_scale
in_temp0_raw
in_temp0_scale
in_temp0_offset
in_voltage1_raw
in_voltage1_scale

in this case we also have the following.

sampling_frequency
sampling_frequency_available
reset

iio:deviceX/buffer // control elements for the buffer - these depend on the selected
                   // buffer implementation - right now this driver only uses ring_sw,
                   // but others support kfifo and ring_sw.
                   // Note the device driver has nothing to do with these, they are
                   // provided by the buffer implementation.  There is no known reason
                   // why a device driver would need to add anything to here.
contains

length
bytes_per_datum
enable

iio:deviceX/scan_elements  // Control and description of 'scans'. These are the mass
                           // data reads that are pushed to the buffer on a trigger.  

in_voltage0_supply_en
in_voltage0_supply_type
in_gyro_x_en
in_gyro_x_type
in_gyro_y_en
in_gyro_y_type
in_gyro_z_en
in_gyro_z_type
in_accel_x_en
in_accel_x_type
in_accel_y_en
in_accel_y_type
in_accel_z_en
in_accel_z_type
in_magn_x_en
in_magn_x_type
in_magn_z_en
in_magn_z_type
in_voltage0_en
in_voltage0_type
in_timestamp_en
in_timestamp_type

All of these are only of interest if you are using the buffer.

If the events were implemented we would have

iio:deviceX/events //control of events (thresholds basically)

in_voltage0_supply_thresh_rising_value
in_voltage0_supply_thresh_rising_en
in_voltage0_supply_thresh_falling_value
in_voltage0_supply_thresh_falling_en
in_voltage0_supply_roc_rising_value
in_voltage0_supply_roc_rising_en
in_voltage0_supply_roc_falling_value
in_voltage0_supply_roc_falling_en

in_gyro_x_supply_thresh_rising_value
in_gyro_x_supply_thresh_rising_en
in_gyro_x_supply_thresh_falling_value
in_gyro_x_supply_thresh_falling_en
in_gyro_x_supply_roc_rising_value
in_gyro_x_supply_roc_rising_en
in_gyro_x_supply_roc_falling_value
in_gyro_x_supply_roc_falling_en

etc for all the other channels so 88 attrs in here.

These are only relevant if you are interested in events.  There are
occasionaly reasons for other elements in this directory not created
from the channel spec (for example if the event monitoring sampling
frequency is separate from the main one), but in most cases this
directory is entirely created from our channel description structures.

Note as I said above all of these are created at probe time and
correspond directly to the equivalent:

static const attribute adis16400_event_attrs[] = {
};

static const attribute_group adis16400_event_group {
       .name = "events",
       .attrs = adis16400_event_attrs,
};

This is more obvious after the recent flattenning of our
tree as a result of combining the previous 3 chrdevs per
complex device into one.

(as an aside there is also a triggerX device for the adis16400
data ready, but that is nice and simple and conventional).

We could flattern everything into one directory and do everything
with appropriate prefixes but that is really ugly.

So how should we do this?

Thanks,

Jonathan

p.s. Having just grepped our tree for various things whilst writing this I've
noticed some 'interesting' drivers that were setting attributes up in a directory
with DRV_NAME as the directory name.  Patch will follow to get rid of those...
Actually we aren't the only ones.  There are a couple of examples in misc as well..

Re: Sysfs - export sysfs_create_subdir?

[Jonathan Cameron]

As a heads up there is something very nasty going on that
looks vaguely like it might be connected to some of the hacks in here.
I'm trying to chase it down.

It's manifesting as a random issue if we don't have a dummy group
(which was a nasty hack that just happened to work). With the dummy
group it appears as a null pointer dereference in sysfs_remove_bin_file
as called ultimately from sys_delete_module (on module removal).

> On 07/24/11 04:18, Greg KH wrote:
>> On Fri, Jul 22, 2011 at 06:43:13PM +0100, Jonathan Cameron wrote:
>>> Hi All,
>>>
>>> I have a couple of cases in IIO where under some conditions
>>> we end up using dummy attr_groups (no elements) to initialize
>>> a sub directory before dynamically creating all of its attributes.
>>
>> Ick, you really shouldn't do that.
> Agreed :)
>>
>>> Now having dummy groups in the IIO core is fine, but it does seem
>>> a little messy.
>>>
>>> The obvious choice would be to put together a small wrapper function
>>> for sysfs_create_subdir that also does sysfs_get on the result.
>>
> 
> The sysfs_get issue still exists if we are creating all the elements in
> the base directory (which we are in a few drivers - most have something
> that isn't handled by the channel spec though).
> 
>> That's why that function is not exported.  Creating subdirectories in
>> sysfs is not a good idea, udev doesn't like it, and you don't get
>> userspace notification for it very well either.
>>
>> What specifically are you trying to do in sysfs that you are needing
>> this?
> We are using it purely for organisation.  Note that when I said dynamically
> creating all the attributes I meant at probe of the device.  We aren't
> talking about creating subdirectories under any other conditions.
> 
> The primary reason is that the attributes in IIO fall into a couple of nice
> clean groups and there can be an awful lot of them.
> 
> Hence taking an adis16400 as an example (from memory). Note the naming is based
> on some more abi tweaks that are in progress.  I've also for reference added
> the events side of things which this driver doesn't currently have (last patches
> for this have long since bit rotted beyond being useful!).  Note this is
> a moderately complex example.  There are devices with more channels, but this
> has the greatest mix of anything currently in tree.
> 
> We have the following structure currently
> 
> iio:deviceX/  // top level directory containing the basic read parameters
>               // for the channels + conversion factors (offsets calibration stuff
>               // etc) + stuff that effects everything such as sampling frequency.
> 
> In this device that contains the following created from the chan_spec
> structures.
> 
> in_voltage0_supply_raw
> in_voltage0_supply_scale
> in_accel_x_raw
> in_accel_y_raw
> in_accel_z_raw
> in_accel_scale
> in_gyro_x_raw
> in_gyro_y_raw
> in_gyro_z_raw
> in_gyro_scale
> in_magn_x_raw
> in_magn_y_raw
> in_magn_z_raw
> in_magn_scale
> in_temp0_raw
> in_temp0_scale
> in_temp0_offset
> in_voltage1_raw
> in_voltage1_scale
> 
> in this case we also have the following.
> 
> sampling_frequency
> sampling_frequency_available
> reset
> 
> iio:deviceX/buffer // control elements for the buffer - these depend on the selected
>                    // buffer implementation - right now this driver only uses ring_sw,
>                    // but others support kfifo and ring_sw.
>                    // Note the device driver has nothing to do with these, they are
>                    // provided by the buffer implementation.  There is no known reason
>                    // why a device driver would need to add anything to here.
> contains
> 
> length
> bytes_per_datum
> enable
> 
> iio:deviceX/scan_elements  // Control and description of 'scans'. These are the mass
>                            // data reads that are pushed to the buffer on a trigger.  
> 
> in_voltage0_supply_en
> in_voltage0_supply_type
> in_gyro_x_en
> in_gyro_x_type
> in_gyro_y_en
> in_gyro_y_type
> in_gyro_z_en
> in_gyro_z_type
> in_accel_x_en
> in_accel_x_type
> in_accel_y_en
> in_accel_y_type
> in_accel_z_en
> in_accel_z_type
> in_magn_x_en
> in_magn_x_type
> in_magn_z_en
> in_magn_z_type
> in_voltage0_en
> in_voltage0_type
> in_timestamp_en
> in_timestamp_type
> 
> All of these are only of interest if you are using the buffer.
> 
> If the events were implemented we would have
> 
> iio:deviceX/events //control of events (thresholds basically)
> 
> in_voltage0_supply_thresh_rising_value
> in_voltage0_supply_thresh_rising_en
> in_voltage0_supply_thresh_falling_value
> in_voltage0_supply_thresh_falling_en
> in_voltage0_supply_roc_rising_value
> in_voltage0_supply_roc_rising_en
> in_voltage0_supply_roc_falling_value
> in_voltage0_supply_roc_falling_en
> 
> in_gyro_x_supply_thresh_rising_value
> in_gyro_x_supply_thresh_rising_en
> in_gyro_x_supply_thresh_falling_value
> in_gyro_x_supply_thresh_falling_en
> in_gyro_x_supply_roc_rising_value
> in_gyro_x_supply_roc_rising_en
> in_gyro_x_supply_roc_falling_value
> in_gyro_x_supply_roc_falling_en
> 
> etc for all the other channels so 88 attrs in here.
> 
> These are only relevant if you are interested in events.  There are
> occasionaly reasons for other elements in this directory not created
> from the channel spec (for example if the event monitoring sampling
> frequency is separate from the main one), but in most cases this
> directory is entirely created from our channel description structures.
> 
> Note as I said above all of these are created at probe time and
> correspond directly to the equivalent:
> 
> static const attribute adis16400_event_attrs[] = {
> };
> 
> static const attribute_group adis16400_event_group {
>        .name = "events",
>        .attrs = adis16400_event_attrs,
> };
> 
> This is more obvious after the recent flattenning of our
> tree as a result of combining the previous 3 chrdevs per
> complex device into one.
> 
> (as an aside there is also a triggerX device for the adis16400
> data ready, but that is nice and simple and conventional).
> 
> We could flattern everything into one directory and do everything
> with appropriate prefixes but that is really ugly.
> 
> So how should we do this?
> 
> Thanks,
> 
> Jonathan
> 
> p.s. Having just grepped our tree for various things whilst writing this I've
> noticed some 'interesting' drivers that were setting attributes up in a directory
> with DRV_NAME as the directory name.  Patch will follow to get rid of those...
> Actually we aren't the only ones.  There are a couple of examples in misc as well..