[RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
times out and returnes an error which then is propagated to the
caller and causes the entire system suspend to be aborted which isn't
very useful.

Instead of aborting system suspend, print the error into the log
and continue.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thunderbolt/icm.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/thunderbolt/icm.c
===================================================================
--- linux-pm.orig/drivers/thunderbolt/icm.c
+++ linux-pm/drivers/thunderbolt/icm.c
@@ -904,7 +904,14 @@ static int icm_driver_ready(struct tb *t
 
 static int icm_suspend(struct tb *tb)
 {
-	return nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_SAVE_DEVS, 0);
+	int ret;
+
+	ret = nhi_mailbox_cmd(tb->nhi, NHI_MAILBOX_SAVE_DEVS, 0);
+	if (ret)
+		tb_info(tb, "Ignoring mailbox command error (%d) in %s\n",
+			ret, __func__);
+
+	return 0;
 }
 
 /*

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Mika Westerberg]

On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> times out and returnes an error which then is propagated to the
> caller and causes the entire system suspend to be aborted which isn't
> very useful.
> 
> Instead of aborting system suspend, print the error into the log
> and continue.

I agree, it should not prevent suspend but I wonder why it fails in the
first place? Can you check what is the return value?

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Rafael J. Wysocki]

On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > times out and returnes an error which then is propagated to the
> > caller and causes the entire system suspend to be aborted which isn't
> > very useful.
> > 
> > Instead of aborting system suspend, print the error into the log
> > and continue.
> 
> I agree, it should not prevent suspend but I wonder why it fails in the
> first place? Can you check what is the return value?

As per the above, the error is a timeout, ie. -ETIMEDOUT.

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Mika Westerberg]

On Tue, Jul 25, 2017 at 06:10:57PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> > On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > > times out and returnes an error which then is propagated to the
> > > caller and causes the entire system suspend to be aborted which isn't
> > > very useful.
> > > 
> > > Instead of aborting system suspend, print the error into the log
> > > and continue.
> > 
> > I agree, it should not prevent suspend but I wonder why it fails in the
> > first place? Can you check what is the return value?
> 
> As per the above, the error is a timeout, ie. -ETIMEDOUT.

Ah, right I somehow missed that.

Does it have Falcon Ridge controller or Alpine Ridge? Just to make sure,
can you increase the timeout in nhi_mailbox_cmd() to 1000ms or so. It
should not take that long though but better to check.

Which system this is BTW?

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Rafael J. Wysocki]

On Wednesday, July 26, 2017 11:32:44 AM Mika Westerberg wrote:
> On Tue, Jul 25, 2017 at 06:10:57PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> > > On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > > > times out and returnes an error which then is propagated to the
> > > > caller and causes the entire system suspend to be aborted which isn't
> > > > very useful.
> > > > 
> > > > Instead of aborting system suspend, print the error into the log
> > > > and continue.
> > > 
> > > I agree, it should not prevent suspend but I wonder why it fails in the
> > > first place? Can you check what is the return value?
> > 
> > As per the above, the error is a timeout, ie. -ETIMEDOUT.
> 
> Ah, right I somehow missed that.
> 
> Does it have Falcon Ridge controller or Alpine Ridge?

I'll check later today, but i guess you'll know (see below).

> Just to make sure, can you increase the timeout in nhi_mailbox_cmd()
> to 1000ms or so. It should not take that long though but better to check.

Well, I can do that, but I don't think it will help.

It just looks like the chip is not responding at all at that point.

> Which system this is BTW?

It's the Dell 9360. :-)

Sometimes after a reboot or a power cycle it starts in a state in which the
TBT controller and a USB one (which seem to be somehow connected)
appear to be dead or at least really flaky.  Basically, the box needs to be
power-cycled again to get rid of this condition and then everything works.

Thanks,
Rafael

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Mika Westerberg]

On Wed, Jul 26, 2017 at 02:48:54PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 26, 2017 11:32:44 AM Mika Westerberg wrote:
> > On Tue, Jul 25, 2017 at 06:10:57PM +0200, Rafael J. Wysocki wrote:
> > > On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> > > > On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > 
> > > > > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > > > > times out and returnes an error which then is propagated to the
> > > > > caller and causes the entire system suspend to be aborted which isn't
> > > > > very useful.
> > > > > 
> > > > > Instead of aborting system suspend, print the error into the log
> > > > > and continue.
> > > > 
> > > > I agree, it should not prevent suspend but I wonder why it fails in the
> > > > first place? Can you check what is the return value?
> > > 
> > > As per the above, the error is a timeout, ie. -ETIMEDOUT.
> > 
> > Ah, right I somehow missed that.
> > 
> > Does it have Falcon Ridge controller or Alpine Ridge?
> 
> I'll check later today, but i guess you'll know (see below).

No need to check, it is Alpine Ridge (since it is Dell 9360).

> > Just to make sure, can you increase the timeout in nhi_mailbox_cmd()
> > to 1000ms or so. It should not take that long though but better to check.
> 
> Well, I can do that, but I don't think it will help.
> 
> It just looks like the chip is not responding at all at that point.

I see.

Then I think we should apply your patch now and we can investigate this
further offline and hopefully find the root cause for the problem.

For this patch:

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> > Which system this is BTW?
> 
> It's the Dell 9360. :-)
> 
> Sometimes after a reboot or a power cycle it starts in a state in which the
> TBT controller and a USB one (which seem to be somehow connected)
> appear to be dead or at least really flaky.  Basically, the box needs to be
> power-cycled again to get rid of this condition and then everything works.

The xHCI controller is part of the Thunderbolt controller so whenever
you have normal USB-C device connected there, you should also see the
Alpine Ridge hierarchy in lspci output but the Thunderbolt host
controller is not there.

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Rafael J. Wysocki]

On Wednesday, July 26, 2017 04:12:59 PM Mika Westerberg wrote:
> On Wed, Jul 26, 2017 at 02:48:54PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 26, 2017 11:32:44 AM Mika Westerberg wrote:
> > > On Tue, Jul 25, 2017 at 06:10:57PM +0200, Rafael J. Wysocki wrote:
> > > > On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> > > > > On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > 
> > > > > > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > > > > > times out and returnes an error which then is propagated to the
> > > > > > caller and causes the entire system suspend to be aborted which isn't
> > > > > > very useful.
> > > > > > 
> > > > > > Instead of aborting system suspend, print the error into the log
> > > > > > and continue.
> > > > > 
> > > > > I agree, it should not prevent suspend but I wonder why it fails in the
> > > > > first place? Can you check what is the return value?
> > > > 
> > > > As per the above, the error is a timeout, ie. -ETIMEDOUT.
> > > 
> > > Ah, right I somehow missed that.
> > > 
> > > Does it have Falcon Ridge controller or Alpine Ridge?
> > 
> > I'll check later today, but i guess you'll know (see below).
> 
> No need to check, it is Alpine Ridge (since it is Dell 9360).
> 
> > > Just to make sure, can you increase the timeout in nhi_mailbox_cmd()
> > > to 1000ms or so. It should not take that long though but better to check.
> > 
> > Well, I can do that, but I don't think it will help.
> > 
> > It just looks like the chip is not responding at all at that point.
> 
> I see.
> 
> Then I think we should apply your patch now and we can investigate this
> further offline and hopefully find the root cause for the problem.
> 
> For this patch:
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

OK

I guess I can apply it, then, or if anyone else in the CC wants to do that,
please let me know.

> 
> > > Which system this is BTW?
> > 
> > It's the Dell 9360. :-)
> > 
> > Sometimes after a reboot or a power cycle it starts in a state in which the
> > TBT controller and a USB one (which seem to be somehow connected)
> > appear to be dead or at least really flaky.  Basically, the box needs to be
> > power-cycled again to get rid of this condition and then everything works.
> 
> The xHCI controller is part of the Thunderbolt controller so whenever
> you have normal USB-C device connected there, you should also see the
> Alpine Ridge hierarchy in lspci output but the Thunderbolt host
> controller is not there.

I don't have any USB-C devices, though, so I can't really test it this way ATM.

It's jjust never used now. :-)

Thanks,
Rafael

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Mika Westerberg]

On Wed, Jul 26, 2017 at 07:05:01PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, July 26, 2017 04:12:59 PM Mika Westerberg wrote:
> > On Wed, Jul 26, 2017 at 02:48:54PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, July 26, 2017 11:32:44 AM Mika Westerberg wrote:
> > > > On Tue, Jul 25, 2017 at 06:10:57PM +0200, Rafael J. Wysocki wrote:
> > > > > On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> > > > > > On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > 
> > > > > > > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > > > > > > times out and returnes an error which then is propagated to the
> > > > > > > caller and causes the entire system suspend to be aborted which isn't
> > > > > > > very useful.
> > > > > > > 
> > > > > > > Instead of aborting system suspend, print the error into the log
> > > > > > > and continue.
> > > > > > 
> > > > > > I agree, it should not prevent suspend but I wonder why it fails in the
> > > > > > first place? Can you check what is the return value?
> > > > > 
> > > > > As per the above, the error is a timeout, ie. -ETIMEDOUT.
> > > > 
> > > > Ah, right I somehow missed that.
> > > > 
> > > > Does it have Falcon Ridge controller or Alpine Ridge?
> > > 
> > > I'll check later today, but i guess you'll know (see below).
> > 
> > No need to check, it is Alpine Ridge (since it is Dell 9360).
> > 
> > > > Just to make sure, can you increase the timeout in nhi_mailbox_cmd()
> > > > to 1000ms or so. It should not take that long though but better to check.
> > > 
> > > Well, I can do that, but I don't think it will help.
> > > 
> > > It just looks like the chip is not responding at all at that point.
> > 
> > I see.
> > 
> > Then I think we should apply your patch now and we can investigate this
> > further offline and hopefully find the root cause for the problem.
> > 
> > For this patch:
> > 
> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> OK
> 
> I guess I can apply it, then, or if anyone else in the CC wants to do that,
> please let me know.

Greg typically takes these but I'm fine either way.

(Eventually we should probably establish Thunderbolt tree/branch in
 kernel.org where we gather all Thunderbolt related patches and just send
 pull request to Greg or something like that).

> > > > Which system this is BTW?
> > > 
> > > It's the Dell 9360. :-)
> > > 
> > > Sometimes after a reboot or a power cycle it starts in a state in which the
> > > TBT controller and a USB one (which seem to be somehow connected)
> > > appear to be dead or at least really flaky.  Basically, the box needs to be
> > > power-cycled again to get rid of this condition and then everything works.
> > 
> > The xHCI controller is part of the Thunderbolt controller so whenever
> > you have normal USB-C device connected there, you should also see the
> > Alpine Ridge hierarchy in lspci output but the Thunderbolt host
> > controller is not there.
> 
> I don't have any USB-C devices, though, so I can't really test it this way ATM.
> 
> It's jjust never used now. :-)

OK. We have one 9365 which I think should be pretty similar. I'll give
it a try once I find it ;-)

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Rafael J. Wysocki]

On Thursday, July 27, 2017 11:50:11 AM Mika Westerberg wrote:
> On Wed, Jul 26, 2017 at 07:05:01PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, July 26, 2017 04:12:59 PM Mika Westerberg wrote:
> > > On Wed, Jul 26, 2017 at 02:48:54PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, July 26, 2017 11:32:44 AM Mika Westerberg wrote:
> > > > > On Tue, Jul 25, 2017 at 06:10:57PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Tuesday, July 25, 2017 01:00:12 PM Mika Westerberg wrote:
> > > > > > > On Tue, Jul 25, 2017 at 01:31:00AM +0200, Rafael J. Wysocki wrote:
> > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > > 
> > > > > > > > On one of my test machines nhi_mailbox_cmd() called from icm_suspend()
> > > > > > > > times out and returnes an error which then is propagated to the
> > > > > > > > caller and causes the entire system suspend to be aborted which isn't
> > > > > > > > very useful.
> > > > > > > > 
> > > > > > > > Instead of aborting system suspend, print the error into the log
> > > > > > > > and continue.
> > > > > > > 
> > > > > > > I agree, it should not prevent suspend but I wonder why it fails in the
> > > > > > > first place? Can you check what is the return value?
> > > > > > 
> > > > > > As per the above, the error is a timeout, ie. -ETIMEDOUT.
> > > > > 
> > > > > Ah, right I somehow missed that.
> > > > > 
> > > > > Does it have Falcon Ridge controller or Alpine Ridge?
> > > > 
> > > > I'll check later today, but i guess you'll know (see below).
> > > 
> > > No need to check, it is Alpine Ridge (since it is Dell 9360).
> > > 
> > > > > Just to make sure, can you increase the timeout in nhi_mailbox_cmd()
> > > > > to 1000ms or so. It should not take that long though but better to check.
> > > > 
> > > > Well, I can do that, but I don't think it will help.
> > > > 
> > > > It just looks like the chip is not responding at all at that point.
> > > 
> > > I see.
> > > 
> > > Then I think we should apply your patch now and we can investigate this
> > > further offline and hopefully find the root cause for the problem.
> > > 
> > > For this patch:
> > > 
> > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > OK
> > 
> > I guess I can apply it, then, or if anyone else in the CC wants to do that,
> > please let me know.
> 
> Greg typically takes these but I'm fine either way.

OK, applied then.

RE: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Jamet, Michael]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Wednesday, July 26, 2017 20:05
> To: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: LKML <linux-kernel@vger.kernel.org>; Linux PM <linux-
> pm@vger.kernel.org>; Andreas Noever <andreas.noever@gmail.com>;
> Jamet, Michael <michael.jamet@intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>
> Subject: Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in
> icm_suspend()
> 
> >
> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

A bit late in the party.
Acked-by: Michael Jamet <michael.jamet@intel.com>
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Andy Shevchenko]

> ---------------------------------------------------------------------
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

This mail will be destroyed before reading. If you aware of open
source contribution or discussion you _must_ remove this footer.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend()

[Greg Kroah-Hartman]

On Mon, Jul 31, 2017 at 07:00:32AM +0000, Jamet, Michael wrote:
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Now deleted!!!

Go kick your email system, such footers are not compatible with open
source development at all, and cause your emails to be ignored...