This series applies a second batch of cleanups to the floppy driver and
its multiple arch-specific parts. Here the focus was on getting rid of
hard-coded registers and flags values to switch to their symbolic
definitions instead, and on making use of the global current_fdc variable
much more explicit throughout the code to reduce the risk of accidental
misuse as was the case with the most recently fixed bug.
Note that this code base is very old and the purpose is not to rewrite
nor reorganize the driver at all, but instead to make certain things
more obvious while keeping changes reviewable. It does not even address
style issues that make checkpatch continue to complain a little bit (15
total warnings which were already there and don't seem worth addressing
without more careful testing). Some comments were added to document a
few non-obvious assumptions though.
This series was rediffed against today's master (458ef2a25e0c) which
contains the first series. The changes were tested on x86 with real
hardware, and was build-tested on ARM.
Willy Tarreau (23):
floppy: split the base port from the register in I/O accesses
floppy: add references to 82077's extra registers
floppy: use symbolic register names in the m68k port
floppy: use symbolic register names in the parisc port
floppy: use symbolic register names in the powerpc port
floppy: use symbolic register names in the sparc32 port
floppy: use symbolic register names in the sparc64 port
floppy: use symbolic register names in the x86 port
floppy: cleanup: make twaddle() not rely on current_{fdc,drive}
anymore
floppy: cleanup: make reset_fdc_info() not rely on current_fdc anymore
floppy: cleanup: make show_floppy() not rely on current_fdc anymore
floppy: cleanup: make wait_til_ready() not rely on current_fdc anymore
floppy: cleanup: make output_byte() not rely on current_fdc anymore
floppy: cleanup: make result() not rely on current_fdc anymore
floppy: cleanup: make need_more_output() not rely on current_fdc
anymore
floppy: cleanup: make perpendicular_mode() not rely on current_fdc
anymore
floppy: cleanup: make fdc_configure() not rely on current_fdc anymore
floppy: cleanup: make fdc_specify() not rely on current_{fdc,drive}
anymore
floppy: cleanup: make check_wp() not rely on current_{fdc,drive}
anymore
floppy: cleanup: make next_valid_format() not rely on current_drive
anymore
floppy: cleanup: make get_fdc_version() not rely on current_fdc
anymore
floppy: cleanup: do not iterate on current_fdc in DMA grab/release
functions
floppy: cleanup: add a few comments about expectations in certain
functions
arch/alpha/include/asm/floppy.h | 4 +-
arch/arm/include/asm/floppy.h | 8 +-
arch/m68k/include/asm/floppy.h | 27 +-
arch/mips/include/asm/mach-generic/floppy.h | 8 +-
arch/mips/include/asm/mach-jazz/floppy.h | 8 +-
arch/parisc/include/asm/floppy.h | 19 +-
arch/powerpc/include/asm/floppy.h | 19 +-
arch/sparc/include/asm/floppy_32.h | 50 +--
arch/sparc/include/asm/floppy_64.h | 59 ++--
arch/x86/include/asm/floppy.h | 19 +-
drivers/block/floppy.c | 330 ++++++++++----------
include/uapi/linux/fdreg.h | 16 +-
12 files changed, 299 insertions(+), 268 deletions(-)
Cc: "David S. Miller" <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Ian Molton <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Russell King <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: [email protected]
--
2.20.1
Now the fdc is passed in argument so that the function does not
use current_fdc anymore.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index dd739594fce7..5dfddd4726fb 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -1107,30 +1107,30 @@ static void setup_DMA(void)
static void show_floppy(int fdc);
/* waits until the fdc becomes ready */
-static int wait_til_ready(void)
+static int wait_til_ready(int fdc)
{
int status;
int counter;
- if (fdc_state[current_fdc].reset)
+ if (fdc_state[fdc].reset)
return -1;
for (counter = 0; counter < 10000; counter++) {
- status = fdc_inb(current_fdc, FD_STATUS);
+ status = fdc_inb(fdc, FD_STATUS);
if (status & STATUS_READY)
return status;
}
if (initialized) {
- DPRINT("Getstatus times out (%x) on fdc %d\n", status, current_fdc);
- show_floppy(current_fdc);
+ DPRINT("Getstatus times out (%x) on fdc %d\n", status, fdc);
+ show_floppy(fdc);
}
- fdc_state[current_fdc].reset = 1;
+ fdc_state[fdc].reset = 1;
return -1;
}
/* sends a command byte to the fdc */
static int output_byte(char byte)
{
- int status = wait_til_ready();
+ int status = wait_til_ready(current_fdc);
if (status < 0)
return -1;
@@ -1159,7 +1159,7 @@ static int result(void)
int status = 0;
for (i = 0; i < MAX_REPLIES; i++) {
- status = wait_til_ready();
+ status = wait_til_ready(current_fdc);
if (status < 0)
break;
status &= STATUS_DIR | STATUS_READY | STATUS_BUSY | STATUS_DMA;
@@ -1186,7 +1186,7 @@ static int result(void)
/* does the fdc need more output? */
static int need_more_output(void)
{
- int status = wait_til_ready();
+ int status = wait_til_ready(current_fdc);
if (status < 0)
return -1;
--
2.20.1
The locking in the driver is far from being obvious, with unlocking
automatically happening at end of operations scheduled by interrupt,
especially for the error paths where one does not necessarily expect
that such an interrupt may be triggered. Let's add a few comments
about what to expect at certain places to avoid misdetecting bugs
which are not.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 77bb9a5fcd33..07218f8b17f9 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -1791,7 +1791,9 @@ static void reset_interrupt(void)
/*
* reset is done by pulling bit 2 of DOR low for a while (old FDCs),
- * or by setting the self clearing bit 7 of STATUS (newer FDCs)
+ * or by setting the self clearing bit 7 of STATUS (newer FDCs).
+ * This WILL trigger an interrupt, causing the handlers in the current
+ * cont's ->redo() to be called via reset_interrupt().
*/
static void reset_fdc(void)
{
@@ -2003,6 +2005,9 @@ static const struct cont_t intr_cont = {
.done = (done_f)empty
};
+/* schedules handler, waiting for completion. May be interrupted, will then
+ * return -EINTR, in which case the driver will automatically be unlocked.
+ */
static int wait_til_done(void (*handler)(void), bool interruptible)
{
int ret;
@@ -2842,6 +2847,9 @@ static int set_next_request(void)
return current_req != NULL;
}
+/* Starts or continues processing request. Will automatically unlock the
+ * driver at end of request.
+ */
static void redo_fd_request(void)
{
int drive;
@@ -2916,6 +2924,7 @@ static const struct cont_t rw_cont = {
.done = request_done
};
+/* schedule the request and automatically unlock the driver on completion */
static void process_fd_request(void)
{
cont = &rw_cont;
@@ -3005,6 +3014,9 @@ static int user_reset_fdc(int drive, int arg, bool interruptible)
if (arg == FD_RESET_ALWAYS)
fdc_state[current_fdc].reset = 1;
if (fdc_state[current_fdc].reset) {
+ /* note: reset_fdc will take care of unlocking the driver
+ * on completion.
+ */
cont = &reset_cont;
ret = wait_til_done(reset_fdc, interruptible);
if (ret == -EINTR)
--
2.20.1
Now the fdc is passed in argument so that the function does not
use current_fdc anymore.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 64 +++++++++++++++++++++---------------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 5dfddd4726fb..81fd06eaea7d 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -1128,26 +1128,26 @@ static int wait_til_ready(int fdc)
}
/* sends a command byte to the fdc */
-static int output_byte(char byte)
+static int output_byte(int fdc, char byte)
{
- int status = wait_til_ready(current_fdc);
+ int status = wait_til_ready(fdc);
if (status < 0)
return -1;
if (is_ready_state(status)) {
- fdc_outb(byte, current_fdc, FD_DATA);
+ fdc_outb(byte, fdc, FD_DATA);
output_log[output_log_pos].data = byte;
output_log[output_log_pos].status = status;
output_log[output_log_pos].jiffies = jiffies;
output_log_pos = (output_log_pos + 1) % OLOGSIZE;
return 0;
}
- fdc_state[current_fdc].reset = 1;
+ fdc_state[fdc].reset = 1;
if (initialized) {
DPRINT("Unable to send byte %x to FDC. Fdc=%x Status=%x\n",
- byte, current_fdc, status);
- show_floppy(current_fdc);
+ byte, fdc, status);
+ show_floppy(fdc);
}
return -1;
}
@@ -1229,8 +1229,8 @@ static void perpendicular_mode(void)
if (fdc_state[current_fdc].perp_mode == perp_mode)
return;
if (fdc_state[current_fdc].version >= FDC_82077_ORIG) {
- output_byte(FD_PERPENDICULAR);
- output_byte(perp_mode);
+ output_byte(current_fdc, FD_PERPENDICULAR);
+ output_byte(current_fdc, perp_mode);
fdc_state[current_fdc].perp_mode = perp_mode;
} else if (perp_mode) {
DPRINT("perpendicular mode not supported by this FDC.\n");
@@ -1243,12 +1243,12 @@ static int no_fifo;
static int fdc_configure(void)
{
/* Turn on FIFO */
- output_byte(FD_CONFIGURE);
+ output_byte(current_fdc, FD_CONFIGURE);
if (need_more_output() != MORE_OUTPUT)
return 0;
- output_byte(0);
- output_byte(0x10 | (no_fifo & 0x20) | (fifo_depth & 0xf));
- output_byte(0); /* pre-compensation from track
+ output_byte(current_fdc, 0);
+ output_byte(current_fdc, 0x10 | (no_fifo & 0x20) | (fifo_depth & 0xf));
+ output_byte(current_fdc, 0); /* pre-compensation from track
0 upwards */
return 1;
}
@@ -1301,10 +1301,10 @@ static void fdc_specify(void)
if (fdc_state[current_fdc].version >= FDC_82078) {
/* chose the default rate table, not the one
* where 1 = 2 Mbps */
- output_byte(FD_DRIVESPEC);
+ output_byte(current_fdc, FD_DRIVESPEC);
if (need_more_output() == MORE_OUTPUT) {
- output_byte(UNIT(current_drive));
- output_byte(0xc0);
+ output_byte(current_fdc, UNIT(current_drive));
+ output_byte(current_fdc, 0xc0);
}
}
break;
@@ -1349,9 +1349,9 @@ static void fdc_specify(void)
if (fdc_state[current_fdc].spec1 != spec1 ||
fdc_state[current_fdc].spec2 != spec2) {
/* Go ahead and set spec1 and spec2 */
- output_byte(FD_SPECIFY);
- output_byte(fdc_state[current_fdc].spec1 = spec1);
- output_byte(fdc_state[current_fdc].spec2 = spec2);
+ output_byte(current_fdc, FD_SPECIFY);
+ output_byte(current_fdc, fdc_state[current_fdc].spec1 = spec1);
+ output_byte(current_fdc, fdc_state[current_fdc].spec2 = spec2);
}
} /* fdc_specify */
@@ -1513,7 +1513,7 @@ static void setup_rw_floppy(void)
r = 0;
for (i = 0; i < raw_cmd->cmd_count; i++)
- r |= output_byte(raw_cmd->cmd[i]);
+ r |= output_byte(current_fdc, raw_cmd->cmd[i]);
debugt(__func__, "rw_command");
@@ -1566,8 +1566,8 @@ static void check_wp(void)
{
if (test_bit(FD_VERIFY_BIT, &drive_state[current_drive].flags)) {
/* check write protection */
- output_byte(FD_GETSTATUS);
- output_byte(UNIT(current_drive));
+ output_byte(current_fdc, FD_GETSTATUS);
+ output_byte(current_fdc, UNIT(current_drive));
if (result() != 1) {
fdc_state[current_fdc].reset = 1;
return;
@@ -1639,9 +1639,9 @@ static void seek_floppy(void)
}
do_floppy = seek_interrupt;
- output_byte(FD_SEEK);
- output_byte(UNIT(current_drive));
- if (output_byte(track) < 0) {
+ output_byte(current_fdc, FD_SEEK);
+ output_byte(current_fdc, UNIT(current_drive));
+ if (output_byte(current_fdc, track) < 0) {
reset_fdc();
return;
}
@@ -1748,7 +1748,7 @@ irqreturn_t floppy_interrupt(int irq, void *dev_id)
if (inr == 0) {
int max_sensei = 4;
do {
- output_byte(FD_SENSEI);
+ output_byte(current_fdc, FD_SENSEI);
inr = result();
if (do_print)
print_result("sensei", inr);
@@ -1771,8 +1771,8 @@ static void recalibrate_floppy(void)
{
debugt(__func__, "");
do_floppy = recal_interrupt;
- output_byte(FD_RECALIBRATE);
- if (output_byte(UNIT(current_drive)) < 0)
+ output_byte(current_fdc, FD_RECALIBRATE);
+ if (output_byte(current_fdc, UNIT(current_drive)) < 0)
reset_fdc();
}
@@ -4302,7 +4302,7 @@ static char __init get_fdc_version(void)
{
int r;
- output_byte(FD_DUMPREGS); /* 82072 and better know DUMPREGS */
+ output_byte(current_fdc, FD_DUMPREGS); /* 82072 and better know DUMPREGS */
if (fdc_state[current_fdc].reset)
return FDC_NONE;
r = result();
@@ -4323,15 +4323,15 @@ static char __init get_fdc_version(void)
return FDC_82072; /* 82072 doesn't know CONFIGURE */
}
- output_byte(FD_PERPENDICULAR);
+ output_byte(current_fdc, FD_PERPENDICULAR);
if (need_more_output() == MORE_OUTPUT) {
- output_byte(0);
+ output_byte(current_fdc, 0);
} else {
pr_info("FDC %d is an 82072A\n", current_fdc);
return FDC_82072A; /* 82072A as found on Sparcs. */
}
- output_byte(FD_UNLOCK);
+ output_byte(current_fdc, FD_UNLOCK);
r = result();
if ((r == 1) && (reply_buffer[0] == 0x80)) {
pr_info("FDC %d is a pre-1991 82077\n", current_fdc);
@@ -4343,7 +4343,7 @@ static char __init get_fdc_version(void)
current_fdc, r);
return FDC_UNKNOWN;
}
- output_byte(FD_PARTID);
+ output_byte(current_fdc, FD_PARTID);
r = result();
if (r != 1) {
pr_info("FDC %d init: PARTID: unexpected return of %d bytes.\n",
--
2.20.1
Currently we have architecture-specific fd_inb() and fd_outb() functions
or macros, taking just a port which is in fact made of a base address and
a register. The base address is FDC-specific and derived from the local or
global "fdc" variable through the FD_IOPORT macro used in the base address
calculation.
This change splits this by explicitly passing the FDC's base address and
the register separately to fd_outb() and fd_inb(). It affects the
following archs:
- x86, alpha, mips, powerpc, parisc, arm, m68k:
simple remap of port -> base+reg
- sparc32: use of reg only, since the base address was already masked
out and the FDC controller is known from a static struct.
- sparc64: like x86 for PCI, like sparc32 for 82077
Some archs use inline functions and others macros. This was not
unified in order to minimize the number of changes to review. For the
same reason checkpatch still spews a few warnings about things that
were already there before.
The parisc still uses hard-coded register values and could be cleaned up
by taking the register definitions.
The sparc per-controller inb/outb functions could further be refined
to explicitly take an FDC register instead of a port in argument but it
was not needed yet and may be cleaned later.
Cc: Ivan Kokshaysky <[email protected]>
Cc: Richard Henderson <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Ian Molton <[email protected]>
Cc: Russell King <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>
Cc: Thomas Bogendoerfer <[email protected]>
Cc: Helge Deller <[email protected]>
Cc: Benjamin Herrenschmidt <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Signed-off-by: Willy Tarreau <[email protected]>
---
arch/alpha/include/asm/floppy.h | 4 ++--
arch/arm/include/asm/floppy.h | 8 ++++----
arch/m68k/include/asm/floppy.h | 12 ++++++------
arch/mips/include/asm/mach-generic/floppy.h | 8 ++++----
arch/mips/include/asm/mach-jazz/floppy.h | 8 ++++----
arch/parisc/include/asm/floppy.h | 12 ++++++------
arch/powerpc/include/asm/floppy.h | 4 ++--
arch/sparc/include/asm/floppy_32.h | 4 ++--
arch/sparc/include/asm/floppy_64.h | 4 ++--
arch/x86/include/asm/floppy.h | 4 ++--
drivers/block/floppy.c | 4 ++--
11 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/arch/alpha/include/asm/floppy.h b/arch/alpha/include/asm/floppy.h
index 942924756cf2..8dfdb3aa1d96 100644
--- a/arch/alpha/include/asm/floppy.h
+++ b/arch/alpha/include/asm/floppy.h
@@ -11,8 +11,8 @@
#define __ASM_ALPHA_FLOPPY_H
-#define fd_inb(port) inb_p(port)
-#define fd_outb(value,port) outb_p(value,port)
+#define fd_inb(base, reg) inb_p((base) + (reg))
+#define fd_outb(value, base, reg) outb_p(value, (base) + (reg))
#define fd_enable_dma() enable_dma(FLOPPY_DMA)
#define fd_disable_dma() disable_dma(FLOPPY_DMA)
diff --git a/arch/arm/include/asm/floppy.h b/arch/arm/include/asm/floppy.h
index 79fa327238e8..e1cb04ed5008 100644
--- a/arch/arm/include/asm/floppy.h
+++ b/arch/arm/include/asm/floppy.h
@@ -9,20 +9,20 @@
#ifndef __ASM_ARM_FLOPPY_H
#define __ASM_ARM_FLOPPY_H
-#define fd_outb(val,port) \
+#define fd_outb(val, base, reg) \
do { \
int new_val = (val); \
- if (((port) & 7) == FD_DOR) { \
+ if ((reg) == FD_DOR) { \
if (new_val & 0xf0) \
new_val = (new_val & 0x0c) | \
floppy_selects[new_val & 3]; \
else \
new_val &= 0x0c; \
} \
- outb(new_val, (port)); \
+ outb(new_val, (base) + (reg)); \
} while(0)
-#define fd_inb(port) inb((port))
+#define fd_inb(base, reg) inb((base) + (reg))
#define fd_request_irq() request_irq(IRQ_FLOPPYDISK,floppy_interrupt,\
0,"floppy",NULL)
#define fd_free_irq() free_irq(IRQ_FLOPPYDISK,NULL)
diff --git a/arch/m68k/include/asm/floppy.h b/arch/m68k/include/asm/floppy.h
index c3b9ad6732fc..2a6ce29b92aa 100644
--- a/arch/m68k/include/asm/floppy.h
+++ b/arch/m68k/include/asm/floppy.h
@@ -63,21 +63,21 @@ static __inline__ void release_dma_lock(unsigned long flags)
}
-static __inline__ unsigned char fd_inb(int port)
+static __inline__ unsigned char fd_inb(int base, int reg)
{
if(MACH_IS_Q40)
- return inb_p(port);
+ return inb_p(base + reg);
else if(MACH_IS_SUN3X)
- return sun3x_82072_fd_inb(port);
+ return sun3x_82072_fd_inb(base + reg);
return 0;
}
-static __inline__ void fd_outb(unsigned char value, int port)
+static __inline__ void fd_outb(unsigned char value, int base, int reg)
{
if(MACH_IS_Q40)
- outb_p(value, port);
+ outb_p(value, base + reg);
else if(MACH_IS_SUN3X)
- sun3x_82072_fd_outb(value, port);
+ sun3x_82072_fd_outb(value, base + reg);
}
diff --git a/arch/mips/include/asm/mach-generic/floppy.h b/arch/mips/include/asm/mach-generic/floppy.h
index 9ec2f6a5200b..e3f446d54827 100644
--- a/arch/mips/include/asm/mach-generic/floppy.h
+++ b/arch/mips/include/asm/mach-generic/floppy.h
@@ -26,14 +26,14 @@
/*
* How to access the FDC's registers.
*/
-static inline unsigned char fd_inb(unsigned int port)
+static inline unsigned char fd_inb(unsigned int base, unsigned int reg)
{
- return inb_p(port);
+ return inb_p(base + reg);
}
-static inline void fd_outb(unsigned char value, unsigned int port)
+static inline void fd_outb(unsigned char value, unsigned int base, unsigned int reg)
{
- outb_p(value, port);
+ outb_p(value, base + reg);
}
/*
diff --git a/arch/mips/include/asm/mach-jazz/floppy.h b/arch/mips/include/asm/mach-jazz/floppy.h
index 4b86c88a03b7..095000c290e5 100644
--- a/arch/mips/include/asm/mach-jazz/floppy.h
+++ b/arch/mips/include/asm/mach-jazz/floppy.h
@@ -17,19 +17,19 @@
#include <asm/jazzdma.h>
#include <asm/pgtable.h>
-static inline unsigned char fd_inb(unsigned int port)
+static inline unsigned char fd_inb(unsigned int base, unsigned int reg)
{
unsigned char c;
- c = *(volatile unsigned char *) port;
+ c = *(volatile unsigned char *) (base + reg);
udelay(1);
return c;
}
-static inline void fd_outb(unsigned char value, unsigned int port)
+static inline void fd_outb(unsigned char value, unsigned int base, unsigned int reg)
{
- *(volatile unsigned char *) port = value;
+ *(volatile unsigned char *) (base + reg) = value;
}
/*
diff --git a/arch/parisc/include/asm/floppy.h b/arch/parisc/include/asm/floppy.h
index 09b6f4c1687e..1aebc23b7744 100644
--- a/arch/parisc/include/asm/floppy.h
+++ b/arch/parisc/include/asm/floppy.h
@@ -29,8 +29,8 @@
#define CSW fd_routine[can_use_virtual_dma & 1]
-#define fd_inb(port) readb(port)
-#define fd_outb(value, port) writeb(value, port)
+#define fd_inb(base, reg) readb((base) + (reg))
+#define fd_outb(value, base, reg) writeb(value, (base) + (reg))
#define fd_request_dma() CSW._request_dma(FLOPPY_DMA,"floppy")
#define fd_free_dma() CSW._free_dma(FLOPPY_DMA)
@@ -75,19 +75,19 @@ static void floppy_hardint(int irq, void *dev_id, struct pt_regs * regs)
register char *lptr = virtual_dma_addr;
for (lcount = virtual_dma_count; lcount; lcount--) {
- st = fd_inb(virtual_dma_port+4) & 0xa0 ;
+ st = fd_inb(virtual_dma_port, 4) & 0xa0;
if (st != 0xa0)
break;
if (virtual_dma_mode) {
- fd_outb(*lptr, virtual_dma_port+5);
+ fd_outb(*lptr, virtual_dma_port, 5);
} else {
- *lptr = fd_inb(virtual_dma_port+5);
+ *lptr = fd_inb(virtual_dma_port, 5);
}
lptr++;
}
virtual_dma_count = lcount;
virtual_dma_addr = lptr;
- st = fd_inb(virtual_dma_port+4);
+ st = fd_inb(virtual_dma_port, 4);
}
#ifdef TRACE_FLPY_INT
diff --git a/arch/powerpc/include/asm/floppy.h b/arch/powerpc/include/asm/floppy.h
index 167c44b58848..ed467eb0c4c8 100644
--- a/arch/powerpc/include/asm/floppy.h
+++ b/arch/powerpc/include/asm/floppy.h
@@ -13,8 +13,8 @@
#include <asm/machdep.h>
-#define fd_inb(port) inb_p(port)
-#define fd_outb(value,port) outb_p(value,port)
+#define fd_inb(base, reg) inb_p((base) + (reg))
+#define fd_outb(value, base, reg) outb_p(value, (base) + (reg))
#define fd_enable_dma() enable_dma(FLOPPY_DMA)
#define fd_disable_dma() fd_ops->_disable_dma(FLOPPY_DMA)
diff --git a/arch/sparc/include/asm/floppy_32.h b/arch/sparc/include/asm/floppy_32.h
index b519acf4383d..4d08df4f4deb 100644
--- a/arch/sparc/include/asm/floppy_32.h
+++ b/arch/sparc/include/asm/floppy_32.h
@@ -59,8 +59,8 @@ struct sun_floppy_ops {
static struct sun_floppy_ops sun_fdops;
-#define fd_inb(port) sun_fdops.fd_inb(port)
-#define fd_outb(value,port) sun_fdops.fd_outb(value,port)
+#define fd_inb(base, reg) sun_fdops.fd_inb(reg)
+#define fd_outb(value, base, reg) sun_fdops.fd_outb(value, reg)
#define fd_enable_dma() sun_fd_enable_dma()
#define fd_disable_dma() sun_fd_disable_dma()
#define fd_request_dma() (0) /* nothing... */
diff --git a/arch/sparc/include/asm/floppy_64.h b/arch/sparc/include/asm/floppy_64.h
index 3729fc35ba83..c0cf157e5b15 100644
--- a/arch/sparc/include/asm/floppy_64.h
+++ b/arch/sparc/include/asm/floppy_64.h
@@ -62,8 +62,8 @@ struct sun_floppy_ops {
static struct sun_floppy_ops sun_fdops;
-#define fd_inb(port) sun_fdops.fd_inb(port)
-#define fd_outb(value,port) sun_fdops.fd_outb(value,port)
+#define fd_inb(base, reg) sun_fdops.fd_inb((base) + (reg))
+#define fd_outb(value, base, reg) sun_fdops.fd_outb(value, (base) + (reg))
#define fd_enable_dma() sun_fdops.fd_enable_dma()
#define fd_disable_dma() sun_fdops.fd_disable_dma()
#define fd_request_dma() (0) /* nothing... */
diff --git a/arch/x86/include/asm/floppy.h b/arch/x86/include/asm/floppy.h
index 7ec59edde154..20088cb08f5e 100644
--- a/arch/x86/include/asm/floppy.h
+++ b/arch/x86/include/asm/floppy.h
@@ -31,8 +31,8 @@
#define CSW fd_routine[can_use_virtual_dma & 1]
-#define fd_inb(port) inb_p(port)
-#define fd_outb(value, port) outb_p(value, port)
+#define fd_inb(base, reg) inb_p((base) + (reg))
+#define fd_outb(value, base, reg) outb_p(value, (base) + (reg))
#define fd_request_dma() CSW._request_dma(FLOPPY_DMA, "floppy")
#define fd_free_dma() CSW._free_dma(FLOPPY_DMA)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c3daa64cb52c..1cda39098b07 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -595,12 +595,12 @@ static unsigned char in_sector_offset; /* offset within physical sector,
static inline unsigned char fdc_inb(int fdc, int reg)
{
- return fd_inb(fdc_state[fdc].address + reg);
+ return fd_inb(fdc_state[fdc].address, reg);
}
static inline void fdc_outb(unsigned char value, int fdc, int reg)
{
- fd_outb(value, fdc_state[fdc].address + reg);
+ fd_outb(value, fdc_state[fdc].address, reg);
}
static inline bool drive_no_geom(int drive)
--
2.20.1
Now the drive is passed in argument so that the function does not
use current_drive anymore.
Signed-off-by: Willy Tarreau <[email protected]>
---
drivers/block/floppy.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index b9a3a04c2636..f53810ba486d 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -2058,18 +2058,18 @@ static void success_and_wakeup(void)
* ==========================
*/
-static int next_valid_format(void)
+static int next_valid_format(int drive)
{
int probed_format;
- probed_format = drive_state[current_drive].probed_format;
+ probed_format = drive_state[drive].probed_format;
while (1) {
- if (probed_format >= 8 || !drive_params[current_drive].autodetect[probed_format]) {
- drive_state[current_drive].probed_format = 0;
+ if (probed_format >= 8 || !drive_params[drive].autodetect[probed_format]) {
+ drive_state[drive].probed_format = 0;
return 1;
}
- if (floppy_type[drive_params[current_drive].autodetect[probed_format]].sect) {
- drive_state[current_drive].probed_format = probed_format;
+ if (floppy_type[drive_params[drive].autodetect[probed_format]].sect) {
+ drive_state[drive].probed_format = probed_format;
return 0;
}
probed_format++;
@@ -2082,7 +2082,7 @@ static void bad_flp_intr(void)
if (probing) {
drive_state[current_drive].probed_format++;
- if (!next_valid_format())
+ if (!next_valid_format(current_drive))
return;
}
err_count = ++(*errors);
@@ -2884,7 +2884,7 @@ static void redo_fd_request(void)
if (!_floppy) { /* Autodetection */
if (!probing) {
drive_state[current_drive].probed_format = 0;
- if (next_valid_format()) {
+ if (next_valid_format(current_drive)) {
DPRINT("no autodetectable formats\n");
_floppy = NULL;
request_done(0);
--
2.20.1
Hi Willy,
given that you are actively maintaining the floppy driver now, any
chance I could trick you into proper highmem handling? I've been trying
to phase out block layer bounce buffering, and any help from a competent
maintainer to move their drivers to properly support highmem by kmapping
for PIO/MMIO I/O would be very helpful.
Hi Christoph,
On Tue, Mar 31, 2020 at 03:10:19AM -0700, Christoph Hellwig wrote:
> Hi Willy,
>
> given that you are actively maintaining the floppy driver now,
No no no I'm not! Denis is :-) Really, I mean I just proposed some help
to clean up this mess after being tricked into not believing a bug report
just because the code was too confusing.
> any
> chance I could trick you into proper highmem handling? I've been trying
> to phase out block layer bounce buffering, and any help from a competent
> maintainer to move their drivers to properly support highmem by kmapping
> for PIO/MMIO I/O would be very helpful.
I'm not sure what this implies regarding this code, to be honest. It's
very tricky and implements sort of a state machine using function pointers
within its interrupt handler so you never know exactly what accesses what,
and quite a part of it remains obscure to me :-/ I can accept to help, I
can even run tests since I still have running hardware, but I'd at least
need some guidance. And probably Denis would know better than me there.
Also I doubt we'd get sufficient testing on less common archs. While I
do have sparc64/parisc/alpha available, I haven't booted a recent kernel
on any of them for a while (2.4 used to be the last ones), and I'm not
sure it's reasonable to go into such changes without proper testing.
What do you think ?
Willy
On Tue, Mar 31, 2020 at 01:01:36PM +0200, Willy Tarreau wrote:
> I'm not sure what this implies regarding this code, to be honest. It's
> very tricky and implements sort of a state machine using function pointers
> within its interrupt handler so you never know exactly what accesses what,
> and quite a part of it remains obscure to me :-/ I can accept to help, I
> can even run tests since I still have running hardware, but I'd at least
> need some guidance. And probably Denis would know better than me there.
> Also I doubt we'd get sufficient testing on less common archs. While I
> do have sparc64/parisc/alpha available, I haven't booted a recent kernel
> on any of them for a while (2.4 used to be the last ones), and I'm not
> sure it's reasonable to go into such changes without proper testing.
The basic change is that instead of using bio_data() or page_address
all pages coming from the block layer need to be properly kmap()ed.
I'll try to cook something up an will send it to Denis and you for
review and testing. The code things about sparc64/parisc/alpha is
that they all don't have highmem, so these changes should be effective
no-ops for them.
On Tue, Mar 31, 2020 at 08:28:12AM -0700, Christoph Hellwig wrote:
> On Tue, Mar 31, 2020 at 01:01:36PM +0200, Willy Tarreau wrote:
> > I'm not sure what this implies regarding this code, to be honest. It's
> > very tricky and implements sort of a state machine using function pointers
> > within its interrupt handler so you never know exactly what accesses what,
> > and quite a part of it remains obscure to me :-/ I can accept to help, I
> > can even run tests since I still have running hardware, but I'd at least
> > need some guidance. And probably Denis would know better than me there.
> > Also I doubt we'd get sufficient testing on less common archs. While I
> > do have sparc64/parisc/alpha available, I haven't booted a recent kernel
> > on any of them for a while (2.4 used to be the last ones), and I'm not
> > sure it's reasonable to go into such changes without proper testing.
>
> The basic change is that instead of using bio_data() or page_address
> all pages coming from the block layer need to be properly kmap()ed.
> I'll try to cook something up an will send it to Denis and you for
> review and testing. The code things about sparc64/parisc/alpha is
> that they all don't have highmem, so these changes should be effective
> no-ops for them.
OK, thanks for the explanation!
Willy
On 3/31/20 3:40 AM, Willy Tarreau wrote:
> This series applies a second batch of cleanups to the floppy driver and
> its multiple arch-specific parts. Here the focus was on getting rid of
> hard-coded registers and flags values to switch to their symbolic
> definitions instead, and on making use of the global current_fdc variable
> much more explicit throughout the code to reduce the risk of accidental
> misuse as was the case with the most recently fixed bug.
>
> Note that this code base is very old and the purpose is not to rewrite
> nor reorganize the driver at all, but instead to make certain things
> more obvious while keeping changes reviewable. It does not even address
> style issues that make checkpatch continue to complain a little bit (15
> total warnings which were already there and don't seem worth addressing
> without more careful testing). Some comments were added to document a
> few non-obvious assumptions though.
>
> This series was rediffed against today's master (458ef2a25e0c) which
> contains the first series. The changes were tested on x86 with real
> hardware, and was build-tested on ARM.
I'll be happy to queue these up for 5.8 when ready. Would be handy
if you could resend a v2 patchset with the extra patches, makes my
life so much easier...
--
Jens Axboe
Hi Jens,
On Mon, Apr 13, 2020 at 04:46:41PM -0600, Jens Axboe wrote:
> I'll be happy to queue these up for 5.8 when ready. Would be handy
> if you could resend a v2 patchset with the extra patches, makes my
> life so much easier...
Sure, will do once Denis confirms he's done with the review and is
OK with the series.
Thanks!
Willy
Hi,
On 4/14/20 8:31 AM, Willy Tarreau wrote:
> Hi Jens,
>
> On Mon, Apr 13, 2020 at 04:46:41PM -0600, Jens Axboe wrote:
>> I'll be happy to queue these up for 5.8 when ready. Would be handy
>> if you could resend a v2 patchset with the extra patches, makes my
>> life so much easier...
>
> Sure, will do once Denis confirms he's done with the review and is
> OK with the series.
>
> Thanks!
> Willy
>
I can see no new issues, respecting that the initial version
was sent privately and additional [24-27] fixups.
[+] eye checked the changes
[+] compile tested the patches on x86, arm, powerpc, sparc64,
m68k (forced ARCH_MAY_HAVE_PC_FDC by removing BROKEN)
sparc64 showed a couple of warnings in printks
I was expecting that some of the arch maintainers will at least
ack the patches.
[+] tested on real hardware for x86
[+] local syzkaller fuzzing reveals no new issues
Willy, could you please resend the patchset with printks fix for sparc64?
Or if Jens don't mind and you don't want to send 30 patches again you can
resend only sparc64 patch and I will reapply it and send everything to Jens
with merge request. I applied your patches a couple of days ago here
https://github.com/evdenis/linux-floppy/ to cleanups branch.
I also faced minor ubsan warning in setup_rw_floppy that is not related
to these patches. It's false alarm of cross-boundary access of cmd,
reply_count, reply in floppy_raw_cmd. This access is intentional.
I will send a patch on top of your patchset.
Thanks,
Denis
Hi Denis,
On Tue, Apr 14, 2020 at 01:29:02PM +0300, Denis Efremov wrote:
> I was expecting that some of the arch maintainers will at least
> ack the patches.
TBH, floppy is probably very low on any arch maintainer's priority list.
> Willy, could you please resend the patchset with printks fix for sparc64?
> Or if Jens don't mind and you don't want to send 30 patches again you can
> resend only sparc64 patch and I will reapply it and send everything to Jens
> with merge request. I applied your patches a couple of days ago here
> https://github.com/evdenis/linux-floppy/ to cleanups branch.
Then I'll redo this one only and directly send it to you as I really hate
spamming innocents with patches. This will also help Jens in that you've
already recomposed the whole series for him.
> I also faced minor ubsan warning in setup_rw_floppy that is not related
> to these patches. It's false alarm of cross-boundary access of cmd,
> reply_count, reply in floppy_raw_cmd. This access is intentional.
> I will send a patch on top of your patchset.
OK, cool!
Thanks!
Willy
Hi,
On 4/14/20 7:12 PM, Willy Tarreau wrote>
> Then I'll redo this one only and directly send it to you as I really hate
> spamming innocents with patches. This will also help Jens in that you've
> already recomposed the whole series for him.
>
Applied https://github.com/evdenis/linux-floppy/tree/cleanups
With your warnings fix for sparc64 here:
https://github.com/evdenis/linux-floppy/commit/9c51b73efc71afd52c4eb00d93fc09d3c95010c8
I've sent small fix on top of your patches to linux-block. If everything is ok
with my part I will send all patches to Jens for 5.8 in a week.
Thanks,
Denis