Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310933 > unrolled thread

Re: [GIT] Networking

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT] Networking Kalle Valo <kvalo@codeaurora.org> - 2016-01-16 13:00 +0100
    Re: [GIT] Networking Marc Zyngier <marc.zyngier@arm.com> - 2016-01-18 12:40 +0100
      Re: [GIT] Networking Arend van Spriel <arend@broadcom.com> - 2016-01-18 23:10 +0100
        Re: [GIT] Networking Marc Zyngier <marc.zyngier@arm.com> - 2016-01-19 10:00 +0100
          Re: [GIT] Networking Arend van Spriel <arend@broadcom.com> - 2016-01-19 10:40 +0100
            Re: [GIT] Networking Marc Zyngier <marc.zyngier@arm.com> - 2016-01-19 11:00 +0100
          Re: [GIT] Networking Kalle Valo <kvalo@codeaurora.org> - 2016-01-19 14:10 +0100

#1310933 — Re: [GIT] Networking

Marc Zyngier <marc.zyngier@arm.com> writes:

> David, Hante,
>
> On 13/01/16 02:51, David Miller wrote:
>
> [...]
>
>> Hante Meuleman (33):
> [...]
>>       brcmfmac: Move all module parameters to one place

As a reminder to myself this is the commit id:

7d34b0560567 brcmfmac: Move all module parameters to one place

