Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 7 Feb 2019 23:56:48 +0100 (CET)
From:   David Kozub <zub@linux.fjfi.cvut.cz>
To:     Christoph Hellwig <hch@infradead.org>
cc:     Jens Axboe <axboe@kernel.dk>,
        Jonathan Derrick <jonathan.derrick@intel.com>,
        Scott Bauer <sbauer@plzdonthack.me>,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        Jonas Rabenstein <jonas.rabenstein@studium.uni-erlangen.de>
Subject: Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of
 shadow mbr
In-Reply-To: <20190204145244.GJ31132@infradead.org>
Message-ID: <alpine.LRH.2.21.1902072247060.29258@linux.fjfi.cvut.cz>
References: <1549054223-12220-1-git-send-email-zub@linux.fjfi.cvut.cz> <1549054223-12220-11-git-send-email-zub@linux.fjfi.cvut.cz> <20190204145244.GJ31132@infradead.org>
User-Agent: Alpine 2.21 (LRH 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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