Message-ID: <75510d23-2ac9-4d83-b254-96712c6a8154@intel.com>
Date: Tue, 6 Aug 2024 15:20:20 +0200
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Mengyuan Lou <mengyuanlou@...-swift.com>
CC: <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v5 01/10] net: libwx: Add malibox api for wangxun
 pf drivers

On 8/4/24 14:48, Mengyuan Lou wrote:
> Implements the mailbox interfaces for wangxun pf drivers
> ngbe and txgbe.
> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>

it would be great to have a cover letter over the series

> ---
>   drivers/net/ethernet/wangxun/libwx/Makefile  |   2 +-
>   drivers/net/ethernet/wangxun/libwx/wx_mbx.c  | 175 +++++++++++++++++++
>   drivers/net/ethernet/wangxun/libwx/wx_mbx.h  |  32 ++++
>   drivers/net/ethernet/wangxun/libwx/wx_type.h |   8 +
>   4 files changed, 216 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/net/ethernet/wangxun/libwx/wx_mbx.c
>   create mode 100644 drivers/net/ethernet/wangxun/libwx/wx_mbx.h
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/Makefile b/drivers/net/ethernet/wangxun/libwx/Makefile
> index 42ccd6e4052e..913a978c9032 100644
> --- a/drivers/net/ethernet/wangxun/libwx/Makefile
> +++ b/drivers/net/ethernet/wangxun/libwx/Makefile
> @@ -4,4 +4,4 @@
>   
>   obj-$(CONFIG_LIBWX) += libwx.o
>   
> -libwx-objs := wx_hw.o wx_lib.o wx_ethtool.o
> +libwx-objs := wx_hw.o wx_lib.o wx_ethtool.o wx_mbx.o
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_mbx.c b/drivers/net/ethernet/wangxun/libwx/wx_mbx.c
> new file mode 100644
> index 000000000000..5062ddb2ce39
> --- /dev/null
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_mbx.c
> @@ -0,0 +1,175 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2015 - 2024 Beijing WangXun Technology Co., Ltd. */
> +#include <linux/pci.h>

typically you put a newline between different subsystems and another one
prior to your own headers (this case)

> +#include "wx_type.h"
> +#include "wx_mbx.h"
and it's good to start with include list sorted

> +
> +/**
> + *  wx_obtain_mbx_lock_pf - obtain mailbox lock
> + *  @wx: pointer to the HW structure
> + *  @vf: the VF index
> + *
> + *  return: return 0 on success and -EBUSY on failure

capitalize the first return word

> + **/
> +static int wx_obtain_mbx_lock_pf(struct wx *wx, u16 vf)
> +{
> +	int count = 5;
> +	u32 mailbox;
> +
> +	while (count--) {
> +		/* Take ownership of the buffer */
> +		wr32(wx, WX_PXMAILBOX(vf), WX_PXMAILBOX_PFU);
> +
> +		/* reserve mailbox for vf use */
> +		mailbox = rd32(wx, WX_PXMAILBOX(vf));
> +		if (mailbox & WX_PXMAILBOX_PFU)
> +			return 0;
> +		else if (count != 1)

you should exclude count == 0, not == 1

> +			udelay(10);
> +	}
> +	wx_err(wx, "Failed to obtain mailbox lock for PF%d", vf);

would be good to ratelimit that

> +
> +	return -EBUSY;
> +}
> +
> +static int wx_check_for_bit_pf(struct wx *wx, u32 mask, int index)
> +{
> +	u32 mbvficr = rd32(wx, WX_MBVFICR(index));
> +
> +	if ((mbvficr & mask) == 0)

the preferred style is to simply write !cond instead of cond == 0

> +		return -EBUSY;
> +	wr32(wx, WX_MBVFICR(index), mask);
> +
> +	return 0;
> +}
> +
> +/**
> + *  wx_check_for_ack_pf - checks to see if the VF has acked

I would rephrase to: check if PF was acked by VF

> + *  @wx: pointer to the HW structure
> + *  @vf: the VF index
> + *
> + *  return: return 0 if the VF has set the status bit or else -EBUSY
> + **/
> +int wx_check_for_ack_pf(struct wx *wx, u16 vf)
> +{
> +	u32 index = vf / 16, vf_bit = vf % 16;
> +
> +	return wx_check_for_bit_pf(wx,
> +				   FIELD_PREP(WX_MBVFICR_VFACK_MASK,
> +					      BIT(vf_bit)),

double check: do you want to set just one bit, in the upper half
of the u32 word, that will be passed as @mask to wx_check_for_bit_pf()?

> +				   index);
> +}
> +
> +/**
> + *  wx_check_for_msg_pf - checks to see if the VF has sent mail
> + *  @wx: pointer to the HW structure
> + *  @vf: the VF index
> + *
> + *  return: return 0 if the VF has got req bit or else -EBUSY
> + **/
> +int wx_check_for_msg_pf(struct wx *wx, u16 vf)
> +{
> +	u32 index = vf / 16, vf_bit = vf % 16;
> +
> +	return wx_check_for_bit_pf(wx,
> +				   FIELD_PREP(WX_MBVFICR_VFREQ_MASK,
> +					      BIT(vf_bit)),
> +				   index);
> +}
> +
> +/**
> + *  wx_write_mbx_pf - Places a message in the mailbox
> + *  @wx: pointer to the HW structure
> + *  @msg: The message buffer
> + *  @size: Length of buffer
> + *  @vf: the VF index
> + *
> + *  return: return 0 on success and -EINVAL/-EBUSY on failure
> + **/
> +int wx_write_mbx_pf(struct wx *wx, u32 *msg, u16 size, u16 vf)

