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

From: Maximilian Luz <luzmaximilian@gmail.com>
To: Leon Romanovsky <leon@kernel.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org,
	"Mark Gross" <mgross@linux.intel.com>,
	"Andy Shevchenko" <andy.shevchenko@gmail.com>,
	"Barnabás Pőcze" <pobrn@protonmail.com>,
	"Arnd Bergmann" <arnd@arndb.de>, "Rob Herring" <robh@kernel.org>,
	"Jiri Slaby" <jirislaby@kernel.org>,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	"Len Brown" <lenb@kernel.org>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Masahiro Yamada" <masahiroy@kernel.org>,
	"Michal Marek" <michal.lkml@markovi.net>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Blaž Hrastnik" <blaz@mxxn.io>,
	"Dorian Stoll" <dorian.stoll@tmsp.io>,
	platform-driver-x86@vger.kernel.org,
	linux-serial@vger.kernel.org, linux-acpi@vger.kernel.org,
	linux-kbuild@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH v2 0/9] Add support for Microsoft Surface System Aggregator Module
Date: Sun, 6 Dec 2020 14:43:53 +0100	[thread overview]
Message-ID: <040a5b5f-f268-79ea-c32c-95f229a34b4a@gmail.com> (raw)
In-Reply-To: <20201206114153.GD693271@unreal>

On 12/6/20 12:41 PM, Leon Romanovsky wrote:
> On Sun, Dec 06, 2020 at 11:41:46AM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 12/6/20 11:33 AM, Leon Romanovsky wrote:
>>> On Sun, Dec 06, 2020 at 11:04:06AM +0100, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> 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).
>>>
>>> I saw, the author misunderstood the Greg's comments.
>>
>> Quoting from patch 8/9:
>>
>> "
>> +==============================
>> +User-Space EC Interface (cdev)
>> +==============================
>> +
>> +The ``surface_aggregator_cdev`` module provides a misc-device for the SSAM
>> +controller to allow for a (more or less) direct connection from user-space to
>> +the SAM EC. It is intended to be used for development and debugging, and
>> +therefore should not be used or relied upon in any other way. Note that this
>> +module is not loaded automatically, but instead must be loaded manually.
>> "
>>
>> If I'm not mistaken that seems to be pretty much what Greg asked for.
> 
> Right, unless you forget the end of his request.
>   "
>    The "joy" of creating a user api is that no matter how much you tell
>    people "do not depend on this", they will, so no matter the file being
>    in debugfs, or a misc device, you might be stuck with it for forever,
>    sorry.
>   "

Which to me reads as "if you want a user-space interface for development and
debugging, you'll have to make it a stable interface, regardless where it is
implemented". Rather making a point for a well though-out stable interface.
Specifically with regards to

    "
     >  - So you suggest I go with a misc device instead of putting this into
     >    debugfs?

     Yes.
    "

Unless of course I'm misunderstanding things entirely. Greg, please feel free
to yell at me if I've got this wrong.

> So I still think that exposing user api for a development and debug of device
> that has no future is wrong thing to do.

Unless you know something that I don't, MS is rumored to come out with a new
Surface Pro 8 and Surface Laptop 4 early next year, which I fully expect to
also have this EC built in. And if they once again decided to move some
functionality normally provided via ACPI or other means to the EC for some
reason, we'll likely need that interface again.

Yes, it has no future outside of Surface devices, but so has every other
platform driver with respect to their specific platform. What are your
alternatives to exposing a user API? If we want to be able to easily test
and attempt to provide support for Surface devices, there has to be some
interaction with user-space.

With respect to stability of the interface and future changes, I believe
that IOCTLs are the way to go. If that's in debugfs or, as was the
result from the previous discussion about this, via a misc-device...
I'll be happy to implement whatever a consensus yields, as long as it
can be used for its intended purpose: aid development with regards to
the EC found on Surface devices.

Regards,
Max