| 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
| ||
|
Message-ID: <877c3uuy7u.ffs@tglx>
Date: Tue, 08 Apr 2025 23:42:29 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>, Marc Zyngier
<maz@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Catalin Marinas
<catalin.marinas@....com>, Will Deacon <will@...nel.org>
Cc: Sascha Bischoff <sascha.bischoff@....com>, Timothy Hayes
<timothy.hayes@....com>, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, Lorenzo
Pieralisi <lpieralisi@...nel.org>
Subject: Re: [PATCH 18/24] irqchip/gic-v5: Add GICv5 PPI support
On Tue, Apr 08 2025 at 12:50, Lorenzo Pieralisi wrote:
> +
> +static void gicv5_ppi_priority_init(void)
> +{
> + write_sysreg_s(REPEAT_BYTE(GICV5_IRQ_PRIORITY_MI),
> + SYS_ICC_PPI_PRIORITYR0_EL1);
Just let stick it out. You have 100 characters. All over the place...
> +static int gicv5_ppi_irq_set_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which,
> + bool val)
> +{
> + u64 hwirq_id_bit = BIT_ULL(d->hwirq % 64);
> +
> + switch (which) {
> + case IRQCHIP_STATE_PENDING:
> + if (val) {
> + if (d->hwirq < 64)
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_SPENDR0_EL1);
> + else
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_SPENDR1_EL1);
> +
> + } else {
> + if (d->hwirq < 64)
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_CPENDR0_EL1);
> + else
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_CPENDR1_EL1);
> + }
> +
> + return 0;
> + case IRQCHIP_STATE_ACTIVE:
> + if (val) {
> + if (d->hwirq < 64)
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_SACTIVER0_EL1);
> + else
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_SACTIVER1_EL1);
> + } else {
> + if (d->hwirq < 64)
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_CACTIVER0_EL1);
> + else
> + write_sysreg_s(hwirq_id_bit,
> + SYS_ICC_PPI_CACTIVER1_EL1);
> + }
You already precalculate hwirq_id_bit. Can't you do something similar
for the registers?
case IRQCHIP_STATE_PENDING:
u32 reg = val ? SYS_ICC_PPI_SPENDR1_EL1 : SYS_ICC_PPI_SPENDR0_EL1;
write_sysreg_s(hwirq_id_bit, reg);
return 0;
case IRQCHIP_STATE_ACTIVE:
....
Ditto in the get_state() function.
No?
> +static int gicv5_irq_ppi_domain_translate(struct irq_domain *d,
> + struct irq_fwspec *fwspec,
> + irq_hw_number_t *hwirq,
> + unsigned int *type)
> +{
> + if (is_of_node(fwspec->fwnode)) {
It'd be way more readable to invert this check
if (!is_of_node(...))
return -EINVAL;
so that the subsequent checks are just a read through.
> + if (fwspec->param_count < 3)
> + return -EINVAL;
> +
> + if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI)
> + return -EINVAL;
> +
> + *hwirq = fwspec->param[1];
> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +static void gicv5_irq_ppi_domain_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct irq_data *d;
> +
> + if (WARN_ON(nr_irqs != 1))
WARN_ON_ONCE ?
> + return;
> +
> + d = irq_domain_get_irq_data(domain, virq);
> +
> + irq_set_handler(virq, NULL);
> + irq_domain_reset_irq_data(d);
> +}
> +
> +static int gicv5_irq_ppi_domain_select(struct irq_domain *d,
> + struct irq_fwspec *fwspec,
> + enum irq_domain_bus_token bus_token)
> +{
> + /* Not for us */
> + if (fwspec->fwnode != d->fwnode)
> + return 0;
> +
> + if (fwspec->param[0] != GICV5_HWIRQ_TYPE_PPI) {
> + // only handle PPIs
Commenting the obvious?
> + return 0;
> + }
> +
> + return (d == gicv5_global_data.ppi_domain);
> +}
> +
> +static const struct irq_domain_ops gicv5_irq_ppi_domain_ops = {
> + .translate = gicv5_irq_ppi_domain_translate,
> + .alloc = gicv5_irq_ppi_domain_alloc,
> + .free = gicv5_irq_ppi_domain_free,
> + .select = gicv5_irq_ppi_domain_select
> +};
> +
> +static inline void handle_irq_per_domain(u32 hwirq)
> +{
> + u32 hwirq_id;
> + struct irq_domain *domain = NULL;
> + u8 hwirq_type = FIELD_GET(GICV5_HWIRQ_TYPE, hwirq);
So far you managed to comply with the documented reverse fir tree
ordering.
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#variable-declarations
Why are you changing coding style in the middle of the code?
> +
> + hwirq_id = FIELD_GET(GICV5_HWIRQ_ID, hwirq);
> +
> + if (hwirq_type == GICV5_HWIRQ_TYPE_PPI)
> + domain = gicv5_global_data.ppi_domain;
> +
> + if (generic_handle_domain_irq(domain, hwirq_id)) {
> + pr_err("Could not handle, hwirq = 0x%x", hwirq_id);
pr_err_once() perhaps?
> + gicv5_hwirq_eoi(hwirq_id, hwirq_type);
> + }
> +}
> +
> +static asmlinkage void __exception_irq_entry
> +gicv5_handle_irq(struct pt_regs *regs)
> +{
> + u64 ia;
> + bool valid;
> + u32 hwirq;
See above
> + ia = gicr_insn(GICV5_OP_GICR_CDIA);
> + valid = GICV5_GIC_CDIA_VALID(ia);
And please move that to the declaration lines
> +static int __init gicv5_init_domains(struct fwnode_handle *handle)
> +{
> + gicv5_global_data.fwnode = handle;
> + gicv5_global_data.ppi_domain = irq_domain_create_linear(
> + handle, 128, &gicv5_irq_ppi_domain_ops, NULL);
The ever changing choice of coding styles across functions is really
interesting. Obviously the length of 'gicv5_global_data.ppi_domain'
forces ugly, but that does not mean it needs to be that way:
struct irqdomain *d;
d = irq_domain_create_linear(handle, 128, &gicv5_irq_ppi_domain_ops, NULL);
if (!d)
return - ENOMEM;
irq_domain_update_bus_token(d, DOMAIN_BUS_WIRED);
gicv5_global_data.fwnode = handle;
gicv5_global_data.ppi_domain = d;
return 0;
No?
> +static int __init gicv5_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + int ret;
> +
> + ret = gicv5_init_domains(&node->fwnode);
> + if (ret)
> + return ret;
> +
> + gicv5_set_cpuif_pribits();
> +
> + ret = gicv5_starting_cpu(smp_processor_id());
You invoke the CPU hotplug callback for the boot CPU explicitly, but
what the heck installs the actual hotplug callback for the secondary
CPUs?
Thanks,
tglx
Powered by blists - more mailing lists