[PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Toke Høiland-Jørgensen]

This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.

Turns out the channelmap variable is not actually read-only, it's modified
through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
so making it read-only causes page faults when that code is hit.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and channelmap static const")
Cc: stable@vger.kernel.org
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
 drivers/net/wireless/ath/ath9k/mci.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/mci.c b/drivers/net/wireless/ath/ath9k/mci.c
index 3363fc4e8966..a0845002d6fe 100644
--- a/drivers/net/wireless/ath/ath9k/mci.c
+++ b/drivers/net/wireless/ath/ath9k/mci.c
@@ -646,9 +646,7 @@ void ath9k_mci_update_wlan_channels(struct ath_softc *sc, bool allow_all)
 	struct ath_hw *ah = sc->sc_ah;
 	struct ath9k_hw_mci *mci = &ah->btcoex_hw.mci;
 	struct ath9k_channel *chan = ah->curchan;
-	static const u32 channelmap[] = {
-		0x00000000, 0xffff0000, 0xffffffff, 0x7fffffff
-	};
+	u32 channelmap[] = {0x00000000, 0xffff0000, 0xffffffff, 0x7fffffff};
 	int i;
 	s16 chan_start, chan_end;
 	u16 wlan_chan;
-- 
2.40.0

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Kalle Valo]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
>
> Turns out the channelmap variable is not actually read-only, it's modified
> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
> so making it read-only causes page faults when that code is hit.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and channelmap static const")
> Cc: stable@vger.kernel.org
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

I guess the casting in MCI_GPM_CLR_CHANNEL_BIT() hide this and made it
impossible for the compiler to detect it? A perfect example why I hate
casting :)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
>>
>> Turns out the channelmap variable is not actually read-only, it's modified
>> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
>> so making it read-only causes page faults when that code is hit.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
>> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and channelmap static const")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> I guess the casting in MCI_GPM_CLR_CHANNEL_BIT() hide this and made it
> impossible for the compiler to detect it? A perfect example why I hate
> casting :)

Yup, exactly. I was also assuming the compiler would catch it, but yay, C! :/

Anyway, cf the bugzilla this was a pretty bad regression for 6.2, so
would be good to move this along reasonably quickly (although I guess we
just missed the -net PR for rc7)...

-Toke

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Kalle Valo]

(dropping stable from cc)

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Kalle Valo <kvalo@kernel.org> writes:
>
>> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>>
>>> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
>>>
>>> Turns out the channelmap variable is not actually read-only, it's modified
>>> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
>>> so making it read-only causes page faults when that code is hit.
>>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
>>> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and
>>> channelmap static const")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>
>> I guess the casting in MCI_GPM_CLR_CHANNEL_BIT() hide this and made it
>> impossible for the compiler to detect it? A perfect example why I hate
>> casting :)
>
> Yup, exactly. I was also assuming the compiler would catch it, but yay, C! :/

We have so many static checkers that I wonder if those would be able to
catch these kind of buggy casts? We had a similar bug in rtw89 something
like a year ago.

> Anyway, cf the bugzilla this was a pretty bad regression for 6.2, so
> would be good to move this along reasonably quickly (although I guess we
> just missed the -net PR for rc7)...

I'm not planning to send anymore stuff to v6.3 so my plan is to take
this to -next. The merge window is very close anyway so this shouldn't
cause too much delay.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> (dropping stable from cc)
>
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>> Kalle Valo <kvalo@kernel.org> writes:
>>
>>> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>>>
>>>> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
>>>>
>>>> Turns out the channelmap variable is not actually read-only, it's modified
>>>> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
>>>> so making it read-only causes page faults when that code is hit.
>>>>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
>>>> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and
>>>> channelmap static const")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>>
>>> I guess the casting in MCI_GPM_CLR_CHANNEL_BIT() hide this and made it
>>> impossible for the compiler to detect it? A perfect example why I hate
>>> casting :)
>>
>> Yup, exactly. I was also assuming the compiler would catch it, but yay, C! :/
>
> We have so many static checkers that I wonder if those would be able to
> catch these kind of buggy casts? We had a similar bug in rtw89 something
> like a year ago.

