From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Chris Lew <quic_clew@quicinc.com>,
Bjorn Andersson <andersson@kernel.org>,
Baolin Wang <baolin.wang@linux.alibaba.com>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
Waiman Long <longman@redhat.com>,
Boqun Feng <boqun.feng@gmail.com>,
Jonathan Corbet <corbet@lwn.net>,
Mathieu Poirier <mathieu.poirier@linaro.org>,
Rob Herring <robh@kernel.org>,
"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
<linux-remoteproc@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
<linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>
Subject: Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks
Date: Wed, 22 May 2024 10:50:33 -0700 [thread overview]
Message-ID: <Zk4wab/NZOOZ3hA6@hu-bjorande-lv.qualcomm.com> (raw)
In-Reply-To: <92dcd555-69b1-4111-92dd-debe5107d526@kernel.org>
On Wed, May 22, 2024 at 09:26:00AM +0200, Krzysztof Kozlowski wrote:
> On 21/05/2024 21:17, Bjorn Andersson wrote:
> >>>
> >>> 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.
>
> I understood, but the remoteproc driver did not acquire the hardware
> lock. It was taken by smem, if I got it correctly, so you should poke
> smem to bust the spinlock.
>
The remoteproc instance is the closest representation of the entity that
took the lock (i.e. the firmware). SMEM here is just another consumer of
the same lock.
> The hwlock is not a property of remote proc, because remote proc does
> not care, right? Other device cares... and now for every smem user you
> will add new binding property?
>
Right, the issue seen relates to SMEM, because the remote processor (not
the remoteproc driver) took the lock.
> No, you are adding a binding based on your driver solution.
Similar to how hwspinlocks are used in other platforms (e.g. TI) the
firmware could take multiple locks, e.g. to synchronize access to other
shared memory mechanism (i.e. not SMEM). While I am not aware of such
use case today, my expectation was that in such case we just list all
the hwlocks related to the firmware and bust those from the remoteproc
instance.
Having to export APIs from each one of such drivers and make the
remoteproc identify the relevant instances and call those APIs does
indeed seem inconvenient.
SMEM is special here because it's singleton, but this would not
necessarily be true for other cases.
Regards,
Bjorn
next prev parent reply other threads:[~2024-05-22 17:50 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-16 22:58 [PATCH 0/7] Add support for hwspinlock bust Chris Lew
2024-05-16 22:58 ` [PATCH 1/7] hwspinlock: Introduce refcount Chris Lew
2024-05-17 8:58 ` Bryan O'Donoghue
2024-05-17 18:32 ` Chris Lew
2024-05-16 22:58 ` [PATCH 2/7] hwspinlock: Enable hwspinlock sharing Chris Lew
2024-05-16 22:58 ` [PATCH 3/7] hwspinlock: Introduce hwspin_lock_bust() Chris Lew
2024-05-17 8:07 ` Bryan O'Donoghue
2024-05-17 8:47 ` Bryan O'Donoghue
2024-05-16 22:58 ` [PATCH 4/7] hwspinlock: qcom: implement bust operation Chris Lew
2024-05-16 22:58 ` [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks Chris Lew
2024-05-19 17:36 ` Krzysztof Kozlowski
2024-05-21 4:08 ` Chris Lew
2024-05-21 7:36 ` Krzysztof Kozlowski
2024-05-21 19:17 ` Bjorn Andersson
2024-05-22 7:26 ` Krzysztof Kozlowski
2024-05-22 17:50 ` Bjorn Andersson [this message]
2024-05-23 6:15 ` Krzysztof Kozlowski
2024-05-24 19:23 ` Bjorn Andersson
2024-05-25 16:45 ` Krzysztof Kozlowski
2024-05-16 22:58 ` [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop Chris Lew
2024-05-17 7:19 ` Mukesh Ojha
2024-05-17 7:21 ` Mukesh Ojha
2024-05-17 17:25 ` Chris Lew
2024-05-17 9:08 ` Bryan O'Donoghue
2024-05-17 17:20 ` Chris Lew
2024-05-21 17:38 ` Konrad Dybcio
2024-05-21 21:08 ` Chris Lew
2024-05-16 22:58 ` [PATCH 7/7] arm64: dts: qcom: sm8650: Add hwlock to remoteproc Chris Lew
2024-05-22 7:27 ` Krzysztof Kozlowski
2024-05-22 17:51 ` Bjorn Andersson
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=Zk4wab/NZOOZ3hA6@hu-bjorande-lv.qualcomm.com \
--to=quic_bjorande@quicinc.com \
--cc=andersson@kernel.org \
--cc=baolin.wang@linux.alibaba.com \
--cc=boqun.feng@gmail.com \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzk+dt@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=longman@redhat.com \
--cc=manivannan.sadhasivam@linaro.org \
--cc=mathieu.poirier@linaro.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=quic_clew@quicinc.com \
--cc=robh@kernel.org \
--cc=will@kernel.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).