[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Guennadi Liakhovetski]

On platforms with no support for the shdma dmaengine driver build is
currently failing with

drivers/built-in.o: In function `sh_mobile_sdhi_probe':
drivers/mmc/host/sh_mobile_sdhi.c:170: undefined reference to`shdma_chan_filter'

Fix the breakage by defining shdma_chan_filter to NULL in such
configurations.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
---

v2: in "next" shdma_chan_filter() is defined in shdma-base.c, which is 
built if CONFIG_SH_DMAE_BASE is defined. This version uses the correct 
symbol.

This is for "next." Compile-tested only. I'll test it on hardware next 
week, but I don't think it shall break anything.

 include/linux/sh_dma.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h
index b64d6be..1fd8a20 100644
--- a/include/linux/sh_dma.h
+++ b/include/linux/sh_dma.h
@@ -99,6 +99,10 @@ struct sh_dmae_pdata {
 #define CHCR_TE	0x00000002
 #define CHCR_IE	0x00000004
 
+#if IS_ENABLED(CONFIG_SH_DMAE_BASE)
 bool shdma_chan_filter(struct dma_chan *chan, void *arg);
+#else
+#define shdma_chan_filter NULL
+#endif
 
 #endif
-- 
1.7.2.5

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Dan Murphy]

On 05/30/2013 10:01 PM, Guennadi Liakhovetski wrote:
> On platforms with no support for the shdma dmaengine driver build is
> currently failing with
>
> drivers/built-in.o: In function `sh_mobile_sdhi_probe':
> drivers/mmc/host/sh_mobile_sdhi.c:170: undefined reference to`shdma_chan_filter'
>
> Fix the breakage by defining shdma_chan_filter to NULL in such
> configurations.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> ---
>
> v2: in "next" shdma_chan_filter() is defined in shdma-base.c, which is 
> built if CONFIG_SH_DMAE_BASE is defined. This version uses the correct 
> symbol.
>
> This is for "next." Compile-tested only. I'll test it on hardware next 
> week, but I don't think it shall break anything.
>
>  include/linux/sh_dma.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h
> index b64d6be..1fd8a20 100644
> --- a/include/linux/sh_dma.h
> +++ b/include/linux/sh_dma.h
> @@ -99,6 +99,10 @@ struct sh_dmae_pdata {
>  #define CHCR_TE	0x00000002
>  #define CHCR_IE	0x00000004
>  
> +#if IS_ENABLED(CONFIG_SH_DMAE_BASE)
>  bool shdma_chan_filter(struct dma_chan *chan, void *arg);
> +#else
> +#define shdma_chan_filter NULL
OK I did not see a reply to my previous comment
Would this not be better as a
#else
static inline bool shdma_chan_filter(struct dma_chan *chan, void *arg) { return false; }
#endif

Otherwise runtime will call a NULL pointer
> +#endif
>  
>  #endif


-- 
------------------
Dan Murphy

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Guennadi Liakhovetski]

On Fri, 31 May 2013, Dan Murphy wrote:

> On 05/30/2013 10:01 PM, Guennadi Liakhovetski wrote:
> > On platforms with no support for the shdma dmaengine driver build is
> > currently failing with
> >
> > drivers/built-in.o: In function `sh_mobile_sdhi_probe':
> > drivers/mmc/host/sh_mobile_sdhi.c:170: undefined reference to`shdma_chan_filter'
> >
> > Fix the breakage by defining shdma_chan_filter to NULL in such
> > configurations.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> > ---
> >
> > v2: in "next" shdma_chan_filter() is defined in shdma-base.c, which is 
> > built if CONFIG_SH_DMAE_BASE is defined. This version uses the correct 
> > symbol.
> >
> > This is for "next." Compile-tested only. I'll test it on hardware next 
> > week, but I don't think it shall break anything.
> >
> >  include/linux/sh_dma.h |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h
> > index b64d6be..1fd8a20 100644
> > --- a/include/linux/sh_dma.h
> > +++ b/include/linux/sh_dma.h
> > @@ -99,6 +99,10 @@ struct sh_dmae_pdata {
> >  #define CHCR_TE	0x00000002
> >  #define CHCR_IE	0x00000004
> >  
> > +#if IS_ENABLED(CONFIG_SH_DMAE_BASE)
> >  bool shdma_chan_filter(struct dma_chan *chan, void *arg);
> > +#else
> > +#define shdma_chan_filter NULL
> OK I did not see a reply to my previous comment
> Would this not be better as a
> #else
> static inline bool shdma_chan_filter(struct dma_chan *chan, void *arg) { return false; }
> #endif
> 
> Otherwise runtime will call a NULL pointer

Have you looked at the code?:

		if (fn && !fn(chan, fn_param)) {

Have you considered, when this channel allocation at all has a chance to 
get as far as to checking the filter? Have you thought about the meaning 
of "inline" when uing a function to assign it to a pointer? Are you sure 
"will" is the right word here?

Thanks
Guennadi

> > +#endif
> >  
> >  #endif
> 
> 
> -- 
> ------------------
> Dan Murphy
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Dan Murphy]

On 05/31/2013 08:08 AM, Guennadi Liakhovetski wrote:
> On Fri, 31 May 2013, Dan Murphy wrote:
>
>> On 05/30/2013 10:01 PM, Guennadi Liakhovetski wrote:
>>> On platforms with no support for the shdma dmaengine driver build is
>>> currently failing with
>>>
>>> drivers/built-in.o: In function `sh_mobile_sdhi_probe':
>>> drivers/mmc/host/sh_mobile_sdhi.c:170: undefined reference to`shdma_chan_filter'
>>>
>>> Fix the breakage by defining shdma_chan_filter to NULL in such
>>> configurations.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
>>> ---
>>>
>>> v2: in "next" shdma_chan_filter() is defined in shdma-base.c, which is 
>>> built if CONFIG_SH_DMAE_BASE is defined. This version uses the correct 
>>> symbol.
>>>
>>> This is for "next." Compile-tested only. I'll test it on hardware next 
>>> week, but I don't think it shall break anything.
>>>
>>>  include/linux/sh_dma.h |    4 ++++
>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h
>>> index b64d6be..1fd8a20 100644
>>> --- a/include/linux/sh_dma.h
>>> +++ b/include/linux/sh_dma.h
>>> @@ -99,6 +99,10 @@ struct sh_dmae_pdata {
>>>  #define CHCR_TE	0x00000002
>>>  #define CHCR_IE	0x00000004
>>>  
>>> +#if IS_ENABLED(CONFIG_SH_DMAE_BASE)
>>>  bool shdma_chan_filter(struct dma_chan *chan, void *arg);
>>> +#else
>>> +#define shdma_chan_filter NULL
>> OK I did not see a reply to my previous comment
>> Would this not be better as a
>> #else
>> static inline bool shdma_chan_filter(struct dma_chan *chan, void *arg) { return false; }
>> #endif
>>
>> Otherwise runtime will call a NULL pointer
> Have you looked at the code?:
>
> 		if (fn && !fn(chan, fn_param)) {
>
> Have you considered, when this channel allocation at all has a chance to 
> get as far as to checking the filter? Have you thought about the meaning 
> of "inline" when uing a function to assign it to a pointer? Are you sure 
> "will" is the right word here?
My consideration was that since this is an exported symbol that a driver could call this API explicitly without checking for
the function pointer first, there is a potential of calling a NULL pointer.

Dan
> Thanks
> Guennadi
>
>>> +#endif
>>>  
>>>  #endif
>>
>> -- 
>> ------------------
>> Dan Murphy
>>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/


-- 
------------------
Dan Murphy

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Arnd Bergmann]

On Friday 31 May 2013 10:00:29 Dan Murphy wrote:
> My consideration was that since this is an exported symbol that a driver
> could call this API explicitly without checking for
> the function pointer first, there is a potential of calling a NULL pointer.

Drivers never call a filter function directly, so that is not an issue.
Even if a driver would try it, that would not actually compile.
	
	Arnd

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Arnd Bergmann]

On Friday 31 May 2013 05:01:31 Guennadi Liakhovetski wrote:
> On platforms with no support for the shdma dmaengine driver build is
> currently failing with
> 
> drivers/built-in.o: In function `sh_mobile_sdhi_probe':
> drivers/mmc/host/sh_mobile_sdhi.c:170: undefined reference to`shdma_chan_filter'
> 
> Fix the breakage by defining shdma_chan_filter to NULL in such
> configurations.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>

Sorry, I saw your patch too late and already spent some time doing a different one.

> diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h
> index b64d6be..1fd8a20 100644
> --- a/include/linux/sh_dma.h
> +++ b/include/linux/sh_dma.h
> @@ -99,6 +99,10 @@ struct sh_dmae_pdata {
>  #define CHCR_TE        0x00000002
>  #define CHCR_IE        0x00000004
>  
> +#if IS_ENABLED(CONFIG_SH_DMAE_BASE)
>  bool shdma_chan_filter(struct dma_chan *chan, void *arg);
> +#else
> +#define shdma_chan_filter NULL
> +#endif

I still think that this is not the right solution, or at least not
complete: No slave driver should care about which dma-engine it is
connected to. You have already done most of the necessary conversion,
so it would be straightforward to also pass the filter function
pointer to the sdhi driver along with the data that gets passed into
it.

	Arnd

[PATCH v2] dmaengine: shdma: fix a build failure on platforms with no DMA support

[Simon Horman]

On Fri, May 31, 2013 at 04:51:52PM +0200, Arnd Bergmann wrote:
> On Friday 31 May 2013 05:01:31 Guennadi Liakhovetski wrote:
> > On platforms with no support for the shdma dmaengine driver build is
> > currently failing with
> > 
> > drivers/built-in.o: In function `sh_mobile_sdhi_probe':
> > drivers/mmc/host/sh_mobile_sdhi.c:170: undefined reference to`shdma_chan_filter'
> > 
> > Fix the breakage by defining shdma_chan_filter to NULL in such
> > configurations.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com>
> 
> Sorry, I saw your patch too late and already spent some time doing a different one.
> 
> > diff --git a/include/linux/sh_dma.h b/include/linux/sh_dma.h
> > index b64d6be..1fd8a20 100644
> > --- a/include/linux/sh_dma.h
> > +++ b/include/linux/sh_dma.h
> > @@ -99,6 +99,10 @@ struct sh_dmae_pdata {
> >  #define CHCR_TE        0x00000002
> >  #define CHCR_IE        0x00000004
> >  
> > +#if IS_ENABLED(CONFIG_SH_DMAE_BASE)
> >  bool shdma_chan_filter(struct dma_chan *chan, void *arg);
> > +#else
> > +#define shdma_chan_filter NULL
> > +#endif
> 
> I still think that this is not the right solution, or at least not
> complete: No slave driver should care about which dma-engine it is
> connected to. You have already done most of the necessary conversion,
> so it would be straightforward to also pass the filter function
> pointer to the sdhi driver along with the data that gets passed into
> it.

Hi Arnd, Hi Guennadi,

It seems to me that the solution above, though not particularly generic,
is sufficient as according to Guennadi SHDMA users cannot use other DMA
drivers[1]. The change above also has the merit of being rather small.

[1] https://patchwork.kernel.org/patch/2644101/

With that reasoning I would like to provide:

Acked-by: Simon Horman <horms+renesas@verge.net.au>