Hi, Thank you for the feedback. Responses Inline. -----Original Message----- >From: Hans de Goede <hdegoede@xxxxxxxxxx> >Sent: Thursday, June 17, 2021 8:06 PM >To: Shravan, S <s.shravan@xxxxxxxxx>; mgross@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx >Cc: An, Sudhakar <sudhakar.an@xxxxxxxxx> >Subject: Re: [PATCH 0/1] [x86] BIOS SAR Driver for M.2 Intel Modems > >Hi, > >On 6/17/21 4:28 PM, Hans de Goede wrote: >> Hi Shravan, >> >> On 4/28/21 5:22 AM, Shravan S wrote: >>> SAR (Specific Absorption Rate) driver is a host driver implemented >>> for Linux or chrome platform to limit the exposure of human body to >>> RF frequency by informing the Intel M.2 modem to regulate the RF >>> power based on SAR data obtained from the sensors captured in the >>> BIOS. ACPI interface exposes this data from the BIOS to SAR driver. >>> The front end application in userspace ( eg: Modem Manager) will >>> interact with SAR driver to obtain information like the device mode >>> (Example: tablets, laptops, etx), Antenna index, baseband index, SAR table index and use available communication like MBIM interface to enable data communication to modem for RF power regulation. >>> >>> The BIOS gets notified about device mode changes through Sensor >>> Driver. This information is given to a (newly created) WWAN ACPI function driver when there is a device mode change. >>> The driver then uses a _DSM method to retrieve the required information from BIOS. >>> This information is then forwarded/multicast to the User-space using the NETLINK interface. >>> A lookup table is maintained inside the BIOS which suggests the SAR >>> Table index and Antenna Tuner Table Index values for individual device modes. >>> >>> The SAR parameters to be used on the Modem differs for each Regulatory Mode like FCC, CE and ISED. >>> Hence, the SAR parameters are stored separately in the SMBIOS table >>> in the OEM BIOS, for each of the Regulatory mode. Regulatory modes >>> will be different based on the region and network available in that region. >> >> If I'm reading the above correct then this code is really doing 2 >> things in 1 driver: >> >> 1. Listening to some sensors, which readings may impact the maximum >> amount of tx power which the modem may use. What kind of sensors are these ? >> Currently chrome-os based devices are using iio for proximity sensors, >> with specific labels added to each sensor to tell userspace that they >> indicate a human is close to one of the antennas: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/com >> mit/Documentation/ABI/testing/sysfs-bus-iio?id=6505dfab33c519368e54ae7 >> f3ea1bf4d9916fdc5 >> >> Would it be possible to use this standardized userspace interface for >> your use case ? [Shravan] Proximity sensors can work in scenarios where there is no other sources of information which can alter the sar handling. OEMs have given feedback that the device mode (tablet mode/laptop mode/clamshell etc) also play a part in SAR handling. Hence this has to be aggregated with the proximity sensor information. Also such an aggregation is specific to given host platform. As a consequence, this is best realized within an entity like embedded controller available on the host platform. This new driver exposes such aggregated SAR parameters that needs to be configured on specific RF device. >> >> 2. Exporting a table with various information from the BIOS tables to >> userspace. We do probably need a new userspace API for this, but I'm >> not sure netlink is the right answer here. >> >> Maybe just use a binary sysfs attribute under >> /sys/bus/platform/devices/INTC1092:00 ? >> That will make the interface non-generic, but I assume that the table >> contents are going to be Intel specific anyways so that should be >> fine. This will also allow simply exporting the table without the >> kernel needing to parse it. >> >> Using netlink is highly unusual for a driver living under >> platform/drivers/x86; and if you really want to use netlink for this >> then you should first define a generic protocol which is also going to >> work for other vendors' modems, which is impossible ATM because we >> don't know yet what other vendor's modems will need... [Shravan] As per your suggestion we are switching to the sysfs interface to expose this information. We have reworked the driver implemetation with the latest patch to remove the usage of netlink which should address this concern. Please do have a look at the latest patch to confirm the same. >Some more remarks from a quick look at the code, I see that besides a netlink interface you are also creating a chardev and doing ioctls on that. >That is really not good, it seems that the userspace API for this needs to be completely redesigned. Using both netlink and ioctls at the same time not acceptable. [Shravan] With the latest patch we have removed the IOCTL interface and switched to sysfs mode of communictaion to and from userspace. >Also you seem to create the chardev and netlink and module-load time, rather then from the sar_add() probe function. Which means that they will also be created on laptops which don't have an INTC1092 ACPI device at all, again not good. >Please drop the sar_init() and sar_exit() functions and use module_platform_driver to have module_init / exit register / unregister the driver and have them only do that, everything else should be done from the platform_driver probe function. [Shravan] The latest patch addresses this concern. Please do confirm. >Also you use "sar" for a lot of identifiers in the driver, but that is very generic where as this is a highly Intel specific driver; and likely in the future even newer Intel modems may use a different driver, so please replace the sar prefix with something more specific, e.g. "intc1092" [Shravan] Based on the feedback sar_device_table name has been changed from "sar" to "intc1092" on the latest patch V2. Please let us know if these changes addresses this concern. Thanks and Regards, Shravan S
