linux-kernel - Re: [PATCH v3] pwm: atmel: Enable clk when pwm already enabled in bootloader

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: <90830c22-0437-591e-cee0-67b16214bc55@gmail.com>
Date:   Wed, 12 Jul 2023 09:45:08 +0800
From:   Guiting Shen <aarongt.shen@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     claudiu.beznea@...rochip.com, thierry.reding@...il.com,
        nicolas.ferre@...rochip.com, alexandre.belloni@...tlin.com,
        linux-arm-kernel@...ts.infradead.org, linux-pwm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] pwm: atmel: Enable clk when pwm already enabled in
 bootloader

On Wed, Jul 12, 2023 at 04:30:17AM GMT+8, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Jul 12, 2023 at 04:09:05AM +0800, Guiting Shen wrote:
>> +static int atmel_pwm_enable_clk_if_on(struct atmel_pwm_chip *atmel_pwm)
>> +{
>> +	unsigned int i;
>> +	int err;
>> +	u32 sr;
>> +
>> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
>> +	if (!sr)
>> +		return 0;
>> +
>> +	for (i = 0; i < atmel_pwm->chip.npwm; i++) {
>> +		if (!(sr & (1 << i)))
>> +			continue;
>> +
>> +		err = clk_enable(atmel_pwm->clk);
>> +		if (err) {
>> +			dev_err(atmel_pwm->chip.dev,
>> +				"failed to enable clock: %pe\n", ERR_PTR(err));
> 
> Here you leak possibly a few enables. While it's not likely that the
> (say) third enable goes wrong, it's also not that hard to handle?!

The driver used the enable_count member of struct clk_core to count the
PWM channels(4 channels). It will enable hardware clock only when one of
the PWM channels becomed on from all PWM channels off which maybe return
error. And in second/third/fourth times to clk_enable(), it just
increased the enable_count of struct clk_core which would never return
error.

It maybe confused at first time to view the code.
Do it need to add something like that: ?

for (i = 0; i < atmel_pwm->chip.npwm; i++) {
	if (!(sr & (1 << i)))
		continue;

	err = clk_enable(atmel_pwm->clk);
	if (err) {
		dev_err(atmel_pwm->chip.dev,
			"failed to enable clock: %pe\n", ERR_PTR(err));

		for (i = 0; i < cnt; i++)
			clk_disable(atmel_pwm->clk);
		return err;
	}
	cnt++;
}

-- 
Regards,
Guiting Shen