2019-06-05 22:52:26

by Aaro Koskinen

[permalink] [raw]
Subject: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

Hi,

When upgrading from v5.0 -> v5.1 on G4 PowerBook, I noticed WLAN does
not work anymore:

[ 42.004303] b43legacy-phy0: Loading firmware version 0x127, patch level 14 (2005-04-18 02:36:27)
[ 42.184837] b43legacy-phy0 debug: Chip initialized
[ 42.184873] b43legacy-phy0 ERROR: The machine/kernel does not support the required 30-bit DMA mask

The same happens with the current mainline.

Bisected to:

commit 65a21b71f948406201e4f62e41f06513350ca390
Author: Christoph Hellwig <[email protected]>
Date: Wed Feb 13 08:01:26 2019 +0100

powerpc/dma: remove dma_nommu_dma_supported

This function is largely identical to the generic version used
everywhere else. Replace it with the generic version.

Signed-off-by: Christoph Hellwig <[email protected]>
Tested-by: Christian Zigotzky <[email protected]>
Signed-off-by: Michael Ellerman <[email protected]>

A.


2019-06-06 00:56:39

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Thu, 2019-06-06 at 01:50 +0300, Aaro Koskinen wrote:
> Hi,
>
> When upgrading from v5.0 -> v5.1 on G4 PowerBook, I noticed WLAN does
> not work anymore:
>
> [ 42.004303] b43legacy-phy0: Loading firmware version 0x127, patch level 14 (2005-04-18 02:36:27)
> [ 42.184837] b43legacy-phy0 debug: Chip initialized
> [ 42.184873] b43legacy-phy0 ERROR: The machine/kernel does not support the required 30-bit DMA mask
>
> The same happens with the current mainline.

How much RAM do you have ?

Ben.

>
> Bisected to:
>
> commit 65a21b71f948406201e4f62e41f06513350ca390
> Author: Christoph Hellwig <[email protected]>
> Date: Wed Feb 13 08:01:26 2019 +0100
>
> powerpc/dma: remove dma_nommu_dma_supported
>
> This function is largely identical to the generic version used
> everywhere else. Replace it with the generic version.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> Tested-by: Christian Zigotzky <[email protected]>
> Signed-off-by: Michael Ellerman <[email protected]>
>
> A.

2019-06-06 03:08:10

by Larry Finger

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On 6/5/19 5:50 PM, Aaro Koskinen wrote:
> Hi,
>
> When upgrading from v5.0 -> v5.1 on G4 PowerBook, I noticed WLAN does
> not work anymore:
>
> [ 42.004303] b43legacy-phy0: Loading firmware version 0x127, patch level 14 (2005-04-18 02:36:27)
> [ 42.184837] b43legacy-phy0 debug: Chip initialized
> [ 42.184873] b43legacy-phy0 ERROR: The machine/kernel does not support the required 30-bit DMA mask
>
> The same happens with the current mainline.
>
> Bisected to:
>
> commit 65a21b71f948406201e4f62e41f06513350ca390
> Author: Christoph Hellwig <[email protected]>
> Date: Wed Feb 13 08:01:26 2019 +0100
>
> powerpc/dma: remove dma_nommu_dma_supported
>
> This function is largely identical to the generic version used
> everywhere else. Replace it with the generic version.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> Tested-by: Christian Zigotzky <[email protected]>
> Signed-off-by: Michael Ellerman <[email protected]>

Aaro,

First of all, you have my sympathy for the laborious bisection on a PowerBook
G4. I have done several myself. Thank you.

I confirm your results.

The ppc code has a maximum DMA size of 31 bits, thus a 32-bit request will fail.
Why the 30-bit fallback fails in b43legacy fails while it works in b43 is a mystery.

Although dma_nommu_dma_supported() may be "largely identical" to
dma_direct_supported(), they obviously differ. Routine dma_nommu_dma_supported()
returns 1 for 32-bit systems, but I do not know what dma_direct_supported() returns.

I am trying to find a patch.

Larry

