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 <[email protected]>
Acked-by: Florian Fainelli <[email protected]>
--
Florian
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
>>> 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 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
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 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:
> [email protected].
>
> 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: [email protected].
>
> 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 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:
[email protected].
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
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