Messages in this thread | | | Date | Sun, 10 Feb 2019 13:26:55 -0500 | From | Scott Bauer <> | Subject | Re: [PATCH v4 10/16] block: sed-opal: add ioctl for done-mark of shadow mbr |
| |
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.
| |