Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[me]

On Tue, Feb 15, 2011 at 02:29:13PM +0000, Николай Ледовских wrote:
> This patch fixes seqfault on ar2316 and other AHB based chips.

Ok, but I'm guessing it also breaks PCI functionality?

>      struct ath5k_softc *sc = common->priv;
> -    struct platform_device *pdev = to_platform_device(sc->dev);
> +    struct platform_device *pdev =
> to_platform_device(wiphy_dev(sc->hw->wiphy));

Your patch got wrapped strangely here...

> -    mem = ioremap_nocache(res->start, res->end - res->start + 1);
> +    mem = res->start;

Won't this break iommu platforms?

> +    SET_IEEE80211_DEV(hw, &pdev->dev);
> 
> -    sc->dev = &pdev->dev;
> +    sc->dev = NULL;

>      /* Initialize driver private data */
> -    SET_IEEE80211_DEV(hw, sc->dev);

> @@ -216,6 +216,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
>          ret = -ENOMEM;
>          goto err_map;
>      }
> +    SET_IEEE80211_DEV(hw, &pdev->dev);

This set of changes needs some rationale...  in other words, what is
the real problem the patch fixes?

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Jiri Slaby]

On 02/15/2011 03:44 PM, me@bobcopeland.com wrote:
> On Tue, Feb 15, 2011 at 02:29:13PM +0000, Николай Ледовских wrote:
>> -    mem = ioremap_nocache(res->start, res->end - res->start + 1);
>> +    mem = res->start;
> 
> Won't this break iommu platforms?

And not only those. This is incorrect on _all_ platforms. You try to
dereference a physical address.

regards,
-- 
js

Re: [ath5k-devel] [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Hugh Davenport]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 16/02/11 03:29, Николай Ледовских wrote:
> This patch fixes seqfault on ar2316 and other AHB based chips.
> 

This could fix the issues I have been having with my ubiquiti bullet 2.
It segfaults all the time :P

Cheers,

Hugh
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNWvi1AAoJEBwrtoQ2Ia06JfUIAIMAYYjFp5n1081UfEQ8BUhJ
3QZiwEleuND+OKGOyCUbA+Ua6Pq9FOZRwpKmPus1p94a3PI5e5hVBtYVgNE+ENJg
M4S7Rc7YPCIGie3OdjztdIR9rscWzV7Z8s1MUt/cFrUqlncIfRE5bpDe8qboXjX8
uSpelqP7EAE6QlS7bKRc40t2ysInPgjsHNURr1uoUkhprsYrxsIvVM9oZtWcqkFq
9GwW/+81cEUS8+d04xrEuF4Upu/9mcP3POFGaCmesdnRPl16ZrSNfK53Fc08v6rG
pT2oYknuUSzFBRl9NBsjwOaZkYVifGvRWpWBLvhMv4og3ylEOw2qtksVFFnRvgc=
=buhf
-----END PGP SIGNATURE-----

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Nikolay Ledovskikh]

This patch fixes seqfault on ar2316 and other AHB based chips.

