Messages in this thread | | | Date | Thu, 7 Feb 2019 23:56:48 +0100 (CET) | From | David Kozub <> | Subject | Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of shadow mbr |
| |
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@studium.uni-erlangen.de> >> >> 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@studium.uni-erlangen.de> >> Reviewed-by: Scott Bauer <sbauer@plzdonthack.me> >> --- >> 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
| |