| Message ID | 7bbc067a-c412-3d2e-174a-abc31b46e246@lwfinger.net (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
On Mon, 7 May 2018 10:44:34 -0500 Larry Finger <Larry.Finger@lwfinger.net> wrote: > Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in > module") appeared to be harmless, it leads to complete failure of drivers b43. > config SSB_DRIVER_PCICORE_POSSIBLE > bool > - depends on SSB_PCIHOST && SSB = y > + depends on SSB_PCIHOST && (SSB = y || !MIPS) > default y > > config SSB_DRIVER_PCICORE https://patchwork.kernel.org/patch/10161131/ Could we _please_ switch to not applying patches to ssb or b43, if nobody acked (or better reviewed) a patch? We had multiple changes to ssb and b43 in the recent past that did not have a review at all and broke something. I don't think such software quality is acceptable at all. So please revert 882164a4a928. I'm sorry that this patch slipped through the cracks of my inbox. But the reaction to that shall not be to just apply the patch. It shall be to resubmit it for review. But back to the technical topic. I don't remember why SSB_DRIVER_PCICORE_POSSIBLE depends on SSB_PCIHOST. But that looks and feels wrong. I would say it should rather look like config SSB_DRIVER_PCICORE_POSSIBLE depends on SSB && (PCI = y || PCI = SSB) completely untested, though.
Michael Büsch <m@bues.ch> writes: > On Mon, 7 May 2018 10:44:34 -0500 > Larry Finger <Larry.Finger@lwfinger.net> wrote: > >> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in >> module") appeared to be harmless, it leads to complete failure of drivers b43. > >> config SSB_DRIVER_PCICORE_POSSIBLE >> bool >> - depends on SSB_PCIHOST && SSB = y >> + depends on SSB_PCIHOST && (SSB = y || !MIPS) >> default y >> >> config SSB_DRIVER_PCICORE > > > https://patchwork.kernel.org/patch/10161131/ > > Could we _please_ switch to not applying patches to ssb or b43, if > nobody acked (or better reviewed) a patch? > > We had multiple changes to ssb and b43 in the recent past that did not > have a review at all and broke something. I don't think such software > quality is acceptable at all. > So please revert 882164a4a928. Yes, someone please send a revert so that this can be fixed quickly for v4.17. > I'm sorry that this patch slipped through the cracks of my inbox. > But the reaction to that shall not be to just apply the patch. It > shall be to resubmit it for review. The thing is that in general I do not have time to ping people for every patch, I get enough of emails as is. If there are no review comments I have to assume the patch is ok to apply. But as ssb has had two major regressions recently I'm going to significantly raise the bar for ssb patches, and will refuse to apply random patches if they have not been tested with b43/b44.
On Mon, 07 May 2018 22:03:58 +0300 Kalle Valo <kvalo@codeaurora.org> wrote: > Michael Büsch <m@bues.ch> writes: > > > On Mon, 7 May 2018 10:44:34 -0500 > > Larry Finger <Larry.Finger@lwfinger.net> wrote: > > > >> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in > >> module") appeared to be harmless, it leads to complete failure of drivers b43. > > > >> config SSB_DRIVER_PCICORE_POSSIBLE > >> bool > >> - depends on SSB_PCIHOST && SSB = y > >> + depends on SSB_PCIHOST && (SSB = y || !MIPS) > >> default y > >> > >> config SSB_DRIVER_PCICORE > > > > > > https://patchwork.kernel.org/patch/10161131/ > > > > Could we _please_ switch to not applying patches to ssb or b43, if > > nobody acked (or better reviewed) a patch? > > > > We had multiple changes to ssb and b43 in the recent past that did not > > have a review at all and broke something. I don't think such software > > quality is acceptable at all. > > So please revert 882164a4a928. > > Yes, someone please send a revert so that this can be fixed quickly for > v4.17. Uhm, can you just type git revert 882164a4a928? :) Or do I have to send you a pull request? > > I'm sorry that this patch slipped through the cracks of my inbox. > > But the reaction to that shall not be to just apply the patch. It > > shall be to resubmit it for review. > > The thing is that in general I do not have time to ping people for every > patch, I get enough of emails as is. If there are no review comments I > have to assume the patch is ok to apply. Yes, I understand that pinging people can be annoying and time consuming. But we have tools like patchwork. Why isn't pinging (semi)automated? Patchwork should really track the review status of a patch. I think the concept of no-comments = everything-ok is fundamentally broken. But it has always been that way for wireless and lots of other subsystems. > But as ssb has had two major regressions recently I'm going to > significantly raise the bar for ssb patches, and will refuse to apply > random patches if they have not been tested with b43/b44. Thanks.
Michael Büsch <m@bues.ch> writes: > On Mon, 07 May 2018 22:03:58 +0300 > Kalle Valo <kvalo@codeaurora.org> wrote: > >> Michael Büsch <m@bues.ch> writes: >> >> > On Mon, 7 May 2018 10:44:34 -0500 >> > Larry Finger <Larry.Finger@lwfinger.net> wrote: >> > >> >> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in >> >> module") appeared to be harmless, it leads to complete failure of drivers b43. >> > >> >> config SSB_DRIVER_PCICORE_POSSIBLE >> >> bool >> >> - depends on SSB_PCIHOST && SSB = y >> >> + depends on SSB_PCIHOST && (SSB = y || !MIPS) >> >> default y >> >> >> >> config SSB_DRIVER_PCICORE >> > >> > >> > https://patchwork.kernel.org/patch/10161131/ >> > >> > Could we _please_ switch to not applying patches to ssb or b43, if >> > nobody acked (or better reviewed) a patch? >> > >> > We had multiple changes to ssb and b43 in the recent past that did not >> > have a review at all and broke something. I don't think such software >> > quality is acceptable at all. >> > So please revert 882164a4a928. >> >> Yes, someone please send a revert so that this can be fixed quickly for >> v4.17. > > Uhm, can you just type git revert 882164a4a928? :) But it needs a proper commit log explaining why it's reverted (links to bugzilla report etc). And I prefer also reverts to be reviewed on the list. > Or do I have to send you a pull request? A revert is a regular commit, so you can submit it using git format-patch and git send-email. >> > I'm sorry that this patch slipped through the cracks of my inbox. >> > But the reaction to that shall not be to just apply the patch. It >> > shall be to resubmit it for review. >> >> The thing is that in general I do not have time to ping people for every >> patch, I get enough of emails as is. If there are no review comments I >> have to assume the patch is ok to apply. > > Yes, I understand that pinging people can be annoying and time > consuming. But we have tools like patchwork. Why isn't pinging > (semi)automated? Patchwork should really track the review status of a > patch. That would be awesome but patchwork is nowhere near that kind of sophistication. I like it but to be honest it's really simple at the moment. My custom client script has a simple way to ping about patches but even that is too much work on the long run. Some people do send Acks to the driver they maintain but not always, I guess because too busy with real life or something which is totally understandable. But it would not scale at all if I would start pinging for the 25% of patches that they have not acked. > I think the concept of no-comments = everything-ok is > fundamentally broken. But it has always been that way for wireless and > lots of other subsystems. It's a balance between bureaucracy and getting things done. From my POV the only viable solution is that maintainers actively follow the patches on the mailing list.
Hi Larry On 07/05/18 16:44, Larry Finger wrote: > Matt, > > Although commit 882164a4a928 ("ssb: Prevent build of PCI host features > in module") appeared to be harmless, it leads to complete failure of > drivers b43. and b43legacy, and likely affects b44 as well. The problem > is that CONFIG_SSB_PCIHOST is undefined, which prevents the compilation > of the code that controls the PCI cores of the device. See > https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details. Sorry for the breakage :-/ > > As the underlying errors ("pcibios_enable_device" undefined, and > "register_pci_controller" undefined) do not appear on the architectures > that I have tested (x86_64, x86, and ppc), I suspect something in the > arch-specific code for your setup (MIPS?). As I have no idea on how to > fix that problem, would the following patch work for you? > > diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig > index 9371651d8017..3743533c8057 100644 > --- a/drivers/ssb/Kconfig > +++ b/drivers/ssb/Kconfig > @@ -117,7 +117,7 @@ config SSB_SERIAL > > config SSB_DRIVER_PCICORE_POSSIBLE > bool > - depends on SSB_PCIHOST && SSB = y > + depends on SSB_PCIHOST && (SSB = y || !MIPS) > default y > > config SSB_DRIVER_PCICORE I believe that the problem stems from these drivers being used for some wireless AP functionality built into some MIPS based SoCs. The Kconfig rules sort out building this additional functionality when configured for MIPS (in a round about sort of way), but it allowed it even when SSB is a module, leading to build failures. My patch was intended to prevent that. There was a similar issue in the same Kconfig file, introduced by c5611df96804 and fixed by a9e6d44ddecc. It was fixed the same way as you suggest. I've tested the above patch and it does work for MIPS (preventing the PCICORE being built into the module). Tested-by: Matt Redfearn <matt.redfearn@mips.com> Thanks & sorry again for the breakage, Matt > > Thanks, > > Larry
On Wed, 9 May 2018 13:55:43 +0100 Matt Redfearn <matt.redfearn@mips.com> wrote: > Hi Larry > > On 07/05/18 16:44, Larry Finger wrote: > > Matt, > > > > Although commit 882164a4a928 ("ssb: Prevent build of PCI host features > > in module") appeared to be harmless, it leads to complete failure of > > drivers b43. and b43legacy, and likely affects b44 as well. The problem > > is that CONFIG_SSB_PCIHOST is undefined, which prevents the compilation > > of the code that controls the PCI cores of the device. See > > https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details. > > Sorry for the breakage :-/ > > > > > As the underlying errors ("pcibios_enable_device" undefined, and > > "register_pci_controller" undefined) do not appear on the architectures > > that I have tested (x86_64, x86, and ppc), I suspect something in the > > arch-specific code for your setup (MIPS?). As I have no idea on how to > > fix that problem, would the following patch work for you? > > > > diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig > > index 9371651d8017..3743533c8057 100644 > > --- a/drivers/ssb/Kconfig > > +++ b/drivers/ssb/Kconfig > > @@ -117,7 +117,7 @@ config SSB_SERIAL > > > > config SSB_DRIVER_PCICORE_POSSIBLE > > bool > > - depends on SSB_PCIHOST && SSB = y > > + depends on SSB_PCIHOST && (SSB = y || !MIPS) > > default y > > > > config SSB_DRIVER_PCICORE > > I believe that the problem stems from these drivers being used for some > wireless AP functionality built into some MIPS based SoCs. The Kconfig > rules sort out building this additional functionality when configured > for MIPS (in a round about sort of way), but it allowed it even when SSB > is a module, leading to build failures. My patch was intended to prevent > that. > > There was a similar issue in the same Kconfig file, introduced by > c5611df96804 and fixed by a9e6d44ddecc. It was fixed the same way as you > suggest. I've tested the above patch and it does work for MIPS > (preventing the PCICORE being built into the module). > > Tested-by: Matt Redfearn <matt.redfearn@mips.com> Could you please try this? config SSB_DRIVER_PCICORE_POSSIBLE depends on SSB_PCIHOST config SSB_PCICORE_HOSTMODE depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && (SSB = y) && PCI_DRIVERS_LEGACY The affected API pcibios_enable_device() and register_pci_controller() is only used in HOSTMODE. So I think it makes sense to make HOSTMODE depend on SSB=y and PCI_DRIVERS_LEGACY. PCICore itself does not use the API, if hostmode is disabled.
On 7 May 2018 at 17:44, Larry Finger <Larry.Finger@lwfinger.net> wrote: > Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in > module") appeared to be harmless, it leads to complete failure of drivers > b43. and b43legacy, and likely affects b44 as well. The problem is that > CONFIG_SSB_PCIHOST is undefined, which prevents the compilation of the code > that controls the PCI cores of the device. See > https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details. > > As the underlying errors ("pcibios_enable_device" undefined, and > "register_pci_controller" undefined) do not appear on the architectures that > I have tested (x86_64, x86, and ppc), I suspect something in the > arch-specific code for your setup (MIPS?). As I have no idea on how to fix > that problem, would the following patch work for you? > > diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig > index 9371651d8017..3743533c8057 100644 > --- a/drivers/ssb/Kconfig > +++ b/drivers/ssb/Kconfig > @@ -117,7 +117,7 @@ config SSB_SERIAL > > config SSB_DRIVER_PCICORE_POSSIBLE > bool > - depends on SSB_PCIHOST && SSB = y > + depends on SSB_PCIHOST && (SSB = y || !MIPS) > default y > > config SSB_DRIVER_PCICORE I strongly suggest we take a step back, slow down a bit and look at the original problem. In driver_pcicore.c there is MIPS specific code. It's protected using #ifdef CONFIG_SSB_PCICORE_HOSTMODE (...) #endif If anyone has ever seen ERROR: "pcibios_enable_device" [drivers/ssb/ssb.ko] undefined! ERROR: "register_pci_controller" [drivers/ssb/ssb.ko] undefined! make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1 it means he managed to get CONFIG_SSB_PCICORE_HOSTMODE set on non-MIPS system. We should rather answer how did that happen and fix it. SSB_PCICORE_HOSTMODE depends on SSB_DRIVER_MIPS SSB_DRIVER_MIPS depends on MIPS How is that possible to set SSB_PCICORE_HOSTMODE with non-MIPS config? Is there some mistake in Kconfig I can't see?
On 10 May 2018 at 12:41, Rafał Miłecki <zajec5@gmail.com> wrote: > On 7 May 2018 at 17:44, Larry Finger <Larry.Finger@lwfinger.net> wrote: >> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in >> module") appeared to be harmless, it leads to complete failure of drivers >> b43. and b43legacy, and likely affects b44 as well. The problem is that >> CONFIG_SSB_PCIHOST is undefined, which prevents the compilation of the code >> that controls the PCI cores of the device. See >> https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details. >> >> As the underlying errors ("pcibios_enable_device" undefined, and >> "register_pci_controller" undefined) do not appear on the architectures that >> I have tested (x86_64, x86, and ppc), I suspect something in the >> arch-specific code for your setup (MIPS?). As I have no idea on how to fix >> that problem, would the following patch work for you? >> >> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig >> index 9371651d8017..3743533c8057 100644 >> --- a/drivers/ssb/Kconfig >> +++ b/drivers/ssb/Kconfig >> @@ -117,7 +117,7 @@ config SSB_SERIAL >> >> config SSB_DRIVER_PCICORE_POSSIBLE >> bool >> - depends on SSB_PCIHOST && SSB = y >> + depends on SSB_PCIHOST && (SSB = y || !MIPS) >> default y >> >> config SSB_DRIVER_PCICORE > > I strongly suggest we take a step back, slow down a bit and look at > the original problem. > > In driver_pcicore.c there is MIPS specific code. It's protected using > #ifdef CONFIG_SSB_PCICORE_HOSTMODE > (...) > #endif > > If anyone has ever seen > ERROR: "pcibios_enable_device" [drivers/ssb/ssb.ko] undefined! > ERROR: "register_pci_controller" [drivers/ssb/ssb.ko] undefined! > make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1 > it means he managed to get CONFIG_SSB_PCICORE_HOSTMODE set on non-MIPS system. > > We should rather answer how did that happen and fix it. > > SSB_PCICORE_HOSTMODE depends on SSB_DRIVER_MIPS > SSB_DRIVER_MIPS depends on MIPS > > How is that possible to set SSB_PCICORE_HOSTMODE with non-MIPS config? > Is there some mistake in Kconfig I can't see? I think SSB = y should be added as dependency for SSB_PCICORE_HOSTMODE. See also commit 79ca239a68f8f ("bcma: Prevent build of PCI host features in module").
Hi Rafał, On 10/05/18 11:41, Rafał Miłecki wrote: > On 7 May 2018 at 17:44, Larry Finger <Larry.Finger@lwfinger.net> wrote: >> Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in >> module") appeared to be harmless, it leads to complete failure of drivers >> b43. and b43legacy, and likely affects b44 as well. The problem is that >> CONFIG_SSB_PCIHOST is undefined, which prevents the compilation of the code >> that controls the PCI cores of the device. See >> https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details. >> >> As the underlying errors ("pcibios_enable_device" undefined, and >> "register_pci_controller" undefined) do not appear on the architectures that >> I have tested (x86_64, x86, and ppc), I suspect something in the >> arch-specific code for your setup (MIPS?). As I have no idea on how to fix >> that problem, would the following patch work for you? >> >> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig >> index 9371651d8017..3743533c8057 100644 >> --- a/drivers/ssb/Kconfig >> +++ b/drivers/ssb/Kconfig >> @@ -117,7 +117,7 @@ config SSB_SERIAL >> >> config SSB_DRIVER_PCICORE_POSSIBLE >> bool >> - depends on SSB_PCIHOST && SSB = y >> + depends on SSB_PCIHOST && (SSB = y || !MIPS) >> default y >> >> config SSB_DRIVER_PCICORE > > I strongly suggest we take a step back, slow down a bit and look at > the original problem. > > In driver_pcicore.c there is MIPS specific code. It's protected using > #ifdef CONFIG_SSB_PCICORE_HOSTMODE > (...) > #endif > > If anyone has ever seen > ERROR: "pcibios_enable_device" [drivers/ssb/ssb.ko] undefined! > ERROR: "register_pci_controller" [drivers/ssb/ssb.ko] undefined! > make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1 > it means he managed to get CONFIG_SSB_PCICORE_HOSTMODE set on non-MIPS system. I saw this on a MIPS system (to my knowledge, this does not happen on other arches due to the Kconfig rules you describe), which is what my original patch was attempting to fix, but appears to have caused problems on other arches. Thanks, Matt > > We should rather answer how did that happen and fix it. > > SSB_PCICORE_HOSTMODE depends on SSB_DRIVER_MIPS > SSB_DRIVER_MIPS depends on MIPS > > How is that possible to set SSB_PCICORE_HOSTMODE with non-MIPS config? > Is there some mistake in Kconfig I can't see? >
diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig index 9371651d8017..3743533c8057 100644 --- a/drivers/ssb/Kconfig +++ b/drivers/ssb/Kconfig @@ -117,7 +117,7 @@ config SSB_SERIAL config SSB_DRIVER_PCICORE_POSSIBLE bool - depends on SSB_PCIHOST && SSB = y + depends on SSB_PCIHOST && (SSB = y || !MIPS) default y config SSB_DRIVER_PCICORE
Matt, Although commit 882164a4a928 ("ssb: Prevent build of PCI host features in module") appeared to be harmless, it leads to complete failure of drivers b43. and b43legacy, and likely affects b44 as well. The problem is that CONFIG_SSB_PCIHOST is undefined, which prevents the compilation of the code that controls the PCI cores of the device. See https://bugzilla.redhat.com/show_bug.cgi?id=1572349 for details. As the underlying errors ("pcibios_enable_device" undefined, and "register_pci_controller" undefined) do not appear on the architectures that I have tested (x86_64, x86, and ppc), I suspect something in the arch-specific code for your setup (MIPS?). As I have no idea on how to fix that problem, would the following patch work for you? Thanks, Larry