| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20200320134112.GC4885@vkoul-mobl>
Date: Fri, 20 Mar 2020 19:11:12 +0530
From: Vinod Koul <vkoul@...nel.org>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc: alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
jank@...ence.com, srinivas.kandagatla@...aro.org,
slawomir.blauciak@...el.com,
Bard liao <yung-chuan.liao@...ux.intel.com>,
Rander Wang <rander.wang@...ux.intel.com>,
Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
Hui Wang <hui.wang@...onical.com>,
Sanyog Kale <sanyog.r.kale@...el.com>
Subject: Re: [PATCH 3/7] soundwire: intel: add mutex to prevent concurrent
access to SHIM registers
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@...ux.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...
--
~Vinod
Powered by blists - more mailing lists