Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Bjorn Andersson]

On Tue, May 21, 2024 at 09:36:03AM +0200, Krzysztof Kozlowski wrote:
> On 21/05/2024 06:08, Chris Lew wrote:
> > 
> > 
> > On 5/19/2024 10:36 AM, Krzysztof Kozlowski wrote:
> >> On 17/05/2024 00:58, Chris Lew wrote:
> >>> Add hwlocks property to describe the hwspinlock that remoteproc can try
> >>> to bust on behalf of the remoteproc's smem.
> >>
> >> Sorry, as you wrote, the lock is part of smem, not here. Drivers do not
> >> crash, so if your crashes as you imply in the cover letter, then first
> >> fix the driver.
> >>
> > 
> > Hi Krzysztof,
> > 
> > Sorry for the confusion, I dont think I meant that the smem driver will 
> > ever crash. The referred to crash in the cover letter is a crash in the 
> > firmware running on the remoteproc. The remoteproc could crash for any 
> > unexpected reason, related or unrelated to smem, while holding the tcsr 
> > mutex. I want to ensure that all resources that a remoteproc might be 
> > using are released as part of remoteproc stop.
> > 
> > The SMEM driver manages the lock/unlock operations on the tcsr mutex 
> > from the Linux CPU's perspective. This case is for cleaning up from the 
> > remote side's perspective.
> > 
> > In this case it's the hwspinlock used to synchronize SMEM, but it's 
> > conceivable that firmware running on the remoteproc has additional locks 
> > that need to be busted in order for the system to continue executing 
> > until the firmware is reinitialized.
> > 
> > We did consider tying this to the SMEM instance, but the entitiy 
> > relating to firmware is the remoteproc instance.
> 
> I still do not understand why you have to add hwlock to remoteproc, even
> though it is not directly used. Your driver problem looks like lack of
> proper driver architecture - you want to control the locks not from the
> layer took the lock, but one layer up. Sorry, no, fix the driver
> architecture.
> 

No, it is the firmware's reference to the lock that is represented in
the remoteproc node, while SMEM deals with Linux's reference to the lock.

This reference would be used to release the lock - on behalf of the
firmware - in the event that the firmware held it when it
stopped/crashed.

Regards,
Bjorn