Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 24 Mar 2022 16:44:33 -0700
From:   Ira Weiny <ira.weiny@intel.com>
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc:     Bjorn Helgaas <helgaas@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ben Widawsky <ben.widawsky@intel.com>,
        linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org,
        linux-pci@vger.kernel.org
Subject: Re: [PATCH V6 04/10] PCI/DOE: Introduce pci_doe_create_doe_devices
Message-ID: <Yj0CYdSrVMxlmXyJ@iweiny-desk3>
References: <20220201071952.900068-5-ira.weiny@intel.com>
 <20220203224437.GA120552@bhelgaas>
 <Yju6quQsBDSMaNC2@iweiny-desk3>
 <20220324140539.00004be8@Huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220324140539.00004be8@Huawei.com>
Precedence: bulk

On Thu, Mar 24, 2022 at 02:05:39PM +0000, Jonathan Cameron wrote:
> Hi Ira,
> 
> > Here is the code to be more clear...
> > 
> > 
> > drivers/cxl/pci.c:
> > 
> > int cxl_pci_create_doe_devices(struct pci_dev *pdev)
> > {               
> >         struct device *dev = &pdev->dev;
> >         bool use_irq = true;
> >         int irqs = 0;
> >         u16 off = 0;         
> >         int rc;
> >         
> >         pci_doe_for_each_off(pdev, off)
> >                 irqs++;
> >         pci_info(pdev, "Found %d DOE mailbox's\n", irqs);
> >         
> >         /*                         
> >          * Allocate enough vectors for the DOE's
> >          */     
> >         rc = pci_alloc_irq_vectors(pdev, irqs, irqs, PCI_IRQ_MSI |
> >                                                      PCI_IRQ_MSIX);
> >         if (rc != irqs) {
> >                 pci_err(pdev, "Not enough interrupts for all the DOEs; use polling\n");
> >                 use_irq = false;
> >                 /* Some got allocated; clean them up */
> >                 if (rc > 0)
> >                         cxl_pci_free_irq_vectors(pdev); 
> >         } else {
> >                 /*
> >                  * Enabling bus mastering is require for MSI/MSIx.  It could be
> >                  * done later within the DOE initialization, but as it
> >                  * potentially has other impacts keep it here when setting up
> >                  * the IRQ's.
> >                  */
> >                 pci_set_master(pdev);
> >                 rc = devm_add_action_or_reset(dev,
> >                                               cxl_pci_free_irq_vectors,
> >                                               pdev);
> >                 if (rc)
> >                         return rc;
> >         }
> > 
> >         pci_doe_for_each_off(pdev, off) {
> > ...
> > 		/* Create each auxiliary device which internally calls */
> > 		pci_doe_create_mb(pdev, off, use_irq);
> > ...
> > 	}
> > ...
> > }
> > 
> > 
> > drivers/pci/pci-doe.c:
> > 
> > static irqreturn_t pci_doe_irq_handler(int irq, void *data)
> > {
> > ...
> > }
> > 
> > static int pci_doe_request_irq(struct pci_doe_mb *doe_mb)
> > {
> >         struct pci_dev *pdev = doe_mb->pdev;
> >         int offset = doe_mb->cap_offset;
> >         int doe_irq, rc;
> >         u32 val;
> > 
> >         pci_read_config_dword(pdev, offset + PCI_DOE_CAP, &val);
> > 
> >         if (!FIELD_GET(PCI_DOE_CAP_INT, val))
> >                 return -ENOTSUPP;
> > 
> >         doe_irq = FIELD_GET(PCI_DOE_CAP_IRQ, val);
> >         rc = pci_request_irq(pdev, doe_irq, pci_doe_irq_handler,
> >                              NULL, doe_mb,
> >                              "DOE[%d:%s]", doe_irq, pci_name(pdev));
> >         if (rc) 
> >                 return rc;
> > 
> >         doe_mb->irq = doe_irq;
> >         pci_write_config_dword(pdev, offset + PCI_DOE_CTRL,
> >                                PCI_DOE_CTRL_INT_EN);
> >         return 0;
> > }
> > 
> > struct pci_doe_mb *pci_doe_create_mb(struct pci_dev *pdev, u16 cap_offset,
> >                                      bool use_irq)
> > {
> > ...
> >         if (use_irq) {
> >                 rc = pci_doe_request_irq(doe_mb);
> >                 if (rc) 
> >                         pci_err(pdev, "DOE request irq failed for mailbox @ %u : %d\n",
> >                                 cap_offset, rc);
> >         }
> > ...
> > }
> > 
> > 
> > Does this look reasonable?
> 
> I'm a little nervous about how we are going to make DOEs on switches work.
> Guess I'll do an experiment once your next version is out and check we
> can do that reasonably cleanly.  For switches we'll probably have to
> check for DOEs on all such ports and end up with infrastructure to
> map to all protocols we might see on a switch.

Are the switches not represented as PCI devices in linux?

If my vision of switches is correct I think that problem is independent of what
I'm solving here.  In other words the relationship between a port on a switch
and a DOE capability on that switch will have to be established somehow and
nothing I'm doing precludes doing that, but at the same time nothing I'm doing
helps that either.

Ira

> 
> Jonathan
> 
> >