Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <17b5e57b-326e-11c1-d8c7-87f3d3d7d0ff@huawei.com>
Date:   Thu, 4 Aug 2022 11:59:05 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.1
Subject: Re: PCI MSI issue with reinserting a driver
From:   John Garry <john.garry@huawei.com>
To:     Marc Zyngier <maz@kernel.org>
CC:     Thomas Gleixner <tglx@linutronix.de>,
        Zhou Wang <wangzhou1@hisilicon.com>,
        <linux-kernel@vger.kernel.org>, <shenyang39@huawei.com>,
        "zhanjie (F)" <zhanjie9@hisilicon.com>
References: <cc224272-15db-968b-46a0-95951e11b23f@huawei.com>
 <87o8h3lj0n.wl-maz@kernel.org>
 <a80b9be0-c455-c852-ddac-3f514a15e896@huawei.com>
 <8a54fdd0-950b-f801-e83d-750aef73ab3c@huawei.com>
 <4848792ce8c9ed7490e2205281a3cbda@kernel.org>
 <28c56995-501a-880b-e6dd-ac76b8290c2c@huawei.com>
 <3d3d0155e66429968cb4f6b4feeae4b3@kernel.org>
 <fd88ce05-8aee-5b1f-5ab6-be88fa53d3aa@huawei.com>
In-Reply-To: <fd88ce05-8aee-5b1f-5ab6-be88fa53d3aa@huawei.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 04/02/2021 10:45, John Garry wrote:

Hi Marc,

Just a friendly reminder that we still have the issue with reinseting a 
PCI driver which does not allocate a power-of-2 MSIs.

> On 03/02/2021 17:23, Marc Zyngier wrote:
>>>
>>> Before:
>>>  In free path, we have:
>>>      its_irq_domain_free(nvecs = 27)
>>>        bitmap_release_region(count order = 5 == 32bits)
>>>
>>> Current:
>>>  In free path, we have:
>>>      its_irq_domain_free(nvecs = 1) for free each 27 vecs
>>>        bitmap_release_region(count order = 0 == 1bit)
>>
>> Right. I was focusing on the patch and blindly ignored the explanation
>> at the top of the email. Apologies for that.
> 
> Yeah, it was a distraction.
> 
>>
>> I'm not overly keen on handling this in the ITS though, and I'd rather
>> we try and do it in the generic code. How about this (compile tested
>> only)?
>>
>> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
>> index 6aacd342cd14..cfccad83c2df 100644
>> --- a/kernel/irq/irqdomain.c
>> +++ b/kernel/irq/irqdomain.c
>> @@ -1399,8 +1399,19 @@ static void 
>> irq_domain_free_irqs_hierarchy(struct irq_domain *domain,
>>           return;
>>
>>       for (i = 0; i < nr_irqs; i++) {
>> -        if (irq_domain_get_irq_data(domain, irq_base + i))
>> -            domain->ops->free(domain, irq_base + i, 1);
>> +        int n ;
>> +
>> +        /* Find the largest possible span of IRQs to free in one go */
>> +        for (n = 0;
>> +             ((i + n) < nr_irqs &&
>> +              irq_domain_get_irq_data(domain, irq_base + i + n));
>> +             n++);
>> +
>> +        if (!n)
>> +            continue;
>> +
>> +        domain->ops->free(domain, irq_base + i, n);
>> +        i += n;
>>       }
>>   }
> 
> That fixed my problem.
> 
> For my benefit, if MSIs must be allocated in power of 2, could we check 
> a flag for the dealloc method? Like, if MSI domain, then do as before 
> 4615fbc3788d. But I'm not sure on the specific scenario which that 
> commit fixed. Or even whether you want specifics like that in core code.

Thanks,
John