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

[Robin Murphy]

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