Re: [PATCH] serial: serial_txx9 driver update

[Russell King <rmk+lkml@arm.linux.org.uk>]

On Sun, Jan 22, 2006 at 12:33:07AM -0800, Andrew Morton wrote:
> Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> >
> > On Sat, Jan 21, 2006 at 11:36:49PM -0800, Andrew Morton wrote:
> > > Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> > > >
> > > >  serial_txx9_verify_port(struct uart_port *port, struct serial_struct *ser)
> > > >   {
> > > >  -	if (ser->irq < 0 ||
> > > >  -	    ser->baud_base < 9600 || ser->type != PORT_TXX9)
> > > >  +	unsigned long new_port = (unsigned long)ser->port +
> > > >  +		((unsigned long)ser->port_high << ((sizeof(long) - sizeof(int)) * 8));
> > > 
> > > Are you sure about this part?  Shifting something left by sizeof(something)
> > > seems very strange.  It'll give different results on 64-bit machines for
> > > the same hardware.  Are you sure it wasn't supposed to be an addition?
> > 
> > There is a definition for that constant - it's called HIGH_BITS_OFFSET.
> 
> There are two definitions, actually.  drivers/serial/serial_core.c and
> drivers/serial/8250.h.
> 
> > No need to try to buggily recreate it.
> 
> Where's the bug in the proposed code?

There isn't, but because it's wider than 80 columns, it got mis-read.
As demonstrated, the 80 column rule _still_ matters for readability.

> Can you tell us what HIGH_BITS_OFFSET actually does?  Stuffing the port
> address into the upper 32-bits of a ulong on 64-bit machines.  Am consumed
> by curiosity.

It's history.  port in serial_struct has been declared as:

        unsigned int    port;

since the year dot, and has been exposed to userland since it's
inception. I guess that 64-bit machines came along, and this was
found to be inadequate.  Rather than deprecate the current interface
and provide a sane API, whoever did this came up with this yucky
solution where

        unsigned int    port_high;

contains the address bits outside the range of "port".  Presumably
they assumed that sizeof(unsigned long) == 2 * sizeof(unsigned int)
here. If not, this silly interface again breaks on us.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core