Re: [PATCH] wifi: rtw89: use int type to store negative error codes

[Qianfeng Rong <rongqianfeng@vivo.com>]

在 2025/8/27 8:42, Ping-Ke Shih 写道:
> Qianfeng Rong <rongqianfeng@vivo.com> wrote:
>> The 'ret' variable stores returns from other functions, which return
>> either zero on success or negative error codes on failure.  Storing
>> error codes in u32 (an unsigned type) causes no runtime issues but is
>> stylistically inconsistent and very ugly.  Change 'ret' from u32 to
>> int - this has no runtime impact.
>>
>> Signed-off-by: Qianfeng Rong <rongqianfeng@vivo.com>
>> ---
>>   drivers/net/wireless/realtek/rtw89/fw.c  |  7 ++++---
>>   drivers/net/wireless/realtek/rtw89/mac.c | 16 ++++++++--------
>>   drivers/net/wireless/realtek/rtw89/pci.c |  4 ++--
>>   3 files changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtw89/fw.c b/drivers/net/wireless/realtek/rtw89/fw.c
>> index 16e59a4a486e..01d53f7c142d 100644
>> --- a/drivers/net/wireless/realtek/rtw89/fw.c
>> +++ b/drivers/net/wireless/realtek/rtw89/fw.c
>> @@ -1537,7 +1537,7 @@ static int __rtw89_fw_download_hdr(struct rtw89_dev *rtwdev,
>>          struct rtw89_fw_hdr *fw_hdr;
>>          struct sk_buff *skb;
>>          u32 truncated;
>> -       u32 ret = 0;
>> +       int ret = 0;
> Initializer is not necessary, by the way.
>
>>          skb = rtw89_fw_h2c_alloc_skb_with_hdr(rtwdev, len);
>>          if (!skb) {
>> @@ -6826,7 +6826,8 @@ static int rtw89_fw_read_c2h_reg(struct rtw89_dev *rtwdev,
>>          const struct rtw89_chip_info *chip = rtwdev->chip;
>>          struct rtw89_fw_info *fw_info = &rtwdev->fw;
>>          const u32 *c2h_reg = chip->c2h_regs;
>> -       u32 ret, timeout;
>> +       u32 timeout;
>> +       int ret;
>>          u8 i, val;
> Keep it in reverse X'mas tree order.
>
>>          info->id = RTW89_FWCMD_C2HREG_FUNC_NULL;
>> @@ -6865,7 +6866,7 @@ int rtw89_fw_msg_reg(struct rtw89_dev *rtwdev,
>>                       struct rtw89_mac_h2c_info *h2c_info,
>>                       struct rtw89_mac_c2h_info *c2h_info)
>>   {
>> -       u32 ret;
>> +       int ret;
>>
>>          if (h2c_info && h2c_info->id != RTW89_FWCMD_H2CREG_FUNC_GET_FEATURE)
>>                  lockdep_assert_wiphy(rtwdev->hw->wiphy);
> [...]
>
>> @@ -3105,7 +3105,7 @@ int rtw89_mac_setup_phycap(struct rtw89_dev *rtwdev)
>>   static int rtw89_hw_sch_tx_en_h2c(struct rtw89_dev *rtwdev, u8 band,
>>                                    u16 tx_en_u16, u16 mask_u16)
>>   {
>> -       u32 ret;
>> +       int ret;
> Please move below to be reverse X'mas tree order.
>
>>          struct rtw89_mac_c2h_info c2h_info = {0};
>>          struct rtw89_mac_h2c_info h2c_info = {0};
>>          struct rtw89_h2creg_sch_tx_en *sch_tx_en = &h2c_info.u.sch_tx_en;
> (move here)
>
> [...]
>
>> @@ -4158,7 +4158,7 @@ static int rtw89_pci_lv1rst_stop_dma_ax(struct rtw89_dev *rtwdev)
>>
>>   static int rtw89_pci_lv1rst_start_dma_ax(struct rtw89_dev *rtwdev)
>>   {
>> -       u32 ret;
>> +       int ret;
>>
>>          if (rtwdev->chip->chip_id == RTL8852C)
>>                  return 0;
>
> The last statement of this function is 'return ret;', but actually it can
> just be 'return 0;'. Please change it by the way.

Thanks for taking the time to reply. All your suggestions will be 
modified in v2.

Best regards, Qianfeng