| Message ID | 20220804131837.336769-1-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: wilc1000: fix DMA on stack objects | expand |
Hi, thanks for the patch! Am 2022-08-04 15:18, schrieb Ajay.Kathat@microchip.com: > From: Ajay Singh <ajay.kathat@microchip.com> > > 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@kernel.org> > Suggested-by: Michael Walle <mwalle@kernel.org> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > --- > > 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@walle.cc/ > > .../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. > 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. > + 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? -michael > + cmd.buffer = sdio_priv->cmd53_buf; > cmd.block_size = sdio_priv->block_size; > ret = wilc_sdio_cmd53(wilc, &cmd); > if (ret) > @@ -492,8 +504,8 @@ static int wilc_sdio_read_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); > + cmd.buffer = sdio_priv->cmd53_buf; > > cmd.block_size = sdio_priv->block_size; > ret = wilc_sdio_cmd53(wilc, &cmd); > @@ -502,6 +514,7 @@ static int wilc_sdio_read_reg(struct wilc *wilc, > u32 addr, u32 *data) > "Failed cmd53, read reg (%08x)...\n", addr); > return ret; > } > + memcpy(data, sdio_priv->cmd53_buf, sizeof(u32)); > } > > le32_to_cpus(data); > 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;
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@microchip.com: >> From: Ajay Singh <ajay.kathat@microchip.com> >> >> 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@kernel.org> >> Suggested-by: Michael Walle <mwalle@kernel.org> >> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> >> --- >> >> 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@walle.cc/ >> >> >> .../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'
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..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; + 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)); + cmd.buffer = sdio_priv->cmd53_buf; cmd.block_size = sdio_priv->block_size; ret = wilc_sdio_cmd53(wilc, &cmd); if (ret) @@ -492,8 +504,8 @@ static int wilc_sdio_read_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); + cmd.buffer = sdio_priv->cmd53_buf; cmd.block_size = sdio_priv->block_size; ret = wilc_sdio_cmd53(wilc, &cmd); @@ -502,6 +514,7 @@ static int wilc_sdio_read_reg(struct wilc *wilc, u32 addr, u32 *data) "Failed cmd53, read reg (%08x)...\n", addr); return ret; } + memcpy(data, sdio_priv->cmd53_buf, sizeof(u32)); } le32_to_cpus(data); 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;