NetBSD Problem Report #45932

From khorben@defora.org  Mon Feb  6 00:31:00 2012
Return-Path: <khorben@defora.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 3A8DE63BCF4
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  6 Feb 2012 00:31:00 +0000 (UTC)
Message-Id: <20120206002840.9AAFB1A461@syn.defora.rom>
Date: Mon,  6 Feb 2012 01:28:40 +0100 (CET)
From: Pierre Pronchery <khorben@defora.org>
Reply-To:
To: gnats-bugs@gnats.NetBSD.org
Subject: The tpm(4) driver prevents suspending
X-Send-Pr-Version: 3.95

>Number:         45932
>Category:       kern
>Synopsis:       It is impossible to suspend with tpm(4) attached
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 06 00:35:00 +0000 2012
>Closed-Date:    Sun Apr 22 19:45:49 +0000 2012
>Last-Modified:  Sun Apr 22 19:45:49 +0000 2012
>Originator:     Pierre Pronchery <khorben@defora.org>
>Release:        NetBSD 5.99.60
>Organization:
>Environment:
System: NetBSD syn.defora.rom 5.99.60 NetBSD 5.99.60 (GENERIC) #0: Sat Jan 28 14:38:06 CET 2012 khorben@syn.defora.rom:/home/amd64/obj/sys/arch/amd64/compile/GENERIC amd64
Architecture: x86_64
Machine: amd64
>Description:
The tpm(4) driver was introduced recently to the kernel (thanks!). It is
found on some laptop computers, like on my Lenovo ThinkPad T60 2007:

/netbsd: tpm0 at isa0 iomem 0xfed40000-0xfed44fff irq 7: ATML 97SC3203 rev 0x5

Unfortunately, with this driver attached, the laptop fails to suspend:

/netbsd: acpi0: entering state S3
/netbsd: Devices without power management support: tpm0
/netbsd: Devices withoug suspend

This error message is a bit confusing, since the actual issue is not
with tpm(4) not supporting power management; it supports it, but the
callback functions do not return the proper value, indicating instead
(and apparently, wrongly so) that the attempt at suspending the device
failed.

I wonder therefore if it would make sense to warn about devices not
supporting power management as soon as they are done attaching. Then I
suppose it would be easier to report about failures when actually trying
to suspend.

>How-To-Repeat:
1. Run netbsd-current on a computer featuring a device matched by tpm(4)
2. # sysctl -w machdep.sleep_state=3
3. Suspending is aborted:
>Fix:
Fixing the tpm(4) driver itself is trivial once this figured out (found
below).

I haven't looked at how to let the error message be more clear.

Index: sys/dev/ic/tpm.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/tpm.c,v
retrieving revision 1.6
diff -p -u -r1.6 tpm.c
--- sys/dev/ic/tpm.c	3 Feb 2012 15:42:46 -0000	1.6
+++ sys/dev/ic/tpm.c	6 Feb 2012 00:26:23 -0000
@@ -333,7 +333,7 @@ tpm_suspend(device_t dev, const pmf_qual
 #ifdef TPM_DEBUG
 	aprint_debug_dev(sc->sc_dev, "%s: power down\n", __func__);
 #endif
-	return 0;
+	return true;
 }

 /*
@@ -347,7 +347,7 @@ tpm_resume(device_t dev, const pmf_qual_
 	struct tpm_softc *sc = device_private(dev);
 	aprint_debug_dev(sc->sc_dev, "%s: resume\n", __func__);
 #endif
-	return 0;
+	return true;
 }

 /* Wait for given status bits using polling. */

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45932 CVS commit: src/sys/dev/ic
Date: Sun, 5 Feb 2012 21:03:32 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Mon Feb  6 02:03:32 UTC 2012

 Modified Files:
 	src/sys/dev/ic: tpm.c

 Log Message:
 PR/45932: Pierre Pronchery: Fix the return values of the suspend and resume
 functions.


 To generate a diff of this commit:
 cvs rdiff -u -r1.6 -r1.7 src/sys/dev/ic/tpm.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Pierre Pronchery <khorben@defora.org>
To: gnats-bugs@NetBSD.org
Cc: Christos Zoulas <christos@netbsd.org>, kern-bug-people@netbsd.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/45932 CVS commit: src/sys/dev/ic
Date: Mon, 06 Feb 2012 04:14:25 +0100

 			Hey Christos,

 thanks for committing this - unfortunately I realized that it was not
 enough. Here's maybe the reason why I was confused by the error messages
 from pmf:

 Index: sys/dev/isa/tpm_isa.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/isa/tpm_isa.c,v
 retrieving revision 1.1
 diff -p -u -r1.1 tpm_isa.c
 --- sys/dev/isa/tpm_isa.c	22 Jan 2012 06:44:28 -0000	1.1
 +++ sys/dev/isa/tpm_isa.c	6 Feb 2012 03:06:30 -0000
 @@ -117,10 +117,11 @@ tpm_isa_attach(device_t parent, device_t
  		return;
  	}

 -	    device_xname(sc->sc_dev))) != 0)
 +	    device_xname(sc->sc_dev))) != 0) {
  		bus_space_unmap(sc->sc_bt, sc->sc_bh, size);
  		return;
 +	}

  	/*
  	 * Only setup interrupt handler when we have a vector and the

 With the brackets missing, attaching was returning prematurely,
 therefore not setting the interrupt handler and obviously, not really
 registering to PMF either.

 Now it suspends :)

 Cheers,
 -- khorben

 On 06/02/2012 03:05, Christos Zoulas wrote:
 > The following reply was made to PR kern/45932; it has been noted by GNATS.
 > 
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc: 
 > Subject: PR/45932 CVS commit: src/sys/dev/ic
 > Date: Sun, 5 Feb 2012 21:03:32 -0500
 > 
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Mon Feb  6 02:03:32 UTC 2012
 >  
 >  Modified Files:
 >  	src/sys/dev/ic: tpm.c
 >  
 >  Log Message:
 >  PR/45932: Pierre Pronchery: Fix the return values of the suspend and resume
 >  functions.
 >  
 >  
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.6 -r1.7 src/sys/dev/ic/tpm.c
 >  
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >  


 -- 
 khorben

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45932 CVS commit: src/sys/dev/isa
Date: Sun, 5 Feb 2012 23:29:47 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Mon Feb  6 04:29:47 UTC 2012

 Modified Files:
 	src/sys/dev/isa: tpm_isa.c

 Log Message:
 PR/45932: Add missing braces... This will probably fix interrupts too!


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/sys/dev/isa/tpm_isa.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: christos@zoulas.com (Christos Zoulas)
To: Pierre Pronchery <khorben@defora.org>, gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org
Subject: Re: PR/45932 CVS commit: src/sys/dev/ic
Date: Sun, 5 Feb 2012 23:30:12 -0500

 On Feb 6,  4:14am, khorben@defora.org (Pierre Pronchery) wrote:
 -- Subject: Re: PR/45932 CVS commit: src/sys/dev/ic

 | 			Hey Christos,
 | 
 | thanks for committing this - unfortunately I realized that it was not
 | enough. Here's maybe the reason why I was confused by the error messages
 | from pmf:

 Not only that, it will probably fix my interrupt issue!

 christos

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 22 Apr 2012 19:45:49 +0000
State-Changed-Why:
fixed


>Unformatted:

Home	PR Database Search