Re: [PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
On 3/9/21 10:42 AM, Greg Kurz wrote:
> On Tue, 9 Mar 2021 10:13:39 +0100
> Greg Kurz wrote:
>
>> On Mon, 8 Mar 2021 19:11:11 +0100
>> Cédric Le Goater wrote:
>>
>>> On 3/8/21 7:07 PM, Greg Kurz wrote:
On Wed, 3 Mar 2021 18:48:53 +0100
Cédric Le Goater wrote:
> Now that the IPI interrupt has its own domain, the checks on the HW
> interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
> check on the domain.
>
> Signed-off-by: Cédric Le Goater
> ---
Shouldn't this have the following tags ?
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Fixes: 930914b7d528 ("powerpc/xive: Add a debugfs file to dump internal
XIVE state")
>>>
>>> The next patch has because it removes the useless check on irq_data.
>>>
>>
>> Ok I get it. This report isn't about an actual crash. Just a false
>> positive because of the not needed check in the caller.
>>
>
> Hrm... I meant because of the check in xive_debug_show_irq(). On the
> contrary, the check removed by this patch in xive_core_debug_show()
> was rather an explicit hint that xive_debug_show_irq() couldn't be
> called with d being NULL.
yes. irq_desc_get_irq_data() does not return a NULL value and
xive_debug_show_irq() is only called from the for_each_irq_desc()
loop.
C.
>
>>> C.
>>>
Anyway,
Reviewed-by: Greg Kurz
> arch/powerpc/sysdev/xive/common.c | 18 --
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/xive/common.c
> b/arch/powerpc/sysdev/xive/common.c
> index 678680531d26..7581cb12bb53 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file
> *m, int cpu)
> seq_puts(m, "\n");
> }
>
> -static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct
> irq_data *d)
> +static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
> {
> - struct irq_chip *chip = irq_data_get_irq_chip(d);
> + unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
> int rc;
> u32 target;
> u8 prio;
> u32 lirq;
>
> - if (!is_xive_irq(chip))
> - return;
> -
> rc = xive_ops->get_irq_config(hw_irq, , , );
> if (rc) {
> seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
> @@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file
> *m, void *private)
>
> for_each_irq_desc(i, desc) {
> struct irq_data *d = irq_desc_get_irq_data(desc);
> - unsigned int hw_irq;
> -
> - if (!d)
> - continue;
> -
> - hw_irq = (unsigned int)irqd_to_hwirq(d);
>
> - /* IPIs are special (HW number 0) */
> - if (hw_irq != XIVE_IPI_HW_IRQ)
> - xive_debug_show_irq(m, hw_irq, d);
> + if (d->domain == xive_irq_domain)
> + xive_debug_show_irq(m, d);
> }
> return 0;
> }
>>>
>>
>
Re: [PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
On Tue, 9 Mar 2021 10:13:39 +0100
Greg Kurz wrote:
> On Mon, 8 Mar 2021 19:11:11 +0100
> Cédric Le Goater wrote:
>
> > On 3/8/21 7:07 PM, Greg Kurz wrote:
> > > On Wed, 3 Mar 2021 18:48:53 +0100
> > > Cédric Le Goater wrote:
> > >
> > >> Now that the IPI interrupt has its own domain, the checks on the HW
> > >> interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
> > >> check on the domain.
> > >>
> > >> Signed-off-by: Cédric Le Goater
> > >> ---
> > >
> > > Shouldn't this have the following tags ?
> > >
> > > Reported-by: kernel test robot
> > > Reported-by: Dan Carpenter
> > > Fixes: 930914b7d528 ("powerpc/xive: Add a debugfs file to dump internal
> > > XIVE state")
> >
> > The next patch has because it removes the useless check on irq_data.
> >
>
> Ok I get it. This report isn't about an actual crash. Just a false
> positive because of the not needed check in the caller.
>
Hrm... I meant because of the check in xive_debug_show_irq(). On the
contrary, the check removed by this patch in xive_core_debug_show()
was rather an explicit hint that xive_debug_show_irq() couldn't be
called with d being NULL.
> > C.
> >
> > >
> > > Anyway,
> > >
> > > Reviewed-by: Greg Kurz
> > >
> > >> arch/powerpc/sysdev/xive/common.c | 18 --
> > >> 1 file changed, 4 insertions(+), 14 deletions(-)
> > >>
> > >> diff --git a/arch/powerpc/sysdev/xive/common.c
> > >> b/arch/powerpc/sysdev/xive/common.c
> > >> index 678680531d26..7581cb12bb53 100644
> > >> --- a/arch/powerpc/sysdev/xive/common.c
> > >> +++ b/arch/powerpc/sysdev/xive/common.c
> > >> @@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file
> > >> *m, int cpu)
> > >> seq_puts(m, "\n");
> > >> }
> > >>
> > >> -static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct
> > >> irq_data *d)
> > >> +static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
> > >> {
> > >> -struct irq_chip *chip = irq_data_get_irq_chip(d);
> > >> +unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
> > >> int rc;
> > >> u32 target;
> > >> u8 prio;
> > >> u32 lirq;
> > >>
> > >> -if (!is_xive_irq(chip))
> > >> -return;
> > >> -
> > >> rc = xive_ops->get_irq_config(hw_irq, , , );
> > >> if (rc) {
> > >> seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq,
> > >> rc);
> > >> @@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file
> > >> *m, void *private)
> > >>
> > >> for_each_irq_desc(i, desc) {
> > >> struct irq_data *d = irq_desc_get_irq_data(desc);
> > >> -unsigned int hw_irq;
> > >> -
> > >> -if (!d)
> > >> -continue;
> > >> -
> > >> -hw_irq = (unsigned int)irqd_to_hwirq(d);
> > >>
> > >> -/* IPIs are special (HW number 0) */
> > >> -if (hw_irq != XIVE_IPI_HW_IRQ)
> > >> -xive_debug_show_irq(m, hw_irq, d);
> > >> +if (d->domain == xive_irq_domain)
> > >> +xive_debug_show_irq(m, d);
> > >> }
> > >> return 0;
> > >> }
> > >
> >
>
Re: [PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
On Mon, 8 Mar 2021 19:11:11 +0100
Cédric Le Goater wrote:
> On 3/8/21 7:07 PM, Greg Kurz wrote:
> > On Wed, 3 Mar 2021 18:48:53 +0100
> > Cédric Le Goater wrote:
> >
> >> Now that the IPI interrupt has its own domain, the checks on the HW
> >> interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
> >> check on the domain.
> >>
> >> Signed-off-by: Cédric Le Goater
> >> ---
> >
> > Shouldn't this have the following tags ?
> >
> > Reported-by: kernel test robot
> > Reported-by: Dan Carpenter
> > Fixes: 930914b7d528 ("powerpc/xive: Add a debugfs file to dump internal
> > XIVE state")
>
> The next patch has because it removes the useless check on irq_data.
>
Ok I get it. This report isn't about an actual crash. Just a false
positive because of the not needed check in the caller.
> C.
>
> >
> > Anyway,
> >
> > Reviewed-by: Greg Kurz
> >
> >> arch/powerpc/sysdev/xive/common.c | 18 --
> >> 1 file changed, 4 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/arch/powerpc/sysdev/xive/common.c
> >> b/arch/powerpc/sysdev/xive/common.c
> >> index 678680531d26..7581cb12bb53 100644
> >> --- a/arch/powerpc/sysdev/xive/common.c
> >> +++ b/arch/powerpc/sysdev/xive/common.c
> >> @@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file
> >> *m, int cpu)
> >>seq_puts(m, "\n");
> >> }
> >>
> >> -static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct
> >> irq_data *d)
> >> +static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
> >> {
> >> - struct irq_chip *chip = irq_data_get_irq_chip(d);
> >> + unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
> >>int rc;
> >>u32 target;
> >>u8 prio;
> >>u32 lirq;
> >>
> >> - if (!is_xive_irq(chip))
> >> - return;
> >> -
> >>rc = xive_ops->get_irq_config(hw_irq, , , );
> >>if (rc) {
> >>seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
> >> @@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file *m,
> >> void *private)
> >>
> >>for_each_irq_desc(i, desc) {
> >>struct irq_data *d = irq_desc_get_irq_data(desc);
> >> - unsigned int hw_irq;
> >> -
> >> - if (!d)
> >> - continue;
> >> -
> >> - hw_irq = (unsigned int)irqd_to_hwirq(d);
> >>
> >> - /* IPIs are special (HW number 0) */
> >> - if (hw_irq != XIVE_IPI_HW_IRQ)
> >> - xive_debug_show_irq(m, hw_irq, d);
> >> + if (d->domain == xive_irq_domain)
> >> + xive_debug_show_irq(m, d);
> >>}
> >>return 0;
> >> }
> >
>
Re: [PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
On 3/8/21 7:07 PM, Greg Kurz wrote:
> On Wed, 3 Mar 2021 18:48:53 +0100
> Cédric Le Goater wrote:
>
>> Now that the IPI interrupt has its own domain, the checks on the HW
>> interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
>> check on the domain.
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>
> Shouldn't this have the following tags ?
>
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
> Fixes: 930914b7d528 ("powerpc/xive: Add a debugfs file to dump internal XIVE
> state")
The next patch has because it removes the useless check on irq_data.
C.
>
> Anyway,
>
> Reviewed-by: Greg Kurz
>
>> arch/powerpc/sysdev/xive/common.c | 18 --
>> 1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/powerpc/sysdev/xive/common.c
>> b/arch/powerpc/sysdev/xive/common.c
>> index 678680531d26..7581cb12bb53 100644
>> --- a/arch/powerpc/sysdev/xive/common.c
>> +++ b/arch/powerpc/sysdev/xive/common.c
>> @@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file *m,
>> int cpu)
>> seq_puts(m, "\n");
>> }
>>
>> -static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct
>> irq_data *d)
>> +static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
>> {
>> -struct irq_chip *chip = irq_data_get_irq_chip(d);
>> +unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
>> int rc;
>> u32 target;
>> u8 prio;
>> u32 lirq;
>>
>> -if (!is_xive_irq(chip))
>> -return;
>> -
>> rc = xive_ops->get_irq_config(hw_irq, , , );
>> if (rc) {
>> seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
>> @@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file *m,
>> void *private)
>>
>> for_each_irq_desc(i, desc) {
>> struct irq_data *d = irq_desc_get_irq_data(desc);
>> -unsigned int hw_irq;
>> -
>> -if (!d)
>> -continue;
>> -
>> -hw_irq = (unsigned int)irqd_to_hwirq(d);
>>
>> -/* IPIs are special (HW number 0) */
>> -if (hw_irq != XIVE_IPI_HW_IRQ)
>> -xive_debug_show_irq(m, hw_irq, d);
>> +if (d->domain == xive_irq_domain)
>> +xive_debug_show_irq(m, d);
>> }
>> return 0;
>> }
>
Re: [PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
On Wed, 3 Mar 2021 18:48:53 +0100
Cédric Le Goater wrote:
> Now that the IPI interrupt has its own domain, the checks on the HW
> interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
> check on the domain.
>
> Signed-off-by: Cédric Le Goater
> ---
Shouldn't this have the following tags ?
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Fixes: 930914b7d528 ("powerpc/xive: Add a debugfs file to dump internal XIVE
state")
Anyway,
Reviewed-by: Greg Kurz
> arch/powerpc/sysdev/xive/common.c | 18 --
> 1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/xive/common.c
> b/arch/powerpc/sysdev/xive/common.c
> index 678680531d26..7581cb12bb53 100644
> --- a/arch/powerpc/sysdev/xive/common.c
> +++ b/arch/powerpc/sysdev/xive/common.c
> @@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file *m,
> int cpu)
> seq_puts(m, "\n");
> }
>
> -static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct
> irq_data *d)
> +static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
> {
> - struct irq_chip *chip = irq_data_get_irq_chip(d);
> + unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
> int rc;
> u32 target;
> u8 prio;
> u32 lirq;
>
> - if (!is_xive_irq(chip))
> - return;
> -
> rc = xive_ops->get_irq_config(hw_irq, , , );
> if (rc) {
> seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
> @@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file *m,
> void *private)
>
> for_each_irq_desc(i, desc) {
> struct irq_data *d = irq_desc_get_irq_data(desc);
> - unsigned int hw_irq;
> -
> - if (!d)
> - continue;
> -
> - hw_irq = (unsigned int)irqd_to_hwirq(d);
>
> - /* IPIs are special (HW number 0) */
> - if (hw_irq != XIVE_IPI_HW_IRQ)
> - xive_debug_show_irq(m, hw_irq, d);
> + if (d->domain == xive_irq_domain)
> + xive_debug_show_irq(m, d);
> }
> return 0;
> }
[PATCH v2 4/8] powerpc/xive: Simplify xive_core_debug_show()
Now that the IPI interrupt has its own domain, the checks on the HW
interrupt number XIVE_IPI_HW_IRQ and on the chip can be replaced by a
check on the domain.
Signed-off-by: Cédric Le Goater
---
arch/powerpc/sysdev/xive/common.c | 18 --
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/sysdev/xive/common.c
b/arch/powerpc/sysdev/xive/common.c
index 678680531d26..7581cb12bb53 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -1579,17 +1579,14 @@ static void xive_debug_show_cpu(struct seq_file *m, int
cpu)
seq_puts(m, "\n");
}
-static void xive_debug_show_irq(struct seq_file *m, u32 hw_irq, struct
irq_data *d)
+static void xive_debug_show_irq(struct seq_file *m, struct irq_data *d)
{
- struct irq_chip *chip = irq_data_get_irq_chip(d);
+ unsigned int hw_irq = (unsigned int)irqd_to_hwirq(d);
int rc;
u32 target;
u8 prio;
u32 lirq;
- if (!is_xive_irq(chip))
- return;
-
rc = xive_ops->get_irq_config(hw_irq, , , );
if (rc) {
seq_printf(m, "IRQ 0x%08x : no config rc=%d\n", hw_irq, rc);
@@ -1627,16 +1624,9 @@ static int xive_core_debug_show(struct seq_file *m, void
*private)
for_each_irq_desc(i, desc) {
struct irq_data *d = irq_desc_get_irq_data(desc);
- unsigned int hw_irq;
-
- if (!d)
- continue;
-
- hw_irq = (unsigned int)irqd_to_hwirq(d);
- /* IPIs are special (HW number 0) */
- if (hw_irq != XIVE_IPI_HW_IRQ)
- xive_debug_show_irq(m, hw_irq, d);
+ if (d->domain == xive_irq_domain)
+ xive_debug_show_irq(m, d);
}
return 0;
}
--
2.26.2
