From: Tianyang Zhang <zhangtianyang@loongson.cn>
To: Thomas Gleixner <tglx@linutronix.de>,
chenhuacai@kernel.org, jiaxun.yang@flygoat.com
Cc: linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
Baoqi Zhang <zhangbaoqi@loongson.cn>,
Biao Dong <dongbiao@loongson.cn>
Subject: Re: [PATCH] irqchip/loongson-pch-pic: Update interrupt registration policy
Date: Mon, 26 Feb 2024 17:24:12 +0800 [thread overview]
Message-ID: <a1a42903-eac1-0dfd-d500-64a93ea81da3@loongson.cn> (raw)
In-Reply-To: <875xyccu1n.ffs@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
prev parent reply other threads:[~2024-02-26 9:24 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-23 10:26 [PATCH] irqchip/loongson-pch-pic: Update interrupt registration policy Tianyang Zhang
2024-02-25 17:50 ` Thomas Gleixner
2024-02-26 9:24 ` Tianyang Zhang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a1a42903-eac1-0dfd-d500-64a93ea81da3@loongson.cn \
--to=zhangtianyang@loongson.cn \
--cc=chenhuacai@kernel.org \
--cc=dongbiao@loongson.cn \
--cc=jiaxun.yang@flygoat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=zhangbaoqi@loongson.cn \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).