Signed-off-by: Nikolay Ledovskikh <nledovskikh@gmail.com>
---
diff --git a/drivers/net/wireless/ath/ath5k/ahb.c b/drivers/net/wireless/ath/ath5k/ahb.c
index 707cde1..35d5273 100644
--- a/drivers/net/wireless/ath/ath5k/ahb.c
+++ b/drivers/net/wireless/ath/ath5k/ahb.c
@@ -34,7 +34,7 @@ static void ath5k_ahb_read_cachesize(struct ath_common *common, int *csz)
 bool ath5k_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 {
 	struct ath5k_softc *sc = common->priv;
-	struct platform_device *pdev = to_platform_device(sc->dev);
+	struct platform_device *pdev = to_platform_device(wiphy_dev(sc->hw->wiphy));
 	struct ar231x_board_config *bcfg = pdev->dev.platform_data;
 	u16 *eeprom, *eeprom_end;
 
@@ -55,7 +55,7 @@ bool ath5k_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
 int ath5k_hw_read_srev(struct ath5k_hw *ah)
 {
 	struct ath5k_softc *sc = ah->ah_sc;
-	struct platform_device *pdev = to_platform_device(sc->dev);
+	struct platform_device *pdev = to_platform_device(wiphy_dev(sc->hw->wiphy));
 	struct ar231x_board_config *bcfg = pdev->dev.platform_data;
 	ah->ah_mac_srev = bcfg->devid;
 	return 0;
@@ -92,7 +92,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
 		goto err_out;
 	}
 
-	mem = ioremap_nocache(res->start, res->end - res->start + 1);
+	mem = res->start;
 	if (mem == NULL) {
 		dev_err(&pdev->dev, "ioremap failed\n");
 		ret = -ENOMEM;
@@ -114,10 +114,11 @@ static int ath_ahb_probe(struct platform_device *pdev)
 		ret = -ENOMEM;
 		goto err_out;
 	}
+	SET_IEEE80211_DEV(hw, &pdev->dev);
 
 	sc = hw->priv;
 	sc->hw = hw;
-	sc->dev = &pdev->dev;
+	sc->dev = NULL;
 	sc->iobase = mem;
 	sc->irq = irq;
 	sc->devid = bcfg->devid;
@@ -136,7 +137,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
 		/* Enable WMAC DMA access (assuming 5312 or 231x*/
 		/* TODO: check other platforms */
 		reg = __raw_readl((void __iomem *) AR5K_AR5312_ENABLE);
-		if (to_platform_device(sc->dev)->id == 0)
+		if (to_platform_device(wiphy_dev(sc->hw->wiphy))->id == 0)
 			reg |= AR5K_AR5312_ENABLE_WLAN0;
 		else
 			reg |= AR5K_AR5312_ENABLE_WLAN1;
@@ -181,7 +182,7 @@ static int ath_ahb_remove(struct platform_device *pdev)
 	} else {
 		/*Stop DMA access */
 		reg = __raw_readl((void __iomem *) AR5K_AR5312_ENABLE);
-		if (to_platform_device(sc->dev)->id == 0)
+		if (to_platform_device(wiphy_dev(sc->hw->wiphy))->id == 0)
 			reg &= ~AR5K_AR5312_ENABLE_WLAN0;
 		else
 			reg &= ~AR5K_AR5312_ENABLE_WLAN1;
diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 09ae4ef..8633762 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -2343,7 +2343,6 @@ ath5k_init_softc(struct ath5k_softc *sc, const struct ath_bus_ops *bus_ops)
 	int csz;
 
 	/* Initialize driver private data */
-	SET_IEEE80211_DEV(hw, sc->dev);
 	hw->flags = IEEE80211_HW_RX_INCLUDES_FCS |
 			IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING |
 			IEEE80211_HW_SIGNAL_DBM |
diff --git a/drivers/net/wireless/ath/ath5k/pci.c b/drivers/net/wireless/ath/ath5k/pci.c
index 7f8c5b0..7fb1eaf 100644
--- a/drivers/net/wireless/ath/ath5k/pci.c
+++ b/drivers/net/wireless/ath/ath5k/pci.c
@@ -216,6 +216,7 @@ ath5k_pci_probe(struct pci_dev *pdev,
 		ret = -ENOMEM;
 		goto err_map;
 	}
+	SET_IEEE80211_DEV(hw, &pdev->dev);
 
 	dev_info(&pdev->dev, "registered as '%s'\n", wiphy_name(hw->wiphy));
 
diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
index 8420689..2430dfb 100644
--- a/drivers/net/wireless/ath/ath5k/reset.c
+++ b/drivers/net/wireless/ath/ath5k/reset.c
@@ -383,7 +383,7 @@ static int ath5k_hw_wisoc_reset(struct ath5k_hw *ah, u32 flags)
 			val |= AR5K_AR2315_RESET_BB_WARM;
 	} else {
 		reg = (u32 *) AR5K_AR5312_RESET;
-		if (to_platform_device(ah->ah_sc->dev)->id == 0) {
+		if (to_platform_device(wiphy_dev(ah->ah_sc->hw->wiphy))->id == 0) {
 			if (mask & AR5K_RESET_CTL_PCU)
 				val |= AR5K_AR5312_RESET_WMAC0;
 			if (mask & AR5K_RESET_CTL_BASEBAND)
diff --git a/drivers/net/wireless/ath/ath5k/sysfs.c b/drivers/net/wireless/ath/ath5k/sysfs.c
index 929c68c..c93c685 100644
--- a/drivers/net/wireless/ath/ath5k/sysfs.c
+++ b/drivers/net/wireless/ath/ath5k/sysfs.c
@@ -95,7 +95,7 @@ static struct attribute_group ath5k_attribute_group_ani = {
 int
 ath5k_sysfs_register(struct ath5k_softc *sc)
 {
-	struct device *dev = sc->dev;
+	struct device *dev = wiphy_dev(sc->hw->wiphy);
 	int err;
 
 	err = sysfs_create_group(&dev->kobj, &ath5k_attribute_group_ani);
@@ -110,7 +110,7 @@ ath5k_sysfs_register(struct ath5k_softc *sc)
 void
 ath5k_sysfs_unregister(struct ath5k_softc *sc)
 {
-	struct device *dev = sc->dev;
+	struct device *dev = wiphy_dev(sc->hw->wiphy);
 
 	sysfs_remove_group(&dev->kobj, &ath5k_attribute_group_ani);
 }

-- 
Best regards, Nikolay Ledovskikh.

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Jiri Slaby]

On 02/15/2011 08:09 PM, Nikolay Ledovskikh wrote:
> This patch fixes seqfault on ar2316 and other AHB based chips.

You need to spare few more words on the changelog.

> Signed-off-by: Nikolay Ledovskikh <nledovskikh@gmail.com>
> ---
> diff --git a/drivers/net/wireless/ath/ath5k/ahb.c b/drivers/net/wireless/ath/ath5k/ahb.c
> index 707cde1..35d5273 100644
> --- a/drivers/net/wireless/ath/ath5k/ahb.c
> +++ b/drivers/net/wireless/ath/ath5k/ahb.c
...
> @@ -92,7 +92,7 @@ static int ath_ahb_probe(struct platform_device *pdev)
>  		goto err_out;
>  	}
>  
> -	mem = ioremap_nocache(res->start, res->end - res->start + 1);
> +	mem = res->start;

Looking into the code which fills this, I still don't get the change.
res->start is a physical address which should be remapped. Care to
elaborate?

>  	if (mem == NULL) {
>  		dev_err(&pdev->dev, "ioremap failed\n");
>  		ret = -ENOMEM;
> @@ -114,10 +114,11 @@ static int ath_ahb_probe(struct platform_device *pdev)
>  		ret = -ENOMEM;
>  		goto err_out;
>  	}
> +	SET_IEEE80211_DEV(hw, &pdev->dev);
>  
>  	sc = hw->priv;
>  	sc->hw = hw;
> -	sc->dev = &pdev->dev;
> +	sc->dev = NULL;

This I don't understand too. Maybe because of missing changelog...

regards,
-- 
js

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Jiri Slaby]

Please don't prune CCs.

On 02/15/2011 09:12 PM, Nikolay Ledovskikh wrote:
> Dear Jiri,
> 
> Should I resubmit the patch with another changelog?

Yes, definitely.

> 1. mem = res->start;
>     That is what madwifi driver does
> 
>     dev->mem_start = KSEG1ADDR(res->start);
>     dev->mem_end = KSEG1ADDR(res->end);
>     sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;

That's something completely different to what you did. This assignment
makes sense to me. But still prefer ioremap. Doesn't it work for you?

I'm no mips guy, sorry.

> 2.
>     I set sc->dev to NULL and setup save pointer to real device
> structure using SET_IEEE80211_DEV.
>     Now it's saved in wiphy structure and then used to get
> platform_device pointer.
>     I set sc->dev to NULL according to madwifi sources which look following
>         sc->aps_sc.sc_bdev = NULL; (if_ath_ahb.c)
>     and use this value in dma mapping functions.

Don't get inspired by madwifi sources, it's crap. The question is if you
have to do that to avoid the oops.

>     Looks like addresses mapped already and we shouldn't do ioremap once
> more?!

Maybe the address you got from the platform side was already ored by
KSEG1...

regards,
-- 
js

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Nikolay Ledovskikh]

Madwifi is a crap, but
       #define KSEG1ADDR(addr) (addr)
and only difference is in pointer type cast
      sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;
in my case it would be something like this:
      sc->aps_sc.sc_iobase = dev->mem_start;
and it works.

2011/2/15 Jiri Slaby <jirislaby@gmail.com>:
> Please don't prune CCs.
>
> On 02/15/2011 09:12 PM, Nikolay Ledovskikh wrote:
>> Dear Jiri,
>>
>> Should I resubmit the patch with another changelog?
>
> Yes, definitely.
>
>> 1. mem = res->start;
>>     That is what madwifi driver does
>>
>>     dev->mem_start = KSEG1ADDR(res->start);
>>     dev->mem_end = KSEG1ADDR(res->end);
>>     sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;
>
> That's something completely different to what you did. This assignment
> makes sense to me. But still prefer ioremap. Doesn't it work for you?
>
> I'm no mips guy, sorry.
>
>> 2.
>>     I set sc->dev to NULL and setup save pointer to real device
>> structure using SET_IEEE80211_DEV.
>>     Now it's saved in wiphy structure and then used to get
>> platform_device pointer.
>>     I set sc->dev to NULL according to madwifi sources which look following
>>         sc->aps_sc.sc_bdev = NULL; (if_ath_ahb.c)
>>     and use this value in dma mapping functions.
>
> Don't get inspired by madwifi sources, it's crap. The question is if you
> have to do that to avoid the oops.
>
>>     Looks like addresses mapped already and we shouldn't do ioremap once
>> more?!
>
> Maybe the address you got from the platform side was already ored by
> KSEG1...
>
> regards,
> --
> js
>



--
Best regards, Nikolay Ledovskikh.

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Jiri Slaby]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

