NetBSD Problem Report #49926

From www@NetBSD.org  Sun May 24 14:12:11 2015
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 4D947A57FE
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 24 May 2015 14:12:11 +0000 (UTC)
Message-Id: <20150524141210.3D288A65BB@mollari.NetBSD.org>
Date: Sun, 24 May 2015 14:12:10 +0000 (UTC)
From: rmh@freebsd.org
Reply-To: rmh@freebsd.org
To: gnats-bugs@NetBSD.org
Subject: [PATCH] Avoid deadlock condition in auich_read_codec() and auich_write_codec()
X-Send-Pr-Version: www-1.0

>Number:         49926
>Category:       kern
>Synopsis:       [PATCH] Avoid deadlock condition in auich_read_codec() and auich_write_codec()
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun May 24 14:15:00 +0000 2015
>Closed-Date:    Sat Jun 13 04:25:02 +0000 2015
>Last-Modified:  Sat Jun 13 04:25:02 +0000 2015
>Originator:     Robert Millan
>Release:        HEAD
>Organization:
>Environment:
RUMP environment (on top of GNU/Linux host)
>Description:
Please consider the following fix for auich(4):

    Avoid deadlock condition in auich_read_codec() and auich_write_codec()

    When running this code in userspace (e.g. RUMP), it can be permanently interrupted at any time if the process which contains it is killed.

    If this happens in certain parts of auich_read_codec() or auich_write_codec() routines, ICH_CAS register may be left in an inconsistent state which leads to deadlock:

      - a command is expected by ICH_CAS before granting access to the driver
      - the driver has requested access and is polling ICH_CAS before issuing a command

    This commit makes these routines more permissive when run for first time, so that they can recover from this inconsistent state.

    For details on ICH_CAS semaphore, refer to Intel datasheet (page 328):

    http://www.intel.com/design/chipsets/datashts/290655.htm 


>How-To-Repeat:

>Fix:
diff --git a/sys/dev/pci/auich.c b/sys/dev/pci/auich.c
index 3038091..b2adad1 100644
--- a/sys/dev/pci/auich.c
+++ b/sys/dev/pci/auich.c
@@ -789,6 +789,19 @@ auich_read_codec(void *v, uint8_t reg, uint16_t *val)
 		ICH_CAS + sc->sc_modem_offset) & 1;
 	    DELAY(ICH_CODECIO_INTERVAL));

+	/*
+	 * Be permissive in first attempt. If previous instances of
+	 * this routine were interrupted precisely at this point (after
+	 * access is granted by CAS but before a command is sent),
+	 * they could have left hardware in an inconsistent state where
+	 * a command is expected and therefore semaphore wait would hit
+	 * the timeout.
+	 */
+	static int first = 1;
+	if (i <= 0 && first)
+		i = 1;
+	first = 0;
+
 	if (i > 0) {
 		*val = bus_space_read_2(sc->iot, sc->mix_ioh,
 		    reg + (sc->sc_codecnum * ICH_CODEC_OFFSET));
@@ -832,6 +845,12 @@ auich_write_codec(void *v, uint8_t reg, uint16_t val)
 		ICH_CAS + sc->sc_modem_offset) & 1;
 	    DELAY(ICH_CODECIO_INTERVAL));

