From: Christoph Hellwig <hch@infradead.org> To: Satya Tangirala <satyat@google.com> Cc: linux-block@vger.kernel.org, linux-scsi@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, Barani Muthukumaran <bmuthuku@qti.qualcomm.com>, Kuohong Wang <kuohong.wang@mediatek.com>, Kim Boojin <boojin.kim@samsung.com> Subject: Re: [PATCH v5 5/9] scsi: ufs: UFS crypto API Date: Thu, 31 Oct 2019 11:23:14 -0700 [thread overview] Message-ID: <20191031182314.GD23601@infradead.org> (raw) In-Reply-To: <20191028072032.6911-6-satyat@google.com> > +static size_t get_keysize_bytes(enum ufs_crypto_key_size size) > +{ > + switch (size) { > + case UFS_CRYPTO_KEY_SIZE_128: return 16; > + case UFS_CRYPTO_KEY_SIZE_192: return 24; > + case UFS_CRYPTO_KEY_SIZE_256: return 32; > + case UFS_CRYPTO_KEY_SIZE_512: return 64; > + default: return 0; > + } > +} Please fix the indentation and move all the returns to their own lines. There are various more spots that will need to be fixed like this as well later in the patch. > + > +static int ufshcd_crypto_cap_find(void *hba_p, > + enum blk_crypto_mode_num crypto_mode, > + unsigned int data_unit_size) > +{ > + struct ufs_hba *hba = hba_p; Please properly type the first argument. > + case UFS_CRYPTO_ALG_BITLOCKER_AES_CBC: // fallthrough Please don't use // comments. > +static void program_key(struct ufs_hba *hba, > + const union ufs_crypto_cfg_entry *cfg, > + int slot) The function name needs a ufshcd prefix. > + wmb(); > + for (i = 0; i < 16; i++) { > + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[i]), > + slot_offset + i * sizeof(cfg->reg_val[0])); > + /* Spec says each dword in key must be written sequentially */ > + wmb(); > + } > + /* Write dword 17 */ > + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[17]), > + slot_offset + 17 * sizeof(cfg->reg_val[0])); > + /* Dword 16 must be written last */ > + wmb(); > + /* Write dword 16 */ > + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[16]), > + slot_offset + 16 * sizeof(cfg->reg_val[0])); > + wmb(); wmb() has no meaning for MMIO operations, something looks very fishy here. > +static int ufshcd_crypto_keyslot_program(void *hba_p, const u8 *key, > + enum blk_crypto_mode_num crypto_mode, > + unsigned int data_unit_size, > + unsigned int slot) > +{ > + struct ufs_hba *hba = hba_p; This is not a very type safe API. I think the proper thing to do would be to allocte the struct keyslot_manager in the driver (ufshcd) as part of the containing structure (ufs_hba) and then just have a keyslot_manager_init that initializes the field. Then pass the struct keyslot_manager to the methods, which can use container_of to get the containing structure. > +#define NUM_KEYSLOTS(hba) (hba->crypto_capabilities.config_count + 1) Please make this an inline function.
WARNING: multiple messages have this Message-ID (diff)
From: Christoph Hellwig <hch@infradead.org> To: Satya Tangirala <satyat@google.com> Cc: linux-scsi@vger.kernel.org, Kim Boojin <boojin.kim@samsung.com>, Kuohong Wang <kuohong.wang@mediatek.com>, Barani Muthukumaran <bmuthuku@qti.qualcomm.com>, linux-f2fs-devel@lists.sourceforge.net, linux-block@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [f2fs-dev] [PATCH v5 5/9] scsi: ufs: UFS crypto API Date: Thu, 31 Oct 2019 11:23:14 -0700 [thread overview] Message-ID: <20191031182314.GD23601@infradead.org> (raw) In-Reply-To: <20191028072032.6911-6-satyat@google.com> > +static size_t get_keysize_bytes(enum ufs_crypto_key_size size) > +{ > + switch (size) { > + case UFS_CRYPTO_KEY_SIZE_128: return 16; > + case UFS_CRYPTO_KEY_SIZE_192: return 24; > + case UFS_CRYPTO_KEY_SIZE_256: return 32; > + case UFS_CRYPTO_KEY_SIZE_512: return 64; > + default: return 0; > + } > +} Please fix the indentation and move all the returns to their own lines. There are various more spots that will need to be fixed like this as well later in the patch. > + > +static int ufshcd_crypto_cap_find(void *hba_p, > + enum blk_crypto_mode_num crypto_mode, > + unsigned int data_unit_size) > +{ > + struct ufs_hba *hba = hba_p; Please properly type the first argument. > + case UFS_CRYPTO_ALG_BITLOCKER_AES_CBC: // fallthrough Please don't use // comments. > +static void program_key(struct ufs_hba *hba, > + const union ufs_crypto_cfg_entry *cfg, > + int slot) The function name needs a ufshcd prefix. > + wmb(); > + for (i = 0; i < 16; i++) { > + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[i]), > + slot_offset + i * sizeof(cfg->reg_val[0])); > + /* Spec says each dword in key must be written sequentially */ > + wmb(); > + } > + /* Write dword 17 */ > + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[17]), > + slot_offset + 17 * sizeof(cfg->reg_val[0])); > + /* Dword 16 must be written last */ > + wmb(); > + /* Write dword 16 */ > + ufshcd_writel(hba, le32_to_cpu(cfg->reg_val[16]), > + slot_offset + 16 * sizeof(cfg->reg_val[0])); > + wmb(); wmb() has no meaning for MMIO operations, something looks very fishy here. > +static int ufshcd_crypto_keyslot_program(void *hba_p, const u8 *key, > + enum blk_crypto_mode_num crypto_mode, > + unsigned int data_unit_size, > + unsigned int slot) > +{ > + struct ufs_hba *hba = hba_p; This is not a very type safe API. I think the proper thing to do would be to allocte the struct keyslot_manager in the driver (ufshcd) as part of the containing structure (ufs_hba) and then just have a keyslot_manager_init that initializes the field. Then pass the struct keyslot_manager to the methods, which can use container_of to get the containing structure. > +#define NUM_KEYSLOTS(hba) (hba->crypto_capabilities.config_count + 1) Please make this an inline function. _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2019-10-31 18:23 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-10-28 7:20 [PATCH v5 0/9] Inline Encryption Support Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-28 7:20 ` [PATCH v5 1/9] block: Keyslot Manager for Inline Encryption Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:04 ` Christoph Hellwig 2019-10-31 18:04 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 2/9] block: Add encryption context to struct bio Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:16 ` Christoph Hellwig 2019-10-31 18:16 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 3/9] block: blk-crypto for Inline Encryption Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 17:57 ` Christoph Hellwig 2019-10-31 17:57 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 20:50 ` Theodore Y. Ts'o 2019-10-31 20:50 ` [f2fs-dev] " Theodore Y. Ts'o 2019-10-31 21:22 ` Christoph Hellwig 2019-10-31 21:22 ` [f2fs-dev] " Christoph Hellwig 2019-11-05 2:01 ` Eric Biggers 2019-11-05 2:01 ` [f2fs-dev] " Eric Biggers 2019-11-05 15:39 ` Christoph Hellwig 2019-11-05 15:39 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 4/9] scsi: ufs: UFS driver v2.1 spec crypto additions Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-28 7:20 ` [PATCH v5 5/9] scsi: ufs: UFS crypto API Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:23 ` Christoph Hellwig [this message] 2019-10-31 18:23 ` Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 6/9] scsi: ufs: Add inline encryption support to UFS Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:26 ` Christoph Hellwig 2019-10-31 18:26 ` [f2fs-dev] " Christoph Hellwig 2019-10-28 7:20 ` [PATCH v5 7/9] fscrypt: add inline encryption support Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 18:32 ` Christoph Hellwig 2019-10-31 18:32 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 20:21 ` Eric Biggers 2019-10-31 20:21 ` [f2fs-dev] " Eric Biggers 2019-10-31 21:21 ` Christoph Hellwig 2019-10-31 21:21 ` [f2fs-dev] " Christoph Hellwig 2019-10-31 22:25 ` Eric Biggers 2019-10-31 22:25 ` [f2fs-dev] " Eric Biggers 2019-11-05 0:15 ` Christoph Hellwig 2019-11-05 0:15 ` [f2fs-dev] " Christoph Hellwig 2019-11-05 1:03 ` Eric Biggers 2019-11-05 1:03 ` [f2fs-dev] " Eric Biggers 2019-11-05 3:12 ` Eric Biggers 2019-11-05 3:12 ` [f2fs-dev] " Eric Biggers 2019-10-28 7:20 ` [PATCH v5 8/9] f2fs: " Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2019-10-31 17:14 ` Jaegeuk Kim 2019-10-31 17:14 ` [f2fs-dev] " Jaegeuk Kim 2019-10-28 7:20 ` [PATCH v5 9/9] ext4: " Satya Tangirala 2019-10-28 7:20 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191031182314.GD23601@infradead.org \ --to=hch@infradead.org \ --cc=bmuthuku@qti.qualcomm.com \ --cc=boojin.kim@samsung.com \ --cc=kuohong.wang@mediatek.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fscrypt@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=satyat@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.