subject:"\[PATCH 06\/13\] ACPI\/IPMI\: Add reference counting for ACPI operation region handlers"

> On Friday, July 26, 2013 10:49 PM Rafael J. Wysocki wrote:
> > On Friday, July 26, 2013 01:54:00 AM Zheng, Lv wrote:
> > > On Friday, July 26, 2013 5:29 AM Rafael J. Wysocki wrote:
> > > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > > This patch adds reference couting for ACPI operation region
> > > > handlers to fix races caused by the ACPICA address space callback
> invocations.
> > > >
> > > > ACPICA address space callback invocation is not suitable for Linux
> > > > CONFIG_MODULE=y execution environment.
> > >
> > > Actually, can you please explain to me what *exactly* the problem is?
> >
> > OK.  I'll add race explanations in the next revision.
> >
> > The problem is there is no "lock" held inside ACPICA for invoking
> > operation region handlers.
> > Thus races happens between the
> > acpi_remove/install_address_space_handler and the handler/setup
> callbacks.
> 
> I see.  Now you're trying to introduce something that would prevent those
> races from happening, right?

Yes. Let me explain this later in this email.

> 
> > This is correct per ACPI specification.
> > As if there is interpreter locks held for invoking operation region
> > handlers, the timeout implemented inside the operation region handlers
> > will make all locking facilities (Acquire or Sleep,...) timed out.
> > Please refer to ACPI specification "5.5.2 Control Method Execution":
> > Interpretation of a Control Method is not preemptive, but it can
> > block. When a control method does block, OSPM can initiate or continue
> > the execution of a different control method. A control method can only
> > assume that access to global objects is exclusive for any period the control
> method does not block.
> >
> > So it is pretty much likely that ACPI IO transfers are locked inside
> > the operation region callback implementations.
> > Using locking facility to protect the callback invocation will risk dead 
> > locks.
> 
> No.  If you use a single global lock around all invocations of operation 
> region
> handlers, it won't deadlock, but it will *serialize* things.  This means that
> there won't be two handlers executing in parallel.  That may or may not be
> bad depending on what those handlers actually do.
> 
> Your concern seems to be that if one address space handler is buggy and it
> blocks indefinitely, executing it under such a lock would affect the other 
> address
> space handlers and in my opinion this is a valid concern.

It can be expressed in more detailed ways:

The interpreter runs control methods in the following style according to the 
ACPI spec.
CM1_Enter -> EnterInter -> CM1_Running -> OpRegion1 -> ExitInter -> EnterInter  
  -> CM1_running -> ExitInter -> CM1_Exit
CM2_Enter -> EnterInter ->   -> CM2_Running 
-> OpRegion1 -> ExitInter -> EnterInter   -> CM2_running -> 
ExitInter -> CM2_Exit

EnterInter: Enter interpreter lock
ExitInter: Leave interpreter lock

Let me introduce two situations:

1. If we hold global "mutex" before "EnterInter", then no second control method 
can be run "NotSerialized".
If the CM1 just have some codes waiting for a hardware flag and CM2 can access 
other hardware IOs to trigger this flag, then nothing can happen any longer.
This is a practical bug as what we have already seen in "NotSerialized" marked 
ACPI control methods behave in the interpreter mode executed in serialized way 
- kernel parameter "acpi_serialize".

2. If we hold global "mutex" after "EnterInter" and Before OpRegion1
If we do things this way, then all IO accesses are serialized, if we have 
something in an IPMI operation region failed due to timeout, then any other 
system IOs that should happen in parallel will just happen after 5 seconds.  
This is not an acceptable experience.

> 
> So the idea seems to be to add wrappers around
> acpi_install_address_space_handler()
> and acpi_remove_address_space_handler (but I don't see where the latter is
> called after the change?), such that they will know when it is safe to 
> unregister
> the handler.  That is simple enough.

An obvious bug, it should be put between the while (atomic_read() > 1) block 
and the final atomic_dec().

> However, I'm not sure it is needed in the context of IPMI.

I think I do this just because I need a quick fix to test IPMI bug-fix series.
The issue is highly related to ACPI interpreter design, and codes should be 
implemented inside ACPICA.
And there is not only ACPI_ROOT_OBJECT based address space handlers, but also 
non-ACPI_ROOT_OBJECT based address space handlers, this patch can't protect the 
latter ones.

> Your address space
> handler's context is NULL, so even it if is executed after
> acpi_remove_address_space_handler() has been called for it (or in parallel), 
> it
> doesn't depend on anything passed by the caller, so I don't see why the issue
> can't be addressed by a proper synchronization between
> acpi_ipmi_exit() and acpi_ipmi_space_handler().
>

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

> On Friday, July 26, 2013 10:01 PM Rafael J. Wysocki wrote:
> > On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
> >
> > > On Friday, July 26, 2013 4:27 AM Rafael J. Wysocki wrote:
> > >
> > > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > > This patch adds reference couting for ACPI operation region handlers
> > > > to fix races caused by the ACPICA address space callback invocations.
> > > >
> > > > ACPICA address space callback invocation is not suitable for Linux
> > > > CONFIG_MODULE=y execution environment.  This patch tries to protect
> > > > the address space callbacks by invoking them under a module safe
> > > environment.
> > > > The IPMI address space handler is also upgraded in this patch.
> > > > The acpi_unregister_region() is designed to meet the following
> > > > requirements:
> > > > 1. It acts as a barrier for operation region callbacks - no callback 
> > > > will
> > > >happen after acpi_unregister_region().
> > > > 2. acpi_unregister_region() is safe to be called in moudle->exit()
> > > >functions.
> > > > Using reference counting rather than module referencing allows such
> > > > benefits to be achieved even when acpi_unregister_region() is called
> > > > in the environments other than module->exit().
> > > > The header file of include/acpi/acpi_bus.h should contain the
> > > > declarations that have references to some ACPICA defined types.
> > > >
> > > > Signed-off-by: Lv Zheng 
> > > > Reviewed-by: Huang Ying 
> > > > ---
> > > >  drivers/acpi/acpi_ipmi.c |   16 ++--
> > > >  drivers/acpi/osl.c   |  224
> > > ++
> > > >  include/acpi/acpi_bus.h  |5 ++
> > > >  3 files changed, 235 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > > > 5f8f495..2a09156 100644
> > > > --- a/drivers/acpi/acpi_ipmi.c
> > > > +++ b/drivers/acpi/acpi_ipmi.c
> > > > @@ -539,20 +539,18 @@ out_ref:
> > > >  static int __init acpi_ipmi_init(void)  {
> > > > int result = 0;
> > > > -   acpi_status status;
> > > >
> > > > if (acpi_disabled)
> > > > return result;
> > > >
> > > > mutex_init(_data.ipmi_lock);
> > > >
> > > > -   status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > > > -   ACPI_ADR_SPACE_IPMI,
> > > > -   
> > > > _ipmi_space_handler,
> > > > -   NULL, NULL);
> > > > -   if (ACPI_FAILURE(status)) {
> > > > +   result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
> > > > + _ipmi_space_handler,
> > > > + NULL, NULL);
> > > > +   if (result) {
> > > > pr_warn("Can't register IPMI opregion space handle\n");
> > > > -   return -EINVAL;
> > > > +   return result;
> > > > }
> > > >
> > > > result = ipmi_smi_watcher_register(_data.bmc_events);
> > > > @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
> > > > }
> > > > mutex_unlock(_data.ipmi_lock);
> > > >
> > > > -   acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
> > > > - ACPI_ADR_SPACE_IPMI,
> > > > - _ipmi_space_handler);
> > > > +   acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
> > > >  }
> > > >
> > > >  module_init(acpi_ipmi_init);
> > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > > > 6ab2c35..8398e51 100644
> > > > --- a/drivers/acpi/osl.c
> > > > +++ b/drivers/acpi/osl.c
> > > > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
> static
> > > > struct workqueue_struct *kacpi_notify_wq;  static struct
> > > > workqueue_struct *kacpi_hotplug_wq;
> > > >
> > > > +struct acpi_region {
> > > > +   unsigned long flags;
> > > > +#define ACPI_REGION_DEFAULT0x01
> > > > +#define ACPI_REGION_INSTALLED  0x02
> > > > +#define ACPI_REGION_REGISTERED 0x04
> > > > +#define ACPI_REGION_UNREGISTERING  0x08
> > > > +#define ACPI_REGION_INSTALLING 0x10
> > >
> > > What about (1UL << 1), (1UL << 2) etc.?
> > >
> > > Also please remove the #defines out of the struct definition.
> >
> > OK.
> >
> > >
> > > > +   /*
> > > > +* NOTE: Upgrading All Region Handlers
> > > > +* This flag is only used during the period where not all of the
> > > > +* region handers are upgraded to the new interfaces.
> > > > +*/
> > > > +#define ACPI_REGION_MANAGED0x80
> > > > +   acpi_adr_space_handler handler;
> > > > +   acpi_adr_space_setup setup;
> > > > +   void *context;
> > > > +   /* Invoking references */
> > > > +   atomic_t refcnt;
> > >
> > > Actually, why don't you use krefs?
> >
> > If you take a look at other piece of my codes,

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

 On Friday, July 26, 2013 10:01 PM Rafael J. Wysocki wrote:
  On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
 
   On Friday, July 26, 2013 4:27 AM Rafael J. Wysocki wrote:
  
   On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
This patch adds reference couting for ACPI operation region handlers
to fix races caused by the ACPICA address space callback invocations.
   
ACPICA address space callback invocation is not suitable for Linux
CONFIG_MODULE=y execution environment.  This patch tries to protect
the address space callbacks by invoking them under a module safe
   environment.
The IPMI address space handler is also upgraded in this patch.
The acpi_unregister_region() is designed to meet the following
requirements:
1. It acts as a barrier for operation region callbacks - no callback 
will
   happen after acpi_unregister_region().
2. acpi_unregister_region() is safe to be called in moudle-exit()
   functions.
Using reference counting rather than module referencing allows such
benefits to be achieved even when acpi_unregister_region() is called
in the environments other than module-exit().
The header file of include/acpi/acpi_bus.h should contain the
declarations that have references to some ACPICA defined types.
   
Signed-off-by: Lv Zheng lv.zh...@intel.com
Reviewed-by: Huang Ying ying.hu...@intel.com
---
 drivers/acpi/acpi_ipmi.c |   16 ++--
 drivers/acpi/osl.c   |  224
   ++
 include/acpi/acpi_bus.h  |5 ++
 3 files changed, 235 insertions(+), 10 deletions(-)
   
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
5f8f495..2a09156 100644
--- a/drivers/acpi/acpi_ipmi.c
+++ b/drivers/acpi/acpi_ipmi.c
@@ -539,20 +539,18 @@ out_ref:
 static int __init acpi_ipmi_init(void)  {
int result = 0;
-   acpi_status status;
   
if (acpi_disabled)
return result;
   
mutex_init(driver_data.ipmi_lock);
   
-   status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
-   ACPI_ADR_SPACE_IPMI,
-   
acpi_ipmi_space_handler,
-   NULL, NULL);
-   if (ACPI_FAILURE(status)) {
+   result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
+ acpi_ipmi_space_handler,
+ NULL, NULL);
+   if (result) {
pr_warn(Can't register IPMI opregion space handle\n);
-   return -EINVAL;
+   return result;
}
   
result = ipmi_smi_watcher_register(driver_data.bmc_events);
@@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
}
mutex_unlock(driver_data.ipmi_lock);
   
-   acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
- ACPI_ADR_SPACE_IPMI,
- acpi_ipmi_space_handler);
+   acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
 }
   
 module_init(acpi_ipmi_init);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
6ab2c35..8398e51 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
 static
struct workqueue_struct *kacpi_notify_wq;  static struct
workqueue_struct *kacpi_hotplug_wq;
   
+struct acpi_region {
+   unsigned long flags;
+#define ACPI_REGION_DEFAULT0x01
+#define ACPI_REGION_INSTALLED  0x02
+#define ACPI_REGION_REGISTERED 0x04
+#define ACPI_REGION_UNREGISTERING  0x08
+#define ACPI_REGION_INSTALLING 0x10
  
   What about (1UL  1), (1UL  2) etc.?
  
   Also please remove the #defines out of the struct definition.
 
  OK.
 
  
+   /*
+* NOTE: Upgrading All Region Handlers
+* This flag is only used during the period where not all of the
+* region handers are upgraded to the new interfaces.
+*/
+#define ACPI_REGION_MANAGED0x80
+   acpi_adr_space_handler handler;
+   acpi_adr_space_setup setup;
+   void *context;
+   /* Invoking references */
+   atomic_t refcnt;
  
   Actually, why don't you use krefs?
 
  If you take a look at other piece of my codes, you'll find there are two
 reasons:
 
  1. I'm using while (atomic_read()  1) to implement the objects' flushing 
  and
 there is no kref API to do so.
 
 No, there's not any, but you can read kref.refcount directly, can't you?
 
 Moreover, it is not entirely clear to me that doing the while (atomic_read() 
  1)
 is actually correct.
 
I just think it is not suitable for

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

 On Friday, July 26, 2013 10:49 PM Rafael J. Wysocki wrote:
  On Friday, July 26, 2013 01:54:00 AM Zheng, Lv wrote:
   On Friday, July 26, 2013 5:29 AM Rafael J. Wysocki wrote:
   On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
This patch adds reference couting for ACPI operation region
handlers to fix races caused by the ACPICA address space callback
 invocations.
   
ACPICA address space callback invocation is not suitable for Linux
CONFIG_MODULE=y execution environment.
  
   Actually, can you please explain to me what *exactly* the problem is?
 
  OK.  I'll add race explanations in the next revision.
 
  The problem is there is no lock held inside ACPICA for invoking
  operation region handlers.
  Thus races happens between the
  acpi_remove/install_address_space_handler and the handler/setup
 callbacks.
 
 I see.  Now you're trying to introduce something that would prevent those
 races from happening, right?

Yes. Let me explain this later in this email.

 
  This is correct per ACPI specification.
  As if there is interpreter locks held for invoking operation region
  handlers, the timeout implemented inside the operation region handlers
  will make all locking facilities (Acquire or Sleep,...) timed out.
  Please refer to ACPI specification 5.5.2 Control Method Execution:
  Interpretation of a Control Method is not preemptive, but it can
  block. When a control method does block, OSPM can initiate or continue
  the execution of a different control method. A control method can only
  assume that access to global objects is exclusive for any period the control
 method does not block.
 
  So it is pretty much likely that ACPI IO transfers are locked inside
  the operation region callback implementations.
  Using locking facility to protect the callback invocation will risk dead 
  locks.
 
 No.  If you use a single global lock around all invocations of operation 
 region
 handlers, it won't deadlock, but it will *serialize* things.  This means that
 there won't be two handlers executing in parallel.  That may or may not be
 bad depending on what those handlers actually do.
 
 Your concern seems to be that if one address space handler is buggy and it
 blocks indefinitely, executing it under such a lock would affect the other 
 address
 space handlers and in my opinion this is a valid concern.

It can be expressed in more detailed ways:

The interpreter runs control methods in the following style according to the 
ACPI spec.
CM1_Enter - EnterInter - CM1_Running - OpRegion1 - ExitInter - EnterInter  
  - CM1_running - ExitInter - CM1_Exit
CM2_Enter - EnterInter -   - CM2_Running 
- OpRegion1 - ExitInter - EnterInter   - CM2_running - 
ExitInter - CM2_Exit

EnterInter: Enter interpreter lock
ExitInter: Leave interpreter lock

Let me introduce two situations:

1. If we hold global mutex before EnterInter, then no second control method 
can be run NotSerialized.
If the CM1 just have some codes waiting for a hardware flag and CM2 can access 
other hardware IOs to trigger this flag, then nothing can happen any longer.
This is a practical bug as what we have already seen in NotSerialized marked 
ACPI control methods behave in the interpreter mode executed in serialized way 
- kernel parameter acpi_serialize.

2. If we hold global mutex after EnterInter and Before OpRegion1
If we do things this way, then all IO accesses are serialized, if we have 
something in an IPMI operation region failed due to timeout, then any other 
system IOs that should happen in parallel will just happen after 5 seconds.  
This is not an acceptable experience.

 
 So the idea seems to be to add wrappers around
 acpi_install_address_space_handler()
 and acpi_remove_address_space_handler (but I don't see where the latter is
 called after the change?), such that they will know when it is safe to 
 unregister
 the handler.  That is simple enough.

An obvious bug, it should be put between the while (atomic_read()  1) block 
and the final atomic_dec().

 However, I'm not sure it is needed in the context of IPMI.

I think I do this just because I need a quick fix to test IPMI bug-fix series.
The issue is highly related to ACPI interpreter design, and codes should be 
implemented inside ACPICA.
And there is not only ACPI_ROOT_OBJECT based address space handlers, but also 
non-ACPI_ROOT_OBJECT based address space handlers, this patch can't protect the 
latter ones.

 Your address space
 handler's context is NULL, so even it if is executed after
 acpi_remove_address_space_handler() has been called for it (or in parallel), 
 it
 doesn't depend on anything passed by the caller, so I don't see why the issue
 can't be addressed by a proper synchronization between
 acpi_ipmi_exit() and acpi_ipmi_space_handler().
 
 Clearly, acpi_ipmi_exit() should wait for all already running instances of
 acpi_ipmi_space_handler() to complete and all

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Friday, July 26, 2013 01:54:00 AM Zheng, Lv wrote:
> > From: Rafael J. Wysocki [mailto:r...@sisk.pl]
> > Sent: Friday, July 26, 2013 5:29 AM
> > 
> > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > This patch adds reference couting for ACPI operation region handlers to 
> > > fix
> > > races caused by the ACPICA address space callback invocations.
> > >
> > > ACPICA address space callback invocation is not suitable for Linux
> > > CONFIG_MODULE=y execution environment.
> > 
> > Actually, can you please explain to me what *exactly* the problem is?
> 
> OK.  I'll add race explanations in the next revision.
> 
> The problem is there is no "lock" held inside ACPICA for invoking operation
> region handlers.
> Thus races happens between the acpi_remove/install_address_space_handler and
> the handler/setup callbacks.

I see.  Now you're trying to introduce something that would prevent those
races from happening, right?

> This is correct per ACPI specification.
> As if there is interpreter locks held for invoking operation region handlers,
> the timeout implemented inside the operation region handlers will make all
> locking facilities (Acquire or Sleep,...) timed out.
> Please refer to ACPI specification "5.5.2 Control Method Execution":
> Interpretation of a Control Method is not preemptive, but it can block. When
> a control method does block, OSPM can initiate or continue the execution of
> a different control method. A control method can only assume that access to
> global objects is exclusive for any period the control method does not block.
> 
> So it is pretty much likely that ACPI IO transfers are locked inside the
> operation region callback implementations.
> Using locking facility to protect the callback invocation will risk dead 
> locks.

No.  If you use a single global lock around all invocations of operation region
handlers, it won't deadlock, but it will *serialize* things.  This means that
there won't be two handlers executing in parallel.  That may or may not be bad
depending on what those handlers actually do.

Your concern seems to be that if one address space handler is buggy and it
blocks indefinitely, executing it under such a lock would affect the other
address space handlers and in my opinion this is a valid concern.

So the idea seems to be to add wrappers around 
acpi_install_address_space_handler()
and acpi_remove_address_space_handler (but I don't see where the latter is 
called
after the change?), such that they will know when it is safe to unregister the
handler.  That is simple enough.

However, I'm not sure it is needed in the context of IPMI.  Your address space
handler's context is NULL, so even it if is executed after
acpi_remove_address_space_handler() has been called for it (or in parallel),
it doesn't depend on anything passed by the caller, so I don't see why the
issue can't be addressed by a proper synchronization between
acpi_ipmi_exit() and acpi_ipmi_space_handler().

Clearly, acpi_ipmi_exit() should wait for all already running instances of
acpi_ipmi_space_handler() to complete and all acpi_ipmi_space_handler()
instances started after acpi_ipmi_exit() has been called must return
immediately.

I would imagine an algorithm like this:

acpi_ipmi_exit()
 1. Take "address space handler lock".
 2. Set "unregistering address space handler" flag.
 3. Check if "count of currently running handlers" is 0.  If so,
call acpi_remove_address_space_handler(), drop the lock (possibly clear the
flag) and return.
 4. Otherwise drop the lock and go to sleep in "address space handler wait 
queue".
 5. When woken up, take "address space handler lock" and go to 3.

acpi_ipmi_space_handler()
 1. Take "address space handler lock".
 2. Check "unregistering address space handler" flag.  If set, drop the lock
and return.
 3. Increment "count of currently running handlers".
 4. Drop the lock.
 5. Do your work.
 6. Take "address space handler lock".
 7. Decrement "count of currently running handlers" and if 0, signal the
tasks waiting on it to wake up.
 8. Drop the lock.

Thanks,
Rafael

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
> 
> > From: Rafael J. Wysocki [mailto:r...@sisk.pl]
> > Sent: Friday, July 26, 2013 4:27 AM
> > 
> > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > This patch adds reference couting for ACPI operation region handlers
> > > to fix races caused by the ACPICA address space callback invocations.
> > >
> > > ACPICA address space callback invocation is not suitable for Linux
> > > CONFIG_MODULE=y execution environment.  This patch tries to protect
> > > the address space callbacks by invoking them under a module safe
> > environment.
> > > The IPMI address space handler is also upgraded in this patch.
> > > The acpi_unregister_region() is designed to meet the following
> > > requirements:
> > > 1. It acts as a barrier for operation region callbacks - no callback will
> > >happen after acpi_unregister_region().
> > > 2. acpi_unregister_region() is safe to be called in moudle->exit()
> > >functions.
> > > Using reference counting rather than module referencing allows such
> > > benefits to be achieved even when acpi_unregister_region() is called
> > > in the environments other than module->exit().
> > > The header file of include/acpi/acpi_bus.h should contain the
> > > declarations that have references to some ACPICA defined types.
> > >
> > > Signed-off-by: Lv Zheng 
> > > Reviewed-by: Huang Ying 
> > > ---
> > >  drivers/acpi/acpi_ipmi.c |   16 ++--
> > >  drivers/acpi/osl.c   |  224
> > ++
> > >  include/acpi/acpi_bus.h  |5 ++
> > >  3 files changed, 235 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > > 5f8f495..2a09156 100644
> > > --- a/drivers/acpi/acpi_ipmi.c
> > > +++ b/drivers/acpi/acpi_ipmi.c
> > > @@ -539,20 +539,18 @@ out_ref:
> > >  static int __init acpi_ipmi_init(void)  {
> > >   int result = 0;
> > > - acpi_status status;
> > >
> > >   if (acpi_disabled)
> > >   return result;
> > >
> > >   mutex_init(_data.ipmi_lock);
> > >
> > > - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > > - ACPI_ADR_SPACE_IPMI,
> > > - _ipmi_space_handler,
> > > - NULL, NULL);
> > > - if (ACPI_FAILURE(status)) {
> > > + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
> > > +   _ipmi_space_handler,
> > > +   NULL, NULL);
> > > + if (result) {
> > >   pr_warn("Can't register IPMI opregion space handle\n");
> > > - return -EINVAL;
> > > + return result;
> > >   }
> > >
> > >   result = ipmi_smi_watcher_register(_data.bmc_events);
> > > @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
> > >   }
> > >   mutex_unlock(_data.ipmi_lock);
> > >
> > > - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
> > > -   ACPI_ADR_SPACE_IPMI,
> > > -   _ipmi_space_handler);
> > > + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
> > >  }
> > >
> > >  module_init(acpi_ipmi_init);
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > > 6ab2c35..8398e51 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;  static
> > > struct workqueue_struct *kacpi_notify_wq;  static struct
> > > workqueue_struct *kacpi_hotplug_wq;
> > >
> > > +struct acpi_region {
> > > + unsigned long flags;
> > > +#define ACPI_REGION_DEFAULT  0x01
> > > +#define ACPI_REGION_INSTALLED0x02
> > > +#define ACPI_REGION_REGISTERED   0x04
> > > +#define ACPI_REGION_UNREGISTERING0x08
> > > +#define ACPI_REGION_INSTALLING   0x10
> > 
> > What about (1UL << 1), (1UL << 2) etc.?
> > 
> > Also please remove the #defines out of the struct definition.
> 
> OK.
> 
> > 
> > > + /*
> > > +  * NOTE: Upgrading All Region Handlers
> > > +  * This flag is only used during the period where not all of the
> > > +  * region handers are upgraded to the new interfaces.
> > > +  */
> > > +#define ACPI_REGION_MANAGED  0x80
> > > + acpi_adr_space_handler handler;
> > > + acpi_adr_space_setup setup;
> > > + void *context;
> > > + /* Invoking references */
> > > + atomic_t refcnt;
> > 
> > Actually, why don't you use krefs?
> 
> If you take a look at other piece of my codes, you'll find there are two 
> reasons:
> 
> 1. I'm using while (atomic_read() > 1) to implement the objects' flushing and 
> there is no kref API to do so.

No, there's not any, but you can read kref.refcount directly, can't you?

Moreover, it is not entirely clear to me that doing the while (atomic_read() > 
1)
is actually correct.

>   I just think it is not suitable for me to introduce such an API into kref.h 
> and start another argument around kref designs in this bug

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

> From: linux-acpi-ow...@vger.kernel.org
> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Friday, July 26, 2013 9:54 AM
> To: Rafael J. Wysocki
> Cc: Wysocki, Rafael J; Brown, Len; linux-kernel@vger.kernel.org;
> linux-a...@vger.kernel.org
> Subject: RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI
> operation region handlers
> 
> > From: Rafael J. Wysocki [mailto:r...@sisk.pl]
> > Sent: Friday, July 26, 2013 5:29 AM
> >
> > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > This patch adds reference couting for ACPI operation region handlers
> > > to fix races caused by the ACPICA address space callback invocations.
> > >
> > > ACPICA address space callback invocation is not suitable for Linux
> > > CONFIG_MODULE=y execution environment.
> >
> > Actually, can you please explain to me what *exactly* the problem is?
> 
> OK.  I'll add race explanations in the next revision.
> 
> The problem is there is no "lock" held inside ACPICA for invoking operation
> region handlers.
> Thus races happens between the acpi_remove/install_address_space_handler
> and the handler/setup callbacks.

This seems not a good explanation of the intent of this patch.
I think the intent is here in the patch description:

1. It acts as a barrier for operation region callbacks - no callback will
   happen after acpi_unregister_region().
2. acpi_unregister_region() is safe to be called in moudle->exit()
   functions.

Hmm, maybe I need to re-order the patch description for this patch.

Thanks for commenting.

Best regards
-Lv

> 
> This is correct per ACPI specification.
> As if there is interpreter locks held for invoking operation region handlers, 
> the
> timeout implemented inside the operation region handlers will make all locking
> facilities (Acquire or Sleep,...) timed out.
> Please refer to ACPI specification "5.5.2 Control Method Execution":
> Interpretation of a Control Method is not preemptive, but it can block. When a
> control method does block, OSPM can initiate or continue the execution of a
> different control method. A control method can only assume that access to
> global objects is exclusive for any period the control method does not block.
> 
> So it is pretty much likely that ACPI IO transfers are locked inside the 
> operation
> region callback implementations.
> Using locking facility to protect the callback invocation will risk dead 
> locks.
> 
> Thanks
> -Lv
> 
> > Rafael
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

> From: linux-acpi-ow...@vger.kernel.org
> [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
> Sent: Friday, July 26, 2013 8:48 AM
> 
> 
> 
> > From: Rafael J. Wysocki [mailto:r...@sisk.pl]
> > Sent: Friday, July 26, 2013 4:27 AM
> >
> > On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > > This patch adds reference couting for ACPI operation region handlers
> > > to fix races caused by the ACPICA address space callback invocations.
> > >
> > > ACPICA address space callback invocation is not suitable for Linux
> > > CONFIG_MODULE=y execution environment.  This patch tries to protect
> > > the address space callbacks by invoking them under a module safe
> > environment.
> > > The IPMI address space handler is also upgraded in this patch.
> > > The acpi_unregister_region() is designed to meet the following
> > > requirements:
> > > 1. It acts as a barrier for operation region callbacks - no callback will
> > >happen after acpi_unregister_region().
> > > 2. acpi_unregister_region() is safe to be called in moudle->exit()
> > >functions.
> > > Using reference counting rather than module referencing allows such
> > > benefits to be achieved even when acpi_unregister_region() is called
> > > in the environments other than module->exit().
> > > The header file of include/acpi/acpi_bus.h should contain the
> > > declarations that have references to some ACPICA defined types.
> > >
> > > Signed-off-by: Lv Zheng 
> > > Reviewed-by: Huang Ying 
> > > ---
> > >  drivers/acpi/acpi_ipmi.c |   16 ++--
> > >  drivers/acpi/osl.c   |  224
> > ++
> > >  include/acpi/acpi_bus.h  |5 ++
> > >  3 files changed, 235 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> > > index
> > > 5f8f495..2a09156 100644
> > > --- a/drivers/acpi/acpi_ipmi.c
> > > +++ b/drivers/acpi/acpi_ipmi.c
> > > @@ -539,20 +539,18 @@ out_ref:
> > >  static int __init acpi_ipmi_init(void)  {
> > >   int result = 0;
> > > - acpi_status status;
> > >
> > >   if (acpi_disabled)
> > >   return result;
> > >
> > >   mutex_init(_data.ipmi_lock);
> > >
> > > - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > > - ACPI_ADR_SPACE_IPMI,
> > > - _ipmi_space_handler,
> > > - NULL, NULL);
> > > - if (ACPI_FAILURE(status)) {
> > > + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
> > > +   _ipmi_space_handler,
> > > +   NULL, NULL);
> > > + if (result) {
> > >   pr_warn("Can't register IPMI opregion space handle\n");
> > > - return -EINVAL;
> > > + return result;
> > >   }
> > >
> > >   result = ipmi_smi_watcher_register(_data.bmc_events);
> > > @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
> > >   }
> > >   mutex_unlock(_data.ipmi_lock);
> > >
> > > - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
> > > -   ACPI_ADR_SPACE_IPMI,
> > > -   _ipmi_space_handler);
> > > + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
> > >  }
> > >
> > >  module_init(acpi_ipmi_init);
> > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > > 6ab2c35..8398e51 100644
> > > --- a/drivers/acpi/osl.c
> > > +++ b/drivers/acpi/osl.c
> > > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
> > > static struct workqueue_struct *kacpi_notify_wq;  static struct
> > > workqueue_struct *kacpi_hotplug_wq;
> > >
> > > +struct acpi_region {
> > > + unsigned long flags;
> > > +#define ACPI_REGION_DEFAULT  0x01
> > > +#define ACPI_REGION_INSTALLED0x02
> > > +#define ACPI_REGION_REGISTERED   0x04
> > > +#define ACPI_REGION_UNREGISTERING0x08
> > > +#define ACPI_REGION_INSTALLING   0x10
> >
> > What about (1UL << 1), (1UL << 2) etc.?
> >
> > Also please remove the #defines out of the struct definition.
> 
> OK.
> 
> >
> > > + /*
> > > +  * NOTE: Upgrading All Region Handlers
> > > +  * This flag is only used during the period where not all of the
> > > +  * region handers are upgraded to the new interfaces.
> > > +  */
> > > +#define ACPI_REGION_MANAGED  0x80
> > > + acpi_adr_space_handler handler;
> > > + acpi_adr_space_setup setup;
> > > + void *context;
> > > + /* Invoking references */
> > > + atomic_t refcnt;
> >
> > Actually, why don't you use krefs?
> 
> If you take a look at other piece of my codes, you'll find there are two 
> reasons:
> 
> 1. I'm using while (atomic_read() > 1) to implement the objects' flushing and
> there is no kref API to do so.
>   I just think it is not suitable for me to introduce such an API into kref.h 
> and
> start another argument around kref designs in this bug fix patch. :-)
>   I'll start a discussion about kref design using another thread.

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

 From: linux-acpi-ow...@vger.kernel.org
 [mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
 Sent: Friday, July 26, 2013 8:48 AM

  From: Rafael J. Wysocki [mailto:r...@sisk.pl]
  Sent: Friday, July 26, 2013 4:27 AM

  On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
   This patch adds reference couting for ACPI operation region handlers
   to fix races caused by the ACPICA address space callback invocations.

   ACPICA address space callback invocation is not suitable for Linux
   CONFIG_MODULE=y execution environment.  This patch tries to protect
   the address space callbacks by invoking them under a module safe
  environment.
   The IPMI address space handler is also upgraded in this patch.
   The acpi_unregister_region() is designed to meet the following
   requirements:
   1. It acts as a barrier for operation region callbacks - no callback will
  happen after acpi_unregister_region().
   2. acpi_unregister_region() is safe to be called in moudle-exit()
  functions.
   Using reference counting rather than module referencing allows such
   benefits to be achieved even when acpi_unregister_region() is called
   in the environments other than module-exit().
   The header file of include/acpi/acpi_bus.h should contain the
   declarations that have references to some ACPICA defined types.

   Signed-off-by: Lv Zheng lv.zh...@intel.com
   Reviewed-by: Huang Ying ying.hu...@intel.com
   ---
drivers/acpi/acpi_ipmi.c |   16 ++--
drivers/acpi/osl.c   |  224
  ++
include/acpi/acpi_bus.h  |5 ++
3 files changed, 235 insertions(+), 10 deletions(-)

   diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
   index
   5f8f495..2a09156 100644
   --- a/drivers/acpi/acpi_ipmi.c
   +++ b/drivers/acpi/acpi_ipmi.c
   @@ -539,20 +539,18 @@ out_ref:
static int __init acpi_ipmi_init(void)  {
 int result = 0;
   - acpi_status status;

 if (acpi_disabled)
 return result;

 mutex_init(driver_data.ipmi_lock);

   - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
   - ACPI_ADR_SPACE_IPMI,
   - acpi_ipmi_space_handler,
   - NULL, NULL);
   - if (ACPI_FAILURE(status)) {
   + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
   +   acpi_ipmi_space_handler,
   +   NULL, NULL);
   + if (result) {
 pr_warn(Can't register IPMI opregion space handle\n);
   - return -EINVAL;
   + return result;
 }

 result = ipmi_smi_watcher_register(driver_data.bmc_events);
   @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
 }
 mutex_unlock(driver_data.ipmi_lock);

   - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
   -   ACPI_ADR_SPACE_IPMI,
   -   acpi_ipmi_space_handler);
   + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
}

