[PATCH] irqchip/loongson-pch-pic: Update interrupt registration policy

[PATCH] irqchip/loongson-pch-pic: Update interrupt registration policy

[Tianyang Zhang]

From: Baoqi Zhang <zhangbaoqi@loongson.cn>

We have removed the fixed mapping between the 7A interrupt source
and the HT interrupt vector, and replaced it with a dynamically
allocated approach. This will be more conducive to fully utilizing
existing vectors to support more devices

Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
Signed-off-by: Zhang Tianyang <zhangtianyang@loongson.cn>
Signed-off-by: Biao Dong <dongbiao@loongson.cn>
---
 drivers/irqchip/irq-loongson-pch-pic.c | 64 +++++++++++++++++++-------
 1 file changed, 47 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
index 63db8e2172e0..86549356e76e 100644
--- a/drivers/irqchip/irq-loongson-pch-pic.c
+++ b/drivers/irqchip/irq-loongson-pch-pic.c
@@ -34,6 +34,8 @@
 #define PIC_REG_IDX(irq_id)	((irq_id) / PIC_COUNT_PER_REG)
 #define PIC_REG_BIT(irq_id)	((irq_id) % PIC_COUNT_PER_REG)
 
+#define hwirq_to_bit(priv, hirq)	(((priv)->table)[(hirq)])
+
 static int nr_pics;
 
 struct pch_pic {
@@ -46,6 +48,8 @@ struct pch_pic {
 	u32			saved_vec_en[PIC_REG_COUNT];
 	u32			saved_vec_pol[PIC_REG_COUNT];
 	u32			saved_vec_edge[PIC_REG_COUNT];
+	u8			table[PIC_COUNT];
+	int			inuse;
 };
 
 static struct pch_pic *pch_pic_priv[MAX_IO_PICS];
@@ -80,45 +84,47 @@ static void pch_pic_mask_irq(struct irq_data *d)
 {
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
 
-	pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq);
+	pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq));
 	irq_chip_mask_parent(d);
 }
 
 static void pch_pic_unmask_irq(struct irq_data *d)
 {
+	int bit = hwirq_to_bit(priv, d->hwirq);
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
 
-	writel(BIT(PIC_REG_BIT(d->hwirq)),
-			priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
+	writel(BIT(PIC_REG_BIT(bit)),
+			priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
 
 	irq_chip_unmask_parent(d);
-	pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq);
+	pch_pic_bitclr(priv, PCH_PIC_MASK, bit);
 }
 
 static int pch_pic_set_type(struct irq_data *d, unsigned int type)
 {
+	int bit = hwirq_to_bit(priv, d->hwirq);
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
 	int ret = 0;
 
 	switch (type) {
 	case IRQ_TYPE_EDGE_RISING:
-		pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitset(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitclr(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_edge_irq);
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		pch_pic_bitset(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitset(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitset(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_edge_irq);
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
-		pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitclr(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitclr(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitclr(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_level_irq);
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		pch_pic_bitclr(priv, PCH_PIC_EDGE, d->hwirq);
-		pch_pic_bitset(priv, PCH_PIC_POL, d->hwirq);
+		pch_pic_bitclr(priv, PCH_PIC_EDGE, bit);
+		pch_pic_bitset(priv, PCH_PIC_POL, bit);
 		irq_set_handler_locked(d, handle_level_irq);
 		break;
 	default:
@@ -132,12 +138,13 @@ static int pch_pic_set_type(struct irq_data *d, unsigned int type)
 static void pch_pic_ack_irq(struct irq_data *d)
 {
 	unsigned int reg;
+	int bit = hwirq_to_bit(priv, d->hwirq);
 	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
 
-	reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(d->hwirq) * 4);
-	if (reg & BIT(PIC_REG_BIT(d->hwirq))) {
-		writel(BIT(PIC_REG_BIT(d->hwirq)),
-			priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
+	reg = readl(priv->base + PCH_PIC_EDGE + PIC_REG_IDX(bit) * 4);
+	if (reg & BIT(PIC_REG_BIT(bit))) {
+		writel(BIT(PIC_REG_BIT(bit)),
+			priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
 	}
 	irq_chip_ack_parent(d);
 }
@@ -157,6 +164,7 @@ static int pch_pic_domain_translate(struct irq_domain *d,
 					unsigned long *hwirq,
 					unsigned int *type)
 {
+	int i;
 	struct pch_pic *priv = d->host_data;
 	struct device_node *of_node = to_of_node(fwspec->fwnode);
 
@@ -171,6 +179,20 @@ static int pch_pic_domain_translate(struct irq_domain *d,
 			return -EINVAL;
 
 		*hwirq = fwspec->param[0] - priv->gsi_base;
+
+		raw_spin_lock(&priv->pic_lock);
+		for (i = 0; i < priv->inuse; i++) {
+			if (priv->table[i] == *hwirq) {
+				*hwirq = i;
+				break;
+			}
+		}
+		if (i == priv->inuse && priv->inuse < PIC_COUNT) {
+			priv->table[priv->inuse] = *hwirq;
+			*hwirq = priv->inuse++;
+		}
+		raw_spin_unlock(&priv->pic_lock);
+
 		if (fwspec->param_count > 1)
 			*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
 		else
@@ -194,6 +216,9 @@ static int pch_pic_alloc(struct irq_domain *domain, unsigned int virq,
 	if (err)
 		return err;
 
+	/* Write vector ID */
+	writeb(priv->ht_vec_base + hwirq, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, hwirq)));
+
 	parent_fwspec.fwnode = domain->parent->fwnode;
 	parent_fwspec.param_count = 1;
 	parent_fwspec.param[0] = hwirq + priv->ht_vec_base;
@@ -222,7 +247,7 @@ static void pch_pic_reset(struct pch_pic *priv)
 
 	for (i = 0; i < PIC_COUNT; i++) {
 		/* Write vector ID */
-		writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(i));
+		writeb(priv->ht_vec_base + i, priv->base + PCH_INT_HTVEC(hwirq_to_bit(priv, i)));
 		/* Hardcode route to HT0 Lo */
 		writeb(1, priv->base + PCH_INT_ROUTE(i));
 	}
@@ -283,6 +308,7 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
 			struct irq_domain *parent_domain, struct fwnode_handle *domain_handle,
 			u32 gsi_base)
 {
+	int i;
 	struct pch_pic *priv;
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
@@ -294,6 +320,10 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
 	if (!priv->base)
 		goto free_priv;
 
+	priv->inuse = 0;
+	for (i = 0; i < PIC_COUNT; i++)
+		priv->table[i] = -1;
+
 	priv->ht_vec_base = vec_base;
 	priv->vec_count = ((readq(priv->base) >> 48) & 0xff) + 1;
 	priv->gsi_base = gsi_base;
-- 
2.20.1

Re: [PATCH] irqchip/loongson-pch-pic: Update interrupt registration policy

[Thomas Gleixner]

On Fri, Feb 23 2024 at 18:26, Tianyang Zhang wrote:
> From: Baoqi Zhang <zhangbaoqi@loongson.cn>
>
> We have removed the fixed mapping between the 7A interrupt source
> and the HT interrupt vector, and replaced it with a dynamically
> allocated approach. This will be more conducive to fully utilizing
> existing vectors to support more devices

You are describing _WHAT_ the patch is doing, but you fail to explain
the context and the _WHY_.

> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
> Signed-off-by: Zhang Tianyang <zhangtianyang@loongson.cn>
> Signed-off-by: Biao Dong <dongbiao@loongson.cn>

This Signed-off-by chain is wrong. You, Tianyang, are sending this,
right?

See

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

and the following chapters.

> ---
>  drivers/irqchip/irq-loongson-pch-pic.c | 64 +++++++++++++++++++-------
>  1 file changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
> index 63db8e2172e0..86549356e76e 100644
> --- a/drivers/irqchip/irq-loongson-pch-pic.c
> +++ b/drivers/irqchip/irq-loongson-pch-pic.c

> @@ -34,6 +34,8 @@
>  #define PIC_REG_IDX(irq_id)	((irq_id) / PIC_COUNT_PER_REG)
>  #define PIC_REG_BIT(irq_id)	((irq_id) % PIC_COUNT_PER_REG)
>  
> +#define hwirq_to_bit(priv, hirq)	(((priv)->table)[(hirq)])

Make this a static inline please.

>  static int nr_pics;
>  
>  struct pch_pic {
> @@ -46,6 +48,8 @@ struct pch_pic {
>  	u32			saved_vec_en[PIC_REG_COUNT];
>  	u32			saved_vec_pol[PIC_REG_COUNT];
>  	u32			saved_vec_edge[PIC_REG_COUNT];
> +	u8			table[PIC_COUNT];
> +	int			inuse;
>  };
>  
>  static struct pch_pic *pch_pic_priv[MAX_IO_PICS];
> @@ -80,45 +84,47 @@ static void pch_pic_mask_irq(struct irq_data *d)
>  {
>  	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
>  
> -	pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq);
> +	pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq));
>  	irq_chip_mask_parent(d);
>  }
>  
>  static void pch_pic_unmask_irq(struct irq_data *d)
>  {
> +	int bit = hwirq_to_bit(priv, d->hwirq);
>  	struct pch_pic *priv = irq_data_get_irq_chip_data(d);

How does this even compile?

>  
> -	writel(BIT(PIC_REG_BIT(d->hwirq)),
> -			priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
> +	writel(BIT(PIC_REG_BIT(bit)),
> +			priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
>  
>  	irq_chip_unmask_parent(d);
> -	pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq);
> +	pch_pic_bitclr(priv, PCH_PIC_MASK, bit);
>  }
>  
>  static int pch_pic_set_type(struct irq_data *d, unsigned int type)
>  {
> +	int bit = hwirq_to_bit(priv, d->hwirq);
>  	struct pch_pic *priv = irq_data_get_irq_chip_data(d);

And this?

By chance because you used a macro instead of an inline function. But
it's still incorrect and wrong.

> @@ -157,6 +164,7 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>  					unsigned long *hwirq,
>  					unsigned int *type)
>  {
> +	int i;
>  	struct pch_pic *priv = d->host_data;
>  	struct device_node *of_node = to_of_node(fwspec->fwnode);

Please see:

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
  
> @@ -171,6 +179,20 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>  			return -EINVAL;
>  
>  		*hwirq = fwspec->param[0] - priv->gsi_base;
> +
> +		raw_spin_lock(&priv->pic_lock);

This was clearly never tested with lockdep enabled. Why?

Because lockdep would have told you that this takes the spinlock with
interrupts enabled while it is taken in the mask()/unmask() callbacks
from hard interrupt context.

> +		for (i = 0; i < priv->inuse; i++) {
> +			if (priv->table[i] == *hwirq) {
> +				*hwirq = i;
> +				break;
> +			}
> +		}
> +		if (i == priv->inuse && priv->inuse < PIC_COUNT) {
> +			priv->table[priv->inuse] = *hwirq;
> +			*hwirq = priv->inuse++;
> +		}

So in case that priv->inuse == PIC_COUNT this does not set hwirq and
returns with bogus values.

> +		raw_spin_unlock(&priv->pic_lock);
> +
> @@ -294,6 +320,10 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
>  	if (!priv->base)
>  		goto free_priv;
>  
> +	priv->inuse = 0;
> +	for (i = 0; i < PIC_COUNT; i++)
> +		priv->table[i] = -1;

table is an array of u8. So how does -1 make sense? Even if it would
make sense, then you can't ever have 256 interrupts in use because the
truncated -1 is equivalent to hwirq 255.

Thanks,

        tglx

Re: [PATCH] irqchip/loongson-pch-pic: Update interrupt registration policy

[Tianyang Zhang]

Hi Thomas,

Thank you very much for your reply. my response below, please review

在 2024/2/26 上午1:50, Thomas Gleixner 写道:
> On Fri, Feb 23 2024 at 18:26, Tianyang Zhang wrote:
>> From: Baoqi Zhang <zhangbaoqi@loongson.cn>
>>
>> We have removed the fixed mapping between the 7A interrupt source
>> and the HT interrupt vector, and replaced it with a dynamically
>> allocated approach. This will be more conducive to fully utilizing
>> existing vectors to support more devices
> You are describing _WHAT_ the patch is doing, but you fail to explain
> the context and the _WHY_.
I will rewrite the commit as required
>> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
>> Signed-off-by: Zhang Tianyang <zhangtianyang@loongson.cn>
>> Signed-off-by: Biao Dong <dongbiao@loongson.cn>
> This Signed-off-by chain is wrong. You, Tianyang, are sending this,
> right?
>
> See
>
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
>
> and the following chapters.
sorry, I will reorganize the Signed-off-by chain
>> ---
>>   drivers/irqchip/irq-loongson-pch-pic.c | 64 +++++++++++++++++++-------
>>   1 file changed, 47 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-loongson-pch-pic.c b/drivers/irqchip/irq-loongson-pch-pic.c
>> index 63db8e2172e0..86549356e76e 100644
>> --- a/drivers/irqchip/irq-loongson-pch-pic.c
>> +++ b/drivers/irqchip/irq-loongson-pch-pic.c
>> @@ -34,6 +34,8 @@
>>   #define PIC_REG_IDX(irq_id)	((irq_id) / PIC_COUNT_PER_REG)
>>   #define PIC_REG_BIT(irq_id)	((irq_id) % PIC_COUNT_PER_REG)
>>   
>> +#define hwirq_to_bit(priv, hirq)	(((priv)->table)[(hirq)])
> Make this a static inline please.
Okay, I will follow the suggestions and make the necessary modifications
>>   static int nr_pics;
>>   
>>   struct pch_pic {
>> @@ -46,6 +48,8 @@ struct pch_pic {
>>   	u32			saved_vec_en[PIC_REG_COUNT];
>>   	u32			saved_vec_pol[PIC_REG_COUNT];
>>   	u32			saved_vec_edge[PIC_REG_COUNT];
>> +	u8			table[PIC_COUNT];
>> +	int			inuse;
>>   };
>>   
>>   static struct pch_pic *pch_pic_priv[MAX_IO_PICS];
>> @@ -80,45 +84,47 @@ static void pch_pic_mask_irq(struct irq_data *d)
>>   {
>>   	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
>>   
>> -	pch_pic_bitset(priv, PCH_PIC_MASK, d->hwirq);
>> +	pch_pic_bitset(priv, PCH_PIC_MASK, hwirq_to_bit(priv, d->hwirq));
>>   	irq_chip_mask_parent(d);
>>   }
>>   
>>   static void pch_pic_unmask_irq(struct irq_data *d)
>>   {
>> +	int bit = hwirq_to_bit(priv, d->hwirq);
>>   	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
> How does this even compile?
This is an error that occurred during the patch delivery process. I am 
very sorry and will immediately correct and improve the submission process
>>   
>> -	writel(BIT(PIC_REG_BIT(d->hwirq)),
>> -			priv->base + PCH_PIC_CLR + PIC_REG_IDX(d->hwirq) * 4);
>> +	writel(BIT(PIC_REG_BIT(bit)),
>> +			priv->base + PCH_PIC_CLR + PIC_REG_IDX(bit) * 4);
>>   
>>   	irq_chip_unmask_parent(d);
>> -	pch_pic_bitclr(priv, PCH_PIC_MASK, d->hwirq);
>> +	pch_pic_bitclr(priv, PCH_PIC_MASK, bit);
>>   }
>>   
>>   static int pch_pic_set_type(struct irq_data *d, unsigned int type)
>>   {
>> +	int bit = hwirq_to_bit(priv, d->hwirq);
>>   	struct pch_pic *priv = irq_data_get_irq_chip_data(d);
> And this?
>
> By chance because you used a macro instead of an inline function. But
> it's still incorrect and wrong.
Just like above, I apologize again
>> @@ -157,6 +164,7 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>>   					unsigned long *hwirq,
>>   					unsigned int *type)
>>   {
>> +	int i;
>>   	struct pch_pic *priv = d->host_data;
>>   	struct device_node *of_node = to_of_node(fwspec->fwnode);
> Please see:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
Thanks for reminder, this will be fixed to follow the declaration method 
of the reverse fir tree
>    
>> @@ -171,6 +179,20 @@ static int pch_pic_domain_translate(struct irq_domain *d,
>>   			return -EINVAL;
>>   
>>   		*hwirq = fwspec->param[0] - priv->gsi_base;
>> +
>> +		raw_spin_lock(&priv->pic_lock);
> This was clearly never tested with lockdep enabled. Why?
>
> Because lockdep would have told you that this takes the spinlock with
> interrupts enabled while it is taken in the mask()/unmask() callbacks
> from hard interrupt context.

Thank you for your correction. After using lockdep testing and learning 
the details of spinlock,

I will replace the original function with 
raw_spin_lock_irqsave/raw_spin_lock_irqrestore

>> +		for (i = 0; i < priv->inuse; i++) {
>> +			if (priv->table[i] == *hwirq) {
>> +				*hwirq = i;
>> +				break;
>> +			}
>> +		}
>> +		if (i == priv->inuse && priv->inuse < PIC_COUNT) {
>> +			priv->table[priv->inuse] = *hwirq;
>> +			*hwirq = priv->inuse++;
>> +		}
> So in case that priv->inuse == PIC_COUNT this does not set hwirq and
> returns with bogus values.

We did miss the code for exception handling here, partly because the 
number of interrupt

sources on our device is far less than PIC_COUNT. However, this will 
still cause problems,

and we will add a code to prompt error handling

>> +		raw_spin_unlock(&priv->pic_lock);
>> +
>> @@ -294,6 +320,10 @@ static int pch_pic_init(phys_addr_t addr, unsigned long size, int vec_base,
>>   	if (!priv->base)
>>   		goto free_priv;
>>   
>> +	priv->inuse = 0;
>> +	for (i = 0; i < PIC_COUNT; i++)
>> +		priv->table[i] = -1;
> table is an array of u8. So how does -1 make sense? Even if it would
> make sense, then you can't ever have 256 interrupts in use because the
> truncated -1 is equivalent to hwirq 255.

The original intention of using -1 here was to mark the initialization 
status of a table entry,

For indicating an invalid state, we think -1 is a more prominent 
notation compared to 255

but we overlooked its original type . In next patch, we will replace 
this immediate with a macro

Thanks again

                 Tianyang