Re: [PATCH] wilc1000: fix DMA on stack objects

[Michael Walle <michael@walle.cc>]

Am 2022-08-04 14:43, schrieb Ajay.Kathat@microchip.com:
> On 04/08/22 12:52, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>> 
>> Am 2022-07-29 17:39, schrieb Ajay.Kathat@microchip.com:
>>> On 29/07/22 20:28, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Am 29. Juli 2022 11:51:12 MESZ schrieb David Laight
>>>> <David.Laight@ACULAB.COM>:
>>>>> From: Michael Walle
>>>>>> Sent: 28 July 2022 16:21
>>>>>> 
>>>>>> From: Michael Walle <mwalle@kernel.org>
>>>>>> 
>>>>>> Sometimes wilc_sdio_cmd53() is called with addresses pointing to 
>>>>>> an
>>>>>> object on the stack. E.g. wilc_sdio_write_reg() will call it with 
>>>>>> an
>>>>>> address pointing to one of its arguments. Detect whether the 
>>>>>> buffer
>>>>>> address is not DMA-able in which case a bounce buffer is used. The
>>>>>> bounce
>>>>>> buffer itself is protected from parallel accesses by
>>>>>> sdio_claim_host().
>>>>>> 
>>>>>> Fixes: 5625f965d764 ("wilc1000: move wilc driver out of staging")
>>>>>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>>>>>> ---
>>>>>> The bug itself probably goes back way more, but I don't know if it
>>>>>> makes
>>>>>> any sense to use an older commit for the Fixes tag. If so, please
>>>>>> suggest
>>>>>> one.
>>>>>> 
>>>>>> The bug leads to an actual error on an imx8mn SoC with 1GiB of 
>>>>>> RAM.
>>>>>> But the
>>>>>> error will also be catched by CONFIG_DEBUG_VIRTUAL:
>>>>>> [    9.817512] virt_to_phys used for non-linear address:
>>>>>> (____ptrval____) (0xffff80000a94bc9c)
>>>>>> 
>>>>>>   .../net/wireless/microchip/wilc1000/sdio.c    | 28
>>>>>> ++++++++++++++++---
>>>>>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c
>>>>>> b/drivers/net/wireless/microchip/wilc1000/sdio.c
>>>>>> index 7962c11cfe84..e988bede880c 100644
>>>>>> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
>>>>>> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
>>>>>> @@ -27,6 +27,7 @@ struct wilc_sdio {
>>>>>>       bool irq_gpio;
>>>>>>       u32 block_size;
>>>>>>       int has_thrpt_enh3;
>>>>>> +    u8 *dma_buffer;
>>>>>>   };
>>>>>> 
>>>>>>   struct sdio_cmd52 {
>>>>>> @@ -89,6 +90,9 @@ static int wilc_sdio_cmd52(struct wilc *wilc,
>>>>>> struct sdio_cmd52 *cmd)
>>>>>>   static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53
>>>>>> *cmd)
>>>>>>   {
>>>>>>       struct sdio_func *func = container_of(wilc->dev, struct
>>>>>> sdio_func, dev);
>>>>>> +    struct wilc_sdio *sdio_priv = wilc->bus_data;
>>>>>> +    bool need_bounce_buf = false;
>>>>>> +    u8 *buf = cmd->buffer;
>>>>>>       int size, ret;
>>>>>> 
>>>>>>       sdio_claim_host(func);
>>>>>> @@ -100,12 +104,20 @@ static int wilc_sdio_cmd53(struct wilc 
>>>>>> *wilc,
>>>>>> struct sdio_cmd53 *cmd)
>>>>>>       else
>>>>>>               size = cmd->count;
>>>>>> 
>>>>>> +    if ((!virt_addr_valid(buf) || object_is_on_stack(buf)) &&
>>>>> How cheap are the above tests?
>>>>> It might just be worth always doing the 'bounce'?
>>>> I'm not sure how cheap they are, but I don't think it costs more 
>>>> than
>>>> copying the bulk data around. That's up to the maintainer to decide.
>>> 
>>> 
>>> I think, the above checks for each CMD53 might add up to the 
>>> processing
>>> time of this function. These checks can be avoided, if we add new
>>> function similar to 'wilc_sdio_cmd53' which can be called when the
>>> local
>>> variables are used. Though we have to perform the memcpy operation
>>> which
>>> is anyway required to handle this scenario for small size data.
>>> 
>>> Mostly, either the static global data or dynamically allocated buffer
>>> is
>>> used with cmd53 except wilc_sdio_write_reg, wilc_sdio_read_reg
>>> wilc_wlan_handle_txq functions.
>>> 
>>> I have created a patch using the above approach which can fix this
>>> issue
>>> and will have no or minimal impact on existing functionality. The 
>>> same
>>> is copied below:
>>> 
>>> 
>>> ---
>>>   .../net/wireless/microchip/wilc1000/netdev.h  |  1 +
>>>   .../net/wireless/microchip/wilc1000/sdio.c    | 46
>>> +++++++++++++++++--
>>>   .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>>>   3 files changed, 45 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h
>>> b/drivers/net/wireless/microchip/wilc1000/netdev.h
>>> index 43c085c74b7a..2137ef294953 100644
>>> --- a/drivers/net/wireless/microchip/wilc1000/netdev.h
>>> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
>>> @@ -245,6 +245,7 @@ struct wilc {
>>>       u8 *rx_buffer;
>>>       u32 rx_buffer_offset;
>>>       u8 *tx_buffer;
>>> +    u32 vmm_table[WILC_VMM_TBL_SIZE];
>>> 
>>>       struct txq_handle txq[NQUEUES];
>>>       int txq_entries;
>>> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c
>>> b/drivers/net/wireless/microchip/wilc1000/sdio.c
>>> index 600cc57e9da2..19d4350ecc22 100644
>>> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
>>> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
>>> @@ -28,6 +28,7 @@ struct wilc_sdio {
>>>       u32 block_size;
>>>       bool isinit;
>>>       int has_thrpt_enh3;
>>> +    u8 *dma_buffer;
>>>   };
>>> 
>>>   struct sdio_cmd52 {
>>> @@ -117,6 +118,36 @@ static int wilc_sdio_cmd53(struct wilc *wilc,
>>> struct sdio_cmd53 *cmd)
>>>       return ret;
>>>   }
>>> 
>>> +static int wilc_sdio_cmd53_extend(struct wilc *wilc, struct 
>>> sdio_cmd53
>>> *cmd)
>> 
>> If you handle all the stack cases anyway, the caller can just use
>> a bounce buffer and you don't need to duplicate the function.
> 
> 
> Thanks. Indeed, the duplicate function can be avoided. I will update 
> the
> patch and send modified patch for the review.
> Btw, I was trying to reproduce the warning message by enabling
> CONFIG_DEBUG_VIRTUAL config but no luck. It seems enabling the config 
> is
> not enough to test on my host or may be I am missing something.

Did you bring the interface up?

> I would
> need the help to test and confirm if the modified patch do solve the
> issue with imx8mn.

sure, just put me on cc and i can test it on my board.

-michael