From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B15CAC4708E for ; Mon, 5 Dec 2022 09:32:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=MEuM7cPCJTfj/NpgzHd4JGMRaSwjqo39X0sGHXyT59E=; b=p/iFK7uN1HehVuD2DdAGisUYVV 9Oytlml+mCMPloEjrn+cvnd9R7vOywxN+7WtBeDK41NoPHKjXmCPd3UhSW/eHLgDFo8yo9Ug/LOK7 oE6e9AKnuCeOgfx+UmP2T2U3NmD2qT8ZA7mR8rj3rJA7SMepGIUkLuCIPHZxaCwWJYSVYgGVWH9ig csHCVitr2oC5N00lb33tgjWaBH+3ztj76GloO23qGu7UPrWUrFh8h3EGjy3ho3h/pdJkF+d95MoTE VozuZ1/QZ6u/EYQ+UIK7KqkmDIXBv8D+vnb5TULtVnIKmIxTGCSLTTJ/sybalGaK3qG+27glcRgpm 40uJDGRA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1p27pg-00H9Tz-K4; Mon, 05 Dec 2022 09:32:16 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1p27pa-00H9Ii-EV for linux-mediatek@lists.infradead.org; Mon, 05 Dec 2022 09:32:14 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id CB0E5B80D5F; Mon, 5 Dec 2022 09:32:08 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 157C6C433C1; Mon, 5 Dec 2022 09:32:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1670232727; bh=WOC4NGSKcpE3psehYws6xoUdfp+/+TNrXzZHo7XVmas=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=iG0XrvLqHIBCkpesgAG8ZeHH6uezRpKKkjKF9WI4v0UClNi5HBuISupRm/Ef+8Tik Njn2VtTYhiFN7uEc++EJQL0fbANBwl+ktGrYI/8VZlpLHQ9h6rlhcynpYyZJ1xcUq6 vV1qXs1hVmUTr0c7cNWbUEIDICFhLRxGPNokTjoo0O7N8DHWin8qE3bkE79QXknXKl Qv3CaKYDH7cjy5pxsLn41lNSLydVBV4TVAqqsbNNRIai2GCGyk1PC74G5NLWkQOylt AN+wGUfM6NMRajwivjHyLOVksmaFKfFCssOs9oZnpcw9TxNx3bV1Y/woXoHSqePkZt aqktQI2JhZUUQ== Date: Mon, 5 Dec 2022 11:32:03 +0200 From: Leon Romanovsky To: Lorenzo Bianconi Cc: netdev@vger.kernel.org, nbd@nbd.name, john@phrozen.org, sean.wang@mediatek.com, Mark-MC.Lee@mediatek.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, matthias.bgg@gmail.com, linux-mediatek@lists.infradead.org, lorenzo.bianconi@redhat.com Subject: Re: [PATCH net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221205_013210_804991_9270B1D0 X-CRM114-Status: GOOD ( 30.44 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Mon, Dec 05, 2022 at 10:04:07AM +0100, Lorenzo Bianconi wrote: > On Dec 05, Leon Romanovsky wrote: > > On Sun, Dec 04, 2022 at 04:09:21PM +0100, Lorenzo Bianconi wrote: > > > > On Fri, Dec 02, 2022 at 06:36:33PM +0100, Lorenzo Bianconi wrote: > > > > > Introduce __mtk_wed_detach() in order to avoid a possible deadloc= k in > > > > > mtk_wed_attach routine if mtk_wed_wo_init fails. > > > > >=20 > > > > > Fixes: 4c5de09eb0d0 ("net: ethernet: mtk_wed: add configure wed w= o support") > > > > > Signed-off-by: Lorenzo Bianconi > > > > > --- > > > > > drivers/net/ethernet/mediatek/mtk_wed.c | 24 ++++++++++++++-= ------ > > > > > drivers/net/ethernet/mediatek/mtk_wed_mcu.c | 10 ++++++--- > > > > > drivers/net/ethernet/mediatek/mtk_wed_wo.c | 3 +++ > > > > > 3 files changed, 26 insertions(+), 11 deletions(-) > > > >=20 > > > > <...> > > > >=20 > > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c b/driver= s/net/ethernet/mediatek/mtk_wed_mcu.c > > > > > index f9539e6233c9..b084009a32f9 100644 > > > > > --- a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c > > > > > +++ b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c > > > > > @@ -176,6 +176,9 @@ int mtk_wed_mcu_send_msg(struct mtk_wed_wo *w= o, int id, int cmd, > > > > > u16 seq; > > > > > int ret; > > > > > =20 > > > > > + if (!wo) > > > > > + return -ENODEV; > > > >=20 > > > > <...> > > > >=20 > > > > > static void > > > > > mtk_wed_wo_hw_deinit(struct mtk_wed_wo *wo) > > > > > { > > > > > + if (!wo) > > > > > + return; > > > >=20 > > > > How are these changes related to the written in deadlock? > > > > How is it possible to get internal mtk functions without valid wo? > > >=20 > > > Hi Leon, > > >=20 > > > if mtk_wed_rro_alloc() fails in mtk_wed_attach(), we will end up runn= ing > > > __mtk_wed_detach() when wo struct is not allocated yet (wo is allocat= ed in > > > mtk_wed_wo_init()). > >=20 > > IMHO, it is a culprit, proper error unwind means that you won't call to > > uninit functions for something that is not initialized yet. It is better > > to fix it instead of adding "if (!wo) ..." checks. >=20 > So, iiuc, you would prefer to do something like: >=20 > __mtk_wed_detach() > { > ... > if (mtk_wed_get_rx_capa(dev) && wo) { > mtk_wed_wo_reset(dev); > mtk_wed_free_rx_rings(dev); > mtk_wed_wo_deinit(hw); > } > ... > =09 > Right? I am fine both ways :) Yes >=20 > >=20 > > > Moreover __mtk_wed_detach() can run mtk_wed_wo_reset() and mtk_wed_wo= _deinit() > >=20 > > This is another side of same coin. If you can run them in parallel, you > > need locking protection and ability to cancel work, so nothing is going > > to be executed once cleanup succeeded. >=20 > Sorry, I did not get what you mean here with 'in parallel'. __mtk_wed_det= ach() > always run with hw_lock mutex help in both mtk_wed_attach() or > mtk_wed_detach(). Lock is not enough, you need to make sure that no underlying code is called without wo. You suggestion above is fine. The less low level code will have "if (!wo) ...", the better will be. Thanks >=20 > Regards, > Lorenzo >=20 > >=20 > > These were my 2 cents, totally IMHO. > >=20 > > Thanks