Subject: Re: [PATCH v8 08/45] powerpc/powernv: Fix initial IO and M32 segmap

From: Alexey Kardashevskiy <[email-masked]>
Date: Wed, 13 Apr 2016 16:21:07 +1000

On 02/17/2016 02:43 PM, Gavin Shan wrote:
> There are two arrays for IO and M32 segment maps on every PHB.
> The index of the arrays are segment number and the value stored
> in the corresponding element is PE number, indicating the segment
> is assigned to the PE. Initially, all elements in those two arrays
> are zeroes, meaning all segments are assigned to PE#0. It's wrong.
 >
> This fixes the initial values in the elements of those two arrays
> to IODA_INVALID_PE, meaning all segments aren't assigned to any
> PE.

This is ok.

> In order to use IODA_INVALID_PE (-1) to represent invalid PE
> number, the types of those two arrays are changed from "unsigned int"
> to "int".

"unsigned" can carry (-1) perfectly fine, just add a type cast to 
IODA_INVALID_PE:

#define IODA_INVALID_PE    (unsigned int)(-1)

Using "signed" type for indexes which cannot be negative does not make much 
sense - instead of checking for the upper boundary, you have to check for 
"< 0" too.

OPAL uses unsigned type for PE (uint64_t or uint32_t or uint16_t - this is 
quite funny).

pnv_ioda_pe::pe_number is "unsigned" and this pe_number is the same thing 
as I can see in pnv_ioda_setup_dev_PE().

Some printk() print the PE number as "%x" (which implies "unsigned").

I suggest changing the pci_dn::pe_number type from "int" to "unsigned int" 
to match pnv_ioda_pe::pe_number, in a separate patch. Or do not touch types 
for now.

> Signed-off-by: Gavin Shan <[email-masked]>
> ---
>   arch/powerpc/platforms/powernv/pci-ioda.c | 9 +++++++--
>   arch/powerpc/platforms/powernv/pci.h      | 4 ++--
>   2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> index 1d2514f..44cc5f3 100644
> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> @@ -3239,7 +3239,7 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
>   	unsigned long size, m32map_off, pemap_off, iomap_off = 0;
>   	const __be64 *prop64;
>   	const __be32 *prop32;
> -	int len;
> +	int i, len;
>   	u64 phb_id;
>   	void *aux;
>   	long rc;
> @@ -3334,8 +3334,13 @@ static void __init pnv_pci_init_ioda_phb(struct device_node *np,
>   	aux = memblock_virt_alloc(size, 0);
>   	phb->ioda.pe_alloc = aux;
>   	phb->ioda.m32_segmap = aux + m32map_off;
> -	if (phb->type == PNV_PHB_IODA1)
> +	for (i = 0; i < phb->ioda.total_pe_num; i++)
> +		phb->ioda.m32_segmap[i] = IODA_INVALID_PE;
> +	if (phb->type == PNV_PHB_IODA1) {
>   		phb->ioda.io_segmap = aux + iomap_off;
> +		for (i = 0; i < phb->ioda.total_pe_num; i++)
> +			phb->ioda.io_segmap[i] = IODA_INVALID_PE;
> +	}
>   	phb->ioda.pe_array = aux + pemap_off;
>   	set_bit(phb->ioda.reserved_pe_idx, phb->ioda.pe_alloc);
>
> diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
> index 784882a..36c4965 100644
> --- a/arch/powerpc/platforms/powernv/pci.h
> +++ b/arch/powerpc/platforms/powernv/pci.h
> @@ -146,8 +146,8 @@ struct pnv_phb {
>   		struct pnv_ioda_pe	*pe_array;
>
>   		/* M32 & IO segment maps */
> -		unsigned int		*m32_segmap;
> -		unsigned int		*io_segmap;
> +		int			*m32_segmap;
> +		int			*io_segmap;
>
>   		/* IRQ chip */
>   		int			irq_chip_init;
>

-- 
Alexey