On Wed, May 13, 2020 at 05:46:13PM +0100, Dave Martin wrote:

On Mon, May 11, 2020 at 11:04:03PM +0100, Cristian Marussi wrote:

quoted

Hi Dave

thanks for the review first of all.

[snip]

quoted

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 

scmi_notify_init() {

	if (unlikely(!ni))
		return 0;     /// Stop notification dispatching from ISR
}

So ni and !ni will signify (initialized && enabled) or not.

Can this still race?

It should not race now with a few barriers...V8 is not posted still (and it's a
bit different from the above broken snippet of mine :D)

If teardown is impossible or useless, perhaps it would make sense not to
have the code for it (?)

Well yes in fact I was doubtful from the start if this rarely used code was
needed at all...I added just for consistency since some cleanup happens
elsewhere in SCMI...but...

Do we expect SCMI related drivers to be buildable as modules?

... talking with Sudeep these days around the content of this thread, it seems that
we should be able anyway to modularize cleanly most of the SCMI core due to the
Android GKI thing....so I'm going to review the whole SCMI core and notification
shutdown/cleanup process soon....still I'm not convinced if I'll do anything different
here for notif at that point...as of now I fixed as above the current series foro v8
following your feedback then we'll see if I'll change it more drastically....

quoted

Not really a short explanation...sorry

Sounds like it's not simple :)

What you've said sounds plausible -- I haven't found much time to think
about it this week I'm afraid!

Thanks you for the feedback !

Cheers,

Cristian
 

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel