From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCH 08/16] xen/events: allow setup of irq_info to fail Date: Tue, 15 Oct 2013 20:20:54 +0100 Message-ID: <525D9596.9080506@citrix.com> References: <1381236555-27493-1-git-send-email-david.vrabel@citrix.com> <1381236555-27493-9-git-send-email-david.vrabel@citrix.com> <525C2957.1020001@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <525C2957.1020001@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky Cc: Jan Beulich , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 14/10/13 18:26, Boris Ostrovsky wrote: > On 10/08/2013 08:49 AM, David Vrabel wrote: >> From: David Vrabel >> >> The FIFO-based event ABI requires additional setup of newly bound >> events (it may need to expand the event array) and this setup may >> fail. >> >> xen_irq_info_common_init() is a useful place to put this setup so >> allow this call to fail. This call and the other similar calls are >> renamed to be *_setup() to reflect that they may now fail. >> >> This failure can only occur with new event channels not on rebind. [...] >> --- a/drivers/xen/events/events.c >> +++ b/drivers/xen/events/events.c [...] >> +static int xen_irq_info_ipi_setup(unsigned cpu, >> unsigned irq, >> unsigned short evtchn, >> enum ipi_vector ipi) >> { >> struct irq_info *info = info_for_irq(irq); >> - xen_irq_info_common_init(info, irq, IRQT_IPI, evtchn, 0); >> - >> info->u.ipi = ipi; >> per_cpu(ipi_to_irq, cpu)[ipi] = irq; >> + >> + return xen_irq_info_common_setup(info, irq, IRQT_IPI, evtchn, 0); > > Do you need to cleanup on error here (and in similar routines below)? I.e. > > per_cpu(ipi_to_irq, cpu)[ipi] = -1; > > You may be trying to do this in __unbind_from _irq() but I think a > routine that fails shouldn't expects others to clean up after it. If > __unbind from irq() is used in other contexts (it probably is) then > perhaps you can factor our the part that is relevant to setup routines > only and call it from here. It's a lot easier to do all the cleanup in __unbind_from_irq(), no matter what (part-initialized) state the irq is in. >> +static void __unbind_from_irq(unsigned int irq) >> +{ >> + struct evtchn_close close; >> + int evtchn = evtchn_from_irq(irq); >> + struct irq_info *info = irq_get_handler_data(irq); >> + >> + if (info->refcnt > 0) { > > info may be NULL. I couldn't see how. There used to be a WARN_ON(!info) but I've never seen reports of this ever triggering and I couldn't find a case where it might. David