[PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[Wang Yufen]

Fix to return a negative error code -EINVAL instead of 0.

Compile tested only.

Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 465d95d..329ec8ac 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
 	/* Take arm out of reset */
 	if (!brcmf_chip_set_active(bus->ci, rstvec)) {
 		brcmf_err("error getting out of ARM core reset\n");
+		bcmerror = -EINVAL;
 		goto err;
 	}
 
-- 
1.8.3.1

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[Franky Lin]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> wrote:
>
> Fix to return a negative error code -EINVAL instead of 0.
>
> Compile tested only.
>
> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 465d95d..329ec8ac 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
>         /* Take arm out of reset */
>         if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>                 brcmf_err("error getting out of ARM core reset\n");
> +               bcmerror = -EINVAL;

ENODEV seems more appropriate here.

>                 goto err;
>         }
>
> --
> 1.8.3.1
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4203 bytes --]

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[wangyufen]

在 2022/11/30 1:41, Franky Lin 写道:
> On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> wrote:
>>
>> Fix to return a negative error code -EINVAL instead of 0.
>>
>> Compile tested only.
>>
>> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index 465d95d..329ec8ac 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
>>          /* Take arm out of reset */
>>          if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>>                  brcmf_err("error getting out of ARM core reset\n");
>> +               bcmerror = -EINVAL;
> 
> ENODEV seems more appropriate here.

However, if brcmf_chip_set_active()  fails in 
brcmf_pcie_exit_download_state(), "-EINVAL" is returned.
Is it necessary to keep consistent?

> 
>>                  goto err;
>>          }
>>
>> --
>> 1.8.3.1
>>

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On 11/30/2022 3:00 AM, wangyufen wrote:
> 
> 
> 在 2022/11/30 1:41, Franky Lin 写道:
>> On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> wrote:
>>>
>>> Fix to return a negative error code -EINVAL instead of 0.
>>>
>>> Compile tested only.
>>>
>>> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>> ---
>>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> index 465d95d..329ec8ac 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct 
>>> brcmf_sdio *bus,
>>>          /* Take arm out of reset */
>>>          if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>>>                  brcmf_err("error getting out of ARM core reset\n");
>>> +               bcmerror = -EINVAL;
>>
>> ENODEV seems more appropriate here.
> 
> However, if brcmf_chip_set_active()  fails in 
> brcmf_pcie_exit_download_state(), "-EINVAL" is returned.
> Is it necessary to keep consistent?

If we can not get the ARM on the chip out of reset things will fail soon 
enough further down the road. Anyway, the other function calls return 
-EIO so let's do the same here.

Thanks,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[wangyufen]

在 2022/11/30 19:19, Arend van Spriel 写道:
> On 11/30/2022 3:00 AM, wangyufen wrote:
>>
>>
>> 在 2022/11/30 1:41, Franky Lin 写道:
>>> On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> wrote:
>>>>
>>>> Fix to return a negative error code -EINVAL instead of 0.
>>>>
>>>> Compile tested only.
>>>>
>>>> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
>>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>>> ---
>>>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> index 465d95d..329ec8ac 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct 
>>>> brcmf_sdio *bus,
>>>>          /* Take arm out of reset */
>>>>          if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>>>>                  brcmf_err("error getting out of ARM core reset\n");
>>>> +               bcmerror = -EINVAL;
>>>
>>> ENODEV seems more appropriate here.
>>
>> However, if brcmf_chip_set_active()  fails in 
>> brcmf_pcie_exit_download_state(), "-EINVAL" is returned.
>> Is it necessary to keep consistent?
> 
> If we can not get the ARM on the chip out of reset things will fail soon 
> enough further down the road. Anyway, the other function calls return 
> -EIO so let's do the same here.
> 

So -EIO is better?  Anyone else have any other opinions? 😄

Thanks,
Wang

> Thanks,
> Arend

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

On December 1, 2022 4:01:39 AM wangyufen <wangyufen@huawei.com> wrote:

> 在 2022/11/30 19:19, Arend van Spriel 写道:
>> On 11/30/2022 3:00 AM, wangyufen wrote:
>>>
>>>
>>> 在 2022/11/30 1:41, Franky Lin 写道:
>>>> On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> wrote:
>>>>>
>>>>> Fix to return a negative error code -EINVAL instead of 0.
>>>>>
>>>>> Compile tested only.
>>>>>
>>>>> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
>>>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>>>> ---
>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>> index 465d95d..329ec8ac 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct
>>>>> brcmf_sdio *bus,
>>>>>         /* Take arm out of reset */
>>>>>         if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>>>>>                 brcmf_err("error getting out of ARM core reset\n");
>>>>> +               bcmerror = -EINVAL;
>>>>
>>>> ENODEV seems more appropriate here.
>>>
>>> However, if brcmf_chip_set_active()  fails in
>>> brcmf_pcie_exit_download_state(), "-EINVAL" is returned.
>>> Is it necessary to keep consistent?
>>
>> If we can not get the ARM on the chip out of reset things will fail soon
>> enough further down the road. Anyway, the other function calls return
>> -EIO so let's do the same here.
>
> So -EIO is better?  Anyone else have any other opinions? 😄

Obviously it is no better than -EINVAL when you look at the behavior. It is 
just a feeble attempt to be a little bit more consistent. Feel free to 
change the return value for brcmf_pcie_exit_download_state() as well.

Regards,
Arend
>>




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On December 1, 2022 4:01:39 AM wangyufen <wangyufen@huawei.com> wrote:
>
>> 在 2022/11/30 19:19, Arend van Spriel 写道:
>>> On 11/30/2022 3:00 AM, wangyufen wrote:
>>>>
>>>>
>>>> 在 2022/11/30 1:41, Franky Lin 写道:
>>>>> On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> wrote:
>>>>>>
>>>>>> Fix to return a negative error code -EINVAL instead of 0.
>>>>>>
>>>>>> Compile tested only.
>>>>>>
>>>>>> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
>>>>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>>>>> ---
>>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> index 465d95d..329ec8ac 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct
>>>>>> brcmf_sdio *bus,
>>>>>>         /* Take arm out of reset */
>>>>>>         if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>>>>>>                 brcmf_err("error getting out of ARM core reset\n");
>>>>>> +               bcmerror = -EINVAL;
>>>>>
>>>>> ENODEV seems more appropriate here.
>>>>
>>>> However, if brcmf_chip_set_active()  fails in
>>>> brcmf_pcie_exit_download_state(), "-EINVAL" is returned.
>>>> Is it necessary to keep consistent?
>>>
>>> If we can not get the ARM on the chip out of reset things will fail soon
>>> enough further down the road. Anyway, the other function calls return
>>> -EIO so let's do the same here.
>>
>> So -EIO is better?  Anyone else have any other opinions? 😄
>
> Obviously it is no better than -EINVAL when you look at the behavior.
> It is just a feeble attempt to be a little bit more consistent. Feel
> free to change the return value for brcmf_pcie_exit_download_state()
> as well.

Weirdly Arend's last comment is not visible in patchwork:

https://patchwork.kernel.org/project/linux-wireless/patch/1669716458-15327-1-git-send-email-wangyufen@huawei.com/

His last email is visible, but the last paragraph is not shown. Some
strange hiccup somewhere I guess, just wanted to mention it in case we
see more of them.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: brcmfmac: Fix error return code in brcmf_sdio_download_firmware()

[wangyufen]

在 2022/12/1 14:18, Arend Van Spriel 写道:
> On December 1, 2022 4:01:39 AM wangyufen <wangyufen@huawei.com> wrote:
> 
>> 在 2022/11/30 19:19, Arend van Spriel 写道:
>>> On 11/30/2022 3:00 AM, wangyufen wrote:
>>>>
>>>>
>>>> 在 2022/11/30 1:41, Franky Lin 写道:
>>>>> On Tue, Nov 29, 2022 at 1:47 AM Wang Yufen <wangyufen@huawei.com> 
>>>>> wrote:
>>>>>>
>>>>>> Fix to return a negative error code -EINVAL instead of 0.
>>>>>>
>>>>>> Compile tested only.
>>>>>>
>>>>>> Fixes: d380ebc9b6fb ("brcmfmac: rename chip download functions")
>>>>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>>>>> ---
>>>>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 +
>>>>>>  1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> index 465d95d..329ec8ac 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> @@ -3414,6 +3414,7 @@ static int brcmf_sdio_download_firmware(struct
>>>>>> brcmf_sdio *bus,
>>>>>>         /* Take arm out of reset */
>>>>>>         if (!brcmf_chip_set_active(bus->ci, rstvec)) {
>>>>>>                 brcmf_err("error getting out of ARM core reset\n");
>>>>>> +               bcmerror = -EINVAL;
>>>>>
>>>>> ENODEV seems more appropriate here.
>>>>
>>>> However, if brcmf_chip_set_active()  fails in
>>>> brcmf_pcie_exit_download_state(), "-EINVAL" is returned.
>>>> Is it necessary to keep consistent?
>>>
>>> If we can not get the ARM on the chip out of reset things will fail soon
>>> enough further down the road. Anyway, the other function calls return
>>> -EIO so let's do the same here.
>>
>> So -EIO is better?  Anyone else have any other opinions? 😄
> 
> Obviously it is no better than -EINVAL when you look at the behavior. It 
> is just a feeble attempt to be a little bit more consistent. Feel free 
> to change the return value for brcmf_pcie_exit_download_state() as well.
> 
All right, I'll send a v2 and change the error return values on both 
sides to -EIO.

Thanks，
Wang
> Regards,
> Arend
>>>
> 
> 
>