Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1482905 > unrolled thread

Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift gpio_mxc_init() to subsys_initcall level'

Back to article view | Back to linux.kernel

Contents

  Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Guenter Roeck <linux@roeck-us.net> - 2016-09-14 05:30 +0200
    Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Linus Walleij <linus.walleij@linaro.org> - 2016-09-14 09:30 +0200
      Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Guenter Roeck <linux@roeck-us.net> - 2016-09-14 16:40 +0200
        Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Linus Walleij <linus.walleij@linaro.org> - 2016-09-15 14:20 +0200
          Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-09-15 15:20 +0200
            Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-09-15 15:50 +0200
            Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Linus Walleij <linus.walleij@linaro.org> - 2016-09-15 16:00 +0200
      Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-09-15 15:20 +0200
    Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-09-15 15:40 +0200
      Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2016-09-15 16:30 +0200
        Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Guenter Roeck <linux@roeck-us.net> - 2016-09-15 16:40 +0200
          Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Uwe Kleine-König          <u.kleine-koenig@pengutronix.de> - 2016-09-15 16:50 +0200
            Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Guenter Roeck <linux@roeck-us.net> - 2016-09-16 04:30 +0200
      Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Shawn Guo <shawnguo@kernel.org> - 2016-09-18 02:10 +0200
        Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Guenter Roeck <linux@roeck-us.net> - 2016-09-18 03:10 +0200
          Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Linus Walleij <linus.walleij@linaro.org> - 2016-09-18 13:30 +0200
            Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Shawn Guo <shawnguo@kernel.org> - 2016-09-20 15:00 +0200
              Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-09-20 16:00 +0200
                Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Shawn Guo <shawnguo@kernel.org> - 2016-09-20 16:20 +0200
              Re: Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift  gpio_mxc_init() to subsys_initcall level' Linus Walleij <linus.walleij@linaro.org> - 2016-09-20 23:40 +0200

#1482905 — Crashing 'kzm' target in next-20160913 due to 'gpio: mxc: shift gpio_mxc_init() to subsys_initcall level'

Hi Vladimir,

your commit e188cbf7564f ("gpio: mxc: shift gpio_mxc_init() to subsys_initcall level")
in -next causes the following crash when running the 'kzm' target (and most likely
the real thing) with qemu.

[    1.211426] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
[    1.211600] pgd = c0004000
[    1.211680] [0000000c] *pgd=00000000
[    1.212067] Internal error: Oops: 5 [#1] SMP ARM
[    1.212245] Modules linked in:
[    1.212542] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-next-20160913 #1
[    1.212671] Hardware name: Kyoto Microcomputer Co., Ltd. KZM-ARM11-01
[    1.212825] task: c6848000 task.stack: c683e000
[    1.213231] PC is at platform_get_irq+0xc0/0xe8

See http://kerneltests.org/builders/qemu-arm-next/builds/525/steps/qemubuildcommand/logs/stdio
for a complete log.

Problem is quite subtle. The change causes the gpio driver to be installed later.
As a result, kzm_init_smsc9118() fails to initialize the gpio pins correctly.
gpio_request() in that function returns -EPROBE_DEFER, which is ignored,
gpio_to_irq() then returns -22 which is unconditionally assigned as interrupt number.
platform_get_irq(), as called from the smsc driver, gets this negative interrupt
number, and passes it unconditionally to irq_get_irq_data(), which returns NULL.
The NULL pointer is then passed to irqd_set_trigger_type() which, not entirely
surprisingly, crashes.

So, in other words, lots of bugs here. Nevertheless, I would suggest to keep using
postcore_initcall(), at least until it is sure that all gpio clients handle -EPROBE_DEFER
correctly.

Thanks,
Guenter

[toc] | [next] | [standalone]

#1482978

On Wed, Sep 14, 2016 at 5:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:

> So, in other words, lots of bugs here. Nevertheless, I would suggest to keep
> using postcore_initcall(), at least until it is sure that all gpio clients handle
> -EPROBE_DEFER
> correctly.

So can I just revert this patch in isolation? None of the other patches
depend on it?

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1483394

On 09/14/2016 12:19 AM, Linus Walleij wrote:
> On Wed, Sep 14, 2016 at 5:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>
>> So, in other words, lots of bugs here. Nevertheless, I would suggest to keep
>> using postcore_initcall(), at least until it is sure that all gpio clients handle
>> -EPROBE_DEFER
>> correctly.
>
> So can I just revert this patch in isolation? None of the other patches
> depend on it?
>
Good question, if this was part of a series. If it was and can not be removed
in isolation, it nevertheless introduces a regression. Obviously there was
a reason for using postcore_initcall (which is quite evident in the crash),
and I don't think it is a good idea to just change it without addressing
the reason(s) first. Who knows what other platforms crash because of this
change.

Guenter

[toc] | [prev] | [next] | [standalone]

#1484078

On Wed, Sep 14, 2016 at 4:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 09/14/2016 12:19 AM, Linus Walleij wrote:
>>
>> On Wed, Sep 14, 2016 at 5:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>>> So, in other words, lots of bugs here. Nevertheless, I would suggest to
>>> keep
>>> using postcore_initcall(), at least until it is sure that all gpio
>>> clients handle
>>> -EPROBE_DEFER
>>> correctly.
>>
>>
>> So can I just revert this patch in isolation? None of the other patches
>> depend on it?
>>
> Good question, if this was part of a series.

No response from author, so betting on it and reverting this in isolation
with your Reported-by.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1484159

Hi Linus,

On 09/15/2016 03:19 PM, Linus Walleij wrote:
> On Wed, Sep 14, 2016 at 4:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 09/14/2016 12:19 AM, Linus Walleij wrote:
>>>
>>> On Wed, Sep 14, 2016 at 5:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>>> So, in other words, lots of bugs here. Nevertheless, I would suggest to
>>>> keep
>>>> using postcore_initcall(), at least until it is sure that all gpio
>>>> clients handle
>>>> -EPROBE_DEFER
>>>> correctly.
>>>
>>>
>>> So can I just revert this patch in isolation? None of the other patches
>>> depend on it?
>>>
>> Good question, if this was part of a series.
>
> No response from author, so betting on it and reverting this in isolation
> with your Reported-by.
>

please revert the revert :)

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1484186

Hi Linus,

On 09/15/2016 04:18 PM, Vladimir Zapolskiy wrote:
> Hi Linus,
>
> On 09/15/2016 03:19 PM, Linus Walleij wrote:
>> On Wed, Sep 14, 2016 at 4:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On 09/14/2016 12:19 AM, Linus Walleij wrote:
>>>>
>>>> On Wed, Sep 14, 2016 at 5:20 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>
>>>>> So, in other words, lots of bugs here. Nevertheless, I would suggest to
>>>>> keep
>>>>> using postcore_initcall(), at least until it is sure that all gpio
>>>>> clients handle
>>>>> -EPROBE_DEFER
>>>>> correctly.
>>>>
>>>>
>>>> So can I just revert this patch in isolation? None of the other patches
>>>> depend on it?
>>>>
>>> Good question, if this was part of a series.
>>
>> No response from author, so betting on it and reverting this in isolation
>> with your Reported-by.
>>
>
> please revert the revert :)
>

ok, the problem is recognized, but as I said it is not sufficient to revert
only that change, if it is selected to go by path of reverts commit
("gpio: mxc: add generic gpio request/free callbacks to pinctrl") from
your for-next branch should be reverted as well, the latter commit depends
on the reverted commit.

I have one i.MX31 powered board on hand, I'll start preparing a proper
fix for the issue.

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1484199

On Thu, Sep 15, 2016 at 3:18 PM, Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:

>> No response from author, so betting on it and reverting this in isolation
>> with your Reported-by.
>>
>
> please revert the revert :)

Sure I hadn't pushed it yet anyway.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1484149

Hi Linus, Guenter,

On 09/14/2016 10:19 AM, Linus Walleij wrote:
> On Wed, Sep 14, 2016 at 5:20 AM, Guenter Roeck <linux@roeck-us.net>
> wrote:
>
>> So, in other words, lots of bugs here. Nevertheless, I would
>> suggest to keep using postcore_initcall(), at least until it is
>> sure that all gpio clients handle -EPROBE_DEFER correctly.
>
> So can I just revert this patch in isolation? None of the other
> patches depend on it?
>

no, its revert I believe will break (at least cause -EPROBE_DEFER)
on most of modern iMX platforms.

Let me check the essense of the problem, in my opinion it would
be better to fix kzm target.

Guenter, thank you for the early report.

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1484168

Hi Guenter,

