On 4/17/2020 10:02 AM, Christoph Hellwig wrote:
+#ifdef CONFIG_PCI_P2PDMA
+int64_t opal_pci_set_p2p(uint64_t phb_init, uint64_t phb_target,
+ uint64_t desc, uint16_t pe_number);
+#endif
There should be no need for the ifdef here.
+ /*
+ * 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.
+ */
This could be shortened a bit by using up the whole 80 lines available
in source files.
+ 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;
+}
The enable and disable path shares almost no code, why not split it into
two functions?
how about also changing the defines OPAL_PCI_P2P_* to an enum ?
/* PCI p2p operation descriptors */
enum opal_pci_p2p {
OPAL_PCI_P2P_DISABLE = 0,
OPAL_PCI_P2P_ENABLE = (1 << 0),
OPAL_PCI_P2P_LOAD = (1 << 1),
OPAL_PCI_P2P_STORE = (1 << 2),
};
Fred ?
+static bool pnv_pci_controller_owns_addr(struct pci_controller *hose,
+ phys_addr_t addr, size_t size)
+{
+ struct resource *r;
+ int i;
+
+ /*
+ * it seems safe to assume the full range is under the same
+ * PHB, so we can ignore the size
Capitalize the first character in a multi-line comment, and use up the
whole 80 chars.
+ */
+ for (i = 0; i < 3; i++) {
Replace the magic 3 with ARRAY_SIZE(hose->mem_resources) ?
+ r = &hose->mem_resources[i];
Move the r declaration here and initialize it on the same line.
+ if (r->flags && (addr >= r->start) && (addr < r->end))
No need for the inner braces.
+ return true;
+ }
+ return false;
+}
+
+/*
+ * find the phb owning a mmio address if not owned locally
+ */
+static struct pnv_phb *pnv_pci_find_owning_phb(struct pci_dev *pdev,
+ phys_addr_t addr, size_t size)
+{
+ struct pci_controller *hose = pci_bus_to_host(pdev->bus);
+ struct pnv_phb *phb;
+
+ /* fast path */
+ if (pnv_pci_controller_owns_addr(hose, addr, size))
+ return NULL;
Maybe open code the pci_bus_to_host(pdev->bus) call here, as we just
overwrite host in the list iteration?
if this is more readable so no problem:
if (pnv_pci_controller_owns_addr(pdev->bus->sysdata, addr, size))
+
+ list_for_each_entry(hose, &hose_list, list_node) {
+ phb = hose->private_data;
Move the ohb declaration here and initialize it on the same line.
+ if (phb->type != PNV_PHB_NPU_NVLINK &&
+ phb->type != PNV_PHB_NPU_OCAPI) {
+ if (pnv_pci_controller_owns_addr(hose, addr, size))
+ return phb;
+ }
+ }
+ return NULL;
+}
+
+static int pnv_pci_dma_map_resource(struct pci_dev *pdev,
+ phys_addr_t phys_addr, size_t size,
+ enum dma_data_direction dir)
+{
+ struct pnv_phb *target_phb;
+ int rc;
+ u64 desc;
+
+ target_phb = pnv_pci_find_owning_phb(pdev, phys_addr, size);
+ if (target_phb) {
Return early here for the !target_phb case?
+ desc = OPAL_PCI_P2P_ENABLE;
+ if (dir == DMA_TO_DEVICE)
+ desc |= OPAL_PCI_P2P_STORE;
+ else if (dir == DMA_FROM_DEVICE)
+ desc |= OPAL_PCI_P2P_LOAD;
+ else if (dir == DMA_BIDIRECTIONAL)
+ desc |= OPAL_PCI_P2P_LOAD | OPAL_PCI_P2P_STORE;
Seems like this could be split into a little helper.
sure.
+ rc = pnv_pci_ioda_set_p2p(pdev, target_phb, desc);
+ if (rc) {
+ dev_err(&pdev->dev, "Failed to setup PCI peer-to-peer for address %llx: %d\n",
+ phys_addr, rc);
+ return rc;
+ }
No need for this printk, the callers should already deal with mapping
failures.
