| Message ID | 20240318162958.991829-1-paul.greenwalt@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v1] ice: Fix package download algorithm | expand |
Dear Paul, dear Dan, Thank you for the patch. Am 18.03.24 um 17:29 schrieb Paul Greenwalt: > From: Dan Nowlin <dan.nowlin@intel.com> > > Previously, the code would assume that only "Modular Signature Segment" > existed. Given a package with both a "Reference Signature Segment" and a > "Modular Signature Segment" the download would not have been successful > because an incorrect sequence of buffers would be sent to the firmware. > > Update download flow to detect a "Reference Signature Segment" and to > only download the buffers in the signature segment in this case, and to > skip downloading any buffers from the configuration segment. Could you please document the test setup (with firmware version) so people can reproduce the error and fix? > Fixes: 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Dan Nowlin <dan.nowlin@intel.com> > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ddp.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c > index 8b7504a9df31..90b9e28ddba9 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ddp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c > @@ -1424,14 +1424,14 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr, > goto exit; > } > > - conf_idx = le32_to_cpu(seg->signed_seg_idx); > - start = le32_to_cpu(seg->signed_buf_start); > count = le32_to_cpu(seg->signed_buf_count); > - > state = ice_download_pkg_sig_seg(hw, seg); > - if (state) > + if (state || !count) > goto exit; > > + conf_idx = le32_to_cpu(seg->signed_seg_idx); > + start = le32_to_cpu(seg->signed_buf_start); > + > state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start, > count); Sorry, just reading the diff, it’s not clear to me why skipping is correct. Isn’t now nothing read, if the Modular and Reference Signature Segment exist? Maybe comments would be nice. Kind regards, Paul
On 3/18/2024 10:44 AM, Paul Menzel wrote: > Dear Paul, dear Dan, > > > Thank you for the patch. > > Am 18.03.24 um 17:29 schrieb Paul Greenwalt: >> From: Dan Nowlin <dan.nowlin@intel.com> >> >> Previously, the code would assume that only "Modular Signature Segment" >> existed. Given a package with both a "Reference Signature Segment" and a >> "Modular Signature Segment" the download would not have been successful >> because an incorrect sequence of buffers would be sent to the firmware. >> >> Update download flow to detect a "Reference Signature Segment" and to >> only download the buffers in the signature segment in this case, and to >> skip downloading any buffers from the configuration segment. > > Could you please document the test setup (with firmware version) so > people can reproduce the error and fix? > The package isn't available yet, so there is no reproducing the error or validating the fix. >> Fixes: 3cbdb0343022 ("ice: Add support for E830 DDP package segment") >> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> >> Signed-off-by: Dan Nowlin <dan.nowlin@intel.com> >> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> >> --- >> drivers/net/ethernet/intel/ice/ice_ddp.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c >> b/drivers/net/ethernet/intel/ice/ice_ddp.c >> index 8b7504a9df31..90b9e28ddba9 100644 >> --- a/drivers/net/ethernet/intel/ice/ice_ddp.c >> +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c >> @@ -1424,14 +1424,14 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, >> struct ice_pkg_hdr *pkg_hdr, >> goto exit; >> } >> - conf_idx = le32_to_cpu(seg->signed_seg_idx); >> - start = le32_to_cpu(seg->signed_buf_start); >> count = le32_to_cpu(seg->signed_buf_count); >> - >> state = ice_download_pkg_sig_seg(hw, seg); >> - if (state) >> + if (state || !count) >> goto exit; >> + conf_idx = le32_to_cpu(seg->signed_seg_idx); >> + start = le32_to_cpu(seg->signed_buf_start); >> + >> state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start, >> count); > > Sorry, just reading the diff, it’s not clear to me why skipping is > correct. Isn’t now nothing read, if the Modular and Reference Signature > Segment exist? Maybe comments would be nice. > The driver can skip the downloading process for this segment since there are no buffers to download (seg->signed_buf_count == 0) . The package will contain one or more additional segments with the actual buffers (seg->signed_buf_count > 0) to be downloaded. I will redo the commit message to try and make this clearer. Thanks, Paul > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c index 8b7504a9df31..90b9e28ddba9 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.c +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c @@ -1424,14 +1424,14 @@ ice_dwnld_sign_and_cfg_segs(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr, goto exit; } - conf_idx = le32_to_cpu(seg->signed_seg_idx); - start = le32_to_cpu(seg->signed_buf_start); count = le32_to_cpu(seg->signed_buf_count); - state = ice_download_pkg_sig_seg(hw, seg); - if (state) + if (state || !count) goto exit; + conf_idx = le32_to_cpu(seg->signed_seg_idx); + start = le32_to_cpu(seg->signed_buf_start); + state = ice_download_pkg_config_seg(hw, pkg_hdr, conf_idx, start, count);