diff mbox series

[PATCHv2,1/4] fwu: gpt: use cached meta-data partition numbers

Message ID 20221203031651.579771-1-jassisinghbrar@gmail.com
State Changes Requested
Delegated to: Ilias Apalodimas
Headers show
Series FWU: Handle meta-data in common code | expand

Commit Message

Jassi Brar Dec. 3, 2022, 3:16 a.m. UTC 
From: Jassi Brar <jaswinder.singh@linaro.org>

Use cached values and avoid parsing and scanning through partitions
everytime for meta-data partitions because they can't change after bootup.

Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
---
 drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

Comments

Ilias Apalodimas Dec. 22, 2022, 12:45 p.m. UTC | #1 
On Fri, Dec 02, 2022 at 09:16:51PM -0600, jassisinghbrar@gmail.com wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> Use cached values and avoid parsing and scanning through partitions
> everytime for meta-data partitions because they can't change after bootup.
>
> Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> index d35ce49c5c..28f5d23e1e 100644
> --- a/drivers/fwu-mdata/gpt_blk.c
> +++ b/drivers/fwu-mdata/gpt_blk.c
> @@ -24,8 +24,9 @@ enum {
>  	MDATA_WRITE,
>  };
>
> -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> -				    uint mdata_parts[2])
> +static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
> +
> +static int gpt_get_mdata_partitions(struct blk_desc *desc)
>  {
>  	int i, ret;
>  	u32 nparts;
> @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
>  	struct disk_partition info;
>  	const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
>
> +	/* if primary and secondary partitions already found */
> +	if (g_mdata_part[0] && g_mdata_part[1])
> +		return 0;
> +
>  	nparts = 0;
> -	for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> +	for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
>  		if (part_get_info(desc, i, &info))
>  			continue;
>  		uuid_str_to_bin(info.type_guid, part_type_guid.b,
>  				UUID_STR_FORMAT_GUID);
>
> -		if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
> -			if (nparts < 2)
> -				mdata_parts[nparts] = i;
> -			++nparts;
> -		}
> +		if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
> +			g_mdata_part[nparts++] = i;

The reason the 'if (nparts < 2)' was outside the main loop was to show
errors in case the user defined more than two partitions.  Can we keep it
like that or am I the only being paranoid here?

>  	}
>
>  	if (nparts != 2) {
> @@ -127,26 +129,25 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
>  {
>  	int ret;
>  	struct blk_desc *desc;
> -	uint mdata_parts[2];
>  	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
>
>  	desc = dev_get_uclass_plat(priv->blk_dev);
>
> -	ret = gpt_get_mdata_partitions(desc, mdata_parts);
> +	ret = gpt_get_mdata_partitions(desc);
>  	if (ret < 0) {
>  		log_debug("Error getting the FWU metadata partitions\n");
>  		return -ENOENT;
>  	}
>
>  	/* First write the primary partition */
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[0]);
> +	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
>  	if (ret < 0) {
>  		log_debug("Updating primary FWU metadata partition failed\n");
>  		return ret;
>  	}
>
>  	/* And now the replica */
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[1]);
> +	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
>  	if (ret < 0) {
>  		log_debug("Updating secondary FWU metadata partition failed\n");
>  		return ret;
> @@ -158,16 +159,14 @@ static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
>  static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
>  {
>  	int ret;
> -	uint mdata_parts[2];
> -
> -	ret = gpt_get_mdata_partitions(desc, mdata_parts);
>
> +	ret = gpt_get_mdata_partitions(desc);
>  	if (ret < 0) {
>  		log_debug("Error getting the FWU metadata partitions\n");
>  		return -ENOENT;
>  	}
>
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[0]);
> +	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
>  	if (ret < 0) {
>  		log_debug("Failed to read the FWU metadata from the device\n");
>  		return -EIO;
> @@ -182,7 +181,7 @@ static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
>  	 * Try to read the replica.
>  	 */
>  	memset(mdata, '\0', sizeof(struct fwu_mdata));
> -	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[1]);
> +	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
>  	if (ret < 0) {
>  		log_debug("Failed to read the FWU metadata from the device\n");
>  		return -EIO;
> @@ -206,9 +205,15 @@ static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
>  static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
>  {
>  	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
> +	int err;
> +
> +	err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
> +	if (!err) {
> +		mdata_parts[0] = g_mdata_part[0];
> +		mdata_parts[1] = g_mdata_part[1];
> +	}
>
> -	return gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev),
> -					mdata_parts);
> +	return err;
>  }
>
>  static int fwu_gpt_read_mdata_partition(struct udevice *dev,
> --
> 2.34.1
>

Thanks
/Ilias
Jassi Brar Jan. 2, 2023, 5:15 p.m. UTC | #2 
On Thu, 22 Dec 2022 at 06:45, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, Dec 02, 2022 at 09:16:51PM -0600, jassisinghbrar@gmail.com wrote:
> > From: Jassi Brar <jaswinder.singh@linaro.org>
> >
> > Use cached values and avoid parsing and scanning through partitions
> > everytime for meta-data partitions because they can't change after bootup.
> >
> > Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
> > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > ---
> >  drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
> >  1 file changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > index d35ce49c5c..28f5d23e1e 100644
> > --- a/drivers/fwu-mdata/gpt_blk.c
> > +++ b/drivers/fwu-mdata/gpt_blk.c
> > @@ -24,8 +24,9 @@ enum {
> >       MDATA_WRITE,
> >  };
> >
> > -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > -                                 uint mdata_parts[2])
> > +static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
> > +
> > +static int gpt_get_mdata_partitions(struct blk_desc *desc)
> >  {
> >       int i, ret;
> >       u32 nparts;
> > @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
> >       struct disk_partition info;
> >       const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
> >
> > +     /* if primary and secondary partitions already found */
> > +     if (g_mdata_part[0] && g_mdata_part[1])
> > +             return 0;
> > +
> >       nparts = 0;
> > -     for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> > +     for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
> >               if (part_get_info(desc, i, &info))
> >                       continue;
> >               uuid_str_to_bin(info.type_guid, part_type_guid.b,
> >                               UUID_STR_FORMAT_GUID);
> >
> > -             if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
> > -                     if (nparts < 2)
> > -                             mdata_parts[nparts] = i;
> > -                     ++nparts;
> > -             }
> > +             if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
> > +                     g_mdata_part[nparts++] = i;
>
> The reason the 'if (nparts < 2)' was outside the main loop was to show
> errors in case the user defined more than two partitions.  Can we keep it
> like that or am I the only being paranoid here?
>
I am not sure if that is an 'error', because FWU only cares about the
first two partitions it comes across. There is no way other ones could
impact the operations.
And if fwu code is responsible for correctness of the setup, there are
things that we already don't care about. So maybe we keep the code
simple?

cheers.
Etienne Carriere Jan. 4, 2023, 11:45 a.m. UTC | #3 
On Mon, 2 Jan 2023 at 18:15, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>
> On Thu, 22 Dec 2022 at 06:45, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, Dec 02, 2022 at 09:16:51PM -0600, jassisinghbrar@gmail.com wrote:
> > > From: Jassi Brar <jaswinder.singh@linaro.org>
> > >
> > > Use cached values and avoid parsing and scanning through partitions
> > > everytime for meta-data partitions because they can't change after bootup.
> > >
> > > Acked-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> > > ---
> > >  drivers/fwu-mdata/gpt_blk.c | 43 +++++++++++++++++++++----------------
> > >  1 file changed, 24 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
> > > index d35ce49c5c..28f5d23e1e 100644
> > > --- a/drivers/fwu-mdata/gpt_blk.c
> > > +++ b/drivers/fwu-mdata/gpt_blk.c
> > > @@ -24,8 +24,9 @@ enum {
> > >       MDATA_WRITE,
> > >  };
> > >
> > > -static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > > -                                 uint mdata_parts[2])
> > > +static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
> > > +
> > > +static int gpt_get_mdata_partitions(struct blk_desc *desc)
> > >  {
> > >       int i, ret;
> > >       u32 nparts;
> > > @@ -33,18 +34,19 @@ static int gpt_get_mdata_partitions(struct blk_desc *desc,
> > >       struct disk_partition info;
> > >       const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
> > >
> > > +     /* if primary and secondary partitions already found */
> > > +     if (g_mdata_part[0] && g_mdata_part[1])
> > > +             return 0;
> > > +
> > >       nparts = 0;
> > > -     for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
> > > +     for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
> > >               if (part_get_info(desc, i, &info))
> > >                       continue;
> > >               uuid_str_to_bin(info.type_guid, part_type_guid.b,
> > >                               UUID_STR_FORMAT_GUID);
> > >
> > > -             if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
> > > -                     if (nparts < 2)
> > > -                             mdata_parts[nparts] = i;
> > > -                     ++nparts;
> > > -             }
> > > +             if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
> > > +                     g_mdata_part[nparts++] = i;
> >
> > The reason the 'if (nparts < 2)' was outside the main loop was to show
> > errors in case the user defined more than two partitions.  Can we keep it
> > like that or am I the only being paranoid here?
> >
> I am not sure if that is an 'error', because FWU only cares about the
> first two partitions it comes across. There is no way other ones could
> impact the operations.
> And if fwu code is responsible for correctness of the setup, there are
> things that we already don't care about. So maybe we keep the code
> simple?
>
> cheers.

I agree with Jassi. We can keep code simple here. The embedded code is
not designed to valide DT content but to use it.

Etienne
diff mbox series

Patch

diff --git a/drivers/fwu-mdata/gpt_blk.c b/drivers/fwu-mdata/gpt_blk.c
index d35ce49c5c..28f5d23e1e 100644
--- a/drivers/fwu-mdata/gpt_blk.c
+++ b/drivers/fwu-mdata/gpt_blk.c
@@ -24,8 +24,9 @@  enum {
 	MDATA_WRITE,
 };
 
-static int gpt_get_mdata_partitions(struct blk_desc *desc,
-				    uint mdata_parts[2])
+static uint g_mdata_part[2]; /* = {0, 0} to check against uninit parts */
+
+static int gpt_get_mdata_partitions(struct blk_desc *desc)
 {
 	int i, ret;
 	u32 nparts;
@@ -33,18 +34,19 @@  static int gpt_get_mdata_partitions(struct blk_desc *desc,
 	struct disk_partition info;
 	const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID;
 
+	/* if primary and secondary partitions already found */
+	if (g_mdata_part[0] && g_mdata_part[1])
+		return 0;
+
 	nparts = 0;
-	for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) {
+	for (i = 1; i < MAX_SEARCH_PARTITIONS && nparts < 2; i++) {
 		if (part_get_info(desc, i, &info))
 			continue;
 		uuid_str_to_bin(info.type_guid, part_type_guid.b,
 				UUID_STR_FORMAT_GUID);
 
-		if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) {
-			if (nparts < 2)
-				mdata_parts[nparts] = i;
-			++nparts;
-		}
+		if (!guidcmp(&fwu_mdata_guid, &part_type_guid))
+			g_mdata_part[nparts++] = i;
 	}
 
 	if (nparts != 2) {
@@ -127,26 +129,25 @@  static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
 {
 	int ret;
 	struct blk_desc *desc;
-	uint mdata_parts[2];
 	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
 
 	desc = dev_get_uclass_plat(priv->blk_dev);
 
-	ret = gpt_get_mdata_partitions(desc, mdata_parts);
+	ret = gpt_get_mdata_partitions(desc);
 	if (ret < 0) {
 		log_debug("Error getting the FWU metadata partitions\n");
 		return -ENOENT;
 	}
 
 	/* First write the primary partition */
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[0]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[0]);
 	if (ret < 0) {
 		log_debug("Updating primary FWU metadata partition failed\n");
 		return ret;
 	}
 
 	/* And now the replica */
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, mdata_parts[1]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_WRITE, g_mdata_part[1]);
 	if (ret < 0) {
 		log_debug("Updating secondary FWU metadata partition failed\n");
 		return ret;
@@ -158,16 +159,14 @@  static int fwu_gpt_update_mdata(struct udevice *dev, struct fwu_mdata *mdata)
 static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
 {
 	int ret;
-	uint mdata_parts[2];
-
-	ret = gpt_get_mdata_partitions(desc, mdata_parts);
 
+	ret = gpt_get_mdata_partitions(desc);
 	if (ret < 0) {
 		log_debug("Error getting the FWU metadata partitions\n");
 		return -ENOENT;
 	}
 
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[0]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[0]);
 	if (ret < 0) {
 		log_debug("Failed to read the FWU metadata from the device\n");
 		return -EIO;
@@ -182,7 +181,7 @@  static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata *mdata)
 	 * Try to read the replica.
 	 */
 	memset(mdata, '\0', sizeof(struct fwu_mdata));
-	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, mdata_parts[1]);
+	ret = gpt_read_write_mdata(desc, mdata, MDATA_READ, g_mdata_part[1]);
 	if (ret < 0) {
 		log_debug("Failed to read the FWU metadata from the device\n");
 		return -EIO;
@@ -206,9 +205,15 @@  static int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata *mdata)
 static int fwu_gpt_get_mdata_partitions(struct udevice *dev, uint *mdata_parts)
 {
 	struct fwu_mdata_gpt_blk_priv *priv = dev_get_priv(dev);
+	int err;
+
+	err = gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev));
+	if (!err) {
+		mdata_parts[0] = g_mdata_part[0];
+		mdata_parts[1] = g_mdata_part[1];
+	}
 
-	return gpt_get_mdata_partitions(dev_get_uclass_plat(priv->blk_dev),
-					mdata_parts);
+	return err;
 }
 
 static int fwu_gpt_read_mdata_partition(struct udevice *dev,