Patchwork [v3,10/27] genirq: Split __irq_reserve_irqs from irq_alloc_descs

login
register
mail settings
Submitter Yinghai Lu
Date June 7, 2013, 10:30 p.m.
Message ID <1370644273-10495-11-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/249884/
State Not Applicable
Headers show

Comments

Yinghai Lu - June 7, 2013, 10:30 p.m.
irq_alloc_descs and irq_reserve_irqs are almost the same.
Separate code out to __irq_reserved_irqs, and other two reuse
__irq_reserve_irqs.

We will use __irq_reserve_irqs for coming ioapic hotplug support.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Alexander Gordeev <agordeev@redhat.com>
---
 include/linux/irq.h  |  1 +
 kernel/irq/irqdesc.c | 54 ++++++++++++++++++++++++++++++----------------------
 2 files changed, 32 insertions(+), 23 deletions(-)
Alexander Gordeev - June 10, 2013, 1:51 p.m.
On Fri, Jun 07, 2013 at 03:30:56PM -0700, Yinghai Lu wrote:
> irq_alloc_descs and irq_reserve_irqs are almost the same.
> Separate code out to __irq_reserved_irqs, and other two reuse
> __irq_reserve_irqs.
> 
> We will use __irq_reserve_irqs for coming ioapic hotplug support.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Alexander Gordeev <agordeev@redhat.com>
> ---
>  include/linux/irq.h  |  1 +
>  kernel/irq/irqdesc.c | 54 ++++++++++++++++++++++++++++++----------------------
>  2 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index bc4e066..4e0fcbb 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -600,6 +600,7 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>  
>  void irq_free_descs(unsigned int irq, unsigned int cnt);
>  int irq_reserve_irqs(unsigned int from, unsigned int cnt);
> +int __irq_reserve_irqs(int irq, unsigned int from, unsigned int cnt);
>  
>  static inline void irq_free_desc(unsigned int irq)
>  {
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 192a302..3b9fb92 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -340,18 +340,15 @@ void irq_free_descs(unsigned int from, unsigned int cnt)
>  EXPORT_SYMBOL_GPL(irq_free_descs);
>  
>  /**
> - * irq_alloc_descs - allocate and initialize a range of irq descriptors
> - * @irq:	Allocate for specific irq number if irq >= 0
> + * __irq_reserve_descs - reserve and initialize a range of irq descriptors
> + * @irq:	Reserve for specific irq number if irq >= 0
>   * @from:	Start the search from this irq number
> - * @cnt:	Number of consecutive irqs to allocate.
> - * @node:	Preferred node on which the irq descriptor should be allocated
> - * @owner:	Owning module (can be NULL)
> + * @cnt:	Number of consecutive irqs to reserve.
>   *
>   * Returns the first irq number or error code
>   */
>  int __ref
> -__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> -		  struct module *owner)
> +__irq_reserve_irqs(int irq, unsigned int from, unsigned int cnt)
>  {
>  	int start, ret;
>  
> @@ -369,7 +366,7 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>  	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
>  					   from, cnt, 0);
>  	ret = -EEXIST;
> -	if (irq >=0 && start != irq)
> +	if (irq >= 0 && start != irq)
>  		goto err;
>  
>  	if (start + cnt > nr_irqs) {
> @@ -380,12 +377,36 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>  
>  	bitmap_set(allocated_irqs, start, cnt);
>  	mutex_unlock(&sparse_irq_lock);
> -	return alloc_descs(start, cnt, node, owner);
> +	return start;
>  
>  err:
>  	mutex_unlock(&sparse_irq_lock);
>  	return ret;
>  }
> +
> +/**
> + * irq_alloc_descs - allocate and initialize a range of irq descriptors
> + * @irq:	Allocate for specific irq number if irq >= 0
> + * @from:	Start the search from this irq number
> + * @cnt:	Number of consecutive irqs to allocate.
> + * @node:	Preferred node on which the irq descriptor should be allocated
> + * @owner:	Owning module (can be NULL)
> + *
> + * Returns the first irq number or error code
> + */
> +int __ref
> +__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> +		  struct module *owner)
> +{
> +	int start;
> +
> +	start = __irq_reserve_irqs(irq, from, cnt);
> +
> +	if (start < 0)
> +		return start;
> +
> +	return alloc_descs(start, cnt, node, owner);

I think alloc_descs() fail path is needed before return.

> +}
>  EXPORT_SYMBOL_GPL(__irq_alloc_descs);
>  
>  /**
> @@ -397,20 +418,7 @@ EXPORT_SYMBOL_GPL(__irq_alloc_descs);
>   */
>  int irq_reserve_irqs(unsigned int from, unsigned int cnt)
>  {
> -	unsigned int start;
> -	int ret = 0;
> -
> -	if (!cnt || (from + cnt) > nr_irqs)
> -		return -EINVAL;
> -
> -	mutex_lock(&sparse_irq_lock);
> -	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
> -	if (start == from)
> -		bitmap_set(allocated_irqs, start, cnt);
> -	else
> -		ret = -EEXIST;
> -	mutex_unlock(&sparse_irq_lock);
> -	return ret;
> +	return __irq_reserve_irqs(from, from, cnt);
>  }
>  
>  /**
> -- 
> 1.8.1.4
>
Yinghai Lu - June 10, 2013, 7:16 p.m.
On Mon, Jun 10, 2013 at 6:51 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> On Fri, Jun 07, 2013 at 03:30:56PM -0700, Yinghai Lu wrote:
>> irq_alloc_descs and irq_reserve_irqs are almost the same.
>> Separate code out to __irq_reserved_irqs, and other two reuse
>> __irq_reserve_irqs.
>>
>> We will use __irq_reserve_irqs for coming ioapic hotplug support.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Alexander Gordeev <agordeev@redhat.com>
>> ---
>>  include/linux/irq.h  |  1 +
>>  kernel/irq/irqdesc.c | 54 ++++++++++++++++++++++++++++++----------------------
>>  2 files changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index bc4e066..4e0fcbb 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -600,6 +600,7 @@ int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>>
>>  void irq_free_descs(unsigned int irq, unsigned int cnt);
>>  int irq_reserve_irqs(unsigned int from, unsigned int cnt);
>> +int __irq_reserve_irqs(int irq, unsigned int from, unsigned int cnt);
>>
>>  static inline void irq_free_desc(unsigned int irq)
>>  {
>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index 192a302..3b9fb92 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -340,18 +340,15 @@ void irq_free_descs(unsigned int from, unsigned int cnt)
>>  EXPORT_SYMBOL_GPL(irq_free_descs);
>>
>>  /**
>> - * irq_alloc_descs - allocate and initialize a range of irq descriptors
>> - * @irq:     Allocate for specific irq number if irq >= 0
>> + * __irq_reserve_descs - reserve and initialize a range of irq descriptors
>> + * @irq:     Reserve for specific irq number if irq >= 0
>>   * @from:    Start the search from this irq number
>> - * @cnt:     Number of consecutive irqs to allocate.
>> - * @node:    Preferred node on which the irq descriptor should be allocated
>> - * @owner:   Owning module (can be NULL)
>> + * @cnt:     Number of consecutive irqs to reserve.
>>   *
>>   * Returns the first irq number or error code
>>   */
>>  int __ref
>> -__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>> -               struct module *owner)
>> +__irq_reserve_irqs(int irq, unsigned int from, unsigned int cnt)
>>  {
>>       int start, ret;
>>
>> @@ -369,7 +366,7 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>>       start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
>>                                          from, cnt, 0);
>>       ret = -EEXIST;
>> -     if (irq >=0 && start != irq)
>> +     if (irq >= 0 && start != irq)
>>               goto err;
>>
>>       if (start + cnt > nr_irqs) {
>> @@ -380,12 +377,36 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>>
>>       bitmap_set(allocated_irqs, start, cnt);
>>       mutex_unlock(&sparse_irq_lock);
>> -     return alloc_descs(start, cnt, node, owner);
>> +     return start;
>>
>>  err:
>>       mutex_unlock(&sparse_irq_lock);
>>       return ret;
>>  }
>> +
>> +/**
>> + * irq_alloc_descs - allocate and initialize a range of irq descriptors
>> + * @irq:     Allocate for specific irq number if irq >= 0
>> + * @from:    Start the search from this irq number
>> + * @cnt:     Number of consecutive irqs to allocate.
>> + * @node:    Preferred node on which the irq descriptor should be allocated
>> + * @owner:   Owning module (can be NULL)
>> + *
>> + * Returns the first irq number or error code
>> + */
>> +int __ref
>> +__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
>> +               struct module *owner)
>> +{
>> +     int start;
>> +
>> +     start = __irq_reserve_irqs(irq, from, cnt);
>> +
>> +     if (start < 0)
>> +             return start;
>> +
>> +     return alloc_descs(start, cnt, node, owner);
>
> I think alloc_descs() fail path is needed before return.

__irq_reserve_irqs already return -EEXIST etc,

old kernel is like:

>> @@ -380,12 +377,36 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,

       bitmap_set(allocated_irqs, start, cnt);
       mutex_unlock(&sparse_irq_lock);
     return alloc_descs(start, cnt, node, owner);

 err:
      mutex_unlock(&sparse_irq_lock);
      return ret;

so i don't change the fail path handling.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner - June 10, 2013, 7:39 p.m.
On Fri, 7 Jun 2013, Yinghai Lu wrote:
>  /**
> - * irq_alloc_descs - allocate and initialize a range of irq descriptors
> - * @irq:	Allocate for specific irq number if irq >= 0
> + * __irq_reserve_descs - reserve and initialize a range of irq descriptors

Did you even bother to run docbook ?

No you didn't. Otherwise you'd have noticed that documentation for
__irq_reserve_descs is not the right thing for a function named
__irq_reserve_irqs.

> + * __irq_reserve_descs - reserve and initialize a range of irq descriptors

The function only reserves a range of irq descriptors and does not
initialize them.

> +
> +/**
> + * irq_alloc_descs - allocate and initialize a range of irq descriptors

Again you document non matching function names.

> +int __ref
> +__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> +		  struct module *owner)

Sloppy as usual along with a sucky changelog as usual.....

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Gordeev - June 10, 2013, 7:42 p.m.
On Mon, Jun 10, 2013 at 12:16:16PM -0700, Yinghai Lu wrote:
> On Mon, Jun 10, 2013 at 6:51 AM, Alexander Gordeev <agordeev@redhat.com> wrote:
> >> +/**
> >> + * irq_alloc_descs - allocate and initialize a range of irq descriptors
> >> + * @irq:     Allocate for specific irq number if irq >= 0
> >> + * @from:    Start the search from this irq number
> >> + * @cnt:     Number of consecutive irqs to allocate.
> >> + * @node:    Preferred node on which the irq descriptor should be allocated
> >> + * @owner:   Owning module (can be NULL)
> >> + *
> >> + * Returns the first irq number or error code
> >> + */
> >> +int __ref
> >> +__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> >> +               struct module *owner)
> >> +{
> >> +     int start;
> >> +
> >> +     start = __irq_reserve_irqs(irq, from, cnt);
> >> +
> >> +     if (start < 0)
> >> +             return start;
> >> +
> >> +     return alloc_descs(start, cnt, node, owner);
> >
> > I think alloc_descs() fail path is needed before return.
> 
> __irq_reserve_irqs already return -EEXIST etc,
> 
> old kernel is like:
> 
> >> @@ -380,12 +377,36 @@ __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
> 
>        bitmap_set(allocated_irqs, start, cnt);
>        mutex_unlock(&sparse_irq_lock);
>      return alloc_descs(start, cnt, node, owner);
> 
>  err:
>       mutex_unlock(&sparse_irq_lock);
>       return ret;
> 
> so i don't change the fail path handling.

I rather meant the bits should be unset in case alloc_descs() failed.
But I failed to notice alloc_descs() does it. Therefore..

Reviewed-by: Alexander Gordeev <agordeev@redhat.com>

> Thanks
> 
> Yinghai
Yinghai Lu - June 10, 2013, 11:55 p.m.
On Mon, Jun 10, 2013 at 12:39 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 7 Jun 2013, Yinghai Lu wrote:
>>  /**
>> - * irq_alloc_descs - allocate and initialize a range of irq descriptors
>> - * @irq:     Allocate for specific irq number if irq >= 0
>> + * __irq_reserve_descs - reserve and initialize a range of irq descriptors
>
> Did you even bother to run docbook ?
>
> No you didn't. Otherwise you'd have noticed that documentation for
> __irq_reserve_descs is not the right thing for a function named
> __irq_reserve_irqs.

sorry, will them consistent.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/irq.h b/include/linux/irq.h
index bc4e066..4e0fcbb 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -600,6 +600,7 @@  int __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 
 void irq_free_descs(unsigned int irq, unsigned int cnt);
 int irq_reserve_irqs(unsigned int from, unsigned int cnt);
+int __irq_reserve_irqs(int irq, unsigned int from, unsigned int cnt);
 
 static inline void irq_free_desc(unsigned int irq)
 {
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 192a302..3b9fb92 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -340,18 +340,15 @@  void irq_free_descs(unsigned int from, unsigned int cnt)
 EXPORT_SYMBOL_GPL(irq_free_descs);
 
 /**
- * irq_alloc_descs - allocate and initialize a range of irq descriptors
- * @irq:	Allocate for specific irq number if irq >= 0
+ * __irq_reserve_descs - reserve and initialize a range of irq descriptors
+ * @irq:	Reserve for specific irq number if irq >= 0
  * @from:	Start the search from this irq number
- * @cnt:	Number of consecutive irqs to allocate.
- * @node:	Preferred node on which the irq descriptor should be allocated
- * @owner:	Owning module (can be NULL)
+ * @cnt:	Number of consecutive irqs to reserve.
  *
  * Returns the first irq number or error code
  */
 int __ref
-__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
-		  struct module *owner)
+__irq_reserve_irqs(int irq, unsigned int from, unsigned int cnt)
 {
 	int start, ret;
 
@@ -369,7 +366,7 @@  __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 	start = bitmap_find_next_zero_area(allocated_irqs, IRQ_BITMAP_BITS,
 					   from, cnt, 0);
 	ret = -EEXIST;
-	if (irq >=0 && start != irq)
+	if (irq >= 0 && start != irq)
 		goto err;
 
 	if (start + cnt > nr_irqs) {
@@ -380,12 +377,36 @@  __irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
 
 	bitmap_set(allocated_irqs, start, cnt);
 	mutex_unlock(&sparse_irq_lock);
-	return alloc_descs(start, cnt, node, owner);
+	return start;
 
 err:
 	mutex_unlock(&sparse_irq_lock);
 	return ret;
 }
+
+/**
+ * irq_alloc_descs - allocate and initialize a range of irq descriptors
+ * @irq:	Allocate for specific irq number if irq >= 0
+ * @from:	Start the search from this irq number
+ * @cnt:	Number of consecutive irqs to allocate.
+ * @node:	Preferred node on which the irq descriptor should be allocated
+ * @owner:	Owning module (can be NULL)
+ *
+ * Returns the first irq number or error code
+ */
+int __ref
+__irq_alloc_descs(int irq, unsigned int from, unsigned int cnt, int node,
+		  struct module *owner)
+{
+	int start;
+
+	start = __irq_reserve_irqs(irq, from, cnt);
+
+	if (start < 0)
+		return start;
+
+	return alloc_descs(start, cnt, node, owner);
+}
 EXPORT_SYMBOL_GPL(__irq_alloc_descs);
 
 /**
@@ -397,20 +418,7 @@  EXPORT_SYMBOL_GPL(__irq_alloc_descs);
  */
 int irq_reserve_irqs(unsigned int from, unsigned int cnt)
 {
-	unsigned int start;
-	int ret = 0;
-
-	if (!cnt || (from + cnt) > nr_irqs)
-		return -EINVAL;
-
-	mutex_lock(&sparse_irq_lock);
-	start = bitmap_find_next_zero_area(allocated_irqs, nr_irqs, from, cnt, 0);
-	if (start == from)
-		bitmap_set(allocated_irqs, start, cnt);
-	else
-		ret = -EEXIST;
-	mutex_unlock(&sparse_irq_lock);
-	return ret;
+	return __irq_reserve_irqs(from, from, cnt);
 }
 
 /**