LinuxLists.cc - pull-request: wireless-next-2023-04-21

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
> Hi,
>
> here's a pull request to net-next tree, more info below. Please let me know if
> there are any problems.

Sparse warning to follow up on:

drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: warning: invalid assignment: |=
drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: left side has type restricted __le32
drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: right side has type unsigned long

2023-04-21 15:03:44

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
> .../net/wireless/realtek/rtw89/rtw8851b_table.c | 14824 +++++++++++++++++++
> .../net/wireless/realtek/rtw89/rtw8851b_table.h | 21 +

We should load these like FW, see the proposal outlined in
https://lore.kernel.org/all/[email protected]/
for example. Would that not work?

The huge register tables in C sources makes it look like
a Windows drivers :(

2023-04-21 15:15:48

by patchwork-bot+netdevbpf

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Hello:

This pull request was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:

On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) you wrote:
> Hi,
>
> here's a pull request to net-next tree, more info below. Please let me know if
> there are any problems.
>
> Kalle
>
> [...]

Here is the summary with links:
- pull-request: wireless-next-2023-04-21
https://git.kernel.org/netdev/net-next/c/ca2889658015

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

2023-04-24 17:38:11

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Jakub Kicinski <[email protected]> writes:

> On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
>> Hi,
>>
>> here's a pull request to net-next tree, more info below. Please let me know if
>> there are any problems.
>
> Sparse warning to follow up on:
>
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: warning:
> invalid assignment: |=
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: left side has
> type restricted __le32
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: right side
> has type unsigned long

Ah, sorry about that. We still have some sparse warnings left so I don't
check them for each pull request. We should fix all the remaining sparse
warnings in drivers/net/wireless, any volunteers? :) Would be a good
task for a newcomer.

Felix, could you submit a fix for this? I can then apply it to wireless
tree and send a pull request to net tree in two weeks or so.

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

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

2023-04-25 02:46:36

[permalink] [raw]

Subject: RE: pull-request: wireless-next-2023-04-21

> -----Original Message-----
> From: Jakub Kicinski <[email protected]>
> Sent: Friday, April 21, 2023 10:54 PM
> To: Kalle Valo <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: pull-request: wireless-next-2023-04-21
>
> On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
> > .../net/wireless/realtek/rtw89/rtw8851b_table.c | 14824 +++++++++++++++++++
> > .../net/wireless/realtek/rtw89/rtw8851b_table.h | 21 +
>
> We should load these like FW, see the proposal outlined in
> https://lore.kernel.org/all/[email protected]/
> for example. Would that not work?
>

That would work, and I think struct fields addr and val should be __le32.
And, I have some draft ideas to handle some situations we will face:

1. upgrading to newer driver without built-in tables will break user space
if people don't download table file from linux-firmware.git.
Maybe, we can keep the built-in tables and support loading from files
for couple years at least.

2. c code can do changes along with these tables, so driver should do some
compatibility things for register version.

3. The file contains not only simple registers tables but also TX power tables
and power tracking tables. These tables are multiple dimensions, and
dimensions can be changed due to more channels are supported, for example.
To be backward compatible, we need to add conversion function from
v1, v2 ... to current.

I will think further to make this change smooth.

Ping-Ke

2023-04-25 04:20:01

by Gregg Wonderly

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Could this be expressed in a /proc structure of files and directories?

Gregg Wonderly