typically function parameters should not be limited to smaller sizes
than 4B

> +{
> +	struct wx_mbx_info *mbx = &wx->mbx;
> +	int ret, i;

best place to declare variable @i is inside the for statement

> +
> +	/* mbx->size is up to 15 */
> +	if (size > mbx->size) {
> +		wx_err(wx, "Invalid mailbox message size %d", size);
> +		return -EINVAL;
> +	}
> +
> +	/* lock the mailbox to prevent pf/vf race condition */
> +	ret = wx_obtain_mbx_lock_pf(wx, vf);
> +	if (ret)
> +		return ret;
> +
> +	/* flush msg and acks as we are overwriting the message buffer */

should the buffer be already empty? that's a bad smell

> +	wx_check_for_msg_pf(wx, vf);
> +	wx_check_for_ack_pf(wx, vf);
> +
> +	/* copy the caller specified message to the mailbox memory buffer */
> +	for (i = 0; i < size; i++)
> +		wr32a(wx, WX_PXMBMEM(vf), i, msg[i]);
> +
> +	/* Interrupt VF to tell it a message has been sent and release buffer*/

missing space before "*/", and your are releasing the lock, not
a buffer

> +	/* set mirrored mailbox flags */
> +	wr32a(wx, WX_PXMBMEM(vf), WX_VXMAILBOX_SIZE, WX_PXMAILBOX_STS);
> +	wr32(wx, WX_PXMAILBOX(vf), WX_PXMAILBOX_STS);
> +
> +	return 0;
> +}
> +
> +/**
> + *  wx_read_mbx_pf - Read a message from the mailbox
> + *  @wx: pointer to the HW structure
> + *  @msg: The message buffer
> + *  @size: Length of buffer
> + *  @vf: the VF index
> + *
> + *  return: return 0 on success and -EBUSY on failure
> + **/
> +int wx_read_mbx_pf(struct wx *wx, u32 *msg, u16 size, u16 vf)
> +{
> +	struct wx_mbx_info *mbx = &wx->mbx;
> +	int ret;
> +	u16 i;
> +
> +	/* limit read to size of mailbox and mbx->size is up to 15 */
> +	if (size > mbx->size)
> +		size = mbx->size;
> +
> +	/* lock the mailbox to prevent pf/vf race condition */
> +	ret = wx_obtain_mbx_lock_pf(wx, vf);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < size; i++)
> +		msg[i] = rd32a(wx, WX_PXMBMEM(vf), i);
> +
> +	/* Acknowledge the message and release buffer */
> +	/* set mirrored mailbox flags */
> +	wr32a(wx, WX_PXMBMEM(vf), WX_VXMAILBOX_SIZE, WX_PXMAILBOX_ACK);
> +	wr32(wx, WX_PXMAILBOX(vf), WX_PXMAILBOX_ACK);
> +
> +	return 0;
> +}
> +
> +/**
> + *  wx_check_for_rst_pf - checks to see if the VF has reset
> + *  @wx: pointer to the HW structure
> + *  @vf: the VF index
> + *
> + *  return: return 0 on success and -EBUSY on failure
> + **/
> +int wx_check_for_rst_pf(struct wx *wx, u16 vf)

I would spell "reset" fully

> +{
> +	u32 reg_offset = WX_VF_REG_OFFSET(vf);
> +	u32 vf_shift = WX_VF_IND_SHIFT(vf);
> +	u32 vflre = 0;
> +
> +	vflre = rd32(wx, WX_VFLRE(reg_offset));
> +	if (!(vflre & BIT(vf_shift)))
> +		return -EBUSY;
> +	wr32(wx, WX_VFLREC(reg_offset), BIT(vf_shift));
> +
> +	return 0;
> +}
please apply all my feedback in analogous places through whole series

Powered by blists - more mailing lists