Eddie, Maurizio,
Since it looks like there can be a difference in the pdev would it make
sense instead to convert the unmap function to use the bare DMA
API so we're consistent in our use of hba->pcidev->dev? Maybe something
like this:
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c
b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 32a5e0a..8b4adcf 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1700,9 +1700,12 @@ static int bnx2fc_build_bd_list_from_sg(struct
bnx2fc_cmd *io_req)
static void bnx2fc_unmap_sg_list(struct bnx2fc_cmd *io_req)
{
struct scsi_cmnd *sc = io_req->sc_cmd;
+ struct bnx2fc_interface *interface = io_req->port->priv;
+ struct bnx2fc_hba *hba = interface->hba;
- if (io_req->bd_tbl->bd_valid && sc) {
- scsi_dma_unmap(sc);
+ if (io_req->bd_tbl->bd_valid && sc && scsi_sg_count(sc)) {
+ dma_unmap_sg(&hba->pcidev->dev, scsi_sglist(sc),
+ scsi_sg_count(sc), sc->sc_data_direction);
io_req->bd_tbl->bd_valid = 0;
}
}
On Fri, 22 Aug 2014, Maurizio Lombardi wrote:
Hi Eddie,
On 08/20/2014 07:35 PM, Eddie Wai wrote:
On Mon, 2014-08-04 at 10:20 +0200, Maurizio Lombardi wrote:
In the bnx2fc_map_sg() function, the original behaviour is to
allocate the DMA memory by directly calling dma_map_sg()
instead of using scsi_dma_map().
In contrast, bnx2fc_unmap_sg_list() calls scsi_dma_unmap().
The problem is that bnx2fc_map_sg() passes to the dma_map_sg() function
the pointer to the "device" structure taken from pci_dev
and not from Scsi_Host (as scsi_dma_map/unmap() do).
Great find, Maurizio, Thanks again.
Thanks :)
In some circumstances, the two devices may be different and the
DMA library will be unable to find the correct entry
when freeing the memory.
I'm curious at how the hba->pcidev->dev in the dma_map_sg call could end
up being different from the scsi_cmnd's device->host->dma_dev... Can
you share the test scenario?
It happens every time you set up an fcoe interface, so you just have to compile the
kernel with the DMA API debug option.
It is 100% reproducible with RHEL6, RHEL7 and the latest mainline kernel also.
I see that scsi_dma_map could possibly return a -ENOMEM. It would be
best if we also add the check for sg_count being < 0 and return the
bd_count or 0.
scsi_for_each_sg(sc, sg, sg_count, i) {
sg_len = sg_dma_len(sg);
addr = sg_dma_address(sg);
True, but sg_count is only used for the "scsi_for_each()",
if it is 0 or -ENOMEM it will simply skip the loop and will execute "return bd_count".
Regards,
Maurizio Lombardi
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
