Re: [PATCH] spi: dw-dma: decrease reference count in dw_spi_dma_init_mfld()

[Serge Semin <fancer.lancer@xxxxxxxxx>]

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@xxxxxxxxxx>
> > 
> > Nice catch. Thanks for the patch.
> > Acked-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > 
> > -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
> >>
> > .
> >