Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override

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

Hello Basti,

On 28.06.21 11:35, Bastian Krause wrote:
> On 6/28/21 8:40 AM, Ahmad Fatoum wrote:
>> The Kconfig option already warns that the current behavior of
>> machine_id_set_hashable() overriding previous calls can lead to the
>> machine-id changing over updates. We don't yet have this problem in
>> practice, because the only two upstream users are for bsec and ocotp,
>> which are efuse drivers for different SoCs. On the other hand, users
>> may want to get the unique ID from an EEPROM and with deep probe
>> support, the initcall ordering will be independent of the actual probe
>> order.
>>
>> Work around this issue by introducing a way for each board to explicitly
>> reference a nvmem cell that should be hashed to produce the machine-id.
>>
>> If no such device tree property is supplied, the last call to
>> machine_id_set_hashable() will be used as before.
>>
>> Cc: Bastian Krause <bst@pengutronix.de>
>> Cc: Uwe Kleine-König <ukl@pengutronix.de>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/Kconfig                 | 13 ++++++----
>>  common/machine_id.c            | 41 ++++++++++++++++++++++++++++---
>>  drivers/nvmem/core.c           | 44 ++++++++++++++++++++++++----------
>>  drivers/of/base.c              | 11 +++++++++
>>  include/linux/nvmem-consumer.h |  5 ++++
>>  include/of.h                   |  6 +++++
>>  6 files changed, 99 insertions(+), 21 deletions(-)
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index 54f1eb633a32..a4a109f04f08 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -1066,13 +1066,16 @@ config MACHINE_ID
>>  	bool "compute unique machine-id"
>>  	depends on FLEXIBLE_BOOTARGS
>>  	depends on SHA1
>> +	select NVMEM
>>  	help
>>  	  Compute a persistent machine-specific id and store it to $global.machine_id.
>> -	  The id is a hash of device-specific information added via
>> -	  machine_id_set_hashable(). If multiple sources are available, the
>> -	  information provided by the last call prior to the late initcall
>> -	  set_machine_id() is used to generate the machine id from. Thus when
>> -	  updating barebox the machine id might change.
>> +	  The id is a hash of device-specific information added either via
>> +	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
>> +	  /chosen/barebox,machine-id device tree property.
>> +
>> +	  With machine_id_set_hashable(), the last call prior to the late initcall
>> +	  set_machine_id() willl be used to generate the machine id. This means
>> +	  updating barebox may change the machine id!
>>  
>>  	  global.bootm.provide_machine_id may be used to automatically set
>>  	  the linux.bootargs.machine_id global variable with a value of
>> diff --git a/common/machine_id.c b/common/machine_id.c
>> index 6480806cd287..fd2f0888a6cd 100644
>> --- a/common/machine_id.c
>> +++ b/common/machine_id.c
>> @@ -10,6 +10,9 @@
>>  #include <magicvar.h>
>>  #include <crypto/sha.h>
>>  #include <machine_id.h>
>> +#include <linux/err.h>
>> +#include <linux/nvmem-consumer.h>
>> +#include <of.h>
>>  
>>  #define MACHINE_ID_LENGTH 32
>>  
>> @@ -24,16 +27,49 @@ void machine_id_set_hashable(const void *hashable, size_t len)
>>  	__machine_id_hashable_length = len;
>>  }
>>  
>> +static const void *machine_id_get_hashable(size_t *len)
>> +{
>> +	struct device_node *np = of_get_machineidpath();
>> +	struct nvmem_cell *cell;
>> +	void *ret;
>> +
>> +	if (!np)
>> +		goto no_cell;
>> +
>> +	cell = of_nvmem_cell_get_from_node(np);
>> +	if (IS_ERR(cell)) {
>> +		pr_err("Invalid barebox,machine-id-path: %pe\n", cell);
>> +		goto no_cell;
>> +	}
>> +
>> +	ret = nvmem_cell_read(cell, len);
>> +	nvmem_cell_put(cell);
>> +	if (IS_ERR(ret)) {
>> +		pr_err("Couldn't read from barebox,machine-id-path: %pe\n", ret);
>> +		goto no_cell;
>> +	}
>> +
>> +	return ret;
>> +
>> +no_cell:
>> +	*len = __machine_id_hashable_length;
>> +	return __machine_id_hashable;
>> +}
>> +
>>  static int machine_id_set_globalvar(void)
>>  {
>>  	struct digest *digest = NULL;
>>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>>  	char hex_machine_id[MACHINE_ID_LENGTH];
>>  	char *env_machine_id;
>> +	const void *hashable;
>> +	size_t length;
>>  	int ret = 0;
>>  
>> +	hashable = machine_id_get_hashable(&length);
>> +
>>  	/* nothing to do if no hashable information provided */
>> -	if (!__machine_id_hashable)
>> +	if (!hashable)
>>  		goto out;
>>  
>>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
>> @@ -41,8 +77,7 @@ static int machine_id_set_globalvar(void)
>>  	if (ret)
>>  		goto out;
>>  
>> -	ret = digest_update(digest, __machine_id_hashable,
>> -			    __machine_id_hashable_length);
>> +	ret = digest_update(digest, hashable, length);
>>  	if (ret)
>>  		goto out;
>>  
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 4e558e165063..980304a8078b 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -361,29 +361,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
>>  
>>  #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
>>  /**
>> - * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>> + * of_nvmem_cell_get_from_node() - Get a nvmem cell from cell device node
>>   *
>> - * @dev node: Device tree node that uses the nvmem cell
>> - * @id: nvmem cell name from nvmem-cell-names property.
>> + * @cell_np: Device tree node of the nvmem cell
>>   *
>>   * Return: Will be an ERR_PTR() on error or a valid pointer
>>   * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>>   * nvmem_cell_put().
>>   */
>> -struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>> -					    const char *name)
>> +struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
>>  {
>> -	struct device_node *cell_np, *nvmem_np;
>> +	struct device_node *nvmem_np;
>>  	struct nvmem_cell *cell;
>>  	struct nvmem_device *nvmem;
>>  	const __be32 *addr;
>> -	int rval, len, index;
>> -
>> -	index = of_property_match_string(np, "nvmem-cell-names", name);
>> -
>> -	cell_np = of_parse_phandle(np, "nvmem-cells", index);
>> -	if (!cell_np)
>> -		return ERR_PTR(-EINVAL);
>> +	int rval, len;
>>  
>>  	nvmem_np = of_get_parent(cell_np);
>>  	if (!nvmem_np)
>> @@ -445,6 +437,32 @@ err_mem:
>>  
>>  	return ERR_PTR(rval);
>>  }
>> +EXPORT_SYMBOL_GPL(of_nvmem_cell_get_from_node);
>> +
>> +/**
>> + * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>> + *
>> + * @dev node: Device tree node that uses the nvmem cell
>> + * @id: nvmem cell name from nvmem-cell-names property.
>> + *
>> + * Return: Will be an ERR_PTR() on error or a valid pointer
>> + * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>> + * nvmem_cell_put().
>> + */
>> +struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>> +					    const char *name)
>> +{
>> +	struct device_node *cell_np;
>> +	int index;
>> +
>> +	index = of_property_match_string(np, "nvmem-cell-names", name);
>> +
>> +	cell_np = of_parse_phandle(np, "nvmem-cells", index);
>> +	if (!cell_np)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return of_nvmem_cell_get_from_node(cell_np);
>> +}
>>  EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>>  #endif
> 
> Shouldn't these changes be part of a separate patch?

