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@xxxxxxxxxxxxxx>
>> Cc: Uwe Kleine-König <ukl@xxxxxxxxxxxxxx>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
