Re: [PATCH 4/9] ASoC: cs35l41: Fix broken shared boost activation

On Sun, Sep 03, 2023 at 12:06:16AM +0300, Cristian Ciocaltea wrote:

Enabling the active/passive shared boosts involves writing the MDSYNC UP register sequence, which cannot be performed before receiving the PLL lock signal.

Due to improper error handling, it was not obvious the wait operation times out and, consequently, the shared boost gets never enabled.

Further investigations revealed the signal is triggered while snd_pcm_start() is executed, right after receiving the SNDRV_PCM_TRIGGER_START command, which happens long after the SND_SOC_DAPM_PRE_PMU event handler is invoked as part of snd_pcm_prepare(). That is where cs35l41_global_enable() is called from.

Increasing the wait duration doesn't help, as it only causes an unnecessary delay in the invocation of snd_pcm_start(). Moving the wait and the subsequent regmap operations to the SNDRV_PCM_TRIGGER_START callback is not a solution either, since they would be executed in an IRQ-off atomic context.

Solve the issue by deferring the processing to a workqueue task, which allows to correctly wait for the signal and then safely proceed with the required regmap operations.

Fixes: f5030564938b ("ALSA: cs35l41: Add shared boost feature") Signed-off-by: Cristian Ciocaltea cristian.ciocaltea@collabora.com

---

Thanks for looking at this apologies this was missed in the initial review of the patch.

+int cs35l41_mdsync_up(struct regmap *regmap) +{

• struct reg_sequence cs35l41_mdsync_up_seq[] = {

• {CS35L41_PWR_CTRL3, 0},
  

• {CS35L41_PWR_CTRL1, 0x00000000, 3000},
  

• {CS35L41_PWR_CTRL1, 0x00000001, 3000},
  

• };
• unsigned int pwr_ctrl3, int_status;
• int ret;
• regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);
• pwr_ctrl3 |= CS35L41_SYNC_EN_MASK;
• cs35l41_mdsync_up_seq[0].def = pwr_ctrl3;
• ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_up_seq,

• 		     ARRAY_SIZE(cs35l41_mdsync_up_seq));
  

• if (ret < 0)

• return ret;
  

Is this now safe? By pulling this out into a worker thread, it is no longer under the DAPM lock, which makes me worry this can race with the other uses of PWR_CTRL3 which could theoretically change state between when you read the reg and when you write it.

@@ -1243,33 +1289,27 @@ int cs35l41_global_enable(struct device *dev, struct regmap *regmap, enum cs35l4 cs35l41_mdsync_down_seq[2].def = pwr_ctrl1; ret = regmap_multi_reg_write(regmap, cs35l41_mdsync_down_seq, ARRAY_SIZE(cs35l41_mdsync_down_seq));

• if (ret || !enable)
  

• if (ret)
  break;
  
  

• if (!pll_lock)
  

• 	return -EINVAL;
  

• ret = wait_for_completion_timeout(pll_lock, msecs_to_jiffies(1000));
  

• if (ret == 0) {
  

• 	dev_err(dev, "Timed out waiting for pll_lock\n");
  

• 	return -ETIMEDOUT;
  

• if (enable) {
  

• 	if (mdsync_up_work) {
  

• 		/* Call cs35l41_mdsync_up() after receiving PLL lock signal */
  

• 		schedule_work(mdsync_up_work);
  

• 	} else {
  

• 		dev_err(dev, "MDSYNC UP work not provided\n");
  

• 		ret = -EINVAL;
  

• 	}
  

• 	break;
  

One question I might also have would be does a worker thread make more sense or would it be simpler to do the mdsync power up directly in response to the PLL lock IRQ?

Thanks, Charles