[plbossart]

When the cache_only flag is set, reading from a volatile register
results in -EBUSY. Change the test to allow volatile reads.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

[plbossart]

I shared this in an issue but somehow no one looked into it. Re-sharing as a formal PR for comments.

[bardliao]

@plbossart This doesn't work with the existing sdw drivers. The point is that we resume sdw bus, but do not wait for codec attached when we try to read codec register in suspend. Currently, codec driver will wait for initialization_complete then set cache only false and sync the regmap cache. But we didn't wait for codec attached on sdw bus driver. As a result, we will try to read the data before codec is attached.
Please see below log. I set all registers on rt1308 as volatile to test it.

log

[   36.368593] rt1308 sdw:2:25d:1308:0: bard: c070 is volatile
[   36.368602] rt1308 sdw:2:25d:1308:0: regmap_sdw_read read c070
[   36.368605] soundwire sdw-master-2: bard: sdw_nread start
[   36.491255] intel-sdw intel-sdw.2: bard: intel_resume_runtime start
[   36.491260] intel-sdw intel-sdw.2: intel_link_power_up: powering up all links
[   36.491260] intel-sdw intel-sdw.2: intel_link_power_up: first link up, programming SYNCPRD
[   36.491600] rt1308 sdw:2:25d:1308:0: sdw_modify_slave_status: initializing completion for Slave 1
[   36.491845] intel-sdw intel-sdw.2: bard: intel_resume_runtime ret 0
[   36.492389] intel-sdw intel-sdw.2: Program SCP Ack not received
[   36.492389] intel-sdw intel-sdw.2: Program SCP Ack not received
[   36.492390] soundwire sdw-master-2: trf on Slave 1 failed:-5
[   36.492466] intel-sdw intel-sdw.2: Program SCP Ack not received
[   36.492467] intel-sdw intel-sdw.2: Program SCP Ack not received
[   36.492468] soundwire sdw-master-2: bard: sdw_nread ret -5
[   36.492472] rt1308 sdw:2:25d:1308:0: ASoC: error at soc_component_read_no_lock on sdw:2:25d:1308:0: -5
[   36.492948] soundwire sdw-master-2: Slave attached, programming device number
[   36.493112] soundwire sdw-master-2: SDW Slave Addr: 20025d130800
[   36.493113] soundwire sdw-master-2: SDW Slave class_id 0, part_id 1308, mfg_id 25d, unique_id 0, version 2
[   36.493114] soundwire sdw-master-2: Slave already registered, reusing dev_num:1
[   36.493343] soundwire sdw-master-2: No more devices to enumerate
[   36.493366] rt1308 sdw:2:25d:1308:0: sdw_modify_slave_status: signaling completion for Slave 1
[   36.493366] soundwire sdw-master-2: bard: sdw_nread start
[   36.493419] soundwire sdw-master-2: bard: sdw_nread ret 0
[   36.493490] rt1308 sdw:2:25d:1308:0: bard: rt1308_update_status status=1

[naveen-manohar]

@plbossart still observe the dump with patch, PFA-
dmesg_volatile_regmap_codec_ebusy.log

[plbossart]

@plbossart This doesn't work with the existing sdw drivers. The point is that we resume sdw bus, but do not wait for codec attached when we try to read codec register in suspend. Currently, codec driver will wait for initialization_complete then set cache only false and sync the regmap cache. But we didn't wait for codec attached on sdw bus driver. As a result, we will try to read the data before codec is attached.
Please see below log. I set all registers on rt1308 as volatile to test it.

log

When we do a sdw_read, we do a pm_runtime_get_sync() so we should wait for everything to resume nicely.
Something's not right in our resume process if we can't get this to work.

initialization_complete should also be set when the codec is actually present.

I can accept this doesn't work but this probably shows a bug somewhere else.

[bardliao]

initialization_complete

So we may want to

@plbossart This doesn't work with the existing sdw drivers. The point is that we resume sdw bus, but do not wait for codec attached when we try to read codec register in suspend. Currently, codec driver will wait for initialization_complete then set cache only false and sync the regmap cache. But we didn't wait for codec attached on sdw bus driver. As a result, we will try to read the data before codec is attached.
Please see below log. I set all registers on rt1308 as volatile to test it.
log

When we do a sdw_read, we do a pm_runtime_get_sync() so we should wait for everything to resume nicely.
Something's not right in our resume process if we can't get this to work.

initialization_complete should also be set when the codec is actually present.

Maybe enumeration_complete is a better choice in this case. We don't need to wait for codec initialization if we just want to read a register. @naveen-manohar Can you test #2526

I can accept this doesn't work but this probably shows a bug somewhere else.

I still have a concern on other devices with different IO interfaces. For example, I2C codecs may be powered off when it set cache only flag and I don't know if it will be woke up if we try to read its register when it is powered off.

[plbossart]

I still have a concern on other devices with different IO interfaces. For example, I2C codecs may be powered off when it set cache only flag and I don't know if it will be woke up if we try to read its register when it is powered off.

Agree, it's risky since it impacts everyone using regmap, but at the same time is returning - EBUSY the right thing to do if the codec is not powered? My solution may not be the right one, but the existing behavior doesn't seem right either.

[plbossart]

@bardliao I found a couple of bugs in our soundwire code, but there is also a conceptual deadlock with regmap.

When we use regmap_read, we take a lock

int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
{
	int ret;

	if (!IS_ALIGNED(reg, map->reg_stride))
		return -EINVAL;

	map->lock(map->lock_arg);

	ret = _regmap_read(map, reg, val);

	map->unlock(map->lock_arg);

	return ret;
}

then if we use my patch and do a sdw_regmap_read, we will resume the slave device, but in the resume routines, we call regcache_cache_only which takes the same lock -> deadlock

void regcache_cache_only(struct regmap *map, bool enable)
{
	map->lock(map->lock_arg);
	WARN_ON(map->cache_bypass && enable);
	map->cache_only = enable;
	trace_regmap_cache_only(map, enable);
	map->unlock(map->lock_arg);
}
EXPORT_SYMBOL_GPL(regcache_cache_only);

what we would need to do is change the ASoC core to call pm_runtime_get_sync() before calling regmap_read.

Or maybe we keep the existing solution and accessing a volatile register while in suspend returns -EBUSY.

[bardliao]

Agree, it's risky since it impacts everyone using regmap, but at the same time is returning - EBUSY the right thing to do if the codec is not powered? My solution may not be the right one, but the existing behavior doesn't seem right either.

@plbossart Yeah, returning -EBUSY doesn't seem be right. -EIO makes more sense to me.

what we would need to do is change the ASoC core to call pm_runtime_get_sync() before calling regmap_read.

IMHO, codec driver should avoid reading volatile registers in suspend. See #2532

[naveen-manohar]

Agree, it's risky since it impacts everyone using regmap, but at the same time is returning - EBUSY the right thing to do if the codec is not powered? My solution may not be the right one, but the existing behavior doesn't seem right either.

@plbossart Yeah, returning -EBUSY doesn't seem be right. -EIO makes more sense to me.

what we would need to do is change the ASoC core to call pm_runtime_get_sync() before calling regmap_read.

IMHO, codec driver should avoid reading volatile registers in suspend. See #2532

JFYI, with #2532 Verified Basic Maxim-Speaker Functionality(non-dsm) & also issue mentioned at #2510 isn't observed. Thanks

[plbossart]

closing, this PR exposed just too many issues with pm_runtime that need to be fixed first.