Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <a015924b542fd35fe84357eebddd14cfae83dace.camel@codeconstruct.com.au>
Subject: Re: [PATCH 08/10] ipmi: kcs_bmc: Track clients in core
From:   Andrew Jeffery <andrew@codeconstruct.com.au>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc:     minyard@acm.org, openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, aladyshev22@gmail.com,
        jk@codeconstruct.com.au
Date:   Mon, 06 Nov 2023 10:26:38 +1030
In-Reply-To: <20231103150522.00004539@Huawei.com>
References: <20231103061522.1268637-1-andrew@codeconstruct.com.au>
         <20231103061522.1268637-9-andrew@codeconstruct.com.au>
         <20231103150522.00004539@Huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4-2 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2023-11-03 at 15:05 +0000, Jonathan Cameron wrote:
> On Fri,  3 Nov 2023 16:45:20 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
>=20
> > I ran out of spoons before I could come up with a better client trackin=
g
> > scheme back in the original refactoring series:
> >=20
> > https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/
> >=20
> > Jonathan prodded Konstantin about the issue in a review of Konstantin's
> > MCTP patches[1], prompting an attempt to clean it up.
> >=20
> > [1]: https://lore.kernel.org/all/20230929120835.0000108e@Huawei.com/
> >=20
> > Prevent client modules from having to track their own instances by
> > requiring they return a pointer to a client object from their
> > add_device() implementation. We can then track this in the core, and
> > provide it as the argument to the remove_device() implementation to sav=
e
> > the client module from further work. The usual container_of() pattern
> > gets the client module access to its private data.
> >=20
> > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
>=20
> Hi Andrew,
>=20
> A few comments inline.
> More generally, whilst this is definitely an improvement I'd have been te=
mpted
> to make more use of the linux device model for this with the clients adde=
d
> as devices with a parent of the kcs_bmc_device.  That would then allow fo=
r
> simple dependency tracking, binding of individual drivers and all that.
>=20
> What you have here feels fine though and is a much less invasive change.

Yeah, I had this debate with myself before posting the patches. My
reasoning for the current approach is that the clients don't typically
represent a device, rather a protocol implementation that is
communicated over a KCS device (maybe more like pairing a line
discipline with a UART). It was unclear to me whether associating a
`struct device` with a protocol implementation was stretching the
abstraction a bit, or whether I haven't considered some other
perspective hard enough - maybe we treat the client as the remote
device, similar to e.g. a `struct i2c_client`?

>=20
> Jonathan
>=20
>=20
> > diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/=
kcs_bmc_cdev_ipmi.c
> > index 98f231f24c26..9fca31f8c7c2 100644
> > --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> > @@ -71,8 +71,6 @@ enum kcs_ipmi_errors {
>=20
>=20
>=20
> > +static struct kcs_bmc_client *
> > +kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_dev=
ice *dev)
> >  {
> >  	struct kcs_bmc_ipmi *priv;
> >  	int rc;
> > =20
> >  	priv =3D kzalloc(sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> > -		return -ENOMEM;
> > +		return ERR_PTR(ENOMEM);
> As below. I thought it took negatives..

I should have double checked that. It requires negatives. Thanks.

> > =20
> >  	spin_lock_init(&priv->lock);
> >  	mutex_init(&priv->mutex);
> >  	init_waitqueue_head(&priv->queue);
> > =20
> > -	priv->client.dev =3D kcs_bmc;
> > -	priv->client.ops =3D &kcs_bmc_ipmi_client_ops;
> > +	kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev=
);
> > =20
> >  	priv->miscdev.minor =3D MISC_DYNAMIC_MINOR;
> > -	priv->miscdev.name =3D kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, kcs=
_bmc->channel);
> > +	priv->miscdev.name =3D kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, dev=
->channel);
> >  	if (!priv->miscdev.name) {
> >  		rc =3D -ENOMEM;
> ERR_PTR

I converted it to an ERR_PTR in the return after the cleanup_priv
label. Maybe it's preferable I do the conversion immediately? Easy
enough to change if you think so.

> >  		goto cleanup_priv;
>=20
>=20
>=20
> ...
>=20
> > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_=
bmc_serio.c
> > index 0a68c76da955..3cfda39506f6 100644
> > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > +++ b/drivers/char/ipmi/kcs_bmc_serio.c
>=20
> ...
>=20
>=20
> > +static struct kcs_bmc_client *
> > +kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct kcs_bmc_de=
vice *dev)
> >  {
> >  	struct kcs_bmc_serio *priv;
> >  	struct serio *port;
> > @@ -75,12 +71,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_=
device *kcs_bmc)
> > =20
> >  	priv =3D kzalloc(sizeof(*priv), GFP_KERNEL);
> >  	if (!priv)
> > -		return -ENOMEM;
> > +		return ERR_PTR(ENOMEM);
> > =20
> >  	/* Use kzalloc() as the allocation is cleaned up with kfree() via ser=
io_unregister_port() */
> >  	port =3D kzalloc(sizeof(*port), GFP_KERNEL);
> >  	if (!port) {
> > -		rc =3D -ENOMEM;
> > +		rc =3D ENOMEM;
> Why positive?
> Doesn't ERR_PTR() typically get passed negatives?=20

Ack, as above.

Thanks for the review,

Andrew