[v12,7/7] nvmem: core: Expose cells through sysfs

  The binary content of nvmem devices is available to the user so in the
easiest cases, finding the content of a cell is rather easy as it is
just a matter of looking at a known and fixed offset. However, nvmem
layouts have been recently introduced to cope with more advanced
situations, where the offset and size of the cells is not known in
advance or is dynamic. When using layouts, more advanced parsers are
used by the kernel in order to give direct access to the content of each
cell, regardless of its position/size in the underlying
device. Unfortunately, these information are not accessible by users,
unless by fully re-implementing the parser logic in userland.

Let's expose the cells and their content through sysfs to avoid these
situations. Of course the relevant NVMEM sysfs Kconfig option must be
enabled for this support to be available.

Not all nvmem devices expose cells. Indeed, the .bin_attrs attribute
group member will be filled at runtime only when relevant and will
remain empty otherwise. In this case, as the cells attribute group will
be empty, it will not lead to any additional folder/file creation.

Exposed cells are read-only. There is, in practice, everything in the
core to support a write path, but as I don't see any need for that, I
prefer to keep the interface simple (and probably safer). The interface
is documented as being in the "testing" state which means we can later
add a write attribute if though relevant.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Tested-by: Rafał Miłecki <rafal@milecki.pl>
Tested-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/nvmem/core.c      | 131 +++++++++++++++++++++++++++++++++++++-
 drivers/nvmem/internals.h |   1 +
 2 files changed, 131 insertions(+), 1 deletion(-)
  

