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.
> >>
> >>>
> >>> 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