On 09/14/2016 06:20 AM, Guenter Roeck wrote:
> Hi Vladimir,
>
> your commit e188cbf7564f ("gpio: mxc: shift gpio_mxc_init() to subsys_initcall level")
> in -next causes the following crash when running the 'kzm' target (and most likely
> the real thing) with qemu.
>
> [    1.211426] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> [    1.211600] pgd = c0004000
> [    1.211680] [0000000c] *pgd=00000000
> [    1.212067] Internal error: Oops: 5 [#1] SMP ARM
> [    1.212245] Modules linked in:
> [    1.212542] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-next-20160913 #1
> [    1.212671] Hardware name: Kyoto Microcomputer Co., Ltd. KZM-ARM11-01
> [    1.212825] task: c6848000 task.stack: c683e000
> [    1.213231] PC is at platform_get_irq+0xc0/0xe8
>
> See http://kerneltests.org/builders/qemu-arm-next/builds/525/steps/qemubuildcommand/logs/stdio
> for a complete log.
>
> Problem is quite subtle. The change causes the gpio driver to be installed later.
> As a result, kzm_init_smsc9118() fails to initialize the gpio pins correctly.
> gpio_request() in that function returns -EPROBE_DEFER, which is ignored,
> gpio_to_irq() then returns -22 which is unconditionally assigned as interrupt number.
> platform_get_irq(), as called from the smsc driver, gets this negative interrupt
> number, and passes it unconditionally to irq_get_irq_data(), which returns NULL.
> The NULL pointer is then passed to irqd_set_trigger_type() which, not entirely
> surprisingly, crashes.
>
> So, in other words, lots of bugs here. Nevertheless, I would suggest to keep using
> postcore_initcall(), at least until it is sure that all gpio clients handle -EPROBE_DEFER
> correctly.

I'm inviting Shawn and Uwe to the discussion.

The proper fix in this particular case should be like this one:

diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
index 31df4361996f..8288acfe7221 100644
--- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
+++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
@@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
  
  	mxc_iomux_setup_multiple_pins(kzm_pins,
  				      ARRAY_SIZE(kzm_pins), "kzm");
-	kzm_init_ext_uart();
-	kzm_init_smsc9118();
  	kzm_init_imx_uart();
  
  	pr_info("Clock input source is 26MHz\n");
  }
  
+static void __init kzm_late_init(void)
+{
+	kzm_init_ext_uart();
+	kzm_init_smsc9118();
+}
+
  /*
   * This structure defines static mappings for the kzm-arm11-01 board.
   */
@@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
  	.init_irq = mx31_init_irq,
  	.init_time	= kzm_timer_init,
  	.init_machine = kzm_board_init,
+	.init_late	= kzm_late_init,
  	.restart	= mxc_restart,
  MACHINE_END
--

But I agree that there might be more legacy boards (i.MX31 only IMHO),
which may attempt to manipulate GPIO lines before subsys_initcall()
level.

Would it be better to move i.MX31 IOMUX controller driver under pinctrl
roof?

Any suggestions are welcome.

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1484223

On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
> Hi Guenter,
> 
> On 09/14/2016 06:20 AM, Guenter Roeck wrote:
> > Hi Vladimir,
> > 
> > your commit e188cbf7564f ("gpio: mxc: shift gpio_mxc_init() to subsys_initcall level")
> > in -next causes the following crash when running the 'kzm' target (and most likely
> > the real thing) with qemu.
> > 
> > [    1.211426] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> > [    1.211600] pgd = c0004000
> > [    1.211680] [0000000c] *pgd=00000000
> > [    1.212067] Internal error: Oops: 5 [#1] SMP ARM
> > [    1.212245] Modules linked in:
> > [    1.212542] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-next-20160913 #1
> > [    1.212671] Hardware name: Kyoto Microcomputer Co., Ltd. KZM-ARM11-01
> > [    1.212825] task: c6848000 task.stack: c683e000
> > [    1.213231] PC is at platform_get_irq+0xc0/0xe8
> > 
> > See http://kerneltests.org/builders/qemu-arm-next/builds/525/steps/qemubuildcommand/logs/stdio
> > for a complete log.
> > 
> > Problem is quite subtle. The change causes the gpio driver to be installed later.
> > As a result, kzm_init_smsc9118() fails to initialize the gpio pins correctly.
> > gpio_request() in that function returns -EPROBE_DEFER, which is ignored,
> > gpio_to_irq() then returns -22 which is unconditionally assigned as interrupt number.
> > platform_get_irq(), as called from the smsc driver, gets this negative interrupt
> > number, and passes it unconditionally to irq_get_irq_data(), which returns NULL.
> > The NULL pointer is then passed to irqd_set_trigger_type() which, not entirely
> > surprisingly, crashes.
> > 
> > So, in other words, lots of bugs here. Nevertheless, I would suggest to keep using
> > postcore_initcall(), at least until it is sure that all gpio clients handle -EPROBE_DEFER
> > correctly.
> 
> I'm inviting Shawn and Uwe to the discussion.
> 
> The proper fix in this particular case should be like this one:
> 
> diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
> index 31df4361996f..8288acfe7221 100644
> --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
> +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
> @@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
>  	mxc_iomux_setup_multiple_pins(kzm_pins,
>  				      ARRAY_SIZE(kzm_pins), "kzm");
> -	kzm_init_ext_uart();
> -	kzm_init_smsc9118();
>  	kzm_init_imx_uart();
>  	pr_info("Clock input source is 26MHz\n");
>  }
> +static void __init kzm_late_init(void)
> +{
> +	kzm_init_ext_uart();
> +	kzm_init_smsc9118();
> +}
> +
>  /*
>   * This structure defines static mappings for the kzm-arm11-01 board.
>   */
> @@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
>  	.init_irq = mx31_init_irq,
>  	.init_time	= kzm_timer_init,
>  	.init_machine = kzm_board_init,
> +	.init_late	= kzm_late_init,
>  	.restart	= mxc_restart,
>  MACHINE_END

That + checking the return code of gpio_request and the other calls.
Or better, convert the machine to dt.

> But I agree that there might be more legacy boards (i.MX31 only IMHO),
> which may attempt to manipulate GPIO lines before subsys_initcall()
> level.

I wouldn't revert anything for legacy boards. That's the chance to say
in the near future: They stopped working in September 2016, obviously
nobody cares, let's rip them. :-)
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[toc] | [prev] | [next] | [standalone]

#1484232

On 09/15/2016 07:23 AM, Uwe Kleine-König wrote:
> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
>> Hi Guenter,
>>
>> On 09/14/2016 06:20 AM, Guenter Roeck wrote:
>>> Hi Vladimir,
>>>
>>> your commit e188cbf7564f ("gpio: mxc: shift gpio_mxc_init() to subsys_initcall level")
>>> in -next causes the following crash when running the 'kzm' target (and most likely
>>> the real thing) with qemu.
>>>
>>> [    1.211426] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
>>> [    1.211600] pgd = c0004000
>>> [    1.211680] [0000000c] *pgd=00000000
>>> [    1.212067] Internal error: Oops: 5 [#1] SMP ARM
>>> [    1.212245] Modules linked in:
>>> [    1.212542] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-next-20160913 #1
>>> [    1.212671] Hardware name: Kyoto Microcomputer Co., Ltd. KZM-ARM11-01
>>> [    1.212825] task: c6848000 task.stack: c683e000
>>> [    1.213231] PC is at platform_get_irq+0xc0/0xe8
>>>
>>> See http://kerneltests.org/builders/qemu-arm-next/builds/525/steps/qemubuildcommand/logs/stdio
>>> for a complete log.
>>>
>>> Problem is quite subtle. The change causes the gpio driver to be installed later.
>>> As a result, kzm_init_smsc9118() fails to initialize the gpio pins correctly.
>>> gpio_request() in that function returns -EPROBE_DEFER, which is ignored,
>>> gpio_to_irq() then returns -22 which is unconditionally assigned as interrupt number.
>>> platform_get_irq(), as called from the smsc driver, gets this negative interrupt
>>> number, and passes it unconditionally to irq_get_irq_data(), which returns NULL.
>>> The NULL pointer is then passed to irqd_set_trigger_type() which, not entirely
>>> surprisingly, crashes.
>>>
>>> So, in other words, lots of bugs here. Nevertheless, I would suggest to keep using
>>> postcore_initcall(), at least until it is sure that all gpio clients handle -EPROBE_DEFER
>>> correctly.
>>
>> I'm inviting Shawn and Uwe to the discussion.
>>
>> The proper fix in this particular case should be like this one:
>>
>> diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
>> index 31df4361996f..8288acfe7221 100644
>> --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
>> +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
>> @@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
>>  	mxc_iomux_setup_multiple_pins(kzm_pins,
>>  				      ARRAY_SIZE(kzm_pins), "kzm");
>> -	kzm_init_ext_uart();
>> -	kzm_init_smsc9118();
>>  	kzm_init_imx_uart();
>>  	pr_info("Clock input source is 26MHz\n");
>>  }
>> +static void __init kzm_late_init(void)
>> +{
>> +	kzm_init_ext_uart();
>> +	kzm_init_smsc9118();
>> +}
>> +
>>  /*
>>   * This structure defines static mappings for the kzm-arm11-01 board.
>>   */
>> @@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
>>  	.init_irq = mx31_init_irq,
>>  	.init_time	= kzm_timer_init,
>>  	.init_machine = kzm_board_init,
>> +	.init_late	= kzm_late_init,
>>  	.restart	= mxc_restart,
>>  MACHINE_END
>
> That + checking the return code of gpio_request and the other calls.
> Or better, convert the machine to dt.
>
>> But I agree that there might be more legacy boards (i.MX31 only IMHO),
>> which may attempt to manipulate GPIO lines before subsys_initcall()
>> level.
>
> I wouldn't revert anything for legacy boards. That's the chance to say
> in the near future: They stopped working in September 2016, obviously
> nobody cares, let's rip them. :-)
>
New kernel development philosophy ? Regressions are acceptable as long as
they affect a board older than X years ? What is your cut-off date for accepting
regressions like that ?