On 02/15/2011 09:57 PM, Nikolay Ledovskikh wrote:
> Madwifi is a crap, but
>        #define KSEG1ADDR(addr) (addr)
> and only difference is in pointer type cast
>       sc->aps_sc.sc_iobase = (void __iomem *) dev->mem_start;
> in my case it would be something like this:
>       sc->aps_sc.sc_iobase = dev->mem_start;
> and it works.

In that case yes, but KSEG1ADDR should write as:
#define KSEG1ADDR(a) (CPHYSADDR(a) | KSEG1)

As I wrote, this needs some mips expert. But it definitely shouldn't be
a plain assignment.

regards,
-- 
js

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Nikolay Ledovskikh]

Thanks, I take it into account.
So we need to investigate the platform code?

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Nikolay Ledovskikh]

> Maybe the address you got from the platform side was already ored by
> KSEG1...

I took a look at openwrt atheros platform code and suppose you are right.
So what we should do for now? Add pointer cast (void __iomem *)?
Because ioremap_nocache doesn't work as expected. I think it's better
to rewrote the openwrt
code, but not now.

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Jiri Slaby]

On 02/15/2011 10:39 PM, Nikolay Ledovskikh wrote:
>> Maybe the address you got from the platform side was already ored by
>> KSEG1...
> 
> I took a look at openwrt atheros platform code and suppose you are right.
> So what we should do for now? Add pointer cast (void __iomem *)?
> Because ioremap_nocache doesn't work as expected. I think it's better
> to rewrote the openwrt
> code, but not now.

