[PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[Joseph Salisbury]

BugLink: http://bugs.launchpad.net/bugs/1210848

On an ASUSTek G60JX laptop, the intel_ips driver spams the log with a warning message: "ME failed to update for more than 1s, likely hung".  This ME doesn't support the feature, so requesting it be blacklisted for now.   

Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>
Tested-by: Nick Jenkins <tech.crew.jenkins@gmail.com> 
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: platform-driver-x86@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
---
 drivers/platform/x86/intel_ips.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
index 18dcb58..6d8c5e0 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -1501,6 +1501,14 @@ static const struct dmi_system_id ips_blacklist[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "HP ProBook"),
 		},
 	},
+	{
+		.callback = ips_blacklist_callback,
+		.ident = "G60JX",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK Computer Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "G60JX"),
+		},
+	},
 	{ }	/* terminating entry */
 };
 
-- 
1.7.9.5

Re: [PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[Jesse Barnes]

On Wed, 14 Aug 2013 12:22:49 -0400
Joseph Salisbury <joseph.salisbury@canonical.com> wrote:

> BugLink: http://bugs.launchpad.net/bugs/1210848
> 
> On an ASUSTek G60JX laptop, the intel_ips driver spams the log with a warning message: "ME failed to update for more than 1s, likely hung".  This ME doesn't support the feature, so requesting it be blacklisted for now.   
> 
> Signed-off-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> Tested-by: Nick Jenkins <tech.crew.jenkins@gmail.com> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: platform-driver-x86@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>  drivers/platform/x86/intel_ips.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel_ips.c b/drivers/platform/x86/intel_ips.c
> index 18dcb58..6d8c5e0 100644
> --- a/drivers/platform/x86/intel_ips.c
> +++ b/drivers/platform/x86/intel_ips.c
> @@ -1501,6 +1501,14 @@ static const struct dmi_system_id ips_blacklist[] = {
>  			DMI_MATCH(DMI_PRODUCT_NAME, "HP ProBook"),
>  		},
>  	},
> +	{
> +		.callback = ips_blacklist_callback,
> +		.ident = "G60JX",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK Computer Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "G60JX"),
> +		},
> +	},
>  	{ }	/* terminating entry */
>  };
>  

Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Linus, you may want to pick this up directly, as I'm not sure if
Matthew is still looking after the x86 drivers these days.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[Matthew Garrett]

On Wed, Aug 14, 2013 at 09:38:36AM -0700, Jesse Barnes wrote:

> Linus, you may want to pick this up directly, as I'm not sure if
> Matthew is still looking after the x86 drivers these days.

I am, but I'm not keen on this patch. Which is more likely - that the ME 
is hung, or that it was never working on this in the first place? Is 
there really no bit to indicate whether or not the platform supports the 
feature?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[Linus Torvalds]

On Wed, Aug 14, 2013 at 9:38 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
> Linus, you may want to pick this up directly, as I'm not sure if
> Matthew is still looking after the x86 drivers these days.

Can't we make the simpler patch be to just not spam the logs? Do it
once, and forget about it. Maybe make the timeouts long enough to make
sure that there are no other downsides from having the driver
occasionally testing if it's back?

I *detest* hardware-specific blacklists. They are impossible to
maintain, and we've had the situation more than once that we fixed the
real bug that caused the blacklist in the first place, but the entry
stays around because nobody knows/cares/tests it.

                  Linus

Re: [PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[Joseph Salisbury]

On 08/14/2013 05:11 PM, Linus Torvalds wrote:
> On Wed, Aug 14, 2013 at 9:38 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> Linus, you may want to pick this up directly, as I'm not sure if
>> Matthew is still looking after the x86 drivers these days.
> Can't we make the simpler patch be to just not spam the logs? Do it
> once, and forget about it. Maybe make the timeouts long enough to make
> sure that there are no other downsides from having the driver
> occasionally testing if it's back?
>
> I *detest* hardware-specific blacklists. They are impossible to
> maintain, and we've had the situation more than once that we fixed the
> real bug that caused the blacklist in the first place, but the entry
> stays around because nobody knows/cares/tests it.
>
>                   Linus
Hi Jesse,

What are your thoughts on alternatives to using a blacklist?  Is there a
bit we can read to determine if IPS is supported on a platform?

If not, can we do something like the following?  Basically increase the
timeout to 30 seconds then return an error from the monitor if there was
no update in that time:

diff --git a/drivers/platform/x86/intel_ips.c
b/drivers/platform/x86/intel_ips.c
index 18dcb58..a2391f7 100644
--- a/drivers/platform/x86/intel_ips.c
+++ b/drivers/platform/x86/intel_ips.c
@@ -1095,8 +1095,15 @@ static int ips_monitor(void *data)
                cur_seqno = (thm_readl(THM_ITV) & ITV_ME_SEQNO_MASK) >>
                        ITV_ME_SEQNO_SHIFT;
                if (cur_seqno == last_seqno &&
-                   time_after(jiffies, seqno_timestamp + HZ)) {
-                       dev_warn(&ips->dev->dev, "ME failed to update
for more than 1s, likely hung\n");
+                   time_after(jiffies, seqno_timestamp + (HZ*30))) {
+                       /* It's been 30s.  ME is likely hung.  Print
message and return. */
+                       dev_warn(&ips->dev->dev, "ME failed to update
for more than 30s, likely hung so exiting\n");
+                       del_timer_sync(&timer);
+                       destroy_timer_on_stack(&timer);
+
+                       dev_dbg(&ips->dev->dev, "ips-monitor thread
stopped\n");
+                       return -1;
+
                } else {
                        seqno_timestamp = get_jiffies_64();
                        last_seqno = cur_seqno;

There may be more to include such as restarting the monitor later to see
if the ME ever came back, just looking for other ideas and/or suggestions.

Thanks,

Joe

Re: [PATCH 1/1] intel_ips: blacklist ASUSTek G60JX laptops

[Jesse Barnes]

On Thu, 29 Aug 2013 12:18:14 -0400
Joseph Salisbury <joseph.salisbury@canonical.com> wrote:

> On 08/14/2013 05:11 PM, Linus Torvalds wrote:
> > On Wed, Aug 14, 2013 at 9:38 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> Linus, you may want to pick this up directly, as I'm not sure if
> >> Matthew is still looking after the x86 drivers these days.
> > Can't we make the simpler patch be to just not spam the logs? Do it
> > once, and forget about it. Maybe make the timeouts long enough to make
> > sure that there are no other downsides from having the driver
> > occasionally testing if it's back?
> >
> > I *detest* hardware-specific blacklists. They are impossible to
> > maintain, and we've had the situation more than once that we fixed the
> > real bug that caused the blacklist in the first place, but the entry
> > stays around because nobody knows/cares/tests it.
> >
> >                   Linus
> Hi Jesse,
> 
> What are your thoughts on alternatives to using a blacklist?  Is there a
> bit we can read to determine if IPS is supported on a platform?
> 
> If not, can we do something like the following?  Basically increase the
> timeout to 30 seconds then return an error from the monitor if there was
> no update in that time:
> 
> diff --git a/drivers/platform/x86/intel_ips.c
> b/drivers/platform/x86/intel_ips.c
> index 18dcb58..a2391f7 100644
> --- a/drivers/platform/x86/intel_ips.c
> +++ b/drivers/platform/x86/intel_ips.c
> @@ -1095,8 +1095,15 @@ static int ips_monitor(void *data)
>                 cur_seqno = (thm_readl(THM_ITV) & ITV_ME_SEQNO_MASK) >>
>                         ITV_ME_SEQNO_SHIFT;
>                 if (cur_seqno == last_seqno &&
> -                   time_after(jiffies, seqno_timestamp + HZ)) {
> -                       dev_warn(&ips->dev->dev, "ME failed to update
> for more than 1s, likely hung\n");
> +                   time_after(jiffies, seqno_timestamp + (HZ*30))) {
> +                       /* It's been 30s.  ME is likely hung.  Print
> message and return. */
> +                       dev_warn(&ips->dev->dev, "ME failed to update
> for more than 30s, likely hung so exiting\n");
> +                       del_timer_sync(&timer);
> +                       destroy_timer_on_stack(&timer);
> +
> +                       dev_dbg(&ips->dev->dev, "ips-monitor thread
> stopped\n");
> +                       return -1;
> +
>                 } else {
>                         seqno_timestamp = get_jiffies_64();
>                         last_seqno = cur_seqno;
> 
> There may be more to include such as restarting the monitor later to see
> if the ME ever came back, just looking for other ideas and/or suggestions.

Yeah, something like that might be ok.  But ideally we'd detect whether
an ME was present at all like Matthew says.

I'm not sure how to do that, but maybe Anas does (or can point us to
someone else who would know).

-- 
Jesse Barnes, Intel Open Source Technology Center