linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Declare PNP option parsing functions as __init
@ 2007-11-30 17:04 Thomas Renninger
  2007-11-30 23:37 ` Rene Herman
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Renninger @ 2007-11-30 17:04 UTC (permalink / raw)
  To: linux-kernel; +Cc: Bjorn Helgaas, Rene Herman, akpm, Li, Shaohua

If I have not overseen something, it should be rather obvious that those
can all be declared __init...
---------------

Declare PNP option parsing functions as __init

There are three kind of parse functions provided by PNP acpi/bios:
 - get current resources
 - set resources
 - get possible resources
The first two may be needed later at runtime.
The possible resource settings should never change dynamically.
And even if this would make any sense (I doubt it), the current implementation
only parses possible resource settings at early init time:
  -> declare all the option parsing __init

Signed-off-by: Thomas Renninger <trenn@suse.de>

---
 drivers/pnp/pnpacpi/rsparser.c |   44 ++++++++++++++++++++---------------------
 drivers/pnp/pnpbios/core.c     |    2 -
 drivers/pnp/pnpbios/rsparser.c |   32 ++++++++++++++---------------
 3 files changed, 39 insertions(+), 39 deletions(-)

Index: linux-2.6.24-rc3/drivers/pnp/pnpacpi/rsparser.c
===================================================================
--- linux-2.6.24-rc3.orig/drivers/pnp/pnpacpi/rsparser.c
+++ linux-2.6.24-rc3/drivers/pnp/pnpacpi/rsparser.c
@@ -384,8 +384,8 @@ acpi_status pnpacpi_parse_allocated_reso
 				   pnpacpi_allocated_resource, res);
 }
 
-static void pnpacpi_parse_dma_option(struct pnp_option *option,
-				     struct acpi_resource_dma *p)
+static __init void pnpacpi_parse_dma_option(struct pnp_option *option,
+					    struct acpi_resource_dma *p)
 {
 	int i;
 	struct pnp_dma *dma;
@@ -404,8 +404,8 @@ static void pnpacpi_parse_dma_option(str
 	pnp_register_dma_resource(option, dma);
 }
 
-static void pnpacpi_parse_irq_option(struct pnp_option *option,
-				     struct acpi_resource_irq *p)
+static __init void pnpacpi_parse_irq_option(struct pnp_option *option,
+					    struct acpi_resource_irq *p)
 {
 	int i;
 	struct pnp_irq *irq;
@@ -424,8 +424,8 @@ static void pnpacpi_parse_irq_option(str
 	pnp_register_irq_resource(option, irq);
 }
 
-static void pnpacpi_parse_ext_irq_option(struct pnp_option *option,
-					 struct acpi_resource_extended_irq *p)
+static __init void pnpacpi_parse_ext_irq_option(struct pnp_option *option,
+						struct acpi_resource_extended_irq *p)
 {
 	int i;
 	struct pnp_irq *irq;
@@ -444,8 +444,8 @@ static void pnpacpi_parse_ext_irq_option
 	pnp_register_irq_resource(option, irq);
 }
 
-static void pnpacpi_parse_port_option(struct pnp_option *option,
-				      struct acpi_resource_io *io)
+static __init void pnpacpi_parse_port_option(struct pnp_option *option,
+					     struct acpi_resource_io *io)
 {
 	struct pnp_port *port;
 
@@ -463,8 +463,8 @@ static void pnpacpi_parse_port_option(st
 	pnp_register_port_resource(option, port);
 }
 
-static void pnpacpi_parse_fixed_port_option(struct pnp_option *option,
-					    struct acpi_resource_fixed_io *io)
+static __init void pnpacpi_parse_fixed_port_option(struct pnp_option *option,
+						   struct acpi_resource_fixed_io *io)
 {
 	struct pnp_port *port;
 
@@ -480,8 +480,8 @@ static void pnpacpi_parse_fixed_port_opt
 	pnp_register_port_resource(option, port);
 }
 
-static void pnpacpi_parse_mem24_option(struct pnp_option *option,
-				       struct acpi_resource_memory24 *p)
+static __init void pnpacpi_parse_mem24_option(struct pnp_option *option,
+					      struct acpi_resource_memory24 *p)
 {
 	struct pnp_mem *mem;
 
@@ -501,8 +501,8 @@ static void pnpacpi_parse_mem24_option(s
 	pnp_register_mem_resource(option, mem);
 }
 
-static void pnpacpi_parse_mem32_option(struct pnp_option *option,
-				       struct acpi_resource_memory32 *p)
+static __init void pnpacpi_parse_mem32_option(struct pnp_option *option,
+					      struct acpi_resource_memory32 *p)
 {
 	struct pnp_mem *mem;
 
@@ -522,8 +522,8 @@ static void pnpacpi_parse_mem32_option(s
 	pnp_register_mem_resource(option, mem);
 }
 
-static void pnpacpi_parse_fixed_mem32_option(struct pnp_option *option,
-					 struct acpi_resource_fixed_memory32 *p)
+static __init void pnpacpi_parse_fixed_mem32_option(struct pnp_option *option,
+						    struct acpi_resource_fixed_memory32 *p)
 {
 	struct pnp_mem *mem;
 
@@ -542,8 +542,8 @@ static void pnpacpi_parse_fixed_mem32_op
 	pnp_register_mem_resource(option, mem);
 }
 
-static void pnpacpi_parse_address_option(struct pnp_option *option,
-					 struct acpi_resource *r)
+static __init void pnpacpi_parse_address_option(struct pnp_option *option,
+						struct acpi_resource *r)
 {
 	struct acpi_resource_address64 addr, *p = &addr;
 	acpi_status status;
@@ -589,8 +589,8 @@ struct acpipnp_parse_option_s {
 	struct pnp_dev *dev;
 };
 
-static acpi_status pnpacpi_option_resource(struct acpi_resource *res,
-					   void *data)
+static __init acpi_status pnpacpi_option_resource(struct acpi_resource *res,
+						  void *data)
 {
 	int priority = 0;
 	struct acpipnp_parse_option_s *parse_data = data;
@@ -689,8 +689,8 @@ static acpi_status pnpacpi_option_resour
 	return AE_OK;
 }
 
-acpi_status pnpacpi_parse_resource_option_data(acpi_handle handle,
-					       struct pnp_dev * dev)
+acpi_status __init pnpacpi_parse_resource_option_data(acpi_handle handle,
+						      struct pnp_dev * dev)
 {
 	acpi_status status;
 	struct acpipnp_parse_option_s parse_data;
Index: linux-2.6.24-rc3/drivers/pnp/pnpbios/rsparser.c
===================================================================
--- linux-2.6.24-rc3.orig/drivers/pnp/pnpbios/rsparser.c
+++ linux-2.6.24-rc3/drivers/pnp/pnpbios/rsparser.c
@@ -262,8 +262,8 @@ len_err:
  * Resource Configuration Options
  */
 
-static void pnpbios_parse_mem_option(unsigned char *p, int size,
-				     struct pnp_option *option)
+static __init void pnpbios_parse_mem_option(unsigned char *p, int size,
+					    struct pnp_option *option)
 {
 	struct pnp_mem *mem;
 
@@ -278,8 +278,8 @@ static void pnpbios_parse_mem_option(uns
 	pnp_register_mem_resource(option, mem);
 }
 
-static void pnpbios_parse_mem32_option(unsigned char *p, int size,
-				       struct pnp_option *option)
+static __init void pnpbios_parse_mem32_option(unsigned char *p, int size,
+					      struct pnp_option *option)
 {
 	struct pnp_mem *mem;
 
@@ -294,8 +294,8 @@ static void pnpbios_parse_mem32_option(u
 	pnp_register_mem_resource(option, mem);
 }
 
-static void pnpbios_parse_fixed_mem32_option(unsigned char *p, int size,
-					     struct pnp_option *option)
+static __init void pnpbios_parse_fixed_mem32_option(unsigned char *p, int size,
+						    struct pnp_option *option)
 {
 	struct pnp_mem *mem;
 
@@ -309,7 +309,7 @@ static void pnpbios_parse_fixed_mem32_op
 	pnp_register_mem_resource(option, mem);
 }
 
-static void pnpbios_parse_irq_option(unsigned char *p, int size,
+static __init void pnpbios_parse_irq_option(unsigned char *p, int size,
 				     struct pnp_option *option)
 {
 	struct pnp_irq *irq;
@@ -327,7 +327,7 @@ static void pnpbios_parse_irq_option(uns
 	pnp_register_irq_resource(option, irq);
 }
 
-static void pnpbios_parse_dma_option(unsigned char *p, int size,
+static __init void pnpbios_parse_dma_option(unsigned char *p, int size,
 				     struct pnp_option *option)
 {
 	struct pnp_dma *dma;
@@ -340,8 +340,8 @@ static void pnpbios_parse_dma_option(uns
 	pnp_register_dma_resource(option, dma);
 }
 
-static void pnpbios_parse_port_option(unsigned char *p, int size,
-				      struct pnp_option *option)
+static __init void pnpbios_parse_port_option(unsigned char *p, int size,
+					     struct pnp_option *option)
 {
 	struct pnp_port *port;
 
@@ -356,8 +356,8 @@ static void pnpbios_parse_port_option(un
 	pnp_register_port_resource(option, port);
 }
 
-static void pnpbios_parse_fixed_port_option(unsigned char *p, int size,
-					    struct pnp_option *option)
+static __init void pnpbios_parse_fixed_port_option(unsigned char *p, int size,
+						   struct pnp_option *option)
 {
 	struct pnp_port *port;
 
@@ -371,9 +371,9 @@ static void pnpbios_parse_fixed_port_opt
 	pnp_register_port_resource(option, port);
 }
 
-static unsigned char *pnpbios_parse_resource_option_data(unsigned char *p,
-							 unsigned char *end,
-							 struct pnp_dev *dev)
+static __init unsigned char *pnpbios_parse_resource_option_data(unsigned char *p,
+								unsigned char *end,
+								struct pnp_dev *dev)
 {
 	unsigned int len, tag;
 	int priority = 0;
@@ -781,7 +781,7 @@ len_err:
  * Core Parsing Functions
  */
 
-int pnpbios_parse_data_stream(struct pnp_dev *dev, struct pnp_bios_node *node)
+int __init pnpbios_parse_data_stream(struct pnp_dev *dev, struct pnp_bios_node *node)
 {
 	unsigned char *p = (char *)node->data;
 	unsigned char *end = (char *)(node->data + node->size);
Index: linux-2.6.24-rc3/drivers/pnp/pnpbios/core.c
===================================================================
--- linux-2.6.24-rc3.orig/drivers/pnp/pnpbios/core.c
+++ linux-2.6.24-rc3/drivers/pnp/pnpbios/core.c
@@ -313,7 +313,7 @@ struct pnp_protocol pnpbios_protocol = {
 	.disable = pnpbios_disable_resources,
 };
 
-static int insert_device(struct pnp_bios_node *node)
+static int __init insert_device(struct pnp_bios_node *node)
 {
 	struct list_head *pos;
 	struct pnp_dev *dev;



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-11-30 17:04 [PATCH] Declare PNP option parsing functions as __init Thomas Renninger
@ 2007-11-30 23:37 ` Rene Herman
  2007-11-30 23:52   ` Bjorn Helgaas
  0 siblings, 1 reply; 9+ messages in thread
From: Rene Herman @ 2007-11-30 23:37 UTC (permalink / raw)
  To: trenn; +Cc: linux-kernel, Bjorn Helgaas, akpm, Li, Shaohua

On 30-11-07 18:04, Thomas Renninger wrote:

> If I have not overseen something, it should be rather obvious that those
> can all be declared __init...
> ---------------
> 
> Declare PNP option parsing functions as __init
> 
> There are three kind of parse functions provided by PNP acpi/bios:
>  - get current resources
>  - set resources
>  - get possible resources
> The first two may be needed later at runtime.
> The possible resource settings should never change dynamically.
> And even if this would make any sense (I doubt it), the current implementation
> only parses possible resource settings at early init time:
>   -> declare all the option parsing __init
> 
> Signed-off-by: Thomas Renninger <trenn@suse.de>

Yes. Obviousness aside,

(0) pnpacpi_add_device                          is only caller of
(1)   pnpacpi_parse_resource_option_data        is only caller of
(2)     pnpacpi_option_resource                 is only caller of
(3)       pnpacpi_parse_irq_option
(3)       pnpacpi_parse_dma_option
(3)       pnpacpi_parse_port_option
(3)       pnpacpi_parse_fixed_port_option
(3)       pnpacpi_parse_mem24_option
(3)       pnpacpi_parse_mem32_option
(3)       pnpacpi_parse_fixed_mem32_option
(3)       pnpacpi_parse_address_option
(3)       pnpacpi_parse_ext_irq_option

and

(0) build_devlist                               is only caller of
(1)   insert_device                             is only caller of
(2)     pnpbios_parse_data_stream               is only caller of
(3)       pnpbios_parse_resource_option_data    is only caller of
(4)         pnpbios_parse_mem_option
(4)         pnpbios_parse_mem32_option
(4)         pnpbios_parse_fixed_mem32_option
(4)         pnpbios_parse_irq_option
(4)         pnpbios_parse_dma_option
(4)         pnpbios_parse_port_option
(4)         pnpbios_parse_fixed_port_option

which given that both (0)s are __init already, means all are fine indeed.

Acked-By: Rene Herman <rene.herman@gmail.com>

Rene.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-11-30 23:37 ` Rene Herman
@ 2007-11-30 23:52   ` Bjorn Helgaas
  2007-12-01  0:33     ` Rene Herman
                       ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2007-11-30 23:52 UTC (permalink / raw)
  To: Rene Herman; +Cc: trenn, linux-kernel, akpm, Li, Shaohua

