Re: [PATCH v6 2/2] hwspinlock: add sun6i hardware spinlock support

[Maxime Ripard]

On Sat, Mar 06, 2021 at 12:05:22PM +0100, Wilken Gottwalt wrote:
> On Tue, 2 Mar 2021 18:20:02 +0100
> Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> 
> > Hi,
> > 
> > On Mon, Mar 01, 2021 at 03:06:08PM +0100, Wilken Gottwalt wrote:
> > > On Mon, 1 Mar 2021 14:13:05 +0100
> > > Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> > > 
> > > > On Sat, Feb 27, 2021 at 02:03:54PM +0100, Wilken Gottwalt wrote:
> > > > > Adds the sun6i_hwspinlock driver for the hardware spinlock unit found in
> > > > > most of the sun6i compatible SoCs.
> > > > >
> > > > > This unit provides at least 32 spinlocks in hardware. The implementation
> > > > > supports 32, 64, 128 or 256 32bit registers. A lock can be taken by
> > > > > reading a register and released by writing a 0 to it. This driver
> > > > > supports all 4 spinlock setups, but for now only the first setup (32
> > > > > locks) seem to exist in available devices. This spinlock unit is shared
> > > > > between all ARM cores and the embedded companion core. All of them can
> > > > > take/release a lock with a single cycle operation. It can be used to
> > > > > sync access to devices shared by the ARM cores and the companion core.
> > > > >
> > > > > There are two ways to check if a lock is taken. The first way is to read
> > > > > a lock. If a 0 is returned, the lock was free and is taken now. If an 1
> > > > > is returned, the caller has to try again. Which means the lock is taken.
> > > > > The second way is to read a 32bit wide status register where every bit
> > > > > represents one of the 32 first locks. According to the datasheets this
> > > > > status register supports only the 32 first locks. This is the reason the
> > > > > first way (lock read/write) approach is used to be able to cover all 256
> > > > > locks in future devices. The driver also reports the amount of supported
> > > > > locks via debugfs.
> > > > >
> > > > > Signed-off-by: Wilken Gottwalt <wilken.gottwalt@xxxxxxxxxx>
> > > 
> > > Nope, I had to replace the devm_hwspin_lock_register function by the
> > > hwspin_lock_register function because like Bjorn pointed out that it can
> > > fail and needs to handled correctly. And having a devm_* function does not
> > > play well with the non-devm clock/reset functions and winding back if an
> > > error occurs. It also messes with the call order in the remove function. So
> > > I went back to the classic way where I have full control over the call order.
> > 
> > If you're talking about the clock and reset line reassertion, I don't
> > really see what the trouble is. Sure, it's not going to be in the exact
> > same order in remove, but it's still going to execute in the proper
> > order (ie, clock disable, then reset disable, then clock put and reset
> > put). And you can use devm_add_action if you want to handle things
> > automatically.
> 
> See, in v5 zje result of devm_hwspin_lock_register was returned directly. The
> remove callback or the bank_fail/clk_fail labels would not run, if the registering
> fails. In v6 it is fixed.

Yeah, and it's indeed an issue...

> +	platform_set_drvdata(pdev, priv);
> +
> +	return devm_hwspin_lock_register(&pdev->dev, priv->bank, &sun6i_hwspinlock_ops,
> +					 SPINLOCK_BASE_ID, priv->nlocks);
> +bank_fail:
> +	clk_disable_unprepare(priv->ahb_clk);
> +clk_fail:
> +	reset_control_assert(priv->reset);
> +
> +	return err;
> +}
> 
> So, is v6 fine for you even if it uses a more classic approach?

... but completely getting rid of the devm_ calls isn't a solution, if
that's what you're calling the more classic approach.

if (devm_hwspin_lock_register(..))
   goto bank_fail

return 0;

works, and using devm_add_action like I suggested works as well to fix
the issue you mentioned above

Maxime

Attachment: signature.asc
Description: PGP signature