scsi: aacraid: Added support for hotplug
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.
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
Hi Don,
Quoted 49 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Monday, February 13, 2017 10:47 AM
To: Raghava Aditya Renukunta <[email protected]>
Cc: [email protected]
Subject: [bug report] scsi: aacraid: Added support for hotplugEXTERNAL 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:2243aac_process_events()
error: double unlock 'spin_lock:t_lock'
drivers/scsi/aacraid/commsup.c2130 spin_lock_irqsave(t_lock, flags); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^2131
2132 while (!list_empty(&(dev->queues-Quoted 1 line
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
2139set_current_state(TASK_RUNNING);
2140
2141 entry = dev->queues-Quoted 1 line
queue[HostNormCmdQueue].cmdq.next;
2142
list_del(entry);
2143
2144t_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;
Quoted 2 lines
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)?
Thank you,
Raghava Aditya
Quoted 41 lines
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
Quoted 51 lines · Raghava Aditya Renukunta
Hi Don,
Quoted 49 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Monday, February 13, 2017 10:47 AM
To: Raghava Aditya Renukunta <[email protected]>
Cc: [email protected]
Subject: [bug report] scsi: aacraid: Added support for hotplugEXTERNAL 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:2243aac_process_events()
error: double unlock 'spin_lock:t_lock'
drivers/scsi/aacraid/commsup.c2130 spin_lock_irqsave(t_lock, flags); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^2131
2132 while (!list_empty(&(dev->queues-Quoted 1 line
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
2139set_current_state(TASK_RUNNING);
2140
2141 entry = dev->queues-Quoted 1 line
queue[HostNormCmdQueue].cmdq.next;
2142
list_del(entry);
2143
2144t_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;
Quoted 6 lines
Quoted 2 lines
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 onespin_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
Quoted 44 lines
Thank you,
Raghava AdityaQuoted 41 lines
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
Hi Dan,
Quoted 77 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Monday, February 13, 2017 11:44 PM
To: Raghava Aditya Renukunta <[email protected]>
Cc: [email protected]
Subject: Re: [bug report] scsi: aacraid: Added support for hotplugEXTERNAL EMAIL
Quoted 52 lines · Raghava Aditya Renukunta
Hi Don,
Quoted 30 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Monday, February 13, 2017 10:47 AM
To: Raghava Aditya Renukunta <[email protected]>
Cc: [email protected]
Subject: [bug report] scsi: aacraid: Added support for hotplugEXTERNAL 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:2243aac_process_events()
error: double unlock 'spin_lock:t_lock'
drivers/scsi/aacraid/commsup.c2130 spin_lock_irqsave(t_lock, flags); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^2131
2132 while (!list_empty(&(dev->queues-Quoted 1 line
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;Quoted 8 lines
2138
2139set_current_state(TASK_RUNNING);
2140
2141 entry = dev->queues-Quoted 1 line
queue[HostNormCmdQueue].cmdq.next;
2142
list_del(entry);
2143
2144t_lock= dev->queues-queue[HostNormCmdQueue].lock;
Quoted 11 lines
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;Quoted 5 lines
Quoted 2 lines
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)?Quoted 1 line
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.
Quoted 8 lines
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.
Quoted 45 lines
Quoted 45 lines
Thank you,
Raghava AdityaQuoted 10 lines
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 ]
Quoted 16 lines
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;
Quoted 15 lines
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