| Message ID | 20201203212640.663931-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for Microsoft Surface System Aggregator Module | expand |
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. 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'm sorry to say, but this series is not ready to be merged yet. NAK: Leon Romanovsky <leon@kernel.org> Thanks
On Sun, Dec 06, 2020 at 09:07:05AM +0200, 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. No, that's not ok, we do this for loads of devices out there. If there is a device that wants to be supported for Linux, and a developer that wants to support it, we will take it. > More on that, the whole purpose of proposed interface is to debug and > not intended to be used by any user space code. I thought that debugfs was going to be used for most of the debugging code, or has that changed in newer versions of this patchset? thanks, greg k-h
On Sun, Dec 06, 2020 at 09:32:30AM +0100, Greg Kroah-Hartman wrote: > On Sun, Dec 06, 2020 at 09:07:05AM +0200, 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. > > No, that's not ok, we do this for loads of devices out there. If there > is a device that wants to be supported for Linux, and a developer that > wants to support it, we will take it. > > > More on that, the whole purpose of proposed interface is to debug and > > not intended to be used by any user space code. > > I thought that debugfs was going to be used for most of the debugging > code, or has that changed in newer versions of this patchset? https://lore.kernel.org/lkml/20201203212640.663931-9-luzmaximilian@gmail.com/#Z30include:uapi:linux:surface_aggregator:cdev.h + * Definitions, structs, and IOCTLs for the /dev/surface/aggregator misc + * device. This device provides direct user-space access to the SSAM EC. + * Intended for debugging and development. > > thanks, > > greg k-h
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... > 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. > 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. 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/ 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. > 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 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)
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) >
> > > 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
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. Thanks
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. >> 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) ? >>> 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. AFAIK the idea is to be able to combine multiple physical devices, e.g. a PCI device + an ACPI enumerated platform device and then slice the combination of those 2 up in new devices which may use parts of both parent devices, quoting from: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tree/Documentation/driver-api/auxiliary_bus.rst?h=driver-core-next&id=7de3697e9cbd4bd3d62bafa249d57990e1b8f294 "multiple devices might implement a common intersection of functionality" IOW this is for cases where the simpler bus + devices model does not work well. AFAICT in this case the simpler bus + devices does work well, so there is no need to use the auxiliary bus. >> 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. It is relevant, you cannot expect drivers which were written during the last 6 months to use functionality which is not even in the mainline kernel yet (yes it is in -next, but not in mainline). Now if that new functionality where to provide major benefits to the code making it much cleaner / better then yes asking to rewrite it to use that new functionality would make sense. I need to take a closer look at the code but ATM I'm not convinced that rewriting it to use the new auxiliary bus stuff will make it better at all, let alone enough to warrant the effort of the rewrite. <snip> Regards, Hans p.s. For the record I do not own any Microsoft Surface devices myself, so I have no interests here other then to make sure that the Linux kernel is welcoming to new new contributors and does not needlessly scare them away.
On Sun, Dec 06, 2020 at 11:04:06AM +0100, 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). They are not different from any other vendor, it is much cheaper and easier to do not follow standard implementations. > > 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/ It is different from the proposed /dev/surface... char device. > > An any IBM/Lenovo Thinkpad (and only on a Thinkpad) to see a bunch > of laptop vendor specific UAPI. Yes, it is gross, IBM did it in early days of Linux. Other vendors don't have anything like that. > > 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). I saw, the author misunderstood the Greg's comments. > > >>> 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. Greg wrote how to do it. > > >> 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) ? > > >>> 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. > > AFAIK the idea is to be able to combine multiple physical devices, e.g. > a PCI device + an ACPI enumerated platform device and then slice the > combination of those 2 up in new devices which may use parts of both > parent devices, quoting from: > > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/tree/Documentation/driver-api/auxiliary_bus.rst?h=driver-core-next&id=7de3697e9cbd4bd3d62bafa249d57990e1b8f294 > > "multiple devices might implement a common intersection of functionality" It is one way, another is to take one device and create many small devices out of it. https://lore.kernel.org/alsa-devel/20201026111849.1035786-1-leon@kernel.org/ > > IOW this is for cases where the simpler bus + devices model does not > work well. AFAICT in this case the simpler bus + devices does work > well, so there is no need to use the auxiliary bus. It is designed to replace invention of custom buses. > > >> 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. > > It is relevant, you cannot expect drivers which were written during > the last 6 months to use functionality which is not even in the > mainline kernel yet (yes it is in -next, but not in mainline). > > Now if that new functionality where to provide major benefits to > the code making it much cleaner / better then yes asking to rewrite > it to use that new functionality would make sense. And who will rewrite it? My experience shows that right code should be written from the beginning otherwise the code has too many chances to be abandoned. Thanks
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? So I don't see how this is a) any different 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. 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? Regards, Max
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. Regards, Hans
Hi, On 12/6/20 11:33 AM, 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. Good then I really do not see a problem with this. > And (I might be wrong > about this) the "not-break-user constraints" hold as soon as I register > a misc device at all, no? Correct. > So I don't see how this is a) any different > 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. > > 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. Right, if we are going to use a misc chardev for this, then the correct thing to do is to put the API bits for that chardev under include/uapi. It would still be good if you can provide a pointer to some userspace tools using this new API; and for the next version maybe add that pointer to the commit message Regards, Hans
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
On 12/6/20 11:43 AM, Hans de Goede wrote: > Hi, > > On 12/6/20 11:33 AM, 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. > > Good then I really do not see a problem with this. > >> And (I might be wrong >> about this) the "not-break-user constraints" hold as soon as I register >> a misc device at all, no? > > Correct. > >> So I don't see how this is a) any different >> 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. >> >> 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. > > Right, if we are going to use a misc chardev for this, then the > correct thing to do is to put the API bits for that chardev under > include/uapi. > > It would still be good if you can provide a pointer to some userspace > tools using this new API; and for the next version maybe add that > pointer to the commit message Right, I will add that to the commit message. I just linked you the scripts in my other response, but here again for completeness: https://github.com/linux-surface/surface-aggregator-module/tree/master/scripts/ssam While I'm not using the header directly (the scripts are written in python) I still think uapi is the right place to put this (please correct me if I'm wrong). Not putting them there seems to be needless obfuscating to me. Regards, Max
On 12/6/20 9:32 AM, Greg Kroah-Hartman wrote: > On Sun, Dec 06, 2020 at 09:07:05AM +0200, 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. > > No, that's not ok, we do this for loads of devices out there. If there > is a device that wants to be supported for Linux, and a developer that > wants to support it, we will take it. > >> More on that, the whole purpose of proposed interface is to debug and >> not intended to be used by any user space code. > > I thought that debugfs was going to be used for most of the debugging > code, or has that changed in newer versions of this patchset? As per previous discussion (https://lkml.org/lkml/2020/9/24/96) I have replaced the debugfs device by a misc-device with stable interface. I also believe that this is probably the better option long-term. The general idea is to have a device that has direct access to the EC/transport protocol and can be used for development and prototyping. Debugging is a part of that. So it's more akin to something raw access via i2cdev, hidraw, or raw access to USB devices as Hans de Goede mentioned in one of his mails. Note that the module must still be loaded manually Regards, Max
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 >
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. " So I still think that exposing user api for a development and debug of device that has no future is wrong thing to do. Thanks > > Regards, > > Hans >
On 12/6/20 12:30 PM, Leon Romanovsky wrote: > 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. Can I get a link to the documentation stating that or someone else confirming that? Also I don't think it makes sense to have a platform/surface device in drivers/misc, after we've explicitly decided to move this code out of there. IIRC drivers/misc is not a place for misc-devices, but the directory for devices that don't have any good place elsewhere. >> 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. So it's okay to break stuff that's not explicitly in include/uapi/? Again, can I get someone to confirm that for me? As already said, I'm okay with moving the definitions from the header to the module itself (if there is a consensus on that, CC Greg, Hans), however both allow you to build user-space tools against the API. Case in point my python scripts, which don't use the header. Or any other non-C-based tool. So unless there's a rule that anything without a header in uapi is fair game, I fail to see your point. >> 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. This point again depends on the interface not being stable. Unless, of course, you want me to remove the interface completely and just maintain it out of tree... I'm happy to be told otherwise by the authorities here, but from past conversations it seems that basically everything providing some sort of user-space access falls under the "don't break user-space" rule as soon as somebody uses it. Regards, Max
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
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. 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. Regards, Max [1]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_battery.c#L1075-L1079 [2]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_aggregator_registry.c
On Sun, Dec 06, 2020 at 04:58:52PM +0100, 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. > > 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'm not so certain why you are so focused on the names and UIDs. The names are simply the aliases and needed to match between major device driver that is responsible for the discovery and triggering sub-driver with implementation. Nothing prohibits from the logic: "if uid = 123; trigger "sensor"" Different devices and separation between them (*_registry module) are expected to be in that main driver. As an outcome of it, you will get: proper module autoload, device discover and power management. Thanks > > Regards, > Max > > [1]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_battery.c#L1075-L1079 > [2]: https://github.com/linux-surface/surface-aggregator-module/blob/61b9bb859c30a8e17654c3a06696feb2691438f7/module/src/clients/surface_aggregator_registry.c
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
On Mon, Dec 07, 2020 at 09:49:03AM +0100, Hans de Goede wrote: > 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. I too do not see the benefit at this time to switch either. thanks, greg k-h
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. Regards, Max Note: This patch depends on [PATCH v4] platform/surface: Create a platform subdirectory for Microsoft Surface devices which can be found at https://lore.kernel.org/platform-driver-x86/20201009141128.683254-1-luzmaximilian@gmail.com/ and is currently in platform-drivers-x86/for-next. Changes in v1 (from RFC, overview): - move to platform/surface - add copyright lines - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later) - change user-space interface from debugfs to misc-device - address issues in user-space interface - fix typos in commit messages and documentation - fix some bugs, address other issues Changes in v2 (overview): - simplify some code, mostly with regards to concurrency - add architectural overview to documentation - improve comments for documentation - use printk specifier for hex prefix instead of hard-coding it - spell check comments and strings, fix typos - unify comment style - run checkpatch --strict, fix these and other style issues Changes regarding specific patches (and more details) can be found on the individual patch. Maximilian Luz (9): platform/surface: Add Surface Aggregator subsystem platform/surface: aggregator: Add control packet allocation caching platform/surface: aggregator: Add event item allocation caching platform/surface: aggregator: Add trace points platform/surface: aggregator: Add error injection capabilities platform/surface: aggregator: Add dedicated bus and device type docs: driver-api: Add Surface Aggregator subsystem documentation platform/surface: Add Surface Aggregator user-space interface platform/surface: Add Surface ACPI Notify driver Documentation/driver-api/index.rst | 1 + .../surface_aggregator/client-api.rst | 38 + .../driver-api/surface_aggregator/client.rst | 393 +++ .../surface_aggregator/clients/cdev.rst | 85 + .../surface_aggregator/clients/index.rst | 21 + .../surface_aggregator/clients/san.rst | 44 + .../driver-api/surface_aggregator/index.rst | 21 + .../surface_aggregator/internal-api.rst | 67 + .../surface_aggregator/internal.rst | 577 ++++ .../surface_aggregator/overview.rst | 77 + .../driver-api/surface_aggregator/ssh.rst | 344 +++ .../userspace-api/ioctl/ioctl-number.rst | 2 + MAINTAINERS | 13 + drivers/platform/surface/Kconfig | 38 + drivers/platform/surface/Makefile | 3 + drivers/platform/surface/aggregator/Kconfig | 69 + drivers/platform/surface/aggregator/Makefile | 17 + drivers/platform/surface/aggregator/bus.c | 415 +++ drivers/platform/surface/aggregator/bus.h | 27 + .../platform/surface/aggregator/controller.c | 2543 +++++++++++++++++ .../platform/surface/aggregator/controller.h | 285 ++ drivers/platform/surface/aggregator/core.c | 833 ++++++ .../platform/surface/aggregator/ssh_msgb.h | 205 ++ .../surface/aggregator/ssh_packet_layer.c | 2046 +++++++++++++ .../surface/aggregator/ssh_packet_layer.h | 190 ++ .../platform/surface/aggregator/ssh_parser.c | 228 ++ .../platform/surface/aggregator/ssh_parser.h | 154 + .../surface/aggregator/ssh_request_layer.c | 1256 ++++++++ .../surface/aggregator/ssh_request_layer.h | 143 + drivers/platform/surface/aggregator/trace.h | 632 ++++ .../platform/surface/surface_acpi_notify.c | 886 ++++++ .../surface/surface_aggregator_cdev.c | 299 ++ include/linux/mod_devicetable.h | 18 + include/linux/surface_acpi_notify.h | 39 + include/linux/surface_aggregator/controller.h | 824 ++++++ include/linux/surface_aggregator/device.h | 423 +++ include/linux/surface_aggregator/serial_hub.h | 672 +++++ include/uapi/linux/surface_aggregator/cdev.h | 58 + scripts/mod/devicetable-offsets.c | 8 + scripts/mod/file2alias.c | 23 + 40 files changed, 14017 insertions(+) create mode 100644 Documentation/driver-api/surface_aggregator/client-api.rst create mode 100644 Documentation/driver-api/surface_aggregator/client.rst create mode 100644 Documentation/driver-api/surface_aggregator/clients/cdev.rst create mode 100644 Documentation/driver-api/surface_aggregator/clients/index.rst create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst create mode 100644 Documentation/driver-api/surface_aggregator/index.rst create mode 100644 Documentation/driver-api/surface_aggregator/internal-api.rst create mode 100644 Documentation/driver-api/surface_aggregator/internal.rst create mode 100644 Documentation/driver-api/surface_aggregator/overview.rst create mode 100644 Documentation/driver-api/surface_aggregator/ssh.rst create mode 100644 drivers/platform/surface/aggregator/Kconfig create mode 100644 drivers/platform/surface/aggregator/Makefile create mode 100644 drivers/platform/surface/aggregator/bus.c create mode 100644 drivers/platform/surface/aggregator/bus.h create mode 100644 drivers/platform/surface/aggregator/controller.c create mode 100644 drivers/platform/surface/aggregator/controller.h create mode 100644 drivers/platform/surface/aggregator/core.c create mode 100644 drivers/platform/surface/aggregator/ssh_msgb.h create mode 100644 drivers/platform/surface/aggregator/ssh_packet_layer.c create mode 100644 drivers/platform/surface/aggregator/ssh_packet_layer.h create mode 100644 drivers/platform/surface/aggregator/ssh_parser.c create mode 100644 drivers/platform/surface/aggregator/ssh_parser.h create mode 100644 drivers/platform/surface/aggregator/ssh_request_layer.c create mode 100644 drivers/platform/surface/aggregator/ssh_request_layer.h create mode 100644 drivers/platform/surface/aggregator/trace.h create mode 100644 drivers/platform/surface/surface_acpi_notify.c create mode 100644 drivers/platform/surface/surface_aggregator_cdev.c create mode 100644 include/linux/surface_acpi_notify.h create mode 100644 include/linux/surface_aggregator/controller.h create mode 100644 include/linux/surface_aggregator/device.h create mode 100644 include/linux/surface_aggregator/serial_hub.h create mode 100644 include/uapi/linux/surface_aggregator/cdev.h -- 2.29.2