linux-kernel - [PATCH] Staging: comedi: add timeouts to while loops in s626.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-Id: <1393572936-9676-1-git-send-email-chase.southwood@yahoo.com>
Date:	Fri, 28 Feb 2014 01:35:36 -0600
From:	Chase Southwood <chase.southwood@...oo.com>
To:	gregkh@...uxfoundation.org
Cc:	abbotti@....co.uk, hsweeten@...ionengravers.com,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	Chase Southwood <chase.southwood@...oo.com>
Subject: [PATCH] Staging: comedi: add timeouts to while loops in s626.c

Smatch located a handful of while loops testing readl calls in s626.c.
Since these while loops depend on readl succeeding, it's safer to make
sure they time out eventually.

Signed-off-by: Chase Southwood <chase.southwood@...oo.com>
---
Ian and/or Hartley, I'd love your comments on this.  It seems to me that
we want these kinds of while loops properly timed out, but I want to make
sure I'm doing everything properly.  First off, s626_debi_transfer() says
directly that it is called from within critical sections, so I assume
that means that the new comedi_timeout() function is no good here, and
s626_send_dac() looked equally suspicious, so I opted for iterative
timeouts.  Is this correct?  Also, for these timeouts, I used a very
conservative 10000 iterations, would it be better to decrease that?
Also, do my error strings appear acceptable?

And finally, are timeouts here even necessary or helpful, or are there
any better ways to do it?

Thanks,
Chase

 drivers/staging/comedi/drivers/s626.c | 49 ++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/comedi/drivers/s626.c b/drivers/staging/comedi/drivers/s626.c
index 5ba4b4a..282636b 100644
--- a/drivers/staging/comedi/drivers/s626.c
+++ b/drivers/staging/comedi/drivers/s626.c
@@ -209,6 +209,8 @@ static const struct comedi_lrange s626_range_table = {
 static void s626_debi_transfer(struct comedi_device *dev)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* Initiate upload of shadow RAM to DEBI control register */
 	s626_mc_enable(dev, S626_MC2_UPLD_DEBI, S626_P_MC2);
@@ -221,8 +223,13 @@ static void s626_debi_transfer(struct comedi_device *dev)
 		;
 
 	/* Wait until DEBI transfer is done */
-	while (readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S)
-		;
+	for (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_PSR) & S626_PSR_DEBI_S))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DEBI transfer timeout.");
 }
 
 /*
@@ -359,6 +366,8 @@ static const uint8_t s626_trimadrs[] = {
 static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 {
 	struct s626_private *devpriv = dev->private;
+	static const int timeout = 10000;
+	int i;
 
 	/* START THE SERIAL CLOCK RUNNING ------------- */
 
@@ -404,8 +413,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * Done by polling the DMAC enable flag; this flag is automatically
 	 * cleared when the transfer has finished.
 	 */
-	while (readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT)
-		;
+	for  (i = 0; i < timeout; i++) {
+		if (!(readl(devpriv->mmio + S626_P_MC1) & S626_MC1_A2OUT))
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "DMA transfer timeout.");
 
 	/* START THE OUTPUT STREAM TO THE TARGET DAC -------------------- */
 
@@ -425,8 +439,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * finished transferring the DAC's data DWORD from the output FIFO
 	 * to the output buffer register.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (readl(devpriv->mmio + S626_P_SSR) & S626_SSR_AF2_OUT)
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "TSL timeout waiting for slot 1 to execute.");
 
 	/*
 	 * Set up to trap execution at slot 0 when the TSL sequencer cycles
@@ -466,8 +485,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 		 * from 0xFF to 0x00, which slot 0 causes to happen by shifting
 		 * out/in on SD2 the 0x00 that is always referenced by slot 5.
 		 */
-		while (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
-			;
+		for (i = 0; i < timeout; i++) {
+			if (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
+				break;
+			udelay(1);
+		}
+		if (i == timeout)
+			comedi_error(dev, "TSL timeout waiting for slot 0 to execute.");
 	}
 	/*
 	 * Either (1) we were too late setting the slot 0 trap; the TSL
@@ -486,8 +510,13 @@ static void s626_send_dac(struct comedi_device *dev, uint32_t val)
 	 * the next DAC write.  This is detected when FB_BUFFER2 MSB changes
 	 * from 0x00 to 0xFF.
 	 */
-	while (!(readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000))
-		;
+	for (i = 0; i < timeout; i++) {
+		if (readl(devpriv->mmio + S626_P_FB_BUFFER2) & 0xff000000)
+			break;
+		udelay(1);
+	}
+	if (i == timeout)
+		comedi_error(dev, "TLS timeout waiting for slot 0 to execute.");
 }
 
 /*
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/