Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
To:     Keith Busch <kbusch@kernel.org>
Cc:     linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-block@vger.kernel.org, axboe@kernel.dk, hch@lst.de,
        martin.petersen@oracle.com, colyli@suse.de
Subject: Re: [PATCHv2 6/7] block: add pi for nvme enhanced integrity
From:   "Martin K. Petersen" <martin.petersen@oracle.com>
Organization: Oracle Corporation
Message-ID: <yq11r0l6gd7.fsf@ca-mkp.ca.oracle.com>
References: <20220201190128.3075065-1-kbusch@kernel.org>
        <20220201190128.3075065-7-kbusch@kernel.org>
Date:   Tue, 01 Feb 2022 23:35:35 -0500
In-Reply-To: <20220201190128.3075065-7-kbusch@kernel.org> (Keith Busch's
        message of "Tue, 1 Feb 2022 11:01:27 -0800")
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?nxOP4jKeKVRmJ+6qXDLd9Vr3q8ixnQ9Ezkm0AEdyAAigXDzVJy43JkB9jBQC?=
 =?us-ascii?Q?0Gy03emkQU5jJP0CL/qn12r1TWlH/+4RmAR0gb661aRkmZ6bezfrW44q+N3k?=
 =?us-ascii?Q?byY9KSZMpf9mf65WUIOsaVL5u+hMyeDXCyadI9rOADriF9eyS/i734l1LBvq?=
 =?us-ascii?Q?oQt8e2WPsbG7BylJ5HkyaZbSUGs9nYkkvX1JnjBIPPyB6VckJW2SrmBmov7M?=
 =?us-ascii?Q?/Wb587Vwq7k7UxLu+ZF8FswEIU8EMHC8c+ZO2nWkL3nASVDpx0Oycwxlm8Af?=
 =?us-ascii?Q?aKca2tpy5+sqsDYyP9ILfak+fd9GLjrT0IvQJ5wzd5lsiXKGpPM1cKsTfOwS?=
 =?us-ascii?Q?+7Y1JEpeApf5b/J8dXtAr0plOWtuLscAW7sWjo8u52veRD/p7dFmCufvjLU3?=
 =?us-ascii?Q?4O8hWFkh0NpmkNRLeXjiwpWpMIG0rEy41vzCNNTN1OpK+z1CKM6RVYmWlMVo?=
 =?us-ascii?Q?Hs1s7/H5Ab/SpE+hJ3ztWkKS6fZrl5piR7hZFuIac1pMRvMw9nlp6ym1SPEm?=
 =?us-ascii?Q?76vDyjmi0E2z/NMXlQJWM4FMcKSg9kTlDBe6YWBvwDhF1Gbalpw9lkV8CVaJ?=
 =?us-ascii?Q?/vkC4KAaswpx9bk++OrhHL/DXwzxO8PrJGsBtRPZK9D/GJWMImG/R3wWENYj?=
 =?us-ascii?Q?SQfjRFgystPL0fnB6ftUOBVFBSvaQRqBLpKY247vXLDs5nzbaIORnG9HYW5f?=
 =?us-ascii?Q?MrVFYXfiWzuUuWoocnWV5YZYQA++Avh8es/4JMgmFRJ28jETZl8d/mXk5DLl?=
 =?us-ascii?Q?nijli8dlvdvGtBgGQA7Z64LNHQpHOeaerE5nfmp3di72R9Z6+nh4PAk+Qn0m?=
 =?us-ascii?Q?2eoMowVfTnZPA4tWs7N1/BoMbEr9LTbakjTe3WvDfjQQRCNYRarCi/RGrBJN?=
 =?us-ascii?Q?AeHJPPll1jPaaXbgW9nq62vq5CdWHSfKi2XWcAzIijRQH2Bfwsh3jzkJCE2o?=
 =?us-ascii?Q?bNQ84pQXjnZJ+4VPy0Q+AuKBz8TKK8QtLsQOynC8jH/DEWqTUru7LAOhyPMT?=
 =?us-ascii?Q?IoE6GxK2blMqPiRQ4aLP6G1b/ko63HdpO4goltGukTTMFgz0saR3AdvrEEDB?=
 =?us-ascii?Q?etrbENCIkL2KkXBxqYiHiZjhCw7b2ZJBspL3IukvDzVJjUNqpHdpxW2OcZqA?=
 =?us-ascii?Q?rX2d6Kqpf+RZhCJ/okx2h4+WtWzQL4ke7ss8gcSnrm/1DeSgyftWzNvA6NP3?=
 =?us-ascii?Q?btplzMUCHeeODGPwWvC5cHZf5k1REtIWvKC2936JYbTzvEN8KSQ6/QGLGf6G?=
 =?us-ascii?Q?0ph7n5Cgq+cRJ0f5m9dYim4Hg3s3cYMEETbMjewL7swN+1z8I+gIHofxZXIR?=
 =?us-ascii?Q?wapZQmyOZo2td3xIqWecsPM2K+eyHInXguHZPeSRV+L38EHd++Pw/tDuTKgf?=
 =?us-ascii?Q?uSRgYiHcsHICDy0xr5J4NuA2VBWypcTEUom9h5ixufXTnNuAxtj0ll3MBZs2?=
 =?us-ascii?Q?SQr9cvoOHmXYkwKsntGOaNuoiT/wQ3et0NwZFI8Mjb4cdOEzjOfLFryj2eMw?=
 =?us-ascii?Q?m7ZA/W5a27gpri6z4iAjGEJxYuRAaG/aLogtvUcNh1X246Gg6oage6e9+XUm?=
 =?us-ascii?Q?595rsqS5+MYdl3MCMJUlHX+RWu9dq91gSkFByPCTuamEYfFtc9DKHpzaQzu3?=
 =?us-ascii?Q?P5G+gMAEH082X+zhyDkr7dzrDAsRXWj16vbFIl4mJkugUR4vqsKJpGRkzhoq?=
 =?us-ascii?Q?qugQ9Q=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ce1f9f0d-afe5-4022-79eb-08d9e60575e2
