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

[Heiko Stübner <heiko@xxxxxxxxx>]

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html