netdev - Re: [PATCH v3] Marvell phy: add fiber status check and configuration for some phys

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] [day] [month] [year] [list]

Message-ID: <20160713153953.GC6667@lunn.ch>
Date:	Wed, 13 Jul 2016 17:39:53 +0200
From:	Andrew Lunn <andrew@...n.ch>
To:	Charles-Antoine Couret <charles-antoine.couret@...vision.fr>
Cc:	netdev <netdev@...r.kernel.org>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH v3] Marvell phy: add fiber status check and configuration
 for some phys

> >>>> +static int marvell_resume_fiber(struct phy_device *phydev)
> >>>> +{
> >>>> +	int err;
> >>>> +
> >>>> +	/* Resume the fiber mode first */
> >>>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> >>>> +	if (err < 0)
> >>>> +		goto error;
> >>>> +
> >>>> +	err = genphy_resume(phydev);
> >>>> +	if (err < 0)
> >>>> +		goto error;
> >>>> +
> >>>> +	/* Then, the copper link */
> >>>> +	err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> >>>> +	if (err < 0)
> >>>> +		goto error;
> >>>> +
> >>>> +	return genphy_resume(phydev);
> >>>
> >>> Should it be resumed twice? Or just once at the end?  Same question
> >>> for suspend.
> >>
> >> I don't understand your question.
> > 
> > You call genphy_resume(phydev) twice. Once is sufficient.
> 
> Yes, but it's normal because each interface could be suspended or resumed independently.

> genphy_* functions use BMCR register which are identical between
> fiber and copper link. But each link has its own register to change.

Ah! Now i get it. I think you need a comment here. Something like:

    /* With the page set, use the generic resume */

What i was worried about is that there is some reference counting
going on inside these functions. And so suspending the same phydev
multiple times will mess up the reference counts. But no, it just
twiddles a register bit, so that is O.K.

	 Andrew