| Message ID | 96510d925cb0ca1a3a132f8f8affd4bbdafd8fc9.1689802933.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nvmem: add block device NVMEM provider | expand |
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
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 {
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(+)