diff mbox series

[v4,4/5] mtd_blkdevs: add option to enable scanning for partitions

Message ID Y2rgbfpYfpbLKHaf@makrotopia.org
State New
Headers
Series partition parser for U-Boot's uImage.FIT |

Commit Message

Daniel Golle Nov. 8, 2022, 11:04 p.m. UTC 
  Add Kconfig boolean CONFIG_MTD_BLOCK_PARTITIONS and enable block
partition parsers on non-NAND mtdblock devices in case it is selected.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/Kconfig       | 11 +++++++++++
 drivers/mtd/mtd_blkdevs.c |  4 +++-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Miquel Raynal Nov. 9, 2022, 12:45 p.m. UTC | #1 
Hi Daniel,

daniel@makrotopia.org wrote on Tue, 8 Nov 2022 23:04:13 +0000:

> Add Kconfig boolean CONFIG_MTD_BLOCK_PARTITIONS and enable block
> partition parsers on non-NAND mtdblock devices in case it is selected.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/Kconfig       | 11 +++++++++++
>  drivers/mtd/mtd_blkdevs.c |  4 +++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index 796a2eccbef0..12874dec1569 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -69,6 +69,17 @@ config MTD_BLOCK_RO
>  	  You do not need this option for use with the DiskOnChip devices. For
>  	  those, enable NFTL support (CONFIG_NFTL) instead.
>  
> +config MTD_BLOCK_PARTITIONS
> +	bool "Scan for partitions on MTD block devices"
> +	depends on MTD_BLOCK || MTD_BLOCK_RO
> +	default y if FIT_PARTITION
> +	help
> +	  Scan MTD block devices for partitions (ie. MBR, GPT, uImage.FIT, ...).
> +	  (NAND devices are omitted, ubiblock should be used instead when)
> +
> +	  Unless your MTD partitions contain sub-partitions mapped using a
> +	  partition table, say no.

Actually I know I've acked this patch in the past but I am now
wondering again if this is the right way so I would like to discuss
this again. I don't feel making partitions on top of blk devices like
that is the right solution. Why not using the advanced partition
parsers that we now have in MTD to create these subpartitions and then
have blocks emulated on top of them?

> +
>  comment "Note that in some cases UBI block is preferred. See MTD_UBI_BLOCK."
>  	depends on MTD_BLOCK || MTD_BLOCK_RO
>  
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 60b222799871..e6f2e0888246 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -359,7 +359,9 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
>  	} else {
>  		snprintf(gd->disk_name, sizeof(gd->disk_name),
>  			 "%s%d", tr->name, new->devnum);
> -		gd->flags |= GENHD_FL_NO_PART;
> +
> +		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
> +			gd->flags |= GENHD_FL_NO_PART;
>  	}
>  
>  	set_capacity(gd, ((u64)new->size * tr->blksize) >> 9);


Thanks,
Miquèl
Daniel Golle Nov. 9, 2022, 1:10 p.m. UTC | #2 
Hi Miquel,

On Wed, Nov 09, 2022 at 01:45:26PM +0100, Miquel Raynal wrote:
> Hi Daniel,
> 
> daniel@makrotopia.org wrote on Tue, 8 Nov 2022 23:04:13 +0000:
> 
> > Add Kconfig boolean CONFIG_MTD_BLOCK_PARTITIONS and enable block
> > partition parsers on non-NAND mtdblock devices in case it is selected.
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/Kconfig       | 11 +++++++++++
> >  drivers/mtd/mtd_blkdevs.c |  4 +++-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> > index 796a2eccbef0..12874dec1569 100644
> > --- a/drivers/mtd/Kconfig
> > +++ b/drivers/mtd/Kconfig
> > @@ -69,6 +69,17 @@ config MTD_BLOCK_RO
> >  	  You do not need this option for use with the DiskOnChip devices. For
> >  	  those, enable NFTL support (CONFIG_NFTL) instead.
> >  
> > +config MTD_BLOCK_PARTITIONS
> > +	bool "Scan for partitions on MTD block devices"
> > +	depends on MTD_BLOCK || MTD_BLOCK_RO
> > +	default y if FIT_PARTITION
> > +	help
> > +	  Scan MTD block devices for partitions (ie. MBR, GPT, uImage.FIT, ...).
> > +	  (NAND devices are omitted, ubiblock should be used instead when)
> > +
> > +	  Unless your MTD partitions contain sub-partitions mapped using a
> > +	  partition table, say no.
> 
> Actually I know I've acked this patch in the past but I am now
> wondering again if this is the right way so I would like to discuss
> this again. I don't feel making partitions on top of blk devices like
> that is the right solution. Why not using the advanced partition
> parsers that we now have in MTD to create these subpartitions and then
> have blocks emulated on top of them?