If that is the new philosophy, can it be made official ? That would help me
understand when I don't need to bother sending bug reports.

Following that new development paradigm, should I refrain from reporting that
arm-realview-eb-11mp-revb has been broken in -next since next-20160914 ?

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1484256

On Thu, Sep 15, 2016 at 07:35:16AM -0700, Guenter Roeck wrote:
> On 09/15/2016 07:23 AM, Uwe Kleine-König wrote:
> > On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
> > > Hi Guenter,
> > > 
> > > On 09/14/2016 06:20 AM, Guenter Roeck wrote:
> > > > Hi Vladimir,
> > > > 
> > > > your commit e188cbf7564f ("gpio: mxc: shift gpio_mxc_init() to subsys_initcall level")
> > > > in -next causes the following crash when running the 'kzm' target (and most likely
> > > > the real thing) with qemu.
> > > > 
> > > > [    1.211426] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
> > > > [    1.211600] pgd = c0004000
> > > > [    1.211680] [0000000c] *pgd=00000000
> > > > [    1.212067] Internal error: Oops: 5 [#1] SMP ARM
> > > > [    1.212245] Modules linked in:
> > > > [    1.212542] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-next-20160913 #1
> > > > [    1.212671] Hardware name: Kyoto Microcomputer Co., Ltd. KZM-ARM11-01
> > > > [    1.212825] task: c6848000 task.stack: c683e000
> > > > [    1.213231] PC is at platform_get_irq+0xc0/0xe8
> > > > 
> > > > See http://kerneltests.org/builders/qemu-arm-next/builds/525/steps/qemubuildcommand/logs/stdio
> > > > for a complete log.
> > > > 
> > > > Problem is quite subtle. The change causes the gpio driver to be installed later.
> > > > As a result, kzm_init_smsc9118() fails to initialize the gpio pins correctly.
> > > > gpio_request() in that function returns -EPROBE_DEFER, which is ignored,
> > > > gpio_to_irq() then returns -22 which is unconditionally assigned as interrupt number.
> > > > platform_get_irq(), as called from the smsc driver, gets this negative interrupt
> > > > number, and passes it unconditionally to irq_get_irq_data(), which returns NULL.
> > > > The NULL pointer is then passed to irqd_set_trigger_type() which, not entirely
> > > > surprisingly, crashes.
> > > > 
> > > > So, in other words, lots of bugs here. Nevertheless, I would suggest to keep using
> > > > postcore_initcall(), at least until it is sure that all gpio clients handle -EPROBE_DEFER
> > > > correctly.
> > > 
> > > I'm inviting Shawn and Uwe to the discussion.
> > > 
> > > The proper fix in this particular case should be like this one:
> > > 
> > > diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
> > > index 31df4361996f..8288acfe7221 100644
> > > --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
> > > +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
> > > @@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
> > >  	mxc_iomux_setup_multiple_pins(kzm_pins,
> > >  				      ARRAY_SIZE(kzm_pins), "kzm");
> > > -	kzm_init_ext_uart();
> > > -	kzm_init_smsc9118();
> > >  	kzm_init_imx_uart();
> > >  	pr_info("Clock input source is 26MHz\n");
> > >  }
> > > +static void __init kzm_late_init(void)
> > > +{
> > > +	kzm_init_ext_uart();
> > > +	kzm_init_smsc9118();
> > > +}
> > > +
> > >  /*
> > >   * This structure defines static mappings for the kzm-arm11-01 board.
> > >   */
> > > @@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
> > >  	.init_irq = mx31_init_irq,
> > >  	.init_time	= kzm_timer_init,
> > >  	.init_machine = kzm_board_init,
> > > +	.init_late	= kzm_late_init,
> > >  	.restart	= mxc_restart,
> > >  MACHINE_END
> > 
> > That + checking the return code of gpio_request and the other calls.
> > Or better, convert the machine to dt.
> > 
> > > But I agree that there might be more legacy boards (i.MX31 only IMHO),
> > > which may attempt to manipulate GPIO lines before subsys_initcall()
> > > level.
> > 
> > I wouldn't revert anything for legacy boards. That's the chance to say
> > in the near future: They stopped working in September 2016, obviously
> > nobody cares, let's rip them. :-)
> > 
> New kernel development philosophy ? Regressions are acceptable as long as
> they affect a board older than X years ? What is your cut-off date for accepting
> regressions like that ?

