netdev - Re: [PATCH V3 net-next 1/2] liquidio: switchdev support for LiquidIO NIC

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: <20171101192948.GA21476@felix-thinkpad.cavium.com>
Date:   Wed, 1 Nov 2017 12:29:48 -0700
From:   Felix Manlunas <felix.manlunas@...ium.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        raghu.vatsavayi@...ium.com, derek.chickles@...ium.com,
        satananda.burla@...ium.com, vijaya.guvva@...ium.com
Subject: Re: [PATCH V3 net-next 1/2] liquidio: switchdev support for LiquidIO
 NIC

On Wed, Nov 01, 2017 at 02:02:07PM +0100, Andrew Lunn wrote:
> > +static int
> > +lio_pf_switchdev_attr_get(struct net_device *dev, struct switchdev_attr *attr)
> > +{
> > +	struct lio *lio = GET_LIO(dev);
> > +
> > +	switch (attr->id) {
> > +	case SWITCHDEV_ATTR_ID_PORT_PARENT_ID:
> > +		attr->u.ppid.id_len = ETH_ALEN;
> > +		ether_addr_copy(attr->u.ppid.id,
> > +				(void *)&lio->linfo.hw_addr + 2);
> 
> The + 2 seems odd. Please could you explain why it is there?

The "+ 2" is the byte offset of the start of the mac address.  In a
future patch, we will replace "(void *)&lio->linfo.hw_addr + 2" with
"dev->dev_addr" which is an elegant equivalent.


> > +static int lio_vf_rep_open(struct net_device *ndev);
> > +static int lio_vf_rep_stop(struct net_device *ndev);
> > +static int lio_vf_rep_pkt_xmit(struct sk_buff *skb, struct net_device *ndev);
> > +static void lio_vf_rep_tx_timeout(struct net_device *netdev);
> > +static int lio_vf_rep_phys_port_name(struct net_device *dev,
> > +				     char *buf, size_t len);
> > +static void lio_vf_rep_get_stats64(struct net_device *dev,
> > +				   struct rtnl_link_stats64 *stats64);
> > +static int lio_vf_rep_change_mtu(struct net_device *ndev, int new_mtu);
> > +
> > +static const struct net_device_ops lio_vf_rep_ndev_ops = {
> > +	.ndo_open = lio_vf_rep_open,
> > +	.ndo_stop = lio_vf_rep_stop,
> > +	.ndo_start_xmit = lio_vf_rep_pkt_xmit,
> > +	.ndo_tx_timeout = lio_vf_rep_tx_timeout,
> > +	.ndo_get_phys_port_name = lio_vf_rep_phys_port_name,
> > +	.ndo_get_stats64 = lio_vf_rep_get_stats64,
> > +	.ndo_change_mtu = lio_vf_rep_change_mtu,
> > +};
> 
> Please don't use forward references. Change the order of the code and
> put this structure towards the end of the file.

We will fix this in a future patch.


> > +lio_vf_rep_phys_port_name(struct net_device *dev,
> > +			  char *buf, size_t len)
> > +{
> > +	struct lio_vf_rep_desc *vf_rep = netdev_priv(dev);
> > +	struct octeon_device *oct = vf_rep->oct;
> > +	int ret;
> > +
> > +	ret = snprintf(buf, len, "pf%dvf%d", oct->pf_num,
> > +		       vf_rep->ifidx - oct->pf_num * 64 - 1);
> > +	if (ret >= len)
> > +		return -EOPNOTSUPP;
> 
> EOPNOTSUPP seems an odd return code for too short a buffer?

We will replace that with ENOBUFS in a future patch.