Messages in this thread | | | Date | Tue, 12 May 2020 18:19:49 +0100 | From | Sudeep Holla <> | Subject | Re: [PATCH v7 1/9] firmware: arm_scmi: Add notification protocol-registration |
| |
On Tue, May 12, 2020 at 06:00:20PM +0100, Cristian Marussi wrote: > 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...) >
We may need this eventually, I just kept initial implementation simple. The scmi_drivers should be module and loading/unloading should be stable and must work today.
Looking at the driver again, I am wondering why haven't I added scmi_device_destroy in scmi_remove. We should be able to add that.
Lastly we can see how to make protocol registration and unregistration as a module.
-- Regards, Sudeep
| |