2007-12-06 02:31:20

by Daniel Walker

[permalink] [raw]
Subject: [PATCH 3/3] printer port driver: semaphore to mutex

The port_mutex is actually a semaphore, so easily converted to
a struct mutex.

Signed-off-by: Daniel Walker <[email protected]>

---
drivers/char/lp.c | 11 ++++++-----
include/linux/lp.h | 2 +-
2 files changed, 7 insertions(+), 6 deletions(-)

Index: linux-2.6.23/drivers/char/lp.c
===================================================================
--- linux-2.6.23.orig/drivers/char/lp.c
+++ linux-2.6.23/drivers/char/lp.c
@@ -126,6 +126,7 @@
#include <linux/device.h>
#include <linux/wait.h>
#include <linux/jiffies.h>
+#include <linux/mutex.h>

#include <linux/parport.h>
#undef LP_STATS
@@ -312,7 +313,7 @@ static ssize_t lp_write(struct file * fi
if (copy_size > LP_BUFFER_SIZE)
copy_size = LP_BUFFER_SIZE;

- if (down_interruptible (&lp_table[minor].port_mutex))
+ if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
return -EINTR;

if (copy_from_user (kbuf, buf, copy_size)) {
@@ -399,7 +400,7 @@ static ssize_t lp_write(struct file * fi
lp_release_parport (&lp_table[minor]);
}
out_unlock:
- up (&lp_table[minor].port_mutex);
+ mutex_unlock(&lp_table[minor].port_mutex);

return retv;
}
@@ -421,7 +422,7 @@ static ssize_t lp_read(struct file * fil
if (count > LP_BUFFER_SIZE)
count = LP_BUFFER_SIZE;

- if (down_interruptible (&lp_table[minor].port_mutex))
+ if (mutex_lock_interruptible(&lp_table[minor].port_mutex))
return -EINTR;

lp_claim_parport_or_block (&lp_table[minor]);
@@ -479,7 +480,7 @@ static ssize_t lp_read(struct file * fil
if (retval > 0 && copy_to_user (buf, kbuf, retval))
retval = -EFAULT;

- up (&lp_table[minor].port_mutex);
+ mutex_unlock(&lp_table[minor].port_mutex);

return retval;
}
@@ -888,7 +889,7 @@ static int __init lp_init (void)
lp_table[i].last_error = 0;
init_waitqueue_head (&lp_table[i].waitq);
init_waitqueue_head (&lp_table[i].dataq);
- init_MUTEX (&lp_table[i].port_mutex);
+ mutex_init(&lp_table[i].port_mutex);
lp_table[i].timeout = 10 * HZ;
}

Index: linux-2.6.23/include/linux/lp.h
===================================================================
--- linux-2.6.23.orig/include/linux/lp.h
+++ linux-2.6.23/include/linux/lp.h
@@ -145,7 +145,7 @@ struct lp_struct {
#endif
wait_queue_head_t waitq;
unsigned int last_error;
- struct semaphore port_mutex;
+ struct mutex port_mutex;
wait_queue_head_t dataq;
long timeout;
unsigned int best_mode;

--


2007-12-06 10:24:18

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex


* Daniel Walker <[email protected]> wrote:

> The port_mutex is actually a semaphore, so easily converted to a
> struct mutex.
>
> Signed-off-by: Daniel Walker <[email protected]>

Acked-by: Ingo Molnar <[email protected]>

cool. How far away are we from being able to remove all the semaphore
code? :-)

Ingo

2007-12-06 16:47:05

by Daniel Walker

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> * Daniel Walker <[email protected]> wrote:
>
> > The port_mutex is actually a semaphore, so easily converted to a
> > struct mutex.
> >
> > Signed-off-by: Daniel Walker <[email protected]>
>
> Acked-by: Ingo Molnar <[email protected]>
>
> cool. How far away are we from being able to remove all the semaphore
> code? :-)

