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: <CAHp75VeOefY_BK7MitFtdb7enrh5TOOwZ8kDJfVxvW28gejUbg@mail.gmail.com>
Date:   Mon, 13 Jan 2020 15:42:47 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC net-next] net: phy: Add basic support for Synopsys XPCS
 using a PHY driver

On Mon, Jan 13, 2020 at 3:13 PM Jose Abreu <Jose.Abreu@...opsys.com> wrote:
>
> Adds the basic support for XPCS including support for USXGMII.

...

> +#define SYNOPSYS_XPHY_ID               0x7996ced0
> +#define SYNOPSYS_XPHY_MASK             0xffffffff

GENMASK() ?
(It seems bits.h is missed in the headers block)

...

> +#define DW_USXGMII_2500                        (BIT(5))
> +#define DW_USXGMII_1000                        (BIT(6))
> +#define DW_USXGMII_100                 (BIT(13))
> +#define DW_USXGMII_10                  (0)

Useless parentheses.

...

> +static int dw_poll_reset(struct phy_device *phydev, int dev)
> +{
> +       /* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
> +       unsigned int retries = 12;
> +       int ret;
> +
> +       do {

> +               msleep(50);

It's a bit unusual to have timeout loop to be started with sleep.
Imagine the case when between writing a reset bit and actual checking
the scheduling happens and reset has been done You add here
unnecessary 50 ms of waiting.

> +               ret = phy_read_mmd(phydev, dev, MDIO_CTRL1);
> +               if (ret < 0)
> +                       return ret;
> +       } while (ret & MDIO_CTRL1_RESET && --retries);
> +       if (ret & MDIO_CTRL1_RESET)
> +               return -ETIMEDOUT;
> +
> +       return 0;
> +}
> +
> +static int __dw_soft_reset(struct phy_device *phydev, int dev, int reg)
> +{
> +       int val;

val?! Perhaps ret is better name?
Applicable to the rest of functions.

> +
> +       val = phy_write_mmd(phydev, dev, reg, MDIO_CTRL1_RESET);
> +       if (val < 0)
> +               return val;
> +

> +       val = dw_poll_reset(phydev, dev);
> +       if (val < 0)
> +               return val;
> +
> +       return 0;

return dw_poll_reset(...);

> +}
> +
> +static int dw_soft_reset(struct phy_device *phydev, u32 mmd_mask)
> +{
> +       int val, devad;
> +
> +       while (mmd_mask) {
> +               devad = __ffs(mmd_mask);
> +               mmd_mask &= ~BIT(devad);

for_each_set_bit()

> +
> +               val = __dw_soft_reset(phydev, devad, MDIO_CTRL1);
> +               if (val < 0)
> +                       return val;
> +       }
> +
> +       return 0;
> +}
> +
> +static int dw_read_link(struct phy_device *phydev, u32 mmd_mask)
> +{
> +       bool link = true;
> +       int val, devad;
> +
> +       while (mmd_mask) {
> +               devad = __ffs(mmd_mask);
> +               mmd_mask &= ~BIT(devad);

Ditto.

> +
> +               val = phy_read_mmd(phydev, devad, MDIO_STAT1);
> +               if (val < 0)
> +                       return val;
> +
> +               if (!(val & MDIO_STAT1_LSTATUS))
> +                       link = false;
> +       }
> +
> +       return link;
> +}

> +#define dw_warn(__phy, __args...) \

dw_warn() -> dw_warn_if_phy_link()

> +({ \
> +       if ((__phy)->link) \
> +               dev_warn(&(__phy)->mdio.dev, ##__args); \
> +})

...

> +       int val, speed_sel = 0x0;

Redundant assignment.

> +       switch (phydev->speed) {
> +       case SPEED_10:
> +               speed_sel = DW_USXGMII_10;
> +               break;
> +       case SPEED_100:
> +               speed_sel = DW_USXGMII_100;
> +               break;
> +       case SPEED_1000:
> +               speed_sel = DW_USXGMII_1000;
> +               break;
> +       case SPEED_2500:
> +               speed_sel = DW_USXGMII_2500;
> +               break;
> +       case SPEED_5000:
> +               speed_sel = DW_USXGMII_5000;
> +               break;
> +       case SPEED_10000:
> +               speed_sel = DW_USXGMII_10000;
> +               break;
> +       default:
> +               /* Nothing to do here */
> +               return 0;
> +       }

...

> +static int dw_config_aneg_c73(struct phy_device *phydev)
> +{
> +       u32 adv = 0;

Redundant assignment.

> +       int ret;
> +
> +       /* SR_AN_ADV3 */
> +       adv = phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV3);
> +       if (adv < 0)
> +               return adv;

> +}

...

> +       do {
> +               msleep(50);

Same as above about timeout loops.

> +               val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
> +               if (val < 0)
> +                       return val;
> +       } while (val & MDIO_AN_CTRL1_RESTART && --retries);

...

> +static int dw_aneg_done(struct phy_device *phydev)
> +{
> +       int val;
> +
> +       val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +       if (val < 0)
> +               return val;
> +
> +       if (val & MDIO_AN_STAT1_COMPLETE) {
> +               val = phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL1);
> +               if (val < 0)
> +                       return val;
> +
> +               /* Check if Aneg outcome is valid */
> +               if (!(val & 0x1))
> +                       goto fault;
> +

> +               return 1;

1?! What does it mean?

> +       }
> +
> +       if (val & MDIO_AN_STAT1_RFAULT)
> +               goto fault;
> +
> +       return 0;
> +fault:
> +       dev_err(&phydev->mdio.dev, "Invalid Autoneg result!\n");
> +       dev_err(&phydev->mdio.dev, "CTRL1=0x%x, STAT1=0x%x\n",
> +               phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1),
> +               phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1));
> +       dev_err(&phydev->mdio.dev, "ADV1=0x%x, ADV2=0x%x, ADV3=0x%x\n",
> +               phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV1),
> +               phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV2),
> +               phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_ADV3));
> +       dev_err(&phydev->mdio.dev, "LP_ADV1=0x%x, LP_ADV2=0x%x, LP_ADV3=0x%x\n",
> +               phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL1),
> +               phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL2),
> +               phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL3));
> +
> +       val = dw_soft_reset(phydev, MDIO_DEVS_PCS);
> +       if (val < 0)
> +               return val;
> +
> +       return dw_config_aneg(phydev);
> +}

...

> +       phydev->pause = val & DW_C73_PAUSE ? 1 : 0;
> +       phydev->asym_pause = val & DW_C73_ASYM_PAUSE ? 1 : 0;

!!(x) should work as well. But I think compiler optimizes that ternary well.

...

> +               val = dw_aneg_done(phydev);
> +               if (val <= 0) {

This <= 0 should be explained. Why we set link when it's < 0 or
otherwise why we return 0 when link is set to false.

> +                       phydev->link = false;
> +                       return val;
> +               }

...

> +static struct mdio_device_id __maybe_unused dw_tbl[] = {
> +       { SYNOPSYS_XPHY_ID, SYNOPSYS_XPHY_MASK },
> +       { },

Comma is not needed.

> +};

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