[PATCH -next] serial: etraxfs-uart: Fix crash

[PATCH -next] serial: etraxfs-uart: Fix crash

[Guenter Roeck]

Since commit 7d8c70d8048c ("serial: mctrl-gpio: rename init function"),
crisv32 either do not build or crash as follows.

Unable to handle kernel NULL pointer dereference
Linux 4.3.0-rc7-next-20151101 #1 Sun Nov 1 11:41:28 PST 2015
...
Call Trace: [<c0004a0e>] show_stack+0x0/0x9e
[<c004c0c0>] printk+0x0/0x2c
[<c00059d4>] show_registers+0x14a/0x1c2
[<c004c0c0>] printk+0x0/0x2c
[<c0004b52>] die_if_kernel+0x7c/0x9e
[<c0005346>] do_page_fault+0x32e/0x3e6
[<c01dc59c>] of_get_property+0x0/0x2c
[<c01e0558>] of_irq_parse_raw+0x12a/0x376
[<c01dc59c>] of_get_property+0x0/0x2c
[<c0053aca>] get_page_from_freelist+0x73e/0x856
[<c01dc59c>] of_get_property+0x0/0x2c
[<c0008912>] d_mmu_refill+0x10a/0x112
[<c01b488c>] devm_kmalloc+0x40/0x56
[<c01b47d0>] add_dr+0xc/0x1c
[<c01b4800>] devm_add_action+0x2/0x4e
[<c01abdbc>] mctrl_gpio_init_noauto+0x1c/0x76
[<c01abf9e>] mctrl_gpio_init+0x22/0x110

The function call in the etraxfs-uart driver was not renamed,
possibly due to interference with commit 7b9c5162c182 ("serial:
etraxfs-uart: use mctrl_gpio helpers for handling modem signals").

Fixes: 7d8c70d8048c ("serial: mctrl-gpio: rename init function")
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Niklas Cassel <nks@flawful.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
It might make sense to merge this patch into the original patch if possible.

 drivers/tty/serial/etraxfs-uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/etraxfs-uart.c b/drivers/tty/serial/etraxfs-uart.c
index 6813e316e9ff..2f80bc7e44fb 100644
--- a/drivers/tty/serial/etraxfs-uart.c
+++ b/drivers/tty/serial/etraxfs-uart.c
@@ -894,7 +894,7 @@ static int etraxfs_uart_probe(struct platform_device *pdev)
 	up->regi_ser = of_iomap(np, 0);
 	up->port.dev = &pdev->dev;
 
-	up->gpios = mctrl_gpio_init(&pdev->dev, 0);
+	up->gpios = mctrl_gpio_init_noauto(&pdev->dev, 0);
 	if (IS_ERR(up->gpios))
 		return PTR_ERR(up->gpios);
 
-- 
2.1.4

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Uwe Kleine-König]

Hello,

On Sun, Nov 01, 2015 at 06:32:56PM -0800, Guenter Roeck wrote:
> Since commit 7d8c70d8048c ("serial: mctrl-gpio: rename init function"),
> crisv32 either do not build or crash as follows.
> 
> Unable to handle kernel NULL pointer dereference
> Linux 4.3.0-rc7-next-20151101 #1 Sun Nov 1 11:41:28 PST 2015
> ...
> Call Trace: [<c0004a0e>] show_stack+0x0/0x9e
> [<c004c0c0>] printk+0x0/0x2c
> [<c00059d4>] show_registers+0x14a/0x1c2
> [<c004c0c0>] printk+0x0/0x2c
> [<c0004b52>] die_if_kernel+0x7c/0x9e
> [<c0005346>] do_page_fault+0x32e/0x3e6
> [<c01dc59c>] of_get_property+0x0/0x2c
> [<c01e0558>] of_irq_parse_raw+0x12a/0x376
> [<c01dc59c>] of_get_property+0x0/0x2c
> [<c0053aca>] get_page_from_freelist+0x73e/0x856
> [<c01dc59c>] of_get_property+0x0/0x2c
> [<c0008912>] d_mmu_refill+0x10a/0x112
> [<c01b488c>] devm_kmalloc+0x40/0x56
> [<c01b47d0>] add_dr+0xc/0x1c
> [<c01b4800>] devm_add_action+0x2/0x4e
> [<c01abdbc>] mctrl_gpio_init_noauto+0x1c/0x76
> [<c01abf9e>] mctrl_gpio_init+0x22/0x110

