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

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

[Maurizio Lombardi]

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

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.

This patch fixes the bug by replacing dma_map_sg() with scsi_dma_map().

[56068.747547] WARNING: CPU: 1 PID: 12048 at lib/dma-debug.c:1080 check_unmap+0x90a/0xa00()
[56068.785706] fcoe ctlr_0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x0000000202a1aa00] [size=36 bytes]
[56068.846700] Modules linked in: 8021q garp stp mrp llc fcoe bnx2fc cnic uio libfcoe libfc scsi_transport_fc scsi_tgt cfg80211 rfkill x86_pkg_temp_thermal coretemp kvm_intel kvm crct10dif_pclmul iTCO_wdt crc32_pclmul gpio_ich crc32c_intel iTCO_vendor_support ghash_clmulni_intel joydev lpc_ich hpwdt nfsd mfd_core microcode hpilo serio_raw pcspkr ipmi_si auth_rpcgss nfs_acl lockd shpchp ipmi_msghandler sunrpc xfs e1000e mgag200 i2c_algo_bit drm_kms_helper ttm ptp drm ata_generic pata_acpi bnx2x i2c_core pps_core hpsa mdio libcrc32c
[56069.077673] CPU: 1 PID: 12048 Comm: bnx2fc_thread/1 Not tainted 3.16.0-rc7+ #1
[56069.115286] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
[56069.145091]  0000000000000009 ffff8801f36dbbd8 ffffffff816c323a ffff8801f36dbc20
[56069.185083]  ffff8801f36dbc10 ffffffff8108350d ffff8800e9880960 ffff8801f36dbd00
[56069.222738]  0000000000000024 0000000202a1aa00 0000000000000001 ffff8801f36dbc70
[56069.263389] Call Trace:
[56069.275945]  [<ffffffff816c323a>] dump_stack+0x45/0x56
[56069.302133]  [<ffffffff8108350d>] warn_slowpath_common+0x7d/0xa0
[56069.334050]  [<ffffffff8108357c>] warn_slowpath_fmt+0x4c/0x50
[56069.364537]  [<ffffffff8135214c>] ? debug_dma_mapping_error+0x7c/0x90
[56069.398268]  [<ffffffff8135423a>] check_unmap+0x90a/0xa00
[56069.423761]  [<ffffffff81354485>] debug_dma_unmap_sg+0x65/0x110
[56069.474134]  [<ffffffff8145ee93>] scsi_dma_unmap+0x73/0xb0
[56069.499313]  [<ffffffffa04cd9bc>] bnx2fc_process_scsi_cmd_compl+0xcc/0x2a0 [bnx2fc]
[56069.535588]  [<ffffffffa04c759b>] bnx2fc_process_cq_compl+0x21b/0x280 [bnx2fc]
[56069.570311]  [<ffffffffa04c2d76>] bnx2fc_percpu_io_thread+0x106/0x180 [bnx2fc]
[56069.604560]  [<ffffffffa04c2c70>] ? bnx2fc_get_src_mac+0x20/0x20 [bnx2fc]
[56069.635801]  [<ffffffff810a4392>] kthread+0xd2/0xf0
[56069.659421]  [<ffffffff810a42c0>] ? kthread_create_on_node+0x170/0x170
[56069.689416]  [<ffffffff816ca3bc>] ret_from_fork+0x7c/0xb0
[56069.715510]  [<ffffffff810a42c0>] ? kthread_create_on_node+0x170/0x170

Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/scsi/bnx2fc/bnx2fc_io.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
index 7bc47fc..fe2cd9b 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_io.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
@@ -1640,8 +1640,6 @@ static int bnx2fc_split_bd(struct bnx2fc_cmd *io_req, u64 addr, int sg_len,
 
 static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
 {
-	struct bnx2fc_interface *interface = io_req->port->priv;
-	struct bnx2fc_hba *hba = interface->hba;
 	struct scsi_cmnd *sc = io_req->sc_cmd;
 	struct fcoe_bd_ctx *bd = io_req->bd_tbl->bd_tbl;
 	struct scatterlist *sg;
@@ -1653,8 +1651,8 @@ static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
 	u64 addr;
 	int i;
 
-	sg_count = dma_map_sg(&hba->pcidev->dev, scsi_sglist(sc),
-			      scsi_sg_count(sc), sc->sc_data_direction);
+	sg_count = scsi_dma_map(sc);
+
 	scsi_for_each_sg(sc, sg, sg_count, i) {
 		sg_len = sg_dma_len(sg);
 		addr = sg_dma_address(sg);
-- 
Maurizio Lombardi

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

[Christoph Hellwig]

Can I get a review for this one, please?

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

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> Can I get a review for this one, please?

Looks good to me.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Eddie Wai]

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.

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

> This patch fixes the bug by replacing dma_map_sg() with scsi_dma_map().
> 
> [56068.747547] WARNING: CPU: 1 PID: 12048 at lib/dma-debug.c:1080 check_unmap+0x90a/0xa00()
> [56068.785706] fcoe ctlr_0: DMA-API: device driver tries to free DMA memory it has not allocated [device address=0x0000000202a1aa00] [size=36 bytes]
> [56068.846700] Modules linked in: 8021q garp stp mrp llc fcoe bnx2fc cnic uio libfcoe libfc scsi_transport_fc scsi_tgt cfg80211 rfkill x86_pkg_temp_thermal coretemp kvm_intel kvm crct10dif_pclmul iTCO_wdt crc32_pclmul gpio_ich crc32c_intel iTCO_vendor_support ghash_clmulni_intel joydev lpc_ich hpwdt nfsd mfd_core microcode hpilo serio_raw pcspkr ipmi_si auth_rpcgss nfs_acl lockd shpchp ipmi_msghandler sunrpc xfs e1000e mgag200 i2c_algo_bit drm_kms_helper ttm ptp drm ata_generic pata_acpi bnx2x i2c_core pps_core hpsa mdio libcrc32c
> [56069.077673] CPU: 1 PID: 12048 Comm: bnx2fc_thread/1 Not tainted 3.16.0-rc7+ #1
> [56069.115286] Hardware name: HP ProLiant DL120 G7, BIOS J01 07/01/2013
> [56069.145091]  0000000000000009 ffff8801f36dbbd8 ffffffff816c323a ffff8801f36dbc20
> [56069.185083]  ffff8801f36dbc10 ffffffff8108350d ffff8800e9880960 ffff8801f36dbd00
> [56069.222738]  0000000000000024 0000000202a1aa00 0000000000000001 ffff8801f36dbc70
> [56069.263389] Call Trace:
> [56069.275945]  [<ffffffff816c323a>] dump_stack+0x45/0x56
> [56069.302133]  [<ffffffff8108350d>] warn_slowpath_common+0x7d/0xa0
> [56069.334050]  [<ffffffff8108357c>] warn_slowpath_fmt+0x4c/0x50
> [56069.364537]  [<ffffffff8135214c>] ? debug_dma_mapping_error+0x7c/0x90
> [56069.398268]  [<ffffffff8135423a>] check_unmap+0x90a/0xa00
> [56069.423761]  [<ffffffff81354485>] debug_dma_unmap_sg+0x65/0x110
> [56069.474134]  [<ffffffff8145ee93>] scsi_dma_unmap+0x73/0xb0
> [56069.499313]  [<ffffffffa04cd9bc>] bnx2fc_process_scsi_cmd_compl+0xcc/0x2a0 [bnx2fc]
> [56069.535588]  [<ffffffffa04c759b>] bnx2fc_process_cq_compl+0x21b/0x280 [bnx2fc]
> [56069.570311]  [<ffffffffa04c2d76>] bnx2fc_percpu_io_thread+0x106/0x180 [bnx2fc]
> [56069.604560]  [<ffffffffa04c2c70>] ? bnx2fc_get_src_mac+0x20/0x20 [bnx2fc]
> [56069.635801]  [<ffffffff810a4392>] kthread+0xd2/0xf0
> [56069.659421]  [<ffffffff810a42c0>] ? kthread_create_on_node+0x170/0x170
> [56069.689416]  [<ffffffff816ca3bc>] ret_from_fork+0x7c/0xb0
> [56069.715510]  [<ffffffff810a42c0>] ? kthread_create_on_node+0x170/0x170
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/scsi/bnx2fc/bnx2fc_io.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_io.c b/drivers/scsi/bnx2fc/bnx2fc_io.c
> index 7bc47fc..fe2cd9b 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_io.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_io.c
> @@ -1640,8 +1640,6 @@ static int bnx2fc_split_bd(struct bnx2fc_cmd *io_req, u64 addr, int sg_len,
>  
>  static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
>  {
> -	struct bnx2fc_interface *interface = io_req->port->priv;
> -	struct bnx2fc_hba *hba = interface->hba;
>  	struct scsi_cmnd *sc = io_req->sc_cmd;
>  	struct fcoe_bd_ctx *bd = io_req->bd_tbl->bd_tbl;
>  	struct scatterlist *sg;
> @@ -1653,8 +1651,8 @@ static int bnx2fc_map_sg(struct bnx2fc_cmd *io_req)
>  	u64 addr;
>  	int i;
>  
> -	sg_count = dma_map_sg(&hba->pcidev->dev, scsi_sglist(sc),
> -			      scsi_sg_count(sc), sc->sc_data_direction);
> +	sg_count = scsi_dma_map(sc);
> +
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);

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

[Maurizio Lombardi]

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

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

[Chad Dupuis]

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
>

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

[Christoph Hellwig]

On Fri, Aug 22, 2014 at 08:08:33AM -0400, 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:

This looks like a better plan to me.  Can you also document why you
are using plain dma_map/unmap_sg and a different dma device in the code?

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

[Maurizio Lombardi]

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

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

[Eddie Wai]

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

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

[Chad Dupuis]

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

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

[Eddie Wai]

On Mon, 2014-08-25 at 15:15 -0400, Chad Dupuis wrote:
> 
> 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;
>          }
>   }
> 
Chad, the patch looks fine to me.  Thanks for verifying it.  I asked for
some test coverage earlier only because I was afraid that there might be
corner cases where the hba might not be available during the unmap
process.

Acked-by: Eddie Wai <eddie.wai@broadcom.com>

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

[Christoph Hellwig]

Chad,

can you send out your last version with a proper changelog, signoff,
and the ack from Eddie included?  Also can you prioritize getting
the shared skb patch tested?

Thanks,
	Christoph