diff mbox series

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

Message ID	20230523100239.307574-3-miquel.raynal@bootlin.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; sender: miquel.raynal@bootlin.com) by mail.gandi.net (Postfix) with ESMTPSA id 1D6351BF205; Tue, 23 May 2023 10:02:42 +0000 (UTC) From: Miquel Raynal <miquel.raynal@bootlin.com> To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Luka Perkov <luka.perkov@sartura.hr>, Robert Marko <robert.marko@sartura.hr>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, <linux-kernel@vger.kernel.org>, Miquel Raynal <miquel.raynal@bootlin.com> Subject: [PATCH 2/2] nvmem: core: Expose cells through sysfs Date: Tue, 23 May 2023 12:02:39 +0200 Message-Id: <20230523100239.307574-3-miquel.raynal@bootlin.com> In-Reply-To: <20230523100239.307574-1-miquel.raynal@bootlin.com> References: <20230523100239.307574-1-miquel.raynal@bootlin.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	NVMEM cells in sysfs \| [0/2] NVMEM cells in sysfs [1/2] ABI: sysfs-nvmem-cells: Expose cells through sysfs [2/2] nvmem: core: Expose cells through sysfs

Commit Message

Miquel Raynal May 23, 2023, 10:02 a.m. UTC

  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 their position/size in the underlying device, but
these information were not accessible to the user without re-writing the
parser logic in userland.

The current implementation only adds the 'cells' attributes if cells are
found within the device, otherwise the folder does not appear.

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.

Of course the relevant NVMEM sysfs Kconfig option must be enabled for
this support to be compiled-in.

There is one limitation though: if a layout is built as a module but is
not properly installed in the system and loaded manually with insmod
while the nvmem device driver was built-in, the cells won't appear in
sysfs. But if done like that, the cells won't be usable by the built-in
kernel drivers anyway.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/nvmem/core.c | 135 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 131 insertions(+), 4 deletions(-)

Comments

Greg KH May 23, 2023, 4:58 p.m. UTC | #1

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.

thanks,

greg k-h

Miquel Raynal May 23, 2023, 5:14 p.m. UTC | #2

Hi Greg,

gregkh@linuxfoundation.org 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.

Thanks,
Miquèl

kernel test robot May 23, 2023, 11:51 p.m. UTC | #3

Hi Miquel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.4-rc3 next-20230523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Miquel-Raynal/ABI-sysfs-nvmem-cells-Expose-cells-through-sysfs/20230523-203042
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20230523100239.307574-3-miquel.raynal%40bootlin.com
patch subject: [PATCH 2/2] nvmem: core: Expose cells through sysfs
config: arm-randconfig-r015-20230521
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/22c370b2163e592b9c7b2b1a29c080110dbad018
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Miquel-Raynal/ABI-sysfs-nvmem-cells-Expose-cells-through-sysfs/20230523-203042
        git checkout 22c370b2163e592b9c7b2b1a29c080110dbad018
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/nvmem/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305240724.z3McDuYM-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/nvmem/core.c:367:6: warning: variable 'read_len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (IS_ERR(content)) {
               ^~~~~~~~~~~~~~~
   drivers/nvmem/core.c:380:9: note: uninitialized use occurs here
           return read_len;
                  ^~~~~~~~
   drivers/nvmem/core.c:367:2: note: remove the 'if' if its condition is always false
           if (IS_ERR(content)) {
           ^~~~~~~~~~~~~~~~~~~~~~
   drivers/nvmem/core.c:338:26: note: initialize the variable 'read_len' to silence this warning
           size_t cell_sz, read_len;
                                   ^
                                    = 0
   1 warning generated.


vim +367 drivers/nvmem/core.c

   327	
   328	static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
   329						    const char *id, int index);
   330	
   331	static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
   332					    struct bin_attribute *attr, char *buf,
   333					    loff_t pos, size_t count)
   334	{
   335		struct nvmem_cell_entry *entry;
   336		struct nvmem_cell *cell = NULL;
   337		struct nvmem_device *nvmem;
   338		size_t cell_sz, read_len;
   339		struct device *dev;
   340		void *content;
   341	
   342		if (attr->private)
   343			dev = attr->private;
   344		else
   345			dev = kobj_to_dev(kobj);
   346		nvmem = to_nvmem_device(dev);
   347	
   348		mutex_lock(&nvmem_mutex);
   349		list_for_each_entry(entry, &nvmem->cells, node) {
   350			if (strncmp(entry->name, attr->attr.name, XATTR_NAME_MAX))
   351				continue;
   352	
   353			cell = nvmem_create_cell(entry, entry->name, 0);
   354			if (IS_ERR(cell)) {
   355				mutex_unlock(&nvmem_mutex);
   356				return PTR_ERR(cell);
   357			}
   358	
   359			break;
   360		}
   361		mutex_unlock(&nvmem_mutex);
   362	
   363		if (!cell)
   364			return -EINVAL;
   365	
   366		content = nvmem_cell_read(cell, &cell_sz);
 > 367		if (IS_ERR(content)) {
   368			count = PTR_ERR(content);
   369			goto destroy_cell;
   370		}
   371	
   372		read_len = min_t(unsigned int, cell_sz - pos, count);
   373		memcpy(buf, content + pos, read_len);
   374		kfree(content);
   375	
   376	destroy_cell:
   377		kfree_const(cell->id);
   378		kfree(cell);
   379	
   380		return read_len;
   381	}
   382

