Re: [PATCH 18/18] state: allow lookup of barebox state partition by Type GUID

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 01.06.23 10:11, Marco Felsch wrote:
> On 23-06-01, Ahmad Fatoum wrote:
>> On 31.05.23 22:01, Marco Felsch wrote:
>>> Hi Ahmad,
>>>
>>> On 23-05-31, Ahmad Fatoum wrote:
>>>> The backend device tree property so far always pointed at a partition.
>>>> Let's allow pointing it at GPT storage devices directly and lookup
>>>> the correct barebox state partition by the well-known type GUID:
>>>>
>>>>   4778ed65-bf42-45fa-9c5b-287a1dc4aab1
>>>
>>> we should add an example within the Documetation/ too.
>>
>> I'd wait until we have a new dt-utils release with the same
>> binding. Anything else would be confusing. 
> 
> Sure.
> 
>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>> ---
>>>>  common/state/state.c | 22 ++++++++++++++++++++++
>>>>  include/driver.h     | 17 +++++++++++++++++
>>>>  include/state.h      |  4 ++++
>>>>  3 files changed, 43 insertions(+)
>>>>
>>>> diff --git a/common/state/state.c b/common/state/state.c
>>>> index 88e246198fb8..8f56c60b0e82 100644
>>>> --- a/common/state/state.c
>>>> +++ b/common/state/state.c
>>>> @@ -21,8 +21,10 @@
>>>>  #include <fs.h>
>>>>  #include <crc.h>
>>>>  #include <init.h>
>>>> +#include <block.h>
>>>>  #include <linux/err.h>
>>>>  #include <linux/list.h>
>>>> +#include <linux/uuid.h>
>>>>  
>>>>  #include <linux/mtd/mtd-abi.h>
>>>>  #include <malloc.h>
>>>> @@ -595,6 +597,8 @@ static char *cdev_to_devpath(struct cdev *cdev, off_t *offset, size_t *size)
>>>>  }
>>>>  #endif
>>>>  
>>>> +static guid_t barebox_state_partition_guid = BAREBOX_STATE_PARTITION_GUID;
>>>> +
>>>>  /*
>>>>   * state_new_from_node - create a new state instance from a device_node
>>>>   *
>>>> @@ -641,6 +645,24 @@ struct state *state_new_from_node(struct device_node *node, bool readonly)
>>>>  		goto out_release_state;
>>>>  	}
>>>>  
>>>> +	/* Is the backend referencing an on-disk partitonable block device? */
>>>> +	if (cdev_is_block_disk(cdev)) {
>>>> +		struct cdev *partcdev = NULL;
>>>> +
>>>> +		if (cdev_is_gpt_partitioned(cdev))
>>>> +			partcdev = cdev_find_child_by_typeuuid(cdev, &barebox_state_partition_guid);
>>>> +
>>>> +		if (!partcdev) {
>>>> +			ret = -EINVAL;
>>>> +			goto out_release_state;
>>>> +		}
>>>> +
>>>> +		pr_debug("%s: backend GPT partition looked up via PartitionTypeGUID\n",
>>>> +			 node->full_name);
>>>> +
>>>> +		cdev = partcdev;
>>>> +	}
>>>
>>> What about having the above logic within a seperate function and the
>>> above code would be:
>>>
>>> 	if (cdev_is_block_disk(cdev))
>>> 		cdev = get_cdev_by_typeuuid(cdev, &barebox_state_partition_guid)
>>> 	
>>> 	if (!cdev) {
>>> 		ret = -EINVAL;
>>> 		goto out_release_state;
>>> 	}
>>>
>>> This way we would have everything in place to re-use the same logic for
>>> the barebox-environmnet too. What do you think?
>>
>> cdev from of_cdev_find not being available -> -EPROBE_DEFER
> 
> This should be checked earlier or do I miss something?

I don't think it aids readability to have constructs like:

if (ret)
	return ret;
if (check())
	ret = do_foo();
if (ret)
	return ret;

I want to print a debug message anyway, so I'd just keep if (ret) inside.

> 
>> cdev GPT partition not existing,despite backend pointing at root device -> -EINVAL
> 
> So this could follow without..
> 
>> So the cdev check needs to be within if (cdev_is_block_disk(cdev)).
> 
> having it within the if. The cdev_find_child_by_gpt_typeuuid() could
> return NULL in case nothing is found. But that's just a minor nit.

I want it to return errors codes. barebox-state doesn't care and would
return -EINVAL on IS_ERR unconditionally, but it may be useful to future
users to differentiate between disk is not gpt and disk is gpt but device
doesn't exist.

> 
> Regards,
>   Marco
> 
>> I don't object to combining cdev_is_gpt_partitioned and cdev_find_child_by_typeuuid into
>> cdev_find_child_by_gpt_typeuuid though.
>>
>>
>> Thanks for all the review!
>> Ahmad
>>
>>>
>>> Regards,
>>>   Marco
>>>
>>>> +
>>>>  	state->backend_path = cdev_to_devpath(cdev, &offset, &size);
>>>>  
>>>>  	pr_debug("%s: backend resolved to %s\n", node->full_name, state->backend_path);
>>>> diff --git a/include/driver.h b/include/driver.h
>>>> index 6407f7d6ba36..579b03fbac34 100644
>>>> --- a/include/driver.h
>>>> +++ b/include/driver.h
>>>> @@ -585,6 +585,23 @@ extern struct list_head cdev_list;
>>>>  #define for_each_cdev(cdev) \
>>>>  	list_for_each_entry(cdev, &cdev_list, list)
>>>>  
>>>> +#define for_each_cdev_partition(partcdev, cdev) \
>>>> +	list_for_each_entry((partcdev), &(cdev)->partitions, partition_entry)
>>>> +
>>>> +
>>>> +static inline struct cdev *cdev_find_child_by_typeuuid(struct cdev *cdev,
>>>> +						       guid_t *typeuuid)
>>>> +{
>>>> +	struct cdev *partcdev;
>>>> +
>>>> +	for_each_cdev_partition(partcdev, cdev) {
>>>> +		if (guid_equal(&partcdev->typeuuid, typeuuid))
>>>> +			return partcdev;
>>>> +	}
>>>> +
>>>> +	return NULL;
>>>> +}
>>>> +
>>>>  #define DEVFS_PARTITION_FIXED		(1U << 0)
>>>>  #define DEVFS_PARTITION_READONLY	(1U << 1)
>>>>  #define DEVFS_IS_CHARACTER_DEV		(1U << 3)
>>>> diff --git a/include/state.h b/include/state.h
>>>> index bffcd5a9007f..3daf82c0735f 100644
>>>> --- a/include/state.h
>>>> +++ b/include/state.h
>>>> @@ -62,4 +62,8 @@ static inline int state_read_mac(struct state *state, const char *name, u8 *buf)
>>>>  
>>>>  #endif /* #if IS_ENABLED(CONFIG_STATE) / #else */
>>>>  
>>>> +#define BAREBOX_STATE_PARTITION_GUID \
>>>> +	GUID_INIT(0x4778ed65, 0xbf42, 0x45fa, 0x9c, 0x5b, \
>>>> +		 0x28, 0x7a, 0x1d, 0xc4, 0xaa, 0xb1)
>>>> +
>>>>  #endif /* __STATE_H */
>>>> -- 
>>>> 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 |
>>
>>
> 

-- 
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 |