Re: [PATCH] brcmfmac: transfer firmware error to Linux error code in fwil

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 11/24/2017 7:35 AM, Wright Feng wrote:
> Hi Arend,
>

[...]

>>
>> However, the consequence of this was that we ended up returning
>> firmware error codes into the kernel domain and user-space >
>> Below the change I came up with. Let me know what you think.
> Thanks for the info, now I know the whole history.
> And I have 2 comments below.
>>
>> Regards,
>> Arend
>> ---8<--------------------------------------------------------
>> commit e340892a4bb5a74248b19504bb972497d38a4a03
>> Author: Arend van Spriel <arend.vanspriel@broadcom.com>
>> Date:   Thu Nov 23 11:13:41 2017 +0100
>>
>>      brcmfmac: separate firmware errors from i/o errors
>>
>>      When using the firmware api it can fail simply because firmware does
>>      not like the request or it fails due to issues in the host
>> interface.
>>      Currently, there is only a single error code which is confusing. So
>>      adding a parameter to pass the firmware error separately and in case
>>      of a firmware error always return -EBADE to user-space.
>>
>>      Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
>>      Reviewed-by: Pieter-Paul Giesberts
>> <pieter-paul.giesberts@broadcom.com>
>>      Reviewed-by: Franky Lin <franky.lin@broadcom.com>
>>      Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> index 9f2d0b0..353ed28 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcdc.c
>> @@ -165,7 +165,7 @@ static int brcmf_proto_bcdc_cmplt(struct brcmf_pub
>> *drvr, u32 id, u32 len)
>>
>>   static int
>>   brcmf_proto_bcdc_query_dcmd(struct brcmf_pub *drvr, int ifidx, uint
>> cmd,
>> -                void *buf, uint len)
>> +                void *buf, uint len, int *fwerr)
>>   {
>>       struct brcmf_bcdc *bcdc = (struct brcmf_bcdc *)drvr->proto->pd;
>>       struct brcmf_proto_bcdc_dcmd *msg = &bcdc->msg;
>> @@ -175,6 +175,7 @@ static int brcmf_proto_bcdc_cmplt(struct brcmf_pub
>> *drvr, u32 id, u32 len)
>>
>>       brcmf_dbg(BCDC, "Enter, cmd %d len %d\n", cmd, len);
>>
>> +    *fwerr = 0;
>>       ret = brcmf_proto_bcdc_msg(drvr, ifidx, cmd, buf, len, false);
>>       if (ret < 0) {
>>           brcmf_err("brcmf_proto_bcdc_msg failed w/status %d\n",
>> @@ -211,17 +212,18 @@ static int brcmf_proto_bcdc_cmplt(struct
>> brcmf_pub *drvr, u32 id, u32 len)
>>           memcpy(buf, info, len);
>>       }
>>
>> +    ret = 0;
>> +
>>       /* Check the ERROR flag */
>>       if (flags & BCDC_DCMD_ERROR)
>> -        ret = le32_to_cpu(msg->status);
>> -
>> +        *fwerr = le32_to_cpu(msg->status);
>>   done:
>>       return ret;
>>   }
>>
>>   static int
>>   brcmf_proto_bcdc_set_dcmd(struct brcmf_pub *drvr, int ifidx, uint cmd,
>> -              void *buf, uint len)
>> +              void *buf, uint len, int *fwerr)
>>   {
>>       struct brcmf_bcdc *bcdc = (struct brcmf_bcdc *)drvr->proto->pd;
>>       struct brcmf_proto_bcdc_dcmd *msg = &bcdc->msg;
>> @@ -230,6 +232,7 @@ static int brcmf_proto_bcdc_cmplt(struct brcmf_pub
>> *drvr, u32 id, u32 len)
>>
>>       brcmf_dbg(BCDC, "Enter, cmd %d len %d\n", cmd, len);
>>
>> +    *fwerr = 0;
>>       ret = brcmf_proto_bcdc_msg(drvr, ifidx, cmd, buf, len, true);
>>       if (ret < 0)
>>           goto done;
>> @@ -251,7 +254,7 @@ static int brcmf_proto_bcdc_cmplt(struct brcmf_pub
>> *drvr, u32 id, u32 len)
>>
> Does it exist any possible case or set command that causes the ret value
> is greater than 0? The positive ret value may be identified as error
> because following check is removed from brcmf_fil_cmd_data function.
>      if (err >= 0)
>          return 0;

The only possible case is with brcmf_proto_bcdc_query_dcmd() as it 
returns the length of the firmware response. However, I added ret = 0 
there after dealing with (ret < 0) case. I actually want to do that in a 
separate patch.

>
>>       /* Check the ERROR flag */
>>       if (flags & BCDC_DCMD_ERROR)
>> -        ret = le32_to_cpu(msg->status);
>> +        *fwerr = le32_to_cpu(msg->status);
>>
>>   done:
>>       return ret;

[...]

>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> index 2404f8a..8a8e08f 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/proto.h
>> @@ -30,9 +30,9 @@ struct brcmf_proto {
>>       int (*hdrpull)(struct brcmf_pub *drvr, bool do_fws,
>>                  struct sk_buff *skb, struct brcmf_if **ifp);
>>       int (*query_dcmd)(struct brcmf_pub *drvr, int ifidx, uint cmd,
>> -              void *buf, uint len);
>> +              void *buf, uint len, int *fwerr);
>>       int (*set_dcmd)(struct brcmf_pub *drvr, int ifidx, uint cmd,
>> void *buf,
>> -            uint len);
>> +            uint len, int *fwerr);
>>       int (*tx_queue_data)(struct brcmf_pub *drvr, int ifidx,
>>                    struct sk_buff *skb);
>>       int (*txdata)(struct brcmf_pub *drvr, int ifidx, u8 offset,
>> @@ -71,14 +71,16 @@ static inline int brcmf_proto_hdrpull(struct
>> brcmf_pub *drvr, bool do_fws,
>>       return drvr->proto->hdrpull(drvr, do_fws, skb, ifp);
>>   }
>>   static inline int brcmf_proto_query_dcmd(struct brcmf_pub *drvr, int
>> ifidx,
>> -                     uint cmd, void *buf, uint len)
>> +                     uint cmd, void *buf, uint len,
>> +                     int *fwerr)
>>   {
>> -    return drvr->proto->query_dcmd(drvr, ifidx, cmd, buf, len);
>> +    return drvr->proto->query_dcmd(drvr, ifidx, cmd, buf, len,fwerr);
> A blank space is missing before fwerr.

Good catch. Will fix it.

Thanks,
Arend