Re: [External] Re: [PATCH v2 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,

On 3/5/21 1:42 AM, Nitin Joshi1 wrote:
> Hello Hans,
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@xxxxxxxxxx>
>> Sent: Thursday, March 4, 2021 8:44 PM
>> To: Nitin Joshi <nitjoshi@xxxxxxxxx>
>> Cc: ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx; platform-driver-
>> x86@xxxxxxxxxxxxxxx; Nitin Joshi1 <njoshi1@xxxxxxxxxx>; Mark RH Pearson
>> <markpearson@xxxxxxxxxx>
>> Subject: [External] Re: [PATCH v2 1/2] platorm/x86: thinkpad_acpi: sysfs
>> interface to reduce wlan tx power
>>
>> Hi,
>>
>> On 2/16/21 8:36 AM, Nitin Joshi wrote:
>>> 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>
>>
>> Thank you for your patches, I'm afraid that there are still a couple of small
>> issues which need to be fixed before I can apply these:
> 
> Thank you for your comments and apologize for any inconvenience caused.
> 
>>
>> 1. Both patches have "platform" misspelled in the patch Subject.
> Ack.  I will correct it in next version.
> 
>> 2. The patches don't apply cleanly because your kbdlang patch has
>>   been merged and these are based on a thinkpad_acpi version without
>>   these.
> Ack.  I will take latest file and correct it in next version.
> 
>> 3. I've some review remarks about this patch, see my inline comments below.
>>   Note some of these remarks apply to patch 2/2 too
>>   (I've indicated when this is the case).
> Ack with thanks
> 
>>
>>> ---
>>>  .../admin-guide/laptops/thinkpad-acpi.rst     |  18 +++
>>>  drivers/platform/x86/thinkpad_acpi.c          | 130 ++++++++++++++++++
>>>  2 files changed, 148 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..af90251d79d7 100644
>>> --- a/drivers/platform/x86/thinkpad_acpi.c
>>> +++ b/drivers/platform/x86/thinkpad_acpi.c
>>> @@ -9983,6 +9983,132 @@ 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
>>> +#define DPRC_WLAN_POWER_REDUCE_BIT      BIT(4)
>>> +#define DPRC_WLAN_POWER_FULL_BIT        BIT(8)
>>> +static bool has_wlantxreduce;
>>> +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 *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 & DPRC_WLAN_POWER_REDUCE_BIT)
>>> +		*wlan_txreduce = 1;
>>> +	else if (output & DPRC_WLAN_POWER_FULL_BIT)
>>> +		*wlan_txreduce = 0;
>>> +	else
>>> +		return -ENODATA;
>>
>> If you return -ENODEV here, then the error handling in tpacpi_dprc_init()
>> becomes a lot simpler / easier to read.
>>
>> Note this remark applies to patch 2/2 too.
> Ack. I will modify it on next version.
> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/* sysfs wlan entry */
>>> +static ssize_t wlan_tx_strength_reduce_show(struct device *dev,
>>> +						struct device_attribute *attr,
>>> +						char *buf)
>>> +{
>>> +	int err;
>>> +
>>> +	err = get_wlan_state(&wlan_txreduce);
>>> +	if (err)
>>> +		return err;
>>
>> Is it necessary to re-query the setting here? Can't you just query it from
>> tpacpi_dprc_init() once and store the updated value in
>> wlan_tx_strength_reduce_store() on success?
> We will have to call this sys for WLAN power reduce or full from userspace based on
> some conditions. After setting we need to make sure if wlan is set correctly in BIOS .
> I can understand that if setting is success, then we can store updated value on success.
> However , since, we have command to get wlan tx state in "DPRC" method , so I just want to make sure 
> If wlan tx state is set correctly in BIOS as I don’t have any other way to confirm it.
> So, I think it's better to keep this setting here.   

Keeping the querying of the setting here is fine.

> 
>>
>>
>>> +
>>> +	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)
>>> +{
>>> +	int output, err, ret;
>>
>> Please use just err here, there is no need to have both err and ret.
> Ack. I will modify it in next version.
> 
>>
>>> +	bool state;
>>> +
>>> +	ret = kstrtobool(buf, &state);
>>> +	if (ret)
>>> +		return ret;
>>
>> So change to using err here.
>>
> Ack. I will modify it in next version.
> 
>>> +
>>> +	if (state)
>>> +		err = dprc_command(DPRC_SET_WLAN_POWER_REDUCE,
>> &output);
>>> +	else
>>> +		err = dprc_command(DPRC_SET_WLAN_POWER_FULL,
>> &output);
>>
>>
>> You are not doing anything with err here, shouldn't this have a:
>>
>> 	if (err)
>> 		return err;
>>
>> here ?
> Ack. I will recheck it and modify it in next version.
> 
> I will incorporate all comments and send updated patch by next week or asap.

Thank you.

Regards,

Hans



>>> +
>>> +	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(&wlan_txreduce);
>>> +	/*
>>> +	 * If support isn't available (ENODEV) for both devices then quit, but
>>> +	 * don't return an error.
>>> +	 */
>>> +	if ((wlantx_err == -ENODEV) || (wlantx_err == -ENODATA))
>>> +		return 0;
>>> +	/* Otherwise, if there was an error return it */
>>> +	if (wlantx_err && (wlantx_err != -ENODEV) && (wlantx_err != -
>> ENODATA))
>>> +		return wlantx_err;
>>> +	else if (!wlantx_err)
>>> +		has_wlantxreduce = true;
>>> +
>>> +	if (has_wlantxreduce) {
>>> +		err = sysfs_create_file(&tpacpi_pdev->dev.kobj,
>>> +
>> 	&dev_attr_wlan_tx_strength_reduce.attr);
>>> +		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);
>>> +}
>>> +
>>> +static struct ibm_struct dprc_driver_data = {
>>> +	.name = "dprc",
>>> +	.exit = dprc_exit,
>>> +};
>>> +
>>>
>> /****************************************************************
>> ************
>>>
>> *****************************************************************
>> ***********
>>>   *
>>> @@ -10475,6 +10601,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)
>>>
> 



_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel




[Index of Archives]     [Linux ACPI]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Photo]     [Yosemite Photos]     [Yosemite Advice]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]

  Powered by Linux