| Message ID | 20231104182908.15389-1-paul.greenwalt@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] ice: fix DDP package download for packages without signature segment | expand |
On 11/4/2023 11:29 AM, Paul Greenwalt wrote: > From: Dan Nowlin <dan.nowlin@intel.com> > > Commit 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > incorrectly removed support for package download for packages without a > signature segment. These packages include the signature buffer inline > in the configurations buffers, and do not in a signature segment. > > Fix package download by providing download support for both packages > with (ice_download_pkg_with_sig_seg()) and without signature segment > (ice_download_pkg_without_sig_seg()). > > Fixes: 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > Reported-by: Fijalkowski, Maciej <maciej.fijalkowski@intel.com> > Signed-off-by: Dan Nowlin <dan.nowlin@intel.com> > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> > --- Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
On Sat, Nov 04, 2023 at 02:29:08PM -0400, Paul Greenwalt wrote: > From: Dan Nowlin <dan.nowlin@intel.com> > > Commit 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > incorrectly removed support for package download for packages without a > signature segment. These packages include the signature buffer inline > in the configurations buffers, and do not in a signature segment. > > Fix package download by providing download support for both packages > with (ice_download_pkg_with_sig_seg()) and without signature segment > (ice_download_pkg_without_sig_seg()). > > Fixes: 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > Reported-by: Fijalkowski, Maciej <maciej.fijalkowski@intel.com> s/Fijalkowski, Maciej/Maciej Fijalkowski > Signed-off-by: Dan Nowlin <dan.nowlin@intel.com> > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> Tested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Thanks a lot for the quick fix! > --- > drivers/net/ethernet/intel/ice/ice_ddp.c | 106 ++++++++++++++++++++++- > 1 file changed, 103 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c > index cfb1580f5850..3f1a11d0252c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ddp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c > @@ -1479,14 +1479,14 @@ ice_post_dwnld_pkg_actions(struct ice_hw *hw) > } > > /** > - * ice_download_pkg > + * ice_download_pkg_with_sig_seg > * @hw: pointer to the hardware structure > * @pkg_hdr: pointer to package header > * > * Handles the download of a complete package. > */ > static enum ice_ddp_state > -ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) > +ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) > { > enum ice_aq_err aq_err = hw->adminq.sq_last_status; > enum ice_ddp_state state = ICE_DDP_PKG_ERR; > @@ -1519,6 +1519,106 @@ ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) > state = ice_post_dwnld_pkg_actions(hw); > > ice_release_global_cfg_lock(hw); > + > + return state; > +} > + > +/** > + * ice_dwnld_cfg_bufs > + * @hw: pointer to the hardware structure > + * @bufs: pointer to an array of buffers > + * @count: the number of buffers in the array > + * > + * Obtains global config lock and downloads the package configuration buffers > + * to the firmware. > + */ > +static enum ice_ddp_state > +ice_dwnld_cfg_bufs(struct ice_hw *hw, struct ice_buf *bufs, u32 count) > +{ > + enum ice_ddp_state state = ICE_DDP_PKG_SUCCESS; > + struct ice_buf_hdr *bh; > + int status; > + > + if (!bufs || !count) > + return ICE_DDP_PKG_ERR; > + > + /* If the first buffer's first section has its metadata bit set > + * then there are no buffers to be downloaded, and the operation is > + * considered a success. > + */ > + bh = (struct ice_buf_hdr *)bufs; > + if (le32_to_cpu(bh->section_entry[0].type) & ICE_METADATA_BUF) > + return ICE_DDP_PKG_SUCCESS; > + > + status = ice_acquire_global_cfg_lock(hw, ICE_RES_WRITE); > + if (status) { > + if (status == -EALREADY) > + return ICE_DDP_PKG_ALREADY_LOADED; > + return ice_map_aq_err_to_ddp_state(hw->adminq.sq_last_status); > + } > + > + state = ice_dwnld_cfg_bufs_no_lock(hw, bufs, 0, count, true); > + if (!state) > + state = ice_post_dwnld_pkg_actions(hw); > + > + ice_release_global_cfg_lock(hw); > + > + return state; > +} > + > +/** > + * ice_download_pkg_without_sig_seg > + * @hw: pointer to the hardware structure > + * @ice_seg: pointer to the segment of the package to be downloaded > + * > + * Handles the download of a complete package without signature segment. > + */ > +static enum ice_ddp_state > +ice_download_pkg_without_sig_seg(struct ice_hw *hw, struct ice_seg *ice_seg) > +{ > + struct ice_buf_table *ice_buf_tbl; > + enum ice_ddp_state state; > + > + ice_debug(hw, ICE_DBG_PKG, "Segment format version: %d.%d.%d.%d\n", > + ice_seg->hdr.seg_format_ver.major, > + ice_seg->hdr.seg_format_ver.minor, > + ice_seg->hdr.seg_format_ver.update, > + ice_seg->hdr.seg_format_ver.draft); > + > + ice_debug(hw, ICE_DBG_PKG, "Seg: type 0x%X, size %d, name %s\n", > + le32_to_cpu(ice_seg->hdr.seg_type), > + le32_to_cpu(ice_seg->hdr.seg_size), ice_seg->hdr.seg_id); > + > + ice_buf_tbl = ice_find_buf_table(ice_seg); > + > + ice_debug(hw, ICE_DBG_PKG, "Seg buf count: %d\n", > + le32_to_cpu(ice_buf_tbl->buf_count)); > + > + state = ice_dwnld_cfg_bufs(hw, ice_buf_tbl->buf_array, > + le32_to_cpu(ice_buf_tbl->buf_count)); > + > + return state; > +} > + > +/** > + * ice_download_pkg > + * @hw: pointer to the hardware structure > + * @pkg_hdr: pointer to package header > + * @ice_seg: pointer to the segment of the package to be downloaded > + * > + * Handles the download of a complete package. > + */ > +static enum ice_ddp_state > +ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr, > + struct ice_seg *ice_seg) > +{ > + enum ice_ddp_state state; > + > + if (hw->pkg_has_signing_seg) > + state = ice_download_pkg_with_sig_seg(hw, pkg_hdr); > + else > + state = ice_download_pkg_without_sig_seg(hw, ice_seg); > + > ice_post_pkg_dwnld_vlan_mode_cfg(hw); > > return state; > @@ -2083,7 +2183,7 @@ enum ice_ddp_state ice_init_pkg(struct ice_hw *hw, u8 *buf, u32 len) > > /* initialize package hints and then download package */ > ice_init_pkg_hints(hw, seg); > - state = ice_download_pkg(hw, pkg); > + state = ice_download_pkg(hw, pkg, seg); > if (state == ICE_DDP_PKG_ALREADY_LOADED) { > ice_debug(hw, ICE_DBG_INIT, > "package previously loaded - no work.\n"); > > base-commit: 016b9332a3346e97a6cacffea0f9dc10e1235a75 > -- > 2.41.0 >
On 04.11.2023 19:29, Paul Greenwalt wrote: > From: Dan Nowlin <dan.nowlin@intel.com> > > Commit 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > incorrectly removed support for package download for packages without a > signature segment. These packages include the signature buffer inline > in the configurations buffers, and do not in a signature segment. > > Fix package download by providing download support for both packages > with (ice_download_pkg_with_sig_seg()) and without signature segment > (ice_download_pkg_without_sig_seg()). > > Fixes: 3cbdb0343022 ("ice: Add support for E830 DDP package segment") > Reported-by: Fijalkowski, Maciej <maciej.fijalkowski@intel.com> > Signed-off-by: Dan Nowlin <dan.nowlin@intel.com> > Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_ddp.c | 106 ++++++++++++++++++++++- > 1 file changed, 103 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c > index cfb1580f5850..3f1a11d0252c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_ddp.c > +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c > @@ -1479,14 +1479,14 @@ ice_post_dwnld_pkg_actions(struct ice_hw *hw) > } > > /** > - * ice_download_pkg > + * ice_download_pkg_with_sig_seg > * @hw: pointer to the hardware structure > * @pkg_hdr: pointer to package header > * > * Handles the download of a complete package. > */ > static enum ice_ddp_state > -ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) > +ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) > { > enum ice_aq_err aq_err = hw->adminq.sq_last_status; > enum ice_ddp_state state = ICE_DDP_PKG_ERR; > @@ -1519,6 +1519,106 @@ ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) > state = ice_post_dwnld_pkg_actions(hw); > > ice_release_global_cfg_lock(hw); > + > + return state; > +} > + > +/** > + * ice_dwnld_cfg_bufs > + * @hw: pointer to the hardware structure > + * @bufs: pointer to an array of buffers > + * @count: the number of buffers in the array > + * > + * Obtains global config lock and downloads the package configuration buffers > + * to the firmware. > + */ > +static enum ice_ddp_state > +ice_dwnld_cfg_bufs(struct ice_hw *hw, struct ice_buf *bufs, u32 count) > +{ > + enum ice_ddp_state state = ICE_DDP_PKG_SUCCESS; > + struct ice_buf_hdr *bh; > + int status; > + > + if (!bufs || !count) > + return ICE_DDP_PKG_ERR; > + > + /* If the first buffer's first section has its metadata bit set > + * then there are no buffers to be downloaded, and the operation is > + * considered a success. > + */ > + bh = (struct ice_buf_hdr *)bufs; > + if (le32_to_cpu(bh->section_entry[0].type) & ICE_METADATA_BUF) > + return ICE_DDP_PKG_SUCCESS; > + > + status = ice_acquire_global_cfg_lock(hw, ICE_RES_WRITE); > + if (status) { > + if (status == -EALREADY) > + return ICE_DDP_PKG_ALREADY_LOADED; > + return ice_map_aq_err_to_ddp_state(hw->adminq.sq_last_status); > + } > + > + state = ice_dwnld_cfg_bufs_no_lock(hw, bufs, 0, count, true); > + if (!state) > + state = ice_post_dwnld_pkg_actions(hw); > + > + ice_release_global_cfg_lock(hw); > + > + return state; > +} > + > +/** > + * ice_download_pkg_without_sig_seg > + * @hw: pointer to the hardware structure > + * @ice_seg: pointer to the segment of the package to be downloaded > + * > + * Handles the download of a complete package without signature segment. > + */ > +static enum ice_ddp_state > +ice_download_pkg_without_sig_seg(struct ice_hw *hw, struct ice_seg *ice_seg) > +{ > + struct ice_buf_table *ice_buf_tbl; > + enum ice_ddp_state state; > + > + ice_debug(hw, ICE_DBG_PKG, "Segment format version: %d.%d.%d.%d\n", > + ice_seg->hdr.seg_format_ver.major, > + ice_seg->hdr.seg_format_ver.minor, > + ice_seg->hdr.seg_format_ver.update, > + ice_seg->hdr.seg_format_ver.draft); > + > + ice_debug(hw, ICE_DBG_PKG, "Seg: type 0x%X, size %d, name %s\n", > + le32_to_cpu(ice_seg->hdr.seg_type), > + le32_to_cpu(ice_seg->hdr.seg_size), ice_seg->hdr.seg_id); > + > + ice_buf_tbl = ice_find_buf_table(ice_seg); > + > + ice_debug(hw, ICE_DBG_PKG, "Seg buf count: %d\n", > + le32_to_cpu(ice_buf_tbl->buf_count)); > + > + state = ice_dwnld_cfg_bufs(hw, ice_buf_tbl->buf_array, > + le32_to_cpu(ice_buf_tbl->buf_count)); > + > + return state; > +} > + > +/** > + * ice_download_pkg > + * @hw: pointer to the hardware structure > + * @pkg_hdr: pointer to package header > + * @ice_seg: pointer to the segment of the package to be downloaded > + * > + * Handles the download of a complete package. > + */ > +static enum ice_ddp_state > +ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr, > + struct ice_seg *ice_seg) > +{ > + enum ice_ddp_state state; > + > + if (hw->pkg_has_signing_seg) > + state = ice_download_pkg_with_sig_seg(hw, pkg_hdr); > + else > + state = ice_download_pkg_without_sig_seg(hw, ice_seg); > + > ice_post_pkg_dwnld_vlan_mode_cfg(hw); > > return state; > @@ -2083,7 +2183,7 @@ enum ice_ddp_state ice_init_pkg(struct ice_hw *hw, u8 *buf, u32 len) > > /* initialize package hints and then download package */ > ice_init_pkg_hints(hw, seg); > - state = ice_download_pkg(hw, pkg); > + state = ice_download_pkg(hw, pkg, seg); > if (state == ICE_DDP_PKG_ALREADY_LOADED) { > ice_debug(hw, ICE_DBG_INIT, > "package previously loaded - no work.\n"); > > base-commit: 016b9332a3346e97a6cacffea0f9dc10e1235a75
diff --git a/drivers/net/ethernet/intel/ice/ice_ddp.c b/drivers/net/ethernet/intel/ice/ice_ddp.c index cfb1580f5850..3f1a11d0252c 100644 --- a/drivers/net/ethernet/intel/ice/ice_ddp.c +++ b/drivers/net/ethernet/intel/ice/ice_ddp.c @@ -1479,14 +1479,14 @@ ice_post_dwnld_pkg_actions(struct ice_hw *hw) } /** - * ice_download_pkg + * ice_download_pkg_with_sig_seg * @hw: pointer to the hardware structure * @pkg_hdr: pointer to package header * * Handles the download of a complete package. */ static enum ice_ddp_state -ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) +ice_download_pkg_with_sig_seg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) { enum ice_aq_err aq_err = hw->adminq.sq_last_status; enum ice_ddp_state state = ICE_DDP_PKG_ERR; @@ -1519,6 +1519,106 @@ ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr) state = ice_post_dwnld_pkg_actions(hw); ice_release_global_cfg_lock(hw); + + return state; +} + +/** + * ice_dwnld_cfg_bufs + * @hw: pointer to the hardware structure + * @bufs: pointer to an array of buffers + * @count: the number of buffers in the array + * + * Obtains global config lock and downloads the package configuration buffers + * to the firmware. + */ +static enum ice_ddp_state +ice_dwnld_cfg_bufs(struct ice_hw *hw, struct ice_buf *bufs, u32 count) +{ + enum ice_ddp_state state = ICE_DDP_PKG_SUCCESS; + struct ice_buf_hdr *bh; + int status; + + if (!bufs || !count) + return ICE_DDP_PKG_ERR; + + /* If the first buffer's first section has its metadata bit set + * then there are no buffers to be downloaded, and the operation is + * considered a success. + */ + bh = (struct ice_buf_hdr *)bufs; + if (le32_to_cpu(bh->section_entry[0].type) & ICE_METADATA_BUF) + return ICE_DDP_PKG_SUCCESS; + + status = ice_acquire_global_cfg_lock(hw, ICE_RES_WRITE); + if (status) { + if (status == -EALREADY) + return ICE_DDP_PKG_ALREADY_LOADED; + return ice_map_aq_err_to_ddp_state(hw->adminq.sq_last_status); + } + + state = ice_dwnld_cfg_bufs_no_lock(hw, bufs, 0, count, true); + if (!state) + state = ice_post_dwnld_pkg_actions(hw); + + ice_release_global_cfg_lock(hw); + + return state; +} + +/** + * ice_download_pkg_without_sig_seg + * @hw: pointer to the hardware structure + * @ice_seg: pointer to the segment of the package to be downloaded + * + * Handles the download of a complete package without signature segment. + */ +static enum ice_ddp_state +ice_download_pkg_without_sig_seg(struct ice_hw *hw, struct ice_seg *ice_seg) +{ + struct ice_buf_table *ice_buf_tbl; + enum ice_ddp_state state; + + ice_debug(hw, ICE_DBG_PKG, "Segment format version: %d.%d.%d.%d\n", + ice_seg->hdr.seg_format_ver.major, + ice_seg->hdr.seg_format_ver.minor, + ice_seg->hdr.seg_format_ver.update, + ice_seg->hdr.seg_format_ver.draft); + + ice_debug(hw, ICE_DBG_PKG, "Seg: type 0x%X, size %d, name %s\n", + le32_to_cpu(ice_seg->hdr.seg_type), + le32_to_cpu(ice_seg->hdr.seg_size), ice_seg->hdr.seg_id); + + ice_buf_tbl = ice_find_buf_table(ice_seg); + + ice_debug(hw, ICE_DBG_PKG, "Seg buf count: %d\n", + le32_to_cpu(ice_buf_tbl->buf_count)); + + state = ice_dwnld_cfg_bufs(hw, ice_buf_tbl->buf_array, + le32_to_cpu(ice_buf_tbl->buf_count)); + + return state; +} + +/** + * ice_download_pkg + * @hw: pointer to the hardware structure + * @pkg_hdr: pointer to package header + * @ice_seg: pointer to the segment of the package to be downloaded + * + * Handles the download of a complete package. + */ +static enum ice_ddp_state +ice_download_pkg(struct ice_hw *hw, struct ice_pkg_hdr *pkg_hdr, + struct ice_seg *ice_seg) +{ + enum ice_ddp_state state; + + if (hw->pkg_has_signing_seg) + state = ice_download_pkg_with_sig_seg(hw, pkg_hdr); + else + state = ice_download_pkg_without_sig_seg(hw, ice_seg); + ice_post_pkg_dwnld_vlan_mode_cfg(hw); return state; @@ -2083,7 +2183,7 @@ enum ice_ddp_state ice_init_pkg(struct ice_hw *hw, u8 *buf, u32 len) /* initialize package hints and then download package */ ice_init_pkg_hints(hw, seg); - state = ice_download_pkg(hw, pkg); + state = ice_download_pkg(hw, pkg, seg); if (state == ICE_DDP_PKG_ALREADY_LOADED) { ice_debug(hw, ICE_DBG_INIT, "package previously loaded - no work.\n");