On 06/12/2018 09:40, David Gibson wrote:
> On Wed, Dec 05, 2018 at 05:17:57PM +1100, Alexey Kardashevskiy wrote:
>>
>>
>> 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...
>
> More to the point, shouldn't you then fail immediately, rather than
> leaving the NULL floating around for later code?
Not sure I am following. pnv_npu2_init() is called at boot time so
failing-and-not-leaving-null-pointer here means panic and I definitely
do not want that. I am adding !npu checks in the next respin though,
pretty much replacing firmware_has_feature(FW_FEATURE_OPAL), do i miss
anything here?
--
Alexey
