| Message ID | 20160914101048.1061-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday, September 14, 2016 1:10:48 PM CEST Peter Ujfalusi wrote: > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 24ebc9a8de89..0a5640156159 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -1409,7 +1409,7 @@ static int omap_hsmmc_pre_dma_transfer(struct omap_hsmmc_host *host, > static int omap_hsmmc_setup_dma_transfer(struct omap_hsmmc_host *host, > struct mmc_request *req) > { > - struct dma_slave_config cfg; > + struct dma_slave_config cfg = {}; > struct dma_async_tx_descriptor *tx; > int ret = 0, i; > struct mmc_data *data = req->data; > This change looks correct and is certainly the simplest solution if you want to get it into v4.8, but generally speaking, I think it would be nicer to combine it with the initialization of the fields: struct dma_slave_config cfg = { .src_addr = host->mapbase + OMAP_HSMMC_DATA; .dst_addr = host->mapbase + OMAP_HSMMC_DATA; .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; .dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; .src_maxburst = data->blksz / 4; .dst_maxburst = data->blksz / 4; }; Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 September 2016 at 12:23, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday, September 14, 2016 1:10:48 PM CEST Peter Ujfalusi wrote: >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c >> index 24ebc9a8de89..0a5640156159 100644 >> --- a/drivers/mmc/host/omap_hsmmc.c >> +++ b/drivers/mmc/host/omap_hsmmc.c >> @@ -1409,7 +1409,7 @@ static int omap_hsmmc_pre_dma_transfer(struct omap_hsmmc_host *host, >> static int omap_hsmmc_setup_dma_transfer(struct omap_hsmmc_host *host, >> struct mmc_request *req) >> { >> - struct dma_slave_config cfg; >> + struct dma_slave_config cfg = {}; >> struct dma_async_tx_descriptor *tx; >> int ret = 0, i; >> struct mmc_data *data = req->data; >> > > This change looks correct and is certainly the simplest solution if you want to > get it into v4.8, but generally speaking, I think it would be nicer to combine > it with the initialization of the fields: > > struct dma_slave_config cfg = { > .src_addr = host->mapbase + OMAP_HSMMC_DATA; > .dst_addr = host->mapbase + OMAP_HSMMC_DATA; > .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > .dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > .src_maxburst = data->blksz / 4; > .dst_maxburst = data->blksz / 4; > }; This seems like a small an easy change as well, so I wouldn't mind picking up a v2 adopting this pattern instead. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/14/16 13:23, Arnd Bergmann wrote: > On Wednesday, September 14, 2016 1:10:48 PM CEST Peter Ujfalusi wrote: >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c >> index 24ebc9a8de89..0a5640156159 100644 >> --- a/drivers/mmc/host/omap_hsmmc.c >> +++ b/drivers/mmc/host/omap_hsmmc.c >> @@ -1409,7 +1409,7 @@ static int omap_hsmmc_pre_dma_transfer(struct omap_hsmmc_host *host, >> static int omap_hsmmc_setup_dma_transfer(struct omap_hsmmc_host *host, >> struct mmc_request *req) >> { >> - struct dma_slave_config cfg; >> + struct dma_slave_config cfg = {}; >> struct dma_async_tx_descriptor *tx; >> int ret = 0, i; >> struct mmc_data *data = req->data; >> > > This change looks correct and is certainly the simplest solution if you want to > get it into v4.8, but generally speaking, I think it would be nicer to combine > it with the initialization of the fields: > > struct dma_slave_config cfg = { > .src_addr = host->mapbase + OMAP_HSMMC_DATA; > .dst_addr = host->mapbase + OMAP_HSMMC_DATA; > .src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > .dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > .src_maxburst = data->blksz / 4; > .dst_maxburst = data->blksz / 4; I opted to not do this because the data->blksz is validated later in the function (twice). It is not a big issue, but looks a bit odd.
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 24ebc9a8de89..0a5640156159 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1409,7 +1409,7 @@ static int omap_hsmmc_pre_dma_transfer(struct omap_hsmmc_host *host, static int omap_hsmmc_setup_dma_transfer(struct omap_hsmmc_host *host, struct mmc_request *req) { - struct dma_slave_config cfg; + struct dma_slave_config cfg = {}; struct dma_async_tx_descriptor *tx; int ret = 0, i; struct mmc_data *data = req->data;
It is wrong to not initialize the dma_slave_config struct as the DMAengine driver might look at non initialized (random data) fields and tries to interpret it. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- Hi, would it be possible to send this patch for 4.8? I have omap-dma DMAengine driver changes pending, but they break the MMC/SD because of the uninitialized fields in dma_slave_config. Thanks, Peter drivers/mmc/host/omap_hsmmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.10.0 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html