module_init(acpi_ipmi_init);
   diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
   6ab2c35..8398e51 100644
   --- a/drivers/acpi/osl.c
   +++ b/drivers/acpi/osl.c
   @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
   static struct workqueue_struct *kacpi_notify_wq;  static struct
   workqueue_struct *kacpi_hotplug_wq;

   +struct acpi_region {
   + unsigned long flags;
   +#define ACPI_REGION_DEFAULT  0x01
   +#define ACPI_REGION_INSTALLED0x02
   +#define ACPI_REGION_REGISTERED   0x04
   +#define ACPI_REGION_UNREGISTERING0x08
   +#define ACPI_REGION_INSTALLING   0x10

  What about (1UL  1), (1UL  2) etc.?

  Also please remove the #defines out of the struct definition.

 OK.

   + /*
   +  * NOTE: Upgrading All Region Handlers
   +  * This flag is only used during the period where not all of the
   +  * region handers are upgraded to the new interfaces.
   +  */
   +#define ACPI_REGION_MANAGED  0x80
   + acpi_adr_space_handler handler;
   + acpi_adr_space_setup setup;
   + void *context;
   + /* Invoking references */
   + atomic_t refcnt;

  Actually, why don't you use krefs?

 If you take a look at other piece of my codes, you'll find there are two 
 reasons:

 1. I'm using while (atomic_read()  1) to implement the objects' flushing and
 there is no kref API to do so.
   I just think it is not suitable for me to introduce such an API into kref.h 
 and
 start another argument around kref designs in this bug fix patch. :-)
   I'll start a discussion about kref design using another thread.
 2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's 
 kind
 of atomic_t coding style.
   If atomic_t is changed to struct kref, I will need to implement two API,
 __ipmi_dev_release() to take a struct kref as parameter and call

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