2019-06-06 06:42:03

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Wed, Jun 05, 2019 at 10:06:18PM -0500, Larry Finger wrote:
> First of all, you have my sympathy for the laborious bisection on a
> PowerBook G4. I have done several myself. Thank you.
>
> I confirm your results.
>
> The ppc code has a maximum DMA size of 31 bits, thus a 32-bit request will
> fail. Why the 30-bit fallback fails in b43legacy fails while it works in
> b43 is a mystery.
>
> Although dma_nommu_dma_supported() may be "largely identical" to
> dma_direct_supported(), they obviously differ. Routine
> dma_nommu_dma_supported() returns 1 for 32-bit systems, but I do not know
> what dma_direct_supported() returns.
>
> I am trying to find a patch.

if (IS_ENABLED(CONFIG_ZONE_DMA))
min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
else
min_mask = DMA_BIT_MASK(32);

min_mask = min_t(u64, min_mask, (max_pfn - 1) << PAGE_SHIFT);
return mask >= __phys_to_dma(dev, min_mask);

So the smaller or:

(1) 32-bit
(2) ARCH_ZONE_DMA_BITS
(3) the actual amount of memory in the system

modolo any DMA offsets that come into play.

No offsets should exists on pmac, and ARCH_ZONE_DMA_BITS is 31 on
powerpc. So unless the system has 1GB or less memory it will probably
return false for b43, because it can't actually guarantee reliable
allocation. It will work fine on x86 with the smaller ZONE_DMA.

2019-06-08 07:26:06

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Sat, Jun 08, 2019 at 02:21:23PM +1000, Benjamin Herrenschmidt wrote:
>
> > Please try the attached patch. I'm not really pleased with it and I will
> > continue to determine why the fallback to a 30-bit mask fails, but at least this
> > one works for me.
>
> Your patch only makes sense if the device is indeed capable of
> addressing 31-bits.
>
> So either the driver is buggy and asks for a too small mask in which
> case your patch is ok, or it's not and you're just going to cause all
> sort of interesting random problems including possible memory
> corruption.

Well, my patches changes ZONE_DMA to be 30-bits instead of 31, and
thus should allow requesting a 30-bit mask to succeed.

2019-06-10 08:20:29

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Sat, Jun 08, 2019 at 04:52:24PM -0500, Larry Finger wrote:
> On 6/7/19 12:29 PM, Christoph Hellwig wrote:
>> I don't think we should work around this in the driver, we need to fix
>> it in the core. I'm curious why my previous patch didn't work. Can
>> you throw in a few printks what failed? I.e. did dma_direct_supported
>> return false? Did the actual allocation fail?
>
> Routine dma_direct_supported() returns true.
>
> The failure is in routine dma_set_mask() in the following if test:
>
> if (!dev->dma_mask || !dma_supported(dev, mask))
> return -EIO;
>
> For b43legacy, dev->dma_mask is 0xc265684800000000.
> dma_supported(dev, mask) is 0xc08b000000000000, mask is 0x3fffffff, and
> the routine returns -EIO.
>
> For b43, dev->dma_mask is 0xc265684800000001,
> dma_supported(dev, mask) is 0xc08b000000000000, mask is 0x77777777, and
> the routine returns 0.

I don't fully understand what values the above map to. Can you send
me your actual debugging patch as well?

2019-06-10 18:45:12

by Larry Finger

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On 6/7/19 11:21 PM, Benjamin Herrenschmidt wrote:
>
>> Please try the attached patch. I'm not really pleased with it and I will
>> continue to determine why the fallback to a 30-bit mask fails, but at least this
>> one works for me.
>
> Your patch only makes sense if the device is indeed capable of
> addressing 31-bits.
>
> So either the driver is buggy and asks for a too small mask in which
> case your patch is ok, or it's not and you're just going to cause all
> sort of interesting random problems including possible memory
> corruption.

Of course the driver may be buggy, but it asks for the correct mask.

This particular device is not capable of handling 32-bit DMA. The driver detects
the 32-bit failure and falls back to 30 bits. It works on x86, and did on PPC32
until 5.1. As Christoph said, it should always be possible to use fewer bits
than the maximum.

Similar devices that are new enough to use b43 rather than b43legacy work with
new kernels; however, they have and use 32-bit DMA.

Larry

