Re: [PATCH net-next v10 6/9] net: phy: phy_port: Store information about a MII port's vacant state

[Maxime Chevallier]

Hi again,

On 5/15/26 15:12, Andrew Lunn wrote:
> On Fri, May 15, 2026 at 09:22:29AM +0200, Maxime Chevallier wrote:
> > Hi Andrew,
> >
> > On 5/14/26 15:58, Andrew Lunn wrote:
> > > > We have information about what MII they can handle, however we don't
> > > > store anything about whether they are currently connected to an SFP
> > > > module or not. As phy_port aims at listing the front-facing ports, let's
> > > > store an "vacant" bit to know whether or not a MII port is currently
> > > > vacant (i.e. there's no module in the SFP cage), or not (i.e.
> > > > there's an SFP module).
> > >
> > > vacant == False actually means a bit more than there is a module in
> > > the cage. It also means that, so far, we have not had any errors
> > > trying to use the module. If there is an error, we might have a module
> > > physically in the cage, but vacant == True.
> >
> > This is a good point. Even with an error, we should have vacant = false.
> >
> > TBH I'm unsure how to name this, vacant may not be the best word there, but
> > I'm struggling to find a better term.
> >
> > maybe "empty" ?
>
> My comment is really about, what do we mean by vacant/empty?
>
> Is it simply about there physically being a module in the cage, or is
> it also about being able to drive that module? If it is just about
> there being a module in the cage, then vacant/empty is good. If we
> also want to say that we have been able to read out the EEPROM and
> determined the module will work in combination with the PCS and MAC,
> then i would probably use a different name. "usable", "compatible"?
>
> Either way, i think the code either needs simplifying, or made more
> complex.

I've given this a go, and now I remember why it's that way. In a 
nutshell, if something fails during .module_insert() (e.g. module isn't 
compatible with the MII modes ont the cage), then .module_remove() isn't 
called, so we can't track module presence unless we add that in the sfp 
code itself.

However a better way to deal with this whole situation may just be to 
store the presence information in the module's port itself, as the 
parent port id :
                   phy_port.id = 1
                  /                   phy_port.id = 2;
                 /                  / phy_port.upstream_port = 1
+------+     +----------+     +------------+
| MAC  | --- | SFP Cage | --- | SFP module |
+------+     +----------+     +------------+

So, we lose one piece of information, that is "is there an SFP module 
present, even if the module doesn't work". It's now indirectly present
as "there's another port that references the SFP cage port, hence it's
not empty".

We gain however information about the topology, and this is relevant for 
combo-port support :


                   phy_port.id = 1
                  /                   phy_port.id = 3;
                 /                  / phy_port.upstream_port = 1
+------+     +----------+     +------------+
| PHY  | --- | SFP Cage | --- | SFP module |
|      | -   +----------+     +------------+
+------+  \
           \   +------+
            -- | RJ45 |
               +------+
                 \
                  phy_port.id = 2

If we want to allow userspace to select which port to use, the following
commands will be equivalent (the command itself isn't set in stone, just 
a rough idea of what it'll look like) :

# Force using SFP

  ethtool --set-port eth0 port 1 force-active on
and
  ethtool --set-port eth0 port 3 force-active on

These commands above should be exactly equivalent

As they act on ports, having the relationship "port 3 is actually 
controlled through port 1" readily available will ease the implementation.

If that's ok, add this change to the next iteration (only the GET part, 
not yet the port selection :) )

Maxime