MPIC cleanup series

MPIC cleanup series

[Benjamin Herrenschmidt]

Hi Kyle !

I can't reply to the patches themselves easily unfortunately as I
accidentally deleted them from my mailer :-)

Overall I really look your series. It doesn't quite apply cleanly
anymore so I'll as you for a new shoot after you address the comments
below, at which point, if you're fast enough, I'll stick it in -next :-)

Just a couple of comments on some of the patches:

 - 5/10: search for open-pic device-tree node if NULL

The idea is fine, however most callers ignore the device-type and only
compare on compatible, while you replace that with a match entry that
seems to require matching on both. This is likely to break stuff. The
"type" part of te march entry should be NULL I believe.

 - 9/10: cache the node

of_node_get() is your friend. 

 - 10/10: Makes me a bit nervous. It 'looks' right but I wouldn't bet on
Apple device-trees being sane vs. chaining. I would like a test that
doesn't do the cascade if the mpic is a primary to at least limit the
risk of messup.

Cheers,
Ben.

Re: MPIC cleanup series

[Kyle Moffett]

On Sun, Nov 27, 2011 at 18:51, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> Overall I really look your series. It doesn't quite apply cleanly
> anymore so I'll as you for a new shoot after you address the comments
> below, at which point, if you're fast enough, I'll stick it in -next :-)

Awesome! Thanks!

As I mentioned before, I have precious little of the hardware to test
this all on, so I hope I don't break anything.  At minimum I need to
do a final build-and-run test on my e500 boards before I send it out.
:-D


> Just a couple of comments on some of the patches:
>
> =C2=A0- 5/10: search for open-pic device-tree node if NULL
>
> The idea is fine, however most callers ignore the device-type and only
> compare on compatible, while you replace that with a match entry that
> seems to require matching on both. This is likely to break stuff. The
> "type" part of te march entry should be NULL I believe.

If you re-read that, the match table used if no of_node is passed in
has *two* separate entries, one of them with a "type" and the other
with a "compatible", as opposed to a single entry which matches both
"type" and "compatible".

There are a lot of callers which do:
  dnp =3D of_find_node_by_type(NULL, "open-pic");

So I doubt I can remove the "type" entry all together, unfortunately.


> =C2=A0- 9/10: cache the node
>
> of_node_get() is your friend.

Yes, I actually messed this one up in the prior patch too, thanks for
noticing.  It should all be fixed now.


> =C2=A0- 10/10: Makes me a bit nervous. It 'looks' right but I wouldn't be=
t on
> Apple device-trees being sane vs. chaining. I would like a test that
> doesn't do the cascade if the mpic is a primary to at least limit the
> risk of messup.

Oh, you mean to wrap that block like this?

if (mpic->flags & MPIC_SECONDARY) {
  virq =3D irq_of_parse_and_map(mpic->node, 0);
  ...
}

Sure, makes sense to me.  I've made that change.

Thanks for the review!

Cheers,
Kyle Moffett

--=20
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

Re: MPIC cleanup series

[Benjamin Herrenschmidt]

On Mon, 2011-11-28 at 15:48 -0500, Kyle Moffett wrote:
> On Sun, Nov 27, 2011 at 18:51, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > Overall I really look your series. It doesn't quite apply cleanly
> > anymore so I'll as you for a new shoot after you address the comments
> > below, at which point, if you're fast enough, I'll stick it in -next :-)
> 
> Awesome! Thanks!
> 
> As I mentioned before, I have precious little of the hardware to test
> this all on, so I hope I don't break anything.  At minimum I need to
> do a final build-and-run test on my e500 boards before I send it out.
> :-D

That's ok, I was planning on letting it simmer in -test for a week or
so, giving myself time to test on a range of powermacs etc...

> > Just a couple of comments on some of the patches:
> >
> >  - 5/10: search for open-pic device-tree node if NULL
> >
> > The idea is fine, however most callers ignore the device-type and only
> > compare on compatible, while you replace that with a match entry that
> > seems to require matching on both. This is likely to break stuff. The
> > "type" part of te march entry should be NULL I believe.
> 
> If you re-read that, the match table used if no of_node is passed in
> has *two* separate entries, one of them with a "type" and the other
> with a "compatible", as opposed to a single entry which matches both
> "type" and "compatible".

Oh, my bad. Ok.

> There are a lot of callers which do:
>   dnp = of_find_node_by_type(NULL, "open-pic");
> 
> So I doubt I can remove the "type" entry all together, unfortunately.
> 
> 
> >  - 9/10: cache the node
> >
> > of_node_get() is your friend.
> 
> Yes, I actually messed this one up in the prior patch too, thanks for
> noticing.  It should all be fixed now.
> 
> 
> >  - 10/10: Makes me a bit nervous. It 'looks' right but I wouldn't bet on
> > Apple device-trees being sane vs. chaining. I would like a test that
> > doesn't do the cascade if the mpic is a primary to at least limit the
> > risk of messup.
> 
> Oh, you mean to wrap that block like this?
> 
> if (mpic->flags & MPIC_SECONDARY) {
>   virq = irq_of_parse_and_map(mpic->node, 0);
>   ...
> }

Yes.

> Sure, makes sense to me.  I've made that change.
> 
> Thanks for the review!

Thanks. Re-post the whole series and I'll merge it.

Cheers,
Ben.