Re: [PATCH] Input: snvs_pwrkey - Add clk handling

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Uwe,

On Tue, Oct 12, 2021 at 09:39:59AM +0200, Uwe Kleine-König wrote:
> Hello Dmitry,
> 
> On Mon, Oct 11, 2021 at 06:48:51PM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 05, 2021 at 10:00:05PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Sep 22, 2021 at 11:43:00AM +0200, Uwe Kleine-König wrote:
> > > > On i.MX7S and i.MX8M* (but not i.MX6*) the pwrkey device has an
> > > > associated clock. Accessing the registers requires that this clock is
> > > > enabled. Binding the driver on at least i.MX7S and i.MX8MP while not
> > > > having the clock enabled results in a complete hang of the machine.
> > > > (This usually only happens if snvs_pwrkey is built as a module and the
> > > > rtc-snvs driver isn't already bound because at bootup the required clk
> > > > is on and only gets disabled when the clk framework disables unused clks
> > > > late during boot.)
> > > > 
> > > > This completes the fix in commit 135be16d3505 ("ARM: dts: imx7s: add
> > > > snvs clock to pwrkey").
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > This patch fixes a hard machine hang that occurs on an i.MX8MP based
> > > machine in ~10% of the boot ups. In my eyes it's suitable to be applied
> > > before v5.14 even.
> > > 
> > > Any feedback on it?
> > 
> > Sorry for the delay. As you may know I strongly dislike dev_err_probe()
> > as it conflates the 2 issue - error printing and noting the deferral
> > event that should be implemented by the resource providers (and I
> > believe Rob had WIP patches to push this reporting down too providers).
> 
> I didn't know your dislike (and I probably will forget it again soon,
> given that there seems to be disagreement among maintainers :-), and
> from your words I don't understand it. The improved idea is that
> devm_clk_get_optional() already registers the deferral event for the
> clk? My first intuition is that this won't work, so I'd like to see the
> WIP series. (Added Rob to Cc.) Someone has a link?

I think this is here:

https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=dev_err-removal

I do not think it adds calls to device_set_deferred_probe_reason() but
that should be trivial to add, given we have device pointer and ID of
the resource, which should be enough to track it.

> 
> Also I don't share that sentiment, given that today
> devm_clk_get_optional() and all the other resource providers don't do
> the necessary stuff for deferral handling, I strongly prefer to use the
> mechanism that is available today (even if it might be possible to
> improve it) instead of open coding it. And if it's only because once the
> improved variant is available it's easier to identify the code locations
> that need adaption if they all use a common function instead of
> identifying something like
> 
> 	clk = devm_clk_get_optional(&pdev->dev, NULL);
> 	if (IS_ERR(clk)) {
> 		error = PTR_ERR(clk);
> 		if (error != -EPROBE_DEFER)
> 			dev_err(pdev->dev, "Failed to get clk: %pe\n", clk)
> 		else
> 			device_set_deferred_probe_reason(dev, oh_I_need_a_struct_va_format_how_do_I_get_this?);

You do not, you happily ignore it and wait for providers to do it for
you instead of forcing the change through all drivers.

> 		return error;
> 	}
> 
> instead of
> 
> 	clk = devm_clk_get_optional(&pdev->dev, NULL);
> 	if (IS_ERR(clk))
> 		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "Failed to get clock\n");
> 	
> Even if the driver does not call device_set_deferred_probe_reason(), the
> additional check for error != -EPROBE_DEFER is ugly, isn't it?

I'd simply do

	clk = devm_clk_get_optional(...);
	error = PTR_ERR_OR_ZERO(clk);
	if (error) {
		dev_err(&pdev->dev, "...", error);
		return error;
	}

> 
> > Could you p lease resubmit with "normal" dev_err()/dev_warn()/etc and I
> > will be happy to apply.
> 
> Is the above the variant you prefer? Maybe without the call to
> device_set_deferred_probe_reason()? Or maybe even without the check for
> -EPROBE_DEFER (which however might result in wrong error messages which
> is IMHO worse than the ugliness of the additional check)?

Why are they wrong? They are not. They would literally say that some
resource was not obtained because it is not ready.

> 
> Please advice. Given that adding clk handling prevents a machine hang,
> I'm willing to add it in the way you prefer, even if I don't agree to
> your reasoning.

The form above would work for me.

Thank you.

-- 
Dmitry