From: Mike Leach <mike.leach@linaro.org> To: Robin Murphy <robin.murphy@arm.com> Cc: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>, Mathieu Poirier <mathieu.poirier@linaro.org>, Suzuki K Poulose <suzuki.poulose@arm.com>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, linux-arm-msm@vger.kernel.org, Coresight ML <coresight@lists.linaro.org>, Stephen Boyd <swboyd@chromium.org> Subject: Re: [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF Date: Wed, 3 Jun 2020 14:51:10 +0100 [thread overview] Message-ID: <CAJ9a7Vj3sL=4O3DU+aJWYLhue1UxQmX4Ba5JdEnmxKDEYo_z4Q@mail.gmail.com> (raw) In-Reply-To: <1a5a6a6d-b86d-df45-cf91-7081e70d88a3@arm.com> Hi, On Wed, 3 Jun 2020 at 14:34, Robin Murphy <robin.murphy@arm.com> wrote: > > On 2020-06-03 14:22, Mike Leach wrote: > > Hi Sai, > > > > On Wed, 3 Jun 2020 at 13:14, Sai Prakash Ranjan > > <saiprakash.ranjan@codeaurora.org> wrote: > >> > >> Hi Mike, > >> > >> On 2020-06-03 16:57, Mike Leach wrote: > >>> Hi, > >>> > >>> On Wed, 3 Jun 2020 at 11:24, Sai Prakash Ranjan > >>> <saiprakash.ranjan@codeaurora.org> wrote: > >>>> > >>>> Hi Mike, > >>>> > >>>> Thanks again for looking at this. > >>>> > >>>> On 2020-06-03 03:42, Mike Leach wrote: > >>>> [...] > >>>> > >>>>>> > >>>>>> SMMU/IOMMU won't be able to do much here as it is the client's > >>>>>> responsiblity to > >>>>>> properly shutdown and SMMU device link just makes sure that > >>>>>> SMMU(supplier) shutdown is > >>>>>> called only after its consumers shutdown callbacks are called. > >>>>> > >>>>> I think this use case can be handled slightly differently than the > >>>>> general requirements for modular CoreSight drivers. > >>>>> > >>>>> What is needed here is a way of stopping the underlying ETR hardware > >>>>> from issuing data to the SMMU, until the entire device has been shut > >>>>> down, in a way that does not remove the driver, breaking existing > >>>>> references and causing a system crash. > >>>>> > >>>>> We could introduce a new mode to the ETR driver - e.g. > >>>>> CS_MODE_SHUTDOWN. > >>>>> > >>>>> At the end of the block tmc_shutdown(struct amba_device *adev), set > >>>>> drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). > >>>>> This new mode can be used to prevent the underlying hardware from > >>>>> being able to restart until the device is re-powered. > >>>>> > >>>>> This mode can be detected in the code that enables / disables the ETR > >>>>> and handled appropriately (updates to tmc_enable_etr_sink and > >>>>> tmc_disable_etr_sink). > >>>>> This mode will persist until the device is re-started - but because we > >>>>> are on the device shutdown path this is not an issue. > >>>>> > >>>>> This should leave the CoreSight infrastructure stable until the > >>>>> drivers are shut down normally as part of the device power down > >>>>> process. > >>>>> > >>>> > >>>> Sounds good to me, but if the coresight_unregister() is the trouble > >>>> point > >>>> causing these crashes, then can't we just remove that from > >>>> tmc_shutdown() > >>>> callback? This would be like maintaining the same behaviour as now > >>>> where > >>>> on reboot/shutdown we basically don't do anything except for disabling > >>>> ETR. > >>> > >>> No - the new mode prevents race conditions where the thread shutting > >>> down the SMMU does the ETR shutdown, but then another thread happens > >>> to be trying to start trace and restarts the ETR. > >>> It also prevents the condition Mathieu discussed where a thread might > >>> be attempting to shutdown trace - this could try to disable the > >>> hardware again re-releasing resources/ re-flushing and waiting for > >>> stop. > >>> > >> > >> I do not think there will a race between SMMU shutdown and ETR shutdown. > >> Driver core takes care of calling SMMU shutdown after its consumer > >> shutdown callbacks via device link, otherwise there would already be > >> bugs in all other client drivers. > >> > > > > I am not saying there could be a race between tmc_shutdowm and > > Smmu_shutdown - there may be a case if the coresight_disable_path > > sequence is running and gets to the point of disabling the ETR after > > the SMMU callback has disabled it. > > I'm confused now - there is no "SMMU callback", we're talking about the > system-wide cleanup from kernel_shutdown_prepare() or > kernel_restart_prepare(). As far as I'm aware userspace should be long > gone by that point, so although trace may have been left running || ((offset >= TRCCIDCVRn(0)) && (offset <= TRCVMIDCVRn(7)), the > chance of racing against other driver operations seems pretty unlikely. > Sorry - bad choice of terminology. I was referring to the SMMU ensuring that it had all its clients shut-down before if shut down. To quote Sai... >>>>> SMMU device link just makes sure that > >>>>>> SMMU(supplier) shutdown is > >>>>>> called only after its consumers shutdown callbacks are called. I agree it is unlikely, but if removing the device from the CoreSight infrastructure via coresight_unregister() is a potential source of a crash, it would seem that there is a potential path where some CoreSight driver side work might be possible. therefore a mode to prevent this crash, and ensure that the device hardware remains off and not sending trace to SMMU until such time as shutdown / reboot restart occurs, seemed prudent. Mike > Robin. -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
WARNING: multiple messages have this Message-ID (diff)
From: Mike Leach <mike.leach@linaro.org> To: Robin Murphy <robin.murphy@arm.com> Cc: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>, Mathieu Poirier <mathieu.poirier@linaro.org>, Suzuki K Poulose <suzuki.poulose@arm.com>, linux-arm-msm@vger.kernel.org, Coresight ML <coresight@lists.linaro.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org>, linux-arm-kernel <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF Date: Wed, 3 Jun 2020 14:51:10 +0100 [thread overview] Message-ID: <CAJ9a7Vj3sL=4O3DU+aJWYLhue1UxQmX4Ba5JdEnmxKDEYo_z4Q@mail.gmail.com> (raw) In-Reply-To: <1a5a6a6d-b86d-df45-cf91-7081e70d88a3@arm.com> Hi, On Wed, 3 Jun 2020 at 14:34, Robin Murphy <robin.murphy@arm.com> wrote: > > On 2020-06-03 14:22, Mike Leach wrote: > > Hi Sai, > > > > On Wed, 3 Jun 2020 at 13:14, Sai Prakash Ranjan > > <saiprakash.ranjan@codeaurora.org> wrote: > >> > >> Hi Mike, > >> > >> On 2020-06-03 16:57, Mike Leach wrote: > >>> Hi, > >>> > >>> On Wed, 3 Jun 2020 at 11:24, Sai Prakash Ranjan > >>> <saiprakash.ranjan@codeaurora.org> wrote: > >>>> > >>>> Hi Mike, > >>>> > >>>> Thanks again for looking at this. > >>>> > >>>> On 2020-06-03 03:42, Mike Leach wrote: > >>>> [...] > >>>> > >>>>>> > >>>>>> SMMU/IOMMU won't be able to do much here as it is the client's > >>>>>> responsiblity to > >>>>>> properly shutdown and SMMU device link just makes sure that > >>>>>> SMMU(supplier) shutdown is > >>>>>> called only after its consumers shutdown callbacks are called. > >>>>> > >>>>> I think this use case can be handled slightly differently than the > >>>>> general requirements for modular CoreSight drivers. > >>>>> > >>>>> What is needed here is a way of stopping the underlying ETR hardware > >>>>> from issuing data to the SMMU, until the entire device has been shut > >>>>> down, in a way that does not remove the driver, breaking existing > >>>>> references and causing a system crash. > >>>>> > >>>>> We could introduce a new mode to the ETR driver - e.g. > >>>>> CS_MODE_SHUTDOWN. > >>>>> > >>>>> At the end of the block tmc_shutdown(struct amba_device *adev), set > >>>>> drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). > >>>>> This new mode can be used to prevent the underlying hardware from > >>>>> being able to restart until the device is re-powered. > >>>>> > >>>>> This mode can be detected in the code that enables / disables the ETR > >>>>> and handled appropriately (updates to tmc_enable_etr_sink and > >>>>> tmc_disable_etr_sink). > >>>>> This mode will persist until the device is re-started - but because we > >>>>> are on the device shutdown path this is not an issue. > >>>>> > >>>>> This should leave the CoreSight infrastructure stable until the > >>>>> drivers are shut down normally as part of the device power down > >>>>> process. > >>>>> > >>>> > >>>> Sounds good to me, but if the coresight_unregister() is the trouble > >>>> point > >>>> causing these crashes, then can't we just remove that from > >>>> tmc_shutdown() > >>>> callback? This would be like maintaining the same behaviour as now > >>>> where > >>>> on reboot/shutdown we basically don't do anything except for disabling > >>>> ETR. > >>> > >>> No - the new mode prevents race conditions where the thread shutting > >>> down the SMMU does the ETR shutdown, but then another thread happens > >>> to be trying to start trace and restarts the ETR. > >>> It also prevents the condition Mathieu discussed where a thread might > >>> be attempting to shutdown trace - this could try to disable the > >>> hardware again re-releasing resources/ re-flushing and waiting for > >>> stop. > >>> > >> > >> I do not think there will a race between SMMU shutdown and ETR shutdown. > >> Driver core takes care of calling SMMU shutdown after its consumer > >> shutdown callbacks via device link, otherwise there would already be > >> bugs in all other client drivers. > >> > > > > I am not saying there could be a race between tmc_shutdowm and > > Smmu_shutdown - there may be a case if the coresight_disable_path > > sequence is running and gets to the point of disabling the ETR after > > the SMMU callback has disabled it. > > I'm confused now - there is no "SMMU callback", we're talking about the > system-wide cleanup from kernel_shutdown_prepare() or > kernel_restart_prepare(). As far as I'm aware userspace should be long > gone by that point, so although trace may have been left running || ((offset >= TRCCIDCVRn(0)) && (offset <= TRCVMIDCVRn(7)), the > chance of racing against other driver operations seems pretty unlikely. > Sorry - bad choice of terminology. I was referring to the SMMU ensuring that it had all its clients shut-down before if shut down. To quote Sai... >>>>> SMMU device link just makes sure that > >>>>>> SMMU(supplier) shutdown is > >>>>>> called only after its consumers shutdown callbacks are called. I agree it is unlikely, but if removing the device from the CoreSight infrastructure via coresight_unregister() is a potential source of a crash, it would seem that there is a potential path where some CoreSight driver side work might be possible. therefore a mode to prevent this crash, and ensure that the device hardware remains off and not sending trace to SMMU until such time as shutdown / reboot restart occurs, seemed prudent. Mike > Robin. -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-06-03 13:51 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-06-01 8:02 [PATCH 0/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF Sai Prakash Ranjan 2020-06-01 8:02 ` Sai Prakash Ranjan 2020-06-01 8:02 ` [PATCH 1/2] coresight: tmc: Add enable flag to indicate the status of ETR/ETF Sai Prakash Ranjan 2020-06-01 8:02 ` Sai Prakash Ranjan 2020-06-01 13:27 ` Mike Leach 2020-06-01 13:27 ` Mike Leach 2020-06-01 17:13 ` Sai Prakash Ranjan 2020-06-01 17:13 ` Sai Prakash Ranjan 2020-06-01 8:02 ` [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF Sai Prakash Ranjan 2020-06-01 8:02 ` Sai Prakash Ranjan 2020-06-01 13:35 ` Mike Leach 2020-06-01 13:35 ` Mike Leach 2020-06-01 17:15 ` Sai Prakash Ranjan 2020-06-01 17:15 ` Sai Prakash Ranjan 2020-06-01 21:28 ` Mathieu Poirier 2020-06-01 21:28 ` Mathieu Poirier 2020-06-02 7:30 ` Sai Prakash Ranjan 2020-06-02 7:30 ` Sai Prakash Ranjan 2020-06-02 22:12 ` Mike Leach 2020-06-02 22:12 ` Mike Leach 2020-06-03 10:24 ` Sai Prakash Ranjan 2020-06-03 10:24 ` Sai Prakash Ranjan 2020-06-03 11:27 ` Mike Leach 2020-06-03 11:27 ` Mike Leach 2020-06-03 12:14 ` Sai Prakash Ranjan 2020-06-03 12:14 ` Sai Prakash Ranjan 2020-06-03 13:22 ` Mike Leach 2020-06-03 13:22 ` Mike Leach 2020-06-03 13:34 ` Robin Murphy 2020-06-03 13:34 ` Robin Murphy 2020-06-03 13:43 ` Sai Prakash Ranjan 2020-06-03 13:43 ` Sai Prakash Ranjan 2020-06-03 13:51 ` Mike Leach [this message] 2020-06-03 13:51 ` Mike Leach 2020-06-03 14:02 ` Sai Prakash Ranjan 2020-06-03 14:02 ` Sai Prakash Ranjan 2020-06-03 17:44 ` Mathieu Poirier 2020-06-03 17:44 ` Mathieu Poirier 2020-06-04 7:27 ` Sai Prakash Ranjan 2020-06-04 7:27 ` Sai Prakash Ranjan 2020-06-08 14:07 ` Sai Prakash Ranjan 2020-06-08 14:07 ` Sai Prakash Ranjan 2020-06-09 15:27 ` Mathieu Poirier 2020-06-09 15:27 ` Mathieu Poirier 2020-06-09 15:37 ` Sai Prakash Ranjan 2020-06-09 15:37 ` Sai Prakash Ranjan 2020-06-03 11:37 ` Robin Murphy 2020-06-03 11:37 ` Robin Murphy 2020-06-03 12:00 ` Sai Prakash Ranjan 2020-06-03 12:00 ` Sai Prakash Ranjan 2020-06-03 12:21 ` Robin Murphy 2020-06-03 12:21 ` Robin Murphy 2020-06-03 12:26 ` Sai Prakash Ranjan 2020-06-03 12:26 ` Sai Prakash Ranjan 2020-06-03 13:40 ` Robin Murphy 2020-06-03 13:40 ` Robin Murphy 2020-06-03 13:51 ` Sai Prakash Ranjan 2020-06-03 13:51 ` Sai Prakash Ranjan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAJ9a7Vj3sL=4O3DU+aJWYLhue1UxQmX4Ba5JdEnmxKDEYo_z4Q@mail.gmail.com' \ --to=mike.leach@linaro.org \ --cc=coresight@lists.linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=robin.murphy@arm.com \ --cc=saiprakash.ranjan@codeaurora.org \ --cc=suzuki.poulose@arm.com \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.