| Message ID | 20240606175107.53130-1-michael.chan@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] bnxt_en: Cap the size of HWRM_PORT_PHY_QCFG forwarded response | expand |
On Thu, Jun 06, 2024 at 10:51:07AM -0700, Michael Chan wrote: > Firmware interface 1.10.2.118 has increased the size of > HWRM_PORT_PHY_QCFG response beyond the maximum size that can be > forwarded. When the VF's link state is not the default auto state, > the PF will need to forward the response back to the VF to indicate > the forced state. This regression may cause the VF to fail to > initialize. > > Fix it by capping the HWRM_PORT_PHY_QCFG response to the maximum > 96 bytes. Also modify bnxt_hwrm_fwd_resp() to print a warning if the > message size exceeds 96 bytes to make this failure more obvious. > > Bug: DCSG01725771 > Change-Id: I4cd5e06a7625f9d06e779e4acd9603d355883e7c Hi Michael, Does the above relate to publicly available information? If so, a link is probably in order. If not, let's drop it. > Fixes: 84a911db8305 ("bnxt_en: Update firmware interface to 1.10.2.118") > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > Signed-off-by: Michael Chan <michael.chan@broadcom.com> ... > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c ... > @@ -1096,6 +1099,8 @@ static int bnxt_vf_set_link(struct bnxt *bp, struct bnxt_vf_info *vf) > mutex_unlock(&bp->link_lock); > phy_qcfg_resp.resp_len = cpu_to_le16(sizeof(phy_qcfg_resp)); > phy_qcfg_resp.seq_id = phy_qcfg_req->seq_id; > + phy_qcfg_resp.option_flags &= > + ~PORT_PHY_QCAPS_RESP_FLAGS2_SPEEDS2_SUPPORTED; I may be missing something obvious, but it is not clear to me how this change relates to the rest of the patch. > phy_qcfg_resp.valid = 1; > > if (vf->flags & BNXT_VF_LINK_UP) { > -- > 2.30.1 >
On Fri, Jun 7, 2024 at 6:50 AM Simon Horman <horms@kernel.org> wrote: > > On Thu, Jun 06, 2024 at 10:51:07AM -0700, Michael Chan wrote: > > Firmware interface 1.10.2.118 has increased the size of > > HWRM_PORT_PHY_QCFG response beyond the maximum size that can be > > forwarded. When the VF's link state is not the default auto state, > > the PF will need to forward the response back to the VF to indicate > > the forced state. This regression may cause the VF to fail to > > initialize. > > > > Fix it by capping the HWRM_PORT_PHY_QCFG response to the maximum > > 96 bytes. Also modify bnxt_hwrm_fwd_resp() to print a warning if the > > message size exceeds 96 bytes to make this failure more obvious. > > > > Bug: DCSG01725771 > > Change-Id: I4cd5e06a7625f9d06e779e4acd9603d355883e7c > > Hi Michael, > > Does the above relate to publicly available information? > If so, a link is probably in order. If not, let's drop it. Sorry, my mistake. I will drop it in v2. > > > Fixes: 84a911db8305 ("bnxt_en: Update firmware interface to 1.10.2.118") > > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> > > Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com> > > ... > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c > > ... > > > @@ -1096,6 +1099,8 @@ static int bnxt_vf_set_link(struct bnxt *bp, struct bnxt_vf_info *vf) > > mutex_unlock(&bp->link_lock); > > phy_qcfg_resp.resp_len = cpu_to_le16(sizeof(phy_qcfg_resp)); > > phy_qcfg_resp.seq_id = phy_qcfg_req->seq_id; > > + phy_qcfg_resp.option_flags &= > > + ~PORT_PHY_QCAPS_RESP_FLAGS2_SPEEDS2_SUPPORTED; > > I may be missing something obvious, but it is not clear to me > how this change relates to the rest of the patch. The SPEEDS2 fields were added to the structure and made the structure bigger. For example, support_speeds2 was added beyond the 96 bytes. So here, we're clearing this SPEEDS2_SUPPORTED flag in the legacy structure so that the VF driver will not interpret the new SPEEDS2 fields beyond the legacy structure. I will add a comment to make it more clear. Thanks for the review.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h index 656ab81c0272..94d242aca8d5 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h @@ -1434,6 +1434,54 @@ struct bnxt_l2_filter { atomic_t refcnt; }; +/* hwrm_port_phy_qcfg_output (size:96 bytes) */ +struct hwrm_port_phy_qcfg_output_compat { + __le16 error_code; + __le16 req_type; + __le16 seq_id; + __le16 resp_len; + u8 link; + u8 active_fec_signal_mode; + __le16 link_speed; + u8 duplex_cfg; + u8 pause; + __le16 support_speeds; + __le16 force_link_speed; + u8 auto_mode; + u8 auto_pause; + __le16 auto_link_speed; + __le16 auto_link_speed_mask; + u8 wirespeed; + u8 lpbk; + u8 force_pause; + u8 module_status; + __le32 preemphasis; + u8 phy_maj; + u8 phy_min; + u8 phy_bld; + u8 phy_type; + u8 media_type; + u8 xcvr_pkg_type; + u8 eee_config_phy_addr; + u8 parallel_detect; + __le16 link_partner_adv_speeds; + u8 link_partner_adv_auto_mode; + u8 link_partner_adv_pause; + __le16 adv_eee_link_speed_mask; + __le16 link_partner_adv_eee_link_speed_mask; + __le32 xcvr_identifier_type_tx_lpi_timer; + __le16 fec_cfg; + u8 duplex_state; + u8 option_flags; + char phy_vendor_name[16]; + char phy_vendor_partnumber[16]; + __le16 support_pam4_speeds; + __le16 force_pam4_link_speed; + __le16 auto_pam4_link_speed_mask; + u8 link_partner_pam4_adv_speeds; + u8 valid; +}; + struct bnxt_link_info { u8 phy_type; u8 media_type; diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c index 175192ebaa77..377e66d5a23e 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c @@ -950,8 +950,11 @@ static int bnxt_hwrm_fwd_resp(struct bnxt *bp, struct bnxt_vf_info *vf, struct hwrm_fwd_resp_input *req; int rc; - if (BNXT_FWD_RESP_SIZE_ERR(msg_size)) + if (BNXT_FWD_RESP_SIZE_ERR(msg_size)) { + netdev_warn_once(bp->dev, "HWRM fwd response too big (%d bytes)\n", + msg_size); return -EINVAL; + } rc = hwrm_req_init(bp, req, HWRM_FWD_RESP); if (!rc) { @@ -1085,7 +1088,7 @@ static int bnxt_vf_set_link(struct bnxt *bp, struct bnxt_vf_info *vf) rc = bnxt_hwrm_exec_fwd_resp( bp, vf, sizeof(struct hwrm_port_phy_qcfg_input)); } else { - struct hwrm_port_phy_qcfg_output phy_qcfg_resp = {0}; + struct hwrm_port_phy_qcfg_output_compat phy_qcfg_resp = {0}; struct hwrm_port_phy_qcfg_input *phy_qcfg_req; phy_qcfg_req = @@ -1096,6 +1099,8 @@ static int bnxt_vf_set_link(struct bnxt *bp, struct bnxt_vf_info *vf) mutex_unlock(&bp->link_lock); phy_qcfg_resp.resp_len = cpu_to_le16(sizeof(phy_qcfg_resp)); phy_qcfg_resp.seq_id = phy_qcfg_req->seq_id; + phy_qcfg_resp.option_flags &= + ~PORT_PHY_QCAPS_RESP_FLAGS2_SPEEDS2_SUPPORTED; phy_qcfg_resp.valid = 1; if (vf->flags & BNXT_VF_LINK_UP) {
Firmware interface 1.10.2.118 has increased the size of HWRM_PORT_PHY_QCFG response beyond the maximum size that can be forwarded. When the VF's link state is not the default auto state, the PF will need to forward the response back to the VF to indicate the forced state. This regression may cause the VF to fail to initialize. Fix it by capping the HWRM_PORT_PHY_QCFG response to the maximum 96 bytes. Also modify bnxt_hwrm_fwd_resp() to print a warning if the message size exceeds 96 bytes to make this failure more obvious. Bug: DCSG01725771 Change-Id: I4cd5e06a7625f9d06e779e4acd9603d355883e7c Fixes: 84a911db8305 ("bnxt_en: Update firmware interface to 1.10.2.118") Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com> Signed-off-by: Michael Chan <michael.chan@broadcom.com> --- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 48 +++++++++++++++++++ .../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 9 +++- 2 files changed, 55 insertions(+), 2 deletions(-)