iio: at91-sama5d2_adc: Fix use after free bug in at91_adc_remove due to race condition
Message ID | 20230310091239.1440279-1-zyytlz.wz@163.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp770197wrd; Fri, 10 Mar 2023 01:27:28 -0800 (PST) X-Google-Smtp-Source: AK7set/HNxm/lYO94Y7iZRLB1tF+eBJFUYo9GnbKdbn1t4m2LIweoWxkrOY+9WkoVASttflXy/Te X-Received: by 2002:a17:902:bc42:b0:19c:faaf:b9d6 with SMTP id t2-20020a170902bc4200b0019cfaafb9d6mr22658783plz.8.1678440448098; Fri, 10 Mar 2023 01:27:28 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1678440448; cv=none; d=google.com; s=arc-20160816; b=Vk3XlPnmQ5FZJCBKqhbCkIqNe4uOf9E/5sQhPwxqKz6s/64phHUx8z5Uhks8nihGoR rXzqFV0IpKuNI2fERNEMA0/+bxQqPYRN5Eggg1N662h5zLUyoZjRlB2Y4rX31dSSVHlQ zP8ut3/GkcCIfFuCE5mB7L6Xee06eiydw+aMFQc/fSA8kMdGS8qotB0WfmQUucGGaDFc yhSKNBymb7uJwz0KrBJYYxGuE6MsRtsUSskhu5sHiVAl347qXqWVkJrikjfxBT/KCyEC DE19ZGLitt6GG4xNaZOyViDDfKVjzOO0s/jsvgXGr+qr5ffotrUCh6IcdNMliyGMJBEl ShXg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=y8znzv8EogzdwmgG5+xhB25PTiRTxgErQ92Q7uiUT9o=; b=AdZ1mq3lL4XViavNKP3f74AOj80ExpJYg61wsvFfqffEgNjw7dJkGFmS0QUt/SLCBz i4ptHd2A090sbO7n9w7AT8kfRhGB/v0mMo5NLxn4z0XGi0MeYBOjyK5o2Kl47DKZQISW cWh9t8gE1jc3TtIVh3acudfWpX4f2tNvvGJuMxDsjMfqM6cR9UBE6/6GHyerwVk/3oHX VtaKDMbNOOVIvpZCF7IvWgGAp6Np2l8Glm2W1Zpsgve7CcDFFkQifyTOavuJZUWStAvl yiDAn3K/41/Qs1k7wgq7rCATs0E+WtQXLiK17nor+7o0CryblFiG9Nrf2/LL5Trj0+Jl QKwA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@163.com header.s=s110527 header.b=jxTSZ8sp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ld5-20020a170902fac500b0019edb3ae424si1702264plb.20.2023.03.10.01.27.15; Fri, 10 Mar 2023 01:27:28 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@163.com header.s=s110527 header.b=jxTSZ8sp; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=163.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231438AbjCJJR6 (ORCPT <rfc822;carlos.wei.hk@gmail.com> + 99 others); Fri, 10 Mar 2023 04:17:58 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229874AbjCJJR1 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 10 Mar 2023 04:17:27 -0500 Received: from m12.mail.163.com (m12.mail.163.com [220.181.12.198]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 5CAEE90098; Fri, 10 Mar 2023 01:13:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=163.com; s=s110527; h=From:Subject:Date:Message-Id:MIME-Version; bh=y8znz v8EogzdwmgG5+xhB25PTiRTxgErQ92Q7uiUT9o=; b=jxTSZ8spskYRjkBy3BupA T6GOPeknXjQlZLWEjGDogJb4h4jDqzEu2KwONowyqwxW5f8twLSzMula8L0//fce mRrDMyv6JPV3h5PJOk+FTlO63Hk6Rh7TMwdG281UGJM5dfyOWoln3+4unHbDNVq8 AJu/q4JqQY0rufwYmoJeGA= Received: from leanderwang-LC2.localdomain (unknown [111.206.145.21]) by zwqz-smtp-mta-g2-1 (Coremail) with SMTP id _____wD3_5uI9Apkyr5QCw--.41643S2; Fri, 10 Mar 2023 17:12:40 +0800 (CST) From: Zheng Wang <zyytlz.wz@163.com> To: eugen.hristev@collabora.com Cc: jic23@kernel.org, lars@metafoo.de, nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com, claudiu.beznea@microchip.com, linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Zheng Wang <zyytlz.wz@163.com> Subject: [PATCH] iio: at91-sama5d2_adc: Fix use after free bug in at91_adc_remove due to race condition Date: Fri, 10 Mar 2023 17:12:39 +0800 Message-Id: <20230310091239.1440279-1-zyytlz.wz@163.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID: _____wD3_5uI9Apkyr5QCw--.41643S2 X-Coremail-Antispam: 1Uf129KBjvJXoWxJr17XrWUCFW3ur1DtF4Dtwb_yoW8GF4Dpa na9FWDCFW7WF10qa1UA34jyFy8t3W5Kw10grZxua43uw45ZryYvr15Ka48XFW5tFyjyFsr XFy8Kw45KF1jyrJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x0zRnmRUUUUUU= X-Originating-IP: [111.206.145.21] X-CM-SenderInfo: h2113zf2oz6qqrwthudrp/1tbiXQ4uU1WBo3P3ewABsn X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759972371415110169?= X-GMAIL-MSGID: =?utf-8?q?1759972371415110169?= |
Series |
iio: at91-sama5d2_adc: Fix use after free bug in at91_adc_remove due to race condition
|
|
Commit Message
Zheng Wang
March 10, 2023, 9:12 a.m. UTC
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@163.com>
---
drivers/iio/adc/at91-sama5d2_adc.c | 2 ++
1 file changed, 2 insertions(+)
Comments
On 3/18/23 10:39, Jonathan Cameron wrote: > On Fri, 10 Mar 2023 17:12:39 +0800 > Zheng Wang <zyytlz.wz@163.com> 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@163.com> >> --- >> 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().
On Fri, 10 Mar 2023 17:12:39 +0800 Zheng Wang <zyytlz.wz@163.com> 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@163.com> > --- > 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. > iio_device_unregister(indio_dev); > > at91_adc_dma_disable(st);
On Sat, 18 Mar 2023 10:36:04 -0700 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 3/18/23 10:39, Jonathan Cameron wrote: > > On Fri, 10 Mar 2023 17:12:39 +0800 > > Zheng Wang <zyytlz.wz@163.com> 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@163.com> > >> --- > >> 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(). I'd go a little further than that and disable the interrupt source at the device (if possible) then call free_irq() then cancel_work_sync() Otherwise the device is merrily monitoring something and generating interrupts that we don't care about. Might well be wasting power doing that, though I haven't checked the flow in this particular case. Jonathan
At 2023-03-19 00:39:13, "Jonathan Cameron" <jic23@kernel.org> wrote: >On Fri, 10 Mar 2023 17:12:39 +0800 >Zheng Wang <zyytlz.wz@163.com> 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@163.com> >> --- >> 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. Hi, Sorry for my late reply. I think we need to replace disable_irq_nosync with free_irq. Thanks, Zheng > >> iio_device_unregister(indio_dev); >> >> at91_adc_dma_disable(st);
At 2023-03-19 00:36:04, "Lars-Peter Clausen" <lars@metafoo.de> wrote: >On 3/18/23 10:39, Jonathan Cameron wrote: >> On Fri, 10 Mar 2023 17:12:39 +0800 >> Zheng Wang <zyytlz.wz@163.com> 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@163.com> >>> --- >>> 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 >
At 2023-03-19 22:22:22, "Jonathan Cameron" <jic23@kernel.org> wrote: >On Sat, 18 Mar 2023 10:36:04 -0700 >Lars-Peter Clausen <lars@metafoo.de> wrote: > >> On 3/18/23 10:39, Jonathan Cameron wrote: >> > On Fri, 10 Mar 2023 17:12:39 +0800 >> > Zheng Wang <zyytlz.wz@163.com> 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@163.com> >> >> --- >> >> 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(). > >I'd go a little further than that and disable the interrupt source at the >device (if possible) then call free_irq() then cancel_work_sync() > >Otherwise the device is merrily monitoring something and generating interrupts >that we don't care about. Might well be wasting power doing that, though I haven't >checked the flow in this particular case. > Dear Lars-Peter Clausen, Thank you for your response to my question. I appreciate your suggestion to disable the interrupt source at the device before calling free_irq() and cancel_work_sync(). However, I am not sure which specific function to use in order to achieve this. Can you please provide more information on which function to use and how to use it? Thank you very much for your help. Best regards, Zheng Wang >Jonathan
On 10/03/2023 at 10:12, Zheng Wang 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 There is no such thing as a SMP platform using this driver (yet?), so we agree that this fix is purely theoretical, cannot be reproduced nor its fix validated. That being said, I'm happy that enhancements are provided to this driver, no doubt about that. > Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels") > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > --- > 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); About stopping the source of interrupt, I would recommend using a sequence already exposed in at91_adc_hw_init (and possibly make it common), like: if (st->soc_info.platform->layout->EOC_IDR) at91_adc_writel(st, EOC_IDR, 0xffffffff); at91_adc_writel(st, IDR, 0xffffffff); Regards, Nicolas > iio_device_unregister(indio_dev); > > at91_adc_dma_disable(st); > -- > 2.25.1 >
At 2023-03-20 16:35:24, "Nicolas Ferre" <nicolas.ferre@microchip.com> wrote: >On 10/03/2023 at 10:12, Zheng Wang 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 > >There is no such thing as a SMP platform using this driver (yet?), so we >agree that this fix is purely theoretical, cannot be reproduced nor its >fix validated. > >That being said, I'm happy that enhancements are provided to this >driver, no doubt about that. > Hi Nicolas, Thanks for your reply. I'm not familiar with the module and I think you're right. > >> Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels") >> Signed-off-by: Zheng Wang <zyytlz.wz@163.com> >> --- >> 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); > >About stopping the source of interrupt, I would recommend using a >sequence already exposed in at91_adc_hw_init (and possibly make it >common), like: > > if (st->soc_info.platform->layout->EOC_IDR) > at91_adc_writel(st, EOC_IDR, 0xffffffff); > at91_adc_writel(st, IDR, 0xffffffff); > Thanks fou your advice. I'll apply it in the next version. Best regards, Zheng >Regards, > Nicolas > >> iio_device_unregister(indio_dev); >> >> at91_adc_dma_disable(st); >> -- >> 2.25.1 >> > >-- >Nicolas Ferre
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); iio_device_unregister(indio_dev); at91_adc_dma_disable(st);