[08/10] ipmi: kcs_bmc: Track clients in core
Commit Message
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-andrew@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.0000108e@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 <andrew@codeconstruct.com.au>
---
drivers/char/ipmi/kcs_bmc.c | 68 ++++++++++++++++-----------
drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 42 ++++-------------
drivers/char/ipmi/kcs_bmc_client.h | 35 ++++++++++----
drivers/char/ipmi/kcs_bmc_device.h | 4 +-
drivers/char/ipmi/kcs_bmc_serio.c | 43 +++++------------
5 files changed, 88 insertions(+), 104 deletions(-)
Comments
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:
>
> > 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-andrew@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.0000108e@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 <andrew@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.
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`?
>
> 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.
> > 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
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:
>
> > 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:
> > >
> > > > 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-andrew@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.0000108e@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 <andrew@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.
No worries, I've just got back from travel myself :)
>
> > 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).
Alright, let me think about that a bit more.
Thanks for the feedback,
Andrew
@@ -19,6 +19,7 @@
static DEFINE_MUTEX(kcs_bmc_lock);
static LIST_HEAD(kcs_bmc_devices);
static LIST_HEAD(kcs_bmc_drivers);
+static LIST_HEAD(kcs_bmc_clients);
/* Consumer data access */
@@ -129,25 +130,27 @@ void kcs_bmc_disable_device(struct kcs_bmc_client *client)
}
EXPORT_SYMBOL(kcs_bmc_disable_device);
-int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
+int kcs_bmc_add_device(struct kcs_bmc_device *dev)
{
+ struct kcs_bmc_client *client;
struct kcs_bmc_driver *drv;
int error = 0;
- int rc;
- spin_lock_init(&kcs_bmc->lock);
- kcs_bmc->client = NULL;
+ spin_lock_init(&dev->lock);
+ dev->client = NULL;
mutex_lock(&kcs_bmc_lock);
- list_add(&kcs_bmc->entry, &kcs_bmc_devices);
+ list_add(&dev->entry, &kcs_bmc_devices);
list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
- rc = drv->ops->add_device(kcs_bmc);
- if (!rc)
- continue;
-
- dev_err(kcs_bmc->dev, "Failed to add chardev for KCS channel %d: %d",
- kcs_bmc->channel, rc);
- error = rc;
+ client = drv->ops->add_device(drv, dev);
+ if (IS_ERR(client)) {
+ error = PTR_ERR(client);
+ dev_err(dev->dev,
+ "Failed to add chardev for KCS channel %d: %d",
+ dev->channel, error);
+ break;
+ }
+ list_add(&client->entry, &kcs_bmc_clients);
}
mutex_unlock(&kcs_bmc_lock);
@@ -155,31 +158,37 @@ int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc)
}
EXPORT_SYMBOL(kcs_bmc_add_device);
-void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc)
+void kcs_bmc_remove_device(struct kcs_bmc_device *dev)
{
- struct kcs_bmc_driver *drv;
+ struct kcs_bmc_client *curr, *tmp;
mutex_lock(&kcs_bmc_lock);
- list_del(&kcs_bmc->entry);
- list_for_each_entry(drv, &kcs_bmc_drivers, entry) {
- drv->ops->remove_device(kcs_bmc);
+ list_for_each_entry_safe(curr, tmp, &kcs_bmc_clients, entry) {
+ if (curr->dev == dev) {
+ list_del(&curr->entry);
+ curr->drv->ops->remove_device(curr);
+ }
}
+ list_del(&dev->entry);
mutex_unlock(&kcs_bmc_lock);
}
EXPORT_SYMBOL(kcs_bmc_remove_device);
void kcs_bmc_register_driver(struct kcs_bmc_driver *drv)
{
- struct kcs_bmc_device *kcs_bmc;
- int rc;
+ struct kcs_bmc_client *client;
+ struct kcs_bmc_device *dev;
mutex_lock(&kcs_bmc_lock);
list_add(&drv->entry, &kcs_bmc_drivers);
- list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
- rc = drv->ops->add_device(kcs_bmc);
- if (rc)
- dev_err(kcs_bmc->dev, "Failed to add driver for KCS channel %d: %d",
- kcs_bmc->channel, rc);
+ list_for_each_entry(dev, &kcs_bmc_devices, entry) {
+ client = drv->ops->add_device(drv, dev);
+ if (IS_ERR(client)) {
+ dev_err(dev->dev, "Failed to add driver for KCS channel %d: %ld",
+ dev->channel, PTR_ERR(client));
+ continue;
+ }
+ list_add(&client->entry, &kcs_bmc_clients);
}
mutex_unlock(&kcs_bmc_lock);
}
@@ -187,13 +196,16 @@ EXPORT_SYMBOL(kcs_bmc_register_driver);
void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv)
{
- struct kcs_bmc_device *kcs_bmc;
+ struct kcs_bmc_client *curr, *tmp;
mutex_lock(&kcs_bmc_lock);
- list_del(&drv->entry);
- list_for_each_entry(kcs_bmc, &kcs_bmc_devices, entry) {
- drv->ops->remove_device(kcs_bmc);
+ list_for_each_entry_safe(curr, tmp, &kcs_bmc_clients, entry) {
+ if (curr->drv == drv) {
+ list_del(&curr->entry);
+ drv->ops->remove_device(curr);
+ }
}
+ list_del(&drv->entry);
mutex_unlock(&kcs_bmc_lock);
}
EXPORT_SYMBOL(kcs_bmc_unregister_driver);
@@ -71,8 +71,6 @@ enum kcs_ipmi_errors {
#define KCS_ZERO_DATA 0
struct kcs_bmc_ipmi {
- struct list_head entry;
-
struct kcs_bmc_client client;
spinlock_t lock;
@@ -462,27 +460,24 @@ static const struct file_operations kcs_bmc_ipmi_fops = {
.unlocked_ioctl = kcs_bmc_ipmi_ioctl,
};
-static DEFINE_SPINLOCK(kcs_bmc_ipmi_instances_lock);
-static LIST_HEAD(kcs_bmc_ipmi_instances);
-
-static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
+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);
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;
goto cleanup_priv;
@@ -496,13 +491,9 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
goto cleanup_miscdev_name;
}
- spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
- list_add(&priv->entry, &kcs_bmc_ipmi_instances);
- spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
+ pr_info("Initialised IPMI client for channel %d\n", dev->channel);
- pr_info("Initialised IPMI client for channel %d\n", kcs_bmc->channel);
-
- return 0;
+ return &priv->client;
cleanup_miscdev_name:
kfree(priv->miscdev.name);
@@ -510,25 +501,12 @@ static int kcs_bmc_ipmi_add_device(struct kcs_bmc_device *kcs_bmc)
cleanup_priv:
kfree(priv);
- return rc;
+ return ERR_PTR(rc);
}
-static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_ipmi_remove_device(struct kcs_bmc_client *client)
{
- struct kcs_bmc_ipmi *priv = NULL, *pos;
-
- spin_lock_irq(&kcs_bmc_ipmi_instances_lock);
- list_for_each_entry(pos, &kcs_bmc_ipmi_instances, entry) {
- if (pos->client.dev == kcs_bmc) {
- priv = pos;
- list_del(&pos->entry);
- break;
- }
- }
- spin_unlock_irq(&kcs_bmc_ipmi_instances_lock);
-
- if (!priv)
- return;
+ struct kcs_bmc_ipmi *priv = client_to_kcs_bmc_ipmi(client);
misc_deregister(&priv->miscdev);
kcs_bmc_disable_device(&priv->client);
@@ -8,16 +8,7 @@
#include "kcs_bmc.h"
-struct kcs_bmc_driver_ops {
- int (*add_device)(struct kcs_bmc_device *kcs_bmc);
- void (*remove_device)(struct kcs_bmc_device *kcs_bmc);
-};
-
-struct kcs_bmc_driver {
- struct list_head entry;
-
- const struct kcs_bmc_driver_ops *ops;
-};
+struct kcs_bmc_driver;
struct kcs_bmc_client_ops {
irqreturn_t (*event)(struct kcs_bmc_client *client);
@@ -26,7 +17,31 @@ struct kcs_bmc_client_ops {
struct kcs_bmc_client {
const struct kcs_bmc_client_ops *ops;
+ struct kcs_bmc_driver *drv;
struct kcs_bmc_device *dev;
+ struct list_head entry;
+};
+
+struct kcs_bmc_driver_ops {
+ struct kcs_bmc_client *(*add_device)(struct kcs_bmc_driver *drv,
+ struct kcs_bmc_device *dev);
+ void (*remove_device)(struct kcs_bmc_client *client);
+};
+
+static inline void kcs_bmc_client_init(struct kcs_bmc_client *client,
+ const struct kcs_bmc_client_ops *ops,
+ struct kcs_bmc_driver *drv,
+ struct kcs_bmc_device *dev)
+{
+ client->ops = ops;
+ client->drv = drv;
+ client->dev = dev;
+}
+
+struct kcs_bmc_driver {
+ struct list_head entry;
+
+ const struct kcs_bmc_driver_ops *ops;
};
void kcs_bmc_register_driver(struct kcs_bmc_driver *drv);
@@ -16,7 +16,7 @@ struct kcs_bmc_device_ops {
};
irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device *kcs_bmc);
-int kcs_bmc_add_device(struct kcs_bmc_device *kcs_bmc);
-void kcs_bmc_remove_device(struct kcs_bmc_device *kcs_bmc);
+int kcs_bmc_add_device(struct kcs_bmc_device *dev);
+void kcs_bmc_remove_device(struct kcs_bmc_device *dev);
#endif
@@ -13,8 +13,6 @@
#include "kcs_bmc_client.h"
struct kcs_bmc_serio {
- struct list_head entry;
-
struct kcs_bmc_client client;
struct serio *port;
@@ -64,10 +62,8 @@ static void kcs_bmc_serio_close(struct serio *port)
kcs_bmc_disable_device(&priv->client);
}
-static DEFINE_SPINLOCK(kcs_bmc_serio_instances_lock);
-static LIST_HEAD(kcs_bmc_serio_instances);
-
-static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
+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;
goto cleanup_priv;
}
@@ -88,45 +84,28 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
port->open = kcs_bmc_serio_open;
port->close = kcs_bmc_serio_close;
port->port_data = priv;
- port->dev.parent = kcs_bmc->dev;
+ port->dev.parent = dev->dev;
spin_lock_init(&priv->lock);
priv->port = port;
- priv->client.dev = kcs_bmc;
- priv->client.ops = &kcs_bmc_serio_client_ops;
- spin_lock_irq(&kcs_bmc_serio_instances_lock);
- list_add(&priv->entry, &kcs_bmc_serio_instances);
- spin_unlock_irq(&kcs_bmc_serio_instances_lock);
+ kcs_bmc_client_init(&priv->client, &kcs_bmc_serio_client_ops, drv, dev);
serio_register_port(port);
- pr_info("Initialised serio client for channel %d\n", kcs_bmc->channel);
+ pr_info("Initialised serio client for channel %d\n", dev->channel);
- return 0;
+ return &priv->client;
cleanup_priv:
kfree(priv);
- return rc;
+ return ERR_PTR(rc);
}
-static void kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc)
+static void kcs_bmc_serio_remove_device(struct kcs_bmc_client *client)
{
- struct kcs_bmc_serio *priv = NULL, *pos;
-
- spin_lock_irq(&kcs_bmc_serio_instances_lock);
- list_for_each_entry(pos, &kcs_bmc_serio_instances, entry) {
- if (pos->client.dev == kcs_bmc) {
- priv = pos;
- list_del(&pos->entry);
- break;
- }
- }
- spin_unlock_irq(&kcs_bmc_serio_instances_lock);
-
- if (!priv)
- return;
+ struct kcs_bmc_serio *priv = client_to_kcs_bmc_serio(client);
/* kfree()s priv->port via put_device() */
serio_unregister_port(priv->port);