Message-ID: <aecfdbf2-32bf-1ee5-fe72-0c0923773507@redhat.com>
Date:   Mon, 7 Dec 2020 09:49:03 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Maximilian Luz <luzmaximilian@...il.com>,
        Leon Romanovsky <leon@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     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

Hi,

On 12/6/20 4:58 PM, Maximilian Luz wrote:
> 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.
>>
>> More on that, the whole purpose of proposed interface is to debug and
>> not intended to be used by any user space code.
> 
> I believe this has already been extensively discussed. I want to focus
> more on the part below in this response:
> 
>> Also the idea that you are creating new bus just for this device doesn't
>> really sound right. I recommend you to take a look on auxiliary bus and
>> use it or come with very strong justifications why it is not fit yet.
> 
> I tend to agree that this is a valid concern to bring up, and adding a
> new bus is not something that should be done lightly.
> 
> Let's ignore that this has been merged into -next after I've submitted
> this (and that I only recently became aware of this) for the time being.
> If I would see a clear benefit, I would not hesitate to switch the
> driver and subsystem over to this.
> 
> What does concern me most, is the device/driver matching by string.
> Right now, this subsystem matches those via a device UID. This UID is
> directly tied to the EC functionality provided by the device. A bit of
> background to this:
> 
> Requests sent to the EC contain an address, so to say. This consists of
> 
>  - Target category (TC): Broad group of functionality, e.g. battery/AC,
>    thermal, HID input, ..., i.e. a subsystem of sorts.
> 
>  - Target ID (TID): Some major device, e.g. the dual batteries on the
>    Surface Book 3 are addressed by target ID 1 and 2, some functionality
>    is only available at 2 and some only at 1. May be related to physical
>    parts of/locations on the device.
> 
>  - Instance ID (IID): A device instance, e.g. for thermal sensors each
>    sensor is at TC=0x03 (thermal) and has a different instance ID.
> 
> Those can be used to pretty much uniquely identify a sub-device on the
> EC.

Thank you for this explanation, that is going to be useful to know
when I get around to reviewing this set (although I guess that you
probably also have written this down in one of the commit msgs /
docs I did not check).

> 
> Note the "pretty much". To truly make them unique we can add a function
> ID (FN). With that, we can for example match for TC=0x03, TID=*, IID=*,
> FN=0x00 to load a driver against all thermal sensors. And this is
> basically the device UID that the subsystem uses for matching (modulo
> domain for virtual devices, i.e. device hubs). Sure, we can use some
> string, but that then leads to having to come up with creative names
> once we need some driver specific data, e.g. in the battery driver [1]:
> 
>     const struct auxiliary_device_id my_auxiliary_id_table[] = {
>         { .name = "surface_aggregator_registry.battery", .driver_data = x },
>         { .name = "surface_aggregator_registry.battery_sb3", .driver_data = y },
>         { },
>     }
> 
> Arguably, not _that_ big of a deal.
> 
> What worries me more is that this will block any path of auto-detecting
> devices on a more general/global level. Right now, we hard-code devices
> because we haven't found any way to detect them via some EC query yet
> [2] (FYI the node groups contain all devices that will eventually be
> added to the bus, which are already 11 devices on the Surface Book 3
> without taking missing thermal sensors into account; also they are
> spread across a bunch of subsystems, so not just platform). That's of
> course not an ideal solution and one that I hope we can eventually fix.
> If we can auto-detect devices, it's very likely that we know or can
> easily get to the device UID. A meaningful string is somewhat more
> difficult.
> 
> This registry, which is loaded against a platform device that, from what
> we can tell differentiates the models for some driver bindings by
> Windows (that's speculation), is also the reason why we don't register
> client devices directly under the main module, so instead of a nice
> "surface_aggregator.<devicename>", you'll get
> "surface_aggregator_registry.<devicename>". And it may not end there.
> 
> Something that's currently not implemented is support for thermal
> sensors on 7th generation devices. With thermal sensors, we can already
> detect which sensors, i.e. which IIDs, are present. Naturally, that's
> part of the EC-API for thermal devices (TC=0x03), so would warrant a
> master driver that registers the individual sensor drivers (that's a
> place where I'd argue that in a normal situation, the auxiliary bus
> makes sense). So with the auxiliary bus we'd now end up with devices
> with "surface_thermal.sensor" for the sensors as well as
> "surface_aggregator_registry.<devicename>", both of type ssam_device
> (which then would be a wrapper around auxiliary_device with UID stored
> in that wrapper). Note that they need to be of type ssam_device (or
> another wrapper around that) as they again need the reference to the
> controller device, their UID for access, etc. With a proper bus, device,
> and the UID for matching, we can just add the sensor devices to the bus
> again, as they will have a meaningful and guaranteed unique UID.
> 
> From some reports I've seen it looks like thermal sensors may also be
> available separately on TID=0x01 as well as TID=0x02 on some devices,
> at which point I believe you'd need to introduce some IDA for ID
> allocation to not cause a clash with IDs. At least if you separate the
> base drivers for each TC, which I guess should be preferred due to
> code-reuse. Then again they might use different event registries so you
> may end up needing "surface_thermal.sensor_tc1" and
> "surface_thermal.sensor_tc2" as device names to differentiate those
> for driver loading. Or store the registry in software node properties
> when registering the device.
> 
> I'm repeating myself here, but to me it looks cleaner to have a single
> bus type as opposed to spreading the same base auxiliary device type
> over several namespaces.
> 
> Which then leads me to the question of how a function like
> "is_ssam_device()", i.e. a function testing if the device is of a given
> type, would be implemented without enforcing and testing against some
> part of the device name. Something that, again, doesn't look clean to
> me. Although the use of such a function could probably avoided, but that
> then feels like working around the auxiliary bus.
> 
> Unfortunately, there are a couple more hypotheticals at play than I'd
> like to have (making this not an easy decision), but it's a reverse
> engineered driver so I guess that comes with the territory. All in all,


> I believe it's possible to do this (i.e. use the auxiliary bus), but, to
> me at least, the implementation using a discrete bus feels tidier and
> more true to the hardware (or virtual hardware anyway) behind this. I'm
> happy to hear any arguments against this though.

I agree, the whole setup with the TC + TID + IID feels like the functionality
is nicely (and cleanly) split into separate functions and as with other
busses using a bus + 1 device per function for this is a perfectly clean
way to handle this.

Note if in the future you do see benefit in switching the auxiliary bus
I have no problems with that. But atm I don't really see any benefits of
doing so, so then we would just be switching over for the sake of switching
over which does not seem productive.

Regards,

Hans

Powered by blists - more mailing lists