Re: [PATCH] rtc: ds1685: actually spin forever in poweroff error path

Re: [PATCH] rtc: ds1685: actually spin forever in poweroff error path

[Joshua Kinard]

On 03/07/2016 10:03, Josh Poimboeuf wrote:
> objtool reports the following warnings:
> 
>   drivers/rtc/rtc-ds1685.o: warning: objtool: ds1685_rtc_work_queue()+0x0: duplicate frame pointer save
>   drivers/rtc/rtc-ds1685.o: warning: objtool: ds1685_rtc_work_queue()+0x3: duplicate frame pointer setup
>   drivers/rtc/rtc-ds1685.o: warning: objtool: ds1685_rtc_work_queue()+0x0: frame pointer state mismatch
> 
> The warning message needs to be improved, but what it really means in
> this case is that ds1685_rtc_poweroff() has a possible code path where
> it can actually fall through to the next function in the object code,
> ds1685_rtc_work_queue().
> 
> The bug is caused by the use of the unreachable() macro in a place which
> is actually reachable.  That causes gcc to assume that the printk()
> immediately before the unreachable() macro never returns, when in fact
> it does.  So gcc places the printk() at the very end of the function's
> object code.  When the printk() returns, the next function starts
> executing.
> 
> The surrounding comment and printk message state that the code should
> spin forever, which explains the unreachable() statement.  However the
> actual spin code is missing.

So this power down trick is used by both SGI O2 (IP32) and SGI Octane (IP30)
systems via this RTC chip, and I've noticed lately that the Octane has stopped
powering off via this function (it just sits and spins forever).  The O2 powers
off as expected.  When I initially wrote this driver from the original version
I found on LKML in '09, I hadn't gotten the Octane code back into a working
shape, and once that happened, I only tested the non-SMP case (fixed Octane SMP
in 4.1).  I suspect on the Octane, the use of SMP may be what is interfering
somehow, and this bug may partially explain it.  This patch doesn't fix
poweroff for me, but it's something to start from when I can get some time to
chase it down.

That said, I initially left the 'while (1);' clause out because at one point
during development, gcc yelled at me for using that at the end of the function,
so I looked at some other drivers and saw the use of 'unreachable();' and used
it instead.  Wasn't aware both of them are needed together in this instance.  I
thought 'unreachable()' evaluated out to a 'while (1)' at the end.  Seems to
actually be some kind of internal gcc trick.

How exactly did the kbuild bot trigger the above warnings?  I've only built and
tested this driver on a MIPS platform and haven't seen that particular warning
before.


> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  drivers/rtc/rtc-ds1685.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
> index 08e0ff8..1e6cfc8 100644
> --- a/drivers/rtc/rtc-ds1685.c
> +++ b/drivers/rtc/rtc-ds1685.c
> @@ -2161,6 +2161,7 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
>  	/* Check for valid RTC data, else, spin forever. */
>  	if (unlikely(!pdev)) {
>  		pr_emerg("platform device data not available, spinning forever ...\n");
> +		while(1);
>  		unreachable();
>  	} else {
>  		/* Get the rtc data. */
> 


-- 
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
6144R/F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943

"The past tempts us, the present confuses us, the future frightens us.  And our
lives slip away, moment by moment, lost in that vast, terrible in-between."

--Emperor Turhan, Centauri Republic

Re: [PATCH] rtc: ds1685: actually spin forever in poweroff error path

[Josh Poimboeuf]

On Mon, Mar 07, 2016 at 04:30:50PM -0500, Joshua Kinard wrote:
> On 03/07/2016 10:03, Josh Poimboeuf wrote:
> > objtool reports the following warnings:
> > 
> >   drivers/rtc/rtc-ds1685.o: warning: objtool: ds1685_rtc_work_queue()+0x0: duplicate frame pointer save
> >   drivers/rtc/rtc-ds1685.o: warning: objtool: ds1685_rtc_work_queue()+0x3: duplicate frame pointer setup
> >   drivers/rtc/rtc-ds1685.o: warning: objtool: ds1685_rtc_work_queue()+0x0: frame pointer state mismatch
> > 
> > The warning message needs to be improved, but what it really means in
> > this case is that ds1685_rtc_poweroff() has a possible code path where
> > it can actually fall through to the next function in the object code,
> > ds1685_rtc_work_queue().
> > 
> > The bug is caused by the use of the unreachable() macro in a place which
> > is actually reachable.  That causes gcc to assume that the printk()
> > immediately before the unreachable() macro never returns, when in fact
> > it does.  So gcc places the printk() at the very end of the function's
> > object code.  When the printk() returns, the next function starts
> > executing.
> > 
> > The surrounding comment and printk message state that the code should
> > spin forever, which explains the unreachable() statement.  However the
> > actual spin code is missing.
> 
> So this power down trick is used by both SGI O2 (IP32) and SGI Octane (IP30)
> systems via this RTC chip, and I've noticed lately that the Octane has stopped
> powering off via this function (it just sits and spins forever).  The O2 powers
> off as expected.  When I initially wrote this driver from the original version
> I found on LKML in '09, I hadn't gotten the Octane code back into a working
> shape, and once that happened, I only tested the non-SMP case (fixed Octane SMP
> in 4.1).  I suspect on the Octane, the use of SMP may be what is interfering
> somehow, and this bug may partially explain it.  This patch doesn't fix
> poweroff for me, but it's something to start from when I can get some time to
> chase it down.
> 
> That said, I initially left the 'while (1);' clause out because at one point
> during development, gcc yelled at me for using that at the end of the function,
> so I looked at some other drivers and saw the use of 'unreachable();' and used
> it instead.  Wasn't aware both of them are needed together in this instance.  I
> thought 'unreachable()' evaluated out to a 'while (1)' at the end.  Seems to
> actually be some kind of internal gcc trick.
> 
> How exactly did the kbuild bot trigger the above warnings?  I've only built and
> tested this driver on a MIPS platform and haven't seen that particular warning
> before.

Hi Joshua,

The warning was emitted by a brand new tool named objtool which does
some static object code analysis. It's currently only in linux-next, not
yet in Linus's tree.  To get the warning, you'd need to build the
linux-next tree for x86_64 with CONFIG_STACK_VALIDATION enabled.

Here's the kbuild bot warning:

  https://lkml.kernel.org/r/201603060005.PHCyifJr%fengguang.wu@intel.com


-- 
Josh