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 59926CD1284 for ; Tue, 2 Apr 2024 14:05:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To: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=lu5R3NDX0A2djIL8k9RO1UpfA9FWda7WZPnqN2bmZWo=; b=OZicRT9OVH5Iz2 cm7G1o5IM6i/mJhLGFpz9MfJrobCRikttRH3fLTnyTS49kZL0RqwONVcfXS7LVfPGKqtfo/kN/h0e FEyCTH8eHIPdEqTqOjPzzGQlfYlTMObHN75bEtBci2xY53NjhcbIT1gA0adQ4ew4pIF1eBKvFImYX PG4cA7DUDdh27CU+5Zl9lA1mYH/eDCjCKzGfBxMQpxBIE5mElI6HweQTvXMm5JJ/0s8jhI5apognL x9nMo68q+7m82VhqVyfaf7xYH/iMElqmMwlWhXL+7sngfAtGfWsnHlnr+mr7UjxtwATL4ufeGkuEh vlCxeeF1f3sYLRcx3CWQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rrelX-0000000BV4V-1Kev; Tue, 02 Apr 2024 14:05:31 +0000 Received: from mgamail.intel.com ([198.175.65.12]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rrelU-0000000BV31-1Clg for linux-arm-kernel@lists.infradead.org; Tue, 02 Apr 2024 14:05:29 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1712066728; x=1743602728; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=Z8Q+FvrgVLtKZvinSRjIAzqcwxHLMko61zcYomKVbtY=; b=fTYVmV1yhcI+TwVc2dGK6dvLE2KPqF6qV8IskuTnifx3P4BI+MikfPA7 IsnIbNSQma8NNcXhRz9QLqEzuLGsAojezUTX+bsuJuVALEEig16Pi4s5k sVARdH6PqqWLw/MjOdFHGoMyiviYAl2tUNjIDnjTVstZCJiFt3qgKbucW nSFEhzyoxadVTLSTFL4QzJnXEVbmyrdSkXiXfhdhd5LwkFyEzLLUjKqyR Y2GPPV0KbVfNvYVcrvGreBb7fN+81hJOvKG+hd62ddqQrA3EUEkbz/kS/ ebslKiN/mSV4P3ODRsUMNiHEd8XyFP9PfT0WeE3rn+tJ3K+wbUOhFKrOD A==; X-CSE-ConnectionGUID: nrfMnKJQTC6ObJdw6Somzw== X-CSE-MsgGUID: ctfBkKlWRsKAjnkJAFBsJg== X-IronPort-AV: E=McAfee;i="6600,9927,11032"; a="18688673" X-IronPort-AV: E=Sophos;i="6.07,175,1708416000"; d="scan'208";a="18688673" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orvoesa104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Apr 2024 07:05:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,11032"; a="915142121" X-IronPort-AV: E=Sophos;i="6.07,175,1708416000"; d="scan'208";a="915142121" Received: from smile.fi.intel.com (HELO smile) ([10.237.72.54]) by fmsmga002.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Apr 2024 07:05:21 -0700 Received: from andy by smile with local (Exim 4.97) (envelope-from ) id 1rrelL-00000000pMt-1Qwx; Tue, 02 Apr 2024 17:05:19 +0300 Date: Tue, 2 Apr 2024 17:05:19 +0300 From: Andy Shevchenko To: Peng Fan Cc: "Peng Fan (OSS)" , Sudeep Holla , Cristian Marussi , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Linus Walleij , Dan Carpenter , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-gpio@vger.kernel.org" , Oleksii Moisieiev Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol protocol basic support Message-ID: References: <20240402-pinctrl-scmi-v7-0-3ea519d12cf7@nxp.com> <20240402-pinctrl-scmi-v7-3-3ea519d12cf7@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240402_070528_440853_C170C235 X-CRM114-Status: GOOD ( 25.28 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Apr 02, 2024 at 01:27:19PM +0000, Peng Fan wrote: > > On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote: ... > > > +#include > > > +#include > > > +#include > > > > Please, follow IWYU principle, a lot of headers are missed. > > ok. I will try to figure out. BTW, is there an easy way to filter > out what is missed? For you is much easier than to me as I'm not familiar with the code. Basically you should know what you wrote :-) But if you are asking about tooling, we would appreciate when somebody comes with a such. > > > +#include "common.h" > > > +#include "protocols.h" ... > > > + ret = scmi_pinctrl_get_pin_info(ph, selector, > > > + &pi->pins[selector]); > > > > It's netter as a single line. > > I try to follow 80 max chars per SCMI coding style. If Sudeep and Cristian > is ok, I could use a single line. It's minor, but even before relaxation of 80 limit it was and is mentioned in the documentation that you may go over if it increases readability. > > > + if (ret) > > > + return ret; > > > + } ... > > > +static int scmi_pinctrl_protocol_init(const struct > > > +scmi_protocol_handle *ph) { > > > + int ret; > > > + u32 version; > > > + struct scmi_pinctrl_info *pinfo; > > > + > > > + ret = ph->xops->version_get(ph, &version); > > > + if (ret) > > > + return ret; > > > + > > > + dev_dbg(ph->dev, "Pinctrl Version %d.%d\n", > > > + PROTOCOL_REV_MAJOR(version), > > PROTOCOL_REV_MINOR(version)); > > > + > > > + pinfo = devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL); > > > + if (!pinfo) > > > + return -ENOMEM; > > > + > > > + ret = scmi_pinctrl_attributes_get(ph, pinfo); > > > + if (ret) > > > + return ret; > > > + > > > + pinfo->pins = devm_kcalloc(ph->dev, pinfo->nr_pins, > > > + sizeof(*pinfo->pins), GFP_KERNEL); > > > + if (!pinfo->pins) > > > + return -ENOMEM; > > > + > > > + pinfo->groups = devm_kcalloc(ph->dev, pinfo->nr_groups, > > > + sizeof(*pinfo->groups), GFP_KERNEL); > > > + if (!pinfo->groups) > > > + return -ENOMEM; > > > + > > > + pinfo->functions = devm_kcalloc(ph->dev, pinfo->nr_functions, > > > + sizeof(*pinfo->functions), > > GFP_KERNEL); > > > + if (!pinfo->functions) > > > + return -ENOMEM; > > > + > > > + pinfo->version = version; > > > + > > > + return ph->set_priv(ph, pinfo, version); > > > > Same comments as per previous version. devm_ here is simply wrong. > > It breaks the order of freeing resources. > > > > I.o.w. I see *no guarantee* that these init-deinit functions will be properly > > called from the respective probe-remove. Moreover the latter one may also > > have its own devm allocations (which are rightfully placed) and you get > > completely out of control the resource management. > > I see an old thread. > https://lore.kernel.org/linux-arm-kernel/ZJ78hBcjAhiU+ZBO@e120937-lin/#t > > The free in deinit is not to free the ones alloced in init, it is to free the ones > alloced such as in scmi_pinctrl_get_function_info Even messier than I thought. For bare minimum these two should be documented and renamed accordingly that no-one will think that deinit is a counter part of init. -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel