| Message ID | 20241023100702.12606-2-konrad.knitter@intel.com |
|---|---|
| State | Under Review |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | support FW Recovery Mode | expand |
Dear Konrad, Thank you for your patch. Am 23.10.24 um 12:07 schrieb Konrad Knitter: > Enable update of a selected component. It’d be great if you used that for the summary/title to make it a statement (by adding a verb in imperative mood). It’d be great, if you elaborated, what that feature is, and included the documentation used for the implementation. Also, how can it be tested? > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Konrad Knitter <konrad.knitter@intel.com> > --- > include/linux/pldmfw.h | 8 ++++++++ > lib/pldmfw/pldmfw.c | 8 ++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h > index 0fc831338226..f5047983004f 100644 > --- a/include/linux/pldmfw.h > +++ b/include/linux/pldmfw.h > @@ -125,9 +125,17 @@ struct pldmfw_ops; > * a pointer to their own data, used to implement the device specific > * operations. > */ > + > +enum pldmfw_update_mode { > + PLDMFW_UPDATE_MODE_FULL, > + PLDMFW_UPDATE_MODE_SINGLE_COMPONENT, > +}; > + > struct pldmfw { > const struct pldmfw_ops *ops; > struct device *dev; > + u16 component_identifier; > + enum pldmfw_update_mode mode; > }; > > bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record); > diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c > index 6e1581b9a616..6264e2013f25 100644 > --- a/lib/pldmfw/pldmfw.c > +++ b/lib/pldmfw/pldmfw.c > @@ -481,9 +481,17 @@ static int pldm_parse_components(struct pldmfw_priv *data) > component->component_data = data->fw->data + offset; > component->component_size = size; > > + if (data->context->mode == PLDMFW_UPDATE_MODE_SINGLE_COMPONENT && > + data->context->component_identifier != component->identifier) > + continue; > + > list_add_tail(&component->entry, &data->components); > } > > + if (data->context->mode == PLDMFW_UPDATE_MODE_SINGLE_COMPONENT && > + list_empty(&data->components)) > + return -ENOENT; > + > header_crc_ptr = data->fw->data + data->offset; > > err = pldm_move_fw_offset(data, sizeof(data->header_crc)); Kind regards, Paul
> -----Original Message----- > From: Paul Menzel <pmenzel@molgen.mpg.de> > Sent: Wednesday, October 23, 2024 12:07 PM > To: Knitter, Konrad <konrad.knitter@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; Keller, Jacob E > <jacob.e.keller@intel.com>; netdev@vger.kernel.org; jiri@resnulli.us; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; linux-kernel@vger.kernel.org; Nguyen, Anthony L > <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Marcin Szycik > <marcin.szycik@linux.intel.com> > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] pldmfw: selected > component update > > Dear Konrad, > > > Thank you for your patch. > > Am 23.10.24 um 12:07 schrieb Konrad Knitter: > > Enable update of a selected component. > > It’d be great if you used that for the summary/title to make it a > statement (by adding a verb in imperative mood). > > It’d be great, if you elaborated, what that feature is, and included the > documentation used for the implementation. > Please comment if this would be enough: This patch enables to update a selected component from PLDM image containing multiple components. Example usage: struct pldmfw; data.mode = PLDMFW_UPDATE_MODE_SINGLE_COMPONENT; data.compontent_identifier = DRIVER_FW_MGMT_COMPONENT_ID; >> Also, how can it be tested? This is enabler for ice patch, where on legacy FW versions where only "fw.mgmt" component can is allowed to be flashed in recovery mode. There are no dedicated images for recovery. Usual image contains multiple components. This patch can be then tested together with other patches in series on Intel E810 NIC - where you can now execute update full card or just "fw.mgmt" component. > > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com> > > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > > Signed-off-by: Konrad Knitter <konrad.knitter@intel.com> > > --- > > include/linux/pldmfw.h | 8 ++++++++ > > lib/pldmfw/pldmfw.c | 8 ++++++++ > > 2 files changed, 16 insertions(+) > > > > diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h > > index 0fc831338226..f5047983004f 100644 > > --- a/include/linux/pldmfw.h > > +++ b/include/linux/pldmfw.h > > @@ -125,9 +125,17 @@ struct pldmfw_ops; > > * a pointer to their own data, used to implement the device specific > > * operations. > > */ > > + > > +enum pldmfw_update_mode { > > + PLDMFW_UPDATE_MODE_FULL, > > + PLDMFW_UPDATE_MODE_SINGLE_COMPONENT, > > +}; > > + > > struct pldmfw { > > const struct pldmfw_ops *ops; > > struct device *dev; > > + u16 component_identifier; > > + enum pldmfw_update_mode mode; > > }; > > > > bool pldmfw_op_pci_match_record(struct pldmfw *context, struct > pldmfw_record *record); > > diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c > > index 6e1581b9a616..6264e2013f25 100644 > > --- a/lib/pldmfw/pldmfw.c > > +++ b/lib/pldmfw/pldmfw.c > > @@ -481,9 +481,17 @@ static int pldm_parse_components(struct > pldmfw_priv *data) > > component->component_data = data->fw->data + offset; > > component->component_size = size; > > > > + if (data->context->mode == > PLDMFW_UPDATE_MODE_SINGLE_COMPONENT && > > + data->context->component_identifier != component- > >identifier) > > + continue; > > + > > list_add_tail(&component->entry, &data->components); > > } > > > > + if (data->context->mode == > PLDMFW_UPDATE_MODE_SINGLE_COMPONENT && > > + list_empty(&data->components)) > > + return -ENOENT; > > + > > header_crc_ptr = data->fw->data + data->offset; > > > > err = pldm_move_fw_offset(data, sizeof(data->header_crc)); > > > Kind regards, > > Paul
> -----Original Message----- > From: Knitter, Konrad <konrad.knitter@intel.com> > Sent: Wednesday, October 23, 2024 3:07 AM > To: intel-wired-lan@lists.osuosl.org > Cc: Keller, Jacob E <jacob.e.keller@intel.com>; netdev@vger.kernel.org; > jiri@resnulli.us; davem@davemloft.net; edumazet@google.com; > kuba@kernel.org; pabeni@redhat.com; linux-kernel@vger.kernel.org; Nguyen, > Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Knitter, Konrad <konrad.knitter@intel.com>; > Marcin Szycik <marcin.szycik@linux.intel.com> > Subject: [PATCH iwl-next v1 1/3] pldmfw: selected component update > > Enable update of a selected component. > > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Konrad Knitter <konrad.knitter@intel.com> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Thanks for the PLDM improvement!
diff --git a/include/linux/pldmfw.h b/include/linux/pldmfw.h index 0fc831338226..f5047983004f 100644 --- a/include/linux/pldmfw.h +++ b/include/linux/pldmfw.h @@ -125,9 +125,17 @@ struct pldmfw_ops; * a pointer to their own data, used to implement the device specific * operations. */ + +enum pldmfw_update_mode { + PLDMFW_UPDATE_MODE_FULL, + PLDMFW_UPDATE_MODE_SINGLE_COMPONENT, +}; + struct pldmfw { const struct pldmfw_ops *ops; struct device *dev; + u16 component_identifier; + enum pldmfw_update_mode mode; }; bool pldmfw_op_pci_match_record(struct pldmfw *context, struct pldmfw_record *record); diff --git a/lib/pldmfw/pldmfw.c b/lib/pldmfw/pldmfw.c index 6e1581b9a616..6264e2013f25 100644 --- a/lib/pldmfw/pldmfw.c +++ b/lib/pldmfw/pldmfw.c @@ -481,9 +481,17 @@ static int pldm_parse_components(struct pldmfw_priv *data) component->component_data = data->fw->data + offset; component->component_size = size; + if (data->context->mode == PLDMFW_UPDATE_MODE_SINGLE_COMPONENT && + data->context->component_identifier != component->identifier) + continue; + list_add_tail(&component->entry, &data->components); } + if (data->context->mode == PLDMFW_UPDATE_MODE_SINGLE_COMPONENT && + list_empty(&data->components)) + return -ENOENT; + header_crc_ptr = data->fw->data + data->offset; err = pldm_move_fw_offset(data, sizeof(data->header_crc));