On Wed, Oct 16, 2024 at 06:30:40PM +0200, Thomas Gleixner wrote:
> On Tue, Oct 15 2024 at 18:07, Frank Li wrote:
> > +static int pci_epc_alloc_doorbell(struct pci_epc *epc, u8 func_no, u8 vfunc_no, int num_db)
> > +{
> > + struct msi_desc *desc, *failed_desc;
> > + struct pci_epf *epf;
> > + struct device *dev;
> > + int i = 0;
> > + int ret;
> > +
> > + if (IS_ERR_OR_NULL(epc))
> > + return -EINVAL;
> > +
> > + /* Currently only support one func and one vfunc for doorbell */
> > + if (func_no || vfunc_no)
> > + return -EINVAL;
> > +
> > + epf = list_first_entry_or_null(&epc->pci_epf, struct pci_epf, list);
> > + if (!epf)
> > + return -EINVAL;
> > +
> > + dev = epc->dev.parent;
> > + ret = platform_device_msi_init_and_alloc_irqs(dev, num_db, pci_epc_write_msi_msg);
> > + if (ret) {
> > + dev_err(dev, "Failed to allocate MSI\n");
> > + return -ENOMEM;
> > + }
> > +
> > + scoped_guard(msi_descs, dev)
> > + msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
>
> That's just wrong. Nothing in this code has to fiddle with MSI
> descriptors or the descriptor lock.
>
> for (i = 0; i < num_db; i++) {
> virq = msi_get_virq(dev, i);
Thanks, Change to msi_for_each_desc() is based on comments on
https://lore.kernel.org/imx/20231017183722.GB137137@thinkpad/
So my original implement is correct.
>
> > + ret = request_irq(desc->irq, pci_epf_doorbell_handler, 0,
> > + kasprintf(GFP_KERNEL, "pci-epc-doorbell%d", i++), epf);
> > + if (ret) {
> > + dev_err(dev, "Failed to request doorbell\n");
> > + failed_desc = desc;
> > + goto err_request_irq;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_request_irq:
> > + scoped_guard(msi_descs, dev)
> > + msi_for_each_desc(desc, dev, MSI_DESC_ALL) {
> > + if (desc == failed_desc)
> > + break;
> > + kfree(free_irq(desc->irq, epf));
>
> All instances of interrupts get 'epf' as device id. So if the third
> instance failed to be requested, you free 'epf' when freeing the first
> interrupt and then again when freeing the second one.
Thanks, I actually want to free kasprintf() at request_irq(). I miss
understand the return value of free_irq().
Frank
>
> Thanks,
>
> tglx
