Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1643442 > unrolled thread

Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Marc Gonzalez <marc_gonzalez@sigmadesigns.com> - 2017-05-17 17:00 +0200
    Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Bjorn Helgaas <helgaas@kernel.org> - 2017-05-23 19:10 +0200
      Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Mason <slash.tmp@free.fr> - 2017-05-23 20:00 +0200
        Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Robin Murphy <robin.murphy@arm.com> - 2017-05-23 20:10 +0200
          Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Mason <slash.tmp@free.fr> - 2017-05-23 21:20 +0200
            Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Robin Murphy <robin.murphy@arm.com> - 2017-05-24 12:10 +0200
              Re: [PATCH v4 1/2] PCI: Add tango MSI controller support Marc Zyngier <marc.zyngier@arm.com> - 2017-05-24 12:30 +0200

#1643442 — Re: [PATCH v4 1/2] PCI: Add tango MSI controller support

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
...
[    1.135809] [<c0160eb4>] (msi_domain_set_affinity) from [<c015a864>] (irq_do_set_affinity+0x18/0x48)
[    1.144990] [<c015a864>] (irq_do_set_affinity) from [<c015a918>] (setup_affinity+0x84/0xd4)
[    1.153384] [<c015a918>] (setup_affinity) from [<c015b20c>] (__setup_irq+0x40c/0x5d4)
[    1.161254] [<c015b20c>] (__setup_irq) from [<c015b57c>] (request_threaded_irq+0xe4/0x184)
[    1.169569] [<c015b57c>] (request_threaded_irq) from [<c03102e0>] (aer_probe+0x9c/0x218)
[    1.177704] [<c03102e0>] (aer_probe) from [<c030e4bc>] (pcie_port_probe_service+0x34/0x70)
[    1.186017] [<c030e4bc>] (pcie_port_probe_service) from [<c0351cd0>] (really_probe+0x1c4/0x250)
[    1.194763] [<c0351cd0>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
[    1.203245] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
[    1.212076] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
[    1.220296] [<c0351abc>] (__device_attach) from [<c0351ff0>] (device_initial_probe+0xc/0x10)
[    1.228777] [<c0351ff0>] (device_initial_probe) from [<c0351090>] (bus_probe_device+0x84/0x8c)
[    1.237433] [<c0351090>] (bus_probe_device) from [<c034f484>] (device_add+0x3cc/0x548)
[    1.245390] [<c034f484>] (device_add) from [<c034f614>] (device_register+0x14/0x18)
[    1.253086] [<c034f614>] (device_register) from [<c030e8b8>] (pcie_port_device_register+0x3b0/0x450)
[    1.262267] [<c030e8b8>] (pcie_port_device_register) from [<c030ecfc>] (pcie_portdrv_probe+0x2c/0x50)
[    1.271539] [<c030ecfc>] (pcie_portdrv_probe) from [<c0302ba8>] (pci_device_probe+0x7c/0xc8)
[    1.280022] [<c0302ba8>] (pci_device_probe) from [<c0351c84>] (really_probe+0x178/0x250)
[    1.288154] [<c0351c84>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
[    1.296635] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
[    1.305465] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
[    1.313684] [<c0351abc>] (__device_attach) from [<c0351b08>] (device_attach+0xc/0x10)
[    1.321559] [<c0351b08>] (device_attach) from [<c02f9a20>] (pci_bus_add_device+0x44/0x90)
[    1.329778] [<c02f9a20>] (pci_bus_add_device) from [<c02f9aa8>] (pci_bus_add_devices+0x3c/0x80)
[    1.338523] [<c02f9aa8>] (pci_bus_add_devices) from [<c0312dfc>] (pci_host_common_probe+0x100/0x314)
[    1.347703] [<c0312dfc>] (pci_host_common_probe) from [<c0313550>] (tango_pcie_probe+0x138/0x340)
[    1.356624] [<c0313550>] (tango_pcie_probe) from [<c03532c4>] (platform_drv_probe+0x34/0x6c)

[toc] | [next] | [standalone]

#1648282

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?

Or are you asking whether msi_domain_set_affinity() should be changed
to check whether parent->chip->irq_set_affinity is implemented?

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.

> ...
> [    1.135809] [<c0160eb4>] (msi_domain_set_affinity) from [<c015a864>] (irq_do_set_affinity+0x18/0x48)
> [    1.144990] [<c015a864>] (irq_do_set_affinity) from [<c015a918>] (setup_affinity+0x84/0xd4)
> [    1.153384] [<c015a918>] (setup_affinity) from [<c015b20c>] (__setup_irq+0x40c/0x5d4)
> [    1.161254] [<c015b20c>] (__setup_irq) from [<c015b57c>] (request_threaded_irq+0xe4/0x184)
> [    1.169569] [<c015b57c>] (request_threaded_irq) from [<c03102e0>] (aer_probe+0x9c/0x218)
> [    1.177704] [<c03102e0>] (aer_probe) from [<c030e4bc>] (pcie_port_probe_service+0x34/0x70)
> [    1.186017] [<c030e4bc>] (pcie_port_probe_service) from [<c0351cd0>] (really_probe+0x1c4/0x250)
> [    1.194763] [<c0351cd0>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
> [    1.203245] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
> [    1.212076] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
> [    1.220296] [<c0351abc>] (__device_attach) from [<c0351ff0>] (device_initial_probe+0xc/0x10)
> [    1.228777] [<c0351ff0>] (device_initial_probe) from [<c0351090>] (bus_probe_device+0x84/0x8c)
> [    1.237433] [<c0351090>] (bus_probe_device) from [<c034f484>] (device_add+0x3cc/0x548)
> [    1.245390] [<c034f484>] (device_add) from [<c034f614>] (device_register+0x14/0x18)
> [    1.253086] [<c034f614>] (device_register) from [<c030e8b8>] (pcie_port_device_register+0x3b0/0x450)
> [    1.262267] [<c030e8b8>] (pcie_port_device_register) from [<c030ecfc>] (pcie_portdrv_probe+0x2c/0x50)
> [    1.271539] [<c030ecfc>] (pcie_portdrv_probe) from [<c0302ba8>] (pci_device_probe+0x7c/0xc8)
> [    1.280022] [<c0302ba8>] (pci_device_probe) from [<c0351c84>] (really_probe+0x178/0x250)
> [    1.288154] [<c0351c84>] (really_probe) from [<c0351ec8>] (__device_attach_driver+0xa4/0xe0)
> [    1.296635] [<c0351ec8>] (__device_attach_driver) from [<c0350274>] (bus_for_each_drv+0x60/0x94)
> [    1.305465] [<c0350274>] (bus_for_each_drv) from [<c0351abc>] (__device_attach+0x9c/0xdc)
> [    1.313684] [<c0351abc>] (__device_attach) from [<c0351b08>] (device_attach+0xc/0x10)
> [    1.321559] [<c0351b08>] (device_attach) from [<c02f9a20>] (pci_bus_add_device+0x44/0x90)
> [    1.329778] [<c02f9a20>] (pci_bus_add_device) from [<c02f9aa8>] (pci_bus_add_devices+0x3c/0x80)
> [    1.338523] [<c02f9aa8>] (pci_bus_add_devices) from [<c0312dfc>] (pci_host_common_probe+0x100/0x314)
> [    1.347703] [<c0312dfc>] (pci_host_common_probe) from [<c0313550>] (tango_pcie_probe+0x138/0x340)
> [    1.356624] [<c0313550>] (tango_pcie_probe) from [<c03532c4>] (platform_drv_probe+0x34/0x6c)
> 

[toc] | [prev] | [next] | [standalone]

#1648306

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.)

I'll send a patch illustrating exactly what I intended.

Regards.

[toc] | [prev] | [next] | [standalone]

#1648312

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().

Robin.

> 
> I'll send a patch illustrating exactly what I intended.
> 
> Regards.
> 

[toc] | [prev] | [next] | [standalone]

#1648355

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.

[toc] | [prev] | [next] | [standalone]

#1649410

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()?

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.

Robin.

[toc] | [prev] | [next] | [standalone]

#1649442

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.

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web