[PATCH 00/16] sed-opal: fix shadow MBR enable/disable and clean up code

[PATCH 00/16] sed-opal: fix shadow MBR enable/disable and clean up code

[David Kozub]

This patch series contains various code cleanup and fixes for Opal SED support.
It's been created by taking a part of the original patch series by Jonas and me
(PATCHv4 block: sed-opal: support shadow MBR done flag and write) [1], as
suggested by Christoph in [2].

The most important patch is the first one where I tried to fix the shadow MBR
enable/disable issue we discussed in [3]. This change goes against Christoph's
original propsal in [4] but I think - in light of the issue and keeping in mind
the planned addition of an IOCTL specifically for toggling the done flag - that
passing just OPAL_TRUE or OPAL_FALSE to set_mbr_done and set_mbr_enable_disable
is more useful and also more understandable. Maybe this change is superfluous if
Scott found the time to submit his take on the fix. (?)

I tried to include all the feedback from the v4 review[1]. I also added some
more trivial changes (11/16 as suggested in [4]) and also 13/16 motivated by the
same idea - that's why I again reached the magical number of 16 patches.

I kept the reviewed-by/acked-by tags where the changes were trivial but I
removed them where I thought a re-review would be useful.

I plan to submit the remaining patches from the original series (these that add
new Opal IOCTLs) after this fix and cleanup is accepted.

I did a brief test toggling shadow MBR and unlocking a locking range. I will try
to do more thorough tests - but I will not get to it before the beginning of the
next week. It would be great if this could get some more testing. Especially the
unlock from suspend part - that's something I don't have set up and I have not
tested.

The series applies on v5.0-rc6.

[1] https://lore.kernel.org/lkml/1549054223-12220-1-git-send-email-zub@linux.fjfi.cvut.cz/
[2] https://lore.kernel.org/lkml/20190204150415.GO31132@infradead.org/
[3] https://lore.kernel.org/lkml/alpine.LRH.2.21.1902072247060.29258@linux.fjfi.cvut.cz/
[4] https://lore.kernel.org/lkml/20190204145244.GJ31132@infradead.org/

David Kozub (12):
  block: sed-opal: fix IOC_OPAL_ENABLE_DISABLE_MBR
  block: sed-opal: fix typos and formatting
  block: sed-opal: close parameter list in cmd_finalize
  block: sed-opal: unify cmd start
  block: sed-opal: unify error handling of responses
  block: sed-opal: reuse response_get_token to decrease code duplication
  block: sed-opal: add token for OPAL_LIFECYCLE
  block: sed-opal: unify retrieval of table columns
  block: sed-opal: use named Opal tokens instead of integer literals
  block: sed-opal: pass steps via argument rather than via opal_dev
  block: sed-opal: don't repeat opal_discovery0 in each steps array
  block: sed-opal: rename next to execute_steps

Jonas Rabenstein (4):
  block: sed-opal: use correct macro for method length
  block: sed-opal: unify space check in add_token_*
  block: sed-opal: print failed function address
  block: sed-opal: split generation of bytestring header and content

 block/opal_proto.h            |   2 +
 block/sed-opal.c              | 716 ++++++++++++++--------------------
 include/uapi/linux/sed-opal.h |   2 +-
 3 files changed, 287 insertions(+), 433 deletions(-)

-- 
2.20.1

[PATCH 01/16] block: sed-opal: fix IOC_OPAL_ENABLE_DISABLE_MBR

[David Kozub]

The implementation of IOC_OPAL_ENABLE_DISABLE_MBR handled the value
opal_mbr_data.enable_disable incorrectly: enable_disable is expected
to be one of OPAL_MBR_ENABLE(0) or OPAL_MBR_DISABLE(1). enable_disable
was passed directly to set_mbr_done and set_mbr_enable_disable where
is was interpreted as either OPAL_TRUE(1) or OPAL_FALSE(0). The end
result was that calling IOC_OPAL_ENABLE_DISABLE_MBR with OPAL_MBR_ENABLE
actually disabled the shadow MBR and vice versa.

This patch adds correct conversion from OPAL_MBR_DISABLE/ENABLE to
OPAL_FALSE/TRUE. The change affects existing programs using
IOC_OPAL_ENABLE_DISABLE_MBR but this is typically used only once when
setting up an Opal drive.

Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Acked-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index e0de4dd448b3..119640897293 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -2095,13 +2095,16 @@ static int opal_erase_locking_range(struct opal_dev *dev,
 static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 					  struct opal_mbr_data *opal_mbr)
 {
+	u8 enable_disable = 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, &opal_mbr->enable_disable },
+		{ set_mbr_done, &enable_disable },
 		{ end_opal_session, },
 		{ start_admin1LSP_opal_session, &opal_mbr->key },
-		{ set_mbr_enable_disable, &opal_mbr->enable_disable },
+		{ set_mbr_enable_disable, &enable_disable },
 		{ end_opal_session, },
 		{ NULL, }
 	};
