Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking

From: Julia Cartwright
Date: Thu Mar 23 2017 - 16:43:59 EST


On Thu, Mar 23, 2017 at 09:01:43PM +0100, Heiko St?bner wrote:
> Am Donnerstag, 23. März 2017, 13:29:10 CET schrieb Julia Cartwright:
> > On Thu, Mar 23, 2017 at 06:55:50PM +0100, Heiko St?bner wrote:
> > > Am Donnerstag, 23. März 2017, 17:51:53 CET schrieb John Keeping:
> > > > On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote:
[..]
> > > > > > @@ -1185,17 +1177,14 @@ static int rockchip_set_drive_perpin(struct
> > > > > > rockchip_pin_bank *bank,> >
> > > > > >
> > > > > > rmask = BIT(15) | BIT(31);
> > > > > > data |= BIT(31);
> > > > > > ret = regmap_update_bits(regmap, reg, rmask, data);
> > > > > >
> > > > > > - if (ret) {
> > > > > > - spin_unlock_irqrestore(&bank->slock, flags);
> > > > > > + if (ret)
> > > > > >
> > > > > > return ret;
> > > > > >
> > > > > > - }
> > > > > >
> > > > > > rmask = 0x3 | (0x3 << 16);
> > > > > > temp |= (0x3 << 16);
> > > > > > reg += 0x4;
> > > > > > ret = regmap_update_bits(regmap, reg, rmask, temp);
> > > > >
> > > > > Killing the lock here means the writes to to this pair of registers
> > > > > (reg
> > > > > and reg + 4) can be observed non-atomically. Have you convinced
> > > > > yourself that this isn't a problem?
> > > >
> > > > I called it out in v1 [1] since this bit is new since v4.4 where I
> > > > originally wrote this patch, and didn't get any comments about it.
> > > >
> > > > I've convinced myself that removing the lock doesn't cause any problems
> > > > for writing to the hardware: if the lock would prevent writes
> > > > interleaving then it means that two callers are trying to write
> > > > different drive strengths to the same pin, and even with a lock here one
> > > > of them will end up with the wrong drive strength.
> > > >
> > > > But it does mean that a read via rockchip_get_drive_perpin() may see an
> > > > inconsistent state. I think adding a new lock specifically for this
> > > > particular drive strength bit is overkill and I can't find a scenario
> > > > where this will actually matter; any driver setting a pinctrl config
> > > > must already be doing something to avoid racing two configurations
> > > > against each other, mustn't it?
> > >
> > > also, pins can normally only be requested once - see drivers complaining
> > > if
> > > one of their pins is already held by a different driver. So if you really
> > > end up with two things writing to the same drive strength bits, the
> > > driver holding the pins must be really messed up anyway :-)
> >
> > My concern would be if two independent pins' drive strength
> > configuration would walk on each other, because they happen to be
> > configured via the same registers.
> >
> > If that's not possible, then great!
>
> ah sorry that we didn't make that clearer in the beginning, but no that also
> isn't possible. The registers use a "hiword-mask" scheme, so on each write to
> bit (x) the corresponding write-mask in bit (x+16) also needs to be set, so
> the each write will always only affect bits enabled in that way.

Awesome, thanks for clearing things up.

Thanks!
Julia

Attachment: signature.asc
Description: PGP signature