> On Apr 24, 2023, at 9:41 PM, Ping-Ke Shih <[email protected]> wrote:
>
>
>
>> -----Original Message-----
>> From: Jakub Kicinski <[email protected]>
>> Sent: Friday, April 21, 2023 10:54 PM
>> To: Kalle Valo <[email protected]>
>> Cc: [email protected]; [email protected]
>> Subject: Re: pull-request: wireless-next-2023-04-21
>>
>> On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
>>> .../net/wireless/realtek/rtw89/rtw8851b_table.c | 14824 +++++++++++++++++++
>>> .../net/wireless/realtek/rtw89/rtw8851b_table.h | 21 +
>>
>> We should load these like FW, see the proposal outlined in
>> https://lore.kernel.org/all/[email protected]/
>> for example. Would that not work?
>>
>
> That would work, and I think struct fields addr and val should be __le32.
> And, I have some draft ideas to handle some situations we will face:
>
> 1. upgrading to newer driver without built-in tables will break user space
> if people don't download table file from linux-firmware.git.
> Maybe, we can keep the built-in tables and support loading from files
> for couple years at least.
>
> 2. c code can do changes along with these tables, so driver should do some
> compatibility things for register version.
>
> 3. The file contains not only simple registers tables but also TX power tables
> and power tracking tables. These tables are multiple dimensions, and
> dimensions can be changed due to more channels are supported, for example.
> To be backward compatible, we need to add conversion function from
> v1, v2 ... to current.
>
> I will think further to make this change smooth.
>
> Ping-Ke
>

2023-04-25 04:43:43

[permalink] [raw]

Subject: RE: pull-request: wireless-next-2023-04-21

Hi Gregg,

(no top posting for wireless mailing list, so I move your post to bottom)

> -----Original Message-----
> From: Gregg Wonderly <[email protected]>
> Sent: Tuesday, April 25, 2023 12:15 PM
> To: Ping-Ke Shih <[email protected]>
> Cc: Jakub Kicinski <[email protected]>; Kalle Valo <[email protected]>; [email protected];
> [email protected]
> Subject: Re: pull-request: wireless-next-2023-04-21
>
> > On Apr 24, 2023, at 9:41 PM, Ping-Ke Shih <[email protected]> wrote:
> >
> >
> >
> >> -----Original Message-----
> >> From: Jakub Kicinski <[email protected]>
> >> Sent: Friday, April 21, 2023 10:54 PM
> >> To: Kalle Valo <[email protected]>
> >> Cc: [email protected]; [email protected]
> >> Subject: Re: pull-request: wireless-next-2023-04-21
> >>
> >> On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
> >>> .../net/wireless/realtek/rtw89/rtw8851b_table.c | 14824 +++++++++++++++++++
> >>> .../net/wireless/realtek/rtw89/rtw8851b_table.h | 21 +
> >>
> >> We should load these like FW, see the proposal outlined in
> >> https://lore.kernel.org/all/[email protected]/
> >> for example. Would that not work?
> >>
> >
> > That would work, and I think struct fields addr and val should be __le32.
> > And, I have some draft ideas to handle some situations we will face:
> >
> > 1. upgrading to newer driver without built-in tables will break user space
> > if people don't download table file from linux-firmware.git.
> > Maybe, we can keep the built-in tables and support loading from files
> > for couple years at least.
> >
> > 2. c code can do changes along with these tables, so driver should do some
> > compatibility things for register version.
> >
> > 3. The file contains not only simple registers tables but also TX power tables
> > and power tracking tables. These tables are multiple dimensions, and
> > dimensions can be changed due to more channels are supported, for example.
> > To be backward compatible, we need to add conversion function from
> > v1, v2 ... to current.
> >
> > I will think further to make this change smooth.
> >
>
> Could this be expressed in a /proc structure of files and directories?
>

I'm not clear what you meant. Could you please give me an example for reference?

Ping-Ke

2023-04-25 05:42:33

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Ping-Ke Shih <[email protected]> writes:

>> -----Original Message-----
>> From: Jakub Kicinski <[email protected]>
>> Sent: Friday, April 21, 2023 10:54 PM
>> To: Kalle Valo <[email protected]>
>> Cc: [email protected]; [email protected]
>> Subject: Re: pull-request: wireless-next-2023-04-21
>>
>> On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
>> > .../net/wireless/realtek/rtw89/rtw8851b_table.c | 14824 +++++++++++++++++++
>> > .../net/wireless/realtek/rtw89/rtw8851b_table.h | 21 +
>>
>> We should load these like FW, see the proposal outlined in
>> https://lore.kernel.org/all/[email protected]/
>> for example. Would that not work?
>>
>
> That would work, and I think struct fields addr and val should be __le32.
> And, I have some draft ideas to handle some situations we will face:
>
> 1. upgrading to newer driver without built-in tables will break user space
> if people don't download table file from linux-firmware.git.
> Maybe, we can keep the built-in tables and support loading from files
> for couple years at least.
>
> 2. c code can do changes along with these tables, so driver should do some
> compatibility things for register version.
>
> 3. The file contains not only simple registers tables but also TX power tables
> and power tracking tables. These tables are multiple dimensions, and
> dimensions can be changed due to more channels are supported, for example.
> To be backward compatible, we need to add conversion function from
> v1, v2 ... to current.
>
> I will think further to make this change smooth.

IIRC we discussed this back in initial rtw88 or rtw89 driver review (not
sure which one). At the time I pushed for the current solution to have
the initvals in static variables just to avoid any backwards
compatibility issues. I agree that the initvals in .c files are ugly but
is it worth all the extra effort and complexity to move them outside the
kernel? I'm starting to lean towards it's not worth all the extra work.

For me most important is that backwards compatibility is not broken,
that would be bad for the users. So whatever we decide let's keep that
in mind.

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

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

2023-04-25 14:29:43

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

On Tue, 25 Apr 2023 08:38:17 +0300 Kalle Valo wrote:
> IIRC we discussed this back in initial rtw88 or rtw89 driver review (not
> sure which one). At the time I pushed for the current solution to have
> the initvals in static variables just to avoid any backwards
> compatibility issues. I agree that the initvals in .c files are ugly but
> is it worth all the extra effort and complexity to move them outside the
> kernel? I'm starting to lean towards it's not worth all the extra work.

I don't think it's that much extra work, the driver requires FW
according to modinfo, anyway, so /lib/firmware is already required.
And on smaller systems with few hundred MB of RAM it'd be nice to not
hold all the stuff in kernel memory, I'd think.

We have a rule against putting FW as a static table in the driver
source, right? Or did we abandon that? Isn't this fundamentally similar?

> For me most important is that backwards compatibility is not broken,
> that would be bad for the users. So whatever we decide let's keep that
> in mind.

Right, not for existing devices, only when new device is added.

2023-04-25 17:17:23

by Johannes Berg

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

On Tue, 2023-04-25 at 07:18 -0700, Jakub Kicinski wrote:
> On Tue, 25 Apr 2023 08:38:17 +0300 Kalle Valo wrote:
> > IIRC we discussed this back in initial rtw88 or rtw89 driver review (not
> > sure which one). At the time I pushed for the current solution to have
> > the initvals in static variables just to avoid any backwards
> > compatibility issues. I agree that the initvals in .c files are ugly but
> > is it worth all the extra effort and complexity to move them outside the
> > kernel? I'm starting to lean towards it's not worth all the extra work.
>
> I don't think it's that much extra work, the driver requires FW
> according to modinfo, anyway, so /lib/firmware is already required.

If the firmware is sufficiently unique to a device (which is likely) it
could even just be appended to that same file, assuming the file format
has any kind of container layout. But even that could be done fairly
easily.

johannes

2023-04-25 21:00:05

by Ryder Lee

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

On Mon, 2023-04-24 at 20:34 +0300, Kalle Valo wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Jakub Kicinski <[email protected]> writes:
>
> > On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
> > > Hi,
> > >
> > > here's a pull request to net-next tree, more info below. Please
> > > let me know if
> > > there are any problems.
> >
> > Sparse warning to follow up on:
> >
> > drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: warning:
> > invalid assignment: |=
> > drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: left side
> > has
> > type restricted __le32
> > drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: right side
> > has type unsigned long
>
> Ah, sorry about that. We still have some sparse warnings left so I
> don't
> check them for each pull request. We should fix all the remaining
> sparse
> warnings in drivers/net/wireless, any volunteers? :) Would be a good
> task for a newcomer.
>
> Felix, could you submit a fix for this? I can then apply it to
> wireless
> tree and send a pull request to net tree in two weeks or so.
>

