[PATCH] powerpc/vas: Fix IRQ name allocation

[Cédric Le Goater]

On 12/15/20 11:56 AM, Haren Myneni wrote:
> On Sat, 2020-12-12 at 15:27 +0100, Cédric Le Goater wrote:
>> The VAS device allocates a generic interrupt to handle page faults
>> but
>> the IRQ name doesn't show under /proc. This is because it's on
>> stack. Allocate the name.
>>
>> Signed-off-by: Cédric Le Goater <clg at kaod.org>
> 
> Thanks for fixing.

I was wondering where those ^B interrupt numbers were coming from.

/proc/interrupts looks better now: 

     36:  ...   0  XIVE-IRQ 50331732 Edge      vas-6
     40:  ...   0  XIVE-IRQ 33554504 Edge      vas-4
     72:  ...   0  XIVE-IRQ 16777304 Edge      vas-2
    124:  ...   0  XIVE-IRQ      124 Edge      vas-0


> 
> Acked-by: Haren Myneni <haren at linux.ibm.com>
> 
>> ---
>>
>>  I didn't understand this part in init_vas_instance() :
>>
>> 	if (vinst->virq) {
>> 		rc = vas_irq_fault_window_setup(vinst);
>> 		/*
>> 		 * Fault window is used only for user space send
>> windows.
>> 		 * So if vinst->virq is NULL, tx_win_open returns
>> -ENODEV
>> 		 * for user space.
>> 		 */
>> 		if (rc)
>> 			vinst->virq = 0;
>> 	}
>>
>>  If the IRQ cannot be requested, the device probing should fail but
>>  it's not today. The use of 'vinst->virq' is suspicious.
> 
> VAS raises an interrupt only when NX sees fault on request buffers and
> faults can happen only for user space requests. So Fault window setup
> is needed for user space requests. For kernel requests, continue even
> if IRQ / fault_window_setup is failed. 
>
> When window open request is issued from user space, kernel returns
> -ENODEV if vinst->virq = 0 (means fault window setup is failed). 

It looks ok to deactivate a feature (page faulting for user space 
requests) if vas_setup_fault_window() fails but if the IRQ layer 
routine request_threaded_irq() fails, something is really wrong 
in the system and we should stop probing IMO.

We should probably move the IRQ request after allocating/mapping 
the XIVE IPI IRQ.

this test is always true : 

	if (vinst->virq) {
		rc = vas_irq_fault_window_setup(vinst);
	
since above, we did : 

	vinst->virq = irq_create_mapping(NULL, hwirq);
	if (!vinst->virq) {
		pr_err("Inst%d: Unable to map global irq %d\n",
				vinst->vas_id, hwirq);
		return -EINVAL;
	}

Cheers,

C.


> 
> 
>>
>>  arch/powerpc/platforms/powernv/vas.h |  1 +
>>  arch/powerpc/platforms/powernv/vas.c | 11 ++++++++---
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/powernv/vas.h
>> b/arch/powerpc/platforms/powernv/vas.h
>> index 70f793e8f6cc..c7db3190baca 100644
>> --- a/arch/powerpc/platforms/powernv/vas.h
>> +++ b/arch/powerpc/platforms/powernv/vas.h
>> @@ -340,6 +340,7 @@ struct vas_instance {
>>  	struct vas_window *rxwin[VAS_COP_TYPE_MAX];
>>  	struct vas_window *windows[VAS_WINDOWS_PER_CHIP];
>>  
>> +	char *name;
>>  	char *dbgname;
>>  	struct dentry *dbgdir;
>>  };
>> diff --git a/arch/powerpc/platforms/powernv/vas.c
>> b/arch/powerpc/platforms/powernv/vas.c
>> index 598e4cd563fb..b65256a63e87 100644
>> --- a/arch/powerpc/platforms/powernv/vas.c
>> +++ b/arch/powerpc/platforms/powernv/vas.c
>> @@ -28,12 +28,10 @@ static DEFINE_PER_CPU(int, cpu_vas_id);
>>  
>>  static int vas_irq_fault_window_setup(struct vas_instance *vinst)
>>  {
>> -	char devname[64];
>>  	int rc = 0;
>>  
>> -	snprintf(devname, sizeof(devname), "vas-%d", vinst->vas_id);
>>  	rc = request_threaded_irq(vinst->virq, vas_fault_handler,
>> -				vas_fault_thread_fn, 0, devname,
>> vinst);
>> +				vas_fault_thread_fn, 0, vinst->name,
>> vinst);
>>  
>>  	if (rc) {
>>  		pr_err("VAS[%d]: Request IRQ(%d) failed with %d\n",
>> @@ -80,6 +78,12 @@ static int init_vas_instance(struct
>> platform_device *pdev)
>>  	if (!vinst)
>>  		return -ENOMEM;
>>  
>> +	vinst->name = kasprintf(GFP_KERNEL, "vas-%d", vasid);
>> +	if (!vinst->name) {
>> +		kfree(vinst);
>> +		return -ENOMEM;
>> +	}
>> +
>>  	INIT_LIST_HEAD(&vinst->node);
>>  	ida_init(&vinst->ida);
>>  	mutex_init(&vinst->mutex);
>> @@ -162,6 +166,7 @@ static int init_vas_instance(struct
>> platform_device *pdev)
>>  	return 0;
>>  
>>  free_vinst:
>> +	kfree(vinst->name);
>>  	kfree(vinst);
>>  	return -ENODEV;
>>  
>