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 Regards, Hans
