| Message ID | 20180915143926.12870-3-vaibhav@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Enable fast-reboot support for CAPI-2 | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
On 16/9/18 12:39 am, Vaibhav Jain wrote: > Current implementation of opal_del_host_sync_notifier() will only > delete the first entry of the 'notify' callback found from opal_syners > list irrespective of the 'data' of list-node. This is problematic when > multiple notifiers with same callback function but different 'data' > are registered. In this case when the cleanup code will call > opal_del_host_sync_notifier() it cannot be sure if correct opal_syncer > is removed. > > Hence we introduce a new function named > opal_del_host_sync_notifier_with_data() that iterates over the > opal_syncers list and only removes the first node node having the > matching value for 'notify' callback and 'data'. > > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes: > Current implementation of opal_del_host_sync_notifier() will only > delete the first entry of the 'notify' callback found from opal_syners opal_syncers (missing the c)
On 09/15/2018 08:09 PM, Vaibhav Jain wrote: > Current implementation of opal_del_host_sync_notifier() will only > delete the first entry of the 'notify' callback found from opal_syners > list irrespective of the 'data' of list-node. This is problematic when > multiple notifiers with same callback function but different 'data' > are registered. In this case when the cleanup code will call > opal_del_host_sync_notifier() it cannot be sure if correct opal_syncer > is removed. Patch itself looks good to me. But may be its better to convert existing opal_del_host_sync_notifier() itself? Anyway no one is using that interface today and nothing harm is adding additional check for `data` as well. -Vasant
Stewart Smith <stewart@linux.ibm.com> writes: > Vaibhav Jain <vaibhav@linux.ibm.com> writes: >> Current implementation of opal_del_host_sync_notifier() will only >> delete the first entry of the 'notify' callback found from opal_syners > > opal_syncers (missing the c) Thanks Stewart, I will get it fixed in v2.
Thanks for reviewing thie patch Vasant, Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes: > Patch itself looks good to me. But may be its better to convert existing > opal_del_host_sync_notifier() itself? Anyway no one is using that interface > today and nothing > harm is adding additional check for `data` as well. That seems like a better idea. I will address it in v2 I am working on.
diff --git a/core/opal.c b/core/opal.c index 63a08510..4a3b4a61 100644 --- a/core/opal.c +++ b/core/opal.c @@ -687,6 +687,24 @@ void opal_del_host_sync_notifier(bool (*notify)(void *data)) } } + +/* + * Remove a host sync notifier for given callback and data + */ +void opal_del_host_sync_notifier_with_data(bool (*notify)(void *data), + void *data) +{ + struct opal_sync_entry *ent; + + list_for_each(&opal_syncers, ent, link) { + if (ent->notify == notify && ent->data == data) { + list_del(&ent->link); + free(ent); + return; + } + } +} + /* * OPAL call to handle host kexec'ing scenario */ diff --git a/include/opal-internal.h b/include/opal-internal.h index 40bad457..4ed62997 100644 --- a/include/opal-internal.h +++ b/include/opal-internal.h @@ -77,6 +77,8 @@ extern void opal_run_pollers(void); */ extern void opal_add_host_sync_notifier(bool (*notify)(void *data), void *data); extern void opal_del_host_sync_notifier(bool (*notify)(void *data)); +extern void opal_del_host_sync_notifier_with_data(bool (*notify)(void *data), + void *data); /* * Opal internal function prototype
Current implementation of opal_del_host_sync_notifier() will only delete the first entry of the 'notify' callback found from opal_syners list irrespective of the 'data' of list-node. This is problematic when multiple notifiers with same callback function but different 'data' are registered. In this case when the cleanup code will call opal_del_host_sync_notifier() it cannot be sure if correct opal_syncer is removed. Hence we introduce a new function named opal_del_host_sync_notifier_with_data() that iterates over the opal_syncers list and only removes the first node node having the matching value for 'notify' callback and 'data'. Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- core/opal.c | 18 ++++++++++++++++++ include/opal-internal.h | 2 ++ 2 files changed, 20 insertions(+)