PNP cleanups - Version 2 - Pass struct pnp_dev to pnp_clean_resource_table for cleanup reasons

PATCH In review Thomas Renninger 3 participants 8 messages Nov 20, 2007 – Nov 21, 2007 Pnp support

lore

Share & tools

lore

Thomas Renninger

author

2007-11-20 17:52 UTC #0 lore raw

PATCH 3/3 PNP cleanups - Version 2 - Pass struct pnp_dev to pnp_clean_resource_table for cleanup reasons

Pass struct pnp_dev to pnp_clean_resource_table for cleanup reasons

Signed-off-by Thomas Renninger <[email protected]>

---
 drivers/pnp/manager.c |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

drivers/pnp/manager.c +18 ~~-18~~

Index: linux-2.6.24-rc2-mm1/drivers/pnp/manager.c
===================================================================
@@ -235,38 +235,38 @@ void pnp_init_resource_table(struct pnp_
  * pnp_clean_resources - clears resources that were not manually set
  * @res: the resources to clean
  */
-static void pnp_clean_resource_table(struct pnp_resource_table *res)
+static void pnp_clean_resource_table(struct pnp_dev *dev)
 {
 	int idx;
 
 	for (idx = 0; idx < PNP_MAX_IRQ; idx++) {
-		if (!(res->irq_resource[idx].flags & IORESOURCE_AUTO))
+		if (!(pnp_irq_flags(dev, idx) & IORESOURCE_AUTO))
 			continue;
-		res->irq_resource[idx].start = -1;
-		res->irq_resource[idx].flags =
+		pnp_irq_no(dev, idx) = -1;
+		pnp_irq_flags(dev, idx) =
 		    IORESOURCE_IRQ | IORESOURCE_AUTO | IORESOURCE_UNSET;
 	}
 	for (idx = 0; idx < PNP_MAX_DMA; idx++) {
-		if (!(res->dma_resource[idx].flags & IORESOURCE_AUTO))
+		if (!(pnp_dma_flags(dev, idx) & IORESOURCE_AUTO))
 			continue;
-		res->dma_resource[idx].start = -1;
-		res->dma_resource[idx].flags =
+		pnp_dma_no(dev, idx) = -1;
+		pnp_dma_flags(dev, idx) =
 		    IORESOURCE_DMA | IORESOURCE_AUTO | IORESOURCE_UNSET;
 	}
 	for (idx = 0; idx < PNP_MAX_PORT; idx++) {
-		if (!(res->port_resource[idx].flags & IORESOURCE_AUTO))
+		if (!(pnp_port_flags(dev, idx) & IORESOURCE_AUTO))
 			continue;
-		res->port_resource[idx].start = 0;
-		res->port_resource[idx].end = 0;
-		res->port_resource[idx].flags =
+		pnp_port_start(dev, idx) = 0;
+		pnp_port_end(dev, idx) = 0;
+		pnp_port_flags(dev, idx) =
 		    IORESOURCE_IO | IORESOURCE_AUTO | IORESOURCE_UNSET;
 	}
 	for (idx = 0; idx < PNP_MAX_MEM; idx++) {
-		if (!(res->mem_resource[idx].flags & IORESOURCE_AUTO))
+		if (!(pnp_mem_flags(dev, idx) & IORESOURCE_AUTO))
 			continue;
-		res->mem_resource[idx].start = 0;
-		res->mem_resource[idx].end = 0;
-		res->mem_resource[idx].flags =
+		pnp_mem_start(dev, idx) = 0;
+		pnp_mem_end(dev, idx) = 0;
+		pnp_mem_flags(dev, idx) =
 		    IORESOURCE_MEM | IORESOURCE_AUTO | IORESOURCE_UNSET;
 	}
 }

@@ -290,7 +290,7 @@ static int pnp_assign_resources(struct p
 		return -ENODEV;
 
 	down(&pnp_res_mutex);
-	pnp_clean_resource_table(&dev->res);	/* start with a fresh slate */
+	pnp_clean_resource_table(dev);	/* start with a fresh slate */
 	if (dev->independent) {
 		port = dev->independent->port;
 		mem = dev->independent->mem;

@@ -362,7 +362,7 @@ static int pnp_assign_resources(struct p
 	return 1;
 
 fail:
-	pnp_clean_resource_table(&dev->res);
+	pnp_clean_resource_table(dev);
 	up(&pnp_res_mutex);
 	return 0;
 }

@@ -546,7 +546,7 @@ int pnp_disable_dev(struct pnp_dev *dev)
 
 	/* release the resources so that other devices can use them */
 	down(&pnp_res_mutex);
-	pnp_clean_resource_table(&dev->res);
+	pnp_clean_resource_table(dev);
 	up(&pnp_res_mutex);
 
 	return 0;

Alan Cox

2007-11-20 18:03 UTC #1 lore raw

Quoted 1 line · On Tue, 20 Nov 2007 18:52:04 +0100 Thomas Renninger <[email protected]> wrote:

Pass struct pnp_dev to pnp_clean_resource_table for cleanup reasons

Again I don't see the point of this change. A routine for cleaning up resource tables expects logically to be passed a resource table to clean up not some device it may be attached to.

Perhaps if you could explain where you are trying to end up, it would help understand what you are trying to do.

I don't see why pnp_dma() and pnp_irq() should change either. It just causes noise and breaks driver code. I don't see where it needs to change to make internal pnp changes work ?

Alan

Rene Herman

2007-11-20 19:47 UTC #2 lore raw

Quoted 8 lines · Alan Cox

Quoted 1 line · On Tue, 20 Nov 2007 18:52:04 +0100 Thomas Renninger <[email protected]> wrote:
Pass struct pnp_dev to pnp_clean_resource_table for cleanup reasons
Again I don't see the point of this change. A routine for cleaning up resource tables expects logically to be passed a resource table to clean up not some device it may be attached to.

He needs to pass the pnp_dev to later be able to replace the:

	for (idx = 0; idx < PNP_MAX_PORT; idx++)

loops with:

	for (idx = 0; pnp_port(dev, idx); idx++)

in a later patch when he introduces dynamic resource tables -- pnp-acpi can (and does) now sometimes require more resources than the current pnp limits allow but simply upping the limits uncoditionally wastes too much space in the resource tables. He therefore aims to krealloc() the arrays as required.

Quoted 4 lines

Perhaps if you could explain where you are trying to end up, it would help understand what you are trying to do.
I don't see why pnp_dma() and pnp_irq() should change either. It just causes noise and breaks driver code. I don't see where it needs to change to make internal pnp changes work ?

As he explained in his 0/3, his pnp_port() would look like:

#define pnp_port(dev,bar)      ((dev)->res.allocated_ports > (bar) \
	? (&(dev)->res.port_resource[(bar)]) : NULL)

If the above replacement was the only use for the macros, he could as well do:

	for (idx = 0; idx < dev->res.allocated_ports; idx++)

but given that he'll need to get at the resource more generally, the simple pnp_port(), pnp_irq(), pnp_dma() and pnp_mem() names sound best. It would ofcourse be possible to call them something like pnp_port_addr() as well but given that he only needs to get rid of pnp_irq() and pnp_dma() to have these better names available, I'd say go for it.

pnp_{irq,dma}_no(), or pnp_{irq,dma}_start() as he originally proposed and which has consistency both with the existing pnp_{port,mem}_start() and the struct resource name as its plus should be fine and he then frees up the better names for the new use which should make for better readable code at the end of things.

My vote's with pnp_irq_start(). As said, consistent both with the port and mem variants and the struct resource usage and name.

Rene.

Alan Cox

2007-11-20 22:20 UTC #3 lore raw

Quoted 11 lines

Quoted 3 lines
Again I don't see the point of this change. A routine for cleaning up resource tables expects logically to be passed a resource table to clean up not some device it may be attached to.
He needs to pass the pnp_dev to later be able to replace the:
	for (idx = 0; idx < PNP_MAX_PORT; idx++)
loops with:
	for (idx = 0; pnp_port(dev, idx); idx++)

I can see why he does it, but that doesn't answer the problem that the code is messing up the sensible interface.

Quoted 4 lines

in a later patch when he introduces dynamic resource tables -- pnp-acpi can (and does) now sometimes require more resources than the current pnp limits allow but simply upping the limits uncoditionally wastes too much space in the resource tables. He therefore aims to krealloc() the arrays as required.

That bit is fine, but put the count in the resource structure. Then you can pass a resource around as a meaningful self contained object. That means you can pass them, ref count them and whatever else is needed.

(Remember if you krealloc a pnp resource you have to know *nobody* is using the existing resource pointer at that moment, so you will need locks attached to the resource as well - or RCU later)

Quoted 5 lines

Quoted 3 lines
I don't see why pnp_dma() and pnp_irq() should change either. It just causes noise and breaks driver code. I don't see where it needs to change to make internal pnp changes work ?
As he explained in his 0/3, his pnp_port() would look like:

You don't need to change the names - just the code.

Lets split the problems I have with it into three areas

1. Changes the interface to drivers but for no apparent reason in
part.

2. Hides a lot of stuff in macros so we get peculiar un C like
assignments that hide the actual functionality. I want to *see* what is going on when I touch the code not hide it, at least within the pnp layer

3. Object lifetime and internal completeness - dev->res should
point to everything that is neccessary to manage the resource set. It should be updatable in a consistent locked manner. That matters the moment you do any dynamic reassignment of resources affecting a live object, it matters the moment you want to do things like work with resources cleanly without the device structure.

Now I would suggest that #2 and #3 actually matter right now. We can argue about whether its called a wombat or banana and rename them any day of the week.

What I would thus like to see is a patch set which

1. Switches to krealloc resources internally and moves the
resource count sizes into the resource object but without hiding internal detail in magic macros (macros for device side are good internal bad generally). Nothing dynamic after setup *yet*

2. Update any driver specific assumptions about resource walking

Rene Herman

2007-11-21 08:42 UTC #4 lore raw

Quoted 6 lines · Alan Cox

Quoted 6 lines
Quoted 3 lines
Again I don't see the point of this change. A routine for cleaning up resource tables expects logically to be passed a resource table to clean up not some device it may be attached to.
He needs to pass the pnp_dev to later be able to replace the:

Quoted 8 lines

Quoted 5 lines
	for (idx = 0; idx < PNP_MAX_PORT; idx++)
loops with:
	for (idx = 0; pnp_port(dev, idx); idx++)
I can see why he does it, but that doesn't answer the problem that the code is messing up the sensible interface.

Not much of an interface. Note that pnp_clean_resource_table is an internal helper function in drivers/pnp/manager.c, unexported and with a mere three call sites that each call it as pnp_clean_resource_table(&dev->res). A small internal helper like this need really not be an argument.

Quoted 5 lines

Quoted 1 line
in a later patch when he introduces dynamic resource tables -- pnp-acpi can (and does) now sometimes require more resources than the current pnp limits allow but simply upping the limits uncoditionally wastes too much space in the resource tables. He therefore aims to krealloc() the arrays as required.
That bit is fine, but put the count in the resource structure. Then you can pass a resource around as a meaningful self contained object. That means you can pass them, ref count them and whatever else is needed.

I suppose you mean put the count inside the resource _table_ structure? It is a count of resource structures after all. Inside the table is where he did in fact propose it would go (dev->res.allocated_ports) but logically a pnp_dev and its pnp_resource_table are one and the same; "res" is embedded in the pnp_dev. That is, the resources are tied to the device, with struct pnp_resource_table being no more than a handy container to group them under a single name.

Only the pnp_resource_change export (that is, the pnp_init_resource_table and pnp_manual_config_dev exports) give a pnp_resource_table struct slightly more of a life of its own but those are only used by the ALSA ISAPnP drivers in what I as stated before personally consider a bad layering violation and would be more than happy to fix after which they can just go -- they have no possible uses _other_ than layering violations.

See the existing pnp design as well; all the resource stuff does the same thing in taking a pnp_dev and then returning from dev->res:

#define pnp_port_start(dev,bar) ((dev)->res.port_resource[(bar)].start)
#define pnp_port_end(dev,bar) ((dev)->res.port_resource[(bar)].end)

and so on. From a consistency standpoint making his new setup use this same paradigm makes sense to me therefore.

Quoted 1 line

(Remember if you krealloc a pnp resource you have to know *nobody* is using the existing resource pointer at that moment, so you will need locks attached to the resource as well - or RCU later)

Yes, I dont know how he intends to deal with this (nor, in fact, just how dynamic things are supposed to end up to begin with) so over to Thomas.

Quoted 10 lines

Quoted 3 lines
Quoted 2 lines
I don't see why pnp_dma() and pnp_irq() should change either. It just causes noise and breaks driver code. I don't see where it needs to change to make internal pnp changes work ?
As he explained in his 0/3, his pnp_port() would look like:
You don't need to change the names - just the code.
Lets split the problems I have with it into three areas
1. Changes the interface to drivers but for no apparent reason in
part.

Really. Only pnp_irq -> pnp_irq_start (or _no if really need be) and same for dma. Not exactly a high-invasiveness thing.

Quoted 3 lines

2. Hides a lot of stuff in macros so we get peculiar un C like
assignments that hide the actual functionality. I want to *see* what is going on when I touch the code not hide it, at least within the pnp layer

Yes, not hugely fond of the lvalue thing either. Andrew commented that he found if to be okay-ish from a consistency standpoint. <shrug>

Quite frankly, I've had enough email to PnP maintainer(s) disappear into blackholes now that I'm happy already to see some action. If I'm right that I should consider Bjorn Helgaas the PnP maintainer now, I'd vote he gets a say here. Without the magic assignments I guess you'd need helper functions for them.

Quoted 6 lines

3. Object lifetime and internal completeness - dev->res should
point to everything that is neccessary to manage the resource set. It should be updatable in a consistent locked manner. That matters the moment you do any dynamic reassignment of resources affecting a live object, it matters the moment you want to do things like work with resources cleanly without the device structure.

As to the latter bit, I'm not sure there's any point in the "without the device structure" bit. As argued above, the resources are fully tied to the device.

As to the locking, I suppose we could use a more fully complete patchset to judge whether or not these preparations are good but it _looks_ like Thomas is trying for a fairly minimally invasive route here which in itself seems to coincide with your wishes.

Quoted 5 lines

Now I would suggest that #2 and #3 actually matter right now. We can argue about whether its called a wombat or banana and rename them any day of the week.

Quoted 8 lines

What I would thus like to see is a patch set which
1. Switches to krealloc resources internally and moves the
resource count sizes into the resource object but without hiding internal detail in magic macros (macros for device side are good internal bad generally). Nothing dynamic after setup *yet*
2. Update any driver specific assumptions about resource walking

Rene.

Thomas Renninger

author

2007-11-21 09:43 UTC #5 lore raw

Quoted 31 lines · Rene Herman

Quoted 6 lines · Alan Cox
Quoted 6 lines
Quoted 3 lines
Again I don't see the point of this change. A routine for cleaning up resource tables expects logically to be passed a resource table to clean up not some device it may be attached to.
He needs to pass the pnp_dev to later be able to replace the:
Quoted 8 lines
Quoted 5 lines
	for (idx = 0; idx < PNP_MAX_PORT; idx++)
loops with:
	for (idx = 0; pnp_port(dev, idx); idx++)
I can see why he does it, but that doesn't answer the problem that the code is messing up the sensible interface.
Not much of an interface. Note that pnp_clean_resource_table is an internal helper function in drivers/pnp/manager.c, unexported and with a mere three call sites that each call it as pnp_clean_resource_table(&dev->res). A small internal helper like this need really not be an argument.
Quoted 8 lines
Quoted 4 lines
in a later patch when he introduces dynamic resource tables -- pnp-acpi can (and does) now sometimes require more resources than the current pnp limits allow but simply upping the limits uncoditionally wastes too much space in the resource tables. He therefore aims to krealloc() the arrays as required.
That bit is fine, but put the count in the resource structure. Then you can pass a resource around as a meaningful self contained object. That means you can pass them, ref count them and whatever else is needed.

Quoted 7 lines

I suppose you mean put the count inside the resource _table_ structure? It is a count of resource structures after all. Inside the table is where he did in fact propose it would go (dev->res.allocated_ports) but logically a pnp_dev and its pnp_resource_table are one and the same; "res" is embedded in the pnp_dev. That is, the resources are tied to the device, with struct pnp_resource_table being no more than a handy container to group them under a single name.

Putting the count into struct resource does not make sense.

Quoted 7 lines

Only the pnp_resource_change export (that is, the pnp_init_resource_table and pnp_manual_config_dev exports) give a pnp_resource_table struct slightly more of a life of its own but those are only used by the ALSA ISAPnP drivers in what I as stated before personally consider a bad layering violation and would be more than happy to fix after which they can just go -- they have no possible uses _other_ than layering violations.

Quoted 8 lines

See the existing pnp design as well; all the resource stuff does the same thing in taking a pnp_dev and then returning from dev->res:
#define pnp_port_start(dev,bar) ((dev)->res.port_resource[(bar)].start)
#define pnp_port_end(dev,bar) ((dev)->res.port_resource[(bar)].end)
and so on. From a consistency standpoint making his new setup use this same paradigm makes sense to me therefore.

The idea is to not rely on the exact pnp resource table structure and abstracting this to macros. If krealloc approach works, dev->res.port_resource[i].start would even still work, if not, it's easier to alter the pnp resource table and the macros internally.

Quoted 6 lines

Quoted 3 lines
(Remember if you krealloc a pnp resource you have to know *nobody* is using the existing resource pointer at that moment, so you will need locks attached to the resource as well - or RCU later)
Yes, I dont know how he intends to deal with this (nor, in fact, just how dynamic things are supposed to end up to begin with) so over to Thomas.

Krealloc should only get used at early pnp init time, when the BIOS structures are parsed. The devices shouldn't be active then... A bit of a problem, as said, could be the sysfs interfaces, there it must be insured krealloc is not used anymore.

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
I don't see why pnp_dma() and pnp_irq() should change either. It just causes noise and breaks driver code. I don't see where it needs to change to make internal pnp changes work ?
As he explained in his 0/3, his pnp_port() would look like:
You don't need to change the names - just the code.

Quoted 7 lines

Quoted 4 lines
Lets split the problems I have with it into three areas
1. Changes the interface to drivers but for no apparent reason in
part.
Really. Only pnp_irq -> pnp_irq_start (or _no if really need be) and same for dma. Not exactly a high-invasiveness thing.

Ok, I think I see the main problem: Externally built drivers would break.
I can try with a "do not alter exported interfaces if it could be avoided" approach.

I'd add these then:
#define pnp_port_ptr(dev,bar)
#define pnp_dma_ptr(dev,bar)
...

I will come back in some days...

Thomas

Alan Cox

2007-11-21 18:15 UTC #6 lore raw

Quoted 4 lines

Quoted 3 lines
in the pnp_dev. That is, the resources are tied to the device, with struct pnp_resource_table being no more than a handy container to group them under a single name.
Putting the count into struct resource does not make sense.

Can you explain that claim ?

Quoted 4 lines

The idea is to not rely on the exact pnp resource table structure and abstracting this to macros. If krealloc approach works, dev->res.port_resource[i].start would even still work, if not, it's easier to alter the pnp resource table and the macros internally.

Externally in drivers yes. Internally in code no - it makes the code harder to work with.

Quoted 6 lines

Quoted 2 lines
Yes, I dont know how he intends to deal with this (nor, in fact, just how dynamic things are supposed to end up to begin with) so over to Thomas.
Krealloc should only get used at early pnp init time, when the BIOS structures are parsed. The devices shouldn't be active then... A bit of a problem, as said, could be the sysfs interfaces, there it must be insured krealloc is not used anymore.

I don't think its that simple but that can be dealt with one the changes are in place if the objects are sensibly laid out.

Alan

Thomas Renninger

author

2007-11-21 23:06 UTC #7 lore raw

Quoted 6 lines · Alan Cox

Quoted 4 lines
Quoted 3 lines
in the pnp_dev. That is, the resources are tied to the device, with struct pnp_resource_table being no more than a handy container to group them under a single name.
Putting the count into struct resource does not make sense.
Can you explain that claim ?

The additional variable would only make sense for the pnp layer, or only for the pnp resource table in the pnp layer, but struct resource is used at much more places...
It is meant for System Memory and IO port resources in general, why waste bytes and an additional name at all places it is used, just for the pnp resource table?

Quoted 10 lines

Quoted 4 lines
The idea is to not rely on the exact pnp resource table structure and abstracting this to macros. If krealloc approach works, dev->res.port_resource[i].start would even still work, if not, it's easier to alter the pnp resource table and the macros internally.
Externally in drivers yes. Internally in code no - it makes the code harder to work with.
Quoted 2 lines
Quoted 2 lines
Yes, I dont know how he intends to deal with this (nor, in fact, just how dynamic things are supposed to end up to begin with) so over to Thomas.

Quoted 7 lines

Quoted 4 lines
Krealloc should only get used at early pnp init time, when the BIOS structures are parsed. The devices shouldn't be active then... A bit of a problem, as said, could be the sysfs interfaces, there it must be insured krealloc is not used anymore.
I don't think its that simple but that can be dealt with one the changes are in place if the objects are sensibly laid out.

I hope it is, stay tuned there will come something soon... If it's not that easy, another structure would be needed and every dev->res.port_resource[i].start and friends need to be touched (I don't see how this could still be resolved in a simple array then...).

Thomas