| Message ID | 20170323105931.10455-2-john@metanate.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello John- One quick question below. Apologies if this has been covered, but just want to be sure. On Thu, Mar 23, 2017 at 10:59:28AM +0000, John Keeping wrote: > regmap_update_bits does its own locking and everything else accessed > here is a local variable so there is no need to lock around it. > > Signed-off-by: John Keeping <john@metanate.com> > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > Tested-by: Heiko Stuebner <heiko@sntech.de> > --- > v3: unchanged > v2.1: > - Remove RK2928 locking in rockchip_set_pull() > v2: > - Also remove locking in rockchip_set_schmitt() > --- > drivers/pinctrl/pinctrl-rockchip.c | 33 ++------------------------------- > 1 file changed, 2 insertions(+), 31 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c > index bd4b63f66220..6568c867bdcd 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c [..] > @@ -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? Julia
On Thu, 23 Mar 2017 11:10:20 -0500, Julia Cartwright wrote: > One quick question below. Apologies if this has been covered, but just > want to be sure. > > On Thu, Mar 23, 2017 at 10:59:28AM +0000, John Keeping wrote: > > regmap_update_bits does its own locking and everything else accessed > > here is a local variable so there is no need to lock around it. > > > > Signed-off-by: John Keeping <john@metanate.com> > > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > --- > > v3: unchanged > > v2.1: > > - Remove RK2928 locking in rockchip_set_pull() > > v2: > > - Also remove locking in rockchip_set_schmitt() > > --- > > drivers/pinctrl/pinctrl-rockchip.c | 33 ++------------------------------- > > 1 file changed, 2 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c > > index bd4b63f66220..6568c867bdcd 100644 > > --- a/drivers/pinctrl/pinctrl-rockchip.c > > +++ b/drivers/pinctrl/pinctrl-rockchip.c > [..] > > @@ -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? [1] https://www.spinics.net/lists/arm-kernel/msg568925.html John
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: > > One quick question below. Apologies if this has been covered, but just > > want to be sure. > > > > On Thu, Mar 23, 2017 at 10:59:28AM +0000, John Keeping wrote: > > > regmap_update_bits does its own locking and everything else accessed > > > here is a local variable so there is no need to lock around it. > > > > > > Signed-off-by: John Keeping <john@metanate.com> > > > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > > Tested-by: Heiko Stuebner <heiko@sntech.de> > > > --- > > > v3: unchanged > > > v2.1: > > > - Remove RK2928 locking in rockchip_set_pull() > > > v2: > > > - Also remove locking in rockchip_set_schmitt() > > > --- > > > > > > drivers/pinctrl/pinctrl-rockchip.c | 33 > > > ++------------------------------- > > > 1 file changed, 2 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c > > > b/drivers/pinctrl/pinctrl-rockchip.c index bd4b63f66220..6568c867bdcd > > > 100644 > > > --- a/drivers/pinctrl/pinctrl-rockchip.c > > > +++ b/drivers/pinctrl/pinctrl-rockchip.c > > > > [..] > > > > > @@ -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 :-) Heiko
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
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.
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
On Thu, Mar 23, 2017 at 11:59 AM, John Keeping <john@metanate.com> wrote: > regmap_update_bits does its own locking and everything else accessed > here is a local variable so there is no need to lock around it. > > Signed-off-by: John Keeping <john@metanate.com> > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > Tested-by: Heiko Stuebner <heiko@sntech.de> > --- > v3: unchanged > v2.1: > - Remove RK2928 locking in rockchip_set_pull() > v2: > - Also remove locking in rockchip_set_schmitt() Patch applied. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c index bd4b63f66220..6568c867bdcd 100644 --- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c @@ -652,7 +652,6 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) int iomux_num = (pin / 8); struct regmap *regmap; int reg, ret, mask, mux_type; - unsigned long flags; u8 bit; u32 data, rmask; @@ -701,15 +700,11 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) if (ctrl->iomux_recalc && (mux_type & IOMUX_RECALCED)) ctrl->iomux_recalc(bank->bank_num, pin, ®, &bit, &mask); - spin_lock_irqsave(&bank->slock, flags); - data = (mask << (bit + 16)); rmask = data | (data >> 16); data |= (mux & mask) << bit; ret = regmap_update_bits(regmap, reg, rmask, data); - spin_unlock_irqrestore(&bank->slock, flags); - return ret; } @@ -1135,7 +1130,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank, struct rockchip_pinctrl *info = bank->drvdata; struct rockchip_pin_ctrl *ctrl = info->ctrl; struct regmap *regmap; - unsigned long flags; int reg, ret, i; u32 data, rmask, rmask_bits, temp; u8 bit; @@ -1163,8 +1157,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank, return ret; } - spin_lock_irqsave(&bank->slock, flags); - switch (drv_type) { case DRV_TYPE_IO_1V8_3V0_AUTO: case DRV_TYPE_IO_3V3_ONLY: @@ -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); - spin_unlock_irqrestore(&bank->slock, flags); return ret; case 18 ... 21: /* setting fully enclosed in the second register */ @@ -1203,7 +1192,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank, bit -= 16; break; default: - spin_unlock_irqrestore(&bank->slock, flags); dev_err(info->dev, "unsupported bit: %d for pinctrl drive type: %d\n", bit, drv_type); return -EINVAL; @@ -1215,7 +1203,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank, rmask_bits = RK3288_DRV_BITS_PER_PIN; break; default: - spin_unlock_irqrestore(&bank->slock, flags); dev_err(info->dev, "unsupported pinctrl drive type: %d\n", drv_type); return -EINVAL; @@ -1227,7 +1214,6 @@ static int rockchip_set_drive_perpin(struct rockchip_pin_bank *bank, data |= (ret << bit); ret = regmap_update_bits(regmap, reg, rmask, data); - spin_unlock_irqrestore(&bank->slock, flags); return ret; } @@ -1294,7 +1280,6 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank, struct rockchip_pin_ctrl *ctrl = info->ctrl; struct regmap *regmap; int reg, ret, i, pull_type; - unsigned long flags; u8 bit; u32 data, rmask; @@ -1309,14 +1294,10 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank, switch (ctrl->type) { case RK2928: - spin_lock_irqsave(&bank->slock, flags); - data = BIT(bit + 16); if (pull == PIN_CONFIG_BIAS_DISABLE) data |= BIT(bit); ret = regmap_write(regmap, reg, data); - - spin_unlock_irqrestore(&bank->slock, flags); break; case RV1108: case RK3188: @@ -1339,16 +1320,12 @@ static int rockchip_set_pull(struct rockchip_pin_bank *bank, return ret; } - spin_lock_irqsave(&bank->slock, flags); - /* enable the write to the equivalent lower bits */ data = ((1 << RK3188_PULL_BITS_PER_PIN) - 1) << (bit + 16); rmask = data | (data >> 16); data |= (ret << bit); ret = regmap_update_bits(regmap, reg, rmask, data); - - spin_unlock_irqrestore(&bank->slock, flags); break; default: dev_err(info->dev, "unsupported pinctrl type\n"); @@ -1408,7 +1385,6 @@ static int rockchip_set_schmitt(struct rockchip_pin_bank *bank, struct rockchip_pin_ctrl *ctrl = info->ctrl; struct regmap *regmap; int reg, ret; - unsigned long flags; u8 bit; u32 data, rmask; @@ -1419,16 +1395,11 @@ static int rockchip_set_schmitt(struct rockchip_pin_bank *bank, if (ret) return ret; - spin_lock_irqsave(&bank->slock, flags); - /* enable the write to the equivalent lower bits */ data = BIT(bit + 16) | (enable << bit); rmask = BIT(bit + 16) | BIT(bit); - ret = regmap_update_bits(regmap, reg, rmask, data); - spin_unlock_irqrestore(&bank->slock, flags); - - return ret; + return regmap_update_bits(regmap, reg, rmask, data); } /*