linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bjorn Andersson <andersson@kernel.org>
To: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
Cc: Andy Gross <agross@kernel.org>,
	Konrad Dybcio <konrad.dybcio@somainline.org>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Trilok Soni <quic_tsoni@quicinc.com>,
	Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>,
	Rajendra Nayak <quic_rjendra@quicinc.com>,
	Elliot Berman <quic_eberman@quicinc.com>,
	Guru Das Srinagesh <quic_gurus@quicinc.com>
Subject: Re: [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic
Date: Wed, 28 Dec 2022 10:20:40 -0600	[thread overview]
Message-ID: <20221228162040.m3ucsyau3s55rkfn@builder.lan> (raw)
In-Reply-To: <842a6b6307d26874959d29f2065aad544ff0b86c.1662995608.git.quic_gokukris@quicinc.com>

On Mon, Sep 19, 2022 at 09:00:38AM -0700, Gokul krishna Krishnakumar wrote:

Looking back, I don't think I gave you proper feedback on this feature.
Sorry about that Gokul.

> Subdevice notifications after a remoteproc has crashed are useful to any
> clients that might want to preserve data pertaining to the driver after the
> remoteproc crashed. Sending subdevice notifications before triggering a
> kernel panic gives these drivers the time to do collect this information.

The elephant in the room here is the call to panic(), this deserves more
attention in the commit messages.

> 
> Change-Id: Id6e55fb038b70f54ff5854d2adff72b74b6a9570

Please remember to remove your Gerrit Change-Id when posting to the
list.

> Signed-off-by: Gokul krishna Krishnakumar <quic_gokukris@quicinc.com>
> ---
>  drivers/remoteproc/qcom_q6v5.c | 31 +++++++++++++++++++++++++++++++
>  drivers/remoteproc/qcom_q6v5.h |  2 ++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
> index 497acfb..89f5384 100644
> --- a/drivers/remoteproc/qcom_q6v5.c
> +++ b/drivers/remoteproc/qcom_q6v5.c
> @@ -15,6 +15,7 @@
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
>  #include <linux/remoteproc.h>
> +#include <linux/delay.h>
>  #include "qcom_common.h"
>  #include "qcom_q6v5.h"
>  
> @@ -94,6 +95,29 @@ int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5)
>  }
>  EXPORT_SYMBOL_GPL(qcom_q6v5_unprepare);
>  
> +static void qcom_q6v5_crash_handler_work(struct work_struct *work)
> +{
> +	struct qcom_q6v5 *q6v5 = container_of(work, struct qcom_q6v5, crash_handler);
> +	struct rproc *rproc = q6v5->rproc;
> +	struct rproc_subdev *subdev;
> +
> +	mutex_lock(&rproc->lock);
> +
> +	list_for_each_entry_reverse(subdev, &rproc->subdevs, node) {
> +		if (subdev->stop)
> +			subdev->stop(subdev, true);
> +	}
> +
> +	mutex_unlock(&rproc->lock);
> +
> +	/*
> +	 * Temporary workaround until ramdump userspace application calls
> +	 * sync() and fclose() on attempting the dump.
> +	 */

I'm not able to see how this would happen, you only schedule_work() if
rproc->recovery_disabled and the coredump generation will only happen if
!rproc->recovery_disabled.

Also, the reason I can see for invoking panic() here would be to gather
a full system coredump to do post mortem analysis of all systems
involved. So why would you capture a coredump as well? (This needs to be
documented clearly...)

> +	msleep(100);
> +	panic("Panicking, remoteproc %s crashed\n", q6v5->rproc->name);

As you might know, calling panic() is frowned upon. But I can see the
benefit of being able to do full system post mortem analysis.

I do however think that your patch indicates a shortcoming (to you) in
the remoteproc_core's recovery logic. So please fix that shortcoming,
rather than circumventing it in your driver.

I.e. make it possible to get rproc_crash_handler_work() behave like you
want it to, with a good motivation to why you want that.

Regards,
Bjorn

> +}
> +
>  static irqreturn_t q6v5_wdog_interrupt(int irq, void *data)
>  {
>  	struct qcom_q6v5 *q6v5 = data;
> @@ -113,6 +137,9 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data)
>  		dev_err(q6v5->dev, "watchdog without message\n");
>  
>  	q6v5->running = false;
> +	if (q6v5->rproc->recovery_disabled)
> +		schedule_work(&q6v5->crash_handler);
> +
>  	rproc_report_crash(q6v5->rproc, RPROC_WATCHDOG);
>  
>  	return IRQ_HANDLED;
> @@ -134,6 +161,9 @@ static irqreturn_t q6v5_fatal_interrupt(int irq, void *data)
>  		dev_err(q6v5->dev, "fatal error without message\n");
>  
>  	q6v5->running = false;
> +	if (q6v5->rproc->recovery_disabled)
> +		schedule_work(&q6v5->crash_handler);
> +
>  	rproc_report_crash(q6v5->rproc, RPROC_FATAL_ERROR);
>  
>  	return IRQ_HANDLED;
> @@ -354,6 +384,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
>  	if (IS_ERR(q6v5->path))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(q6v5->path),
>  				     "failed to acquire interconnect path\n");
> +	INIT_WORK(&q6v5->crash_handler, qcom_q6v5_crash_handler_work);
>  
>  	return 0;
>  }
> diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
> index 5a859c4..b1654be 100644
> --- a/drivers/remoteproc/qcom_q6v5.h
> +++ b/drivers/remoteproc/qcom_q6v5.h
> @@ -29,6 +29,8 @@ struct qcom_q6v5 {
>  	int handover_irq;
>  	int stop_irq;
>  
> +	struct work_struct crash_handler;
> +
>  	bool handover_issued;
>  
>  	struct completion start_done;
> -- 
> 2.7.4
> 

  parent reply	other threads:[~2022-12-28 16:25 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-13 18:35 [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar
2022-09-13 18:35 ` [PATCH v1 1/3] remoteproc: qcom: q6v5: Send subdevice notifications before panic Gokul krishna Krishnakumar
2022-09-19 16:00   ` Gokul krishna Krishnakumar
2022-09-23  7:03   ` Peng Fan
2022-12-28 16:20   ` Bjorn Andersson [this message]
2022-09-13 18:35 ` [PATCH v1 2/3] remoteproc: qcom: q6v5: Do not report crash if SSR is disabled Gokul krishna Krishnakumar
2022-09-19 16:00   ` Gokul krishna Krishnakumar
2022-09-23  7:05   ` Peng Fan
2022-12-28 16:22   ` Bjorn Andersson
2022-09-13 18:35 ` [PATCH v1 3/3] remoteproc: qcom: q6v5: Avoid setting smem bit in case of crash shutdown Gokul krishna Krishnakumar
2022-09-19 16:00   ` Gokul krishna Krishnakumar
2022-09-19 16:00 ` [PATCH v1 0/3] Handle coprocessor crash Gokul krishna Krishnakumar
2022-12-28 18:18 ` (subset) " 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=20221228162040.m3ucsyau3s55rkfn@builder.lan \
    --to=andersson@kernel.org \
    --cc=agross@kernel.org \
    --cc=konrad.dybcio@somainline.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=quic_eberman@quicinc.com \
    --cc=quic_gokukris@quicinc.com \
    --cc=quic_gurus@quicinc.com \
    --cc=quic_rjendra@quicinc.com \
    --cc=quic_satyap@quicinc.com \
    --cc=quic_tsoni@quicinc.com \
    /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 NNTP newsgroup(s).