lkml.org 
[lkml]   [2020]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF
    Hi Mike,

    On 2020-06-03 19:04, Robin Murphy 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, the
    > chance of racing against other driver operations seems pretty
    > unlikely.
    >

    As Robin said, it is not SMMU callback but the normal reboot/shutdown
    flow and race is unlikely at that point.

    tmc_shutdown()
    platform_drv_shutdown()
    device_shutdown()
    kernel_restart_prepare()
    kernel_restart()

    If I am not clear enough, first all the consumer shutdown callbacks of
    SMMU
    are called like above tmc_shutdown() and then we call the
    arm_smmu_device_shutdown(),
    this ordering is ensured by the device links.

    Thanks,
    Sai

    --
    QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
    member
    of Code Aurora Forum, hosted by The Linux Foundation

    \
     
     \ /
      Last update: 2020-06-03 15:44    [W:8.472 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site