Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1695265 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-25 01:40 +0200
    Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in  icm_suspend() Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-07-25 12:10 +0200
      Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-25 18:20 +0200
        Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in  icm_suspend() Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-07-26 10:40 +0200
          Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-26 15:00 +0200
            Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in  icm_suspend() Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-07-26 15:20 +0200
              Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-26 19:20 +0200
                Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in  icm_suspend() Mika Westerberg <mika.westerberg@linux.intel.com> - 2017-07-27 11:00 +0200
                  Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-07-28 02:10 +0200
                RE: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in  icm_suspend() "Jamet, Michael" <michael.jamet@intel.com> - 2017-07-31 09:10 +0200
                  Re: [RFC][PATCH] thunderbolt: icm: Ignore mailbox errors in icm_suspend() Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-07-31 11:40 +0200

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

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;
 }
 
 /*

[toc] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

#1695923

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.

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

#1697064

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

#1697397

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

[toc] | [prev] | [next] | [standalone]

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

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 ;-)

[toc] | [prev] | [next] | [standalone]

#1698374

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.

[toc] | [prev] | [next] | [standalone]

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

> -----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.

[toc] | [prev] | [next] | [standalone]

#1699855

> ---------------------------------------------------------------------
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web