Re: [Openipmi-developer] [PATCH] ipmi:ssif: Exit early when there is a SMBus error

[Corey M. <co...@mi...>]

On Fri, Aug 16, 2024 at 09:54:58AM +0300, Ivan T. Ivanov wrote:
> It is pointless to continue module probing when communication
> with device is failing. This just fill logs with misleading
> messages like this:

So the BMC (or whatever is there) responds to a GET_DEVICE_ID command,
but then doesn't properly handle any other valid and mandatory commands?
And this device was added via ACPI or SMBIOS or device tree, almost
certainly.

My comments are:

1) This fix is wrong, because it may mask important things that need to
be reported.

2) Fix the source of the problem.  You can't expect software to
compensate for all bad hardware and firmware.  I'd guess the firmware
tables are pointing to something that's not a BMC.

3) It appears the response to the GET_DEVICE_ID command, though a
response is returned, is not valid.  The right way to handle this would
be to do more validation in the ssif_detect() function.  It doesn't do
any validation of the response data, and that's really what needs to be
done.

-corey

> 
> [Fri Jul 26 18:32:34 2024] ipmi_ssif i2c-IPI0001:00: ipmi_ssif: Error fetching SSIF: -121 180453376 62, your system probably doesn't support this command so using defaults
> [Fri Jul 26 18:32:54 2024] ipmi_ssif i2c-IPI0001:00: ipmi_ssif: Unable to clear message flags: -121 180453376 62
> [Fri Jul 26 18:33:14 2024] ipmi_ssif i2c-IPI0001:00: ipmi_ssif: Error getting global enables: -121 180453376 62
> [Fri Jul 26 18:33:49 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: device id demangle failed: -22
> [Fri Jul 26 18:33:50 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: BMC returned 0xff, retry get bmc device id
> [Fri Jul 26 18:34:07 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: device id demangle failed: -22
> [Fri Jul 26 18:34:07 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: BMC returned 0xff, retry get bmc device id
> [Fri Jul 26 18:34:25 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: device id demangle failed: -22
> [Fri Jul 26 18:34:25 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: BMC returned 0xff, retry get bmc device id
> [Fri Jul 26 18:34:43 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: device id demangle failed: -22
> [Fri Jul 26 18:34:43 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: BMC returned 0xff, retry get bmc device id
> [Fri Jul 26 18:35:01 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: device id demangle failed: -22
> [Fri Jul 26 18:35:01 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: BMC returned 0xff, retry get bmc device id
> [Fri Jul 26 18:35:19 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: device id demangle failed: -22
> [Fri Jul 26 18:35:19 2024] ipmi_ssif i2c-IPI0001:00: IPMI message handler: Unable to get the device id: -5
> [Fri Jul 26 18:35:19 2024] ipmi_ssif i2c-IPI0001:00: ipmi_ssif: Unable to register device: error -5
> [Fri Jul 26 18:35:19 2024] ipmi_ssif i2c-IPI0001:00: ipmi_ssif: Unable to start IPMI SSIF: -5
> [Fri Jul 26 18:35:19 2024] ipmi_ssif: probe of i2c-IPI0001:00 failed with error -5
> 
> Also in some of these prints uninitialized variables are used.
> So just exit early when communication with device is flawed.
> 
> Signed-off-by: Ivan T. Ivanov <ii...@su...>
> ---
>  drivers/char/ipmi/ipmi_ssif.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index 96ad571d041a..37516733e5c8 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1315,6 +1315,16 @@ static int read_response(struct i2c_client *client, unsigned char *resp)
>  	return ret;
>  }
>  
> +/* Filter SMBus communication errors from incorrect response errors */
> +static bool is_smbus_error(struct device *dev, int err)
> +{
> +	if (!err || err == -EINVAL || err == -E2BIG)
> +		return false;
> +
> +	dev_err(dev, "SMbus error: %d\n", err);
> +	return true;
> +}
> +
>  static int do_cmd(struct i2c_client *client, int len, unsigned char *msg,
>  		  int *resp_len, unsigned char *resp)
>  {
> @@ -1709,6 +1719,8 @@ static int ssif_probe(struct i2c_client *client)
>  	msg[1] = IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD;
>  	msg[2] = 0; /* SSIF */
>  	rv = do_cmd(client, 3, msg, &len, resp);
> +	if (is_smbus_error(&client->dev, rv))
> +		goto out;
>  	if (!rv && (len >= 3) && (resp[2] == 0)) {
>  		if (len < 7) {
>  			if (ssif_dbg_probe)
> @@ -1767,6 +1779,8 @@ static int ssif_probe(struct i2c_client *client)
>  	msg[1] = IPMI_CLEAR_MSG_FLAGS_CMD;
>  	msg[2] = WDT_PRE_TIMEOUT_INT;
>  	rv = do_cmd(client, 3, msg, &len, resp);
> +	if (is_smbus_error(&client->dev, rv))
> +		goto out;
>  	if (rv || (len < 3) || (resp[2] != 0))
>  		dev_warn(&ssif_info->client->dev,
>  			 "Unable to clear message flags: %d %d %2.2x\n",
> @@ -1776,6 +1790,8 @@ static int ssif_probe(struct i2c_client *client)
>  	msg[0] = IPMI_NETFN_APP_REQUEST << 2;
>  	msg[1] = IPMI_GET_BMC_GLOBAL_ENABLES_CMD;
>  	rv = do_cmd(client, 2, msg, &len, resp);
> +	if (is_smbus_error(&client->dev, rv))
> +		goto out;
>  	if (rv || (len < 4) || (resp[2] != 0)) {
>  		dev_warn(&ssif_info->client->dev,
>  			 "Error getting global enables: %d %d %2.2x\n",
> @@ -1796,6 +1812,8 @@ static int ssif_probe(struct i2c_client *client)
>  	msg[1] = IPMI_SET_BMC_GLOBAL_ENABLES_CMD;
>  	msg[2] = ssif_info->global_enables | IPMI_BMC_EVT_MSG_BUFF;
>  	rv = do_cmd(client, 3, msg, &len, resp);
> +	if (is_smbus_error(&client->dev, rv))
> +		goto out;
>  	if (rv || (len < 2)) {
>  		dev_warn(&ssif_info->client->dev,
>  			 "Error setting global enables: %d %d %2.2x\n",
> @@ -1818,6 +1836,8 @@ static int ssif_probe(struct i2c_client *client)
>  	msg[1] = IPMI_SET_BMC_GLOBAL_ENABLES_CMD;
>  	msg[2] = ssif_info->global_enables | IPMI_BMC_RCV_MSG_INTR;
>  	rv = do_cmd(client, 3, msg, &len, resp);
> +	if (is_smbus_error(&client->dev, rv))
> +		goto out;
>  	if (rv || (len < 2)) {
>  		dev_warn(&ssif_info->client->dev,
>  			 "Error setting global enables: %d %d %2.2x\n",
> -- 
> 2.43.0
>