Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
Date:   Thu, 17 Feb 2022 17:17:06 +0000
From:   Marc Zyngier <maz@kernel.org>
To:     John Garry <john.garry@huawei.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Marcin Wojtas <mw@semihalf.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>, kernel-team@android.com
Subject: Re: [PATCH 1/2] genirq: Extract irq_set_affinity_masks() from
 devm_platform_get_irqs_affinity()
In-Reply-To: <bdfe935b-6ee0-b588-e1e8-776d85f91813@huawei.com>
References: <20220216090845.1278114-1-maz@kernel.org>
 <20220216090845.1278114-2-maz@kernel.org>
 <bdfe935b-6ee0-b588-e1e8-776d85f91813@huawei.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <b502141b201a68eb4896c1653b67663a@kernel.org>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi John,

On 2022-02-17 17:07, John Garry wrote:
> On 16/02/2022 09:08, Marc Zyngier wrote:
> 
> Hi Marc,
> 
>> In order to better support drivers that deal with interrupts in a more
>> "hands-on" way, extract the core of devm_platform_get_irqs_affinity()
>> and expose it as irq_set_affinity_masks().
>> 
>> This helper allows a driver to provide a set of wired interrupts that
>> are to be configured as managed interrupts. As with the original 
>> helper,
>> this is exported as EXPORT_SYMBOL_GPL.
> 
> I know you mentioned it in 2/2, but it would be interesting to see how
> network controller drivers can handle the problem of missing in-flight
> IO completions for managed irq shutdown. For storage controllers this
> is all now safely handled in the block layer.

Do you have a pointer to this? It'd be interesting to see if there is
a common pattern.

> 
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> Just a small comment, below.
> 
> Tested-by: John Garry <john.garry@huawei.com> #D05
> 
> Thanks,
> John
> 
>> ---
>>   drivers/base/platform.c   | 20 +++-----------------
>>   include/linux/interrupt.h |  8 ++++++++
>>   kernel/irq/affinity.c     | 27 +++++++++++++++++++++++++++
>>   3 files changed, 38 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index 6cb04ac48bf0..b363cf6ce5be 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -335,7 +335,6 @@ int devm_platform_get_irqs_affinity(struct 
>> platform_device *dev,
>>   				    int **irqs)
>>   {
>>   	struct irq_affinity_devres *ptr;
>> -	struct irq_affinity_desc *desc;
>>   	size_t size;
>>   	int i, ret, nvec;
>>   @@ -376,31 +375,18 @@ int devm_platform_get_irqs_affinity(struct 
>> platform_device *dev,
>>   		ptr->irq[i] = irq;
>>   	}
>>   -	desc = irq_create_affinity_masks(nvec, affd);
>> -	if (!desc) {
>> -		ret = -ENOMEM;
>> +	ret = irq_set_affinity_masks(affd, ptr->irq, nvec);
>> +	if (ret) {
>> +		dev_err(&dev->dev, "failed to update affinity descriptors (%d)\n", 
>> ret);
>>   		goto err_free_devres;
>>   	}
>>   -	for (i = 0; i < nvec; i++) {
>> -		ret = irq_update_affinity_desc(ptr->irq[i], &desc[i]);
>> -		if (ret) {
>> -			dev_err(&dev->dev, "failed to update irq%d affinity descriptor 
>> (%d)\n",
>> -				ptr->irq[i], ret);
>> -			goto err_free_desc;
>> -		}
>> -	}
>> -
>>   	devres_add(&dev->dev, ptr);
>>   -	kfree(desc);
>> -
>>   	*irqs = ptr->irq;
>>     	return nvec;
>>   -err_free_desc:
>> -	kfree(desc);
>>   err_free_devres:
>>   	devres_free(ptr);
>>   	return ret;
>> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
>> index 9367f1cb2e3c..6bfce96206f8 100644
>> --- a/include/linux/interrupt.h
>> +++ b/include/linux/interrupt.h
>> @@ -381,6 +381,8 @@ irq_create_affinity_masks(unsigned int nvec, 
>> struct irq_affinity *affd);
>>   unsigned int irq_calc_affinity_vectors(unsigned int minvec, unsigned 
>> int maxvec,
>>   				       const struct irq_affinity *affd);
>>   +int irq_set_affinity_masks(struct irq_affinity *affd, int *irqs, 
>> int nvec);
>> +
>>   #else /* CONFIG_SMP */
>>     static inline int irq_set_affinity(unsigned int irq, const struct 
>> cpumask *m)
>> @@ -443,6 +445,12 @@ irq_calc_affinity_vectors(unsigned int minvec, 
>> unsigned int maxvec,
>>   	return maxvec;
>>   }
>>   +static inline int
>> +irq_set_affinity_masks(struct irq_affinity *affd, int *irqs, int 
>> nvec)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>>   #endif /* CONFIG_SMP */
>>     /*
>> diff --git a/kernel/irq/affinity.c b/kernel/irq/affinity.c
>> index f7ff8919dc9b..c0f868cd5b87 100644
>> --- a/kernel/irq/affinity.c
>> +++ b/kernel/irq/affinity.c
>> @@ -512,3 +512,30 @@ unsigned int irq_calc_affinity_vectors(unsigned 
>> int minvec, unsigned int maxvec,
>>     	return resv + min(set_vecs, maxvec - resv);
>>   }
>> +
>> +/*
>> + * irq_set_affinity_masks - Set the affinity masks of a number of 
>> interrupts
>> + *                          for multiqueue spreading
>> + * @affd:	Description of the affinity requirements
>> + * @irqs:	An array of interrupt numbers
>> + * @nvec:	The total number of interrupts
>> + */
>> +int irq_set_affinity_masks(struct irq_affinity *affd, int *irqs, int 
>> nvec)
>> +{
>> +	struct irq_affinity_desc *desc;
>> +	int i, err = 0;
> 
> nit: it might be worth doing something similar to how
> pci_alloc_irq_vectors_affinity() handles sets with no pre- and
> post-vectors with msi_default_affd

Yes, good point. This would probably simplify most callers of this code.

Thanks,

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