On 05/10/2023 16:59, Miquel Raynal wrote:
> , int index);
> +
> +static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
> +				    struct bin_attribute *attr, char *buf,
> +				    loff_t pos, size_t count)
> +{
> +	struct nvmem_cell_entry *entry;
> +	struct nvmem_cell *cell = NULL;
> +	size_t cell_sz, read_len;
> +	void *content;
> +
> +	entry = attr->private;
> +	cell = nvmem_create_cell(entry, entry->name, 0);
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	if (!cell)
> +		return -EINVAL;
> +
> +	content = nvmem_cell_read(cell, &cell_sz);
> +	if (IS_ERR(content)) {
> +		read_len = PTR_ERR(content);
> +		goto destroy_cell;
> +	}
> +
> +	read_len = min_t(unsigned int, cell_sz - pos, count);
> +	memcpy(buf, content + pos, read_len);
> +	kfree(content);
> +
> +destroy_cell:
> +	kfree_const(cell->id);
> +	kfree(cell);
> +
> +	return read_len;
> +}
> +
>   /* default read/write permissions */
>   static struct bin_attribute bin_attr_rw_nvmem = {
>   	.attr	= {
> @@ -320,11 +357,21 @@ static const struct attribute_group nvmem_bin_group = {
>   	.is_bin_visible = nvmem_bin_attr_is_visible,
>   };
>   
> +/* 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,
>   };
>   
> +static const struct attribute_group *nvmem_cells_groups[] = {
> +	&nvmem_cells_group,
> +	NULL,
> +};
> +
>   static struct bin_attribute bin_attr_nvmem_eeprom_compat = {
>   	.attr	= {
>   		.name	= "eeprom",
> @@ -380,6 +427,68 @@ static void nvmem_sysfs_remove_compat(struct nvmem_device *nvmem,
>   		device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
>   }
>   
> +static int nvmem_populate_sysfs_cells(struct nvmem_device *nvmem)
> +{
> +	struct bin_attribute **cells_attrs, *attrs;
> +	struct nvmem_cell_entry *entry;
> +	unsigned int ncells = 0, i = 0;
> +	int ret = 0;
> +
> +	mutex_lock(&nvmem_mutex);
> +
> +	if (list_empty(&nvmem->cells) || nvmem->sysfs_cells_populated) {
> +		nvmem_cells_group.bin_attrs = NULL;
> +		goto unlock_mutex;
> +	}
> +
> +	/* Allocate an array of attributes with a sentinel */
> +	ncells = list_count_nodes(&nvmem->cells);
> +	cells_attrs = devm_kcalloc(&nvmem->dev, ncells + 1,
> +				   sizeof(struct bin_attribute *), GFP_KERNEL);
> +	if (!cells_attrs) {
> +		ret = -ENOMEM;
> +		goto unlock_mutex;
> +	}
> +
> +	attrs = devm_kcalloc(&nvmem->dev, ncells, sizeof(struct bin_attribute), GFP_KERNEL);
> +	if (!attrs) {
> +		ret = -ENOMEM;
> +		goto unlock_mutex;
> +	}
> +
> +	/* Initialize each attribute to take the name and size of the cell */
> +	list_for_each_entry(entry, &nvmem->cells, node) {
> +		sysfs_bin_attr_init(&attrs[i]);
> +		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
> +						    "%s@%x", entry->name,
> +						    entry->offset);
> +		attrs[i].attr.mode = 0444;
> +		attrs[i].size = entry->bytes;
> +		attrs[i].read = &nvmem_cell_attr_read;
> +		attrs[i].private = entry;
> +		if (!attrs[i].attr.name) {
> +			ret = -ENOMEM;
> +			goto unlock_mutex;
> +		}
> +
> +		cells_attrs[i] = &attrs[i];
> +		i++;
> +	}
> +
> +	nvmem_cells_group.bin_attrs = cells_attrs;
> +
> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> +	if (ret)
> +		goto unlock_mutex;

This is going to create groups after the nvmem device is added, isn't 
this going to be problem with user space notifications?

--srini
> +
> +	nvmem->sysfs_cells_populated = true;
> +
> +unlock_mutex:
> +	mutex_unlock(&nvmem_mutex);
> +
> +	return ret;
  

Hi Srinivas,

> > +{
> > +	struct bin_attribute **cells_attrs, *attrs;
> > +	struct nvmem_cell_entry *entry;
> > +	unsigned int ncells = 0, i = 0;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&nvmem_mutex);
> > +
> > +	if (list_empty(&nvmem->cells) || nvmem->sysfs_cells_populated) {
> > +		nvmem_cells_group.bin_attrs = NULL;
> > +		goto unlock_mutex;
> > +	}
> > +
> > +	/* Allocate an array of attributes with a sentinel */
> > +	ncells = list_count_nodes(&nvmem->cells);
> > +	cells_attrs = devm_kcalloc(&nvmem->dev, ncells + 1,
> > +				   sizeof(struct bin_attribute *), GFP_KERNEL);
> > +	if (!cells_attrs) {
> > +		ret = -ENOMEM;
> > +		goto unlock_mutex;
> > +	}
> > +
> > +	attrs = devm_kcalloc(&nvmem->dev, ncells, sizeof(struct bin_attribute), GFP_KERNEL);
> > +	if (!attrs) {
> > +		ret = -ENOMEM;
> > +		goto unlock_mutex;
> > +	}
> > +
> > +	/* Initialize each attribute to take the name and size of the cell */
> > +	list_for_each_entry(entry, &nvmem->cells, node) {
> > +		sysfs_bin_attr_init(&attrs[i]);
> > +		attrs[i].attr.name = devm_kasprintf(&nvmem->dev, GFP_KERNEL,
> > +						    "%s@%x", entry->name,
> > +						    entry->offset);
> > +		attrs[i].attr.mode = 0444;
> > +		attrs[i].size = entry->bytes;
> > +		attrs[i].read = &nvmem_cell_attr_read;
> > +		attrs[i].private = entry;
> > +		if (!attrs[i].attr.name) {
> > +			ret = -ENOMEM;
> > +			goto unlock_mutex;
> > +		}
> > +
> > +		cells_attrs[i] = &attrs[i];
> > +		i++;
> > +	}
> > +
> > +	nvmem_cells_group.bin_attrs = cells_attrs;
> > +
> > +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> > +	if (ret)
> > +		goto unlock_mutex;  
> 
> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?

Greg said it was not. I hope I understood correctly :-)

And anyway, cells have never been available to userspace, so there is
nothing userspace might expect yet?

Thanks,
Miquèl
  

On 11/10/2023 08:15, Miquel Raynal wrote:
>>> +
>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
>>> +
>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
>>> +	if (ret)
>>> +		goto unlock_mutex;
>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?
> Greg said it was not. I hope I understood correctly 😄
> 
> And anyway, cells have never been available to userspace, so there is
> nothing userspace might expect yet?
I agree, but once we add sysfs uapi then this is going to change.

--srini


> 
> Thanks,
> Miquèl
  

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:

> On 11/10/2023 08:15, Miquel Raynal wrote:
> >>> +
> >>> +	nvmem_cells_group.bin_attrs = cells_attrs;
> >>> +
> >>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> >>> +	if (ret)
> >>> +		goto unlock_mutex;  
> >> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?  
> > Greg said it was not. I hope I understood correctly 😄
> > 
> > And anyway, cells have never been available to userspace, so there is
> > nothing userspace might expect yet?  
> I agree, but once we add sysfs uapi then this is going to change.

Can you elaborate? I'm not sure I follow you here. Is there still a
problem you fear or you think it's okay?

Thanks,
Miquèl
  

On 11/10/2023 09:33, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:
> 
>> On 11/10/2023 08:15, Miquel Raynal wrote:
>>>>> +
>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
>>>>> +
>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
>>>>> +	if (ret)
>>>>> +		goto unlock_mutex;
>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?
>>> Greg said it was not. I hope I understood correctly 😄
>>>
>>> And anyway, cells have never been available to userspace, so there is
>>> nothing userspace might expect yet?
>> I agree, but once we add sysfs uapi then this is going to change.
> 
> Can you elaborate? I'm not sure I follow you here. Is there still a
> problem you fear or you think it's okay?
> 
Now that we add cells to sysfs.
AFAIU, By the time the userspace sees the udev event from this device we 
might not have cells populated.

--srini


> Thanks,
> Miquèl
  

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:

> On 11/10/2023 09:33, Miquel Raynal wrote:
> > Hi Srinivas,
> > 
> > srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:
> >   
> >> On 11/10/2023 08:15, Miquel Raynal wrote:  
> >>>>> +
> >>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
> >>>>> +
> >>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> >>>>> +	if (ret)
> >>>>> +		goto unlock_mutex;  
> >>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?  
> >>> Greg said it was not. I hope I understood correctly 😄
> >>>
> >>> And anyway, cells have never been available to userspace, so there is
> >>> nothing userspace might expect yet?  
> >> I agree, but once we add sysfs uapi then this is going to change.  
> > 
> > Can you elaborate? I'm not sure I follow you here. Is there still a
> > problem you fear or you think it's okay?
> >   
> Now that we add cells to sysfs.
> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.

Yes, but why would this be a problem?

Thanks,
Miquèl
  

On 11/10/2023 09:58, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:
> 
>> On 11/10/2023 09:33, Miquel Raynal wrote:
>>> Hi Srinivas,
>>>
>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:
>>>    
>>>> On 11/10/2023 08:15, Miquel Raynal wrote:
>>>>>>> +
>>>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
>>>>>>> +
>>>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
>>>>>>> +	if (ret)
>>>>>>> +		goto unlock_mutex;
>>>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?
>>>>> Greg said it was not. I hope I understood correctly 😄
>>>>>
>>>>> And anyway, cells have never been available to userspace, so there is
>>>>> nothing userspace might expect yet?
>>>> I agree, but once we add sysfs uapi then this is going to change.
>>>
>>> Can you elaborate? I'm not sure I follow you here. Is there still a
>>> problem you fear or you think it's okay?
>>>    
>> Now that we add cells to sysfs.
>> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.
> 
> Yes, but why would this be a problem?
> 
It will be problem if the userspace is using things like libudev to act 
on these events. There seems to be some caching of attributes in udev 
during event more info 
http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/


