On Tue, Feb 21, 2017 at 1:47 AM, Ashish Kumar <ashish.ku...@nxp.com> wrote:
> Hello Joe,
>
> Please see inline.
>
> Regards
> Ashish
>
> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershber...@gmail.com]
> Sent: Thursday, February 16, 2017 5:22 AM
> To: Ashish Kumar <ashish.ku...@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.ku...@nxp.com> wrote:
>> From: Prabhakar Kushwaha <prabhakar.kushw...@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.kushw...@nxp.com>
>> Signed-off-by: Ashish Kumar <ashish.ku...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
