lkml.org 
[lkml]   [2017]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer
From
Date
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@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

\
 
 \ /
  Last update: 2017-12-05 15:49    [W:0.445 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site