LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [PATCH 0/21] MSI rework
       [not found] <1174560686.307511.956605711793.qpush@cradle>
@ 2007-03-22 15:01 ` Eric W. Biederman
  2007-03-22 19:08   ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Eric W. Biederman @ 2007-03-22 15:01 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: linux-pci, Greg Kroah-Hartman, David S. Miller,
	Benjamin Herrenschmidt, linux-kernel, Andrew Morton,
	daniel.e.wolstenholme

Michael Ellerman <michael@ellerman.id.au> writes:

> This is my series to rework the generic MSI code into something we can use
> on powerpc[1].
>
> I've tried as much as possible not to change the semantics for other archs,
> but there's a few little changes. I think they're all OK in their own right.
>
> I don't have any serious hardware to test on, but my little x86_64 box with
> an e1000 using MSI still works with these changes. I've also got MSI working
> on a powerpc blade with a tg3. I haven't tested MSI-X _at all_.
>
> I've also tested on the blade with a debug hack to make the MSI case
> allocate/free 8 MSIs, but only use the last one, just to exercise the n > 1
> case a little. All seems to work fine.

Generally I think this looks good.  However there is a lot here, and some
of it potentially at least has some pretty subtle side effects.

So reviewing all of these patches at once is almost certain to cause
something important to be missed :(

Can we slow this process down a few days by taking this one logical chunk
at a time?

i.e.  First the simple bug fixes that should purely be restructure of
msi.c with no affect on anything outside of it.

And then get into the architecture enhancements.

Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-22 15:01 ` [PATCH 0/21] MSI rework Eric W. Biederman
@ 2007-03-22 19:08   ` Greg KH
  2007-03-22 22:02     ` Benjamin Herrenschmidt
  0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2007-03-22 19:08 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Michael Ellerman, linux-pci, David S. Miller,
	Benjamin Herrenschmidt, linux-kernel, Andrew Morton,
	daniel.e.wolstenholme

