From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752525AbdLEGqX (ORCPT ); Tue, 5 Dec 2017 01:46:23 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:34354 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751416AbdLEGqS (ORCPT ); Tue, 5 Dec 2017 01:46:18 -0500 X-Google-Smtp-Source: AGs4zMYbE9vJwkOirczVZuKIPyf6J3YR5MoJSrv9t7EMNFyWrdyt691+x2TfZ88ZX36yqMtpx3u7fQ== Date: Mon, 4 Dec 2017 22:46:11 -0800 From: Bjorn Andersson To: Arnaud Pouliquen Cc: Andy Gross , Ohad Ben-Cohen , Arun Kumar Neelakantam , Chris Lew , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org, linux-remoteproc@vger.kernel.org Subject: Re: [PATCH v4 3/5] remoteproc: Pass type of shutdown to subdev remove Message-ID: <20171205064611.GM28761@minitux> References: <20171130011644.9421-1-bjorn.andersson@linaro.org> <20171130011644.9421-4-bjorn.andersson@linaro.org> <6d6d01bb-f8f4-3380-c9ee-3d14c707bdf0@st.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6d6d01bb-f8f4-3380-c9ee-3d14c707bdf0@st.com> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 01 Dec 06:50 PST 2017, Arnaud Pouliquen wrote: > hello Bjorn, > > Sorry for these late remarks/questions > No worries, I'm happy to see you reading the patch! > > On 11/30/2017 02:16 AM, Bjorn Andersson wrote: [..] > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c [..] > > @@ -785,17 +785,17 @@ static int rproc_probe_subdevices(struct rproc *rproc) > > > > unroll_registration: > > list_for_each_entry_continue_reverse(subdev, &rproc->subdevs, node) > > - subdev->remove(subdev); > > + subdev->remove(subdev, false); > Why do you need to do a non graceful remove in this case? This could > lead to side effect like memory leakage... > Regardless of this being true or false resources should always be reclaimed. The reason for introducing this is that the modem in the Qualcomm platforms implements persistent storage and it's preferred to tell it to flush the latest data to the storage server (on the Linux side) before pulling the plug. But in the case of a firmware crash this mechanism will not be operational and there's no point in attempting this "graceful shutdown". [..] > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > index 44e630eb3d94..20a9467744ea 100644 > > --- a/include/linux/remoteproc.h > > +++ b/include/linux/remoteproc.h > > @@ -456,7 +456,7 @@ struct rproc_subdev { > > struct list_head node; > > > > int (*probe)(struct rproc_subdev *subdev); > > - void (*remove)(struct rproc_subdev *subdev); > > + void (*remove)(struct rproc_subdev *subdev, bool graceful); > What about adding a new ops instead of a parameter, like a recovery > callback? > I think that for symmetry purposes it should be probe/remove in both code paths. A possible alternative to the proposal would be to introduce an operation "request_shutdown()" the would be called in the proposed graceful code path. However, in the Qualcomm SMD and GLINK (conceptually equivalent to virtio-rpmsg) it is possible to open and close communication channels and it's conceivable to see that the graceful case would close all channels cleanly while the non-graceful case would just remove the rpmsg devices (and leave the channel states/memory as is). In this case a "request_shutdown()" would complicate things, compared to the boolean. Regards, Bjorn