From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from m16.mail.126.com (m16.mail.126.com [220.197.31.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 58091282F17 for ; Thu, 28 May 2026 01:21:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=220.197.31.6 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779931316; cv=none; b=erbw0LrwvmLFOzt/bs6CZedlLtYZTHWLG2KgIyg8bjq6X9Luz7ObJsrNUzb1goXm9viyHt9AScPRAIwcVFTo1biHEARfb8RlKgcRe/lGABv30Mm7cEn1KBzhR+hV8MRGu+2cpK1qOSFNc4cxs6kVFN2SJolYomqnD0V2tpp0oeg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779931316; c=relaxed/simple; bh=rrsEynE3HJS7SfrTZ5BsHv1U503JPjDlKwS3R/+yTOE=; h=Message-ID:Date:From:MIME-Version:To:CC:Subject:References: In-Reply-To:Content-Type; b=OIrAPkmvT9ulInaT7TIcywJ4G3z/7q7ARXbLiX67qEAfZIoFn4IByghCRnoSIw/KgR/0bfneziU5EnCNVLRITDaJ30nh7lippYCkPX8zOanPxhOAHPhMaTGgCmXpIDasLIyekyjlE1fcRyBc8Dk4V6O6LIgIRYzbyjgbMHTa1qo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=126.com; spf=pass smtp.mailfrom=126.com; dkim=pass (1024-bit key) header.d=126.com header.i=@126.com header.b=h28cbuQa; arc=none smtp.client-ip=220.197.31.6 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=126.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=126.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=126.com header.i=@126.com header.b="h28cbuQa" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=126.com; s=s110527; h=Message-ID:Date:From:MIME-Version:To:Subject: Content-Type; bh=DvglU8w+sIfpxposs9zkLD3a8M5HVZm7O4Pd7puZUZ4=; b=h28cbuQa7P5i9gOgbxelO1QH0yTPd+1UiffKDooGnvnTbW6V+kTtC2peCidmAu zaGeX9gTBxFQyeM8Wx6/W5khrreUkvEr1uTk7WaPFHgMAWk/RgrJ/QAUPvQtkNNw nfHAIHd3Fs7kkJzyWzAbWebBqvHuKfgwdqhuf7DFSEQSI= Received: from localhost.localdomain (unknown []) by gzga-smtp-mtada-g1-3 (Coremail) with SMTP id _____wDn77tDmBdqin+vAQ--.44062S2; Thu, 28 May 2026 09:20:04 +0800 (CST) Message-ID: <6A179847.6000107@126.com> Date: Thu, 28 May 2026 09:20:07 +0800 From: Hongling Zeng User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 To: =?UTF-8?B?RG9taW5payBLYXJvbCBQacSFdGtvd3NraQ==?= , Hongling Zeng CC: dpenkler@gmail.com, gregkh@linuxfoundation.org, kees@kernel.org, dan.carpenter@linaro.org, lukeyang.dev@gmail.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH] gpib: fmh_gpib: Fix resource leaks in fmh_gpib_attach_impl References: <20260527095450.647869-1-zenghongling@kylinos.cn> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-CM-TRANSID:_____wDn77tDmBdqin+vAQ--.44062S2 X-Coremail-Antispam: 1Uf129KBjvJXoW3AF1kWFWfAF4fJr47uryxGrg_yoW7CF13pa 1xXa15Kr1UtrZaqFsxX3WDWFsYvw42vFy5Zw42ka43Aa4qyF92yF4UGay2kryvyrWkJw45 trW0qr409F4DZaDanT9S1TB71UUUUU7qnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x07j-WrJUUUUU= X-CM-SenderInfo: x2kr0wpolqwiqxrzqiyswou0bp/xtbBrwSXaGoXmETqcAAA3v Hi Dominik, Thank you for the review. You're right on all three points: 在 2026年05月27日 21:14, Dominik Karol Piątkowski 写道: > Hi Hongling Zeng, > > On Wednesday, May 27th, 2026 at 11:55, Hongling Zeng wrote: > >> The fmh_gpib_attach_impl() function has multiple resource leaks in its >> error handling paths. When any initialization step fails, the function >> returns early without properly releasing previously acquired resources. >> >> Fix by adding proper error handling labels and cleanup code that releases >> resources in the reverse order they were acquired. >> >> Fixes: 8e4841a0888c7 ("staging: gpib: Add Frank Mori Hess FPGA PCI GPIB driver") >> Signed-off-by: Hongling Zeng >> --- >> drivers/gpib/fmh_gpib/fmh_gpib.c | 46 ++++++++++++++++++++++++-------- >> 1 file changed, 35 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpib/fmh_gpib/fmh_gpib.c b/drivers/gpib/fmh_gpib/fmh_gpib.c >> index fcafdc02ea2e..af582453fef7 100644 >> --- a/drivers/gpib/fmh_gpib/fmh_gpib.c >> +++ b/drivers/gpib/fmh_gpib/fmh_gpib.c >> @@ -1403,14 +1403,17 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpib_control_status"); >> if (!res) { >> dev_err(board->dev, "Unable to locate mmio resource\n"); >> - return -ENODEV; >> + retval = -ENODEV; >> + return retval; > Replacing `return -errno` with `retval = -errno` just to have `return retval` > directly under it does not convince me. Early error returns can just use return -ENODEV directly >> } >> >> if (request_mem_region(res->start, >> resource_size(res), >> pdev->name) == NULL) { >> dev_err(board->dev, "cannot claim registers\n"); >> - return -ENXIO; >> + retval = -ENXIO; >> + return retval; >> + > Same here, also introducing unnecessary newline. Will remove the extra newline > >> } >> e_priv->gpib_iomem_res = res; >> >> @@ -1418,7 +1421,8 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar >> resource_size(e_priv->gpib_iomem_res)); >> if (!nec_priv->mmiobase) { >> dev_err(board->dev, "Could not map I/O memory\n"); >> - return -ENOMEM; >> + retval = -ENOMEM; >> + goto err_release_gpib_region; >> } >> dev_dbg(board->dev, "iobase %pr remapped to %p\n", >> e_priv->gpib_iomem_res, nec_priv->mmiobase); >> @@ -1426,42 +1430,48 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma_fifos"); >> if (!res) { >> dev_err(board->dev, "Unable to locate mmio resource for gpib dma port\n"); >> - return -ENODEV; >> + retval = -ENODEV; >> + goto err_iounmap_gpib; >> } >> if (request_mem_region(res->start, >> resource_size(res), >> pdev->name) == NULL) { >> dev_err(board->dev, "cannot claim registers\n"); >> - return -ENXIO; >> + retval = -ENXIO; >> + goto err_iounmap_gpib; >> } >> e_priv->dma_port_res = res; >> e_priv->fifo_base = ioremap(e_priv->dma_port_res->start, >> resource_size(e_priv->dma_port_res)); >> if (!e_priv->fifo_base) { >> dev_err(board->dev, "Could not map I/O memory for fifos\n"); >> - return -ENOMEM; >> + retval = -ENOMEM; >> + goto err_release_dma_region; >> } >> dev_dbg(board->dev, "dma fifos 0x%lx remapped to %p, length=%ld\n", >> (unsigned long)e_priv->dma_port_res->start, e_priv->fifo_base, >> (unsigned long)resource_size(e_priv->dma_port_res)); >> >> irq = platform_get_irq(pdev, 0); >> - if (irq < 0) >> - return -EBUSY; >> + if (irq < 0) { >> + retval = -EBUSY; >> + goto err_iounmap_fifo; >> + } >> + e_priv->irq = irq; > I don't think this assignment should be moved before request_irq. > > Thanks, > Dominik Karol e_priv->irq = irq; should stay after request_irq() succeeds - my concern about uninitialized values in error paths was incorrect since err_iounmap_fifo doesn't use e_priv->irq I'll fix these issues and resubmit. Best regards, Hongling Zeng >> retval = request_irq(irq, fmh_gpib_interrupt, IRQF_SHARED, pdev->name, board); >> if (retval) { >> dev_err(board->dev, >> "cannot register interrupt handler err=%d\n", >> retval); >> - return retval; >> + goto err_iounmap_fifo; >> } >> - e_priv->irq = irq; >> >> if (acquire_dma) { >> e_priv->dma_channel = dma_request_slave_channel(board->dev, "rxtx"); >> if (!e_priv->dma_channel) { >> dev_err(board->dev, "failed to acquire dma channel \"rxtx\".\n"); >> - return -EIO; >> + retval = -EIO; >> + goto err_free_irq; >> } >> } >> /* >> @@ -1473,6 +1483,20 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar >> fifo_max_burst_length_mask; >> >> return fmh_gpib_init(e_priv, board, handshake_mode); >> + >> +err_free_irq: >> + free_irq(e_priv->irq, board); >> +err_iounmap_fifo: >> + iounmap(e_priv->fifo_base); >> +err_release_dma_region: >> + release_mem_region(e_priv->dma_port_res->start, >> + resource_size(e_priv->dma_port_res)); >> +err_iounmap_gpib: >> + iounmap(nec_priv->mmiobase); >> +err_release_gpib_region: >> + release_mem_region(e_priv->gpib_iomem_res->start, >> + resource_size(e_priv->gpib_iomem_res)); >> + return retval; >> } >> >> int fmh_gpib_attach_holdoff_all(struct gpib_board *board, const struct gpib_board_config *config) >> -- >> 2.25.1 >> >>