From: linux-acpi-ow...@vger.kernel.org
[mailto:linux-acpi-ow...@vger.kernel.org] On Behalf Of Zheng, Lv
Sent: Friday, July 26, 2013 9:54 AM
To: Rafael J. Wysocki
Cc: Wysocki, Rafael J; Brown, Len; linux-kernel@vger.kernel.org;
linux-a...@vger.kernel.org
Subject: RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI
operation region handlers

From: Rafael J. Wysocki [mailto:r...@sisk.pl]
Sent: Friday, July 26, 2013 5:29 AM

On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
This patch adds reference couting for ACPI operation region handlers
to fix races caused by the ACPICA address space callback invocations.

ACPICA address space callback invocation is not suitable for Linux
CONFIG_MODULE=y execution environment.

Actually, can you please explain to me what *exactly* the problem is?

OK. I'll add race explanations in the next revision.

The problem is there is no lock held inside ACPICA for invoking operation
region handlers.
Thus races happens between the acpi_remove/install_address_space_handler
and the handler/setup callbacks.

This seems not a good explanation of the intent of this patch.
I think the intent is here in the patch description:

1. It acts as a barrier for operation region callbacks - no callback will
happen after acpi_unregister_region().
2. acpi_unregister_region() is safe to be called in moudle-exit()
functions.

Hmm, maybe I need to re-order the patch description for this patch.

Thanks for commenting.

Best regards
-Lv

This is correct per ACPI specification.
As if there is interpreter locks held for invoking operation region handlers,
the
timeout implemented inside the operation region handlers will make all locking
facilities (Acquire or Sleep,...) timed out.
Please refer to ACPI specification 5.5.2 Control Method Execution:
Interpretation of a Control Method is not preemptive, but it can block. When a
control method does block, OSPM can initiate or continue the execution of a
different control method. A control method can only assume that access to
global objects is exclusive for any period the control method does not block.

So it is pretty much likely that ACPI IO transfers are locked inside the
operation
region callback implementations.
Using locking facility to protect the callback invocation will risk dead
locks.

Thanks
-Lv

Rafael
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Friday, July 26, 2013 12:47:44 AM Zheng, Lv wrote:
 
  From: Rafael J. Wysocki [mailto:r...@sisk.pl]
  Sent: Friday, July 26, 2013 4:27 AM
  
  On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
   This patch adds reference couting for ACPI operation region handlers
   to fix races caused by the ACPICA address space callback invocations.
  
   ACPICA address space callback invocation is not suitable for Linux
   CONFIG_MODULE=y execution environment.  This patch tries to protect
   the address space callbacks by invoking them under a module safe
  environment.
   The IPMI address space handler is also upgraded in this patch.
   The acpi_unregister_region() is designed to meet the following
   requirements:
   1. It acts as a barrier for operation region callbacks - no callback will
  happen after acpi_unregister_region().
   2. acpi_unregister_region() is safe to be called in moudle-exit()
  functions.
   Using reference counting rather than module referencing allows such
   benefits to be achieved even when acpi_unregister_region() is called
   in the environments other than module-exit().
   The header file of include/acpi/acpi_bus.h should contain the
   declarations that have references to some ACPICA defined types.
  
   Signed-off-by: Lv Zheng lv.zh...@intel.com
   Reviewed-by: Huang Ying ying.hu...@intel.com
   ---
drivers/acpi/acpi_ipmi.c |   16 ++--
drivers/acpi/osl.c   |  224
  ++
include/acpi/acpi_bus.h  |5 ++
3 files changed, 235 insertions(+), 10 deletions(-)
  
   diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
   5f8f495..2a09156 100644
   --- a/drivers/acpi/acpi_ipmi.c
   +++ b/drivers/acpi/acpi_ipmi.c
   @@ -539,20 +539,18 @@ out_ref:
static int __init acpi_ipmi_init(void)  {
 int result = 0;
   - acpi_status status;
  
 if (acpi_disabled)
 return result;
  
 mutex_init(driver_data.ipmi_lock);
  
   - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
   - ACPI_ADR_SPACE_IPMI,
   - acpi_ipmi_space_handler,
   - NULL, NULL);
   - if (ACPI_FAILURE(status)) {
   + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
   +   acpi_ipmi_space_handler,
   +   NULL, NULL);
   + if (result) {
 pr_warn(Can't register IPMI opregion space handle\n);
   - return -EINVAL;
   + return result;
 }
  
 result = ipmi_smi_watcher_register(driver_data.bmc_events);
   @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
 }
 mutex_unlock(driver_data.ipmi_lock);
  
   - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
   -   ACPI_ADR_SPACE_IPMI,
   -   acpi_ipmi_space_handler);
   + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
}
  
