Re: [PATCH 2/2] coresight: tmc: Add shutdown callback for TMC ETR/ETF

[Sai Prakash Ranjan]

Hi Mathieu,

+Will

On 2020-06-03 23:14, Mathieu Poirier wrote:
> On Wed, Jun 03, 2020 at 02:34:10PM +0100, 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.
>
> Robin has a point - user space is long gone at this time.  As such the 
> first
> question to ask is what kind of CS session was running at the time the 
> system
> was shutting down.  Was it a perf session of a sysfs session?
>
> I'm guessing it was a sysfs session because user space has been blown 
> away a
> while back and part of that process should have killed all perf 
> sessions.

I was enabling trace via sysfs.

> If I am correct then simply switching off the ETR HW in the shutdown() 
> amba bus
> callback should be fine - otherwise Mike's approach is mandatory.  
> There is
> also the exchange between Robin and Sai about removing the SMMU 
> shutdown
> callback, but that thread is still incomplete.

If Robin is hinting at removing SMMU shutdown callback, then I think 
adding
all these shutdown callbacks to all clients of SMMU can be avoided. Git 
blaming
the thing shows it was added to avoid some kexec memory corruption.

Thanks,
Sai

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