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@xxxxxxxxxxx>
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:
signature.asc
Description: This is a digitally signed message part.
