Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220409055135.54921-1-duoming@zju.edu.cn>
In-Reply-To: <20220409055135.54921-1-duoming@zju.edu.cn>
From:   Max Filippov <jcmvbkbc@gmail.com>
Date:   Fri, 8 Apr 2022 23:20:37 -0700
Message-ID: <CAMo8BfKfxA3b2DmRPc4KK1Mn28-xUBaQeu7ZMUTdoZWr4ESGLw@mail.gmail.com>
Subject: Re: [PATCH V2 10/11] arch: xtensa: platforms: Fix deadlock in iss_net_close()
To:     Duoming Zhou <duoming@zju.edu.cn>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "open list:TENSILICA XTENSA PORT (xtensa)" 
        <linux-xtensa@linux-xtensa.org>, Jakub Kicinski <kuba@kernel.org>,
        alexander.deucher@amd.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "David S. Miller" <davem@davemloft.net>,
        Chris Zankel <chris@zankel.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Duoming,

On Fri, Apr 8, 2022 at 10:51 PM Duoming Zhou <duoming@zju.edu.cn> wrote:
> There is a deadlock in iss_net_close(), which is shown
> below:
>
>    (Thread 1)              |      (Thread 2)
>                            | iss_net_open()
> iss_net_close()            |  mod_timer()
>  spin_lock_bh() //(1)      |  (wait a time)
>  ...                       | iss_net_timer()
>  del_timer_sync()          |  spin_lock() //(2)
>  (wait timer to stop)      |  ...
>
> We hold lp->lock in position (1) of thread 1 and use
> del_timer_sync() to wait timer to stop, but timer handler
> also need lp->lock in position (2) of thread 2. As a result,
> iss_net_close() will block forever.

Would block forever in SMP, that's true. But the ISS machine
that uses this driver had never had SMP support, so we
haven't seen it blocking so far.

> This patch extracts del_timer_sync() from the protection of
> spin_lock_bh(), which could let timer handler to obtain
> the needed lock. What`s more, we should remove spin_lock()
> in iss_net_timer(), because there is no resource need to
> protect and spin_lock() may cause deadlock in timer handler.

There's a lot more strange locking in this driver, so I've made
a cleanup series that gets rid of most of it, including what
causes the deadlock mentioned above. I'll post it shortly.

-- 
Thanks.
-- Max