X-MS-Exchange-CrossTenant-AuthSource: PH0PR10MB4759.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2022 04:35:37.8200
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: RNkTnpiHzs+30lRaVNOIOiIwNmtq3DVVvWGxEUhPzuAqWbOwbVnavrjj3sUKBGzf0632wmqWlxU9TBelIp1HmGGxu+oMs2+thePHGkpkPxU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3882
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10245 signatures=673430
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 phishscore=0
 mlxscore=0 adultscore=0 suspectscore=0 mlxlogscore=894 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202020020
X-Proofpoint-GUID: yQ-8ZMI2NV_JcW5fj8sfoy0LPbnGddGM
X-Proofpoint-ORIG-GUID: yQ-8ZMI2NV_JcW5fj8sfoy0LPbnGddGM
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Keith,

This all looks pretty good to me. Only nit I have is:

> +static blk_status_t nvme_pi_type1_verify_crc(struct blk_integrity_iter *iter)
> +{
> +	return nvme_crc64_verify(iter, T10_PI_TYPE1_PROTECTION);
> +}
> +
> +static blk_status_t nvme_pi_type1_generate_crc(struct blk_integrity_iter *iter)
> +{
> +	return nvme_crc64_generate(iter, T10_PI_TYPE1_PROTECTION);
> +}

Since we will definitely need to support the CRC32C variants, the
nvme_pi_type1_ prefix is a bit too generic. Wish we had gone with Type 4
and 5 like I originally proposed in SCSI. Not a big fan of this "almost
exactly like T10 Type 1 except for all these differences" situation that
NVMe ended up with.

Anyway. So I think the NVMe-specific format helpers need to at the very
least capture that they are for the CRC64 case.

Other than that it looks OK.

-- 
Martin K. Petersen	Oracle Linux Engineering