Re: [PATCH kernel v5 14/20] powerpc/powernv/npu: Add compound IOMMU groups

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 19/12/2018 21:00, Michael Ellerman wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> On 19/12/2018 11:17, Michael Ellerman wrote:
>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
>>>> index dc629ee..3468eaa 100644
>>>> --- a/arch/powerpc/platforms/powernv/npu-dma.c
>>>> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
>>>> @@ -372,8 +358,263 @@ struct npu {
>>> ...
>>>> +
>>>> +static void pnv_comp_attach_table_group(struct npu_comp *npucomp,
>>>> +		struct pnv_ioda_pe *pe)
>>>> +{
>>>> +	if (WARN_ON(npucomp->pe_num == NV_NPU_MAX_PE_NUM))
>>>> +		return;
>>>> +
>>>> +	npucomp->pe[npucomp->pe_num] = pe;
>>>> +	++npucomp->pe_num;
>>>> +}
>>>> +
>>>> +struct iommu_table_group *pnv_try_setup_npu_table_group(struct pnv_ioda_pe *pe)
>>>> +{
>>>> +	struct iommu_table_group *table_group;
>>>> +	struct npu_comp *npucomp;
>>>> +	struct pci_dev *gpdev = NULL;
>>>> +	struct pci_controller *hose;
>>>> +	struct pci_dev *npdev;
>>>> +
>>>> +	list_for_each_entry(gpdev, &pe->pbus->devices, bus_list) {
>>>> +		npdev = pnv_pci_get_npu_dev(gpdev, 0);
>>>> +		if (npdev)
>>>> +			break;
>>>> +	}
>>>> +
>>>> +	if (!npdev)
>>>> +		/* It is not an NPU attached device, skip */
>>>> +		return NULL;
>>>
>>> This breaks some configs with:
>>>
>>>   arch/powerpc/platforms/powernv/npu-dma.c:550:5: error: 'npdev' may be used uninitialized in this function [-Werror=uninitialized]
>>
>>
>> gcc 5, 7 and 8 do not warn about this, I have to disable
>> list_for_each_entry() above to recreate this.
>>
>> I even compiled gcc 5.5 which some of your buildmachines use and yet no
>> error on this:
>>
>> make O=/home/aik/pbuild/kernel-le/ KCFLAGS=-Werror=all ARCH=powerpc
>> CROSS_COMPILE=/opt/cross/gcc-powerpc64le-linux-5.5.0-nolibc/bin/powerpc64le-linux-
>> arch/powerpc/platforms/powernv/npu-dma.o
> 
> Odd. That error is from kisskb like the others.
> 
> http://kisskb.ellerman.id.au/kisskb/buildresult/13622793/
> 
> Seems it's GCC 4.6.3 that is producing that one. Not sure why newer
> compilers aren't warning about it.
> 
> 
> It's pretty obviously correct though, unless you can prove that the list
> is never empty?

I know these are correct and I want to catch these before I post patches :-/

Can I get that gcc 4.6.3? It does not compile on my ubuntu for whatever
reason.


> 
> 	struct pci_dev *npdev;
> 
> 	list_for_each_entry(gpdev, &pe->pbus->devices, bus_list) {
> 		npdev = pnv_pci_get_npu_dev(gpdev, 0);
> 		if (npdev)
> 			break;
> 	}
> 
> 	if (!npdev)
> 		/* It is not an NPU attached device, skip */
> 		return NULL;
> 
> 
> cheers
> 

-- 
Alexey