diff mbox series

[net-next,v2,1/7] net: devlink: Add unused port flavour

Message ID 20200926210632.3888886-2-andrew@lunn.ch
State Changes Requested
Delegated to: David Miller
Headers show
Series mv88e6xxx: Add per port devlink regions | expand

Commit Message

Andrew Lunn Sept. 26, 2020, 9:06 p.m. UTC
Not all ports of a switch need to be used, particularly in embedded
systems. Add a port flavour for ports which physically exist in the
switch, but are not connected to the front panel etc, and so are
unused.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/uapi/linux/devlink.h | 3 +++
 net/core/devlink.c           | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Vladimir Oltean Sept. 26, 2020, 9:51 p.m. UTC | #1
On Sat, Sep 26, 2020 at 11:06:26PM +0200, Andrew Lunn wrote:
> Not all ports of a switch need to be used, particularly in embedded
> systems. Add a port flavour for ports which physically exist in the
> switch, but are not connected to the front panel etc, and so are
> unused.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  include/uapi/linux/devlink.h | 3 +++
>  net/core/devlink.c           | 4 +++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index a2ecc8b00611..e1f209feac74 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -195,6 +195,9 @@ enum devlink_port_flavour {
>  				      * port that faces the PCI VF.
>  				      */
>  	DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
> +	DEVLINK_PORT_FLAVOUR_UNUSED, /* Port which exists in the switch, but
> +				      *	is not used in any way.

Nitpicking: there is an extraneous tab character here between "*" and "is".

> +				      */
>  };
>  
>  enum devlink_param_cmode {
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index ac32b672a04b..fc9589eb4115 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -7569,7 +7569,8 @@ static bool devlink_port_type_should_warn(struct devlink_port *devlink_port)
>  {
>  	/* Ignore CPU and DSA flavours. */
>  	return devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_CPU &&
> -	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA;
> +	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA &&
> +	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_UNUSED;
>  }
>  
>  #define DEVLINK_PORT_TYPE_WARN_TIMEOUT (HZ * 3600)
> @@ -7854,6 +7855,7 @@ static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>  		break;
>  	case DEVLINK_PORT_FLAVOUR_CPU:
>  	case DEVLINK_PORT_FLAVOUR_DSA:
> +	case DEVLINK_PORT_FLAVOUR_UNUSED:
>  		/* As CPU and DSA ports do not have a netdevice associated
>  		 * case should not ever happen.
>  		 */
> -- 
> 2.28.0
>
Vladimir Oltean Sept. 26, 2020, 10 p.m. UTC | #2
On Sat, Sep 26, 2020 at 11:06:26PM +0200, Andrew Lunn wrote:
> Not all ports of a switch need to be used, particularly in embedded
> systems. Add a port flavour for ports which physically exist in the
> switch, but are not connected to the front panel etc, and so are
> unused.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---

You also have an iproute2 patch prepared, right?

$ devlink port
spi/spi0.1/0: type notset flavour <unknown flavour>  <- this should read "unused"
spi/spi0.1/1: type eth netdev swp2 flavour physical port 1
spi/spi0.1/2: type eth netdev swp3 flavour physical port 2
spi/spi0.1/3: type eth netdev swp4 flavour physical port 3
spi/spi0.1/4: type notset flavour cpu port 4
Andrew Lunn Sept. 27, 2020, 12:33 a.m. UTC | #3
On Sun, Sep 27, 2020 at 01:00:34AM +0300, Vladimir Oltean wrote:
> On Sat, Sep 26, 2020 at 11:06:26PM +0200, Andrew Lunn wrote:
> > Not all ports of a switch need to be used, particularly in embedded
> > systems. Add a port flavour for ports which physically exist in the
> > switch, but are not connected to the front panel etc, and so are
> > unused.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> 
> You also have an iproute2 patch prepared, right?

https://github.com/lunn/iproute2 port-regions

	 Andrew
Jakub Kicinski Sept. 28, 2020, 9:31 p.m. UTC | #4
On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
> Not all ports of a switch need to be used, particularly in embedded
> systems. Add a port flavour for ports which physically exist in the
> switch, but are not connected to the front panel etc, and so are
> unused.

This is missing the explanation of why reporting such ports makes sense.
Vladimir Oltean Sept. 28, 2020, 10:05 p.m. UTC | #5
On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
> > Not all ports of a switch need to be used, particularly in embedded
> > systems. Add a port flavour for ports which physically exist in the
> > switch, but are not connected to the front panel etc, and so are
> > unused.
>
> This is missing the explanation of why reporting such ports makes sense.

Because this is a core devlink patch, we're talking really generalistic
here. And since devlink regions are a debugging features, it makes sense
to tell a debugging story. Let's say there is a switch which had
configured all its ports to be part of the flooding replication lists,
but also configured other things incorrectly such that attempting to
flood a frame to a disabled port would leak a memory buffer. After
enough time, the system would lock up. So unused ports are not absent
from the system and they might even make a difference if the procedure
to disable a port is buggy (and there would be no debugging without
bugs, right?). I see no reason why we would hide them. Devlink ports are
not net devices, I thought that was the whole point, to have insight
into the hardware and not have to deal with an approximate abstraction.
Andrew Lunn Sept. 28, 2020, 10:07 p.m. UTC | #6
On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
> > On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
> > > Not all ports of a switch need to be used, particularly in embedded
> > > systems. Add a port flavour for ports which physically exist in the
> > > switch, but are not connected to the front panel etc, and so are
> > > unused.
> >
> > This is missing the explanation of why reporting such ports makes sense.
> 
> Because this is a core devlink patch, we're talking really generalistic
> here.

Hi Vladimir

I don't think Jakub is questioning the why. He just wants it in the
commit message.

       Andrew
Jakub Kicinski Sept. 28, 2020, 10:35 p.m. UTC | #7
On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:
> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
> > On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:  
> > > On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:  
> > > > Not all ports of a switch need to be used, particularly in embedded
> > > > systems. Add a port flavour for ports which physically exist in the
> > > > switch, but are not connected to the front panel etc, and so are
> > > > unused.  
> > >
> > > This is missing the explanation of why reporting such ports makes sense.  
> > 
> > Because this is a core devlink patch, we're talking really generalistic
> > here.  
> 
> Hi Vladimir
> 
> I don't think Jakub is questioning the why. He just wants it in the
> commit message.

Ack, I think we need to clearly say when those should be exposed. 
Most ASICs will have disabled ports, and we don't expect NICs to
suddenly start reporting ports for all PCI PFs they may have.

Also I keep thinking that these ports and all their objects should 
be hidden under some switch from user space perspective because they 
are unlikely to be valuable to see for a normal user. Thoughts?
Florian Fainelli Sept. 28, 2020, 10:36 p.m. UTC | #8
On 9/28/2020 3:35 PM, Jakub Kicinski wrote:
> On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:
>> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
>>> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
>>>> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
>>>>> Not all ports of a switch need to be used, particularly in embedded
>>>>> systems. Add a port flavour for ports which physically exist in the
>>>>> switch, but are not connected to the front panel etc, and so are
>>>>> unused.
>>>>
>>>> This is missing the explanation of why reporting such ports makes sense.
>>>
>>> Because this is a core devlink patch, we're talking really generalistic
>>> here.
>>
>> Hi Vladimir
>>
>> I don't think Jakub is questioning the why. He just wants it in the
>> commit message.
> 
> Ack, I think we need to clearly say when those should be exposed.
> Most ASICs will have disabled ports, and we don't expect NICs to
> suddenly start reporting ports for all PCI PFs they may have.
> 
> Also I keep thinking that these ports and all their objects should
> be hidden under some switch from user space perspective because they
> are unlikely to be valuable to see for a normal user. Thoughts?

Hidden in what sense? They are already hidden in that there is no 
net_device object being created for them. Are you asking for adding 
another option to say, devlink show like:

devlink show -a

which would also show the ports that are disabled during a dump?
Jakub Kicinski Sept. 28, 2020, 11:39 p.m. UTC | #9
On Mon, 28 Sep 2020 15:36:50 -0700 Florian Fainelli wrote:
> On 9/28/2020 3:35 PM, Jakub Kicinski wrote:
> > On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:  
> >> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:  
> >>> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:  
> >>>> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:  
> >>>>> Not all ports of a switch need to be used, particularly in embedded
> >>>>> systems. Add a port flavour for ports which physically exist in the
> >>>>> switch, but are not connected to the front panel etc, and so are
> >>>>> unused.  
> >>>>
> >>>> This is missing the explanation of why reporting such ports makes sense.  
> >>>
> >>> Because this is a core devlink patch, we're talking really generalistic
> >>> here.  
> >>
> >> Hi Vladimir
> >>
> >> I don't think Jakub is questioning the why. He just wants it in the
> >> commit message.  
> > 
> > Ack, I think we need to clearly say when those should be exposed.
> > Most ASICs will have disabled ports, and we don't expect NICs to
> > suddenly start reporting ports for all PCI PFs they may have.
> > 
> > Also I keep thinking that these ports and all their objects should
> > be hidden under some switch from user space perspective because they
> > are unlikely to be valuable to see for a normal user. Thoughts?  
> 
> Hidden in what sense? They are already hidden in that there is no 
> net_device object being created for them. Are you asking for adding 
> another option to say, devlink show like:
> 
> devlink show -a
> 
> which would also show the ports that are disabled during a dump?

Yup, exactly. Looks like ip uses -a for something I don't quite
understand - but some switch along those lines. We already have 
-d for hiding less-relevant attributes.

Do you think this is an overkill? I don't feel strongly.
Florian Fainelli Sept. 29, 2020, 1:46 a.m. UTC | #10
On 9/28/2020 4:39 PM, Jakub Kicinski wrote:
> On Mon, 28 Sep 2020 15:36:50 -0700 Florian Fainelli wrote:
>> On 9/28/2020 3:35 PM, Jakub Kicinski wrote:
>>> On Tue, 29 Sep 2020 00:07:30 +0200 Andrew Lunn wrote:
>>>> On Mon, Sep 28, 2020 at 10:05:08PM +0000, Vladimir Oltean wrote:
>>>>> On Mon, Sep 28, 2020 at 02:31:55PM -0700, Jakub Kicinski wrote:
>>>>>> On Sat, 26 Sep 2020 23:06:26 +0200 Andrew Lunn wrote:
>>>>>>> Not all ports of a switch need to be used, particularly in embedded
>>>>>>> systems. Add a port flavour for ports which physically exist in the
>>>>>>> switch, but are not connected to the front panel etc, and so are
>>>>>>> unused.
>>>>>>
>>>>>> This is missing the explanation of why reporting such ports makes sense.
>>>>>
>>>>> Because this is a core devlink patch, we're talking really generalistic
>>>>> here.
>>>>
>>>> Hi Vladimir
>>>>
>>>> I don't think Jakub is questioning the why. He just wants it in the
>>>> commit message.
>>>
>>> Ack, I think we need to clearly say when those should be exposed.
>>> Most ASICs will have disabled ports, and we don't expect NICs to
>>> suddenly start reporting ports for all PCI PFs they may have.
>>>
>>> Also I keep thinking that these ports and all their objects should
>>> be hidden under some switch from user space perspective because they
>>> are unlikely to be valuable to see for a normal user. Thoughts?
>>
>> Hidden in what sense? They are already hidden in that there is no
>> net_device object being created for them. Are you asking for adding
>> another option to say, devlink show like:
>>
>> devlink show -a
>>
>> which would also show the ports that are disabled during a dump?
> 
> Yup, exactly. Looks like ip uses -a for something I don't quite
> understand - but some switch along those lines. We already have
> -d for hiding less-relevant attributes.
> 
> Do you think this is an overkill? I don't feel strongly.

That makes sense to me as it would be confusing to suddenly show unused 
port flavors after this patch series land. Andrew, Vladimir, does that 
work for you as well?
Vladimir Oltean Sept. 29, 2020, 11:03 a.m. UTC | #11
On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
> That makes sense to me as it would be confusing to suddenly show unused port
> flavors after this patch series land. Andrew, Vladimir, does that work for
> you as well?

I have nothing to object against somebody adding a '--all' argument to
devlink port commands.
Jiri Pirko Sept. 29, 2020, 1:07 p.m. UTC | #12
Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
>On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
>> That makes sense to me as it would be confusing to suddenly show unused port
>> flavors after this patch series land. Andrew, Vladimir, does that work for
>> you as well?
>
>I have nothing to object against somebody adding a '--all' argument to
>devlink port commands.

How "unused" is a "flavour"? It seems to me more like a separate
attribute as port of any "flavour" may be potentially "unused". I don't
think we should mix these 2.
Vladimir Oltean Sept. 29, 2020, 1:48 p.m. UTC | #13
On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
> >> That makes sense to me as it would be confusing to suddenly show unused port
> >> flavors after this patch series land. Andrew, Vladimir, does that work for
> >> you as well?
> >
> >I have nothing to object against somebody adding a '--all' argument to
> >devlink port commands.
> 
> How "unused" is a "flavour"? It seems to me more like a separate
> attribute as port of any "flavour" may be potentially "unused". I don't
> think we should mix these 2.
> 

I guess it's you who suggested it might make sense to add an "unused"
port flavour?

> Okay. That looks fine. I wonder if it would make sense to have another
> flavour for "unused" ports.

https://patchwork.ozlabs.org/project/netdev/cover/20180322105522.8186-1-jiri@resnulli.us/

Thanks,
-Vladimir
Andrew Lunn Sept. 29, 2020, 1:57 p.m. UTC | #14
On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
> >> That makes sense to me as it would be confusing to suddenly show unused port
> >> flavors after this patch series land. Andrew, Vladimir, does that work for
> >> you as well?
> >
> >I have nothing to object against somebody adding a '--all' argument to
> >devlink port commands.
> 
> How "unused" is a "flavour"? It seems to me more like a separate
> attribute as port of any "flavour" may be potentially "unused". I don't
> think we should mix these 2.

Hi Jiri

Current flavours are:

enum devlink_port_flavour {
        DEVLINK_PORT_FLAVOUR_PHYSICAL, /* Any kind of a port physically
                                        * facing the user.
                                        */
        DEVLINK_PORT_FLAVOUR_CPU, /* CPU port */
        DEVLINK_PORT_FLAVOUR_DSA, /* Distributed switch architecture
                                   * interconnect port.
                                   */
        DEVLINK_PORT_FLAVOUR_PCI_PF, /* Represents eswitch port for
                                      * the PCI PF. It is an internal
                                      * port that faces the PCI PF.
                                      */
        DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port
                                      * for the PCI VF. It is an internal
                                      * port that faces the PCI VF.
                                      */
        DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
};

A port in the DSA world is generally just a port on the switch. It is
not limited in nature. It can be used as a PHYSICAL, or CPU, or a DSA
port. We don't consider them as unused PHYISCAL ports, or unused CPU
ports. They are just unused ports. Which is why i added an UNUSED
flavour.

Now, it could be this world view is actually just a DSA world
view. Maybe some ASICs have strict roles for their ports? They are not
configurable? And then i could see it being an attribute? But that
messes up the DSA world view :-(

      Andrew
Jiri Pirko Sept. 29, 2020, 3:12 p.m. UTC | #15
Tue, Sep 29, 2020 at 03:48:56PM CEST, vladimir.oltean@nxp.com wrote:
>On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
>> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
>> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
>> >> That makes sense to me as it would be confusing to suddenly show unused port
>> >> flavors after this patch series land. Andrew, Vladimir, does that work for
>> >> you as well?
>> >
>> >I have nothing to object against somebody adding a '--all' argument to
>> >devlink port commands.
>> 
>> How "unused" is a "flavour"? It seems to me more like a separate
>> attribute as port of any "flavour" may be potentially "unused". I don't
>> think we should mix these 2.
>> 
>
>I guess it's you who suggested it might make sense to add an "unused"
>port flavour?

Maybe, but that was just an idea. 2 years later, I don't think it is a
good idea.

>
>> Okay. That looks fine. I wonder if it would make sense to have another
>> flavour for "unused" ports.
>
>https://patchwork.ozlabs.org/project/netdev/cover/20180322105522.8186-1-jiri@resnulli.us/
>
>Thanks,
>-Vladimir
Jiri Pirko Sept. 30, 2020, 6:56 a.m. UTC | #16
Tue, Sep 29, 2020 at 03:57:00PM CEST, andrew@lunn.ch wrote:
>On Tue, Sep 29, 2020 at 03:07:58PM +0200, Jiri Pirko wrote:
>> Tue, Sep 29, 2020 at 01:03:56PM CEST, vladimir.oltean@nxp.com wrote:
>> >On Mon, Sep 28, 2020 at 06:46:14PM -0700, Florian Fainelli wrote:
>> >> That makes sense to me as it would be confusing to suddenly show unused port
>> >> flavors after this patch series land. Andrew, Vladimir, does that work for
>> >> you as well?
>> >
>> >I have nothing to object against somebody adding a '--all' argument to
>> >devlink port commands.
>> 
>> How "unused" is a "flavour"? It seems to me more like a separate
>> attribute as port of any "flavour" may be potentially "unused". I don't
>> think we should mix these 2.
>
>Hi Jiri
>
>Current flavours are:
>
>enum devlink_port_flavour {
>        DEVLINK_PORT_FLAVOUR_PHYSICAL, /* Any kind of a port physically
>                                        * facing the user.
>                                        */
>        DEVLINK_PORT_FLAVOUR_CPU, /* CPU port */
>        DEVLINK_PORT_FLAVOUR_DSA, /* Distributed switch architecture
>                                   * interconnect port.
>                                   */
>        DEVLINK_PORT_FLAVOUR_PCI_PF, /* Represents eswitch port for
>                                      * the PCI PF. It is an internal
>                                      * port that faces the PCI PF.
>                                      */
>        DEVLINK_PORT_FLAVOUR_PCI_VF, /* Represents eswitch port
>                                      * for the PCI VF. It is an internal
>                                      * port that faces the PCI VF.
>                                      */
>        DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
>};
>
>A port in the DSA world is generally just a port on the switch. It is
>not limited in nature. It can be used as a PHYSICAL, or CPU, or a DSA
>port. We don't consider them as unused PHYISCAL ports, or unused CPU
>ports. They are just unused ports. Which is why i added an UNUSED
>flavour.

I get it. But I as I wrote previously, I wonder if used/unused should
not be another attribute. Then the flavour can be "undefined".

But, why do you want to show "unused" ports? Can the user do something
with them? What is the value in showing them?



>
>Now, it could be this world view is actually just a DSA world
>view. Maybe some ASICs have strict roles for their ports? They are not
>configurable? And then i could see it being an attribute? But that
>messes up the DSA world view :-(
>
>      Andrew
Andrew Lunn Sept. 30, 2020, 1:57 p.m. UTC | #17
> I get it. But I as I wrote previously, I wonder if used/unused should
> not be another attribute. Then the flavour can be "undefined".

In the DSA world, it is not undefined. It is clear defined as
unused. And it cannot be on-the-fly changed. It is a property of the
PCB, in that the pins exist on the chip, but they simply don't go
anywhere on the PCB. This is quite common on appliances, e.g. The
switch has 7 ports, but the installation in the aircraft is a big
ring, so there is a 'left', 'right', 'aux' and the CPU port. That
leaves 3 ports totally unused.

> But, why do you want to show "unused" ports? Can the user do something
> with them? What is the value in showing them?

Because they are just ports, they can have regions. We can look at the
region and be sure they are powered off, the boot loader etc has not
left them in a funny state, bridged to other ports, etc.

Regions are a developers tool, not a 'user' tools. So the idea of
hiding them by default in 'devlink port show' does make some sense,
and have a flag like -d for details, which includes them. In 'devlink
region show' i would probably list all regions, independent of any -d
flag.

      Andrew
Jiri Pirko Sept. 30, 2020, 2:34 p.m. UTC | #18
Wed, Sep 30, 2020 at 03:57:25PM CEST, andrew@lunn.ch wrote:
>> I get it. But I as I wrote previously, I wonder if used/unused should
>> not be another attribute. Then the flavour can be "undefined".
>
>In the DSA world, it is not undefined. It is clear defined as
>unused. And it cannot be on-the-fly changed. It is a property of the
>PCB, in that the pins exist on the chip, but they simply don't go
>anywhere on the PCB. This is quite common on appliances, e.g. The
>switch has 7 ports, but the installation in the aircraft is a big
>ring, so there is a 'left', 'right', 'aux' and the CPU port. That
>leaves 3 ports totally unused.

Understand the DSA usecase.


>
>> But, why do you want to show "unused" ports? Can the user do something
>> with them? What is the value in showing them?
>
>Because they are just ports, they can have regions. We can look at the

What do you mean by "regions"? Devlink regions? They are per-device, not
per-port. I have to be missing something.


>region and be sure they are powered off, the boot loader etc has not
>left them in a funny state, bridged to other ports, etc.

It is driver's responsibility to ensure that. But that does not mean
that the devlink port needs to be visible.


>
>Regions are a developers tool, not a 'user' tools. So the idea of
>hiding them by default in 'devlink port show' does make some sense,
>and have a flag like -d for details, which includes them. In 'devlink
>region show' i would probably list all regions, independent of any -d
>flag.
>
>      Andrew
Andrew Lunn Sept. 30, 2020, 2:53 p.m. UTC | #19
> What do you mean by "regions"? Devlink regions? They are per-device, not
> per-port. I have to be missing something.

The rest of the patch series, which add regions per port! This came
out of the discussion from the first version of this patchset, and
Jakub said it would make sense to add per port regions, rather than
have regions which embedded a port number in there name.

     Andrew
Jiri Pirko Oct. 1, 2020, 7:39 a.m. UTC | #20
Wed, Sep 30, 2020 at 04:53:57PM CEST, andrew@lunn.ch wrote:
>> What do you mean by "regions"? Devlink regions? They are per-device, not
>> per-port. I have to be missing something.
>
>The rest of the patch series, which add regions per port! This came

Okay. Sorry about that. netdev ml kicked me out so I didn't receive
emails from it for couple of days :/


>out of the discussion from the first version of this patchset, and
>Jakub said it would make sense to add per port regions, rather than
>have regions which embedded a port number in there name.

For sure. If something is per-port, should be per-port. I agree.

>
>     Andrew
>
diff mbox series

Patch

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index a2ecc8b00611..e1f209feac74 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -195,6 +195,9 @@  enum devlink_port_flavour {
 				      * port that faces the PCI VF.
 				      */
 	DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
+	DEVLINK_PORT_FLAVOUR_UNUSED, /* Port which exists in the switch, but
+				      *	is not used in any way.
+				      */
 };
 
 enum devlink_param_cmode {
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ac32b672a04b..fc9589eb4115 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -7569,7 +7569,8 @@  static bool devlink_port_type_should_warn(struct devlink_port *devlink_port)
 {
 	/* Ignore CPU and DSA flavours. */
 	return devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_CPU &&
-	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA;
+	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_DSA &&
+	       devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_UNUSED;
 }
 
 #define DEVLINK_PORT_TYPE_WARN_TIMEOUT (HZ * 3600)
@@ -7854,6 +7855,7 @@  static int __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
 		break;
 	case DEVLINK_PORT_FLAVOUR_CPU:
 	case DEVLINK_PORT_FLAVOUR_DSA:
+	case DEVLINK_PORT_FLAVOUR_UNUSED:
 		/* As CPU and DSA ports do not have a netdevice associated
 		 * case should not ever happen.
 		 */