| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 3 May 2022 16:32:15 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ira Weiny <ira.weiny@...el.com>
CC: Dan Williams <dan.j.williams@...el.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Alison Schofield <alison.schofield@...el.com>,
"Vishal Verma" <vishal.l.verma@...el.com>,
Ben Widawsky <ben.widawsky@...el.com>,
<linux-kernel@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
<linux-pci@...r.kernel.org>
Subject: Re: [PATCH V8 04/10] cxl/pci: Create auxiliary devices for each DOE
mailbox
On Fri, 29 Apr 2022 10:20:54 -0700
Ira Weiny <ira.weiny@...el.com> wrote:
> On Fri, Apr 29, 2022 at 04:55:02PM +0100, Jonathan Cameron wrote:
> > On Thu, 14 Apr 2022 13:32:31 -0700
> > ira.weiny@...el.com wrote:
> >
> > > From: Ira Weiny <ira.weiny@...el.com>
> > >
>
> [snip]
>
> > > +
> > > +/**
> > > + * cxl_pci_create_doe_devices - Create auxiliary bus DOE devices for all DOE
> > > + * mailboxes found
> > > + *
> > > + * @pci_dev: The PCI device to scan for DOE mailboxes
> > > + *
> > > + * There is no coresponding destroy of these devices. This function associates
> > > + * the DOE auxiliary devices created with the pci_dev passed in. That
> > > + * association is device managed (devm_*) such that the DOE auxiliary device
> > > + * lifetime is always less than or equal to the lifetime of the pci_dev.
> > > + *
> > > + * RETURNS: 0 on success -ERRNO on failure.
> > > + */
> > > +static 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++;
> > I believe this is insufficient because there may be other irqs in use
> > on the device.
>
> I did not think that was true for any current CXL device. From what I could
> tell all CXL devices would be covered by this simple calculation. I left it to
> the reader to determine if a new CXL device came along which needed other irq's
> to lift this somewhere to cover those allocations. I probably should have made
> some comment. Sorry.
>
> > In a similar fashion to that done in pcie/portdrv_core.c
> > I think we need to instead find the maximum msi/msix vector number
> > by reading the config space.
>
> I was not aware I could do that...
>
> > Then we request one more vector
> > than that max value to ensure the vector we need has been requested.
>
> Yea at some point I figured this would be lifted out of here as part of a
> larger 'allocate all the vectors for the device' function.
>
> But for now this is the only place that needs irqs so I punted. I can lift
> this into something like
>
> cxl_pci_alloc_irq_vectors(...) and then pass use_irq here.
>
> But to move this series forward I would propose that
> cxl_pci_alloc_irq_vectors() do what I'm doing here for now.
Handling this right is pretty simple code e.g. equivalent of
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pcie/portdrv_core.c#L62
So my inclination would be to fix it now rather than leaving chance
of some odd failures later.
Jonathan
>
> Ira
>
> >
> > Jonathan
> >
> > > + 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) {
> > > + struct auxiliary_device *adev;
> > > + struct cxl_doe_dev *new_dev;
> > > + int id;
> > > +
> > > + new_dev = kzalloc(sizeof(*new_dev), GFP_KERNEL);
> > > + if (!new_dev)
> > > + return -ENOMEM;
> > > +
> > > + new_dev->pdev = pdev;
> > > + new_dev->cap_offset = off;
> > > + new_dev->use_irq = use_irq;
> > > +
> > > + /* Set up struct auxiliary_device */
> > > + adev = &new_dev->adev;
> > > + id = ida_alloc(&pci_doe_adev_ida, GFP_KERNEL);
> > > + if (id < 0) {
> > > + kfree(new_dev);
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + adev->id = id;
> > > + adev->name = DOE_DEV_NAME;
> > > + adev->dev.release = cxl_pci_doe_dev_release;
> > > + adev->dev.parent = dev;
> > > +
> > > + if (auxiliary_device_init(adev)) {
> > > + cxl_pci_doe_dev_release(&adev->dev);
> > > + return -EIO;
> > > + }
> > > +
> > > + if (auxiliary_device_add(adev)) {
> > > + auxiliary_device_uninit(adev);
> > > + return -EIO;
> > > + }
> > > +
> > > + rc = devm_add_action_or_reset(dev, cxl_pci_doe_destroy_device,
> > > + adev);
> > > + if (rc)
> > > + return rc;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > {
> > > struct cxl_register_map map;
> > > @@ -630,6 +753,10 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > > if (rc)
> > > return rc;
> > >
> > > + rc = cxl_pci_create_doe_devices(pdev);
> > > + if (rc)
> > > + return rc;
> > > +
> > > cxl_dvsec_ranges(cxlds);
> > >
> > > cxlmd = devm_cxl_add_memdev(cxlds);
> >
Powered by blists - more mailing lists