Simply because then we would need the same parser implemented one time
as block/partitions/fit.c (for devices booting from SATA or eMMC, for
example) and then another time as drivers/mtd/parsers/parse_fit.c (for
devices using raw NOR flash) and for UBI there isn't even any
infrastructure allowing the dynamic splitting of UBI volumes (hence we
would need to invent a new splitting/partitioning mechanism on top of
UBI just for that).

Using the block partition parser as common ground for all three storage
types typically found in embedded devices seemed like the best idea, as
in that way I only had to implement the uImage.FIT parser once.


> 
> > +
> >  comment "Note that in some cases UBI block is preferred. See MTD_UBI_BLOCK."
> >  	depends on MTD_BLOCK || MTD_BLOCK_RO
> >  
> > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> > index 60b222799871..e6f2e0888246 100644
> > --- a/drivers/mtd/mtd_blkdevs.c
> > +++ b/drivers/mtd/mtd_blkdevs.c
> > @@ -359,7 +359,9 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
> >  	} else {
> >  		snprintf(gd->disk_name, sizeof(gd->disk_name),
> >  			 "%s%d", tr->name, new->devnum);
> > -		gd->flags |= GENHD_FL_NO_PART;
> > +
> > +		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
> > +			gd->flags |= GENHD_FL_NO_PART;
> >  	}
> >  
> >  	set_capacity(gd, ((u64)new->size * tr->blksize) >> 9);
> 
> 
> Thanks,
> Miquèl
Richard Weinberger Nov. 10, 2022, 7:44 p.m. UTC | #3 
----- Ursprüngliche Mail -----
> Von: "Daniel Golle" <daniel@makrotopia.org>
> +
> +		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
> +			gd->flags |= GENHD_FL_NO_PART;

I know that NAND should not get used with mtdblock because lack of wearleveling and
in general too many writes. But what exactly is the rationale to deny part scanning for NAND?

Thanks,
//richard
Daniel Golle Nov. 10, 2022, 8:04 p.m. UTC | #4 
On Thu, Nov 10, 2022 at 08:44:53PM +0100, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Daniel Golle" <daniel@makrotopia.org>
> > +
> > +		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
> > +			gd->flags |= GENHD_FL_NO_PART;
> 
> I know that NAND should not get used with mtdblock because lack of wearleveling and
> in general too many writes. But what exactly is the rationale to deny part scanning for NAND?

As UBI should be used on NAND, partition scanning should be enabled for
ubiblock devices to have uImage.FIT filesystem subimages mapped by the
partition parser.

