linux-kernel - Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZthmWoJbjrEmzOzu@smile.fi.intel.com>
Date: Wed, 4 Sep 2024 16:53:30 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Wolfram Sang <wsa@...nel.org>, Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>, Mark Brown <broonie@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Douglas Anderson <dianders@...omium.org>,
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v6 09/12] i2c: of-prober: Add regulator support

On Wed, Sep 04, 2024 at 05:00:11PM +0800, Chen-Yu Tsai wrote:
> This adds regulator management to the I2C OF component prober.
> Components that the prober intends to probe likely require their
> regulator supplies be enabled, and GPIOs be toggled to enable them or
> bring them out of reset before they will respond to probe attempts.
> GPIOs will be handled in the next patch.
> 
> Without specific knowledge of each component's resource names or
> power sequencing requirements, the prober can only enable the
> regulator supplies all at once, and toggle the GPIOs all at once.
> Luckily, reset pins tend to be active low, while enable pins tend to
> be active high, so setting the raw status of all GPIO pins to high
> should work. The wait time before and after resources are enabled
> are collected from existing drivers and device trees.
> 
> The prober collects resources from all possible components and enables
> them together, instead of enabling resources and probing each component
> one by one. The latter approach does not provide any boot time benefits
> over simply enabling each component and letting each driver probe
> sequentially.
> 
> The prober will also deduplicate the resources, since on a component
> swap out or co-layout design, the resources are always the same.
> While duplicate regulator supplies won't cause much issue, shared
> GPIOs don't work reliably, especially with other drivers. For the
> same reason, the prober will release the GPIOs before the successfully
> probed component is actually enabled.

...

> +static int i2c_of_probe_get_regulators(struct device *dev, struct device_node *node,
> +				       struct i2c_of_probe_data *data)
> +{
> +	struct regulator_bulk_data *tmp, *new_regulators;
> +	int ret;
> +
> +	ret = of_regulator_bulk_get_all(dev, node, &tmp);
> +	if (ret < 0) {
> +		return ret;
> +	} else if (ret == 0) {
> +		/*
> +		 * It's entirely possible for a device node to not have
> +		 * regulator supplies. While it doesn't make sense from
> +		 * a hardware perspective, the supplies could be always
> +		 * on or otherwise not modeled in the device tree, but
> +		 * the device would still work.
> +		 */
> +		return ret;
> +	}

	if (ret < 0)
		return ret;

	/*
	 * It's entirely possible for a device node to not have regulator
	 * supplies. While it doesn't make sense from a hardware perspective,
	 * the supplies could be always on or otherwise not modeled in
	 * the device tree, but the device would still work.
	 */
	if (ret == 0)
		return ret;

> +	if (!data->regulators) {
> +		data->regulators = tmp;
> +		data->regulators_num = ret;
> +		return ret;
> +	};
> +
> +	new_regulators = krealloc_array(data->regulators, (data->regulators_num + ret),

Redundant parentheses.

> +					sizeof(*tmp), GFP_KERNEL);
> +	if (!new_regulators) {
> +		regulator_bulk_free(ret, tmp);
> +		return -ENOMEM;
> +	}
> +
> +	data->regulators = new_regulators;
> +	memcpy(&data->regulators[data->regulators_num], tmp, sizeof(*tmp) * ret);

Shouldn't be the size calculated based on the size of the destination?

> +	data->regulators_num += ret;
> +
> +	return ret;
> +}

...

As I said earlier my main concern is that timeout heuristic which seems fragile.
But I have no ideas to propose, leave this to others to comment on / think about.

-- 
With Best Regards,
Andy Shevchenko