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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AM0PR05MB4866C19C9E6ED767A44C3064D1F80@AM0PR05MB4866.eurprd05.prod.outlook.com>
Date:   Tue, 2 Jul 2019 18:50:31 +0000
From:   Parav Pandit <parav@...lanox.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.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



> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Sent: Tuesday, July 2, 2019 11:17 PM
> To: Parav Pandit <parav@...lanox.com>
> Cc: Jiri Pirko <jiri@...lanox.com>; 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 :)
> 
Ok. :-)

> > 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.
>
Few attributes may be common and few will be port specific.
So as it evolves, data structure will evolve.
Common attribute I can think of is - mac address.
 
> > > > 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.
> 
I am not sure what do you mean by network port.
Do you intent to see a physical port that connects to physical network?

As I described in the comment of the PF and VF flavour, it is an eswitch port.
I have shown the diagram also of the eswitch in the cover letter.
Port_number doesn't have to a physical port. Flavour describe what port type is and number says what is the eswitch port number.
Hope it clarifies.

> > > > +	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))