Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1607771

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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!

   Julia

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v4 0/4] pinctrl: rockchip: PREEMPT_RT_FULL fixes John Keeping <john@metanate.com> - 2017-03-23 12:10 +0100
  [PATCH v4 2/4] pinctrl: rockchip: convert to raw spinlock John Keeping <john@metanate.com> - 2017-03-23 12:10 +0100
  [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking John Keeping <john@metanate.com> - 2017-03-23 12:10 +0100
    Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking Julia Cartwright <julia@ni.com> - 2017-03-23 17:20 +0100
      Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking John Keeping <john@metanate.com> - 2017-03-23 19:00 +0100
        Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking Heiko Stübner <heiko@sntech.de> - 2017-03-23 19:00 +0100
          Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking Julia Cartwright <julia@ni.com> - 2017-03-23 19:30 +0100
            Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking Heiko Stübner <heiko@sntech.de> - 2017-03-23 21:10 +0100
              Re: [PATCH v4 1/4] pinctrl: rockchip: remove unnecessary locking Julia Cartwright <julia@ni.com> - 2017-03-23 21:50 +0100

csiph-web