| 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
| ||
|
Date: Mon, 1 Feb 2021 15:50:53 +0000
From: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To: Vinod Koul <vkoul@...nel.org>
Cc: yung-chuan.liao@...ux.intel.com,
pierre-louis.bossart@...ux.intel.com, sanyog.r.kale@...el.com,
linux-kernel@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH 5/6] soundwire: qcom: update register read/write routine
On 01/02/2021 14:26, Vinod Koul wrote:
> On 29-01-21, 17:32, Srinivas Kandagatla wrote:
>> In the existing code every soundwire register read and register write
>> are kinda blocked. Each of these are using a special command id that
>> generates interrupt after it successfully finishes. This is really
>> overhead, limiting and not really necessary unless we are doing
>> something special.
>>
>> We can simply read/write the fifo that should also give exactly
>> what we need! This will also allow to read/write registers in
>> interrupt context, which was not possible with the special
>
> Okay but then why use a mutex ?
Read and writes can come from two places,
1> from SoundWire Core itself which takes care of locking
2> from interrupt handler in the driver itself, which is why we need an
additional locking!
Having said that I did have look at the current state of driver
interrupt handler, and it does not do any soundwire register read/writes
from interrupt handler so we can remove the lock for now!
>
>> command approach.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>> ---
>> drivers/soundwire/qcom.c | 148 +++++++++++++++++++++++++--------------
>> 1 file changed, 96 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index 83df15d83935..d61b204dc284 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -78,13 +78,15 @@
>> #define SWRM_SPECIAL_CMD_ID 0xF
>> #define MAX_FREQ_NUM 1
>> #define TIMEOUT_MS (2 * HZ)
>> -#define QCOM_SWRM_MAX_RD_LEN 0xf
>> +#define QCOM_SWRM_MAX_RD_LEN 0x1
>> #define QCOM_SDW_MAX_PORTS 14
>> #define DEFAULT_CLK_FREQ 9600000
>> #define SWRM_MAX_DAIS 0xF
>> #define SWR_INVALID_PARAM 0xFF
>> #define SWR_HSTOP_MAX_VAL 0xF
>> #define SWR_HSTART_MIN_VAL 0x0
>> +#define SWR_BROADCAST_CMD_ID 0x0F
>> +#define MAX_FIFO_RD_FAIL_RETRY 3
>>
>> struct qcom_swrm_port_config {
>> u8 si;
>> @@ -104,11 +106,13 @@ struct qcom_swrm_ctrl {
>> struct regmap *regmap;
>> void __iomem *mmio;
>> struct completion *comp;
>> + struct completion broadcast;
>> struct work_struct slave_work;
>> /* read/write lock */
>> spinlock_t comp_lock;
>> /* Port alloc/free lock */
>> struct mutex port_lock;
>> + struct mutex io_lock;
>> struct clk *hclk;
>> u8 wr_cmd_id;
>> u8 rd_cmd_id;
>> @@ -122,6 +126,8 @@ struct qcom_swrm_ctrl {
>> int rows_index;
>> unsigned long dout_port_mask;
>> unsigned long din_port_mask;
>> + u8 rcmd_id;
>> + u8 wcmd_id;
>> struct qcom_swrm_port_config pconfig[QCOM_SDW_MAX_PORTS];
>> struct sdw_stream_runtime *sruntime[SWRM_MAX_DAIS];
>> enum sdw_slave_status status[SDW_MAX_DEVICES];
>> @@ -200,75 +206,111 @@ static int qcom_swrm_cpu_reg_write(struct qcom_swrm_ctrl *ctrl, int reg,
>> return SDW_CMD_OK;
>> }
>>
>> -static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 cmd_data,
>> - u8 dev_addr, u16 reg_addr)
>> +static u32 swrm_get_packed_reg_val(u8 *cmd_id, u8 cmd_data,
>> + u8 dev_addr, u16 reg_addr)
>> {
>> - DECLARE_COMPLETION_ONSTACK(comp);
>> - unsigned long flags;
>> u32 val;
>> - int ret;
>> -
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = ∁
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> - val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
>> - SWRM_SPECIAL_CMD_ID, reg_addr);
>> - ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>> - if (ret)
>> - goto err;
>> -
>> - ret = wait_for_completion_timeout(ctrl->comp,
>> - msecs_to_jiffies(TIMEOUT_MS));
>> + u8 id = *cmd_id;
>>
>> - if (!ret)
>> - ret = SDW_CMD_IGNORED;
>> - else
>> - ret = SDW_CMD_OK;
>> -err:
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = NULL;
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> + if (id != SWR_BROADCAST_CMD_ID) {
>> + if (id < 14)
>> + id += 1;
>> + else
>> + id = 0;
>> + *cmd_id = id;
>> + }
>> + val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, id, reg_addr);
>>
>> - return ret;
>> + return val;
>> }
>>
>> -static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *ctrl,
>> - u8 dev_addr, u16 reg_addr,
>> - u32 len, u8 *rval)
>> +
>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *swrm, u8 cmd_data,
>> + u8 dev_addr, u16 reg_addr)
>> {
>> - int i, ret;
>> +
>> u32 val;
>> - DECLARE_COMPLETION_ONSTACK(comp);
>> - unsigned long flags;
>> + int ret = 0;
>> + u8 cmd_id = 0x0;
>> +
>> + mutex_lock(&swrm->io_lock);
>> + if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>> + cmd_id = SWR_BROADCAST_CMD_ID;
>> + val = swrm_get_packed_reg_val(&cmd_id, cmd_data,
>> + dev_addr, reg_addr);
>> + } else {
>> + val = swrm_get_packed_reg_val(&swrm->wcmd_id, cmd_data,
>> + dev_addr, reg_addr);
>> + }
>>
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = ∁
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_WR_CMD, val);
>>
>> - val = SWRM_REG_VAL_PACK(len, dev_addr, SWRM_SPECIAL_CMD_ID, reg_addr);
>> - ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_RD_CMD, val);
>> - if (ret)
>> - goto err;
>> + /* version 1.3 or less */
>> + if (swrm->version_major == 1 && swrm->version_minor <= 3)
>> + usleep_range(150, 155);
>>
>> - ret = wait_for_completion_timeout(ctrl->comp,
>> - msecs_to_jif
--srini
fies(TIMEOUT_MS));
>> + if (cmd_id == SWR_BROADCAST_CMD_ID) {
>> + /*
>> + * sleep for 10ms for MSM soundwire variant to allow broadcast
>> + * command to complete.
>> + */
>> + ret = wait_for_completion_timeout(&swrm->broadcast, (2 * HZ/10));
>> + if (!ret)
>> + ret = SDW_CMD_IGNORED;
>> + else
>> + ret = SDW_CMD_OK;
>>
>> - if (!ret) {
>> - ret = SDW_CMD_IGNORED;
>> - goto err;
>> } else {
>> ret = SDW_CMD_OK;
>> }
>> + mutex_unlock(&swrm->io_lock);
>> + return ret;
>> +}
>>
>> - for (i = 0; i < len; i++) {
>> - ctrl->reg_read(ctrl, SWRM_CMD_FIFO_RD_FIFO_ADDR, &val);
>> - rval[i] = val & 0xFF;
>> +static int qcom_swrm_cmd_fifo_rd_cmd(struct qcom_swrm_ctrl *swrm,
>> + u8 dev_addr, u16 reg_addr,
>> + u32 len, u8 *rval)
>> +{
>> + u32 val;
>> + u32 retry_attempt = 0;
>> + u32 cmd_data;
>> + int ret = SDW_CMD_OK;
>> +
>> + mutex_lock(&swrm->io_lock);
>> + val = swrm_get_packed_reg_val(&swrm->rcmd_id, len, dev_addr, reg_addr);
>> +
>> + /* wait for FIFO RD to complete to avoid overflow */
>> + usleep_range(100, 105);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
>> + /* wait for FIFO RD CMD complete to avoid overflow */
>> + usleep_range(250, 255);
>> +
>> +retry_read:
>
> do while{} ?
Sure!
>
>> +
>> + swrm->reg_read(swrm, SWRM_CMD_FIFO_RD_FIFO_ADDR, &cmd_data);
>> + rval[0] = cmd_data & 0xFF;
>> +
>> + if ((((cmd_data) & 0xF00) >> 8) != swrm->rcmd_id) {
>> + if (retry_attempt < MAX_FIFO_RD_FAIL_RETRY) {
>> + /* wait 500 us before retry on fifo read failure */
>> + usleep_range(500, 505);
>> + if (retry_attempt == (MAX_FIFO_RD_FAIL_RETRY - 1)) {
>
> why not do this at the end if retry fails, that will make code look
> neater
I agree, will clean this one in next version!
>
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_CMD, 0x1);
>> + swrm->reg_write(swrm, SWRM_CMD_FIFO_RD_CMD, val);
>> + }
>> + retry_attempt++;
>> + goto retry_read;
>> + } else {
>> + dev_err(swrm->dev,
>> + "failed to read fifo: reg: 0x%x, \
>> + rcmd_id: 0x%x, dev_num: 0x%x, cmd_data: 0x%x\n",
>> + reg_addr, swrm->rcmd_id,
>> + dev_addr, cmd_data);
>
> Do you want to log retry as err..?
>
>> + ret = SDW_CMD_IGNORED;
>> + }
>> }
>>
>> -err:
>> - spin_lock_irqsave(&ctrl->comp_lock, flags);
>> - ctrl->comp = NULL;
>> - spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>> + mutex_unlock(&swrm->io_lock);
>>
>> return ret;
>> }
>> @@ -949,6 +991,8 @@ static int qcom_swrm_probe(struct platform_device *pdev)
>> dev_set_drvdata(&pdev->dev, ctrl);
>> spin_lock_init(&ctrl->comp_lock);
>> mutex_init(&ctrl->port_lock);
>> + mutex_init(&ctrl->io_lock);
>> + init_completion(&ctrl->broadcast);
>> INIT_WORK(&ctrl->slave_work, qcom_swrm_slave_wq);
>>
>> ctrl->bus.ops = &qcom_swrm_ops;
>> --
>> 2.21.0
>
Powered by blists - more mailing lists