Re: [PATCH v4 06/19] rtw89: add files to download and communicate with firmware

[Brian Norris <briannorris@chromium.org>]

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;
> +}