soc/tegra: Register SoC device

Message ID 20170822111922.GA15965@ulmo
State Superseded
Headers show

Commit Message

Thierry Reding Aug. 22, 2017, 11:19 a.m.
On Tue, Aug 22, 2017 at 11:15:21AM +0100, Sudeep Holla wrote:
> 
> 
> On 17/08/17 15:42, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Move this code from arch/arm/mach-tegra and make it common among 32-bit
> > and 64-bit Tegra SoCs. This is slightly complicated by the fact that on
> > 32-bit Tegra, the SoC device is used as the parent for all devices that
> > are instantiated from device tree.
> > 
> 
> This seem to be in linux-next and causing the below splat on my platform
> which is not Tegra :)
> 
> WARNING: .... at drivers/soc/tegra/fuse/tegra-apbmisc.c:48
> tegra_get_chip_id+0x30/0x40
> Modules linked in:
> CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> 4.13.0-rc6-next-20170822-00008-g52a8e57512ae #13
> task: ffff8009768a0000 task.stack: ffff000008038000
> PC is at tegra_get_chip_id+0x30/0x40
> LR is at tegra_get_chip_id+0x30/0x40
>  tegra_get_chip_id+0x30/0x40
>  tegra_soc_device_register+0x68/0xd0
>  tegra_init_soc+0x10/0x44
>  do_one_initcall+0x38/0x120
>  kernel_init_freeable+0x184/0x224
>  kernel_init+0x10/0x100
>  ret_from_fork+0x10/0x18

Indeed. Does the below patch fix this?

Thierry

--- >8 ---
From 5706cacc2af5e4ef138d1cd9e1269ca4947a447f Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Tue, 22 Aug 2017 13:15:18 +0200
Subject: [PATCH] soc/tegra: Restrict SoC device registration to Tegra

Commit 8a46828e623c ("soc/tegra: Register SoC device") added an initcall
to register the SoC device on Tegra. However, that code is unrestricted
and will run on all platforms, causing unwanted warnings.

Fix this by first checking that we're running on hardware that supports
the fuses block that we use to provide SoC information.

Fixes: 8a46828e623c ("soc/tegra: Register SoC device")
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/soc/tegra/fuse/fuse-tegra.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Sudeep Holla Aug. 22, 2017, 3 p.m. | #1
On 22/08/17 12:19, Thierry Reding wrote:
> On Tue, Aug 22, 2017 at 11:15:21AM +0100, Sudeep Holla wrote:
>>
>>
>> On 17/08/17 15:42, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Move this code from arch/arm/mach-tegra and make it common among 32-bit
>>> and 64-bit Tegra SoCs. This is slightly complicated by the fact that on
>>> 32-bit Tegra, the SoC device is used as the parent for all devices that
>>> are instantiated from device tree.
>>>
>>
>> This seem to be in linux-next and causing the below splat on my platform
>> which is not Tegra :)
>>
>> WARNING: .... at drivers/soc/tegra/fuse/tegra-apbmisc.c:48
>> tegra_get_chip_id+0x30/0x40
>> Modules linked in:
>> CPU: 2 PID: 1 Comm: swapper/0 Not tainted
>> 4.13.0-rc6-next-20170822-00008-g52a8e57512ae #13
>> task: ffff8009768a0000 task.stack: ffff000008038000
>> PC is at tegra_get_chip_id+0x30/0x40
>> LR is at tegra_get_chip_id+0x30/0x40
>>  tegra_get_chip_id+0x30/0x40
>>  tegra_soc_device_register+0x68/0xd0
>>  tegra_init_soc+0x10/0x44
>>  do_one_initcall+0x38/0x120
>>  kernel_init_freeable+0x184/0x224
>>  kernel_init+0x10/0x100
>>  ret_from_fork+0x10/0x18
> 
> Indeed. Does the below patch fix this?
> 

