[PATCH v2 0/9] soundwire/regmap: use _no_pm routines
When a Slave device is resumed, it may resume the bus and restart the enumeration. And Slave drivers will wait for initialization_complete complete in their resume function, however initialization_complete will complete after sdw_update_slave_status function is finished and codec driver usually call some IO functions in the update_status callback function. It will become a deadlock if we use regular read/write routines during the resuming process.
This series touches both soundwire and regmap trees. commit fb5103f9d6ce ("regmap/SoundWire: sdw: add support for SoundWire 1.2 MBQ") is needed for soundwire tree to complie. On the other hands, commit 6e06a85556f9 ("soundwire: bus: add comments to explain interrupt loop filter") to commit 47b8520997a8 ("soundwire: bus: only clear valid DPN interrupts") are needed for regmap tree.
v2: - Separate commits according to maintainer's comments.
Bard Liao (4): soundwire: export sdw_write/read_no_pm functions regmap: sdw: use _no_pm functions in regmap_read/write regmap: sdw: use no_pm routines for SoundWire 1.2 MBQ regmap: sdw-mbq: use MODULE_LICENSE("GPL")
Pierre-Louis Bossart (5): soundwire: bus: use sdw_update_no_pm when initializing a device soundwire: bus: use sdw_write_no_pm when setting the bus scale registers soundwire: bus: use no_pm IO routines for all interrupt handling soundwire: bus: fix confusion on device used by pm_runtime soundwire: bus: clarify dev_err/dbg device references
drivers/base/regmap/regmap-sdw-mbq.c | 10 +- drivers/base/regmap/regmap-sdw.c | 4 +- drivers/soundwire/bus.c | 136 +++++++++++++++------------ include/linux/soundwire/sdw.h | 2 + 4 files changed, 85 insertions(+), 67 deletions(-)
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(-)
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);
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.
This patch fixes the same problem as the previous one, but is split to make the life of linux-stable maintainers less painful.
Fixes: 29d158f90690 ('soundwire: bus: initialize bus clock base and scale registers') 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 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 60c42508c6c6..b1830032b052 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -1222,7 +1222,7 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave) } scale_index++;
- ret = sdw_write(slave, SDW_SCP_BUS_CLOCK_BASE, base); + ret = sdw_write_no_pm(slave, SDW_SCP_BUS_CLOCK_BASE, base); if (ret < 0) { dev_err(&slave->dev, "SDW_SCP_BUS_CLOCK_BASE write failed:%d\n", ret); @@ -1230,13 +1230,13 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave) }
/* initialize scale for both banks */ - ret = sdw_write(slave, SDW_SCP_BUSCLOCK_SCALE_B0, scale_index); + ret = sdw_write_no_pm(slave, SDW_SCP_BUSCLOCK_SCALE_B0, scale_index); if (ret < 0) { dev_err(&slave->dev, "SDW_SCP_BUSCLOCK_SCALE_B0 write failed:%d\n", ret); return ret; } - ret = sdw_write(slave, SDW_SCP_BUSCLOCK_SCALE_B1, scale_index); + ret = sdw_write_no_pm(slave, SDW_SCP_BUSCLOCK_SCALE_B1, scale_index); if (ret < 0) dev_err(&slave->dev, "SDW_SCP_BUSCLOCK_SCALE_B1 write failed:%d\n", ret);
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
There is no need to play with pm_runtime reference counts, if needed the codec drivers are already explicitly resumed.
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 | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index b1830032b052..86c339d77a39 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -1295,7 +1295,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) u8 clear, impl_int_mask; int status, status2, ret, count = 0;
- status = sdw_read(slave, SDW_DP0_INT); + status = sdw_read_no_pm(slave, SDW_DP0_INT); if (status < 0) { dev_err(slave->bus->dev, "SDW_DP0_INT read failed:%d\n", status); @@ -1334,7 +1334,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) }
/* clear the interrupts but don't touch reserved and SDCA_CASCADE fields */ - ret = sdw_write(slave, SDW_DP0_INT, clear); + ret = sdw_write_no_pm(slave, SDW_DP0_INT, clear); if (ret < 0) { dev_err(slave->bus->dev, "SDW_DP0_INT write failed:%d\n", ret); @@ -1342,7 +1342,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) }
/* Read DP0 interrupt again */ - status2 = sdw_read(slave, SDW_DP0_INT); + status2 = sdw_read_no_pm(slave, SDW_DP0_INT); if (status2 < 0) { dev_err(slave->bus->dev, "SDW_DP0_INT read failed:%d\n", status2); @@ -1373,7 +1373,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, return sdw_handle_dp0_interrupt(slave, slave_status);
addr = SDW_DPN_INT(port); - status = sdw_read(slave, addr); + status = sdw_read_no_pm(slave, addr); if (status < 0) { dev_err(slave->bus->dev, "SDW_DPN_INT read failed:%d\n", status); @@ -1407,7 +1407,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, }
/* clear the interrupt but don't touch reserved fields */ - ret = sdw_write(slave, addr, clear); + ret = sdw_write_no_pm(slave, addr, clear); if (ret < 0) { dev_err(slave->bus->dev, "SDW_DPN_INT write failed:%d\n", ret); @@ -1415,7 +1415,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, }
/* Read DPN interrupt again */ - status2 = sdw_read(slave, addr); + status2 = sdw_read_no_pm(slave, addr); if (status2 < 0) { dev_err(slave->bus->dev, "SDW_DPN_INT read failed:%d\n", status2); @@ -1457,7 +1457,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) }
/* Read Intstat 1, Intstat 2 and Intstat 3 registers */ - ret = sdw_read(slave, SDW_SCP_INT1); + ret = sdw_read_no_pm(slave, SDW_SCP_INT1); if (ret < 0) { dev_err(slave->bus->dev, "SDW_SCP_INT1 read failed:%d\n", ret); @@ -1465,7 +1465,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) } buf = ret;
- ret = sdw_nread(slave, SDW_SCP_INTSTAT2, 2, buf2); + ret = sdw_nread_no_pm(slave, SDW_SCP_INTSTAT2, 2, buf2); if (ret < 0) { dev_err(slave->bus->dev, "SDW_SCP_INT2/3 read failed:%d\n", ret); @@ -1473,7 +1473,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) }
if (slave->prop.is_sdca) { - ret = sdw_read(slave, SDW_DP0_INT); + ret = sdw_read_no_pm(slave, SDW_DP0_INT); if (ret < 0) { dev_err(slave->bus->dev, "SDW_DP0_INT read failed:%d\n", ret); @@ -1570,7 +1570,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) }
/* Ack interrupt */ - ret = sdw_write(slave, SDW_SCP_INT1, clear); + ret = sdw_write_no_pm(slave, SDW_SCP_INT1, clear); if (ret < 0) { dev_err(slave->bus->dev, "SDW_SCP_INT1 write failed:%d\n", ret); @@ -1584,7 +1584,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) * Read status again to ensure no new interrupts arrived * while servicing interrupts. */ - ret = sdw_read(slave, SDW_SCP_INT1); + ret = sdw_read_no_pm(slave, SDW_SCP_INT1); if (ret < 0) { dev_err(slave->bus->dev, "SDW_SCP_INT1 read failed:%d\n", ret); @@ -1592,7 +1592,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) } _buf = ret;
- ret = sdw_nread(slave, SDW_SCP_INTSTAT2, 2, _buf2); + ret = sdw_nread_no_pm(slave, SDW_SCP_INTSTAT2, 2, _buf2); if (ret < 0) { dev_err(slave->bus->dev, "SDW_SCP_INT2/3 read failed:%d\n", ret); @@ -1600,7 +1600,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) }
if (slave->prop.is_sdca) { - ret = sdw_read(slave, SDW_DP0_INT); + ret = sdw_read_no_pm(slave, SDW_DP0_INT); if (ret < 0) { dev_err(slave->bus->dev, "SDW_DP0_INT read failed:%d\n", ret);
sdw_write_no_pm and sdw_read_no_pm are useful when we want to do IO without touching PM.
Fixes: 0231453bc08f ('soundwire: bus: add clock stop helpers') Fixes: 60ee9be25571 ('soundwire: bus: add PM/no-PM versions of read/write functions') Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/soundwire/bus.c | 7 ++++--- include/linux/soundwire/sdw.h | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 86c339d77a39..4eaeeb4090f0 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -405,10 +405,11 @@ sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) return sdw_transfer(slave->bus, &msg); }
-static int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value) +int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value) { return sdw_nwrite_no_pm(slave, addr, 1, &value); } +EXPORT_SYMBOL(sdw_write_no_pm);
static int sdw_bread_no_pm(struct sdw_bus *bus, u16 dev_num, u32 addr) @@ -476,8 +477,7 @@ int sdw_bwrite_no_pm_unlocked(struct sdw_bus *bus, u16 dev_num, u32 addr, u8 val } EXPORT_SYMBOL(sdw_bwrite_no_pm_unlocked);
-static int -sdw_read_no_pm(struct sdw_slave *slave, u32 addr) +int sdw_read_no_pm(struct sdw_slave *slave, u32 addr) { u8 buf; int ret; @@ -488,6 +488,7 @@ sdw_read_no_pm(struct sdw_slave *slave, u32 addr) else return buf; } +EXPORT_SYMBOL(sdw_read_no_pm);
static int sdw_update_no_pm(struct sdw_slave *slave, u32 addr, u8 mask, u8 val) { diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index f0b01b728640..d08039d65825 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -1005,6 +1005,8 @@ int sdw_bus_exit_clk_stop(struct sdw_bus *bus);
int sdw_read(struct sdw_slave *slave, u32 addr); int sdw_write(struct sdw_slave *slave, u32 addr, u8 value); +int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value); +int sdw_read_no_pm(struct sdw_slave *slave, u32 addr); int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val); int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val);
sdw_update_slave_status will be invoked when a codec is attached, and the codec driver will initialize the codec with regmap functions while the codec device is pm_runtime suspended.
regmap routines currently rely on regular SoundWire IO functions, which will call pm_runtime_get_sync()/put_autosuspend.
This causes a deadlock where the resume routine waits for an initialization complete signal that while the initialization complete can only be reached when the resume completes.
The only solution if we allow regmap functions to be used in resume operations as well as during codec initialization is to use _no_pm routines. The duty of making sure the bus is operational needs to be handled above the regmap level.
Fixes: 7c22ce6e21840 ('regmap: Add SoundWire bus support') Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/base/regmap/regmap-sdw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index c92d614b4943..4b8d2d010cab 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -11,7 +11,7 @@ static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val) struct device *dev = context; struct sdw_slave *slave = dev_to_sdw_dev(dev);
- return sdw_write(slave, reg, val); + return sdw_write_no_pm(slave, reg, val); }
static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val) @@ -20,7 +20,7 @@ static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val) struct sdw_slave *slave = dev_to_sdw_dev(dev); int read;
- read = sdw_read(slave, reg); + read = sdw_read_no_pm(slave, reg); if (read < 0) return read;
HeY Mark,
On 09-12-20, 13:34, Bard Liao wrote:
sdw_update_slave_status will be invoked when a codec is attached, and the codec driver will initialize the codec with regmap functions while the codec device is pm_runtime suspended.
regmap routines currently rely on regular SoundWire IO functions, which will call pm_runtime_get_sync()/put_autosuspend.
This causes a deadlock where the resume routine waits for an initialization complete signal that while the initialization complete can only be reached when the resume completes.
The only solution if we allow regmap functions to be used in resume operations as well as during codec initialization is to use _no_pm routines. The duty of making sure the bus is operational needs to be handled above the regmap level.
You okay these going thru sdw tree..?
Fixes: 7c22ce6e21840 ('regmap: Add SoundWire bus support') Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com
drivers/base/regmap/regmap-sdw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index c92d614b4943..4b8d2d010cab 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -11,7 +11,7 @@ static int regmap_sdw_write(void *context, unsigned int reg, unsigned int val) struct device *dev = context; struct sdw_slave *slave = dev_to_sdw_dev(dev);
- return sdw_write(slave, reg, val);
- return sdw_write_no_pm(slave, reg, val);
}
static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val) @@ -20,7 +20,7 @@ static int regmap_sdw_read(void *context, unsigned int reg, unsigned int *val) struct sdw_slave *slave = dev_to_sdw_dev(dev); int read;
- read = sdw_read(slave, reg);
- read = sdw_read_no_pm(slave, reg); if (read < 0) return read;
-- 2.17.1
Use no_pm versions for write and read.
Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Rander Wang rander.wang@linux.intel.com --- drivers/base/regmap/regmap-sdw-mbq.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/base/regmap/regmap-sdw-mbq.c b/drivers/base/regmap/regmap-sdw-mbq.c index 8ce30650b97c..6675c3a4b829 100644 --- a/drivers/base/regmap/regmap-sdw-mbq.c +++ b/drivers/base/regmap/regmap-sdw-mbq.c @@ -15,11 +15,11 @@ static int regmap_sdw_mbq_write(void *context, unsigned int reg, unsigned int va struct sdw_slave *slave = dev_to_sdw_dev(dev); int ret;
- ret = sdw_write(slave, SDW_SDCA_MBQ_CTL(reg), (val >> 8) & 0xff); + ret = sdw_write_no_pm(slave, SDW_SDCA_MBQ_CTL(reg), (val >> 8) & 0xff); if (ret < 0) return ret;
- return sdw_write(slave, reg, val & 0xff); + return sdw_write_no_pm(slave, reg, val & 0xff); }
static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int *val) @@ -29,11 +29,11 @@ static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int *va int read0; int read1;
- read0 = sdw_read(slave, reg); + read0 = sdw_read_no_pm(slave, reg); if (read0 < 0) return read0;
- read1 = sdw_read(slave, SDW_SDCA_MBQ_CTL(reg)); + read1 = sdw_read_no_pm(slave, SDW_SDCA_MBQ_CTL(reg)); if (read1 < 0) return read1;
"GPL v2" is the same as "GPL". It exists for historic reasons. See Documentation/process/license-rules.rst
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Signed-off-by: Bard Liao yung-chuan.liao@linux.intel.com --- drivers/base/regmap/regmap-sdw-mbq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/base/regmap/regmap-sdw-mbq.c b/drivers/base/regmap/regmap-sdw-mbq.c index 6675c3a4b829..fe3ac26b66ad 100644 --- a/drivers/base/regmap/regmap-sdw-mbq.c +++ b/drivers/base/regmap/regmap-sdw-mbq.c @@ -98,4 +98,4 @@ struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw, EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq);
MODULE_DESCRIPTION("Regmap SoundWire MBQ Module"); -MODULE_LICENSE("GPL v2"); +MODULE_LICENSE("GPL");
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Intel stress-tests routinely report IO timeouts and invalid power management transitions. Upon further analysis, we seem to be using the wrong devices in pm_runtime calls.
Before reading and writing registers, we first need to make sure the Slave is fully resumed. The existing code attempts to do such that, however because of a confusion dating from 2017 and copy/paste, we end-up resuming the parent only instead of resuming the codec device.
This can lead to accesses to the Slave registers while the bus is still being configured and the Slave not enumerated, and as a result IO errors occur.
This is a classic problem, similar confusions happened for HDaudio between bus and codec device, leading to power management issues.
Fix by using the relevant device for all uses of pm_runtime functions.
Fixes: 60ee9be255712 ('soundwire: bus: add PM/no-PM versions of read/write functions') Fixes: aa79293517b39 ('soundwire: bus: fix io error when processing alert event') Fixes: 9d715fa005ebc ('soundwire: Add IO transfer') Reported-by: Bard Liao yung-chuan.liao@linux.intel.com 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 | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 4eaeeb4090f0..7c4717dd9a34 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -513,16 +513,16 @@ int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) { int ret;
- ret = pm_runtime_get_sync(slave->bus->dev); + ret = pm_runtime_get_sync(&slave->dev); if (ret < 0 && ret != -EACCES) { - pm_runtime_put_noidle(slave->bus->dev); + pm_runtime_put_noidle(&slave->dev); return ret; }
ret = sdw_nread_no_pm(slave, addr, count, val);
- pm_runtime_mark_last_busy(slave->bus->dev); - pm_runtime_put(slave->bus->dev); + pm_runtime_mark_last_busy(&slave->dev); + pm_runtime_put(&slave->dev);
return ret; } @@ -539,16 +539,16 @@ int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val) { int ret;
- ret = pm_runtime_get_sync(slave->bus->dev); + ret = pm_runtime_get_sync(&slave->dev); if (ret < 0 && ret != -EACCES) { - pm_runtime_put_noidle(slave->bus->dev); + pm_runtime_put_noidle(&slave->dev); return ret; }
ret = sdw_nwrite_no_pm(slave, addr, count, val);
- pm_runtime_mark_last_busy(slave->bus->dev); - pm_runtime_put(slave->bus->dev); + pm_runtime_mark_last_busy(&slave->dev); + pm_runtime_put(&slave->dev);
return ret; } @@ -1453,7 +1453,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) ret = pm_runtime_get_sync(&slave->dev); if (ret < 0 && ret != -EACCES) { dev_err(&slave->dev, "Failed to resume device: %d\n", ret); - pm_runtime_put_noidle(slave->bus->dev); + pm_runtime_put_noidle(&slave->dev); return ret; }
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
The SoundWire bus code confuses bus and Slave device levels for dev_err/dbg logs. That's not impacting functionality but the accuracy of kernel logs.
We should only use bus->dev for bus-level operations and handling of Device0. For all other logs where the device number is not zero, we should use &slave->dev to provide more precisions to the user/integrator.
Reported-by: Rander Wang rander.wang@linux.intel.com 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 | 63 +++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 30 deletions(-)
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index 7c4717dd9a34..39edf87cf832 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -636,6 +636,7 @@ static int sdw_get_device_num(struct sdw_slave *slave)
static int sdw_assign_device_num(struct sdw_slave *slave) { + struct sdw_bus *bus = slave->bus; int ret, dev_num; bool new_device = false;
@@ -646,7 +647,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave) dev_num = sdw_get_device_num(slave); mutex_unlock(&slave->bus->bus_lock); if (dev_num < 0) { - dev_err(slave->bus->dev, "Get dev_num failed: %d\n", + dev_err(bus->dev, "Get dev_num failed: %d\n", dev_num); return dev_num; } @@ -659,7 +660,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave) }
if (!new_device) - dev_dbg(slave->bus->dev, + dev_dbg(bus->dev, "Slave already registered, reusing dev_num:%d\n", slave->dev_num);
@@ -669,7 +670,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
ret = sdw_write_no_pm(slave, SDW_SCP_DEVNUMBER, dev_num); if (ret < 0) { - dev_err(&slave->dev, "Program device_num %d failed: %d\n", + dev_err(bus->dev, "Program device_num %d failed: %d\n", dev_num, ret); return ret; } @@ -748,7 +749,7 @@ static int sdw_program_device_num(struct sdw_bus *bus) */ ret = sdw_assign_device_num(slave); if (ret) { - dev_err(slave->bus->dev, + dev_err(bus->dev, "Assign dev_num failed:%d\n", ret); return ret; @@ -788,9 +789,11 @@ static int sdw_program_device_num(struct sdw_bus *bus) static void sdw_modify_slave_status(struct sdw_slave *slave, enum sdw_slave_status status) { - mutex_lock(&slave->bus->bus_lock); + struct sdw_bus *bus = slave->bus;
- dev_vdbg(&slave->dev, + mutex_lock(&bus->bus_lock); + + dev_vdbg(bus->dev, "%s: changing status slave %d status %d new status %d\n", __func__, slave->dev_num, slave->status, status);
@@ -811,7 +814,7 @@ static void sdw_modify_slave_status(struct sdw_slave *slave, complete(&slave->enumeration_complete); } slave->status = status; - mutex_unlock(&slave->bus->bus_lock); + mutex_unlock(&bus->bus_lock); }
static enum sdw_clk_stop_mode sdw_get_clk_stop_mode(struct sdw_slave *slave) @@ -1140,7 +1143,7 @@ int sdw_configure_dpn_intr(struct sdw_slave *slave,
ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val); if (ret < 0) - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DPN_INTMASK write failed:%d\n", val);
return ret; @@ -1271,7 +1274,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave) /* Enable SCP interrupts */ ret = sdw_update_no_pm(slave, SDW_SCP_INTMASK1, val, val); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_SCP_INTMASK1 write failed:%d\n", ret); return ret; } @@ -1286,7 +1289,7 @@ static int sdw_initialize_slave(struct sdw_slave *slave)
ret = sdw_update_no_pm(slave, SDW_DP0_INTMASK, val, val); if (ret < 0) - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DP0_INTMASK read failed:%d\n", ret); return ret; } @@ -1298,7 +1301,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status)
status = sdw_read_no_pm(slave, SDW_DP0_INT); if (status < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DP0_INT read failed:%d\n", status); return status; } @@ -1337,7 +1340,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) /* clear the interrupts but don't touch reserved and SDCA_CASCADE fields */ ret = sdw_write_no_pm(slave, SDW_DP0_INT, clear); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DP0_INT write failed:%d\n", ret); return ret; } @@ -1345,7 +1348,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) /* Read DP0 interrupt again */ status2 = sdw_read_no_pm(slave, SDW_DP0_INT); if (status2 < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DP0_INT read failed:%d\n", status2); return status2; } @@ -1358,7 +1361,7 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) } while ((status & SDW_DP0_INTERRUPTS) && (count < SDW_READ_INTR_CLEAR_RETRY));
if (count == SDW_READ_INTR_CLEAR_RETRY) - dev_warn(slave->bus->dev, "Reached MAX_RETRY on DP0 read\n"); + dev_warn(&slave->dev, "Reached MAX_RETRY on DP0 read\n");
return ret; } @@ -1376,7 +1379,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, addr = SDW_DPN_INT(port); status = sdw_read_no_pm(slave, addr); if (status < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DPN_INT read failed:%d\n", status);
return status; @@ -1410,7 +1413,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, /* clear the interrupt but don't touch reserved fields */ ret = sdw_write_no_pm(slave, addr, clear); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DPN_INT write failed:%d\n", ret); return ret; } @@ -1418,7 +1421,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, /* Read DPN interrupt again */ status2 = sdw_read_no_pm(slave, addr); if (status2 < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DPN_INT read failed:%d\n", status2); return status2; } @@ -1431,7 +1434,7 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, } while ((status & SDW_DPN_INTERRUPTS) && (count < SDW_READ_INTR_CLEAR_RETRY));
if (count == SDW_READ_INTR_CLEAR_RETRY) - dev_warn(slave->bus->dev, "Reached MAX_RETRY on port read"); + dev_warn(&slave->dev, "Reached MAX_RETRY on port read");
return ret; } @@ -1460,7 +1463,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) /* Read Intstat 1, Intstat 2 and Intstat 3 registers */ ret = sdw_read_no_pm(slave, SDW_SCP_INT1); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_SCP_INT1 read failed:%d\n", ret); goto io_err; } @@ -1468,7 +1471,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
ret = sdw_nread_no_pm(slave, SDW_SCP_INTSTAT2, 2, buf2); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_SCP_INT2/3 read failed:%d\n", ret); goto io_err; } @@ -1476,7 +1479,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) if (slave->prop.is_sdca) { ret = sdw_read_no_pm(slave, SDW_DP0_INT); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DP0_INT read failed:%d\n", ret); goto io_err; } @@ -1573,7 +1576,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) /* Ack interrupt */ ret = sdw_write_no_pm(slave, SDW_SCP_INT1, clear); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_SCP_INT1 write failed:%d\n", ret); goto io_err; } @@ -1587,7 +1590,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) */ ret = sdw_read_no_pm(slave, SDW_SCP_INT1); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_SCP_INT1 read failed:%d\n", ret); goto io_err; } @@ -1595,7 +1598,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
ret = sdw_nread_no_pm(slave, SDW_SCP_INTSTAT2, 2, _buf2); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_SCP_INT2/3 read failed:%d\n", ret); goto io_err; } @@ -1603,7 +1606,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) if (slave->prop.is_sdca) { ret = sdw_read_no_pm(slave, SDW_DP0_INT); if (ret < 0) { - dev_err(slave->bus->dev, + dev_err(&slave->dev, "SDW_DP0_INT read failed:%d\n", ret); goto io_err; } @@ -1629,7 +1632,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave) } while (stat != 0 && count < SDW_READ_INTR_CLEAR_RETRY);
if (count == SDW_READ_INTR_CLEAR_RETRY) - dev_warn(slave->bus->dev, "Reached MAX_RETRY on alert read\n"); + dev_warn(&slave->dev, "Reached MAX_RETRY on alert read\n");
io_err: pm_runtime_mark_last_busy(&slave->dev); @@ -1735,7 +1738,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus, case SDW_SLAVE_ALERT: ret = sdw_handle_slave_alerts(slave); if (ret) - dev_err(bus->dev, + dev_err(&slave->dev, "Slave %d alert handling failed: %d\n", i, ret); break; @@ -1754,21 +1757,21 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
ret = sdw_initialize_slave(slave); if (ret) - dev_err(bus->dev, + dev_err(&slave->dev, "Slave %d initialization failed: %d\n", i, ret);
break;
default: - dev_err(bus->dev, "Invalid slave %d status:%d\n", + dev_err(&slave->dev, "Invalid slave %d status:%d\n", i, status[i]); break; }
ret = sdw_update_slave_status(slave, status[i]); if (ret) - dev_err(slave->bus->dev, + dev_err(&slave->dev, "Update Slave status failed:%d\n", ret); if (attached_initializing) complete(&slave->initialization_complete);
participants (2)
-
Bard Liao -
Vinod Koul