From: Akinobu Mita Subject: Re: [PATCH v3 3/5] target: handle odd SG mapping for data transfer memory Date: Mon, 27 Apr 2015 22:03:25 +0900 Message-ID: References: <1429972410-7146-1-git-send-email-akinobu.mita@gmail.com> <1429972410-7146-4-git-send-email-akinobu.mita@gmail.com> <553CB8E7.8060402@dev.mellanox.co.il> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Cc: target-devel@vger.kernel.org, Tim Chen , Herbert Xu , "David S. Miller" , linux-crypto@vger.kernel.org, Nicholas Bellinger , Sagi Grimberg , "Martin K. Petersen" , Christoph Hellwig , "James E.J. Bottomley" , "linux-scsi@vger.kernel.org" To: Sagi Grimberg Return-path: Received: from mail-lb0-f172.google.com ([209.85.217.172]:33060 "EHLO mail-lb0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752490AbbD0ND1 (ORCPT ); Mon, 27 Apr 2015 09:03:27 -0400 In-Reply-To: <553CB8E7.8060402@dev.mellanox.co.il> Sender: linux-crypto-owner@vger.kernel.org List-ID: 2015-04-26 19:07 GMT+09:00 Sagi Grimberg : > On 4/25/2015 5:33 PM, Akinobu Mita wrote: >> >> sbc_dif_generate() and sbc_dif_verify() currently assume that each >> SG element for data transfer memory doesn't straddle the block size >> boundary. >> >> However, when using SG_IO ioctl, we can choose the data transfer >> memory which doesn't satisfy that alignment requirement. >> >> In order to handle such cases correctly, this change inverts the outer >> loop to iterate data transfer memory and the inner loop to iterate >> protection information and enables to calculate CRC for a block which >> straddles multiple SG elements. >> >> Signed-off-by: Akinobu Mita >> Cc: Tim Chen >> Cc: Herbert Xu >> Cc: "David S. Miller" >> Cc: linux-crypto@vger.kernel.org >> Cc: Nicholas Bellinger >> Cc: Sagi Grimberg >> Cc: "Martin K. Petersen" >> Cc: Christoph Hellwig >> Cc: "James E.J. Bottomley" >> Cc: target-devel@vger.kernel.org >> Cc: linux-scsi@vger.kernel.org >> --- >> * Changes from v2: >> - Handle odd SG mapping correctly instead of giving up >> >> drivers/target/target_core_sbc.c | 108 >> +++++++++++++++++++++++++-------------- >> 1 file changed, 69 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/target/target_core_sbc.c >> b/drivers/target/target_core_sbc.c >> index edba39f..33d2426 100644 >> --- a/drivers/target/target_core_sbc.c >> +++ b/drivers/target/target_core_sbc.c >> @@ -1182,27 +1182,43 @@ sbc_dif_generate(struct se_cmd *cmd) >> { >> struct se_device *dev = cmd->se_dev; >> struct se_dif_v1_tuple *sdt; >> - struct scatterlist *dsg, *psg = cmd->t_prot_sg; >> + struct scatterlist *dsg = cmd->t_data_sg, *psg; >> sector_t sector = cmd->t_task_lba; >> void *daddr, *paddr; >> int i, j, offset = 0; >> + unsigned int block_size = dev->dev_attrib.block_size; >> >> - for_each_sg(cmd->t_data_sg, dsg, cmd->t_data_nents, i) { >> - daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; >> + for_each_sg(cmd->t_prot_sg, psg, cmd->t_prot_nents, i) { >> paddr = kmap_atomic(sg_page(psg)) + psg->offset; >> + daddr = kmap_atomic(sg_page(dsg)) + dsg->offset; >> >> - for (j = 0; j < dsg->length; j += >> dev->dev_attrib.block_size) { >> + for (j = 0; j < psg->length; >> + j += sizeof(struct se_dif_v1_tuple)) { >> + __u16 crc = 0; >> + unsigned int avail; >> >> - if (offset >= psg->length) { >> - kunmap_atomic(paddr); >> - psg = sg_next(psg); >> - paddr = kmap_atomic(sg_page(psg)) + >> psg->offset; >> - offset = 0; >> + if (offset >= dsg->length) { >> + offset -= dsg->length; >> + kunmap_atomic(daddr); > > > This unmap is inconsistent. You need to unmap (daddr - dsg->offset). > > This applies throughout the patch. Thanks for pointing out. I'll fix them all.