On 23/05/17 18:54, Mason wrote:

On 23/05/2017 19:03, Bjorn Helgaas wrote:

quoted

On Wed, May 17, 2017 at 04:56:08PM +0200, Marc Gonzalez wrote:

quoted

On 20/04/2017 16:28, Marc Gonzalez wrote:

quoted

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

quoted

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. 

quoted

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.