From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-out.m-online.net ([212.18.0.9]:37223 "EHLO mail-out.m-online.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752322Ab3LBIKb convert rfc822-to-8bit (ORCPT ); Mon, 2 Dec 2013 03:10:31 -0500 From: Marek Vasut To: =?iso-8859-1?q?Bj=F8rn_Erik_Nilsen?= Subject: Re: [PATCH v5 1/2] PCI: designware: Fix crash in dw_msi_teardown_irq Date: Mon, 2 Dec 2013 09:10:26 +0100 Cc: "jg1.han@samsung.com" , "pratyush.anand@gmail.com" , "bhelgaas@google.com" , "linux-pci@vger.kernel.org" , "kishon@ti.com" , "Mohit.KUMAR@st.com" , "ajay.khandelwal@st.com" , "tharvey@gateworks.com" , "Eric.Nelson@boundarydevices.com" , "troy.kisky@boundarydevices.com" References: <003101ceecd5$dd4e79e0$97eb6da0$%han@samsung.com> <201311291802.53655.marex@denx.de> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Message-Id: <201312020910.26538.marex@denx.de> Sender: linux-pci-owner@vger.kernel.org List-ID: Dear Bjørn Erik Nilsen, > 29. nov. 2013 kl. 18:02 skrev Marek Vasut : > > 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,