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

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

On 2025/2/19 0:31, Simon Horman wrote:

On Thu, Feb 13, 2025 at 05:14:11PM +0800, Xin Tian wrote:

quoted

Add qp(queue pair) and cq(completion queue) resource management APIs

Co-developed-by: Honggang Wei <redacted>
Signed-off-by: Honggang Wei <redacted>
Co-developed-by: Lei Yan <redacted>
Signed-off-by: Lei Yan <redacted>
Signed-off-by: Xin Tian <redacted>

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.

...

OK, I will change subjects and add more descriptions in next version

quoted

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.

You're right, with FIELD_PREP and FIELD_GET, there's no need to add 
XSC_SET_FIELD and XSC_GET_FIELD anymore. Thanks a lot!

quoted

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

Sure, I will change these.

quoted

+};
+
+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;
+};

...