Re: [PATCH net-next v6 3/3] mctp i3c: MCTP I3C driver

From: Paolo Abeni <pabeni@redhat.com>
To: Simon Horman <horms@kernel.org>,
	Matt Johnston <matt@codeconstruct.com.au>
Cc: linux-i3c@lists.infradead.org, netdev@vger.kernel.org,
	 devicetree@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Jeremy Kerr <jk@codeconstruct.com.au>,
	 Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 miquel.raynal@bootlin.com
Subject: Re: [PATCH net-next v6 3/3] mctp i3c: MCTP I3C driver
Date: Tue, 17 Oct 2023 12:46:07 +0200	[thread overview]
Message-ID: <caa89a15568b2d92592476995bfcf362475be11f.camel@redhat.com> (raw)
In-Reply-To: <20231017082427.GH1751252@kernel.org>

On Tue, 2023-10-17 at 10:24 +0200, Simon Horman wrote:
> On Fri, Oct 13, 2023 at 12:06:25PM +0800, Matt Johnston wrote:
> > Provides MCTP network transport over an I3C bus, as specified in
> > DMTF DSP0233.
> > 
> > Each I3C bus (with "mctp-controller" devicetree property) gets an
> > "mctpi3cX" net device created. I3C devices are reachable as remote
> > endpoints through that net device. Link layer addressing uses the
> > I3C PID as a fixed hardware address for neighbour table entries.
> > 
> > The driver matches I3C devices that have the MIPI assigned DCR 0xCC for
> > MCTP.
> > 
> > Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> 
> Hi Matt,
> 
> one minor nit below, which you can take, leave, or leave for later
> as far as I am concerned.
> 
> Overall the patch looks good to me and I see that Paolo's review of v5 has
> has been addressed.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> > +/* List of mctp_i3c_busdev */
> > +static LIST_HEAD(busdevs);
> > +/* Protects busdevs, as well as mctp_i3c_bus.devs lists */
> > +static DEFINE_MUTEX(busdevs_lock);
> > +
> > +struct mctp_i3c_bus {
> > +	struct net_device *ndev;
> > +
> > +	struct task_struct *tx_thread;
> > +	wait_queue_head_t tx_wq;
> > +	/* tx_lock protects tx_skb and devs */
> > +	spinlock_t tx_lock;
> > +	/* Next skb to transmit */
> > +	struct sk_buff *tx_skb;
> > +	/* Scratch buffer for xmit */
> > +	u8 tx_scratch[MCTP_I3C_MAXBUF];
> > +
> > +	/* Element of busdevs */
> > +	struct list_head list;
> 
> I am unsure if it is important, but I observe that on x86_64
> list spans a cacheline.

It looks like 'list' is only touched on control path, so it's should
not critical.

Cheers,

Paolo