[PATCH] staging: most: do not show interface dependent attrs by default in sysfs

[Greg KH]

On Wed, Aug 08, 2018 at 04:51:26PM +0200, Christian Gromm wrote:
> On 08.08.2018 14:10, Greg KH wrote:
> > On Mon, Aug 06, 2018 at 12:03:10PM +0200, Christian Gromm wrote:
> > > The channel attribute dbr_size is only relevant for the DIM2 interface. So
> > > is the packets_per_xact for USB. Currently, all attrs are shown by default
> > > in sysfs for any channel. To get a clean content of a channel directory,
> > > this patch makes the attributes show up only on the channel they belong to.
> > > 
> > > Signed-off-by: Christian Gromm <christian.gromm at microchip.com>
> > > ---
> > >   drivers/staging/most/core.c      | 12 +++++++++---
> > >   drivers/staging/most/core.h      |  5 +++++
> > >   drivers/staging/most/dim2/dim2.c |  1 +
> > >   drivers/staging/most/usb/usb.c   |  1 +
> > >   4 files changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/most/core.c b/drivers/staging/most/core.c
> > > index f4c4646..19694a1 100644
> > > --- a/drivers/staging/most/core.c
> > > +++ b/drivers/staging/most/core.c
> > > @@ -25,6 +25,7 @@
> > >   #define MAX_CHANNELS	64
> > >   #define STRING_SIZE	80
> > > +#define MAX_NUM_ATTRS	14
> > >   static struct ida mdev_id;
> > >   static int dummy_num_buffers;
> > > @@ -459,7 +460,7 @@ static DEVICE_ATTR_RW(set_subbuffer_size);
> > >   static DEVICE_ATTR_RW(set_packets_per_xact);
> > >   static DEVICE_ATTR_RW(set_dbr_size);
> > > -static struct attribute *channel_attrs[] = {
> > > +static struct attribute *channel_attrs[MAX_NUM_ATTRS] = {
> > 
> > Oh, this is ripe for abuse :)
> > 
> > >   	DEV_ATTR(available_directions),
> > >   	DEV_ATTR(available_datatypes),
> > >   	DEV_ATTR(number_of_packet_buffers),
> > > @@ -472,8 +473,6 @@ static struct attribute *channel_attrs[] = {
> > >   	DEV_ATTR(set_direction),
> > >   	DEV_ATTR(set_datatype),
> > >   	DEV_ATTR(set_subbuffer_size),
> > > -	DEV_ATTR(set_packets_per_xact),
> > > -	DEV_ATTR(set_dbr_size),
> > >   	NULL,
> > >   };
> > > @@ -1416,6 +1415,13 @@ int most_register_interface(struct most_interface *iface)
> > >   	iface->dev.init_name = iface->p->name;
> > >   	iface->dev.bus = &mc.bus;
> > >   	iface->dev.parent = &mc.dev;
> > > +	if (iface->extra_attrs == XACT_ATTRS) {
> > > +		channel_attrs[12] = DEV_ATTR(set_packets_per_xact);
> > > +		channel_attrs[13] = NULL;
> > > +	} else if (iface->extra_attrs == DBR_ATTRS) {
> > > +		channel_attrs[12] = DEV_ATTR(set_dbr_size);
> > > +		channel_attrs[13] = NULL;
> > > +	}
> > 
> > No, please use the proper way of doing this.  Your attribute can have a
> > callback when it is being created to test if it should be created or
> > not.  Use that, this is exactly what it is there for.
> > 
> Hmm, I don't see how to hook a custom callback to a device attribute
> as its structure does not provide any pointer to do so.
> 
> I went down the rabbit hole staring at "device_register", but
> I couldn't find any code that checks for such a function before
> adding the attributes to sysfs.
> 
> Can you please point out some driver code that makes use of such
> a callback you are talking about? That would be nice.

Look at the use of the "is_visible" callback the struct attribute_group.
That should help you out here.  The kerneldoc for it should give you
enough information to go off of.

greg k-h