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:26:34 +0200
From:	Joachim Eastwood <manabian@...il.com>
To:	Marek Vasut <marex@...x.de>
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 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.


>>> +
>>> +       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.


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.



regards,
Joachim Eastwood