From: Michael Walle <[email protected]>
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 <[email protected]>
---
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)) &&
+ !WARN_ON_ONCE(size > WILC_SDIO_BLOCK_SIZE)) {
+ need_bounce_buf = true;
+ buf = sdio_priv->dma_buffer;
+ }
+
if (cmd->read_write) { /* write */
- ret = sdio_memcpy_toio(func, cmd->address,
- (void *)cmd->buffer, size);
+ if (need_bounce_buf)
+ memcpy(buf, cmd->buffer, size);
+ ret = sdio_memcpy_toio(func, cmd->address, buf, size);
} else { /* read */
- ret = sdio_memcpy_fromio(func, (void *)cmd->buffer,
- cmd->address, size);
+ ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
+ if (need_bounce_buf)
+ memcpy(cmd->buffer, buf, size);
}
sdio_release_host(func);
@@ -127,6 +139,12 @@ static int wilc_sdio_probe(struct sdio_func *func,
if (!sdio_priv)
return -ENOMEM;
+ sdio_priv->dma_buffer = kzalloc(WILC_SDIO_BLOCK_SIZE, GFP_KERNEL);
+ if (!sdio_priv->dma_buffer) {
+ ret = -ENOMEM;
+ goto free;
+ }
+
ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO,
&wilc_hif_sdio);
if (ret)
@@ -160,6 +178,7 @@ static int wilc_sdio_probe(struct sdio_func *func,
irq_dispose_mapping(wilc->dev_irq_num);
wilc_netdev_cleanup(wilc);
free:
+ kfree(sdio_priv->dma_buffer);
kfree(sdio_priv);
return ret;
}
@@ -171,6 +190,7 @@ static void wilc_sdio_remove(struct sdio_func *func)
clk_disable_unprepare(wilc->rtc_clk);
wilc_netdev_cleanup(wilc);
+ kfree(sdio_priv->dma_buffer);
kfree(sdio_priv);
}
--
2.30.2
From: Michael Walle
> Sent: 28 July 2022 16:21
>
> From: Michael Walle <[email protected]>
>
> 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 <[email protected]>
> ---
> 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'?
> + !WARN_ON_ONCE(size > WILC_SDIO_BLOCK_SIZE)) {
That WARN() ought to be an error return?
Or just assume that large buffers will dma-capable?
David
> + need_bounce_buf = true;
> + buf = sdio_priv->dma_buffer;
> + }
> +
> if (cmd->read_write) { /* write */
> - ret = sdio_memcpy_toio(func, cmd->address,
> - (void *)cmd->buffer, size);
> + if (need_bounce_buf)
> + memcpy(buf, cmd->buffer, size);
> + ret = sdio_memcpy_toio(func, cmd->address, buf, size);
> } else { /* read */
> - ret = sdio_memcpy_fromio(func, (void *)cmd->buffer,
> - cmd->address, size);
> + ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
> + if (need_bounce_buf)
> + memcpy(cmd->buffer, buf, size);
> }
>
> sdio_release_host(func);
> @@ -127,6 +139,12 @@ static int wilc_sdio_probe(struct sdio_func *func,
> if (!sdio_priv)
> return -ENOMEM;
>
> + sdio_priv->dma_buffer = kzalloc(WILC_SDIO_BLOCK_SIZE, GFP_KERNEL);
> + if (!sdio_priv->dma_buffer) {
> + ret = -ENOMEM;
> + goto free;
> + }
> +
> ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO,
> &wilc_hif_sdio);
> if (ret)
> @@ -160,6 +178,7 @@ static int wilc_sdio_probe(struct sdio_func *func,
> irq_dispose_mapping(wilc->dev_irq_num);
> wilc_netdev_cleanup(wilc);
> free:
> + kfree(sdio_priv->dma_buffer);
> kfree(sdio_priv);
> return ret;
> }
> @@ -171,6 +190,7 @@ static void wilc_sdio_remove(struct sdio_func *func)
>
> clk_disable_unprepare(wilc->rtc_clk);
> wilc_netdev_cleanup(wilc);
> + kfree(sdio_priv->dma_buffer);
> kfree(sdio_priv);
> }
>
> --
> 2.30.2
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Am 29. Juli 2022 11:51:12 MESZ schrieb David Laight <[email protected]>:
>From: Michael Walle
>> Sent: 28 July 2022 16:21
>>
>> From: Michael Walle <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.
>
>> + !WARN_ON_ONCE(size > WILC_SDIO_BLOCK_SIZE)) {
>
>That WARN() ought to be an error return?
It will just behave as before. I noticed it *will* work in some cases, although the object is on the stack. I mean the driver seems to work fine at least on some SoCs. So I didn't want to change the behavior if the bounce buffer is too small. Of course we could also return an error here. All the calls with stack adresses I've seen for now were the register reads and writes and the txq handling (the vmm_tables IIRC).
>Or just assume that large buffers will dma-capable?
See above. It should be large enough. But I didn't audit everything.
-michael
>
> David
>
>> + need_bounce_buf = true;
>> + buf = sdio_priv->dma_buffer;
>> + }
>> +
>> if (cmd->read_write) { /* write */
>> - ret = sdio_memcpy_toio(func, cmd->address,
>> - (void *)cmd->buffer, size);
>> + if (need_bounce_buf)
>> + memcpy(buf, cmd->buffer, size);
>> + ret = sdio_memcpy_toio(func, cmd->address, buf, size);
>> } else { /* read */
>> - ret = sdio_memcpy_fromio(func, (void *)cmd->buffer,
>> - cmd->address, size);
>> + ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
>> + if (need_bounce_buf)
>> + memcpy(cmd->buffer, buf, size);
>> }
>>
>> sdio_release_host(func);
>> @@ -127,6 +139,12 @@ static int wilc_sdio_probe(struct sdio_func *func,
>> if (!sdio_priv)
>> return -ENOMEM;
>>
>> + sdio_priv->dma_buffer = kzalloc(WILC_SDIO_BLOCK_SIZE, GFP_KERNEL);
>> + if (!sdio_priv->dma_buffer) {
>> + ret = -ENOMEM;
>> + goto free;
>> + }
>> +
>> ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO,
>> &wilc_hif_sdio);
>> if (ret)
>> @@ -160,6 +178,7 @@ static int wilc_sdio_probe(struct sdio_func *func,
>> irq_dispose_mapping(wilc->dev_irq_num);
>> wilc_netdev_cleanup(wilc);
>> free:
>> + kfree(sdio_priv->dma_buffer);
>> kfree(sdio_priv);
>> return ret;
>> }
>> @@ -171,6 +190,7 @@ static void wilc_sdio_remove(struct sdio_func *func)
>>
>> clk_disable_unprepare(wilc->rtc_clk);
>> wilc_netdev_cleanup(wilc);
>> + kfree(sdio_priv->dma_buffer);
>> kfree(sdio_priv);
>> }
>>
>> --
>> 2.30.2
>
>-
>Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>Registration No: 1397386 (Wales)
>
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 <[email protected]>:
>> From: Michael Walle
>>> Sent: 28 July 2022 16:21
>>>
>>> From: Michael Walle <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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)
+{
+ struct sdio_func *func = container_of(wilc->dev, struct sdio_func,
dev);
+ int size, ret;
+ struct wilc_sdio *sdio_priv = wilc->bus_data;
+ u8 *buf;
+
+ sdio_claim_host(func);
+
+ func->num = cmd->function;
+ func->cur_blksize = cmd->block_size;
+ size = cmd->count;
+ buf = sdio_priv->dma_buffer; /* use allocated memory */
+
+ if (cmd->read_write) { /* write */
+ memcpy(buf, cmd->buffer, size);
+ ret = sdio_memcpy_toio(func, cmd->address, buf, size);
+ } else { /* read */
+ ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
+ memcpy(cmd->buffer, buf, size);
+ }
+
+ sdio_release_host(func);
+
+ if (ret)
+ dev_err(&func->dev, "%s..failed, err(%d)\n", __func__, ret);
+
+ return ret;
+}
+
static int wilc_sdio_probe(struct sdio_func *func,
const struct sdio_device_id *id)
{
@@ -128,10 +159,16 @@ static int wilc_sdio_probe(struct sdio_func *func,
if (!sdio_priv)
return -ENOMEM;
+ sdio_priv->dma_buffer = kzalloc(WILC_SDIO_BLOCK_SIZE, GFP_KERNEL);
+ if (!sdio_priv->dma_buffer) {
+ ret = -ENOMEM;
+ goto free;
+ }
+
ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO,
&wilc_hif_sdio);
if (ret)
- goto free;
+ goto free_dma_buffer;
if (IS_ENABLED(CONFIG_WILC1000_HW_OOB_INTR)) {
struct device_node *np = func->card->dev.of_node;
@@ -160,6 +197,8 @@ static int wilc_sdio_probe(struct sdio_func *func,
dispose_irq:
irq_dispose_mapping(wilc->dev_irq_num);
wilc_netdev_cleanup(wilc);
+free_dma_buffer:
+ kfree(sdio_priv->dma_buffer);
free:
kfree(sdio_priv);
return ret;
@@ -172,6 +211,7 @@ static void wilc_sdio_remove(struct sdio_func *func)
clk_disable_unprepare(wilc->rtc_clk);
wilc_netdev_cleanup(wilc);
+ kfree(sdio_priv->dma_buffer);
kfree(sdio_priv);
}
@@ -378,7 +418,7 @@ static int wilc_sdio_write_reg(struct wilc *wilc,
u32 addr, u32 data)
cmd.count = 4;
cmd.buffer = (u8 *)&data;
cmd.block_size = sdio_priv->block_size;
- ret = wilc_sdio_cmd53(wilc, &cmd);
+ ret = wilc_sdio_cmd53_extend(wilc, &cmd);
if (ret)
dev_err(&func->dev,
"Failed cmd53, write reg (%08x)...\n", addr);
@@ -496,7 +536,7 @@ static int wilc_sdio_read_reg(struct wilc *wilc, u32
addr, u32 *data)
cmd.buffer = (u8 *)data;
cmd.block_size = sdio_priv->block_size;
- ret = wilc_sdio_cmd53(wilc, &cmd);
+ ret = wilc_sdio_cmd53_extend(wilc, &cmd);
if (ret) {
dev_err(&func->dev,
"Failed cmd53, read reg (%08x)...\n", addr);
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
b/drivers/net/wireless/microchip/wilc1000/wlan.c
index 947d9a0a494e..0576d865c603 100644
--- a/drivers/net/wireless/microchip/wilc1000/wlan.c
+++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
@@ -714,7 +714,7 @@ int wilc_wlan_handle_txq(struct wilc *wilc, u32
*txq_count)
int ret = 0;
int counter;
int timeout;
- u32 vmm_table[WILC_VMM_TBL_SIZE];
+ u32 *vmm_table = wilc->vmm_table;
u8 ac_pkt_num_to_chip[NQUEUES] = {0, 0, 0, 0};
const struct wilc_hif_func *func;
int srcu_idx;
--
2.34.1
Hi Michael,
I love your patch! Yet something to improve:
[auto build test ERROR on wireless/main]
[also build test ERROR on linus/master v5.19-rc8 next-20220728]
[cannot apply to wireless-next/main]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Michael-Walle/wilc1000-fix-DMA-on-stack-objects/20220728-232309
base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless.git main
config: nios2-allyesconfig (https://download.01.org/0day-ci/archive/20220731/[email protected]/config)
compiler: nios2-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c04b2e109aebded7849c37f13a3ab7b76b4c0496
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Michael-Walle/wilc1000-fix-DMA-on-stack-objects/20220728-232309
git checkout c04b2e109aebded7849c37f13a3ab7b76b4c0496
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/net/wireless/microchip/wilc1000/sdio.c: In function 'wilc_sdio_cmd53':
>> drivers/net/wireless/microchip/wilc1000/sdio.c:107:39: error: implicit declaration of function 'object_is_on_stack' [-Werror=implicit-function-declaration]
107 | if ((!virt_addr_valid(buf) || object_is_on_stack(buf)) &&
| ^~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/object_is_on_stack +107 drivers/net/wireless/microchip/wilc1000/sdio.c
89
90 static int wilc_sdio_cmd53(struct wilc *wilc, struct sdio_cmd53 *cmd)
91 {
92 struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
93 struct wilc_sdio *sdio_priv = wilc->bus_data;
94 bool need_bounce_buf = false;
95 u8 *buf = cmd->buffer;
96 int size, ret;
97
98 sdio_claim_host(func);
99
100 func->num = cmd->function;
101 func->cur_blksize = cmd->block_size;
102 if (cmd->block_mode)
103 size = cmd->count * cmd->block_size;
104 else
105 size = cmd->count;
106
> 107 if ((!virt_addr_valid(buf) || object_is_on_stack(buf)) &&
108 !WARN_ON_ONCE(size > WILC_SDIO_BLOCK_SIZE)) {
109 need_bounce_buf = true;
110 buf = sdio_priv->dma_buffer;
111 }
112
113 if (cmd->read_write) { /* write */
114 if (need_bounce_buf)
115 memcpy(buf, cmd->buffer, size);
116 ret = sdio_memcpy_toio(func, cmd->address, buf, size);
117 } else { /* read */
118 ret = sdio_memcpy_fromio(func, buf, cmd->address, size);
119 if (need_bounce_buf)
120 memcpy(cmd->buffer, buf, size);
121 }
122
123 sdio_release_host(func);
124
125 if (ret)
126 dev_err(&func->dev, "%s..failed, err(%d)\n", __func__, ret);
127
128 return ret;
129 }
130
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Am 2022-07-29 17:39, schrieb [email protected]:
> 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
>> <[email protected]>:
>>> From: Michael Walle
>>>> Sent: 28 July 2022 16:21
>>>>
>>>> From: Michael Walle <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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.
-michael
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 [email protected]:
>> 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
>>> <[email protected]>:
>>>> From: Michael Walle
>>>>> Sent: 28 July 2022 16:21
>>>>>
>>>>> From: Michael Walle <[email protected]>
>>>>>
>>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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. I would
need the help to test and confirm if the modified patch do solve the
issue with imx8mn.
Regards,
Ajay
Am 2022-08-04 14:43, schrieb [email protected]:
> 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 [email protected]:
>>> 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
>>>> <[email protected]>:
>>>>> From: Michael Walle
>>>>>> Sent: 28 July 2022 16:21
>>>>>>
>>>>>> From: Michael Walle <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
On 04/08/22 18:26, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> Am 2022-08-04 14:43, schrieb [email protected]:
>> 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 [email protected]:
>>>> 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
>>>>> <[email protected]>:
>>>>>> From: Michael Walle
>>>>>>> Sent: 28 July 2022 16:21
>>>>>>>
>>>>>>> From: Michael Walle <[email protected]>
>>>>>>>
>>>>>>> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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?
Yes, I tested by bringing the interface up on SAMA5D4 Xplained host.
>
>> 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.
Sure. Thanks.
Regards,
Ajay