[PATCH 3/4] m68k/mac: Don't send uninitialized data in IOP message reply

[PATCH 3/4] m68k/mac: Don't send uninitialized data in IOP message reply

[Finn Thain]

Clear the message reply before calling iop_complete(). This code path is
not normally executed but should that happen let's arrange for consistent
behaviour from the IOP.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 5fc3b59ba811..8d6946edf2c8 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -449,6 +449,7 @@ static void iop_handle_recv(uint iop_num, uint chan)
 		iop_pr_debug("unclaimed message on iop_num %d chan %d\n",
 		             iop_num, chan);
 		iop_pr_debug("%*ph\n", IOP_MSG_LEN, msg->message);
+		memset(msg->reply, 0, IOP_MSG_LEN);
 		iop_complete_message(msg);
 	}
 }
-- 
2.26.2

[PATCH 4/4] m68k/mac: Improve IOP debug messages

[Finn Thain]

Always dump the full message and reply. Avoid printing partial lines
as this output gets mixed up with the output from called functions.
Don't output the state of idle channels.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 8d6946edf2c8..373a74c99356 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -347,8 +347,8 @@ void iop_complete_message(struct iop_msg *msg)
 	int chan = msg->channel;
 	int i,offset;
 
-	iop_pr_debug("msg %p iop_num %d channel %d\n", msg, msg->iop_num,
-	             msg->channel);
+	iop_pr_debug("iop_num %d chan %d reply %*ph\n",
+	             msg->iop_num, msg->channel, IOP_MSG_LEN, msg->reply);
 
 	offset = IOP_ADDR_RECV_MSG + (msg->channel * IOP_MSG_LEN);
 
@@ -372,6 +372,9 @@ static void iop_do_send(struct iop_msg *msg)
 	volatile struct mac_iop *iop = iop_base[msg->iop_num];
 	int i,offset;
 
