While working on rtl818x driver for adding rtl8187se support I have
made some fixes and improvements unrelated to rtl8187se support itself.
In this second patch set I include the following seven changes:
andrea merello (7):
rtl818x: Explicitly enable contetion window
rtl818x: pci_iomap() should pair with pci_iounmap()
rtl818x: check for pci_map_single() success when initializing RX ring
rtl818x: make dev_alloc_skb() null pointer check to really work
rtl818x: add comments to explain few not obvious HW configs
rtl818x: Make sure the TX descriptor "valid" flag is written by last
rtl818x: make sure TX descriptor writes are done before kicking DMA
drivers/net/wireless/rtl818x/rtl8180/dev.c | 48 +++++++++++++++++++++++++++---
1 file changed, 44 insertions(+), 4 deletions(-)
--
1.8.3.2
From: andrea merello <[email protected]>
The TX descriptors are consumed by the HW using DMA.
Even if in the driver code the TX descriptor writes appears before
the HW "dma kick" register writes, the CPU may reorder them.
If this happens, the TX may not happen at all becase the "valid"
descriptor flag may have not been set yet.
This patch adds a write memory barrier to ensures the TX
descriptor is written before writing to the HW "dma kick" register.
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index 7027cb7..7ce58d2 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -341,6 +341,12 @@ static void rtl8180_tx(struct ieee80211_hw *dev,
*/
wmb();
entry->flags = cpu_to_le32(tx_flags);
+ /* We must be sure this has been written before followings HW
+ * register write, because this write will made the HW attempts
+ * to DMA the just-written data
+ */
+ wmb();
+
__skb_queue_tail(&ring->queue, skb);
if (ring->entries - skb_queue_len(&ring->queue) < 2)
ieee80211_stop_queue(dev, prio);
--
1.8.3.2
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 <[email protected]> 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
>
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
<[email protected]> wrote:
> On Tue, Feb 18, 2014 at 02:10:42AM +0100, Andrea Merello wrote:
>> From: andrea merello <[email protected]>
>>
>> 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
>
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
On Tue, Feb 18, 2014 at 02:10:42AM +0100, Andrea Merello wrote:
> From: andrea merello <[email protected]>
>
> 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
From: andrea merello <[email protected]>
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 adds check for this condition and eventually causes the
driver not to initialize, avoiding at least dangerous DMAs.
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index bf59ff9..0102da2 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -483,6 +483,13 @@ 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);
+ wiphy_err(dev->wiphy, "Cannot map DMA for RX skb\n");
+ return -ENOMEM;
+ }
+
entry->rx_buf = cpu_to_le32(*mapping);
entry->flags = cpu_to_le32(RTL818X_RX_DESC_FLAG_OWN |
MAX_RX_SIZE);
--
1.8.3.2
From: andrea merello <[email protected]>
The TX descriptors are consumed by the HW using DMA.
Even if in the driver code the memory write that sets the "valid"
flag appears after all other writes, the CPU may reorder writes,
causing the HW to consider as valid a not-fully-written yet
descriptor.
This may cause HW incorrect behaviour.
This can happen because (AFAIK) the HW may attempt DMA
asynchronously without waiting to be kicked by the following
register write.
This patch adds a write memory barrier to enforce writes ordering.
Reported-by: Dan Carpenter <[email protected]>
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index be9a8a1..7027cb7 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -335,7 +335,11 @@ static void rtl8180_tx(struct ieee80211_hw *dev,
entry->flags2 = info->control.rates[1].idx >= 0 ?
ieee80211_get_alt_retry_rate(dev, info, 0)->bitrate << 4 : 0;
entry->retry_limit = info->control.rates[0].count;
+
+ /* We must be sure that tx_flags is written last because the HW
+ * looks at it to check if the rest of data is valid or not
+ */
+ wmb();
entry->flags = cpu_to_le32(tx_flags);
__skb_queue_tail(&ring->queue, skb);
if (ring->entries - skb_queue_len(&ring->queue) < 2)
--
1.8.3.2
From: andrea merello <[email protected]>
Certain HW options (TX packet retry count, CW configuration and
TX power configuration) can be specified in both the TX packet
descriptor and also into HW "global" registers.
The HW is thus configured to honour the global register or the
TX descriptor field depending by the case.
This patch adds few comments that hopefully clarify in which cases
the driver uses one method and in which cases it uses the other.
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index cb97380..be9a8a1 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -634,11 +634,23 @@ static int rtl8180_start(struct ieee80211_hw *dev)
if (priv->r8185) {
reg = rtl818x_ioread8(priv, &priv->map->CW_CONF);
+
+ /* CW is not on per-packet basis.
+ * in rtl8185 the CW_VALUE reg is used.
+ */
reg &= ~RTL818X_CW_CONF_PERPACKET_CW;
+ /* retry limit IS on per-packet basis.
+ * the short and long retry limit in TX_CONF
+ * reg are ignored
+ */
reg |= RTL818X_CW_CONF_PERPACKET_RETRY;
rtl818x_iowrite8(priv, &priv->map->CW_CONF, reg);
reg = rtl818x_ioread8(priv, &priv->map->TX_AGC_CTL);
+ /* TX antenna and TX gain are not on per-packet basis.
+ * TX Antenna is selected by ANTSEL reg (RX in BB regs).
+ * TX gain is selected with CCK_TX_AGC and OFDM_TX_AGC regs
+ */
reg &= ~RTL818X_TX_AGC_CTL_PERPACKET_GAIN;
reg &= ~RTL818X_TX_AGC_CTL_PERPACKET_ANTSEL;
reg |= RTL818X_TX_AGC_CTL_FEEDBACK_ANT;
--
1.8.3.2
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 <[email protected]> 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
>
>
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 <[email protected]> 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
>
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
From: andrea merello <[email protected]>
Currently the contention window enable/disable HW flag is not
touched by the driver.
This patch explicitly set it to the correct value to make sure
contention window is enabled (AFAIK contention window must be
enabled in most (if not all) cases.
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index 7980ab1..470a1e3 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -642,6 +642,8 @@ static int rtl8180_start(struct ieee80211_hw *dev)
else
reg &= ~RTL818X_TX_CONF_HW_SEQNUM;
+ reg &= ~RTL818X_TX_CONF_DISCW;
+
/* different meaning, same value on both rtl8185 and rtl8180 */
reg &= ~RTL818X_TX_CONF_SAT_HWPLCP;
--
1.8.3.2
From: andrea merello <[email protected]>
During driver initialization, some skbs are preallocated for RX.
Currenly if the allocation fails, the driver's allocation routine
exits immediatly but it will return zero (success) anyway.
In this way the driver will continue initialization with buggy
pointers around.
This patch makes the driver's allocation routine to return
an error value and to print a complaint message when skb allocation
fails.
In this way its caller will not go further, avoinding the driver to
successfully load, and preventing dereferencing buggy pointers.
An hint is thus printed about why the driver failed.
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index 848ea59..cb97380 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -477,9 +477,10 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw *dev)
struct sk_buff *skb = dev_alloc_skb(MAX_RX_SIZE);
dma_addr_t *mapping;
entry = &priv->rx_ring[i];
- if (!skb)
- return 0;
-
+ if (!skb) {
+ wiphy_err(dev->wiphy, "Cannot allocate RX skb\n");
+ return -ENOMEM;
+ }
priv->rx_buf[i] = skb;
mapping = (dma_addr_t *)skb->cb;
*mapping = pci_map_single(priv->pdev, skb_tail_pointer(skb),
--
1.8.3.2
From: andrea merello <[email protected]>
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.
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
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);
--
1.8.3.2
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
From: andrea merello <[email protected]>
Currently the driver uses pci_iomap() but iounmap() is called in
the error path
Change to use pci_iounmap() instead.
Reported-by: Huqiu Liu <[email protected]>
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/rtl818x/rtl8180/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index 470a1e3..bf59ff9 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -1137,7 +1137,7 @@ static int rtl8180_probe(struct pci_dev *pdev,
return 0;
err_iounmap:
- iounmap(priv->map);
+ pci_iounmap(pdev, priv->map);
err_free_dev:
ieee80211_free_hw(dev);
--
1.8.3.2