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

Message-ID: <d41fd615-2e6a-4cfb-90c9-fc0122ea992f@linaro.org>
Date: Mon, 6 Jan 2025 10:20:40 +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 9:33 AM, Tudor Ambarus wrote:
> Hi, Krzysztof,
> 
> On 12/31/24 2:32 PM, Tudor Ambarus wrote:
>>>> diff --git a/drivers/firmware/samsung/Kconfig b/drivers/firmware/samsung/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..750b41342174
>>>> --- /dev/null
>>>> +++ b/drivers/firmware/samsung/Kconfig
>>>> @@ -0,0 +1,14 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>> +
>>>> +config EXYNOS_ACPM_PROTOCOL
>>>> +	tristate "Exynos Alive Clock and Power Manager (ACPM) Message Protocol"
>>>> +	depends on ARCH_EXYNOS || COMPILE_TEST
>>>> +	depends on EXYNOS_MBOX
>>>
>>> Is it build time dependency? No || COMPILE_TEST?
>>
>> There's no build time dependency, I'll drop this line.
>>>
>>> Is it fine when EXYNOS_MBOX is a module?
>>
>> Yes. When the EXYNOS_MBOX module is not loaded, and one tries to load
>> EXYNOS_ACPM_PROTOCOL module, the later will defer probe when requesting
>> the mailbox channels, but that's fine.
>>
> 
> I'll need to select EXYNOS_MBOX, I explain why below.
> 
> cut
> 
>>>> + */
>>>> +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
> 
> I find these necessary too, will add them. try_module_get() must be
> called when the module exists and is alive, otherwise I get a NULL ptr
> dereference. I need a module dependency between acpm-protocol.ko and
> exynos-mailbox.ko.
> 	select EXYNOS_MBOX and
> 	MODULE_SOFTDEP("pre: exynos-mailbox");
> shall do the trick I think.
> 
>>> 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().
> 
> Using platform_get_drvdata() will simplify the code, thanks. It still
> assumes the platform driver exists and is alive, otherwise we get a NULL
> ptr dereference when getting the drvdata. But we'll be safe if I add the
> module dependency.

Ah, MODULE_SOFTDEP is a soft dependency, so I can't use
platform_get_drvdata(), because if someone removes the exynos-mailbox.ko
from the file system for example, the acpm protocol will defer and its
clients still get a NULL ptr dereference when trying to get the handle
(where try_module_get() and platform_get_drvdata() are called). I'm
better off with the list and mutex.