[PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Niklas Söderlund]

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(+)

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);
 }
 
-- 
2.18.0

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Niklas Söderlund]

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

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]


> 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Geert Uytterhoeven]

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

-- 
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

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 112 bytes --]


> Promote drivers/gpu/drm/exynos/exynos_drm_iommu.c:configure_dma_max_seg_size()
> to a generic helper?

Yes!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Niklas Söderlund]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 930 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] mmc: renesas_sdhi_internal_dmac: set scatter/gather max segment size

[Niklas Söderlund]

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