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@xxxxxxxxxxxxx/
