netdev - RE: [PATCH net-next v3] Add support of Cavium Liquidio ethernet adapters

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]

Date:	Fri, 19 Dec 2014 18:39:44 +0000
From:	"Chickles, Derek" <Derek.Chickles@...iumnetworks.com>
To:	Stephen Hemminger <stephen@...workplumber.org>,
	"Vatsavayi, Raghu" <Raghu.Vatsavayi@...iumnetworks.com>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Burla, Satananda" <Satananda.Burla@...iumnetworks.com>,
	"Manlunas, Felix" <Felix.Manlunas@...iumnetworks.com>,
	"Vatsavayi, Raghu" <Raghu.Vatsavayi@...iumnetworks.com>
Subject: RE: [PATCH net-next v3] Add support of Cavium Liquidio ethernet
 adapters

> > +
> > +static u32 lio_get_link(struct net_device *dev)
> > +{
> > +	u32 ret;
> > +
> > +	ret = netif_carrier_ok(dev) ? 1 : 0;
> > +	return ret;
> > +}
> > +
> 
> This is unnecessary, this is what the default ethtool handler already does
> if you give a NULL handler.

Okay

> > +	if (linfo->link.s.status) {
> > +		ecmd->speed = linfo->link.s.speed;
> > +		ecmd->duplex = linfo->link.s.duplex;
> > +	} else {
> > +		ecmd->speed = -1;
> > +		ecmd->duplex = -1;
> > +	}
> > +
> 
> You should SPEED_UNKNOWN and DUPLEX_UNKNOWN (not -1).
> and don't set ecmd->speed directly, use
>      ethtool_cmd_speed_sed(ecmd, speed)

Okay

> > +static void
> > +lio_get_ethtool_stats(struct net_device *netdev,
> > +		      struct ethtool_stats *stats, u64 *data)
> > +{
> > +	struct lio *lio = GET_LIO(netdev);
> > +	struct octeon_device *oct_dev = lio->oct_dev;
> > +	int i = 0, j;
> > +	
> > +	data[i++] = jiffies;
> > +	data[i++] = lio->stats.tx_packets;
> > +	data[i++] = lio->stats.tx_bytes;
> > +	data[i++] = lio->stats.rx_packets;
> > +	data[i++] = lio->stats.rx_bytes;
> > +	data[i++] = lio->stats.tx_errors;
> > +	data[i++] = lio->stats.tx_dropped;
> > +	data[i++] = lio->stats.rx_dropped;
> > +
> 
> Do not mirror existing netdevice statistics in ethtool.
> The purpose of ethtool stats is to provide device specific information.

Okay.

> > +/** \brief Network device get stats
> > + * @param netdev    pointer to network device
> > + * @returns pointer stats structure
> > + */
> > +static struct net_device_stats *liquidio_stats(struct net_device *netdev)
> > +{
> > +	return &(GET_LIO(netdev)->stats);
> > +}
> > +
> 
> Unnecessary function, this is what network core does already
> if .ndo_get_stats is NULL;

Okay.

> > +static int liquidio_ioctl(struct net_device *netdev, struct ifreq *ifr, int
> cmd)
> > +{
> > +	switch (cmd) {
> > +	case SIOCSHWTSTAMP:
> > +		return hwtstamp_ioctl(netdev, ifr, cmd);
> > +	default:
> > +		return -EINVAL
> 
> Standard error code for unsupported ioctl is -EOPNOTSUPP

Okay.

> > +static int __init liquidio_init(void)
> > +{
> > +	int i;
> > +	struct handshake *hs;
> > +
> > +	pr_info("LiquidIO: Starting Network module version %s\n",
> > +		LIQUIDIO_VERSION);
> > +
> 
> Do you really need to log this. Systems are already too chatty.

Okay. This is available with ethtool anyway.

> > +/**
> > + * \brief Alloc net device
> > + * @param size Size to allocate
> > + * @param nq how many queues
> > + * @returns pointer to net device structure
> > + */	
> > +static inline struct net_device *liquidio_alloc_netdev(int size, int nq)
> > +{
> > +	if (nq > 1)
> > +		return alloc_netdev_mq(size, "lio%d",
> NET_NAME_UNKNOWN,
> > +				       ether_setup, nq);
> > +	else
> > +		return alloc_netdev(size, "lio%d", NET_NAME_UNKNOWN,
> > +				    ether_setup);
> > +}
> 
> There is no need for the (nq > 1) test, just do:
> > +		return alloc_netdev_mq(size, "lio%d",
> NET_NAME_UNKNOWN,
> > +				       ether_setup, nq);
> 
Okay.

> Also, why does this device not have standard ethernet naming?

No good reason. We will follow the p<slot_number>p<port_number> convention.


> > +void process_noresponse_list(struct octeon_device *oct,
> > +			     struct octeon_instr_queue *iq)
> > +{
> > +	uint32_t get_idx;
> > +	struct octeon_soft_command *sc;
> > +	struct octeon_instr_ih *ih;
> 
> All global function names must start with same prefix, driver may not
> always be built as a module.
> 
> In this case oceteon_process_noresponse_list.

Okay. All global symbols will get an octeon_ or lio_ prefix.

We'll reply to the last couple issues separately.

Thanks for all the feedback.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html