There should be a warning at compile time about mismatching parameters.

> The function call in the etraxfs-uart driver was not renamed,
> possibly due to interference with commit 7b9c5162c182 ("serial:
> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").

Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
is missing.

> Fixes: 7d8c70d8048c ("serial: mctrl-gpio: rename init function")
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Niklas Cassel <nks@flawful.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

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

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Guenter Roeck]

On 11/01/2015 11:37 PM, Uwe Kleine-König wrote:
> Hello,
>
> On Sun, Nov 01, 2015 at 06:32:56PM -0800, Guenter Roeck wrote:
>> Since commit 7d8c70d8048c ("serial: mctrl-gpio: rename init function"),
>> crisv32 either do not build or crash as follows.
>>
>> Unable to handle kernel NULL pointer dereference
>> Linux 4.3.0-rc7-next-20151101 #1 Sun Nov 1 11:41:28 PST 2015
>> ...
>> Call Trace: [<c0004a0e>] show_stack+0x0/0x9e
>> [<c004c0c0>] printk+0x0/0x2c
>> [<c00059d4>] show_registers+0x14a/0x1c2
>> [<c004c0c0>] printk+0x0/0x2c
>> [<c0004b52>] die_if_kernel+0x7c/0x9e
>> [<c0005346>] do_page_fault+0x32e/0x3e6
>> [<c01dc59c>] of_get_property+0x0/0x2c
>> [<c01e0558>] of_irq_parse_raw+0x12a/0x376
>> [<c01dc59c>] of_get_property+0x0/0x2c
>> [<c0053aca>] get_page_from_freelist+0x73e/0x856
>> [<c01dc59c>] of_get_property+0x0/0x2c
>> [<c0008912>] d_mmu_refill+0x10a/0x112
>> [<c01b488c>] devm_kmalloc+0x40/0x56
>> [<c01b47d0>] add_dr+0xc/0x1c
>> [<c01b4800>] devm_add_action+0x2/0x4e
>> [<c01abdbc>] mctrl_gpio_init_noauto+0x1c/0x76
>> [<c01abf9e>] mctrl_gpio_init+0x22/0x110
>
> There should be a warning at compile time about mismatching parameters.
>
>> The function call in the etraxfs-uart driver was not renamed,
>> possibly due to interference with commit 7b9c5162c182 ("serial:
>> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
>
> Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
> is missing.
>

I can not comment on that; all I can say is that it works for me with qemu.

Guenter

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Uwe Kleine-König]

