Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Peng Fan <peng.fan@nxp.com>
To:     Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>,
        "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
        "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH V2] remoteproc: support self recovery after rproc crash
Thread-Topic: [PATCH V2] remoteproc: support self recovery after rproc crash
Thread-Index: AQHYEpIEHEs++qV8cUuKksCGalsIFKyTf94AgA9q+oCAAMec4A==
Date:   Fri, 25 Feb 2022 02:10:13 +0000
Message-ID: <DU0PR04MB9417ED8CBFEE1C1FD4DFD850883E9@DU0PR04MB9417.eurprd04.prod.outlook.com>
References: <20220126085120.3397450-1-peng.fan@oss.nxp.com>
 <67ddf940-6f87-d8cc-8dc6-29a39a022265@foss.st.com>
 <da6bee63-6a79-2341-e099-d26b53fb557f@foss.st.com>
In-Reply-To: <da6bee63-6a79-2341-e099-d26b53fb557f@foss.st.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?wsrC21QbVxo371FgxYMz8x+zAvrvox5TJUJYY52DaU6b3s4kZQ685rWtYQG+?=
 =?us-ascii?Q?qxXB4NQV3tjHX5KhEfDZDJp77yXrN+hDip8XhX4WHstUfBvgkYcZFtavZARq?=
 =?us-ascii?Q?QeLCVgjG29jY6c+oObttvUngEE+BrQGjQOBnV9hcv9QYypyBMI+6ypGv+SGF?=
 =?us-ascii?Q?25n5cIv6ij2DsSSR56vCxWqW59DjyGODvoUOijRcigmHzwZwt42ySR0pUrE4?=
 =?us-ascii?Q?PwT1wqoVGOOxDI1KXVxmH2RLKaYgrte7/fvwGfzSsF0mOl5L0yvXh0dCahxG?=
 =?us-ascii?Q?2lwJ3zFYNfDv+060jZho33UIPL+4/WC0Z3OzWMl6K72kMp855mBKdMqi+XJH?=
 =?us-ascii?Q?X3xOpDnUvpCNYP7nwMKo9CrGQowp5IsvwXaNNJP/N1idohcEKjHtKqMgrDP3?=
 =?us-ascii?Q?vPpQXR83vLSKW+PsyYZQjr+cDxTPs0t65pzUv+TCWVeUVBdaCsrX4b5nu3F1?=
 =?us-ascii?Q?FBgPrzrlns3RuUrJQoaCNXBzCq7nOAkOdjS78fE7gSkcsZSKryHkIwAH+oKI?=
 =?us-ascii?Q?crwwzAKSILJ7PRsW4aiIVlaJ93+lpo1sWBv1fy+SOXTSlU+Z5iHsmzVx1AY3?=
 =?us-ascii?Q?7s6aW6tcUmv3LaZ17tNS5QuQGNTzQkIGlhmhtLftQBNXUGlUxtJ7RLAMA2xv?=
 =?us-ascii?Q?2BjnZaIzN9M3fsxm/H76Kis3uawBSID7m/A85DTcTfzO3ATsZAsZgNd7/K/9?=
 =?us-ascii?Q?fMVESIPrHrAW5mb78A0dmzY44JhmhcapmEVmeHTMqPwUeF/oqBdBu4QrERp/?=
 =?us-ascii?Q?2351j/0Ku8WpaFy4W51Q/p36NgxZSt+ppfw/CHqAbcVYAgUKE6yjKw8eFYJL?=
 =?us-ascii?Q?OhxxyChrAY7KBbe0ToPn7L7a7s5qjsADjuBB4a7MqtHWDNa9n5ru/MjOdOgQ?=
 =?us-ascii?Q?su2qpisddzoWQDfk8Gje56pTYAvGZ2C/9hkP/Y1CbG2yQ8X4sLcm/5nv0wnl?=
 =?us-ascii?Q?mCUFN8w2mw23CbMOZM62p30/SwZy8wJC1KwxbumeKOeRGKyYoU0EDSJU0zk/?=
 =?us-ascii?Q?LyMyeHzVK1KRipE9UWZCmhq+/nxKEO4U6JlzNyoDyGC+8q8C3u8EZwGDYrSe?=
 =?us-ascii?Q?ZN67f5HGZKzNImobs893IbPBa2BQLEz+/XPG9a1Zsx5Ozh21QqG1lLytAX9i?=
 =?us-ascii?Q?9SBCg3r/9mToUgWjKT44jhiFxju9hRWIseW/pHIsylAv7bUcW/avGR90XYHr?=
 =?us-ascii?Q?qxVxRVsqUz6zwsblMj46Ee3E6+DebyXbnxO5z+blshFJlT5Wxhjm0BEoc42n?=
 =?us-ascii?Q?U8xTn0w4AARLoZmC5Y+nT91SETS0KJHCM1N7Lg3QupCiu8Fk2MQ1WBIJOHCq?=
 =?us-ascii?Q?xTqacsFVhtYYHUEKBsbwn5RurvIBxIwE1l8Pd/5Fn9PnS8I7C1RfWURz1Iha?=
 =?us-ascii?Q?PWvXQqSo2yUuz4ElmYfgfpFr19AXMfRJ4ZcL5pwlqG8Ziy1TU4t9N+fTm4w7?=
 =?us-ascii?Q?5SuqoSdGORVCMJYAkZyeh4/aYQSnuWHRidbEK1ZwmT9YDTPP9tmk7cQrZeNX?=
 =?us-ascii?Q?RIYzjvG5T+c4TVfY5wdi4y7qYwEJ+pvIFdC+sYK+57Aq7gij0ny0qilguZ6s?=
 =?us-ascii?Q?xRmvYP2/OfLWVOuXtkFFWcMbq6uxdP300NCSFibMm7uz4g6eb3Piw+JE/Ubr?=
 =?us-ascii?Q?+UinSRQTo0yq9w1GTrMg7es=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DU0PR04MB9417.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1a8a5916-3b58-4fee-d99a-08d9f803f55c
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Feb 2022 02:10:13.3791
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: iGCuU3IM8TG6WobIJdOFuquSV8bd9xM02La+OQY8YayF3IF3fnkArhjVa/VJ0SPIy8fbQvzh60k+bCzK1Ntf3w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB8PR04MB6492
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> Subject: Re: [PATCH V2] remoteproc: support self recovery after rproc cra=
sh
>=20
> Hi Peng,
>=20
> On 2/14/22 19:41, Arnaud POULIQUEN wrote:
> > Hi Peng,
> >
> > On 1/26/22 09:51, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >>
> >> Current logic only support main processor to stop/start the remote
> >> processor after rproc crash. However to SoC, such as i.MX8QM/QXP, the
> >> remote processor could do self recovery after crash and trigger
> >> watchdog reboot. It does not need main processor to load image,
> >> stop/start M4 core.
> >
> >
> > On stm32mp1 platform the remote processor watchdog generates an early
> > interrupt that could be used to detach and reattach before the reset of=
 the
