On 07/23/2016 09:24 AM, Ivan Khoronzhuk wrote:
>
>
> On 22.07.16 16:58, Grygorii Strashko wrote:
>> Fix deadlock in cpdma_ctlr_destroy() which is triggered now on
>> cpsw module removal:
>> cpsw_remove()
>> - cpdma_ctlr_destroy()
>> - spin_lock_irqsave(&ctlr->lock, flags)
>> - cpdma_ctlr_stop()
>> - spin_lock_irqsave(&ctlr->lock, flags); <- deadlock
>> - cpdma_chan_destroy()
>> - spin_lock_irqsave(&ctlr->lock, flags); <- deadlock
>>
>> The issue has not been observed before because CPDMA channels have
>> been destroyed manually by CPSW until commit d941ebe88a41 ("net:
>> ethernet: ti: cpsw: use destroy ctlr to destroy channels") was merged.
>>
>> Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
>> ---
>> drivers/net/ethernet/ti/davinci_cpdma.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c
>> b/drivers/net/ethernet/ti/davinci_cpdma.c
>> index a68652a..89242e9 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> @@ -436,7 +436,6 @@ int cpdma_ctlr_destroy(struct cpdma_ctlr *ctlr)
>> if (!ctlr)
>> return -EINVAL;
>>
>> - spin_lock_irqsave(&ctlr->lock, flags);
> Should ctlr->state be checked under lock?
> Seems like here should be used unlocked static versions of
> cpdma_ctlr_stop() and cpdma_chan_destroy() instead.
As per my understanding it's not expected the ctlr->state will be changed at
this
moment as all net devices has been unregistered already.
>
>> if (ctlr->state != CPDMA_STATE_IDLE)
May be I can move above check in cpdma_ctlr_stop() instead.
What do you think?
>> cpdma_ctlr_stop(ctlr);
>>
>> @@ -444,7 +443,6 @@ int cpdma_ctlr_destroy(struct cpdma_ctlr *ctlr)
>> cpdma_chan_destroy(ctlr->channels[i]);
>>
>> cpdma_desc_pool_destroy(ctlr->pool);
>> - spin_unlock_irqrestore(&ctlr->lock, flags);
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(cpdma_ctlr_destroy);
>>
>
--
regards,
-grygorii
