[22/23] floppy: cleanup: do not iterate on current_fdc in DMA grab/release functions
diff mbox series

Message ID 20200331094054.24441-23-w@1wt.eu
State New
Headers show
Series
  • Floppy driver cleanups
Related show

Commit Message

Willy Tarreau March 31, 2020, 9:40 a.m. UTC
Both floppy_grab_irq_and_dma() and floppy_release_irq_and_dma() used to
iterate on the global variable while setting up or freeing resources.
Now that they exclusively rely on functions which take the fdc as an
argument, so let's not touch the global one anymore.

Signed-off-by: Willy Tarreau <w@1wt.eu>
---
 drivers/block/floppy.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

Comments

Denis Efremov April 10, 2020, 8:35 a.m. UTC | #1
I see a couple of similar cycles in do_floppy_init:

for (i = 0; i < N_FDC; i++) {
        current_fdc = i;
        memset(&fdc_state[current_fdc], 0, sizeof(*fdc_state));
        fdc_state[current_fdc].dtr = -1;
        fdc_state[current_fdc].dor = 0x4;
...
}

for (i = 0; i < N_FDC; i++) {
        current_fdc = i;
        fdc_state[current_fdc].driver_version = FD_DRIVER_VERSION;
...
}

On 3/31/20 12:40 PM, Willy Tarreau wrote:
> Both floppy_grab_irq_and_dma() and floppy_release_irq_and_dma() used to
> iterate on the global variable while setting up or freeing resources.
> Now that they exclusively rely on functions which take the fdc as an
> argument, so let's not touch the global one anymore.
> 
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> ---
>  drivers/block/floppy.c | 39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 8850baa3372a..77bb9a5fcd33 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4854,6 +4854,8 @@ static void floppy_release_regions(int fdc)
>  
>  static int floppy_grab_irq_and_dma(void)
>  {
> +	int fdc;
> +
>  	if (atomic_inc_return(&usage_count) > 1)
>  		return 0;
>  
> @@ -4881,24 +4883,24 @@ static int floppy_grab_irq_and_dma(void)
>  		}
>  	}
>  
> -	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++) {
> -		if (fdc_state[current_fdc].address != -1) {
> -			if (floppy_request_regions(current_fdc))
> +	for (fdc = 0; fdc < N_FDC; fdc++) {
> +		if (fdc_state[fdc].address != -1) {
> +			if (floppy_request_regions(fdc))
>  				goto cleanup;
>  		}
>  	}
> -	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++) {
> -		if (fdc_state[current_fdc].address != -1) {
> -			reset_fdc_info(current_fdc, 1);
> -			fdc_outb(fdc_state[current_fdc].dor, current_fdc, FD_DOR);
> +	for (fdc = 0; fdc < N_FDC; fdc++) {
> +		if (fdc_state[fdc].address != -1) {
> +			reset_fdc_info(fdc, 1);
> +			fdc_outb(fdc_state[fdc].dor, fdc, FD_DOR);
>  		}
>  	}
> -	current_fdc = 0;
> +
>  	set_dor(0, ~0, 8);	/* avoid immediate interrupt */
>  
> -	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++)
> -		if (fdc_state[current_fdc].address != -1)
> -			fdc_outb(fdc_state[current_fdc].dor, current_fdc, FD_DOR);
> +	for (fdc = 0; fdc < N_FDC; fdc++)
> +		if (fdc_state[fdc].address != -1)
> +			fdc_outb(fdc_state[fdc].dor, fdc, FD_DOR);
>  	/*
>  	 * The driver will try and free resources and relies on us
>  	 * to know if they were allocated or not.
> @@ -4909,15 +4911,16 @@ static int floppy_grab_irq_and_dma(void)
>  cleanup:
>  	fd_free_irq();
>  	fd_free_dma();
> -	while (--current_fdc >= 0)
> -		floppy_release_regions(current_fdc);
> +	while (--fdc >= 0)
> +		floppy_release_regions(fdc);
> +	current_fdc = 0;
>  	atomic_dec(&usage_count);
>  	return -1;
>  }
>  
>  static void floppy_release_irq_and_dma(void)
>  {
> -	int old_fdc;
> +	int fdc;
>  #ifndef __sparc__
>  	int drive;
>  #endif
> @@ -4958,11 +4961,9 @@ static void floppy_release_irq_and_dma(void)
>  		pr_info("auxiliary floppy timer still active\n");
>  	if (work_pending(&floppy_work))
>  		pr_info("work still pending\n");
> -	old_fdc = current_fdc;
> -	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++)
> -		if (fdc_state[current_fdc].address != -1)
> -			floppy_release_regions(current_fdc);
> -	current_fdc = old_fdc;
> +	for (fdc = 0; fdc < N_FDC; fdc++)
> +		if (fdc_state[fdc].address != -1)
> +			floppy_release_regions(fdc);
>  }
>  
>  #ifdef MODULE
>
Willy Tarreau April 10, 2020, 8:45 a.m. UTC | #2
On Fri, Apr 10, 2020 at 11:35:51AM +0300, Denis Efremov wrote:
> I see a couple of similar cycles in do_floppy_init:
> 
> for (i = 0; i < N_FDC; i++) {
>         current_fdc = i;
>         memset(&fdc_state[current_fdc], 0, sizeof(*fdc_state));
>         fdc_state[current_fdc].dtr = -1;
>         fdc_state[current_fdc].dor = 0x4;
> ...
> }
> 
> for (i = 0; i < N_FDC; i++) {
>         current_fdc = i;
>         fdc_state[current_fdc].driver_version = FD_DRIVER_VERSION;
> ...
> }

Ah thanks, I missed these ones! Do you want me to respin this patch ?

Willy
Denis Efremov April 10, 2020, 8:48 a.m. UTC | #3
On 4/10/20 11:45 AM, Willy Tarreau wrote:
> Ah thanks, I missed these ones! Do you want me to respin this patch ?

I think you can resend only this patch, or send an additional one 24/23.
Willy Tarreau April 10, 2020, 9:32 a.m. UTC | #4
On Fri, Apr 10, 2020 at 11:48:21AM +0300, Denis Efremov wrote:
> On 4/10/20 11:45 AM, Willy Tarreau wrote:
> > Ah thanks, I missed these ones! Do you want me to respin this patch ?
> 
> I think you can resend only this patch, or send an additional one 24/23.

OK, now done as 24/23. Apparently we can now get rid of the code dealing
with drive==-1 in lock_fdc()/set_fdc() etc. It's only used by
user_reset_fdc() and the last call place is in floppy_resume() which
even forgets to set the fdc number, so it only resets the current FDC,
as many times as there are FDCs in the system. I'm going to fix this
one and see make sure we drop this special case of -1.

Willy

Patch
diff mbox series

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 8850baa3372a..77bb9a5fcd33 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4854,6 +4854,8 @@  static void floppy_release_regions(int fdc)
 
 static int floppy_grab_irq_and_dma(void)
 {
+	int fdc;
+
 	if (atomic_inc_return(&usage_count) > 1)
 		return 0;
 
@@ -4881,24 +4883,24 @@  static int floppy_grab_irq_and_dma(void)
 		}
 	}
 
-	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++) {
-		if (fdc_state[current_fdc].address != -1) {
-			if (floppy_request_regions(current_fdc))
+	for (fdc = 0; fdc < N_FDC; fdc++) {
+		if (fdc_state[fdc].address != -1) {
+			if (floppy_request_regions(fdc))
 				goto cleanup;
 		}
 	}
-	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++) {
-		if (fdc_state[current_fdc].address != -1) {
-			reset_fdc_info(current_fdc, 1);
-			fdc_outb(fdc_state[current_fdc].dor, current_fdc, FD_DOR);
+	for (fdc = 0; fdc < N_FDC; fdc++) {
+		if (fdc_state[fdc].address != -1) {
+			reset_fdc_info(fdc, 1);
+			fdc_outb(fdc_state[fdc].dor, fdc, FD_DOR);
 		}
 	}
-	current_fdc = 0;
+
 	set_dor(0, ~0, 8);	/* avoid immediate interrupt */
 
-	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++)
-		if (fdc_state[current_fdc].address != -1)
-			fdc_outb(fdc_state[current_fdc].dor, current_fdc, FD_DOR);
+	for (fdc = 0; fdc < N_FDC; fdc++)
+		if (fdc_state[fdc].address != -1)
+			fdc_outb(fdc_state[fdc].dor, fdc, FD_DOR);
 	/*
 	 * The driver will try and free resources and relies on us
 	 * to know if they were allocated or not.
@@ -4909,15 +4911,16 @@  static int floppy_grab_irq_and_dma(void)
 cleanup:
 	fd_free_irq();
 	fd_free_dma();
-	while (--current_fdc >= 0)
-		floppy_release_regions(current_fdc);
+	while (--fdc >= 0)
+		floppy_release_regions(fdc);
+	current_fdc = 0;
 	atomic_dec(&usage_count);
 	return -1;
 }
 
 static void floppy_release_irq_and_dma(void)
 {
-	int old_fdc;
+	int fdc;
 #ifndef __sparc__
 	int drive;
 #endif
@@ -4958,11 +4961,9 @@  static void floppy_release_irq_and_dma(void)
 		pr_info("auxiliary floppy timer still active\n");
 	if (work_pending(&floppy_work))
 		pr_info("work still pending\n");
-	old_fdc = current_fdc;
-	for (current_fdc = 0; current_fdc < N_FDC; current_fdc++)
-		if (fdc_state[current_fdc].address != -1)
-			floppy_release_regions(current_fdc);
-	current_fdc = old_fdc;
+	for (fdc = 0; fdc < N_FDC; fdc++)
+		if (fdc_state[fdc].address != -1)
+			floppy_release_regions(fdc);
 }
 
 #ifdef MODULE