For barebox, I usually fold smaller changes into the commit that depends on them.
Sascha was ok with it so far. When the changes get bigger, I split them up,
but here I didn't deem this necessary.

> 
> Regards,
> Bastian
> 
>>  
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 4104d7589305..a5b74707fc4f 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -2525,6 +2525,17 @@ int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate)
>>  	return true;
>>  }
>>  
>> +struct device_node *of_get_machineidpath(void)
>> +{
>> +	const char *name;
>> +
>> +	name = of_get_property(of_chosen, "barebox,machine-id-path", NULL);
>> +	if (!name)
>> +		return NULL;
>> +
>> +	return of_find_node_by_path_or_alias(NULL, name);
>> +}
>> +
>>  /**
>>   * of_add_initrd - add initrd properties to the devicetree
>>   * @root - the root node of the tree
>> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
>> index b979f23372a6..639a7ebbabae 100644
>> --- a/include/linux/nvmem-consumer.h
>> +++ b/include/linux/nvmem-consumer.h
>> @@ -122,11 +122,16 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
>>  #endif /* CONFIG_NVMEM */
>>  
>>  #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
>> +struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np);
>>  struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>  				     const char *name);
>>  struct nvmem_device *of_nvmem_device_get(struct device_node *np,
>>  					 const char *name);
>>  #else
>> +static inline struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
>> +{
>> +	return ERR_PTR(-ENOSYS);
>> +}
>>  static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>  				     const char *name)
>>  {
>> diff --git a/include/of.h b/include/of.h
>> index d82790c0523e..677f48d0aba1 100644
>> --- a/include/of.h
>> +++ b/include/of.h
>> @@ -290,6 +290,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev);
>>  int of_partitions_register_fixup(struct cdev *cdev);
>>  struct device_node *of_get_stdoutpath(unsigned int *);
>>  int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate);
>> +struct device_node *of_get_machineidpath(void);
>>  const char *of_get_model(void);
>>  void *of_flatten_dtb(struct device_node *node);
>>  int of_add_memory(struct device_node *node, bool dump);
>> @@ -336,6 +337,11 @@ static inline int of_device_is_stdout_path(struct device_d *dev, unsigned int *b
>>  	return 0;
>>  }
>>  
>> +static inline struct device_node *of_get_machineidpath(void)
>> +{
>> +	return NULL;
>> +}
>> +
>>  static inline const char *of_get_model(void)
>>  {
>>  	return NULL;
>>
> 
> 

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

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox