Messages in this thread | | | Date | Thu, 04 Aug 2022 09:22:51 +0200 | From | Michael Walle <> | Subject | Re: [PATCH] wilc1000: fix DMA on stack objects |
| |
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.
-michael
| |