* [PATCH 1/2] PCI: iproc: fix 32-bit build
@ 2016-11-22 14:17 Arnd Bergmann
2016-11-22 14:17 ` [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning Arnd Bergmann
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-22 14:17 UTC (permalink / raw)
To: linux-arm-kernel
The newly added code to setup the inbound ranges causes a link error
on 32-bit machines from a 32-bit division:
drivers/pci/host/pcie-iproc.o: In function `iproc_pcie_setup_ib':
pcie-iproc.c:(.text.iproc_pcie_setup_ib+0x14c): undefined reference to `__aeabi_uldivmod'
As both sides of the division are always power-of-two numbers and
we already rely on that, we can use a shift instead.
Fixes: 87c240b19bba ("PCI: iproc: Add inbound DMA mapping support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/pci/host/pcie-iproc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index d10e6aa32e0d..857ff5198317 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -865,7 +865,7 @@ static int iproc_pcie_ib_write(struct iproc_pcie *pcie, int region_idx,
* Now program the IMAP registers. Each IARR region may have one or
* more IMAP windows.
*/
- size /= nr_windows;
+ size >>= ilog2(nr_windows);
for (window_idx = 0; window_idx < nr_windows; window_idx++) {
val = readl(pcie->base + imap_offset);
val |= lower_32_bits(axi_addr) | IMAP_VALID;
--
2.9.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning
2016-11-22 14:17 [PATCH 1/2] PCI: iproc: fix 32-bit build Arnd Bergmann
@ 2016-11-22 14:17 ` Arnd Bergmann
2016-11-22 17:45 ` Ray Jui
2016-11-22 17:42 ` [PATCH 1/2] PCI: iproc: fix 32-bit build Ray Jui
2016-11-23 22:53 ` Bjorn Helgaas
2 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-22 14:17 UTC (permalink / raw)
To: linux-arm-kernel
gcc notices that calling iproc_pcie_setup_ib with ib->nr_regions==0
would result in an uninitialized return value:
drivers/pci/host/pcie-iproc.c: In function 'iproc_pcie_setup_ib':
drivers/pci/host/pcie-iproc.c:894:6: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
This can't really happen, but the correct behavior of the function
is probably to return -EINVAL if it ever did.
Fixes: 4213e15c364e ("PCI: iproc: Make outbound mapping code more generic")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/pci/host/pcie-iproc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 857ff5198317..0359569c8d78 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -936,6 +936,7 @@ static int iproc_pcie_setup_ib(struct iproc_pcie *pcie,
}
}
+ ret = -EINVAL;
err_ib:
dev_err(dev, "unable to configure inbound mapping\n");
dev_err(dev, "axi %pap, pci %pap, res size %pap\n",
--
2.9.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning
2016-11-22 14:17 ` [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning Arnd Bergmann
@ 2016-11-22 17:45 ` Ray Jui
2016-11-22 21:15 ` Arnd Bergmann
0 siblings, 1 reply; 9+ messages in thread
From: Ray Jui @ 2016-11-22 17:45 UTC (permalink / raw)
To: linux-arm-kernel
Hi Arnd,
On 11/22/2016 6:17 AM, Arnd Bergmann wrote:
> gcc notices that calling iproc_pcie_setup_ib with ib->nr_regions==0
> would result in an uninitialized return value:
>
> drivers/pci/host/pcie-iproc.c: In function 'iproc_pcie_setup_ib':
> drivers/pci/host/pcie-iproc.c:894:6: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This can't really happen, but the correct behavior of the function
> is probably to return -EINVAL if it ever did.
>
> Fixes: 4213e15c364e ("PCI: iproc: Make outbound mapping code more generic")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/pci/host/pcie-iproc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index 857ff5198317..0359569c8d78 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -936,6 +936,7 @@ static int iproc_pcie_setup_ib(struct iproc_pcie *pcie,
>
> }
> }
> + ret = -EINVAL;
> err_ib:
> dev_err(dev, "unable to configure inbound mapping\n");
> dev_err(dev, "axi %pap, pci %pap, res size %pap\n",
>
This change is good, but in my opinion, a further improvement for
clarity would be to initialize 'ret' to -EINVAL in the beginning of this
function when 'ret' is declared. What do you think?
Thanks,
Ray
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning
2016-11-22 17:45 ` Ray Jui
@ 2016-11-22 21:15 ` Arnd Bergmann
2016-11-23 3:00 ` Ray Jui
0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-22 21:15 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday, November 22, 2016 9:45:24 AM CET Ray Jui wrote:
> > diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> > index 857ff5198317..0359569c8d78 100644
> > --- a/drivers/pci/host/pcie-iproc.c
> > +++ b/drivers/pci/host/pcie-iproc.c
> > @@ -936,6 +936,7 @@ static int iproc_pcie_setup_ib(struct iproc_pcie *pcie,
> >
> > }
> > }
> > + ret = -EINVAL;
> > err_ib:
> > dev_err(dev, "unable to configure inbound mapping\n");
> > dev_err(dev, "axi %pap, pci %pap, res size %pap\n",
> >
>
> This change is good, but in my opinion, a further improvement for
> clarity would be to initialize 'ret' to -EINVAL in the beginning of this
> function when 'ret' is declared. What do you think?
>
I never do that, see https://rusty.ozlabs.org/?p=232 for a great
explanation about why.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning
2016-11-22 21:15 ` Arnd Bergmann
@ 2016-11-23 3:00 ` Ray Jui
0 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2016-11-23 3:00 UTC (permalink / raw)
To: linux-arm-kernel
On 11/22/2016 1:15 PM, Arnd Bergmann wrote:
> On Tuesday, November 22, 2016 9:45:24 AM CET Ray Jui wrote:
>>> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
>>> index 857ff5198317..0359569c8d78 100644
>>> --- a/drivers/pci/host/pcie-iproc.c
>>> +++ b/drivers/pci/host/pcie-iproc.c
>>> @@ -936,6 +936,7 @@ static int iproc_pcie_setup_ib(struct iproc_pcie *pcie,
>>>
>>> }
>>> }
>>> + ret = -EINVAL;
>>> err_ib:
>>> dev_err(dev, "unable to configure inbound mapping\n");
>>> dev_err(dev, "axi %pap, pci %pap, res size %pap\n",
>>>
>>
>> This change is good, but in my opinion, a further improvement for
>> clarity would be to initialize 'ret' to -EINVAL in the beginning of this
>> function when 'ret' is declared. What do you think?
>>
>
> I never do that, see https://rusty.ozlabs.org/?p=232 for a great
> explanation about why.
>
> Arnd
>
Okay got it. Thanks!
Ray
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] PCI: iproc: fix 32-bit build
2016-11-22 14:17 [PATCH 1/2] PCI: iproc: fix 32-bit build Arnd Bergmann
2016-11-22 14:17 ` [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning Arnd Bergmann
@ 2016-11-22 17:42 ` Ray Jui
2016-11-22 21:13 ` Arnd Bergmann
2016-11-23 22:53 ` Bjorn Helgaas
2 siblings, 1 reply; 9+ messages in thread
From: Ray Jui @ 2016-11-22 17:42 UTC (permalink / raw)
To: linux-arm-kernel
On 11/22/2016 6:17 AM, Arnd Bergmann wrote:
> The newly added code to setup the inbound ranges causes a link error
> on 32-bit machines from a 32-bit division:
>
> drivers/pci/host/pcie-iproc.o: In function `iproc_pcie_setup_ib':
> pcie-iproc.c:(.text.iproc_pcie_setup_ib+0x14c): undefined reference to `__aeabi_uldivmod'
>
> As both sides of the division are always power-of-two numbers and
> we already rely on that, we can use a shift instead.
>
> Fixes: 87c240b19bba ("PCI: iproc: Add inbound DMA mapping support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/pci/host/pcie-iproc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index d10e6aa32e0d..857ff5198317 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -865,7 +865,7 @@ static int iproc_pcie_ib_write(struct iproc_pcie *pcie, int region_idx,
> * Now program the IMAP registers. Each IARR region may have one or
> * more IMAP windows.
> */
> - size /= nr_windows;
> + size >>= ilog2(nr_windows);
> for (window_idx = 0; window_idx < nr_windows; window_idx++) {
> val = readl(pcie->base + imap_offset);
> val |= lower_32_bits(axi_addr) | IMAP_VALID;
>
Hmmm, somehow we've never seen this link error for the ARM32 based
platforms that we build for. Does it behave differently between
different versions of compilers?
Nevertheless, this is a good change to take, thanks!
Acked-by: Ray Jui <ray.jui@broadcom.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] PCI: iproc: fix 32-bit build
2016-11-22 17:42 ` [PATCH 1/2] PCI: iproc: fix 32-bit build Ray Jui
@ 2016-11-22 21:13 ` Arnd Bergmann
2016-11-23 2:59 ` Ray Jui
0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2016-11-22 21:13 UTC (permalink / raw)
To: linux-arm-kernel
On Tuesday, November 22, 2016 9:42:05 AM CET Ray Jui wrote:
>
> Hmmm, somehow we've never seen this link error for the ARM32 based
> platforms that we build for. Does it behave differently between
> different versions of compilers?
>
> Nevertheless, this is a good change to take, thanks!
I looked at it again, and see now that it only happens with
a 64-bit RESOURCE_SIZE_T, which is only used on 32-bit
platforms with more than 4GB of memory addresses. It would
however show up in a multiplatform distro build for ARMv7VE,
so the fix is still clearly needed.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] PCI: iproc: fix 32-bit build
2016-11-22 21:13 ` Arnd Bergmann
@ 2016-11-23 2:59 ` Ray Jui
0 siblings, 0 replies; 9+ messages in thread
From: Ray Jui @ 2016-11-23 2:59 UTC (permalink / raw)
To: linux-arm-kernel
On 11/22/2016 1:13 PM, Arnd Bergmann wrote:
> On Tuesday, November 22, 2016 9:42:05 AM CET Ray Jui wrote:
>>
>> Hmmm, somehow we've never seen this link error for the ARM32 based
>> platforms that we build for. Does it behave differently between
>> different versions of compilers?
>>
>> Nevertheless, this is a good change to take, thanks!
>
> I looked at it again, and see now that it only happens with
> a 64-bit RESOURCE_SIZE_T, which is only used on 32-bit
> platforms with more than 4GB of memory addresses. It would
> however show up in a multiplatform distro build for ARMv7VE,
> so the fix is still clearly needed.
>
> Arnd
>
Yes, got it for how it showed up in your environment, :)
This change stands good as it has been.
Thanks,
Ray
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] PCI: iproc: fix 32-bit build
2016-11-22 14:17 [PATCH 1/2] PCI: iproc: fix 32-bit build Arnd Bergmann
2016-11-22 14:17 ` [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning Arnd Bergmann
2016-11-22 17:42 ` [PATCH 1/2] PCI: iproc: fix 32-bit build Ray Jui
@ 2016-11-23 22:53 ` Bjorn Helgaas
2 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2016-11-23 22:53 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Nov 22, 2016 at 03:17:51PM +0100, Arnd Bergmann wrote:
> The newly added code to setup the inbound ranges causes a link error
> on 32-bit machines from a 32-bit division:
>
> drivers/pci/host/pcie-iproc.o: In function `iproc_pcie_setup_ib':
> pcie-iproc.c:(.text.iproc_pcie_setup_ib+0x14c): undefined reference to `__aeabi_uldivmod'
>
> As both sides of the division are always power-of-two numbers and
> we already rely on that, we can use a shift instead.
>
> Fixes: 87c240b19bba ("PCI: iproc: Add inbound DMA mapping support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
I folded these fixes into the pci/host-iproc branch for v4.10, thanks!
For some reason, I don't see Ray's responses on the list. Maybe still
some email problem?
> ---
> drivers/pci/host/pcie-iproc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> index d10e6aa32e0d..857ff5198317 100644
> --- a/drivers/pci/host/pcie-iproc.c
> +++ b/drivers/pci/host/pcie-iproc.c
> @@ -865,7 +865,7 @@ static int iproc_pcie_ib_write(struct iproc_pcie *pcie, int region_idx,
> * Now program the IMAP registers. Each IARR region may have one or
> * more IMAP windows.
> */
> - size /= nr_windows;
> + size >>= ilog2(nr_windows);
> for (window_idx = 0; window_idx < nr_windows; window_idx++) {
> val = readl(pcie->base + imap_offset);
> val |= lower_32_bits(axi_addr) | IMAP_VALID;
> --
> 2.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-23 22:53 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-22 14:17 [PATCH 1/2] PCI: iproc: fix 32-bit build Arnd Bergmann
2016-11-22 14:17 ` [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning Arnd Bergmann
2016-11-22 17:45 ` Ray Jui
2016-11-22 21:15 ` Arnd Bergmann
2016-11-23 3:00 ` Ray Jui
2016-11-22 17:42 ` [PATCH 1/2] PCI: iproc: fix 32-bit build Ray Jui
2016-11-22 21:13 ` Arnd Bergmann
2016-11-23 2:59 ` Ray Jui
2016-11-23 22:53 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox