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.now...@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.kits...@intel.com>
>> Signed-off-by: Dan Nowlin <dan.now...@intel.com>
>> Signed-off-by: Paul Greenwalt <paul.greenw...@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
