mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Bastian Krause <bst@pengutronix.de>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>, barebox@lists.infradead.org
Cc: "Uwe Kleine-König" <ukl@pengutronix.de>
Subject: Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
Date: Mon, 28 Jun 2021 11:35:56 +0200	[thread overview]
Message-ID: <52d74c00-f249-b87f-bbe9-0dbeb78ba1e0@pengutronix.de> (raw)
In-Reply-To: <20210628064036.25991-1-a.fatoum@pengutronix.de>

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?

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

  parent reply	other threads:[~2021-06-28  9:37 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-28  6:40 Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 2/5] ARM: stm32mp: migrate to barebox,machine-id-path Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable Ahmad Fatoum
2021-06-28  9:50   ` Bastian Krause
2021-06-28 10:12     ` Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 4/5] sandbox: dts: populate $global.machine_id Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 5/5] ARM: dts: stm32mp: retire barebox, provide-mac-address in favor of NVMEM Ahmad Fatoum
2021-06-28  9:28 ` [PATCH 1/5] common: machine_id: support /chosen/barebox,machine-id-path override Ahmad Fatoum
2021-06-28  9:35 ` Bastian Krause [this message]
2021-06-28 10:11   ` [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
2021-06-28 20:20 ` Sascha Hauer
     [not found] ` <CAMHeXxOT__KBUKG6GkNAEkqz4tMBBzuZ7OgnKa0_OX5hz-JEig@mail.gmail.com>
2021-06-30 10:27   ` Ahmad Fatoum
2021-06-30 20:13     ` Trent Piepho
2021-09-15 10:55       ` Ahmad Fatoum

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52d74c00-f249-b87f-bbe9-0dbeb78ba1e0@pengutronix.de \
    --to=bst@pengutronix.de \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --cc=ukl@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox