netdev - RE: [PATCH net-next v2] Add Mellanox BlueField Gigabit Ethernet driver

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: <MN2PR12MB2975DAA7292C27DEB0B518A8C75A0@MN2PR12MB2975.namprd12.prod.outlook.com>
Date:   Thu, 20 Aug 2020 18:51:39 +0000
From:   David Thompson <davthompson@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        David Thompson <dthompson@...lanox.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Jiri Pirko <jiri@...lanox.com>,
        Asmaa Mnebhi <Asmaa@...lanox.com>
Subject: RE: [PATCH net-next v2] Add Mellanox BlueField Gigabit Ethernet
 driver

> > +config MLXBF_GIGE
> > +	tristate "Mellanox Technologies BlueField Gigabit Ethernet support"
> > +	depends on (ARM64 || COMPILE_TEST) && ACPI && INET
> 
> Why do you depend on INET? :S
> 

When I wrote the Kconfig definition I was thinking that "INET" is an
obvious functional dependency for an Ethernet driver.  However, if
Kconfig is just intended to express build-time dependencies, then yes,
the "INET" keyword can be removed.

> > +	for (i = 0; i < priv->rx_q_entries; i++) {
> > +		/* Allocate a receive buffer for this RX WQE. The DMA
> > +		 * form (dma_addr_t) of the receive buffer address is
> > +		 * stored in the RX WQE array (via 'rx_wqe_ptr') where
> > +		 * it is accessible by the GigE device. The VA form of
> > +		 * the receive buffer is stored in 'rx_buf[]' array in
> > +		 * the driver private storage for housekeeping.
> > +		 */
> > +		priv->rx_buf[i] = dma_alloc_coherent(priv->dev,
> > +
> MLXBF_GIGE_DEFAULT_BUF_SZ,
> > +						     &rx_buf_dma,
> > +						     GFP_KERNEL);
> 
> Do the buffers have to be in coherent memory? That's kinda strange.
> 

Yes, the mlxbf_gige silicon block needs to be programmed with the
buffer's physical address so that the silicon logic can DMA incoming
packet data into the buffer.  The kernel API "dma_alloc_coherent()"
meets the driver's requirements in that it returns a CPU-useable address
as well as a bus/physical address (used by silicon).

> > +static void mlxbf_gige_get_ethtool_stats(struct net_device *netdev,
> > +					 struct ethtool_stats *estats,
> > +					 u64 *data)
> > +{
> > +	struct mlxbf_gige *priv = netdev_priv(netdev);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> 
> Why do you take a lock around stats?
> 

I wrote the logic with a lock so that it implements an atomic "snapshot"
of the driver's statistics.  This is useful since the standard TX/RX stats
are being incremented in packet completion logic triggered by the 
NAPI framework.  Do you see a disadvantage to using a lock here?

> > +	/* Fill data array with interface statistics
> > +	 *
> > +	 * NOTE: the data writes must be in
> > +	 *       sync with the strings shown in
> > +	 *       the mlxbf_gige_ethtool_stats_keys[] array
> > +	 *
> > +	 * NOTE2: certain statistics below are zeroed upon
> > +	 *        port disable, so the calculation below
> > +	 *        must include the "cached" value of the stat
> > +	 *        plus the value read directly from hardware.
> > +	 *        Cached statistics are currently:
> > +	 *          rx_din_dropped_pkts
> > +	 *          rx_filter_passed_pkts
> > +	 *          rx_filter_discard_pkts
> > +	 */
> > +	*data++ = netdev->stats.rx_bytes;
> > +	*data++ = netdev->stats.rx_packets;
> > +	*data++ = netdev->stats.tx_bytes;
> > +	*data++ = netdev->stats.tx_packets;
> 
> Please don't duplicate standard stats in ethtool.
> 

Understood.

> > +static const struct net_device_ops mlxbf_gige_netdev_ops = {
> > +	.ndo_open		= mlxbf_gige_open,
> > +	.ndo_stop		= mlxbf_gige_stop,
> > +	.ndo_start_xmit		= mlxbf_gige_start_xmit,
> > +	.ndo_set_mac_address	= eth_mac_addr,
> > +	.ndo_validate_addr	= eth_validate_addr,
> > +	.ndo_do_ioctl		= mlxbf_gige_do_ioctl,
> > +	.ndo_set_rx_mode        = mlxbf_gige_set_rx_mode,
> 
> You must report standard stats.
> 

Are you referring to the three possible methods that a driver
must use the implement support of standard stats reporting:

>From include/linux/netdevice.h -->
* void (*ndo_get_stats64)(struct net_device *dev,
 *                         struct rtnl_link_stats64 *storage);
 * struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
 *      Called when a user wants to get the network device usage
 *      statistics. Drivers must do one of the following:
 *      1. Define @ndo_get_stats64 to fill in a zero-initialised
 *         rtnl_link_stats64 structure passed by the caller.
 *      2. Define @ndo_get_stats to update a net_device_stats structure
 *         (which should normally be dev->stats) and return a pointer to
 *         it. The structure may be changed asynchronously only if each
 *         field is written atomically.
 *      3. Update dev->stats asynchronously and atomically, and define
 *         neither operation.

The mlxbf_gige driver has implemented #3 above, as there is logic
in the RX and TX completion handlers that increments RX/TX packet
and byte counts in the net_device->stats structure.  Is that sufficient
for support of standard stats?