Re: [PATCH] powerpc/vas: Fix IRQ name allocation
On Sat, 12 Dec 2020 15:27:07 +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. Applied to powerpc/next. [1/1] powerpc/vas: Fix IRQ name allocation https://git.kernel.org/powerpc/c/9dd31b11370380c488c8f2d347058617cd3fff99 cheers
Re: [PATCH] powerpc/vas: Fix IRQ name allocation
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
>
> 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
>
>> ---
>>
>> 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;
>>
>
Re: [PATCH] powerpc/vas: Fix IRQ name allocation
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
Thanks for fixing.
Acked-by: Haren Myneni
> ---
>
> 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).
>
> 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;
>
[PATCH] powerpc/vas: Fix IRQ name allocation
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
---
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.
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;
--
2.26.2
