Re: [PATCH net-next v2 03/10] net: sparx5: add hostmode with phylink support

[Jakub Kicinski]

On Mon, 31 May 2021 16:02:54 +0200 Steen Hegelund wrote:
> > > +     val = ether_addr_to_u64(sparx5->base_mac) + portno + 1;
> > > +     u64_to_ether_addr(val, ndev->dev_addr);
> > > +
> > > +     return ndev;
> > > +}  
> >   
> > > +static void sparx5_xtr_grp(struct sparx5 *sparx5, u8 grp, bool byte_swap)
> > > +{
> > > +     bool eof_flag = false, pruned_flag = false, abort_flag = false;
> > > +     struct net_device *netdev;
> > > +     struct sparx5_port *port;
> > > +     struct frame_info fi;
> > > +     int i, byte_cnt = 0;
> > > +     struct sk_buff *skb;
> > > +     u32 ifh[IFH_LEN];
> > > +     u32 *rxbuf;
> > > +
> > > +     /* Get IFH */
> > > +     for (i = 0; i < IFH_LEN; i++)
> > > +             ifh[i] = spx5_rd(sparx5, QS_XTR_RD(grp));
> > > +
> > > +     /* Decode IFH (whats needed) */
> > > +     sparx5_ifh_parse(ifh, &fi);
> > > +
> > > +     /* Map to port netdev */
> > > +     port = fi.src_port < SPX5_PORTS ?
> > > +             sparx5->ports[fi.src_port] : NULL;
> > > +     if (!port || !port->ndev) {
> > > +             dev_err(sparx5->dev, "Data on inactive port %d\n", fi.src_port);
> > > +             sparx5_xtr_flush(sparx5, grp);
> > > +             return;  
> > 
> > You should probably increment appropriate counter for each error
> > condition.  
> 
> At this first check I do not have the netdev, so it will not be
> possible to update any counters, but below I can use rx_dropped.  
> Is that what you mean?

Yes, sorry, I just scrolled up to the earliest drop I could find.
Indeed nothing we can increment here. 

> > > +
> > > +     /* Finish up skb */
> > > +     skb_put(skb, byte_cnt - ETH_FCS_LEN);
> > > +     eth_skb_pad(skb);
> > > +     skb->protocol = eth_type_trans(skb, netdev);
> > > +     netif_rx(skb);
> > > +     netdev->stats.rx_bytes += skb->len;
> > > +     netdev->stats.rx_packets++;  
> > 
> > Does the Rx really need to happen in an interrupt context?
> > Did you consider using NAPI or a tasklet?  
> 
> This register base injection and extraction is just preliminary.  I
> have the next series waiting with support for Frame DMA'ing and there
> I use NAPI, so if possible I would like to leave this as it is, since
> it only a stopgap.

Ah, that's fine.

> > What do you expect to happen at this point? Kernel can retry sending
> > for ever, is there a way for the driver to find out that the fifo is
> > no longer busy to stop/start the software queuing appropriately?  
> 
> Hmm.  I am not too familiar with the netdev queuing, but would this
> be a way forward?
> 
> 1) In sparx5_inject: After injecting a frame then test for HW queue
> readiness and watermark levels, and if there is a problem then call
> netif_queue_stop
> 
> 2) Add an implementation of ndo_tx_timeout where the HW queue and
> Watermark level is checked and if all is OK, then do a
> netif_wake_queue.

timeout is not a good mechanism because it will print a stack trace and
an error to logs. timeout is used for detecting broken interfaced.
Perhaps use a hrtimer or a normal timer? What kind of time scales are
we talking here?

> 3) But if the HW queue and/or Watermark level is still not OK - then
> probably something went seriously wrong, or the wait was to short.
> Will the ndo_tx_timeout be called again or is this a one-off?
> 
> If the ndo_tx_timeout call is a one-off the driver would need to
> reset the HW queue system or even deeper down...