Re: [PATCH] genirq/msi: Activate Multi-MSI early when MSI_FLAG_ACTIVATE_EARLY is set

[Marc Zyngier <maz@kernel.org>]

On 2021-01-25 14:39, Shameerali Kolothum Thodi wrote:
>> -----Original Message-----
>> From: Marc Zyngier [mailto:maz@kernel.org]
>> Sent: 23 January 2021 12:28
>> To: linux-kernel@vger.kernel.org
>> Cc: Thomas Gleixner <tglx@linutronix.de>; Bjorn Helgaas
>> <bhelgaas@google.com>; Shameerali Kolothum Thodi
>> <shameerali.kolothum.thodi@huawei.com>; stable@vger.kernel.org
>> Subject: [PATCH] genirq/msi: Activate Multi-MSI early when
>> MSI_FLAG_ACTIVATE_EARLY is set
>> 
>> When MSI_FLAG_ACTIVATE_EARLY is set (which is the case for PCI),
>> we perform the activation of the interrupt (which in the case of
>> PCI results in the endpoint being programmed) as soon as the
>> interrupt is allocated.
>> 
>> But it appears that this is only done for the first vector,
>> introducing an inconsistent behaviour for PCI Multi-MSI.
>> 
>> Fix it by iterating over the number of vectors allocated to
>> each MSI descriptor. This is easily achieved by introducing
>> a new "for_each_msi_vector" iterator, together with a tiny
>> bit of refactoring.
>> 
>> Fixes: f3b0946d629c ("genirq/msi: Make sure PCI MSIs are activated 
>> early")
>> Reported-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> Cc: stable@vger.kernel.org
>> ---
>>  include/linux/msi.h |  6 ++++++
>>  kernel/irq/msi.c    | 44 ++++++++++++++++++++------------------------
>>  2 files changed, 26 insertions(+), 24 deletions(-)
>> 
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index 360a0a7e7341..aef35fd1cf11 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -178,6 +178,12 @@ struct msi_desc {
>>  	list_for_each_entry((desc), dev_to_msi_list((dev)), list)
>>  #define for_each_msi_entry_safe(desc, tmp, dev)	\
>>  	list_for_each_entry_safe((desc), (tmp), dev_to_msi_list((dev)), 
>> list)
>> +#define for_each_msi_vector(desc, __irq, dev)				\
>> +	for_each_msi_entry((desc), (dev))				\
>> +		if ((desc)->irq)					\
>> +			for (__irq = (desc)->irq;			\
>> +			     __irq < ((desc)->irq + (desc)->nvec_used);	\
>> +			     __irq++)
>> 
>>  #ifdef CONFIG_IRQ_MSI_IOMMU
>>  static inline const void *msi_desc_get_iommu_cookie(struct msi_desc 
>> *desc)
>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
>> index 2c0c4d6d0f83..d924676c8781 100644
>> --- a/kernel/irq/msi.c
>> +++ b/kernel/irq/msi.c
>> @@ -436,22 +436,22 @@ int __msi_domain_alloc_irqs(struct irq_domain
>> *domain, struct device *dev,
>> 
>>  	can_reserve = msi_check_reservation_mode(domain, info, dev);
>> 
>> -	for_each_msi_entry(desc, dev) {
>> -		virq = desc->irq;
>> -		if (desc->nvec_used == 1)
>> -			dev_dbg(dev, "irq %d for MSI\n", virq);
>> -		else
>> +	/*
>> +	 * This flag is set by the PCI layer as we need to activate
>> +	 * the MSI entries before the PCI layer enables MSI in the
>> +	 * card. Otherwise the card latches a random msi message.
>> +	 */
>> +	if (!(info->flags & MSI_FLAG_ACTIVATE_EARLY))
>> +		goto skip_activate;
> 
> This will change the dbg print behavior. From the commit f3b0946d629c,
> it looks like the below dev_dbg() code was there for 
> !MSI_FLAG_ACTIVATE_EARLY
> case as well. Not sure how much this matters though.

I'm not sure this matters either. We may have relied on these statements
some 6/7 years ago, as the whole hierarchy stuff was brand new, but we
now have a much better debug infrastructure thanks to Thomas. I'd be
totally in favour of dropping it.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...