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 <[email protected]>
---
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
--
2.20.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 <[email protected]>
> ---
> 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.
There's no need to iterate on current_fdc in do_floppy_init() anymore,
in the first case it's only used as an array index to access fdc_state[],
so let's get rid of this confusing assignment. The second case is a bit
trickier because user_reset_fdc() needs to already know current_fdc when
called with drive==-1 due to this call chain:
user_reset_fdc()
lock_fdc()
set_fdc()
drive<0 ==> new_fdc = current_fdc
Note that current_drive is not used in this code part and may even not
match a unit belonging to current_fdc. Instead of passing -1 we can
simply pass the first drive of the FDC being initialized, which is even
cleaner as it will allow the function chain above to consistently assign
both variables.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 38 ++++++++++++++++++--------------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 07218f8b17f9..8da7921659f1 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4657,16 +4657,15 @@ static int __init do_floppy_init(void)
config_types();
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;
+ memset(&fdc_state[i], 0, sizeof(*fdc_state));
+ fdc_state[i].dtr = -1;
+ fdc_state[i].dor = 0x4;
#if defined(__sparc__) || defined(__mc68000__)
/*sparcs/sun3x don't have a DOR reset which we can fall back on to */
#ifdef __mc68000__
if (MACH_IS_SUN3X)
#endif
- fdc_state[current_fdc].version = FDC_82072A;
+ fdc_state[i].version = FDC_82072A;
#endif
}
@@ -4708,30 +4707,29 @@ static int __init do_floppy_init(void)
msleep(10);
for (i = 0; i < N_FDC; i++) {
- current_fdc = i;
- fdc_state[current_fdc].driver_version = FD_DRIVER_VERSION;
+ fdc_state[i].driver_version = FD_DRIVER_VERSION;
for (unit = 0; unit < 4; unit++)
- fdc_state[current_fdc].track[unit] = 0;
- if (fdc_state[current_fdc].address == -1)
+ fdc_state[i].track[unit] = 0;
+ if (fdc_state[i].address == -1)
continue;
- fdc_state[current_fdc].rawcmd = 2;
- if (user_reset_fdc(-1, FD_RESET_ALWAYS, false)) {
+ fdc_state[i].rawcmd = 2;
+ if (user_reset_fdc(REVDRIVE(i, 0), FD_RESET_ALWAYS, false)) {
/* free ioports reserved by floppy_grab_irq_and_dma() */
- floppy_release_regions(current_fdc);
- fdc_state[current_fdc].address = -1;
- fdc_state[current_fdc].version = FDC_NONE;
+ floppy_release_regions(i);
+ fdc_state[i].address = -1;
+ fdc_state[i].version = FDC_NONE;
continue;
}
/* Try to determine the floppy controller type */
- fdc_state[current_fdc].version = get_fdc_version(current_fdc);
- if (fdc_state[current_fdc].version == FDC_NONE) {
+ fdc_state[i].version = get_fdc_version(i);
+ if (fdc_state[i].version == FDC_NONE) {
/* free ioports reserved by floppy_grab_irq_and_dma() */
- floppy_release_regions(current_fdc);
- fdc_state[current_fdc].address = -1;
+ floppy_release_regions(i);
+ fdc_state[i].address = -1;
continue;
}
if (can_use_virtual_dma == 2 &&
- fdc_state[current_fdc].version < FDC_82072A)
+ fdc_state[i].version < FDC_82072A)
can_use_virtual_dma = 0;
have_no_fdc = 0;
@@ -4739,7 +4737,7 @@ static int __init do_floppy_init(void)
* properly, so force a reset for the standard FDC clones,
* to avoid interrupt garbage.
*/
- user_reset_fdc(-1, FD_RESET_ALWAYS, false);
+ user_reset_fdc(REVDRIVE(i, 0), FD_RESET_ALWAYS, false);
}
current_fdc = 0;
cancel_delayed_work(&fd_timeout);
--
2.20.1
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
In floppy_resume() we don't properly reinitialize all FDCs, instead
we reinitialize the current FDC once per available FDC because value
-1 is passed to user_reset_fdc(). Let's simply save the current drive
and properly reinitialize each FDC.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 8da7921659f1..b102f55dfa5d 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4545,11 +4545,13 @@ static void floppy_device_release(struct device *dev)
static int floppy_resume(struct device *dev)
{
int fdc;
+ int saved_drive;
+ saved_drive = current_drive;
for (fdc = 0; fdc < N_FDC; fdc++)
if (fdc_state[fdc].address != -1)
- user_reset_fdc(-1, FD_RESET_ALWAYS, false);
-
+ user_reset_fdc(REVDRIVE(fdc, 0), FD_RESET_ALWAYS, false);
+ set_fdc(saved_drive);
return 0;
}
--
2.20.1
When called with a negative drive value, set_fdc() would stick to the
current fdc (which was assumed to reflect the current_drive's FDC). We
do not need this anymore as the last call place with a negative value
was just addressed. Let's make this function always set both current_fdc
and current_drive so that there's no more ambiguity. A few comments
stating this were added to a few non-obvious places.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 43 ++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 20646d4c5437..2817170dd403 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -851,31 +851,42 @@ static void reset_fdc_info(int fdc, int mode)
drive_state[drive].track = NEED_2_RECAL;
}
-/* selects the fdc and drive, and enables the fdc's input/dma. */
+/*
+ * selects the fdc and drive, and enables the fdc's input/dma.
+ * Both current_drive and current_fdc are changed to match the new drive.
+ */
static void set_fdc(int drive)
{
- unsigned int new_fdc = current_fdc;
+ unsigned int fdc;
- if (drive >= 0 && drive < N_DRIVE) {
- new_fdc = FDC(drive);
- current_drive = drive;
+ if (drive < 0 || drive >= N_DRIVE) {
+ pr_info("bad drive value %d\n", drive);
+ return;
}
- if (new_fdc >= N_FDC) {
+
+ fdc = FDC(drive);
+ if (fdc >= N_FDC) {
pr_info("bad fdc value\n");
return;
}
- current_fdc = new_fdc;
- set_dor(current_fdc, ~0, 8);
+
+ set_dor(fdc, ~0, 8);
#if N_FDC > 1
- set_dor(1 - current_fdc, ~8, 0);
+ set_dor(1 - fdc, ~8, 0);
#endif
- if (fdc_state[current_fdc].rawcmd == 2)
- reset_fdc_info(current_fdc, 1);
- if (fdc_inb(current_fdc, FD_STATUS) != STATUS_READY)
- fdc_state[current_fdc].reset = 1;
+ if (fdc_state[fdc].rawcmd == 2)
+ reset_fdc_info(fdc, 1);
+ if (fdc_inb(fdc, FD_STATUS) != STATUS_READY)
+ fdc_state[fdc].reset = 1;
+
+ current_drive = drive;
+ current_fdc = fdc;
}
-/* locks the driver */
+/*
+ * locks the driver.
+ * Both current_drive and current_fdc are changed to match the new drive.
+ */
static int lock_fdc(int drive)
{
if (WARN(atomic_read(&usage_count) == 0,
@@ -3000,6 +3011,10 @@ static const struct cont_t reset_cont = {
.done = generic_done
};
+/*
+ * Resets the FDC connected to drive <drive>.
+ * Both current_drive and current_fdc are changed to match the new drive.
+ */
static int user_reset_fdc(int drive, int arg, bool interruptible)
{
int ret;
--
2.20.1
This macro equals -1 and is used as an alternative for current_drive when
calling reschedule_timeout(), which in turn needs to remap it. This only
adds obfuscation, let's simply use current_drive.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index b102f55dfa5d..20646d4c5437 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -668,16 +668,12 @@ static struct output_log {
static int output_log_pos;
-#define current_reqD -1
#define MAXTIMEOUT -2
static void __reschedule_timeout(int drive, const char *message)
{
unsigned long delay;
- if (drive == current_reqD)
- drive = current_drive;
-
if (drive < 0 || drive >= N_DRIVE) {
delay = 20UL * HZ;
drive = 0;
@@ -1960,7 +1956,7 @@ static void floppy_ready(void)
static void floppy_start(void)
{
- reschedule_timeout(current_reqD, "floppy start");
+ reschedule_timeout(current_drive, "floppy start");
scandrives();
debug_dcl(drive_params[current_drive].flags,
@@ -2874,7 +2870,7 @@ static void redo_fd_request(void)
}
drive = (long)current_req->rq_disk->private_data;
set_fdc(drive);
- reschedule_timeout(current_reqD, "redo fd request");
+ reschedule_timeout(current_drive, "redo fd request");
set_floppy(drive);
raw_cmd = &default_raw_cmd;
--
2.20.1