Re: [PATCH 2/3] rsi: sdio: Add WOWLAN support for S4 hibernate state

[Amitkumar Karwar]

On Tue, Sep 26, 2017 at 3:27 PM, Kalle Valo  wrote:
> Amitkumar Karwar  writes:
>
>> From: Karun Eagalapati 
>>
>> We are disabling of interrupts from firmware in freeze handler.
>> Also setting power management capability KEEP_MMC_POWER to make
>> device wakeup for WoWLAN trigger.
>> At restore, we observed a device reset on some platforms. Hence
>> reloading of firmware and device initialization is performed.
>
> With "reloading of firmware and device initialization" you mean calling
> rsi_mac80211_detach()?

After rsi_mac80211_detach, we have a call for rsi_hal_device_init() in
which reloading of firmware happens.

>
> void rsi_mac80211_detach(struct rsi_hw *adapter)
> {
> struct ieee80211_hw *hw = adapter->hw;
> enum nl80211_band band;
>
> if (hw) {
> ieee80211_stop_queues(hw);
> ieee80211_unregister_hw(hw);
> ieee80211_free_hw(hw);
> }
>
> That looks like a quite heavy sledgehammer workaround to a resume
> problem. Is it really the best way to handle this?

I agree with you. I will appreciate if someone propose a better
solution. Following are details about the problem.

Connection remain alive after suspend(S4 state). System is resumed
from S4 through GPIO pin after receiving magic packet. But SDIO
doesn't work after this. We need to do perform SDIO reset and
redownload the firmware. Mac80211 is under impression that connection
is alive. We keep on getting mac80211 handler calls and data while
firmware re-download is in progress. This leads to different race
issues and occasionally kernel crashes. detaching from mac80211 solves
the problem here.

>
> And even if that would be the right approach it needs to be properly
> described in the commit log, a vague sentence in the end of a commit log
> is not enough.

Understood. I will add detailed description and send updated version
if the patch is fine.

Regards,
Amitkumar Karwar

Re: [PATCH 1/3] rsi: sdio: add WOWLAN support for S3 suspend state

[Amitkumar Karwar]

On Tue, Sep 26, 2017 at 3:29 PM, Kalle Valo  wrote:
> Amitkumar Karwar  writes:
>
>> From: Karun Eagalapati 
>>
>> WoWLAN is supported in RS9113 device through GPIO pin2.
>> wowlan config frame is internally sent to firmware in mac80211
>> suspend handler. Also beacon miss threshold and keep-alive time
>> values are increased to avoid un-necessary disconnection with AP.
>>
>> Signed-off-by: Karun Eagalapati 
>> Signed-off-by: Amitkumar Karwar 
>> ---
>>  drivers/net/wireless/rsi/Kconfig|   7 ++
>>  drivers/net/wireless/rsi/rsi_91x_core.c |   9 +-
>>  drivers/net/wireless/rsi/rsi_91x_mac80211.c | 125 
>> 
>>  drivers/net/wireless/rsi/rsi_91x_mgmt.c |  66 +--
>>  drivers/net/wireless/rsi/rsi_91x_sdio.c |  11 +++
>>  drivers/net/wireless/rsi/rsi_common.h   |   3 +
>>  drivers/net/wireless/rsi/rsi_main.h |  12 ++-
>>  drivers/net/wireless/rsi/rsi_mgmt.h |  37 +++-
>>  8 files changed, 258 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rsi/Kconfig 
>> b/drivers/net/wireless/rsi/Kconfig
>> index 7c5e4ca..0882b4e 100644
>> --- a/drivers/net/wireless/rsi/Kconfig
>> +++ b/drivers/net/wireless/rsi/Kconfig
>> @@ -42,4 +42,11 @@ config RSI_USB
>> This option enables the USB bus support in rsi drivers.
>> Select M (recommended), if you have a RSI 1x1 wireless module.
>>
>> +config RSI_WOW
>> + bool "Redpine Signals WOWLAN support"
>> + depends on RSI_SDIO && RSI_91X
>> + default y
>> + ---help---
>> +  Say Y, if you would like to enable wowlan support for RSI module.
>
> This adds a lot of ifdefs to the code. Is the Kconfig option really
> needed?

Thanks for the review.
Kconfig option can be removed. I will get rid of it.

Regards,
Amitkumar Karwar

Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN

[Arnd Bergmann]

On Tue, Sep 26, 2017 at 9:49 AM, Andrey Ryabinin
 wrote:
>
>
> On 09/26/2017 09:47 AM, Arnd Bergmann wrote:
>> On Mon, Sep 25, 2017 at 11:32 PM, Arnd Bergmann  wrote:

>> +   ret = __builtin_strlen(q);
>
>
> I think this is not correct. Fortified strlen called here on purpose. If 
> sizeof q is known at compile time
> and 'q' contains not-null fortified strlen() will panic.

Ok, got it.

>> if (size) {
>> size_t len = (ret >= size) ? size - 1 : ret;
>> if (__builtin_constant_p(len) && len >= p_size)
>>
>> The problem is apparently that the fortified strlcpy calls the fortified 
>> strlen,
>> which in turn calls strnlen and that ends up calling the extern 
>> '__real_strnlen'
>> that gcc cannot reduce to a constant expression for a constant input.
>
>
> Per my observation, it's the code like this:
> if ()
> fortify_panic(__func__);
>
>
> somehow prevent gcc to merge several "struct i2c_board_info info;" into one 
> stack slot.
> With the hack bellow, stack usage reduced to ~1,6K:

1.6k is also what I see with my patch, or any other approach I tried
that changes
string.h. With the split up em28xx_dvb_init() function (and without
changes to string.h),
I got down to a few hundred bytes for the largest handler.

> ---
>  include/linux/string.h | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 54d21783e18d..9a96ff3ebf94 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -261,8 +261,6 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
> if (p_size == (size_t)-1)
> return __builtin_strlen(p);
> ret = strnlen(p, p_size);
> -   if (p_size <= ret)
> -   fortify_panic(__func__);
> return ret;
>  }
>
> @@ -271,8 +269,6 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, 
> __kernel_size_t maxlen)
>  {
> size_t p_size = __builtin_object_size(p, 0);
> __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : 
> p_size);
> -   if (p_size <= ret && maxlen != ret)
> -   fortify_panic(__func__);
> return ret;

