Re: [PATCH v2 00/19] Dynamically load/remove serdev devices via sysfs*

[Johan Hovold]

On Thu, Jun 14, 2018 at 05:20:40PM +0200, Ricardo Ribalda Delgado wrote:
> Hi Johan,
> On Thu, Jun 14, 2018 at 4:55 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> >
> > On Thu, Jun 14, 2018 at 04:06:18PM +0200, Ricardo Ribalda Delgado wrote:
> > > Hi Johan,
> > > On Thu, Jun 14, 2018 at 3:34 PM Johan Hovold <johan@xxxxxxxxxx> wrote:
> >
> > > > And there are more issues with the series which are less apparent than
> > > > the rx (and partial tx) regression.
> > >
> > > Any hints about this? What else should I change on the series?
> >
> > There are implementation issues and there's the more fundamental
> > question about whether your approach to this is the right one.
> >
> > Like Rob, I'm not sure we want to have the device topology depend on a
> > kernel config symbol (serdev and your ttydev driver). We may need to
> > explore Rob's sibling-device idea further.
> 
> From my point of view, if the user enables serdev, then everything has
> to be a serdev, because serdev does not provide the same functionality
> as a core tty device I had to implement, serdev-ttydev.c. Which is
> nothing more than a wrapper.
> 
> It is very hacky, but allows replacing the core tty device with
> another serdev.

Yes, and I'm a bit surprised (and impressed) that you got it to work
(mostly) so easily.

My point was that we probably don't want the tty devices to move around
in the device hierarchy depending on if serdev (and your ttydev driver)
is enabled or not.

> > I also want to make sure that this can be used for discoverable buses
> > (e.g. the USB CEC device the I've used as an example before).
> >
> 
> I have tried your patch:
> 
> https://github.com/ribalda/linux/commit/5cb30b4ce6477132a23492c674d8b3dc81ecff86
> 
> the only issue is that the serdev device sometimes explotes (OOPS)
> when the usb is unplugged :S.
> 
> And that might be quite tricy to solve

Yes, we all know serdev doesn't support hotplug, that's why it's not
enabled for usbserial.

But when/if we get that sorted, we may want to be able to reuse some of
the matching infrastructure that a sysfs interface would use also for
discoverable buses (e.g. passing a compatible string to serdev core).

Also note that the interface you're proposing suffers from similar
problems as hotplug in that serdev drivers must be prepared to handle
devices going away at anytime; be it through your delete_device
interface or from a sysfs driver unbind (i.e. already an issue today!).

This would be were the oops comes from.

> > As for the current implementation there are both larger and smaller
> > issues, like for example:
> >
> >  - the fact that your sysfs and lookup interface does not use any
> >    locking whatsoever and thus is susceptible to races
> 
> I thought that sysfs access where serialised. If that is not the case
> yes, we need a lock.

It's only serialised per attribute.

> >  - your ttyport driver currently breaks the sysfs interface for all
> >    serial (core) devices by ignoring the attribute groups
> 
> Yep, you are right, I screwed up that one :).

Easy to miss.

> >  - the ttyport driver is arguably a hack with layering issues (which
> >    admittedly may be hard to avoid given the retrofitting of serdev into
> >    the tty layer)
> >
> > Again, I suggest you submit a subset of your series (aim at 10 patches
> > or so) as an RFC which can be used as a basis for further discussion. No
> > point in discussing every implementation detail if the underlying
> > approach needs to be revised.
> 
> Will do. Give me some time to give it a hand of paint.
> 
> Thanks for time reviewing my little moster

You're welcome.

Johan