Re: [PATCH net-next v1] eea: Add basic driver framework for Alibaba Elastic Ethernet Adaptor

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Thu, 24 Jul 2025 17:44:18 +0100, Simon Horman <horms@kernel.org> wrote:
> On Thu, Jul 24, 2025 at 07:06:45PM +0800, Xuan Zhuo wrote:
> > Add a driver framework for EEA that will be available in the future.
>
> This is a 40k LoC patch.
> It would be nice to add a bit more information about
> what it includes.
>
> >
> > Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
> > Reviewed-by: Philo Lu <lulie@linux.alibaba.com>
> > Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
>
> ...
>
> >  19 files changed, 4067 insertions(+)
>
> ...
>
> > diff --git a/drivers/net/ethernet/alibaba/eea/eea_adminq.c b/drivers/net/ethernet/alibaba/eea/eea_adminq.c
>
> > +int eea_adminq_config_host_info(struct eea_net *enet)
> > +{
> > +	struct device *dev = enet->edev->dma_dev;
> > +	struct eea_aq_host_info_cfg *cfg;
> > +	struct eea_aq_host_info_rep *rep;
> > +	int rc = -ENOMEM;
> > +
> > +	cfg = kzalloc(sizeof(*cfg), GFP_KERNEL);
> > +	if (!cfg)
> > +		return rc;
> > +
> > +	rep = kzalloc(sizeof(*rep), GFP_KERNEL);
> > +	if (!rep)
> > +		goto free_cfg;
> > +
> > +	cfg->os_type = EEA_OS_LINUX;
>
> The type of the lvalue above is a little endian integer.
> While the type of the rvalue is a host byte order integer.
> I would address this as follows:
>
> 	cfg->os_type = cpu_to_le16(EEA_OS_LINUX);
>
> Likewise for other members of cfg set in this function,
> noting that pci_domain is a 32bit entity.
>
> Flagged by Sparse.
>
> There are a number of other (minor) problems flagged by Sparse
> in this patch. Please address them and make sure patches
> are Sparse-clean.

I did the check by make C=1, but I found that my sparse is old ^_^.
I updated the sparse, and cleared all the reports.

...

>
> > diff --git a/drivers/net/ethernet/alibaba/eea/eea_net.c b/drivers/net/ethernet/alibaba/eea/eea_net.c
>
> ...
>
> > +void *ering_cq_get_desc(const struct ering *ering)
> > +{
> > +	u8 phase;
> > +	u8 *desc;
> > +
> > +	desc = ering->cq.desc + (ering->cq.head << ering->cq.desc_size_shift);
> > +
> > +	phase = *(u8 *)(desc + ering->cq.desc_size - 1);
> > +
> > +	if ((phase & ERING_DESC_F_CQ_PHASE)  == ering->cq.phase) {
>
> nit: unnecessary inner-parentheses

I think we should keep it.

Thanks.