Fwd: Issues with parity error handling in s3c2410 serial driver

Fwd: Issues with parity error handling in s3c2410 serial driver

[Guillermo Rodriguez Garcia]

Hello all,

I think I have found some issues with the handling of parity errors in the
s3c2410 serial driver (drivers/tty/serial/samsung.c)

1.
The driver defines S3C2410_UERSTAT_PARITY as 0x1000
(see http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L265)

However, at least in the S3C2440, the correct value should be 0x02.
This matches the definition in include/linux/serial_s3c.h for the S3C2443 (see
http://lxr.free-electrons.com/source/include/linux/serial_s3c.h#L165)

2.
The driver currently contains code to detect parity errors, but this code is
guarded by the following check:

if (unlikely(uerstat & S3C2410_UERSTAT_ANY)) {
...
}

And the definition of S3C2410_UERSTAT_ANY in include/linux/serial_s3c.h
does not include the parity bit.

This means that the code that currently checks for parity errors in the serial
driver won't actually be run when a parity error occurs. I think that this is a
bug and that S3C2410_UERSTAT_ANY should also contain the parity bit

3.
The code that detects parity errors does not increment the parity error
counter in the serial_icounter_struct structure. Other errors (framing and
overrun) are accounted for -- it is just the parity error that is not traced
(see http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L277).

This means that parity errors will not be reported via the TIOCGICOUNT
ioctl call.


Can someone comment on the above?

Thank you,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: Fwd: Issues with parity error handling in s3c2410 serial driver

[Greg Kroah-Hartman]

On Thu, May 08, 2014 at 01:31:10PM +0200, Guillermo Rodriguez Garcia wrote:
> Hello all,
> 
> I think I have found some issues with the handling of parity errors in the
> s3c2410 serial driver (drivers/tty/serial/samsung.c)
> 
> 1.
> The driver defines S3C2410_UERSTAT_PARITY as 0x1000
> (see http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L265)
> 
> However, at least in the S3C2440, the correct value should be 0x02.
> This matches the definition in include/linux/serial_s3c.h for the S3C2443 (see
> http://lxr.free-electrons.com/source/include/linux/serial_s3c.h#L165)
> 
> 2.
> The driver currently contains code to detect parity errors, but this code is
> guarded by the following check:
> 
> if (unlikely(uerstat & S3C2410_UERSTAT_ANY)) {
> ...
> }
> 
> And the definition of S3C2410_UERSTAT_ANY in include/linux/serial_s3c.h
> does not include the parity bit.
> 
> This means that the code that currently checks for parity errors in the serial
> driver won't actually be run when a parity error occurs. I think that this is a
> bug and that S3C2410_UERSTAT_ANY should also contain the parity bit
> 
> 3.
> The code that detects parity errors does not increment the parity error
> counter in the serial_icounter_struct structure. Other errors (framing and
> overrun) are accounted for -- it is just the parity error that is not traced
> (see http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L277).
> 
> This means that parity errors will not be reported via the TIOCGICOUNT
> ioctl call.
> 
> 
> Can someone comment on the above?

Can you make up a patch to resolve these and be sure to cc: the driver
authors and this list?

thanks,

greg k-h

Re: Fwd: Issues with parity error handling in s3c2410 serial driver

[Guillermo Rodriguez]

El 08/05/2014 21:55, Greg Kroah-Hartman escribió:
> On Thu, May 08, 2014 at 01:31:10PM +0200, Guillermo Rodriguez Garcia wrote:
>> Hello all,
>>
>> I think I have found some issues with the handling of parity errors in the
>> s3c2410 serial driver (drivers/tty/serial/samsung.c)
>>
>> 1.
>> The driver defines S3C2410_UERSTAT_PARITY as 0x1000
>> (see http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L265)
>>
>> However, at least in the S3C2440, the correct value should be 0x02.
>> This matches the definition in include/linux/serial_s3c.h for the S3C2443 (see
>> http://lxr.free-electrons.com/source/include/linux/serial_s3c.h#L165)
>>
>> 2.
>> The driver currently contains code to detect parity errors, but this code is
>> guarded by the following check:
>>
>> if (unlikely(uerstat & S3C2410_UERSTAT_ANY)) {
>> ...
>> }
>>
>> And the definition of S3C2410_UERSTAT_ANY in include/linux/serial_s3c.h
>> does not include the parity bit.
>>
>> This means that the code that currently checks for parity errors in the serial
>> driver won't actually be run when a parity error occurs. I think that this is a
>> bug and that S3C2410_UERSTAT_ANY should also contain the parity bit
>>
>> 3.
>> The code that detects parity errors does not increment the parity error
>> counter in the serial_icounter_struct structure. Other errors (framing and
>> overrun) are accounted for -- it is just the parity error that is not traced
>> (see http://lxr.free-electrons.com/source/drivers/tty/serial/samsung.c#L277).
>>
>> This means that parity errors will not be reported via the TIOCGICOUNT
>> ioctl call.
>>
>>
>> Can someone comment on the above?
> 
> Can you make up a patch to resolve these and be sure to cc: the driver
> authors and this list?

I think it would be a good idea to split this in two patches. Issue 3/ above
is trivial to fix and this can get through easily.

For 1/ and 2/ I am sure that the problem is _not_ specific to the s3c2440
(the way it is now, parity errors are never checked since S3C2410_UERSTAT_ANY
only contains the overrun and framing bit masks). However I can only test on
the s3c2440. Shall I make a patch specifically for the s3c2440, or should I
try to make it generic? If the latter, I could use some help with testing on
other s3c SoCs.

Thank you,

Guillermo Rodriguez
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html