Messages in this thread | | | From | Patrick Venture <> | Date | Tue, 18 Sep 2018 11:42:40 -0700 | Subject | Re: [PATCH v2] ipmi: looped device detection |
| |
On Wed, Sep 12, 2018 at 3:54 PM Patrick Venture <venture@google.com> wrote: > > On Wed, Sep 12, 2018 at 3:10 PM Corey Minyard <minyard@acm.org> wrote: > > > > On 09/11/2018 05:56 PM, Patrick Venture wrote: > > > Try to get the device ID repeatedly during initialization before giving up. > > > The BMC isn't always responsive, and this allows it to be slightly flaky > > > during early boot. > > > > > > Tested: Installed on a system with the BMC software disabled > > > such that it was non-responsive. The driver correctly detected this > > > and gave up as expected. Then I re-enabled the BMC software unloaded > > > and reloaded the driver and it was detected properly. > > > > The patch looks fine, but I wonder if this is something that is really > > valuable. > > I have wondered about this before. > > > > The question is: If the BMC is unavailable, what are the chances of it > > becoming > > available by the time you do 5 attempts? I would guess that is a pretty > > small > > chance, which is why I haven't done this already.
Friendly ping. I'd like to get a sense of whether you're likely to accept this. If not, it's fine, will close out patch in current downstream rebase.
Thanks
> > This patch was actually critical for us to provide a reliable IPMI > interface. The version of OpenBMC or the state of the BMC at the > point the kernel was loading was flaky, so following the example in > the BIOS source, we just re-try a few times. We also can hold boot X > seconds until it's responding, but, this avoided some issues inherent > with that. > > > > > You could have something that re-tested periodically, but there are so many > > systems with IPMI specified in ACPI or SMBIOS that is wrong, and it would > > try forever. Also not really a good thing. > > If we did a periodic check, it could check X times, but I felt going > for a simple solution was ideal -- and this idea was proved out on a > few platforms. We have other drivers that are loaded by the kernel > (not at run-time) and they depend on IPMI, and without this patch they > would then have a non-trivial probability of failure. > > > > > So I've left it to reload the driver or use the hotmod interface. > > > > -corey > > > > > Signed-off-by: Patrick Venture <venture@google.com> > > > --- > > > v2: > > > - removed extra variable that was set but not used. > > > --- > > > drivers/char/ipmi/ipmi_si_intf.c | 23 ++++++++++++++++++++++- > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c > > > index 90ec010bffbd..5fed96897fe8 100644 > > > --- a/drivers/char/ipmi/ipmi_si_intf.c > > > +++ b/drivers/char/ipmi/ipmi_si_intf.c > > > @@ -1918,11 +1918,13 @@ int ipmi_si_add_smi(struct si_sm_io *io) > > > * held, primarily to keep smi_num consistent, we only one to do these > > > * one at a time. > > > */ > > > +#define GET_DEVICE_ID_ATTEMPTS 5 > > > static int try_smi_init(struct smi_info *new_smi) > > > { > > > int rv = 0; > > > int i; > > > char *init_name = NULL; > > > + unsigned long sleep_rm; > > > > > > pr_info(PFX "Trying %s-specified %s state machine at %s address 0x%lx, slave address 0x%x, irq %d\n", > > > ipmi_addr_src_to_str(new_smi->io.addr_source), > > > @@ -2003,7 +2005,26 @@ static int try_smi_init(struct smi_info *new_smi) > > > * Attempt a get device id command. If it fails, we probably > > > * don't have a BMC here. > > > */ > > > - rv = try_get_dev_id(new_smi); > > > + for (i = 0; i < GET_DEVICE_ID_ATTEMPTS; i++) { > > > + pr_info(PFX "Attempting to read BMC device ID\n"); > > > + rv = try_get_dev_id(new_smi); > > > + /* If it succeeded, stop trying */ > > > + if (!rv) > > > + break; > > > + > > > + /* Sleep for ~0.25s before trying again instead of hammering > > > + * the BMC. > > > + */ > > > + sleep_rm = msleep_interruptible(250); > > > + if (sleep_rm != 0) { > > > + pr_info(PFX "Find BMC interrupted\n"); > > > + rv = -EINTR; > > > + goto out_err; > > > + } > > > + } > > > + > > > + /* If we exited the loop above and rv is non-zero we ran out of tries. > > > + */ > > > if (rv) { > > > if (new_smi->io.addr_source) > > > dev_err(new_smi->io.dev, > > > >
| |