linux-kernel - Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for RGMII modes only

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:   Mon, 29 Apr 2019 10:37:21 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Serge Semin <fancer.lancer@...il.com>, Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Serge Semin <Sergey.Semin@...latforms.ru>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] net: phy: realtek: Change TX-delay setting for
 RGMII modes only

On 4/26/19 4:35 PM, Serge Semin wrote:
> On Fri, Apr 26, 2019 at 11:46:31PM +0200, Andrew Lunn wrote:
>> On Sat, Apr 27, 2019 at 12:21:12AM +0300, Serge Semin wrote:
>>> It's prone to problems if delay is cleared out for other than RGMII
>>> modes. So lets set/clear the TX-delay in the config register only
>>> if actually RGMII-like interface mode is requested.
>>>
>>> Signed-off-by: Serge Semin <fancer.lancer@...il.com>
>>>
>>> ---
>>>  drivers/net/phy/realtek.c | 16 ++++++++++++----
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index ab567a1923ad..a18cb01158f9 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -163,16 +163,24 @@ static int rtl8211c_config_init(struct phy_device *phydev)
>>>  static int rtl8211f_config_init(struct phy_device *phydev)
>>>  {
>>>  	int ret;
>>> -	u16 val = 0;
>>> +	u16 val;
>>>  
>>>  	ret = genphy_config_init(phydev);
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> -	/* enable TX-delay for rgmii-id and rgmii-txid, otherwise disable it */
>>> -	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>>> -	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
>>> +	/* enable TX-delay for rgmii-id/rgmii-txid, and disable it for rgmii */
>>> +	switch (phydev->interface) {
>>> +	case PHY_INTERFACE_MODE_RGMII:
>>> +		val = 0;
>>> +		break;
>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>> +	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>  		val = RTL8211F_TX_DELAY;
>>> +		break;
>>> +	default: /* the rest of the modes imply leaving delay as is. */
>>> +		return 0;
>>> +	}
>>
>> So there is no control of the RX delay?
>>
> 
> As you can see it hasn't been there even before this change. So I suppose
> either the hardware just doesn't support it (although the openly available
> datasheet states that there is an RXD pin) or the original driver developer
> decided to set TX-delay only.
> 
> Just to make sure you understand. I am not working for realtek and don't
> posses any inside info regarding these PHYs. I was working on a project,
> which happened to utilize a rtl8211e PHY. We needed to find a way to
> programmatically change the delays setting. So I searched the Internet
> and found the U-boot rtl8211f driver and freebsd-folks discussion. This
> info has been used to write the config_init method for Linux version of the
> PHY' driver. That's it.
> 
>> That means PHY_INTERFACE_MODE_RGMII_ID and
>> PHY_INTERFACE_MODE_RGMII_RXID are not supported, and you should return
>> -EINVAL.
>>
> 
> Apparently the current config_init method doesn't support RXID setting.
> The patch introduced current function code was submitted by
> Martin Blumenstingl in 2016:
> https://patchwork.kernel.org/patch/9447581/
> and was reviewed by Florian. So we'd better ask him why it was ok to mark
> the RGMII_ID as supported while only TX-delay could be set.
> I also failed to find anything regarding programmatic rtl8211f delays setting
> in the Internet. So at this point we can set TX-delay only for f-model of the PHY.
> 
> Anyway lets clarify the situation before to proceed further. You are suggesting
> to return an error in case if either RGMII_ID or RGMII_RXID interface mode is
> requested to be enabled for the PHY. It's fair seeing the driver can't fully
> support either of them.

That is how I read Andrew's suggestion and it is reasonable. WRT to the
original changes from Martin, he is probably the one you would want to
add to this conversation in case there are any RX delay control knobs
available, I certainly don't have the datasheet, and Martin's change
looks and looked reasonable, seemingly independent of the direction of
this very conversation we are having.

But what about the rest of the modes like GMII, MII
> and others? 

The delays should be largely irrelevant for GMII and MII, since a) the
PCB is required to have matching length traces, and b) these are not
double data rate interfaces

> Shouldn't we also return an error instead of leaving a default
> delay value?

That seems a bit harsh, those could have been configured by firmware,
whatever before Linux comes up and be correct and valid. We don't know
of a way to configure it, but that does not mean it does not exist and
some software is doing it already.

> 
> The same question can be actually asked regarding the config_init method of
> rtl8211e PHY, which BTW you already tagged as Reviewed-by.
> 
>> This is where we get into interesting backwards compatibility
>> issues. Are there any broken DT blobs with rgmii-id or rgmii-rxid,
>> which will break with such a change?
>>
> 
> Not that I am aware of and which simple grep rtl8211 could find. Do you
> know about one?
> 
> -Sergey
> 
>> 	Andrew


-- 
Florian