On Tue, 2022-08-09 at 15:02 +0200, Christian Brauner wrote:
> On Fri, Aug 05, 2022 at 11:55:22PM +0100, luca.boccassi@xxxxxxxxx wrote:
> > From: "dougmill@xxxxxxxxxxxxxxxxxx" <dougmill@xxxxxxxxxxxxxxxxxx>
> >
> > Provide a mechanism to retrieve basic status information about
> > the device, including the "supported" flag indicating whether
> > SED-OPAL is supported. The information returned is from the various
> > feature descriptors received during the discovery0 step, and so
> > this ioctl does nothing more than perform the discovery0 step
> > and then save the information received. See "struct opal_status"
> > and OPAL_FL_* bits for the status information currently returned.
> >
> > Signed-off-by: Douglas Miller <dougmill@xxxxxxxxxxxxxxxxxx>
> > Tested-by: Luca Boccassi <bluca@xxxxxxxxxx>
> > ---
>
> No expert in this area but I think this looks straightforward overall.
> But could you expand on the use-case a bit in the commit message.
> I have a tiny suggestion/questions below other than that seems like
> useful info to expose.
Thank you for your review, updated in v6 (sent v5 with the struct
update but forgot to update this too).
> >
> > +struct opal_status {
> > + __u32 flags;
> > +};
>
> You expect this struct to reasonably grow additional members? If not you
> might just pass a single __u64?
>
> In case that the struct makes more sense: I don't know what's common in
> this subsystem but for other parts of the kernel we try to align all
> structs to 64 bits. So it wouldn't hurt to do that here too. Either by
> turning that into __u64 flags or by adding a __u32 reserved member.
I believe it is a good idea to be able to expand in the future, since
this is driven by a hardware protocol. Added '__u32 reserved' as
suggested in v6.
--
Kind regards,
Luca Boccassi
