| Message ID | 20150624170849.5612.79108.stgit@localhost.localdomain |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Neelesh, Comments below. Note that the sync function message queuing functions might go away so it would be best not to rely on them (it's only used in one place at the moment). On Wed, 24 Jun 2015 22:38:59 Neelesh Gupta wrote: > Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com> > --- > core/ipmi.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/core/ipmi.c b/core/ipmi.c > index 9d91c26..bee2987 100644 > --- a/core/ipmi.c > +++ b/core/ipmi.c > @@ -160,13 +160,11 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) > } > > lock(&sync_lock); > - while (sync_msg); This won't work. The loop is there to wait for any outstanding sync message to be sent. > sync_msg = msg; > ipmi_queue_msg(msg); > - unlock(&sync_lock); > - > - while (sync_msg == msg) > + while (sync_msg) > time_wait_ms(100); > + unlock(&sync_lock); You can't call time_wait_ms with a lock held as the pollers won't run so the message will never complete. > } > > static void ipmi_read_event_complete(struct ipmi_msg *msg) > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot >
Hi Alistair, On 06/26/2015 11:21 AM, Alistair Popple wrote: > Hi Neelesh, > > Comments below. Note that the sync function message queuing functions might go > away so it would be best not to rely on them (it's only used in one place at > the moment). > > On Wed, 24 Jun 2015 22:38:59 Neelesh Gupta wrote: >> Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com> >> --- >> core/ipmi.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/core/ipmi.c b/core/ipmi.c >> index 9d91c26..bee2987 100644 >> --- a/core/ipmi.c >> +++ b/core/ipmi.c >> @@ -160,13 +160,11 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) >> } >> >> lock(&sync_lock); >> - while (sync_msg); > This won't work. The loop is there to wait for any outstanding sync message to > be sent. > >> sync_msg = msg; >> ipmi_queue_msg(msg); >> - unlock(&sync_lock); >> - >> - while (sync_msg == msg) >> + while (sync_msg) >> time_wait_ms(100); >> + unlock(&sync_lock); > You can't call time_wait_ms with a lock held as the pollers won't run so the > message will never complete. Got it, also seeing the problem when testing. So, this change is *not* required. Thanks for the review ! Neelesh. > >> } >> >> static void ipmi_read_event_complete(struct ipmi_msg *msg) >> >> _______________________________________________ >> Skiboot mailing list >> Skiboot@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/skiboot >>
diff --git a/core/ipmi.c b/core/ipmi.c index 9d91c26..bee2987 100644 --- a/core/ipmi.c +++ b/core/ipmi.c @@ -160,13 +160,11 @@ void ipmi_queue_msg_sync(struct ipmi_msg *msg) } lock(&sync_lock); - while (sync_msg); sync_msg = msg; ipmi_queue_msg(msg); - unlock(&sync_lock); - - while (sync_msg == msg) + while (sync_msg) time_wait_ms(100); + unlock(&sync_lock); } static void ipmi_read_event_complete(struct ipmi_msg *msg)
Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com> --- core/ipmi.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)