linux-kernel - Re: [PATCH v5 2/3] firmware: add Exynos ACPM protocol driver

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] [day] [month] [year] [list]

Message-ID: <366ab35f-b881-4694-9d36-573f9922175d@linaro.org>
Date: Tue, 7 Jan 2025 10:50:00 +0000
From: Tudor Ambarus <tudor.ambarus@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>,
 Jassi Brar <jassisinghbrar@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 andre.draszik@...aro.org, kernel-team@...roid.com, willmcvicker@...gle.com,
 peter.griffin@...aro.org, daniel.lezcano@...aro.org,
 vincent.guittot@...aro.org, ulf.hansson@...aro.org, arnd@...db.de
Subject: Re: [PATCH v5 2/3] firmware: add Exynos ACPM protocol driver



On 1/6/25 10:20 AM, Tudor Ambarus wrote:
>>>>> +static const struct acpm_handle *acpm_get_by_phandle(struct device_node *np,
>>>>> +						     const char *property)
>>>>> +{
>>>>> +	struct acpm_handle *handle = NULL;
>>>>> +	struct device_node *acpm_np;
>>>>> +	struct acpm_info *info;
>>>>> +
>>>>> +	if (!np) {
>>>>> +		pr_err("I need a device pointer\n");
>>>>> +		return ERR_PTR(-EINVAL);
>>>>> +	}
>>>>> +
>>>>> +	acpm_np = of_parse_phandle(np, property, 0);
>>>>> +	if (!acpm_np)
>>>>> +		return ERR_PTR(-ENODEV);
>>>>> +
>>>>> +	mutex_lock(&acpm_list_mutex);
>>>>> +	list_for_each_entry(info, &acpm_list, node) {
>>>>> +		if (acpm_np == info->dev->of_node) {
>>>>> +			handle = &info->handle;
>>>>> +			info->users++;
>>>>> +			break;
>>>>> +		}
>>>>> +	}
>>>>> +	mutex_unlock(&acpm_list_mutex);
>>>>> +	of_node_put(acpm_np);
>>>>> +
>>>>
>>>> You also need device links and probably try_module_get. See clk.c
>>>> clk_hw_create_clk() or of_qcom_ice_get(). Interestingly, none of them
>>>> perform both operations, which I think is necessary.
>>>>
>>>> I think you could also avoid entire list and mutex by using
>>>> platform_get_drvdata(), see of_qcom_ice_get().

[snip, irrelevant now]

I made my mind, I think the solution is threefold.
1/ use kref with a release callback so that the firmware drvdata will be
destroyed only when there are no consumers. Example:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1e7c57355a3bc617fc220234889e49fe722a6305

2/ In get_handle() after kref_get_unless_zero() succeeds, I'll be
calling try_module_get() for supplier. Even if kref will make sure that
consumers will still have a valid supplier drvdata, I'd like to delete
the supplier module when there are no consumers left.

3/ use device_link_add() so that when the supplier device unbinds, to
unbind the consumer devices as well.

I'll implement these. Cheers,
ta