linux-kernel - Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y++CNk2Pv3aGYzLa@shell.armlinux.org.uk>
Date:   Fri, 17 Feb 2023 13:33:42 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        andrew@...n.ch, hkallweit1@...il.com, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        richardcochran@...il.com
Subject: Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_PEROUT
 for lan8841

On Fri, Feb 17, 2023 at 01:30:06PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> > +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > +	struct phy_device *phydev = ptp_priv->phydev;
> > +	u16 tmp;
> > +
> > +	tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > +	tmp &= ~BIT(pin);
> > +	phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
> 
> Problem 1: doesn't check the return value of phy_read_mmd(), so a
> spurious error results in an error code written back to the register.
> 
> Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
> probably also makes sense to cache the mask. Thus, this whole thing
> becomes:
> 
> 	u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
> 
> 	phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
> 	phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
> 	phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
> 
> although I'm not sure why you need to mask off bits 15:11.

Or even use phy_clear_bits_mmd(). There's also phy_set_bits_mmd() which I
think would also be useful elsewhere in this driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!