[PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Dave Young]

Signed-off-by: Dave Young <hidave.darkstar@gmail.com> 

---
drivers/pci/bus.c |    4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff -upr linux/drivers/pci/bus.c linux.new/drivers/pci/bus.c
--- linux/drivers/pci/bus.c	2007-12-28 10:25:07.000000000 +0800
+++ linux.new/drivers/pci/bus.c	2007-12-28 10:28:56.000000000 +0800
@@ -207,9 +207,9 @@ void pci_walk_bus(struct pci_bus *top, v
 			next = dev->bus_list.next;
 
 		/* Run device routines with the device locked */
-		down(&dev->dev.sem);
+		mutex_lock(&dev->dev.mutex);
 		cb(dev, userdata);
-		up(&dev->dev.sem);
+		mutex_unlock(&dev->dev.mutex);
 	}
 	up_read(&pci_bus_sem);
 }

Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Matthew Wilcox]

On Sat, Dec 29, 2007 at 09:10:57AM +0800, Dave Young wrote:
> @@ -207,9 +207,9 @@ void pci_walk_bus(struct pci_bus *top, v
>  			next = dev->bus_list.next;
>  
>  		/* Run device routines with the device locked */
> -		down(&dev->dev.sem);
> +		mutex_lock(&dev->dev.mutex);
>  		cb(dev, userdata);
> -		up(&dev->dev.sem);
> +		mutex_unlock(&dev->dev.mutex);
>  	}
>  	up_read(&pci_bus_sem);
>  }

Patches should be self-contained for ease of bisecting.  I can't tell
whether this patch is correct or not because you haven't included all
the other places that need to change at the same time as this.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Dave Young]

On Dec 29, 2007 10:55 AM, Matthew Wilcox <matthew@wil.cx> wrote:
> On Sat, Dec 29, 2007 at 09:10:57AM +0800, Dave Young wrote:
> > @@ -207,9 +207,9 @@ void pci_walk_bus(struct pci_bus *top, v
> >                       next = dev->bus_list.next;
> >
> >               /* Run device routines with the device locked */
> > -             down(&dev->dev.sem);
> > +             mutex_lock(&dev->dev.mutex);
> >               cb(dev, userdata);
> > -             up(&dev->dev.sem);
> > +             mutex_unlock(&dev->dev.mutex);
> >       }
> >       up_read(&pci_bus_sem);
> >  }
>
> Patches should be self-contained for ease of bisecting.  I can't tell
> whether this patch is correct or not because you haven't included all
> the other places that need to change at the same time as this.

Hi,

I will repost them after some fixes.

>
> --
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."
>

Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Stefan Richter]

Matthew Wilcox wrote:
> Patches should be self-contained for ease of bisecting.  I can't tell
> whether this patch is correct or not because you haven't included all
> the other places that need to change at the same time as this.

I think a broken-up patch series isn't totally wrong to do for a first
look at these RFC patches.  Of course the series needs to become a
single patch before it is committed to a tree whose history needs to
support bijection, e.g. -mm.

However, Dave's postings lack a References: header which refer to his
00/12 posting.

(Also, a bonus in the 00/12 posting would be a listing of all patch
titles in the series and the total diffstat of the series, but nearly
nobody does this.)
-- 
Stefan Richter
-=====-=-=== ==-- ===-=
http://arcgraph.de/sr/

Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Matthew Wilcox]

On Sat, Dec 29, 2007 at 12:42:31PM +0100, Stefan Richter wrote:
> I think a broken-up patch series isn't totally wrong to do for a first
> look at these RFC patches.  Of course the series needs to become a
> single patch before it is committed to a tree whose history needs to
> support bijection, e.g. -mm.

For this kind of patch (converting a semaphore to a mutex), it is
necessary to see everywhere that changes.  There's no way to tell if what
he's doing is safe without seeing all the places that have to change,
and verifying whether it breaks any of the rules in include/linux/mutex.h

For other kinds of patches, you could well be right.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Stefan Richter]

Matthew Wilcox wrote:
> On Sat, Dec 29, 2007 at 12:42:31PM +0100, Stefan Richter wrote:
>> I think a broken-up patch series isn't totally wrong to do for a first
>> look at these RFC patches.  Of course the series needs to become a
>> single patch before it is committed to a tree whose history needs to
>> support bijection, e.g. -mm.
> 
> For this kind of patch (converting a semaphore to a mutex), it is
> necessary to see everywhere that changes.  There's no way to tell if what
> he's doing is safe without seeing all the places that have to change,
> and verifying whether it breaks any of the rules in include/linux/mutex.h

