RE: [RFC v2 06/12] rtw88: fw and efuse files

[Tony Chuang <yhchuang@realtek.com>]

> -----Original Message-----
> From: Stanislaw Gruszka [mailto:sgruszka@redhat.com]
> Sent: Thursday, October 04, 2018 6:50 PM
> To: Tony Chuang
> Cc: kvalo@codeaurora.org; Larry.Finger@lwfinger.net; Pkshih; Andy Huang;
> linux-wireless@vger.kernel.org
> Subject: Re: [RFC v2 06/12] rtw88: fw and efuse files
> 
> Hi
> 
> On Wed, Oct 03, 2018 at 04:02:22PM +0800, yhchuang@realtek.com wrote:
> > +void rtw_fw_do_iqk(struct rtw_dev *rtwdev, struct rtw_iqk_para *para)
> > +{
> > +	u8 h2c_pkt[H2C_PKT_SIZE] = {0};
> 
> Not sure if '= {0}' work as expected for arrays, you
> want to nulify first byte or whole h2c_pkt ?

I want to nullify the whole h2c_pkt.
And I think it works as expected.

> 
> > +void rtw_add_rsvd_page(struct rtw_dev *rtwdev, enum
> rtw_rsvd_packet_type type,
> > +		       bool txdesc)
> > +{
> > +	struct rtw_rsvd_page *rsvd_pkt;
> > +
> > +	list_for_each_entry(rsvd_pkt, &rtwdev->rsvd_page_list, list) {
> > +		if (rsvd_pkt->type == type)
> > +			return;
> > +	}
> > +
> > +	rsvd_pkt = kmalloc(sizeof(*rsvd_pkt),
> > +			   in_interrupt() ? GFP_ATOMIC : GFP_KERNEL);
> 
> It's never called from interrupt. You have to check kmalloc output value.
> 

OK, it's better.

> > +	rsvd_pkt->type = type;
> > +	rsvd_pkt->add_txdesc = txdesc;
> > +	list_add_tail(&rsvd_pkt->list, &rtwdev->rsvd_page_list);
> <snip>
> > +
> > +void rtw_reset_rsvd_page(struct rtw_dev *rtwdev)
> > +{
> > +	struct rtw_rsvd_page *rsvd_pkt, *tmp;
> > +
> > +	list_for_each_entry_safe(rsvd_pkt, tmp, &rtwdev->rsvd_page_list, list) {
> > +		if (rsvd_pkt->type == RSVD_BEACON)
> > +			continue;
> > +		list_del(&rsvd_pkt->list);
> > +		kfree(rsvd_pkt);
> > +	}
> 
> It also not clear how the list is protected agains concurrent access.
> However seems like much simpler solution would be use array instead of
> the list. For example above code could be replaced by:


It should be protected bu rtwdev->mutex, but indeed it is not clear
Would have to add lockdep for it.


> 
> for (i = RSVD_BEACON + 1; i <= RSVD_QOS_NULL ; i++) {
> 	kfree(rwdev->rsvd_page_arr[i]));
> 	rsvd_page_arr[i] = NULL;
> }
> 
> And other operations like adding / removing / checking if present could
> be simplified as well.
> 
> Also RSVD_PROBE_RESP page is never created.

RSVD_PROBE_RESP will be used for AP/GO mode,
so I think I might leave it here.

And change rsvd_list to array is not a good idea if there is more than
one interfaces running. For this case will need to download more rsvd
packets, and the order can be changed. Array seems not enough to
handle that.

> 
> > +	*size = (total_page - 1) * page_size + page_margin;
> > +	buf = kzalloc(*size, GFP_KERNEL | GFP_ATOMIC);
> 
> Those GPF flags contradict to each other :-)
> and buf not checked agains NULL.

fixed

> 
> Thanks
> Stanislaw
> 

Thanks
Yan-Hsuan Chuang