Re: [PATCH net-next v25 1/1] mctp pcc: Implement MCTP over PCC Transport

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Adam,

> In addition to the below comment, I am removing the additional lock on
> the skb lists and using the internal one for all operations.  It leads
> to leaner and cleaner code.

Ok, neat!

Just be careful with locking as you're iterating the queues. Your
current approach of doing the drain under one lock acquire is probably
the best, if you can do the same with the queue-internal locking.

> > > +       mctp_pcc_ndev->inbox.chan->rx_alloc = mctp_pcc_rx_alloc;
> > > +       mctp_pcc_ndev->outbox.chan->manage_writes = true;
> > > +
> > > +       /* There is no clean way to pass the MTU to the callback function
> > > +        * used for registration, so set the values ahead of time.
> > > +        */
> > For my own clarity, what's "the callback function used for
> > registration"?
> 
> 
> Actually, this is not longer true: we can do it in ndo_open, and it
> is clean.  Removed the comment.

OK, The current patch *does* do it in ndo_open though, hence my
confusion.

From your other reply:

> > > +static int mctp_pcc_ndo_open(struct net_device *ndev)
> > > +{
> > > +       struct mctp_pcc_ndev *mctp_pcc_ndev =
> > > +           netdev_priv(ndev);
> > > +       struct mctp_pcc_mailbox *outbox =
> > > +           &mctp_pcc_ndev->outbox;
> > > +       struct mctp_pcc_mailbox *inbox =
> > > +           &mctp_pcc_ndev->inbox;
> > Minor: I don't think these need wrapping?
> 
> The outbox and inbox lines are longer than the mctp_pcc_ndev line, and
> they depend on it.  This ordering and wrapping passes the xmas  tree
> check and keeps assignment with declaration.

If you need a specific ordering for actual correctness, no need to
force that into a reverse-christmas-tree. Spacing requirements like that
are secondary.

The only remaining query I had was the TX flow control. You're returning
NETDEV_TX_BUSY while the queues are still running, so are likely to get
repeated TX in a loop there.

Cheers,


Jeremy