Re: [PATCH RFC 1/2] irqchip/ls-scfg-msi: refactor allocation to bitmap_find_free_region()

[Thomas Gleixner]

On Thu, Jul 16 2026 at 12:15, Alexander Wilhelm wrote:
> Replace the manual find_first_zero_bit()/__set_bit() pair with
> bitmap_find_free_region(), and __clear_bit() with bitmap_release_region().

Please don't enumerate WHAT the patch is doing. Explain context, problem
and solution in that order:

  https://docs.kernel.org/process/maintainer-tip.html#changelog

> @@ -141,28 +141,32 @@ static int ls_scfg_msi_domain_irq_alloc(struct irq_domain *domain,
>  {
>  	msi_alloc_info_t *info = args;
>  	struct ls_scfg_msi *msi_data = domain->host_data;
> -	int pos, err = 0;
> +	int order = get_count_order(nr_irqs);
> +	int pos, err;
> +	unsigned int i;

See the variable declaration chapter in the documentation above.

>  	WARN_ON(nr_irqs != 1);
>  
>  	spin_lock(&msi_data->lock);
> -	pos = find_first_zero_bit(msi_data->used, msi_data->irqs_num);
> -	if (pos < msi_data->irqs_num)
> -		__set_bit(pos, msi_data->used);
> -	else
> -		err = -ENOSPC;
> +	pos = bitmap_find_free_region(msi_data->used, msi_data->irqs_num,
> +				      order);
>  	spin_unlock(&msi_data->lock);

Please convert this to

	scoped_guard(spinlock, &msi_data->lock)
		pos = bitmap_find_free_region(msi_data->used, msi_data->irqs_num, order);

No line break required. You have 100 characters. 
  
> -	if (err)
> -		return err;
> +	if (pos < 0)
> +		return pos;
>  
>  	err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr);
> -	if (err)
> +	if (err) {
> +		spin_lock(&msi_data->lock);
> +		bitmap_release_region(msi_data->used, pos, order);
> +		spin_unlock(&msi_data->lock);

scoped_guard() please

>  		return err;
> +	}
>  
> -	irq_domain_set_info(domain, virq, pos,
> -			    &ls_scfg_msi_parent_chip, msi_data,
> -			    handle_simple_irq, NULL, NULL);
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_domain_set_info(domain, virq + i, pos + i,
> +				    &ls_scfg_msi_parent_chip, msi_data,
> +				    handle_simple_irq, NULL, NULL);
>  

Lacks brackets. See bracket rules in documentation.

>  	return 0;
>  }
> @@ -172,16 +176,18 @@ static void ls_scfg_msi_domain_irq_free(struct irq_domain *domain,
>  {
>  	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>  	struct ls_scfg_msi *msi_data = irq_data_get_irq_chip_data(d);
> +	int order = get_count_order(nr_irqs);
>  	int pos;
>  
>  	pos = d->hwirq;
> -	if (pos < 0 || pos >= msi_data->irqs_num) {
> -		pr_err("failed to teardown msi. Invalid hwirq %d\n", pos);
> +	if (pos < 0 || pos + nr_irqs > msi_data->irqs_num) {
> +		pr_err("failed to teardown msi. Invalid hwirq %d nr %u\n",
> +		       pos, nr_irqs);

No line break required.

>  		return;
>  	}
>  
>  	spin_lock(&msi_data->lock);
> -	__clear_bit(pos, msi_data->used);
> +	bitmap_release_region(msi_data->used, pos, order);
>  	spin_unlock(&msi_data->lock);
>  }

Thanks,

        tglx