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

[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 ?

> > > Now, if you want me to make another patch do you prefer a standalone
> > > patch on top of the other patches, or a completely new patchset?
> > 
> > Let's keep iterating :) But please wait for more feedback first.
> 
> OK. I will wait for more feedback then.

ACK, thanks ;-)

Best regards,
Marek Vasut