Hi Dan, > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Monday, February 13, 2017 11:44 PM > To: Raghava Aditya Renukunta > <RaghavaAditya.Renukunta@xxxxxxxxxxxxx> > Cc: linux-scsi@xxxxxxxxxxxxxxx > Subject: Re: [bug report] scsi: aacraid: Added support for hotplug > > EXTERNAL EMAIL > > > On Mon, Feb 13, 2017 at 07:39:15PM +0000, Raghava Aditya Renukunta > wrote: > > Hi Don, > > > > > -----Original Message----- > > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > > Sent: Monday, February 13, 2017 10:47 AM > > > To: Raghava Aditya Renukunta > > > <RaghavaAditya.Renukunta@xxxxxxxxxxxxx> > > > Cc: linux-scsi@xxxxxxxxxxxxxxx > > > Subject: [bug report] scsi: aacraid: Added support for hotplug > > > > > > EXTERNAL EMAIL > > > > > > > > > Hello Raghava Aditya Renukunta, > > > > > > The patch 6223a39fe6fb: "scsi: aacraid: Added support for hotplug" > > > from Feb 2, 2017, leads to the following static checker warning: > > > > > > drivers/scsi/aacraid/commsup.c:2243 aac_process_events() > > > error: double unlock 'spin_lock:t_lock' > > > > > > drivers/scsi/aacraid/commsup.c > > > 2130 spin_lock_irqsave(t_lock, flags); > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > 2131 > > > 2132 while (!list_empty(&(dev->queues- > > > >queue[HostNormCmdQueue].cmdq))) { > > > 2133 struct list_head *entry; > > > 2134 struct aac_aifcmd *aifcmd; > > > 2135 unsigned int num; > > > 2136 struct hw_fib **hw_fib_pool, **hw_fib_p; > > > 2137 struct fib **fib_pool, **fib_p; > > > 2138 > > > 2139 set_current_state(TASK_RUNNING); > > > 2140 > > > 2141 entry = dev->queues- > > > >queue[HostNormCmdQueue].cmdq.next; > > > 2142 list_del(entry); > > > 2143 > > > 2144 t_lock = dev->queues- > >queue[HostNormCmdQueue].lock; > > > 2145 spin_unlock_irqrestore(t_lock, flags); > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > 2146 > > > 2147 fib = list_entry(entry, struct fib, fiblink); > > > 2148 hw_fib = fib->hw_fib_va; > > > 2149 if (dev->sa_firmware) { > > > 2150 /* Thor AIF */ > > > 2151 aac_handle_sa_aif(dev, fib); > > > 2152 aac_fib_adapter_complete(fib, (u16)sizeof(u32)); > > > 2153 continue; > > > > > > The locking isn't right here. We should re-take the spinlock before > > > continuing. > > > > The intention here is to protect the retrieval of entry from the queues. > > Or do you mean that we should just protect the whole while loop with one > spin_lock (t_lock)? > > > > This is a static checker warning that says we call > spin_unlock_irqrestore(t_lock, flags); at the end of the loop but > sometimes we're not holding the lock. > > This is a Smatch warning and it doesn't handle loops correctly. It > should also warn that on line 2145 we might not be holding the lock > either but it misses that bug. > > There is no way this continue is correct with regards to locking. > > regards, > dan carpenter I agree I will fix it shortly. Thank you. > > Thank you, > > Raghava Aditya > > > > > 2154 } > > > 2155 /* > > > 2156 * We will process the FIB here or pass it to a > > > 2157 * worker thread that is TBD. We Really can't > > > 2158 * do anything at this point since we don't have > > > 2159 * anything defined for this thread to do. > > > 2160 */ > > > > > > [ snip ] > > > > > > 2221 free_mem: > > > 2222 /* Free up the remaining resources */ > > > 2223 hw_fib_p = hw_fib_pool; > > > 2224 fib_p = fib_pool; > > > 2225 while (hw_fib_p < &hw_fib_pool[num]) { > > > 2226 kfree(*hw_fib_p); > > > 2227 kfree(*fib_p); > > > 2228 ++fib_p; > > > 2229 ++hw_fib_p; > > > 2230 } > > > 2231 kfree(fib_pool); > > > 2232 free_hw_fib_pool: > > > 2233 kfree(hw_fib_pool); > > > 2234 free_fib: > > > 2235 kfree(fib); > > > 2236 t_lock = dev->queues- > >queue[HostNormCmdQueue].lock; > > > 2237 spin_lock_irqsave(t_lock, flags); > > > 2238 } > > > 2239 /* > > > 2240 * There are no more AIF's > > > 2241 */ > > > 2242 t_lock = dev->queues->queue[HostNormCmdQueue].lock; > > > 2243 spin_unlock_irqrestore(t_lock, flags); > > > > > > Otherwise it is a double unlock bug. > > > > > > 2244 } > > > > > > > > > regards, > > > dan carpenter
