| Message ID | 20240223102612.1499-1-zhangtianyang@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | irqchip/loongson-pch-pic: Update interrupt registration policy | expand |
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
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
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;