netdev - Re: [PATCH net-next v2 04/10] net: phy: replace is_c45 with phy_accces

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: <52cdebe9-0f94-430d-93ff-11f26d2e3c5b@lunn.ch>
Date: Fri, 23 Jun 2023 19:34:22 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Michael Walle <mwalle@...nel.org>
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>,
	Yisen Zhuang <yisen.zhuang@...wei.com>,
	Salil Mehta <salil.mehta@...wei.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Marek Behún <kabel@...nel.org>,
	Xu Liang <lxu@...linear.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 04/10] net: phy: replace is_c45 with
 phy_accces_mode

> @@ -131,9 +131,11 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>  
>  	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
>  	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> -		phy = get_phy_device(bus, addr, is_c45);
> +		phy = get_phy_device(bus, addr,
> +				     is_c45 ? PHY_ACCESS_C45 : PHY_ACCESS_C22);
>  	else
> -		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +		phy = phy_device_create(bus, addr, phy_id, PHY_ACCESS_C22,
> +					NULL);

Documentation/devicetree/bindings/net/ethernet-phy.yaml says:

  compatible:
    oneOf:
      - const: ethernet-phy-ieee802.3-c22
        description: PHYs that implement IEEE802.3 clause 22
      - const: ethernet-phy-ieee802.3-c45
        description: PHYs that implement IEEE802.3 clause 45

It would be nice to make this documentation more specific. It now
refers to 'bus transaction', so maybe we want to append that to these
lines?

> -static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr, bool c45)
> +static struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr,
> +				       enum phy_access_mode mode)

> +/**
> + * enum phy_access_mode - PHY register access mode definitions
> + *
> + * @PHY_ACCESS_C22: use 802.3 c22 MDIO transactions
> + * @PHY_ACCESS_C45: use 802.3 c45 MDIO transactions
> + */
> +enum phy_access_mode {
> +	PHY_ACCESS_C22,
> +	PHY_ACCESS_C45,
> +};

Was the change from bool to enum enough to make the compiler warn when
passed the wrong type? i.e. a true/false? not PHY_ACCESS_C22 and
PHY_ACCESS_C45? Maybe we could set these enum values to 22 and 45?
true/false would then not match, and we get some sort of error, like
-EIO from the switch statement?

>  /**
>   * struct phy_device - An instance of a PHY
>   *
> @@ -539,8 +550,8 @@ struct macsec_ops;
>   * @devlink: Create a link between phy dev and mac dev, if the external phy
>   *           used by current mac interface is managed by another mac interface.
>   * @phy_id: UID for this device found during discovery
> - * @c45_ids: 802.3-c45 Device Identifiers if is_c45.
> - * @is_c45:  Set to true if this PHY uses clause 45 addressing.
> + * @access_mode:  MDIO access mode of the PHY.
> + * @c45_ids: 802.3-c45 Device Identifiers if it's an C45 PHY.
>   * @is_internal: Set to true if this PHY is internal to a MAC.
>   * @is_pseudo_fixed_link: Set to true if this PHY is an Ethernet switch, etc.
>   * @is_gigabit_capable: Set to true if PHY supports 1000Mbps
> @@ -637,8 +648,9 @@ struct phy_device {
>  
>  	u32 phy_id;
>  
> +	enum phy_access_mode access_mode;
> +

This enum might not pad too well between a u32 and struct? If you put
it after the bitfields, or maybe next to the enum phy_state the
compiler might make both a u16 and put them together?

	 Andrew