From patchwork Wed May 31 14:18:44 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mason X-Patchwork-Id: 9757369 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id CFB6860390 for ; Wed, 31 May 2017 14:19:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BD66327FC0 for ; Wed, 31 May 2017 14:19:38 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B06F2283C9; Wed, 31 May 2017 14:19:38 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.4 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, FREEMAIL_FROM, RCVD_IN_SORBS_SPAM autolearn=no version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 53B6D27FC0 for ; Wed, 31 May 2017 14:19:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=oxrFXJs9TAJxIYAHJWmM32jn5Jz5jMPMxcIluX88FI0=; b=KI0peCz9qMTX/v Ushn+Az5xQRlOxZn3Bn634yDGNHeMpoRSPiTWrn3SwJsWMohoYI5Gqs+bsVspfygGOHCLUM16AFHr SGw6ud5yWlunp+vLwnK16PKq8AsmlPKQqrxG/AI0HyzU1zJixAjymJbU/cFlUIHl+T/mh8df+aeeD wyUJefRfJYXJqzCHiyMksuGhrogoiAwnYiPRAhOSMNiEWnzwLuvojpA+eMaeoTYrP4Kro/wvi0/t+ f5apJtaVh3lHfxpYTHdOryz+vC0jPZN92RpO7xHamU/hRGp9g8Wuwo5ARpWjdJwsoHJO2HdBDRc3U vPOFhiwS3hA8z9CX+G+A==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dG4TJ-0004kj-Fs; Wed, 31 May 2017 14:19:37 +0000 Received: from smtp5-g21.free.fr ([212.27.42.5]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dG4TF-0004Yx-Oc for linux-arm-kernel@lists.infradead.org; Wed, 31 May 2017 14:19:36 +0000 Received: from [172.27.0.114] (unknown [92.154.11.170]) (Authenticated sender: slash.tmp) by smtp5-g21.free.fr (Postfix) with ESMTPSA id 15C395FFFD; Wed, 31 May 2017 16:18:45 +0200 (CEST) Subject: Re: [PATCH v4 1/2] PCI: Add tango MSI controller support To: Marc Zyngier , Robin Murphy References: <1f6b50c7-4887-838e-8c7d-c014d82b6d8e@sigmadesigns.com> <8e56647d-3b2d-9bfc-b59e-79a2efaa7826@sigmadesigns.com> <20170523170319.GB24431@bhelgaas-glaptop.roam.corp.google.com> From: Mason Message-ID: Date: Wed, 31 May 2017 16:18:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49 MIME-Version: 1.0 In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170531_071934_126094_084F29CF X-CRM114-Status: GOOD ( 21.17 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Lorenzo Pieralisi , linux-pci , Liviu Dudau , LKML , David Laight , Bjorn Helgaas , Thomas Gleixner , Linux ARM , Marc Gonzalez Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 24/05/2017 12:22, Marc Zyngier wrote: > On 24/05/17 11:00, Robin Murphy wrote: >> On 23/05/17 20:15, Mason wrote: >>> On 23/05/2017 20:03, Robin Murphy wrote: >>>> On 23/05/17 18:54, Mason wrote: >>>>> On 23/05/2017 19:03, Bjorn Helgaas wrote: >>>>>> On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote: >>>>>>> On 20/04/2017 16:28, Marc Gonzalez wrote: >>>>>>> >>>>>>>> +static int tango_set_affinity(struct irq_data *data, >>>>>>>> + const struct cpumask *mask, bool force) >>>>>>>> +{ >>>>>>>> + return -EINVAL; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static struct irq_chip tango_chip = { >>>>>>>> + .irq_ack = tango_ack, >>>>>>>> + .irq_mask = tango_mask, >>>>>>>> + .irq_unmask = tango_unmask, >>>>>>>> + .irq_set_affinity = tango_set_affinity, >>>>>>>> + .irq_compose_msi_msg = tango_compose_msi_msg, >>>>>>>> +}; >>>>>>> >>>>>>> Hmmm... I'm wondering why .irq_set_affinity is required. >>>>>>> >>>>>>> static int setup_affinity(struct irq_desc *desc, struct cpumask *mask) >>>>>>> first calls __irq_can_set_affinity() to check whether >>>>>>> desc->irq_data.chip->irq_set_affinity) exists. >>>>>>> >>>>>>> then calls irq_do_set_affinity(&desc->irq_data, mask, false); >>>>>>> which calls chip->irq_set_affinity(data, mask, force); >>>>>>> = msi_domain_set_affinity() >>>>>>> which calls parent->chip->irq_set_affinity() unconditionally. >>>>>>> >>>>>>> Would it make sense to test that the callback is implemented >>>>>>> before calling it? >>>>>>> >>>>>>> >>>>>>> [ 0.723895] Unable to handle kernel NULL pointer dereference at virtual address 00000000 >>>>>> >>>>>> I'm not sure what you're asking. >>>>>> >>>>>> Is this a bug report for the v4 tango driver? >>>>> >>>>> No. >>>>> >>>>>> Or are you asking whether msi_domain_set_affinity() should be changed >>>>>> to check whether parent->chip->irq_set_affinity is implemented? >>>>> >>>>> Yes. The way things are implemented now, drivers are forced >>>>> to define an irq_set_affinity callback, even if it just returns >>>>> an error, otherwise, the kernel crashes, because of the >>>>> unconditional function pointer deref. >>>>> >>>>>> msi_domain_set_affinity() has called parent->chip->irq_set_affinity() >>>>>> without checking since it was added in 2014 by f3cf8bb0d6c3 ("genirq: Add >>>>>> generic msi irq domain support"), so if there's a problem here, it's most >>>>>> likely in the tango code. >>>>> >>>>> The issue is having to define an "empty" function. >>>>> (Unnecessary code bloat and maintenance.) >>>> >>>> AFAICS, only one driver (other than this one) implements a "do nothing" >>>> set_affinity callback - everyone else who doesn't do anything hardware >>>> specific just passes it along via irq_chip_set_affinity_parent(). >>> >>> I counted 4. Where did I mess up? >>> >>> advk_msi_set_affinity >>> altera_msi_set_affinity >>> nwl_msi_set_affinity >>> vmd_irq_set_affinity >>> tango_set_affinity >> >> Fair point - I went through drivers/irqchip and the various >> arch-specific instances and found ls_scfg_msi_set_affinity(), but >> somehow skipped over drivers/pci. Anyway, I think the question stands of >> why are these handful of drivers *not* using irq_chip_set_affinity_parent()? > > Probably because they don't have a parent, in the hierarchical sense. > All they have is a chained interrupt (*puke*). Which implies that > changing the affinity of one MSI would affect all of them, completely > confusing unsuspecting userspace such as irqbalance. > >> As an outsider, it naively seems logical that the affinity of an MSI >> which just gets translated to a wired interrupt should propagate through >> to the affinity of that wired interrupt, but maybe there are reasons not >> to; I really don't know. > > See above. The main issue is that HW folks haven't understood the actual > use of MSIs, and persist in implementing them as an afterthought, based > on some cascading interrupt controller design. > > Sad. So sad. For the record, below is the patch I had in mind: Then, it would be safe to remove "do-nothing" .irq_set_affinity callbacks in drivers/pci/host Regards. diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index 8a3e872798f3..edfc95575a37 100644 --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -91,9 +91,11 @@ int msi_domain_set_affinity(struct irq_data *irq_data, { struct irq_data *parent = irq_data->parent_data; struct msi_msg msg; - int ret; + int ret = -EINVAL; + + if (parent->chip->irq_set_affinity) + ret = parent->chip->irq_set_affinity(parent, mask, force); - ret = parent->chip->irq_set_affinity(parent, mask, force); if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) { BUG_ON(irq_chip_compose_msi_msg(irq_data, &msg)); irq_chip_write_msi_msg(irq_data, &msg);