Re: [PATCH net-next v6 03/14] net: phy: Introduce PHY ports representation

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Romain,

On Mon, 12 May 2025 09:53:09 +0200
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hi Maxime,

I'm about to send a V7, but I realise now I didn't address these
reviews... sorry about that

> On Wednesday, 7 May 2025 15:53:19 CEST Maxime Chevallier wrote:
> > Ethernet provides a wide variety of layer 1 protocols and standards for
> > data transmission. The front-facing ports of an interface have their own
> > complexity and configurability.
> >   
> ...
> > +
> > +static int phy_default_setup_single_port(struct phy_device *phydev)
> > +{
> > +	struct phy_port *port = phy_port_alloc();
> > +
> > +	if (!port)
> > +		return -ENOMEM;
> > +
> > +	port->parent_type = PHY_PORT_PHY;
> > +	port->phy = phydev;
> > +	linkmode_copy(port->supported, phydev->supported);
> > +
> > +	phy_add_port(phydev, port);
> > +
> > +	/* default medium is copper */
> > +	if (!port->mediums)
> > +		port->mediums |= BIT(ETHTOOL_LINK_MEDIUM_BASET);  
> 
> Could this be moved to phy_port_alloc() instead? That way, you'd avoid the 
> extra conditional and the "default medium == baseT" rule would be enforced as 
> early as possible.

I'll actually remove that altogether. For a single-port PHY, we can
actually derive the mediums from the PHY's supported field.

[...]

> > +	for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST) {
> > +		linkmode_zero(supported);  
> 
> ethtool_medium_get_supported() can only set bits in "supported", so you could 
> just do:
> 
> ```
> for_each_set_bit(i, &port->mediums, __ETHTOOL_LINK_MEDIUM_LAST)
> 	ethtool_medium_get_supported(port->supported, i, port->lanes);
> ```
> 
> unless you're wary of someone modifying ethtool_medium_get_supported() in a 
> way that breaks this in the future?

Hmm yes, but at least here it makes it obvious we're accumulating the
linkmodes :/

> 
> > +		ethtool_medium_get_supported(supported, i, port->lanes);
> > +		linkmode_or(port->supported, port->supported, supported);
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(phy_port_update_supported);
> > +
> > +/**
> > + * phy_port_get_type() - get the PORT_* attribut for that port.
> > + * @port: The port we want the information from
> > + *
> > + * Returns: A PORT_XXX value.
> > + */
> > +int phy_port_get_type(struct phy_port *port)
> > +{
> > +	if (port->mediums & ETHTOOL_LINK_MEDIUM_BASET)
> > +		return PORT_TP;
> > +
> > +	if (phy_port_is_fiber(port))
> > +		return PORT_FIBRE;
> > +
> > +	return PORT_OTHER;
> > +}
> > +EXPORT_SYMBOL_GPL(phy_port_get_type);
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index c1d805d3e02f..0d3063af5905 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -226,6 +226,10 @@ extern const struct link_mode_info link_mode_params[];
> > 
> >  extern const char ethtool_link_medium_names[][ETH_GSTRING_LEN];
> > 
> > +#define ETHTOOL_MEDIUM_FIBER_BITS (BIT(ETHTOOL_LINK_MEDIUM_BASES) | \
> > +				   BIT(ETHTOOL_LINK_MEDIUM_BASEL) | \
> > +				   BIT(ETHTOOL_LINK_MEDIUM_BASEF))
> > +
> >  static inline const char *phy_mediums(enum ethtool_link_medium medium)
> >  {
> >  	if (medium >= __ETHTOOL_LINK_MEDIUM_LAST)
> > @@ -234,6 +238,17 @@ static inline const char *phy_mediums(enum
> > ethtool_link_medium medium) return ethtool_link_medium_names[medium];
> >  }
> > 
> > +static inline int phy_medium_default_lanes(enum ethtool_link_medium medium)
> > +{
> > +	/* Let's consider that the default BaseT ethernet is BaseT4, i.e.
> > +	 * Gigabit Ethernet.
> > +	 */
> > +	if (medium == ETHTOOL_LINK_MEDIUM_BASET)
> > +		return 4;
> > +
> > +	return 1;
> > +}
> > +
> >  /* declare a link mode bitmap */
> >  #define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
> >  	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index d62d292024bc..0180f4d4fd7d 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -299,6 +299,7 @@ static inline long rgmii_clock(int speed)
> >  struct device;
> >  struct kernel_hwtstamp_config;
> >  struct phylink;
> > +struct phy_port;
> >  struct sfp_bus;
> >  struct sfp_upstream_ops;
> >  struct sk_buff;
> > @@ -590,6 +591,9 @@ struct macsec_ops;
> >   * @master_slave_state: Current master/slave configuration
> >   * @mii_ts: Pointer to time stamper callbacks
> >   * @psec: Pointer to Power Sourcing Equipment control struct
> > + * @ports: List of PHY ports structures
> > + * @n_ports: Number of ports currently attached to the PHY
> > + * @max_n_ports: Max number of ports this PHY can expose
> >   * @lock:  Mutex for serialization access to PHY
> >   * @state_queue: Work queue for state machine
> >   * @link_down_events: Number of times link was lost
> > @@ -724,6 +728,10 @@ struct phy_device {
> >  	struct mii_timestamper *mii_ts;
> >  	struct pse_control *psec;
> > 
> > +	struct list_head ports;
> > +	int n_ports;
> > +	int max_n_ports;
> > +
> >  	u8 mdix;
> >  	u8 mdix_ctrl;
> > 
> > @@ -1242,6 +1250,27 @@ struct phy_driver {
> >  	 * Returns the time in jiffies until the next update event.
> >  	 */
> >  	unsigned int (*get_next_update_time)(struct phy_device *dev);
> > +
> > +	/**
> > +	 * @attach_port: Indicates to the PHY driver that a port is detected
> > +	 * @dev: PHY device to notify
> > +	 * @port: The port being added
> > +	 *
> > +	 * Called when a port that needs to be driven by the PHY is found. The
> > +	 * number of time this will be called depends on phydev->max_n_ports,
> > +	 * which the driver can change in .probe().
> > +	 *
> > +	 * The port that is being passed may or may not be initialized. If it   
> is
> > +	 * already initialized, it is by the generic port representation from
> > +	 * devicetree, which superseeds any strapping or vendor-specific
> > +	 * properties.
> > +	 *
> > +	 * If the port isn't initialized, the port->mediums and port->lanes
> > +	 * fields must be set, possibly according to stapping information.
> > +	 *
> > +	 * Returns 0, or an error code.
> > +	 */
> > +	int (*attach_port)(struct phy_device *dev, struct phy_port *port);
> >  };
> >  #define to_phy_driver(d) container_of_const(to_mdio_common_driver(d),		  
> \
> >  				      struct phy_driver, mdiodrv)
> > @@ -1968,6 +1997,7 @@ void phy_trigger_machine(struct phy_device *phydev);
> >  void phy_mac_interrupt(struct phy_device *phydev);
> >  void phy_start_machine(struct phy_device *phydev);
> >  void phy_stop_machine(struct phy_device *phydev);
> > +
> >  void phy_ethtool_ksettings_get(struct phy_device *phydev,
> >  			       struct ethtool_link_ksettings *cmd);
> >  int phy_ethtool_ksettings_set(struct phy_device *phydev,
> > diff --git a/include/linux/phy_port.h b/include/linux/phy_port.h
> > new file mode 100644
> > index 000000000000..70aa75f93096
> > --- /dev/null
> > +++ b/include/linux/phy_port.h
> > @@ -0,0 +1,93 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef __PHY_PORT_H
> > +#define __PHY_PORT_H
> > +
> > +#include <linux/ethtool.h>
> > +#include <linux/types.h>
> > +#include <linux/phy.h>
> > +
> > +struct phy_port;
> > +
> > +/**
> > + * enum phy_port_parent - The device this port is attached to
> > + *
> > + * @PHY_PORT_PHY: Indicates that the port is driven by a PHY device
> > + */
> > +enum phy_port_parent {
> > +	PHY_PORT_PHY,
> > +};
> > +
> > +struct phy_port_ops {
> > +	/* Sometimes, the link state can be retrieved from physical,
> > +	 * out-of-band channels such as the LOS signal on SFP. These
> > +	 * callbacks allows notifying the port about state changes
> > +	 */
> > +	void (*link_up)(struct phy_port *port);
> > +	void (*link_down)(struct phy_port *port);
> > +
> > +	/* If the port acts as a Media Independent Interface (Serdes port),
> > +	 * configures the port with the relevant state and mode. When enable is
> > +	 * not set, interface should be ignored
> > +	 */
> > +	int (*configure_mii)(struct phy_port *port, bool enable,   
> phy_interface_t
> > interface); +};
> > +
> > +/**
> > + * struct phy_port - A representation of a network device physical
> > interface + *
> > + * @head: Used by the port's parent to list ports
> > + * @parent_type: The type of device this port is directly connected to
> > + * @phy: If the parent is PHY_PORT_PHYDEV, the PHY controlling that port
> > + * @ops: Callback ops implemented by the port controller
> > + * @lanes: The number of lanes (diff pairs) this port has, 0 if not
> > applicable + * @mediums: Bitmask of the physical mediums this port provides
> > access to + * @supported: The link modes this port can expose, if this port
> > is MDI (not MII) + * @interfaces: The MII interfaces this port supports, if
> > this port is MII + * @active: Indicates if the port is currently part of
> > the active link. + * @is_serdes: Indicates if this port is Serialised MII
> > (Media Independent + *	       Interface), or an MDI (Media Dependent
> > Interface).
> > + */
> > +struct phy_port {
> > +	struct list_head head;
> > +	enum phy_port_parent parent_type;
> > +	union {
> > +		struct phy_device *phy;
> > +	};
> > +
> > +	const struct phy_port_ops *ops;
> > +
> > +	int lanes;
> > +	unsigned long mediums;
> > +	__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> > +	DECLARE_PHY_INTERFACE_MASK(interfaces);
> > +
> > +	unsigned int active:1;
> > +	unsigned int is_serdes:1;
> > +};
> > +
> > +struct phy_port *phy_port_alloc(void);
> > +void phy_port_destroy(struct phy_port *port);
> > +
> > +static inline struct phy_device *port_phydev(struct phy_port *port)
> > +{
> > +	return port->phy;
> > +}
> > +
> > +struct phy_port *phy_of_parse_port(struct device_node *dn);
> > +
> > +static inline bool phy_port_is_copper(struct phy_port *port)
> > +{
> > +	return port->mediums == BIT(ETHTOOL_LINK_MEDIUM_BASET);  
> 
> BaseC is also "copper" right? Maybe this should be renamed to 
> "phy_port_is_tp"?

Yeah indeed...

Maxime

>