Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
> On 03/08/2015 06:30 PM, Måns Rullgård wrote:
>> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
>>
>>> On 03/08/2015 05:04 PM, Måns Rullgård wrote:
>>>> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
>>>>
>>>>> Au1x00/RT2800+ doesn't implement the 8250 scratch register (and
>>>>> this may be true of other h/w currently supported by the 8250 driver);
>>>>> read back the canary value written to the scratch register to enable
>>>>> the console h/w restart after resume from system suspend.
>>>>>
>>>>> Fixes: 4516d50aabedb ("serial: 8250: Use canary to restart console ...")
>>>>> Reported-by: Mason <slash.tmp@xxxxxxx>
>>>>> Signed-off-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>>>>> ---
>>>>> drivers/tty/serial/8250/8250_core.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>>>>> index 87df908..50c80b1 100644
>>>>> --- a/drivers/tty/serial/8250/8250_core.c
>>>>> +++ b/drivers/tty/serial/8250/8250_core.c
>>>>> @@ -3486,7 +3486,8 @@ void serial8250_suspend_port(int line)
>>>>> port->type != PORT_8250) {
>>>>> unsigned char canary = 0xa5;
>>>>> serial_out(up, UART_SCR, canary);
>>>>> - up->canary = canary;
>>>>> + if (serial_in(up, UART_SCR) == canary)
>>>>> + up->canary = canary;
>>>>> }
>>>>
>>>> I think this has the opposite of the intended effect. If there is no
>>>> scratch register, up->canary will remain zero, and the reset code in
>>>> serial8250_console_write() won't execute even if it is needed.
>>>
>>> That was intended.
>>>
>>> Case #1 - UART has UART_SCR
>>>
>>> The canary written is read back so up->canary is set to non-zero.
>>> At each subsequent write to console, the scratch register will be read
>>> until the canary has changed, at which time the UART will be
>>> reinitialized and the canary test disabled (on the presumption that
>>> what changed the canary was a power-off).
>>>
>>> Note that if the port does not power-down at system suspend (eg, if
>>> system suspend failed), the canary test is disabled anyway at port resume.
>>
>> If UART_SCR exists, it's obvious that nothing is changed by this patch.
>
> I wrote that for the benefit of those who may be following along, for
> whom that might not be obvious.
>
>>> Case #2 - UART does not have UART_SCR
>>>
>>> The canary written is not read back properly so up->canary remains zero.
>>> No attempt is made to test the canary or reinitialize the UART.
>>> This leaves the uart in the same state it would have been in;
>>> the canary test is useless here because there continues to be output
>>> after port suspend but before machine suspend.
>>
>> It seems to me that the purpose of writing the canary to UART_SCR is to
>> detect if the port needs reinitialising. If UART_SCR doesn't exist, the
>> detection cannot work, and we can assume either 1) that the port needs
>> reinitialising, or 2) that it does not. The current code does #1. You
>> are proposing a switch to #2. I'd be more careful about making such
>> changes.
>
> When I wrote the canary code, I believed that all h/w this driver supports
> (besides the original 8250 which is already special-cased) had a scratch
> register (I was not aware that the Au1x00/RT2880+ did not).
>
> So this patch restores my original intention to not reinit the port unless
> required.
How do you know if it is required without a working scratch register?
--
Måns Rullgård
mans@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
