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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <13847c17-9c71-42ef-a1dd-31ef29caabf5@yunsilicon.com>
Date: Thu, 20 Feb 2025 16:58:21 +0800
From: "tianx" <tianx@...silicon.com>
To: "Simon Horman" <horms@...nel.org>
Cc: <netdev@...r.kernel.org>, <leon@...nel.org>, <andrew+netdev@...n.ch>, 
	<kuba@...nel.org>, <pabeni@...hat.com>, <edumazet@...gle.com>, 
	<davem@...emloft.net>, <jeff.johnson@....qualcomm.com>, 
	<przemyslaw.kitszel@...el.com>, <weihg@...silicon.com>, 
	<wanry@...silicon.com>, <parthiban.veerasooran@...rochip.com>, 
	<masahiroy@...nel.org>
Subject: 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:
>> Add qp(queue pair) and cq(completion queue) resource management APIs
>>
>> Co-developed-by: Honggang Wei <weihg@...silicon.com>
>> Signed-off-by: Honggang Wei <weihg@...silicon.com>
>> Co-developed-by: Lei Yan <jacky@...silicon.com>
>> Signed-off-by: Lei Yan <jacky@...silicon.com>
>> Signed-off-by: Xin Tian <tianx@...silicon.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.
>
> ...
OK, I will change subjects and add more descriptions in next version
>> 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!
>> +
>> +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.
>> +};
>> +
>> +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;
>> +};
> ...