On Mon, Nov 02, 2015 at 06:29:36AM -0800, Guenter Roeck wrote:
> On 11/01/2015 11:37 PM, Uwe Kleine-König wrote:
> >Hello,
> >
> >On Sun, Nov 01, 2015 at 06:32:56PM -0800, Guenter Roeck wrote:
> >>Since commit 7d8c70d8048c ("serial: mctrl-gpio: rename init function"),
> >>crisv32 either do not build or crash as follows.
> >>
> >>Unable to handle kernel NULL pointer dereference
> >>Linux 4.3.0-rc7-next-20151101 #1 Sun Nov 1 11:41:28 PST 2015
> >>...
> >>Call Trace: [<c0004a0e>] show_stack+0x0/0x9e
> >>[<c004c0c0>] printk+0x0/0x2c
> >>[<c00059d4>] show_registers+0x14a/0x1c2
> >>[<c004c0c0>] printk+0x0/0x2c
> >>[<c0004b52>] die_if_kernel+0x7c/0x9e
> >>[<c0005346>] do_page_fault+0x32e/0x3e6
> >>[<c01dc59c>] of_get_property+0x0/0x2c
> >>[<c01e0558>] of_irq_parse_raw+0x12a/0x376
> >>[<c01dc59c>] of_get_property+0x0/0x2c
> >>[<c0053aca>] get_page_from_freelist+0x73e/0x856
> >>[<c01dc59c>] of_get_property+0x0/0x2c
> >>[<c0008912>] d_mmu_refill+0x10a/0x112
> >>[<c01b488c>] devm_kmalloc+0x40/0x56
> >>[<c01b47d0>] add_dr+0xc/0x1c
> >>[<c01b4800>] devm_add_action+0x2/0x4e
> >>[<c01abdbc>] mctrl_gpio_init_noauto+0x1c/0x76
> >>[<c01abf9e>] mctrl_gpio_init+0x22/0x110
> >
> >There should be a warning at compile time about mismatching parameters.
> >
> >>The function call in the etraxfs-uart driver was not renamed,
> >>possibly due to interference with commit 7b9c5162c182 ("serial:
> >>etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
> >
> >Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
> >is missing.
> >
> 
> I can not comment on that; all I can say is that it works for me with qemu.
What does "works for me" mean? You get output on the qemu console? Fine,
but that also works if you don't do any handshaking at all I guess.

Best regards
Uwe

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

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Niklas Cassel]

Acked-by: Niklas Cassel <nks@flawful.org>

On 11/02/2015 03:32 AM, Guenter Roeck wrote:
> Since commit 7d8c70d8048c ("serial: mctrl-gpio: rename init function"),
> crisv32 either do not build or crash as follows.
>
> Unable to handle kernel NULL pointer dereference
> Linux 4.3.0-rc7-next-20151101 #1 Sun Nov 1 11:41:28 PST 2015
> ...
> Call Trace: [<c0004a0e>] show_stack+0x0/0x9e
> [<c004c0c0>] printk+0x0/0x2c
> [<c00059d4>] show_registers+0x14a/0x1c2
> [<c004c0c0>] printk+0x0/0x2c
> [<c0004b52>] die_if_kernel+0x7c/0x9e
> [<c0005346>] do_page_fault+0x32e/0x3e6
> [<c01dc59c>] of_get_property+0x0/0x2c
> [<c01e0558>] of_irq_parse_raw+0x12a/0x376
> [<c01dc59c>] of_get_property+0x0/0x2c
> [<c0053aca>] get_page_from_freelist+0x73e/0x856
> [<c01dc59c>] of_get_property+0x0/0x2c
> [<c0008912>] d_mmu_refill+0x10a/0x112
> [<c01b488c>] devm_kmalloc+0x40/0x56
> [<c01b47d0>] add_dr+0xc/0x1c
> [<c01b4800>] devm_add_action+0x2/0x4e
> [<c01abdbc>] mctrl_gpio_init_noauto+0x1c/0x76
> [<c01abf9e>] mctrl_gpio_init+0x22/0x110
>
> The function call in the etraxfs-uart driver was not renamed,
> possibly due to interference with commit 7b9c5162c182 ("serial:
> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
>
> Fixes: 7d8c70d8048c ("serial: mctrl-gpio: rename init function")
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Niklas Cassel <nks@flawful.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> It might make sense to merge this patch into the original patch if possible.
>
>  drivers/tty/serial/etraxfs-uart.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/etraxfs-uart.c b/drivers/tty/serial/etraxfs-uart.c
> index 6813e316e9ff..2f80bc7e44fb 100644
> --- a/drivers/tty/serial/etraxfs-uart.c
> +++ b/drivers/tty/serial/etraxfs-uart.c
> @@ -894,7 +894,7 @@ static int etraxfs_uart_probe(struct platform_device *pdev)
>  	up->regi_ser = of_iomap(np, 0);
>  	up->port.dev = &pdev->dev;
>  
> -	up->gpios = mctrl_gpio_init(&pdev->dev, 0);
> +	up->gpios = mctrl_gpio_init_noauto(&pdev->dev, 0);
>  	if (IS_ERR(up->gpios))
>  		return PTR_ERR(up->gpios);
>  

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Niklas Cassel]

