Received-SPF: pass (google.com: domain of linux-kernel+bounces-128048-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Peng Fan <peng.fan@nxp.com>
To: Andy Shevchenko <andriy.shevchenko@intel.com>, "Peng Fan (OSS)"
	<peng.fan@oss.nxp.com>
CC: Sudeep Holla <sudeep.holla@arm.com>, Cristian Marussi
	<cristian.marussi@arm.com>, Rob Herring <robh@kernel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley
	<conor+dt@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Dan
 Carpenter <dan.carpenter@linaro.org>, "linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, "devicetree@vger.kernel.org"
	<devicetree@vger.kernel.org>, "linux-gpio@vger.kernel.org"
	<linux-gpio@vger.kernel.org>, Oleksii Moisieiev <oleksii_moisieiev@epam.com>
Subject: RE: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
Thread-Topic: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
 protocol basic support
Thread-Index: AQHahKOBYTQv1KoiVkioSbRA3lNLQLFU9h6AgAACEtA=
Date: Tue, 2 Apr 2024 13:27:19 +0000
Message-ID:
 <DU0PR04MB941780B4C28DB353C64966F8883E2@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20240402-pinctrl-scmi-v7-0-3ea519d12cf7@nxp.com>
 <20240402-pinctrl-scmi-v7-3-3ea519d12cf7@nxp.com>
 <ZgwEtxj-qi6uy_m2@smile.fi.intel.com>
In-Reply-To: <ZgwEtxj-qi6uy_m2@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0:
 =?us-ascii?Q?AkLa5wqYT/teBhPzwEHU+RZAwCFunnM9ZyBelP2irwUo07feuiJjiY6mbqzI?=
 =?us-ascii?Q?Cyq9LxKhp3EozGtOAHy5qwDSejhKKSk7Z5IWMT5RAdCGUz9xUC/+aOtOIZ5+?=
 =?us-ascii?Q?dKjOpbXwa2EgNSpR1SXkvmvoPz+onfz7kBwO/a7fyn/9PEBNR7qyHTQMHQhU?=
 =?us-ascii?Q?7etiFJe9hLrIud1y6PBgysSBAYxUaqKdpbAJQzojEKvjSUr7rSlInqshtGCH?=
 =?us-ascii?Q?WR4XBQWH7SQZoBeOD4/HzsCmYU8HteS35K7ailra/6boT5jhbBOEDtOoaEXC?=
 =?us-ascii?Q?7UxIHac2VbsUUYyrcmTCc767F4136NCZboYRX1ZRauxoSFK1do2JkO35SJxd?=
 =?us-ascii?Q?TlgWb6X+tQm0bjKMIn397XAK9wvAsLStg8slOTTMyuB6TEhMJv7+liJgx7eV?=
 =?us-ascii?Q?hSUeoFXtv93atsnIe3RvQi7qFp8BPs4Bcp3mgub6Tfn5RtwlXP5RyO3VXnyd?=
 =?us-ascii?Q?H0RAdxeuRy+MOobWDcYES1PXS3wyjwlx93P1qXV5yOKCoEyU70uo1dpu38p8?=
 =?us-ascii?Q?doYk40kjtVL8+SE8vP/S1gBUcHEynUQtArbJ9oz97+q0LdY7SJv6w6o1czAO?=
 =?us-ascii?Q?x2aimLS52RYVRH2/aQajMMRViYIkDdXmNfe4hjiAaDT0viEraenlTniwL/Dy?=
 =?us-ascii?Q?Ly+Ia4ZVqjflaYQ6FaYo3hWEhGPDgcrzO6n19TVSJQ1WHpG/FtCwGsLT3oWw?=
 =?us-ascii?Q?0vtll85botpLqGNWt9oHr+cT4o2/3LuuaA9FKRSo+fQzXzd3hfCImv6nZbxd?=
 =?us-ascii?Q?281wHummW4eScygZQnp8ORWX9PIxaXsCyQbLUXEJbj3Hsyzo0qkXx84kzA3V?=
 =?us-ascii?Q?S09J4opPqwaBWIVq4tb4NnFAM8gtcSWOYzfXScJTELr1ju6C7Pc5j9P+PCIm?=
 =?us-ascii?Q?ZYVD0PpMjDBEEBBrRbN9vAWHDM1Z458PU4LngV2+kV4L7mvKboUa0ZGNnb5P?=
 =?us-ascii?Q?56O3JpegCOiVfXpiSOVBSdAttruVo8zjMfJaul2iSDKwh5F4vQNu3Zcd6qdn?=
 =?us-ascii?Q?9M6DFRkf2mlRvioLtwPN5CvXbOw64yGHDdMO99RFHSzl0YPHFyHT3/EJjGCC?=
 =?us-ascii?Q?/NyBbRrsglpm1YnrWYfpbJZN4ppyWZAwopWiNvwCMh9aRsze8dyp0YgAgZJQ?=
 =?us-ascii?Q?KXRPlfDz3ceDJJrOIVdvJuPjKdnkIBGo774xPqcA22G0RoNbBUwiS81iIoIL?=
 =?us-ascii?Q?KBhcT20Mka/HhdlRXeoNsCeNgSHDL8VvI7PDc59WOL0oBznJVaxroW2CvZyK?=
 =?us-ascii?Q?sS4DcbsEDp/7aO7V2pVnY3PEHLQLVbcrJQm0F8GZ0/Vh2zSFo4RzO2ZwDTV6?=
 =?us-ascii?Q?US6FR+K0wzYtugtR5NwhZiT6JS2lqCmo0hrg2HCsRYywJ0Vj3MHZe6aDbROr?=
 =?us-ascii?Q?XDn+jRc/1nnJBblGgQVx+fgeFQrdz0Kjw0R2Ryu1iXlPcpfkQ+UuaScx1vFL?=
 =?us-ascii?Q?hJlCFq+adPezHrzOAG19GgSFXGC/Uz5yXLS7fyxlDfYEYKMWGJjK0eEF8+5U?=
 =?us-ascii?Q?sfHGNlAKsVBKdqolSCYv0DGBIRJIq3Un4YzY8LKQ81wQ2Se3l/N6++9Yi7Q7?=
 =?us-ascii?Q?4p3BkbJMLqaD2J2tf+4=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b22decdf-2976-4316-a3c3-08dc53189f38
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Apr 2024 13:27:19.4818
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: oSdtxMQGh5WF7HJKP/Jz4Mva37F+h/8kv1FENt0OeL8ndYmwCAiYyNQCJkjp9+vsUDJv1VgXEOTrGfS/8mlcjA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB8058

Hi Andy

> Subject: Re: [PATCH v7 3/4] firmware: arm_scmi: Add SCMI v3.2 pincontrol
> protocol basic support
>=20
> On Tue, Apr 02, 2024 at 10:22:23AM +0800, Peng Fan (OSS) wrote:
>=20
> ...
>=20
> > +#include <linux/module.h>
> > +#include <linux/scmi_protocol.h>
> > +#include <linux/slab.h>
>=20
> 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?

>=20
> > +#include "common.h"
> > +#include "protocols.h"
>=20
> ...
>=20
> > +		ret =3D scmi_pinctrl_get_pin_info(ph, selector,
> > +						&pi->pins[selector]);
>=20
> 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.

>=20
> > +		if (ret)
> > +			return ret;
> > +	}
>=20
> ...
>=20
> > +static int scmi_pinctrl_protocol_init(const struct
> > +scmi_protocol_handle *ph) {
> > +	int ret;
> > +	u32 version;
> > +	struct scmi_pinctrl_info *pinfo;
> > +
> > +	ret =3D 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 =3D devm_kzalloc(ph->dev, sizeof(*pinfo), GFP_KERNEL);
> > +	if (!pinfo)
> > +		return -ENOMEM;
> > +
> > +	ret =3D scmi_pinctrl_attributes_get(ph, pinfo);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pinfo->pins =3D devm_kcalloc(ph->dev, pinfo->nr_pins,
> > +				   sizeof(*pinfo->pins), GFP_KERNEL);
> > +	if (!pinfo->pins)
> > +		return -ENOMEM;
> > +
> > +	pinfo->groups =3D devm_kcalloc(ph->dev, pinfo->nr_groups,
> > +				     sizeof(*pinfo->groups), GFP_KERNEL);
> > +	if (!pinfo->groups)
> > +		return -ENOMEM;
> > +
> > +	pinfo->functions =3D devm_kcalloc(ph->dev, pinfo->nr_functions,
> > +					sizeof(*pinfo->functions),
> GFP_KERNEL);
> > +	if (!pinfo->functions)
> > +		return -ENOMEM;
> > +
> > +	pinfo->version =3D version;
> > +
> > +	return ph->set_priv(ph, pinfo, version);
>=20
> Same comments as per previous version. devm_ here is simply wrong.
> It breaks the order of freeing resources.
>=20
> I.o.w. I see *no guarantee* that these init-deinit functions will be prop=
erly
> 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 t=
he ones
alloced such as in scmi_pinctrl_get_function_info

Thanks,
Peng.

>=20
> > +}
>=20
> --
> With Best Regards,
> Andy Shevchenko
>=20