Re: [PATCH 0/1] [x86] BIOS SAR Driver for M.2 Intel Modems

[Hans de Goede <hdegoede@redhat.com>]

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/commit/Documentation/ABI/testing/sysfs-bus-iio?id=6505dfab33c519368e54ae7f3ea1bf4d9916fdc5
> 
> Would it be possible to use this standardized userspace interface for
> your use case ?
> 
> 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...

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.

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.

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"

Regards,

Hans