| Message ID | 20240608191335.52174-1-michael.chan@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] bnxt_en: Cap the size of HWRM_PORT_PHY_QCFG forwarded response | expand |
On 6/8/24 21:13, 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. The SPEEDS2_SUPPORTED flag needs to be cleared because the > new speeds2 fields are beyond the legacy structure. Also modify > bnxt_hwrm_fwd_resp() to print a warning if the message size exceeds 96 > bytes to make this failure more obvious. > > 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> > --- > v2: Remove Bug and ChangeID from ChangeLog > Add comment to explain the clearing of the SPEEDS2_SUPPORTED flag > --- > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 48 +++++++++++++++++++ > .../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 12 ++++- > 2 files changed, 58 insertions(+), 2 deletions(-) > > 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 { I assume that the first 96 bytes of the current struct hwrm_port_phy_qcfg are the same as here; with that you could wrap those there by struct_group_tagged, giving the very same name as here, but without replicating all the content. > + __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..b28073777ef5 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}; nit: we usually just write `{}` > struct hwrm_port_phy_qcfg_input *phy_qcfg_req; > > phy_qcfg_req = > @@ -1096,6 +1099,11 @@ 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; > + /* New SPEEDS2 fields are beyond the legacy structure, so > + * clear the SPEEDS2_SUPPORTED flag. > + */ > + phy_qcfg_resp.option_flags &= > + ~PORT_PHY_QCAPS_RESP_FLAGS2_SPEEDS2_SUPPORTED; > phy_qcfg_resp.valid = 1; > > if (vf->flags & BNXT_VF_LINK_UP) {
On Mon, Jun 10, 2024 at 4:38 AM Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote: > > On 6/8/24 21:13, 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. The SPEEDS2_SUPPORTED flag needs to be cleared because the > > new speeds2 fields are beyond the legacy structure. Also modify > > bnxt_hwrm_fwd_resp() to print a warning if the message size exceeds 96 > > bytes to make this failure more obvious. > > > > 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> > > --- > > v2: Remove Bug and ChangeID from ChangeLog > > Add comment to explain the clearing of the SPEEDS2_SUPPORTED flag > > --- > > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 48 +++++++++++++++++++ > > .../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 12 ++++- > > 2 files changed, 58 insertions(+), 2 deletions(-) > > > > 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 { > > I assume that the first 96 bytes of the current > struct hwrm_port_phy_qcfg are the same as here; with that you could wrap > those there by struct_group_tagged, giving the very same name as here, > but without replicating all the content. Except for the valid bit at the end of the struct. Let me see if I can define the struct_group thing for 95 bytes and add the valid bit here. Thanks. > > > + __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; This is the valid bit that is different. > > +};
On Mon, Jun 10, 2024 at 7:00 AM Michael Chan <michael.chan@broadcom.com> wrote: > > On Mon, Jun 10, 2024 at 4:38 AM Przemek Kitszel > <przemyslaw.kitszel@intel.com> wrote: > > > > I assume that the first 96 bytes of the current > > struct hwrm_port_phy_qcfg are the same as here; with that you could wrap > > those there by struct_group_tagged, giving the very same name as here, > > but without replicating all the content. > > Except for the valid bit at the end of the struct. Let me see if I > can define the struct_group thing for 95 bytes and add the valid bit > here. Thanks. > The struct_group_tagged() idea works in general. However, the hwrm_port_phy_qcfg_output struct is generated from yaml and it contains a lot of #define within the structure. So it looks like this with struct_group_tagged added: struct hwrm_port_phy_qcfg_output { struct_group_tagged(hwrm_port_phy_qcfg_output_legacy, legacy, __le16 error_code; __le16 req_type; __le16 seq_id; __le16 resp_len; u8 link; #define PORT_PHY_QCFG_RESP_LINK_NO_LINK 0x0UL #define PORT_PHY_QCFG_RESP_LINK_SIGNAL 0x1UL #define PORT_PHY_QCFG_RESP_LINK_LINK 0x2UL .... ); .... }; The #define within the struct_group generates a lot of warnings with make C=1: CC [M] drivers/net/ethernet/broadcom/bnxt/bnxt.o CHECK drivers/net/ethernet/broadcom/bnxt/bnxt.c drivers/net/ethernet/broadcom/bnxt/bnxt.c: note: in included file: drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h:4211:9: warning: directive in macro's argument list Because it's a generated file, it's hard to make the drastic change to move all the #define macros. Maybe in the future when we restructure these generated structs, we can do it in a better way.
On 6/11/24 01:52, Michael Chan wrote: > On Mon, Jun 10, 2024 at 7:00 AM Michael Chan <michael.chan@broadcom.com> wrote: >> >> On Mon, Jun 10, 2024 at 4:38 AM Przemek Kitszel >> <przemyslaw.kitszel@intel.com> wrote: >>> >>> I assume that the first 96 bytes of the current >>> struct hwrm_port_phy_qcfg are the same as here; with that you could wrap >>> those there by struct_group_tagged, giving the very same name as here, >>> but without replicating all the content. >> >> Except for the valid bit at the end of the struct. Let me see if I >> can define the struct_group thing for 95 bytes and add the valid bit >> here. Thanks. >> > > The struct_group_tagged() idea works in general. However, the > hwrm_port_phy_qcfg_output struct is generated from yaml and it > contains a lot of #define within the structure. So it looks like this > with struct_group_tagged added: > > struct hwrm_port_phy_qcfg_output { > struct_group_tagged(hwrm_port_phy_qcfg_output_legacy, legacy, > __le16 error_code; > __le16 req_type; > __le16 seq_id; > __le16 resp_len; > u8 link; > #define PORT_PHY_QCFG_RESP_LINK_NO_LINK 0x0UL > #define PORT_PHY_QCFG_RESP_LINK_SIGNAL 0x1UL > #define PORT_PHY_QCFG_RESP_LINK_LINK 0x2UL > .... > ); > .... > }; > > The #define within the struct_group generates a lot of warnings with make C=1: > > CC [M] drivers/net/ethernet/broadcom/bnxt/bnxt.o > CHECK drivers/net/ethernet/broadcom/bnxt/bnxt.c > drivers/net/ethernet/broadcom/bnxt/bnxt.c: note: in included file: > drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h:4211:9: warning: > directive in macro's argument list > > Because it's a generated file, it's hard to make the drastic change to is this generated as part of upstream build process or just manually? > move all the #define macros. Maybe in the future when we restructure > these generated structs, we can do it in a better way. You could also just split struct into two and combine them (packed) into hwrm_port_phy_qcfg_output and add compat one as combination of hwrm_port_phy_qcfg_output + valid bit If you don't want to do so, please at least document in code that only the first 95 bytes match and the last one is different
On Tue, Jun 11, 2024 at 4:10 AM Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote: > > On 6/11/24 01:52, Michael Chan wrote: > > The #define within the struct_group generates a lot of warnings with make C=1: > > > > CC [M] drivers/net/ethernet/broadcom/bnxt/bnxt.o > > CHECK drivers/net/ethernet/broadcom/bnxt/bnxt.c > > drivers/net/ethernet/broadcom/bnxt/bnxt.c: note: in included file: > > drivers/net/ethernet/broadcom/bnxt/bnxt_hsi.h:4211:9: warning: > > directive in macro's argument list > > > > Because it's a generated file, it's hard to make the drastic change to > > is this generated as part of upstream build process or just manually? This is generated internally for our firmware interface structures and definitions. > > > move all the #define macros. Maybe in the future when we restructure > > these generated structs, we can do it in a better way. > > You could also just split struct into two and combine them (packed) > into hwrm_port_phy_qcfg_output and add compat one as combination of > hwrm_port_phy_qcfg_output + valid bit What I tried to do was to use struct_group_tagged to wrap the first 95 bytes of the structure. No changes are required for the existing code using the structure and that's the good thing. The new compat. structure uses the struct_group plus 1 more byte at the end for the valid bit. This works fine except for the warning due to the embedded #define within the struct_group. > > If you don't want to do so, please at least document in code that only > the first 95 bytes match and the last one is different Will do. 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..b28073777ef5 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,11 @@ 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; + /* New SPEEDS2 fields are beyond the legacy structure, so + * clear the SPEEDS2_SUPPORTED flag. + */ + 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. The SPEEDS2_SUPPORTED flag needs to be cleared because the new speeds2 fields are beyond the legacy structure. Also modify bnxt_hwrm_fwd_resp() to print a warning if the message size exceeds 96 bytes to make this failure more obvious. 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> --- v2: Remove Bug and ChangeID from ChangeLog Add comment to explain the clearing of the SPEEDS2_SUPPORTED flag --- drivers/net/ethernet/broadcom/bnxt/bnxt.h | 48 +++++++++++++++++++ .../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 12 ++++- 2 files changed, 58 insertions(+), 2 deletions(-)