+	iop_pr_debug("iop_num %d chan %d message %*ph\n",
+	             msg->iop_num, msg->channel, IOP_MSG_LEN, msg->message);
+
 	offset = IOP_ADDR_SEND_MSG + (msg->channel * IOP_MSG_LEN);
 
 	for (i = 0 ; i < IOP_MSG_LEN ; i++, offset++) {
@@ -394,8 +397,6 @@ static void iop_handle_send(uint iop_num, uint chan)
 	struct iop_msg *msg;
 	int i,offset;
 
-	iop_pr_debug("iop_num %d chan %d\n", iop_num, chan);
-
 	iop_writeb(iop, IOP_ADDR_SEND_STATE + chan, IOP_MSG_IDLE);
 
 	if (!(msg = iop_send_queue[iop_num][chan])) return;
@@ -405,6 +406,9 @@ static void iop_handle_send(uint iop_num, uint chan)
 	for (i = 0 ; i < IOP_MSG_LEN ; i++, offset++) {
 		msg->reply[i] = iop_readb(iop, offset);
 	}
+	iop_pr_debug("iop_num %d chan %d reply %*ph\n",
+	             iop_num, chan, IOP_MSG_LEN, msg->reply);
+
 	if (msg->handler) (*msg->handler)(msg);
 	msg->status = IOP_MSGSTATUS_UNUSED;
 	msg = msg->next;
@@ -424,8 +428,6 @@ static void iop_handle_recv(uint iop_num, uint chan)
 	int i,offset;
 	struct iop_msg *msg;
 
-	iop_pr_debug("iop_num %d chan %d\n", iop_num, chan);
-
 	msg = iop_get_unused_msg();
 	msg->iop_num = iop_num;
 	msg->channel = chan;
@@ -437,6 +439,8 @@ static void iop_handle_recv(uint iop_num, uint chan)
 	for (i = 0 ; i < IOP_MSG_LEN ; i++, offset++) {
 		msg->message[i] = iop_readb(iop, offset);
 	}
+	iop_pr_debug("iop_num %d chan %d message %*ph\n",
+	             iop_num, chan, IOP_MSG_LEN, msg->message);
 
 	iop_writeb(iop, IOP_ADDR_RECV_STATE + chan, IOP_MSG_RCVD);
 
@@ -446,9 +450,6 @@ static void iop_handle_recv(uint iop_num, uint chan)
 	if (msg->handler) {
 		(*msg->handler)(msg);
 	} else {
-		iop_pr_debug("unclaimed message on iop_num %d chan %d\n",
-		             iop_num, chan);
-		iop_pr_debug("%*ph\n", IOP_MSG_LEN, msg->message);
 		memset(msg->reply, 0, IOP_MSG_LEN);
 		iop_complete_message(msg);
 	}
@@ -559,35 +560,34 @@ irqreturn_t iop_ism_irq(int irq, void *dev_id)
 	int i,state;
 	u8 events = iop->status_ctrl & (IOP_INT0 | IOP_INT1);
 
-	iop_pr_debug("status %02X\n", iop->status_ctrl);
-
 	do {
+		iop_pr_debug("iop_num %d status %02X\n", iop_num,
+		             iop->status_ctrl);
+
 		/* INT0 indicates state change on an outgoing message channel */
 		if (events & IOP_INT0) {
 			iop->status_ctrl = IOP_INT0 | IOP_RUN | IOP_AUTOINC;
-			iop_pr_debug("new status %02X, send states",
-			             iop->status_ctrl);
 			for (i = 0; i < NUM_IOP_CHAN; i++) {
 				state = iop_readb(iop, IOP_ADDR_SEND_STATE + i);
-				iop_pr_cont(" %02X", state);
 				if (state == IOP_MSG_COMPLETE)
 					iop_handle_send(iop_num, i);
+				else if (state != IOP_MSG_IDLE)
+					iop_pr_debug("chan %d send state %02X\n",
+					             i, state);
 			}
-			iop_pr_cont("\n");
 		}
 
 		/* INT1 for incoming messages */
 		if (events & IOP_INT1) {
 			iop->status_ctrl = IOP_INT1 | IOP_RUN | IOP_AUTOINC;
-			iop_pr_debug("new status %02X, recv states",
-			             iop->status_ctrl);
 			for (i = 0; i < NUM_IOP_CHAN; i++) {
 				state = iop_readb(iop, IOP_ADDR_RECV_STATE + i);
-				iop_pr_cont(" %02X", state);
 				if (state == IOP_MSG_NEW)
 					iop_handle_recv(iop_num, i);
+				else if (state != IOP_MSG_IDLE)
+					iop_pr_debug("chan %d recv state %02X\n",
+					             i, state);
 			}
-			iop_pr_cont("\n");
 		}
 
 		events = iop->status_ctrl & (IOP_INT0 | IOP_INT1);
-- 
2.26.2

[PATCH 2/4] m68k/mac: Fix IOP status/control register writes

[Finn Thain]

When writing values to the IOP status/control register make sure those
values do not have any extraneous bits that will clear interrupt flags.

To place the SCC IOP into bypass mode would be desirable but this is not
achieved by writing IOP_DMAINACTIVE | IOP_RUN | IOP_AUTOINC | IOP_BYPASS
to the control register. Drop this ineffective register write.

Remove the flawed and unused iop_bypass() function. Make use of the
unused iop_stop() function.

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index 22c4e2b8bdf2..5fc3b59ba811 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -183,7 +183,7 @@ static __inline__ void iop_writeb(volatile struct mac_iop *iop, __u16 addr, __u8
 
 static __inline__ void iop_stop(volatile struct mac_iop *iop)
 {
-	iop->status_ctrl &= ~IOP_RUN;
+	iop->status_ctrl = IOP_AUTOINC;
 }
 
 static __inline__ void iop_start(volatile struct mac_iop *iop)
@@ -191,14 +191,9 @@ static __inline__ void iop_start(volatile struct mac_iop *iop)
 	iop->status_ctrl = IOP_RUN | IOP_AUTOINC;
 }
 
-static __inline__ void iop_bypass(volatile struct mac_iop *iop)
-{
-	iop->status_ctrl |= IOP_BYPASS;
-}
-
 static __inline__ void iop_interrupt(volatile struct mac_iop *iop)
 {
-	iop->status_ctrl |= IOP_IRQ;
+	iop->status_ctrl = IOP_IRQ | IOP_RUN | IOP_AUTOINC;
 }
 
 static int iop_alive(volatile struct mac_iop *iop)
@@ -244,7 +239,6 @@ void __init iop_preinit(void)
 		} else {
 			iop_base[IOP_NUM_SCC] = (struct mac_iop *) SCC_IOP_BASE_QUADRA;
 		}
-		iop_base[IOP_NUM_SCC]->status_ctrl = 0x87;
 		iop_scc_present = 1;
 	} else {
 		iop_base[IOP_NUM_SCC] = NULL;
@@ -256,7 +250,7 @@ void __init iop_preinit(void)
 		} else {
 			iop_base[IOP_NUM_ISM] = (struct mac_iop *) ISM_IOP_BASE_QUADRA;
 		}
