Re: [PATCH] ssb: Use true and false for bool variable

[Michael Büsch]

On Fri,  5 Feb 2021 14:56:39 +0800
Jiapeng Chong  wrote:

> Fix the following coccicheck warnings:

Acked-by: Michael Büsch 

-- 
Michael


pgpHQ7VvY18zO.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: make array pwr_info_offset static const, makes object smaller

[Michael Büsch]

On Fri,  6 Sep 2019 16:40:53 +0100
Colin King  wrote:

> diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
> index da2d2ab8104d..7c3ae52f2b15 100644
> --- a/drivers/ssb/pci.c
> +++ b/drivers/ssb/pci.c
> @@ -595,7 +595,7 @@ static void sprom_extract_r8(struct ssb_sprom *out, const 
> u16 *in)
>  {
>   int i;
>   u16 o;
> - u16 pwr_info_offset[] = {
> + static const u16 pwr_info_offset[] = {
>   SSB_SROM8_PWR_INFO_CORE0, SSB_SROM8_PWR_INFO_CORE1,
>   SSB_SROM8_PWR_INFO_CORE2, SSB_SROM8_PWR_INFO_CORE3
>   };

Thanks for your contribution. This change makes sense.

Kalle, can you please take it?

Acked-by: Michael Büsch 

-- 
Michael


pgpySXyseXA2l.pgp
Description: OpenPGP digital signature

Re: [PATCH] char: hw_random: Fix missing check during driver release

[Michael Büsch]

On Wed, 26 Dec 2018 11:23:31 -0600
Aditya Pakki  wrote:

> devres_release can return -ENOENT if the device is not freed. The fix
> throws a warning consistent with other invocations.
> 
> Signed-off-by: Aditya Pakki 
> ---
>  drivers/char/hw_random/core.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 95be7228f327..582d983fa93f 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -578,7 +578,11 @@ EXPORT_SYMBOL_GPL(devm_hwrng_register);
>  
>  void devm_hwrng_unregister(struct device *dev, struct hwrng *rng)
>  {
> - devres_release(dev, devm_hwrng_release, devm_hwrng_match, rng);
> + int rc;
> +
> + rc = devres_release(dev, devm_hwrng_release, devm_hwrng_match, rng);
> + if (rc)

The if statement is redundant and can be removed.

> + WARN_ON(rc);
>  }
>  EXPORT_SYMBOL_GPL(devm_hwrng_unregister);
>  

-- 
Michael


pgpVKn6fiRO6Y.pgp
Description: OpenPGP digital signature

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Michael Büsch]

On Wed, 14 Nov 2018 20:27:52 +0200
Priit Laes  wrote:

> Kernel library has a common cordic algorithm which is identical
> to internally implementatd one, so use it and drop the duplicate
> implementation.


In v2 of the series it has been said that:

Arend van Spriel  wrote:
> I recall doing a comparison between the algorithms and thought I put 
> that in the original commit message. However, it is not there. It is not 
> exactly the same as in b43 so there are difference for certain angles, 
> most results are the same however. This implementation is slightly more 
> accurate on the full scale.


That's not my definition of "identical".

Please do not apply this patch without doing a thorough regression test
on actual b43 LP hardware.


-- 
Michael


pgpqIer0xQw15.pgp
Description: OpenPGP digital signature

Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

[Michael Büsch]

On Wed, 14 Nov 2018 20:27:52 +0200
Priit Laes  wrote:

> Kernel library has a common cordic algorithm which is identical
> to internally implementatd one, so use it and drop the duplicate
> implementation.


In v2 of the series it has been said that:

Arend van Spriel  wrote:
> I recall doing a comparison between the algorithms and thought I put 
> that in the original commit message. However, it is not there. It is not 
> exactly the same as in b43 so there are difference for certain angles, 
> most results are the same however. This implementation is slightly more 
> accurate on the full scale.


That's not my definition of "identical".

Please do not apply this patch without doing a thorough regression test
on actual b43 LP hardware.


-- 
Michael


pgpqIer0xQw15.pgp
Description: OpenPGP digital signature

Re: [TRIVIAL RFC PATCH] Kconfigs - reduce use of "depends on EXPERT"

[Michael Büsch]

On Sun, 29 Jul 2018 11:16:37 -0700
Joe Perches  wrote:

> (removing a bunch of cc's)
> 
> On Sun, 2018-07-29 at 13:42 +0200, Michael Büsch wrote:
> > On Sat, 28 Jul 2018 15:13:00 -0700
> > Joe Perches  wrote:
> >   
> > >  config SSB_SILENT
> > > - bool "No SSB kernel messages"
> > > - depends on SSB && EXPERT
> > > + bool "No SSB kernel messages" if EXPERT
> > > + depends on SSB
> > >   help
> > > This option turns off all Sonics Silicon Backplane printks.
> > > Note that you won't be able to identify problems, once  
> > 
> > 
> > What about removing this option entirely?
> > We would just have to remove it from Kconfig and from its only use in
> > drivers/ssb/ssb_private.h
> > I don't think anybody uses (or should use) this option anyway.
> > That would reduce the EXPERT dependencies by one, which is a good
> > thing. :)  
> 
> I'm fine with that, but it was originally your code from
> the first ssb commit in 2007:
> 
>   This might only be desired for production kernels on
>   embedded devices to reduce the kernel size.
> 
> Presumably for ddwrt and such.

Yeah, but it doesn't make sense to do this in ssb only.
And it only saves a couple of k at best.

> Removal could simplify the ssb_ printk logic a bit too.
> 
> Perhaps something like the below,
> but your code, your decisions...:


This is great!
Thanks.

Reviewed-by: Michael Buesch 


-- 
Michael


pgpUyrQIFmckt.pgp
Description: OpenPGP digital signature

Re: [TRIVIAL RFC PATCH] Kconfigs - reduce use of "depends on EXPERT"

[Michael Büsch]

On Sun, 29 Jul 2018 11:16:37 -0700
Joe Perches  wrote:

> (removing a bunch of cc's)
> 
> On Sun, 2018-07-29 at 13:42 +0200, Michael Büsch wrote:
> > On Sat, 28 Jul 2018 15:13:00 -0700
> > Joe Perches  wrote:
> >   
> > >  config SSB_SILENT
> > > - bool "No SSB kernel messages"
> > > - depends on SSB && EXPERT
> > > + bool "No SSB kernel messages" if EXPERT
> > > + depends on SSB
> > >   help
> > > This option turns off all Sonics Silicon Backplane printks.
> > > Note that you won't be able to identify problems, once  
> > 
> > 
> > What about removing this option entirely?
> > We would just have to remove it from Kconfig and from its only use in
> > drivers/ssb/ssb_private.h
> > I don't think anybody uses (or should use) this option anyway.
> > That would reduce the EXPERT dependencies by one, which is a good
> > thing. :)  
> 
> I'm fine with that, but it was originally your code from
> the first ssb commit in 2007:
> 
>   This might only be desired for production kernels on
>   embedded devices to reduce the kernel size.
> 
> Presumably for ddwrt and such.

Yeah, but it doesn't make sense to do this in ssb only.
And it only saves a couple of k at best.

> Removal could simplify the ssb_ printk logic a bit too.
> 
> Perhaps something like the below,
> but your code, your decisions...:


This is great!
Thanks.

Reviewed-by: Michael Buesch 


-- 
Michael


pgpUyrQIFmckt.pgp
Description: OpenPGP digital signature

Re: [TRIVIAL RFC PATCH] Kconfigs - reduce use of "depends on EXPERT"

[Michael Büsch]

On Sat, 28 Jul 2018 15:13:00 -0700
Joe Perches  wrote:

>  config SSB_SILENT
> - bool "No SSB kernel messages"
> - depends on SSB && EXPERT
> + bool "No SSB kernel messages" if EXPERT
> + depends on SSB
>   help
> This option turns off all Sonics Silicon Backplane printks.
> Note that you won't be able to identify problems, once


What about removing this option entirely?
We would just have to remove it from Kconfig and from its only use in
drivers/ssb/ssb_private.h
I don't think anybody uses (or should use) this option anyway.
That would reduce the EXPERT dependencies by one, which is a good
thing. :)

-- 
Michael


pgpx9pHV1ovN2.pgp
Description: OpenPGP digital signature

Re: [TRIVIAL RFC PATCH] Kconfigs - reduce use of "depends on EXPERT"

[Michael Büsch]

On Sat, 28 Jul 2018 15:13:00 -0700
Joe Perches  wrote:

>  config SSB_SILENT
> - bool "No SSB kernel messages"
> - depends on SSB && EXPERT
> + bool "No SSB kernel messages" if EXPERT
> + depends on SSB
>   help
> This option turns off all Sonics Silicon Backplane printks.
> Note that you won't be able to identify problems, once


What about removing this option entirely?
We would just have to remove it from Kconfig and from its only use in
drivers/ssb/ssb_private.h
I don't think anybody uses (or should use) this option anyway.
That would reduce the EXPERT dependencies by one, which is a good
thing. :)

-- 
Michael


pgpx9pHV1ovN2.pgp
Description: OpenPGP digital signature

Re: [PATCH V2 1/2] Revert "ssb: Prevent build of PCI host features in module"

[Michael Büsch]

On Sat, 12 May 2018 12:00:07 +0200
Rafał Miłecki  wrote:

