* [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA
@ 2019-08-22 13:35 Colin King
2019-08-22 16:03 ` Larry Finger
2019-08-22 16:55 ` Johannes Berg
0 siblings, 2 replies; 7+ messages in thread
From: Colin King @ 2019-08-22 13:35 UTC (permalink / raw)
To: Hauke Mehrtens, Rafał Miłecki, linux-wireless
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
An earlier commit re-worked the setting of the bitmask and is now
assigning v with some bit flags rather than bitwise or-ing them
into v, consequently the earlier bit-settings of v are being lost.
Fix this by replacing an assignment with the bitwise or instead.
Addresses-Coverity: ("Unused value")
Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/bcma/driver_pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c
index f499a469e66d..d219ee947c07 100644
--- a/drivers/bcma/driver_pci.c
+++ b/drivers/bcma/driver_pci.c
@@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci *pc, u16 device, u8 address)
v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD);
}
- v = BCMA_CORE_PCI_MDIODATA_START;
+ v |= BCMA_CORE_PCI_MDIODATA_START;
v |= BCMA_CORE_PCI_MDIODATA_READ;
v |= BCMA_CORE_PCI_MDIODATA_TA;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA 2019-08-22 13:35 [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA Colin King @ 2019-08-22 16:03 ` Larry Finger 2019-08-22 16:11 ` Colin Ian King 2019-08-22 16:55 ` Johannes Berg 1 sibling, 1 reply; 7+ messages in thread From: Larry Finger @ 2019-08-22 16:03 UTC (permalink / raw) To: Colin King, Hauke Mehrtens, Rafał Miłecki, linux-wireless Cc: kernel-janitors, linux-kernel On 8/22/19 8:35 AM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > An earlier commit re-worked the setting of the bitmask and is now > assigning v with some bit flags rather than bitwise or-ing them > into v, consequently the earlier bit-settings of v are being lost. > Fix this by replacing an assignment with the bitwise or instead. > > Addresses-Coverity: ("Unused value") > Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/bcma/driver_pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c > index f499a469e66d..d219ee947c07 100644 > --- a/drivers/bcma/driver_pci.c > +++ b/drivers/bcma/driver_pci.c > @@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci *pc, u16 device, u8 address) > v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); > } > > - v = BCMA_CORE_PCI_MDIODATA_START; > + v |= BCMA_CORE_PCI_MDIODATA_START; > v |= BCMA_CORE_PCI_MDIODATA_READ; > v |= BCMA_CORE_PCI_MDIODATA_TA; I'm not sure the "Fixes" attribute is correct. The changes for this section in commit 2be25cac8402 are - v = (1 << 30); /* Start of Transaction */ - v |= (1 << 28); /* Write Transaction */ - v |= (1 << 17); /* Turnaround */ - v |= (0x1F << 18); + v = BCMA_CORE_PCI_MDIODATA_START; + v |= BCMA_CORE_PCI_MDIODATA_WRITE; + v |= (BCMA_CORE_PCI_MDIODATA_DEV_ADDR << + BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF); + v |= (BCMA_CORE_PCI_MDIODATA_BLK_ADDR << + BCMA_CORE_PCI_MDIODATA_REGADDR_SHF); + v |= BCMA_CORE_PCI_MDIODATA_TA; Because the code has done quite a bit of work on v just above this section, I agree that this is likely an error, but that error happened in an earlier commit. Thus 2be25cac8402 did not introduce the error, merely copied it. Has this change been tested? Larry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA 2019-08-22 16:03 ` Larry Finger @ 2019-08-22 16:11 ` Colin Ian King 2019-08-22 16:38 ` Larry Finger 2019-08-25 19:59 ` Rafał Miłecki 0 siblings, 2 replies; 7+ messages in thread From: Colin Ian King @ 2019-08-22 16:11 UTC (permalink / raw) To: Larry Finger, Hauke Mehrtens, Rafał Miłecki, linux-wireless Cc: kernel-janitors, linux-kernel On 22/08/2019 17:03, Larry Finger wrote: > On 8/22/19 8:35 AM, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> An earlier commit re-worked the setting of the bitmask and is now >> assigning v with some bit flags rather than bitwise or-ing them >> into v, consequently the earlier bit-settings of v are being lost. >> Fix this by replacing an assignment with the bitwise or instead. >> >> Addresses-Coverity: ("Unused value") >> Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them") >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/bcma/driver_pci.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c >> index f499a469e66d..d219ee947c07 100644 >> --- a/drivers/bcma/driver_pci.c >> +++ b/drivers/bcma/driver_pci.c >> @@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci >> *pc, u16 device, u8 address) >> v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); >> } >> - v = BCMA_CORE_PCI_MDIODATA_START; >> + v |= BCMA_CORE_PCI_MDIODATA_START; >> v |= BCMA_CORE_PCI_MDIODATA_READ; >> v |= BCMA_CORE_PCI_MDIODATA_TA; > > I'm not sure the "Fixes" attribute is correct. > > The changes for this section in commit 2be25cac8402 are > > - v = (1 << 30); /* Start of Transaction */ > - v |= (1 << 28); /* Write Transaction */ > - v |= (1 << 17); /* Turnaround */ > - v |= (0x1F << 18); > + v = BCMA_CORE_PCI_MDIODATA_START; > + v |= BCMA_CORE_PCI_MDIODATA_WRITE; > + v |= (BCMA_CORE_PCI_MDIODATA_DEV_ADDR << > + BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF); > + v |= (BCMA_CORE_PCI_MDIODATA_BLK_ADDR << > + BCMA_CORE_PCI_MDIODATA_REGADDR_SHF); > + v |= BCMA_CORE_PCI_MDIODATA_TA; > > Because the code has done quite a bit of work on v just above this > section, I agree that this is likely an error, but that error happened > in an earlier commit. Thus 2be25cac8402 did not introduce the error, > merely copied it. Ugh, this goes back further. I didn't spot that. I'm less confident of what the correct settings should be now. > > Has this change been tested? Afraid not, I don't have the H/W. > > Larry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA 2019-08-22 16:11 ` Colin Ian King @ 2019-08-22 16:38 ` Larry Finger 2019-08-27 7:58 ` Colin Ian King 2019-08-25 19:59 ` Rafał Miłecki 1 sibling, 1 reply; 7+ messages in thread From: Larry Finger @ 2019-08-22 16:38 UTC (permalink / raw) To: Colin Ian King, Hauke Mehrtens, Rafał Miłecki, linux-wireless Cc: kernel-janitors, linux-kernel On 8/22/19 11:11 AM, Colin Ian King wrote: > On 22/08/2019 17:03, Larry Finger wrote: >> On 8/22/19 8:35 AM, Colin King wrote: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> An earlier commit re-worked the setting of the bitmask and is now >>> assigning v with some bit flags rather than bitwise or-ing them >>> into v, consequently the earlier bit-settings of v are being lost. >>> Fix this by replacing an assignment with the bitwise or instead. >>> >>> Addresses-Coverity: ("Unused value") >>> Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them") >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>> --- >>> drivers/bcma/driver_pci.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c >>> index f499a469e66d..d219ee947c07 100644 >>> --- a/drivers/bcma/driver_pci.c >>> +++ b/drivers/bcma/driver_pci.c >>> @@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci >>> *pc, u16 device, u8 address) >>> v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); >>> } >>> - v = BCMA_CORE_PCI_MDIODATA_START; >>> + v |= BCMA_CORE_PCI_MDIODATA_START; >>> v |= BCMA_CORE_PCI_MDIODATA_READ; >>> v |= BCMA_CORE_PCI_MDIODATA_TA; >> >> I'm not sure the "Fixes" attribute is correct. >> >> The changes for this section in commit 2be25cac8402 are >> >> - v = (1 << 30); /* Start of Transaction */ >> - v |= (1 << 28); /* Write Transaction */ >> - v |= (1 << 17); /* Turnaround */ >> - v |= (0x1F << 18); >> + v = BCMA_CORE_PCI_MDIODATA_START; >> + v |= BCMA_CORE_PCI_MDIODATA_WRITE; >> + v |= (BCMA_CORE_PCI_MDIODATA_DEV_ADDR << >> + BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF); >> + v |= (BCMA_CORE_PCI_MDIODATA_BLK_ADDR << >> + BCMA_CORE_PCI_MDIODATA_REGADDR_SHF); >> + v |= BCMA_CORE_PCI_MDIODATA_TA; >> >> Because the code has done quite a bit of work on v just above this >> section, I agree that this is likely an error, but that error happened >> in an earlier commit. Thus 2be25cac8402 did not introduce the error, >> merely copied it. > > Ugh, this goes back further. I didn't spot that. I'm less confident of > what the correct settings should be now. > >> >> Has this change been tested? > > Afraid not, I don't have the H/W. I admit that I looked at this only because I found it hard to believe that the collective wisdom of the list would have missed the usage of "=" instead of "|=". At least that test was passed. :) Larry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA 2019-08-22 16:38 ` Larry Finger @ 2019-08-27 7:58 ` Colin Ian King 0 siblings, 0 replies; 7+ messages in thread From: Colin Ian King @ 2019-08-27 7:58 UTC (permalink / raw) To: Larry Finger, Hauke Mehrtens, Rafał Miłecki, linux-wireless Cc: kernel-janitors, linux-kernel On 22/08/2019 17:38, Larry Finger wrote: > On 8/22/19 11:11 AM, Colin Ian King wrote: >> On 22/08/2019 17:03, Larry Finger wrote: >>> On 8/22/19 8:35 AM, Colin King wrote: >>>> From: Colin Ian King <colin.king@canonical.com> >>>> >>>> An earlier commit re-worked the setting of the bitmask and is now >>>> assigning v with some bit flags rather than bitwise or-ing them >>>> into v, consequently the earlier bit-settings of v are being lost. >>>> Fix this by replacing an assignment with the bitwise or instead. >>>> >>>> Addresses-Coverity: ("Unused value") >>>> Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them") >>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>> --- >>>> drivers/bcma/driver_pci.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c >>>> index f499a469e66d..d219ee947c07 100644 >>>> --- a/drivers/bcma/driver_pci.c >>>> +++ b/drivers/bcma/driver_pci.c >>>> @@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci >>>> *pc, u16 device, u8 address) >>>> v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); >>>> } >>>> - v = BCMA_CORE_PCI_MDIODATA_START; >>>> + v |= BCMA_CORE_PCI_MDIODATA_START; >>>> v |= BCMA_CORE_PCI_MDIODATA_READ; >>>> v |= BCMA_CORE_PCI_MDIODATA_TA; >>> >>> I'm not sure the "Fixes" attribute is correct. >>> >>> The changes for this section in commit 2be25cac8402 are >>> >>> - v = (1 << 30); /* Start of Transaction */ >>> - v |= (1 << 28); /* Write Transaction */ >>> - v |= (1 << 17); /* Turnaround */ >>> - v |= (0x1F << 18); >>> + v = BCMA_CORE_PCI_MDIODATA_START; >>> + v |= BCMA_CORE_PCI_MDIODATA_WRITE; >>> + v |= (BCMA_CORE_PCI_MDIODATA_DEV_ADDR << >>> + BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF); >>> + v |= (BCMA_CORE_PCI_MDIODATA_BLK_ADDR << >>> + BCMA_CORE_PCI_MDIODATA_REGADDR_SHF); >>> + v |= BCMA_CORE_PCI_MDIODATA_TA; >>> >>> Because the code has done quite a bit of work on v just above this >>> section, I agree that this is likely an error, but that error happened >>> in an earlier commit. Thus 2be25cac8402 did not introduce the error, >>> merely copied it. I did a second look at Larry's comments above and realized the code he is referring to is in bcma_pcie_mdio_set_phy which is OK Instead, the code I'm fixing is in bcma_pcie_mdio_read, which was broken by commit 2be25cac8402fab56bb51166f464d1b420bcf744 if (pc->core->id.rev >= 10) { max_retries = 200; bcma_pcie_mdio_set_phy(pc, device); + v = (BCMA_CORE_PCI_MDIODATA_DEV_ADDR << + BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF); + v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF); + } else { + v = (device << BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF_OLD); + v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); } - v = (1 << 30); /* Start of Transaction */ - v |= (1 << 29); /* Read Transaction */ - v |= (1 << 17); /* Turnaround */ - if (pc->core->id.rev < 10) - v |= (u32)device << 22; - v |= (u32)address << 18; - pcicore_write32(pc, mdio_data, v); + v = BCMA_CORE_PCI_MDIODATA_START; + v |= BCMA_CORE_PCI_MDIODATA_READ; + v |= BCMA_CORE_PCI_MDIODATA_TA; + + pcicore_write32(pc, BCMA_CORE_PCI_MDIO_DATA, v); >> >> Ugh, this goes back further. I didn't spot that. I'm less confident of >> what the correct settings should be now. >> >>> >>> Has this change been tested? >> >> Afraid not, I don't have the H/W. > > I admit that I looked at this only because I found it hard to believe > that the collective wisdom of the list would have missed the usage of > "=" instead of "|=". At least that test was passed. :) Maybe not after all :-) > > Larry > I'll send a V2 with the write fix, and the same fixes commit sha ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA 2019-08-22 16:11 ` Colin Ian King 2019-08-22 16:38 ` Larry Finger @ 2019-08-25 19:59 ` Rafał Miłecki 1 sibling, 0 replies; 7+ messages in thread From: Rafał Miłecki @ 2019-08-25 19:59 UTC (permalink / raw) To: Colin Ian King Cc: Larry Finger, Hauke Mehrtens, linux-wireless, kernel-janitors, Linux Kernel Mailing List On Thu, 22 Aug 2019 at 18:11, Colin Ian King <colin.king@canonical.com> wrote: > On 22/08/2019 17:03, Larry Finger wrote: > > On 8/22/19 8:35 AM, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> An earlier commit re-worked the setting of the bitmask and is now > >> assigning v with some bit flags rather than bitwise or-ing them > >> into v, consequently the earlier bit-settings of v are being lost. > >> Fix this by replacing an assignment with the bitwise or instead. > >> > >> Addresses-Coverity: ("Unused value") > >> Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them") > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >> --- > >> drivers/bcma/driver_pci.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c > >> index f499a469e66d..d219ee947c07 100644 > >> --- a/drivers/bcma/driver_pci.c > >> +++ b/drivers/bcma/driver_pci.c > >> @@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci > >> *pc, u16 device, u8 address) > >> v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); > >> } > >> - v = BCMA_CORE_PCI_MDIODATA_START; > >> + v |= BCMA_CORE_PCI_MDIODATA_START; > >> v |= BCMA_CORE_PCI_MDIODATA_READ; > >> v |= BCMA_CORE_PCI_MDIODATA_TA; > > > > I'm not sure the "Fixes" attribute is correct. > > > > The changes for this section in commit 2be25cac8402 are > > > > - v = (1 << 30); /* Start of Transaction */ > > - v |= (1 << 28); /* Write Transaction */ > > - v |= (1 << 17); /* Turnaround */ > > - v |= (0x1F << 18); > > + v = BCMA_CORE_PCI_MDIODATA_START; > > + v |= BCMA_CORE_PCI_MDIODATA_WRITE; > > + v |= (BCMA_CORE_PCI_MDIODATA_DEV_ADDR << > > + BCMA_CORE_PCI_MDIODATA_DEVADDR_SHF); > > + v |= (BCMA_CORE_PCI_MDIODATA_BLK_ADDR << > > + BCMA_CORE_PCI_MDIODATA_REGADDR_SHF); > > + v |= BCMA_CORE_PCI_MDIODATA_TA; > > > > Because the code has done quite a bit of work on v just above this > > section, I agree that this is likely an error, but that error happened > > in an earlier commit. Thus 2be25cac8402 did not introduce the error, > > merely copied it. > > Ugh, this goes back further. I didn't spot that. I'm less confident of > what the correct settings should be now. > > > > > Has this change been tested? > > Afraid not, I don't have the H/W. Please send V2 with updated commit message (Fixes tag) + bcma_pcie_mdio_write fixed. I'll try to test it. -- Rafał ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA 2019-08-22 13:35 [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA Colin King 2019-08-22 16:03 ` Larry Finger @ 2019-08-22 16:55 ` Johannes Berg 1 sibling, 0 replies; 7+ messages in thread From: Johannes Berg @ 2019-08-22 16:55 UTC (permalink / raw) To: Colin King, Hauke Mehrtens, Rafał Miłecki, linux-wireless Cc: kernel-janitors, linux-kernel On Thu, 2019-08-22 at 14:35 +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > An earlier commit re-worked the setting of the bitmask and is now > assigning v with some bit flags rather than bitwise or-ing them > into v, consequently the earlier bit-settings of v are being lost. > Fix this by replacing an assignment with the bitwise or instead. > > Addresses-Coverity: ("Unused value") > Fixes: 2be25cac8402 ("bcma: add constants for PCI and use them") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/bcma/driver_pci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/bcma/driver_pci.c b/drivers/bcma/driver_pci.c > index f499a469e66d..d219ee947c07 100644 > --- a/drivers/bcma/driver_pci.c > +++ b/drivers/bcma/driver_pci.c > @@ -78,7 +78,7 @@ static u16 bcma_pcie_mdio_read(struct bcma_drv_pci *pc, u16 device, u8 address) > v |= (address << BCMA_CORE_PCI_MDIODATA_REGADDR_SHF_OLD); > } > > - v = BCMA_CORE_PCI_MDIODATA_START; > + v |= BCMA_CORE_PCI_MDIODATA_START; The same bug/issue is in bcma_pcie_mdio_write() btw. It *seems* correct to me - otherwise the "address" parameter to the function is entirely unused, which can't really be right. There are only two code paths that ever get here: * bcma_pcicore_serdes_workaround * bcma_core_pci_power_save The register at 0 is BCMA_CORE_PCI_CTL, which only has a few bits so even bad values written there by accident might not hurt much ... So it seems possible that it was just always broken. johannes ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-27 7:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-22 13:35 [PATCH] bcma: fix incorrect update of BCMA_CORE_PCI_MDIO_DATA Colin King 2019-08-22 16:03 ` Larry Finger 2019-08-22 16:11 ` Colin Ian King 2019-08-22 16:38 ` Larry Finger 2019-08-27 7:58 ` Colin Ian King 2019-08-25 19:59 ` Rafał Miłecki 2019-08-22 16:55 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).