Re: [PATCH v5 1/2] PCI: designware: Fix crash in dw_msi_teardown_irq

[Marek Vasut <marex@denx.de>]

Dear Bjørn Erik Nilsen,

> 29. nov. 2013 kl. 18:02 skrev Marek Vasut <marex@denx.de>:
> > Dear Bjørn Erik Nilsen,
> > 
> >> Dear Marek Vasut,
> >> 
> >> On Fri, 2013-11-29 at 16:36 +0100, Marek Vasut wrote:
> >>> Dear Bjørn Erik Nilsen,
> >>> [...]
> >>> 
> >>>>> Won't simple
> >>>>> 
> >>>>> for (i = 0; i < nvec; i++) {
> >>>>> 
> >>>>> do here?
> >>>> 
> >>>> Yes.
> >>>> 
> >>>> The very same syntax ('while i < nvec') is used in assign_irq. That is
> >>>> why I wanted to keep it like that to avoid adding too much confusion,
> >>>> or at least make it easy to recognize the same pattern.
> >>>> 
> >>>> You still want me to change it?
> >>> 
> >>> Your loop does exactly what a for() loop would do, there's no need to
> >>> emulate it with a while() loop. If you can fix the other one, fix the
> >>> other one as well please.
> >> 
> >> Yes, it is syntax sugar indeed and I did it purely for readability as
> >> explained. You don't seem to agree with me on the readability argument,
> >> so I can make the new code use a for-loop. And then in an unrelated
> >> commit (which would look awfully silly) I can change the other
> >> while-loop too.
> > 
> > Either way is fine with me, I'm just more inclined to the for loop ;-)
> > Let's wait for what the others have to say, they can judge this
> > argument.
> > 
> >>>> As for the other nitpick, I don't agree with you.
> >>>> 
> >>>> In fact, dw_msi_teardown_irq has no return value itself. Moreover, if
> >>>> setting the msi desc to NULL fails, then it simply means there is no
> >>>> irq desc and there is nothing to unwind. Also, skipping the other
> >>>> unwind operations just because that single operation failed would
> >>>> leave the driver in a much worse state. At least in my opinion.
> >>>> 
> >>>> What's your opinion on that?
> >>> 
> >>> I see this:
> >>> 
> >>> $ git grep irq_set_msi_desc_off include/
> >>> include/linux/irq.h:extern int irq_set_msi_desc_off(unsigned int
> >>> irq_base, unsigned int irq_offset,
> >>> 
> >>> So it has a return value which needs to be handled. Sorry if I wasn't
> >>> clear on the first try.
> >> 
> >> I agree that return values should be checked.
> >> 
> >> However based on my reasoning there is absolutely nothing to do with the
> >> return value in this particular case, and in fact bailing out will leave
> >> the driver in a much worse state (as other unwind operations will be
> >> skipped).
> >> 
> >> That is what I kindly asked you to comment on.
> > 
> > How come you cannot unwind what you did thus far and bail out ? That
> > means the design here is seriously broken, don't you think ?
> 
> Maybe you need to take a closer look at the patch because the code in
> question is *the* unwinding code. So bailing out from there  doesn't make
> much sense.
> 
> Also note that I do check the return value of the same function call in
> setup, but the one you are complaining is for unwinding stuff that has
> already been setup.
> 
> I hope this helps :-)

Please consider me stupid and blind, sorry. I think maybe a WARN_ON(ret) might 
be a good sign things have gone severely awry here at least. What do you think?

Best regards,