[Openipmi-developer] OpenIPMI 2.0.35 released
I spent some time going analyzing with Coverity and CodeQL to clean up a bunch of issues. No functional changes, just bug fixes. You should upgrade. -corey ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 0/6] ipmi: Convert to platform remove callback returning void
On Thu, Apr 11, 2024 at 09:15:03AM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Mar 05, 2024 at 05:26:57PM +0100, Uwe Kleine-König wrote:
> > this series converts all drivers below drivers/char/ipmi to struct
> > platform_driver::remove_new(). See commit 5c5a7680e67b ("platform: Provide a
> > remove callback that returns no value") for an extended explanation and the
> > eventual goal.
> >
> > All conversations are trivial, because their .remove() callbacks
> > returned zero unconditionally.
> >
> > There are no interdependencies between these patches, so they could be
> > picked up individually. But I'd hope that they get picked up all
> > together by Corey.
Yeah, I was kind of waiting for more reviews, but this is pretty
straightforward. I've pulled this into my tree.
-corey
>
> Apart from a (positive) review reply I didn't get any feedback to this
> series. My quest to change the prototype of struct
> platform_driver::remove depends on these patches, so it would be great
> if they made it in during the next merge window.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 0/1] char: ipmi: Handle HAS_IOPORT dependencies
On Thu, Apr 04, 2024 at 12:45:05PM +0200, Niklas Schnelle wrote: > Hi Corey, > > This is a follow up in my ongoing effort of making inb()/outb() and > similar I/O port accessors compile-time optional. Previously I sent this > as a treewide series titled "treewide: Remove I/O port accessors for > HAS_IOPORT=n" with the latest being its 5th version[0]. With a significant > subset of patches merged I've changed over to per-subsystem series. These > series are stand alone and should be merged via the relevant tree such > that with all subsystems complete we can follow this up with the final > patch that will make the I/O port accessors compile-time optional. > > The current state of the full series with changes to the remaining > subsystems and the aforementioned final patch can be found for your > convenience on my git.kernel.org tree in the has_ioport_v6 branch[1] with > signed tags. As for compile-time vs runtime see Linus' reply to my first > attempt[2]. Sorry, my bad, I've been out a lot recently and dealing with a bunch of issues and I missed this. It's in my tree now and it looks good. -corey > > Thanks, > Niklas > > [0] > https://lore.kernel.org/all/20230522105049.1467313-1-schne...@linux.ibm.com/ > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/niks/linux.git/log/?h=has_ioport_v6 > [2] > https://lore.kernel.org/lkml/CAHk-=wg80je=k7madf4e7wrrnp37e3qh6y10svhdc7o8sz_...@mail.gmail.com/ > > Niklas Schnelle (1): > char: ipmi: handle HAS_IOPORT dependencies > > drivers/char/ipmi/Makefile | 11 --- > drivers/char/ipmi/ipmi_si_intf.c | 3 ++- > drivers/char/ipmi/ipmi_si_pci.c | 3 +++ > 3 files changed, 9 insertions(+), 8 deletions(-) > > -- > 2.40.1 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 33/34] drivers: remove incorrect of_match_ptr/ACPI_PTR annotations
On Wed, Apr 03, 2024 at 12:30:44PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 03, 2024 at 10:06:51AM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > When building with CONFIG_OF and/or CONFIG_ACPI disabled but W=1 extra
> > warnings enabled, a lot of driver cause a warning about an unused
> > ID table:
> >
> > drivers/char/tpm/tpm_ftpm_tee.c:356:34: error: unused variable
> > 'of_ftpm_tee_ids' [-Werror,-Wunused-const-variable]
> > drivers/dma/img-mdc-dma.c:863:34: error: unused variable 'mdc_dma_of_match'
> > [-Werror,-Wunused-const-variable]
> > drivers/fpga/versal-fpga.c:62:34: error: unused variable
> > 'versal_fpga_of_match' [-Werror,-Wunused-const-variable]
> > drivers/i2c/muxes/i2c-mux-ltc4306.c:200:34: error: unused variable
> > 'ltc4306_of_match' [-Werror,-Wunused-const-variable]
> > drivers/i2c/muxes/i2c-mux-reg.c:242:34: error: unused variable
> > 'i2c_mux_reg_of_match' [-Werror,-Wunused-const-variable]
> > drivers/memory/pl353-smc.c:62:34: error: unused variable
> > 'pl353_smc_supported_children' [-Werror,-Wunused-const-variable]
> > drivers/regulator/pbias-regulator.c:136:34: error: unused variable
> > 'pbias_of_match' [-Werror,-Wunused-const-variable]
> > drivers/regulator/twl-regulator.c:552:34: error: unused variable
> > 'twl_of_match' [-Werror,-Wunused-const-variable]
> > drivers/regulator/twl6030-regulator.c:645:34: error: unused variable
> > 'twl_of_match' [-Werror,-Wunused-const-variable]
> > drivers/scsi/hisi_sas/hisi_sas_v2_hw.c:3635:36: error: unused variable
> > 'sas_v2_acpi_match' [-Werror,-Wunused-const-variable]
> > drivers/staging/pi433/pi433_if.c:1359:34: error: unused variable
> > 'pi433_dt_ids' [-Werror,-Wunused-const-variable]
> > drivers/tty/serial/amba-pl011.c:2945:34: error: unused variable
> > 'sbsa_uart_of_match' [-Werror,-Wunused-const-variable]
> >
> > The fix is always to just remove the of_match_ptr() and ACPI_PTR() wrappers
> > that remove the reference, rather than adding another #ifdef just for build
> > testing for a configuration that doesn't matter in practice.
>
> > I considered splitting up the large patch into per subsystem patches, but
> > since
> > it's really just the same thing everywhere it feels better to do it all at
> > once.
>
> Can we split to three groups:
> - Dropping ACPI_PTR()
> - Dropping of_match_ptr() (which I won't review in depth, for example)
> - Dropping both
> ?
Why?
-corey
>
> ...
>
> > --- a/drivers/char/ipmi/ipmb_dev_int.c
> > +++ b/drivers/char/ipmi/ipmb_dev_int.c
> > @@ -364,7 +364,7 @@ MODULE_DEVICE_TABLE(acpi, acpi_ipmb_id);
> > static struct i2c_driver ipmb_driver = {
> > .driver = {
> > .name = "ipmb-dev",
> > - .acpi_match_table = ACPI_PTR(acpi_ipmb_id),
> > + .acpi_match_table = acpi_ipmb_id,
> > },
> > .probe = ipmb_probe,
> > .remove = ipmb_remove,
>
> acpi.h --> mod_devicetable.h.
>
> ...
>
> > --- a/drivers/hid/hid-google-hammer.c
> > +++ b/drivers/hid/hid-google-hammer.c
> > @@ -275,21 +275,19 @@ static const struct acpi_device_id cbas_ec_acpi_ids[]
> > = {
> > };
> > MODULE_DEVICE_TABLE(acpi, cbas_ec_acpi_ids);
> >
> > -#ifdef CONFIG_OF
> > static const struct of_device_id cbas_ec_of_match[] = {
> > { .compatible = "google,cros-cbas" },
> > { },
> > };
> > MODULE_DEVICE_TABLE(of, cbas_ec_of_match);
> > -#endif
> >
> > static struct platform_driver cbas_ec_driver = {
> > .probe = cbas_ec_probe,
> > .remove = cbas_ec_remove,
> > .driver = {
> > .name = "cbas_ec",
> > - .acpi_match_table = ACPI_PTR(cbas_ec_acpi_ids),
> > - .of_match_table = of_match_ptr(cbas_ec_of_match),
> > + .acpi_match_table = cbas_ec_acpi_ids,
> > + .of_match_table = cbas_ec_of_match,
> > .pm = _ec_pm_ops,
> > },
> > };
>
> Ditto, and likely of.h can be also dropped.
>
> ...
>
> > --- a/drivers/input/touchscreen/wdt87xx_i2c.c
> > +++ b/drivers/input/touchscreen/wdt87xx_i2c.c
> > @@ -1166,7 +1166,7 @@ static struct i2c_driver wdt87xx_driver = {
> > .name = WDT87XX_NAME,
> > .dev_groups = wdt87xx_groups,
> > .pm = pm_sleep_ptr(_pm_ops),
> > - .acpi_match_table = ACPI_PTR(wdt87xx_acpi_id),
> > + .acpi_match_table = wdt87xx_acpi_id,
> > },
> > };
> > module_i2c_driver(wdt87xx_driver);
>
> Ditto.
>
> ...
>
> > --- a/drivers/net/ethernet/apm/xgene-v2/main.c
> > +++ b/drivers/net/ethernet/apm/xgene-v2/main.c
> > @@ -731,7 +731,7 @@ MODULE_DEVICE_TABLE(acpi, xge_acpi_match);
> > static struct platform_driver xge_driver = {
> > .driver = {
> >.name = "xgene-enet-v2",
> > - .acpi_match_table = ACPI_PTR(xge_acpi_match),
> > + .acpi_match_table = xge_acpi_match,
> > },
> > .probe = xge_probe,
> > .remove_new = xge_remove,
>
> Ditto. And remove forward declaration of the variable as well.
>
> ...
>
> > --- a/drivers/rtc/rtc-fsl-ftm-alarm.c
> > +++
Re: [Openipmi-developer] [PATCH 33/34] drivers: remove incorrect of_match_ptr/ACPI_PTR annotations
On Wed, Apr 03, 2024 at 10:06:51AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When building with CONFIG_OF and/or CONFIG_ACPI disabled but W=1 extra
> warnings enabled, a lot of driver cause a warning about an unused
> ID table:
>
> drivers/char/tpm/tpm_ftpm_tee.c:356:34: error: unused variable
> 'of_ftpm_tee_ids' [-Werror,-Wunused-const-variable]
> drivers/dma/img-mdc-dma.c:863:34: error: unused variable 'mdc_dma_of_match'
> [-Werror,-Wunused-const-variable]
> drivers/fpga/versal-fpga.c:62:34: error: unused variable
> 'versal_fpga_of_match' [-Werror,-Wunused-const-variable]
> drivers/i2c/muxes/i2c-mux-ltc4306.c:200:34: error: unused variable
> 'ltc4306_of_match' [-Werror,-Wunused-const-variable]
> drivers/i2c/muxes/i2c-mux-reg.c:242:34: error: unused variable
> 'i2c_mux_reg_of_match' [-Werror,-Wunused-const-variable]
> drivers/memory/pl353-smc.c:62:34: error: unused variable
> 'pl353_smc_supported_children' [-Werror,-Wunused-const-variable]
> drivers/regulator/pbias-regulator.c:136:34: error: unused variable
> 'pbias_of_match' [-Werror,-Wunused-const-variable]
> drivers/regulator/twl-regulator.c:552:34: error: unused variable
> 'twl_of_match' [-Werror,-Wunused-const-variable]
> drivers/regulator/twl6030-regulator.c:645:34: error: unused variable
> 'twl_of_match' [-Werror,-Wunused-const-variable]
> drivers/scsi/hisi_sas/hisi_sas_v2_hw.c:3635:36: error: unused variable
> 'sas_v2_acpi_match' [-Werror,-Wunused-const-variable]
> drivers/staging/pi433/pi433_if.c:1359:34: error: unused variable
> 'pi433_dt_ids' [-Werror,-Wunused-const-variable]
> drivers/tty/serial/amba-pl011.c:2945:34: error: unused variable
> 'sbsa_uart_of_match' [-Werror,-Wunused-const-variable]
>
> The fix is always to just remove the of_match_ptr() and ACPI_PTR() wrappers
> that remove the reference, rather than adding another #ifdef just for build
> testing for a configuration that doesn't matter in practice.
>
> I considered splitting up the large patch into per subsystem patches, but
> since
> it's really just the same thing everywhere it feels better to do it all at
> once.
>
> Signed-off-by: Arnd Bergmann
For the IPMI part:
Acked-by: Corey Minyard
> ---
> drivers/char/ipmi/ipmb_dev_int.c | 2 +-
> drivers/char/tpm/tpm_ftpm_tee.c | 2 +-
> drivers/dma/img-mdc-dma.c | 2 +-
> drivers/fpga/versal-fpga.c| 2 +-
> drivers/hid/hid-google-hammer.c | 6 ++
> drivers/i2c/muxes/i2c-mux-ltc4306.c | 2 +-
> drivers/i2c/muxes/i2c-mux-reg.c | 2 +-
> drivers/input/touchscreen/wdt87xx_i2c.c | 2 +-
> drivers/mux/adg792a.c | 2 +-
> drivers/net/ethernet/apm/xgene-v2/main.c | 2 +-
> drivers/net/ethernet/hisilicon/hns_mdio.c | 2 +-
> drivers/regulator/pbias-regulator.c | 2 +-
> drivers/regulator/twl-regulator.c | 2 +-
> drivers/regulator/twl6030-regulator.c | 2 +-
> drivers/rtc/rtc-fsl-ftm-alarm.c | 2 +-
> drivers/scsi/hisi_sas/hisi_sas_v1_hw.c| 2 +-
> drivers/scsi/hisi_sas/hisi_sas_v2_hw.c| 2 +-
> drivers/staging/pi433/pi433_if.c | 2 +-
> drivers/tty/serial/amba-pl011.c | 6 +++---
> drivers/tty/serial/ma35d1_serial.c| 2 +-
> 20 files changed, 23 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c
> b/drivers/char/ipmi/ipmb_dev_int.c
> index 49100845fcb7..5e7bfc7c26e2 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -364,7 +364,7 @@ MODULE_DEVICE_TABLE(acpi, acpi_ipmb_id);
> static struct i2c_driver ipmb_driver = {
> .driver = {
> .name = "ipmb-dev",
> - .acpi_match_table = ACPI_PTR(acpi_ipmb_id),
> + .acpi_match_table = acpi_ipmb_id,
> },
> .probe = ipmb_probe,
> .remove = ipmb_remove,
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 2ea4882251cf..0c453f3f928d 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -362,7 +362,7 @@ MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
> static struct platform_driver ftpm_tee_plat_driver = {
> .driver = {
> .name = "ftpm-tee",
> - .of_match_table = of_match_ptr(of_ftpm_tee_ids),
> + .of_match_table = of_ftpm_tee_ids,
> },
> .shutdown = ftpm_plat_tee_shutdown,
> .probe = ftpm_plat_tee_probe,
> diff --git a/drivers/dma/img-mdc-dma.c b/drivers/dma/img-mdc-dma.c
> index 0532dd2640dc..6931c8a65415 100644
> --- a/drivers/dma/img-mdc-dma.c
> +++ b/drivers/dma/img-mdc-dma.c
> @@ -1073,7 +1073,7 @@ static struct platform_driver md
Re: [Openipmi-developer] [PATCH 6/9] ipmi: Convert from tasklet to BH workqueue
On Thu, Mar 28, 2024 at 12:41:22PM -0700, Allen wrote: > > > > I believe that work queues items are execute single-threaded for a work > > > > queue, so this should be good. I need to test this, though. It may be > > > > that an IPMI device can have its own work queue; it may not be important > > > > to run it in bh context. > > > > > > Fair point. Could you please let me know once you have had a chance to > > > test > > > these changes. Meanwhile, I will work on RFC wherein IPMI will have its > > > own > > > workqueue. > > > > > > Thanks for taking time out to review. > > > > After looking and thinking about it a bit, a BH context is still > > probably the best for this. > > > > I have tested this patch under load and various scenarios and it seems > > to work ok. So: > > > > Tested-by: Corey Minyard > > Acked-by: Corey Minyard > > > > Or I can take this into my tree. > > > > -corey > > Thank you very much. I think it should be okay for you to carry it into > your tree. Ok, it's in my for-next tree. I fixed the directory reference, and I changed all the comments where you changed "tasklet" to "work" to instead say "workqueue". -corey > > - Allen > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 6/9] ipmi: Convert from tasklet to BH workqueue
On Thu, Mar 28, 2024 at 10:52:16AM -0700, Allen wrote:
> On Wed, Mar 27, 2024 at 11:05 AM Corey Minyard wrote:
> >
> > I believe that work queues items are execute single-threaded for a work
> > queue, so this should be good. I need to test this, though. It may be
> > that an IPMI device can have its own work queue; it may not be important
> > to run it in bh context.
>
> Fair point. Could you please let me know once you have had a chance to test
> these changes. Meanwhile, I will work on RFC wherein IPMI will have its own
> workqueue.
>
> Thanks for taking time out to review.
After looking and thinking about it a bit, a BH context is still
probably the best for this.
I have tested this patch under load and various scenarios and it seems
to work ok. So:
Tested-by: Corey Minyard
Acked-by: Corey Minyard
Or I can take this into my tree.
-corey
>
> - Allen
>
> >
> > -corey
> >
> > >
> > > Based on the work done by Tejun Heo
> > > Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-610
> > >
> > > Signed-off-by: Allen Pais
> > > ---
> > > drivers/char/ipmi/ipmi_msghandler.c | 30 ++---
> > > 1 file changed, 15 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > > b/drivers/char/ipmi/ipmi_msghandler.c
> > > index b0eedc4595b3..fce2a2dbdc82 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -36,12 +36,13 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #define IPMI_DRIVER_VERSION "39.2"
> > >
> > > static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void);
> > > static int ipmi_init_msghandler(void);
> > > -static void smi_recv_tasklet(struct tasklet_struct *t);
> > > +static void smi_recv_work(struct work_struct *t);
> > > static void handle_new_recv_msgs(struct ipmi_smi *intf);
> > > static void need_waiter(struct ipmi_smi *intf);
> > > static int handle_one_recv_msg(struct ipmi_smi *intf,
> > > @@ -498,13 +499,13 @@ struct ipmi_smi {
> > > /*
> > >* Messages queued for delivery. If delivery fails (out of memory
> > >* for instance), They will stay in here to be processed later in a
> > > - * periodic timer interrupt. The tasklet is for handling received
> > > + * periodic timer interrupt. The work is for handling received
> > >* messages directly from the handler.
> > >*/
> > > spinlock_t waiting_rcv_msgs_lock;
> > > struct list_head waiting_rcv_msgs;
> > > atomic_t watchdog_pretimeouts_to_deliver;
> > > - struct tasklet_struct recv_tasklet;
> > > + struct work_struct recv_work;
> > >
> > > spinlock_t xmit_msgs_lock;
> > > struct list_head xmit_msgs;
> > > @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi
> > > *intf)
> > > struct cmd_rcvr *rcvr, *rcvr2;
> > > struct list_head list;
> > >
> > > - tasklet_kill(>recv_tasklet);
> > > + cancel_work_sync(>recv_work);
> > >
> > > free_smi_msg_list(>waiting_rcv_msgs);
> > > free_recv_msg_list(>waiting_events);
> > > @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref)
> > > {
> > > struct ipmi_user *user = container_of(ref, struct ipmi_user,
> > > refcount);
> > >
> > > - /* SRCU cleanup must happen in task context. */
> > > + /* SRCU cleanup must happen in work context. */
> > > queue_work(remove_work_wq, >remove_work);
> > > }
> > >
> > > @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner,
> > > intf->curr_seq = 0;
> > > spin_lock_init(>waiting_rcv_msgs_lock);
> > > INIT_LIST_HEAD(>waiting_rcv_msgs);
> > > - tasklet_setup(>recv_tasklet,
> > > - smi_recv_tasklet);
> > > + INIT_WORK(>recv_work, smi_recv_work);
> > > atomic_set(>watchdog_pretimeouts_to_deliver, 0);
> > > spin_lock_init(>xmit_msgs_lock);
> > > INIT_LIST_HEAD(>xmit_msgs);
> > > @@ -4779,7 +4779,7 @@ static void handle_new_recv_msgs(struct ipmi_smi
> > > *intf)
> > >* To preser
Re: [Openipmi-developer] [PATCH 6/9] ipmi: Convert from tasklet to BH workqueue
On Wed, Mar 27, 2024 at 04:03:11PM +, Allen Pais wrote:
> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
>
> This patch converts drivers/infiniband/* from tasklet to BH workqueue.
I think you mean drivers/char/ipmi/* here.
I believe that work queues items are execute single-threaded for a work
queue, so this should be good. I need to test this, though. It may be
that an IPMI device can have its own work queue; it may not be important
to run it in bh context.
-corey
>
> Based on the work done by Tejun Heo
> Branch: https://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git for-6.10
>
> Signed-off-by: Allen Pais
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 30 ++---
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index b0eedc4595b3..fce2a2dbdc82 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -36,12 +36,13 @@
> #include
> #include
> #include
> +#include
>
> #define IPMI_DRIVER_VERSION "39.2"
>
> static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void);
> static int ipmi_init_msghandler(void);
> -static void smi_recv_tasklet(struct tasklet_struct *t);
> +static void smi_recv_work(struct work_struct *t);
> static void handle_new_recv_msgs(struct ipmi_smi *intf);
> static void need_waiter(struct ipmi_smi *intf);
> static int handle_one_recv_msg(struct ipmi_smi *intf,
> @@ -498,13 +499,13 @@ struct ipmi_smi {
> /*
>* Messages queued for delivery. If delivery fails (out of memory
>* for instance), They will stay in here to be processed later in a
> - * periodic timer interrupt. The tasklet is for handling received
> + * periodic timer interrupt. The work is for handling received
>* messages directly from the handler.
>*/
> spinlock_t waiting_rcv_msgs_lock;
> struct list_head waiting_rcv_msgs;
> atomic_t watchdog_pretimeouts_to_deliver;
> - struct tasklet_struct recv_tasklet;
> + struct work_struct recv_work;
>
> spinlock_t xmit_msgs_lock;
> struct list_head xmit_msgs;
> @@ -704,7 +705,7 @@ static void clean_up_interface_data(struct ipmi_smi *intf)
> struct cmd_rcvr *rcvr, *rcvr2;
> struct list_head list;
>
> - tasklet_kill(>recv_tasklet);
> + cancel_work_sync(>recv_work);
>
> free_smi_msg_list(>waiting_rcv_msgs);
> free_recv_msg_list(>waiting_events);
> @@ -1319,7 +1320,7 @@ static void free_user(struct kref *ref)
> {
> struct ipmi_user *user = container_of(ref, struct ipmi_user, refcount);
>
> - /* SRCU cleanup must happen in task context. */
> + /* SRCU cleanup must happen in work context. */
> queue_work(remove_work_wq, >remove_work);
> }
>
> @@ -3605,8 +3606,7 @@ int ipmi_add_smi(struct module *owner,
> intf->curr_seq = 0;
> spin_lock_init(>waiting_rcv_msgs_lock);
> INIT_LIST_HEAD(>waiting_rcv_msgs);
> - tasklet_setup(>recv_tasklet,
> - smi_recv_tasklet);
> + INIT_WORK(>recv_work, smi_recv_work);
> atomic_set(>watchdog_pretimeouts_to_deliver, 0);
> spin_lock_init(>xmit_msgs_lock);
> INIT_LIST_HEAD(>xmit_msgs);
> @@ -4779,7 +4779,7 @@ static void handle_new_recv_msgs(struct ipmi_smi *intf)
>* To preserve message order, quit if we
>* can't handle a message. Add the message
>* back at the head, this is safe because this
> - * tasklet is the only thing that pulls the
> + * work is the only thing that pulls the
>* messages.
>*/
> list_add(_msg->link, >waiting_rcv_msgs);
> @@ -4812,10 +4812,10 @@ static void handle_new_recv_msgs(struct ipmi_smi
> *intf)
> }
> }
>
> -static void smi_recv_tasklet(struct tasklet_struct *t)
> +static void smi_recv_work(struct work_struct *t)
> {
> unsigned long flags = 0; /* keep us warning-free. */
> - struct ipmi_smi *intf = from_tasklet(intf, t, recv_tasklet);
> + struct ipmi_smi *intf = from_work(intf, t, recv_work);
> int run_to_completion = intf->run_to_completion;
> struct ipmi_smi_msg *newmsg = NULL;
>
> @@ -4866,7 +4866,7 @@ void ipmi_smi_msg_received(struct ipmi_smi *intf,
>
> /*
>* To preserve message order, we keep a queue and deliver from
> - * a tasklet.
> + * a work.
>*/
> if (!run_to_completion)
> spin_lock_irqsave(>waiting_rcv_msgs_lock, flags);
> @@ -4887,9
Re: [Openipmi-developer] [PATCH] ipmi: kcs: Update OBF poll timeout to reduce latency
On Wed, Feb 21, 2024 at 10:57:38AM -0600, Andrew Geissler wrote: > > > > On Feb 20, 2024, at 4:36 PM, Andrew Jeffery > > wrote: > > > > On Tue, 2024-02-20 at 13:33 -0600, Corey Minyard wrote: > >> On Tue, Feb 20, 2024 at 04:51:21PM +0100, Paul Menzel wrote: > >>> Dear Andrew, > >> > >> It's because increasing that number causes it to poll longer for the > >> event, the host takes longer than 100us to generate the event, and if > >> the event is missed the time when it is checked again is very long. > >> > >> Polling for 100us is already pretty extreme. 200us is really too long. > >> > >> The real problem is that there is no interrupt for this. I'd also guess > >> there is no interrupt on the host side, because that would solve this > >> problem, too, as it would certainly get around to handling the interupt > >> in 100us. I'm assuming the host driver is not the Linux driver, as it > >> should also handle this in a timely manner, even when polling. > > > > I expect the issues Andrew G is observing are with the Power10 boot > > firmware. The boot firmware only polls. The runtime firmware enables > > interrupts. > > Yep, this is with the low level host boot firmware. > Also, further testing over night showed that 200us wasn’t enough for > our larger Everest P10 machines, I needed to go to 300us. As we > were struggling to allow 200us, I assume 300us is going to be a no-go. It seems odd to me that firmware polling would be an issue. Usually, with firmware, you have it just spinning waiting for something. At least in the firmware I worked with. I'm not familiar with this firmware, though, maybe it has timers and such and parallel execution. Can this be fixed on the firmware side? > > >> > > > >> > >> The right way to fix this is probably to do the same thing the host side > >> Linux driver does. It has a kernel thread that is kicked off to do > >> this. Unfortunately, that's more complicated to implement, but it > >> avoids polling in this location (which causes latency issues on the BMC > >> side) and lets you poll longer without causing issues. > > > > In Andrew G's case he's talking MCTP over KCS using a vendor-defined > > transport binding (that also leverages LPC FWH cycles for bulk data > > transfers)[1]. I think it could have taken more inspiration from the > > IPMI KCS protocol: It might be worth an experiment to write the dummy > > command value to IDR from the host side after each ODR read to signal > > the host's clearing of OBF (no interrupt for the BMC) with an IBF > > (which does interrupt the BMC). And doing the obverse for the BMC. Some > > brief thought suggests that if the dummy value is read there's no need > > to send a dummy value in reply (as it's an indicator to read the status > > register). With that the need for the spin here (or on the host side) > > is reduced at the cost of some constant protocol overhead. > > > > Thanks for the quick reviews and ideas. > I’ll see if I can find someone on the team to help out with Andrew J’s > thoughts and if that doesn’t work, look into the kernel thread idea. I don't really understand Andrew J's ideas very well, but hopefully they help. The kernel thread idea is fairly complicated to implement, and there has been an impetus in the kernel to not create new kernel threads. But there just has to be a good reason, and this probably is one. We worked on it a lot in the IPMI host driver to tune it and got it to a point where it provided decent performance without causing power management issues. When I first read the title I was worried it was talking about this code; I'm lothe to touch it for fear of breaking things. -corey ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs: Update OBF poll timeout to reduce latency
On Tue, Feb 20, 2024 at 04:51:21PM +0100, Paul Menzel wrote:
> Dear Andrew,
>
>
> Thank you for your patch. Some style suggestions.
>
> Am 20.02.24 um 13:36 schrieb Andrew Geissler:
> > From: Andrew Geissler
>
> (Oh no, Yahoo. (ignore))
>
> You could be more specific in the git commit message by using *Double*:
>
> > ipmi: kcs: Double OBF poll timeout to reduce latency
>
> > ipmi: kcs: Double OBF poll timeout to 200 us to reduce latency
>
> > Commit f90bc0f97f2b ("ipmi: kcs: Poll OBF briefly to reduce OBE
> > latency") introduced an optimization to poll when the host has
I assume that removing that patch doesn't fix the issue, it would only
make it worse, right?
> > read the output data register (ODR). Testing has shown that the 100us
> > timeout was not always enough. When we miss that 100us window, it
> > results in 10x the time to get the next message from the BMC to the
> > host. When you're sending 100's of messages between the BMC and Host,
>
> I do not understand, how this poll timeout can result in such an increase,
> and why a quite big timeout hurts, but I do not know the implementation.
It's because increasing that number causes it to poll longer for the
event, the host takes longer than 100us to generate the event, and if
the event is missed the time when it is checked again is very long.
Polling for 100us is already pretty extreme. 200us is really too long.
The real problem is that there is no interrupt for this. I'd also guess
there is no interrupt on the host side, because that would solve this
problem, too, as it would certainly get around to handling the interupt
in 100us. I'm assuming the host driver is not the Linux driver, as it
should also handle this in a timely manner, even when polling.
If people want hardware to perform well, they ought to design it and not
expect software to fix all the problems.
The right way to fix this is probably to do the same thing the host side
Linux driver does. It has a kernel thread that is kicked off to do
this. Unfortunately, that's more complicated to implement, but it
avoids polling in this location (which causes latency issues on the BMC
side) and lets you poll longer without causing issues.
I'll let the people who maintain that code comment.
-corey
>
> > this results in a server boot taking 50% longer for IBM P10 machines.
> >
> > Started with 1000 and worked it down until the issue started to reoccur.
> > 200 was the sweet spot in my testing. 150 showed the issue
> > intermittently.
>
> I’d add a blank line here.
>
> > Signed-off-by: Andrew Geissler
> > ---
> > drivers/char/ipmi/kcs_bmc_aspeed.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
> > b/drivers/char/ipmi/kcs_bmc_aspeed.c
> > index 72640da55380..af1eae6153f6 100644
> > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> > @@ -422,7 +422,7 @@ static void aspeed_kcs_irq_mask_update(struct
> > kcs_bmc_device *kcs_bmc, u8 mask,
> > * missed the event.
> > */
> > rc = read_poll_timeout_atomic(aspeed_kcs_inb, str,
> > - !(str & KCS_BMC_STR_OBF),
> > 1, 100, false,
> > + !(str & KCS_BMC_STR_OBF),
> > 1, 200, false,
> > >kcs_bmc,
> > priv->kcs_bmc.ioreg.str);
> > /* Time for the slow path? */
> > if (rc == -ETIMEDOUT)
>
>
> Kind regards,
>
> Paul
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes for 6.8
The following changes since commit ceb6a6f023fd3e8b07761ed900352ef574010bcb: Linux 6.7-rc6 (2023-12-17 15:19:28 -0800) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-6.8-1 for you to fetch changes up to 9bd9fbd9032a3b7e9ea916d6e58ba0116e0621be: ipmi: Remove usage of the deprecated ida_simple_xx() API (2023-12-19 06:33:45 -0600) IPMI: Some small fixes Nothing big, just aligning things with some changes. Christophe JAILLET (1): ipmi: Remove usage of the deprecated ida_simple_xx() API Emilio Perez (1): ipmi: Use regspacings passed as a module parameter Rob Herring (1): ipmi: si: Use device_get_match_data() drivers/char/ipmi/ipmi_msghandler.c | 4 ++-- drivers/char/ipmi/ipmi_si_hardcode.c | 2 +- drivers/char/ipmi/ipmi_si_platform.c | 12 3 files changed, 7 insertions(+), 11 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Remove usage of the deprecated ida_simple_xx() API
On Tue, Dec 19, 2023 at 06:00:39AM +0100, Christophe JAILLET wrote:
> ida_alloc() and ida_free() should be preferred to the deprecated
> ida_simple_get() and ida_simple_remove().
>
> This is less verbose.
Thanks, queued for next release.
-corey
>
> Signed-off-by: Christophe JAILLET
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index d6f14279684d..b0eedc4595b3 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -3053,7 +3053,7 @@ static void cleanup_bmc_work(struct work_struct *work)
> int id = bmc->pdev.id; /* Unregister overwrites id */
>
> platform_device_unregister(>pdev);
> - ida_simple_remove(_bmc_ida, id);
> + ida_free(_bmc_ida, id);
> }
>
> static void
> @@ -3169,7 +3169,7 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
>
> bmc->pdev.name = "ipmi_bmc";
>
> - rv = ida_simple_get(_bmc_ida, 0, 0, GFP_KERNEL);
> + rv = ida_alloc(_bmc_ida, GFP_KERNEL);
> if (rv < 0) {
> kfree(bmc);
> goto out;
> --
> 2.34.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Use regspacings passed as a module parameter
On Wed, Nov 22, 2023 at 08:34:28PM +, Emilio Perez wrote:
> regspacings parameter is currently ignored and the platform data uses a
> default value of 0, this has been fixed by setting the appropriate field
> in the platform data.
Yep, queued for next release. Thank you.
-corey
>
> Fixes: 3cd83bac481d ("ipmi: Consolidate the adding of platform devices")
> Signed-off-by: Emilio Perez
> ---
> drivers/char/ipmi/ipmi_si_hardcode.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_hardcode.c
> b/drivers/char/ipmi/ipmi_si_hardcode.c
> index ed5e91b1e040..0c92fa3eee88 100644
> --- a/drivers/char/ipmi/ipmi_si_hardcode.c
> +++ b/drivers/char/ipmi/ipmi_si_hardcode.c
> @@ -80,10 +80,10 @@ static void __init ipmi_hardcode_init_one(const char
> *si_type_str,
> }
>
> p.regsize = regsizes[i];
> + p.regspacing = regspacings[i];
> p.slave_addr = slave_addrs[i];
> p.addr_source = SI_HARDCODED;
> p.regshift = regshifts[i];
> - p.regsize = regsizes[i];
> p.addr = addr;
> p.space = addr_space;
>
> --
> 2.39.3
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [RESEND PATCH] ipmi: si: Use device_get_match_data()
On Wed, Nov 15, 2023 at 03:02:29PM -0600, Rob Herring wrote:
> Use preferred device_get_match_data() instead of of_match_device() to
> get the driver match data. With this, adjust the includes to explicitly
> include the correct headers.
Sorry, this is now queue for 6.8.
-corey
>
> Signed-off-by: Rob Herring
> ---
> drivers/char/ipmi/ipmi_si_platform.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_platform.c
> b/drivers/char/ipmi/ipmi_si_platform.c
> index c3d8ac7873ba..cd2edd8f8a03 100644
> --- a/drivers/char/ipmi/ipmi_si_platform.c
> +++ b/drivers/char/ipmi/ipmi_si_platform.c
> @@ -11,10 +11,11 @@
>
> #include
> #include
> -#include
> -#include
> +#include
> #include
> #include
> +#include
> +#include
> #include
> #include "ipmi_si.h"
> #include "ipmi_dmi.h"
> @@ -224,7 +225,6 @@ MODULE_DEVICE_TABLE(of, of_ipmi_match);
>
> static int of_ipmi_probe(struct platform_device *pdev)
> {
> - const struct of_device_id *match;
> struct si_sm_io io;
> struct resource resource;
> const __be32 *regsize, *regspacing, *regshift;
> @@ -237,10 +237,6 @@ static int of_ipmi_probe(struct platform_device *pdev)
>
> dev_info(>dev, "probing via device tree\n");
>
> - match = of_match_device(of_ipmi_match, >dev);
> - if (!match)
> - return -ENODEV;
> -
> if (!of_device_is_available(np))
> return -EINVAL;
>
> @@ -269,7 +265,7 @@ static int of_ipmi_probe(struct platform_device *pdev)
> }
>
> memset(, 0, sizeof(io));
> - io.si_type = (unsigned long) match->data;
> + io.si_type = (enum si_type)device_get_match_data(>dev);
> io.addr_source = SI_DEVICETREE;
> io.irq_setup= ipmi_std_irq_setup;
>
> --
> 2.42.0
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] SOL via syslog?
On Thu, Nov 09, 2023 at 04:41:00PM +0100, Christian Theune wrote: > > > On 9. Nov 2023, at 14:50, Corey Minyard wrote: > > > > It's 09c5ba0aa2fc "printk: add kthread console printers" and some > > others. It's in 5.19, so it was later than I thought. > > Interesting. I can see it merged but then also reverted AFAICT before the > 5.19 release. Oh, yeah. Getting this in has been a fight, it's been going on a long time. Printk ties into so many things. > > I saw a lot of work around kprint() happened in and after 5.15 so I guess > upgrading to 5.15 shouldn’t hurt in any case. > > Not having a reproducer is the real bummer. > > I was also wondering whether using other utilities like storing kernel crash > dumps on swap would be a good avenue. The only issue here being that this is > a KVM/Qemu host with lots of RAM and I think I don’t have enough disk space > to capture full system memory dumps … You might be surprised, it is probably smaller than you think. > > > I'm not being a lot of help here, but hopefully it can lead you > > somewhere you can figure this out. These can be hard problems to > > track down. > > I guess we’re both pretty blind here. I appreciate any assistance. :) > > > I don't remember, had you done anything with the kernel preempt tracing? > > That can be useful for tracking down long preempt off times. > > Uhm no, not yet. Any pointers where to start and how this relates to > potential kprint buffers? Actually, I'm confusing this with another issue dealing with real time latencies and printk. Preempt tracing won't help your issue. A assume you are using the "normal" NMI watchdog and it's not triggerring. It should be on by default. You can look in /proc/sys/kenel/nmi_watchdog to see. I was working with a customer of our companies on something similar, a watchdog reset with not discernable reason. They couldn't use the standard NMI watchdog, so we switched to using an NMI watch from the BMC. Set the preaction to nmi and the preop to panic. With that, you can take a kernel coredump. You generally only take a coredump of kernel memory (without buffers), so it's not generally a huge amount of disk space, and it's compressed. Of course, then you have to analyze a coredump, which has its own difficulties :-(. -corey ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] SOL via syslog?
On Thu, Nov 09, 2023 at 10:14:22AM +0100, Christian Theune via Openipmi-developer wrote: > Hi, > > > > > On 3. Oct 2023, at 07:13, Corey Minyard wrote: > > > > Yeah, I understand how this would be a strange scenario. I have seen > > this happen in the real world, so it is something that's possible, but I > > think the printk changes went in before 5.10. > > > > Maybe a firmware update to the BMC? I think you would have mentioned > > that, though. > > do you know how to reproduce this error? I’ve disabled SOL logging on one of > the affected machines (it might be related to the specific BMC and I’m > considering BMC firmware updates) but I’ve spammed the kmsg that are sent to > the SOL for 12+ hours in a tight bash loop without an SOL attached and did > not trigger the issue… I believe you have to call printk from some sort of atomic context, like an interrupt or with preemption disabled. Then, on an SMP system, it would have to somehow block the running of the thread of execution you care about. > > Next to the BMC firmware update I’m also considering switching from 5.10 to > 5.15 (we’re having issues in 6.1 at the moment so I don’t want to go there > right now) but I’d love if I could construct a reproducer … > > Unfortunately the BMC firmwares do not show changelogs and I’m a bit wary of > thinking that a BMC issue would be the culprit here … -_- I'd agree, it doesn't really seem so, and even if it is, it doesn't really matter. > > I also didn’t find the original commit that you mentioned would be fixing it > … a hint for what to search for in the changelogs would be much appreciated! It's 09c5ba0aa2fc "printk: add kthread console printers" and some others. It's in 5.19, so it was later than I thought. I'm not being a lot of help here, but hopefully it can lead you somewhere you can figure this out. These can be hard problems to track down. I don't remember, had you done anything with the kernel preempt tracing? That can be useful for tracking down long preempt off times. -corey > > Kind regards, > Christian ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI fixes for 6.7
The following changes since commit 3669558bdf354cd352be955ef2764cde6a9bf5ec: Merge tag 'for-6.6-rc1-tag' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux (2023-09-12 11:28:00 -0700) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-6.7-1 for you to fetch changes up to b00839ca4cca8aa9641c121c848a553d6220ce70: ipmi: refactor deprecated strncpy (2023-09-13 12:55:11 -0500) Pull request for IPMI Well, only one change, and I would normally just wait, but it will make the people trying to get rid of strncpy happy. Its a good change, anyway. Justin Stitt (1): ipmi: refactor deprecated strncpy drivers/char/ipmi/ipmi_msghandler.c | 11 +++ drivers/char/ipmi/ipmi_ssif.c | 2 +- 2 files changed, 4 insertions(+), 9 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] SOL via syslog?
On Tue, Oct 03, 2023 at 06:47:49AM +0200, Christian Theune wrote: > Hey, > > > On 2. Oct 2023, at 17:08, Corey Minyard wrote: > > > > On Mon, Oct 02, 2023 at 08:05:09AM +0200, Christian Theune wrote: > > > > ...snip... > > > >>>> Can you not get kernel coredumps? > >>> Unfortunately no and I still have absolutely now idea why the watchdog > >>> triggers… I have currently attached dozens of servers that are part of a > >>> mysterious series of crashes but they didn’t crash after I attached the > >>> SOL continuously. Just my kind of luck I guess … ;) > >>> > >>> It might be a clue. Can you make sure flow-control is turned off on the > >>> SOL connection and console? If you have "r" on the console= command > >>> (like console=115200n81r) , if the BMC stops taking characters you can > >>> hang the kernel. > >>> > >>> You might want to make sure getty has RTS turned off, too. > >>> > >>> The trouble is, of course, that you can lose characters because of a slow > >>> BMC. But it's generally a bad idea to run a console with flow control > >>> enabled. > >> > >> Sorry, that might have been a misunderstanding: I’m not catching the > >> crashes currently because all the machines that used to crash now seem to > >> not want to crash anymore. I guess we’re on a Heisenbug here. Getting > >> output from the SOL works absolutely fine, so I expect to see a kernel > >> crash in the SOL once it happens. > >> > >> I’m somewhat suspecting that we’ll find another bug that causes those > >> specific crashes not appear in the SEL, though … > >> > >> And then again: maybe it’s not a Heisenbug, but maybe whatever caused the > >> crashes has been fixed in between and I’ll never know … ;) > >> > > > > I understood. I'm saying that maybe the machines aren't crashing any > > more *because* you are monitoring them with SOL. > > Oooh. I’m glad we took this detour - I knew something was off, but I was > the one misunderstanding. Thanks for taking the time to explain it again! I > was a bit stuck on the “well it’s a Heisenbug then” but didn’t get that it > was literally so… > > > Perhaps a lot of kernel output comes out all at once, it gets flow > > controlled by the BMC, the kernel hangs waiting for printk output, and > > the watchdog then goes off. Newer kernels have fixes to avoid this > > problem, but older ones don't. > > > > There would be no OS crash, no SEL output, no coredump, just a watchdog > > reboot. > > Understood. What would be a newer kernel? We’re running 5.10(.190+) at the > moment. > > The interesting part here is that we have been logging to the serial console > without anything attached normally > for a long long time (think: 10 years plus) so there is still a bit of doubt > as this started to creep up only recently. Yeah, I understand how this would be a strange scenario. I have seen this happen in the real world, so it is something that's possible, but I think the printk changes went in before 5.10. Maybe a firmware update to the BMC? I think you would have mentioned that, though. Anyway, the only way to know for sure would be to turn off the SOL monitoring and see if it re-occurs. I can understand why you wouldn't want to do that :). -corey > > > If you turn off the SOL monitoring and the problem comes back, that > > would be a pretty good indication that something like that is happening. > > Unfortunately, it's hard to debug because you can't get info from your > > primary debugging interface. > > Yeah. That’s something I’ll discuss with my team. I originally intended to > turn off the continuous SOL monitoring but after this goose chase I’m > somewhat willing to make it a regular thing. > > > Of course, the bug may have been fixed by a kernel or app upgrade, too. > > Like you say with things like this, you may never know :). > > Kernel would be the most obvious choice for us as the affected hosts are > really only Qemu/KVM servers that didn’t see any relevant updates in the > userland in the past months. > > Thanks again, > Christian > > -- > Christian Theune · c...@flyingcircus.io · +49 345 219401 0 > Flying Circus Internet Operations GmbH · https://flyingcircus.io > Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland > HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian Zagrodnick > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] SOL via syslog?
On Mon, Oct 02, 2023 at 08:05:09AM +0200, Christian Theune wrote: ...snip... > > > Can you not get kernel coredumps? > > Unfortunately no and I still have absolutely now idea why the watchdog > > triggers… I have currently attached dozens of servers that are part of a > > mysterious series of crashes but they didn’t crash after I attached the SOL > > continuously. Just my kind of luck I guess … ;) > > > > It might be a clue. Can you make sure flow-control is turned off on the > > SOL connection and console? If you have "r" on the console= command (like > > console=115200n81r) , if the BMC stops taking characters you can hang the > > kernel. > > > > You might want to make sure getty has RTS turned off, too. > > > > The trouble is, of course, that you can lose characters because of a slow > > BMC. But it's generally a bad idea to run a console with flow control > > enabled. > > Sorry, that might have been a misunderstanding: I’m not catching the crashes > currently because all the machines that used to crash now seem to not want to > crash anymore. I guess we’re on a Heisenbug here. Getting output from the SOL > works absolutely fine, so I expect to see a kernel crash in the SOL once it > happens. > > I’m somewhat suspecting that we’ll find another bug that causes those > specific crashes not appear in the SEL, though … > > And then again: maybe it’s not a Heisenbug, but maybe whatever caused the > crashes has been fixed in between and I’ll never know … ;) > I understood. I'm saying that maybe the machines aren't crashing any more *because* you are monitoring them with SOL. Perhaps a lot of kernel output comes out all at once, it gets flow controlled by the BMC, the kernel hangs waiting for printk output, and the watchdog then goes off. Newer kernels have fixes to avoid this problem, but older ones don't. There would be no OS crash, no SEL output, no coredump, just a watchdog reboot. If you turn off the SOL monitoring and the problem comes back, that would be a pretty good indication that something like that is happening. Unfortunately, it's hard to debug because you can't get info from your primary debugging interface. Of course, the bug may have been fixed by a kernel or app upgrade, too. Like you say with things like this, you may never know :). -corey ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] SOL via syslog?
On Oct 1, 2023 11:14 AM, Christian Theune via Openipmi-developer wrote:Hi, > On 1. Oct 2023, at 03:49, Corey Minyard wrote: > > On Sat, Sep 30, 2023 at 11:14:01PM +0200, Christian Theune via Openipmi-developer wrote: >> Hi, >> >> sorry if this isn’t directly a developers question, but I’ve run out of avenues after googling and looking around… >> >> We’re experiencing weird system stability issue where the “log to SEL” doesn’t cut it: we see watchdog reboots but no kernel output whatsoever ending up in the SEL. (I’ve debugged this with Corey before and we found something to fix but the watchdog events we’re experiencing still don’t get logged in more detail.) > > Can you not get kernel coredumps? Unfortunately no and I still have absolutely now idea why the watchdog triggers… I have currently attached dozens of servers that are part of a mysterious series of crashes but they didn’t crash after I attached the SOL continuously. Just my kind of luck I guess … ;)It might be a clue. Can you make sure flow-control is turned off on the SOL connection and console? If you have "r" on the console= command (like console=115200n81r) , if the BMC stops taking characters you can hang the kernel.You might want to make sure getty has RTS turned off, too.The trouble is, of course, that you can lose characters because of a slow BMC. But it's generally a bad idea to run a console with flow control enabled. As we’re continuously updating our environment it might also be that we’ve successfully evaded a kernel bug that was haunting us … maybe … ;) >> >> I’m wondering: does anyone know of a “push” solution to instruct the BMC (mostly Supermicro in our case) to push SOL output proactively through some protocol like syslog? > > The SEL probably isn't big or fast enough to take system logs. You > could create something like this as part of printk, but I suspect that > it would quickly overflow the SEL. Yeah, I wasn’t thinking about the SEL but wondering whether serial output could be shipped in a push-manner from the BMC without having to attach and authenticate.That would take some work in the BMC. >> Otherwise we’d need to set up a central host with passwords for dozens of hosts to pull the SOL for logging and that doesn’t feel right either … -__ > > I know people that do this; it's not terrible. You do have all of your > IPMI passwords in one place, that's the biggest issue, but IMHO you > should be monitoring the output of your consoles, anyway. Yeah, that’s what I’m pondering, too. IMHO it’s quite a bit terrible and thus I was wondering whether the BMC might have a built-in solution that would turn this upside down … but I gess not > I support a program called ser2net that is capable of making SOL > connections, logging the output, and allowing connections to the > console. That would be a pretty complicated setup, but I can help you > with it, if you like. The multiplexing sounds great. I’ve built a small shell wrapper to manage SOL connections and their logging (and reconnecting if the BMC acts up) which works for now. From a design perspective I’d really love this to be push-based. I researched the dmtf site, but didn’t find anything there either … I guess I’m the odd-one out then …No idea. So with your little wrapper connected everything seems to work ok.Outside of the flow control thing, I have no idea.-corey Christian -- Christian Theune · c...@flyingcircus.io · +49 345 219401 0 Flying Circus Internet Operations GmbH · https://flyingcircus.io Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian Zagrodnick ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] SOL via syslog?
On Sat, Sep 30, 2023 at 11:14:01PM +0200, Christian Theune via Openipmi-developer wrote: > Hi, > > sorry if this isn’t directly a developers question, but I’ve run out of > avenues after googling and looking around… > > We’re experiencing weird system stability issue where the “log to SEL” > doesn’t cut it: we see watchdog reboots but no kernel output whatsoever > ending up in the SEL. (I’ve debugged this with Corey before and we found > something to fix but the watchdog events we’re experiencing still don’t get > logged in more detail.) Can you not get kernel coredumps? > > I’m wondering: does anyone know of a “push” solution to instruct the BMC > (mostly Supermicro in our case) to push SOL output proactively through some > protocol like syslog? The SEL probably isn't big or fast enough to take system logs. You could create something like this as part of printk, but I suspect that it would quickly overflow the SEL. > > Otherwise we’d need to set up a central host with passwords for dozens of > hosts to pull the SOL for logging and that doesn’t feel right either … -__ I know people that do this; it's not terrible. You do have all of your IPMI passwords in one place, that's the biggest issue, but IMHO you should be monitoring the output of your consoles, anyway. I support a program called ser2net that is capable of making SOL connections, logging the output, and allowing connections to the console. That would be a pretty complicated setup, but I can help you with it, if you like. -corey > > Grateful for any ideas … > Christian > > -- > Christian Theune · c...@flyingcircus.io · +49 345 219401 0 > Flying Circus Internet Operations GmbH · https://flyingcircus.io > Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland > HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian Zagrodnick > > > > ___ > Openipmi-developer mailing list > Openipmi-developer@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openipmi-developer ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2] ipmi: refactor deprecated strncpy
On Wed, Sep 13, 2023 at 05:13:04PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
Thanks, applied to my next tree.
-corey
>
> In this case, strncpy is being used specifically for its NUL-padding
> behavior (and has been commented as such). Moreover, the destination
> string is not required to be NUL-terminated [2].
>
> We can use a more robust and less ambiguous interface in
> `memcpy_and_pad` which makes the code more readable and even eliminates
> the need for that comment.
>
> Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> check as this is intrinsically a part of `strnlen`.
>
> Also included in this patch is a simple 1:1 change of `strncpy` to
> `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> should opt again for `strscpy_pad`.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://lore.kernel.org/all/zqeadybl0uz1n...@mail.minyard.net/ [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> Changes in v2:
> - use memcpy_and_pad (thanks Corey)
> - Link to v1:
> https://lore.kernel.org/r/20230912-strncpy-drivers-char-ipmi-ipmi-v1-1-cc43e0d1c...@google.com
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> drivers/char/ipmi/ipmi_ssif.c | 2 +-
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 186f1fee7534..d6f14279684d 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf,
> char *str)
>
> j = 0;
> while (*p) {
> - int size = strlen(p);
> + int size = strnlen(p, 11);
>
> - if (size > 11)
> - size = 11;
> data[0] = 0;
> data[1] = 0;
> data[2] = 0xf0; /* OEM event without timestamp. */
> data[3] = intf->addrinfo[0].address;
> data[4] = j++; /* sequence # */
> - /*
> - * Always give 11 bytes, so strncpy will fill
> - * it with zeroes for me.
> - */
> - strncpy(data+5, p, 11);
> +
> + memcpy_and_pad(data+5, 11, p, size, '\0');
> p += size;
>
> ipmi_panic_request_and_wait(intf, , );
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 3b921c78ba08..edcb83765dce 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
> }
> }
>
> - strncpy(addr_info->binfo.type, DEVICE_NAME,
> + strscpy(addr_info->binfo.type, DEVICE_NAME,
> sizeof(addr_info->binfo.type));
> addr_info->binfo.addr = addr;
> addr_info->binfo.platform_data = addr_info;
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
>
> Best regards,
> --
> Justin Stitt
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 05:55:02PM -0700, Justin Stitt wrote:
> On Tue, Sep 12, 2023 at 5:19 PM Corey Minyard wrote:
> >
> > On Tue, Sep 12, 2023 at 11:43:05PM +, Justin Stitt wrote:
> > > `strncpy` is deprecated for use on NUL-terminated destination strings [1].
> > >
> > > In this case, strncpy is being used specifically for its NUL-padding
> > > behavior (and has been commented as such). We can use a more robust and
> > > less ambiguous interface in `strscpy_pad` which makes the code more
> > > readable and even eliminates the need for that comment.
> > >
> > > Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> > > check as this is intrinsically a part of `strnlen`.
> > >
> > > Also included in this patch is a simple 1:1 change of `strncpy` to
> > > `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> > > should opt again for `strscpy_pad`.
> > >
> > > Link:
> > > https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> > > [1]
> > > Link: https://github.com/KSPP/linux/issues/90
> > > Cc: linux-harden...@vger.kernel.org
> > > Cc: Kees Cook
> > > Signed-off-by: Justin Stitt
> > > ---
> > > drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> > > drivers/char/ipmi/ipmi_ssif.c | 2 +-
> > > 2 files changed, 4 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > > b/drivers/char/ipmi/ipmi_msghandler.c
> > > index 186f1fee7534..04f7622cb703 100644
> > > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > > @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi
> > > *intf, char *str)
> > >
> > > j = 0;
> > > while (*p) {
> > > - int size = strlen(p);
> > > + int size = strnlen(p, 11);
> > >
> > > - if (size > 11)
> > > - size = 11;
> > > data[0] = 0;
> > > data[1] = 0;
> > > data[2] = 0xf0; /* OEM event without timestamp. */
> > > data[3] = intf->addrinfo[0].address;
> > > data[4] = j++; /* sequence # */
> > > - /*
> > > - * Always give 11 bytes, so strncpy will fill
> > > - * it with zeroes for me.
> > > - */
> > > - strncpy(data+5, p, 11);
> > > +
> > > + strscpy_pad(data+5, p, 11);
> >
> > This is incorrect, the destination should *not* be nil terminated if the
> > destination is full. strncpy does exactly what is needed here.
>
> Could we use `memcpy_and_pad()` as this matches the behavior of
> strncpy in this case? I understand strncpy works here but I'm really
> keen on snuffing out all its uses -- treewide.
Sure, I think "memcpy_and_pad(data + 5, 11, p, size, 0);" should work.
And that's self-documenting.
-corey
>
> >
> > A comment should be added here, this is not the first time this has been
> > brought up.
> >
> > > p += size;
> > >
> > > ipmi_panic_request_and_wait(intf, , );
> > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > > index 3b921c78ba08..edcb83765dce 100644
> > > --- a/drivers/char/ipmi/ipmi_ssif.c
> > > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > > @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char
> > > *adapter_name,
> > > }
> > > }
> > >
> > > - strncpy(addr_info->binfo.type, DEVICE_NAME,
> > > + strscpy(addr_info->binfo.type, DEVICE_NAME,
> > > sizeof(addr_info->binfo.type));
> >
> > This one is good.
> >
> > -corey
> >
> > > addr_info->binfo.addr = addr;
> > > addr_info->binfo.platform_data = addr_info;
> > >
> > > ---
> > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> > > change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
> > >
> > > Best regards,
> > > --
> > > Justin Stitt
> > >
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: refactor deprecated strncpy
On Tue, Sep 12, 2023 at 11:43:05PM +, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1].
>
> In this case, strncpy is being used specifically for its NUL-padding
> behavior (and has been commented as such). We can use a more robust and
> less ambiguous interface in `strscpy_pad` which makes the code more
> readable and even eliminates the need for that comment.
>
> Let's also use `strnlen` instead of `strlen()` with an upper-bounds
> check as this is intrinsically a part of `strnlen`.
>
> Also included in this patch is a simple 1:1 change of `strncpy` to
> `strscpy` for ipmi_ssif.c. If NUL-padding is wanted here as well then we
> should opt again for `strscpy_pad`.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Cc: Kees Cook
> Signed-off-by: Justin Stitt
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 11 +++
> drivers/char/ipmi/ipmi_ssif.c | 2 +-
> 2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 186f1fee7534..04f7622cb703 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -5377,20 +5377,15 @@ static void send_panic_events(struct ipmi_smi *intf,
> char *str)
>
> j = 0;
> while (*p) {
> - int size = strlen(p);
> + int size = strnlen(p, 11);
>
> - if (size > 11)
> - size = 11;
> data[0] = 0;
> data[1] = 0;
> data[2] = 0xf0; /* OEM event without timestamp. */
> data[3] = intf->addrinfo[0].address;
> data[4] = j++; /* sequence # */
> - /*
> - * Always give 11 bytes, so strncpy will fill
> - * it with zeroes for me.
> - */
> - strncpy(data+5, p, 11);
> +
> + strscpy_pad(data+5, p, 11);
This is incorrect, the destination should *not* be nil terminated if the
destination is full. strncpy does exactly what is needed here.
A comment should be added here, this is not the first time this has been
brought up.
> p += size;
>
> ipmi_panic_request_and_wait(intf, , );
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 3b921c78ba08..edcb83765dce 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1940,7 +1940,7 @@ static int new_ssif_client(int addr, char *adapter_name,
> }
> }
>
> - strncpy(addr_info->binfo.type, DEVICE_NAME,
> + strscpy(addr_info->binfo.type, DEVICE_NAME,
> sizeof(addr_info->binfo.type));
This one is good.
-corey
> addr_info->binfo.addr = addr;
> addr_info->binfo.platform_data = addr_info;
>
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230912-strncpy-drivers-char-ipmi-ipmi-dda47b3773fd
>
> Best regards,
> --
> Justin Stitt
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] OpenIPMI 2.0.34 Released
It's been over a year since 2.0.33. Not much has changed, really, a few small things for improved usability. This has some required changes for gensio testing of ipmi SOL. Probably worth upgrading if you can. ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes for 6.6
The following changes since commit 4d6d4c7f541d7027beed4fb86eb2c451bd8d6fff: Merge tag 'linux-kselftest-fixes-6.4-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest (2023-05-17 11:16:36 -0700) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-6.6-1 for you to fetch changes up to d40f09c1a23024f0e550d9423f4d389672e1dfaf: ipmi_si: fix -Wvoid-pointer-to-enum-cast warning (2023-08-15 15:46:06 -0500) Minor fixes for IPMI Lots of small unconnected things, memory leaks on error, a possible (though unlikely) deadlock, changes for updates to other things that have changed. Nothing earth-shattering, but things that need update. Chengfeng Ye (1): ipmi: fix potential deadlock on _bmc->lock Corey Minyard (3): ipmi_watchdog: Fix read syscall not responding to signals during sleep ipmi:ssif: Fix a memory leak when scanning for an adapter ipmi: Change request_module to request_module_nowait Ivan Orlov (1): ipmi: make ipmi_class a static const structure Jiasheng Jiang (1): ipmi:ssif: Add check for kstrdup Justin Stitt (1): ipmi_si: fix -Wvoid-pointer-to-enum-cast warning Krzysztof Kozlowski (1): dt-bindings: ipmi: aspeed,ast2400-kcs-bmc: drop unneeded quotes Uwe Kleine-König (1): ipmi: Switch i2c drivers back to use .probe() Yi Yang (1): ipmi_si: fix a memleak in try_smi_init() .../bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 8 drivers/char/ipmi/ipmb_dev_int.c | 2 +- drivers/char/ipmi/ipmi_devintf.c | 24 +++--- drivers/char/ipmi/ipmi_ipmb.c | 2 +- drivers/char/ipmi/ipmi_si_intf.c | 5 + drivers/char/ipmi/ipmi_si_platform.c | 4 ++-- drivers/char/ipmi/ipmi_ssif.c | 11 +++--- drivers/char/ipmi/ipmi_watchdog.c | 2 +- drivers/char/ipmi/kcs_bmc.c| 5 +++-- drivers/char/ipmi/ssif_bmc.c | 2 +- 10 files changed, 38 insertions(+), 27 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi_si: fix -Wvoid-pointer-to-enum-cast warning
On Wed, Aug 09, 2023 at 09:05:17PM +, Justin Stitt wrote: > With W=1 we see the following warning: > > | drivers/char/ipmi/ipmi_si_platform.c:272:15: error: \ > | cast to smaller integer type 'enum si_type' from \ > | 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast] > |272 | io.si_type = (enum si_type) match->data; > || ^~ Ok, this is included in my tree. Thanks. -corey > > This is due to the fact that the `si_type` enum members are int-width > and a cast from pointer-width down to int will cause truncation and > possible data loss. Although in this case `si_type` has only a few > enumerated fields and thus there is likely no data loss occurring. > Nonetheless, this patch is necessary to the goal of promoting this > warning out of W=1. > > Link: https://github.com/ClangBuiltLinux/linux/issues/1902 > Link: https://lore.kernel.org/llvm/202308081000.ttl1eltr-...@intel.com/ > Reported-by: kernel test robot > Signed-off-by: Justin Stitt > --- > Note: > Arnd had mentioned that there perhaps may be some semantic differences > between GCC and Clang regarding this warning or family of warnings. For > now, this patch (and others following) will yield less noisy W=1 builds > and hopefully materialize into this warning getting promoted out of W=1 > to an always-on warning. > --- > drivers/char/ipmi/ipmi_si_platform.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/ipmi/ipmi_si_platform.c > b/drivers/char/ipmi/ipmi_si_platform.c > index 505cc978c97a..0d509d683c0f 100644 > --- a/drivers/char/ipmi/ipmi_si_platform.c > +++ b/drivers/char/ipmi/ipmi_si_platform.c > @@ -269,7 +269,7 @@ static int of_ipmi_probe(struct platform_device *pdev) > } > > memset(, 0, sizeof(io)); > - io.si_type = (enum si_type) match->data; > + io.si_type = (unsigned long) match->data; > io.addr_source = SI_DEVICETREE; > io.irq_setup= ipmi_std_irq_setup; > > > --- > base-commit: c1a515d3c0270628df8ae5f5118ba859b85464a2 > change-id: 20230809-cbl-1902-7532a747b731 > > Best regards, > -- > Justin Stitt > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi_si: fix -Wvoid-pointer-to-enum-cast warning
On Wed, Aug 09, 2023 at 09:05:17PM +, Justin Stitt wrote: > With W=1 we see the following warning: > > | drivers/char/ipmi/ipmi_si_platform.c:272:15: error: \ > | cast to smaller integer type 'enum si_type' from \ > | 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast] > |272 | io.si_type = (enum si_type) match->data; > || ^~ > > This is due to the fact that the `si_type` enum members are int-width > and a cast from pointer-width down to int will cause truncation and > possible data loss. Although in this case `si_type` has only a few > enumerated fields and thus there is likely no data loss occurring. > Nonetheless, this patch is necessary to the goal of promoting this > warning out of W=1. > > Link: https://github.com/ClangBuiltLinux/linux/issues/1902 > Link: https://lore.kernel.org/llvm/202308081000.ttl1eltr-...@intel.com/ > Reported-by: kernel test robot > Signed-off-by: Justin Stitt > --- > Note: > Arnd had mentioned that there perhaps may be some semantic differences > between GCC and Clang regarding this warning or family of warnings. For > now, this patch (and others following) will yield less noisy W=1 builds > and hopefully materialize into this warning getting promoted out of W=1 > to an always-on warning. > --- > drivers/char/ipmi/ipmi_si_platform.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/ipmi/ipmi_si_platform.c > b/drivers/char/ipmi/ipmi_si_platform.c > index 505cc978c97a..0d509d683c0f 100644 > --- a/drivers/char/ipmi/ipmi_si_platform.c > +++ b/drivers/char/ipmi/ipmi_si_platform.c > @@ -269,7 +269,7 @@ static int of_ipmi_probe(struct platform_device *pdev) > } > > memset(, 0, sizeof(io)); > - io.si_type = (enum si_type) match->data; > + io.si_type = (unsigned long) match->data; Wouldn't you want to use intptr_t or uintptr_t? -corey > io.addr_source = SI_DEVICETREE; > io.irq_setup= ipmi_std_irq_setup; > > > --- > base-commit: c1a515d3c0270628df8ae5f5118ba859b85464a2 > change-id: 20230809-cbl-1902-7532a747b731 > > Best regards, > -- > Justin Stitt > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: fix potential deadlock on _bmc->lock
On Fri, Jun 30, 2023 at 10:31:02AM +0930, Andrew Jeffery wrote:
> Hi Corey, Chengfeng,
>
> On Wed, 28 Jun 2023, at 21:17, Corey Minyard wrote:
> > Indeed, this looks like an issue.
> >
> > Andrew, any opinions on this? The attached patch will work, the other
> > option would be to disable interrupts when calling
> > kcs_bmc_handle_event() in the timer handler. But then you have to worry
> > about RT.
>
> Right, I think we'd do best to not over-complicate things.
> spin_lock_irq{save,restore}() are the intuitive choice to me.
>
> I'll follow up with R-b/T-b tags once I've booted the patch
> and done some testing.
Thanks. This is in my for-next tree, I'd like to get this in the merge
window, which I believe ends this Sunday.
> >> This flaw was found using an experimental static analysis tool we are
> >> developing for irq-related deadlock.
Will this tool be available for general use? It's obviously quite
handy.
-corey
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi_si: fix a memleak in try_smi_init()
On Thu, Jun 29, 2023 at 08:33:28PM +0800, GONG, Ruiqi wrote: > From: Yi Yang > > Kmemleak reported the following leak info in try_smi_init(): > > unreferenced object 0x00018ecf9400 (size 1024): > comm "modprobe", pid 2707763, jiffies 4300851415 (age 773.308s) > backtrace: > [<4ca5b312>] __kmalloc+0x4b8/0x7b0 > [<953b1072>] try_smi_init+0x148/0x5dc [ipmi_si] > [<6460d325>] 0x800081b10148 > [<39206ea5>] do_one_initcall+0x64/0x2a4 > [<601399ce>] do_init_module+0x50/0x300 > [<3c12ba3c>] load_module+0x7a8/0x9e0 > [] __se_sys_init_module+0x104/0x180 > [ ] __arm64_sys_init_module+0x24/0x30 > [<21b1ef87>] el0_svc_common.constprop.0+0x94/0x250 > [<70f4f8b7>] do_el0_svc+0x48/0xe0 > [<5a05337f>] el0_svc+0x24/0x3c > [<5eb248d6>] el0_sync_handler+0x160/0x164 > [<30a59039>] el0_sync+0x160/0x180 > > The problem was that when an error occurred before handlers registration > and after allocating `new_smi->si_sm`, the variable wouldn't be freed in > the error handling afterwards since `shutdown_smi()` hadn't been > registered yet. Fix it by adding a `kfree()` in the error handling path > in `try_smi_init()`. Thanks, I have included this. And thanks for handling the stable thing, too. -corey > > Cc: sta...@vger.kernel.org # 4.19+ > Fixes: 7960f18a5647 ("ipmi_si: Convert over to a shutdown handler") > Signed-off-by: Yi Yang > Co-developed-by: GONG, Ruiqi > Signed-off-by: GONG, Ruiqi > --- > drivers/char/ipmi/ipmi_si_intf.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c > b/drivers/char/ipmi/ipmi_si_intf.c > index abddd7e43a9a..5cd031f3fc97 100644 > --- a/drivers/char/ipmi/ipmi_si_intf.c > +++ b/drivers/char/ipmi/ipmi_si_intf.c > @@ -2082,6 +2082,11 @@ static int try_smi_init(struct smi_info *new_smi) > new_smi->io.io_cleanup = NULL; > } > > + if (rv && new_smi->si_sm) { > + kfree(new_smi->si_sm); > + new_smi->si_sm = NULL; > + } > + > return rv; > } > > -- > 2.25.1 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: fix potential deadlock on _bmc->lock
Indeed, this looks like an issue.
Andrew, any opinions on this? The attached patch will work, the other
option would be to disable interrupts when calling
kcs_bmc_handle_event() in the timer handler. But then you have to worry
about RT.
-corey
On Tue, Jun 27, 2023 at 03:24:49PM +, Chengfeng Ye wrote:
> As kcs_bmc_handle_event() is executed inside both a timer and a hardirq,
> it should disable irq before lock acquisition otherwise deadlock could
> happen if the timmer is preemtped by the irq.
>
> Possible deadlock scenario:
> aspeed_kcs_check_obe() (timer)
> -> kcs_bmc_handle_event()
> -> spin_lock(_bmc->lock)
>
> -> aspeed_kcs_irq()
> -> kcs_bmc_handle_event()
> -> spin_lock(_bmc->lock) (deadlock here)
>
> This flaw was found using an experimental static analysis tool we are
> developing for irq-related deadlock.
>
> The tentative patch fix the potential deadlock by spin_lock_irqsave()
>
> Signed-off-by: Chengfeng Ye
> ---
> drivers/char/ipmi/kcs_bmc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c
> index 03d02a848f3a..8b1161d5194a 100644
> --- a/drivers/char/ipmi/kcs_bmc.c
> +++ b/drivers/char/ipmi/kcs_bmc.c
> @@ -56,12 +56,13 @@ irqreturn_t kcs_bmc_handle_event(struct kcs_bmc_device
> *kcs_bmc)
> {
> struct kcs_bmc_client *client;
> irqreturn_t rc = IRQ_NONE;
> + unsigned long flags;
>
> - spin_lock(_bmc->lock);
> + spin_lock_irqsave(_bmc->lock, flags);
> client = kcs_bmc->client;
> if (client)
> rc = client->ops->event(client);
> - spin_unlock(_bmc->lock);
> + spin_unlock_irqrestore(_bmc->lock, flags);
>
> return rc;
> }
> --
> 2.17.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: make ipmi_class a static const structure
On Tue, Jun 20, 2023 at 04:37:02PM +0200, Greg Kroah-Hartman wrote:
> From: Ivan Orlov
>
> Now that the driver core allows for struct class to be in read-only
> memory, move the ipmi_class structure to be declared at build time
> placing it into read-only memory, instead of having to be dynamically
> allocated at boot time.
This is in my next tree and seems to work fine. Thanks.
-corey
>
> Cc: Corey Minyard
> Cc: openipmi-developer@lists.sourceforge.net
> Suggested-by: Greg Kroah-Hartman
> Signed-off-by: Ivan Orlov
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/char/ipmi/ipmi_devintf.c | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_devintf.c
> b/drivers/char/ipmi/ipmi_devintf.c
> index 73e5a9e28f85..332082e02ea5 100644
> --- a/drivers/char/ipmi/ipmi_devintf.c
> +++ b/drivers/char/ipmi/ipmi_devintf.c
> @@ -807,7 +807,9 @@ struct ipmi_reg_list {
> static LIST_HEAD(reg_list);
> static DEFINE_MUTEX(reg_list_mutex);
>
> -static struct class *ipmi_class;
> +static const struct class ipmi_class = {
> + .name = "ipmi",
> +};
>
> static void ipmi_new_smi(int if_num, struct device *device)
> {
> @@ -822,7 +824,7 @@ static void ipmi_new_smi(int if_num, struct device
> *device)
> entry->dev = dev;
>
> mutex_lock(_list_mutex);
> - device_create(ipmi_class, device, dev, NULL, "ipmi%d", if_num);
> + device_create(_class, device, dev, NULL, "ipmi%d", if_num);
> list_add(>link, _list);
> mutex_unlock(_list_mutex);
> }
> @@ -840,7 +842,7 @@ static void ipmi_smi_gone(int if_num)
> break;
> }
> }
> - device_destroy(ipmi_class, dev);
> + device_destroy(_class, dev);
> mutex_unlock(_list_mutex);
> }
>
> @@ -860,15 +862,13 @@ static int __init init_ipmi_devintf(void)
>
> pr_info("ipmi device interface\n");
>
> - ipmi_class = class_create("ipmi");
> - if (IS_ERR(ipmi_class)) {
> - pr_err("ipmi: can't register device class\n");
> - return PTR_ERR(ipmi_class);
> - }
> + rv = class_register(_class);
> + if (rv)
> + return rv;
>
> rv = register_chrdev(ipmi_major, DEVICE_NAME, _fops);
> if (rv < 0) {
> - class_destroy(ipmi_class);
> + class_unregister(_class);
> pr_err("ipmi: can't get major %d\n", ipmi_major);
> return rv;
> }
> @@ -880,7 +880,7 @@ static int __init init_ipmi_devintf(void)
> rv = ipmi_smi_watcher_register(_watcher);
> if (rv) {
> unregister_chrdev(ipmi_major, DEVICE_NAME);
> - class_destroy(ipmi_class);
> + class_unregister(_class);
> pr_warn("ipmi: can't register smi watcher\n");
> return rv;
> }
> @@ -895,11 +895,11 @@ static void __exit cleanup_ipmi(void)
> mutex_lock(_list_mutex);
> list_for_each_entry_safe(entry, entry2, _list, link) {
> list_del(>link);
> - device_destroy(ipmi_class, entry->dev);
> + device_destroy(_class, entry->dev);
> kfree(entry);
> }
> mutex_unlock(_list_mutex);
> - class_destroy(ipmi_class);
> + class_unregister(_class);
> ipmi_smi_watcher_unregister(_watcher);
> unregister_chrdev(ipmi_major, DEVICE_NAME);
> }
> --
> 2.41.0
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi:ssif: Add check for kstrdup
On Mon, Jun 19, 2023 at 05:28:02PM +0800, Jiasheng Jiang wrote:
> Add check for the return value of kstrdup() and return the error
> if it fails in order to avoid NULL pointer dereference.
Thanks, this is in my next tree.
-corey
>
> Fixes: c4436c9149c5 ("ipmi_ssif: avoid registering duplicate ssif interface")
> Signed-off-by: Jiasheng Jiang
> ---
> drivers/char/ipmi/ipmi_ssif.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 3b921c78ba08..3b87a2726e99 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1600,6 +1600,11 @@ static int ssif_add_infos(struct i2c_client *client)
> info->addr_src = SI_ACPI;
> info->client = client;
> info->adapter_name = kstrdup(client->adapter->name, GFP_KERNEL);
> + if (!info->adapter_name) {
> + kfree(info);
> + return -ENOMEM;
> + }
> +
> info->binfo.addr = client->addr;
> list_add_tail(>link, _infos);
> return 0;
> --
> 2.25.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] dt-bindings: ipmi: aspeed, ast2400-kcs-bmc: drop unneeded quotes
On Sat, Jun 10, 2023 at 08:49:27AM +0930, Andrew Jeffery wrote: > > > On Fri, 9 Jun 2023, at 23:37, Krzysztof Kozlowski wrote: > > Cleanup bindings dropping unneeded quotes. Once all these are fixed, > > checking for this can be enabled in yamllint. > > > > Signed-off-by: Krzysztof Kozlowski > > Acked-by: Andrew Jeffery This is in my next tree. Thank you. -corey > > > --- > > .../devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > > b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > > index 4ff6fabfcb30..129e32c4c774 100644 > > --- a/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > > +++ b/Documentation/devicetree/bindings/ipmi/aspeed,ast2400-kcs-bmc.yaml > > @@ -41,7 +41,7 @@ properties: > >- description: STR register > > > >aspeed,lpc-io-reg: > > -$ref: '/schemas/types.yaml#/definitions/uint32-array' > > +$ref: /schemas/types.yaml#/definitions/uint32-array > > minItems: 1 > > maxItems: 2 > > description: | > > @@ -50,7 +50,7 @@ properties: > >status address may be optionally provided. > > > >aspeed,lpc-interrupts: > > -$ref: "/schemas/types.yaml#/definitions/uint32-array" > > +$ref: /schemas/types.yaml#/definitions/uint32-array > > minItems: 2 > > maxItems: 2 > > description: | > > @@ -63,12 +63,12 @@ properties: > > > >kcs_chan: > > deprecated: true > > -$ref: '/schemas/types.yaml#/definitions/uint32' > > +$ref: /schemas/types.yaml#/definitions/uint32 > > description: The LPC channel number in the controller > > > >kcs_addr: > > deprecated: true > > -$ref: '/schemas/types.yaml#/definitions/uint32' > > +$ref: /schemas/types.yaml#/definitions/uint32 > > description: The host CPU IO map address > > > > required: > > -- > > 2.34.1 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Switch i2c drivers back to use .probe()
On Thu, May 25, 2023 at 10:40:21PM +0200, Uwe Kleine-König wrote:
> After commit b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new()
> call-back type"), all drivers being converted to .probe_new() and then
> 03c835f498b5 ("i2c: Switch .probe() to not take an id parameter") convert
> back to (the new) .probe() to be able to eventually drop .probe_new() from
> struct i2c_driver.
Ok, this is in my for-next tree.
Or, if you prefer:
Acked-by: Corey Minyard
if you would prefer to apply this, and I can drop mine.
-corey
>
> Signed-off-by: Uwe Kleine-König
> ---
> drivers/char/ipmi/ipmb_dev_int.c | 2 +-
> drivers/char/ipmi/ipmi_ipmb.c| 2 +-
> drivers/char/ipmi/ipmi_ssif.c| 2 +-
> drivers/char/ipmi/ssif_bmc.c | 2 +-
> 4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c
> b/drivers/char/ipmi/ipmb_dev_int.c
> index a0e9e80d92ee..49100845fcb7 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -366,7 +366,7 @@ static struct i2c_driver ipmb_driver = {
> .name = "ipmb-dev",
> .acpi_match_table = ACPI_PTR(acpi_ipmb_id),
> },
> - .probe_new = ipmb_probe,
> + .probe = ipmb_probe,
> .remove = ipmb_remove,
> .id_table = ipmb_id,
> };
> diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
> index 3f1c9f1573e7..4e335832fc26 100644
> --- a/drivers/char/ipmi/ipmi_ipmb.c
> +++ b/drivers/char/ipmi/ipmi_ipmb.c
> @@ -572,7 +572,7 @@ static struct i2c_driver ipmi_ipmb_driver = {
> .name = DEVICE_NAME,
> .of_match_table = of_ipmi_ipmb_match,
> },
> - .probe_new = ipmi_ipmb_probe,
> + .probe = ipmi_ipmb_probe,
> .remove = ipmi_ipmb_remove,
> .id_table = ipmi_ipmb_id,
> };
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 3b921c78ba08..c6c9bcf6bf55 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -2054,7 +2054,7 @@ static struct i2c_driver ssif_i2c_driver = {
> .driver = {
> .name = DEVICE_NAME
> },
> - .probe_new = ssif_probe,
> + .probe = ssif_probe,
> .remove = ssif_remove,
> .alert = ssif_alert,
> .id_table = ssif_id,
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index caee848261e9..56346fb32872 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -860,7 +860,7 @@ static struct i2c_driver ssif_bmc_driver = {
> .name = DEVICE_NAME,
> .of_match_table = ssif_bmc_match,
> },
> - .probe_new = ssif_bmc_probe,
> + .probe = ssif_bmc_probe,
> .remove = ssif_bmc_remove,
> .id_table = ssif_bmc_id,
> };
>
> base-commit: ac9a78681b921877518763ba0e89202254349d1b
> --
> 2.39.2
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] watchdog: Avoid 100% CPU usage during reading watchdog when a task get signal
On Mon, May 15, 2023 at 05:19:41AM -0700, Yu Chen wrote:
> A simple reproducer demonstrating the problem: (use ipmi_watchdog.ko)
>
> In one terminal:
>
> $ cat /dev/watchdog
> ...
>
> In another terminal:
>
> $ ps -aux | grep cat
> 14755 pts/1R+43:00 cat /dev/watchdog
> 51943 pts/2S+ 0:00 grep --color=auto cat
>
> $ kill -9 14755
> $
> $ cat /proc/14755/status | grep SigPnd
> SigPnd: 0100
> $
> $ top
>
> Tasks: 1049 total, 2 running, 1047 sleeping, 0 stopped, 0 zombie
> %Cpu(s): 0.0 us, 1.0 sy, 0.0 ni, 98.9 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st
> MiB Mem : 522594.8 total, 517241.4 free, 2922.1 used, 2431.2 buff/cache
> MiB Swap: 0.0 total, 0.0 free, 0.0 used. 516589.2 avail Mem
>
> PID USERPR NIVIRTRESSHR S %CPU %MEM TIME+ COMMAND
> 14755 root 20 0 215552 1024576 R 100.0 0.0 0:15.12 cat
> 53417 root 20 0 224960 7040 3648 R 0.7 0.0 0:00.10 top
> 11 root 20 0 0 0 0 I 0.3 0.0 0:02.85 rcu_sched
> 1772 root 20 0 512256 387776 380800 S 0.3 0.1 0:32.05 python
>
> We can see that when the cat process gets the signal, the CPU usage
> is 100%, Since signal_pending is true, the pick_next_task function
> in schedule always returns itself, it retries schedule indefinitely.
> ipmi_read() will busyloop.
>
> Signed-off-by: Yu Chen
> ---
> drivers/char/ipmi/ipmi_watchdog.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_watchdog.c
> b/drivers/char/ipmi/ipmi_watchdog.c
> index 0d4a8dcac..173ed4266 100644
> --- a/drivers/char/ipmi/ipmi_watchdog.c
> +++ b/drivers/char/ipmi/ipmi_watchdog.c
> @@ -803,6 +803,11 @@ static ssize_t ipmi_read(struct file *file,
> init_waitqueue_entry(, current);
> add_wait_queue(_q, );
> while (!data_to_read) {
> + if (signal_pending(current)) {
> + remove_wait_queue(_q, );
> + rv = -ERESTARTSYS;
> + goto out;
> + }
This skips the call to remove_from_wait_queue(), which is bad. I
already have a fix for this from someone else.
-corey
> set_current_state(TASK_INTERRUPTIBLE);
> spin_unlock_irq(_read_lock);
> schedule();
> @@ -810,10 +815,6 @@ static ssize_t ipmi_read(struct file *file,
> }
> remove_wait_queue(_q, );
>
> - if (signal_pending(current)) {
> - rv = -ERESTARTSYS;
> - goto out;
> - }
> }
> data_to_read = 0;
>
> --
> 2.27.0
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi_watchdog: Fix read syscall not responding to signals during sleep
On Wed, May 17, 2023 at 04:54:12PM +0800, Zhen Ni wrote:
> Read syscall cannot response to sigals when data_to_read remains at 0
> and the while loop cannot break. Fix it.
>
> Signed-off-by: Zhen Ni
> ---
> drivers/char/ipmi/ipmi_watchdog.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_watchdog.c
> b/drivers/char/ipmi/ipmi_watchdog.c
> index 0d4a8dcacfd4..e7eb3e140444 100644
> --- a/drivers/char/ipmi/ipmi_watchdog.c
> +++ b/drivers/char/ipmi/ipmi_watchdog.c
> @@ -807,13 +807,12 @@ static ssize_t ipmi_read(struct file *file,
> spin_unlock_irq(_read_lock);
> schedule();
> spin_lock_irq(_read_lock);
> + if (signal_pending(current)) {
> + rv = -ERESTARTSYS;
> + break;
> + }
This is a bug, but your fix isn't quite correct. If you do this, then
data_to_read will be set to zero on a signal, and you want to return the
ERESTARTSYS and not clear data_to_read in that case.
Instead of your fix, I have added a "!signal_pending()" to the while
loop check, which was probably my original intent.
-corey
> }
> remove_wait_queue(_q, );
> -
> - if (signal_pending(current)) {
> - rv = -ERESTARTSYS;
> - goto out;
> - }
> }
> data_to_read = 0;
>
> --
> 2.20.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes for 6.4
Pull request: - The following changes since commit 982818426a0ffaf93b0621826ed39a84be3d7d62: Merge tag 'arm-fixes-6.3-1' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc (2023-02-27 10:09:40 -0800) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-6.4-1 for you to fetch changes up to d08076678ce72140a40553d226f90d189fbe06d1: ipmi:ssif: Drop if blocks with always false condition (2023-04-12 11:13:26 -0500) Minor bug fixes for the IPMI driver There was a bug in the SSIF driver where in certain conditions it could stop working. Outside of that: spelling fixes, removing some dead code, re-adding a missing statistic increment, and removal of register_sysctl_table(). Corey Minyard (1): ipmi:ssif: Add send_retries increment Luis Chamberlain (1): ipmi: simplify sysctl registration Randy Dunlap (1): ipmi: ASPEED_BT_IPMI_BMC: select REGMAP_MMIO instead of depending on it Uwe Kleine-König (1): ipmi:ssif: Drop if blocks with always false condition Zhang Yuchen (1): ipmi: fix SSIF not responding under certain cond. zipeng zhang (1): char:ipmi:Fix spelling mistake "asychronously" -> "asynchronously" drivers/char/ipmi/Kconfig | 3 ++- drivers/char/ipmi/ipmi_poweroff.c | 16 +--- drivers/char/ipmi/ipmi_ssif.c | 16 ++-- 3 files changed, 9 insertions(+), 26 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: ipmi-bmc: Improve errno returned to userspace
Andrew, what do you think?
-corey
On Wed, Apr 19, 2023 at 05:00:32PM +0200, Govert Overgaauw via
Openipmi-developer wrote:
> While the KCS driver is not in KCS_PHASE_WAIT_READ state it returns
> -EINVAL to userspace on a write call. change this to -EAGAIN to indicate
> that the error is related to the state and not the argument.
>
> Signed-off-by: Govert Overgaauw
> ---
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index cf670e891966..4c7400faf333 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -405,7 +405,7 @@ static ssize_t kcs_bmc_ipmi_write(struct file *filp,
> const char __user *buf,
> kcs_bmc_write_data(priv->client.dev, priv->data_out[0]);
> ret = count;
> } else {
> - ret = -EINVAL;
> + ret = -EAGAIN;
> }
> spin_unlock_irq(>lock);
>
> --
> 2.30.2
>
>
>
> ___
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi:ssif: Drop if blocks with always false condition
On Wed, Apr 12, 2023 at 05:07:21PM +0200, Uwe Kleine-König wrote:
> On Fri, Dec 30, 2022 at 01:44:31PM +0100, Uwe Kleine-König wrote:
> > For both variants (platform and i2c driver) after a successful bind
> > (i.e. .probe completed without error) driver data is set to a non-NULL
> > value.
> >
> > So the return value of i2c_get_clientdata and dev_get_drvdata
> > respectively are not NULL and so the if blocks are never executed. (And
> > if you fear they might, they shouldn't return silently and yield a
> > resource leak.)
> >
> > Signed-off-by: Uwe Kleine-König
>
> This patch waits for feedback now for > 3 month. Is this still on
> someone's todo list?
I apologise, this got lost. It's queued for next merge window.
Thank you,
-corey
>
> Best regards
> Uwe
>
> > ---
> > drivers/char/ipmi/ipmi_ssif.c | 6 --
> > 1 file changed, 6 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > index 4bfd1e306616..a0090ea54e48 100644
> > --- a/drivers/char/ipmi/ipmi_ssif.c
> > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > @@ -1286,9 +1286,6 @@ static void ssif_remove(struct i2c_client *client)
> > struct ssif_info *ssif_info = i2c_get_clientdata(client);
> > struct ssif_addr_info *addr_info;
> >
> > - if (!ssif_info)
> > - return;
> > -
> > /*
> > * After this point, we won't deliver anything asychronously
> > * to the message handler. We can unregister ourself.
> > @@ -2074,9 +2071,6 @@ static int ssif_platform_remove(struct
> > platform_device *dev)
> > {
> > struct ssif_addr_info *addr_info = dev_get_drvdata(>dev);
> >
> > - if (!addr_info)
> > - return 0;
> > -
> > mutex_lock(_infos_mutex);
> > list_del(_info->link);
> > kfree(addr_info);
> > --
> > 2.38.1
> >
> >
> >
>
> --
> Pengutronix e.K. | Uwe Kleine-König|
> Industrial Linux Solutions | https://www.pengutronix.de/ |
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: fix SSIF not responding under certain cond.
On Wed, Apr 12, 2023 at 03:49:07PM +0800, Zhang Yuchen wrote:
> The ipmi communication is not restored after a specific version of BMC is
> upgraded on our server.
> The ipmi driver does not respond after printing the following log:
Excellent explaination, this is queued in my branch for the next release
and marked for backport.
Thank you,
-corey
>
> ipmi_ssif: Invalid response getting flags: 1c 1
>
> I found that after entering this branch, ssif_info->ssif_state always
> holds SSIF_GETTING_FLAGS and never return to IDLE.
>
> As a result, the driver cannot be loaded, because the driver status is
> checked during the unload process and must be IDLE in shutdown_ssif():
>
> while (ssif_info->ssif_state != SSIF_IDLE)
> schedule_timeout(1);
>
> The process trigger this problem is:
>
> 1. One msg timeout and next msg start send, and call
> ssif_set_need_watch().
>
> 2. ssif_set_need_watch()->watch_timeout()->start_flag_fetch() change
> ssif_state to SSIF_GETTING_FLAGS.
>
> 3. In msg_done_handler() ssif_state == SSIF_GETTING_FLAGS, if an error
> message is received, the second branch does not modify the ssif_state.
>
> 4. All retry action need IS_SSIF_IDLE() == True. Include retry action in
> watch_timeout(), msg_done_handler(). Sending msg does not work either.
> SSIF_IDLE is also checked in start_next_msg().
>
> 5. The only thing that can be triggered in the SSIF driver is
> watch_timeout(), after destory_user(), this timer will stop too.
>
> So, if enter this branch, the ssif_state will remain SSIF_GETTING_FLAGS
> and can't send msg, no timer started, can't unload.
>
> We did a comparative test before and after adding this patch, and the
> result is effective.
>
> Fixes: 259307074bfc ("ipmi: Add SMBus interface driver (SSIF)")
>
> Signed-off-by: Zhang Yuchen
> ---
> drivers/char/ipmi/ipmi_ssif.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index a5ddebb1edea..48be3694fa64 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -784,9 +784,9 @@ static void msg_done_handler(struct ssif_info *ssif_info,
> int result,
> } else if (data[0] != (IPMI_NETFN_APP_REQUEST | 1) << 2
> || data[1] != IPMI_GET_MSG_FLAGS_CMD) {
> /*
> - * Don't abort here, maybe it was a queued
> - * response to a previous command.
> + * Recv error response, give up.
>*/
> + ssif_info->ssif_state = SSIF_IDLE;
> ipmi_ssif_unlock_cond(ssif_info, flags);
> dev_warn(_info->client->dev,
>"Invalid response getting flags: %x %x\n",
> --
> 2.20.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] char:ipmi:Fix spelling mistake "asychronously" -> "asynchronously"
On Thu, Mar 16, 2023 at 02:39:58PM +0800, zipeng zhang wrote: > There is a spelling mistake in the comment information. Fix it. It's in my queue. Thanks. -corey > > Signed-off-by: zipeng zhang > --- > drivers/char/ipmi/ipmi_ssif.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c > index a5ddebb1edea..1a85b400e929 100644 > --- a/drivers/char/ipmi/ipmi_ssif.c > +++ b/drivers/char/ipmi/ipmi_ssif.c > @@ -1283,7 +1283,7 @@ static void ssif_remove(struct i2c_client *client) > return; > > /* > - * After this point, we won't deliver anything asychronously > + * After this point, we won't deliver anything asynchronously >* to the message handler. We can unregister ourself. >*/ > ipmi_unregister_smi(ssif_info->intf); > -- > 2.39.2 > > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Wed, Mar 15, 2023 at 01:12:05PM +0100, Christian Theune via
Openipmi-developer wrote:
> Ah, fantastic! That explains it of course … :)
>
> From my side I guess this works and I don’t have to retry with that, but I’d
> be happy to just wait for 5.10.175 … or would you prefer me explicitly
> testing your original?
We can just wait. The problem is obvious now, and the backports are in
progress.
Thanks for helping me with this.
-corey
>
> Christian
>
> > On 15. Mar 2023, at 13:07, Corey Minyard wrote:
> >
> > On Wed, Mar 15, 2023 at 07:32:41AM +0100, Christian Theune via
> > Openipmi-developer wrote:
> >> Hi,
> >>
> >> that didn’t apply on 5.10. Here’s what I’m currently trying to build after
> >> manually inspecting the rejected patch:
> >>
> >
> > Well, I guess I should have sent the prerequisite patch, too. Her it
> > is:
> >
> > a01a89b1db ("ipmi/watchdog: replace atomic_add() and atomic_sub()")
> >
> > Also attached.
> >
> > -corey
> >
> >>
> >>
> >>> On 14. Mar 2023, at 18:29, Corey Minyard wrote:
> >>>
> >>> Well, dang, I had already fixed this a year and a half ago. I wish I
> >>> had a better memory.
> >>>
> >>> Anyway, the fix is commit db05ddf7f321634c5659a0cf7ea56594e22365f7
> >>> ("ipmi:watchdog: Set panic count to proper value on a panic") in
> >>> mainstream 5.16. I'm attaching that patch.
> >>>
> >>> -corey
> >>>
> >>> On Tue, Mar 14, 2023 at 03:58:26PM +0100, Christian Theune via
> >>> Openipmi-developer wrote:
> >>>> Awesome!
> >>>>
> >>>>> On 14. Mar 2023, at 15:54, Corey Minyard wrote:
> >>>>>
> >>>>> On Tue, Mar 14, 2023 at 03:22:39PM +0100, Christian Theune via
> >>>>> Openipmi-developer wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> sorry, I didn’t expect you to make me a branch. I had already taken
> >>>>>> your diff over to 5.10 as it applied cleanly … sorry for the
> >>>>>> additional work and thanks anyways.
> >>>>>
> >>>>> Ok, that's great. It's something about the IPMI watchdog panic
> >>>>> routines, and I can reproduce. I should be able to fix this pretty
> >>>>> quickly. I'll send a patch when I get this fixed.
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> -corey
> >>>>>
> >>>>>>
> >>>>>> Here’s the output:
> >>>>>>
> >>>>>> [ 6521.905890] sysrq: Trigger a crash
> >>>>>> [ 6521.909294] Kernel panic - not syncing: sysrq triggered crash
> >>>>>> [ 6521.915026] CPU: 1 PID: 43785 Comm: bash Tainted: G I
> >>>>>> 5.10.159 #1-NixOS
> >>>>>> [ 6521.922925] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS
> >>>>>> 1.11.0 07/23/2012
> >>>>>> [ 6521.930475] Call Trace:
> >>>>>> [ 6521.932923] dump_stack+0x6b/0x83
> >>>>>> [ 6521.936230] panic+0x101/0x2c8
> >>>>>> [ 6521.939276] ? printk+0x58/0x73
> >>>>>> [ 6521.942408] sysrq_handle_crash+0x16/0x20
> >>>>>> [ 6521.946407] __handle_sysrq.cold+0x43/0x11a
> >>>>>> [ 6521.950580] write_sysrq_trigger+0x24/0x40
> >>>>>> [ 6521.954668] proc_reg_write+0x51/0x90
> >>>>>> [ 6521.958322] vfs_write+0xc3/0x280
> >>>>>> [ 6521.961627] ksys_write+0x5f/0xe0
> >>>>>> [ 6521.964935] do_syscall_64+0x33/0x40
> >>>>>> [ 6521.968502] entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >>>>>> [ 6521.973540] RIP: 0033:0x7f2c6b91a133
> >>>>>> [ 6521.977106] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3
> >>>>>> 0f 1f 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00
> >>>>>> 00 0f 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48
> >>>>>> 89 f5
> >>>>>> [ 6521.995836] RSP: 002b:7ffc4cf11088 EFLAGS: 0246 ORIG_RAX:
> >>>>>> 0001
> >>>>>> [ 6522.003387] RAX: ffda RBX: 0002 RCX:
> >>>>>> 000
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Wed, Mar 15, 2023 at 07:32:41AM +0100, Christian Theune via
Openipmi-developer wrote:
> Hi,
>
> that didn’t apply on 5.10. Here’s what I’m currently trying to build after
> manually inspecting the rejected patch:
>
Well, I guess I should have sent the prerequisite patch, too. Her it
is:
a01a89b1db ("ipmi/watchdog: replace atomic_add() and atomic_sub()")
Also attached.
-corey
>
>
> > On 14. Mar 2023, at 18:29, Corey Minyard wrote:
> >
> > Well, dang, I had already fixed this a year and a half ago. I wish I
> > had a better memory.
> >
> > Anyway, the fix is commit db05ddf7f321634c5659a0cf7ea56594e22365f7
> > ("ipmi:watchdog: Set panic count to proper value on a panic") in
> > mainstream 5.16. I'm attaching that patch.
> >
> > -corey
> >
> > On Tue, Mar 14, 2023 at 03:58:26PM +0100, Christian Theune via
> > Openipmi-developer wrote:
> >> Awesome!
> >>
> >>> On 14. Mar 2023, at 15:54, Corey Minyard wrote:
> >>>
> >>> On Tue, Mar 14, 2023 at 03:22:39PM +0100, Christian Theune via
> >>> Openipmi-developer wrote:
> >>>> Hi,
> >>>>
> >>>> sorry, I didn’t expect you to make me a branch. I had already taken your
> >>>> diff over to 5.10 as it applied cleanly … sorry for the additional work
> >>>> and thanks anyways.
> >>>
> >>> Ok, that's great. It's something about the IPMI watchdog panic
> >>> routines, and I can reproduce. I should be able to fix this pretty
> >>> quickly. I'll send a patch when I get this fixed.
> >>>
> >>> Thanks,
> >>>
> >>> -corey
> >>>
> >>>>
> >>>> Here’s the output:
> >>>>
> >>>> [ 6521.905890] sysrq: Trigger a crash
> >>>> [ 6521.909294] Kernel panic - not syncing: sysrq triggered crash
> >>>> [ 6521.915026] CPU: 1 PID: 43785 Comm: bash Tainted: G I
> >>>> 5.10.159 #1-NixOS
> >>>> [ 6521.922925] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS
> >>>> 1.11.0 07/23/2012
> >>>> [ 6521.930475] Call Trace:
> >>>> [ 6521.932923] dump_stack+0x6b/0x83
> >>>> [ 6521.936230] panic+0x101/0x2c8
> >>>> [ 6521.939276] ? printk+0x58/0x73
> >>>> [ 6521.942408] sysrq_handle_crash+0x16/0x20
> >>>> [ 6521.946407] __handle_sysrq.cold+0x43/0x11a
> >>>> [ 6521.950580] write_sysrq_trigger+0x24/0x40
> >>>> [ 6521.954668] proc_reg_write+0x51/0x90
> >>>> [ 6521.958322] vfs_write+0xc3/0x280
> >>>> [ 6521.961627] ksys_write+0x5f/0xe0
> >>>> [ 6521.964935] do_syscall_64+0x33/0x40
> >>>> [ 6521.968502] entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >>>> [ 6521.973540] RIP: 0033:0x7f2c6b91a133
> >>>> [ 6521.977106] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f
> >>>> 1f 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f
> >>>> 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48 89 f5
> >>>> [ 6521.995836] RSP: 002b:7ffc4cf11088 EFLAGS: 0246 ORIG_RAX:
> >>>> 0001
> >>>> [ 6522.003387] RAX: ffda RBX: 0002 RCX:
> >>>> 7f2c6b91a133
> >>>> [ 6522.010505] RDX: 0002 RSI: 01555c08 RDI:
> >>>> 0001
> >>>> [ 6522.017623] RBP: 01555c08 R08: 000a R09:
> >>>> 7f2c6b9aaf40
> >>>> [ 6522.024743] R10: 016e4218 R11: 0246 R12:
> >>>> 0002
> >>>> [ 6522.031864] R13: 7f2c6b9e8520 R14: 7f2c6b9e8720 R15:
> >>>> 0002
> >>>> [ 6522.039085] Calling notifier panic_event+0x0/0x410 [ipmi_msghandler]
> >>>> (8eb8cb44)
> >>>> [ 6522.047071] IPMI message handler: IPMI: panic event handler
> >>>> [ 6522.052628] IPMI message handler: IPMI: handling panic event for intf
> >>>> 0: 443777b3 67d05ff8
> >>>> …
> >>>> and then it reboots after the 255 seconds from the watchdog timer are
> >>>> passed.
> >>>>
> >>>> Christian
> >>>>
> >>>>> On 13. Mar 2023, at 18:13, Corey Minyard wrote:
> >>>>>
> >>>>> On Mon, Mar 13, 2023 at 05:4
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Tue, Mar 14, 2023 at 06:42:00PM +0100, Christian Theune via
Openipmi-developer wrote:
> Hi,
>
> ouch, sorry to hear that. I’ll test it with that patch and will ask for a
> backport to 5.10 after that. Is that something you’d do or should I contact
> the maintainers?
I've already requested a backport to 5.4 and 5.10. The backport was
requested in the patch, but it fell out in the earlier kernels because
of a missing prerequisite.
Thanks,
-corey
>
> Christian
>
> > On 14. Mar 2023, at 18:29, Corey Minyard wrote:
> >
> > Well, dang, I had already fixed this a year and a half ago. I wish I
> > had a better memory.
> >
> > Anyway, the fix is commit db05ddf7f321634c5659a0cf7ea56594e22365f7
> > ("ipmi:watchdog: Set panic count to proper value on a panic") in
> > mainstream 5.16. I'm attaching that patch.
> >
> > -corey
> >
> > On Tue, Mar 14, 2023 at 03:58:26PM +0100, Christian Theune via
> > Openipmi-developer wrote:
> >> Awesome!
> >>
> >>> On 14. Mar 2023, at 15:54, Corey Minyard wrote:
> >>>
> >>> On Tue, Mar 14, 2023 at 03:22:39PM +0100, Christian Theune via
> >>> Openipmi-developer wrote:
> >>>> Hi,
> >>>>
> >>>> sorry, I didn’t expect you to make me a branch. I had already taken your
> >>>> diff over to 5.10 as it applied cleanly … sorry for the additional work
> >>>> and thanks anyways.
> >>>
> >>> Ok, that's great. It's something about the IPMI watchdog panic
> >>> routines, and I can reproduce. I should be able to fix this pretty
> >>> quickly. I'll send a patch when I get this fixed.
> >>>
> >>> Thanks,
> >>>
> >>> -corey
> >>>
> >>>>
> >>>> Here’s the output:
> >>>>
> >>>> [ 6521.905890] sysrq: Trigger a crash
> >>>> [ 6521.909294] Kernel panic - not syncing: sysrq triggered crash
> >>>> [ 6521.915026] CPU: 1 PID: 43785 Comm: bash Tainted: G I
> >>>> 5.10.159 #1-NixOS
> >>>> [ 6521.922925] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS
> >>>> 1.11.0 07/23/2012
> >>>> [ 6521.930475] Call Trace:
> >>>> [ 6521.932923] dump_stack+0x6b/0x83
> >>>> [ 6521.936230] panic+0x101/0x2c8
> >>>> [ 6521.939276] ? printk+0x58/0x73
> >>>> [ 6521.942408] sysrq_handle_crash+0x16/0x20
> >>>> [ 6521.946407] __handle_sysrq.cold+0x43/0x11a
> >>>> [ 6521.950580] write_sysrq_trigger+0x24/0x40
> >>>> [ 6521.954668] proc_reg_write+0x51/0x90
> >>>> [ 6521.958322] vfs_write+0xc3/0x280
> >>>> [ 6521.961627] ksys_write+0x5f/0xe0
> >>>> [ 6521.964935] do_syscall_64+0x33/0x40
> >>>> [ 6521.968502] entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >>>> [ 6521.973540] RIP: 0033:0x7f2c6b91a133
> >>>> [ 6521.977106] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f
> >>>> 1f 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f
> >>>> 05 <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48 89 f5
> >>>> [ 6521.995836] RSP: 002b:7ffc4cf11088 EFLAGS: 0246 ORIG_RAX:
> >>>> 0001
> >>>> [ 6522.003387] RAX: ffda RBX: 0002 RCX:
> >>>> 7f2c6b91a133
> >>>> [ 6522.010505] RDX: 0002 RSI: 000001555c08 RDI:
> >>>> 0001
> >>>> [ 6522.017623] RBP: 01555c08 R08: 000a R09:
> >>>> 7f2c6b9aaf40
> >>>> [ 6522.024743] R10: 016e4218 R11: 0246 R12:
> >>>> 0002
> >>>> [ 6522.031864] R13: 7f2c6b9e8520 R14: 7f2c6b9e8720 R15:
> >>>> 0002
> >>>> [ 6522.039085] Calling notifier panic_event+0x0/0x410 [ipmi_msghandler]
> >>>> (8eb8cb44)
> >>>> [ 6522.047071] IPMI message handler: IPMI: panic event handler
> >>>> [ 6522.052628] IPMI message handler: IPMI: handling panic event for intf
> >>>> 0: 443777b3 67d05ff8
> >>>> …
> >>>> and then it reboots after the 255 seconds from the watchdog timer are
> >>>> passed.
> >>>>
> >>>> Christian
> >>>>
> >>>>> On 13. Mar 2023, at 18:13, Corey Mi
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
Well, dang, I had already fixed this a year and a half ago. I wish I
had a better memory.
Anyway, the fix is commit db05ddf7f321634c5659a0cf7ea56594e22365f7
("ipmi:watchdog: Set panic count to proper value on a panic") in
mainstream 5.16. I'm attaching that patch.
-corey
On Tue, Mar 14, 2023 at 03:58:26PM +0100, Christian Theune via
Openipmi-developer wrote:
> Awesome!
>
> > On 14. Mar 2023, at 15:54, Corey Minyard wrote:
> >
> > On Tue, Mar 14, 2023 at 03:22:39PM +0100, Christian Theune via
> > Openipmi-developer wrote:
> >> Hi,
> >>
> >> sorry, I didn’t expect you to make me a branch. I had already taken your
> >> diff over to 5.10 as it applied cleanly … sorry for the additional work
> >> and thanks anyways.
> >
> > Ok, that's great. It's something about the IPMI watchdog panic
> > routines, and I can reproduce. I should be able to fix this pretty
> > quickly. I'll send a patch when I get this fixed.
> >
> > Thanks,
> >
> > -corey
> >
> >>
> >> Here’s the output:
> >>
> >> [ 6521.905890] sysrq: Trigger a crash
> >> [ 6521.909294] Kernel panic - not syncing: sysrq triggered crash
> >> [ 6521.915026] CPU: 1 PID: 43785 Comm: bash Tainted: G I
> >> 5.10.159 #1-NixOS
> >> [ 6521.922925] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS 1.11.0
> >> 07/23/2012
> >> [ 6521.930475] Call Trace:
> >> [ 6521.932923] dump_stack+0x6b/0x83
> >> [ 6521.936230] panic+0x101/0x2c8
> >> [ 6521.939276] ? printk+0x58/0x73
> >> [ 6521.942408] sysrq_handle_crash+0x16/0x20
> >> [ 6521.946407] __handle_sysrq.cold+0x43/0x11a
> >> [ 6521.950580] write_sysrq_trigger+0x24/0x40
> >> [ 6521.954668] proc_reg_write+0x51/0x90
> >> [ 6521.958322] vfs_write+0xc3/0x280
> >> [ 6521.961627] ksys_write+0x5f/0xe0
> >> [ 6521.964935] do_syscall_64+0x33/0x40
> >> [ 6521.968502] entry_SYSCALL_64_after_hwframe+0x61/0xc6
> >> [ 6521.973540] RIP: 0033:0x7f2c6b91a133
> >> [ 6521.977106] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f
> >> 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05
> >> <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48 89 f5
> >> [ 6521.995836] RSP: 002b:7ffc4cf11088 EFLAGS: 0246 ORIG_RAX:
> >> 0001
> >> [ 6522.003387] RAX: ffda RBX: 0002 RCX:
> >> 7f2c6b91a133
> >> [ 6522.010505] RDX: 0002 RSI: 01555c08 RDI:
> >> 0001
> >> [ 6522.017623] RBP: 01555c08 R08: 000a R09:
> >> 7f2c6b9aaf40
> >> [ 6522.024743] R10: 016e4218 R11: 0246 R12:
> >> 0002
> >> [ 6522.031864] R13: 7f2c6b9e8520 R14: 7f2c6b9e8720 R15:
> >> 0002
> >> [ 6522.039085] Calling notifier panic_event+0x0/0x410 [ipmi_msghandler]
> >> (8eb8cb44)
> >> [ 6522.047071] IPMI message handler: IPMI: panic event handler
> >> [ 6522.052628] IPMI message handler: IPMI: handling panic event for intf
> >> 0: 443777b3 67d05ff8
> >> …
> >> and then it reboots after the 255 seconds from the watchdog timer are
> >> passed.
> >>
> >> Christian
> >>
> >>> On 13. Mar 2023, at 18:13, Corey Minyard wrote:
> >>>
> >>> On Mon, Mar 13, 2023 at 05:42:39PM +0100, Christian Theune wrote:
> >>>> Hrghs. I’m applying your patch to 5.10 as my distro build infrastructure
> >>>> has some patches that don’t apply to 6.2 and that I don’t know how to
> >>>> circumvent quickly enough… :)
> >>>
> >>> Ok, there's a
> >>>
> >>> https://github.com/cminyard/linux-ipmi.git:debug-panic-oem-events-5.10
> >>>
> >>> branch available for you to pull. It's on top of latest 5.10.
> >>>
> >>> -corey
> >>>
> >>>>
> >>>>> On 13. Mar 2023, at 16:59, Christian Theune
> >>>>> wrote:
> >>>>>
> >>>>> I should be easily able to run 6.2, no worries.
> >>>>>
> >>>>>
> >>>>>> On 13. Mar 2023, at 16:33, Corey Minyard wrote:
> >>>>>>
> >>>>>> On Mon, Mar 13, 2023 at 02:07:01PM +0100, Christian Theune wrote:
> >>>>>>> Hi,
> >>>>>>>
>
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Tue, Mar 14, 2023 at 03:22:39PM +0100, Christian Theune via Openipmi-developer wrote: > Hi, > > sorry, I didn’t expect you to make me a branch. I had already taken your diff > over to 5.10 as it applied cleanly … sorry for the additional work and thanks > anyways. Ok, that's great. It's something about the IPMI watchdog panic routines, and I can reproduce. I should be able to fix this pretty quickly. I'll send a patch when I get this fixed. Thanks, -corey > > Here’s the output: > > [ 6521.905890] sysrq: Trigger a crash > [ 6521.909294] Kernel panic - not syncing: sysrq triggered crash > [ 6521.915026] CPU: 1 PID: 43785 Comm: bash Tainted: G I > 5.10.159 #1-NixOS > [ 6521.922925] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS 1.11.0 > 07/23/2012 > [ 6521.930475] Call Trace: > [ 6521.932923] dump_stack+0x6b/0x83 > [ 6521.936230] panic+0x101/0x2c8 > [ 6521.939276] ? printk+0x58/0x73 > [ 6521.942408] sysrq_handle_crash+0x16/0x20 > [ 6521.946407] __handle_sysrq.cold+0x43/0x11a > [ 6521.950580] write_sysrq_trigger+0x24/0x40 > [ 6521.954668] proc_reg_write+0x51/0x90 > [ 6521.958322] vfs_write+0xc3/0x280 > [ 6521.961627] ksys_write+0x5f/0xe0 > [ 6521.964935] do_syscall_64+0x33/0x40 > [ 6521.968502] entry_SYSCALL_64_after_hwframe+0x61/0xc6 > [ 6521.973540] RIP: 0033:0x7f2c6b91a133 > [ 6521.977106] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 > 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 <48> 3d > 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48 89 f5 > [ 6521.995836] RSP: 002b:7ffc4cf11088 EFLAGS: 0246 ORIG_RAX: > 0001 > [ 6522.003387] RAX: ffda RBX: 0002 RCX: > 7f2c6b91a133 > [ 6522.010505] RDX: 0002 RSI: 01555c08 RDI: > 0001 > [ 6522.017623] RBP: 01555c08 R08: 000a R09: > 7f2c6b9aaf40 > [ 6522.024743] R10: 016e4218 R11: 0246 R12: > 0002 > [ 6522.031864] R13: 7f2c6b9e8520 R14: 7f2c6b9e8720 R15: > 0002 > [ 6522.039085] Calling notifier panic_event+0x0/0x410 [ipmi_msghandler] > (8eb8cb44) > [ 6522.047071] IPMI message handler: IPMI: panic event handler > [ 6522.052628] IPMI message handler: IPMI: handling panic event for intf 0: > 443777b3 000067d05ff8 > … > and then it reboots after the 255 seconds from the watchdog timer are passed. > > Christian > > > On 13. Mar 2023, at 18:13, Corey Minyard wrote: > > > > On Mon, Mar 13, 2023 at 05:42:39PM +0100, Christian Theune wrote: > >> Hrghs. I’m applying your patch to 5.10 as my distro build infrastructure > >> has some patches that don’t apply to 6.2 and that I don’t know how to > >> circumvent quickly enough… :) > > > > Ok, there's a > > > > https://github.com/cminyard/linux-ipmi.git:debug-panic-oem-events-5.10 > > > > branch available for you to pull. It's on top of latest 5.10. > > > > -corey > > > >> > >>> On 13. Mar 2023, at 16:59, Christian Theune wrote: > >>> > >>> I should be easily able to run 6.2, no worries. > >>> > >>> > >>>> On 13. Mar 2023, at 16:33, Corey Minyard wrote: > >>>> > >>>> On Mon, Mar 13, 2023 at 02:07:01PM +0100, Christian Theune wrote: > >>>>> Hi, > >>>>> > >>>>> yeah, the IPMI log is fine. This is a 10 minute interval job in our > >>>>> system that exports the log and clears it: > >>>>> > >>>>> The job looks like this: > >>>>> > >>>>> /nix/store/m7lb36dr93qj27r9vskmjihz8imywy86-ipmitool-1.8.18/bin/ipmitool > >>>>> sel elist > >>>>> /nix/store/m7lb36dr93qj27r9vskmjihz8imywy86-ipmitool-1.8.18/bin/ipmitool > >>>>> sel clear > >>>>> > >>>>> So it’s not atomic but it runs after the boot and the elist should > >>>>> output it properly … at least it did in the past. ;) > >>>>> > >>>>> As I said - I’m happy to run any patches you have. If you point me to a > >>>>> git branch somewhere I can switch that system easily. > >>>> > >>>> Ok, I have a branch at > >>>> > >>>> https://github.com/cminyard/linux-ipmi.git:debug-panic-oem-events > >>>> > >>>> that has debug tracing. It will print the function for all panic event > >>>> handlers, their return values, and adds t
Re: [Openipmi-developer] [PATCH v3 03/38] char: impi, tpm: depend on HAS_IOPORT
On Tue, Mar 14, 2023 at 01:11:41PM +0100, Niklas Schnelle wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to add this dependency and ifdef
> sections of code using inb()/outb() as alternative access methods.
For the IPMI portions, this looks good.
Acked-by: Corey Minyard
Thanks
-corey
>
> Co-developed-by: Arnd Bergmann
> Signed-off-by: Niklas Schnelle
> ---
> drivers/char/Kconfig | 3 ++-
> drivers/char/ipmi/Makefile | 11 ---
> drivers/char/ipmi/ipmi_si_intf.c | 3 ++-
> drivers/char/ipmi/ipmi_si_pci.c | 3 +++
> drivers/char/pcmcia/Kconfig | 8
> drivers/char/tpm/Kconfig | 1 +
> drivers/char/tpm/tpm_infineon.c | 14 ++
> drivers/char/tpm/tpm_tis_core.c | 19 ---
> 8 files changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 30fe9848dac1..c34679c6da70 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -34,6 +34,7 @@ config TTY_PRINTK_LEVEL
> config PRINTER
> tristate "Parallel printer support"
> depends on PARPORT
> + depends on HAS_IOPORT
> help
> If you intend to attach a printer to the parallel port of your Linux
> box (as opposed to using a serial printer; if the connector at the
> @@ -342,7 +343,7 @@ config NVRAM
>
> config DEVPORT
> bool "/dev/port character device"
> - depends on ISA || PCI
> + depends on HAS_IOPORT
> default y
> help
> Say Y here if you want to support the /dev/port device. The /dev/port
> diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile
> index cb6138b8ded9..e0944547c9d0 100644
> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -5,13 +5,10 @@
>
> ipmi_si-y := ipmi_si_intf.o ipmi_kcs_sm.o ipmi_smic_sm.o ipmi_bt_sm.o \
> ipmi_si_hotmod.o ipmi_si_hardcode.o ipmi_si_platform.o \
> - ipmi_si_port_io.o ipmi_si_mem_io.o
> -ifdef CONFIG_PCI
> -ipmi_si-y += ipmi_si_pci.o
> -endif
> -ifdef CONFIG_PARISC
> -ipmi_si-y += ipmi_si_parisc.o
> -endif
> + 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_PARISC) += ipmi_si_parisc.o
>
> obj-$(CONFIG_IPMI_HANDLER) += ipmi_msghandler.o
> obj-$(CONFIG_IPMI_DEVICE_INTERFACE) += ipmi_devintf.o
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c
> b/drivers/char/ipmi/ipmi_si_intf.c
> index abddd7e43a9a..edbbdb804913 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1882,7 +1882,8 @@ int ipmi_si_add_smi(struct si_sm_io *io)
> }
>
> if (!io->io_setup) {
> - if (io->addr_space == IPMI_IO_ADDR_SPACE) {
> + if (IS_ENABLED(CONFIG_HAS_IOPORT) &&
> + io->addr_space == IPMI_IO_ADDR_SPACE) {
> io->io_setup = ipmi_si_port_setup;
> } else if (io->addr_space == IPMI_MEM_ADDR_SPACE) {
> io->io_setup = ipmi_si_mem_setup;
> diff --git a/drivers/char/ipmi/ipmi_si_pci.c b/drivers/char/ipmi/ipmi_si_pci.c
> index 74fa2055868b..b83d55685b22 100644
> --- a/drivers/char/ipmi/ipmi_si_pci.c
> +++ b/drivers/char/ipmi/ipmi_si_pci.c
> @@ -97,6 +97,9 @@ static int ipmi_pci_probe(struct pci_dev *pdev,
> }
>
> if (pci_resource_flags(pdev, 0) & IORESOURCE_IO) {
> + if (!IS_ENABLED(CONFIG_HAS_IOPORT))
> + return -ENXIO;
> +
> io.addr_space = IPMI_IO_ADDR_SPACE;
> io.io_setup = ipmi_si_port_setup;
> } else {
> diff --git a/drivers/char/pcmcia/Kconfig b/drivers/char/pcmcia/Kconfig
> index f5d589b2be44..788422627b43 100644
> --- a/drivers/char/pcmcia/Kconfig
> +++ b/drivers/char/pcmcia/Kconfig
> @@ -8,7 +8,7 @@ menu "PCMCIA character devices"
>
> config SYNCLINK_CS
> tristate "SyncLink PC Card support"
> - depends on PCMCIA && TTY
> + depends on PCMCIA && TTY && HAS_IOPORT
> help
> Enable support for the SyncLink PC Card serial adapter, running
> asynchronous and HDLC communications up to 512Kbps. The port is
> @@ -21,7 +21,7 @@ config SYNCLINK_CS
>
> config CARDMAN_4000
> tristate "Omnikey Cardman 4000 support"
> - depends on PCMCIA
> + depends on PCMCIA && HAS_IOPORT
> select BITREVERSE
> help
> Enable support for the Omnikey Cardman 4000 PCMCIA Smartcard
> @@ -33,7 +33,7 @@ conf
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Mon, Mar 13, 2023 at 02:07:01PM +0100, Christian Theune wrote: > Hi, > > yeah, the IPMI log is fine. This is a 10 minute interval job in our system > that exports the log and clears it: > > The job looks like this: > > /nix/store/m7lb36dr93qj27r9vskmjihz8imywy86-ipmitool-1.8.18/bin/ipmitool sel > elist > /nix/store/m7lb36dr93qj27r9vskmjihz8imywy86-ipmitool-1.8.18/bin/ipmitool sel > clear > > So it’s not atomic but it runs after the boot and the elist should output it > properly … at least it did in the past. ;) > > As I said - I’m happy to run any patches you have. If you point me to a git > branch somewhere I can switch that system easily. Ok, I have a branch at https://github.com/cminyard/linux-ipmi.git:debug-panic-oem-events that has debug tracing. It will print the function for all panic event handlers, their return values, and adds traces in the IPMI panic event handlers. It's a single patch right on top of 6.2; I'm not sure how portable it is to other kernel versions. I can port if you like. Thanks, -corey > > Cheers, > Christian > > > On 13. Mar 2023, at 13:58, Corey Minyard wrote: > > > > On Mon, Mar 13, 2023 at 10:27:51AM +0100, Christian Theune wrote: > >> Hi, > >> > >> alright, so here’s the output from the NixOS machine: > >> > >> root@xxx ~ # echo c >/proc/sysrq-trigger > >> client_loop: send disconnect: Broken pipe > >> … > >> > >> root@xxx ~ # journalctl -u ipmi-log.service > >> -- Journal begins at Sun 2023-02-26 14:25:36 CET, ends at Mon 2023-03-13 > >> 10:25:27 CET. -- > >> Mar 13 10:12:38 xxx ipmi-log-start[520973]: Clearing SEL. Please allow a > >> few seconds to erase. > >> ... > >> -- Boot fdef496e784e4541abd9ae40df472a0b -- > >> Mar 13 10:25:07 xxx ipmi-log-start[1973]:1 | 03/13/2023 | 09:12:49 | > >> Event Logging Disabled SEL | Log area reset/cleared | Asserted > >> Mar 13 10:25:07 xxx ipmi-log-start[1973]:2 | 03/13/2023 | 09:21:06 | > >> Watchdog2 OS Watchdog | Hard reset | Asserted > >> Mar 13 10:25:07 xxx ipmi-log-start[1977]: Clearing SEL. Please allow a > >> few seconds to erase. > > > > Hmm, the SEL got cleared. That would clear out any of the logs that > > were issued before that time. I'm not sure when the above happened > > verses the crash, though. It looks like it occurred as part of the > > reboot, but I'm not sure what I'm seeing. Maybe you have a startup > > process that clears the SEL? > > > > Assuming that's not the issue, what you have looks ok. I'd need to add > > some logs to the kernel to see if the log operation ever happens. > > > > -corey > > > >> > >> The SOL log looks like this: > >> > >> > >> [1107585.917689] sysrq: Trigger a crash > >> [1107585.921272] Kernel panic - not syncing: sysrq triggered crash > >> [1107585.927178] CPU: 1 PID: 521033 Comm: bash Tainted: G I > >> 5.10.159 #1-NixOS > >> [1107585.935335] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS > >> 1.11.0 07/23/2012 > >> [1107585.943058] Call Trace: > >> [1107585.945680] dump_stack+0x6b/0x83 > >> [1107585.949158] panic+0x101/0x2c8 > >> [1107585.952379] ? printk+0x58/0x73 > >> [1107585.955687] sysrq_handle_crash+0x16/0x20 > >> [1107585.959859] __handle_sysrq.cold+0x43/0x11a > >> [1107585.964203] write_sysrq_trigger+0x24/0x40 > >> [1107585.968463] proc_reg_write+0x51/0x90 > >> [1107585.972290] vfs_write+0xc3/0x280 > >> [1107585.975768] ksys_write+0x5f/0xe0 > >> [1107585.979248] do_syscall_64+0x33/0x40 > >> [1107585.982987] entry_SYSCALL_64_after_hwframe+0x61/0xc6 > >> [1107585.988199] RIP: 0033:0x7f5873932133 > >> [1107585.991938] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f > >> 1f 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 > >> <48> 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48 89 f5 > >> [1107586.010842] RSP: 002b:7ffcc13808c8 EFLAGS: 0246 ORIG_RAX: > >> 0001 > >> [1107586.018566] RAX: ffda RBX: 0002 RCX: > >> 00007f5873932133 > >> [1107586.025923] RDX: 0002 RSI: 005c1c08 RDI: > >> 0001 > >> [1107586.033213] RBP: 005c1c08 R08: 000a R09: > >> 7f58739c2f40 > >> [1107586.040504] R10: 005cc348 R11: 0246 R12: > >> 0002 > >> [1107586.047794] R13:
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Mon, Mar 13, 2023 at 10:27:51AM +0100, Christian Theune wrote: > Hi, > > alright, so here’s the output from the NixOS machine: > > root@xxx ~ # echo c >/proc/sysrq-trigger > client_loop: send disconnect: Broken pipe > … > > root@xxx ~ # journalctl -u ipmi-log.service > -- Journal begins at Sun 2023-02-26 14:25:36 CET, ends at Mon 2023-03-13 > 10:25:27 CET. -- > Mar 13 10:12:38 xxx ipmi-log-start[520973]: Clearing SEL. Please allow a few > seconds to erase. > ... > -- Boot fdef496e784e4541abd9ae40df472a0b -- > Mar 13 10:25:07 xxx ipmi-log-start[1973]:1 | 03/13/2023 | 09:12:49 | > Event Logging Disabled SEL | Log area reset/cleared | Asserted > Mar 13 10:25:07 xxx ipmi-log-start[1973]:2 | 03/13/2023 | 09:21:06 | > Watchdog2 OS Watchdog | Hard reset | Asserted > Mar 13 10:25:07 xxx ipmi-log-start[1977]: Clearing SEL. Please allow a few > seconds to erase. Hmm, the SEL got cleared. That would clear out any of the logs that were issued before that time. I'm not sure when the above happened verses the crash, though. It looks like it occurred as part of the reboot, but I'm not sure what I'm seeing. Maybe you have a startup process that clears the SEL? Assuming that's not the issue, what you have looks ok. I'd need to add some logs to the kernel to see if the log operation ever happens. -corey > > The SOL log looks like this: > > > [1107585.917689] sysrq: Trigger a crash > [1107585.921272] Kernel panic - not syncing: sysrq triggered crash > [1107585.927178] CPU: 1 PID: 521033 Comm: bash Tainted: G I > 5.10.159 #1-NixOS > [1107585.935335] Hardware name: Dell Inc. PowerEdge R510/00HDP0, BIOS 1.11.0 > 07/23/2012 > [1107585.943058] Call Trace: > [1107585.945680] dump_stack+0x6b/0x83 > [1107585.949158] panic+0x101/0x2c8 > [1107585.952379] ? printk+0x58/0x73 > [1107585.955687] sysrq_handle_crash+0x16/0x20 > [1107585.959859] __handle_sysrq.cold+0x43/0x11a > [1107585.964203] write_sysrq_trigger+0x24/0x40 > [1107585.968463] proc_reg_write+0x51/0x90 > [1107585.972290] vfs_write+0xc3/0x280 > [1107585.975768] ksys_write+0x5f/0xe0 > [1107585.979248] do_syscall_64+0x33/0x40 > [1107585.982987] entry_SYSCALL_64_after_hwframe+0x61/0xc6 > [1107585.988199] RIP: 0033:0x7f5873932133 > [1107585.991938] Code: 0c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f > 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 01 00 00 00 0f 05 <48> > 3d 00 f0 ff ff 77 55 c3 0f 1f 40 00 41 54 49 89 d4 55 48 89 f5 > [1107586.010842] RSP: 002b:7ffcc13808c8 EFLAGS: 0246 ORIG_RAX: > 0001 > [1107586.018566] RAX: ffda RBX: 0002 RCX: > 7f5873932133 > [1107586.025923] RDX: 0002 RSI: 005c1c08 RDI: > 0001 > [1107586.033213] RBP: 005c1c08 R08: 000a R09: > 7f58739c2f40 > [1107586.040504] R10: 005cc348 R11: 0246 R12: > 0002 > [1107586.047794] R13: 7f5873a00520 R14: 7f5873a00720 R15: > 0002 > > Nothing obvious to me here … if you have any further ideas what to test, let > me know. I should be more responsive again now. > > Thanks and kind regards, > Christian > > > On 5. Mar 2023, at 23:53, Corey Minyard wrote: > > > > On Wed, Mar 01, 2023 at 06:00:07PM +0100, Christian Theune wrote: > >> I’m going to actually attach a serial console to watch the “echo c” panic, > >> maybe that gives _some_ indication. > >> > >> Otherwise: I can quickly run patches on the kernel there to try out > >> things. (And the funding offer still stands.) > > > > Any news on this? I'm curious what this could be. > > > > -corey > > > >> > >> Christian > >> > >>> On 1. Mar 2023, at 17:58, Corey Minyard wrote: > >>> > >>> On Tue, Feb 28, 2023 at 06:36:17PM +0100, Christian Theune wrote: > >>>> Thanks, both machines report: > >>>> > >>>> # cat /sys/module/ipmi_msghandler/parameters/panic_op > >>>> string > >>> > >>> At this point, I have no idea. I'd have to start adding printks into > >>> the code and cause crashes to see what is happing. > >>> > >>> Maybe something is getting in the way of the panic notifiers and doing > >>> something to prevent the IPMI driver from working. > >>> > >>> -corey > >>> > >>>> > >>>> > >>>>> On 28. Feb 2023, at 18:04, Corey Minyard wrote: > >>>>> > >>>>> Oh, I forgot. You can look at panic_op in > >&
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Wed, Mar 01, 2023 at 06:00:07PM +0100, Christian Theune wrote: > I’m going to actually attach a serial console to watch the “echo c” panic, > maybe that gives _some_ indication. > > Otherwise: I can quickly run patches on the kernel there to try out things. > (And the funding offer still stands.) Any news on this? I'm curious what this could be. -corey > > Christian > > > On 1. Mar 2023, at 17:58, Corey Minyard wrote: > > > > On Tue, Feb 28, 2023 at 06:36:17PM +0100, Christian Theune wrote: > >> Thanks, both machines report: > >> > >> # cat /sys/module/ipmi_msghandler/parameters/panic_op > >> string > > > > At this point, I have no idea. I'd have to start adding printks into > > the code and cause crashes to see what is happing. > > > > Maybe something is getting in the way of the panic notifiers and doing > > something to prevent the IPMI driver from working. > > > > -corey > > > >> > >> > >>> On 28. Feb 2023, at 18:04, Corey Minyard wrote: > >>> > >>> Oh, I forgot. You can look at panic_op in > >>> /sys/module/ipmi_msghandler/parameters/panic_op > >>> > >>> -corey > >>> > >>> On Tue, Feb 28, 2023 at 05:48:07PM +0100, Christian Theune via > >>> Openipmi-developer wrote: > >>>> Hi, > >>>> > >>>>> On 28. Feb 2023, at 17:36, Corey Minyard wrote: > >>>>> > >>>>> On Tue, Feb 28, 2023 at 02:53:12PM +0100, Christian Theune via > >>>>> Openipmi-developer wrote: > >>>>>> Hi, > >>>>>> > >>>>>> I’ve been trying to debug the PANIC and OEM string handling and am > >>>>>> running out of ideas whether this is a bug or whether something so > >>>>>> subtle has changed in my config that I’m just not seeing it. > >>>>>> > >>>>>> (Note: I’m willing to pay for consulting.) > >>>>> > >>>>> Probably not necessary. > >>>> > >>>> Thanks! The offer always stands. If we should ever meet I’m also able to > >>>> pay in beverages. ;) > >>>> > >>>>>> I have machines that we’ve moved from an older setup (Gentoo, (mostly) > >>>>>> vanilla kernel 4.19.157) to a newer setup (NixOS, (mostly) vanilla > >>>>>> kernel 5.10.159) and I’m now experiencing crashes that seem to be > >>>>>> kernel panics but do not get the usual messages in the IPMI SEL. > >>>>> > >>>>> I just tested on stock 5.10.159 and it worked without issue. Everything > >>>>> you have below looks ok. > >>>>> > >>>>> Can you test by causing a crash with: > >>>>> > >>>>> echo c >/proc/sysrq-trigger > >>>>> > >>>>> and see if it works? > >>>> > >>>> Yeah, already tried that and unfortunately that _doesn’t_ work. > >>>> > >>>>> It sounds like you are having some type of crash that you would normally > >>>>> use the IPMI logs to debug. However, they aren't perfect, the system > >>>>> has to stay up long enough to get them into the event log. > >>>> > >>>> I think they are staying up long enough because a panic triggers the 255 > >>>> second bump in the watchdog and only then pass on. However, i’ve also > >>>> noticed that the kernel _should_ be rebooting after a panic much faster > >>>> (and not rely on the watchdog) and that doesn’t happen either. (Sorry > >>>> this just popped from the back of my head). > >>>> > >>>>> In this situation, getting a serial console (probably through IPMI > >>>>> Serial over LAN) and getting the console output on a crash is probably > >>>>> your best option. You can use ipmitool for this, or I have a library > >>>>> that is able to make connections to serial ports, including through IPMI > >>>>> SoL. > >>>> > >>>> Yup. Been there, too. :) > >>>> > >>>> Unfortunately we’re currently chasing something that pops up very > >>>> randomly on somewhat odd machines and I also have the feeling that it’s > >>>> systematically broken right now (as the “echo c” doesn’t work). > &
Re: [Openipmi-developer] [PATCH 2/7] ipmi: simplify sysctl registration
On Thu, Mar 02, 2023 at 12:46:07PM -0800, Luis Chamberlain wrote:
> register_sysctl_table() is a deprecated compatibility wrapper.
> register_sysctl() can do the directory creation for you so just use
> that.
Thanks, I have included this in my tree for the next merge window.
-corey
>
> Signed-off-by: Luis Chamberlain
> ---
> drivers/char/ipmi/ipmi_poweroff.c | 16 +---
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_poweroff.c
> b/drivers/char/ipmi/ipmi_poweroff.c
> index 163ec9749e55..870659d91db2 100644
> --- a/drivers/char/ipmi/ipmi_poweroff.c
> +++ b/drivers/char/ipmi/ipmi_poweroff.c
> @@ -659,20 +659,6 @@ static struct ctl_table ipmi_table[] = {
> { }
> };
>
> -static struct ctl_table ipmi_dir_table[] = {
> - { .procname = "ipmi",
> - .mode = 0555,
> - .child= ipmi_table },
> - { }
> -};
> -
> -static struct ctl_table ipmi_root_table[] = {
> - { .procname = "dev",
> - .mode = 0555,
> - .child= ipmi_dir_table },
> - { }
> -};
> -
> static struct ctl_table_header *ipmi_table_header;
> #endif /* CONFIG_PROC_FS */
>
> @@ -689,7 +675,7 @@ static int __init ipmi_poweroff_init(void)
> pr_info("Power cycle is enabled\n");
>
> #ifdef CONFIG_PROC_FS
> - ipmi_table_header = register_sysctl_table(ipmi_root_table);
> + ipmi_table_header = register_sysctl("dev/ipmi", ipmi_table);
> if (!ipmi_table_header) {
> pr_err("Unable to register powercycle sysctl\n");
> rv = -ENOMEM;
> --
> 2.39.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Tue, Feb 28, 2023 at 06:36:17PM +0100, Christian Theune wrote: > Thanks, both machines report: > > # cat /sys/module/ipmi_msghandler/parameters/panic_op > string At this point, I have no idea. I'd have to start adding printks into the code and cause crashes to see what is happing. Maybe something is getting in the way of the panic notifiers and doing something to prevent the IPMI driver from working. -corey > > > > On 28. Feb 2023, at 18:04, Corey Minyard wrote: > > > > Oh, I forgot. You can look at panic_op in > > /sys/module/ipmi_msghandler/parameters/panic_op > > > > -corey > > > > On Tue, Feb 28, 2023 at 05:48:07PM +0100, Christian Theune via > > Openipmi-developer wrote: > >> Hi, > >> > >>> On 28. Feb 2023, at 17:36, Corey Minyard wrote: > >>> > >>> On Tue, Feb 28, 2023 at 02:53:12PM +0100, Christian Theune via > >>> Openipmi-developer wrote: > >>>> Hi, > >>>> > >>>> I’ve been trying to debug the PANIC and OEM string handling and am > >>>> running out of ideas whether this is a bug or whether something so > >>>> subtle has changed in my config that I’m just not seeing it. > >>>> > >>>> (Note: I’m willing to pay for consulting.) > >>> > >>> Probably not necessary. > >> > >> Thanks! The offer always stands. If we should ever meet I’m also able to > >> pay in beverages. ;) > >> > >>>> I have machines that we’ve moved from an older setup (Gentoo, (mostly) > >>>> vanilla kernel 4.19.157) to a newer setup (NixOS, (mostly) vanilla > >>>> kernel 5.10.159) and I’m now experiencing crashes that seem to be kernel > >>>> panics but do not get the usual messages in the IPMI SEL. > >>> > >>> I just tested on stock 5.10.159 and it worked without issue. Everything > >>> you have below looks ok. > >>> > >>> Can you test by causing a crash with: > >>> > >>> echo c >/proc/sysrq-trigger > >>> > >>> and see if it works? > >> > >> Yeah, already tried that and unfortunately that _doesn’t_ work. > >> > >>> It sounds like you are having some type of crash that you would normally > >>> use the IPMI logs to debug. However, they aren't perfect, the system > >>> has to stay up long enough to get them into the event log. > >> > >> I think they are staying up long enough because a panic triggers the 255 > >> second bump in the watchdog and only then pass on. However, i’ve also > >> noticed that the kernel _should_ be rebooting after a panic much faster > >> (and not rely on the watchdog) and that doesn’t happen either. (Sorry this > >> just popped from the back of my head). > >> > >>> In this situation, getting a serial console (probably through IPMI > >>> Serial over LAN) and getting the console output on a crash is probably > >>> your best option. You can use ipmitool for this, or I have a library > >>> that is able to make connections to serial ports, including through IPMI > >>> SoL. > >> > >> Yup. Been there, too. :) > >> > >> Unfortunately we’re currently chasing something that pops up very randomly > >> on somewhat odd machines and I also have the feeling that it’s > >> systematically broken right now (as the “echo c” doesn’t work). > >> > >> Thanks a lot, > >> Christian > >> > >> -- > >> Christian Theune · c...@flyingcircus.io · +49 345 219401 0 > >> Flying Circus Internet Operations GmbH · https://flyingcircus.io > >> Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland > >> HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian > >> Zagrodnick > >> > >> > >> > >> ___ > >> Openipmi-developer mailing list > >> Openipmi-developer@lists.sourceforge.net > >> https://lists.sourceforge.net/lists/listinfo/openipmi-developer > > Liebe Grüße, > Christian Theune > > -- > Christian Theune · c...@flyingcircus.io · +49 345 219401 0 > Flying Circus Internet Operations GmbH · https://flyingcircus.io > Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland > HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian Zagrodnick > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
Oh, I forgot. You can look at panic_op in /sys/module/ipmi_msghandler/parameters/panic_op -corey On Tue, Feb 28, 2023 at 05:48:07PM +0100, Christian Theune via Openipmi-developer wrote: > Hi, > > > On 28. Feb 2023, at 17:36, Corey Minyard wrote: > > > > On Tue, Feb 28, 2023 at 02:53:12PM +0100, Christian Theune via > > Openipmi-developer wrote: > >> Hi, > >> > >> I’ve been trying to debug the PANIC and OEM string handling and am running > >> out of ideas whether this is a bug or whether something so subtle has > >> changed in my config that I’m just not seeing it. > >> > >> (Note: I’m willing to pay for consulting.) > > > > Probably not necessary. > > Thanks! The offer always stands. If we should ever meet I’m also able to pay > in beverages. ;) > > >> I have machines that we’ve moved from an older setup (Gentoo, (mostly) > >> vanilla kernel 4.19.157) to a newer setup (NixOS, (mostly) vanilla kernel > >> 5.10.159) and I’m now experiencing crashes that seem to be kernel panics > >> but do not get the usual messages in the IPMI SEL. > > > > I just tested on stock 5.10.159 and it worked without issue. Everything > > you have below looks ok. > > > > Can you test by causing a crash with: > > > > echo c >/proc/sysrq-trigger > > > > and see if it works? > > Yeah, already tried that and unfortunately that _doesn’t_ work. > > > It sounds like you are having some type of crash that you would normally > > use the IPMI logs to debug. However, they aren't perfect, the system > > has to stay up long enough to get them into the event log. > > I think they are staying up long enough because a panic triggers the 255 > second bump in the watchdog and only then pass on. However, i’ve also noticed > that the kernel _should_ be rebooting after a panic much faster (and not rely > on the watchdog) and that doesn’t happen either. (Sorry this just popped from > the back of my head). > > > In this situation, getting a serial console (probably through IPMI > > Serial over LAN) and getting the console output on a crash is probably > > your best option. You can use ipmitool for this, or I have a library > > that is able to make connections to serial ports, including through IPMI > > SoL. > > Yup. Been there, too. :) > > Unfortunately we’re currently chasing something that pops up very randomly on > somewhat odd machines and I also have the feeling that it’s systematically > broken right now (as the “echo c” doesn’t work). > > Thanks a lot, > Christian > > -- > Christian Theune · c...@flyingcircus.io · +49 345 219401 0 > Flying Circus Internet Operations GmbH · https://flyingcircus.io > Leipziger Str. 70/71 · 06108 Halle (Saale) · Deutschland > HR Stendal HRB 21169 · Geschäftsführer: Christian Theune, Christian Zagrodnick > > > > ___ > Openipmi-developer mailing list > Openipmi-developer@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openipmi-developer ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] PANIC / OEM strings missing, not sure whether misconfiguration or a bug
On Tue, Feb 28, 2023 at 02:53:12PM +0100, Christian Theune via Openipmi-developer wrote: > Hi, > > I’ve been trying to debug the PANIC and OEM string handling and am running > out of ideas whether this is a bug or whether something so subtle has changed > in my config that I’m just not seeing it. > > (Note: I’m willing to pay for consulting.) Probably not necessary. > > I have machines that we’ve moved from an older setup (Gentoo, (mostly) > vanilla kernel 4.19.157) to a newer setup (NixOS, (mostly) vanilla kernel > 5.10.159) and I’m now experiencing crashes that seem to be kernel panics but > do not get the usual messages in the IPMI SEL. I just tested on stock 5.10.159 and it worked without issue. Everything you have below looks ok. Can you test by causing a crash with: echo c >/proc/sysrq-trigger and see if it works? It sounds like you are having some type of crash that you would normally use the IPMI logs to debug. However, they aren't perfect, the system has to stay up long enough to get them into the event log. In this situation, getting a serial console (probably through IPMI Serial over LAN) and getting the console output on a crash is probably your best option. You can use ipmitool for this, or I have a library that is able to make connections to serial ports, including through IPMI SoL. -corey > > The kernel does include the necessary drivers, the watchdog is active and the > SEL shows the watchdog action. I have reason to think that it’s a panic > because the typical behaviour of the timeout jumping to 255 happens. > > Here’s the IPMI-related config and cmdline from the old kernel where it works: > > BOOT_IMAGE=/kernel-genkernel-x86_64-4.19.157 root=/dev/vgsys/root ro > rootfstype=ext4 dolvm ipmi_watchdog.timeout=60 igb.InterruptThrottleRate=1 > ixgbe.InterruptThrottleRate=1 console=ttyS2,57600 > > # CONFIG_ACPI_IPMI is not set > CONFIG_IPMI_HANDLER=y > CONFIG_IPMI_DMI_DECODE=y > CONFIG_IPMI_PANIC_EVENT=y > CONFIG_IPMI_PANIC_STRING=y > CONFIG_IPMI_DEVICE_INTERFACE=y > CONFIG_IPMI_SI=y > # CONFIG_IPMI_SSIF is not set > CONFIG_IPMI_WATCHDOG=y > CONFIG_IPMI_POWEROFF=y > > On that system (as everything is statically compiled) the lsmod is empty WRT > ipmi and the kernel log shows: > > [4.374757] ipmi device interface > [4.389388] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS > [4.402087] ipmi_si: SMBIOS: io 0xca2 regsize 1 spacing 1 irq 0 > [4.413907] ipmi_si: Adding SMBIOS-specified kcs state machine > [4.425570] ipmi_si IPI0001:00: ipmi_platform: probing via ACPI > [4.437408] ipmi_si IPI0001:00: [io 0x0ca2] regsize 1 spacing 1 irq 0 > [4.450449] ipmi_si dmi-ipmi-si.0: Removing SMBIOS-specified kcs state > machine in favor of ACPI > [4.467818] ipmi_si: Adding ACPI-specified kcs state machine > [4.479139] ipmi_si: Trying ACPI-specified kcs state machine at i/o > address 0xca2, slave address 0x0, irq 0 > [4.567613] ipmi_si IPI0001:00: The BMC does not support clearing the recv > irq bit, compensating, but the BMC needs to be fixed. > [4.617693] ipmi_si IPI0001:00: Found new BMC (man_id: 0x002a7c, prod_id: > 0x0624, dev_id: 0x20) > [4.671871] ipmi_si IPI0001:00: IPMI kcs interface initialized > > And here’s the controller info: > > Device ID : 32 > Device Revision : 1 > Firmware Revision : 2.24 > IPMI Version : 2.0 > Manufacturer ID : 10876 > Manufacturer Name : Supermicro > Product ID: 1572 (0x0624) > Product Name : Unknown (0x624) > Device Available : yes > Provides Device SDRs : no > Additional Device Support : > Sensor Device > SDR Repository Device > SEL Device > FRU Inventory Device > IPMB Event Receiver > IPMB Event Generator > Chassis Device > Aux Firmware Rev Info : > 0x06 > 0x00 > 0x00 > 0x00 > > And here’s the NixOS environment where it doesn’t work: > > BOOT_IMAGE=/kernels/qy42jhicvvqb0p7x2h0i46b2x0f1w74q-linux-5.10.159-bzImage > init=/nix/store/qx33nyr0f60y76yzmbgsikxr60lqzdb3-nixos-system-...-21.05pre-git/init > dolvm ipmi_watchdog.timeout=60 igb.InterruptThrottleRate=1 > ixgbe.InterruptThrottleRate=1 panic=1 boot.panic_on_fail > systemd.journald.forward_to_console=no systemd.log_target=kmsg > console=ttyS1,115200 loglevel=7 > > CONFIG_ACPI_IPMI=m > CONFIG_IPMI_HANDLER=m > CONFIG_IPMI_DMI_DECODE=y > CONFIG_IPMI_PLAT_DATA=y > CONFIG_IPMI_PANIC_EVENT=y > CONFIG_IPMI_PANIC_STRING=y > CONFIG_IPMI_DEVICE_INTERFACE=m > CONFIG_IPMI_SI=m > CONFIG_IPMI_SSIF=m > CONFIG_IPMI_WATCHDOG=m > CONFIG_IPMI_POWEROFF=m > > On the newer system this is what appears in the kernel log: > > [ 22.070935] ipmi device interface > [ 22.086353] systemd-modules-load[572]: Inserted module 'ipmi_watchdog' > [ 22.904717] ipmi_si: IPMI System Interface driver > [ 22.911022] ipmi_si dmi-ipmi-si.0: ipmi_platform: probing via SMBIOS > [ 22.917393]
Re: [Openipmi-developer] [PATCH 1/8] ipmi: ASPEED_BT_IPMI_BMC: select REGMAP_MMIO instead of depending on it
On Sat, Feb 25, 2023 at 09:39:46PM -0800, Randy Dunlap wrote:
> REGMAP is a hidden (not user visible) symbol. Users cannot set it
> directly thru "make *config", so drivers should select it instead of
> depending on it if they need it.
>
> Consistently using "select" or "depends on" can also help reduce
> Kconfig circular dependency issues.
>
> Therefore, change the use of "depends on REGMAP_MMIO" to
> "select REGMAP_MMIO", which will also set REGMAP.
This seems reasonable. I can take it into my tree, or..
Acked-by: Corey Minyard
>
> Fixes: eb994594bc22 ("ipmi: bt-bmc: Use a regmap for register access")
> Signed-off-by: Randy Dunlap
> Cc: Andrew Jeffery
> Cc: Corey Minyard
> Cc: openipmi-developer@lists.sourceforge.net
> Cc: Arnd Bergmann
> Cc: Greg Kroah-Hartman
> ---
> drivers/char/ipmi/Kconfig |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff -- a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -162,7 +162,8 @@ config IPMI_KCS_BMC_SERIO
>
> config ASPEED_BT_IPMI_BMC
> depends on ARCH_ASPEED || COMPILE_TEST
> - depends on REGMAP && REGMAP_MMIO && MFD_SYSCON
> + depends on MFD_SYSCON
> + select REGMAP_MMIO
> tristate "BT IPMI bmc driver"
> help
> Provides a driver for the BT (Block Transfer) IPMI interface
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes for 6.3
The following changes since commit 041fae9c105ae342a4245cf1e0dc56a23fbb9d3c: Merge tag 'f2fs-for-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs (2022-12-14 15:27:57 -0800) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-6.3-1 for you to fetch changes up to befb28f2676a65a5a4cc4626ae224461d8785af6: ipmi: ipmb: Fix the MODULE_PARM_DESC associated to 'retry_time_ms' (2023-02-10 07:38:18 -0600) Small fixes to the SMBus IPMI and IPMB driver Nothing big, cleanups, fixing names, and one small deviation from the specification fixed. Christophe JAILLET (1): ipmi: ipmb: Fix the MODULE_PARM_DESC associated to 'retry_time_ms' Corey Minyard (4): ipmi:ssif: resend_msg() cannot fail ipmi_ssif: Rename idle state and check ipmi:ssif: Remove rtc_us_timer ipmi:ssif: Add a timer between request retries drivers/char/ipmi/ipmi_ipmb.c | 2 +- drivers/char/ipmi/ipmi_ssif.c | 113 -- 2 files changed, 56 insertions(+), 59 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: ipmb: Fix the MODULE_PARM_DESC associated to 'retry_time_ms'
On Sun, Feb 05, 2023 at 11:04:01AM +0100, Christophe JAILLET wrote:
> 'This should be 'retry_time_ms' instead of 'max_retries'.
Oops. Applied to my next tree.
Thanks,
-corey
>
> Fixes: 63c4eb347164 ("ipmi:ipmb: Add initial support for IPMI over IPMB")
> Signed-off-by: Christophe JAILLET
> ---
> drivers/char/ipmi/ipmi_ipmb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
> index 7c1aee5e11b7..3f1c9f1573e7 100644
> --- a/drivers/char/ipmi/ipmi_ipmb.c
> +++ b/drivers/char/ipmi/ipmi_ipmb.c
> @@ -27,7 +27,7 @@ MODULE_PARM_DESC(bmcaddr, "Address to use for BMC.");
>
> static unsigned int retry_time_ms = 250;
> module_param(retry_time_ms, uint, 0644);
> -MODULE_PARM_DESC(max_retries, "Timeout time between retries, in
> milliseconds.");
> +MODULE_PARM_DESC(retry_time_ms, "Timeout time between retries, in
> milliseconds.");
>
> static unsigned int max_retries = 1;
> module_param(max_retries, uint, 0644);
> --
> 2.34.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes and additions for 6.2
The following changes since commit 9abf2313adc1ca1b6180c508c25f22f9395cc780: Linux 6.1-rc1 (2022-10-16 15:36:24 -0700) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-6.2-1 for you to fetch changes up to c6f613e5f35b0e2154d5ca12f0e8e0be0c19be9a: ipmi/watchdog: use strscpy() to instead of strncpy() (2022-12-05 06:50:09 -0600) Small fixes, a new SSIF i2c BMC-side interface This includes a number of small fixes, as usual. It also includes a new driver for doing the i2c (SSIF) interface BMC-side, pretty much completing the BMC side interfaces. Andrew Jeffery (1): ipmi: kcs: Poll OBF briefly to reduce OBE latency Bo Liu (1): ipmi: Fix some kernel-doc warnings Christophe JAILLET (1): ipmi/watchdog: Include when appropriate Corey Minyard (1): ipmi:ssif: Increase the message retry time Dan Carpenter (1): ipmi: fix use after free in _ipmi_destroy_user() Quan Nguyen (3): ipmi: ssif_bmc: Add SSIF BMC driver bindings: ipmi: Add binding for SSIF BMC driver ipmi: ssif_bmc: Use EPOLLIN instead of POLLIN Uwe Kleine-König (1): ipmi: ssif_bmc: Convert to i2c's .probe_new() Zhang Yuchen (3): ipmi: fix long wait in unload when IPMI disconnect ipmi: fix memleak when unload ipmi driver ipmi: fix msg stack when IPMI is disconnected yang.yan...@zte.com.cn (1): ipmi/watchdog: use strscpy() to instead of strncpy() .../devicetree/bindings/ipmi/ssif-bmc.yaml | 38 + drivers/char/ipmi/Kconfig | 10 + drivers/char/ipmi/Makefile | 1 + drivers/char/ipmi/ipmi_kcs_sm.c| 16 +- drivers/char/ipmi/ipmi_msghandler.c| 14 +- drivers/char/ipmi/ipmi_si_intf.c | 27 +- drivers/char/ipmi/ipmi_ssif.c | 2 +- drivers/char/ipmi/ipmi_watchdog.c | 4 +- drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +- drivers/char/ipmi/ssif_bmc.c | 873 + include/uapi/linux/ipmi_ssif_bmc.h | 18 + 11 files changed, 1004 insertions(+), 23 deletions(-) create mode 100644 Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml create mode 100644 drivers/char/ipmi/ssif_bmc.c create mode 100644 include/uapi/linux/ipmi_ssif_bmc.h ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH linux-next] ipmi/watchdog: use strscpy() to instead of strncpy()
On Mon, Dec 05, 2022 at 07:36:40PM +0800, yang.yan...@zte.com.cn wrote: > Xu Panda > > The implementation of strscpy() is more robust and safer. > That's now the recommended way to copy NUL terminated strings. This looks right. Applied, thanks. -corey > > Signed-off-by: Xu Panda > Signed-off-by: Yang Yang > --- > drivers/char/ipmi/ipmi_watchdog.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_watchdog.c > b/drivers/char/ipmi/ipmi_watchdog.c > index 47365150e431..0d4a8dcacfd4 100644 > --- a/drivers/char/ipmi/ipmi_watchdog.c > +++ b/drivers/char/ipmi/ipmi_watchdog.c > @@ -213,8 +213,7 @@ static int set_param_str(const char *val, const struct > kernel_param *kp) > char valcp[16]; > char *s; > > - strncpy(valcp, val, 15); > - valcp[15] = '\0'; > + strscpy(valcp, val, 16); > > s = strstrip(valcp); > > -- > 2.15.2 ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 605/606] ipmi: ssif_bmc: Convert to i2c's .probe_new()
On Fri, Nov 18, 2022 at 11:45:39PM +0100, Uwe Kleine-König wrote:
> From: Uwe Kleine-König
>
> The probe function doesn't make use of the i2c_device_id * parameter so it
> can be trivially converted.
Thanks, queued for next release.
-corey
>
> Signed-off-by: Uwe Kleine-König
> ---
> drivers/char/ipmi/ssif_bmc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index 2d8069386398..caee848261e9 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -797,7 +797,7 @@ static int ssif_bmc_cb(struct i2c_client *client, enum
> i2c_slave_event event, u8
> return ret;
> }
>
> -static int ssif_bmc_probe(struct i2c_client *client, const struct
> i2c_device_id *id)
> +static int ssif_bmc_probe(struct i2c_client *client)
> {
> struct ssif_bmc_ctx *ssif_bmc;
> int ret;
> @@ -860,7 +860,7 @@ static struct i2c_driver ssif_bmc_driver = {
> .name = DEVICE_NAME,
> .of_match_table = ssif_bmc_match,
> },
> - .probe = ssif_bmc_probe,
> + .probe_new = ssif_bmc_probe,
> .remove = ssif_bmc_remove,
> .id_table = ssif_bmc_id,
> };
> --
> 2.38.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: fix use after free in _ipmi_destroy_user()
On Tue, Nov 15, 2022 at 04:17:43PM +0300, Dan Carpenter wrote:
> The intf_free() function frees the "intf" pointer so we cannot
> dereference it again on the next line.
Thanks. I will request a backport for 5.5 and later.
>
> Fixes: cbb79863fc31 ("ipmi: Don't allow device module unload when in use")
> Signed-off-by: Dan Carpenter
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index f6b8ca6df9b5..186f1fee7534 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -1330,6 +1330,7 @@ static void _ipmi_destroy_user(struct ipmi_user *user)
> unsigned longflags;
> struct cmd_rcvr *rcvr;
> struct cmd_rcvr *rcvrs = NULL;
> + struct module*owner;
>
> if (!acquire_ipmi_user(user, )) {
> /*
> @@ -1392,8 +1393,9 @@ static void _ipmi_destroy_user(struct ipmi_user *user)
> kfree(rcvr);
> }
>
> + owner = intf->owner;
> kref_put(>refcount, intf_free);
> - module_put(intf->owner);
> + module_put(owner);
> }
>
> int ipmi_destroy_user(struct ipmi_user *user)
> --
> 2.35.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi/watchdog: Include when appropriate
On Sat, Nov 05, 2022 at 12:16:54PM +0100, Christophe JAILLET wrote: > The kstrto() functions have been moved from kernel.h to > kstrtox.h. > > So, in order to eventually remove from , > include the latter directly in the appropriate files. This is in my queue. Thanks. -corey > > Signed-off-by: Christophe JAILLET > --- > drivers/char/ipmi/ipmi_watchdog.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/char/ipmi/ipmi_watchdog.c > b/drivers/char/ipmi/ipmi_watchdog.c > index 5b4e677929ca..47365150e431 100644 > --- a/drivers/char/ipmi/ipmi_watchdog.c > +++ b/drivers/char/ipmi/ipmi_watchdog.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > #include > #include > #include > -- > 2.34.1 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [RFC][PATCH v2 10/31] timers: ipmi: Use del_timer_shutdown() before freeing timer
On Thu, Oct 27, 2022 at 10:20:15AM -0500, Corey Minyard wrote:
> On Thu, Oct 27, 2022 at 11:05:35AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)"
> >
> > Before a timer is freed, del_timer_shutdown() must be called.
>
> Thanks, this is in my queue, or:
>
> Acked-by: Corey Minyard
>
> if you prefer that.
Well, del_timer_shutdown() isn't there yet, so I guess the Ack is what
you need.
-corey
>
> -corey
>
> >
> > Link:
> > https://lore.kernel.org/all/20220407161745.7d675...@gandalf.local.home/
> >
> > Cc: Corey Minyard
> > Cc: openipmi-developer@lists.sourceforge.net
> > Signed-off-by: Steven Rostedt (Google)
> > ---
> > drivers/char/ipmi/ipmi_msghandler.c | 2 +-
> > drivers/char/ipmi/ipmi_ssif.c | 4 ++--
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> > b/drivers/char/ipmi/ipmi_msghandler.c
> > index 49a1707693c9..b577f66f3ca6 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -5540,7 +5540,7 @@ static void __exit cleanup_ipmi(void)
> > * here.
> > */
> > atomic_set(_operation, 1);
> > - del_timer_sync(_timer);
> > + del_timer_shutdown(_timer);
> >
> > initialized = false;
> >
> > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > index e1072809fe31..bb4df879a5ab 100644
> > --- a/drivers/char/ipmi/ipmi_ssif.c
> > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > @@ -1273,8 +1273,8 @@ static void shutdown_ssif(void *send_info)
> > schedule_timeout(1);
> >
> > ssif_info->stopping = true;
> > - del_timer_sync(_info->watch_timer);
> > - del_timer_sync(_info->retry_timer);
> > + del_timer_shutdown(_info->watch_timer);
> > + del_timer_shutdown(_info->retry_timer);
> > if (ssif_info->thread) {
> > complete(_info->wake_thread);
> > kthread_stop(ssif_info->thread);
> > --
> > 2.35.1
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [RFC][PATCH v2 10/31] timers: ipmi: Use del_timer_shutdown() before freeing timer
On Thu, Oct 27, 2022 at 11:05:35AM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)"
>
> Before a timer is freed, del_timer_shutdown() must be called.
Thanks, this is in my queue, or:
Acked-by: Corey Minyard
if you prefer that.
-corey
>
> Link: https://lore.kernel.org/all/20220407161745.7d675...@gandalf.local.home/
>
> Cc: Corey Minyard
> Cc: openipmi-developer@lists.sourceforge.net
> Signed-off-by: Steven Rostedt (Google)
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 2 +-
> drivers/char/ipmi/ipmi_ssif.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 49a1707693c9..b577f66f3ca6 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -5540,7 +5540,7 @@ static void __exit cleanup_ipmi(void)
>* here.
>*/
> atomic_set(_operation, 1);
> - del_timer_sync(_timer);
> + del_timer_shutdown(_timer);
>
> initialized = false;
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index e1072809fe31..bb4df879a5ab 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1273,8 +1273,8 @@ static void shutdown_ssif(void *send_info)
> schedule_timeout(1);
>
> ssif_info->stopping = true;
> - del_timer_sync(_info->watch_timer);
> - del_timer_sync(_info->retry_timer);
> + del_timer_shutdown(_info->watch_timer);
> + del_timer_shutdown(_info->retry_timer);
> if (ssif_info->thread) {
> complete(_info->wake_thread);
> kthread_stop(ssif_info->thread);
> --
> 2.35.1
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Fix some kernel-doc warnings
On Tue, Oct 25, 2022 at 02:04:36AM -0400, Bo Liu wrote:
> The current code provokes some kernel-doc warnings:
> drivers/char/ipmi/ipmi_msghandler.c:618: warning: This comment starts
> with '/**', but isn't a kernel-doc comment. Refer
> Documentation/doc-guide/kernel-doc.rst
Thanks, this is in my for-next tree.
-corey
>
> Signed-off-by: Bo Liu
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index d5ee52be176d..f6b8ca6df9b5 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -614,7 +614,7 @@ static int __ipmi_bmc_register(struct ipmi_smi *intf,
> static int __scan_channels(struct ipmi_smi *intf, struct ipmi_device_id *id);
>
>
> -/**
> +/*
> * The driver model view of the IPMI messaging driver.
> */
> static struct platform_driver ipmidriver = {
> --
> 2.27.0
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: ssif_bmc: Use EPOLLIN instead of POLLIN
On Mon, Oct 24, 2022 at 02:59:56PM +0700, Quan Nguyen wrote:
> This fixes the following sparse warning:
> sparse warnings: (new ones prefixed by >>)
> >> drivers/char/ipmi/ssif_bmc.c:254:22: sparse: sparse: invalid assignment: |=
> >> drivers/char/ipmi/ssif_bmc.c:254:22: sparse:left side has type
> >> restricted __poll_t
> >> drivers/char/ipmi/ssif_bmc.c:254:22: sparse:right side has type int
Thanks, you beat me to tracing this down. It's in my for-next queue.
-corey
>
> Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
> Reported-by: kernel test robot
> Link: https://lore.kernel.org/all/202210181103.ontd9trt-...@intel.com/
> Signed-off-by: Quan Nguyen
> ---
> drivers/char/ipmi/ssif_bmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index a7bb4b99000e..2d8069386398 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -251,7 +251,7 @@ static __poll_t ssif_bmc_poll(struct file *file,
> poll_table *wait)
> spin_lock_irq(_bmc->lock);
> /* The request is available, userspace application can get the request
> */
> if (ssif_bmc->request_available)
> - mask |= POLLIN;
> + mask |= EPOLLIN;
>
> spin_unlock_irq(_bmc->lock);
>
> --
> 2.35.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2 1/3] ipmi: fix msg stack when IPMI is disconnected
On Sun, Oct 09, 2022 at 05:18:09PM +0800, Zhang Yuchen wrote:
> If you continue to access and send messages at a high frequency (once
> every 55s) when the IPMI is disconnected, messages will accumulate in
> intf->[hp_]xmit_msg. If it lasts long enough, it takes up a lot of
> memory.
This is queued for 6.2. Thanks. I already had the other two patches
queued.
-corey
>
> The reason is that if IPMI is disconnected, each message will be set to
> IDLE after it returns to HOSED through IDLE->ERROR0->HOSED. The next
> message goes through the same process when it comes in. This process
> needs to wait for IBF_TIMEOUT * (MAX_ERROR_RETRIES + 1) = 55s.
>
> Each message takes 55S to destroy. This results in a continuous increase
> in memory.
>
> I find that if I wait 5 seconds after the first message fails, the
> status changes to ERROR0 in smi_timeout(). The next message will return
> the error code IPMI_NOT_IN_MY_STATE_ERR directly without wait.
>
> This is more in line with our needs.
>
> So instead of setting each message state to IDLE after it reaches the
> state HOSED, set state to ERROR0.
>
> After testing, the problem has been solved, no matter how many
> consecutive sends, will not cause continuous memory growth. It also
> returns to normal immediately after the IPMI is restored.
>
> In addition, the HOSED state should also count as invalid. So the HOSED
> is removed from the invalid judgment in start_kcs_transaction().
>
> The verification operations are as follows:
>
> 1. Use BPF to record the ipmi_alloc/free_smi_msg().
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc
> %p\n",retval);} kprobe:free_recv_msg {printf("free %p\n",arg0)}'
>
> 2. Exec `date; time for x in $(seq 1 2); do ipmitool mc info; done`.
> 3. Record the output of `time` and when free all msgs.
>
> Before:
>
> `time` takes 120s, This is because `ipmitool mc info` send 4 msgs and
> waits only 15 seconds for each message. Last msg is free after 440s.
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc
> %p\n",retval);} kprobe:free_recv_msg {printf("free %p\n",arg0)}'
> Oct 05 11:40:55 Attaching 2 probes...
> Oct 05 11:41:12 alloc 0x9558a05f0c00
> Oct 05 11:41:27 alloc 0x9558a05f1a00
> Oct 05 11:41:42 alloc 0x9558a05f
> Oct 05 11:41:57 alloc 0x9558a05f1400
> Oct 05 11:42:07 free 0x9558a05f0c00
> Oct 05 11:42:07 alloc 0x9558a05f7000
> Oct 05 11:42:22 alloc 0x9558a05f2a00
> Oct 05 11:42:37 alloc 0x9558a05f5a00
> Oct 05 11:42:52 alloc 0x9558a05f3a00
> Oct 05 11:43:02 free 0x9558a05f1a00
> Oct 05 11:43:57 free 0x9558a05f
> Oct 05 11:44:52 free 0x9558a05f1400
> Oct 05 11:45:47 free 0x9558a05f7000
> Oct 05 11:46:42 free 0x9558a05f2a00
> Oct 05 11:47:37 free 0x9558a05f5a00
> Oct 05 11:48:32 free 0x9558a05f3a00
>
> $ root@dc00-pb003-t106-n078:~# date;time for x in $(seq 1 2); do
> ipmitool mc info; done
>
> Wed Oct 5 11:41:12 CST 2022
> No data available
> Get Device ID command failed
> No data available
> No data available
> No valid response received
> Get Device ID command failed: Unspecified error
> No data available
> Get Device ID command failed
> No data available
> No data available
> No valid response received
> No data available
> Get Device ID command failed
>
> real1m55.052s
> user0m0.001s
> sys0m0.001s
>
> After:
>
> `time` takes 55s, all msgs is returned and free after 55s.
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc
> %p\n",retval);} kprobe:free_recv_msg {printf("free %p\n",arg0)}'
>
> Oct 07 16:30:35 Attaching 2 probes...
> Oct 07 16:30:45 alloc 0x955943aa9800
> Oct 07 16:31:00 alloc 0x955943aacc00
> Oct 07 16:31:15 alloc 0x955943aa8c00
> Oct 07 16:31:30 alloc 0x955943aaf600
> Oct 07 16:31:40 free 0x955943aa9800
> Oct 07 16:31:40 free 0x955943aacc00
> Oct 07 16:31:40 free 0x955943aa8c00
> Oct 07 16:31:40 free 0x955943aaf600
> Oct 07 16:31:40 alloc 0x9558ec8f7e00
> Oct 07 16:31:40 free 0x9558ec8f7e00
> Oct 07 16:31:40 alloc 0x9558ec8f7800
> Oct 07 16:31:40 free 0x9558ec8f7800
> Oct 07 16:31:40 alloc 0x9558ec8f7e00
> Oct 07 16:31:40 free 0x9558ec8f7e00
> Oct 07 16:31:40 alloc 0x9558ec8f7800
> Oct 07 16:31:40 free 0x9558ec8f7800
>
> root@dc00-pb003-t106-n078:~# date;time for x in $(seq 1 2); do
> ipmitool mc info; done
> Fri Oct 7 16:30:45 CST 2022
> No data available
> Get Device ID command failed
> No data available
> No data available
> No valid response received
> Get Device ID command failed: Unspecified error
> Get Device ID command failed: 0xd5 Command not supported in present state
> Get Device ID command failed: Command not supported in present state
>
> real0m55.038s
> user0m0.001s
> sys0m0.001s
>
Re: [Openipmi-developer] [PATCH 1/3] ipmi: fix msg stack when IPMI is disconnected
On Sat, Oct 08, 2022 at 09:36:16AM +0800, Yuchen Zhang wrote:
> > Also, the following is in start_kcs_transaction():
> >
> > if ((kcs->state != KCS_IDLE) && (kcs->state != KCS_HOSED)) {
> > dev_warn(kcs->io->dev, "KCS in invalid state %d\n", kcs->state);
> > return IPMI_NOT_IN_MY_STATE_ERR;
> > }
> >
> > You probably need to remove the (kcs->state != KCS_HOSED) part of this
> > now. Would you agree?
> >
> > -corey
> >
> I agree. KCS_HOSED state should be an invalid state.
Can you make this change, run a quick test, and re-submit this one
patch? With that, I can include this.
Thanks,
-corey
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 3/3] ipmi: fix memleak when unload ipmi driver
On Fri, Oct 07, 2022 at 05:26:17PM +0800, Zhang Yuchen wrote:
> After the IPMI disconnect problem, the memory kept rising and we tried
> to unload the driver to free the memory. However, only part of the
> free memory is recovered after the driver is uninstalled. Using
> ebpf to hook free functions, we find that neither ipmi_user nor
> ipmi_smi_msg is free, only ipmi_recv_msg is free.
>
> We find that the deliver_smi_err_response call in clean_smi_msgs does
> the destroy processing on each message from the xmit_msg queue without
> checking the return value and free ipmi_smi_msg.
>
> deliver_smi_err_response is called only at this location. Adding the
> free handling has no effect.
>
> To verify, try using ebpf to trace the free function.
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc rcv
> %p\n",retval);} kprobe:free_recv_msg {printf("free recv %p\n",
> arg0)} kretprobe:ipmi_alloc_smi_msg {printf("alloc smi %p\n",
> retval);} kprobe:free_smi_msg {printf("free smi %p\n",arg0)}'
>
> Signed-off-by: Zhang Yuchen
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index c8a3b208f923..7a7534046b5b 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -3710,12 +3710,15 @@ static void deliver_smi_err_response(struct ipmi_smi
> *intf,
>struct ipmi_smi_msg *msg,
>unsigned char err)
> {
> + int rv;
> msg->rsp[0] = msg->data[0] | 4;
> msg->rsp[1] = msg->data[1];
> msg->rsp[2] = err;
> msg->rsp_size = 3;
> /* It's an error, so it will never requeue, no need to check return. */
The above comment is wrong, but yes, this is correct. I'll queue this
and remove the comment.
Thanks,
-corey
> - handle_one_recv_msg(intf, msg);
> + rv = handle_one_recv_msg(intf, msg);
> + if (rv == 0)
> + ipmi_free_smi_msg(msg);
> }
>
> static void cleanup_smi_msgs(struct ipmi_smi *intf)
> --
> 2.30.2
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 2/3] ipmi: fix long wait in unload when IPMI disconnect
On Fri, Oct 07, 2022 at 05:26:16PM +0800, Zhang Yuchen wrote:
> When fixing the problem mentioned in PATCH1, we also found
> the following problem:
>
> If the IPMI is disconnected and in the sending process, the
> uninstallation driver will be stuck for a long time.
>
> The main problem is that uninstalling the driver waits for curr_msg to
> be sent or HOSED. After stopping tasklet, the only place to trigger the
> timeout mechanism is the circular poll in shutdown_smi.
>
> The poll function delays 10us and calls smi_event_handler(smi_info,10).
> Smi_event_handler deducts 10us from kcs->ibf_timeout.
>
> But the poll func is followed by schedule_timeout_uninterruptible(1).
> The time consumed here is not counted in kcs->ibf_timeout.
>
> So when 10us is deducted from kcs->ibf_timeout, at least 1 jiffies has
> actually passed. The waiting time has increased by more than a
> hundredfold.
>
> Now instead of calling poll(). call smi_event_handler() directly and
> calculate the elapsed time.
Yes, you are correct.
I've included this for 6.2, and added:
Cc: sta...@vger.kernel.org
I would like it to soak for a bit.
-corey
>
> For verification, you can directly use ebpf to check the kcs->
> ibf_timeout for each call to kcs_event() when IPMI is disconnected.
> Decrement at normal rate before unloading. The decrement rate becomes
> very slow after unloading.
>
> $ bpftrace -e 'kprobe:kcs_event {printf("kcs->ibftimeout : %d\n",
> *(arg0+584));}'
>
> Signed-off-by: Zhang Yuchen
> ---
> drivers/char/ipmi/ipmi_si_intf.c | 27 +++
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c
> b/drivers/char/ipmi/ipmi_si_intf.c
> index 6e357ad76f2e..abddd7e43a9a 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2153,6 +2153,20 @@ static int __init init_ipmi_si(void)
> }
> module_init(init_ipmi_si);
>
> +static void wait_msg_processed(struct smi_info *smi_info)
> +{
> + unsigned long jiffies_now;
> + long time_diff;
> +
> + while (smi_info->curr_msg || (smi_info->si_state != SI_NORMAL)) {
> + jiffies_now = jiffies;
> + time_diff = (((long)jiffies_now -
> (long)smi_info->last_timeout_jiffies)
> + * SI_USEC_PER_JIFFY);
> + smi_event_handler(smi_info, time_diff);
> + schedule_timeout_uninterruptible(1);
> + }
> +}
> +
> static void shutdown_smi(void *send_info)
> {
> struct smi_info *smi_info = send_info;
> @@ -2187,16 +2201,13 @@ static void shutdown_smi(void *send_info)
>* in the BMC. Note that timers and CPU interrupts are off,
>* so no need for locks.
>*/
> - while (smi_info->curr_msg || (smi_info->si_state != SI_NORMAL)) {
> - poll(smi_info);
> - schedule_timeout_uninterruptible(1);
> - }
> + wait_msg_processed(smi_info);
> +
> if (smi_info->handlers)
> disable_si_irq(smi_info);
> - while (smi_info->curr_msg || (smi_info->si_state != SI_NORMAL)) {
> - poll(smi_info);
> - schedule_timeout_uninterruptible(1);
> - }
> +
> + wait_msg_processed(smi_info);
> +
> if (smi_info->handlers)
> smi_info->handlers->cleanup(smi_info->si_sm);
>
> --
> 2.30.2
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 1/3] ipmi: fix msg stack when IPMI is disconnected
On Fri, Oct 07, 2022 at 05:26:15PM +0800, Zhang Yuchen wrote:
> If you continue to access and send messages at a high frequency (once
> every 55s) when the IPMI is disconnected, messages will accumulate in
> intf->[hp_]xmit_msg. If it lasts long enough, it takes up a lot of
> memory.
The IPMI driver really wasn't designed to handle this sort of thing. If
there is a BMC there, it should be there except when it's rebooting,
which should only take a few seconds. Which is what this is all
designed to handle.
>
> The reason is that if IPMI is disconnected, each message will be set to
> IDLE after it returns to HOSED through IDLE->ERROR0->HOSED. The next
> message goes through the same process when it comes in. This process
> needs to wait for IBF_TIMEOUT * (MAX_ERROR_RETRIES + 1) = 55s.
>
> Each message takes 55S to destroy. This results in a continuous increase
> in memory.
>
> I find that if I wait 5 seconds after the first message fails, the
> status changes to ERROR0 in smi_timeout(). The next message will return
> the error code IPMI_NOT_IN_MY_STATE_ERR directly without wait.
So basically, you will stay in error state until the BMC recovers. The
KCS state machine will reject messages until the state machine detects
that the BMC is working again. I think this is ok.
Have you tested that if the BMC comes back that the driver recovers and
works? Looking at the code it seems to be the case, but can you test to
be sure, if you have not already?
Also, the following is in start_kcs_transaction():
if ((kcs->state != KCS_IDLE) && (kcs->state != KCS_HOSED)) {
dev_warn(kcs->io->dev, "KCS in invalid state %d\n", kcs->state);
return IPMI_NOT_IN_MY_STATE_ERR;
}
You probably need to remove the (kcs->state != KCS_HOSED) part of this
now. Would you agree?
-corey
>
> This is more in line with our needs.
>
> So instead of setting each message state to IDLE after it reaches the
> state HOSED, set state to ERROR0.
>
> After testing, the problem has been solved, no matter how many
> consecutive sends, will not cause continuous memory growth. It also
> returns to normal immediately after the IPMI is restored.
>
> The verification operations are as follows:
>
> 1. Use BPF to record the ipmi_alloc/free_smi_msg().
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc
> %p\n",retval);} kprobe:free_recv_msg {printf("free %p\n",arg0)}'
>
> 2. Exec `date; time for x in $(seq 1 2); do ipmitool mc info; done`.
> 3. Record the output of `time` and when free all msgs.
>
> Before:
>
> `time` takes 120s, This is because `ipmitool mc info` send 4 msgs and
> waits only 15 seconds for each message. Last msg is free after 440s.
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc
> %p\n",retval);} kprobe:free_recv_msg {printf("free %p\n",arg0)}'
> Oct 05 11:40:55 Attaching 2 probes...
> Oct 05 11:41:12 alloc 0x9558a05f0c00
> Oct 05 11:41:27 alloc 0x9558a05f1a00
> Oct 05 11:41:42 alloc 0x9558a05f
> Oct 05 11:41:57 alloc 0x9558a05f1400
> Oct 05 11:42:07 free 0x9558a05f0c00
> Oct 05 11:42:07 alloc 0x9558a05f7000
> Oct 05 11:42:22 alloc 0x9558a05f2a00
> Oct 05 11:42:37 alloc 0x9558a05f5a00
> Oct 05 11:42:52 alloc 0x9558a05f3a00
> Oct 05 11:43:02 free 0x9558a05f1a00
> Oct 05 11:43:57 free 0x9558a05f
> Oct 05 11:44:52 free 0x9558a05f1400
> Oct 05 11:45:47 free 0x9558a05f7000
> Oct 05 11:46:42 free 0x9558a05f2a00
> Oct 05 11:47:37 free 0x9558a05f5a00
> Oct 05 11:48:32 free 0x9558a05f3a00
>
> $ root@dc00-pb003-t106-n078:~# date;time for x in $(seq 1 2); do
> ipmitool mc info; done
>
> Wed Oct 5 11:41:12 CST 2022
> No data available
> Get Device ID command failed
> No data available
> No data available
> No valid response received
> Get Device ID command failed: Unspecified error
> No data available
> Get Device ID command failed
> No data available
> No data available
> No valid response received
> No data available
> Get Device ID command failed
>
> real1m55.052s
> user0m0.001s
> sys0m0.001s
>
> After:
>
> `time` takes 55s, all msgs is returned and free after 55s.
>
> $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc
> %p\n",retval);} kprobe:free_recv_msg {printf("free %p\n",arg0)}'
>
> Oct 07 16:30:35 Attaching 2 probes...
> Oct 07 16:30:45 alloc 0x955943aa9800
> Oct 07 16:31:00 alloc 0x955943aacc00
> Oct 07 16:31:15 alloc 0x955943aa8c00
> Oct 07 16:31:30 alloc 0x955943aaf600
> Oct 07 16:31:40 free 0x955943aa9800
> Oct 07 16:31:40 free 0x955943aacc00
> Oct 07 16:31:40 free 0x955943aa8c00
> Oct 07 16:31:40 free 0x955943aaf600
> Oct 07 16:31:40 alloc 0x9558ec8f7e00
> Oct 07 16:31:40 free 0x9558ec8f7e00
> Oct 07 16:31:40 alloc 0x9558ec8f7800
> Oct 07 16:31:40 free
Re: [Openipmi-developer] [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency
On Thu, Oct 06, 2022 at 01:36:51PM +1030, Andrew Jeffery wrote:
>
>
> On Thu, 6 Oct 2022, at 10:20, Joel Stanley wrote:
> > On Fri, 12 Aug 2022 at 14:48, Andrew Jeffery wrote:
> >>
> >> The ASPEED KCS devices don't provide a BMC-side interrupt for the host
> >> reading the output data register (ODR). The act of the host reading ODR
> >> clears the output buffer full (OBF) flag in the status register (STR),
> >> informing the BMC it can transmit a subsequent byte.
> >>
> >> On the BMC side the KCS client must enable the OBE event *and* perform a
> >> subsequent read of STR anyway to avoid races - the polling provides a
> >> window for the host to read ODR if data was freshly written while
> >> minimising BMC-side latency.
> >>
> >
> > Fixes...?
>
> Is it a fix though? It's definitely an *improvement* in behaviour, but
> the existing behaviour also wasn't *incorrect*, just kinda unfortunate
> under certain timings? Dunno. I'm probably splitting hairs.
>
> In any case, if we do want a fixes line:
>
> Fixes: 28651e6c4237 ("ipmi: kcs_bmc: Allow clients to control KCS IRQ state")
I added the Fixes and Joel's review.
Thanks,
-corey
>
> >
> >> Signed-off-by: Andrew Jeffery
> >
> > Reviewed-by: Joel Stanley
>
> Thanks!
>
> >
> >> ---
> >> drivers/char/ipmi/kcs_bmc_aspeed.c | 24 +---
> >> 1 file changed, 21 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
> >> b/drivers/char/ipmi/kcs_bmc_aspeed.c
> >> index cdc88cde1e9a..417e5a3ccfae 100644
> >> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> >> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> >> @@ -399,13 +399,31 @@ static void aspeed_kcs_check_obe(struct timer_list
> >> *timer)
> >> 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);
> >> + int rc;
> >> + u8 str;
> >
> > str is status, it would be good to spell that out in full.
>
> I guess if it trips enough people up we can rename it later.
>
> >
> >>
> >> /* 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(>obe.timer, jiffies +
> >> OBE_POLL_PERIOD);
> >> - else
> >> + if (KCS_BMC_EVENT_TYPE_OBE & state) {
> >> + /*
> >> +* Given we don't have an OBE IRQ, delay by
> >> polling briefly to see if we can
> >> +* observe such an event before returning to the
> >> caller. This is not
> >> +* incorrect because OBF may have already become
> >> clear before enabling the
> >> +* IRQ if we had one, under which circumstance no
> >> event will be propagated
> >> +* anyway.
> >> +*
> >> +* The onus is on the client to perform a
> >> race-free check that it hasn't
> >> +* missed the event.
> >> +*/
> >> + rc = read_poll_timeout_atomic(aspeed_kcs_inb, str,
> >> + !(str &
> >> KCS_BMC_STR_OBF), 1, 100, false,
> >> + >kcs_bmc,
> >> priv->kcs_bmc.ioreg.str);
> >> + /* Time for the slow path? */
> >
> > The mod_timer is the slow path? The question mark threw me.
>
> Yeah, mod_timer() is the slow path; read_poll_timeout_atomic() is the
> fast path and we've exhausted the time we're willing to wait there if
> we get -ETIMEDOUT.
>
> The comment was intended as a description for the question posed by the
> condition. It made sense in my head but maybe it's confusing more than
> it is enlightening?
>
> Andrew
>
> >
> >> + if (rc == -ETIMEDOUT)
> >> + mod_timer(>obe.timer, jiffies +
> >> OBE_POLL_PERIOD);
> >> + } else {
> >> del_timer(>obe.timer);
> >> + }
> >> }
> >>
> >> if (mask & KCS_BMC_EVENT_TYPE_IBF) {
> >> --
> >> 2.34.1
> >>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs: Poll OBF briefly to reduce OBE latency
On Thu, Oct 06, 2022 at 09:42:57AM +1030, Andrew Jeffery wrote: > Hi Corey, > > On Sat, 13 Aug 2022, at 00:17, Andrew Jeffery wrote: > > The ASPEED KCS devices don't provide a BMC-side interrupt for the host > > reading the output data register (ODR). The act of the host reading ODR > > clears the output buffer full (OBF) flag in the status register (STR), > > informing the BMC it can transmit a subsequent byte. > > > > On the BMC side the KCS client must enable the OBE event *and* perform a > > subsequent read of STR anyway to avoid races - the polling provides a > > window for the host to read ODR if data was freshly written while > > minimising BMC-side latency. > > Just wondering whether you're happy to pick this one up? I haven't seen > it hit the IPMI tree yet. Sorry. It's in my tree for 6.2 right now. I can't push it up to for-next until 6.1-rc1 comes out. -corey > > Cheers, > > Andrew ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v10 0/3] Add SSIF BMC driver
On Tue, Oct 04, 2022 at 04:31:03PM +0700, Quan Nguyen wrote: > This series add support the SSIF BMC driver which is to perform in-band > IPMI communication with their host in management (BMC) side. > > SSIF BMC driver in this series is tested with Aspeed AST2500 and AST2600 I have applied the two IPMI patches to the IPMI tree for 6.2. Thanks for sticking with this. -corey > > Discussion for v9: > https://lore.kernel.org/lkml/20220929080326.752907-1-q...@os.amperecomputing.com/ > > v10: > + Issuing RxCmdLast command for all errnos [Wolfram] > > v9: > + Fix dependence with I2C subsystem[Randy] > + Update missing Reviewed-by tag from v7 [Rob] > + Remove useless error handling path [CJ] > + Update comment for SSIF_ABORTING state [CJ] > + Fix "unknown type name --u8" [kernel test robot] > + Update commit message and add comment to explain > the effect of issuing RxCmdLast when Slave busy [Quan] > > v8: > + Dropped ssif_bmc.h file and move its content to ssif_bmc.c [Corey] > + Add struct ipmi_ssif_msg to include/uapi/linux/ipmi_ssif_bmc.h > header file[Corey] > + Use unsigned int for len field in struct ipmi_ssif_msg [Corey] > + Avoid using packed structure [Corey] > + Add comment to clarify the logic flow[Corey] > + Fix multipart read end with len=0 issue [Corey] > + Refactor code handle the too big request message [Corey] > + Fix code indentation issue [Corey] > + Clean buffer before receiving request to avoid garbage[Quan] > + Fix the license to SPDX-License-Identifier: GPL-2.0-only [Quan] > > v7: > + Remove unnecessary del_timer() in response_timeout() [Corey] > + Change compatible string from "ampere,ssif-bmc" to "ssif-bmc" [Jae] > + Dropped the use of ssif_msg_len() macro, use the len directly [Quan] > + Solve possible issue if both response timer and ssif_bmc_write() > occurred at the same time [Corey] > + Fix wrong return type of ssif_bmc_poll() [kernel robot test] > + Refactor and introduce ssif_part_buffer struct to replace the > response_buf to manage each send/receive part of ssif [Quan] > + Change SSIF_BAD_SMBUS state to SSIF_ABORTING state [Corey] > + Support abort feature to skip the current bad request/response and > wait until next new request[Corey] > + Refactor the PEC calculation to avoid the re-calculate the PEC on > each I2C_SLAVE_WRITE_RECEIVED event [Quan] > + Fix the use of error-proned idx [Corey] > + Defer the test for valid SMBus command until the read/write part > is determined [Quan] > + Change/split unsupported_smbus_cmd() to > supported_[write|read]_cmd() [Corey] > + Abort the request if somehow its size exceeded 255 bytes [Quan] > > v6: > + Drop the use of slave_enable() [Wolfram] > + Make i2c-aspeed to issue RxCmdLast command on all > I2C_SLAVE_WRITE_REQUESTED event to assert NAK when slave busy [Quan] > + Make i2c slave to return -EBUSY when it's busy[Quan] > + Drop the aborting feature as return Completion Code 0xFF may stop > host to retry and make ipmi_ssif.so fails to load [Quan] > + Add timer to recover slave from busy state when no response [Quan] > + Clean request/response buffer appropriately [Quan] > + Add some minor change on error and warning messages [Quan] > > v5: > + Correct the patches order to fix the bisect issue found by > kernel build robot > > v4: > + Fix recursive spinlock [Graeme] > + Send response with Completion code 0xFF when aborting [Quan] > + Fix warning with dt_binding_check [Rob] > + Change aspeed-ssif-bmc.yaml to ssif-bmc.yaml [Quan] > + Added bounding check on SMBus writes and the whole request [Dan] > + Moved buffer to end of struct ssif_bmc_ctx to avoid context > corruption if somehow buffer is written past the end [Dan] > + Return -EINVAL if userspace buffer too small, don't > silence truncate [Corey, Joel] > + Not necessary to check NONBLOCK in lock [Corey] > + Enforce one user at a time[Joel] > + Reject write with invalid response length from userspace [Corey] > + Add state machines for better ssif bmc state
Re: [Openipmi-developer] [PATCH] ipmi: Remove unused struct watcher_entry
On Tue, Sep 27, 2022 at 01:38:14PM +, Yuan Can wrote:
> After commit e86ee2d44b44("ipmi: Rework locking and shutdown for hot remove"),
> no one use struct watcher_entry, so remove it.
Thanks, got it.
-corey
>
> Signed-off-by: Yuan Can
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index c8a3b208f923..49a1707693c9 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -736,12 +736,6 @@ static void intf_free(struct kref *ref)
> kfree(intf);
> }
>
> -struct watcher_entry {
> - int intf_num;
> - struct ipmi_smi *intf;
> - struct list_head link;
> -};
> -
> int ipmi_smi_watcher_register(struct ipmi_smi_watcher *watcher)
> {
> struct ipmi_smi *intf;
> --
> 2.17.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs: aspeed: Update port address comments
On Fri, Sep 23, 2022 at 12:38:08AM +, ChiaWei Wang wrote: > > From: Corey Minyard On Behalf Of Corey Minyard > > Sent: Friday, September 23, 2022 8:21 AM > > > > On Fri, Sep 23, 2022 at 12:08:07AM +, ChiaWei Wang wrote: > > > Hi Corey, > > > > > > > From: Corey Minyard On Behalf Of Corey > > Minyard > > > > Sent: Friday, September 23, 2022 2:58 AM > > > > > > > > On Tue, Sep 20, 2022 at 10:03:33AM +0800, Chia-Wei Wang wrote: > > > > > Remove AST_usrGuide_KCS.pdf as it is no longer maintained. > > > > > > > > Even if it's no longer maintained, is it useful? It seems better to > > > > leave in useful documentation unless it has been replaced with something > > else. > > > > > > This document has no permeant public link to access. > > > Aspeed has dropped this file but we keep receiving customer request asking > > for this document. > > > The most important part regarding KCS port rule is still kept in the > > > updated > > comment. > > > > I mean, if you have code that is implementing what is documented, why did > > you remove the document? I don't understand why you would retire > > documentation that people still want to use. > > > > I could put it on the IPMI sourceforge or github page as a historical > > document. > > This document is a personal note of one of our former employee. > It is known to only the owner and his supporting customers. > Most of us has no access to this document. > In addition, after chip revision, certain information is not guaranteed > anymore. Ok, I'll take the patch. Thanks. -corey > > Regards, > Chiawei ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs: aspeed: Update port address comments
On Fri, Sep 23, 2022 at 12:08:07AM +, ChiaWei Wang wrote:
> Hi Corey,
>
> > From: Corey Minyard On Behalf Of Corey Minyard
> > Sent: Friday, September 23, 2022 2:58 AM
> >
> > On Tue, Sep 20, 2022 at 10:03:33AM +0800, Chia-Wei Wang wrote:
> > > Remove AST_usrGuide_KCS.pdf as it is no longer maintained.
> >
> > Even if it's no longer maintained, is it useful? It seems better to leave
> > in
> > useful documentation unless it has been replaced with something else.
>
> This document has no permeant public link to access.
> Aspeed has dropped this file but we keep receiving customer request asking
> for this document.
> The most important part regarding KCS port rule is still kept in the updated
> comment.
I mean, if you have code that is implementing what is documented, why
did you remove the document? I don't understand why you would retire
documentation that people still want to use.
I could put it on the IPMI sourceforge or github page as a historical
document.
-corey
>
> Regards,
> Chiawei
>
> >
> >
> > >
> > > Add more descriptions as the driver now supports the I/O address
> > > configurations for both the KCS Data and Cmd/Status interface
> > > registers.
> > >
> > > Signed-off-by: Chia-Wei Wang
> > > ---
> > > drivers/char/ipmi/kcs_bmc_aspeed.c | 29 ++---
> > > 1 file changed, 18 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
> > > b/drivers/char/ipmi/kcs_bmc_aspeed.c
> > > index cdc88cde1e9a..19c32bf50e0e 100644
> > > --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> > > +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> > > @@ -207,17 +207,24 @@ static void aspeed_kcs_updateb(struct
> > > kcs_bmc_device *kcs_bmc, u32 reg, u8 mask, }
> > >
> > > /*
> > > - * AST_usrGuide_KCS.pdf
> > > - * 2. Background:
> > > - * we note D for Data, and C for Cmd/Status, default rules are
> > > - * A. KCS1 / KCS2 ( D / C:X / X+4 )
> > > - *D / C : CA0h / CA4h
> > > - *D / C : CA8h / CACh
> > > - * B. KCS3 ( D / C:XX2h / XX3h )
> > > - *D / C : CA2h / CA3h
> > > - *D / C : CB2h / CB3h
> > > - * C. KCS4
> > > - *D / C : CA4h / CA5h
> > > + * We note D for Data, and C for Cmd/Status, default rules are
> > > + *
> > > + * 1. Only the D address is given:
> > > + * A. KCS1/KCS2 (D/C: X/X+4)
> > > + * D/C: CA0h/CA4h
> > > + * D/C: CA8h/CACh
> > > + * B. KCS3 (D/C: XX2/XX3h)
> > > + * D/C: CA2h/CA3h
> > > + * C. KCS4 (D/C: X/X+1)
> > > + * D/C: CA4h/CA5h
> > > + *
> > > + * 2. Both the D/C addresses are given:
> > > + * A. KCS1/KCS2/KCS4 (D/C: X/Y)
> > > + * D/C: CA0h/CA1h
> > > + * D/C: CA8h/CA9h
> > > + * D/C: CA4h/CA5h
> > > + * B. KCS3 (D/C: XX2/XX3h)
> > > + * D/C: CA2h/CA3h
> > > */
> > > static int aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u32
> > > addrs[2], int nr_addrs) {
> > > --
> > > 2.25.1
> > >
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs: aspeed: Update port address comments
On Tue, Sep 20, 2022 at 10:03:33AM +0800, Chia-Wei Wang wrote:
> Remove AST_usrGuide_KCS.pdf as it is no longer maintained.
Even if it's no longer maintained, is it useful? It seems better to
leave in useful documentation unless it has been replaced with something
else.
-corey
>
> Add more descriptions as the driver now supports the I/O
> address configurations for both the KCS Data and Cmd/Status
> interface registers.
>
> Signed-off-by: Chia-Wei Wang
> ---
> drivers/char/ipmi/kcs_bmc_aspeed.c | 29 ++---
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c
> b/drivers/char/ipmi/kcs_bmc_aspeed.c
> index cdc88cde1e9a..19c32bf50e0e 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
> @@ -207,17 +207,24 @@ static void aspeed_kcs_updateb(struct kcs_bmc_device
> *kcs_bmc, u32 reg, u8 mask,
> }
>
> /*
> - * AST_usrGuide_KCS.pdf
> - * 2. Background:
> - * we note D for Data, and C for Cmd/Status, default rules are
> - * A. KCS1 / KCS2 ( D / C:X / X+4 )
> - *D / C : CA0h / CA4h
> - *D / C : CA8h / CACh
> - * B. KCS3 ( D / C:XX2h / XX3h )
> - *D / C : CA2h / CA3h
> - *D / C : CB2h / CB3h
> - * C. KCS4
> - *D / C : CA4h / CA5h
> + * We note D for Data, and C for Cmd/Status, default rules are
> + *
> + * 1. Only the D address is given:
> + * A. KCS1/KCS2 (D/C: X/X+4)
> + * D/C: CA0h/CA4h
> + * D/C: CA8h/CACh
> + * B. KCS3 (D/C: XX2/XX3h)
> + * D/C: CA2h/CA3h
> + * C. KCS4 (D/C: X/X+1)
> + * D/C: CA4h/CA5h
> + *
> + * 2. Both the D/C addresses are given:
> + * A. KCS1/KCS2/KCS4 (D/C: X/Y)
> + * D/C: CA0h/CA1h
> + * D/C: CA8h/CA9h
> + * D/C: CA4h/CA5h
> + * B. KCS3 (D/C: XX2/XX3h)
> + * D/C: CA2h/CA3h
> */
> static int aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u32
> addrs[2], int nr_addrs)
> {
> --
> 2.25.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Add __init/__exit annotations to module init/exit funcs
On Thu, Sep 22, 2022 at 07:19:24PM +0800, Xiu Jianfeng wrote:
> Add missing __init/__exit annotations to module init/exit funcs.
Applied, thanks.
-corey
>
> Signed-off-by: Xiu Jianfeng
> ---
> drivers/char/ipmi/ipmi_ssif.c | 4 ++--
> drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 4 ++--
> drivers/char/ipmi/kcs_bmc_serio.c | 4 ++--
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 13da021e7c6b..e1072809fe31 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -2098,7 +2098,7 @@ static struct platform_driver ipmi_driver = {
> .id_table = ssif_plat_ids
> };
>
> -static int init_ipmi_ssif(void)
> +static int __init init_ipmi_ssif(void)
> {
> int i;
> int rv;
> @@ -2140,7 +2140,7 @@ static int init_ipmi_ssif(void)
> }
> module_init(init_ipmi_ssif);
>
> -static void cleanup_ipmi_ssif(void)
> +static void __exit cleanup_ipmi_ssif(void)
> {
> if (!initialized)
> return;
> diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> index 486834a962c3..cf670e891966 100644
> --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c
> @@ -548,7 +548,7 @@ static struct kcs_bmc_driver kcs_bmc_ipmi_driver = {
> .ops = _bmc_ipmi_driver_ops,
> };
>
> -static int kcs_bmc_ipmi_init(void)
> +static int __init kcs_bmc_ipmi_init(void)
> {
> kcs_bmc_register_driver(_bmc_ipmi_driver);
>
> @@ -556,7 +556,7 @@ static int kcs_bmc_ipmi_init(void)
> }
> module_init(kcs_bmc_ipmi_init);
>
> -static void kcs_bmc_ipmi_exit(void)
> +static void __exit kcs_bmc_ipmi_exit(void)
> {
> kcs_bmc_unregister_driver(_bmc_ipmi_driver);
> }
> diff --git a/drivers/char/ipmi/kcs_bmc_serio.c
> b/drivers/char/ipmi/kcs_bmc_serio.c
> index 7e2067628a6c..1793358be782 100644
> --- a/drivers/char/ipmi/kcs_bmc_serio.c
> +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> @@ -140,7 +140,7 @@ static struct kcs_bmc_driver kcs_bmc_serio_driver = {
> .ops = _bmc_serio_driver_ops,
> };
>
> -static int kcs_bmc_serio_init(void)
> +static int __init kcs_bmc_serio_init(void)
> {
> kcs_bmc_register_driver(_bmc_serio_driver);
>
> @@ -148,7 +148,7 @@ static int kcs_bmc_serio_init(void)
> }
> module_init(kcs_bmc_serio_init);
>
> -static void kcs_bmc_serio_exit(void)
> +static void __exit kcs_bmc_serio_exit(void)
> {
> kcs_bmc_unregister_driver(_bmc_serio_driver);
> }
> --
> 2.17.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: kcs_bmc: Avoid wasting some memory.
Adding Andrew, the author of this code. On Sun, Sep 04, 2022 at 03:35:16PM +0200, Christophe JAILLET wrote: > KCS_MSG_BUFSIZ is 1000. > > When using devm_kmalloc(), there is a small memory overhead and, on most > systems, this leads to 40 bytes of extra memory allocation. > So 1040 bytes are expected to be allocated. > > The memory allocator works with fixed size hunks of memory. In this case, > it will require 2048 bytes of memory because more than 1024 bytes are > required. > > So, when requesting 3 x 1000 bytes, it ends up to 2048 x 3. > > In order to avoid wasting 3ko of memory, allocate buffers all at once. > 3000+40 bytes will be required and 4ko allocated. This still wastes 1ko, > but it is already better. > > Signed-off-by: Christophe JAILLET > --- > Looking at this code, I wonder why priv->miscdev.name is not freed in > kcs_bmc_ipmi_remove_device()? If I understand correctly, none of these need to be freed. devm allocated memory is freed automatically when the device is removed. > > If this make sense, this also mean that KCS_MSG_BUFSIZ can be increased at > no cost. > Or it could be slightly reduce to around 1024-40-1 bytes to keep the logic > which is in place. > > Another solution would be to use just kmalloc and add a > devm_add_action_or_reset() call and a function that frees the memory. > If it make sense, KCS_MSG_BUFSIZ could be increased to 1024 and we would > allocate just a little above 3x1024 bytes. > --- > drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 11 +-- > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > index 486834a962c3..15a4a39a6478 100644 > --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c > @@ -485,14 +485,15 @@ static int kcs_bmc_ipmi_add_device(struct > kcs_bmc_device *kcs_bmc) > > priv->client.dev = kcs_bmc; > priv->client.ops = _bmc_ipmi_client_ops; > - priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > - priv->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > - priv->kbuffer = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > + /* Allocate buffers all at once */ > + priv->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ * 3, > GFP_KERNEL); > + priv->data_out = priv->data_in + KCS_MSG_BUFSIZ; > + priv->kbuffer = priv->data_in + KCS_MSG_BUFSIZ * 2; You are doing arithmetic on a possibly NULL pointer. It's generally ok, but kind of frowned upon. Andew, what do you think? I guess it saves a little memory. -Corey > > priv->miscdev.minor = MISC_DYNAMIC_MINOR; > priv->miscdev.name = devm_kasprintf(kcs_bmc->dev, GFP_KERNEL, "%s%u", > DEVICE_NAME, > kcs_bmc->channel); > - if (!priv->data_in || !priv->data_out || !priv->kbuffer || > !priv->miscdev.name) > + if (!priv->data_in || !priv->miscdev.name) > return -EINVAL; > > priv->miscdev.fops = _bmc_ipmi_fops; > @@ -531,8 +532,6 @@ static int kcs_bmc_ipmi_remove_device(struct > kcs_bmc_device *kcs_bmc) > > misc_deregister(>miscdev); > kcs_bmc_disable_device(priv->client.dev, >client); > - devm_kfree(kcs_bmc->dev, priv->kbuffer); > - devm_kfree(kcs_bmc->dev, priv->data_out); > devm_kfree(kcs_bmc->dev, priv->data_in); > devm_kfree(kcs_bmc->dev, priv); > > -- > 2.34.1 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: don't wait forever when querying BMC device id
On Mon, Aug 15, 2022 at 06:15:40PM -0700, Jay Vosburgh wrote:
> Corey Minyard wrote:
>
> >On Fri, Aug 12, 2022 at 04:33:18PM -0700, Jay Vosburgh wrote:
> >> We have observed issues wherein the IPMI driver will sleep forever in
> >> uninterruptible wait with mutexes held (specifically, dyn_mutex and
> >> bmc_reg_mutex, acquired by __bmc_get_device_id). This occurs ultimately
> >> due to a BMC firmware bug; the BMC will fail to respond to requests,
> >> apparently related to the request rate, and the current logic will wait
> >> forever.
> >
> >This really isn't the right fix. The state machines running the
> >interfaces are required to time out after a period of time, usually
> >5 seconds, but that depends on how the hardware is behaving, or
> >misbehaving in this case. So though these are not timed mutexes, what
> >is running below should be timed, so it shouldn't be necessary here.
>
> We did try and follow the code paths to figure out what's going
> on at the lower levels, but the IPMI logic is fairly complex.
Yes. IPMI defines four different interfaces, and the code makes them
all look pretty much the same. There are a large number of ways
interfaces can be discovered or added (hard coded, hot plug, pci,
OF, ACPI, SMBIOS, and machine-specific methods) and three different ways
to talk to the hardware (port, memory mapped, and SMBus). Plus
interrupts or no interrupts, different register spacing and layout. It
leads to an unfortunate amount of complexity.
The ipmi_msghandler.c code is mostly a message router, not interface
handling. You can ignore most of that code and look right at the
interface you are using.
>
> As best we could determine, the send path for __bmc_get_device_id
> is something like:
>
> __bmc_get_device_id
> __get_guid
> send_guid_cmd
> i_ipmi_request
> smi_send
> wait_event
>
> where i_ipmi_request() calls i_ipmi_req_sysintf(), because
> send_guid_cmd() sets addr_type == IPMI_SYSTEM_INTERFACE_ADDR_TYPE, and
> then smi_send() to issue the request.
>
> send_guid_cmd
> i_ipmi_request
> i_ipmi_req_sysintf
> smi_send
>
> I presume that if the request goes out via SMI, the response
> should arrive the same way (perhaps this is incorrect).
It should come back from the same place you sent it to, yes. But
smi_send itself is not specific.
>
> It looks like the only place outside of the "make a request"
> path that will tweak the dyn_guid_set (needed to exit the wait_event()
> call) is within guid_handler(), which is the intf->null_user_handler set
> by __get_guid(), which in turn seems to be only called by
> deliver_response().
>
> Where are the state machines for the interfaces that should time
> out? Do they (or should they) call into deliver_response() to break out
> of the wait_event()?
The state machine depends on the low-leel interface. If it is kcs, bt,
or smic, it uses ipmi_si_intf.c, which then uses one of ipmi_kcs_sm.c,
ipmi_bt_sm.c, or ipmi_smic_sm.c to run the low-level state machine.
There is debugging you can enable in ipmi_smi_intf.c and in each
low-level driver.
If your interface is ssif (SMBus based), then ipmi_ssif.c contains the
code. You can enabled debugging there, too.
We have had this issue before, and someone suggested the same fix you
did, but we ended up fixing the issue in the state machine. If you fix
this in the upper level like you have, you will just have the same issue
again if the user software does the same command the BMC does here.
Which is quite likely.
If worse comes to worse, we can add a workaround for your specific
hardware. Which has also been done. But fixing the state machine is
really the right thing to do.
-corey
>
> >What is the particular hardware involved? The buggy hardware may be
> >exercising a software bug.
>
> The systems are white box servers from Ciara, the BMC chipset is
> an AST2500.
>
> -J
>
> >-corey
> >
> >>
> >> When the problem occurs, as each successive process queries the BMC,
> >> they will block in D state waiting to acquire the mutex, and with time
> >> the machine hangs. The BMC vendor has agreed it may be a hardware fault.
> >>
> >> This patch addresses the problem by replacing wait_event() with
> >> wait_event_timeout(). If the event times out (meaning the problem has
> >> occurred) the bmc->dyn_id_set and bmc->dyn_guid_set are set to 0 and the
> >> process eventually returns.
> >>
> >> Fixes: aa9c9ab2443e ("ipmi: allow dynamic BMC
Re: [Openipmi-developer] [PATCH] ipmi: don't wait forever when querying BMC device id
On Fri, Aug 12, 2022 at 04:33:18PM -0700, Jay Vosburgh wrote:
> We have observed issues wherein the IPMI driver will sleep forever in
> uninterruptible wait with mutexes held (specifically, dyn_mutex and
> bmc_reg_mutex, acquired by __bmc_get_device_id). This occurs ultimately
> due to a BMC firmware bug; the BMC will fail to respond to requests,
> apparently related to the request rate, and the current logic will wait
> forever.
This really isn't the right fix. The state machines running the
interfaces are required to time out after a period of time, usually
5 seconds, but that depends on how the hardware is behaving, or
misbehaving in this case. So though these are not timed mutexes, what
is running below should be timed, so it shouldn't be necessary here.
What is the particular hardware involved? The buggy hardware may be
exercising a software bug.
-corey
>
> When the problem occurs, as each successive process queries the BMC,
> they will block in D state waiting to acquire the mutex, and with time
> the machine hangs. The BMC vendor has agreed it may be a hardware fault.
>
> This patch addresses the problem by replacing wait_event() with
> wait_event_timeout(). If the event times out (meaning the problem has
> occurred) the bmc->dyn_id_set and bmc->dyn_guid_set are set to 0 and the
> process eventually returns.
>
> Fixes: aa9c9ab2443e ("ipmi: allow dynamic BMC version information")
> Signed-off-by: Jay Vosburgh
> Signed-off-by: Ioanna Alifieraki
>
> ---
> drivers/char/ipmi/ipmi_msghandler.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c
> b/drivers/char/ipmi/ipmi_msghandler.c
> index 703433493c85..a510839853b5 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -2572,7 +2572,9 @@ static int __get_device_id(struct ipmi_smi *intf,
> struct bmc_device *bmc)
> if (rv)
> goto out_reset_handler;
>
> - wait_event(intf->waitq, bmc->dyn_id_set != 2);
> + rv = wait_event_timeout(intf->waitq, bmc->dyn_id_set != 2, HZ * 5);
> + if (!rv)
> + bmc->dyn_id_set = 0;
>
> if (!bmc->dyn_id_set) {
> if (bmc->cc != IPMI_CC_NO_ERROR &&
> @@ -3337,11 +3339,15 @@ static void __get_guid(struct ipmi_smi *intf)
> bmc->dyn_guid_set = 2;
> intf->null_user_handler = guid_handler;
> rv = send_guid_cmd(intf, 0);
> - if (rv)
> + if (rv) {
> /* Send failed, no GUID available. */
> bmc->dyn_guid_set = 0;
> - else
> - wait_event(intf->waitq, bmc->dyn_guid_set != 2);
> + } else {
> + rv = wait_event_timeout(intf->waitq, bmc->dyn_guid_set != 2,
> + HZ * 5);
> + if (!rv)
> + bmc->dyn_guid_set = 0;
> + }
>
> /* dyn_guid_set makes the guid data available. */
> smp_rmb();
> --
> 2.34.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v3] dt-binding: ipmi: add fallback to npcm845 compatible
On Mon, Aug 08, 2022 at 03:38:45PM +0300, Krzysztof Kozlowski wrote:
> On 08/08/2022 15:26, Corey Minyard wrote:
> > On Mon, Aug 08, 2022 at 11:11:16AM +0300, Krzysztof Kozlowski wrote:
> >> On 08/08/2022 09:54, Tomer Maimon wrote:
> >>> Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible
> >>> string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules.
> >>>
> >>> Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible")
> >>> Signed-off-by: Tomer Maimon
> >>
> >>
> >> Acked-by: Krzysztof Kozlowski
> >
> > Ok, I think I understand how this is supposed to work. It's not
> > altogether clear from the device tree documentation. It says in
> > Documentation/devicetree/bindings/writing-bindings.rst:
> >
> > - DO make 'compatible' properties specific. DON'T use wildcards in
> > compatible
> > strings. DO use fallback compatibles when devices are the same as or a
> > subset
> > of prior implementations. DO add new compatibles in case there are new
> > features or bugs.
>
> This documentation is short, so it explains what should be done, not
> exactly why it should be done. If we wanted "why" this would not be set
> of 4 sentences but twice more...
>
> >
> > AFAICT, there are no new features or bugs, just a new SOC with the same
> > device. In general usage I have seen, you would just use the same
> > compatible.
>
> Most of the usages are like shown here. There are several other cases.
> It's the same with poor or good code - you will always find both patterns.
It is true, but lack of specified good examples makes it hard for people
like me to know what is right and wrong.
>
> > However, if I understand this, that last sentence should say:
> >
> > DO add new compatibles in case there is a new version of hardware with
> > the possibility of new features and/or bugs.
> >
> > Also, the term "specific" is, ironically, vague. Specific to what?
>
> To me it is rather clear. Specific as in first meanings of the word (1,
> 3, 4 and 5):
> https://en.wiktionary.org/wiki/specific
Everything is always clear when you understand something :).
The really hard part about technical documentation is forgetting what
you know and approaching it from a newbie's context.
>
> nuvoton,npcm7xx-kcs-bmc would not be definite (allows more meanings),
> unique (in terms of devices it expresses), distinctive (as two different
> devices use the same) or serving to identify one thing (again - two SoCs).
>
> What other meaning do you think of?
It is not the definition of specific that is vague, it is what
"specific" applies to. Is it specific to a SOC? Specific to a board?
Specific to a particular device implementation? Specific to a rev of
the silicon?
I will say that when I read that sentence, it means nothing to me.
If it said "DO make compatible properties as specific as possible to the
particular hardware implementation of the device" that would have more
meaning, but still leaves the reader wondering exactly how to do this.
For instance, should I put board/rev specific compatibles in? Would it
be:
"mycompany,myboard-rev1-npcm845-kcs-bmc",
"mycompany,myboard-npcm845-kcs-bmc",
"nuvoton,npcm845-revb-kcs-bmc", "nuvoton,npcm845-kcs-bmc",
"nuvoton,npcm750-kcs-bmc"
That's about as specific as you can get with fallbacks for everything,
but it is too specific? How far do you go? There might be wiring
differences on specific board, maybe that makes a difference.
That's where good (and identified bad) examples and rationale come in.
Tell the user why something is being done and it's easier to understand
what to do in different situations. It's not a matter of number of
sentences. Just like code, shorter is not always better.
Anyway, I have ranted for too long. Thank you for clearing this up for
me.
-corey
>
> >
> > It would be nice to have something added to "Typical cases and caveats"
> > that says:
> >
> > - If you are writing a binding for a new device that is the same as, or
> > a superset of another existing device, add a new specific compatible
> > for the new device followed by a compatible for the existing device.
> > That way, if the device has new bugs or new specific features are
> > added, you can add workarounds without modifying the device tree.
> >
> > Anyway, I have added this to my tree with your ack.
>
> Fantastic, thanks!
>
>
> Best regards,
> Krzysztof
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v3] dt-binding: ipmi: add fallback to npcm845 compatible
On Mon, Aug 08, 2022 at 11:11:16AM +0300, Krzysztof Kozlowski wrote:
> On 08/08/2022 09:54, Tomer Maimon wrote:
> > Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible
> > string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules.
> >
> > Fixes: 84261749e58a ("dt-bindings: ipmi: Add npcm845 compatible")
> > Signed-off-by: Tomer Maimon
>
>
> Acked-by: Krzysztof Kozlowski
Ok, I think I understand how this is supposed to work. It's not
altogether clear from the device tree documentation. It says in
Documentation/devicetree/bindings/writing-bindings.rst:
- DO make 'compatible' properties specific. DON'T use wildcards in compatible
strings. DO use fallback compatibles when devices are the same as or a subset
of prior implementations. DO add new compatibles in case there are new
features or bugs.
AFAICT, there are no new features or bugs, just a new SOC with the same
device. In general usage I have seen, you would just use the same
compatible. However, if I understand this, that last sentence should say:
DO add new compatibles in case there is a new version of hardware with
the possibility of new features and/or bugs.
Also, the term "specific" is, ironically, vague. Specific to what?
It would be nice to have something added to "Typical cases and caveats"
that says:
- If you are writing a binding for a new device that is the same as, or
a superset of another existing device, add a new specific compatible
for the new device followed by a compatible for the existing device.
That way, if the device has new bugs or new specific features are
added, you can add workarounds without modifying the device tree.
Anyway, I have added this to my tree with your ack.
-corey
>
>
> Best regards,
> Krzysztof
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2] dt-binding: ipmi: add fallback to npcm845 compatible
On Sun, Aug 07, 2022 at 05:54:28PM +0300, Tomer Maimon wrote: > On Sun, 7 Aug 2022 at 15:11, Corey Minyard wrote: > > > > On Sun, Aug 07, 2022 at 11:03:56AM +0300, Tomer Maimon wrote: > > > Hi Corey, > > > > > > Thanks for your comment. > > > > > > On Fri, 5 Aug 2022 at 14:58, Corey Minyard wrote: > > > > > > > > On Thu, Aug 04, 2022 at 09:18:00PM +0300, Tomer Maimon wrote: > > > > > Add to npcm845 KCS compatible string a fallback to npcm750 KCS > > > > > compatible > > > > > string becuase NPCM845 and NPCM750 BMCs are using identical KCS > > > > > modules. > > > > > > > > > > Signed-off-by: Tomer Maimon > > > > > --- > > > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git > > > > > a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > > > b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > > > index cbc10a68ddef..4fda76e63396 100644 > > > > > --- a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > > > +++ b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > > > @@ -7,7 +7,7 @@ used to perform in-band IPMI communication with their > > > > > host. > > > > > Required properties: > > > > > - compatible : should be one of > > > > > "nuvoton,npcm750-kcs-bmc" > > > > > -"nuvoton,npcm845-kcs-bmc" > > > > > +"nuvoton,npcm845-kcs-bmc", "nuvoton,npcm750-kcs-bmc" > > > > > > > > This is just wrong. The compatible is supposed to identify the device, > > > > not the board the device is on. I think compatible here should be > > > > "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is > > > > used on a board named npcm845. > > > The NPCM8XX is not a board, The Nuvoton NPCM8XX is a fourth-generation > > > BMC SoC device family. > > > > Ok, but same principle applies. > > > > If the device is exactly the same, then you would only use one of the > > "npcm7xx-kcs-bmc" and put that in both device trees. You can use > > "nuvoton,npcm750-kcs-bmc", it's really not that important. Or even > > "nuvoton,npcm-kcs-bmc" > If we use "nuvoton, npcm-kcs-bmc" we should take care of backward dts > compatibility, and I am not sure we like to change NPCM KCS driver. > > > > If the device has a minor difference that can be expressed in a > > parameter, then create a parameter for it. > > > > If the device has enough differences that a parameter or two doesn't > > cover it, then you put either nuvoton,npcm750-kcs-bmc or > > nuvoton,npcm750-kcs-bmc in the device tree. Not both. Then you need > > two entries in the of_device_id array and you use the data field or > > something to express the difference. > > > > Since there appears to be no difference, just put > > "nuvoton,npcm750-kcs-bmc" in the npcm845 and I will drop the patch > > adding all this. Then a patch can be added saying it applies to both > > the 7xx and 8xx series of BMC SOCs. If you want to change the name, > > then a patch will be needed for that, but then you will need multiple > > entries in your device tree, but you would not document it as such, as > > there would only be one that applies for this kernel. > > It little bit confusing to use nuvoton,npcm750-kcs-bmc that are > related to NPCM7XX for NPCM8XX KCS. A little, but it's not unusual. > We can use the generic name "nuvoton, npcm-kcs-bmc" as you suggested > above but we should take care of backward dts compatibility, and I am > not sure we like to change NPCM KCS driver. > > We had a disscation with Arnd, Arnd asked us to use a fallback as we > did here if NPCM8XX device module is similar to NPCM7XX module: > https://lore.kernel.org/lkml/20220522155046.260146-5-tmaimo...@gmail.com/ > > I think we should use a fallback to describe the NPCM8XX KCS in the > dt-binding document. I'm ok with that option. I guess I should have mentioned it. Add nuvoton,npcm-kcs-bmc to the driver's of_device_id table. Then use that in that compatible string in the device tree. Leave the 750 compatible string in the table for backwards compatibility. There's no point in having a bunch of those strings if they are all the same. If a new one comes out that is different, we can handle that when the time comes. -corey > > > > I'm pretty sure the only reason to have muliple compatible entries in a > > device tree is to cover multiple kernels where the name changed. > > > > -corey > > > > > > > > > > -corey > > > > > > > > > - interrupts : interrupt generated by the controller > > > > > - kcs_chan : The KCS channel number in the controller > > > > > > > > > > -- > > > > > 2.33.0 > > > > > > > > > > > Best regards, > > > > > > Tomer > > Best regards, > > Tomer ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2] dt-binding: ipmi: add fallback to npcm845 compatible
On Sun, Aug 07, 2022 at 11:03:56AM +0300, Tomer Maimon wrote: > Hi Corey, > > Thanks for your comment. > > On Fri, 5 Aug 2022 at 14:58, Corey Minyard wrote: > > > > On Thu, Aug 04, 2022 at 09:18:00PM +0300, Tomer Maimon wrote: > > > Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible > > > string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. > > > > > > Signed-off-by: Tomer Maimon > > > --- > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > index cbc10a68ddef..4fda76e63396 100644 > > > --- a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > +++ b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > > > @@ -7,7 +7,7 @@ used to perform in-band IPMI communication with their > > > host. > > > Required properties: > > > - compatible : should be one of > > > "nuvoton,npcm750-kcs-bmc" > > > -"nuvoton,npcm845-kcs-bmc" > > > +"nuvoton,npcm845-kcs-bmc", "nuvoton,npcm750-kcs-bmc" > > > > This is just wrong. The compatible is supposed to identify the device, > > not the board the device is on. I think compatible here should be > > "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is > > used on a board named npcm845. > The NPCM8XX is not a board, The Nuvoton NPCM8XX is a fourth-generation > BMC SoC device family. Ok, but same principle applies. If the device is exactly the same, then you would only use one of the "npcm7xx-kcs-bmc" and put that in both device trees. You can use "nuvoton,npcm750-kcs-bmc", it's really not that important. Or even "nuvoton,npcm-kcs-bmc" If the device has a minor difference that can be expressed in a parameter, then create a parameter for it. If the device has enough differences that a parameter or two doesn't cover it, then you put either nuvoton,npcm750-kcs-bmc or nuvoton,npcm750-kcs-bmc in the device tree. Not both. Then you need two entries in the of_device_id array and you use the data field or something to express the difference. Since there appears to be no difference, just put "nuvoton,npcm750-kcs-bmc" in the npcm845 and I will drop the patch adding all this. Then a patch can be added saying it applies to both the 7xx and 8xx series of BMC SOCs. If you want to change the name, then a patch will be needed for that, but then you will need multiple entries in your device tree, but you would not document it as such, as there would only be one that applies for this kernel. I'm pretty sure the only reason to have muliple compatible entries in a device tree is to cover multiple kernels where the name changed. -corey > > > > -corey > > > > > - interrupts : interrupt generated by the controller > > > - kcs_chan : The KCS channel number in the controller > > > > > > -- > > > 2.33.0 > > > > > Best regards, > > Tomer ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v2] dt-binding: ipmi: add fallback to npcm845 compatible
On Thu, Aug 04, 2022 at 09:18:00PM +0300, Tomer Maimon wrote: > Add to npcm845 KCS compatible string a fallback to npcm750 KCS compatible > string becuase NPCM845 and NPCM750 BMCs are using identical KCS modules. > > Signed-off-by: Tomer Maimon > --- > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > index cbc10a68ddef..4fda76e63396 100644 > --- a/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > +++ b/Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt > @@ -7,7 +7,7 @@ used to perform in-band IPMI communication with their host. > Required properties: > - compatible : should be one of > "nuvoton,npcm750-kcs-bmc" > -"nuvoton,npcm845-kcs-bmc" > +"nuvoton,npcm845-kcs-bmc", "nuvoton,npcm750-kcs-bmc" This is just wrong. The compatible is supposed to identify the device, not the board the device is on. I think compatible here should be "npcm7xx-kcs-bmc", and just use that everywhere. It's fine if that is used on a board named npcm845. -corey > - interrupts : interrupt generated by the controller > - kcs_chan : The KCS channel number in the controller > > -- > 2.33.0 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v1 0/2] char: ipmi: kcs: add Arbel NPCM8XX support
On Wed, Jul 27, 2022 at 08:39:08AM +0300, Tomer Maimon wrote: > Hi Corey, > > On Tue, 26 Jul 2022 at 22:47, Corey Minyard wrote: > > > > On Tue, Jul 26, 2022 at 10:41:38PM +0300, Tomer Maimon wrote: > > > Hi Corey, > > > > > > > > > On Mon, 18 Jul 2022 at 15:51, Corey Minyard wrote: > > > > > > > > On Sun, Jul 17, 2022 at 03:11:22PM +0300, Tomer Maimon wrote: > > > > > This patch set adds Arbel NPCM8XX Keyboard Controller Style (KCS) > > > > > support to > > > > > KCS NPCM driver. > > > > > > > > > > The NPCM KCS driver tested on NPCM845 evaluation board. > > > > > > > > This seems reasonable, I've pulled it into my tree. If anyone has any > > > > issues with this, please respond. > > > > > > > > -corey > > > > > > > > > > > > > > Tomer Maimon (2): > > > > > dt-bindings: ipmi: Add npcm845 compatible > > > > > char: ipmi: modify NPCM KCS configuration > > > > > > > > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 5 +++-- > > > > > drivers/char/ipmi/Kconfig | 6 +++--- > > > > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > > > > > > > -- > > > > > 2.33.0 > > > > > > > > > > > Sorry but I need to do a little fix in the document file. > > > > > > Can I do it or have you already applied the patches? > > > > At this point I'd prefer a patch on top of what is there. 5.19 isn't > > released yet, so the window isn't open, but that will happen soon and I > > don't want to rebase at this point. > O.K. thanks, > I will wait until 5.19 is released and then I will send the patch. Oh, sorry I wasn't clear. You can send it now, I just don't want to rebase what I have already. Just a new patch on top of it, and I'll get it in to 5.19. -corey ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v1 0/2] char: ipmi: kcs: add Arbel NPCM8XX support
On Tue, Jul 26, 2022 at 10:41:38PM +0300, Tomer Maimon wrote: > Hi Corey, > > > On Mon, 18 Jul 2022 at 15:51, Corey Minyard wrote: > > > > On Sun, Jul 17, 2022 at 03:11:22PM +0300, Tomer Maimon wrote: > > > This patch set adds Arbel NPCM8XX Keyboard Controller Style (KCS) support > > > to > > > KCS NPCM driver. > > > > > > The NPCM KCS driver tested on NPCM845 evaluation board. > > > > This seems reasonable, I've pulled it into my tree. If anyone has any > > issues with this, please respond. > > > > -corey > > > > > > > > Tomer Maimon (2): > > > dt-bindings: ipmi: Add npcm845 compatible > > > char: ipmi: modify NPCM KCS configuration > > > > > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 5 +++-- > > > drivers/char/ipmi/Kconfig | 6 +++--- > > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > > > -- > > > 2.33.0 > > > > > Sorry but I need to do a little fix in the document file. > > Can I do it or have you already applied the patches? At this point I'd prefer a patch on top of what is there. 5.19 isn't released yet, so the window isn't open, but that will happen soon and I don't want to rebase at this point. -corey > > Thanks, > > Tomer ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi: Fix comment typo
On Fri, Jul 15, 2022 at 01:41:56PM +0800, Jason Wang wrote: > The double `the' is duplicated in line 4360, remove one. Applied, thanks. -corey > > Signed-off-by: Jason Wang > --- > drivers/char/ipmi/ipmi_msghandler.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/ipmi/ipmi_msghandler.c > b/drivers/char/ipmi/ipmi_msghandler.c > index 703433493c85..c8a3b208f923 100644 > --- a/drivers/char/ipmi/ipmi_msghandler.c > +++ b/drivers/char/ipmi/ipmi_msghandler.c > @@ -4357,7 +4357,7 @@ static int handle_oem_get_msg_cmd(struct ipmi_smi *intf, > > /* >* The message starts at byte 4 which follows the > - * the Channel Byte in the "GET MESSAGE" command > + * Channel Byte in the "GET MESSAGE" command >*/ > recv_msg->msg.data_len = msg->rsp_size - 4; > memcpy(recv_msg->msg_data, >rsp[4], > -- > 2.35.1 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v1 0/2] char: ipmi: kcs: add Arbel NPCM8XX support
On Sun, Jul 17, 2022 at 03:11:22PM +0300, Tomer Maimon wrote: > This patch set adds Arbel NPCM8XX Keyboard Controller Style (KCS) support to > KCS NPCM driver. > > The NPCM KCS driver tested on NPCM845 evaluation board. This seems reasonable, I've pulled it into my tree. If anyone has any issues with this, please respond. -corey > > Tomer Maimon (2): > dt-bindings: ipmi: Add npcm845 compatible > char: ipmi: modify NPCM KCS configuration > > Documentation/devicetree/bindings/ipmi/npcm7xx-kcs-bmc.txt | 5 +++-- > drivers/char/ipmi/Kconfig | 6 +++--- > 2 files changed, 6 insertions(+), 5 deletions(-) > > -- > 2.33.0 > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 6/6] i2c: Make remove callback return void
On Tue, Jun 28, 2022 at 04:03:12PM +0200, Uwe Kleine-König wrote:
> From: Uwe Kleine-König
>
> The value returned by an i2c driver's remove function is mostly ignored.
> (Only an error message is printed if the value is non-zero that the
> error is ignored.)
>
> So change the prototype of the remove function to return no value. This
> way driver authors are not tempted to assume that passing an error to
> the upper layer is a good idea. All drivers are adapted accordingly.
> There is no intended change of behaviour, all callbacks were prepared to
> return 0 before.
For IPMI portions below:
Acked-by: Corey Minyard
>
> static const struct i2c_device_id lcd2s_i2c_id[] = {
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c
> b/drivers/char/ipmi/ipmb_dev_int.c
> index db40037eb347..a0e9e80d92ee 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -341,14 +341,12 @@ static int ipmb_probe(struct i2c_client *client)
> return 0;
> }
>
> -static int ipmb_remove(struct i2c_client *client)
> +static void ipmb_remove(struct i2c_client *client)
> {
> struct ipmb_dev *ipmb_dev = i2c_get_clientdata(client);
>
> i2c_slave_unregister(client);
> misc_deregister(_dev->miscdev);
> -
> - return 0;
> }
>
> static const struct i2c_device_id ipmb_id[] = {
> diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
> index ab19b4b3317e..25c010c9ec25 100644
> --- a/drivers/char/ipmi/ipmi_ipmb.c
> +++ b/drivers/char/ipmi/ipmi_ipmb.c
> @@ -424,7 +424,7 @@ static void ipmi_ipmb_request_events(void *send_info)
> /* We don't fetch events here. */
> }
>
> -static int ipmi_ipmb_remove(struct i2c_client *client)
> +static void ipmi_ipmb_remove(struct i2c_client *client)
> {
> struct ipmi_ipmb_dev *iidev = i2c_get_clientdata(client);
>
> @@ -438,8 +438,6 @@ static int ipmi_ipmb_remove(struct i2c_client *client)
> ipmi_ipmb_stop_thread(iidev);
>
> ipmi_unregister_smi(iidev->intf);
> -
> - return 0;
> }
>
> static int ipmi_ipmb_probe(struct i2c_client *client)
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index fc742ee9c046..13da021e7c6b 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1281,13 +1281,13 @@ static void shutdown_ssif(void *send_info)
> }
> }
>
> -static int ssif_remove(struct i2c_client *client)
> +static void ssif_remove(struct i2c_client *client)
> {
> struct ssif_info *ssif_info = i2c_get_clientdata(client);
> struct ssif_addr_info *addr_info;
>
> if (!ssif_info)
> - return 0;
> + return;
>
> /*
>* After this point, we won't deliver anything asychronously
> @@ -1303,8 +1303,6 @@ static int ssif_remove(struct i2c_client *client)
> }
>
> kfree(ssif_info);
> -
> - return 0;
> }
>
> static int read_response(struct i2c_client *client, unsigned char *resp)
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v7 1/3] ipmi: ssif_bmc: Add SSIF BMC driver
On Wed, Jun 01, 2022 at 03:23:11PM +0700, Quan Nguyen wrote: > On 04/05/2022 19:06, Corey Minyard wrote: > > On Wed, May 04, 2022 at 01:45:03PM +0700, Quan Nguyen via > > Openipmi-developer wrote: > > > > > > > > I seem to remember mentioning this before, but there is no reason to > > > > pack the structures below. > > > > > > > > > > The packed structure is because we want to pick the len directly from user > > > space without worry about the padding byte. > > > > > > As we plan not to use the .h file in next version, I still would like to > > > use > > > packed structure internally inside ssif_bmc.c file. > > > > Packed doesn't matter for the userspace API. If you look at other > > structures in the userspace API, they are not packed, either. The > > compiler will do the right thing on both ends. > > > > > > > > > And second, the following is a userspace API structures, so it needs to > > > > be in its own file in include/uapi/linux, along with any supporting > > > > things that users will need to use. And your userspace code should be > > > > using that file. > > > > > > > > > > Meantime, I'd like not to use .h as I see there is no demand for sharing > > > the > > > data structure between kernel and user space yet. But we may do it in the > > > future. > > > > If you have a userspace API, it needs to be in include/uapi/linux. > > You may not be the only user of this code. In fact, you probably won't > > be. You need to have a .h with the structures in it, you don't want the > > same structure in two places if you can help it. > > > > Dear Corey, > > Is it OK to push the structure definition into the > include/uapi/linux/ipmi_bmc.h ? > > Or should it need to be in separate new header file in uapi/linux ? I think a different file, like ipmi_ssif_bmc, to match the file and operation. Unless you need the things in ipmi_bmc.h, which I don't think is the case. -corey > > Thank you, > - Quan > > ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes for 4.19
The following changes since commit a7391ad3572431a354c927cf8896e86e50d7d0bf: Merge tag 'iomm-fixes-v5.18-rc5' of git://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu (2022-05-04 11:04:52 -0700) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-4.19-1 for you to fetch changes up to a508e33956b538e034ed5df619a73ec7c15bda72: ipmi:ipmb: Fix refcount leak in ipmi_ipmb_probe (2022-05-12 10:00:04 -0500) Fixes for IPMI Add limits on the number of users and messages, plus sysfs interfaces to control those limits. Other than that, little cleanups, use dev_xxx() insted of pr_xxx(), create initializers for structures, fix a refcount leak, etc. Corey Minyard (11): ipmi: Add a limit on the number of users that may use IPMI ipmi: Limit the number of message a user may have outstanding ipmi: Add a sysfs interface to view the number of users ipmi: Add a sysfs count of total outstanding messages for an interface ipmi:ssif: Check for NULL msg when handling events and messages ipmi: Add an intializer for ipmi_smi_msg struct ipmi: Add an intializer for ipmi_recv_msg struct ipmi: Fix pr_fmt to avoid compilation issues ipmi: Convert pr_debug() to dev_dbg() ipmi:si: Convert pr_debug() to dev_dbg() ipmi: Make two logs unique Miaoqian Lin (1): ipmi:ipmb: Fix refcount leak in ipmi_ipmb_probe Stephen Kitt (1): ipmi: use simple i2c probe function Yu Zhe (1): ipmi: remove unnecessary type castings drivers/char/ipmi/ipmb_dev_int.c| 5 +- drivers/char/ipmi/ipmi_ipmb.c | 6 +- drivers/char/ipmi/ipmi_msghandler.c | 111 drivers/char/ipmi/ipmi_poweroff.c | 8 +-- drivers/char/ipmi/ipmi_si_intf.c| 17 +++--- drivers/char/ipmi/ipmi_ssif.c | 33 +-- drivers/char/ipmi/ipmi_watchdog.c | 28 - include/linux/ipmi.h| 5 ++ include/linux/ipmi_smi.h| 6 ++ 9 files changed, 165 insertions(+), 54 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH] ipmi:ipmb: Fix refcount leak in ipmi_ipmb_probe
On Thu, May 12, 2022 at 08:44:45AM +0400, Miaoqian Lin wrote:
> of_parse_phandle() returns a node pointer with refcount
> incremented, we should use of_node_put() on it when done.
> Add missing of_node_put() to avoid refcount leak.
Thanks, applied and backport requested for 5.17.
-corey
>
> Fixes: 00d93611f002 ("ipmi:ipmb: Add the ability to have a separate slave and
> master device")
> Signed-off-by: Miaoqian Lin
> ---
> drivers/char/ipmi/ipmi_ipmb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
> index b81b862532fb..a8bfe0ade082 100644
> --- a/drivers/char/ipmi/ipmi_ipmb.c
> +++ b/drivers/char/ipmi/ipmi_ipmb.c
> @@ -476,6 +476,7 @@ static int ipmi_ipmb_probe(struct i2c_client *client,
> slave_np = of_parse_phandle(dev->of_node, "slave-dev", 0);
> if (slave_np) {
> slave_adap = of_get_i2c_adapter_by_node(slave_np);
> + of_node_put(slave_np);
> if (!slave_adap) {
> dev_notice(>dev,
> "Could not find slave adapter\n");
> --
> 2.25.1
>
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH v7 1/3] ipmi: ssif_bmc: Add SSIF BMC driver
On Wed, May 04, 2022 at 01:45:03PM +0700, Quan Nguyen via Openipmi-developer
wrote:
> >
> > I seem to remember mentioning this before, but there is no reason to
> > pack the structures below.
> >
>
> The packed structure is because we want to pick the len directly from user
> space without worry about the padding byte.
>
> As we plan not to use the .h file in next version, I still would like to use
> packed structure internally inside ssif_bmc.c file.
Packed doesn't matter for the userspace API. If you look at other
structures in the userspace API, they are not packed, either. The
compiler will do the right thing on both ends.
>
> > And second, the following is a userspace API structures, so it needs to
> > be in its own file in include/uapi/linux, along with any supporting
> > things that users will need to use. And your userspace code should be
> > using that file.
> >
>
> Meantime, I'd like not to use .h as I see there is no demand for sharing the
> data structure between kernel and user space yet. But we may do it in the
> future.
If you have a userspace API, it needs to be in include/uapi/linux.
You may not be the only user of this code. In fact, you probably won't
be. You need to have a .h with the structures in it, you don't want the
same structure in two places if you can help it.
>
> > > +struct ssif_msg {
> > > + u8 len;
> >
> > Just to be 100% safe, it might be better to use a u16 on this. The spec
> > sort of limits this to 255 bytes, but it also sort of leaves it open to
> > be larger.
> >
> Yes, u8 only limited to 255 bytes and there is no space for future grow.
Please make it a unsigned int for the length and __u8 for the data to
give necessary flexibility.
Thanks,
-corey
>
> > > + u8 payload[MSG_PAYLOAD_LEN_MAX];
> > > +} __packed;
> > > +
> > > +struct ssif_part_buffer {
> > > + u8 address;
> > > + u8 smbus_cmd;
> > > + u8 length;
> > > + u8 payload[MAX_PAYLOAD_PER_TRANSACTION];
> > > + u8 pec;
> > > + u8 index;
> > > +} __packed;
> > > +
> > > +/*
> > > + * SSIF internal states:
> > > + * SSIF_READY 0x00 : Ready state
> > > + * SSIF_START 0x01 : Start smbus transaction
> > > + * SSIF_SMBUS_CMD 0x02 : Received SMBus command
> > > + * SSIF_REQ_RECVING 0x03 : Receiving request
> > > + * SSIF_RES_SENDING 0x04 : Sending response
> > > + * SSIF_BAD_SMBUS 0x05 : Bad SMbus transaction
> > > + */
> > > +enum ssif_state {
> > > + SSIF_READY,
> > > + SSIF_START,
> > > + SSIF_SMBUS_CMD,
> > > + SSIF_REQ_RECVING,
> > > + SSIF_RES_SENDING,
> > > + SSIF_ABORTING,
> > > + SSIF_STATE_MAX
> > > +};
> > > +
> > > +struct ssif_bmc_ctx {
> > > + struct i2c_client *client;
> > > + struct miscdevice miscdev;
> > > + int msg_idx;
> > > + boolpec_support;
> > > + /* ssif bmc spinlock */
> > > + spinlock_t lock;
> > > + wait_queue_head_t wait_queue;
> > > + u8 running;
> > > + enum ssif_state state;
> > > + /* Timeout waiting for response */
> > > + struct timer_list response_timer;
> > > + boolresponse_timer_inited;
> > > + /* Flag to identify a Multi-part Read Transaction */
> > > + boolis_singlepart_read;
> > > + u8 nbytes_processed;
> > > + u8 remain_len;
> > > + u8 recv_len;
> > > + /* Block Number of a Multi-part Read Transaction */
> > > + u8 block_num;
> > > + boolrequest_available;
> > > + boolresponse_in_progress;
> > > + boolbusy;
> > > + boolaborting;
> > > + /* Buffer for SSIF Transaction part*/
> > > + struct ssif_part_buffer part_buf;
> > > + struct ssif_msg response;
> > > + struct ssif_msg request;
> > > +};
> > > +
> > > +static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
> > > +{
> > > + return container_of(file->private_data, struct ssif_bmc_ctx, miscdev);
> > > +}
> > > +#endif /* __SSIF_BMC_H__ */
> > > --
> > > 2.35.1
> > >
> > >
>
>
>
> ___
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer
___
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
[Openipmi-developer] [GIT PULL] IPMI bug fixes for 5.17 (second set)
The following changes since commit ae085d7f9365de7da27ab5c0d16b12d51ea7fca9: mm: kfence: fix missing objcg housekeeping for SLAB (2022-03-27 18:47:00 -0700) are available in the Git repository at: https://github.com/cminyard/linux-ipmi.git tags/for-linus-5.17-2 for you to fetch changes up to 9cc3aac42566a0021e0ab7c4e9b31667ad75b1e3: ipmi:ipmi_ipmb: Fix null-ptr-deref in ipmi_unregister_smi() (2022-04-29 10:06:52 -0500) Fix some issues that were reported This has been in for-next for a bit (longer than the times would indicate, I had to rebase to add some text to the headers) and these are fixes that need to go in. Corey Minyard (2): ipmi: When handling send message responses, don't process the message ipmi:ipmi_ipmb: Fix null-ptr-deref in ipmi_unregister_smi() drivers/char/ipmi/ipmi_msghandler.c | 7 ++- drivers/char/ipmi/ipmi_si_intf.c| 5 + 2 files changed, 7 insertions(+), 5 deletions(-) ___ Openipmi-developer mailing list Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [PATCH 21/30] panic: Introduce the panic pre-reboot notifier list
On Wed, Apr 27, 2022 at 07:49:15PM -0300, Guilherme G. Piccoli wrote: > This patch renames the panic_notifier_list to panic_pre_reboot_list; > the idea is that a subsequent patch will refactor the panic path > in order to better split the notifiers, running some of them very > early, some of them not so early [but still before kmsg_dump()] and > finally, the rest should execute late, after kdump. The latter ones > are now in the panic pre-reboot list - the name comes from the idea > that these notifiers execute before panic() attempts rebooting the > machine (if that option is set). > > We also took the opportunity to clean-up useless header inclusions, > improve some notifier block declarations (e.g. in ibmasm/heartbeat.c) > and more important, change some priorities - we hereby set 2 notifiers > to run late in the list [iss_panic_event() and the IPMI panic_event()] > due to the risks they offer (may not return, for example). > Proper documentation is going to be provided in a subsequent patch, > that effectively refactors the panic path. For the IPMI portion: Acked-by: Corey Minyard Note that the IPMI panic_event() should always return, but it may take some time, especially if the IPMI controller is no longer functional. So the risk of a long delay is there and it makes sense to move it very late. -corey > > Cc: Alex Elder > Cc: Alexander Gordeev > Cc: Anton Ivanov > Cc: Benjamin Herrenschmidt > Cc: Bjorn Andersson > Cc: Boris Ostrovsky > Cc: Chris Zankel > Cc: Christian Borntraeger > Cc: Corey Minyard > Cc: Dexuan Cui > Cc: "H. Peter Anvin" > Cc: Haiyang Zhang > Cc: Heiko Carstens > Cc: Helge Deller > Cc: Ivan Kokshaysky > Cc: "James E.J. Bottomley" > Cc: James Morse > Cc: Johannes Berg > Cc: Juergen Gross > Cc: "K. Y. Srinivasan" > Cc: Mathieu Poirier > Cc: Matt Turner > Cc: Mauro Carvalho Chehab > Cc: Max Filippov > Cc: Michael Ellerman > Cc: Paul Mackerras > Cc: Pavel Machek > Cc: Richard Henderson > Cc: Richard Weinberger > Cc: Robert Richter > Cc: Stefano Stabellini > Cc: Stephen Hemminger > Cc: Sven Schnelle > Cc: Tony Luck > Cc: Vasily Gorbik > Cc: Wei Liu > Signed-off-by: Guilherme G. Piccoli > --- > > Notice that, with this name change, out-of-tree code that relies in the global > exported "panic_notifier_list" will fail to build. We could easily keep the > retro-compatibility by making the old symbol to still exist and point to the > pre_reboot list (or even, keep the old naming). > > But our design choice was to allow the breakage, making users rethink their > notifiers, adding them in the list that fits best. If that wasn't a good > decision, we're open to change it, of course. > Thanks in advance for the review! > > arch/alpha/kernel/setup.c | 4 ++-- > arch/parisc/kernel/pdc_chassis.c | 3 +-- > arch/powerpc/kernel/setup-common.c| 2 +- > arch/s390/kernel/ipl.c| 4 ++-- > arch/um/drivers/mconsole_kern.c | 2 +- > arch/um/kernel/um_arch.c | 2 +- > arch/x86/xen/enlighten.c | 2 +- > arch/xtensa/platforms/iss/setup.c | 4 ++-- > drivers/char/ipmi/ipmi_msghandler.c | 12 +++- > drivers/edac/altera_edac.c| 3 +-- > drivers/hv/vmbus_drv.c| 4 ++-- > drivers/leds/trigger/ledtrig-panic.c | 3 +-- > drivers/misc/ibmasm/heartbeat.c | 16 +--- > drivers/net/ipa/ipa_smp2p.c | 5 ++--- > drivers/parisc/power.c| 4 ++-- > drivers/remoteproc/remoteproc_core.c | 6 -- > drivers/s390/char/con3215.c | 2 +- > drivers/s390/char/con3270.c | 2 +- > drivers/s390/char/sclp_con.c | 2 +- > drivers/s390/char/sclp_vt220.c| 2 +- > drivers/staging/olpc_dcon/olpc_dcon.c | 6 -- > drivers/video/fbdev/hyperv_fb.c | 4 ++-- > include/linux/panic_notifier.h| 2 +- > kernel/panic.c| 9 - > 24 files changed, 54 insertions(+), 51 deletions(-) > > diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c > index d88bdf852753..8ace0d7113b6 100644 > --- a/arch/alpha/kernel/setup.c > +++ b/arch/alpha/kernel/setup.c > @@ -472,8 +472,8 @@ setup_arch(char **cmdline_p) > } > > /* Register a call for panic conditions. */ > - atomic_notifier_chain_register(_notifier_list, > - _panic_block); > + atomic_notifier_chain_register(_pre_reboot_list, > + _panic_block); > > #ifndef alpha_using_srm > /* Assume that we've booted from SRM if we haven't booted from
Re: [Openipmi-developer] [PATCH v7 1/3] ipmi: ssif_bmc: Add SSIF BMC driver
On Fri, Apr 22, 2022 at 11:08:01AM +0700, Quan Nguyen wrote: > The SMBus system interface (SSIF) IPMI BMC driver can be used to perform > in-band IPMI communication with their host in management (BMC) side. > > Thanks Dan for the copy_from_user() fix in the link below. This is much better, the handling of lengths and indexes is much easier to understand. I hope you agree. I may be repeating myself on some things, it's been a while since the last submit. So please forgive me if I do. Comments inline... > > Link: > https://lore.kernel.org/linux-arm-kernel/20220310114119.13736-4-q...@os.amperecomputing.com/ > Signed-off-by: Quan Nguyen > --- > v7: > + Remove unneccessary del_timer() in response_timeout()[Corey] > + Change compatible string from "ampere,ssif-bmc" to "ssif-bmc" [Jae] > + Add MODULE_DEVICE_TABLE(of, ssif_bmc_match), fix blank line[Jae] > + Dropped the use of ssif_msg_len() macro, use the len directly [Quan] > + Solve possible issue if both response timer and ssif_bmc_write() > occurred at the same time [Corey] > + Fix wrong return type of ssif_bmc_poll() [kernel robot test] > + Refactor and introduce ssif_part_buffer struct to replace the > response_buf to manage each send/receive part of ssif [Quan] > + Change SSIF_BAD_SMBUS state to SSIF_ABORTING state [Corey] > + Support abort feature to skip the current bad request/response and > wait until next new request[Corey] > + Refactor the PEC calculation to avoid the re-calculate the PEC on > each I2C_SLAVE_WRITE_RECEIVED event [Quan] > + Fix the use of error-proned idx [Corey] > + Defer the test for valid SMBus command until the read/write part > is determined [Quan] > + Change/split unsupported_smbus_cmd() to > supported_[write|read]_cmd() [Corey] > + Abort the request if somehow its size exceeded 255 bytes [Quan] > > v6: > + Drop the use of slave_enable() [Wolfram] > + Make i2c-aspeed to issue RxCmdLast command on all > I2C_SLAVE_WRITE_REQUESTED event to assert NAK when slave busy [Quan] > + Make i2c slave to return -EBUSY when it's busy[Quan] > + Drop the aborting feature as return Completion Code 0xFF may stop > host to retry and make ipmi_ssif.so fails to load [Quan] > + Add timer to recover slave from busy state when no response [Quan] > + Clean request/response buffer appropriately [Quan] > + Add some minor change on error and warning messages [Quan] > > v5: > + None > > v4: > + Send response with Completion code 0xFF when aborting [Quan] > + Added bounding check on SMBus writes and the whole request [Dan] > + Moved buffer to end of struct ssif_bmc_ctx to avoid context > corruption if somehow buffer is written past the end [Dan] > + Return -EINVAL if userspace buffer too small, dont > silence truncate [Corey, Joel] > + Not necessary to check NONBLOCK in lock [Corey] > + Enforce one user at a time[Joel] > + Reject write with invalid response length from userspace [Corey] > + Add state machines for better ssif bmc state handling [Quan] > + Drop ssif_bmc_aspeed.c and make ssif_bmc.c is generic > SSIF BMC driver [Quan] > + Change compatible string "aspeed,ast2500-ssif-bmc" to > "ampere,ssif-bmc" [Quan] > + Abort current request with invalid SMBus write or > invalid command [Quan] > + Abort all request if there is pending response[Quan] > + Changed validate_pec() to validate_request() [Quan] > + Add unsupported_smbus_cmd() to handle unknown SMBus command [Quan] > + Print internal state string for ease investigating issue [Quan] > + Move to READY state on SLAVE_STOP event [Quan] > + Change initilize_transfer() to process_smbus_cmd()[Quan] > + Introduce functions for each slave event [Quan] > > v3: > + Removed redundant license info[Joel] > + Switched to use traditional if-else [Joel] > + Removed unused ssif_bmc_ioctl() [Joel] > + Made handle_request()/complete_response() to return void [Joel] > + Refactored send_ssif_bmc_response() and > receive_ssif_bmc_request() [Corey] > + Removed mutex[Corey] > + Use
