2018-09-26 16:12:01

by Mario Limonciello

[permalink] [raw]
Subject: [PATCH] platform/x86: intel-wmi-thunderbolt: Add dynamic debugging

Some users have been reporting issues with thunderbolt being turned off
before fully initialized. This is suspected to be caused by userspace
turning off the Thunderbolt controller using intel-wmi-thunderbolt
prematurely.

Details are available here:
https://bugzilla.kernel.org/show_bug.cgi?id=201227
https://bugzilla.kernel.org/show_bug.cgi?id=199631

Userspace has already made some mitigiations for this situation:
https://github.com/hughsie/fwupd/commit/ef6f1d76983c9b66
https://github.com/hughsie/fwupd/commit/c07ce5b4889a5384

To allow easier debugging of this situation add output that can be turned
on with dynamic debugging to better root cause this problem.

Suggested-by: Mika Westerberg <[email protected]>
Signed-off-by: Mario Limonciello <[email protected]>
---
drivers/platform/x86/intel-wmi-thunderbolt.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c b/drivers/platform/x86/intel-wmi-thunderbolt.c
index c2257bd..ce5fbf0 100644
--- a/drivers/platform/x86/intel-wmi-thunderbolt.c
+++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
@@ -38,12 +38,16 @@ static ssize_t force_power_store(struct device *dev,
input.length = sizeof(u8);
input.pointer = &mode;
mode = hex_to_bin(buf[0]);
+ dev_dbg(dev, "force_power: storing %#x\n", mode);
if (mode == 0 || mode == 1) {
status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, 0, 1,
&input, NULL);
- if (ACPI_FAILURE(status))
+ if (ACPI_FAILURE(status)) {
+ dev_dbg(dev, "force_power: failed to evaluate ACPI method\n");
return -ENODEV;
+ }
} else {
+ dev_dbg(dev, "force_power: unsupported mode\n");
return -EINVAL;
}
return count;
--
2.7.4



2018-09-26 17:04:36

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] platform/x86: intel-wmi-thunderbolt: Add dynamic debugging

On Wed, Sep 26, 2018 at 7:11 PM Mario Limonciello
<[email protected]> wrote:
>
> Some users have been reporting issues with thunderbolt being turned off
> before fully initialized. This is suspected to be caused by userspace
> turning off the Thunderbolt controller using intel-wmi-thunderbolt
> prematurely.
>

> Details are available here:
> https://bugzilla.kernel.org/show_bug.cgi?id=201227
> https://bugzilla.kernel.org/show_bug.cgi?id=199631

BugLink: ... ?
BugLink: ... ?

I can do it myself if you are okay with this format.

The patch itself LGTM.

>
> Userspace has already made some mitigiations for this situation:
> https://github.com/hughsie/fwupd/commit/ef6f1d76983c9b66
> https://github.com/hughsie/fwupd/commit/c07ce5b4889a5384
>
> To allow easier debugging of this situation add output that can be turned
> on with dynamic debugging to better root cause this problem.
>
> Suggested-by: Mika Westerberg <[email protected]>
> Signed-off-by: Mario Limonciello <[email protected]>
> ---
> drivers/platform/x86/intel-wmi-thunderbolt.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c b/drivers/platform/x86/intel-wmi-thunderbolt.c
> index c2257bd..ce5fbf0 100644
> --- a/drivers/platform/x86/intel-wmi-thunderbolt.c
> +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
> @@ -38,12 +38,16 @@ static ssize_t force_power_store(struct device *dev,
> input.length = sizeof(u8);
> input.pointer = &mode;
> mode = hex_to_bin(buf[0]);
> + dev_dbg(dev, "force_power: storing %#x\n", mode);
> if (mode == 0 || mode == 1) {
> status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, 0, 1,
> &input, NULL);
> - if (ACPI_FAILURE(status))
> + if (ACPI_FAILURE(status)) {
> + dev_dbg(dev, "force_power: failed to evaluate ACPI method\n");
> return -ENODEV;
> + }
> } else {
> + dev_dbg(dev, "force_power: unsupported mode\n");
> return -EINVAL;
> }
> return count;
> --
> 2.7.4
>


