linux-kernel - Re: [PATCH v2 12/12] media: fsd-csis: Add support for FSD CSIS DMA

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: <4af38093-007e-4bfc-8439-0c3dc84012d8@kernel.org>
Date: Sat, 23 Aug 2025 17:34:33 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Inbaraj E <inbaraj.e@...sung.com>, mturquette@...libre.com,
 sboyd@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 s.nawrocki@...sung.com, s.hauer@...gutronix.de, shawnguo@...nel.org,
 cw00.choi@...sung.com, rmfrfs@...il.com, laurent.pinchart@...asonboard.com,
 martink@...teo.de, mchehab@...nel.org, linux-fsd@...la.com, will@...nel.org,
 catalin.marinas@....com, pankaj.dubey@...sung.com, shradha.t@...sung.com,
 ravi.patel@...sung.com
Cc: linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, alim.akhtar@...sung.com,
 linux-samsung-soc@...r.kernel.org, kernel@...i.sm, kernel@...gutronix.de,
 festevam@...il.com, linux-media@...r.kernel.org, imx@...ts.linux.dev,
 linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 12/12] media: fsd-csis: Add support for FSD CSIS DMA

On 23/08/2025 13:49, Inbaraj E wrote:
>>
>>> +
>>> +	ret = fsd_csis_clk_get(csis);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	pm_runtime_enable(dev);
>>> +	if (!pm_runtime_enabled(dev)) {
>>
>> That's odd code. Why?
>>
>>> +		ret = fsd_csis_runtime_resume(dev);
>>
>> Even more questions why?
> 
> If CONFIG_PM is enabled, the clocks are enabled manually in the
> driver through fsd_csis_runtime_resume API.
> 
> If CONFIG_PM is enabled, the clocks are managed through the PM
> runtime framework.
> 
> Can you please help me understand what wrong here?

I think I see such code for the first time, so wrong is doing something
common in completely unusual way.

> 
>>
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, csis);
>>> +
>>> +	ret = fsd_csis_enable_pll(csis);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = fsd_csis_media_init(csis);
>>> +	if (ret)
>>> +		return ret;
>>
>> I think you miss clean up of csis->pll completely. Just use
>> devm_clk_get_enabled and convert everything here to devm.
>>
>>
> 
> I'll fix in next patchset.
> 
>>> +
>>> +	ret = fsd_csis_async_register(csis);
>>> +	if (ret)
>>> +		goto err_media_cleanup;
>>> +
>>> +	return 0;
>>> +
>>> +err_media_cleanup:
>>> +	fsd_csis_media_cleanup(csis);
>>
>> Also this...
>>
> 
> if fsd_csis_media_init fails, the cleanup is handled internally.

What does it mean internally?

> Here, cleanup is used only for fsd_csis_async_register failure.
> 
> can you please help me understand what is wrong here?

Yeah, you leak clock resources.



Best regards,
Krzysztof