* [PATCH] powerpc/vas: Fix IRQ name allocation
@ 2020-12-12 14:27 Cédric Le Goater
2020-12-15 10:56 ` Haren Myneni
2021-02-03 11:40 ` Michael Ellerman
0 siblings, 2 replies; 4+ messages in thread
From: Cédric Le Goater @ 2020-12-12 14:27 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Sukadev Bhattiprolu, Haren Myneni, Cédric Le Goater
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@kaod.org>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/vas: Fix IRQ name allocation
2020-12-12 14:27 [PATCH] powerpc/vas: Fix IRQ name allocation Cédric Le Goater
@ 2020-12-15 10:56 ` Haren Myneni
2020-12-15 12:33 ` Cédric Le Goater
2021-02-03 11:40 ` Michael Ellerman
1 sibling, 1 reply; 4+ messages in thread
From: Haren Myneni @ 2020-12-15 10:56 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev; +Cc: Sukadev Bhattiprolu
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@kaod.org>
Thanks for fixing.
Acked-by: Haren Myneni <haren@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).
>
> 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;
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] powerpc/vas: Fix IRQ name allocation
2020-12-15 10:56 ` Haren Myneni
@ 2020-12-15 12:33 ` Cédric Le Goater
0 siblings, 0 replies; 4+ messages in thread
From: Cédric Le Goater @ 2020-12-15 12:33 UTC (permalink / raw)
To: Haren Myneni, linuxppc-dev; +Cc: Sukadev Bhattiprolu
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@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@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;
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc/vas: Fix IRQ name allocation
2020-12-12 14:27 [PATCH] powerpc/vas: Fix IRQ name allocation Cédric Le Goater
2020-12-15 10:56 ` Haren Myneni
@ 2021-02-03 11:40 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2021-02-03 11:40 UTC (permalink / raw)
To: linuxppc-dev, Cédric Le Goater; +Cc: Sukadev Bhattiprolu, Haren Myneni
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 385 bytes --]
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-03 12:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-12 14:27 [PATCH] powerpc/vas: Fix IRQ name allocation Cédric Le Goater
2020-12-15 10:56 ` Haren Myneni
2020-12-15 12:33 ` Cédric Le Goater
2021-02-03 11:40 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).