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