[v2,14/15] scsi: ncr53c8xx: replace CCB_MAGIC with bool busy
Commit Message
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/YyMlovoskUcHLEb7@kroah.com/
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
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
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
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
@@ -68,5 +68,4 @@ Changelog::
===================== ================ ======================== ==========================================
Magic Name Number Structure File
===================== ================ ======================== ==========================================
-CCB_MAGIC 0xf2691ad2 ccb ``drivers/scsi/ncr53c8xx.c``
===================== ================ ======================== ==========================================
@@ -74,5 +74,4 @@ Registro dei cambiamenti::
===================== ================ ======================== ==========================================
Nome magico Numero Struttura File
===================== ================ ======================== ==========================================
-CCB_MAGIC 0xf2691ad2 ccb ``drivers/scsi/ncr53c8xx.c``
===================== ================ ======================== ==========================================
@@ -57,5 +57,4 @@ Linux 魔术数
===================== ================ ======================== ==========================================
魔术数名 数字 结构 文件
===================== ================ ======================== ==========================================
-CCB_MAGIC 0xf2691ad2 ccb ``drivers/scsi/ncr53c8xx.c``
===================== ================ ======================== ==========================================
@@ -60,5 +60,4 @@ Linux 魔術數
===================== ================ ======================== ==========================================
魔術數名 數字 結構 文件
===================== ================ ======================== ==========================================
-CCB_MAGIC 0xf2691ad2 ccb ``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;