[v2,14/15] scsi: ncr53c8xx: replace CCB_MAGIC with bool busy

Commit Message

наб Nov. 1, 2022, 11:06 p.m. UTC

The only non-boolean check might as well be, since it just early-exits
instead of noting the bug: lower it to a boolean and make it less
confusing.

As for magic numbers, we have largely moved away from this approach,
and we have better debugging instrumentation nowadays: kill it.

Link: https://lore.kernel.org/linux-doc/[e-mail]/
Signed-off-by: Ahelenia Ziemiańska <[e-mail]>
---
 Documentation/process/magic-number.rst        |  1 -
 .../it_IT/process/magic-number.rst            |  1 -
 .../zh_CN/process/magic-number.rst            |  1 -
 .../zh_TW/process/magic-number.rst            |  1 -
 drivers/scsi/ncr53c8xx.c                      | 25 ++++++-------------
 5 files changed, 8 insertions(+), 21 deletions(-)

Comments

James Bottomley Nov. 4, 2022, 7:57 p.m. UTC | #1

On Wed, 2022-11-02 at 00:06 +0100, Ahelenia Ziemiańska wrote:
[...]
> @@ -4356,7 +4347,7 @@ static int ncr_queue_command (struct ncb *np,
> struct scsi_cmnd *cmd)
>  	*/
>  
>  	/* activate this job.  */
> -	cp->magic		= CCB_MAGIC;
> +	cp->busy		= true;
>  
>  	/*
>  	**	insert next CCBs into start queue.

Are you sure this is just an internal magic number?  The way these old
scripts engines used to work is that the CCB array forms a mailbox and
the card continually scans the mailbox to see if it has any work to do.
These magic values are often a signal to the card that the CCB array
contains a valid entry it hasn't seen and it should work on it, so the
card might be expecting to see this full 32 bit value.

Regards,

James

наб Nov. 9, 2022, 7:40 p.m. UTC | #2

On Fri, Nov 04, 2022 at 03:57:20PM -0400, James Bottomley wrote:
> On Wed, 2022-11-02 at 00:06 +0100, Ahelenia Ziemiańska wrote:
> [...]
> > @@ -4356,7 +4347,7 @@ static int ncr_queue_command (struct ncb *np,
> > struct scsi_cmnd *cmd)
> >  	*/
> >  
> >  	/* activate this job.  */
> > -	cp->magic		= CCB_MAGIC;
> > +	cp->busy		= true;
> >  
> >  	/*
> >  	**	insert next CCBs into start queue.
> 
> Are you sure this is just an internal magic number?  The way these old
> scripts engines used to work is that the CCB array forms a mailbox and
> the card continually scans the mailbox to see if it has any work to do.
> These magic values are often a signal to the card that the CCB array
> contains a valid entry it hasn't seen and it should work on it, so the
> card might be expecting to see this full 32 bit value.

AFAICT this is an internal flag: in FreeBSD, whence this is copied, this
driver is introduced in 37bd2c9c33d9684c0bf650e02579ebb329724d6a[1],
and AFAICT it's used in exactly the same way as it is here; the manual[2]
doesn't mention it at all; neither do the scripts; and the layout is
vastly different between there and current Linux; so I'm happy to
conclude it's not used by the device, I think?

1: https://cgit.freebsd.org/src/commit/?id=37bd2c9c33d9684c0bf650e02579ebb329724d6a
2: http://bitsavers.trailing-edge.com/components/ncr_symbios/scsi/53C810/53C810_PCI-SCSI_IO_Processor_Data_Manual_May1993.pdf

Patch

diff --git a/Documentation/process/magic-number.rst b/Documentation/process/magic-number.rst
index 6e432917a5a8..5a8c2755ac9c 100644
--- a/Documentation/process/magic-number.rst
+++ b/Documentation/process/magic-number.rst
@@ -68,5 +68,4 @@  Changelog::
 ===================== ================ ======================== ==========================================
 Magic Name            Number           Structure                File
 ===================== ================ ======================== ==========================================
-CCB_MAGIC             0xf2691ad2       ccb                      ``drivers/scsi/ncr53c8xx.c``
 ===================== ================ ======================== ==========================================
diff --git a/Documentation/translations/it_IT/process/magic-number.rst b/Documentation/translations/it_IT/process/magic-number.rst
index 7d4c117ac626..2fbc1876534a 100644
--- a/Documentation/translations/it_IT/process/magic-number.rst
+++ b/Documentation/translations/it_IT/process/magic-number.rst
@@ -74,5 +74,4 @@  Registro dei cambiamenti::
 ===================== ================ ======================== ==========================================
 Nome magico           Numero           Struttura                File
 ===================== ================ ======================== ==========================================
-CCB_MAGIC             0xf2691ad2       ccb                      ``drivers/scsi/ncr53c8xx.c``
 ===================== ================ ======================== ==========================================
diff --git a/Documentation/translations/zh_CN/process/magic-number.rst b/Documentation/translations/zh_CN/process/magic-number.rst
index c17e3f20440a..f8ec4767bc4e 100644
--- a/Documentation/translations/zh_CN/process/magic-number.rst
+++ b/Documentation/translations/zh_CN/process/magic-number.rst
@@ -57,5 +57,4 @@  Linux 魔术数
 ===================== ================ ======================== ==========================================
 魔术数名              数字             结构                     文件
 ===================== ================ ======================== ==========================================
-CCB_MAGIC             0xf2691ad2       ccb                      ``drivers/scsi/ncr53c8xx.c``
 ===================== ================ ======================== ==========================================
diff --git a/Documentation/translations/zh_TW/process/magic-number.rst b/Documentation/translations/zh_TW/process/magic-number.rst
index e2eeb74e7192..0ccc60bee3d6 100644
--- a/Documentation/translations/zh_TW/process/magic-number.rst
+++ b/Documentation/translations/zh_TW/process/magic-number.rst
@@ -60,5 +60,4 @@  Linux 魔術數
 ===================== ================ ======================== ==========================================
 魔術數名              數字             結構                     文件
 ===================== ================ ======================== ==========================================
-CCB_MAGIC             0xf2691ad2       ccb                      ``drivers/scsi/ncr53c8xx.c``
 ===================== ================ ======================== ==========================================
diff --git a/drivers/scsi/ncr53c8xx.c b/drivers/scsi/ncr53c8xx.c
index 4458449c960b..928417fca495 100644
--- a/drivers/scsi/ncr53c8xx.c
+++ b/drivers/scsi/ncr53c8xx.c
@@ -1095,15 +1095,6 @@  typedef u32 tagmap_t;
 #define NS_WIDE		(2)
 #define NS_PPR		(4)
 
-/*==========================================================
-**
-**	Misc.
-**
-**==========================================================
-*/
-
-#define CCB_MAGIC	(0xf2691ad2)
-
 /*==========================================================
 **
 **	Declaration of structs.
@@ -1567,7 +1558,7 @@  struct ccb {
 	struct ccb *	link_ccb;	/* Host adapter CCB chain	*/
 	struct list_head link_ccbq;	/* Link to unit CCB queue	*/
 	u32		startp;		/* Initial data pointer		*/
