All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process
@ 2014-02-18  1:10 Andrea Merello
  2014-02-18  1:10 ` [PATCH 1/7] rtl818x: Explicitly enable contetion window Andrea Merello
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	Andrea Merello

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/7] rtl818x: Explicitly enable contetion window
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  2014-02-18  1:10 ` [PATCH 2/7] rtl818x: pci_iomap() should pair with pci_iounmap() Andrea Merello
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

From: andrea merello <andrea.merello@gmail.com>

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 <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/7] rtl818x: pci_iomap() should pair with pci_iounmap()
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
  2014-02-18  1:10 ` [PATCH 1/7] rtl818x: Explicitly enable contetion window Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  2014-02-18  1:10 ` [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring Andrea Merello
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

From: andrea merello <andrea.merello@gmail.com>

Currently the driver uses pci_iomap() but iounmap() is called in
the error path

Change to use pci_iounmap() instead.

Reported-by: Huqiu Liu <liuhq11@mails.tsinghua.edu.cn>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
  2014-02-18  1:10 ` [PATCH 1/7] rtl818x: Explicitly enable contetion window Andrea Merello
  2014-02-18  1:10 ` [PATCH 2/7] rtl818x: pci_iomap() should pair with pci_iounmap() Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  2014-02-18  9:31   ` Dan Carpenter
  2014-02-18  1:10 ` [PATCH 4/7] rtl818x: make dev_alloc_skb() null pointer check to really work Andrea Merello
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

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.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/7] rtl818x: make dev_alloc_skb() null pointer check to really work
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
                   ` (2 preceding siblings ...)
  2014-02-18  1:10 ` [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  2014-02-18  1:10 ` [PATCH 5/7] rtl818x: add comments to explain few not obvious HW configs Andrea Merello
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

From: andrea merello <andrea.merello@gmail.com>

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 <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 5/7] rtl818x: add comments to explain few not obvious HW configs.
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
                   ` (3 preceding siblings ...)
  2014-02-18  1:10 ` [PATCH 4/7] rtl818x: make dev_alloc_skb() null pointer check to really work Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  2014-02-18  1:10 ` [PATCH 6/7] rtl818x: Make sure the TX descriptor "valid" flag is written by last Andrea Merello
  2014-02-18  1:10 ` [PATCH 7/7] rtl818x: make sure TX descriptor writes are done before kicking DMA Andrea Merello
  6 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

From: andrea merello <andrea.merello@gmail.com>

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 <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 6/7] rtl818x: Make sure the TX descriptor "valid" flag is written by last
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
                   ` (4 preceding siblings ...)
  2014-02-18  1:10 ` [PATCH 5/7] rtl818x: add comments to explain few not obvious HW configs Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  2014-02-18  1:10 ` [PATCH 7/7] rtl818x: make sure TX descriptor writes are done before kicking DMA Andrea Merello
  6 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

From: andrea merello <andrea.merello@gmail.com>

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 <dan.carpenter@oracle.com>
Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 7/7] rtl818x: make sure TX descriptor writes are done before kicking DMA
  2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
                   ` (5 preceding siblings ...)
  2014-02-18  1:10 ` [PATCH 6/7] rtl818x: Make sure the TX descriptor "valid" flag is written by last Andrea Merello
