Adding support of skipping ARM SBMR boot progress response, to avoid
SSIF state machine problem if host chosse not read back the response.
Potin Lai (2):
bindings: ipmi: Add property for skipping SBMR boot progress response
ipmi: ssif_bmc: support skipping ARM SBMR bootprogress response
.../devicetree/bindings/ipmi/ssif-bmc.yaml | 5 ++++
drivers/char/ipmi/ssif_bmc.c | 25 +++++++++++++++++++
2 files changed, 30 insertions(+)
--
2.31.1
In ARM Server Base Manageability Requirements (SBMR) document, Callers can
choose to not read back Response Data after sending the command "Send Boot
Progress Code".
Define "arm-sbmr,skip-bootprogress-response" property for skipping the
response of "Send Boot Progress Code" from userspace.
Signed-off-by: Potin Lai <[email protected]>
---
Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml b/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
index 02b662d780bbb..b21e958efc184 100644
--- a/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
+++ b/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
@@ -19,6 +19,11 @@ properties:
reg:
maxItems: 1
+ arm-sbmr,skip-bootprogress-response:
+ type: boolean
+ description:
+ Skipping ARM SBMR “Send Boot Progress Code” response.
+
required:
- compatible
- reg
--
2.31.1
In ARM SBMR document, the host can chosse to not read back the response of
“Send Boot Progress Code” command.
To avoid SSIF being in a wrong state due to host not read back the
response, add the implementation of "arm-sbmr,skip-bootprogress-response"
property for skipping the response of "Send Boot Progress Code" command
from userspace.
Signed-off-by: Potin Lai <[email protected]>
---
drivers/char/ipmi/ssif_bmc.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 56346fb328727..3386a8bd18afd 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -39,6 +39,11 @@
#define SSIF_IPMI_MULTIPART_READ_START 0x3
#define SSIF_IPMI_MULTIPART_READ_MIDDLE 0x9
+#define GET_NETFN(netfn_lun) ((netfn_lun >> 2) & 0xfe)
+#define IPMI_GROUP_EXT_NETFN 0x2C
+#define IPMI_SBMR_GROUP 0xAE
+#define IPMI_SBMR_BOOTPROGRESS_CMD 0x02
+
/*
* IPMI 2.0 Spec, section 12.7 SSIF Timing,
* Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
@@ -102,6 +107,8 @@ struct ssif_bmc_ctx {
struct ssif_part_buffer part_buf;
struct ipmi_ssif_msg response;
struct ipmi_ssif_msg request;
+ /* Flag to skip response of Send Boot Progress Code */
+ bool skip_bootprogress_resp;
};
static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
@@ -187,6 +194,20 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
return -EINVAL;
spin_lock_irqsave(&ssif_bmc->lock, flags);
+ if (ssif_bmc->skip_bootprogress_resp &&
+ GET_NETFN(msg.payload[0]) == IPMI_GROUP_EXT_NETFN &&
+ msg.payload[1] == IPMI_SBMR_BOOTPROGRESS_CMD &&
+ msg.payload[3] == IPMI_SBMR_GROUP) {
+ if (ssif_bmc->response_timer_inited) {
+ del_timer(&ssif_bmc->response_timer);
+ ssif_bmc->response_timer_inited = false;
+ }
+ ssif_bmc->busy = false;
+ memset(&ssif_bmc->request, 0, sizeof(struct ipmi_ssif_msg));
+ spin_unlock_irqrestore(&ssif_bmc->lock, flags);
+ return count;
+ }
+
while (ssif_bmc->response_in_progress) {
spin_unlock_irqrestore(&ssif_bmc->lock, flags);
if (file->f_flags & O_NONBLOCK)
@@ -806,6 +827,10 @@ static int ssif_bmc_probe(struct i2c_client *client)
if (!ssif_bmc)
return -ENOMEM;
+ if (of_property_read_bool(client->dev.of_node,
+ "arm-sbmr,skip-bootprogress-response"))
+ ssif_bmc->skip_bootprogress_resp = true;
+
spin_lock_init(&ssif_bmc->lock);
init_waitqueue_head(&ssif_bmc->wait_queue);
--
2.31.1
On Wed, Jun 12, 2024 at 12:32:54PM +0800, Potin Lai wrote:
> In ARM Server Base Manageability Requirements (SBMR) document, Callers can
> choose to not read back Response Data after sending the command "Send Boot
> Progress Code".
Got a link to that document?
> Define "arm-sbmr,skip-bootprogress-response" property for skipping the
> response of "Send Boot Progress Code" from userspace.
I don't understand why this would be conditional? How can you define in
the BMC what the host behavior is? Doesn't the host side decide
that? So don't you always have to support no response?
>
> Signed-off-by: Potin Lai <[email protected]>
> ---
> Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml b/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
> index 02b662d780bbb..b21e958efc184 100644
> --- a/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
> +++ b/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
> @@ -19,6 +19,11 @@ properties:
> reg:
> maxItems: 1
>
> + arm-sbmr,skip-bootprogress-response:
Form is vendor,property-name where vendor is defined in
vendor-prefixes.yaml. 'arm-sbmr' is not a vendor.
> + type: boolean
> + description:
> + Skipping ARM SBMR “Send Boot Progress Code” response.
> +
> required:
> - compatible
> - reg
> --
> 2.31.1
>
On Thu, Jun 13, 2024 at 11:59:46AM -0600, Rob Herring wrote:
> On Wed, Jun 12, 2024 at 12:32:54PM +0800, Potin Lai wrote:
> > In ARM Server Base Manageability Requirements (SBMR) document, Callers can
> > choose to not read back Response Data after sending the command "Send Boot
> > Progress Code".
>
> Got a link to that document?
>
> > Define "arm-sbmr,skip-bootprogress-response" property for skipping the
> > response of "Send Boot Progress Code" from userspace.
>
> I don't understand why this would be conditional? How can you define in
> the BMC what the host behavior is? Doesn't the host side decide
> that? So don't you always have to support no response?
Yeah, this doesn't make any sense for two reasons:
What if the host wanted to read back the response? You make no
provision for that, as I believe Rob said above.
The BMC should be able to start a new transaction without the previous
response being read. So this should be pointless. If that's not
happening, it's a bug and should be fixed. Otherwise an untimely reset
could hang the SSIF interface.
-corey
>
> >
> > Signed-off-by: Potin Lai <[email protected]>
> > ---
> > Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml b/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
> > index 02b662d780bbb..b21e958efc184 100644
> > --- a/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
> > +++ b/Documentation/devicetree/bindings/ipmi/ssif-bmc.yaml
> > @@ -19,6 +19,11 @@ properties:
> > reg:
> > maxItems: 1
> >
> > + arm-sbmr,skip-bootprogress-response:
>
> Form is vendor,property-name where vendor is defined in
> vendor-prefixes.yaml. 'arm-sbmr' is not a vendor.
>
> > + type: boolean
> > + description:
> > + Skipping ARM SBMR “Send Boot Progress Code” response.
> > +
> > required:
> > - compatible
> > - reg
> > --
> > 2.31.1
> >
On 12/06/2024 11:32, Potin Lai wrote:
> In ARM SBMR document, the host can chosse to not read back the response of
> “Send Boot Progress Code” command.
>
Thanks for proposing a solution for the case.
As per my understanding from the ARM SBMR document, the "host can choose
not to read back the response of the "Send Boot Progress Code" command"
is from the implementation note (U) in Section F.1.1, which is to
provide guidance on the implementation, not a rule. This item also
clarifies some consequences if the host decides not to read back the
response.
There is also an information statement (I) in this section that
recommends reading back the response after sending "Send Boot Progress
Code." I found this statement suitable for SSIF, which was designed as
single-threaded from the beginning.
I'm not totally sure about this understanding yet so please let me know
if any.
Thank you,
- Quan
> To avoid SSIF being in a wrong state due to host not read back the
> response, add the implementation of "arm-sbmr,skip-bootprogress-response"
> property for skipping the response of "Send Boot Progress Code" command
> from userspace.
>
> Signed-off-by: Potin Lai <[email protected]>
> ---
> drivers/char/ipmi/ssif_bmc.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index 56346fb328727..3386a8bd18afd 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -39,6 +39,11 @@
> #define SSIF_IPMI_MULTIPART_READ_START 0x3
> #define SSIF_IPMI_MULTIPART_READ_MIDDLE 0x9
>
> +#define GET_NETFN(netfn_lun) ((netfn_lun >> 2) & 0xfe)
> +#define IPMI_GROUP_EXT_NETFN 0x2C
> +#define IPMI_SBMR_GROUP 0xAE
> +#define IPMI_SBMR_BOOTPROGRESS_CMD 0x02
> +
> /*
> * IPMI 2.0 Spec, section 12.7 SSIF Timing,
> * Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
> @@ -102,6 +107,8 @@ struct ssif_bmc_ctx {
> struct ssif_part_buffer part_buf;
> struct ipmi_ssif_msg response;
> struct ipmi_ssif_msg request;
> + /* Flag to skip response of Send Boot Progress Code */
> + bool skip_bootprogress_resp;
> };
>
> static inline struct ssif_bmc_ctx *to_ssif_bmc(struct file *file)
> @@ -187,6 +194,20 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
> return -EINVAL;
>
> spin_lock_irqsave(&ssif_bmc->lock, flags);
> + if (ssif_bmc->skip_bootprogress_resp &&
> + GET_NETFN(msg.payload[0]) == IPMI_GROUP_EXT_NETFN &&
> + msg.payload[1] == IPMI_SBMR_BOOTPROGRESS_CMD &&
> + msg.payload[3] == IPMI_SBMR_GROUP) {
> + if (ssif_bmc->response_timer_inited) {
> + del_timer(&ssif_bmc->response_timer);
> + ssif_bmc->response_timer_inited = false;
> + }
> + ssif_bmc->busy = false;
> + memset(&ssif_bmc->request, 0, sizeof(struct ipmi_ssif_msg));
> + spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> + return count;
> + }
> +
> while (ssif_bmc->response_in_progress) {
> spin_unlock_irqrestore(&ssif_bmc->lock, flags);
> if (file->f_flags & O_NONBLOCK)
> @@ -806,6 +827,10 @@ static int ssif_bmc_probe(struct i2c_client *client)
> if (!ssif_bmc)
> return -ENOMEM;
>
> + if (of_property_read_bool(client->dev.of_node,
> + "arm-sbmr,skip-bootprogress-response"))
> + ssif_bmc->skip_bootprogress_resp = true;
> +
> spin_lock_init(&ssif_bmc->lock);
>
> init_waitqueue_head(&ssif_bmc->wait_queue);