Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Apr 2021 18:10:30 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     kvalo@codeaurora.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCH v4 06/19] rtw89: add files to download and communicate
 with firmware
Message-ID: <YItZBolH5sSYZT3v@google.com>
References: <20210429080149.7068-1-pkshih@realtek.com>
 <20210429080149.7068-7-pkshih@realtek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210429080149.7068-7-pkshih@realtek.com>
Precedence: bulk

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