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

[Ashish Kumar]

Hello Joe,

Sorry for late response, please see inline.

Regards
Ashish

-----Original Message-----
From: Joe Hershberger [mailto:joe.hershberger at gmail.com] 
Sent: Tuesday, March 07, 2017 3:39 AM
To: Ashish Kumar <ashish.kumar at nxp.com>
Cc: u-boot <u-boot at lists.denx.de>; Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
Subject: 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 at nxp.com> wrote:
> Hello Joe,
>
> Please see inline.
>
> Regards
> Ashish
>
> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> Sent: Thursday, February 16, 2017 5:22 AM
> To: Ashish Kumar <ashish.kumar at nxp.com>
> Cc: u-boot <u-boot at 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 at nxp.com> wrote:
>> From: Prabhakar Kushwaha <prabhakar.kushwaha at 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 at nxp.com>
>> Signed-off-by: Ashish Kumar <Ashish.Kumar at 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?
[Ashish Kumar] In that case, I have to add flag all over the code as to first check if it is XFI, then skip the usage of structure (phydev)'s member.

>         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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot