Re: [PATCH] ath10k: fix vdev-start timeout on error

[Ben Greear <greearb@candelatech.com>]

On 07/13/2018 10:17 AM, Michał Kazior wrote:
> On 13 July 2018 at 19:08,  <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> The vdev-start-response message should cause the
>> completion to fire, even in the error case.  Otherwise,
>> the user still gets no useful information and everything
>> is blocked until the timeout period.
>>
>> Add some warning text to print out the invalid status
>> code to aid debugging.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/wmi.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
>> index a60de71..ec4cd1e 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
>> @@ -3443,12 +3443,18 @@ void ath10k_wmi_event_vdev_start_resp(struct ath10k *ar, struct sk_buff *skb)
>>         ret = ath10k_wmi_pull_vdev_start(ar, skb, &arg);
>>         if (ret) {
>>                 ath10k_warn(ar, "failed to parse vdev start event: %d\n", ret);
>> -               return;
>> +               goto out;
>>         }
>>
>> -       if (WARN_ON(__le32_to_cpu(arg.status)))
>> -               return;
>> +       if (WARN_ON_ONCE(__le32_to_cpu(arg.status))) {
>> +               ath10k_warn(ar, "vdev-start-response reports status error: %d\n",
>> +                           __le32_to_cpu(arg.status));
>> +               /* Setup is done one way or another though, so we should still
>> +                * do the completion, so don't return here.
>> +                */
>> +       }
>>
>> +out:
>>         complete(&ar->vdev_setup_done);
>
> With this the waiter can no longer tell if vdev_start succeeded or
> not. It'll always think it succeeded even if arg.status or parsing
> failed. Waiter instead of erroring out will continue to play out happy
> scenario and may end up crashing firmware.
>
> Not stalling is nice, but I'd argue the status should be propagated
> back to the waiter so it can error-check.

So, maybe set ar->last_wmi_error = __le32_to_cpu(arg.status) before calling
the complete and change the code that waits for vdev_setup_done to check that
error code?

Or, maybe we need an error code specific to this call, ar->last_wmi_vdev_start_status?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com