* [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
@ 2017-03-16 16:51 Mark Salter
2017-03-16 17:28 ` Mark Salter
0 siblings, 1 reply; 6+ messages in thread
From: Mark Salter @ 2017-03-16 16:51 UTC (permalink / raw)
To: Russell King; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, Sudeep Holla
This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.
It completely breaks PL011 SPCR support (a console=ttyAMA0 is
required to get a console even with a valid SPCR table).
Aleksey Makarov also noticed this:
https://lkml.org/lkml/2017/3/1/324
and he is working on a more generalized patch to fix the original
"double printing" problem which this reverted patch was trying to
fix.
https://lkml.org/lkml/2017/3/15/223
So, let's revert the broken patch and live with the double printing
until Aleksey's patch gets upstream.
Signed-off-by: Mark Salter <msalter@redhat.com>
---
drivers/tty/serial/amba-pl011.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8789ea4..a81631f 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -2373,9 +2373,8 @@ 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;
- }
if (uart_parse_earlycon(options, &iotype, &addr, &options))
return -ENODEV;
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
2017-03-16 16:51 [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console" Mark Salter
@ 2017-03-16 17:28 ` Mark Salter
0 siblings, 0 replies; 6+ messages in thread
From: Mark Salter @ 2017-03-16 17:28 UTC (permalink / raw)
To: Russell King; +Cc: Greg Kroah-Hartman, linux-serial, linux-kernel, Sudeep Holla
On Thu, 2017-03-16 at 12:51 -0400, Mark Salter wrote:
> This reverts commit aea9a80ba98a0c9b4de88850260e9fbdcc98360b.
I see now that Aleksey already submitted a revert and it is in tty-next,
so ignore this.
>
> It completely breaks PL011 SPCR support (a console=ttyAMA0 is
> required to get a console even with a valid SPCR table).
>
> Aleksey Makarov also noticed this:
>
> https://lkml.org/lkml/2017/3/1/324
>
> and he is working on a more generalized patch to fix the original
> "double printing" problem which this reverted patch was trying to
> fix.
>
> https://lkml.org/lkml/2017/3/15/223
>
> So, let's revert the broken patch and live with the double printing
> until Aleksey's patch gets upstream.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
> drivers/tty/serial/amba-pl011.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 8789ea4..a81631f 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -2373,9 +2373,8 @@ 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;
> - }
>
> if (uart_parse_earlycon(options, &iotype, &addr, &options))
> return -ENODEV;
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
@ 2017-03-01 12:26 Aleksey Makarov
2017-03-01 12:59 ` Robin Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Aleksey Makarov @ 2017-03-01 12:26 UTC (permalink / raw)
To: linux-serial
Cc: linux-kernel, linux-arm-kernel, Aleksey Makarov, Sudeep Holla,
Greg Kroah-Hartman, Peter Hurley, Russell King, Jiri Slaby
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
2017-03-01 12:26 Aleksey Makarov
@ 2017-03-01 12:59 ` Robin Murphy
2017-03-01 13:01 ` Aleksey Makarov
0 siblings, 1 reply; 6+ messages in thread
From: Robin Murphy @ 2017-03-01 12:59 UTC (permalink / raw)
To: Aleksey Makarov, linux-serial
Cc: Peter Hurley, Greg Kroah-Hartman, Jiri Slaby, linux-kernel,
Russell King, Sudeep Holla, linux-arm-kernel
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;
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
2017-03-01 12:59 ` Robin Murphy
@ 2017-03-01 13:01 ` Aleksey Makarov
2017-03-01 13:55 ` Robin Murphy
0 siblings, 1 reply; 6+ messages in thread
From: Aleksey Makarov @ 2017-03-01 13:01 UTC (permalink / raw)
To: Robin Murphy, linux-serial
Cc: Peter Hurley, Greg Kroah-Hartman, Jiri Slaby, linux-kernel,
Russell King, Sudeep Holla, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console"
2017-03-01 13:01 ` Aleksey Makarov
@ 2017-03-01 13:55 ` Robin Murphy
0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2017-03-01 13:55 UTC (permalink / raw)
To: Aleksey Makarov, linux-serial
Cc: Peter Hurley, Greg Kroah-Hartman, Jiri Slaby, linux-kernel,
Russell King, Sudeep Holla, linux-arm-kernel
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;
>>> }
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-16 17:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-16 16:51 [PATCH] Revert "tty: serial: pl011: add ttyAMA for matching pl011 console" Mark Salter
2017-03-16 17:28 ` Mark Salter
-- strict thread matches above, loose matches on Subject: below --
2017-03-01 12:26 Aleksey Makarov
2017-03-01 12:59 ` Robin Murphy
2017-03-01 13:01 ` Aleksey Makarov
2017-03-01 13:55 ` Robin Murphy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox