| Message ID | 20180830213843.14680-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size | expand |
Hi Niklas, Thanks for your patch! On Thu, Aug 30, 2018 at 11:39 PM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > Fix warning when running with CONFIG_DMA_API_DEBUG_SG=y by allocating a > device_dma_parameters structure and filling in the max segment size. The > size used is the result of a discussion with Renesas hardware engineers > and unfortunately not found in the datasheet. > > renesas_sdhi_internal_dmac ee140000.sd: DMA-API: mapping sg segment > longer than device claims to support [len=126976] [max=65536A] Bogus trailing "A". > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > > --- > > * Changes since v1 > - Use devm_kzalloc() instead of a custom remove function to clean up. > --- > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > index c9dab9ce1ed55174..715e0c5dc30e4ebf 100644 > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > @@ -308,12 +308,23 @@ static const struct soc_device_attribute gen3_soc_whitelist[] = { > static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev) > { > const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist); > + struct device *dev = &pdev->dev; > > if (!soc) > return -ENODEV; > > global_flags |= (unsigned long)soc->data; > > + if (!dev->dma_parms) { I guess dev->dma_parms is retained on unbind/rebind? > + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > + GFP_KERNEL); But by using devm_kzalloc(), the memory will be freed, and lead to reuse after free? > + if (!dev->dma_parms) > + return -ENOMEM; > + } > + > + if (dma_set_max_seg_size(dev, 0xffffffff)) > + dev_warn(dev, "Unable to set seg size\n"); > + > return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops); > } Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for your feedback. On 2018-08-31 00:10:28 +0200, Geert Uytterhoeven wrote: > Hi Niklas, > > Thanks for your patch! > > On Thu, Aug 30, 2018 at 11:39 PM Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: > > Fix warning when running with CONFIG_DMA_API_DEBUG_SG=y by allocating a > > device_dma_parameters structure and filling in the max segment size. The > > size used is the result of a discussion with Renesas hardware engineers > > and unfortunately not found in the datasheet. > > > > renesas_sdhi_internal_dmac ee140000.sd: DMA-API: mapping sg segment > > longer than device claims to support [len=126976] [max=65536A] > > Bogus trailing "A". Thanks, that is a odd copy-and-past error :-) > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > --- > > > > * Changes since v1 > > - Use devm_kzalloc() instead of a custom remove function to clean up. > > --- > > drivers/mmc/host/renesas_sdhi_internal_dmac.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > > index c9dab9ce1ed55174..715e0c5dc30e4ebf 100644 > > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > > @@ -308,12 +308,23 @@ static const struct soc_device_attribute gen3_soc_whitelist[] = { > > static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev) > > { > > const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist); > > + struct device *dev = &pdev->dev; > > > > if (!soc) > > return -ENODEV; > > > > global_flags |= (unsigned long)soc->data; > > > > + if (!dev->dma_parms) { > > I guess dev->dma_parms is retained on unbind/rebind? > > > + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > > + GFP_KERNEL); > > But by using devm_kzalloc(), the memory will be freed, and lead to reuse after > free? I don't know how the unbind/rebind behaves in this regard. In v1 of this patch I used kzalloc() and a remove function to free the memory and reset the dev->dma_parms pointe to NULL. Do you think that is the correct approach here? > > > + if (!dev->dma_parms) > > + return -ENOMEM; > > + } > > + > > + if (dma_set_max_seg_size(dev, 0xffffffff)) > > + dev_warn(dev, "Unable to set seg size\n"); > > + > > return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops); > > } > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Niklas, On Fri, Aug 31, 2018 at 9:57 AM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > On 2018-08-31 00:10:28 +0200, Geert Uytterhoeven wrote: > > On Thu, Aug 30, 2018 at 11:39 PM Niklas Söderlund > > <niklas.soderlund+renesas@ragnatech.se> wrote: > > > Fix warning when running with CONFIG_DMA_API_DEBUG_SG=y by allocating a > > > device_dma_parameters structure and filling in the max segment size. The > > > size used is the result of a discussion with Renesas hardware engineers > > > and unfortunately not found in the datasheet. > > > > > > renesas_sdhi_internal_dmac ee140000.sd: DMA-API: mapping sg segment > > > longer than device claims to support [len=126976] [max=65536A] > > > --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c > > > +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c > > > @@ -308,12 +308,23 @@ static const struct soc_device_attribute gen3_soc_whitelist[] = { > > > static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev) > > > { > > > const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist); > > > + struct device *dev = &pdev->dev; > > > > > > if (!soc) > > > return -ENODEV; > > > > > > global_flags |= (unsigned long)soc->data; > > > > > > + if (!dev->dma_parms) { > > > > I guess dev->dma_parms is retained on unbind/rebind? > > > > > + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), > > > + GFP_KERNEL); > > > > But by using devm_kzalloc(), the memory will be freed, and lead to reuse after > > free? > > I don't know how the unbind/rebind behaves in this regard. In v1 of this I expect the struct device to be retained. You can check with sysfs unbind/bind, and CONFIG_DEBUG_SLAB=y to enable poisoning on free (or just print the value found for less spectacular behavior ;-) > patch I used kzalloc() and a remove function to free the memory and > reset the dev->dma_parms pointe to NULL. Do you think that is the > correct approach here? As a comment in the other thread said you should not set it multiple times, probably the best solution is to: (a) check if dev->dma_parms is already set, and if not: (b) allocate using plain kzalloc() (with a comment why!), (c) not free the memory nor reset dev->dma_parms (with a comment why!). Gr{oetje,eeting}s, Geert
> As a comment in the other thread said you should not set it multiple times, > probably the best solution is to: > (a) check if dev->dma_parms is already set, and if not: > (b) allocate using plain kzalloc() (with a comment why!), > (c) not free the memory nor reset dev->dma_parms (with a comment why!). Quite clumsy if done per driver, or? If this is the intended behaviour, then there is something to improve with the dma_params API, or? If something which is meant to have a lifecycle same as the device but needs to get set during probe/remove lifecycle, this calls for at least a helper function which hides all these details?
Hi Wolfram, On Fri, Aug 31, 2018 at 11:44 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > As a comment in the other thread said you should not set it multiple times, > > probably the best solution is to: > > (a) check if dev->dma_parms is already set, and if not: > > (b) allocate using plain kzalloc() (with a comment why!), > > (c) not free the memory nor reset dev->dma_parms (with a comment why!). > > Quite clumsy if done per driver, or? Yes it is. As for other DMA parameters (e.g. DMA mask), it's a property of the device and/or platform. So (in theory) it should be set up that way... > If this is the intended behaviour, then there is something to improve > with the dma_params API, or? If something which is meant to have a > lifecycle same as the device but needs to get set during probe/remove > lifecycle, this calls for at least a helper function which hides all > these details? Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size() to a generic helper? Gr{oetje,eeting}s, Geert
> Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size() > to a generic helper? Yes!
On 2018-08-31 12:35:24 +0200, Wolfram Sang wrote: > > > Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size() > > to a generic helper? > > Yes! > If that is promoted should not drivers/gpu/drm/exynos/exynos_drm_iommu.c:clear_dma_max_seg_size() also be promoted? And if so should this patch revert back to v1 with a custom remove function which clears and free the dma_parms ?
On Fri, Aug 31, 2018 at 12:49:08PM +0200, Niklas Söderlund wrote: > On 2018-08-31 12:35:24 +0200, Wolfram Sang wrote: > > > > > Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size() > > > to a generic helper? > > > > Yes! > > > If that is promoted should not > drivers/gpu/drm/exynos/exynos_drm_iommu.c:clear_dma_max_seg_size() also > be promoted? And if so should this patch revert back to v1 with a custom > remove function which clears and free the dma_parms ? My preference would be easy to use helpers for drivers because this is easy to get wrong. I think we need to discuss with the creators of that API: a) if the drm/exynos driver does the right thing(tm) b) if these functions should be generic helpers c) when to clear the pointer (a bit related to a)) Niklas, do you have an interest to do that? Or would you rather go with SDHI hacking? :) I can do it, too.
Hi Wolfram, On 2018-08-31 13:04:59 +0200, Wolfram Sang wrote: > On Fri, Aug 31, 2018 at 12:49:08PM +0200, Niklas Söderlund wrote: > > On 2018-08-31 12:35:24 +0200, Wolfram Sang wrote: > > > > > > > Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size() > > > > to a generic helper? > > > > > > Yes! > > > > > If that is promoted should not > > drivers/gpu/drm/exynos/exynos_drm_iommu.c:clear_dma_max_seg_size() also > > be promoted? And if so should this patch revert back to v1 with a custom > > remove function which clears and free the dma_parms ? > > My preference would be easy to use helpers for drivers because this is > easy to get wrong. I think we need to discuss with the creators of that > API: > > a) if the drm/exynos driver does the right thing(tm) > b) if these functions should be generic helpers > c) when to clear the pointer (a bit related to a)) > > Niklas, do you have an interest to do that? Or would you rather go with > SDHI hacking? :) I can do it, too. > If you would like to spearhead this please go a head :-) If not please let me know so it won't fall thru the cracks.
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c index c9dab9ce1ed55174..715e0c5dc30e4ebf 100644 --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c @@ -308,12 +308,23 @@ static const struct soc_device_attribute gen3_soc_whitelist[] = { static int renesas_sdhi_internal_dmac_probe(struct platform_device *pdev) { const struct soc_device_attribute *soc = soc_device_match(gen3_soc_whitelist); + struct device *dev = &pdev->dev; if (!soc) return -ENODEV; global_flags |= (unsigned long)soc->data; + if (!dev->dma_parms) { + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), + GFP_KERNEL); + if (!dev->dma_parms) + return -ENOMEM; + } + + if (dma_set_max_seg_size(dev, 0xffffffff)) + dev_warn(dev, "Unable to set seg size\n"); + return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops); }
Fix warning when running with CONFIG_DMA_API_DEBUG_SG=y by allocating a device_dma_parameters structure and filling in the max segment size. The size used is the result of a discussion with Renesas hardware engineers and unfortunately not found in the datasheet. renesas_sdhi_internal_dmac ee140000.sd: DMA-API: mapping sg segment longer than device claims to support [len=126976] [max=65536A] Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> --- * Changes since v1 - Use devm_kzalloc() instead of a custom remove function to clean up. --- drivers/mmc/host/renesas_sdhi_internal_dmac.c | 11 +++++++++++ 1 file changed, 11 insertions(+)