> This particular patch breaks one of my boxes in a spectacular way:
>
> [    3.602155] Unable to handle kernel paging request at virtual address 000027e4
> [    3.602160] pgd = c0003000
> [    3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000
> [    3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM

[...]

> This is caused by this hunk:
>
> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
>  	if (!sdiodev->sg_support)
>  		return;
>  
> -	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
> +	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
> +		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
>  	nents += (nents >> 4) + 1;
>  
>  	WARN_ON(nents > sdiodev->max_segment_count);
>
> were drvr->settings is NULL (as the settings allocation seems to be done
> much later). The fix is not completely obvious to me (probably requires
> pushing the call to brcmf_mp_device_attach() down into the various bus
> specific functions). An alternative would be to restore the txglomsz
> parameter as it was before and not rely on settings being allocated.

Should we revert the patch or can you Hante fix this? The revert doesn't
seem to be trivial so I would appreciate if someone could send a patch.

-- 
Kalle Valo

[toc] | [next] | [standalone]

#1311494

Hi Kalle,

On 16/01/16 11:57, Kalle Valo wrote:
> Marc Zyngier <marc.zyngier@arm.com> writes:
> 
>> David, Hante,
>>
>> On 13/01/16 02:51, David Miller wrote:
>>
>> [...]
>>
>>> Hante Meuleman (33):
>> [...]
>>>       brcmfmac: Move all module parameters to one place
> 
> As a reminder to myself this is the commit id:
> 
> 7d34b0560567 brcmfmac: Move all module parameters to one place
> 
>> This particular patch breaks one of my boxes in a spectacular way:
>>
>> [    3.602155] Unable to handle kernel paging request at virtual address 000027e4
>> [    3.602160] pgd = c0003000
>> [    3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000
>> [    3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM
> 
> [...]
> 
>> This is caused by this hunk:
>>
>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
>>  	if (!sdiodev->sg_support)
>>  		return;
>>  
>> -	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
>> +	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
>> +		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
>>  	nents += (nents >> 4) + 1;
>>  
>>  	WARN_ON(nents > sdiodev->max_segment_count);
>>
>> were drvr->settings is NULL (as the settings allocation seems to be done
>> much later). The fix is not completely obvious to me (probably requires
>> pushing the call to brcmf_mp_device_attach() down into the various bus
>> specific functions). An alternative would be to restore the txglomsz
>> parameter as it was before and not rely on settings being allocated.
> 
> Should we revert the patch or can you Hante fix this? The revert doesn't
> seem to be trivial so I would appreciate if someone could send a patch.

I've worked out a partial revert (see below) that allows my system to
boot, but I'd rather see a proper fix from the maintainer of this code.

Thanks,

	M.

From 4dcd43e859ebf4453da90de2034c37174bb88d13 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Mon, 18 Jan 2016 11:16:36 +0000
Subject: [PATCH] brcmfmac: Partial revert of module parameter rework

Commit 7d34b0560567 ("brcmfmac: Move all module parameters to one
place") reworked the brcmfmac driver parameters and moved them to
a dynamically allocated structure.

It turns out that at least one parameter (sdiod_txglomsz) is required
before this structure is allocated, leading to a crash.

This patch reverts the handling of that parameter until the maintainer
of this driver can sort it out for good.

Cc: Arend Van Spriel <arend@broadcom.com>
Cc: Hante Meuleman <meuleman@broadcom.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 12 +++++++-----
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c |  7 -------
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h |  2 --
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 5363739..410a664 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -47,8 +47,6 @@
 #include "debug.h"
 #include "sdio.h"
 #include "of.h"
-#include "core.h"
-#include "common.h"
 
 #define SDIOH_API_ACCESS_RETRY_LIMIT	2
 
@@ -59,6 +57,7 @@
 /* Maximum milliseconds to wait for F2 to come up */
 #define SDIO_WAIT_F2RDY	3000
 
+#define BRCMF_DEFAULT_TXGLOM_SIZE	32  /* max tx frames in glom chain */
 #define BRCMF_DEFAULT_RXGLOM_SIZE	32  /* max rx frames in glom chain */
 
 struct brcmf_sdiod_freezer {
@@ -69,6 +68,10 @@ struct brcmf_sdiod_freezer {
 	struct completion resumed;
 };
 
+static int brcmf_sdiod_txglomsz = BRCMF_DEFAULT_TXGLOM_SIZE;
+module_param_named(txglomsz, brcmf_sdiod_txglomsz, int, 0);
+MODULE_PARM_DESC(txglomsz, "maximum tx packet chain size [SDIO]");
+
 static irqreturn_t brcmf_sdiod_oob_irqhandler(int irq, void *dev_id)
 {
 	struct brcmf_bus *bus_if = dev_get_drvdata(dev_id);
@@ -887,8 +890,7 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
 	if (!sdiodev->sg_support)
 		return;
 
-	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
-		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
+	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
 	nents += (nents >> 4) + 1;
 
 	WARN_ON(nents > sdiodev->max_segment_count);
@@ -900,7 +902,7 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
 		sdiodev->sg_support = false;
 	}
 
-	sdiodev->txglomsz = sdiodev->bus_if->drvr->settings->sdiod_txglomsz;
+	sdiodev->txglomsz = brcmf_sdiod_txglomsz;
 }
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 4265b50..1f57c42 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -35,12 +35,6 @@ const u8 ALLFFMAC[ETH_ALEN] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
 /* boost value for RSSI_DELTA in preferred join selection */
 #define BRCMF_JOIN_PREF_RSSI_BOOST	8
 
-#define BRCMF_DEFAULT_TXGLOM_SIZE	32  /* max tx frames in glom chain */
-
-static int brcmf_sdiod_txglomsz = BRCMF_DEFAULT_TXGLOM_SIZE;
-module_param_named(txglomsz, brcmf_sdiod_txglomsz, int, 0);
-MODULE_PARM_DESC(txglomsz, "Maximum tx packet chain size [SDIO]");
-
 /* Debug level configuration. See debug.h for bits, sysfs modifiable */
 int brcmf_msg_level;
 module_param_named(debug, brcmf_msg_level, int, S_IRUSR | S_IWUSR);
@@ -234,7 +228,6 @@ int brcmf_mp_device_attach(struct brcmf_pub *drvr)
 		return -ENOMEM;
 	}
 
-	drvr->settings->sdiod_txglomsz = brcmf_sdiod_txglomsz;
 	drvr->settings->p2p_enable = !!brcmf_p2p_enable;
 	drvr->settings->feature_disable = brcmf_feature_disable;
 	drvr->settings->fcmode = brcmf_fcmode;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
index 3b0a63b..05ad4b8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h
@@ -41,7 +41,6 @@ extern struct brcmf_mp_global_t brcmf_mp_global;
 /**
  * struct brcmf_mp_device - Device module paramaters.
  *
- * @sdiod_txglomsz: SDIO txglom size.
  * @joinboost_5g_rssi: 5g rssi booost for preferred join selection.
  * @p2p_enable: Legacy P2P0 enable (old wpa_supplicant).
  * @feature_disable: Feature_disable bitmask.
@@ -49,7 +48,6 @@ extern struct brcmf_mp_global_t brcmf_mp_global;
  * @roamoff: Firmware roaming off?
  */
 struct brcmf_mp_device {
-	int	sdiod_txglomsz;
 	int	joinboost_5g_rssi;
 	bool	p2p_enable;
 	int	feature_disable;
-- 
2.1.4


-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1311818

On 18-1-2016 12:30, Marc Zyngier wrote:
> Hi Kalle,
> 
> On 16/01/16 11:57, Kalle Valo wrote:
>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>
>>> David, Hante,
>>>
>>> On 13/01/16 02:51, David Miller wrote:
>>>
>>> [...]
>>>
>>>> Hante Meuleman (33):
>>> [...]
>>>>       brcmfmac: Move all module parameters to one place
>>
>> As a reminder to myself this is the commit id:
>>
>> 7d34b0560567 brcmfmac: Move all module parameters to one place
>>
>>> This particular patch breaks one of my boxes in a spectacular way:
>>>
>>> [    3.602155] Unable to handle kernel paging request at virtual address 000027e4
>>> [    3.602160] pgd = c0003000
>>> [    3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000
>>> [    3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM
>>
>> [...]
>>
>>> This is caused by this hunk:
>>>
>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
>>>  	if (!sdiodev->sg_support)
>>>  		return;
>>>  
>>> -	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
>>> +	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
>>> +		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
>>>  	nents += (nents >> 4) + 1;
>>>  
>>>  	WARN_ON(nents > sdiodev->max_segment_count);
>>>
>>> were drvr->settings is NULL (as the settings allocation seems to be done
>>> much later). The fix is not completely obvious to me (probably requires
>>> pushing the call to brcmf_mp_device_attach() down into the various bus
>>> specific functions). An alternative would be to restore the txglomsz
>>> parameter as it was before and not rely on settings being allocated.
>>
>> Should we revert the patch or can you Hante fix this? The revert doesn't
>> seem to be trivial so I would appreciate if someone could send a patch.
> 
> I've worked out a partial revert (see below) that allows my system to
> boot, but I'd rather see a proper fix from the maintainer of this code.

Hi Marc,

Thanks for the patch, but Hante has created a different patch basically
deferring the allocation of the sgtable. Feel free to give it a spin on
your box and share the results. I am looking for a arm64 platform to add
to my test machines so if you can recommend one (with PCIe).

Regards,
Arend

From f7d015866f24cfad5af0c35b1ab60c567835c5e8 Mon Sep 17 00:00:00 2001
From: Hante Meuleman <meuleman@broadcom.com>
Date: Mon, 18 Jan 2016 15:19:45 +0100
Subject: [PATCH] brcmfmac: fix sdio sg table alloc crash

With the patch to move all module paramaters to one place a bug was
introduced causing a null pointer exception. This patch fixes the
bug by initializing the sg table till after the settings have been
initialized.

Reviewed-by: Arend Van Spriel <arend@broadcom.com>
Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
Signed-off-by: Hante Meuleman <meuleman@broadcom.com>
Signed-off-by: Arend van Spriel <arend@broadcom.com>
---
 .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c  | 40
++++++++--------------
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    |  5 +++
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.h    |  1 +
 3 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 5363739..b98db8a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -879,11 +879,24 @@ int brcmf_sdiod_abort(struct brcmf_sdio_dev
*sdiodev, uint fn)
 	return 0;
 }

-static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
+void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
 {
+	struct sdio_func *func;
+	struct mmc_host *host;
+	uint max_blocks;
 	uint nents;
 	int err;

+	func = sdiodev->func[2];
+	host = func->card->host;
+	sdiodev->sg_support = host->max_segs > 1;
+	max_blocks = min_t(uint, host->max_blk_count, 511u);
+	sdiodev->max_request_size = min_t(uint, host->max_req_size,
+					  max_blocks * func->cur_blksize);
+	sdiodev->max_segment_count = min_t(uint, host->max_segs,
+					   SG_MAX_SINGLE_ALLOC);
+	sdiodev->max_segment_size = host->max_seg_size;
+
 	if (!sdiodev->sg_support)
 		return;

@@ -1021,9 +1034,6 @@ static void brcmf_sdiod_host_fixup(struct mmc_host
*host)

 static int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
 {
-	struct sdio_func *func;
-	struct mmc_host *host;
-	uint max_blocks;
 	int ret = 0;

 	sdiodev->num_funcs = 2;
@@ -1054,26 +1064,6 @@ static int brcmf_sdiod_probe(struct
brcmf_sdio_dev *sdiodev)
 		goto out;
 	}

-	/*
-	 * determine host related variables after brcmf_sdiod_probe()
-	 * as func->cur_blksize is properly set and F2 init has been
-	 * completed successfully.
-	 */
-	func = sdiodev->func[2];
-	host = func->card->host;
-	sdiodev->sg_support = host->max_segs > 1;
-	max_blocks = min_t(uint, host->max_blk_count, 511u);
-	sdiodev->max_request_size = min_t(uint, host->max_req_size,
-					  max_blocks * func->cur_blksize);
-	sdiodev->max_segment_count = min_t(uint, host->max_segs,
-					   SG_MAX_SINGLE_ALLOC);
-	sdiodev->max_segment_size = host->max_seg_size;
-
-	/* allocate scatter-gather table. sg support
-	 * will be disabled upon allocation failure.
-	 */
-	brcmf_sdiod_sgtable_alloc(sdiodev);
-
 	ret = brcmf_sdiod_freezer_attach(sdiodev);
 	if (ret)
 		goto out;
@@ -1084,7 +1074,7 @@ static int brcmf_sdiod_probe(struct brcmf_sdio_dev
*sdiodev)
 		ret = -ENODEV;
 		goto out;
 	}
-	brcmf_sdiod_host_fixup(host);
+	brcmf_sdiod_host_fixup(sdiodev->func[2]->card->host);
 out:
 	if (ret)
 		brcmf_sdiod_remove(sdiodev);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index dd66143..a14d9d9d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -4114,6 +4114,11 @@ struct brcmf_sdio *brcmf_sdio_probe(struct
brcmf_sdio_dev *sdiodev)
 		goto fail;
 	}

+	/* allocate scatter-gather table. sg support
+	 * will be disabled upon allocation failure.
+	 */
+	brcmf_sdiod_sgtable_alloc(bus->sdiodev);
+
 	/* Query the F2 block size, set roundup accordingly */
 	bus->blocksize = bus->sdiodev->func[2]->cur_blksize;
 	bus->roundup = min(max_roundup, bus->blocksize);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
index 5ec7a6d..23f2231 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
@@ -342,6 +342,7 @@ int brcmf_sdiod_ramrw(struct brcmf_sdio_dev
*sdiodev, bool write, u32 address,

 /* Issue an abort to the specified function */
 int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, uint fn);
+void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev);
 void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev,
 			      enum brcmf_sdiod_state state);
 #ifdef CONFIG_PM_SLEEP
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1312020

On 18/01/16 22:05, Arend van Spriel wrote:
> On 18-1-2016 12:30, Marc Zyngier wrote:
>> Hi Kalle,
>>
>> On 16/01/16 11:57, Kalle Valo wrote:
>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>
>>>> David, Hante,
>>>>
>>>> On 13/01/16 02:51, David Miller wrote:
>>>>
>>>> [...]
>>>>
>>>>> Hante Meuleman (33):
>>>> [...]
>>>>>       brcmfmac: Move all module parameters to one place
>>>
>>> As a reminder to myself this is the commit id:
>>>
>>> 7d34b0560567 brcmfmac: Move all module parameters to one place
>>>
>>>> This particular patch breaks one of my boxes in a spectacular way:
>>>>
>>>> [    3.602155] Unable to handle kernel paging request at virtual address 000027e4
>>>> [    3.602160] pgd = c0003000
>>>> [    3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000
>>>> [    3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM
>>>
>>> [...]
>>>
>>>> This is caused by this hunk:
>>>>
>>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
>>>>  	if (!sdiodev->sg_support)
>>>>  		return;
>>>>  
>>>> -	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
>>>> +	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
>>>> +		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
>>>>  	nents += (nents >> 4) + 1;
>>>>  
>>>>  	WARN_ON(nents > sdiodev->max_segment_count);
>>>>
>>>> were drvr->settings is NULL (as the settings allocation seems to be done
>>>> much later). The fix is not completely obvious to me (probably requires
>>>> pushing the call to brcmf_mp_device_attach() down into the various bus
>>>> specific functions). An alternative would be to restore the txglomsz
>>>> parameter as it was before and not rely on settings being allocated.
>>>
>>> Should we revert the patch or can you Hante fix this? The revert doesn't
>>> seem to be trivial so I would appreciate if someone could send a patch.
>>
>> I've worked out a partial revert (see below) that allows my system to
>> boot, but I'd rather see a proper fix from the maintainer of this code.
> 
> Hi Marc,
> 
> Thanks for the patch, but Hante has created a different patch basically
> deferring the allocation of the sgtable. Feel free to give it a spin on
> your box and share the results.

Hi Arend,

This patch fixes indeed the problem, thanks (I had to undo the mangling
your mailer had done, though). So feel free to add my:

Reported-by: Marc Zyngier <marc.zyngier@arm.com>
Tested-by: Marc Zyngier <marc.zyngier@arm.com>

It would be good if this could make it in -rc1.

> I am looking for a arm64 platform to add to my test machines so if
> you can recommend one (with PCIe).

Note that this crash happened with 32bit ARM, without PCIe (the box is a
"Cubietruck").

As for arm64, there is now quite a few machines out there (ARM sells a
dev board called Juno, but there is also various offerings with AMD
Opteron A1100, Cavium ThunderX, APM X-Gene, and maybe even some Broadcom
based systems, who knows! ;-).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1312040

On 19-1-2016 9:55, Marc Zyngier wrote:
> On 18/01/16 22:05, Arend van Spriel wrote:
>> On 18-1-2016 12:30, Marc Zyngier wrote:
>>> Hi Kalle,
>>>
>>> On 16/01/16 11:57, Kalle Valo wrote:
>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>
>>>>> David, Hante,
>>>>>
>>>>> On 13/01/16 02:51, David Miller wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>> Hante Meuleman (33):
>>>>> [...]
>>>>>>       brcmfmac: Move all module parameters to one place
>>>>
>>>> As a reminder to myself this is the commit id:
>>>>
>>>> 7d34b0560567 brcmfmac: Move all module parameters to one place
>>>>
>>>>> This particular patch breaks one of my boxes in a spectacular way:
>>>>>
>>>>> [    3.602155] Unable to handle kernel paging request at virtual address 000027e4
>>>>> [    3.602160] pgd = c0003000
>>>>> [    3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000
>>>>> [    3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM
>>>>
>>>> [...]
>>>>
>>>>> This is caused by this hunk:
>>>>>
>>>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
>>>>>  	if (!sdiodev->sg_support)
>>>>>  		return;
>>>>>  
>>>>> -	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
>>>>> +	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
>>>>> +		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
>>>>>  	nents += (nents >> 4) + 1;
>>>>>  
>>>>>  	WARN_ON(nents > sdiodev->max_segment_count);
>>>>>
>>>>> were drvr->settings is NULL (as the settings allocation seems to be done
>>>>> much later). The fix is not completely obvious to me (probably requires
>>>>> pushing the call to brcmf_mp_device_attach() down into the various bus
>>>>> specific functions). An alternative would be to restore the txglomsz
>>>>> parameter as it was before and not rely on settings being allocated.
>>>>
>>>> Should we revert the patch or can you Hante fix this? The revert doesn't
>>>> seem to be trivial so I would appreciate if someone could send a patch.
>>>
>>> I've worked out a partial revert (see below) that allows my system to
>>> boot, but I'd rather see a proper fix from the maintainer of this code.
>>
>> Hi Marc,
>>
>> Thanks for the patch, but Hante has created a different patch basically
>> deferring the allocation of the sgtable. Feel free to give it a spin on
>> your box and share the results.
> 
> Hi Arend,
> 
> This patch fixes indeed the problem, thanks (I had to undo the mangling
> your mailer had done, though). So feel free to add my:

Thanks, Marc

I started using thunderbird on windows 7. Probably better revert to the
one on Citrix.

> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> It would be good if this could make it in -rc1.

Will try to submit a clean patch to Kalle.

>> I am looking for a arm64 platform to add to my test machines so if
>> you can recommend one (with PCIe).
> 
> Note that this crash happened with 32bit ARM, without PCIe (the box is a
> "Cubietruck").
> 
> As for arm64, there is now quite a few machines out there (ARM sells a
> dev board called Juno, but there is also various offerings with AMD
> Opteron A1100, Cavium ThunderX, APM X-Gene, and maybe even some Broadcom
> based systems, who knows! ;-).

Thanks. Actually found something in our corporate spam:

http://www.broadcom.com/press/release.php?id=s948493

Regards,
Arend

> Thanks,
> 
> 	M.
> 

[toc] | [prev] | [next] | [standalone]

#1312058

On 19/01/16 09:36, Arend van Spriel wrote:
> On 19-1-2016 9:55, Marc Zyngier wrote:
>> On 18/01/16 22:05, Arend van Spriel wrote:
>>> On 18-1-2016 12:30, Marc Zyngier wrote:
>>>> Hi Kalle,
>>>>
>>>> On 16/01/16 11:57, Kalle Valo wrote:
>>>>> Marc Zyngier <marc.zyngier@arm.com> writes:
>>>>>
>>>>>> David, Hante,
>>>>>>
>>>>>> On 13/01/16 02:51, David Miller wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> Hante Meuleman (33):
>>>>>> [...]
>>>>>>>       brcmfmac: Move all module parameters to one place
>>>>>
>>>>> As a reminder to myself this is the commit id:
>>>>>
>>>>> 7d34b0560567 brcmfmac: Move all module parameters to one place
>>>>>
>>>>>> This particular patch breaks one of my boxes in a spectacular way:
>>>>>>
>>>>>> [    3.602155] Unable to handle kernel paging request at virtual address 000027e4
>>>>>> [    3.602160] pgd = c0003000
>>>>>> [    3.602169] [000027e4] *pgd=80000040004003, *pmd=00000000
>>>>>> [    3.602181] Internal error: Oops: 206 [#1] PREEMPT SMP ARM
>>>>>
>>>>> [...]
>>>>>
>>>>>> This is caused by this hunk:
>>>>>>
>>>>>> @@ -890,7 +887,8 @@ static void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
>>>>>>  	if (!sdiodev->sg_support)
>>>>>>  		return;
>>>>>>  
>>>>>> -	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE, brcmf_sdiod_txglomsz);
>>>>>> +	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
>>>>>> +		      sdiodev->bus_if->drvr->settings->sdiod_txglomsz);
>>>>>>  	nents += (nents >> 4) + 1;
>>>>>>  
>>>>>>  	WARN_ON(nents > sdiodev->max_segment_count);
>>>>>>
>>>>>> were drvr->settings is NULL (as the settings allocation seems to be done
>>>>>> much later). The fix is not completely obvious to me (probably requires
>>>>>> pushing the call to brcmf_mp_device_attach() down into the various bus
>>>>>> specific functions). An alternative would be to restore the txglomsz
>>>>>> parameter as it was before and not rely on settings being allocated.
>>>>>
>>>>> Should we revert the patch or can you Hante fix this? The revert doesn't
>>>>> seem to be trivial so I would appreciate if someone could send a patch.
>>>>
>>>> I've worked out a partial revert (see below) that allows my system to
>>>> boot, but I'd rather see a proper fix from the maintainer of this code.
>>>
>>> Hi Marc,
>>>
>>> Thanks for the patch, but Hante has created a different patch basically
>>> deferring the allocation of the sgtable. Feel free to give it a spin on
>>> your box and share the results.
>>
>> Hi Arend,
>>
>> This patch fixes indeed the problem, thanks (I had to undo the mangling
>> your mailer had done, though). So feel free to add my:
> 
> Thanks, Marc
> 
> I started using thunderbird on windows 7. Probably better revert to the
> one on Citrix.
> 
>> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
>> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> It would be good if this could make it in -rc1.
> 
> Will try to submit a clean patch to Kalle.
> 
>>> I am looking for a arm64 platform to add to my test machines so if
>>> you can recommend one (with PCIe).
>>
>> Note that this crash happened with 32bit ARM, without PCIe (the box is a
>> "Cubietruck").
>>
>> As for arm64, there is now quite a few machines out there (ARM sells a
>> dev board called Juno, but there is also various offerings with AMD
>> Opteron A1100, Cavium ThunderX, APM X-Gene, and maybe even some Broadcom
>> based systems, who knows! ;-).
> 
> Thanks. Actually found something in our corporate spam:
> 
> http://www.broadcom.com/press/release.php?id=s948493

Ah, quality marketroid blurb! :-) Yeah, this looks promising, assuming
it is available in the not too distant future (I think I reviewed some
of the PCIe code for that platform...).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1312190

Marc Zyngier <marc.zyngier@arm.com> writes:

>>>> Should we revert the patch or can you Hante fix this? The revert doesn't
>>>> seem to be trivial so I would appreciate if someone could send a patch.
>>>
>>> I've worked out a partial revert (see below) that allows my system to
>>> boot, but I'd rather see a proper fix from the maintainer of this code.
>> 
>> Hi Marc,
>> 
>> Thanks for the patch, but Hante has created a different patch basically
>> deferring the allocation of the sgtable. Feel free to give it a spin on
>> your box and share the results.
>
> Hi Arend,
>
> This patch fixes indeed the problem, thanks (I had to undo the mangling
> your mailer had done, though). So feel free to add my:
>
> Reported-by: Marc Zyngier <marc.zyngier@arm.com>
> Tested-by: Marc Zyngier <marc.zyngier@arm.com>
>
> It would be good if this could make it in -rc1.

I doubt it makes it to -rc1, most likely to -rc2. But let's see.

-- 
Kalle Valo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web