netdev - Re: [PATCH net-next v2 3/6] smsc95xx: add PAL support to use external PHY drivers

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: <c2cb789ac1beebc5c337e97c05e462202d19abcf.camel@microchip.com>
Date:   Fri, 24 Jul 2020 15:17:26 +0000
From:   <Andre.Edich@...rochip.com>
To:     <andrew@...n.ch>
CC:     <Parthiban.Veerasooran@...rochip.com>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>, <steve.glendinning@...well.net>
Subject: Re: [PATCH net-next v2 3/6] smsc95xx: add PAL support to use external
 PHY drivers

On Fri, 2020-07-24 at 00:39 +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, Jul 23, 2020 at 01:55:04PM +0200, Andre Edich wrote:
> > Generally, each PHY has their own configuration and it can be done
> > through an external PHY driver.  The smsc95xx driver uses only the
> > hard-coded internal PHY configuration.
> > 
> > This patch adds PAL (PHY Abstraction Layer) support to probe
> > external
> > PHY drivers for configuring external PHYs.
> 
> Hi Andre
> 
> We call it phylib, not PAL.

Hi Andrew,

thank you for the feedback. In the next version, I will correct these
wordings as well.

> 
> >  static int __must_check smsc95xx_wait_eeprom(struct usbnet *dev)
> >  {
> >       unsigned long start_time = jiffies;
> > @@ -559,15 +580,20 @@ static int smsc95xx_link_reset(struct usbnet
> > *dev)
> >       u16 lcladv, rmtadv;
> >       int ret;
> > 
> > -     /* clear interrupt status */
> > -     ret = smsc95xx_mdio_read(dev->net, mii->phy_id, PHY_INT_SRC);
> > -     if (ret < 0)
> > -             return ret;
> > -
> >       ret = smsc95xx_write_reg(dev, INT_STS, INT_STS_CLEAR_ALL_);
> >       if (ret < 0)
> >               return ret;
> > 
> > +     if (pdata->internal_phy) {
> > +             /* clear interrupt status */
> > +             ret = smsc95xx_mdio_read(dev->net, mii->phy_id,
> > PHY_INT_SRC);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             smsc95xx_mdio_write(dev->net, mii->phy_id,
> > PHY_INT_MASK,
> > +                                 PHY_INT_MASK_DEFAULT_);
> > +     }
> 
> The PHY driver should do this, not the MAC driver.
> 
> Which PHY driver is used for the internal PHY? In theory, you should
> not need to know if it is internal or external, it is just a PHY. 

Yes sure, you are right.  I see the drivers/net/phy/smsc.c that is
probed for the internal PHY of the DUT's Ethernet controller.

> 
> That
> might mean you need to move some code from this driver into the PHY
> driver, if it is currently missing in the PHY driver.

Correct, the PHY driver does interrupt setup activities, so that they
can be removed from the smsc95xx.
 
> 
> > +
> >       mii_check_media(mii, 1, 1);
> >       mii_ethtool_gset(&dev->mii, &ecmd);
> >       lcladv = smsc95xx_mdio_read(dev->net, mii->phy_id,
> > MII_ADVERTISE);
> > @@ -851,10 +877,10 @@ static int smsc95xx_get_link_ksettings(struct
> > net_device *net,
> >       int retval;
> > 
> >       retval = usbnet_get_link_ksettings(net, cmd);
> > -
> > -     cmd->base.eth_tp_mdix = pdata->mdix_ctrl;
> > -     cmd->base.eth_tp_mdix_ctrl = pdata->mdix_ctrl;
> > -
> > +     if (pdata->internal_phy) {
> > +             cmd->base.eth_tp_mdix = pdata->mdix_ctrl;
> > +             cmd->base.eth_tp_mdix_ctrl = pdata->mdix_ctrl;
> > +     }
> 
> Again, they PHY driver should take care of this. You need to set
> phydev->mdix_ctrl before starting the PHY. The PHY driver should set
> phdev->mdix to the current status.

The SMSC Phy driver does not have any MDIX setup code, but I think I've
got the idea now.

> 
> > +static void smsc95xx_handle_link_change(struct net_device *net)
> > +{
> > +     phy_print_status(net->phydev);
> 
> So the MAC does not care about the speed? The pause configuration?
> Duplex?

Now, I'm wondering how those "care about speed", "pause", and "duplex"
work in the current smsc95xx.  I guess, we did not touch any of those
activities with our patches.

Thanks a lot.
Andre

> 
>         Andrew