Re: [PATCH V6 04/10] PCI/DOE: Introduce pci_doe_create_doe_devices

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Thu, 24 Mar 2022 16:44:33 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

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

Sure, I'm just expressing nervousness and would want a PoC of that at least
to check it's not too nasty.  The port drivers are rather 'unusual' in PCI
so touching them always ends up more complex than I expect.

Anyhow, start of cycle so should be plenty of time to do such an RFC
once your code is out there.

Jonathan

> 
> Ira
> 
> > 
> > Jonathan
> >   
> > >