irqchip/gic-v5: Delete a stray tab
Subsystems (3)
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/3 irqchip/gic-v5: Delete a stray tab
This line is indented one tab too far. Delete the tab.
--- drivers/irqchip/irq-gic-v5-irs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/irqchip/irq-gic-v5-irs.c +1 -1
@@ -568,7 +568,7 @@ static void __init gicv5_irs_init_bases(struct gicv5_irs_chip_data *irs_data,FIELD_PREP (GICV5_IRS_CR1_IST_RA ,GICV5_NO_READ_ALLOC ) |FIELD_PREP (GICV5_IRS_CR1_IC ,GICV5_NON_CACHE ) |FIELD_PREP (GICV5_IRS_CR1_OC ,GICV5_NON_CACHE ); - irs_data->flags |=IRS_FLAGS_NON_COHERENT ; + irs_data->flags |=IRS_FLAGS_NON_COHERENT ; }else { cr1 =FIELD_PREP (GICV5_IRS_CR1_VPED_WA ,GICV5_WRITE_ALLOC ) | FIELD_PREP(GICV5_IRS_CR1_VPED_RA, GICV5_READ_ALLOC) |
Signature · 1 line
2.47.2
PATCH 2/3 irqchip/gic-v5: Fix forever loop in gicv5_its_create_itt_two_level() error handling
In reply to a message not in the archiveThe "i" variable needs to be signed otherwise there is a forever loop in the cleanup code.
--- drivers/irqchip/irq-gic-v5-its.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/irqchip/irq-gic-v5-its.c +2 -2
@@ -191,9 +191,9 @@ static int gicv5_its_create_itt_two_level(struct gicv5_its_chip_data *its, unsigned int num_events) {unsigned int l1_bits, l2_bits, span, events_per_l2_table; -unsigned int i, complete_tables, final_span, num_ents; +unsigned int complete_tables, final_span, num_ents;__le64 *itt_l1, *itt_l2, **l2ptrs; -int ret; +int i, ret;u64 val; ret =gicv5_its_l2sz_to_l2_bits (itt_l2sz);
Signature · 1 line
2.47.2
PATCH 3/3 irqchip/gic-v5: Fix error handling in gicv5_its_irq_domain_alloc()
In reply to a message not in the archiveThere are two issues to fix in this code:
1) If gicv5_alloc_lpi() fails the original code was checking the wrong variable. Fix the mixup between "ret" and "lpi".
2) If irq_domain_alloc_irqs_parent() fails, then clean up all the loop iterations instead of just the current iteration.
--- drivers/irqchip/irq-gic-v5-its.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/irqchip/irq-gic-v5-its.c +11 -3
@@ -949,15 +949,18 @@ static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi device_id = its_dev->device_id;for (i =0 ; i < nr_irqs; i++) { - lpi =gicv5_alloc_lpi (); + ret =gicv5_alloc_lpi ();if (ret <0 ) {pr_debug ("Failed to find free LPI!\n" ); goto out_eventid; } + lpi = ret; ret =irq_domain_alloc_irqs_parent (domain, virq + i,1 , &lpi); -if (ret) +if (ret) { +gicv5_free_lpi (lpi); goto out_free_lpi; + }/* * Store eventid and deviceid into the hwirq for later use.
@@ -979,7 +982,12 @@ static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vireturn 0 ; out_free_lpi: -gicv5_free_lpi (lpi); +while (--i >=0 ) { + irqd =irq_domain_get_irq_data (domain, virq + i); +gicv5_free_lpi (irqd->parent_data->hwirq); +irq_domain_reset_irq_data (irqd); +irq_domain_free_irqs_parent (domain, virq + i,1 ); + } out_eventid:gicv5_its_free_eventid (its_dev, event_id_base, nr_irqs);return ret;
Signature · 1 line
2.47.2
Lorenzo Pieralisi
maintainerRe: [PATCH 3/3] irqchip/gic-v5: Fix error handling in gicv5_its_irq_domain_alloc()
Quoted 17 lines · Dan Carpenter
There are two issues to fix in this code:
1) Ifgicv5_alloc_lpi()fails the original code was checking the wrong variable. Fix the mixup between "ret" and "lpi".
2) Ifirq_domain_alloc_irqs_parent()fails, then clean up all the loop iterations instead of just the current iteration.Fixes 57d72196dfc8 ("irqchip/gic-v5: Add GICv5 ITS support")Signed-off-by Dan Carpenter <[email protected]>--- drivers/irqchip/irq-gic-v5-its.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/irqchip/irq-gic-v5-its.c +0
-0
drivers/irqchip/irq-gic-v5-its.c static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi · line 955
Quoted 8 lines
drivers/irqchip/irq-gic-v5-its.c +1
-1@@ -949,15 +949,18 @@ static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi device_id = its_dev->device_id;for (i =0 ; i < nr_irqs; i++) { - lpi =gicv5_alloc_lpi (); + ret =gicv5_alloc_lpi ();if (ret <0 ) {pr_debug ("Failed to find free LPI!\n" ); goto out_eventid;
This should be:
goto out_free_lpi;
otherwise we miss cleaning up for [0, i - 1] on LPI alloc failure.
I can fix it up - not sure it is worth splitting it into two patches, just let me know please how you want me to handle it.
Thanks,
Lorenzo
Quoted 29 lines
drivers/irqchip/irq-gic-v5-its.c +10
-2} + lpi = ret; ret =irq_domain_alloc_irqs_parent (domain, virq + i,1 , &lpi); -if (ret) +if (ret) { +gicv5_free_lpi (lpi); goto out_free_lpi; + }/* * Store eventid and deviceid into the hwirq for later use. @@ -979,7 +982,12 @@ static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vireturn 0 ; out_free_lpi: -gicv5_free_lpi (lpi); +while (--i >=0 ) { + irqd =irq_domain_get_irq_data (domain, virq + i); +gicv5_free_lpi (irqd->parent_data->hwirq); +irq_domain_reset_irq_data (irqd); +irq_domain_free_irqs_parent (domain, virq + i,1 ); + } out_eventid:gicv5_its_free_eventid (its_dev, event_id_base, nr_irqs);return ret;Signature · 1 line
2.47.2
Re: [PATCH 3/3] irqchip/gic-v5: Fix error handling in gicv5_its_irq_domain_alloc()
Quoted 29 lines · Lorenzo Pieralisi
Quoted 17 lines · Dan Carpenter
There are two issues to fix in this code:
1) Ifgicv5_alloc_lpi()fails the original code was checking the wrong variable. Fix the mixup between "ret" and "lpi".
2) Ifirq_domain_alloc_irqs_parent()fails, then clean up all the loop iterations instead of just the current iteration.Fixes 57d72196dfc8 ("irqchip/gic-v5: Add GICv5 ITS support")Signed-off-by Dan Carpenter <[email protected]>--- drivers/irqchip/irq-gic-v5-its.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/irqchip/irq-gic-v5-its.c +0
-0drivers/irqchip/irq-gic-v5-its.c static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi · line 955
Quoted 8 lines
drivers/irqchip/irq-gic-v5-its.c +1
-1@@ -949,15 +949,18 @@ static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi device_id = its_dev->device_id;for (i =0 ; i < nr_irqs; i++) { - lpi =gicv5_alloc_lpi (); + ret =gicv5_alloc_lpi ();if (ret <0 ) {pr_debug ("Failed to find free LPI!\n" ); goto out_eventid;This should be:
Quoted 2 lines
goto
out_free_lpi;
Yes, you're right.
Quoted 4 lines
otherwise we miss cleaning up for [0, i - 1] on LPI alloc failure.
I can fix it up - not sure it is worth splitting it into two patches, just let me know please how you want me to handle it.
I don't think it should be split up. As a reviewer I would be annoyed by a split up version of this.
I'm a little bit surprised by the offer to fix it up for me... Is this going through your tree? It's probably easiest if I just send a v2... Let me do that.
regards,
dan carpenter
Lorenzo Pieralisi
maintainerRe: [PATCH 3/3] irqchip/gic-v5: Fix error handling in gicv5_its_irq_domain_alloc()
Quoted 30 lines · Dan Carpenter
Quoted 29 lines · Lorenzo Pieralisi
Quoted 17 lines · Dan Carpenter
There are two issues to fix in this code:
1) Ifgicv5_alloc_lpi()fails the original code was checking the wrong variable. Fix the mixup between "ret" and "lpi".
2) Ifirq_domain_alloc_irqs_parent()fails, then clean up all the loop iterations instead of just the current iteration.Fixes 57d72196dfc8 ("irqchip/gic-v5: Add GICv5 ITS support")Signed-off-by Dan Carpenter <[email protected]>--- drivers/irqchip/irq-gic-v5-its.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/irqchip/irq-gic-v5-its.c +0
-0drivers/irqchip/irq-gic-v5-its.c static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi · line 955
Quoted 8 lines
drivers/irqchip/irq-gic-v5-its.c +1
-1@@ -949,15 +949,18 @@ static int gicv5_its_irq_domain_alloc(struct irq_domain *domain, unsigned int vi device_id = its_dev->device_id;for (i =0 ; i < nr_irqs; i++) { - lpi =gicv5_alloc_lpi (); + ret =gicv5_alloc_lpi ();if (ret <0 ) {pr_debug ("Failed to find free LPI!\n" ); goto out_eventid;This should be:
Quoted 4 lines
Quoted 2 lines
goto
out_free_lpi;Yes, you're right.
While at it I'd rename the label, out_free_irqs or something like that, now we are doing more than freeing an LPI.
Quoted 11 lines
Quoted 4 lines
otherwise we miss cleaning up for [0, i - 1] on LPI alloc failure.
I can fix it up - not sure it is worth splitting it into two patches, just let me know please how you want me to handle it.
I don't think it should be split up. As a reviewer I would be annoyed by a split up version of this.
I'm a little bit surprised by the offer to fix it up for me... Is this going through your tree? It's probably easiest if I just send a v2... Let me do that.
I just wanted to help. FYI Marc has sent a PR today mentioning these patches:
https://lore.kernel.org/lkml/[email protected]/
I think that keeping the CC list is good so that Thomas can pick them up if he pulls the branch.
Thanks again for fixing these issues.
Lorenzo