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

[Stanislaw Gruszka <sgruszka@redhat.com>]

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 ?

> +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.

> +	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:

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.

> +	*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.

Thanks
Stanislaw