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@xxxxxxxxxx>
> ---
>  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