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

Groups > linux.kernel > #1590307 > unrolled thread

[PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

Back to article view | Back to linux.kernel

Contents

  [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console" Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-01 13:50 +0100
    Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011  console" Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-01 14:10 +0100
      Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011  console" Robin Murphy <robin.murphy@arm.com> - 2017-03-01 15:00 +0100
    Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011  console" Robin Murphy <robin.murphy@arm.com> - 2017-03-01 14:20 +0100
    [PATCH v3] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console" Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-01 16:30 +0100
    [PATCH v2] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console" Aleksey Makarov <aleksey.makarov@linaro.org> - 2017-03-01 17:00 +0100

#1590307 — [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

The original patch makes condition always true, so it is wrong.

This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.
---
 drivers/tty/serial/amba-pl011.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8789ea423ccf..56f92d7348bf 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2373,7 +2373,7 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
 	if (strcmp(name, "qdf2400_e44") == 0) {
 		pr_info_once("UART: Working around QDF2400 SoC erratum 44");
 		qdf2400_e44_present = true;
-	} else if (strcmp(name, "pl011") != 0 || strcmp(name, "ttyAMA") != 0) {
+	} else if (strcmp(name, "pl011") != 0) {
 		return -ENODEV;
 	}
 
-- 
2.11.1

[toc] | [next] | [standalone]

#1590314 — Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

On 03/01/2017 03:59 PM, Robin Murphy wrote:
> On 01/03/17 12:26, Aleksey Makarov wrote:
>> The original patch makes condition always true, so it is wrong.
>>
>> This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.
> 
> It seems fairly clear that the intent of the code merely warrants
> s/||/&&/ - wouldn't it be more straightforward to just fix that?

No, I don't think so.  The description of the patch says that it fixes a problem
of double printing the logs with SPCR and both console=ttyAMA and earlycon are specified
on the command string.  That wrong patch does "fix" it, but introduces
a regression with the regular case.

With s/||/&&/ it would not even 'fix' the described problem.

Thank you
Aleksey Makarov

> Robin.
> 
>> ---
>>  drivers/tty/serial/amba-pl011.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>> index 8789ea423ccf..56f92d7348bf 100644
>> --- a/drivers/tty/serial/amba-pl011.c
>> +++ b/drivers/tty/serial/amba-pl011.c
>> @@ -2373,7 +2373,7 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
>>  	if (strcmp(name, "qdf2400_e44") == 0) {
>>  		pr_info_once("UART: Working around QDF2400 SoC erratum 44");
>>  		qdf2400_e44_present = true;
>> -	} else if (strcmp(name, "pl011") != 0 || strcmp(name, "ttyAMA") != 0) {
>> +	} else if (strcmp(name, "pl011") != 0) {
>>  		return -ENODEV;
>>  	}
>>  
>>
> 

-- 
All the best
Alekséy Makárov

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

#1590337 — Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

On 01/03/17 13:01, Aleksey Makarov wrote:
> 
> 
> On 03/01/2017 03:59 PM, Robin Murphy wrote:
>> On 01/03/17 12:26, Aleksey Makarov wrote:
>>> The original patch makes condition always true, so it is wrong.
>>>
>>> This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.
>>
>> It seems fairly clear that the intent of the code merely warrants
>> s/||/&&/ - wouldn't it be more straightforward to just fix that?
> 
> No, I don't think so.  The description of the patch says that it fixes a problem
> of double printing the logs with SPCR and both console=ttyAMA and earlycon are specified
> on the command string.  That wrong patch does "fix" it, but introduces
> a regression with the regular case.
> 
> With s/||/&&/ it would not even 'fix' the described problem.

Ah, I see, so it's that this fundamental approach itself was flawed, but
the bug causing it to match nothing happened to hide the underlying
problem. It might be useful to call that out explicitly in the commit
log. FWIW the "enabling the main console reprints earlycon contents"
problem has also been present for a while in the non-ACPI case when
relying on stdout-path for both main console and earlycon in DT, i.e.
with just "earlycon" on the command line (it seems to be OK if you
specify "earlycon=pl011,..." or explicitly add "console=ttyAMA0").

Thanks,
Robin.

> 
> Thank you
> Aleksey Makarov
> 
>> Robin.
>>
>>> ---
>>>  drivers/tty/serial/amba-pl011.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
>>> index 8789ea423ccf..56f92d7348bf 100644
>>> --- a/drivers/tty/serial/amba-pl011.c
>>> +++ b/drivers/tty/serial/amba-pl011.c
>>> @@ -2373,7 +2373,7 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
>>>  	if (strcmp(name, "qdf2400_e44") == 0) {
>>>  		pr_info_once("UART: Working around QDF2400 SoC erratum 44");
>>>  		qdf2400_e44_present = true;
>>> -	} else if (strcmp(name, "pl011") != 0 || strcmp(name, "ttyAMA") != 0) {
>>> +	} else if (strcmp(name, "pl011") != 0) {
>>>  		return -ENODEV;
>>>  	}
>>>  
>>>
>>
> 

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

#1590318 — Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

On 01/03/17 12:26, Aleksey Makarov wrote:
> The original patch makes condition always true, so it is wrong.
> 
> This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.

It seems fairly clear that the intent of the code merely warrants
s/||/&&/ - wouldn't it be more straightforward to just fix that?

Robin.

> ---
>  drivers/tty/serial/amba-pl011.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8789ea423ccf..56f92d7348bf 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2373,7 +2373,7 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
>  	if (strcmp(name, "qdf2400_e44") == 0) {
>  		pr_info_once("UART: Working around QDF2400 SoC erratum 44");
>  		qdf2400_e44_present = true;
> -	} else if (strcmp(name, "pl011") != 0 || strcmp(name, "ttyAMA") != 0) {
> +	} else if (strcmp(name, "pl011") != 0) {
>  		return -ENODEV;
>  	}
>  
> 

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

#1590403 — [PATCH v3] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

The original patch makes the condition always true, so it is wrong.

It masks (but not fixes) the bug described in the commit message
but introduces a regression (no console is selected by SPCR)
in regular (no 'console=ttyAMA') case.

s/||/&&/ would not fix the problem as the root cause was identified
incorrectly.

This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---

v3: fix commit message (Robin Murphy)
v2: add Signed-off-by:

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

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8789ea423ccf..56f92d7348bf 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2373,7 +2373,7 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
 	if (strcmp(name, "qdf2400_e44") == 0) {
 		pr_info_once("UART: Working around QDF2400 SoC erratum 44");
 		qdf2400_e44_present = true;
-	} else if (strcmp(name, "pl011") != 0 || strcmp(name, "ttyAMA") != 0) {
+	} else if (strcmp(name, "pl011") != 0) {
 		return -ENODEV;
 	}
 
-- 
2.11.1

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

#1590440 — [PATCH v2] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"

The original patch makes condition always true, so it is wrong.

This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.

Signed-off-by: Aleksey Makarov <aleksey.makarov@linaro.org>
---
v2: add Signed-off-by:

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

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8789ea423ccf..56f92d7348bf 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2373,7 +2373,7 @@ static int __init pl011_console_match(struct console *co, char *name, int idx,
 	if (strcmp(name, "qdf2400_e44") == 0) {
 		pr_info_once("UART: Working around QDF2400 SoC erratum 44");
 		qdf2400_e44_present = true;
-	} else if (strcmp(name, "pl011") != 0 || strcmp(name, "ttyAMA") != 0) {
+	} else if (strcmp(name, "pl011") != 0) {
 		return -ENODEV;
 	}
 
-- 
2.11.1

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web