Make the KCS device drivers responsible for allocating their own memory.
Until now the private data for the device driver was allocated internal
to the private data for the chardev interface. This coupling required
the slightly awkward API of passing through the struct size for the
driver private data to the chardev constructor, and then retrieving a
pointer to the driver private data from the allocated chardev memory.
In addition to being awkward, the arrangement prevents the
implementation of alternative userspace interfaces as the device driver
private data is not independent.
Peel a layer off the onion and turn the data-structures inside out by
exploiting container_of() and embedding `struct kcs_device` in the
driver private data.
Signed-off-by: Andrew Jeffery <[email protected]>
Reviewed-by: Zev Weiss <[email protected]>
---
drivers/char/ipmi/kcs_bmc.c | 19 +++++++--
drivers/char/ipmi/kcs_bmc.h | 12 ++----
drivers/char/ipmi/kcs_bmc_aspeed.c | 56 +++++++++++++------------
drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++++++++++-------
5 files changed, 111 insertions(+), 73 deletions(-)
diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
index ef5c48ffe74a..83da681bf49e 100644
--- a/drivers/char/ipmi/kcs_bmc.c
+++ b/drivers/char/ipmi/kcs_bmc.c
@@ -44,12 +44,23 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
}
EXPORT_SYMBOL(kcs_bmc_handle_event);
-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
+int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
+void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
{
- return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
+ if (kcs_bmc_ipmi_add_device(kcs_bmc))
+ pr_warn("Failed to add device for KCS channel %d\n",
+ kcs_bmc->channel);
}
-EXPORT_SYMBOL(kcs_bmc_alloc);
+EXPORT_SYMBOL(kcs_bmc_add_device);
+
+int kcs_bmc_ipmi_remove_device(struct kcs_bmc *kcs_bmc);
+void kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
+{
+ if (kcs_bmc_ipmi_remove_device(kcs_bmc))
+ pr_warn("Failed to remove device for KCS channel %d\n",
+ kcs_bmc->channel);
+}
+EXPORT_SYMBOL(kcs_bmc_remove_device);
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Haiyue Wang <[email protected]>");
diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
index febea0c8deb4..b2e6b7a7fe62 100644
--- a/drivers/char/ipmi/kcs_bmc.h
+++ b/drivers/char/ipmi/kcs_bmc.h
@@ -67,6 +67,8 @@ struct kcs_ioreg {
};
struct kcs_bmc {
+ struct device *dev;
+
spinlock_t lock;
u32 channel;
@@ -94,17 +96,11 @@ struct kcs_bmc {
u8 *kbuffer;
struct miscdevice miscdev;
-
- unsigned long priv[];
};
-static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
-{
- return kcs_bmc->priv;
-}
-
int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
-struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel);
+void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
+void kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index 06628ca69750..5d433dea5714 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -61,6 +61,8 @@
#define LPC_STR4 0x11C
struct aspeed_kcs_bmc {
+ struct kcs_bmc kcs_bmc;
+
struct regmap *map;
};
@@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
int (*get_io_address)(struct platform_device *pdev);
};
+static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc *kcs_bmc)
+{
+ return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
+}
+
static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
u32 val = 0;
int rc;
@@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
int rc;
rc = regmap_write(priv->map, reg, data);
@@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 val)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
int rc;
rc = regmap_update_bits(priv->map, reg, mask, val);
@@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 val
*/
static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
switch (kcs_bmc->channel) {
case 1:
@@ -148,7 +155,7 @@ static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
{
- struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
switch (kcs_bmc->channel) {
case 1:
@@ -323,17 +330,16 @@ static int aspeed_kcs_of_v2_get_io_address(struct platform_device *pdev)
static int aspeed_kcs_probe(struct platform_device *pdev)
{
const struct aspeed_kcs_of_ops *ops;
- struct device *dev = &pdev->dev;
struct aspeed_kcs_bmc *priv;
struct kcs_bmc *kcs_bmc;
struct device_node *np;
int rc, channel, addr;
- np = dev->of_node->parent;
+ np = pdev->dev.of_node->parent;
if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
!of_device_is_compatible(np, "aspeed,ast2500-lpc-v2") &&
!of_device_is_compatible(np, "aspeed,ast2600-lpc-v2")) {
- dev_err(dev, "unsupported LPC device binding\n");
+ dev_err(&pdev->dev, "unsupported LPC device binding\n");
return -ENODEV;
}
ops = of_device_get_match_data(&pdev->dev);
@@ -344,20 +350,22 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
if (channel < 0)
return channel;
- kcs_bmc = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel);
- if (!kcs_bmc)
+ addr = ops->get_io_address(pdev);
+ if (addr < 0)
+ return addr;
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
return -ENOMEM;
+ kcs_bmc = &priv->kcs_bmc;
+ kcs_bmc->dev = &pdev->dev;
+ kcs_bmc->channel = channel;
kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1];
kcs_bmc->io_inputb = aspeed_kcs_inb;
kcs_bmc->io_outputb = aspeed_kcs_outb;
kcs_bmc->io_updateb = aspeed_kcs_updateb;
- addr = ops->get_io_address(pdev);
- if (addr < 0)
- return addr;
-
- priv = kcs_bmc_priv(kcs_bmc);
priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
if (IS_ERR(priv->map)) {
dev_err(&pdev->dev, "Couldn't get regmap\n");
@@ -370,29 +378,23 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
if (rc)
return rc;
- dev_set_drvdata(dev, kcs_bmc);
+ platform_set_drvdata(pdev, priv);
aspeed_kcs_enable_channel(kcs_bmc, true);
- rc = misc_register(&kcs_bmc->miscdev);
- if (rc) {
- dev_err(dev, "Unable to register device\n");
- return rc;
- }
+ kcs_bmc_add_device(&priv->kcs_bmc);
- dev_dbg(&pdev->dev,
- "Probed KCS device %d (IDR=0x%x, ODR=0x%x, STR=0x%x)\n",
- kcs_bmc->channel, kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr,
- kcs_bmc->ioreg.str);
+ dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n", kcs_bmc->channel, addr);
return 0;
}
static int aspeed_kcs_remove(struct platform_device *pdev)
{
- struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
+ struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev);
+ struct kcs_bmc *kcs_bmc = &priv->kcs_bmc;
- misc_deregister(&kcs_bmc->miscdev);
+ kcs_bmc_remove_device(kcs_bmc);
return 0;
}
diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
index 82c77994e481..5060643bf530 100644
--- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
+++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
@@ -382,7 +382,7 @@ static int kcs_bmc_ipmi_release(struct inode *inode, struct file *filp)
return 0;
}
-static const struct file_operations kcs_bmc_fops = {
+static const struct file_operations kcs_bmc_ipmi_fops = {
.owner = THIS_MODULE,
.open = kcs_bmc_ipmi_open,
.read = kcs_bmc_ipmi_read,
@@ -392,36 +392,58 @@ static const struct file_operations kcs_bmc_fops = {
.unlocked_ioctl = kcs_bmc_ipmi_ioctl,
};
-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
-struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel)
+int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
+int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc)
{
- struct kcs_bmc *kcs_bmc;
-
- kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv, GFP_KERNEL);
- if (!kcs_bmc)
- return NULL;
+ int rc;
spin_lock_init(&kcs_bmc->lock);
- kcs_bmc->channel = channel;
-
mutex_init(&kcs_bmc->mutex);
init_waitqueue_head(&kcs_bmc->queue);
- kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
- kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
- kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+ kcs_bmc->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+ kcs_bmc->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
+ kcs_bmc->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
- kcs_bmc->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%u",
- DEVICE_NAME, channel);
+ kcs_bmc->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u",
+ DEVICE_NAME, kcs_bmc->channel);
if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
!kcs_bmc->miscdev.name)
- return NULL;
- kcs_bmc->miscdev.fops = &kcs_bmc_fops;
+ return -ENOMEM;
- return kcs_bmc;
+ kcs_bmc->miscdev.fops = &kcs_bmc_ipmi_fops;
+
+ rc = misc_register(&kcs_bmc->miscdev);
+ if (rc) {
+ dev_err(kcs_bmc->dev, "Unable to register device: %d\n", rc);
+ return rc;
+ }
+
+ dev_info(kcs_bmc->dev, "Initialised IPMI client for channel %d", kcs_bmc->channel);
+
+ return 0;
+}
+EXPORT_SYMBOL(kcs_bmc_ipmi_add_device);
+
+int kcs_bmc_ipmi_remove_device(struct kcs_bmc *kcs_bmc);
+int kcs_bmc_ipmi_remove_device(struct kcs_bmc *kcs_bmc)
+{
+ misc_deregister(&kcs_bmc->miscdev);
+
+ spin_lock_irq(&kcs_bmc->lock);
+ kcs_bmc->running = 0;
+ kcs_bmc_ipmi_force_abort(kcs_bmc);
+ spin_unlock_irq(&kcs_bmc->lock);
+
+ devm_kfree(kcs_bmc->dev, kcs_bmc->kbuffer);
+ devm_kfree(kcs_bmc->dev, kcs_bmc->data_out);
+ devm_kfree(kcs_bmc->dev, kcs_bmc->data_in);
+ devm_kfree(kcs_bmc->dev, kcs_bmc);
+
+ return 0;
}
-EXPORT_SYMBOL(kcs_bmc_ipmi_alloc);
+EXPORT_SYMBOL(kcs_bmc_ipmi_remove_device);
MODULE_LICENSE("GPL v2");
MODULE_AUTHOR("Haiyue Wang <[email protected]>");
diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
index 1f44aadec9e8..f7b4e866f86e 100644
--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
@@ -65,6 +65,8 @@ struct npcm7xx_kcs_reg {
};
struct npcm7xx_kcs_bmc {
+ struct kcs_bmc kcs_bmc;
+
struct regmap *map;
const struct npcm7xx_kcs_reg *reg;
@@ -76,9 +78,14 @@ static const struct npcm7xx_kcs_reg npcm7xx_kcs_reg_tbl[KCS_CHANNEL_MAX] = {
{ .sts = KCS3ST, .dob = KCS3DO, .dib = KCS3DI, .ctl = KCS3CTL, .ie = KCS3IE },
};
+static inline struct npcm7xx_kcs_bmc *to_npcm7xx_kcs_bmc(struct kcs_bmc *kcs_bmc)
+{
+ return container_of(kcs_bmc, struct npcm7xx_kcs_bmc, kcs_bmc);
+}
+
static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
{
- struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
u32 val = 0;
int rc;
@@ -90,7 +97,7 @@ static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
{
- struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
int rc;
rc = regmap_write(priv->map, reg, data);
@@ -99,7 +106,7 @@ static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
static void npcm7xx_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 data)
{
- struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
int rc;
rc = regmap_update_bits(priv->map, reg, mask, data);
@@ -108,7 +115,7 @@ static void npcm7xx_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 da
static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
{
- struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
+ struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE,
enable ? KCS_CTL_IBFIE : 0);
@@ -155,11 +162,10 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
return -ENODEV;
}
- kcs_bmc = kcs_bmc_alloc(dev, sizeof(*priv), chan);
- if (!kcs_bmc)
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
return -ENOMEM;
- priv = kcs_bmc_priv(kcs_bmc);
priv->map = syscon_node_to_regmap(dev->parent->of_node);
if (IS_ERR(priv->map)) {
dev_err(dev, "Couldn't get regmap\n");
@@ -167,6 +173,9 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
}
priv->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
+ kcs_bmc = &priv->kcs_bmc;
+ kcs_bmc->dev = &pdev->dev;
+ kcs_bmc->channel = chan;
kcs_bmc->ioreg.idr = priv->reg->dib;
kcs_bmc->ioreg.odr = priv->reg->dob;
kcs_bmc->ioreg.str = priv->reg->sts;
@@ -174,31 +183,29 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
kcs_bmc->io_outputb = npcm7xx_kcs_outb;
kcs_bmc->io_updateb = npcm7xx_kcs_updateb;
- dev_set_drvdata(dev, kcs_bmc);
+ platform_set_drvdata(pdev, priv);
npcm7xx_kcs_enable_channel(kcs_bmc, true);
rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
if (rc)
return rc;
- rc = misc_register(&kcs_bmc->miscdev);
- if (rc) {
- dev_err(dev, "Unable to register device\n");
- return rc;
- }
pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n",
chan,
kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr, kcs_bmc->ioreg.str);
+ kcs_bmc_add_device(kcs_bmc);
+
return 0;
}
static int npcm7xx_kcs_remove(struct platform_device *pdev)
{
- struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
+ struct npcm7xx_kcs_bmc *priv = platform_get_drvdata(pdev);
+ struct kcs_bmc *kcs_bmc = &priv->kcs_bmc;
- misc_deregister(&kcs_bmc->miscdev);
+ kcs_bmc_remove_device(kcs_bmc);
return 0;
}
--
2.27.0
On Mon, May 10, 2021 at 03:12:02PM +0930, Andrew Jeffery wrote:
> Make the KCS device drivers responsible for allocating their own memory.
>
> Until now the private data for the device driver was allocated internal
> to the private data for the chardev interface. This coupling required
> the slightly awkward API of passing through the struct size for the
> driver private data to the chardev constructor, and then retrieving a
> pointer to the driver private data from the allocated chardev memory.
>
> In addition to being awkward, the arrangement prevents the
> implementation of alternative userspace interfaces as the device driver
> private data is not independent.
>
> Peel a layer off the onion and turn the data-structures inside out by
> exploiting container_of() and embedding `struct kcs_device` in the
> driver private data.
All in all a very nice cleanup. A few nits inline.
>
> Signed-off-by: Andrew Jeffery <[email protected]>
> Reviewed-by: Zev Weiss <[email protected]>
> ---
> drivers/char/ipmi/kcs_bmc.c | 19 +++++++--
> drivers/char/ipmi/kcs_bmc.h | 12 ++----
> drivers/char/ipmi/kcs_bmc_aspeed.c | 56 +++++++++++++------------
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
> drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++++++++++-------
> 5 files changed, 111 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index ef5c48ffe74a..83da681bf49e 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -44,12 +44,23 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> }
> EXPORT_SYMBOL(kcs_bmc_handle_event);
>
> -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
> -struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
> +int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
The above (and it's remove function) should be in an include file.
> +void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
This should return an error so the probe can be failed and cleaned up
and so confusing message don't get printed after this in one case.
> {
> - return kcs_bmc_ipmi_alloc(dev, sizeof_priv, channel);
> + if (kcs_bmc_ipmi_add_device(kcs_bmc))
> + pr_warn("Failed to add device for KCS channel %d\n",
> + kcs_bmc->channel);
> }
> -EXPORT_SYMBOL(kcs_bmc_alloc);
> +EXPORT_SYMBOL(kcs_bmc_add_device);
> +
> +int kcs_bmc_ipmi_remove_device(struct kcs_bmc *kcs_bmc);
> +void kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc)
> +{
> + if (kcs_bmc_ipmi_remove_device(kcs_bmc))
> + pr_warn("Failed to remove device for KCS channel %d\n",
> + kcs_bmc->channel);
> +}
> +EXPORT_SYMBOL(kcs_bmc_remove_device);
>
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Haiyue Wang <[email protected]>");
> diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h
> index febea0c8deb4..b2e6b7a7fe62 100644
> --- a/drivers/char/ipmi/kcs_bmc.h
> +++ b/drivers/char/ipmi/kcs_bmc.h
> @@ -67,6 +67,8 @@ struct kcs_ioreg {
> };
>
> struct kcs_bmc {
> + struct device *dev;
> +
> spinlock_t lock;
>
> u32 channel;
> @@ -94,17 +96,11 @@ struct kcs_bmc {
> u8 *kbuffer;
>
> struct miscdevice miscdev;
> -
> - unsigned long priv[];
> };
>
> -static inline void *kcs_bmc_priv(struct kcs_bmc *kcs_bmc)
> -{
> - return kcs_bmc->priv;
> -}
> -
> int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc);
> -struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel);
> +void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc);
> +void kcs_bmc_remove_device(struct kcs_bmc *kcs_bmc);
>
> u8 kcs_bmc_read_data(struct kcs_bmc *kcs_bmc);
> void kcs_bmc_write_data(struct kcs_bmc *kcs_bmc, u8 data);
> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
> index 06628ca69750..5d433dea5714 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> @@ -61,6 +61,8 @@
> #define LPC_STR4 0x11C
>
> struct aspeed_kcs_bmc {
> + struct kcs_bmc kcs_bmc;
> +
> struct regmap *map;
> };
>
> @@ -69,9 +71,14 @@ struct aspeed_kcs_of_ops {
> int (*get_io_address)(struct platform_device *pdev);
> };
>
> +static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc *kcs_bmc)
> +{
> + return container_of(kcs_bmc, struct aspeed_kcs_bmc, kcs_bmc);
> +}
> +
> static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
> - struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> u32 val = 0;
> int rc;
>
> @@ -83,7 +90,7 @@ static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>
> static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> {
> - struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_write(priv->map, reg, data);
> @@ -92,7 +99,7 @@ static void aspeed_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
>
> static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 val)
> {
> - struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_update_bits(priv->map, reg, mask, val);
> @@ -114,7 +121,7 @@ static void aspeed_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 val
> */
> static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
> {
> - struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
>
> switch (kcs_bmc->channel) {
> case 1:
> @@ -148,7 +155,7 @@ static void aspeed_kcs_set_address(struct kcs_bmc *kcs_bmc, u16 addr)
>
> static void aspeed_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
> {
> - struct aspeed_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
>
> switch (kcs_bmc->channel) {
> case 1:
> @@ -323,17 +330,16 @@ static int aspeed_kcs_of_v2_get_io_address(struct platform_device *pdev)
> static int aspeed_kcs_probe(struct platform_device *pdev)
> {
> const struct aspeed_kcs_of_ops *ops;
> - struct device *dev = &pdev->dev;
> struct aspeed_kcs_bmc *priv;
> struct kcs_bmc *kcs_bmc;
> struct device_node *np;
> int rc, channel, addr;
>
> - np = dev->of_node->parent;
> + np = pdev->dev.of_node->parent;
> if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") &&
> !of_device_is_compatible(np, "aspeed,ast2500-lpc-v2") &&
> !of_device_is_compatible(np, "aspeed,ast2600-lpc-v2")) {
> - dev_err(dev, "unsupported LPC device binding\n");
> + dev_err(&pdev->dev, "unsupported LPC device binding\n");
> return -ENODEV;
> }
> ops = of_device_get_match_data(&pdev->dev);
> @@ -344,20 +350,22 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
> if (channel < 0)
> return channel;
>
> - kcs_bmc = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel);
> - if (!kcs_bmc)
> + addr = ops->get_io_address(pdev);
> + if (addr < 0)
> + return addr;
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> return -ENOMEM;
>
> + kcs_bmc = &priv->kcs_bmc;
> + kcs_bmc->dev = &pdev->dev;
> + kcs_bmc->channel = channel;
> kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1];
> kcs_bmc->io_inputb = aspeed_kcs_inb;
> kcs_bmc->io_outputb = aspeed_kcs_outb;
> kcs_bmc->io_updateb = aspeed_kcs_updateb;
>
> - addr = ops->get_io_address(pdev);
> - if (addr < 0)
> - return addr;
> -
> - priv = kcs_bmc_priv(kcs_bmc);
> priv->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> if (IS_ERR(priv->map)) {
> dev_err(&pdev->dev, "Couldn't get regmap\n");
> @@ -370,29 +378,23 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
> if (rc)
> return rc;
>
> - dev_set_drvdata(dev, kcs_bmc);
> + platform_set_drvdata(pdev, priv);
>
> aspeed_kcs_enable_channel(kcs_bmc, true);
>
> - rc = misc_register(&kcs_bmc->miscdev);
> - if (rc) {
> - dev_err(dev, "Unable to register device\n");
> - return rc;
> - }
> + kcs_bmc_add_device(&priv->kcs_bmc);
>
> - dev_dbg(&pdev->dev,
> - "Probed KCS device %d (IDR=0x%x, ODR=0x%x, STR=0x%x)\n",
> - kcs_bmc->channel, kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr,
> - kcs_bmc->ioreg.str);
> + dev_info(&pdev->dev, "Initialised channel %d at 0x%x\n", kcs_bmc->channel, addr);
>
> return 0;
> }
>
> static int aspeed_kcs_remove(struct platform_device *pdev)
> {
> - struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
> + struct aspeed_kcs_bmc *priv = platform_get_drvdata(pdev);
> + struct kcs_bmc *kcs_bmc = &priv->kcs_bmc;
>
> - misc_deregister(&kcs_bmc->miscdev);
> + kcs_bmc_remove_device(kcs_bmc);
>
> return 0;
> }
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index 82c77994e481..5060643bf530 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -382,7 +382,7 @@ static int kcs_bmc_ipmi_release(struct inode *inode, struct file *filp)
> return 0;
> }
>
> -static const struct file_operations kcs_bmc_fops = {
> +static const struct file_operations kcs_bmc_ipmi_fops = {
> .owner = THIS_MODULE,
> .open = kcs_bmc_ipmi_open,
> .read = kcs_bmc_ipmi_read,
> @@ -392,36 +392,58 @@ static const struct file_operations kcs_bmc_fops = {
> .unlocked_ioctl = kcs_bmc_ipmi_ioctl,
> };
>
> -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
> -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel)
> +int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
> +int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc)
> {
> - struct kcs_bmc *kcs_bmc;
> -
> - kcs_bmc = devm_kzalloc(dev, sizeof(*kcs_bmc) + sizeof_priv, GFP_KERNEL);
> - if (!kcs_bmc)
> - return NULL;
> + int rc;
>
> spin_lock_init(&kcs_bmc->lock);
> - kcs_bmc->channel = channel;
> -
> mutex_init(&kcs_bmc->mutex);
> init_waitqueue_head(&kcs_bmc->queue);
>
> - kcs_bmc->data_in = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> - kcs_bmc->data_out = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> - kcs_bmc->kbuffer = devm_kmalloc(dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> + kcs_bmc->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> + kcs_bmc->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
> + kcs_bmc->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL);
>
> kcs_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
> - kcs_bmc->miscdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%u",
> - DEVICE_NAME, channel);
> + kcs_bmc->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u",
> + DEVICE_NAME, kcs_bmc->channel);
> if (!kcs_bmc->data_in || !kcs_bmc->data_out || !kcs_bmc->kbuffer ||
> !kcs_bmc->miscdev.name)
> - return NULL;
> - kcs_bmc->miscdev.fops = &kcs_bmc_fops;
> + return -ENOMEM;
>
> - return kcs_bmc;
> + kcs_bmc->miscdev.fops = &kcs_bmc_ipmi_fops;
> +
> + rc = misc_register(&kcs_bmc->miscdev);
> + if (rc) {
> + dev_err(kcs_bmc->dev, "Unable to register device: %d\n", rc);
> + return rc;
> + }
> +
> + dev_info(kcs_bmc->dev, "Initialised IPMI client for channel %d", kcs_bmc->channel);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(kcs_bmc_ipmi_add_device);
> +
> +int kcs_bmc_ipmi_remove_device(struct kcs_bmc *kcs_bmc);
> +int kcs_bmc_ipmi_remove_device(struct kcs_bmc *kcs_bmc)
> +{
> + misc_deregister(&kcs_bmc->miscdev);
> +
> + spin_lock_irq(&kcs_bmc->lock);
> + kcs_bmc->running = 0;
> + kcs_bmc_ipmi_force_abort(kcs_bmc);
> + spin_unlock_irq(&kcs_bmc->lock);
> +
> + devm_kfree(kcs_bmc->dev, kcs_bmc->kbuffer);
> + devm_kfree(kcs_bmc->dev, kcs_bmc->data_out);
> + devm_kfree(kcs_bmc->dev, kcs_bmc->data_in);
> + devm_kfree(kcs_bmc->dev, kcs_bmc);
> +
> + return 0;
> }
> -EXPORT_SYMBOL(kcs_bmc_ipmi_alloc);
> +EXPORT_SYMBOL(kcs_bmc_ipmi_remove_device);
>
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Haiyue Wang <[email protected]>");
> diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
> index 1f44aadec9e8..f7b4e866f86e 100644
> --- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c
> +++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c
> @@ -65,6 +65,8 @@ struct npcm7xx_kcs_reg {
> };
>
> struct npcm7xx_kcs_bmc {
> + struct kcs_bmc kcs_bmc;
> +
> struct regmap *map;
>
> const struct npcm7xx_kcs_reg *reg;
> @@ -76,9 +78,14 @@ static const struct npcm7xx_kcs_reg npcm7xx_kcs_reg_tbl[KCS_CHANNEL_MAX] = {
> { .sts = KCS3ST, .dob = KCS3DO, .dib = KCS3DI, .ctl = KCS3CTL, .ie = KCS3IE },
> };
>
> +static inline struct npcm7xx_kcs_bmc *to_npcm7xx_kcs_bmc(struct kcs_bmc *kcs_bmc)
> +{
> + return container_of(kcs_bmc, struct npcm7xx_kcs_bmc, kcs_bmc);
> +}
> +
> static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
> {
> - struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
> u32 val = 0;
> int rc;
>
> @@ -90,7 +97,7 @@ static u8 npcm7xx_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg)
>
> static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
> {
> - struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_write(priv->map, reg, data);
> @@ -99,7 +106,7 @@ static void npcm7xx_kcs_outb(struct kcs_bmc *kcs_bmc, u32 reg, u8 data)
>
> static void npcm7xx_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 data)
> {
> - struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
> int rc;
>
> rc = regmap_update_bits(priv->map, reg, mask, data);
> @@ -108,7 +115,7 @@ static void npcm7xx_kcs_updateb(struct kcs_bmc *kcs_bmc, u32 reg, u8 mask, u8 da
>
> static void npcm7xx_kcs_enable_channel(struct kcs_bmc *kcs_bmc, bool enable)
> {
> - struct npcm7xx_kcs_bmc *priv = kcs_bmc_priv(kcs_bmc);
> + struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc);
>
> regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE,
> enable ? KCS_CTL_IBFIE : 0);
> @@ -155,11 +162,10 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> - kcs_bmc = kcs_bmc_alloc(dev, sizeof(*priv), chan);
> - if (!kcs_bmc)
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> return -ENOMEM;
>
> - priv = kcs_bmc_priv(kcs_bmc);
> priv->map = syscon_node_to_regmap(dev->parent->of_node);
> if (IS_ERR(priv->map)) {
> dev_err(dev, "Couldn't get regmap\n");
> @@ -167,6 +173,9 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
> }
> priv->reg = &npcm7xx_kcs_reg_tbl[chan - 1];
>
> + kcs_bmc = &priv->kcs_bmc;
> + kcs_bmc->dev = &pdev->dev;
> + kcs_bmc->channel = chan;
> kcs_bmc->ioreg.idr = priv->reg->dib;
> kcs_bmc->ioreg.odr = priv->reg->dob;
> kcs_bmc->ioreg.str = priv->reg->sts;
> @@ -174,31 +183,29 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev)
> kcs_bmc->io_outputb = npcm7xx_kcs_outb;
> kcs_bmc->io_updateb = npcm7xx_kcs_updateb;
>
> - dev_set_drvdata(dev, kcs_bmc);
> + platform_set_drvdata(pdev, priv);
>
> npcm7xx_kcs_enable_channel(kcs_bmc, true);
> rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
> if (rc)
> return rc;
>
> - rc = misc_register(&kcs_bmc->miscdev);
> - if (rc) {
> - dev_err(dev, "Unable to register device\n");
> - return rc;
> - }
>
> pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n",
> chan,
> kcs_bmc->ioreg.idr, kcs_bmc->ioreg.odr, kcs_bmc->ioreg.str);
>
> + kcs_bmc_add_device(kcs_bmc);
> +
> return 0;
> }
>
> static int npcm7xx_kcs_remove(struct platform_device *pdev)
> {
> - struct kcs_bmc *kcs_bmc = dev_get_drvdata(&pdev->dev);
> + struct npcm7xx_kcs_bmc *priv = platform_get_drvdata(pdev);
> + struct kcs_bmc *kcs_bmc = &priv->kcs_bmc;
>
> - misc_deregister(&kcs_bmc->miscdev);
> + kcs_bmc_remove_device(kcs_bmc);
>
> return 0;
> }
> --
> 2.27.0
>
On Sat, 22 May 2021, at 02:44, Corey Minyard wrote:
> On Mon, May 10, 2021 at 03:12:02PM +0930, Andrew Jeffery wrote:
> > Make the KCS device drivers responsible for allocating their own memory.
> >
> > Until now the private data for the device driver was allocated internal
> > to the private data for the chardev interface. This coupling required
> > the slightly awkward API of passing through the struct size for the
> > driver private data to the chardev constructor, and then retrieving a
> > pointer to the driver private data from the allocated chardev memory.
> >
> > In addition to being awkward, the arrangement prevents the
> > implementation of alternative userspace interfaces as the device driver
> > private data is not independent.
> >
> > Peel a layer off the onion and turn the data-structures inside out by
> > exploiting container_of() and embedding `struct kcs_device` in the
> > driver private data.
>
> All in all a very nice cleanup. A few nits inline.
>
> >
> > Signed-off-by: Andrew Jeffery <[email protected]>
> > Reviewed-by: Zev Weiss <[email protected]>
> > ---
> > drivers/char/ipmi/kcs_bmc.c | 19 +++++++--
> > drivers/char/ipmi/kcs_bmc.h | 12 ++----
> > drivers/char/ipmi/kcs_bmc_aspeed.c | 56 +++++++++++++------------
> > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
> > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++++++++++-------
> > 5 files changed, 111 insertions(+), 73 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> > index ef5c48ffe74a..83da681bf49e 100644
> > --- a/drivers/char/ipmi/kcs_bmc.c
> > +++ b/drivers/char/ipmi/kcs_bmc.c
> > @@ -44,12 +44,23 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> > }
> > EXPORT_SYMBOL(kcs_bmc_handle_event);
> >
> > -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
> > -struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
> > +int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
>
> The above (and it's remove function) should be in an include file.
This is a short-term hack while I'm refactoring the code. It goes away
in a later patch when we switch to using an ops struct.
I didn't move it to a header as it's an implementation detail at the
end of the day. I see headers as describing a public interface, and in
the bigger picture this function isn't part of the public API. But
maybe it's too tricky by half. My approach here generated some
discussion with Zev as well.
>
> > +void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
>
> This should return an error so the probe can be failed and cleaned up
> and so confusing message don't get printed after this in one case.
Hmm. I did this because the end result of the series is that we can
have multiple chardev interfaces in distinct modules exposing the one
KCS device in the one kernel. If more than one of the chardev modules
is configured in and one of them fails to initialise themselves with
respect to the device driver I didn't think it was right to fail the
probe of the device driver (and thus remove any chardev interfaces that
did succeed to initialise against it).
But this does limit the usefulness of the device driver instance in the
case that only one of the chardev interfaces is configured in and it
fails to initialise.
So I think we need to decide on the direction before I adjust the
interface here. The patches are architected around the idea of multiple
chardevs being configured in to the kernel build and all are exposed at
runtime.
The serio subsystem does have the 'drvctl' sysfs knob that allows
userspace to dictate which serio chardev interface they want to connect
to a serio device driver. Maybe that's preferred over my "connect them
all" strategy?
Andrew
On Mon, May 24, 2021 at 10:23:36AM +0930, Andrew Jeffery wrote:
>
>
> On Sat, 22 May 2021, at 02:44, Corey Minyard wrote:
> > On Mon, May 10, 2021 at 03:12:02PM +0930, Andrew Jeffery wrote:
> > > Make the KCS device drivers responsible for allocating their own memory.
> > >
> > > Until now the private data for the device driver was allocated internal
> > > to the private data for the chardev interface. This coupling required
> > > the slightly awkward API of passing through the struct size for the
> > > driver private data to the chardev constructor, and then retrieving a
> > > pointer to the driver private data from the allocated chardev memory.
> > >
> > > In addition to being awkward, the arrangement prevents the
> > > implementation of alternative userspace interfaces as the device driver
> > > private data is not independent.
> > >
> > > Peel a layer off the onion and turn the data-structures inside out by
> > > exploiting container_of() and embedding `struct kcs_device` in the
> > > driver private data.
> >
> > All in all a very nice cleanup. A few nits inline.
> >
> > >
> > > Signed-off-by: Andrew Jeffery <[email protected]>
> > > Reviewed-by: Zev Weiss <[email protected]>
> > > ---
> > > drivers/char/ipmi/kcs_bmc.c | 19 +++++++--
> > > drivers/char/ipmi/kcs_bmc.h | 12 ++----
> > > drivers/char/ipmi/kcs_bmc_aspeed.c | 56 +++++++++++++------------
> > > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
> > > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++++++++++-------
> > > 5 files changed, 111 insertions(+), 73 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> > > index ef5c48ffe74a..83da681bf49e 100644
> > > --- a/drivers/char/ipmi/kcs_bmc.c
> > > +++ b/drivers/char/ipmi/kcs_bmc.c
> > > @@ -44,12 +44,23 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> > > }
> > > EXPORT_SYMBOL(kcs_bmc_handle_event);
> > >
> > > -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
> > > -struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
> > > +int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
> >
> > The above (and it's remove function) should be in an include file.
>
> This is a short-term hack while I'm refactoring the code. It goes away
> in a later patch when we switch to using an ops struct.
>
> I didn't move it to a header as it's an implementation detail at the
> end of the day. I see headers as describing a public interface, and in
> the bigger picture this function isn't part of the public API. But
> maybe it's too tricky by half. My approach here generated some
> discussion with Zev as well.
>
> >
> > > +void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> >
> > This should return an error so the probe can be failed and cleaned up
> > and so confusing message don't get printed after this in one case.
>
> Hmm. I did this because the end result of the series is that we can
> have multiple chardev interfaces in distinct modules exposing the one
> KCS device in the one kernel. If more than one of the chardev modules
> is configured in and one of them fails to initialise themselves with
> respect to the device driver I didn't think it was right to fail the
> probe of the device driver (and thus remove any chardev interfaces that
> did succeed to initialise against it).
>
> But this does limit the usefulness of the device driver instance in the
> case that only one of the chardev interfaces is configured in and it
> fails to initialise.
>
> So I think we need to decide on the direction before I adjust the
> interface here. The patches are architected around the idea of multiple
> chardevs being configured in to the kernel build and all are exposed at
> runtime.
Ok, I understand. The host IPMI driver will attempt to start all
interfaces, if none fail to come up it will return an error, but if any
come up it will not return an error. So it's a similar situation.
I stole that from something else, but I can't remember what. I don't
know what the best policy is, really, that was kind of a compromise and
nobody has complained about it.
I will say that the success print in aspeed_kcs_probe() needs to not
happen if there is a failure, though.
-corey
>
> The serio subsystem does have the 'drvctl' sysfs knob that allows
> userspace to dictate which serio chardev interface they want to connect
> to a serio device driver. Maybe that's preferred over my "connect them
> all" strategy?
>
> Andrew
>
>
> _______________________________________________
> Openipmi-developer mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer
On Tue, 25 May 2021, at 01:11, Corey Minyard wrote:
> On Mon, May 24, 2021 at 10:23:36AM +0930, Andrew Jeffery wrote:
> >
> >
> > On Sat, 22 May 2021, at 02:44, Corey Minyard wrote:
> > > On Mon, May 10, 2021 at 03:12:02PM +0930, Andrew Jeffery wrote:
> > > > Make the KCS device drivers responsible for allocating their own memory.
> > > >
> > > > Until now the private data for the device driver was allocated internal
> > > > to the private data for the chardev interface. This coupling required
> > > > the slightly awkward API of passing through the struct size for the
> > > > driver private data to the chardev constructor, and then retrieving a
> > > > pointer to the driver private data from the allocated chardev memory.
> > > >
> > > > In addition to being awkward, the arrangement prevents the
> > > > implementation of alternative userspace interfaces as the device driver
> > > > private data is not independent.
> > > >
> > > > Peel a layer off the onion and turn the data-structures inside out by
> > > > exploiting container_of() and embedding `struct kcs_device` in the
> > > > driver private data.
> > >
> > > All in all a very nice cleanup. A few nits inline.
> > >
> > > >
> > > > Signed-off-by: Andrew Jeffery <[email protected]>
> > > > Reviewed-by: Zev Weiss <[email protected]>
> > > > ---
> > > > drivers/char/ipmi/kcs_bmc.c | 19 +++++++--
> > > > drivers/char/ipmi/kcs_bmc.h | 12 ++----
> > > > drivers/char/ipmi/kcs_bmc_aspeed.c | 56 +++++++++++++------------
> > > > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 60 ++++++++++++++++++---------
> > > > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 37 ++++++++++-------
> > > > 5 files changed, 111 insertions(+), 73 deletions(-)
> > > >
> > > > diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> > > > index ef5c48ffe74a..83da681bf49e 100644
> > > > --- a/drivers/char/ipmi/kcs_bmc.c
> > > > +++ b/drivers/char/ipmi/kcs_bmc.c
> > > > @@ -44,12 +44,23 @@ int kcs_bmc_handle_event(struct kcs_bmc *kcs_bmc)
> > > > }
> > > > EXPORT_SYMBOL(kcs_bmc_handle_event);
> > > >
> > > > -struct kcs_bmc *kcs_bmc_ipmi_alloc(struct device *dev, int sizeof_priv, u32 channel);
> > > > -struct kcs_bmc *kcs_bmc_alloc(struct device *dev, int sizeof_priv, u32 channel)
> > > > +int kcs_bmc_ipmi_add_device(struct kcs_bmc *kcs_bmc);
> > >
> > > The above (and it's remove function) should be in an include file.
> >
> > This is a short-term hack while I'm refactoring the code. It goes away
> > in a later patch when we switch to using an ops struct.
> >
> > I didn't move it to a header as it's an implementation detail at the
> > end of the day. I see headers as describing a public interface, and in
> > the bigger picture this function isn't part of the public API. But
> > maybe it's too tricky by half. My approach here generated some
> > discussion with Zev as well.
> >
> > >
> > > > +void kcs_bmc_add_device(struct kcs_bmc *kcs_bmc)
> > >
> > > This should return an error so the probe can be failed and cleaned up
> > > and so confusing message don't get printed after this in one case.
> >
> > Hmm. I did this because the end result of the series is that we can
> > have multiple chardev interfaces in distinct modules exposing the one
> > KCS device in the one kernel. If more than one of the chardev modules
> > is configured in and one of them fails to initialise themselves with
> > respect to the device driver I didn't think it was right to fail the
> > probe of the device driver (and thus remove any chardev interfaces that
> > did succeed to initialise against it).
> >
> > But this does limit the usefulness of the device driver instance in the
> > case that only one of the chardev interfaces is configured in and it
> > fails to initialise.
> >
> > So I think we need to decide on the direction before I adjust the
> > interface here. The patches are architected around the idea of multiple
> > chardevs being configured in to the kernel build and all are exposed at
> > runtime.
>
> Ok, I understand. The host IPMI driver will attempt to start all
> interfaces, if none fail to come up it will return an error, but if any
> come up it will not return an error. So it's a similar situation.
That sounds reasonable. I'll implement this strategy.
>
> I stole that from something else, but I can't remember what. I don't
> know what the best policy is, really, that was kind of a compromise and
> nobody has complained about it.
>
> I will say that the success print in aspeed_kcs_probe() needs to not
> happen if there is a failure, though.
With the strategy you outlined above that's easy enough.
Thanks,
Andrew