BUG in netxen_release_tx_buffers when TSO enabled on kernels >= 3.3 and <= 3.6

On Tue, 2013-01-22 at 05:32 -0800, Eric Dumazet wrote:

> 
> Something doesn't properly test MAX_SKB_FRAGS, we should track it and
> fix.

I guess netxen driver has a bug.

Please try the following patch :



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

On Tuesday 22 January 2013 05:56:06 Eric Dumazet wrote:
> I guess netxen driver has a bug.
> 
> Please try the following patch :
> 
> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c index
> bc165f4..695667d 100644
> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> @@ -144,7 +144,7 @@ void netxen_release_tx_buffers(struct netxen_adapter
> *adapter) buffrag->length, PCI_DMA_TODEVICE); buffrag->dma = 0ULL;
>                 }
> -               for (j = 0; j < cmd_buf->frag_count; j++) {
> +               for (j = 1; j < cmd_buf->frag_count; j++) {
>                         buffrag++;
>                         if (buffrag->dma) {
>                                 pci_unmap_page(adapter->pdev, buffrag->dma,

Perfect, I tested it, and this fixes the bug.

I should have found it on my own, I have been starring for too long at this 
function... :)

Feel free to add
Tested-by: Christoph Paasch <christoph.paasch@uclouvain.be>


Thanks, Eric!


Christoph

On Tue, 2013-01-22 at 05:56 -0800, Eric Dumazet wrote:
> On Tue, 2013-01-22 at 05:32 -0800, Eric Dumazet wrote:
> 
> > 
> > Something doesn't properly test MAX_SKB_FRAGS, we should track it and
> > fix.
> 
> I guess netxen driver has a bug.
> 
> Please try the following patch :
> 
> diff --git a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> index bc165f4..695667d 100644
> --- a/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> +++ b/drivers/net/ethernet/qlogic/netxen/netxen_nic_init.c
> @@ -144,7 +144,7 @@ void netxen_release_tx_buffers(struct netxen_adapter *adapter)
>  					 buffrag->length, PCI_DMA_TODEVICE);
>  			buffrag->dma = 0ULL;
>  		}
> -		for (j = 0; j < cmd_buf->frag_count; j++) {
> +		for (j = 1; j < cmd_buf->frag_count; j++) {
>  			buffrag++;
>  			if (buffrag->dma) {
>  				pci_unmap_page(adapter->pdev, buffrag->dma,
> 

There's another bug right here, which is that 0 is a valid DMA address
in some systems.  The driver should be calling pci_dma_mapping_error()
to find out whether an address is valid or not.  But it also wants to be
able to assign an invalid address to netxen_skb_frag::dma, and
unfortunately there is no way to do that in the current DMA API.

Ben.

On Tue, 2013-01-22 at 19:36 +0000, Ben Hutchings wrote:

> There's another bug right here, which is that 0 is a valid DMA address
> in some systems.  The driver should be calling pci_dma_mapping_error()
> to find out whether an address is valid or not.  But it also wants to be
> able to assign an invalid address to netxen_skb_frag::dma, and
> unfortunately there is no way to do that in the current DMA API.
> 

I guess we should only test ->frag_count then, and set it to 0 in TX
completion path.




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

>-----Original Message-----

>From: Eric Dumazet [mailto:eric.dumazet@gmail.com]

>Sent: Wednesday, January 23, 2013 1:30 AM

>To: Ben Hutchings

>Cc: christoph.paasch@uclouvain.be; Ian Campbell; Sony Chacko; Rajesh

>Borundia; David Miller; netdev

>Subject: Re: BUG in netxen_release_tx_buffers when TSO enabled on

>kernels >= 3.3 and <= 3.6

>

>On Tue, 2013-01-22 at 19:36 +0000, Ben Hutchings wrote:

>

>> There's another bug right here, which is that 0 is a valid DMA address

>> in some systems.  The driver should be calling pci_dma_mapping_error()

>> to find out whether an address is valid or not.  But it also wants to

>be

>> able to assign an invalid address to netxen_skb_frag::dma, and

>> unfortunately there is no way to do that in the current DMA API.

>>

>

>I guess we should only test ->frag_count then, and set it to 0 in TX

>completion path.

>

We will send a patch with the suggested fix.