| Message ID | 20190725234032.21152-16-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: updates for 5.4 | expand |
On Thu, Jul 25, 2019 at 06:40:07PM -0500, Pierre-Louis Bossart wrote: > When a Slave reports multiple status in the sticky bits, find the > latest configuration from the mirror of the PING frame status and > update the status directly. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/cadence_master.c | 34 ++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index 889fa2cd49ae..25d5c7267c15 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, > > /* first check if Slave reported multiple status */ > if (set_status > 1) { > + u32 val; > + > dev_warn_ratelimited(cdns->dev, > - "Slave reported multiple Status: %d\n", > - mask); > - /* > - * TODO: we need to reread the status here by > - * issuing a PING cmd > - */ > + "Slave %d reported multiple Status: %d\n", > + i, mask); > + > + /* re-check latest status extracted from PING commands */ > + val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); > + val >>= (i * 2); Superfluous parentheses. > + > + switch (val & 0x3) { > + case 0: > + status[i] = SDW_SLAVE_UNATTACHED; > + break; > + case 1: > + status[i] = SDW_SLAVE_ATTACHED; > + break; > + case 2: > + status[i] = SDW_SLAVE_ALERT; > + break; > + default: There aren't many values left for the "default" case :-) But I'm not sure whether any of + case 3: or + case 3: + default: would improve readability. Thanks Guennadi > + status[i] = SDW_SLAVE_RESERVED; > + break; > + } > + > + dev_warn_ratelimited(cdns->dev, > + "Slave %d status updated to %d\n", > + i, status[i]); > + > } > } > > -- > 2.20.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c >> index 889fa2cd49ae..25d5c7267c15 100644 >> --- a/drivers/soundwire/cadence_master.c >> +++ b/drivers/soundwire/cadence_master.c >> @@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, >> >> /* first check if Slave reported multiple status */ >> if (set_status > 1) { >> + u32 val; >> + >> dev_warn_ratelimited(cdns->dev, >> - "Slave reported multiple Status: %d\n", >> - mask); >> - /* >> - * TODO: we need to reread the status here by >> - * issuing a PING cmd >> - */ >> + "Slave %d reported multiple Status: %d\n", >> + i, mask); >> + >> + /* re-check latest status extracted from PING commands */ >> + val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); >> + val >>= (i * 2); > > Superfluous parentheses. Humm, I don't know my left from my right and I can't remember operator precedence, so i'd rather make it explicit... > >> + >> + switch (val & 0x3) { >> + case 0: >> + status[i] = SDW_SLAVE_UNATTACHED; >> + break; >> + case 1: >> + status[i] = SDW_SLAVE_ATTACHED; >> + break; >> + case 2: >> + status[i] = SDW_SLAVE_ALERT; >> + break; >> + default: > > There aren't many values left for the "default" case :-) But I'm not sure whether > any of > > + case 3: > > or > > + case 3: > + default: > > would improve readability. > > Thanks > Guennadi > >> + status[i] = SDW_SLAVE_RESERVED; >> + break; Yes, those defaults are annoying. Some tools complain so I tend to leave them.
On 25-07-19, 18:40, Pierre-Louis Bossart wrote: > When a Slave reports multiple status in the sticky bits, find the > latest configuration from the mirror of the PING frame status and > update the status directly. > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/cadence_master.c | 34 ++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c > index 889fa2cd49ae..25d5c7267c15 100644 > --- a/drivers/soundwire/cadence_master.c > +++ b/drivers/soundwire/cadence_master.c > @@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, > > /* first check if Slave reported multiple status */ > if (set_status > 1) { > + u32 val; > + > dev_warn_ratelimited(cdns->dev, > - "Slave reported multiple Status: %d\n", > - mask); > - /* > - * TODO: we need to reread the status here by > - * issuing a PING cmd > - */ > + "Slave %d reported multiple Status: %d\n", > + i, mask); > + > + /* re-check latest status extracted from PING commands */ > + val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); > + val >>= (i * 2); > + > + switch (val & 0x3) { > + case 0: why not case CDNS_MCP_SLAVE_INTSTAT_NPRESENT: > + status[i] = SDW_SLAVE_UNATTACHED; > + break; > + case 1: > + status[i] = SDW_SLAVE_ATTACHED; > + break; > + case 2: > + status[i] = SDW_SLAVE_ALERT; > + break; > + default: > + status[i] = SDW_SLAVE_RESERVED; > + break; > + } we have same logic in the code block preceding this, maybe good idea to write a helper and use for both Also IIRC we can have multiple status set right?
On 8/2/19 7:20 AM, Vinod Koul wrote: > On 25-07-19, 18:40, Pierre-Louis Bossart wrote: >> When a Slave reports multiple status in the sticky bits, find the >> latest configuration from the mirror of the PING frame status and >> update the status directly. >> >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> --- >> drivers/soundwire/cadence_master.c | 34 ++++++++++++++++++++++++------ >> 1 file changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c >> index 889fa2cd49ae..25d5c7267c15 100644 >> --- a/drivers/soundwire/cadence_master.c >> +++ b/drivers/soundwire/cadence_master.c >> @@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, >> >> /* first check if Slave reported multiple status */ >> if (set_status > 1) { >> + u32 val; >> + >> dev_warn_ratelimited(cdns->dev, >> - "Slave reported multiple Status: %d\n", >> - mask); >> - /* >> - * TODO: we need to reread the status here by >> - * issuing a PING cmd >> - */ >> + "Slave %d reported multiple Status: %d\n", >> + i, mask); >> + >> + /* re-check latest status extracted from PING commands */ >> + val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); >> + val >>= (i * 2); >> + >> + switch (val & 0x3) { >> + case 0: > > why not case CDNS_MCP_SLAVE_INTSTAT_NPRESENT: ok > >> + status[i] = SDW_SLAVE_UNATTACHED; >> + break; >> + case 1: >> + status[i] = SDW_SLAVE_ATTACHED; >> + break; >> + case 2: >> + status[i] = SDW_SLAVE_ALERT; >> + break; >> + default: >> + status[i] = SDW_SLAVE_RESERVED; >> + break; >> + } > > we have same logic in the code block preceding this, maybe good idea to > write a helper and use for both Yes, I am thinking about this. There are multiple cases where we want to re-check the status and clear some bits, so helpers would be good. > > Also IIRC we can have multiple status set right? Yes, the status bits are sticky and mirror all values reported in PING frames. I am still working on how to clear those bits, there are cases where we clear bits and end-up never hearing from that device ever again. classic edge/level issue I suppose.
On 02-08-19, 10:29, Pierre-Louis Bossart wrote: > On 8/2/19 7:20 AM, Vinod Koul wrote: > > On 25-07-19, 18:40, Pierre-Louis Bossart wrote: > > > + status[i] = SDW_SLAVE_UNATTACHED; > > > + break; > > > + case 1: > > > + status[i] = SDW_SLAVE_ATTACHED; > > > + break; > > > + case 2: > > > + status[i] = SDW_SLAVE_ALERT; > > > + break; > > > + default: > > > + status[i] = SDW_SLAVE_RESERVED; > > > + break; > > > + } > > > > we have same logic in the code block preceding this, maybe good idea to > > write a helper and use for both > > Yes, I am thinking about this. There are multiple cases where we want to > re-check the status and clear some bits, so helpers would be good. > > > > > Also IIRC we can have multiple status set right? > > Yes, the status bits are sticky and mirror all values reported in PING > frames. I am still working on how to clear those bits, there are cases where > we clear bits and end-up never hearing from that device ever again. classic > edge/level issue I suppose. Then the case logic above doesn't work, it should be like the code block preceding this.. Thanks
On 8/2/19 11:01 AM, Vinod Koul wrote: > On 02-08-19, 10:29, Pierre-Louis Bossart wrote: >> On 8/2/19 7:20 AM, Vinod Koul wrote: >>> On 25-07-19, 18:40, Pierre-Louis Bossart wrote: > >>>> + status[i] = SDW_SLAVE_UNATTACHED; >>>> + break; >>>> + case 1: >>>> + status[i] = SDW_SLAVE_ATTACHED; >>>> + break; >>>> + case 2: >>>> + status[i] = SDW_SLAVE_ALERT; >>>> + break; >>>> + default: >>>> + status[i] = SDW_SLAVE_RESERVED; >>>> + break; >>>> + } >>> >>> we have same logic in the code block preceding this, maybe good idea to >>> write a helper and use for both >> >> Yes, I am thinking about this. There are multiple cases where we want to >> re-check the status and clear some bits, so helpers would be good. >> >>> >>> Also IIRC we can have multiple status set right? >> >> Yes, the status bits are sticky and mirror all values reported in PING >> frames. I am still working on how to clear those bits, there are cases where >> we clear bits and end-up never hearing from that device ever again. classic >> edge/level issue I suppose. > > Then the case logic above doesn't work, it should be like the code block > preceding this.. what I was referring to already is a problem even in single status mode. Let's say for example that a device shows up as Device0, then we try to enumerate it and program a non-zero device number. If that fails, we currently clear the Attached status for Device0, so will never have an interrupt ever again. The device is there, attached as Device0, but we've lost the single opportunity to make it usable. The link is in most cases going to be extremely reliable, but if we know of state machines that lead to a terminal state then we should proactively have a recovery mechanism to avoid complicated debug down the road for cases where the hardware has transient issues. For the multiple status case, we will have to look at the details and figure out which of the flags get cleared and which ones don't. One certainty is that we absolutely have to track IO errors in interrupt context. They are recoverable in regular context but not quite in interrupt context if we clear the status bits unconditionally. Maybe a tangent here but to be transparent there are really multiple topics we are tracking at the moment: 1. error handling in interrupts. I found a leak where if a device goes in the weeds while we program its device number and resynchronizes then we allocate a new device number instead of reusing the initial one. The bit clearing is also to be checked as explained above. 2. module dependencies: there is a race condition leading to a kernel oops if the Slave probe is not complete before the .update_status is invoked. 3. jack detection. The jack detection routine is called as a result of an imp-def Slave interrupt. We never documented the assumption that if this jack detection takes time then it needs to be done offline, e.g. in a work queue. Or if we still want it to be done in a the interrupt thread then we need to re-enable interrupts earlier, otherwise one device can stop interrupt handling for a fairly long duration. 4. streaming stop on link errors. We've seen in tests that if you reset the link or a Slave device with debugfs while audio is playing then streaming continues. This condition could happen if a device loses sync, and the spec says the Slave needs to reset its channel enable bits. At the command level, we handle this situation and will recover, but there is no notification to the ALSA layers to try and recover on the PCM side of things (as if it were an underflow condition). We also try to disable a stream but get all kinds of errors since it's lost state. All of those points are corner cases but they are important to solve for actual products.
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 889fa2cd49ae..25d5c7267c15 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -643,13 +643,35 @@ static int cdns_update_slave_status(struct sdw_cdns *cdns, /* first check if Slave reported multiple status */ if (set_status > 1) { + u32 val; + dev_warn_ratelimited(cdns->dev, - "Slave reported multiple Status: %d\n", - mask); - /* - * TODO: we need to reread the status here by - * issuing a PING cmd - */ + "Slave %d reported multiple Status: %d\n", + i, mask); + + /* re-check latest status extracted from PING commands */ + val = cdns_readl(cdns, CDNS_MCP_SLAVE_STAT); + val >>= (i * 2); + + switch (val & 0x3) { + case 0: + status[i] = SDW_SLAVE_UNATTACHED; + break; + case 1: + status[i] = SDW_SLAVE_ATTACHED; + break; + case 2: + status[i] = SDW_SLAVE_ALERT; + break; + default: + status[i] = SDW_SLAVE_RESERVED; + break; + } + + dev_warn_ratelimited(cdns->dev, + "Slave %d status updated to %d\n", + i, status[i]); + } }
When a Slave reports multiple status in the sticky bits, find the latest configuration from the mirror of the PING frame status and update the status directly. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/soundwire/cadence_master.c | 34 ++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-)