Message-ID: <aFKK+SiUG1jMXr10@mev-dev.igk.intel.com>
Date: Wed, 18 Jun 2025 11:46:33 +0200
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Mengyuan Lou <mengyuanlou@...-swift.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
	horms@...nel.org, andrew+netdev@...n.ch, duanqiangwen@...-swift.com,
	linglingzhang@...stnetic.com, jiawenwu@...-swift.com
Subject: Re: [PATCH net-next 01/12] net: libwx: add mailbox api for wangxun
 vf drivers

On Wed, Jun 11, 2025 at 04:35:48PM +0800, Mengyuan Lou wrote:
> Implements the mailbox interfaces for Wangxun vf drivers which
> will be used in txgbevf and ngbevf.
> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@...-swift.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_mbx.c  | 256 +++++++++++++++++++
>  drivers/net/ethernet/wangxun/libwx/wx_mbx.h  |  22 ++
>  drivers/net/ethernet/wangxun/libwx/wx_type.h |   3 +
>  3 files changed, 281 insertions(+)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_mbx.c b/drivers/net/ethernet/wangxun/libwx/wx_mbx.c
> index 73af5f11c3bd..ebfa07d50bd2 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_mbx.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_mbx.c
> @@ -174,3 +174,259 @@ int wx_check_for_rst_pf(struct wx *wx, u16 vf)
>  
>  	return 0;
>  }
> +
> +static u32 wx_read_v2p_mailbox(struct wx *wx)
> +{
> +	u32 mailbox = rd32(wx, WX_VXMAILBOX);
> +
> +	mailbox |= wx->mbx.mailbox;
> +	wx->mbx.mailbox |= mailbox & WX_VXMAILBOX_R2C_BITS;
> +
> +	return mailbox;
> +}
> +
> +/**
> + *  wx_obtain_mbx_lock_vf - obtain mailbox lock
> + *  @wx: pointer to the HW structure
> + *
> + *  Return: return 0 on success and -EBUSY on failure
> + **/
> +static int wx_obtain_mbx_lock_vf(struct wx *wx)
> +{
> +	int count = 5;
> +	u32 mailbox;
> +
> +	while (count--) {
> +		/* Take ownership of the buffer */
> +		wr32(wx, WX_VXMAILBOX, WX_VXMAILBOX_VFU);
> +
> +		/* reserve mailbox for vf use */
> +		mailbox = wx_read_v2p_mailbox(wx);
> +		if (mailbox & WX_VXMAILBOX_VFU)
> +			return 0;
> +	}

You can try to use read_poll_timeout(). In other poll also.

> +
> +	wx_err(wx, "Failed to obtain mailbox lock for VF.\n");
> +
> +	return -EBUSY;
> +}
> +
> +static int wx_check_for_bit_vf(struct wx *wx, u32 mask)
> +{
> +	u32 mailbox = wx_read_v2p_mailbox(wx);
> +
> +	wx->mbx.mailbox &= ~mask;
> +
> +	return (mailbox & mask ? 0 : -EBUSY);
> +}
> +
> +/**
> + *  wx_check_for_ack_vf - checks to see if the PF has ACK'd
> + *  @wx: pointer to the HW structure
> + *
> + *  Return: return 0 if the PF has set the status bit or else -EBUSY
> + **/
> +static int wx_check_for_ack_vf(struct wx *wx)
> +{
> +	/* read clear the pf ack bit */
> +	return wx_check_for_bit_vf(wx, WX_VXMAILBOX_PFACK);
> +}
> +
> +/**
> + *  wx_check_for_msg_vf - checks to see if the PF has sent mail
> + *  @wx: pointer to the HW structure
> + *
> + *  Return: return 0 if the PF has got req bit or else -EBUSY
> + **/
> +int wx_check_for_msg_vf(struct wx *wx)
> +{
> +	/* read clear the pf sts bit */
> +	return wx_check_for_bit_vf(wx, WX_VXMAILBOX_PFSTS);
> +}
> +
> +/**
> + *  wx_check_for_rst_vf - checks to see if the PF has reset
> + *  @wx: pointer to the HW structure
> + *
> + *  Return: return 0 if the PF has set the reset done and -EBUSY on failure
> + **/
> +int wx_check_for_rst_vf(struct wx *wx)
> +{
> +	/* read clear the pf reset done bit */
> +	return wx_check_for_bit_vf(wx,
> +				   WX_VXMAILBOX_RSTD |
> +				   WX_VXMAILBOX_RSTI);
> +}
> +
> +/**
> + *  wx_poll_for_msg - Wait for message notification
> + *  @wx: pointer to the HW structure
> + *
> + *  Return: return 0 if the VF has successfully received a message notification
> + **/
> +static int wx_poll_for_msg(struct wx *wx)
> +{
> +	struct wx_mbx_info *mbx = &wx->mbx;
> +	int countdown = mbx->timeout;
> +
> +	while (countdown && wx_check_for_msg_vf(wx)) {
> +		countdown--;
> +		if (!countdown)
> +			break;
> +		udelay(mbx->udelay);
> +	}

Here

> +
> +	return countdown ? 0 : -EBUSY;
> +}
> +
> +/**
> + *  wx_poll_for_ack - Wait for message acknowledgment
> + *  @wx: pointer to the HW structure
> + *
> + *  Return: return 0 if the VF has successfully received a message ack
> + **/
> +static int wx_poll_for_ack(struct wx *wx)
> +{
> +	struct wx_mbx_info *mbx = &wx->mbx;
> +	int countdown = mbx->timeout;
> +
> +	while (countdown && wx_check_for_ack_vf(wx)) {
> +		countdown--;
> +		if (!countdown)
> +			break;
> +		udelay(mbx->udelay);
> +	}

And here

> +
> +	return countdown ? 0 : -EBUSY;
> +}
> +
> +/**
> + *  wx_read_posted_mbx - Wait for message notification and receive message
> + *  @wx: pointer to the HW structure
> + *  @msg: The message buffer
> + *  @size: Length of buffer
> + *
> + *  Return: returns 0 if it successfully received a message notification and
> + *  copied it into the receive buffer.
> + **/
> +int wx_read_posted_mbx(struct wx *wx, u32 *msg, u16 size)
> +{
> +	int ret;
> +
> +	ret = wx_poll_for_msg(wx);
> +	/* if ack received read message, otherwise we timed out */
> +	if (!ret)
> +		ret = wx_read_mbx_vf(wx, msg, size);
> +
> +	return ret;

Nit, but usuall error path is in if statement. Sth like:

if (ret)
	return ret;

return wx_read_mbx_vf();

can be more readable for someone.

> +}
> +
> +/**
> + *  wx_write_posted_mbx - Write a message to the mailbox, wait for ack
> + *  @wx: pointer to the HW structure
> + *  @msg: The message buffer
> + *  @size: Length of buffer
> + *
> + *  Return: returns 0 if it successfully copied message into the buffer and
> + *  received an ack to that message within delay * timeout period
> + **/
> +int wx_write_posted_mbx(struct wx *wx, u32 *msg, u16 size)
> +{
> +	int ret;
> +
> +	/* send msg */
> +	ret = wx_write_mbx_vf(wx, msg, size);
> +	/* if msg sent wait until we receive an ack */
> +	if (!ret)
> +		ret = wx_poll_for_ack(wx);
> +
> +	return ret;
> +}
> +
> +/**
> + *  wx_write_mbx_vf - Write a message to the mailbox
> + *  @wx: pointer to the HW structure
> + *  @msg: The message buffer
> + *  @size: Length of buffer
> + *
> + *  Return: returns 0 if it successfully copied message into the buffer
> + **/
> +int wx_write_mbx_vf(struct wx *wx, u32 *msg, u16 size)
> +{
> +	struct wx_mbx_info *mbx = &wx->mbx;
> +	int ret, i;
> +
> +	/* 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_vf(wx);
> +	if (ret)
> +		return ret;
> +
> +	/* flush msg and acks as we are overwriting the message buffer */
> +	wx_check_for_msg_vf(wx);
> +	wx_check_for_ack_vf(wx);

Isn't checking returned value needed here?

> +
> +	/* copy the caller specified message to the mailbox memory buffer */
> +	for (i = 0; i < size; i++)
> +		wr32a(wx, WX_VXMBMEM, i, msg[i]);
> +
> +	/* Drop VFU and interrupt the PF to tell it a message has been sent */
> +	wr32(wx, WX_VXMAILBOX, WX_VXMAILBOX_REQ);

It isn't clear that it drops lock, maybe do it in a function like
wx_drop_mbx_lock_vf()? (just preference)

> +
> +	return 0;
> +}
> +
> +/**
> + *  wx_read_mbx_vf - Reads a message from the inbox intended for vf
> + *  @wx: pointer to the HW structure
> + *  @msg: The message buffer
> + *  @size: Length of buffer
> + *
> + *  Return: returns 0 if it successfully copied message into the buffer
> + **/
> +int wx_read_mbx_vf(struct wx *wx, u32 *msg, u16 size)
> +{
> +	struct wx_mbx_info *mbx = &wx->mbx;
> +	int ret;
> +	u16 i;

int ret, i; like in previous function

> +
> +	/* 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_vf(wx);
> +	if (ret)
> +		return ret;
> +
> +	/* copy the message from the mailbox memory buffer */
> +	for (i = 0; i < size; i++)
> +		msg[i] = rd32a(wx, WX_VXMBMEM, i);
> +
> +	/* Acknowledge receipt and release mailbox, then we're done */
> +	wr32(wx, WX_VXMAILBOX, WX_VXMAILBOX_ACK);

Oh, so any value written into WX_VXMAILVOX drop the lock. Ignore my
comment about function for that.

> +
> +	return 0;
> +}
> +
> +int wx_init_mbx_params_vf(struct wx *wx)
> +{
> +	wx->vfinfo = kcalloc(1, sizeof(struct vf_data_storage),
> +			     GFP_KERNEL);

Why kcalloc() for 1 element?

> +	if (!wx->vfinfo)
> +		return -ENOMEM;
> +
> +	/* Initialize mailbox parameters */
> +	wx->mbx.size = WX_VXMAILBOX_SIZE;
> +	wx->mbx.mailbox = WX_VXMAILBOX;
> +	wx->mbx.udelay = 10;
> +	wx->mbx.timeout = 1000;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(wx_init_mbx_params_vf);
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_mbx.h b/drivers/net/ethernet/wangxun/libwx/wx_mbx.h
> index 05aae138dbc3..82df9218490a 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_mbx.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_mbx.h
> @@ -11,6 +11,20 @@
>  #define WX_PXMAILBOX_ACK     BIT(1) /* Ack message recv'd from VF */
>  #define WX_PXMAILBOX_PFU     BIT(3) /* PF owns the mailbox buffer */
>  
> +/* VF Registers */
> +#define WX_VXMAILBOX         0x600
> +#define WX_VXMAILBOX_REQ     BIT(0) /* Request for PF Ready bit */
> +#define WX_VXMAILBOX_ACK     BIT(1) /* Ack PF message received */
> +#define WX_VXMAILBOX_VFU     BIT(2) /* VF owns the mailbox buffer */
> +#define WX_VXMAILBOX_PFU     BIT(3) /* PF owns the mailbox buffer */
> +#define WX_VXMAILBOX_PFSTS   BIT(4) /* PF wrote a message in the MB */
> +#define WX_VXMAILBOX_PFACK   BIT(5) /* PF ack the previous VF msg */
> +#define WX_VXMAILBOX_RSTI    BIT(6) /* PF has reset indication */
> +#define WX_VXMAILBOX_RSTD    BIT(7) /* PF has indicated reset done */
> +#define WX_VXMAILBOX_R2C_BITS (WX_VXMAILBOX_RSTD | \
> +	    WX_VXMAILBOX_PFSTS | WX_VXMAILBOX_PFACK)
> +
> +#define WX_VXMBMEM           0x00C00 /* 16*4B */
>  #define WX_PXMBMEM(i)        (0x5000 + (64 * (i))) /* i=[0,63] */
>  
>  #define WX_VFLRE(i)          (0x4A0 + (4 * (i))) /* i=[0,1] */
> @@ -74,4 +88,12 @@ int wx_check_for_rst_pf(struct wx *wx, u16 mbx_id);
>  int wx_check_for_msg_pf(struct wx *wx, u16 mbx_id);
>  int wx_check_for_ack_pf(struct wx *wx, u16 mbx_id);
>  
> +int wx_read_posted_mbx(struct wx *wx, u32 *msg, u16 size);
> +int wx_write_posted_mbx(struct wx *wx, u32 *msg, u16 size);
> +int wx_check_for_rst_vf(struct wx *wx);
> +int wx_check_for_msg_vf(struct wx *wx);
> +int wx_read_mbx_vf(struct wx *wx, u32 *msg, u16 size);
> +int wx_write_mbx_vf(struct wx *wx, u32 *msg, u16 size);
> +int wx_init_mbx_params_vf(struct wx *wx);
> +
>  #endif /* _WX_MBX_H_ */
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 7730c9fc3e02..f2061c893358 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -825,6 +825,9 @@ struct wx_bus_info {
>  
>  struct wx_mbx_info {
>  	u16 size;
> +	u32 mailbox;
> +	u32 udelay;
> +	u32 timeout;
>  };
>  
>  struct wx_thermal_sensor_data {
> -- 
> 2.30.1

Powered by blists - more mailing lists