diff mbox

dmaengine: edma: fix build without CONFIG_OF

Message ID 17811472.bY8CqmdEVy@wuerfel (mailing list archive)
State Accepted
Headers show

Commit Message

Arnd Bergmann Nov. 3, 2015, 2 p.m. UTC 
During the edma rework, a build error was introduced for the
case that CONFIG_OF is disabled:

drivers/built-in.o: In function `edma_tc_set_pm_state':
:(.text+0x43bf0): undefined reference to `of_find_device_by_node'

As the edma_tc_set_pm_state() function does nothing in case
we are running without OF, this adds an IS_ENABLED() check
that turns the function into an empty stub then and avoids the
link error.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: ca304fa9bb76 ("ARM/dmaengine: edma: Public API to use private struct pointer")
---
Found on ARM randconfig builds with today's linux-next


--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Peter Ujfalusi Nov. 4, 2015, 7:42 a.m. UTC | #1 
On 11/03/2015 04:00 PM, Arnd Bergmann wrote:
> During the edma rework, a build error was introduced for the
> case that CONFIG_OF is disabled:
> 
> drivers/built-in.o: In function `edma_tc_set_pm_state':
> :(.text+0x43bf0): undefined reference to `of_find_device_by_node'
> 
> As the edma_tc_set_pm_state() function does nothing in case
> we are running without OF, this adds an IS_ENABLED() check
> that turns the function into an empty stub then and avoids the
> link error.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: ca304fa9bb76 ("ARM/dmaengine: edma: Public API to use private struct pointer")

The actual commit this patch is fixing is:
1be5336bc7ba dmaengine: edma: New device tree binding

> ---
> Found on ARM randconfig builds with today's linux-next

I have sanity built the kernel with omap2plus_defconfig and
davinci_all_defconfig since eDMA is used by these platforms and did not faced
with this issue, as obviously these defconfigs will result OF to be enabled.

> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> index 31722d436a42..16713a93da10 100644
> --- a/drivers/dma/edma.c
> +++ b/drivers/dma/edma.c
> @@ -1560,7 +1560,7 @@ static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
>  	struct platform_device *tc_pdev;
>  	int ret;
>  
> -	if (!tc)
> +	if (!IS_ENABLED(CONFIG_OF) || !tc)
>  		return;

Should we instead put the function inside of:
#if IS_ENABLED(CONFIG_OF)
static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
{
...
}
#else
static inline void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
{
}
#endif /* IS_ENABLED(CONFIG_OF) */


>  
>  	tc_pdev = of_find_device_by_node(tc->node);
>
Arnd Bergmann Nov. 4, 2015, 8:46 a.m. UTC | #2 
On Wednesday 04 November 2015 09:42:35 Peter Ujfalusi wrote:
> On 11/03/2015 04:00 PM, Arnd Bergmann wrote:
> > During the edma rework, a build error was introduced for the
> > case that CONFIG_OF is disabled:
> > 
> > drivers/built-in.o: In function `edma_tc_set_pm_state':
> > :(.text+0x43bf0): undefined reference to `of_find_device_by_node'
> > 
> > As the edma_tc_set_pm_state() function does nothing in case
> > we are running without OF, this adds an IS_ENABLED() check
> > that turns the function into an empty stub then and avoids the
> > link error.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: ca304fa9bb76 ("ARM/dmaengine: edma: Public API to use private struct pointer")
> 
> The actual commit this patch is fixing is:
> 1be5336bc7ba dmaengine: edma: New device tree binding

That's what I first thought, but it seems to just move around the
call to of_find_device_by_node that was first introduced in the
commit I mentioned. Did you build-test it successfully with
ca304fa9bb76 and CONFIG_OF enabled? I have to admit that I
was just guessing from the contents and did not bisect this
fully.

> > ---
> > Found on ARM randconfig builds with today's linux-next
> 
> I have sanity built the kernel with omap2plus_defconfig and
> davinci_all_defconfig since eDMA is used by these platforms and did not faced
> with this issue, as obviously these defconfigs will result OF to be enabled.

Right. The defconfigs were all fine, and this is hard to hit
even in the randconfig builds.

> > diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
> > index 31722d436a42..16713a93da10 100644
> > --- a/drivers/dma/edma.c
> > +++ b/drivers/dma/edma.c
> > @@ -1560,7 +1560,7 @@ static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
> >       struct platform_device *tc_pdev;
> >       int ret;
> >  
> > -     if (!tc)
> > +     if (!IS_ENABLED(CONFIG_OF) || !tc)
> >               return;
> 
> Should we instead put the function inside of:
> #if IS_ENABLED(CONFIG_OF)
> static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
> {
> ...
> }
> #else
> static inline void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
> {
> }
> #endif /* IS_ENABLED(CONFIG_OF) */

I think that would be less readable, and gives no compile-time coverage
to the contents of the edma_tc_set_pm_state function.

The effect is the same, so I'd rather stay with my version.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Ujfalusi Nov. 4, 2015, 9:05 a.m. UTC | #3 
On 11/04/2015 10:46 AM, Arnd Bergmann wrote:
> On Wednesday 04 November 2015 09:42:35 Peter Ujfalusi wrote:
>> On 11/03/2015 04:00 PM, Arnd Bergmann wrote:
>>> During the edma rework, a build error was introduced for the
>>> case that CONFIG_OF is disabled:
>>>
>>> drivers/built-in.o: In function `edma_tc_set_pm_state':
>>> :(.text+0x43bf0): undefined reference to `of_find_device_by_node'
>>>
>>> As the edma_tc_set_pm_state() function does nothing in case
>>> we are running without OF, this adds an IS_ENABLED() check
>>> that turns the function into an empty stub then and avoids the
>>> link error.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>> Fixes: ca304fa9bb76 ("ARM/dmaengine: edma: Public API to use private struct pointer")
>>
>> The actual commit this patch is fixing is:
>> 1be5336bc7ba dmaengine: edma: New device tree binding
> 
> That's what I first thought, but it seems to just move around the
> call to of_find_device_by_node that was first introduced in the
> commit I mentioned. Did you build-test it successfully with
> ca304fa9bb76 and CONFIG_OF enabled? I have to admit that I
> was just guessing from the contents and did not bisect this
> fully.

Ah, I see. That of_find_device_by_node() was used by the function used to
build the unused channel list for the legacy old code. The whole unused
channel list has been removed by the new DT binding patch since it was bogus
to start with and there is no need for it anymore.

> 
>>> ---
>>> Found on ARM randconfig builds with today's linux-next
>>
>> I have sanity built the kernel with omap2plus_defconfig and
>> davinci_all_defconfig since eDMA is used by these platforms and did not faced
>> with this issue, as obviously these defconfigs will result OF to be enabled.
> 
> Right. The defconfigs were all fine, and this is hard to hit
> even in the randconfig builds.

I just did a sanity clean _defconfig builds and they both built fine (even w/o
this patch), phew.

>>> diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
>>> index 31722d436a42..16713a93da10 100644
>>> --- a/drivers/dma/edma.c
>>> +++ b/drivers/dma/edma.c
>>> @@ -1560,7 +1560,7 @@ static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
>>>       struct platform_device *tc_pdev;
>>>       int ret;
>>>  
>>> -     if (!tc)
>>> +     if (!IS_ENABLED(CONFIG_OF) || !tc)
>>>               return;
>>
>> Should we instead put the function inside of:
>> #if IS_ENABLED(CONFIG_OF)
>> static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
>> {
>> ...
>> }
>> #else
>> static inline void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
>> {
>> }
>> #endif /* IS_ENABLED(CONFIG_OF) */
> 
> I think that would be less readable, and gives no compile-time coverage
> to the contents of the edma_tc_set_pm_state function.

Hrm, if the compiler knows that there is no need to compile the code after the:
if (!IS_ENABLED(CONFIG_OF) || !tc)
when OF is disabled, then it does not give more compile coverage then have
empty inline function in case of !OF.
Not sure about it, but if we disable all optimization in gcc, then we might
get the same missing symbol?

> The effect is the same, so I'd rather stay with my version.

I'm fine with both. It is up to Vinod to decide which one he prefers (I prefer
the #if #else #endif version).

Anyways:
Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Nov. 4, 2015, 10:29 a.m. UTC | #4 
On Wednesday 04 November 2015 11:05:54 Peter Ujfalusi wrote:
> > 
> > I think that would be less readable, and gives no compile-time coverage
> > to the contents of the edma_tc_set_pm_state function.
> 
> Hrm, if the compiler knows that there is no need to compile the code after the:
> if (!IS_ENABLED(CONFIG_OF) || !tc)
> when OF is disabled, then it does not give more compile coverage then have
> empty inline function in case of !OF.

No, the way it works (simplified but close enough here) is that gcc parses
all the source code first and throws warnings or errors for everything that
looks wrong to it, and only later throws out all code that it knows is never
used before emitting the object code.

The difference to the #ifdef is that the preprocessor here throws out all
disabled code before it gets parsed, so we don't get warnings for it.

> Not sure about it, but if we disable all optimization in gcc, then we might
> get the same missing symbol?

It's impossible to build the kernel with inlining disabled, because we
rely on the same kind of optimization in lots of places.

> > The effect is the same, so I'd rather stay with my version.
> 
> I'm fine with both. It is up to Vinod to decide which one he prefers (I prefer
> the #if #else #endif version).
> 
> Anyways:
> Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Thanks!

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul Nov. 16, 2015, 3:36 a.m. UTC | #5 
On Tue, Nov 03, 2015 at 03:00:57PM +0100, Arnd Bergmann wrote:
> During the edma rework, a build error was introduced for the
> case that CONFIG_OF is disabled:
> 
> drivers/built-in.o: In function `edma_tc_set_pm_state':
> :(.text+0x43bf0): undefined reference to `of_find_device_by_node'
> 
> As the edma_tc_set_pm_state() function does nothing in case
> we are running without OF, this adds an IS_ENABLED() check
> that turns the function into an empty stub then and avoids the
> link error.

Applied, thanks
diff mbox

Patch

diff --git a/drivers/dma/edma.c b/drivers/dma/edma.c
index 31722d436a42..16713a93da10 100644
--- a/drivers/dma/edma.c
+++ b/drivers/dma/edma.c
@@ -1560,7 +1560,7 @@  static void edma_tc_set_pm_state(struct edma_tc *tc, bool enable)
 	struct platform_device *tc_pdev;
 	int ret;
 
-	if (!tc)
+	if (!IS_ENABLED(CONFIG_OF) || !tc)
 		return;
 
 	tc_pdev = of_find_device_by_node(tc->node);