Re: [PATCH] bnx2fc: fix incorrect DMA memory mapping in bnx2fc_map_sg()

[Maurizio Lombardi <mlombard@redhat.com>]

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