diff mbox series

[net] net: bcmasp: fix memory leak when bringing down if

Message ID 20240412181631.3488324-1-justin.chen@broadcom.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net] net: bcmasp: fix memory leak when bringing down if | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 926 this patch: 926
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 937 this patch: 937
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 937 this patch: 937
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 46 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-15--15-00 (tests: 961)

Commit Message

Justin Chen April 12, 2024, 6:16 p.m. UTC
When bringing down the TX rings we flush the rings but forget to
reclaimed the flushed packets. This lead to a memory leak since we
do not free the dma mapped buffers. This also leads to tx control
block corruption when bringing down the interface for power
management.

Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 21 ++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Florian Fainelli April 12, 2024, 9:09 p.m. UTC | #1
On 4/12/24 11:16, Justin Chen wrote:
> When bringing down the TX rings we flush the rings but forget to
> reclaimed the flushed packets. This lead to a memory leak since we
> do not free the dma mapped buffers. This also leads to tx control
> block corruption when bringing down the interface for power
> management.
> 
> Fixes: 490cb412007d ("net: bcmasp: Add support for ASP2.0 Ethernet controller")
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
Markus Elfring April 14, 2024, 11:23 a.m. UTC | #2
Can it be nicer to use the word “interface” instead of “if”
in the summary phrase?


> When bringing down the TX rings we flush the rings but forget to
> reclaimed the flushed packets. This lead to a memory leak since we
> do not free the dma mapped buffers. …

I find this change description improvable.

* How do you think about to avoid typos?

* Would another imperative wording be more desirable?

Regards,
Markus
Justin Chen April 15, 2024, 5:36 p.m. UTC | #3
On 4/14/24 4:23 AM, Markus Elfring wrote:
> Can it be nicer to use the word “interface” instead of “if”
> in the summary phrase?
> 

"if" for interface is a term used in the network stack, but I can see 
why it is confusing. Can submit a v2.

> 
>> When bringing down the TX rings we flush the rings but forget to
>> reclaimed the flushed packets. This lead to a memory leak since we
>> do not free the dma mapped buffers. …
> 
> I find this change description improvable.
> 
> * How do you think about to avoid typos?
> 
> * Would another imperative wording be more desirable?
>

The change description makes sense to me. Can you be a bit more specific 
as to what isn't clear here?

Thanks,
Justin

> Regards,
> Markus
Markus Elfring April 15, 2024, 7:46 p.m. UTC | #4
>>> When bringing down the TX rings we flush the rings but forget to
>>> reclaimed the flushed packets. This lead to a memory leak since we
>>> do not free the dma mapped buffers. …
>>
>> I find this change description improvable.
>>
>> * How do you think about to avoid typos?
>>
>> * Would another imperative wording be more desirable?
>
> The change description makes sense to me. Can you be a bit more specific as to what isn't clear here?

Spelling suggestions:
+ … forget to reclaim …
+ … This leads to …


Advices from a corresponding known information source:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc4#n94

Regards,
Markus
Simon Horman April 17, 2024, 4:19 p.m. UTC | #5
On Mon, Apr 15, 2024 at 09:46:44PM +0200, Markus Elfring wrote:
> >>> When bringing down the TX rings we flush the rings but forget to
> >>> reclaimed the flushed packets. This lead to a memory leak since we
> >>> do not free the dma mapped buffers. …
> >>
> >> I find this change description improvable.
> >>
> >> * How do you think about to avoid typos?
> >>
> >> * Would another imperative wording be more desirable?
> >
> > The change description makes sense to me. Can you be a bit more specific as to what isn't clear here?
> 
> Spelling suggestions:
> + … forget to reclaim …
> + … This leads to …

Markus, let's cut to the chase.

What portion of your responses of this thread were produced
by an LLM or similar technology?

The suggestions in your second email are correct.
But, ironically, your first response appears to be grammatically incorrect.

Specifically:

* What does "improvable" mean in this context?
* "How do you think about to avoid typos?"
  is, in my opinion, grammatically incorrect.
  And, FWIW, I see no typos.
* "Would another imperative wording be more desirable?"
  is, in my opinion, also grammatically incorrect.

And yet your comment is ostensibly about grammar.
I'm sorry, but this strikes me as absurd.
Florian Fainelli April 17, 2024, 4:52 p.m. UTC | #6
On 4/17/24 09:19, Simon Horman wrote:
> On Mon, Apr 15, 2024 at 09:46:44PM +0200, Markus Elfring wrote:
>>>>> When bringing down the TX rings we flush the rings but forget to
>>>>> reclaimed the flushed packets. This lead to a memory leak since we
>>>>> do not free the dma mapped buffers. …
>>>>
>>>> I find this change description improvable.
>>>>
>>>> * How do you think about to avoid typos?
>>>>
>>>> * Would another imperative wording be more desirable?
>>>
>>> The change description makes sense to me. Can you be a bit more specific as to what isn't clear here?
>>
>> Spelling suggestions:
>> + … forget to reclaim …
>> + … This leads to …
> 
> Markus, let's cut to the chase.
> 
> What portion of your responses of this thread were produced
> by an LLM or similar technology?
> 
> The suggestions in your second email are correct.
> But, ironically, your first response appears to be grammatically incorrect.
> 
> Specifically:
> 
> * What does "improvable" mean in this context?

