* [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()
@ 2017-11-02 19:40 SF Markus Elfring
2017-11-03 14:00 ` Ian Abbott
0 siblings, 1 reply; 5+ messages in thread
From: SF Markus Elfring @ 2017-11-02 19:40 UTC (permalink / raw)
To: devel, Greg Kroah-Hartman, H Hartley Sweeten, Ian Abbott
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 2 Nov 2017 20:30:31 +0100
* Add a jump target so that a call of the function "mutex_unlock" is stored
only twice in this function implementation.
* Replace five calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/staging/comedi/drivers/usbduxfast.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
index 608403c7586b..e5884faf7275 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -777,8 +777,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
if (devpriv->ai_cmd_running) {
dev_err(dev->class_dev,
"ai_insn_read not possible, async cmd is running\n");
- mutex_unlock(&devpriv->mut);
- return -EBUSY;
+ ret = -EBUSY;
+ goto unlock;
}
/* set command for the first channel */
@@ -798,10 +798,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
usbduxfast_cmd_data(dev, 6, 0x01, 0x00, rngmask, 0x00);
ret = usbduxfast_send_cmd(dev, SENDADCOMMANDS);
- if (ret < 0) {
- mutex_unlock(&devpriv->mut);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
for (i = 0; i < PACKETS_TO_IGNORE; i++) {
ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, BULKINEP),
@@ -809,8 +807,7 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
&actual_length, 10000);
if (ret < 0) {
dev_err(dev->class_dev, "insn timeout, no data\n");
- mutex_unlock(&devpriv->mut);
- return ret;
+ goto unlock;
}
}
@@ -820,14 +817,13 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
&actual_length, 10000);
if (ret < 0) {
dev_err(dev->class_dev, "insn data error: %d\n", ret);
- mutex_unlock(&devpriv->mut);
- return ret;
+ goto unlock;
}
n = actual_length / sizeof(u16);
if ((n % 16) != 0) {
dev_err(dev->class_dev, "insn data packet corrupted\n");
- mutex_unlock(&devpriv->mut);
- return -EINVAL;
+ ret = -EINVAL;
+ goto unlock;
}
for (j = chan; (j < n) && (i < insn->n); j = j + 16) {
data[i] = ((u16 *)(devpriv->inbuf))[j];
@@ -838,6 +834,10 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
mutex_unlock(&devpriv->mut);
return insn->n;
+
+unlock:
+ mutex_unlock(&devpriv->mut);
+ return ret;
}
static int usbduxfast_upload_firmware(struct comedi_device *dev,
--
2.15.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()
2017-11-02 19:40 [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read() SF Markus Elfring
@ 2017-11-03 14:00 ` Ian Abbott
2017-11-03 14:14 ` Greg Kroah-Hartman
2017-11-03 16:39 ` [PATCH] " SF Markus Elfring
0 siblings, 2 replies; 5+ messages in thread
From: Ian Abbott @ 2017-11-03 14:00 UTC (permalink / raw)
To: SF Markus Elfring, devel, Greg Kroah-Hartman, H Hartley Sweeten
Cc: LKML, kernel-janitors
On 02/11/17 19:40, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 2 Nov 2017 20:30:31 +0100
>
> * Add a jump target so that a call of the function "mutex_unlock" is stored
> only twice in this function implementation.
>
> * Replace five calls by goto statements.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/staging/comedi/drivers/usbduxfast.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
> index 608403c7586b..e5884faf7275 100644
> --- a/drivers/staging/comedi/drivers/usbduxfast.c
> +++ b/drivers/staging/comedi/drivers/usbduxfast.c
> @@ -777,8 +777,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> if (devpriv->ai_cmd_running) {
> dev_err(dev->class_dev,
> "ai_insn_read not possible, async cmd is running\n");
> - mutex_unlock(&devpriv->mut);
> - return -EBUSY;
> + ret = -EBUSY;
> + goto unlock;
> }
>
> /* set command for the first channel */
> @@ -798,10 +798,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> usbduxfast_cmd_data(dev, 6, 0x01, 0x00, rngmask, 0x00);
>
> ret = usbduxfast_send_cmd(dev, SENDADCOMMANDS);
> - if (ret < 0) {
> - mutex_unlock(&devpriv->mut);
> - return ret;
> - }
> + if (ret < 0)
> + goto unlock;
>
> for (i = 0; i < PACKETS_TO_IGNORE; i++) {
> ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, BULKINEP),
> @@ -809,8 +807,7 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> &actual_length, 10000);
> if (ret < 0) {
> dev_err(dev->class_dev, "insn timeout, no data\n");
> - mutex_unlock(&devpriv->mut);
> - return ret;
> + goto unlock;
> }
> }
>
> @@ -820,14 +817,13 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> &actual_length, 10000);
> if (ret < 0) {
> dev_err(dev->class_dev, "insn data error: %d\n", ret);
> - mutex_unlock(&devpriv->mut);
> - return ret;
> + goto unlock;
> }
> n = actual_length / sizeof(u16);
> if ((n % 16) != 0) {
> dev_err(dev->class_dev, "insn data packet corrupted\n");
> - mutex_unlock(&devpriv->mut);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto unlock;
> }
> for (j = chan; (j < n) && (i < insn->n); j = j + 16) {
> data[i] = ((u16 *)(devpriv->inbuf))[j];
> @@ -838,6 +834,10 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> mutex_unlock(&devpriv->mut);
>
> return insn->n;
Minor niggle: You could also remove that call to mutex_unlock() by
replacing the above three lines with:
ret = insn->n;
which will fall through to the 'unlock:' label below.
> +
> +unlock:
> + mutex_unlock(&devpriv->mut);
> + return ret;
> }
>
> static int usbduxfast_upload_firmware(struct comedi_device *dev,
>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=-
-=( Web: http://www.mev.co.uk/ )=-
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()
2017-11-03 14:00 ` Ian Abbott
@ 2017-11-03 14:14 ` Greg Kroah-Hartman
2017-11-03 17:07 ` SF Markus Elfring
2017-11-03 16:39 ` [PATCH] " SF Markus Elfring
1 sibling, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2017-11-03 14:14 UTC (permalink / raw)
To: Ian Abbott
Cc: SF Markus Elfring, devel, H Hartley Sweeten, LKML, kernel-janitors
On Fri, Nov 03, 2017 at 02:00:18PM +0000, Ian Abbott wrote:
> On 02/11/17 19:40, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Thu, 2 Nov 2017 20:30:31 +0100
> >
> > * Add a jump target so that a call of the function "mutex_unlock" is stored
> > only twice in this function implementation.
> >
> > * Replace five calls by goto statements.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> > drivers/staging/comedi/drivers/usbduxfast.c | 24 ++++++++++++------------
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
> > index 608403c7586b..e5884faf7275 100644
> > --- a/drivers/staging/comedi/drivers/usbduxfast.c
> > +++ b/drivers/staging/comedi/drivers/usbduxfast.c
> > @@ -777,8 +777,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> > if (devpriv->ai_cmd_running) {
> > dev_err(dev->class_dev,
> > "ai_insn_read not possible, async cmd is running\n");
> > - mutex_unlock(&devpriv->mut);
> > - return -EBUSY;
> > + ret = -EBUSY;
> > + goto unlock;
> > }
> > /* set command for the first channel */
> > @@ -798,10 +798,8 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> > usbduxfast_cmd_data(dev, 6, 0x01, 0x00, rngmask, 0x00);
> > ret = usbduxfast_send_cmd(dev, SENDADCOMMANDS);
> > - if (ret < 0) {
> > - mutex_unlock(&devpriv->mut);
> > - return ret;
> > - }
> > + if (ret < 0)
> > + goto unlock;
> > for (i = 0; i < PACKETS_TO_IGNORE; i++) {
> > ret = usb_bulk_msg(usb, usb_rcvbulkpipe(usb, BULKINEP),
> > @@ -809,8 +807,7 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> > &actual_length, 10000);
> > if (ret < 0) {
> > dev_err(dev->class_dev, "insn timeout, no data\n");
> > - mutex_unlock(&devpriv->mut);
> > - return ret;
> > + goto unlock;
> > }
> > }
> > @@ -820,14 +817,13 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> > &actual_length, 10000);
> > if (ret < 0) {
> > dev_err(dev->class_dev, "insn data error: %d\n", ret);
> > - mutex_unlock(&devpriv->mut);
> > - return ret;
> > + goto unlock;
> > }
> > n = actual_length / sizeof(u16);
> > if ((n % 16) != 0) {
> > dev_err(dev->class_dev, "insn data packet corrupted\n");
> > - mutex_unlock(&devpriv->mut);
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto unlock;
> > }
> > for (j = chan; (j < n) && (i < insn->n); j = j + 16) {
> > data[i] = ((u16 *)(devpriv->inbuf))[j];
> > @@ -838,6 +834,10 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
> > mutex_unlock(&devpriv->mut);
> > return insn->n;
>
> Minor niggle: You could also remove that call to mutex_unlock() by replacing
> the above three lines with:
>
> ret = insn->n;
>
> which will fall through to the 'unlock:' label below.
FYI, you are responding to someone who is on my blacklist and I never
accept patches from. I wouldn't waste my time reviewing any patches
from them, sorry.
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()
2017-11-03 14:00 ` Ian Abbott
2017-11-03 14:14 ` Greg Kroah-Hartman
@ 2017-11-03 16:39 ` SF Markus Elfring
1 sibling, 0 replies; 5+ messages in thread
From: SF Markus Elfring @ 2017-11-03 16:39 UTC (permalink / raw)
To: Ian Abbott, devel
Cc: Greg Kroah-Hartman, H Hartley Sweeten, LKML, kernel-janitors
>> @@ -838,6 +834,10 @@ static int usbduxfast_ai_insn_read(struct comedi_device *dev,
>> mutex_unlock(&devpriv->mut);
>> return insn->n;
>
> Minor niggle: You could also remove that call to mutex_unlock() by replacing the above three lines with:
>
> ret = insn->n;
>
> which will fall through to the 'unlock:' label below.
Thanks for your suggestion.
Such a software refactoring is also possible if a corresponding
consensus could be achieved.
* Can such a change mean that the lock scope will be extended
for both use cases (successful and failed function execution)?
* How much does this implementation matter for you?
* Would you like to achieve a small reduction of the object code there?
* How do you think about consequences from special communication settings
by a well-known maintainer for my update suggestions?
Regards,
Markus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read()
2017-11-03 14:14 ` Greg Kroah-Hartman
@ 2017-11-03 17:07 ` SF Markus Elfring
0 siblings, 0 replies; 5+ messages in thread
From: SF Markus Elfring @ 2017-11-03 17:07 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel
Cc: Ian Abbott, H Hartley Sweeten, LKML, kernel-janitors
> FYI, you are responding to someone who is on my blacklist
I am curious if this communication setting will ever be adjusted.
> and I never accept patches from.
The history shows that our collaboration style changed over time.
I got a few update suggestions integrated (also by you) because
other contributors found them good enough to repeat them.
Regards,
Markus
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-03 17:07 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-02 19:40 [PATCH] staging: comedi: usbduxfast: Improve unlocking of a mutex in usbduxfast_ai_insn_read() SF Markus Elfring
2017-11-03 14:00 ` Ian Abbott
2017-11-03 14:14 ` Greg Kroah-Hartman
2017-11-03 17:07 ` SF Markus Elfring
2017-11-03 16:39 ` [PATCH] " SF Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).