| Message ID | 20221116093204.46700-1-wangxiongfeng2@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 804313b64e412a81b0b3389a10e7622452004aa6 |
| Headers | show |
| Series | spi: dw-dma: decrease reference count in dw_spi_dma_init_mfld() | expand |
Hi Xiongfeng, On Wed, Nov 16, 2022 at 05:32:04PM +0800, Xiongfeng Wang wrote: > pci_get_device() will increase the reference count for the returned > pci_dev. Since 'dma_dev' is only used to filter the channel in > dw_spi_dma_chan_filer(). After using it, we need to use pci_dev_put() to ^ ^ ^ | | | drop the dot and comma s/use/call * Although this could be fixed by Mark on merging the patch in. > decrease the reference count. Also add pci_dev_put() for the error case. > > Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support") > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> Nice catch. Thanks for the patch. Acked-by: Serge Semin <fancer.lancer@gmail.com> -Sergey > --- > drivers/spi/spi-dw-dma.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c > index 1322b8cce5b7..ababb910b391 100644 > --- a/drivers/spi/spi-dw-dma.c > +++ b/drivers/spi/spi-dw-dma.c > @@ -128,12 +128,15 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > > dw_spi_dma_sg_burst_init(dws); > > + pci_dev_put(dma_dev); > + > return 0; > > free_rxchan: > dma_release_channel(dws->rxchan); > dws->rxchan = NULL; > err_exit: > + pci_dev_put(dma_dev); > return -EBUSY; > } > > -- > 2.20.1 >
Hi Serge, On 2022/11/16 19:19, Serge Semin wrote: > Hi Xiongfeng, > > On Wed, Nov 16, 2022 at 05:32:04PM +0800, Xiongfeng Wang wrote: >> pci_get_device() will increase the reference count for the returned >> pci_dev. Since 'dma_dev' is only used to filter the channel in >> dw_spi_dma_chan_filer(). After using it, we need to use pci_dev_put() to > ^ ^ ^ > | | | > drop the dot and comma s/use/call > > * Although this could be fixed by Mark on merging the patch in. Thanks for your reply ! Sorry, I am not so sure about the modification. Let me make sure it and send another version. Do you mean change it like below: dw_spi_dma_chan_filer(), we need to use pci_dev_put() to Thanks, Xiongfeng > >> decrease the reference count. Also add pci_dev_put() for the error case. >> >> Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support") >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > Nice catch. Thanks for the patch. > Acked-by: Serge Semin <fancer.lancer@gmail.com> > > -Sergey > >> --- >> drivers/spi/spi-dw-dma.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c >> index 1322b8cce5b7..ababb910b391 100644 >> --- a/drivers/spi/spi-dw-dma.c >> +++ b/drivers/spi/spi-dw-dma.c >> @@ -128,12 +128,15 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) >> >> dw_spi_dma_sg_burst_init(dws); >> >> + pci_dev_put(dma_dev); >> + >> return 0; >> >> free_rxchan: >> dma_release_channel(dws->rxchan); >> dws->rxchan = NULL; >> err_exit: >> + pci_dev_put(dma_dev); >> return -EBUSY; >> } >> >> -- >> 2.20.1 >> > . >
On Wed, Nov 16, 2022 at 08:46:07PM +0800, Xiongfeng Wang wrote: > Hi Serge, > > On 2022/11/16 19:19, Serge Semin wrote: > > Hi Xiongfeng, > > > > On Wed, Nov 16, 2022 at 05:32:04PM +0800, Xiongfeng Wang wrote: > >> pci_get_device() will increase the reference count for the returned > >> pci_dev. Since 'dma_dev' is only used to filter the channel in > >> dw_spi_dma_chan_filer(). After using it, we need to use pci_dev_put() to > > ^ ^ ^ > > | | | > > drop the dot and comma s/use/call > > > > * Although this could be fixed by Mark on merging the patch in. > > Thanks for your reply ! Sorry, I am not so sure about the modification. > Let me make sure it and send another version. > Do you mean change it like below: > > dw_spi_dma_chan_filer(), we need to use pci_dev_put() to I meant that the sentence should have looked like this: "Since 'dma_dev' is only used to filter the channel in dw_spi_dma_chan_filer() after using it we need to call pci_dev_put() to decrease the reference count." Though on the second thought the whole message sounds a bit clumsy since with no driver internals knowledge a reader won't be able figure out a link between the pci_get_device(), dw_spi_dma_init_mfld() and dw_spi_dma_chan_filer() methods. I would convert it to something like this: + [PATCH v2] spi: dw-dma: Fix PCI device reference count leak + + The pci_get_device() method returns a pointer to a PCI device with + the incremented reference count. Since the pointer is defined and used + within the dw_spi_dma_init_mfld() function only we must decrement the + device reference count before exiting from the function otherwise the + count state will be unbalanced afterwards. Note it must be done for + both normal and error paths of the function. Anyway the solution looks good to me. So don't forget to add my ab-tag to v2 of the patch (if you intend to send one). -Sergey > > Thanks, > Xiongfeng > > > > >> decrease the reference count. Also add pci_dev_put() for the error case. > >> > >> Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support") > >> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > > > Nice catch. Thanks for the patch. > > Acked-by: Serge Semin <fancer.lancer@gmail.com> > > > > -Sergey > > > >> --- > >> drivers/spi/spi-dw-dma.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c > >> index 1322b8cce5b7..ababb910b391 100644 > >> --- a/drivers/spi/spi-dw-dma.c > >> +++ b/drivers/spi/spi-dw-dma.c > >> @@ -128,12 +128,15 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) > >> > >> dw_spi_dma_sg_burst_init(dws); > >> > >> + pci_dev_put(dma_dev); > >> + > >> return 0; > >> > >> free_rxchan: > >> dma_release_channel(dws->rxchan); > >> dws->rxchan = NULL; > >> err_exit: > >> + pci_dev_put(dma_dev); > >> return -EBUSY; > >> } > >> > >> -- > >> 2.20.1 > >> > > . > >
On Wed, 16 Nov 2022 17:32:04 +0800, Xiongfeng Wang wrote: > pci_get_device() will increase the reference count for the returned > pci_dev. Since 'dma_dev' is only used to filter the channel in > dw_spi_dma_chan_filer(). After using it, we need to use pci_dev_put() to > decrease the reference count. Also add pci_dev_put() for the error case. > > Applied to broonie/spi.git for-next Thanks! [1/1] spi: dw-dma: decrease reference count in dw_spi_dma_init_mfld() commit: 804313b64e412a81b0b3389a10e7622452004aa6 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi-dw-dma.c b/drivers/spi/spi-dw-dma.c index 1322b8cce5b7..ababb910b391 100644 --- a/drivers/spi/spi-dw-dma.c +++ b/drivers/spi/spi-dw-dma.c @@ -128,12 +128,15 @@ static int dw_spi_dma_init_mfld(struct device *dev, struct dw_spi *dws) dw_spi_dma_sg_burst_init(dws); + pci_dev_put(dma_dev); + return 0; free_rxchan: dma_release_channel(dws->rxchan); dws->rxchan = NULL; err_exit: + pci_dev_put(dma_dev); return -EBUSY; }
pci_get_device() will increase the reference count for the returned pci_dev. Since 'dma_dev' is only used to filter the channel in dw_spi_dma_chan_filer(). After using it, we need to use pci_dev_put() to decrease the reference count. Also add pci_dev_put() for the error case. Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support") Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> --- drivers/spi/spi-dw-dma.c | 3 +++ 1 file changed, 3 insertions(+)