[PATCH net v2 0/4] bnxt_en: Misc. fixes

[PATCH net v2 0/4] bnxt_en: Misc. fixes

[Michael Chan]

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

4 miscellaneous driver fixes covering PM resume, SKB recycling,
wrong return value check, and PTP HWTSTAMP_FILTER_ALL.

v2: Fix SOB tags in patch 1 and 3.

Kalesh AP (1):
  bnxt_en: Fix wrong return value check in bnxt_close_nic()

Michael Chan (1):
  bnxt_en: Fix HWTSTAMP_FILTER_ALL packet timestamp logic

Somnath Kotur (1):
  bnxt_en: Clear resource reservation during resume

Sreekanth Reddy (1):
  bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()

 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 27 ++++++++++++++++++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  8 ++++++-
 2 files changed, 29 insertions(+), 6 deletions(-)

-- 
2.30.1


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

[PATCH net v2 1/4] bnxt_en: Clear resource reservation during resume

[Michael Chan]

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

From: Somnath Kotur <somnath.kotur@broadcom.com>

We are issuing HWRM_FUNC_RESET cmd to reset the device including
all reserved resources, but not clearing the reservations
within the driver struct. As a result, when the driver re-initializes
as part of resume, it believes that there is no need to do any
resource reservation and goes ahead and tries to allocate rings
which will eventually fail beyond a certain number pre-reserved by
the firmware.

Fixes: b4c66425771d ("bnxt_en: refactor bnxt_cancel_reservations()")
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2: Fix typo in SOB tag.
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d0359b569afe..72f2fc983940 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -13940,6 +13940,8 @@ static int bnxt_resume(struct device *device)
 	if (rc)
 		goto resume_exit;
 
