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

Message-ID: <cbe62b8d-3796-a514-9783-c1ddae5711a6@nexvision.fr>
Date:	Wed, 13 Jul 2016 15:46:46 +0200
From:	Charles-Antoine Couret <charles-antoine.couret@...vision.fr>
To:	Andrew Lunn <andrew@...n.ch>
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

Hi Andrew,

Le 13/07/2016 à 15:26, Andrew Lunn a écrit :
>>>>   *
>>>>   * Generic status code does not detect Fiber correctly!
>>>> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
>>>>  	int lpa;
>>>>  	int lpagb;
>>>>  	int status = 0;
>>>> +	int page, fiber;
>>>>  
>>>> -	/* Update the link, but return if there
>>>> +	/* Detect and update the link, but return if there
>>>>  	 * was an error */
>>>> -	err = genphy_update_link(phydev);
>>>> -	if (err)
>>>> -		return err;
>>>> +	page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
>>>> +	if (page == MII_M1111_FIBER)
>>>> +		fiber = 1;
>>>> +	else
>>>> +		fiber = 0;
>>>
>>> This read is expensive, since the MDIO bus is slow. It would be better
>>> just to pass fibre as a parameter.
>>
>> But this function is used for other Marvell's phy, without fiber link for example.
>> And this function should has only the struct phy_device as parameter.
>>
>> I don't have idea to avoid that, without create a custom function for that which would be very similar to this function.
>> Or used a phy_device field for that? I think it's awful idea...
> 
> So i would have
> 
> static int marvell_read_status_page(struct phy_device *phydev, int page)
> {}
> 
> basically doing what you have above, but without the read.
> 
> static int marvell_read_status(struct phy_device *phydev)
> {
> 	if (phydev->supported & SUPPORTED_FIBRE) {
> 		marvell_read_status_page(phydev, MII_M1111_FIBER);
> 	   	if (phydev->link)
> 	      	return;
> 
> 	return marvell_read_status_page(phydev, MII_M1111_COPPER);
> }

Oh I see. Thank you!

> 
>>> I think it would be better to look for SUPPORTED_FIBRE in
>>> drv->features, rather than have two different functions.
>>>
>>> In fact, i would do that in general, rather than add your _fibre()
>>> functions.
>>
>> So, you suggest to do that in genphy_* functions or create marvell_* functions with this condition?
>> I'm agree with the second suggestion.
> 
> The second.

I'm working on this.
It's done for _resume and _suspend. It will be done for _status.

But, for aneg or ethtool concerned, I think adding these functions is better.

>>>> +
>>>> +/* marvell_resume_fiber
>>>> + *
>>>> + * Some Marvell's phys have two modes: fiber and copper.
>>>> + * Both need to be resumed
>>>> + */
>>>> +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.

Thank you.
Regards.

Charles-Antoine Couret