Re: [PATCH MOREWORK 14/19] rtl818x_pci: Fix a memory leak in rtl8180_init_rx_ring

[Andrea Merello]

On Mon, Apr 11, 2016 at 2:11 AM, Julian Calaby <julian.cal...@gmail.com> wrote:
> Hi Andrea,
>
> On Sat, Apr 9, 2016 at 4:56 AM, Andrea Merello <andrea.mere...@gmail.com> 
> wrote:
>>
>> On Fri, Mar 18, 2016 at 3:27 AM, Julian Calaby <julian.cal...@gmail.com>
>> wrote:
>>> From: Jia-Ju Bai <baijiaju1...@163.com>
>>>
>>> When dev_alloc_skb or pci_dma_mapping_error in rtl8180_init_rx_ring fails,
>>> the memory allocated by pci_zalloc_consistent is not freed.
>>>
>>> This patch fixes the bug by adding pci_free_consistent
>>> in error handling code.
>>>
>>> Signed-off-by: Jia-Ju Bai <baijiaju1...@163.com>
>>> Signed-off-by: Julian Calaby <julian.cal...@gmail.com>
>>>
>>> ---
>>>
>>> Could someone else please review this?
>>
>> This patch does address one leak, but the piece of code it changes is
>> totally leaky and IMHO it probably needs to be reworked more deeply, maybe
>> in a single shot. This is why I didn't acked the patch.
>
> Ugh.
>
> I can't work on this as I don't have hardware to test it with, could
> you (or someone who does) please have a good look at this section of
> the Realtek drivers (I see a lot of this pattern repeated in them) and
> craft (a) patch(es) to fix this properly?

On my TODO list. BTW Unfortunately I have a huge TODO list, and my
WiFi test box is currently unusable, so it will probably not happen
very soon on my side :(

>> BTW if you feel that this could be better than nothing, please feel free to
>> apply it :).
>
> IMHO this patch makes things better in that there's less options for a
> leak, however I marked this as "morework" as it clearly doesn't solve
> the _entire_ problem.
>
> I feel it should be applied as it does make it better, however this
> isn't my driver and I'd appreciate a nod of approval from someone who
> knows it better than I do.

I don't consider myself one that should finally decide about what to
merge into the Kernel, BTW I don't see any drawback in merging it.

>>> In particular, immediately after the call to pci_zalloc_coherent(), it
>>> fails if the result is null or if '(unsigned long)result & 0xFF'.
>>>
>>> Is the second arm of the or statement possible, and if so, should we be
>>> pci_free_coherent()ing the result there too?
>>
>> I honestly don't know if the coherent allocator is supposed to always return
>> page-aligned memory areas (I'm assuming a page wouldn't be smaller than 256
>> bytes); AFAIK no one ever hit that.
>>
>> If there are cases/archs where it can  really happen, then allocating an
>> oversized memory area, and providing to the HW an aligned pointer inside
>> that area, may be the way to go (IIRC calling pci_set_consistent_dma_mask()
>> with 0xFF00 does not provides any benefit, but again, I'm not sure).
>
> I believe that calling *_set_*_dma_mask() is how it's supposed to be
> done. If this isn't working, then that's a bug elsewhere. IMHO drivers
> should be able to say "we need a DMA address like *this*" and it's the
> PCI / DMA code's responsibility to either provide a compatible one or
> fail. Drivers shouldn't be working around bugs in the services they
> use.
>

I didn't meant it didn't worked.. I never thought *_set_*_dma_mask()
to be buggy.. (and Indeed I think I never saw a unaligned DMA alloc
either with or without it).

Well, to make it short: IIRC, I used it in my very first version of
the driver, but it has been changed in the rewritten version that is
in-kernel right now. At that time IIRC I ended up in thinking that
*_set_*_dma_mask() was just meant (due to its semantic, no bug..) only
for the purpose of restricting allocations to lower memory (for boards
that cannot address full memory size), while it wouldn't care about
LSBs. I don't remember if I was told about this, or if I looked into
the kernel code (maybe I did some mistake). BTW if you know that it is
intended to works as you said (and that indeed seems absolutely
reasonable to me, given that it takes a "mask" parameter), then it's
obviously the way to go.

After a quick grep in the kernel, it seems that there is at least one
driver that actually specifies a "weird" mask like ours..

>> Either way, If we do that, or if we can trust that it does never really
>> happen, then we can drop the check (or maybe just change it in a BUG_ON()
>> just to be paranoid).
>
> A WARN_ON() and failing gracefully would be the ideal way to do this
> IMHO, however it really shouldn't be necessary.
>
>> To address your question - if we let this check stay there - then of course
>> we need to take 

Re: [PATCH resend] rtl818x_pci: Fix a memory leak in rtl8180_init_rx_ring

[Andrea Merello]

Thanks for pointing this out!

At a first look I'd propose to merge the two identical
pci_fee_consistent() in a single one, and place it in an error exit
path at the end of function.

BTW, looking at the code, it seems there is another leak here that
your patch does not address: we still leaks allocated (and dma-mapped)
skbs.

Indeed we probably need to rework error handling in this piece of code..

Andrea

On Sat, Jan 16, 2016 at 2:07 PM, Jia-Ju Bai  wrote:
> When dev_alloc_skb or pci_dma_mapping_error in rtl8180_init_rx_ring fails,
> the memory allocated by pci_zalloc_consistent is not freed.
>
> This patch fixes the bug by adding pci_free_consistent
> in error handling code.
>
> Signed-off-by: Jia-Ju Bai 
> ---
>  drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c |4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c 
> b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
> index a43a16f..28479b1 100644
> --- a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
> +++ b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
> @@ -1018,6 +1018,8 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw 
> *dev)
> dma_addr_t *mapping;
> entry = priv->rx_ring + priv->rx_ring_sz*i;
> if (!skb) {
> +   pci_free_consistent(priv->pdev, priv->rx_ring_sz * 32,
> +   priv->rx_ring, priv->rx_ring_dma);
> wiphy_err(dev->wiphy, "Cannot allocate RX skb\n");
> return -ENOMEM;
> }
> @@ -1028,6 +1030,8 @@ static int rtl8180_init_rx_ring(struct ieee80211_hw 
> *dev)
>
> if (pci_dma_mapping_error(priv->pdev, *mapping)) {
> kfree_skb(skb);
> +   pci_free_consistent(priv->pdev, priv->rx_ring_sz * 32,
> +   priv->rx_ring, priv->rx_ring_dma);
> wiphy_err(dev->wiphy, "Cannot map DMA for RX skb\n");
> return -ENOMEM;
> }
> --
> 1.7.9.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rtl818x_pci: Disable pci device in error handling code

[Andrea Merello]

Acked-by: Andrea Merello <andrea.mere...@gmail.com>

On Sat, Jan 16, 2016 at 2:02 PM, Jia-Ju Bai <baijiaju1...@163.com> wrote:
> When pci_request_regions in rtl8180_probe fails, pci_disable_device is not
> called to disable the device which is enabled by pci_enbale_device.
>
> This patch fixes the problem by adding a new lable in error handling code.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1...@163.com>
> ---
>  drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c |4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c 
> b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
> index a43a16f..c76af5d 100644
> --- a/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
> +++ b/drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c
> @@ -1736,7 +1736,7 @@ static int rtl8180_probe(struct pci_dev *pdev,
> if (err) {
> printk(KERN_ERR "%s (rtl8180): Cannot obtain PCI resources\n",
>pci_name(pdev));
> -   return err;
> +   goto err_disable_dev;
> }
>
> io_addr = pci_resource_start(pdev, 0);
> @@ -1938,6 +1938,8 @@ static int rtl8180_probe(struct pci_dev *pdev,
>
>   err_free_reg:
> pci_release_regions(pdev);
> +
> + err_disable_dev:
> pci_disable_device(pdev);
> return err;
>  }
> --
> 1.7.9.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rtl818x_pci: fix response rate may be incorrect.

[Andrea Merello]

Currently the allowed respose rate set (rates for HW generated frames
like ACKs) is the same as the basic rate set.

The HW will use the higher allowed response rate that is lower than the
rate of the received frame.

This is more or less what IEEE80211 mandates, but I missed the fact
that IEEE80211 also says that whenever it happens that for a modulation
class there is no any rate in the basic rates set, then the response rate
set shall include also all the mandatory rates for that modulation class.

This patch adds mandatory OFDM rates to the allowed response rate set if
no OFDM rate is included in the basic rate set.

Depending by the AP, I faced cases in which this patch seems to cause a
noticeable perfomance improvement.

- With my usual test AP there is no particular perfomance difference.
- With a prism54/hostapd AP this patch causes RX thoughput increase from
  about 5Mbps to about 20Mbps.

Hopefully this patch may help people that faced performance regression wrt
the old staging driver.

Signed-off-by: Andrea Merello andrea.mere...@gmail.com
---
 drivers/net/wireless/rtl818x/rtl8180/dev.c | 36 +-
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/rtl818x/rtl8180/dev.c 
b/drivers/net/wireless/rtl818x/rtl8180/dev.c
index ded967a..706b844 100644
--- a/drivers/net/wireless/rtl818x/rtl8180/dev.c
+++ b/drivers/net/wireless/rtl818x/rtl8180/dev.c
@@ -742,35 +742,49 @@ static void rtl8180_int_disable(struct ieee80211_hw *dev)
 }
 
 static void rtl8180_conf_basic_rates(struct ieee80211_hw *dev,
-   u32 rates_mask)
+   u32 basic_mask)
 {
struct rtl8180_priv *priv = dev-priv;
-
-   u8 max, min;
u16 reg;
-
-   max = fls(rates_mask) - 1;
-   min = ffs(rates_mask) - 1;
+   u32 resp_mask;
+   u8 basic_max;
+   u8 resp_max, resp_min;
+
+   resp_mask = basic_mask;
+   /* IEEE80211 says the response rate should be equal to the highest basic
+* rate that is not faster than received frame. But it says also that if
+* the basic rate set does not contains any rate for the current
+* modulation class then mandatory rate set must be used for that
+* modulation class. Eventually add OFDM mandatory rates..
+*/
+   if ((resp_mask  0xf) == resp_mask)
+   resp_mask |= 0x150; /* 6, 12, 24Mbps */
 
switch (priv-chip_family) {
 
case RTL818X_CHIP_FAMILY_RTL8180:
/* in 8180 this is NOT a BITMAP */
+   basic_max = fls(basic_mask) - 1;
reg = rtl818x_ioread16(priv, priv-map-BRSR);
reg = ~3;
-   reg |= max;
+   reg |= basic_max;
rtl818x_iowrite16(priv, priv-map-BRSR, reg);
break;
 
case RTL818X_CHIP_FAMILY_RTL8185:
+   resp_max = fls(resp_mask) - 1;
+   resp_min = ffs(resp_mask) - 1;
/* in 8185 this is a BITMAP */
-   rtl818x_iowrite16(priv, priv-map-BRSR, rates_mask);
-   rtl818x_iowrite8(priv, priv-map-RESP_RATE, (max  4) | min);
+   rtl818x_iowrite16(priv, priv-map-BRSR, basic_mask);
+   rtl818x_iowrite8(priv, priv-map-RESP_RATE, (resp_max  4) |
+   resp_min);
break;
 
case RTL818X_CHIP_FAMILY_RTL8187SE:
-   /* in 8187se this is a BITMAP */
-   rtl818x_iowrite16(priv, priv-map-BRSR_8187SE, rates_mask);
+   /* in 8187se this is a BITMAP. BRSR reg actually sets
+* response rates.
+*/
+   rtl818x_iowrite16(priv, priv-map-BRSR_8187SE, resp_mask);
break;
}
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-wireless in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html