| Message ID | CA+55aFzNJbM=ohXninJ06518e3a6E8vySg8GT6dx+Qovk1QW0Q@mail.gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On 01/12/2012 08:08 PM, Linus Torvalds wrote: > On Thu, Jan 12, 2012 at 11:00 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> I'll send out a patch that seems to get things to a working state for >> me. At least I have wireless connectivity again, I don't know if there >> are some other problems remaining. > > Ok, this is the patch that gets me going, and this is sent from the Macbook Air. > > NOTE! The BCMA confusion about the sprom still exists, but doesn't > seem to matter: That is because brcmsmac is not relying on the sprom logic provided by bcma. As Rafał indicated that is duplicated code so if we are to use bcma sprom functionality that needs to be fixed (after the merge window). > bcma: Core 0 found: ChipCommon (manuf 0x4BF, id 0x800, rev 0x22, class 0x0) > bcma: Core 1 found: IEEE 802.11 (manuf 0x4BF, id 0x812, rev 0x17, class 0x0) > bcma: Core 2 found: PCIe (manuf 0x4BF, id 0x820, rev 0x0F, class 0x0) > bcma: Found rev 6 PMU (capabilities 0x108C2606) > bcma: Unsupported SPROM revision: 255 > bcma: No SPROM available > bcma: Bus registered > .. > brcmsmac bcma0:0: mfg 4bf core 812 rev 23 class 0 irq 17 > brcmsmac: Found chip type AI (0x1381a8d8) > brcmsmac: Applying 43224B0+ WARs > bcma: Switched to core: 0x812 > ieee80211 phy0: Selected rate control algorithm 'minstrel_ht' > brcms_module_init: register returned 0 > > so this does seem to work, but there are clearly some issues still.. > > Linus That output look fine. The patch looks fine although you can use the new do_crc_check() function in otp_read_pci as well. Gr. AvS -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/1/12 Arend van Spriel <arend@broadcom.com>: > > That output look fine. The patch looks fine although you can use the new > do_crc_check() function in otp_read_pci as well. I'll leave that as a separate cleanup for somebody who has the hardware to test it. I committed the 16-bit read fix for now. But I'm currently also trying to work out why that macbook air no longer comes back from a suspend alive, and it looks like it may be another problem with that brcmsmac driver. The bisection is in its early stages yet, but it looks like it is coming in from the network merge, and nothing else looks relevant. Has suspend/resume been tested exhaustively with that driver? Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2012 09:27 PM, Linus Torvalds wrote: > 2012/1/12 Arend van Spriel <arend@broadcom.com>: >> >> That output look fine. The patch looks fine although you can use the new >> do_crc_check() function in otp_read_pci as well. > > I'll leave that as a separate cleanup for somebody who has the > hardware to test it. I committed the 16-bit read fix for now. Ah. yet another hint :-p > But I'm currently also trying to work out why that macbook air no > longer comes back from a suspend alive, and it looks like it may be > another problem with that brcmsmac driver. The bisection is in its > early stages yet, but it looks like it is coming in from the network > merge, and nothing else looks relevant. > > Has suspend/resume been tested exhaustively with that driver? > > Linus > BCMA introduced suspend/resume after the BCMA changes in brcmsmac so there may still be issues. Gr. AvS -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 12, 2012 at 12:27 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > But I'm currently also trying to work out why that macbook air no > longer comes back from a suspend alive, and it looks like it may be > another problem with that brcmsmac driver. The bisection is in its > early stages yet, but it looks like it is coming in from the network > merge, and nothing else looks relevant. Ugh. This is nasty to bisect, because it goes back to the pre-3.2 days that didn't support graphics properly on that Macbook Air either. So I've been having to work around not just the "wireless doesn't work", but also the "graphics doesn't work" issue. But after lots of nasty bisection problems and a few false starts, it definitely looks like the brcmsmac driver. I don't know exactly which commit, but it's all in network drivers now, and the only network driver on this machine is the brcmsmac one. Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: > On Thu, Jan 12, 2012 at 12:27 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> But I'm currently also trying to work out why that macbook air no >> longer comes back from a suspend alive, and it looks like it may be >> another problem with that brcmsmac driver. The bisection is in its >> early stages yet, but it looks like it is coming in from the network >> merge, and nothing else looks relevant. > > Ugh. This is nasty to bisect, because it goes back to the pre-3.2 days > that didn't support graphics properly on that Macbook Air either. So > I've been having to work around not just the "wireless doesn't work", > but also the "graphics doesn't work" issue. > > But after lots of nasty bisection problems and a few false starts, it > definitely looks like the brcmsmac driver. I don't know exactly which > commit, but it's all in network drivers now, and the only network > driver on this machine is the brcmsmac one. Make sure you have commit 775ab52142b02237a54184238e922251c59a2b5c Author: Rafał Miłecki <zajec5@gmail.com> Date: Fri Dec 9 22:16:07 2011 +0100 bcma: support for suspend and resume applied. I believe this patch already has hit your tree, but maybe because of bisecting you are at some old commit without this patch.
2012/1/12 Rafał Miłecki <zajec5@gmail.com>: > > Make sure you have > > commit 775ab52142b02237a54184238e922251c59a2b5c > Author: Rafał Miłecki <zajec5@gmail.com> > Date: Fri Dec 9 22:16:07 2011 +0100 > > bcma: support for suspend and resume > > applied. I believe this patch already has hit your tree, but maybe > because of bisecting you are at some old commit without this patch. That one is *not* sufficient. Current -git doesn't suspend/resume. Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 12 stycznia 2012 23:45 użytkownik Linus Torvalds <torvalds@linux-foundation.org> napisał: > 2012/1/12 Rafał Miłecki <zajec5@gmail.com>: >> >> Make sure you have >> >> commit 775ab52142b02237a54184238e922251c59a2b5c >> Author: Rafał Miłecki <zajec5@gmail.com> >> Date: Fri Dec 9 22:16:07 2011 +0100 >> >> bcma: support for suspend and resume >> >> applied. I believe this patch already has hit your tree, but maybe >> because of bisecting you are at some old commit without this patch. > > That one is *not* sufficient. Current -git doesn't suspend/resume. Forgive me if it was already said, but I didn't see it. Have you tried booting with bcma & brcmsmac blacklisted? Does suspend&resume work then? Have you tried blacklisting just brcmsmac (letting bcma load)? Does s&r work then?
2012/1/12 Rafał Miłecki <zajec5@gmail.com>: > > Have you tried booting with bcma & brcmsmac blacklisted? Does > suspend&resume work then? > > Have you tried blacklisting just brcmsmac (letting bcma load)? Does > s&r work then? If I unload brcmsmac, I can suspend/resume. Once. It can't suspend a second time. I did see some message flash about "does not have a release() function", but don't know if that was bcma or something else. I do notice that both the bcma and suspend/resume seems quite broken. It's using the legacy suspend/resume stuff and does the PCI resume on its own (with no matching suspend!). That *really* isn't a good idea these days. The way to do it these days is to have a struct dev_pm_ops embedded in the struct pci_driver (".driver.pm"), and let the PCI layer handle all the generic PCI suspend/resume details - you only handle the device-specific ones (ie in this case suspending/resuming the bcma bus itself). The generic PCI layer will do all the PCI stuff correctly, including all the nasty races with shared interrupts etc. In a way that no driver ever got it right. And it simplifies the driver too. And the brcms driver does suspend/resume *completely* wrong, and seems to actually re-suspend and re-resume the PCI device. I'm surprised it has ever worked for anybody. It certainly doesn't work for me. Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: > > I'm surprised it has ever worked for anybody. It certainly doesn't work for me. So I can suspend the bcma driver on its own until the cows come home. But after I have suspended the bcma driver even once, just doing a "modprobe brcmsmac" will hang the machine hard. Dunno where, but this is probably the same thing as "hangs on resume". Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: > 2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: >> >> I'm surprised it has ever worked for anybody. It certainly doesn't work for me. > > So I can suspend the bcma driver on its own until the cows come home. > > But after I have suspended the bcma driver even once, just doing a > "modprobe brcmsmac" will hang the machine hard. Dunno where, but this > is probably the same thing as "hangs on resume". Guys, has suspend/resume with the bcma interface been tested AT ALL? The suspend/resume fields of "struct bcma_driver" are COMPLETELY UNUSED. The only place in the kernel that uses them is the brcmsmac driver that does this write-only assignment: .suspend = brcms_suspend, .resume = brcms_resume, nothing else uses them. NOTHING. I tested by just removing the fields and compiling the bcma subsystem, just in case there was something really subtle that I was missing and was hidden through some magic hidden approach. But no. Seriously - how was something that isn't even connected ever supposed to work at all? And why was the BCMA conversion of that driver sent up-stream if something as fundamental as suspend/resume had never been done, and didn't actually work? What am I missing now? How the hell can this ever have worked for ANYBODY? What kind of f*&*ing sick joke is this all? Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
W dniu 13 stycznia 2012 06:34 użytkownik Linus Torvalds <torvalds@linux-foundation.org> napisał: > 2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: >> 2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: >>> >>> I'm surprised it has ever worked for anybody. It certainly doesn't work for me. >> >> So I can suspend the bcma driver on its own until the cows come home. >> >> But after I have suspended the bcma driver even once, just doing a >> "modprobe brcmsmac" will hang the machine hard. Dunno where, but this >> is probably the same thing as "hangs on resume". > > Guys, has suspend/resume with the bcma interface been tested AT ALL? > > The suspend/resume fields of "struct bcma_driver" are COMPLETELY > UNUSED. The only place in the kernel that uses them is the brcmsmac > driver that does this write-only assignment: > > .suspend = brcms_suspend, > .resume = brcms_resume, > > nothing else uses them. NOTHING. I tested by just removing the fields > and compiling the bcma subsystem, just in case there was something > really subtle that I was missing and was hidden through some magic > hidden approach. But no. > > Seriously - how was something that isn't even connected ever supposed > to work at all? And why was the BCMA conversion of that driver sent > up-stream if something as fundamental as suspend/resume had never been > done, and didn't actually work? > > What am I missing now? How the hell can this ever have worked for > ANYBODY? What kind of f*&*ing sick joke is this all? The suspend&resume wasn't implemented for some time because my PC doesn't s&r. And I don't have access to notebook with mini PCIe slot. I've implemented support for s&r in bcma when I got to open my Sony VAIO to replace A/C power slot. It was one time I was able to change WiFi card in my notebook which has really-ugly-hidden mini PCIe slot. S&r was working fine for me with bcma&b43 after writing that patch! That includes suspending and resuming multiple times. And tests were done with the same card you're using. The lock up on (resume|loading brcmsmac) means bus wasn't initialized correctly after resume. It does not have to be brcmsmac bug. We're accessing some registers before they're ready. Linus: can you do one trivial test for me? Please simply try unloading bcma before suspending. Then resume and load bcma and brcmsmac. Does it still lockup your machine?
W dniu 13 stycznia 2012 07:50 użytkownik Rafał Miłecki <zajec5@gmail.com> napisał: > W dniu 13 stycznia 2012 06:34 użytkownik Linus Torvalds > <torvalds@linux-foundation.org> napisał: >> 2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: >>> 2012/1/12 Linus Torvalds <torvalds@linux-foundation.org>: >>>> >>>> I'm surprised it has ever worked for anybody. It certainly doesn't work for me. >>> >>> So I can suspend the bcma driver on its own until the cows come home. >>> >>> But after I have suspended the bcma driver even once, just doing a >>> "modprobe brcmsmac" will hang the machine hard. Dunno where, but this >>> is probably the same thing as "hangs on resume". >> >> Guys, has suspend/resume with the bcma interface been tested AT ALL? >> >> The suspend/resume fields of "struct bcma_driver" are COMPLETELY >> UNUSED. The only place in the kernel that uses them is the brcmsmac >> driver that does this write-only assignment: >> >> .suspend = brcms_suspend, >> .resume = brcms_resume, >> >> nothing else uses them. NOTHING. I tested by just removing the fields >> and compiling the bcma subsystem, just in case there was something >> really subtle that I was missing and was hidden through some magic >> hidden approach. But no. >> >> Seriously - how was something that isn't even connected ever supposed >> to work at all? And why was the BCMA conversion of that driver sent >> up-stream if something as fundamental as suspend/resume had never been >> done, and didn't actually work? >> >> What am I missing now? How the hell can this ever have worked for >> ANYBODY? What kind of f*&*ing sick joke is this all? > > The suspend&resume wasn't implemented for some time because my PC > doesn't s&r. And I don't have access to notebook with mini PCIe slot. > > I've implemented support for s&r in bcma when I got to open my Sony > VAIO to replace A/C power slot. It was one time I was able to change > WiFi card in my notebook which has really-ugly-hidden mini PCIe slot. > > S&r was working fine for me with bcma&b43 after writing that patch! > That includes suspending and resuming multiple times. And tests were > done with the same card you're using. > > The lock up on (resume|loading brcmsmac) means bus wasn't initialized > correctly after resume. It does not have to be brcmsmac bug. We're > accessing some registers before they're ready. > > Linus: can you do one trivial test for me? Please simply try unloading > bcma before suspending. Then resume and load bcma and brcmsmac. Does > it still lockup your machine? Actually.. I've re-read your mail and I got it wrong at first. I though you can suspend&resume once, but then loading brcmsmac causes lock up. I interpreted that as broken initialization after resume. Now I see you *can't suspend for the second time*. I don't get it :/ I've no idea what wrong we may be doing in that trivial bcma_host_pci_suspend and bcma_host_pci_resume stopping you from suspending for the second time. I'll take a look at that new pm ops you told me about.
2012/1/12 Rafał Miłecki <zajec5@gmail.com>: > > Linus: can you do one trivial test for me? Please simply try unloading > bcma before suspending. Then resume and load bcma and brcmsmac. Does > it still lockup your machine? That works, but is not interesting. It just reloads everything. The thing is, your hardware clearly never powers anything down, because the bcma suspend/resume functions aren't hooked up to anything, so the brcmsmac suspend/resume never gets called at all. And it sounds like it works for you for the simple reason that your hardware never loses power - so you don't need to do anything for suspend/resume. But there is absolutely zero question about it - the code does not work. Never has. It's just that your hardware doesn't *need* any code at all, and as far as you are concerned, suspend/resume doesn't even really happen (the PCI layer handles the regular "set to D3 and back to D0", so the fact that the driver doesn't do anything never shows up) Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2012/1/12 Rafał Miłecki <zajec5@gmail.com>: > > Actually.. I've re-read your mail and I got it wrong at first. I > though you can suspend&resume once, but then loading brcmsmac causes > lock up. I interpreted that as broken initialization after resume. That is correct. And I cannot suspend/resume AT ALL if I actually keep brcmsmac loaded - then it will lock up at resume - exactly the same way it locks up at loading brcmsmac time if I had unloaded it. > Now I see you *can't suspend for the second time*. I don't get it :/ No, that was an unrelated bug, I'm chasing that one down too and it seems to be in the machine check driver. Linus -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
drivers/net/wireless/brcm80211/Makefile | 2 +- drivers/net/wireless/brcm80211/brcmsmac/srom.c | 31 +++++++++++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/drivers/net/wireless/brcm80211/Makefile b/drivers/net/wireless/brcm80211/Makefile index f41c047eca82..bae5219cd8be 100644 --- a/drivers/net/wireless/brcm80211/Makefile +++ b/drivers/net/wireless/brcm80211/Makefile @@ -16,7 +16,7 @@ # CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. # common flags -subdir-ccflags-$(CONFIG_BRCMDBG) += -DBCMDBG +subdir-ccflags-$(CONFIG_BRCMDBG) += -DBCMDBG -DDEBUG obj-$(CONFIG_BRCMUTIL) += brcmutil/ obj-$(CONFIG_BRCMFMAC) += brcmfmac/ diff --git a/drivers/net/wireless/brcm80211/brcmsmac/srom.c b/drivers/net/wireless/brcm80211/brcmsmac/srom.c index 61092156755e..563743643038 100644 --- a/drivers/net/wireless/brcm80211/brcmsmac/srom.c +++ b/drivers/net/wireless/brcm80211/brcmsmac/srom.c @@ -764,6 +764,22 @@ _initvars_srom_pci(u8 sromrev, u16 *srom, struct list_head *var_list) } /* + * The crc check is done on a little-endian array, we need + * to switch the bytes around before checking crc (and + * then switch it back). + */ +static int do_crc_check(u16 *buf, unsigned nwords) +{ + u8 crc; + + cpu_to_le16_buf(buf, nwords); + crc = crc8(brcms_srom_crc8_table, (void *)buf, nwords << 1, CRC8_INIT_VALUE); + le16_to_cpu_buf(buf, nwords); + + return crc == CRC8_GOOD_VALUE(brcms_srom_crc8_table); +} + +/* * Read in and validate sprom. * Return 0 on success, nonzero on error. */ @@ -772,8 +788,6 @@ sprom_read_pci(struct si_pub *sih, u16 *buf, uint nwords, bool check_crc) { int err = 0; uint i; - u8 *bbuf = (u8 *)buf; /* byte buffer */ - uint nbytes = nwords << 1; struct bcma_device *core; uint sprom_offset; @@ -786,9 +800,9 @@ sprom_read_pci(struct si_pub *sih, u16 *buf, uint nwords, bool check_crc) sprom_offset = CHIPCREGOFFS(sromotp); } - /* read the sprom in bytes */ - for (i = 0; i < nbytes; i++) - bbuf[i] = bcma_read8(core, sprom_offset+i); + /* read the sprom */ + for (i = 0; i < nwords; i++) + buf[i] = bcma_read16(core, sprom_offset+i*2); if (buf[0] == 0xffff) /* @@ -798,13 +812,8 @@ sprom_read_pci(struct si_pub *sih, u16 *buf, uint nwords, bool check_crc) */ return -ENODATA; - if (check_crc && - crc8(brcms_srom_crc8_table, bbuf, nbytes, CRC8_INIT_VALUE) != - CRC8_GOOD_VALUE(brcms_srom_crc8_table)) + if (check_crc && !do_crc_check(buf, nwords)) err = -EIO; - else - /* now correct the endianness of the byte array */ - le16_to_cpu_buf(buf, nwords); return err; }