Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755675AbcDLGUR (ORCPT ); Tue, 12 Apr 2016 02:20:17 -0400 Received: from mga01.intel.com ([192.55.52.88]:27009 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750888AbcDLGUP (ORCPT ); Tue, 12 Apr 2016 02:20:15 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,472,1455004800"; d="asc'?scan'208";a="684794060" From: Felipe Balbi To: "Du\, Changbin" Cc: "gregkh\@linuxfoundation.org" , "linux-usb\@vger.kernel.org" , "linux-kernel\@vger.kernel.org" Subject: RE: [PATCH] usb: dwc3: free dwc->regset on dwc3_debugfs_exit In-Reply-To: <0C18FE92A7765D4EB9EE5D38D86A563A05CFE62A@shsmsx102.ccr.corp.intel.com> References: <1460108522-31664-1-git-send-email-changbin.du@intel.com> <87bn5gpo1v.fsf@intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05CFE203@shsmsx102.ccr.corp.intel.com> <87shyso0tp.fsf@intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05CFE28D@shsmsx102.ccr.corp.intel.com> <87mvp0ny81.fsf@intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05CFE62A@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: Tue, 12 Apr 2016 09:18:17 +0300 Message-ID: <877fg3mjti.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: 3253 Lines: 97 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi Changbin, "Du, Changbin" writes: >> Hi, >>=20 >> "Du, Changbin" writes: >> >> >> >> >> > + dwc->regset =3D NULL; >> >> >> >> >> >> setting regset to NULL is unnecessary. We only call >> dwc3_debugfs_exit() >> >> >> when removing the driver. >> >> >> >> >> >> -- >> >> >> Balbi >> >> > I'd like keep this line even it is unnecessary, because It is a goo= d habit to >> >> > Avoid wild pointers. Just like the dwc->root =3D NULL. >> >> >> >> there won't be any wild pointers here, we'll free struct dwc3 *dwc it= self. >> >> >> >> -- >> >> Balbi >> > I agree the dwc will be freed in current code. But the 'free' logical = is out >> > of the debugfs code. They should be treat as some logical independent. >> Per >> > this point, I still think set pointer to null is not bad. For example,= if dwc3 >> core >> > code invoke dwc3_debugfs_exit twice by mistake(just an example case, >> not >> > really), then no crash/impact for the second call. >>=20 >> the second call should crash because it's clearly wrong ;-) If dwc3 ever >> calls dwc3_debugfs_exit() twice, it really deserves to crash. It's >> something so wrong that we want the verbosity and urgency of a kernel >> oops to make sure we fix it ASAP. >>=20 >> If, however, we set it to null, it might be years before we notice >> anything's wrong. >>=20 >> -- >> Balbi > > Hmm, I agree from this point. I will combine this patch with other two pa= tches > (due to their dependency). And I'd like remove the 'dwc->root=3DNULL' as = well, you are creating a dependency that doesn't exist. Please stop that. You should have two separate branches based on v4.6-rc3 (or, if you prefer, one based on my testing/fixes and another based on my testing/next). On one branch you have *only* $subject and you fix *all* the memory leaks. On the other branch you have the other two patches. Ignore the fact that we might have a conflict, that's for git (and maintainers) to handle when they happen. Again, don't create dependencies between fixes for the -rc cycle and changes for the next merge window. > Is it ok for you? yeah, please remove root =3D NULL as that's completely unnecessary, but split these patches in separate branches and fix all memory leaks. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXDJMqAAoJEIaOsuA1yqREqugP/3xCjHNmsL5H6sXVoOrOARFy 3HLzBTOgY7EO3I9Efz5SDwQLanx2fEb8xipeneo6v+eQJbEFCRWrvzx1LsVAbAIy 43WXDJb/0DjmqcImKxh0gTijZSBEBosflrXkeay7QWxjETZRMZLvLfA5e+cw4nCI PgRPn+2kPxXm8r5bRhQioaoWduaEJr7M2Lk2dJCASIilPuirIsqooUyjVroJ/tuw P7+He4XR8VesgHwd62+VmOdDQyKVWbHVis20A/5eDIpajEddRp5XV9kPCNk/YVqg 4yzx9w961q2NOGUsNNzbR9LTf+0yT9pMEY1km41kH41+RnwlAnK5KmapwRy8Gacv M+07D4eW0q7snRk4JUCsHrU+Uzwrb7dHL8KkUC4jtVSR1Z/820Jx3RoSNpr+vEc5 /Vc2uMXP6SFwuwREbR4MsSJMksQcr4MlhvHag94Ehtl2Zc9JMTcPzj4b6PshkXKU a6mjoD51eoCYObhR6MWx5dIVl8Nue3kTVKSaldXUiuE7Hpu4WLNIONTS7mr6bxVJ Q26wFa4bdTxa3rO8eyALmUL2FEwBe+uCEkpVkPczpZZHZar2NLr8OvZe9xmFIwYe d/5sd/25zeAfEq+SKTx3Bdtjdt1+SPwWvuW0Byf+ZD07eSrbZFyXuQnwjVEHZsCM wvldHuQ419c1omNhPTL8 =iZIU -----END PGP SIGNATURE----- --=-=-=--