+	/* Be permissive in first attempt (see comments in auich_read_codec) */
+	static int first = 1;
+	if (i <= 0 && first)
+		i = 1;
+	first = 0;
+
 	if (i > 0) {
 		bus_space_write_2(sc->iot, sc->mix_ioh,
 		    reg + (sc->sc_codecnum * ICH_CODEC_OFFSET), val);

>Release-Note:

>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: kern/49926: [PATCH] Avoid deadlock condition in auich_read_codec() and auich_write_codec()
Date: Sun, 24 May 2015 11:02:31 -0400

 On May 24,  2:15pm, rmh@freebsd.org (rmh@freebsd.org) wrote:
 -- Subject: kern/49926: [PATCH] Avoid deadlock condition in auich_read_codec(

 Should we have 2 static "first" variables, one per function, or one is
 enough?

 christos

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/49926 CVS commit: src/sys/dev/pci
Date: Sun, 24 May 2015 18:03:02 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Sun May 24 22:03:02 UTC 2015

 Modified Files:
 	src/sys/dev/pci: auich.c

 Log Message:
 PR/49926: Robert Millan: Avoid deadlock condition in auich_read_codec()
 and auich_write_codec() when running in userspase.


 To generate a diff of this commit:
 cvs rdiff -u -r1.147 -r1.148 src/sys/dev/pci/auich.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: Robert Millan <rmh@freebsd.org>, gnats-bugs@NetBSD.org, 
	kern-bug-people@netbsd.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org, rumpkernel-users@freelists.org
Cc: 
Subject: Re: kern/49926: [PATCH] Avoid deadlock condition in auich_read_codec() and auich_write_codec()
Date: Sun, 24 May 2015 18:03:46 -0400

 On May 24, 10:07pm, rmh@freebsd.org (Robert Millan) wrote:
 -- Subject: Re: kern/49926: [PATCH] Avoid deadlock condition in auich_read_co

 | Good point. A shared variable should be enough. And it works fine according
 | to my tests.
 | 
 | Furthermore, while doing this change I realized this isn't supposed to be
 | global, as the information it contains applies independently to each auich
 | device in the system, so I moved it to the sc_auich structure.
 | 
 | Here's a new (tested) patch.
 | 
 | Many thanks

 Thank you! Committed.

 christos

From: Robert Millan <rmh@freebsd.org>
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
 gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, 
 rumpkernel-users@freelists.org
Cc: 
Subject: Re: kern/49926: [PATCH] Avoid deadlock condition in auich_read_codec()
 and auich_write_codec()
Date: Sun, 24 May 2015 22:07:37 +0200

 This is a multi-part message in MIME format.
 --------------090706070508000801080506
 Content-Type: text/plain; charset=windows-1252; format=flowed
 Content-Transfer-Encoding: 7bit

 El 24/05/15 a les 17:05, Christos Zoulas ha escrit:
 >   On May 24,  2:15pm, rmh@freebsd.org (rmh@freebsd.org) wrote:
 >   -- Subject: kern/49926: [PATCH] Avoid deadlock condition in auich_read_codec(
 >
 >   Should we have 2 static "first" variables, one per function, or one is
 >   enough?

 Good point. A shared variable should be enough. And it works fine according
 to my tests.

 Furthermore, while doing this change I realized this isn't supposed to be
 global, as the information it contains applies independently to each auich
 device in the system, so I moved it to the sc_auich structure.

 Here's a new (tested) patch.

 Many thanks

 -- 
 Robert Millan

 --------------090706070508000801080506
 Content-Type: text/x-patch;
  name="auich_deadlock.diff"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="auich_deadlock.diff"

 diff --git a/sys/dev/pci/auich.c b/sys/dev/pci/auich.c
 index 481ed16..ae1662f 100644
 --- a/sys/dev/pci/auich.c
 +++ b/sys/dev/pci/auich.c
 @@ -232,6 +232,8 @@ struct auich_softc {
  	struct audio_format sc_modem_formats[AUICH_MODEM_NFORMATS];
  	struct audio_encoding_set *sc_encodings;
  	struct audio_encoding_set *sc_spdif_encodings;
 +
 +	int sc_cas_been_used;
  };

  /* Debug */
 @@ -796,10 +798,9 @@ auich_read_codec(void *v, uint8_t reg, uint16_t *val)
  	 * a command is expected and therefore semaphore wait would hit
  	 * the timeout.
  	 */
 -	static int first = 1;
 -	if (i <= 0 && first)
 +	if (!sc->sc_cas_been_used && i <= 0)
  		i = 1;
 -	first = 0;
 +	sc->sc_cas_been_used = 1;

  	if (i > 0) {
  		*val = bus_space_read_2(sc->iot, sc->mix_ioh,
 @@ -845,10 +846,9 @@ auich_write_codec(void *v, uint8_t reg, uint16_t val)
  	    DELAY(ICH_CODECIO_INTERVAL));

  	/* Be permissive in first attempt (see comments in auich_read_codec) */
 -	static int first = 1;
 -	if (i <= 0 && first)
 +	if (!sc->sc_cas_been_used && i <= 0)
  		i = 1;
 -	first = 0;
 +	sc->sc_cas_been_used = 1;

  	if (i > 0) {
  		bus_space_write_2(sc->iot, sc->mix_ioh,

 --------------090706070508000801080506--

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sat, 13 Jun 2015 04:25:02 +0000
State-Changed-Why:
patch committed


>Unformatted:

Home	PR Database Search