lkml.org 
[lkml]   [2023]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 08/10] ipmi: kcs_bmc: Track clients in core
    Date
    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(-)

    diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
    index d70e503041bd..203cb73faa91 100644
    --- a/drivers/char/ipmi/kcs_bmc.c
    +++ b/drivers/char/ipmi/kcs_bmc.c
    @@ -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);
    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 {
    #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);
    diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h
    index 1251e9669bcd..1c6c812d6edc 100644
    --- a/drivers/char/ipmi/kcs_bmc_client.h
    +++ b/drivers/char/ipmi/kcs_bmc_client.h
    @@ -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);
    diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h
    index 17c572f25c54..ca2b5dc2031d 100644
    --- a/drivers/char/ipmi/kcs_bmc_device.h
    +++ b/drivers/char/ipmi/kcs_bmc_device.h
    @@ -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
    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
    @@ -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);
    --
    2.39.2
    \
     
     \ /
      Last update: 2023-11-20 13:47    [W:7.349 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site