On Thu, Mar 22, 2007 at 09:01:10AM -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > This is my series to rework the generic MSI code into something we can use
> > on powerpc[1].
> >
> > I've tried as much as possible not to change the semantics for other archs,
> > but there's a few little changes. I think they're all OK in their own right.
> >
> > I don't have any serious hardware to test on, but my little x86_64 box with
> > an e1000 using MSI still works with these changes. I've also got MSI working
> > on a powerpc blade with a tg3. I haven't tested MSI-X _at all_.
> >
> > I've also tested on the blade with a debug hack to make the MSI case
> > allocate/free 8 MSIs, but only use the last one, just to exercise the n > 1
> > case a little. All seems to work fine.
> 
> Generally I think this looks good.  However there is a lot here, and some
> of it potentially at least has some pretty subtle side effects.
> 
> So reviewing all of these patches at once is almost certain to cause
> something important to be missed :(
> 
> Can we slow this process down a few days by taking this one logical chunk
> at a time?
> 
> i.e.  First the simple bug fixes that should purely be restructure of
> msi.c with no affect on anything outside of it.
> 
> And then get into the architecture enhancements.

I agree, care to break these down into a smaller series of patches that
can go into -mm for testing?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-22 19:08   ` Greg KH
@ 2007-03-22 22:02     ` Benjamin Herrenschmidt
  2007-03-22 22:08       ` Greg KH
  0 siblings, 1 reply; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2007-03-22 22:02 UTC (permalink / raw)
  To: Greg KH
  Cc: Eric W. Biederman, Michael Ellerman, linux-pci, David S. Miller,
	linux-kernel, Andrew Morton, daniel.e.wolstenholme

> > i.e.  First the simple bug fixes that should purely be restructure of
> > msi.c with no affect on anything outside of it.
> > 
> > And then get into the architecture enhancements.
> 
> I agree, care to break these down into a smaller series of patches that
> can go into -mm for testing?

I don't see the point in breaking the serie... you can bisect half way
through if necessary... it's made of small patches that are done, afaik,
in such a way that the whole thing should still work at any level in the
serie.

The serie just expresses the dependency between them.

Ben.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-22 22:02     ` Benjamin Herrenschmidt
@ 2007-03-22 22:08       ` Greg KH
  2007-03-22 22:31         ` Benjamin Herrenschmidt
  2007-03-23  4:17         ` Michael Ellerman
  0 siblings, 2 replies; 9+ messages in thread
From: Greg KH @ 2007-03-22 22:08 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Eric W. Biederman, Michael Ellerman, linux-pci, David S. Miller,
	linux-kernel, Andrew Morton, daniel.e.wolstenholme

On Fri, Mar 23, 2007 at 09:02:16AM +1100, Benjamin Herrenschmidt wrote:
> > > i.e.  First the simple bug fixes that should purely be restructure of
> > > msi.c with no affect on anything outside of it.
> > > 
> > > And then get into the architecture enhancements.
> > 
> > I agree, care to break these down into a smaller series of patches that
> > can go into -mm for testing?
> 
> I don't see the point in breaking the serie... you can bisect half way
> through if necessary... it's made of small patches that are done, afaik,
> in such a way that the whole thing should still work at any level in the
> serie.
> 
> The serie just expresses the dependency between them.

Ok, then which patches in the series should be acceptable to take right
now for 2.6.22?  The "clean up the BUG" ones?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-22 22:08       ` Greg KH
@ 2007-03-22 22:31         ` Benjamin Herrenschmidt
  2007-03-23  4:17         ` Michael Ellerman
  1 sibling, 0 replies; 9+ messages in thread
From: Benjamin Herrenschmidt @ 2007-03-22 22:31 UTC (permalink / raw)
  To: Greg KH
  Cc: Eric W. Biederman, Michael Ellerman, linux-pci, David S. Miller,
	linux-kernel, Andrew Morton, daniel.e.wolstenholme


> Ok, then which patches in the series should be acceptable to take right
> now for 2.6.22?  The "clean up the BUG" ones?

I'd say 1...8 (BUG cleanups and removing msi_cache) looks to be on the
safe side to me, though of course, some review by Eric & a bit of
testing would be welcome regardless :-)

The others could go into -mm right away I suppose (or rather as soon as
Eric gets a chance to verify they don't break his test setups as I think
he can test MSI-X) and we can pour them into mainline one by one if
necessary as they get hammered / verified.

Cheers,
Ben.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-22 22:08       ` Greg KH
  2007-03-22 22:31         ` Benjamin Herrenschmidt
@ 2007-03-23  4:17         ` Michael Ellerman
  2007-03-23 10:25           ` Eric W. Biederman
  1 sibling, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2007-03-23  4:17 UTC (permalink / raw)
  To: Greg KH
  Cc: Benjamin Herrenschmidt, Eric W. Biederman, linux-pci,
	David S. Miller, linux-kernel, Andrew Morton,
	daniel.e.wolstenholme

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

On Thu, 2007-03-22 at 15:08 -0700, Greg KH wrote:
> On Fri, Mar 23, 2007 at 09:02:16AM +1100, Benjamin Herrenschmidt wrote:
> > > > i.e.  First the simple bug fixes that should purely be restructure of
> > > > msi.c with no affect on anything outside of it.
> > > > 
> > > > And then get into the architecture enhancements.
> > > 
> > > I agree, care to break these down into a smaller series of patches that
> > > can go into -mm for testing?
> > 
> > I don't see the point in breaking the serie... you can bisect half way
> > through if necessary... it's made of small patches that are done, afaik,
> > in such a way that the whole thing should still work at any level in the
> > serie.
> > 
> > The serie just expresses the dependency between them.
> 
> Ok, then which patches in the series should be acceptable to take right
> now for 2.6.22?  The "clean up the BUG" ones?

