[External Mail] Re: [PATCH v3 3/7] net: wwan: t9xx: Add control DMA interface

[Wu. JackBB (GSM)]

Hi Andrew,

> > +static inline void mtk_cldma_clr_bd_dsc(struct cldma_drv_info *drv_info,
> > +					struct bd_dsc *bd_dsc_pool, int nr_bds)
>
> No inline functions in C files. Please let the compiler decide.

Will remove inline from all static functions in .c files.

> > +		bd_dsc->skb = __dev_alloc_skb(req->frag_size, GFP_KERNEL);
> > +		if (!bd_dsc->skb) {
> > +			dev_warn((mdev)->dev, "Failed to alloc SKB\n");
>
> You might want to rate limit this, and the other similar messages in
> the data path, otherwise it could be a DOS.

Will switch to dev_warn_ratelimited() for data path warnings.

> > +	} while (++cnt < 10);
>
> Please use one of the helpers from iopoll.h.

Will replace with read_poll_timeout() in v4.

> > +			devm_kfree(trans->mdev->dev, srv_que);
>
> It is unusual to see devm_kfree(). Why is it needed?

Will remove redundant devm_kfree() calls and let devres handle
cleanup on probe failure.

> > +static unsigned int ctrl_port_chl_mtu;
>
> Is this a global variable? Why is it not part of priv?

> > +module_param(ctrl_port_chl_mtu, uint, 0644);
>
> No modules parameters please. If this is an MTU, why not use the
> normal networking interfaces to set the MTU?

Will remove the module parameter entirely. The control port MTU
is determined by the device during handshake.

Thanks.

Jack Wu