No idea. Would be nice, yeah... :)

>> Anyway, cf the bugzilla this was a pretty bad regression for 6.2, so
>> would be good to move this along reasonably quickly (although I guess we
>> just missed the -net PR for rc7)...
>
> I'm not planning to send anymore stuff to v6.3 so my plan is to take
> this to -next. The merge window is very close anyway so this shouldn't
> cause too much delay.

Hmm, okay, a bit unfortunate that we'll ship 6.3 with the same bug, but
if it goes in during the merge window, I guess we'll get the fix into
6.3.1 (or something close to that) via stable? I can live with that...

-Toke

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Kalle Valo]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

>>> Anyway, cf the bugzilla this was a pretty bad regression for 6.2, so
>>> would be good to move this along reasonably quickly (although I guess we
>>> just missed the -net PR for rc7)...
>>
>> I'm not planning to send anymore stuff to v6.3 so my plan is to take
>> this to -next. The merge window is very close anyway so this shouldn't
>> cause too much delay.
>
> Hmm, okay, a bit unfortunate that we'll ship 6.3 with the same bug

Yeah, it is unfortunate. But it is always a question of time :) To save
time I usually try to send two wireless tree pull requests per cycle,
one early in the cycle and the second around the middle.

> but if it goes in during the merge window, I guess we'll get the fix
> into 6.3.1 (or something close to that) via stable? I can live with
> that...

Yeah, that's what I'm hoping as well. The patch has cc stable so I
assume it should go quickly to stable releases.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Thorsten Leemhuis]

[CCing Jakub, Greg and the regressions list]

On 19.04.23 06:54, Kalle Valo wrote:
> Toke Høiland-Jørgensen <toke@toke.dk> writes:
> 
>>>> Anyway, cf the bugzilla

[FWIW, it's this ticket Toke meant:
https://bugzilla.kernel.org/show_bug.cgi?id=217286 ]

>>>> this was a pretty bad regression for 6.2, so
>>>> would be good to move this along reasonably quickly (although I guess we
>>>> just missed the -net PR for rc7)...
>>>
>>> I'm not planning to send anymore stuff to v6.3 so my plan is to take
>>> this to -next. The merge window is very close anyway so this shouldn't
>>> cause too much delay.
>>
>> Hmm, okay, a bit unfortunate that we'll ship 6.3 with the same bug
> 
> Yeah, it is unfortunate.

Agreed.

> But it is always a question of time :) To save
> time I usually try to send two wireless tree pull requests per cycle,
> one early in the cycle and the second around the middle.

Why not ask Linus to pull this directly from the list then? He doesn't
mind doing that for an occasional regression fix. And then he can decide
himself if the change is worth the risk -- and obviously can take into
account if he'll release and rc8 or not.

That's why Documentation/process/handling-regressions.rst
(https://docs.kernel.org/process/handling-regressions.html ) even tells
people to use that approach in a situation like this one.

Fun fact: that document also says "Subsystem maintainers are expected to
assist in reaching those periods [...]. They thus might have to send
git-pull requests earlier or more often than usual; [...]". That whole
section has a lot of "Ideally" in it, because yes, this thing called
real life is complicated sometimes and we are all volunteers. But still
it's a bit unfortunate that such a important tree like wireless doesn't
sent its fixes upstream every week.

> The patch has cc stable so I assume it should go quickly to stable
> releases.

To me it looks a lot like Greg most of the time only backports important
bug fixes during merge windows (e.g. when asked or when he noticed
something important) -- and leaves everything else often until after
rc1. And when there are a lot of backports, he might even do it in two
batches then. Hence it might easily take until ~May 11th or 18th (if we
get a rc8) until this fix reaches 6.2.y and 6.3.y.

Then it in the end would have taken about one month for the fix to reach
the users. That is really unfortunate. Preventing such situations (which
are not rare) is one of the main reason why handling-regressions.rst was
written like it is...

Ciao, Thorsten

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Toke Høiland-Jørgensen]

Thorsten Leemhuis <regressions@leemhuis.info> writes:

> [CCing Jakub, Greg and the regressions list]
>
> On 19.04.23 06:54, Kalle Valo wrote:
>> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>> 
>>>>> Anyway, cf the bugzilla
>
> [FWIW, it's this ticket Toke meant:
> https://bugzilla.kernel.org/show_bug.cgi?id=217286 ]
>
>>>>> this was a pretty bad regression for 6.2, so
>>>>> would be good to move this along reasonably quickly (although I guess we
>>>>> just missed the -net PR for rc7)...
>>>>
>>>> I'm not planning to send anymore stuff to v6.3 so my plan is to take
>>>> this to -next. The merge window is very close anyway so this shouldn't
>>>> cause too much delay.
>>>
>>> Hmm, okay, a bit unfortunate that we'll ship 6.3 with the same bug
>> 
>> Yeah, it is unfortunate.
>
> Agreed.
>
>> But it is always a question of time :) To save
>> time I usually try to send two wireless tree pull requests per cycle,
>> one early in the cycle and the second around the middle.
>
> Why not ask Linus to pull this directly from the list then? He doesn't
> mind doing that for an occasional regression fix. And then he can decide
> himself if the change is worth the risk -- and obviously can take into
> account if he'll release and rc8 or not.

I'm OK with doing it that way; I'll do so later tonight unless Kalle or
Jakub complains before then...

-Toke

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Jakub Kicinski]

On Thu, 20 Apr 2023 16:24:53 +0200 Toke Høiland-Jørgensen wrote:
> >> But it is always a question of time :) To save
> >> time I usually try to send two wireless tree pull requests per cycle,
> >> one early in the cycle and the second around the middle.  
> >
> > Why not ask Linus to pull this directly from the list then?

Out of curiosity, Thorsten, do you have stats on "how long does it take
fixes to reach Linus" per tree? Stats get people to act much quicker
than pleas, just sayin' ;)

> > He doesn't
> > mind doing that for an occasional regression fix. And then he can decide
> > himself if the change is worth the risk -- and obviously can take into
> > account if he'll release and rc8 or not.  
> 
> I'm OK with doing it that way; I'll do so later tonight unless Kalle or
> Jakub complains before then...

Ah, just after our last(?) 6.3 PR was submitted :(
No objections to you posting this directly to Linus...

That said it is a 6.2 regression AFAICT so it's not exactly in the
"must be fixed in 6.3" category. Assuming Kalle doesn't want a PR -
should we take it into net and have it reach Linus either next Tue
(assuming no -rc8) or Thu (if -rc8)?

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Toke Høiland-Jørgensen]

Jakub Kicinski <kuba@kernel.org> writes:

>> > He doesn't
>> > mind doing that for an occasional regression fix. And then he can decide
>> > himself if the change is worth the risk -- and obviously can take into
>> > account if he'll release and rc8 or not.  
>> 
>> I'm OK with doing it that way; I'll do so later tonight unless Kalle or
>> Jakub complains before then...
>
> Ah, just after our last(?) 6.3 PR was submitted :(
> No objections to you posting this directly to Linus...

Heh, yeah :( In retrospect, just asking you to take it directly into
-net might have been the expedient thing to do here.

> That said it is a 6.2 regression AFAICT so it's not exactly in the
> "must be fixed in 6.3" category. Assuming Kalle doesn't want a PR -
> should we take it into net and have it reach Linus either next Tue
> (assuming no -rc8) or Thu (if -rc8)?

How about I ping Linus and if he doesn't want to take it directly, you
can pull it into -net?

-Toke

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Jakub Kicinski]

On Thu, 20 Apr 2023 17:56:26 +0200 Toke Høiland-Jørgensen wrote:
> > That said it is a 6.2 regression AFAICT so it's not exactly in the
> > "must be fixed in 6.3" category. Assuming Kalle doesn't want a PR -
> > should we take it into net and have it reach Linus either next Tue
> > (assuming no -rc8) or Thu (if -rc8)?  
> 
> How about I ping Linus and if he doesn't want to take it directly, you
> can pull it into -net?

Fine by me :)  Just make sure to CC netdev

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Thorsten Leemhuis]

