On 03/09/2015 08:07 AM, Måns Rullgård wrote:
> 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?
It can't.
And without a working scratch register, the canary check code in console
write would reinitialize port _before_ the system is suspended anyway, which
serves no purpose.
IOW, even without this change, hardware without a working scratch register
will not accidentally be reinitialized on resume from system suspend, because
that will have already happened.
Regards,
Peter Hurley
--
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
