| Message ID | 20200331094054.24441-23-w@1wt.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Floppy driver cleanups | expand |
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
>
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
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.
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
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
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(-)