10.12.2021 22:05, Rafael J. Wysocki пишет:
On Fri, Dec 10, 2021 at 7:52 PM Dmitry Osipenko <digetx@xxxxxxxxx> wrote:
10.12.2021 21:19, Rafael J. Wysocki пишет:
...
+bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
+ struct notifier_block *n)
+{
+ unsigned long flags;
+ bool ret;
+
+ spin_lock_irqsave(&nh->lock, flags);
+ ret = notifier_has_unique_priority(&nh->head, n);
+ spin_unlock_irqrestore(&nh->lock, flags);
This only works if the caller can prevent new entries from being added
to the list at this point or if the caller knows that they cannot be
added for some reason, but the kerneldoc doesn't mention this
limitation.
I'll update the comment.
..
+bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
+ struct notifier_block *n)
+{
+ bool ret;
+
+ /*
+ * This code gets used during boot-up, when task switching is
+ * not yet working and interrupts must remain disabled. At such
+ * times we must not call down_read().
+ */
+ if (system_state != SYSTEM_BOOTING)
No, please don't do this, it makes the whole thing error-prone.
What should I do then?
First of all, do you know of any users who may want to call this
during early initialization? If so, then why may they want to do
that?
I'll need to carefully review all those dozens of platform restart
handlers to answer this question.
Depending on the above, I would consider adding a special mechanism for them.
Please notice that every blocking_notifier_*() function has this
SYSTEM_BOOTING check, it's not my invention. Notifier API needs to be
generic.
+ down_read(&nh->rwsem);
+
+ ret = notifier_has_unique_priority(&nh->head, n);
+
+ if (system_state != SYSTEM_BOOTING)
+ up_read(&nh->rwsem);
And still what if a new entry with a non-unique priority is added to
the chain at this point?
If entry with a non-unique priority is added after the check, then
obviously it won't be detected.
Why isn't this a problem?>> I don't understand the question. These
down/up_read() are the locks that prevent the race, if that's the question.
Not really, they only prevent the race from occurring while
notifier_has_unique_priority() is running.
If anyone depends on this check for correctness, they need to lock the
rwsem, do the check, do the thing depending on the check while holding
the rwsem and then release the rwsem. Otherwise it is racy.
It's fine that it's a bit "racy" since in the context of this series. We
always do the check after adding new entry, so it's not a problem.
There are two options:
1. Use blocking_notifier_has_unique_priority() like it's done in this
patchset. Remove it after all drivers are converted to the new API and
add blocking_notifier_chain_register_unique().
2. Add blocking_notifier_chain_register_unique(), but don't let it fail
the registration of non-unique entries until all drivers are converted
to the new API.
