Regression caused by commit 882164a4a928

Regression caused by commit 882164a4a928

[Larry Finger]

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?

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

Thanks,

Larry

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Regression caused by commit 882164a4a928

[Kalle Valo]

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.

-- 
Kalle Valo

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 2211 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Regression caused by commit 882164a4a928

[Kalle Valo]

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.

-- 
Kalle Valo

Re: Regression caused by commit 882164a4a928

[Matt Redfearn]

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

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 2698 bytes --]

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.

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Regression caused by commit 882164a4a928

[Matt Redfearn]

Hi Michael,

On 09/05/18 17:27, Michael Büsch wrote:
> 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.
> 

Sure - I've tested the patch:

--- 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
         default y

  config SSB_DRIVER_PCICORE
@@ -131,7 +131,7 @@ config SSB_DRIVER_PCICORE

  config SSB_PCICORE_HOSTMODE
         bool "Hostmode support for SSB PCI core"
-       depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS
+       depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && (SSB = y) && 
PCI_DRIVERS_LEGACY
         help
           PCIcore hostmode operation (external PCI bus).


And this seems to work for MIPS, we don't get the build error from 
building the SSB module under nec_markeins allmodconfig, and 
SSB_PCICORE_HOSTMODE=y for bcm47xx allmodconfig, which selects SSB=y.

So this looks like a good fix for MIPS, at least.

Tested-by: Matt Redfearn <matt.redfearn@mips.com>

Thanks,
Matt

Re: Regression caused by commit 882164a4a928

[Rafał Miłecki]

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?

-- 
Rafał

Re: Regression caused by commit 882164a4a928

[Rafał Miłecki]

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").

-- 
Rafał

Re: Regression caused by commit 882164a4a928

[Matt Redfearn]

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?
> 

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 1737 bytes --]

On Thu, 10 May 2018 11:24:12 +0100
Matt Redfearn <matt.redfearn@mips.com> wrote:

> > 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.
> >   
> 
> Sure - I've tested the patch:
> 
> --- 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
>          default y
> 
>   config SSB_DRIVER_PCICORE
> @@ -131,7 +131,7 @@ config SSB_DRIVER_PCICORE
> 
>   config SSB_PCICORE_HOSTMODE
>          bool "Hostmode support for SSB PCI core"
> -       depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS
> +       depends on SSB_DRIVER_PCICORE && SSB_DRIVER_MIPS && (SSB = y) && 
> PCI_DRIVERS_LEGACY
>          help
>            PCIcore hostmode operation (external PCI bus).
> 
> 
> And this seems to work for MIPS, we don't get the build error from 
> building the SSB module under nec_markeins allmodconfig, and 
> SSB_PCICORE_HOSTMODE=y for bcm47xx allmodconfig, which selects SSB=y.
> 
> So this looks like a good fix for MIPS, at least.
> 
> Tested-by: Matt Redfearn <matt.redfearn@mips.com>


Thanks.
Could you please submit it?
You can add my Acked-by.

-- 
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]