netdev - Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 26 Apr 2016 14:47:40 +0200
From:	Marek Vasut <marex@...x.de>
To:	Joachim Eastwood <manabian@...il.com>
CC:	netdev <netdev@...r.kernel.org>,
	"peppe.cavallaro" <peppe.cavallaro@...com>,
	alexandre.torgue@...com,
	Matthew Gerlach <mgerlach@...nsource.altera.com>,
	Dinh Nguyen <dinguyen@...nsource.altera.com>,
	"David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH V3] net: stmmac: socfpga: Remove re-registration of reset
 controller

On 04/26/2016 02:26 PM, Joachim Eastwood wrote:
> On 26 April 2016 at 00:55, Marek Vasut <marex@...x.de> wrote:
>> On 04/25/2016 08:11 PM, Joachim Eastwood wrote:
>>> On 21 April 2016 at 14:11, Marek Vasut <marex@...x.de> wrote:
>>>>
>>>> Finally, plat_dat->exit and socfpga_dwmac_exit() is no longer necessary,
>>>> since the functionality is already performed by the stmmac core.
>>>
>>> I am trying to rebase my changes on top of your two patches and
>>> noticed a couple of things.
>>>
>>>>  static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>  {
>>>> -       struct socfpga_dwmac    *dwmac = priv;
>>>> +       struct socfpga_dwmac *dwmac = priv;
>>>>         struct net_device *ndev = platform_get_drvdata(pdev);
>>>>         struct stmmac_priv *stpriv = NULL;
>>>>         int ret = 0;
>>>>
>>>> -       if (ndev)
>>>> -               stpriv = netdev_priv(ndev);
>>>> +       if (!ndev)
>>>> +               return -EINVAL;
>>>
>>> ndev can never be NULL here. socfpga_dwmac_init() is only called if
>>> stmmac_dvr_probe() succeeds or we are running the resume callback. So
>>> I don't see how this could ever be NULL.
>>
>> That's a good point, this check can indeed be removed. While you're at
>> the patching, can you remove this one ?
> 
> Yes, my patch will refactor the init() function so this will go away.

Thanks!

>>>> +
>>>> +       stpriv = netdev_priv(ndev);
>>>
>>> It's not really nice to access 'stmmac_priv' as it should be private
>>> to the core driver, but I don't see any other good solution right now.
>>
>> I guess some stmmac_reset_assert() wrapper would be nicer, yes. What do
>> you think ?
>>
>>>> +       if (!stpriv)
>>>> +               return -EINVAL;
>>>>
>>>>         /* Assert reset to the enet controller before changing the phy mode */
>>>> -       if (dwmac->stmmac_rst)
>>>> -               reset_control_assert(dwmac->stmmac_rst);
>>>> +       if (stpriv->stmmac_rst)
>>>> +               reset_control_assert(stpriv->stmmac_rst);
>>>>
>>>>         /* Setup the phy mode in the system manager registers according to
>>>>          * devicetree configuration
>>>> @@ -227,8 +210,8 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>         /* Deassert reset for the phy configuration to be sampled by
>>>>          * the enet controller, and operation to start in requested mode
>>>>          */
>>>> -       if (dwmac->stmmac_rst)
>>>> -               reset_control_deassert(dwmac->stmmac_rst);
>>>> +       if (stpriv->stmmac_rst)
>>>> +               reset_control_deassert(stpriv->stmmac_rst);
>>>>
>>>>         /* Before the enet controller is suspended, the phy is suspended.
>>>>          * This causes the phy clock to be gated. The enet controller is
>>>> @@ -245,7 +228,7 @@ static int socfpga_dwmac_init(struct platform_device *pdev, void *priv)
>>>>          * control register 0, and can be modified by the phy driver
>>>>          * framework.
>>>>          */
>>>> -       if (stpriv && stpriv->phydev)
>>>> +       if (stpriv->phydev)
>>>>                 phy_resume(stpriv->phydev);
>>>
>>> Before this change phy_resume() was only called during driver resume
>>> when , but your patches cause phy_resume() to called at probe time as
>>> well. Is this okey?
>>
>> I _hope_ it's OK. The cryptic comment above is not very helpful in this
>> aspect. Dinh ? :)
> 
> My patches will move phy_resume() to the resume callback so it
> preserves the previous behavior. But if someone knows more about this
> that would be useful.
> 
> 
>> btw I wish you reviewed my patch a bit earlier to catch these bits.
> 
> Sorry, about that. I have been really busy with other things lately.

Oh I'm real happy someone is doing the refactoring :) I appreciate your
work, sorry if that was unclear.

> My patches based on next from Friday can be found here now:
> https://github.com/manabian/linux-lpc/tree/net-socfpga-dwmac-on-next
> 
> I had to add your latest patch as well since the next version I used
> didn't have it. I'll post the patches on netdev later today or
> tomorrow.

Looks like next wasn't synced for a few days, yeah.

You can add my:

On SoCFPGA Cyclone V SoC (DENX MCVEVK):
Tested-by: Marek Vasut <marex@...x.de>

to those patches

> regards,
> Joachim Eastwood
> 


-- 
Best regards,
Marek Vasut