So I've found:
http://www.google.com/codesearch/p?hl=en#sayuPQDVf4c/trunk/openwrt/target/linux/atheros/patches-2.6.32/100-board.patch&q=ar231x-wmac&sa=N&cd=4&ct=rc

There, the res->start may be either of the following:
AR531X_WLAN0 .. 0x18000000
AR531X_WLAN1 .. 0x18500000
AR2315_WLAN0 .. 0xB0000000

I suppose you have the 3rd otherwise it should die without ORing KSEG1?

Or maybe MIPS guys will correct me? (The problem is that ioremap of one
of the addresses above kills the box. If Nikolaj removes the ioremap and
uses the address directly, it works for him. I'm saying it will die for
the first 2 addresses if we remove ioremap completely -- from what I
found in MIPS specs.)

I _think_ there should be (instead of ioremap):
sc->iobase = (void __iomem *)KSEG1ADDR(res->start);

Then we do readl(sc->iobase) et al. in ath5k.

thanks,
-- 
js

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Jiri Slaby]

On 02/15/2011 11:16 PM, Jiri Slaby wrote:
> On 02/15/2011 10:39 PM, Nikolay Ledovskikh wrote:
>>> Maybe the address you got from the platform side was already ored by
>>> KSEG1...
>>
>> I took a look at openwrt atheros platform code and suppose you are right.
>> So what we should do for now? Add pointer cast (void __iomem *)?
>> Because ioremap_nocache doesn't work as expected. I think it's better
>> to rewrote the openwrt
>> code, but not now.
> 
> So I've found:
> http://www.google.com/codesearch/p?hl=en#sayuPQDVf4c/trunk/openwrt/target/linux/atheros/patches-2.6.32/100-board.patch&q=ar231x-wmac&sa=N&cd=4&ct=rc
> 
> There, the res->start may be either of the following:
> AR531X_WLAN0 .. 0x18000000
> AR531X_WLAN1 .. 0x18500000


