From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6E3413EA977 for ; Thu, 9 Jul 2026 09:27:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783589256; cv=none; b=XK7k89bPmX+niMXUYBL1fHX1sAP0KdCZZmZcqNMU4MU8JuyVPl+Tjol7YKv2MOce3YEbelPnxKeftk2IQcRGrA7ggfl8aSsVMPqQ4DstuCpGdK9YTNFMS4/UGx+an59JWW/7lVveJejiH48+kYtcpiDlrFdOd6sssxV/OAFXXDk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783589256; c=relaxed/simple; bh=BkT9UJil5M86iN9RlBXvsIgkh/X+ivN1WKwIaLgtvtE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=h95TAFpQ4tTwQEzDL4GnyK8kiY2wWpQXwMxXmGvCkWRTLFrZhmWMUrtN9CQScbFMGN69tmtW6aDoPJpr4LrwBlxxzQb/MlGv3GESJfGsEJkX/h/9Jckz/ZIm2OMa/kHh+qacy58EjHNHDth0aqRaxpoaitNaoxqrfBpvGFIMtMQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=SFKBFoye; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="SFKBFoye" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 494F31F00A3A; Thu, 9 Jul 2026 09:27:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1783589245; bh=YvUtTbf87LythJADEoxaijxOfE/Z3lqPIKM9myd83O8=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=SFKBFoyeJMr+rTzpy00HQ2RmD0RqBIym8F6XdIyrMrjLVPw4eh7ypnV9qhjvl4b3g 3CecdFQ0zbYJrZzOsvdrkWBfOh3EhrALxqEv+d1bGb2ba0JfnJYwGYCZFZMchbbORx QsdbAI2qjvixEbQ+4U7LXiPj5pBPsQCBlVykteF9pmHxHOlWfDpz2CTx02V6reN5iA Msi9dqJgGpnUz5NOLd3zDeohimdRqcwHdmtKiudHRYragX4/byx5PbuDM3zvAHYMUJ 48/uPIBF7T92jxI5YD5N+j900GghaogghqOK46I+nJHwVOGjMV/ECbyYJFENc7FRHY 9c1/BBRKZmQnA== Date: Thu, 9 Jul 2026 10:27:21 +0100 From: Lee Jones To: a0282524688@gmail.com Cc: Ming Yu , linux-kernel@vger.kernel.org Subject: Re: [PATCH v6 7/7] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver Message-ID: <20260709092721.GB2045740@google.com> References: <20260701035025.3082927-1-a0282524688@gmail.com> <20260701035025.3082927-8-a0282524688@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260701035025.3082927-8-a0282524688@gmail.com> /* Sashiko Automation: Issues Found (7 Findings) */ Some relevant ones here. On Wed, 01 Jul 2026, a0282524688@gmail.com wrote: > From: Ming Yu > > Add support for the Host Interface (HIF) transport via eSPI for the > Nuvoton NCT6694 peripheral expander. > > This transport driver initializes the Super-I/O to configure the > device's shared memory base address and SIRQ. It provides a regmap_bus > implementation that drives the firmware command/response exchange over > the shared-memory window, plus an internal regmap_mmio for the report > region. The initialized device config is then passed to the MFD core > (nct6694_core_probe) for sub-device registration and IRQ domain setup. > > Signed-off-by: Ming Yu > --- > Changes in v6: > - Reworked the transport to implement a regmap_bus instead of the v5 > read_msg/write_msg function pointers. > > Changes in v5: > - Split from the monolithic v4 patch. > - Adapted to re-use the newly introduced nct6694_core_probe() and > abstracted I/O APIs. > > Changes since version 3: > - Remove redundant module type macro definitions from sub-device drivers > that are now provided by the shared header , > fixing -Wmacro-redefined warnings. > > Changes since version 2: > - Restore per-device IDA and mfd_add_hotplug_devices()/PLATFORM_DEVID_AUTO > to avoid child device ID conflicts with multiple NCT6694 chips. > - Validate irq_find_mapping() return value before dispatching IRQs. > - Check superio_enter() return value in nct6694_irq_init(). > > Changes since version 1: > - Drop function pointers from Super-I/O access and use static inline > helpers with proper types. > > drivers/mfd/Kconfig | 16 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/nct6694-hif.c | 565 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 582 insertions(+) > create mode 100644 drivers/mfd/nct6694-hif.c > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 742fc26e6ff7..f37acf585671 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1175,6 +1175,22 @@ config MFD_NCT6694 > > It is selected automatically by the transport interface drivers. > > +config MFD_NCT6694_HIF > + tristate "Nuvoton NCT6694 HIF (eSPI) interface support" > + depends on HAS_IOPORT && ACPI > + select MFD_NCT6694 > + select REGMAP_MMIO > + help > + This enables support for the Nuvoton NCT6694 peripheral expander > + connected via the Host Interface (HIF) using eSPI transport. > + > + The transport driver uses Super-I/O mapping and shared memory to > + communicate with the NCT6694 firmware. Enable this option if you > + are using the NCT6694 over an eSPI interface on an ACPI platform. > + > + To compile this driver as a module, choose M here: the module > + will be called nct6694-hif. > + > config MFD_NCT6694_USB > tristate "Nuvoton NCT6694 USB interface support" > select MFD_NCT6694 > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 10c19a19541e..97924dd3552c 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -125,6 +125,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o > obj-$(CONFIG_MFD_PF1550) += pf1550.o > > obj-$(CONFIG_MFD_NCT6694) += nct6694-core.o > +obj-$(CONFIG_MFD_NCT6694_HIF) += nct6694-hif.o > obj-$(CONFIG_MFD_NCT6694_USB) += nct6694-usb.o > > obj-$(CONFIG_MFD_CORE) += mfd-core.o > diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c > new file mode 100644 > index 000000000000..c47534a1e526 > --- /dev/null > +++ b/drivers/mfd/nct6694-hif.c > @@ -0,0 +1,565 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2026 Nuvoton Technology Corp. > + * > + * Nuvoton NCT6694 host-interface (eSPI) transport driver. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DRVNAME "nct6694-hif" > + > +#define NCT6694_POLL_INTERVAL_US 10 > +#define NCT6694_POLL_TIMEOUT_US 10000 > + > +/* > + * Super-I/O registers > + */ > +#define SIO_REG_LDSEL 0x07 /* Logical device select */ > +#define SIO_REG_DEVID 0x20 /* Device ID (2 bytes) */ > +#define SIO_REG_LD_SHM 0x0F /* Logical device shared memory control */ > + > +#define SIO_REG_SHM_ENABLE 0x30 /* Enable shared memory */ > +#define SIO_REG_SHM_BASE_ADDR 0x60 /* Shared memory base address (2 bytes) */ > +#define SIO_REG_SHM_IRQ_NR 0x70 /* Shared memory interrupt number */ > + > +#define SIO_REG_UNLOCK_KEY 0x87 /* Key to enable Super-I/O */ > +#define SIO_REG_LOCK_KEY 0xAA /* Key to disable Super-I/O */ > + > +#define SIO_NCT6694B_ID 0xD029 > +#define SIO_NCT6694D_ID 0x5832 > + > +/* > + * Super-I/O Shared Memory Logical Device registers > + */ > +#define NCT6694_SHM_COFS_STS 0x2E > +#define NCT6694_SHM_COFS_STS_COFS4W BIT(7) > + > +#define NCT6694_SHM_COFS_CTL2 0x3B > +#define NCT6694_SHM_COFS_CTL2_COFS4W_IE BIT(3) > + > +/* COFS register block [STS..CTL2] is the only SHM range driven via inb/outb */ > +#define NCT6694_SHM_COFS_LEN \ > + (NCT6694_SHM_COFS_CTL2 - NCT6694_SHM_COFS_STS + 1) > + > +#define NCT6694_SHM_INTR_STATUS 0x9C /* Interrupt status register (4 bytes) */ > + > +enum nct6694_chips { > + NCT6694B = 0, > + NCT6694D, > +}; > + > +struct __packed nct6694_hif_msg { > + struct nct6694_cmd_header cmd_header; > + struct nct6694_response_header response_header; > + unsigned char data[]; > +}; > + > +struct nct6694_sio_data { > + enum nct6694_chips chip; > + int sioreg; /* Super-I/O index port */ > +}; > + > +struct nct6694_hif_data { > + struct regmap *rpt_regmap; > + struct nct6694_sio_data *sio_data; > + void __iomem *msg_base; > + void *xfer_buf; > + unsigned int shm_base; > +}; > + > +static const char * const nct6694_chip_names[] = { > + [NCT6694B] = "NCT6694B", > + [NCT6694D] = "NCT6694D", > +}; > + > +/* > + * Super-I/O functions. > + */ > +static inline int superio_enter(struct nct6694_sio_data *sio_data) > +{ > + int ioreg = sio_data->sioreg; > + > + /* > + * Try to reserve and for exclusive access. > + */ > + if (!request_muxed_region(ioreg, 2, DRVNAME)) > + return -EBUSY; > + > + outb(SIO_REG_UNLOCK_KEY, ioreg); > + outb(SIO_REG_UNLOCK_KEY, ioreg); > + > + return 0; > +} [Severity: High] If the hardware returns a response length smaller than the requested length, does this leave the remainder of the caller buffer uninitialized? Can this lead to an information leak or logic corruption when caller drivers parse the buffer? > + > +static inline void superio_exit(struct nct6694_sio_data *sio_data) > +{ > + int ioreg = sio_data->sioreg; > + > + outb(SIO_REG_LOCK_KEY, ioreg); > + > + release_region(ioreg, 2); > +} > + > +static inline void superio_select(struct nct6694_sio_data *sio_data, int ld) > +{ > + int ioreg = sio_data->sioreg; > + > + outb(SIO_REG_LDSEL, ioreg); > + outb(ld, ioreg + 1); > +} > + > +static inline int superio_inb(struct nct6694_sio_data *sio_data, int reg) > +{ > + int ioreg = sio_data->sioreg; > + > + outb(reg, ioreg); > + return inb(ioreg + 1); > +} > + > +static inline int superio_inw(struct nct6694_sio_data *sio_data, int reg) > +{ > + int ioreg = sio_data->sioreg; > + int val; > + > + outb(reg++, ioreg); > + val = inb(ioreg + 1) << 8; > + outb(reg, ioreg); > + val |= inb(ioreg + 1); > + > + return val; > +} > + > +static inline void superio_outb(struct nct6694_sio_data *sio_data, int reg, u8 val) > +{ > + int ioreg = sio_data->sioreg; > + > + outb(reg, ioreg); > + outb(val, ioreg + 1); > +} > + > +static int nct6694_sio_find(struct nct6694_sio_data *sio_data, u8 sioreg) > +{ > + int ret; > + u16 devid; > + > + sio_data->sioreg = sioreg; > + > + ret = superio_enter(sio_data); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + /* Check Chip ID */ > + devid = superio_inw(sio_data, SIO_REG_DEVID); > + switch (devid) { > + case SIO_NCT6694B_ID: > + sio_data->chip = NCT6694B; > + break; > + case SIO_NCT6694D_ID: > + sio_data->chip = NCT6694D; > + break; > + default: > + superio_exit(sio_data); > + return -ENODEV; > + } > + > + superio_exit(sio_data); > + > + return 0; > +} [Severity: High] If the hardware returns a response length smaller than the requested length, does this leave the remainder of the caller buffer uninitialized? Can this lead to an information leak or logic corruption when caller drivers parse the buffer? > + > +static const struct mfd_cell nct6694_hif_devs[] = { > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + MFD_CELL_NAME("nct6694-gpio"), > + > + MFD_CELL_NAME("nct6694-i2c"), > + MFD_CELL_NAME("nct6694-i2c"), > + MFD_CELL_NAME("nct6694-i2c"), > + MFD_CELL_NAME("nct6694-i2c"), > + MFD_CELL_NAME("nct6694-i2c"), > + MFD_CELL_NAME("nct6694-i2c"), > + > + MFD_CELL_NAME("nct6694-canfd"), > + MFD_CELL_NAME("nct6694-canfd"), > +}; > + > +static int nct6694_hif_err_handling(struct nct6694 *nct6694, unsigned char err_status) > +{ > + switch (err_status) { > + case NCT6694_NO_ERROR: > + return 0; > + case NCT6694_NOT_SUPPORT_ERROR: > + dev_err(nct6694->dev, "Command is not supported!\n"); > + break; > + case NCT6694_NO_RESPONSE_ERROR: > + dev_warn(nct6694->dev, "Command received no response!\n"); > + break; > + case NCT6694_TIMEOUT_ERROR: > + dev_warn(nct6694->dev, "Command timed out!\n"); > + break; > + case NCT6694_PENDING: > + dev_err(nct6694->dev, "Command is pending!\n"); > + break; > + default: > + return -EINVAL; > + } > + > + return -EIO; > +} > + > +static int nct6694_hif_xfer_msg(struct nct6694 *nct6694, > + const struct nct6694_cmd_header *cmd_hd, > + u8 hctrl, void *buf) > +{ > + struct nct6694_hif_data *hdata = nct6694->priv; > + void __iomem *hdr = hdata->msg_base + offsetof(struct nct6694_hif_msg, cmd_header); > + struct nct6694_cmd_header cmd = *cmd_hd; > + struct nct6694_response_header resp; > + u16 len = le16_to_cpu(cmd.len); > + u8 status; > + int ret; > + > + /* Wait until the previous command is completed */ > + ret = readb_poll_timeout(hdr + offsetof(struct nct6694_cmd_header, hctrl), > + status, status == 0, NCT6694_POLL_INTERVAL_US, > + NCT6694_POLL_TIMEOUT_US); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + /* > + * Write cmd header fields, but skip hctrl — writing to it triggers > + * firmware command processing and must be deferred until data is ready. > + */ > + memcpy_toio(hdr, &cmd, offsetof(struct nct6694_cmd_header, hctrl)); > + memcpy_toio(hdr + offsetof(struct nct6694_cmd_header, rsv2), &cmd.rsv2, > + sizeof(cmd) - offsetof(struct nct6694_cmd_header, rsv2)); > + > + if (hctrl == NCT6694_HCTRL_SET && len) > + memcpy_toio(hdata->msg_base + offsetof(struct nct6694_hif_msg, data), > + buf, len); > + > + /* Write hctrl last to trigger command processing */ > + writeb(hctrl, hdr + offsetof(struct nct6694_cmd_header, hctrl)); > + > + ret = readb_poll_timeout(hdr + offsetof(struct nct6694_cmd_header, hctrl), > + status, status == 0, NCT6694_POLL_INTERVAL_US, > + NCT6694_POLL_TIMEOUT_US); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + memcpy_fromio(&resp, hdata->msg_base + offsetof(struct nct6694_hif_msg, response_header), > + sizeof(resp)); > + > + ret = nct6694_hif_err_handling(nct6694, resp.sts); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + if (le16_to_cpu(resp.len)) > + memcpy_fromio(buf, hdata->msg_base + offsetof(struct nct6694_hif_msg, data), > + min(len, le16_to_cpu(resp.len))); > + > + return 0; > +} [Severity: High] If the hardware returns a response length smaller than the requested length, does this leave the remainder of the caller buffer uninitialized? Can this lead to an information leak or logic corruption when caller drivers parse the buffer? > + > +static int nct6694_hif_regmap_read(void *context, const void *reg_buf, > + size_t reg_size, void *val_buf, > + size_t val_size) > +{ > + struct nct6694 *nct6694 = context; > + struct nct6694_hif_data *hdata = nct6694->priv; > + u32 reg = get_unaligned_be32(reg_buf); > + const struct nct6694_cmd_header cmd_hd = { > + .mod = FIELD_GET(NCT6694_REG_MOD, reg), > + .offset = cpu_to_le16(FIELD_GET(NCT6694_REG_OFFSET, reg)), > + .len = cpu_to_le16(val_size), > + }; > + > + if (FIELD_GET(NCT6694_REG_MOD, reg) == NCT6694_RPT_MOD) > + return regmap_bulk_read(hdata->rpt_regmap, > + FIELD_GET(NCT6694_REG_OFFSET, reg), > + val_buf, val_size); > + > + return nct6694_hif_xfer_msg(nct6694, &cmd_hd, > + FIELD_GET(NCT6694_REG_HCTRL, reg), val_buf); > +} > + > +static int nct6694_hif_regmap_write(void *context, const void *data, > + size_t count) > +{ > + struct nct6694 *nct6694 = context; > + struct nct6694_hif_data *hdata = nct6694->priv; > + u32 reg = get_unaligned_be32(data); > + size_t len = count - sizeof(reg); > + const struct nct6694_cmd_header cmd_hd = { > + .mod = FIELD_GET(NCT6694_REG_MOD, reg), > + .offset = cpu_to_le16(FIELD_GET(NCT6694_REG_OFFSET, reg)), > + .len = cpu_to_le16(len), > + }; > + > + if (FIELD_GET(NCT6694_REG_MOD, reg) == NCT6694_RPT_MOD) > + return regmap_bulk_write(hdata->rpt_regmap, > + FIELD_GET(NCT6694_REG_OFFSET, reg), > + data + sizeof(reg), len); > + > + if (len > NCT6694_MAX_PACKET_SIZE) > + return -EINVAL; > + > + /* > + * nct6694_hif_xfer_msg() reads the firmware response back into the > + * payload buffer, so copy the const regmap data into a scratch buffer > + * it can write to. > + */ > + memcpy(hdata->xfer_buf, data + sizeof(reg), len); > + > + return nct6694_hif_xfer_msg(nct6694, &cmd_hd, NCT6694_HCTRL_SET, > + hdata->xfer_buf); > +} > + > +static const struct regmap_bus nct6694_hif_regmap_bus = { > + .read = nct6694_hif_regmap_read, > + .write = nct6694_hif_regmap_write, > +}; > + > +static const struct regmap_config nct6694_hif_msg_regmap_config = { > + .name = "msg", > + .reg_bits = 32, > + .val_bits = 8, > + .reg_stride = 1, > + .max_raw_read = NCT6694_MAX_PACKET_SIZE, > + .max_raw_write = NCT6694_MAX_PACKET_SIZE, > +}; > + > +static const struct regmap_config nct6694_hif_rpt_regmap_config = { > + .name = "rpt", > + .reg_bits = 8, > + .val_bits = 8, > + .reg_stride = 1, > +}; > + > +static irqreturn_t nct6694_hif_irq_handler(int irq, void *data) > +{ > + struct nct6694 *nct6694 = data; > + struct nct6694_hif_data *hdata = nct6694->priv; > + u8 reg_data[4]; > + u32 intr_status; > + int ret; > + > + /* Check interrupt status is set */ > + if (!(inb(hdata->shm_base + NCT6694_SHM_COFS_STS) & NCT6694_SHM_COFS_STS_COFS4W)) > + return IRQ_NONE; > + > + /* Clear interrupt status */ > + outb(NCT6694_SHM_COFS_STS_COFS4W, hdata->shm_base + NCT6694_SHM_COFS_STS); > + > + ret = regmap_bulk_read(hdata->rpt_regmap, NCT6694_SHM_INTR_STATUS, > + reg_data, ARRAY_SIZE(reg_data)); > + if (ret) > + return IRQ_NONE; [Severity: Medium] Is it safe to return IRQ_NONE here after the hardware interrupt has already been successfully cleared via outb()? Could this mislead the kernel's spurious interrupt detector into permanently disabling the shared IRQ line if the read fails repeatedly? > + > + intr_status = get_unaligned_le32(reg_data); > + > + while (intr_status) { > + int hwirq = __ffs(intr_status); > + > + generic_handle_irq_safe(irq_find_mapping(nct6694->domain, hwirq)); [Severity: High] If an unmapped hardware interrupt or spurious bit is processed, irq_find_mapping() will return 0. Does passing this directly into generic_handle_irq_safe(0) risk invoking the system timer handler from this device's interrupt context? [Severity: High] Since child drivers access their registers over a slow eSPI bus, does dispatching them via generic_handle_irq_safe() cause a sleep-in-atomic bug? Would handle_nested_irq() be more appropriate for a threaded parent dispatching to slow child devices? > + intr_status &= ~BIT(hwirq); > + } > + > + return IRQ_HANDLED; > +} > + > +static void nct6694_hif_irq_disable(void *data) > +{ > + struct nct6694 *nct6694 = data; > + struct nct6694_hif_data *hdata = nct6694->priv; > + u8 cofs_ctl2; > + > + /* Disable SIRQ interrupt */ > + cofs_ctl2 = inb(hdata->shm_base + NCT6694_SHM_COFS_CTL2); > + cofs_ctl2 &= ~NCT6694_SHM_COFS_CTL2_COFS4W_IE; > + outb(cofs_ctl2, hdata->shm_base + NCT6694_SHM_COFS_CTL2); > +} > + > +static void nct6694_hif_irq_enable(struct nct6694 *nct6694) > +{ > + struct nct6694_hif_data *hdata = nct6694->priv; > + u8 cofs_ctl2; > + > + /* Enable SIRQ interrupt */ > + cofs_ctl2 = inb(hdata->shm_base + NCT6694_SHM_COFS_CTL2); > + cofs_ctl2 |= NCT6694_SHM_COFS_CTL2_COFS4W_IE; > + outb(cofs_ctl2, hdata->shm_base + NCT6694_SHM_COFS_CTL2); > +} > + > +static int nct6694_hif_irq_init(struct nct6694 *nct6694, int irq) > +{ > + struct nct6694_hif_data *hdata = nct6694->priv; > + struct nct6694_sio_data *sio_data = hdata->sio_data; > + int ret; > + > + /* Set SIRQ number */ > + ret = superio_enter(sio_data); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + superio_select(sio_data, SIO_REG_LD_SHM); > + > + if (!superio_inb(sio_data, SIO_REG_SHM_ENABLE)) { > + superio_exit(sio_data); > + return -EIO; > + } > + > + hdata->shm_base = superio_inw(sio_data, SIO_REG_SHM_BASE_ADDR); [Severity: High] What happens if the system BIOS leaves the Logical Device unconfigured and SIO_REG_SHM_BASE_ADDR reads as 0? Will this cause the driver to incorrectly map and write to the Super-I/O index port (0x2E) during interrupt handling? > + superio_outb(sio_data, SIO_REG_SHM_IRQ_NR, irq); [Severity: High] Does writing the irq parameter directly into SIO_REG_SHM_IRQ_NR write a Linux virtual IRQ number (which might be dynamically allocated) into the physical hardware configuration register? > + > + superio_exit(sio_data); > + > + if (!devm_request_region(nct6694->dev, > + hdata->shm_base + NCT6694_SHM_COFS_STS, > + NCT6694_SHM_COFS_LEN, DRVNAME)) > + return -EBUSY; > + > + return 0; > +} [Severity: High] If the hardware returns a response length smaller than the requested length, does this leave the remainder of the caller buffer uninitialized? Can this lead to an information leak or logic corruption when caller drivers parse the buffer? > + > +static void nct6694_hif_core_remove_action(void *data) > +{ > + struct nct6694 *nct6694 = data; > + > + nct6694_core_remove(nct6694); > +} > + > +static const u8 sio_addrs[] = { 0x2e, 0x4e }; > + > +static int nct6694_hif_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct nct6694_sio_data *sio_data; > + struct nct6694_hif_data *hdata; > + struct nct6694 *nct6694; > + void __iomem *rpt_base, *msg_base; > + int ret, i, irq; > + > + rpt_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(rpt_base)) > + return PTR_ERR(rpt_base); > + > + msg_base = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(msg_base)) > + return PTR_ERR(msg_base); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + sio_data = devm_kzalloc(dev, sizeof(*sio_data), GFP_KERNEL); > + if (!sio_data) > + return -ENOMEM; > + > + for (i = 0; i < ARRAY_SIZE(sio_addrs); i++) { > + if (!nct6694_sio_find(sio_data, sio_addrs[i])) > + break; > + } > + if (i == ARRAY_SIZE(sio_addrs)) > + return -ENODEV; > + > + dev_dbg(dev, "Found %s at %#x\n", nct6694_chip_names[sio_data->chip], sio_data->sioreg); > + > + nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL); > + if (!nct6694) > + return -ENOMEM; > + > + hdata = devm_kzalloc(dev, sizeof(*hdata), GFP_KERNEL); > + if (!hdata) > + return -ENOMEM; > + > + hdata->xfer_buf = devm_kzalloc(dev, NCT6694_MAX_PACKET_SIZE, GFP_KERNEL); > + if (!hdata->xfer_buf) > + return -ENOMEM; > + > + hdata->sio_data = sio_data; > + hdata->msg_base = msg_base; > + hdata->rpt_regmap = devm_regmap_init_mmio(dev, rpt_base, > + &nct6694_hif_rpt_regmap_config); > + if (IS_ERR(hdata->rpt_regmap)) > + return PTR_ERR(hdata->rpt_regmap); > + > + nct6694->dev = dev; > + nct6694->priv = hdata; > + nct6694->regmap = devm_regmap_init(dev, &nct6694_hif_regmap_bus, nct6694, > + &nct6694_hif_msg_regmap_config); > + if (IS_ERR(nct6694->regmap)) > + return PTR_ERR(nct6694->regmap); > + > + ret = nct6694_hif_irq_init(nct6694, irq); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + platform_set_drvdata(pdev, nct6694); > + > + ret = nct6694_core_probe(dev, nct6694, nct6694_hif_devs, > + ARRAY_SIZE(nct6694_hif_devs)); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + ret = devm_add_action_or_reset(dev, nct6694_hif_core_remove_action, nct6694); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_hif_irq_handler, > + IRQF_ONESHOT | IRQF_SHARED, > + dev_name(dev), nct6694); > + if (ret) > + return ret; [Severity: High] Because nct6694_core_probe() is called before the threaded IRQ is requested, can child devices that rely on interrupts during initialization hang or fail? Furthermore, due to LIFO devres unwinding, will the parent IRQ be freed before child devices are removed on driver unload? > + > + nct6694_hif_irq_enable(nct6694); > + > + return devm_add_action_or_reset(dev, nct6694_hif_irq_disable, nct6694); > +} > + > +static const struct acpi_device_id nct6694_hif_acpi_ids[] = { > + { "NTN0538", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(acpi, nct6694_hif_acpi_ids); > + > +static struct platform_driver nct6694_hif_driver = { > + .driver = { > + .name = DRVNAME, > + .acpi_match_table = nct6694_hif_acpi_ids, > + }, > + .probe = nct6694_hif_probe, > +}; > +module_platform_driver(nct6694_hif_driver); > + > +MODULE_DESCRIPTION("Nuvoton NCT6694 host-interface transport driver"); > +MODULE_AUTHOR("Ming Yu "); > +MODULE_LICENSE("GPL"); > -- > 2.34.1 > -- Lee Jones