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

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: <9b66a22a2fe689a993d9ff83baf8b7bbecbb8c90.camel@codeconstruct.com.au>
Date: Wed, 27 Aug 2025 09:37:05 +0800
From: Jeremy Kerr <jk@...econstruct.com.au>
To: Adam Young <admiyo@...eremail.onmicrosoft.com>, 
 admiyo@...amperecomputing.com, Matt Johnston <matt@...econstruct.com.au>, 
 Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, Sudeep Holla
	 <sudeep.holla@....com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>, 
	Huisong Li <lihuisong@...wei.com>
Subject: Re: [PATCH net-next v25 1/1] mctp pcc: Implement MCTP over PCC
 Transport

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