-		iop_base[IOP_NUM_ISM]->status_ctrl = 0;
+		iop_stop(iop_base[IOP_NUM_ISM]);
 		iop_ism_present = 1;
 	} else {
 		iop_base[IOP_NUM_ISM] = NULL;
-- 
2.26.2

[PATCH 1/4] m68k/mac: Don't send IOP message until channel is idle

[Finn Thain]

In the following sequence of calls, iop_do_send() gets called when the
"send" channel is not in the IOP_MSG_IDLE state:

iop_ism_irq()
        iop_handle_send()
                (msg->handler)()
                        iop_send_message()
                iop_do_send()

Avoid this by testing the channel state before calling iop_do_send().

When sending, and iop_send_queue is empty, call iop_do_send() because
the channel is idle. If iop_send_queue is not empty, iop_do_send() will
get called later by iop_handle_send().

Cc: Joshua Thompson <funaho@jurai.org>
Tested-by: Stan Johnson <userm57@yahoo.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
---
 arch/m68k/mac/iop.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/m68k/mac/iop.c b/arch/m68k/mac/iop.c
index d99c7ea08d8c..22c4e2b8bdf2 100644
--- a/arch/m68k/mac/iop.c
+++ b/arch/m68k/mac/iop.c
@@ -415,7 +415,8 @@ static void iop_handle_send(uint iop_num, uint chan)
 	msg->status = IOP_MSGSTATUS_UNUSED;
 	msg = msg->next;
 	iop_send_queue[iop_num][chan] = msg;
-	if (msg) iop_do_send(msg);
+	if (msg && iop_readb(iop, IOP_ADDR_SEND_STATE + chan) == IOP_MSG_IDLE)
+		iop_do_send(msg);
 }
 
 /*
@@ -489,16 +490,12 @@ int iop_send_message(uint iop_num, uint chan, void *privdata,
 
 	if (!(q = iop_send_queue[iop_num][chan])) {
 		iop_send_queue[iop_num][chan] = msg;
+		iop_do_send(msg);
 	} else {
 		while (q->next) q = q->next;
 		q->next = msg;
 	}
 
-	if (iop_readb(iop_base[iop_num],
-	    IOP_ADDR_SEND_STATE + chan) == IOP_MSG_IDLE) {
-		iop_do_send(msg);
-	}
-
 	return 0;
 }
 
-- 
2.26.2

[PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

This patch series has several bug fixes for the IOP driver and some
improvements to the debug level log messages.

Geert, please consider pushing these fixes for v5.8, if not the
whole series.


Finn Thain (4):
  m68k/mac: Don't send IOP message until channel is idle
  m68k/mac: Fix IOP status/control register writes
  m68k/mac: Don't send uninitialized data in IOP message reply
  m68k/mac: Improve IOP debug messages

 arch/m68k/mac/iop.c | 60 ++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 34 deletions(-)

-- 
2.26.2

Re: [PATCH 0/4] Mac IOP driver fixes

[Geert Uytterhoeven]

Hi Finn,

On Sun, May 31, 2020 at 1:16 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> This patch series has several bug fixes for the IOP driver and some
> improvements to the debug level log messages.

Thanks for your series!

> Geert, please consider pushing these fixes for v5.8, if not the
> whole series.

I'm afraid it's a bit too late for that, as I expect the v5.8 merge window
to open tonight.  Unless the fix is for a regression in v5.7.
BTW, how does the issue being fixed manifest itself? That's not clear to
me from the patch description.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] Mac IOP driver fixes

[Finn Thain]

On Sun, 31 May 2020, Geert Uytterhoeven wrote:

> Hi Finn,
> 
> On Sun, May 31, 2020 at 1:16 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> 
> > This patch series has several bug fixes for the IOP driver and some 
> > improvements to the debug level log messages.
> 
> Thanks for your series!
> 

Thanks for your review and for your diligence in the performance of all of 
your duties as maintainer.

> > Geert, please consider pushing these fixes for v5.8, if not the whole 
> > series.
> 
> I'm afraid it's a bit too late for that, as I expect the v5.8 merge 
> window to open tonight.

Well, it's not that important.

> Unless the fix is for a regression in v5.7.

AFAICT these bugs are as old as the original driver. But I don't think 
that disqualifies them from backporting, which I plan to do that once they 
hit mainline.

> BTW, how does the issue being fixed manifest itself? That's not clear to 
> me from the patch description.
> 

The bugs in the iop driver were found by inspection, in the course of 
debugging adb-iop driver failures that Stan encountered. It's possible 
that the adb-iop driver is not badly affected by these bugs (I don't 
know). It's possible that the iop driver bugs are among the reasons why 
the swim_iop driver was never stabilized. That driver was removed in 
b21a323710e7 ("[PATCH] remove the broken BLK_DEV_SWIM_IOP driver").

Re: [PATCH 0/4] Mac IOP driver fixes

[Geert Uytterhoeven]

On Sun, May 31, 2020 at 10:41 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sun, May 31, 2020 at 1:16 AM Finn Thain <fthain@telegraphics.com.au> wrote:
> > This patch series has several bug fixes for the IOP driver and some
> > improvements to the debug level log messages.
>
> Thanks for your series!
>
> > Geert, please consider pushing these fixes for v5.8, if not the
> > whole series.
>
> I'm afraid it's a bit too late for that, as I expect the v5.8 merge window
> to open tonight.  Unless the fix is for a regression in v5.7.

Queued in the m68k for-v5.9 branch.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds