Re: [PATCH v4 04/14] net-next/yunsilicon: Add qp and cq management

[Simon Horman <horms@kernel.org>]

On Thu, Feb 13, 2025 at 05:14:11PM +0800, Xin Tian wrote:
> Add qp(queue pair) and cq(completion queue) resource management APIs
> 
> Co-developed-by: Honggang Wei <weihg@yunsilicon.com>
> Signed-off-by: Honggang Wei <weihg@yunsilicon.com>
> Co-developed-by: Lei Yan <jacky@yunsilicon.com>
> Signed-off-by: Lei Yan <jacky@yunsilicon.com>
> Signed-off-by: Xin Tian <tianx@yunsilicon.com>

Some general remark regarding this patchset:

1. "xsc" is probably a more appropriate prefix than "net-next/yunsilicon"
   in the patch subjects: it seems to be the name of the driver, and
   conveniently is nice and short.

2. Please provide more descriptive patch descriptions, ideally
   explaining why changes are being made. As this is a new driver
   I think it is appropriate for the "why" to to describe how
   the patches fill-out the driver, leading to something users
   can use.

...

> diff --git a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
> index 4c8b26660..4e19b0989 100644
> --- a/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
> +++ b/drivers/net/ethernet/yunsilicon/xsc/common/xsc_core.h
> @@ -29,6 +29,14 @@
>  
>  #define XSC_REG_ADDR(dev, offset)	\
>  	(((dev)->bar) + ((offset) - 0xA0000000))
> +#define XSC_SET_FIELD(value, field)	\
> +	(((value) << field ## _SHIFT) & field ## _MASK)
> +#define XSC_GET_FIELD(word, field)	\
> +	(((word)  & field ## _MASK) >> field ## _SHIFT)

I did not try, but I expect that if you express XSC_SET_FIELD() and
XSC_GET_FIELD() in terms of FIELD_PREP() and FIELD_GET() then the _SHIFT
part disappears. And, ideally, the corresponding _SHIFT defines don't need
to be defined.

> +
> +enum {
> +	XSC_MAX_EQ_NAME	= 20
> +};
>  
>  enum {
>  	XSC_MAX_PORTS	= 2,
> @@ -44,6 +52,147 @@ enum {
>  	XSC_MAX_UUARS		= XSC_MAX_UAR_PAGES * XSC_BF_REGS_PER_PAGE,
>  };
>  
> +// alloc
> +struct xsc_buf_list {
> +	void		       *buf;
> +	dma_addr_t		map;
> +};
> +
> +struct xsc_buf {
> +	struct xsc_buf_list	direct;
> +	struct xsc_buf_list	*page_list;
> +	int			nbufs;
> +	int			npages;
> +	int			page_shift;
> +	int			size;

Looking over the way the fields are used in this patchset
I think that unsigned long would be slightly better types
for nbufs, npages, and size.

And more generally, I think it would be nice to use unsigned
types throughout this patchset for, in structure members, function
parameters, and local variables, to hold unsigned values.

And likewise to use unsigned long (instead of unsigned int) as
appropriate, e.g. the size parameter of xsc_buf_alloc() which
is passed to get_order() in a subsequent patch in this series.

> +};
> +
> +struct xsc_frag_buf {
> +	struct xsc_buf_list	*frags;
> +	int			npages;
> +	int			size;
> +	u8			page_shift;
> +};
> +
> +struct xsc_frag_buf_ctrl {
> +	struct xsc_buf_list	*frags;
> +	u32			sz_m1;
> +	u16			frag_sz_m1;
> +	u16			strides_offset;
> +	u8			log_sz;
> +	u8			log_stride;
> +	u8			log_frag_strides;
> +};

...