* [bug report] scsi: aacraid: Added support for hotplug
@ 2017-02-13 18:47 Dan Carpenter
2017-02-13 19:39 ` Raghava Aditya Renukunta
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-02-13 18:47 UTC (permalink / raw)
To: RaghavaAditya.Renukunta; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [bug report] scsi: aacraid: Added support for hotplug
2017-02-13 18:47 [bug report] scsi: aacraid: Added support for hotplug Dan Carpenter
@ 2017-02-13 19:39 ` Raghava Aditya Renukunta
2017-02-14 7:43 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Raghava Aditya Renukunta @ 2017-02-13 19:39 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-scsi@vger.kernel.org
Hi Don,
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, February 13, 2017 10:47 AM
> To: Raghava Aditya Renukunta
> <RaghavaAditya.Renukunta@microsemi.com>
> Cc: linux-scsi@vger.kernel.org
> 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)?
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] scsi: aacraid: Added support for hotplug
2017-02-13 19:39 ` Raghava Aditya Renukunta
@ 2017-02-14 7:43 ` Dan Carpenter
2017-02-14 19:04 ` Raghava Aditya Renukunta
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2017-02-14 7:43 UTC (permalink / raw)
To: Raghava Aditya Renukunta; +Cc: linux-scsi@vger.kernel.org
On Mon, Feb 13, 2017 at 07:39:15PM +0000, Raghava Aditya Renukunta wrote:
> Hi Don,
>
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Monday, February 13, 2017 10:47 AM
> > To: Raghava Aditya Renukunta
> > <RaghavaAditya.Renukunta@microsemi.com>
> > Cc: linux-scsi@vger.kernel.org
> > 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
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [bug report] scsi: aacraid: Added support for hotplug
2017-02-14 7:43 ` Dan Carpenter
@ 2017-02-14 19:04 ` Raghava Aditya Renukunta
0 siblings, 0 replies; 4+ messages in thread
From: Raghava Aditya Renukunta @ 2017-02-14 19:04 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-scsi@vger.kernel.org
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, February 13, 2017 11:44 PM
> To: Raghava Aditya Renukunta
> <RaghavaAditya.Renukunta@microsemi.com>
> Cc: linux-scsi@vger.kernel.org
> 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@oracle.com]
> > > Sent: Monday, February 13, 2017 10:47 AM
> > > To: Raghava Aditya Renukunta
> > > <RaghavaAditya.Renukunta@microsemi.com>
> > > Cc: linux-scsi@vger.kernel.org
> > > 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-02-14 19:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-13 18:47 [bug report] scsi: aacraid: Added support for hotplug Dan Carpenter
2017-02-13 19:39 ` Raghava Aditya Renukunta
2017-02-14 7:43 ` Dan Carpenter
2017-02-14 19:04 ` Raghava Aditya Renukunta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox