linux-kernel - Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support using platform MSI controller

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zw/vq4EweR+yTphB@lizhi-Precision-Tower-5810>
Date: Wed, 16 Oct 2024 12:54:03 -0400
From: Frank Li <Frank.li@....com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, Niklas Cassel <cassel@...nel.org>,
	dlemoal@...nel.org, maz@...nel.org, jdmason@...zu.us
Subject: Re: [PATCH v3 3/6] PCI: endpoint: Add RC-to-EP doorbell support
 using platform MSI controller

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