[PATCH] irqchip/mbigen: Fix mbigen node address layout

[PATCH] irqchip/mbigen: Fix mbigen node address layout

[Yipeng Zou]

Mbigen chip contains several mbigen nodes, and mapped address space per
nodes one by one.

                    mbigen chip
       |-----------------|------------|--------------|
   mgn_node_0         mgn_node_1     ...         mgn_node_i
|--------------|   |--------------|       |----------------------|
[0x0000, 0x1000)   [0x1000, 0x2000)       [i*0x1000, (i+1)*0x1000)

Mbigen also defined a clear register with all other mbigen nodes in
uniform address space.

                         mbigen chip
    |-----------|--------|--------|---------------|--------|
mgn_node_0  mgn_node_1  ...  mgn_clear_register  ...   mgn_node_i
                            |-----------------|
                             [0xA000, 0xB000)

Everything is OK for now, when the mbigen nodes number less than 10,
there is no conflict with clear register.

Once we defined mbigen node more than 10, it's going to touch clear
register in unexpected way.

There should have a gap of 0x1000 between mgn_node9 and mgn_node10.

The simplest solution is directly skip clear register when access to
more than 10 mbigen nodes.

Fixes: a6c2f87b8820 ("irqchip/mbigen: Implement the mbigen irq chip operation functions")
Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
---
 drivers/irqchip/irq-mbigen.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
index 58881d313979..b600637f5cd7 100644
--- a/drivers/irqchip/irq-mbigen.c
+++ b/drivers/irqchip/irq-mbigen.c
@@ -64,6 +64,20 @@ struct mbigen_device {
 	void __iomem		*base;
 };
 
+static inline unsigned int get_mbigen_node_offset(unsigned int nid)
+{
+	unsigned int offset = nid * MBIGEN_NODE_OFFSET;
+
+	/**
+	 * To avoid touched clear register in unexpected way, we need to directly
+	 * skip clear register when access to more than 10 mbigen nodes.
+	 */
+	if (nid >= (REG_MBIGEN_CLEAR_OFFSET / MBIGEN_NODE_OFFSET))
+		offset += MBIGEN_NODE_OFFSET;
+
+	return offset;
+}
+
 static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
 {
 	unsigned int nid, pin;
@@ -72,7 +86,7 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
 	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
 	pin = hwirq % IRQS_PER_MBIGEN_NODE;
 
-	return pin * 4 + nid * MBIGEN_NODE_OFFSET
+	return pin * 4 + get_mbigen_node_offset(nid)
 			+ REG_MBIGEN_VEC_OFFSET;
 }
 
@@ -88,7 +102,7 @@ static inline void get_mbigen_type_reg(irq_hw_number_t hwirq,
 	*mask = 1 << (irq_ofst % 32);
 	ofst = irq_ofst / 32 * 4;
 
-	*addr = ofst + nid * MBIGEN_NODE_OFFSET
+	*addr = ofst + get_mbigen_node_offset(nid)
 		+ REG_MBIGEN_TYPE_OFFSET;
 }
 
-- 
2.34.1

Re: [PATCH] irqchip/mbigen: Fix mbigen node address layout

[Thomas Gleixner]

On Sat, Jul 20 2024 at 09:35, Yipeng Zou wrote:
> Mbigen chip contains several mbigen nodes, and mapped address space per
> nodes one by one.
>
>                     mbigen chip
>        |-----------------|------------|--------------|
>    mgn_node_0         mgn_node_1     ...         mgn_node_i
> |--------------|   |--------------|       |----------------------|
> [0x0000, 0x1000)   [0x1000, 0x2000)       [i*0x1000, (i+1)*0x1000)
>
> Mbigen also defined a clear register with all other mbigen nodes in
> uniform address space.
>
>                          mbigen chip
>     |-----------|--------|--------|---------------|--------|
> mgn_node_0  mgn_node_1  ...  mgn_clear_register  ...   mgn_node_i
>                             |-----------------|
>                              [0xA000, 0xB000)
>
> Everything is OK for now, when the mbigen nodes number less than 10,
> there is no conflict with clear register.
>
> Once we defined mbigen node more than 10, it's going to touch clear
> register in unexpected way.
>
> There should have a gap of 0x1000 between mgn_node9 and mgn_node10.
>
> The simplest solution is directly skip clear register when access to
> more than 10 mbigen nodes.

I see what you are trying to tell. Something like this makes it more
clear:

   The mbigen interrupt chip has its per node registers located in a
   contiguous region of page sized chunks. The code maps them into
   virtual address space as a contiguous region and determines the
   address of a node by using the node ID as index.

   This works correctly up to 10 nodes, but then fails because the
   11th's array slot is used for the MGN_CLEAR registers.

   Skip the MGN_CLEAR register space when calculating the offset for
   node IDs greater or equal ten.

Hmm?

