diff mbox series

[1/7] soundwire: bus: use sdw_update_no_pm when initializing a device

Message ID 20201202204645.23891-2-yung-chuan.liao@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series soundwire/regmap: use _no_pm routines | expand

Commit Message

Bard Liao Dec. 2, 2020, 8:46 p.m. UTC 
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

When a Slave device is resumed, it may resume the bus and restart the
enumeration. During that process, we absolutely don't want to call
regular read/write routines which will wait for the resume to
complete, otherwise a deadlock occurs.

Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/bus.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Vinod Koul Dec. 5, 2020, 7:45 a.m. UTC | #1 
On 03-12-20, 04:46, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> When a Slave device is resumed, it may resume the bus and restart the
> enumeration. During that process, we absolutely don't want to call
> regular read/write routines which will wait for the resume to
> complete, otherwise a deadlock occurs.
> 
> Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')

Change looks okay, but not sure why this is a fix for adding no pm
version?

> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/bus.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index d1e8c3a54976..60c42508c6c6 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -489,6 +489,18 @@ sdw_read_no_pm(struct sdw_slave *slave, u32 addr)
>  		return buf;
>  }
>  
> +static int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
> +{
> +	int tmp;
> +
> +	tmp = sdw_read_no_pm(slave, addr);
> +	if (tmp < 0)
> +		return tmp;
> +
> +	tmp = (tmp & ~mask) | val;
> +	return sdw_write_no_pm(slave, addr, tmp);
> +}
> +
>  /**
>   * sdw_nread() - Read "n" contiguous SDW Slave registers
>   * @slave: SDW Slave
> @@ -1256,7 +1268,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
>  	val = slave->prop.scp_int1_mask;
>  
>  	/* Enable SCP interrupts */
> -	ret = sdw_update(slave, SDW_SCP_INTMASK1, val, val);
> +	ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1, val, val);
>  	if (ret < 0) {
>  		dev_err(slave->bus->dev,
>  			"SDW_SCP_INTMASK1 write failed:%d\n", ret);
> @@ -1271,7 +1283,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
>  	val = prop->dp0_prop->imp_def_interrupts;
>  	val |= SDW_DP0_INT_PORT_READY | SDW_DP0_INT_BRA_FAILURE;
>  
> -	ret = sdw_update(slave, SDW_DP0_INTMASK, val, val);
> +	ret = sdw_update_no_pm(slave, SDW_DP0_INTMASK, val, val);
>  	if (ret < 0)
>  		dev_err(slave->bus->dev,
>  			"SDW_DP0_INTMASK read failed:%d\n", ret);
> -- 
> 2.17.1
Pierre-Louis Bossart Dec. 5, 2020, 2:59 p.m. UTC | #2 
Thanks for the review Vinod.

On 12/5/20 1:45 AM, Vinod Koul wrote:
> On 03-12-20, 04:46, Bard Liao wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> When a Slave device is resumed, it may resume the bus and restart the
>> enumeration. During that process, we absolutely don't want to call
>> regular read/write routines which will wait for the resume to
>> complete, otherwise a deadlock occurs.
>>
>> Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
> 
> Change looks okay, but not sure why this is a fix for adding no pm
> version?

when we added the no_pm version, we missed the two cases below where 
sdw_update() was used and that creates a deadlock. To me that's a 
conceptual bug, we didn't fully use the no_pm versions, hence the Fixes tag.

It's ok to remove the tag if you don't think it's useful/relevant, what 
matters is that we agree on the content.

>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> ---
>>   drivers/soundwire/bus.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
>> index d1e8c3a54976..60c42508c6c6 100644
>> --- a/drivers/soundwire/bus.c
>> +++ b/drivers/soundwire/bus.c
>> @@ -489,6 +489,18 @@ sdw_read_no_pm(struct sdw_slave *slave, u32 addr)
>>   		return buf;
>>   }
>>   
>> +static int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
>> +{
>> +	int tmp;
>> +
>> +	tmp = sdw_read_no_pm(slave, addr);
>> +	if (tmp < 0)
>> +		return tmp;
>> +
>> +	tmp = (tmp & ~mask) | val;
>> +	return sdw_write_no_pm(slave, addr, tmp);
>> +}
>> +
>>   /**
>>    * sdw_nread() - Read "n" contiguous SDW Slave registers
>>    * @slave: SDW Slave
>> @@ -1256,7 +1268,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
>>   	val = slave->prop.scp_int1_mask;
>>   
>>   	/* Enable SCP interrupts */
>> -	ret = sdw_update(slave, SDW_SCP_INTMASK1, val, val);
>> +	ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1, val, val);
>>   	if (ret < 0) {
>>   		dev_err(slave->bus->dev,
>>   			"SDW_SCP_INTMASK1 write failed:%d\n", ret);
>> @@ -1271,7 +1283,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
>>   	val = prop->dp0_prop->imp_def_interrupts;
>>   	val |= SDW_DP0_INT_PORT_READY | SDW_DP0_INT_BRA_FAILURE;
>>   
>> -	ret = sdw_update(slave, SDW_DP0_INTMASK, val, val);
>> +	ret = sdw_update_no_pm(slave, SDW_DP0_INTMASK, val, val);
>>   	if (ret < 0)
>>   		dev_err(slave->bus->dev,
>>   			"SDW_DP0_INTMASK read failed:%d\n", ret);
>> -- 
>> 2.17.1
>
Vinod Koul Dec. 7, 2020, 4:43 a.m. UTC | #3 
On 05-12-20, 08:59, Pierre-Louis Bossart wrote:
> Thanks for the review Vinod.
> 
> On 12/5/20 1:45 AM, Vinod Koul wrote:
> > On 03-12-20, 04:46, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > When a Slave device is resumed, it may resume the bus and restart the
> > > enumeration. During that process, we absolutely don't want to call
> > > regular read/write routines which will wait for the resume to
> > > complete, otherwise a deadlock occurs.
> > > 
> > > Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
> > 
> > Change looks okay, but not sure why this is a fix for adding no pm
> > version?
> 
> when we added the no_pm version, we missed the two cases below where
> sdw_update() was used and that creates a deadlock. To me that's a conceptual
> bug, we didn't fully use the no_pm versions, hence the Fixes tag.

Documentation says:
"A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
is used to make it easy to determine where a bug originated, which can help
review a bug fix. This tag also assists the stable kernel team in determining
which stable kernel versions should receive your fix. This is the preferred
method for indicating a bug fixed by the patch. See :ref:`describe_changes`
for more details."

I do not this this helps here as this does not help distros etc
I would say this is incremental development which improved a case not
done properly earlier, unless you would like this to be backported.. In
that case it helps folks...

> It's ok to remove the tag if you don't think it's useful/relevant, what
> matters is that we agree on the content.
Pierre-Louis Bossart Dec. 7, 2020, 3:31 p.m. UTC | #4 
On 12/6/20 10:43 PM, Vinod Koul wrote:
> On 05-12-20, 08:59, Pierre-Louis Bossart wrote:
>> Thanks for the review Vinod.
>>
>> On 12/5/20 1:45 AM, Vinod Koul wrote:
>>> On 03-12-20, 04:46, Bard Liao wrote:
>>>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>
>>>> When a Slave device is resumed, it may resume the bus and restart the
>>>> enumeration. During that process, we absolutely don't want to call
>>>> regular read/write routines which will wait for the resume to
>>>> complete, otherwise a deadlock occurs.
>>>>
>>>> Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
>>>
>>> Change looks okay, but not sure why this is a fix for adding no pm
>>> version?
>>
>> when we added the no_pm version, we missed the two cases below where
>> sdw_update() was used and that creates a deadlock. To me that's a conceptual
>> bug, we didn't fully use the no_pm versions, hence the Fixes tag.
> 
> Documentation says:
> "A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
> is used to make it easy to determine where a bug originated, which can help
> review a bug fix. This tag also assists the stable kernel team in determining
> which stable kernel versions should receive your fix. This is the preferred
> method for indicating a bug fixed by the patch. See :ref:`describe_changes`
> for more details."
> 
> I do not this this helps here as this does not help distros etc
> I would say this is incremental development which improved a case not
> done properly earlier, unless you would like this to be backported.. In
> that case it helps folks...

IMHO the changes in the series are absolutely required to have a 
reliable suspend-resume operation and will need to be back-ported. When 
I said 'conceptual bug', I didn't mean a hypothetical case, I really 
meant that a proven race condition and timeouts will occur. That's not 
good... We will provide the list of this patches to distros that are 
known to support SoundWire as a 'must apply'.

If you look at the series, we provided Fixes tags for all patches except 
'cosmetic' ones which don't fundamentally change the behavior (Patch 3, 
patch 7) or when the code has not reached Linus' tree (patch 5).

