On 03/09/2015 05:34 AM, Mason wrote:
> On 08/03/2015 18:31, Peter Hurley wrote:
>
>> Au1x00/RT2800+ hardware has an alternate register layout which is
>> remapped with lookup tables by the au_serial_in()/out() i/o accessors.
>> However, the h/w does not support the complete 8250 register set, and
>> accesses to unmapped registers cause out-of-bounds lookups. Further,
>> because the lookup tables are defined by designated initializers, the
>> tables may contain unmapped entries (although the current tables do not).
>>
>> Add the AU_MAP() macro to distinguish mapped and unmapped register
>> lookups (unmapped register lookups (initialized to 0) result in negative
>> register offsets). Validate the register index (ie., 'offset') is in the
>> range [0, table size). Return fixed value for unmapped register reads and
>> ignore unmapped register writes.
>>
>> Reported-by: Mason <slash.tmp@xxxxxxx>
>> Signed-off-by: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/tty/serial/8250/8250_core.c | 41 ++++++++++++++++++++++---------------
>> 1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index 5eb95fd..87df908 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -358,34 +358,43 @@ static void default_serial_dl_write(struct uart_8250_port *up, int value)
>> #if defined(CONFIG_MIPS_ALCHEMY) || defined(CONFIG_SERIAL_8250_RT288X)
>>
>> /* Au1x00/RT288x UART hardware has a weird register layout */
>> +#define AU_MAP(x) (0x80 + x)
>> static const u8 au_io_in_map[] = {
>> - [UART_RX] = 0,
>> - [UART_IER] = 2,
>> - [UART_IIR] = 3,
>> - [UART_LCR] = 5,
>> - [UART_MCR] = 6,
>> - [UART_LSR] = 7,
>> - [UART_MSR] = 8,
>> + [UART_RX] = AU_MAP(0),
>> + [UART_IER] = AU_MAP(2),
>> + [UART_IIR] = AU_MAP(3),
>> + [UART_LCR] = AU_MAP(5),
>> + [UART_MCR] = AU_MAP(6),
>> + [UART_LSR] = AU_MAP(7),
>> + [UART_MSR] = AU_MAP(8),
>> };
>>
>> static const u8 au_io_out_map[] = {
>> - [UART_TX] = 1,
>> - [UART_IER] = 2,
>> - [UART_FCR] = 4,
>> - [UART_LCR] = 5,
>> - [UART_MCR] = 6,
>> + [UART_TX] = AU_MAP(1),
>> + [UART_IER] = AU_MAP(2),
>> + [UART_FCR] = AU_MAP(4),
>> + [UART_LCR] = AU_MAP(5),
>> + [UART_MCR] = AU_MAP(6),
>> };
>>
>> static unsigned int au_serial_in(struct uart_port *p, int offset)
>> {
>> - offset = au_io_in_map[offset] << p->regshift;
>> - return __raw_readl(p->membase + offset);
>> + if (offset >= ARRAYSIZE(au_io_in_map))
>> + return UINT_MAX;
>> + offset = au_io_in_map[offset] - AU_MAP(0);
>> + if (offset < 0)
>> + return UINT_MAX;
>> + return __raw_readl(p->membase + (offset << p->regshift));
>> }
>>
>> static void au_serial_out(struct uart_port *p, int offset, int value)
>> {
>> - offset = au_io_out_map[offset] << p->regshift;
>> - __raw_writel(value, p->membase + offset);
>> + if (offset >= ARRAYSIZE(au_io_out_map))
>> + return;
>> + offset = au_io_out_map[offset] - AU_MAP(0);
>> + if (offset < 0)
>> + return;
>> + __raw_writel(value, p->membase + (offset << p->regshift));
>> }
>
> Hello Peter,
>
> I understand your concern about the lookup tables being sparse,
> but once you've checked that they are not, why go to great length
> to detect the issue at run-time?
The code is ridiculously fragile as written, and the fix is
trivial. The register i/o time dwarfs the 2 cpu insns.
> Do you expect register offsets to change in the future?
> (They've been stable for a very long time, according to git-log)
>
> I would just go with "if (offset >= ARRAYSIZE(foo))" and not
> the AU_MAP gymnastics.
I think you answered your question.
On 03/09/2015 05:34 AM, Mason wrote:
> (Although adding offset 7 to au_io_out_map
> would then make that table sparse!)
Nobody will notice at the time if that changes, which will cause more
breakage requiring a bunch more effort. I'd rather just fix it the once.
> On a tangential subject, as Mans stated, Au1x00 does not support
> the scratch register; but Ralink and Tango do. These three
> platforms share the weird register layout (requiring the LUTs),
> but they differ in scratch support.
>
> Would it make any sense to differentiate them, to support scratch
> where it is implemented? (Although adding offset 7 to au_io_out_map
> would then make that table sparse!)
I don't have hardware for that, but if you'd like to submit patches
for that, great! Be aware that the support is really only useful for
debugging system suspend/resume.
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
