Dear Bjørn Erik Nilsen,
> 29. nov. 2013 kl. 18:02 skrev Marek Vasut <marex@xxxxxxx>:
> > 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,
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
