On Thu, 20 Aug 2020 18:51:39 +0000 David Thompson wrote:
> > > + 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).
It's highly unusual, all drivers I know use the streaming DMA interface.
IDK what the performance implications for using coherent mappings on
your platforms are, but I'd prefer if you took the more common approach.
> > > +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?
The Linux APIs don't provide any "snapshot" guarantees, and you're
stalling the datapath to read stats.
> > > +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?
You only update the basic stats. Please take a look at all members,
some of the hw stats will probably fit there, and other HW errors need
to be added to the cumulative rx/tx error stats.
