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

From: Leon Romanovsky <leon@kernel.org>
To: Maximilian Luz <luzmaximilian@gmail.com>
Cc: "Blaž Hrastnik" <blaz@mxxn.io>,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	lkml <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>,
	"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 13:30:47 +0200	[thread overview]
Message-ID: <20201206113047.GC693271@unreal> (raw)
In-Reply-To: <a1db4b23-8159-924a-a99b-2fa4b0eb6fff@gmail.com>

On Sun, Dec 06, 2020 at 11:33:40AM +0100, 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. And (I might be wrong
> about this) the "not-break-user constraints" hold as soon as I register
> a misc device at all, no?

I don't think so, files in drivers/misc/* don't have such strict policy.

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

There is a huge difference between char device for the debug and
exposed UAPI header. The first requires from the user to build and
explicitly run it, while header allows to reliably build on top of
it various applications that we don't control. The not-break-rule
talks about the second.

>
> 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. If someone really wants to
> use this interface, they can just as well copy the definitions from the
> module source itself. So why not be upfront about it and make life
> easier for everyone?

Because you are actually making life harder for everyone who cares about
UAPIs exposed by the Linux and they definitely different in numbers from
those who needs debug interface for the Microsoft Surface board.

Thanks

>
> Regards,
> Max
>