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

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

[Justin Chen]

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

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

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);
-- 
2.34.1


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

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

[Florian Fainelli]

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>
-- 
Florian

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

[Markus Elfring]

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

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

[Justin Chen]

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



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

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

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

[Markus Elfring]

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

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

[Simon Horman]

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.

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

[Florian Fainelli]

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

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.
-- 
Florian


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

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

[Justin Chen]

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



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


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

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

[Jakub Kicinski]

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.

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

[Simon Horman]

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.

Re: net: bcmasp: Patch review challenges

[Markus Elfring]

> 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