Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: minyard@acm.org
Subject: Re: [PATCH v2] ipmi: looped device detection
To:     Patrick Venture <venture@google.com>, arnd@arndb.de,
        gregkh@linuxfoundation.org
Cc:     openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org
References: <20180911225630.124502-1-venture@google.com>
From:   Corey Minyard <minyard@acm.org>
Message-ID: <585d1c3a-6121-c20d-f6d6-7567595cd1af@acm.org>
Date:   Wed, 12 Sep 2018 17:09:58 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180911225630.124502-1-venture@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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.

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,