[PATCH net] bnxt_en: Cap the size of HWRM_PORT_PHY_QCFG forwarded response

[PATCH net] bnxt_en: Cap the size of HWRM_PORT_PHY_QCFG forwarded response

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3951 bytes --]

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(-)

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) {
-- 
2.30.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net] bnxt_en: Cap the size of HWRM_PORT_PHY_QCFG forwarded response

[Simon Horman]

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
> 

Re: [PATCH net] bnxt_en: Cap the size of HWRM_PORT_PHY_QCFG forwarded response

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]