On 11/02/2015 08:37 AM, Uwe Kleine-König wrote:
>> The function call in the etraxfs-uart driver was not renamed,
>> possibly due to interference with commit 7b9c5162c182 ("serial:
>> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
> Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
> is missing.

The only drivers including serial_mctrl_gpio.h is atmel_serial.c,
clps711x.c, mxs-auart.c and etraxfs-uart.c

etraxfs commit
7b9c5162c182 ("serial: etraxfs-uart: use mctrl_gpio helpers for handling modem signals")
is heavily influenced by
62b0a1b3e759 ("serial: clps711x: Use mctrl_gpio helpers for handling modem signals")

a driver which also appears to lack mctrl irq setup/handling.


Uwe, are you going to convert all serial drivers that includes serial_mctrl_gpio.h to
use the new mctrl_gpio_init? (which also sets up irqs)

Since mctrl gpios are optional in the device tree, I guess mctrl_gpio_init handles if
they are missing. So even if etraxfs-uart.c currently lacks mctrl irq setup/handling,
if we migrate it to use the new mctrl_gpio_init, we will get this automatically?

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Uwe Kleine-König]

Helo Niklas,

On Mon, Nov 02, 2015 at 10:13:25PM +0100, Niklas Cassel wrote:
> On 11/02/2015 08:37 AM, Uwe Kleine-König wrote:
> >> The function call in the etraxfs-uart driver was not renamed,
> >> possibly due to interference with commit 7b9c5162c182 ("serial:
> >> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
> > Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
> > is missing.
> 
> The only drivers including serial_mctrl_gpio.h is atmel_serial.c,
> clps711x.c, mxs-auart.c and etraxfs-uart.c
> 
> etraxfs commit
> 7b9c5162c182 ("serial: etraxfs-uart: use mctrl_gpio helpers for handling modem signals")
> is heavily influenced by
> 62b0a1b3e759 ("serial: clps711x: Use mctrl_gpio helpers for handling modem signals")
> 
> a driver which also appears to lack mctrl irq setup/handling.
> 
> 
> Uwe, are you going to convert all serial drivers that includes serial_mctrl_gpio.h to
> use the new mctrl_gpio_init? (which also sets up irqs)
That is the plan. For drivers like mxs-auart (that do irq handling) this
is just simplification. When the irq handling is missing converting to
the new mctrl_gpio_init might introduce regressions because it does
request_irq which might fail.

> Since mctrl gpios are optional in the device tree, I guess mctrl_gpio_init handles if
> they are missing. So even if etraxfs-uart.c currently lacks mctrl irq setup/handling,
> if we migrate it to use the new mctrl_gpio_init, we will get this automatically?
Right, if there are no gpios specified (new and old) mctrl_gpio is a
nop.

Best regards
Uwe

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

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Niklas Cassel]

I think that Guenter's patch should be merged asap.
Right now the bug it fixes stops us from boot testing in qemu.

For the broken mctrl irq handling part, when Uwe
has converted mxs-auart to use the generic irq init,
I can do the same for etraxfs + make sure that it gets tested
properly on real hardware.