--srini

> Thanks,
> Miquèl
  

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 10:26:43 +0100:

> On 11/10/2023 09:58, Miquel Raynal wrote:
> > Hi Srinivas,
> > 
> > srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:
> >   
> >> On 11/10/2023 09:33, Miquel Raynal wrote:  
> >>> Hi Srinivas,
> >>>
> >>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:  
> >>>    >>>> On 11/10/2023 08:15, Miquel Raynal wrote:
> >>>>>>> +
> >>>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
> >>>>>>> +
> >>>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> >>>>>>> +	if (ret)
> >>>>>>> +		goto unlock_mutex;  
> >>>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?  
> >>>>> Greg said it was not. I hope I understood correctly 😄
> >>>>>
> >>>>> And anyway, cells have never been available to userspace, so there is
> >>>>> nothing userspace might expect yet?  
> >>>> I agree, but once we add sysfs uapi then this is going to change.  
> >>>
> >>> Can you elaborate? I'm not sure I follow you here. Is there still a
> >>> problem you fear or you think it's okay?  
> >>>    >> Now that we add cells to sysfs.  
> >> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.  
> > 
> > Yes, but why would this be a problem?
> >   
> It will be problem if the userspace is using things like libudev to act on these events. There seems to be some caching of attributes in udev during event more info http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

I am already using these attributes, right? The problem here is that we
always attach cells sysfs attributes to the nvmem device, but in some
cases (when using layout devices/drivers) the probe of these devices
will happen after the main nvmem device has been announced to userspace
and thus these attributes might not be populated yet (but Greg said it
was "supported" and I assumed it was fine).

So what is your idea here to overcome this?

Thanks,
Miquèl
  

Hi Miquel,

On 11/10/2023 10:44, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 10:26:43 +0100:
> 
>> On 11/10/2023 09:58, Miquel Raynal wrote:
>>> Hi Srinivas,
>>>
>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:
>>>    
>>>> On 11/10/2023 09:33, Miquel Raynal wrote:
>>>>> Hi Srinivas,
>>>>>
>>>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:
>>>>>     >>>> On 11/10/2023 08:15, Miquel Raynal wrote:
>>>>>>>>> +
>>>>>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
>>>>>>>>> +
>>>>>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
>>>>>>>>> +	if (ret)
>>>>>>>>> +		goto unlock_mutex;
>>>>>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?
>>>>>>> Greg said it was not. I hope I understood correctly 😄
>>>>>>>
>>>>>>> And anyway, cells have never been available to userspace, so there is
>>>>>>> nothing userspace might expect yet?
>>>>>> I agree, but once we add sysfs uapi then this is going to change.
>>>>>
>>>>> Can you elaborate? I'm not sure I follow you here. Is there still a
>>>>> problem you fear or you think it's okay?
>>>>>     >> Now that we add cells to sysfs.
>>>> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.
>>>
>>> Yes, but why would this be a problem?
>>>    
>> It will be problem if the userspace is using things like libudev to act on these events. There seems to be some caching of attributes in udev during event more info http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
> 
> I am already using these attributes, right? The problem here is that we
> always attach cells sysfs attributes to the nvmem device, but in some
> cases (when using layout devices/drivers) the probe of these devices
> will happen after the main nvmem device has been announced to userspace
> and thus these attributes might not be populated yet (but Greg said it
> was "supported" and I assumed it was fine).
>  > So what is your idea here to overcome this?

Ideally we should have all the cells definitions ready by the time nvmem 
is registered.

--srini

> 
> Thanks,
> Miquèl
  

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 11:02:16 +0100:

