| 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
| ||
|
Message-ID: <0bd6ac4b-f943-c5d6-7154-dddc538008f2@gmail.com>
Date: Wed, 25 Apr 2018 12:23:41 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Raghuram Chary J <raghuramchary.jallipalli@...rochip.com>,
davem@...emloft.net
Cc: netdev@...r.kernel.org, unglinuxdriver@...rochip.com,
woojung.huh@...rochip.com
Subject: Re: [PATCH v2 net-next] lan78xx: Lan7801 Support for Fixed PHY
On 04/25/2018 12:07 PM, Raghuram Chary J wrote:
> Adding Fixed PHY support to the lan78xx driver.
>
> Signed-off-by: Raghuram Chary J <raghuramchary.jallipalli@...rochip.com>
> ---
> v0->v1:
> * Remove driver version #define
This should be a separate patch of its own, more comment below.
> * Modify netdev_info to netdev_dbg
> * Move lan7801 specific to new routine and add switch case
> * Minor cleanup
> -static int lan78xx_phy_init(struct lan78xx_net *dev)
> +static int lan7801_phy_init(struct phy_device **phydev,
> + struct lan78xx_net *dev)
Passing a **phydev looks really unnecessary here, why don't just return
a struct phy_device * as a return argument here? If you need to
communicate a specific return value, you can use ERR_PTR()/PTR_ERR() for
that purpose.
> {
> + u32 buf;
> int ret;
> - u32 mii_adv;
> - struct phy_device *phydev;
> -
> - phydev = phy_find_first(dev->mdiobus);
> - if (!phydev) {
> - netdev_err(dev->net, "no PHY found\n");
> - return -EIO;
> - }
> -
> - if ((dev->chipid == ID_REV_CHIP_ID_7800_) ||
> - (dev->chipid == ID_REV_CHIP_ID_7850_)) {
> - phydev->is_internal = true;
> - dev->interface = PHY_INTERFACE_MODE_GMII;
> -
> - } else if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> - if (!phydev->drv) {
> + struct fixed_phy_status fphy_status = {
> + .link = 1,
> + .speed = SPEED_1000,
> + .duplex = DUPLEX_FULL,
> + };
> +
> + if (!*phydev) {
> + netdev_dbg(dev->net, "PHY Not Found!! Registering Fixed PHY\n");
> + *phydev = fixed_phy_register(PHY_POLL, &fphy_status, -1,
> + NULL);
> + if (IS_ERR(*phydev)) {
> + netdev_err(dev->net, "No PHY/fixed_PHY found\n");
> + return -ENODEV;
> + }
> + netdev_dbg(dev->net, "Registered FIXED PHY\n");
> + dev->interface = PHY_INTERFACE_MODE_RGMII;
> + ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
> + MAC_RGMII_ID_TXC_DELAY_EN_);
> + ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL, 0x3D00);
> + ret = lan78xx_read_reg(dev, HW_CFG, &buf);
> + buf |= HW_CFG_CLK125_EN_;
> + buf |= HW_CFG_REFCLK25_EN_;
> + ret = lan78xx_write_reg(dev, HW_CFG, buf);
> + } else {
> + if (!(*phydev)->drv) {
> netdev_err(dev->net, "no PHY driver found\n");
> return -EIO;
> }
> -
> dev->interface = PHY_INTERFACE_MODE_RGMII;
> -
> /* external PHY fixup for KSZ9031RNX */
> ret = phy_register_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0,
> ksz9031rnx_fixup);
> if (ret < 0) {
> - netdev_err(dev->net, "fail to register fixup\n");
> + netdev_err(dev->net, "fail to register fixup PHY_KSZ9031RNX\n");
Unrelated message change, that should be a separate commit.
> return ret;
> }
> /* external PHY fixup for LAN8835 */
> ret = phy_register_fixup_for_uid(PHY_LAN8835, 0xfffffff0,
> lan8835_fixup);
> if (ret < 0) {
> - netdev_err(dev->net, "fail to register fixup\n");
> + netdev_err(dev->net, "fail to register fixup for PHY_LAN8835\n");
Same here, can be grouped in the same commit as the one right above.
> return ret;
> }
> /* add more external PHY fixup here if needed */
>
> - phydev->is_internal = false;
> - } else {
> - netdev_err(dev->net, "unknown ID found\n");
> - ret = -EIO;
> - goto error;
> + (*phydev)->is_internal = false;
> + }
> + return 0;
> +}
> +
> +static int lan78xx_phy_init(struct lan78xx_net *dev)
> +{
> + int ret;
> + u32 mii_adv;
> + struct phy_device *phydev;
> +
> + phydev = phy_find_first(dev->mdiobus);
> + switch (dev->chipid) {
> + case ID_REV_CHIP_ID_7801_:
> + ret = lan7801_phy_init(&phydev, dev);
> + if (ret < 0) {
> + netdev_err(dev->net, "lan7801: PHY Init Failed");
> + return ret;
> + }
> + break;
> +
> + case ID_REV_CHIP_ID_7800_:
> + case ID_REV_CHIP_ID_7850_:
> + if (!phydev) {
> + netdev_err(dev->net, "no PHY found\n");
> + return -EIO;
> + }
> + phydev->is_internal = true;
Uggh you are not really supposed to set that, the matching PHY driver
should have PHY_IS_INTERNAL in the .flags member to indicate that to the
core PHY library.
> + dev->interface = PHY_INTERFACE_MODE_GMII;
> + break;
> +
> + default:
> + netdev_err(dev->net, "Unknown CHIP ID found\n");
> + return -EIO;
> }
>
> /* if phyirq is not set, use polling mode in phylib */
> @@ -2067,6 +2103,12 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> if (ret) {
> netdev_err(dev->net, "can't attach PHY to %s\n",
> dev->mdiobus->id);
> + if (dev->chipid == ID_REV_CHIP_ID_7801_) {
> + phy_unregister_fixup_for_uid(PHY_KSZ9031RNX,
> + 0xfffffff0);
> + phy_unregister_fixup_for_uid(PHY_LAN8835,
> + 0xfffffff0);
> + }
> return -EIO;
> }
>
> @@ -2084,12 +2126,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
> dev->fc_autoneg = phydev->autoneg;
>
> return 0;
> -
> -error:
> - phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
> - phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
> -
> - return ret;
> }
>
> static int lan78xx_set_rx_max_frame_length(struct lan78xx_net *dev, int size)
> @@ -3487,6 +3523,7 @@ static void lan78xx_disconnect(struct usb_interface *intf)
> struct lan78xx_net *dev;
> struct usb_device *udev;
> struct net_device *net;
> + struct phy_device *phydev;
>
> dev = usb_get_intfdata(intf);
> usb_set_intfdata(intf, NULL);
> @@ -3495,12 +3532,16 @@ static void lan78xx_disconnect(struct usb_interface *intf)
>
> udev = interface_to_usbdev(intf);
> net = dev->net;
> + phydev = net->phydev;
>
> phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
> phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
>
> phy_disconnect(net->phydev);
>
> + if (phy_is_pseudo_fixed_link(phydev))
> + fixed_phy_unregister(phydev);
That part now looks good.
> +
> unregister_netdev(net);
>
> cancel_delayed_work_sync(&dev->wq);
>
--
Florian
Powered by blists - more mailing lists