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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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
