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'?
> + !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)
