| Message ID | 20240906031345.1052241-2-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spi: geni-qcom: Fix incorrect free_irq() sequence | expand |
Hi Jinjie, On 9/6/2024 8:43 AM, Jinjie Ruan wrote: > In spi_geni_remove(), the IRQ will still remain and it's interrupt handler > may use the dma channel after release dma channel and before free irq, > which is not secure, fix it. > What's the possibility of having irq if spi_geni_release_dma_chan(mas) is completed ? As such controller is already unregistered so transfer request can't come. > Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- > drivers/spi/spi-geni-qcom.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index 37ef8c40b276..fc2819effe2d 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct platform_device *pdev) > /* Unregister _before_ disabling pm_runtime() so we stop transfers */ > spi_unregister_controller(spi); > > + free_irq(mas->irq, spi); > + > spi_geni_release_dma_chan(mas); > > - free_irq(mas->irq, spi); > pm_runtime_disable(&pdev->dev); > } >
On 2024/9/6 12:27, Mukesh Kumar Savaliya wrote: > Hi Jinjie, > > On 9/6/2024 8:43 AM, Jinjie Ruan wrote: >> In spi_geni_remove(), the IRQ will still remain and it's interrupt >> handler >> may use the dma channel after release dma channel and before free irq, >> which is not secure, fix it. >> > What's the possibility of having irq if spi_geni_release_dma_chan(mas) > is completed ? As such controller is already unregistered so transfer > request can't come. The irq is not freed, the IRQ can come and then it may enter the irq handler with the registered one. >> Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- >> drivers/spi/spi-geni-qcom.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c >> index 37ef8c40b276..fc2819effe2d 100644 >> --- a/drivers/spi/spi-geni-qcom.c >> +++ b/drivers/spi/spi-geni-qcom.c >> @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct >> platform_device *pdev) >> /* Unregister _before_ disabling pm_runtime() so we stop >> transfers */ >> spi_unregister_controller(spi); >> + free_irq(mas->irq, spi); >> + >> spi_geni_release_dma_chan(mas); >> - free_irq(mas->irq, spi); >> pm_runtime_disable(&pdev->dev); >> } >> >
Thanks ! On 9/6/2024 12:17 PM, Jinjie Ruan wrote: > > > On 2024/9/6 12:27, Mukesh Kumar Savaliya wrote: >> Hi Jinjie, >> >> On 9/6/2024 8:43 AM, Jinjie Ruan wrote: >>> In spi_geni_remove(), the IRQ will still remain and it's interrupt >>> handler >>> may use the dma channel after release dma channel and before free irq, >>> which is not secure, fix it. >>> >> What's the possibility of having irq if spi_geni_release_dma_chan(mas) >> is completed ? As such controller is already unregistered so transfer >> request can't come. > > The irq is not freed, the IRQ can come and then it may enter the irq > handler with the registered one. > My question is about knowing source of interrupt at earlier place. By just moving it above to spi_geni_release_dma_chan(), is there anything changing ? >>> Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") >>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >>> --- >>> drivers/spi/spi-geni-qcom.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c >>> index 37ef8c40b276..fc2819effe2d 100644 >>> --- a/drivers/spi/spi-geni-qcom.c >>> +++ b/drivers/spi/spi-geni-qcom.c >>> @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct >>> platform_device *pdev) >>> /* Unregister _before_ disabling pm_runtime() so we stop >>> transfers */ >>> spi_unregister_controller(spi); >>> + free_irq(mas->irq, spi); >>> + >>> spi_geni_release_dma_chan(mas); >>> - free_irq(mas->irq, spi); >>> pm_runtime_disable(&pdev->dev); >>> } >>> >>
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 37ef8c40b276..fc2819effe2d 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -1170,9 +1170,10 @@ static void spi_geni_remove(struct platform_device *pdev) /* Unregister _before_ disabling pm_runtime() so we stop transfers */ spi_unregister_controller(spi); + free_irq(mas->irq, spi); + spi_geni_release_dma_chan(mas); - free_irq(mas->irq, spi); pm_runtime_disable(&pdev->dev); }
In spi_geni_remove(), the IRQ will still remain and it's interrupt handler may use the dma channel after release dma channel and before free irq, which is not secure, fix it. Fixes: b59c122484ec ("spi: spi-geni-qcom: Add support for GPI dma") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- drivers/spi/spi-geni-qcom.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)