> remote processor.
> > I need to test race condition,but I suppose that this should works if
> > the resource table is not reinitialized by the remote processor firmwar=
e.
> >
> > Another option for the stm32mp1 is that remoteproc manages the reset
> > of the remote processor.
> > For instance this allows to save a core-dump before manually resetting
> > the remote processor.
> > But looks like this use case can be handled later, as mentioned below.
> >
> >>
> >> This patch add a new flag to indicate whether the SoC has self
> >> recovery capability. And introduce two functions:
> >> rproc_self_recovery, rproc_assisted_recovery for the two cases.
> >> Assisted recovery is as before, let main processor to help recovery,
> >> while self recovery is recover itself withou help. To self recovery,
> >> we only do detach and attach.
> >
> >
> >>
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >>
> >> V2:
> >>  Nothing change in V2.
> >>  Only move this patch out from
> >>
> >> https://eur01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fpa=
t
> >>
> chwork.kernel.org%2Fproject%2Flinux-remoteproc%2Flist%2F%3Fseries%3D
> 6
> >>
> 04364&amp;data=3D04%7C01%7Cpeng.fan%40nxp.com%7C3d617c5ddb8c42b
> 315f808d
> >>
> 9f79f2ac7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378130
> 8526949
> >>
> 0105%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLCJB
> >>
> TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=3DVKKrqOnFz%2BoXjr%2
> FGMKpm4
> >> yyqmRfApeYY0l2V8A0yy4Y%3D&amp;reserved=3D0
> >>
> >>  drivers/remoteproc/remoteproc_core.c | 66
> ++++++++++++++++++++--------
> >>  include/linux/remoteproc.h           |  2 +
> >>  2 files changed, 49 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index 69f51acf235e..4bd5544dab8f 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1887,6 +1887,49 @@ static int __rproc_detach(struct rproc *rproc)
> >>  	return 0;
> >>  }
> >>
> >> +static int rproc_self_recovery(struct rproc *rproc) {
> >> +	int ret;
> >> +
> >> +	mutex_unlock(&rproc->lock);
> >> +	ret =3D rproc_detach(rproc);
> >> +	mutex_lock(&rproc->lock);
> >> +	if (ret)
> >> +		return ret;
> >
> > Here we would want to perform a core dump and manually reset the
> > co-processor.
> > I suppose that a new rproc ops could be called here in a next step.
> >
> >> +
> >> +	if (atomic_inc_return(&rproc->power) > 1)
> >> +		return 0;
> >
> > Do you identify a use case that needs to test rproc->power to skip the
> > attach?
> > If yes could you add a comment to describe it?
> >
> >> +	return rproc_attach(rproc);
> >> +}
> >> +
> >> +static int rproc_assisted_recovery(struct rproc *rproc) {
> >> +	const struct firmware *firmware_p;
> >> +	struct device *dev =3D &rproc->dev;
> >> +	int ret;
> >> +
> >> +	ret =3D rproc_stop(rproc, true);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* generate coredump */
> >> +	rproc->ops->coredump(rproc);
> >> +
> >> +	/* load firmware */
> >> +	ret =3D request_firmware(&firmware_p, rproc->firmware, dev);
> >> +	if (ret < 0) {
> >> +		dev_err(dev, "request_firmware failed: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	/* boot the remote processor up again */
> >> +	ret =3D rproc_start(rproc, firmware_p);
> >> +
> >> +	release_firmware(firmware_p);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  /**
> >>   * rproc_trigger_recovery() - recover a remoteproc
> >>   * @rproc: the remote processor
> >> @@ -1901,7 +1944,6 @@ static int __rproc_detach(struct rproc *rproc)
> >>   */
> >>  int rproc_trigger_recovery(struct rproc *rproc)  {
> >> -	const struct firmware *firmware_p;
> >>  	struct device *dev =3D &rproc->dev;
> >>  	int ret;
> >>
> >> @@ -1915,24 +1957,10 @@ int rproc_trigger_recovery(struct rproc
> >> *rproc)
> >>
> >>  	dev_err(dev, "recovering %s\n", rproc->name);
> >>
> >> -	ret =3D rproc_stop(rproc, true);
> >> -	if (ret)
> >> -		goto unlock_mutex;
> >> -
> >> -	/* generate coredump */
> >> -	rproc->ops->coredump(rproc);
> >> -
> >> -	/* load firmware */
> >> -	ret =3D request_firmware(&firmware_p, rproc->firmware, dev);
> >> -	if (ret < 0) {
> >> -		dev_err(dev, "request_firmware failed: %d\n", ret);
> >> -		goto unlock_mutex;
> >> -	}
> >> -
> >> -	/* boot the remote processor up again */
> >> -	ret =3D rproc_start(rproc, firmware_p);
> >> -
> >> -	release_firmware(firmware_p);
> >> +	if (rproc->self_recovery)
> >> +		ret =3D rproc_self_recovery(rproc);
> >
> > If some platforms have to manually reset the remote processor (without
> > reloading the firmware) the name could not be relevant...
> >
> > Following comments are only suggestions that needs to be commented by
> > maintainers
> >
> > What about rproc_attach_recovery ?
> >
> >> +	else
> >> +		ret =3D rproc_assisted_recovery(rproc);
> >
> > and rproc_firmware_recovery ?
> >
> >
> >>
> >>  unlock_mutex:
> >>  	mutex_unlock(&rproc->lock);
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index e0600e1e5c17..b32ef46f8aa4 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -529,6 +529,7 @@ struct rproc_dump_segment {
> >>   * @elf_machine: firmware ELF machine
> >>   * @cdev: character device of the rproc
> >>   * @cdev_put_on_release: flag to indicate if remoteproc should be
> >> shutdown on @char_dev release
> >> + * @self_recovery: flag to indicate if remoteproc support self
> >> + recovery
> >>   */
> >>  struct rproc {
> >>  	struct list_head node;
> >> @@ -568,6 +569,7 @@ struct rproc {
> >>  	u16 elf_machine;
> >>  	struct cdev cdev;
> >>  	bool cdev_put_on_release;
> >> +	bool self_recovery;
> >
> > This bool seems needed because we have lost the previous state before
> crash.
> > I wonder if a new rproc->state such as RPROC_REBOOT could avoid this
> boolean.
> >
> >
> > I will try to test you patch on stm32mp1 next week
>=20
> I performed few tests on the stm32mp1 with your patch.
> Thanks to the resetting of the resource tables on detachment, this works
> quite well.

Thanks very much for you testing this patch. I'll try to address your
previous comments and send out v3.

Thanks,
Peng.

>=20
> Regards,
> Arnaud
>=20
> >
> > Regards,
> > Arnaud
> >
> >>  };
> >>
> >>  /**