Yes, it does.
Arnd Bergmann Aug. 23, 2017, 8:49 a.m. | #2
On Tue, Aug 22, 2017 at 1:19 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Tue, Aug 22, 2017 at 11:15:21AM +0100, Sudeep Holla wrote:
>>
>>
>> On 17/08/17 15:42, Thierry Reding wrote:
>> > From: Thierry Reding <treding@nvidia.com>
>> >
>> > Move this code from arch/arm/mach-tegra and make it common among 32-bit
>> > and 64-bit Tegra SoCs. This is slightly complicated by the fact that on
>> > 32-bit Tegra, the SoC device is used as the parent for all devices that
>> > are instantiated from device tree.
>> >
>>
>> This seem to be in linux-next and causing the below splat on my platform
>> which is not Tegra :)
>>
>> WARNING: .... at drivers/soc/tegra/fuse/tegra-apbmisc.c:48
>> tegra_get_chip_id+0x30/0x40
>> Modules linked in:
>> CPU: 2 PID: 1 Comm: swapper/0 Not tainted
>> 4.13.0-rc6-next-20170822-00008-g52a8e57512ae #13
>> task: ffff8009768a0000 task.stack: ffff000008038000
>> PC is at tegra_get_chip_id+0x30/0x40
>> LR is at tegra_get_chip_id+0x30/0x40
>>  tegra_get_chip_id+0x30/0x40
>>  tegra_soc_device_register+0x68/0xd0
>>  tegra_init_soc+0x10/0x44
>>  do_one_initcall+0x38/0x120
>>  kernel_init_freeable+0x184/0x224
>>  kernel_init+0x10/0x100
>>  ret_from_fork+0x10/0x18
>
> Indeed. Does the below patch fix this?
>
> Thierry
>
> --- >8 ---
> From 5706cacc2af5e4ef138d1cd9e1269ca4947a447f Mon Sep 17 00:00:00 2001
> From: Thierry Reding <treding@nvidia.com>
> Date: Tue, 22 Aug 2017 13:15:18 +0200
> Subject: [PATCH] soc/tegra: Restrict SoC device registration to Tegra
>
> Commit 8a46828e623c ("soc/tegra: Register SoC device") added an initcall
> to register the SoC device on Tegra. However, that code is unrestricted
> and will run on all platforms, causing unwanted warnings.
>
> Fix this by first checking that we're running on hardware that supports
> the fuses block that we use to provide SoC information.
>
> Fixes: 8a46828e623c ("soc/tegra: Register SoC device")
> Signed-off-by: Thierry Reding <treding@nvidia.com>

I just found the same thing by inspection after running into

drivers/soc/tegra/fuse/fuse-tegra.c:360:0: error: expected ';' at end of input

Please fix that as well.

Lastly, I believe that tegra_init_fuse() has the same bug, it just doesn't lead
to an Oops, just to a harmless warning message, but please add another
check there, or combine the two checks in some form.

     Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Aug. 23, 2017, 9:11 a.m. | #3
On Wed, Aug 23, 2017 at 10:49:36AM +0200, Arnd Bergmann wrote:
> On Tue, Aug 22, 2017 at 1:19 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Tue, Aug 22, 2017 at 11:15:21AM +0100, Sudeep Holla wrote:
> >>
> >>
> >> On 17/08/17 15:42, Thierry Reding wrote:
> >> > From: Thierry Reding <treding@nvidia.com>
> >> >
> >> > Move this code from arch/arm/mach-tegra and make it common among 32-bit
> >> > and 64-bit Tegra SoCs. This is slightly complicated by the fact that on
> >> > 32-bit Tegra, the SoC device is used as the parent for all devices that
> >> > are instantiated from device tree.
> >> >
> >>
> >> This seem to be in linux-next and causing the below splat on my platform
> >> which is not Tegra :)
> >>
> >> WARNING: .... at drivers/soc/tegra/fuse/tegra-apbmisc.c:48
> >> tegra_get_chip_id+0x30/0x40
> >> Modules linked in:
> >> CPU: 2 PID: 1 Comm: swapper/0 Not tainted
> >> 4.13.0-rc6-next-20170822-00008-g52a8e57512ae #13
> >> task: ffff8009768a0000 task.stack: ffff000008038000
> >> PC is at tegra_get_chip_id+0x30/0x40
> >> LR is at tegra_get_chip_id+0x30/0x40
> >>  tegra_get_chip_id+0x30/0x40
> >>  tegra_soc_device_register+0x68/0xd0
> >>  tegra_init_soc+0x10/0x44
> >>  do_one_initcall+0x38/0x120
> >>  kernel_init_freeable+0x184/0x224
> >>  kernel_init+0x10/0x100
> >>  ret_from_fork+0x10/0x18
> >
> > Indeed. Does the below patch fix this?
> >
> > Thierry
> >
> > --- >8 ---
> > From 5706cacc2af5e4ef138d1cd9e1269ca4947a447f Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <treding@nvidia.com>
> > Date: Tue, 22 Aug 2017 13:15:18 +0200
> > Subject: [PATCH] soc/tegra: Restrict SoC device registration to Tegra
> >
> > Commit 8a46828e623c ("soc/tegra: Register SoC device") added an initcall
> > to register the SoC device on Tegra. However, that code is unrestricted
> > and will run on all platforms, causing unwanted warnings.
> >
> > Fix this by first checking that we're running on hardware that supports
> > the fuses block that we use to provide SoC information.
> >
> > Fixes: 8a46828e623c ("soc/tegra: Register SoC device")
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> I just found the same thing by inspection after running into
> 
> drivers/soc/tegra/fuse/fuse-tegra.c:360:0: error: expected ';' at end of input
> 
> Please fix that as well.

Done, though it's strange that I didn't see those errors. Are those
compiler errors or sparse errors?

I'll be sending out both patches right away.

> Lastly, I believe that tegra_init_fuse() has the same bug, it just doesn't lead
> to an Oops, just to a harmless warning message, but please add another
> check there, or combine the two checks in some form.

I don't think that's the case. The of_find_matching_node_and_match()
should catch all non-Tegra cases, and the backward-compatibility
fallback has an extra check on the SoC compatible. See the comment on
lines 295-298.