That said, 5.10 was the first release where SoundWire started to be 
functional so there's no need to apply these patches to earlier versions 
of the stable tree.

Does this help?
Vinod Koul Dec. 8, 2020, 4:56 a.m. UTC | #5 
On 07-12-20, 09:31, Pierre-Louis Bossart wrote:
> On 12/6/20 10:43 PM, Vinod Koul wrote:
> > On 05-12-20, 08:59, Pierre-Louis Bossart wrote:
> > > Thanks for the review Vinod.
> > > 
> > > On 12/5/20 1:45 AM, Vinod Koul wrote:
> > > > On 03-12-20, 04:46, Bard Liao wrote:
> > > > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > 
> > > > > When a Slave device is resumed, it may resume the bus and restart the
> > > > > enumeration. During that process, we absolutely don't want to call
> > > > > regular read/write routines which will wait for the resume to
> > > > > complete, otherwise a deadlock occurs.
> > > > > 
> > > > > Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions')
> > > > 
> > > > Change looks okay, but not sure why this is a fix for adding no pm
> > > > version?
> > > 
> > > when we added the no_pm version, we missed the two cases below where
> > > sdw_update() was used and that creates a deadlock. To me that's a conceptual
> > > bug, we didn't fully use the no_pm versions, hence the Fixes tag.
> > 
> > Documentation says:
> > "A Fixes: tag indicates that the patch fixes an issue in a previous commit. It
> > is used to make it easy to determine where a bug originated, which can help
> > review a bug fix. This tag also assists the stable kernel team in determining
> > which stable kernel versions should receive your fix. This is the preferred
> > method for indicating a bug fixed by the patch. See :ref:`describe_changes`
> > for more details."
> > 
> > I do not this this helps here as this does not help distros etc
> > I would say this is incremental development which improved a case not
> > done properly earlier, unless you would like this to be backported.. In
> > that case it helps folks...
> 
> IMHO the changes in the series are absolutely required to have a reliable
> suspend-resume operation and will need to be back-ported. When I said
> 'conceptual bug', I didn't mean a hypothetical case, I really meant that a
> proven race condition and timeouts will occur. That's not good... We will
> provide the list of this patches to distros that are known to support
> SoundWire as a 'must apply'.
> 
> If you look at the series, we provided Fixes tags for all patches except
> 'cosmetic' ones which don't fundamentally change the behavior (Patch 3,
> patch 7) or when the code has not reached Linus' tree (patch 5).
> 
> That said, 5.10 was the first release where SoundWire started to be
> functional so there's no need to apply these patches to earlier versions of
> the stable tree.
> 
> Does this help?

