linux-kernel - Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY

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]

Date:   Tue, 6 Dec 2022 14:47:49 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Piergiorgio Beruto <piergiorgio.beruto@...il.com>
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>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the
 onsemi NCN26000 10BASE-T1S PHY

> +// module parameter: if set, the link status is derived from the PLCA status
> +// default: false
> +static bool link_status_plca;
> +module_param(link_status_plca, bool, 0644);

No module parameters, they are considered a bad user interface.

> +static int ncn26000_get_features(struct phy_device *phydev)
> +{
> +	linkmode_zero(phydev->supported);
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
> +
> +	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
> +			 phydev->supported);
> +
> +	linkmode_copy(phydev->advertising, phydev->supported);

That should not be needed.

Also, look at PHY_BASIC_T1_FEATURES, and how it is used in
microchip_t1.c.

> +static int ncn26000_read_status(struct phy_device *phydev)
> +{
> +	// The NCN26000 reports NCN26000_LINK_STATUS_BIT if the link status of
> +	// the PHY is up. It further reports the logical AND of the link status
> +	// and the PLCA status in the BMSR_LSTATUS bit. Thus, report the link
> +	// status by testing the appropriate BMSR bit according to the module's
> +	// parameter configuration.
> +	const int lstatus_flag = link_status_plca ?
> +		BMSR_LSTATUS : NCN26000_BMSR_LINK_STATUS_BIT;
> +
> +	int ret;
> +
> +	ret = phy_read(phydev, MII_BMSR);
> +	if (unlikely(ret < 0))
> +		return ret;
> +
> +	// update link status
> +	phydev->link = (ret & lstatus_flag) ? 1 : 0;

What about the latching behaviour of LSTATUS?

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2289

> +
> +	// handle more IRQs here

You are not in an IRQ handler...

You should also be setting speed and duplex. I don't think they are
guaranteed to have any specific value if you don't set them.

> +
> +	return 0;
> +}
> +
> +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
> +{
> +	const struct ncn26000_priv *const priv = phydev->priv;
> +	int ret;
> +
> +	// clear the latched bits in MII_BMSR
> +	phy_read(phydev, MII_BMSR);

Why?

> +
> +	// read and aknowledge the IRQ status register
> +	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
> +
> +	if (unlikely(ret < 0) || (ret & priv->enabled_irqs) == 0)

How does NCN26000_REG_IRQ_STATUS work? Can it have bits set which are
not in NCN26000_REG_IRQ_CTL ? That does happen sometimes, but is
pretty unusual. If not, you don't need to track priv->enabled_irqs,
just ensure ret is not 0.

     Andrew