15 messages, 6 authors, 2025-01-10 · open the first message on its own page
From: Ariel Otilibili <hidden>
Date: 2024-12-21 12:45:04
Hello, This series clears out the Coverity-IDs 1525307, 1269173, & 1575053. Thank you, Ariel Otilibili (2): rt2x00: Remove unusued value net/phy,neterion: Remove dead values drivers/net/ethernet/neterion/s2io.c | 2 -- drivers/net/phy/nxp-c45-tja11xx-macsec.c | 1 - drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 6 ------ 3 files changed, 9 deletions(-) -- 2.47.1
From: Ariel Otilibili <hidden>
Date: 2024-12-21 12:45:08
Coverity-ID: 1525307 Signed-off-by: Ariel Otilibili <redacted> --- 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; 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
From: Ariel Otilibili <hidden>
Date: 2024-12-21 12:45:14
Coverity-ID: 1269173, 1575053 Signed-off-by: Ariel Otilibili <redacted> --- drivers/net/ethernet/neterion/s2io.c | 2 -- drivers/net/phy/nxp-c45-tja11xx-macsec.c | 1 - 2 files changed, 3 deletions(-)
diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c
index f8016dc25e0a..4f89f9fd8043 100644
--- a/drivers/net/ethernet/neterion/s2io.c
+++ b/drivers/net/ethernet/neterion/s2io.c@@ -1969,8 +1969,6 @@ static void en_dis_err_alarms(struct s2io_nic *nic, u16 mask, int flag) MC_ERR_REG_ECC_ALL_DBL | PLL_LOCK_N, flag, &bar0->mc_err_mask); } - nic->general_int_mask = gen_int_mask; - /* Remove this line when alarm interrupts are enabled */ nic->general_int_mask = 0; }
diff --git a/drivers/net/phy/nxp-c45-tja11xx-macsec.c b/drivers/net/phy/nxp-c45-tja11xx-macsec.c
index 550ef08970f4..e15ab9ba2f50 100644
--- a/drivers/net/phy/nxp-c45-tja11xx-macsec.c
+++ b/drivers/net/phy/nxp-c45-tja11xx-macsec.c@@ -818,7 +818,6 @@ static void nxp_c45_rx_sc_update(struct phy_device *phydev, u32 cfg = 0; nxp_c45_macsec_read(phydev, MACSEC_RXSC_CFG, &cfg); - cfg &= ~MACSEC_RXSC_CFG_VF_MASK; cfg = phy_secy->secy->validate_frames << MACSEC_RXSC_CFG_VF_OFF; phydev_dbg(phydev, "validate frames %u\n",
--
2.47.1
From: Andrew Lunn <andrew@lunn.ch>
Date: 2024-12-21 15:06:15
On Sat, Dec 21, 2024 at 01:39:33PM +0100, Ariel Otilibili wrote:
Coverity-ID: 1269173, 1575053 Signed-off-by: Ariel Otilibili <redacted> --- drivers/net/ethernet/neterion/s2io.c | 2 -- drivers/net/phy/nxp-c45-tja11xx-macsec.c | 1 -
Please split this into a patch per driver.
2 files changed, 3 deletions(-)diff --git a/drivers/net/ethernet/neterion/s2io.c b/drivers/net/ethernet/neterion/s2io.c index f8016dc25e0a..4f89f9fd8043 100644 --- a/drivers/net/ethernet/neterion/s2io.c +++ b/drivers/net/ethernet/neterion/s2io.c@@ -1969,8 +1969,6 @@ static void en_dis_err_alarms(struct s2io_nic *nic, u16 mask, int flag) MC_ERR_REG_ECC_ALL_DBL | PLL_LOCK_N, flag, &bar0->mc_err_mask); } - nic->general_int_mask = gen_int_mask; - /* Remove this line when alarm interrupts are enabled */ nic->general_int_mask = 0;
So this change looks reasonable, but you need to update the comment, because it is now wrong.
}diff --git a/drivers/net/phy/nxp-c45-tja11xx-macsec.c b/drivers/net/phy/nxp-c45-tja11xx-macsec.c index 550ef08970f4..e15ab9ba2f50 100644 --- a/drivers/net/phy/nxp-c45-tja11xx-macsec.c +++ b/drivers/net/phy/nxp-c45-tja11xx-macsec.c@@ -818,7 +818,6 @@ static void nxp_c45_rx_sc_update(struct phy_device *phydev, u32 cfg = 0; nxp_c45_macsec_read(phydev, MACSEC_RXSC_CFG, &cfg); - cfg &= ~MACSEC_RXSC_CFG_VF_MASK; cfg = phy_secy->secy->validate_frames << MACSEC_RXSC_CFG_VF_OFF;
I can see why a static analysis tool would point this out, but you need to include some justification why your change is correct, and it is not a bug, maybe cfg should be written back after being masked? Or the assignment to cfg should actually be an |= not = ? Andrew
From: Stanislaw Gruszka <stf_xl@wp.pl>
Date: 2025-01-03 09:02:22
On Sat, Dec 21, 2024 at 01:39:32PM +0100, Ariel Otilibili wrote:
Coverity-ID: 1525307 Signed-off-by: Ariel Otilibili <redacted> --- 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
From: Daniel Golle <daniel@makrotopia.org>
Date: 2025-01-03 11:41:07
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:quoted
Coverity-ID: 1525307 Signed-off-by: Ariel Otilibili <redacted> --- 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.quoted
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
From: Stanislaw Gruszka <stf_xl@wp.pl>
Date: 2025-01-03 13:10:09
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:quoted
On Sat, Dec 21, 2024 at 01:39:32PM +0100, Ariel Otilibili wrote:quoted
Coverity-ID: 1525307 Signed-off-by: Ariel Otilibili <redacted> --- 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
quoted
Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> But Tomislav and Daniel, please check if this code is correct.quoted
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
From: Ariel Otilibili-Anieli <hidden>
Date: 2025-01-03 13:39:23
Hello Stanislaw, hello Daniel; happy new year, On Friday, January 03, 2025 14:10 CET, Stanislaw Gruszka [off-list ref] wrote:
On Fri, Jan 03, 2025 at 11:40:52AM +0000, Daniel Golle wrote:quoted
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? If so; then, I have nothing else to do.
Regards Stanislaw
From: Stanislaw Gruszka <stf_xl@wp.pl>
Date: 2025-01-04 10:38:02
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 [off-list ref] wrote:quoted
On Fri, Jan 03, 2025 at 11:40:52AM +0000, Daniel Golle wrote:quoted
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.quoted
Regards Stanislaw
From: Ariel Otilibili-Anieli <hidden>
Date: 2025-01-04 12:51:33
Hi Stanislaw, On Saturday, January 04, 2025 11:37 CET, Stanislaw Gruszka [off-list ref] wrote:
Hi On Fri, Jan 03, 2025 at 02:39:21PM +0100, Ariel Otilibili-Anieli wrote:quoted
On Friday, January 03, 2025 14:10 CET, Stanislaw Gruszka [off-list ref] 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 Stanislawquoted
If so; then, I have nothing else to do.
From: Daniel Golle <daniel@makrotopia.org>
Date: 2025-01-05 21:21:15
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
From: Ariel Otilibili-Anieli <hidden>
Date: 2025-01-06 07:23:41
Hi Daniel, hi Shiji, hi Stanislaw, On Sunday, January 05, 2025 22:21 CET, Daniel Golle [off-list ref] wrote:
H again, On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote:quoted
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
From: Stanislaw Gruszka <stf_xl@wp.pl>
Date: 2025-01-07 10:23:45
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 [off-list ref] wrote:quoted
H again, On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote:quoted
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
From: Jonas Gorski <jonas.gorski@gmail.com>
Date: 2025-01-07 11:01:15
Hi, On Mon, Jan 6, 2025 at 8:23 AM Ariel Otilibili-Anieli [off-list ref] wrote:
Hi Daniel, hi Shiji, hi Stanislaw, On Sunday, January 05, 2025 22:21 CET, Daniel Golle [off-list ref] wrote:quoted
H again, On Sat, Jan 04, 2025 at 01:51:25PM +0100, Ariel Otilibili-Anieli wrote:quoted
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
From: Kalle Valo <kvalo@kernel.org>
Date: 2025-01-10 13:12:22
Ariel Otilibili [off-list ref] 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 <redacted> 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 -- https://patchwork.kernel.org/project/linux-wireless/patch/20241221124445.1094460-2-ariel.otilibili-anieli@eurecom.fr/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches