Message ID | 20230421104726.800BCC433D2@smtp.kernel.org (mailing list archive) |
---|---|
State | Accepted |
Commit | ca288965801572fe41386560d4e6c5cc0e5cc56d |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | pull-request: wireless-next-2023-04-21 | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Pull request for net-next |
netdev/build_32bit | fail | Errors and warnings before: 237 this patch: 238 |
netdev/build_clang | success | Errors and warnings before: 63 this patch: 61 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/verify_fixes | success | Fixes tag looks correct |
netdev/build_allmodconfig_warn | fail | Errors and warnings before: 239 this patch: 240 |
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
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/20221116222339.54052a83@kernel.org/ for example. Would that not work? The huge register tables in C sources makes it look like a Windows drivers :(
Hello: This pull request was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: 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!
Jakub Kicinski <kuba@kernel.org> 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.
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, April 21, 2023 10:54 PM > To: Kalle Valo <kvalo@kernel.org> > Cc: netdev@vger.kernel.org; linux-wireless@vger.kernel.org > 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/20221116222339.54052a83@kernel.org/ > 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
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 <pkshih@realtek.com> wrote: > > > >> -----Original Message----- >> From: Jakub Kicinski <kuba@kernel.org> >> Sent: Friday, April 21, 2023 10:54 PM >> To: Kalle Valo <kvalo@kernel.org> >> Cc: netdev@vger.kernel.org; linux-wireless@vger.kernel.org >> 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/20221116222339.54052a83@kernel.org/ >> 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 >
Hi Gregg, (no top posting for wireless mailing list, so I move your post to bottom) > -----Original Message----- > From: Gregg Wonderly <greggwonderly@seqtechllc.com> > Sent: Tuesday, April 25, 2023 12:15 PM > To: Ping-Ke Shih <pkshih@realtek.com> > Cc: Jakub Kicinski <kuba@kernel.org>; Kalle Valo <kvalo@kernel.org>; netdev@vger.kernel.org; > linux-wireless@vger.kernel.org > Subject: Re: pull-request: wireless-next-2023-04-21 > > > On Apr 24, 2023, at 9:41 PM, Ping-Ke Shih <pkshih@realtek.com> wrote: > > > > > > > >> -----Original Message----- > >> From: Jakub Kicinski <kuba@kernel.org> > >> Sent: Friday, April 21, 2023 10:54 PM > >> To: Kalle Valo <kvalo@kernel.org> > >> Cc: netdev@vger.kernel.org; linux-wireless@vger.kernel.org > >> 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/20221116222339.54052a83@kernel.org/ > >> 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
Ping-Ke Shih <pkshih@realtek.com> writes: >> -----Original Message----- >> From: Jakub Kicinski <kuba@kernel.org> >> Sent: Friday, April 21, 2023 10:54 PM >> To: Kalle Valo <kvalo@kernel.org> >> Cc: netdev@vger.kernel.org; linux-wireless@vger.kernel.org >> 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/20221116222339.54052a83@kernel.org/ >> 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.
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.
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
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 <kuba@kernel.org> 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
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Wednesday, April 26, 2023 1:09 AM > To: Jakub Kicinski <kuba@kernel.org>; Kalle Valo <kvalo@kernel.org> > Cc: Ping-Ke Shih <pkshih@realtek.com>; netdev@vger.kernel.org; linux-wireless@vger.kernel.org > 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
> -----Original Message----- > From: Ping-Ke Shih <pkshih@realtek.com> > Sent: Wednesday, April 26, 2023 11:16 AM > To: Johannes Berg <johannes@sipsolutions.net>; Jakub Kicinski <kuba@kernel.org>; Kalle Valo > <kvalo@kernel.org> > Cc: netdev@vger.kernel.org; linux-wireless@vger.kernel.org > Subject: RE: pull-request: wireless-next-2023-04-21 > > > -----Original Message----- > > From: Johannes Berg <johannes@sipsolutions.net> > > Sent: Wednesday, April 26, 2023 1:09 AM > > To: Jakub Kicinski <kuba@kernel.org>; Kalle Valo <kvalo@kernel.org> > > Cc: Ping-Ke Shih <pkshih@realtek.com>; netdev@vger.kernel.org; linux-wireless@vger.kernel.org > > 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.
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
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Wednesday, April 26, 2023 4:25 PM > To: Ping-Ke Shih <pkshih@realtek.com>; Jakub Kicinski <kuba@kernel.org>; Kalle Valo <kvalo@kernel.org> > Cc: netdev@vger.kernel.org; linux-wireless@vger.kernel.org > 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
Jakub Kicinski <kuba@kernel.org> 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/
Ping-Ke Shih <pkshih@realtek.com> 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.
Jakub Kicinski <kuba@kernel.org> 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.
On Fri, 28 Apr 2023 13:43:16 +0300 Kalle Valo wrote: > > 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. Alright, I'll try to make someone else do this outside of wireless, and come back with real life experience disproving the concerns :)