On Friday 30 November 2007 04:37:26 pm Rene Herman wrote:
> On 30-11-07 18:04, Thomas Renninger wrote:
> > If I have not overseen something, it should be rather obvious that those
> > can all be declared __init...
> > ---------------
> > 
> > Declare PNP option parsing functions as __init
> > 
> > There are three kind of parse functions provided by PNP acpi/bios:
> >  - get current resources
> >  - set resources
> >  - get possible resources
> > The first two may be needed later at runtime.
> > The possible resource settings should never change dynamically.
> > And even if this would make any sense (I doubt it), the current implementation
> > only parses possible resource settings at early init time:
> >   -> declare all the option parsing __init
> > 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> 
> Yes. Obviousness aside,
> 
> (0) pnpacpi_add_device                          is only caller of
> ...

I agree this is probably safe in the current implementation.

However, I think the current implementation is just broken because
we can't really handle hotplug of ACPI devices.  Specifically, I think
the first TBD in acpi_bus_check_device() should be fleshed out so it
does something like pnpacpi_add_device().

So my dissenting opinion is that this patch would just get reverted
soon anyway when somebody finishes implementing ACPI hotplug, and
therefore it's not worth doing.

Bjorn

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-11-30 23:52   ` Bjorn Helgaas
@ 2007-12-01  0:33     ` Rene Herman
  2007-12-02 13:34       ` Thomas Renninger
  2007-12-02 13:32     ` Thomas Renninger
  2007-12-03 11:53     ` Thomas Renninger
  2 siblings, 1 reply; 9+ messages in thread
From: Rene Herman @ 2007-12-01  0:33 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: trenn, linux-kernel, akpm, Li, Shaohua, Alan Cox

On 01-12-07 00:52, Bjorn Helgaas wrote:

> On Friday 30 November 2007 04:37:26 pm Rene Herman wrote:
>> On 30-11-07 18:04, Thomas Renninger wrote:
>>> If I have not overseen something, it should be rather obvious that those
>>> can all be declared __init...
>>> ---------------
>>>
>>> Declare PNP option parsing functions as __init
>>>
>>> There are three kind of parse functions provided by PNP acpi/bios:
>>>  - get current resources
>>>  - set resources
>>>  - get possible resources
>>> The first two may be needed later at runtime.
>>> The possible resource settings should never change dynamically.
>>> And even if this would make any sense (I doubt it), the current implementation
>>> only parses possible resource settings at early init time:
>>>   -> declare all the option parsing __init
>>>
>>> Signed-off-by: Thomas Renninger <trenn@suse.de>
>> Yes. Obviousness aside,
>>
>> (0) pnpacpi_add_device                          is only caller of
>> ...
> 
> I agree this is probably safe in the current implementation.
> 
> However, I think the current implementation is just broken because
> we can't really handle hotplug of ACPI devices.  Specifically, I think
> the first TBD in acpi_bus_check_device() should be fleshed out so it
> does something like pnpacpi_add_device().
> 
> So my dissenting opinion is that this patch would just get reverted
> soon anyway when somebody finishes implementing ACPI hotplug, and
> therefore it's not worth doing.

<shrug>

The PnPBIOS bits should still be fine at least I guess. And, it would seem 
this is rather essential to Thomas' efforts of making this stuff dynamic in 
the first place anyway. In these threads, Alan Cox (added to CC) earlier 
commented that with required locking when things are quite _that_ dynamic a 
different setup than the one currently on the table seems in order.

I don't know, but small steps may make sense giving that it seems dynamic 
allocation is fairly wanted with the massive number of PnPACPI resources 
people are reporting since the warning about overrunning them was added.

Rene.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-11-30 23:52   ` Bjorn Helgaas
  2007-12-01  0:33     ` Rene Herman
@ 2007-12-02 13:32     ` Thomas Renninger
  2007-12-03 11:53     ` Thomas Renninger
  2 siblings, 0 replies; 9+ messages in thread
From: Thomas Renninger @ 2007-12-02 13:32 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rene Herman, linux-kernel, akpm, Li, Shaohua


On Fri, 2007-11-30 at 16:52 -0700, Bjorn Helgaas wrote:
> On Friday 30 November 2007 04:37:26 pm Rene Herman wrote:
> > On 30-11-07 18:04, Thomas Renninger wrote:
> > > If I have not overseen something, it should be rather obvious that those
> > > can all be declared __init...
> > > ---------------
> > > 
> > > Declare PNP option parsing functions as __init
> > > 
> > > There are three kind of parse functions provided by PNP acpi/bios:
> > >  - get current resources
> > >  - set resources
> > >  - get possible resources
> > > The first two may be needed later at runtime.
> > > The possible resource settings should never change dynamically.
> > > And even if this would make any sense (I doubt it), the current implementation
> > > only parses possible resource settings at early init time:
> > >   -> declare all the option parsing __init
> > > 
> > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > 
> > Yes. Obviousness aside,
> > 
> > (0) pnpacpi_add_device                          is only caller of
> > ...
> 
> I agree this is probably safe in the current implementation.
> 
> However, I think the current implementation is just broken because
> we can't really handle hotplug of ACPI devices.  Specifically, I think
> the first TBD in acpi_bus_check_device() should be fleshed out so it
> does something like pnpacpi_add_device().
Yes, making the ACPI layer more hotplug capable is something that should
be worked on again.

> So my dissenting opinion is that this patch would just get reverted
> soon anyway when somebody finishes implementing ACPI hotplug, and
> therefore it's not worth doing.

Yes you are right. I thought _PSR could always be called at init time,
whether present or not (which would certainly work for most/all devices
as _PSR info should be kind of static), but ACPI spec forbids it:

6.3.7 _STA (Status)
If bit 0 is cleared, then bit 1 must also be cleared (in other words, a
device that is not present cannot be enabled).
A device can only decode its hardware resources if both bits 0 and 1 are
set. If the device is not present (bit 0 cleared) or not enabled (bit 1
cleared), then the device must not decode its resources.

I only saw:
6.2.10 _PRS (Possible Resource Settings):
If the device is disabled when _PRS is called, it must remain disabled.

But disabled and not present are different things...


It's stupid as the possible resources of a device will always remain the
same, whether it is present or not, but if it is written down in the
spec, there is not much to argue about it...

Thanks,

    Thomas


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-12-01  0:33     ` Rene Herman
@ 2007-12-02 13:34       ` Thomas Renninger
  2007-12-02 13:50         ` Rene Herman
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Renninger @ 2007-12-02 13:34 UTC (permalink / raw)
  To: Rene Herman; +Cc: Bjorn Helgaas, linux-kernel, akpm, Li, Shaohua, Alan Cox


On Sat, 2007-12-01 at 01:33 +0100, Rene Herman wrote:
> On 01-12-07 00:52, Bjorn Helgaas wrote:
> 
> > On Friday 30 November 2007 04:37:26 pm Rene Herman wrote:
> >> On 30-11-07 18:04, Thomas Renninger wrote:
> >>> If I have not overseen something, it should be rather obvious that those
> >>> can all be declared __init...
> >>> ---------------
> >>>
> >>> Declare PNP option parsing functions as __init
> >>>
> >>> There are three kind of parse functions provided by PNP acpi/bios:
> >>>  - get current resources
> >>>  - set resources
> >>>  - get possible resources
> >>> The first two may be needed later at runtime.
> >>> The possible resource settings should never change dynamically.
> >>> And even if this would make any sense (I doubt it), the current implementation
> >>> only parses possible resource settings at early init time:
> >>>   -> declare all the option parsing __init
> >>>
> >>> Signed-off-by: Thomas Renninger <trenn@suse.de>
> >> Yes. Obviousness aside,
> >>
> >> (0) pnpacpi_add_device                          is only caller of
> >> ...
> > 
> > I agree this is probably safe in the current implementation.
> > 
> > However, I think the current implementation is just broken because
> > we can't really handle hotplug of ACPI devices.  Specifically, I think
> > the first TBD in acpi_bus_check_device() should be fleshed out so it
> > does something like pnpacpi_add_device().
> > 
> > So my dissenting opinion is that this patch would just get reverted
> > soon anyway when somebody finishes implementing ACPI hotplug, and
> > therefore it's not worth doing.
> 
> <shrug>
> 
> The PnPBIOS bits should still be fine at least I guess. And, it would seem 
> this is rather essential to Thomas' efforts of making this stuff dynamic in 
> the first place anyway.
No it is not. It is just another optimization I saw while going through
these code parts...

   Thomas


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-12-02 13:34       ` Thomas Renninger
@ 2007-12-02 13:50         ` Rene Herman
  0 siblings, 0 replies; 9+ messages in thread
From: Rene Herman @ 2007-12-02 13:50 UTC (permalink / raw)
  To: trenn; +Cc: Bjorn Helgaas, linux-kernel, akpm, Li, Shaohua, Alan Cox

On 02-12-07 14:34, Thomas Renninger wrote:

> On Sat, 2007-12-01 at 01:33 +0100, Rene Herman wrote:
>> On 01-12-07 00:52, Bjorn Helgaas wrote:

>>> I agree this is probably safe in the current implementation.
>>>
>>> However, I think the current implementation is just broken because
>>> we can't really handle hotplug of ACPI devices.  Specifically, I think
>>> the first TBD in acpi_bus_check_device() should be fleshed out so it
>>> does something like pnpacpi_add_device().
>>>
>>> So my dissenting opinion is that this patch would just get reverted
>>> soon anyway when somebody finishes implementing ACPI hotplug, and
>>> therefore it's not worth doing.
>> <shrug>
>>
>> The PnPBIOS bits should still be fine at least I guess. And, it would seem 
>> this is rather essential to Thomas' efforts of making this stuff dynamic in 
>> the first place anyway.
> No it is not. It is just another optimization I saw while going through
> these code parts...

Bjorn's argument of making the possible resources runtime dynamic is the 
essential bit, not the patch. You weren't doing that in the simple scheme 
you've outlined till now. Are you or is anyone else now after all?