--
With Best Regards,
Andy Shevchenko

2018-09-26 17:05:06

by Mario Limonciello

[permalink] [raw]
Subject: RE: [PATCH] platform/x86: intel-wmi-thunderbolt: Add dynamic debugging

>
> On Wed, Sep 26, 2018 at 7:11 PM Mario Limonciello
> <[email protected]> wrote:
> >
> > Some users have been reporting issues with thunderbolt being turned off
> > before fully initialized. This is suspected to be caused by userspace
> > turning off the Thunderbolt controller using intel-wmi-thunderbolt
> > prematurely.
> >
>
> > Details are available here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=201227
> > https://bugzilla.kernel.org/show_bug.cgi?id=199631
>
> BugLink: ... ?
> BugLink: ... ?
>
> I can do it myself if you are okay with this format.
>
> The patch itself LGTM.

Sure, thank you.

>
> >
> > Userspace has already made some mitigiations for this situation:
> > https://github.com/hughsie/fwupd/commit/ef6f1d76983c9b66
> > https://github.com/hughsie/fwupd/commit/c07ce5b4889a5384
> >
> > To allow easier debugging of this situation add output that can be turned
> > on with dynamic debugging to better root cause this problem.
> >
> > Suggested-by: Mika Westerberg <[email protected]>
> > Signed-off-by: Mario Limonciello <[email protected]>
> > ---
> > drivers/platform/x86/intel-wmi-thunderbolt.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c
> b/drivers/platform/x86/intel-wmi-thunderbolt.c
> > index c2257bd..ce5fbf0 100644
> > --- a/drivers/platform/x86/intel-wmi-thunderbolt.c
> > +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
> > @@ -38,12 +38,16 @@ static ssize_t force_power_store(struct device *dev,
> > input.length = sizeof(u8);
> > input.pointer = &mode;
> > mode = hex_to_bin(buf[0]);
> > + dev_dbg(dev, "force_power: storing %#x\n", mode);
> > if (mode == 0 || mode == 1) {
> > status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, 0, 1,
> > &input, NULL);
> > - if (ACPI_FAILURE(status))
> > + if (ACPI_FAILURE(status)) {
> > + dev_dbg(dev, "force_power: failed to evaluate ACPI method\n");
> > return -ENODEV;
> > + }
> > } else {
> > + dev_dbg(dev, "force_power: unsupported mode\n");
> > return -EINVAL;
> > }
> > return count;
> > --
> > 2.7.4
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko

2018-09-26 17:12:39

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] platform/x86: intel-wmi-thunderbolt: Add dynamic debugging

On Wed, Sep 26, 2018 at 8:03 PM <[email protected]> wrote:
>
> >
> > On Wed, Sep 26, 2018 at 7:11 PM Mario Limonciello
> > <[email protected]> wrote:
> > >
> > > Some users have been reporting issues with thunderbolt being turned off
> > > before fully initialized. This is suspected to be caused by userspace
> > > turning off the Thunderbolt controller using intel-wmi-thunderbolt
> > > prematurely.
> > >
> >
> > > Details are available here:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=201227
> > > https://bugzilla.kernel.org/show_bug.cgi?id=199631
> >
> > BugLink: ... ?
> > BugLink: ... ?
> >
> > I can do it myself if you are okay with this format.
> >
> > The patch itself LGTM.
>
> Sure, thank you.

Pushed to my review and testing queue with mentioned changes, thanks!

