netdev - Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb subdriver for RTL8365MB-VC

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, 23 Aug 2021 10:08:54 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Alvin Šipraga <alvin@...s.dk>,
        Linus Walleij <linus.walleij@...aro.org>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Michael Rasmussen <MIR@...g-olufsen.dk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH net-next 4/5] net: dsa: realtek-smi: add rtl8365mb
 subdriver for RTL8365MB-VC

On 8/23/21 3:14 AM, Andrew Lunn wrote:
>> Just to clarify, this means I should set the physical port number in the
>> reg field of the device tree? i.e.:
>>
>> 	port@4 {
>> 		reg = <6>; /* <--- instead of <4>? */
>> 		label = "cpu";
>> 		ethernet = <&fec1>;
>> 		phy-mode = "rgmii-id";
>> 		fixed-link {
>> 			speed = <1000>;
>> 			full-duplex;
>> 			pause;
>> 		};
>> 	};
> 
> Yes, this is fine.
> 
>>>> +static int rtl8365mb_port_enable(struct dsa_switch *ds, int port,
>>>> +				 struct phy_device *phy)
>>>> +{
>>>> +	struct realtek_smi *smi = ds->priv;
>>>> +	int val;
>>>> +
>>>> +	if (dsa_is_user_port(ds, port)) {
>>>> +		/* Power up the internal PHY and restart autonegotiation */
>>>> +		val = rtl8365mb_phy_read(smi, port, MII_BMCR);
>>>> +		if (val < 0)
>>>> +			return val;
>>>> +
>>>> +		val &= ~BMCR_PDOWN;
>>>> +		val |= BMCR_ANRESTART;
>>>> +
>>>> +		return rtl8365mb_phy_write(smi, port, MII_BMCR, val);
>>>> +	}
>>>
>>> There should not be any need to do this. phylib should do it. In
>>> generally, you should not need to touch any PHY registers, you just
>>> need to allow access to the PHY registers.
>>
>> Will phylib also the opposite when the interface is administratively put
>> down (e.g. ip link set dev swp2 down)? The switch doesn't seem to have
>> any other handle for stopping the flow of packets from a port except to
>> power down the internal PHY, hence why I put this in for port_disable.
>> For port_enable I just did the opposite but I can see why it's not
>> necessary.
> 
> When the MAC and PHY are attached phy_attach_direct() gets called,
> which calls phy_resume(phydev); The generic implementation clears the
> BMCR_PDOWN bit.
> 
> phy_detach() will suspend the PHY, which sets the BMCR_PDOWN bit.
> 
> But there are two different schemes for doing this.  Some MAC drivers
> connect the PHY during probe. Others do it at open. DSA does it at
> probe. So this is generic feature is not going to work for you. You
> might want to put some printk() in phy_suspend and phy_resume to check
> that.

Thanks, I will double check.

> 
> So, setting/clearing the PDOWN bit does seems reasonable. But please
> document in the functions why you are doing this. Also, don't restart
> autoneg. That really should be up to phylib, and it should be
> triggered by phylink_start() which should be called directly after
> port_enable().

Understood. I'll review the necessity of these calls and add a comment 
in v2 if I keep them.

> 
> 	Andrew
>