diff mbox

[2/2] benet: fix the misusage of zero dma address

Message ID 20100405163942P.fujita.tomonori@lab.ntt.co.jp
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

FUJITA Tomonori April 5, 2010, 7:40 a.m. UTC 
On Mon, 5 Apr 2010 12:40:59 +0530
Sathya Perla <sathyap@serverengines.com> wrote:

> Hi Fujita, thanks for the patch; pls see below:
> 
> On 02/04/10 11:53 +0900, FUJITA Tomonori wrote:
> > benet driver wrongly assumes that zero is an invalid dma address
> > (calls dma_unmap_page for only non zero dma addresses). Zero is a
> > valid dma address on some architectures. The dma length can be used
> > here.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  drivers/net/benet/be_main.c |    6 ++++--
> >  1 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> > index 8d5e27b..8828c7d 100644
> > --- a/drivers/net/benet/be_main.c
> > +++ b/drivers/net/benet/be_main.c
> > @@ -394,13 +394,15 @@ static void unmap_tx_frag(struct pci_dev *pdev, struct be_eth_wrb *wrb,
> >  	be_dws_le_to_cpu(wrb, sizeof(*wrb));
> >  
> >  	dma = (u64)wrb->frag_pa_hi << 32 | (u64)wrb->frag_pa_lo;
> > -	if (dma != 0) {
> > +	if (wrb->frag_len) {
> >  		if (unmap_single)
> >  			pci_unmap_single(pdev, dma, wrb->frag_len,
> >  				PCI_DMA_TODEVICE);
> >  		else
> >  			pci_unmap_page(pdev, dma, wrb->frag_len,
> >  				PCI_DMA_TODEVICE);
> > +
> > +		wrb->frag_len = 0;
> Why does wrb->frag_len need to be reset here?
> In the TX path, it is set to the proper value for data wrbs and zero
> for dummy and hdr wrbs.

I guess that I misunderstood why unmap_tx_frag() checks a dma address.
The checking is necessary to avoid calling pci_unamp_* API for dummy
hdr wrbs?

Anyway, if wrb->frag_len doesn't need to be reset here, the following
patch is ok?

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH v2 2/2] benet: fix the misusage of zero dma address

benet driver wrongly assumes that zero is an invalid dma address
(calls dma_unmap_page for only non zero dma addresses). Zero is a
valid dma address on some architectures. The dma length can be used
here.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
---
 drivers/net/benet/be_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Sathya Perla April 5, 2010, 8:22 a.m. UTC | #1 
On 05/04/10 16:40 +0900, FUJITA Tomonori wrote:
> > > +		wrb->frag_len = 0;
> > Why does wrb->frag_len need to be reset here?
> > In the TX path, it is set to the proper value for data wrbs and zero
> > for dummy and hdr wrbs.
> 
> I guess that I misunderstood why unmap_tx_frag() checks a dma address.
> The checking is necessary to avoid calling pci_unamp_* API for dummy
> hdr wrbs?
Yes.
> 
> Anyway, if wrb->frag_len doesn't need to be reset here, the following
> patch is ok?
Yes. Thanks.

Acked-by: Sathya Perla <sathyap@serverengines.com>
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH v2 2/2] benet: fix the misusage of zero dma address
> 
> benet driver wrongly assumes that zero is an invalid dma address
> (calls dma_unmap_page for only non zero dma addresses). Zero is a
> valid dma address on some architectures. The dma length can be used
> here.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> ---
>  drivers/net/benet/be_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
> index a27a0a1..c95e1a2 100644
> --- a/drivers/net/benet/be_main.c
> +++ b/drivers/net/benet/be_main.c
> @@ -404,7 +404,7 @@ static void unmap_tx_frag(struct pci_dev *pdev, struct be_eth_wrb *wrb,
>  	be_dws_le_to_cpu(wrb, sizeof(*wrb));
>  
>  	dma = (u64)wrb->frag_pa_hi << 32 | (u64)wrb->frag_pa_lo;
> -	if (dma != 0) {
> +	if (wrb->frag_len) {
>  		if (unmap_single)
>  			pci_unmap_single(pdev, dma, wrb->frag_len,
>  				PCI_DMA_TODEVICE);
> -- 
> 1.7.0
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
FUJITA Tomonori April 5, 2010, 8:38 a.m. UTC | #2 
On Mon, 5 Apr 2010 13:52:02 +0530
Sathya Perla <sathyap@serverengines.com> wrote:

> On 05/04/10 16:40 +0900, FUJITA Tomonori wrote:
> > > > +		wrb->frag_len = 0;
> > > Why does wrb->frag_len need to be reset here?
> > > In the TX path, it is set to the proper value for data wrbs and zero
> > > for dummy and hdr wrbs.
> > 
> > I guess that I misunderstood why unmap_tx_frag() checks a dma address.
> > The checking is necessary to avoid calling pci_unamp_* API for dummy
> > hdr wrbs?
> Yes.
> > 
> > Anyway, if wrb->frag_len doesn't need to be reset here, the following
> > patch is ok?
> Yes. Thanks.
> 
> Acked-by: Sathya Perla <sathyap@serverengines.com>

Thanks!

Can I also get your ack on "[PATCH 1/2] benet: use the dma state API
instead of the pci equivalents"?

http://patchwork.ozlabs.org/patch/49265/


The reason why I want to replace the PCI DMA state API is:

http://marc.info/?l=linux-netdev&m=127037540020276&w=2

Note that I use DEFINE_DMA_UNMAP_ADDR for bus in struct
be_rx_page_info because dma_unmap_addr and DEFINE_DMA_UNMAP_ADDR are
supposed to be used together.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller April 8, 2010, 3:59 a.m. UTC | #3 
From: Sathya Perla <sathyap@serverengines.com>
Date: Mon, 5 Apr 2010 13:52:02 +0530

> On 05/04/10 16:40 +0900, FUJITA Tomonori wrote:
>> > > +		wrb->frag_len = 0;
>> > Why does wrb->frag_len need to be reset here?
>> > In the TX path, it is set to the proper value for data wrbs and zero
>> > for dummy and hdr wrbs.
>> 
>> I guess that I misunderstood why unmap_tx_frag() checks a dma address.
>> The checking is necessary to avoid calling pci_unamp_* API for dummy
>> hdr wrbs?
> Yes.
>> 
>> Anyway, if wrb->frag_len doesn't need to be reset here, the following
>> patch is ok?
> Yes. Thanks.
> 
> Acked-by: Sathya Perla <sathyap@serverengines.com>

Applied to net-next-2.6
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/benet/be_main.c b/drivers/net/benet/be_main.c
index a27a0a1..c95e1a2 100644
--- a/drivers/net/benet/be_main.c
+++ b/drivers/net/benet/be_main.c
@@ -404,7 +404,7 @@  static void unmap_tx_frag(struct pci_dev *pdev, struct be_eth_wrb *wrb,
 	be_dws_le_to_cpu(wrb, sizeof(*wrb));
 
 	dma = (u64)wrb->frag_pa_hi << 32 | (u64)wrb->frag_pa_lo;
-	if (dma != 0) {
+	if (wrb->frag_len) {
 		if (unmap_single)
 			pci_unmap_single(pdev, dma, wrb->frag_len,
 				PCI_DMA_TODEVICE);