Re: [U-Boot] [PATCH v2] driver: net: ldpaa: Update priv->phydev after free()

On Tue, Feb 21, 2017 at 1:47 AM, Ashish Kumar ashish.kumar@nxp.com wrote:

Hello Joe,

Please see inline.

Regards Ashish

-----Original Message----- From: Joe Hershberger [mailto:joe.hershberger@gmail.com] Sent: Thursday, February 16, 2017 5:22 AM To: Ashish Kumar ashish.kumar@nxp.com Cc: u-boot u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v2] driver: net: ldpaa: Update priv->phydev after free()

On Wed, Feb 15, 2017 at 9:26 AM, Ashish Kumar Ashish.Kumar@nxp.com wrote:

...

From: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com

Even after memory free of phydev, priv is still pointing to the obsolete address. So update priv->phydev as NULL after memory free.

Signed-off-by: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Signed-off-by: Ashish Kumar Ashish.Kumar@nxp.com

Please always Cc me on network changes.

...

---

v2: Add signoff

drivers/net/ldpaa_eth/ldpaa_eth.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ldpaa_eth/ldpaa_eth.c b/drivers/net/ldpaa_eth/ldpaa_eth.c index 4e61700..f235b62 100644 --- a/drivers/net/ldpaa_eth/ldpaa_eth.c +++ b/drivers/net/ldpaa_eth/ldpaa_eth.c @@ -587,8 +587,10 @@ static void ldpaa_eth_stop(struct eth_device *net_dev) #ifdef CONFIG_PHYLIB if (priv->phydev && bus != NULL) phy_shutdown(priv->phydev);

•   else
  

•   else {
            free(priv->phydev);
  

•           priv->phydev = NULL;
  

•   }
  

This is strange. Why not just drop the free? It seems bad to delete the phydev just because the mdio interface is not there, especially in stop(). [Ashish Kumar] To keep the flow consistent of XFI(which is phy-less) to other phy based interface, in ldpaa_eth_open there are some dummy allocation, which is freed in the end in stop().

It just seems strange to spread the logic out like that. Why not just check for this condition and avoid the initial allocation?

    if ((bus == NULL) &&
        (enet_if == PHY_INTERFACE_MODE_XGMII)) {
            printf("%s %s %d\n",__FILE__, __func__, __LINE__);
            priv->phydev = (struct phy_device *)
                            malloc(sizeof(struct phy_device));
            memset(priv->phydev, 0, sizeof(struct phy_device));

            priv->phydev->speed = SPEED_10000;
            priv->phydev->link = 1;
            priv->phydev->duplex = DUPLEX_FULL;
    }

...

#endif

    ldpaa_dpbp_free();

-- 1.9.1

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot