Messages in this thread | | | Subject | Re: [PATCH v2] ipmi: looped device detection | From | Corey Minyard <> | Date | Tue, 18 Sep 2018 16:37:11 -0500 |
| |
On 09/18/2018 01:42 PM, Patrick Venture wrote: > 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.
I'm ok with doing this, but I lied about the patch being fine, there are some issue. Well, I didn't lie, but I didn't look closely enough.
Can you use dev_xxx() instead of pr_xxx(). I know the driver isn't currently consistent, but there are a number of patches I have pending to make it better and it's a longer-term goal.
Can you make GET_DEVICE_ID_ATTEMPTS more specific, add IPMI_SI_ to the beginning or something.
I am not sure that I'm ok with waiting up to 1.25 seconds in the init function. As I mentioned before, a large number of systems have broken ACPI/SMBIOS information, and for those it will add 1.25 seconds to the boot time of every one of those systems. That won't make me a popular guy :-).
This is a harder problem to figure out what to do. To solve it properly would mean having a timer or thread drive this, and unload the module later if the process fails.
-corey
> 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, >>>
| |