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

[Greg Kroah-Hartman]

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

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

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

thanks,

greg k-h