The series is already very verbose, I don't think I can split most of
them any smaller without producing an unbuildable kernel.

I think 1 up to and including 11 are safe as houses, they shouldn't have
any effect other than to clean up the code.

The rest make functional changes, but they're all quite small, self
contained, and easily bisectable. I'd certainly like Eric to have a look
at them, but at some point I think we're just going to have to bite the
bullet and merge them, and see what we get in the way of bug reports.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-23  4:17         ` Michael Ellerman
@ 2007-03-23 10:25           ` Eric W. Biederman
  2007-03-26  7:09             ` Michael Ellerman
  0 siblings, 1 reply; 9+ messages in thread
From: Eric W. Biederman @ 2007-03-23 10:25 UTC (permalink / raw)
  To: michael
  Cc: Greg KH, Benjamin Herrenschmidt, linux-pci, David S. Miller,
	linux-kernel, Andrew Morton, daniel.e.wolstenholme

Michael Ellerman <michael@ellerman.id.au> writes:

> On Thu, 2007-03-22 at 15:08 -0700, Greg KH wrote:
>> On Fri, Mar 23, 2007 at 09:02:16AM +1100, Benjamin Herrenschmidt wrote:
>> > > > i.e.  First the simple bug fixes that should purely be restructure of
>> > > > msi.c with no affect on anything outside of it.
>> > > > 
>> > > > And then get into the architecture enhancements.
>> > > 
>> > > I agree, care to break these down into a smaller series of patches that
>> > > can go into -mm for testing?
>> > 
>> > I don't see the point in breaking the serie... you can bisect half way
>> > through if necessary... it's made of small patches that are done, afaik,
>> > in such a way that the whole thing should still work at any level in the
>> > serie.
>> > 
>> > The serie just expresses the dependency between them.
>> 
>> Ok, then which patches in the series should be acceptable to take right
>> now for 2.6.22?  The "clean up the BUG" ones?
>
> The series is already very verbose, I don't think I can split most of
> them any smaller without producing an unbuildable kernel.

That wasn't the request.

> I think 1 up to and including 11 are safe as houses, they shouldn't have
> any effect other than to clean up the code.
>
> The rest make functional changes, but they're all quite small, self
> contained, and easily bisectable. I'd certainly like Eric to have a look
> at them, but at some point I think we're just going to have to bite the
> bullet and merge them, and see what we get in the way of bug reports.

What I wanted was the patches organized into functional groups that
were small enough to review as a unit.  (Feed the existing patches slower
please).

This seems to be to much change to read and review as a unit, I just get
bleary eyed, and start to get confused.

So far I have found one subtle bug.  Where admittedly my code wasn't as
obvious as it could be and you were proposing to use an irq that had already
been freed.

What I had hoped we can do is you would send a handful at a time I
would review them.  Then we could get the next handful.  I expect
doing it that way it should take about a week to get through them all.

I guess I can try going through the review that way as well.  Pick a
subset of what you have sent and review it very carefully, and the
next day pick a different subset.  

I have been sufficiently active in this code lately that I think I can
do a good review, and I want to.  Unfortunately I'm only human and
a good review is nearly as much work as writing the patches myself
which means it takes time.

Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-23 10:25           ` Eric W. Biederman
@ 2007-03-26  7:09             ` Michael Ellerman
  2007-03-26 11:52               ` Eric W. Biederman
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Ellerman @ 2007-03-26  7:09 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Greg KH, Benjamin Herrenschmidt, linux-pci, David S. Miller,
	linux-kernel, Andrew Morton, daniel.e.wolstenholme

[-- Attachment #1: Type: text/plain, Size: 3071 bytes --]

On Fri, 2007-03-23 at 04:25 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> writes:
> 
> > On Thu, 2007-03-22 at 15:08 -0700, Greg KH wrote:
> >> On Fri, Mar 23, 2007 at 09:02:16AM +1100, Benjamin Herrenschmidt wrote:
> >> > > > i.e.  First the simple bug fixes that should purely be restructure of
> >> > > > msi.c with no affect on anything outside of it.
> >> > > > 
> >> > > > And then get into the architecture enhancements.
> >> > > 
> >> > > I agree, care to break these down into a smaller series of patches that
> >> > > can go into -mm for testing?
> >> > 
> >> > I don't see the point in breaking the serie... you can bisect half way
> >> > through if necessary... it's made of small patches that are done, afaik,
> >> > in such a way that the whole thing should still work at any level in the
> >> > serie.
> >> > 
> >> > The serie just expresses the dependency between them.
> >> 
> >> Ok, then which patches in the series should be acceptable to take right
> >> now for 2.6.22?  The "clean up the BUG" ones?
> >
> > The series is already very verbose, I don't think I can split most of
> > them any smaller without producing an unbuildable kernel.
> 
> That wasn't the request.
> 
> > I think 1 up to and including 11 are safe as houses, they shouldn't have
> > any effect other than to clean up the code.
> >
> > The rest make functional changes, but they're all quite small, self
> > contained, and easily bisectable. I'd certainly like Eric to have a look
> > at them, but at some point I think we're just going to have to bite the
> > bullet and merge them, and see what we get in the way of bug reports.
> 
> What I wanted was the patches organized into functional groups that
> were small enough to review as a unit.  (Feed the existing patches slower
> please).
> 
> This seems to be to much change to read and review as a unit, I just get
> bleary eyed, and start to get confused.
> 
> So far I have found one subtle bug.  Where admittedly my code wasn't as
> obvious as it could be and you were proposing to use an irq that had already
> been freed.
> 
> What I had hoped we can do is you would send a handful at a time I
> would review them.  Then we could get the next handful.  I expect
> doing it that way it should take about a week to get through them all.
> 
> I guess I can try going through the review that way as well.  Pick a
> subset of what you have sent and review it very carefully, and the
> next day pick a different subset.  

OK. For starters, do you want to review the first eleven as I've sent
them already, that saves spamming everyone again.

If you're OK with those eleven, then I'll send the remaining 10 or so
later in the week, broken up into (sort-of) functional groups.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/21] MSI rework
  2007-03-26  7:09             ` Michael Ellerman
@ 2007-03-26 11:52               ` Eric W. Biederman
  0 siblings, 0 replies; 9+ messages in thread
From: Eric W. Biederman @ 2007-03-26 11:52 UTC (permalink / raw)
  To: michael
  Cc: Eric W. Biederman, Greg KH, Benjamin Herrenschmidt, linux-pci,
	David S. Miller, linux-kernel, Andrew Morton,
	daniel.e.wolstenholme

Michael Ellerman <michael@ellerman.id.au> writes:

> OK. For starters, do you want to review the first eleven as I've sent
> them already, that saves spamming everyone again.
>
> If you're OK with those eleven, then I'll send the remaining 10 or so
> later in the week, broken up into (sort-of) functional groups.

Sounds like a decent plan.  At a quick glance there is a reasonable chance
I'm going to run into issues with patch 9 or 10.  Let me get as far as I can
with the acks and then we can see where we go with the next round of patches.

Eric

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2007-03-26 11:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1174560686.307511.956605711793.qpush@cradle>
2007-03-22 15:01 ` [PATCH 0/21] MSI rework Eric W. Biederman
2007-03-22 19:08   ` Greg KH
2007-03-22 22:02     ` Benjamin Herrenschmidt
2007-03-22 22:08       ` Greg KH
2007-03-22 22:31         ` Benjamin Herrenschmidt
2007-03-23  4:17         ` Michael Ellerman
2007-03-23 10:25           ` Eric W. Biederman
2007-03-26  7:09             ` Michael Ellerman
2007-03-26 11:52               ` Eric W. Biederman

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).