clocksource: timer-ti-dm: Fix regression
diff mbox series

Message ID 20200106203700.21009-1-tony@atomide.com
State New
Headers show
Series
  • clocksource: timer-ti-dm: Fix regression
Related show

Commit Message

Tony Lindgren Jan. 6, 2020, 8:37 p.m. UTC
Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
platform_get_irq") caused a regression where we now try to access
uninitialized data for timer:

drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
uninitialized in this function [-Wmaybe-uninitialized]

On boot we now get:

Unable to handle kernel NULL pointer dereference at virtual address
00000004
...
(omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
(platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
(really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)

Let's fix the issue by moving platform_get_irq to happen after timer has
been allocated.

Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to platform_get_irq")
Cc: Yangtao Li <tiny.windzz@gmail.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

I did not notice simlar issue with other patches in the series, but
please do double check Yangtao.

---
 drivers/clocksource/timer-ti-dm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Olof Johansson Jan. 6, 2020, 9:07 p.m. UTC | #1
On Mon, Jan 6, 2020 at 12:37 PM Tony Lindgren <tony@atomide.com> wrote:
>
> Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
> platform_get_irq") caused a regression where we now try to access
> uninitialized data for timer:
>
> drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
> drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>
> On boot we now get:
>
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000004
> ...
> (omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
> (platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
> (really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)
>
> Let's fix the issue by moving platform_get_irq to happen after timer has
> been allocated.
>
> Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to platform_get_irq")
> Cc: Yangtao Li <tiny.windzz@gmail.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Acked-by: Olof Johansson <olof@lixom.net>

> ---
>
> I did not notice simlar issue with other patches in the series, but
> please do double check Yangtao.

Yeah, this even seems to be caught at build (but our builds have been
so noisy with warnings lately that they're hard to spot):

/build/drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
/build/drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may
be used uninitialized in this function [-Wmaybe-uninitialized]
  798 |  timer->irq = platform_get_irq(pdev, 0);
      |  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~


-Olof
Yangtao Li Jan. 7, 2020, 7:14 p.m. UTC | #2
On Tue, Jan 7, 2020 at 5:07 AM Olof Johansson <olof@lixom.net> wrote:
>
> On Mon, Jan 6, 2020 at 12:37 PM Tony Lindgren <tony@atomide.com> wrote:
> >
> > Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
> > platform_get_irq") caused a regression where we now try to access
> > uninitialized data for timer:
> >
> > drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
> > drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> >
> > On boot we now get:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address
> > 00000004
> > ...
> > (omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
> > (platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
> > (really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)
> >
> > Let's fix the issue by moving platform_get_irq to happen after timer has
> > been allocated.
> >
> > Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to platform_get_irq")
> > Cc: Yangtao Li <tiny.windzz@gmail.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
>
> Acked-by: Olof Johansson <olof@lixom.net>

Acked-by: Yangtao Li <tiny.windzz@gmail.com>

I am sorry. I will pay attention next time.

>
> > ---
> >
> > I did not notice simlar issue with other patches in the series, but
> > please do double check Yangtao.
>
> Yeah, this even seems to be caught at build (but our builds have been
> so noisy with warnings lately that they're hard to spot):
>
> /build/drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
> /build/drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may
> be used uninitialized in this function [-Wmaybe-uninitialized]
>   798 |  timer->irq = platform_get_irq(pdev, 0);
>       |  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
> -Olof
Daniel Lezcano Jan. 9, 2020, 11:42 a.m. UTC | #3
On 06/01/2020 21:37, Tony Lindgren wrote:
> Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
> platform_get_irq") caused a regression where we now try to access
> uninitialized data for timer:
> 
> drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
> drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> 
> On boot we now get:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000004
> ...
> (omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
> (platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
> (really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)
> 
> Let's fix the issue by moving platform_get_irq to happen after timer has
> been allocated.
> 
> Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to platform_get_irq")
> Cc: Yangtao Li <tiny.windzz@gmail.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

Applied, thanks.
Naresh Kamboju Jan. 11, 2020, 6:28 a.m. UTC | #4
On Tue, 7 Jan 2020 at 02:07, Tony Lindgren <tony@atomide.com> wrote:
>
> Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
> platform_get_irq") caused a regression where we now try to access
> uninitialized data for timer:
>
> drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
> drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>
> On boot we now get:
>
> Unable to handle kernel NULL pointer dereference at virtual address
> 00000004
> ...
> (omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
> (platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
> (really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)
>
> Let's fix the issue by moving platform_get_irq to happen after timer has
> been allocated.
>
> Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to platform_get_irq")

Thanks for fixing this issue.
I have noticed arm BeagleBoard-X15 boot failed on linux next tree
(5.5.0-rc5-next-20200110).

[    6.157822] 8<--- cut here ---
[    6.160911] Unable to handle kernel NULL pointer dereference at
virtual address 00000004
[    6.169120] pgd = 25d83e32
[    6.171903] [00000004] *pgd=80000080204003, *pmd=00000000
[    6.177358] Internal error: Oops: a06 [#1] SMP ARM
[    6.182179] Modules linked in:
[    6.185260] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
5.5.0-rc5-next-20200110 #1
[    6.192694] Hardware name: Generic DRA74X (Flattened Device Tree)
[    6.198832] PC is at omap_dm_timer_probe+0x48/0x310

- Naresh
J, KEERTHY Jan. 16, 2020, 3:46 a.m. UTC | #5
On 11/01/20 11:58 am, Naresh Kamboju wrote:
> On Tue, 7 Jan 2020 at 02:07, Tony Lindgren <tony@atomide.com> wrote:
>>
>> Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
>> platform_get_irq") caused a regression where we now try to access
>> uninitialized data for timer:
>>
>> drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
>> drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
>> uninitialized in this function [-Wmaybe-uninitialized]
>>
>> On boot we now get:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 00000004
>> ...
>> (omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
>> (platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
>> (really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)
>>
>> Let's fix the issue by moving platform_get_irq to happen after timer has
>> been allocated.
>>
>> Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to platform_get_irq")
> 
> Thanks for fixing this issue.
> I have noticed arm BeagleBoard-X15 boot failed on linux next tree
> (5.5.0-rc5-next-20200110).
> 
> [    6.157822] 8<--- cut here ---
> [    6.160911] Unable to handle kernel NULL pointer dereference at
> virtual address 00000004
> [    6.169120] pgd = 25d83e32
> [    6.171903] [00000004] *pgd=80000080204003, *pmd=00000000
> [    6.177358] Internal error: Oops: a06 [#1] SMP ARM
> [    6.182179] Modules linked in:
> [    6.185260] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 5.5.0-rc5-next-20200110 #1
> [    6.192694] Hardware name: Generic DRA74X (Flattened Device Tree)
> [    6.198832] PC is at omap_dm_timer_probe+0x48/0x310

Tony/Daniel,

This is still not in linux-next. Any idea when this will be pushed to 
linux-next branch?

- Keerthy
> 
> - Naresh
>
Daniel Lezcano Jan. 16, 2020, 1:32 p.m. UTC | #6
On 16/01/2020 04:46, Keerthy wrote:
> 
> 
> On 11/01/20 11:58 am, Naresh Kamboju wrote:
>> On Tue, 7 Jan 2020 at 02:07, Tony Lindgren <tony@atomide.com> wrote:
>>>
>>> Clean-up commit 8c82723414d5 ("clocksource/drivers/timer-ti-dm:
>>> Switch to
>>> platform_get_irq") caused a regression where we now try to access
>>> uninitialized data for timer:
>>>
>>> drivers/clocksource/timer-ti-dm.c: In function 'omap_dm_timer_probe':
>>> drivers/clocksource/timer-ti-dm.c:798:13: warning: 'timer' may be used
>>> uninitialized in this function [-Wmaybe-uninitialized]
>>>
>>> On boot we now get:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address
>>> 00000004
>>> ...
>>> (omap_dm_timer_probe) from [<c061ac7c>] (platform_drv_probe+0x48/0x98)
>>> (platform_drv_probe) from [<c0618c04>] (really_probe+0x1dc/0x348)
>>> (really_probe) from [<c0618ef4>] (driver_probe_device+0x5c/0x160)
>>>
>>> Let's fix the issue by moving platform_get_irq to happen after timer has
>>> been allocated.
>>>
>>> Fixes: 8c82723414d5 ("clocksource/drivers/timer-ti-dm: Switch to
>>> platform_get_irq")
>>
>> Thanks for fixing this issue.
>> I have noticed arm BeagleBoard-X15 boot failed on linux next tree
>> (5.5.0-rc5-next-20200110).
>>
>> [    6.157822] 8<--- cut here ---
>> [    6.160911] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000004
>> [    6.169120] pgd = 25d83e32
>> [    6.171903] [00000004] *pgd=80000080204003, *pmd=00000000
>> [    6.177358] Internal error: Oops: a06 [#1] SMP ARM
>> [    6.182179] Modules linked in:
>> [    6.185260] CPU: 0 PID: 1 Comm: swapper/0 Not tainted
>> 5.5.0-rc5-next-20200110 #1
>> [    6.192694] Hardware name: Generic DRA74X (Flattened Device Tree)
>> [    6.198832] PC is at omap_dm_timer_probe+0x48/0x310
> 
> Tony/Daniel,
> 
> This is still not in linux-next. Any idea when this will be pushed to
> linux-next branch?

It should be actually.

Patch
diff mbox series

diff --git a/drivers/clocksource/timer-ti-dm.c b/drivers/clocksource/timer-ti-dm.c
--- a/drivers/clocksource/timer-ti-dm.c
+++ b/drivers/clocksource/timer-ti-dm.c
@@ -795,14 +795,14 @@  static int omap_dm_timer_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	timer->irq = platform_get_irq(pdev, 0);
-	if (timer->irq < 0)
-		return timer->irq;
-
 	timer = devm_kzalloc(dev, sizeof(*timer), GFP_KERNEL);
 	if (!timer)
 		return  -ENOMEM;
 
+	timer->irq = platform_get_irq(pdev, 0);
+	if (timer->irq < 0)
+		return timer->irq;
+
 	timer->fclk = ERR_PTR(-ENODEV);
 	timer->io_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(timer->io_base))