linux-kernel - Re: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410

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: <20240801142802.GA212266@debian>
Date: Thu, 1 Aug 2024 16:28:02 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-pwm@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] pwm: add support for NXPs high-side switch
 MC33XS2410

Am Thu, Aug 01, 2024 at 12:24:28AM +0200 schrieb Uwe Kleine-König:
Hi Uwe,

[...]

> > > > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle)
> > > > +{
> > > > +	if (!period)
> > > > +		return 0;
> > > > +
> > > > +	duty_cycle *= 256;
> > > 
> > > This might overflow.
> > > 
> > 
> > How ? Max period and duty_cycle is checked by the caller and can be
> > maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I
> > miss anything ?
> 
> I didn't notice the check in the caller. Please add a respective comment
> for the next reader who might miss that.
> 
Ok.

> > > > +	duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period);
> > > 
> > > round-closest is most probably wrong. Please test your driver with
> > > PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and
> > > period.
> > 
> > Yes, I should probably round it down. But I tested with PWM_DEBUG enabled
> > and it gave me the best results so far. There are still few cases where
> > there are complaints. I try to fix it.
> 
> I don't have the hardware so I cannot test myself. Please make sure that
> there are no more complaints, at least none you are aware of. PWM_DEBUG
> should be happy if you pick a hardware setting where period is maximal
> but not bigger than requested and then for that given period duty_cycle
> is maximal but not bigger than requested. So typically use round-down
> division in .apply(). In .get_state() you should return a pwm_state that
> makes .apply() write the exact same state as found when .get_state() was
> called. So typically you have to use round-up there.
>

Thanks, this fixed the remaining complaints. Did it exactly as you
suggested.

> > > > +	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
> > > > +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > > > +
> > > > +	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > [...]
> > > > +static int mc33xs2410_probe(struct spi_device *spi)
> > > > +{
> > > > [...]
> > > > +	/* Disable watchdog */
> > > > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> > > > +	if (ret < 0)
> > > > +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
> > > 
> > > Wouldn't the watchdog functionality better be handled by a dedicated
> > > watchdog driver? Disabling it here unconditionally looks wrong.
> > 
> > Yes, would be better. I planned this after this patchset is accepted.
> > Without disabling the watchdog, the device is not able to operate. So I
> > would stick to it for now and come up with a patch later on.
> 
> How long is the default timeout? Don't you need to disable the watchdog
> in the bootloader anyhow?
> 

Default and also maximum timeout is 256ms. My hardware is configured to
let the device stay in reset until the driver puts it out of reset by
setting the associated GPIO. Datasheet refers to it as "Disable mode".
Outputs are turned off.
Without having such reset logic, the device would need to have the
watchdog disabled in the bootloader and continue in "Normal mode" or it
would go into "Safe mode" while booting the kernel almost sure violating
the timeout. Outputs are then controlled by the INx input logic signals.
I think there is no single solution but rather depends on the use case.
There are three modes which the device can be in when the driver is probed:
"Disable", "Safe" and "Normal". All three are handled by this driver,
assuming the watchdog should be disabled.

> If you still think the watchdog should be disabled here, please add a
> comment that it's conceptually wrong to do here, but needed until there
> is a proper watchdog driver.
> 

I will leave a comment, but I'm not sure if I can come up with a good
solution since the maximum timeout is very low with 256ms. For interaction
with userspace it must be at least 1s.
Why is the handling of the watchdog conceptually wrong here, I could use
devm_watchdog_register_device to register it, why can't I just disable it
here ?

> Should this better be a mfd driver then?
> 

I don't thinks so, the watchdog and the outputs belong somehow together.
When the device runs into an timeout it switches into "Safe" mode and the
outputs are controlled by the INx input logic signals.
I thought of a mfd device as a device which provides multiple functions
not really belonging together. Didn't find a clear definition. What do you
think of it ?

Best regards
Dimitri