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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to