| 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: <CA+V-a8vFEHr+3yR7=JAki3YDe==dAUv3m4PrD-nWhVg8hXgJcQ@mail.gmail.com>
Date: Fri, 7 Nov 2025 10:34:32 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Horatiu Vultur <horatiu.vultur@...rochip.com>, Geert Uytterhoeven <geert+renesas@...der.be>,
Vladimir Oltean <vladimir.oltean@....com>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
Maxime Chevallier <maxime.chevallier@...tlin.com>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH net-next] net: phy: mscc: Add support for PHY LEDs on VSC8541
Hi Andrew,
Thank you for the review.
On Thu, Nov 6, 2025 at 8:45 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +static int vsc85xx_led_cntl_set_lock_unlock(struct phy_device *phydev,
> > + u8 led_num,
> > + u8 mode, bool lock)
> > {
> > int rc;
> > u16 reg_val;
> >
> > - mutex_lock(&phydev->lock);
> > + if (lock)
> > + mutex_lock(&phydev->lock);
> > reg_val = phy_read(phydev, MSCC_PHY_LED_MODE_SEL);
> > reg_val &= ~LED_MODE_SEL_MASK(led_num);
> > reg_val |= LED_MODE_SEL(led_num, (u16)mode);
> > rc = phy_write(phydev, MSCC_PHY_LED_MODE_SEL, reg_val);
> > - mutex_unlock(&phydev->lock);
> > + if (lock)
> > + mutex_unlock(&phydev->lock);
> >
> > return rc;
> > }
>
> The normal way to do this is have _vsc85xx_led_cntl_set() manipulate
> the hardware, no locking. And have vsc85xx_led_cntl_set() take the
> lock, call _vsc85xx_led_cntl_set(), and then release the lock. You can
> then call _vsc85xx_led_cntl_set() if needed.
>
Ok, I will add _vsc85xx_led_cntl_set() helper and use it in
vsc85xx_led_cntl_set() and led functions (for example
vsc85xx_mdix_get).
> > +static int vsc8541_led_combine_disable_set(struct phy_device *phydev, u8 led_num,
> > + bool combine_disable)
> > +{
> > + u16 reg_val;
> > +
> > + reg_val = phy_read(phydev, MSCC_PHY_LED_BEHAVIOR);
>
> phy_read() can return a negative value. You should not assign that to
> a u16.
>
Agreed, I will check the return value of this. I followed the approach
which was currently used in the driver.
> Also, BEHAVIOUR.
>
> > + reg_val &= ~LED_COMBINE_DIS_MASK(led_num);
> > + reg_val |= LED_COMBINE_DIS(led_num, combine_disable);
> > +
> > + return phy_write(phydev, MSCC_PHY_LED_BEHAVIOR, reg_val);
>
> You can probably use phy_modify() here.
>
Agreed, that will simplify the code.
> > +static int vsc8541_led_hw_is_supported(struct phy_device *phydev, u8 index,
> > + unsigned long rules)
> > +{
> > + struct vsc8531_private *vsc8531 = phydev->priv;
> > + static const unsigned long supported = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_1000) |
> > + BIT(TRIGGER_NETDEV_LINK_100) |
> > + BIT(TRIGGER_NETDEV_LINK_10) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > +
>
> Reverse Christmas tree. The lines should be sorted, longest first,
> shortest last.
>
Agreed.
> > +static int vsc8541_led_hw_control_get(struct phy_device *phydev, u8 index,
> > + unsigned long *rules)
> > +{
> > + struct vsc8531_private *vsc8531 = phydev->priv;
> > + u16 reg;
> > +
> > + if (index >= vsc8531->nleds)
> > + return -EINVAL;
> > +
> > + reg = phy_read(phydev, MSCC_PHY_LED_MODE_SEL) & LED_MODE_SEL_MASK(index);
>
> Another cause of u16, when int should be used. Please check all
> instances of phy_read().
>
Ok.
> > + reg >>= LED_MODE_SEL_POS(index);
> > + switch (reg) {
> > + case VSC8531_LINK_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_LINK_1000_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_1000) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_LINK_100_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_100) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_LINK_10_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_10) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_LINK_100_1000_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_100) |
> > + BIT(TRIGGER_NETDEV_LINK_1000) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_LINK_10_1000_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_10) |
> > + BIT(TRIGGER_NETDEV_LINK_1000) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_LINK_10_100_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_LINK_10) |
> > + BIT(TRIGGER_NETDEV_LINK_100) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
> > +
> > + case VSC8531_ACTIVITY:
> > + *rules = BIT(TRIGGER_NETDEV_LINK) |
> > + BIT(TRIGGER_NETDEV_RX) |
> > + BIT(TRIGGER_NETDEV_TX);
> > + break;
>
> Should the combine bit be taken into account here?
>
Agreed, I will drop setting TRIGGER_NETDEV_RX/TRIGGER_NETDEV_TX from
all the above case and set it based on the combined bit like below:
if (!behavior && *rules)
*rules |= BIT(TRIGGER_NETDEV_RX) | BIT(TRIGGER_NETDEV_TX);
> > @@ -2343,6 +2532,26 @@ static int vsc85xx_probe(struct phy_device *phydev)
> > if (!vsc8531->stats)
> > return -ENOMEM;
> >
> > + phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> > + if (phy_id == PHY_ID_VSC8541) {
>
> The VSC8541 has its own probe function, vsc8514_probe(). Why is this
> needed?
>
vsc85xx_probe() is used for other PHYs along with VSC8541 hence this
check, vsc8514_probe() is for 8514 PHY.
Cheers,
Prabhakar
Powered by blists - more mailing lists