@ 2014-02-18  1:10 ` Andrea Merello
  6 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-18  1:10 UTC (permalink / raw)
  To: linville
  Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, liuhq11,
	andrea merello

From: andrea merello <andrea.merello@gmail.com>

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 <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18  1:10 ` [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring Andrea Merello
@ 2014-02-18  9:31   ` Dan Carpenter
  2014-02-18 18:05     ` Andrea Merello
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2014-02-18  9:31 UTC (permalink / raw)
  To: Andrea Merello; +Cc: linville, linux-wireless, Larry.Finger, bernhard, liuhq11

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18  9:31   ` Dan Carpenter
@ 2014-02-18 18:05     ` Andrea Merello
  2014-02-18 18:27       ` Dan Carpenter
  0 siblings, 1 reply; 17+ messages in thread
From: Andrea Merello @ 2014-02-18 18:05 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 18:05     ` Andrea Merello
@ 2014-02-18 18:27       ` Dan Carpenter
  2014-02-18 19:06         ` Andrea Merello
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2014-02-18 18:27 UTC (permalink / raw)
  To: Andrea Merello
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 18:27       ` Dan Carpenter
@ 2014-02-18 19:06         ` Andrea Merello
  2014-02-18 19:22           ` Dan Carpenter
  0 siblings, 1 reply; 17+ messages in thread
From: Andrea Merello @ 2014-02-18 19:06 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 19:06         ` Andrea Merello
@ 2014-02-18 19:22           ` Dan Carpenter
  2014-02-18 20:17             ` Andrea Merello
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2014-02-18 19:22 UTC (permalink / raw)
  To: Andrea Merello
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 19:22           ` Dan Carpenter
@ 2014-02-18 20:17             ` Andrea Merello
  2014-02-18 20:36               ` Dan Carpenter
  0 siblings, 1 reply; 17+ messages in thread
From: Andrea Merello @ 2014-02-18 20:17 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 20:17             ` Andrea Merello
@ 2014-02-18 20:36               ` Dan Carpenter
  2014-02-18 22:10                 ` Andrea Merello
  0 siblings, 1 reply; 17+ messages in thread
From: Dan Carpenter @ 2014-02-18 20:36 UTC (permalink / raw)
  To: Andrea Merello
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 20:36               ` Dan Carpenter
@ 2014-02-18 22:10                 ` Andrea Merello
  2014-02-22 16:57                   ` [PATCH 3/7 V2] " Andrea Merello
  0 siblings, 1 reply; 17+ messages in thread
From: Andrea Merello @ 2014-02-18 22:10 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: John Linville, Linux Wireless List, Larry Finger,
	Bernhard Schiffner, Huqiu Liu

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
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 3/7 V2] rtl818x: check for pci_map_single() success when initializing RX ring
  2014-02-18 22:10                 ` Andrea Merello
@ 2014-02-22 16:57                   ` Andrea Merello
  0 siblings, 0 replies; 17+ messages in thread
From: Andrea Merello @ 2014-02-22 16:57 UTC (permalink / raw)
  To: dan.carpenter, linville
  Cc: linux-wireless, Larry.Finger, bernhard, andrea merello

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 adds check for this condition and eventually causes the
driver not to initialize, avoiding at least dangerous DMAs.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
---
 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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2014-02-22 16:56 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-18  1:10 [PATCH 0/7] rtl818x: second bunch of fixes ported from rtl8187se merge process Andrea Merello
2014-02-18  1:10 ` [PATCH 1/7] rtl818x: Explicitly enable contetion window Andrea Merello
2014-02-18  1:10 ` [PATCH 2/7] rtl818x: pci_iomap() should pair with pci_iounmap() Andrea Merello
2014-02-18  1:10 ` [PATCH 3/7] rtl818x: check for pci_map_single() success when initializing RX ring Andrea Merello
2014-02-18  9:31   ` Dan Carpenter
2014-02-18 18:05     ` Andrea Merello
2014-02-18 18:27       ` Dan Carpenter
2014-02-18 19:06         ` Andrea Merello
2014-02-18 19:22           ` Dan Carpenter
2014-02-18 20:17             ` Andrea Merello
2014-02-18 20:36               ` Dan Carpenter
2014-02-18 22:10                 ` Andrea Merello
2014-02-22 16:57                   ` [PATCH 3/7 V2] " Andrea Merello
2014-02-18  1:10 ` [PATCH 4/7] rtl818x: make dev_alloc_skb() null pointer check to really work Andrea Merello
2014-02-18  1:10 ` [PATCH 5/7] rtl818x: add comments to explain few not obvious HW configs Andrea Merello
2014-02-18  1:10 ` [PATCH 6/7] rtl818x: Make sure the TX descriptor "valid" flag is written by last Andrea Merello
2014-02-18  1:10 ` [PATCH 7/7] rtl818x: make sure TX descriptor writes are done before kicking DMA Andrea Merello

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.