I would soften your statement about regressions to: Regressions like
these are an incentive to pre-dt platforms to modernize. Of course they
should ideally be prevented, and fixed when they happen, but I wouldn't
start reverting gpio-changes because of a regression on a machine that
is hardly used with modern kernel (ok, that's an assumption, but it
didn't receive enough love to be converted to dt), that fails to even
check return values of gpio_request and that can be easily fixed.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[toc] | [prev] | [next] | [standalone]

#1484651

On 09/15/2016 07:46 AM, Uwe Kleine-König wrote:
> On Thu, Sep 15, 2016 at 07:35:16AM -0700, Guenter Roeck wrote:
>> On 09/15/2016 07:23 AM, Uwe Kleine-König wrote:
>>> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
>>>> Hi Guenter,
>>>>
>>>> On 09/14/2016 06:20 AM, Guenter Roeck wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> your commit e188cbf7564f ("gpio: mxc: shift gpio_mxc_init() to subsys_initcall level")
>>>>> in -next causes the following crash when running the 'kzm' target (and most likely
>>>>> the real thing) with qemu.
>>>>>
>>>>> [    1.211426] Unable to handle kernel NULL pointer dereference at virtual address 0000000c
>>>>> [    1.211600] pgd = c0004000
>>>>> [    1.211680] [0000000c] *pgd=00000000
>>>>> [    1.212067] Internal error: Oops: 5 [#1] SMP ARM
>>>>> [    1.212245] Modules linked in:
>>>>> [    1.212542] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc6-next-20160913 #1
>>>>> [    1.212671] Hardware name: Kyoto Microcomputer Co., Ltd. KZM-ARM11-01
>>>>> [    1.212825] task: c6848000 task.stack: c683e000
>>>>> [    1.213231] PC is at platform_get_irq+0xc0/0xe8
>>>>>
>>>>> See http://kerneltests.org/builders/qemu-arm-next/builds/525/steps/qemubuildcommand/logs/stdio
>>>>> for a complete log.
>>>>>
>>>>> Problem is quite subtle. The change causes the gpio driver to be installed later.
>>>>> As a result, kzm_init_smsc9118() fails to initialize the gpio pins correctly.
>>>>> gpio_request() in that function returns -EPROBE_DEFER, which is ignored,
>>>>> gpio_to_irq() then returns -22 which is unconditionally assigned as interrupt number.
>>>>> platform_get_irq(), as called from the smsc driver, gets this negative interrupt
>>>>> number, and passes it unconditionally to irq_get_irq_data(), which returns NULL.
>>>>> The NULL pointer is then passed to irqd_set_trigger_type() which, not entirely
>>>>> surprisingly, crashes.
>>>>>
>>>>> So, in other words, lots of bugs here. Nevertheless, I would suggest to keep using
>>>>> postcore_initcall(), at least until it is sure that all gpio clients handle -EPROBE_DEFER
>>>>> correctly.
>>>>
>>>> I'm inviting Shawn and Uwe to the discussion.
>>>>
>>>> The proper fix in this particular case should be like this one:
>>>>
>>>> diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
>>>> index 31df4361996f..8288acfe7221 100644
>>>> --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
>>>> +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
>>>> @@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
>>>>  	mxc_iomux_setup_multiple_pins(kzm_pins,
>>>>  				      ARRAY_SIZE(kzm_pins), "kzm");
>>>> -	kzm_init_ext_uart();
>>>> -	kzm_init_smsc9118();
>>>>  	kzm_init_imx_uart();
>>>>  	pr_info("Clock input source is 26MHz\n");
>>>>  }
>>>> +static void __init kzm_late_init(void)
>>>> +{
>>>> +	kzm_init_ext_uart();
>>>> +	kzm_init_smsc9118();
>>>> +}
>>>> +
>>>>  /*
>>>>   * This structure defines static mappings for the kzm-arm11-01 board.
>>>>   */
>>>> @@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
>>>>  	.init_irq = mx31_init_irq,
>>>>  	.init_time	= kzm_timer_init,
>>>>  	.init_machine = kzm_board_init,
>>>> +	.init_late	= kzm_late_init,
>>>>  	.restart	= mxc_restart,
>>>>  MACHINE_END
>>>
>>> That + checking the return code of gpio_request and the other calls.
>>> Or better, convert the machine to dt.
>>>
>>>> But I agree that there might be more legacy boards (i.MX31 only IMHO),
>>>> which may attempt to manipulate GPIO lines before subsys_initcall()
>>>> level.
>>>
>>> I wouldn't revert anything for legacy boards. That's the chance to say
>>> in the near future: They stopped working in September 2016, obviously
>>> nobody cares, let's rip them. :-)
>>>
>> New kernel development philosophy ? Regressions are acceptable as long as
>> they affect a board older than X years ? What is your cut-off date for accepting
>> regressions like that ?
>
> I would soften your statement about regressions to: Regressions like
> these are an incentive to pre-dt platforms to modernize. Of course they
> should ideally be prevented, and fixed when they happen, but I wouldn't
> start reverting gpio-changes because of a regression on a machine that
> is hardly used with modern kernel (ok, that's an assumption, but it
> didn't receive enough love to be converted to dt), that fails to even
> check return values of gpio_request and that can be easily fixed.
>
All 890 of them ? Really ?

Guenter

[toc] | [prev] | [next] | [standalone]

#1485650

Hi Guenter,

On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
> The proper fix in this particular case should be like this one:
> 

Does Vladimir's patch below fix your problem?

Shawn

> diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
> index 31df4361996f..8288acfe7221 100644
> --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
> +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
> @@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
>  	mxc_iomux_setup_multiple_pins(kzm_pins,
>  				      ARRAY_SIZE(kzm_pins), "kzm");
> -	kzm_init_ext_uart();
> -	kzm_init_smsc9118();
>  	kzm_init_imx_uart();
>  	pr_info("Clock input source is 26MHz\n");
>  }
> +static void __init kzm_late_init(void)
> +{
> +	kzm_init_ext_uart();
> +	kzm_init_smsc9118();
> +}
> +
>  /*
>   * This structure defines static mappings for the kzm-arm11-01 board.
>   */
> @@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
>  	.init_irq = mx31_init_irq,
>  	.init_time	= kzm_timer_init,
>  	.init_machine = kzm_board_init,
> +	.init_late	= kzm_late_init,
>  	.restart	= mxc_restart,
>  MACHINE_END
> --

[toc] | [prev] | [next] | [standalone]

#1485657

On 09/17/2016 05:08 PM, Shawn Guo wrote:
> Hi Guenter,
>
> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
>> The proper fix in this particular case should be like this one:
>>
>
> Does Vladimir's patch below fix your problem?
>

Yes, it does. Feel free to add
Tested-by: Guenter Roeck <linux@roeck-us.net>
to an official patch.

Guenter

> Shawn
>
>> diff --git a/arch/arm/mach-imx/mach-kzm_arm11_01.c b/arch/arm/mach-imx/mach-kzm_arm11_01.c
>> index 31df4361996f..8288acfe7221 100644
>> --- a/arch/arm/mach-imx/mach-kzm_arm11_01.c
>> +++ b/arch/arm/mach-imx/mach-kzm_arm11_01.c
>> @@ -245,13 +245,17 @@ static void __init kzm_board_init(void)
>>  	mxc_iomux_setup_multiple_pins(kzm_pins,
>>  				      ARRAY_SIZE(kzm_pins), "kzm");
>> -	kzm_init_ext_uart();
>> -	kzm_init_smsc9118();
>>  	kzm_init_imx_uart();
>>  	pr_info("Clock input source is 26MHz\n");
>>  }
>> +static void __init kzm_late_init(void)
>> +{
>> +	kzm_init_ext_uart();
>> +	kzm_init_smsc9118();
>> +}
>> +
>>  /*
>>   * This structure defines static mappings for the kzm-arm11-01 board.
>>   */
>> @@ -291,5 +295,6 @@ MACHINE_START(KZM_ARM11_01, "Kyoto Microcomputer Co., Ltd. KZM-ARM11-01")
>>  	.init_irq = mx31_init_irq,
>>  	.init_time	= kzm_timer_init,
>>  	.init_machine = kzm_board_init,
>> +	.init_late	= kzm_late_init,
>>  	.restart	= mxc_restart,
>>  MACHINE_END
>> --
>

