diff mbox series

[3/7] soundwire: intel: add mutex to prevent concurrent access to SHIM registers

Message ID 20200311221026.18174-4-pierre-louis.bossart@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series SoundWire: intel: fix SHIM programming sequences | expand

Commit Message

Pierre-Louis Bossart March 11, 2020, 10:10 p.m. UTC 
Some of the SHIM registers exposed fields that are link specific, and
in addition some of the power-related registers (SPA/CPA) take time to
be updated. Uncontrolled access leads to timeouts or errors.

Add a mutex, shared by all links, so that all accesses to such
registers are serialized, and follow a pattern of read-modify-write.

The mutex initialization is done at the higher layer since the same
mutex is used for all links.

GitHub issue: https://github.com/thesofproject/linux/issues/1555
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/intel.c | 37 +++++++++++++++++++++++++++++++------
 drivers/soundwire/intel.h |  2 ++
 2 files changed, 33 insertions(+), 6 deletions(-)

Comments

Vinod Koul March 20, 2020, 1:41 p.m. UTC | #1 
On 11-03-20, 17:10, Pierre-Louis Bossart wrote:
> Some of the SHIM registers exposed fields that are link specific, and
> in addition some of the power-related registers (SPA/CPA) take time to
> be updated. Uncontrolled access leads to timeouts or errors.
> 
> Add a mutex, shared by all links, so that all accesses to such
> registers are serialized, and follow a pattern of read-modify-write.
> 
> The mutex initialization is done at the higher layer since the same
> mutex is used for all links.
> 
> GitHub issue: https://github.com/thesofproject/linux/issues/1555
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/intel.c | 37 +++++++++++++++++++++++++++++++------
>  drivers/soundwire/intel.h |  2 ++
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 1a3b828b03a1..3c271a8044b8 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -286,6 +286,8 @@ static int intel_link_power_up(struct sdw_intel *sdw)
>  	int spa_mask, cpa_mask;
>  	int link_control, ret;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Link power up sequence */
>  	link_control = intel_readl(shim, SDW_SHIM_LCTL);
>  	spa_mask = (SDW_SHIM_LCTL_SPA << link_id);
> @@ -293,6 +295,8 @@ static int intel_link_power_up(struct sdw_intel *sdw)
>  	link_control |=  spa_mask;
>  
>  	ret = intel_set_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -307,6 +311,8 @@ static int intel_shim_init(struct sdw_intel *sdw)
>  	int sync_reg, ret;
>  	u16 ioctl = 0, act = 0;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Initialize Shim */
>  	ioctl |= SDW_SHIM_IOCTL_BKE;
>  	intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
> @@ -351,6 +357,8 @@ static int intel_shim_init(struct sdw_intel *sdw)
>  	sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
>  	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
>  			      SDW_SHIM_SYNC_SYNCCPU);
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		dev_err(sdw->cdns.dev, "Failed to set sync period: %d\n", ret);
>  
> @@ -363,13 +371,15 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>  	unsigned int link_id = sdw->instance;
>  	u16 wake_en, wake_sts;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +	wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
> +
>  	if (wake_enable) {
>  		/* Enable the wakeup */
> -		intel_writew(shim, SDW_SHIM_WAKEEN,
> -			     (SDW_SHIM_WAKEEN_ENABLE << link_id));
> +		wake_en |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
> +		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
>  	} else {
>  		/* Disable the wake up interrupt */
> -		wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
>  		wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id);
>  		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
>  
> @@ -378,6 +388,7 @@ static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
>  		wake_sts |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
>  		intel_writew(shim, SDW_SHIM_WAKESTS_STATUS, wake_sts);
>  	}
> +	mutex_unlock(sdw->link_res->shim_lock);
>  }
>  
>  static int intel_link_power_down(struct sdw_intel *sdw)
> @@ -387,6 +398,8 @@ static int intel_link_power_down(struct sdw_intel *sdw)
>  	void __iomem *shim = sdw->link_res->shim;
>  	u16 ioctl;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Glue logic */
>  	ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));
>  	ioctl |= SDW_SHIM_IOCTL_BKE;
> @@ -403,6 +416,8 @@ static int intel_link_power_down(struct sdw_intel *sdw)
>  	link_control &=  spa_mask;
>  
>  	ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		return ret;
>  
> @@ -630,11 +645,15 @@ static int intel_pre_bank_switch(struct sdw_bus *bus)
>  	if (!bus->multi_link)
>  		return 0;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Read SYNC register */
>  	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
>  	sync_reg |= SDW_SHIM_SYNC_CMDSYNC << sdw->instance;
>  	intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
>  
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	return 0;
>  }
>  
> @@ -649,6 +668,8 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
>  	if (!bus->multi_link)
>  		return 0;
>  
> +	mutex_lock(sdw->link_res->shim_lock);
> +
>  	/* Read SYNC register */
>  	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
>  
> @@ -660,9 +681,10 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
>  	 *
>  	 * So, set the SYNCGO bit only if CMDSYNC bit is set for any Master.
>  	 */
> -	if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK))
> -		return 0;
> -
> +	if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK)) {
> +		ret = 0;
> +		goto unlock;
> +	}
>  	/*
>  	 * Set SyncGO bit to synchronously trigger a bank switch for
>  	 * all the masters. A write to SYNCGO bit clears CMDSYNC bit for all
> @@ -672,6 +694,9 @@ static int intel_post_bank_switch(struct sdw_bus *bus)
>  
>  	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
>  			      SDW_SHIM_SYNC_SYNCGO);
> +unlock:
> +	mutex_unlock(sdw->link_res->shim_lock);
> +
>  	if (ret < 0)
>  		dev_err(sdw->cdns.dev, "Post bank switch failed: %d\n", ret);
>  
> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> index 38b7c125fb10..568c84a80d79 100644
> --- a/drivers/soundwire/intel.h
> +++ b/drivers/soundwire/intel.h
> @@ -15,6 +15,7 @@
>   * @irq: Interrupt line
>   * @ops: Shim callback ops
>   * @dev: device implementing hw_params and free callbacks
> + * @shim_lock: mutex to handle access to shared SHIM registers
>   */
>  struct sdw_intel_link_res {
>  	struct platform_device *pdev;
> @@ -25,6 +26,7 @@ struct sdw_intel_link_res {
>  	int irq;
>  	const struct sdw_intel_ops *ops;
>  	struct device *dev;
> +	struct mutex *shim_lock; /* protect shared registers */

Where is this mutex initialized? Did you test this...
Pierre-Louis Bossart March 20, 2020, 2:07 p.m. UTC | #2 
>> diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
>> index 38b7c125fb10..568c84a80d79 100644
>> --- a/drivers/soundwire/intel.h
>> +++ b/drivers/soundwire/intel.h
>> @@ -15,6 +15,7 @@
>>    * @irq: Interrupt line
>>    * @ops: Shim callback ops
>>    * @dev: device implementing hw_params and free callbacks
>> + * @shim_lock: mutex to handle access to shared SHIM registers
>>    */
>>   struct sdw_intel_link_res {
>>   	struct platform_device *pdev;
>> @@ -25,6 +26,7 @@ struct sdw_intel_link_res {
>>   	int irq;
>>   	const struct sdw_intel_ops *ops;
>>   	struct device *dev;
>> +	struct mutex *shim_lock; /* protect shared registers */
> 
> Where is this mutex initialized? Did you test this...

Dude, we've been testing the heck out of SoundWire.

If you want to see the actual initialization it's in the intel_init.c code:

https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L231

And this was clearly stated in the cover letter:

"Reviewers might object that the code is provided without some required
initializations for mutexes and shim masks, they will be added as part
of the transition to sdw_master_device - still stuck as of 3/11."
Vinod Koul March 23, 2020, 12:18 p.m. UTC | #3 
On 20-03-20, 09:07, Pierre-Louis Bossart wrote:
> 
> > > diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
> > > index 38b7c125fb10..568c84a80d79 100644
> > > --- a/drivers/soundwire/intel.h
> > > +++ b/drivers/soundwire/intel.h
> > > @@ -15,6 +15,7 @@
> > >    * @irq: Interrupt line
> > >    * @ops: Shim callback ops
> > >    * @dev: device implementing hw_params and free callbacks
> > > + * @shim_lock: mutex to handle access to shared SHIM registers
> > >    */
> > >   struct sdw_intel_link_res {
> > >   	struct platform_device *pdev;
> > > @@ -25,6 +26,7 @@ struct sdw_intel_link_res {
> > >   	int irq;
> > >   	const struct sdw_intel_ops *ops;
> > >   	struct device *dev;
> > > +	struct mutex *shim_lock; /* protect shared registers */
> > 
> > Where is this mutex initialized? Did you test this...
> 
> Dude, we've been testing the heck out of SoundWire.
> 
> If you want to see the actual initialization it's in the intel_init.c code:
> 
> https://github.com/thesofproject/linux/blob/9c7487b33072040ab755d32ca173b75151c0160c/drivers/soundwire/intel_init.c#L231

Which doesn't make much sense. A patch should do complete thing. I don't
see a reason why you cannot pull this single line into this patch.

It belongs here, not anywhere else.
diff mbox series

Patch

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 1a3b828b03a1..3c271a8044b8 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -286,6 +286,8 @@  static int intel_link_power_up(struct sdw_intel *sdw)
 	int spa_mask, cpa_mask;
 	int link_control, ret;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Link power up sequence */
 	link_control = intel_readl(shim, SDW_SHIM_LCTL);
 	spa_mask = (SDW_SHIM_LCTL_SPA << link_id);
@@ -293,6 +295,8 @@  static int intel_link_power_up(struct sdw_intel *sdw)
 	link_control |=  spa_mask;
 
 	ret = intel_set_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		return ret;
 
@@ -307,6 +311,8 @@  static int intel_shim_init(struct sdw_intel *sdw)
 	int sync_reg, ret;
 	u16 ioctl = 0, act = 0;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Initialize Shim */
 	ioctl |= SDW_SHIM_IOCTL_BKE;
 	intel_writew(shim, SDW_SHIM_IOCTL(link_id), ioctl);
@@ -351,6 +357,8 @@  static int intel_shim_init(struct sdw_intel *sdw)
 	sync_reg |= SDW_SHIM_SYNC_SYNCCPU;
 	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
 			      SDW_SHIM_SYNC_SYNCCPU);
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		dev_err(sdw->cdns.dev, "Failed to set sync period: %d\n", ret);
 
@@ -363,13 +371,15 @@  static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
 	unsigned int link_id = sdw->instance;
 	u16 wake_en, wake_sts;
 
+	mutex_lock(sdw->link_res->shim_lock);
+	wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
+
 	if (wake_enable) {
 		/* Enable the wakeup */
-		intel_writew(shim, SDW_SHIM_WAKEEN,
-			     (SDW_SHIM_WAKEEN_ENABLE << link_id));
+		wake_en |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
+		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
 	} else {
 		/* Disable the wake up interrupt */
-		wake_en = intel_readw(shim, SDW_SHIM_WAKEEN);
 		wake_en &= ~(SDW_SHIM_WAKEEN_ENABLE << link_id);
 		intel_writew(shim, SDW_SHIM_WAKEEN, wake_en);
 
@@ -378,6 +388,7 @@  static void intel_shim_wake(struct sdw_intel *sdw, bool wake_enable)
 		wake_sts |= (SDW_SHIM_WAKEEN_ENABLE << link_id);
 		intel_writew(shim, SDW_SHIM_WAKESTS_STATUS, wake_sts);
 	}
+	mutex_unlock(sdw->link_res->shim_lock);
 }
 
 static int intel_link_power_down(struct sdw_intel *sdw)
@@ -387,6 +398,8 @@  static int intel_link_power_down(struct sdw_intel *sdw)
 	void __iomem *shim = sdw->link_res->shim;
 	u16 ioctl;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Glue logic */
 	ioctl = intel_readw(shim, SDW_SHIM_IOCTL(link_id));
 	ioctl |= SDW_SHIM_IOCTL_BKE;
@@ -403,6 +416,8 @@  static int intel_link_power_down(struct sdw_intel *sdw)
 	link_control &=  spa_mask;
 
 	ret = intel_clear_bit(shim, SDW_SHIM_LCTL, link_control, cpa_mask);
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		return ret;
 
@@ -630,11 +645,15 @@  static int intel_pre_bank_switch(struct sdw_bus *bus)
 	if (!bus->multi_link)
 		return 0;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Read SYNC register */
 	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
 	sync_reg |= SDW_SHIM_SYNC_CMDSYNC << sdw->instance;
 	intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
 
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	return 0;
 }
 
@@ -649,6 +668,8 @@  static int intel_post_bank_switch(struct sdw_bus *bus)
 	if (!bus->multi_link)
 		return 0;
 
+	mutex_lock(sdw->link_res->shim_lock);
+
 	/* Read SYNC register */
 	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
 
@@ -660,9 +681,10 @@  static int intel_post_bank_switch(struct sdw_bus *bus)
 	 *
 	 * So, set the SYNCGO bit only if CMDSYNC bit is set for any Master.
 	 */
-	if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK))
-		return 0;
-
+	if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK)) {
+		ret = 0;
+		goto unlock;
+	}
 	/*
 	 * Set SyncGO bit to synchronously trigger a bank switch for
 	 * all the masters. A write to SYNCGO bit clears CMDSYNC bit for all
@@ -672,6 +694,9 @@  static int intel_post_bank_switch(struct sdw_bus *bus)
 
 	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
 			      SDW_SHIM_SYNC_SYNCGO);
+unlock:
+	mutex_unlock(sdw->link_res->shim_lock);
+
 	if (ret < 0)
 		dev_err(sdw->cdns.dev, "Post bank switch failed: %d\n", ret);
 
diff --git a/drivers/soundwire/intel.h b/drivers/soundwire/intel.h
index 38b7c125fb10..568c84a80d79 100644
--- a/drivers/soundwire/intel.h
+++ b/drivers/soundwire/intel.h
@@ -15,6 +15,7 @@ 
  * @irq: Interrupt line
  * @ops: Shim callback ops
  * @dev: device implementing hw_params and free callbacks
+ * @shim_lock: mutex to handle access to shared SHIM registers
  */
 struct sdw_intel_link_res {
 	struct platform_device *pdev;
@@ -25,6 +26,7 @@  struct sdw_intel_link_res {
 	int irq;
 	const struct sdw_intel_ops *ops;
 	struct device *dev;
+	struct mutex *shim_lock; /* protect shared registers */
 };
 
 #endif /* __SDW_INTEL_LOCAL_H */