drivers/char/ipmi/ipmi_si.h | 6 +++++- drivers/char/ipmi/ipmi_si_intf.c | 30 ++++++++++++++-------------- drivers/char/ipmi/ipmi_si_pci.c | 8 ++++---- drivers/char/ipmi/ipmi_si_platform.c | 26 +++++++++++++----------- 4 files changed, 38 insertions(+), 32 deletions(-)
Andy reported:
Debian clang version 19.1.7 is not happy when compiled with
`make W=1` (note, CONFIG_WERROR=y is the default):
ipmi_si_platform.c:268:15: error: cast to smaller integer type 'enum si_type'
+from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
268 | io.si_type = (enum
+si_type)device_get_match_data(&pdev->dev);
The IPMI SI type is an enum that was cast into a pointer that was
then cast into an enum again. That's not the greatest style, so
instead create an info structure to hold the data and use that.
Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Corey Minyard <corey@minyard.net>
---
drivers/char/ipmi/ipmi_si.h | 6 +++++-
drivers/char/ipmi/ipmi_si_intf.c | 30 ++++++++++++++--------------
drivers/char/ipmi/ipmi_si_pci.c | 8 ++++----
drivers/char/ipmi/ipmi_si_platform.c | 26 +++++++++++++-----------
4 files changed, 38 insertions(+), 32 deletions(-)
diff --git a/drivers/char/ipmi/ipmi_si.h b/drivers/char/ipmi/ipmi_si.h
index a7ead2a4c753..30b5e5f54172 100644
--- a/drivers/char/ipmi/ipmi_si.h
+++ b/drivers/char/ipmi/ipmi_si.h
@@ -23,6 +23,10 @@ enum si_type {
SI_TYPE_INVALID, SI_KCS, SI_SMIC, SI_BT, SI_TYPE_MAX
};
+struct ipmi_match_info {
+ enum si_type type;
+};
+
/* Array is defined in the ipmi_si_intf.c */
extern const char *const si_to_str[];
@@ -64,7 +68,7 @@ struct si_sm_io {
void (*irq_cleanup)(struct si_sm_io *io);
u8 slave_addr;
- enum si_type si_type;
+ struct ipmi_match_info si_info;
struct device *dev;
};
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7dcf934d5902..21a2c2998a3d 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -692,7 +692,7 @@ static void handle_transaction_done(struct smi_info *smi_info)
break;
}
enables = current_global_enables(smi_info, 0, &irq_on);
- if (smi_info->io.si_type == SI_BT)
+ if (smi_info->io.si_info.type == SI_BT)
/* BT has its own interrupt enable bit. */
check_bt_irq(smi_info, irq_on);
if (enables != (msg[3] & GLOBAL_ENABLES_MASK)) {
@@ -1119,7 +1119,7 @@ irqreturn_t ipmi_si_irq_handler(int irq, void *data)
struct smi_info *smi_info = data;
unsigned long flags;
- if (smi_info->io.si_type == SI_BT)
+ if (smi_info->io.si_info.type == SI_BT)
/* We need to clear the IRQ flag for the BT interface. */
smi_info->io.outputb(&smi_info->io, IPMI_BT_INTMASK_REG,
IPMI_BT_INTMASK_CLEAR_IRQ_BIT
@@ -1164,7 +1164,7 @@ static int smi_start_processing(void *send_info,
* The BT interface is efficient enough to not need a thread,
* and there is no need for a thread if we have interrupts.
*/
- else if ((new_smi->io.si_type != SI_BT) && (!new_smi->io.irq))
+ else if ((new_smi->io.si_info.type != SI_BT) && (!new_smi->io.irq))
enable = 1;
if (enable) {
@@ -1235,7 +1235,7 @@ MODULE_PARM_DESC(kipmid_max_busy_us,
void ipmi_irq_finish_setup(struct si_sm_io *io)
{
- if (io->si_type == SI_BT)
+ if (io->si_info.type == SI_BT)
/* Enable the interrupt in the BT interface. */
io->outputb(io, IPMI_BT_INTMASK_REG,
IPMI_BT_INTMASK_ENABLE_IRQ_BIT);
@@ -1243,7 +1243,7 @@ void ipmi_irq_finish_setup(struct si_sm_io *io)
void ipmi_irq_start_cleanup(struct si_sm_io *io)
{
- if (io->si_type == SI_BT)
+ if (io->si_info.type == SI_BT)
/* Disable the interrupt in the BT interface. */
io->outputb(io, IPMI_BT_INTMASK_REG, 0);
}
@@ -1614,7 +1614,7 @@ static ssize_t type_show(struct device *dev,
{
struct smi_info *smi_info = dev_get_drvdata(dev);
- return sysfs_emit(buf, "%s\n", si_to_str[smi_info->io.si_type]);
+ return sysfs_emit(buf, "%s\n", si_to_str[smi_info->io.si_info.type]);
}
static DEVICE_ATTR_RO(type);
@@ -1649,7 +1649,7 @@ static ssize_t params_show(struct device *dev,
return sysfs_emit(buf,
"%s,%s,0x%lx,rsp=%d,rsi=%d,rsh=%d,irq=%d,ipmb=%d\n",
- si_to_str[smi_info->io.si_type],
+ si_to_str[smi_info->io.si_info.type],
addr_space_to_str[smi_info->io.addr_space],
smi_info->io.addr_data,
smi_info->io.regspacing,
@@ -1803,7 +1803,7 @@ setup_dell_poweredge_bt_xaction_handler(struct smi_info *smi_info)
{
struct ipmi_device_id *id = &smi_info->device_id;
if (id->manufacturer_id == DELL_IANA_MFR_ID &&
- smi_info->io.si_type == SI_BT)
+ smi_info->io.si_info.type == SI_BT)
register_xaction_notifier(&dell_poweredge_bt_xaction_notifier);
}
@@ -1907,13 +1907,13 @@ int ipmi_si_add_smi(struct si_sm_io *io)
/* We prefer ACPI over SMBIOS. */
dev_info(dup->io.dev,
"Removing SMBIOS-specified %s state machine in favor of ACPI\n",
- si_to_str[new_smi->io.si_type]);
+ si_to_str[new_smi->io.si_info.type]);
cleanup_one_si(dup);
} else {
dev_info(new_smi->io.dev,
"%s-specified %s state machine: duplicate\n",
ipmi_addr_src_to_str(new_smi->io.addr_source),
- si_to_str[new_smi->io.si_type]);
+ si_to_str[new_smi->io.si_info.type]);
rv = -EBUSY;
kfree(new_smi);
goto out_err;
@@ -1922,7 +1922,7 @@ int ipmi_si_add_smi(struct si_sm_io *io)
pr_info("Adding %s-specified %s state machine\n",
ipmi_addr_src_to_str(new_smi->io.addr_source),
- si_to_str[new_smi->io.si_type]);
+ si_to_str[new_smi->io.si_info.type]);
list_add_tail(&new_smi->link, &smi_infos);
@@ -1945,12 +1945,12 @@ static int try_smi_init(struct smi_info *new_smi)
pr_info("Trying %s-specified %s state machine at %s address 0x%lx, slave address 0x%x, irq %d\n",
ipmi_addr_src_to_str(new_smi->io.addr_source),
- si_to_str[new_smi->io.si_type],
+ si_to_str[new_smi->io.si_info.type],
addr_space_to_str[new_smi->io.addr_space],
new_smi->io.addr_data,
new_smi->io.slave_addr, new_smi->io.irq);
- switch (new_smi->io.si_type) {
+ switch (new_smi->io.si_info.type) {
case SI_KCS:
new_smi->handlers = &kcs_smi_handlers;
break;
@@ -2073,7 +2073,7 @@ static int try_smi_init(struct smi_info *new_smi)
smi_num++;
dev_info(new_smi->io.dev, "IPMI %s interface initialized\n",
- si_to_str[new_smi->io.si_type]);
+ si_to_str[new_smi->io.si_info.type]);
WARN_ON(new_smi->io.dev->init_name != NULL);
@@ -2309,7 +2309,7 @@ struct device *ipmi_si_remove_by_data(int addr_space, enum si_type si_type,
list_for_each_entry_safe(e, tmp_e, &smi_infos, link) {
if (e->io.addr_space != addr_space)
continue;
- if (e->io.si_type != si_type)
+ if (e->io.si_info.type != si_type)
continue;
if (e->io.addr_data == addr) {
dev = get_device(e->io.dev);
diff --git a/drivers/char/ipmi/ipmi_si_pci.c b/drivers/char/ipmi/ipmi_si_pci.c
index 8c0ea637aba0..1f4bdb3bbdd4 100644
--- a/drivers/char/ipmi/ipmi_si_pci.c
+++ b/drivers/char/ipmi/ipmi_si_pci.c
@@ -23,7 +23,7 @@ MODULE_PARM_DESC(trypci,
static int ipmi_pci_probe_regspacing(struct si_sm_io *io)
{
- if (io->si_type == SI_KCS) {
+ if (io->si_info.type == SI_KCS) {
unsigned char status;
int regspacing;
@@ -74,15 +74,15 @@ static int ipmi_pci_probe(struct pci_dev *pdev,
switch (pdev->class) {
case PCI_CLASS_SERIAL_IPMI_SMIC:
- io.si_type = SI_SMIC;
+ io.si_info.type = SI_SMIC;
break;
case PCI_CLASS_SERIAL_IPMI_KCS:
- io.si_type = SI_KCS;
+ io.si_info.type = SI_KCS;
break;
case PCI_CLASS_SERIAL_IPMI_BT:
- io.si_type = SI_BT;
+ io.si_info.type = SI_BT;
break;
default:
diff --git a/drivers/char/ipmi/ipmi_si_platform.c b/drivers/char/ipmi/ipmi_si_platform.c
index 550cabd43ae6..53066713f8e4 100644
--- a/drivers/char/ipmi/ipmi_si_platform.c
+++ b/drivers/char/ipmi/ipmi_si_platform.c
@@ -165,7 +165,7 @@ static int platform_ipmi_probe(struct platform_device *pdev)
case SI_KCS:
case SI_SMIC:
case SI_BT:
- io.si_type = type;
+ io.si_info.type = type;
break;
case SI_TYPE_INVALID: /* User disabled this in hardcode. */
return -ENODEV;
@@ -212,13 +212,14 @@ static int platform_ipmi_probe(struct platform_device *pdev)
}
#ifdef CONFIG_OF
+static struct ipmi_match_info kcs_info = { .type = SI_KCS };
+static struct ipmi_match_info smic_info = { .type = SI_SMIC };
+static struct ipmi_match_info bt_info = { .type = SI_BT };
+
static const struct of_device_id of_ipmi_match[] = {
- { .type = "ipmi", .compatible = "ipmi-kcs",
- .data = (void *)(unsigned long) SI_KCS },
- { .type = "ipmi", .compatible = "ipmi-smic",
- .data = (void *)(unsigned long) SI_SMIC },
- { .type = "ipmi", .compatible = "ipmi-bt",
- .data = (void *)(unsigned long) SI_BT },
+ { .type = "ipmi", .compatible = "ipmi-kcs", .data = &kcs_info },
+ { .type = "ipmi", .compatible = "ipmi-smic", .data = &smic_info },
+ { .type = "ipmi", .compatible = "ipmi-bt", .data = &bt_info },
{},
};
MODULE_DEVICE_TABLE(of, of_ipmi_match);
@@ -265,7 +266,8 @@ static int of_ipmi_probe(struct platform_device *pdev)
}
memset(&io, 0, sizeof(io));
- io.si_type = (enum si_type)device_get_match_data(&pdev->dev);
+ io.si_info = *((struct ipmi_match_info *)
+ device_get_match_data(&pdev->dev));
io.addr_source = SI_DEVICETREE;
io.irq_setup = ipmi_std_irq_setup;
@@ -296,7 +298,7 @@ static int find_slave_address(struct si_sm_io *io, int slave_addr)
{
#ifdef CONFIG_IPMI_DMI_DECODE
if (!slave_addr)
- slave_addr = ipmi_dmi_get_slave_addr(io->si_type,
+ slave_addr = ipmi_dmi_get_slave_addr(io->si_info.type,
io->addr_space,
io->addr_data);
#endif
@@ -335,13 +337,13 @@ static int acpi_ipmi_probe(struct platform_device *pdev)
switch (tmp) {
case 1:
- io.si_type = SI_KCS;
+ io.si_info.type = SI_KCS;
break;
case 2:
- io.si_type = SI_SMIC;
+ io.si_info.type = SI_SMIC;
break;
case 3:
- io.si_type = SI_BT;
+ io.si_info.type = SI_BT;
break;
case 4: /* SSIF, just ignore */
return -ENODEV;
--
2.43.0On Wed, Apr 16, 2025 at 07:09:12AM -0500, Corey Minyard wrote:
> Andy reported:
>
> Debian clang version 19.1.7 is not happy when compiled with
> `make W=1` (note, CONFIG_WERROR=y is the default):
>
> ipmi_si_platform.c:268:15: error: cast to smaller integer type 'enum si_type'
> +from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
> 268 | io.si_type = (enum
> +si_type)device_get_match_data(&pdev->dev);
>
> The IPMI SI type is an enum that was cast into a pointer that was
> then cast into an enum again. That's not the greatest style, so
> instead create an info structure to hold the data and use that.
I'm going to test this today or latest tomorrow, below some comments.
...
> u8 slave_addr;
> - enum si_type si_type;
> + struct ipmi_match_info si_info;
const struct ipmi_match_info *si_info;
?
I understand that right now there is no benefit, but my suggestion is scalable
and prevents from sudden values rewrites. Also that's how other drivers do, but
of course not all of them.
> struct device *dev;
...
> - else if ((new_smi->io.si_type != SI_BT) && (!new_smi->io.irq))
> + else if ((new_smi->io.si_info.type != SI_BT) && (!new_smi->io.irq))
While at it, drop unneeded parentheses (at least in the second part).
> enable = 1;
...
> static int ipmi_pci_probe_regspacing(struct si_sm_io *io)
> {
> - if (io->si_type == SI_KCS) {
> + if (io->si_info.type == SI_KCS) {
> unsigned char status;
> int regspacing;
It looks like the above conditional is for the whole function, if this is
the case, I would go with an additional patch to drop the indentation level.
unsigned char status;
int regspacing;
...
if (io->si_info.type != SI_KCS)
return ...
...
> #ifdef CONFIG_OF
I would clean up this ugly ifdeffery as well, but it can be done after this
patch.
> +static struct ipmi_match_info kcs_info = { .type = SI_KCS };
> +static struct ipmi_match_info smic_info = { .type = SI_SMIC };
> +static struct ipmi_match_info bt_info = { .type = SI_BT };
> +
> static const struct of_device_id of_ipmi_match[] = {
> - { .type = "ipmi", .compatible = "ipmi-kcs",
> - .data = (void *)(unsigned long) SI_KCS },
> - { .type = "ipmi", .compatible = "ipmi-smic",
> - .data = (void *)(unsigned long) SI_SMIC },
> - { .type = "ipmi", .compatible = "ipmi-bt",
> - .data = (void *)(unsigned long) SI_BT },
> + { .type = "ipmi", .compatible = "ipmi-kcs", .data = &kcs_info },
> + { .type = "ipmi", .compatible = "ipmi-smic", .data = &smic_info },
> + { .type = "ipmi", .compatible = "ipmi-bt", .data = &bt_info },
> {},
...and this line should not have a trailing comma.
> };
> MODULE_DEVICE_TABLE(of, of_ipmi_match);
...
> + io.si_info = *((struct ipmi_match_info *)
> + device_get_match_data(&pdev->dev));
This ugly casting won't be needed if you use const pointer as suggested above.
io.si_info = *((struct ipmi_match_info *)
device_get_match_data(&pdev->dev));
...
> switch (tmp) {
> case 1:
> - io.si_type = SI_KCS;
> + io.si_info.type = SI_KCS;
> break;
> case 2:
> - io.si_type = SI_SMIC;
> + io.si_info.type = SI_SMIC;
> break;
> case 3:
> - io.si_type = SI_BT;
> + io.si_info.type = SI_BT;
> break;
> case 4: /* SSIF, just ignore */
> return -ENODEV;
I haven't looked where tmp comes from, but maybe that's a candidate to be in
the info structure to begin with?
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 16, 2025 at 08:45:51PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 16, 2025 at 07:09:12AM -0500, Corey Minyard wrote:
> > Andy reported:
> >
> > Debian clang version 19.1.7 is not happy when compiled with
> > `make W=1` (note, CONFIG_WERROR=y is the default):
> >
> > ipmi_si_platform.c:268:15: error: cast to smaller integer type 'enum si_type'
> > +from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
> > 268 | io.si_type = (enum
> > +si_type)device_get_match_data(&pdev->dev);
> >
> > The IPMI SI type is an enum that was cast into a pointer that was
> > then cast into an enum again. That's not the greatest style, so
> > instead create an info structure to hold the data and use that.
>
> I'm going to test this today or latest tomorrow, below some comments.
I have noticed the commit 37631eee2063 ("ipmi:si: Move SI type information into
an info structure") which is in Linux Next and it compiles for me as expected,
thank you for the prompt fix!
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.