From mboxrd@z Thu Jan 1 00:00:00 1970 From: Luis Henriques Subject: Re: [net PATCH] atl1e: fix dma mapping warnings Date: Fri, 12 Jul 2013 17:38:10 +0100 Message-ID: <877ggvsphp.fsf@canonical.com> References: <1373641128-13634-1-git-send-email-nhorman@tuxdriver.com> <87bo67sr1x.fsf@canonical.com> <20130712162928.GI5854@hmsreliant.think-freely.org> Mime-Version: 1.0 Content-Type: text/plain Cc: netdev@vger.kernel.org, Jay Cliburn , Chris Snook , "David S. Miller" To: Neil Horman Return-path: Received: from youngberry.canonical.com ([91.189.89.112]:50958 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964936Ab3GLQiP (ORCPT ); Fri, 12 Jul 2013 12:38:15 -0400 In-Reply-To: <20130712162928.GI5854@hmsreliant.think-freely.org> (Neil Horman's message of "Fri, 12 Jul 2013 12:29:28 -0400") Sender: netdev-owner@vger.kernel.org List-ID: Neil Horman writes: > On Fri, Jul 12, 2013 at 05:04:26PM +0100, Luis Henriques wrote: >> Neil Horman writes: >> >> > Recently had this backtrace reported: >> > WARNING: at lib/dma-debug.c:937 check_unmap+0x47d/0x930() >> > Hardware name: System Product Name >> > ATL1E 0000:02:00.0: DMA-API: device driver failed to check map error[device >> > address=0x00000000cbfd1000] [size=90 bytes] [mapped as single] >> > Modules linked in: xt_conntrack nf_conntrack ebtable_filter ebtables >> > ip6table_filter ip6_tables snd_hda_codec_hdmi snd_hda_codec_realtek iTCO_wdt >> > iTCO_vendor_support snd_hda_intel acpi_cpufreq mperf coretemp btrfs zlib_deflate >> > snd_hda_codec snd_hwdep microcode raid6_pq libcrc32c snd_seq usblp serio_raw xor >> > snd_seq_device joydev snd_pcm snd_page_alloc snd_timer snd lpc_ich i2c_i801 >> > soundcore mfd_core atl1e asus_atk0110 ata_generic pata_acpi radeon i2c_algo_bit >> > drm_kms_helper ttm drm i2c_core pata_marvell uinput >> > Pid: 314, comm: systemd-journal Not tainted 3.9.0-0.rc6.git2.3.fc19.x86_64 #1 >> > Call Trace: >> > [] warn_slowpath_common+0x66/0x80 >> > [] warn_slowpath_fmt+0x4c/0x50 >> > [] check_unmap+0x47d/0x930 >> > [] ? sched_clock_cpu+0xa8/0x100 >> > [] debug_dma_unmap_page+0x5f/0x70 >> > [] ? unmap_single+0x20/0x30 >> > [] atl1e_intr+0x3a1/0x5b0 [atl1e] >> > [] ? trace_hardirqs_off+0xd/0x10 >> > [] handle_irq_event_percpu+0x56/0x390 >> > [] handle_irq_event+0x3d/0x60 >> > [] handle_fasteoi_irq+0x5a/0x100 >> > [] handle_irq+0xbf/0x150 >> > [] ? file_sb_list_del+0x3f/0x50 >> > [] ? irq_enter+0x50/0xa0 >> > [] do_IRQ+0x4d/0xc0 >> > [] ? file_sb_list_del+0x3f/0x50 >> > [] common_interrupt+0x72/0x72 >> > [] ? lock_release+0xc2/0x310 >> > [] lg_local_unlock_cpu+0x24/0x50 >> > [] file_sb_list_del+0x3f/0x50 >> > [] fput+0x2d/0xc0 >> > [] filp_close+0x61/0x90 >> > [] __close_fd+0x8d/0x150 >> > [] sys_close+0x20/0x50 >> > [] system_call_fastpath+0x16/0x1b >> > >> > The usual straighforward failure to check for dma_mapping_error after a map >> > operation is completed. >> > >> > This patch should fix it, the reporter wandered off after filing this bz: >> > https://bugzilla.redhat.com/show_bug.cgi?id=954170 >> > >> > and I don't have hardware to test, but the fix is pretty straightforward, so I >> > figured I'd post it for review. >> > >> > Signed-off-by: Neil Horman >> > CC: Jay Cliburn >> > CC: Chris Snook >> > CC: "David S. Miller" >> > --- >> > drivers/net/ethernet/atheros/atl1e/atl1e_main.c | 26 +++++++++++++++++++++++-- >> > 1 file changed, 24 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >> > index 895f537..53a725b 100644 >> > --- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >> > +++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c >> > @@ -1665,7 +1665,7 @@ check_sum: >> > return 0; >> > } >> > >> > -static void atl1e_tx_map(struct atl1e_adapter *adapter, >> > +static int atl1e_tx_map(struct atl1e_adapter *adapter, >> > struct sk_buff *skb, struct atl1e_tpd_desc *tpd) >> > { >> > struct atl1e_tpd_desc *use_tpd = NULL; >> > @@ -1677,6 +1677,7 @@ static void atl1e_tx_map(struct atl1e_adapter *adapter, >> > u16 nr_frags; >> > u16 f; >> > int segment; >> > + int ring_start = adapter->tx_ring.next_to_use; >> > >> > nr_frags = skb_shinfo(skb)->nr_frags; >> > segment = (tpd->word3 >> TPD_SEGMENT_EN_SHIFT) & TPD_SEGMENT_EN_MASK; >> > @@ -1689,6 +1690,9 @@ static void atl1e_tx_map(struct atl1e_adapter *adapter, >> > tx_buffer->length = map_len; >> > tx_buffer->dma = pci_map_single(adapter->pdev, >> > skb->data, hdr_len, PCI_DMA_TODEVICE); >> > + if (dma_mapping_error(&adapter->pdev->dev, tx_buffer->dma)) >> > + return -ENOSPC; >> > + >> > ATL1E_SET_PCIMAP_TYPE(tx_buffer, ATL1E_TX_PCIMAP_SINGLE); >> > mapped_len += map_len; >> > use_tpd->buffer_addr = cpu_to_le64(tx_buffer->dma); >> > @@ -1715,6 +1719,13 @@ static void atl1e_tx_map(struct atl1e_adapter *adapter, >> > tx_buffer->dma = >> > pci_map_single(adapter->pdev, skb->data + mapped_len, >> > map_len, PCI_DMA_TODEVICE); >> > + >> > + if (dma_mapping_error(&adapter->pdev->dev, tx_buffer->dma)) { >> > + /* Reset the tx rings next pointer */ >> > + adapter->tx_ring.next_to_use = ring_start; >> > + return -ENOSPC; >> > + } >> > + >> > ATL1E_SET_PCIMAP_TYPE(tx_buffer, ATL1E_TX_PCIMAP_SINGLE); >> > mapped_len += map_len; >> > use_tpd->buffer_addr = cpu_to_le64(tx_buffer->dma); >> > @@ -1750,6 +1761,13 @@ static void atl1e_tx_map(struct atl1e_adapter *adapter, >> > (i * MAX_TX_BUF_LEN), >> > tx_buffer->length, >> > DMA_TO_DEVICE); >> > + >> > + if (dma_mapping_error(&adapter->pdev->dev, tx_buffer->dma)) { >> > + /* Reset the ring next to use pointer */ >> > + adapter->tx_ring.next_to_use = ring_start; >> > + return -ENOSPC; >> > + } >> > + >> > ATL1E_SET_PCIMAP_TYPE(tx_buffer, ATL1E_TX_PCIMAP_PAGE); >> > use_tpd->buffer_addr = cpu_to_le64(tx_buffer->dma); >> > use_tpd->word2 = (use_tpd->word2 & (~TPD_BUFLEN_MASK)) | >> > @@ -1767,6 +1785,7 @@ static void atl1e_tx_map(struct atl1e_adapter *adapter, >> > /* The last buffer info contain the skb address, >> > so it will be free after unmap */ >> > tx_buffer->skb = skb; >> > + return 0; >> >> Looks good to me, except from return statement in a void function. >> >> Cheers, >> -- >> Luis >> > Actually scratch that last note, I don't see any return statements in fuctions > that return void. I modified atl1e_tx_map to return an int so we could do the > right thing in atl1e_xmit_frame. Is there something I'm missing? > Neil Ups, you're right. Sorry for the noise. So, just as a suggestion, maybe your patch should also change atl1e_xmit_frame to do something with the new return code (maybe return NETDEV_TX_BUSY...) Cheers, -- Luis