Rene.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-11-30 23:52   ` Bjorn Helgaas
  2007-12-01  0:33     ` Rene Herman
  2007-12-02 13:32     ` Thomas Renninger
@ 2007-12-03 11:53     ` Thomas Renninger
  2007-12-03 15:52       ` Bjorn Helgaas
  2 siblings, 1 reply; 9+ messages in thread
From: Thomas Renninger @ 2007-12-03 11:53 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Rene Herman, linux-kernel, akpm, Li, Shaohua

On Fri, 2007-11-30 at 16:52 -0700, Bjorn Helgaas wrote:
> On Friday 30 November 2007 04:37:26 pm Rene Herman wrote:
> > On 30-11-07 18:04, Thomas Renninger wrote:
> > > If I have not overseen something, it should be rather obvious that those
> > > can all be declared __init...
> > > ---------------
> > > 
> > > Declare PNP option parsing functions as __init
> > > 
> > > There are three kind of parse functions provided by PNP acpi/bios:
> > >  - get current resources
> > >  - set resources
> > >  - get possible resources
> > > The first two may be needed later at runtime.
> > > The possible resource settings should never change dynamically.
> > > And even if this would make any sense (I doubt it), the current implementation
> > > only parses possible resource settings at early init time:
> > >   -> declare all the option parsing __init
> > > 
> > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > 
> > Yes. Obviousness aside,
> > 
> > (0) pnpacpi_add_device                          is only caller of
> > ...
> 
> I agree this is probably safe in the current implementation.
> 
> However, I think the current implementation is just broken because
> we can't really handle hotplug of ACPI devices.  Specifically, I think
> the first TBD in acpi_bus_check_device() should be fleshed out so it
> does something like pnpacpi_add_device().
> 
> So my dissenting opinion is that this patch would just get reverted
> soon anyway when somebody finishes implementing ACPI hotplug, and
> therefore it's not worth doing.

Ok, this all applies to the ACPI parts, but for pnpbios it probably
makes sense to only evaluate possible resources only once at boot time?

   Thomas


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Declare PNP option parsing functions as __init
  2007-12-03 11:53     ` Thomas Renninger
