lkml.org 
[lkml]   [2018]   [Sep]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] ipmi: looped device detection
From
Date
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,
>>>

\
 
 \ /
  Last update: 2018-09-18 23:38    [W:1.841 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site