Dan Carpenter May 29, 2023, 4:28 a.m. UTC | #4

Hi Miquel,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Miquel-Raynal/ABI-sysfs-nvmem-cells-Expose-cells-through-sysfs/20230523-203042
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20230523100239.307574-3-miquel.raynal%40bootlin.com
patch subject: [PATCH 2/2] nvmem: core: Expose cells through sysfs
config: i386-randconfig-m021-20230525 (https://download.01.org/0day-ci/archive/20230528/202305280054.NloN5RLk-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202305280054.NloN5RLk-lkp@intel.com/

smatch warnings:
drivers/nvmem/core.c:380 nvmem_cell_attr_read() error: uninitialized symbol 'read_len'.

vim +/read_len +380 drivers/nvmem/core.c

22c370b2163e59 Miquel Raynal       2023-05-23  328  static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
22c370b2163e59 Miquel Raynal       2023-05-23  329  					    const char *id, int index);
22c370b2163e59 Miquel Raynal       2023-05-23  330  
22c370b2163e59 Miquel Raynal       2023-05-23  331  static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
22c370b2163e59 Miquel Raynal       2023-05-23  332  				    struct bin_attribute *attr, char *buf,
22c370b2163e59 Miquel Raynal       2023-05-23  333  				    loff_t pos, size_t count)
22c370b2163e59 Miquel Raynal       2023-05-23  334  {
22c370b2163e59 Miquel Raynal       2023-05-23  335  	struct nvmem_cell_entry *entry;
22c370b2163e59 Miquel Raynal       2023-05-23  336  	struct nvmem_cell *cell = NULL;
22c370b2163e59 Miquel Raynal       2023-05-23  337  	struct nvmem_device *nvmem;
22c370b2163e59 Miquel Raynal       2023-05-23  338  	size_t cell_sz, read_len;
22c370b2163e59 Miquel Raynal       2023-05-23  339  	struct device *dev;
22c370b2163e59 Miquel Raynal       2023-05-23  340  	void *content;
22c370b2163e59 Miquel Raynal       2023-05-23  341  
22c370b2163e59 Miquel Raynal       2023-05-23  342  	if (attr->private)
22c370b2163e59 Miquel Raynal       2023-05-23  343  		dev = attr->private;
22c370b2163e59 Miquel Raynal       2023-05-23  344  	else
22c370b2163e59 Miquel Raynal       2023-05-23  345  		dev = kobj_to_dev(kobj);
22c370b2163e59 Miquel Raynal       2023-05-23  346  	nvmem = to_nvmem_device(dev);
22c370b2163e59 Miquel Raynal       2023-05-23  347  
22c370b2163e59 Miquel Raynal       2023-05-23  348  	mutex_lock(&nvmem_mutex);
22c370b2163e59 Miquel Raynal       2023-05-23  349  	list_for_each_entry(entry, &nvmem->cells, node) {
22c370b2163e59 Miquel Raynal       2023-05-23  350  		if (strncmp(entry->name, attr->attr.name, XATTR_NAME_MAX))
22c370b2163e59 Miquel Raynal       2023-05-23  351  			continue;
22c370b2163e59 Miquel Raynal       2023-05-23  352  
22c370b2163e59 Miquel Raynal       2023-05-23  353  		cell = nvmem_create_cell(entry, entry->name, 0);
22c370b2163e59 Miquel Raynal       2023-05-23  354  		if (IS_ERR(cell)) {
22c370b2163e59 Miquel Raynal       2023-05-23  355  			mutex_unlock(&nvmem_mutex);
22c370b2163e59 Miquel Raynal       2023-05-23  356  			return PTR_ERR(cell);
22c370b2163e59 Miquel Raynal       2023-05-23  357  		}
22c370b2163e59 Miquel Raynal       2023-05-23  358  
22c370b2163e59 Miquel Raynal       2023-05-23  359  		break;
22c370b2163e59 Miquel Raynal       2023-05-23  360  	}
22c370b2163e59 Miquel Raynal       2023-05-23  361  	mutex_unlock(&nvmem_mutex);
22c370b2163e59 Miquel Raynal       2023-05-23  362  
22c370b2163e59 Miquel Raynal       2023-05-23  363  	if (!cell)
22c370b2163e59 Miquel Raynal       2023-05-23  364  		return -EINVAL;
22c370b2163e59 Miquel Raynal       2023-05-23  365  
22c370b2163e59 Miquel Raynal       2023-05-23  366  	content = nvmem_cell_read(cell, &cell_sz);
22c370b2163e59 Miquel Raynal       2023-05-23  367  	if (IS_ERR(content)) {
22c370b2163e59 Miquel Raynal       2023-05-23  368  		count = PTR_ERR(content);
22c370b2163e59 Miquel Raynal       2023-05-23  369  		goto destroy_cell;

read_len not initialized on this goto path.

22c370b2163e59 Miquel Raynal       2023-05-23  370  	}
22c370b2163e59 Miquel Raynal       2023-05-23  371  
22c370b2163e59 Miquel Raynal       2023-05-23  372  	read_len = min_t(unsigned int, cell_sz - pos, count);
22c370b2163e59 Miquel Raynal       2023-05-23  373  	memcpy(buf, content + pos, read_len);
22c370b2163e59 Miquel Raynal       2023-05-23  374  	kfree(content);
22c370b2163e59 Miquel Raynal       2023-05-23  375  
22c370b2163e59 Miquel Raynal       2023-05-23  376  destroy_cell:
22c370b2163e59 Miquel Raynal       2023-05-23  377  	kfree_const(cell->id);
22c370b2163e59 Miquel Raynal       2023-05-23  378  	kfree(cell);
22c370b2163e59 Miquel Raynal       2023-05-23  379  
22c370b2163e59 Miquel Raynal       2023-05-23 @380  	return read_len;
                                                        ^^^^^^^^^^^^^^^

22c370b2163e59 Miquel Raynal       2023-05-23  381  }

