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