Am 2022-08-04 14:43, schrieb Ajay.Kathat@xxxxxxxxxxxxx:
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@xxxxxxxxxxxxx:
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@xxxxxxxxxx>:
From: Michael Walle
Sent: 28 July 2022 16:21
From: Michael Walle <mwalle@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