> AR2315_WLAN0 .. 0xB0000000

Or maybe this should be 0x10000000 in openwrt in the first place? Then
ioremap should do the right thing, right?

> I suppose you have the 3rd otherwise it should die without ORing KSEG1?
> 
> Or maybe MIPS guys will correct me? (The problem is that ioremap of one
> of the addresses above kills the box. If Nikolaj removes the ioremap and
> uses the address directly, it works for him. I'm saying it will die for
> the first 2 addresses if we remove ioremap completely -- from what I
> found in MIPS specs.)
> 
> I _think_ there should be (instead of ioremap):
> sc->iobase = (void __iomem *)KSEG1ADDR(res->start);
> 
> Then we do readl(sc->iobase) et al. in ath5k.
> 
> thanks,
-- 
js

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Nikolay Ledovskikh]

So, the problems are in openwrt platform patches that should be rewriten:
1. Changes needed to avoid using code like "mem=res->start" without ioremap.
2. Changes needed to avoid using NULL in first argument of
dma_alloc_coherent and others.

Without these changes ahb patch won't get into the kernel and we'll be
lack of wisoc devices support?
Is there any chance for the patch to get into the kernel and what
should be done for it?

(PS: Sorry for my english.)

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[Wojciech Dubowik]

> So, the problems are in openwrt platform patches that should be
> rewriten:
> 1. Changes needed to avoid using code like "mem=res->start" without
> ioremap.
> 2. Changes needed to avoid using NULL in first argument of
> dma_alloc_coherent and others.
> 
> Without these changes ahb patch won't get into the kernel and we'll be
> lack of wisoc devices support?
> Is there any chance for the patch to get into the kernel and what
> should be done for it?
I can confirm that the problem is not in the driver but in openwrt
code. Dma mapping is assuming it has to shift address to pci bar
no matter whether PCI is enabled or not.
It seems to work on devices which don't have AHB arbitration
and PCI core. Somehow dma doesn't mind that destination address
with extra 0x2 at the beginning. Or at least it doesn't crash.

Also physical addresses need to be consistent in platform code which
is not the case at the moment. I am working now on the patch to fix it 
in openwrt.

To get ar231x platform into the kernel is another story but if it
was done for ar71xx it just matter of some effort. Actually is 
anyone working on it? It would allow to use platform code in the
wireless driver without extensive patching.

Cheers,
Wojtek

> 
> (PS: Sorry for my english.)
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ath5k: Use mips generic dma-mapping functions to avoid seqfault on AHB chips

[John W. Linville]

On Tue, Feb 15, 2011 at 09:49:24PM +0300, Nikolay Ledovskikh wrote:
> This patch fixes seqfault on ar2316 and other AHB based chips.
> (I don't know why it's not in archives)
> 
> Signed-off-by: Nikolay Ledovskikh <nledovskikh@gmail.com>
> ---
> diff --git a/drivers/net/wireless/ath/
> ath5k/ahb.c b/drivers/net/wireless/ath/ath5k/ahb.c
> index 707cde1..35d5273 100644
> --- a/drivers/net/wireless/ath/ath5k/ahb.c
> +++ b/drivers/net/wireless/ath/ath5k/ahb.c
> @@ -34,7 +34,7 @@ static void ath5k_ahb_read_cachesize(struct ath_common
> *common, int *csz)
>  bool ath5k_ahb_eeprom_read(struct ath_common *common, u32 off, u16 *data)
>  {
>      struct ath5k_softc *sc = common->priv;
> -    struct platform_device *pdev = to_platform_device(sc->dev);
> +    struct platform_device *pdev =
> to_platform_device(wiphy_dev(sc->hw->wiphy));
>      struct ar231x_board_config *bcfg = pdev->dev.platform_data;
>      u16 *eeprom, *eeprom_end;
> 

Please use a mailer that doesn't do whitespace damage to patches --
I'll never live long enough to make-up for the time I've already
spent fixing-up broken patches...

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.