netdev - Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute

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: <20190702104711.77618f6a@cakuba.netronome.com>
Date:   Tue, 2 Jul 2019 10:47:11 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Parav Pandit <parav@...lanox.com>
Cc:     Jiri Pirko <jiri@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and
 port attribute

On Tue, 2 Jul 2019 04:26:47 +0000, Parav Pandit wrote:
> > On Mon,  1 Jul 2019 07:27:32 -0500, Parav Pandit wrote:  
> > > In an eswitch, PCI PF may have port which is normally represented
> > > using a representor netdevice.
> > > To have better visibility of eswitch port, its association with PF, a
> > > representor netdevice and port number, introduce a PCI PF port flavour
> > > and port attriute.
> > >
> > > When devlink port flavour is PCI PF, fill up PCI PF attributes of the
> > > port.
> > >
> > > Extend port name creation using PCI PF number on best effort basis.
> > > So that vendor drivers can skip defining their own scheme.
> > >
> > > $ devlink port show
> > > pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
> > >
> > > Acked-by: Jiri Pirko <jiri@...lanox.com>
> > > Signed-off-by: Parav Pandit <parav@...lanox.com>
> > > diff --git a/include/net/devlink.h b/include/net/devlink.h index
> > > 6625ea068d5e..8db9c0e83fb5 100644
> > > --- a/include/net/devlink.h
> > > +++ b/include/net/devlink.h
> > > @@ -38,6 +38,10 @@ struct devlink {
> > >  	char priv[0] __aligned(NETDEV_ALIGN);  };
> > >
> > > +struct devlink_port_pci_pf_attrs {  
> > 
> > Why the named structure?  Anonymous one should be just fine?
> >  
> No specific reason for this patch. But named structure allows to
> extend it more easily with code readability. 

I'd argue the readability - I hove to scroll up/look up the structure
just to see it has a single member.  But no big deal :)

> Such as subsequently we want to add the peer_mac etc port attributes.
> Named structure to store those attributes are helpful. 

It remains to be seen if peer attributes are flavour specific 🤔
I'd imagine most port types would have some form of a peer (other
than a network port, perhaps).  But perhaps different peer attributes.

> > > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > > 89c533778135..001f9e2c96f0 100644
> > > --- a/net/core/devlink.c
> > > +++ b/net/core/devlink.c
> > > @@ -517,6 +517,11 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,  
> > >  		return -EMSGSIZE;
> > >  	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs->port_number))
> > >  		return -EMSGSIZE;  
> > 
> > Why would we report network port information for PF and VF port
> > flavours?  
>
> I didn't see any immediate need to report, at the same time didn't
> find any reason to treat such port flavours differently than existing
> one. It just gives a clear view of the device's eswitch. Might find
> it useful during debugging while inspecting device internal tables..

PFs and VFs ports are not tied to network ports in switchdev mode.
You have only one network port under a devlink instance AFAIR, anyway.

> > > +	if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF) {
> > > +		if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
> > > +				attrs->pci_pf.pf))
> > > +			return -EMSGSIZE;
> > > +	}
> > >  	if (!attrs->split)
> > >  		return 0;
> > >  	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP, attrs->port_number))