Re: [PATCH] ipmi:ssif: Improve detecting during probing

["Ivan T. Ivanov" <iivanov@suse.de>]

Hi Corey,

On 08-20 20:05, Corey Minyard wrote:
> 
> If an IPMI SSIF device is probed and there is something there, but
> probably not an actual BMC, the code would just issue a lot of errors
> before it failed.  We kind of need these errors to help with certain
> issues, and some of the failure reports are non-fatal.
> 
> However, a get device id command should alway work.  If that fails,
> nothing else is going to work and it's a pretty good indication that
> there's no valid BMC there.  So issue and check that command and bail
> if it fails.
> 
> Reported-by: Ivan T. Ivanov <iivanov@suse.de>
> Signed-off-by: Corey Minyard <corey@minyard.net>
> ---
>  drivers/char/ipmi/ipmi_ssif.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> Ivan, is it possible for you to test this patch on the broken system?

This exact system is not available to me at the moment. I have few
other machines on which I could test this.

> It should work based on what you reported, but it's nice to be sure.
> 
> Also, I discovered that the detect function is kind of bogus, it only
> works on an address list that isn't present (any more).  However, I
> re-used it for my purposes in the probe function.
> 
> Thanks.
> 
> diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> index e8e7b832c060..4c403e7a9fc8 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -1368,8 +1368,20 @@ static int ssif_detect(struct i2c_client *client, struct i2c_board_info *info)
>  	rv = do_cmd(client, 2, msg, &len, resp);
>  	if (rv)
>  		rv = -ENODEV;

What is my worry is that in case of SMBus errors, device is there but
for some reason it got stuck/crashed or whatever, so will get out of
detect function from here and with ENODEV return code probe function
will be called for no reason.

> -	else
> +	else {
> +	    if (len < 3) {
> +		rv = -ENODEV;

No point to call probe(), right?

> +	    } else {
> +		struct ipmi_device_id id;
> +
> +		rv = ipmi_demangle_device_id(resp[0] >> 2, resp[1],
> +					     resp + 2, len - 2, &id);
> +		if (rv)
> +		    rv = -ENODEV; /* Error means a BMC probably isn't there. */

Same.

> +	    }
> +	    if (!rv && info)
>  		strscpy(info->type, DEVICE_NAME, I2C_NAME_SIZE);
> +	}
>  	kfree(resp);
>  	return rv;
>  }
> @@ -1704,6 +1716,16 @@ static int ssif_probe(struct i2c_client *client)
>  		ipmi_addr_src_to_str(ssif_info->addr_source),
>  		client->addr, client->adapter->name, slave_addr);
>  
> +	/*
> +	 * Send a get device id command and validate its response to
> +	 * make sure a valid BMC is there.
> +	 */
> +	rv = ssif_detect(client, NULL);
> +	if (rv) {
> +		dev_err(&client->dev, "Not present\n");
> +		goto out;
> +	}
> +

The point is that even after this point IPMI device can start failing
to properly communicate with the OS, real SMBus errors, like EREMOTEIO
in my case, but unfortunately code bellow do not handle this very well,
I think.


>  	/* Now check for system interface capabilities */
>  	msg[0] = IPMI_NETFN_APP_REQUEST << 2;
>  	msg[1] = IPMI_GET_SYSTEM_INTERFACE_CAPABILITIES_CMD;
> -- 
> 2.34.1
> 

Regards,
Ivan