Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module

[Leon Romanovsky <leon@kernel.org>]

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?

>
> > 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.

>
> 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.
>
> > 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.
>
> AFAIK the auxiliary bus is for sharing a single device between multiple
> drivers, while the main device-driver also still offers functionality
> (beyond the providing of access) itself.

The idea behind auxiliary bus is to slice various functionalities into
different sub-drivers, see it as a way to create subsystem inside one
driver.

>
> This is more akin to how the WMI driver also models different WMI
> functions as a bus + devices on the bus.
>
> Or how the SDIO driver multiplex a single SDIO device into its
> functions by again using a bus + devices on the bus model.
>
> Also this has been in the works for quite a while now, the Linux on
> Microsoft Surface devices community has been working on this out of
> tree for a long time, see:
> https://github.com/linux-surface/

It is not relevant, the code is merged than it is ready.

>
> And an RFC and a v1 have been posted a while ago, while auxiliary
> bus support is not even in the mainline kernel yet. I would agree
> with you that this should switch to auxiliary bus, despite the timing,
> if that would lead to much better code. But ATM I don't really see
> switching to auxiliary bus offering much benefits here.

The auxiliary bus is merged and part of linux-next.
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tag/?h=auxbus-5.11-rc1

>
> > I'm sorry to say, but this series is not ready to be merged yet.
> >
> > NAK: Leon Romanovsky <leon@kernel.org>
>
> See above, I believe that this all is a bit harsh and I have not
> really heard convincing arguments for not merging this.
>
> Moreover such a quick nack does not really promote working upstream,
> where as we actually want people to work upstream as much as possible.
> I know this is not a reason for taking bad code, but I'm not
> convinced that this is bad code.

I naked explicitly for two reasons: UAPI(chardev) and lack of rationale
for the custom bus, never said "bad code".

>
> I have not reviewed this myself yet, but once I have reviewed
> this and any review remarks have been addressed I do expect to
> merge this series through the platform-drivers-x86 tree.
>
> Regards,
>
> Hans de Goede
> (drivers/platform/x86 and drivers/platform/surface subsys maintainer)
>