| Message ID | 1392685846-10116-4-git-send-email-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Feb 18, 2014 at 02:10:42AM +0100, Andrea Merello wrote: > From: andrea merello <andrea.merello@gmail.com> > > During initialization a number of RX skbs are allocated and mapped > for DMA. > Currently if pci_map_single() fails, it will result in passing to the > HW a wrong DMA address (to write to!). > > This patch detects this condition, eventually it tries to get a more > luky skb, and if it finally fails to get things right, it makes the > driver not to initialize, avoiding at least dangerous DMAs. > In normal life then why would pci_map_single() fail even once? (I am a newbie with this code so it is an honest question and not a rhetorical one). regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Honestly I'm not so expert in deep on this subject me too (and I would like to learn more me too). I have not a detailed example right now, but consider that this is arch dependent thing, and we should think to wide plethora of archs.. even future ones... AFAIK on x86 the dma error checking function will do nothing (this arch uses nommu_dma_ops function pointers collection for implementing those kind of DMA operations, and that structure does not even assigns the dma error checking function pointer), but on other archs it may do something. Maybe in some cases for example a bounce-buffer may be needed (give a look to /lib/swiotlb.c), and I suppose it can't be guaranteed it can be successfully allocated. Looking at the kernel source it seems that, for example, ARM64 may need this. And, if I understood code right (and my flu does not help), it seems that even on x64 arch, if our board is the "sta2x11" (that I doesn't know what exactly is) then the dma operations are changed to the ones in swiotlb.c ... If someone more expert on that topic can give us better or more complete explanation, I will appreciate it very much (in true I wrote this mail with the purpose of trying to trigger some of them :) ) Andrea On Tue, Feb 18, 2014 at 10:31 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Tue, Feb 18, 2014 at 02:10:42AM +0100, Andrea Merello wrote: >> From: andrea merello <andrea.merello@gmail.com> >> >> During initialization a number of RX skbs are allocated and mapped >> for DMA. >> Currently if pci_map_single() fails, it will result in passing to the >> HW a wrong DMA address (to write to!). >> >> This patch detects this condition, eventually it tries to get a more >> luky skb, and if it finally fails to get things right, it makes the >> driver not to initialize, avoiding at least dangerous DMAs. >> > > In normal life then why would pci_map_single() fail even once? (I am > a newbie with this code so it is an honest question and not a rhetorical > one). > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I guess what I was going to suggest is that you are overthinking the error handling. Your code tries very hard to allocate 32 buffers. But I bet it's just a matter of allocating the buffers and if it doesn't work then clean up and return an error. Probably it alocates 32 buffers successfully or it doesn't allocate any, it's less likely that we will allocate a partial number of buffers. If this were in the receive path or something like that then maybe we would worry about situations like that. Although even in that case probably the right approach would be to drop everything and let the protocol handle the error... Offtopic. What I mean is that this is in the init path so everything will likely work except in manufactured situations. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thank for pointing out this. Yes, I probably exaggerated trying hard to allocate memory. If your point is about if pci_map_single may or not ever fail in real life (that is was I thought you meant in your first mail), then I think that it's worth to keep the check anyway (I think it could happen). If your point is that it can be not so much useful to try and retry hard the allocation (I must admit I'm not even sure that after freeing the skb the kernel will not likely re-allocate the same one at the next try), then I will keep the error check, but I can remove code that retries allocation (failing at the first error as you suggested). BTW consider that the allocation is done in ieee80211 start callback, that is called every time the interface is brought down/up, and not once at the begining. On one hand I cared avoiding wasting memory when the interface is not up. On the other hand I had thought also to move memory allocation in initialization, exactly to increase success probability as you pointed out.. I chosen the first option because after the interface is brought up, the RX ant TX processes will start to perform a lot of other skb allocations and dma maps. If we assume, as you pointed out, they will likely ALL fail or succeeds, not just few of them, then probably even if we allocated some memory at init time, we have gained not advantage. Do you agree ? Thank you Andrea On Tue, Feb 18, 2014 at 7:27 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > I guess what I was going to suggest is that you are overthinking the > error handling. Your code tries very hard to allocate 32 buffers. But > I bet it's just a matter of allocating the buffers and if it doesn't > work then clean up and return an error. Probably it alocates 32 buffers > successfully or it doesn't allocate any, it's less likely that we will > allocate a partial number of buffers. > > If this were in the receive path or something like that then maybe we > would worry about situations like that. Although even in that case > probably the right approach would be to drop everything and let the > protocol handle the error... Offtopic. What I mean is that this is in > the init path so everything will likely work except in manufactured > situations. > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 18, 2014 at 08:06:51PM +0100, Andrea Merello wrote: > Thank for pointing out this. Yes, I probably exaggerated trying hard > to allocate memory. > > If your point is about if pci_map_single may or not ever fail in real > life (that is was I thought you meant in your first mail), then I > think that it's worth to keep the check anyway (I think it could > happen). Please keep the check. > > If your point is that it can be not so much useful to try and retry > hard the allocation (I must admit I'm not even sure that after freeing > the skb the kernel will not likely re-allocate the same one at the > next try), then I will keep the error check, but I can remove code > that retries allocation (failing at the first error as you suggested). Yes. Please remove the retry code unless there is some real life justification for it where you have seen partial allocations before. > > BTW consider that the allocation is done in ieee80211 start callback, > that is called every time the interface is brought down/up, and not > once at the begining. > You are right. I didn't realize that. > On one hand I cared avoiding wasting memory when the interface is not up. > On the other hand I had thought also to move memory allocation in > initialization, exactly to increase success probability as you pointed > out.. > > I chosen the first option because after the interface is brought up, > the RX ant TX processes will start to perform a lot of other skb > allocations and dma maps. > If we assume, as you pointed out, they will likely ALL fail or > succeeds, not just few of them, then probably even if we allocated > some memory at init time, we have gained not advantage. > > Do you agree ? Yes. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ok, I will fix my patch in this way. I will resend it. Should I also rebase following patches in the patch serie and resent them also? Thanks Andrea On Tue, Feb 18, 2014 at 8:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Tue, Feb 18, 2014 at 08:06:51PM +0100, Andrea Merello wrote: >> Thank for pointing out this. Yes, I probably exaggerated trying hard >> to allocate memory. >> >> If your point is about if pci_map_single may or not ever fail in real >> life (that is was I thought you meant in your first mail), then I >> think that it's worth to keep the check anyway (I think it could >> happen). > > Please keep the check. > >> >> If your point is that it can be not so much useful to try and retry >> hard the allocation (I must admit I'm not even sure that after freeing >> the skb the kernel will not likely re-allocate the same one at the >> next try), then I will keep the error check, but I can remove code >> that retries allocation (failing at the first error as you suggested). > > Yes. Please remove the retry code unless there is some real life > justification for it where you have seen partial allocations before. > >> >> BTW consider that the allocation is done in ieee80211 start callback, >> that is called every time the interface is brought down/up, and not >> once at the begining. >> > > You are right. I didn't realize that. > >> On one hand I cared avoiding wasting memory when the interface is not up. >> On the other hand I had thought also to move memory allocation in >> initialization, exactly to increase success probability as you pointed >> out.. >> >> I chosen the first option because after the interface is brought up, >> the RX ant TX processes will start to perform a lot of other skb >> allocations and dma maps. >> If we assume, as you pointed out, they will likely ALL fail or >> succeeds, not just few of them, then probably even if we allocated >> some memory at init time, we have gained not advantage. >> >> Do you agree ? > > Yes. > > regards, > dan carpenter > > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 18, 2014 at 09:17:22PM +0100, Andrea Merello wrote: > Ok, I will fix my patch in this way. > I will resend it. > Should I also rebase following patches in the patch serie and resent them also? > Don't rebase if you don't have to. Do you know how to use the In-Reply-To header so that the v2 patch will show up in the thread? There is an --in-reply-to option in git send-email. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
No, I didn't know about that.. I will give a look to git documentation.. Thank you for telling me about it :) Andrea On Tue, Feb 18, 2014 at 9:36 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Tue, Feb 18, 2014 at 09:17:22PM +0100, Andrea Merello wrote: >> Ok, I will fix my patch in this way. >> I will resend it. >> Should I also rebase following patches in the patch serie and resent them also? >> > > Don't rebase if you don't have to. > > Do you know how to use the In-Reply-To header so that the v2 patch will > show up in the thread? There is an --in-reply-to option in git > send-email. > > regards, > dan carpenter > -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c index bf59ff9..848ea59 100644 --- a/drivers/net/wireless/rtl818x/rtl8180/dev.c +++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c @@ -458,6 +458,7 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw *dev) { struct rtl8180_priv *priv = dev->priv; struct rtl8180_rx_desc *entry; + int dma_map_fail_count = 0; int i; priv->rx_ring = pci_alloc_consistent(priv->pdev, @@ -483,6 +484,19 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw *dev) mapping = (dma_addr_t *)skb->cb; *mapping = pci_map_single(priv->pdev, skb_tail_pointer(skb), MAX_RX_SIZE, PCI_DMA_FROMDEVICE); + + if (pci_dma_mapping_error(priv->pdev, *mapping)) { + kfree_skb(skb); + if (dma_map_fail_count++ > 32) { + wiphy_err(dev->wiphy, "Cannot map DMA for RX skb\n"); + return -ENOMEM; + } + + i--; + continue; + } + + entry->rx_buf = cpu_to_le32(*mapping); entry->flags = cpu_to_le32(RTL818X_RX_DESC_FLAG_OWN | MAX_RX_SIZE);