| Message ID | 20210212055856.232702-1-njoshi1@lenovo.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] platorm/x86: thinkpad_acpi: sysfs interface to reduce wlan tx power | expand |
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@lenovo.com> > Signed-off-by: Nitin Joshi <njoshi1@lenovo.com> > --- > .../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. > +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). > + *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? > + > + *has_wlantxreduce = 1; Is it necessary for the getter to set this? Couldn't it be set in `tpacpi_dprc_init()` once during initialization? > + 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 ... ...); > +{ > + 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? > + 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. > +{ > + int output, err; > + unsigned long t; > + > + if (parse_strtoul(buf, 1, &t)) Maybe `kstrtobool`? > + 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. > + 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. > + 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()`. > +} > + > +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
Hi, Thank you for your comments. >-----Original Message----- >From: Barnabás Pőcze <pobrn@protonmail.com> >Sent: Friday, February 12, 2021 5:56 PM >To: Nitin Joshi <nitjoshi@gmail.com> >Cc: hdegoede@redhat.com; ibm-acpi-devel@lists.sourceforge.net; platform- >driver-x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH >Pearson <markpearson@lenovo.com> >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@lenovo.com> >> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com> >> --- >> .../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
Hi 2021. február 12., péntek 11:40 keltezéssel, Nitin Joshi1 <njoshi1@lenovo.com> írta: > [...] > >> > >+/*************************************************************** > >***** > >> +***** > >> + * 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. > If `dprc_command()` handles all error cases (i.e. it is not possible that `dprc_command()` returns 0, but there was an error) - which seems to be the case - then I think in that branch you should return -ENODATA and not touch `wlan_txreduce`. Because if that branch runs, then there was no error, but the state cannot be determined, so I think -ENODATA is an appropriate error code. > > > > > >> + > >> + *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. > I was thinking of something like the following: static int tpacpi_dprc_init(...) { ... int err = get_wlan_state(&reduced); if (err && err != -ENODEV) return err; else if (!err) has_wlantxreduce = true; err = get_wwan_antenna(&antenna); if (err && err != -ENODEV) return err; else if (!err) has_antennatype = true; /* as I've commented on the second patch, this variable is probably not needed */ ... If I'm not mistaken this is enough not to need the second argument in either `get_wlan_state()` or `get_wwan_antenna()`. Note, that if both functions may return -ENODATA, you might also want to add `err != -ENODATA` to the conditions. > > > > > >> + 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. > I think the place to determine whether the operation is supported or not is in `tpacpi_dprc_init()` and the attribute should not be created if it's not supported. > > > > > >> + 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 ? > If you use `kstrtobool()`, you can even drop the entire switch statement. > > > > > >> + 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. > There's not much difference, so sysfs_{create,remove}_file() would work just as fine here. The reason why I believe `device_{create,remove}_file()` is possibly preferable is that you don't need to reference the kobj and the attribute when calling it, and you're adding an attribute to a *device* (not any kobj), so device_*() functions are a better fit syntactically in my opinion. But in the end, both will achieve the same effect, so you are free to choose whichever you like. > > > >> + 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. > [...] Regards, Barnabás Pőcze
Hi, >-----Original Message----- >From: Barnabás Pőcze <pobrn@protonmail.com> >Sent: Sunday, February 14, 2021 5:48 AM >To: Nitin Joshi1 <njoshi1@lenovo.com> >Cc: Nitin Joshi <nitjoshi@gmail.com>; hdegoede@redhat.com; ibm-acpi- >devel@lists.sourceforge.net; platform-driver-x86@vger.kernel.org; Mark RH >Pearson <markpearson@lenovo.com> >Subject: RE: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs >interface to reduce wlan tx power > >Hi > > >2021. február 12., péntek 11:40 keltezéssel, Nitin Joshi1 ><njoshi1@lenovo.com> írta: > >> [...] >> >> >> >>+/************************************************************** >* >> >***** >> >> +***** >> >> + * 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. >> > >If `dprc_command()` handles all error cases (i.e. it is not possible that >`dprc_command()` returns 0, but there was an error) - which seems to be the >case - then I think in that branch you should return -ENODATA and not touch >`wlan_txreduce`. Because if that branch runs, then there was no error, but the >state cannot be determined, so I think -ENODATA is an appropriate error code. Ack . Noted, I will check it > > >> > >> > >> >> + >> >> + *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. >> > >I was thinking of something like the following: > > static int tpacpi_dprc_init(...) { > ... > int err = get_wlan_state(&reduced); > if (err && err != -ENODEV) > return err; > else if (!err) > has_wlantxreduce = true; > > err = get_wwan_antenna(&antenna); > if (err && err != -ENODEV) > return err; > else if (!err) > has_antennatype = true; /* as I've commented on the second patch, this > variable is probably not needed */ > ... > Ack . I will check it >If I'm not mistaken this is enough not to need the second argument in either >`get_wlan_state()` or `get_wwan_antenna()`. Note, that if both functions may >return -ENODATA, you might also want to add `err != -ENODATA` to the >conditions. > Ack . yes. > >> > >> > >> >> + 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. >> > >I think the place to determine whether the operation is supported or not is in >`tpacpi_dprc_init()` and the attribute should not be created if it's not >supported. Ack . I will check it and send next patch version soon. > > >> > >> > >> >> + 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 ? >> > >If you use `kstrtobool()`, you can even drop the entire switch statement. Ack . yes , I think so too. > > >> > >> > >> >> + 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. >> > >There's not much difference, so sysfs_{create,remove}_file() would work just >as fine here. The reason why I believe `device_{create,remove}_file()` is >possibly preferable is that you don't need to reference the kobj and the >attribute when calling it, and you're adding an attribute to a *device* (not any >kobj), so device_*() functions are a better fit syntactically in my opinion. But in >the end, both will achieve the same effect, so you are free to choose >whichever you like. Ack . Noted. I would like to keep sysfs_{ create,remove}_file() now. I will recheck code, incorporate comments and send next patch version soon. > >> > >> >> + 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. >> [...] > > >Regards, >Barnabás Pőcze Thanks & Regards, Nitin Joshi
Hi Nitin, On 2/12/21 6:58 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@lenovo.com> > Signed-off-by: Nitin Joshi <njoshi1@lenovo.com> This patch as well as patch 2/2 generally look ok to me. The one thing which stands out is the: *wlan_txreduce = -1; Resp: *wwan_antennatype = -1; default: return sysfs_emit(buf, "unknown type\n"); Bits, which Barnabás already pointed out. If you can prepare a v2 of this patch-set addressing all the review remarks which you have received so far then that would be great. Regards, Hans > --- > .../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; > +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)) > + *wlan_txreduce = 1; > + else if (output & BIT(8)) > + *wlan_txreduce = 0; > + else > + *wlan_txreduce = -1; > + > + *has_wlantxreduce = 1; > + 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(&has_wlantxreduce, &wlan_txreduce); > + if (err) > + return err; > + > + 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; > + unsigned long t; > + > + if (parse_strtoul(buf, 1, &t)) > + 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; > + } > + > + 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); > + 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 +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) >
Hello Hans, >-----Original Message----- >From: Hans de Goede <hdegoede@redhat.com> >Sent: Monday, February 15, 2021 11:48 PM >To: Nitin Joshi <nitjoshi@gmail.com> >Cc: ibm-acpi-devel@lists.sourceforge.net; platform-driver- >x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH Pearson ><markpearson@lenovo.com> >Subject: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs >interface to reduce wlan tx power > >Hi Nitin, > >On 2/12/21 6:58 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@lenovo.com> >> Signed-off-by: Nitin Joshi <njoshi1@lenovo.com> > >This patch as well as patch 2/2 generally look ok to me. > >The one thing which stands out is the: > > *wlan_txreduce = -1; > >Resp: > > *wwan_antennatype = -1; > > default: > return sysfs_emit(buf, "unknown type\n"); > >Bits, which Barnabás already pointed out. > >If you can prepare a v2 of this patch-set addressing all the review remarks >which you have received so far then that would be great. > Thank you for your comment. I have already prepared patch and will send patch soon. >Regards, > >Hans Thanks & Regards, Nitin Joshi > > > > >> --- >> .../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; >> +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)) >> + *wlan_txreduce = 1; >> + else if (output & BIT(8)) >> + *wlan_txreduce = 0; >> + else >> + *wlan_txreduce = -1; >> + >> + *has_wlantxreduce = 1; >> + 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(&has_wlantxreduce, &wlan_txreduce); >> + if (err) >> + return err; >> + >> + 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; >> + unsigned long t; >> + >> + if (parse_strtoul(buf, 1, &t)) >> + 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; >> + } >> + >> + 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); >> + 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 +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) >>
On Fri, 2021-02-12 at 14:58 +0900, 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. Question from a user: How does wlan_tx_strength_reduce relate to or interact with ioctl(SIOCSIWTXPOW) (i.e. iwconfig txpower) and NL80211_ATTR_WIPHY_TX_POWER_LEVEL (i.e. iw dev set txpower)? If I request 30 dBm then enable wlan_tx_strength_reduce, what happens? Same in the opposite order? Will ioctl(SIOCGIWTXPOW) show the reduced txpower? Thanks, Kevin
Hello, >-----Original Message----- >From: Kevin Locke <kevin@kevinlocke.name> >Sent: Saturday, March 6, 2021 1:43 AM >To: Nitin Joshi <nitjoshi@gmail.com> >Cc: hdegoede@redhat.com; ibm-acpi-devel@lists.sourceforge.net; platform- >driver-x86@vger.kernel.org; Nitin Joshi1 <njoshi1@lenovo.com>; Mark RH >Pearson <markpearson@lenovo.com> >Subject: [External] Re: [PATCH 1/2] platorm/x86: thinkpad_acpi: sysfs >interface to reduce wlan tx power > >On Fri, 2021-02-12 at 14:58 +0900, 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. > >Question from a user: How does wlan_tx_strength_reduce relate to or >interact with ioctl(SIOCSIWTXPOW) (i.e. iwconfig txpower) and >NL80211_ATTR_WIPHY_TX_POWER_LEVEL (i .e. iw dev set txpower)? If I >request 30 dBm then enable wlan_tx_strength_reduce, what happens? Same >in the opposite order? Will ioctl(SIOCGIWTXPOW) show the reduced >txpower? Below comment is just to update current status in this e-mail chain: As informed in separate e-mail, after testing by changing txpower using "iwconfig tx power xx" I cannot see any change in wlan_tx_strength_reduce It seems wlan_tx_strength_reduce is not directly related to iwconfig or iw. So, I am currently trying to find more information regarding this. Although, this patch is working fine but need some more information for better understanding. Hence , dropping this patch series and splitting it i.e submitting only "WWAN Antenna patch " now. Thank you !! Thanks & Regards, Nitin Joshi > >Thanks, >Kevin
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; +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)) + *wlan_txreduce = 1; + else if (output & BIT(8)) + *wlan_txreduce = 0; + else + *wlan_txreduce = -1; + + *has_wlantxreduce = 1; + 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(&has_wlantxreduce, &wlan_txreduce); + if (err) + return err; + + 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; + unsigned long t; + + if (parse_strtoul(buf, 1, &t)) + 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; + } + + 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); + 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 +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)