Re: [PATCH 12/18] common: partitions: record whether disk is GPT or MBR partitioned

[Marco Felsch <m.felsch@pengutronix.de>]

On 23-06-01, Ahmad Fatoum wrote:
> On 31.05.23 19:33, Marco Felsch wrote:
> > On 23-05-31, Ahmad Fatoum wrote:
> >> Currently, the only way to differentiate between a GPT disk and a MBR
> >> one is to check whether the cdev's device has a guid (=> GPT) or a
> >> nt_signature (=> MBR) device parameter. We already have a flag parameter
> >> though, so let's record this info there for easy retrieval.
> >>
> >> We intentionally don't use the struct cdev::filetype member, because
> >> we don't want to change behavior of file_detect_type().
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  common/partitions/dos.c |  2 ++
> >>  common/partitions/efi.c |  2 ++
> >>  fs/fs.c                 |  4 ++++
> >>  include/driver.h        | 20 +++++++++++++++++---
> >>  4 files changed, 25 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/common/partitions/dos.c b/common/partitions/dos.c
> >> index ad60c0b27b46..7472824b00b9 100644
> >> --- a/common/partitions/dos.c
> >> +++ b/common/partitions/dos.c
> >> @@ -185,6 +185,8 @@ static void dos_partition(void *buf, struct block_device *blk,
> >>  	if (signature)
> >>  		sprintf(blk->cdev.diskuuid, "%08x", signature);
> >>  
> >> +	blk->cdev.flags |= DEVFS_IS_MBR_PARTITIONED;
> >> +
> >>  	table = (struct partition_entry *)&buffer[446];
> >>  
> >>  	for (i = 0; i < 4; i++) {
> >> diff --git a/common/partitions/efi.c b/common/partitions/efi.c
> >> index 780a8695e8a8..df63b82afe24 100644
> >> --- a/common/partitions/efi.c
> >> +++ b/common/partitions/efi.c
> >> @@ -449,6 +449,8 @@ static void efi_partition(void *buf, struct block_device *blk,
> >>  	snprintf(blk->cdev.diskuuid, sizeof(blk->cdev.diskuuid), "%pUl", &gpt->disk_guid);
> >>  	dev_add_param_string_fixed(blk->dev, "guid", blk->cdev.diskuuid);
> >>  
> >> +	blk->cdev.flags |= DEVFS_IS_GPT_PARTITIONED;
> >> +
> >>  	nb_part = le32_to_cpu(gpt->num_partition_entries);
> >>  
> >>  	if (nb_part > MAX_PARTITION) {
> >> diff --git a/fs/fs.c b/fs/fs.c
> >> index 9d8aab268ca4..2d2d327c5fbc 100644
> >> --- a/fs/fs.c
> >> +++ b/fs/fs.c
> >> @@ -94,6 +94,10 @@ void cdev_print(const struct cdev *cdev)
> >>  			printf(" table-partition");
> >>  		if (cdev->flags & DEVFS_IS_MCI_MAIN_PART_DEV)
> >>  			printf(" mci-main-partition");
> >> +		if (cdev->flags & DEVFS_IS_GPT_PARTITIONED)
> >> +			printf(" gpt-partitioned");
> >> +		if (cdev->flags & DEVFS_IS_MBR_PARTITIONED)
> >> +			printf(" mbr-partitioned");
> >>  		if (cdev->mtd)
> >>  			printf(" mtd");
> >>  		printf(" )");
> >> diff --git a/include/driver.h b/include/driver.h
> >> index 118d2adb6750..5f2eae65466f 100644
> >> --- a/include/driver.h
> >> +++ b/include/driver.h
> >> @@ -584,9 +584,23 @@ extern struct list_head cdev_list;
> >>  #define DEVFS_PARTITION_FIXED		(1U << 0)
> >>  #define DEVFS_PARTITION_READONLY	(1U << 1)
> >>  #define DEVFS_IS_CHARACTER_DEV		(1U << 3)
> >> -#define DEVFS_IS_MCI_MAIN_PART_DEV	(1U << 4)
> >> -#define DEVFS_PARTITION_FROM_OF		(1U << 5)
> >> -#define DEVFS_PARTITION_FROM_TABLE	(1U << 6)
> >> +#define DEVFS_PARTITION_FROM_OF		(1U << 4)
> >> +#define DEVFS_PARTITION_FROM_TABLE	(1U << 5)
> >> +#define DEVFS_IS_GPT_PARTITIONED	(1U << 6)
> >> +#define DEVFS_IS_MBR_PARTITIONED	(1U << 7)
> >> +#define DEVFS_IS_MCI_MAIN_PART_DEV	(1U << 8)
> > 
> > Why do you reorder the bits here again?
> 
> I wanted the partition bits to be together.
> 
> >> +
> >> +static inline bool cdev_is_gpt_partitioned(const struct cdev *master)
> >> +{
> >> +	return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
> >> +		== DEVFS_IS_GPT_PARTITIONED;
> > 
> > Why not just: 'return !!(master->flags & DEVFS_IS_GPT_PARTITIONED)' ?
> 
> Left over from when this was more than one bit. I will change to:
> 
>   return master && (master->flags & DEVFS_IS_GPT_PARTITIONED)
> 
> I want to keep the NULL check, as we only set this for the master device.

By 'as we only set this for the master device' you mean the flags right?
We don't expect the 'struct cdev*' parameter to be NULL.

Regards,
  Marco

> 
> Cheers,
> Ahmad
> 
> 
> > 
> > Regards,
> >   Marco
> > 
> >> +}
> >> +
> >> +static inline bool cdev_is_mbr_partitioned(const struct cdev *master)
> >> +{
> >> +	return master && (master->flags & DEVFS_IS_MBR_PARTITIONED)
> >> +		== DEVFS_IS_MBR_PARTITIONED;
> >> +}
> >>  
> >>  struct cdev *devfs_add_partition(const char *devname, loff_t offset,
> >>  		loff_t size, unsigned int flags, const char *name);
> >> -- 
> >> 2.39.2
> >>
> >>
> >>
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
>