Re: [RFC PATCH 8/9] surface_aggregator: Add DebugFS interface

[Maximilian Luz]

On 9/24/20 8:46 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 24, 2020 at 12:06:54AM +0200, Maximilian Luz wrote:
> > On 9/23/20 8:29 PM, Greg Kroah-Hartman wrote:
> > > On Wed, Sep 23, 2020 at 08:03:38PM +0200, Maximilian Luz wrote:
> > > > On 9/23/20 6:14 PM, Greg Kroah-Hartman wrote:
> >
> > [...]
> >
> > > > So the -EFAULT returned by put_user should have precedence? I was aiming
> > > > for "in case it fails, return with the first error".
> > >
> > > -EFAULT trumps everything :)
> >
> > Perfect, thanks!
> >
> > > > > Listen, I'm all for doing whatever you want in debugfs, but why are you
> > > > > doing random ioctls here?  Why not just read/write a file to do what you
> > > > > need/want to do here instead?
> > > >
> > > > Two reasons, mostly: First, the IOCTL allows me to execute requests in
> > > > parallel with just one open file descriptor and not having to maintain
> > > > some sort of back-buffer to wait around until the reader gets to reading
> > > > the thing. I've used that for stress-testing the EC communication in the
> > > > past, which had some issues (dropping bytes, invalid CRCs, ...) under
> > > > heavy(-ish) load. Second, I'm considering adding support for events to
> > > > this device in the future by having user-space receive events by reading
> > > > from the device. Events would also be enabled or disabled via an IOCTL.
> > > > That could be implemented in a second device though. Events were also my
> > > > main reason for adding a version to this interface: Discerning between
> > > > one that has event support and one that has not.
> > >
> > > A misc device can also do this, much simpler, right?  Why not use that?
> >
> > Sorry to ask so many questions, just want to make sure I understand you
> > correctly:
> >
> >   - So you suggest I go with a misc device instead of putting this into
> >     debugfs?
>
> Yes.
>
> >   - And I keep the IOCTL?
>
> If you need it, although the interface Arnd says might be much simpler
> (read/write)
>
> >   - Can I still tell people to not use it and that it's not my fault if a
> >     change in the interface breaks their tools if it's not in debugfs?
>
> Yes :)
>
> >   - Also load it via a separate module (module_misc_device, I assume)?
>
> That works.
>
> > One reason why the platform_device approach is practical in this
> > scenario is that I can leverage the driver core to defer probing and
> > thus defer creating the device if the controller isn't there yet.
>
> That's fine, and is a nice abuse of the platform driver interface.  I
> say "abuse" because we really don't have a simpler way to do this at the
> moment, but this really isn't a platform device...

Yeah, it is a bit of a hack...

> > Similarly, the driver is automatically unbound if the controller goes
> > away and the device should be destroyed. All of this should currently be
> > handled via the device link created by ssam_client_bind() (unless I
> > really misunderstood those).
>
> That all is fine, just create the misc device when your driver binds to
> the device, just like you create the debugfs file entries today.
> There's no difference except you get a "real" char device node instead
> of a debugfs file.
>
> > I should be able to handle that by having the device refuse to open the
> > file if the controller isn't there. Holding the state-lock during the
> > request execution should ensure that the controller doesn't get shut
> > down.
>
> Nah, no need for that, again, keep the platform driver/device and then
> create the misc device (and remove it) where you are creating/removing
> the debugfs files.

Okay, I'll do that. Thank you!

> > > A simple misc device would make it very simple and easy to do instead,
> > > why not do that?
> >
> > Again, I considered the probe deferring of the platform driver fairly
> > handy (in addition to having the implicit debugfs warning of "don't rely
> > on this"), but if you prefer me implementing this as misc device, I'll
> > do that.
>
> The "joy" of creating a user api is that no matter how much you tell
> people "do not depend on this", they will, so no matter the file being
> in debugfs, or a misc device, you might be stuck with it for forever,
> sorry.

Hmm, true. I'm fairly confident that the request-IOCTL, as is right now,
should be sound (regarding to 5th and later gen. requests). It also can
be extended in a non-breaking way to handle events by reading from the
device in the future. So might as well commit to that.

Thanks,
Max