| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 21 Jun 2024 12:17:54 +0100
From: Mark Rutland <mark.rutland@....com>
To: Jinjie Ruan <ruanjinjie@...wei.com>
Cc: linux-arm-kernel@...ts.infradead.org, alexandru.elisei@....com,
catalin.marinas@....com, linux-kernel@...r.kernel.org,
maz@...nel.org, tglx@...utronix.de, will@...nel.org
Subject: Re: [PATCH v2 2/5] irqchip/gic-common: Remove sync_access callback
On Fri, Jun 21, 2024 at 10:22:26AM +0800, Jinjie Ruan wrote:
>
>
> On 2024/6/17 19:18, Mark Rutland wrote:
> > The gic_configure_irq(), gic_dist_config(), and gic_cpu_config()
> > functions each take an optional "sync_access" callback, but in almost
> > all cases this is not used. The only user is the GICv3 driver's
> > gic_cpu_init() function, which uses gic_redist_wait_for_rwp() as the
> > "sync_access" callback for gic_cpu_config().
> >
> > It would be simpler and clearer to remove the callback and have the
> > GICv3 driver call gic_redist_wait_for_rwp() explicitly after
> > gic_cpu_config().
> >
> > Remove the "sync_access" callback, and call gic_redist_wait_for_rwp()
> > explicitly in the GICv3 driver.
> >
> > There should be no functional change as a result of this patch.
>
> There seems to be a similar patch already:
>
> https://lore.kernel.org/all/20230902134106.1969-1-yuzenghui@huawei.com/
Ok; looking at the diffstat and the diff, that patch didn't adjust
gic_cpu_config(), and Marc has already reviewed and tested this.
Thanks for the pointer, but I don't think that changes anything?
Mark.
>
> >
> > Signed-off-by: Mark Rutland <mark.rutland@....com>
> > Cc: Alexandru Elisei <alexandru.elisei@....com>
> > Cc: Catalin Marinas <catalin.marinas@....com>
> > Cc: Marc Zyngier <maz@...nel.org>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Will Deacon <will@...nel.org>
> > Reviewed-by: Marc Zyngier <maz@...nel.org>
> > Tested-by: Marc Zyngier <maz@...nel.org>
> > ---
> > drivers/irqchip/irq-gic-common.c | 16 +++-------------
> > drivers/irqchip/irq-gic-common.h | 7 +++----
> > drivers/irqchip/irq-gic-v3.c | 7 ++++---
> > drivers/irqchip/irq-gic.c | 6 +++---
> > drivers/irqchip/irq-hip04.c | 6 +++---
> > 5 files changed, 16 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
> > index afd6a1841715a..4ed17620dc4d7 100644
> > --- a/drivers/irqchip/irq-gic-common.c
> > +++ b/drivers/irqchip/irq-gic-common.c
> > @@ -45,7 +45,7 @@ void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
> > }
> >
> > int gic_configure_irq(unsigned int irq, unsigned int type,
> > - void __iomem *base, void (*sync_access)(void))
> > + void __iomem *base)
> > {
> > u32 confmask = 0x2 << ((irq % 16) * 2);
> > u32 confoff = (irq / 16) * 4;
> > @@ -84,14 +84,10 @@ int gic_configure_irq(unsigned int irq, unsigned int type,
> >
> > raw_spin_unlock_irqrestore(&irq_controller_lock, flags);
> >
> > - if (sync_access)
> > - sync_access();
> > -
> > return ret;
> > }
> >
> > -void gic_dist_config(void __iomem *base, int gic_irqs,
> > - void (*sync_access)(void))
> > +void gic_dist_config(void __iomem *base, int gic_irqs)
> > {
> > unsigned int i;
> >
> > @@ -118,12 +114,9 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
> > writel_relaxed(GICD_INT_EN_CLR_X32,
> > base + GIC_DIST_ENABLE_CLEAR + i / 8);
> > }
> > -
> > - if (sync_access)
> > - sync_access();
> > }
> >
> > -void gic_cpu_config(void __iomem *base, int nr, void (*sync_access)(void))
> > +void gic_cpu_config(void __iomem *base, int nr)
> > {
> > int i;
> >
> > @@ -144,7 +137,4 @@ void gic_cpu_config(void __iomem *base, int nr, void (*sync_access)(void))
> > for (i = 0; i < nr; i += 4)
> > writel_relaxed(GICD_INT_DEF_PRI_X4,
> > base + GIC_DIST_PRI + i * 4 / 4);
> > -
> > - if (sync_access)
> > - sync_access();
> > }
> > diff --git a/drivers/irqchip/irq-gic-common.h b/drivers/irqchip/irq-gic-common.h
> > index f407cce9ecaaa..c230175dd584c 100644
> > --- a/drivers/irqchip/irq-gic-common.h
> > +++ b/drivers/irqchip/irq-gic-common.h
> > @@ -20,10 +20,9 @@ struct gic_quirk {
> > };
> >
> > int gic_configure_irq(unsigned int irq, unsigned int type,
> > - void __iomem *base, void (*sync_access)(void));
> > -void gic_dist_config(void __iomem *base, int gic_irqs,
> > - void (*sync_access)(void));
> > -void gic_cpu_config(void __iomem *base, int nr, void (*sync_access)(void));
> > + void __iomem *base);
> > +void gic_dist_config(void __iomem *base, int gic_irqs);
> > +void gic_cpu_config(void __iomem *base, int nr);
> > void gic_enable_quirks(u32 iidr, const struct gic_quirk *quirks,
> > void *data);
> > void gic_enable_of_quirks(const struct device_node *np,
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index 6fb276504bcc8..d95dda2383fb5 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -670,7 +670,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> >
> > offset = convert_offset_index(d, GICD_ICFGR, &index);
> >
> > - ret = gic_configure_irq(index, type, base + offset, NULL);
> > + ret = gic_configure_irq(index, type, base + offset);
> > if (ret && (range == PPI_RANGE || range == EPPI_RANGE)) {
> > /* Misconfigured PPIs are usually not fatal */
> > pr_warn("GIC: PPI INTID%ld is secure or misconfigured\n", irq);
> > @@ -940,7 +940,7 @@ static void __init gic_dist_init(void)
> > writel_relaxed(GICD_INT_DEF_PRI_X4, base + GICD_IPRIORITYRnE + i);
> >
> > /* Now do the common stuff */
> > - gic_dist_config(base, GIC_LINE_NR, NULL);
> > + gic_dist_config(base, GIC_LINE_NR);
> >
> > val = GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1;
> > if (gic_data.rdists.gicd_typer2 & GICD_TYPER2_nASSGIcap) {
> > @@ -1282,7 +1282,8 @@ static void gic_cpu_init(void)
> > for (i = 0; i < gic_data.ppi_nr + SGI_NR; i += 32)
> > writel_relaxed(~0, rbase + GICR_IGROUPR0 + i / 8);
> >
> > - gic_cpu_config(rbase, gic_data.ppi_nr + SGI_NR, gic_redist_wait_for_rwp);
> > + gic_cpu_config(rbase, gic_data.ppi_nr + SGI_NR);
> > + gic_redist_wait_for_rwp();
> >
> > /* initialise system registers */
> > gic_cpu_sys_reg_init();
> > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > index 98aa383e39db1..87255bde960fc 100644
> > --- a/drivers/irqchip/irq-gic.c
> > +++ b/drivers/irqchip/irq-gic.c
> > @@ -303,7 +303,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
> > type != IRQ_TYPE_EDGE_RISING)
> > return -EINVAL;
> >
> > - ret = gic_configure_irq(gicirq, type, base + GIC_DIST_CONFIG, NULL);
> > + ret = gic_configure_irq(gicirq, type, base + GIC_DIST_CONFIG);
> > if (ret && gicirq < 32) {
> > /* Misconfigured PPIs are usually not fatal */
> > pr_warn("GIC: PPI%ld is secure or misconfigured\n", gicirq - 16);
> > @@ -479,7 +479,7 @@ static void gic_dist_init(struct gic_chip_data *gic)
> > for (i = 32; i < gic_irqs; i += 4)
> > writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
> >
> > - gic_dist_config(base, gic_irqs, NULL);
> > + gic_dist_config(base, gic_irqs);
> >
> > writel_relaxed(GICD_ENABLE, base + GIC_DIST_CTRL);
> > }
> > @@ -516,7 +516,7 @@ static int gic_cpu_init(struct gic_chip_data *gic)
> > gic_cpu_map[i] &= ~cpu_mask;
> > }
> >
> > - gic_cpu_config(dist_base, 32, NULL);
> > + gic_cpu_config(dist_base, 32);
> >
> > writel_relaxed(GICC_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
> > gic_cpu_if_up(gic);
> > diff --git a/drivers/irqchip/irq-hip04.c b/drivers/irqchip/irq-hip04.c
> > index 46161f6ff289d..5285150fd9096 100644
> > --- a/drivers/irqchip/irq-hip04.c
> > +++ b/drivers/irqchip/irq-hip04.c
> > @@ -130,7 +130,7 @@ static int hip04_irq_set_type(struct irq_data *d, unsigned int type)
> >
> > raw_spin_lock(&irq_controller_lock);
> >
> > - ret = gic_configure_irq(irq, type, base + GIC_DIST_CONFIG, NULL);
> > + ret = gic_configure_irq(irq, type, base + GIC_DIST_CONFIG);
> > if (ret && irq < 32) {
> > /* Misconfigured PPIs are usually not fatal */
> > pr_warn("GIC: PPI%d is secure or misconfigured\n", irq - 16);
> > @@ -260,7 +260,7 @@ static void __init hip04_irq_dist_init(struct hip04_irq_data *intc)
> > for (i = 32; i < nr_irqs; i += 2)
> > writel_relaxed(cpumask, base + GIC_DIST_TARGET + ((i * 2) & ~3));
> >
> > - gic_dist_config(base, nr_irqs, NULL);
> > + gic_dist_config(base, nr_irqs);
> >
> > writel_relaxed(1, base + GIC_DIST_CTRL);
> > }
> > @@ -287,7 +287,7 @@ static void hip04_irq_cpu_init(struct hip04_irq_data *intc)
> > if (i != cpu)
> > hip04_cpu_map[i] &= ~cpu_mask;
> >
> > - gic_cpu_config(dist_base, 32, NULL);
> > + gic_cpu_config(dist_base, 32);
> >
> > writel_relaxed(0xf0, base + GIC_CPU_PRIMASK);
> > writel_relaxed(1, base + GIC_CPU_CTRL);
Powered by blists - more mailing lists