Re: [PATCH 0/9] Serial slave device bus

["H. Nikolaus Schaller" <hns@goldelico.com>]

Hi Rob,

> Am 13.01.2017 um 15:48 schrieb Rob Herring <robh@kernel.org>:
> 
> On Fri, Jan 13, 2017 at 5:22 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> Hi Rob,
>> was it intentional to answer privately only?
> 
> Damn gmail. Added everyone back.

No problem. Happens to everyone every now and then.

> 
>>> Am 12.01.2017 um 23:07 schrieb Rob Herring <robh@kernel.org>:
>>> 
>>> On Tue, Jan 10, 2017 at 5:44 AM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>> Hi Rob,
>>>> 
>>>>> Am 06.01.2017 um 17:26 schrieb Rob Herring <robh@kernel.org>:
>>>>> 
> 
> [...]
> 
>>>> 2. When I try to open the tty from user space to fetch the serial data I
>>>> just get
>>>> 
>>>> root@letux:~# cat /dev/ttyO1
>>>> [  659.290618] ttyO ttyO1: tty_open: tty->count(2) != #fd's(1)
>>>> [  665.257232] ttyO ttyO1: tty_release: tty->count(2) != #fd's(1)
>>>> ^C
>>>> root@letux:~#
>>>> 
>>>> So it does not seem to be possible to read the data from the tty any more.
>>>> 
>>>> Maybe there can be some function serdev_device_set_shared(dev, flag).
>>>> If set to exclusive the /dev node would be hidden from user-space.
>>> 
>>> I don't think sharing should be allowed. Either you have an in-kernel
>>> driver or you handle it in userspace. Sharing is just asking for
>>> trouble IMO.
>> 
>> My tty-slave patch series works and has no trouble with sharing (the UART)
>> because it was designed with this in mind.
>> 
>> The only trouble is that it did not find maintainer's acceptance...
>> 
>>> Though it could be supported later.
>> 
>> Firstly, let me point out once again that we have a mobile device, battery
>> powered and every component should be and stay turned off, if not needed by
>> any consumer.
> 
> That's every device...
> 
>> The only component that can reliably detect if there is no consumer in user-space
>> is the kernel. Hence it must be a kernel driver that powers the device on/off.
>> 
>> On the other side, it should simply pass data unmodified to user space (because the
>> chip provides cooked data), so we do not need a driver for doing any data processing.
>> The data stream is provided perfectly (without proper power control) by simply
>> accessing /dev/ttyO1 from user-space.
>> 
>> So if there were no power control topic, we would not even ask for a driver.
> 
> I think this reasoning is exactly why we have no proper subsystem
> today. We *almost* don't need one. It all works fine except I have
> this one GPIO to control. Oh, and a regulator and firmware and
> suspend/resume control and ...

In our case we have no firmware, just the gpio.

> 
>> We only need the driver to detect the power state of the chip by *monitoring*
>> the data stream that goes to user space.
>> 
>> Of course shared writing to the chip would give trouble, but we do not need it.
>> Therefore I am happy if this sharing is an option (with a big WARNING sign).
>> 
>> If we would try to achieve the same power management in user-space we would have
>> to run a power-consuming daemon and make sure that it *never* crashes (or people
>> come and complain about short battery life even if they think they have GPS
>> turned off).
>> 
>> And we need to be able to maintain the daemon for many different distributions
>> people want to run on our device. This takes years to get it into Debian and
>> others where we are not developers at all.
> 
> Exactly one of the problems we're trying to solve. With your desired
> approach though, you are still leaving the problem of having to know
> which tty device the GPS chip is connected to which varies with each
> board.
> Either you hardcode the tty device in userspace, provide some
> sysfs file with the tty name (like TI-ST) or link, provide the tty
> name in DT (which I'll NAK) or have userspace parse the DT to find the
> connection. I've seen all but the last case. I want to solve this
> problem, too, such that userspace just opens the BT, WiFi, GPS, etc.
> device.

Yes, this is indeed an issue. There are also USB or Bluetooth based external
GPS devices which do not need a driver because they speak some standard
profile, identify themselves as GPS and create some tty port through standard
mechanisms.

> 
>> So this data stream sharing/monitoring is the most important part for getting our
>> chip supported by a kernel driver.
>> 
>>> 
>>> I've updated the series to skip creating the /dev nodes.
>> 
>> That is exactly what we do NOT need for this chip. Now I can't even access it
>> any more when powered on...
> 
> It's a minor change. Essentially, it is call tty_register_device_attr
> or not.

Fine!

> There's the issue of the file open count warning which I don't
> know how to solve.

Unfortunately I am not experienced with the tty layer to help here.

> 
>>>> 3. for completely implementing my w2sg0004 driver (and two others) it would
>>>> be nice to have additional serdev_device_ops:
>>>> 
>>>> a) to be notified about user-space clients doing open/close on the tty
>>>> b) and/or to be notified about user-space tcsetattr or TIOCMSET (for DTR)
>>>> 
>>>> There may be other means (ldisc?) to get these notifications, but that
>>>> needs the serdev driver to register with two different subsystems.
>>>> 
>>>> Another approach could be to completely rewrite the driver so that it wraps
>>>> and hides the /dev/ttyO1 and registers its own /dev/gps tty port for user-space
>>>> communication. Then it would be notified for all user-space and serial
>>>> interface activities as a man-in-the-middle.
>>> 
>>> This was my thinking. If we only need data read and write, I don't
>>> think we gain much using a tty vs. a new char dev.
>> 
>> We gain re-use of the existing tty for its key purpose to mediate between an uart
>> device and user-space.
>> 
>> I have looked into the chardev approach and it appears to be quite some overkill
>> to copy serial data from the UART to a user space file handle.
>> 
>> About buffering: with the chardev approach we have to implement our own buffer
>> in the driver and decide what happens on overflow while the tty layer already
>> efficiently handles this.
> 
> Not exactly. There's already buffering in tty_buffer.c. That handles
> overflow of the UART. Then there is a 4K circular buffer in n_tty. The
> question really is how much of the per character and flag processing
> of n_tty do you need as that is where the complexity is. I'd guess not
> much of it. If it is needed, then perhaps n_tty.c could be refactored
> to provide common functions.

Ah, that is good if the serdev driver can rely on / reuse this buffer.

Where I don't see immediately is how I can make the chardev read file operation
block until I get some receive_buf serdev_device_op and how I manage different
data size requests without introducing another driver-internal buffer.

So it could be better to alloc_tty_driver another tty and copy to its
read buffer. 

> 
>> And it is a little strange that the device can either be accessed through
>> /dev/ttyO1 if the driver is not loaded and only through /dev/gps if it is.
> 
> We have similar things with SPI, I2C, and USB. Either you have a
> kernel driver or you have a userspace driver. The userspace drivers
> have limitations and if those limitations are a problem, we right
> kernel drivers instead.
> 
>> New problems arise if there were two such chips. Then we must be prepared
>> that several instances provide different /dev/gps[1-9] nodes and that they
>> can be identified and are stable. Maybe we have to introduce udev-rules...
> 
> That's a solved problem generally (though not all like the answer).

Well, I have no problem doing it that way but it increases complexity of the
driver and makes it more difficult to configure in user space.

> 
>> So what seems to be an obvious and straightforward solution (from serdev perspective)
>> is not, if we look into implementation details of the driver.
> 
> I can't solve all problems for all possible drivers on day one. It
> does provide a solution for drivers that are already in the kernel.

Well, I hope you understand that I am a little impatient here...

It is more than 2 years ago that we proposed the first driver for this chip
for upstreaming (while we have something running on our own kernels much longer
time):

	http://lkml.iu.edu/hypermail/linux/kernel/1410.2/00634.html

And if I counted correctly, I am now working on the fourth completely different
proposal with no finalization in sight.

So this situation is a little circular: because we are not in kernel we have to
wait longer until we can get in...

> I'd suggest we debate adding sharing capability vs. a GPS subsystem
> separately.

Well, I would be perfectly happy without having to wait for a fully worked out
GPS subsystem. If it arrives in my (device's) life-time we can rework the driver
to fit into it.

This approach seems not to be an exception to me. For example the original misc/bmp085
driver has completely gone in 4.9 and been replaced by a generic iio driver.

> If there was already a GPS subsystem there would be no
> debate. I don't think what's here is preventing either case.
> Similarly, I don't pretend the configuration API (baud rate and
> flow-control) is complete. I'm sure someone will need additional
> functions, but those can all be incrementally added as needed.
> 
>>>> But I expect that it delays the communication and is quite some overhead.
>>>> 
>>>> 
>>>> 4. It seems as if I have to modprobe the driver explicitly (it is not
>>>> located and loaded automatically based on the compatible string in DT
>>>> like i2c clients).
>>> 
>>> I've added what I think should be needed for that. I pushed out a new
>>> branch[1]. Can you give it a try?
>> 
>> Yes, sure. I have tried and now our driver module is loaded as expected :)
>> 
>> But in general we are turning away from a solution for our w2sg0004 driver
>> (see above).
>> 
>> BTW: I see an issue in our kernel (config?) that the console and initd
>> blocks for approx. 60 seconds during boot when serdev is merged (even
>> if not configured). This issue disappears when using the omap2plus_defconfig.
>> 
>> But I have not digged into that (because it may be spurious or EPROBE_DEFER
>> related).
> 
> I've not seen anything like that. Do you have a diff of your configs?
> And what is your init system?

I did use Debian with sysv init and a getty on /dev/ttyO2 (OMAP3 console UART).

But let me cross-check some things before digging deeper into it. To exclude
that it is our fault and indeed in the serdev patch set.

BR and thanks,
Nikolaus