@ 2007-12-03 15:52       ` Bjorn Helgaas
  0 siblings, 0 replies; 9+ messages in thread
From: Bjorn Helgaas @ 2007-12-03 15:52 UTC (permalink / raw)
  To: trenn; +Cc: Rene Herman, linux-kernel, akpm, Li, Shaohua

On Monday 03 December 2007 04:53:01 am Thomas Renninger wrote:
> On Fri, 2007-11-30 at 16:52 -0700, Bjorn Helgaas wrote:
> > On Friday 30 November 2007 04:37:26 pm Rene Herman wrote:
> > > On 30-11-07 18:04, Thomas Renninger wrote:
> > > > If I have not overseen something, it should be rather obvious that those
> > > > can all be declared __init...
> > > > ---------------
> > > > 
> > > > Declare PNP option parsing functions as __init
> > > > 
> > > > There are three kind of parse functions provided by PNP acpi/bios:
> > > >  - get current resources
> > > >  - set resources
> > > >  - get possible resources
> > > > The first two may be needed later at runtime.
> > > > The possible resource settings should never change dynamically.
> > > > And even if this would make any sense (I doubt it), the current implementation
> > > > only parses possible resource settings at early init time:
> > > >   -> declare all the option parsing __init
> > > > 
> > > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > > 
> > > Yes. Obviousness aside,
> > > 
> > > (0) pnpacpi_add_device                          is only caller of
> > > ...
> > 
> > I agree this is probably safe in the current implementation.
> > 
> > However, I think the current implementation is just broken because
> > we can't really handle hotplug of ACPI devices.  Specifically, I think
> > the first TBD in acpi_bus_check_device() should be fleshed out so it
> > does something like pnpacpi_add_device().
> > 
> > So my dissenting opinion is that this patch would just get reverted
> > soon anyway when somebody finishes implementing ACPI hotplug, and
> > therefore it's not worth doing.
> 
> Ok, this all applies to the ACPI parts, but for pnpbios it probably
> makes sense to only evaluate possible resources only once at boot time?

There are PNPBIOS functions that are related to hotplug (for docking
stations and the like), but it doesn't look like we ever use them.
There's not much use in implementing PNPBIOS hotplug now, so I think
you're right that we could make PNPBIOS a boot-time only thing.

Bjorn

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2007-12-03 15:53 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-11-30 17:04 [PATCH] Declare PNP option parsing functions as __init Thomas Renninger
2007-11-30 23:37 ` Rene Herman
2007-11-30 23:52   ` Bjorn Helgaas
2007-12-01  0:33     ` Rene Herman
2007-12-02 13:34       ` Thomas Renninger
2007-12-02 13:50         ` Rene Herman
2007-12-02 13:32     ` Thomas Renninger
2007-12-03 11:53     ` Thomas Renninger
2007-12-03 15:52       ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).