| Message ID | 25c2e99dc116c666a05e641082a2690c05c09a23.1457362965.git.jpoimboe@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
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. */ >
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
On 07/03/2016 at 09:03:02 -0600, 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. > > 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(+) > Applied, thanks.
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. */
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. 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(+)