I read it as "improbable", but this patch came out of an actual bug 
report we had internally and code inspection revealed the leaks being 
plugged by this patch.

> * "How do you think about to avoid typos?"
>    is, in my opinion, grammatically incorrect.
>    And, FWIW, I see no typos.

There was one, "This lead to a memory leak" -> "This leads to a memory leak"

> * "Would another imperative wording be more desirable?"
>    is, in my opinion, also grammatically incorrect.
> 
> And yet your comment is ostensibly about grammar.
> I'm sorry, but this strikes me as absurd.

Yeah, I share that too, if you are to nitpick on every single word 
someone wrote in a commit message, your responses better be squeaky 
clean such that Shakespeare himself would be proud of you.

There is a track record of what people might consider bike shedding, 
others might consider useless, and others might find uber pedantic 
comments from Markus done under his other email address: 
elfring@users.sourceforge.net.

Me personally, I read his comments and apply my own judgement as to 
whether they justify spinning a new patch just to address the feedback 
given. He has not landed on my ignore filter, but of course that can 
change at a moments notice.
Justin Chen April 17, 2024, 6:48 p.m. UTC | #7
On 4/17/24 9:52 AM, Florian Fainelli wrote:
> On 4/17/24 09:19, Simon Horman wrote:
>> On Mon, Apr 15, 2024 at 09:46:44PM +0200, Markus Elfring wrote:
>>>>>> When bringing down the TX rings we flush the rings but forget to
>>>>>> reclaimed the flushed packets. This lead to a memory leak since we
>>>>>> do not free the dma mapped buffers. …
>>>>>
>>>>> I find this change description improvable.
>>>>>
>>>>> * How do you think about to avoid typos?
>>>>>
>>>>> * Would another imperative wording be more desirable?
>>>>
>>>> The change description makes sense to me. Can you be a bit more 
>>>> specific as to what isn't clear here?
>>>
>>> Spelling suggestions:
>>> + … forget to reclaim …
>>> + … This leads to …
>>
>> Markus, let's cut to the chase.
>>
>> What portion of your responses of this thread were produced
>> by an LLM or similar technology?
>>
>> The suggestions in your second email are correct.
>> But, ironically, your first response appears to be grammatically 
>> incorrect.
>>
>> Specifically:
>>
>> * What does "improvable" mean in this context?
> 
> I read it as "improbable", but this patch came out of an actual bug 
> report we had internally and code inspection revealed the leaks being 
> plugged by this patch.
> 
>> * "How do you think about to avoid typos?"
>>    is, in my opinion, grammatically incorrect.
>>    And, FWIW, I see no typos.
> 
> There was one, "This lead to a memory leak" -> "This leads to a memory 
> leak"
> 
>> * "Would another imperative wording be more desirable?"
>>    is, in my opinion, also grammatically incorrect.
>>
>> And yet your comment is ostensibly about grammar.
>> I'm sorry, but this strikes me as absurd.
> 
> Yeah, I share that too, if you are to nitpick on every single word 
> someone wrote in a commit message, your responses better be squeaky 
> clean such that Shakespeare himself would be proud of you.
> 
> There is a track record of what people might consider bike shedding, 
> others might consider useless, and others might find uber pedantic 
> comments from Markus done under his other email address: 
> elfring@users.sourceforge.net.
> 
> Me personally, I read his comments and apply my own judgement as to 
> whether they justify spinning a new patch just to address the feedback 
> given. He has not landed on my ignore filter, but of course that can 
> change at a moments notice.

I try my best to address feedback. After a bit of thought, I feel the 
feedback given was not out of good faith. I would like to keep the patch 
as-is unless someone else has feedback. That is if the maintainers are 
ok with that.

