Re: [PATCH] floppy: release only the ports we actually requested

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

On Monday 09 February 2009 02:15:27 am Philippe De Muyter wrote:
> Bjorn,
> 
> On Fri, Feb 06, 2009 at 10:57:06AM -0700, Bjorn Helgaas wrote:
> > On Friday 06 February 2009 09:20:13 am Bjorn Helgaas wrote:
> > > On Friday 06 February 2009 01:55:01 am Philippe De Muyter wrote:
> > > > Bjorn, Andrew,
> > > > 
> > > > --
> > > > 
> > > > With the last floppy patch, the floppy driver requests only the ports that
> > > > it really uses, but the code contains yet places where it releases those
> > > > unrequested ports.  I don't know if it is harmfull, but I think it is cleaner
> > > > that the parameters of the release_region calls match the request_region ones.
> > > 
> > > Doh!  Boy, do I feel stupid.  I doubt this is in any git trees yet,
> > > so I'll combine the two patches and send a cleaner one.
> > 
> > Philippe, can you give this a once-over?  I can compile it, but I don't
> > have a machine with a floppy drive handy to test it.  Since you pointed
> > out all the places I forgot to change, I went ahead and factored out
> > the request/release stuff so they're all in one place now.
> 
> As expected, your code works (tested linked in kernel and as a module),
> but I took your idea a little bit further, avoiding to keep 3 places with
> the io regions list, avoiding gotos and producing a slightly smaller binary.
> Of course, it is also compiled and tested both linked in kernel and in module
> mode.
> 
> Is it OK for you ?

Looks great to me.  Feel free to add a suitable changelog and my
Reviewed-by.  Thanks for all your work on this!

Bjorn

> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -558,6 +558,8 @@ static void recalibrate_floppy(void);
>  static void recalibrate_floppy(void);
>  static void floppy_shutdown(unsigned long);
>  
> +static int floppy_request_regions(int);
> +static void floppy_release_regions(int);
>  static int floppy_grab_irq_and_dma(void);
>  static void floppy_release_irq_and_dma(void);
>  
> @@ -4274,8 +4276,7 @@ static int __init floppy_init(void)
>  		FDCS->rawcmd = 2;
>  		if (user_reset_fdc(-1, FD_RESET_ALWAYS, 0)) {
>  			/* free ioports reserved by floppy_grab_irq_and_dma() */
> -			release_region(FDCS->address + 2, 4);
> -			release_region(FDCS->address + 7, 1);
> +			floppy_release_regions(fdc);
>  			FDCS->address = -1;
>  			FDCS->version = FDC_NONE;
>  			continue;
> @@ -4284,8 +4285,7 @@ static int __init floppy_init(void)
>  		FDCS->version = get_fdc_version();
>  		if (FDCS->version == FDC_NONE) {
>  			/* free ioports reserved by floppy_grab_irq_and_dma() */
> -			release_region(FDCS->address + 2, 4);
> -			release_region(FDCS->address + 7, 1);
> +			floppy_release_regions(fdc);
>  			FDCS->address = -1;
>  			continue;
>  		}
> @@ -4358,6 +4358,47 @@ out_put_disk:
>  
>  static DEFINE_SPINLOCK(floppy_usage_lock);
>  
> +static struct io_region {
> +	int offset;
> +	int size;
> +} io_regions[] = {
> +	{ 2, 1 },
> +	/* address + 3 is sometimes reserved by pnp bios for motherboard */
> +	{ 4, 2 },
> +	/* address + 6 is reserved, and may be taken by IDE.
> +	 * Unfortunately, Adaptec doesn't know this :-(, */
> +	{ 7, 1 },
> +};
> +
> +static void floppy_release_allocated_regions(int fdc, struct io_region *p)
> +{
> +	while (p != io_regions) {
> +		p--;
> +		release_region(FDCS->address + p->offset, p->size);
> +	}
> +}
> +	
> +#define ARRAY_END(X) (&((X)[ARRAY_SIZE(X)]))
> +
> +static int floppy_request_regions(int fdc)
> +{
> +	struct io_region *p;
> +
> +	for (p = io_regions; p < ARRAY_END(io_regions); p++) {
> +		if (!request_region(FDCS->address + p->offset, p->size, "floppy")) {
> +			DPRINT("Floppy io-port 0x%04lx in use\n", FDCS->address + p->offset);
> +			floppy_release_allocated_regions(fdc, p);
> +			return -EBUSY;
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void floppy_release_regions(int fdc)
> +{
> +	floppy_release_allocated_regions(fdc, ARRAY_END(io_regions));
> +}
> +
>  static int floppy_grab_irq_and_dma(void)
>  {
>  	unsigned long flags;
> @@ -4399,18 +4440,8 @@ static int floppy_grab_irq_and_dma(void)
>  
>  	for (fdc = 0; fdc < N_FDC; fdc++) {
>  		if (FDCS->address != -1) {
> -			if (!request_region(FDCS->address + 2, 4, "floppy")) {
> -				DPRINT("Floppy io-port 0x%04lx in use\n",
> -				       FDCS->address + 2);
> -				goto cleanup1;
> -			}
> -			if (!request_region(FDCS->address + 7, 1, "floppy DIR")) {
> -				DPRINT("Floppy io-port 0x%04lx in use\n",
> -				       FDCS->address + 7);
> -				goto cleanup2;
> -			}
> -			/* address + 6 is reserved, and may be taken by IDE.
> -			 * Unfortunately, Adaptec doesn't know this :-(, */
> +			if (floppy_request_regions(fdc))
> +				goto cleanup;
>  		}
>  	}
>  	for (fdc = 0; fdc < N_FDC; fdc++) {
> @@ -4432,15 +4463,11 @@ static int floppy_grab_irq_and_dma(void)
>  	fdc = 0;
>  	irqdma_allocated = 1;
>  	return 0;
> -cleanup2:
> -	release_region(FDCS->address + 2, 4);
> -cleanup1:
> +cleanup:
>  	fd_free_irq();
>  	fd_free_dma();
> -	while (--fdc >= 0) {
> -		release_region(FDCS->address + 2, 4);
> -		release_region(FDCS->address + 7, 1);
> -	}
> +	while (--fdc >= 0)
> +		floppy_release_regions(fdc);
>  	spin_lock_irqsave(&floppy_usage_lock, flags);
>  	usage_count--;
>  	spin_unlock_irqrestore(&floppy_usage_lock, flags);
> @@ -4502,8 +4529,7 @@ static void floppy_release_irq_and_dma(v
>  	old_fdc = fdc;
>  	for (fdc = 0; fdc < N_FDC; fdc++)
>  		if (FDCS->address != -1) {
> -			release_region(FDCS->address + 2, 4);
> -			release_region(FDCS->address + 7, 1);
> +			floppy_release_regions(fdc);
>  		}
>  	fdc = old_fdc;
>  }
>