Miquel Raynal May 29, 2023, 10:12 a.m. UTC | #5

Hi Greg,

miquel.raynal@bootlin.com wrote on Tue, 23 May 2023 19:14:02 +0200:

> Hi Greg,
> 
> gregkh@linuxfoundation.org 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). 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.

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). This implies two things: I need to provide an empty list of
files (otherwise the core warns) and if no list is provided then the
"cells" folder will always appear, no matter if there are cells or not
exposed by this nvmem device. So the folder can be empty.

If this is fine, I can:

* Expose the "cells" bin group, 
* Provide an empty .bin_attrs list (otherwise I get a complain from
  the sysfs core when the nvmem device does not expose any cell)
* Overwrite the dummy .bin_attrs member when the device expose cells,
  keep it empty otherwise.

Otherwise if we prefer avoiding empty folders, I can as well hide the
"cells" bin group by writing NULL to the relevant member of the
attribute_group list.

Let me know what is your preference.

Thanks,
Miquèl

Miquel Raynal May 29, 2023, 10:14 a.m. UTC | #6

Hi Dan,

dan.carpenter@linaro.org wrote on Mon, 29 May 2023 07:28:59 +0300:

> Hi Miquel,
> 
> kernel test robot noticed the following build warnings:
> 
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Miquel-Raynal/ABI-sysfs-nvmem-cells-Expose-cells-through-sysfs/20230523-203042
> base:   char-misc/char-misc-testing
> patch link:    https://lore.kernel.org/r/20230523100239.307574-3-miquel.raynal%40bootlin.com
> patch subject: [PATCH 2/2] nvmem: core: Expose cells through sysfs
> config: i386-randconfig-m021-20230525 (https://download.01.org/0day-ci/archive/20230528/202305280054.NloN5RLk-lkp@intel.com/config)
> compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> | Closes: https://lore.kernel.org/r/202305280054.NloN5RLk-lkp@intel.com/
> 
> smatch warnings:
> drivers/nvmem/core.c:380 nvmem_cell_attr_read() error: uninitialized symbol 'read_len'.
> 
> vim +/read_len +380 drivers/nvmem/core.c
> 
> 22c370b2163e59 Miquel Raynal       2023-05-23  328  static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
> 22c370b2163e59 Miquel Raynal       2023-05-23  329  					    const char *id, int index);
> 22c370b2163e59 Miquel Raynal       2023-05-23  330  
> 22c370b2163e59 Miquel Raynal       2023-05-23  331  static ssize_t nvmem_cell_attr_read(struct file *filp, struct kobject *kobj,
> 22c370b2163e59 Miquel Raynal       2023-05-23  332  				    struct bin_attribute *attr, char *buf,
> 22c370b2163e59 Miquel Raynal       2023-05-23  333  				    loff_t pos, size_t count)
> 22c370b2163e59 Miquel Raynal       2023-05-23  334  {
> 22c370b2163e59 Miquel Raynal       2023-05-23  335  	struct nvmem_cell_entry *entry;
> 22c370b2163e59 Miquel Raynal       2023-05-23  336  	struct nvmem_cell *cell = NULL;
> 22c370b2163e59 Miquel Raynal       2023-05-23  337  	struct nvmem_device *nvmem;
> 22c370b2163e59 Miquel Raynal       2023-05-23  338  	size_t cell_sz, read_len;
> 22c370b2163e59 Miquel Raynal       2023-05-23  339  	struct device *dev;
> 22c370b2163e59 Miquel Raynal       2023-05-23  340  	void *content;
> 22c370b2163e59 Miquel Raynal       2023-05-23  341  
> 22c370b2163e59 Miquel Raynal       2023-05-23  342  	if (attr->private)
> 22c370b2163e59 Miquel Raynal       2023-05-23  343  		dev = attr->private;
> 22c370b2163e59 Miquel Raynal       2023-05-23  344  	else
> 22c370b2163e59 Miquel Raynal       2023-05-23  345  		dev = kobj_to_dev(kobj);
> 22c370b2163e59 Miquel Raynal       2023-05-23  346  	nvmem = to_nvmem_device(dev);
> 22c370b2163e59 Miquel Raynal       2023-05-23  347  
> 22c370b2163e59 Miquel Raynal       2023-05-23  348  	mutex_lock(&nvmem_mutex);
> 22c370b2163e59 Miquel Raynal       2023-05-23  349  	list_for_each_entry(entry, &nvmem->cells, node) {
> 22c370b2163e59 Miquel Raynal       2023-05-23  350  		if (strncmp(entry->name, attr->attr.name, XATTR_NAME_MAX))
> 22c370b2163e59 Miquel Raynal       2023-05-23  351  			continue;
> 22c370b2163e59 Miquel Raynal       2023-05-23  352  
> 22c370b2163e59 Miquel Raynal       2023-05-23  353  		cell = nvmem_create_cell(entry, entry->name, 0);
> 22c370b2163e59 Miquel Raynal       2023-05-23  354  		if (IS_ERR(cell)) {
> 22c370b2163e59 Miquel Raynal       2023-05-23  355  			mutex_unlock(&nvmem_mutex);
> 22c370b2163e59 Miquel Raynal       2023-05-23  356  			return PTR_ERR(cell);
> 22c370b2163e59 Miquel Raynal       2023-05-23  357  		}
> 22c370b2163e59 Miquel Raynal       2023-05-23  358  
> 22c370b2163e59 Miquel Raynal       2023-05-23  359  		break;
> 22c370b2163e59 Miquel Raynal       2023-05-23  360  	}
> 22c370b2163e59 Miquel Raynal       2023-05-23  361  	mutex_unlock(&nvmem_mutex);
> 22c370b2163e59 Miquel Raynal       2023-05-23  362  
> 22c370b2163e59 Miquel Raynal       2023-05-23  363  	if (!cell)
> 22c370b2163e59 Miquel Raynal       2023-05-23  364  		return -EINVAL;
> 22c370b2163e59 Miquel Raynal       2023-05-23  365  
> 22c370b2163e59 Miquel Raynal       2023-05-23  366  	content = nvmem_cell_read(cell, &cell_sz);
> 22c370b2163e59 Miquel Raynal       2023-05-23  367  	if (IS_ERR(content)) {
> 22c370b2163e59 Miquel Raynal       2023-05-23  368  		count = PTR_ERR(content);
> 22c370b2163e59 Miquel Raynal       2023-05-23  369  		goto destroy_cell;
> 
> read_len not initialized on this goto path.

It should be:							read_len = PTR_ERR...

I will correct this in the next version, thanks for the report.

> 
> 22c370b2163e59 Miquel Raynal       2023-05-23  370  	}
> 22c370b2163e59 Miquel Raynal       2023-05-23  371  
> 22c370b2163e59 Miquel Raynal       2023-05-23  372  	read_len = min_t(unsigned int, cell_sz - pos, count);
> 22c370b2163e59 Miquel Raynal       2023-05-23  373  	memcpy(buf, content + pos, read_len);
> 22c370b2163e59 Miquel Raynal       2023-05-23  374  	kfree(content);
> 22c370b2163e59 Miquel Raynal       2023-05-23  375  
> 22c370b2163e59 Miquel Raynal       2023-05-23  376  destroy_cell:
> 22c370b2163e59 Miquel Raynal       2023-05-23  377  	kfree_const(cell->id);
> 22c370b2163e59 Miquel Raynal       2023-05-23  378  	kfree(cell);
> 22c370b2163e59 Miquel Raynal       2023-05-23  379  
> 22c370b2163e59 Miquel Raynal       2023-05-23 @380  	return read_len;
>                                                         ^^^^^^^^^^^^^^^
> 
> 22c370b2163e59 Miquel Raynal       2023-05-23  381  }
> 


