| Message ID | 20210617153410.27922-1-rf@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: stream: Use polling for DP Prepare completion | expand |
Thanks Richard for the patch, very interesting. > sdw_prep_deprep_slave_ports() cannot use the port interrupt to signal > CP_SM completion because it is called while holding the bus lock, which > blocks the alert handler from running. > > Change to polling the PREPARESTATUS register and remove the > infrastructure that was implemented for the previous interrupt waiting. I am afraid the entire PORT_READY support is completely untested at the moment: all the existing codecs use the simpler state machine, e.g. dpn[i].simple_ch_prep_sm = true; So my main question is: how did you test this change? Is this on a platform already supported upstream? yes/no answer is fine, no details necessary. I am also not fully clear on the locking issue. Is the problem that sdw_handle_slave_status() uses a mutex_lock(&bus->bus_lock), which is already held in sdw_prepare_stream int sdw_prepare_stream(struct sdw_stream_runtime *stream) { [...] sdw_acquire_bus_lock(stream); [...] ret = _sdw_prepare_stream(stream, update_params); sdw_release_bus_lock(stream); return ret; } so dealing with an ALERT status while doing pre-deprep would cause a self-inflicted deadlock? If yes, that's a more general problem that would need to be fixed. this lock is taken in ALL stream operations, not just prepare/deprepare. If e.g. a jack detection was signaled during any stream reconfiguration, we would also not be able to deal with the information, would we? the purpose of the lock in sdw_handle_slave_status() was to check if devices attach or fall-off the bus before handling their status. The command/control protocol is always operational so nothing prevents the interrupt from being handled. There's also something odd about directly polling on the status bits. The status bits will be used to signal the ALERT condition which is transmitted to the host during PING frames. This solution would result in the host noticing an interrupt: host controllers typically have a sticky status where all changes in PING frames are detected and used as a trigger for interrupt processing. In this case the interrupt would still be handled, but the sdw_handle_slave_status() would be deferred until the stream prepare is complete, and at that point the interrupt processing would not see any sources. It's not illegal but it's a bit odd to cause empty interrupts to be handled. It might be a better solution to fix conflicts between stream reconfiguration and interrupts. I don't have a turn-key proposal but the suggested patch feels like a work-around. maybe using mutex_is_locked()? If we can't figure something out, then at least the PORT_READY mask should not be set, i.e. this code would need to be removed: int sdw_configure_dpn_intr(struct sdw_slave *slave, int port, bool enable, int mask) { ... /* Set/Clear port ready interrupt mask */ if (enable) { val |= mask; val |= SDW_DPN_INT_PORT_READY; } else { val &= ~(mask); val &= ~SDW_DPN_INT_PORT_READY; } ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val); } > A new configuration field 'ch_prep_poll_us' is added to struct > sdw_dpn_prop so that the peripheral driver may select a polling interval. > If this is left at zero a default interval of 500 usec is used. we already have a 'ch_prep_timeout' that's defined, do you need to redefine a new field? why not just read once at the end of that timeout? It's a prepare operation, there's no requirement to be fast, is there? Those properties were supposed to be populated in platform firmware btw, we should try and limit what properties need to be set in a codec driver. > + prep_timeout_us = dpn_prop->ch_prep_timeout * USEC_PER_MSEC; I also just noticed that we don't have a default for ch_prep_timeout, that's a bug. > + if (dpn_prop->ch_prep_poll_us) > + prep_poll_us = dpn_prop->ch_prep_poll_us; > + else > + prep_poll_us = SDW_DEFAULT_DP_PREP_POLL_US; Thanks -Pierre
On 17/06/2021 17:53, Pierre-Louis Bossart wrote: > Thanks Richard for the patch, very interesting. > >> sdw_prep_deprep_slave_ports() cannot use the port interrupt to signal >> CP_SM completion because it is called while holding the bus lock, which >> blocks the alert handler from running. >> >> Change to polling the PREPARESTATUS register and remove the >> infrastructure that was implemented for the previous interrupt waiting. > > I am afraid the entire PORT_READY support is completely untested at the moment: all the existing codecs use the simpler state machine, e.g. > > dpn[i].simple_ch_prep_sm = true; > > So my main question is: how did you test this change? Is this on a platform already supported upstream? yes/no answer is fine, no details necessary. > No, it isn't upstream yet, but it doesn't support Simplified_CP_SM. > I am also not fully clear on the locking issue. > > Is the problem that sdw_handle_slave_status() uses a mutex_lock(&bus->bus_lock), which is already held in sdw_prepare_stream > Yes > so dealing with an ALERT status while doing pre-deprep would cause a self-inflicted deadlock? > Not strictly a deadlock, but the ALERT handling will be delayed until the sdw_prepare_stream() has released the bus lock. Of course, by then the wait_for_completion() has timed out. There is another bug in the original code. After wait_for_completion() times out, there is a read of the PREPARESTATUS register. But the test if (!time_left || val) will always treat a timeout as a failure even if the port is now reporting successful prepare. I can do a fix for that bug so that full CP_SM devices will prepare successfully after the wait_for_completion() times out. > If yes, that's a more general problem that would need to be fixed. this lock is taken in ALL stream operations, not just prepare/deprepare. > > If e.g. a jack detection was signaled during any stream reconfiguration, we would also not be able to deal with the information, would we? > The ALERT would be delayed until after the stream reconfig has ended. > the purpose of the lock in sdw_handle_slave_status() was to check if devices attach or fall-off the bus before handling their status. The command/control protocol is always operational so nothing prevents the interrupt from being handled. > > There's also something odd about directly polling on the status bits. The status bits will be used to signal the ALERT condition which is transmitted to the host during PING frames. This solution would result in the host noticing an interrupt: host controllers typically have a sticky status where all changes in PING frames are detected and used as a trigger for interrupt processing. In this case the interrupt would still be handled, but the sdw_handle_slave_status() would be deferred until the stream prepare is complete, and at that point the interrupt processing would not see any sources. It's not illegal but it's a bit odd to cause empty interrupts to be handled. > > It might be a better solution to fix conflicts between stream reconfiguration and interrupts. I don't have a turn-key proposal but the suggested patch feels like a work-around. > maybe using mutex_is_locked()? > Possibly but I am very reluctant to rewrite bus locking, as I don't have the ability to test a wide range of hardware configurations. > If we can't figure something out, then at least the PORT_READY mask should not be set, i.e. this code would need to be removed: > > ... >> A new configuration field 'ch_prep_poll_us' is added to struct >> sdw_dpn_prop so that the peripheral driver may select a polling interval. >> If this is left at zero a default interval of 500 usec is used. > > we already have a 'ch_prep_timeout' that's defined, do you need to redefine a new field? The new field is how often to poll, so the driver can select a slower poll rate if it knows its CP_SM takes longer. > why not just read once at the end of that timeout? It's a prepare operation, there's no requirement to be fast, is there? That is broken in the current code, as noted above. But I could make a patch only to fix that bug.
>>> sdw_prep_deprep_slave_ports() cannot use the port interrupt to signal >>> CP_SM completion because it is called while holding the bus lock, which >>> blocks the alert handler from running. >>> >>> Change to polling the PREPARESTATUS register and remove the >>> infrastructure that was implemented for the previous interrupt waiting. >> >> I am afraid the entire PORT_READY support is completely untested at the moment: all the existing codecs use the simpler state machine, e.g. >> >> dpn[i].simple_ch_prep_sm = true; >> >> So my main question is: how did you test this change? Is this on a platform already supported upstream? yes/no answer is fine, no details necessary. >> > > No, it isn't upstream yet, but it doesn't support Simplified_CP_SM. ok, good to know. >> I am also not fully clear on the locking issue. >> >> Is the problem that sdw_handle_slave_status() uses a mutex_lock(&bus->bus_lock), which is already held in sdw_prepare_stream >> > > Yes > >> so dealing with an ALERT status while doing pre-deprep would cause a self-inflicted deadlock? >> > > Not strictly a deadlock, but the ALERT handling will be delayed until > the sdw_prepare_stream() has released the bus lock. Of course, by then > the wait_for_completion() has timed out. ok. > There is another bug in the original code. After wait_for_completion() > times out, there is a read of the PREPARESTATUS register. But the test > > if (!time_left || val) > > will always treat a timeout as a failure even if the port is now > reporting successful prepare. > > I can do a fix for that bug so that full CP_SM devices will prepare > successfully after the wait_for_completion() times out. What you are describing seems to be a case of the device completing the preparation just after the timeout expires but just before double-checking the register value? Doesn't this indicate that the timeout value is just wrong? Or did you mean that a possible work-around would be to essentially ignore the timeout due to the locking? That might help you make progress and in a second step we could attempt to remove the locking issue. >> If yes, that's a more general problem that would need to be fixed. this lock is taken in ALL stream operations, not just prepare/deprepare. >> >> If e.g. a jack detection was signaled during any stream reconfiguration, we would also not be able to deal with the information, would we? >> > > The ALERT would be delayed until after the stream reconfig has ended. > >> the purpose of the lock in sdw_handle_slave_status() was to check if devices attach or fall-off the bus before handling their status. The command/control protocol is always operational so nothing prevents the interrupt from being handled. >> >> There's also something odd about directly polling on the status bits. The status bits will be used to signal the ALERT condition which is transmitted to the host during PING frames. This solution would result in the host noticing an interrupt: host controllers typically have a sticky status where all changes in PING frames are detected and used as a trigger for interrupt processing. In this case the interrupt would still be handled, but the sdw_handle_slave_status() would be deferred until the stream prepare is complete, and at that point the interrupt processing would not see any sources. It's not illegal but it's a bit odd to cause empty interrupts to be handled. >> >> It might be a better solution to fix conflicts between stream reconfiguration and interrupts. I don't have a turn-key proposal but the suggested patch feels like a work-around. >> maybe using mutex_is_locked()? >> > > Possibly but I am very reluctant to rewrite bus locking, as I don't have > the ability to test a wide range of hardware configurations. I wasn't referring to a complete rewrite of the bus locking, only for a check of the interaction between alerts and stream handling. >> If we can't figure something out, then at least the PORT_READY mask should not be set, i.e. this code would need to be removed: >> >> ... >>> A new configuration field 'ch_prep_poll_us' is added to struct >>> sdw_dpn_prop so that the peripheral driver may select a polling interval. >>> If this is left at zero a default interval of 500 usec is used. >> >> we already have a 'ch_prep_timeout' that's defined, do you need to redefine a new field? > > The new field is how often to poll, so the driver can select a slower > poll rate if it knows its CP_SM takes longer. You missed my point: if you think this property is needed, we'd want to add it to the MIPI DisCo list of properties so that it can be optionally provided by platform firmware. >> why not just read once at the end of that timeout? It's a prepare operation, there's no requirement to be fast, is there? > > That is broken in the current code, as noted above. But I could make a > patch only to fix that bug. that might indeed be less invasive for an initial fix, and we can deal with the locking problem later.
diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c index adcbf3969110..606fc26d407f 100644 --- a/drivers/soundwire/bus.c +++ b/drivers/soundwire/bus.c @@ -1351,7 +1351,6 @@ static int sdw_handle_dp0_interrupt(struct sdw_slave *slave, u8 *slave_status) */ if (status & SDW_DP0_INT_PORT_READY) { - complete(&slave->port_ready[0]); clear |= SDW_DP0_INT_PORT_READY; } @@ -1429,7 +1428,6 @@ static int sdw_handle_port_interrupt(struct sdw_slave *slave, * for ports implementing CP_SM. */ if (status & SDW_DPN_INT_PORT_READY) { - complete(&slave->port_ready[port]); clear |= SDW_DPN_INT_PORT_READY; } diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c index 669d7573320b..55ca884ea951 100644 --- a/drivers/soundwire/slave.c +++ b/drivers/soundwire/slave.c @@ -26,7 +26,6 @@ int sdw_slave_add(struct sdw_bus *bus, { struct sdw_slave *slave; int ret; - int i; slave = kzalloc(sizeof(*slave), GFP_KERNEL); if (!slave) @@ -62,9 +61,6 @@ int sdw_slave_add(struct sdw_bus *bus, slave->probed = false; slave->first_interrupt_done = false; - for (i = 0; i < SDW_MAX_PORTS; i++) - init_completion(&slave->port_ready[i]); - mutex_lock(&bus->bus_lock); list_add_tail(&slave->node, &bus->slaves); mutex_unlock(&bus->bus_lock); diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 1eaedaaba094..bd6b3b64de90 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -8,11 +8,13 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/init.h> +#include <linux/iopoll.h> #include <linux/module.h> #include <linux/mod_devicetable.h> #include <linux/slab.h> #include <linux/soundwire/sdw_registers.h> #include <linux/soundwire/sdw.h> +#include <linux/time.h> #include <sound/soc.h> #include "bus.h" @@ -419,11 +421,10 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, struct sdw_port_runtime *p_rt, bool prep) { - struct completion *port_ready; struct sdw_dpn_prop *dpn_prop; struct sdw_prepare_ch prep_ch; - unsigned int time_left; bool intr = false; + unsigned long prep_poll_us, prep_timeout_us; int ret = 0, val; u32 addr; @@ -478,16 +479,19 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus, } /* Wait for completion on port ready */ - port_ready = &s_rt->slave->port_ready[prep_ch.num]; - time_left = wait_for_completion_timeout(port_ready, - msecs_to_jiffies(dpn_prop->ch_prep_timeout)); + prep_timeout_us = dpn_prop->ch_prep_timeout * USEC_PER_MSEC; + if (dpn_prop->ch_prep_poll_us) + prep_poll_us = dpn_prop->ch_prep_poll_us; + else + prep_poll_us = SDW_DEFAULT_DP_PREP_POLL_US; - val = sdw_read(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num)); - val &= p_rt->ch_mask; - if (!time_left || val) { + ret = read_poll_timeout(sdw_read, val, ((val & p_rt->ch_mask) == 0), + prep_poll_us, prep_timeout_us, false, + s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num)); + if (ret < 0) { dev_err(&s_rt->slave->dev, - "Chn prep failed for port:%d\n", prep_ch.num); - return -ETIMEDOUT; + "Chn prep failed for port %d: %d\n", prep_ch.num, ret); + return ret; } } diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h index ddbeb00799e4..4e5290b083bf 100644 --- a/include/linux/soundwire/sdw.h +++ b/include/linux/soundwire/sdw.h @@ -67,6 +67,9 @@ enum { #define SDW_BLOCK_PACKG_PER_PORT BIT(0) #define SDW_BLOCK_PACKG_PER_CH BIT(1) +/* Default interval to poll for DP prepare completion */ +#define SDW_DEFAULT_DP_PREP_POLL_US 500 + /** * enum sdw_slave_status - Slave status * @SDW_SLAVE_UNATTACHED: Slave is not attached with the bus. @@ -295,6 +298,8 @@ struct sdw_dpn_audio_mode { * @simple_ch_prep_sm: If the port supports simplified channel prepare state * machine * @ch_prep_timeout: Port-specific timeout value, in milliseconds + * @ch_prep_poll_us: Interval to poll for CP_SM prepare completion. Zero means + * poll at SDW_DEFAULT_DP_PREP_POLL_US intervals. * @imp_def_interrupts: If set, each bit corresponds to support for * implementation-defined interrupts * @max_ch: Maximum channels supported @@ -321,6 +326,7 @@ struct sdw_dpn_prop { u32 max_grouping; bool simple_ch_prep_sm; u32 ch_prep_timeout; + u32 ch_prep_poll_us; u32 imp_def_interrupts; u32 max_ch; u32 min_ch; @@ -641,7 +647,6 @@ struct sdw_slave_ops { * @prop: Slave properties * @debugfs: Slave debugfs * @node: node for bus list - * @port_ready: Port ready completion flag for each Slave port * @m_port_map: static Master port map for each Slave port * @dev_num: Current Device Number, values can be 0 or dev_num_sticky * @dev_num_sticky: one-time static Device Number assigned by Bus @@ -673,7 +678,6 @@ struct sdw_slave { struct dentry *debugfs; #endif struct list_head node; - struct completion port_ready[SDW_MAX_PORTS]; unsigned int m_port_map[SDW_MAX_PORTS]; u16 dev_num; u16 dev_num_sticky;
sdw_prep_deprep_slave_ports() cannot use the port interrupt to signal CP_SM completion because it is called while holding the bus lock, which blocks the alert handler from running. Change to polling the PREPARESTATUS register and remove the infrastructure that was implemented for the previous interrupt waiting. A new configuration field 'ch_prep_poll_us' is added to struct sdw_dpn_prop so that the peripheral driver may select a polling interval. If this is left at zero a default interval of 500 usec is used. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> --- drivers/soundwire/bus.c | 2 -- drivers/soundwire/slave.c | 4 ---- drivers/soundwire/stream.c | 24 ++++++++++++++---------- include/linux/soundwire/sdw.h | 8 ++++++-- 4 files changed, 20 insertions(+), 18 deletions(-)