> Hi Miquel,
> 
> On 11/10/2023 10:44, Miquel Raynal wrote:
> > Hi Srinivas,
> > 
> > srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 10:26:43 +0100:
> >   
> >> On 11/10/2023 09:58, Miquel Raynal wrote:  
> >>> Hi Srinivas,
> >>>
> >>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:  
> >>>    >>>> On 11/10/2023 09:33, Miquel Raynal wrote:  
> >>>>> Hi Srinivas,
> >>>>>
> >>>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:  
> >>>>>     >>>> On 11/10/2023 08:15, Miquel Raynal wrote:
> >>>>>>>>> +
> >>>>>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
> >>>>>>>>> +
> >>>>>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> >>>>>>>>> +	if (ret)
> >>>>>>>>> +		goto unlock_mutex;  
> >>>>>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?  
> >>>>>>> Greg said it was not. I hope I understood correctly 😄
> >>>>>>>
> >>>>>>> And anyway, cells have never been available to userspace, so there is
> >>>>>>> nothing userspace might expect yet?  
> >>>>>> I agree, but once we add sysfs uapi then this is going to change.  
> >>>>>
> >>>>> Can you elaborate? I'm not sure I follow you here. Is there still a
> >>>>> problem you fear or you think it's okay?  
> >>>>>     >> Now that we add cells to sysfs.  
> >>>> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.  
> >>>
> >>> Yes, but why would this be a problem?  
> >>>    >> It will be problem if the userspace is using things like libudev to act on these events. There seems to be some caching of attributes in udev during event more info http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/  
> > 
> > I am already using these attributes, right? The problem here is that we
> > always attach cells sysfs attributes to the nvmem device, but in some
> > cases (when using layout devices/drivers) the probe of these devices
> > will happen after the main nvmem device has been announced to userspace
> > and thus these attributes might not be populated yet (but Greg said it
> > was "supported" and I assumed it was fine).  
> >  > So what is your idea here to overcome this?  
> 
> Ideally we should have all the cells definitions ready by the time nvmem is registered.

I no longer think what you describe can happen because even though the
rootfs might be mounted, the daemons will only be 'started' once the
kernel is done starting and starts the init process, which means all
the devices have probed and all the cells have been registered as well.

Thanks,
Miquèl
  

On 11/10/2023 12:09, Miquel Raynal wrote:
> Hi Srinivas,
> 
> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 11:02:16 +0100:
> 
>> Hi Miquel,
>>
>> On 11/10/2023 10:44, Miquel Raynal wrote:
>>> Hi Srinivas,
>>>
>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 10:26:43 +0100:
>>>    
>>>> On 11/10/2023 09:58, Miquel Raynal wrote:
>>>>> Hi Srinivas,
>>>>>
>>>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:
>>>>>     >>>> On 11/10/2023 09:33, Miquel Raynal wrote:
>>>>>>> Hi Srinivas,
>>>>>>>
>>>>>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:
>>>>>>>      >>>> On 11/10/2023 08:15, Miquel Raynal wrote:
>>>>>>>>>>> +
>>>>>>>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
>>>>>>>>>>> +
>>>>>>>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
>>>>>>>>>>> +	if (ret)
>>>>>>>>>>> +		goto unlock_mutex;
>>>>>>>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?
>>>>>>>>> Greg said it was not. I hope I understood correctly 😄
>>>>>>>>>
>>>>>>>>> And anyway, cells have never been available to userspace, so there is
>>>>>>>>> nothing userspace might expect yet?
>>>>>>>> I agree, but once we add sysfs uapi then this is going to change.
>>>>>>>
>>>>>>> Can you elaborate? I'm not sure I follow you here. Is there still a
>>>>>>> problem you fear or you think it's okay?
>>>>>>>      >> Now that we add cells to sysfs.
>>>>>> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.
>>>>>
>>>>> Yes, but why would this be a problem?
>>>>>     >> It will be problem if the userspace is using things like libudev to act on these events. There seems to be some caching of attributes in udev during event more info http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/
>>>
>>> I am already using these attributes, right? The problem here is that we
>>> always attach cells sysfs attributes to the nvmem device, but in some
>>> cases (when using layout devices/drivers) the probe of these devices
>>> will happen after the main nvmem device has been announced to userspace
>>> and thus these attributes might not be populated yet (but Greg said it
>>> was "supported" and I assumed it was fine).
>>>   > So what is your idea here to overcome this?
>>
>> Ideally we should have all the cells definitions ready by the time nvmem is registered.
> 
> I no longer think what you describe can happen because even though the
> rootfs might be mounted, the daemons will only be 'started' once the
> kernel is done starting and starts the init process, which means all
> the devices have probed and all the cells have been registered as well.
I think you forgot about modules in the above flow.