> > Yes, I'm OK with the patch, if we have a third patch that cleans up the
> > PCI_DRIVERS_LEGACY dependency by moving it to SSB_PCICORE_HOSTMODE
> > where it belongs. (This doesn't need to go into the stable tree.)
> > We currently implicitly get that via dependency chain, so this is OK
> > for now as-is.  
> 
> I'm planning to handle PCI_DRIVERS_LEGACY cleanup once my patches hit
> net-next.git and then wireless-drivers-next.git. It's to avoid
> conflicts.

Yes, thanks. Take your time. We're not in a hurry. :)
This change should not make a functional difference.

-- 
Michael


pgp7rpvrfTlkY.pgp
Description: OpenPGP digital signature

Re: [PATCH V2 1/2] Revert "ssb: Prevent build of PCI host features in module"

[Michael Büsch]

On Sat, 12 May 2018 12:00:07 +0200
Rafał Miłecki  wrote:

> > Yes, I'm OK with the patch, if we have a third patch that cleans up the
> > PCI_DRIVERS_LEGACY dependency by moving it to SSB_PCICORE_HOSTMODE
> > where it belongs. (This doesn't need to go into the stable tree.)
> > We currently implicitly get that via dependency chain, so this is OK
> > for now as-is.  
> 
> I'm planning to handle PCI_DRIVERS_LEGACY cleanup once my patches hit
> net-next.git and then wireless-drivers-next.git. It's to avoid
> conflicts.

Yes, thanks. Take your time. We're not in a hurry. :)
This change should not make a functional difference.

-- 
Michael


pgp7rpvrfTlkY.pgp
Description: OpenPGP digital signature

Re: [PATCH V2 1/2] Revert "ssb: Prevent build of PCI host features in module"

[Michael Büsch]

On Sat, 12 May 2018 10:50:42 +0300
Kalle Valo  wrote:

> Larry Finger  writes:
> 
> > On 05/11/2018 05:13 AM, Kalle Valo wrote:  
> >> Rafał Miłecki  writes:
> >>  
> >>> On 11 May 2018 at 11:17, Rafał Miłecki  wrote:  
>  [...]  
> >>>
> >>> As these patches fix regression/build error, I believe both should get
> >>> into 4.17.  
> >>
> >> How much confidence do we have that we don't need to end up reverting
> >> patch 2 as well? I rather be pushing patch 2 to 4.18 so that there's
> >> more time for testing and waiting for feedback.  
> >
> > Although I do not have the hardware to test the builds, I worked
> > closely with the OP in the bug at b.r.c noted above. From that effort,
> > it became clear what configuration variables were missing to cause the
> > x86 failures. Patch 2 satisfies the requirement, and prevents the
> > build problems found by the MIPS users. Both patches are needed in
> > 4.17.  
> 
> And I assume Michael is ok with this approach as well as I haven't heard
> from him. I'll then push both of these to 4.17.
> 

Yes, I'm OK with the patch, if we have a third patch that cleans up the
PCI_DRIVERS_LEGACY dependency by moving it to SSB_PCICORE_HOSTMODE
where it belongs. (This doesn't need to go into the stable tree.)
We currently implicitly get that via dependency chain, so this is OK
for now as-is.

-- 
Michael


pgpZw5S0s7E0K.pgp
Description: OpenPGP digital signature

Re: [PATCH V2 1/2] Revert "ssb: Prevent build of PCI host features in module"

[Michael Büsch]

On Sat, 12 May 2018 10:50:42 +0300
Kalle Valo  wrote:

> Larry Finger  writes:
> 
> > On 05/11/2018 05:13 AM, Kalle Valo wrote:  
> >> Rafał Miłecki  writes:
> >>  
> >>> On 11 May 2018 at 11:17, Rafał Miłecki  wrote:  
>  [...]  
> >>>
> >>> As these patches fix regression/build error, I believe both should get
> >>> into 4.17.  
> >>
> >> How much confidence do we have that we don't need to end up reverting
> >> patch 2 as well? I rather be pushing patch 2 to 4.18 so that there's
> >> more time for testing and waiting for feedback.  
> >
> > Although I do not have the hardware to test the builds, I worked
> > closely with the OP in the bug at b.r.c noted above. From that effort,
> > it became clear what configuration variables were missing to cause the
> > x86 failures. Patch 2 satisfies the requirement, and prevents the
> > build problems found by the MIPS users. Both patches are needed in
> > 4.17.  
> 
> And I assume Michael is ok with this approach as well as I haven't heard
> from him. I'll then push both of these to 4.17.
> 

Yes, I'm OK with the patch, if we have a third patch that cleans up the
PCI_DRIVERS_LEGACY dependency by moving it to SSB_PCICORE_HOSTMODE
where it belongs. (This doesn't need to go into the stable tree.)
We currently implicitly get that via dependency chain, so this is OK
for now as-is.

-- 
Michael


pgpZw5S0s7E0K.pgp
Description: OpenPGP digital signature

Re: [PATCH 4.17 2/2] ssb: make SSB_PCICORE_HOSTMODE depend on SSB = y

[Michael Büsch]

On Thu, 10 May 2018 13:20:01 +0200
Rafał Miłecki <zaj...@gmail.com> wrote:

> On 10 May 2018 at 13:17, Michael Büsch <m...@bues.ch> wrote:
> > On Thu, 10 May 2018 13:14:01 +0200
> > Rafał Miłecki <zaj...@gmail.com> wrote:
> >  
> >> From: Rafał Miłecki <ra...@milecki.pl>
> >>
> >> SSB_PCICORE_HOSTMODE protects MIPS specific code that calls not exported
> >> symbols pcibios_enable_device and register_pci_controller. This code is
> >> supposed to be compiled only with ssb builtin.
> >>
> >> This fixes:
> >> 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
> >>
> >> Signed-off-by: Rafał Miłecki <ra...@milecki.pl>
> >> ---
> >>  drivers/ssb/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> >> index b3f5cae98ea6..c574dd210500 100644
> >> --- a/drivers/ssb/Kconfig
> >> +++ b/drivers/ssb/Kconfig
> >> @@ -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
> >>   help
> >> PCIcore hostmode operation (external PCI bus).
> >>  
> >
> >
> > I think we also need to depend on PCI_DRIVERS_LEGACY.
> > See the other patch that floats around.  
> 
> I believe it's already handled by SSB_PCIHOST_POSSIBLE's dependency on
> PCI_DRIVERS_LEGACY.


That dependency seems to be wrong there.
Was it added among some other "let's just unbreak some random
build" change as well?

SSB_PCIHOST enables support for SSB on top of PCI. (Which is 99% of it
uses). I don't see how this uses the legacy API.

SSB_PCICORE_HOSTMODE enables PCI on top of SSB. Which is a MIPS corner
case. This uses the legacy MIPS API to register a PCI bus.

-- 
Michael


pgpcbehTFYXq7.pgp
Description: OpenPGP digital signature

Re: [PATCH 4.17 2/2] ssb: make SSB_PCICORE_HOSTMODE depend on SSB = y

[Michael Büsch]

On Thu, 10 May 2018 13:20:01 +0200
Rafał Miłecki  wrote:

> On 10 May 2018 at 13:17, Michael Büsch  wrote:
> > On Thu, 10 May 2018 13:14:01 +0200
> > Rafał Miłecki  wrote:
> >  
> >> From: Rafał Miłecki 
> >>
> >> SSB_PCICORE_HOSTMODE protects MIPS specific code that calls not exported
> >> symbols pcibios_enable_device and register_pci_controller. This code is
> >> supposed to be compiled only with ssb builtin.
> >>
> >> This fixes:
> >> 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
> >>
> >> Signed-off-by: Rafał Miłecki 
> >> ---
> >>  drivers/ssb/Kconfig | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> >> index b3f5cae98ea6..c574dd210500 100644
> >> --- a/drivers/ssb/Kconfig
> >> +++ b/drivers/ssb/Kconfig
> >> @@ -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
> >>   help
> >> PCIcore hostmode operation (external PCI bus).
> >>  
> >
> >
> > I think we also need to depend on PCI_DRIVERS_LEGACY.
> > See the other patch that floats around.  
> 
> I believe it's already handled by SSB_PCIHOST_POSSIBLE's dependency on
> PCI_DRIVERS_LEGACY.


That dependency seems to be wrong there.
Was it added among some other "let's just unbreak some random
build" change as well?

SSB_PCIHOST enables support for SSB on top of PCI. (Which is 99% of it
uses). I don't see how this uses the legacy API.

SSB_PCICORE_HOSTMODE enables PCI on top of SSB. Which is a MIPS corner
case. This uses the legacy MIPS API to register a PCI bus.

-- 
Michael


pgpcbehTFYXq7.pgp
Description: OpenPGP digital signature

Re: [PATCH 4.17 2/2] ssb: make SSB_PCICORE_HOSTMODE depend on SSB = y

[Michael Büsch]

On Thu, 10 May 2018 13:14:01 +0200
Rafał Miłecki  wrote:

> From: Rafał Miłecki 
> 
> SSB_PCICORE_HOSTMODE protects MIPS specific code that calls not exported
> symbols pcibios_enable_device and register_pci_controller. This code is
> supposed to be compiled only with ssb builtin.
> 
> This fixes:
> 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
> 
> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> index b3f5cae98ea6..c574dd210500 100644
> --- a/drivers/ssb/Kconfig
> +++ b/drivers/ssb/Kconfig
> @@ -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
>   help
> PCIcore hostmode operation (external PCI bus).
>  


I think we also need to depend on PCI_DRIVERS_LEGACY.
See the other patch that floats around.


-- 
Michael


pgplKmRSJP7y0.pgp
Description: OpenPGP digital signature

Re: [PATCH 4.17 2/2] ssb: make SSB_PCICORE_HOSTMODE depend on SSB = y

[Michael Büsch]

On Thu, 10 May 2018 13:14:01 +0200
Rafał Miłecki  wrote:

> From: Rafał Miłecki 
> 
> SSB_PCICORE_HOSTMODE protects MIPS specific code that calls not exported
> symbols pcibios_enable_device and register_pci_controller. This code is
> supposed to be compiled only with ssb builtin.
> 
> This fixes:
> 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
> 
> Signed-off-by: Rafał Miłecki 
> ---
>  drivers/ssb/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig
> index b3f5cae98ea6..c574dd210500 100644
> --- a/drivers/ssb/Kconfig
> +++ b/drivers/ssb/Kconfig
> @@ -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
>   help
> PCIcore hostmode operation (external PCI bus).
>  


I think we also need to depend on PCI_DRIVERS_LEGACY.
See the other patch that floats around.


-- 
Michael


pgplKmRSJP7y0.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Thu, 10 May 2018 11:24:12 +0100
Matt Redfearn  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 


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

-- 
Michael


pgpaLoxTVs140.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Thu, 10 May 2018 11:24:12 +0100
Matt Redfearn  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 


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

-- 
Michael


pgpaLoxTVs140.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Wed, 9 May 2018 13:55:43 +0100
Matt Redfearn  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 


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


pgph7pm_t79fh.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Wed, 9 May 2018 13:55:43 +0100
Matt Redfearn  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 


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


pgph7pm_t79fh.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Mon, 07 May 2018 22:03:58 +0300
Kalle Valo <kv...@codeaurora.org> wrote:

> Michael Büsch <m...@bues.ch> writes:
> 
> > On Mon, 7 May 2018 10:44:34 -0500
> > Larry Finger <larry.fin...@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


pgpGjhf5m7m1i.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Mon, 07 May 2018 22:03:58 +0300
Kalle Valo  wrote:

> Michael Büsch  writes:
> 
> > On Mon, 7 May 2018 10:44:34 -0500
> > Larry Finger  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


pgpGjhf5m7m1i.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Mon, 7 May 2018 10:44:34 -0500
Larry Finger  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


pgpanKn1qGUX5.pgp
Description: OpenPGP digital signature

Re: Regression caused by commit 882164a4a928

[Michael Büsch]

On Mon, 7 May 2018 10:44:34 -0500
Larry Finger  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


pgpanKn1qGUX5.pgp
Description: OpenPGP digital signature

Re: [PATCH] net: wireless: b43legacy: Replace GFP_ATOMIC with GFP_KERNEL in dma_tx_fragment

[Michael Büsch]

On Tue, 10 Apr 2018 21:54:19 +0800
Jia-Ju Bai  wrote:

> dma_tx_fragment() is never called in atomic context.
> 
> dma_tx_fragment() is only called by b43legacy_dma_tx(), which is 
> only called by b43legacy_tx_work().
> b43legacy_tx_work() is only set a parameter of INIT_WORK() in 
> b43legacy_wireless_init().
> 
> Despite never getting called from atomic context,
> dma_tx_fragment() calls alloc_skb() with GFP_ATOMIC,
> which does not sleep for allocation.
> GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
> which can sleep and improve the possibility of sucessful allocation.
> 
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/broadcom/b43legacy/dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c 
> b/drivers/net/wireless/broadcom/b43legacy/dma.c
> index cfa617d..2f0c64c 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/dma.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
> @@ -1064,7 +1064,7 @@ static int dma_tx_fragment(struct b43legacy_dmaring 
> *ring,
>   meta->dmaaddr = map_descbuffer(ring, skb->data, skb->len, 1);
>   /* create a bounce buffer in zone_dma on mapping failure. */
>   if (b43legacy_dma_mapping_error(ring, meta->dmaaddr, skb->len, 1)) {
> - bounce_skb = alloc_skb(skb->len, GFP_ATOMIC | GFP_DMA);
> + bounce_skb = alloc_skb(skb->len, GFP_KERNEL | GFP_DMA);
>   if (!bounce_skb) {
>   ring->current_slot = old_top_slot;
>   ring->used_slots = old_used_slots;


Ack.
I think the GFP_ATOMIC came from the days where we did DMA operations
under spinlock instead of mutex.

The same thing can be done in b43.

Also 
setup_rx_descbuffer(ring, desc, meta, GFP_ATOMIC)
could be GFP_KERNEL in dma_rx().
This function is called from IRQ thread context.

-- 
Michael


pgpNu3Jd3cdA1.pgp
Description: OpenPGP digital signature

Re: [PATCH] net: wireless: b43legacy: Replace GFP_ATOMIC with GFP_KERNEL in dma_tx_fragment

[Michael Büsch]

On Tue, 10 Apr 2018 21:54:19 +0800
Jia-Ju Bai  wrote:

> dma_tx_fragment() is never called in atomic context.
> 
> dma_tx_fragment() is only called by b43legacy_dma_tx(), which is 
> only called by b43legacy_tx_work().
> b43legacy_tx_work() is only set a parameter of INIT_WORK() in 
> b43legacy_wireless_init().
> 
> Despite never getting called from atomic context,
> dma_tx_fragment() calls alloc_skb() with GFP_ATOMIC,
> which does not sleep for allocation.
> GFP_ATOMIC is not necessary and can be replaced with GFP_KERNEL,
> which can sleep and improve the possibility of sucessful allocation.
> 
> This is found by a static analysis tool named DCNS written by myself.
> And I also manually check it.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/broadcom/b43legacy/dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c 
> b/drivers/net/wireless/broadcom/b43legacy/dma.c
> index cfa617d..2f0c64c 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/dma.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
> @@ -1064,7 +1064,7 @@ static int dma_tx_fragment(struct b43legacy_dmaring 
> *ring,
>   meta->dmaaddr = map_descbuffer(ring, skb->data, skb->len, 1);
>   /* create a bounce buffer in zone_dma on mapping failure. */
>   if (b43legacy_dma_mapping_error(ring, meta->dmaaddr, skb->len, 1)) {
> - bounce_skb = alloc_skb(skb->len, GFP_ATOMIC | GFP_DMA);
> + bounce_skb = alloc_skb(skb->len, GFP_KERNEL | GFP_DMA);
>   if (!bounce_skb) {
>   ring->current_slot = old_top_slot;
>   ring->used_slots = old_used_slots;


Ack.
I think the GFP_ATOMIC came from the days where we did DMA operations
under spinlock instead of mutex.

The same thing can be done in b43.

Also 
setup_rx_descbuffer(ring, desc, meta, GFP_ATOMIC)
could be GFP_KERNEL in dma_rx().
This function is called from IRQ thread context.

-- 
Michael


pgpNu3Jd3cdA1.pgp
Description: OpenPGP digital signature

Re: [PATCH v2] ssb: use put_device() if device_register fail

[Michael Büsch]

On Thu,  8 Mar 2018 10:39:49 +0530
Arvind Yadav  wrote:

> Never directly free @dev after calling device_register(), even
> if it returned an error! Always use put_device() to give up the
> reference initialized.
> 
> Signed-off-by: Arvind Yadav 
> ---
> changes in v2:
>  Removed kfree() call for @dev.
> 
>  drivers/ssb/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 65420a9..a7a062b 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -521,8 +521,8 @@ static int ssb_devices_register(struct ssb_bus *bus)
>   ssb_err("Could not register %s\n", dev_name(dev));
>   /* Set dev to NULL to not unregister
>* dev on error unwinding. */
> + put_device(dev);
>   sdev->dev = NULL;
> - kfree(devwrap);
>   goto error;
>   }
>   dev_idx++;


Would you please put the put_device where the kfree was?
So that the comment still matches.
Thanks.


-- 
Michael


pgpEPaQ_ojN5f.pgp
Description: OpenPGP digital signature

Re: [PATCH v2] ssb: use put_device() if device_register fail

[Michael Büsch]

On Thu,  8 Mar 2018 10:39:49 +0530
Arvind Yadav  wrote:

> Never directly free @dev after calling device_register(), even
> if it returned an error! Always use put_device() to give up the
> reference initialized.
> 
> Signed-off-by: Arvind Yadav 
> ---
> changes in v2:
>  Removed kfree() call for @dev.
> 
>  drivers/ssb/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 65420a9..a7a062b 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -521,8 +521,8 @@ static int ssb_devices_register(struct ssb_bus *bus)
>   ssb_err("Could not register %s\n", dev_name(dev));
>   /* Set dev to NULL to not unregister
>* dev on error unwinding. */
> + put_device(dev);
>   sdev->dev = NULL;
> - kfree(devwrap);
>   goto error;
>   }
>   dev_idx++;


Would you please put the put_device where the kfree was?
So that the comment still matches.
Thanks.


-- 
Michael


pgpEPaQ_ojN5f.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb:: use put_device() if device_register fail

[Michael Büsch]

On Wed, 7 Mar 2018 22:46:14 +0530
arvindY  wrote:
> >> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> >> index 65420a9..c4449e0 100644
> >> --- a/drivers/ssb/main.c
> >> +++ b/drivers/ssb/main.c
> >> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus)
> >>ssb_err("Could not register %s\n", dev_name(dev));
> >>/* Set dev to NULL to not unregister
> >> * dev on error unwinding. */
> >> +  put_device(dev);
> >>sdev->dev = NULL;
> >>kfree(devwrap);
> >>goto error;  
> >
> > I don't think this is correct.
> > The dev structure is allocated as part of devwrap, which is freed here.
> >
> > Why do you think we need put_device here?
> >  
> Yes this patch is not correct, We must not use kfree() after you called 
> device_register() (even
> if it was not successful!) -- see the comment for device_register().
> I will delete kfree() and send updated patch.


Is device_put() going to call ssb_release_dev() to free the structure?

Can you please elaborate on why device_put() must be used? The comment
is not really of any use here.

-- 
Michael


pgpLKKfTxTtMX.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb:: use put_device() if device_register fail

[Michael Büsch]

On Wed, 7 Mar 2018 22:46:14 +0530
arvindY  wrote:
> >> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> >> index 65420a9..c4449e0 100644
> >> --- a/drivers/ssb/main.c
> >> +++ b/drivers/ssb/main.c
> >> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus)
> >>ssb_err("Could not register %s\n", dev_name(dev));
> >>/* Set dev to NULL to not unregister
> >> * dev on error unwinding. */
> >> +  put_device(dev);
> >>sdev->dev = NULL;
> >>kfree(devwrap);
> >>goto error;  
> >
> > I don't think this is correct.
> > The dev structure is allocated as part of devwrap, which is freed here.
> >
> > Why do you think we need put_device here?
> >  
> Yes this patch is not correct, We must not use kfree() after you called 
> device_register() (even
> if it was not successful!) -- see the comment for device_register().
> I will delete kfree() and send updated patch.


Is device_put() going to call ssb_release_dev() to free the structure?

Can you please elaborate on why device_put() must be used? The comment
is not really of any use here.

-- 
Michael


pgpLKKfTxTtMX.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb:: use put_device() if device_register fail

[Michael Büsch]

On Wed,  7 Mar 2018 15:31:30 +0530
Arvind Yadav  wrote:

> if device_register() returned an error! Always use put_device()
> to give up the reference initialized.
> 
> Signed-off-by: Arvind Yadav 
> ---
>  drivers/ssb/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 65420a9..c4449e0 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus)
>   ssb_err("Could not register %s\n", dev_name(dev));
>   /* Set dev to NULL to not unregister
>* dev on error unwinding. */
> + put_device(dev);
>   sdev->dev = NULL;
>   kfree(devwrap);
>   goto error;


I don't think this is correct.
The dev structure is allocated as part of devwrap, which is freed here.

Why do you think we need put_device here?

-- 
Michael


pgpato_DG7p3U.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb:: use put_device() if device_register fail

[Michael Büsch]

On Wed,  7 Mar 2018 15:31:30 +0530
Arvind Yadav  wrote:

> if device_register() returned an error! Always use put_device()
> to give up the reference initialized.
> 
> Signed-off-by: Arvind Yadav 
> ---
>  drivers/ssb/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index 65420a9..c4449e0 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -521,6 +521,7 @@ static int ssb_devices_register(struct ssb_bus *bus)
>   ssb_err("Could not register %s\n", dev_name(dev));
>   /* Set dev to NULL to not unregister
>* dev on error unwinding. */
> + put_device(dev);
>   sdev->dev = NULL;
>   kfree(devwrap);
>   goto error;


I don't think this is correct.
The dev structure is allocated as part of devwrap, which is freed here.

Why do you think we need put_device here?

-- 
Michael


pgpato_DG7p3U.pgp
Description: OpenPGP digital signature

Re: B43 driver no longer works in Linux 4.15 (bisected)

[Michael Büsch]

On Mon, 5 Feb 2018 13:14:28 -0500
Adric Blake  wrote:

> In the time between Linux 4.15-rc8 and -rc9, my wireless driver, b43, would
> no longer load automatically. When I modprobe the b43 (and ssb) modules,
> the device still didn't appear in NetworkManager. Comparing the kernel logs
> between working (4.14.16-lts) and nonworking (4.15) kernels reveals that
> there is zero output from the ssb module, and no devices recognized by the
> b43 driver:


Thanks for your bug report.
A fix for this exists already:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=a9e6d44ddeccd3522670e641f1ed9b068e746ff7

-- 
Michael


pgp_XyPOWWFn7.pgp
Description: OpenPGP digital signature

Re: B43 driver no longer works in Linux 4.15 (bisected)

[Michael Büsch]

On Mon, 5 Feb 2018 13:14:28 -0500
Adric Blake  wrote:

> In the time between Linux 4.15-rc8 and -rc9, my wireless driver, b43, would
> no longer load automatically. When I modprobe the b43 (and ssb) modules,
> the device still didn't appear in NetworkManager. Comparing the kernel logs
> between working (4.14.16-lts) and nonworking (4.15) kernels reveals that
> there is zero output from the ssb module, and no devices recognized by the
> b43 driver:


Thanks for your bug report.
A fix for this exists already:
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git/commit/?id=a9e6d44ddeccd3522670e641f1ed9b068e746ff7

-- 
Michael


pgp_XyPOWWFn7.pgp
Description: OpenPGP digital signature

Re: [BUG] ssb: Possible sleep-in-atomic bugs in ssb_pcmcia_read8

[Michael Büsch]

On Mon, 9 Oct 2017 09:29:17 +0800
Jia-Ju Bai  wrote:

> According to pcmcia.c, the driver may sleep under a spinlock.
> The function call paths are:
> ssb_pcmcia_read8 (acquire the spinlock)
>select_core_and_segment
>  ssb_pcmcia_switch_segment
>ssb_pcmcia_cfg_write
>  pcmcia_write_config_byte
>pcmcia_access_config (drivers/pcmcia/pcmcia_resource.c)
>  mutex_lock --> may sleep
> 
> ssb_pcmcia_read8 (acquire the spinlock)
>select_core_and_segment
>  ssb_pcmcia_switch_segment
>sssb_pcmcia_cfg_read
>  pcmcia_read_config_byte
>pcmcia_access_config (drivers/pcmcia/pcmcia_resource.c)
>  mutex_lock --> may sleep
> 
> A possible fix is to use spinlock instead of mutex lock in 
> pcmcia_access_config in drivers/pcmcia/pcmcia_resource.c.
> 
> These bugs are found by my static analysis tool and my code review.



Thanks for scanning and your resulting bug notification.
I currently don't have the hardware at hand to develop and test a
proper fix for this.
That said, I'm not so sure anymore why bar_lock is a spinlock instead
of a mutex. It might be possible to convert this to mutex.

I will try to look into this.

-- 
Michael


pgprHLua9lSV_.pgp
Description: OpenPGP digital signature

Re: [BUG] ssb: Possible sleep-in-atomic bugs in ssb_pcmcia_read8

[Michael Büsch]

On Mon, 9 Oct 2017 09:29:17 +0800
Jia-Ju Bai  wrote:

> According to pcmcia.c, the driver may sleep under a spinlock.
> The function call paths are:
> ssb_pcmcia_read8 (acquire the spinlock)
>select_core_and_segment
>  ssb_pcmcia_switch_segment
>ssb_pcmcia_cfg_write
>  pcmcia_write_config_byte
>pcmcia_access_config (drivers/pcmcia/pcmcia_resource.c)
>  mutex_lock --> may sleep
> 
> ssb_pcmcia_read8 (acquire the spinlock)
>select_core_and_segment
>  ssb_pcmcia_switch_segment
>sssb_pcmcia_cfg_read
>  pcmcia_read_config_byte
>pcmcia_access_config (drivers/pcmcia/pcmcia_resource.c)
>  mutex_lock --> may sleep
> 
> A possible fix is to use spinlock instead of mutex lock in 
> pcmcia_access_config in drivers/pcmcia/pcmcia_resource.c.
> 
> These bugs are found by my static analysis tool and my code review.



Thanks for scanning and your resulting bug notification.
I currently don't have the hardware at hand to develop and test a
proper fix for this.
That said, I'm not so sure anymore why bar_lock is a spinlock instead
of a mutex. It might be possible to convert this to mutex.

I will try to look into this.

-- 
Michael


pgprHLua9lSV_.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/2] b43legacy: fix unitialized reads of ret by initializing the array to zero

[Michael Büsch]

On Tue,  5 Sep 2017 19:16:58 +0100
Colin King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139653 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/broadcom/b43legacy/radio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/radio.c 
> b/drivers/net/wireless/broadcom/b43legacy/radio.c
> index 9501420340a9..eab1c9387846 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/radio.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/radio.c
> @@ -280,7 +280,7 @@ u8 b43legacy_radio_aci_detect(struct b43legacy_wldev 
> *dev, u8 channel)
>  u8 b43legacy_radio_aci_scan(struct b43legacy_wldev *dev)
>  {
>   struct b43legacy_phy *phy = >phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
>   unsigned int channel = phy->channel;
>   unsigned int i;
>   unsigned int j;


This fix seems to be correct.
Thanks for finding and fixing the issue.

Reviewed-by: Michael Buesch 



-- 
Michael


pgpTHx8sgw7o6.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/2] b43legacy: fix unitialized reads of ret by initializing the array to zero

[Michael Büsch]

On Tue,  5 Sep 2017 19:16:58 +0100
Colin King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139653 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/broadcom/b43legacy/radio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/radio.c 
> b/drivers/net/wireless/broadcom/b43legacy/radio.c
> index 9501420340a9..eab1c9387846 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/radio.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/radio.c
> @@ -280,7 +280,7 @@ u8 b43legacy_radio_aci_detect(struct b43legacy_wldev 
> *dev, u8 channel)
>  u8 b43legacy_radio_aci_scan(struct b43legacy_wldev *dev)
>  {
>   struct b43legacy_phy *phy = >phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
>   unsigned int channel = phy->channel;
>   unsigned int i;
>   unsigned int j;


This fix seems to be correct.
Thanks for finding and fixing the issue.

Reviewed-by: Michael Buesch 



-- 
Michael


pgpTHx8sgw7o6.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/2] b43: fix unitialized reads of ret by initializing the array to zero

[Michael Büsch]

On Tue,  5 Sep 2017 19:15:50 +0100
Colin King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/broadcom/b43/phy_g.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/phy_g.c 
> b/drivers/net/wireless/broadcom/b43/phy_g.c
> index 822dcaa8ace6..f59c02166462 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_g.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_g.c
> @@ -2297,7 +2297,7 @@ static u8 b43_gphy_aci_detect(struct b43_wldev *dev, u8 
> channel)
>  static u8 b43_gphy_aci_scan(struct b43_wldev *dev)
>  {
>   struct b43_phy *phy = >phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
>   unsigned int channel = phy->channel;
>   unsigned int i, j, start, end;
>  


This fix seems to be correct.
Thanks for finding and fixing the issue.

Reviewed-by: Michael Buesch 


-- 
Michael


pgpEoVMbDw38f.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/2] b43: fix unitialized reads of ret by initializing the array to zero

[Michael Büsch]

On Tue,  5 Sep 2017 19:15:50 +0100
Colin King  wrote:

> From: Colin Ian King 
> 
> The u8 char array ret is not being initialized and elements outside
> the range start to end contain just garbage values from the stack.
> This results in a later scan of the array to read potentially
> uninitialized values.  Fix this by initializing the array to zero.
> This seems to have been an issue since the very first commit.
> 
> Detected by CoverityScan CID#139652 ("Uninitialized scalar variable")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/broadcom/b43/phy_g.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/phy_g.c 
> b/drivers/net/wireless/broadcom/b43/phy_g.c
> index 822dcaa8ace6..f59c02166462 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_g.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_g.c
> @@ -2297,7 +2297,7 @@ static u8 b43_gphy_aci_detect(struct b43_wldev *dev, u8 
> channel)
>  static u8 b43_gphy_aci_scan(struct b43_wldev *dev)
>  {
>   struct b43_phy *phy = >phy;
> - u8 ret[13];
> + u8 ret[13] = { 0 };
>   unsigned int channel = phy->channel;
>   unsigned int i, j, start, end;
>  


This fix seems to be correct.
Thanks for finding and fixing the issue.

Reviewed-by: Michael Buesch 


-- 
Michael


pgpEoVMbDw38f.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store

[Michael Büsch]

On Fri, 02 Jun 2017 09:18:14 +0800
Jia-Ju Bai  wrote:

> On 06/02/2017 12:11 AM, Jonathan Corbet wrote:
> > On Thu, 01 Jun 2017 09:05:07 +0800
> > Jia-Ju Bai  wrote:
> >  
> >> I admit my patches are not well tested, and they may not well fix the bugs.
> >> I am looking forward to opinions and suggestions :)  
> > May I politely suggest that sending out untested locking changes is a
> > dangerous thing to do?  You really should not be changing the locking in a
> > piece of kernel code without understanding very well what the lock is
> > protecting and being able to say why your changes are safe.  Without that,
> > the risk of introducing subtle bugs is very high.
> >
> > It looks like you have written a useful tool that could help us to make
> > the kernel more robust.  If you are interested in my suggestion, I would
> > recommend that you post the sleep-in-atomic scenarios that you are
> > finding, but refrain from "fixing" them in any case where you cannot offer
> > a strong explanation of why your fix is correct.
> >
> > Thanks for working to find bugs in the kernel!
> >
> > jon  
> Hi,
> 
> Thanks for your good and helpful advice. I am sorry for my improper patches.
> I will only report bugs instead of sending improper patches when I have 
> no good solution of fixing the bugs.


Is somebody still working on these fixes?
I think I found my old b43-legacy based 4306, so that I will
be able to get these patches into properly tested shape.

-- 
Michael


pgpSRaLzxvqv8.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store

[Michael Büsch]

On Fri, 02 Jun 2017 09:18:14 +0800
Jia-Ju Bai  wrote:

> On 06/02/2017 12:11 AM, Jonathan Corbet wrote:
> > On Thu, 01 Jun 2017 09:05:07 +0800
> > Jia-Ju Bai  wrote:
> >  
> >> I admit my patches are not well tested, and they may not well fix the bugs.
> >> I am looking forward to opinions and suggestions :)  
> > May I politely suggest that sending out untested locking changes is a
> > dangerous thing to do?  You really should not be changing the locking in a
> > piece of kernel code without understanding very well what the lock is
> > protecting and being able to say why your changes are safe.  Without that,
> > the risk of introducing subtle bugs is very high.
> >
> > It looks like you have written a useful tool that could help us to make
> > the kernel more robust.  If you are interested in my suggestion, I would
> > recommend that you post the sleep-in-atomic scenarios that you are
> > finding, but refrain from "fixing" them in any case where you cannot offer
> > a strong explanation of why your fix is correct.
> >
> > Thanks for working to find bugs in the kernel!
> >
> > jon  
> Hi,
> 
> Thanks for your good and helpful advice. I am sorry for my improper patches.
> I will only report bugs instead of sending improper patches when I have 
> no good solution of fixing the bugs.


Is somebody still working on these fixes?
I think I found my old b43-legacy based 4306, so that I will
be able to get these patches into properly tested shape.

-- 
Michael


pgpSRaLzxvqv8.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Wed, 31 May 2017 19:07:15 -0500
Larry Finger <larry.fin...@lwfinger.net> wrote:

> On 05/31/2017 10:32 AM, Michael Büsch wrote:
> > On Wed, 31 May 2017 13:26:43 +0300
> > Kalle Valo <kv...@codeaurora.org> wrote:
> >   
> >> Jia-Ju Bai <baijiaju1...@163.com> writes:
> >>  
> >>> The driver may sleep under a spin lock, and the function call path is:
> >>> b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
> >>>b43legacy_synchronize_irq
> >>>  synchronize_irq --> may sleep
> >>>
> >>> To fix it, the lock is released before b43legacy_synchronize_irq, and the
> >>> lock is acquired again after this function.
> >>>
> >>> Signed-off-by: Jia-Ju Bai <baijiaju1...@163.com>
> >>> ---
> >>>   drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
> >>> b/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> index f1e3dad..31ead21 100644
> >>> --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> >>> ieee80211_hw *hw,
> >>>   b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >>>   
> >>>   if (changed & BSS_CHANGED_BSSID) {
> >>> + spin_unlock_irqrestore(>irq_lock, flags);
> >>>   b43legacy_synchronize_irq(dev);
> >>> + spin_lock_irqsave(>irq_lock, flags);  
> >>
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>  
> > 
> > 
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.
> > We disabled the device interrupts just before this line.
> > 
> > However I think the synchronize_irq should be outside of the
> > conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
> > two lines above)
> > I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
> > is set.
> > 
> > 
> > On the other hand b43 does not have this irq-disabling foobar anymore.
> > So somebody must have removed it. Maybe you can find the commit that
> > removed this stuff from b43 and port it to b43legacy?
> > 
> > 
> > So I would vote for moving the synchronize_irq up outside of the
> > conditional and put the unlock/lock sequence around it.
> > And as a second patch on top of that try to remove this stuff
> > altogether like b43 did.  
> 
> The patch that removed it in b43 is
> 
> commit 36dbd9548e92268127b0c31b0e121e63e9207108
> Author: Michael Buesch <m...@bu3sch.de>
> Date:   Fri Sep 4 22:51:29 2009 +0200

Damn it :D

>  b43: Use a threaded IRQ handler
> 
>  Use a threaded IRQ handler to allow locking the mutex and
>  sleeping while executing an interrupt.
>  This removes usage of the irq_lock spinlock, but introduces
>  a new hardirq_lock, which is _only_ used for the PCI/SSB lowlevel
>  hard-irq handler. Sleeping busses (SDIO) will use mutex instead.
> 
>  Signed-off-by: Michael Buesch <m...@bu3sch.de>
>  Tested-by: Larry Finger <larry.fin...@lwfinger.net>
>  Signed-off-by: John W. Linville <linvi...@tuxdriver.com>
> 
> I vaguely remember this patch. Although it is roughly a 1000-line fix, I will 
> try to port it to b43legacy. I still have an old BCM4306 PCMCIA card that I 
> can 
> test in a PowerBook G4.
> 
> I agree with Michael that this is the way to go. Both of Jia-Ju's patches 
> should 
> be rejected.


I'm not sure if it's worth it. There is a risk that this would
introduce new bugs.
But sure, please feel free to try it. This way we can find out how big
this change becomes.

-- 
Michael


pgpGuuJkoasqH.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Wed, 31 May 2017 19:07:15 -0500
Larry Finger  wrote:

> On 05/31/2017 10:32 AM, Michael Büsch wrote:
> > On Wed, 31 May 2017 13:26:43 +0300
> > Kalle Valo  wrote:
> >   
> >> Jia-Ju Bai  writes:
> >>  
> >>> The driver may sleep under a spin lock, and the function call path is:
> >>> b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
> >>>b43legacy_synchronize_irq
> >>>  synchronize_irq --> may sleep
> >>>
> >>> To fix it, the lock is released before b43legacy_synchronize_irq, and the
> >>> lock is acquired again after this function.
> >>>
> >>> Signed-off-by: Jia-Ju Bai 
> >>> ---
> >>>   drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
> >>> b/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> index f1e3dad..31ead21 100644
> >>> --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >>> @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> >>> ieee80211_hw *hw,
> >>>   b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >>>   
> >>>   if (changed & BSS_CHANGED_BSSID) {
> >>> + spin_unlock_irqrestore(>irq_lock, flags);
> >>>   b43legacy_synchronize_irq(dev);
> >>> + spin_lock_irqsave(>irq_lock, flags);  
> >>
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>  
> > 
> > 
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.
> > We disabled the device interrupts just before this line.
> > 
> > However I think the synchronize_irq should be outside of the
> > conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
> > two lines above)
> > I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
> > is set.
> > 
> > 
> > On the other hand b43 does not have this irq-disabling foobar anymore.
> > So somebody must have removed it. Maybe you can find the commit that
> > removed this stuff from b43 and port it to b43legacy?
> > 
> > 
> > So I would vote for moving the synchronize_irq up outside of the
> > conditional and put the unlock/lock sequence around it.
> > And as a second patch on top of that try to remove this stuff
> > altogether like b43 did.  
> 
> The patch that removed it in b43 is
> 
> commit 36dbd9548e92268127b0c31b0e121e63e9207108
> Author: Michael Buesch 
> Date:   Fri Sep 4 22:51:29 2009 +0200

Damn it :D

>  b43: Use a threaded IRQ handler
> 
>  Use a threaded IRQ handler to allow locking the mutex and
>  sleeping while executing an interrupt.
>  This removes usage of the irq_lock spinlock, but introduces
>  a new hardirq_lock, which is _only_ used for the PCI/SSB lowlevel
>  hard-irq handler. Sleeping busses (SDIO) will use mutex instead.
> 
>  Signed-off-by: Michael Buesch 
>  Tested-by: Larry Finger 
>  Signed-off-by: John W. Linville 
> 
> I vaguely remember this patch. Although it is roughly a 1000-line fix, I will 
> try to port it to b43legacy. I still have an old BCM4306 PCMCIA card that I 
> can 
> test in a PowerBook G4.
> 
> I agree with Michael that this is the way to go. Both of Jia-Ju's patches 
> should 
> be rejected.


I'm not sure if it's worth it. There is a risk that this would
introduce new bugs.
But sure, please feel free to try it. This way we can find out how big
this change becomes.

-- 
Michael


pgpGuuJkoasqH.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Thu, 01 Jun 2017 07:27:20 +0300
Kalle Valo <kv...@codeaurora.org> wrote:

> Michael Büsch <m...@bues.ch> writes:
> 
> >> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> >> > ieee80211_hw *hw,
> >> >  b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >> >  
> >> >  if (changed & BSS_CHANGED_BSSID) {
> >> > +spin_unlock_irqrestore(>irq_lock, flags);
> >> >  b43legacy_synchronize_irq(dev);
> >> > +spin_lock_irqsave(>irq_lock, flags);
> >> 
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>   
> >
> >
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.  
> 
> Sure, but IMHO in general I think the practise of releasing the lock
> like this in a middle of function is dangerous as one can easily miss
> that upper and lower halves of the function are not actually atomic
> anymore. And in this case that it's under a conditional makes it even
> worse.
> 


Yes in general I agree. Releasing and re-acquiring a lock is dangerous.
But I think in this special case here it might be harmless.
The irq_lock is used mostly (if not exclusively; I don't fully
remember) to protect against the IRQ top and bottom half.
But we disabled the device IRQs a line above and the purpose of this
synchronize is to make sure the handler will finish and thus make
dropping the lock save.
Of course it does not make sense to do this with the lock held :)

-- 
Michael


pgpzxV10ouNjW.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Thu, 01 Jun 2017 07:27:20 +0300
Kalle Valo  wrote:

> Michael Büsch  writes:
> 
> >> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> >> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> >> > ieee80211_hw *hw,
> >> >  b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >> >  
> >> >  if (changed & BSS_CHANGED_BSSID) {
> >> > +spin_unlock_irqrestore(>irq_lock, flags);
> >> >  b43legacy_synchronize_irq(dev);
> >> > +spin_lock_irqsave(>irq_lock, flags);
> >> 
> >> To me this looks like a fragile workaround and not a real fix. You can
> >> easily add new race conditions with releasing the lock like this.
> >>   
> >
> >
> > I think releasing the lock possibly is fine. It certainly is better than
> > sleeping with a lock held.  
> 
> Sure, but IMHO in general I think the practise of releasing the lock
> like this in a middle of function is dangerous as one can easily miss
> that upper and lower halves of the function are not actually atomic
> anymore. And in this case that it's under a conditional makes it even
> worse.
> 


Yes in general I agree. Releasing and re-acquiring a lock is dangerous.
But I think in this special case here it might be harmless.
The irq_lock is used mostly (if not exclusively; I don't fully
remember) to protect against the IRQ top and bottom half.
But we disabled the device IRQs a line above and the purpose of this
synchronize is to make sure the handler will finish and thus make
dropping the lock save.
Of course it does not make sense to do this with the lock held :)

-- 
Michael


pgpzxV10ouNjW.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Wed, 31 May 2017 13:26:43 +0300
Kalle Valo  wrote:

> Jia-Ju Bai  writes:
> 
> > The driver may sleep under a spin lock, and the function call path is:
> > b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
> >   b43legacy_synchronize_irq
> > synchronize_irq --> may sleep
> >
> > To fix it, the lock is released before b43legacy_synchronize_irq, and the 
> > lock is acquired again after this function.
> >
> > Signed-off-by: Jia-Ju Bai 
> > ---
> >  drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
> > b/drivers/net/wireless/broadcom/b43legacy/main.c
> > index f1e3dad..31ead21 100644
> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> > ieee80211_hw *hw,
> > b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >  
> > if (changed & BSS_CHANGED_BSSID) {
> > +   spin_unlock_irqrestore(>irq_lock, flags);
> > b43legacy_synchronize_irq(dev);
> > +   spin_lock_irqsave(>irq_lock, flags);  
> 
> To me this looks like a fragile workaround and not a real fix. You can
> easily add new race conditions with releasing the lock like this.
> 


I think releasing the lock possibly is fine. It certainly is better than
sleeping with a lock held.
We disabled the device interrupts just before this line.

However I think the synchronize_irq should be outside of the
conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
two lines above)
I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
is set.


On the other hand b43 does not have this irq-disabling foobar anymore.
So somebody must have removed it. Maybe you can find the commit that
removed this stuff from b43 and port it to b43legacy?


So I would vote for moving the synchronize_irq up outside of the
conditional and put the unlock/lock sequence around it.
And as a second patch on top of that try to remove this stuff
altogether like b43 did.

-- 
Michael


pgpm9nm8Mumi0.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_op_bss_info_changed

[Michael Büsch]

On Wed, 31 May 2017 13:26:43 +0300
Kalle Valo  wrote:

> Jia-Ju Bai  writes:
> 
> > The driver may sleep under a spin lock, and the function call path is:
> > b43legacy_op_bss_info_changed (acquire the lock by spin_lock_irqsave)
> >   b43legacy_synchronize_irq
> > synchronize_irq --> may sleep
> >
> > To fix it, the lock is released before b43legacy_synchronize_irq, and the 
> > lock is acquired again after this function.
> >
> > Signed-off-by: Jia-Ju Bai 
> > ---
> >  drivers/net/wireless/broadcom/b43legacy/main.c |2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c 
> > b/drivers/net/wireless/broadcom/b43legacy/main.c
> > index f1e3dad..31ead21 100644
> > --- a/drivers/net/wireless/broadcom/b43legacy/main.c
> > +++ b/drivers/net/wireless/broadcom/b43legacy/main.c
> > @@ -2859,7 +2859,9 @@ static void b43legacy_op_bss_info_changed(struct 
> > ieee80211_hw *hw,
> > b43legacy_write32(dev, B43legacy_MMIO_GEN_IRQ_MASK, 0);
> >  
> > if (changed & BSS_CHANGED_BSSID) {
> > +   spin_unlock_irqrestore(>irq_lock, flags);
> > b43legacy_synchronize_irq(dev);
> > +   spin_lock_irqsave(>irq_lock, flags);  
> 
> To me this looks like a fragile workaround and not a real fix. You can
> easily add new race conditions with releasing the lock like this.
> 


I think releasing the lock possibly is fine. It certainly is better than
sleeping with a lock held.
We disabled the device interrupts just before this line.

However I think the synchronize_irq should be outside of the
conditional right after the write to B43legacy_MMIO_GEN_IRQ_MASK. (So
two lines above)
I don't think it makes sense to only synchronize if BSS_CHANGED_BSSID
is set.


On the other hand b43 does not have this irq-disabling foobar anymore.
So somebody must have removed it. Maybe you can find the commit that
removed this stuff from b43 and port it to b43legacy?


So I would vote for moving the synchronize_irq up outside of the
conditional and put the unlock/lock sequence around it.
And as a second patch on top of that try to remove this stuff
altogether like b43 did.

-- 
Michael


pgpm9nm8Mumi0.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store

[Michael Büsch]

On Wed, 31 May 2017 18:29:07 +0800
Jia-Ju Bai  wrote:

> The driver may sleep under a spin lock, and the function call path is:
> b43legacy_attr_interfmode_store (acquire the lock by spin_lock_irqsave)
>   b43legacy_radio_set_interference_mitigation
> b43legacy_radio_interference_mitigation_disable
>   b43legacy_calc_nrssi_slope
> b43legacy_synth_pu_workaround
>   might_sleep and msleep --> may sleep
> 
> Fixing it may be complex, and a possible way is to remove 
> spin_lock_irqsave and spin_lock_irqrestore in 
> b43legacy_attr_interfmode_store, and the code has been protected by
> mutex_lock and mutex_unlock.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/broadcom/b43legacy/sysfs.c |2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/sysfs.c 
> b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> index 2a1da15..9ede143 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> @@ -137,14 +137,12 @@ static ssize_t b43legacy_attr_interfmode_store(struct 
> device *dev,
>   }
>  
>   mutex_lock(>wl->mutex);
> - spin_lock_irqsave(>wl->irq_lock, flags);
>  
>   err = b43legacy_radio_set_interference_mitigation(wldev, mode);
>   if (err)
>   b43legacyerr(wldev->wl, "Interference Mitigation not "
>  "supported by device\n");
>   mmiowb();
> - spin_unlock_irqrestore(>wl->irq_lock, flags);
>   mutex_unlock(>wl->mutex);
>  
>   return err ? err : count;


Interference mitigation has never been properly implemented and tested.
As such nobody should use it and I would be surprised if anybody uses
this attribute.
So I would suggest to remove this sysfs attribute entirely instead of
having this incorrect fix.

-- 
Michael


pgpl6WSPPZSXc.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43legacy: Fix a sleep-in-atomic bug in b43legacy_attr_interfmode_store

[Michael Büsch]

On Wed, 31 May 2017 18:29:07 +0800
Jia-Ju Bai  wrote:

> The driver may sleep under a spin lock, and the function call path is:
> b43legacy_attr_interfmode_store (acquire the lock by spin_lock_irqsave)
>   b43legacy_radio_set_interference_mitigation
> b43legacy_radio_interference_mitigation_disable
>   b43legacy_calc_nrssi_slope
> b43legacy_synth_pu_workaround
>   might_sleep and msleep --> may sleep
> 
> Fixing it may be complex, and a possible way is to remove 
> spin_lock_irqsave and spin_lock_irqrestore in 
> b43legacy_attr_interfmode_store, and the code has been protected by
> mutex_lock and mutex_unlock.
> 
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/broadcom/b43legacy/sysfs.c |2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/b43legacy/sysfs.c 
> b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> index 2a1da15..9ede143 100644
> --- a/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> +++ b/drivers/net/wireless/broadcom/b43legacy/sysfs.c
> @@ -137,14 +137,12 @@ static ssize_t b43legacy_attr_interfmode_store(struct 
> device *dev,
>   }
>  
>   mutex_lock(>wl->mutex);
> - spin_lock_irqsave(>wl->irq_lock, flags);
>  
>   err = b43legacy_radio_set_interference_mitigation(wldev, mode);
>   if (err)
>   b43legacyerr(wldev->wl, "Interference Mitigation not "
>  "supported by device\n");
>   mmiowb();
> - spin_unlock_irqrestore(>wl->irq_lock, flags);
>   mutex_unlock(>wl->mutex);
>  
>   return err ? err : count;


Interference mitigation has never been properly implemented and tested.
As such nobody should use it and I would be surprised if anybody uses
this attribute.
So I would suggest to remove this sysfs attribute entirely instead of
having this incorrect fix.

-- 
Michael


pgpl6WSPPZSXc.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: Delete an error message for a failed memory allocation in ssb_devices_register()

[Michael Büsch]

On Wed, 17 May 2017 18:22:49 +0200
SF Markus Elfring <elfr...@users.sourceforge.net> wrote:

> From: Markus Elfring <elfr...@users.sourceforge.net>
> Date: Wed, 17 May 2017 18:12:16 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
> ---
>  drivers/ssb/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index d1a750760cf3..65420a9f0e82 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -480,7 +480,6 @@ static int ssb_devices_register(struct ssb_bus *bus)
>  
>   devwrap = kzalloc(sizeof(*devwrap), GFP_KERNEL);
>   if (!devwrap) {
> - ssb_err("Could not allocate device\n");
>   err = -ENOMEM;
>   goto error;
>   }


This looks good.

Acked-by: Michael Büsch <m...@bues.ch>


-- 
Michael


pgpU1nQEYRjWo.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: Delete an error message for a failed memory allocation in ssb_devices_register()

[Michael Büsch]

On Wed, 17 May 2017 18:22:49 +0200
SF Markus Elfring  wrote:

> From: Markus Elfring 
> Date: Wed, 17 May 2017 18:12:16 +0200
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Link: 
> http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring 
> ---
>  drivers/ssb/main.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
> index d1a750760cf3..65420a9f0e82 100644
> --- a/drivers/ssb/main.c
> +++ b/drivers/ssb/main.c
> @@ -480,7 +480,6 @@ static int ssb_devices_register(struct ssb_bus *bus)
>  
>   devwrap = kzalloc(sizeof(*devwrap), GFP_KERNEL);
>   if (!devwrap) {
> - ssb_err("Could not allocate device\n");
>   err = -ENOMEM;
>   goto error;
>   }


This looks good.

Acked-by: Michael Büsch 


-- 
Michael


pgpU1nQEYRjWo.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: main.c: This patch removes unnecessary return statement using spatch tool

[Michael Büsch]

> > @@ -1272,9 +1272,7 @@ u32 ssb_admatch_size(u32 adm)
> > default:
> > SSB_WARN_ON(1);
> > }
> > -   size = (1 << (size + 1));
> > -
> > -   return size;
> > +   return (1 << (size + 1));
> >  }
> >  EXPORT_SYMBOL(ssb_admatch_size);  

I'm all for cleaning up code, but I don't really see how this
change improves the code.

-- 
Michael


pgpBI7CSf1jSC.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: main.c: This patch removes unnecessary return statement using spatch tool

[Michael Büsch]

> > @@ -1272,9 +1272,7 @@ u32 ssb_admatch_size(u32 adm)
> > default:
> > SSB_WARN_ON(1);
> > }
> > -   size = (1 << (size + 1));
> > -
> > -   return size;
> > +   return (1 << (size + 1));
> >  }
> >  EXPORT_SYMBOL(ssb_admatch_size);  

I'm all for cleaning up code, but I don't really see how this
change improves the code.

-- 
Michael


pgpBI7CSf1jSC.pgp
Description: OpenPGP digital signature

Re: [v3,1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Thu, 16 Jun 2016 18:56:14 +0300
Kalle Valo <kv...@codeaurora.org> wrote:

> Michael Büsch <m...@bues.ch> writes:
> 
> > On Thu, 16 Jun 2016 15:23:37 + (UTC)
> > Kalle Valo <kv...@codeaurora.org> wrote:
> >  
> >> Guenter Roeck <li...@roeck-us.net> wrote:  
> >> > gcc-6 reports the following error with -Werror=unused-const-variable.
> >> > 
> >> > drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
> >> >  'b43_phyops_a' defined but not used
> >> > 
> >> > Per Michael Büsch: "All a-phy code is usused", so remove it all,
> >> > and move the remaining Type-G initialization code into phy_g.c.
> >> > 
> >> > Reported-by: Fengguang Wu <fengguang...@intel.com> [0-day test robot]
> >> > Cc: Michael Büsch <m...@bues.ch>
> >> > Signed-off-by: Guenter Roeck <li...@roeck-us.net>
> >> 
> >> Thanks, 2 patches applied to wireless-drivers-next.git:
> >> 
> >> 9791333a840f b43: Remove unused phy_a code
> >> afdfdc481ea9 b43: Completely remove support for phy_a  
> >
> > Did anybody test this on any hardware? I think this should be tested
> > on some G-PHY hardware, before it goes to Linus.  
> 
> Larry tested these:
> 
> "These two patches have been tested on a BCM4318."
> 
> https://patchwork.kernel.org/patch/9154719/


Ah OK. I forgot.
Everything is fine then.

-- 
Michael


pgpad0JPxlF29.pgp
Description: OpenPGP digital signature

Re: [v3,1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Thu, 16 Jun 2016 18:56:14 +0300
Kalle Valo  wrote:

> Michael Büsch  writes:
> 
> > On Thu, 16 Jun 2016 15:23:37 + (UTC)
> > Kalle Valo  wrote:
> >  
> >> Guenter Roeck  wrote:  
> >> > gcc-6 reports the following error with -Werror=unused-const-variable.
> >> > 
> >> > drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
> >> >  'b43_phyops_a' defined but not used
> >> > 
> >> > Per Michael Büsch: "All a-phy code is usused", so remove it all,
> >> > and move the remaining Type-G initialization code into phy_g.c.
> >> > 
> >> > Reported-by: Fengguang Wu  [0-day test robot]
> >> > Cc: Michael Büsch 
> >> > Signed-off-by: Guenter Roeck 
> >> 
> >> Thanks, 2 patches applied to wireless-drivers-next.git:
> >> 
> >> 9791333a840f b43: Remove unused phy_a code
> >> afdfdc481ea9 b43: Completely remove support for phy_a  
> >
> > Did anybody test this on any hardware? I think this should be tested
> > on some G-PHY hardware, before it goes to Linus.  
> 
> Larry tested these:
> 
> "These two patches have been tested on a BCM4318."
> 
> https://patchwork.kernel.org/patch/9154719/


Ah OK. I forgot.
Everything is fine then.

-- 
Michael


pgpad0JPxlF29.pgp
Description: OpenPGP digital signature

Re: [v3,1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Thu, 16 Jun 2016 15:23:37 + (UTC)
Kalle Valo <kv...@codeaurora.org> wrote:

> Guenter Roeck <li...@roeck-us.net> wrote:
> > gcc-6 reports the following error with -Werror=unused-const-variable.
> > 
> > drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
> > 'b43_phyops_a' defined but not used
> > 
> > Per Michael Büsch: "All a-phy code is usused", so remove it all,
> > and move the remaining Type-G initialization code into phy_g.c.
> > 
> > Reported-by: Fengguang Wu <fengguang...@intel.com> [0-day test robot]
> > Cc: Michael Büsch <m...@bues.ch>
> > Signed-off-by: Guenter Roeck <li...@roeck-us.net>  
> 
> Thanks, 2 patches applied to wireless-drivers-next.git:
> 
> 9791333a840f b43: Remove unused phy_a code
> afdfdc481ea9 b43: Completely remove support for phy_a


Did anybody test this on any hardware?
I think this should be tested on some G-PHY hardware, before it goes to
Linus.


-- 
Michael


pgpcpsawFdy6X.pgp
Description: OpenPGP digital signature

Re: [v3,1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Thu, 16 Jun 2016 15:23:37 + (UTC)
Kalle Valo  wrote:

> Guenter Roeck  wrote:
> > gcc-6 reports the following error with -Werror=unused-const-variable.
> > 
> > drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
> > 'b43_phyops_a' defined but not used
> > 
> > Per Michael Büsch: "All a-phy code is usused", so remove it all,
> > and move the remaining Type-G initialization code into phy_g.c.
> > 
> > Reported-by: Fengguang Wu  [0-day test robot]
> > Cc: Michael Büsch 
> > Signed-off-by: Guenter Roeck   
> 
> Thanks, 2 patches applied to wireless-drivers-next.git:
> 
> 9791333a840f b43: Remove unused phy_a code
> afdfdc481ea9 b43: Completely remove support for phy_a


Did anybody test this on any hardware?
I think this should be tested on some G-PHY hardware, before it goes to
Linus.


-- 
Michael


pgpcpsawFdy6X.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers: ssb: Change bare unsigned to unsigned int to suit coding style

[Michael Büsch]

On Sat,  4 Jun 2016 12:50:05 +0100
Hugh Sipière  wrote:

> These lines just have unsigned gpio rather than unsigned int gpio.
> I changed it to suit the coding style.
> 
> Signed-off-by: Hugh Sipière 

Acked-by: Michael Buesch 

Please send this to the MIPS tree, because this basically is MIPS-only
code:

MIPS
M:  Ralf Baechle 
L:  linux-m...@linux-mips.org


-- 
Michael


pgpMcuwA4VOAp.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers: ssb: Change bare unsigned to unsigned int to suit coding style

[Michael Büsch]

On Sat,  4 Jun 2016 12:50:05 +0100
Hugh Sipière  wrote:

> These lines just have unsigned gpio rather than unsigned int gpio.
> I changed it to suit the coding style.
> 
> Signed-off-by: Hugh Sipière 

Acked-by: Michael Buesch 

Please send this to the MIPS tree, because this basically is MIPS-only
code:

MIPS
M:  Ralf Baechle 
L:  linux-m...@linux-mips.org


-- 
Michael


pgpMcuwA4VOAp.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers: ssb: Fix comments to suit coding style

[Michael Büsch]

On Sat,  4 Jun 2016 12:32:13 +0100
Hugh Sipière  wrote:

> I changed it so that these two comments do not end on a line with text.
> 
> Signed-off-by: Hugh Sipière 
> ---
>  drivers/ssb/driver_gpio.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ssb/driver_gpio.c b/drivers/ssb/driver_gpio.c
> index 180e027..f2435f3 100644
> --- a/drivers/ssb/driver_gpio.c
> +++ b/drivers/ssb/driver_gpio.c
> @@ -231,7 +231,8 @@ static int ssb_gpio_chipco_init(struct ssb_bus *bus)
>   chip->ngpio = 16;
>   /* There is just one SoC in one device and its GPIO addresses should be
>* deterministic to address them more easily. The other buses could get
> -  * a random base number. */
> +  * a random base number.
> +  */
>   if (bus->bustype == SSB_BUSTYPE_SSB)
>   chip->base  = 0;
>   else
> @@ -424,7 +425,8 @@ static int ssb_gpio_extif_init(struct ssb_bus *bus)
>   chip->ngpio = 5;
>   /* There is just one SoC in one device and its GPIO addresses should be
>* deterministic to address them more easily. The other buses could get
> -  * a random base number. */
> +  * a random base number.
> +  */
>   if (bus->bustype == SSB_BUSTYPE_SSB)
>   chip->base  = 0;
>   else


What's the benefit from this change?
Will the next person submit a patch to remove wasted lines that just
contain a */?


-- 
Michael


pgpwHFL4sJekp.pgp
Description: OpenPGP digital signature

Re: [PATCH] drivers: ssb: Fix comments to suit coding style

[Michael Büsch]

On Sat,  4 Jun 2016 12:32:13 +0100
Hugh Sipière  wrote:

> I changed it so that these two comments do not end on a line with text.
> 
> Signed-off-by: Hugh Sipière 
> ---
>  drivers/ssb/driver_gpio.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ssb/driver_gpio.c b/drivers/ssb/driver_gpio.c
> index 180e027..f2435f3 100644
> --- a/drivers/ssb/driver_gpio.c
> +++ b/drivers/ssb/driver_gpio.c
> @@ -231,7 +231,8 @@ static int ssb_gpio_chipco_init(struct ssb_bus *bus)
>   chip->ngpio = 16;
>   /* There is just one SoC in one device and its GPIO addresses should be
>* deterministic to address them more easily. The other buses could get
> -  * a random base number. */
> +  * a random base number.
> +  */
>   if (bus->bustype == SSB_BUSTYPE_SSB)
>   chip->base  = 0;
>   else
> @@ -424,7 +425,8 @@ static int ssb_gpio_extif_init(struct ssb_bus *bus)
>   chip->ngpio = 5;
>   /* There is just one SoC in one device and its GPIO addresses should be
>* deterministic to address them more easily. The other buses could get
> -  * a random base number. */
> +  * a random base number.
> +  */
>   if (bus->bustype == SSB_BUSTYPE_SSB)
>   chip->base  = 0;
>   else


What's the benefit from this change?
Will the next person submit a patch to remove wasted lines that just
contain a */?


-- 
Michael


pgpwHFL4sJekp.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Fri,  3 Jun 2016 21:11:51 -0700
Guenter Roeck  wrote:

> +static void __b43_phy_initg(struct b43_wldev *dev)
> +{
> + struct b43_phy *phy = >phy;
> +
> + might_sleep();
> +
> + if (phy->rev >= 6) {
> + if (b43_phy_read(dev, B43_PHY_ENCORE) & B43_PHY_ENCORE_EN)
> + b43_phy_set(dev, B43_PHY_ENCORE, 0x0010);
> + else
> + b43_phy_mask(dev, B43_PHY_ENCORE, ~0x1010);
> + }
> +
> + b43_wa_all(dev);
> +
> + if (dev->dev->bus_sprom->boardflags_lo & B43_BFL_PACTRL)
> + b43_phy_maskset(dev, B43_PHY_OFDM(0x6E), 0xE000, 0x3CF);
> +}
> +
>  static void b43_phy_initg(struct b43_wldev *dev)
>  {
>   struct b43_phy *phy = >phy;
> @@ -1999,7 +2019,7 @@ static void b43_phy_initg(struct b43_wldev *dev)
>   b43_phy_initb6(dev);
>  
>   if (phy->rev >= 2 || phy->gmode)
> - b43_phy_inita(dev);
> + __b43_phy_initg(dev);

This actually is correctly called inita(), because there are A-phy parts
in the G-phy.
So I wasn't 100% correct saying that _all_ a-phy code is unused.
I'm Ok with moving that into the g-phy file though. But don't rename it.


-- 
Michael


pgp_lzos00RtM.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 1/2] b43: Remove unused phy_a code

[Michael Büsch]

On Fri,  3 Jun 2016 21:11:51 -0700
Guenter Roeck  wrote:

> +static void __b43_phy_initg(struct b43_wldev *dev)
> +{
> + struct b43_phy *phy = >phy;
> +
> + might_sleep();
> +
> + if (phy->rev >= 6) {
> + if (b43_phy_read(dev, B43_PHY_ENCORE) & B43_PHY_ENCORE_EN)
> + b43_phy_set(dev, B43_PHY_ENCORE, 0x0010);
> + else
> + b43_phy_mask(dev, B43_PHY_ENCORE, ~0x1010);
> + }
> +
> + b43_wa_all(dev);
> +
> + if (dev->dev->bus_sprom->boardflags_lo & B43_BFL_PACTRL)
> + b43_phy_maskset(dev, B43_PHY_OFDM(0x6E), 0xE000, 0x3CF);
> +}
> +
>  static void b43_phy_initg(struct b43_wldev *dev)
>  {
>   struct b43_phy *phy = >phy;
> @@ -1999,7 +2019,7 @@ static void b43_phy_initg(struct b43_wldev *dev)
>   b43_phy_initb6(dev);
>  
>   if (phy->rev >= 2 || phy->gmode)
> - b43_phy_inita(dev);
> + __b43_phy_initg(dev);

This actually is correctly called inita(), because there are A-phy parts
in the G-phy.
So I wasn't 100% correct saying that _all_ a-phy code is unused.
I'm Ok with moving that into the g-phy file though. But don't rename it.


-- 
Michael


pgp_lzos00RtM.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: Remove unused phy_a code

[Michael Büsch]

On Fri,  3 Jun 2016 14:32:46 -0700
Guenter Roeck  wrote:

> gcc-6 reports the following error with -Werror=unused-const-variable.
> 
> drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
>   'b43_phyops_a' defined but not used
> 
> Turns out a lot of code in this file is unused, so let's remove it.


All a-phy code is usused.
So you can basically remove the whole file and any other A-PHY code.
... or just leave it as-is.


-- 
Michael


pgpYJAY10nnbY.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: Remove unused phy_a code

[Michael Büsch]

On Fri,  3 Jun 2016 14:32:46 -0700
Guenter Roeck  wrote:

> gcc-6 reports the following error with -Werror=unused-const-variable.
> 
> drivers/net/wireless/broadcom/b43/phy_a.c:576:40: error:
>   'b43_phyops_a' defined but not used
> 
> Turns out a lot of code in this file is unused, so let's remove it.


All a-phy code is usused.
So you can basically remove the whole file and any other A-PHY code.
... or just leave it as-is.


-- 
Michael


pgpYJAY10nnbY.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: fix memory leak

[Michael Büsch]

On Fri, 19 Feb 2016 20:37:18 +0530
Sudip Mukherjee  wrote:

> > https://patchwork.kernel.org/patch/8049041/  
> 
> I have an old laptop running on 800Mhz CPU. It has "Broadcom BCM4311 
> [14e4:4311] (rev 01)".
> I will try to test it on this weekend.

Any news on this one?


-- 
Michael


pgp4nmCjQbR_a.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: fix memory leak

[Michael Büsch]

On Fri, 19 Feb 2016 20:37:18 +0530
Sudip Mukherjee  wrote:

> > https://patchwork.kernel.org/patch/8049041/  
> 
> I have an old laptop running on 800Mhz CPU. It has "Broadcom BCM4311 
> [14e4:4311] (rev 01)".
> I will try to test it on this weekend.

Any news on this one?


-- 
Michael


pgp4nmCjQbR_a.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: fix memory leak

[Michael Büsch]

On Thu, 18 Feb 2016 18:04:36 +0530
Sudip Mukherjee  wrote:

> From: Sudip Mukherjee 
> 
> On error we jumped to the label bcma_out and returned the error code but
> we missed freeing dev.
> 
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/net/wireless/broadcom/b43/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/main.c 
> b/drivers/net/wireless/broadcom/b43/main.c
> index c279211..78f670a 100644
> --- a/drivers/net/wireless/broadcom/b43/main.c
> +++ b/drivers/net/wireless/broadcom/b43/main.c
> @@ -5671,6 +5671,7 @@ static int b43_bcma_probe(struct bcma_device *core)
>   wl = b43_wireless_init(dev);
>   if (IS_ERR(wl)) {
>   err = PTR_ERR(wl);
> + kfree(dev);
>   goto bcma_out;
>   }
>  

We recently had a patch that fixes this, among more leaks. Subject:
[PATCH v2 resend] b43: Fix memory leaks in b43_bus_dev_ssb_init and
b43_bus_dev_bcma_init

Please test that patch instead, so we can finally apply it.

It needs to be tested on both ssb and bcma. Come on. This isn't too
hard. :) Please somebody with any hardware test it. (I currently don't
have any b43 hardware)

-- 
Michael


pgpf6NDalqVdV.pgp
Description: OpenPGP digital signature

Re: [PATCH] b43: fix memory leak

[Michael Büsch]

On Thu, 18 Feb 2016 18:04:36 +0530
Sudip Mukherjee  wrote:

> From: Sudip Mukherjee 
> 
> On error we jumped to the label bcma_out and returned the error code but
> we missed freeing dev.
> 
> Signed-off-by: Sudip Mukherjee 
> ---
>  drivers/net/wireless/broadcom/b43/main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/wireless/broadcom/b43/main.c 
> b/drivers/net/wireless/broadcom/b43/main.c
> index c279211..78f670a 100644
> --- a/drivers/net/wireless/broadcom/b43/main.c
> +++ b/drivers/net/wireless/broadcom/b43/main.c
> @@ -5671,6 +5671,7 @@ static int b43_bcma_probe(struct bcma_device *core)
>   wl = b43_wireless_init(dev);
>   if (IS_ERR(wl)) {
>   err = PTR_ERR(wl);
> + kfree(dev);
>   goto bcma_out;
>   }
>  

We recently had a patch that fixes this, among more leaks. Subject:
[PATCH v2 resend] b43: Fix memory leaks in b43_bus_dev_ssb_init and
b43_bus_dev_bcma_init

Please test that patch instead, so we can finally apply it.

It needs to be tested on both ssb and bcma. Come on. This isn't too
hard. :) Please somebody with any hardware test it. (I currently don't
have any b43 hardware)

-- 
Michael


pgpf6NDalqVdV.pgp
Description: OpenPGP digital signature

Re: [PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

On Thu, 19 Nov 2015 15:08:32 -0800
Andrew Morton  wrote:

> I don't think we should apply this unless someone can runtime test it. 
> Presumably the current code works OK, but we just don't know what
> nasties the fixed version might expose.

I fully agree. But who can test it?

> The best I can think of is to put a big FIXME comment in there, so
> perhaps one day if someone is working on m32r stuff, they may try
> fixing it.

Or remove the architecture, if nobody is using it? :)

-- 
Michael


pgpCEUDCb0tdO.pgp
Description: OpenPGP digital signature

Re: [PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

On Thu, 19 Nov 2015 15:08:32 -0800
Andrew Morton  wrote:

> I don't think we should apply this unless someone can runtime test it. 
> Presumably the current code works OK, but we just don't know what
> nasties the fixed version might expose.

I fully agree. But who can test it?

> The best I can think of is to put a big FIXME comment in there, so
> perhaps one day if someone is working on m32r stuff, they may try
> fixing it.

Or remove the architecture, if nobody is using it? :)

-- 
Michael


pgpCEUDCb0tdO.pgp
Description: OpenPGP digital signature

[PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok
for this arch.

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 18 Jun 2015.




pgplAgpLvXjD_.pgp
Description: OpenPGP digital signature

[PATCH] sh: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok
for this arch.

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 18 Jun 2015.



Index: linux/arch/sh/include/asm/thread_info.h
===
--- linux.orig/arch/sh/include/asm/thread_info.h
+++ linux/arch/sh/include/asm/thread_info.h
@@ -172,7 +172,7 @@ static inline void set_restore_sigmask(v
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti->flags = (ti->flags & (~0UL >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val << TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpL2X_WNUdZJ.pgp
Description: OpenPGP digital signature

[PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok
for this arch.

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 18 Jun 2015.

(Sorry, hit the send button early, so here goes the actual patch.)


Index: linux/arch/m32r/include/asm/thread_info.h
===
--- linux.orig/arch/m32r/include/asm/thread_info.h
+++ linux/arch/m32r/include/asm/thread_info.h
@@ -77,7 +77,7 @@ static inline struct thread_info *curren
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti->flags = (ti->flags & (~0UL >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val << TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpaHtKBGcUeX.pgp
Description: OpenPGP digital signature

[PATCH] nvidia/noveau: Fix color mask

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Accordingly ~(~0 >> x) will always be zero.
Hence 'mask' will always be zero in this case.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned int constant.

Signed-off-by: Michael Buesch 

---

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 17 Jun 2015.


Index: linux/drivers/gpu/drm/nouveau/nv50_fbcon.c
===
--- linux.orig/drivers/gpu/drm/nouveau/nv50_fbcon.c
+++ linux/drivers/gpu/drm/nouveau/nv50_fbcon.c
@@ -96,7 +96,7 @@ nv50_fbcon_imageblit(struct fb_info *inf
struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
struct nouveau_channel *chan = drm->channel;
uint32_t width, dwords, *data = (uint32_t *)image->data;
-   uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+   uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
uint32_t *palette = info->pseudo_palette;
int ret;
 
Index: linux/drivers/gpu/drm/nouveau/nvc0_fbcon.c
===
--- linux.orig/drivers/gpu/drm/nouveau/nvc0_fbcon.c
+++ linux/drivers/gpu/drm/nouveau/nvc0_fbcon.c
@@ -96,7 +96,7 @@ nvc0_fbcon_imageblit(struct fb_info *inf
struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
struct nouveau_channel *chan = drm->channel;
uint32_t width, dwords, *data = (uint32_t *)image->data;
-   uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+   uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
uint32_t *palette = info->pseudo_palette;
int ret;
 
Index: linux/drivers/video/fbdev/nvidia/nv_accel.c
===
--- linux.orig/drivers/video/fbdev/nvidia/nv_accel.c
+++ linux/drivers/video/fbdev/nvidia/nv_accel.c
@@ -351,7 +351,7 @@ static void nvidiafb_mono_color_expand(s
   const struct fb_image *image)
 {
struct nvidia_par *par = info->par;
-   u32 fg, bg, mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+   u32 fg, bg, mask = ~(~0U >> (32 - info->var.bits_per_pixel));
u32 dsize, width, *data = (u32 *) image->data, tmp;
int j, k = 0;
 


pgpQPeF9q0yCV.pgp
Description: OpenPGP digital signature

[PATCH] nvidia/noveau: Fix color mask

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Accordingly ~(~0 >> x) will always be zero.
Hence 'mask' will always be zero in this case.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned int constant.

Signed-off-by: Michael Buesch 

---

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 17 Jun 2015.


Index: linux/drivers/gpu/drm/nouveau/nv50_fbcon.c
===
--- linux.orig/drivers/gpu/drm/nouveau/nv50_fbcon.c
+++ linux/drivers/gpu/drm/nouveau/nv50_fbcon.c
@@ -96,7 +96,7 @@ nv50_fbcon_imageblit(struct fb_info *inf
struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
struct nouveau_channel *chan = drm->channel;
uint32_t width, dwords, *data = (uint32_t *)image->data;
-   uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+   uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
uint32_t *palette = info->pseudo_palette;
int ret;
 
Index: linux/drivers/gpu/drm/nouveau/nvc0_fbcon.c
===
--- linux.orig/drivers/gpu/drm/nouveau/nvc0_fbcon.c
+++ linux/drivers/gpu/drm/nouveau/nvc0_fbcon.c
@@ -96,7 +96,7 @@ nvc0_fbcon_imageblit(struct fb_info *inf
struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
struct nouveau_channel *chan = drm->channel;
uint32_t width, dwords, *data = (uint32_t *)image->data;
-   uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+   uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
uint32_t *palette = info->pseudo_palette;
int ret;
 
Index: linux/drivers/video/fbdev/nvidia/nv_accel.c
===
--- linux.orig/drivers/video/fbdev/nvidia/nv_accel.c
+++ linux/drivers/video/fbdev/nvidia/nv_accel.c
@@ -351,7 +351,7 @@ static void nvidiafb_mono_color_expand(s
   const struct fb_image *image)
 {
struct nvidia_par *par = info->par;
-   u32 fg, bg, mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+   u32 fg, bg, mask = ~(~0U >> (32 - info->var.bits_per_pixel));
u32 dsize, width, *data = (u32 *) image->data, tmp;
int j, k = 0;
 


pgpQPeF9q0yCV.pgp
Description: OpenPGP digital signature

[PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok
for this arch.

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 18 Jun 2015.

(Sorry, hit the send button early, so here goes the actual patch.)


Index: linux/arch/m32r/include/asm/thread_info.h
===
--- linux.orig/arch/m32r/include/asm/thread_info.h
+++ linux/arch/m32r/include/asm/thread_info.h
@@ -77,7 +77,7 @@ static inline struct thread_info *curren
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti->flags = (ti->flags & (~0UL >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val << TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpaHtKBGcUeX.pgp
Description: OpenPGP digital signature

[PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok
for this arch.

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 18 Jun 2015.




pgplAgpLvXjD_.pgp
Description: OpenPGP digital signature

[PATCH] sh: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok
for this arch.

This patch is untested, because I do not have the hardware.

Resend: Patch was originally sent on Wed, 18 Jun 2015.



Index: linux/arch/sh/include/asm/thread_info.h
===
--- linux.orig/arch/sh/include/asm/thread_info.h
+++ linux/arch/sh/include/asm/thread_info.h
@@ -172,7 +172,7 @@ static inline void set_restore_sigmask(v
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti->flags = (ti->flags & (~0UL >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val << TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpL2X_WNUdZJ.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: mark ssb_bus_register as __maybe_unused

[Michael Büsch]

On Tue, 03 Nov 2015 17:42:26 +0100
Arnd Bergmann  wrote:

> On Tuesday 03 November 2015 17:27:21 Michael Büsch wrote:
> > On Tue, 03 Nov 2015 16:05:51 +0100
> > Arnd Bergmann  wrote:
> >   
> > > The SoC variant of the ssb code is now optional like the other
> > > ones, which means we can build the framwork without any
> > > front-end, but that results in a warning:
> > > 
> > > drivers/ssb/main.c:616:12: warning: 'ssb_bus_register' defined but not 
> > > used [-Wunused-function]
> > > 
> > > This annotates the ssb_bus_register function as __maybe_unused to
> > > shut up the warning. A configuration like this will not work on
> > > any hardware of course, but we still want this to silently build
> > > without warnings if the configuration is allowed in the first
> > > place.  
> > 
> > 
> > Is there a simple way to disallow this configuration?  
> 
> I could not come up with a simple one. We could turn 'CONFIG_SSB' into
> a silent option and have it selected by each bus specific driver,
> but then we also have to change all the device drivers (usb and
> wireless I guess) to use 'depends on' rather than 'select'.

The other way around?
The drivers already select SSB. However I think we should have SSB
selectable by user for the embedded case.

But well, I can live with this patch then. Kalle, you might apply it
with

Acked-by: Michael Buesch 

-- 
Michael


pgpWRfzW7n6KR.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: mark ssb_bus_register as __maybe_unused

[Michael Büsch]

On Tue, 03 Nov 2015 16:05:51 +0100
Arnd Bergmann  wrote:

> The SoC variant of the ssb code is now optional like the other
> ones, which means we can build the framwork without any
> front-end, but that results in a warning:
> 
> drivers/ssb/main.c:616:12: warning: 'ssb_bus_register' defined but not used 
> [-Wunused-function]
> 
> This annotates the ssb_bus_register function as __maybe_unused to
> shut up the warning. A configuration like this will not work on
> any hardware of course, but we still want this to silently build
> without warnings if the configuration is allowed in the first
> place.


Is there a simple way to disallow this configuration?

-- 
Michael


pgp34hM_DbgLA.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: mark ssb_bus_register as __maybe_unused

[Michael Büsch]

On Tue, 03 Nov 2015 16:05:51 +0100
Arnd Bergmann  wrote:

> The SoC variant of the ssb code is now optional like the other
> ones, which means we can build the framwork without any
> front-end, but that results in a warning:
> 
> drivers/ssb/main.c:616:12: warning: 'ssb_bus_register' defined but not used 
> [-Wunused-function]
> 
> This annotates the ssb_bus_register function as __maybe_unused to
> shut up the warning. A configuration like this will not work on
> any hardware of course, but we still want this to silently build
> without warnings if the configuration is allowed in the first
> place.


Is there a simple way to disallow this configuration?

-- 
Michael


pgp34hM_DbgLA.pgp
Description: OpenPGP digital signature

Re: [PATCH] ssb: mark ssb_bus_register as __maybe_unused

[Michael Büsch]

On Tue, 03 Nov 2015 17:42:26 +0100
Arnd Bergmann <a...@arndb.de> wrote:

> On Tuesday 03 November 2015 17:27:21 Michael Büsch wrote:
> > On Tue, 03 Nov 2015 16:05:51 +0100
> > Arnd Bergmann <a...@arndb.de> wrote:
> >   
> > > The SoC variant of the ssb code is now optional like the other
> > > ones, which means we can build the framwork without any
> > > front-end, but that results in a warning:
> > > 
> > > drivers/ssb/main.c:616:12: warning: 'ssb_bus_register' defined but not 
> > > used [-Wunused-function]
> > > 
> > > This annotates the ssb_bus_register function as __maybe_unused to
> > > shut up the warning. A configuration like this will not work on
> > > any hardware of course, but we still want this to silently build
> > > without warnings if the configuration is allowed in the first
> > > place.  
> > 
> > 
> > Is there a simple way to disallow this configuration?  
> 
> I could not come up with a simple one. We could turn 'CONFIG_SSB' into
> a silent option and have it selected by each bus specific driver,
> but then we also have to change all the device drivers (usb and
> wireless I guess) to use 'depends on' rather than 'select'.

The other way around?
The drivers already select SSB. However I think we should have SSB
selectable by user for the embedded case.

But well, I can live with this patch then. Kalle, you might apply it
with

Acked-by: Michael Buesch <m...@bues.ch>

-- 
Michael


pgpWRfzW7n6KR.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 3/3] net: wireless: b43: Statics are init to 0

[Michael Büsch]

On Mon, 19 Oct 2015 17:02:23 +0100
Paul McQuade  wrote:

> diff --git a/drivers/net/wireless/b43/phy_lp.c 
> b/drivers/net/wireless/b43/phy_lp.c
> index 058a9f2..086f0ba 100644
> --- a/drivers/net/wireless/b43/phy_lp.c
> +++ b/drivers/net/wireless/b43/phy_lp.c
> @@ -2502,7 +2502,7 @@ static int lpphy_b2063_tune(struct b43_wldev *dev,
>  {
>   struct ssb_bus *bus = dev->dev->sdev->bus;
>  
> - static const struct b206x_channel *chandata = NULL;
> + static const struct b206x_channel *chandata;
>   u32 crystal_freq = bus->chipco.pmu.crystalfreq * 1000;
>   u32 freqref, vco_freq, val1, val2, val3, timeout, timeoutref, count;
>   u16 old_comm15, scale;


Why on earth is this static anyway? That seems really wrong here.
The static should be removed (and the =NULL init be left in place.)


-- 
Michael


pgpXmHiZGiEhU.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 2/3] net: wireless: b43: Coding Style

[Michael Büsch]

On Mon, 19 Oct 2015 17:02:22 +0100
Paul McQuade  wrote:

> Fixed Pointer Coding Style
> 
> Signed-off-by: Paul McQuade 
> ---
>  drivers/net/wireless/b43/main.c | 6 +++---
>  drivers/net/wireless/b43/main.h | 2 +-
>  drivers/net/wireless/b43/xmit.h | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 2849070..040caa4 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -364,7 +364,7 @@ static struct ieee80211_supported_band 
> b43_band_2ghz_limited = {
>  
>  static void b43_wireless_core_exit(struct b43_wldev *dev);
>  static int b43_wireless_core_init(struct b43_wldev *dev);
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev);
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev);
>  static int b43_wireless_core_start(struct b43_wldev *dev);
>  static void b43_op_bss_info_changed(struct ieee80211_hw *hw,
>   struct ieee80211_vif *vif,
> @@ -989,7 +989,7 @@ static void do_key_write(struct b43_wldev *dev,
>* 0x and let's b43_op_update_tkip_key provide a
>* correct pair.
>*/
> - rx_tkip_phase1_write(dev, index, 0x, (u16*)buf);
> + rx_tkip_phase1_write(dev, index, 0x, (u16 *)buf);
>   } else if (index >= pairwise_keys_start) /* clear it */
>   rx_tkip_phase1_write(dev, index, 0, NULL);
>   if (key)
> @@ -4334,7 +4334,7 @@ out_unlock:
>  /* Locking: wl->mutex
>   * Returns the current dev. This might be different from the passed in dev,
>   * because the core might be gone away while we unlocked the mutex. */
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev)
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev)
>  {
>   struct b43_wl *wl;
>   struct b43_wldev *orig_dev;
> diff --git a/drivers/net/wireless/b43/main.h b/drivers/net/wireless/b43/main.h
> index c46430c..fa56b22 100644
> --- a/drivers/net/wireless/b43/main.h
> +++ b/drivers/net/wireless/b43/main.h
> @@ -73,7 +73,7 @@ static inline int b43_is_ofdm_rate(int rate)
>  u8 b43_ieee80211_antenna_sanitize(struct b43_wldev *dev,
> u8 antenna_nr);
>  
> -void b43_tsf_read(struct b43_wldev *dev, u64 * tsf);
> +void b43_tsf_read(struct b43_wldev *dev, u64 *tsf);
>  void b43_tsf_write(struct b43_wldev *dev, u64 tsf);
>  
>  u32 b43_shm_read32(struct b43_wldev *dev, u16 routing, u16 offset);
> diff --git a/drivers/net/wireless/b43/xmit.h b/drivers/net/wireless/b43/xmit.h
> index ba61153..fed8657 100644
> --- a/drivers/net/wireless/b43/xmit.h
> +++ b/drivers/net/wireless/b43/xmit.h
> @@ -203,7 +203,7 @@ size_t b43_txhdr_size(struct b43_wldev *dev)
>  
>  
>  int b43_generate_txhdr(struct b43_wldev *dev,
> -u8 * txhdr,
> +u8 *txhdr,
>  struct sk_buff *skb_frag,
>  struct ieee80211_tx_info *txctl, u16 cookie);
>  


Acked-by: Michael Buesch 


-- 
Michael


pgpbzKdVROeYp.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 2/3] net: wireless: b43: Coding Style

[Michael Büsch]

On Mon, 19 Oct 2015 17:02:22 +0100
Paul McQuade  wrote:

> Fixed Pointer Coding Style
> 
> Signed-off-by: Paul McQuade 
> ---
>  drivers/net/wireless/b43/main.c | 6 +++---
>  drivers/net/wireless/b43/main.h | 2 +-
>  drivers/net/wireless/b43/xmit.h | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 2849070..040caa4 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -364,7 +364,7 @@ static struct ieee80211_supported_band 
> b43_band_2ghz_limited = {
>  
>  static void b43_wireless_core_exit(struct b43_wldev *dev);
>  static int b43_wireless_core_init(struct b43_wldev *dev);
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev);
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev);
>  static int b43_wireless_core_start(struct b43_wldev *dev);
>  static void b43_op_bss_info_changed(struct ieee80211_hw *hw,
>   struct ieee80211_vif *vif,
> @@ -989,7 +989,7 @@ static void do_key_write(struct b43_wldev *dev,
>* 0x and let's b43_op_update_tkip_key provide a
>* correct pair.
>*/
> - rx_tkip_phase1_write(dev, index, 0x, (u16*)buf);
> + rx_tkip_phase1_write(dev, index, 0x, (u16 *)buf);
>   } else if (index >= pairwise_keys_start) /* clear it */
>   rx_tkip_phase1_write(dev, index, 0, NULL);
>   if (key)
> @@ -4334,7 +4334,7 @@ out_unlock:
>  /* Locking: wl->mutex
>   * Returns the current dev. This might be different from the passed in dev,
>   * because the core might be gone away while we unlocked the mutex. */
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev)
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev)
>  {
>   struct b43_wl *wl;
>   struct b43_wldev *orig_dev;
> diff --git a/drivers/net/wireless/b43/main.h b/drivers/net/wireless/b43/main.h
> index c46430c..fa56b22 100644
> --- a/drivers/net/wireless/b43/main.h
> +++ b/drivers/net/wireless/b43/main.h
> @@ -73,7 +73,7 @@ static inline int b43_is_ofdm_rate(int rate)
>  u8 b43_ieee80211_antenna_sanitize(struct b43_wldev *dev,
> u8 antenna_nr);
>  
> -void b43_tsf_read(struct b43_wldev *dev, u64 * tsf);
> +void b43_tsf_read(struct b43_wldev *dev, u64 *tsf);
>  void b43_tsf_write(struct b43_wldev *dev, u64 tsf);
>  
>  u32 b43_shm_read32(struct b43_wldev *dev, u16 routing, u16 offset);
> diff --git a/drivers/net/wireless/b43/xmit.h b/drivers/net/wireless/b43/xmit.h
> index ba61153..fed8657 100644
> --- a/drivers/net/wireless/b43/xmit.h
> +++ b/drivers/net/wireless/b43/xmit.h
> @@ -203,7 +203,7 @@ size_t b43_txhdr_size(struct b43_wldev *dev)
>  
>  
>  int b43_generate_txhdr(struct b43_wldev *dev,
> -u8 * txhdr,
> +u8 *txhdr,
>  struct sk_buff *skb_frag,
>  struct ieee80211_tx_info *txctl, u16 cookie);
>  


Acked-by: Michael Buesch 


-- 
Michael


pgpbzKdVROeYp.pgp
Description: OpenPGP digital signature

Re: [PATCH v2 3/3] net: wireless: b43: Statics are init to 0

[Michael Büsch]

On Mon, 19 Oct 2015 17:02:23 +0100
Paul McQuade  wrote:

> diff --git a/drivers/net/wireless/b43/phy_lp.c 
> b/drivers/net/wireless/b43/phy_lp.c
> index 058a9f2..086f0ba 100644
> --- a/drivers/net/wireless/b43/phy_lp.c
> +++ b/drivers/net/wireless/b43/phy_lp.c
> @@ -2502,7 +2502,7 @@ static int lpphy_b2063_tune(struct b43_wldev *dev,
>  {
>   struct ssb_bus *bus = dev->dev->sdev->bus;
>  
> - static const struct b206x_channel *chandata = NULL;
> + static const struct b206x_channel *chandata;
>   u32 crystal_freq = bus->chipco.pmu.crystalfreq * 1000;
>   u32 freqref, vco_freq, val1, val2, val3, timeout, timeoutref, count;
>   u16 old_comm15, scale;


Why on earth is this static anyway? That seems really wrong here.
The static should be removed (and the =NULL init be left in place.)


-- 
Michael


pgpXmHiZGiEhU.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/3] net: wireless: b43: Fixed Pointer issue

[Michael Büsch]

On Sun, 18 Oct 2015 00:01:37 +0100
Paul McQuade  wrote:

> Moved around pointer to avoid ERROR
> 
> Signed-off-by: Paul McQuade 
> ---
>  drivers/net/wireless/b43/dma.h  | 14 +++---
>  drivers/net/wireless/b43/main.c |  6 +++---
>  drivers/net/wireless/b43/main.h |  2 +-
>  drivers/net/wireless/b43/xmit.h |  2 +-
>  4 files changed, 12 insertions(+), 12 deletions(-)
>  
>  enum b43_dmatype {
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 2849070..040caa4 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -364,7 +364,7 @@ static struct ieee80211_supported_band 
> b43_band_2ghz_limited = {
>  
>  static void b43_wireless_core_exit(struct b43_wldev *dev);
>  static int b43_wireless_core_init(struct b43_wldev *dev);
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev);
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev);
>  static int b43_wireless_core_start(struct b43_wldev *dev);
>  static void b43_op_bss_info_changed(struct ieee80211_hw *hw,
>   struct ieee80211_vif *vif,


Can we have a sane commit message, please?
This is not an 'error' or an 'issue'. It's purely cosmetical and coding
style related.

-- 
Michael


pgpmv_EekzGNz.pgp
Description: OpenPGP digital signature

Re: [PATCH 2/3] net: wireless: b43: Fixed Pointer issue

[Michael Büsch]

On Sun, 18 Oct 2015 00:01:37 +0100
Paul McQuade  wrote:

> Moved around pointer to avoid ERROR
> 
> Signed-off-by: Paul McQuade 
> ---
>  drivers/net/wireless/b43/dma.h  | 14 +++---
>  drivers/net/wireless/b43/main.c |  6 +++---
>  drivers/net/wireless/b43/main.h |  2 +-
>  drivers/net/wireless/b43/xmit.h |  2 +-
>  4 files changed, 12 insertions(+), 12 deletions(-)
>  
>  enum b43_dmatype {
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 2849070..040caa4 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -364,7 +364,7 @@ static struct ieee80211_supported_band 
> b43_band_2ghz_limited = {
>  
>  static void b43_wireless_core_exit(struct b43_wldev *dev);
>  static int b43_wireless_core_init(struct b43_wldev *dev);
> -static struct b43_wldev * b43_wireless_core_stop(struct b43_wldev *dev);
> +static struct b43_wldev *b43_wireless_core_stop(struct b43_wldev *dev);
>  static int b43_wireless_core_start(struct b43_wldev *dev);
>  static void b43_op_bss_info_changed(struct ieee80211_hw *hw,
>   struct ieee80211_vif *vif,


Can we have a sane commit message, please?
This is not an 'error' or an 'issue'. It's purely cosmetical and coding
style related.

-- 
Michael


pgpmv_EekzGNz.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/1] ssb: remove unncessary out label

[Michael Büsch]

On Fri, 19 Jun 2015 14:23:45 +0530
Maninder Singh  wrote:

> This patch removes unnecessary label "out" and
> some restructring for using device_create_file directly.
> 
> Signed-off-by: Maninder Singh 
> Reviewed-by: Rohit Thapliyal 
> ---
>  drivers/ssb/pci.c |8 +---
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
> index 0f28c08..d6ca4d3 100644
> --- a/drivers/ssb/pci.c
> +++ b/drivers/ssb/pci.c
> @@ -1173,17 +1173,11 @@ void ssb_pci_exit(struct ssb_bus *bus)
>  int ssb_pci_init(struct ssb_bus *bus)
>  {
>   struct pci_dev *pdev;
> - int err;
>  
>   if (bus->bustype != SSB_BUSTYPE_PCI)
>   return 0;
>  
>   pdev = bus->host_pci;
>   mutex_init(>sprom_mutex);
> - err = device_create_file(>dev, _attr_ssb_sprom);
> - if (err)
> - goto out;
> -
> -out:
> - return err;
> + return device_create_file(>dev, _attr_ssb_sprom);
>  }


I don't really think this change adds any value, but if you insist on
it you get my acked-by.


-- 
Michael


pgpWxJR5QPsTs.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/1] ssb: remove unncessary out label

[Michael Büsch]

On Fri, 19 Jun 2015 14:23:45 +0530
Maninder Singh maninder...@samsung.com wrote:

 This patch removes unnecessary label out and
 some restructring for using device_create_file directly.
 
 Signed-off-by: Maninder Singh maninder...@samsung.com
 Reviewed-by: Rohit Thapliyal r.thapli...@samsung.com
 ---
  drivers/ssb/pci.c |8 +---
  1 files changed, 1 insertions(+), 7 deletions(-)
 
 diff --git a/drivers/ssb/pci.c b/drivers/ssb/pci.c
 index 0f28c08..d6ca4d3 100644
 --- a/drivers/ssb/pci.c
 +++ b/drivers/ssb/pci.c
 @@ -1173,17 +1173,11 @@ void ssb_pci_exit(struct ssb_bus *bus)
  int ssb_pci_init(struct ssb_bus *bus)
  {
   struct pci_dev *pdev;
 - int err;
  
   if (bus-bustype != SSB_BUSTYPE_PCI)
   return 0;
  
   pdev = bus-host_pci;
   mutex_init(bus-sprom_mutex);
 - err = device_create_file(pdev-dev, dev_attr_ssb_sprom);
 - if (err)
 - goto out;
 -
 -out:
 - return err;
 + return device_create_file(pdev-dev, dev_attr_ssb_sprom);
  }


I don't really think this change adds any value, but if you insist on
it you get my acked-by.


-- 
Michael


pgpWxJR5QPsTs.pgp
Description: OpenPGP digital signature

[PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok for this 
arch.

This patch is untested, because I do not have the hardware.


Index: linux/arch/m32r/include/asm/thread_info.h
===
--- linux.orig/arch/m32r/include/asm/thread_info.h
+++ linux/arch/m32r/include/asm/thread_info.h
@@ -77,7 +77,7 @@ static inline struct thread_info *curren
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti->flags = (ti->flags & (~0UL >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val << TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpNHabX6PaHP.pgp
Description: OpenPGP digital signature

[PATCH] sh: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0 >> x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin 
Signed-off-by: Michael Buesch 

---

The code also assumes sizeof(ti->flags) == 4. But that probably is ok for this 
arch.

This patch is untested, because I do not have the hardware.


Index: linux/arch/sh/include/asm/thread_info.h
===
--- linux.orig/arch/sh/include/asm/thread_info.h
+++ linux/arch/sh/include/asm/thread_info.h
@@ -172,7 +172,7 @@ static inline void set_restore_sigmask(v
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti->flags = (ti->flags & (~0UL >> (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val << TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpFjF1SHx5s0.pgp
Description: OpenPGP digital signature

[PATCH] sh: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0  x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin imir...@alum.mit.edu
Signed-off-by: Michael Buesch m...@bues.ch

---

The code also assumes sizeof(ti-flags) == 4. But that probably is ok for this 
arch.

This patch is untested, because I do not have the hardware.


Index: linux/arch/sh/include/asm/thread_info.h
===
--- linux.orig/arch/sh/include/asm/thread_info.h
+++ linux/arch/sh/include/asm/thread_info.h
@@ -172,7 +172,7 @@ static inline void set_restore_sigmask(v
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti-flags = (ti-flags  (~0  (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti-flags = (ti-flags  (~0UL  (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val  TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpFjF1SHx5s0.pgp
Description: OpenPGP digital signature

[PATCH] m32r: Fix clearing of thread info fault code

[Michael Büsch]

The expression (~0  x) will always yield all-ones, because the right
shift is an arithmetic right shift that will always shift ones in.
Hence the old fault code bits will not be cleared before being ORed
with the new fault code.

Fix this by forcing a logical right shift instead of an arithmetic
right shift by using an unsigned long constant.

Reported-by: Ilia Mirkin imir...@alum.mit.edu
Signed-off-by: Michael Buesch m...@bues.ch

---

The code also assumes sizeof(ti-flags) == 4. But that probably is ok for this 
arch.

This patch is untested, because I do not have the hardware.


Index: linux/arch/m32r/include/asm/thread_info.h
===
--- linux.orig/arch/m32r/include/asm/thread_info.h
+++ linux/arch/m32r/include/asm/thread_info.h
@@ -77,7 +77,7 @@ static inline struct thread_info *curren
 static inline void set_thread_fault_code(unsigned int val)
 {
struct thread_info *ti = current_thread_info();
-   ti-flags = (ti-flags  (~0  (32 - TI_FLAG_FAULT_CODE_SHIFT)))
+   ti-flags = (ti-flags  (~0UL  (32 - TI_FLAG_FAULT_CODE_SHIFT)))
| (val  TI_FLAG_FAULT_CODE_SHIFT);
 }
 


pgpNHabX6PaHP.pgp
Description: OpenPGP digital signature

Re: [PATCH 5/7] ssb: Use bool function return values of true/false not 1/0

[Michael Büsch]

On Mon, 30 Mar 2015 10:43:21 -0700
Joe Perches  wrote:

> Use the normal return values for bool functions
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/ssb/driver_gige.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ssb/driver_gige.c b/drivers/ssb/driver_gige.c
> index e973405..ebee6b0 100644
> --- a/drivers/ssb/driver_gige.c
> +++ b/drivers/ssb/driver_gige.c
> @@ -242,7 +242,7 @@ static int ssb_gige_probe(struct ssb_device *sdev,
>  bool pdev_is_ssb_gige_core(struct pci_dev *pdev)
>  {
>   if (!pdev->resource[0].name)
> - return 0;
> + return false;
>   return (strcmp(pdev->resource[0].name, SSB_GIGE_MEM_RES_NAME) == 0);
>  }
>  EXPORT_SYMBOL(pdev_is_ssb_gige_core);

Looks good.

Signed-off-by: Michael Buesch 

Kalle, can you take this one, please?

-- 
Michael


pgpHcotMOX0HE.pgp
Description: OpenPGP digital signature

Re: [PATCH 5/7] ssb: Use bool function return values of true/false not 1/0

[Michael Büsch]

On Mon, 30 Mar 2015 10:43:21 -0700
Joe Perches j...@perches.com wrote:

 Use the normal return values for bool functions
 
 Signed-off-by: Joe Perches j...@perches.com
 ---
  drivers/ssb/driver_gige.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/ssb/driver_gige.c b/drivers/ssb/driver_gige.c
 index e973405..ebee6b0 100644
 --- a/drivers/ssb/driver_gige.c
 +++ b/drivers/ssb/driver_gige.c
 @@ -242,7 +242,7 @@ static int ssb_gige_probe(struct ssb_device *sdev,
  bool pdev_is_ssb_gige_core(struct pci_dev *pdev)
  {
   if (!pdev-resource[0].name)
 - return 0;
 + return false;
   return (strcmp(pdev-resource[0].name, SSB_GIGE_MEM_RES_NAME) == 0);
  }
  EXPORT_SYMBOL(pdev_is_ssb_gige_core);

Looks good.

Signed-off-by: Michael Buesch m...@bues.ch

Kalle, can you take this one, please?

-- 
Michael


pgpHcotMOX0HE.pgp
Description: OpenPGP digital signature

Re: [PATCH] bcma: Kconfig: Let it depend on PCI

[Michael Büsch]

On Wed, 4 Mar 2015 14:36:10 +0100
Rafał Miłecki  wrote:

> Any other opinions?

I think this is the only way to go.
In ssb we always had optional pcicore driver, as far as I remember,
so we should have the same in bcma, too.
The old WRT54G kernel used to compile just fine with SSB and without any PCI 
enabled.

-- 
Michael


pgpJImhYmaFlG.pgp
Description: OpenPGP digital signature