Re: [PATCH 2/2] nvmem: core: Expose cells through sysfs

[Greg Kroah-Hartman]

On Mon, May 29, 2023 at 03:35:39PM +0200, Miquel Raynal wrote:
> Hi Greg,
> 
> gregkh@xxxxxxxxxxxxxxxxxxx wrote on Mon, 29 May 2023 14:05:30 +0100:
> 
> > On Mon, May 29, 2023 at 12:12:26PM +0200, Miquel Raynal wrote:
> > > Hi Greg,
> > > 
> > > miquel.raynal@xxxxxxxxxxx wrote on Tue, 23 May 2023 19:14:02 +0200:
> > >   
> > > > Hi Greg,
> > > > 
> > > > gregkh@xxxxxxxxxxxxxxxxxxx wrote on Tue, 23 May 2023 17:58:51 +0100:
> > > >   
> > > > > On Tue, May 23, 2023 at 12:02:39PM +0200, Miquel Raynal wrote:    
> > > > > > +/* Cell attributes will be dynamically allocated */
> > > > > > +static struct attribute_group nvmem_cells_group = {
> > > > > > +	.name		= "cells",
> > > > > > +};
> > > > > > +
> > > > > >  static const struct attribute_group *nvmem_dev_groups[] = {
> > > > > >  	&nvmem_bin_group,
> > > > > > +	NULL, /* Reserved for exposing cells, if any */      
> > > > > 
> > > > > Please don't do this, but rather use the is_visible callback to
> > > > > determine if it should be shown or not.    
> > > > 
> > > > Ah, excellent point. Don't know why I overlooked that member.  
> > > 
> > > Actually, the .is_visible callback only acts on the files and
> > > not the directories (created based on the group name).  
> > 
> > That is true, I have a non-working patch somewhere around here that will
> > not create the directory if no files are in that directory, and need to
> > get that working someday...
> 
> I see, indeed that would be a nice addition.
> 
> > > This
> > > means whether they are visible or not, the attributes must be
> > > valid, the nvmem core cannot just toggle a boolean value with
> > > .is_visible because the sysfs core makes a number of checks
> > > regarding the content of the attributes, without checking if
> > > they are visible at all.  
> > 
> > You can't toggle a value, that's not how is_visible works.  It's a
> > callback at the creation time, you do know if you should, or should not,
> > show the files at creation time, right?
> 
> I think I should be able to do that.
> 
> > If so, all should be fine, just ignore the empty directory, it's fine.
> 
> Ok.
> 
> > And hopefully one day, it will not be created if there are no files in
> > it.  If I can ever get that patch working...
> > 
> > > I can however expose the "cells" bin group by default by having
> > > it listed in the static bin_attribute list and discard it by
> > > overwriting the list member with NULL (ie. the opposite of the current
> > > solution).  
> > 
> > Ick, no, please don't do that.  attribute lists should be able to be put
> > into read-only memory, and are not set up to be dynamically messed with
> > like this at all.
> 
> I get the idea, makes sense.
> 
> However in my case, the list of attribute groups can lay into RO
> memory, that's fine, but the attribute group for the cells
> will not. Indeed, the .bin_attrs list must be populated at run time
> as we do not know it's size at compilation time.

Ick.  I thought we had a way to update the size of a sysfs file
dynamically but I guess not.  So that's ok for now.

thanks,

greg k-h