Hi,
On 6/24/21 12:24 PM, Andy Shevchenko wrote:
> On Wed, Jun 23, 2021 at 5:04 PM Shravan S <s.shravan@xxxxxxxxx> wrote:
>
> Besides the fact of missed documentation, this code is far from the
> upstream-ready quality.
I agree that the code needs more work here.
> Send a new version to our internal mailing list for the detailed review.
I'm not in favor of internal reviews, esp. not when new userspace
API is involved. I would greatly prefer for the discussions surrounding
this to be kept public.
> And as I said, the interface should be provided by WWAN subsystem (or
> whatever subsystem(s) for this will be suitable *), while this should
> be a driver between actual implementation and the abstract interface.
> I believe this kind of feature will be needed (if not used already) by
> non-Intel hardware and we should have a more unified interface from
> day 1. Is it possible?
I agree that ideally we should have some generic userspace API for this,
but I'm afraid that ATM that simply is not possible. This whole notion
of maximum tx power being controlled based on various sensors because
of SAR reasons is pretty new (at least in the PC/laptop space) and
I know of a couple of vendors who are slapping some adhoc firmware
interface on the sensor readings for some modem related userspace
process to consume.
The 2 implementations which I know about for this are wildly different
and I expect we will see more, equally creative firmware interfaces
for this.
I don't think that it is realistic to try and layer a common userspace
interface over this at this point time. Actually I believe that even
trying to do so is a bad idea at this point in time, since this is
all so new that we are bound to get it badly wrong if we try to
design a common userspace API for this now.
I also don't want to wait for this to all crystallize out since that
will likely take years; and we should add support for this under Linux
in a reasonable time-frame. For laptops which ship with Linux
pre-installed it is important that there is feature parity between
Windows and Linux; and support for these new type of modems which need
this "SAR" integration is one of the biggest pain points with this ATM.
So for now I'm ok with having vendor specific userspace API for this
as long as it is somewhat sane and as long as it is very clear from
the sysfs path that this is vendor specific.
> *) maybe you need to introduce a layer which will provide a common
> interface between subsystems and hardware blocks for this kind of
> functionality. Either way, lack of documentation is perceptible.
>
>> V2 :
>> * Changes to the remove netlink and introduce sysfs to communicate to Userspace via notify
>> * Handled review comments by changing ioctl calls to sysfs
I've not looked at the actual code at al yet, with that set thank you
changing over to sysfs.
I assume the sysfs atttributes for this all sit under
/sys/bus/platform/devices/INTC1092:00 ?
And thus it is very clear that these are sysfs attributes specifically
for the INTC1092 device ?
If that is the case; and assuming that the new sysfs attributes API
is sane I don't see the API issue as blocking this driver.
But as both Andy and Enrico have pointed out the code needs a lot
of work.
Shravan, please post a new version addressing all remarks made
sofar and then I'll try to make some time to review the sysfs
userspace API for this (and later I'll also review the code).
Regards,
Hans
>> * "sar" name change for platform_device_id to "intc1092"
>> * sar_init and sar_exit is modified as per review to minimal initialization
>> * Inclusion of debug only enabling of device mode change parameters
>
> You mixed up changelog / commit message / cover letter altogether.
> ...
>
>> +config INTEL_SAR
>> + tristate "Intel Specific Absorption Rate Driver"
>> + depends on ACPI
>> + help
>> + This driver 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 sensorscaptured in the BIOS. ACPI interface exposes this data
>
> sensors captured
>
>> + from the BIOS to SAR driver. The front end application in userspace
>> + will interact with SAR driver to obtain information like the device mode,
>> + Antenna index,baseband index, SAR table index and use available communication
>> + like MBIM interface to enable data communication to modem for RF power regulation.
>
> ...
>
>> +static void sar_send_notify(void)
>> +{
>> + if (!context) {
>
>> + pr_alert("%s: context is null\n", __func__);
>
> Is it dead code?
>
>> + return;
>> + }
>> + pr_debug("%s: %d, %d, %d, %d\n", __func__, context->sar_data.device_mode,
>> + context->sar_data.bandtable_index, context->sar_data.antennatable_index,
>> + context->sar_data.sartable_index);
>> + sysfs_notify(context->sar_kobject, NULL, "intc_data");
>> +}
>
> ...
>
>> +static void update_sar_data(void)
>> +{
>> + pr_debug("%s: Update SAR data\n", __func__);
>> + if (context->config_data[context->reg_value].device_mode_info &&
>> + context->sar_data.device_mode <=
>> + context->config_data[context->reg_value].total_dev_mode) {
>> + context->sar_data.antennatable_index =
>> + context->config_data[context->reg_value]
>> + .device_mode_info[context->sar_data.device_mode].antennatable_index;
>> + context->sar_data.bandtable_index =
>> + context->config_data[context->reg_value]
>> + .device_mode_info[context->sar_data.device_mode].bandtable_index;
>> + context->sar_data.sartable_index =
>> + context->config_data[context->reg_value]
>> + .device_mode_info[context->sar_data.device_mode].sartable_index;
>
> Oy vey, this is quite hard to read, use temporary variables.
>
>> + pr_debug("bandtable_index: %d\n", context->sar_data.bandtable_index);
>> + pr_debug("antennatable_index : %d\n", context->sar_data.antennatable_index);
>> + pr_debug("sartable_index: %d\n", context->sar_data.sartable_index);
>
> No way. Drop debug prints from production code.
>
>
>> + } else {
>> + pr_err("%s: sar data not assigned! Dev mode: %d, total_dev_mode: %d\n",
>> + __func__, context->sar_data.device_mode,
>> + context->config_data[context->reg_value].total_dev_mode);
>> + }
>> +}
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
