diff mbox series

[v2,15/16] block: sed-opal: don't repeat opal_discovery0 in each steps array

Message ID 1547760716-7304-16-git-send-email-zub@linux.fjfi.cvut.cz (mailing list archive)
State New, archived
Headers show
Series block: sed-opal: support shadow MBR done flag and write | expand

Commit Message

David Kozub Jan. 17, 2019, 9:31 p.m. UTC 
Originally each of the opal functions that call next include
opal_discovery0 in the array of steps. This is superfluous and
can be done always inside next.

Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
---
 block/sed-opal.c | 88 +++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 46 deletions(-)

Comments

Scott Bauer Jan. 19, 2019, 5:46 p.m. UTC | #1 
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
> 
>
David Kozub Jan. 20, 2019, 8:23 p.m. UTC | #2 
On Sat, 19 Jan 2019, Scott Bauer wrote:

> 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.

Ouch! You're right. I'll fix this for v3 by comparing against 1.

There is one more issue that was bugging me. If next() fails at the 
discovery0 step, or at steps[0], in both cases the error message will say 
step 0 failed. But as it's just a pr_debug message, the function address 
is included and I don't see a short and nice solution (should I report the 
steps as starting from 1? but that might be confusing; or a different 
string? sounds like not worth it), I kept it that way.

But if someone thinks this is worth improving, please let me know.
diff mbox series

Patch

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 82ef81b66ed5..cedf4d12138d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -221,6 +221,7 @@  static const u8 opalmethod[][OPAL_METHOD_LENGTH] = {
 };
 
 static int end_opal_session_error(struct opal_dev *dev);
+static int opal_discovery0_step(struct opal_dev *dev);
 
 struct opal_suspend_data {
 	struct opal_lock_unlock unlk;
@@ -386,36 +387,41 @@  static void check_geometry(struct opal_dev *dev, const void *data)
 	dev->lowest_lba = geo->lowest_aligned_lba;
 }
 
+static int execute_step(struct opal_dev *dev,
+			const struct opal_step *step, size_t stepIndex)
+{
+	int error = step->fn(dev, step->data);
+
+	if (error) {
+		pr_debug("Step %zu (%pS) failed with error %d: %s\n",
+			 stepIndex, step->fn, error,
+			 opal_error_to_human(error));
+	}
+
+	return error;
+}
+
 static int next(struct opal_dev *dev, const struct opal_step *steps,
 		size_t n_steps)
 {
-	const struct opal_step *step;
 	size_t state;
-	int error = 0;
+	int error;
 
-	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);
 
 	return error;
 }
@@ -513,6 +519,14 @@  static int opal_discovery0(struct opal_dev *dev, void *data)
 	return opal_discovery0_end(dev);
 }
 
