On Thu, Aug 22, 2024 at 10:22:55AM +0300, Ivan T. Ivanov wrote:
> 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 <iiva...@suse.de>
> > Signed-off-by: Corey Minyard <co...@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.
That's not how the i2c code works. See my next comment.
>
> > - else
> > + else {
> > + if (len < 3) {
> > + rv = -ENODEV;
>
> No point to call probe(), right?
Originally (before I add the call from ssif_probe()), this is not involved in
the probe() call. Instead, the detect function is involved in calling a
table of addresses in driver->address_list. So in this case this
function is never called at all from the i2c code, since there is no
address list.
>
> > + } 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.
It is possible that the BMC gets rebooted or something between the call
to ssif_detect() and the code below, but the probability is really low.
If it answers a detect, the rest of the things should work.
-corey
>
>
> > /* 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
>
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
