Date:   Wed, 22 Nov 2023 23:45:53 +0100
From:   Marco Felsch <m.felsch@...gutronix.de>
To:     Miquel Raynal <miquel.raynal@...tlin.com>
Cc:     devicetree@...r.kernel.org, Luka Perkov <luka.perkov@...tura.hr>,
        kernel@...gutronix.de,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-kernel@...r.kernel.org, Daniel Golle <daniel@...rotopia.org>,
        Michael Walle <michael@...le.cc>,
        Rob Herring <robh+dt@...nel.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Chen-Yu Tsai <wenst@...omium.org>,
        Rafał Miłecki <rafal@...ecki.pl>,
        Robert Marko <robert.marko@...tura.hr>,
        Frank Rowand <frowand.list@...il.com>
Subject: Re: [PATCH v13 4/6] nvmem: core: Rework layouts to become regular
 devices

Hi Miquel,

sorry for answering to my own mail, I forgot something I noticed later.

On 23-11-22, Marco Felsch wrote:
> Hi Miquel,
> 
> thanks a lot for your effort on this. Please see my comments inline.
> 
> On 23-10-11, Miquel Raynal wrote:
> > Current layout support was initially written without modules support in
> > mind. When the requirement for module support rose, the existing base
> > was improved to adopt modularization support, but kind of a design flaw
> > was introduced. With the existing implementation, when a storage device
> > registers into NVMEM, the core tries to hook a layout (if any) and
> > populates its cells immediately. This means, if the hardware description
> > expects a layout to be hooked up, but no driver was provided for that,
> > the storage medium will fail to probe and try later from
> > scratch. Even if we consider that the hardware description shall be
> > correct, we could still probe the storage device (especially if it
> > contains the rootfs).
> > 
> > One way to overcome this situation is to consider the layouts as
> > devices, and leverage the existing notifier mechanism. When a new NVMEM
> > device is registered, we can:
> > - populate its nvmem-layout child, if any
> > - try to modprobe the relevant driver, if relevant

I'm not sure why we call of_request_module() the driver framework should
handle that right?

> > - try to hook the NVMEM device with a layout in the notifier

The last part is no longer true since you don't use the notifier
anymore.

> > And when a new layout is registered:
> > - try to hook all the existing NVMEM devices which are not yet hooked to
> >   a layout with the new layout
> > This way, there is no strong order to enforce, any NVMEM device creation
> > or NVMEM layout driver insertion will be observed as a new event which
> > may lead to the creation of additional cells, without disturbing the
> > probes with costly (and sometimes endless) deferrals.
> > 
> > In order to achieve that goal we need:
> > * To keep track of all nvmem devices
> > * To create a new bus for the nvmem-layouts with minimal logic to match
> >   nvmem-layout devices with nvmem-layout drivers.
> > All this infrastructure code is created in the layouts.c file.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@...tlin.com>
> > Tested-by: Rafał Miłecki <rafal@...ecki.pl>

...

> > @@ -944,19 +872,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  			goto err_put_device;
> >  	}
> >  
> > -	/*
> > -	 * If the driver supplied a layout by config->layout, the module
> > -	 * pointer will be NULL and nvmem_layout_put() will be a noop.
> > -	 */
> > -	nvmem->layout = config->layout ?: nvmem_layout_get(nvmem);
> > -	if (IS_ERR(nvmem->layout)) {
> > -		rval = PTR_ERR(nvmem->layout);
> > -		nvmem->layout = NULL;
> > -
> > -		if (rval == -EPROBE_DEFER)
> > -			goto err_teardown_compat;
> > -	}

Since this logic will be gone and the layout became a device the fixup
hook for the layout is more than confusing. E.g. the imx-ocotp driver
uses the layout to register a fixup for a cell which is fine but the
hook should be moved from the layout[-dev] to the config. Please see
below.

> > -
> >  	if (config->cells) {
> >  		rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
> >  		if (rval)
> > @@ -975,7 +890,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  	if (rval)
> >  		goto err_remove_cells;
> >  
> > -	rval = nvmem_add_cells_from_layout(nvmem);
> > +	rval = nvmem_populate_layout(nvmem);
> >  	if (rval)
> >  		goto err_remove_cells;

Also why do we populate the nvmem-layout device infront of the nvmem
device?

> >  
> > @@ -983,16 +898,17 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >  
> >  	rval = device_add(&nvmem->dev);
> >  	if (rval)
> > -		goto err_remove_cells;
> > +		goto err_destroy_layout;
> > +
> >  
> >  	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
> >  
> >  	return nvmem;
> >  
> > +err_destroy_layout:
> > +	nvmem_destroy_layout(nvmem);
> >  err_remove_cells:
> >  	nvmem_device_remove_all_cells(nvmem);
> > -	nvmem_layout_put(nvmem->layout);
> > -err_teardown_compat:
> >  	if (config->compat)
> >  		nvmem_sysfs_remove_compat(nvmem, config);
> >  err_put_device:
> > @@ -1014,7 +930,7 @@ static void nvmem_device_release(struct kref *kref)
> >  		device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
> >  
> >  	nvmem_device_remove_all_cells(nvmem);
> > -	nvmem_layout_put(nvmem->layout);
> > +	nvmem_destroy_layout(nvmem);
> >  	device_unregister(&nvmem->dev);
> >  }
> >  

...

> > diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> > index 2905f9e6fc2a..a0ea8326605a 100644
> > --- a/include/linux/nvmem-provider.h
> > +++ b/include/linux/nvmem-provider.h
> > @@ -9,6 +9,7 @@
> >  #ifndef _LINUX_NVMEM_PROVIDER_H
> >  #define _LINUX_NVMEM_PROVIDER_H
> >  
> > +#include <linux/device.h>
> >  #include <linux/device/driver.h>
> >  #include <linux/err.h>
> >  #include <linux/errno.h>
> > @@ -154,15 +155,13 @@ struct nvmem_cell_table {
> >  /**
> >   * struct nvmem_layout - NVMEM layout definitions
> >   *
> > - * @name:		Layout name.
> > - * @of_match_table:	Open firmware match table.
> > + * @dev:		Device-model layout device.
> > + * @nvmem:		The underlying NVMEM device
> >   * @add_cells:		Will be called if a nvmem device is found which
> >   *			has this layout. The function will add layout
> >   *			specific cells with nvmem_add_one_cell().
> >   * @fixup_cell_info:	Will be called before a cell is added. Can be
> >   *			used to modify the nvmem_cell_info.
> > - * @owner:		Pointer to struct module.
> > - * @node:		List node.
> >   *
> >   * A nvmem device can hold a well defined structure which can just be
> >   * evaluated during runtime. For example a TLV list, or a list of "name=val"
> > @@ -170,17 +169,19 @@ struct nvmem_cell_table {
> >   * cells.
> >   */
> >  struct nvmem_layout {
> 
> Since this became a device now should we refelct this within the struct
> name, e.g. nvmem_layout_dev, nvmem_ldev, nvm_ldev?
> 
> Regards,
>   Marco
> 
> > -	const char *name;
> > -	const struct of_device_id *of_match_table;
> > +	struct device dev;
> > +	struct nvmem_device *nvmem;
> >  	int (*add_cells)(struct device *dev, struct nvmem_device *nvmem,
> >  			 struct nvmem_layout *layout);
> >  	void (*fixup_cell_info)(struct nvmem_device *nvmem,
> >  				struct nvmem_layout *layout,
> >  				struct nvmem_cell_info *cell);

I speak about this hook. This should be moved into the config, maybe
also renamed to fixup_dt_cell_info() or so to not confuse the users. If
we move that hook and remove the add_cells hook there are only two
members left for the cross-link.

Regards,
  Marco

Powered by blists - more mailing lists