Re: [PATCH 0/9] XSHM: Shared Memory Driver for ST-E Thor M7400 LTE modem

[Arnd Bergmann <arnd@arndb.de>]

Hi Sjur,

I've tried to do a review of your code, but I really need to understand
more of the background first, to be able to tell if it's using the
right high-level approach. There are a lot of details I could
comment on, but let's first look at the overall design.

On Wednesday 07 December 2011, Sjur Brændeland wrote:
> Introduction:
> ~~~~~~~~~~~~~
> This patch-set introduces the Shared Memory Driver for ST-Ericsson's
> Thor M7400 LTE modem.
> The shared memory model is implemented for Chip-to-chip and
> uses a reserved memory area and a number of bi-directional
> channels. Each channel has it's own designated data area where payload
> data is copied into.

Can you explain what the scope of this framework is? What I don't
understand from the code is whether this is meant to be very
broadly applicable to a lot of devices and competing with e.g.
rpmsg from Ohad Ben-Cohen, or whether this is really speciific
to one hardware implementation for caif.

Also, to what degree is the protocol design flexible? Is the modem
side fixed, so you have to live with any shortcomings of the interface,
or are you at this point able to improve both sides when something
is found to be lacking?

> Two different channel types are defined, one stream channel which is
> implemented as a traditional ring-buffer, and a packet channel which
> is a ring-buffer of fix sized buffers where each buffer contains
> an array of CAIF frames.
> 
> The notification of read and write index updates are handled in a
> separate driver called c2c_genio. This driver will be contributed separately,
> but the API is included in this patch-set.
> 
> The channel configuration is stored in shared memory, each channel
> has a designated area in shared memory for configuration data,
> read and write indexes and a data area.

How many channels will there be on of each type in a typical system,
and respectively in the maximum you can realistically expect over
the next 10 years?

> Stream data
> ~~~~~~~~~~~
> The driver for the stream channel is implemented as a character device
> interface to user space. The character device implements non-blocking open
> and non-blocking IO in general. The character device is implementing
> a traditional circular buffer directly in the shared memory region for
> the channel.

It seems unusual to have both a socket interface and a character device
interface. What is the motivation behind this? Why can't you use the
socket for non-blocking I/O?

> Driver model
> ~~~~~~~~~~~~~~
> Current implementation is using the platform bus for XSHM devices.
> The packet channels are named "xshmp", and stream channel "xshms".
>
> /sys/devices:
> |-- platform
> |   |-- uevent
> |   `-- xshm
> |       |-- bootimg
> |       |-- caif_ready
> |       |-- ipc_ready
> |       |-- subsystem -> ../../../bus/platform
> |       |-- xshmp.1
> |       |   |-- driver -> ../../../../bus/platform/drivers/xshmp
> |       |   |-- subsystem -> ../../../../bus/platform
> |       `-- xshms.0
> |           |-- driver -> ../../../../bus/platform/drivers/xshms
> |           |-- misc
> |           |   `-- xshm0
> |           |       |-- device -> ../../../xshms.0
> |           |       |-- subsystem -> ../../../../../../class/misc
> `-- virtual
>     |-- net
>     |   |-- cfshm0

Why are the channels /platform/ devices? With all the infrastructure
you have in the xshm driver, I would think that you should be
able to probe the available channels yourself instead of relying the
board to define them. If you cannot, that is something that I think
needs to be fixed in the interface to the modem. If you can, you
probably should not be using the platform_bus but instead create
your own bus_type.

	Arnd