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

[Johan Hovold]

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.

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).

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

 - your ttyport driver currently breaks the sysfs interface for all
   serial (core) devices by ignoring the attribute groups

 - 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.

> > It's legacy as in old, and to be used for one-off hacks and such. But
> > sure, that is also what this series aims at even if that doesn't mean
> > you *have to* copy the interface.
> 
> It is not only one-off hack. It is the ONLY way to use i2c devices
> that are not enumerated.
> 
> The same way as today we do not have any way of using serdev on non
> enumerated devices.

You're missing the point: none of that means you have to copy the
interface.

Johan