>
> >
> > >
> > > Userspace has already made some mitigiations for this situation:
> > > https://github.com/hughsie/fwupd/commit/ef6f1d76983c9b66
> > > https://github.com/hughsie/fwupd/commit/c07ce5b4889a5384
> > >
> > > To allow easier debugging of this situation add output that can be turned
> > > on with dynamic debugging to better root cause this problem.
> > >
> > > Suggested-by: Mika Westerberg <[email protected]>
> > > Signed-off-by: Mario Limonciello <[email protected]>
> > > ---
> > > drivers/platform/x86/intel-wmi-thunderbolt.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/platform/x86/intel-wmi-thunderbolt.c
> > b/drivers/platform/x86/intel-wmi-thunderbolt.c
> > > index c2257bd..ce5fbf0 100644
> > > --- a/drivers/platform/x86/intel-wmi-thunderbolt.c
> > > +++ b/drivers/platform/x86/intel-wmi-thunderbolt.c
> > > @@ -38,12 +38,16 @@ static ssize_t force_power_store(struct device *dev,
> > > input.length = sizeof(u8);
> > > input.pointer = &mode;
> > > mode = hex_to_bin(buf[0]);
> > > + dev_dbg(dev, "force_power: storing %#x\n", mode);
> > > if (mode == 0 || mode == 1) {
> > > status = wmi_evaluate_method(INTEL_WMI_THUNDERBOLT_GUID, 0, 1,
> > > &input, NULL);
> > > - if (ACPI_FAILURE(status))
> > > + if (ACPI_FAILURE(status)) {
> > > + dev_dbg(dev, "force_power: failed to evaluate ACPI method\n");
> > > return -ENODEV;
> > > + }
> > > } else {
> > > + dev_dbg(dev, "force_power: unsupported mode\n");
> > > return -EINVAL;
> > > }
> > > return count;
> > > --
> > > 2.7.4
> > >
> >
> >
> > --
> > With Best Regards,
> > Andy Shevchenko



--
With Best Regards,
Andy Shevchenko

2018-09-26 18:34:57

by Mika Westerberg

[permalink] [raw]
Subject: Re: [PATCH] platform/x86: intel-wmi-thunderbolt: Add dynamic debugging

On Wed, Sep 26, 2018 at 11:10:58AM -0500, Mario Limonciello wrote:
> Some users have been reporting issues with thunderbolt being turned off
> before fully initialized. This is suspected to be caused by userspace
> turning off the Thunderbolt controller using intel-wmi-thunderbolt
> prematurely.
>
> Details are available here:
> https://bugzilla.kernel.org/show_bug.cgi?id=201227
> https://bugzilla.kernel.org/show_bug.cgi?id=199631
>
> Userspace has already made some mitigiations for this situation:
^^^^^^^^^^^^
mitigations?

> https://github.com/hughsie/fwupd/commit/ef6f1d76983c9b66
> https://github.com/hughsie/fwupd/commit/c07ce5b4889a5384
>
> To allow easier debugging of this situation add output that can be turned
> on with dynamic debugging to better root cause this problem.
>
> Suggested-by: Mika Westerberg <[email protected]>
> Signed-off-by: Mario Limonciello <[email protected]>

Reviewed-by: Mika Westerberg <[email protected]>

2018-09-26 19:26:37

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH] platform/x86: intel-wmi-thunderbolt: Add dynamic debugging

On Wed, Sep 26, 2018 at 9:34 PM Mika Westerberg
<[email protected]> wrote:
>
> On Wed, Sep 26, 2018 at 11:10:58AM -0500, Mario Limonciello wrote:
> > Some users have been reporting issues with thunderbolt being turned off
> > before fully initialized. This is suspected to be caused by userspace
> > turning off the Thunderbolt controller using intel-wmi-thunderbolt
> > prematurely.
> >
> > Details are available here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=201227
> > https://bugzilla.kernel.org/show_bug.cgi?id=199631
> >
> > Userspace has already made some mitigiations for this situation:
> ^^^^^^^^^^^^
> mitigations?

I've fixed that, but in any case I need to add your tag, thanks!


>
> > https://github.com/hughsie/fwupd/commit/ef6f1d76983c9b66
> > https://github.com/hughsie/fwupd/commit/c07ce5b4889a5384
> >
> > To allow easier debugging of this situation add output that can be turned
> > on with dynamic debugging to better root cause this problem.
> >
> > Suggested-by: Mika Westerberg <[email protected]>
> > Signed-off-by: Mario Limonciello <[email protected]>
>
> Reviewed-by: Mika Westerberg <[email protected]>



--
With Best Regards,
Andy Shevchenko