+	bnxt_clear_reservations(bp, true);
+
 	if (bnxt_hwrm_func_drv_rgtr(bp, NULL, 0, false)) {
 		rc = -ENODEV;
 		goto resume_exit;
-- 
2.30.1


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

Re: [PATCH net v2 1/4] bnxt_en: Clear resource reservation during resume

[Jakub Kicinski]

On Wed,  6 Dec 2023 16:05:48 -0800 Michael Chan wrote:
> From: Somnath Kotur <somnath.kotur@broadcom.com>
> 
> We are issuing HWRM_FUNC_RESET cmd to reset the device including
> all reserved resources, but not clearing the reservations
> within the driver struct. As a result, when the driver re-initializes
> as part of resume, it believes that there is no need to do any
> resource reservation and goes ahead and tries to allocate rings
> which will eventually fail beyond a certain number pre-reserved by
> the firmware.
> 
> Fixes: b4c66425771d ("bnxt_en: refactor bnxt_cancel_reservations()")

Are you sure this is the right tag? That commit looks like a noop
refactoring. Keep in mind Fixes should point to whether the bug was
first present, not where the patch applies.

[PATCH net v2 2/4] bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()

[Michael Chan]

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

From: Sreekanth Reddy <sreekanth.reddy@broadcom.com>

Receive SKBs can go through the VF-rep path or the normal path.
skb_mark_for_recycle() is only called for the normal path.  Fix it
to do it for both paths to fix possible stalled page pool shutdown
errors.

Fixes: 86b05508f775 ("bnxt_en: Use the unified RX page pool buffers for XDP and non-XDP")
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
Signed-off-by: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 72f2fc983940..b4a5311bdeb5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1748,13 +1748,14 @@ static void bnxt_tpa_agg(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
 static void bnxt_deliver_skb(struct bnxt *bp, struct bnxt_napi *bnapi,
 			     struct sk_buff *skb)
 {
+	skb_mark_for_recycle(skb);
+
 	if (skb->dev != bp->dev) {
 		/* this packet belongs to a vf-rep */
 		bnxt_vf_rep_rx(bp, skb);
 		return;
 	}
 	skb_record_rx_queue(skb, bnapi->index);
-	skb_mark_for_recycle(skb);
 	napi_gro_receive(&bnapi->napi, skb);
 }
 
-- 
2.30.1


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

Re: [PATCH net v2 2/4] bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()

[Jakub Kicinski]

On Wed,  6 Dec 2023 16:05:49 -0800 Michael Chan wrote:
> Receive SKBs can go through the VF-rep path or the normal path.
> skb_mark_for_recycle() is only called for the normal path.  Fix it
> to do it for both paths to fix possible stalled page pool shutdown
> errors.

This patch is probably fine, but since I'm complaining -
IMHO it may be better to mark the skbs right after they
are allocated. Catching all "exit points" seems very error
prone...

Re: [PATCH net v2 2/4] bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()

[Jakub Kicinski]

On Thu, 7 Dec 2023 10:21:44 -0800 Jakub Kicinski wrote:
> On Wed,  6 Dec 2023 16:05:49 -0800 Michael Chan wrote:
> > Receive SKBs can go through the VF-rep path or the normal path.
> > skb_mark_for_recycle() is only called for the normal path.  Fix it
> > to do it for both paths to fix possible stalled page pool shutdown
> > errors.  
> 
> This patch is probably fine, but since I'm complaining -
> IMHO it may be better to mark the skbs right after they
> are allocated. Catching all "exit points" seems very error
> prone...

To be 100% clear - I mean that as a suggestion for a potential 
net-next cleanup.

Re: [PATCH net v2 2/4] bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()

[Andy Gospodarek]

On Thu, Dec 07, 2023 at 10:21:44AM -0800, Jakub Kicinski wrote:
> On Wed,  6 Dec 2023 16:05:49 -0800 Michael Chan wrote:
> > Receive SKBs can go through the VF-rep path or the normal path.
> > skb_mark_for_recycle() is only called for the normal path.  Fix it
> > to do it for both paths to fix possible stalled page pool shutdown
> > errors.
> 
> This patch is probably fine, but since I'm complaining -
> IMHO it may be better to mark the skbs right after they
> are allocated. Catching all "exit points" seems very error
> prone...

That's a good suggestion.  To take it a step further...what about a
third arg (bool) to build_skb that would automatically call
skb_mark_for_recycle if the new 3rd arg was true?  I don't love the
extra arg, but that would avoid duplicating the need to call
skb_mark_for_recycle for all drivers that use the page pool for all
data.

Re: [PATCH net v2 2/4] bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()

[Jakub Kicinski]

On Thu, 7 Dec 2023 13:37:40 -0500 Andy Gospodarek wrote:
> That's a good suggestion.  To take it a step further...what about a
> third arg (bool) to build_skb that would automatically call
> skb_mark_for_recycle if the new 3rd arg was true?  I don't love the
> extra arg, but that would avoid duplicating the need to call
> skb_mark_for_recycle for all drivers that use the page pool for all
> data.

2x yes, would be great to have a function which just sets as recycling
by default; also don't love the extra arg / I could not come up with 
a nice API quickly :(

[PATCH net v2 3/4] bnxt_en: Fix wrong return value check in bnxt_close_nic()

[Michael Chan]

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

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The wait_event_interruptible_timeout() function returns 0
if the timeout elapsed, -ERESTARTSYS if it was interrupted
by a signal, and the remaining jiffies otherwise if the
condition evaluated to true before the timeout elapsed.

Driver should have checked for zero return value instead of
a positive value.

Fixes: c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver.")
Reviewed-by: Vikas Gupta <vikas.gupta@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
v2: Add missing SoB tag.
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b4a5311bdeb5..16b7cf6b01a4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -10753,7 +10753,7 @@ int bnxt_close_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 		rc = wait_event_interruptible_timeout(bp->sriov_cfg_wait,
 						      !bp->sriov_cfg,
 						      BNXT_SRIOV_CFG_WAIT_TMO);
-		if (rc)
+		if (!rc)
 			netdev_warn(bp->dev, "timeout waiting for SRIOV config operation to complete!\n");
 	}
 #endif
-- 
2.30.1


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

Re: [PATCH net v2 3/4] bnxt_en: Fix wrong return value check in bnxt_close_nic()

[Jakub Kicinski]

On Wed,  6 Dec 2023 16:05:50 -0800 Michael Chan wrote:
> The wait_event_interruptible_timeout() function returns 0
> if the timeout elapsed, -ERESTARTSYS if it was interrupted
> by a signal, and the remaining jiffies otherwise if the
> condition evaluated to true before the timeout elapsed.
> 
> Driver should have checked for zero return value instead of
> a positive value.

Not sure how this was not caught earlier, maybe there is a more
complicated story behind it. Otherwise you should handle -ERESTARTSYS
as well.

Re: [PATCH net v2 3/4] bnxt_en: Fix wrong return value check in bnxt_close_nic()

[Michael Chan]

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

On Thu, Dec 7, 2023 at 10:27 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed,  6 Dec 2023 16:05:50 -0800 Michael Chan wrote:
> > The wait_event_interruptible_timeout() function returns 0
> > if the timeout elapsed, -ERESTARTSYS if it was interrupted
> > by a signal, and the remaining jiffies otherwise if the
> > condition evaluated to true before the timeout elapsed.
> >
> > Driver should have checked for zero return value instead of
> > a positive value.
>
> Not sure how this was not caught earlier, maybe there is a more
> complicated story behind it. Otherwise you should handle -ERESTARTSYS
> as well.

The code will always proceed to do the close when
wait_event_interruptible_timeout() returns for any reason.  The check
is just to log a warning message that the wait has timed out and we're
closing anyway.

What I can do is to log another warning if the wait is interrupted by
a signal.  Since we do the close no matter what, the error code should
not be returned to the caller and the function should be changed to
void.  Does that sound reasonable?

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

Re: [PATCH net v2 3/4] bnxt_en: Fix wrong return value check in bnxt_close_nic()

[Jakub Kicinski]

On Thu, 7 Dec 2023 14:24:24 -0800 Michael Chan wrote:
> The code will always proceed to do the close when
> wait_event_interruptible_timeout() returns for any reason.  The check
> is just to log a warning message that the wait has timed out and we're
> closing anyway.
> 
> What I can do is to log another warning if the wait is interrupted by
> a signal.  Since we do the close no matter what, the error code should
> not be returned to the caller and the function should be changed to
> void.  Does that sound reasonable?

Yup!

[PATCH net v2 4/4] bnxt_en: Fix HWTSTAMP_FILTER_ALL packet timestamp logic

[Michael Chan]

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

When the chip is configured to timestamp all receive packets, the
timestamp in the RX completion is only valid if the metadata
present flag is not set for packets received on the wire.  In
addition, internal loopback packets will never have a valid timestamp
and the timestamp field will always be zero.  We must exclude
any 0 value in the timestamp field because there is no way to
determine if it is a loopback packet or not.

Add a new function bnxt_rx_ts_valid() to check for all timestamp
valid conditions.

Fixes: 66ed81dcedc6 ("bnxt_en: Enable packet timestamping for all RX packets")
Reviewed-by: Andy Gospodarek <andrew.gospodarek@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.c | 20 +++++++++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  8 +++++++-
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 16b7cf6b01a4..55ac281a9b06 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1759,6 +1759,21 @@ static void bnxt_deliver_skb(struct bnxt *bp, struct bnxt_napi *bnapi,
 	napi_gro_receive(&bnapi->napi, skb);
 }
 
+static bool bnxt_rx_ts_valid(struct bnxt *bp, u32 flags,
+			     struct rx_cmp_ext *rxcmp1, u32 *cmpl_ts)
+{
+	u32 ts = le32_to_cpu(rxcmp1->rx_cmp_timestamp);
+
+	if (BNXT_PTP_RX_TS_VALID(flags))
+		goto ts_valid;
+	if (!bp->ptp_all_rx_tstamp || !ts || !BNXT_ALL_RX_TS_VALID(flags))
+		return false;
+
+ts_valid:
+	*cmpl_ts = ts;
+	return true;
+}
+
 /* returns the following:
  * 1       - 1 packet successfully received
  * 0       - successful TPA_START, packet not completed yet
@@ -1784,6 +1799,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 	struct sk_buff *skb;
 	struct xdp_buff xdp;
 	u32 flags, misc;
+	u32 cmpl_ts;
 	void *data;
 	int rc = 0;
 
@@ -2006,10 +2022,8 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		}
 	}
 
-	if (unlikely((flags & RX_CMP_FLAGS_ITYPES_MASK) ==
-		     RX_CMP_FLAGS_ITYPE_PTP_W_TS) || bp->ptp_all_rx_tstamp) {
+	if (bnxt_rx_ts_valid(bp, flags, rxcmp1, &cmpl_ts)) {
 		if (bp->flags & BNXT_FLAG_CHIP_P5) {
-			u32 cmpl_ts = le32_to_cpu(rxcmp1->rx_cmp_timestamp);
 			u64 ns, ts;
 
 			if (!bnxt_get_rx_ts_p5(bp, &ts, cmpl_ts)) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index e702dbc3e6b1..5b570058e1c9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -161,7 +161,7 @@ struct rx_cmp {
 	#define RX_CMP_FLAGS_ERROR				(1 << 6)
 	#define RX_CMP_FLAGS_PLACEMENT				(7 << 7)
 	#define RX_CMP_FLAGS_RSS_VALID				(1 << 10)
-	#define RX_CMP_FLAGS_UNUSED				(1 << 11)
+	#define RX_CMP_FLAGS_PKT_METADATA_PRESENT		(1 << 11)
 	 #define RX_CMP_FLAGS_ITYPES_SHIFT			 12
 	 #define RX_CMP_FLAGS_ITYPES_MASK			 0xf000
 	 #define RX_CMP_FLAGS_ITYPE_UNKNOWN			 (0 << 12)
@@ -188,6 +188,12 @@ struct rx_cmp {
 	__le32 rx_cmp_rss_hash;
 };
 
+#define BNXT_PTP_RX_TS_VALID(flags)				\
+	(((flags) & RX_CMP_FLAGS_ITYPES_MASK) == RX_CMP_FLAGS_ITYPE_PTP_W_TS)
+
+#define BNXT_ALL_RX_TS_VALID(flags)				\
+	!((flags) & RX_CMP_FLAGS_PKT_METADATA_PRESENT)
+
 #define RX_CMP_HASH_VALID(rxcmp)				\
 	((rxcmp)->rx_cmp_len_flags_type & cpu_to_le32(RX_CMP_FLAGS_RSS_VALID))
 
-- 
2.30.1


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

Re: [PATCH net v2 0/4] bnxt_en: Misc. fixes

[Vadim Fedorenko]

On 06/12/2023 16:05, Michael Chan wrote:
> 4 miscellaneous driver fixes covering PM resume, SKB recycling,
> wrong return value check, and PTP HWTSTAMP_FILTER_ALL.
> 
> v2: Fix SOB tags in patch 1 and 3.
> 
> Kalesh AP (1):
>    bnxt_en: Fix wrong return value check in bnxt_close_nic()
> 
> Michael Chan (1):
>    bnxt_en: Fix HWTSTAMP_FILTER_ALL packet timestamp logic

Hi Michael!

What about reconfiguration part? I thought this fix was the only
blocker to remove bnxt_close_nic/bnxt_open_nic logic.

> 
> Somnath Kotur (1):
>    bnxt_en: Clear resource reservation during resume
> 
> Sreekanth Reddy (1):
>    bnxt_en: Fix skb recycling logic in bnxt_deliver_skb()
> 
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c | 27 ++++++++++++++++++-----
>   drivers/net/ethernet/broadcom/bnxt/bnxt.h |  8 ++++++-
>   2 files changed, 29 insertions(+), 6 deletions(-)
> 

Re: [PATCH net v2 0/4] bnxt_en: Misc. fixes

[Michael Chan]

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

On Thu, Dec 7, 2023 at 4:04 AM Vadim Fedorenko
<vadim.fedorenko@linux.dev> wrote:
>
> On 06/12/2023 16:05, Michael Chan wrote:
> > 4 miscellaneous driver fixes covering PM resume, SKB recycling,
> > wrong return value check, and PTP HWTSTAMP_FILTER_ALL.
> >
> > v2: Fix SOB tags in patch 1 and 3.
> >
> > Kalesh AP (1):
> >    bnxt_en: Fix wrong return value check in bnxt_close_nic()
> >
> > Michael Chan (1):
> >    bnxt_en: Fix HWTSTAMP_FILTER_ALL packet timestamp logic
>
> Hi Michael!
>
> What about reconfiguration part? I thought this fix was the only
> blocker to remove bnxt_close_nic/bnxt_open_nic logic.

The other PTP patches from Pavan will be submitted to net-next in a
few days because those are technically improvements and not bug fixes.
Thanks.

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