On 05/12/2018 16:47, Alexey Kardashevskiy wrote:
>
>
> On 05/12/2018 16:14, David Gibson wrote:
>> On Fri, Nov 23, 2018 at 04:52:49PM +1100, Alexey Kardashevskiy wrote:
>>> The powernv PCI code stores NPU data in the pnv_phb struct. The latter
>>> is referenced by pci_controller::private_data. We are going to have NPU2
>>> support in the pseries platform as well but it does not store any
>>> private_data in in the pci_controller struct; and even if it did,
>>> it would be a different data structure.
>>>
>>> This makes npu a pointer and stores it one level higher in
>>> the pci_controller struct.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru>
>>> ---
>>> Changes:
>>> v4:
>>> * changed subj from "powerpc/powernv: Detach npu struct from pnv_phb"
>>> * got rid of global list of npus - store them now in pci_controller
>>> * got rid of npdev_to_npu() helper
>>> ---
>>> arch/powerpc/include/asm/pci-bridge.h | 1 +
>>> arch/powerpc/platforms/powernv/pci.h | 16 -----
>>> arch/powerpc/platforms/powernv/npu-dma.c | 81 ++++++++++++++++++------
>>> 3 files changed, 64 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/pci-bridge.h
>>> b/arch/powerpc/include/asm/pci-bridge.h
>>> index 94d4490..aee4fcc 100644
>>> --- a/arch/powerpc/include/asm/pci-bridge.h
>>> +++ b/arch/powerpc/include/asm/pci-bridge.h
>>> @@ -129,6 +129,7 @@ struct pci_controller {
>>> #endif /* CONFIG_PPC64 */
>>>
>>> void *private_data;
>>> + struct npu *npu;
>>> };
>>>
>>> /* These are used for config access before all the PCI probing
>>> diff --git a/arch/powerpc/platforms/powernv/pci.h
>>> b/arch/powerpc/platforms/powernv/pci.h
>>> index 2131373..f2d50974 100644
>>> --- a/arch/powerpc/platforms/powernv/pci.h
>>> +++ b/arch/powerpc/platforms/powernv/pci.h
>>> @@ -8,9 +8,6 @@
>>>
>>> struct pci_dn;
>>>
>>> -/* Maximum possible number of ATSD MMIO registers per NPU */
>>> -#define NV_NMMU_ATSD_REGS 8
>>> -
>>> enum pnv_phb_type {
>>> PNV_PHB_IODA1 = 0,
>>> PNV_PHB_IODA2 = 1,
>>> @@ -176,19 +173,6 @@ struct pnv_phb {
>>> unsigned int diag_data_size;
>>> u8 *diag_data;
>>>
>>> - /* Nvlink2 data */
>>> - struct npu {
>>> - int index;
>>> - __be64 *mmio_atsd_regs[NV_NMMU_ATSD_REGS];
>>> - unsigned int mmio_atsd_count;
>>> -
>>> - /* Bitmask for MMIO register usage */
>>> - unsigned long mmio_atsd_usage;
>>> -
>>> - /* Do we need to explicitly flush the nest mmu? */
>>> - bool nmmu_flush;
>>> - } npu;
>>> -
>>> int p2p_target_count;
>>> };
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/npu-dma.c
>>> b/arch/powerpc/platforms/powernv/npu-dma.c
>>> index 91d488f..7dd5c0e5 100644
>>> --- a/arch/powerpc/platforms/powernv/npu-dma.c
>>> +++ b/arch/powerpc/platforms/powernv/npu-dma.c
>>> @@ -327,6 +327,25 @@ struct pnv_ioda_pe *pnv_pci_npu_setup_iommu(struct
>>> pnv_ioda_pe *npe)
>>> return gpe;
>>> }
>>>
>>> +/*
>>> + * NPU2 ATS
>>> + */
>>> +/* Maximum possible number of ATSD MMIO registers per NPU */
>>> +#define NV_NMMU_ATSD_REGS 8
>>> +
>>> +/* An NPU descriptor, valid for POWER9 only */
>>> +struct npu {
>>> + int index;
>>> + __be64 *mmio_atsd_regs[NV_NMMU_ATSD_REGS];
>>> + unsigned int mmio_atsd_count;
>>> +
>>> + /* Bitmask for MMIO register usage */
>>> + unsigned long mmio_atsd_usage;
>>> +
>>> + /* Do we need to explicitly flush the nest mmu? */
>>> + bool nmmu_flush;
>>> +};
>>> +
>>> /* Maximum number of nvlinks per npu */
>>> #define NV_MAX_LINKS 6
>>>
>>> @@ -478,7 +497,6 @@ static void acquire_atsd_reg(struct npu_context
>>> *npu_context,
>>> int i, j;
>>> struct npu *npu;
>>> struct pci_dev *npdev;
>>> - struct pnv_phb *nphb;
>>>
>>> for (i = 0; i <= max_npu2_index; i++) {
>>> mmio_atsd_reg[i].reg = -1;
>>> @@ -493,8 +511,10 @@ static void acquire_atsd_reg(struct npu_context
>>> *npu_context,
>>> if (!npdev)
>>> continue;
>>>
>>> - nphb = pci_bus_to_host(npdev->bus)->private_data;
>>> - npu = &nphb->npu;
>>> + npu = pci_bus_to_host(npdev->bus)->npu;
>>> + if (!npu)
>>> + continue;
>>
>> This patch changes a bunch of places that used to unconditionally
>> locate an NPU now have a failure path.
>>
>> Given that this used to always have an NPU, doesn't that mean that if
>> the NPU is not present something has already gone wrong, and we should
>> WARN_ON() or something?
>
>
>
> That means this is a leftover since I dropped that npdev_to_npu helper
> which could help but there was no real value in it. I'll remove the
> check here in the next respin.
Well, technically kmalloc() can fail in pnv_npu2_init() (but not later)
so can (in theory) end up with an NPU PHB and npu==NULL but it is sooo
unlikely...
>
> I'll probably add checks for npu!=NULL where we used to have
> firmware_has_feature(FW_FEATURE_OPAL) in 05/19.
--
Alexey
