Re: [patch 02/46] genirq/irqdesc: Switch to lock guards

[Jiri Slaby]

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@xxxxxxxxxxxxx>
> ---
>   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