RE: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Thank you for your comments.

>-----Original Message-----
>From: Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx>
>Sent: Friday, February 12, 2021 5:56 PM
>To: Nitin Joshi <nitjoshi@xxxxxxxxx>
>Cc: hdegoede@xxxxxxxxxx; ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform-
>driver-x86@xxxxxxxxxxxxxxx; Nitin Joshi1 <njoshi1@xxxxxxxxxx>; Mark RH
>Pearson <markpearson@xxxxxxxxxx>
>Subject: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs
>interface to reduce wlan tx power
>
>Hi
>
>
>2021. február 12., péntek 6:58 keltezéssel, Nitin Joshi írta:
>
>> Some newer Thinkpads have the WLAN antenna placed close to the WWAN
>> antenna. In these cases FCC certification requires that when WWAN is
>> active we reduce WLAN transmission power. A new Dynamic Power
>> Reduction Control (DPRC) method is available from the BIOS on these
>> platforms to reduce or restore WLAN Tx power.
>>
>> This patch provides a sysfs interface that userspace can use to adjust
>> the WLAN power appropriately.
>>
>> Reviewed-by: Mark Pearson <markpearson@xxxxxxxxxx>
>> Signed-off-by: Nitin Joshi <njoshi1@xxxxxxxxxx>
>> ---
>>  .../admin-guide/laptops/thinkpad-acpi.rst     |  18 +++
>>  drivers/platform/x86/thinkpad_acpi.c          | 136 ++++++++++++++++++
>>  2 files changed, 154 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> index 5fe1ade88c17..10410811b990 100644
>> --- a/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> +++ b/Documentation/admin-guide/laptops/thinkpad-acpi.rst
>> @@ -51,6 +51,7 @@ detailed description):
>>  	- UWB enable and disable
>>  	- LCD Shadow (PrivacyGuard) enable and disable
>>  	- Lap mode sensor
>> +	- WLAN transmission power control
>>
>>  A compatibility table by model and feature is maintained on the web
>> site, http://ibm-acpi.sf.net/. I appreciate any success or failure @@
>> -1447,6 +1448,23 @@ they differ between desk and lap mode.
>>  The property is read-only. If the platform doesn't have support the
>> sysfs  class is not created.
>>
>> +WLAN transmission power control
>> +--------------------------------
>> +
>> +sysfs: wlan_tx_strength_reduce
>> +
>> +Some newer Thinkpads have the WLAN antenna placed close to the WWAN
>antenna.
>> +This interface will be used by userspace to reduce the WLAN Tx power
>> +strength when WWAN is active, as is required for FCC certification.
>> +
>> +The available commands are::
>> +
>> +        echo '0' >
>/sys/devices/platform/thinkpad_acpi/wlan_tx_strength_reduce
>> +        echo '1' >
>> + /sys/devices/platform/thinkpad_acpi/wlan_tx_strength_reduce
>> +
>> +The first command restores the wlan transmission power and the latter
>> +one reduces wlan transmission power.
>> +
>>  EXPERIMENTAL: UWB
>>  -----------------
>>
>> diff --git a/drivers/platform/x86/thinkpad_acpi.c
>> b/drivers/platform/x86/thinkpad_acpi.c
>> index f3e8eca8d86d..6dbbd4a14264 100644
>> --- a/drivers/platform/x86/thinkpad_acpi.c
>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>> @@ -9983,6 +9983,138 @@ static struct ibm_struct proxsensor_driver_data
>= {
>>  	.exit = proxsensor_exit,
>>  };
>>
>>
>+/***************************************************************
>*****
>> +*****
>> + * DPRC(Dynamic Power Reduction Control) subdriver, for the Lenovo
>> +WWAN
>> + * and WLAN feature.
>> + */
>> +#define DPRC_GET_WLAN_STATE             0x20000
>> +#define DPRC_SET_WLAN_POWER_REDUCE      0x00030010
>> +#define DPRC_SET_WLAN_POWER_FULL        0x00030100
>> +static int has_wlantxreduce;
>
>I think `bool` would be better.

