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

[Maximilian Luz <luzmaximilian@gmail.com>]

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