Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754123AbcDKLZU (ORCPT ); Mon, 11 Apr 2016 07:25:20 -0400 Received: from mga03.intel.com ([134.134.136.65]:44396 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753411AbcDKLZT (ORCPT ); Mon, 11 Apr 2016 07:25:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,462,1455004800"; d="asc'?scan'208";a="956124399" From: Felipe Balbi To: "Du\, Changbin" Cc: "gregkh\@linuxfoundation.org" , "linux-usb\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" Subject: RE: [PATCH v3 1/2] usb: dwc3: make dwc3_debugfs_init return value be void In-Reply-To: <0C18FE92A7765D4EB9EE5D38D86A563A05CFE237@shsmsx102.ccr.corp.intel.com> References: <1460108062-31398-1-git-send-email-changbin.du@intel.com> <1460108062-31398-2-git-send-email-changbin.du@intel.com> <878u0kpnod.fsf@intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05CFE237@shsmsx102.ccr.corp.intel.com> User-Agent: Notmuch/0.21+96~g9bbc54b (http://notmuchmail.org) Emacs/25.0.90.3 (x86_64-pc-linux-gnu) Date: Mon, 11 Apr 2016 14:23:21 +0300 Message-ID: <87potwo0d2.fsf@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2630 Lines: 90 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, "Du, Changbin" writes: >> > root =3D debugfs_create_dir(dev_name(dwc->dev), NULL); >> > - if (!root) { >> > - ret =3D -ENOMEM; >> > - goto err0; >> > - } >> > + if (IS_ERR_OR_NULL(root)) >> > + return; >>=20 >> We can definitely keep on going, but I'd still like to know that we >> enabled CONFIG_DEBUG_FS but failed to create a file or a >> directory. Seems like this should read as follows: >>=20 >> if (IS_ERR_OR_NULL(root)) { >> if (!root) >> dev_err(dwc->dev, "Can't create debugfs root\n"); >> return; >> } >>=20 >> ditto to all bellow. >>=20 > Balbi, so you mean we should not let the failure go silent, right? yeah, but only iff CONFIG_DEBUG_FS is enabled. From what I can tell, in case CONFIG_DEBUG_FS is enabled and it fails, it'll return a NULL pointer instead of ERR_PTR() ;-) >> > dwc->root =3D root; >> > >> > dwc->regset =3D kzalloc(sizeof(*dwc->regset), GFP_KERNEL); >> > if (!dwc->regset) { >> > - ret =3D -ENOMEM; >> > - goto err1; >> > + debugfs_remove_recursive(root); >>=20 >> you're now duplicating debugfs_remove_recursive(root) in all braches and >> that's error prone. It's probably better to keep our gotos, but change >> them so they read as follows: >>=20 >> if (!dwc->regset) >> goto err1; >>=20 >> [...] >>=20 >> return; /* this is our successful exit point */ >>=20 >> err1: >> debugfs_remove_recursive(root); >> kfree(dwc->regset); >>=20 >>=20 >> -- >> Balbi > > No, no need anymore. Because no branch share this code now. > Then remove the goto would make code a little clear. fair enough. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXC4kpAAoJEIaOsuA1yqREoQwP/ifgqD2W8r/hwHzqw2tOIl8Q 9tugireGjpsWx1cj5riKoAUygCEIghScdUmHy4fD1da3fSQa2TRauKGNiSHVWeqW 3Rzb54fBDE4uqRdTwpsHPNdQkXsAWgZvxkYT7fpjSUthW8pqGkeTiUvjMFdd5+Lr MpTNa1OBRxAzwyW4T/M12hybvgeBWbABXQhigehprM+Oj9oueGmTU0bflhZgoBW+ FSi4ExSE8sYpJbCiEuVcJPOVG062exkPoJqsJc9RkILn/PKW9MQX2aTzzRyssrc9 c2pcrnRJxbIUL7nmW9iB1VrxnnO60HgJ9wAYxnkNPvZprPwzAfevLWJk1kNJgYn+ UGHvgPtup9+QdwgjwE5KdGw0w3UDCZM/77yFfkAnEnIhWYsEIlXtxdul03oMFy6/ O6yY8zZfoDg8RfKEjzTalpel4Yw/zpzJ6JFhoAI9acT70Oi8DbOKHVdmAAfBOvDI Qm9+U6P2p11r/zUwGo2m3cQwYZFYobdUjFuLJ5JDFO01+HzUJvXhQ8k176CVjL2X 99PBEloPkW1uW089lqyzKtbpgCCQsCoxhvPVGiKqPb3EB4mVh83pC3/8y36X2pCg PNiOyUYEnkfqV0tkME6Lj6lFhZWypKLwYUIS9H2pKB84ZGUC/QWHA4Mg7xw8DG73 Ubx6tPCmIRp1MATmTlol =zwyN -----END PGP SIGNATURE----- --=-=-=--