@@ -2221,7 +2224,7 @@ static int __opal_lock_unlock(struct opal_dev *dev,
 
 static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
 {
-	u8 mbr_done_tf = 1;
+	u8 mbr_done_tf = OPAL_TRUE;
 	const struct opal_step mbrdone_step [] = {
 		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, key },
-- 
2.20.1

Re: [PATCH 01/16] block: sed-opal: fix IOC_OPAL_ENABLE_DISABLE_MBR

[Christoph Hellwig]

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 01/16] block: sed-opal: fix IOC_OPAL_ENABLE_DISABLE_MBR

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:15:53AM +0100, David Kozub wrote:
> The implementation of IOC_OPAL_ENABLE_DISABLE_MBR handled the value
> opal_mbr_data.enable_disable incorrectly: enable_disable is expected
> to be one of OPAL_MBR_ENABLE(0) or OPAL_MBR_DISABLE(1). enable_disable
> was passed directly to set_mbr_done and set_mbr_enable_disable where
> is was interpreted as either OPAL_TRUE(1) or OPAL_FALSE(0). The end
> result was that calling IOC_OPAL_ENABLE_DISABLE_MBR with OPAL_MBR_ENABLE
> actually disabled the shadow MBR and vice versa.
> 
> This patch adds correct conversion from OPAL_MBR_DISABLE/ENABLE to
> OPAL_FALSE/TRUE. The change affects existing programs using
> IOC_OPAL_ENABLE_DISABLE_MBR but this is typically used only once when
> setting up an Opal drive.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
> Acked-by: Jon Derrick <jonathan.derrick@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

[PATCH 02/16] block: sed-opal: fix typos and formatting

[David Kozub]

This should make no change in functionality.
The formatting changes were triggered by checkpatch.pl.

Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c              | 18 ++++++++++--------
 include/uapi/linux/sed-opal.h |  2 +-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 119640897293..d12a910e06cb 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -157,7 +157,7 @@ static const u8 opaluid[][OPAL_UID_LENGTH] = {
 
 	/* C_PIN_TABLE object ID's */
 
-        [OPAL_C_PIN_MSID] =
+	[OPAL_C_PIN_MSID] =
 		{ 0x00, 0x00, 0x00, 0x0B, 0x00, 0x00, 0x84, 0x02},
 	[OPAL_C_PIN_SID] =
 		{ 0x00, 0x00, 0x00, 0x0B, 0x00, 0x00, 0x00, 0x01},
@@ -551,7 +551,6 @@ static void add_medium_atom_header(struct opal_dev *cmd, bool bytestring,
 
 static void add_token_u64(int *err, struct opal_dev *cmd, u64 number)
 {
-
 	size_t len;
 	int msb;
 
@@ -623,7 +622,7 @@ static int build_locking_range(u8 *buffer, size_t length, u8 lr)
 static int build_locking_user(u8 *buffer, size_t length, u8 lr)
 {
 	if (length > OPAL_UID_LENGTH) {
-		pr_debug("Can't build locking range user, Length OOB\n");
+		pr_debug("Can't build locking range user. Length OOB\n");
 		return -ERANGE;
 	}
 
@@ -1324,6 +1323,7 @@ static int start_SIDASP_opal_session(struct opal_dev *dev, void *data)
 
 	if (!key) {
 		const struct opal_key *okey = data;
+
 		ret = start_generic_opal_session(dev, OPAL_SID_UID,
 						 OPAL_ADMINSP_UID,
 						 okey->key,
@@ -1341,6 +1341,7 @@ static int start_SIDASP_opal_session(struct opal_dev *dev, void *data)
 static int start_admin1LSP_opal_session(struct opal_dev *dev, void *data)
 {
 	struct opal_key *key = data;
+
 	return start_generic_opal_session(dev, OPAL_ADMIN1_UID,
 					  OPAL_LOCKINGSP_UID,
 					  key->key, key->key_len);
@@ -1714,7 +1715,7 @@ static int lock_unlock_locking_range(struct opal_dev *dev, void *data)
 		write_locked = 0;
 		break;
 	case OPAL_LK:
-		/* vars are initalized to locked */
+		/* vars are initialized to locked */
 		break;
 	default:
 		pr_debug("Tried to set an invalid locking state... returning to uland\n");
@@ -1775,7 +1776,7 @@ static int lock_unlock_locking_range_sum(struct opal_dev *dev, void *data)
 		write_locked = 0;
 		break;
 	case OPAL_LK:
-		/* vars are initalized to locked */
+		/* vars are initialized to locked */
 		break;
 	default:
 		pr_debug("Tried to set an invalid locking state.\n");
@@ -1854,7 +1855,7 @@ static int get_lsp_lifecycle_cont(struct opal_dev *dev)
 		return error;
 
 	lc_status = response_get_u64(&dev->parsed, 4);
-	/* 0x08 is Manufacured Inactive */
+	/* 0x08 is Manufactured Inactive */
 	/* 0x09 is Manufactured */
 	if (lc_status != OPAL_MANUFACTURED_INACTIVE) {
 		pr_debug("Couldn't determine the status of the Lifecycle state\n");
@@ -2225,7 +2226,7 @@ static int __opal_lock_unlock(struct opal_dev *dev,
 static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
 {
 	u8 mbr_done_tf = OPAL_TRUE;
-	const struct opal_step mbrdone_step [] = {
+	const struct opal_step mbrdone_step[] = {
 		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, key },
 		{ set_mbr_done, &mbr_done_tf },
@@ -2276,7 +2277,8 @@ static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
 	return ret;
 }
 
-static int opal_activate_lsp(struct opal_dev *dev, struct opal_lr_act *opal_lr_act)
+static int opal_activate_lsp(struct opal_dev *dev,
+			     struct opal_lr_act *opal_lr_act)
 {
 	const struct opal_step active_steps[] = {
 		{ opal_discovery0, },
diff --git a/include/uapi/linux/sed-opal.h b/include/uapi/linux/sed-opal.h
index 627624d35030..e092e124dd16 100644
--- a/include/uapi/linux/sed-opal.h
+++ b/include/uapi/linux/sed-opal.h
@@ -58,7 +58,7 @@ struct opal_key {
 struct opal_lr_act {
 	struct opal_key key;
 	__u32 sum;
-	__u8    num_lrs;
+	__u8 num_lrs;
 	__u8 lr[OPAL_MAX_LRS];
 	__u8 align[2]; /* Align to 8 byte boundary */
 };
-- 
2.20.1

Re: [PATCH 02/16] block: sed-opal: fix typos and formatting

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 03/16] block: sed-opal: use correct macro for method length

[David Kozub]

From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>

Also the values of OPAL_UID_LENGTH and OPAL_METHOD_LENGTH are the same,
it is weird to use OPAL_UID_LENGTH for the definition of the methods.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index d12a910e06cb..e59ae364f1ef 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -181,7 +181,7 @@ static const u8 opaluid[][OPAL_UID_LENGTH] = {
  * Derived from: TCG_Storage_Architecture_Core_Spec_v2.01_r1.00
  * Section: 6.3 Assigned UIDs
  */
-static const u8 opalmethod[][OPAL_UID_LENGTH] = {
+static const u8 opalmethod[][OPAL_METHOD_LENGTH] = {
 	[OPAL_PROPERTIES] =
 		{ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0x01 },
 	[OPAL_STARTSESSION] =
-- 
2.20.1

[PATCH 04/16] block: sed-opal: unify space check in add_token_*

[David Kozub]

From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>

All add_token_* functions have a common set of conditions that have to
be checked. Use a common function for those checks in order to avoid
different behaviour as well as code duplication.

Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz>
Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
---
 block/sed-opal.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index e59ae364f1ef..d285bd4b2b9b 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -510,15 +510,24 @@ static int opal_discovery0(struct opal_dev *dev, void *data)
 	return opal_discovery0_end(dev);
 }
 
-static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok)
+static bool can_add(int *err, struct opal_dev *cmd, size_t len)
 {
 	if (*err)
-		return;
-	if (cmd->pos >= IO_BUFFER_LENGTH - 1) {
-		pr_debug("Error adding u8: end of buffer.\n");
+		return false;
+
+	if (len > IO_BUFFER_LENGTH || cmd->pos > IO_BUFFER_LENGTH - len) {
+		pr_debug("Error adding %zu bytes: end of buffer.\n", len);
 		*err = -ERANGE;
-		return;
+		return false;
 	}
+
+	return true;
+}
+
+static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok)
+{
+	if (!can_add(err, cmd, 1))
+		return;
 	cmd->cmd[cmd->pos++] = tok;
 }
 
@@ -562,9 +571,8 @@ static void add_token_u64(int *err, struct opal_dev *cmd, u64 number)
 	msb = fls64(number);
 	len = DIV_ROUND_UP(msb, 8);
 
-	if (cmd->pos >= IO_BUFFER_LENGTH - len - 1) {
+	if (!can_add(err, cmd, len + 1)) {
 		pr_debug("Error adding u64: end of buffer.\n");
-		*err = -ERANGE;
 		return;
 	}
 	add_short_atom_header(cmd, false, false, len);
@@ -586,9 +594,8 @@ static void add_token_bytestring(int *err, struct opal_dev *cmd,
 		is_short_atom = false;
 	}
 
-	if (len >= IO_BUFFER_LENGTH - cmd->pos - header_len) {
+	if (!can_add(err, cmd, header_len + len)) {
 		pr_debug("Error adding bytestring: end of buffer.\n");
-		*err = -ERANGE;
 		return;
 	}
 
-- 
2.20.1

Re: [PATCH 04/16] block: sed-opal: unify space check in add_token_*

[Derrick, Jonathan]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

Looks good

Acked-by: Jon Derrick <jonathan.derrick@intel.com>

On Thu, 2019-02-14 at 01:15 +0100, David Kozub wrote:
> From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
> 
> All add_token_* functions have a common set of conditions that have
> to
> be checked. Use a common function for those checks in order to avoid
> different behaviour as well as code duplication.
> 
> Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz>
> Signed-off-by: Jonas Rabenstein <
> jonas.rabenstein@studium.uni-erlangen.de>
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
> ---
>  block/sed-opal.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index e59ae364f1ef..d285bd4b2b9b 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -510,15 +510,24 @@ static int opal_discovery0(struct opal_dev
> *dev, void *data)
>  	return opal_discovery0_end(dev);
>  }
>  
> -static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok)
> +static bool can_add(int *err, struct opal_dev *cmd, size_t len)
>  {
>  	if (*err)
> -		return;
> -	if (cmd->pos >= IO_BUFFER_LENGTH - 1) {
> -		pr_debug("Error adding u8: end of buffer.\n");
> +		return false;
> +
> +	if (len > IO_BUFFER_LENGTH || cmd->pos > IO_BUFFER_LENGTH -
> len) {
> +		pr_debug("Error adding %zu bytes: end of buffer.\n",
> len);
>  		*err = -ERANGE;
> -		return;
> +		return false;
>  	}
> +
> +	return true;
> +}
> +
> +static void add_token_u8(int *err, struct opal_dev *cmd, u8 tok)
> +{
> +	if (!can_add(err, cmd, 1))
> +		return;
>  	cmd->cmd[cmd->pos++] = tok;
>  }
>  
> @@ -562,9 +571,8 @@ static void add_token_u64(int *err, struct
> opal_dev *cmd, u64 number)
>  	msb = fls64(number);
>  	len = DIV_ROUND_UP(msb, 8);
>  
> -	if (cmd->pos >= IO_BUFFER_LENGTH - len - 1) {
> +	if (!can_add(err, cmd, len + 1)) {
>  		pr_debug("Error adding u64: end of buffer.\n");
> -		*err = -ERANGE;
>  		return;
>  	}
>  	add_short_atom_header(cmd, false, false, len);
> @@ -586,9 +594,8 @@ static void add_token_bytestring(int *err, struct
> opal_dev *cmd,
>  		is_short_atom = false;
>  	}
>  
> -	if (len >= IO_BUFFER_LENGTH - cmd->pos - header_len) {
> +	if (!can_add(err, cmd, header_len + len)) {
>  		pr_debug("Error adding bytestring: end of buffer.\n");
> -		*err = -ERANGE;
>  		return;
>  	}
>  

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

Re: [PATCH 04/16] block: sed-opal: unify space check in add_token_*

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 04/16] block: sed-opal: unify space check in add_token_*

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:15:56AM +0100, David Kozub wrote:
> From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
> 
> All add_token_* functions have a common set of conditions that have to
> be checked. Use a common function for those checks in order to avoid
> different behaviour as well as code duplication.
> 
> Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz>
> Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>

Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

Re: [PATCH 04/16] block: sed-opal: unify space check in add_token_*

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:15:56AM +0100, David Kozub wrote:
> From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
> 
> All add_token_* functions have a common set of conditions that have to
> be checked. Use a common function for those checks in order to avoid
> different behaviour as well as code duplication.
> 
> Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz>
> Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

[PATCH 05/16] block: sed-opal: close parameter list in cmd_finalize

[David Kozub]

Every step ends by calling cmd_finalize (via finalize_and_send)
yet every step adds the token OPAL_ENDLIST on its own. Moving
this into cmd_finalize decreases code duplication.

Co-authored-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index d285bd4b2b9b..c5dff6199bd6 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -655,6 +655,9 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn, u32 tsn)
 	struct opal_header *hdr;
 	int err = 0;
 
+	/* close parameter list */
+	add_token_u8(&err, cmd, OPAL_ENDLIST);
+
 	add_token_u8(&err, cmd, OPAL_ENDOFDATA);
 	add_token_u8(&err, cmd, OPAL_STARTLIST);
 	add_token_u8(&err, cmd, 0);
@@ -1073,7 +1076,6 @@ static int gen_key(struct opal_dev *dev, void *data)
 	add_token_bytestring(&err, dev, opalmethod[OPAL_GENKEY],
 			     OPAL_UID_LENGTH);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error building gen key command\n");
@@ -1136,7 +1138,6 @@ static int get_active_key(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, 10); /* ActiveKey */
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 	if (err) {
 		pr_debug("Error building get active key command\n");
 		return err;
@@ -1182,7 +1183,6 @@ static int generic_lr_enable_disable(struct opal_dev *dev,
 
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 	return err;
 }
 
@@ -1248,8 +1248,6 @@ static int setup_locking_range(struct opal_dev *dev, void *data)
 
 		add_token_u8(&err, dev, OPAL_ENDLIST);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
-		add_token_u8(&err, dev, OPAL_ENDLIST);
-
 	}
 	if (err) {
 		pr_debug("Error building Setup Locking range command.\n");
@@ -1289,7 +1287,6 @@ static int start_generic_opal_session(struct opal_dev *dev,
 
 	switch (auth) {
 	case OPAL_ANYBODY_UID:
-		add_token_u8(&err, dev, OPAL_ENDLIST);
 		break;
 	case OPAL_ADMIN1_UID:
 	case OPAL_SID_UID:
@@ -1302,7 +1299,6 @@ static int start_generic_opal_session(struct opal_dev *dev,
 		add_token_bytestring(&err, dev, opaluid[auth],
 				     OPAL_UID_LENGTH);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
-		add_token_u8(&err, dev, OPAL_ENDLIST);
 		break;
 	default:
 		pr_debug("Cannot start Admin SP session with auth %d\n", auth);
@@ -1400,7 +1396,6 @@ static int start_auth_opal_session(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, 3);
 	add_token_bytestring(&err, dev, lk_ul_user, OPAL_UID_LENGTH);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error building STARTSESSION command.\n");
@@ -1423,7 +1418,6 @@ static int revert_tper(struct opal_dev *dev, void *data)
 	add_token_bytestring(&err, dev, opalmethod[OPAL_REVERT],
 			     OPAL_UID_LENGTH);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 	if (err) {
 		pr_debug("Error building REVERT TPER command.\n");
 		return err;
@@ -1457,7 +1451,6 @@ static int internal_activate_user(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error building Activate UserN command.\n");
@@ -1484,7 +1477,6 @@ static int erase_locking_range(struct opal_dev *dev, void *data)
 	add_token_bytestring(&err, dev, opalmethod[OPAL_ERASE],
 			     OPAL_UID_LENGTH);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error building Erase Locking Range Command.\n");
@@ -1515,7 +1507,6 @@ static int set_mbr_done(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error Building set MBR Done command\n");
@@ -1547,7 +1538,6 @@ static int set_mbr_enable_disable(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error Building set MBR done command\n");
@@ -1579,7 +1569,6 @@ static int generic_pw_cmd(u8 *key, size_t key_len, u8 *cpin_uid,
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	return err;
 }
@@ -1688,7 +1677,6 @@ static int add_user_to_lr(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error building add user to locking range command.\n");
@@ -1749,7 +1737,6 @@ static int lock_unlock_locking_range(struct opal_dev *dev, void *data)
 
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
 		pr_debug("Error building SET command.\n");
@@ -1837,11 +1824,8 @@ static int activate_lsp(struct opal_dev *dev, void *data)
 		}
 		add_token_u8(&err, dev, OPAL_ENDLIST);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
-		add_token_u8(&err, dev, OPAL_ENDLIST);
-
 	} else {
 		add_token_u8(&err, dev, OPAL_STARTLIST);
-		add_token_u8(&err, dev, OPAL_ENDLIST);
 	}
 
 	if (err) {
@@ -1898,7 +1882,6 @@ static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, 6); /* Lifecycle Column */
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
@@ -1958,8 +1941,6 @@ static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, 4); /* End Column */
 	add_token_u8(&err, dev, 3); /* Lifecycle Column */
 	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_ENDLIST);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
 
 	if (err) {
-- 
2.20.1

[PATCH 06/16] block: sed-opal: unify cmd start

[David Kozub]

Every step starts with resetting the cmd buffer as well as the comid and
constructs the appropriate OPAL_CALL command. Consequently, those
actions may be combined into one generic function. On should take care
that the opening and closing tokens for the argument list are already
emitted by cmd_start and cmd_finalize respectively and thus must not be
additionally added.

Co-authored-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 228 ++++++++++++++---------------------------------
 1 file changed, 69 insertions(+), 159 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index c5dff6199bd6..0348fb896a5d 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -655,7 +655,7 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn, u32 tsn)
 	struct opal_header *hdr;
 	int err = 0;
 
-	/* close parameter list */
+	/* close the parameter list opened from cmd_start */
 	add_token_u8(&err, cmd, OPAL_ENDLIST);
 
 	add_token_u8(&err, cmd, OPAL_ENDOFDATA);
@@ -1000,6 +1000,27 @@ static void clear_opal_cmd(struct opal_dev *dev)
 	memset(dev->cmd, 0, IO_BUFFER_LENGTH);
 }
 
+static int cmd_start(struct opal_dev *dev, const u8 *uid, const u8 *method)
+{
+	int err = 0;
+
+	clear_opal_cmd(dev);
+	set_comid(dev, dev->comid);
+
+	add_token_u8(&err, dev, OPAL_CALL);
+	add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
+	add_token_bytestring(&err, dev, method, OPAL_METHOD_LENGTH);
+
+	/*
+	 * Every method call is followed by its parameters enclosed within
+	 * OPAL_STARTLIST and OPAL_ENDLIST tokens. We automatically open the
+	 * parameter list here and close it later in cmd_finalize.
+	 */
+	add_token_u8(&err, dev, OPAL_STARTLIST);
+
+	return err;
+}
+
 static int start_opal_session_cont(struct opal_dev *dev)
 {
 	u32 hsn, tsn;
@@ -1062,20 +1083,13 @@ static int finalize_and_send(struct opal_dev *dev, cont_fn cont)
 static int gen_key(struct opal_dev *dev, void *data)
 {
 	u8 uid[OPAL_UID_LENGTH];
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
 	memcpy(uid, dev->prev_data, min(sizeof(uid), dev->prev_d_len));
 	kfree(dev->prev_data);
 	dev->prev_data = NULL;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_GENKEY],
-			     OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
+	err = cmd_start(dev, uid, opalmethod[OPAL_GENKEY]);
 
 	if (err) {
 		pr_debug("Error building gen key command\n");
@@ -1113,21 +1127,14 @@ static int get_active_key_cont(struct opal_dev *dev)
 static int get_active_key(struct opal_dev *dev, void *data)
 {
 	u8 uid[OPAL_UID_LENGTH];
-	int err = 0;
+	int err;
 	u8 *lr = data;
 
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
-
 	err = build_locking_range(uid, sizeof(uid), *lr);
 	if (err)
 		return err;
 
-	err = 0;
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_GET], OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
+	err = cmd_start(dev, uid, opalmethod[OPAL_GET]);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, 3); /* startCloumn */
@@ -1150,13 +1157,10 @@ static int generic_lr_enable_disable(struct opal_dev *dev,
 				     u8 *uid, bool rle, bool wle,
 				     bool rl, bool wl)
 {
-	int err = 0;
+	int err;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET], OPAL_UID_LENGTH);
+	err = cmd_start(dev, uid, opalmethod[OPAL_SET]);
 
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1203,10 +1207,7 @@ static int setup_locking_range(struct opal_dev *dev, void *data)
 	u8 uid[OPAL_UID_LENGTH];
 	struct opal_user_lr_setup *setup = data;
 	u8 lr;
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
 	lr = setup->session.opal_key.lr;
 	err = build_locking_range(uid, sizeof(uid), lr);
@@ -1216,12 +1217,8 @@ static int setup_locking_range(struct opal_dev *dev, void *data)
 	if (lr == 0)
 		err = enable_global_lr(dev, uid, setup);
 	else {
-		add_token_u8(&err, dev, OPAL_CALL);
-		add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
-		add_token_bytestring(&err, dev, opalmethod[OPAL_SET],
-				     OPAL_UID_LENGTH);
+		err = cmd_start(dev, uid, opalmethod[OPAL_SET]);
 
-		add_token_u8(&err, dev, OPAL_STARTLIST);
 		add_token_u8(&err, dev, OPAL_STARTNAME);
 		add_token_u8(&err, dev, OPAL_VALUES);
 		add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1265,22 +1262,15 @@ static int start_generic_opal_session(struct opal_dev *dev,
 				      u8 key_len)
 {
 	u32 hsn;
-	int err = 0;
+	int err;
 
 	if (key == NULL && auth != OPAL_ANYBODY_UID)
 		return OPAL_INVAL_PARAM;
 
-	clear_opal_cmd(dev);
-
-	set_comid(dev, dev->comid);
 	hsn = GENERIC_HOST_SESSION_NUM;
+	err = cmd_start(dev, opaluid[OPAL_SMUID_UID],
+			opalmethod[OPAL_STARTSESSION]);
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_SMUID_UID],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_STARTSESSION],
-			     OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u64(&err, dev, hsn);
 	add_token_bytestring(&err, dev, opaluid[sp_type], OPAL_UID_LENGTH);
 	add_token_u8(&err, dev, 1);
@@ -1360,30 +1350,21 @@ static int start_auth_opal_session(struct opal_dev *dev, void *data)
 	u8 *key = session->opal_key.key;
 	u32 hsn = GENERIC_HOST_SESSION_NUM;
 
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
-
-	if (session->sum) {
+	if (session->sum)
 		err = build_locking_user(lk_ul_user, sizeof(lk_ul_user),
 					 session->opal_key.lr);
-		if (err)
-			return err;
-
-	} else if (session->who != OPAL_ADMIN1 && !session->sum) {
+	else if (session->who != OPAL_ADMIN1 && !session->sum)
 		err = build_locking_user(lk_ul_user, sizeof(lk_ul_user),
 					 session->who - 1);
-		if (err)
-			return err;
-	} else
+	else
 		memcpy(lk_ul_user, opaluid[OPAL_ADMIN1_UID], OPAL_UID_LENGTH);
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_SMUID_UID],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_STARTSESSION],
-			     OPAL_UID_LENGTH);
+	if (err)
+		return err;
+
+	err = cmd_start(dev, opaluid[OPAL_SMUID_UID],
+			opalmethod[OPAL_STARTSESSION]);
 
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u64(&err, dev, hsn);
 	add_token_bytestring(&err, dev, opaluid[OPAL_LOCKINGSP_UID],
 			     OPAL_UID_LENGTH);
@@ -1407,17 +1388,10 @@ static int start_auth_opal_session(struct opal_dev *dev, void *data)
 
 static int revert_tper(struct opal_dev *dev, void *data)
 {
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_ADMINSP_UID],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_REVERT],
-			     OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
+	err = cmd_start(dev, opaluid[OPAL_ADMINSP_UID],
+			opalmethod[OPAL_REVERT]);
 	if (err) {
 		pr_debug("Error building REVERT TPER command.\n");
 		return err;
@@ -1430,18 +1404,12 @@ static int internal_activate_user(struct opal_dev *dev, void *data)
 {
 	struct opal_session_info *session = data;
 	u8 uid[OPAL_UID_LENGTH];
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
 	memcpy(uid, opaluid[OPAL_USER1_UID], OPAL_UID_LENGTH);
 	uid[7] = session->who;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET], OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
+	err = cmd_start(dev, uid, opalmethod[OPAL_SET]);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1464,19 +1432,12 @@ static int erase_locking_range(struct opal_dev *dev, void *data)
 {
 	struct opal_session_info *session = data;
 	u8 uid[OPAL_UID_LENGTH];
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
 	if (build_locking_range(uid, sizeof(uid), session->opal_key.lr) < 0)
 		return -ERANGE;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, uid, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_ERASE],
-			     OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
+	err = cmd_start(dev, uid, opalmethod[OPAL_ERASE]);
 
 	if (err) {
 		pr_debug("Error building Erase Locking Range Command.\n");
@@ -1488,16 +1449,11 @@ static int erase_locking_range(struct opal_dev *dev, void *data)
 static int set_mbr_done(struct opal_dev *dev, void *data)
 {
 	u8 *mbr_done_tf = data;
-	int err = 0;
+	int err;
 
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	err = cmd_start(dev, opaluid[OPAL_MBRCONTROL],
+			opalmethod[OPAL_SET]);
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_MBRCONTROL],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET], OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1519,16 +1475,11 @@ static int set_mbr_done(struct opal_dev *dev, void *data)
 static int set_mbr_enable_disable(struct opal_dev *dev, void *data)
 {
 	u8 *mbr_en_dis = data;
-	int err = 0;
+	int err;
 
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	err = cmd_start(dev, opaluid[OPAL_MBRCONTROL],
+			opalmethod[OPAL_SET]);
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_MBRCONTROL],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET], OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1550,16 +1501,10 @@ static int set_mbr_enable_disable(struct opal_dev *dev, void *data)
 static int generic_pw_cmd(u8 *key, size_t key_len, u8 *cpin_uid,
 			  struct opal_dev *dev)
 {
-	int err = 0;
+	int err;
 
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	err = cmd_start(dev, cpin_uid, opalmethod[OPAL_SET]);
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, cpin_uid, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET],
-			     OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1616,10 +1561,7 @@ static int add_user_to_lr(struct opal_dev *dev, void *data)
 	u8 lr_buffer[OPAL_UID_LENGTH];
 	u8 user_uid[OPAL_UID_LENGTH];
 	struct opal_lock_unlock *lkul = data;
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
 	memcpy(lr_buffer, opaluid[OPAL_LOCKINGRANGE_ACE_RDLOCKED],
 	       OPAL_UID_LENGTH);
@@ -1634,12 +1576,8 @@ static int add_user_to_lr(struct opal_dev *dev, void *data)
 
 	user_uid[7] = lkul->session.who;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, lr_buffer, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET],
-			     OPAL_UID_LENGTH);
+	err = cmd_start(dev, lr_buffer, opalmethod[OPAL_SET]);
 
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 
@@ -1693,9 +1631,6 @@ static int lock_unlock_locking_range(struct opal_dev *dev, void *data)
 	u8 read_locked = 1, write_locked = 1;
 	int err = 0;
 
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
-
 	if (build_locking_range(lr_buffer, sizeof(lr_buffer),
 				lkul->session.opal_key.lr) < 0)
 		return -ERANGE;
@@ -1717,10 +1652,8 @@ static int lock_unlock_locking_range(struct opal_dev *dev, void *data)
 		return OPAL_INVAL_PARAM;
 	}
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, lr_buffer, OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_SET], OPAL_UID_LENGTH);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
+	err = cmd_start(dev, lr_buffer, opalmethod[OPAL_SET]);
+
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
@@ -1791,17 +1724,10 @@ static int activate_lsp(struct opal_dev *dev, void *data)
 	struct opal_lr_act *opal_act = data;
 	u8 user_lr[OPAL_UID_LENGTH];
 	u8 uint_3 = 0x83;
-	int err = 0, i;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
-
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_LOCKINGSP_UID],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_ACTIVATE],
-			     OPAL_UID_LENGTH);
+	int err, i;
 
+	err = cmd_start(dev, opaluid[OPAL_LOCKINGSP_UID],
+			opalmethod[OPAL_ACTIVATE]);
 
 	if (opal_act->sum) {
 		err = build_locking_range(user_lr, sizeof(user_lr),
@@ -1809,7 +1735,6 @@ static int activate_lsp(struct opal_dev *dev, void *data)
 		if (err)
 			return err;
 
-		add_token_u8(&err, dev, OPAL_STARTLIST);
 		add_token_u8(&err, dev, OPAL_STARTNAME);
 		add_token_u8(&err, dev, uint_3);
 		add_token_u8(&err, dev, 6);
@@ -1824,8 +1749,6 @@ static int activate_lsp(struct opal_dev *dev, void *data)
 		}
 		add_token_u8(&err, dev, OPAL_ENDLIST);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
-	} else {
-		add_token_u8(&err, dev, OPAL_STARTLIST);
 	}
 
 	if (err) {
@@ -1859,17 +1782,11 @@ static int get_lsp_lifecycle_cont(struct opal_dev *dev)
 /* Determine if we're in the Manufactured Inactive or Active state */
 static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
 {
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_LOCKINGSP_UID],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_GET], OPAL_UID_LENGTH);
+	err = cmd_start(dev, opaluid[OPAL_LOCKINGSP_UID],
+			opalmethod[OPAL_GET]);
 
-	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
 
 	add_token_u8(&err, dev, OPAL_STARTNAME);
@@ -1919,19 +1836,12 @@ static int get_msid_cpin_pin_cont(struct opal_dev *dev)
 
 static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
 {
-	int err = 0;
-
-	clear_opal_cmd(dev);
-	set_comid(dev, dev->comid);
+	int err;
 
-	add_token_u8(&err, dev, OPAL_CALL);
-	add_token_bytestring(&err, dev, opaluid[OPAL_C_PIN_MSID],
-			     OPAL_UID_LENGTH);
-	add_token_bytestring(&err, dev, opalmethod[OPAL_GET], OPAL_UID_LENGTH);
+	err = cmd_start(dev, opaluid[OPAL_C_PIN_MSID],
+			opalmethod[OPAL_GET]);
 
 	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, 3); /* Start Column */
 	add_token_u8(&err, dev, 3); /* PIN */
-- 
2.20.1

[PATCH 07/16] block: sed-opal: unify error handling of responses

[David Kozub]

response_get_{string,u64} include error handling for argument resp being
NULL but response_get_token does not handle this.

Make all three of response_get_{string,u64,token} handle NULL resp in
the same way.

Co-authored-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 0348fb896a5d..3f368b14efd9 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -696,6 +696,11 @@ static const struct opal_resp_tok *response_get_token(
 {
 	const struct opal_resp_tok *tok;
 
+	if (!resp) {
+		pr_debug("Response is NULL\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	if (n >= resp->num) {
 		pr_debug("Token number doesn't exist: %d, resp: %d\n",
 			 n, resp->num);
-- 
2.20.1

[PATCH 08/16] block: sed-opal: reuse response_get_token to decrease code duplication

[David Kozub]

response_get_token had already been in place, its functionality had
been duplicated within response_get_{u64,bytestring} with the same error
handling. Unify the handling by reusing response_get_token within the
other functions.

Co-authored-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 46 +++++++++++++++-------------------------------
 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 3f368b14efd9..5cb8034b53c8 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -883,27 +883,19 @@ static size_t response_get_string(const struct parsed_resp *resp, int n,
 				  const char **store)
 {
 	u8 skip;
-	const struct opal_resp_tok *token;
+	const struct opal_resp_tok *tok;
 
 	*store = NULL;
-	if (!resp) {
-		pr_debug("Response is NULL\n");
-		return 0;
-	}
-
-	if (n >= resp->num) {
-		pr_debug("Response has %d tokens. Can't access %d\n",
-			 resp->num, n);
+	tok = response_get_token(resp, n);
+	if (IS_ERR(tok))
 		return 0;
-	}
 
-	token = &resp->toks[n];
-	if (token->type != OPAL_DTA_TOKENID_BYTESTRING) {
+	if (tok->type != OPAL_DTA_TOKENID_BYTESTRING) {
 		pr_debug("Token is not a byte string!\n");
 		return 0;
 	}
 
-	switch (token->width) {
+	switch (tok->width) {
 	case OPAL_WIDTH_TINY:
 	case OPAL_WIDTH_SHORT:
 		skip = 1;
@@ -919,37 +911,29 @@ static size_t response_get_string(const struct parsed_resp *resp, int n,
 		return 0;
 	}
 
-	*store = token->pos + skip;
-	return token->len - skip;
+	*store = tok->pos + skip;
+	return tok->len - skip;
 }
 
 static u64 response_get_u64(const struct parsed_resp *resp, int n)
 {
-	if (!resp) {
-		pr_debug("Response is NULL\n");
-		return 0;
-	}
+	const struct opal_resp_tok *tok;
 
-	if (n >= resp->num) {
-		pr_debug("Response has %d tokens. Can't access %d\n",
-			 resp->num, n);
+	tok = response_get_token(resp, n);
+	if (IS_ERR(tok))
 		return 0;
-	}
 
-	if (resp->toks[n].type != OPAL_DTA_TOKENID_UINT) {
-		pr_debug("Token is not unsigned it: %d\n",
-			 resp->toks[n].type);
+	if (tok->type != OPAL_DTA_TOKENID_UINT) {
+		pr_debug("Token is not unsigned int: %d\n", tok->type);
 		return 0;
 	}
 
-	if (!(resp->toks[n].width == OPAL_WIDTH_TINY ||
-	      resp->toks[n].width == OPAL_WIDTH_SHORT)) {
-		pr_debug("Atom is not short or tiny: %d\n",
-			 resp->toks[n].width);
+	if (tok->width != OPAL_WIDTH_TINY && tok->width != OPAL_WIDTH_SHORT) {
+		pr_debug("Atom is not short or tiny: %d\n", tok->width);
 		return 0;
 	}
 
-	return resp->toks[n].stored.u;
+	return tok->stored.u;
 }
 
 static bool response_token_matches(const struct opal_resp_tok *token, u8 match)
-- 
2.20.1

[PATCH 09/16] block: sed-opal: print failed function address

[David Kozub]

From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>

Add function address (and if available its symbol) to the message if a
step function fails.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com
---
 block/sed-opal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 5cb8034b53c8..1f246200b574 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -394,8 +394,8 @@ static int next(struct opal_dev *dev)
 
 		error = step->fn(dev, step->data);
 		if (error) {
-			pr_debug("Error on step function: %d with error %d: %s\n",
-				 state, error,
+			pr_debug("Step %d (%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
-- 
2.20.1

[PATCH 10/16] block: sed-opal: split generation of bytestring header and content

[David Kozub]

From: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>

Split the header generation from the (normal) memcpy part if a
bytestring is copied into the command buffer. This allows in-place
generation of the bytestring content. For example, copy_from_user may be
used without an intermediate buffer.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 1f246200b574..ad66d1dc725a 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -580,15 +580,11 @@ static void add_token_u64(int *err, struct opal_dev *cmd, u64 number)
 		add_token_u8(err, cmd, number >> (len * 8));
 }
 
-static void add_token_bytestring(int *err, struct opal_dev *cmd,
-				 const u8 *bytestring, size_t len)
+static u8 *add_bytestring_header(int *err, struct opal_dev *cmd, size_t len)
 {
 	size_t header_len = 1;
 	bool is_short_atom = true;
 
-	if (*err)
-		return;
-
 	if (len & ~SHORT_ATOM_LEN_MASK) {
 		header_len = 2;
 		is_short_atom = false;
@@ -596,7 +592,7 @@ static void add_token_bytestring(int *err, struct opal_dev *cmd,
 
 	if (!can_add(err, cmd, header_len + len)) {
 		pr_debug("Error adding bytestring: end of buffer.\n");
-		return;
+		return NULL;
 	}
 
 	if (is_short_atom)
@@ -604,9 +600,19 @@ static void add_token_bytestring(int *err, struct opal_dev *cmd,
 	else
 		add_medium_atom_header(cmd, true, false, len);
 
-	memcpy(&cmd->cmd[cmd->pos], bytestring, len);
-	cmd->pos += len;
+	return &cmd->cmd[cmd->pos];
+}
 
+static void add_token_bytestring(int *err, struct opal_dev *cmd,
+				 const u8 *bytestring, size_t len)
+{
+	u8 *start;
+
+	start = add_bytestring_header(err, cmd, len);
+	if (!start)
+		return;
+	memcpy(start, bytestring, len);
+	cmd->pos += len;
 }
 
 static int build_locking_range(u8 *buffer, size_t length, u8 lr)
-- 
2.20.1

[PATCH 11/16] block: sed-opal: add token for OPAL_LIFECYCLE

[David Kozub]

Define OPAL_LIFECYCLE token and use it instead of literals in
get_lsp_lifecycle.

Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
---
 block/opal_proto.h | 2 ++
 block/sed-opal.c   | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/opal_proto.h b/block/opal_proto.h
index e20be8258854..b6e352cfe982 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -170,6 +170,8 @@ enum opal_token {
 	OPAL_READLOCKED = 0x07,
 	OPAL_WRITELOCKED = 0x08,
 	OPAL_ACTIVEKEY = 0x0A,
+	/* lockingsp table */
+	OPAL_LIFECYCLE = 0x06,
 	/* locking info table */
 	OPAL_MAXRANGES = 0x04,
 	 /* mbr control */
diff --git a/block/sed-opal.c b/block/sed-opal.c
index ad66d1dc725a..7f02e50e2bce 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1786,12 +1786,12 @@ static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
 
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, 3); /* Start Column */
-	add_token_u8(&err, dev, 6); /* Lifecycle Column */
+	add_token_u8(&err, dev, OPAL_LIFECYCLE);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 
 	add_token_u8(&err, dev, OPAL_STARTNAME);
 	add_token_u8(&err, dev, 4); /* End Column */
-	add_token_u8(&err, dev, 6); /* Lifecycle Column */
+	add_token_u8(&err, dev, OPAL_LIFECYCLE);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 
 	add_token_u8(&err, dev, OPAL_ENDLIST);
-- 
2.20.1

Re: [PATCH 11/16] block: sed-opal: add token for OPAL_LIFECYCLE

[Derrick, Jonathan]

[-- Attachment #1: Type: text/plain, Size: 1586 bytes --]

Acked-by: Jon Derrick <jonathan.derrick@intel.com>

On Thu, 2019-02-14 at 01:16 +0100, David Kozub wrote:
> Define OPAL_LIFECYCLE token and use it instead of literals in
> get_lsp_lifecycle.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
> ---
>  block/opal_proto.h | 2 ++
>  block/sed-opal.c   | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/block/opal_proto.h b/block/opal_proto.h
> index e20be8258854..b6e352cfe982 100644
> --- a/block/opal_proto.h
> +++ b/block/opal_proto.h
> @@ -170,6 +170,8 @@ enum opal_token {
>  	OPAL_READLOCKED = 0x07,
>  	OPAL_WRITELOCKED = 0x08,
>  	OPAL_ACTIVEKEY = 0x0A,
> +	/* lockingsp table */
> +	OPAL_LIFECYCLE = 0x06,
>  	/* locking info table */
>  	OPAL_MAXRANGES = 0x04,
>  	 /* mbr control */
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index ad66d1dc725a..7f02e50e2bce 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -1786,12 +1786,12 @@ static int get_lsp_lifecycle(struct opal_dev
> *dev, void *data)
>  
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
>  	add_token_u8(&err, dev, 3); /* Start Column */
> -	add_token_u8(&err, dev, 6); /* Lifecycle Column */
> +	add_token_u8(&err, dev, OPAL_LIFECYCLE);
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
>  	add_token_u8(&err, dev, 4); /* End Column */
> -	add_token_u8(&err, dev, 6); /* Lifecycle Column */
> +	add_token_u8(&err, dev, OPAL_LIFECYCLE);
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  
>  	add_token_u8(&err, dev, OPAL_ENDLIST);

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

Re: [PATCH 11/16] block: sed-opal: add token for OPAL_LIFECYCLE

[Christoph Hellwig]

On Thu, Feb 14, 2019 at 01:16:03AM +0100, David Kozub wrote:
> Define OPAL_LIFECYCLE token and use it instead of literals in
> get_lsp_lifecycle.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 11/16] block: sed-opal: add token for OPAL_LIFECYCLE

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:16:03AM +0100, David Kozub wrote:
> Define OPAL_LIFECYCLE token and use it instead of literals in
> get_lsp_lifecycle.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

[PATCH 12/16] block: sed-opal: unify retrieval of table columns

[David Kozub]

Instead of having multiple places defining the same argument list to get
a specific column of a sed-opal table, provide a generic version and
call it from those functions.

Co-authored-by: David Kozub <zub@linux.fjfi.cvut.cz>
Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 130 +++++++++++++++++------------------------------
 1 file changed, 47 insertions(+), 83 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 7f02e50e2bce..5395ab1c5248 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1075,6 +1075,37 @@ static int finalize_and_send(struct opal_dev *dev, cont_fn cont)
 	return opal_send_recv(dev, cont);
 }
 
+/*
+ * request @column from table @table on device @dev. On success, the column
+ * data will be available in dev->resp->tok[4]
+ */
+static int generic_get_column(struct opal_dev *dev, const u8 *table,
+			      u64 column)
+{
+	int err;
+
+	err = cmd_start(dev, table, opalmethod[OPAL_GET]);
+
+	add_token_u8(&err, dev, OPAL_STARTLIST);
+
+	add_token_u8(&err, dev, OPAL_STARTNAME);
+	add_token_u8(&err, dev, OPAL_STARTCOLUMN);
+	add_token_u64(&err, dev, column);
+	add_token_u8(&err, dev, OPAL_ENDNAME);
+
+	add_token_u8(&err, dev, OPAL_STARTNAME);
+	add_token_u8(&err, dev, OPAL_ENDCOLUMN);
+	add_token_u64(&err, dev, column);
+	add_token_u8(&err, dev, OPAL_ENDNAME);
+
+	add_token_u8(&err, dev, OPAL_ENDLIST);
+
+	if (err)
+		return err;
+
+	return finalize_and_send(dev, parse_and_check_status);
+}
+
 static int gen_key(struct opal_dev *dev, void *data)
 {
 	u8 uid[OPAL_UID_LENGTH];
@@ -1129,23 +1160,11 @@ static int get_active_key(struct opal_dev *dev, void *data)
 	if (err)
 		return err;
 
-	err = cmd_start(dev, uid, opalmethod[OPAL_GET]);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* startCloumn */
-	add_token_u8(&err, dev, 10); /* ActiveKey */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 4); /* endColumn */
-	add_token_u8(&err, dev, 10); /* ActiveKey */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
-	if (err) {
-		pr_debug("Error building get active key command\n");
+	err = generic_get_column(dev, uid, OPAL_ACTIVEKEY);
+	if (err)
 		return err;
-	}
 
-	return finalize_and_send(dev, get_active_key_cont);
+	return get_active_key_cont(dev);
 }
 
 static int generic_lr_enable_disable(struct opal_dev *dev,
@@ -1754,14 +1773,16 @@ static int activate_lsp(struct opal_dev *dev, void *data)
 	return finalize_and_send(dev, parse_and_check_status);
 }
 
-static int get_lsp_lifecycle_cont(struct opal_dev *dev)
+/* Determine if we're in the Manufactured Inactive or Active state */
+static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
 {
 	u8 lc_status;
-	int error = 0;
+	int err;
 
-	error = parse_and_check_status(dev);
-	if (error)
-		return error;
+	err = generic_get_column(dev, opaluid[OPAL_LOCKINGSP_UID],
+				 OPAL_LIFECYCLE);
+	if (err)
+		return err;
 
 	lc_status = response_get_u64(&dev->parsed, 4);
 	/* 0x08 is Manufactured Inactive */
@@ -1774,49 +1795,19 @@ static int get_lsp_lifecycle_cont(struct opal_dev *dev)
 	return 0;
 }
 
-/* Determine if we're in the Manufactured Inactive or Active state */
-static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
-{
-	int err;
-
-	err = cmd_start(dev, opaluid[OPAL_LOCKINGSP_UID],
-			opalmethod[OPAL_GET]);
-
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* Start Column */
-	add_token_u8(&err, dev, OPAL_LIFECYCLE);
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 4); /* End Column */
-	add_token_u8(&err, dev, OPAL_LIFECYCLE);
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_ENDLIST);
-
-	if (err) {
-		pr_debug("Error Building GET Lifecycle Status command\n");
-		return err;
-	}
-
-	return finalize_and_send(dev, get_lsp_lifecycle_cont);
-}
-
-static int get_msid_cpin_pin_cont(struct opal_dev *dev)
+static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
 {
 	const char *msid_pin;
 	size_t strlen;
-	int error = 0;
+	int err;
 
-	error = parse_and_check_status(dev);
-	if (error)
-		return error;
+	err = generic_get_column(dev, opaluid[OPAL_C_PIN_MSID], OPAL_PIN);
+	if (err)
+		return err;
 
 	strlen = response_get_string(&dev->parsed, 4, &msid_pin);
 	if (!msid_pin) {
-		pr_debug("%s: Couldn't extract PIN from response\n", __func__);
+		pr_debug("Couldn't extract MSID_CPIN from response\n");
 		return OPAL_INVAL_PARAM;
 	}
 
@@ -1829,33 +1820,6 @@ static int get_msid_cpin_pin_cont(struct opal_dev *dev)
 	return 0;
 }
 
-static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
-{
-	int err;
-
-	err = cmd_start(dev, opaluid[OPAL_C_PIN_MSID],
-			opalmethod[OPAL_GET]);
-
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* Start Column */
-	add_token_u8(&err, dev, 3); /* PIN */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 4); /* End Column */
-	add_token_u8(&err, dev, 3); /* Lifecycle Column */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
-
-	if (err) {
-		pr_debug("Error building Get MSID CPIN PIN command.\n");
-		return err;
-	}
-
-	return finalize_and_send(dev, get_msid_cpin_pin_cont);
-}
-
 static int end_opal_session(struct opal_dev *dev, void *data)
 {
 	int err = 0;
-- 
2.20.1

[PATCH 13/16] block: sed-opal: use named Opal tokens instead of integer literals

[David Kozub]

Replace integer literals by Opal tokens defined in opal_proto.h where
possible.

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

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 5395ab1c5248..be0d633783c8 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1180,12 +1180,12 @@ static int generic_lr_enable_disable(struct opal_dev *dev,
 	add_token_u8(&err, dev, OPAL_STARTLIST);
 
 	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 5); /* ReadLockEnabled */
+	add_token_u8(&err, dev, OPAL_READLOCKENABLED);
 	add_token_u8(&err, dev, rle);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 
 	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 6); /* WriteLockEnabled */
+	add_token_u8(&err, dev, OPAL_WRITELOCKENABLED);
 	add_token_u8(&err, dev, wle);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 
@@ -1238,22 +1238,22 @@ static int setup_locking_range(struct opal_dev *dev, void *data)
 		add_token_u8(&err, dev, OPAL_STARTLIST);
 
 		add_token_u8(&err, dev, OPAL_STARTNAME);
-		add_token_u8(&err, dev, 3); /* Ranges Start */
+		add_token_u8(&err, dev, OPAL_RANGESTART);
 		add_token_u64(&err, dev, setup->range_start);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
 
 		add_token_u8(&err, dev, OPAL_STARTNAME);
-		add_token_u8(&err, dev, 4); /* Ranges length */
+		add_token_u8(&err, dev, OPAL_RANGELENGTH);
 		add_token_u64(&err, dev, setup->range_length);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
 
 		add_token_u8(&err, dev, OPAL_STARTNAME);
-		add_token_u8(&err, dev, 5); /*ReadLockEnabled */
+		add_token_u8(&err, dev, OPAL_READLOCKENABLED);
 		add_token_u64(&err, dev, !!setup->RLE);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
 
 		add_token_u8(&err, dev, OPAL_STARTNAME);
-		add_token_u8(&err, dev, 6); /*WriteLockEnabled*/
+		add_token_u8(&err, dev, OPAL_WRITELOCKENABLED);
 		add_token_u64(&err, dev, !!setup->WLE);
 		add_token_u8(&err, dev, OPAL_ENDNAME);
 
@@ -1472,7 +1472,7 @@ static int set_mbr_done(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 2); /* Done */
+	add_token_u8(&err, dev, OPAL_MBRDONE);
 	add_token_u8(&err, dev, *mbr_done_tf); /* Done T or F */
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
@@ -1498,7 +1498,7 @@ static int set_mbr_enable_disable(struct opal_dev *dev, void *data)
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 1);
+	add_token_u8(&err, dev, OPAL_MBRENABLE);
 	add_token_u8(&err, dev, *mbr_en_dis);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
@@ -1523,7 +1523,7 @@ static int generic_pw_cmd(u8 *key, size_t key_len, u8 *cpin_uid,
 	add_token_u8(&err, dev, OPAL_VALUES);
 	add_token_u8(&err, dev, OPAL_STARTLIST);
 	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* PIN */
+	add_token_u8(&err, dev, OPAL_PIN);
 	add_token_bytestring(&err, dev, key, key_len);
 	add_token_u8(&err, dev, OPAL_ENDNAME);
 	add_token_u8(&err, dev, OPAL_ENDLIST);
-- 
2.20.1

Re: [PATCH 13/16] block: sed-opal: use named Opal tokens instead of integer literals

[Derrick, Jonathan]

[-- Attachment #1: Type: text/plain, Size: 3629 bytes --]

Acked-by: Jon Derrick <jonathan.derrick@intel.com>

On Thu, 2019-02-14 at 01:16 +0100, David Kozub wrote:
> Replace integer literals by Opal tokens defined in opal_proto.h where
> possible.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
> ---
>  block/sed-opal.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 5395ab1c5248..be0d633783c8 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -1180,12 +1180,12 @@ static int generic_lr_enable_disable(struct
> opal_dev *dev,
>  	add_token_u8(&err, dev, OPAL_STARTLIST);
>  
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
> -	add_token_u8(&err, dev, 5); /* ReadLockEnabled */
> +	add_token_u8(&err, dev, OPAL_READLOCKENABLED);
>  	add_token_u8(&err, dev, rle);
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
> -	add_token_u8(&err, dev, 6); /* WriteLockEnabled */
> +	add_token_u8(&err, dev, OPAL_WRITELOCKENABLED);
>  	add_token_u8(&err, dev, wle);
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  
> @@ -1238,22 +1238,22 @@ static int setup_locking_range(struct
> opal_dev *dev, void *data)
>  		add_token_u8(&err, dev, OPAL_STARTLIST);
>  
>  		add_token_u8(&err, dev, OPAL_STARTNAME);
> -		add_token_u8(&err, dev, 3); /* Ranges Start */
> +		add_token_u8(&err, dev, OPAL_RANGESTART);
>  		add_token_u64(&err, dev, setup->range_start);
>  		add_token_u8(&err, dev, OPAL_ENDNAME);
>  
>  		add_token_u8(&err, dev, OPAL_STARTNAME);
> -		add_token_u8(&err, dev, 4); /* Ranges length */
> +		add_token_u8(&err, dev, OPAL_RANGELENGTH);
>  		add_token_u64(&err, dev, setup->range_length);
>  		add_token_u8(&err, dev, OPAL_ENDNAME);
>  
>  		add_token_u8(&err, dev, OPAL_STARTNAME);
> -		add_token_u8(&err, dev, 5); /*ReadLockEnabled */
> +		add_token_u8(&err, dev, OPAL_READLOCKENABLED);
>  		add_token_u64(&err, dev, !!setup->RLE);
>  		add_token_u8(&err, dev, OPAL_ENDNAME);
>  
>  		add_token_u8(&err, dev, OPAL_STARTNAME);
> -		add_token_u8(&err, dev, 6); /*WriteLockEnabled*/
> +		add_token_u8(&err, dev, OPAL_WRITELOCKENABLED);
>  		add_token_u64(&err, dev, !!setup->WLE);
>  		add_token_u8(&err, dev, OPAL_ENDNAME);
>  
> @@ -1472,7 +1472,7 @@ static int set_mbr_done(struct opal_dev *dev,
> void *data)
>  	add_token_u8(&err, dev, OPAL_VALUES);
>  	add_token_u8(&err, dev, OPAL_STARTLIST);
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
> -	add_token_u8(&err, dev, 2); /* Done */
> +	add_token_u8(&err, dev, OPAL_MBRDONE);
>  	add_token_u8(&err, dev, *mbr_done_tf); /* Done T or F */
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  	add_token_u8(&err, dev, OPAL_ENDLIST);
> @@ -1498,7 +1498,7 @@ static int set_mbr_enable_disable(struct
> opal_dev *dev, void *data)
>  	add_token_u8(&err, dev, OPAL_VALUES);
>  	add_token_u8(&err, dev, OPAL_STARTLIST);
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
> -	add_token_u8(&err, dev, 1);
> +	add_token_u8(&err, dev, OPAL_MBRENABLE);
>  	add_token_u8(&err, dev, *mbr_en_dis);
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  	add_token_u8(&err, dev, OPAL_ENDLIST);
> @@ -1523,7 +1523,7 @@ static int generic_pw_cmd(u8 *key, size_t
> key_len, u8 *cpin_uid,
>  	add_token_u8(&err, dev, OPAL_VALUES);
>  	add_token_u8(&err, dev, OPAL_STARTLIST);
>  	add_token_u8(&err, dev, OPAL_STARTNAME);
> -	add_token_u8(&err, dev, 3); /* PIN */
> +	add_token_u8(&err, dev, OPAL_PIN);
>  	add_token_bytestring(&err, dev, key, key_len);
>  	add_token_u8(&err, dev, OPAL_ENDNAME);
>  	add_token_u8(&err, dev, OPAL_ENDLIST);

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

Re: [PATCH 13/16] block: sed-opal: use named Opal tokens instead of integer literals

[Christoph Hellwig]

On Thu, Feb 14, 2019 at 01:16:05AM +0100, David Kozub wrote:
> Replace integer literals by Opal tokens defined in opal_proto.h where
> possible.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 13/16] block: sed-opal: use named Opal tokens instead of integer literals

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:16:05AM +0100, David Kozub wrote:
> Replace integer literals by Opal tokens defined in opal_proto.h where
> possible.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

[PATCH 14/16] block: sed-opal: pass steps via argument rather than via opal_dev

[David Kozub]

The steps argument is only read by the next function, so it can
be passed directly as an argument rather than via opal_dev.

Normally, the steps is an array on the stack, so the pointer stops
being valid then the function that set opal_dev.steps returns.
If opal_dev.steps was not set to NULL before return it would become
a dangling pointer. When the steps are passed as argument this
becomes easier to see and more difficult to misuse.

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

diff --git a/block/sed-opal.c b/block/sed-opal.c
index be0d633783c8..f027c0cb682e 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -85,7 +85,6 @@ struct opal_dev {
 	void *data;
 	sec_send_recv *send_recv;
 
-	const struct opal_step *steps;
 	struct mutex dev_lock;
 	u16 comid;
 	u32 hsn;
@@ -382,37 +381,34 @@ static void check_geometry(struct opal_dev *dev, const void *data)
 	dev->lowest_lba = geo->lowest_aligned_lba;
 }
 
-static int next(struct opal_dev *dev)
+static int next(struct opal_dev *dev, const struct opal_step *steps,
+		size_t n_steps)
 {
 	const struct opal_step *step;
-	int state = 0, error = 0;
+	size_t state;
+	int error = 0;
 
-	do {
-		step = &dev->steps[state];
-		if (!step->fn)
-			break;
+	for (state = 0; state < n_steps; state++) {
+		step = &steps[state];
 
 		error = step->fn(dev, step->data);
-		if (error) {
-			pr_debug("Step %d (%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;
-			}
+		if (error)
+			goto out_error;
+	}
 
-		}
-		state++;
-	} while (!error);
+	return 0;
+
+out_error:
+	/*
+	 * For each OPAL command the first step in steps does a discovery0
+	 * and the second step starts some sort of session. If an error occurred
+	 * in the first two steps (and thus stopping the loop with state <= 1)
+	 * 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 (state > 1)
+		end_opal_session_error(dev);
 
 	return error;
 }
@@ -1836,17 +1832,13 @@ 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, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
-	dev->steps = error_end_session;
-	return next(dev);
+	return next(dev, error_end_session, ARRAY_SIZE(error_end_session));
 }
 
-static inline void setup_opal_dev(struct opal_dev *dev,
-				  const struct opal_step *steps)
+static inline void setup_opal_dev(struct opal_dev *dev)
 {
-	dev->steps = steps;
 	dev->tsn = 0;
 	dev->hsn = 0;
 	dev->prev_data = NULL;
@@ -1855,14 +1847,13 @@ 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, },
-		{ NULL, }
+		{ opal_discovery0, }
 	};
 	int ret;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, steps, ARRAY_SIZE(steps));
 	dev->supported = !ret;
 	mutex_unlock(&dev->dev_lock);
 	return ret;
@@ -1919,14 +1910,13 @@ static int opal_secure_erase_locking_range(struct opal_dev *dev,
 		{ start_auth_opal_session, opal_session },
 		{ get_active_key, &opal_session->opal_key.lr },
 		{ gen_key, },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, erase_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, erase_steps, ARRAY_SIZE(erase_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -1938,14 +1928,13 @@ static int opal_erase_locking_range(struct opal_dev *dev,
 		{ opal_discovery0, },
 		{ start_auth_opal_session, opal_session },
 		{ erase_locking_range, opal_session },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, erase_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, erase_steps, ARRAY_SIZE(erase_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -1963,8 +1952,7 @@ static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 		{ end_opal_session, },
 		{ start_admin1LSP_opal_session, &opal_mbr->key },
 		{ set_mbr_enable_disable, &enable_disable },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
@@ -1973,8 +1961,8 @@ static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, mbr_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, mbr_steps, ARRAY_SIZE(mbr_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -1991,7 +1979,7 @@ static int opal_save(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk)
 	suspend->lr = lk_unlk->session.opal_key.lr;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, NULL);
+	setup_opal_dev(dev);
 	add_suspend_info(dev, suspend);
 	mutex_unlock(&dev->dev_lock);
 	return 0;
@@ -2004,8 +1992,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
 		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &lk_unlk->session.opal_key },
 		{ add_user_to_lr, lk_unlk },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
@@ -2027,8 +2014,8 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
 	}
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, steps, ARRAY_SIZE(steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2038,14 +2025,13 @@ 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 */
-		{ NULL, }
+		{ revert_tper, } /* controller will terminate session */
 	};
 	int ret;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, revert_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, revert_steps, ARRAY_SIZE(revert_steps));
 	mutex_unlock(&dev->dev_lock);
 
 	/*
@@ -2065,19 +2051,20 @@ static int __opal_lock_unlock(struct opal_dev *dev,
 		{ opal_discovery0, },
 		{ start_auth_opal_session, &lk_unlk->session },
 		{ lock_unlock_locking_range, lk_unlk },
-		{ end_opal_session, },
-		{ NULL, }
+		{ 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, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 
-	dev->steps = lk_unlk->session.sum ? unlock_sum_steps : unlock_steps;
-	return next(dev);
+	if (lk_unlk->session.sum)
+		return next(dev, unlock_sum_steps,
+			    ARRAY_SIZE(unlock_sum_steps));
+	else
+		return next(dev, unlock_steps, ARRAY_SIZE(unlock_steps));
 }
 
 static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
@@ -2087,12 +2074,10 @@ static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
 		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, key },
 		{ set_mbr_done, &mbr_done_tf },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 
-	dev->steps = mbrdone_step;
-	return next(dev);
+	return next(dev, mbrdone_step, ARRAY_SIZE(mbrdone_step));
 }
 
 static int opal_lock_unlock(struct opal_dev *dev,
@@ -2119,8 +2104,7 @@ static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
 		{ end_opal_session, },
 		{ start_SIDASP_opal_session, opal },
 		{ set_sid_cpin_pin, opal },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
@@ -2128,8 +2112,8 @@ static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
 		return -ENODEV;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, owner_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, owner_steps, ARRAY_SIZE(owner_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2142,8 +2126,7 @@ static int opal_activate_lsp(struct opal_dev *dev,
 		{ start_SIDASP_opal_session, &opal_lr_act->key },
 		{ get_lsp_lifecycle, },
 		{ activate_lsp, opal_lr_act },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
@@ -2151,8 +2134,8 @@ static int opal_activate_lsp(struct opal_dev *dev,
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, active_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, active_steps, ARRAY_SIZE(active_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2164,14 +2147,13 @@ static int opal_setup_locking_range(struct opal_dev *dev,
 		{ opal_discovery0, },
 		{ start_auth_opal_session, &opal_lrs->session },
 		{ setup_locking_range, opal_lrs },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, lr_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, lr_steps, ARRAY_SIZE(lr_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2182,8 +2164,7 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
 		{ opal_discovery0, },
 		{ start_auth_opal_session, &opal_pw->session },
 		{ set_new_pw, &opal_pw->new_user_pw },
-		{ end_opal_session, },
-		{ NULL }
+		{ end_opal_session, }
 	};
 	int ret;
 
@@ -2194,8 +2175,8 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
 		return -EINVAL;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, pw_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, pw_steps, ARRAY_SIZE(pw_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2207,8 +2188,7 @@ static int opal_activate_user(struct opal_dev *dev,
 		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &opal_session->opal_key },
 		{ internal_activate_user, opal_session },
-		{ end_opal_session, },
-		{ NULL, }
+		{ end_opal_session, }
 	};
 	int ret;
 
@@ -2220,8 +2200,8 @@ static int opal_activate_user(struct opal_dev *dev,
 	}
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, act_steps);
-	ret = next(dev);
+	setup_opal_dev(dev);
+	ret = next(dev, act_steps, ARRAY_SIZE(act_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2238,7 +2218,7 @@ bool opal_unlock_from_suspend(struct opal_dev *dev)
 		return false;
 
 	mutex_lock(&dev->dev_lock);
-	setup_opal_dev(dev, NULL);
+	setup_opal_dev(dev);
 
 	list_for_each_entry(suspend, &dev->unlk_lst, node) {
 		dev->tsn = 0;
-- 
2.20.1

Re: [PATCH 14/16] block: sed-opal: pass steps via argument rather than via opal_dev

[Derrick, Jonathan]

[-- Attachment #1: Type: text/plain, Size: 12503 bytes --]

Acked-by: Jon Derrick <jonathan.derrick@intel.com>

On Thu, 2019-02-14 at 01:16 +0100, David Kozub wrote:
> The steps argument is only read by the next function, so it can
> be passed directly as an argument rather than via opal_dev.
> 
> Normally, the steps is an array on the stack, so the pointer stops
> being valid then the function that set opal_dev.steps returns.
> If opal_dev.steps was not set to NULL before return it would become
> a dangling pointer. When the steps are passed as argument this
> becomes easier to see and more difficult to misuse.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
> ---
>  block/sed-opal.c | 158 +++++++++++++++++++++----------------------
> ----
>  1 file changed, 69 insertions(+), 89 deletions(-)
> 
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index be0d633783c8..f027c0cb682e 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -85,7 +85,6 @@ struct opal_dev {
>  	void *data;
>  	sec_send_recv *send_recv;
>  
> -	const struct opal_step *steps;
>  	struct mutex dev_lock;
>  	u16 comid;
>  	u32 hsn;
> @@ -382,37 +381,34 @@ static void check_geometry(struct opal_dev
> *dev, const void *data)
>  	dev->lowest_lba = geo->lowest_aligned_lba;
>  }
>  
> -static int next(struct opal_dev *dev)
> +static int next(struct opal_dev *dev, const struct opal_step *steps,
> +		size_t n_steps)
>  {
>  	const struct opal_step *step;
> -	int state = 0, error = 0;
> +	size_t state;
> +	int error = 0;
>  
> -	do {
> -		step = &dev->steps[state];
> -		if (!step->fn)
> -			break;
> +	for (state = 0; state < n_steps; state++) {
> +		step = &steps[state];
>  
>  		error = step->fn(dev, step->data);
> -		if (error) {
> -			pr_debug("Step %d (%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;
> -			}
> +		if (error)
> +			goto out_error;
> +	}
>  
> -		}
> -		state++;
> -	} while (!error);
> +	return 0;
> +
> +out_error:
> +	/*
> +	 * For each OPAL command the first step in steps does a
> discovery0
> +	 * and the second step starts some sort of session. If an error
> occurred
> +	 * in the first two steps (and thus stopping the loop with
> state <= 1)
> +	 * 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 (state > 1)
> +		end_opal_session_error(dev);
>  
>  	return error;
>  }
> @@ -1836,17 +1832,13 @@ 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, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
> -	dev->steps = error_end_session;
> -	return next(dev);
> +	return next(dev, error_end_session,
> ARRAY_SIZE(error_end_session));
>  }
>  
> -static inline void setup_opal_dev(struct opal_dev *dev,
> -				  const struct opal_step *steps)
> +static inline void setup_opal_dev(struct opal_dev *dev)
>  {
> -	dev->steps = steps;
>  	dev->tsn = 0;
>  	dev->hsn = 0;
>  	dev->prev_data = NULL;
> @@ -1855,14 +1847,13 @@ 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, },
> -		{ NULL, }
> +		{ opal_discovery0, }
>  	};
>  	int ret;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, steps, ARRAY_SIZE(steps));
>  	dev->supported = !ret;
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
> @@ -1919,14 +1910,13 @@ static int
> opal_secure_erase_locking_range(struct opal_dev *dev,
>  		{ start_auth_opal_session, opal_session },
>  		{ get_active_key, &opal_session->opal_key.lr },
>  		{ gen_key, },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, erase_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, erase_steps, ARRAY_SIZE(erase_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -1938,14 +1928,13 @@ static int opal_erase_locking_range(struct
> opal_dev *dev,
>  		{ opal_discovery0, },
>  		{ start_auth_opal_session, opal_session },
>  		{ erase_locking_range, opal_session },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, erase_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, erase_steps, ARRAY_SIZE(erase_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -1963,8 +1952,7 @@ static int
> opal_enable_disable_shadow_mbr(struct opal_dev *dev,
>  		{ end_opal_session, },
>  		{ start_admin1LSP_opal_session, &opal_mbr->key },
>  		{ set_mbr_enable_disable, &enable_disable },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
> @@ -1973,8 +1961,8 @@ static int
> opal_enable_disable_shadow_mbr(struct opal_dev *dev,
>  		return -EINVAL;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, mbr_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, mbr_steps, ARRAY_SIZE(mbr_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -1991,7 +1979,7 @@ static int opal_save(struct opal_dev *dev,
> struct opal_lock_unlock *lk_unlk)
>  	suspend->lr = lk_unlk->session.opal_key.lr;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, NULL);
> +	setup_opal_dev(dev);
>  	add_suspend_info(dev, suspend);
>  	mutex_unlock(&dev->dev_lock);
>  	return 0;
> @@ -2004,8 +1992,7 @@ static int opal_add_user_to_lr(struct opal_dev
> *dev,
>  		{ opal_discovery0, },
>  		{ start_admin1LSP_opal_session, &lk_unlk-
> >session.opal_key },
>  		{ add_user_to_lr, lk_unlk },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
> @@ -2027,8 +2014,8 @@ static int opal_add_user_to_lr(struct opal_dev
> *dev,
>  	}
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, steps, ARRAY_SIZE(steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -2038,14 +2025,13 @@ 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
> */
> -		{ NULL, }
> +		{ revert_tper, } /* controller will terminate session
> */
>  	};
>  	int ret;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, revert_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, revert_steps, ARRAY_SIZE(revert_steps));
>  	mutex_unlock(&dev->dev_lock);
>  
>  	/*
> @@ -2065,19 +2051,20 @@ static int __opal_lock_unlock(struct opal_dev
> *dev,
>  		{ opal_discovery0, },
>  		{ start_auth_opal_session, &lk_unlk->session },
>  		{ lock_unlock_locking_range, lk_unlk },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ 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, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  
> -	dev->steps = lk_unlk->session.sum ? unlock_sum_steps :
> unlock_steps;
> -	return next(dev);
> +	if (lk_unlk->session.sum)
> +		return next(dev, unlock_sum_steps,
> +			    ARRAY_SIZE(unlock_sum_steps));
> +	else
> +		return next(dev, unlock_steps,
> ARRAY_SIZE(unlock_steps));
>  }
>  
>  static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key
> *key)
> @@ -2087,12 +2074,10 @@ static int __opal_set_mbr_done(struct
> opal_dev *dev, struct opal_key *key)
>  		{ opal_discovery0, },
>  		{ start_admin1LSP_opal_session, key },
>  		{ set_mbr_done, &mbr_done_tf },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  
> -	dev->steps = mbrdone_step;
> -	return next(dev);
> +	return next(dev, mbrdone_step, ARRAY_SIZE(mbrdone_step));
>  }
>  
>  static int opal_lock_unlock(struct opal_dev *dev,
> @@ -2119,8 +2104,7 @@ static int opal_take_ownership(struct opal_dev
> *dev, struct opal_key *opal)
>  		{ end_opal_session, },
>  		{ start_SIDASP_opal_session, opal },
>  		{ set_sid_cpin_pin, opal },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
> @@ -2128,8 +2112,8 @@ static int opal_take_ownership(struct opal_dev
> *dev, struct opal_key *opal)
>  		return -ENODEV;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, owner_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, owner_steps, ARRAY_SIZE(owner_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -2142,8 +2126,7 @@ static int opal_activate_lsp(struct opal_dev
> *dev,
>  		{ start_SIDASP_opal_session, &opal_lr_act->key },
>  		{ get_lsp_lifecycle, },
>  		{ activate_lsp, opal_lr_act },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
> @@ -2151,8 +2134,8 @@ static int opal_activate_lsp(struct opal_dev
> *dev,
>  		return -EINVAL;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, active_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, active_steps, ARRAY_SIZE(active_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -2164,14 +2147,13 @@ static int opal_setup_locking_range(struct
> opal_dev *dev,
>  		{ opal_discovery0, },
>  		{ start_auth_opal_session, &opal_lrs->session },
>  		{ setup_locking_range, opal_lrs },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, lr_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, lr_steps, ARRAY_SIZE(lr_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -2182,8 +2164,7 @@ static int opal_set_new_pw(struct opal_dev
> *dev, struct opal_new_pw *opal_pw)
>  		{ opal_discovery0, },
>  		{ start_auth_opal_session, &opal_pw->session },
>  		{ set_new_pw, &opal_pw->new_user_pw },
> -		{ end_opal_session, },
> -		{ NULL }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
> @@ -2194,8 +2175,8 @@ static int opal_set_new_pw(struct opal_dev
> *dev, struct opal_new_pw *opal_pw)
>  		return -EINVAL;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, pw_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, pw_steps, ARRAY_SIZE(pw_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -2207,8 +2188,7 @@ static int opal_activate_user(struct opal_dev
> *dev,
>  		{ opal_discovery0, },
>  		{ start_admin1LSP_opal_session, &opal_session->opal_key 
> },
>  		{ internal_activate_user, opal_session },
> -		{ end_opal_session, },
> -		{ NULL, }
> +		{ end_opal_session, }
>  	};
>  	int ret;
>  
> @@ -2220,8 +2200,8 @@ static int opal_activate_user(struct opal_dev
> *dev,
>  	}
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, act_steps);
> -	ret = next(dev);
> +	setup_opal_dev(dev);
> +	ret = next(dev, act_steps, ARRAY_SIZE(act_steps));
>  	mutex_unlock(&dev->dev_lock);
>  	return ret;
>  }
> @@ -2238,7 +2218,7 @@ bool opal_unlock_from_suspend(struct opal_dev
> *dev)
>  		return false;
>  
>  	mutex_lock(&dev->dev_lock);
> -	setup_opal_dev(dev, NULL);
> +	setup_opal_dev(dev);
>  
>  	list_for_each_entry(suspend, &dev->unlk_lst, node) {
>  		dev->tsn = 0;

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

Re: [PATCH 14/16] block: sed-opal: pass steps via argument rather than via opal_dev

[Christoph Hellwig]

On Thu, Feb 14, 2019 at 01:16:06AM +0100, David Kozub wrote:
> The steps argument is only read by the next function, so it can
> be passed directly as an argument rather than via opal_dev.
> 
> Normally, the steps is an array on the stack, so the pointer stops
> being valid then the function that set opal_dev.steps returns.
> If opal_dev.steps was not set to NULL before return it would become
> a dangling pointer. When the steps are passed as argument this
> becomes easier to see and more difficult to misuse.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 14/16] block: sed-opal: pass steps via argument rather than via opal_dev

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:16:06AM +0100, David Kozub wrote:
> The steps argument is only read by the next function, so it can
> be passed directly as an argument rather than via opal_dev.
> 
> Normally, the steps is an array on the stack, so the pointer stops
> being valid then the function that set opal_dev.steps returns.
> If opal_dev.steps was not set to NULL before return it would become
> a dangling pointer. When the steps are passed as argument this
> becomes easier to see and more difficult to misuse.
> 
> Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

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

[David Kozub]

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 | 75 +++++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 33 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index f027c0cb682e..b947efd6d4d9 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -216,6 +216,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;
@@ -381,17 +382,33 @@ 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;
+	size_t state = 0;
+	int error;
 
-	for (state = 0; state < n_steps; state++) {
-		step = &steps[state];
+	/* first do a discovery0 */
+	error = opal_discovery0_step(dev);
+	if (error)
+		return error;
 
-		error = step->fn(dev, step->data);
+	for (state = 0; state < n_steps; state++) {
+		error = execute_step(dev, &steps[state], state);
 		if (error)
 			goto out_error;
 	}
@@ -400,14 +417,14 @@ static int next(struct opal_dev *dev, const struct opal_step *steps,
 
 out_error:
 	/*
-	 * For each OPAL command the first step in steps does a discovery0
-	 * and the second step starts some sort of session. If an error occurred
-	 * in the first two steps (and thus stopping the loop with state <= 1)
-	 * 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.
+	 * 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 occurred in the first step (and thus stopping the loop with
+	 * 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 (state > 1)
+	if (state > 0)
 		end_opal_session_error(dev);
 
 	return error;
@@ -506,6 +523,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 bool can_add(int *err, struct opal_dev *cmd, size_t len)
 {
 	if (*err)
@@ -1831,10 +1856,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)
@@ -1846,14 +1871,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;
@@ -1906,7 +1928,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, },
@@ -1925,7 +1946,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, }
@@ -1946,7 +1966,6 @@ static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 		OPAL_TRUE : OPAL_FALSE;
 
 	const struct opal_step mbr_steps[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, &opal_mbr->key },
 		{ set_mbr_done, &enable_disable },
 		{ end_opal_session, },
@@ -1989,7 +2008,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, }
@@ -2023,7 +2041,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 */
 	};
@@ -2048,13 +2065,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, }
@@ -2071,7 +2086,6 @@ static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
 {
 	u8 mbr_done_tf = OPAL_TRUE;
 	const struct opal_step mbrdone_step[] = {
-		{ opal_discovery0, },
 		{ start_admin1LSP_opal_session, key },
 		{ set_mbr_done, &mbr_done_tf },
 		{ end_opal_session, }
@@ -2098,7 +2112,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, },
@@ -2122,7 +2135,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 },
@@ -2144,7 +2156,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, }
@@ -2161,7 +2172,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, }
@@ -2185,7 +2195,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, }
-- 
2.20.1

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

[Derrick, Jonathan]

[-- Attachment #1: Type: text/plain, Size: 8898 bytes --]

Acked-by: Jon Derrick <jonathan.derrick@intel.com>

On Thu, 2019-02-14 at 01:16 +0100, David Kozub wrote:
> 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 | 75 +++++++++++++++++++++++++++-------------------
> --
>  1 file changed, 42 insertions(+), 33 deletions(-)
> 
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index f027c0cb682e..b947efd6d4d9 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -216,6 +216,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;
> @@ -381,17 +382,33 @@ 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;
> +	size_t state = 0;
> +	int error;
>  
> -	for (state = 0; state < n_steps; state++) {
> -		step = &steps[state];
> +	/* first do a discovery0 */
> +	error = opal_discovery0_step(dev);
> +	if (error)
> +		return error;
>  
> -		error = step->fn(dev, step->data);
> +	for (state = 0; state < n_steps; state++) {
> +		error = execute_step(dev, &steps[state], state);
>  		if (error)
>  			goto out_error;
>  	}
> @@ -400,14 +417,14 @@ static int next(struct opal_dev *dev, const
> struct opal_step *steps,
>  
>  out_error:
>  	/*
> -	 * For each OPAL command the first step in steps does a
> discovery0
> -	 * and the second step starts some sort of session. If an error
> occurred
> -	 * in the first two steps (and thus stopping the loop with
> state <= 1)
> -	 * 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.
> +	 * 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 occurred in the first step (and thus stopping the loop
> with
> +	 * 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 (state > 1)
> +	if (state > 0)
>  		end_opal_session_error(dev);
>  
>  	return error;
> @@ -506,6 +523,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 bool can_add(int *err, struct opal_dev *cmd, size_t len)
>  {
>  	if (*err)
> @@ -1831,10 +1856,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)
> @@ -1846,14 +1871,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;
> @@ -1906,7 +1928,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, },
> @@ -1925,7 +1946,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, }
> @@ -1946,7 +1966,6 @@ static int
> opal_enable_disable_shadow_mbr(struct opal_dev *dev,
>  		OPAL_TRUE : OPAL_FALSE;
>  
>  	const struct opal_step mbr_steps[] = {
> -		{ opal_discovery0, },
>  		{ start_admin1LSP_opal_session, &opal_mbr->key },
>  		{ set_mbr_done, &enable_disable },
>  		{ end_opal_session, },
> @@ -1989,7 +2008,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, }
> @@ -2023,7 +2041,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
> */
>  	};
> @@ -2048,13 +2065,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, }
> @@ -2071,7 +2086,6 @@ static int __opal_set_mbr_done(struct opal_dev
> *dev, struct opal_key *key)
>  {
>  	u8 mbr_done_tf = OPAL_TRUE;
>  	const struct opal_step mbrdone_step[] = {
> -		{ opal_discovery0, },
>  		{ start_admin1LSP_opal_session, key },
>  		{ set_mbr_done, &mbr_done_tf },
>  		{ end_opal_session, }
> @@ -2098,7 +2112,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, },
> @@ -2122,7 +2135,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 },
> @@ -2144,7 +2156,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, }
> @@ -2161,7 +2172,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, }
> @@ -2185,7 +2195,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, }
> -- 
> 2.20.1
> 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3278 bytes --]

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

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:16:07AM +0100, David Kozub wrote:
> 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>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>

[PATCH 16/16] block: sed-opal: rename next to execute_steps

[David Kozub]

As the function is responsible for executing the individual steps supplied
in the steps argument, execute_steps is a more descriptive name than the
rather generic next.

Signed-off-by: David Kozub <zub@linux.fjfi.cvut.cz>
Reviewed-by: Scott Bauer <sbauer@plzdonthack.me>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index b947efd6d4d9..b1aa0cc25803 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -396,8 +396,8 @@ static int execute_step(struct opal_dev *dev,
 	return error;
 }
 
-static int next(struct opal_dev *dev, const struct opal_step *steps,
-		size_t n_steps)
+static int execute_steps(struct opal_dev *dev,
+			 const struct opal_step *steps, size_t n_steps)
 {
 	size_t state = 0;
 	int error;
@@ -1937,7 +1937,7 @@ static int opal_secure_erase_locking_range(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, erase_steps, ARRAY_SIZE(erase_steps));
+	ret = execute_steps(dev, erase_steps, ARRAY_SIZE(erase_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -1954,7 +1954,7 @@ static int opal_erase_locking_range(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, erase_steps, ARRAY_SIZE(erase_steps));
+	ret = execute_steps(dev, erase_steps, ARRAY_SIZE(erase_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -1981,7 +1981,7 @@ static int opal_enable_disable_shadow_mbr(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, mbr_steps, ARRAY_SIZE(mbr_steps));
+	ret = execute_steps(dev, mbr_steps, ARRAY_SIZE(mbr_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2033,7 +2033,7 @@ static int opal_add_user_to_lr(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, steps, ARRAY_SIZE(steps));
+	ret = execute_steps(dev, steps, ARRAY_SIZE(steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2048,7 +2048,7 @@ static int opal_reverttper(struct opal_dev *dev, struct opal_key *opal)
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, revert_steps, ARRAY_SIZE(revert_steps));
+	ret = execute_steps(dev, revert_steps, ARRAY_SIZE(revert_steps));
 	mutex_unlock(&dev->dev_lock);
 
 	/*
@@ -2076,10 +2076,11 @@ static int __opal_lock_unlock(struct opal_dev *dev,
 	};
 
 	if (lk_unlk->session.sum)
-		return next(dev, unlock_sum_steps,
-			    ARRAY_SIZE(unlock_sum_steps));
+		return execute_steps(dev, unlock_sum_steps,
+				     ARRAY_SIZE(unlock_sum_steps));
 	else
-		return next(dev, unlock_steps, ARRAY_SIZE(unlock_steps));
+		return execute_steps(dev, unlock_steps,
+				     ARRAY_SIZE(unlock_steps));
 }
 
 static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
@@ -2091,7 +2092,7 @@ static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key)
 		{ end_opal_session, }
 	};
 
-	return next(dev, mbrdone_step, ARRAY_SIZE(mbrdone_step));
+	return execute_steps(dev, mbrdone_step, ARRAY_SIZE(mbrdone_step));
 }
 
 static int opal_lock_unlock(struct opal_dev *dev,
@@ -2126,7 +2127,7 @@ static int opal_take_ownership(struct opal_dev *dev, struct opal_key *opal)
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, owner_steps, ARRAY_SIZE(owner_steps));
+	ret = execute_steps(dev, owner_steps, ARRAY_SIZE(owner_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2147,7 +2148,7 @@ static int opal_activate_lsp(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, active_steps, ARRAY_SIZE(active_steps));
+	ret = execute_steps(dev, active_steps, ARRAY_SIZE(active_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2164,7 +2165,7 @@ static int opal_setup_locking_range(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, lr_steps, ARRAY_SIZE(lr_steps));
+	ret = execute_steps(dev, lr_steps, ARRAY_SIZE(lr_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2186,7 +2187,7 @@ static int opal_set_new_pw(struct opal_dev *dev, struct opal_new_pw *opal_pw)
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, pw_steps, ARRAY_SIZE(pw_steps));
+	ret = execute_steps(dev, pw_steps, ARRAY_SIZE(pw_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
@@ -2210,7 +2211,7 @@ static int opal_activate_user(struct opal_dev *dev,
 
 	mutex_lock(&dev->dev_lock);
 	setup_opal_dev(dev);
-	ret = next(dev, act_steps, ARRAY_SIZE(act_steps));
+	ret = execute_steps(dev, act_steps, ARRAY_SIZE(act_steps));
 	mutex_unlock(&dev->dev_lock);
 	return ret;
 }
-- 
2.20.1

Re: [PATCH 00/16] sed-opal: fix shadow MBR enable/disable and clean up code

[Scott Bauer]

On Thu, Feb 14, 2019 at 01:15:52AM +0100, David Kozub wrote:
> David Kozub (12):
>   block: sed-opal: fix IOC_OPAL_ENABLE_DISABLE_MBR
>   block: sed-opal: fix typos and formatting
>   block: sed-opal: close parameter list in cmd_finalize
>   block: sed-opal: unify cmd start
>   block: sed-opal: unify error handling of responses
>   block: sed-opal: reuse response_get_token to decrease code duplication
>   block: sed-opal: add token for OPAL_LIFECYCLE
>   block: sed-opal: unify retrieval of table columns
>   block: sed-opal: use named Opal tokens instead of integer literals
>   block: sed-opal: pass steps via argument rather than via opal_dev
>   block: sed-opal: don't repeat opal_discovery0 in each steps array
>   block: sed-opal: rename next to execute_steps
> 
> Jonas Rabenstein (4):
>   block: sed-opal: use correct macro for method length
>   block: sed-opal: unify space check in add_token_*
>   block: sed-opal: print failed function address
>   block: sed-opal: split generation of bytestring header and content
> 
>  block/opal_proto.h            |   2 +
>  block/sed-opal.c              | 716 ++++++++++++++--------------------
>  include/uapi/linux/sed-opal.h |   2 +-
>  3 files changed, 287 insertions(+), 433 deletions(-)

Jens,

We have all the reviews done for this series from Jon, Christoph, and myself.
When you get time will you please queue this up for 5.2?

Thank you.

Re: [PATCH 00/16] sed-opal: fix shadow MBR enable/disable and clean up code

[Jens Axboe]

On 4/6/19 9:26 AM, Scott Bauer wrote:
> 
> 
> 
> On Thu, Feb 14, 2019 at 01:15:52AM +0100, David Kozub wrote:
>> David Kozub (12):
>>   block: sed-opal: fix IOC_OPAL_ENABLE_DISABLE_MBR
>>   block: sed-opal: fix typos and formatting
>>   block: sed-opal: close parameter list in cmd_finalize
>>   block: sed-opal: unify cmd start
>>   block: sed-opal: unify error handling of responses
>>   block: sed-opal: reuse response_get_token to decrease code duplication
>>   block: sed-opal: add token for OPAL_LIFECYCLE
>>   block: sed-opal: unify retrieval of table columns
>>   block: sed-opal: use named Opal tokens instead of integer literals
>>   block: sed-opal: pass steps via argument rather than via opal_dev
>>   block: sed-opal: don't repeat opal_discovery0 in each steps array
>>   block: sed-opal: rename next to execute_steps
>>
>> Jonas Rabenstein (4):
>>   block: sed-opal: use correct macro for method length
>>   block: sed-opal: unify space check in add_token_*
>>   block: sed-opal: print failed function address
>>   block: sed-opal: split generation of bytestring header and content
>>
>>  block/opal_proto.h            |   2 +
>>  block/sed-opal.c              | 716 ++++++++++++++--------------------
>>  include/uapi/linux/sed-opal.h |   2 +-
>>  3 files changed, 287 insertions(+), 433 deletions(-)
> 
> Jens,
> 
> We have all the reviews done for this series from Jon, Christoph, and myself.
> When you get time will you please queue this up for 5.2?

Applied, thanks everyone.

-- 
Jens Axboe

[PATCH 12/16] block: sed-opal: unify retrieval of table columns

[Jonas Rabenstein]

Instead of having multiple places defining the same argument list to get
a specific column of a sed-opal table, provide a generic version and
call it from those functions.

Signed-off-by: Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
---
 block/opal_proto.h |   2 +
 block/sed-opal.c   | 132 +++++++++++++++++----------------------------
 2 files changed, 50 insertions(+), 84 deletions(-)

diff --git a/block/opal_proto.h b/block/opal_proto.h
index e20be8258854..b6e352cfe982 100644
--- a/block/opal_proto.h
+++ b/block/opal_proto.h
@@ -170,6 +170,8 @@ enum opal_token {
 	OPAL_READLOCKED = 0x07,
 	OPAL_WRITELOCKED = 0x08,
 	OPAL_ACTIVEKEY = 0x0A,
+	/* lockingsp table */
+	OPAL_LIFECYCLE = 0x06,
 	/* locking info table */
 	OPAL_MAXRANGES = 0x04,
 	 /* mbr control */
diff --git a/block/sed-opal.c b/block/sed-opal.c
index ee24e4e7ad6e..034cb940b34a 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -1089,6 +1089,37 @@ static int finalize_and_send(struct opal_dev *dev, cont_fn cont)
 	return opal_send_recv(dev, cont);
 }
 
+/*
+ * request @column from table @table on device @dev. On success, the column
+ * data will be available in dev->resp->tok[4]
+ */
+static int generic_get_column(struct opal_dev *dev, const u8 *table,
+			      u64 column)
+{
+	int err;
+
+	err = cmd_start(dev, table, opalmethod[OPAL_GET]);
+
+	add_token_u8(&err, dev, OPAL_STARTLIST);
+
+	add_token_u8(&err, dev, OPAL_STARTNAME);
+	add_token_u8(&err, dev, OPAL_STARTCOLUMN);
+	add_token_u64(&err, dev, column);
+	add_token_u8(&err, dev, OPAL_ENDNAME);
+
+	add_token_u8(&err, dev, OPAL_STARTNAME);
+	add_token_u8(&err, dev, OPAL_ENDCOLUMN);
+	add_token_u64(&err, dev, column);
+	add_token_u8(&err, dev, OPAL_ENDNAME);
+
+	add_token_u8(&err, dev, OPAL_ENDLIST);
+
+	if (err)
+		return err;
+
+	return finalize_and_send(dev, parse_and_check_status);
+}
+
 static int gen_key(struct opal_dev *dev, void *data)
 {
 	u8 uid[OPAL_UID_LENGTH];
@@ -1143,23 +1174,11 @@ static int get_active_key(struct opal_dev *dev, void *data)
 	if (err)
 		return err;
 
-	err = cmd_start(dev, uid, opalmethod[OPAL_GET]);
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* startCloumn */
-	add_token_u8(&err, dev, 10); /* ActiveKey */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 4); /* endColumn */
-	add_token_u8(&err, dev, 10); /* ActiveKey */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
-	if (err) {
-		pr_debug("Error building get active key command\n");
+	err = generic_get_column(dev, uid, OPAL_ACTIVEKEY);
+	if (err)
 		return err;
-	}
 
-	return finalize_and_send(dev, get_active_key_cont);
+	return get_active_key_cont(dev);
 }
 
 static int generic_lr_enable_disable(struct opal_dev *dev,
@@ -1819,17 +1838,19 @@ static int activate_lsp(struct opal_dev *dev, void *data)
 	return finalize_and_send(dev, parse_and_check_status);
 }
 
-static int get_lsp_lifecycle_cont(struct opal_dev *dev)
+/* Determine if we're in the Manufactured Inactive or Active state */
+static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
 {
 	u8 lc_status;
-	int error = 0;
+	int err;
 
-	error = parse_and_check_status(dev);
-	if (error)
-		return error;
+	err = generic_get_column(dev, opaluid[OPAL_LOCKINGSP_UID],
+				 OPAL_LIFECYCLE);
+	if (err)
+		return err;
 
 	lc_status = response_get_u64(&dev->parsed, 4);
-	/* 0x08 is Manufacured Inactive */
+	/* 0x08 is Manufactured Inactive */
 	/* 0x09 is Manufactured */
 	if (lc_status != OPAL_MANUFACTURED_INACTIVE) {
 		pr_debug("Couldn't determine the status of the Lifecycle state\n");
@@ -1839,49 +1860,19 @@ static int get_lsp_lifecycle_cont(struct opal_dev *dev)
 	return 0;
 }
 
-/* Determine if we're in the Manufactured Inactive or Active state */
-static int get_lsp_lifecycle(struct opal_dev *dev, void *data)
-{
-	int err;
-
-	err = cmd_start(dev, opaluid[OPAL_LOCKINGSP_UID],
-			opalmethod[OPAL_GET]);
-
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* Start Column */
-	add_token_u8(&err, dev, 6); /* Lifecycle Column */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 4); /* End Column */
-	add_token_u8(&err, dev, 6); /* Lifecycle Column */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_ENDLIST);
-
-	if (err) {
-		pr_debug("Error Building GET Lifecycle Status command\n");
-		return err;
-	}
-
-	return finalize_and_send(dev, get_lsp_lifecycle_cont);
-}
-
-static int get_msid_cpin_pin_cont(struct opal_dev *dev)
+static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
 {
 	const char *msid_pin;
 	size_t strlen;
-	int error = 0;
+	int err;
 
-	error = parse_and_check_status(dev);
-	if (error)
-		return error;
+	err = generic_get_column(dev, opaluid[OPAL_C_PIN_MSID], OPAL_PIN);
+	if (err)
+		return err;
 
 	strlen = response_get_string(&dev->parsed, 4, &msid_pin);
 	if (!msid_pin) {
-		pr_debug("%s: Couldn't extract PIN from response\n", __func__);
+		pr_debug("Couldn't extract MSID_CPIN from response\n");
 		return OPAL_INVAL_PARAM;
 	}
 
@@ -1894,33 +1885,6 @@ static int get_msid_cpin_pin_cont(struct opal_dev *dev)
 	return 0;
 }
 
-static int get_msid_cpin_pin(struct opal_dev *dev, void *data)
-{
-	int err;
-
-	err = cmd_start(dev, opaluid[OPAL_C_PIN_MSID],
-			opalmethod[OPAL_GET]);
-
-	add_token_u8(&err, dev, OPAL_STARTLIST);
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 3); /* Start Column */
-	add_token_u8(&err, dev, 3); /* PIN */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-
-	add_token_u8(&err, dev, OPAL_STARTNAME);
-	add_token_u8(&err, dev, 4); /* End Column */
-	add_token_u8(&err, dev, 3); /* Lifecycle Column */
-	add_token_u8(&err, dev, OPAL_ENDNAME);
-	add_token_u8(&err, dev, OPAL_ENDLIST);
-
-	if (err) {
-		pr_debug("Error building Get MSID CPIN PIN command.\n");
-		return err;
-	}
-
-	return finalize_and_send(dev, get_msid_cpin_pin_cont);
-}
-
 static int end_opal_session(struct opal_dev *dev, void *data)
 {
 	int err = 0;
-- 
2.20.1