From: Cristian Marussi <cristian.marussi@arm.com> To: Dave Martin <Dave.Martin@arm.com> Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Jonathan.Cameron@Huawei.com, james.quinlan@broadcom.com, lukasz.luba@arm.com, sudeep.holla@arm.com Subject: Re: [PATCH v7 1/9] firmware: arm_scmi: Add notification protocol-registration Date: Tue, 12 May 2020 18:00:20 +0100 [thread overview] Message-ID: <20200512170020.GC17648@e119603-lin.cambridge.arm.com> (raw) In-Reply-To: <20200511220403.GB17648@e119603-lin.cambridge.arm.com> On Mon, May 11, 2020 at 11:04:03PM +0100, Cristian Marussi wrote: > Hi Dave > > thanks for the review first of all. > [snip] > > I'm not sure about scmi_notification_exit() (see below). [snip] > > > +/** > > > + * scmi_notification_exit() - Shutdown and clean Notification core > > > + * @handle: The handle identifying the platform instance to shutdown > > > + */ > > > +void scmi_notification_exit(struct scmi_handle *handle) > > > +{ > > > + struct scmi_notify_instance *ni = handle->notify_priv; > > > + > > > + if (unlikely(!ni || !atomic_read(&ni->initialized))) > > > + return; > > > + > > > + atomic_set(&ni->enabled, 0); > > > + /* Ensure atomic values are updated */ > > > + smp_mb__after_atomic(); > > > > If users can race with this, we're dead: the atomic by itself doesn't > > ensure that handle is not in use once we arrive here. Should this > > be a refcount instead? > > > > If users can't race with this, we probably don't protection here. > > > > > > I may be misunderstanding what this code is doing... > > > > First of all the enabled flag does not probably belong to this commit properly; > here is initialized but it is really fully used only in subsequent patches > (...so makes apparently little sense here... my bad...) > > Anyway, in general SCMI protocols (beside notifications stuff) are initialized > as depicted above, BUT they are never deinitialized explicitly (there's no > equivalent scmi_protocol_deinit()) and also proto devices are never destroyed: > so there's no scmi_protocol_deregister_events() neither in this series, because > it would have been tricky to properly invoke it and would have not been consistent > with the original SCMI design. > > On the other side since in protocol driver _remove() some general protos resources > are in fact freed anyway, for consistency I decided to free the devm notification > resources allocated with the above init() in scmi_notification_exit(): this should > happen only at system shutdown in fact when notification are no more of a concern. > > So given there's no explicit deregister I had to ensure somehow that the wanna-be-freed > notif devm resources were safe to be released. > > In this context the 'enabled' atomic flag is set to 0 @_exit to stop the dispatch of the > events (possibly still coming from the fw) from the ISR into the kfifo queues: once such > pkts flow is stopped I destroy_sync() (in a subsequent patch @_exit too) all the workqueues > fetching the events from the kfifos: this way I can be sure that all the notif resources > are no more used at all when I free all of them with devm_release() at the end. > > All of this is an additional precaution against buggy fw not stopping sending events > even when asked to do so (by drivers when deregistering notif callbacks in their shutdown) > > Give the above scenario on shutdown (which I never observed to tell the truth), and the fact > I'm freeing all devm res (including ni) at shutdown, it's now apparent ALSO that I cannot use > 'enabled' to keep stopped the flow in a safe way after its enclosing struct ni has been freed ! > > So I'll remove the 'enable' atomic_t too and rely equally on the bare !ni check to determine > if the notification are enabled and should be dispatched. So that in > ...replying to my early self here (o_O)....I'd add that I've tested the above changes (removing initialized and enabled) triggering this _exit path by brutally unbinding the platform protocol driver and I can see the notifications flow stop and the queues emptied as expected without tragedy...the SCMI stack in general is not so happy though at that point, since it is not even supposed to be unloaded ever in fact...I wonder if this limit condition(unbind of a core SCMI driver which is not even modularizable in Kconfig) makes sense to be tested at all... (if not for testing this specific code path...) Cheers Cristian
WARNING: multiple messages have this Message-ID (diff)
From: Cristian Marussi <cristian.marussi@arm.com> To: Dave Martin <Dave.Martin@arm.com> Cc: Jonathan.Cameron@Huawei.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.quinlan@broadcom.com, sudeep.holla@arm.com, lukasz.luba@arm.com Subject: Re: [PATCH v7 1/9] firmware: arm_scmi: Add notification protocol-registration Date: Tue, 12 May 2020 18:00:20 +0100 [thread overview] Message-ID: <20200512170020.GC17648@e119603-lin.cambridge.arm.com> (raw) In-Reply-To: <20200511220403.GB17648@e119603-lin.cambridge.arm.com> On Mon, May 11, 2020 at 11:04:03PM +0100, Cristian Marussi wrote: > Hi Dave > > thanks for the review first of all. > [snip] > > I'm not sure about scmi_notification_exit() (see below). [snip] > > > +/** > > > + * scmi_notification_exit() - Shutdown and clean Notification core > > > + * @handle: The handle identifying the platform instance to shutdown > > > + */ > > > +void scmi_notification_exit(struct scmi_handle *handle) > > > +{ > > > + struct scmi_notify_instance *ni = handle->notify_priv; > > > + > > > + if (unlikely(!ni || !atomic_read(&ni->initialized))) > > > + return; > > > + > > > + atomic_set(&ni->enabled, 0); > > > + /* Ensure atomic values are updated */ > > > + smp_mb__after_atomic(); > > > > If users can race with this, we're dead: the atomic by itself doesn't > > ensure that handle is not in use once we arrive here. Should this > > be a refcount instead? > > > > If users can't race with this, we probably don't protection here. > > > > > > I may be misunderstanding what this code is doing... > > > > First of all the enabled flag does not probably belong to this commit properly; > here is initialized but it is really fully used only in subsequent patches > (...so makes apparently little sense here... my bad...) > > Anyway, in general SCMI protocols (beside notifications stuff) are initialized > as depicted above, BUT they are never deinitialized explicitly (there's no > equivalent scmi_protocol_deinit()) and also proto devices are never destroyed: > so there's no scmi_protocol_deregister_events() neither in this series, because > it would have been tricky to properly invoke it and would have not been consistent > with the original SCMI design. > > On the other side since in protocol driver _remove() some general protos resources > are in fact freed anyway, for consistency I decided to free the devm notification > resources allocated with the above init() in scmi_notification_exit(): this should > happen only at system shutdown in fact when notification are no more of a concern. > > So given there's no explicit deregister I had to ensure somehow that the wanna-be-freed > notif devm resources were safe to be released. > > In this context the 'enabled' atomic flag is set to 0 @_exit to stop the dispatch of the > events (possibly still coming from the fw) from the ISR into the kfifo queues: once such > pkts flow is stopped I destroy_sync() (in a subsequent patch @_exit too) all the workqueues > fetching the events from the kfifos: this way I can be sure that all the notif resources > are no more used at all when I free all of them with devm_release() at the end. > > All of this is an additional precaution against buggy fw not stopping sending events > even when asked to do so (by drivers when deregistering notif callbacks in their shutdown) > > Give the above scenario on shutdown (which I never observed to tell the truth), and the fact > I'm freeing all devm res (including ni) at shutdown, it's now apparent ALSO that I cannot use > 'enabled' to keep stopped the flow in a safe way after its enclosing struct ni has been freed ! > > So I'll remove the 'enable' atomic_t too and rely equally on the bare !ni check to determine > if the notification are enabled and should be dispatched. So that in > ...replying to my early self here (o_O)....I'd add that I've tested the above changes (removing initialized and enabled) triggering this _exit path by brutally unbinding the platform protocol driver and I can see the notifications flow stop and the queues emptied as expected without tragedy...the SCMI stack in general is not so happy though at that point, since it is not even supposed to be unloaded ever in fact...I wonder if this limit condition(unbind of a core SCMI driver which is not even modularizable in Kconfig) makes sense to be tested at all... (if not for testing this specific code path...) Cheers Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-05-12 17:00 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-04 16:38 [PATCH v7 0/9] SCMI Notifications Core Support Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 1/9] firmware: arm_scmi: Add notification protocol-registration Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-06 15:25 ` Dave Martin 2020-05-06 15:25 ` Dave Martin 2020-05-11 22:04 ` Cristian Marussi 2020-05-11 22:04 ` Cristian Marussi 2020-05-12 17:00 ` Cristian Marussi [this message] 2020-05-12 17:00 ` Cristian Marussi 2020-05-12 17:19 ` Sudeep Holla 2020-05-12 17:19 ` Sudeep Holla 2020-05-13 16:46 ` Dave Martin 2020-05-13 16:46 ` Dave Martin 2020-05-13 18:56 ` Cristian Marussi 2020-05-13 18:56 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 2/9] firmware: arm_scmi: Add notification callbacks-registration Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 3/9] firmware: arm_scmi: Add notification dispatch and delivery Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 4/9] firmware: arm_scmi: Enable notification core Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 5/9] firmware: arm_scmi: Add Power notifications support Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 6/9] firmware: arm_scmi: Add Perf " Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 7/9] firmware: arm_scmi: Add Sensor " Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 8/9] firmware: arm_scmi: Add Reset " Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi 2020-05-04 16:38 ` [PATCH v7 9/9] firmware: arm_scmi: Add Base " Cristian Marussi 2020-05-04 16:38 ` Cristian Marussi
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=20200512170020.GC17648@e119603-lin.cambridge.arm.com \ --to=cristian.marussi@arm.com \ --cc=Dave.Martin@arm.com \ --cc=Jonathan.Cameron@Huawei.com \ --cc=james.quinlan@broadcom.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lukasz.luba@arm.com \ --cc=sudeep.holla@arm.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.