module_init(acpi_ipmi_init);
   diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
   6ab2c35..8398e51 100644
   --- a/drivers/acpi/osl.c
   +++ b/drivers/acpi/osl.c
   @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;  static
   struct workqueue_struct *kacpi_notify_wq;  static struct
   workqueue_struct *kacpi_hotplug_wq;
  
   +struct acpi_region {
   + unsigned long flags;
   +#define ACPI_REGION_DEFAULT  0x01
   +#define ACPI_REGION_INSTALLED0x02
   +#define ACPI_REGION_REGISTERED   0x04
   +#define ACPI_REGION_UNREGISTERING0x08
   +#define ACPI_REGION_INSTALLING   0x10
  
  What about (1UL  1), (1UL  2) etc.?
  
  Also please remove the #defines out of the struct definition.
 
 OK.
 
  
   + /*
   +  * NOTE: Upgrading All Region Handlers
   +  * This flag is only used during the period where not all of the
   +  * region handers are upgraded to the new interfaces.
   +  */
   +#define ACPI_REGION_MANAGED  0x80
   + acpi_adr_space_handler handler;
   + acpi_adr_space_setup setup;
   + void *context;
   + /* Invoking references */
   + atomic_t refcnt;
  
  Actually, why don't you use krefs?
 
 If you take a look at other piece of my codes, you'll find there are two 
 reasons:
 
 1. I'm using while (atomic_read()  1) to implement the objects' flushing and 
 there is no kref API to do so.

No, there's not any, but you can read kref.refcount directly, can't you?

Moreover, it is not entirely clear to me that doing the while (atomic_read()  
1)
is actually correct.

   I just think it is not suitable for me to introduce such an API into kref.h 
 and start another argument around kref designs in this bug fix patch. :-)
   I'll start a discussion about kref design using another thread.

You don't need to do that at all.

 2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's 
 kind of atomic_t coding style.
   If atomic_t is changed to

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Friday, July 26, 2013 01:54:00 AM Zheng, Lv wrote:
  From: Rafael J. Wysocki [mailto:r...@sisk.pl]
  Sent: Friday, July 26, 2013 5:29 AM
  
  On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
   This patch adds reference couting for ACPI operation region handlers to 
   fix
   races caused by the ACPICA address space callback invocations.
  
   ACPICA address space callback invocation is not suitable for Linux
   CONFIG_MODULE=y execution environment.
  
  Actually, can you please explain to me what *exactly* the problem is?
 
 OK.  I'll add race explanations in the next revision.
 
 The problem is there is no lock held inside ACPICA for invoking operation
 region handlers.
 Thus races happens between the acpi_remove/install_address_space_handler and
 the handler/setup callbacks.

I see.  Now you're trying to introduce something that would prevent those
races from happening, right?

 This is correct per ACPI specification.
 As if there is interpreter locks held for invoking operation region handlers,
 the timeout implemented inside the operation region handlers will make all
 locking facilities (Acquire or Sleep,...) timed out.
 Please refer to ACPI specification 5.5.2 Control Method Execution:
 Interpretation of a Control Method is not preemptive, but it can block. When
 a control method does block, OSPM can initiate or continue the execution of
 a different control method. A control method can only assume that access to
 global objects is exclusive for any period the control method does not block.
 
 So it is pretty much likely that ACPI IO transfers are locked inside the
 operation region callback implementations.
 Using locking facility to protect the callback invocation will risk dead 
 locks.

No.  If you use a single global lock around all invocations of operation region
handlers, it won't deadlock, but it will *serialize* things.  This means that
there won't be two handlers executing in parallel.  That may or may not be bad
depending on what those handlers actually do.

Your concern seems to be that if one address space handler is buggy and it
blocks indefinitely, executing it under such a lock would affect the other
address space handlers and in my opinion this is a valid concern.

So the idea seems to be to add wrappers around 
acpi_install_address_space_handler()
and acpi_remove_address_space_handler (but I don't see where the latter is 
called
after the change?), such that they will know when it is safe to unregister the
handler.  That is simple enough.

However, I'm not sure it is needed in the context of IPMI.  Your address space
handler's context is NULL, so even it if is executed after
acpi_remove_address_space_handler() has been called for it (or in parallel),
it doesn't depend on anything passed by the caller, so I don't see why the
issue can't be addressed by a proper synchronization between
acpi_ipmi_exit() and acpi_ipmi_space_handler().

Clearly, acpi_ipmi_exit() should wait for all already running instances of
acpi_ipmi_space_handler() to complete and all acpi_ipmi_space_handler()
instances started after acpi_ipmi_exit() has been called must return
immediately.

I would imagine an algorithm like this:

acpi_ipmi_exit()
 1. Take address space handler lock.
 2. Set unregistering address space handler flag.
 3. Check if count of currently running handlers is 0.  If so,
call acpi_remove_address_space_handler(), drop the lock (possibly clear the
flag) and return.
 4. Otherwise drop the lock and go to sleep in address space handler wait 
queue.
 5. When woken up, take address space handler lock and go to 3.

acpi_ipmi_space_handler()
 1. Take address space handler lock.
 2. Check unregistering address space handler flag.  If set, drop the lock
and return.
 3. Increment count of currently running handlers.
 4. Drop the lock.
 5. Do your work.
 6. Take address space handler lock.
 7. Decrement count of currently running handlers and if 0, signal the
tasks waiting on it to wake up.
 8. Drop the lock.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

> From: Rafael J. Wysocki [mailto:r...@sisk.pl]
> Sent: Friday, July 26, 2013 5:29 AM
> 
> On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > This patch adds reference couting for ACPI operation region handlers to fix
> > races caused by the ACPICA address space callback invocations.
> >
> > ACPICA address space callback invocation is not suitable for Linux
> > CONFIG_MODULE=y execution environment.
> 
> Actually, can you please explain to me what *exactly* the problem is?

OK.  I'll add race explanations in the next revision.

The problem is there is no "lock" held inside ACPICA for invoking operation 
region handlers.
Thus races happens between the acpi_remove/install_address_space_handler and 
the handler/setup callbacks.

This is correct per ACPI specification.
As if there is interpreter locks held for invoking operation region handlers, 
the timeout implemented inside the operation region handlers will make all 
locking facilities (Acquire or Sleep,...) timed out.
Please refer to ACPI specification "5.5.2 Control Method Execution":
Interpretation of a Control Method is not preemptive, but it can block. When a 
control method does block, OSPM can initiate or continue the execution of a 
different control method. A control method can only assume that access to 
global objects is exclusive for any period the control method does not block.

So it is pretty much likely that ACPI IO transfers are locked inside the 
operation region callback implementations.
Using locking facility to protect the callback invocation will risk dead locks.

Thanks
-Lv

> Rafael

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers



> From: Rafael J. Wysocki [mailto:r...@sisk.pl]
> Sent: Friday, July 26, 2013 4:27 AM
> 
> On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> > This patch adds reference couting for ACPI operation region handlers
> > to fix races caused by the ACPICA address space callback invocations.
> >
> > ACPICA address space callback invocation is not suitable for Linux
> > CONFIG_MODULE=y execution environment.  This patch tries to protect
> > the address space callbacks by invoking them under a module safe
> environment.
> > The IPMI address space handler is also upgraded in this patch.
> > The acpi_unregister_region() is designed to meet the following
> > requirements:
> > 1. It acts as a barrier for operation region callbacks - no callback will
> >happen after acpi_unregister_region().
> > 2. acpi_unregister_region() is safe to be called in moudle->exit()
> >functions.
> > Using reference counting rather than module referencing allows such
> > benefits to be achieved even when acpi_unregister_region() is called
> > in the environments other than module->exit().
> > The header file of include/acpi/acpi_bus.h should contain the
> > declarations that have references to some ACPICA defined types.
> >
> > Signed-off-by: Lv Zheng 
> > Reviewed-by: Huang Ying 
> > ---
> >  drivers/acpi/acpi_ipmi.c |   16 ++--
> >  drivers/acpi/osl.c   |  224
> ++
> >  include/acpi/acpi_bus.h  |5 ++
> >  3 files changed, 235 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> > 5f8f495..2a09156 100644
> > --- a/drivers/acpi/acpi_ipmi.c
> > +++ b/drivers/acpi/acpi_ipmi.c
> > @@ -539,20 +539,18 @@ out_ref:
> >  static int __init acpi_ipmi_init(void)  {
> > int result = 0;
> > -   acpi_status status;
> >
> > if (acpi_disabled)
> > return result;
> >
> > mutex_init(_data.ipmi_lock);
> >
> > -   status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> > -   ACPI_ADR_SPACE_IPMI,
> > -   _ipmi_space_handler,
> > -   NULL, NULL);
> > -   if (ACPI_FAILURE(status)) {
> > +   result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
> > + _ipmi_space_handler,
> > + NULL, NULL);
> > +   if (result) {
> > pr_warn("Can't register IPMI opregion space handle\n");
> > -   return -EINVAL;
> > +   return result;
> > }
> >
> > result = ipmi_smi_watcher_register(_data.bmc_events);
> > @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
> > }
> > mutex_unlock(_data.ipmi_lock);
> >
> > -   acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
> > - ACPI_ADR_SPACE_IPMI,
> > - _ipmi_space_handler);
> > +   acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
> >  }
> >
> >  module_init(acpi_ipmi_init);
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
> > 6ab2c35..8398e51 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;  static
> > struct workqueue_struct *kacpi_notify_wq;  static struct
> > workqueue_struct *kacpi_hotplug_wq;
> >
> > +struct acpi_region {
> > +   unsigned long flags;
> > +#define ACPI_REGION_DEFAULT0x01
> > +#define ACPI_REGION_INSTALLED  0x02
> > +#define ACPI_REGION_REGISTERED 0x04
> > +#define ACPI_REGION_UNREGISTERING  0x08
> > +#define ACPI_REGION_INSTALLING 0x10
> 
> What about (1UL << 1), (1UL << 2) etc.?
> 
> Also please remove the #defines out of the struct definition.

OK.

> 
> > +   /*
> > +* NOTE: Upgrading All Region Handlers
> > +* This flag is only used during the period where not all of the
> > +* region handers are upgraded to the new interfaces.
> > +*/
> > +#define ACPI_REGION_MANAGED0x80
> > +   acpi_adr_space_handler handler;
> > +   acpi_adr_space_setup setup;
> > +   void *context;
> > +   /* Invoking references */
> > +   atomic_t refcnt;
> 
> Actually, why don't you use krefs?

If you take a look at other piece of my codes, you'll find there are two 
reasons:

1. I'm using while (atomic_read() > 1) to implement the objects' flushing and 
there is no kref API to do so.
  I just think it is not suitable for me to introduce such an API into kref.h 
and start another argument around kref designs in this bug fix patch. :-)
  I'll start a discussion about kref design using another thread.