My bad. I've posted a fix for that.

[1/2] wifi: mt76: mt7996: fix endianness of MT_TXD6_TX_RATE

Ryder

2023-04-26 03:18:05

[permalink] [raw]

Subject: RE: pull-request: wireless-next-2023-04-21

> -----Original Message-----
> From: Johannes Berg <[email protected]>
> Sent: Wednesday, April 26, 2023 1:09 AM
> To: Jakub Kicinski <[email protected]>; Kalle Valo <[email protected]>
> Cc: Ping-Ke Shih <[email protected]>; [email protected]; [email protected]
> Subject: Re: pull-request: wireless-next-2023-04-21
>
> On Tue, 2023-04-25 at 07:18 -0700, Jakub Kicinski wrote:
> > On Tue, 25 Apr 2023 08:38:17 +0300 Kalle Valo wrote:
> > > IIRC we discussed this back in initial rtw88 or rtw89 driver review (not
> > > sure which one). At the time I pushed for the current solution to have
> > > the initvals in static variables just to avoid any backwards
> > > compatibility issues. I agree that the initvals in .c files are ugly but
> > > is it worth all the extra effort and complexity to move them outside the
> > > kernel? I'm starting to lean towards it's not worth all the extra work.
> >
> > I don't think it's that much extra work, the driver requires FW
> > according to modinfo, anyway, so /lib/firmware is already required.
>
> If the firmware is sufficiently unique to a device (which is likely) it
> could even just be appended to that same file, assuming the file format
> has any kind of container layout. But even that could be done fairly
> easily.
>

I think the extra work Kalle meant is what I mentioned previously --
need functions to convert old tables v1, v2, ... to current. Like,

struct table_v1 { // from file
__le32 data[10];
};

struct table_v2 { // from file
__le32 data[20];
};

struct table { // from file, the latest version of current use
__le32 data[30];
};

struct table_cpu { // current table in cpu order
u32 data[30];
};

If loading a table_v1 table, for example, we need to convert to table_cpu by
some rules. Also, maybe we need to disable some features relay on the values
introduced by table_cpu. I think it will work, but just add some flags and
rules to handle them.

Another question is about number of files for single device. Since firmware and
tables (e.g. TX power, registers) are released by different people, and they
maintain their own version, if I append tables to firmware, it's a little hard
to have a clear version code. So, I would like to know the rule if I can just
add additional one file for these tables?

Ping-Ke

2023-04-26 03:48:45

[permalink] [raw]

Subject: RE: pull-request: wireless-next-2023-04-21

> -----Original Message-----
> From: Ping-Ke Shih <[email protected]>
> Sent: Wednesday, April 26, 2023 11:16 AM
> To: Johannes Berg <[email protected]>; Jakub Kicinski <[email protected]>; Kalle Valo
> <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: RE: pull-request: wireless-next-2023-04-21
>
> > -----Original Message-----
> > From: Johannes Berg <[email protected]>
> > Sent: Wednesday, April 26, 2023 1:09 AM
> > To: Jakub Kicinski <[email protected]>; Kalle Valo <[email protected]>
> > Cc: Ping-Ke Shih <[email protected]>; [email protected]; [email protected]
> > Subject: Re: pull-request: wireless-next-2023-04-21
> >
> > On Tue, 2023-04-25 at 07:18 -0700, Jakub Kicinski wrote:
> > > On Tue, 25 Apr 2023 08:38:17 +0300 Kalle Valo wrote:
> > > > IIRC we discussed this back in initial rtw88 or rtw89 driver review (not
> > > > sure which one). At the time I pushed for the current solution to have
> > > > the initvals in static variables just to avoid any backwards
> > > > compatibility issues. I agree that the initvals in .c files are ugly but
> > > > is it worth all the extra effort and complexity to move them outside the
> > > > kernel? I'm starting to lean towards it's not worth all the extra work.
> > >
> > > I don't think it's that much extra work, the driver requires FW
> > > according to modinfo, anyway, so /lib/firmware is already required.
> >
> > If the firmware is sufficiently unique to a device (which is likely) it
> > could even just be appended to that same file, assuming the file format
> > has any kind of container layout. But even that could be done fairly
> > easily.
> >
>
> I think the extra work Kalle meant is what I mentioned previously --
> need functions to convert old tables v1, v2, ... to current. Like,
>

