mfd: intel-m10-bmc: Change staging size to a variable

Message ID	20240328233559.6949-1-peter.colberg@intel.com (mailing list archive)
State	Superseded
Headers	show Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.11]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5053645946; Thu, 28 Mar 2024 23:36:23 +0000 (UTC) From: Peter Colberg <peter.colberg@intel.com> To: Moritz Fischer <mdf@kernel.org>, Wu Hao <hao.wu@intel.com>, Xu Yilun <yilun.xu@intel.com>, Tom Rix <trix@redhat.com>, Lee Jones <lee@kernel.org>, linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Russ Weight <russ.weight@linux.dev>, Marco Pagani <marpagan@redhat.com>, Matthew Gerlach <matthew.gerlach@linux.intel.com>, =?utf-8?q?Ilpo_J=C3=A4rv?= =?utf-8?q?inen?= <ilpo.jarvinen@linux.intel.com>, Russ Weight <russell.h.weight@intel.com>, Peter Colberg <peter.colberg@intel.com> Subject: [PATCH] mfd: intel-m10-bmc: Change staging size to a variable Date: Thu, 28 Mar 2024 19:35:59 -0400 Message-ID: <20240328233559.6949-1-peter.colberg@intel.com> Precedence: bulk MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Series	mfd: intel-m10-bmc: Change staging size to a variable \| expand mfd: intel-m10-bmc: Change staging size to a variable

Colberg, Peter March 28, 2024, 11:35 p.m. UTC

From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

The size of the staging area in FLASH for FPGA updates is dependent on the
size of the FPGA. Currently, the staging size is defined as a constant.
Larger FPGAs are coming soon and it will soon be necessary to support
different sizes for the staging area. Add a new staging_size member to the
csr_map structure to support a variable staging size.

The secure update driver does a sanity-check of the image size in
comparison to the size of the staging area in FLASH. Change the
staging size reference to a variable instead of a constant in order
to more readily support future, larger FPGAs.

Co-developed-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Peter Colberg <peter.colberg@intel.com>
---
 drivers/fpga/intel-m10-bmc-sec-update.c | 3 ++-
 drivers/mfd/intel-m10-bmc-pmci.c        | 1 +
 drivers/mfd/intel-m10-bmc-spi.c         | 1 +
 include/linux/mfd/intel-m10-bmc.h       | 1 +
 4 files changed, 5 insertions(+), 1 deletion(-)

Xu Yilun April 1, 2024, 9:46 a.m. UTC | #1

On Thu, Mar 28, 2024 at 07:35:59PM -0400, Peter Colberg wrote:
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> The size of the staging area in FLASH for FPGA updates is dependent on the
> size of the FPGA. Currently, the staging size is defined as a constant.
> Larger FPGAs are coming soon and it will soon be necessary to support

Soon? When? You cannot add some feature without a user case. If you do
have a use case, put the patch in the same patchset.

Thanks,
Yilun

> different sizes for the staging area. Add a new staging_size member to the
> csr_map structure to support a variable staging size.
> 
> The secure update driver does a sanity-check of the image size in
> comparison to the size of the staging area in FLASH. Change the
> staging size reference to a variable instead of a constant in order
> to more readily support future, larger FPGAs.
> 
> Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> ---
>  drivers/fpga/intel-m10-bmc-sec-update.c | 3 ++-
>  drivers/mfd/intel-m10-bmc-pmci.c        | 1 +
>  drivers/mfd/intel-m10-bmc-spi.c         | 1 +
>  include/linux/mfd/intel-m10-bmc.h       | 1 +
>  4 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> index 89851b133709..7ac9f9f5af12 100644
> --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> @@ -529,11 +529,12 @@ static enum fw_upload_err m10bmc_sec_prepare(struct fw_upload *fwl,
>  					     const u8 *data, u32 size)
>  {
>  	struct m10bmc_sec *sec = fwl->dd_handle;
> +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
>  	u32 ret;
>  
>  	sec->cancel_request = false;
>  
> -	if (!size || size > M10BMC_STAGING_SIZE)
> +	if (!size || size > csr_map->staging_size)
>  		return FW_UPLOAD_ERR_INVALID_SIZE;
>  
>  	if (sec->m10bmc->flash_bulk_ops)
> diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
> index 0392ef8b57d8..698c5933938b 100644
> --- a/drivers/mfd/intel-m10-bmc-pmci.c
> +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> @@ -370,6 +370,7 @@ static const struct m10bmc_csr_map m10bmc_n6000_csr_map = {
>  	.pr_reh_addr = M10BMC_N6000_PR_REH_ADDR,
>  	.pr_magic = M10BMC_N6000_PR_PROG_MAGIC,
>  	.rsu_update_counter = M10BMC_N6000_STAGING_FLASH_COUNT,
> +	.staging_size = M10BMC_STAGING_SIZE,
>  };
>  
>  static const struct intel_m10bmc_platform_info m10bmc_pmci_n6000 = {
> diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
> index cbeb7de9e041..d64d28199df6 100644
> --- a/drivers/mfd/intel-m10-bmc-spi.c
> +++ b/drivers/mfd/intel-m10-bmc-spi.c
> @@ -109,6 +109,7 @@ static const struct m10bmc_csr_map m10bmc_n3000_csr_map = {
>  	.pr_reh_addr = M10BMC_N3000_PR_REH_ADDR,
>  	.pr_magic = M10BMC_N3000_PR_PROG_MAGIC,
>  	.rsu_update_counter = M10BMC_N3000_STAGING_FLASH_COUNT,
> +	.staging_size = M10BMC_STAGING_SIZE,
>  };
>  
>  static struct mfd_cell m10bmc_d5005_subdevs[] = {
> diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> index ee66c9751003..988f1cd90032 100644
> --- a/include/linux/mfd/intel-m10-bmc.h
> +++ b/include/linux/mfd/intel-m10-bmc.h
> @@ -205,6 +205,7 @@ struct m10bmc_csr_map {
>  	unsigned int pr_reh_addr;
>  	unsigned int pr_magic;
>  	unsigned int rsu_update_counter;
> +	unsigned int staging_size;
>  };
>  
>  /**
> -- 
> 2.44.0
> 
>

Russ Weight April 1, 2024, 5:09 p.m. UTC | #2

On Mon, Apr 01, 2024 at 05:46:29PM +0800, Xu Yilun wrote:
> On Thu, Mar 28, 2024 at 07:35:59PM -0400, Peter Colberg wrote:
> > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > 
> > The size of the staging area in FLASH for FPGA updates is dependent on the
> > size of the FPGA. Currently, the staging size is defined as a constant.
> > Larger FPGAs are coming soon and it will soon be necessary to support
> 
> Soon? When? You cannot add some feature without a user case. If you do
> have a use case, put the patch in the same patchset.

There may never be an up-streamed use-case. This is a very small
change intended to make it easier for a third-party vendor to
build a card that requires a larger staging area in FLASH. They
would have to add a new "struct m10bmc_csr_map", but they
wouldn't have to refactor this code as part of the change

This change does not introduce an unused function or variable.
It is more of a clean-up, making the code more flexible.

Can it not be taken as is?

- Russ

> 
> Thanks,
> Yilun
> 
> > different sizes for the staging area. Add a new staging_size member to the
> > csr_map structure to support a variable staging size.
> > 
> > The secure update driver does a sanity-check of the image size in
> > comparison to the size of the staging area in FLASH. Change the
> > staging size reference to a variable instead of a constant in order
> > to more readily support future, larger FPGAs.
> > 
> > Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> > ---
> >  drivers/fpga/intel-m10-bmc-sec-update.c | 3 ++-
> >  drivers/mfd/intel-m10-bmc-pmci.c        | 1 +
> >  drivers/mfd/intel-m10-bmc-spi.c         | 1 +
> >  include/linux/mfd/intel-m10-bmc.h       | 1 +
> >  4 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > index 89851b133709..7ac9f9f5af12 100644
> > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > @@ -529,11 +529,12 @@ static enum fw_upload_err m10bmc_sec_prepare(struct fw_upload *fwl,
> >  					     const u8 *data, u32 size)
> >  {
> >  	struct m10bmc_sec *sec = fwl->dd_handle;
> > +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> >  	u32 ret;
> >  
> >  	sec->cancel_request = false;
> >  
> > -	if (!size || size > M10BMC_STAGING_SIZE)
> > +	if (!size || size > csr_map->staging_size)
> >  		return FW_UPLOAD_ERR_INVALID_SIZE;
> >  
> >  	if (sec->m10bmc->flash_bulk_ops)
> > diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
> > index 0392ef8b57d8..698c5933938b 100644
> > --- a/drivers/mfd/intel-m10-bmc-pmci.c
> > +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> > @@ -370,6 +370,7 @@ static const struct m10bmc_csr_map m10bmc_n6000_csr_map = {
> >  	.pr_reh_addr = M10BMC_N6000_PR_REH_ADDR,
> >  	.pr_magic = M10BMC_N6000_PR_PROG_MAGIC,
> >  	.rsu_update_counter = M10BMC_N6000_STAGING_FLASH_COUNT,
> > +	.staging_size = M10BMC_STAGING_SIZE,
> >  };
> >  
> >  static const struct intel_m10bmc_platform_info m10bmc_pmci_n6000 = {
> > diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
> > index cbeb7de9e041..d64d28199df6 100644
> > --- a/drivers/mfd/intel-m10-bmc-spi.c
> > +++ b/drivers/mfd/intel-m10-bmc-spi.c
> > @@ -109,6 +109,7 @@ static const struct m10bmc_csr_map m10bmc_n3000_csr_map = {
> >  	.pr_reh_addr = M10BMC_N3000_PR_REH_ADDR,
> >  	.pr_magic = M10BMC_N3000_PR_PROG_MAGIC,
> >  	.rsu_update_counter = M10BMC_N3000_STAGING_FLASH_COUNT,
> > +	.staging_size = M10BMC_STAGING_SIZE,
> >  };
> >  
> >  static struct mfd_cell m10bmc_d5005_subdevs[] = {
> > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> > index ee66c9751003..988f1cd90032 100644
> > --- a/include/linux/mfd/intel-m10-bmc.h
> > +++ b/include/linux/mfd/intel-m10-bmc.h
> > @@ -205,6 +205,7 @@ struct m10bmc_csr_map {
> >  	unsigned int pr_reh_addr;
> >  	unsigned int pr_magic;
> >  	unsigned int rsu_update_counter;
> > +	unsigned int staging_size;
> >  };
> >  
> >  /**
> > -- 
> > 2.44.0
> > 
> >

Russ Weight April 1, 2024, 5:19 p.m. UTC | #3

On Mon, Apr 01, 2024 at 10:09:05AM -0700, Russ Weight wrote:
> On Mon, Apr 01, 2024 at 05:46:29PM +0800, Xu Yilun wrote:
> > On Thu, Mar 28, 2024 at 07:35:59PM -0400, Peter Colberg wrote:
> > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > 
> > > The size of the staging area in FLASH for FPGA updates is dependent on the
> > > size of the FPGA. Currently, the staging size is defined as a constant.
> > > Larger FPGAs are coming soon and it will soon be necessary to support
> > 
> > Soon? When? You cannot add some feature without a user case. If you do
> > have a use case, put the patch in the same patchset.
> 
> There may never be an up-streamed use-case. This is a very small
> change intended to make it easier for a third-party vendor to
> build a card that requires a larger staging area in FLASH. They
> would have to add a new "struct m10bmc_csr_map", but they
> wouldn't have to refactor this code as part of the change
> 
> This change does not introduce an unused function or variable.
> It is more of a clean-up, making the code more flexible.
> 
> Can it not be taken as is?

Would it be acceptable to just change the commit message to something
like:

Do not hardwire the staging size in the secure update driver. Move
the staging size to the m10bmc_csr_map structure to make the size
assignment more flexible.

> 
> - Russ
> 
> > 
> > Thanks,
> > Yilun
> > 
> > > different sizes for the staging area. Add a new staging_size member to the
> > > csr_map structure to support a variable staging size.
> > > 
> > > The secure update driver does a sanity-check of the image size in
> > > comparison to the size of the staging area in FLASH. Change the
> > > staging size reference to a variable instead of a constant in order
> > > to more readily support future, larger FPGAs.
> > > 
> > > Co-developed-by: Russ Weight <russell.h.weight@intel.com>
> > > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > Signed-off-by: Peter Colberg <peter.colberg@intel.com>
> > > ---
> > >  drivers/fpga/intel-m10-bmc-sec-update.c | 3 ++-
> > >  drivers/mfd/intel-m10-bmc-pmci.c        | 1 +
> > >  drivers/mfd/intel-m10-bmc-spi.c         | 1 +
> > >  include/linux/mfd/intel-m10-bmc.h       | 1 +
> > >  4 files changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/fpga/intel-m10-bmc-sec-update.c b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > index 89851b133709..7ac9f9f5af12 100644
> > > --- a/drivers/fpga/intel-m10-bmc-sec-update.c
> > > +++ b/drivers/fpga/intel-m10-bmc-sec-update.c
> > > @@ -529,11 +529,12 @@ static enum fw_upload_err m10bmc_sec_prepare(struct fw_upload *fwl,
> > >  					     const u8 *data, u32 size)
> > >  {
> > >  	struct m10bmc_sec *sec = fwl->dd_handle;
> > > +	const struct m10bmc_csr_map *csr_map = sec->m10bmc->info->csr_map;
> > >  	u32 ret;
> > >  
> > >  	sec->cancel_request = false;
> > >  
> > > -	if (!size || size > M10BMC_STAGING_SIZE)
> > > +	if (!size || size > csr_map->staging_size)
> > >  		return FW_UPLOAD_ERR_INVALID_SIZE;
> > >  
> > >  	if (sec->m10bmc->flash_bulk_ops)
> > > diff --git a/drivers/mfd/intel-m10-bmc-pmci.c b/drivers/mfd/intel-m10-bmc-pmci.c
> > > index 0392ef8b57d8..698c5933938b 100644
> > > --- a/drivers/mfd/intel-m10-bmc-pmci.c
> > > +++ b/drivers/mfd/intel-m10-bmc-pmci.c
> > > @@ -370,6 +370,7 @@ static const struct m10bmc_csr_map m10bmc_n6000_csr_map = {
> > >  	.pr_reh_addr = M10BMC_N6000_PR_REH_ADDR,
> > >  	.pr_magic = M10BMC_N6000_PR_PROG_MAGIC,
> > >  	.rsu_update_counter = M10BMC_N6000_STAGING_FLASH_COUNT,
> > > +	.staging_size = M10BMC_STAGING_SIZE,
> > >  };
> > >  
> > >  static const struct intel_m10bmc_platform_info m10bmc_pmci_n6000 = {
> > > diff --git a/drivers/mfd/intel-m10-bmc-spi.c b/drivers/mfd/intel-m10-bmc-spi.c
> > > index cbeb7de9e041..d64d28199df6 100644
> > > --- a/drivers/mfd/intel-m10-bmc-spi.c
> > > +++ b/drivers/mfd/intel-m10-bmc-spi.c
> > > @@ -109,6 +109,7 @@ static const struct m10bmc_csr_map m10bmc_n3000_csr_map = {
> > >  	.pr_reh_addr = M10BMC_N3000_PR_REH_ADDR,
> > >  	.pr_magic = M10BMC_N3000_PR_PROG_MAGIC,
> > >  	.rsu_update_counter = M10BMC_N3000_STAGING_FLASH_COUNT,
> > > +	.staging_size = M10BMC_STAGING_SIZE,
> > >  };
> > >  
> > >  static struct mfd_cell m10bmc_d5005_subdevs[] = {
> > > diff --git a/include/linux/mfd/intel-m10-bmc.h b/include/linux/mfd/intel-m10-bmc.h
> > > index ee66c9751003..988f1cd90032 100644
> > > --- a/include/linux/mfd/intel-m10-bmc.h
> > > +++ b/include/linux/mfd/intel-m10-bmc.h
> > > @@ -205,6 +205,7 @@ struct m10bmc_csr_map {
> > >  	unsigned int pr_reh_addr;
> > >  	unsigned int pr_magic;
> > >  	unsigned int rsu_update_counter;
> > > +	unsigned int staging_size;
> > >  };
> > >  
> > >  /**
> > > -- 
> > > 2.44.0
> > > 
> > >

Xu Yilun April 2, 2024, 6:52 a.m. UTC | #4

On Mon, Apr 01, 2024 at 10:19:47AM -0700, Russ Weight wrote:
> 
> On Mon, Apr 01, 2024 at 10:09:05AM -0700, Russ Weight wrote:
> > On Mon, Apr 01, 2024 at 05:46:29PM +0800, Xu Yilun wrote:
> > > On Thu, Mar 28, 2024 at 07:35:59PM -0400, Peter Colberg wrote:
> > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > 
> > > > The size of the staging area in FLASH for FPGA updates is dependent on the
> > > > size of the FPGA. Currently, the staging size is defined as a constant.
> > > > Larger FPGAs are coming soon and it will soon be necessary to support
> > > 
> > > Soon? When? You cannot add some feature without a user case. If you do
> > > have a use case, put the patch in the same patchset.
> > 
> > There may never be an up-streamed use-case. This is a very small
> > change intended to make it easier for a third-party vendor to
> > build a card that requires a larger staging area in FLASH. They
> > would have to add a new "struct m10bmc_csr_map", but they
> > wouldn't have to refactor this code as part of the change

I'm OK with this description.

Peter, is that what you mean? Or you do have a board type to follow, in
which case you need to submit the new board type as well.

> > 
> > This change does not introduce an unused function or variable.
> > It is more of a clean-up, making the code more flexible.
> > 
> > Can it not be taken as is?
> 
> Would it be acceptable to just change the commit message to something
> like:
> 
> Do not hardwire the staging size in the secure update driver. Move
> the staging size to the m10bmc_csr_map structure to make the size
> assignment more flexible.

That would be much better.

Thanks,
Yilun

Colberg, Peter April 2, 2024, 6:44 p.m. UTC | #5

On Tue, 2024-04-02 at 14:52 +0800, Xu Yilun wrote:
> On Mon, Apr 01, 2024 at 10:19:47AM -0700, Russ Weight wrote:
> > 
> > On Mon, Apr 01, 2024 at 10:09:05AM -0700, Russ Weight wrote:
> > > On Mon, Apr 01, 2024 at 05:46:29PM +0800, Xu Yilun wrote:
> > > > On Thu, Mar 28, 2024 at 07:35:59PM -0400, Peter Colberg wrote:
> > > > > From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > > > 
> > > > > The size of the staging area in FLASH for FPGA updates is dependent on the
> > > > > size of the FPGA. Currently, the staging size is defined as a constant.
> > > > > Larger FPGAs are coming soon and it will soon be necessary to support
> > > > 
> > > > Soon? When? You cannot add some feature without a user case. If you do
> > > > have a use case, put the patch in the same patchset.
> > > 
> > > There may never be an up-streamed use-case. This is a very small
> > > change intended to make it easier for a third-party vendor to
> > > build a card that requires a larger staging area in FLASH. They
> > > would have to add a new "struct m10bmc_csr_map", but they
> > > wouldn't have to refactor this code as part of the change
> 
> I'm OK with this description.
> 
> Peter, is that what you mean?

Yes.

> Or you do have a board type to follow, in
> which case you need to submit the new board type as well.
> 
> > > 
> > > This change does not introduce an unused function or variable.
> > > It is more of a clean-up, making the code more flexible.
> > > 
> > > Can it not be taken as is?
> > 
> > Would it be acceptable to just change the commit message to something
> > like:
> > 
> > Do not hardwire the staging size in the secure update driver. Move
> > the staging size to the m10bmc_csr_map structure to make the size
> > assignment more flexible.
> 
> That would be much better.

Thanks Russ, thanks Yilun, I will send a revised patch.

Peter

> 
> Thanks,
> Yilun

mfd: intel-m10-bmc: Change staging size to a variable

Commit Message

Comments

Patch