Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Vinod Koul]

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

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Vinod Koul]

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

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

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 
 ---
  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 = >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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Lars-Peter Clausen]

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 
>>> ---
>>>  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 = >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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

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 
>> ---
>>  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 = >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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Lars-Peter Clausen]

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 
> ---
>  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 = >channels[i];
> + disable_irq(tdc->irq);

How about just calling free_irq()? That's how you'd typically handle this.

>   tasklet_kill(>tasklet);
>   }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Lars-Peter Clausen]

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 
>>> ---
>>>  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 = >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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Lars-Peter Clausen]

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 
> ---
>  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 = >channels[i];
> + disable_irq(tdc->irq);

How about just calling free_irq()? That's how you'd typically handle this.

>   tasklet_kill(>tasklet);
>   }
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

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 
>> ---
>>  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 = >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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

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 
 ---
  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 = >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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/