Small review note, since I was looking through the PS code due to
another bug in this patchset:
On Thu, Apr 29, 2021 at 04:01:36PM +0800, Ping-Ke Shih wrote:
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtw89/fw.c
> +
> +#define H2C_LPS_PARM_LEN 8
> +int rtw89_fw_h2c_lps_parm(struct rtw89_dev *rtwdev, u8 macid)
You're not actually using the macid param from this function. Instead,
you're implicitly passing data to this function via
rtwdev->lps_parm...except that it looks like you only set and use it
directly in this callchain, and you don't actually need to save it in
your driver structure.
IIUC, I believe this would be clearer and less error-prone if you just
pass a 'struct rtw89_lps_parm' arg (here, and in
rtw89_fw_h2c_general_pkt()), and drop 'struct rtw89_lps_parm' from
rtwdev.
Brian
> +{
> + struct sk_buff *skb;
> + struct rtw89_lps_parm *lps_param = &rtwdev->lps_parm;
> +
> + skb = rtw89_fw_h2c_alloc_skb_with_hdr(H2C_LPS_PARM_LEN);
> + if (!skb) {
> + rtw89_err(rtwdev, "failed to alloc skb for fw dl\n");
> + return -ENOMEM;
> + }
> + skb_put(skb, H2C_LPS_PARM_LEN);
> +
> + SET_LPS_PARM_MACID(skb->data, lps_param->macid);
> + SET_LPS_PARM_PSMODE(skb->data, lps_param->psmode);
> + SET_LPS_PARM_LASTRPWM(skb->data, lps_param->lastrpwm);
> + SET_LPS_PARM_RLBM(skb->data, 1);
> + SET_LPS_PARM_SMARTPS(skb->data, 1);
> + SET_LPS_PARM_AWAKEINTERVAL(skb->data, 1);
> + SET_LPS_PARM_VOUAPSD(skb->data, 0);
> + SET_LPS_PARM_VIUAPSD(skb->data, 0);
> + SET_LPS_PARM_BEUAPSD(skb->data, 0);
> + SET_LPS_PARM_BKUAPSD(skb->data, 0);
> +
> + rtw89_h2c_pkt_set_hdr(rtwdev, skb, FWCMD_TYPE_H2C,
> + H2C_CAT_MAC,
> + H2C_CL_MAC_PS,
> + H2C_FUNC_MAC_LPS_PARM, 0, 1,
> + H2C_LPS_PARM_LEN);
> +
> + if (rtw89_h2c_tx(rtwdev, skb, false)) {
> + rtw89_err(rtwdev, "failed to send h2c\n");
> + goto fail;
> + }
> +
> + return 0;
> +fail:
> + dev_kfree_skb_any(skb);
> +
> + return -EBUSY;
> +}
