| Message ID | 20190426124917.23789-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] powerpc/powernv: remove the unused pnv_pci_set_p2p function | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (26ad26718dfaa7cf49d106d212ebf2370076c253) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 126 lines checked |
Hi, The PCI p2p and tunnel code is used by the Mellanox CX5 driver, at least their latest, out of tree version, which is used for CORAL. My understanding is that they'll upstream it at some point, though I don't know what their schedule is like. Fred Le 26/04/2019 à 14:49, Christoph Hellwig a écrit : > This function has never been used since it has been added to the tree. > We also now have proper PCIe P2P APIs in the core kernel, and any new > P2P support should be using those. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/powerpc/include/asm/opal.h | 2 - > arch/powerpc/include/asm/pnv-pci.h | 2 - > arch/powerpc/platforms/powernv/opal-call.c | 1 - > arch/powerpc/platforms/powernv/pci.c | 74 ---------------------- > arch/powerpc/platforms/powernv/pci.h | 5 -- > 5 files changed, 84 deletions(-) > > diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h > index a55b01c90bb1..16c67f4eeb71 100644 > --- a/arch/powerpc/include/asm/opal.h > +++ b/arch/powerpc/include/asm/opal.h > @@ -279,8 +279,6 @@ int64_t opal_xive_allocate_irq(uint32_t chip_id); > int64_t opal_xive_free_irq(uint32_t girq); > int64_t opal_xive_sync(uint32_t type, uint32_t id); > int64_t opal_xive_dump(uint32_t type, uint32_t id); > -int64_t opal_pci_set_p2p(uint64_t phb_init, uint64_t phb_target, > - uint64_t desc, uint16_t pe_number); > > int64_t opal_imc_counters_init(uint32_t type, uint64_t address, > uint64_t cpu_pir); > diff --git a/arch/powerpc/include/asm/pnv-pci.h b/arch/powerpc/include/asm/pnv-pci.h > index 630eb8b1b7ed..9fcb0bc462c6 100644 > --- a/arch/powerpc/include/asm/pnv-pci.h > +++ b/arch/powerpc/include/asm/pnv-pci.h > @@ -26,8 +26,6 @@ extern int pnv_pci_get_presence_state(uint64_t id, uint8_t *state); > extern int pnv_pci_get_power_state(uint64_t id, uint8_t *state); > extern int pnv_pci_set_power_state(uint64_t id, uint8_t state, > struct opal_msg *msg); > -extern int pnv_pci_set_p2p(struct pci_dev *initiator, struct pci_dev *target, > - u64 desc); > > extern int pnv_pci_enable_tunnel(struct pci_dev *dev, uint64_t *asnind); > extern int pnv_pci_disable_tunnel(struct pci_dev *dev); > diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c > index daad8c45c8e7..a89f0e31b468 100644 > --- a/arch/powerpc/platforms/powernv/opal-call.c > +++ b/arch/powerpc/platforms/powernv/opal-call.c > @@ -267,7 +267,6 @@ OPAL_CALL(opal_npu_map_lpar, OPAL_NPU_MAP_LPAR); > OPAL_CALL(opal_imc_counters_init, OPAL_IMC_COUNTERS_INIT); > OPAL_CALL(opal_imc_counters_start, OPAL_IMC_COUNTERS_START); > OPAL_CALL(opal_imc_counters_stop, OPAL_IMC_COUNTERS_STOP); > -OPAL_CALL(opal_pci_set_p2p, OPAL_PCI_SET_P2P); > OPAL_CALL(opal_get_powercap, OPAL_GET_POWERCAP); > OPAL_CALL(opal_set_powercap, OPAL_SET_POWERCAP); > OPAL_CALL(opal_get_power_shift_ratio, OPAL_GET_POWER_SHIFT_RATIO); > diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c > index ef9448a907c6..8d28f2932c3b 100644 > --- a/arch/powerpc/platforms/powernv/pci.c > +++ b/arch/powerpc/platforms/powernv/pci.c > @@ -38,7 +38,6 @@ > #include "powernv.h" > #include "pci.h" > > -static DEFINE_MUTEX(p2p_mutex); > static DEFINE_MUTEX(tunnel_mutex); > > int pnv_pci_get_slot_id(struct device_node *np, uint64_t *id) > @@ -861,79 +860,6 @@ void pnv_pci_dma_bus_setup(struct pci_bus *bus) > } > } > > -int pnv_pci_set_p2p(struct pci_dev *initiator, struct pci_dev *target, u64 desc) > -{ > - struct pci_controller *hose; > - struct pnv_phb *phb_init, *phb_target; > - struct pnv_ioda_pe *pe_init; > - int rc; > - > - if (!opal_check_token(OPAL_PCI_SET_P2P)) > - return -ENXIO; > - > - hose = pci_bus_to_host(initiator->bus); > - phb_init = hose->private_data; > - > - hose = pci_bus_to_host(target->bus); > - phb_target = hose->private_data; > - > - pe_init = pnv_ioda_get_pe(initiator); > - if (!pe_init) > - return -ENODEV; > - > - /* > - * Configuring the initiator's PHB requires to adjust its > - * TVE#1 setting. Since the same device can be an initiator > - * several times for different target devices, we need to keep > - * a reference count to know when we can restore the default > - * bypass setting on its TVE#1 when disabling. Opal is not > - * tracking PE states, so we add a reference count on the PE > - * in linux. > - * > - * For the target, the configuration is per PHB, so we keep a > - * target reference count on the PHB. > - */ > - mutex_lock(&p2p_mutex); > - > - if (desc & OPAL_PCI_P2P_ENABLE) { > - /* always go to opal to validate the configuration */ > - rc = opal_pci_set_p2p(phb_init->opal_id, phb_target->opal_id, > - desc, pe_init->pe_number); > - > - if (rc != OPAL_SUCCESS) { > - rc = -EIO; > - goto out; > - } > - > - pe_init->p2p_initiator_count++; > - phb_target->p2p_target_count++; > - } else { > - if (!pe_init->p2p_initiator_count || > - !phb_target->p2p_target_count) { > - rc = -EINVAL; > - goto out; > - } > - > - if (--pe_init->p2p_initiator_count == 0) > - pnv_pci_ioda2_set_bypass(pe_init, true); > - > - if (--phb_target->p2p_target_count == 0) { > - rc = opal_pci_set_p2p(phb_init->opal_id, > - phb_target->opal_id, desc, > - pe_init->pe_number); > - if (rc != OPAL_SUCCESS) { > - rc = -EIO; > - goto out; > - } > - } > - } > - rc = 0; > -out: > - mutex_unlock(&p2p_mutex); > - return rc; > -} > -EXPORT_SYMBOL_GPL(pnv_pci_set_p2p); > - > struct device_node *pnv_pci_get_phb_node(struct pci_dev *dev) > { > struct pci_controller *hose = pci_bus_to_host(dev->bus); > diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h > index 8e36da379252..fbec1c76d7a7 100644 > --- a/arch/powerpc/platforms/powernv/pci.h > +++ b/arch/powerpc/platforms/powernv/pci.h > @@ -78,9 +78,6 @@ struct pnv_ioda_pe { > struct pnv_ioda_pe *master; > struct list_head slaves; > > - /* PCI peer-to-peer*/ > - int p2p_initiator_count; > - > /* Link in list of PE#s */ > struct list_head list; > }; > @@ -171,8 +168,6 @@ struct pnv_phb { > /* PHB and hub diagnostics */ > unsigned int diag_data_size; > u8 *diag_data; > - > - int p2p_target_count; > }; > > extern struct pci_ops pnv_pci_ops; >
On Mon, May 06, 2019 at 10:46:11AM +0200, Frederic Barrat wrote: > Hi, > > The PCI p2p and tunnel code is used by the Mellanox CX5 driver, at least > their latest, out of tree version, which is used for CORAL. My > understanding is that they'll upstream it at some point, though I don't > know what their schedule is like. As said before, we only keep exports in tree for in-tree users. If the CX5 driver grows special P2P support it will have to use the proper existing kernel infrastructure for that anyway.
On Mon, May 06, 2019 at 10:46:11AM +0200, Frederic Barrat wrote: > Hi, > > The PCI p2p and tunnel code is used by the Mellanox CX5 driver, at least > their latest, out of tree version, which is used for CORAL. My > understanding is that they'll upstream it at some point, though I don't > know what their schedule is like. FYI, Max who wrote (at least larger parts of) that code is on Cc agreed that all P2P code should go through the kernel P2P infrastructure and might be able to spend some cycles on it. Which still doesn't change anything about that fact that we [1] generally don't add infrastructure for anything that is not in the tree. [1] well, powernv seems to have handles this a little oddly, and now is on my special watchlist.
Hi Greg/Christoph, Can we leave it meanwhile till we'll find a general solution (for the upcoming kernel) ? I guess we can somehow generalize the P2P initialization process for PPC and leave it empty for now for other archs. Or maybe we can find some other solution (sysfs/configfs/module param), but it will take time since we'll need to work closely with the IBM pci guys that wrote this code. -Max. -----Original Message----- From: Christoph Hellwig <hch@lst.de> Sent: Thursday, May 23, 2019 10:53 AM To: Frederic Barrat <fbarrat@linux.ibm.com> Cc: Christoph Hellwig <hch@lst.de>; Benjamin Herrenschmidt <benh@kernel.crashing.org>; Paul Mackerras <paulus@samba.org>; Michael Ellerman <mpe@ellerman.id.au>; linuxppc-dev@lists.ozlabs.org; Max Gurtovoy <maxg@mellanox.com> Subject: Re: [PATCH 1/3] powerpc/powernv: remove the unused pnv_pci_set_p2p function On Mon, May 06, 2019 at 10:46:11AM +0200, Frederic Barrat wrote: > Hi, > > The PCI p2p and tunnel code is used by the Mellanox CX5 driver, at > least their latest, out of tree version, which is used for CORAL. My > understanding is that they'll upstream it at some point, though I > don't know what their schedule is like. FYI, Max who wrote (at least larger parts of) that code is on Cc agreed that all P2P code should go through the kernel P2P infrastructure and might be able to spend some cycles on it. Which still doesn't change anything about that fact that we [1] generally don't add infrastructure for anything that is not in the tree. [1] well, powernv seems to have handles this a little oddly, and now is on my special watchlist.
On Tue, Jul 09, 2019 at 01:49:04PM +0000, Max Gurtovoy wrote: > Hi Greg/Christoph, > Can we leave it meanwhile till we'll find a general solution (for the upcoming kernel) ? > I guess we can somehow generalize the P2P initialization process for PPC and leave it empty for now for other archs. > Or maybe we can find some other solution (sysfs/configfs/module param), but it will take time since we'll need to work closely with the IBM pci guys that wrote this code. We do not keep code without in-tree users around, especially not if we have a better API with in-tree users. AFAICS the only thing you'll need is to wire up the enable/disable calls.
On 7/9/2019 4:59 PM, Christoph Hellwig wrote: > On Tue, Jul 09, 2019 at 01:49:04PM +0000, Max Gurtovoy wrote: >> Hi Greg/Christoph, >> Can we leave it meanwhile till we'll find a general solution (for the upcoming kernel) ? >> I guess we can somehow generalize the P2P initialization process for PPC and leave it empty for now for other archs. >> Or maybe we can find some other solution (sysfs/configfs/module param), but it will take time since we'll need to work closely with the IBM pci guys that wrote this code. > We do not keep code without in-tree users around, especially not if > we have a better API with in-tree users. > > AFAICS the only thing you'll need is to wire up the enable/disable > calls. I guess you're right, but we still need to know the time frame we have here since this should be tested carefully on the P9 hardware. Are we ok with working on a solution during kernel-5.3 cycle ?
On Tue, Jul 09, 2019 at 05:31:37PM +0300, Max Gurtovoy wrote:
> Are we ok with working on a solution during kernel-5.3 cycle ?
You can start working on it any time, no need to ask for permission.
On 7/9/2019 5:32 PM, Christoph Hellwig wrote: > On Tue, Jul 09, 2019 at 05:31:37PM +0300, Max Gurtovoy wrote: >> Are we ok with working on a solution during kernel-5.3 cycle ? > You can start working on it any time, no need to ask for permission. I just want to make sure we don't remove it from the kernel before we send a general API solution. This way we'll make sure that all the kernel versions has this functionality...
On Tue, Jul 09, 2019 at 05:37:18PM +0300, Max Gurtovoy wrote: > > On 7/9/2019 5:32 PM, Christoph Hellwig wrote: >> On Tue, Jul 09, 2019 at 05:31:37PM +0300, Max Gurtovoy wrote: >>> Are we ok with working on a solution during kernel-5.3 cycle ? >> You can start working on it any time, no need to ask for permission. > > I just want to make sure we don't remove it from the kernel before we send > a general API solution. The code is gone in this merge window. > This way we'll make sure that all the kernel versions has this > functionality... Again, we do not provide functionality for out of tree modules. We've had the p2p API for about a year now, its not like you didn't have plenty of time.
On 7/9/2019 5:40 PM, Christoph Hellwig wrote: > On Tue, Jul 09, 2019 at 05:37:18PM +0300, Max Gurtovoy wrote: >> On 7/9/2019 5:32 PM, Christoph Hellwig wrote: >>> On Tue, Jul 09, 2019 at 05:31:37PM +0300, Max Gurtovoy wrote: >>>> Are we ok with working on a solution during kernel-5.3 cycle ? >>> You can start working on it any time, no need to ask for permission. >> I just want to make sure we don't remove it from the kernel before we send >> a general API solution. > The code is gone in this merge window. Ok, so we must fix it to kernel-5.3 to make sure we're covered. Understood. > >> This way we'll make sure that all the kernel versions has this >> functionality... > Again, we do not provide functionality for out of tree modules. We've > had the p2p API for about a year now, its not like you didn't have > plenty of time. I didn't know about the intention to remove this code... Also this code was merged before the p2p API for p2pmem.
On Tue, Jul 09, 2019 at 06:06:54PM +0300, Max Gurtovoy wrote:
> Also this code was merged before the p2p API for p2pmem.
Yes, without a single user or intention to submit a user, and without
covering the most useful use case (PCIe switches). While at the same
time the people involved completely ignored the PCIe P2P discussions
that have been going on the PCI list for a long time.
This is a text book example of why code needs to be upstream first
with a broad discussion instead of slipping some crap in for out
of tree drivers.
On Tue, Jul 09, 2019 at 06:06:54PM +0300, Max Gurtovoy wrote: > > On 7/9/2019 5:40 PM, Christoph Hellwig wrote: > > On Tue, Jul 09, 2019 at 05:37:18PM +0300, Max Gurtovoy wrote: > > > On 7/9/2019 5:32 PM, Christoph Hellwig wrote: > > > > On Tue, Jul 09, 2019 at 05:31:37PM +0300, Max Gurtovoy wrote: > > > > > Are we ok with working on a solution during kernel-5.3 cycle ? > > > > You can start working on it any time, no need to ask for permission. > > > I just want to make sure we don't remove it from the kernel before we send > > > a general API solution. > > The code is gone in this merge window. > > Ok, so we must fix it to kernel-5.3 to make sure we're covered. > > Understood. > > > > > > This way we'll make sure that all the kernel versions has this > > > functionality... > > Again, we do not provide functionality for out of tree modules. We've > > had the p2p API for about a year now, its not like you didn't have > > plenty of time. > > I didn't know about the intention to remove this code... The original email you responded to in this thread was received by you back in May. It is now July, 5.3 will not be out for 8-9 weeks. There has been plenty of time here... greg k-h
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h index a55b01c90bb1..16c67f4eeb71 100644 --- a/arch/powerpc/include/asm/opal.h +++ b/arch/powerpc/include/asm/opal.h @@ -279,8 +279,6 @@ int64_t opal_xive_allocate_irq(uint32_t chip_id); int64_t opal_xive_free_irq(uint32_t girq); int64_t opal_xive_sync(uint32_t type, uint32_t id); int64_t opal_xive_dump(uint32_t type, uint32_t id); -int64_t opal_pci_set_p2p(uint64_t phb_init, uint64_t phb_target, - uint64_t desc, uint16_t pe_number); int64_t opal_imc_counters_init(uint32_t type, uint64_t address, uint64_t cpu_pir); diff --git a/arch/powerpc/include/asm/pnv-pci.h b/arch/powerpc/include/asm/pnv-pci.h index 630eb8b1b7ed..9fcb0bc462c6 100644 --- a/arch/powerpc/include/asm/pnv-pci.h +++ b/arch/powerpc/include/asm/pnv-pci.h @@ -26,8 +26,6 @@ extern int pnv_pci_get_presence_state(uint64_t id, uint8_t *state); extern int pnv_pci_get_power_state(uint64_t id, uint8_t *state); extern int pnv_pci_set_power_state(uint64_t id, uint8_t state, struct opal_msg *msg); -extern int pnv_pci_set_p2p(struct pci_dev *initiator, struct pci_dev *target, - u64 desc); extern int pnv_pci_enable_tunnel(struct pci_dev *dev, uint64_t *asnind); extern int pnv_pci_disable_tunnel(struct pci_dev *dev); diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c index daad8c45c8e7..a89f0e31b468 100644 --- a/arch/powerpc/platforms/powernv/opal-call.c +++ b/arch/powerpc/platforms/powernv/opal-call.c @@ -267,7 +267,6 @@ OPAL_CALL(opal_npu_map_lpar, OPAL_NPU_MAP_LPAR); OPAL_CALL(opal_imc_counters_init, OPAL_IMC_COUNTERS_INIT); OPAL_CALL(opal_imc_counters_start, OPAL_IMC_COUNTERS_START); OPAL_CALL(opal_imc_counters_stop, OPAL_IMC_COUNTERS_STOP); -OPAL_CALL(opal_pci_set_p2p, OPAL_PCI_SET_P2P); OPAL_CALL(opal_get_powercap, OPAL_GET_POWERCAP); OPAL_CALL(opal_set_powercap, OPAL_SET_POWERCAP); OPAL_CALL(opal_get_power_shift_ratio, OPAL_GET_POWER_SHIFT_RATIO); diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c index ef9448a907c6..8d28f2932c3b 100644 --- a/arch/powerpc/platforms/powernv/pci.c +++ b/arch/powerpc/platforms/powernv/pci.c @@ -38,7 +38,6 @@ #include "powernv.h" #include "pci.h" -static DEFINE_MUTEX(p2p_mutex); static DEFINE_MUTEX(tunnel_mutex); int pnv_pci_get_slot_id(struct device_node *np, uint64_t *id) @@ -861,79 +860,6 @@ void pnv_pci_dma_bus_setup(struct pci_bus *bus) } } -int pnv_pci_set_p2p(struct pci_dev *initiator, struct pci_dev *target, u64 desc) -{ - struct pci_controller *hose; - struct pnv_phb *phb_init, *phb_target; - struct pnv_ioda_pe *pe_init; - int rc; - - if (!opal_check_token(OPAL_PCI_SET_P2P)) - return -ENXIO; - - hose = pci_bus_to_host(initiator->bus); - phb_init = hose->private_data; - - hose = pci_bus_to_host(target->bus); - phb_target = hose->private_data; - - pe_init = pnv_ioda_get_pe(initiator); - if (!pe_init) - return -ENODEV; - - /* - * Configuring the initiator's PHB requires to adjust its - * TVE#1 setting. Since the same device can be an initiator - * several times for different target devices, we need to keep - * a reference count to know when we can restore the default - * bypass setting on its TVE#1 when disabling. Opal is not - * tracking PE states, so we add a reference count on the PE - * in linux. - * - * For the target, the configuration is per PHB, so we keep a - * target reference count on the PHB. - */ - mutex_lock(&p2p_mutex); - - if (desc & OPAL_PCI_P2P_ENABLE) { - /* always go to opal to validate the configuration */ - rc = opal_pci_set_p2p(phb_init->opal_id, phb_target->opal_id, - desc, pe_init->pe_number); - - if (rc != OPAL_SUCCESS) { - rc = -EIO; - goto out; - } - - pe_init->p2p_initiator_count++; - phb_target->p2p_target_count++; - } else { - if (!pe_init->p2p_initiator_count || - !phb_target->p2p_target_count) { - rc = -EINVAL; - goto out; - } - - if (--pe_init->p2p_initiator_count == 0) - pnv_pci_ioda2_set_bypass(pe_init, true); - - if (--phb_target->p2p_target_count == 0) { - rc = opal_pci_set_p2p(phb_init->opal_id, - phb_target->opal_id, desc, - pe_init->pe_number); - if (rc != OPAL_SUCCESS) { - rc = -EIO; - goto out; - } - } - } - rc = 0; -out: - mutex_unlock(&p2p_mutex); - return rc; -} -EXPORT_SYMBOL_GPL(pnv_pci_set_p2p); - struct device_node *pnv_pci_get_phb_node(struct pci_dev *dev) { struct pci_controller *hose = pci_bus_to_host(dev->bus); diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h index 8e36da379252..fbec1c76d7a7 100644 --- a/arch/powerpc/platforms/powernv/pci.h +++ b/arch/powerpc/platforms/powernv/pci.h @@ -78,9 +78,6 @@ struct pnv_ioda_pe { struct pnv_ioda_pe *master; struct list_head slaves; - /* PCI peer-to-peer*/ - int p2p_initiator_count; - /* Link in list of PE#s */ struct list_head list; }; @@ -171,8 +168,6 @@ struct pnv_phb { /* PHB and hub diagnostics */ unsigned int diag_data_size; u8 *diag_data; - - int p2p_target_count; }; extern struct pci_ops pnv_pci_ops;
This function has never been used since it has been added to the tree. We also now have proper PCIe P2P APIs in the core kernel, and any new P2P support should be using those. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/powerpc/include/asm/opal.h | 2 - arch/powerpc/include/asm/pnv-pci.h | 2 - arch/powerpc/platforms/powernv/opal-call.c | 1 - arch/powerpc/platforms/powernv/pci.c | 74 ---------------------- arch/powerpc/platforms/powernv/pci.h | 5 -- 5 files changed, 84 deletions(-)