[toc] | [prev] | [next] | [standalone]

#1485767

On Sun, Sep 18, 2016 at 2:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 09/17/2016 05:08 PM, Shawn Guo wrote:
>>
>> Hi Guenter,
>>
>> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
>>>
>>> The proper fix in this particular case should be like this one:
>>>
>>
>> Does Vladimir's patch below fix your problem?
>>
>
> Yes, it does. Feel free to add
> Tested-by: Guenter Roeck <linux@roeck-us.net>
> to an official patch.

Hope to get this patch soon...

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1487313

On Sun, Sep 18, 2016 at 01:24:42PM +0200, Linus Walleij wrote:
> On Sun, Sep 18, 2016 at 2:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 09/17/2016 05:08 PM, Shawn Guo wrote:
> >>
> >> Hi Guenter,
> >>
> >> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
> >>>
> >>> The proper fix in this particular case should be like this one:
> >>>
> >>
> >> Does Vladimir's patch below fix your problem?
> >>
> >
> > Yes, it does. Feel free to add
> > Tested-by: Guenter Roeck <linux@roeck-us.net>
> > to an official patch.
> 
> Hope to get this patch soon...

That will be an IMX platform patch, and we can merge it through arm-soc
tree as a fix during -rc.

Shawn

[toc] | [prev] | [next] | [standalone]