I've reduced it further to this change:

--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -227,7 +227,7 @@ static inline const char *kbasename(const char *path)
 #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
 #define __RENAME(x) __asm__(#x)

-void fortify_panic(const char *name) __noreturn __cold;
+void fortify_panic(const char *name) __cold;
 void __read_overflow(void) __compiletime_error("detected read beyond
size of object passed as 1st parameter");
 void __read_overflow2(void) __compiletime_error("detected read beyond
size of object passed as 2nd parameter");
 void __read_overflow3(void) __compiletime_error("detected read beyond
size of object passed as 3rd parameter");

I don't immediately see why the __noreturn changes the behavior here, any idea?

>> Not sure if that change is the best fix, but it seems to address the problem 
>> in
>> this driver and probably leads to better code in other places as well.
>>
>
> Probably it would be better to solve this on the strlcpy side, but I haven't 
> found the way to do this right.
> Alternative solutions:
>
>  - use memcpy() instead of strlcpy(). All source strings are smaller than 
> I2C_NAME_SIZE, so we could
>do something like this - memcpy(info.type, "si2168", sizeof("si2168"));
>Also this should be faster.

This would be very similar to the patch I posted at the start of this
thread to use strncpy(), right?
I was hoping that changing strlcpy() here could also improve other
users that might run into
the same situation, but stay below the 2048-byte stack frame limit.

>  - Move code under different "case:" in the switch(dev->model) to the 
> separate function should help as well.
>But it might be harder to backport into stables.

Agreed, I posted this in earlier versions of the patch series, see
https://patchwork.kernel.org/patch/9601025/

The new patch was a result of me trying to come up with a less
invasive version to
make it easier to backport, since I would like to backport the last
patch in the series
that depends on all the earlier ones.

 Arnd

Re: [PATCH] PCI MSI: allow alignment restrictions on vector allocation

[Thomas Gleixner]

On Mon, 25 Sep 2017, Daniel Drake wrote:

Please send x86 related patches to LKML as documented in MAINTAINERS. That
patch is mainly x86 and not PCI.

> ath9k hardware claims to support up to 4 MSI vectors, and when run in
> that configuration, it would be allowed to modify the lower bits of the
> MSI Message Data when generating interrupts in order to signal which
> of the 4 vectors the interrupt is being raised on.
> 
> Linux's PCI-MSI irqchip only supports a single MSI vector for each
> device, and it tells the device this, but the device appears to assume
> it is working with 4, as it will unset the lower 2 bits of Message Data
> presumably to indicate that it is an IRQ for the first of 4 possible
> vectors.
> 
> Linux will then receive an interrupt on the wrong vector, so the
> ath9k interrupt handler will not be invoked.
> 
> To work around this, introduce a mechanism where the vector assignment
> algorithm can be restricted to only a subset of available vector numbers
> based on a bitmap.
> 
> As a user of this bitmap, introduce a pci_dev.align_msi_vector flag which
> can be used to state that MSI vector numbers must be aligned to a specific
> amount. If we 4-align the ath9k MSI vector then the lower bits will
> already be 0 and hence the device will not modify the Message Data away
> from its original value.
> 
> This is needed in order to support the wifi card in at least 8 new Acer
> consumer laptop models which come with the Foxconn NFA335 WiFi module.
> Legacy interrupts do not work on that module, so MSI support is required.

Sigh, what a mess.

> diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
> index 6dfe366a8804..7f35178586a1 100644
> --- a/arch/x86/include/asm/hw_irq.h
> +++ b/arch/x86/include/asm/hw_irq.h
> @@ -77,6 +77,7 @@ struct irq_alloc_info {
>   struct {
>   struct pci_dev  *msi_dev;
>   irq_hw_number_t msi_hwirq;
> + DECLARE_BITMAP(allowed_vectors, NR_VECTORS);

Uurgh. No. You want to express an alignment requirement. Why would you need
a bitmap for that? A simple

unsigned int align_vector;

is sufficient.


> @@ -178,6 +179,9 @@ static int __assign_irq_vector(int irq, struct 
> apic_chip_data *d,
>   if (test_bit(vector, used_vectors))
>   goto next;
>  
> + if (allowed_vectors && !test_bit(vector, allowed_vectors))
> + goto next;

This code is gone in -next and replaced by a new vector allocator.

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/apic

> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index f68c58a93dd0..7755450be02d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -419,6 +419,8 @@ struct pci_dev {
>  #endif
>  #ifdef CONFIG_PCI_MSI
>   const struct attribute_group **msi_irq_groups;
> + int align_msi_vector;   /* device requires MSI vector numbers aligned
> +  * by this amount */

This wants to be unsigned int and please get rid of this butt ugly
formatted comment.

But the real question is how this is supposed to work with

 - interrupt remapping

 - non X86 machines

I doubt that this can be made generic enough to make it work all over the
place. Tell Acer/Foxconn to fix their stupid firmware.

Thanks,

tglx

[PATCH] bcma: keep *config menu together

[Randy Dunlap]

From: Randy Dunlap 

Use "if BCMA"/"endif" around all Kconfig symbols so that they are
kept together in *config menus instead of showing up in unexpected
places. Also remove "depends on BCMA" since this is handled by the
"if BCMA" addition.

Tested with ARCH={x86_64,MIPS} using make {n,menu,g,x}config.

Signed-off-by: Randy Dunlap 
Cc: Rafał Miłecki 
---
 drivers/bcma/Kconfig |   18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

--- lnx-414-rc2.orig/drivers/bcma/Kconfig
+++ lnx-414-rc2/drivers/bcma/Kconfig
@@ -10,14 +10,15 @@ menuconfig BCMA
  Bus driver for Broadcom specific Advanced Microcontroller Bus
  Architecture.
 
+if BCMA
+
 # Support for Block-I/O. SELECT this from the driver that needs it.
 config BCMA_BLOCKIO
bool
-   depends on BCMA
 
 config BCMA_HOST_PCI_POSSIBLE
bool
-   depends on BCMA && PCI = y
+   depends on PCI = y
default y
 
 config BCMA_HOST_PCI
@@ -28,7 +29,6 @@ config BCMA_HOST_PCI
 
 config BCMA_HOST_SOC
bool "Support for BCMA in a SoC"
-   depends on BCMA
help
  Host interface for a Broadcom AIX bus directly mapped into
  the memory. This only works with the Broadcom SoCs from the
@@ -38,7 +38,7 @@ config BCMA_HOST_SOC
 
 config BCMA_DRIVER_PCI
bool "BCMA Broadcom PCI core driver"
-   depends on BCMA && PCI
+   depends on PCI
default y
help
  BCMA bus may have many versions of PCIe core. This driver
@@ -54,13 +54,13 @@ config BCMA_DRIVER_PCI
 
 config BCMA_DRIVER_PCI_HOSTMODE
bool "Driver for PCI core working in hostmode"
-   depends on BCMA && MIPS && BCMA_DRIVER_PCI
+   depends on MIPS && BCMA_DRIVER_PCI
help
  PCI core hostmode operation (external PCI bus).
 
 config BCMA_DRIVER_MIPS
bool "BCMA Broadcom MIPS core driver"
-   depends on BCMA && MIPS
+   depends on MIPS
help
  Driver for the Broadcom MIPS core attached to Broadcom specific
  Advanced Microcontroller Bus.
@@ -91,7 +91,6 @@ config BCMA_NFLASH
 
 config BCMA_DRIVER_GMAC_CMN
bool "BCMA Broadcom GBIT MAC COMMON core driver"
-   depends on BCMA
help
  Driver for the Broadcom GBIT MAC COMMON core attached to Broadcom
  specific Advanced Microcontroller Bus.
@@ -100,7 +99,7 @@ config BCMA_DRIVER_GMAC_CMN
 
 config BCMA_DRIVER_GPIO
bool "BCMA GPIO driver"
-   depends on BCMA && GPIOLIB
+   depends on GPIOLIB
select GPIOLIB_IRQCHIP if BCMA_HOST_SOC
help
  Driver to provide access to the GPIO pins of the bcma bus.
@@ -109,8 +108,9 @@ config BCMA_DRIVER_GPIO
 
 config BCMA_DEBUG
bool "BCMA debugging"
-   depends on BCMA
help
  This turns on additional debugging messages.
 
  If unsure, say N
+
+endif # BCMA