| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aCaOAVgb8V7_-rLR@mail.minyard.net>
Date: Thu, 15 May 2025 19:59:45 -0500
From: Corey Minyard <corey@...yard.net>
To: Binbin Zhou <zhoubinbin@...ngson.cn>
Cc: Binbin Zhou <zhoubb.aaron@...il.com>,
Huacai Chen <chenhuacai@...ngson.cn>, Lee Jones <lee@...nel.org>,
Corey Minyard <minyard@....org>,
Huacai Chen <chenhuacai@...nel.org>,
Xuerui Wang <kernel@...0n.name>, loongarch@...ts.linux.dev,
linux-kernel@...r.kernel.org,
openipmi-developer@...ts.sourceforge.net,
Chong Qiao <qiaochong@...ngson.cn>
Subject: Re: [PATCH v2 2/3] ipmi: Add Loongson-2K BMC support
On Thu, May 15, 2025 at 10:32:25AM +0800, Binbin Zhou wrote:
> This patch adds Loongson-2K BMC IPMI support.
>
> According to the existing design, we use software simulation to
> implement the KCS interface registers: Stauts/Command/Data_Out/Data_In.
This is a strange way to do this. My preference would be to have a
separate driver for this and not put it under the ipmi_si driver.
But it's annoyingly close and it would duplicate a lot of ipmi_si_intf.c
Anyway, I think I'm ok with this basic design. But there are problems.
>
> Also since both host side and BMC side read and write kcs status, I use
> fifo pointer to ensure data consistency.
I assume this fifo pointer is part of the interface hardware or the
implementation on the other side of the interface.
>
> Therefore I made the whole IPMI driver independent.
What do you mean by this statement?
More comments inline.
>
> Co-developed-by: Chong Qiao <qiaochong@...ngson.cn>
> Signed-off-by: Chong Qiao <qiaochong@...ngson.cn>
> Signed-off-by: Binbin Zhou <zhoubinbin@...ngson.cn>
> ---
> drivers/char/ipmi/Makefile | 1 +
> drivers/char/ipmi/ipmi_si.h | 7 +
> drivers/char/ipmi/ipmi_si_intf.c | 3 +
> drivers/char/ipmi/ipmi_si_ls2k.c | 250 +++++++++++++++++++++++++++++++
> 4 files changed, 261 insertions(+)
> create mode 100644 drivers/char/ipmi/ipmi_si_ls2k.c
>
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index e0944547c9d0..5eb3494f5f39 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -8,6 +8,7 @@ ipmi_si-y := ipmi_si_intf.o ipmi_kcs_sm.o ipmi_smic_sm.o ipmi_bt_sm.o \
> ipmi_si_mem_io.o
> ipmi_si-$(CONFIG_HAS_IOPORT) += ipmi_si_port_io.o
> ipmi_si-$(CONFIG_PCI) += ipmi_si_pci.o
> +ipmi_si-$(CONFIG_LOONGARCH) += ipmi_si_ls2k.o
Shouldn't this be dependent on MFD_LS2K_BMC? It appears you can disable
that and still have CONFIG_LOONGARCH enabled.
And this MFD can have multiple things hanging off of it, wouldn't you
want to make the individual drivers their own CONFIG items?
> ipmi_si-$(CONFIG_PARISC) += ipmi_si_parisc.o
>
> obj-$(CONFIG_IPMI_HANDLER) += ipmi_msghandler.o
> diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
> index a7ead2a4c753..71f1d4e1272c 100644
> --- a/drivers/char/ipmi/ipmi_si.h
> +++ b/drivers/char/ipmi/ipmi_si.h
> @@ -93,6 +93,13 @@ void ipmi_si_pci_shutdown(void);
> static inline void ipmi_si_pci_init(void) { }
> static inline void ipmi_si_pci_shutdown(void) { }
> #endif
> +#ifdef CONFIG_LOONGARCH
> +void ipmi_si_ls2k_init(void);
> +void ipmi_si_ls2k_shutdown(void);
> +#else
> +static inline void ipmi_si_ls2k_init(void) { }
> +static inline void ipmi_si_ls2k_shutdown(void) { }
> +#endif
I'm not excited about this, but there is history, I guess.
Same comment as the Makefile on CONFIG_LOONGARCH.
> #ifdef CONFIG_PARISC
> void ipmi_si_parisc_init(void);
> void ipmi_si_parisc_shutdown(void);
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 12b0b77eb1cc..323da77698ea 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2107,6 +2107,7 @@ static int __init init_ipmi_si(void)
>
> ipmi_si_pci_init();
>
> + ipmi_si_ls2k_init();
> ipmi_si_parisc_init();
>
> /* We prefer devices with interrupts, but in the case of a machine
> @@ -2288,6 +2289,8 @@ static void cleanup_ipmi_si(void)
>
> ipmi_si_pci_shutdown();
>
> + ipmi_si_ls2k_shutdown();
> +
> ipmi_si_parisc_shutdown();
>
> ipmi_si_platform_shutdown();
> diff --git a/drivers/char/ipmi/ipmi_si_ls2k.c b/drivers/char/ipmi/ipmi_si_ls2k.c
> new file mode 100644
> index 000000000000..cb31bb989fca
> --- /dev/null
> +++ b/drivers/char/ipmi/ipmi_si_ls2k.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for Loongson-2K BMC IPMI
> + *
> + * Copyright (C) 2024 Loongson Technology Corporation Limited.
> + *
> + * Originally written by Chong Qiao <qiaochong@...ngson.cn>
> + * Rewritten for mainline by Binbin Zhou <zhoubinbin@...ngson.cn>
> + */
> +
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +#include "ipmi_si.h"
> +
> +#define LS2K_KCS_STS_OBF BIT(0)
> +#define LS2K_KCS_STS_IBF BIT(1)
> +#define LS2K_KCS_STS_SMS_ATN BIT(2)
> +#define LS2K_KCS_STS_CMD BIT(3)
> +
> +#define LS2K_KCS_DATA_MASK (LS2K_KCS_STS_OBF | LS2K_KCS_STS_IBF | LS2K_KCS_STS_CMD)
> +
> +/* Read and write fifo pointers for data consistency. */
> +struct ls2k_fifo_flag {
> + u8 ibfh;
> + u8 ibft;
> + u8 obfh;
> + u8 obft;
> +};
> +
> +struct ls2k_kcs_reg {
> + u8 status;
> + u8 data_out;
> + s16 data_in;
> + s16 cmd;
> +};
> +
> +struct ls2k_kcs_data {
> + struct ls2k_fifo_flag fifo;
> + struct ls2k_kcs_reg reg;
> + u8 cmd_data;
> + u8 version;
> + u32 write_req;
> + u32 write_ack;
> + u32 reserved[2];
> +};
The above appears to be a memory overlay for registers. But you aren't
using readb/writeb and associated functions to read/write it. That is
not the right way to do things. Please read
Documentation/driver-api/device-io.rst
> +
> +static void ls2k_set_obf(struct ls2k_kcs_data *ik, u8 sts)
> +{
> + ik->reg.status = (ik->reg.status & ~LS2K_KCS_STS_OBF) | (sts & BIT(0));
> +}
> +
> +static void ls2k_set_ibf(struct ls2k_kcs_data *ik, u8 sts)
> +{
> + ik->reg.status = (ik->reg.status & ~LS2K_KCS_STS_IBF) | ((sts & BIT(0)) << 1);
> +}
> +
> +static u8 ls2k_get_ibf(struct ls2k_kcs_data *ik)
> +{
> + return (ik->reg.status >> 1) & BIT(0);
> +}
> +
> +static unsigned char intf_sim_inb_v0(struct ls2k_kcs_data *ik,
> + unsigned int offset)
> +{
> + u32 inb = 0;
> +
> + switch (offset & BIT(0)) {
> + case 0:
> + inb = ik->reg.data_out;
> + ls2k_set_obf(ik, 0);
> + break;
> + case 1:
> + inb = ik->reg.status;
> + break;
> + }
> +
> + return inb;
> +}
> +
> +static unsigned char intf_sim_inb_v1(struct ls2k_kcs_data *ik,
> + unsigned int offset)
> +{
> + u32 inb = 0;
> + int cmd;
> + bool obf, ibf;
> +
> + obf = ik->fifo.obfh != ik->fifo.obft;
> + ibf = ik->fifo.ibfh != ik->fifo.ibft;
> + cmd = ik->cmd_data;
> +
> + switch (offset & BIT(0)) {
> + case 0:
> + inb = ik->reg.data_out;
> + ik->fifo.obft = ik->fifo.obfh;
> + break;
> + case 1:
> + inb = ik->reg.status & ~LS2K_KCS_DATA_MASK;
> + inb |= obf | (ibf << 1) | (cmd << 3);
> + break;
> + }
> +
> + return inb;
> +}
> +
> +static unsigned char ls2k_mem_inb(const struct si_sm_io *io,
> + unsigned int offset)
> +{
> + struct ls2k_kcs_data *ik = io->addr;
> + int inb = 0;
> +
> + if (ik->version == 0)
> + inb = intf_sim_inb_v0(ik, offset);
> + else if (ik->version == 1)
> + inb = intf_sim_inb_v1(ik, offset);
> +
> + return inb;
> +}
> +
> +static void intf_sim_outb_v0(struct ls2k_kcs_data *ik, unsigned int offset,
> + unsigned char val)
> +{
> + if (ls2k_get_ibf(ik))
> + return;
> +
> + switch (offset & BIT(0)) {
> + case 0:
> + ik->reg.data_in = val;
> + ik->reg.status &= ~LS2K_KCS_STS_CMD;
> + break;
> +
> + case 1:
> + ik->reg.cmd = val;
> + ik->reg.status |= LS2K_KCS_STS_CMD;
> + break;
> + }
> +
> + ls2k_set_ibf(ik, 1);
> + ik->write_req++;
> +}
> +
> +static void intf_sim_outb_v1(struct ls2k_kcs_data *ik, unsigned int offset,
> + unsigned char val)
> +{
> + if (ik->fifo.ibfh != ik->fifo.ibft)
> + return;
> +
> + switch (offset & BIT(0)) {
> + case 0:
> + ik->reg.data_in = val;
> + ik->cmd_data = 0;
> + break;
> +
> + case 1:
> + ik->reg.cmd = val;
> + ik->cmd_data = 1;
> + break;
> + }
> +
> + ik->fifo.ibfh = !ik->fifo.ibft;
> + ik->write_req++;
> +}
> +
> +static void ls2k_mem_outb(const struct si_sm_io *io, unsigned int offset,
> + unsigned char val)
> +{
> + struct ls2k_kcs_data *ik = io->addr;
> +
> + if (ik->version == 0)
> + intf_sim_outb_v0(ik, offset, val);
> + else if (ik->version == 1)
> + intf_sim_outb_v1(ik, offset, val);
> +}
> +
> +static void ls2k_mem_cleanup(struct si_sm_io *io)
> +{
> + if (io->addr)
> + iounmap(io->addr);
> +}
> +
> +static int ipmi_ls2k_sim_setup(struct si_sm_io *io)
> +{
> + io->addr = ioremap(io->addr_data, io->regspacing);
> + if (!io->addr)
> + return -EIO;
> +
> + io->inputb = ls2k_mem_inb;
> + io->outputb = ls2k_mem_outb;
> + io->io_cleanup = ls2k_mem_cleanup;
> +
> + return 0;
> +}
> +
> +static int ipmi_ls2k_probe(struct platform_device *pdev)
> +{
> + struct si_sm_io io;
> +
> + dev_info(&pdev->dev, "probing via ls2k platform");
> + memset(&io, 0, sizeof(io));
> +
> + io.addr_source = SI_PLATFORM;
> + io.si_type = SI_KCS;
si_type has been reworked recently, the linux next tree has the changes.
I'll need this modified to work with the linux next changes.
> + io.addr_space = IPMI_MEM_ADDR_SPACE;
> + io.io_setup = ipmi_ls2k_sim_setup;
> + io.addr_data = pdev->resource[0].start;
> + io.regspacing = pdev->resource[0].end - pdev->resource[0].start + 1;
> + io.regsize = DEFAULT_REGSIZE;
> + io.regshift = 0;
The above items, except for io_setup, don't have much meaning for your
device; there's not much need to set them, and there's no need to
initialize things to zero. They are for ipmi_si_port and ipmi_si_mem.
> + io.dev = &pdev->dev;
> + io.irq = 0;
> + if (io.irq)
> + io.irq_setup = ipmi_std_irq_setup;
Just remove the irq thing, don't set it to zero and then check it.
> +
> + dev_info(&pdev->dev, "%pR regsize %d spacing %d irq %d\n",
> + &pdev->resource[0], io.regsize, io.regspacing, io.irq);
> +
> + return ipmi_si_add_smi(&io);
> +}
> +
> +static void ipmi_ls2k_remove(struct platform_device *pdev)
> +{
> + ipmi_si_remove_by_dev(&pdev->dev);
> +}
> +
> +struct platform_driver ipmi_ls2k_platform_driver = {
> + .driver = {
> + .name = "ls2k-ipmi-si",
> + },
> + .probe = ipmi_ls2k_probe,
> + .remove = ipmi_ls2k_remove,
> +};
> +
> +static bool platform_registered;
> +void ipmi_si_ls2k_init(void)
> +{
> + int rv;
> +
> + rv = platform_driver_register(&ipmi_ls2k_platform_driver);
> + if (rv)
> + pr_err("Unable to register driver: %d\n", rv);
That's far to vague to be useful.
> + else
> + platform_registered = true;
> +}
> +
> +void ipmi_si_ls2k_shutdown(void)
> +{
> + if (platform_registered)
> + platform_driver_unregister(&ipmi_ls2k_platform_driver);
> +}
> --
> 2.47.1
>
Powered by blists - more mailing lists