#1487371

Hi,

On 09/20/2016 03:55 PM, Shawn Guo wrote:
> On Sun, Sep 18, 2016 at 01:24:42PM +0200, Linus Walleij wrote:
>> On Sun, Sep 18, 2016 at 2:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On 09/17/2016 05:08 PM, Shawn Guo wrote:
>>>>
>>>> Hi Guenter,
>>>>
>>>> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
>>>>>
>>>>> The proper fix in this particular case should be like this one:
>>>>>
>>>>
>>>> Does Vladimir's patch below fix your problem?
>>>>
>>>
>>> Yes, it does. Feel free to add
>>> Tested-by: Guenter Roeck <linux@roeck-us.net>
>>> to an official patch.
>>
>> Hope to get this patch soon...
>
> That will be an IMX platform patch, and we can merge it through arm-soc
> tree as a fix during -rc.
>

for sake of clarity the discussed change is the first one in the series
of fixes:

   https://patchwork.ozlabs.org/patch/671519/

I've noticed that Guenter's Tested-by tag is missing though.

--
With best wishes,
Vladimir

[toc] | [prev] | [next] | [standalone]

#1487383

On Tue, Sep 20, 2016 at 04:53:59PM +0300, Vladimir Zapolskiy wrote:
> for sake of clarity the discussed change is the first one in the series
> of fixes:
> 
>   https://patchwork.ozlabs.org/patch/671519/
> 
> I've noticed that Guenter's Tested-by tag is missing though.

Do not worry.  I will add it when applying the patch.

Shawn

[toc] | [prev] | [next] | [standalone]

#1487652

On Tue, Sep 20, 2016 at 2:55 PM, Shawn Guo <shawnguo@kernel.org> wrote:
> On Sun, Sep 18, 2016 at 01:24:42PM +0200, Linus Walleij wrote:
>> On Sun, Sep 18, 2016 at 2:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On 09/17/2016 05:08 PM, Shawn Guo wrote:
>> >>
>> >> Hi Guenter,
>> >>
>> >> On Thu, Sep 15, 2016 at 04:35:04PM +0300, Vladimir Zapolskiy wrote:
>> >>>
>> >>> The proper fix in this particular case should be like this one:
>> >>>
>> >>
>> >> Does Vladimir's patch below fix your problem?
>> >>
>> >
>> > Yes, it does. Feel free to add
>> > Tested-by: Guenter Roeck <linux@roeck-us.net>
>> > to an official patch.
>>
>> Hope to get this patch soon...
>
> That will be an IMX platform patch, and we can merge it through arm-soc
> tree as a fix during -rc.

Awesome, thanks!

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web