At 2023-03-19 00:36:04, "Lars-Peter Clausen" <lars@xxxxxxxxxx> wrote:
>On 3/18/23 10:39, Jonathan Cameron wrote:
>> On Fri, 10 Mar 2023 17:12:39 +0800
>> Zheng Wang <zyytlz.wz@xxxxxxx> wrote:
>>
>>> In at91_adc_probe, &st->touch_st.workq is bound with
>>> at91_adc_workq_handler. Then it will be started by irq
>>> handler at91_adc_touch_data_handler
>>>
>>> If we remove the driver which will call at91_adc_remove
>>> to make cleanup, there may be a unfinished work.
>>>
>>> The possible sequence is as follows:
>>>
>>> Fix it by finishing the work before cleanup in the at91_adc_remove
>>>
>>> CPU0 CPU1
>>>
>>> |at91_adc_workq_handler
>>> at91_adc_remove |
>>> iio_device_unregister|
>>> iio_dev_release |
>>> kfree(iio_dev_opaque);|
>>> |
>>> |iio_push_to_buffers
>>> |&iio_dev_opaque->buffer_list
>>> |//use
>>> Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")
>>> Signed-off-by: Zheng Wang <zyytlz.wz@xxxxxxx>
>>> ---
>>> drivers/iio/adc/at91-sama5d2_adc.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>>> index 50d02e5fc6fc..1b95d18d9e0b 100644
>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>>> @@ -2495,6 +2495,8 @@ static int at91_adc_remove(struct platform_device *pdev)
>>> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> struct at91_adc_state *st = iio_priv(indio_dev);
>>>
>>> + disable_irq_nosync(st->irq);
>>> + cancel_work_sync(&st->touch_st.workq);
>> I'd like some input form someone more familiar with this driver than I am.
>>
>> In particular, whilst it fixes the bug seen I'm not sure what the most
>> logical ordering for the disable is or the best way to do it.
>>
>> I'd prefer to see the irq cut off at source by disabling it at the device
>> feature that is generating the irq followed by cancelling or waiting for
>> completion of any in flight work.
>The usually way you'd do this by calling free_irq() before the
>cancel_work_sync().
Hi,
Thank you for your response and feedback on my patch. I appreciate your input and would like to address your concerns.
Regarding the best way to disable the IRQ, I agree that calling free_irq() before cancel_work_sync() would be a better approach. This ensures that the IRQ is completely disabled at the source, and any in-flight work is finished before removing the driver. I will make this change in the patch.
Best regards,
Zheng Wang
>