On 11/02/2015 10:29 PM, Uwe Kleine-König wrote:
> Helo Niklas,
>
> On Mon, Nov 02, 2015 at 10:13:25PM +0100, Niklas Cassel wrote:
>> On 11/02/2015 08:37 AM, Uwe Kleine-König wrote:
>>>> The function call in the etraxfs-uart driver was not renamed,
>>>> possibly due to interference with commit 7b9c5162c182 ("serial:
>>>> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
>>> Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
>>> is missing.
>> The only drivers including serial_mctrl_gpio.h is atmel_serial.c,
>> clps711x.c, mxs-auart.c and etraxfs-uart.c
>>
>> etraxfs commit
>> 7b9c5162c182 ("serial: etraxfs-uart: use mctrl_gpio helpers for handling modem signals")
>> is heavily influenced by
>> 62b0a1b3e759 ("serial: clps711x: Use mctrl_gpio helpers for handling modem signals")
>>
>> a driver which also appears to lack mctrl irq setup/handling.
>>
>>
>> Uwe, are you going to convert all serial drivers that includes serial_mctrl_gpio.h to
>> use the new mctrl_gpio_init? (which also sets up irqs)
> That is the plan. For drivers like mxs-auart (that do irq handling) this
> is just simplification. When the irq handling is missing converting to
> the new mctrl_gpio_init might introduce regressions because it does
> request_irq which might fail.
>
>> Since mctrl gpios are optional in the device tree, I guess mctrl_gpio_init handles if
>> they are missing. So even if etraxfs-uart.c currently lacks mctrl irq setup/handling,
>> if we migrate it to use the new mctrl_gpio_init, we will get this automatically?
> Right, if there are no gpios specified (new and old) mctrl_gpio is a
> nop.
>
> Best regards
> Uwe
>

Re: [PATCH -next] serial: etraxfs-uart: Fix crash

[Guenter Roeck]

On 11/15/2015 03:54 PM, Niklas Cassel wrote:
> I think that Guenter's patch should be merged asap.
> Right now the bug it fixes stops us from boot testing in qemu.
>

I'll send out a build (failure) report after -rc1 is out.
Maybe that will create enough visibility to get the patch accepted
(or officially Nacked).

Guenter

> For the broken mctrl irq handling part, when Uwe
> has converted mxs-auart to use the generic irq init,
> I can do the same for etraxfs + make sure that it gets tested
> properly on real hardware.
>
> On 11/02/2015 10:29 PM, Uwe Kleine-König wrote:
>> Helo Niklas,
>>
>> On Mon, Nov 02, 2015 at 10:13:25PM +0100, Niklas Cassel wrote:
>>> On 11/02/2015 08:37 AM, Uwe Kleine-König wrote:
>>>>> The function call in the etraxfs-uart driver was not renamed,
>>>>> possibly due to interference with commit 7b9c5162c182 ("serial:
>>>>> etraxfs-uart: use mctrl_gpio helpers for handling modem signals").
>>>> Yes. BTW, 7b9c5162c182 looks broken, too, because the interrupt handling
>>>> is missing.
>>> The only drivers including serial_mctrl_gpio.h is atmel_serial.c,
>>> clps711x.c, mxs-auart.c and etraxfs-uart.c
>>>
>>> etraxfs commit
>>> 7b9c5162c182 ("serial: etraxfs-uart: use mctrl_gpio helpers for handling modem signals")
>>> is heavily influenced by
>>> 62b0a1b3e759 ("serial: clps711x: Use mctrl_gpio helpers for handling modem signals")
>>>
>>> a driver which also appears to lack mctrl irq setup/handling.
>>>
>>>
>>> Uwe, are you going to convert all serial drivers that includes serial_mctrl_gpio.h to
>>> use the new mctrl_gpio_init? (which also sets up irqs)
>> That is the plan. For drivers like mxs-auart (that do irq handling) this
>> is just simplification. When the irq handling is missing converting to
>> the new mctrl_gpio_init might introduce regressions because it does
>> request_irq which might fail.
>>
>>> Since mctrl gpios are optional in the device tree, I guess mctrl_gpio_init handles if
>>> they are missing. So even if etraxfs-uart.c currently lacks mctrl irq setup/handling,
>>> if we migrate it to use the new mctrl_gpio_init, we will get this automatically?
>> Right, if there are no gpios specified (new and old) mctrl_gpio is a
>> nop.
>>
>> Best regards
>> Uwe
>>
>
>