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

--
Regards,
Yipeng Zou