Re: [PATCH v7 10/50] powerpc/powernv: Simplify pnv_ioda_setup_pe_seg()

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, Nov 16, 2015 at 07:01:18PM +1100, Alexey Kardashevskiy wrote:
>On 11/06/2015 10:52 AM, Gavin Shan wrote:
>>On Fri, Nov 06, 2015 at 09:56:06AM +1100, Daniel Axtens wrote:
>>>Gavin Shan <gwshan@linux.vnet.ibm.com> writes:
>>>
>>>>The original implementation of pnv_ioda_setup_pe_seg() configures
>>>>IO and M32 segments by separate logics, which can be merged by
>>>>by caching @segmap, @seg_size, @win in advance. This shouldn't
>>>>cause any behavioural changes.
>>>>
>>>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>---
>>>>  arch/powerpc/platforms/powernv/pci-ioda.c | 62 ++++++++++++++-----------------
>>>>  1 file changed, 28 insertions(+), 34 deletions(-)
>>>>
>>>>diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>index 7ee7cfe..553d3f3 100644
>>>>--- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>@@ -2752,8 +2752,10 @@ static void pnv_ioda_setup_pe_seg(struct pci_controller *hose,
>>>>  	struct pnv_phb *phb = hose->private_data;
>>>>  	struct pci_bus_region region;
>>>>  	struct resource *res;
>>>>-	int i, index;
>>>>-	int rc;
>>>>+	unsigned int segsize;
>>>>+	int *segmap, index, i;
>>>>+	uint16_t win;
>>>>+	int64_t rc;
>>>
>>>Good catch! Opal return codes are 64 bit and that should be explicit
>>>in the type. However, I seem to remember that we preferred a different
>>>type for 64 bit ints in the kernel. I think it's s64, and there are some
>>>other uses of that in pci_ioda.c for return codes.
>>>
>>
>>Both int64_t and s64 are fine. I used s64 for the OPAL return value, but
>>Alexey likes "int64_t", which is ok to me as well. I won't change it back
>>to s64 :-)
>>
>>>(I'm actually surprised that's not picked up as a compiler
>>>warning. Maybe that's something to look at in future.)
>>>
>>
>>Indeed, I didn't see a warning from gcc.
>>
>>>The rest of the patch looks good on casual inspection - to be sure I'll
>>>test the entire series on a machine. (hopefully, time permitting!)
>>>
>>
>>I run scripts/checkpatch.pl on the patchset. Only one warning came from
>>[PATCH 44/50], but I won't bother to change that as the warning was
>>brought by original code.
>
>None of these patches failed checkpatch.pl check, what was the error in 44/50?
>

You're right that none of those patches failed with checkpatch.pl. I had revison
6.1, 6.2, 6.3 before this revision (v7) was posted. There was a warning for 44/50
in 6.3 (perhaps).

I run checkpatch.pl on all (v7) patches, no warning reported.

Thanks,
Gavin