Messages in this thread | | | Subject | Re: [alsa-devel] [PATCH v4 06/15] soundwire: Add IO transfer | From | Pierre-Louis Bossart <> | Date | Tue, 5 Dec 2017 07:43:46 -0600 |
| |
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.
| |