struct table_v1 { // from file
__le32 channel_tx_power[10];
};

struct table_v2 { // from file
__le32 channel_tx_power[20];
};

struct table { // from file, the latest version of current use
__le32 channel_tx_power[30];
};

struct table_cpu { // current table in cpu order
u32 channel_tx_power[30];
};

To make example clearer, I change the name of fields, because the thing I
want to mention is not register table that wouldn't need conversion.

>
> If loading a table_v1 table, for example, we need to convert to table_cpu by
> some rules. Also, maybe we need to disable some features relay on the values
> introduced by table_cpu. I think it will work, but just add some flags and
> rules to handle them.
>
>
> Another question is about number of files for single device. Since firmware and
> tables (e.g. TX power, registers) are released by different people, and they
> maintain their own version, if I append tables to firmware, it's a little hard
> to have a clear version code. So, I would like to know the rule if I can just
> add additional one file for these tables?
>
> Ping-Ke
>
>
> ------Please consider the environment before printing this e-mail.

2023-04-26 08:35:04

by Johannes Berg

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

On Wed, 2023-04-26 at 03:30 +0000, Ping-Ke Shih wrote:
> > >

> >
> > I think the extra work Kalle meant is what I mentioned previously --
> > need functions to convert old tables v1, v2, ... to current. Like,
> >
>
> struct table_v1 { // from file
> __le32 channel_tx_power[10];
> };
>
> struct table_v2 { // from file
> __le32 channel_tx_power[20];
> };
>
> struct table { // from file, the latest version of current use
> __le32 channel_tx_power[30];
> };
>
> struct table_cpu { // current table in cpu order
> u32 channel_tx_power[30];
> };
>
> To make example clearer, I change the name of fields, because the thing I
> want to mention is not register table that wouldn't need conversion.

Right, the file format would have to be __le32 (or __be32), but that's
pretty easy to handle while writing it to the device?

Not sure I understand the other thing about conversion.

> > If loading a table_v1 table, for example, we need to convert to table_cpu by
> > some rules. Also, maybe we need to disable some features relay on the values
> > introduced by table_cpu. I think it will work, but just add some flags and
> > rules to handle them.

But wouldn't this basically be tied to a driver? I mean you could have a
file called "rtlwifi/rtlxyz.v1.tables" that the driver in kernel 6.4
loads, and ...v2... that the driver in 6.5 loads, and requires for
operation?

Then again - it'd be better if the driver in 6.5 can deal with it if a
user didn't install the v2 file yet, is that what you meant?

> > Another question is about number of files for single device. Since firmware and
> > tables (e.g. TX power, registers) are released by different people, and they
> > maintain their own version, if I append tables to firmware, it's a little hard
> > to have a clear version code. So, I would like to know the rule if I can just
> > add additional one file for these tables?

Oh, I certainly wasn't trying to say that it should be done by combining
the file, just that it might be _easier_ to distribute this stuff then.
If not, then not!

johannes

2023-04-27 00:46:53

[permalink] [raw]

Subject: RE: pull-request: wireless-next-2023-04-21

> -----Original Message-----
> From: Johannes Berg <[email protected]>
> Sent: Wednesday, April 26, 2023 4:25 PM
> To: Ping-Ke Shih <[email protected]>; Jakub Kicinski <[email protected]>; Kalle Valo <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: pull-request: wireless-next-2023-04-21
>
> On Wed, 2023-04-26 at 03:30 +0000, Ping-Ke Shih wrote:
> > > >
>
> > >
> > > I think the extra work Kalle meant is what I mentioned previously --
> > > need functions to convert old tables v1, v2, ... to current. Like,
> > >
> >
> > struct table_v1 { // from file
> > __le32 channel_tx_power[10];
> > };
> >
> > struct table_v2 { // from file
> > __le32 channel_tx_power[20];
> > };
> >
> > struct table { // from file, the latest version of current use
> > __le32 channel_tx_power[30];
> > };
> >
> > struct table_cpu { // current table in cpu order
> > u32 channel_tx_power[30];
> > };
> >
> > To make example clearer, I change the name of fields, because the thing I
> > want to mention is not register table that wouldn't need conversion.
>
> Right, the file format would have to be __le32 (or __be32), but that's
> pretty easy to handle while writing it to the device?
>
> Not sure I understand the other thing about conversion.

Right. If all elements are the same type (e.g. __le32), it would be much easier.
The difficulty I want to say is backward compatibility.

>
> > > If loading a table_v1 table, for example, we need to convert to table_cpu by
> > > some rules. Also, maybe we need to disable some features relay on the values
> > > introduced by table_cpu. I think it will work, but just add some flags and
> > > rules to handle them.
>
> But wouldn't this basically be tied to a driver? I mean you could have a
> file called "rtlwifi/rtlxyz.v1.tables" that the driver in kernel 6.4
> loads, and ...v2... that the driver in 6.5 loads, and requires for
> operation?
>
> Then again - it'd be better if the driver in 6.5 can deal with it if a
> user didn't install the v2 file yet, is that what you meant?

Yes, this is my point, and I think 6.5 _must_ deal with v1 file.

Considering below artificial drama:

1. kernel 6.4, driver support 2GHz channels only (table v1)
__le32 channel_tx_power_v1[2GHz_NUM]

2. kernel 6.5, driver support 2 + 5GHz channels (table v2)
__le32 channel_tx_power_v2[2GHz_NUM + 5GHz_NUM]

A user could not install v2, so I need a conversion, like
convert_v1_to_v2(struct table_v1 *v1, struct table_v2 *v2) // also disable 5GHz channels

3. kernel 6.6, driver support 2 + 5 + 6GHz channels (table v3)
__le32 channel_tx_power_v2[2GHz_NUM + 5GHz_NUM + 6GHz_NUM]
A user could not install v3, so I need an additional conversion, like
convert_v2_to_v3(struct table_v2 *v2, struct table_v3 *v3) // also disable 6GHz channels

If more table versions are introduced, more conversions are needed. Also,
I'm not sure how these tables can change in the future, so the conversion
may be complicated if they have a big change for certain reason.

My point is that this work is possible, but introduce some extra works that
maybe look a little dirty.

Ping-Ke

2023-04-28 08:52:55

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Jakub Kicinski <[email protected]> writes:

> On Fri, 21 Apr 2023 10:47:26 +0000 (UTC) Kalle Valo wrote:
>> Hi,
>>
>> here's a pull request to net-next tree, more info below. Please let me know if
>> there are any problems.
>
> Sparse warning to follow up on:
>
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: warning: invalid assignment: |=
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: left side has type restricted __le32
> drivers/net/wireless/mediatek/mt76/mt7996/mac.c:1091:25: right side has type unsigned long

This patch should fix it:

https://patchwork.kernel.org/project/linux-wireless/patch/16fa938373e3b145cb07a2c98d2428fea2abadba.1682285873.git.ryder.lee@mediatek.com/

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

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

2023-04-28 10:39:18

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Ping-Ke Shih <[email protected]> writes:

