[RFC,3/6] block: add new genhd flag GENHD_FL_NO_NVMEM

  Add new flag to destinguish block devices which should not act as an
NVMEM provider, such as for example an emulated block device on top of
an MTD partition which already acts as an NVMEM provider itself.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 include/linux/blkdev.h | 3 +++
 1 file changed, 3 insertions(+)
  

On 7/20/23 00:03, Daniel Golle wrote:
> Add new flag to destinguish block devices which should not act as an
> NVMEM provider, such as for example an emulated block device on top of
> an MTD partition which already acts as an NVMEM provider itself.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>   include/linux/blkdev.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 2f5371b8482c0..e853d1815be15 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -80,11 +80,14 @@ struct partition_meta_info {
>    * ``GENHD_FL_NO_PART``: partition support is disabled.  The kernel will not
>    * scan for partitions from add_disk, and users can't add partitions manually.
>    *
> + * ``GENHD_FL_NO_NVMEM``: NVMEM emulation is disabled.  The kernel will not
> + * emulate an NVMEM device on top of this disk.
>    */
>   enum {
>   	GENHD_FL_REMOVABLE			= 1 << 0,
>   	GENHD_FL_HIDDEN				= 1 << 1,
>   	GENHD_FL_NO_PART			= 1 << 2,
> +	GENHD_FL_NO_NVMEM			= 1 << 3,
>   };
>   
>   enum {
Please reverse this flag. Most of the devices will not have an NVMEM 
partition, and we shouldn't require each and every driver to tag their 
devices.
So please use GENHD_FL_NVMEM and only set this flag on devices which 
really have an NVMEM partition.

Cheers,

Hannes
  

On Thu, Jul 20, 2023 at 10:24:18AM +0200, Hannes Reinecke wrote:
> On 7/20/23 00:03, Daniel Golle wrote:
> > Add new flag to destinguish block devices which should not act as an
> > NVMEM provider, such as for example an emulated block device on top of
> > an MTD partition which already acts as an NVMEM provider itself.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> >   include/linux/blkdev.h | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 2f5371b8482c0..e853d1815be15 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -80,11 +80,14 @@ struct partition_meta_info {
> >    * ``GENHD_FL_NO_PART``: partition support is disabled.  The kernel will not
> >    * scan for partitions from add_disk, and users can't add partitions manually.
> >    *
> > + * ``GENHD_FL_NO_NVMEM``: NVMEM emulation is disabled.  The kernel will not
> > + * emulate an NVMEM device on top of this disk.
> >    */
> >   enum {
> >   	GENHD_FL_REMOVABLE			= 1 << 0,
> >   	GENHD_FL_HIDDEN				= 1 << 1,
> >   	GENHD_FL_NO_PART			= 1 << 2,
> > +	GENHD_FL_NO_NVMEM			= 1 << 3,
> >   };
> >   enum {
> Please reverse this flag. Most of the devices will not have an NVMEM
> partition, and we shouldn't require each and every driver to tag their
> devices.
> So please use GENHD_FL_NVMEM and only set this flag on devices which really
> have an NVMEM partition.

The idea here was to exclude all those devices which already implement
an NVMEM provider on a lower layer themselves, such as MTD.
In this cases it would be ambigous if the OF node represents the
NVMEM device registered by the MTD framework or if blk-nvmem should be
used.

In all other cases device tree can unambigously indicate whether a
block device should serve as NVMEM provider (and right, most of them
never will).

However, reversing the logic seems fine just as well.
  

On 7/20/23 15:47, Daniel Golle wrote:
> On Thu, Jul 20, 2023 at 10:24:18AM +0200, Hannes Reinecke wrote:
>> On 7/20/23 00:03, Daniel Golle wrote:
>>> Add new flag to destinguish block devices which should not act as an
>>> NVMEM provider, such as for example an emulated block device on top of
>>> an MTD partition which already acts as an NVMEM provider itself.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>    include/linux/blkdev.h | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>> index 2f5371b8482c0..e853d1815be15 100644
>>> --- a/include/linux/blkdev.h
>>> +++ b/include/linux/blkdev.h
>>> @@ -80,11 +80,14 @@ struct partition_meta_info {
>>>     * ``GENHD_FL_NO_PART``: partition support is disabled.  The kernel will not
>>>     * scan for partitions from add_disk, and users can't add partitions manually.
>>>     *
>>> + * ``GENHD_FL_NO_NVMEM``: NVMEM emulation is disabled.  The kernel will not
>>> + * emulate an NVMEM device on top of this disk.
>>>     */
>>>    enum {
>>>    	GENHD_FL_REMOVABLE			= 1 << 0,
>>>    	GENHD_FL_HIDDEN				= 1 << 1,
>>>    	GENHD_FL_NO_PART			= 1 << 2,
>>> +	GENHD_FL_NO_NVMEM			= 1 << 3,
>>>    };
>>>    enum {
>> Please reverse this flag. Most of the devices will not have an NVMEM
>> partition, and we shouldn't require each and every driver to tag their
>> devices.
>> So please use GENHD_FL_NVMEM and only set this flag on devices which really
>> have an NVMEM partition.
> 
> The idea here was to exclude all those devices which already implement
> an NVMEM provider on a lower layer themselves, such as MTD.
> In this cases it would be ambigous if the OF node represents the
> NVMEM device registered by the MTD framework or if blk-nvmem should be
> used.
> 
Hmm; not sure if I follow.
In the end, it doesn't really matter whether you check for
GENHD_FL_NO_NVMEM or !GENHD_FL_NVMEM.
With the difference being that in the former case you have to
tag 99% of all existing block devices, and in the latter you
have to tag 1%.

> In all other cases device tree can unambigously indicate whether a
> block device should serve as NVMEM provider (and right, most of them
> never will).
> 
> However, reversing the logic seems fine just as well.

Thanks. Please do.

Cheers,

Hannes
  

On Thu, Jul 20, 2023 at 04:03:22PM +0200, Hannes Reinecke wrote:
> On 7/20/23 15:47, Daniel Golle wrote:
> > On Thu, Jul 20, 2023 at 10:24:18AM +0200, Hannes Reinecke wrote:
> > > On 7/20/23 00:03, Daniel Golle wrote:
> > > > Add new flag to destinguish block devices which should not act as an
> > > > NVMEM provider, such as for example an emulated block device on top of
> > > > an MTD partition which already acts as an NVMEM provider itself.
> > > > 
> > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > > > ---
> > > >    include/linux/blkdev.h | 3 +++
> > > >    1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > > index 2f5371b8482c0..e853d1815be15 100644
> > > > --- a/include/linux/blkdev.h
> > > > +++ b/include/linux/blkdev.h
> > > > @@ -80,11 +80,14 @@ struct partition_meta_info {
> > > >     * ``GENHD_FL_NO_PART``: partition support is disabled.  The kernel will not
> > > >     * scan for partitions from add_disk, and users can't add partitions manually.
> > > >     *
> > > > + * ``GENHD_FL_NO_NVMEM``: NVMEM emulation is disabled.  The kernel will not
> > > > + * emulate an NVMEM device on top of this disk.
> > > >     */
> > > >    enum {
> > > >    	GENHD_FL_REMOVABLE			= 1 << 0,
> > > >    	GENHD_FL_HIDDEN				= 1 << 1,
> > > >    	GENHD_FL_NO_PART			= 1 << 2,
> > > > +	GENHD_FL_NO_NVMEM			= 1 << 3,
> > > >    };
> > > >    enum {
> > > Please reverse this flag. Most of the devices will not have an NVMEM
> > > partition, and we shouldn't require each and every driver to tag their
> > > devices.
> > > So please use GENHD_FL_NVMEM and only set this flag on devices which really
> > > have an NVMEM partition.
> > 
> > The idea here was to exclude all those devices which already implement
> > an NVMEM provider on a lower layer themselves, such as MTD.
> > In this cases it would be ambigous if the OF node represents the
> > NVMEM device registered by the MTD framework or if blk-nvmem should be
> > used.
> > 
> Hmm; not sure if I follow.
> In the end, it doesn't really matter whether you check for
> GENHD_FL_NO_NVMEM or !GENHD_FL_NVMEM.
> With the difference being that in the former case you have to
> tag 99% of all existing block devices, and in the latter you
> have to tag 1%.

That's not exactly true. In the current case I only have to flag MTD
(and UBI in future, I'm working on a UBI NVMEM provider as well) with
GENHD_FL_NO_NVMEM, so a 'compatible = "nvmem-cells"' in the
corresponding device tree node should not result in blk-nvmem creating
an NVMEM device based on the (mtd/ubi)block device, simply because the
MTD framework (and UBI in future) will already have created their own
NVMEM device attached to the very same device tree node.

In all other cases of block devices, the compatible string can be used
to unambigously decide whether an NVMEM device should be created or
not. blk-nvmem is opt-in, so unless the device is flagged by
'compatible = "nvmem-cells"' it will not do anything.

For all devices which anyway do not have any device tree representation
it won't do anything (think: loop, nbd, ...), we would not need to opt
them out using GENHD_FL_NO_NVMEM. Also all other drivers which do not
already bring their own NVMEM implementation won't need GENHD_FL_NO_NVMEM,
the absence of 'compatible = "nvmem-cells"' is enough to indicate that
they should not be considered as NVMEM providers.

The way you are suggesting will require that, in addition to selecting
the targetted block device in device tree, the block driver will also
have to set GENHD_FL_NVMEM. Hence we will need changes in MMC, NVMe
and potentially also SATA disk drivers setting GENHD_FL_NVMEM when
registering the disk.

> 
> > In all other cases device tree can unambigously indicate whether a
> > block device should serve as NVMEM provider (and right, most of them
> > never will).
> > 
> > However, reversing the logic seems fine just as well.
> 
> Thanks. Please do.

Either way is fine with me, just 99% vs. 1% is not the right argument
in this case.
  

On 7/20/23 16:28, Daniel Golle wrote:
> On Thu, Jul 20, 2023 at 04:03:22PM +0200, Hannes Reinecke wrote:
>> On 7/20/23 15:47, Daniel Golle wrote:
>>> On Thu, Jul 20, 2023 at 10:24:18AM +0200, Hannes Reinecke wrote:
>>>> On 7/20/23 00:03, Daniel Golle wrote:
>>>>> Add new flag to destinguish block devices which should not act as an
>>>>> NVMEM provider, such as for example an emulated block device on top of
>>>>> an MTD partition which already acts as an NVMEM provider itself.
>>>>>
>>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>>>> ---
>>>>>     include/linux/blkdev.h | 3 +++
>>>>>     1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>>>>> index 2f5371b8482c0..e853d1815be15 100644
>>>>> --- a/include/linux/blkdev.h
>>>>> +++ b/include/linux/blkdev.h
>>>>> @@ -80,11 +80,14 @@ struct partition_meta_info {
>>>>>      * ``GENHD_FL_NO_PART``: partition support is disabled.  The kernel will not
>>>>>      * scan for partitions from add_disk, and users can't add partitions manually.
>>>>>      *
>>>>> + * ``GENHD_FL_NO_NVMEM``: NVMEM emulation is disabled.  The kernel will not
>>>>> + * emulate an NVMEM device on top of this disk.
>>>>>      */
>>>>>     enum {
>>>>>     	GENHD_FL_REMOVABLE			= 1 << 0,
>>>>>     	GENHD_FL_HIDDEN				= 1 << 1,
>>>>>     	GENHD_FL_NO_PART			= 1 << 2,
>>>>> +	GENHD_FL_NO_NVMEM			= 1 << 3,
>>>>>     };
>>>>>     enum {
>>>> Please reverse this flag. Most of the devices will not have an NVMEM
>>>> partition, and we shouldn't require each and every driver to tag their
>>>> devices.
>>>> So please use GENHD_FL_NVMEM and only set this flag on devices which really
>>>> have an NVMEM partition.
>>>
>>> The idea here was to exclude all those devices which already implement
>>> an NVMEM provider on a lower layer themselves, such as MTD.
>>> In this cases it would be ambigous if the OF node represents the
>>> NVMEM device registered by the MTD framework or if blk-nvmem should be
>>> used.
>>>
>> Hmm; not sure if I follow.
>> In the end, it doesn't really matter whether you check for
>> GENHD_FL_NO_NVMEM or !GENHD_FL_NVMEM.
>> With the difference being that in the former case you have to
>> tag 99% of all existing block devices, and in the latter you
>> have to tag 1%.
> 
> That's not exactly true. In the current case I only have to flag MTD
> (and UBI in future, I'm working on a UBI NVMEM provider as well) with
> GENHD_FL_NO_NVMEM, so a 'compatible = "nvmem-cells"' in the
> corresponding device tree node should not result in blk-nvmem creating
> an NVMEM device based on the (mtd/ubi)block device, simply because the
> MTD framework (and UBI in future) will already have created their own
> NVMEM device attached to the very same device tree node.
> 
> In all other cases of block devices, the compatible string can be used
> to unambigously decide whether an NVMEM device should be created or
> not. blk-nvmem is opt-in, so unless the device is flagged by
> 'compatible = "nvmem-cells"' it will not do anything.
> 
> For all devices which anyway do not have any device tree representation
> it won't do anything (think: loop, nbd, ...), we would not need to opt
> them out using GENHD_FL_NO_NVMEM. Also all other drivers which do not
> already bring their own NVMEM implementation won't need GENHD_FL_NO_NVMEM,
> the absence of 'compatible = "nvmem-cells"' is enough to indicate that
> they should not be considered as NVMEM providers.
> 
> The way you are suggesting will require that, in addition to selecting
> the targetted block device in device tree, the block driver will also
> have to set GENHD_FL_NVMEM. Hence we will need changes in MMC, NVMe
> and potentially also SATA disk drivers setting GENHD_FL_NVMEM when
> registering the disk.
> 
That is absolutely correct, and was my intention all along.
Drivers which can (and do) supply an NVMEM partition should be required 
to set this flag, yes.

Cheers,

Hannes