| Message ID | 20170831174238.2714-1-scott.bauer@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 31, 2017 at 02:10:36PM -0600, Jon Derrick wrote: > Hey Scott, > > +static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key) > > +{ > > + u8 mbr_done_tf = 1; > > + const struct opal_step mbrdone_step [] = { > > + { opal_discovery0, }, > > + { start_admin1LSP_opal_session, key }, > > + { set_mbr_done, &mbr_done_tf }, > Do you need to end_opal_session here? > Yep, sure do. I'll wait for Christoph to look at it tonight before spinning another. > > + { NULL, } > > + }; > > +
Hey Scott, > +static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key) > +{ > + u8 mbr_done_tf = 1; > + const struct opal_step mbrdone_step [] = { > + { opal_discovery0, }, > + { start_admin1LSP_opal_session, key }, > + { set_mbr_done, &mbr_done_tf }, Do you need to end_opal_session here? > + { NULL, } > + }; > +
On Thu, Aug 31, 2017 at 02:19:44PM -0600, Jens Axboe wrote: > On Thu, Aug 31 2017, Scott Bauer wrote: > > @@ -2345,6 +2371,11 @@ bool opal_unlock_from_suspend(struct opal_dev *dev) > > suspend->unlk.session.sum); > > was_failure = true; > > } > > + if (dev->mbr_enabled) { > > + ret = __opal_set_mbr_done(dev, &suspend->unlk.session.opal_key); > > + if (ret) > > + pr_debug("Failed to set MBR Done in S3 resume\n"); > > + } > > Should ret != 0 set was_failure = true here? I thought about that too and decided against it. The reasoning is was_failure was supposed to designate an unlock failure, specifically on the unlock comamnd, not the new mbr_enable command. An unlock can still succeed and the MBR set can fail under some extreme scenario, in which case the pr_debug will let us know (maybe we should promote that to pr_warn?). More over, it seems like none of the callers scsi/nvme are even using the return value. Since not being able to unlock a range isn't a failure of the actual bring up of the drive everyone ignores It, I guess. Since no one is using the return value "was_failure" perhaps I should just refactor this to void. That is unless others think it should stay for potential future devices which may care if something isn't unlocked and want to do something else? > > -- > Jens Axboe >
On Thu, Aug 31 2017, Scott Bauer wrote: > @@ -2345,6 +2371,11 @@ bool opal_unlock_from_suspend(struct opal_dev *dev) > suspend->unlk.session.sum); > was_failure = true; > } > + if (dev->mbr_enabled) { > + ret = __opal_set_mbr_done(dev, &suspend->unlk.session.opal_key); > + if (ret) > + pr_debug("Failed to set MBR Done in S3 resume\n"); > + } Should ret != 0 set was_failure = true here?
On Thu, Aug 31, 2017 at 01:58:01PM -0600, Scott Bauer wrote: > > > + { set_mbr_done, &mbr_done_tf }, > > Do you need to end_opal_session here? > > > Yep, sure do. I'll wait for Christoph to look at it tonight before spinning another. No additional comment from me, please just resend with that fix.
diff --git a/block/opal_proto.h b/block/opal_proto.h index f40c9acf8895..e20be8258854 100644 --- a/block/opal_proto.h +++ b/block/opal_proto.h @@ -46,6 +46,7 @@ enum opal_response_token { #define GENERIC_HOST_SESSION_NUM 0x41 #define TPER_SYNC_SUPPORTED 0x01 +#define MBR_ENABLED_MASK 0x10 #define TINY_ATOM_DATA_MASK 0x3F #define TINY_ATOM_SIGNED 0x40 diff --git a/block/sed-opal.c b/block/sed-opal.c index 9b30ae5ab843..35ddb6026334 100644 --- a/block/sed-opal.c +++ b/block/sed-opal.c @@ -80,6 +80,7 @@ struct parsed_resp { struct opal_dev { bool supported; + bool mbr_enabled; void *data; sec_send_recv *send_recv; @@ -283,6 +284,14 @@ static bool check_tper(const void *data) return true; } +static bool check_mbrenabled(const void *data) +{ + const struct d0_locking_features *lfeat = data; + u8 sup_feat = lfeat->supported_features; + + return !!(sup_feat & MBR_ENABLED_MASK); +} + static bool check_sum(const void *data) { const struct d0_single_user_mode *sum = data; @@ -417,6 +426,7 @@ static int opal_discovery0_end(struct opal_dev *dev) u32 hlen = be32_to_cpu(hdr->length); print_buffer(dev->resp, hlen); + dev->mbr_enabled = false; if (hlen > IO_BUFFER_LENGTH - sizeof(*hdr)) { pr_debug("Discovery length overflows buffer (%zu+%u)/%u\n", @@ -442,6 +452,8 @@ static int opal_discovery0_end(struct opal_dev *dev) check_geometry(dev, body); break; case FC_LOCKING: + dev->mbr_enabled = check_mbrenabled(body->features); + break; case FC_ENTERPRISE: case FC_DATASTORE: /* some ignored properties */ @@ -2190,6 +2202,20 @@ static int __opal_lock_unlock(struct opal_dev *dev, return next(dev); } +static int __opal_set_mbr_done(struct opal_dev *dev, struct opal_key *key) +{ + u8 mbr_done_tf = 1; + const struct opal_step mbrdone_step [] = { + { opal_discovery0, }, + { start_admin1LSP_opal_session, key }, + { set_mbr_done, &mbr_done_tf }, + { NULL, } + }; + + dev->steps = mbrdone_step; + return next(dev); +} + static int opal_lock_unlock(struct opal_dev *dev, struct opal_lock_unlock *lk_unlk) { @@ -2345,6 +2371,11 @@ bool opal_unlock_from_suspend(struct opal_dev *dev) suspend->unlk.session.sum); was_failure = true; } + if (dev->mbr_enabled) { + ret = __opal_set_mbr_done(dev, &suspend->unlk.session.opal_key); + if (ret) + pr_debug("Failed to set MBR Done in S3 resume\n"); + } } mutex_unlock(&dev->dev_lock); return was_failure;
Users who are booting off their Opal enabled drives are having issues when they have a shadow MBR set up after s3/resume cycle. When the Drive has a shadow MBR setup the MBRDone flag is set to false upon power loss (S3/S4/S5). When the MBRDone flag is false I/O to LBA 0 -> LBA_END_MBR are remapped to the shadow mbr of the drive. If the drive contains useful data in the 0 -> end_mbr range upon s3 resume the user can never get to that data as the drive will keep remapping it to the MBR. To fix this when we unlock on S3 resume, we need to tell the drive that we're done with the shadow mbr (even though we didnt use it) by setting true to MBRDone. This way the drive will stop the remapping and the user can access their data. Signed-off-by: Scott Bauer <scott.bauer@intel.com> --- block/opal_proto.h | 1 + block/sed-opal.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)