Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of shadow mbr

[David Kozub]

On Mon, 4 Feb 2019, Christoph Hellwig wrote:

> On Fri, Feb 01, 2019 at 09:50:17PM +0100, David Kozub wrote:
> > From: Jonas Rabenstein <jonas.rabenstein@xxxxxxxxxxxxxxxxxxxxxxx>
> >
> > Enable users to mark the shadow mbr as done without completely
> > deactivating the shadow mbr feature. This may be useful on reboots,
> > when the power to the disk is not disconnected in between and the shadow
> > mbr stores the required boot files. Of course, this saves also the
> > (few) commands required to enable the feature if it is already enabled
> > and one only wants to mark the shadow mbr as done.
> >
> > Signed-off-by: Jonas Rabenstein <jonas.rabenstein@xxxxxxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Scott Bauer <sbauer@xxxxxxxxxxxxxx>
> > ---
> >  block/sed-opal.c              | 33 +++++++++++++++++++++++++++++++--
> >  include/linux/sed-opal.h      |  1 +
> >  include/uapi/linux/sed-opal.h |  1 +
> >  3 files changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > index 4b0a63b9d7c9..e03838cfd31b 100644
> > --- a/block/sed-opal.c
> > +++ b/block/sed-opal.c
> > @@ -1996,13 +1996,39 @@ 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 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
> > +		? OPAL_TRUE : OPAL_FALSE;
> >  	const struct opal_step mbr_steps[] = {
> >  		{ opal_discovery0, },
> >  		{ start_admin1LSP_opal_session, &opal_mbr->key },
> > -		{ set_mbr_done, &opal_mbr->enable_disable },
> > +		{ set_mbr_done, &token },
> >  		{ end_opal_session, },
> >  		{ start_admin1LSP_opal_session, &opal_mbr->key },
> > -		{ set_mbr_enable_disable, &opal_mbr->enable_disable },
> > +		{ set_mbr_enable_disable, &token },
> > +		{ end_opal_session, },
> > +		{ NULL, }
>
> This seems to be a change of what we pass to set_mbr_done /
> set_mbr_enable_disable and not really related to the new functionality
> here, so it should be split into a separate patch.
>
> That being said if we really care about this translation between
> the two sets of constants, why not do it inside
> set_mbr_done and set_mbr_enable_disable?

Hi Christoph,

I agree, this should be split. Furthermore I think I found an issue here: 
OPAL_MBR_ENABLE and OPAL_MBR_DISABLE are defined as follows:

enum opal_mbr {
	OPAL_MBR_ENABLE = 0x0,
	OPAL_MBR_DISABLE = 0x01,
};

... while OPAL_TRUE and OPAL_FALSE tokens are:

	OPAL_TRUE = 0x01,
	OPAL_FALSE = 0x00,

so in the current code in kernel, when the IOCTL input is directly passed 
in place of the TRUE/FALSE tokens (in opal_enable_disable_shadow_mbr), 
passing OPAL_MBR_ENABLE (0) to IOC_OPAL_ENABLE_DISABLE_MBR ends up being 
interpreted as OPAL_FALSE (0) and passing OPAL_MBR_DISABLE (1) ended up 
being interpreted as OPAL_TRUE (1). So the behavior is:

OPAL_MBR_ENABLE: set MBR enable to OPAL_FALSE and done to OPAL_FALSE
OPAL_MBR_DISABLE: set MBR enable to OPAL_TRUE and done to OPAL_TRUE

Am I missing something here? This seems wrong to me. And I think this 
patch actually changes it by introducing:

+    u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
+            ? OPAL_TRUE : OPAL_FALSE;

which is essentially a negation (map 0 to 1 and 1 to 0).

I had a strange feeling of IOC_OPAL_ENABLE_DISABLE_MBR behaving 
incorrectly when I first tried it. But when I checked later I was not able 
to reproduce it - probably originally I tested without this patch.

With regard to the new IOC_OPAL_MBR_STATUS: I find the usage of 
OPAL_MBR_ENABLE/DISABLE for this confusing: what should passing 
OPAL_MBR_ENABLE do? Should it enable the shadow MBR? Or should it 
enable the MBR-done flag? I think the implementation in this patch 
interprets OPAL_MBR_ENABLE as 'set the "done" flag to true', thus hiding 
the shadow MBR. But this is not obvious looking at the IOCTL name.

What if I introduced two new constants for this? OPAL_MBR_DONE and 
OPAL_MBR_NOT_DONE? Maybe the IOCTL could be renamed too - 
IOC_OPAL_MBR_DONE? Or is it only me who finds this confusing?

Best regards,
David