On Mon, Dec 10, 2018 at 01:50:35PM +1100, Alexey Kardashevskiy wrote:
>
>
> 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.
Well, if it's a choice between panic then and panic later, I'm not so
sure that's true.
> I am adding !npu checks in the next respin though,
> pretty much replacing firmware_has_feature(FW_FEATURE_OPAL), do i miss
> anything here?
That seems reasonable.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
