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

[Parav Pandit <parav@mellanox.com>]

> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Saturday, July 6, 2019 12:47 AM
> To: Parav Pandit <parav@mellanox.com>
> Cc: netdev@vger.kernel.org; Jiri Pirko <jiri@mellanox.com>; Saeed Mahameed
> <saeedm@mellanox.com>
> Subject: Re: [PATCH net-next v2 1/3] devlink: Introduce PCI PF port flavour and
> port attribute
> 
> On Fri,  5 Jul 2019 02:37:09 -0500, Parav Pandit wrote:
> > @@ -38,14 +38,24 @@ struct devlink {
> >  	char priv[0] __aligned(NETDEV_ALIGN);  };
> >
> > +struct devlink_port_pci_pf_attrs {
> > +	u16 pf;	/* Associated PCI PF for this port. */
> > +};
> > +
> >  struct devlink_port_attrs {
> >  	u8 set:1,
> >  	   split:1,
> >  	   switch_port:1;
> >  	enum devlink_port_flavour flavour;
> > -	u32 port_number; /* same value as "split group" */
> > +	u32 port_number; /* same value as "split group".
> > +			  * Valid only when a port is physical and visible
> > +			  * to the user for a given port flavour.
> > +			  */
> 
> port_number can be in the per-flavour union below.
> 
Ack.
> >  	u32 split_subport_number;
> 
> As can split_subport_number.
> 
Ack.

> >  	struct netdev_phys_item_id switch_id;
> > +	union {
> > +		struct devlink_port_pci_pf_attrs pci_pf;
> > +	};
> >  };
> >
> >  struct devlink_port {
> 
> > @@ -515,8 +523,14 @@ static int devlink_nl_port_attrs_put(struct sk_buff
> *msg,
> >  		return 0;
> >  	if (nla_put_u16(msg, DEVLINK_ATTR_PORT_FLAVOUR, attrs->flavour))
> >  		return -EMSGSIZE;
> > -	if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
> >port_number))
> > +	if (is_devlink_phy_port_num_supported(devlink_port) &&
> > +	    nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
> >port_number))
> >  		return -EMSGSIZE;
> > +	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))
> 
> Split attributes as well, please:
> 
Ack.
> On Tue, 2 Jul 2019 16:42:52 -0700, Jakub Kicinski wrote:
> > port_number, and split attributes should not be exposed for PCI ports.
> 
> We have no clear semantics for those, yet, and the phys_port_name
> implementation in this patch doesn't handle split PCI, so let's leave them out
> for now.
Ok. Sending v3.
Thanks for the review.