netdev - Re: [PATCH net-next v2 7/8] dpaa2-mac: configure the SerDes phy on a protocol change

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220309203350.qzqgbu6hmb5hiamn@skbuf>
Date:   Wed, 9 Mar 2022 20:33:50 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kishon@...com" <kishon@...com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        Leo Li <leoyang.li@....com>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Hongxing Zhu <hongxing.zhu@....com>
Subject: Re: [PATCH net-next v2 7/8] dpaa2-mac: configure the SerDes phy on a
 protocol change

On Wed, Mar 09, 2022 at 06:46:26PM +0000, Russell King (Oracle) wrote:
> Hi Ioana,
> 

Hi Russell,

> > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> > @@ -2077,8 +2077,10 @@ static int dpaa2_eth_open(struct net_device *net_dev)
> >  		goto enable_err;
> >  	}
> >  
> > -	if (dpaa2_eth_is_type_phy(priv))
> > +	if (dpaa2_eth_is_type_phy(priv)) {
> >  		phylink_start(priv->mac->phylink);
> > +		dpaa2_mac_start(priv->mac);
> 
> Is this safe? Shouldn't dpaa2_mac_start() come before phylink_start()
> in case phylink determines that the link is somehow already up? I'm
> a big fan of teardown being in the reverse order of setup so having
> the start and stop below in the same order just doesn't look right.

Agree that the teardown being done in the reverse order just looks
better. I can change it, of course.

I didn't really spot any actual problems with how are things now, but it
would be better to just bring up the SerDes lanes and then call
phylink_start().

> > +static enum dpmac_eth_if dpmac_eth_if_mode(phy_interface_t if_mode)
> > +{
> > +	switch (if_mode) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> 
> Shouldn't this also include the other RGMII modes (which, from the MAC
> point of view, are all synonymous?
> 

Good point. Thanks for pointing it out.

> > +static int dpaa2_mac_prepare(struct phylink_config *config, unsigned int mode,
> > +			     phy_interface_t interface)
> > +{
> > +	dpaa2_mac_link_down(config, mode, interface);
> 
> You should never see a reconfiguration while the link is up. However,
> if the link is in in-band mode, then obviously the link could come up
> at any moment, and in that case, forcing it down in mac_prepare() is
> a good idea - but that forcing needs to be removed in mac_finish()
> to allow in-band to work again. Not sure that your firmware allows
> that though, and I'm not convinced that calling the above function
> achieves any of those guarantees.
> 

Ok, I didn't know that I this was a guarantee from phylink's part.
In this case, I can just remove the prepare step, sure.

> > +	/* In case we have access to the SerDes phy/lane, then ask the SerDes
> > +	 * driver what interfaces are supported based on the current PLL
> > +	 * configuration.
> > +	 */
> > +	for (intf = 0; intf < PHY_INTERFACE_MODE_MAX; intf++) {
> 
> You probably want to avoid PHY_INTERFACE_MODE_NA here, even though your
> driver may reject it anyway.

Yes, I'll just start from PHY_INTERFACE_MODE_INTERNAL.

> > -	if (dpaa2_switch_port_is_type_phy(port_priv))
> > +	if (dpaa2_switch_port_is_type_phy(port_priv)) {
> >  		phylink_start(port_priv->mac->phylink);
> > +		dpaa2_mac_start(port_priv->mac);
> 
> Same comments as for dpaa2-mac.

Sure. I'll change the order.

> 
> Thanks!
> 

Thanks!
Ioana