Re: [PATCH 08/16] xen/events: allow setup of irq_info to fail

[David Vrabel <david.vrabel@citrix.com>]

On 14/10/13 18:26, Boris Ostrovsky wrote:
> On 10/08/2013 08:49 AM, David Vrabel wrote:
>> From: David Vrabel <david.vrabel@citrix.com>
>>
>> The FIFO-based event ABI requires additional setup of newly bound
>> events (it may need to expand the event array) and this setup may
>> fail.
>>
>> xen_irq_info_common_init() is a useful place to put this setup so
>> allow this call to fail.  This call and the other similar calls are
>> renamed to be *_setup() to reflect that they may now fail.
>>
>> This failure can only occur with new event channels not on rebind.
[...]
>> --- a/drivers/xen/events/events.c
>> +++ b/drivers/xen/events/events.c
[...]
>> +static int xen_irq_info_ipi_setup(unsigned cpu,
>>                     unsigned irq,
>>                     unsigned short evtchn,
>>                     enum ipi_vector ipi)
>>   {
>>       struct irq_info *info = info_for_irq(irq);
>>   -    xen_irq_info_common_init(info, irq, IRQT_IPI, evtchn, 0);
>> -
>>       info->u.ipi = ipi;
>>         per_cpu(ipi_to_irq, cpu)[ipi] = irq;
>> +
>> +    return xen_irq_info_common_setup(info, irq, IRQT_IPI, evtchn, 0);
> 
> Do you need to cleanup on error here (and in similar routines below)? I.e.
> 
>      per_cpu(ipi_to_irq, cpu)[ipi] = -1;
> 
> You may be trying to do this in __unbind_from _irq() but I think a
> routine that fails shouldn't expects others to clean up after it. If
> __unbind from irq() is used in other contexts (it probably is) then
> perhaps you can factor our the part that is relevant to setup routines
> only and call it from here.

It's a lot easier to do all the cleanup in __unbind_from_irq(), no
matter what (part-initialized) state the irq is in.

>>   +static void __unbind_from_irq(unsigned int irq)
>> +{
>> +    struct evtchn_close close;
>> +    int evtchn = evtchn_from_irq(irq);
>> +    struct irq_info *info = irq_get_handler_data(irq);
>> +
>> +    if (info->refcnt > 0) {
> 
> info may be NULL.

I couldn't see how.

There used to be a WARN_ON(!info) but I've never seen reports of this
ever triggering and I couldn't find a case where it might.

David