Re: [PATCH v1 0/7] SED OPAL Library

[Christoph Hellwig <hch@infradead.org>]

Hi Scott,

I took a look at the code and here are some very high level comments:

 - we only call into block_device_operations.sec_ops from the ioctl
   handlers.  So instead of adding it to the block layer I'd rather
   structure the code so that the driver itself calls a new common
   blkdev_sed_ioctl handler implemented in lib/sed.c, which then gets
   callbacks passed directly from the calling, similar to how
   opal_unlock_from_suspend works.  And the callbacks might actually
   be condensed to one I think, given that all potential
   implementations would basically just dispatch to two
   different opcode but otherwise use the same implementation.
 - talking about lib/sed*.c - I'd move it to block/
 - there are a lot of levels of indirection in the code, I think
   we can condense them down a bit to basically just having the
   main blkdev_sed_ioctl entry point, which should check
   bdev_sec_capable first, and then dispatch to the security
   types, probably through a little method table.
 - what's so special about request_user_key that it can't be inline
   into the only caller but needs a separate file?
 - please don't use pointer indirections in your userspace ABI,
   struct sed_key will be a pain to handle for 32-bit userspace
   on 64-bit kernels.  I don't fully understand what the key_type
   is for anyway - it seems like exactly one type is supported
   per call anyway.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme