From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031005AbdEWTPy (ORCPT ); Tue, 23 May 2017 15:15:54 -0400 Received: from smtp2-g21.free.fr ([212.27.42.2]:20255 "EHLO smtp2-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030963AbdEWTPv (ORCPT ); Tue, 23 May 2017 15:15:51 -0400 Subject: Re: [PATCH v4 1/2] PCI: Add tango MSI controller support To: Robin Murphy , Bjorn Helgaas Cc: Marc Gonzalez , Marc Zyngier , Thomas Gleixner , Jiang Liu , Lorenzo Pieralisi , Liviu Dudau , David Laight , linux-pci , Linux ARM , Thibaud Cornic , Phuong Nguyen , LKML 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: Tue, 23 May 2017 21:15:16 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Regards.