linux-kernel - Re: [PATCH v2] serial: 8250_of: Fix for lack of interrupt support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2064e8da-d0da-6d55-e700-af3f428b348d@huawei.com>
Date:   Mon, 17 Sep 2018 15:55:20 +0100
From:   John Garry <john.garry@...wei.com>
To:     <gregkh@...uxfoundation.org>
CC:     Alexander Sverdlin <alexander.sverdlin@...ia.com>,
        <jslaby@...e.com>, <robh@...nel.org>, <joel@....id.au>,
        <kurt@...utronix.de>, <yamada.masahiro@...ionext.com>,
        <linux-serial@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linuxarm@...wei.com>
Subject: Re: [PATCH v2] serial: 8250_of: Fix for lack of interrupt support

On 30/08/2018 19:21, Alexander Sverdlin wrote:
> Hello John!
>

Hi Greg,

Can you kindly consider picking up this patch?

It still applies to the branches today in your tty git, but let me know 
if you still rather I resend.

Thanks,
John

> On 30/08/2018 11:08, John Garry wrote:
>> In commit c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ"), a
>> check was added for the UART driver being probed prior to the parent IRQ
>> controller.
>>
>> Unfortunately this breaks certain boards which have no interrupt support,
>> like Huawei D03.
>>
>> Indeed, the 8250 DT bindings state that interrupts should be supported -
>> not must.
>>
>> To fix, switch from irq_of_parse_and_map() to of_irq_get(), which
>> does relay whether the IRQ host controller domain is not ready, i.e.
>> defer probe, instead of assuming it.
>>
>> Fixes: c58caaab3bf8 ("serial: 8250: of: Defer probe on missing IRQ")
>> Signed-off-by: John Garry <john.garry@...wei.com>
>
> This indeed looks like a proper way to handle both cases:
>
> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@...ia.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@...ia.com>
>
>> ---
>>
>> Change in v2:
>> - fix check on irq value
>>
>> Note: I think that it would better if we could try to get the interrupt
>> 	  before clk+pm enabling, so we don't need to disable later when
>> 	  deferring, but this is not a fix.
>>
>> diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c
>> index af8beef..877fd7f 100644
>> --- a/drivers/tty/serial/8250/8250_of.c
>> +++ b/drivers/tty/serial/8250/8250_of.c
>> @@ -58,7 +58,7 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>>  	struct resource resource;
>>  	struct device_node *np = ofdev->dev.of_node;
>>  	u32 clk, spd, prop;
>> -	int ret;
>> +	int ret, irq;
>>
>>  	memset(port, 0, sizeof *port);
>>
>> @@ -143,21 +143,27 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>>  	if (ret >= 0)
>>  		port->line = ret;
>>
>> -	port->irq = irq_of_parse_and_map(np, 0);
>> -	if (!port->irq) {
>> -		ret = -EPROBE_DEFER;
>> -		goto err_unprepare;
>> +	irq = of_irq_get(np, 0);
>> +	if (irq < 0) {
>> +		if (irq == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto err_unprepare;
>> +		}
>> +		/* IRQ support not mandatory */
>> +		irq = 0;
>>  	}
>>
>> +	port->irq = irq;
>> +
>>  	info->rst = devm_reset_control_get_optional_shared(&ofdev->dev, NULL);
>>  	if (IS_ERR(info->rst)) {
>>  		ret = PTR_ERR(info->rst);
>> -		goto err_dispose;
>> +		goto err_unprepare;
>>  	}
>>
>>  	ret = reset_control_deassert(info->rst);
>>  	if (ret)
>> -		goto err_dispose;
>> +		goto err_unprepare;
>>
>>  	port->type = type;
>>  	port->uartclk = clk;
>> @@ -184,8 +190,6 @@ static int of_platform_serial_setup(struct platform_device *ofdev,
>>  		port->handle_irq = fsl8250_handle_irq;
>>
>>  	return 0;
>> -err_dispose:
>> -	irq_dispose_mapping(port->irq);
>>  err_unprepare:
>>  	clk_disable_unprepare(info->clk);
>>  err_pmruntime:
>>
>