Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer

[Pierre-Louis Bossart]

On 12/5/17 7:43 AM, Pierre-Louis Bossart wrote:
> On 12/5/17 12:31 AM, Vinod Koul wrote:
> > On Sun, Dec 03, 2017 at 09:01:41PM -0600, Pierre-Louis Bossart wrote:
> > > On 12/3/17 11:04 AM, Vinod Koul wrote:
> > > > On Fri, Dec 01, 2017 at 05:27:31PM -0600, Pierre-Louis Bossart wrote:
> >
> > Sorry looks like I missed replying to this one earlier.
> >
> > > > > > +static inline int find_response_code(enum sdw_command_response resp)
> > > > > > +{
> > > > > > +ÂÂÂ switch (resp) {
> > > > > > +ÂÂÂ case SDW_CMD_OK:
> > > > > > +ÂÂÂÂÂÂÂ return 0;
> > > > > > +
> > > > > > +ÂÂÂ case SDW_CMD_IGNORED:
> > > > > > +ÂÂÂÂÂÂÂ return -ENODATA;
> > > > > > +
> > > > > > +ÂÂÂ case SDW_CMD_TIMEOUT:
> > > > > > +ÂÂÂÂÂÂÂ return -ETIMEDOUT;
> > > > > > +
> > > > > > +ÂÂÂ default:
> > > > > > +ÂÂÂÂÂÂÂ return -EIO;
> > > > >
> > > > > the 'default' case will handle both SDW_CMD_FAIL (which is a bus event
> > > > > usually due to bus clash or parity issues) and SDW_CMD_FAIL_OTHER 
> > > > > (which is
> > > > > an imp-def IP event).
> > > > >
> > > > > Do they really belong in the same basket? From a debug perspective 
> > > > > there is
> > > > > quite a bit of information lost.
> > > >
> > > > at higher level the error handling is same. the information is not 
> > > > lost as
> > > > it is expected that you would log it at error source.
> > >
> > > I don't understand this. It's certainly not the same for me if you 
> > > detect an
> > > electric problem or if the IP is in the weeds. Logging at the source 
> > > is fine
> > > but this filtering prevents higher levels from doing anything different.
> >
> > The point is higher levels like here cant do much than bail out and 
> > complain.
> >
> > Can you point out what would be different behaviour in each of these 
> > cases?
> >
> > > > > > +static inline int do_transfer(struct sdw_bus *bus, struct sdw_msg 
> > > > > > *msg)
> > > > > > +{
> > > > > > +ÂÂÂ int retry = bus->prop.err_threshold;
> > > > > > +ÂÂÂ enum sdw_command_response resp;
> > > > > > +ÂÂÂ int ret = 0, i;
> > > > > > +
> > > > > > +ÂÂÂ for (i = 0; i <= retry; i++) {
> > > > > > +ÂÂÂÂÂÂÂ resp = bus->ops->xfer_msg(bus, msg);
> > > > > > +ÂÂÂÂÂÂÂ ret = find_response_code(resp);
> > > > > > +
> > > > > > +ÂÂÂÂÂÂÂ /* if cmd is ok or ignored return */
> > > > > > +ÂÂÂÂÂÂÂ if (ret == 0 || ret == -ENODATA)
> > > > >
> > > > > Can you document why you don't retry on a CMD_IGNORED? I know there 
> > > > > was a
> > > > > reason, I just can't remember it.
> > > >
> > > > CMD_IGNORED can be okay on broadcast. User of this API can retry all 
> > > > they
> > > > want!
> > >
> > > So you retry if this is a CMD_FAILED but let higher levels retry for
> > > CMD_IGNORED, sorry I don't see the logic.
> >
> > Yes that is right.
> >
> > If I am doing a broadcast read, lets say for Device Id registers, why 
> > in the
> > world would I want to retry? CMD_IGNORED is a valid response and 
> > required to
> > stop enumeration cycle in that case.
> >
> > But if I am not expecting a CMD_IGNORED response, I can very well go 
> > ahead
> > and retry from caller. The context is with caller and they can choose 
> > to do
> > appropriate handling.
> >
> > And I have clarified this couple of times to you already, not sure how 
> > many
> > more times I would have to do that.
>
> Until you clarify what you are doing.
> There is ONE case where IGNORED is a valid answer (reading the Prepare 
> not finished bits), and it should not only be documented but analyzed in 
> more details.
I meant Read SCP_DevID registers from Device0... prepare bits should 
never return a CMD_IGNORED

> For a write an IGNORED is never OK.
>
> > > > > Now that I think of it, the retry on TIMEOUT makes no sense to me. 
> > > > > The retry
> > > > > was intended for bus-level issues, where maybe a single bit error 
> > > > > causes an
> > > > > issue without consequences, but the TIMEOUT is a completely 
> > > > > different beast,
> > > > > it's the master IP that doesn't answer really, a completely 
> > > > > different case.
> > > >
> > > > well in those cases where you have blue wires, it actually helps :)
> > >
> > > Blue wires are not supposed to change electrical behavior. TIMEOUT is 
> > > only
> > > an internal SOC level issue, so no I don't get how this helps.
> > >
> > > You have a retry count that is provided in the BIOS/firmware through 
> > > disco
> > > properties and it's meant to bus errors. You are abusing the 
> > > definitions. A
> > > command failed is supposed to be detected at the frame rate, which is
> > > typically 20us. a timeout is likely a 100s of ms value, so if you 
> > > retry on
> > > top it's going to lock up the bus.
> >
> > The world is not perfect! A guy debugging setups needs all the help. I do
> > not see any reason for not to retry. Bus is anyway locked up while a
> > transfer is ongoing (we serialize transfers).
> >
> > Now if you feel this should be abhorred, I can change this for timeout.
>
> This TIMEOUT thing is your own definition, it's not part of the spec, so 
> I don't see how it can be lumped together with spec-related parts.
>
> It's fine to keep a retry but please document what the expectations are 
> for the TIMEOUT case.
>
> > > > > > +enum sdw_command_response {
> > > > > > +ÂÂÂ SDW_CMD_OK = 0,
> > > > > > +ÂÂÂ SDW_CMD_IGNORED = 1,
> > > > > > +ÂÂÂ SDW_CMD_FAIL = 2,
> > > > > > +ÂÂÂ SDW_CMD_TIMEOUT = 4,
> > > > > > +ÂÂÂ SDW_CMD_FAIL_OTHER = 8,
> > > > >
> > > > > Humm, I can't recall if/why this is a mask? does it need to be?
> > > >
> > > > mask, not following!
> > > >
> > > > Taking a wild guess that you are asking about last error, which is 
> > > > for SW
> > > > errors like malloc fail etc...
> > >
> > > no, I was asking why this is declared as if it was used for a 
> > > bitmask, why
> > > not 0,1,2,3,4?
> >
> > Oh okay, I think it was something to do with bits for errors, but don 
> > see it
> > helping so I can change it either way...
>
> Unless you use bit-wise operators and combined responses there is no 
> reason to keep the current definitions.
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel