Skip to contentSkip to conversation

Staging: iio/dds: double locking bugs

PATCH In review Dan Carpenter 3 participants 3 messages Nov 13, 2010 – Nov 15, 2010

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Staging: iio/dds: double locking bugs

This is a static checker patch and I don't have this hardware.

This code is unusual because while I've often seen a double lock, this is the first time I've seen code that takes a lock 11 times in a row. I feel like I must have missed something. But I've looked very carefully I don't see any way the original code is correct. Does spi_sync() somehow release the lock in a way that I can't see? Even if it does, the locking would still be wrong.

---
This should be easy to test for someone with the hardware. The ad9910 driver should lock up on init.

drivers/staging/iio/dds/ad9951.c +0 -5
index bc3beff..57eddf6 100644
@@ -79,7 +79,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,
 
 	xfer.len = 2;
 	xfer.tx_buf = &config->arr[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -89,7 +88,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->ftw0[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -99,7 +97,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,
 
 	xfer.len = 3;
 	xfer.tx_buf = &config->ftw1[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -143,8 +140,6 @@ static void ad9951_init(struct ad9951_state *st)
 	cfr[2] = HSPD_SYNC;
 	cfr[3] = 0;
 
-	mutex_lock(&st->lock);
-
 	xfer.len = 4;
 	xfer.tx_buf = 𝔠
 
drivers/staging/iio/dds/ad9910.c +6 -20
index c59b4079..e8fb75c 100644
@@ -138,7 +138,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->ioupd[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -148,7 +147,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->ftw[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -158,7 +156,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 3;
 	xfer.tx_buf = &config->pow[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -168,7 +165,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->asf[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -178,7 +174,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->multc[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -188,7 +183,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 9;
 	xfer.tx_buf = &config->dig_rampl[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -198,7 +192,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 9;
 	xfer.tx_buf = &config->dig_ramps[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -208,7 +201,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->dig_rampr[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -218,25 +210,24 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep0[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep1[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep2[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -245,43 +236,42 @@ static ssize_t ad9910_set_parameter(struct device *dev,
 		goto error_ret;
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep3[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep4[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep5[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep6[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 9;
 	xfer.tx_buf = &config->sin_tonep7[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -326,8 +316,6 @@ static void ad9910_init(struct ad9910_state *st)
 	cfr[3] = TX_ENA | PDCLK_INV | PDCLK_ENB;
 	cfr[4] = PARA_ENA;
 
-	mutex_lock(&st->lock);
-
 	xfer.len = 5;
 	xfer.tx_buf = 𝔠
 
@@ -343,8 +331,6 @@ static void ad9910_init(struct ad9910_state *st)
 	cfr[3] = REFCLK_RST | REFCLK_BYP;
 	cfr[4] = 0;
 
-	mutex_lock(&st->lock);
-
 	xfer.len = 5;
 	xfer.tx_buf = 𝔠
 
drivers/staging/iio/dds/ad9852.c +1 -10
index 0a41d25..594fb6a 100644
@@ -86,7 +86,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 3;
 	xfer.tx_buf = &config->phajst1[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -96,7 +95,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 6;
 	xfer.tx_buf = &config->fretun1[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -106,7 +104,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 6;
 	xfer.tx_buf = &config->fretun2[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -116,7 +113,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 6;
 	xfer.tx_buf = &config->dltafre[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -126,7 +122,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->updtclk[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -136,7 +131,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 4;
 	xfer.tx_buf = &config->ramprat[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -146,7 +140,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 5;
 	xfer.tx_buf = &config->control[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -156,7 +149,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 3;
 	xfer.tx_buf = &config->outpskm[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
@@ -166,16 +158,15 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
 	xfer.len = 2;
 	xfer.tx_buf = &config->outpskr[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
 	ret = spi_sync(st->sdev, &msg);
 	if (ret)
 		goto error_ret;
+
 	xfer.len = 3;
 	xfer.tx_buf = &config->daccntl[0];
-	mutex_lock(&st->lock);
 
 	spi_message_init(&msg);
 	spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9832.c +0 -2
index a4bb048..e911893 100644
@@ -166,8 +166,6 @@ static void ad9832_init(struct ad9832_state *st)
 
 	config = 0x3 << 14;
 
-	mutex_lock(&st->lock);
-
 	xfer.len = 2;
 	xfer.tx_buf = &config;
 
maintainer Acked-by on v1 1/1
#1 lore raw

Dan Carpenter wrote on 2010-11-13:

Quoted 8 lines

This is a static checker patch and I don't have this hardware.

This code is unusual because while I've often seen a double lock, this is the first time I've seen code that takes a lock 11 times in a row. I feel like I must have missed something. But I've looked very carefully I don't see any way the original code is correct. Does spi_sync() somehow release the lock in a way that I can't see? Even if it does, the locking would still be wrong.

This is simply bad code, and likely committed untested.

Quoted 1 line
Quoted 258 lines

---
This should be easy to test for someone with the hardware. The ad9910 driver should lock up on init.

(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9951.c index bc3beff..57eddf6 100644 --- a/drivers/staging/iio/dds/ad9951.c +++ b/drivers/staging/iio/dds/ad9951.c @@ -79,7 +79,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,

      xfer.len = 2;
      xfer.tx_buf = &config->arr[0];
drivers/staging/iio/dds/ad9951.c +0 -5
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
@@ -89,7 +88,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->ftw0[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
@@ -99,7 +97,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,
 
      xfer.len = 3;
      xfer.tx_buf = &config->ftw1[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg); @@ -143,8
 +140,6 @@ static void ad9951_init(struct ad9951_state *st)   cfr[2] >  HSPD_SYNC;   cfr[3] = 0;
-     mutex_lock(&st->lock);
-
      xfer.len = 4;
      xfer.tx_buf = &cfr;
(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9910.c index c59b4079..e8fb75c 100644 --- a/drivers/staging/iio/dds/ad9910.c +++ b/drivers/staging/iio/dds/ad9910.c @@ -138,7 +138,6 @@ static ssize_t ad9910_set_parameter(struct device *dev,

      xfer.len = 5;
      xfer.tx_buf = &config->ioupd[0];
drivers/staging/iio/dds/ad9951.c +0 -2
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
@@ -148,7 +147,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->ftw[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -158,7 +156,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 3;
      xfer.tx_buf = &config->pow[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -168,7 +165,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->asf[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -178,7 +174,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->multc[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -188,7 +183,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 9;
      xfer.tx_buf = &config->dig_rampl[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -198,7 +192,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 9;
      xfer.tx_buf = &config->dig_ramps[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -208,7 +201,6 @@ static ssize_t ad9910_set_parameter(struct device
 *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->dig_rampr[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9910.c +0 -12
@@ -218,25 +210,24 @@ static ssize_t ad9910_set_parameter(struct
 device *dev,
 
      xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep0[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep1[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep2[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
@@ -245,43 +236,42 @@ static ssize_t ad9910_set_parameter(struct
 device *dev,
              goto error_ret;
      xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep3[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep4[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep5[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep6[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 9;
      xfer.tx_buf = &config->sin_tonep7[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg); @@ -326,8
 +316,6 @@ static void ad9910_init(struct ad9910_state *st)   cfr[3] >  TX_ENA | PDCLK_INV | PDCLK_ENB;      cfr[4] = PARA_ENA;
-     mutex_lock(&st->lock);
-
      xfer.len = 5;
      xfer.tx_buf = &cfr;
@@ -343,8 +331,6 @@ static void ad9910_init(struct ad9910_state *st)
      cfr[3] = REFCLK_RST | REFCLK_BYP;
      cfr[4] = 0;
-     mutex_lock(&st->lock);
-
      xfer.len = 5;
      xfer.tx_buf = &cfr;
(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9852.c index 0a41d25..594fb6a 100644 --- a/drivers/staging/iio/dds/ad9852.c +++ b/drivers/staging/iio/dds/ad9852.c @@ -86,7 +86,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,

      xfer.len = 3;
      xfer.tx_buf = &config->phajst1[0];
drivers/staging/iio/dds/ad9951.c +0 -3
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
@@ -96,7 +95,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,
 
      xfer.len = 6;
      xfer.tx_buf = &config->fretun1[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
@@ -106,7 +104,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
      xfer.len = 6;
      xfer.tx_buf = &config->fretun2[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9852.c +0 -1
@@ -116,7 +113,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
      xfer.len = 6;
      xfer.tx_buf = &config->dltafre[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9852.c +0 -1
@@ -126,7 +122,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->updtclk[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9852.c +0 -1
@@ -136,7 +131,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
      xfer.len = 4;
      xfer.tx_buf = &config->ramprat[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9852.c +0 -1
@@ -146,7 +140,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
      xfer.len = 5;
      xfer.tx_buf = &config->control[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9852.c +0 -1
@@ -156,7 +149,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
      xfer.len = 3;
      xfer.tx_buf = &config->outpskm[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
drivers/staging/iio/dds/ad9852.c +0 -2
@@ -166,16 +158,15 @@ static ssize_t ad9852_set_parameter(struct
 device *dev,
 
      xfer.len = 2;
      xfer.tx_buf = &config->outpskr[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);         spi_message_add_tail(&xfer, &msg);      ret >  spi_sync(st->sdev, &msg);    if (ret)                goto error_ret; +       xfer.len = 3;
      xfer.tx_buf = &config->daccntl[0];
-     mutex_lock(&st->lock);
 
      spi_message_init(&msg);
      spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9832.c index a4bb048..e911893 100644 --- a/drivers/staging/iio/dds/ad9832.c +++ b/drivers/staging/iio/dds/ad9832.c @@ -166,8 +166,6 @@ static void ad9832_init(struct ad9832_state *st)

      config = 0x3 << 14;

drivers/staging/iio/dds/ad9832.c static void ad9832_init(struct ad9832_state *st) · line 170

Quoted 4 lines
drivers/staging/iio/dds/ad9832.c +0 -2
-     mutex_lock(&st->lock);
-
      xfer.len = 2;
      xfer.tx_buf = &config;

Greetings,
Michael

--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif

Acked-by on v1 1/1
#2 lore raw
In reply to Dan Carpenter
Quoted 21 lines

-----Original Message-----
From: Hennerich, Michael
Sent: Monday, November 15, 2010 4:46 AM
To: Dan Carpenter; Greg Kroah-Hartman
Cc: Mike Frysinger; Jonathan Cameron; Cai, Cliff; [email protected]; [email protected]
Subject: RE: [patch] Staging: iio/dds: double locking bugs

Dan Carpenter wrote on 2010-11-13:

Quoted 3 lines

This is a static checker patch and I don't have this hardware.

This code is unusual because while I've often seen a double

lock, this

Quoted 5 lines

is the first time I've seen code that takes a lock 11 times in a row. I feel like I must have missed something. But I've looked very carefully I don't see any way the original code is correct. Does spi_sync() somehow release the lock in a way that I can't see? Even if it does, the locking would still be wrong.

Quoted 4 lines

This is simply bad code, and likely committed untested.

Quoted 1 line

Thanks

Cliff

Quoted 306 lines
Quoted 1 line

This should be easy to test for someone with the hardware.

The ad9910

Quoted 24 lines

driver should lock up on init.

(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9951.c index bc3beff..57eddf6 100644 --- a/drivers/staging/iio/dds/ad9951.c +++ b/drivers/staging/iio/dds/ad9951.c @@ -79,7 +79,6 @@ static ssize_t ad9951_set_parameter(struct device *dev,

     xfer.len = 2;
     xfer.tx_buf = &config->arr[0];
drivers/staging/iio/dds/ad9951.c +0 -2
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
@@ -89,7 +88,6 @@ static ssize_t ad9951_set_parameter(struct device
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->ftw0[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);

drivers/staging/iio/dds/ad9951.c static ssize_t ad9951_set_parameter(struct device · line 102

Quoted 7 lines
drivers/staging/iio/dds/ad9951.c +0 -1
@@ -99,7 +97,6 @@ static ssize_t ad9951_set_parameter(struct device
 *dev,
 
     xfer.len = 3;
     xfer.tx_buf = &config->ftw1[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); @@ -143,8

Quoted 7 lines

+140,6 @@ static void ad9951_init(struct ad9951_state *st) cfr[2] >> HSPD_SYNC; cfr[3] = 0;

drivers/staging/iio/dds/ad9951.c +0 -2
-    mutex_lock(&st->lock);
-
     xfer.len = 4;
     xfer.tx_buf = &cfr;
(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9910.c index c59b4079..e8fb75c

100644 ---

Quoted 2 lines

a/drivers/staging/iio/dds/ad9910.c +++ b/drivers/staging/iio/dds/ad9910.c @@ -138,7 +138,6 @@

static ssize_t

drivers/staging/iio/dds/ad9951.c static ssize_t

Quoted 9 lines

ad9910_set_parameter(struct device *dev,

     xfer.len = 5;
     xfer.tx_buf = &config->ioupd[0];
drivers/staging/iio/dds/ad9951.c +0 -1
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
@@ -148,7 +147,6 @@ static ssize_t

ad9910_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->ftw[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -158,7 +156,6 @@ static ssize_t

ad9910_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 3;
     xfer.tx_buf = &config->pow[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -168,7 +165,6 @@ static ssize_t

ad9910_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->asf[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -178,7 +174,6 @@ static ssize_t

ad9910_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->multc[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -188,7 +183,6 @@ static ssize_t

ad9910_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 9;
     xfer.tx_buf = &config->dig_rampl[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -198,7 +192,6 @@ static ssize_t

ad9910_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 9;
     xfer.tx_buf = &config->dig_ramps[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -208,7 +201,6 @@ static ssize_t

ad9910_set_parameter(struct device

Quoted 9 lines
drivers/staging/iio/dds/ad9910.c +0 -1
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->dig_rampr[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct · line 215

Quoted 7 lines
drivers/staging/iio/dds/ad9910.c +0 -1
@@ -218,25 +210,24 @@ static ssize_t ad9910_set_parameter(struct
 device *dev,
 
     xfer.len = 9;
     xfer.tx_buf = &config->sin_tonep0[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 9;

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct device *dev, · line 223

Quoted 4 lines
drivers/staging/iio/dds/ad9910.c +0 -1
     xfer.tx_buf = &config->sin_tonep1[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 9;

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct · line 241

Quoted 13 lines
drivers/staging/iio/dds/ad9910.c +0 -2
     xfer.tx_buf = &config->sin_tonep2[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
@@ -245,43 +236,42 @@ static ssize_t ad9910_set_parameter(struct
 device *dev,
             goto error_ret;
     xfer.len = 9;
     xfer.tx_buf = &config->sin_tonep3[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 9;

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct device *dev, · line 249

Quoted 4 lines
drivers/staging/iio/dds/ad9910.c +0 -1
     xfer.tx_buf = &config->sin_tonep4[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 9;

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct device *dev, · line 258

Quoted 4 lines
drivers/staging/iio/dds/ad9910.c +0 -1
     xfer.tx_buf = &config->sin_tonep5[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 9;

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct device *dev, · line 267

Quoted 4 lines
drivers/staging/iio/dds/ad9910.c +0 -1
     xfer.tx_buf = &config->sin_tonep6[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 9;

drivers/staging/iio/dds/ad9910.c static ssize_t ad9910_set_parameter(struct device *dev, · line 276

Quoted 4 lines
drivers/staging/iio/dds/ad9910.c +0 -1
     xfer.tx_buf = &config->sin_tonep7[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); @@ -326,8

Review static ssize_t

Quoted 34 lines
drivers/staging/iio/dds/ad9910.c +0 -4
 +316,6 @@ static void ad9910_init(struct ad9910_state *st)  cfr[3] >>  TX_ENA | PDCLK_INV | PDCLK_ENB;     cfr[4] = PARA_ENA;
-    mutex_lock(&st->lock);
-
     xfer.len = 5;
     xfer.tx_buf = &cfr;
@@ -343,8 +331,6 @@ static void ad9910_init(struct ad9910_state *st)
     cfr[3] = REFCLK_RST | REFCLK_BYP;
     cfr[4] = 0;
-    mutex_lock(&st->lock);
-
     xfer.len = 5;
     xfer.tx_buf = &cfr;
(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9852.c index 0a41d25..594fb6a 100644 --- a/drivers/staging/iio/dds/ad9852.c +++ b/drivers/staging/iio/dds/ad9852.c @@ -86,7 +86,6 @@ static ssize_t ad9852_set_parameter(struct device *dev,

     xfer.len = 3;
     xfer.tx_buf = &config->phajst1[0];
drivers/staging/iio/dds/ad9951.c +0 -2
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
@@ -96,7 +95,6 @@ static ssize_t ad9852_set_parameter(struct device
 *dev,
 
     xfer.len = 6;
     xfer.tx_buf = &config->fretun1[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -106,7 +104,6 @@ static ssize_t

ad9852_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9852.c +0 -1
 *dev,
 
     xfer.len = 6;
     xfer.tx_buf = &config->fretun2[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -116,7 +113,6 @@ static ssize_t

ad9852_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9852.c +0 -1
 *dev,
 
     xfer.len = 6;
     xfer.tx_buf = &config->dltafre[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -126,7 +122,6 @@ static ssize_t

ad9852_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9852.c +0 -1
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->updtclk[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -136,7 +131,6 @@ static ssize_t

ad9852_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9852.c +0 -1
 *dev,
 
     xfer.len = 4;
     xfer.tx_buf = &config->ramprat[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -146,7 +140,6 @@ static ssize_t

ad9852_set_parameter(struct device

Review static ssize_t

Quoted 9 lines
drivers/staging/iio/dds/ad9852.c +0 -1
 *dev,
 
     xfer.len = 5;
     xfer.tx_buf = &config->control[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0
@@ -156,7 +149,6 @@ static ssize_t

ad9852_set_parameter(struct device

Quoted 9 lines
drivers/staging/iio/dds/ad9852.c +0 -1
 *dev,
 
     xfer.len = 3;
     xfer.tx_buf = &config->outpskm[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);

drivers/staging/iio/dds/ad9852.c static ssize_t ad9852_set_parameter(struct · line 163

Quoted 7 lines
drivers/staging/iio/dds/ad9852.c +0 -1
@@ -166,16 +158,15 @@ static ssize_t ad9852_set_parameter(struct
 device *dev,
 
     xfer.len = 2;
     xfer.tx_buf = &config->outpskr[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);

spi_message_add_tail(&xfer, &msg); ret >> spi_sync(st->sdev, &msg); if (ret) goto
error_ret; + xfer.len = 3;

Quoted 12 lines
drivers/staging/iio/dds/ad9852.c +0 -1
     xfer.tx_buf = &config->daccntl[0];
-    mutex_lock(&st->lock);
 
     spi_message_init(&msg);
     spi_message_add_tail(&xfer, &msg);
(quoted diff) +0 -0

b/drivers/staging/iio/dds/ad9832.c index a4bb048..e911893 100644 --- a/drivers/staging/iio/dds/ad9832.c +++ b/drivers/staging/iio/dds/ad9832.c @@ -166,8 +166,6 @@ static void ad9832_init(struct ad9832_state *st)

     config = 0x3 << 14;

drivers/staging/iio/dds/ad9832.c static void ad9832_init(struct ad9832_state *st) · line 170

Quoted 4 lines
drivers/staging/iio/dds/ad9832.c +0 -2
-    mutex_lock(&st->lock);
-
     xfer.len = 2;
     xfer.tx_buf = &config;

Greetings,
Michael

--
Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif