[PATCH]: complete cleanup of check_region

[PATCH]: complete cleanup of check_region

[Surya]

Hi all,
	This patch cleans up all the instances of check_region and
__check_region and replaces them with request_region and
__request_region. Applies and compiles clean on latest Linus tree.

Files affected:
	drivers/cdrom/sbpcd.c
	drivers/pnp/resource.c
	include/linux/ioport.h
	kernel/resource.c
	sound/oss/pss.c


thanks.


Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>
--- 

diff --git a/drivers/cdrom/sbpcd.c b/drivers/cdrom/sbpcd.c
index a1283b1..2c1355e 100644
--- a/drivers/cdrom/sbpcd.c
+++ b/drivers/cdrom/sbpcd.c
@@ -358,6 +358,11 @@
  * Add bio/kdev_t changes for 2.5.x required to make it work again. 
  * Still room for improvement in the request handling here if anyone
  * actually cares.  Bring your own chainsaw.    Paul G.  02/2002
+ *
+ *
+ * Cleaned up the reference for deprecated check_region to 
+ * request_region.
+ * Thu Jun  7 12:14:00 IST 2007 Surya <surya.prabhakar@wipro.com>
  */
 
 
@@ -5670,7 +5675,7 @@ int __init sbpcd_init(void)
 	{
 		addr[1]=sbpcd[port_index];
 		if (addr[1]==0) break;
-		if (check_region(addr[1],4))
+		if (request_region(addr[1],4, "sbpcd driver"))
 		{
 			msg(DBG_INF,"check_region: %03X is not free.\n",addr[1]);
 			continue;
diff --git a/drivers/pnp/resource.c b/drivers/pnp/resource.c
index a685fbe..2be8001 100644
--- a/drivers/pnp/resource.c
+++ b/drivers/pnp/resource.c
@@ -251,7 +251,7 @@ int pnp_check_port(struct pnp_dev * dev, int idx)
 	/* check if the resource is already in use, skip if the
 	 * device is active because it itself may be in use */
 	if(!dev->active) {
-		if (__check_region(&ioport_resource, *port, length(port,end)))
+		if (__request_region(&ioport_resource, *port, length(port,end), "isapnp request-region"))
 			return 0;
 	}
 
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 71ea923..ebe6c22 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -124,19 +124,12 @@ extern struct resource * __request_region(struct resource *,
 
 /* Compatibility cruft */
 #define release_region(start,n)	__release_region(&ioport_resource, (start), (n))
-#define check_mem_region(start,n)	__check_region(&iomem_resource, (start), (n))
+#define check_mem_region(start,n)	__request_region(&iomem_resource, (start), (n), "request-region")
 #define release_mem_region(start,n)	__release_region(&iomem_resource, (start), (n))
 
-extern int __check_region(struct resource *, resource_size_t, resource_size_t);
 extern void __release_region(struct resource *, resource_size_t,
 				resource_size_t);
 
-static inline int __deprecated check_region(resource_size_t s,
-						resource_size_t n)
-{
-	return __check_region(&ioport_resource, s, n);
-}
-
 /* Wrappers for managed devices */
 struct device;
 #define devm_request_region(dev,start,n,name) \
diff --git a/kernel/resource.c b/kernel/resource.c
index 9bd14fd..99a97ca 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -520,36 +520,6 @@ struct resource * __request_region(struct resource *parent,
 EXPORT_SYMBOL(__request_region);
 
 /**
- * __check_region - check if a resource region is busy or free
- * @parent: parent resource descriptor
- * @start: resource start address
- * @n: resource region size
- *
- * Returns 0 if the region is free at the moment it is checked,
- * returns %-EBUSY if the region is busy.
- *
- * NOTE:
- * This function is deprecated because its use is racy.
- * Even if it returns 0, a subsequent call to request_region()
- * may fail because another driver etc. just allocated the region.
- * Do NOT use it.  It will be removed from the kernel.
- */
-int __check_region(struct resource *parent, resource_size_t start,
-			resource_size_t n)
-{
-	struct resource * res;
-
-	res = __request_region(parent, start, n, "check-region");
-	if (!res)
-		return -EBUSY;
-
-	release_resource(res);
-	kfree(res);
-	return 0;
-}
-EXPORT_SYMBOL(__check_region);
-
-/**
  * __release_region - release a previously reserved resource region
  * @parent: parent resource descriptor
  * @start: resource start address
diff --git a/sound/oss/pss.c b/sound/oss/pss.c
index ece428b..c61a1a3 100644
--- a/sound/oss/pss.c
+++ b/sound/oss/pss.c
@@ -54,6 +54,9 @@
  *	    Added __init to probe_pss(), attach_pss() and probe_pss_mpu()
  * 02-Jan-2001: Chris Rankin
  *          Specify that this module owns the coprocessor
+ * 07-Jun-2007: Surya Prabhakar <surya.prabhakar@wipro.com>
+ *	    Cleaned up the reference for deprecated check_region to 
+ * 	    request_region.
  */
 
 
@@ -677,7 +680,7 @@ static void configure_nonsound_components(void)
 	{
 		printk(KERN_INFO "PSS: CDROM port not enabled.\n");
 	}
-	else if(check_region(pss_cdrom_port, 2))
+	else if(request_region(pss_cdrom_port, 2, "CDROM config"))
 	{
 		printk(KERN_ERR "PSS: CDROM I/O port conflict.\n");
 	}


-- 
surya .
07/06/2007

Re: [PATCH]: complete cleanup of check_region

[Jesper Juhl]

On 07/06/07, Surya <surya.prabhakar@wipro.com> wrote:
> Hi all,
>         This patch cleans up all the instances of check_region and
> __check_region and replaces them with request_region and
> __request_region. Applies and compiles clean on latest Linus tree.
>
> Files affected:
>         drivers/cdrom/sbpcd.c
>         drivers/pnp/resource.c
>         include/linux/ioport.h
>         kernel/resource.c
>         sound/oss/pss.c
>
>
> thanks.
>
>
> Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>
> ---
>
> diff --git a/drivers/cdrom/sbpcd.c b/drivers/cdrom/sbpcd.c
> index a1283b1..2c1355e 100644
> --- a/drivers/cdrom/sbpcd.c
> +++ b/drivers/cdrom/sbpcd.c
> @@ -358,6 +358,11 @@
>   * Add bio/kdev_t changes for 2.5.x required to make it work again.
>   * Still room for improvement in the request handling here if anyone
>   * actually cares.  Bring your own chainsaw.    Paul G.  02/2002
> + *
> + *
> + * Cleaned up the reference for deprecated check_region to
> + * request_region.
> + * Thu Jun  7 12:14:00 IST 2007 Surya <surya.prabhakar@wipro.com>
>   */
>
>
> @@ -5670,7 +5675,7 @@ int __init sbpcd_init(void)
>         {
>                 addr[1]=sbpcd[port_index];
>                 if (addr[1]==0) break;
> -               if (check_region(addr[1],4))
> +               if (request_region(addr[1],4, "sbpcd driver"))

No!  You can't just swap one for the other.

check_region() simply checks if the region is available, it doesn't
reserve it (well, it does, briefly, but it lets it go again).
request_region() reserves the region. That's different behaviour that
you need to take into account.

Then there's the matter of return values. check_region() returns 0 on
success while request_region returns != 0 on success. I don't see your
patch dealing with that.

And finally, now that you request (and thus reserve) these regions,
where is the code to release them again when they are no longer
needed. just as memory allocated with kmalloc() needs to be freed with
kfree() after use, so regions reserved with request_region() need to
be released again with release_region() when they are no longer
needed. I don't see anything in your patch that releases the requested
regions.

Same comments for the rest of the patch.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH]: complete cleanup of check_region

[Eberhard Moenkeberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5485 bytes --]

Hi,

On Thu, 7 Jun 2007, Surya wrote:

> 
> Hi all,
> 	This patch cleans up all the instances of check_region and
> __check_region and replaces them with request_region and
> __request_region. Applies and compiles clean on latest Linus tree.
> 
> Files affected:
> 	drivers/cdrom/sbpcd.c
> 	drivers/pnp/resource.c
> 	include/linux/ioport.h
> 	kernel/resource.c
> 	sound/oss/pss.c
> 
> 
> thanks.
> 
> 
> Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>


in drivers/cdrom/sbpcd.c the message string should be changed too.

Acked-by: Eberhard Moenkeberg <emoenke@gwdg.de>

> --- 
> 
> diff --git a/drivers/cdrom/sbpcd.c b/drivers/cdrom/sbpcd.c
> index a1283b1..2c1355e 100644
> --- a/drivers/cdrom/sbpcd.c
> +++ b/drivers/cdrom/sbpcd.c
> @@ -358,6 +358,11 @@
>   * Add bio/kdev_t changes for 2.5.x required to make it work again. 
>   * Still room for improvement in the request handling here if anyone
>   * actually cares.  Bring your own chainsaw.    Paul G.  02/2002
> + *
> + *
> + * Cleaned up the reference for deprecated check_region to 
> + * request_region.
> + * Thu Jun  7 12:14:00 IST 2007 Surya <surya.prabhakar@wipro.com>
>   */
>  
>  
> @@ -5670,7 +5675,7 @@ int __init sbpcd_init(void)
>  	{
>  		addr[1]=sbpcd[port_index];
>  		if (addr[1]==0) break;
> -		if (check_region(addr[1],4))
> +		if (request_region(addr[1],4, "sbpcd driver"))
>  		{
>  			msg(DBG_INF,"check_region: %03X is not free.\n",addr[1]);
>  			continue;
> diff --git a/drivers/pnp/resource.c b/drivers/pnp/resource.c
> index a685fbe..2be8001 100644
> --- a/drivers/pnp/resource.c
> +++ b/drivers/pnp/resource.c
> @@ -251,7 +251,7 @@ int pnp_check_port(struct pnp_dev * dev, int idx)
>  	/* check if the resource is already in use, skip if the
>  	 * device is active because it itself may be in use */
>  	if(!dev->active) {
> -		if (__check_region(&ioport_resource, *port, length(port,end)))
> +		if (__request_region(&ioport_resource, *port, length(port,end), "isapnp request-region"))
>  			return 0;
>  	}
>  
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 71ea923..ebe6c22 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -124,19 +124,12 @@ extern struct resource * __request_region(struct resource *,
>  
>  /* Compatibility cruft */
>  #define release_region(start,n)	__release_region(&ioport_resource, (start), (n))
> -#define check_mem_region(start,n)	__check_region(&iomem_resource, (start), (n))
> +#define check_mem_region(start,n)	__request_region(&iomem_resource, (start), (n), "request-region")
>  #define release_mem_region(start,n)	__release_region(&iomem_resource, (start), (n))
>  
> -extern int __check_region(struct resource *, resource_size_t, resource_size_t);
>  extern void __release_region(struct resource *, resource_size_t,
>  				resource_size_t);
>  
> -static inline int __deprecated check_region(resource_size_t s,
> -						resource_size_t n)
> -{
> -	return __check_region(&ioport_resource, s, n);
> -}
> -
>  /* Wrappers for managed devices */
>  struct device;
>  #define devm_request_region(dev,start,n,name) \
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 9bd14fd..99a97ca 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -520,36 +520,6 @@ struct resource * __request_region(struct resource *parent,
>  EXPORT_SYMBOL(__request_region);
>  
>  /**
> - * __check_region - check if a resource region is busy or free
> - * @parent: parent resource descriptor
> - * @start: resource start address
> - * @n: resource region size
> - *
> - * Returns 0 if the region is free at the moment it is checked,
> - * returns %-EBUSY if the region is busy.
> - *
> - * NOTE:
> - * This function is deprecated because its use is racy.
> - * Even if it returns 0, a subsequent call to request_region()
> - * may fail because another driver etc. just allocated the region.
> - * Do NOT use it.  It will be removed from the kernel.
> - */
> -int __check_region(struct resource *parent, resource_size_t start,
> -			resource_size_t n)
> -{
> -	struct resource * res;
> -
> -	res = __request_region(parent, start, n, "check-region");
> -	if (!res)
> -		return -EBUSY;
> -
> -	release_resource(res);
> -	kfree(res);
> -	return 0;
> -}
> -EXPORT_SYMBOL(__check_region);
> -
> -/**
>   * __release_region - release a previously reserved resource region
>   * @parent: parent resource descriptor
>   * @start: resource start address
> diff --git a/sound/oss/pss.c b/sound/oss/pss.c
> index ece428b..c61a1a3 100644
> --- a/sound/oss/pss.c
> +++ b/sound/oss/pss.c
> @@ -54,6 +54,9 @@
>   *	    Added __init to probe_pss(), attach_pss() and probe_pss_mpu()
>   * 02-Jan-2001: Chris Rankin
>   *          Specify that this module owns the coprocessor
> + * 07-Jun-2007: Surya Prabhakar <surya.prabhakar@wipro.com>
> + *	    Cleaned up the reference for deprecated check_region to 
> + * 	    request_region.
>   */
>  
>  
> @@ -677,7 +680,7 @@ static void configure_nonsound_components(void)
>  	{
>  		printk(KERN_INFO "PSS: CDROM port not enabled.\n");
>  	}
> -	else if(check_region(pss_cdrom_port, 2))
> +	else if(request_region(pss_cdrom_port, 2, "CDROM config"))
>  	{
>  		printk(KERN_ERR "PSS: CDROM I/O port conflict.\n");
>  	}
> 
> 
> -- 
> surya .
> 07/06/2007


Viele Grüße
Eberhard Mönkeberg (emoenke@gwdg.de, em@kki.org)

Re: [PATCH]: complete cleanup of check_region

[Jesper Juhl]

On 07/06/07, Eberhard Moenkeberg <emoenke@gwdg.de> wrote:
> Hi,
>
> On Thu, 7 Jun 2007, Surya wrote:
>
> >
> > Hi all,
> >       This patch cleans up all the instances of check_region and
> > __check_region and replaces them with request_region and
> > __request_region. Applies and compiles clean on latest Linus tree.
> >
> > Files affected:
> >       drivers/cdrom/sbpcd.c
> >       drivers/pnp/resource.c
> >       include/linux/ioport.h
> >       kernel/resource.c
> >       sound/oss/pss.c
> >
> >
> > thanks.
> >
> >
> > Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>
>
>
> in drivers/cdrom/sbpcd.c the message string should be changed too.
>
> Acked-by: Eberhard Moenkeberg <emoenke@gwdg.de>
>

Huh? I don't understand how you can ACK that patch.
Are my comments here:  http://lkml.org/lkml/2007/6/7/102  completely
out in left field?

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH]: complete cleanup of check_region

[Eberhard Moenkeberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1125 bytes --]

Hi,

On Thu, 7 Jun 2007, Jesper Juhl wrote:
> On 07/06/07, Eberhard Moenkeberg <emoenke@gwdg.de> wrote:
> > On Thu, 7 Jun 2007, Surya wrote:
> > > Hi all,

> > >       This patch cleans up all the instances of check_region and
> > > __check_region and replaces them with request_region and
> > > __request_region. Applies and compiles clean on latest Linus tree.
> > >
> > > Files affected:
> > >       drivers/cdrom/sbpcd.c
> > >       drivers/pnp/resource.c
> > >       include/linux/ioport.h
> > >       kernel/resource.c
> > >       sound/oss/pss.c
> > >
> > >
> > > thanks.
> > >
> > >
> > > Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>
> >
> >
> > in drivers/cdrom/sbpcd.c the message string should be changed too.
> >
> > Acked-by: Eberhard Moenkeberg <emoenke@gwdg.de>
> >
> 
> Huh? I don't understand how you can ACK that patch.
> Are my comments here:  http://lkml.org/lkml/2007/6/7/102  completely
> out in left field?

Yes, my ACK is invalid. It has an earlier time stamp than your clearing 
mail.


Viele Grüße
Eberhard Mönkeberg (emoenke@gwdg.de, em@kki.org)

Re: [PATCH]: complete cleanup of check_region

[Surya]

<snip>
> >
> > @@ -5640,6 +5645,7 @@ int __init sbpcd_init(void)
> >         int i=0, j=0;
> >         int addr[2]={1, CDROM_PORT};
> >         int port_index;
> > +       int request_region_flag;
> >
> One could argue that it would be possible to just use the 'j' variable
> for this since it's not used for anything else until the *_region()
> stuff is all done, that would save adding a new variable...
completely eliminated the variable, haven't felt the need for it.

<snip>
> >                 msg(DBG_INI,"check_drives done.\n");
> > +               if(request_region_flag==0)
> > +                       release_region(addr[1],4);
> 
> I know the driver in its current version just tests if the region is
> available and doesn't hang on to it, but I'm wondering if that's not a
> bug.  Shouldn't we actually hold on to this region as long as the
> driver is loaded and only release the region in the sbpcd_exit()
> function, just loke we do with the "CDo_command" region?  
But if any of the conditions fail after the request_region code, it is
returning an error and exiting out of init. Dont we need to cleanup
request_region's allocation before we exit. Infact I had a feeling that
CDo_command region cleanup was not perfect. Did a cleanup of both the
regions wherever we have a return out of the function.

Please have a look at the below patch.


diff --git a/drivers/cdrom/sbpcd.c b/drivers/cdrom/sbpcd.c
index a1283b1..0a85b1b 100644
--- a/drivers/cdrom/sbpcd.c
+++ b/drivers/cdrom/sbpcd.c
@@ -358,6 +358,10 @@
  * Add bio/kdev_t changes for 2.5.x required to make it work again. 
  * Still room for improvement in the request handling here if anyone
  * actually cares.  Bring your own chainsaw.    Paul G.  02/2002
+ *
+ *
+ * Cleaned up the reference for deprecated check_region to 
+ * request_region.            - Surya Prabhakar N      08/07/2007
  */
 
 
@@ -555,6 +559,8 @@ static struct cdrom_read_audio read_audio;
 static unsigned char msgnum;
 static char msgbuf[80];
 
+static int addr[2];
+
 static int max_drives = MAX_DRIVES;
 module_param(max_drives, int, 0);
 #ifndef MODULE
@@ -5638,7 +5644,7 @@ int __init sbpcd_init(void)
 #endif
 {
 	int i=0, j=0;
-	int addr[2]={1, CDROM_PORT};
+	addr[2]={1, CDROM_PORT};
 	int port_index;
 
 	sti();
@@ -5670,9 +5676,9 @@ int __init sbpcd_init(void)
 	{
 		addr[1]=sbpcd[port_index];
 		if (addr[1]==0) break;
-		if (check_region(addr[1],4))
+		if (!request_region(addr[1],4, "sbpcd driver"))
 		{
-			msg(DBG_INF,"check_region: %03X is not free.\n",addr[1]);
+			msg(DBG_INF,"request_region: %03X is not free.\n",addr[1]);
 			continue;
 		}
 		if (sbpcd[port_index+1]==2) type=str_sp;
@@ -5699,6 +5705,7 @@ int __init sbpcd_init(void)
 	if (ndrives==0)
 	{
 		msg(DBG_INF, "No drive found.\n");
+		release_region(addr[1],4);
 #ifdef MODULE
 		return -EIO;
 #else
@@ -5775,6 +5782,7 @@ int __init sbpcd_init(void)
 	if (!request_region(CDo_command,4,major_name))
 	{
 		printk(KERN_WARNING "sbpcd: Unable to request region 0x%x\n", CDo_command);
+		release_region(addr[1],4);
 		return -EIO;
 	}
 
@@ -5788,6 +5796,8 @@ int __init sbpcd_init(void)
 #endif /* SOUND_BASE */
 
 	if (register_blkdev(MAJOR_NR, major_name)) {
+		release_region(CDo_command,4);
+		release_region(addr[1],4);
 #ifdef MODULE
 		return -EIO;
 #else
@@ -5801,6 +5811,7 @@ int __init sbpcd_init(void)
 	sbpcd_queue = blk_init_queue(do_sbpcd_request, &sbpcd_lock);
 	if (!sbpcd_queue) {
 		release_region(CDo_command,4);
+		release_region(addr[1],4);
 		unregister_blkdev(MAJOR_NR, major_name);
 		return -ENOMEM;
 	}
@@ -5834,6 +5845,7 @@ int __init sbpcd_init(void)
 				printk("Can't unregister %s\n", major_name);
 			}
 			release_region(CDo_command,4);
+			release_region(addr[1],4);
 			blk_cleanup_queue(sbpcd_queue);
 			return -EIO;
 		}
@@ -5850,6 +5862,7 @@ int __init sbpcd_init(void)
 		if (sbpcd_infop == NULL)
 		{
                         release_region(CDo_command,4);
+			release_region(addr[1],4);
 			blk_cleanup_queue(sbpcd_queue);
                         return -ENOMEM;
 		}
@@ -5894,6 +5907,7 @@ static void sbpcd_exit(void)
 		return;
 	}
 	release_region(CDo_command,4);
+	release_region(addr[1],4);
 	blk_cleanup_queue(sbpcd_queue);
 	for (j=0;j<NR_SBPCD;j++)
 	{

Kindly update if this is Ok. I will proceed with the original patch.

-surya.

Re: [PATCH]: complete cleanup of check_region

[Jesper Juhl]

On 08/06/07, Surya <surya.prabhakar@wipro.com> wrote:
<snip>
> > >                 msg(DBG_INI,"check_drives done.\n");
> > > +               if(request_region_flag==0)
> > > +                       release_region(addr[1],4);
> >
> > I know the driver in its current version just tests if the region is
> > available and doesn't hang on to it, but I'm wondering if that's not a
> > bug.  Shouldn't we actually hold on to this region as long as the
> > driver is loaded and only release the region in the sbpcd_exit()
> > function, just loke we do with the "CDo_command" region?
> But if any of the conditions fail after the request_region code, it is
> returning an error and exiting out of init. Dont we need to cleanup
> request_region's allocation before we exit.

Probably.

> Infact I had a feeling that
> CDo_command region cleanup was not perfect. Did a cleanup of both the
> regions wherever we have a return out of the function.
>
That makes sense to me.

> Please have a look at the below patch.
>
>
> diff --git a/drivers/cdrom/sbpcd.c b/drivers/cdrom/sbpcd.c
> index a1283b1..0a85b1b 100644
> --- a/drivers/cdrom/sbpcd.c
> +++ b/drivers/cdrom/sbpcd.c
> @@ -358,6 +358,10 @@
>   * Add bio/kdev_t changes for 2.5.x required to make it work again.
>   * Still room for improvement in the request handling here if anyone
>   * actually cares.  Bring your own chainsaw.    Paul G.  02/2002
> + *
> + *

Why two blank comment lines?  Isn't one enough?

> + * Cleaned up the reference for deprecated check_region to
> + * request_region.            - Surya Prabhakar N      08/07/2007
>   */
>
>
> @@ -555,6 +559,8 @@ static struct cdrom_read_audio read_audio;
>  static unsigned char msgnum;
>  static char msgbuf[80];
>
> +static int addr[2];
> +
>  static int max_drives = MAX_DRIVES;
>  module_param(max_drives, int, 0);
>  #ifndef MODULE
> @@ -5638,7 +5644,7 @@ int __init sbpcd_init(void)
>  #endif
>  {
>         int i=0, j=0;

Blank line between variable declarations and code please.

> -       int addr[2]={1, CDROM_PORT};
> +       addr[2]={1, CDROM_PORT};
>         int port_index;

Keep the variables together and seperate from the code. Like this :

{
       int i=0, j=0;
       int port_index;

       addr[2]={1, CDROM_PORT};
...


>
>         sti();

Just in case you are bored, at some point those cli()/sti() need to go
away as well and be replaced with proper locking. But that's a
different patch :-)

> @@ -5670,9 +5676,9 @@ int __init sbpcd_init(void)
>         {
>                 addr[1]=sbpcd[port_index];
>                 if (addr[1]==0) break;
> -               if (check_region(addr[1],4))
> +               if (!request_region(addr[1],4, "sbpcd driver"))
>                 {
> -                       msg(DBG_INF,"check_region: %03X is not free.\n",addr[1]);
> +                       msg(DBG_INF,"request_region: %03X is not free.\n",addr[1]);
>                         continue;
>                 }
>                 if (sbpcd[port_index+1]==2) type=str_sp;
> @@ -5699,6 +5705,7 @@ int __init sbpcd_init(void)
>         if (ndrives==0)
>         {
>                 msg(DBG_INF, "No drive found.\n");
> +               release_region(addr[1],4);

If we get here after actually having obtained a region an ndrives is
then 0, then we should ofcourse release the region before we bail out.
makes sense.

>  #ifdef MODULE
>                 return -EIO;
>  #else
> @@ -5775,6 +5782,7 @@ int __init sbpcd_init(void)
>         if (!request_region(CDo_command,4,major_name))
>         {
>                 printk(KERN_WARNING "sbpcd: Unable to request region 0x%x\n", CDo_command);
> +               release_region(addr[1],4);
>                 return -EIO;
>         }
>

This makes sense. If we bail out due to not being able to obtain the
CDo_command region, then we should release the other region.

> @@ -5788,6 +5796,8 @@ int __init sbpcd_init(void)
>  #endif /* SOUND_BASE */
>
>         if (register_blkdev(MAJOR_NR, major_name)) {
> +               release_region(CDo_command,4);
> +               release_region(addr[1],4);
>  #ifdef MODULE
>                 return -EIO;
>  #else
> @@ -5801,6 +5811,7 @@ int __init sbpcd_init(void)
>         sbpcd_queue = blk_init_queue(do_sbpcd_request, &sbpcd_lock);
>         if (!sbpcd_queue) {
>                 release_region(CDo_command,4);
> +               release_region(addr[1],4);
>                 unregister_blkdev(MAJOR_NR, major_name);
>                 return -ENOMEM;
>         }
> @@ -5834,6 +5845,7 @@ int __init sbpcd_init(void)
>                                 printk("Can't unregister %s\n", major_name);
>                         }
>                         release_region(CDo_command,4);
> +                       release_region(addr[1],4);
>                         blk_cleanup_queue(sbpcd_queue);
>                         return -EIO;
>                 }
> @@ -5850,6 +5862,7 @@ int __init sbpcd_init(void)
>                 if (sbpcd_infop == NULL)
>                 {
>                          release_region(CDo_command,4);
> +                       release_region(addr[1],4);
>                         blk_cleanup_queue(sbpcd_queue);
>                          return -ENOMEM;
>                 }
> @@ -5894,6 +5907,7 @@ static void sbpcd_exit(void)
>                 return;
>         }
>         release_region(CDo_command,4);
> +       release_region(addr[1],4);
>         blk_cleanup_queue(sbpcd_queue);
>         for (j=0;j<NR_SBPCD;j++)
>         {
>
> Kindly update if this is Ok. I will proceed with the original patch.
>

This looks a lot better to me. I have only given it a quick look, but
I don't see any major problems.

> -surya.
>
>  The information contained in this electronic message and any attachments to this message are intended for the exclusive use of the addressee(s) and may contain proprietary, confidential or privileged information. If you are not the intended recipient, you should not disseminate, distribute or copy this e-mail. Please notify the sender immediately and destroy all copies of this message and any attachments.   WARNING: Computer viruses can be transmitted via email. The recipient should check this email and any attachments for the presence of viruses. The company accepts no liability for any damage caused by any virus transmitted by this email.   www.wipro.com
>
Please tell your mail client not to include crap like that when
sending to public mailing lists.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

[PATCH]: check_region cleanup in sbpcd.c

[Surya]

> > + *
> > + *
> 
> Why two blank comment lines?  Isn't one enough?
cleaned it up.
> 
>  sti();
> Just in case you are bored, at some point those cli()/sti() need to go
> away as well and be replaced with proper locking. But that's a
> different patch :-)
sure will take it up in the next patch.
> 
> > Kindly update if this is Ok. I will proceed with the original patch.
> This looks a lot better to me. I have only given it a quick look, but
> I don't see any major problems.
> 
I am sending with all the corrections, if its ok to acknowledge it?

> Please tell your mail client not to include crap like that when
> sending to public mailing lists.
Really sorry, I have no control over this.

Attaching the new patch with all the corrections.. thanks.


Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>
--- 

diff --git a/drivers/cdrom/sbpcd.c b/drivers/cdrom/sbpcd.c
index a1283b1..5414172 100644
--- a/drivers/cdrom/sbpcd.c
+++ b/drivers/cdrom/sbpcd.c
@@ -358,6 +358,9 @@
  * Add bio/kdev_t changes for 2.5.x required to make it work again. 
  * Still room for improvement in the request handling here if anyone
  * actually cares.  Bring your own chainsaw.    Paul G.  02/2002
+ *
+ * Deprecated check_region cleanup to request_region 
+ *				-Surya Prabhakar N    08/07/2007
  */
 
 
@@ -555,6 +558,7 @@ static struct cdrom_read_audio read_audio;
 static unsigned char msgnum;
 static char msgbuf[80];
 
+static int addr[2] = {1, CDROM_PORT};
 static int max_drives = MAX_DRIVES;
 module_param(max_drives, int, 0);
 #ifndef MODULE
@@ -5638,7 +5642,6 @@ int __init sbpcd_init(void)
 #endif
 {
 	int i=0, j=0;
-	int addr[2]={1, CDROM_PORT};
 	int port_index;
 
 	sti();
@@ -5670,9 +5673,9 @@ int __init sbpcd_init(void)
 	{
 		addr[1]=sbpcd[port_index];
 		if (addr[1]==0) break;
-		if (check_region(addr[1],4))
+		if (!request_region(addr[1],4, "sbpcd driver"))
 		{
-			msg(DBG_INF,"check_region: %03X is not free.\n",addr[1]);
+			msg(DBG_INF,"request_region: %03X is not free.\n",addr[1]);
 			continue;
 		}
 		if (sbpcd[port_index+1]==2) type=str_sp;
@@ -5699,6 +5702,7 @@ int __init sbpcd_init(void)
 	if (ndrives==0)
 	{
 		msg(DBG_INF, "No drive found.\n");
+		release_region(addr[1],4);
 #ifdef MODULE
 		return -EIO;
 #else
@@ -5775,6 +5779,7 @@ int __init sbpcd_init(void)
 	if (!request_region(CDo_command,4,major_name))
 	{
 		printk(KERN_WARNING "sbpcd: Unable to request region 0x%x\n", CDo_command);
+		release_region(addr[1],4);
 		return -EIO;
 	}
 
@@ -5788,6 +5793,8 @@ int __init sbpcd_init(void)
 #endif /* SOUND_BASE */
 
 	if (register_blkdev(MAJOR_NR, major_name)) {
+		release_region(CDo_command,4);
+		release_region(addr[1],4);
 #ifdef MODULE
 		return -EIO;
 #else
@@ -5801,6 +5808,7 @@ int __init sbpcd_init(void)
 	sbpcd_queue = blk_init_queue(do_sbpcd_request, &sbpcd_lock);
 	if (!sbpcd_queue) {
 		release_region(CDo_command,4);
+		release_region(addr[1],4);
 		unregister_blkdev(MAJOR_NR, major_name);
 		return -ENOMEM;
 	}
@@ -5834,6 +5842,7 @@ int __init sbpcd_init(void)
 				printk("Can't unregister %s\n", major_name);
 			}
 			release_region(CDo_command,4);
+			release_region(addr[1],4);
 			blk_cleanup_queue(sbpcd_queue);
 			return -EIO;
 		}
@@ -5850,6 +5859,7 @@ int __init sbpcd_init(void)
 		if (sbpcd_infop == NULL)
 		{
                         release_region(CDo_command,4);
+			release_region(addr[1],4);
 			blk_cleanup_queue(sbpcd_queue);
                         return -ENOMEM;
 		}
@@ -5894,6 +5904,7 @@ static void sbpcd_exit(void)
 		return;
 	}
 	release_region(CDo_command,4);
+	release_region(addr[1],4);
 	blk_cleanup_queue(sbpcd_queue);
 	for (j=0;j<NR_SBPCD;j++)
 	{

-surya.

Re: [PATCH]: check_region cleanup in sbpcd.c

[Jesper Juhl]

On 12/06/07, Surya <surya.prabhakar@wipro.com> wrote:
[snip]
> >
> I am sending with all the corrections, if its ok to acknowledge it?
>
Looks good to me.

>
> Signed-off-by: Surya Prabhakar <surya.prabhakar@wipro.com>
Reviewed-by: Jesper Juhl <jesper.juhl@gmail.com>

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html