Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
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. -- Regards, Niklas Söderlund
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
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. signature.asc Description: PGP signature
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
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 ? -- Regards, Niklas Söderlund
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
> Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size() > to a generic helper? Yes! signature.asc Description: PGP signature
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
Hi Wolfram,
On Fri, Aug 31, 2018 at 11:44 AM Wolfram Sang 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
> 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? signature.asc Description: PGP signature
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
Hi Niklas,
On Fri, Aug 31, 2018 at 9:57 AM Niklas Söderlund
wrote:
> On 2018-08-31 00:10:28 +0200, Geert Uytterhoeven wrote:
> > On Thu, Aug 30, 2018 at 11:39 PM Niklas Söderlund
> > 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 ee14.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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
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
> 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 ee14.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
> > Reported-by: Geert Uytterhoeven
> >
> > ---
> >
> > * 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, 0x))
> > + 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 --
> ge...@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
--
Regards,
Niklas Söderlund
Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
Hi Niklas,
Thanks for your patch!
On Thu, Aug 30, 2018 at 11:39 PM Niklas Söderlund
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 ee14.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
> Reported-by: Geert Uytterhoeven
>
> ---
>
> * 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, 0x))
> + 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 -- ge...@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
[PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size
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 ee14.sd: DMA-API: mapping sg segment
longer than device claims to support [len=126976] [max=65536A]
Signed-off-by: Niklas Söderlund
Reported-by: Geert Uytterhoeven
---
* 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) {
+ 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, 0x))
+ dev_warn(dev, "Unable to set seg size\n");
+
return renesas_sdhi_probe(pdev, &renesas_sdhi_internal_dmac_dma_ops);
}
--
2.18.0
