netdev - RE: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support

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] [day] [month] [year] [list]
Message-ID: <680426f9-4e0f-4840-abad-223bf04ef82d@SG2EHSMHS018.ehs.local>
Date:	Thu, 11 Feb 2010 14:22:26 -0700
From:	John Linn <John.Linn@...inx.com>
To:	"Grant Likely" <grant.likely@...retlab.ca>
CC:	<netdev@...r.kernel.org>, <linuxppc-dev@...abs.org>,
	<jgarzik@...ox.com>, <jwboyer@...ux.vnet.ibm.com>,
	<john.williams@...alogix.com>,
	"Sadanand Mutyala" <sadanan@...inx.com>
Subject: RE: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support

> -----Original Message-----
> From: glikely@...retlab.ca [mailto:glikely@...retlab.ca] On Behalf Of Grant Likely
> Sent: Thursday, February 11, 2010 2:16 PM
> To: John Linn
> Cc: netdev@...r.kernel.org; linuxppc-dev@...abs.org; jgarzik@...ox.com; jwboyer@...ux.vnet.ibm.com;
> john.williams@...alogix.com; Sadanand Mutyala
> Subject: Re: [PATCH] [V4] net: emaclite: adding MDIO and phy lib support
> 
> On Thu, Feb 11, 2010 at 1:52 PM, John Linn <john.linn@...inx.com> wrote:
> > These changes add MDIO and phy lib support to the driver as the
> > IP core now supports the MDIO bus.
> >
> > The MDIO bus and phy are added as a child to the emaclite in the device
> > tree as illustrated below.
> >
> > mdio {
> >        #address-cells = <1>;
> >        #size-cells = <0>;
> >        phy0: phy@7 {
> >                compatible = "marvell,88e1111";
> >                reg = <7>;
> >        } ;
> > }
> >
> > Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@...inx.com>
> > Signed-off-by: John Linn <john.linn@...inx.com>
> >
> > ---
> >
> > V2 - updated it for Grant's comments, except I couldn't find any tabs
> > converted to white space issue, let's see if V2 has it also
> >
> > V3 - updated it for Grant's comments, aded mutex release when a timeout
> > happens, and added Grant's acked by.
> >
> > V4 - removed the mutex as I realized the higher layer mdio calls already
> > use a mutex and there are no internal calls to the driver except in open.
> > Didn't integrate John W comments since releasing mutex wasn't needed.
> > ---
> >  drivers/net/Kconfig           |    1 +
> >  drivers/net/xilinx_emaclite.c |  381 ++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 339 insertions(+), 43 deletions(-)
> [...]
> >  /**
> >  * xemaclite_open - Open the network device
> >  * @dev:       Pointer to the network device
> >  *
> >  * This function sets the MAC address, requests an IRQ and enables interrupts
> >  * for the Emaclite device and starts the Tx queue.
> > + * It also connects to the phy device, if MDIO is included in Emaclite device.
> >  */
> >  static int xemaclite_open(struct net_device *dev)
> >  {
> > @@ -656,14 +924,50 @@ static int xemaclite_open(struct net_device *dev)
> >        /* Just to be safe, stop the device first */
> >        xemaclite_disable_interrupts(lp);
> >
> > +       if (lp->phy_node) {
> > +               u32 bmcr;
> > +
> > +               lp->phy_dev = of_phy_connect(lp->ndev, lp->phy_node,
> > +                                            xemaclite_adjust_link, 0,
> > +                                            PHY_INTERFACE_MODE_MII);
> > +               if (!lp->phy_dev) {
> > +                       dev_err(&lp->ndev->dev, "of_phy_connect() failed\n");
> > +                       return -ENODEV;
> > +               }
> > +
> > +               /* EmacLite doesn't support giga-bit speeds */
> > +               lp->phy_dev->supported &= (PHY_BASIC_FEATURES);
> > +               lp->phy_dev->advertising = lp->phy_dev->supported;
> > +
> > +               /* Don't advertise 1000BASE-T Full/Half duplex speeds */
> > +               xemaclite_mdio_write(lp->mii_bus, lp->phy_dev->addr,
> > +                                    MII_CTRL1000, 0x00);
> 
> Wait!  No.  This is wrong.  Sorry, I was all ready to ack this patch,
> but I just realized what was bothering me (and I should have clued
> into it earlier).  The driver cannot assume that the PHY is on an
> xemaclite MDIO bus.  It might be on a GPIO driven MDIO bus.  Or
> something else.  Calling xemaclite_mdio_{write,read} is not the right
> thing to do here.
> 
> Instead, the driver should call the phy library function
> phy_write(lp->phy_dev, MII_CTRL1000, 0);  The phylib code will route
> it to the correct mdio bus.  It also takes care of grabbing the mutex
> lock.
> 
> Make this fix, and then I can ack the driver.
> 

Ok, no worries, one more spin, I'm getting pretty good at spinning it.

Will make those changes to the calls in open, retest and respin.

Thanks,
John

> Cheers,
> g.
> 
> 
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html