[PATCH v5 06/10] serial: General support for multipoint addresses

[Greg KH]

On Tue, Apr 26, 2022 at 04:36:49PM +0300, Ilpo Järvinen wrote:
> On Tue, 26 Apr 2022, Greg KH wrote:
> 
> > On Tue, Apr 26, 2022 at 03:24:44PM +0300, Ilpo Järvinen wrote:
> > > Add generic support for serial multipoint addressing. Two new ioctls
> > > are added. TIOCSADDR is used to indicate the destination/receive
> > > address. TIOCGADDR returns the current address in use. The driver
> > > should implement set_addr and get_addr to support addressing mode.
> > > 
> > > Adjust ADDRB clearing to happen only if driver does not provide
> > > set_addr (=the driver doesn't support address mode).
> > > 
> > > This change is necessary for supporting devices with RS485 multipoint
> > > addressing [*]. A following patch in the patch series adds support for
> > > Synopsys Designware UART capable for 9th bit addressing mode. In this
> > > mode, 9th bit is used to indicate an address (byte) within the
> > > communication line. The 9th bit addressing mode is selected using ADDRB
> > > introduced by the previous patch.
> > > 
> > > Transmit addresses / receiver filter are specified by setting the flags
> > > SER_ADDR_DEST and/or SER_ADDR_RECV. When the user supplies the transmit
> > > address, in the 9bit addressing mode it is sent out immediately with
> > > the 9th bit set to 1. After that, the subsequent normal data bytes are
> > > sent with 9th bit as 0 and they are intended to the device with the
> > > given address. It is up to receiver to enforce the filter using
> > > SER_ADDR_RECV. When userspace has supplied the receive address, the
> > > driver is expected to handle the matching of the address and only data
> > > with that address is forwarded to the user. Both SER_ADDR_DEST and
> > > SER_ADDR_RECV can be given at the same time in a single call if the
> > > addresses are the same.
> > > 
> > > The user can clear the receive filter with SER_ADDR_RECV_CLEAR.
> > > 
> > > [*] Technically, RS485 is just an electronic spec and does not itself
> > > specify the 9th bit addressing mode but 9th bit seems at least
> > > "semi-standard" way to do addressing with RS485.
> > > 
> > > Cc: linux-api at vger.kernel.org
> > > Cc: Ivan Kokshaysky <ink at jurassic.park.msu.ru>
> > > Cc: Matt Turner <mattst88 at gmail.com>
> > > Cc: linux-alpha at vger.kernel.org
> > > Cc: Thomas Bogendoerfer <tsbogend at alpha.franken.de>
> > > Cc: linux-mips at vger.kernel.org
> > > Cc: "James E.J. Bottomley" <James.Bottomley at HansenPartnership.com>
> > > Cc: Helge Deller <deller at gmx.de>
> > > Cc: linux-parisc at vger.kernel.org
> > > Cc: Michael Ellerman <mpe at ellerman.id.au>
> > > Cc: Benjamin Herrenschmidt <benh at kernel.crashing.org>
> > > Cc: Paul Mackerras <paulus at samba.org>
> > > Cc: linuxppc-dev at lists.ozlabs.org
> > > Cc: Yoshinori Sato <ysato at users.sourceforge.jp>
> > > Cc: Rich Felker <dalias at libc.org>
> > > Cc: linux-sh at vger.kernel.org
> > > Cc: "David S. Miller" <davem at davemloft.net>
> > > Cc: sparclinux at vger.kernel.org
> > > Cc: Chris Zankel <chris at zankel.net>
> > > Cc: Max Filippov <jcmvbkbc at gmail.com>
> > > Cc: linux-xtensa at linux-xtensa.org
> > > Cc: Arnd Bergmann <arnd at arndb.de>
> > > Cc: linux-arch at vger.kernel.org
> > > Cc: linux-doc at vger.kernel.org
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen at linux.intel.com>
> > > ---
> 
> > > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > > index fa6b16e5fdd8..8cb785ea7087 100644
> > > --- a/include/uapi/linux/serial.h
> > > +++ b/include/uapi/linux/serial.h
> > > @@ -149,4 +149,12 @@ struct serial_iso7816 {
> > >  	__u32	reserved[5];
> > >  };
> > >  
> > > +struct serial_addr {
> > > +	__u32	flags;
> > > +#define SER_ADDR_RECV			(1 << 0)
> > > +#define SER_ADDR_RECV_CLEAR		(1 << 1)
> > > +#define SER_ADDR_DEST			(1 << 2)
> > 
> > You never check for invalid flags being sent to the kernel, which means
> > this api can never change in the future to add new flags :(
> 
> Ok, so you mean the general level should to check
> if (...->flags & ~(SER_ADDR_FLAGS_ALL))
> 	return -EINVAL;
> ?

For any new kernel api you always have to ensure that no "extra" flags
or bits are set and reject it otherwise you can never add any more bits
or flags in the future.  This should be in the Documentation/ directory
for how to add new ioctls somewhere.

> There's some code in the driver that detects invalid flag combinations 
> (in 10/10) but I guess it doesn't satisfies what you're after. It is 
> similar to how serial_rs485 flags is handled, that is, clearing flags it 
> didn't handle (when it can) and returning -EINVAL for impossible 
> combinations such as getting both RECV and DEST addr at the same time.
> I don't know if serial_rs485 flags is a good example at all, it certainly 
> doesn't check whether bits are set where there's no flag defined.
> 
> > And what about struct serial_rs485?  Shouldn't that be used here
> > instead?  Why do we need a new ioctl and structure?
> 
> It is possible (Lukas already mentioned that option too). It just means
> this will be available only on RS485 which could well be enough but Andy 
> mentioned he has in the past come across addressing mode also with some 
> RS232 thing (he didn't remember details anymore and it could be 
> insignificant for the real world of today).

This is rs485 so let's keep it attached to that.  Lots of people do
their own custom addressing schemes on top of 232 but that's up to them
to support in userspace or as a line discipline.

thanks,

greg k-h