+static int opal_discovery0_step(struct opal_dev *dev)
+{
+	const struct opal_step discovery0_step = {
+		opal_discovery0,
+	};
+	return execute_step(dev, &discovery0_step, 0);
+}
+
 static size_t remaining_size(struct opal_dev *cmd)
 {
 	return IO_BUFFER_LENGTH - cmd->pos;
@@ -1937,10 +1951,10 @@  static int end_opal_session(struct opal_dev *dev, void *data)
 
 static int end_opal_session_error(struct opal_dev *dev)
 {
-	const struct opal_step error_end_session[] = {
-		{ end_opal_session, }
+	const struct opal_step error_end_session = {
+		end_opal_session,
 	};
-	return next(dev, error_end_session, ARRAY_SIZE(error_end_session));
+	return execute_step(dev, &error_end_session, 0);
 }
 
 static inline void setup_opal_dev(struct opal_dev *dev)
@@ -1952,14 +1966,11 @@  static inline void setup_opal_dev(struct opal_dev *dev)
 
 static int check_opal_support(struct opal_dev *dev)
 {
-	const struct opal_step steps[] = {
-		{ opal_discovery0, }
-	};
 	int ret;
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, steps, ARRAY_SIZE(steps));
+	ret = opal_discovery0_step(dev);
 	dev->supported = !ret;
 	mutex_unlock(&dev->dev_lock);
 	return ret;
@@ -2012,7 +2023,6 @@  static int opal_secure_erase_locking_range(struct opal_dev *dev,
 					   struct opal_session_info *opal_session)
 {
 	const struct opal_step erase_steps[] = {
-		{ opal_discovery0, },
 		{ start_auth_opal_session, opal_session },
 		{ get_active_key, &opal_session->opal_key.lr },
 		{ gen_key, },
@@ -2031,7 +2041,6 @@  static int opal_erase_locking_range(struct opal_dev *dev,
 				    struct opal_session_info *opal_session)
 {
 	const struct opal_step erase_steps[] = {
-		{ opal_discovery0, },
 		{ start_auth_opal_session, opal_session },
 		{ erase_locking_range, opal_session },
 		{ end_opal_session, }
@@ -2051,7 +2060,6 @@  static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 	u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
 		? OPAL_TRUE : OPAL_FALSE;
 	const struct opal_step mbr_steps[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &opal_mbr->key },
 		{ set_mbr_done, &token },
 		{ end_opal_session, },
@@ -2077,7 +2085,6 @@  static int opal_mbr_status(struct opal_dev *dev, struct opal_mbr_data *opal_mbr)
 	u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
 		? OPAL_TRUE : OPAL_FALSE;
 	const struct opal_step mbr_steps[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &opal_mbr->key },
 		{ set_mbr_done, &token },
 		{ end_opal_session, }
@@ -2099,7 +2106,6 @@  static int opal_write_shadow_mbr(struct opal_dev *dev,
 				 struct opal_shadow_mbr *info)
 {
 	const struct opal_step mbr_steps[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &info->key },
 		{ write_shadow_mbr, info },
 		{ end_opal_session, }
@@ -2141,7 +2147,6 @@  static int opal_add_user_to_lr(struct opal_dev *dev,
 			       struct opal_lock_unlock *lk_unlk)
 {
 	const struct opal_step steps[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &lk_unlk->session.opal_key },
 		{ add_user_to_lr, lk_unlk },
 		{ end_opal_session, }
@@ -2175,7 +2180,6 @@  static int opal_add_user_to_lr(struct opal_dev *dev,
 static int opal_reverttper(struct opal_dev *dev, struct opal_key *opal)
 {
 	const struct opal_step revert_steps[] = {
-		{ opal_discovery0, },
 		{ start_SIDASP_opal_session, opal },
 		{ revert_tper, } /* controller will terminate session */
 	};
@@ -2200,13 +2204,11 @@  static int __opal_lock_unlock(struct opal_dev *dev,
 			      struct opal_lock_unlock *lk_unlk)
 {
 	const struct opal_step unlock_steps[] = {
-		{ opal_discovery0, },
 		{ start_auth_opal_session, &lk_unlk->session },
 		{ lock_unlock_locking_range, lk_unlk },
 		{ end_opal_session, }
 	};
 	const struct opal_step unlock_sum_steps[] = {
-		{ opal_discovery0, },
 		{ start_auth_opal_session, &lk_unlk->session },
 		{ lock_unlock_locking_range_sum, lk_unlk },
 		{ end_opal_session, }
@@ -2223,7 +2225,6 @@  static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
 {
 	u8 mbr_done_tf = 1;
 	const struct opal_step mbrdone_step[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, key },
 		{ set_mbr_done, &mbr_done_tf },
 		{ end_opal_session, }
@@ -2250,7 +2251,6 @@  static int opal_lock_unlock(struct opal_dev *dev,
 static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
 {
 	const struct opal_step owner_steps[] = {
-		{ opal_discovery0, },
 		{ start_anybodyASP_opal_session, },
 		{ get_msid_cpin_pin, },
 		{ end_opal_session, },
@@ -2274,7 +2274,6 @@  static int opal_activate_lsp(struct opal_dev *dev,
 			     struct opal_lr_act *opal_lr_act)
 {
 	const struct opal_step active_steps[] = {
-		{ opal_discovery0, },
 		{ start_SIDASP_opal_session, &opal_lr_act->key },
 		{ get_lsp_lifecycle, },
 		{ activate_lsp, opal_lr_act },
@@ -2296,7 +2295,6 @@  static int opal_setup_locking_range(struct opal_dev *dev,
 				    struct opal_user_lr_setup *opal_lrs)
 {
 	const struct opal_step lr_steps[] = {
-		{ opal_discovery0, },
 		{ start_auth_opal_session, &opal_lrs->session },
 		{ setup_locking_range, opal_lrs },
 		{ end_opal_session, }
@@ -2313,7 +2311,6 @@  static int opal_setup_locking_range(struct opal_dev *dev,
 static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
 {
 	const struct opal_step pw_steps[] = {
-		{ opal_discovery0, },
 		{ start_auth_opal_session, &opal_pw->session },
 		{ set_new_pw, &opal_pw->new_user_pw },
 		{ end_opal_session, }
@@ -2337,7 +2334,6 @@  static int opal_activate_user(struct opal_dev *dev,
 			      struct opal_session_info *opal_session)
 {
 	const struct opal_step act_steps[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &opal_session->opal_key },
 		{ internal_activate_user, opal_session },
 		{ end_opal_session, }