Re: [PATCH kernel v4 04/19] powerpc/powernv: Move npu struct from pnv_phb to pci_controller

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5476 bytes --]

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 <aik@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?

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]