Yes, that helps, thanks
diff mbox series

Patch

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index d1e8c3a54976..60c42508c6c6 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -489,6 +489,18 @@  sdw_read_no_pm(struct sdw_slave *slave, u32 addr)
 		return buf;
 }
 
+static int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
+{
+	int tmp;
+
+	tmp = sdw_read_no_pm(slave, addr);
+	if (tmp < 0)
+		return tmp;
+
+	tmp = (tmp & ~mask) | val;
+	return sdw_write_no_pm(slave, addr, tmp);
+}
+
 /**
  * sdw_nread() - Read "n" contiguous SDW Slave registers
  * @slave: SDW Slave
@@ -1256,7 +1268,7 @@  static int sdw_initialize_slave(struct sdw_slave *slave)
 	val = slave->prop.scp_int1_mask;
 
 	/* Enable SCP interrupts */
-	ret = sdw_update(slave, SDW_SCP_INTMASK1, val, val);
+	ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1, val, val);
 	if (ret < 0) {
 		dev_err(slave->bus->dev,
 			"SDW_SCP_INTMASK1 write failed:%d\n", ret);
@@ -1271,7 +1283,7 @@  static int sdw_initialize_slave(struct sdw_slave *slave)
 	val = prop->dp0_prop->imp_def_interrupts;
 	val |= SDW_DP0_INT_PORT_READY | SDW_DP0_INT_BRA_FAILURE;
 
-	ret = sdw_update(slave, SDW_DP0_INTMASK, val, val);
+	ret = sdw_update_no_pm(slave, SDW_DP0_INTMASK, val, val);
 	if (ret < 0)
 		dev_err(slave->bus->dev,
 			"SDW_DP0_INTMASK read failed:%d\n", ret);