Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

On Tue, Dec 20, 2016 at 03:07:46PM -0700, Jon Derrick wrote:
> > This pretty much seem to contain the OPAL protocol defintions, so why
> > not opal_proto.h?
> Since there might eventually be a whole class of opal-like sed
> protocols, why does it make more sense to have opal_proto.h instead of
> sed-opal.h or some variation? This is similar to how leds-*.h look to
> me. Although I agree that sed-ATA.h would be dishonest since ATA
> security doesn't imply a self-encrypting-disk.

As far as I can tell the NVMe / SCSI / ATA security landscape looks
like:

 - ATA security - specified in ATA, kinda implicitly referenced by SPC
   and then even more implitly in NVMe.  Actually implemented in various
   NVMe consumer devices because OEMs insist on it.  Basically just
   a trivial plain text password lock/unlock.

 - TCG OPAL including the subsets OPALite and Pyrite, whereas the latter
   is just the above lock/unlock in a TCG way.

 - TCG Enterprise SSC

I think it's pretty obvious that ATA security should be something on
it's own.  OPALite and Pyrite are strict subsets of OPAL, so having them
in something named opal shouldn't be surprising.  The big question
is what to do about Enterprise SSC.  Which has some overlaps with OPAL
but also major differences, so keeping it separate if we ever have to
implement it (I hope we don't) would be best.

> I'm on board with this if you think we won't have enough different, but
> similar, SED protocols to justify the indirection. In that case you can
> ignore the above comment as well.

This goes back to the above.  The only thing that is not a strict subset
of OPAL but kinda sorta similar is TCG Enterprise SSC, but my preference
would be to ignore it, and the second best preference would be to keep
it separate.

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

On Tue, Dec 20, 2016 at 03:07:46PM -0700, Jon Derrick wrote:
> > This pretty much seem to contain the OPAL protocol defintions, so why
> > not opal_proto.h?
> Since there might eventually be a whole class of opal-like sed
> protocols, why does it make more sense to have opal_proto.h instead of
> sed-opal.h or some variation? This is similar to how leds-*.h look to
> me. Although I agree that sed-ATA.h would be dishonest since ATA
> security doesn't imply a self-encrypting-disk.

As far as I can tell the NVMe / SCSI / ATA security landscape looks
like:

 - ATA security - specified in ATA, kinda implicitly referenced by SPC
   and then even more implitly in NVMe.  Actually implemented in various
   NVMe consumer devices because OEMs insist on it.  Basically just
   a trivial plain text password lock/unlock.

 - TCG OPAL including the subsets OPALite and Pyrite, whereas the latter
   is just the above lock/unlock in a TCG way.

 - TCG Enterprise SSC

I think it's pretty obvious that ATA security should be something on
it's own.  OPALite and Pyrite are strict subsets of OPAL, so having them
in something named opal shouldn't be surprising.  The big question
is what to do about Enterprise SSC.  Which has some overlaps with OPAL
but also major differences, so keeping it separate if we ever have to
implement it (I hope we don't) would be best.

> I'm on board with this if you think we won't have enough different, but
> similar, SED protocols to justify the indirection. In that case you can
> ignore the above comment as well.

This goes back to the above.  The only thing that is not a strict subset
of OPAL but kinda sorta similar is TCG Enterprise SSC, but my preference
would be to ignore it, and the second best preference would be to keep
it separate.

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

On Tue, Dec 20, 2016 at 02:55:10PM -0700, Scott Bauer wrote:
> Do you mean from nvme_ioctl? or from blk_ioctl? It was removed from  blk_ioctl
> because I must have misinterpreted your comments here:
> http://lists.infradead.org/pipermail/linux-nvme/2016-December/007364.html

nvme_ioctl, aka the ioctl method of the block driver.  In the future
I expect another caller in scsi_ioctl or sd_ioctl.

> I took your hesitation about the block_device to mean try something new,
> that combined with my concern about having namespaces have the ability to
> lock the global range which can span ourside their LBA ranges lead me to
> remove it from block and put it in the char dev world.

In the link I literally meant the struct block_device, not nessecarily
the block device node.

> 
> On the same note there is a public review of
> TCG Storage Opal SSC Feature Set: Configurable Namespace Locking:
> Which Jon Derrick found for us:
> https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Feature_Set_Namespaces_phase_1b_v1_00_r1_19_public-review.pdf
> 
> Where they are thinking about doing a complete 180 from:
> https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf
> 
> And now Namespaces can have their own global locking range as well as locking
> objects within them. 

Interesting.  From a quick look this is how it should have been done
from the start.  Now the big question is when devices are going to
implement it, and how we can support both from the same driver, as this
means we'll have to deal with two different ways to allocate the
security context based on the device.  And do the horrible mess that the
TCG SIISs are there is absolute no way to discover this at the NVMe
level, but we actually need to start doing TCG method calls to even
figure out what's going on.

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

On Tue, Dec 20, 2016 at 02:55:10PM -0700, Scott Bauer wrote:
> Do you mean from nvme_ioctl? or from blk_ioctl? It was removed from  blk_ioctl
> because I must have misinterpreted your comments here:
> http://lists.infradead.org/pipermail/linux-nvme/2016-December/007364.html

nvme_ioctl, aka the ioctl method of the block driver.  In the future
I expect another caller in scsi_ioctl or sd_ioctl.

> I took your hesitation about the block_device to mean try something new,
> that combined with my concern about having namespaces have the ability to
> lock the global range which can span ourside their LBA ranges lead me to
> remove it from block and put it in the char dev world.

In the link I literally meant the struct block_device, not nessecarily
the block device node.

> 
> On the same note there is a public review of
> TCG Storage Opal SSC Feature Set: Configurable Namespace Locking:
> Which Jon Derrick found for us:
> https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Feature_Set_Namespaces_phase_1b_v1_00_r1_19_public-review.pdf
> 
> Where they are thinking about doing a complete 180 from:
> https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf
> 
> And now Namespaces can have their own global locking range as well as locking
> objects within them. 

Interesting.  From a quick look this is how it should have been done
from the start.  Now the big question is when devices are going to
implement it, and how we can support both from the same driver, as this
means we'll have to deal with two different ways to allocate the
security context based on the device.  And do the horrible mess that the
TCG SIISs are there is absolute no way to discover this at the NVMe
level, but we actually need to start doing TCG method calls to even
figure out what's going on.

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Jon Derrick]

On Mon, Dec 19, 2016 at 11:28:47PM -0800, Christoph Hellwig wrote:
[snip]
> > +   while (cpos < total) {
> > +   if (!(pos[0] & 0x80)) /* tiny atom */
> > +   token_length = response_parse_tiny(iter, pos);
> > +   else if (!(pos[0] & 0x40)) /* short atom */
> > +   token_length = response_parse_short(iter, pos);
> > +   else if (!(pos[0] & 0x20)) /* medium atom */
> > +   token_length = response_parse_medium(iter, pos);
> > +   else if (!(pos[0] & 0x10)) /* long atom */
> > +   token_length = response_parse_long(iter, pos);
> 
> Please add symbolic names for these constants to the protocol header.
> 
I get tripped up by this logic every time I look at it. I'd almost
rather see something like the following, which more closely follows the
TCG core spec and the optimizing compiler should turn it into something
similar to above anyways:

if (pos[0] <= 0x7F)
token_length = response_parse_tiny..
else if (pos[0] <= 0xBF)
token_length = response_parse_short..
else if (pos[0] <= 0xDF)
token_length = response_parse_medium..
else if (pos[0] <= 0xE3)
token_length = response_parse_long..

Then just add those 0x7F, 0xBF, 0xDF, and 0xE3 constants to proto
header. We could even add in a TCG reserved error for 0xE4-0xEF

Also instead of tracking cpos, we could subtract from total until
negative (if you make it signed). It's similar to how we do length in
nvme_setup_prps.

[snip]
> > --- /dev/null
> > +++ b/lib/sed-opal_internal.h
> 
> This pretty much seem to contain the OPAL protocol defintions, so why
> not opal_proto.h?
Since there might eventually be a whole class of opal-like sed
protocols, why does it make more sense to have opal_proto.h instead of
sed-opal.h or some variation? This is similar to how leds-*.h look to
me. Although I agree that sed-ATA.h would be dishonest since ATA
security doesn't imply a self-encrypting-disk.

[snip]
> > +int sed_save(struct sed_context *sed_ctx, struct sed_key *key)
> > +{
> > +   switch (key->sed_type) {
> > +   case OPAL_LOCK_UNLOCK:
> > +   return opal_save(sed_ctx, key);
> > +   }
> > +
> > +   return -EOPNOTSUPP;
> 
> It seems to me that we should skip this whole sed_type indirections
> and just specify the ioctls directly for OPAL (which would include
> opalite and pyrite as subsets).  The only other protocols of interest
> for Linux would be the ATA "security" plain text passwords, which can
> be handled differently, or enterprise SSC which we can hopefully avoid
> to implement entirely.
I'm on board with this if you think we won't have enough different, but
similar, SED protocols to justify the indirection. In that case you can
ignore the above comment as well.

> 
> > +
> > +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> > +  unsigned long arg)
> > +{
> > +   struct sed_key key;
> > +   struct sed_context *sed_ctx;
> > +
> > +   if (!capable(CAP_SYS_ADMIN))
> > +   return -EACCES;
> > +
> > +   if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> > +   return -ENODEV;
> 
> In the previous version this was called from the block driver which
> could pass in the context (and ops).  Why was this changed?
> 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Jon Derrick]

On Mon, Dec 19, 2016 at 11:28:47PM -0800, Christoph Hellwig wrote:
[snip]
> > +   while (cpos < total) {
> > +   if (!(pos[0] & 0x80)) /* tiny atom */
> > +   token_length = response_parse_tiny(iter, pos);
> > +   else if (!(pos[0] & 0x40)) /* short atom */
> > +   token_length = response_parse_short(iter, pos);
> > +   else if (!(pos[0] & 0x20)) /* medium atom */
> > +   token_length = response_parse_medium(iter, pos);
> > +   else if (!(pos[0] & 0x10)) /* long atom */
> > +   token_length = response_parse_long(iter, pos);
> 
> Please add symbolic names for these constants to the protocol header.
> 
I get tripped up by this logic every time I look at it. I'd almost
rather see something like the following, which more closely follows the
TCG core spec and the optimizing compiler should turn it into something
similar to above anyways:

if (pos[0] <= 0x7F)
token_length = response_parse_tiny..
else if (pos[0] <= 0xBF)
token_length = response_parse_short..
else if (pos[0] <= 0xDF)
token_length = response_parse_medium..
else if (pos[0] <= 0xE3)
token_length = response_parse_long..

Then just add those 0x7F, 0xBF, 0xDF, and 0xE3 constants to proto
header. We could even add in a TCG reserved error for 0xE4-0xEF

Also instead of tracking cpos, we could subtract from total until
negative (if you make it signed). It's similar to how we do length in
nvme_setup_prps.

[snip]
> > --- /dev/null
> > +++ b/lib/sed-opal_internal.h
> 
> This pretty much seem to contain the OPAL protocol defintions, so why
> not opal_proto.h?
Since there might eventually be a whole class of opal-like sed
protocols, why does it make more sense to have opal_proto.h instead of
sed-opal.h or some variation? This is similar to how leds-*.h look to
me. Although I agree that sed-ATA.h would be dishonest since ATA
security doesn't imply a self-encrypting-disk.

[snip]
> > +int sed_save(struct sed_context *sed_ctx, struct sed_key *key)
> > +{
> > +   switch (key->sed_type) {
> > +   case OPAL_LOCK_UNLOCK:
> > +   return opal_save(sed_ctx, key);
> > +   }
> > +
> > +   return -EOPNOTSUPP;
> 
> It seems to me that we should skip this whole sed_type indirections
> and just specify the ioctls directly for OPAL (which would include
> opalite and pyrite as subsets).  The only other protocols of interest
> for Linux would be the ATA "security" plain text passwords, which can
> be handled differently, or enterprise SSC which we can hopefully avoid
> to implement entirely.
I'm on board with this if you think we won't have enough different, but
similar, SED protocols to justify the indirection. In that case you can
ignore the above comment as well.

> 
> > +
> > +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> > +  unsigned long arg)
> > +{
> > +   struct sed_key key;
> > +   struct sed_context *sed_ctx;
> > +
> > +   if (!capable(CAP_SYS_ADMIN))
> > +   return -EACCES;
> > +
> > +   if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> > +   return -ENODEV;
> 
> In the previous version this was called from the block driver which
> could pass in the context (and ops).  Why was this changed?
> 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Scott Bauer]

> > +
> > +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> > +  unsigned long arg)
> > +{
> > +   struct sed_key key;
> > +   struct sed_context *sed_ctx;
> > +
> > +   if (!capable(CAP_SYS_ADMIN))
> > +   return -EACCES;
> > +
> > +   if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> > +   return -ENODEV;
> 
> In the previous version this was called from the block driver which
> could pass in the context (and ops).  Why was this changed?
> 

Do you mean from nvme_ioctl? or from blk_ioctl? It was removed from  blk_ioctl
because I must have misinterpreted your comments here:
http://lists.infradead.org/pipermail/linux-nvme/2016-December/007364.html

Copied from the link: 
>
> > and directly use
> > block_device. Then if we add the security send/receive operations to the
> > block_device_operations, that will simplify chaining the security request
> > to the driver without needing to thread the driver's requested callback
> > and data the way you have to here since all the necessary information
> > is encapsulated in the block_device.

> Maybe.  I need to look at the TCG spec again (oh my good, what a fucking
> mess), but if I remember the context if it is the whole nvme controller
> and not just a namespace, so a block_device might be the wrong context.
> Then again we can always go from the block_device to the controller
> fairly easily.  So instead of adding the security operation to the
> block_device_operations which we don't really need for now maybe we
> should add a security_conext to the block device so that we can avoid
> all the lookup code?

I took your hesitation about the block_device to mean try something new,
that combined with my concern about having namespaces have the ability to
lock the global range which can span ourside their LBA ranges lead me to
remove it from block and put it in the char dev world.

On the same note there is a public review of
TCG Storage Opal SSC Feature Set: Configurable Namespace Locking:
Which Jon Derrick found for us:
https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Feature_Set_Namespaces_phase_1b_v1_00_r1_19_public-review.pdf

Where they are thinking about doing a complete 180 from:
https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf

And now Namespaces can have their own global locking range as well as locking
objects within them. 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Scott Bauer]

> > +
> > +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> > +  unsigned long arg)
> > +{
> > +   struct sed_key key;
> > +   struct sed_context *sed_ctx;
> > +
> > +   if (!capable(CAP_SYS_ADMIN))
> > +   return -EACCES;
> > +
> > +   if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> > +   return -ENODEV;
> 
> In the previous version this was called from the block driver which
> could pass in the context (and ops).  Why was this changed?
> 

Do you mean from nvme_ioctl? or from blk_ioctl? It was removed from  blk_ioctl
because I must have misinterpreted your comments here:
http://lists.infradead.org/pipermail/linux-nvme/2016-December/007364.html

Copied from the link: 
>
> > and directly use
> > block_device. Then if we add the security send/receive operations to the
> > block_device_operations, that will simplify chaining the security request
> > to the driver without needing to thread the driver's requested callback
> > and data the way you have to here since all the necessary information
> > is encapsulated in the block_device.

> Maybe.  I need to look at the TCG spec again (oh my good, what a fucking
> mess), but if I remember the context if it is the whole nvme controller
> and not just a namespace, so a block_device might be the wrong context.
> Then again we can always go from the block_device to the controller
> fairly easily.  So instead of adding the security operation to the
> block_device_operations which we don't really need for now maybe we
> should add a security_conext to the block device so that we can avoid
> all the lookup code?

I took your hesitation about the block_device to mean try something new,
that combined with my concern about having namespaces have the ability to
lock the global range which can span ourside their LBA ranges lead me to
remove it from block and put it in the char dev world.

On the same note there is a public review of
TCG Storage Opal SSC Feature Set: Configurable Namespace Locking:
Which Jon Derrick found for us:
https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_Storage_Feature_Set_Namespaces_phase_1b_v1_00_r1_19_public-review.pdf

Where they are thinking about doing a complete 180 from:
https://www.trustedcomputinggroup.org/wp-content/uploads/TCG_SWG_SIIS_Version_1_05_Revision_1_00.pdf

And now Namespaces can have their own global locking range as well as locking
objects within them. 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

> + u8 lr;
> + size_t key_name_len;
> + char key_name[36];

Who is going to use the key_name?  I can't find another reference to
it anywhere else in the code.   The reason why this tripped me off was
the hardcoded length so I was going to check on how access to it is
bounds checked.

> +/**
> + * struct opal_dev - The structure representing a OPAL enabled SED.
> + * @sed_ctx:The SED context, contains fn pointers to sec_send/recv.
> + * @opal_step:A series of opal methods that are necessary to complete a 
> comannd.
> + * @func_data:An array of parameters for the opal methods above.
> + * @state:Describes the current opal_step we're working on.
> + * @dev_lock:Locks the entire opal_dev structure.
> + * @parsed:Parsed response from controller.
> + * @prev_data:Data returned from a method to the controller
> + * @error_cb:Error function that handles closing sessions after a failed 
> method.
> + * @unlk_lst:A list of Locking ranges to unlock on this device during a 
> resume.
> + */

Needs spaces after the colons.

> + u16 comID;
> + u32 HSN;
> + u32 TSN;

Please use lower case variable names in Linux code, and separate
words by underscores if needed.

> +DEFINE_SPINLOCK(list_spinlock);

Should be marked static.

> +#define TPER_SYNC_SUPPORTED BIT(0)

This sounds like a protocol constant and should go into the header
with the rest of the protocol.

> +static bool check_tper(const void *data)
> +{
> + const struct d0_tper_features *tper = data;
> + u8 flags = tper->supported_features;
> +
> + if (!(flags & TPER_SYNC_SUPPORTED)) {
> + pr_err("TPer sync not supported. flags = %d\n",
> +tper->supported_features);

It would be great to use dev_err / dev_warn / etc in the opal code,
although for that you'll need to pass a struct device from the driver
into the code.

> +static bool check_SUM(const void *data)

The comment on member naming above also applies to variables and function
names.

> + bool foundComID = false, supported = true, single_user = false;
> + const struct d0_header *hdr;
> + const u8 *epos, *cpos;
> + u16 comID = 0;
> + int error = 0;
> +
> + epos = dev->cmd.resp;
> + cpos = dev->cmd.resp;
> + hdr = (struct d0_header *)dev->cmd.resp;

You probably want to structure your command buffers to use void pointers
and avoid ths kind of casting.

> +#define TINY_ATOM_DATA_MASK GENMASK(5, 0)
> +#define TINY_ATOM_SIGNED BIT(6)
> +
> +#define SHORT_ATOM_ID BIT(7)
> +#define SHORT_ATOM_BYTESTRING BIT(5)
> +#define SHORT_ATOM_SIGNED BIT(4)
> +#define SHORT_ATOM_LEN_MASK GENMASK(3, 0)

Protocol constants for the header again?

> +#define ADD_TOKEN_STRING(cmd, key, keylen)   \
> + if (!err)   \
> + err = test_and_add_string(cmd, key, keylen);
> +
> +#define ADD_TOKEN(type, cmd, tok)\
> + if (!err)   \
> + err = test_and_add_token_##type(cmd, tok);

Please remove these macros and just open code the calls.  If you want
to avoid writing the if err lines again and again just pass err
by reference to the functions and move the err check into the add_token*
helpers.

> + if ((hdr->cp.length == 0)
> + || (hdr->pkt.length == 0)
> + || (hdr->subpkt.length == 0)) {

No need for the inner braces, also please place the operators at the
end of the previous line instead of the beginning of the new line.

> + while (cpos < total) {
> + if (!(pos[0] & 0x80)) /* tiny atom */
> + token_length = response_parse_tiny(iter, pos);
> + else if (!(pos[0] & 0x40)) /* short atom */
> + token_length = response_parse_short(iter, pos);
> + else if (!(pos[0] & 0x20)) /* medium atom */
> + token_length = response_parse_medium(iter, pos);
> + else if (!(pos[0] & 0x10)) /* long atom */
> + token_length = response_parse_long(iter, pos);

Please add symbolic names for these constants to the protocol header.

> + if (num_entries == 0) {
> + pr_err("Couldn't parse response.\n");
> + return -EINVAL;
> + resp->num = num_entries;

Where is the closing brace for that if?

> + if (!((resp->toks[n].width == OPAL_WIDTH_TINY) ||
> +   (resp->toks[n].width == OPAL_WIDTH_SHORT))) {

No need for the inner braces.

> + pr_err("Atom is not short or tiny: %d\n",
> +resp->toks[n].width);
> + return 0;
> + }
> +
> + return resp->toks[n].stored.u;
> +}
> +
> +static u8 response_status(const struct parsed_resp *resp)
> +{
> + if ((token_type(resp, 0) == OPAL_DTA_TOKENID_TOKEN)
> + && (response_get_token(resp, 0) == OPAL_ENDOFSESSION)) {

Same here, also please fix the operator placement.

> + if ((token_type(resp, resp->num - 1) != 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

> + u8 lr;
> + size_t key_name_len;
> + char key_name[36];

Who is going to use the key_name?  I can't find another reference to
it anywhere else in the code.   The reason why this tripped me off was
the hardcoded length so I was going to check on how access to it is
bounds checked.

> +/**
> + * struct opal_dev - The structure representing a OPAL enabled SED.
> + * @sed_ctx:The SED context, contains fn pointers to sec_send/recv.
> + * @opal_step:A series of opal methods that are necessary to complete a 
> comannd.
> + * @func_data:An array of parameters for the opal methods above.
> + * @state:Describes the current opal_step we're working on.
> + * @dev_lock:Locks the entire opal_dev structure.
> + * @parsed:Parsed response from controller.
> + * @prev_data:Data returned from a method to the controller
> + * @error_cb:Error function that handles closing sessions after a failed 
> method.
> + * @unlk_lst:A list of Locking ranges to unlock on this device during a 
> resume.
> + */

Needs spaces after the colons.

> + u16 comID;
> + u32 HSN;
> + u32 TSN;

Please use lower case variable names in Linux code, and separate
words by underscores if needed.

> +DEFINE_SPINLOCK(list_spinlock);

Should be marked static.

> +#define TPER_SYNC_SUPPORTED BIT(0)

This sounds like a protocol constant and should go into the header
with the rest of the protocol.

> +static bool check_tper(const void *data)
> +{
> + const struct d0_tper_features *tper = data;
> + u8 flags = tper->supported_features;
> +
> + if (!(flags & TPER_SYNC_SUPPORTED)) {
> + pr_err("TPer sync not supported. flags = %d\n",
> +tper->supported_features);

It would be great to use dev_err / dev_warn / etc in the opal code,
although for that you'll need to pass a struct device from the driver
into the code.

> +static bool check_SUM(const void *data)

The comment on member naming above also applies to variables and function
names.

> + bool foundComID = false, supported = true, single_user = false;
> + const struct d0_header *hdr;
> + const u8 *epos, *cpos;
> + u16 comID = 0;
> + int error = 0;
> +
> + epos = dev->cmd.resp;
> + cpos = dev->cmd.resp;
> + hdr = (struct d0_header *)dev->cmd.resp;

You probably want to structure your command buffers to use void pointers
and avoid ths kind of casting.

> +#define TINY_ATOM_DATA_MASK GENMASK(5, 0)
> +#define TINY_ATOM_SIGNED BIT(6)
> +
> +#define SHORT_ATOM_ID BIT(7)
> +#define SHORT_ATOM_BYTESTRING BIT(5)
> +#define SHORT_ATOM_SIGNED BIT(4)
> +#define SHORT_ATOM_LEN_MASK GENMASK(3, 0)

Protocol constants for the header again?

> +#define ADD_TOKEN_STRING(cmd, key, keylen)   \
> + if (!err)   \
> + err = test_and_add_string(cmd, key, keylen);
> +
> +#define ADD_TOKEN(type, cmd, tok)\
> + if (!err)   \
> + err = test_and_add_token_##type(cmd, tok);

Please remove these macros and just open code the calls.  If you want
to avoid writing the if err lines again and again just pass err
by reference to the functions and move the err check into the add_token*
helpers.

> + if ((hdr->cp.length == 0)
> + || (hdr->pkt.length == 0)
> + || (hdr->subpkt.length == 0)) {

No need for the inner braces, also please place the operators at the
end of the previous line instead of the beginning of the new line.

> + while (cpos < total) {
> + if (!(pos[0] & 0x80)) /* tiny atom */
> + token_length = response_parse_tiny(iter, pos);
> + else if (!(pos[0] & 0x40)) /* short atom */
> + token_length = response_parse_short(iter, pos);
> + else if (!(pos[0] & 0x20)) /* medium atom */
> + token_length = response_parse_medium(iter, pos);
> + else if (!(pos[0] & 0x10)) /* long atom */
> + token_length = response_parse_long(iter, pos);

Please add symbolic names for these constants to the protocol header.

> + if (num_entries == 0) {
> + pr_err("Couldn't parse response.\n");
> + return -EINVAL;
> + resp->num = num_entries;

Where is the closing brace for that if?

> + if (!((resp->toks[n].width == OPAL_WIDTH_TINY) ||
> +   (resp->toks[n].width == OPAL_WIDTH_SHORT))) {

No need for the inner braces.

> + pr_err("Atom is not short or tiny: %d\n",
> +resp->toks[n].width);
> + return 0;
> + }
> +
> + return resp->toks[n].stored.u;
> +}
> +
> +static u8 response_status(const struct parsed_resp *resp)
> +{
> + if ((token_type(resp, 0) == OPAL_DTA_TOKENID_TOKEN)
> + && (response_get_token(resp, 0) == OPAL_ENDOFSESSION)) {

Same here, also please fix the operator placement.

> + if ((token_type(resp, resp->num - 1) != 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Al Viro]

On Mon, Dec 19, 2016 at 12:35:46PM -0700, Scott Bauer wrote:

> +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> +unsigned long arg)
> +{
> + struct sed_key key;
> + struct sed_context *sed_ctx;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> + if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> + return -ENODEV;
> +
> + sed_ctx = filep->f_sedctx;

First of all, that's a bisect hazard.  What's more, looking through the
rest of patchset, WTF does it
* need to be called that early in ioctl(2) handling, instead of
having ->ioctl() instance for that sucker calling it?
* _not_ get your ->f_sedctx as an explicit argument, passed by
the caller in ->ioctl(), seeing that it's possible to calculate by
file->private_data?
* store that thing in struct file itself, bloating it for everything
all for the sake of few drivers that might want to use that helper?

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Al Viro]

On Mon, Dec 19, 2016 at 12:35:46PM -0700, Scott Bauer wrote:

> +int fdev_sed_ioctl(struct file *filep, unsigned int cmd,
> +unsigned long arg)
> +{
> + struct sed_key key;
> + struct sed_context *sed_ctx;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
> +
> + if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
> + return -ENODEV;
> +
> + sed_ctx = filep->f_sedctx;

First of all, that's a bisect hazard.  What's more, looking through the
rest of patchset, WTF does it
* need to be called that early in ioctl(2) handling, instead of
having ->ioctl() instance for that sucker calling it?
* _not_ get your ->f_sedctx as an explicit argument, passed by
the caller in ->ioctl(), seeing that it's possible to calculate by
file->private_data?
* store that thing in struct file itself, bloating it for everything
all for the sake of few drivers that might want to use that helper?

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

On Mon, Dec 19, 2016 at 04:34:15PM -0500, Keith Busch wrote:
> This seems like an optional library that some environments may wish to
> opt-out of building into the kernel. Any reason not to add an entry into
> the Kconfig to turn this on/off?

This needs to be a CONFIG_BLOCK_SED / CONFIG_BLOCK_SED_OPAL and
should move to block/.

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Christoph Hellwig]

On Mon, Dec 19, 2016 at 04:34:15PM -0500, Keith Busch wrote:
> This seems like an optional library that some environments may wish to
> opt-out of building into the kernel. Any reason not to add an entry into
> the Kconfig to turn this on/off?

This needs to be a CONFIG_BLOCK_SED / CONFIG_BLOCK_SED_OPAL and
should move to block/.

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[kbuild test robot]

Hi Scott,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.9 next-20161219]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Scott-Bauer/include-Add-definitions-for-sed/20161220-110214
config: i386-randconfig-r0-201651 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from lib/sed.c:20:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
>> include/linux/sed-opal.h:37:40: warning: its scope is only this definition 
>> or declaration, which is probably not what you want
   lib/sed.c: In function 'fdev_sed_ioctl':
   lib/sed.c:161:12: error: dereferencing pointer to incomplete type 'struct 
file'
 if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
   ^
--
   In file included from lib/sed-opal.c:29:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
>> include/linux/sed-opal.h:37:40: warning: its scope is only this definition 
>> or declaration, which is probably not what you want
   lib/sed-opal.c: In function 'opal_discovery0_end':
   lib/sed-opal.c:276:6: warning: unused variable 'error' [-Wunused-variable]
 int error = 0;
 ^
   lib/sed-opal.c: In function 'response_parse':
   lib/sed-opal.c:793:15: error: invalid storage class for function 
'response_get_string'
static size_t response_get_string(const struct parsed_resp *resp, int n,
  ^
   lib/sed-opal.c:793:1: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]
static size_t response_get_string(const struct parsed_resp *resp, int n,
^
   lib/sed-opal.c:817:12: error: invalid storage class for function 
'response_get_u64'
static u64 response_get_u64(const struct parsed_resp *resp, int n)
   ^
   lib/sed-opal.c:846:11: error: invalid storage class for function 
'response_status'
static u8 response_status(const struct parsed_resp *resp)
  ^
   lib/sed-opal.c:866:12: error: invalid storage class for function 
'parse_and_check_status'
static int parse_and_check_status(struct opal_dev *dev)
   ^
   lib/sed-opal.c:883:13: error: invalid storage class for function 
'clear_opal_cmd'
static void clear_opal_cmd(struct opal_cmd *cmd)
^
   lib/sed-opal.c:891:12: error: invalid storage class for function 
'start_opal_session_cont'
static int start_opal_session_cont(struct opal_dev *dev)
   ^
   lib/sed-opal.c:913:20: error: invalid storage class for function 
'opal_dev_get'
static inline void opal_dev_get(struct opal_dev *dev)
   ^
   lib/sed-opal.c:918:20: error: invalid storage class for function 
'opal_dev_put'
static inline void opal_dev_put(struct opal_dev *dev)
   ^
   lib/sed-opal.c:923:12: error: invalid storage class for function 
'add_suspend_info'
static int add_suspend_info(struct opal_dev *dev, struct opal_suspend_data 
*sus)
   ^
   lib/sed-opal.c:949:12: error: invalid storage class for function 
'end_session_cont'
static int end_session_cont(struct opal_dev *dev)
   ^
   lib/sed-opal.c:956:12: error: invalid storage class for function 
'finalize_and_send'
static int finalize_and_send(struct opal_dev *dev, struct opal_cmd *cmd,
   ^
   lib/sed-opal.c:972:12: error: invalid storage class for function 'gen_key'
static int gen_key(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1002:12: error: invalid storage class for function 
'get_active_key_cont'
static int get_active_key_cont(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1027:12: error: invalid storage class for function 
'get_active_key'
static int get_active_key(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1067:12: error: invalid storage class for function 
'generic_lr_enable_disable'
static int generic_lr_enable_disable(struct opal_cmd *cmd,
   ^
   lib/sed-opal.c:1107:19: error: invalid storage class for function 
'enable_global_lr'
static inline int enable_global_lr(struct opal_cmd *cmd, u8 *uid,
  ^
   lib/sed-opal.c:1118:12: error: invalid storage class for function 
'setup_locking_range'
static int setup_locking_range(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1183:12: error: invalid storage class for function 
'start_generic_opal_session'
static int start_generic_opal_session(struct opal_dev 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[kbuild test robot]

Hi Scott,

[auto build test WARNING on linus/master]
[also build test WARNING on v4.9 next-20161219]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Scott-Bauer/include-Add-definitions-for-sed/20161220-110214
config: i386-randconfig-r0-201651 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from lib/sed.c:20:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
>> include/linux/sed-opal.h:37:40: warning: its scope is only this definition 
>> or declaration, which is probably not what you want
   lib/sed.c: In function 'fdev_sed_ioctl':
   lib/sed.c:161:12: error: dereferencing pointer to incomplete type 'struct 
file'
 if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
   ^
--
   In file included from lib/sed-opal.c:29:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
>> include/linux/sed-opal.h:37:40: warning: its scope is only this definition 
>> or declaration, which is probably not what you want
   lib/sed-opal.c: In function 'opal_discovery0_end':
   lib/sed-opal.c:276:6: warning: unused variable 'error' [-Wunused-variable]
 int error = 0;
 ^
   lib/sed-opal.c: In function 'response_parse':
   lib/sed-opal.c:793:15: error: invalid storage class for function 
'response_get_string'
static size_t response_get_string(const struct parsed_resp *resp, int n,
  ^
   lib/sed-opal.c:793:1: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]
static size_t response_get_string(const struct parsed_resp *resp, int n,
^
   lib/sed-opal.c:817:12: error: invalid storage class for function 
'response_get_u64'
static u64 response_get_u64(const struct parsed_resp *resp, int n)
   ^
   lib/sed-opal.c:846:11: error: invalid storage class for function 
'response_status'
static u8 response_status(const struct parsed_resp *resp)
  ^
   lib/sed-opal.c:866:12: error: invalid storage class for function 
'parse_and_check_status'
static int parse_and_check_status(struct opal_dev *dev)
   ^
   lib/sed-opal.c:883:13: error: invalid storage class for function 
'clear_opal_cmd'
static void clear_opal_cmd(struct opal_cmd *cmd)
^
   lib/sed-opal.c:891:12: error: invalid storage class for function 
'start_opal_session_cont'
static int start_opal_session_cont(struct opal_dev *dev)
   ^
   lib/sed-opal.c:913:20: error: invalid storage class for function 
'opal_dev_get'
static inline void opal_dev_get(struct opal_dev *dev)
   ^
   lib/sed-opal.c:918:20: error: invalid storage class for function 
'opal_dev_put'
static inline void opal_dev_put(struct opal_dev *dev)
   ^
   lib/sed-opal.c:923:12: error: invalid storage class for function 
'add_suspend_info'
static int add_suspend_info(struct opal_dev *dev, struct opal_suspend_data 
*sus)
   ^
   lib/sed-opal.c:949:12: error: invalid storage class for function 
'end_session_cont'
static int end_session_cont(struct opal_dev *dev)
   ^
   lib/sed-opal.c:956:12: error: invalid storage class for function 
'finalize_and_send'
static int finalize_and_send(struct opal_dev *dev, struct opal_cmd *cmd,
   ^
   lib/sed-opal.c:972:12: error: invalid storage class for function 'gen_key'
static int gen_key(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1002:12: error: invalid storage class for function 
'get_active_key_cont'
static int get_active_key_cont(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1027:12: error: invalid storage class for function 
'get_active_key'
static int get_active_key(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1067:12: error: invalid storage class for function 
'generic_lr_enable_disable'
static int generic_lr_enable_disable(struct opal_cmd *cmd,
   ^
   lib/sed-opal.c:1107:19: error: invalid storage class for function 
'enable_global_lr'
static inline int enable_global_lr(struct opal_cmd *cmd, u8 *uid,
  ^
   lib/sed-opal.c:1118:12: error: invalid storage class for function 
'setup_locking_range'
static int setup_locking_range(struct opal_dev *dev)
   ^
   lib/sed-opal.c:1183:12: error: invalid storage class for function 
'start_generic_opal_session'
static int start_generic_opal_session(struct opal_dev 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[kbuild test robot]

Hi Scott,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9 next-20161219]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Scott-Bauer/include-Add-definitions-for-sed/20161220-110214
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from lib/sed.c:20:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list will not be visible outside of this definition or 
>> declaration
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
   lib/sed.c: In function 'fdev_sed_ioctl':
>> lib/sed.c:161:12: error: dereferencing pointer to incomplete type 'struct 
>> file'
 if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
   ^~
--
   In file included from lib/sed-opal.c:29:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list will not be visible outside of this definition or 
>> declaration
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
   lib/sed-opal.c: In function 'opal_discovery0_end':
   lib/sed-opal.c:276:6: warning: unused variable 'error' [-Wunused-variable]
 int error = 0;
 ^
   lib/sed-opal.c: In function 'response_parse':
>> lib/sed-opal.c:793:15: error: invalid storage class for function 
>> 'response_get_string'
static size_t response_get_string(const struct parsed_resp *resp, int n,
  ^~~
>> lib/sed-opal.c:793:1: warning: ISO C90 forbids mixed declarations and code 
>> [-Wdeclaration-after-statement]
static size_t response_get_string(const struct parsed_resp *resp, int n,
^~
>> lib/sed-opal.c:817:12: error: invalid storage class for function 
>> 'response_get_u64'
static u64 response_get_u64(const struct parsed_resp *resp, int n)
   ^~~~
>> lib/sed-opal.c:846:11: error: invalid storage class for function 
>> 'response_status'
static u8 response_status(const struct parsed_resp *resp)
  ^~~
>> lib/sed-opal.c:866:12: error: invalid storage class for function 
>> 'parse_and_check_status'
static int parse_and_check_status(struct opal_dev *dev)
   ^~
>> lib/sed-opal.c:883:13: error: invalid storage class for function 
>> 'clear_opal_cmd'
static void clear_opal_cmd(struct opal_cmd *cmd)
^~
>> lib/sed-opal.c:891:12: error: invalid storage class for function 
>> 'start_opal_session_cont'
static int start_opal_session_cont(struct opal_dev *dev)
   ^~~
>> lib/sed-opal.c:913:20: error: invalid storage class for function 
>> 'opal_dev_get'
static inline void opal_dev_get(struct opal_dev *dev)
   ^~~~
>> lib/sed-opal.c:918:20: error: invalid storage class for function 
>> 'opal_dev_put'
static inline void opal_dev_put(struct opal_dev *dev)
   ^~~~
>> lib/sed-opal.c:923:12: error: invalid storage class for function 
>> 'add_suspend_info'
static int add_suspend_info(struct opal_dev *dev, struct opal_suspend_data 
*sus)
   ^~~~
>> lib/sed-opal.c:949:12: error: invalid storage class for function 
>> 'end_session_cont'
static int end_session_cont(struct opal_dev *dev)
   ^~~~
>> lib/sed-opal.c:956:12: error: invalid storage class for function 
>> 'finalize_and_send'
static int finalize_and_send(struct opal_dev *dev, struct opal_cmd *cmd,
   ^
>> lib/sed-opal.c:972:12: error: invalid storage class for function 'gen_key'
static int gen_key(struct opal_dev *dev)
   ^~~
>> lib/sed-opal.c:1002:12: error: invalid storage class for function 
>> 'get_active_key_cont'
static int get_active_key_cont(struct opal_dev *dev)
   ^~~
>> lib/sed-opal.c:1027:12: error: invalid storage class for function 
>> 'get_active_key'
static int get_active_key(struct opal_dev *dev)
   ^~
>> lib/sed-opal.c:1067:12: error: invalid storage class for function 
>> 'generic_lr_enable_disable'
static int generic_lr_enable_disable(struct opal_cmd *cmd,
   ^
>> lib/sed-opal.c:1107:19: error: invalid storage class for function 
>> 'enable_global_lr'
static inline int enable_global_lr(struct opal_cmd *cmd, u8 *uid,
  ^~~~
>> lib/sed-opal.c:1118:12: error: invalid storage class for function 
>> 'setup_locking_range'
static int setup_locking_range(struct 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[kbuild test robot]

Hi Scott,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.9 next-20161219]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Scott-Bauer/include-Add-definitions-for-sed/20161220-110214
config: i386-tinyconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from lib/sed.c:20:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list will not be visible outside of this definition or 
>> declaration
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
   lib/sed.c: In function 'fdev_sed_ioctl':
>> lib/sed.c:161:12: error: dereferencing pointer to incomplete type 'struct 
>> file'
 if (!filep->f_sedctx || !filep->f_sedctx->ops || !filep->f_sedctx->dev)
   ^~
--
   In file included from lib/sed-opal.c:29:0:
>> include/linux/sed-opal.h:37:40: warning: 'struct request_queue' declared 
>> inside parameter list will not be visible outside of this definition or 
>> declaration
struct opal_dev *alloc_opal_dev(struct request_queue *q);
   ^
   lib/sed-opal.c: In function 'opal_discovery0_end':
   lib/sed-opal.c:276:6: warning: unused variable 'error' [-Wunused-variable]
 int error = 0;
 ^
   lib/sed-opal.c: In function 'response_parse':
>> lib/sed-opal.c:793:15: error: invalid storage class for function 
>> 'response_get_string'
static size_t response_get_string(const struct parsed_resp *resp, int n,
  ^~~
>> lib/sed-opal.c:793:1: warning: ISO C90 forbids mixed declarations and code 
>> [-Wdeclaration-after-statement]
static size_t response_get_string(const struct parsed_resp *resp, int n,
^~
>> lib/sed-opal.c:817:12: error: invalid storage class for function 
>> 'response_get_u64'
static u64 response_get_u64(const struct parsed_resp *resp, int n)
   ^~~~
>> lib/sed-opal.c:846:11: error: invalid storage class for function 
>> 'response_status'
static u8 response_status(const struct parsed_resp *resp)
  ^~~
>> lib/sed-opal.c:866:12: error: invalid storage class for function 
>> 'parse_and_check_status'
static int parse_and_check_status(struct opal_dev *dev)
   ^~
>> lib/sed-opal.c:883:13: error: invalid storage class for function 
>> 'clear_opal_cmd'
static void clear_opal_cmd(struct opal_cmd *cmd)
^~
>> lib/sed-opal.c:891:12: error: invalid storage class for function 
>> 'start_opal_session_cont'
static int start_opal_session_cont(struct opal_dev *dev)
   ^~~
>> lib/sed-opal.c:913:20: error: invalid storage class for function 
>> 'opal_dev_get'
static inline void opal_dev_get(struct opal_dev *dev)
   ^~~~
>> lib/sed-opal.c:918:20: error: invalid storage class for function 
>> 'opal_dev_put'
static inline void opal_dev_put(struct opal_dev *dev)
   ^~~~
>> lib/sed-opal.c:923:12: error: invalid storage class for function 
>> 'add_suspend_info'
static int add_suspend_info(struct opal_dev *dev, struct opal_suspend_data 
*sus)
   ^~~~
>> lib/sed-opal.c:949:12: error: invalid storage class for function 
>> 'end_session_cont'
static int end_session_cont(struct opal_dev *dev)
   ^~~~
>> lib/sed-opal.c:956:12: error: invalid storage class for function 
>> 'finalize_and_send'
static int finalize_and_send(struct opal_dev *dev, struct opal_cmd *cmd,
   ^
>> lib/sed-opal.c:972:12: error: invalid storage class for function 'gen_key'
static int gen_key(struct opal_dev *dev)
   ^~~
>> lib/sed-opal.c:1002:12: error: invalid storage class for function 
>> 'get_active_key_cont'
static int get_active_key_cont(struct opal_dev *dev)
   ^~~
>> lib/sed-opal.c:1027:12: error: invalid storage class for function 
>> 'get_active_key'
static int get_active_key(struct opal_dev *dev)
   ^~
>> lib/sed-opal.c:1067:12: error: invalid storage class for function 
>> 'generic_lr_enable_disable'
static int generic_lr_enable_disable(struct opal_cmd *cmd,
   ^
>> lib/sed-opal.c:1107:19: error: invalid storage class for function 
>> 'enable_global_lr'
static inline int enable_global_lr(struct opal_cmd *cmd, u8 *uid,
  ^~~~
>> lib/sed-opal.c:1118:12: error: invalid storage class for function 
>> 'setup_locking_range'
static int setup_locking_range(struct 

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Keith Busch]

On Mon, Dec 19, 2016 at 12:35:46PM -0700, Scott Bauer wrote:
> This patch implements the necessary logic to bring an Opal
> enabled drive out of a factory-enabled into a working
> Opal state.
> 
> This patch set also enables logic to save a password to
> be replayed during a resume from suspend.
> 
> Signed-off-by: Scott Bauer 
> Signed-off-by: Rafael Antognolli 
> ---
>  lib/Makefile|2 +-
>  lib/sed-opal.c  | 2376 
> +++
>  lib/sed-opal_internal.h |  601 
>  lib/sed.c   |  197 
>  4 files changed, 3175 insertions(+), 1 deletion(-)
>  create mode 100644 lib/sed-opal.c
>  create mode 100644 lib/sed-opal_internal.h
>  create mode 100644 lib/sed.c
> 
> diff --git a/lib/Makefile b/lib/Makefile
> index 50144a3..acb5d82 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -36,7 +36,7 @@ obj-y += bcd.o div64.o sort.o parser.o halfmd4.o 
> debug_locks.o random32.o \
>gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
>bsearch.o find_bit.o llist.o memweight.o kfifo.o \
>percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
> -  once.o
> +  once.o sed.o sed-opal.o

This seems like an optional library that some environments may wish to
opt-out of building into the kernel. Any reason not to add an entry into
the Kconfig to turn this on/off?

Re: [PATCH v3 2/5] lib: Add Sed-opal library

[Keith Busch]

On Mon, Dec 19, 2016 at 12:35:46PM -0700, Scott Bauer wrote:
> This patch implements the necessary logic to bring an Opal
> enabled drive out of a factory-enabled into a working
> Opal state.
> 
> This patch set also enables logic to save a password to
> be replayed during a resume from suspend.
> 
> Signed-off-by: Scott Bauer 
> Signed-off-by: Rafael Antognolli 
> ---
>  lib/Makefile|2 +-
>  lib/sed-opal.c  | 2376 
> +++
>  lib/sed-opal_internal.h |  601 
>  lib/sed.c   |  197 
>  4 files changed, 3175 insertions(+), 1 deletion(-)
>  create mode 100644 lib/sed-opal.c
>  create mode 100644 lib/sed-opal_internal.h
>  create mode 100644 lib/sed.c
> 
> diff --git a/lib/Makefile b/lib/Makefile
> index 50144a3..acb5d82 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -36,7 +36,7 @@ obj-y += bcd.o div64.o sort.o parser.o halfmd4.o 
> debug_locks.o random32.o \
>gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
>bsearch.o find_bit.o llist.o memweight.o kfifo.o \
>percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
> -  once.o
> +  once.o sed.o sed-opal.o

This seems like an optional library that some environments may wish to
opt-out of building into the kernel. Any reason not to add an entry into
the Kconfig to turn this on/off?

[PATCH v3 2/5] lib: Add Sed-opal library

[Scott Bauer]

This patch implements the necessary logic to bring an Opal
enabled drive out of a factory-enabled into a working
Opal state.

This patch set also enables logic to save a password to
be replayed during a resume from suspend.

Signed-off-by: Scott Bauer 
Signed-off-by: Rafael Antognolli 
---
 lib/Makefile|2 +-
 lib/sed-opal.c  | 2376 +++
 lib/sed-opal_internal.h |  601 
 lib/sed.c   |  197 
 4 files changed, 3175 insertions(+), 1 deletion(-)
 create mode 100644 lib/sed-opal.c
 create mode 100644 lib/sed-opal_internal.h
 create mode 100644 lib/sed.c

diff --git a/lib/Makefile b/lib/Makefile
index 50144a3..acb5d82 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -36,7 +36,7 @@ obj-y += bcd.o div64.o sort.o parser.o halfmd4.o 
debug_locks.o random32.o \
 gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
 percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
-once.o
+once.o sed.o sed-opal.o
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
diff --git a/lib/sed-opal.c b/lib/sed-opal.c
new file mode 100644
index 000..65f7263
--- /dev/null
+++ b/lib/sed-opal.c
@@ -0,0 +1,2376 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Authors:
+ *Scott  Bauer  
+ *Rafael Antognolli 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":OPAL: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sed-opal_internal.h"
+
+#define IO_BUFFER_LENGTH 2048
+#define MAX_TOKS 64
+
+struct opal_dev;
+typedef int (cont_fn)(struct opal_dev *dev);
+
+struct opal_cmd {
+   cont_fn *cb;
+   void *cb_data;
+
+   size_t pos;
+   u8 cmd_buf[IO_BUFFER_LENGTH * 2];
+   u8 resp_buf[IO_BUFFER_LENGTH * 2];
+   u8 *cmd;
+   u8 *resp;
+};
+
+/*
+ * On the parsed response, we don't store again the toks that are already
+ * stored in the response buffer. Instead, for each token, we just store a
+ * pointer to the position in the buffer where the token starts, and the size
+ * of the token in bytes.
+ */
+struct opal_resp_tok {
+   const u8 *pos;
+   size_t len;
+   enum OPAL_RESPONSE_TOKEN type;
+   enum OPAL_ATOM_WIDTH width;
+   union {
+   u64 u;
+   s64 s;
+   } stored;
+};
+
+/*
+ * From the response header it's not possible to know how many tokens there are
+ * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
+ * if we start dealing with messages that have more than that, we can increase
+ * this number. This is done to avoid having to make two passes through the
+ * response, the first one counting how many tokens we have and the second one
+ * actually storing the positions.
+ */
+struct parsed_resp {
+   int num;
+   struct opal_resp_tok toks[MAX_TOKS];
+};
+
+struct opal_dev;
+
+typedef int (*opal_step)(struct opal_dev *dev);
+
+struct opal_suspend_data {
+   struct opal_lock_unlock unlk;
+   u8 lr;
+   size_t key_name_len;
+   char key_name[36];
+   struct list_head node;
+};
+
+/**
+ * struct opal_dev - The structure representing a OPAL enabled SED.
+ * @sed_ctx:The SED context, contains fn pointers to sec_send/recv.
+ * @opal_step:A series of opal methods that are necessary to complete a 
comannd.
+ * @func_data:An array of parameters for the opal methods above.
+ * @state:Describes the current opal_step we're working on.
+ * @dev_lock:Locks the entire opal_dev structure.
+ * @parsed:Parsed response from controller.
+ * @prev_data:Data returned from a method to the controller
+ * @error_cb:Error function that handles closing sessions after a failed 
method.
+ * @unlk_lst:A list of Locking ranges to unlock on this device during a resume.
+ */
+struct opal_dev {
+   struct sed_context *sed_ctx;
+   const opal_step *funcs;
+   void **func_data;
+   int state;
+   struct mutex dev_lock;
+   u16 comID;
+   u32 HSN;
+   u32 TSN;
+   u64 align;
+   u64 lowest_lba;
+   struct opal_cmd cmd;
+   struct parsed_resp parsed;
+   size_t prev_d_len;
+   void *prev_data;
+   opal_step error_cb;
+   void *error_cb_data;
+
+  

[PATCH v3 2/5] lib: Add Sed-opal library

[Scott Bauer]

This patch implements the necessary logic to bring an Opal
enabled drive out of a factory-enabled into a working
Opal state.

This patch set also enables logic to save a password to
be replayed during a resume from suspend.

Signed-off-by: Scott Bauer 
Signed-off-by: Rafael Antognolli 
---
 lib/Makefile|2 +-
 lib/sed-opal.c  | 2376 +++
 lib/sed-opal_internal.h |  601 
 lib/sed.c   |  197 
 4 files changed, 3175 insertions(+), 1 deletion(-)
 create mode 100644 lib/sed-opal.c
 create mode 100644 lib/sed-opal_internal.h
 create mode 100644 lib/sed.c

diff --git a/lib/Makefile b/lib/Makefile
index 50144a3..acb5d82 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -36,7 +36,7 @@ obj-y += bcd.o div64.o sort.o parser.o halfmd4.o 
debug_locks.o random32.o \
 gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
 percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
-once.o
+once.o sed.o sed-opal.o
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
diff --git a/lib/sed-opal.c b/lib/sed-opal.c
new file mode 100644
index 000..65f7263
--- /dev/null
+++ b/lib/sed-opal.c
@@ -0,0 +1,2376 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Authors:
+ *Scott  Bauer  
+ *Rafael Antognolli 
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":OPAL: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sed-opal_internal.h"
+
+#define IO_BUFFER_LENGTH 2048
+#define MAX_TOKS 64
+
+struct opal_dev;
+typedef int (cont_fn)(struct opal_dev *dev);
+
+struct opal_cmd {
+   cont_fn *cb;
+   void *cb_data;
+
+   size_t pos;
+   u8 cmd_buf[IO_BUFFER_LENGTH * 2];
+   u8 resp_buf[IO_BUFFER_LENGTH * 2];
+   u8 *cmd;
+   u8 *resp;
+};
+
+/*
+ * On the parsed response, we don't store again the toks that are already
+ * stored in the response buffer. Instead, for each token, we just store a
+ * pointer to the position in the buffer where the token starts, and the size
+ * of the token in bytes.
+ */
+struct opal_resp_tok {
+   const u8 *pos;
+   size_t len;
+   enum OPAL_RESPONSE_TOKEN type;
+   enum OPAL_ATOM_WIDTH width;
+   union {
+   u64 u;
+   s64 s;
+   } stored;
+};
+
+/*
+ * From the response header it's not possible to know how many tokens there are
+ * on the payload. So we hardcode that the maximum will be MAX_TOKS, and later
+ * if we start dealing with messages that have more than that, we can increase
+ * this number. This is done to avoid having to make two passes through the
+ * response, the first one counting how many tokens we have and the second one
+ * actually storing the positions.
+ */
+struct parsed_resp {
+   int num;
+   struct opal_resp_tok toks[MAX_TOKS];
+};
+
+struct opal_dev;
+
+typedef int (*opal_step)(struct opal_dev *dev);
+
+struct opal_suspend_data {
+   struct opal_lock_unlock unlk;
+   u8 lr;
+   size_t key_name_len;
+   char key_name[36];
+   struct list_head node;
+};
+
+/**
+ * struct opal_dev - The structure representing a OPAL enabled SED.
+ * @sed_ctx:The SED context, contains fn pointers to sec_send/recv.
+ * @opal_step:A series of opal methods that are necessary to complete a 
comannd.
+ * @func_data:An array of parameters for the opal methods above.
+ * @state:Describes the current opal_step we're working on.
+ * @dev_lock:Locks the entire opal_dev structure.
+ * @parsed:Parsed response from controller.
+ * @prev_data:Data returned from a method to the controller
+ * @error_cb:Error function that handles closing sessions after a failed 
method.
+ * @unlk_lst:A list of Locking ranges to unlock on this device during a resume.
+ */
+struct opal_dev {
+   struct sed_context *sed_ctx;
+   const opal_step *funcs;
+   void **func_data;
+   int state;
+   struct mutex dev_lock;
+   u16 comID;
+   u32 HSN;
+   u32 TSN;
+   u64 align;
+   u64 lowest_lba;
+   struct opal_cmd cmd;
+   struct parsed_resp parsed;
+   size_t prev_d_len;
+   void *prev_data;
+   opal_step error_cb;
+   void *error_cb_data;
+
+   struct list_head unlk_lst;
+};
+
+DEFINE_SPINLOCK(list_spinlock);
+
+static void print_buffer(const u8