On 03/09/2015 07:43 AM, Måns Rullgård wrote:
> Peter Hurley <peter@xxxxxxxxxxxxxxxxxx> writes:
>
>> 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));
>> }
>
> Maybe the intent would be clearer if you used bitwise operations instead
> of arithmetic:
>
> #define AU_MAP(x) (0x80 | (x))
> [...]
> offset = au_io_in_map[offset];
> if (!(offset & 0x80))
> return UINT_MAX;
> offset &= 0x7f;
> return __raw_readl(...);
>
That's how I originally wrote it, but I'm not sure that's any better.
Perhaps a better way to do this would be to explicitly declare the
table sizes for the complete 8250 register set and specifically
initialize the unmapped registers to -1.
That would do away with AU_MAP() and arithmetic, while it would still
make it more obvious that unmapped registers need specific initialization
and would cause compile errors if an initializer extended the register
map.
Also, perhaps this would be a good time to add register map shifting
so that UART_EFR accesses aren't masqueraded.
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