Thanks,
Miquèl

Greg KH May 29, 2023, 1:05 p.m. UTC | #7

On Mon, May 29, 2023 at 12:12:26PM +0200, Miquel Raynal wrote:
> Hi Greg,
> 
> miquel.raynal@bootlin.com wrote on Tue, 23 May 2023 19:14:02 +0200:
> 
> > Hi Greg,
> > 
> > gregkh@linuxfoundation.org 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...

> 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?

If so, all should be fine, just ignore the empty directory, it's fine.
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.

thanks,

greg k-h

Miquel Raynal May 29, 2023, 1:35 p.m. UTC | #8

Hi Greg,

gregkh@linuxfoundation.org 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@bootlin.com wrote on Tue, 23 May 2023 19:14:02 +0200:
> >   
> > > Hi Greg,
> > > 
> > > gregkh@linuxfoundation.org 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.

I hope this is acceptable.

Thanks,
Miquèl

Greg KH May 29, 2023, 1:43 p.m. UTC | #9

On Mon, May 29, 2023 at 03:35:39PM +0200, Miquel Raynal wrote:
> Hi Greg,
> 
> gregkh@linuxfoundation.org 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@bootlin.com wrote on Tue, 23 May 2023 19:14:02 +0200:
> > >   
> > > > Hi Greg,
> > > > 
> > > > gregkh@linuxfoundation.org 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

Miquel Raynal May 29, 2023, 2 p.m. UTC | #10

Hi Greg,

gregkh@linuxfoundation.org wrote on Mon, 29 May 2023 14:43:51 +0100:

> On Mon, May 29, 2023 at 03:35:39PM +0200, Miquel Raynal wrote:
> > Hi Greg,
> > 
> > gregkh@linuxfoundation.org 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@bootlin.com wrote on Tue, 23 May 2023 19:14:02 +0200:
> > > >     
> > > > > Hi Greg,
> > > > > 
> > > > > gregkh@linuxfoundation.org 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.

To be exact, there is a hook to provide the size of a file at
runtime (and I use it), but here I am referring to the number of files
in the directory. In this case, the number of files depends on the
number of cells, which are discovered dynamically at run time. Hence,
the attribute_group for the cells needs a writable .bin_attrs member.

Thanks,
Miquèl

diff mbox series

Patch

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 342cd380b420..234b2f232854 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -325,6 +325,61 @@  static umode_t nvmem_bin_attr_is_visible(struct kobject *kobj,
 	return nvmem_bin_attr_get_umode(nvmem);
 }
 
+static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
+					    const char *id, 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;
+	struct nvmem_device *nvmem;
+	size_t cell_sz, read_len;
+	struct device *dev;
+	void *content;
+
+	if (attr->private)
+		dev = attr->private;
+	else
+		dev = kobj_to_dev(kobj);
+	nvmem = to_nvmem_device(dev);
+
+	mutex_lock(&nvmem_mutex);
+	list_for_each_entry(entry, &nvmem->cells, node) {
+		if (strncmp(entry->name, attr->attr.name, XATTR_NAME_MAX))
+			continue;
+
+		cell = nvmem_create_cell(entry, entry->name, 0);
+		if (IS_ERR(cell)) {
+			mutex_unlock(&nvmem_mutex);
+			return PTR_ERR(cell);
+		}
+
+		break;
+	}
+	mutex_unlock(&nvmem_mutex);
+
+	if (!cell)
+		return -EINVAL;
+
+	content = nvmem_cell_read(cell, &cell_sz);
+	if (IS_ERR(content)) {
+		count = 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	= {
@@ -346,8 +401,14 @@  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, /* Reserved for exposing cells, if any */
 	NULL,
 };
 
@@ -406,6 +467,66 @@  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);
+
+	/* Allocate an array of attributes */
+	list_for_each_entry(entry, &nvmem->cells, node)
+		ncells++;
+
+	if (!ncells)
+		goto unlock_mutex;
+
+	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 have the name of a cell */
+	list_for_each_entry(entry, &nvmem->cells, node) {
+		sysfs_bin_attr_init(&attrs[i]);
+		attrs[i].attr.name = kstrdup(entry->name, GFP_KERNEL);
+		attrs[i].attr.mode = 0444;
+		attrs[i].size = entry->bytes;
+		attrs[i].read = &nvmem_cell_attr_read;
+		if (!attrs[i].attr.name) {
+			ret = -ENOMEM;
+			goto unlock_mutex;
+		}
+
+		cells_attrs[i] = &attrs[i];
+		i++;
+	}
+
+	nvmem_cells_group.bin_attrs = cells_attrs;
+
+	/* Fill the attribute group structure with the cells member */
+	for (i = 0;; i++)
+		if (!nvmem_dev_groups[i])
+			break;
+	nvmem_dev_groups[i] = &nvmem_cells_group;
+
+unlock_mutex:
+	mutex_unlock(&nvmem_mutex);
+
+	return ret;
+}
+
 #else /* CONFIG_NVMEM_SYSFS */
 
 static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
@@ -976,16 +1097,22 @@  struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	if (rval)
 		goto err_remove_cells;
 
+	rval = nvmem_add_cells_from_layout(nvmem);
+	if (rval)
+		goto err_remove_cells;
+
+#ifdef CONFIG_NVMEM_SYSFS
+	rval = nvmem_populate_sysfs_cells(nvmem);
+	if (rval)
+		goto err_remove_cells;
+#endif
+
 	dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name);
 
 	rval = device_add(&nvmem->dev);
 	if (rval)
 		goto err_remove_cells;
 
-	rval = nvmem_add_cells_from_layout(nvmem);
-	if (rval)
-		goto err_remove_cells;
-
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
 
 	return nvmem;