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

[Joe Hershberger]

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?

>         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