Date:   Sun, 6 Dec 2020 11:51:58 +0100
From:   Maximilian Luz <luzmaximilian@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Leon Romanovsky <leon@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, Mark Gross <mgross@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Barnabás Pőcze <pobrn@...tonmail.com>,
        Arnd Bergmann <arnd@...db.de>, Rob Herring <robh@...nel.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Len Brown <lenb@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Jonathan Corbet <corbet@....net>,
        Blaž Hrastnik <blaz@...n.io>,
        Dorian Stoll <dorian.stoll@...p.io>,
        platform-driver-x86@...r.kernel.org, linux-serial@...r.kernel.org,
        linux-acpi@...r.kernel.org, linux-kbuild@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 0/9] Add support for Microsoft Surface System
 Aggregator Module

On 12/6/20 11:04 AM, Hans de Goede wrote:
> Hi,
> 
> On 12/6/20 9:56 AM, Leon Romanovsky wrote:
>> On Sun, Dec 06, 2020 at 09:41:21AM +0100, Hans de Goede wrote:
>>> Hi Leon,
>>>
>>> On 12/6/20 8:07 AM, Leon Romanovsky wrote:
>>>> On Thu, Dec 03, 2020 at 10:26:31PM +0100, Maximilian Luz wrote:
>>>>> Hello,
>>>>>
>>>>> Here is version two of the Surface System Aggregator Module (SAM/SSAM)
>>>>> driver series, adding initial support for the embedded controller on 5th
>>>>> and later generation Microsoft Surface devices. Initial support includes
>>>>> the ACPI interface to the controller, via which battery and thermal
>>>>> information is provided on some of these devices.
>>>>>
>>>>> The previous version and cover letter detailing what this series is
>>>>> about can be found at
>>>>>
>>>>>    https://lore.kernel.org/platform-driver-x86/20201115192143.21571-1-luzmaximilian@gmail.com/
>>>>>
>>>>> This patch-set can also be found at the following repository and
>>>>> reference, if you prefer to look at a kernel tree instead of these
>>>>> emails:
>>>>>
>>>>>    https://github.com/linux-surface/kernel tags/s/surface-aggregator/v2
>>>>>
>>>>> Thank you all for the feedback to v1, I hope I have addressed all
>>>>> comments.
>>>>
>>>>
>>>> I think that it is too far fetched to attempt and expose UAPI headers
>>>> for some obscure char device that we are all know won't be around in
>>>> a couple of years from now due to the nature of how this embedded world
>>>> works.
>>>
>>> This is not for an embedded device, but for the popular line of
>>> Microsoft Surface laptops / 2-in-1s...
>>
>> It is the naming, we don't have char device for every "laptop" vendor.
>> Why is Microsoft different here?
> 
> Because their hardware department has invented a whole new way of dealing
> with a bunch of things at the hardware level (for some reason).
> 
> Also almost all laptop vendors have a whole bunch of laptop vendor
> specific userspace API in the form of sysfs files exported by
> drivers/platform/x86/laptop-vendor.c drivers. E.g. do:
> 
> ls /sys/bus/platform/devices/thinkpad_acpi/
> 
> An any IBM/Lenovo Thinkpad (and only on a Thinkpad) to see a bunch
> of laptop vendor specific UAPI.
> 
> Since I've become the pdx86 subsys maintainer I've actually been
> pushing back against adding more of this, instead trying to
> either use existing UAPIs, or defining new common UAPIs which can
> be shared between vendors.
> 
> So I agree very much with you that we need to be careful about
> needlessly introducing new UAPI.
> 
> But there is a difference between being careful and just nacking
> it because no new UAPI may be added at all (also see GKH's response).
> 
>>>> 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.
>>
>> USB devices implement standard protocol, this surface hub is nothing
>> even close to that.
> 
> The USB protocol just defines a transport layer, outside of the USB classes
> there are plenty of proprietary protocols on top of that transport.
> 
> And this chardev just offers access to the Surface Serial Hub transport
> protocol. And if you want something even closer the i2cdev module offers
> raw I2C transfer access and I2C defines no protocol other then
> how to read or write a number of bytes.
> 
> I do a lot of hw enablement work and being able to poke HID / USB / I2C
> devices directly from userspace is very useful for this.

This is pretty much the whole reason for this module. Surface devices
vary from generation to generation, and I can't expect some random user
to write some kernel module (or repeatedly pull 10 different versions
of it)to test if some EC command does x or y. I can tell them to run a
script with some arguments though. The main reason for this is
development, debugging is secondary and IMHO part of development.

So, IOW this interface provides direct access to the EC that would,
without it, require you to write a kernel driver.

The whole "this is intended for development and debugging" shtick is to
deter people from using it to implement user-space based drivers. While
this may be possible at some point, at the moment there is no good way
to (reliably) detect which client devices are present from user-space.
So any attempts at that will likely be unstable and should be
implemented in the kernel anyway. It is a way of prototyping drivers
though.

>>> 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.
>>
>> I don't know how it goes in device/platform area, but in other large
>> subsystems, UAPI should be presented with working user-space part.
>>
>>>
>>> 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.
> 
> So this ^^^ still stands, I agree with you that adding new UAPI needs
> to be considered carefully and when I get around to reviewing this
> that is exactly what I will do.
> 
> Maximilian, can you perhaps explain a bit more of what you want / expect
> to use the chardev for, and maybe provide pointers to the matching
> userspace utilities (which I presume you have) ?

Sure. There's a bunch of scripts at

   https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam

As described above, the main idea behind this simplifying development
for devices that I can't test myself. See e.g. the "ctrl.py" script
which can be used to send a basic command to the EC. The "hid.py" script
is one that was successfully used to test commands for the Keyboard
driver on the Surface Laptop 1 and 2.

At some point my plan is to maybe split that out into its own repo and
improve usability for all that, but I haven't gotten to that yet.

[...]

Regards,
Max

Powered by blists - more mailing lists