| Message ID | 20241221124445.1094460-2-ariel.otilibili-anieli@eurecom.fr |
|---|---|
| State | New |
| Headers | show |
| Series | rt2x00,net/phy,neterion: Remove dead values | expand |
On Sat, Dec 21, 2024 at 01:39:32PM +0100, Ariel Otilibili wrote: > Coverity-ID: 1525307 > Signed-off-by: Ariel Otilibili <ariel.otilibili-anieli@eurecom.fr> > --- > drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > index 60c2a12e9d5e..e5f553a1ea24 100644 > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > @@ -8882,13 +8882,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) > > for (ch_idx = 0; ch_idx < 2; ch_idx = ch_idx + 1) { > if (ch_idx == 0) { > - rfval = rfb0r1 & (~0x3); > rfval = rfb0r1 | 0x1; I wonder if intention here was different, for example: rfval = rfb0r1 & (~0x3); rfval = rfval | 0x1; For me the patch looks ok - it does not change existing behaviour, since rfval is overwritten by second line anyway. Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> But Tomislav and Daniel, please check if this code is correct. > rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); > - rfval = rfb0r2 & (~0x33); > rfval = rfb0r2 | 0x11; > rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); > - rfval = rfb0r42 & (~0x50); > rfval = rfb0r42 | 0x10; > rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); > > @@ -8901,13 +8898,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) > > rt2800_bbp_dcoc_write(rt2x00dev, 1, 0x00); > } else { > - rfval = rfb0r1 & (~0x3); > rfval = rfb0r1 | 0x2; > rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); > - rfval = rfb0r2 & (~0x33); > rfval = rfb0r2 | 0x22; > rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); > - rfval = rfb0r42 & (~0x50); > rfval = rfb0r42 | 0x40; > rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); > > -- > 2.47.1 >
On Fri, Jan 03, 2025 at 09:55:40AM +0100, Stanislaw Gruszka wrote: > On Sat, Dec 21, 2024 at 01:39:32PM +0100, Ariel Otilibili wrote: > > Coverity-ID: 1525307 > > Signed-off-by: Ariel Otilibili <ariel.otilibili-anieli@eurecom.fr> > > --- > > drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 6 ------ > > 1 file changed, 6 deletions(-) > > > > diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > index 60c2a12e9d5e..e5f553a1ea24 100644 > > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > @@ -8882,13 +8882,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) > > > > for (ch_idx = 0; ch_idx < 2; ch_idx = ch_idx + 1) { > > if (ch_idx == 0) { > > - rfval = rfb0r1 & (~0x3); > > rfval = rfb0r1 | 0x1; > > I wonder if intention here was different, for example: > > rfval = rfb0r1 & (~0x3); > rfval = rfval | 0x1; > > For me the patch looks ok - it does not change existing behaviour, > since rfval is overwritten by second line anyway. I agree with the likely intention here, however, the vendor driver also comes with the dead code, see https://github.com/lixuande/rt2860v2/blob/master/files/rt2860v2/common/cmm_rf_cal.c#L2690 So this is certainly a bug in the vendor driver as well which got ported bug-by-bug to rt2x00... Not sure what is the best thing to do in this case. > > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> > > But Tomislav and Daniel, please check if this code is correct. > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); > > - rfval = rfb0r2 & (~0x33); > > rfval = rfb0r2 | 0x11; > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); > > - rfval = rfb0r42 & (~0x50); > > rfval = rfb0r42 | 0x10; > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); > > > > @@ -8901,13 +8898,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) > > > > rt2800_bbp_dcoc_write(rt2x00dev, 1, 0x00); > > } else { > > - rfval = rfb0r1 & (~0x3); > > rfval = rfb0r1 | 0x2; > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); > > - rfval = rfb0r2 & (~0x33); > > rfval = rfb0r2 | 0x22; > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); > > - rfval = rfb0r42 & (~0x50); > > rfval = rfb0r42 | 0x40; > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); > > > > -- > > 2.47.1 > > >
On Fri, Jan 03, 2025 at 11:40:52AM +0000, Daniel Golle wrote: > On Fri, Jan 03, 2025 at 09:55:40AM +0100, Stanislaw Gruszka wrote: > > On Sat, Dec 21, 2024 at 01:39:32PM +0100, Ariel Otilibili wrote: > > > Coverity-ID: 1525307 > > > Signed-off-by: Ariel Otilibili <ariel.otilibili-anieli@eurecom.fr> > > > --- > > > drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 6 ------ > > > 1 file changed, 6 deletions(-) > > > > > > diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > > index 60c2a12e9d5e..e5f553a1ea24 100644 > > > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > > > @@ -8882,13 +8882,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) > > > > > > for (ch_idx = 0; ch_idx < 2; ch_idx = ch_idx + 1) { > > > if (ch_idx == 0) { > > > - rfval = rfb0r1 & (~0x3); > > > rfval = rfb0r1 | 0x1; > > > > I wonder if intention here was different, for example: > > > > rfval = rfb0r1 & (~0x3); > > rfval = rfval | 0x1; > > > > For me the patch looks ok - it does not change existing behaviour, > > since rfval is overwritten by second line anyway. > > I agree with the likely intention here, however, the vendor driver > also comes with the dead code, see > https://github.com/lixuande/rt2860v2/blob/master/files/rt2860v2/common/cmm_rf_cal.c#L2690 > > So this is certainly a bug in the vendor driver as well which got ported > bug-by-bug to rt2x00... Not sure what is the best thing to do in this > case. As this was already tested and match vendor driver I would prefer not to change behavior even if it looks suspicious. Regards Stanislaw > > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> > > > > But Tomislav and Daniel, please check if this code is correct. > > > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); > > > - rfval = rfb0r2 & (~0x33); > > > rfval = rfb0r2 | 0x11; > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); > > > - rfval = rfb0r42 & (~0x50); > > > rfval = rfb0r42 | 0x10; > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); > > > > > > @@ -8901,13 +8898,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) > > > > > > rt2800_bbp_dcoc_write(rt2x00dev, 1, 0x00); > > > } else { > > > - rfval = rfb0r1 & (~0x3); > > > rfval = rfb0r1 | 0x2; > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); > > > - rfval = rfb0r2 & (~0x33); > > > rfval = rfb0r2 | 0x22; > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); > > > - rfval = rfb0r42 & (~0x50); > > > rfval = rfb0r42 | 0x40; > > > rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); > > > > > > -- > > > 2.47.1 > > > > >
Hello Stanislaw, hello Daniel; happy new year, On Friday, January 03, 2025 14:10 CET, Stanislaw Gruszka <stf_xl@wp.pl> wrote: > On Fri, Jan 03, 2025 at 11:40:52AM +0000, Daniel Golle wrote: > > On Fri, Jan 03, 2025 at 09:55:40AM +0100, Stanislaw Gruszka wrote: > > > > I agree with the likely intention here, however, the vendor driver > > also comes with the dead code, see > > https://github.com/lixuande/rt2860v2/blob/master/files/rt2860v2/common/cmm_rf_cal.c#L2690 > > > > So this is certainly a bug in the vendor driver as well which got ported > > bug-by-bug to rt2x00... Not sure what is the best thing to do in this > > case. > > As this was already tested and match vendor driver I would prefer > not to change behavior even if it looks suspicious. Thanks for having looked into this; I much appreciate your feedback.
Hi On Fri, Jan 03, 2025 at 02:39:21PM +0100, Ariel Otilibili-Anieli wrote: > On Friday, January 03, 2025 14:10 CET, Stanislaw Gruszka <stf_xl@wp.pl> wrote: > > > On Fri, Jan 03, 2025 at 11:40:52AM +0000, Daniel Golle wrote: > > > On Fri, Jan 03, 2025 at 09:55:40AM +0100, Stanislaw Gruszka wrote: > > > > > > I agree with the likely intention here, however, the vendor driver > > > also comes with the dead code, see > > > https://github.com/lixuande/rt2860v2/blob/master/files/rt2860v2/common/cmm_rf_cal.c#L2690 > > > > > > So this is certainly a bug in the vendor driver as well which got ported > > > bug-by-bug to rt2x00... Not sure what is the best thing to do in this > > > case. > > > > As this was already tested and match vendor driver I would prefer > > not to change behavior even if it looks suspicious. > > Thanks for having looked into this; I much appreciate your feedback. > > From what you two said, I understand that the patch should remove the duplicate code, and not change the logic behind. > > Is this right? Yes. Regards Stanislaw > > If so; then, I have nothing else to do. > > > > Regards > > Stanislaw > > >
Hi Stanislaw, On Saturday, January 04, 2025 11:37 CET, Stanislaw Gruszka <stf_xl@wp.pl> wrote: > Hi > > On Fri, Jan 03, 2025 at 02:39:21PM +0100, Ariel Otilibili-Anieli wrote: > > On Friday, January 03, 2025 14:10 CET, Stanislaw Gruszka <stf_xl@wp.pl> wrote: > > > > > > Thanks for having looked into this; I much appreciate your feedback. > > > > From what you two said, I understand that the patch should remove the duplicate code, and not change the logic behind. > > > > Is this right? > > Yes. Great, then; thanks for having acked the patch as such. > > Regards > Stanislaw > > > > If so; then, I have nothing else to do. > >
H again,
On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote:
> Great, then; thanks for having acked the patch as such.
I just noticed that Shiji Yang had posted a series of patches for
OpenWrt which also addresses the same issue, however, instead of
removing the augmented assignment, it fixes it to the supposedly
originally intended way.
See
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/kernel/mac80211/patches/rt2x00/621-04-rt2x00-fix-register-operation-on-RXIQ-calibration.patch;h=aa6f9c437c6447831490588b2cead6919accda58;hb=5d583901657bdfbbf9fad77d9247872427aa5c99
I suppose this was tested together with the other changes of the same
series, so we may want to pick that instead.
Cheers
Daniel
Hi Daniel, hi Shiji, hi Stanislaw, On Sunday, January 05, 2025 22:21 CET, Daniel Golle <daniel@makrotopia.org> wrote: > H again, > > > On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote: > > Great, then; thanks for having acked the patch as such. > > I just noticed that Shiji Yang had posted a series of patches for > OpenWrt which also addresses the same issue, however, instead of > removing the augmented assignment, it fixes it to the supposedly > originally intended way. > > See > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/kernel/mac80211/patches/rt2x00/621-04-rt2x00-fix-register-operation-on-RXIQ-calibration.patch;h=aa6f9c437c6447831490588b2cead6919accda58;hb=5d583901657bdfbbf9fad77d9247872427aa5c99 > > I suppose this was tested together with the other changes of the same > series, so we may want to pick that instead. Thanks for having put some time into the research, Daniel; I looked into the openwrt archives for 2024, none of Shiji’s messages mentions that patch. Though, if you three agree, I will push a new series, modelled on that patch, and you as Suggested-by. Have a good week, Ariel > > > Cheers > > > Daniel >
Hi Ariel, On Mon, Jan 06, 2025 at 08:23:34AM +0100, Ariel Otilibili-Anieli wrote: > Hi Daniel, hi Shiji, hi Stanislaw, > > On Sunday, January 05, 2025 22:21 CET, Daniel Golle <daniel@makrotopia.org> wrote: > > > H again, > > > > > > On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote: > > > Great, then; thanks for having acked the patch as such. > > > > I just noticed that Shiji Yang had posted a series of patches for > > OpenWrt which also addresses the same issue, however, instead of > > removing the augmented assignment, it fixes it to the supposedly > > originally intended way. > > > > See > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/kernel/mac80211/patches/rt2x00/621-04-rt2x00-fix-register-operation-on-RXIQ-calibration.patch;h=aa6f9c437c6447831490588b2cead6919accda58;hb=5d583901657bdfbbf9fad77d9247872427aa5c99 > > > > I suppose this was tested together with the other changes of the same > > series, so we may want to pick that instead. > > Thanks for having put some time into the research, Daniel; I looked into the openwrt archives for 2024, none of Shiji’s messages mentions that patch. > > Though, if you three agree, I will push a new series, modelled on that patch, and you as Suggested-by. Please post that change. But to not mix it with patches against other drivers in the same series. (multiple rt2x00 patches in one patchset are ok). And please use "wifi: rt2x00:" as subject prefix. Thanks Stanislaw
Hi, On Mon, Jan 6, 2025 at 8:23 AM Ariel Otilibili-Anieli <Ariel.Otilibili-Anieli@eurecom.fr> wrote: > > Hi Daniel, hi Shiji, hi Stanislaw, > > On Sunday, January 05, 2025 22:21 CET, Daniel Golle <daniel@makrotopia.org> wrote: > > > H again, > > > > > > On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote: > > > Great, then; thanks for having acked the patch as such. > > > > I just noticed that Shiji Yang had posted a series of patches for > > OpenWrt which also addresses the same issue, however, instead of > > removing the augmented assignment, it fixes it to the supposedly > > originally intended way. > > > > See > > https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=package/kernel/mac80211/patches/rt2x00/621-04-rt2x00-fix-register-operation-on-RXIQ-calibration.patch;h=aa6f9c437c6447831490588b2cead6919accda58;hb=5d583901657bdfbbf9fad77d9247872427aa5c99 > > > > I suppose this was tested together with the other changes of the same > > series, so we may want to pick that instead. > > Thanks for having put some time into the research, Daniel; I looked into the openwrt archives for 2024, none of Shiji’s messages mentions that patch. You didn't find anything because these changes came in via a PR on github: https://github.com/openwrt/openwrt/pull/16845 :) OpenWrt accepts contributions both via email and PR on github. Best Regards, Jonas
Ariel Otilibili <ariel.otilibili-anieli@eurecom.fr> wrote: > The intention here is not clear but as this was already tested and matches > vendor driver it's better not to change behavior even if it looks suspicious. > So just remove the unused values. > > Coverity-ID: 1525307 > > Signed-off-by: Ariel Otilibili <ariel.otilibili-anieli@eurecom.fr> > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> > [kvalo@kernel.org: write commit message] Patch applied to wireless-next.git, thanks. 280c8b39050b wifi: rt2x00: Remove unused rfval values
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c index 60c2a12e9d5e..e5f553a1ea24 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c @@ -8882,13 +8882,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) for (ch_idx = 0; ch_idx < 2; ch_idx = ch_idx + 1) { if (ch_idx == 0) { - rfval = rfb0r1 & (~0x3); rfval = rfb0r1 | 0x1; rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); - rfval = rfb0r2 & (~0x33); rfval = rfb0r2 | 0x11; rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); - rfval = rfb0r42 & (~0x50); rfval = rfb0r42 | 0x10; rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval); @@ -8901,13 +8898,10 @@ static void rt2800_rxiq_calibration(struct rt2x00_dev *rt2x00dev) rt2800_bbp_dcoc_write(rt2x00dev, 1, 0x00); } else { - rfval = rfb0r1 & (~0x3); rfval = rfb0r1 | 0x2; rt2800_rfcsr_write_bank(rt2x00dev, 0, 1, rfval); - rfval = rfb0r2 & (~0x33); rfval = rfb0r2 | 0x22; rt2800_rfcsr_write_bank(rt2x00dev, 0, 2, rfval); - rfval = rfb0r42 & (~0x50); rfval = rfb0r42 | 0x40; rt2800_rfcsr_write_bank(rt2x00dev, 0, 42, rfval);
Coverity-ID: 1525307 Signed-off-by: Ariel Otilibili <ariel.otilibili-anieli@eurecom.fr> --- drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 6 ------ 1 file changed, 6 deletions(-)