Exactly.

> For other kinds of patches, you could well be right.

You can see the whole of the changes by looking at the whole of Dave's
postings.  (This would of course be simpler if it was a single posting.)
-- 
Stefan Richter
-=====-=-=== ==-- ===-=
http://arcgraph.de/sr/

Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core

[Dave Young]

On Dec 29, 2007 7:42 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Matthew Wilcox wrote:
> > Patches should be self-contained for ease of bisecting.  I can't tell
> > whether this patch is correct or not because you haven't included all
> > the other places that need to change at the same time as this.
>
> I think a broken-up patch series isn't totally wrong to do for a first
> look at these RFC patches.  Of course the series needs to become a
> single patch before it is committed to a tree whose history needs to
> support bijection, e.g. -mm.
>
> However, Dave's postings lack a References: header which refer to his
> 00/12 posting.

Thanks.  I agree with you, for bitsection it should be a single one.

>
> (Also, a bonus in the 00/12 posting would be a listing of all patch
> titles in the series and the total diffstat of the series, but nearly
> nobody does this.)

Your sugestion is better.

And, andrew recommends not to use 00/xx introduction email in series
in his "The perfect patch":
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

> --
> Stefan Richter
> -=====-=-=== ==-- ===-=
> http://arcgraph.de/sr/
>

The perfect patch - Posting a patch series (was Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core)

[Stefan Richter]

Dave Young wrote:
> On Dec 29, 2007 7:42 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> However, Dave's postings lack a References: header which refer to his
>> 00/12 posting.
[To let mail readers show it as a thread.]
>> (Also, a bonus in the 00/12 posting would be a listing of all patch
>> titles in the series and the total diffstat of the series,
[similar to the "git pull" requests from maintainers]
>> but nearly nobody does this.)
...
> andrew recommends not to use 00/xx introduction email in series
> in his "The perfect patch":
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

"Please don't post [PATCH 0/n] messages" is a simplified short-hand for
"Please don't move information which we want to include into the SCM
changelog into a separate [PATCH 0/n] message".

There is nothing wrong with a 0/n posting per se.  But whenever you
write a 0/n posting, ask yourself:
  - Isn't the information I provide here necessary to keep around by
    somebody who takes my patch series into his quilt series or into his
    source repository?
  - Couldn't the information here be useful at a later point in time
    when people look into the mainline Linux history?
If "yes" or "maybe yes", then add this information to the changelogs in
the patches.  You can then leave the 0/n posting as is, or make it
briefer, or omit it entirely.

It is never necessary to post a 0/n message, because _everything_ which
could be said in this message can also be said in the i/n messages.
(Things which are not meant for the SCM changelog can be written after a
"---" delimiter line or other patch delimiters.)  However, it is
sometimes convenient to repeat or summarize some of the information from
the i/n messages in a 0/n message.  Think about convenience of the
_recipients_ though, not about the sender's convenience.

Generally, the 0/n message fulfills purposes very similar to "git pull"
messages:  They give a brief overview of what is coming up in the series
and how to handle it, and it adds redundant information about the
contents of the series (titles, authors, overall diffstat, whether it
supersedes an earlier series) as a verification for the recipient
whether he really got what the sender intended to get to him.  This is
to help detect mix-ups at the sender's or receiver's side.

PS:
Writing a changelog is almost never trivial.  Even if it seems trivial
to the patch author, the change may not be trivial from other
developers' and maintainers' perspective, or from the author's
perspective when he looks at his patch a few months later.  This also
means that there may very well be information in the 0/n message which
should also appear in the i/n messages, even if this information seems
obvious to the author.
-- 
Stefan Richter
-=====-==--- ---= ---=-
http://arcgraph.de/sr/

Re: The perfect patch - Posting a patch series (was Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core)

[Jan Engelhardt]

On Jan 2 2008 12:14, Stefan Richter wrote:
>There is nothing wrong with a 0/n posting per se.  But whenever you
>write a 0/n posting, ask yourself:
>  - Isn't the information I provide here necessary to keep around by
>    somebody who takes my patch series into his quilt series or into his
>    source repository?
>  - Couldn't the information here be useful at a later point in time
>    when people look into the mainline Linux history?
>If "yes" or "maybe yes", then add this information to the changelogs in
>the patches.  You can then leave the 0/n posting as is, or make it
>briefer, or omit it entirely.

