On 12/6/20 11:43 AM, Hans de Goede wrote:
Hi,
On 12/6/20 11:33 AM, Maximilian Luz wrote:
On 12/6/20 10:06 AM, Leon Romanovsky wrote:> On Sun, Dec 06, 2020 at 05:58:32PM +0900, Blaž Hrastnik wrote:
More on that, the whole purpose of proposed interface is to debug and
not intended to be used by any user space code.
The purpose is to provide raw access to the Surface Serial Hub protocol,
just like we provide raw access to USB devices and have hidraw devices.
So this goes a litle beyond just debugging; and eventually the choice
may be made to implement some functionality with userspace drivers,
just like we do for some HID and USB devices.
Still I agree with you that adding new userspace API is something which
needs to be considered carefully. So I will look at this closely when
reviewing this set.
To add to that: this was previously a debugfs interface but was moved to misc after review on the initial RFC:
https://lkml.org/lkml/2020/9/24/96
There is a huge difference between the suggestion and final implementation.
Greg suggested to add new debug module to the drivers/misc that will
open char device explicitly after user loaded that module to debug this
hub. However, the author added full blown char device as a first citizen
that has all not-break-user constrains.
This module still needs to be loaded explicitly.
Good then I really do not see a problem with this.
And (I might be wrong
about this) the "not-break-user constraints" hold as soon as I register
a misc device at all, no?
Correct.
So I don't see how this is a) any different
than previously discussed with Greg and b) how the uapi header now
introduces any not-break-user constraints that would not be there
without it.
This interface is intended as a stable interface. That's something that
I committed to as soon as I decided to implement this via a misc-device.
Sure, I can move the definitions in the uapi header to the module
itself, but I don't see any benefit in that.
Right, if we are going to use a misc chardev for this, then the
correct thing to do is to put the API bits for that chardev under
include/uapi.
It would still be good if you can provide a pointer to some userspace
tools using this new API; and for the next version maybe add that
pointer to the commit message
Right, I will add that to the commit message. I just linked you the
scripts in my other response, but here again for completeness:
https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam
While I'm not using the header directly (the scripts are written in
python) I still think uapi is the right place to put this (please
correct me if I'm wrong). Not putting them there seems to be needless
obfuscating to me.
Regards,
Max
