[PATCH] powerpc : dma-mapping : Check null condition for dev->archdata.dma_ops

[PATCH] powerpc : dma-mapping : Check null condition for dev->archdata.dma_ops

[Nikhil Badola]

Modifies get_dma_ops() implementation on ppc arch to check null condition
for dev->archdata.dma_ops; returns common dma_direct_ops structure in
case its NULL

Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
---
 arch/powerpc/include/asm/dma-mapping.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/dma-mapping.h b/arch/powerpc/include/asm/dma-mapping.h
index 150866b..d73bae8 100644
--- a/arch/powerpc/include/asm/dma-mapping.h
+++ b/arch/powerpc/include/asm/dma-mapping.h
@@ -86,10 +86,12 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 	 */
 	if (unlikely(dev == NULL))
 		return NULL;
-
-	return dev->archdata.dma_ops;
+	if (dev->archdata.dma_ops)
+		return dev->archdata.dma_ops;
+	return &dma_direct_ops;
 }
 
+
 static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)
 {
 	dev->archdata.dma_ops = ops;
-- 
1.7.11.7

Re: [PATCH] powerpc : dma-mapping : Check null condition for dev->archdata.dma_ops

[Denis Kirjanov]

On 7/18/14, Nikhil Badola <nikhil.badola@freescale.com> wrote:
> Modifies get_dma_ops() implementation on ppc arch to check null condition
 which means that dma is not supported.

Could you please describe the use case where the ops is null.

> for dev->archdata.dma_ops; returns common dma_direct_ops structure in
> case its NULL
>
> Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> ---
>  arch/powerpc/include/asm/dma-mapping.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/dma-mapping.h
> b/arch/powerpc/include/asm/dma-mapping.h
> index 150866b..d73bae8 100644
> --- a/arch/powerpc/include/asm/dma-mapping.h
> +++ b/arch/powerpc/include/asm/dma-mapping.h
> @@ -86,10 +86,12 @@ static inline struct dma_map_ops *get_dma_ops(struct
> device *dev)
>  	 */
>  	if (unlikely(dev == NULL))
>  		return NULL;
> -
> -	return dev->archdata.dma_ops;
> +	if (dev->archdata.dma_ops)
> +		return dev->archdata.dma_ops;
> +	return &dma_direct_ops;
>  }
>
> +
>  static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)
>  {
>  	dev->archdata.dma_ops = ops;
> --
> 1.7.11.7
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev


-- 
Regards,
Denis

RE: [PATCH] powerpc : dma-mapping : Check null condition for dev->archdata.dma_ops

[nikhil.badola]

> -----Original Message-----
> From: Denis Kirjanov [mailto:kirjanov@gmail.com]
> Sent: Friday, July 18, 2014 7:21 PM
> To: Badola Nikhil-B46172
> Cc: linuxppc-dev@lists.ozlabs.org
> Subject: Re: [PATCH] powerpc : dma-mapping : Check null condition for dev=
-
> >archdata.dma_ops
>=20
> On 7/18/14, Nikhil Badola <nikhil.badola@freescale.com> wrote:
> > Modifies get_dma_ops() implementation on ppc arch to check null
> > condition
>  which means that dma is not supported.
>=20
> Could you please describe the use case where the ops is null.

The use case in which ops are null is while running USB in Gadget and Otg m=
ode.

For PPC architecture, whenever a platform device is registered, pdev->dev i=
s=20
assigned dma_ops.=20
In USB, one single chipidea platform device is registered for any mode (hos=
t, gadget or otg)
whose dev, as explained above, has dma_ops set and this dev is assigned to =
 ci->dev(dev of chipidea struct)
which is used by host controller device. That is why we don't need the abov=
e null case checking in
host mode. But when we run usb in gadget/otg mode, the device structure use=
d is ci->gadget.dev
which does not have dma_ops set and it crashes when dma transaction starts =
when it calls get_dma_ops()
which returns NULL.

A similar approach is used in ARM architecture which checks for null condit=
ion and returns=20
common dma_ops
>=20
> > for dev->archdata.dma_ops; returns common dma_direct_ops structure in
> > case its NULL
> >
> > Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> > ---
> >  arch/powerpc/include/asm/dma-mapping.h | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/dma-mapping.h
> > b/arch/powerpc/include/asm/dma-mapping.h
> > index 150866b..d73bae8 100644
> > --- a/arch/powerpc/include/asm/dma-mapping.h
> > +++ b/arch/powerpc/include/asm/dma-mapping.h
> > @@ -86,10 +86,12 @@ static inline struct dma_map_ops
> > *get_dma_ops(struct device *dev)
> >  	 */
> >  	if (unlikely(dev =3D=3D NULL))
> >  		return NULL;
> > -
> > -	return dev->archdata.dma_ops;
> > +	if (dev->archdata.dma_ops)
> > +		return dev->archdata.dma_ops;
> > +	return &dma_direct_ops;
> >  }
> >
> > +
> >  static inline void set_dma_ops(struct device *dev, struct dma_map_ops
> > *ops)  {
> >  	dev->archdata.dma_ops =3D ops;
> > --
> > 1.7.11.7
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
>=20
>=20
> --
> Regards,
> Denis

Regards,
Nikhil

Re: powerpc : dma-mapping : Check null condition for dev->archdata.dma_ops

[Scott Wood]

On Fri, Jul 18, 2014 at 03:34:52PM +0530, Nikhil Badola wrote:
> Modifies get_dma_ops() implementation on ppc arch to check null condition
> for dev->archdata.dma_ops; returns common dma_direct_ops structure in
> case its NULL
> 
> Signed-off-by: Nikhil Badola <nikhil.badola@freescale.com>
> ---
>  arch/powerpc/include/asm/dma-mapping.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

I'm not sure why this was delegated to me in patchwork, as it doesn't
appear to be FSL-specific...  I wish patchwork showed the history of
changes to a patch.

> The use case in which ops are null is while running USB in Gadget and
> Otg mode.
> 
> For PPC architecture, whenever a platform device is registered,
> pdev->dev is assigned dma_ops.  In USB, one single chipidea platform
> device is registered for any mode (host, gadget or otg) whose dev, as
> explained above, has dma_ops set and this dev is assigned to
> ci->dev(dev of chipidea struct) which is used by host controller
> device.  That is why we don't need the above null case checking in host
> mode.  But when we run usb in gadget/otg mode, the device structure
> used is ci->gadget.dev which does not have dma_ops set and it crashes
> when dma transaction starts when it calls get_dma_ops() which returns
> NULL.
>
> A similar approach is used in ARM architecture which checks for null
> condition and returns common dma_ops

The above doesn't make it clear to me why dma_ops is NULL in non-host
modes, or why "direct" is necessarily the right ops -- what if swiotlb or
an iommu is needed?

If the problem is that you're using a "struct device" other than the
platform device, shouldn't that be fixed?  Or make sure that this other
"struct device" has been properly set up by arch code for doing DMA.

>  }
>  
> +
>  static inline void set_dma_ops(struct device *dev, struct dma_map_ops *ops)

Don't add this newline

-Scott