Various issues with the via-macii driver have become apparent over the
years. Some examples:
- A Talk command response can be lost. This can result in phantom devices
being probed or an incorrect device handler ID being retrieved.
- A reply packet containing a null byte can get truncated. Such packets
are sometimes generated by ADB keyboards.
- A Talk Register 3 reply from device 15 (that is, command byte 0xFF)
can be mistaken for a bus timeout (empty packet).
This patch series contains fixes for all known bugs in the via-macii
driver, plus a few code style improvements. It has been successfully
tested on an Apple Centris 650 and qemu-system-m68k.
The patched kernel does regress on past QEMU releases, due to ADB
transceiver emulation bugs. Those bugs have been fixed in mainline QEMU.
My thanks go to Mark Cave-Ayland for that effort and for figuring out
the improvements to the signalling between the VIA and the transceiver.
Note to -stable maintainers: these fixes can be cherry-picked without
difficulty, if you have the 5 commits that appeared in v5.0:
b52dce8738938 macintosh/via-macii: Synchronous bus reset
5f93d7081a47e macintosh/via-macii: Remove BUG_ON assertions
5ce6185c2ef4e macintosh/via-macii: Simplify locking
351e5ad327d07 macintosh/via-macii, macintosh/adb-iop: Modernize printk calls
47fd2060660e6 macintosh/via-macii, macintosh/adb-iop: Clean up whitespace
Just for the sake of simplicity, the 'fixes' tags in this series limit
backporting to 'v5.0+'.
Finn Thain (9):
macintosh/via-macii: Access autopoll_devs when inside lock
macintosh/via-macii: Poll the device most likely to respond
macintosh/via-macii: Handle /CTLR_IRQ signal correctly
macintosh/via-macii: Remove read_done state
macintosh/via-macii: Handle poll replies correctly
macintosh/via-macii: Use bool type for reading_reply variable
macintosh/via-macii: Use unsigned type for autopoll_devs variable
macintosh/via-macii: Use the stack for reset request storage
macintosh/via-macii: Clarify definition of macii_init()
drivers/macintosh/via-macii.c | 324 +++++++++++++++++++---------------
1 file changed, 179 insertions(+), 145 deletions(-)
--
2.26.2
I'm told that the /CTLR_IRQ signal from the ADB transceiver gets
interpreted by MacOS to mean SRQ, bus timeout or end-of-packet depending
on the circumstances, and that Linux's via-macii driver does not
correctly interpret this signal.
Instead, the via-macii driver interprets certain received byte values
(0x00 and 0xFF) as signalling end of packet and bus timeout
(respectively). Problem is, those values can also appear under other
circumstances.
This patch changes the bus timeout, end of packet and SRQ detection logic
to bring it closer to the logic that MacOS reportedly uses.
Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2") # v5.0+
Reported-by: Mark Cave-Ayland <[email protected]>
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/macintosh/via-macii.c | 166 ++++++++++++++++++++--------------
1 file changed, 97 insertions(+), 69 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index d4f1a65c5f1fd..6a5cd7de05baf 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -80,6 +80,8 @@ static volatile unsigned char *via;
/* ADB command byte structure */
#define ADDR_MASK 0xF0
#define CMD_MASK 0x0F
+#define OP_MASK 0x0C
+#define TALK 0x0C
static int macii_init_via(void);
static void macii_start(void);
@@ -119,9 +121,10 @@ static int reading_reply; /* store reply in reply_buf else req->reply */
static int data_index; /* index of the next byte to send from req->data */
static int reply_len; /* number of bytes received in reply_buf or req->reply */
static int status; /* VIA's ADB status bits captured upon interrupt */
-static int last_status; /* status bits as at previous interrupt */
-static int srq_asserted; /* have to poll for the device that asserted it */
+static bool bus_timeout; /* no data was sent by the device */
+static bool srq_asserted; /* have to poll for the device that asserted it */
static u8 last_cmd; /* the most recent command byte transmitted */
+static u8 last_talk_cmd; /* the most recent Talk command byte transmitted */
static u8 last_poll_cmd; /* the most recent Talk R0 command byte transmitted */
static int autopoll_devs; /* bits set are device addresses to be polled */
@@ -170,7 +173,6 @@ static int macii_init_via(void)
/* Set up state: idle */
via[B] |= ST_IDLE;
- last_status = via[B] & (ST_MASK | CTLR_IRQ);
/* Shift register on input */
via[ACR] = (via[ACR] & ~SR_CTRL) | SR_EXT;
@@ -336,13 +338,6 @@ static void macii_start(void)
* And req->nbytes is the number of bytes of real data plus one.
*/
- /* store command byte */
- last_cmd = req->data[1];
-
- /* If this is a Talk Register 0 command, store the command byte */
- if ((last_cmd & CMD_MASK) == ADB_READREG(0, 0))
- last_poll_cmd = last_cmd;
-
/* Output mode */
via[ACR] |= SR_OUT;
/* Load data */
@@ -352,6 +347,9 @@ static void macii_start(void)
macii_state = sending;
data_index = 2;
+
+ bus_timeout = false;
+ srq_asserted = false;
}
/*
@@ -360,15 +358,17 @@ static void macii_start(void)
* generating shift register interrupts (SR_INT) for us. This means there has
* to be activity on the ADB bus. The chip will poll to achieve this.
*
- * The basic ADB state machine was left unchanged from the original MacII code
- * by Alan Cox, which was based on the CUDA driver for PowerMac.
- * The syntax of the ADB status lines is totally different on MacII,
- * though. MacII uses the states Command -> Even -> Odd -> Even ->...-> Idle
- * for sending and Idle -> Even -> Odd -> Even ->...-> Idle for receiving.
- * Start and end of a receive packet are signalled by asserting /IRQ on the
- * interrupt line (/IRQ means the CTLR_IRQ bit in port B; not to be confused
- * with the VIA shift register interrupt. /IRQ never actually interrupts the
- * processor, it's just an ordinary input.)
+ * The VIA Port B output signalling works as follows. After the ADB transceiver
+ * sees a transition on the PB4 and PB5 lines it will crank over the VIA shift
+ * register which eventually raises the SR_INT interrupt. The PB4/PB5 outputs
+ * are toggled with each byte as the ADB transaction progresses.
+ *
+ * Request with no reply expected (and empty transceiver buffer):
+ * CMD -> IDLE
+ * Request with expected reply packet (or with buffered autopoll packet):
+ * CMD -> EVEN -> ODD -> EVEN -> ... -> IDLE
+ * Unsolicited packet:
+ * IDLE -> EVEN -> ODD -> EVEN -> ... -> IDLE
*/
static irqreturn_t macii_interrupt(int irq, void *arg)
{
@@ -388,31 +388,31 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
}
}
- last_status = status;
status = via[B] & (ST_MASK | CTLR_IRQ);
switch (macii_state) {
case idle:
- if (reading_reply) {
- reply_ptr = current_req->reply;
- } else {
- WARN_ON(current_req);
- reply_ptr = reply_buf;
- }
+ WARN_ON((status & ST_MASK) != ST_IDLE);
+
+ reply_ptr = reply_buf;
+ reading_reply = 0;
+
+ bus_timeout = false;
+ srq_asserted = false;
x = via[SR];
- if ((status & CTLR_IRQ) && (x == 0xFF)) {
- /* Bus timeout without SRQ sequence:
- * data is "FF" while CTLR_IRQ is "H"
+ if (!(status & CTLR_IRQ)) {
+ /* /CTLR_IRQ asserted in idle state means we must
+ * read an autopoll reply from the transceiver buffer.
*/
- reply_len = 0;
- srq_asserted = 0;
- macii_state = read_done;
- } else {
macii_state = reading;
*reply_ptr = x;
reply_len = 1;
+ } else {
+ /* bus timeout */
+ macii_state = read_done;
+ reply_len = 0;
}
/* set ADB state = even for first data byte */
@@ -421,13 +421,52 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
case sending:
req = current_req;
- if (data_index >= req->nbytes) {
+
+ if (status == (ST_CMD | CTLR_IRQ)) {
+ /* /CTLR_IRQ de-asserted after the command byte means
+ * the host can continue with the transaction.
+ */
+
+ /* Store command byte */
+ last_cmd = req->data[1];
+ if ((last_cmd & OP_MASK) == TALK) {
+ last_talk_cmd = last_cmd;
+ if ((last_cmd & CMD_MASK) == ADB_READREG(0, 0))
+ last_poll_cmd = last_cmd;
+ }
+ }
+
+ if (status == ST_CMD) {
+ /* /CTLR_IRQ asserted after the command byte means we
+ * must read an autopoll reply. The first byte was
+ * lost because the shift register was an output.
+ */
+ macii_state = reading;
+
+ reading_reply = 0;
+ reply_ptr = reply_buf;
+ *reply_ptr = last_talk_cmd;
+ reply_len = 1;
+
+ /* reset to shift in */
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
+ } else if (data_index >= req->nbytes) {
req->sent = 1;
- macii_state = idle;
if (req->reply_expected) {
+ macii_state = reading;
+
reading_reply = 1;
+ reply_ptr = req->reply;
+ *reply_ptr = req->data[1];
+ reply_len = 1;
+
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
} else {
+ macii_state = idle;
+
req->complete = 1;
current_req = req->next;
if (req->done)
@@ -438,25 +477,26 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
if (current_req && macii_state == idle)
macii_start();
- }
- if (macii_state == idle) {
- /* reset to shift in */
- via[ACR] &= ~SR_OUT;
- x = via[SR];
- /* set ADB state idle - might get SRQ */
- via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
+ if (macii_state == idle) {
+ /* reset to shift in */
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
+ /* set ADB state idle - might get SRQ */
+ via[B] = (via[B] & ~ST_MASK) | ST_IDLE;
+ }
+ break;
}
} else {
via[SR] = req->data[data_index++];
+ }
- if ((via[B] & ST_MASK) == ST_CMD) {
- /* just sent the command byte, set to EVEN */
- via[B] = (via[B] & ~ST_MASK) | ST_EVEN;
- } else {
- /* invert state bits, toggle ODD/EVEN */
- via[B] ^= ST_MASK;
- }
+ if ((via[B] & ST_MASK) == ST_CMD) {
+ /* just sent the command byte, set to EVEN */
+ via[B] = (via[B] & ~ST_MASK) | ST_EVEN;
+ } else {
+ /* invert state bits, toggle ODD/EVEN */
+ via[B] ^= ST_MASK;
}
break;
@@ -465,28 +505,13 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
WARN_ON((status & ST_MASK) == ST_CMD ||
(status & ST_MASK) == ST_IDLE);
- /* Bus timeout with SRQ sequence:
- * data is "XX FF" while CTLR_IRQ is "L L"
- * End of packet without SRQ sequence:
- * data is "XX...YY 00" while CTLR_IRQ is "L...H L"
- * End of packet SRQ sequence:
- * data is "XX...YY 00" while CTLR_IRQ is "L...L L"
- * (where XX is the first response byte and
- * YY is the last byte of valid response data.)
- */
-
- srq_asserted = 0;
if (!(status & CTLR_IRQ)) {
- if (x == 0xFF) {
- if (!(last_status & CTLR_IRQ)) {
- macii_state = read_done;
- reply_len = 0;
- srq_asserted = 1;
- }
- } else if (x == 0x00) {
+ if (status == ST_EVEN && reply_len == 1) {
+ bus_timeout = true;
+ } else if (status == ST_ODD && reply_len == 2) {
+ srq_asserted = true;
+ } else {
macii_state = read_done;
- if (!(last_status & CTLR_IRQ))
- srq_asserted = 1;
}
}
@@ -504,6 +529,9 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
case read_done:
x = via[SR];
+ if (bus_timeout)
+ reply_len = 0;
+
if (reading_reply) {
reading_reply = 0;
req = current_req;
--
2.26.2
Userspace applications may use /dev/adb to send Talk requests. Such
requests always have req->reply_expected == 1. The same is true of Talk
requests sent by the kernel, except for poll requests queued internally
by the via-macii driver. Those requests have req->reply_expected == 0.
Consequently, poll reply packets get treated like autopoll reply packets.
(It doesn't make sense to try to distinguish them.) Always enter 'reading'
state after a poll request, so that the reply gets collected and passed
to adb_input(), and none go missing.
All Talk replies passed to adb_input() come from polling or autopolling,
so call adb_input() with the autopoll parameter set to 1.
Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/macintosh/via-macii.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index d29c87943ca46..8d5ef77b4a435 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -463,6 +463,21 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
via[ACR] &= ~SR_OUT;
x = via[SR];
+ } else if ((req->data[1] & OP_MASK) == TALK) {
+ macii_state = reading;
+
+ reading_reply = 0;
+ reply_ptr = reply_buf;
+ *reply_ptr = req->data[1];
+ reply_len = 1;
+
+ via[ACR] &= ~SR_OUT;
+ x = via[SR];
+
+ req->complete = 1;
+ current_req = req->next;
+ if (req->done)
+ (*req->done)(req);
} else {
macii_state = idle;
@@ -510,8 +525,9 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
current_req = req->next;
if (req->done)
(*req->done)(req);
- } else if (reply_len && autopoll_devs) {
- adb_input(reply_buf, reply_len, 0);
+ } else if (reply_len && autopoll_devs &&
+ reply_buf[0] == last_poll_cmd) {
+ adb_input(reply_buf, reply_len, 1);
}
break;
}
--
2.26.2
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/macintosh/via-macii.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index 8d5ef77b4a435..e143ddb81de34 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -116,7 +116,7 @@ static struct adb_request *current_req; /* first request struct in the queue */
static struct adb_request *last_req; /* last request struct in the queue */
static unsigned char reply_buf[16]; /* storage for autopolled replies */
static unsigned char *reply_ptr; /* next byte in reply_buf or req->reply */
-static int reading_reply; /* store reply in reply_buf else req->reply */
+static bool reading_reply; /* store reply in reply_buf else req->reply */
static int data_index; /* index of the next byte to send from req->data */
static int reply_len; /* number of bytes received in reply_buf or req->reply */
static int status; /* VIA's ADB status bits captured upon interrupt */
@@ -394,7 +394,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
WARN_ON((status & ST_MASK) != ST_IDLE);
reply_ptr = reply_buf;
- reading_reply = 0;
+ reading_reply = false;
bus_timeout = false;
srq_asserted = false;
@@ -442,7 +442,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
*/
macii_state = reading;
- reading_reply = 0;
+ reading_reply = false;
reply_ptr = reply_buf;
*reply_ptr = last_talk_cmd;
reply_len = 1;
@@ -456,7 +456,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
if (req->reply_expected) {
macii_state = reading;
- reading_reply = 1;
+ reading_reply = true;
reply_ptr = req->reply;
*reply_ptr = req->data[1];
reply_len = 1;
@@ -466,7 +466,7 @@ static irqreturn_t macii_interrupt(int irq, void *arg)
} else if ((req->data[1] & OP_MASK) == TALK) {
macii_state = reading;
- reading_reply = 0;
+ reading_reply = false;
reply_ptr = reply_buf;
*reply_ptr = req->data[1];
reply_len = 1;
--
2.26.2
The interrupt handler should be excluded when accessing the autopoll_devs
variable.
Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
Tested-by: Stan Johnson <[email protected]>
Signed-off-by: Finn Thain <[email protected]>
---
drivers/macintosh/via-macii.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
index ac824d7b2dcfc..6aa903529570d 100644
--- a/drivers/macintosh/via-macii.c
+++ b/drivers/macintosh/via-macii.c
@@ -270,15 +270,12 @@ static int macii_autopoll(int devs)
unsigned long flags;
int err = 0;
+ local_irq_save(flags);
+
/* bit 1 == device 1, and so on. */
autopoll_devs = devs & 0xFFFE;
- if (!autopoll_devs)
- return 0;
-
- local_irq_save(flags);
-
- if (current_req == NULL) {
+ if (autopoll_devs && !current_req) {
/* Send a Talk Reg 0. The controller will repeatedly transmit
* this as long as it is idle.
*/
--
2.26.2
On Sun, 28 Jun 2020 14:23:12 +1000, Finn Thain wrote:
> Various issues with the via-macii driver have become apparent over the
> years. Some examples:
>
> - A Talk command response can be lost. This can result in phantom devices
> being probed or an incorrect device handler ID being retrieved.
>
> - A reply packet containing a null byte can get truncated. Such packets
> are sometimes generated by ADB keyboards.
>
> [...]
Applied to powerpc/next.
[1/9] macintosh/via-macii: Access autopoll_devs when inside lock
https://git.kernel.org/powerpc/c/59ea38f6b3af5636edf541768a1ed721eeaca99e
[2/9] macintosh/via-macii: Poll the device most likely to respond
https://git.kernel.org/powerpc/c/f93bfeb55255bddaa16597e187a99ae6131b964a
[3/9] macintosh/via-macii: Handle /CTLR_IRQ signal correctly
https://git.kernel.org/powerpc/c/b4d76c28eca369b8105fe3a0a9f396e3fbcd0dd5
[4/9] macintosh/via-macii: Remove read_done state
https://git.kernel.org/powerpc/c/b16b67689baa01a5616b651356df7ad3e47a8763
[5/9] macintosh/via-macii: Handle poll replies correctly
https://git.kernel.org/powerpc/c/624cf5b538b507293ec761797bd8ce0702fefe64
[6/9] macintosh/via-macii: Use bool type for reading_reply variable
https://git.kernel.org/powerpc/c/f87a162572c9f7c839a207c7de6c73ffe54a777c
[7/9] macintosh/via-macii: Use unsigned type for autopoll_devs variable
https://git.kernel.org/powerpc/c/5c0c15a1953a7de2878d7e6f5711fd3322b11faa
[8/9] macintosh/via-macii: Use the stack for reset request storage
https://git.kernel.org/powerpc/c/046ace8256489f32740da07de55a913ca09ce5cf
[9/9] macintosh/via-macii: Clarify definition of macii_init()
https://git.kernel.org/powerpc/c/3327e58a04501e06aa531cdb4044aab214a6254a
cheers
Hi,
On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> The interrupt handler should be excluded when accessing the autopoll_devs
> variable.
>
I am quite baffled by this patch. Other than adding an unnecessary lock /
unlock sequence, accessing a variable (which is derived from another
variable) from inside or outside a lock does not make a difference.
If autopoll_devs = devs & 0xfffe is 0 inside the lock, it will just
as much be 0 outside the lock, and vice versa.
Can you explain this in some more detail ? Not that is matters much since
the change already made it into mainline, but I would like to understand
what if anything I am missing here.
Thanks,
Guenter
> Fixes: d95fd5fce88f0 ("m68k: Mac II ADB fixes") # v5.0+
> Tested-by: Stan Johnson <[email protected]>
> Signed-off-by: Finn Thain <[email protected]>
> ---
> drivers/macintosh/via-macii.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/macintosh/via-macii.c b/drivers/macintosh/via-macii.c
> index ac824d7b2dcfc..6aa903529570d 100644
> --- a/drivers/macintosh/via-macii.c
> +++ b/drivers/macintosh/via-macii.c
> @@ -270,15 +270,12 @@ static int macii_autopoll(int devs)
> unsigned long flags;
> int err = 0;
>
> + local_irq_save(flags);
> +
> /* bit 1 == device 1, and so on. */
> autopoll_devs = devs & 0xFFFE;
>
> - if (!autopoll_devs)
> - return 0;
> -
> - local_irq_save(flags);
> -
> - if (current_req == NULL) {
> + if (autopoll_devs && !current_req) {
> /* Send a Talk Reg 0. The controller will repeatedly transmit
> * this as long as it is idle.
> */
On Sun, 9 Aug 2020, Guenter Roeck wrote:
> Hi,
>
> On Sun, Jun 28, 2020 at 02:23:12PM +1000, Finn Thain wrote:
> > The interrupt handler should be excluded when accessing the
> > autopoll_devs variable.
> >
>
> I am quite baffled by this patch. Other than adding an unnecessary lock
> / unlock sequence,
The new lock/unlock sequence means that the expression (autopoll_devs &&
!current_req) can be understood to be atomic. That makes it easier for me
to follow (being that both variables are shared state).
> accessing a variable (which is derived from another variable) from
> inside or outside a lock does not make a difference. If autopoll_devs =
> devs & 0xfffe is 0 inside the lock, it will just as much be 0 outside
> the lock, and vice versa.
>
> Can you explain this in some more detail ? Not that is matters much
> since the change already made it into mainline, but I would like to
> understand what if anything I am missing here.
>
I think the new code is more readable and is obviously free of race
conditions. It's not obvious to me why the old code was free of race
conditions but if you can easily establish that by inspection then you are
a better auditor than I am. Regardless, I'll stick with "Keep It Simple,
Stupid".