Received-SPF: pass (google.com: domain of linux-kernel+bounces-199897-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4eefd134bec51482655018bbbd7c4d5c7668a7fa.camel@codeconstruct.com.au>
Subject: Re: [PATCH v2 3/3] mctp pcc: Implement MCTP over PCC Transport
From: Jeremy Kerr <jk@codeconstruct.com.au>
To: Adam Young <admiyo@amperemail.onmicrosoft.com>, 
 admiyo@os.amperecomputing.com, Matt Johnston <matt@codeconstruct.com.au>, 
 "David S. Miller" <davem@davemloft.net>, Eric Dumazet
 <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
 <pabeni@redhat.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Date: Tue, 04 Jun 2024 09:15:53 +0800
In-Reply-To: <1a38b394-30ae-42c6-b363-9f3a00166259@amperemail.onmicrosoft.com>
References: <20240513173546.679061-1-admiyo@os.amperecomputing.com>
	 <20240528191823.17775-1-admiyo@os.amperecomputing.com>
	 <20240528191823.17775-4-admiyo@os.amperecomputing.com>
	 <6a01ffb4ef800f381d3e494bf1862f6e4468eb7d.camel@codeconstruct.com.au>
	 <1a38b394-30ae-42c6-b363-9f3a00166259@amperemail.onmicrosoft.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4-2 
Precedence: bulk
MIME-Version: 1.0

Hi Adam,

> > And can you include a brief summary of changes since the prior version
> > you have sent?
> >=20
> They are all in the header patch.

Ah, neat. Can you include reviewers on CC for that 0/n patch then?

> > > +static struct list_head mctp_pcc_ndevs;
> > I'm not clear on what this list is doing; it seems to be for freeing
> > devices on module unload (or device remove).
> >=20
> > However, the module will be refcounted while there are devices bound, s=
o
> > module unload shouldn't be possible in that state. So the only time
> > you'll be iterating this list to free everything will be when it's
> > empty.
> >=20
> > You could replace this with the mctp_pcc_driver_remove() just removing =
the
> > device passed in the argument, rather than doing any list iteration.
> >=20
> > ... unless I've missed something?
>=20
> There is no requirement that all the devices=C2=A0 be unloaded in order f=
or=20
> the module to get unloaded.

... aside from the driver refcounting. You're essentially replicating
the driver core's own facility for device-to-driver mappings here.

> It someone wants to disable the MCTP devices, they can unload the=20
> module, and it gets cleaned up.
>=20
> With ACPI, the devices never go away, they are defined in a table read=
=20
> at start up and stay there.

Sure, the ACPI bus devices may always be present, but you can still
unbind the driver from one device:

   echo '<device-id>' > /sys/bus/acpi/drivers/mctp_pcc/unbind

- where device-id is one of the links to a device in that mctp_pcc dir.

then:

> So without this change there is no way to unload the module.

... with no devices bound, you can safely unload the module (but the
unload path will also perform that unbind anyway, more on that below).

> Maybe it is just a convenience for development, but I think most
> modules behave this way.

If you can avoid holding internal references to devices, you have a
whole class of bugs you can avoid.

> > Any benefit in including the pcc_hdr in the skb?
> >=20
> > (not necessarily an issue, just asking...)
> It shows up in=C2=A0 tracing of the packet.=C2=A0 Useful for debugging.

Sounds good!

> > Does anything need to tell the mailbox driver to do that ack after
> > setting ack_rx?
>=20
> Yes.=C2=A0 It is in the previous patch, in the pcc_mailbox code.=C2=A0 I=
=20
> originally had it as a follow on, but reordered to make it a pre-req.=C2=
=A0=20
> That allows me to inline this logic, making the driver easier to review=
=20
> (I hope).

OK. As far as I can tell here this is just setting a member of the
mailbox interface, but not calling back into the mailbox code. If this
is okay, then all good.

> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0netif_stop_queue(ndev);
> > Do you need to stop and restart the queue? Your handling is atomic.
> I guess not.=C2=A0 This was just from following the examples of others. W=
ill=20
> remove.

Those examples (at least, in the MCTP drivers) will not have been able
to complete transmission until way later - say, after a separate
completion, or after a separate thread has processed the outgoing skb.
While that is happening, we may have stopped the queue.

In your case, you complete transmission entirely within the start_xmit
operation (*and* that path is atomic), so the queue is fine to remain
enabled.

> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0if (adev && mctp_pcc_dev->acpi_device =3D=3D adev)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0con=
tinue;
> > I think you meant '!=3D' instead of '=3D=3D'?
> Yes.=C2=A0 Yes I did.=C2=A0 This is code that has to be there for complet=
eness,
> but I don't really have a way to test, except for the "delete all" case.

The 'unbind' example above will test this.

> > > +static int __init mctp_pcc_mod_init(void)
> > > +{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int rc;
> > > +
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pr_debug("Initializing MCT=
P over PCC transport driver\n");
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0INIT_LIST_HEAD(&mctp_pcc_n=
devs);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rc =3D acpi_bus_register_d=
river(&mctp_pcc_driver);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (rc < 0)
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error registering=
 driver\n"));
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0return rc;
> > > +}
> > > +
> > > +static __exit void mctp_pcc_mod_exit(void)
> > > +{
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0pr_debug("Removing MCTP ov=
er PCC transport driver\n");
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0mctp_pcc_driver_remove(NUL=
L);
> > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0acpi_bus_unregister_driver=
(&mctp_pcc_driver);
> > > +}
> > > +
> > > +module_init(mctp_pcc_mod_init);
> > > +module_exit(mctp_pcc_mod_exit);
> > If you end up removing the mctp_pcc_ndevs list, these can all be
> > replaced with module_acpi_driver(mctp_pcc_driver);
>=20
> Yeah, I can't get away with that.=C2=A0 The ACPI devices may still be the=
re=20
> when some one calls rmmod, and so we need to clean up the ndevs.

The core driver unregister path should unbind all devices before the
module is removed, so your mctp_pcc_driver_remove() should get invoked
on each individual device during that process.

(with the above !=3D vs. =3D=3D bug fixed, you'll probably find that
"delete-all" case will always hit an empty list)

... this is unless something is different about the ACPI bus type, but
it all looks standard from a brief look!

Cheers,


Jeremy