| Message ID | 1582167465-2549-7-git-send-email-sidgup@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: qcom: Add callbacks for remoteproc events | expand |
On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: > The SSR subdevice only adds callback for the unprepare event. Add callbacks > for unprepare, start and prepare events. The client driver for a particular > remoteproc might be interested in knowing the status of the remoteproc > while undergoing SSR, not just when the remoteproc has finished shutting > down. > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > --- > drivers/remoteproc/qcom_common.c | 39 +++++++++++++++++++++++++++++++++++---- > include/linux/remoteproc.h | 15 +++++++++++++++ > 2 files changed, 50 insertions(+), 4 deletions(-) > > diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c > index 6714f27..6f04a5b 100644 > --- a/drivers/remoteproc/qcom_common.c > +++ b/drivers/remoteproc/qcom_common.c > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); > * > * Returns pointer to srcu notifier head on success, ERR_PTR on failure. > * > - * This registers the @notify function as handler for restart notifications. As > - * remote processors are stopped this function will be called, with the rproc > - * pointer passed as a parameter. > + * This registers the @notify function as handler for powerup/shutdown > + * notifications. This function will be invoked inside the callbacks registered > + * for the ssr subdevice, with the rproc pointer passed as a parameter. > */ > void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb) > { > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) > } > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) > +{ > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + > + srcu_notifier_call_chain(ssr->rproc_notif_list, > + RPROC_BEFORE_POWERUP, (void *)ssr->name); > + return 0; > +} > + > +static int ssr_notify_start(struct rproc_subdev *subdev) > +{ > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + > + srcu_notifier_call_chain(ssr->rproc_notif_list, > + RPROC_AFTER_POWERUP, (void *)ssr->name); > + return 0; > +} > + > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed) > +{ > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > + > + srcu_notifier_call_chain(ssr->rproc_notif_list, > + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); > +} > + > + > static void ssr_notify_unprepare(struct rproc_subdev *subdev) > { > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > > - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name); > + srcu_notifier_call_chain(ssr->rproc_notif_list, > + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); > } > > /** > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, > { > ssr->name = ssr_name; > ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); > + ssr->subdev.prepare = ssr_notify_prepare; > + ssr->subdev.start = ssr_notify_start; > + ssr->subdev.stop = ssr_notify_stop; Now that I have a better understanding of what this patchset is doing, I realise my comments in patch 04 won't work. To differentiate the subdevs of an rproc I suggest to wrap them in a generic structure with a type and an enum. That way you can differenciate between subdevices without having to add to the core. That being said, I don't understand what patches 5 and 6 are doing... Registering with the global ssr_notifiers allowed to gracefully shutdown all the MCUs in the system when one of them would go down. But now that we are using the notifier on a per MCU, I really don't see why each subdev couldn't implement the right prepare/start/stop functions. Am I missing something here? > ssr->subdev.unprepare = ssr_notify_unprepare; > ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), > GFP_KERNEL); > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > index e2f60cc..4be4478 100644 > --- a/include/linux/remoteproc.h > +++ b/include/linux/remoteproc.h > @@ -449,6 +449,21 @@ struct rproc_dump_segment { > }; > > /** > + * enum rproc_notif_type - Different stages of remoteproc notifications > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc > + * @RPROC_AFTER_POWERUP: start stage of remoteproc > + */ > +enum rproc_notif_type { > + RPROC_BEFORE_SHUTDOWN, > + RPROC_AFTER_SHUTDOWN, > + RPROC_BEFORE_POWERUP, > + RPROC_AFTER_POWERUP, > + RPROC_MAX > +}; > + > +/** > * struct rproc - represents a physical remote processor device > * @node: list node of this rproc object > * @domain: iommu domain > -- > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2020-02-27 13:59, Mathieu Poirier wrote: > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: >> The SSR subdevice only adds callback for the unprepare event. Add >> callbacks >> for unprepare, start and prepare events. The client driver for a >> particular >> remoteproc might be interested in knowing the status of the remoteproc >> while undergoing SSR, not just when the remoteproc has finished >> shutting >> down. >> >> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> --- >> drivers/remoteproc/qcom_common.c | 39 >> +++++++++++++++++++++++++++++++++++---- >> include/linux/remoteproc.h | 15 +++++++++++++++ >> 2 files changed, 50 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_common.c >> b/drivers/remoteproc/qcom_common.c >> index 6714f27..6f04a5b 100644 >> --- a/drivers/remoteproc/qcom_common.c >> +++ b/drivers/remoteproc/qcom_common.c >> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); >> * >> * Returns pointer to srcu notifier head on success, ERR_PTR on >> failure. >> * >> - * This registers the @notify function as handler for restart >> notifications. As >> - * remote processors are stopped this function will be called, with >> the rproc >> - * pointer passed as a parameter. >> + * This registers the @notify function as handler for >> powerup/shutdown >> + * notifications. This function will be invoked inside the callbacks >> registered >> + * for the ssr subdevice, with the rproc pointer passed as a >> parameter. >> */ >> void *qcom_register_ssr_notifier(struct rproc *rproc, struct >> notifier_block *nb) >> { >> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, >> struct notifier_block *nb) >> } >> EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >> >> +static int ssr_notify_prepare(struct rproc_subdev *subdev) >> +{ >> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> + >> + srcu_notifier_call_chain(ssr->rproc_notif_list, >> + RPROC_BEFORE_POWERUP, (void *)ssr->name); >> + return 0; >> +} >> + >> +static int ssr_notify_start(struct rproc_subdev *subdev) >> +{ >> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> + >> + srcu_notifier_call_chain(ssr->rproc_notif_list, >> + RPROC_AFTER_POWERUP, (void *)ssr->name); >> + return 0; >> +} >> + >> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool >> crashed) >> +{ >> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> + >> + srcu_notifier_call_chain(ssr->rproc_notif_list, >> + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); >> +} >> + >> + >> static void ssr_notify_unprepare(struct rproc_subdev *subdev) >> { >> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> >> - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void >> *)ssr->name); >> + srcu_notifier_call_chain(ssr->rproc_notif_list, >> + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); >> } >> >> /** >> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, >> struct qcom_rproc_ssr *ssr, >> { >> ssr->name = ssr_name; >> ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); >> + ssr->subdev.prepare = ssr_notify_prepare; >> + ssr->subdev.start = ssr_notify_start; >> + ssr->subdev.stop = ssr_notify_stop; > > Now that I have a better understanding of what this patchset is doing, > I realise > my comments in patch 04 won't work. To differentiate the subdevs of an > rproc I > suggest to wrap them in a generic structure with a type and an enum. > That way > you can differenciate between subdevices without having to add to the > core. Ok. I can try that. > > That being said, I don't understand what patches 5 and 6 are doing... > Registering with the global ssr_notifiers allowed to gracefully > shutdown all the > MCUs in the system when one of them would go down. But now that we are > using > the notifier on a per MCU, I really don't see why each subdev couldn't > implement > the right prepare/start/stop functions. > > Am I missing something here? We only want kernel clients to be notified when the Remoteproc they are interested in changes state. For e.g. audio kernel driver should be notified when audio processor goes down but it does not care about any other remoteproc. If you are suggesting that these kernel clients be added as subdevices then we will end up having many subdevices registered to each remoteproc. So we implemented a notifier chain per Remoteproc. This keeps the SSR notifications as the subdevice per remoteproc, and all interested clients can register to it. > > >> ssr->subdev.unprepare = ssr_notify_unprepare; >> ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), >> GFP_KERNEL); >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> index e2f60cc..4be4478 100644 >> --- a/include/linux/remoteproc.h >> +++ b/include/linux/remoteproc.h >> @@ -449,6 +449,21 @@ struct rproc_dump_segment { >> }; >> >> /** >> + * enum rproc_notif_type - Different stages of remoteproc >> notifications >> + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc >> + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc >> + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc >> + * @RPROC_AFTER_POWERUP: start stage of remoteproc >> + */ >> +enum rproc_notif_type { >> + RPROC_BEFORE_SHUTDOWN, >> + RPROC_AFTER_SHUTDOWN, >> + RPROC_BEFORE_POWERUP, >> + RPROC_AFTER_POWERUP, >> + RPROC_MAX >> +}; >> + >> +/** >> * struct rproc - represents a physical remote processor device >> * @node: list node of this rproc object >> * @domain: iommu domain >> -- >> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> a Linux Foundation Collaborative Project >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org wrote: > On 2020-02-27 13:59, Mathieu Poirier wrote: > > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: > > > The SSR subdevice only adds callback for the unprepare event. Add > > > callbacks > > > for unprepare, start and prepare events. The client driver for a > > > particular > > > remoteproc might be interested in knowing the status of the remoteproc > > > while undergoing SSR, not just when the remoteproc has finished > > > shutting > > > down. > > > > > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > > > --- > > > drivers/remoteproc/qcom_common.c | 39 > > > +++++++++++++++++++++++++++++++++++---- > > > include/linux/remoteproc.h | 15 +++++++++++++++ > > > 2 files changed, 50 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/remoteproc/qcom_common.c > > > b/drivers/remoteproc/qcom_common.c > > > index 6714f27..6f04a5b 100644 > > > --- a/drivers/remoteproc/qcom_common.c > > > +++ b/drivers/remoteproc/qcom_common.c > > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); > > > * > > > * Returns pointer to srcu notifier head on success, ERR_PTR on > > > failure. > > > * > > > - * This registers the @notify function as handler for restart > > > notifications. As > > > - * remote processors are stopped this function will be called, with > > > the rproc > > > - * pointer passed as a parameter. > > > + * This registers the @notify function as handler for > > > powerup/shutdown > > > + * notifications. This function will be invoked inside the > > > callbacks registered > > > + * for the ssr subdevice, with the rproc pointer passed as a > > > parameter. > > > */ > > > void *qcom_register_ssr_notifier(struct rproc *rproc, struct > > > notifier_block *nb) > > > { > > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, > > > struct notifier_block *nb) > > > } > > > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > > > > > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) > > > +{ > > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > > > + > > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > > > + RPROC_BEFORE_POWERUP, (void *)ssr->name); > > > + return 0; > > > +} > > > + > > > +static int ssr_notify_start(struct rproc_subdev *subdev) > > > +{ > > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > > > + > > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > > > + RPROC_AFTER_POWERUP, (void *)ssr->name); > > > + return 0; > > > +} > > > + > > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool > > > crashed) > > > +{ > > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > > > + > > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > > > + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); > > > +} > > > + > > > + > > > static void ssr_notify_unprepare(struct rproc_subdev *subdev) > > > { > > > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > > > > > > - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void > > > *)ssr->name); > > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > > > + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); > > > } > > > > > > /** > > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, > > > struct qcom_rproc_ssr *ssr, > > > { > > > ssr->name = ssr_name; > > > ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); > > > + ssr->subdev.prepare = ssr_notify_prepare; > > > + ssr->subdev.start = ssr_notify_start; > > > + ssr->subdev.stop = ssr_notify_stop; > > > > Now that I have a better understanding of what this patchset is doing, I > > realise > > my comments in patch 04 won't work. To differentiate the subdevs of an > > rproc I > > suggest to wrap them in a generic structure with a type and an enum. > > That way > > you can differenciate between subdevices without having to add to the > > core. > Ok. I can try that. > > > > That being said, I don't understand what patches 5 and 6 are doing... > > Registering with the global ssr_notifiers allowed to gracefully shutdown > > all the > > MCUs in the system when one of them would go down. But now that we are > > using > > the notifier on a per MCU, I really don't see why each subdev couldn't > > implement > > the right prepare/start/stop functions. > > > > Am I missing something here? > We only want kernel clients to be notified when the Remoteproc they are > interested > in changes state. For e.g. audio kernel driver should be notified when audio > processor goes down but it does not care about any other remoteproc. > If you are suggesting that these kernel clients be added as subdevices then > we will end up having many subdevices registered to each remoteproc. So we > implemented a notifier chain per Remoteproc. This keeps the SSR > notifications as > the subdevice per remoteproc, and all interested clients can register to it. It seems like I am missing information... Your are referring to "kernel clients" and as such I must assume some drivers that are not part of the remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). I must also assume these drivers (or that functionality) are not yet upsream because all I can see calling qcom_register_ssr_notifier() is qcom_glink_ssr_probe(). Speaking of which, what is the role of the qcom_glink_ssr_driver? Is the glink device that driver is handling the same as the glink device registed in adsp_probe() and q6v5_probe()? > > > > > > > ssr->subdev.unprepare = ssr_notify_unprepare; > > > ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), > > > GFP_KERNEL); > > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > > > index e2f60cc..4be4478 100644 > > > --- a/include/linux/remoteproc.h > > > +++ b/include/linux/remoteproc.h > > > @@ -449,6 +449,21 @@ struct rproc_dump_segment { > > > }; > > > > > > /** > > > + * enum rproc_notif_type - Different stages of remoteproc > > > notifications > > > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc > > > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc > > > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc > > > + * @RPROC_AFTER_POWERUP: start stage of remoteproc > > > + */ > > > +enum rproc_notif_type { > > > + RPROC_BEFORE_SHUTDOWN, > > > + RPROC_AFTER_SHUTDOWN, > > > + RPROC_BEFORE_POWERUP, > > > + RPROC_AFTER_POWERUP, > > > + RPROC_MAX > > > +}; > > > + > > > +/** > > > * struct rproc - represents a physical remote processor device > > > * @node: list node of this rproc object > > > * @domain: iommu domain > > > -- > > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > a Linux Foundation Collaborative Project > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2020-02-28 10:38, Mathieu Poirier wrote: > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org > wrote: >> On 2020-02-27 13:59, Mathieu Poirier wrote: >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: >> > > The SSR subdevice only adds callback for the unprepare event. Add >> > > callbacks >> > > for unprepare, start and prepare events. The client driver for a >> > > particular >> > > remoteproc might be interested in knowing the status of the remoteproc >> > > while undergoing SSR, not just when the remoteproc has finished >> > > shutting >> > > down. >> > > >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> > > --- >> > > drivers/remoteproc/qcom_common.c | 39 >> > > +++++++++++++++++++++++++++++++++++---- >> > > include/linux/remoteproc.h | 15 +++++++++++++++ >> > > 2 files changed, 50 insertions(+), 4 deletions(-) >> > > >> > > diff --git a/drivers/remoteproc/qcom_common.c >> > > b/drivers/remoteproc/qcom_common.c >> > > index 6714f27..6f04a5b 100644 >> > > --- a/drivers/remoteproc/qcom_common.c >> > > +++ b/drivers/remoteproc/qcom_common.c >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); >> > > * >> > > * Returns pointer to srcu notifier head on success, ERR_PTR on >> > > failure. >> > > * >> > > - * This registers the @notify function as handler for restart >> > > notifications. As >> > > - * remote processors are stopped this function will be called, with >> > > the rproc >> > > - * pointer passed as a parameter. >> > > + * This registers the @notify function as handler for >> > > powerup/shutdown >> > > + * notifications. This function will be invoked inside the >> > > callbacks registered >> > > + * for the ssr subdevice, with the rproc pointer passed as a >> > > parameter. >> > > */ >> > > void *qcom_register_ssr_notifier(struct rproc *rproc, struct >> > > notifier_block *nb) >> > > { >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, >> > > struct notifier_block *nb) >> > > } >> > > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >> > > >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) >> > > +{ >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> > > + >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> > > + RPROC_BEFORE_POWERUP, (void *)ssr->name); >> > > + return 0; >> > > +} >> > > + >> > > +static int ssr_notify_start(struct rproc_subdev *subdev) >> > > +{ >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> > > + >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> > > + RPROC_AFTER_POWERUP, (void *)ssr->name); >> > > + return 0; >> > > +} >> > > + >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool >> > > crashed) >> > > +{ >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> > > + >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> > > + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); >> > > +} >> > > + >> > > + >> > > static void ssr_notify_unprepare(struct rproc_subdev *subdev) >> > > { >> > > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> > > >> > > - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void >> > > *)ssr->name); >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> > > + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); >> > > } >> > > >> > > /** >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, >> > > struct qcom_rproc_ssr *ssr, >> > > { >> > > ssr->name = ssr_name; >> > > ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); >> > > + ssr->subdev.prepare = ssr_notify_prepare; >> > > + ssr->subdev.start = ssr_notify_start; >> > > + ssr->subdev.stop = ssr_notify_stop; >> > >> > Now that I have a better understanding of what this patchset is doing, I >> > realise >> > my comments in patch 04 won't work. To differentiate the subdevs of an >> > rproc I >> > suggest to wrap them in a generic structure with a type and an enum. >> > That way >> > you can differenciate between subdevices without having to add to the >> > core. >> Ok. I can try that. >> > >> > That being said, I don't understand what patches 5 and 6 are doing... >> > Registering with the global ssr_notifiers allowed to gracefully shutdown >> > all the >> > MCUs in the system when one of them would go down. But now that we are >> > using >> > the notifier on a per MCU, I really don't see why each subdev couldn't >> > implement >> > the right prepare/start/stop functions. >> > >> > Am I missing something here? >> We only want kernel clients to be notified when the Remoteproc they >> are >> interested >> in changes state. For e.g. audio kernel driver should be notified when >> audio >> processor goes down but it does not care about any other remoteproc. >> If you are suggesting that these kernel clients be added as subdevices >> then >> we will end up having many subdevices registered to each remoteproc. >> So we >> implemented a notifier chain per Remoteproc. This keeps the SSR >> notifications as >> the subdevice per remoteproc, and all interested clients can register >> to it. > > It seems like I am missing information... Your are referring to > "kernel > clients" and as such I must assume some drivers that are not part of > the > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). > I must Yes these are not part of remoteproc framework and they will register for notifications. > also assume these drivers (or that functionality) are not yet upsream > because > all I can see calling qcom_register_ssr_notifier() is > qcom_glink_ssr_probe(). Correct.These are not upstreamed. > > Speaking of which, what is the role of the qcom_glink_ssr_driver? Is > the glink > device that driver is handling the same as the glink device registed in > adsp_probe() and q6v5_probe()? glink ssr driver will send out notifications to remoteprocs that have opened the "glink_ssr" channel that some subsystem has gone down or booted up. This helps notify neighboring subsystems about change in state of any other subsystem. > >> > >> > >> > > ssr->subdev.unprepare = ssr_notify_unprepare; >> > > ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), >> > > GFP_KERNEL); >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> > > index e2f60cc..4be4478 100644 >> > > --- a/include/linux/remoteproc.h >> > > +++ b/include/linux/remoteproc.h >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment { >> > > }; >> > > >> > > /** >> > > + * enum rproc_notif_type - Different stages of remoteproc >> > > notifications >> > > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc >> > > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc >> > > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc >> > > + * @RPROC_AFTER_POWERUP: start stage of remoteproc >> > > + */ >> > > +enum rproc_notif_type { >> > > + RPROC_BEFORE_SHUTDOWN, >> > > + RPROC_AFTER_SHUTDOWN, >> > > + RPROC_BEFORE_POWERUP, >> > > + RPROC_AFTER_POWERUP, >> > > + RPROC_MAX >> > > +}; >> > > + >> > > +/** >> > > * struct rproc - represents a physical remote processor device >> > > * @node: list node of this rproc object >> > > * @domain: iommu domain >> > > -- >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> > > a Linux Foundation Collaborative Project >> > > >> > > _______________________________________________ >> > > linux-arm-kernel mailing list >> > > linux-arm-kernel@lists.infradead.org >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote: > > On 2020-02-28 10:38, Mathieu Poirier wrote: > > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org > > wrote: > >> On 2020-02-27 13:59, Mathieu Poirier wrote: > >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: > >> > > The SSR subdevice only adds callback for the unprepare event. Add > >> > > callbacks > >> > > for unprepare, start and prepare events. The client driver for a > >> > > particular > >> > > remoteproc might be interested in knowing the status of the remoteproc > >> > > while undergoing SSR, not just when the remoteproc has finished > >> > > shutting > >> > > down. > >> > > > >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > >> > > --- > >> > > drivers/remoteproc/qcom_common.c | 39 > >> > > +++++++++++++++++++++++++++++++++++---- > >> > > include/linux/remoteproc.h | 15 +++++++++++++++ > >> > > 2 files changed, 50 insertions(+), 4 deletions(-) > >> > > > >> > > diff --git a/drivers/remoteproc/qcom_common.c > >> > > b/drivers/remoteproc/qcom_common.c > >> > > index 6714f27..6f04a5b 100644 > >> > > --- a/drivers/remoteproc/qcom_common.c > >> > > +++ b/drivers/remoteproc/qcom_common.c > >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); > >> > > * > >> > > * Returns pointer to srcu notifier head on success, ERR_PTR on > >> > > failure. > >> > > * > >> > > - * This registers the @notify function as handler for restart > >> > > notifications. As > >> > > - * remote processors are stopped this function will be called, with > >> > > the rproc > >> > > - * pointer passed as a parameter. > >> > > + * This registers the @notify function as handler for > >> > > powerup/shutdown > >> > > + * notifications. This function will be invoked inside the > >> > > callbacks registered > >> > > + * for the ssr subdevice, with the rproc pointer passed as a > >> > > parameter. > >> > > */ > >> > > void *qcom_register_ssr_notifier(struct rproc *rproc, struct > >> > > notifier_block *nb) > >> > > { > >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, > >> > > struct notifier_block *nb) > >> > > } > >> > > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > >> > > > >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) > >> > > +{ > >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> > > + > >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> > > + RPROC_BEFORE_POWERUP, (void *)ssr->name); > >> > > + return 0; > >> > > +} > >> > > + > >> > > +static int ssr_notify_start(struct rproc_subdev *subdev) > >> > > +{ > >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> > > + > >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> > > + RPROC_AFTER_POWERUP, (void *)ssr->name); > >> > > + return 0; > >> > > +} > >> > > + > >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool > >> > > crashed) > >> > > +{ > >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> > > + > >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> > > + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); > >> > > +} > >> > > + > >> > > + > >> > > static void ssr_notify_unprepare(struct rproc_subdev *subdev) > >> > > { > >> > > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> > > > >> > > - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void > >> > > *)ssr->name); > >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> > > + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); > >> > > } > >> > > > >> > > /** > >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, > >> > > struct qcom_rproc_ssr *ssr, > >> > > { > >> > > ssr->name = ssr_name; > >> > > ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); > >> > > + ssr->subdev.prepare = ssr_notify_prepare; > >> > > + ssr->subdev.start = ssr_notify_start; > >> > > + ssr->subdev.stop = ssr_notify_stop; > >> > > >> > Now that I have a better understanding of what this patchset is doing, I > >> > realise > >> > my comments in patch 04 won't work. To differentiate the subdevs of an > >> > rproc I > >> > suggest to wrap them in a generic structure with a type and an enum. > >> > That way > >> > you can differenciate between subdevices without having to add to the > >> > core. > >> Ok. I can try that. > >> > > >> > That being said, I don't understand what patches 5 and 6 are doing... > >> > Registering with the global ssr_notifiers allowed to gracefully shutdown > >> > all the > >> > MCUs in the system when one of them would go down. But now that we are > >> > using > >> > the notifier on a per MCU, I really don't see why each subdev couldn't > >> > implement > >> > the right prepare/start/stop functions. > >> > > >> > Am I missing something here? > >> We only want kernel clients to be notified when the Remoteproc they > >> are > >> interested > >> in changes state. For e.g. audio kernel driver should be notified when > >> audio > >> processor goes down but it does not care about any other remoteproc. > >> If you are suggesting that these kernel clients be added as subdevices > >> then > >> we will end up having many subdevices registered to each remoteproc. > >> So we > >> implemented a notifier chain per Remoteproc. This keeps the SSR > >> notifications as > >> the subdevice per remoteproc, and all interested clients can register > >> to it. > > > > It seems like I am missing information... Your are referring to > > "kernel > > clients" and as such I must assume some drivers that are not part of > > the > > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). > > I must > Yes these are not part of remoteproc framework and they will register > for notifications. > > also assume these drivers (or that functionality) are not yet upsream > > because > > all I can see calling qcom_register_ssr_notifier() is > > qcom_glink_ssr_probe(). > Correct.These are not upstreamed. Ok, things are starting to make sense. > > > > Speaking of which, what is the role of the qcom_glink_ssr_driver? Is > > the glink > > device that driver is handling the same as the glink device registed in > > adsp_probe() and q6v5_probe()? > glink ssr driver will send out notifications to remoteprocs that have > opened the > "glink_ssr" channel that some subsystem has gone down or booted up. This > helps notify > neighboring subsystems about change in state of any other subsystem. I am still looking for an answer to my second question. > > > >> > > >> > > >> > > ssr->subdev.unprepare = ssr_notify_unprepare; > >> > > ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), > >> > > GFP_KERNEL); > >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > >> > > index e2f60cc..4be4478 100644 > >> > > --- a/include/linux/remoteproc.h > >> > > +++ b/include/linux/remoteproc.h > >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment { > >> > > }; > >> > > > >> > > /** > >> > > + * enum rproc_notif_type - Different stages of remoteproc > >> > > notifications > >> > > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc > >> > > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc > >> > > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc > >> > > + * @RPROC_AFTER_POWERUP: start stage of remoteproc > >> > > + */ > >> > > +enum rproc_notif_type { > >> > > + RPROC_BEFORE_SHUTDOWN, > >> > > + RPROC_AFTER_SHUTDOWN, > >> > > + RPROC_BEFORE_POWERUP, > >> > > + RPROC_AFTER_POWERUP, > >> > > + RPROC_MAX > >> > > +}; > >> > > + > >> > > +/** > >> > > * struct rproc - represents a physical remote processor device > >> > > * @node: list node of this rproc object > >> > > * @domain: iommu domain > >> > > -- > >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > >> > > a Linux Foundation Collaborative Project > >> > > > >> > > _______________________________________________ > >> > > linux-arm-kernel mailing list > >> > > linux-arm-kernel@lists.infradead.org > >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2020-03-03 10:05, Mathieu Poirier wrote: > On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote: >> >> On 2020-02-28 10:38, Mathieu Poirier wrote: >> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org >> > wrote: >> >> On 2020-02-27 13:59, Mathieu Poirier wrote: >> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: >> >> > > The SSR subdevice only adds callback for the unprepare event. Add >> >> > > callbacks >> >> > > for unprepare, start and prepare events. The client driver for a >> >> > > particular >> >> > > remoteproc might be interested in knowing the status of the remoteproc >> >> > > while undergoing SSR, not just when the remoteproc has finished >> >> > > shutting >> >> > > down. >> >> > > >> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >> >> > > --- >> >> > > drivers/remoteproc/qcom_common.c | 39 >> >> > > +++++++++++++++++++++++++++++++++++---- >> >> > > include/linux/remoteproc.h | 15 +++++++++++++++ >> >> > > 2 files changed, 50 insertions(+), 4 deletions(-) >> >> > > >> >> > > diff --git a/drivers/remoteproc/qcom_common.c >> >> > > b/drivers/remoteproc/qcom_common.c >> >> > > index 6714f27..6f04a5b 100644 >> >> > > --- a/drivers/remoteproc/qcom_common.c >> >> > > +++ b/drivers/remoteproc/qcom_common.c >> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); >> >> > > * >> >> > > * Returns pointer to srcu notifier head on success, ERR_PTR on >> >> > > failure. >> >> > > * >> >> > > - * This registers the @notify function as handler for restart >> >> > > notifications. As >> >> > > - * remote processors are stopped this function will be called, with >> >> > > the rproc >> >> > > - * pointer passed as a parameter. >> >> > > + * This registers the @notify function as handler for >> >> > > powerup/shutdown >> >> > > + * notifications. This function will be invoked inside the >> >> > > callbacks registered >> >> > > + * for the ssr subdevice, with the rproc pointer passed as a >> >> > > parameter. >> >> > > */ >> >> > > void *qcom_register_ssr_notifier(struct rproc *rproc, struct >> >> > > notifier_block *nb) >> >> > > { >> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, >> >> > > struct notifier_block *nb) >> >> > > } >> >> > > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >> >> > > >> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) >> >> > > +{ >> >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> >> > > + >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> >> > > + RPROC_BEFORE_POWERUP, (void *)ssr->name); >> >> > > + return 0; >> >> > > +} >> >> > > + >> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev) >> >> > > +{ >> >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> >> > > + >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> >> > > + RPROC_AFTER_POWERUP, (void *)ssr->name); >> >> > > + return 0; >> >> > > +} >> >> > > + >> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool >> >> > > crashed) >> >> > > +{ >> >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> >> > > + >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> >> > > + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); >> >> > > +} >> >> > > + >> >> > > + >> >> > > static void ssr_notify_unprepare(struct rproc_subdev *subdev) >> >> > > { >> >> > > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >> >> > > >> >> > > - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void >> >> > > *)ssr->name); >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, >> >> > > + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); >> >> > > } >> >> > > >> >> > > /** >> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, >> >> > > struct qcom_rproc_ssr *ssr, >> >> > > { >> >> > > ssr->name = ssr_name; >> >> > > ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); >> >> > > + ssr->subdev.prepare = ssr_notify_prepare; >> >> > > + ssr->subdev.start = ssr_notify_start; >> >> > > + ssr->subdev.stop = ssr_notify_stop; >> >> > >> >> > Now that I have a better understanding of what this patchset is doing, I >> >> > realise >> >> > my comments in patch 04 won't work. To differentiate the subdevs of an >> >> > rproc I >> >> > suggest to wrap them in a generic structure with a type and an enum. >> >> > That way >> >> > you can differenciate between subdevices without having to add to the >> >> > core. >> >> Ok. I can try that. >> >> > >> >> > That being said, I don't understand what patches 5 and 6 are doing... >> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown >> >> > all the >> >> > MCUs in the system when one of them would go down. But now that we are >> >> > using >> >> > the notifier on a per MCU, I really don't see why each subdev couldn't >> >> > implement >> >> > the right prepare/start/stop functions. >> >> > >> >> > Am I missing something here? >> >> We only want kernel clients to be notified when the Remoteproc they >> >> are >> >> interested >> >> in changes state. For e.g. audio kernel driver should be notified when >> >> audio >> >> processor goes down but it does not care about any other remoteproc. >> >> If you are suggesting that these kernel clients be added as subdevices >> >> then >> >> we will end up having many subdevices registered to each remoteproc. >> >> So we >> >> implemented a notifier chain per Remoteproc. This keeps the SSR >> >> notifications as >> >> the subdevice per remoteproc, and all interested clients can register >> >> to it. >> > >> > It seems like I am missing information... Your are referring to >> > "kernel >> > clients" and as such I must assume some drivers that are not part of >> > the >> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). >> > I must >> Yes these are not part of remoteproc framework and they will register >> for notifications. >> > also assume these drivers (or that functionality) are not yet upsream >> > because >> > all I can see calling qcom_register_ssr_notifier() is >> > qcom_glink_ssr_probe(). >> Correct.These are not upstreamed. > > Ok, things are starting to make sense. > >> > >> > Speaking of which, what is the role of the qcom_glink_ssr_driver? Is >> > the glink >> > device that driver is handling the same as the glink device registed in >> > adsp_probe() and q6v5_probe()? >> glink ssr driver will send out notifications to remoteprocs that have >> opened the >> "glink_ssr" channel that some subsystem has gone down or booted up. >> This >> helps notify >> neighboring subsystems about change in state of any other subsystem. > > I am still looking for an answer to my second question. Yes its the subdevice of the glink device that is registered in adsp_probe. It uses the "glink_ssr" glink channel. > >> > >> >> > >> >> > >> >> > > ssr->subdev.unprepare = ssr_notify_unprepare; >> >> > > ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), >> >> > > GFP_KERNEL); >> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >> >> > > index e2f60cc..4be4478 100644 >> >> > > --- a/include/linux/remoteproc.h >> >> > > +++ b/include/linux/remoteproc.h >> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment { >> >> > > }; >> >> > > >> >> > > /** >> >> > > + * enum rproc_notif_type - Different stages of remoteproc >> >> > > notifications >> >> > > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc >> >> > > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc >> >> > > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc >> >> > > + * @RPROC_AFTER_POWERUP: start stage of remoteproc >> >> > > + */ >> >> > > +enum rproc_notif_type { >> >> > > + RPROC_BEFORE_SHUTDOWN, >> >> > > + RPROC_AFTER_SHUTDOWN, >> >> > > + RPROC_BEFORE_POWERUP, >> >> > > + RPROC_AFTER_POWERUP, >> >> > > + RPROC_MAX >> >> > > +}; >> >> > > + >> >> > > +/** >> >> > > * struct rproc - represents a physical remote processor device >> >> > > * @node: list node of this rproc object >> >> > > * @domain: iommu domain >> >> > > -- >> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> >> > > a Linux Foundation Collaborative Project >> >> > > >> >> > > _______________________________________________ >> >> > > linux-arm-kernel mailing list >> >> > > linux-arm-kernel@lists.infradead.org >> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote: > > On 2020-03-03 10:05, Mathieu Poirier wrote: > > On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote: > >> > >> On 2020-02-28 10:38, Mathieu Poirier wrote: > >> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org > >> > wrote: > >> >> On 2020-02-27 13:59, Mathieu Poirier wrote: > >> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: > >> >> > > The SSR subdevice only adds callback for the unprepare event. Add > >> >> > > callbacks > >> >> > > for unprepare, start and prepare events. The client driver for a > >> >> > > particular > >> >> > > remoteproc might be interested in knowing the status of the remoteproc > >> >> > > while undergoing SSR, not just when the remoteproc has finished > >> >> > > shutting > >> >> > > down. > >> >> > > > >> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > >> >> > > --- > >> >> > > drivers/remoteproc/qcom_common.c | 39 > >> >> > > +++++++++++++++++++++++++++++++++++---- > >> >> > > include/linux/remoteproc.h | 15 +++++++++++++++ > >> >> > > 2 files changed, 50 insertions(+), 4 deletions(-) > >> >> > > > >> >> > > diff --git a/drivers/remoteproc/qcom_common.c > >> >> > > b/drivers/remoteproc/qcom_common.c > >> >> > > index 6714f27..6f04a5b 100644 > >> >> > > --- a/drivers/remoteproc/qcom_common.c > >> >> > > +++ b/drivers/remoteproc/qcom_common.c > >> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); > >> >> > > * > >> >> > > * Returns pointer to srcu notifier head on success, ERR_PTR on > >> >> > > failure. > >> >> > > * > >> >> > > - * This registers the @notify function as handler for restart > >> >> > > notifications. As > >> >> > > - * remote processors are stopped this function will be called, with > >> >> > > the rproc > >> >> > > - * pointer passed as a parameter. > >> >> > > + * This registers the @notify function as handler for > >> >> > > powerup/shutdown > >> >> > > + * notifications. This function will be invoked inside the > >> >> > > callbacks registered > >> >> > > + * for the ssr subdevice, with the rproc pointer passed as a > >> >> > > parameter. > >> >> > > */ > >> >> > > void *qcom_register_ssr_notifier(struct rproc *rproc, struct > >> >> > > notifier_block *nb) > >> >> > > { > >> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, > >> >> > > struct notifier_block *nb) > >> >> > > } > >> >> > > EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > >> >> > > > >> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev) > >> >> > > +{ > >> >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> >> > > + > >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> >> > > + RPROC_BEFORE_POWERUP, (void *)ssr->name); > >> >> > > + return 0; > >> >> > > +} > >> >> > > + > >> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev) > >> >> > > +{ > >> >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> >> > > + > >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> >> > > + RPROC_AFTER_POWERUP, (void *)ssr->name); > >> >> > > + return 0; > >> >> > > +} > >> >> > > + > >> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool > >> >> > > crashed) > >> >> > > +{ > >> >> > > + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> >> > > + > >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> >> > > + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); > >> >> > > +} > >> >> > > + > >> >> > > + > >> >> > > static void ssr_notify_unprepare(struct rproc_subdev *subdev) > >> >> > > { > >> >> > > struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >> >> > > > >> >> > > - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void > >> >> > > *)ssr->name); > >> >> > > + srcu_notifier_call_chain(ssr->rproc_notif_list, > >> >> > > + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); > >> >> > > } > >> >> > > > >> >> > > /** > >> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, > >> >> > > struct qcom_rproc_ssr *ssr, > >> >> > > { > >> >> > > ssr->name = ssr_name; > >> >> > > ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); > >> >> > > + ssr->subdev.prepare = ssr_notify_prepare; > >> >> > > + ssr->subdev.start = ssr_notify_start; > >> >> > > + ssr->subdev.stop = ssr_notify_stop; > >> >> > > >> >> > Now that I have a better understanding of what this patchset is doing, I > >> >> > realise > >> >> > my comments in patch 04 won't work. To differentiate the subdevs of an > >> >> > rproc I > >> >> > suggest to wrap them in a generic structure with a type and an enum. > >> >> > That way > >> >> > you can differenciate between subdevices without having to add to the > >> >> > core. > >> >> Ok. I can try that. > >> >> > > >> >> > That being said, I don't understand what patches 5 and 6 are doing... > >> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown > >> >> > all the > >> >> > MCUs in the system when one of them would go down. But now that we are > >> >> > using > >> >> > the notifier on a per MCU, I really don't see why each subdev couldn't > >> >> > implement > >> >> > the right prepare/start/stop functions. > >> >> > > >> >> > Am I missing something here? > >> >> We only want kernel clients to be notified when the Remoteproc they > >> >> are > >> >> interested > >> >> in changes state. For e.g. audio kernel driver should be notified when > >> >> audio > >> >> processor goes down but it does not care about any other remoteproc. > >> >> If you are suggesting that these kernel clients be added as subdevices > >> >> then > >> >> we will end up having many subdevices registered to each remoteproc. > >> >> So we > >> >> implemented a notifier chain per Remoteproc. This keeps the SSR > >> >> notifications as > >> >> the subdevice per remoteproc, and all interested clients can register > >> >> to it. > >> > > >> > It seems like I am missing information... Your are referring to > >> > "kernel > >> > clients" and as such I must assume some drivers that are not part of > >> > the > >> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). > >> > I must > >> Yes these are not part of remoteproc framework and they will register > >> for notifications. > >> > also assume these drivers (or that functionality) are not yet upsream > >> > because > >> > all I can see calling qcom_register_ssr_notifier() is > >> > qcom_glink_ssr_probe(). > >> Correct.These are not upstreamed. > > > > Ok, things are starting to make sense. > > > >> > > >> > Speaking of which, what is the role of the qcom_glink_ssr_driver? Is > >> > the glink > >> > device that driver is handling the same as the glink device registed in > >> > adsp_probe() and q6v5_probe()? > >> glink ssr driver will send out notifications to remoteprocs that have > >> opened the > >> "glink_ssr" channel that some subsystem has gone down or booted up. > >> This > >> helps notify > >> neighboring subsystems about change in state of any other subsystem. > > > > I am still looking for an answer to my second question. > Yes its the subdevice of the glink device that is registered in > adsp_probe. > It uses the "glink_ssr" glink channel. Since this is confining events to a single MCU, I was mostly worried about opening the "glink_ssr" channel for nothing but taking a step back and thinking further on this, there might be other purposes for the channel than only receiving notifications of other MCUs in the system going down. Please spin off a new revision of this set and I will take another look. Thanks, Mathieu > > > >> > > >> >> > > >> >> > > >> >> > > ssr->subdev.unprepare = ssr_notify_unprepare; > >> >> > > ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), > >> >> > > GFP_KERNEL); > >> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > >> >> > > index e2f60cc..4be4478 100644 > >> >> > > --- a/include/linux/remoteproc.h > >> >> > > +++ b/include/linux/remoteproc.h > >> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment { > >> >> > > }; > >> >> > > > >> >> > > /** > >> >> > > + * enum rproc_notif_type - Different stages of remoteproc > >> >> > > notifications > >> >> > > + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc > >> >> > > + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc > >> >> > > + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc > >> >> > > + * @RPROC_AFTER_POWERUP: start stage of remoteproc > >> >> > > + */ > >> >> > > +enum rproc_notif_type { > >> >> > > + RPROC_BEFORE_SHUTDOWN, > >> >> > > + RPROC_AFTER_SHUTDOWN, > >> >> > > + RPROC_BEFORE_POWERUP, > >> >> > > + RPROC_AFTER_POWERUP, > >> >> > > + RPROC_MAX > >> >> > > +}; > >> >> > > + > >> >> > > +/** > >> >> > > * struct rproc - represents a physical remote processor device > >> >> > > * @node: list node of this rproc object > >> >> > > * @domain: iommu domain > >> >> > > -- > >> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > >> >> > > a Linux Foundation Collaborative Project > >> >> > > > >> >> > > _______________________________________________ > >> >> > > linux-arm-kernel mailing list > >> >> > > linux-arm-kernel@lists.infradead.org > >> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >> > >> _______________________________________________ > >> linux-arm-kernel mailing list > >> linux-arm-kernel@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 3/9/2020 10:34 AM, Mathieu Poirier wrote: > On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote: >> On 2020-03-03 10:05, Mathieu Poirier wrote: >>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote: >>>> On 2020-02-28 10:38, Mathieu Poirier wrote: >>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org >>>>> wrote: >>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote: >>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: >>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add >>>>>>>> callbacks >>>>>>>> for unprepare, start and prepare events. The client driver for a >>>>>>>> particular >>>>>>>> remoteproc might be interested in knowing the status of the remoteproc >>>>>>>> while undergoing SSR, not just when the remoteproc has finished >>>>>>>> shutting >>>>>>>> down. >>>>>>>> >>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> >>>>>>>> --- >>>>>>>> drivers/remoteproc/qcom_common.c | 39 >>>>>>>> +++++++++++++++++++++++++++++++++++---- >>>>>>>> include/linux/remoteproc.h | 15 +++++++++++++++ >>>>>>>> 2 files changed, 50 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c >>>>>>>> b/drivers/remoteproc/qcom_common.c >>>>>>>> index 6714f27..6f04a5b 100644 >>>>>>>> --- a/drivers/remoteproc/qcom_common.c >>>>>>>> +++ b/drivers/remoteproc/qcom_common.c >>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); >>>>>>>> * >>>>>>>> * Returns pointer to srcu notifier head on success, ERR_PTR on >>>>>>>> failure. >>>>>>>> * >>>>>>>> - * This registers the @notify function as handler for restart >>>>>>>> notifications. As >>>>>>>> - * remote processors are stopped this function will be called, with >>>>>>>> the rproc >>>>>>>> - * pointer passed as a parameter. >>>>>>>> + * This registers the @notify function as handler for >>>>>>>> powerup/shutdown >>>>>>>> + * notifications. This function will be invoked inside the >>>>>>>> callbacks registered >>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a >>>>>>>> parameter. >>>>>>>> */ >>>>>>>> void *qcom_register_ssr_notifier(struct rproc *rproc, struct >>>>>>>> notifier_block *nb) >>>>>>>> { >>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, >>>>>>>> struct notifier_block *nb) >>>>>>>> } >>>>>>>> EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); >>>>>>>> >>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev) >>>>>>>> +{ >>>>>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >>>>>>>> + >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, >>>>>>>> + RPROC_BEFORE_POWERUP, (void *)ssr->name); >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev) >>>>>>>> +{ >>>>>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >>>>>>>> + >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, >>>>>>>> + RPROC_AFTER_POWERUP, (void *)ssr->name); >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool >>>>>>>> crashed) >>>>>>>> +{ >>>>>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >>>>>>>> + >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, >>>>>>>> + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); >>>>>>>> +} >>>>>>>> + >>>>>>>> + >>>>>>>> static void ssr_notify_unprepare(struct rproc_subdev *subdev) >>>>>>>> { >>>>>>>> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); >>>>>>>> >>>>>>>> - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void >>>>>>>> *)ssr->name); >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, >>>>>>>> + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); >>>>>>>> } >>>>>>>> >>>>>>>> /** >>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, >>>>>>>> struct qcom_rproc_ssr *ssr, >>>>>>>> { >>>>>>>> ssr->name = ssr_name; >>>>>>>> ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); >>>>>>>> + ssr->subdev.prepare = ssr_notify_prepare; >>>>>>>> + ssr->subdev.start = ssr_notify_start; >>>>>>>> + ssr->subdev.stop = ssr_notify_stop; >>>>>>> Now that I have a better understanding of what this patchset is doing, I >>>>>>> realise >>>>>>> my comments in patch 04 won't work. To differentiate the subdevs of an >>>>>>> rproc I >>>>>>> suggest to wrap them in a generic structure with a type and an enum. >>>>>>> That way >>>>>>> you can differenciate between subdevices without having to add to the >>>>>>> core. While creating a new revision of the patchset we tried to implement this, but a similar issue comes up. If at a later point we wish to utilize the functionality of some common subdevice (not the case right now, but potentially), we might run into a similar problem of accessing illegal memory using container_of. I think it might be a better idea to introduce the name in the subdevice structure over having a potential security bug. What do you think? Thanks, Siddharth >>>>>> Ok. I can try that. >>>>>>> That being said, I don't understand what patches 5 and 6 are doing... >>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown >>>>>>> all the >>>>>>> MCUs in the system when one of them would go down. But now that we are >>>>>>> using >>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't >>>>>>> implement >>>>>>> the right prepare/start/stop functions. >>>>>>> >>>>>>> Am I missing something here? >>>>>> We only want kernel clients to be notified when the Remoteproc they >>>>>> are >>>>>> interested >>>>>> in changes state. For e.g. audio kernel driver should be notified when >>>>>> audio >>>>>> processor goes down but it does not care about any other remoteproc. >>>>>> If you are suggesting that these kernel clients be added as subdevices >>>>>> then >>>>>> we will end up having many subdevices registered to each remoteproc. >>>>>> So we >>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR >>>>>> notifications as >>>>>> the subdevice per remoteproc, and all interested clients can register >>>>>> to it. >>>>> It seems like I am missing information... Your are referring to >>>>> "kernel >>>>> clients" and as such I must assume some drivers that are not part of >>>>> the >>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). >>>>> I must >>>> Yes these are not part of remoteproc framework and they will register >>>> for notifications. >>>>> also assume these drivers (or that functionality) are not yet upsream >>>>> because >>>>> all I can see calling qcom_register_ssr_notifier() is >>>>> qcom_glink_ssr_probe(). >>>> Correct.These are not upstreamed. >>> Ok, things are starting to make sense. >>> >>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver? Is >>>>> the glink >>>>> device that driver is handling the same as the glink device registed in >>>>> adsp_probe() and q6v5_probe()? >>>> glink ssr driver will send out notifications to remoteprocs that have >>>> opened the >>>> "glink_ssr" channel that some subsystem has gone down or booted up. >>>> This >>>> helps notify >>>> neighboring subsystems about change in state of any other subsystem. >>> I am still looking for an answer to my second question. >> Yes its the subdevice of the glink device that is registered in >> adsp_probe. >> It uses the "glink_ssr" glink channel. > Since this is confining events to a single MCU, I was mostly worried > about opening the "glink_ssr" channel for nothing but taking a step > back and thinking further on this, there might be other purposes for > the channel than only receiving notifications of other MCUs in the > system going down. > > Please spin off a new revision of this set and I will take another look. > > Thanks, > Mathieu > >>>>>>> >>>>>>>> ssr->subdev.unprepare = ssr_notify_unprepare; >>>>>>>> ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), >>>>>>>> GFP_KERNEL); >>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h >>>>>>>> index e2f60cc..4be4478 100644 >>>>>>>> --- a/include/linux/remoteproc.h >>>>>>>> +++ b/include/linux/remoteproc.h >>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment { >>>>>>>> }; >>>>>>>> >>>>>>>> /** >>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc >>>>>>>> notifications >>>>>>>> + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc >>>>>>>> + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc >>>>>>>> + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc >>>>>>>> + * @RPROC_AFTER_POWERUP: start stage of remoteproc >>>>>>>> + */ >>>>>>>> +enum rproc_notif_type { >>>>>>>> + RPROC_BEFORE_SHUTDOWN, >>>>>>>> + RPROC_AFTER_SHUTDOWN, >>>>>>>> + RPROC_BEFORE_POWERUP, >>>>>>>> + RPROC_AFTER_POWERUP, >>>>>>>> + RPROC_MAX >>>>>>>> +}; >>>>>>>> + >>>>>>>> +/** >>>>>>>> * struct rproc - represents a physical remote processor device >>>>>>>> * @node: list node of this rproc object >>>>>>>> * @domain: iommu domain >>>>>>>> -- >>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >>>>>>>> a Linux Foundation Collaborative Project >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> linux-arm-kernel mailing list >>>>>>>> linux-arm-kernel@lists.infradead.org >>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 1 Apr 2020 at 19:01, Siddharth Gupta <sidgup@codeaurora.org> wrote: > > On 3/9/2020 10:34 AM, Mathieu Poirier wrote: > > > On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote: > >> On 2020-03-03 10:05, Mathieu Poirier wrote: > >>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote: > >>>> On 2020-02-28 10:38, Mathieu Poirier wrote: > >>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org > >>>>> wrote: > >>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote: > >>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote: > >>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add > >>>>>>>> callbacks > >>>>>>>> for unprepare, start and prepare events. The client driver for a > >>>>>>>> particular > >>>>>>>> remoteproc might be interested in knowing the status of the remoteproc > >>>>>>>> while undergoing SSR, not just when the remoteproc has finished > >>>>>>>> shutting > >>>>>>>> down. > >>>>>>>> > >>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> > >>>>>>>> --- > >>>>>>>> drivers/remoteproc/qcom_common.c | 39 > >>>>>>>> +++++++++++++++++++++++++++++++++++---- > >>>>>>>> include/linux/remoteproc.h | 15 +++++++++++++++ > >>>>>>>> 2 files changed, 50 insertions(+), 4 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c > >>>>>>>> b/drivers/remoteproc/qcom_common.c > >>>>>>>> index 6714f27..6f04a5b 100644 > >>>>>>>> --- a/drivers/remoteproc/qcom_common.c > >>>>>>>> +++ b/drivers/remoteproc/qcom_common.c > >>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); > >>>>>>>> * > >>>>>>>> * Returns pointer to srcu notifier head on success, ERR_PTR on > >>>>>>>> failure. > >>>>>>>> * > >>>>>>>> - * This registers the @notify function as handler for restart > >>>>>>>> notifications. As > >>>>>>>> - * remote processors are stopped this function will be called, with > >>>>>>>> the rproc > >>>>>>>> - * pointer passed as a parameter. > >>>>>>>> + * This registers the @notify function as handler for > >>>>>>>> powerup/shutdown > >>>>>>>> + * notifications. This function will be invoked inside the > >>>>>>>> callbacks registered > >>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a > >>>>>>>> parameter. > >>>>>>>> */ > >>>>>>>> void *qcom_register_ssr_notifier(struct rproc *rproc, struct > >>>>>>>> notifier_block *nb) > >>>>>>>> { > >>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, > >>>>>>>> struct notifier_block *nb) > >>>>>>>> } > >>>>>>>> EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); > >>>>>>>> > >>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev) > >>>>>>>> +{ > >>>>>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >>>>>>>> + > >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, > >>>>>>>> + RPROC_BEFORE_POWERUP, (void *)ssr->name); > >>>>>>>> + return 0; > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev) > >>>>>>>> +{ > >>>>>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >>>>>>>> + > >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, > >>>>>>>> + RPROC_AFTER_POWERUP, (void *)ssr->name); > >>>>>>>> + return 0; > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool > >>>>>>>> crashed) > >>>>>>>> +{ > >>>>>>>> + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >>>>>>>> + > >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, > >>>>>>>> + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); > >>>>>>>> +} > >>>>>>>> + > >>>>>>>> + > >>>>>>>> static void ssr_notify_unprepare(struct rproc_subdev *subdev) > >>>>>>>> { > >>>>>>>> struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); > >>>>>>>> > >>>>>>>> - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void > >>>>>>>> *)ssr->name); > >>>>>>>> + srcu_notifier_call_chain(ssr->rproc_notif_list, > >>>>>>>> + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); > >>>>>>>> } > >>>>>>>> > >>>>>>>> /** > >>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, > >>>>>>>> struct qcom_rproc_ssr *ssr, > >>>>>>>> { > >>>>>>>> ssr->name = ssr_name; > >>>>>>>> ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); > >>>>>>>> + ssr->subdev.prepare = ssr_notify_prepare; > >>>>>>>> + ssr->subdev.start = ssr_notify_start; > >>>>>>>> + ssr->subdev.stop = ssr_notify_stop; > >>>>>>> Now that I have a better understanding of what this patchset is doing, I > >>>>>>> realise > >>>>>>> my comments in patch 04 won't work. To differentiate the subdevs of an > >>>>>>> rproc I > >>>>>>> suggest to wrap them in a generic structure with a type and an enum. > >>>>>>> That way > >>>>>>> you can differenciate between subdevices without having to add to the > >>>>>>> core. > > While creating a new revision of the patchset we tried to implement > this, but a similar issue comes > up. If at a later point we wish to utilize the functionality of some > common subdevice (not the case > right now, but potentially), we might run into a similar problem of > accessing illegal memory using > container_of. I think it might be a better idea to introduce the name in > the subdevice structure over > having a potential security bug. What do you think? I trust that you have given this an honest try but found potential problems that I can't foresee due to the lack of insight on your operating environment. Please move forward with the addition of a new "name" field to the rproc_subdev structure. > > Thanks, > Siddharth > > >>>>>> Ok. I can try that. > >>>>>>> That being said, I don't understand what patches 5 and 6 are doing... > >>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown > >>>>>>> all the > >>>>>>> MCUs in the system when one of them would go down. But now that we are > >>>>>>> using > >>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't > >>>>>>> implement > >>>>>>> the right prepare/start/stop functions. > >>>>>>> > >>>>>>> Am I missing something here? > >>>>>> We only want kernel clients to be notified when the Remoteproc they > >>>>>> are > >>>>>> interested > >>>>>> in changes state. For e.g. audio kernel driver should be notified when > >>>>>> audio > >>>>>> processor goes down but it does not care about any other remoteproc. > >>>>>> If you are suggesting that these kernel clients be added as subdevices > >>>>>> then > >>>>>> we will end up having many subdevices registered to each remoteproc. > >>>>>> So we > >>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR > >>>>>> notifications as > >>>>>> the subdevice per remoteproc, and all interested clients can register > >>>>>> to it. > >>>>> It seems like I am missing information... Your are referring to > >>>>> "kernel > >>>>> clients" and as such I must assume some drivers that are not part of > >>>>> the > >>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier(). > >>>>> I must > >>>> Yes these are not part of remoteproc framework and they will register > >>>> for notifications. > >>>>> also assume these drivers (or that functionality) are not yet upsream > >>>>> because > >>>>> all I can see calling qcom_register_ssr_notifier() is > >>>>> qcom_glink_ssr_probe(). > >>>> Correct.These are not upstreamed. > >>> Ok, things are starting to make sense. > >>> > >>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver? Is > >>>>> the glink > >>>>> device that driver is handling the same as the glink device registed in > >>>>> adsp_probe() and q6v5_probe()? > >>>> glink ssr driver will send out notifications to remoteprocs that have > >>>> opened the > >>>> "glink_ssr" channel that some subsystem has gone down or booted up. > >>>> This > >>>> helps notify > >>>> neighboring subsystems about change in state of any other subsystem. > >>> I am still looking for an answer to my second question. > >> Yes its the subdevice of the glink device that is registered in > >> adsp_probe. > >> It uses the "glink_ssr" glink channel. > > Since this is confining events to a single MCU, I was mostly worried > > about opening the "glink_ssr" channel for nothing but taking a step > > back and thinking further on this, there might be other purposes for > > the channel than only receiving notifications of other MCUs in the > > system going down. > > > > Please spin off a new revision of this set and I will take another look. > > > > Thanks, > > Mathieu > > > >>>>>>> > >>>>>>>> ssr->subdev.unprepare = ssr_notify_unprepare; > >>>>>>>> ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), > >>>>>>>> GFP_KERNEL); > >>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h > >>>>>>>> index e2f60cc..4be4478 100644 > >>>>>>>> --- a/include/linux/remoteproc.h > >>>>>>>> +++ b/include/linux/remoteproc.h > >>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment { > >>>>>>>> }; > >>>>>>>> > >>>>>>>> /** > >>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc > >>>>>>>> notifications > >>>>>>>> + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc > >>>>>>>> + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc > >>>>>>>> + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc > >>>>>>>> + * @RPROC_AFTER_POWERUP: start stage of remoteproc > >>>>>>>> + */ > >>>>>>>> +enum rproc_notif_type { > >>>>>>>> + RPROC_BEFORE_SHUTDOWN, > >>>>>>>> + RPROC_AFTER_SHUTDOWN, > >>>>>>>> + RPROC_BEFORE_POWERUP, > >>>>>>>> + RPROC_AFTER_POWERUP, > >>>>>>>> + RPROC_MAX > >>>>>>>> +}; > >>>>>>>> + > >>>>>>>> +/** > >>>>>>>> * struct rproc - represents a physical remote processor device > >>>>>>>> * @node: list node of this rproc object > >>>>>>>> * @domain: iommu domain > >>>>>>>> -- > >>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > >>>>>>>> a Linux Foundation Collaborative Project > >>>>>>>> > >>>>>>>> _______________________________________________ > >>>>>>>> linux-arm-kernel mailing list > >>>>>>>> linux-arm-kernel@lists.infradead.org > >>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > >>>> _______________________________________________ > >>>> linux-arm-kernel mailing list > >>>> linux-arm-kernel@lists.infradead.org > >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c index 6714f27..6f04a5b 100644 --- a/drivers/remoteproc/qcom_common.c +++ b/drivers/remoteproc/qcom_common.c @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev); * * Returns pointer to srcu notifier head on success, ERR_PTR on failure. * - * This registers the @notify function as handler for restart notifications. As - * remote processors are stopped this function will be called, with the rproc - * pointer passed as a parameter. + * This registers the @notify function as handler for powerup/shutdown + * notifications. This function will be invoked inside the callbacks registered + * for the ssr subdevice, with the rproc pointer passed as a parameter. */ void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb) { @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb) } EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier); +static int ssr_notify_prepare(struct rproc_subdev *subdev) +{ + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + + srcu_notifier_call_chain(ssr->rproc_notif_list, + RPROC_BEFORE_POWERUP, (void *)ssr->name); + return 0; +} + +static int ssr_notify_start(struct rproc_subdev *subdev) +{ + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + + srcu_notifier_call_chain(ssr->rproc_notif_list, + RPROC_AFTER_POWERUP, (void *)ssr->name); + return 0; +} + +static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed) +{ + struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); + + srcu_notifier_call_chain(ssr->rproc_notif_list, + RPROC_BEFORE_SHUTDOWN, (void *)ssr->name); +} + + static void ssr_notify_unprepare(struct rproc_subdev *subdev) { struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev); - srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name); + srcu_notifier_call_chain(ssr->rproc_notif_list, + RPROC_AFTER_SHUTDOWN, (void *)ssr->name); } /** @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr, { ssr->name = ssr_name; ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL); + ssr->subdev.prepare = ssr_notify_prepare; + ssr->subdev.start = ssr_notify_start; + ssr->subdev.stop = ssr_notify_stop; ssr->subdev.unprepare = ssr_notify_unprepare; ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head), GFP_KERNEL); diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index e2f60cc..4be4478 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -449,6 +449,21 @@ struct rproc_dump_segment { }; /** + * enum rproc_notif_type - Different stages of remoteproc notifications + * @RPROC_BEFORE_SHUTDOWN: unprepare stage of remoteproc + * @RPROC_AFTER_SHUTDOWN: stop stage of remoteproc + * @RPROC_BEFORE_POWERUP: prepare stage of remoteproc + * @RPROC_AFTER_POWERUP: start stage of remoteproc + */ +enum rproc_notif_type { + RPROC_BEFORE_SHUTDOWN, + RPROC_AFTER_SHUTDOWN, + RPROC_BEFORE_POWERUP, + RPROC_AFTER_POWERUP, + RPROC_MAX +}; + +/** * struct rproc - represents a physical remote processor device * @node: list node of this rproc object * @domain: iommu domain
The SSR subdevice only adds callback for the unprepare event. Add callbacks for unprepare, start and prepare events. The client driver for a particular remoteproc might be interested in knowing the status of the remoteproc while undergoing SSR, not just when the remoteproc has finished shutting down. Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org> --- drivers/remoteproc/qcom_common.c | 39 +++++++++++++++++++++++++++++++++++---- include/linux/remoteproc.h | 15 +++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-)