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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
Signed-off-by: Zhang Tianyang <zhangtianyang@xxxxxxxxxxx>
Signed-off-by: Biao Dong <dongbiao@xxxxxxxxxxx>
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
