linux-kernel - Re: [PATCH net-next v2 1/5] amd-xgbe: reorganize the code of XPCS access

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: <c962b740-4cbc-4d1f-9dda-02820dc54daa@linux.dev>
Date: Wed, 30 Apr 2025 23:38:51 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Raju Rangoju <Raju.Rangoju@....com>, andrew+netdev@...n.ch,
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 Shyam-sundar.S-k@....com
Subject: Re: [PATCH net-next v2 1/5] amd-xgbe: reorganize the code of XPCS
 access

On 28/04/2025 16:02, Raju Rangoju wrote:
> The xgbe_{read/write}_mmd_regs_v* functions have common code which can
> be moved to helper functions. Add new helper functions to calculate the
> mmd_address for v1/v2 of xpcs access.
> 
> Signed-off-by: Raju Rangoju <Raju.Rangoju@....com>
> ---
> Changes since v1:
> - add the xgbe_ prefix to new functions
> 
>   drivers/net/ethernet/amd/xgbe/xgbe-dev.c | 63 ++++++++++--------------
>   1 file changed, 27 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> index b51a3666dddb..765f20b24722 100644
> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
> @@ -1041,18 +1041,17 @@ static int xgbe_set_gpio(struct xgbe_prv_data *pdata, unsigned int gpio)
>   	return 0;
>   }
>   
> -static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> -				 int mmd_reg)
> +static unsigned int xgbe_get_mmd_address(struct xgbe_prv_data *pdata, int mmd_reg)
>   {
> -	unsigned long flags;
> -	unsigned int mmd_address, index, offset;
> -	int mmd_data;
> -
> -	if (mmd_reg & XGBE_ADDR_C45)
> -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> -	else
> -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> +	return (mmd_reg & XGBE_ADDR_C45) ?
> +		mmd_reg & ~XGBE_ADDR_C45 :
> +		(pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> +}
>   
> +static void xgbe_get_pcs_index_and_offset(struct xgbe_prv_data *pdata,
> +					  unsigned int mmd_address,
> +					  unsigned int *index, unsigned int *offset)
> +{
>   	/* The PCS registers are accessed using mmio. The underlying
>   	 * management interface uses indirect addressing to access the MMD
>   	 * register sets. This requires accessing of the PCS register in two
> @@ -1063,8 +1062,20 @@ static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>   	 * offset 1 bit and reading 16 bits of data.
>   	 */
>   	mmd_address <<= 1;
> -	index = mmd_address & ~pdata->xpcs_window_mask;
> -	offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
> +	*index = mmd_address & ~pdata->xpcs_window_mask;
> +	*offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
> +}
> +
> +static int xgbe_read_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
> +				 int mmd_reg)
> +{
> +	unsigned long flags;
> +	unsigned int mmd_address, index, offset;
> +	int mmd_data;

Please, follow reverse Xmass tree ordering

> +
> +	mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
> +
> +	xgbe_get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
>   
>   	spin_lock_irqsave(&pdata->xpcs_lock, flags);
>   	XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> @@ -1080,23 +1091,9 @@ static void xgbe_write_mmd_regs_v2(struct xgbe_prv_data *pdata, int prtad,
>   	unsigned long flags;
>   	unsigned int mmd_address, index, offset;
>   
> -	if (mmd_reg & XGBE_ADDR_C45)
> -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> -	else
> -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> +	mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
>   
> -	/* The PCS registers are accessed using mmio. The underlying
> -	 * management interface uses indirect addressing to access the MMD
> -	 * register sets. This requires accessing of the PCS register in two
> -	 * phases, an address phase and a data phase.
> -	 *
> -	 * The mmio interface is based on 16-bit offsets and values. All
> -	 * register offsets must therefore be adjusted by left shifting the
> -	 * offset 1 bit and writing 16 bits of data.
> -	 */
> -	mmd_address <<= 1;
> -	index = mmd_address & ~pdata->xpcs_window_mask;
> -	offset = pdata->xpcs_window + (mmd_address & pdata->xpcs_window_mask);
> +	xgbe_get_pcs_index_and_offset(pdata, mmd_address, &index, &offset);
>   
>   	spin_lock_irqsave(&pdata->xpcs_lock, flags);
>   	XPCS32_IOWRITE(pdata, pdata->xpcs_window_sel_reg, index);
> @@ -1111,10 +1108,7 @@ static int xgbe_read_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad,
>   	unsigned int mmd_address;
>   	int mmd_data;
>   
> -	if (mmd_reg & XGBE_ADDR_C45)
> -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> -	else
> -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> +	mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
>   
>   	/* The PCS registers are accessed using mmio. The underlying APB3
>   	 * management interface uses indirect addressing to access the MMD
> @@ -1139,10 +1133,7 @@ static void xgbe_write_mmd_regs_v1(struct xgbe_prv_data *pdata, int prtad,
>   	unsigned int mmd_address;
>   	unsigned long flags;
>   
> -	if (mmd_reg & XGBE_ADDR_C45)
> -		mmd_address = mmd_reg & ~XGBE_ADDR_C45;
> -	else
> -		mmd_address = (pdata->mdio_mmd << 16) | (mmd_reg & 0xffff);
> +	mmd_address = xgbe_get_mmd_address(pdata, mmd_reg);
>   
>   	/* The PCS registers are accessed using mmio. The underlying APB3
>   	 * management interface uses indirect addressing to access the MMD