| Message ID | 1414796095-10107-20-git-send-email-steve_longerbeam@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Steve, Am Freitag, den 31.10.2014, 15:54 -0700 schrieb Steve Longerbeam: > Some cm_reg accesses were not being protected by the IPU spin lock. > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> > --- > drivers/gpu/ipu-v3/ipu-common.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c > index f707d25..d3af206 100644 > --- a/drivers/gpu/ipu-v3/ipu-common.c > +++ b/drivers/gpu/ipu-v3/ipu-common.c > @@ -46,11 +46,16 @@ static inline void ipu_cm_write(struct ipu_soc *ipu, u32 value, unsigned offset) > > void ipu_srm_dp_sync_update(struct ipu_soc *ipu) > { > + unsigned long flags; > u32 val; > > + spin_lock_irqsave(&ipu->lock, flags); > + > val = ipu_cm_read(ipu, IPU_SRM_PRI2); > val |= 0x8; > ipu_cm_write(ipu, val, IPU_SRM_PRI2); > + > + spin_unlock_irqrestore(&ipu->lock, flags); > } > EXPORT_SYMBOL_GPL(ipu_srm_dp_sync_update); This is the only place this register touched, only bit 3 is ever enabled by software. This lock is not needed. > @@ -451,8 +456,17 @@ int ipu_idmac_get_current_buffer(struct ipuv3_channel *channel) > { > struct ipu_soc *ipu = channel->ipu; > unsigned int chno = channel->num; > + unsigned long flags; > + int ret; > + > + spin_lock_irqsave(&ipu->lock, flags); > > - return (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? 1 : 0; > + ret = (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? > + 1 : 0; > + > + spin_unlock_irqrestore(&ipu->lock, flags); > + > + return ret; Dito. This register isn't written partially multiple times under the spinlock anywhere, so there is no gain from this lock around a register read. > } > EXPORT_SYMBOL_GPL(ipu_idmac_get_current_buffer); > > @@ -569,10 +583,14 @@ EXPORT_SYMBOL_GPL(ipu_idmac_wait_busy); > > int ipu_wait_interrupt(struct ipu_soc *ipu, int irq, int ms) > { > - unsigned long timeout; > + unsigned long flags, timeout; > > timeout = jiffies + msecs_to_jiffies(ms); > + > + spin_lock_irqsave(&ipu->lock, flags); > ipu_cm_write(ipu, BIT(irq % 32), IPU_INT_STAT(irq / 32)); > + spin_unlock_irqrestore(&ipu->lock, flags); > + > while (!(ipu_cm_read(ipu, IPU_INT_STAT(irq / 32) & BIT(irq % 32)))) { > if (time_after(jiffies, timeout)) > return -ETIMEDOUT; This is a write to clear register, the only other place it is accessed is by the regmap irq handler. Can you think of a scenario where this lock would protect anything? regards Philipp
On 11/04/2014 09:51 AM, Philipp Zabel wrote: > Hi Steve, > > Am Freitag, den 31.10.2014, 15:54 -0700 schrieb Steve Longerbeam: >> Some cm_reg accesses were not being protected by the IPU spin lock. >> >> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >> --- >> drivers/gpu/ipu-v3/ipu-common.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c >> index f707d25..d3af206 100644 >> --- a/drivers/gpu/ipu-v3/ipu-common.c >> +++ b/drivers/gpu/ipu-v3/ipu-common.c >> @@ -46,11 +46,16 @@ static inline void ipu_cm_write(struct ipu_soc *ipu, u32 value, unsigned offset) >> >> void ipu_srm_dp_sync_update(struct ipu_soc *ipu) >> { >> + unsigned long flags; >> u32 val; >> >> + spin_lock_irqsave(&ipu->lock, flags); >> + >> val = ipu_cm_read(ipu, IPU_SRM_PRI2); >> val |= 0x8; >> ipu_cm_write(ipu, val, IPU_SRM_PRI2); >> + >> + spin_unlock_irqrestore(&ipu->lock, flags); >> } >> EXPORT_SYMBOL_GPL(ipu_srm_dp_sync_update); > This is the only place this register touched, only bit 3 is ever enabled > by software. This lock is not needed. yes, I verified that too. Never mind then. >> @@ -451,8 +456,17 @@ int ipu_idmac_get_current_buffer(struct ipuv3_channel *channel) >> { >> struct ipu_soc *ipu = channel->ipu; >> unsigned int chno = channel->num; >> + unsigned long flags; >> + int ret; >> + >> + spin_lock_irqsave(&ipu->lock, flags); >> >> - return (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? 1 : 0; >> + ret = (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? >> + 1 : 0; >> + >> + spin_unlock_irqrestore(&ipu->lock, flags); >> + >> + return ret; > Dito. This register isn't written partially multiple times under the > spinlock anywhere, so there is no gain from this lock around a register > read. Well, I was thinking get_current_buffer could race with reset_current_buffer, but these functions are doing plain readl() and writel() (i.e. not read-modify-write), so they are atomic operations. So never mind this either. >> } >> EXPORT_SYMBOL_GPL(ipu_idmac_get_current_buffer); >> >> @@ -569,10 +583,14 @@ EXPORT_SYMBOL_GPL(ipu_idmac_wait_busy); >> >> int ipu_wait_interrupt(struct ipu_soc *ipu, int irq, int ms) >> { >> - unsigned long timeout; >> + unsigned long flags, timeout; >> >> timeout = jiffies + msecs_to_jiffies(ms); >> + >> + spin_lock_irqsave(&ipu->lock, flags); >> ipu_cm_write(ipu, BIT(irq % 32), IPU_INT_STAT(irq / 32)); >> + spin_unlock_irqrestore(&ipu->lock, flags); >> + >> while (!(ipu_cm_read(ipu, IPU_INT_STAT(irq / 32) & BIT(irq % 32)))) { >> if (time_after(jiffies, timeout)) >> return -ETIMEDOUT; > This is a write to clear register, the only other place it is accessed > is by the regmap irq handler. Can you think of a scenario where this > lock would protect anything? In this case I do see potential problems. What if thread A calls ipu_wait_interrupt() while thread B is currently looping through irq status bits in ipu_irq_handle()? A has now cleared an irq status before B has had a chance to get to that status bit. Thus the irq status is missed and the irq not handled. That being said this patch will not fix the problem because ipu_wait_interrupt() and ipu_irq_handle() are under different locks. Maybe the solution is to verify ipu_wait_interrupt() is never called on enabled irqs so that it doesn't interfere with irq handling. But I don't know that that is true, is it? Steve
On 11/04/2014 09:51 AM, Philipp Zabel wrote: > Hi Steve, > > Am Freitag, den 31.10.2014, 15:54 -0700 schrieb Steve Longerbeam: >> Some cm_reg accesses were not being protected by the IPU spin lock. >> >> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> >> --- >> drivers/gpu/ipu-v3/ipu-common.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c >> index f707d25..d3af206 100644 >> --- a/drivers/gpu/ipu-v3/ipu-common.c >> +++ b/drivers/gpu/ipu-v3/ipu-common.c >> @@ -46,11 +46,16 @@ static inline void ipu_cm_write(struct ipu_soc *ipu, u32 value, unsigned offset) >> >> void ipu_srm_dp_sync_update(struct ipu_soc *ipu) >> { >> + unsigned long flags; >> u32 val; >> >> + spin_lock_irqsave(&ipu->lock, flags); >> + >> val = ipu_cm_read(ipu, IPU_SRM_PRI2); >> val |= 0x8; >> ipu_cm_write(ipu, val, IPU_SRM_PRI2); >> + >> + spin_unlock_irqrestore(&ipu->lock, flags); >> } >> EXPORT_SYMBOL_GPL(ipu_srm_dp_sync_update); > This is the only place this register touched, only bit 3 is ever enabled > by software. This lock is not needed. yes, I verified that too. Never mind then. >> @@ -451,8 +456,17 @@ int ipu_idmac_get_current_buffer(struct ipuv3_channel *channel) >> { >> struct ipu_soc *ipu = channel->ipu; >> unsigned int chno = channel->num; >> + unsigned long flags; >> + int ret; >> + >> + spin_lock_irqsave(&ipu->lock, flags); >> >> - return (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? 1 : 0; >> + ret = (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? >> + 1 : 0; >> + >> + spin_unlock_irqrestore(&ipu->lock, flags); >> + >> + return ret; > Dito. This register isn't written partially multiple times under the > spinlock anywhere, so there is no gain from this lock around a register > read. Well, I was thinking get_current_buffer could race with reset_current_buffer, but these functions are doing plain readl() and writel() (i.e. not read-modify-write), so they are atomic operations. So never mind this either. >> } >> EXPORT_SYMBOL_GPL(ipu_idmac_get_current_buffer); >> >> @@ -569,10 +583,14 @@ EXPORT_SYMBOL_GPL(ipu_idmac_wait_busy); >> >> int ipu_wait_interrupt(struct ipu_soc *ipu, int irq, int ms) >> { >> - unsigned long timeout; >> + unsigned long flags, timeout; >> >> timeout = jiffies + msecs_to_jiffies(ms); >> + >> + spin_lock_irqsave(&ipu->lock, flags); >> ipu_cm_write(ipu, BIT(irq % 32), IPU_INT_STAT(irq / 32)); >> + spin_unlock_irqrestore(&ipu->lock, flags); >> + >> while (!(ipu_cm_read(ipu, IPU_INT_STAT(irq / 32) & BIT(irq % 32)))) { >> if (time_after(jiffies, timeout)) >> return -ETIMEDOUT; > This is a write to clear register, the only other place it is accessed > is by the regmap irq handler. Can you think of a scenario where this > lock would protect anything? In this case I do see potential problems. What if thread A calls ipu_wait_interrupt() while thread B is currently looping through irq status bits in ipu_irq_handle()? A has now cleared an irq status before B has had a chance to get to that status bit. Thus the irq status is missed and the irq not handled. That being said this patch will not fix the problem because ipu_wait_interrupt() and ipu_irq_handle() are under different locks. Maybe the solution is to verify ipu_wait_interrupt() is never called on enabled irqs so that it doesn't interfere with irq handling. But I don't know that that is true, is it? Steve
diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c index f707d25..d3af206 100644 --- a/drivers/gpu/ipu-v3/ipu-common.c +++ b/drivers/gpu/ipu-v3/ipu-common.c @@ -46,11 +46,16 @@ static inline void ipu_cm_write(struct ipu_soc *ipu, u32 value, unsigned offset) void ipu_srm_dp_sync_update(struct ipu_soc *ipu) { + unsigned long flags; u32 val; + spin_lock_irqsave(&ipu->lock, flags); + val = ipu_cm_read(ipu, IPU_SRM_PRI2); val |= 0x8; ipu_cm_write(ipu, val, IPU_SRM_PRI2); + + spin_unlock_irqrestore(&ipu->lock, flags); } EXPORT_SYMBOL_GPL(ipu_srm_dp_sync_update); @@ -451,8 +456,17 @@ int ipu_idmac_get_current_buffer(struct ipuv3_channel *channel) { struct ipu_soc *ipu = channel->ipu; unsigned int chno = channel->num; + unsigned long flags; + int ret; + + spin_lock_irqsave(&ipu->lock, flags); - return (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? 1 : 0; + ret = (ipu_cm_read(ipu, IPU_CHA_CUR_BUF(chno)) & idma_mask(chno)) ? + 1 : 0; + + spin_unlock_irqrestore(&ipu->lock, flags); + + return ret; } EXPORT_SYMBOL_GPL(ipu_idmac_get_current_buffer); @@ -569,10 +583,14 @@ EXPORT_SYMBOL_GPL(ipu_idmac_wait_busy); int ipu_wait_interrupt(struct ipu_soc *ipu, int irq, int ms) { - unsigned long timeout; + unsigned long flags, timeout; timeout = jiffies + msecs_to_jiffies(ms); + + spin_lock_irqsave(&ipu->lock, flags); ipu_cm_write(ipu, BIT(irq % 32), IPU_INT_STAT(irq / 32)); + spin_unlock_irqrestore(&ipu->lock, flags); + while (!(ipu_cm_read(ipu, IPU_INT_STAT(irq / 32) & BIT(irq % 32)))) { if (time_after(jiffies, timeout)) return -ETIMEDOUT;
Some cm_reg accesses were not being protected by the IPU spin lock. Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com> --- drivers/gpu/ipu-v3/ipu-common.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)