Please emit a [0/n] and the [m/n] as a reply so that threading-aware
mail readers can collapse and/or use delete-thread-at-a-time feature.
If there is no info in [0/n], let it be empty or a one,two-liner brief
introduction.

>It is never necessary to post a 0/n message, because _everything_ which
>could be said in this message can also be said in the i/n messages.
>(Things which are not meant for the SCM changelog can be written after a
>"---" delimiter line or other patch delimiters.)  However, it is
>sometimes convenient to repeat or summarize some of the information from
>the i/n messages in a 0/n message.  Think about convenience of the
>_recipients_ though, not about the sender's convenience.

And [0/n] sometimes contain a diffstat which gives an approximate
line count of how big the patchset actually is.

Re: The perfect patch - Posting a patch series

[Stefan Richter]

Jan Engelhardt wrote:
> And [0/n] sometimes contain a diffstat which gives an approximate
> line count of how big the patchset actually is.

There is actually no good reason for omitting such a diffstat.  It's
easy enough to generate.

$ quilt diff --combine first.patch -P last.patch | diffstat -p1 -w72

$ git diff --stat=72 --summary -M commit_before_first_patch..last_commit
-- 
Stefan Richter
-=====-==--- ---= ---=-
http://arcgraph.de/sr/

Re: The perfect patch - Posting a patch series (was Re: [PATCH 06/12] pci : Use mutex instead of semaphore in driver core)

[Dave Young]

On Jan 2, 2008 7:14 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> Dave Young wrote:
> > On Dec 29, 2007 7:42 PM, Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> However, Dave's postings lack a References: header which refer to his
> >> 00/12 posting.
> [To let mail readers show it as a thread.]
> >> (Also, a bonus in the 00/12 posting would be a listing of all patch
> >> titles in the series and the total diffstat of the series,
> [similar to the "git pull" requests from maintainers]
> >> but nearly nobody does this.)
> ...
> > andrew recommends not to use 00/xx introduction email in series
> > in his "The perfect patch":
> > http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
>
> "Please don't post [PATCH 0/n] messages" is a simplified short-hand for
> "Please don't move information which we want to include into the SCM
> changelog into a separate [PATCH 0/n] message".
>
> There is nothing wrong with a 0/n posting per se.  But whenever you
> write a 0/n posting, ask yourself:
>   - Isn't the information I provide here necessary to keep around by
>     somebody who takes my patch series into his quilt series or into his
>     source repository?
>   - Couldn't the information here be useful at a later point in time
>     when people look into the mainline Linux history?
> If "yes" or "maybe yes", then add this information to the changelogs in
> the patches.  You can then leave the 0/n posting as is, or make it
> briefer, or omit it entirely.
>
> It is never necessary to post a 0/n message, because _everything_ which
> could be said in this message can also be said in the i/n messages.
> (Things which are not meant for the SCM changelog can be written after a
> "---" delimiter line or other patch delimiters.)  However, it is
> sometimes convenient to repeat or summarize some of the information from
> the i/n messages in a 0/n message.  Think about convenience of the
> _recipients_ though, not about the sender's convenience.
>
> Generally, the 0/n message fulfills purposes very similar to "git pull"
> messages:  They give a brief overview of what is coming up in the series
> and how to handle it, and it adds redundant information about the
> contents of the series (titles, authors, overall diffstat, whether it
> supersedes an earlier series) as a verification for the recipient
> whether he really got what the sender intended to get to him.  This is
> to help detect mix-ups at the sender's or receiver's side.
>
> PS:
> Writing a changelog is almost never trivial.  Even if it seems trivial
> to the patch author, the change may not be trivial from other
> developers' and maintainers' perspective, or from the author's
> perspective when he looks at his patch a few months later.  This also
> means that there may very well be information in the 0/n message which
> should also appear in the i/n messages, even if this information seems
> obvious to the author.

Thanks for the explanation, I strongly agree with you.
I think that 0/n message should be a summary of the series. At the
same time the i/n changelog should not be stripped, any info of
changes should be added to the relavant patches.

Regards
dave