| 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: <1e1ffb3a-3354-4c35-b73f-787da014f758@kernel.org>
Date: Fri, 14 Mar 2025 11:57:24 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>, LKML <linux-kernel@...r.kernel.org>
Cc: Peter Zijlstra <peterz@...radead.org>
Subject: Re: [patch 02/46] genirq/irqdesc: Switch to lock guards
On 13. 03. 25, 16:59, Thomas Gleixner wrote:
> Replace all lock/unlock pairs with lock guards and simplify the code flow.
>
> No functional change.
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> kernel/irq/irqdesc.c | 136 +++++++++++++++------------------------------------
> 1 file changed, 42 insertions(+), 94 deletions(-)
>
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -266,104 +266,68 @@ static ssize_t per_cpu_count_show(struct
> }
> IRQ_ATTR_RO(per_cpu_count);
>
> -static ssize_t chip_name_show(struct kobject *kobj,
> - struct kobj_attribute *attr, char *buf)
> +static ssize_t chip_name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
> - ssize_t ret = 0;
> -
> - raw_spin_lock_irq(&desc->lock);
> - if (desc->irq_data.chip && desc->irq_data.chip->name) {
> - ret = scnprintf(buf, PAGE_SIZE, "%s\n",
> - desc->irq_data.chip->name);
> - }
> - raw_spin_unlock_irq(&desc->lock);
>
> - return ret;
> + guard(raw_spinlock_irq)(&desc->lock);
> + if (desc->irq_data.chip && desc->irq_data.chip->name)
> + return scnprintf(buf, PAGE_SIZE, "%s\n", desc->irq_data.chip->name);
> + return 0;
FWIW I consider this ^^^ ...
> }
> IRQ_ATTR_RO(chip_name);
>
> -static ssize_t hwirq_show(struct kobject *kobj,
> - struct kobj_attribute *attr, char *buf)
> +static ssize_t hwirq_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
> - ssize_t ret = 0;
> -
> - raw_spin_lock_irq(&desc->lock);
> - if (desc->irq_data.domain)
> - ret = sprintf(buf, "%lu\n", desc->irq_data.hwirq);
> - raw_spin_unlock_irq(&desc->lock);
>
> - return ret;
> + guard(raw_spinlock_irq)(&desc->lock);
> + return desc->irq_data.domain ? sprintf(buf, "%lu\n", desc->irq_data.hwirq) : 0;
... more readable than this ^^^. (Ie. I would preserve the 'if'. Even
though here is only a simple condition.)
> }
> IRQ_ATTR_RO(hwirq);
> -static ssize_t name_show(struct kobject *kobj,
> - struct kobj_attribute *attr, char *buf)
> +static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
> - ssize_t ret = 0;
>
> - raw_spin_lock_irq(&desc->lock);
> - if (desc->name)
> - ret = scnprintf(buf, PAGE_SIZE, "%s\n", desc->name);
> - raw_spin_unlock_irq(&desc->lock);
> -
> - return ret;
> + guard(raw_spinlock_irq)(&desc->lock);
You do guard() ...
> + return desc->name ? scnprintf(buf, PAGE_SIZE, "%s\n", desc->name) : 0;
> }
> IRQ_ATTR_RO(name);
>
> -static ssize_t actions_show(struct kobject *kobj,
> - struct kobj_attribute *attr, char *buf)
> +static ssize_t actions_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> {
> struct irq_desc *desc = container_of(kobj, struct irq_desc, kobj);
> struct irqaction *action;
> ssize_t ret = 0;
> char *p = "";
>
> - raw_spin_lock_irq(&desc->lock);
> - for_each_action_of_desc(desc, action) {
> - ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%s",
> - p, action->name);
> - p = ",";
> + scoped_guard (raw_spinlock_irq, &desc->lock) {
... but scoped_guard<space>(). Unlike in internals.h where you do
scoped_guard(). Any reason for that?
> + for_each_action_of_desc(desc, action) {
> + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%s%s", p, action->name);
> + p = ",";
> + }
> }
> - raw_spin_unlock_irq(&desc->lock);
> -
> - if (ret)
> - ret += scnprintf(buf + ret, PAGE_SIZE - ret, "\n");
>
> - return ret;
> + return ret ? ret + scnprintf(buf + ret, PAGE_SIZE - ret, "\n") : 0;
> }
> IRQ_ATTR_RO(actions);
...
> @@ -573,12 +532,12 @@ static int alloc_descs(unsigned int star
> return -ENOMEM;
> }
>
> -static int irq_expand_nr_irqs(unsigned int nr)
> +static bool irq_expand_nr_irqs(unsigned int nr)
> {
> if (nr > MAX_SPARSE_IRQS)
> - return -ENOMEM;
> + return false;
> nr_irqs = nr;
> - return 0;
> + return true;
This is sort of unrelated to $SUBJ ^^^. In any case:
> @@ -681,14 +638,13 @@ static inline int alloc_descs(unsigned i
>
> static int irq_expand_nr_irqs(unsigned int nr)
s/int/bool/ here too.
> {
> - return -ENOMEM;
> + return false;
> }
thanks,
--
js
suse labs
Powered by blists - more mailing lists