[PATCH] Staging: comedi: Use rwsemaphore

[PATCH] Staging: comedi: Use rwsemaphore

[Ksenija Stanojevic]

Functions that use semaphore for locking are divided into read/write
functions, so use locking mechanism that provides similar semantics.

Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
---
 drivers/staging/comedi/drivers/vmk80xx.c | 54 ++++++++++++++++----------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/comedi/drivers/vmk80xx.c b/drivers/staging/comedi/drivers/vmk80xx.c
index 8c7393e..f08976c 100644
--- a/drivers/staging/comedi/drivers/vmk80xx.c
+++ b/drivers/staging/comedi/drivers/vmk80xx.c
@@ -152,7 +152,7 @@ static const struct vmk80xx_board vmk80xx_boardinfo[] = {
 struct vmk80xx_private {
 	struct usb_endpoint_descriptor *ep_rx;
 	struct usb_endpoint_descriptor *ep_tx;
-	struct semaphore limit_sem;
+	struct rw_semaphore limit_sem;
 	unsigned char *usb_rx_buf;
 	unsigned char *usb_tx_buf;
 	enum vmk80xx_model model;
@@ -249,7 +249,7 @@ static int vmk80xx_ai_insn_read(struct comedi_device *dev,
 	int reg[2];
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_read(&devpriv->limit_sem);
 	chan = CR_CHAN(insn->chanspec);
 
 	switch (devpriv->model) {
@@ -282,7 +282,7 @@ static int vmk80xx_ai_insn_read(struct comedi_device *dev,
 		    devpriv->usb_rx_buf[reg[1]];
 	}
 
-	up(&devpriv->limit_sem);
+	up_read(&devpriv->limit_sem);
 
 	return n;
 }
@@ -298,7 +298,7 @@ static int vmk80xx_ao_insn_write(struct comedi_device *dev,
 	int reg;
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 	chan = CR_CHAN(insn->chanspec);
 
 	switch (devpriv->model) {
@@ -323,7 +323,7 @@ static int vmk80xx_ao_insn_write(struct comedi_device *dev,
 			break;
 	}
 
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 
 	return n;
 }
@@ -338,7 +338,7 @@ static int vmk80xx_ao_insn_read(struct comedi_device *dev,
 	int reg;
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_read(&devpriv->limit_sem);
 	chan = CR_CHAN(insn->chanspec);
 
 	reg = VMK8061_AO_REG - 1;
@@ -352,7 +352,7 @@ static int vmk80xx_ao_insn_read(struct comedi_device *dev,
 		data[n] = devpriv->usb_rx_buf[reg + chan];
 	}
 
-	up(&devpriv->limit_sem);
+	up_read(&devpriv->limit_sem);
 
 	return n;
 }
@@ -367,7 +367,7 @@ static int vmk80xx_di_insn_bits(struct comedi_device *dev,
 	int reg;
 	int retval;
 
-	down(&devpriv->limit_sem);
+	down_read(&devpriv->limit_sem);
 
 	rx_buf = devpriv->usb_rx_buf;
 
@@ -391,7 +391,7 @@ static int vmk80xx_di_insn_bits(struct comedi_device *dev,
 		retval = 2;
 	}
 
-	up(&devpriv->limit_sem);
+	up_read(&devpriv->limit_sem);
 
 	return retval;
 }
@@ -415,7 +415,7 @@ static int vmk80xx_do_insn_bits(struct comedi_device *dev,
 		cmd = VMK8055_CMD_WRT_AD;
 	}
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 
 	if (comedi_dio_update_state(s, data)) {
 		tx_buf[reg] = s->state;
@@ -435,7 +435,7 @@ static int vmk80xx_do_insn_bits(struct comedi_device *dev,
 	}
 
 out:
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 
 	return ret ? ret : insn->n;
 }
@@ -450,7 +450,7 @@ static int vmk80xx_cnt_insn_read(struct comedi_device *dev,
 	int reg[2];
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_read(&devpriv->limit_sem);
 	chan = CR_CHAN(insn->chanspec);
 
 	switch (devpriv->model) {
@@ -479,7 +479,7 @@ static int vmk80xx_cnt_insn_read(struct comedi_device *dev,
 			    + 256 * devpriv->usb_rx_buf[reg[1] * 2 + 2];
 	}
 
-	up(&devpriv->limit_sem);
+	up_read(&devpriv->limit_sem);
 
 	return n;
 }
@@ -495,7 +495,7 @@ static int vmk80xx_cnt_insn_config(struct comedi_device *dev,
 	int reg;
 	int ret;
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 	switch (data[0]) {
 	case INSN_CONFIG_RESET:
 		if (devpriv->model == VMK8055_MODEL) {
@@ -516,7 +516,7 @@ static int vmk80xx_cnt_insn_config(struct comedi_device *dev,
 		ret = -EINVAL;
 		break;
 	}
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 
 	return ret ? ret : insn->n;
 }
@@ -533,7 +533,7 @@ static int vmk80xx_cnt_insn_write(struct comedi_device *dev,
 	int cmd;
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 	chan = CR_CHAN(insn->chanspec);
 
 	if (!chan)
@@ -560,7 +560,7 @@ static int vmk80xx_cnt_insn_write(struct comedi_device *dev,
 			break;
 	}
 
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 
 	return n;
 }
@@ -576,7 +576,7 @@ static int vmk80xx_pwm_insn_read(struct comedi_device *dev,
 	int reg[2];
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_read(&devpriv->limit_sem);
 
 	tx_buf = devpriv->usb_tx_buf;
 	rx_buf = devpriv->usb_rx_buf;
@@ -593,7 +593,7 @@ static int vmk80xx_pwm_insn_read(struct comedi_device *dev,
 		data[n] = rx_buf[reg[0]] + 4 * rx_buf[reg[1]];
 	}
 
-	up(&devpriv->limit_sem);
+	up_read(&devpriv->limit_sem);
 
 	return n;
 }
@@ -609,7 +609,7 @@ static int vmk80xx_pwm_insn_write(struct comedi_device *dev,
 	int cmd;
 	int n;
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 
 	tx_buf = devpriv->usb_tx_buf;
 
@@ -639,7 +639,7 @@ static int vmk80xx_pwm_insn_write(struct comedi_device *dev,
 			break;
 	}
 
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 
 	return n;
 }
@@ -707,7 +707,7 @@ static int vmk80xx_init_subdevices(struct comedi_device *dev)
 	int n_subd;
 	int ret;
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 
 	if (devpriv->model == VMK8055_MODEL)
 		n_subd = 5;
@@ -715,7 +715,7 @@ static int vmk80xx_init_subdevices(struct comedi_device *dev)
 		n_subd = 6;
 	ret = comedi_alloc_subdevices(dev, n_subd);
 	if (ret) {
-		up(&devpriv->limit_sem);
+		up_write(&devpriv->limit_sem);
 		return ret;
 	}
 
@@ -783,7 +783,7 @@ static int vmk80xx_init_subdevices(struct comedi_device *dev)
 		s->insn_write	= vmk80xx_pwm_insn_write;
 	}
 
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 
 	return 0;
 }
@@ -817,7 +817,7 @@ static int vmk80xx_auto_attach(struct comedi_device *dev,
 	if (ret)
 		return ret;
 
-	sema_init(&devpriv->limit_sem, 8);
+	init_rwsem(&devpriv->limit_sem);
 
 	usb_set_intfdata(intf, devpriv);
 
@@ -835,14 +835,14 @@ static void vmk80xx_detach(struct comedi_device *dev)
 	if (!devpriv)
 		return;
 
-	down(&devpriv->limit_sem);
+	down_write(&devpriv->limit_sem);
 
 	usb_set_intfdata(intf, NULL);
 
 	kfree(devpriv->usb_rx_buf);
 	kfree(devpriv->usb_tx_buf);
 
-	up(&devpriv->limit_sem);
+	up_write(&devpriv->limit_sem);
 }
 
 static struct comedi_driver vmk80xx_driver = {
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] Staging: comedi: Use rwsemaphore

[Greg KH]

On Tue, Oct 27, 2015 at 09:23:16PM -0700, Ksenija Stanojevic wrote:
> Functions that use semaphore for locking are divided into read/write
> functions, so use locking mechanism that provides similar semantics.
> 
> Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>

Unless you can measure the speed differences by switching to such a
lock, never switch to such a lock as they are much more complex and can
actually cause things to go _slower_.

So I can't take this patch unless you have measureable benchmark
numbers.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: comedi: Use rwsemaphore

[Arnd Bergmann]

On Wednesday 28 October 2015 13:46:25 Greg KH wrote:
> On Tue, Oct 27, 2015 at 09:23:16PM -0700, Ksenija Stanojevic wrote:
> > Functions that use semaphore for locking are divided into read/write
> > functions, so use locking mechanism that provides similar semantics.
> > 
> > Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> 
> Unless you can measure the speed differences by switching to such a
> lock, never switch to such a lock as they are much more complex and can
> actually cause things to go _slower_.
> 
> So I can't take this patch unless you have measureable benchmark
> numbers.
> 

Agreed. However, changing this driver to use 'struct mutex' would
be an obvious improvement and should be safe as far as I can tell.

	Arnd

Re: [Outreachy kernel] [PATCH] Staging: comedi: Use rwsemaphore

[Greg KH]

On Thu, Oct 29, 2015 at 11:20:46PM +0100, Arnd Bergmann wrote:
> On Wednesday 28 October 2015 13:46:25 Greg KH wrote:
> > On Tue, Oct 27, 2015 at 09:23:16PM -0700, Ksenija Stanojevic wrote:
> > > Functions that use semaphore for locking are divided into read/write
> > > functions, so use locking mechanism that provides similar semantics.
> > > 
> > > Signed-off-by: Ksenija Stanojevic <ksenija.stanojevic@gmail.com>
> > 
> > Unless you can measure the speed differences by switching to such a
> > lock, never switch to such a lock as they are much more complex and can
> > actually cause things to go _slower_.
> > 
> > So I can't take this patch unless you have measureable benchmark
> > numbers.
> > 
> 
> Agreed. However, changing this driver to use 'struct mutex' would
> be an obvious improvement and should be safe as far as I can tell.

Yes, that's fine to do.

greg k-h