[RFC] Patches to disable messages during BMC reset

[RFC] Patches to disable messages during BMC reset

[Corey Minyard]

I went ahead and did some patches for this, since it was on my mind.

With these, if a reset is sent to the BMC, the driver will disable
messages to the BMC for a time, defaulting to 30 seconds.  Don't
modify message timing, since no messages are allowed, anyway.

If a firmware update command is sent to the BMC, then just reject
sysfs commands that query the BMC.  Modify message timing and
allow direct messages through the driver interface.

Hopefully this will work around the problem, and it's a good idea,
anyway.

-corey

[PATCH 1/4] ipmi: Differentiate between reset and firmware update in maintenance

[Corey Minyard]

This allows later changes to have different behaviour during a reset
verses a firmware update.

Signed-off-by: Corey Minyard <corey@minyard.net>
---
 drivers/char/ipmi/ipmi_msghandler.c | 42 ++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 8e9050f99e9e..f124c0b33db8 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -539,7 +539,11 @@ struct ipmi_smi {
 
 	/* For handling of maintenance mode. */
 	int maintenance_mode;
-	bool maintenance_mode_enable;
+
+#define IPMI_MAINTENANCE_MODE_STATE_OFF		0
+#define IPMI_MAINTENANCE_MODE_STATE_FIRMWARE	1
+#define IPMI_MAINTENANCE_MODE_STATE_RESET	2
+	int maintenance_mode_state;
 	int auto_maintenance_timeout;
 	spinlock_t maintenance_mode_lock; /* Used in a timer... */
 
@@ -1534,8 +1538,15 @@ EXPORT_SYMBOL(ipmi_get_maintenance_mode);
 static void maintenance_mode_update(struct ipmi_smi *intf)
 {
 	if (intf->handlers->set_maintenance_mode)
+		/*
+		 * Lower level drivers only care about firmware mode
+		 * as it affects their timing.  They don't care about
+		 * reset, which disables all commands for a while.
+		 */
 		intf->handlers->set_maintenance_mode(
-			intf->send_info, intf->maintenance_mode_enable);
+			intf->send_info,
+			(intf->maintenance_mode_state ==
+			 IPMI_MAINTENANCE_MODE_STATE_FIRMWARE));
 }
 
 int ipmi_set_maintenance_mode(struct ipmi_user *user, int mode)
@@ -1552,16 +1563,17 @@ int ipmi_set_maintenance_mode(struct ipmi_user *user, int mode)
 	if (intf->maintenance_mode != mode) {
 		switch (mode) {
 		case IPMI_MAINTENANCE_MODE_AUTO:
-			intf->maintenance_mode_enable
-				= (intf->auto_maintenance_timeout > 0);
+			/* Just leave it alone. */
 			break;
 
 		case IPMI_MAINTENANCE_MODE_OFF:
-			intf->maintenance_mode_enable = false;
+			intf->maintenance_mode_state =
+				IPMI_MAINTENANCE_MODE_STATE_OFF;
 			break;
 
 		case IPMI_MAINTENANCE_MODE_ON:
-			intf->maintenance_mode_enable = true;
+			intf->maintenance_mode_state =
+				IPMI_MAINTENANCE_MODE_STATE_FIRMWARE;
 			break;
 
 		default:
@@ -1922,13 +1934,18 @@ static int i_ipmi_req_sysintf(struct ipmi_smi        *intf,
 
 	if (is_maintenance_mode_cmd(msg)) {
 		unsigned long flags;
+		int newst;
+
+		if (msg->netfn == IPMI_NETFN_FIRMWARE_REQUEST)
+			newst = IPMI_MAINTENANCE_MODE_STATE_FIRMWARE;
+		else
+			newst = IPMI_MAINTENANCE_MODE_STATE_RESET;
 
 		spin_lock_irqsave(&intf->maintenance_mode_lock, flags);
-		intf->auto_maintenance_timeout
-			= maintenance_mode_timeout_ms;
+		intf->auto_maintenance_timeout = maintenance_mode_timeout_ms;
 		if (!intf->maintenance_mode
-		    && !intf->maintenance_mode_enable) {
-			intf->maintenance_mode_enable = true;
+				&& intf->maintenance_mode_state < newst) {
+			intf->maintenance_mode_state = newst;
 			maintenance_mode_update(intf);
 		}
 		spin_unlock_irqrestore(&intf->maintenance_mode_lock,
@@ -5083,7 +5100,8 @@ static bool ipmi_timeout_handler(struct ipmi_smi *intf,
 				-= timeout_period;
 			if (!intf->maintenance_mode
 			    && (intf->auto_maintenance_timeout <= 0)) {
-				intf->maintenance_mode_enable = false;
+				intf->maintenance_mode_state =
+					IPMI_MAINTENANCE_MODE_STATE_OFF;
 				maintenance_mode_update(intf);
 			}
 		}
@@ -5099,7 +5117,7 @@ static bool ipmi_timeout_handler(struct ipmi_smi *intf,
 static void ipmi_request_event(struct ipmi_smi *intf)
 {
 	/* No event requests when in maintenance mode. */
-	if (intf->maintenance_mode_enable)
+	if (intf->maintenance_mode_state)
 		return;
 
 	if (!intf->in_shutdown)
-- 
2.43.0

[PATCH 2/4] ipmi: Disable sysfs access and requests in maintenance mode

[Corey Minyard]

If the driver goes into any maintenance mode, disable sysfs access until
it is done.

If the driver goes into reset maintenance mode, disable all messages
until it is done.

Signed-off-by: Corey Minyard <corey@minyard.net>
---
 drivers/char/ipmi/ipmi_msghandler.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index f124c0b33db8..72f5f4a0c056 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2338,6 +2338,11 @@ static int i_ipmi_request(struct ipmi_user     *user,
 
 	if (!run_to_completion)
 		mutex_lock(&intf->users_mutex);
+	if (intf->maintenance_mode_state == IPMI_MAINTENANCE_MODE_STATE_RESET) {
+		/* No messages while the BMC is in reset. */
+		rv = -EBUSY;
+		goto out_err;
+	}
 	if (intf->in_shutdown) {
 		rv = -ENODEV;
 		goto out_err;
@@ -2639,6 +2644,12 @@ static int __bmc_get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc,
 	    (bmc->dyn_id_set && time_is_after_jiffies(bmc->dyn_id_expiry)))
 		goto out_noprocessing;
 
+	/* Don't allow sysfs access when in maintenance mode. */
+	if (intf->maintenance_mode_state) {
+		rv = -EBUSY;
+		goto out_noprocessing;
+	}
+
 	prev_guid_set = bmc->dyn_guid_set;
 	__get_guid(intf);
 
-- 
2.43.0

[PATCH 3/4] ipmi: Add a maintenance mode sysfs file

[Corey Minyard]

So you can see if it's in maintenance mode and see how long is left.

Signed-off-by: Corey Minyard <corey@minyard.net>
---
 drivers/char/ipmi/ipmi_msghandler.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 72f5f4a0c056..5ff35c473b50 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -432,6 +432,7 @@ struct ipmi_smi {
 	atomic_t nr_users;
 	struct device_attribute nr_users_devattr;
 	struct device_attribute nr_msgs_devattr;
+	struct device_attribute maintenance_mode_devattr;
 
 
 	/* Used for wake ups at startup. */
@@ -3545,6 +3546,19 @@ static ssize_t nr_msgs_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(nr_msgs);
 
+static ssize_t maintenance_mode_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct ipmi_smi *intf = container_of(attr,
+					     struct ipmi_smi,
+					     maintenance_mode_devattr);
+
+	return sysfs_emit(buf, "%u %d\n", intf->maintenance_mode_state,
+			  intf->auto_maintenance_timeout);
+}
+static DEVICE_ATTR_RO(maintenance_mode);
+
 static void redo_bmc_reg(struct work_struct *work)
 {
 	struct ipmi_smi *intf = container_of(work, struct ipmi_smi,
@@ -3681,6 +3695,14 @@ int ipmi_add_smi(struct module         *owner,
 		goto out_err_bmc_reg;
 	}
 
+	intf->maintenance_mode_devattr = dev_attr_maintenance_mode;
+	sysfs_attr_init(&intf->maintenance_mode_devattr.attr);
+	rv = device_create_file(intf->si_dev, &intf->maintenance_mode_devattr);
+	if (rv) {
+		device_remove_file(intf->si_dev, &intf->nr_users_devattr);
+		goto out_err_bmc_reg;
+	}
+
 	intf->intf_num = i;
 	mutex_unlock(&ipmi_interfaces_mutex);
 
@@ -3788,6 +3810,7 @@ void ipmi_unregister_smi(struct ipmi_smi *intf)
 	if (intf->handlers->shutdown)
 		intf->handlers->shutdown(intf->send_info);
 
+	device_remove_file(intf->si_dev, &intf->maintenance_mode_devattr);
 	device_remove_file(intf->si_dev, &intf->nr_msgs_devattr);
 	device_remove_file(intf->si_dev, &intf->nr_users_devattr);
 
-- 
2.43.0

[PATCH 4/4] ipmi: Set a timer for maintenance mode

[Corey Minyard]

Now that maintenance mode rejects all messages, there's nothing to
run time timer.  Make sure the timer is running in maintenance mode.

Signed-off-by: Corey Minyard <corey@minyard.net>
---
 drivers/char/ipmi/ipmi_msghandler.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index 5ff35c473b50..786c71eb00f4 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -50,6 +50,8 @@ static void intf_free(struct kref *ref);
 static bool initialized;
 static bool drvregistered;
 
+static struct timer_list ipmi_timer;
+
 /* Numbers in this enumerator should be mapped to ipmi_panic_event_str */
 enum ipmi_panic_event_op {
 	IPMI_SEND_PANIC_EVENT_NONE,
@@ -1948,6 +1950,7 @@ static int i_ipmi_req_sysintf(struct ipmi_smi        *intf,
 				&& intf->maintenance_mode_state < newst) {
 			intf->maintenance_mode_state = newst;
 			maintenance_mode_update(intf);
+			mod_timer(&ipmi_timer, jiffies + IPMI_TIMEOUT_JIFFIES);
 		}
 		spin_unlock_irqrestore(&intf->maintenance_mode_lock,
 				       flags);
@@ -5136,6 +5139,7 @@ static bool ipmi_timeout_handler(struct ipmi_smi *intf,
 			    && (intf->auto_maintenance_timeout <= 0)) {
 				intf->maintenance_mode_state =
 					IPMI_MAINTENANCE_MODE_STATE_OFF;
+				intf->auto_maintenance_timeout = 0;
 				maintenance_mode_update(intf);
 			}
 		}
@@ -5158,8 +5162,6 @@ static void ipmi_request_event(struct ipmi_smi *intf)
 		intf->handlers->request_events(intf->send_info);
 }
 
-static struct timer_list ipmi_timer;
-
 static atomic_t stop_operation;
 
 static void ipmi_timeout_work(struct work_struct *work)
@@ -5183,6 +5185,8 @@ static void ipmi_timeout_work(struct work_struct *work)
 			}
 			need_timer = true;
 		}
+		if (intf->maintenance_mode_state)
+			need_timer = true;
 
 		need_timer |= ipmi_timeout_handler(intf, IPMI_TIMEOUT_TIME);
 	}
-- 
2.43.0

Re: [PATCH 2/4] ipmi: Disable sysfs access and requests in maintenance mode

[Frederick Lawler]

Hi Corey,

On Thu, Aug 07, 2025 at 06:02:33PM -0500, Corey Minyard wrote:
> If the driver goes into any maintenance mode, disable sysfs access until
> it is done.
>

Why specifically sysfs reads during FW update state? Is there an expectation
that during a FW update, that redfish/ipmi/etc... are chunking/buffering the
FW payloads to the device, thus needs write access? I'm assuming that the
device is blocking waiting for paylods to finish, so sending additional messages
just get ignored?

> If the driver goes into reset maintenance mode, disable all messages
> until it is done.
> 
> Signed-off-by: Corey Minyard <corey@minyard.net>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index f124c0b33db8..72f5f4a0c056 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -2338,6 +2338,11 @@ static int i_ipmi_request(struct ipmi_user     *user,
>  
>  	if (!run_to_completion)
>  		mutex_lock(&intf->users_mutex);
> +	if (intf->maintenance_mode_state == IPMI_MAINTENANCE_MODE_STATE_RESET) {
> +		/* No messages while the BMC is in reset. */
> +		rv = -EBUSY;
> +		goto out_err;
> +	}
>  	if (intf->in_shutdown) {
>  		rv = -ENODEV;
>  		goto out_err;
> @@ -2639,6 +2644,12 @@ static int __bmc_get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc,
>  	    (bmc->dyn_id_set && time_is_after_jiffies(bmc->dyn_id_expiry)))
>  		goto out_noprocessing;
>  
> +	/* Don't allow sysfs access when in maintenance mode. */
> +	if (intf->maintenance_mode_state) {
> +		rv = -EBUSY;
> +		goto out_noprocessing;
> +	}
> +
>  	prev_guid_set = bmc->dyn_guid_set;
>  	__get_guid(intf);
>  
> -- 
> 2.43.0
> 

Best, Fred

Re: [PATCH 2/4] ipmi: Disable sysfs access and requests in maintenance mode

[Corey Minyard]

On Fri, Aug 08, 2025 at 03:37:51PM -0500, Frederick Lawler wrote:
> Hi Corey,
> 
> On Thu, Aug 07, 2025 at 06:02:33PM -0500, Corey Minyard wrote:
> > If the driver goes into any maintenance mode, disable sysfs access until
> > it is done.
> >
> 
> Why specifically sysfs reads during FW update state? Is there an expectation
> that during a FW update, that redfish/ipmi/etc... are chunking/buffering the
> FW payloads to the device, thus needs write access? I'm assuming that the
> device is blocking waiting for paylods to finish, so sending additional messages
> just get ignored?

In my experience, when the BMC goes into firmware update mode, it
doesn't behave normally.  But it's just my experience.  It general, it's
best not to mess with something during an update.

-corey

> 
> > If the driver goes into reset maintenance mode, disable all messages
> > until it is done.
> > 
> > Signed-off-by: Corey Minyard <corey@minyard.net>
> > ---
> >  drivers/char/ipmi/ipmi_msghandler.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> > index f124c0b33db8..72f5f4a0c056 100644
> > --- a/drivers/char/ipmi/ipmi_msghandler.c
> > +++ b/drivers/char/ipmi/ipmi_msghandler.c
> > @@ -2338,6 +2338,11 @@ static int i_ipmi_request(struct ipmi_user     *user,
> >  
> >  	if (!run_to_completion)
> >  		mutex_lock(&intf->users_mutex);
> > +	if (intf->maintenance_mode_state == IPMI_MAINTENANCE_MODE_STATE_RESET) {
> > +		/* No messages while the BMC is in reset. */
> > +		rv = -EBUSY;
> > +		goto out_err;
> > +	}
> >  	if (intf->in_shutdown) {
> >  		rv = -ENODEV;
> >  		goto out_err;
> > @@ -2639,6 +2644,12 @@ static int __bmc_get_device_id(struct ipmi_smi *intf, struct bmc_device *bmc,
> >  	    (bmc->dyn_id_set && time_is_after_jiffies(bmc->dyn_id_expiry)))
> >  		goto out_noprocessing;
> >  
> > +	/* Don't allow sysfs access when in maintenance mode. */
> > +	if (intf->maintenance_mode_state) {
> > +		rv = -EBUSY;
> > +		goto out_noprocessing;
> > +	}
> > +
> >  	prev_guid_set = bmc->dyn_guid_set;
> >  	__get_guid(intf);
> >  
> > -- 
> > 2.43.0
> > 
> 
> Best, Fred

Re: [RFC] Patches to disable messages during BMC reset

[Frederick Lawler]

Hi Corey,

On Thu, Aug 07, 2025 at 06:02:31PM -0500, Corey Minyard wrote:
> I went ahead and did some patches for this, since it was on my mind.
> 
> With these, if a reset is sent to the BMC, the driver will disable
> messages to the BMC for a time, defaulting to 30 seconds.  Don't
> modify message timing, since no messages are allowed, anyway.
> 
> If a firmware update command is sent to the BMC, then just reject
> sysfs commands that query the BMC.  Modify message timing and
> allow direct messages through the driver interface.
> 
> Hopefully this will work around the problem, and it's a good idea,
> anyway.
> 
> -corey
> 

Thanks for the patches, and sorry for the delay in response.
It's one of _those weeks_. Anyway, I backported the patch series
to 6.12, and the changes seem reasonable to me overall. Ran it
through our infra on a single node, and nothing seemed to break.

I did observe with testing that resetting BMC via ipmitool on the host
did kick out sysfs reads as expected.

Resetting the BMC remotely, was not handled (this seems obvious given the state
changes are handled via ipmi_msg handler). Would the BMC send an event
to the kernel letting it know its resetting so that case could be
handled?

Best,
Fred

Re: [RFC] Patches to disable messages during BMC reset

[Corey Minyard]

On Fri, Aug 15, 2025 at 04:23:08PM -0500, Frederick Lawler wrote:
> Hi Corey,
> 
> On Thu, Aug 07, 2025 at 06:02:31PM -0500, Corey Minyard wrote:
> > I went ahead and did some patches for this, since it was on my mind.
> > 
> > With these, if a reset is sent to the BMC, the driver will disable
> > messages to the BMC for a time, defaulting to 30 seconds.  Don't
> > modify message timing, since no messages are allowed, anyway.
> > 
> > If a firmware update command is sent to the BMC, then just reject
> > sysfs commands that query the BMC.  Modify message timing and
> > allow direct messages through the driver interface.
> > 
> > Hopefully this will work around the problem, and it's a good idea,
> > anyway.
> > 
> > -corey
> > 
> 
> Thanks for the patches, and sorry for the delay in response.
> It's one of _those weeks_. Anyway, I backported the patch series
> to 6.12, and the changes seem reasonable to me overall. Ran it
> through our infra on a single node, and nothing seemed to break.
> 
> I did observe with testing that resetting BMC via ipmitool on the host
> did kick out sysfs reads as expected.

Ok, I took the liberty of adding a "Tested-by" line with your name.  If
that's not ok, I can pull it out.

> 
> Resetting the BMC remotely, was not handled (this seems obvious given the state
> changes are handled via ipmi_msg handler). Would the BMC send an event
> to the kernel letting it know its resetting so that case could be
> handled?

Unfortunately not.  It's one of the many things that would be nice to
have...

In general, dealing with a BMC being reset is a real pain.  They tend to
do all kinds of different things.  The worst is when they sort of act
like they are operational, but then do strange things.

I haven't thought of a good general purpose way to handle this.  I'm
toying with the idea of making it so if the BMC gets an error, just shut
things down for a second or so and then test it to see if it's working.
During this time just return errors, like the new patches do during
reset.

Thanks for testing these.

-corey

> 
> Best,
> Fred

Re: [RFC] Patches to disable messages during BMC reset

[Frederick Lawler]

On Fri, Aug 15, 2025 at 08:56:33PM -0500, Corey Minyard wrote:
> On Fri, Aug 15, 2025 at 04:23:08PM -0500, Frederick Lawler wrote:
> > Hi Corey,
> > 
> > On Thu, Aug 07, 2025 at 06:02:31PM -0500, Corey Minyard wrote:
> > > I went ahead and did some patches for this, since it was on my mind.
> > > 
> > > With these, if a reset is sent to the BMC, the driver will disable
> > > messages to the BMC for a time, defaulting to 30 seconds.  Don't
> > > modify message timing, since no messages are allowed, anyway.
> > > 
> > > If a firmware update command is sent to the BMC, then just reject
> > > sysfs commands that query the BMC.  Modify message timing and
> > > allow direct messages through the driver interface.
> > > 
> > > Hopefully this will work around the problem, and it's a good idea,
> > > anyway.
> > > 
> > > -corey
> > > 
> > 
> > Thanks for the patches, and sorry for the delay in response.
> > It's one of _those weeks_. Anyway, I backported the patch series
> > to 6.12, and the changes seem reasonable to me overall. Ran it
> > through our infra on a single node, and nothing seemed to break.
> > 
> > I did observe with testing that resetting BMC via ipmitool on the host
> > did kick out sysfs reads as expected.
> 
> Ok, I took the liberty of adding a "Tested-by" line with your name.  If
> that's not ok, I can pull it out.
>

Not a problem.

> > 
> > Resetting the BMC remotely, was not handled (this seems obvious given the state
> > changes are handled via ipmi_msg handler). Would the BMC send an event
> > to the kernel letting it know its resetting so that case could be
> > handled?
> 
> Unfortunately not.  It's one of the many things that would be nice to
> have...
> 
> In general, dealing with a BMC being reset is a real pain.  They tend to
> do all kinds of different things.  The worst is when they sort of act
> like they are operational, but then do strange things.
> 
> I haven't thought of a good general purpose way to handle this.  I'm
> toying with the idea of making it so if the BMC gets an error, just shut
> things down for a second or so and then test it to see if it's working.
> During this time just return errors, like the new patches do during
> reset.
> 
> Thanks for testing these.
> 
> -corey
>

Thanks for working with me on this.

> > 
> > Best,
> > Fred