On Fri, 22 Aug 2014, Eddie Wai wrote:
On Fri, 2014-08-22 at 20:12 +0200, Maurizio Lombardi wrote:
Hi Chad,
On 08/22/2014 02:08 PM, Chad Dupuis wrote:
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:
I looked at what the bnx2i driver code does, it has a hba->pcidev pointer too but makes use of scsi_map_sg()/scsi_unmap_sg().
So, from my point of view, it is the bnx2fc's map operation (that bypasses scsi_map_sg() and uses dma_map_sg()) to look like a suspicious exception and I decided to change it.
So far, I didn't get any error or strange behaviour after this change.
Eddie, what do you think about it?
Regards,
Maurizio Lombardi
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.
This got me thinking that the scsi_host's dma_dev must have been changed
recently and I think I found the culprit:
http://www.spinics.net/lists/linux-scsi/msg65225.html
So back when, we changed the scsi_host's parent from hba->pcidev->dev to
the fcoe_ctlr_device's dev to basically align with the soft fcoe code to
fix a fcoemon expectation. Although this was changed correctly, the sg
DMA mapping routine did not adapt to it; hence the mismatch.
Prior to this change, both sg dma map and unmap were done to the
hba->pcidev->dev along with all other DMA mapping routines. I also see
that even though bnx2i uses the scsi_map_sg/scsi_unmap_sg pairs,
ultimately, they were operating on the hba->pcidev->dev device.
My opinion is that it would probably be in our best interest to have all
DMA mapping/unmapping routines continue to operate on the
hba->pcidev->dev directly to prevent any unforeseen DMA problems.
Thanks Eddie. Would my original patch work then (I added comments per
Christoph's request):
diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c
b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 32a5e0a..6a61a0d 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1651,6 +1651,10 @@ static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
u64 addr;
int i;
+ /*
+ * Use dma_map_sg directly to ensure we're using the correct
+ * dev struct off of pcidev.
+ */
sg_count = dma_map_sg(&hba->pcidev->dev, scsi_sglist(sc),
scsi_sg_count(sc), sc->sc_data_direction);
scsi_for_each_sg(sc, sg, sg_count, i) {
@@ -1700,9 +1704,16 @@ 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);
+ /*
+ * Use dma_unmap_sg directly to ensure we're using the correct
+ * dev struct off of pcidev.
+ */
+ 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;
}
}
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
