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 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.
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.

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