That code is fairly old and I haven't seen anybody bring up any errors
on non-Tegra platforms for that particular piece.

Thanks,
Thierry
Arnd Bergmann Aug. 23, 2017, 9:21 a.m. | #4
On Wed, Aug 23, 2017 at 11:11 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Wed, Aug 23, 2017 at 10:49:36AM +0200, Arnd Bergmann wrote:
>> On Tue, Aug 22, 2017 at 1:19 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Tue, Aug 22, 2017 at 11:15:21AM +0100, Sudeep Holla wrote:
>> >>
>> >>
>> >> On 17/08/17 15:42, Thierry Reding wrote:
>> >> > From: Thierry Reding <treding@nvidia.com>
>> >> >
>> >> > Move this code from arch/arm/mach-tegra and make it common among 32-bit
>> >> > and 64-bit Tegra SoCs. This is slightly complicated by the fact that on
>> >> > 32-bit Tegra, the SoC device is used as the parent for all devices that
>> >> > are instantiated from device tree.
>> >> >
>> >>
>> >> This seem to be in linux-next and causing the below splat on my platform
>> >> which is not Tegra :)
>> >>
>> >> WARNING: .... at drivers/soc/tegra/fuse/tegra-apbmisc.c:48
>> >> tegra_get_chip_id+0x30/0x40
>> >> Modules linked in:
>> >> CPU: 2 PID: 1 Comm: swapper/0 Not tainted
>> >> 4.13.0-rc6-next-20170822-00008-g52a8e57512ae #13
>> >> task: ffff8009768a0000 task.stack: ffff000008038000
>> >> PC is at tegra_get_chip_id+0x30/0x40
>> >> LR is at tegra_get_chip_id+0x30/0x40
>> >>  tegra_get_chip_id+0x30/0x40
>> >>  tegra_soc_device_register+0x68/0xd0
>> >>  tegra_init_soc+0x10/0x44
>> >>  do_one_initcall+0x38/0x120
>> >>  kernel_init_freeable+0x184/0x224
>> >>  kernel_init+0x10/0x100
>> >>  ret_from_fork+0x10/0x18
>> >
>> > Indeed. Does the below patch fix this?
>> >
>> > Thierry
>> >
>> > --- >8 ---
>> > From 5706cacc2af5e4ef138d1cd9e1269ca4947a447f Mon Sep 17 00:00:00 2001
>> > From: Thierry Reding <treding@nvidia.com>
>> > Date: Tue, 22 Aug 2017 13:15:18 +0200
>> > Subject: [PATCH] soc/tegra: Restrict SoC device registration to Tegra
>> >
>> > Commit 8a46828e623c ("soc/tegra: Register SoC device") added an initcall
>> > to register the SoC device on Tegra. However, that code is unrestricted
>> > and will run on all platforms, causing unwanted warnings.
>> >
>> > Fix this by first checking that we're running on hardware that supports
>> > the fuses block that we use to provide SoC information.
>> >
>> > Fixes: 8a46828e623c ("soc/tegra: Register SoC device")
>> > Signed-off-by: Thierry Reding <treding@nvidia.com>
>>
>> I just found the same thing by inspection after running into
>>
>> drivers/soc/tegra/fuse/fuse-tegra.c:360:0: error: expected ';' at end of input
>>
>> Please fix that as well.
>
> Done, though it's strange that I didn't see those errors. Are those
> compiler errors or sparse errors?

It's from the compiler. I just upgraded to gcc-8, which might explain
it, or it could be configuration (module/built-in) dependent.

> I'll be sending out both patches right away.

Thanks!

>> Lastly, I believe that tegra_init_fuse() has the same bug, it just doesn't lead
>> to an Oops, just to a harmless warning message, but please add another
>> check there, or combine the two checks in some form.
>
> I don't think that's the case. The of_find_matching_node_and_match()
> should catch all non-Tegra cases, and the backward-compatibility
> fallback has an extra check on the SoC compatible. See the comment on
> lines 295-298.
>
> That code is fairly old and I haven't seen anybody bring up any errors
> on non-Tegra platforms for that particular piece.

My mistake, I see how it works now. Thanks for checking.

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

Patch

diff --git a/drivers/soc/tegra/fuse/fuse-tegra.c b/drivers/soc/tegra/fuse/fuse-tegra.c
index 02686378dfc1..161522bd98ac 100644
--- a/drivers/soc/tegra/fuse/fuse-tegra.c
+++ b/drivers/soc/tegra/fuse/fuse-tegra.c
@@ -349,8 +349,16 @@  early_initcall(tegra_init_fuse);
 #ifdef CONFIG_ARM64
 static int __init tegra_init_soc(void)
 {
+	struct device_node *np;
 	struct device *soc;
 
+	/* make sure we're running on Tegra */
+	np = of_find_matching_node(NULL, tegra_fuse_match);
+	if (!np)
+		return 0;
+
+	of_node_put(np);
+
 	soc = tegra_soc_device_register();
 	if (IS_ERR(soc)) {
 		pr_err("failed to register SoC device: %ld\n", PTR_ERR(soc));