On Thu, 24 Mar 2022 16:44:33 -0700
Ira Weiny <ira.weiny@xxxxxxxxx> 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
> >
> > >