2019-06-11 05:57:19

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Mon, 2019-06-10 at 13:44 -0500, Larry Finger wrote:
> On 6/7/19 11:21 PM, Benjamin Herrenschmidt wrote:
> >
> > > Please try the attached patch. I'm not really pleased with it and I will
> > > continue to determine why the fallback to a 30-bit mask fails, but at least this
> > > one works for me.
> >
> > Your patch only makes sense if the device is indeed capable of
> > addressing 31-bits.
> >
> > So either the driver is buggy and asks for a too small mask in which
> > case your patch is ok, or it's not and you're just going to cause all
> > sort of interesting random problems including possible memory
> > corruption.
>
> Of course the driver may be buggy, but it asks for the correct mask.
>
> This particular device is not capable of handling 32-bit DMA. The driver detects
> the 32-bit failure and falls back to 30 bits. It works on x86, and did on PPC32
> until 5.1. As Christoph said, it should always be possible to use fewer bits
> than the maximum.

No, I don't think it *worked* on ppc32 before Christoph patch. I think
it "mostly sort-of worked" :-)

The reason I'm saying that is if your system has more than 1GB of RAM,
then you'll have chunks of memory that the device simply cannot
address.

Before Christoph patches, we had no ZONE_DMA or ZONE_DMA32 covering the
30-bit limited space, so any memory allocation could in theory land
above 30-bits, causing all sort of horrible things to happen with that
driver.

The reason I think it sort-of-mostly-worked is that to get more than
1GB of RAM, those machines use CONFIG_HIGHMEM. And *most* network
buffers aren't allocated in Highmem.... so you got lucky.

That said, there is such as thing as no-copy send on network, so I
wouldn't be surprised if some things would still have failed, just not
frequent enough for you to notice.

> Similar devices that are new enough to use b43 rather than b43legacy work with
> new kernels; however, they have and use 32-bit DMA.

Cheres,
Ben.


2019-06-11 06:07:24

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Mon, Jun 10, 2019 at 11:09:47AM -0500, Larry Finger wrote:
>>> return -EIO;
>>>
>>> For b43legacy, dev->dma_mask is 0xc265684800000000.
>>> dma_supported(dev, mask) is 0xc08b000000000000, mask is 0x3fffffff, and
>>> the routine returns -EIO.
>>>
>>> For b43, dev->dma_mask is 0xc265684800000001,
>>> dma_supported(dev, mask) is 0xc08b000000000000, mask is 0x77777777, and
>>> the routine returns 0.
>>
>> I don't fully understand what values the above map to. Can you send
>> me your actual debugging patch as well?
>
> I do not understand why the if statement returns true as neither of the
> values is zero. After seeing the x86 output shown below, I also do not
> understand all the trailing zeros.
>
> My entire patch is attached. That output came from this section:

What might be confusing in your output is that dev->dma_mask is a pointer,
and we are setting it in dma_set_mask. That is before we only check
if the pointer is set, and later we override it. Of course this doesn't
actually explain the failure. But what is even more strange to me
is that you get a return value from dma_supported() that isn't 0 or 1,
as that function is supposed to return a boolean, and I really can't see
how mask >= __phys_to_dma(dev, min_mask), would return anything but 0
or 1. Does the output change if you use the correct printk specifiers?

i.e. with a debug patch like this:


diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 2c2772e9702a..9e5b30b12b10 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -378,6 +378,7 @@ EXPORT_SYMBOL(dma_direct_map_resource);
int dma_direct_supported(struct device *dev, u64 mask)
{
u64 min_mask;
+ bool ret;

if (IS_ENABLED(CONFIG_ZONE_DMA))
min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
@@ -391,7 +392,12 @@ int dma_direct_supported(struct device *dev, u64 mask)
* use __phys_to_dma() here so that the SME encryption mask isn't
* part of the check.
*/
- return mask >= __phys_to_dma(dev, min_mask);
+ ret = (mask >= __phys_to_dma(dev, min_mask));
+ if (!ret)
+ dev_info(dev,
+ "%s: failed (mask = 0x%llx, min_mask = 0x%llx/0x%llx, dma bits = %d\n",
+ __func__, mask, min_mask, __phys_to_dma(dev, min_mask), ARCH_ZONE_DMA_BITS);
+ return ret;
}

size_t dma_direct_max_mapping_size(struct device *dev)
diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
index f7afdadb6770..6c57ccdee2ae 100644
--- a/kernel/dma/mapping.c
+++ b/kernel/dma/mapping.c
@@ -317,8 +317,14 @@ void arch_dma_set_mask(struct device *dev, u64 mask);

int dma_set_mask(struct device *dev, u64 mask)
{
- if (!dev->dma_mask || !dma_supported(dev, mask))
+ if (!dev->dma_mask) {
+ dev_info(dev, "no DMA mask set!\n");
return -EIO;
+ }
+ if (!dma_supported(dev, mask)) {
+ printk("DMA not supported\n");
+ return -EIO;
+ }

arch_dma_set_mask(dev, mask);
dma_check_mask(dev, mask);

2019-06-11 07:03:56

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Tue, 2019-06-11 at 16:58 +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2019-06-11 at 08:08 +0200, Christoph Hellwig wrote:
> > On Tue, Jun 11, 2019 at 03:56:33PM +1000, Benjamin Herrenschmidt
> > wrote:
> > > The reason I think it sort-of-mostly-worked is that to get more
> > > than
> > > 1GB of RAM, those machines use CONFIG_HIGHMEM. And *most* network
> > > buffers aren't allocated in Highmem.... so you got lucky.
> > >
> > > That said, there is such as thing as no-copy send on network, so I
> > > wouldn't be surprised if some things would still have failed, just
> > > not
> > > frequent enough for you to notice.
> >
> > Unless NETIF_F_HIGHDMA is set on a netdev, the core networkign code
> > will bounce buffer highmem pages for the driver under all
> > circumstances.
>
> ... which b43legacy doesn't set to the best of my knowledge ...
>
> Which makes me wonder how come it didn't work even with your patches ?
> AFAIK, we have less than 1GB of lowmem unless the config has been
> tweaked....

Ah stupid me ... it's dma_set_mask that failed, since it has no idea
that the calling driver is limited to lowmem.

That's also why the "wrong" patch worked.

So yes, a ZONE_DMA at 30-bits will work, though it's somewhat overkill.

Cheers,
Ben.


2019-06-11 08:10:22

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Tue, Jun 11, 2019 at 04:58:12PM +1000, Benjamin Herrenschmidt wrote:
> ... which b43legacy doesn't set to the best of my knowledge ...
>
> Which makes me wonder how come it didn't work even with your patches ?
> AFAIK, we have less than 1GB of lowmem unless the config has been
> tweaked....

It needs to bounce to somewhere. And the dma-direct code is pretty
strict to require a zone it can do allocations from when setting the
dma mask. As was the old ppc64 code, but not the ppc32 code that
allowed setting any DMA mask. And something about the more strict
validation seem to trip up now.

2019-06-11 08:11:27

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Tue, Jun 11, 2019 at 04:59:54PM +1000, Benjamin Herrenschmidt wrote:
> Ah stupid me ... it's dma_set_mask that failed, since it has no idea
> that the calling driver is limited to lowmem.
>
> That's also why the "wrong" patch worked.
>
> So yes, a ZONE_DMA at 30-bits will work, though it's somewhat overkill.

Well, according to Larry it doesn't actually work, which is odd.

2019-06-11 09:06:50

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Tue, 2019-06-11 at 09:54 +0200, Christoph Hellwig wrote:
> On Tue, Jun 11, 2019 at 04:59:54PM +1000, Benjamin Herrenschmidt
> wrote:
> > Ah stupid me ... it's dma_set_mask that failed, since it has no
> > idea
> > that the calling driver is limited to lowmem.
> >
> > That's also why the "wrong" patch worked.
> >
> > So yes, a ZONE_DMA at 30-bits will work, though it's somewhat
> > overkill.
>
> Well, according to Larry it doesn't actually work, which is odd.

Oh I assume that's just a glitch in the patch :-)

Cheers,
Ben.


2019-06-11 17:48:37

by Andreas Schwab

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Jun 10 2019, Larry Finger <[email protected]> wrote:

> I do not understand why the if statement returns true as neither of the
> values is zero.

That's because the format string does not make any sense. You are
printing garbage.

> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index f7afdad..ba2489d 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -317,9 +317,12 @@ int dma_supported(struct device *dev, u64 mask)
>
> int dma_set_mask(struct device *dev, u64 mask)
> {
> + pr_info("mask 0x%llx, dma_mask 0x%llx, dma_supported 0x%llx\n",
> mask, dev->dma_mask,
> + dma_supported(dev, mask));

None of the format directives match the type of the arguments.

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."

2019-06-12 04:52:15

by Larry Finger

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On 6/11/19 1:05 AM, Christoph Hellwig wrote:
> On Mon, Jun 10, 2019 at 11:09:47AM -0500, Larry Finger wrote:
>
> What might be confusing in your output is that dev->dma_mask is a pointer,
> and we are setting it in dma_set_mask. That is before we only check
> if the pointer is set, and later we override it. Of course this doesn't
> actually explain the failure. But what is even more strange to me
> is that you get a return value from dma_supported() that isn't 0 or 1,
> as that function is supposed to return a boolean, and I really can't see
> how mask >= __phys_to_dma(dev, min_mask), would return anything but 0
> or 1. Does the output change if you use the correct printk specifiers?
>
> i.e. with a debug patch like this:
>
>
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index 2c2772e9702a..9e5b30b12b10 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -378,6 +378,7 @@ EXPORT_SYMBOL(dma_direct_map_resource);
> int dma_direct_supported(struct device *dev, u64 mask)
> {
> u64 min_mask;
> + bool ret;
>
> if (IS_ENABLED(CONFIG_ZONE_DMA))
> min_mask = DMA_BIT_MASK(ARCH_ZONE_DMA_BITS);
> @@ -391,7 +392,12 @@ int dma_direct_supported(struct device *dev, u64 mask)
> * use __phys_to_dma() here so that the SME encryption mask isn't
> * part of the check.
> */
> - return mask >= __phys_to_dma(dev, min_mask);
> + ret = (mask >= __phys_to_dma(dev, min_mask));
> + if (!ret)
> + dev_info(dev,
> + "%s: failed (mask = 0x%llx, min_mask = 0x%llx/0x%llx, dma bits = %d\n",
> + __func__, mask, min_mask, __phys_to_dma(dev, min_mask), ARCH_ZONE_DMA_BITS);
> + return ret;
> }
>
> size_t dma_direct_max_mapping_size(struct device *dev)
> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c
> index f7afdadb6770..6c57ccdee2ae 100644
> --- a/kernel/dma/mapping.c
> +++ b/kernel/dma/mapping.c
> @@ -317,8 +317,14 @@ void arch_dma_set_mask(struct device *dev, u64 mask);
>
> int dma_set_mask(struct device *dev, u64 mask)
> {
> - if (!dev->dma_mask || !dma_supported(dev, mask))
> + if (!dev->dma_mask) {
> + dev_info(dev, "no DMA mask set!\n");
> return -EIO;
> + }
> + if (!dma_supported(dev, mask)) {
> + printk("DMA not supported\n");
> + return -EIO;
> + }
>
> arch_dma_set_mask(dev, mask);
> dma_check_mask(dev, mask);
>

After I got the correct formatting, the output with this patch only gives the
following in dmesg:

b43-pci-bridge 0001:11:00.0: dma_direct_supported: failed (mask = 0x3fffffff,
min_mask = 0x5ffff000/0x5ffff000, dma bits = 0x1f
DMA not supported
b43legacy-phy0 ERROR: The machine/kernel does not support the required 30-bit
DMA mask

Your first patch did not work as the configuration does not have
CONFIG_ZONE_DMA. As a result, the initial value of min_mask always starts at 32
bits and is taken down to 31 with the maximum pfn minimization. When I forced
the initial value of min_mask to 30 bits, the device worked.

It is obvious that the case of a mask smaller than min_mask should be handled by
the IOMMU. In my system, CONFIG_IOMMU_SUPPORT is selected. All other CONFIG
variables containing IOMMU are not selected. When dma_direct_supported() fails,
should the system not try for an IOMMU solution? Is the driver asking for the
wrong type of memory? It is doing a dma_and_set_mask_coherent() call.

Larry

2019-06-12 05:30:23

by Aaro Koskinen

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

Hi,

On Tue, Jun 11, 2019 at 05:20:12PM -0500, Larry Finger wrote:
> It is obvious that the case of a mask smaller than min_mask should be
> handled by the IOMMU. In my system, CONFIG_IOMMU_SUPPORT is selected. All
> other CONFIG variables containing IOMMU are not selected. When
> dma_direct_supported() fails, should the system not try for an IOMMU
> solution? Is the driver asking for the wrong type of memory? It is doing a
> dma_and_set_mask_coherent() call.

I don't think we have IOMMU on G4. On G5 it should work (I remember fixing
b43 issue on G5, see 4c374af5fdee, unfortunately all my G5 Macs with b43
are dead and waiting for re-capping).

A.

2019-06-12 05:33:07

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Tue, 2019-06-11 at 17:20 -0500, Larry Finger wrote:
> b43-pci-bridge 0001:11:00.0: dma_direct_supported: failed (mask =
> 0x3fffffff,
> min_mask = 0x5ffff000/0x5ffff000, dma bits = 0x1f

Ugh ? A mask with holes in it ? That's very wrong... That min_mask is
bogus.

Ben.


2019-06-12 07:26:04

by Larry Finger

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On 6/11/19 5:46 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2019-06-11 at 17:20 -0500, Larry Finger wrote:
>> b43-pci-bridge 0001:11:00.0: dma_direct_supported: failed (mask =
>> 0x3fffffff,
>> min_mask = 0x5ffff000/0x5ffff000, dma bits = 0x1f
>
> Ugh ? A mask with holes in it ? That's very wrong... That min_mask is
> bogus.

I agree, but that is not likely serious as most systems will have enough memory
that the max_pfn term will be much larger than the initial min_mask, and
min_mask will be unchanged by the min function. In addition, min_mask is not
used beyond this routine, and then only to decide if direct dma is supported.
The following patch generates masks with no holes, but I cannot see that it is
needed.

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 2c2772e9702a..e3edd4f29e80 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -384,7 +384,8 @@ int dma_direct_supported(struct device *dev, u64 mask)
else
min_mask = DMA_BIT_MASK(32);

- min_mask = min_t(u64, min_mask, (max_pfn - 1) << PAGE_SHIFT);
+ min_mask = min_t(u64, min_mask, ((max_pfn - 1) << PAGE_SHIFT) |
+ DMA_BIT_MASK(PAGE_SHIFT));

/*
* This check needs to be against the actual bit mask value, so


Larry

2019-06-12 08:30:24

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On Tue, Jun 11, 2019 at 05:20:12PM -0500, Larry Finger wrote:
> Your first patch did not work as the configuration does not have
> CONFIG_ZONE_DMA. As a result, the initial value of min_mask always starts
> at 32 bits and is taken down to 31 with the maximum pfn minimization. When
> I forced the initial value of min_mask to 30 bits, the device worked.

Ooops, yes. But I think we could just enable ZONE_DMA on 32-bit
powerpc. Crude enablement hack below:

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 8c1c636308c8..1dd71a98b70c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -372,7 +372,7 @@ config PPC_ADV_DEBUG_DAC_RANGE

config ZONE_DMA
bool
- default y if PPC_BOOK3E_64
+ default y

config PGTABLE_LEVELS
int

2019-06-12 19:42:35

by Larry Finger

[permalink] [raw]
Subject: Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook

On 6/12/19 1:55 AM, Christoph Hellwig wrote:
>
> Ooops, yes. But I think we could just enable ZONE_DMA on 32-bit
> powerpc. Crude enablement hack below:
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 8c1c636308c8..1dd71a98b70c 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -372,7 +372,7 @@ config PPC_ADV_DEBUG_DAC_RANGE
>
> config ZONE_DMA
> bool
> - default y if PPC_BOOK3E_64
> + default y
>
> config PGTABLE_LEVELS
> int
>

With the patch for Kconfig above, and the original patch setting
ARCH_ZONE_DMA_BITS to 30, everything works.

Do you have any ideas on what should trigger the change in ARCH_ZONE_BITS?
Should it be CONFIG_PPC32 defined, or perhaps CONFIG_G4_CPU defined?

Larry