On 04/08/22 19:55, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> Hi,
>
> thanks for the patch!
>
> Am 2022-08-04 15:18, schrieb Ajay.Kathat@xxxxxxxxxxxxx:
>> From: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
>>
>> Sometimes 'wilc_sdio_cmd53' is called with addresses pointing to an
>> object on the stack. Use dynamically allocated memory for cmd53 instead
>> of stack address which is not DMA'able.
>>
>> Fixes: 5625f965d764 ("wilc1000: move wilc driver out of staging")
>> Reported-by: Michael Walle <mwalle@xxxxxxxxxx>
>> Suggested-by: Michael Walle <mwalle@xxxxxxxxxx>
>> Signed-off-by: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
>> ---
>>
>> This patch is created based on [1] and changes are done as discussed in
>> the same thread.
>>
>> [1].
>> https://patchwork.kernel.org/project/linux-wireless/patch/20220728152037.386543-1-michael@xxxxxxxx/
>>
>>
>> .../net/wireless/microchip/wilc1000/netdev.h | 1 +
>> .../net/wireless/microchip/wilc1000/sdio.c | 23 +++++++++++++++----
>> .../net/wireless/microchip/wilc1000/wlan.c | 2 +-
>> 3 files changed, 20 insertions(+), 6 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];
>
> Shouldn't this be "u32 *vmm_table" judging by the
> other members of this structure.
>
Okay. I will change it.
>> 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..8bb0b8e189af 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 *cmd53_buf;
>> };
>>
>> struct sdio_cmd52 {
>> @@ -128,10 +129,16 @@ static int wilc_sdio_probe(struct sdio_func
>> *func,
>> if (!sdio_priv)
>> return -ENOMEM;
>>
>> + sdio_priv->cmd53_buf = kzalloc(sizeof(u32), GFP_KERNEL);
>> + if (!sdio_priv->cmd53_buf) {
>> + ret = -ENOMEM;
>> + goto free;
>> + }
>> +
>> ret = wilc_cfg80211_init(&wilc, &func->dev, WILC_HIF_SDIO,
>> &wilc_hif_sdio);
>> if (ret)
>> - goto free;
>
> just use "goto free;". kfree(NULL) is a noop.
>
Ok
>> + goto free_buffer;
>>
>> if (IS_ENABLED(CONFIG_WILC1000_HW_OOB_INTR)) {
>> struct device_node *np = func->card->dev.of_node;
>> @@ -160,6 +167,8 @@ static int wilc_sdio_probe(struct sdio_func *func,
>> dispose_irq:
>> irq_dispose_mapping(wilc->dev_irq_num);
>> wilc_netdev_cleanup(wilc);
>> +free_buffer:
>> + kfree(sdio_priv->cmd53_buf);
>> free:
>> kfree(sdio_priv);
>> return ret;
>> @@ -172,6 +181,7 @@ static void wilc_sdio_remove(struct sdio_func
>> *func)
>>
>> clk_disable_unprepare(wilc->rtc_clk);
>> wilc_netdev_cleanup(wilc);
>> + kfree(sdio_priv->cmd53_buf);
>> kfree(sdio_priv);
>> }
>>
>> @@ -375,8 +385,10 @@ static int wilc_sdio_write_reg(struct wilc *wilc,
>> u32 addr, u32 data)
>> cmd.address = WILC_SDIO_FBR_DATA_REG;
>> cmd.block_mode = 0;
>> cmd.increment = 1;
>> - cmd.count = 4;
>> - cmd.buffer = (u8 *)&data;
>> + cmd.count = sizeof(u32);
>> + /* copy to a bounce buffer to avoid use of stack
>> variable */
>> + memcpy(sdio_priv->cmd53_buf, &data, sizeof(u32));
>
> Locking seems to be missing, no? How is sdio_priv->cmd53_buf
> protected from parallel access?
Yes, I also think lock would be required with these changes. I am
planning to use the existing 'sdio_claim_host' locking instead of
introducing a new lock and also take care to not adding a duplicate API
to handle cmd53. For better understanding, I will send the v2 patch for
review.
Regards,
Ajay'