Ack . I will modify  it in next version.

>
>
>> +static int wlan_txreduce;
>> +
>> +static int dprc_command(int command, int *output) {
>> +	acpi_handle dprc_handle;
>> +
>> +	if (ACPI_FAILURE(acpi_get_handle(hkey_handle, "DPRC",
>&dprc_handle))) {
>> +		/* Platform doesn't support DPRC */
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (!acpi_evalf(dprc_handle, output, NULL, "dd", command))
>> +		return -EIO;
>> +
>> +	/*
>> +	 * METHOD_ERR gets returned on devices where few commands are
>not supported
>> +	 * for example WLAN power reduce command is not supported on
>some devices.
>> +	 */
>> +	if (*output & METHOD_ERR)
>> +		return -ENODEV;
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_wlan_state(int *has_wlantxreduce, int *wlan_txreduce)
>> +{
>> +	int output, err;
>> +
>> +	/* Get current WLAN Power Transmission state */
>> +	err = dprc_command(DPRC_GET_WLAN_STATE, &output);
>> +	if (err)
>> +		return err;
>> +
>> +	if (output & BIT(4))
>
>I believe it'd be preferable to name `BIT(4)` and `BIT(8)`. E.g.:
>
>  #define DPRC_GET_WLAN_STATE_RES_REDUCED BIT(4)
>  #define DPRC_GET_WLAN_STATE_RES_FULL    BIT(8)
>
>(or any name you like).
>
Ack . I will modify  it in next version.

>
>> +		*wlan_txreduce = 1;
>> +	else if (output & BIT(8))
>> +		*wlan_txreduce = 0;
>> +	else
>> +		*wlan_txreduce = -1;
>
>Can you elaborate what -1 means here? Unknown/not
>available/invalid/error?

-1 means "error" .
I had found that when "DPRC" method return METHOD_ERR i.e BIT(31) as 0 then it goes to this condition.
So , therefore I had added METHOD_ERR check in dprc_command() and now , this doesnot goes here.
But, I have still kept it here , just in case if any other error occurred . 
Can you please suggest , if I should remove it (i.e remove *wlan_txreduce = -1; )?  as I still think, there is no harm keeping like this.
 
>
>
>> +
>> +	*has_wlantxreduce = 1;
>
>Is it necessary for the getter to set this? Couldn't it be set in
>`tpacpi_dprc_init()` once during initialization?

Actually, yes we can keep it in init function also but I have not kept it because of other patch (PATCH 2/2)
which I had sent . patch 1 (this patch) and patch 2 ( other patch which create sysfs of WWWAN Antenna type)
both uses "DPRC" method . So , we will need a flag to create sysfs because few system will not have this "wlan tx reduce"
even if it has "DPRC" method exists and vice versa . 
So , in this case, we need to explicitly check whether it require to create corresponding sysfs  or not.

>
>
>> +	return 0;
>> +}
>> +
>> +/* sysfs wlan entry */
>> +static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
>> +				struct device_attribute *attr,
>> +				char *buf)
>
>Please align the arguments:
>
>  ..._show(struct device *dev,
>           struct device_attribute ...
>           ...);
>
Ack . I will modify  it in next version.
Also , I will correct it in my other patch(PATCH 2/2) also.

>
>> +{
>> +	int err;
>> +
>> +	err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> +	if (err)
>> +		return err;
>> +
>
>Wouldn't it be better to return -ENODATA or something similar when
>`wlan_txreduce` == -1?

Ack . I think EOPNOTSUPP will be better ? reason is that "DPRC" method is available but there is error . So , its more likely that command is not supported.
However, I will modify it after I get feedback about my previous comment.

>
>
>> +	return sysfs_emit(buf, "%d\n", wlan_txreduce); }
>> +
>> +static ssize_t wlan_tx_strength_reduce_store(struct device *dev,
>> +				struct device_attribute *attr,
>> +				const char *buf, size_t count)
>
>Same here.
>
Ack . I will modify  it in next version.
>
>> +{
>> +	int output, err;
>> +	unsigned long t;
>> +
>> +	if (parse_strtoul(buf, 1, &t))
>
>Maybe `kstrtobool`?

Thank you for your suggestion.
I can use 'kstrtobool' and will modify on my next version.

>
>
>> +		return -EINVAL;
>> +
>> +	tpacpi_disclose_usertask(attr->attr.name,
>> +				"WLAN tx strength reduced %lu\n", t);
>> +
>> +	switch (t) {
>> +	case 0:
>> +		err = dprc_command(DPRC_SET_WLAN_POWER_FULL,
>&output);
>> +		break;
>> +	case 1:
>> +		err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE,
>&output);
>> +		break;
>> +	default:
>> +		err = -EINVAL;
>> +		dev_err(&tpacpi_pdev->dev, "Unknown operating mode.
>Ignoring\n");
>> +		break;
>> +	}
>> +
>
>If I'm not mistaken, `err` is never returned, so the write() will always seem to
>succeed.

Yes , its correct . I will use 'kstrtobool' and will drop this : "err = -EINVAL;"
Is it Ok ?

>
>
>> +	sysfs_notify(&tpacpi_pdev->dev.kobj, NULL,
>"wlan_tx_strength_reduce");
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(wlan_tx_strength_reduce);
>> +
>> +static int tpacpi_dprc_init(struct ibm_init_struct *iibm) {
>> +	int wlantx_err, err;
>> +
>> +	wlantx_err = get_wlan_state(&has_wlantxreduce, &wlan_txreduce);
>> +	/*
>> +	 * If support isn't available (ENODEV) for both devices then quit, but
>> +	 * don't return an error.
>> +	 */
>> +	if (wlantx_err == -ENODEV)
>> +		return 0;
>> +	/* Otherwise, if there was an error return it */
>> +	if (wlantx_err && (wlantx_err != -ENODEV))
>> +		return wlantx_err;
>> +
>> +	if (has_wlantxreduce) {
>> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>> +				&dev_attr_wlan_tx_strength_reduce.attr);
>
>I believe `device_create_file()` would be better.
>
Since, file will be created in /sys/ directory , hence I think its better to use sysfs_create_file.
Also, by checking in this  file, it looks like sysfs_create_file will be more reasonable to do .

Please let me know, if its Ok to continue using sysfs_create_file or you still feel. we need to use 
device_create_file()  also feel free to correct me, if I am wrong.
 
>
>> +		if (err)
>> +			return err;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void dprc_exit(void)
>> +{
>> +	if (has_wlantxreduce)
>> +		sysfs_remove_file(&tpacpi_pdev->dev.kobj,
>> +&dev_attr_wlan_tx_strength_reduce.attr);
>
>And similarly, `device_remove_file()`.

Same comment as above . I feel, sysfs_remove_file is more reasonable to do.
>
>
>> +}
>> +
>> +static struct ibm_struct dprc_driver_data = {
>> +	.name = "dprc",
>> +	.exit = dprc_exit,
>> +};
>> +
>>
>/****************************************************************
>************
>>
>*****************************************************************
>***********
>>   *
>> @@ -10475,6 +10607,10 @@ static struct ibm_init_struct ibms_init[]
>__initdata = {
>>  		.init = tpacpi_proxsensor_init,
>>  		.data = &proxsensor_driver_data,
>>  	},
>> +	{
>> +		.init = tpacpi_dprc_init,
>> +		.data = &dprc_driver_data,
>> +	},
>>  };
>>
>>  static int __init set_ibm_param(const char *val, const struct
>> kernel_param *kp)
>> --
>> 2.25.1
>
>
>Regards,
>Barnabás Pőcze

Thanks & Regards,
Nitin Joshi 




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux