| Message ID | 20221211065424.806478-1-bmeng@tinylab.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/5] lib: utils/irqchip: plic: Fix the off-by-one error in priority save/restore helpers | expand |
On Dez 11 2022, Bin Meng wrote: > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c > index 73d7788..4df9020 100644 > --- a/lib/utils/irqchip/plic.c > +++ b/lib/utils/irqchip/plic.c > @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) > > void plic_priority_save(const struct plic_data *plic, u8 *priority) > { > - for (u32 i = 0; i < plic->num_src; i++) > + for (u32 i = 1; i <= plic->num_src; i++) > priority[i] = plic_get_priority(plic, i); That needs to adjust the index into the priority array. > void plic_priority_restore(const struct plic_data *plic, const u8 *priority) > { > - for (u32 i = 0; i < plic->num_src; i++) > + for (u32 i = 1; i <= plic->num_src; i++) > plic_set_priority(plic, i, priority[i]); Likewise.
On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote: > > On Dez 11 2022, Bin Meng wrote: > > > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c > > index 73d7788..4df9020 100644 > > --- a/lib/utils/irqchip/plic.c > > +++ b/lib/utils/irqchip/plic.c > > @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) > > > > void plic_priority_save(const struct plic_data *plic, u8 *priority) > > { > > - for (u32 i = 0; i < plic->num_src; i++) > > + for (u32 i = 1; i <= plic->num_src; i++) > > priority[i] = plic_get_priority(plic, i); > > That needs to adjust the index into the priority array. Does that help anything? It just confuses people more. I added function parameter descriptions in patch 2 to make it crystal clear, that the priority array needs to include interrupt source 0. > > > void plic_priority_restore(const struct plic_data *plic, const u8 *priority) > > { > > - for (u32 i = 0; i < plic->num_src; i++) > > + for (u32 i = 1; i <= plic->num_src; i++) > > plic_set_priority(plic, i, priority[i]); > > Likewise. > Regards, Bin
On Dez 11 2022, Bin Meng wrote: > On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote: >> >> On Dez 11 2022, Bin Meng wrote: >> >> > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c >> > index 73d7788..4df9020 100644 >> > --- a/lib/utils/irqchip/plic.c >> > +++ b/lib/utils/irqchip/plic.c >> > @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) >> > >> > void plic_priority_save(const struct plic_data *plic, u8 *priority) >> > { >> > - for (u32 i = 0; i < plic->num_src; i++) >> > + for (u32 i = 1; i <= plic->num_src; i++) >> > priority[i] = plic_get_priority(plic, i); >> >> That needs to adjust the index into the priority array. > > Does that help anything? Yes, it fixes an off-by-one error.
On Dez 11 2022, Bin Meng wrote: > I added function parameter descriptions in patch 2 to make it crystal > clear, that the priority array needs to include interrupt source 0. It should say that the array size must be one more than the maximum number of sources.
On 12/11/22 04:18, Bin Meng wrote: > On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote: >> >> On Dez 11 2022, Bin Meng wrote: >> >>> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c >>> index 73d7788..4df9020 100644 >>> --- a/lib/utils/irqchip/plic.c >>> +++ b/lib/utils/irqchip/plic.c >>> @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) >>> >>> void plic_priority_save(const struct plic_data *plic, u8 *priority) >>> { >>> - for (u32 i = 0; i < plic->num_src; i++) >>> + for (u32 i = 1; i <= plic->num_src; i++) >>> priority[i] = plic_get_priority(plic, i); >> >> That needs to adjust the index into the priority array. > > Does that help anything? It just confuses people more. > > I added function parameter descriptions in patch 2 to make it crystal > clear, that the priority array needs to include interrupt source 0. To me, it is more confusing that when I ask to save the priority for N sources, I need to allocate an array with >N elements. And leaving array element 0 unused wastes memory. Regards, Samuel
On Mon, Dec 12, 2022 at 12:10 PM Samuel Holland <samuel@sholland.org> wrote: > > On 12/11/22 04:18, Bin Meng wrote: > > On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote: > >> > >> On Dez 11 2022, Bin Meng wrote: > >> > >>> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c > >>> index 73d7788..4df9020 100644 > >>> --- a/lib/utils/irqchip/plic.c > >>> +++ b/lib/utils/irqchip/plic.c > >>> @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) > >>> > >>> void plic_priority_save(const struct plic_data *plic, u8 *priority) > >>> { > >>> - for (u32 i = 0; i < plic->num_src; i++) > >>> + for (u32 i = 1; i <= plic->num_src; i++) > >>> priority[i] = plic_get_priority(plic, i); > >> > >> That needs to adjust the index into the priority array. > > > > Does that help anything? It just confuses people more. > > > > I added function parameter descriptions in patch 2 to make it crystal > > clear, that the priority array needs to include interrupt source 0. > > To me, it is more confusing that when I ask to save the priority for N > sources, I need to allocate an array with >N elements. And leaving array > element 0 unused wastes memory. This can be a separate improvement patch. Regards, Anup > > Regards, > Samuel > > > -- > opensbi mailing list > opensbi@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/opensbi
On Sun, Dec 11, 2022 at 12:24 PM Bin Meng <bmeng@tinylab.org> wrote: > > Interrupt source 0 is reserved. Hence the irq should start from 1. > > Fixes: 2b79b694a805 ("lib: irqchip/plic: Add priority save/restore helpers") > Signed-off-by: Bin Meng <bmeng@tinylab.org> > Reviewed-by: Anup Patel <anup@brainfault.org> Applied this patch to the riscv/opensbi repo. Thanks, Anup > --- > > (no changes since v1) > > lib/utils/irqchip/plic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c > index 73d7788..4df9020 100644 > --- a/lib/utils/irqchip/plic.c > +++ b/lib/utils/irqchip/plic.c > @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) > > void plic_priority_save(const struct plic_data *plic, u8 *priority) > { > - for (u32 i = 0; i < plic->num_src; i++) > + for (u32 i = 1; i <= plic->num_src; i++) > priority[i] = plic_get_priority(plic, i); > } > > void plic_priority_restore(const struct plic_data *plic, const u8 *priority) > { > - for (u32 i = 0; i < plic->num_src; i++) > + for (u32 i = 1; i <= plic->num_src; i++) > plic_set_priority(plic, i, priority[i]); > } > > -- > 2.34.1 >
On Dez 17 2022, Anup Patel wrote: > On Mon, Dec 12, 2022 at 12:10 PM Samuel Holland <samuel@sholland.org> wrote: >> >> On 12/11/22 04:18, Bin Meng wrote: >> > On Sun, Dec 11, 2022 at 6:08 PM Andreas Schwab <schwab@linux-m68k.org> wrote: >> >> >> >> On Dez 11 2022, Bin Meng wrote: >> >> >> >>> diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c >> >>> index 73d7788..4df9020 100644 >> >>> --- a/lib/utils/irqchip/plic.c >> >>> +++ b/lib/utils/irqchip/plic.c >> >>> @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) >> >>> >> >>> void plic_priority_save(const struct plic_data *plic, u8 *priority) >> >>> { >> >>> - for (u32 i = 0; i < plic->num_src; i++) >> >>> + for (u32 i = 1; i <= plic->num_src; i++) >> >>> priority[i] = plic_get_priority(plic, i); >> >> >> >> That needs to adjust the index into the priority array. >> > >> > Does that help anything? It just confuses people more. >> > >> > I added function parameter descriptions in patch 2 to make it crystal >> > clear, that the priority array needs to include interrupt source 0. >> >> To me, it is more confusing that when I ask to save the priority for N >> sources, I need to allocate an array with >N elements. And leaving array >> element 0 unused wastes memory. > > This can be a separate improvement patch. It is a bug fix, not just an improvement.
diff --git a/lib/utils/irqchip/plic.c b/lib/utils/irqchip/plic.c index 73d7788..4df9020 100644 --- a/lib/utils/irqchip/plic.c +++ b/lib/utils/irqchip/plic.c @@ -38,13 +38,13 @@ static void plic_set_priority(const struct plic_data *plic, u32 source, u32 val) void plic_priority_save(const struct plic_data *plic, u8 *priority) { - for (u32 i = 0; i < plic->num_src; i++) + for (u32 i = 1; i <= plic->num_src; i++) priority[i] = plic_get_priority(plic, i); } void plic_priority_restore(const struct plic_data *plic, const u8 *priority) { - for (u32 i = 0; i < plic->num_src; i++) + for (u32 i = 1; i <= plic->num_src; i++) plic_set_priority(plic, i, priority[i]); }
Interrupt source 0 is reserved. Hence the irq should start from 1. Fixes: 2b79b694a805 ("lib: irqchip/plic: Add priority save/restore helpers") Signed-off-by: Bin Meng <bmeng@tinylab.org> Reviewed-by: Anup Patel <anup@brainfault.org> --- (no changes since v1) lib/utils/irqchip/plic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)