Thanks,
Justin
Simon Horman April 17, 2024, 8:34 p.m. UTC | #8
On Wed, Apr 17, 2024 at 09:52:47AM -0700, Florian Fainelli wrote:
> On 4/17/24 09:19, Simon Horman wrote:
> > On Mon, Apr 15, 2024 at 09:46:44PM +0200, Markus Elfring wrote:
> > > > > > When bringing down the TX rings we flush the rings but forget to
> > > > > > reclaimed the flushed packets. This lead to a memory leak since we
> > > > > > do not free the dma mapped buffers. …
> > > > > 
> > > > > I find this change description improvable.
> > > > > 
> > > > > * How do you think about to avoid typos?
> > > > > 
> > > > > * Would another imperative wording be more desirable?
> > > > 
> > > > The change description makes sense to me. Can you be a bit more specific as to what isn't clear here?
> > > 
> > > Spelling suggestions:
> > > + … forget to reclaim …
> > > + … This leads to …
> > 
> > Markus, let's cut to the chase.
> > 
> > What portion of your responses of this thread were produced
> > by an LLM or similar technology?
> > 
> > The suggestions in your second email are correct.
> > But, ironically, your first response appears to be grammatically incorrect.
> > 
> > Specifically:
> > 
> > * What does "improvable" mean in this context?
> 
> I read it as "improbable", but this patch came out of an actual bug report
> we had internally and code inspection revealed the leaks being plugged by
> this patch.
> 
> > * "How do you think about to avoid typos?"
> >    is, in my opinion, grammatically incorrect.
> >    And, FWIW, I see no typos.
> 
> There was one, "This lead to a memory leak" -> "This leads to a memory leak"
> 
> > * "Would another imperative wording be more desirable?"
> >    is, in my opinion, also grammatically incorrect.
> > 
> > And yet your comment is ostensibly about grammar.
> > I'm sorry, but this strikes me as absurd.
> 
> Yeah, I share that too, if you are to nitpick on every single word someone
> wrote in a commit message, your responses better be squeaky clean such that
> Shakespeare himself would be proud of you.
> 
> There is a track record of what people might consider bike shedding, others
> might consider useless, and others might find uber pedantic comments from
> Markus done under his other email address: elfring@users.sourceforge.net.
> 
> Me personally, I read his comments and apply my own judgement as to whether
> they justify spinning a new patch just to address the feedback given. He has
> not landed on my ignore filter, but of course that can change at a moments
> notice.

Thanks Florian,

On reflection, my previous email was inappropriate.
I do have reservations about the review provided by Markus,
but should not reacted as I did. I apologise to every for that.
Jakub Kicinski April 18, 2024, 12:56 a.m. UTC | #9
On Wed, 17 Apr 2024 11:48:33 -0700 Justin Chen wrote:
> I try my best to address feedback. After a bit of thought, I feel the 
> feedback given was not out of good faith. I would like to keep the patch 
> as-is unless someone else has feedback. That is if the maintainers are 
> ok with that.

TBH the "if" in the subject gives me pause too, please respin.
Markus Elfring April 18, 2024, 8:02 a.m. UTC | #10
> On reflection, my previous email was inappropriate.

I find such a response interesting.


> I do have reservations about the review provided by Markus,

Which factors are influencing this view?


> but should not reacted as I did. I apologise to every for that.

Will clarification opportunities become more constructive accordingly?

Regards,
Markus
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index 72ea97c5d5d4..82768b0e9026 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -436,10 +436,8 @@  static void umac_init(struct bcmasp_intf *intf)
 	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
 }
 
-static int bcmasp_tx_poll(struct napi_struct *napi, int budget)
+static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)
 {
-	struct bcmasp_intf *intf =
-		container_of(napi, struct bcmasp_intf, tx_napi);
 	struct bcmasp_intf_stats64 *stats = &intf->stats64;
 	struct device *kdev = &intf->parent->pdev->dev;
 	unsigned long read, released = 0;
@@ -482,10 +480,16 @@  static int bcmasp_tx_poll(struct napi_struct *napi, int budget)
 							DESC_RING_COUNT);
 	}
 
-	/* Ensure all descriptors have been written to DRAM for the hardware
-	 * to see updated contents.
-	 */
-	wmb();
+	return released;
+}
+
+static int bcmasp_tx_poll(struct napi_struct *napi, int budget)
+{
+	struct bcmasp_intf *intf =
+		container_of(napi, struct bcmasp_intf, tx_napi);
+	int released = 0;
+
+	released = bcmasp_tx_reclaim(intf);
 
 	napi_complete(&intf->tx_napi);
 
@@ -797,6 +801,7 @@  static void bcmasp_init_tx(struct bcmasp_intf *intf)
 	intf->tx_spb_dma_read = intf->tx_spb_dma_addr;
 	intf->tx_spb_index = 0;
 	intf->tx_spb_clean_index = 0;
+	memset(intf->tx_cbs, 0, sizeof(struct bcmasp_tx_cb) * DESC_RING_COUNT);
 
 	/* Make sure channels are disabled */
 	tx_spb_ctrl_wl(intf, 0x0, TX_SPB_CTRL_ENABLE);
@@ -885,6 +890,8 @@  static void bcmasp_netif_deinit(struct net_device *dev)
 	} while (timeout-- > 0);
 	tx_spb_dma_wl(intf, 0x0, TX_SPB_DMA_FIFO_CTRL);
 
+	bcmasp_tx_reclaim(intf);
+
 	umac_enable_set(intf, UMC_CMD_TX_EN, 0);
 
 	phy_stop(dev->phydev);