| Message ID | Pine.LNX.4.64.1305310423360.1470@axis700.grange (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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/
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
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/
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
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>
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
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(-)