--srini


> 
> Thanks,
> Miquèl
  

Hi Srinivas,

srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 14:56:02 +0100:

> On 11/10/2023 12:09, Miquel Raynal wrote:
> > Hi Srinivas,
> > 
> > srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 11:02:16 +0100:
> >   
> >> Hi Miquel,
> >>
> >> On 11/10/2023 10:44, Miquel Raynal wrote:  
> >>> Hi Srinivas,
> >>>
> >>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 10:26:43 +0100:  
> >>>    >>>> On 11/10/2023 09:58, Miquel Raynal wrote:  
> >>>>> Hi Srinivas,
> >>>>>
> >>>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:45:11 +0100:  
> >>>>>     >>>> On 11/10/2023 09:33, Miquel Raynal wrote:  
> >>>>>>> Hi Srinivas,
> >>>>>>>
> >>>>>>> srinivas.kandagatla@linaro.org wrote on Wed, 11 Oct 2023 09:27:20 +0100:  
> >>>>>>>      >>>> On 11/10/2023 08:15, Miquel Raynal wrote:
> >>>>>>>>>>> +
> >>>>>>>>>>> +	nvmem_cells_group.bin_attrs = cells_attrs;
> >>>>>>>>>>> +
> >>>>>>>>>>> +	ret = devm_device_add_groups(&nvmem->dev, nvmem_cells_groups);
> >>>>>>>>>>> +	if (ret)
> >>>>>>>>>>> +		goto unlock_mutex;  
> >>>>>>>>>> This is going to create groups after the nvmem device is added, isn't this going to be problem with user space notifications?  
> >>>>>>>>> Greg said it was not. I hope I understood correctly 😄
> >>>>>>>>>
> >>>>>>>>> And anyway, cells have never been available to userspace, so there is
> >>>>>>>>> nothing userspace might expect yet?  
> >>>>>>>> I agree, but once we add sysfs uapi then this is going to change.  
> >>>>>>>
> >>>>>>> Can you elaborate? I'm not sure I follow you here. Is there still a
> >>>>>>> problem you fear or you think it's okay?  
> >>>>>>>      >> Now that we add cells to sysfs.  
> >>>>>> AFAIU, By the time the userspace sees the udev event from this device we might not have cells populated.  
> >>>>>
> >>>>> Yes, but why would this be a problem?  
> >>>>>     >> It will be problem if the userspace is using things like libudev to act on these events. There seems to be some caching of attributes in udev during event more info http://www.kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/  
> >>>
> >>> I am already using these attributes, right? The problem here is that we
> >>> always attach cells sysfs attributes to the nvmem device, but in some
> >>> cases (when using layout devices/drivers) the probe of these devices
> >>> will happen after the main nvmem device has been announced to userspace
> >>> and thus these attributes might not be populated yet (but Greg said it
> >>> was "supported" and I assumed it was fine).  
> >>>   > So what is your idea here to overcome this?  
> >>
> >> Ideally we should have all the cells definitions ready by the time nvmem is registered.  
> > 
> > I no longer think what you describe can happen because even though the
> > rootfs might be mounted, the daemons will only be 'started' once the
> > kernel is done starting and starts the init process, which means all
> > the devices have probed and all the cells have been registered as well.  
> I think you forgot about modules in the above flow.

We request module insertion when the layout gets populated. By the time
userspace starts the kernel is done initializing, meaning that any
available nvmem layout module has already been loaded and the devices
probed -> the cells are there already.

Thanks,
Miquèl