Messages in this thread | | | Subject | Re: [PATCH v2 1/3] genirq/affinity: Add irq_update_affinity_desc() | From | John Garry <> | Date | Thu, 19 Nov 2020 09:31:56 +0000 |
| |
Hi Thomas,
>>> +int irq_update_affinity_desc(unsigned int irq, >>> + struct irq_affinity_desc *affinity) >> Just a note on the return value, in the only current callsite - >> platform_get_irqs_affinity() - we don't check the return value and >> propagate the error. This is because we don't want to fail the interrupt >> init just because of problems updating the affinity mask. So I could >> print a message to inform the user of error (at the callsite). > Well, not sure about that. During init on a platform which does not have > the issues with reservation mode there failure cases are: > > 1) Interrupt does not exist. Definitely a full fail > > 2) Interrupt is already started up. Not a good idea on init() and > a clear fail. > > 3) Interrupt has already been switched to managed. Double init is not > really a good sign either.
I just tested that and case 3) would be a problem. I don't see us clearing the managed flag when free'ing the interrupt. So with CONFIG_DEBUG_TEST_DRIVER_REMOVE=y, we attempt this affinity update twice, and error from the irqd_affinity_is_managed() check.
> >>> + /* Requires the interrupt to be shut down */ >>> + if (irqd_is_started(&desc->irq_data)) >> We're missing the unlock here, right? > Duh yes. > >>> + return -EBUSY; >>> + >>> + /* Interrupts which are already managed cannot be modified */ >>> + if (irqd_is_managed(&desc->irq_data)) >> And here, and I figure that this should be irqd_affinity_is_managed() > More duh:) > > I assume you send a fixed variant of this.
Can do.
Thanks, John
| |