On 20.04.23 16:50, Jakub Kicinski wrote:
> On Thu, 20 Apr 2023 16:24:53 +0200 Toke Høiland-Jørgensen wrote:
>>>> But it is always a question of time :) To save
>>>> time I usually try to send two wireless tree pull requests per cycle,
>>>> one early in the cycle and the second around the middle.  
>>>
>>> Why not ask Linus to pull this directly from the list then?

BTW (to reply to two mails with one): Toke, thx for giving it a shot!

> Out of curiosity, Thorsten, do you have stats on "how long does it take
> fixes to reach Linus" per tree? Stats get people to act much quicker
> than pleas, just sayin' ;)

I know, I know... :-( Nevertheless thx for the reminder. :-D

I really wish that I had some, but right now the data I have in regzbot
is too messy and not a good base to generate such stats, as they would
likely be misleading (that's the long story short).

I once had the rough plan to approach this differently by looking at all
commits that ended up in the first big batches of stable updates (e.g.
releases like 6.0.3 with many hundreds of changes). I wanted to filter
out the regression fixes and then (1)look how long it took from posting
the fix till it was mainlined and (2)backported to stable. But there
afaics is no good way to automate the first part of the job: filtering
out fixes for regressions that actually bothered someone and might or
might not have been tracked by regzbot (the "might not" share might be
the bigger one, which is part of the valid stats problem indicated above).

>>> He doesn't
>>> mind doing that for an occasional regression fix. And then he can decide
>>> himself if the change is worth the risk -- and obviously can take into
>>> account if he'll release and rc8 or not.  
>>
>> I'm OK with doing it that way; I'll do so later tonight unless Kalle or
>> Jakub complains before then...
> 
> Ah, just after our last(?) 6.3 PR was submitted :(
> No objections to you posting this directly to Linus...
> 
> That said it is a 6.2 regression AFAICT so it's not exactly in the
> "must be fixed in 6.3" category.

Out of curiosity (really, it's not meant as a rhetorical device, I'm
trying to understand this point of view):

Why do you think that? And should it really be like that?

Sure, if it was an old problem (say from 5.18) that was only recently
found I'd agree, especially if the fix might have a more than average
risk of causing other trouble. But shouldn't we care about regressions
that were found shortly after a release (e.g. 6.2 in this case) at least
as much (or maybe even more?) as we care about those found during the
weeks preceding it?

FWIW, it's not the first time I hear a statement like that and every
time I wonder how Linus thinks about this. But whatever, not going to CC
him for that.

Ciao, Thorsten

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Jakub Kicinski]

On Thu, 20 Apr 2023 17:59:49 +0200 Thorsten Leemhuis wrote:
> > Out of curiosity, Thorsten, do you have stats on "how long does it take
> > fixes to reach Linus" per tree? Stats get people to act much quicker
> > than pleas, just sayin' ;)  
> 
> I know, I know... :-( Nevertheless thx for the reminder. :-D
> 
> I really wish that I had some, but right now the data I have in regzbot
> is too messy and not a good base to generate such stats, as they would
> likely be misleading (that's the long story short).
> 
> I once had the rough plan to approach this differently by looking at all
> commits that ended up in the first big batches of stable updates (e.g.
> releases like 6.0.3 with many hundreds of changes). I wanted to filter
> out the regression fixes and then (1)look how long it took from posting
> the fix till it was mainlined and (2)backported to stable. But there
> afaics is no good way to automate the first part of the job: filtering
> out fixes for regressions that actually bothered someone and might or
> might not have been tracked by regzbot (the "might not" share might be
> the bigger one, which is part of the valid stats problem indicated above).

I wouldn't bother with the patches you didn't track in regzbot.
This probably depends on how various people apply / maintain their
patches (sigh) but for networking (and anything else that's pretty 
pure git management) author date of the commit should give you the
time when patch was posted.

So we could go regzbot report date -> author date in upstream -> commit
date in stable potentially without much coding.

Going to Linus's tree vs stable should also be possible. Chris Mason
has showed me once a git incantation which finds the merge commit in
Linus's tree at which a given patch has arrived.. but I lost it.

> >> I'm OK with doing it that way; I'll do so later tonight unless Kalle or
> >> Jakub complains before then...  
> > 
> > Ah, just after our last(?) 6.3 PR was submitted :(
> > No objections to you posting this directly to Linus...
> > 
> > That said it is a 6.2 regression AFAICT so it's not exactly in the
> > "must be fixed in 6.3" category.  
> 
> Out of curiosity (really, it's not meant as a rhetorical device, I'm
> trying to understand this point of view):
> 
> Why do you think that? And should it really be like that?
> 
> Sure, if it was an old problem (say from 5.18) that was only recently
> found I'd agree, especially if the fix might have a more than average
> risk of causing other trouble. But shouldn't we care about regressions
> that were found shortly after a release (e.g. 6.2 in this case) at least
> as much (or maybe even more?) as we care about those found during the
> weeks preceding it?
> 
> FWIW, it's not the first time I hear a statement like that and every
> time I wonder how Linus thinks about this. But whatever, not going to CC
> him for that.

I'm a but curious what Linus would think, too.

Just to be clear the assumption I operate on is that all regressions
are important to fix within reasonable time frame. The question is
whether it matters for this regression that we're close to final.
Whether we should engage extraordinary means to get the fix in before
final is cut.

If it was a 6.3 regression we should try as hard as we can to fix it
in final (e.g. the mlx5 regression), if it's in 6.2 already - the extra
week of waiting may not be worth skipping trees and bothering Linus.

IOW for older regressions there's only the question of whether the fix
is in upstream in a reasonable time. It doesn't matter as much which
particular tag it hits.

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Linux regression tracking (Thorsten Leemhuis)]

[CCing Linus: there are two things near the end of this mail where we're
wondering how you feel about them]

On 20.04.23 18:55, Jakub Kicinski wrote:
> On Thu, 20 Apr 2023 17:59:49 +0200 Thorsten Leemhuis wrote:
>>> Out of curiosity, Thorsten, do you have stats on "how long does it take
>>> fixes to reach Linus" per tree? Stats get people to act much quicker
>>> than pleas, just sayin' ;)  
>>
>> I know, I know... :-( Nevertheless thx for the reminder. :-D
>>
>> I really wish that I had some, but right now the data I have in regzbot
>> is too messy and not a good base to generate such stats, as they would
>> likely be misleading (that's the long story short).
>>
>> I once had the rough plan to approach this differently by looking at all
>> commits that ended up in the first big batches of stable updates (e.g.
>> releases like 6.0.3 with many hundreds of changes). I wanted to filter
>> out the regression fixes and then (1)look how long it took from posting
>> the fix till it was mainlined and (2)backported to stable. But there
>> afaics is no good way to automate the first part of the job: filtering
>> out fixes for regressions that actually bothered someone and might or
>> might not have been tracked by regzbot (the "might not" share might be
>> the bigger one, which is part of the valid stats problem indicated above).
> 
> I wouldn't bother with the patches you didn't track in regzbot.
> This probably depends on how various people apply / maintain their
> patches (sigh) but for networking (and anything else that's pretty 
> pure git management) author date of the commit should give you the
> time when patch was posted.

Unless the patch went through several revisions during review. But
that's just a detail.

> So we could go regzbot report date -> author date in upstream -> commit
> date in stable potentially without much coding.
>
> Going to Linus's tree vs stable should also be possible. Chris Mason
> has showed me once a git incantation which finds the merge commit in
> Linus's tree at which a given patch has arrived.. but I lost it.

I have something ugly for that (I needed a python variant of this
somewhere in regzbot):

commit=91dcf1e80;
branch=origin/master;
git show $(git rev-list "${commit}".."${branch}" --ancestry-path |
grep -f <(git rev-list "${commit}".."${branch}" --first-parent) |
tail -1)

[note: this will fail for any changes Linus committed directly to mainline]

>>>> I'm OK with doing it that way; I'll do so later tonight unless Kalle or
>>>> Jakub complains before then...  
>>>
>>> Ah, just after our last(?) 6.3 PR was submitted :(
>>> No objections to you posting this directly to Linus...
>>>
>>> That said it is a 6.2 regression AFAICT so it's not exactly in the
>>> "must be fixed in 6.3" category.  
>>
>> Out of curiosity (really, it's not meant as a rhetorical device, I'm
>> trying to understand this point of view):
>>
>> Why do you think that? And should it really be like that?
>>
>> Sure, if it was an old problem (say from 5.18) that was only recently
>> found I'd agree, especially if the fix might have a more than average
>> risk of causing other trouble. But shouldn't we care about regressions
>> that were found shortly after a release (e.g. 6.2 in this case) at least
>> as much (or maybe even more?) as we care about those found during the
>> weeks preceding it?
>>
>> FWIW, it's not the first time I hear a statement like that and every
>> time I wonder how Linus thinks about this. But whatever, not going to CC
>> him for that.
> 
> I'm a but curious what Linus would think, too.

:-)

I CCed Linus now, as another question for him came up below. With a bit
of luck he'll share his view on these matters. And if not I'm pretty
sure we'll all live happily ever after, too. :-D

> Just to be clear the assumption I operate on is that all regressions
> are important to fix within reasonable time frame. The question is
> whether it matters for this regression that we're close to final.
> Whether we should engage extraordinary means to get the fix in before
> final is cut.

Well, the risk obviously is a factor here, especially during a potential
release week (like the one we're in); but I don't think the risk
evaluation for "fixes for regressions introduced in the previous cycle"
and "fixes for regressions introduced this cycle" should be much
different. But that's see what Linus thinks about this.

> If it was a 6.3 regression we should try as hard as we can to fix it
> in final (e.g. the mlx5 regression), if it's in 6.2 already - the extra
> week 

Well, yes, in the ideal case it is just a week. But at this point of the
devel cycle a one week delay in mainlining can easily result in a two
week delay till the fix reaches users through the stable trees. To explain:

* Assume this fix (posted one week ago) is not merged this week and
Linus releases 6.3 on Sunday; assume further the fix then will go to
mainline during the merge window (e.g. the one week delay scenario). If
that happens during the first or second week of the merge window doesn't
even matter much, as Greg apparently often waits till after -rc1 is
released before be starts backporting most fixes. And that's where the
extra week comes from. We of course could avoid this by bothering Greg
to pick up the fix once it reached mainline; but then it might be better
to bother Linus in the first place to merge is this week.

Not to mention that there would have been another week of delay, if (a)
I had not spoken up in this thread and (b) we get a rc8. But we afaics
avoided that scenario already with the plan to merge the fix next week
through the -net tree in case we get a rc8.

> of waiting may not be worth skipping trees and bothering Linus.

Linus, to you feel bothered by an occational additional pull request or
a mail asking you to pick up a patch directly from the list?

> IOW for older regressions there's only the question of whether the fix
> is in upstream in a reasonable time. It doesn't matter as much which
> particular tag it hits.

Ciao, Thorsten

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Kalle Valo]

Toke Høiland-Jørgensen <toke@toke.dk> wrote:

> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
> 
> Turns out the channelmap variable is not actually read-only, it's modified
> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
> so making it read-only causes page faults when that code is hit.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and channelmap static const")
> Cc: stable@vger.kernel.org
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

b956e3110a79 wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230413214118.153781-1-toke@toke.dk/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()

[Colin King (gmail)]

On 19/04/2023 15:24, Kalle Valo wrote:
> Toke Høiland-Jørgensen <toke@toke.dk> wrote:
> 
>> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
>>
>> Turns out the channelmap variable is not actually read-only, it's modified
>> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
>> so making it read-only causes page faults when that code is hit.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
>> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and channelmap static const")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> 
> Patch applied to ath-next branch of ath.git, thanks.
> 
> b956e3110a79 wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()
> 

Thanks. Apologies for the regression. My bad.

One-off regression fix for 6.3 [was: Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()]

[Toke Høiland-Jørgensen]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> This partially reverts commit e161d4b60ae3a5356e07202e0bfedb5fad82c6aa.
>
> Turns out the channelmap variable is not actually read-only, it's modified
> through the MCI_GPM_CLR_CHANNEL_BIT() macro further down in the function,
> so making it read-only causes page faults when that code is hit.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217183
> Fixes: e161d4b60ae3 ("wifi: ath9k: Make arrays prof_prio and channelmap static const")
> Cc: stable@vger.kernel.org
> Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>

Hi Linus

Thorsten already pulled you into the thread further down, but I figured
I'd do this writeup anyway so you have the full context:

The patch quoted above[0] fixes a regression in the ath9k driver that was
introduced in 6.2, which causes a kernel BUG() whenever the "Bluetooth
co-existence" feature in the driver is enabled (which seems to be the
default on at least some systems).

Because of unfortunate timing (caused by an impedance mismatch between
the wireless tree and the -net tree, and my failure to realise this and
push it directly to -net), this patch did not make it into this week's
network tree pull request to you. Which means that unless you decide to
do an -rc8, this regression will also be in the 6.3 release, and it may
take several more weeks before the fix makes it into a stable release.

So, with a bit of prodding from Thorsten, I'm writing this to ask you if
you'd be willing to pull this patch directly from the mailing list as a
one-off? It's a fairly small patch, and since it's a (partial) revert
the risk of it being the cause of new regressions should be fairly
small. One of the reporters on the Bugzilla (linked above) confirmed
that the patch does indeed fix the regression.

In case you *don't* want to take this patch directly, Jakub has agreed
to pull it directly into -net, in which case it'll land in your tree via
the next networking pull request. Either way, as indicated by the
sibling thread Thorsten Cc'ed you on, we'll take your opinion on the
best way to handle this into account in the future. Just let us know :)

Thanks,
-Toke


[0] Direct Lore link: https://lore.kernel.org/r/20230413214118.153781-1-toke@toke.dk

Re: One-off regression fix for 6.3 [was: Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()]

[Linus Torvalds]

On Thu, Apr 20, 2023 at 2:09 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>
> So, with a bit of prodding from Thorsten, I'm writing this to ask you if
> you'd be willing to pull this patch directly from the mailing list as a
> one-off? It's a fairly small patch, and since it's a (partial) revert
> the risk of it being the cause of new regressions should be fairly
> small.

Sure. I'm always open to direct fixes when there is no controversy
about the fix. No problem. I still happily deal with individual
patches.

And yes, I do consider "regression in an earlier release" to be a
regression that needs fixing.

There's obviously a time limit: if that "regression in an earlier
release" was a year or more ago, and just took forever for people to
notice, and it had semantic changes that now mean that fixing the
regression could cause a _new_ regression, then that can cause me to
go "Oh, now the new semantics are what we have to live with".

But something like this, where the regression was in the previous
release and it's just a clear fix with no semantic subtlety, I
consider to be just a regular regression that should be expedited -
partly to make it into stable, and partly to avoid having to put the
fix into _another_ stable kernel..

             Linus

Re: One-off regression fix for 6.3 [was: Re: [PATCH] wifi: ath9k: Don't mark channelmap stack variable read-only in ath9k_mci_update_wlan_channels()]

[Toke Høiland-Jørgensen]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Apr 20, 2023 at 2:09 PM Toke Høiland-Jørgensen <toke@toke.dk> wrote:
>>
>> So, with a bit of prodding from Thorsten, I'm writing this to ask you if
>> you'd be willing to pull this patch directly from the mailing list as a
>> one-off? It's a fairly small patch, and since it's a (partial) revert
>> the risk of it being the cause of new regressions should be fairly
>> small.
>
> Sure. I'm always open to direct fixes when there is no controversy
> about the fix. No problem. I still happily deal with individual
> patches.

Awesome, thanks!

> And yes, I do consider "regression in an earlier release" to be a
> regression that needs fixing.
>
> There's obviously a time limit: if that "regression in an earlier
> release" was a year or more ago, and just took forever for people to
> notice, and it had semantic changes that now mean that fixing the
> regression could cause a _new_ regression, then that can cause me to
> go "Oh, now the new semantics are what we have to live with".
>
> But something like this, where the regression was in the previous
> release and it's just a clear fix with no semantic subtlety, I
> consider to be just a regular regression that should be expedited -
> partly to make it into stable, and partly to avoid having to put the
> fix into _another_ stable kernel..

OK, duly noted; thank you for clarifying :)

-Toke