| Message ID | 20210510054213.1610760-1-andrew@aj.id.au |
|---|---|
| Headers | show |
| Series | ipmi: Allow raw access to KCS devices | expand |
Hi Corey, On Mon, 10 May 2021, at 15:11, Andrew Jeffery wrote: > Hello, > > This is the 3rd spin of the series refactoring the keyboard-controller-style > device drivers in the IPMI subsystem. > > v2 can be found (in two parts because yay patch workflow mistakes) at: > > Cover letter: > https://lore.kernel.org/linux-arm-kernel/20210319061952.145040-1-andrew@aj.id.au/ > > Patches: > https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-andrew@aj.id.au/ > > Several significant changes in v3: > > 1. The series is rebased onto v5.13-rc1 > > 2. v5.13-rc1 includes Chiawei's patches reworking the LPC devicetree bindings, > so they're no-longer required in the series. > > 3. After some discussion with Arnd[1] and investigating the serio subsystem, > I've replaced the "raw" KCS driver (patch 16/21 in v2) with a serio adaptor > (patch 11/16 in this series). The adaptor allows us to take advantage of the > existing chardevs provided by serio. > > [1] > https://lore.kernel.org/linux-arm-kernel/37e75b07-a5c6-422f-84b3-54f2bea0b917@www.fastmail.com/ > > Finally, I've also addressed Zev Weiss' review comments where I thought it was > required. These comments covered a lot of minor issues across (almost) all the > patches, so it's best to review from a clean slate rather than attempt to review > the differences between spins. I backported this series for OpenBMC and posting those patches provoked some feedback: * A bug identified in patch 9/18 for the Nuvoton driver where we enable the OBE interrupt: https://lore.kernel.org/openbmc/HK2PR03MB4371F006185ADBBF812A5892AE509@HK2PR03MB4371.apcprd03.prod.outlook.com/ * A discussion on patch 10/18 about lifting the single-open constraint https://lore.kernel.org/openbmc/CAPnigKku-EjOnV9gsmnXzH=XZxSU78iLeccNbsK8k2_4b4UwSg@mail.gmail.com/ I need to do a v4 to fix the bug in the Nuvoton driver. Did you have any feedback for the remaining patches or thoughts on the discussions linked above? I'd like to incorporate whatever I can into the series before respinning. Cheers, Andrew
On Thu, May 20, 2021 at 04:21:31PM +0930, Andrew Jeffery wrote: > Hi Corey, > > On Mon, 10 May 2021, at 15:11, Andrew Jeffery wrote: > > Hello, > > > > This is the 3rd spin of the series refactoring the keyboard-controller-style > > device drivers in the IPMI subsystem. > > > > v2 can be found (in two parts because yay patch workflow mistakes) at: > > > > Cover letter: > > https://lore.kernel.org/linux-arm-kernel/20210319061952.145040-1-andrew@aj.id.au/ > > > > Patches: > > https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-andrew@aj.id.au/ > > > > Several significant changes in v3: > > > > 1. The series is rebased onto v5.13-rc1 > > > > 2. v5.13-rc1 includes Chiawei's patches reworking the LPC devicetree bindings, > > so they're no-longer required in the series. > > > > 3. After some discussion with Arnd[1] and investigating the serio subsystem, > > I've replaced the "raw" KCS driver (patch 16/21 in v2) with a serio adaptor > > (patch 11/16 in this series). The adaptor allows us to take advantage of the > > existing chardevs provided by serio. > > > > [1] > > https://lore.kernel.org/linux-arm-kernel/37e75b07-a5c6-422f-84b3-54f2bea0b917@www.fastmail.com/ > > > > Finally, I've also addressed Zev Weiss' review comments where I thought it was > > required. These comments covered a lot of minor issues across (almost) all the > > patches, so it's best to review from a clean slate rather than attempt to review > > the differences between spins. > > I backported this series for OpenBMC and posting those patches provoked > some feedback: > > * A bug identified in patch 9/18 for the Nuvoton driver where we enable > the OBE interrupt: > > https://lore.kernel.org/openbmc/HK2PR03MB4371F006185ADBBF812A5892AE509@HK2PR03MB4371.apcprd03.prod.outlook.com/ > > * A discussion on patch 10/18 about lifting the single-open constraint > > https://lore.kernel.org/openbmc/CAPnigKku-EjOnV9gsmnXzH=XZxSU78iLeccNbsK8k2_4b4UwSg@mail.gmail.com/ > > I need to do a v4 to fix the bug in the Nuvoton driver. Did you have any > feedback for the remaining patches or thoughts on the discussions linked > above? I'd like to incorporate whatever I can into the series before > respinning. This will take a little while to review, but I'll try to get to it today. Thanks, -corey > > Cheers, > > Andrew
On Mon, May 10, 2021 at 12:41:58AM CDT, Andrew Jeffery wrote: >Unpack and remove the aspeed_kcs_probe_of_v[12]() functions to aid >rearranging how the private device-driver memory is allocated. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > drivers/char/ipmi/kcs_bmc_aspeed.c | 154 ++++++++++++++--------------- > 1 file changed, 76 insertions(+), 78 deletions(-) > >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >index eefe362f65f0..c94d36e195be 100644 >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >@@ -13,6 +13,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_address.h> >+#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/poll.h> > #include <linux/regmap.h> >@@ -63,6 +64,10 @@ struct aspeed_kcs_bmc { > struct regmap *map; > }; > >+struct aspeed_kcs_of_ops { >+ int (*get_channel)(struct platform_device *pdev); >+ int (*get_io_address)(struct platform_device *pdev); >+}; > > static u8 aspeed_kcs_inb(struct kcs_bmc *kcs_bmc, u32 reg) > { >@@ -231,13 +236,10 @@ static const struct kcs_ioreg ast_kcs_bmc_ioregs[KCS_CHANNEL_MAX] = { > { .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 }, > }; > >-static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev) >+static int aspeed_kcs_of_v1_get_channel(struct platform_device *pdev) > { >- struct aspeed_kcs_bmc *priv; > struct device_node *np; >- struct kcs_bmc *kcs; > u32 channel; >- u32 slave; > int rc; > > np = pdev->dev.of_node; >@@ -245,105 +247,79 @@ static struct kcs_bmc *aspeed_kcs_probe_of_v1(struct platform_device *pdev) > rc = of_property_read_u32(np, "kcs_chan", &channel); > if ((rc != 0) || (channel == 0 || channel > KCS_CHANNEL_MAX)) { > dev_err(&pdev->dev, "no valid 'kcs_chan' configured\n"); >- return ERR_PTR(-EINVAL); >+ return -EINVAL; > } > >- kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel); >- if (!kcs) >- return ERR_PTR(-ENOMEM); >+ return channel; >+} > >- priv = kcs_bmc_priv(kcs); >- 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"); >- return ERR_PTR(-ENODEV); >- } >+static int aspeed_kcs_of_v1_get_io_address(struct platform_device *pdev) >+{ >+ u32 slave; >+ int rc; > >- rc = of_property_read_u32(np, "kcs_addr", &slave); >- if (rc) { >+ rc = of_property_read_u32(pdev->dev.of_node, "kcs_addr", &slave); >+ if (rc || slave > 0xffff) { > dev_err(&pdev->dev, "no valid 'kcs_addr' configured\n"); >- return ERR_PTR(-EINVAL); >+ return -EINVAL; > } > >- kcs->ioreg = ast_kcs_bmc_ioregs[channel - 1]; >- aspeed_kcs_set_address(kcs, slave); >- >- return kcs; >-} >- >-static int aspeed_kcs_calculate_channel(const struct kcs_ioreg *regs) >-{ >- int i; >- >- for (i = 0; i < ARRAY_SIZE(ast_kcs_bmc_ioregs); i++) { >- if (!memcmp(&ast_kcs_bmc_ioregs[i], regs, sizeof(*regs))) >- return i + 1; >- } >- >- return -EINVAL; >+ return slave; > } > >-static struct kcs_bmc *aspeed_kcs_probe_of_v2(struct platform_device *pdev) >+static int aspeed_kcs_of_v2_get_channel(struct platform_device *pdev) > { >- struct aspeed_kcs_bmc *priv; > struct device_node *np; > struct kcs_ioreg ioreg; >- struct kcs_bmc *kcs; > const __be32 *reg; >- int channel; >- u32 slave; >- int rc; >+ int i; > > np = pdev->dev.of_node; > > /* Don't translate addresses, we want offsets for the regmaps */ > reg = of_get_address(np, 0, NULL, NULL); > if (!reg) >- return ERR_PTR(-EINVAL); >+ return -EINVAL; > ioreg.idr = be32_to_cpup(reg); > > reg = of_get_address(np, 1, NULL, NULL); > if (!reg) >- return ERR_PTR(-EINVAL); >+ return -EINVAL; > ioreg.odr = be32_to_cpup(reg); > > reg = of_get_address(np, 2, NULL, NULL); > if (!reg) >- return ERR_PTR(-EINVAL); >+ return -EINVAL; > ioreg.str = be32_to_cpup(reg); > >- channel = aspeed_kcs_calculate_channel(&ioreg); >- if (channel < 0) >- return ERR_PTR(channel); >- >- kcs = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel); >- if (!kcs) >- return ERR_PTR(-ENOMEM); >- >- kcs->ioreg = ioreg; >- >- priv = kcs_bmc_priv(kcs); >- 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"); >- return ERR_PTR(-ENODEV); >+ for (i = 0; i < ARRAY_SIZE(ast_kcs_bmc_ioregs); i++) { >+ if (!memcmp(&ast_kcs_bmc_ioregs[i], &ioreg, sizeof(ioreg))) >+ return i + 1; > } > >- rc = of_property_read_u32(np, "aspeed,lpc-io-reg", &slave); >- if (rc) >- return ERR_PTR(rc); >+ return -EINVAL; >+} > >- aspeed_kcs_set_address(kcs, slave); >+static int aspeed_kcs_of_v2_get_io_address(struct platform_device *pdev) >+{ >+ uint32_t slave; >+ int rc; > >- return kcs; >+ rc = of_property_read_u32(pdev->dev.of_node, "aspeed,lpc-io-reg", &slave); >+ if (rc || slave > 0xffff) >+ return -EINVAL; The v1 get_io_address() function prints an error in this case; it might be nice to do so here as well? (Ideally maintained/extended as appropriate when this function gets adjusted in patch 16.) >+ >+ return slave; > } > > 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; >+ int rc, channel, addr; > > np = dev->of_node->parent; > if (!of_device_is_compatible(np, "aspeed,ast2400-lpc-v2") && >@@ -352,23 +328,35 @@ static int aspeed_kcs_probe(struct platform_device *pdev) > dev_err(dev, "unsupported LPC device binding\n"); > return -ENODEV; > } >- >- np = dev->of_node; >- if (of_device_is_compatible(np, "aspeed,ast2400-kcs-bmc") || >- of_device_is_compatible(np, "aspeed,ast2500-kcs-bmc")) >- kcs_bmc = aspeed_kcs_probe_of_v1(pdev); >- else if (of_device_is_compatible(np, "aspeed,ast2400-kcs-bmc-v2") || >- of_device_is_compatible(np, "aspeed,ast2500-kcs-bmc-v2")) >- kcs_bmc = aspeed_kcs_probe_of_v2(pdev); >- else >+ ops = of_device_get_match_data(&pdev->dev); >+ if (!ops) > return -EINVAL; > >- if (IS_ERR(kcs_bmc)) >- return PTR_ERR(kcs_bmc); >+ channel = ops->get_channel(pdev); >+ if (channel < 0) >+ return channel; > >+ kcs_bmc = kcs_bmc_alloc(&pdev->dev, sizeof(struct aspeed_kcs_bmc), channel); >+ if (!kcs_bmc) >+ return -ENOMEM; >+ >+ kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1]; > kcs_bmc->io_inputb = aspeed_kcs_inb; > kcs_bmc->io_outputb = aspeed_kcs_outb; > >+ 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"); >+ return -ENODEV; >+ } >+ >+ aspeed_kcs_set_address(kcs_bmc, addr); >+ > rc = aspeed_kcs_config_irq(kcs_bmc, pdev); > if (rc) > return rc; >@@ -400,11 +388,21 @@ static int aspeed_kcs_remove(struct platform_device *pdev) > return 0; > } > >+static const struct aspeed_kcs_of_ops of_v1_ops = { >+ .get_channel = aspeed_kcs_of_v1_get_channel, >+ .get_io_address = aspeed_kcs_of_v1_get_io_address, >+}; >+ >+static const struct aspeed_kcs_of_ops of_v2_ops = { >+ .get_channel = aspeed_kcs_of_v2_get_channel, >+ .get_io_address = aspeed_kcs_of_v2_get_io_address, >+}; >+ > static const struct of_device_id ast_kcs_bmc_match[] = { >- { .compatible = "aspeed,ast2400-kcs-bmc" }, >- { .compatible = "aspeed,ast2500-kcs-bmc" }, >- { .compatible = "aspeed,ast2400-kcs-bmc-v2" }, >- { .compatible = "aspeed,ast2500-kcs-bmc-v2" }, >+ { .compatible = "aspeed,ast2400-kcs-bmc", .data = &of_v1_ops }, >+ { .compatible = "aspeed,ast2500-kcs-bmc", .data = &of_v1_ops }, >+ { .compatible = "aspeed,ast2400-kcs-bmc-v2", .data = &of_v2_ops }, >+ { .compatible = "aspeed,ast2500-kcs-bmc-v2", .data = &of_v2_ops }, > { } > }; > MODULE_DEVICE_TABLE(of, ast_kcs_bmc_match); >-- >2.27.0 >
On Mon, May 10, 2021 at 12:42:04AM CDT, Andrew Jeffery wrote: >Move all client-private data out of `struct kcs_bmc` into the KCS client >implementation. > >With this change the KCS BMC core code now only concerns itself with >abstract `struct kcs_bmc` and `struct kcs_bmc_client` types, achieving >expected separation of concerns. Further, the change clears the path for >implementation of alternative userspace interfaces. > >The chardev data-structures are rearranged in the same manner applied to >the KCS device driver data-structures in an earlier patch - `struct >kcs_bmc_client` is embedded in the client's private data and we exploit >container_of() to translate as required. > >Finally, now that it is free of client data, `struct kcs_bmc` is renamed >to `struct kcs_bmc_device` to contrast `struct kcs_bmc_client`. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Zev Weiss <zweiss@equinix.com>
On Mon, May 10, 2021 at 12:42:06AM CDT, Andrew Jeffery wrote: >Add a mechanism for controlling whether the client associated with a >KCS device will receive Input Buffer Full (IBF) and Output Buffer Empty >(OBE) events. This enables an abstract implementation of poll() for KCS >devices. > >A wart in the implementation is that the ASPEED KCS devices don't >support an OBE interrupt for the BMC. Instead we pretend it has one by >polling the status register waiting for the Output Buffer Full (OBF) bit >to clear, and generating an event when OBE is observed. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > drivers/char/ipmi/kcs_bmc.c | 6 ++ > drivers/char/ipmi/kcs_bmc.h | 3 + > drivers/char/ipmi/kcs_bmc_aspeed.c | 150 ++++++++++++++++++---------- > drivers/char/ipmi/kcs_bmc_client.h | 2 + > drivers/char/ipmi/kcs_bmc_device.h | 1 + > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 25 ++++- > 6 files changed, 130 insertions(+), 57 deletions(-) > >diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c >index 2ec8312ce766..7081541bb6ce 100644 >--- a/drivers/char/ipmi/kcs_bmc.c >+++ b/drivers/char/ipmi/kcs_bmc.c >@@ -176,6 +176,12 @@ void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv) > } > EXPORT_SYMBOL(kcs_bmc_unregister_driver); > >+void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events) >+{ >+ kcs_bmc->ops->irq_mask_update(kcs_bmc, mask, events); >+} >+EXPORT_SYMBOL(kcs_bmc_update_event_mask); >+ > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Haiyue Wang <haiyue.wang@linux.intel.com>"); > MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); >diff --git a/drivers/char/ipmi/kcs_bmc.h b/drivers/char/ipmi/kcs_bmc.h >index 3f0cce315b4f..fa408b802c79 100644 >--- a/drivers/char/ipmi/kcs_bmc.h >+++ b/drivers/char/ipmi/kcs_bmc.h >@@ -8,6 +8,9 @@ > > #include <linux/list.h> > >+#define KCS_BMC_EVENT_TYPE_OBE BIT(0) >+#define KCS_BMC_EVENT_TYPE_IBF BIT(1) >+ > #define KCS_BMC_STR_OBF BIT(0) > #define KCS_BMC_STR_IBF BIT(1) > #define KCS_BMC_STR_CMD_DAT BIT(3) >diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c >index 8e00675d1019..8b223e58d900 100644 >--- a/drivers/char/ipmi/kcs_bmc_aspeed.c >+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c >@@ -60,10 +60,18 @@ > #define LPC_ODR4 0x118 > #define LPC_STR4 0x11C > >+#define OBE_POLL_PERIOD (HZ / 2) >+ > struct aspeed_kcs_bmc { > struct kcs_bmc_device kcs_bmc; > > struct regmap *map; >+ >+ struct { >+ spinlock_t lock; >+ bool remove; >+ struct timer_list timer; >+ } obe; > }; > > struct aspeed_kcs_of_ops { >@@ -159,68 +167,89 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc_device *kcs_bmc, bool enabl > > switch (kcs_bmc->channel) { > case 1: >- if (enable) { >- regmap_update_bits(priv->map, LPC_HICR2, >- LPC_HICR2_IBFIF1, LPC_HICR2_IBFIF1); >- regmap_update_bits(priv->map, LPC_HICR0, >- LPC_HICR0_LPC1E, LPC_HICR0_LPC1E); >- } else { >- regmap_update_bits(priv->map, LPC_HICR0, >- LPC_HICR0_LPC1E, 0); >- regmap_update_bits(priv->map, LPC_HICR2, >- LPC_HICR2_IBFIF1, 0); >- } >- break; >- >+ regmap_update_bits(priv->map, LPC_HICR0, LPC_HICR0_LPC1E, enable * LPC_HICR0_LPC1E); >+ return; > case 2: >- if (enable) { >- regmap_update_bits(priv->map, LPC_HICR2, >- LPC_HICR2_IBFIF2, LPC_HICR2_IBFIF2); >- regmap_update_bits(priv->map, LPC_HICR0, >- LPC_HICR0_LPC2E, LPC_HICR0_LPC2E); >- } else { >- regmap_update_bits(priv->map, LPC_HICR0, >- LPC_HICR0_LPC2E, 0); >- regmap_update_bits(priv->map, LPC_HICR2, >- LPC_HICR2_IBFIF2, 0); >- } >- break; >- >+ regmap_update_bits(priv->map, LPC_HICR0, LPC_HICR0_LPC2E, enable * LPC_HICR0_LPC2E); >+ return; > case 3: >- if (enable) { >- regmap_update_bits(priv->map, LPC_HICR2, >- LPC_HICR2_IBFIF3, LPC_HICR2_IBFIF3); >- regmap_update_bits(priv->map, LPC_HICR0, >- LPC_HICR0_LPC3E, LPC_HICR0_LPC3E); >- regmap_update_bits(priv->map, LPC_HICR4, >- LPC_HICR4_KCSENBL, LPC_HICR4_KCSENBL); >- } else { >- regmap_update_bits(priv->map, LPC_HICR0, >- LPC_HICR0_LPC3E, 0); >- regmap_update_bits(priv->map, LPC_HICR4, >- LPC_HICR4_KCSENBL, 0); >- regmap_update_bits(priv->map, LPC_HICR2, >- LPC_HICR2_IBFIF3, 0); >- } >- break; >- >+ regmap_update_bits(priv->map, LPC_HICR0, LPC_HICR0_LPC3E, enable * LPC_HICR0_LPC3E); >+ regmap_update_bits(priv->map, LPC_HICR4, >+ LPC_HICR4_KCSENBL, enable * LPC_HICR4_KCSENBL); >+ return; > case 4: >- if (enable) >- regmap_update_bits(priv->map, LPC_HICRB, >- LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E, >- LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E); >+ regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_LPC4E, enable * LPC_HICRB_LPC4E); >+ return; >+ default: >+ pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel); >+ return; >+ } >+} >+ >+static void aspeed_kcs_check_obe(struct timer_list *timer) >+{ >+ struct aspeed_kcs_bmc *priv = container_of(timer, struct aspeed_kcs_bmc, obe.timer); >+ unsigned long flags; >+ u8 str; >+ >+ spin_lock_irqsave(&priv->obe.lock, flags); >+ if (priv->obe.remove) { >+ spin_unlock_irqrestore(&priv->obe.lock, flags); >+ return; >+ } >+ >+ str = aspeed_kcs_inb(&priv->kcs_bmc, priv->kcs_bmc.ioreg.str); >+ if (str & KCS_BMC_STR_OBF) { >+ mod_timer(timer, jiffies + OBE_POLL_PERIOD); >+ spin_unlock_irqrestore(&priv->obe.lock, flags); >+ return; >+ } >+ spin_unlock_irqrestore(&priv->obe.lock, flags); >+ >+ kcs_bmc_handle_event(&priv->kcs_bmc); >+} >+ >+static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) >+{ >+ struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc); >+ >+ /* We don't have an OBE IRQ, emulate it */ >+ if (mask & KCS_BMC_EVENT_TYPE_OBE) { >+ if (KCS_BMC_EVENT_TYPE_OBE & state) >+ mod_timer(&priv->obe.timer, jiffies + OBE_POLL_PERIOD); > else >- regmap_update_bits(priv->map, LPC_HICRB, >- LPC_HICRB_IBFIF4 | LPC_HICRB_LPC4E, >- 0); >- break; >+ del_timer(&priv->obe.timer); >+ } > >- default: >- break; >+ if (mask & KCS_BMC_EVENT_TYPE_IBF) { >+ const bool enable = !!(state & KCS_BMC_EVENT_TYPE_IBF); >+ >+ switch (kcs_bmc->channel) { >+ case 1: >+ regmap_update_bits(priv->map, LPC_HICR2, LPC_HICR2_IBFIF1, >+ enable * LPC_HICR2_IBFIF1); >+ return; >+ case 2: >+ regmap_update_bits(priv->map, LPC_HICR2, LPC_HICR2_IBFIF2, >+ enable * LPC_HICR2_IBFIF2); >+ return; >+ case 3: >+ regmap_update_bits(priv->map, LPC_HICR2, LPC_HICR2_IBFIF3, >+ enable * LPC_HICR2_IBFIF3); >+ return; >+ case 4: >+ regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_IBFIF4, >+ enable * LPC_HICRB_IBFIF4); >+ return; >+ default: >+ pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel); >+ return; >+ } > } > } > > static const struct kcs_bmc_device_ops aspeed_kcs_ops = { >+ .irq_mask_update = aspeed_kcs_irq_mask_update, > .io_inputb = aspeed_kcs_inb, > .io_outputb = aspeed_kcs_outb, > .io_updateb = aspeed_kcs_updateb, >@@ -373,6 +402,10 @@ static int aspeed_kcs_probe(struct platform_device *pdev) > return -ENODEV; > } > >+ spin_lock_init(&priv->obe.lock); >+ priv->obe.remove = false; >+ timer_setup(&priv->obe.timer, aspeed_kcs_check_obe, 0); >+ > aspeed_kcs_set_address(kcs_bmc, addr); > > rc = aspeed_kcs_config_irq(kcs_bmc, pdev); >@@ -381,6 +414,8 @@ static int aspeed_kcs_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, priv); > >+ aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), >+ KCS_BMC_EVENT_TYPE_IBF); > aspeed_kcs_enable_channel(kcs_bmc, true); > > kcs_bmc_add_device(&priv->kcs_bmc); >@@ -397,6 +432,15 @@ static int aspeed_kcs_remove(struct platform_device *pdev) > > kcs_bmc_remove_device(kcs_bmc); > >+ aspeed_kcs_enable_channel(kcs_bmc, false); >+ aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0); >+ >+ /* Make sure it's proper dead */ >+ spin_lock_irq(&priv->obe.lock); >+ priv->obe.remove = true; >+ spin_unlock_irq(&priv->obe.lock); >+ del_timer_sync(&priv->obe.timer); >+ > return 0; > } > >diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/drivers/char/ipmi/kcs_bmc_client.h >index cb38b56cda85..d14e8e0fa230 100644 >--- a/drivers/char/ipmi/kcs_bmc_client.h >+++ b/drivers/char/ipmi/kcs_bmc_client.h >@@ -38,6 +38,8 @@ void kcs_bmc_unregister_driver(struct kcs_bmc_driver *drv); > int kcs_bmc_enable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > void kcs_bmc_disable_device(struct kcs_bmc_device *kcs_bmc, struct kcs_bmc_client *client); > >+void kcs_bmc_update_event_mask(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 events); >+ > u8 kcs_bmc_read_data(struct kcs_bmc_device *kcs_bmc); > void kcs_bmc_write_data(struct kcs_bmc_device *kcs_bmc, u8 data); > u8 kcs_bmc_read_status(struct kcs_bmc_device *kcs_bmc); >diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/drivers/char/ipmi/kcs_bmc_device.h >index dd90f8c2ebd6..a1410eb16308 100644 >--- a/drivers/char/ipmi/kcs_bmc_device.h >+++ b/drivers/char/ipmi/kcs_bmc_device.h >@@ -9,6 +9,7 @@ > #include "kcs_bmc.h" > > struct kcs_bmc_device_ops { >+ void (*irq_mask_update)(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 enable); > u8 (*io_inputb)(struct kcs_bmc_device *kcs_bmc, u32 reg); > void (*io_outputb)(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 b); > void (*io_updateb)(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 mask, u8 b); >diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c >index 9f0b168e487c..f8b7162fb830 100644 >--- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c >+++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c >@@ -38,6 +38,7 @@ > #define KCS2CTL 0x2A > #define KCS3CTL 0x3C > #define KCS_CTL_IBFIE BIT(0) >+#define KCS_CTL_OBEIE BIT(0) > > #define KCS1IE 0x1C > #define KCS2IE 0x2E >@@ -117,13 +118,23 @@ static void npcm7xx_kcs_enable_channel(struct kcs_bmc_device *kcs_bmc, bool enab > { > 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); >- > regmap_update_bits(priv->map, priv->reg->ie, KCS_IE_IRQE | KCS_IE_HIRQE, > enable ? KCS_IE_IRQE | KCS_IE_HIRQE : 0); > } > >+static void npcm7xx_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 state) >+{ >+ struct npcm7xx_kcs_bmc *priv = to_npcm7xx_kcs_bmc(kcs_bmc); >+ >+ if (KCS_BMC_EVENT_TYPE_OBE & mask) Operand ordering still looks a little backwards here... >+ regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_OBEIE, >+ !!(KCS_BMC_EVENT_TYPE_OBE & state) * KCS_CTL_OBEIE); ...and here... >+ >+ if (KCS_BMC_EVENT_TYPE_IBF & mask) ...and here. Aside from that, Reviewed-by: Zev Weiss <zweiss@equinix.com> >+ regmap_update_bits(priv->map, priv->reg->ctl, KCS_CTL_IBFIE, >+ !!(state & KCS_BMC_EVENT_TYPE_IBF) * KCS_CTL_IBFIE); >+} >+ > static irqreturn_t npcm7xx_kcs_irq(int irq, void *arg) > { > struct kcs_bmc_device *kcs_bmc = arg; >@@ -146,6 +157,7 @@ static int npcm7xx_kcs_config_irq(struct kcs_bmc_device *kcs_bmc, > } > > static const struct kcs_bmc_device_ops npcm7xx_kcs_ops = { >+ .irq_mask_update = npcm7xx_kcs_irq_mask_update, > .io_inputb = npcm7xx_kcs_inb, > .io_outputb = npcm7xx_kcs_outb, > .io_updateb = npcm7xx_kcs_updateb, >@@ -186,11 +198,13 @@ static int npcm7xx_kcs_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, priv); > >- npcm7xx_kcs_enable_channel(kcs_bmc, true); > rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev); > if (rc) > return rc; > >+ npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), >+ KCS_BMC_EVENT_TYPE_IBF); >+ npcm7xx_kcs_enable_channel(kcs_bmc, true); > > pr_info("channel=%u idr=0x%x odr=0x%x str=0x%x\n", > chan, >@@ -208,6 +222,9 @@ static int npcm7xx_kcs_remove(struct platform_device *pdev) > > kcs_bmc_remove_device(kcs_bmc); > >+ npcm7xx_kcs_enable_channel(kcs_bmc, false); >+ npcm7xx_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0); >+ > return 0; > } > >-- >2.27.0 >
On Mon, May 10, 2021 at 12:42:08AM CDT, Andrew Jeffery wrote: >kcs_bmc_serio acts as a bridge between the KCS drivers in the IPMI >subsystem and the existing userspace interfaces available through the >serio subsystem. This is useful when userspace would like to make use of >the BMC KCS devices for purposes that aren't IPMI. > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> >--- > drivers/char/ipmi/Kconfig | 14 +++ > drivers/char/ipmi/Makefile | 1 + > drivers/char/ipmi/kcs_bmc_serio.c | 151 ++++++++++++++++++++++++++++++ > 3 files changed, 166 insertions(+) > create mode 100644 drivers/char/ipmi/kcs_bmc_serio.c > >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig >index bc5f81899b62..249b31197eea 100644 >--- a/drivers/char/ipmi/Kconfig >+++ b/drivers/char/ipmi/Kconfig >@@ -137,6 +137,20 @@ config IPMI_KCS_BMC_CDEV_IPMI > This support is also available as a module. The module will be > called kcs_bmc_cdev_ipmi. > >+config IPMI_KCS_BMC_SERIO >+ depends on IPMI_KCS_BMC && SERIO >+ tristate "SerIO adaptor for BMC KCS devices" >+ help >+ Adapts the BMC KCS device for the SerIO subsystem. This allows users >+ to take advantage of userspace interfaces provided by SerIO where >+ appropriate. >+ >+ Say YES if you wish to expose KCS devices on the BMC via SerIO >+ interfaces. >+ >+ This support is also available as a module. The module will be >+ called kcs_bmc_serio. >+ > config ASPEED_BT_IPMI_BMC > depends on ARCH_ASPEED || COMPILE_TEST > depends on REGMAP && REGMAP_MMIO && MFD_SYSCON >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile >index fcfa676afddb..84f47d18007f 100644 >--- a/drivers/char/ipmi/Makefile >+++ b/drivers/char/ipmi/Makefile >@@ -23,6 +23,7 @@ obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o > obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o >+obj-$(CONFIG_IPMI_KCS_BMC_SERIO) += kcs_bmc_serio.o > obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o > obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o >diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c >new file mode 100644 >index 000000000000..30a2b7ab464b >--- /dev/null >+++ b/drivers/char/ipmi/kcs_bmc_serio.c >@@ -0,0 +1,151 @@ >+// SPDX-License-Identifier: GPL-2.0-or-later >+/* Copyright (c) 2021 IBM Corp. */ >+ >+#include <linux/delay.h> >+#include <linux/device.h> >+#include <linux/errno.h> >+#include <linux/list.h> >+#include <linux/module.h> >+#include <linux/sched/signal.h> >+#include <linux/serio.h> >+#include <linux/slab.h> >+ >+#include "kcs_bmc_client.h" >+ >+struct kcs_bmc_serio { >+ struct list_head entry; >+ >+ struct kcs_bmc_client client; >+ struct serio *port; >+ >+ spinlock_t lock; >+}; >+ >+static inline struct kcs_bmc_serio *client_to_kcs_bmc_serio(struct kcs_bmc_client *client) >+{ >+ return container_of(client, struct kcs_bmc_serio, client); >+} >+ >+static irqreturn_t kcs_bmc_serio_event(struct kcs_bmc_client *client) >+{ >+ struct kcs_bmc_serio *priv; >+ u8 handled = IRQ_NONE; >+ u8 status; >+ >+ priv = client_to_kcs_bmc_serio(client); >+ >+ spin_lock(&priv->lock); >+ >+ status = kcs_bmc_read_status(client->dev); >+ >+ if (status & KCS_BMC_STR_IBF) >+ handled = serio_interrupt(priv->port, kcs_bmc_read_data(client->dev), 0); >+ >+ spin_unlock(&priv->lock); >+ >+ return handled; >+} >+ >+static const struct kcs_bmc_client_ops kcs_bmc_serio_client_ops = { >+ .event = kcs_bmc_serio_event, >+}; >+ >+static int kcs_bmc_serio_open(struct serio *port) >+{ >+ struct kcs_bmc_serio *priv = port->port_data; >+ >+ return kcs_bmc_enable_device(priv->client.dev, &priv->client); >+} >+ >+static void kcs_bmc_serio_close(struct serio *port) >+{ >+ struct kcs_bmc_serio *priv = port->port_data; >+ >+ kcs_bmc_disable_device(priv->client.dev, &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) >+{ >+ struct kcs_bmc_serio *priv; >+ struct serio *port; >+ >+ priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL); >+ port = kzalloc(sizeof(*port), GFP_KERNEL); Is there a particular reason to allocate port with a raw kzalloc() instead of another devm_kzalloc()? >+ if (!(priv && port)) >+ return -ENOMEM; >+ >+ port->id.type = SERIO_8042; >+ port->open = kcs_bmc_serio_open; >+ port->close = kcs_bmc_serio_close; >+ port->port_data = priv; >+ port->dev.parent = kcs_bmc->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); >+ >+ serio_register_port(port); >+ >+ dev_info(kcs_bmc->dev, "Initialised serio client for channel %d", kcs_bmc->channel); >+ >+ return 0; >+} >+ >+static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc) >+{ >+ 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 -ENODEV; >+ >+ serio_unregister_port(priv->port); >+ kcs_bmc_disable_device(kcs_bmc, &priv->client); >+ devm_kfree(priv->client.dev->dev, priv); Looks like the priv->port allocation would leak here I think? Also, is the asymmetry of having kcs_bmc_disable_device() here but no corresponding kcs_bmc_enable_device() in kcs_bmc_serio_add_device() intentional? If so, an explanatory comment of some sort might be nice to add. >+ >+ return 0; >+} >+ >+static const struct kcs_bmc_driver_ops kcs_bmc_serio_driver_ops = { >+ .add_device = kcs_bmc_serio_add_device, >+ .remove_device = kcs_bmc_serio_remove_device, >+}; >+ >+static struct kcs_bmc_driver kcs_bmc_serio_driver = { >+ .ops = &kcs_bmc_serio_driver_ops, >+}; >+ >+static int kcs_bmc_serio_init(void) >+{ >+ kcs_bmc_register_driver(&kcs_bmc_serio_driver); >+ >+ return 0; >+} >+module_init(kcs_bmc_serio_init); >+ >+static void kcs_bmc_serio_exit(void) >+{ >+ kcs_bmc_unregister_driver(&kcs_bmc_serio_driver); >+} >+module_exit(kcs_bmc_serio_exit); >+ >+MODULE_LICENSE("GPL v2"); >+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>"); >+MODULE_DESCRIPTION("Adapter driver for serio access to BMC KCS devices"); >-- >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 <andrew@aj.id.au> > Reviewed-by: Zev Weiss <zweiss@equinix.com> > --- > 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 <haiyue.wang@linux.intel.com>"); > 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 <haiyue.wang@linux.intel.com>"); > 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:11:57PM +0930, Andrew Jeffery wrote: > Hello, > > This is the 3rd spin of the series refactoring the keyboard-controller-style > device drivers in the IPMI subsystem. This is a nice set of cleanups outside of just allowing raw access. I'll let you handle Zev's comments and a few of mine. I almost hate to ask this, but would there be value in allowing the BT driver to use this abstract interface? Or maybe it would be just too hard to get a common abstraction, more work than it's worth. It's surprising that more people don't want BT as it's vastly superior to KCS. Just a thought for now. I guess there's SMIC, but hopefully nobody wants that. -corey > > v2 can be found (in two parts because yay patch workflow mistakes) at: > > Cover letter: > https://lore.kernel.org/linux-arm-kernel/20210319061952.145040-1-andrew@aj.id.au/ > > Patches: > https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-andrew@aj.id.au/ > > Several significant changes in v3: > > 1. The series is rebased onto v5.13-rc1 > > 2. v5.13-rc1 includes Chiawei's patches reworking the LPC devicetree bindings, > so they're no-longer required in the series. > > 3. After some discussion with Arnd[1] and investigating the serio subsystem, > I've replaced the "raw" KCS driver (patch 16/21 in v2) with a serio adaptor > (patch 11/16 in this series). The adaptor allows us to take advantage of the > existing chardevs provided by serio. > > [1] https://lore.kernel.org/linux-arm-kernel/37e75b07-a5c6-422f-84b3-54f2bea0b917@www.fastmail.com/ > > Finally, I've also addressed Zev Weiss' review comments where I thought it was > required. These comments covered a lot of minor issues across (almost) all the > patches, so it's best to review from a clean slate rather than attempt to review > the differences between spins. > > Previously: > > Changes in v2 include: > > * A rebase onto v5.12-rc2 > * Incorporation of off-list feedback on SerIRQ configuration from > Chiawei > * Further validation on hardware for ASPEED KCS devices 2, 3 and 4 > * Lifting the existing single-open constraint of the IPMI chardev > * Fixes addressing Rob's feedback on the conversion of the ASPEED KCS > binding to dt-schema > * Fixes addressing Rob's feedback on the new aspeed,lpc-interrupts > property definition for the ASPEED KCS binding > > Please test and review! > > Andrew > > Andrew Jeffery (16): > ipmi: kcs_bmc_aspeed: Use of match data to extract KCS properties > ipmi: kcs_bmc: Make status update atomic > ipmi: kcs_bmc: Rename {read,write}_{status,data}() functions > ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi > ipmi: kcs_bmc: Turn the driver data-structures inside-out > ipmi: kcs_bmc: Split headers into device and client > ipmi: kcs_bmc: Strip private client data from struct kcs_bmc > ipmi: kcs_bmc: Decouple the IPMI chardev from the core > ipmi: kcs_bmc: Allow clients to control KCS IRQ state > ipmi: kcs_bmc: Don't enforce single-open policy in the kernel > ipmi: kcs_bmc: Add serio adaptor > dt-bindings: ipmi: Convert ASPEED KCS binding to schema > dt-bindings: ipmi: Add optional SerIRQ property to ASPEED KCS devices > ipmi: kcs_bmc_aspeed: Implement KCS SerIRQ configuration > ipmi: kcs_bmc_aspeed: Fix IBFIE typo from datasheet > ipmi: kcs_bmc_aspeed: Optionally apply status address > > .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 106 +++ > .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 - > drivers/char/ipmi/Kconfig | 27 + > drivers/char/ipmi/Makefile | 2 + > drivers/char/ipmi/kcs_bmc.c | 526 ++++----------- > drivers/char/ipmi/kcs_bmc.h | 92 +-- > drivers/char/ipmi/kcs_bmc_aspeed.c | 635 +++++++++++++----- > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 568 ++++++++++++++++ > drivers/char/ipmi/kcs_bmc_client.h | 48 ++ > drivers/char/ipmi/kcs_bmc_device.h | 22 + > drivers/char/ipmi/kcs_bmc_npcm7xx.c | 94 ++- > drivers/char/ipmi/kcs_bmc_serio.c | 151 +++++ > 12 files changed, 1582 insertions(+), 722 deletions(-) > create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt > create mode 100644 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > create mode 100644 drivers/char/ipmi/kcs_bmc_client.h > create mode 100644 drivers/char/ipmi/kcs_bmc_device.h > create mode 100644 drivers/char/ipmi/kcs_bmc_serio.c > > -- > 2.27.0 >
Hi Corey, On Sat, 22 May 2021, at 03:06, Corey Minyard wrote: > On Mon, May 10, 2021 at 03:11:57PM +0930, Andrew Jeffery wrote: > > Hello, > > > > This is the 3rd spin of the series refactoring the keyboard-controller-style > > device drivers in the IPMI subsystem. > > This is a nice set of cleanups outside of just allowing raw access. > I'll let you handle Zev's comments and a few of mine. Thanks for taking the time to review the series. I'll address the comments from you both in v4. > > I almost hate to ask this, but would there be value in allowing the BT > driver to use this abstract interface? Hmm. Possibly, but it's not something I've looked at yet. If we did want to go down that path I don't think it would be too difficult, but I don't have a need to touch the BT side of it right now. > Or maybe it would be just too > hard to get a common abstraction, more work than it's worth. It's > surprising that more people don't want BT as it's vastly superior to > KCS. For bulk data, certainly. However for the use-cases I have I'm using the KCS interface as a control channel that isn't data intensive. Interrupts, a small command set (256 values are more than enough) and a status byte are all I'm really after, so BT is more than I need. Plus for the systems I'm working on we're still using BT for in-band IPMI while we transition to MCTP/PLDM. The current BT implementation is working fine for that :) Cheers, Andrew
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 <andrew@aj.id.au> > > Reviewed-by: Zev Weiss <zweiss@equinix.com> > > --- > > 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 <andrew@aj.id.au> > > > Reviewed-by: Zev Weiss <zweiss@equinix.com> > > > --- > > > 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 > Openipmi-developer@lists.sourceforge.net > 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 <andrew@aj.id.au> > > > > Reviewed-by: Zev Weiss <zweiss@equinix.com> > > > > --- > > > > 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
On Fri, 21 May 2021, at 16:50, Zev Weiss wrote: > On Mon, May 10, 2021 at 12:42:08AM CDT, Andrew Jeffery wrote: > >kcs_bmc_serio acts as a bridge between the KCS drivers in the IPMI > >subsystem and the existing userspace interfaces available through the > >serio subsystem. This is useful when userspace would like to make use of > >the BMC KCS devices for purposes that aren't IPMI. > > > >Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > >--- > > drivers/char/ipmi/Kconfig | 14 +++ > > drivers/char/ipmi/Makefile | 1 + > > drivers/char/ipmi/kcs_bmc_serio.c | 151 ++++++++++++++++++++++++++++++ > > 3 files changed, 166 insertions(+) > > create mode 100644 drivers/char/ipmi/kcs_bmc_serio.c > > > >diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig > >index bc5f81899b62..249b31197eea 100644 > >--- a/drivers/char/ipmi/Kconfig > >+++ b/drivers/char/ipmi/Kconfig > >@@ -137,6 +137,20 @@ config IPMI_KCS_BMC_CDEV_IPMI > > This support is also available as a module. The module will be > > called kcs_bmc_cdev_ipmi. > > > >+config IPMI_KCS_BMC_SERIO > >+ depends on IPMI_KCS_BMC && SERIO > >+ tristate "SerIO adaptor for BMC KCS devices" > >+ help > >+ Adapts the BMC KCS device for the SerIO subsystem. This allows users > >+ to take advantage of userspace interfaces provided by SerIO where > >+ appropriate. > >+ > >+ Say YES if you wish to expose KCS devices on the BMC via SerIO > >+ interfaces. > >+ > >+ This support is also available as a module. The module will be > >+ called kcs_bmc_serio. > >+ > > config ASPEED_BT_IPMI_BMC > > depends on ARCH_ASPEED || COMPILE_TEST > > depends on REGMAP && REGMAP_MMIO && MFD_SYSCON > >diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile > >index fcfa676afddb..84f47d18007f 100644 > >--- a/drivers/char/ipmi/Makefile > >+++ b/drivers/char/ipmi/Makefile > >@@ -23,6 +23,7 @@ obj-$(CONFIG_IPMI_POWERNV) += ipmi_powernv.o > > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o > > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o > > obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o > >+obj-$(CONFIG_IPMI_KCS_BMC_SERIO) += kcs_bmc_serio.o > > obj-$(CONFIG_IPMI_KCS_BMC_CDEV_IPMI) += kcs_bmc_cdev_ipmi.o > > obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o > > obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o > >diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c > >new file mode 100644 > >index 000000000000..30a2b7ab464b > >--- /dev/null > >+++ b/drivers/char/ipmi/kcs_bmc_serio.c > >@@ -0,0 +1,151 @@ > >+// SPDX-License-Identifier: GPL-2.0-or-later > >+/* Copyright (c) 2021 IBM Corp. */ > >+ > >+#include <linux/delay.h> > >+#include <linux/device.h> > >+#include <linux/errno.h> > >+#include <linux/list.h> > >+#include <linux/module.h> > >+#include <linux/sched/signal.h> > >+#include <linux/serio.h> > >+#include <linux/slab.h> > >+ > >+#include "kcs_bmc_client.h" > >+ > >+struct kcs_bmc_serio { > >+ struct list_head entry; > >+ > >+ struct kcs_bmc_client client; > >+ struct serio *port; > >+ > >+ spinlock_t lock; > >+}; > >+ > >+static inline struct kcs_bmc_serio *client_to_kcs_bmc_serio(struct kcs_bmc_client *client) > >+{ > >+ return container_of(client, struct kcs_bmc_serio, client); > >+} > >+ > >+static irqreturn_t kcs_bmc_serio_event(struct kcs_bmc_client *client) > >+{ > >+ struct kcs_bmc_serio *priv; > >+ u8 handled = IRQ_NONE; > >+ u8 status; > >+ > >+ priv = client_to_kcs_bmc_serio(client); > >+ > >+ spin_lock(&priv->lock); > >+ > >+ status = kcs_bmc_read_status(client->dev); > >+ > >+ if (status & KCS_BMC_STR_IBF) > >+ handled = serio_interrupt(priv->port, kcs_bmc_read_data(client->dev), 0); > >+ > >+ spin_unlock(&priv->lock); > >+ > >+ return handled; > >+} > >+ > >+static const struct kcs_bmc_client_ops kcs_bmc_serio_client_ops = { > >+ .event = kcs_bmc_serio_event, > >+}; > >+ > >+static int kcs_bmc_serio_open(struct serio *port) > >+{ > >+ struct kcs_bmc_serio *priv = port->port_data; > >+ > >+ return kcs_bmc_enable_device(priv->client.dev, &priv->client); > >+} > >+ > >+static void kcs_bmc_serio_close(struct serio *port) > >+{ > >+ struct kcs_bmc_serio *priv = port->port_data; > >+ > >+ kcs_bmc_disable_device(priv->client.dev, &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) > >+{ > >+ struct kcs_bmc_serio *priv; > >+ struct serio *port; > >+ > >+ priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL); > >+ port = kzalloc(sizeof(*port), GFP_KERNEL); > > Is there a particular reason to allocate port with a raw kzalloc() > instead of another devm_kzalloc()? Yes, because it causes pointer confusion on remove. We get the following backtrace: [ 95.018845] Backtrace: [ 95.019162] [<802e1fb0>] (___cache_free) from [<802e31cc>] (kfree+0xc0/0x1e8) [ 95.019658] r10:00000081 r9:8667c000 r8:80100284 r7:81000b40 r6:a0000013 r5:80640bd4 [ 95.020032] r4:82a5ec40 [ 95.020206] [<802e310c>] (kfree) from [<80640bd4>] (serio_release_port+0x1c/0x28) [ 95.020571] r7:00000000 r6:819c1540 r5:86acf480 r4:82a5ed70 [ 95.020818] [<80640bb8>] (serio_release_port) from [<80565e00>] (device_release+0x40/0xb4) [ 95.021387] [<80565dc0>] (device_release) from [<804a0bcc>] (kobject_put+0xc8/0x210) [ 95.021961] r5:80c77c30 r4:82a5ed70 [ 95.022141] [<804a0b04>] (kobject_put) from [<80566078>] (put_device+0x20/0x24) [ 95.022537] r7:80c820cc r6:82a5ec40 r5:80c820e4 r4:82a5ed70 [ 95.023017] [<80566058>] (put_device) from [<80640a58>] (serio_destroy_port+0x12c/0x140) [ 95.023719] [<8064092c>] (serio_destroy_port) from [<80640b3c>] (serio_unregister_port+0x34/0x44) [ 95.024326] r7:7f0012b4 r6:7f002024 r5:80c820e4 r4:82a5ec40 [ 95.024792] [<80640b08>] (serio_unregister_port) from [<7f0100b8>] (kcs_bmc_serio_remove_device+0x90/0xbc [kcs_bmc_serio]) [ 95.026951] r5:8668b7c0 r4:86acf0c0 [ 95.027390] [<7f010028>] (kcs_bmc_serio_remove_device [kcs_bmc_serio]) from [<7f00048c>] (kcs_bmc_unregister_driver+0x60/0xbd4 [kcs_bmc]) [ 95.028443] r5:7f012018 r4:8668b7c0 [ 95.028634] [<7f00042c>] (kcs_bmc_unregister_driver [kcs_bmc]) from [<7f0102c4>] (kcs_bmc_serio_exit+0x1c/0xd58 [kcs_bmc_serio]) [ 95.029165] r7:00000081 r6:80cd0dac r5:00000000 r4:7f012040 [ 95.029397] [<7f0102a8>] (kcs_bmc_serio_exit [kcs_bmc_serio]) from [<801cbab0>] (sys_delete_module+0x140/0x280) [ 95.029875] [<801cb970>] (sys_delete_module) from [<80100080>] (ret_fast_syscall+0x0/0x58) > > >+ if (!(priv && port)) > >+ return -ENOMEM; > >+ > >+ port->id.type = SERIO_8042; > >+ port->open = kcs_bmc_serio_open; > >+ port->close = kcs_bmc_serio_close; > >+ port->port_data = priv; > >+ port->dev.parent = kcs_bmc->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); > >+ > >+ serio_register_port(port); > >+ > >+ dev_info(kcs_bmc->dev, "Initialised serio client for channel %d", kcs_bmc->channel); > >+ > >+ return 0; > >+} > >+ > >+static int kcs_bmc_serio_remove_device(struct kcs_bmc_device *kcs_bmc) > >+{ > >+ 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 -ENODEV; > >+ > >+ serio_unregister_port(priv->port); > >+ kcs_bmc_disable_device(kcs_bmc, &priv->client); > >+ devm_kfree(priv->client.dev->dev, priv); > > Looks like the priv->port allocation would leak here I think? No, because port's released through serio_unregister_port(). It's not super obvious though, so I'll add a comment next to the kzalloc(). > > Also, is the asymmetry of having kcs_bmc_disable_device() here but no > corresponding kcs_bmc_enable_device() in kcs_bmc_serio_add_device() > intentional? If so, an explanatory comment of some sort might be nice > to add. It's intentional to make sure the device isn't left registered as enabled in the core. kcs_bmc_enable_device() is called in the open() path. Andrew
Hello, This is the 3rd spin of the series refactoring the keyboard-controller-style device drivers in the IPMI subsystem. v2 can be found (in two parts because yay patch workflow mistakes) at: Cover letter: https://lore.kernel.org/linux-arm-kernel/20210319061952.145040-1-andrew@aj.id.au/ Patches: https://lore.kernel.org/linux-arm-kernel/20210319062752.145730-1-andrew@aj.id.au/ Several significant changes in v3: 1. The series is rebased onto v5.13-rc1 2. v5.13-rc1 includes Chiawei's patches reworking the LPC devicetree bindings, so they're no-longer required in the series. 3. After some discussion with Arnd[1] and investigating the serio subsystem, I've replaced the "raw" KCS driver (patch 16/21 in v2) with a serio adaptor (patch 11/16 in this series). The adaptor allows us to take advantage of the existing chardevs provided by serio. [1] https://lore.kernel.org/linux-arm-kernel/37e75b07-a5c6-422f-84b3-54f2bea0b917@www.fastmail.com/ Finally, I've also addressed Zev Weiss' review comments where I thought it was required. These comments covered a lot of minor issues across (almost) all the patches, so it's best to review from a clean slate rather than attempt to review the differences between spins. Previously: Changes in v2 include: * A rebase onto v5.12-rc2 * Incorporation of off-list feedback on SerIRQ configuration from Chiawei * Further validation on hardware for ASPEED KCS devices 2, 3 and 4 * Lifting the existing single-open constraint of the IPMI chardev * Fixes addressing Rob's feedback on the conversion of the ASPEED KCS binding to dt-schema * Fixes addressing Rob's feedback on the new aspeed,lpc-interrupts property definition for the ASPEED KCS binding Please test and review! Andrew Andrew Jeffery (16): ipmi: kcs_bmc_aspeed: Use of match data to extract KCS properties ipmi: kcs_bmc: Make status update atomic ipmi: kcs_bmc: Rename {read,write}_{status,data}() functions ipmi: kcs_bmc: Split out kcs_bmc_cdev_ipmi ipmi: kcs_bmc: Turn the driver data-structures inside-out ipmi: kcs_bmc: Split headers into device and client ipmi: kcs_bmc: Strip private client data from struct kcs_bmc ipmi: kcs_bmc: Decouple the IPMI chardev from the core ipmi: kcs_bmc: Allow clients to control KCS IRQ state ipmi: kcs_bmc: Don't enforce single-open policy in the kernel ipmi: kcs_bmc: Add serio adaptor dt-bindings: ipmi: Convert ASPEED KCS binding to schema dt-bindings: ipmi: Add optional SerIRQ property to ASPEED KCS devices ipmi: kcs_bmc_aspeed: Implement KCS SerIRQ configuration ipmi: kcs_bmc_aspeed: Fix IBFIE typo from datasheet ipmi: kcs_bmc_aspeed: Optionally apply status address .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 106 +++ .../bindings/ipmi/aspeed-kcs-bmc.txt | 33 - drivers/char/ipmi/Kconfig | 27 + drivers/char/ipmi/Makefile | 2 + drivers/char/ipmi/kcs_bmc.c | 526 ++++----------- drivers/char/ipmi/kcs_bmc.h | 92 +-- drivers/char/ipmi/kcs_bmc_aspeed.c | 635 +++++++++++++----- drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 568 ++++++++++++++++ drivers/char/ipmi/kcs_bmc_client.h | 48 ++ drivers/char/ipmi/kcs_bmc_device.h | 22 + drivers/char/ipmi/kcs_bmc_npcm7xx.c | 94 ++- drivers/char/ipmi/kcs_bmc_serio.c | 151 +++++ 12 files changed, 1582 insertions(+), 722 deletions(-) create mode 100644 Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml delete mode 100644 Documentation/devicetree/bindings/ipmi/aspeed-kcs-bmc.txt create mode 100644 drivers/char/ipmi/kcs_bmc_cdev_ipmi.c create mode 100644 drivers/char/ipmi/kcs_bmc_client.h create mode 100644 drivers/char/ipmi/kcs_bmc_device.h create mode 100644 drivers/char/ipmi/kcs_bmc_serio.c