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

[Rolf Eike Beer]

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 <[EMAIL PROTECTED]>

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

signature.asc
Description: This is a digitally signed message part.