[PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

[PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

[Shaokun Zhang]

From: Wudi Wang <wangwudi@hisilicon.com>

INVALL CMD specifies that the ITS must ensure any caching associated with
the interrupt collection defined by ICID is consistent with the LPI
configuration tables held in memory for all Redistributors. SYNC is
required to ensure that INVALL is executed.

Currently, LPI configuration data may be inconsistent with that in the
memory within a short period of time after the INVALL command is executed.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <maz@kernel.org>
Signed-off-by: Wudi Wang <wangwudi@hisilicon.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d15366..0cb584d9815b 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -742,7 +742,7 @@ static struct its_collection *its_build_invall_cmd(struct its_node *its,
 
 	its_fixup_cmd(cmd);
 
-	return NULL;
+	return desc->its_invall_cmd.col;
 }
 
 static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,
-- 
2.33.0

Re: [PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

[Marc Zyngier]

On 2021-12-08 01:54, Shaokun Zhang wrote:
> From: Wudi Wang <wangwudi@hisilicon.com>
> 
> INVALL CMD specifies that the ITS must ensure any caching associated 
> with
> the interrupt collection defined by ICID is consistent with the LPI
> configuration tables held in memory for all Redistributors. SYNC is
> required to ensure that INVALL is executed.

The patch title doesn't quite spell out the issue. It should say 
something
like:

"Force synchronisation when issuing INVALL"

> 
> Currently, LPI configuration data may be inconsistent with that in the
> memory within a short period of time after the INVALL command is 
> executed.

I'm curious: have you seen any issue with this on actual HW? In my
experience, all implementations treat INVALL as a synchronous command,

Or was this solely done via inspection?

> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Wudi Wang <wangwudi@hisilicon.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>

This needs:

Fixes: cc2d3216f53 ("irqchip: GICv3: ITS command queue")

> ---
>  drivers/irqchip/irq-gic-v3-its.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index eb0882d15366..0cb584d9815b 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -742,7 +742,7 @@ static struct its_collection
> *its_build_invall_cmd(struct its_node *its,
> 
>  	its_fixup_cmd(cmd);
> 
> -	return NULL;
> +	return desc->its_invall_cmd.col;
>  }
> 
>  static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,

I'll fix the above locally, no need to resend.

Thanks,

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

[irqchip: irq/irqchip-fixes] irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

[irqchip-bot for Wudi Wang]

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     d094b4332232c88d07d9884a9c32fec259984351
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d094b4332232c88d07d9884a9c32fec259984351
Author:        Wudi Wang <wangwudi@hisilicon.com>
AuthorDate:    Wed, 08 Dec 2021 09:54:29 +08:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Wed, 08 Dec 2021 08:34:09 

irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

INVALL CMD specifies that the ITS must ensure any caching associated with
the interrupt collection defined by ICID is consistent with the LPI
configuration tables held in memory for all Redistributors. SYNC is
required to ensure that INVALL is executed.

Currently, LPI configuration data may be inconsistent with that in the
memory within a short period of time after the INVALL command is executed.

Signed-off-by: Wudi Wang <wangwudi@hisilicon.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: cc2d3216f53 ("irqchip: GICv3: ITS command queue")
Link: https://lore.kernel.org/r/20211208015429.5007-1-zhangshaokun@hisilicon.com
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d..0cb584d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -742,7 +742,7 @@ static struct its_collection *its_build_invall_cmd(struct its_node *its,
 
 	its_fixup_cmd(cmd);
 
-	return NULL;
+	return desc->its_invall_cmd.col;
 }
 
 static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,

[irqchip: irq/irqchip-fixes] irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

[irqchip-bot for Wudi Wang]

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     b383a42ca523ce54bcbd63f7c8f3cf974abc9b9a
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/b383a42ca523ce54bcbd63f7c8f3cf974abc9b9a
Author:        Wudi Wang <wangwudi@hisilicon.com>
AuthorDate:    Wed, 08 Dec 2021 09:54:29 +08:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Wed, 08 Dec 2021 11:13:18 

irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

INVALL CMD specifies that the ITS must ensure any caching associated with
the interrupt collection defined by ICID is consistent with the LPI
configuration tables held in memory for all Redistributors. SYNC is
required to ensure that INVALL is executed.

Currently, LPI configuration data may be inconsistent with that in the
memory within a short period of time after the INVALL command is executed.

Signed-off-by: Wudi Wang <wangwudi@hisilicon.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: cc2d3216f53c ("irqchip: GICv3: ITS command queue")
Link: https://lore.kernel.org/r/20211208015429.5007-1-zhangshaokun@hisilicon.com
---
 drivers/irqchip/irq-gic-v3-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d..0cb584d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -742,7 +742,7 @@ static struct its_collection *its_build_invall_cmd(struct its_node *its,
 
 	its_fixup_cmd(cmd);
 
-	return NULL;
+	return desc->its_invall_cmd.col;
 }
 
 static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,

Re: [PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

[Shaokun Zhang]

Hi Marc,

On 2021/12/8 16:25, Marc Zyngier wrote:
> On 2021-12-08 01:54, Shaokun Zhang wrote:
>> From: Wudi Wang <wangwudi@hisilicon.com>
>>
>> INVALL CMD specifies that the ITS must ensure any caching associated with
>> the interrupt collection defined by ICID is consistent with the LPI
>> configuration tables held in memory for all Redistributors. SYNC is
>> required to ensure that INVALL is executed.
> 
> The patch title doesn't quite spell out the issue. It should say something
> like:
> 
> "Force synchronisation when issuing INVALL">

Make sense.

>>
>> Currently, LPI configuration data may be inconsistent with that in the
>> memory within a short period of time after the INVALL command is executed.
> 
> I'm curious: have you seen any issue with this on actual HW? In my
> experience, all implementations treat INVALL as a synchronous command,
> 
> Or was this solely done via inspection?
> 

It is noticed by checking the implementation of INVALL API function, not
by on actual HW.

>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Signed-off-by: Wudi Wang <wangwudi@hisilicon.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> 
> This needs:
> 
> Fixes: cc2d3216f53 ("irqchip: GICv3: ITS command queue")
> 

Oops, indeed, apologies that forget to add this tag.

>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index eb0882d15366..0cb584d9815b 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -742,7 +742,7 @@ static struct its_collection
>> *its_build_invall_cmd(struct its_node *its,
>>
>>      its_fixup_cmd(cmd);
>>
>> -    return NULL;
>> +    return desc->its_invall_cmd.col;
>>  }
>>
>>  static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,
> 
> I'll fix the above locally, no need to resend.
> 

Thanks Marc's help.

> Thanks,
> 
>         M.