netdev - Re: [PATCH 0/5] can: enable multi-queue for SocketCAN devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 7 Jun 2018 08:06:17 +0000
From:   "Jonas Mark (BT-FIR/ENG1)" <Mark.Jonas@...bosch.com>
To:     Oliver Hartkopp <socketcan@...tkopp.net>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>
CC:     "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "hs@...x.de" <hs@...x.de>,
        "ZHU Yi (BT-FIR/ENG1-Zhu)" <Yi.Zhu5@...bosch.com>
Subject: Re: [PATCH 0/5] can: enable multi-queue for SocketCAN devices

Hi Oliver,

> > The driver suite consists of three separate drivers. The following
> > diagram illustrates the dependencies in layers.
> >
> >             /dev/companion       SocketCAN                User Space
> > -------------------------------------------------------------------
> >           +----------------+ +---------------+
> >           | companion-char | | companion-can |
> >           +----------------+ +---------------+
> >           +----------------------------------+
> >           |          companion-spi           |
> >           +----------------------------------+
> >           +----------------------------------+
> >           |     standard SPI subsystem       |
> >           +----------------------------------+          Linux Kernel
> > -------------------------------------------------------------------
> >                 | | | |      | |                            Hardware
> >              CS-+ | | |      | +-BUSY
> >              CLK--+ | |      +---REQUEST
> >              MOSI---+ |
> >              MISO-----+
> >
> > companion-spi
> >     core.c: handles SPI, sysfs entry and interface to upper layer
> >     protocol-manager.c: handles protocol with the SPI HW
> >     queue-manager.c: handles buffering and packets scheduling
> >
> > companion-can
> >     makes use of multi-queue support and allows to use tc to configure
> >     the queuing discipline (e.g. mqprio). Together with the SO_PRIORITY
> >     socket option this allows to specify the FIFO a CAN frame shall be
> >     sent to.
> >
> > companion-char
> >     handles messages to other undisclosed functionality beyond CAN.

> >   .../devicetree/bindings/spi/bosch,companion.txt    |   82 ++
> >   drivers/char/Kconfig                               |    7 +
> >   drivers/char/Makefile                              |    2 +
> >   drivers/char/companion-char.c                      |  367 ++++++
> >   drivers/net/can/Kconfig                            |    8 +
> >   drivers/net/can/Makefile                           |    1 +
> >   drivers/net/can/companion-can.c                    |  694 ++++++++++++
> 
> Please place the companion driver in
> 
> drivers/net/can/spi/companion.c
> 
> It also makes more sense in the Kconfig structure.
> 
> Probably this naming scheme also makes sense for
> 
> linux/drivers/char/spi/companion.c
> 
> then ...
> 
> If not it should be named at least
> 
> drivers/char/companion-spi.c
> 
> or
> 
> drivers/char/spi-companion.c

We intentionally left out the spi in the driver path / name because
only the drivers/spi/companion/* driver knows that that it is connected
to SPI. The others (drivers/net/can/companion-can.c and
drivers/char/companion-char.c) only know the API. This could also be
supplied by a driver which talks to the Companion via a different
interface. Actually, we started with a UART connection but switched to
SPI due to latency issues.

Should we still change it?

> >   drivers/net/can/dev.c                              |    8 +-
> >   drivers/spi/Kconfig                                |    2 +
> >   drivers/spi/Makefile                               |    2 +
> >   drivers/spi/companion/Kconfig                      |    5 +
> >   drivers/spi/companion/Makefile                     |    2 +
> >   drivers/spi/companion/core.c                       | 1189 ++++++++++++++++++++
> >   drivers/spi/companion/protocol-manager.c           | 1035 +++++++++++++++++
> >   drivers/spi/companion/protocol-manager.h           |  348 ++++++
> >   drivers/spi/companion/protocol.h                   |  273 +++++
> >   drivers/spi/companion/queue-manager.c              |  146 +++
> >   drivers/spi/companion/queue-manager.h              |  245 ++++
> >   include/linux/can/dev.h                            |    7 +-
> >   include/linux/companion.h                          |  258 +++++
> >   20 files changed, 4677 insertions(+), 4 deletions(-)
> >   create mode 100644
> Documentation/devicetree/bindings/spi/bosch,companion.txt
> >   create mode 100644 drivers/char/companion-char.c
> >   create mode 100644 drivers/net/can/companion-can.c
> >   create mode 100644 drivers/spi/companion/Kconfig
> >   create mode 100644 drivers/spi/companion/Makefile
> >   create mode 100644 drivers/spi/companion/core.c
> >   create mode 100644 drivers/spi/companion/protocol-manager.c
> >   create mode 100644 drivers/spi/companion/protocol-manager.h
> >   create mode 100644 drivers/spi/companion/protocol.h
> >   create mode 100644 drivers/spi/companion/queue-manager.c
> >   create mode 100644 drivers/spi/companion/queue-manager.h
> >   create mode 100644 include/linux/companion.h

Greetings,
Mark

Building Technologies, Panel Software Fire (BT-FIR/ENG1) 
Bosch Sicherheitssysteme GmbH | Postfach 11 11 | 85626 Grasbrunn | GERMANY | www.boschsecurity.com

Sitz: Stuttgart, Registergericht: Amtsgericht Stuttgart HRB 23118 
Aufsichtsratsvorsitzender: Stefan Hartung; Geschäftsführung: Gert van Iperen, Andreas Bartz, Thomas Quante, Bernhard Schuster