Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <4e355b4b19d49753d900094c900f2586d283179e.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, 27 Nov 2023 13:32:57 +1030
In-Reply-To: <20231120124001.00003cbc@Huawei.com>
References: <20231103061522.1268637-1-andrew@codeconstruct.com.au>
         <20231103061522.1268637-9-andrew@codeconstruct.com.au>
         <20231103150522.00004539@Huawei.com>
         <a015924b542fd35fe84357eebddd14cfae83dace.camel@codeconstruct.com.au>
         <20231120124001.00003cbc@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 Mon, 2023-11-20 at 12:40 +0000, Jonathan Cameron wrote:
> On Mon, 06 Nov 2023 10:26:38 +1030
> Andrew Jeffery <andrew@codeconstruct.com.au> wrote:
>=20
> > 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 tra=
cking
> > > > 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 Konstant=
in'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, an=
d
> > > > provide it as the argument to the remove_device() implementation to=
 save
> > > > the client module from further work. The usual container_of() patte=
rn
> > > > gets the client module access to its private data.
> > > >=20
> > > > Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au> =20
> > >=20
> > > Hi Andrew,
> > >=20
> > > A few comments inline.
> > > More generally, whilst this is definitely an improvement I'd have bee=
n tempted
> > > to make more use of the linux device model for this with the clients =
added
> > > as devices with a parent of the kcs_bmc_device.  That would then allo=
w for
> > > simple dependency tracking, binding of individual drivers and all tha=
t.
> > >=20
> > > What you have here feels fine though and is a much less invasive chan=
ge. =20
> >=20
> Sorry for slow reply, been traveling.

No worries, I've just got back from travel myself :)

>=20
> > 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
> That was my thinking.  The protocol is used to talk to someone - the endp=
oint
> (similar to i2c_client) so represent that. If that device is handling mul=
tiple
> protocols (no idea if that is possible) - that is fine as well, it just b=
ecomes
> like having multiple i2c_clients in a single package (fairly common for s=
ensors),
> or the many other cases where we use a struct device to represent just pa=
rt
> of a larger device that operates largely independently of other parts (or=
 with
> well defined boundaries).

Alright, let me think about that a bit more.

Thanks for the feedback,

Andrew