On Fri, Feb 23 2024 at 18:26, Tianyang Zhang wrote:I will rewrite the commit as required
From: Baoqi Zhang <zhangbaoqi@xxxxxxxxxxx>You are describing _WHAT_ the patch is doing, but you fail to explain
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
the context and the _WHY_.
sorry, I will reorganize the Signed-off-by chainSigned-off-by: Baoqi Zhang <zhangbaoqi@xxxxxxxxxxx>This Signed-off-by chain is wrong. You, Tianyang, are sending this,
Signed-off-by: Zhang Tianyang <zhangtianyang@xxxxxxxxxxx>
Signed-off-by: Biao Dong <dongbiao@xxxxxxxxxxx>
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.
Okay, I will follow the suggestions and make the necessary modifications---Make this a static inline please.
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)])
This is an error that occurred during the patch delivery process. I am very sorry and will immediately correct and improve the submission processstatic int nr_pics;How does this even compile?
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);
Just like above, I apologize again- writel(BIT(PIC_REG_BIT(d->hwirq)),And this?
- 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);
By chance because you used a macro instead of an inline function. But
it's still incorrect and wrong.
Thanks for reminder, this will be fixed to follow the declaration method of the reverse fir tree@@ -157,6 +164,7 @@ static int pch_pic_domain_translate(struct irq_domain *d,Please see:
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);
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,This was clearly never tested with lockdep enabled. Why?
return -EINVAL;
*hwirq = fwspec->param[0] - priv->gsi_base;
+
+ raw_spin_lock(&priv->pic_lock);
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++) {So in case that priv->inuse == PIC_COUNT this does not set hwirq and
+ if (priv->table[i] == *hwirq) {
+ *hwirq = i;
+ break;
+ }
+ }
+ if (i == priv->inuse && priv->inuse < PIC_COUNT) {
+ priv->table[priv->inuse] = *hwirq;
+ *hwirq = priv->inuse++;
+ }
returns with bogus values.
+ raw_spin_unlock(&priv->pic_lock);table is an array of u8. So how does -1 make sense? Even if it would
+
@@ -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;
make sense, then you can't ever have 256 interrupts in use because the
truncated -1 is equivalent to hwirq 255.