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;
Date:   Sat, 19 Jan 2019 12:46:33 -0500
From:   Scott Bauer <sbauer@plzdonthack.me>
To:     David Kozub <zub@linux.fjfi.cvut.cz>
Cc:     linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        hch@infradead.org, jonathan.derrick@intel.com
Message-ID: <20190119174633.GA15276@hacktheplanet>
References: <1547760716-7304-16-git-send-email-zub@linux.fjfi.cvut.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1547760716-7304-16-git-send-email-zub@linux.fjfi.cvut.cz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Subject: Re: [PATCH v2 15/16] block: sed-opal: don't repeat opal_discovery0
 in each steps array
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 17, 2019 at 09:31:55PM +0000, David Kozub wrote:

> -	for (state = 0; !error && state < n_steps; state++) {
> -		step = &steps[state];
> -
> -		error = step->fn(dev, step->data);
> -		if (error) {
> -			pr_debug("Step %zu (%pS) failed with error %d: %s\n",
> -				 state, step->fn, error,
> -				 opal_error_to_human(error));
> -
> -			/* For each OPAL command we do a discovery0 then we
> -			 * start some sort of session.
> -			 * If we haven't passed state 1 then there was an error
> -			 * on discovery0 or during the attempt to start a
> -			 * session. Therefore we shouldn't attempt to terminate
> -			 * a session, as one has not yet been created.
> -			 */
> -			if (state > 1) {
> -				end_opal_session_error(dev);
> -				return error;
> -			}


> +	/* first do a discovery0 */
> +	error = opal_discovery0_step(dev);
>  
> -		}
> -	}
> +	for (state = 0; !error && state < n_steps; state++)
> +		error = execute_step(dev, &steps[state], state);
> +
> +	/* For each OPAL command the first step in steps starts some sort
> +	 * of session. If an error occurred in the initial discovery0 or if
> +	 * an error stopped the loop in state 0 then there was an error
> +	 * before or during the attempt to start a session. Therefore we
> +	 * shouldn't attempt to terminate a session, as one has not yet
> +	 * been created.
> +	 */
> +	if (error && state > 0)
> +		end_opal_session_error(dev);


This is subtly wrong. There was some state that was encoded by having the
loop the way we did.

the tl;dr is the check needs to be if (error && state > 1) still.

The why is that in the previous implementation we wanted to end_opal_session_error
only if a successful discovery0 AND a successful start_*_session. In the previous loop,
discovery0 would complete, we'd do state++, then we'd go and attempt to start our
session. If we failed on that session starting we'd still be in state 1, and wouldn't
attempt to close the session.

In the current form, discovery0 is gone, so start session is on state 0. If we fail
on the start session, we set error = true, state gets ++'d, then we look at !error
and we don't loop again.

We go down to the check and attempt to end a session that was never started.


> -- 
> 2.20.1
> 
>