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:   Sun, 10 Feb 2019 13:26:55 -0500
From:   Scott Bauer <sbauer@plzdonthack.me>
To:     "Derrick, Jonathan" <jonathan.derrick@intel.com>
Cc:     "hch@infradead.org" <hch@infradead.org>,
        "zub@linux.fjfi.cvut.cz" <zub@linux.fjfi.cvut.cz>,
        "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>
Message-ID: <20190210182655.GA20491@hacktheplanet>
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>
 <alpine.LRH.2.21.1902072247060.29258@linux.fjfi.cvut.cz>
 <1549586652.11868.12.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1549586652.11868.12.camel@intel.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Subject: Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of
 shadow mbr
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

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.