-	u_long		magic;		/* Free / busy  CCB flag	*/
+	bool		busy;
 };
 
 #define CCB_PHYS(cp,lbl)	(cp->p_ccb + offsetof(struct ccb, lbl))
@@ -4356,7 +4347,7 @@  static int ncr_queue_command (struct ncb *np, struct scsi_cmnd *cmd)
 	*/
 
 	/* activate this job.  */
-	cp->magic		= CCB_MAGIC;
+	cp->busy		= true;
 
 	/*
 	**	insert next CCBs into start queue.
@@ -4667,7 +4658,7 @@  void ncr_complete (struct ncb *np, struct ccb *cp)
 	**	Sanity check
 	*/
 
-	if (!cp || cp->magic != CCB_MAGIC || !cp->cmd)
+	if (!cp || !cp->busy || !cp->cmd)
 		return;
 
 	/*
@@ -6998,7 +6989,7 @@  static struct ccb *ncr_get_ccb(struct ncb *np, struct scsi_cmnd *cmd)
 		qp = ncr_list_pop(&lp->free_ccbq);
 		if (qp) {
 			cp = list_entry(qp, struct ccb, link_ccbq);
-			if (cp->magic) {
+			if (cp->busy) {
 				PRINT_ADDR(cmd, "ccb free list corrupted "
 						"(@%p)\n", cp);
 				cp = NULL;
@@ -7030,17 +7021,17 @@  static struct ccb *ncr_get_ccb(struct ncb *np, struct scsi_cmnd *cmd)
 	**	Wait until available.
 	*/
 #if 0
-	while (cp->magic) {
+	while (cp->busy) {
 		if (flags & SCSI_NOSLEEP) break;
 		if (tsleep ((caddr_t)cp, PRIBIO|PCATCH, "ncr", 0))
 			break;
 	}
 #endif
 
-	if (cp->magic)
+	if (cp->busy)
 		return NULL;
 
-	cp->magic = 1;
+	cp->busy = true;
 
 	/*
 	**	Move to next available tag if tag used.
@@ -7119,7 +7110,7 @@  static void ncr_free_ccb (struct ncb *np, struct ccb *cp)
 		}
 	}
 	cp -> host_status = HS_IDLE;
-	cp -> magic = 0;
+	cp -> busy = false;
 	if (cp->queued) {
 		--np->queuedccbs;
 		cp->queued = 0;