2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's 
kind of atomic_t coding style.
  If atomic_t is changed to struct kref, I will need to implement two API, 
__ipmi_dev_release() to take a struct kref as parameter and call 
ipmi_dev_release inside it.
  By not using kref, I needn't

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> This patch adds reference couting for ACPI operation region handlers to fix
> races caused by the ACPICA address space callback invocations.
> 
> ACPICA address space callback invocation is not suitable for Linux
> CONFIG_MODULE=y execution environment.

Actually, can you please explain to me what *exactly* the problem is?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
> This patch adds reference couting for ACPI operation region handlers to fix
> races caused by the ACPICA address space callback invocations.
> 
> ACPICA address space callback invocation is not suitable for Linux
> CONFIG_MODULE=y execution environment.  This patch tries to protect the
> address space callbacks by invoking them under a module safe environment.
> The IPMI address space handler is also upgraded in this patch.
> The acpi_unregister_region() is designed to meet the following
> requirements:
> 1. It acts as a barrier for operation region callbacks - no callback will
>happen after acpi_unregister_region().
> 2. acpi_unregister_region() is safe to be called in moudle->exit()
>functions.
> Using reference counting rather than module referencing allows
> such benefits to be achieved even when acpi_unregister_region() is called
> in the environments other than module->exit().
> The header file of include/acpi/acpi_bus.h should contain the declarations
> that have references to some ACPICA defined types.
> 
> Signed-off-by: Lv Zheng 
> Reviewed-by: Huang Ying 
> ---
>  drivers/acpi/acpi_ipmi.c |   16 ++--
>  drivers/acpi/osl.c   |  224 
> ++
>  include/acpi/acpi_bus.h  |5 ++
>  3 files changed, 235 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> index 5f8f495..2a09156 100644
> --- a/drivers/acpi/acpi_ipmi.c
> +++ b/drivers/acpi/acpi_ipmi.c
> @@ -539,20 +539,18 @@ out_ref:
>  static int __init acpi_ipmi_init(void)
>  {
>   int result = 0;
> - acpi_status status;
>  
>   if (acpi_disabled)
>   return result;
>  
>   mutex_init(_data.ipmi_lock);
>  
> - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
> - ACPI_ADR_SPACE_IPMI,
> - _ipmi_space_handler,
> - NULL, NULL);
> - if (ACPI_FAILURE(status)) {
> + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
> +   _ipmi_space_handler,
> +   NULL, NULL);
> + if (result) {
>   pr_warn("Can't register IPMI opregion space handle\n");
> - return -EINVAL;
> + return result;
>   }
>  
>   result = ipmi_smi_watcher_register(_data.bmc_events);
> @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
>   }
>   mutex_unlock(_data.ipmi_lock);
>  
> - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
> -   ACPI_ADR_SPACE_IPMI,
> -   _ipmi_space_handler);
> + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
>  }
>  
>  module_init(acpi_ipmi_init);
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 6ab2c35..8398e51 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
>  static struct workqueue_struct *kacpi_notify_wq;
>  static struct workqueue_struct *kacpi_hotplug_wq;
>  
> +struct acpi_region {
> + unsigned long flags;
> +#define ACPI_REGION_DEFAULT  0x01
> +#define ACPI_REGION_INSTALLED0x02
> +#define ACPI_REGION_REGISTERED   0x04
> +#define ACPI_REGION_UNREGISTERING0x08
> +#define ACPI_REGION_INSTALLING   0x10

What about (1UL << 1), (1UL << 2) etc.?

Also please remove the #defines out of the struct definition.

> + /*
> +  * NOTE: Upgrading All Region Handlers
> +  * This flag is only used during the period where not all of the
> +  * region handers are upgraded to the new interfaces.
> +  */
> +#define ACPI_REGION_MANAGED  0x80
> + acpi_adr_space_handler handler;
> + acpi_adr_space_setup setup;
> + void *context;
> + /* Invoking references */
> + atomic_t refcnt;

Actually, why don't you use krefs?

> +};
> +
> +static struct acpi_region acpi_regions[ACPI_NUM_PREDEFINED_REGIONS] = {
> + [ACPI_ADR_SPACE_SYSTEM_MEMORY] = {
> + .flags = ACPI_REGION_DEFAULT,
> + },
> + [ACPI_ADR_SPACE_SYSTEM_IO] = {
> + .flags = ACPI_REGION_DEFAULT,
> + },
> + [ACPI_ADR_SPACE_PCI_CONFIG] = {
> + .flags = ACPI_REGION_DEFAULT,
> + },
> + [ACPI_ADR_SPACE_IPMI] = {
> + .flags = ACPI_REGION_MANAGED,
> + },
> +};
> +static DEFINE_MUTEX(acpi_mutex_region);
> +
>  /*
>   * This list of permanent mappings is for memory that may be accessed from
>   * interrupt context, where we can't do the ioremap().
> @@ -1799,3 +1835,191 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, 
> void *context,
>   kfree(hp_work);
>  }
>  EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
> +
> +static bool acpi_region_managed(struct acpi_region *rgn)
> +{
> + /*
> +  * NOTE:

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
 This patch adds reference couting for ACPI operation region handlers to fix
 races caused by the ACPICA address space callback invocations.
 
 ACPICA address space callback invocation is not suitable for Linux
 CONFIG_MODULE=y execution environment.  This patch tries to protect the
 address space callbacks by invoking them under a module safe environment.
 The IPMI address space handler is also upgraded in this patch.
 The acpi_unregister_region() is designed to meet the following
 requirements:
 1. It acts as a barrier for operation region callbacks - no callback will
happen after acpi_unregister_region().
 2. acpi_unregister_region() is safe to be called in moudle-exit()
functions.
 Using reference counting rather than module referencing allows
 such benefits to be achieved even when acpi_unregister_region() is called
 in the environments other than module-exit().
 The header file of include/acpi/acpi_bus.h should contain the declarations
 that have references to some ACPICA defined types.
 
 Signed-off-by: Lv Zheng lv.zh...@intel.com
 Reviewed-by: Huang Ying ying.hu...@intel.com
 ---
  drivers/acpi/acpi_ipmi.c |   16 ++--
  drivers/acpi/osl.c   |  224 
 ++
  include/acpi/acpi_bus.h  |5 ++
  3 files changed, 235 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
 index 5f8f495..2a09156 100644
 --- a/drivers/acpi/acpi_ipmi.c
 +++ b/drivers/acpi/acpi_ipmi.c
 @@ -539,20 +539,18 @@ out_ref:
  static int __init acpi_ipmi_init(void)
  {
   int result = 0;
 - acpi_status status;
  
   if (acpi_disabled)
   return result;
  
   mutex_init(driver_data.ipmi_lock);
  
 - status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
 - ACPI_ADR_SPACE_IPMI,
 - acpi_ipmi_space_handler,
 - NULL, NULL);
 - if (ACPI_FAILURE(status)) {
 + result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
 +   acpi_ipmi_space_handler,
 +   NULL, NULL);
 + if (result) {
   pr_warn(Can't register IPMI opregion space handle\n);
 - return -EINVAL;
 + return result;
   }
  
   result = ipmi_smi_watcher_register(driver_data.bmc_events);
 @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
   }
   mutex_unlock(driver_data.ipmi_lock);
  
 - acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
 -   ACPI_ADR_SPACE_IPMI,
 -   acpi_ipmi_space_handler);
 + acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
  }
  
  module_init(acpi_ipmi_init);
 diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
 index 6ab2c35..8398e51 100644
 --- a/drivers/acpi/osl.c
 +++ b/drivers/acpi/osl.c
 @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;
  static struct workqueue_struct *kacpi_notify_wq;
  static struct workqueue_struct *kacpi_hotplug_wq;
  
 +struct acpi_region {
 + unsigned long flags;
 +#define ACPI_REGION_DEFAULT  0x01
 +#define ACPI_REGION_INSTALLED0x02
 +#define ACPI_REGION_REGISTERED   0x04
 +#define ACPI_REGION_UNREGISTERING0x08
 +#define ACPI_REGION_INSTALLING   0x10

What about (1UL  1), (1UL  2) etc.?

Also please remove the #defines out of the struct definition.

 + /*
 +  * NOTE: Upgrading All Region Handlers
 +  * This flag is only used during the period where not all of the
 +  * region handers are upgraded to the new interfaces.
 +  */
 +#define ACPI_REGION_MANAGED  0x80
 + acpi_adr_space_handler handler;
 + acpi_adr_space_setup setup;
 + void *context;
 + /* Invoking references */
 + atomic_t refcnt;

Actually, why don't you use krefs?

 +};
 +
 +static struct acpi_region acpi_regions[ACPI_NUM_PREDEFINED_REGIONS] = {
 + [ACPI_ADR_SPACE_SYSTEM_MEMORY] = {
 + .flags = ACPI_REGION_DEFAULT,
 + },
 + [ACPI_ADR_SPACE_SYSTEM_IO] = {
 + .flags = ACPI_REGION_DEFAULT,
 + },
 + [ACPI_ADR_SPACE_PCI_CONFIG] = {
 + .flags = ACPI_REGION_DEFAULT,
 + },
 + [ACPI_ADR_SPACE_IPMI] = {
 + .flags = ACPI_REGION_MANAGED,
 + },
 +};
 +static DEFINE_MUTEX(acpi_mutex_region);
 +
  /*
   * This list of permanent mappings is for memory that may be accessed from
   * interrupt context, where we can't do the ioremap().
 @@ -1799,3 +1835,191 @@ void alloc_acpi_hp_work(acpi_handle handle, u32 type, 
 void *context,
   kfree(hp_work);
  }
  EXPORT_SYMBOL_GPL(alloc_acpi_hp_work);
 +
 +static bool acpi_region_managed(struct acpi_region *rgn)
 +{
 + /*
 +  * NOTE: Default and Managed
 +  * We only need to avoid region

Re: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers

On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
 This patch adds reference couting for ACPI operation region handlers to fix
 races caused by the ACPICA address space callback invocations.
 
 ACPICA address space callback invocation is not suitable for Linux
 CONFIG_MODULE=y execution environment.

Actually, can you please explain to me what *exactly* the problem is?

Rafael

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers



 From: Rafael J. Wysocki [mailto:r...@sisk.pl]
 Sent: Friday, July 26, 2013 4:27 AM
 
 On Tuesday, July 23, 2013 04:09:43 PM Lv Zheng wrote:
  This patch adds reference couting for ACPI operation region handlers
  to fix races caused by the ACPICA address space callback invocations.
 
  ACPICA address space callback invocation is not suitable for Linux
  CONFIG_MODULE=y execution environment.  This patch tries to protect
  the address space callbacks by invoking them under a module safe
 environment.
  The IPMI address space handler is also upgraded in this patch.
  The acpi_unregister_region() is designed to meet the following
  requirements:
  1. It acts as a barrier for operation region callbacks - no callback will
 happen after acpi_unregister_region().
  2. acpi_unregister_region() is safe to be called in moudle-exit()
 functions.
  Using reference counting rather than module referencing allows such
  benefits to be achieved even when acpi_unregister_region() is called
  in the environments other than module-exit().
  The header file of include/acpi/acpi_bus.h should contain the
  declarations that have references to some ACPICA defined types.
 
  Signed-off-by: Lv Zheng lv.zh...@intel.com
  Reviewed-by: Huang Ying ying.hu...@intel.com
  ---
   drivers/acpi/acpi_ipmi.c |   16 ++--
   drivers/acpi/osl.c   |  224
 ++
   include/acpi/acpi_bus.h  |5 ++
   3 files changed, 235 insertions(+), 10 deletions(-)
 
  diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
  5f8f495..2a09156 100644
  --- a/drivers/acpi/acpi_ipmi.c
  +++ b/drivers/acpi/acpi_ipmi.c
  @@ -539,20 +539,18 @@ out_ref:
   static int __init acpi_ipmi_init(void)  {
  int result = 0;
  -   acpi_status status;
 
  if (acpi_disabled)
  return result;
 
  mutex_init(driver_data.ipmi_lock);
 
  -   status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
  -   ACPI_ADR_SPACE_IPMI,
  -   acpi_ipmi_space_handler,
  -   NULL, NULL);
  -   if (ACPI_FAILURE(status)) {
  +   result = acpi_register_region(ACPI_ADR_SPACE_IPMI,
  + acpi_ipmi_space_handler,
  + NULL, NULL);
  +   if (result) {
  pr_warn(Can't register IPMI opregion space handle\n);
  -   return -EINVAL;
  +   return result;
  }
 
  result = ipmi_smi_watcher_register(driver_data.bmc_events);
  @@ -596,9 +594,7 @@ static void __exit acpi_ipmi_exit(void)
  }
  mutex_unlock(driver_data.ipmi_lock);
 
  -   acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
  - ACPI_ADR_SPACE_IPMI,
  - acpi_ipmi_space_handler);
  +   acpi_unregister_region(ACPI_ADR_SPACE_IPMI);
   }
 
   module_init(acpi_ipmi_init);
  diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index
  6ab2c35..8398e51 100644
  --- a/drivers/acpi/osl.c
  +++ b/drivers/acpi/osl.c
  @@ -86,6 +86,42 @@ static struct workqueue_struct *kacpid_wq;  static
  struct workqueue_struct *kacpi_notify_wq;  static struct
  workqueue_struct *kacpi_hotplug_wq;
 
  +struct acpi_region {
  +   unsigned long flags;
  +#define ACPI_REGION_DEFAULT0x01
  +#define ACPI_REGION_INSTALLED  0x02
  +#define ACPI_REGION_REGISTERED 0x04
  +#define ACPI_REGION_UNREGISTERING  0x08
  +#define ACPI_REGION_INSTALLING 0x10
 
 What about (1UL  1), (1UL  2) etc.?
 
 Also please remove the #defines out of the struct definition.

OK.

 
  +   /*
  +* NOTE: Upgrading All Region Handlers
  +* This flag is only used during the period where not all of the
  +* region handers are upgraded to the new interfaces.
  +*/
  +#define ACPI_REGION_MANAGED0x80
  +   acpi_adr_space_handler handler;
  +   acpi_adr_space_setup setup;
  +   void *context;
  +   /* Invoking references */
  +   atomic_t refcnt;
 
 Actually, why don't you use krefs?

If you take a look at other piece of my codes, you'll find there are two 
reasons:

1. I'm using while (atomic_read()  1) to implement the objects' flushing and 
there is no kref API to do so.
  I just think it is not suitable for me to introduce such an API into kref.h 
and start another argument around kref designs in this bug fix patch. :-)
  I'll start a discussion about kref design using another thread.
2. I'm using ipmi_dev|msg_release() as a pair of ipmi_dev|msg_alloc(), it's 
kind of atomic_t coding style.
  If atomic_t is changed to struct kref, I will need to implement two API, 
__ipmi_dev_release() to take a struct kref as parameter and call 
ipmi_dev_release inside it.
  By not using kref, I needn't write codes to implement such API.

 
  +};
  +
  +static struct acpi_region acpi_regions[ACPI_NUM_PREDEFINED_REGIONS]
 = {
  +

RE: [PATCH 06/13] ACPI/IPMI: Add reference counting for ACPI operation region handlers