On Mon, 06 Nov 2023 10:26:38 +1030 Andrew Jeffery <and...@codeconstruct.com.au> wrote:
> On Fri, 2023-11-03 at 15:05 +0000, Jonathan Cameron wrote: > > On Fri, 3 Nov 2023 16:45:20 +1030 > > Andrew Jeffery <and...@codeconstruct.com.au> wrote: > > > > > I ran out of spoons before I could come up with a better client tracking > > > scheme back in the original refactoring series: > > > > > > https://lore.kernel.org/all/20210608104757.582199-1-and...@aj.id.au/ > > > > > > Jonathan prodded Konstantin about the issue in a review of Konstantin's > > > MCTP patches[1], prompting an attempt to clean it up. > > > > > > [1]: https://lore.kernel.org/all/20230929120835.00001...@huawei.com/ > > > > > > 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 save > > > the client module from further work. The usual container_of() pattern > > > gets the client module access to its private data. > > > > > > Signed-off-by: Andrew Jeffery <and...@codeconstruct.com.au> > > > > Hi Andrew, > > > > A few comments inline. > > More generally, whilst this is definitely an improvement I'd have been > > 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 allow for > > simple dependency tracking, binding of individual drivers and all that. > > > > What you have here feels fine though and is a much less invasive change. > Sorry for slow reply, been traveling. > 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`? That was my thinking. The protocol is used to talk to someone - the endpoint (similar to i2c_client) so represent that. If that device is handling multiple protocols (no idea if that is possible) - that is fine as well, it just becomes like having multiple i2c_clients in a single package (fairly common for sensors), or the many other cases where we use a struct device to represent just part of a larger device that operates largely independently of other parts (or with well defined boundaries). Jonathan > > > > > Jonathan > > > > > > > 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 { > > > > > > > > > +static struct kcs_bmc_client * > > > +kcs_bmc_ipmi_add_device(struct kcs_bmc_driver *drv, struct > > > kcs_bmc_device *dev) > > > { > > > struct kcs_bmc_ipmi *priv; > > > int rc; > > > > > > priv = 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. > > > > > > > spin_lock_init(&priv->lock); > > > mutex_init(&priv->mutex); > > > init_waitqueue_head(&priv->queue); > > > > > > - priv->client.dev = kcs_bmc; > > > - priv->client.ops = &kcs_bmc_ipmi_client_ops; > > > + kcs_bmc_client_init(&priv->client, &kcs_bmc_ipmi_client_ops, drv, dev); > > > > > > priv->miscdev.minor = MISC_DYNAMIC_MINOR; > > > - priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, > > > kcs_bmc->channel); > > > + priv->miscdev.name = kasprintf(GFP_KERNEL, "%s%u", DEVICE_NAME, > > > dev->channel); > > > if (!priv->miscdev.name) { > > > rc = -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. I'm not that fussed either way. > > > > goto cleanup_priv; > > > > > > > > ... > > > > > 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 > > > > ... > > > > > > > +static struct kcs_bmc_client * > > > +kcs_bmc_serio_add_device(struct kcs_bmc_driver *drv, struct > > > kcs_bmc_device *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) > > > > > > priv = kzalloc(sizeof(*priv), GFP_KERNEL); > > > if (!priv) > > > - return -ENOMEM; > > > + return ERR_PTR(ENOMEM); > > > > > > /* Use kzalloc() as the allocation is cleaned up with kfree() via > > > serio_unregister_port() */ > > > port = kzalloc(sizeof(*port), GFP_KERNEL); > > > if (!port) { > > > - rc = -ENOMEM; > > > + rc = ENOMEM; > > Why positive? > > Doesn't ERR_PTR() typically get passed negatives? > > Ack, as above. > > Thanks for the review, > > Andrew _______________________________________________ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
