Re: [PATCH 15/16] gdth: Move members from SCp to gdth_cmndinfo, stage 2

Rolf Eike Beer
2007-10-03T15:45:47Z

[-- Attachment #1: Type: text/plain, Size: 1911 bytes --]

Boaz Harrosh wrote:
> - Cleanup the rest of the scsi_cmnd-SCp members and move them
> to gdth_cmndinfo:

> SCp.have_data_in => volatile wait_for_completion

> Signed-off-by Boaz Harrosh <bharrosh@panasas.com>

volatile here is probably nonsense. Either you need proper locking on this
struct in case there may be side-effects between different callers or it's
unneeded at all. This looks really suspicious:

--- a/drivers/scsi/gdth_proc.c
+++ b/drivers/scsi/gdth_proc.c
@@ -728,20 +728,22 @@ static void gdth_wait_completion(gdth_ha_str *ha, int
busnum, int id)
ulong flags;
int i;
Scsi_Cmnd *scp;
+ struct gdth_cmndinfo *cmndinfo;
unchar b, t;

spin_lock_irqsave(&ha->smp_lock, flags);

for (i = 0; i < GDTH_MAXCMDS; ++i) {
scp = ha->cmd_tab[i].cmnd;
+ cmndinfo = gdth_cmnd_priv(scp);

b = scp->device->channel;
t = scp->device->id;
if (!SPECIAL_SCP(scp) && t == (unchar)id &&
b == (unchar)busnum) {
- scp->SCp.have_data_in = 0;
+ cmndinfo->wait_for_completion = 0;
spin_unlock_irqrestore(&ha->smp_lock, flags);
- while (!scp->SCp.have_data_in)
+ while (!cmndinfo->wait_for_completion)
barrier();

I haven't checked for locking around the other accesses, but they look racy.
This looks like wasting CPU cycles by busy-looping for a change. And for
things like these

@@ -3511,8 +3514,8 @@ static int gdth_sync_event(gdth_ha_str *ha, int service,
unchar index,
}
}
}
- if (!scp->SCp.have_data_in)
- scp->SCp.have_data_in++;
+ if (!cmndinfo->wait_for_completion)
+ cmndinfo->wait_for_completion++;

probably atomic_inc_not_zero() should be used if it is really needed at all.

Greetings,

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]