Hi Kalle,
This is a set of all patches in patchwork which were pending when the 4.6
merge window opened that meet the following criteria:
1. They're sane
2. They're either obviously correct, I can review them, or they've been
reviewed or ACK'd by someone else.
3. They don't require any code changes to make them applyable.
That said, if anyone doesn't like any of these patches for any reason,
just give me a reason and I'll drop it.
I've split the pending patches into 4 sets as detailed below. All patches
have been rebased to apply in order on wireless-drivers-next at
wireless-drivers-next-for-davem-2016-03-14.
Git tree: https://github.com/SkUrRiEr/wireless-drivers-pending
Tags are listed below. Note that the tags stack, so applying
pending-cleanup-detail-4.6 will get you all of pending-cleanup-4.6.
By "4.6" I mean these patches were deferred when 4.6's merge window
opened. As there's nothing urgent here, I expect all of them to be applied
to 4.7 at the earliest. If this is version convention is stupid or
confusing, I'll change it to match the version I expect it to be applied
to and re-send this cover letter.
1. Cleanup:
These are the patches that just got lost or forgotten about. Nothing here
should be controversial at all, the only changes required were re-writing
commit messages for Markus Elfring's patches and a couple of others.
There's also a b43 change that stalled waiting for someone to test it on
BCMA.
branch: pending-cleanup
tag: pending-cleanup-4.6
bundle url: http://patchwork.kernel.org/bundle/skurrier/pending-cleanup%204.6/
2. Detail:
Patches that required a detailed review. This is two patches from Markus
Elfring which removed many unnecessary variable initialisations. Dan
Carpenter baulked at reviewing these. I've rewritten the commit messages
to go into detail as to how and where the variables are used and am
confident they are correct.
branch: pending-cleanup-detail
tag: pending-cleanup-detail-4.6
bundle url: http://patchwork.kernel.org/bundle/skurrier/pending-cleanup-detail%204.6/
3. More Work:
Patches that either needed the good bits split out or only fix part of
the problem and need some followup. I've written notes for each change and
added a patch where the additional requirements were obvious.
branch: pending-cleanup-morework
tag: pending-cleanup-morework-4.6
bundle url: http://patchwork.kernel.org/bundle/skurrier/pending-cleanup-morework%204.6/
4. Controversial:
A patch from Markus Elfring which people hated, however it fits my
criteria. I don't care if it gets applied or not.
branch: pending-cleanup-controversial
tag: pending-cleanup-controversial-4.6
bundle url: http://patchwork.kernel.org/bundle/skurrier/pending-cleanup-controversial%204.6/
At this point, everything in patchwork that's deferred is either:
1. Unreviewable by me (I poked the authors of most of the older patches
yesterday)
2. An earlier version of a patch I picked up
3. Too "new" (less than a couple of months old)
(My appologies for the huge CC list, scripts/get_maintainer.pl --git can
be somewhat verbose and I'm sending this to everyone who's getting an
email as part of this series.)
Patches:
Byeoungwook Kim (1):
rtlwifi: Implement 'rtl_addr_delay()' with a switch statement
Geliang Tang (4):
ipw2x00: use to_pci_dev()
wlcore: use to_delayed_work()
wl1251: use to_delayed_work()
rtlwifi: use to_delayed_work()
Ivan Safonov (1):
ath9k: Remove unnecessary ?: operator
Jia-Ju Bai (5):
iwl4965: Fix a null pointer dereference in il_tx_queue_free and
il_cmd_queue_free
b43: Fix memory leaks in b43_bus_dev_ssb_init and
b43_bus_dev_bcma_init
rtl818x_pci: Disable pci device in error handling code
rtl818x_pci: Fix a memory leak in rtl8180_init_rx_ring
iwl4965: Fix a memory leak in error handling code of __il4965_up
Julian Calaby (1):
iwl4965: Fix more memory leaks in __il4965_up()
Markus Elfring (7):
ath9k_htc: Delete unnecessary variable initialisation
brcmfmac: Delete unnecessary variable initialisation
iwlegacy: Return directly if allocation fails in il_eeprom_init()
rsi: Delete unnecessary variable initialisation
rsi: Delete unnecessary variable initialisation
rsi: Move variable initialisation into error code
iwlegacy: Rename label in il_eeprom_init()
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 6 ++---
drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
drivers/net/wireless/broadcom/b43/main.c | 6 +++--
.../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
drivers/net/wireless/intel/ipw2x00/ipw2100.c | 2 +-
drivers/net/wireless/intel/iwlegacy/4965-mac.c | 3 +++
drivers/net/wireless/intel/iwlegacy/common.c | 26 +++++++++++++---------
drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c | 8 ++++++-
drivers/net/wireless/realtek/rtlwifi/core.c | 20 ++++++++++++-----
drivers/net/wireless/realtek/rtlwifi/wifi.h | 2 +-
drivers/net/wireless/rsi/rsi_91x_pkt.c | 22 +++++++++---------
drivers/net/wireless/ti/wl1251/ps.c | 2 +-
drivers/net/wireless/ti/wlcore/main.c | 10 ++++-----
drivers/net/wireless/ti/wlcore/ps.c | 2 +-
drivers/net/wireless/ti/wlcore/scan.c | 2 +-
15 files changed, 70 insertions(+), 45 deletions(-)
--
2.7.0
From: Byeoungwook Kim <[email protected]>
Doing it this way improves readability.
Signed-off-by: Byeoungwook Kim <[email protected]>
[Rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
... or does it.
Honesltly, this is only in this list as Byeoungwook asked about it and I
can see _some_ sort of reason why it exists.
Larry, if you don't like this, please NACK it and we'll never speak of it
again.
Thanks,
Julian Calaby
---
drivers/net/wireless/realtek/rtlwifi/core.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
index 0f48048..1cf367f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/core.c
+++ b/drivers/net/wireless/realtek/rtlwifi/core.c
@@ -53,18 +53,26 @@ EXPORT_SYMBOL(channel5g_80m);
void rtl_addr_delay(u32 addr)
{
- if (addr == 0xfe)
+ switch (addr) {
+ case 0xfe:
msleep(50);
- else if (addr == 0xfd)
+ break;
+ case 0xfd:
msleep(5);
- else if (addr == 0xfc)
+ break;
+ case 0xfc:
msleep(1);
- else if (addr == 0xfb)
+ break;
+ case 0xfb:
usleep_range(50, 100);
- else if (addr == 0xfa)
+ break;
+ case 0xfa:
usleep_range(5, 10);
- else if (addr == 0xf9)
+ break;
+ case 0xf9:
usleep_range(1, 2);
+ break;
+ }
}
EXPORT_SYMBOL(rtl_addr_delay);
--
2.7.0
From: Geliang Tang <[email protected]>
Use to_delayed_work() instead of open-coding it.
Signed-off-by: Geliang Tang <[email protected]>
[Update commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/realtek/rtlwifi/wifi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index 554d814..edece53 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -2867,7 +2867,7 @@ value to host byte ordering.*/
(ppsc->cur_ps_level |= _ps_flg)
#define container_of_dwork_rtl(x, y, z) \
- container_of(container_of(x, struct delayed_work, work), y, z)
+ container_of(to_delayed_work(x), y, z)
#define FILL_OCTET_STRING(_os, _octet, _len) \
(_os).octet = (u8 *)(_octet); \
--
2.7.0
From: Markus Elfring <[email protected]>
In rsi_send_mgmt_pkt(), the following variables are assigned to
before they're used:
* wh - Assigned on line 161, first used on line 180
* bss - Assigned on line 160, first used on line 196
* msg - Assigned on line 168, first used on line 175
* extnd_size - Assigned on line 139, first used on line 142
Signed-off-by: Markus Elfring <[email protected]>
[Rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 702593f..571eaba 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -123,15 +123,15 @@ int rsi_send_mgmt_pkt(struct rsi_common *common,
struct sk_buff *skb)
{
struct rsi_hw *adapter = common->priv;
- struct ieee80211_hdr *wh = NULL;
+ struct ieee80211_hdr *wh;
struct ieee80211_tx_info *info;
- struct ieee80211_bss_conf *bss = NULL;
+ struct ieee80211_bss_conf *bss;
struct ieee80211_hw *hw = adapter->hw;
struct ieee80211_conf *conf = &hw->conf;
struct skb_info *tx_params;
int status = -E2BIG;
- __le16 *msg = NULL;
- u8 extnd_size = 0;
+ __le16 *msg;
+ u8 extnd_size;
u8 vap_id = 0;
info = IEEE80211_SKB_CB(skb);
--
2.7.0
On 03/17/2016 09:28 PM, Julian Calaby wrote:
> From: Byeoungwook Kim <[email protected]>
>
> Doing it this way improves readability.
>
> Signed-off-by: Byeoungwook Kim <[email protected]>
> [Rewrote commit message]
> Signed-off-by: Julian Calaby <[email protected]>
>
> ---
>
> ... or does it.
>
> Honesltly, this is only in this list as Byeoungwook asked about it and I
> can see _some_ sort of reason why it exists.
>
> Larry, if you don't like this, please NACK it and we'll never speak of it
> again.
>
> Thanks,
>
> Julian Calaby
> ---
> drivers/net/wireless/realtek/rtlwifi/core.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
I thought I had already NACKed it, but just in case:
NACK.
Larry
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/core.c b/drivers/net/wireless/realtek/rtlwifi/core.c
> index 0f48048..1cf367f 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/core.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/core.c
> @@ -53,18 +53,26 @@ EXPORT_SYMBOL(channel5g_80m);
>
> void rtl_addr_delay(u32 addr)
> {
> - if (addr == 0xfe)
> + switch (addr) {
> + case 0xfe:
> msleep(50);
> - else if (addr == 0xfd)
> + break;
> + case 0xfd:
> msleep(5);
> - else if (addr == 0xfc)
> + break;
> + case 0xfc:
> msleep(1);
> - else if (addr == 0xfb)
> + break;
> + case 0xfb:
> usleep_range(50, 100);
> - else if (addr == 0xfa)
> + break;
> + case 0xfa:
> usleep_range(5, 10);
> - else if (addr == 0xf9)
> + break;
> + case 0xf9:
> usleep_range(1, 2);
> + break;
> + }
> }
> EXPORT_SYMBOL(rtl_addr_delay);
>
>
From: Markus Elfring <[email protected]>
Rename a jump label according to the current Linux coding style convention.
Signed-off-by: Markus Elfring <[email protected]>
[Rewrote commit title]
Signed-off-by: Julian Calaby <[email protected]>
---
This was controversial when introduced, however the change is obvious and
harmless and is, in the worst case, just churn.
I'm only including this as it meets my criteria for sheparding pending
patches: it's sane, obviously correct or reviewable by me, and doesn't
require any work to apply.
This does meet those requirements, however given how controviersial it was
when introduced, I'm not expecting people to be enthusiastic about
applying it.
Thanks,
Julian Calaby
---
drivers/net/wireless/intel/iwlegacy/common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index 2cc3d42..a1b1360 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -759,7 +759,7 @@ il_eeprom_init(struct il_priv *il)
IL_EEPROM_ACCESS_TIMEOUT);
if (ret < 0) {
IL_ERR("Time out reading EEPROM[%d]\n", addr);
- goto done;
+ goto release_semaphore;
}
r = _il_rd(il, CSR_EEPROM_REG);
e[addr / 2] = cpu_to_le16(r >> 16);
@@ -769,7 +769,7 @@ il_eeprom_init(struct il_priv *il)
il_eeprom_query16(il, EEPROM_VERSION));
ret = 0;
-done:
+release_semaphore:
il->ops->eeprom_release_semaphore(il);
err:
--
2.7.0
On Fri, Mar 18, 2016 at 01:28:33PM +1100, Julian Calaby wrote:
> From: Jia-Ju Bai <[email protected]>
>
> When il4965_hw_nic_init in __il4965_up fails, the memory allocated by
> iwl4965_sta_alloc_lq in iwl4965_alloc_bcast_station is not freed.
>
> This patches adds il_dealloc_bcast_stations in the error handling code of
> __il4965_up to fix this problem.
>
> This patch has been tested in real device, and it actually fixes the bug.
Could the call trace from the bug be provided ?
> Signed-off-by: Jia-Ju Bai <[email protected]>
> Acked-by: Stanislaw Gruszka <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
> ---
> drivers/net/wireless/intel/iwlegacy/4965-mac.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> index b75f4ef..30d9dd3 100644
> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
> @@ -5577,6 +5577,7 @@ __il4965_up(struct il_priv *il)
> ret = il4965_hw_nic_init(il);
> if (ret) {
> IL_ERR("Unable to init nic\n");
> + il_dealloc_bcast_stations(il);
I missed that before, but now this look suspicious for me.
il_dealloc_bcast_stations() do:
il->num_stations--;
BUG_ON(il->num_stations < 0);
But on il4965_alloc_bcast_station() we do not increase il->num_stations
Hence either this BUG_ON should be removed or il->num_stations should be
increased during allocation.
Stanislaw
From: Geliang Tang <[email protected]>
Use to_pci_dev() instead of open-coding it.
Signed-off-by: Geliang Tang <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/intel/ipw2x00/ipw2100.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/ipw2x00/ipw2100.c b/drivers/net/wireless/intel/ipw2x00/ipw2100.c
index f93a7f7..717320b 100644
--- a/drivers/net/wireless/intel/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/intel/ipw2x00/ipw2100.c
@@ -3521,7 +3521,7 @@ static void ipw2100_msg_free(struct ipw2100_priv *priv)
static ssize_t show_pci(struct device *d, struct device_attribute *attr,
char *buf)
{
- struct pci_dev *pci_dev = container_of(d, struct pci_dev, dev);
+ struct pci_dev *pci_dev = to_pci_dev(d);
char *out = buf;
int i, j;
u32 val;
--
2.7.0
On Sat, Mar 19, 2016 at 01:33:16AM +1100, Julian Calaby wrote:
> Hi Markus,
>
> On Sat, Mar 19, 2016 at 12:41 AM, SF Markus Elfring
> <[email protected]> wrote:
> >> Also remove an unused label.
> >
> > Is such a commit message a bit too short?
>
> I don't think so, but I don't decide these sorts of things.
>
> Does anyone else have an opinion on this?
This is certainly not something where we need to redo the patch but
since you asked for the future. A lot of web archives put the subject
and the body of the email far apart:
http://marc.info/?l=linux-wireless&m=145826783627087&w=2
lkml.org is the same way.
It makes these types of patch descriptions awkward.
regards,
dan carpenter
On 18 March 2016 at 06:20, Julian Calaby <[email protected]> wrote:
> From: Geliang Tang <[email protected]>
>
> Use to_pci_dev() instead of open-coding it.
>
> Signed-off-by: Geliang Tang <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
> ---
> drivers/net/wireless/intel/ipw2x00/ipw2100.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Stanislav Yakovlev <[email protected]>
Stanislav.
From: Markus Elfring <[email protected]>
In ath9k_hif_usb_rx_stream(), i is initialised in the for loop it's
used in.
Signed-off-by: Markus Elfring <[email protected]>
Reviewed-by: Oleksij Rempel <[email protected]>
[Rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/ath/ath9k/hif_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 8cbf490..e1c338c 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -527,7 +527,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
struct sk_buff *skb)
{
struct sk_buff *nskb, *skb_pool[MAX_PKT_NUM_IN_TRANSFER];
- int index = 0, i = 0, len = skb->len;
+ int index = 0, i, len = skb->len;
int rx_remain_len, rx_pkt_len;
u16 pool_index = 0;
u8 *ptr;
--
2.7.0
On Fri, Mar 18, 2016 at 01:29:39PM +1100, Julian Calaby wrote:
> From: Markus Elfring <[email protected]>
>
> Rename a jump label according to the current Linux coding style convention.
>
> Signed-off-by: Markus Elfring <[email protected]>
> [Rewrote commit title]
> Signed-off-by: Julian Calaby <[email protected]>
>
> ---
>
> This was controversial when introduced, however the change is obvious and
> harmless and is, in the worst case, just churn.
>
> I'm only including this as it meets my criteria for sheparding pending
> patches: it's sane, obviously correct or reviewable by me, and doesn't
> require any work to apply.
>
> This does meet those requirements, however given how controviersial it was
> when introduced, I'm not expecting people to be enthusiastic about
> applying it.
I don't see the sense of that change, NACK
Stanislaw
On Fri, Mar 18, 2016 at 01:29:11PM +1100, Julian Calaby wrote:
> In some of the non-success return paths, the memory allocated by
> iwl4965_sta_alloc_lq() in iwl4965_alloc_bcast_station() is not freed.
>
> In particular:
> - if the card isn't ready after il4965_prepare_card_hw()
> - if the card is hardware-rfkilled
>
> In the hardware rfkilled path, the driver enables the rfkill
> interrupt. When the card is unrfkilled and this interrupt is raised
> we end up calling il4965_bg_restart() which calls __il4965_up() which
> calls iwl4965_alloc_bcast_station() again.
>
> Suggested-by: Jia-Ju Bai <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
>
> ---
>
> This is only compile tested as I don't have compatible hardware. I also
> don't know the driver enough to know that this is truly correct - however
> it looks right and I stand by my analysis.
>
> Could someone else please review this?
Looks ok.
Acked-by: Stanislaw Gruszka <[email protected]>
Hi Markus,
On Sat, Mar 19, 2016 at 12:41 AM, SF Markus Elfring
<[email protected]> wrote:
>> Also remove an unused label.
>
> Is such a commit message a bit too short?
I don't think so, but I don't decide these sorts of things.
Does anyone else have an opinion on this?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
From: Jia-Ju Bai <[email protected]>
If "txq->cmd = kzalloc(...)" in il_tx_queue_init fails,
"kfree(txq->cmd[i])" in il_tx_queue_free and il_cmd_queue_free
in iwl4965_hw_txq_ctx_free will causes a null pointer dereference,
because txq->cmd is NULL at that time.
This patch fixes this problem by adding a if-check before kfree.
To avoid double free in il_tx_queue_free and il_cmd_queue_free
caused by the fixing, txq->meta and txq->cmd in error handling code
of il_tx_queue_init are assigned null values.
Otherwise, a double free will occur.
This patch has been tested in real device, and it actually fixes the bug.
Thanks Stanislaw for his suggestion.
Signed-off-by: Jia-Ju Bai <[email protected]>
Acked-by: Stanislaw Gruszka <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/intel/iwlegacy/common.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index c3afaf7..2cc3d42 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -2792,8 +2792,10 @@ il_tx_queue_free(struct il_priv *il, int txq_id)
il_tx_queue_unmap(il, txq_id);
/* De-alloc array of command/tx buffers */
- for (i = 0; i < TFD_TX_CMD_SLOTS; i++)
- kfree(txq->cmd[i]);
+ if (txq->cmd) {
+ for (i = 0; i < TFD_TX_CMD_SLOTS; i++)
+ kfree(txq->cmd[i]);
+ }
/* De-alloc circular buffer of TFDs */
if (txq->q.n_bd)
@@ -2871,8 +2873,10 @@ il_cmd_queue_free(struct il_priv *il)
il_cmd_queue_unmap(il);
/* De-alloc array of command/tx buffers */
- for (i = 0; i <= TFD_CMD_SLOTS; i++)
- kfree(txq->cmd[i]);
+ if (txq->cmd) {
+ for (i = 0; i <= TFD_CMD_SLOTS; i++)
+ kfree(txq->cmd[i]);
+ }
/* De-alloc circular buffer of TFDs */
if (txq->q.n_bd)
@@ -3078,7 +3082,9 @@ err:
kfree(txq->cmd[i]);
out_free_arrays:
kfree(txq->meta);
+ txq->meta = NULL;
kfree(txq->cmd);
+ txq->cmd = NULL;
return -ENOMEM;
}
--
2.7.0
From: Jia-Ju Bai <[email protected]>
When il4965_hw_nic_init in __il4965_up fails, the memory allocated by
iwl4965_sta_alloc_lq in iwl4965_alloc_bcast_station is not freed.
This patches adds il_dealloc_bcast_stations in the error handling code of
__il4965_up to fix this problem.
This patch has been tested in real device, and it actually fixes the bug.
Signed-off-by: Jia-Ju Bai <[email protected]>
Acked-by: Stanislaw Gruszka <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/intel/iwlegacy/4965-mac.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index b75f4ef..30d9dd3 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -5577,6 +5577,7 @@ __il4965_up(struct il_priv *il)
ret = il4965_hw_nic_init(il);
if (ret) {
IL_ERR("Unable to init nic\n");
+ il_dealloc_bcast_stations(il);
return ret;
}
--
2.7.0
On 03/17/2016 09:22 PM, Julian Calaby wrote:
> From: Geliang Tang <[email protected]>
>
> Use to_delayed_work() instead of open-coding it.
>
> Signed-off-by: Geliang Tang <[email protected]>
> [Update commit message]
> Signed-off-by: Julian Calaby <[email protected]>
Acked-by: Larry Finger <[email protected]>
Larry
> ---
> drivers/net/wireless/realtek/rtlwifi/wifi.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
> index 554d814..edece53 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
> +++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
> @@ -2867,7 +2867,7 @@ value to host byte ordering.*/
> (ppsc->cur_ps_level |= _ps_flg)
>
> #define container_of_dwork_rtl(x, y, z) \
> - container_of(container_of(x, struct delayed_work, work), y, z)
> + container_of(to_delayed_work(x), y, z)
>
> #define FILL_OCTET_STRING(_os, _octet, _len) \
> (_os).octet = (u8 *)(_octet); \
>
> Also remove an unused label.
Is such a commit message a bit too short?
Regards,
Markus
> In rsi_send_data_pkt(), the following variables are assigned to
> before they're used:
Thanks for your interest in this update suggestion.
How do you think about to add the function name also to such
a mail subject so that the potential for confusion with an other
change can be eventually reduced around the source file "rsi_91x_pkt.c"?
Regards,
Markus
Hi Dan,
On Fri, Mar 18, 2016 at 10:01 PM, Dan Carpenter
<[email protected]> wrote:
> On Fri, Mar 18, 2016 at 01:29:39PM +1100, Julian Calaby wrote:
>> From: Markus Elfring <[email protected]>
>>
>> Rename a jump label according to the current Linux coding style convention.
>>
>> Signed-off-by: Markus Elfring <[email protected]>
>> [Rewrote commit title]
>> Signed-off-by: Julian Calaby <[email protected]>
>>
>> ---
>>
>> This was controversial when introduced, however the change is obvious and
>> harmless and is, in the worst case, just churn.
>>
>> I'm only including this as it meets my criteria for sheparding pending
>> patches: it's sane, obviously correct or reviewable by me, and doesn't
>> require any work to apply.
>>
>> This does meet those requirements, however given how controviersial it was
>> when introduced, I'm not expecting people to be enthusiastic about
>> applying it.
>>
>> Thanks,
>
> It's a bad idea to mark this sort of patch as controversial. It's
> bikeshed stuff. Either apply it or don't but don't lets spend any more
> time thinking about it.
>
> Actual controversial patches have a long term impact.
Noted. I'll label anything like this (someone's done the work but it
makes no difference if it's applied) as "bikeshed" or "churn" in the
future.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
From: Markus Elfring <[email protected]>
In brcmf_sdio_download_firmware(), bcmerror is set by the call to
brcmf_sdio_download_code_file(), before it's checked in the following
line.
Signed-off-by: Markus Elfring <[email protected]>
Acked-by: Arend van Spriel <[email protected]>
[Rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 43fd3f4..fa2179d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3261,7 +3261,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
const struct firmware *fw,
void *nvram, u32 nvlen)
{
- int bcmerror = -EFAULT;
+ int bcmerror;
u32 rstvec;
sdio_claim_host(bus->sdiodev->func[1]);
--
2.7.0
From: Ivan Safonov <[email protected]>
"(thermometer < 0) ? 0 : (thermometer == X)" is equivalent to
"thermometer == X" for X >= 0.
Signed-off-by: Ivan Safonov <[email protected]>
[Updated commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/ath/ath9k/ar9003_eeprom.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
index 54ed2f7..a049f8d 100644
--- a/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
+++ b/drivers/net/wireless/ath/ath9k/ar9003_eeprom.c
@@ -4097,16 +4097,16 @@ static void ar9003_hw_thermometer_apply(struct ath_hw *ah)
REG_RMW_FIELD(ah, AR_PHY_65NM_CH2_RXTX4,
AR_PHY_65NM_CH0_RXTX4_THERM_ON_OVR, therm_on);
- therm_on = (thermometer < 0) ? 0 : (thermometer == 0);
+ therm_on = thermometer == 0;
REG_RMW_FIELD(ah, AR_PHY_65NM_CH0_RXTX4,
AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
if (pCap->chip_chainmask & BIT(1)) {
- therm_on = (thermometer < 0) ? 0 : (thermometer == 1);
+ therm_on = thermometer == 1;
REG_RMW_FIELD(ah, AR_PHY_65NM_CH1_RXTX4,
AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
}
if (pCap->chip_chainmask & BIT(2)) {
- therm_on = (thermometer < 0) ? 0 : (thermometer == 2);
+ therm_on = thermometer == 2;
REG_RMW_FIELD(ah, AR_PHY_65NM_CH2_RXTX4,
AR_PHY_65NM_CH0_RXTX4_THERM_ON, therm_on);
}
--
2.7.0
From: Jia-Ju Bai <[email protected]>
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 <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
Could someone else please review this?
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've had a quick scout around and this check seems to be particular to
realtek drivers.
Thanks,
Julian Calaby
---
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 c76af5d..a8a23d5 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;
}
--
2.7.0
On Fri, Mar 18, 2016 at 10:12:34PM +1100, Julian Calaby wrote:
> I don't think you're right. Looking closely at the code,
> il->num_stations gets incremented in il_prep_station() which is called
> unconditionally from il4965_alloc_bcast_station().
>
> So I think this and the following patch are fine.
You have right.
Thanks
Stanislaw
From: Jia-Ju Bai <[email protected]>
The memory allocated by kzalloc in b43_bus_dev_ssb_init and
b43_bus_dev_bcma_init is not freed.
This patch fixes the bug by adding kfree in b43_ssb_remove,
b43_bcma_remove and error handling code of b43_bcma_probe.
Thanks Michael for his suggestion.
Signed-off-by: Jia-Ju Bai <[email protected]>
Tested-by: Sudip Mukherjee <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
This stalled waiting for testing on BCMA.
The fix _looks_ correct.
For BCMA, b43_bus_dev_bcma_init() allocs the memory that wldev->dev ends
up pointing to, however it appears that this memory is never freed. Hunks
1 and 2 add that freeing to the probe and remove functions respectively.
Hunk 1 makes b43_bcma_probe()'s error handling look like b43_ssb_probe()'s.
Thanks,
Julian Calaby
---
drivers/net/wireless/broadcom/b43/main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/main.c b/drivers/net/wireless/broadcom/b43/main.c
index 72380af..b0603e7 100644
--- a/drivers/net/wireless/broadcom/b43/main.c
+++ b/drivers/net/wireless/broadcom/b43/main.c
@@ -5680,11 +5680,12 @@ static int b43_bcma_probe(struct bcma_device *core)
INIT_WORK(&wl->firmware_load, b43_request_firmware);
schedule_work(&wl->firmware_load);
-bcma_out:
return err;
bcma_err_wireless_exit:
ieee80211_free_hw(wl->hw);
+bcma_out:
+ kfree(dev);
return err;
}
@@ -5712,8 +5713,8 @@ static void b43_bcma_remove(struct bcma_device *core)
b43_rng_exit(wl);
b43_leds_unregister(wl);
-
ieee80211_free_hw(wl->hw);
+ kfree(wldev->dev);
}
static struct bcma_driver b43_bcma_driver = {
@@ -5796,6 +5797,7 @@ static void b43_ssb_remove(struct ssb_device *sdev)
b43_leds_unregister(wl);
b43_wireless_exit(dev, wl);
+ kfree(dev);
}
static struct ssb_driver b43_ssb_driver = {
--
2.7.0
From: Markus Elfring <[email protected]>
In rsi_send_data_pkt(), it's a little more logical to assign 'status' in
the actual error handling code as opposed to at the top of the functon.
Signed-off-by: Markus Elfring <[email protected]>
[Deleted controversial bits, rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
I'm not fussed if this one goes in or not as the central concept of the
patch can be argued both ways.
Markus had originally added changes to move the setting of adapter down to
just before it was used, however people rightly objected to it, so it's
been removed from this re-send and the commit message re-written to
reflect this.
Thanks,
Julian Calaby
---
drivers/net/wireless/rsi/rsi_91x_pkt.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 4322df1..a0b31c0 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -31,7 +31,7 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
struct ieee80211_tx_info *info;
struct skb_info *tx_params;
struct ieee80211_bss_conf *bss;
- int status = -EINVAL;
+ int status;
u8 ieee80211_size = MIN_802_11_HDR_LEN;
u8 extnd_size;
__le16 *frame_desc;
@@ -41,8 +41,10 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
bss = &info->control.vif->bss_conf;
tx_params = (struct skb_info *)info->driver_data;
- if (!bss->assoc)
+ if (!bss->assoc) {
+ status = -EINVAL;
goto err;
+ }
tmp_hdr = (struct ieee80211_hdr *)&skb->data[0];
seq_num = (le16_to_cpu(tmp_hdr->seq_ctrl) >> 4);
--
2.7.0
From: Geliang Tang <[email protected]>
Use to_delayed_work() instead of open-coding it.
Signed-off-by: Geliang Tang <[email protected]>
[Update commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/ti/wl1251/ps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ti/wl1251/ps.c b/drivers/net/wireless/ti/wl1251/ps.c
index b9e27b9..fa01b0a 100644
--- a/drivers/net/wireless/ti/wl1251/ps.c
+++ b/drivers/net/wireless/ti/wl1251/ps.c
@@ -32,7 +32,7 @@ void wl1251_elp_work(struct work_struct *work)
struct delayed_work *dwork;
struct wl1251 *wl;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wl = container_of(dwork, struct wl1251, elp_work);
wl1251_debug(DEBUG_PSM, "elp work");
--
2.7.0
Hi Stanislaw,
On Fri, Mar 18, 2016 at 9:48 PM, Stanislaw Gruszka <[email protected]> wrote:
> On Fri, Mar 18, 2016 at 01:28:33PM +1100, Julian Calaby wrote:
>> From: Jia-Ju Bai <[email protected]>
>>
>> When il4965_hw_nic_init in __il4965_up fails, the memory allocated by
>> iwl4965_sta_alloc_lq in iwl4965_alloc_bcast_station is not freed.
>>
>> This patches adds il_dealloc_bcast_stations in the error handling code of
>> __il4965_up to fix this problem.
>>
>> This patch has been tested in real device, and it actually fixes the bug.
>
> Could the call trace from the bug be provided ?
>
>> Signed-off-by: Jia-Ju Bai <[email protected]>
>> Acked-by: Stanislaw Gruszka <[email protected]>
>> Signed-off-by: Julian Calaby <[email protected]>
>> ---
>> drivers/net/wireless/intel/iwlegacy/4965-mac.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
>> index b75f4ef..30d9dd3 100644
>> --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
>> +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
>> @@ -5577,6 +5577,7 @@ __il4965_up(struct il_priv *il)
>> ret = il4965_hw_nic_init(il);
>> if (ret) {
>> IL_ERR("Unable to init nic\n");
>> + il_dealloc_bcast_stations(il);
>
> I missed that before, but now this look suspicious for me.
>
> il_dealloc_bcast_stations() do:
>
> il->num_stations--;
> BUG_ON(il->num_stations < 0);
>
> But on il4965_alloc_bcast_station() we do not increase il->num_stations
> Hence either this BUG_ON should be removed or il->num_stations should be
> increased during allocation.
I don't think you're right. Looking closely at the code,
il->num_stations gets incremented in il_prep_station() which is called
unconditionally from il4965_alloc_bcast_station().
So I think this and the following patch are fine.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
From: Markus Elfring <[email protected]>
Also remove an unused label.
Signed-off-by: Markus Elfring <[email protected]>
Acked-by: Stanislaw Gruszka <[email protected]>
[Rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/intel/iwlegacy/common.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index eb5cb60..c3afaf7 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -723,10 +723,9 @@ il_eeprom_init(struct il_priv *il)
sz = il->cfg->eeprom_size;
D_EEPROM("NVM size = %d\n", sz);
il->eeprom = kzalloc(sz, GFP_KERNEL);
- if (!il->eeprom) {
- ret = -ENOMEM;
- goto alloc_err;
- }
+ if (!il->eeprom)
+ return -ENOMEM;
+
e = (__le16 *) il->eeprom;
il->ops->apm_init(il);
@@ -778,7 +777,6 @@ err:
il_eeprom_free(il);
/* Reset chip to save power until we load uCode during "up". */
il_apm_stop(il);
-alloc_err:
return ret;
}
EXPORT_SYMBOL(il_eeprom_init);
--
2.7.0
Hi Dan,
On Sat, Mar 19, 2016 at 1:53 AM, Dan Carpenter <[email protected]> wrote:
> On Sat, Mar 19, 2016 at 01:33:16AM +1100, Julian Calaby wrote:
>> Hi Markus,
>>
>> On Sat, Mar 19, 2016 at 12:41 AM, SF Markus Elfring
>> <[email protected]> wrote:
>> >> Also remove an unused label.
>> >
>> > Is such a commit message a bit too short?
>>
>> I don't think so, but I don't decide these sorts of things.
>>
>> Does anyone else have an opinion on this?
>
> This is certainly not something where we need to redo the patch but
> since you asked for the future. A lot of web archives put the subject
> and the body of the email far apart:
>
> http://marc.info/?l=linux-wireless&m=145826783627087&w=2
>
> lkml.org is the same way.
>
> It makes these types of patch descriptions awkward.
Makes sense, I'll try to avoid descriptions like this in the future.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
In some of the non-success return paths, the memory allocated by
iwl4965_sta_alloc_lq() in iwl4965_alloc_bcast_station() is not freed.
In particular:
- if the card isn't ready after il4965_prepare_card_hw()
- if the card is hardware-rfkilled
In the hardware rfkilled path, the driver enables the rfkill
interrupt. When the card is unrfkilled and this interrupt is raised
we end up calling il4965_bg_restart() which calls __il4965_up() which
calls iwl4965_alloc_bcast_station() again.
Suggested-by: Jia-Ju Bai <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
This is only compile tested as I don't have compatible hardware. I also
don't know the driver enough to know that this is truly correct - however
it looks right and I stand by my analysis.
Could someone else please review this?
Thanks,
Julian Calaby
---
drivers/net/wireless/intel/iwlegacy/4965-mac.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
index 30d9dd3..f9ed480 100644
--- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c
+++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c
@@ -5553,6 +5553,7 @@ __il4965_up(struct il_priv *il)
il4965_prepare_card_hw(il);
if (!il->hw_ready) {
+ il_dealloc_bcast_stations(il);
IL_ERR("HW not ready\n");
return -EIO;
}
@@ -5564,6 +5565,7 @@ __il4965_up(struct il_priv *il)
set_bit(S_RFKILL, &il->status);
wiphy_rfkill_set_hw_state(il->hw->wiphy, true);
+ il_dealloc_bcast_stations(il);
il_enable_rfkill_int(il);
IL_WARN("Radio disabled by HW RF Kill switch\n");
return 0;
--
2.7.0
Hi Markus,
On Sat, Mar 19, 2016 at 11:27 PM, SF Markus Elfring
<[email protected]> wrote:
>> In rsi_send_data_pkt(), the following variables are assigned to
>> before they're used:
>
> Thanks for your interest in this update suggestion.
> How do you think about to add the function name also to such
> a mail subject so that the potential for confusion with an other
> change can be eventually reduced around the source file "rsi_91x_pkt.c"?
I don't think it really matters, they're both doing the same thing.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi Larry,
On Fri, Mar 18, 2016 at 1:38 PM, Larry Finger <[email protected]> wrote:
> On 03/17/2016 09:28 PM, Julian Calaby wrote:
>>
>> From: Byeoungwook Kim <[email protected]>
>>
>> Doing it this way improves readability.
>>
>> Signed-off-by: Byeoungwook Kim <[email protected]>
>> [Rewrote commit message]
>> Signed-off-by: Julian Calaby <[email protected]>
>>
>> ---
>>
>> ... or does it.
>>
>> Honesltly, this is only in this list as Byeoungwook asked about it and I
>> can see _some_ sort of reason why it exists.
>>
>> Larry, if you don't like this, please NACK it and we'll never speak of it
>> again.
>>
>> Thanks,
>>
>> Julian Calaby
>> ---
>> drivers/net/wireless/realtek/rtlwifi/core.c | 20 ++++++++++++++------
>> 1 file changed, 14 insertions(+), 6 deletions(-)
>
>
> I thought I had already NACKed it, but just in case:
>
> NACK.
Consider it dropped.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
From: Markus Elfring <[email protected]>
In rsi_send_data_pkt(), the following variables are assigned to
before they're used:
* tmp_hdr - Assigned on line 47, first used on line 48
* bss - Assigned on line 41, first used on line 44
* extnd_size - Assigned on line 50, first used on line 52
* seq_num - Assigned on line 48, first used on line 96
Signed-off-by: Markus Elfring <[email protected]>
[Rewrote commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/rsi/rsi_91x_pkt.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_pkt.c b/drivers/net/wireless/rsi/rsi_91x_pkt.c
index 571eaba..4322df1 100644
--- a/drivers/net/wireless/rsi/rsi_91x_pkt.c
+++ b/drivers/net/wireless/rsi/rsi_91x_pkt.c
@@ -27,15 +27,15 @@
int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
{
struct rsi_hw *adapter = common->priv;
- struct ieee80211_hdr *tmp_hdr = NULL;
+ struct ieee80211_hdr *tmp_hdr;
struct ieee80211_tx_info *info;
struct skb_info *tx_params;
- struct ieee80211_bss_conf *bss = NULL;
+ struct ieee80211_bss_conf *bss;
int status = -EINVAL;
u8 ieee80211_size = MIN_802_11_HDR_LEN;
- u8 extnd_size = 0;
+ u8 extnd_size;
__le16 *frame_desc;
- u16 seq_num = 0;
+ u16 seq_num;
info = IEEE80211_SKB_CB(skb);
bss = &info->control.vif->bss_conf;
--
2.7.0
Hi Stanislaw,
On Fri, Mar 18, 2016 at 9:31 PM, Stanislaw Gruszka <[email protected]> wrote:
> On Fri, Mar 18, 2016 at 01:29:39PM +1100, Julian Calaby wrote:
>> From: Markus Elfring <[email protected]>
>>
>> Rename a jump label according to the current Linux coding style convention.
>>
>> Signed-off-by: Markus Elfring <[email protected]>
>> [Rewrote commit title]
>> Signed-off-by: Julian Calaby <[email protected]>
>>
>> ---
>>
>> This was controversial when introduced, however the change is obvious and
>> harmless and is, in the worst case, just churn.
>>
>> I'm only including this as it meets my criteria for sheparding pending
>> patches: it's sane, obviously correct or reviewable by me, and doesn't
>> require any work to apply.
>>
>> This does meet those requirements, however given how controviersial it was
>> when introduced, I'm not expecting people to be enthusiastic about
>> applying it.
>
> I don't see the sense of that change, NACK
Fair enough, I wasn't expecting this to get applied anyway.
Consider it dropped.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
On Fri, Mar 18, 2016 at 01:29:11PM +1100, Julian Calaby wrote:
> In some of the non-success return paths, the memory allocated by
> iwl4965_sta_alloc_lq() in iwl4965_alloc_bcast_station() is not freed.
>
> In particular:
> - if the card isn't ready after il4965_prepare_card_hw()
> - if the card is hardware-rfkilled
>
> In the hardware rfkilled path, the driver enables the rfkill
> interrupt. When the card is unrfkilled and this interrupt is raised
> we end up calling il4965_bg_restart() which calls __il4965_up() which
> calls iwl4965_alloc_bcast_station() again.
>
> Suggested-by: Jia-Ju Bai <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
>
> ---
>
> This is only compile tested as I don't have compatible hardware. I also
> don't know the driver enough to know that this is truly correct - however
> it looks right and I stand by my analysis.
>
> Could someone else please review this?
I have similar objection like on previous patch "iwl4965: Fix a memory
leak in error handling code of __il4965_up" . Next week I will try
test that on the HW and see how it's going. For now this look for me,
like different fix should be prepared.
Thanks
Stanislaw
On Fri, Mar 18, 2016 at 01:29:39PM +1100, Julian Calaby wrote:
> From: Markus Elfring <[email protected]>
>
> Rename a jump label according to the current Linux coding style convention.
>
> Signed-off-by: Markus Elfring <[email protected]>
> [Rewrote commit title]
> Signed-off-by: Julian Calaby <[email protected]>
>
> ---
>
> This was controversial when introduced, however the change is obvious and
> harmless and is, in the worst case, just churn.
>
> I'm only including this as it meets my criteria for sheparding pending
> patches: it's sane, obviously correct or reviewable by me, and doesn't
> require any work to apply.
>
> This does meet those requirements, however given how controviersial it was
> when introduced, I'm not expecting people to be enthusiastic about
> applying it.
>
> Thanks,
It's a bad idea to mark this sort of patch as controversial. It's
bikeshed stuff. Either apply it or don't but don't lets spend any more
time thinking about it.
Actual controversial patches have a long term impact.
regards,
dan carpenter
From: Jia-Ju Bai <[email protected]>
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 <[email protected]>
Acked-by: Andrea Merello <[email protected]>
Signed-off-by: Julian Calaby <[email protected]>
---
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;
}
--
2.7.0
From: Geliang Tang <[email protected]>
Use to_delayed_work() instead of open-coding it.
Signed-off-by: Geliang Tang <[email protected]>
[Update commit message]
Signed-off-by: Julian Calaby <[email protected]>
---
drivers/net/wireless/ti/wlcore/main.c | 10 +++++-----
drivers/net/wireless/ti/wlcore/ps.c | 2 +-
drivers/net/wireless/ti/wlcore/scan.c | 2 +-
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index dde3620..a872a07 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -243,7 +243,7 @@ static void wl12xx_tx_watchdog_work(struct work_struct *work)
struct delayed_work *dwork;
struct wl1271 *wl;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wl = container_of(dwork, struct wl1271, tx_watchdog_work);
mutex_lock(&wl->mutex);
@@ -2011,7 +2011,7 @@ static void wlcore_channel_switch_work(struct work_struct *work)
struct wl12xx_vif *wlvif;
int ret;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wlvif = container_of(dwork, struct wl12xx_vif, channel_switch_work);
wl = wlvif->wl;
@@ -2047,7 +2047,7 @@ static void wlcore_connection_loss_work(struct work_struct *work)
struct ieee80211_vif *vif;
struct wl12xx_vif *wlvif;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wlvif = container_of(dwork, struct wl12xx_vif, connection_loss_work);
wl = wlvif->wl;
@@ -2076,7 +2076,7 @@ static void wlcore_pending_auth_complete_work(struct work_struct *work)
unsigned long time_spare;
int ret;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wlvif = container_of(dwork, struct wl12xx_vif,
pending_auth_complete_work);
wl = wlvif->wl;
@@ -5588,7 +5588,7 @@ static void wlcore_roc_complete_work(struct work_struct *work)
struct wl1271 *wl;
int ret;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wl = container_of(dwork, struct wl1271, roc_complete_work);
ret = wlcore_roc_completed(wl);
diff --git a/drivers/net/wireless/ti/wlcore/ps.c b/drivers/net/wireless/ti/wlcore/ps.c
index 4cd316e..d4420da 100644
--- a/drivers/net/wireless/ti/wlcore/ps.c
+++ b/drivers/net/wireless/ti/wlcore/ps.c
@@ -38,7 +38,7 @@ void wl1271_elp_work(struct work_struct *work)
struct wl12xx_vif *wlvif;
int ret;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wl = container_of(dwork, struct wl1271, elp_work);
wl1271_debug(DEBUG_PSM, "elp work");
diff --git a/drivers/net/wireless/ti/wlcore/scan.c b/drivers/net/wireless/ti/wlcore/scan.c
index 1e3d51c..a384f3f 100644
--- a/drivers/net/wireless/ti/wlcore/scan.c
+++ b/drivers/net/wireless/ti/wlcore/scan.c
@@ -38,7 +38,7 @@ void wl1271_scan_complete_work(struct work_struct *work)
struct wl12xx_vif *wlvif;
int ret;
- dwork = container_of(work, struct delayed_work, work);
+ dwork = to_delayed_work(work);
wl = container_of(dwork, struct wl1271, scan_complete_work);
wl1271_debug(DEBUG_SCAN, "Scanning complete");
--
2.7.0
Kalle Valo <[email protected]> writes:
> Julian Calaby <[email protected]> writes:
>
>> This is a set of all patches in patchwork which were pending when the 4.6
>> merge window opened that meet the following criteria:
>>
>> 1. They're sane
>> 2. They're either obviously correct, I can review them, or they've been
>> reviewed or ACK'd by someone else.
>> 3. They don't require any code changes to make them applyable.
>>
>> That said, if anyone doesn't like any of these patches for any reason,
>> just give me a reason and I'll drop it.
>>
>> I've split the pending patches into 4 sets as detailed below. All patches
>> have been rebased to apply in order on wireless-drivers-next at
>> wireless-drivers-next-for-davem-2016-03-14.
>
> Thank you for working on this, this is very helpful.
>
> I have deferred patch 14 and dropped patch 19[1], the rest I'm planning
> to apply soon. I also tried to remove the original patches from deferred
> queue by moving them to state "superseded", but I might missed
> something.
>
> [1] Oddly I cannot find patch 19 from patchwork, I don't know what
> happened to it. I do see it on the mailing list, though.
Actually another nacked patch 16 also has disappeared from patchwork, so
I'm not going to apply that either. I'm starting to wonder that maybe
you removed these yourself from patchwork?
--
Kalle Valo
Julian Calaby <[email protected]> writes:
> On Fri, Apr 8, 2016 at 10:56 AM, Julian Calaby <[email protected]> wrote:
>> Hi Kalle,
>>
>> On Fri, Apr 8, 2016 at 2:37 AM, Kalle Valo <[email protected]> wrote:
>>> Kalle Valo <[email protected]> writes:
>>>
>>>> Thank you for working on this, this is very helpful.
>>>>
>>>> I have deferred patch 14 and dropped patch 19[1], the rest I'm planning
>>>> to apply soon. I also tried to remove the original patches from deferred
>>>> queue by moving them to state "superseded", but I might missed
>>>> something.
>>
>> I'll check that later and give you a list of patchwork URLs for
>> anything you missed.
>
> Ok, from the bundles I initially produced, you missed the following:
> https://patchwork.kernel.org/patch/7939701/
> https://patchwork.kernel.org/patch/8003841/
> https://patchwork.kernel.org/patch/8049041/
> https://patchwork.kernel.org/patch/8049011/
> https://patchwork.kernel.org/patch/7940021/
> https://patchwork.kernel.org/patch/8040161/
> https://patchwork.kernel.org/patch/8040181/
> https://patchwork.kernel.org/patch/8197881/
Changed these to "Superseded"
> And this one has the status "changes requested", however I resubmitted
> it as patch #15 without the stupid bits and you applied it:
> https://patchwork.kernel.org/patch/8040201/
Ok, I changed it to Superseded.
> Beyond that, here's some earlier versions of the patches I picked up:
> https://patchwork.kernel.org/patch/8197051/
> https://patchwork.kernel.org/patch/7993931/
> https://patchwork.kernel.org/patch/8040201/
Marked as Superseded.
> This one should be dropped as it's too awful for me to endorse:
> https://patchwork.kernel.org/patch/7940001/
It's now "Rejected".
> And this one is obsolete as it's already been applied in some form:
> https://patchwork.kernel.org/patch/8197071/
I put it to Superseded.
> (If it'd make it easier for you, I can throw these all in a bundle.)
No need, this was perfect.
> I'll poke people for reviews for patch 14 and there's also a patch I
> missed in my first run-through that applies without changes, so I'll
> give it a reviewed-by so it's back on your radar.
Thank you so much for all this, I appreciate it a lot.
--
Kalle Valo
Hi Kalle,
On Fri, Apr 8, 2016 at 10:56 AM, Julian Calaby <[email protected]> wrote:
> Hi Kalle,
>
> On Fri, Apr 8, 2016 at 2:37 AM, Kalle Valo <[email protected]> wrote:
>> Kalle Valo <[email protected]> writes:
>>
>>> Julian Calaby <[email protected]> writes:
>>>
>>>> This is a set of all patches in patchwork which were pending when the 4.6
>>>> merge window opened that meet the following criteria:
>>>>
>>>> 1. They're sane
>>>> 2. They're either obviously correct, I can review them, or they've been
>>>> reviewed or ACK'd by someone else.
>>>> 3. They don't require any code changes to make them applyable.
>>>>
>>>> That said, if anyone doesn't like any of these patches for any reason,
>>>> just give me a reason and I'll drop it.
>>>>
>>>> I've split the pending patches into 4 sets as detailed below. All patches
>>>> have been rebased to apply in order on wireless-drivers-next at
>>>> wireless-drivers-next-for-davem-2016-03-14.
>>>
>>> Thank you for working on this, this is very helpful.
>>>
>>> I have deferred patch 14 and dropped patch 19[1], the rest I'm planning
>>> to apply soon. I also tried to remove the original patches from deferred
>>> queue by moving them to state "superseded", but I might missed
>>> something.
>
> I'll check that later and give you a list of patchwork URLs for
> anything you missed.
Ok, from the bundles I initially produced, you missed the following:
https://patchwork.kernel.org/patch/7939701/
https://patchwork.kernel.org/patch/8003841/
https://patchwork.kernel.org/patch/8049041/
https://patchwork.kernel.org/patch/8049011/
https://patchwork.kernel.org/patch/7940021/
https://patchwork.kernel.org/patch/8040161/
https://patchwork.kernel.org/patch/8040181/
https://patchwork.kernel.org/patch/8197881/
And this one has the status "changes requested", however I resubmitted
it as patch #15 without the stupid bits and you applied it:
https://patchwork.kernel.org/patch/8040201/
Beyond that, here's some earlier versions of the patches I picked up:
https://patchwork.kernel.org/patch/8197051/
https://patchwork.kernel.org/patch/7993931/
https://patchwork.kernel.org/patch/8040201/
This one should be dropped as it's too awful for me to endorse:
https://patchwork.kernel.org/patch/7940001/
And this one is obsolete as it's already been applied in some form:
https://patchwork.kernel.org/patch/8197071/
(If it'd make it easier for you, I can throw these all in a bundle.)
I'll poke people for reviews for patch 14 and there's also a patch I
missed in my first run-through that applies without changes, so I'll
give it a reviewed-by so it's back on your radar.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi Kalle,
On Fri, Apr 8, 2016 at 2:37 AM, Kalle Valo <[email protected]> wrote:
> Kalle Valo <[email protected]> writes:
>
>> Julian Calaby <[email protected]> writes:
>>
>>> This is a set of all patches in patchwork which were pending when the 4.6
>>> merge window opened that meet the following criteria:
>>>
>>> 1. They're sane
>>> 2. They're either obviously correct, I can review them, or they've been
>>> reviewed or ACK'd by someone else.
>>> 3. They don't require any code changes to make them applyable.
>>>
>>> That said, if anyone doesn't like any of these patches for any reason,
>>> just give me a reason and I'll drop it.
>>>
>>> I've split the pending patches into 4 sets as detailed below. All patches
>>> have been rebased to apply in order on wireless-drivers-next at
>>> wireless-drivers-next-for-davem-2016-03-14.
>>
>> Thank you for working on this, this is very helpful.
>>
>> I have deferred patch 14 and dropped patch 19[1], the rest I'm planning
>> to apply soon. I also tried to remove the original patches from deferred
>> queue by moving them to state "superseded", but I might missed
>> something.
I'll check that later and give you a list of patchwork URLs for
anything you missed.
>> [1] Oddly I cannot find patch 19 from patchwork, I don't know what
>> happened to it. I do see it on the mailing list, though.
>
> Actually another nacked patch 16 also has disappeared from patchwork, so
> I'm not going to apply that either. I'm starting to wonder that maybe
> you removed these yourself from patchwork?
I did do that: I updated both to "Rejected" when people NACKed them.
Should I not have done that? I thought I was making things easier for you.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Julian Calaby <[email protected]> writes:
> This is a set of all patches in patchwork which were pending when the 4.6
> merge window opened that meet the following criteria:
>
> 1. They're sane
> 2. They're either obviously correct, I can review them, or they've been
> reviewed or ACK'd by someone else.
> 3. They don't require any code changes to make them applyable.
>
> That said, if anyone doesn't like any of these patches for any reason,
> just give me a reason and I'll drop it.
>
> I've split the pending patches into 4 sets as detailed below. All patches
> have been rebased to apply in order on wireless-drivers-next at
> wireless-drivers-next-for-davem-2016-03-14.
Thank you for working on this, this is very helpful.
I have deferred patch 14 and dropped patch 19[1], the rest I'm planning
to apply soon. I also tried to remove the original patches from deferred
queue by moving them to state "superseded", but I might missed
something.
[1] Oddly I cannot find patch 19 from patchwork, I don't know what
happened to it. I do see it on the mailing list, though.
--
Kalle Valo
Hi all,
Could someone please review this?
On Fri, Mar 18, 2016 at 1:27 PM, Julian Calaby <[email protected]> wrote:
> From: Jia-Ju Bai <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
>
> ---
>
> Could someone else please review this?
>
> 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?
And could someone please comment on this? It doesn't seem correct to
me, however I don't know much about PCI / DMA etc.
> I've had a quick scout around and this check seems to be particular to
> realtek drivers.
>
> Thanks,
>
> Julian Calaby
> ---
> 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 c76af5d..a8a23d5 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;
> }
> --
> 2.7.0
>
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Julian Calaby <[email protected]> writes:
>>> [1] Oddly I cannot find patch 19 from patchwork, I don't know what
>>> happened to it. I do see it on the mailing list, though.
>>
>> Actually another nacked patch 16 also has disappeared from patchwork, so
>> I'm not going to apply that either. I'm starting to wonder that maybe
>> you removed these yourself from patchwork?
>
> I did do that: I updated both to "Rejected" when people NACKed them.
>
> Should I not have done that? I thought I was making things easier for you.
That does help, but I started to wonder what happened to them (did I do
something wrong etc) before I realised that you actually dropped them
yourself. Patchwork is sometimes a bit clumsy like this, so maybe an
email reply like "dropping these from patchwork" or similar is a good
idea?
--
Kalle Valo
On Mon, Apr 11, 2016 at 2:11 AM, Julian Calaby <[email protected]> wrote:
> Hi Andrea,
>
> On Sat, Apr 9, 2016 at 4:56 AM, Andrea Merello <[email protected]> wrote:
>>
>> On Fri, Mar 18, 2016 at 3:27 AM, Julian Calaby <[email protected]>
>> wrote:
>>> From: Jia-Ju Bai <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Julian Calaby <[email protected]>
>>>
>>> ---
>>>
>>> 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 0xFFFFFF00 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 care of freeing the memory whenever the allocator succeeds
>> but that check fails.
>
> I'll craft a followup patch to add a WARN_ON(), free the memory and
> fail in this case.
Ok, thank you.
>>> I've had a quick scout around and this check seems to be particular to
>>> realtek drivers.
>>
>> The HW needs DMA rings to be 256-byte aligned. I never tried to see what
>> happens if we break the rule, but I suspect that the HW will attempt DMAs to
>> bad addresses (actually masked with 0xFFFFFF00); If that's the case, then we
>> really need to avoid this..
>
> I guess if we got an address like 0x.....048 then we could arguably
> just tell the hardware we start at 0x.....100, however that's a
> terrible solution. This really needs to be addressed at the "source"
> i.e. informing the allocator what our requirements are and if it
> doesn't produce correct results, fixing it, not working around it in
> the driver.
Yes, of course. If there is a way to ask for this, then I completely
agree with you.. I never intended to workaround any allocator bug :)
> Thanks,
>
> --
> Julian Calaby
>
> Email: [email protected]
> Profile: http://www.google.com/profiles/julian.calaby/
> From: Ivan Safonov <[email protected]>
>
> "(thermometer < 0) ? 0 : (thermometer == X)" is equivalent to
> "thermometer == X" for X >= 0.
>
> Signed-off-by: Ivan Safonov <[email protected]>
> [Updated commit message]
> Signed-off-by: Julian Calaby <[email protected]>
Thanks, 16 patches applied to wireless-drivers-next.git:
0fef3c768037 ath9k: Remove unnecessary ?: operator
ea544aab42db ipw2x00: use to_pci_dev()
61383412f00d wlcore: use to_delayed_work()
d1162f0283f0 wl1251: use to_delayed_work()
4679f4132201 rtlwifi: use to_delayed_work()
cfbfbd13695c ath9k_htc: Delete unnecessary variable initialisation
9e12904a953c brcmfmac: Delete unnecessary variable initialisation
fb9693f04544 iwlegacy: Return directly if allocation fails in il_eeprom_init()
fe9b47944edf iwl4965: Fix a null pointer dereference in il_tx_queue_free and il_cmd_queue_free
96838d61102a b43: Fix memory leaks in b43_bus_dev_ssb_init and b43_bus_dev_bcma_init
1c76b4902c26 rtl818x_pci: Disable pci device in error handling code
8b28310efe24 rsi: Delete unnecessary variable initialisation
ab2ef1d68f62 rsi: Delete unnecessary variable initialisation
37190b269491 rsi: Move variable initialisation into error code
c2fd34469d16 iwl4965: Fix a memory leak in error handling code of __il4965_up
84d17a2a5a0f iwl4965: Fix more memory leaks in __il4965_up()
Kalle Valo
> From: Jia-Ju Bai <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Julian Calaby <[email protected]>
Thanks, applied to wireless-drivers-next.git.
Kalle Valo
Hi Andrea,
On Sat, Apr 9, 2016 at 4:56 AM, Andrea Merello <[email protected]> wrote:
>
> On Fri, Mar 18, 2016 at 3:27 AM, Julian Calaby <[email protected]>
> wrote:
>> From: Jia-Ju Bai <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Julian Calaby <[email protected]>
>>
>> ---
>>
>> 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?
> 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.
>> 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 0xFFFFFF00 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.
> 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 care of freeing the memory whenever the allocator succeeds
> but that check fails.
I'll craft a followup patch to add a WARN_ON(), free the memory and
fail in this case.
>> I've had a quick scout around and this check seems to be particular to
>> realtek drivers.
>
> The HW needs DMA rings to be 256-byte aligned. I never tried to see what
> happens if we break the rule, but I suspect that the HW will attempt DMAs to
> bad addresses (actually masked with 0xFFFFFF00); If that's the case, then we
> really need to avoid this..
I guess if we got an address like 0x.....048 then we could arguably
just tell the hardware we start at 0x.....100, however that's a
terrible solution. This really needs to be addressed at the "source"
i.e. informing the allocator what our requirements are and if it
doesn't produce correct results, fixing it, not working around it in
the driver.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/