Received-SPF: pass (google.com: domain of linux-kernel+bounces-172827-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 8 May 2024 08:39:52 +0100
From: Simon Horman <horms@kernel.org>
To: Kamil =?utf-8?B?SG9yw6Fr?= - 2N <kamilh@axis.com>
Cc: florian.fainelli@broadcom.com, bcm-kernel-feedback-list@broadcom.com,
	andrew@lunn.ch, hkallweit1@gmail.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach
 link modes
Message-ID: <20240508073952.GL15955@kernel.org>
References: <20240506144015.2409715-1-kamilh@axis.com>
 <20240506144015.2409715-4-kamilh@axis.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240506144015.2409715-4-kamilh@axis.com>

On Mon, May 06, 2024 at 04:40:15PM +0200, Kamil Horák - 2N wrote:
> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> Create set of functions alternative to IEEE 802.3 to handle configuration
> of these modes on compatible Broadcom PHYs.
> 
> Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>

Hi Kamil,

Some minor feedback from my side.

..

> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c

..

> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register.

Please consider including a Return: section in the Kernel doc.

Flagged by ./scripts/kernel-doc -Wall -none

> + */
> +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)

..

> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c

..

> +static int bcm_read_master_slave(struct phy_device *phydev)
> +{
> +	int cfg, state;
> +	int val;
> +
> +	/* In BroadR-Reach mode we are always capable of master-slave
> +	 *  and there is no preferred master or slave configuration
> +	 */
> +	phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> +	phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +
> +	val = phy_read(phydev, MII_BCM54XX_LRECR);
> +	if (val < 0)
> +		return val;
> +
> +	if ((val & LRECR_LDSEN) == 0) {
> +		if (val & LRECR_MASTER)
> +			cfg = MASTER_SLAVE_CFG_MASTER_FORCE;
> +		else
> +			cfg = MASTER_SLAVE_CFG_SLAVE_FORCE;
> +	}
> +
> +	val = phy_read(phydev, MII_BCM54XX_LRELDSE);
> +	if (val < 0)
> +		return val;
> +
> +	if (val & LDSE_MASTER)
> +		state = MASTER_SLAVE_STATE_MASTER;
> +	else
> +		state = MASTER_SLAVE_STATE_SLAVE;
> +
> +	phydev->master_slave_get = cfg;

Perhaps it is not possible, but it appears that if the
((val & LRECR_LDSEN) == 0) condition above is not met then
cfg is used uninitialised here.

Flagged by Smatch.

> +	phydev->master_slave_state = state;
> +
> +	return 0;
> +}

..