| Message ID | 20190103011040.25974-10-marex@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [01/19] usbnet: smsc95xx: Fix memory leak in smsc95xx_bind | expand |
On Thu, Jan 03, 2019 at 02:10:30AM +0100, Marek Vasut wrote: > Add code to detect and connect to PHY. The internal PHY of the SMSC95xx > is a regular SMSC LAN8700 and the driver only supports the internal PHY, > so just use the SMSC PHY driver to configure the PHY. Note that the > driver does a lot of extra configuration of the PHY, which is left in > to avoid breakage. Some of the extra configuration is sorted out by > later patches in this series. Hi Marek A MAC driver is not expected to touch the PHY at all. Please try to remove as much of the extra configuration as possible, adding it to the LAN8700 PHY driver as needed. You also have to be careful of locking. phylib takes the phydev lock when calling into the PHY driver. Anything the MAC does to the PHY is not going to be done with this lock held. So bad things can happen. Andrew
On 1/3/19 2:22 PM, Andrew Lunn wrote: > On Thu, Jan 03, 2019 at 02:10:30AM +0100, Marek Vasut wrote: >> Add code to detect and connect to PHY. The internal PHY of the SMSC95xx >> is a regular SMSC LAN8700 and the driver only supports the internal PHY, >> so just use the SMSC PHY driver to configure the PHY. Note that the >> driver does a lot of extra configuration of the PHY, which is left in >> to avoid breakage. Some of the extra configuration is sorted out by >> later patches in this series. > > Hi Marek > > A MAC driver is not expected to touch the PHY at all. Well, sure, that's not how the SMSC95xx driver is implemented. This series tries to make that a bit better. > Please try to > remove as much of the extra configuration as possible, adding it to > the LAN8700 PHY driver as needed. Sure, that's the plan. > You also have to be careful of locking. phylib takes the phydev lock > when calling into the PHY driver. Anything the MAC does to the PHY is > not going to be done with this lock held. So bad things can happen. I think I can add a patch which grabs the lock in the MDIO accessors used by the SMSC95xx driver, at least temporarily, until it is fully migrated to phydev ? Or is there a better option ?
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 2cd5e1d34559..ce61be8ee32b 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -78,6 +78,7 @@ struct smsc95xx_priv { struct delayed_work carrier_check; struct usbnet *dev; struct mii_bus *mii_bus; + struct phy_device *phydev; }; static bool turbo_mode = true; @@ -960,6 +961,11 @@ static int smsc95xx_start_rx_path(struct usbnet *dev) return smsc95xx_write_reg(dev, MAC_CR, pdata->mac_cr); } + +static void smsc95xx_adjust_link(struct net_device *netdev) +{ +} + static int smsc95xx_phy_initialize(struct usbnet *dev) { int bmcr, ret, timeout = 0; @@ -1308,6 +1314,10 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf) if (ret) goto err_mdiobus_alloc; + ret = mdiobus_register(bus); + if (ret) + goto err_mdiobus_register; + /* Initialize MII structure */ dev->mii.dev = dev->net; dev->mii.mdio_read = smsc95xx_mdio_read; @@ -1316,9 +1326,24 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf) dev->mii.reg_num_mask = 0x1f; dev->mii.phy_id = SMSC95XX_INTERNAL_PHY_ID; - ret = mdiobus_register(bus); - if (ret) + /* Connect to PHY */ + pdata->phydev = phy_find_first(pdata->mii_bus); + if (!pdata->phydev) { + netdev_err(dev->net, "no PHY found\n"); + ret = -EIO; goto err_mdiobus_register; + } + + ret = phy_connect_direct(dev->net, pdata->phydev, + &smsc95xx_adjust_link, + PHY_INTERFACE_MODE_MII); + if (ret) { + netdev_err(dev->net, "Could not connect to PHY device\n"); + goto err_mdiobus_register; + } + + genphy_resume(pdata->phydev); + phy_start(pdata->phydev); ret = smsc95xx_reset_post(dev); if (ret)
Add code to detect and connect to PHY. The internal PHY of the SMSC95xx is a regular SMSC LAN8700 and the driver only supports the internal PHY, so just use the SMSC PHY driver to configure the PHY. Note that the driver does a lot of extra configuration of the PHY, which is left in to avoid breakage. Some of the extra configuration is sorted out by later patches in this series. Signed-off-by: Marek Vasut <marex@denx.de> Cc: David S. Miller <davem@davemloft.net> Cc: Nisar Sayed <Nisar.Sayed@microchip.com> Cc: Woojung Huh <Woojung.Huh@microchip.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: linux-usb@vger.kernel.org To: netdev@vger.kernel.org --- drivers/net/usb/smsc95xx.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)