| Message ID | 1444983957-18691-7-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 10/16/2015 10:25 AM, Jon Hunter wrote: > On driver removal, before killing any tasklets, ensure that the channel > interrupts are disabled so that the tasklet will not try to run during > or after the removal of the driver. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/dma/tegra20-apb-dma.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > index 2bfab8d28b53..0dd6e7deaa8e 100644 > --- a/drivers/dma/tegra20-apb-dma.c > +++ b/drivers/dma/tegra20-apb-dma.c > @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev) > > for (i = 0; i < tdma->chip_data->nr_channels; ++i) { > tdc = &tdma->channels[i]; > + disable_irq(tdc->irq); How about just calling free_irq()? That's how you'd typically handle this. > tasklet_kill(&tdc->tasklet); > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/10/15 09:53, Lars-Peter Clausen wrote: > On 10/16/2015 10:25 AM, Jon Hunter wrote: >> On driver removal, before killing any tasklets, ensure that the channel >> interrupts are disabled so that the tasklet will not try to run during >> or after the removal of the driver. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/dma/tegra20-apb-dma.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >> index 2bfab8d28b53..0dd6e7deaa8e 100644 >> --- a/drivers/dma/tegra20-apb-dma.c >> +++ b/drivers/dma/tegra20-apb-dma.c >> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev) >> >> for (i = 0; i < tdma->chip_data->nr_channels; ++i) { >> tdc = &tdma->channels[i]; >> + disable_irq(tdc->irq); > > How about just calling free_irq()? That's how you'd typically handle this. Yes, however, the interrupt is requested by devm_request_irq(). I guess I could call devm_free_irq() here? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/16/2015 11:29 AM, Jon Hunter wrote: > > On 16/10/15 09:53, Lars-Peter Clausen wrote: >> On 10/16/2015 10:25 AM, Jon Hunter wrote: >>> On driver removal, before killing any tasklets, ensure that the channel >>> interrupts are disabled so that the tasklet will not try to run during >>> or after the removal of the driver. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>> --- >>> drivers/dma/tegra20-apb-dma.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >>> index 2bfab8d28b53..0dd6e7deaa8e 100644 >>> --- a/drivers/dma/tegra20-apb-dma.c >>> +++ b/drivers/dma/tegra20-apb-dma.c >>> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev) >>> >>> for (i = 0; i < tdma->chip_data->nr_channels; ++i) { >>> tdc = &tdma->channels[i]; >>> + disable_irq(tdc->irq); >> >> How about just calling free_irq()? That's how you'd typically handle this. > > Yes, however, the interrupt is requested by devm_request_irq(). I guess > I could call devm_free_irq() here? Just use request_irq() instead of devm_request_irq(). You have the same issue on the error path in the probe function anyway and also need to add the free_irq() before the tasklet_kill() there as well. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/10/15 11:40, Lars-Peter Clausen wrote: > On 10/16/2015 11:29 AM, Jon Hunter wrote: >> >> On 16/10/15 09:53, Lars-Peter Clausen wrote: >>> On 10/16/2015 10:25 AM, Jon Hunter wrote: >>>> On driver removal, before killing any tasklets, ensure that the channel >>>> interrupts are disabled so that the tasklet will not try to run during >>>> or after the removal of the driver. >>>> >>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >>>> --- >>>> drivers/dma/tegra20-apb-dma.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >>>> index 2bfab8d28b53..0dd6e7deaa8e 100644 >>>> --- a/drivers/dma/tegra20-apb-dma.c >>>> +++ b/drivers/dma/tegra20-apb-dma.c >>>> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev) >>>> >>>> for (i = 0; i < tdma->chip_data->nr_channels; ++i) { >>>> tdc = &tdma->channels[i]; >>>> + disable_irq(tdc->irq); >>> >>> How about just calling free_irq()? That's how you'd typically handle this. >> >> Yes, however, the interrupt is requested by devm_request_irq(). I guess >> I could call devm_free_irq() here? > > Just use request_irq() instead of devm_request_irq(). You have the same > issue on the error path in the probe function anyway and also need to add > the free_irq() before the tasklet_kill() there as well. I was wondering about that but the tasklets should never be scheduled if the probe does not succeed, so I think it is ok. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 16, 2015 at 11:57:02AM +0100, Jon Hunter wrote: > >>> How about just calling free_irq()? That's how you'd typically handle this. > >> > >> Yes, however, the interrupt is requested by devm_request_irq(). I guess > >> I could call devm_free_irq() here? > > > > Just use request_irq() instead of devm_request_irq(). You have the same > > issue on the error path in the probe function anyway and also need to add > > the free_irq() before the tasklet_kill() there as well. > > I was wondering about that but the tasklets should never be scheduled if > the probe does not succeed, so I think it is ok. This is actually very racy, if probe fails but due to devm_ calls your irq is alive till it freed by core And a faulty device triggering irq can complicate matters, so for irq IMHO we don't get much benefit with devm_ variant
On 28/10/15 06:53, Vinod Koul wrote: > On Fri, Oct 16, 2015 at 11:57:02AM +0100, Jon Hunter wrote: >>>>> How about just calling free_irq()? That's how you'd typically handle this. >>>> >>>> Yes, however, the interrupt is requested by devm_request_irq(). I guess >>>> I could call devm_free_irq() here? >>> >>> Just use request_irq() instead of devm_request_irq(). You have the same >>> issue on the error path in the probe function anyway and also need to add >>> the free_irq() before the tasklet_kill() there as well. >> >> I was wondering about that but the tasklets should never be scheduled if >> the probe does not succeed, so I think it is ok. > > This is actually very racy, if probe fails but due to devm_ calls your irq > is alive till it freed by core > > And a faulty device triggering irq can complicate matters, so for irq IMHO > we don't get much benefit with devm_ variant That's fine, I will drop the devm_ usage here then. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index 2bfab8d28b53..0dd6e7deaa8e 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev) for (i = 0; i < tdma->chip_data->nr_channels; ++i) { tdc = &tdma->channels[i]; + disable_irq(tdc->irq); tasklet_kill(&tdc->tasklet); }
On driver removal, before killing any tasklets, ensure that the channel interrupts are disabled so that the tasklet will not try to run during or after the removal of the driver. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/dma/tegra20-apb-dma.c | 1 + 1 file changed, 1 insertion(+)