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

From: David Kozub <zub@linux.fjfi.cvut.cz>
To: Scott Bauer <sbauer@plzdonthack.me>
Cc: "Derrick, Jonathan" <jonathan.derrick@intel.com>,
	"hch@infradead.org" <hch@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"jonas.rabenstein@studium.uni-erlangen.de" 
	<jonas.rabenstein@studium.uni-erlangen.de>,
	"axboe@kernel.dk" <axboe@kernel.dk>
Subject: Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of shadow mbr
Date: Sun, 10 Feb 2019 21:25:55 +0100 (CET)	[thread overview]
Message-ID: <alpine.LRH.2.21.1902102117220.8545@linux.fjfi.cvut.cz> (raw)
In-Reply-To: <20190210182655.GA20491@hacktheplanet>

On Sun, 10 Feb 2019, Scott Bauer wrote:

> On Fri, Feb 08, 2019 at 12:44:14AM +0000, Derrick, Jonathan wrote:
>> On Thu, 2019-02-07 at 23:56 +0100, David Kozub wrote:
>>> On Mon, 4 Feb 2019, Christoph Hellwig wrote:
>>>>>  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 },
>
>>> 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).
>
> Agreed the original code did the opposite of what the user wanted, looks like
> when I authored it I messed up that enum which set everything off.
>
>
>
>>> 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.
>
> For the new ioctl I think we should just add a new enum with the correct
> nomenclature.  So OPAL_MBR_DONE, OPAL_MBR_NOT_DONE.
>
>
>> In order to keep the userspace interface consistent, I'll ACK your
>> change in this patch, unless Scott can fill me in on why this looks
>> wrong but is actually right.
>
> I think it is just wrong.
>
>
>>
>> We have 7 bytes in the opal_mbr_data struct we could use for DONE/NOT
>> DONE. I'm not sure how to go about keeping it consistent with old uapi,
>> although arguably opal_enable_disable_shadow_mbr is already doing the
>> wrong thing with DONE and ENABLE so it's low impact.
>
> Can we keep the old mbr struct the same and just add a new struct with new enums
> for the new done ioctl? I think this will keep the new ioctl cleaner instead
> of trying to apply older, some what incorrectly named, enums.

I like this proposal, I'll try to implement it. Although currently I plan 
to first re-submit the cleanup parts, as suggested by Christoph[1]. So 
this will happen after that.

> Lastly someone will need to backport his
>
>>>>> +       u8 token = opal_mbr->enable_disable == OPAL_MBR_ENABLE
>>>>> +               ? OPAL_TRUE : OPAL_FALSE;
>
> to stable so we can fix up my broken coding in older kernels.
>
>
> I can do that or, if David wants to do that that's fine... just want to coordinate.

I'm quite busy juggling patches in this series. If you can find the time, 
please do it.

Best regards,
David

[1] https://lore.kernel.org/lkml/20190204150415.GO31132@infradead.org/