| Message ID | 20200224212352.8640-2-w@1wt.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | floppy driver cleanups (deobfuscation) | expand |
On Mon, Feb 24, 2020 at 1:24 PM Willy Tarreau <w@1wt.eu> wrote: > > Macro FDCS silently uses identifier "fdc" which may be either the > global one or a local one. Let's expand the macro to make this more > obvious. Hmm. These macro expansions feel wrong to me. Or rather, they look right as a first step - and it's probably worth doing this just to have that "exact same code generation" step. But I think there should be a second step (also with "exact same code generation") which then renames the driver-global "fdc" index as "current_fdc". That way you'll _really_ see when you use the global vs local ones. The local ones would continue to be just "fdc". Because with just this patch, I don't think you actually get any more obvious whether it's the global or local "fdc" index that is used. So I'd like to see that second step that does the -static int fdc; /* current fdc */ +static int current_fdc; change. We already call the global 'drive' variable 'current_drive', so it really is 'fdc' that is misnamed and ambiguous because it then has two different cases: the global 'fdc' and then the various shadowing local 'fdc' variables (or function arguments). Mind adding that too? Slightly less automatic, I agree, because then you really do have to disambiguate between the "is this the shadowed use of a local 'fdc'" case or the "this is the global 'fdc' use" case. Can coccinelle do that? Linus
Hi, On 2/25/20 12:53 AM, Linus Torvalds wrote: > So I'd like to see that second step that does the > > -static int fdc; /* current fdc */ > +static int current_fdc; > > change. > > We already call the global 'drive' variable 'current_drive', so it > really is 'fdc' that is misnamed and ambiguous because it then has two > different cases: the global 'fdc' and then the various shadowing local > 'fdc' variables (or function arguments). > > Mind adding that too? Slightly less automatic, I agree, because then > you really do have to disambiguate between the "is this the shadowed > use of a local 'fdc'" case or the "this is the global 'fdc' use" case. > > Can coccinelle do that? Willy, if you don't want to spend your time with this code anymore I can prepare patсhes for the second step. I know coccinelle and could try to automate this transformation. At first sight your patches look good to me. I will answer to the top email after more accurate review. Thanks, Denis
On Tue, Feb 25, 2020 at 02:13:42AM +0300, Denis Efremov wrote: > On 2/25/20 12:53 AM, Linus Torvalds wrote: > > So I'd like to see that second step that does the > > > > -static int fdc; /* current fdc */ > > +static int current_fdc; > > > > change. > > > > We already call the global 'drive' variable 'current_drive', so it > > really is 'fdc' that is misnamed and ambiguous because it then has two > > different cases: the global 'fdc' and then the various shadowing local > > 'fdc' variables (or function arguments). > > > > Mind adding that too? Slightly less automatic, I agree, because then > > you really do have to disambiguate between the "is this the shadowed > > use of a local 'fdc'" case or the "this is the global 'fdc' use" case. I definitely agree. I first wanted to be sure the patches were acceptable as a principle, but disambiguating the variables is easy to do now. > > Can coccinelle do that? I could do it by hand, I did quite a bit of manual changes and checks already and the driver is not that long. > Willy, if you don't want to spend your time with this code anymore I can > prepare pat?hes for the second step. I know coccinelle and could try > to automate this transformation. At first sight your patches look > good to me. I will answer to the top email after more accurate review. OK, it's as you like. If you think you can do the change quickly, feel free to do so, otherwise it should not take me more than one hour. In any case as previously mentioned I still have the hardware in a usable state if you want me to recheck anything. Cheers, Willy
On 2/25/20 6:45 AM, Willy Tarreau wrote: > On Tue, Feb 25, 2020 at 02:13:42AM +0300, Denis Efremov wrote: >> On 2/25/20 12:53 AM, Linus Torvalds wrote: >>> So I'd like to see that second step that does the >>> >>> -static int fdc; /* current fdc */ >>> +static int current_fdc; >>> >>> change. >>> >>> We already call the global 'drive' variable 'current_drive', so it >>> really is 'fdc' that is misnamed and ambiguous because it then has two >>> different cases: the global 'fdc' and then the various shadowing local >>> 'fdc' variables (or function arguments). >>> >>> Mind adding that too? Slightly less automatic, I agree, because then >>> you really do have to disambiguate between the "is this the shadowed >>> use of a local 'fdc'" case or the "this is the global 'fdc' use" case. > > I definitely agree. I first wanted to be sure the patches were acceptable > as a principle, but disambiguating the variables is easy to do now. Ok, I don't want to break in the middle of your changes in this case. > >>> Can coccinelle do that? > > I could do it by hand, I did quite a bit of manual changes and checks > already and the driver is not that long. > >> Willy, if you don't want to spend your time with this code anymore I can >> prepare pat?hes for the second step. I know coccinelle and could try >> to automate this transformation. At first sight your patches look >> good to me. I will answer to the top email after more accurate review. > > OK, it's as you like. If you think you can do the change quickly, feel > free to do so, otherwise it should not take me more than one hour. In > any case as previously mentioned I still have the hardware in a usable > state if you want me to recheck anything. > I also have working hardware to test your changes with the previous patch. However, double check is always welcome if you've got time for that. Please, send patches on top of these ones. Thanks, Denis
On 2/25/20 12:23 AM, Willy Tarreau wrote: > Macro FDCS silently uses identifier "fdc" which may be either the > global one or a local one. Let's expand the macro to make this more > obvious. This patch looks good to me. Just want to leave a note here that FD_IOPORT macro from include/uapi/linux/fdreg.h also silently uses fdc global var: #define FD_IOPORT fdc_state[fdc].address Thanks, Denis > > Signed-off-by: Willy Tarreau <w@1wt.eu> > --- > drivers/block/floppy.c | 183 ++++++++++++++++++++++++------------------------- > 1 file changed, 91 insertions(+), 92 deletions(-) > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > index 8ef65c0..93e0840 100644 > --- a/drivers/block/floppy.c > +++ b/drivers/block/floppy.c > @@ -309,7 +309,6 @@ static bool initialized; > #define DP (&drive_params[current_drive]) > #define DRS (&drive_state[current_drive]) > #define DRWE (&write_errors[current_drive]) > -#define FDCS (&fdc_state[fdc]) > > #define UDP (&drive_params[drive]) > #define UDRS (&drive_state[drive]) > @@ -742,11 +741,11 @@ static int disk_change(int drive) > > if (time_before(jiffies, UDRS->select_date + UDP->select_delay)) > DPRINT("WARNING disk change called early\n"); > - if (!(FDCS->dor & (0x10 << UNIT(drive))) || > - (FDCS->dor & 3) != UNIT(drive) || fdc != FDC(drive)) { > + if (!(fdc_state[fdc].dor & (0x10 << UNIT(drive))) || > + (fdc_state[fdc].dor & 3) != UNIT(drive) || fdc != FDC(drive)) { > DPRINT("probing disk change on unselected drive\n"); > DPRINT("drive=%d fdc=%d dor=%x\n", drive, FDC(drive), > - (unsigned int)FDCS->dor); > + (unsigned int)fdc_state[fdc].dor); > } > > debug_dcl(UDP->flags, > @@ -799,10 +798,10 @@ static int set_dor(int fdc, char mask, char data) > unsigned char newdor; > unsigned char olddor; > > - if (FDCS->address == -1) > + if (fdc_state[fdc].address == -1) > return -1; > > - olddor = FDCS->dor; > + olddor = fdc_state[fdc].dor; > newdor = (olddor & mask) | data; > if (newdor != olddor) { > unit = olddor & 0x3; > @@ -812,7 +811,7 @@ static int set_dor(int fdc, char mask, char data) > "calling disk change from set_dor\n"); > disk_change(drive); > } > - FDCS->dor = newdor; > + fdc_state[fdc].dor = newdor; > fd_outb(newdor, FD_DOR); > > unit = newdor & 0x3; > @@ -828,8 +827,8 @@ static void twaddle(void) > { > if (DP->select_delay) > return; > - fd_outb(FDCS->dor & ~(0x10 << UNIT(current_drive)), FD_DOR); > - fd_outb(FDCS->dor, FD_DOR); > + fd_outb(fdc_state[fdc].dor & ~(0x10 << UNIT(current_drive)), FD_DOR); > + fd_outb(fdc_state[fdc].dor, FD_DOR); > DRS->select_date = jiffies; > } > > @@ -841,10 +840,10 @@ static void reset_fdc_info(int mode) > { > int drive; > > - FDCS->spec1 = FDCS->spec2 = -1; > - FDCS->need_configure = 1; > - FDCS->perp_mode = 1; > - FDCS->rawcmd = 0; > + fdc_state[fdc].spec1 = fdc_state[fdc].spec2 = -1; > + fdc_state[fdc].need_configure = 1; > + fdc_state[fdc].perp_mode = 1; > + fdc_state[fdc].rawcmd = 0; > for (drive = 0; drive < N_DRIVE; drive++) > if (FDC(drive) == fdc && (mode || UDRS->track != NEED_1_RECAL)) > UDRS->track = NEED_2_RECAL; > @@ -868,10 +867,10 @@ static void set_fdc(int drive) > #if N_FDC > 1 > set_dor(1 - fdc, ~8, 0); > #endif > - if (FDCS->rawcmd == 2) > + if (fdc_state[fdc].rawcmd == 2) > reset_fdc_info(1); > if (fd_inb(FD_STATUS) != STATUS_READY) > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > } > > /* locks the driver */ > @@ -924,7 +923,7 @@ static void floppy_off(unsigned int drive) > unsigned long volatile delta; > int fdc = FDC(drive); > > - if (!(FDCS->dor & (0x10 << UNIT(drive)))) > + if (!(fdc_state[fdc].dor & (0x10 << UNIT(drive)))) > return; > > del_timer(motor_off_timer + drive); > @@ -1035,7 +1034,7 @@ static void main_command_interrupt(void) > static int fd_wait_for_completion(unsigned long expires, > void (*function)(void)) > { > - if (FDCS->reset) { > + if (fdc_state[fdc].reset) { > reset_fdc(); /* do the reset during sleep to win time > * if we don't need to sleep, it's a good > * occasion anyways */ > @@ -1063,13 +1062,13 @@ static void setup_DMA(void) > pr_cont("%x,", raw_cmd->cmd[i]); > pr_cont("\n"); > cont->done(0); > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return; > } > if (((unsigned long)raw_cmd->kernel_data) % 512) { > pr_info("non aligned address: %p\n", raw_cmd->kernel_data); > cont->done(0); > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return; > } > f = claim_dma_lock(); > @@ -1077,10 +1076,10 @@ static void setup_DMA(void) > #ifdef fd_dma_setup > if (fd_dma_setup(raw_cmd->kernel_data, raw_cmd->length, > (raw_cmd->flags & FD_RAW_READ) ? > - DMA_MODE_READ : DMA_MODE_WRITE, FDCS->address) < 0) { > + DMA_MODE_READ : DMA_MODE_WRITE, fdc_state[fdc].address) < 0) { > release_dma_lock(f); > cont->done(0); > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return; > } > release_dma_lock(f); > @@ -1091,7 +1090,7 @@ static void setup_DMA(void) > DMA_MODE_READ : DMA_MODE_WRITE); > fd_set_dma_addr(raw_cmd->kernel_data); > fd_set_dma_count(raw_cmd->length); > - virtual_dma_port = FDCS->address; > + virtual_dma_port = fdc_state[fdc].address; > fd_enable_dma(); > release_dma_lock(f); > #endif > @@ -1105,7 +1104,7 @@ static int wait_til_ready(void) > int status; > int counter; > > - if (FDCS->reset) > + if (fdc_state[fdc].reset) > return -1; > for (counter = 0; counter < 10000; counter++) { > status = fd_inb(FD_STATUS); > @@ -1116,7 +1115,7 @@ static int wait_til_ready(void) > DPRINT("Getstatus times out (%x) on fdc %d\n", status, fdc); > show_floppy(); > } > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return -1; > } > > @@ -1136,7 +1135,7 @@ static int output_byte(char byte) > output_log_pos = (output_log_pos + 1) % OLOGSIZE; > return 0; > } > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > if (initialized) { > DPRINT("Unable to send byte %x to FDC. Fdc=%x Status=%x\n", > byte, fdc, status); > @@ -1171,7 +1170,7 @@ static int result(void) > fdc, status, i); > show_floppy(); > } > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return -1; > } > > @@ -1208,7 +1207,7 @@ static void perpendicular_mode(void) > default: > DPRINT("Invalid data rate for perpendicular mode!\n"); > cont->done(0); > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > /* > * convenient way to return to > * redo without too much hassle > @@ -1219,12 +1218,12 @@ static void perpendicular_mode(void) > } else > perp_mode = 0; > > - if (FDCS->perp_mode == perp_mode) > + if (fdc_state[fdc].perp_mode == perp_mode) > return; > - if (FDCS->version >= FDC_82077_ORIG) { > + if (fdc_state[fdc].version >= FDC_82077_ORIG) { > output_byte(FD_PERPENDICULAR); > output_byte(perp_mode); > - FDCS->perp_mode = perp_mode; > + fdc_state[fdc].perp_mode = perp_mode; > } else if (perp_mode) { > DPRINT("perpendicular mode not supported by this FDC.\n"); > } > @@ -1279,9 +1278,9 @@ static void fdc_specify(void) > int hlt_max_code = 0x7f; > int hut_max_code = 0xf; > > - if (FDCS->need_configure && FDCS->version >= FDC_82072A) { > + if (fdc_state[fdc].need_configure && fdc_state[fdc].version >= FDC_82072A) { > fdc_configure(); > - FDCS->need_configure = 0; > + fdc_state[fdc].need_configure = 0; > } > > switch (raw_cmd->rate & 0x03) { > @@ -1290,7 +1289,7 @@ static void fdc_specify(void) > break; > case 1: > dtr = 300; > - if (FDCS->version >= FDC_82078) { > + if (fdc_state[fdc].version >= FDC_82078) { > /* chose the default rate table, not the one > * where 1 = 2 Mbps */ > output_byte(FD_DRIVESPEC); > @@ -1305,7 +1304,7 @@ static void fdc_specify(void) > break; > } > > - if (FDCS->version >= FDC_82072) { > + if (fdc_state[fdc].version >= FDC_82072) { > scale_dtr = dtr; > hlt_max_code = 0x00; /* 0==256msec*dtr0/dtr (not linear!) */ > hut_max_code = 0x0; /* 0==256msec*dtr0/dtr (not linear!) */ > @@ -1335,11 +1334,11 @@ static void fdc_specify(void) > spec2 = (hlt << 1) | (use_virtual_dma & 1); > > /* If these parameters did not change, just return with success */ > - if (FDCS->spec1 != spec1 || FDCS->spec2 != spec2) { > + if (fdc_state[fdc].spec1 != spec1 || fdc_state[fdc].spec2 != spec2) { > /* Go ahead and set spec1 and spec2 */ > output_byte(FD_SPECIFY); > - output_byte(FDCS->spec1 = spec1); > - output_byte(FDCS->spec2 = spec2); > + output_byte(fdc_state[fdc].spec1 = spec1); > + output_byte(fdc_state[fdc].spec2 = spec2); > } > } /* fdc_specify */ > > @@ -1350,7 +1349,7 @@ static void fdc_specify(void) > static int fdc_dtr(void) > { > /* If data rate not already set to desired value, set it. */ > - if ((raw_cmd->rate & 3) == FDCS->dtr) > + if ((raw_cmd->rate & 3) == fdc_state[fdc].dtr) > return 0; > > /* Set dtr */ > @@ -1361,7 +1360,7 @@ static int fdc_dtr(void) > * enforced after data rate changes before R/W operations. > * Pause 5 msec to avoid trouble. (Needs to be 2 jiffies) > */ > - FDCS->dtr = raw_cmd->rate & 3; > + fdc_state[fdc].dtr = raw_cmd->rate & 3; > return fd_wait_for_completion(jiffies + 2UL * HZ / 100, floppy_ready); > } /* fdc_dtr */ > > @@ -1414,7 +1413,7 @@ static int interpret_errors(void) > > if (inr != 7) { > DPRINT("-- FDC reply error\n"); > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return 1; > } > > @@ -1548,7 +1547,7 @@ static void check_wp(void) > output_byte(FD_GETSTATUS); > output_byte(UNIT(current_drive)); > if (result() != 1) { > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return; > } > clear_bit(FD_VERIFY_BIT, &DRS->flags); > @@ -1625,7 +1624,7 @@ static void recal_interrupt(void) > { > debugt(__func__, ""); > if (inr != 2) > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > else if (ST0 & ST0_ECE) { > switch (DRS->track) { > case NEED_1_RECAL: > @@ -1693,7 +1692,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id) > release_dma_lock(f); > > do_floppy = NULL; > - if (fdc >= N_FDC || FDCS->address == -1) { > + if (fdc >= N_FDC || fdc_state[fdc].address == -1) { > /* we don't even know which FDC is the culprit */ > pr_info("DOR0=%x\n", fdc_state[0].dor); > pr_info("floppy interrupt on bizarre fdc %d\n", fdc); > @@ -1702,11 +1701,11 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id) > return IRQ_NONE; > } > > - FDCS->reset = 0; > + fdc_state[fdc].reset = 0; > /* We have to clear the reset flag here, because apparently on boxes > * with level triggered interrupts (PS/2, Sparc, ...), it is needed to > - * emit SENSEI's to clear the interrupt line. And FDCS->reset blocks the > - * emission of the SENSEI's. > + * emit SENSEI's to clear the interrupt line. And fdc_state[fdc].reset > + * blocks the emission of the SENSEI's. > * It is OK to emit floppy commands because we are in an interrupt > * handler here, and thus we have to fear no interference of other > * activity. > @@ -1729,7 +1728,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id) > inr == 2 && max_sensei); > } > if (!handler) { > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > return IRQ_NONE; > } > schedule_bh(handler); > @@ -1755,7 +1754,7 @@ static void reset_interrupt(void) > { > debugt(__func__, ""); > result(); /* get the status ready for set_fdc */ > - if (FDCS->reset) { > + if (fdc_state[fdc].reset) { > pr_info("reset set in interrupt, calling %ps\n", cont->error); > cont->error(); /* a reset just after a reset. BAD! */ > } > @@ -1771,7 +1770,7 @@ static void reset_fdc(void) > unsigned long flags; > > do_floppy = reset_interrupt; > - FDCS->reset = 0; > + fdc_state[fdc].reset = 0; > reset_fdc_info(0); > > /* Pseudo-DMA may intercept 'reset finished' interrupt. */ > @@ -1781,12 +1780,12 @@ static void reset_fdc(void) > fd_disable_dma(); > release_dma_lock(flags); > > - if (FDCS->version >= FDC_82072A) > - fd_outb(0x80 | (FDCS->dtr & 3), FD_STATUS); > + if (fdc_state[fdc].version >= FDC_82072A) > + fd_outb(0x80 | (fdc_state[fdc].dtr & 3), FD_STATUS); > else { > - fd_outb(FDCS->dor & ~0x04, FD_DOR); > + fd_outb(fdc_state[fdc].dor & ~0x04, FD_DOR); > udelay(FD_RESET_DELAY); > - fd_outb(FDCS->dor, FD_DOR); > + fd_outb(fdc_state[fdc].dor, FD_DOR); > } > } > > @@ -1850,7 +1849,7 @@ static void floppy_shutdown(struct work_struct *arg) > > if (initialized) > DPRINT("floppy timeout called\n"); > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > if (cont) { > cont->done(0); > cont->redo(); /* this will recall reset when needed */ > @@ -1870,7 +1869,7 @@ static int start_motor(void (*function)(void)) > mask = 0xfc; > data = UNIT(current_drive); > if (!(raw_cmd->flags & FD_RAW_NO_MOTOR)) { > - if (!(FDCS->dor & (0x10 << UNIT(current_drive)))) { > + if (!(fdc_state[fdc].dor & (0x10 << UNIT(current_drive)))) { > set_debugt(); > /* no read since this drive is running */ > DRS->first_read_date = 0; > @@ -1878,7 +1877,7 @@ static int start_motor(void (*function)(void)) > DRS->spinup_date = jiffies; > data |= (0x10 << UNIT(current_drive)); > } > - } else if (FDCS->dor & (0x10 << UNIT(current_drive))) > + } else if (fdc_state[fdc].dor & (0x10 << UNIT(current_drive))) > mask &= ~(0x10 << UNIT(current_drive)); > > /* starts motor and selects floppy */ > @@ -1892,7 +1891,7 @@ static int start_motor(void (*function)(void)) > > static void floppy_ready(void) > { > - if (FDCS->reset) { > + if (fdc_state[fdc].reset) { > reset_fdc(); > return; > } > @@ -1991,7 +1990,7 @@ static int wait_til_done(void (*handler)(void), bool interruptible) > return -EINTR; > } > > - if (FDCS->reset) > + if (fdc_state[fdc].reset) > command_status = FD_COMMAND_ERROR; > if (command_status == FD_COMMAND_OKAY) > ret = 0; > @@ -2060,7 +2059,7 @@ static void bad_flp_intr(void) > if (err_count > DP->max_errors.abort) > cont->done(0); > if (err_count > DP->max_errors.reset) > - FDCS->reset = 1; > + fdc_state[fdc].reset = 1; > else if (err_count > DP->max_errors.recal) > DRS->track = NEED_2_RECAL; > } > @@ -2967,8 +2966,8 @@ static int user_reset_fdc(int drive, int arg, bool interruptible) > return -EINTR; > > if (arg == FD_RESET_ALWAYS) > - FDCS->reset = 1; > - if (FDCS->reset) { > + fdc_state[fdc].reset = 1; > + if (fdc_state[fdc].reset) { > cont = &reset_cont; > ret = wait_til_done(reset_fdc, interruptible); > if (ret == -EINTR) > @@ -3179,23 +3178,23 @@ static int raw_cmd_ioctl(int cmd, void __user *param) > int ret2; > int ret; > > - if (FDCS->rawcmd <= 1) > - FDCS->rawcmd = 1; > + if (fdc_state[fdc].rawcmd <= 1) > + fdc_state[fdc].rawcmd = 1; > for (drive = 0; drive < N_DRIVE; drive++) { > if (FDC(drive) != fdc) > continue; > if (drive == current_drive) { > if (UDRS->fd_ref > 1) { > - FDCS->rawcmd = 2; > + fdc_state[fdc].rawcmd = 2; > break; > } > } else if (UDRS->fd_ref) { > - FDCS->rawcmd = 2; > + fdc_state[fdc].rawcmd = 2; > break; > } > } > > - if (FDCS->reset) > + if (fdc_state[fdc].reset) > return -EIO; > > ret = raw_cmd_copyin(cmd, param, &my_raw_cmd); > @@ -3209,7 +3208,7 @@ static int raw_cmd_ioctl(int cmd, void __user *param) > ret = wait_til_done(floppy_start, true); > debug_dcl(DP->flags, "calling disk change from raw_cmd ioctl\n"); > > - if (ret != -EINTR && FDCS->reset) > + if (ret != -EINTR && fdc_state[fdc].reset) > ret = -EIO; > > DRS->track = NO_TRACK; > @@ -4261,7 +4260,7 @@ static char __init get_fdc_version(void) > int r; > > output_byte(FD_DUMPREGS); /* 82072 and better know DUMPREGS */ > - if (FDCS->reset) > + if (fdc_state[fdc].reset) > return FDC_NONE; > r = result(); > if (r <= 0x00) > @@ -4494,7 +4493,7 @@ static int floppy_resume(struct device *dev) > int fdc; > > for (fdc = 0; fdc < N_FDC; fdc++) > - if (FDCS->address != -1) > + if (fdc_state[fdc].address != -1) > user_reset_fdc(-1, FD_RESET_ALWAYS, false); > > return 0; > @@ -4605,15 +4604,15 @@ static int __init do_floppy_init(void) > > for (i = 0; i < N_FDC; i++) { > fdc = i; > - memset(FDCS, 0, sizeof(*FDCS)); > - FDCS->dtr = -1; > - FDCS->dor = 0x4; > + memset(&fdc_state[fdc], 0, sizeof(*fdc_state)); > + fdc_state[fdc].dtr = -1; > + fdc_state[fdc].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 > - FDCS->version = FDC_82072A; > + fdc_state[fdc].version = FDC_82072A; > #endif > } > > @@ -4656,28 +4655,28 @@ static int __init do_floppy_init(void) > > for (i = 0; i < N_FDC; i++) { > fdc = i; > - FDCS->driver_version = FD_DRIVER_VERSION; > + fdc_state[fdc].driver_version = FD_DRIVER_VERSION; > for (unit = 0; unit < 4; unit++) > - FDCS->track[unit] = 0; > - if (FDCS->address == -1) > + fdc_state[fdc].track[unit] = 0; > + if (fdc_state[fdc].address == -1) > continue; > - FDCS->rawcmd = 2; > + fdc_state[fdc].rawcmd = 2; > if (user_reset_fdc(-1, FD_RESET_ALWAYS, false)) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > floppy_release_regions(fdc); > - FDCS->address = -1; > - FDCS->version = FDC_NONE; > + fdc_state[fdc].address = -1; > + fdc_state[fdc].version = FDC_NONE; > continue; > } > /* Try to determine the floppy controller type */ > - FDCS->version = get_fdc_version(); > - if (FDCS->version == FDC_NONE) { > + fdc_state[fdc].version = get_fdc_version(); > + if (fdc_state[fdc].version == FDC_NONE) { > /* free ioports reserved by floppy_grab_irq_and_dma() */ > floppy_release_regions(fdc); > - FDCS->address = -1; > + fdc_state[fdc].address = -1; > continue; > } > - if (can_use_virtual_dma == 2 && FDCS->version < FDC_82072A) > + if (can_use_virtual_dma == 2 && fdc_state[fdc].version < FDC_82072A) > can_use_virtual_dma = 0; > > have_no_fdc = 0; > @@ -4783,7 +4782,7 @@ static void floppy_release_allocated_regions(int fdc, const struct io_region *p) > { > while (p != io_regions) { > p--; > - release_region(FDCS->address + p->offset, p->size); > + release_region(fdc_state[fdc].address + p->offset, p->size); > } > } > > @@ -4794,10 +4793,10 @@ static int floppy_request_regions(int fdc) > const struct io_region *p; > > for (p = io_regions; p < ARRAY_END(io_regions); p++) { > - if (!request_region(FDCS->address + p->offset, > + if (!request_region(fdc_state[fdc].address + p->offset, > p->size, "floppy")) { > DPRINT("Floppy io-port 0x%04lx in use\n", > - FDCS->address + p->offset); > + fdc_state[fdc].address + p->offset); > floppy_release_allocated_regions(fdc, p); > return -EBUSY; > } > @@ -4840,23 +4839,23 @@ static int floppy_grab_irq_and_dma(void) > } > > for (fdc = 0; fdc < N_FDC; fdc++) { > - if (FDCS->address != -1) { > + if (fdc_state[fdc].address != -1) { > if (floppy_request_regions(fdc)) > goto cleanup; > } > } > for (fdc = 0; fdc < N_FDC; fdc++) { > - if (FDCS->address != -1) { > + if (fdc_state[fdc].address != -1) { > reset_fdc_info(1); > - fd_outb(FDCS->dor, FD_DOR); > + fd_outb(fdc_state[fdc].dor, FD_DOR); > } > } > fdc = 0; > set_dor(0, ~0, 8); /* avoid immediate interrupt */ > > for (fdc = 0; fdc < N_FDC; fdc++) > - if (FDCS->address != -1) > - fd_outb(FDCS->dor, FD_DOR); > + if (fdc_state[fdc].address != -1) > + fd_outb(fdc_state[fdc].dor, FD_DOR); > /* > * The driver will try and free resources and relies on us > * to know if they were allocated or not. > @@ -4918,7 +4917,7 @@ static void floppy_release_irq_and_dma(void) > pr_info("work still pending\n"); > old_fdc = fdc; > for (fdc = 0; fdc < N_FDC; fdc++) > - if (FDCS->address != -1) > + if (fdc_state[fdc].address != -1) > floppy_release_regions(fdc); > fdc = old_fdc; > } >
On Tue, Feb 25, 2020 at 10:14:40AM +0300, Denis Efremov wrote: > > > On 2/25/20 6:45 AM, Willy Tarreau wrote: > > On Tue, Feb 25, 2020 at 02:13:42AM +0300, Denis Efremov wrote: > >> On 2/25/20 12:53 AM, Linus Torvalds wrote: > >>> So I'd like to see that second step that does the > >>> > >>> -static int fdc; /* current fdc */ > >>> +static int current_fdc; > >>> > >>> change. > >>> > >>> We already call the global 'drive' variable 'current_drive', so it > >>> really is 'fdc' that is misnamed and ambiguous because it then has two > >>> different cases: the global 'fdc' and then the various shadowing local > >>> 'fdc' variables (or function arguments). > >>> > >>> Mind adding that too? Slightly less automatic, I agree, because then > >>> you really do have to disambiguate between the "is this the shadowed > >>> use of a local 'fdc'" case or the "this is the global 'fdc' use" case. > > > > I definitely agree. I first wanted to be sure the patches were acceptable > > as a principle, but disambiguating the variables is easy to do now. > > Ok, I don't want to break in the middle of your changes in this case. So I started this and discovered the nice joke you were telling me about regarding FD_IOPORT which references fdc. Then the address registers FD_STATUS, FD_DATA, FD_DOR, FD_DIR, FD_DCR which are based on FD_IOPORT also depend on it. These ones are used by fd_outb() which is arch-dependent, so if we want to pass a third argument we have to change them all and make sure not to break them too much. In addition the FD_* macros defined above are used by x86, and FD_DOR is also used by arm while all other archs hard-code all the values. ARM also uses floppy_selects[fdc] and new_dor... I'm starting to feel the trap here! I also feel a bit concerned that these are exported in uapi with a hard-coded 0x3f0 base address. I'm just not sure how portable all of this is in the end :-/ Now I'm wondering, how far should we go and how much is it acceptable to change ? I'd rather not have "#define fdc current_fdc" just so that it builds, but on the other hand this problem clearly outlights the roots of the issue, which lies in "fdc" being silently accessed by macros with nobody noticing! Willy
On 2/25/20 5:02 PM, Willy Tarreau wrote: > On Tue, Feb 25, 2020 at 10:14:40AM +0300, Denis Efremov wrote: >> >> >> On 2/25/20 6:45 AM, Willy Tarreau wrote: >>> On Tue, Feb 25, 2020 at 02:13:42AM +0300, Denis Efremov wrote: >>>> On 2/25/20 12:53 AM, Linus Torvalds wrote: >>>>> So I'd like to see that second step that does the >>>>> >>>>> -static int fdc; /* current fdc */ >>>>> +static int current_fdc; >>>>> >>>>> change. >>>>> >>>>> We already call the global 'drive' variable 'current_drive', so it >>>>> really is 'fdc' that is misnamed and ambiguous because it then has two >>>>> different cases: the global 'fdc' and then the various shadowing local >>>>> 'fdc' variables (or function arguments). >>>>> >>>>> Mind adding that too? Slightly less automatic, I agree, because then >>>>> you really do have to disambiguate between the "is this the shadowed >>>>> use of a local 'fdc'" case or the "this is the global 'fdc' use" case. >>> >>> I definitely agree. I first wanted to be sure the patches were acceptable >>> as a principle, but disambiguating the variables is easy to do now. >> >> Ok, I don't want to break in the middle of your changes in this case. > > So I started this and discovered the nice joke you were telling me > about regarding FD_IOPORT which references fdc. Then the address > registers FD_STATUS, FD_DATA, FD_DOR, FD_DIR, FD_DCR which are > based on FD_IOPORT also depend on it. > > These ones are used by fd_outb() which is arch-dependent, so if we > want to pass a third argument we have to change them all and make sure > not to break them too much. > > In addition the FD_* macros defined above are used by x86, and FD_DOR is > also used by arm while all other archs hard-code all the values. ARM also > uses floppy_selects[fdc] and new_dor... I'm starting to feel the trap here! > I also feel a bit concerned that these are exported in uapi with a hard-coded > 0x3f0 base address. I'm just not sure how portable all of this is in > the end :-/ > > Now I'm wondering, how far should we go and how much is it acceptable to > change ? I'd rather not have "#define fdc current_fdc" just so that it > builds, but on the other hand this problem clearly outlights the roots > of the issue, which lies in "fdc" being silently accessed by macros with > nobody noticing! > I think that for the first attempt changing will be enough: -static int fdc; /* current fdc */ +static int current_fdc; /* current fdc */ and -#define FD_IOPORT fdc_state[fdc].address +#define FD_IOPORT fdc_state[current_fdc].address and for fd_setdor in ./arch/arm/include/asm/floppy.h This already will require to at least test the building on x86, arm, sparc arches (maybe more). Just need to leave a note in the commit why it's not easy to change FD_IOPORT in this case. I think that small-step and observable patches are the best option when we change floppy driver. As for now, I can see that only floppy.c includes fdreg.h file with define FDPATCHES. If it's true then #define FD_IOPORT 0x3f0 branch is never used and we can try to fix remaining FD_* macro in the next round. Denis
On 2/25/20 6:22 PM, Denis Efremov wrote: > As for now, I can see that only floppy.c includes fdreg.h file > with define FDPATCHES. If it's true then #define FD_IOPORT 0x3f0 > branch is never used and we can try to fix remaining FD_* macro > in the next round. Ah, I forgot that fdregs.h is uapi. Thus, we can't simplify FDPATCHES. Denis
On Tue, Feb 25, 2020 at 06:39:05PM +0300, Denis Efremov wrote: > On 2/25/20 6:22 PM, Denis Efremov wrote: > > As for now, I can see that only floppy.c includes fdreg.h file > > with define FDPATCHES. If it's true then #define FD_IOPORT 0x3f0 > > branch is never used and we can try to fix remaining FD_* macro > > in the next round. > > Ah, I forgot that fdregs.h is uapi. Thus, we can't simplify FDPATCHES. Yep, that's why we can't do it. I also agree with the other change of fdc->current_fdc in floppy.h, I think it's the most reasonable. And if it breaks anywhere, it will simply have uncovered new latent bugs because it will mean that it was using the wrong fdc. Willy
On Tue, Feb 25, 2020 at 06:22:47PM +0300, Denis Efremov wrote: > I think that for the first attempt changing will be enough: > -static int fdc; /* current fdc */ > +static int current_fdc; /* current fdc */ > and > -#define FD_IOPORT fdc_state[fdc].address > +#define FD_IOPORT fdc_state[current_fdc].address > and for fd_setdor in ./arch/arm/include/asm/floppy.h So after a bit more digging, that should not be correct because: - disk_change() uses a local "fdc" variable with expectations that it will be used by fd_inb(FD_DIR) - set_dor() uses a local fdc argument that's used by fd_outb(newdor, FD_DOR) Here we have "fdc" hidden in: - FD_DOR/FD_DIR (referencing FD_IOPORT) on x86 - fd_outb(), relying on fd_setdor() on ARM I'm now looking how to change fd_outb() to pass the fdc in argument, after all it's not that many places and that's exactly what we need. Maybe afterwards we'll figure that some of them are still wrong :-) Willy
On Tue, Feb 25, 2020 at 07:02:19PM +0100, Willy Tarreau wrote: > On Tue, Feb 25, 2020 at 06:22:47PM +0300, Denis Efremov wrote: > > I think that for the first attempt changing will be enough: > > -static int fdc; /* current fdc */ > > +static int current_fdc; /* current fdc */ > > and > > -#define FD_IOPORT fdc_state[fdc].address > > +#define FD_IOPORT fdc_state[current_fdc].address > > and for fd_setdor in ./arch/arm/include/asm/floppy.h > > So after a bit more digging, that should not be correct because: > - disk_change() uses a local "fdc" variable with expectations that > it will be used by fd_inb(FD_DIR) > > - set_dor() uses a local fdc argument that's used by > fd_outb(newdor, FD_DOR) > > Here we have "fdc" hidden in: > - FD_DOR/FD_DIR (referencing FD_IOPORT) on x86 > - fd_outb(), relying on fd_setdor() on ARM And in the ARM case, fdc is used to index a two-entries array with exactly identical values, with N_FDC set to 1 so even there it's pointless... Maybe I'll get rid of this first. Willy
On Tue, Feb 25, 2020 at 7:22 AM Denis Efremov <efremov@linux.com> wrote: > > I think that for the first attempt changing will be enough: > -static int fdc; /* current fdc */ > +static int current_fdc; /* current fdc */ > and > -#define FD_IOPORT fdc_state[fdc].address > +#define FD_IOPORT fdc_state[current_fdc].address Please don't do this blindly - ie without verifying that there are no cases of that "local fdc variable shadowing" issue. Of course, such a verification might be as easy as "generates exact same code" rather than looking at every use. And btw, don't worry too much about this being in an UAPI file. I'm pretty sure that's because of specialty programs that use the magical ioctls to do special formatting. They want the special commands (FD_FORMAT etc), but I don't think they really use the port addresses. So I think it's in a UAPI file entirely by mistake. We should at least try moving those bits to the floppy.c file and remove it from the header file. For example, doing a Debian code search on "FDPATCHES" doesn't find any user space hits. Searching for "FD_STATUS" gets a lot of hits, but thos all seem to be because it's a symbol used by user space programs, ("file descriptor status"), not because those hits actually used the fdreg.h header file. So we can remove at least the FD_IOPORT mess from the header file, I bet. Worst case - if somebody finds some case that uses them, we can put it back. Linus
On Tue, Feb 25, 2020 at 10:08:51AM -0800, Linus Torvalds wrote: > On Tue, Feb 25, 2020 at 7:22 AM Denis Efremov <efremov@linux.com> wrote: > > > > I think that for the first attempt changing will be enough: > > -static int fdc; /* current fdc */ > > +static int current_fdc; /* current fdc */ > > and > > -#define FD_IOPORT fdc_state[fdc].address > > +#define FD_IOPORT fdc_state[current_fdc].address > > Please don't do this blindly - ie without verifying that there are no > cases of that "local fdc variable shadowing" issue. That's exactly what I'm doing. In fact I first renamed the variable and am manually checking all places which do not compile anymore. Hence the surprizes. > Of course, such a verification might be as easy as "generates exact > same code" rather than looking at every use. That's exactly what I'm doing. > And btw, don't worry too much about this being in an UAPI file. I'm > pretty sure that's because of specialty programs that use the magical > ioctls to do special formatting. They want the special commands > (FD_FORMAT etc), but I don't think they really use the port addresses. > > So I think it's in a UAPI file entirely by mistake. OK this will help me, thanks for the hint :-) > We should at least try moving those bits to the floppy.c file and > remove it from the header file. Makes sense. > For example, doing a Debian code search on "FDPATCHES" doesn't find > any user space hits. Searching for "FD_STATUS" gets a lot of hits, but > thos all seem to be because it's a symbol used by user space programs, > ("file descriptor status"), not because those hits actually used the > fdreg.h header file. > > So we can remove at least the FD_IOPORT mess from the header file, I bet. > > Worst case - if somebody finds some case that uses them, we can put it back. I like that. And at least we'll know how they use it (likely without the dependency on fdc). Thanks, Willy
On Tue, Feb 25, 2020 at 10:15 AM Willy Tarreau <w@1wt.eu> wrote: > > On Tue, Feb 25, 2020 at 10:08:51AM -0800, Linus Torvalds wrote: > > > > So we can remove at least the FD_IOPORT mess from the header file, I bet. > > > > Worst case - if somebody finds some case that uses them, we can put it back. > > I like that. And at least we'll know how they use it (likely without the > dependency on fdc). Note that the way uapi header files generally got created was by just moving header files that user space used mechanically. See for example commit 607ca46e97a1 ("UAPI: (Scripted) Disintegrate include/linux") which created most of them. There was no careful vetting of "this is the part that is used by user space". It was just a "these are the files user space has used". So it's not really a "the uapi files are set in stone and you can't change them". Instead, you should think of the uapi files as a big red blinking warning that says "sure, you can change them, but you need to be very careful and think about the fact that user space may be including this thing". So it's a "think hard about it" rather than a "don't go there". Of course, usually it's much _simpler_ to just "don't go there" ;) Linus
On Tue, Feb 25, 2020 at 10:08:51AM -0800, Linus Torvalds wrote: > We should at least try moving those bits to the floppy.c file and > remove it from the header file. > > For example, doing a Debian code search on "FDPATCHES" doesn't find > any user space hits. Searching for "FD_STATUS" gets a lot of hits, but > thos all seem to be because it's a symbol used by user space programs, > ("file descriptor status"), not because those hits actually used the > fdreg.h header file. > > So we can remove at least the FD_IOPORT mess from the header file, I bet. OK so I think this time I managed to get it done after two failed attempts. I've sent in response to this thread 6 new patches to the series just for validation (11 to 16), I'll spam relevant people when resending the whole if we agree on the principle already. First, still no single byte change in the output code: willy@wtap:master$ diff -u floppy-{before,after}.s --- floppy-before.s 2020-02-26 08:59:04.185152450 +0100 +++ floppy-after.s 2020-02-26 08:58:58.253156733 +0100 @@ -1,5 +1,5 @@ -floppy-before.o: file format elf64-x86-64 +floppy-after.o: file format elf64-x86-64 Disassembly of section .text: Second, I could kill FD_IOPORT entirely. The FD_* macros are now just the registers offsets. I've added two local functions fdc_inb() and fdc_outb() which take an fdc and the register, and remap this to fd_inb() and fd_outb() so that we don't need to fiddle anymore with "fdc". I had one attempt at propagating that cleanup (base+reg instead of port) to various archs, it was OK but didn't bring any visible value in my opinion. Third, I renamed "fdc" to "current_fdc" and carefully replaced all "fdc" instances which didn't build with "current_fdc". This revealed that at many places we iterate over current_fdc just because it was the required name for the register macro (which used to derive from FD_IOPORT). So at this point I'm still seeing a lot of possible cleanups which will produce different binary output but will be quite more reviewable. The common pattern in floppy.c is : for (current_fdc = 0; current_fdc < N_FDC; current_fdc++) { do_something(current_fdc); } current_fdc = 0; or: for (i = 0; i < N_FDC; i++) { current_fdc = i; do_something(current_fdc); } current_fdc = 0; These ones can safely be cleaned up. I also thought that once done we could have a "current_fdc" being a struct floppy_fdc_state* instead of an int and directly point to the correct fdc_state. This way we'll regain a lot of readability in the code. Please just tell me what you think and if I should repost a whole series and/or continue the cleanup. Thanks, Willy
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c index 8ef65c0..93e0840 100644 --- a/drivers/block/floppy.c +++ b/drivers/block/floppy.c @@ -309,7 +309,6 @@ static bool initialized; #define DP (&drive_params[current_drive]) #define DRS (&drive_state[current_drive]) #define DRWE (&write_errors[current_drive]) -#define FDCS (&fdc_state[fdc]) #define UDP (&drive_params[drive]) #define UDRS (&drive_state[drive]) @@ -742,11 +741,11 @@ static int disk_change(int drive) if (time_before(jiffies, UDRS->select_date + UDP->select_delay)) DPRINT("WARNING disk change called early\n"); - if (!(FDCS->dor & (0x10 << UNIT(drive))) || - (FDCS->dor & 3) != UNIT(drive) || fdc != FDC(drive)) { + if (!(fdc_state[fdc].dor & (0x10 << UNIT(drive))) || + (fdc_state[fdc].dor & 3) != UNIT(drive) || fdc != FDC(drive)) { DPRINT("probing disk change on unselected drive\n"); DPRINT("drive=%d fdc=%d dor=%x\n", drive, FDC(drive), - (unsigned int)FDCS->dor); + (unsigned int)fdc_state[fdc].dor); } debug_dcl(UDP->flags, @@ -799,10 +798,10 @@ static int set_dor(int fdc, char mask, char data) unsigned char newdor; unsigned char olddor; - if (FDCS->address == -1) + if (fdc_state[fdc].address == -1) return -1; - olddor = FDCS->dor; + olddor = fdc_state[fdc].dor; newdor = (olddor & mask) | data; if (newdor != olddor) { unit = olddor & 0x3; @@ -812,7 +811,7 @@ static int set_dor(int fdc, char mask, char data) "calling disk change from set_dor\n"); disk_change(drive); } - FDCS->dor = newdor; + fdc_state[fdc].dor = newdor; fd_outb(newdor, FD_DOR); unit = newdor & 0x3; @@ -828,8 +827,8 @@ static void twaddle(void) { if (DP->select_delay) return; - fd_outb(FDCS->dor & ~(0x10 << UNIT(current_drive)), FD_DOR); - fd_outb(FDCS->dor, FD_DOR); + fd_outb(fdc_state[fdc].dor & ~(0x10 << UNIT(current_drive)), FD_DOR); + fd_outb(fdc_state[fdc].dor, FD_DOR); DRS->select_date = jiffies; } @@ -841,10 +840,10 @@ static void reset_fdc_info(int mode) { int drive; - FDCS->spec1 = FDCS->spec2 = -1; - FDCS->need_configure = 1; - FDCS->perp_mode = 1; - FDCS->rawcmd = 0; + fdc_state[fdc].spec1 = fdc_state[fdc].spec2 = -1; + fdc_state[fdc].need_configure = 1; + fdc_state[fdc].perp_mode = 1; + fdc_state[fdc].rawcmd = 0; for (drive = 0; drive < N_DRIVE; drive++) if (FDC(drive) == fdc && (mode || UDRS->track != NEED_1_RECAL)) UDRS->track = NEED_2_RECAL; @@ -868,10 +867,10 @@ static void set_fdc(int drive) #if N_FDC > 1 set_dor(1 - fdc, ~8, 0); #endif - if (FDCS->rawcmd == 2) + if (fdc_state[fdc].rawcmd == 2) reset_fdc_info(1); if (fd_inb(FD_STATUS) != STATUS_READY) - FDCS->reset = 1; + fdc_state[fdc].reset = 1; } /* locks the driver */ @@ -924,7 +923,7 @@ static void floppy_off(unsigned int drive) unsigned long volatile delta; int fdc = FDC(drive); - if (!(FDCS->dor & (0x10 << UNIT(drive)))) + if (!(fdc_state[fdc].dor & (0x10 << UNIT(drive)))) return; del_timer(motor_off_timer + drive); @@ -1035,7 +1034,7 @@ static void main_command_interrupt(void) static int fd_wait_for_completion(unsigned long expires, void (*function)(void)) { - if (FDCS->reset) { + if (fdc_state[fdc].reset) { reset_fdc(); /* do the reset during sleep to win time * if we don't need to sleep, it's a good * occasion anyways */ @@ -1063,13 +1062,13 @@ static void setup_DMA(void) pr_cont("%x,", raw_cmd->cmd[i]); pr_cont("\n"); cont->done(0); - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return; } if (((unsigned long)raw_cmd->kernel_data) % 512) { pr_info("non aligned address: %p\n", raw_cmd->kernel_data); cont->done(0); - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return; } f = claim_dma_lock(); @@ -1077,10 +1076,10 @@ static void setup_DMA(void) #ifdef fd_dma_setup if (fd_dma_setup(raw_cmd->kernel_data, raw_cmd->length, (raw_cmd->flags & FD_RAW_READ) ? - DMA_MODE_READ : DMA_MODE_WRITE, FDCS->address) < 0) { + DMA_MODE_READ : DMA_MODE_WRITE, fdc_state[fdc].address) < 0) { release_dma_lock(f); cont->done(0); - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return; } release_dma_lock(f); @@ -1091,7 +1090,7 @@ static void setup_DMA(void) DMA_MODE_READ : DMA_MODE_WRITE); fd_set_dma_addr(raw_cmd->kernel_data); fd_set_dma_count(raw_cmd->length); - virtual_dma_port = FDCS->address; + virtual_dma_port = fdc_state[fdc].address; fd_enable_dma(); release_dma_lock(f); #endif @@ -1105,7 +1104,7 @@ static int wait_til_ready(void) int status; int counter; - if (FDCS->reset) + if (fdc_state[fdc].reset) return -1; for (counter = 0; counter < 10000; counter++) { status = fd_inb(FD_STATUS); @@ -1116,7 +1115,7 @@ static int wait_til_ready(void) DPRINT("Getstatus times out (%x) on fdc %d\n", status, fdc); show_floppy(); } - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return -1; } @@ -1136,7 +1135,7 @@ static int output_byte(char byte) output_log_pos = (output_log_pos + 1) % OLOGSIZE; return 0; } - FDCS->reset = 1; + fdc_state[fdc].reset = 1; if (initialized) { DPRINT("Unable to send byte %x to FDC. Fdc=%x Status=%x\n", byte, fdc, status); @@ -1171,7 +1170,7 @@ static int result(void) fdc, status, i); show_floppy(); } - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return -1; } @@ -1208,7 +1207,7 @@ static void perpendicular_mode(void) default: DPRINT("Invalid data rate for perpendicular mode!\n"); cont->done(0); - FDCS->reset = 1; + fdc_state[fdc].reset = 1; /* * convenient way to return to * redo without too much hassle @@ -1219,12 +1218,12 @@ static void perpendicular_mode(void) } else perp_mode = 0; - if (FDCS->perp_mode == perp_mode) + if (fdc_state[fdc].perp_mode == perp_mode) return; - if (FDCS->version >= FDC_82077_ORIG) { + if (fdc_state[fdc].version >= FDC_82077_ORIG) { output_byte(FD_PERPENDICULAR); output_byte(perp_mode); - FDCS->perp_mode = perp_mode; + fdc_state[fdc].perp_mode = perp_mode; } else if (perp_mode) { DPRINT("perpendicular mode not supported by this FDC.\n"); } @@ -1279,9 +1278,9 @@ static void fdc_specify(void) int hlt_max_code = 0x7f; int hut_max_code = 0xf; - if (FDCS->need_configure && FDCS->version >= FDC_82072A) { + if (fdc_state[fdc].need_configure && fdc_state[fdc].version >= FDC_82072A) { fdc_configure(); - FDCS->need_configure = 0; + fdc_state[fdc].need_configure = 0; } switch (raw_cmd->rate & 0x03) { @@ -1290,7 +1289,7 @@ static void fdc_specify(void) break; case 1: dtr = 300; - if (FDCS->version >= FDC_82078) { + if (fdc_state[fdc].version >= FDC_82078) { /* chose the default rate table, not the one * where 1 = 2 Mbps */ output_byte(FD_DRIVESPEC); @@ -1305,7 +1304,7 @@ static void fdc_specify(void) break; } - if (FDCS->version >= FDC_82072) { + if (fdc_state[fdc].version >= FDC_82072) { scale_dtr = dtr; hlt_max_code = 0x00; /* 0==256msec*dtr0/dtr (not linear!) */ hut_max_code = 0x0; /* 0==256msec*dtr0/dtr (not linear!) */ @@ -1335,11 +1334,11 @@ static void fdc_specify(void) spec2 = (hlt << 1) | (use_virtual_dma & 1); /* If these parameters did not change, just return with success */ - if (FDCS->spec1 != spec1 || FDCS->spec2 != spec2) { + if (fdc_state[fdc].spec1 != spec1 || fdc_state[fdc].spec2 != spec2) { /* Go ahead and set spec1 and spec2 */ output_byte(FD_SPECIFY); - output_byte(FDCS->spec1 = spec1); - output_byte(FDCS->spec2 = spec2); + output_byte(fdc_state[fdc].spec1 = spec1); + output_byte(fdc_state[fdc].spec2 = spec2); } } /* fdc_specify */ @@ -1350,7 +1349,7 @@ static void fdc_specify(void) static int fdc_dtr(void) { /* If data rate not already set to desired value, set it. */ - if ((raw_cmd->rate & 3) == FDCS->dtr) + if ((raw_cmd->rate & 3) == fdc_state[fdc].dtr) return 0; /* Set dtr */ @@ -1361,7 +1360,7 @@ static int fdc_dtr(void) * enforced after data rate changes before R/W operations. * Pause 5 msec to avoid trouble. (Needs to be 2 jiffies) */ - FDCS->dtr = raw_cmd->rate & 3; + fdc_state[fdc].dtr = raw_cmd->rate & 3; return fd_wait_for_completion(jiffies + 2UL * HZ / 100, floppy_ready); } /* fdc_dtr */ @@ -1414,7 +1413,7 @@ static int interpret_errors(void) if (inr != 7) { DPRINT("-- FDC reply error\n"); - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return 1; } @@ -1548,7 +1547,7 @@ static void check_wp(void) output_byte(FD_GETSTATUS); output_byte(UNIT(current_drive)); if (result() != 1) { - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return; } clear_bit(FD_VERIFY_BIT, &DRS->flags); @@ -1625,7 +1624,7 @@ static void recal_interrupt(void) { debugt(__func__, ""); if (inr != 2) - FDCS->reset = 1; + fdc_state[fdc].reset = 1; else if (ST0 & ST0_ECE) { switch (DRS->track) { case NEED_1_RECAL: @@ -1693,7 +1692,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id) release_dma_lock(f); do_floppy = NULL; - if (fdc >= N_FDC || FDCS->address == -1) { + if (fdc >= N_FDC || fdc_state[fdc].address == -1) { /* we don't even know which FDC is the culprit */ pr_info("DOR0=%x\n", fdc_state[0].dor); pr_info("floppy interrupt on bizarre fdc %d\n", fdc); @@ -1702,11 +1701,11 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id) return IRQ_NONE; } - FDCS->reset = 0; + fdc_state[fdc].reset = 0; /* We have to clear the reset flag here, because apparently on boxes * with level triggered interrupts (PS/2, Sparc, ...), it is needed to - * emit SENSEI's to clear the interrupt line. And FDCS->reset blocks the - * emission of the SENSEI's. + * emit SENSEI's to clear the interrupt line. And fdc_state[fdc].reset + * blocks the emission of the SENSEI's. * It is OK to emit floppy commands because we are in an interrupt * handler here, and thus we have to fear no interference of other * activity. @@ -1729,7 +1728,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id) inr == 2 && max_sensei); } if (!handler) { - FDCS->reset = 1; + fdc_state[fdc].reset = 1; return IRQ_NONE; } schedule_bh(handler); @@ -1755,7 +1754,7 @@ static void reset_interrupt(void) { debugt(__func__, ""); result(); /* get the status ready for set_fdc */ - if (FDCS->reset) { + if (fdc_state[fdc].reset) { pr_info("reset set in interrupt, calling %ps\n", cont->error); cont->error(); /* a reset just after a reset. BAD! */ } @@ -1771,7 +1770,7 @@ static void reset_fdc(void) unsigned long flags; do_floppy = reset_interrupt; - FDCS->reset = 0; + fdc_state[fdc].reset = 0; reset_fdc_info(0); /* Pseudo-DMA may intercept 'reset finished' interrupt. */ @@ -1781,12 +1780,12 @@ static void reset_fdc(void) fd_disable_dma(); release_dma_lock(flags); - if (FDCS->version >= FDC_82072A) - fd_outb(0x80 | (FDCS->dtr & 3), FD_STATUS); + if (fdc_state[fdc].version >= FDC_82072A) + fd_outb(0x80 | (fdc_state[fdc].dtr & 3), FD_STATUS); else { - fd_outb(FDCS->dor & ~0x04, FD_DOR); + fd_outb(fdc_state[fdc].dor & ~0x04, FD_DOR); udelay(FD_RESET_DELAY); - fd_outb(FDCS->dor, FD_DOR); + fd_outb(fdc_state[fdc].dor, FD_DOR); } } @@ -1850,7 +1849,7 @@ static void floppy_shutdown(struct work_struct *arg) if (initialized) DPRINT("floppy timeout called\n"); - FDCS->reset = 1; + fdc_state[fdc].reset = 1; if (cont) { cont->done(0); cont->redo(); /* this will recall reset when needed */ @@ -1870,7 +1869,7 @@ static int start_motor(void (*function)(void)) mask = 0xfc; data = UNIT(current_drive); if (!(raw_cmd->flags & FD_RAW_NO_MOTOR)) { - if (!(FDCS->dor & (0x10 << UNIT(current_drive)))) { + if (!(fdc_state[fdc].dor & (0x10 << UNIT(current_drive)))) { set_debugt(); /* no read since this drive is running */ DRS->first_read_date = 0; @@ -1878,7 +1877,7 @@ static int start_motor(void (*function)(void)) DRS->spinup_date = jiffies; data |= (0x10 << UNIT(current_drive)); } - } else if (FDCS->dor & (0x10 << UNIT(current_drive))) + } else if (fdc_state[fdc].dor & (0x10 << UNIT(current_drive))) mask &= ~(0x10 << UNIT(current_drive)); /* starts motor and selects floppy */ @@ -1892,7 +1891,7 @@ static int start_motor(void (*function)(void)) static void floppy_ready(void) { - if (FDCS->reset) { + if (fdc_state[fdc].reset) { reset_fdc(); return; } @@ -1991,7 +1990,7 @@ static int wait_til_done(void (*handler)(void), bool interruptible) return -EINTR; } - if (FDCS->reset) + if (fdc_state[fdc].reset) command_status = FD_COMMAND_ERROR; if (command_status == FD_COMMAND_OKAY) ret = 0; @@ -2060,7 +2059,7 @@ static void bad_flp_intr(void) if (err_count > DP->max_errors.abort) cont->done(0); if (err_count > DP->max_errors.reset) - FDCS->reset = 1; + fdc_state[fdc].reset = 1; else if (err_count > DP->max_errors.recal) DRS->track = NEED_2_RECAL; } @@ -2967,8 +2966,8 @@ static int user_reset_fdc(int drive, int arg, bool interruptible) return -EINTR; if (arg == FD_RESET_ALWAYS) - FDCS->reset = 1; - if (FDCS->reset) { + fdc_state[fdc].reset = 1; + if (fdc_state[fdc].reset) { cont = &reset_cont; ret = wait_til_done(reset_fdc, interruptible); if (ret == -EINTR) @@ -3179,23 +3178,23 @@ static int raw_cmd_ioctl(int cmd, void __user *param) int ret2; int ret; - if (FDCS->rawcmd <= 1) - FDCS->rawcmd = 1; + if (fdc_state[fdc].rawcmd <= 1) + fdc_state[fdc].rawcmd = 1; for (drive = 0; drive < N_DRIVE; drive++) { if (FDC(drive) != fdc) continue; if (drive == current_drive) { if (UDRS->fd_ref > 1) { - FDCS->rawcmd = 2; + fdc_state[fdc].rawcmd = 2; break; } } else if (UDRS->fd_ref) { - FDCS->rawcmd = 2; + fdc_state[fdc].rawcmd = 2; break; } } - if (FDCS->reset) + if (fdc_state[fdc].reset) return -EIO; ret = raw_cmd_copyin(cmd, param, &my_raw_cmd); @@ -3209,7 +3208,7 @@ static int raw_cmd_ioctl(int cmd, void __user *param) ret = wait_til_done(floppy_start, true); debug_dcl(DP->flags, "calling disk change from raw_cmd ioctl\n"); - if (ret != -EINTR && FDCS->reset) + if (ret != -EINTR && fdc_state[fdc].reset) ret = -EIO; DRS->track = NO_TRACK; @@ -4261,7 +4260,7 @@ static char __init get_fdc_version(void) int r; output_byte(FD_DUMPREGS); /* 82072 and better know DUMPREGS */ - if (FDCS->reset) + if (fdc_state[fdc].reset) return FDC_NONE; r = result(); if (r <= 0x00) @@ -4494,7 +4493,7 @@ static int floppy_resume(struct device *dev) int fdc; for (fdc = 0; fdc < N_FDC; fdc++) - if (FDCS->address != -1) + if (fdc_state[fdc].address != -1) user_reset_fdc(-1, FD_RESET_ALWAYS, false); return 0; @@ -4605,15 +4604,15 @@ static int __init do_floppy_init(void) for (i = 0; i < N_FDC; i++) { fdc = i; - memset(FDCS, 0, sizeof(*FDCS)); - FDCS->dtr = -1; - FDCS->dor = 0x4; + memset(&fdc_state[fdc], 0, sizeof(*fdc_state)); + fdc_state[fdc].dtr = -1; + fdc_state[fdc].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 - FDCS->version = FDC_82072A; + fdc_state[fdc].version = FDC_82072A; #endif } @@ -4656,28 +4655,28 @@ static int __init do_floppy_init(void) for (i = 0; i < N_FDC; i++) { fdc = i; - FDCS->driver_version = FD_DRIVER_VERSION; + fdc_state[fdc].driver_version = FD_DRIVER_VERSION; for (unit = 0; unit < 4; unit++) - FDCS->track[unit] = 0; - if (FDCS->address == -1) + fdc_state[fdc].track[unit] = 0; + if (fdc_state[fdc].address == -1) continue; - FDCS->rawcmd = 2; + fdc_state[fdc].rawcmd = 2; if (user_reset_fdc(-1, FD_RESET_ALWAYS, false)) { /* free ioports reserved by floppy_grab_irq_and_dma() */ floppy_release_regions(fdc); - FDCS->address = -1; - FDCS->version = FDC_NONE; + fdc_state[fdc].address = -1; + fdc_state[fdc].version = FDC_NONE; continue; } /* Try to determine the floppy controller type */ - FDCS->version = get_fdc_version(); - if (FDCS->version == FDC_NONE) { + fdc_state[fdc].version = get_fdc_version(); + if (fdc_state[fdc].version == FDC_NONE) { /* free ioports reserved by floppy_grab_irq_and_dma() */ floppy_release_regions(fdc); - FDCS->address = -1; + fdc_state[fdc].address = -1; continue; } - if (can_use_virtual_dma == 2 && FDCS->version < FDC_82072A) + if (can_use_virtual_dma == 2 && fdc_state[fdc].version < FDC_82072A) can_use_virtual_dma = 0; have_no_fdc = 0; @@ -4783,7 +4782,7 @@ static void floppy_release_allocated_regions(int fdc, const struct io_region *p) { while (p != io_regions) { p--; - release_region(FDCS->address + p->offset, p->size); + release_region(fdc_state[fdc].address + p->offset, p->size); } } @@ -4794,10 +4793,10 @@ static int floppy_request_regions(int fdc) const struct io_region *p; for (p = io_regions; p < ARRAY_END(io_regions); p++) { - if (!request_region(FDCS->address + p->offset, + if (!request_region(fdc_state[fdc].address + p->offset, p->size, "floppy")) { DPRINT("Floppy io-port 0x%04lx in use\n", - FDCS->address + p->offset); + fdc_state[fdc].address + p->offset); floppy_release_allocated_regions(fdc, p); return -EBUSY; } @@ -4840,23 +4839,23 @@ static int floppy_grab_irq_and_dma(void) } for (fdc = 0; fdc < N_FDC; fdc++) { - if (FDCS->address != -1) { + if (fdc_state[fdc].address != -1) { if (floppy_request_regions(fdc)) goto cleanup; } } for (fdc = 0; fdc < N_FDC; fdc++) { - if (FDCS->address != -1) { + if (fdc_state[fdc].address != -1) { reset_fdc_info(1); - fd_outb(FDCS->dor, FD_DOR); + fd_outb(fdc_state[fdc].dor, FD_DOR); } } fdc = 0; set_dor(0, ~0, 8); /* avoid immediate interrupt */ for (fdc = 0; fdc < N_FDC; fdc++) - if (FDCS->address != -1) - fd_outb(FDCS->dor, FD_DOR); + if (fdc_state[fdc].address != -1) + fd_outb(fdc_state[fdc].dor, FD_DOR); /* * The driver will try and free resources and relies on us * to know if they were allocated or not. @@ -4918,7 +4917,7 @@ static void floppy_release_irq_and_dma(void) pr_info("work still pending\n"); old_fdc = fdc; for (fdc = 0; fdc < N_FDC; fdc++) - if (FDCS->address != -1) + if (fdc_state[fdc].address != -1) floppy_release_regions(fdc); fdc = old_fdc; }
Macro FDCS silently uses identifier "fdc" which may be either the global one or a local one. Let's expand the macro to make this more obvious. Signed-off-by: Willy Tarreau <w@1wt.eu> --- drivers/block/floppy.c | 183 ++++++++++++++++++++++++------------------------- 1 file changed, 91 insertions(+), 92 deletions(-)