If not skipping partition scanning on NAND-backed mtdblock devices the
scanning itself will already trigger multiple warnings which now happen
every time when a NAND-backed mtdblock device is being opened since
commit 96a3295c ("mtdblock: warn if opened on NAND").
Richard Weinberger Nov. 10, 2022, 8:12 p.m. UTC | #5 
----- Ursprüngliche Mail -----
> On Thu, Nov 10, 2022 at 08:44:53PM +0100, Richard Weinberger wrote:
>> ----- Ursprüngliche Mail -----
>> > Von: "Daniel Golle" <daniel@makrotopia.org>
>> > +
>> > +		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
>> > +			gd->flags |= GENHD_FL_NO_PART;
>> 
>> I know that NAND should not get used with mtdblock because lack of wearleveling
>> and
>> in general too many writes. But what exactly is the rationale to deny part
>> scanning for NAND?
> 
> As UBI should be used on NAND, partition scanning should be enabled for
> ubiblock devices to have uImage.FIT filesystem subimages mapped by the
> partition parser.
> 
> If not skipping partition scanning on NAND-backed mtdblock devices the
> scanning itself will already trigger multiple warnings which now happen
> every time when a NAND-backed mtdblock device is being opened since
> commit 96a3295c ("mtdblock: warn if opened on NAND").

I see, you want to promote UBI. Makes sense.
In case you do a v5 series, please add a comment to the code.

Thanks,
//richard
Daniel Golle Nov. 11, 2022, 12:39 a.m. UTC | #6 
On Thu, Nov 10, 2022 at 09:12:35PM +0100, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > On Thu, Nov 10, 2022 at 08:44:53PM +0100, Richard Weinberger wrote:
> >> ----- Ursprüngliche Mail -----
> >> > Von: "Daniel Golle" <daniel@makrotopia.org>
> >> > +
> >> > +		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
> >> > +			gd->flags |= GENHD_FL_NO_PART;
> >> 
> >> I know that NAND should not get used with mtdblock because lack of wearleveling
> >> and
> >> in general too many writes. But what exactly is the rationale to deny part
> >> scanning for NAND?
> > 
> > As UBI should be used on NAND, partition scanning should be enabled for
> > ubiblock devices to have uImage.FIT filesystem subimages mapped by the
> > partition parser.
> > 
> > If not skipping partition scanning on NAND-backed mtdblock devices the
> > scanning itself will already trigger multiple warnings which now happen
> > every time when a NAND-backed mtdblock device is being opened since
> > commit 96a3295c ("mtdblock: warn if opened on NAND").
> 
> I see, you want to promote UBI. Makes sense.
> In case you do a v5 series, please add a comment to the code.

Will do, I'm planning to send v5 early next week.

If we are going to have only CONFIG_MTD_BLOCK_PARTITIONS and no other
config symbol for ubiblock devices I'd also merge the two patches for
mtdblock and ubiblock partition scanning into a single one.

Thank you for reviewing!


Daniel
diff mbox series

Patch

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 796a2eccbef0..12874dec1569 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -69,6 +69,17 @@  config MTD_BLOCK_RO
 	  You do not need this option for use with the DiskOnChip devices. For
 	  those, enable NFTL support (CONFIG_NFTL) instead.
 
+config MTD_BLOCK_PARTITIONS
+	bool "Scan for partitions on MTD block devices"
+	depends on MTD_BLOCK || MTD_BLOCK_RO
+	default y if FIT_PARTITION
+	help
+	  Scan MTD block devices for partitions (ie. MBR, GPT, uImage.FIT, ...).
+	  (NAND devices are omitted, ubiblock should be used instead when)
+
+	  Unless your MTD partitions contain sub-partitions mapped using a
+	  partition table, say no.
+
 comment "Note that in some cases UBI block is preferred. See MTD_UBI_BLOCK."
 	depends on MTD_BLOCK || MTD_BLOCK_RO
 
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 60b222799871..e6f2e0888246 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -359,7 +359,9 @@  int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new)
 	} else {
 		snprintf(gd->disk_name, sizeof(gd->disk_name),
 			 "%s%d", tr->name, new->devnum);
-		gd->flags |= GENHD_FL_NO_PART;
+
+		if (!IS_ENABLED(CONFIG_MTD_BLOCK_PARTITIONS) || mtd_type_is_nand(new->mtd))
+			gd->flags |= GENHD_FL_NO_PART;
 	}
 
 	set_capacity(gd, ((u64)new->size * tr->blksize) >> 9);