> Fixes: a6c2f87b8820 ("irqchip/mbigen: Implement the mbigen irq chip operation functions")
> Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
> ---
>  drivers/irqchip/irq-mbigen.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
> index 58881d313979..b600637f5cd7 100644
> --- a/drivers/irqchip/irq-mbigen.c
> +++ b/drivers/irqchip/irq-mbigen.c
> @@ -64,6 +64,20 @@ struct mbigen_device {
>  	void __iomem		*base;
>  };
>  
> +static inline unsigned int get_mbigen_node_offset(unsigned int nid)
> +{
> +	unsigned int offset = nid * MBIGEN_NODE_OFFSET;
> +
> +	/**

This is not a kernel doc comment. Please use '/*'

> +	 * To avoid touched clear register in unexpected way, we need to directly
> +	 * skip clear register when access to more than 10 mbigen nodes.
> +	 */

> @@ -72,7 +86,7 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>  	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>  	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>  
> -	return pin * 4 + nid * MBIGEN_NODE_OFFSET
> +	return pin * 4 + get_mbigen_node_offset(nid)
>  			+ REG_MBIGEN_VEC_OFFSET;

Please get rid of these pointless line breaks.

Thanks,

        tglx

Re: [PATCH] irqchip/mbigen: Fix mbigen node address layout

[Yipeng Zou]

Hi Thomas:

     Reword it in v2.

https://lore.kernel.org/all/20240730014400.1751530-1-zouyipeng@huawei.com/T/#u

在 2024/7/29 18:15, Thomas Gleixner 写道:
> On Sat, Jul 20 2024 at 09:35, Yipeng Zou wrote:
>> Mbigen chip contains several mbigen nodes, and mapped address space per
>> nodes one by one.
>>
>>                      mbigen chip
>>         |-----------------|------------|--------------|
>>     mgn_node_0         mgn_node_1     ...         mgn_node_i
>> |--------------|   |--------------|       |----------------------|
>> [0x0000, 0x1000)   [0x1000, 0x2000)       [i*0x1000, (i+1)*0x1000)
>>
>> Mbigen also defined a clear register with all other mbigen nodes in
>> uniform address space.
>>
>>                           mbigen chip
>>      |-----------|--------|--------|---------------|--------|
>> mgn_node_0  mgn_node_1  ...  mgn_clear_register  ...   mgn_node_i
>>                              |-----------------|
>>                               [0xA000, 0xB000)
>>
>> Everything is OK for now, when the mbigen nodes number less than 10,
>> there is no conflict with clear register.
>>
>> Once we defined mbigen node more than 10, it's going to touch clear
>> register in unexpected way.
>>
>> There should have a gap of 0x1000 between mgn_node9 and mgn_node10.
>>
>> The simplest solution is directly skip clear register when access to
>> more than 10 mbigen nodes.
> I see what you are trying to tell. Something like this makes it more
> clear:
>
>     The mbigen interrupt chip has its per node registers located in a
>     contiguous region of page sized chunks. The code maps them into
>     virtual address space as a contiguous region and determines the
>     address of a node by using the node ID as index.
>
>     This works correctly up to 10 nodes, but then fails because the
>     11th's array slot is used for the MGN_CLEAR registers.
>
>     Skip the MGN_CLEAR register space when calculating the offset for
>     node IDs greater or equal ten.
>
> Hmm?
>
>> Fixes: a6c2f87b8820 ("irqchip/mbigen: Implement the mbigen irq chip operation functions")
>> Signed-off-by: Yipeng Zou <zouyipeng@huawei.com>
>> ---
>>   drivers/irqchip/irq-mbigen.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mbigen.c b/drivers/irqchip/irq-mbigen.c
>> index 58881d313979..b600637f5cd7 100644
>> --- a/drivers/irqchip/irq-mbigen.c
>> +++ b/drivers/irqchip/irq-mbigen.c
>> @@ -64,6 +64,20 @@ struct mbigen_device {
>>   	void __iomem		*base;
>>   };
>>   
>> +static inline unsigned int get_mbigen_node_offset(unsigned int nid)
>> +{
>> +	unsigned int offset = nid * MBIGEN_NODE_OFFSET;
>> +
>> +	/**
> This is not a kernel doc comment. Please use '/*'
>
>> +	 * To avoid touched clear register in unexpected way, we need to directly
>> +	 * skip clear register when access to more than 10 mbigen nodes.
>> +	 */
>> @@ -72,7 +86,7 @@ static inline unsigned int get_mbigen_vec_reg(irq_hw_number_t hwirq)
>>   	nid = hwirq / IRQS_PER_MBIGEN_NODE + 1;
>>   	pin = hwirq % IRQS_PER_MBIGEN_NODE;
>>   
>> -	return pin * 4 + nid * MBIGEN_NODE_OFFSET
>> +	return pin * 4 + get_mbigen_node_offset(nid)
>>   			+ REG_MBIGEN_VEC_OFFSET;
> Please get rid of these pointless line breaks.
>
> Thanks,
>
>          tglx

-- 
Regards,
Yipeng Zou