>> > > If loading a table_v1 table, for example, we need to convert to table_cpu by
>> > > some rules. Also, maybe we need to disable some features relay on the values
>> > > introduced by table_cpu. I think it will work, but just add some flags and
>> > > rules to handle them.
>>
>> But wouldn't this basically be tied to a driver? I mean you could have a
>> file called "rtlwifi/rtlxyz.v1.tables" that the driver in kernel 6.4
>> loads, and ...v2... that the driver in 6.5 loads, and requires for
>> operation?
>>
>> Then again - it'd be better if the driver in 6.5 can deal with it if a
>> user didn't install the v2 file yet, is that what you meant?
>
> Yes, this is my point, and I think 6.5 _must_ deal with v1 file.

I agree. In this example the time between v6.4 and v6.5 is roughly three
months, so we can't drop v1 support that fast as there will be people
upgrading their kernels but don't have the v2 firmware file yet.

I would say supporting one year is too short (think LTS distros etc) so
to be on the safe side we should support old firmware files at least for
two years. That's a long time.

> Considering below artificial drama:
>
> 1. kernel 6.4, driver support 2GHz channels only (table v1)
> __le32 channel_tx_power_v1[2GHz_NUM]
>
> 2. kernel 6.5, driver support 2 + 5GHz channels (table v2)
> __le32 channel_tx_power_v2[2GHz_NUM + 5GHz_NUM]
>
> A user could not install v2, so I need a conversion, like
> convert_v1_to_v2(struct table_v1 *v1, struct table_v2 *v2) // also
> disable 5GHz channels
>
> 3. kernel 6.6, driver support 2 + 5 + 6GHz channels (table v3)
> __le32 channel_tx_power_v2[2GHz_NUM + 5GHz_NUM + 6GHz_NUM]
> A user could not install v3, so I need an additional conversion, like
> convert_v2_to_v3(struct table_v2 *v2, struct table_v3 *v3) // also
> disable 6GHz channels

This is exactly what I'm worried about. And we have also the case that
user space doesn't have the initval.bin file at all so we need to have
initvals in kernel for something like two years.

> If more table versions are introduced, more conversions are needed. Also,
> I'm not sure how these tables can change in the future, so the conversion
> may be complicated if they have a big change for certain reason.
>
> My point is that this work is possible, but introduce some extra works that
> maybe look a little dirty.

Also I agree here. This creates complexity and that of course increases
the risk of bugs. Even if it sounds simple, in practise it's not. Of
course if the initvals don't change then it's easier, but I would not
rely on that.

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

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

2023-04-28 10:59:04

[permalink] [raw]

Subject: Re: pull-request: wireless-next-2023-04-21

Jakub Kicinski <[email protected]> writes:

> On Tue, 25 Apr 2023 08:38:17 +0300 Kalle Valo wrote:
>> IIRC we discussed this back in initial rtw88 or rtw89 driver review (not
>> sure which one). At the time I pushed for the current solution to have
>> the initvals in static variables just to avoid any backwards
>> compatibility issues. I agree that the initvals in .c files are ugly but
>> is it worth all the extra effort and complexity to move them outside the
>> kernel? I'm starting to lean towards it's not worth all the extra work.
>
> I don't think it's that much extra work, the driver requires FW
> according to modinfo, anyway, so /lib/firmware is already required.
> And on smaller systems with few hundred MB of RAM it'd be nice to not
> hold all the stuff in kernel memory, I'd think.

Later in this thread Ping explained pretty well the challenges here,
that sums exactly what I'm worried about.

> We have a rule against putting FW as a static table in the driver
> source, right? Or did we abandon that? Isn't this fundamentally similar?

My understanding is that these are just initialisation values for
hardware, not executable code. (Ping, please correct me if I
misunderstood.) So that's why I thought these are ok to have in kernel.
So I took practicality over elegance here.

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

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

2023-05-01 22:27:21