[PATCH 1/2] PCI: iproc: fix 32-bit build

[PATCH 1/2] PCI: iproc: fix 32-bit build

[Arnd Bergmann]

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

[PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning

[Arnd Bergmann]

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

Re: [PATCH 1/2] PCI: iproc: fix 32-bit build

[Ray Jui]

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>

Re: [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning

[Ray Jui]

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

Re: [PATCH 1/2] PCI: iproc: fix 32-bit build

[Arnd Bergmann]

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

Re: [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning

[Arnd Bergmann]

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

Re: [PATCH 1/2] PCI: iproc: fix 32-bit build

[Ray Jui]

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

Re: [PATCH 2/2] PCI: iproc: avoid maybe-uninitialized warning

[Ray Jui]

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

Re: [PATCH 1/2] PCI: iproc: fix 32-bit build

[Bjorn Helgaas]

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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel