| Message ID | 1567701836-29725-3-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] block: centralize PI remapping logic to the block layer | expand |
On Thu, Sep 05, 2019 at 07:43:56PM +0300, Max Gurtovoy wrote: > Use block layer definition instead of re-defining it with the same > values. > > Suggested-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
> Use block layer definition instead of re-defining it with the same > values. The nvme_setup_rw is fine, but nvme_init_integrity gets values from the controller id structure so I think it will be better to stick with the enums that are referenced in the spec (even if they happen to match the block layer values).
On 9/5/2019 11:52 PM, Sagi Grimberg wrote: > >> Use block layer definition instead of re-defining it with the same >> values. > > The nvme_setup_rw is fine, but nvme_init_integrity gets values from > the controller id structure so I think it will be better to stick with > the enums that are referenced in the spec (even if they happen to match > the block layer values). according to the spec: "NVM Express supports the same end-to-end protection types as DIF. The type of end-to-end data protection (Type 1, Type 2, or Type 3) is selected when a namespace is formatted and is reported in the Identify Namespace data structure." This is exactly what this patch does.
On Thu, Sep 05, 2019 at 01:52:39PM -0700, Sagi Grimberg wrote: > >> Use block layer definition instead of re-defining it with the same >> values. > > The nvme_setup_rw is fine, but nvme_init_integrity gets values from > the controller id structure so I think it will be better to stick with > the enums that are referenced in the spec (even if they happen to match > the block layer values). These values aren't really block layer values, but from the SCSI spec, which NVMe references. So I think this is fine, but if it is a little confusion we'll have to add a comment.
>> The nvme_setup_rw is fine, but nvme_init_integrity gets values from >> the controller id structure so I think it will be better to stick with >> the enums that are referenced in the spec (even if they happen to match >> the block layer values). > > These values aren't really block layer values, but from the SCSI spec, > which NVMe references. So I think this is fine, but if it is a little > confusion we'll have to add a comment. Yes, at least for patch #2 where we set the disk->protection_type we need to explain that the dps match t10 in the type definitions.
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 1850ccd..0f799bd 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -663,11 +663,11 @@ static inline blk_status_t nvme_setup_rw(struct nvme_ns *ns, } switch (req->rq_disk->protection_type) { - case NVME_NS_DPS_PI_TYPE3: + case T10_PI_TYPE3_PROTECTION: control |= NVME_RW_PRINFO_PRCHK_GUARD; break; - case NVME_NS_DPS_PI_TYPE1: - case NVME_NS_DPS_PI_TYPE2: + case T10_PI_TYPE1_PROTECTION: + case T10_PI_TYPE2_PROTECTION: control |= NVME_RW_PRINFO_PRCHK_GUARD | NVME_RW_PRINFO_PRCHK_REF; cmnd->rw.reftag = cpu_to_le32(t10_pi_ref_tag(req)); @@ -1498,13 +1498,13 @@ static void nvme_init_integrity(struct gendisk *disk, u16 ms) memset(&integrity, 0, sizeof(integrity)); switch (disk->protection_type) { - case NVME_NS_DPS_PI_TYPE3: + case T10_PI_TYPE3_PROTECTION: integrity.profile = &t10_pi_type3_crc; integrity.tag_size = sizeof(u16) + sizeof(u32); integrity.flags |= BLK_INTEGRITY_DEVICE_CAPABLE; break; - case NVME_NS_DPS_PI_TYPE1: - case NVME_NS_DPS_PI_TYPE2: + case T10_PI_TYPE1_PROTECTION: + case T10_PI_TYPE2_PROTECTION: integrity.profile = &t10_pi_type1_crc; integrity.tag_size = sizeof(u16); integrity.flags |= BLK_INTEGRITY_DEVICE_CAPABLE; diff --git a/include/linux/nvme.h b/include/linux/nvme.h index 01aa6a6..8d45c3e 100644 --- a/include/linux/nvme.h +++ b/include/linux/nvme.h @@ -381,9 +381,6 @@ enum { NVME_NS_DPC_PI_TYPE1 = 1 << 0, NVME_NS_DPS_PI_FIRST = 1 << 3, NVME_NS_DPS_PI_MASK = 0x7, - NVME_NS_DPS_PI_TYPE1 = 1, - NVME_NS_DPS_PI_TYPE2 = 2, - NVME_NS_DPS_PI_TYPE3 = 3, }; struct nvme_ns_id_desc {
Use block layer definition instead of re-defining it with the same values. Suggested-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> --- drivers/nvme/host/core.c | 12 ++++++------ include/linux/nvme.h | 3 --- 2 files changed, 6 insertions(+), 9 deletions(-)