I wish my 7 patches made a dent, but it's hasn't done much. ;(

I would guess at least a week just to mop up the relatively easy ones..
I've got 12 in my queue, and there still ~50 hopefully trivial ones
still to be looked at.. Then another ~30 more difficult ones (that use
init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..

Daniel

2007-12-06 20:02:11

by Remy Bohmer

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

Daniel,

FYI: I am working on the conversion of the 2 sem->mutex in kernel/printk.c

Remy

2007/12/6, Daniel Walker <[email protected]>:
> On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> > * Daniel Walker <[email protected]> wrote:
> >
> > > The port_mutex is actually a semaphore, so easily converted to a
> > > struct mutex.
> > >
> > > Signed-off-by: Daniel Walker <[email protected]>
> >
> > Acked-by: Ingo Molnar <[email protected]>
> >
> > cool. How far away are we from being able to remove all the semaphore
> > code? :-)
>
> I wish my 7 patches made a dent, but it's hasn't done much. ;(
>
> I would guess at least a week just to mop up the relatively easy ones..
> I've got 12 in my queue, and there still ~50 hopefully trivial ones
> still to be looked at.. Then another ~30 more difficult ones (that use
> init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
>
> Daniel
>
>

2007-12-06 20:05:33

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

El Thu, Dec 06, 2007 at 08:34:06AM -0800 Daniel Walker ha dit:

> On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> > * Daniel Walker <[email protected]> wrote:
> >
> > > The port_mutex is actually a semaphore, so easily converted to a
> > > struct mutex.
> > >
> > > Signed-off-by: Daniel Walker <[email protected]>
> >
> > Acked-by: Ingo Molnar <[email protected]>
> >
> > cool. How far away are we from being able to remove all the semaphore
> > code? :-)
>
> I wish my 7 patches made a dent, but it's hasn't done much. ;(
>
> I would guess at least a week just to mop up the relatively easy ones..
> I've got 12 in my queue, and there still ~50 hopefully trivial ones
> still to be looked at.. Then another ~30 more difficult ones (that use
> init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..

I've also more patches of this type on my list, though i work in a
slower pace

--
Matthias Kaehlcke
Linux Application Developer
Barcelona

You must have a plan. If you don't have a plan,
you'll become part of somebody else's plan
.''`.
using free software / Debian GNU/Linux | http://debian.org : :' :
`. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-

2007-12-06 20:12:18

by Remy Bohmer

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

Hello Matthias,

Which do you have exactly on your list? (good to know, it prevents
double work...)

Remy

2007/12/6, Matthias Kaehlcke <[email protected]>:
> El Thu, Dec 06, 2007 at 08:34:06AM -0800 Daniel Walker ha dit:
>
> > On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> > > * Daniel Walker <[email protected]> wrote:
> > >
> > > > The port_mutex is actually a semaphore, so easily converted to a
> > > > struct mutex.
> > > >
> > > > Signed-off-by: Daniel Walker <[email protected]>
> > >
> > > Acked-by: Ingo Molnar <[email protected]>
> > >
> > > cool. How far away are we from being able to remove all the semaphore
> > > code? :-)
> >
> > I wish my 7 patches made a dent, but it's hasn't done much. ;(
> >
> > I would guess at least a week just to mop up the relatively easy ones..
> > I've got 12 in my queue, and there still ~50 hopefully trivial ones
> > still to be looked at.. Then another ~30 more difficult ones (that use
> > init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
>
> I've also more patches of this type on my list, though i work in a
> slower pace
>
> --
> Matthias Kaehlcke
> Linux Application Developer
> Barcelona
>
> You must have a plan. If you don't have a plan,
> you'll become part of somebody else's plan
> .''`.
> using free software / Debian GNU/Linux | http://debian.org : :' :
> `. `'`
> gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-
>

2007-12-06 20:25:27

by Daniel Walker

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

On Thu, 2007-12-06 at 21:01 +0100, Remy Bohmer wrote:
> Daniel,
>
> FYI: I am working on the conversion of the 2 sem->mutex in kernel/printk.c

I looked at the console_sem , but i was going to leave that as last..

The problem with the console_sem is that it can get locked from
interrupt context, which is discouraged with mutex types.. I think it
will be complicated to convert..

Daniel

2007-12-06 20:36:26

by Daniel Walker

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

On Thu, 2007-12-06 at 21:12 +0100, Remy Bohmer wrote:
> Hello Matthias,
>
> Which do you have exactly on your list? (good to know, it prevents
> double work...)

Here's my list so far (in no particular order).. Most aren't tested
fully yet.. I'll try to post them someplace eventually (later today
maybe)

plip-sem-to-complete.patch
linux-2.6.23/drivers/net/plip.c
spi-semaphore-to-mutex.patch
linux-2.6.23/drivers/spi/spi.c
au1000-semaphore-to-mutex.patch
linux-2.6.23/drivers/pcmcia/au1000_generic.c
soc_common-semaphore-to-mutex.patch
linux-2.6.23/drivers/pcmcia/soc_common.c
linux-2.6.23/drivers/pcmcia/soc_common.h
qla2xxx-semaphore-to-mutex.patch
linux-2.6.23/drivers/scsi/qla2xxx/qla_os.c
bcm43xx-semaphore-to-mutex.patch
linux-2.6.23/drivers/net/wireless/bcm43xx/bcm43xx_debugfs.c
adb_handler_sem-to-mutex.patch
linux-2.6.23/drivers/macintosh/adb.c
therm_pm72-semaphore-to-mutex.patch
linux-2.6.23/drivers/macintosh/therm_pm72.c
mbcs-semaphore-to-mutex.patch
linux-2.6.23/drivers/char/mbcs.c
linux-2.6.23/drivers/char/mbcs.h
ldusb-semaphore-to-mutex.patch
linux-2.6.23/drivers/usb/misc/ldusb.c
u132-hdc-semaphore-to-mutex.patch
linux-2.6.23/drivers/usb/host/u132-hcd.c
watchdog-wdt_pci.patch
linux-2.6.23/drivers/watchdog/wdt_pci.c
mcheck_semaphore-to-mutex.patch
linux-2.6.23/arch/x86/kernel/cpu/mcheck/mce_64.c
doc-kernel-locking-convert-semaphore-reference.patch
linux-2.6.23/Documentation/DocBook/kernel-locking.tmpl
net-9p-trans_virtio-semaphore-to-mutex.patch
linux-2.6.23/net/9p/trans_virtio.c
sound-s3c24xx-semaphore-to-mutex.patch
linux-2.6.23/sound/soc/s3c24xx/s3c2443-ac97.c
usb-microtek-semaphore-to-mutex.patch
linux-2.6.23/drivers/usb/image/microtek.c
linux-2.6.23/drivers/usb/image/microtek.h
video-sa1100fb-semaphore-to-mutex.patch
linux-2.6.23/drivers/video/sa1100fb.c
linux-2.6.23/drivers/video/sa1100fb.h
video-pxafb-semaphore-to-mutex.patch
linux-2.6.23/drivers/video/pxafb.c
linux-2.6.23/drivers/video/pxafb.h
macintosh-windfarm_smu_sat-semaphore-to-mutex.patch
linux-2.6.23/drivers/macintosh/windfarm_smu_sat.c
macintosh-therm_windtunnel-semaphore-to-mutex.patch
linux-2.6.23/drivers/macintosh/therm_windtunnel.c
macintosh-mediabay-semaphore-to-mutex.patch
linux-2.6.23/drivers/macintosh/mediabay.c
md-suspend_lock-semaphore-to-mutex.patch
linux-2.6.23/drivers/md/dm.c
s390-cio-semaphore-to-mutex.patch
linux-2.6.23/drivers/s390/cio/qdio.c
linux-2.6.23/drivers/s390/cio/qdio.h
message-fusion-mpt-semaphore-to-mutex.patch
linux-2.6.23/drivers/message/fusion/mptbase.c
linux-2.6.23/drivers/message/fusion/mptbase.h
linux-2.6.23/drivers/message/fusion/mptscsih.c
net-wan-cosa-semaphore-to-mutex.patch
linux-2.6.23/drivers/net/wan/cosa.c
usbvision-video-error-patch-unlock-fix.patch
linux-2.6.23/drivers/media/video/usbvision/usbvision-video.c
usbvision-video-semaphore-to-mutex.patch
linux-2.6.23/drivers/media/video/usbvision/usbvision-video.c
linux-2.6.23/drivers/media/video/usbvision/usbvision.h
ps3-vuart-locking-fix.patch
linux-2.6.23/drivers/ps3/ps3-vuart.c
ps3-vuart-semaphore-to-mutex.patch
linux-2.6.23/drivers/ps3/ps3-vuart.c
usbvision-video-remove-CtrlUrbLock.patch
linux-2.6.23/drivers/media/video/usbvision/usbvision-core.c
linux-2.6.23/drivers/media/video/usbvision/usbvision-video.c
linux-2.6.23/drivers/media/video/usbvision/usbvision.h

2007-12-06 20:40:40

by Remy Bohmer

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

Hello Daniel,

> I looked at the console_sem , but i was going to leave that as last..
>
> The problem with the console_sem is that it can get locked from
> interrupt context, which is discouraged with mutex types.. I think it
> will be complicated to convert..

At first it looked simple, but after I sent my mail, I saw also the
interrupt issue...
But nevertheless, I am going to look at it anyway... (I think not today)

Further, I looked also at include/linux/device.h where in the 'struct
device', a semaphore is listed that is used at several places as
mutex. I was wondering what was wise here:
* replacing the semaphore with a mutex at once, and replace it
everywhere in the kernel also, or
* just add a new mutex to the struct, and change the the semaphore to
a mutex per driver, and eventually remove the semaphore from the
struct.

The first option creates the biggest patch, the 2nd leaves the risk
for never removing the semaphore from the struct device...

What is your opinion?


BTW: I also will look at the dvb_frontend.c semaphore->mutex.
I also want to look at the mutex in the PS3 code, but I cannot test it
before I receive my new PS3 ;-)

Remy

2007-12-06 20:44:48

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex


* Daniel Walker <[email protected]> wrote:

> > cool. How far away are we from being able to remove all the
> > semaphore code? :-)
>
> I wish my 7 patches made a dent, but it's hasn't done much. ;(

it's a beginning.

> I would guess at least a week just to mop up the relatively easy
> ones.. I've got 12 in my queue, and there still ~50 hopefully trivial
> ones still to be looked at.. Then another ~30 more difficult ones
> (that use init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..

With BKL removal being projected to happen between 5 and 10 years from
now, i think you are moving at lightning's pace :-)

Ingo

2007-12-06 23:30:37

by Kevin Winchester

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

Daniel Walker wrote:
> On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
>> * Daniel Walker <[email protected]> wrote:
>>
>>> The port_mutex is actually a semaphore, so easily converted to a
>>> struct mutex.
>>>
>>> Signed-off-by: Daniel Walker <[email protected]>
>> Acked-by: Ingo Molnar <[email protected]>
>>
>> cool. How far away are we from being able to remove all the semaphore
>> code? :-)
>
> I wish my 7 patches made a dent, but it's hasn't done much. ;(
>
> I would guess at least a week just to mop up the relatively easy ones..
> I've got 12 in my queue, and there still ~50 hopefully trivial ones
> still to be looked at.. Then another ~30 more difficult ones (that use
> init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
>

I didn't realise there were so many of these patch sets still to go. I
could probably help out with some of them. Is there somewhere we could
keep track of which ones are left to do, and who is handling them? If
it would be helpful, I could go through all of the semaphore uses in the
tree and try to figure out which (if any) are true counting semaphores
that cannot be converted, and then I could post/send the list of
convertible cases. Would that be helpful, or has it already been done
somewhere else?

--
Kevin Winchester

2007-12-07 01:18:26

by Daniel Walker

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

On Thu, 2007-12-06 at 19:30 -0400, Kevin Winchester wrote:
> Daniel Walker wrote:
> > On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> >> * Daniel Walker <[email protected]> wrote:
> >>
> >>> The port_mutex is actually a semaphore, so easily converted to a
> >>> struct mutex.
> >>>
> >>> Signed-off-by: Daniel Walker <[email protected]>
> >> Acked-by: Ingo Molnar <[email protected]>
> >>
> >> cool. How far away are we from being able to remove all the semaphore
> >> code? :-)
> >
> > I wish my 7 patches made a dent, but it's hasn't done much. ;(
> >
> > I would guess at least a week just to mop up the relatively easy ones..
> > I've got 12 in my queue, and there still ~50 hopefully trivial ones
> > still to be looked at.. Then another ~30 more difficult ones (that use
> > init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
> >
>
> I didn't realise there were so many of these patch sets still to go. I
> could probably help out with some of them. Is there somewhere we could
> keep track of which ones are left to do, and who is handling them? If
> it would be helpful, I could go through all of the semaphore uses in the
> tree and try to figure out which (if any) are true counting semaphores
> that cannot be converted, and then I could post/send the list of
> convertible cases. Would that be helpful, or has it already been done
> somewhere else?

I've posted all the ones I've done so far ..

ftp://source.mvista.com/pub/dwalker/sem2mutex-2.6.24-rc4/

Feel free to review or test them.. I've found it pretty easy to simply
grep for certain class of semaphore usage, check if it's conforming to
the mutex requirements, then convert it or not.. Checking them is
getting to be a habit, so I don't think a list would help me.. However,
someone else might be able to use it..

Daniel


2007-12-07 01:30:21

by Kevin Winchester

[permalink] [raw]
Subject: Possible locking issue in viotape.c

Daniel Walker wrote:
>
> I've posted all the ones I've done so far ..
>
> ftp://source.mvista.com/pub/dwalker/sem2mutex-2.6.24-rc4/
>
> Feel free to review or test them.. I've found it pretty easy to simply
> grep for certain class of semaphore usage, check if it's conforming to
> the mutex requirements, then convert it or not.. Checking them is
> getting to be a habit, so I don't think a list would help me.. However,
> someone else might be able to use it..
>

Thanks, that helps me not duplicate anything. One of the first ones I
was looking at (before your post) was viotape.c, which is in your patch
set. However, looking at the uses of the semaphore, I see that on line
409-410 the following code:

if (noblock)
return count;

which seems to ignore the fact that the semaphore has been downed (not
to mention the dma buffer and op struct allocations. I think it should be:

if (noblock)
ret = count;
goto free_dma;

instead. Do you want to make sure I'm right about that and fold it into
your patch? Or have you already submitted your patch (or should it be
in a separate patch? Alternatively, I can submit the patch if you don't
want to bother with it.

--
Kevin Winchester

2007-12-07 01:53:49

by Daniel Walker

[permalink] [raw]
Subject: Re: Possible locking issue in viotape.c

On Thu, 2007-12-06 at 21:29 -0400, Kevin Winchester wrote:
> Daniel Walker wrote:
> >
> > I've posted all the ones I've done so far ..
> >
> > ftp://source.mvista.com/pub/dwalker/sem2mutex-2.6.24-rc4/
> >
> > Feel free to review or test them.. I've found it pretty easy to simply
> > grep for certain class of semaphore usage, check if it's conforming to
> > the mutex requirements, then convert it or not.. Checking them is
> > getting to be a habit, so I don't think a list would help me.. However,
> > someone else might be able to use it..
> >
>
> Thanks, that helps me not duplicate anything. One of the first ones I
> was looking at (before your post) was viotape.c, which is in your patch
> set. However, looking at the uses of the semaphore, I see that on line
> 409-410 the following code:
>
> if (noblock)
> return count;
>
> which seems to ignore the fact that the semaphore has been downed (not
> to mention the dma buffer and op struct allocations. I think it should be:
>
> if (noblock)
> ret = count;
> goto free_dma;
>
> instead. Do you want to make sure I'm right about that and fold it into
> your patch? Or have you already submitted your patch (or should it be
> in a separate patch? Alternatively, I can submit the patch if you don't
> want to bother with it.

viotape was one of the first I started converting, but later I noticed
the same thing you found above. I have it commented out of my series for
that reason ..

I think this noblock path is actually doing what the author intended..
There are a few stray up() calls related to event handling and ioctls ,
and I think those are used to release the semaphore..

Daniel

2007-12-07 07:41:21

by Matthias Kaehlcke

[permalink] [raw]
Subject: Re: [PATCH 3/3] printer port driver: semaphore to mutex

El Thu, Dec 06, 2007 at 09:12:05PM +0100 Remy Bohmer ha dit:

> Which do you have exactly on your list? (good to know, it prevents
> double work...)

it isn't really an elaborated list, until now i greped for certain
semaphore usages, had a look at the code and converted it if
necessary. at the moment i'm having a look at usb drivers.

> 2007/12/6, Matthias Kaehlcke <[email protected]>:
> > El Thu, Dec 06, 2007 at 08:34:06AM -0800 Daniel Walker ha dit:
> >
> > > On Thu, 2007-12-06 at 11:23 +0100, Ingo Molnar wrote:
> > > > * Daniel Walker <[email protected]> wrote:
> > > >
> > > > > The port_mutex is actually a semaphore, so easily converted to a
> > > > > struct mutex.
> > > > >
> > > > > Signed-off-by: Daniel Walker <[email protected]>
> > > >
> > > > Acked-by: Ingo Molnar <[email protected]>
> > > >
> > > > cool. How far away are we from being able to remove all the semaphore
> > > > code? :-)
> > >
> > > I wish my 7 patches made a dent, but it's hasn't done much. ;(
> > >
> > > I would guess at least a week just to mop up the relatively easy ones..
> > > I've got 12 in my queue, and there still ~50 hopefully trivial ones
> > > still to be looked at.. Then another ~30 more difficult ones (that use
> > > init_MUTEX_LOCKED, or sema_init with 0 instead of 1) ..
> >
> > I've also more patches of this type on my list, though i work in a
> > slower pace
> >
> >
> > You must have a plan. If you don't have a plan,
> > you'll become part of somebody else's plan
> > .''`.
> > using free software / Debian GNU/Linux | http://debian.org : :' :
> > `. `'`
> > gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-
> >

--
Matthias Kaehlcke
Linux System Developer
Barcelona

Don't walk behind me, I may not lead
Don't walk in front of me, I may not follow
Just walk beside me and be my friend
(Albert Camus)
.''`.
using free software / Debian GNU/Linux | http://debian.org : :' :
`. `'`
gpg --keyserver pgp.mit.edu --recv-keys 47D8E5D4 `-

2007-12-08 18:17:56

by Kevin Winchester

[permalink] [raw]
Subject: Re: Possible locking issue in viotape.c

Daniel Walker wrote:
> On Thu, 2007-12-06 at 21:29 -0400, Kevin Winchester wrote:
>> Daniel Walker wrote:
>>> I've posted all the ones I've done so far ..
>>>
>>> ftp://source.mvista.com/pub/dwalker/sem2mutex-2.6.24-rc4/
>>>
>>> Feel free to review or test them.. I've found it pretty easy to simply
>>> grep for certain class of semaphore usage, check if it's conforming to
>>> the mutex requirements, then convert it or not.. Checking them is
>>> getting to be a habit, so I don't think a list would help me.. However,
>>> someone else might be able to use it..
>>>
>> Thanks, that helps me not duplicate anything. One of the first ones I
>> was looking at (before your post) was viotape.c, which is in your patch
>> set. However, looking at the uses of the semaphore, I see that on line
>> 409-410 the following code:
>>
>> if (noblock)
>> return count;
>>
>> which seems to ignore the fact that the semaphore has been downed (not
>> to mention the dma buffer and op struct allocations. I think it should be:
>>
>> if (noblock)
>> ret = count;
>> goto free_dma;
>>
>> instead. Do you want to make sure I'm right about that and fold it into
>> your patch? Or have you already submitted your patch (or should it be
>> in a separate patch? Alternatively, I can submit the patch if you don't
>> want to bother with it.
>
> viotape was one of the first I started converting, but later I noticed
> the same thing you found above. I have it commented out of my series for
> that reason ..
>
> I think this noblock path is actually doing what the author intended..
> There are a few stray up() calls related to event handling and ioctls ,
> and I think those are used to release the semaphore..
>
> Daniel
>
>

Yes, you are right, I hadn't finished looking at all of the up() calls
when I came to my conclusion. I will try to convert a few that are not
on your list, but I would like to know how you are generating your
patches into those files with the diffstat and recipient list. Is that
a feature of some git command?

--
Kevin Winchester

2007-12-08 18:36:38

by Daniel Walker

[permalink] [raw]
Subject: Re: Possible locking issue in viotape.c

On Sat, 2007-12-08 at 14:17 -0400, Kevin Winchester wrote:
> Daniel Walker wrote:
> > On Thu, 2007-12-06 at 21:29 -0400, Kevin Winchester wrote:
> >> Daniel Walker wrote:
> >>> I've posted all the ones I've done so far ..
> >>>
> >>> ftp://source.mvista.com/pub/dwalker/sem2mutex-2.6.24-rc4/
> >>>
> >>> Feel free to review or test them.. I've found it pretty easy to simply
> >>> grep for certain class of semaphore usage, check if it's conforming to
> >>> the mutex requirements, then convert it or not.. Checking them is
> >>> getting to be a habit, so I don't think a list would help me.. However,
> >>> someone else might be able to use it..
> >>>
> >> Thanks, that helps me not duplicate anything. One of the first ones I
> >> was looking at (before your post) was viotape.c, which is in your patch
> >> set. However, looking at the uses of the semaphore, I see that on line
> >> 409-410 the following code:
> >>
> >> if (noblock)
> >> return count;
> >>
> >> which seems to ignore the fact that the semaphore has been downed (not
> >> to mention the dma buffer and op struct allocations. I think it should be:
> >>
> >> if (noblock)
> >> ret = count;
> >> goto free_dma;
> >>
> >> instead. Do you want to make sure I'm right about that and fold it into
> >> your patch? Or have you already submitted your patch (or should it be
> >> in a separate patch? Alternatively, I can submit the patch if you don't
> >> want to bother with it.
> >
> > viotape was one of the first I started converting, but later I noticed
> > the same thing you found above. I have it commented out of my series for
> > that reason ..
> >
> > I think this noblock path is actually doing what the author intended..
> > There are a few stray up() calls related to event handling and ioctls ,
> > and I think those are used to release the semaphore..
> >
> > Daniel
> >
> >
>
> Yes, you are right, I hadn't finished looking at all of the up() calls
> when I came to my conclusion. I will try to convert a few that are not
> on your list, but I would like to know how you are generating your
> patches into those files with the diffstat and recipient list. Is that
> a feature of some git command?

Git may have a similar feature, but I'm using a tool call Quilt. It's
used for managing a list of patches, and it can automatically add a
diffstat to the patch header (part of the quilt refresh command). I also
use it for emailing a list of patches.

Daniel

2007-12-08 19:19:32

by Kevin Winchester

[permalink] [raw]
Subject: Re: Possible locking issue in viotape.c

Daniel Walker wrote:
>> Yes, you are right, I hadn't finished looking at all of the up() calls
>> when I came to my conclusion. I will try to convert a few that are not
>> on your list, but I would like to know how you are generating your
>> patches into those files with the diffstat and recipient list. Is that
>> a feature of some git command?
>
> Git may have a similar feature, but I'm using a tool call Quilt. It's
> used for managing a list of patches, and it can automatically add a
> diffstat to the patch header (part of the quilt refresh command). I also
> use it for emailing a list of patches.
>

Yes, I've used quilt for working with mm patches in the past, but I'm
not too familiar with the mail features. For example, how do you get
the recipient list and Signed-off-by in the patch file? Do you just
edit it by hand? Or is there some guide to quilt I should be reading?
I looked at the guide in /usr/share/doc/quilt/quilt.pdf, but it didn't
have anything about email. /usr/share/doc/quilt/README.MAIL has the
details of all of the mail options, but doesn't give a nice example of
what to do with the patch file and how to call 'quilt mail'.

--
Kevin Winchester

2007-12-08 19:58:56

by Daniel Walker

[permalink] [raw]
Subject: Re: Possible locking issue in viotape.c

On Sat, 2007-12-08 at 15:19 -0400, Kevin Winchester wrote:

>
> Yes, I've used quilt for working with mm patches in the past, but I'm
> not too familiar with the mail features. For example, how do you get
> the recipient list and Signed-off-by in the patch file? Do you just
> edit it by hand? Or is there some guide to quilt I should be reading?
> I looked at the guide in /usr/share/doc/quilt/quilt.pdf, but it didn't
> have anything about email. /usr/share/doc/quilt/README.MAIL has the
> details of all of the mail options, but doesn't give a nice example of
> what to do with the patch file and how to call 'quilt mail'.

The patch description and Signed-off-by, is added with the command
"quilt header -e" which opens and editor.. I usually use something like
this to mail out patches

'quilt mail --send --to="[email protected]" --cc="[email protected]"'

That's how you add the recipients ..

Daniel