PnP model

PnP model

[Jaroslav Kysela]

Hi all,

	I think that we need to discuss deeply the right PnP model. The 
actual changes proposed by Adam are going to be more and more complex 
without allowing the user interactions inside the "auto" steps. The 
auto-configuration might be good and bad as we all know, but having an 
method to skip it is necessary.

	I strongly vote to follow the same behaviour as PCI code does:
It means call the activation / enabling / setting functions from the 
probe() callbacks. Only the driver knows what's the best. Including 
the manual assignment of resources.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs

Re: PnP Model

[James Bottomley]

> I agree. A lot of drivers should be able to use one model for
> everything including "enable_device" stuff.  Right not its all a bit
> too detailed

Actually, while we're on the subject of PnP-ISA, I was wondering if we
could also migrate straight ISA to the device model probing (The 3c509
now looks a bit of a mess because it's got probes for EISA and MCA,
needs probes for PNP-ISA and still has to use Space.c for ISA).

If we just had a bus matching function that always matched and the
device probe would do its usual port etc. probe and return success if it
found a device or fail if it didn't, that would move ISA over reasonably
effectively.  The only nasty is multiple cards: here the probe routine
would have to detect the extra cards, allocate struct devices for them
and then catch them again when they come back in through the driver
probe routine (this will work today because all probes are single
threaded on the bus semaphore, obviously it will fail if probes ever
become multi-threaded).

The last issue is probably that we'd like the ISA probes to be run after
all the rest of the busses so that all resources in use in the system
have a good chance of being claimed and the ISA memory poking should
have the least potential for doing damage---this would require a change
in the way the device model works, I think, wouldn't it?

James

Re: PnP model

[Alan Cox]

On Mon, 2003-02-03 at 13:55, Jaroslav Kysela wrote:
> 	I strongly vote to follow the same behaviour as PCI code does:
> It means call the activation / enabling / setting functions from the 
> probe() callbacks. Only the driver knows what's the best. Including 
> the manual assignment of resources.

I agree. A lot of drivers should be able to use one model for everything
including "enable_device" stuff. Right now its all a bit too detailed.

Also the locking model seems very unclear and there are hot swappable ISA
bays

Re: PnP Model

[Alan Cox]

On Mon, 2003-02-03 at 15:31, James Bottomley wrote:
> The last issue is probably that we'd like the ISA probes to be run after
> all the rest of the busses so that all resources in use in the system

They need to run very early on in some ways. We don't want to assign a
PnP device over something we didnt know exists. We can scan the other
busses first safely but we can't activate devices or do anything else
until the ISA unsafe probes run. Those also have some very careful
ordering especially in networking. NE2000 must run early, other probes
can make some cards move around so must also be ordered

Re: PnP model

[Adam Belay]

On Mon, Feb 03, 2003 at 02:55:37PM +0100, Jaroslav Kysela wrote:
> Hi all,
>
> 	I think that we need to discuss deeply the right PnP model. The

I'm confident this is the right pnp model.

> actual changes proposed by Adam are going to be more and more complex
> without allowing the user interactions inside the "auto" steps. The
> auto-configuration might be good and bad as we all know, but having an
> method to skip it is necessary.

In many cases, Auto configuration can be better then manual configuration.
1.) The auto configuration engine in my patch is able to resolve almost any
resource conflict and provides the greatest chance for all devices to have
resources allocated.
2.) Certainly some driver developers would like to manually set resources
but many may prefer the option to auto config.
3.) Drivers under the existing system are not aware of many forms of
resource conflicts and could set resources incorrectly.
4.) Some users do not want to worry about manual configuration and would
welcome an auto configuration system that makes intelligent choices
without user input.  This autoconfiguration system monitors many variables
that a user would have a hard time keeping track of and never overlooks any
potential conflicts in its analysis.

The above stated reasons are why I introduced these auto configuration
(Resource Management) improvements.

I feel that one solution is to support both manual and auto configuration so
the user can use what he or she prefers, however, I am confident that in most
cases the auto configuration will be the best option.

Perhaps I didn't make this clear earlier, it was not my intent to remove
support for manual resource configurations.  This was only temporary and
intended for the first release.  Here is a patch against my other 4
patches that updates the pnp code to support manual resource
configurations.  It also merges the pnpbios GPF fix and addresses a few
other issues.

-Adam

P.S.: The new manual config API is pnp_manual_config_dev.

P.S.: I'm currently working on an improved user interface that will allow
the user to manually set resources through the sysfs interface.  It will
include reports on where the conflicts are in order to assist the user in
making decisions.

P.S.: I intend to resolve all of these issues but my top priority is to
convert pnp drivers.  For example, I recently submitted pnp driver
conversions and am currently working on pnp card service revisions.



This patch needs to be tested.  I appreciate any and all feedback.

diff -urN a/drivers/pnp/base.h b/drivers/pnp/base.h
--- a/drivers/pnp/base.h	Sun Feb  2 11:40:10 2003
+++ b/drivers/pnp/base.h	Mon Feb  3 18:11:16 2003
@@ -4,7 +4,6 @@
 extern int pnp_interface_attach_device(struct pnp_dev *dev);
 extern void pnp_name_device(struct pnp_dev *dev);
 extern void pnp_fixup_device(struct pnp_dev *dev);
-extern int pnp_configure_device(struct pnp_dev *dev);
 extern void pnp_free_resources(struct pnp_resources *resources);
 extern int __pnp_add_device(struct pnp_dev *dev);
 extern void __pnp_remove_device(struct pnp_dev *dev);
diff -urN a/drivers/pnp/core.c b/drivers/pnp/core.c
--- a/drivers/pnp/core.c	Sun Feb  2 11:40:10 2003
+++ b/drivers/pnp/core.c	Mon Feb  3 18:06:33 2003
@@ -122,7 +122,7 @@
 	list_add_tail(&dev->global_list, &pnp_global);
 	list_add_tail(&dev->protocol_list, &dev->protocol->devices);
 	spin_unlock(&pnp_lock);
-	pnp_configure_device(dev);
+	pnp_auto_config_dev(dev);
 	ret = device_register(&dev->dev);
 	if (ret == 0)
 		pnp_interface_attach_device(dev);
diff -urN a/drivers/pnp/interface.c b/drivers/pnp/interface.c
--- a/drivers/pnp/interface.c	Sun Feb  2 12:26:25 2003
+++ b/drivers/pnp/interface.c	Mon Feb  3 17:37:32 2003
@@ -232,10 +232,11 @@
 	char *str = buf;
 	int i;

-	if (!dev->active){
-		str += sprintf(str,"DISABLED\n");
-		goto done;
-	}
+	str += sprintf(str,"state = ");
+	if (dev->active)
+		str += sprintf(str,"active\n");
+	else
+		str += sprintf(str,"disabled\n");
 	for (i = 0; i < PNP_MAX_PORT; i++) {
 		if (pnp_port_valid(dev, i)) {
 			str += sprintf(str,"io");
@@ -280,22 +281,6 @@
 	num_args = sscanf(buf,"%10s %i",command,&depnum);
 	if (!num_args)
 		goto done;
-	if (!strnicmp(command,"lock",4)) {
-		if (dev->active) {
-			dev->lock_resources = 1;
-		} else {
-			error = -EINVAL;
-		}
-		goto done;
-	}
-	if (!strnicmp(command,"unlock",6)) {
-		if (dev->lock_resources) {
-			dev->lock_resources = 0;
-		} else {
-			error = -EINVAL;
-		}
-		goto done;
-	}
 	if (!strnicmp(command,"disable",7)) {
 		error = pnp_disable_dev(dev);
 		goto done;
diff -urN a/drivers/pnp/pnpbios/core.c b/drivers/pnp/pnpbios/core.c
--- a/drivers/pnp/pnpbios/core.c	Sun Feb  2 19:25:15 2003
+++ b/drivers/pnp/pnpbios/core.c	Mon Feb  3 15:43:06 2003
@@ -142,11 +142,13 @@
 set_limit(cpu_gdt_table[cpu][(selname) >> 3], size); \
 } while(0)
 
+static struct desc_struct bad_bios_desc = { 0, 0x00409200 };
+
 /*
  * At some point we want to use this stack frame pointer to unwind
- * after PnP BIOS oopses. 
+ * after PnP BIOS oopses.
  */
- 
+
 u32 pnp_bios_fault_esp;
 u32 pnp_bios_fault_eip;
 u32 pnp_bios_is_utter_crap = 0;
@@ -160,6 +162,8 @@
 {
 	unsigned long flags;
 	u16 status;
+	struct desc_struct save_desc_40;
+	int cpu;
 
 	/*
 	 * PnP BIOSes are generally not terribly re-entrant.
@@ -168,6 +172,10 @@
 	if(pnp_bios_is_utter_crap)
 		return PNP_FUNCTION_NOT_SUPPORTED;
 
+	cpu = get_cpu();
+	save_desc_40 = cpu_gdt_table[cpu][0x40 / 8];
+	cpu_gdt_table[cpu][0x40 / 8] = bad_bios_desc;
+
 	/* On some boxes IRQ's during PnP BIOS calls are deadly.  */
 	spin_lock_irqsave(&pnp_bios_lock, flags);
 
@@ -207,6 +215,9 @@
 		: "memory"
 	);
 	spin_unlock_irqrestore(&pnp_bios_lock, flags);
+
+	cpu_gdt_table[cpu][0x40 / 8] = save_desc_40;
+	put_cpu();
 	
 	/* If we get here and this is set then the PnP BIOS faulted on us. */
 	if(pnp_bios_is_utter_crap)
@@ -236,7 +247,8 @@
 	void *p = kmalloc( size, f );
 	if ( p == NULL )
 		printk(KERN_ERR "PnPBIOS: kmalloc() failed\n");
-	memset(p, 0, size);
+	else
+		memset(p, 0, size);
 	return p;
 }
 
@@ -886,14 +898,7 @@
 
 	for(nodenum=0; nodenum<0xff; ) {
 		u8 thisnodenum = nodenum;
-		/* We build the list from the "boot" config because
-		 * we know that the resources couldn't have changed
-		 * at this stage.  Furthermore some buggy PnP BIOSes
-		 * will crash if we request the "current" config
-		 * from devices that are can only be static such as
-		 * those controlled by the "system" driver.
-		 */
-		if (pnp_bios_get_dev_node(&nodenum, (char )1, node))
+		if (pnp_bios_get_dev_node(&nodenum, (char )0, node))
 			break;
 		nodes_got++;
 		dev =  pnpbios_kmalloc(sizeof (struct pnp_dev), GFP_KERNEL);
@@ -996,6 +1001,8 @@
 		pnp_bios_callpoint.segment = PNP_CS16;
 		pnp_bios_hdr = check;
 
+		set_base(bad_bios_desc, __va((unsigned long)0x40 << 4));
+		_set_limit((char *)&bad_bios_desc, 4095 - (0x40 << 4));
 		for(i=0; i < NR_CPUS; i++)
 		{
 			Q2_SET_SEL(i, PNP_CS32, &pnp_bios_callfunc, 64 * 1024);
diff -urN a/drivers/pnp/resource.c b/drivers/pnp/resource.c
--- a/drivers/pnp/resource.c	Sun Feb  2 18:43:34 2003
+++ b/drivers/pnp/resource.c	Mon Feb  3 19:29:12 2003
@@ -989,7 +989,8 @@
 {
 	struct pnp_change * change = pnp_add_change(parent,dev);
 	move--;
-	spin_lock(&pnp_lock);
+	if (!dev->rule)
+		goto fail;
 	if (!pnp_can_configure(dev))
 		goto fail;
 	if (!dev->rule->depnum) {
@@ -1001,7 +1002,6 @@
 			goto fail;
 		pnp_init_resource_table(&dev->res);
 	}
-	spin_unlock(&pnp_lock);
 	if (!parent) {
 		pnp_free_changes(change);
 		kfree(change);
@@ -1009,7 +1009,6 @@
 	return 1;
 
 fail:
-	spin_unlock(&pnp_lock);
 	if (!parent)
 		kfree(change);
 	return 0;
@@ -1027,19 +1026,25 @@
 			return -ENOMEM;
 	}
 	for (move = 1; move <= pnp_max_moves; move++) {
+		spin_lock(&pnp_lock);
 		dev->rule->depnum = 0;
 		pnp_init_resource_table(&dev->res);
-		if (pnp_next_config(dev,move,NULL))
+		if (pnp_next_config(dev,move,NULL)) {
+			spin_unlock(&pnp_lock);
 			return 1;
+		}
+		spin_unlock(&pnp_lock);
 	}
 
+	spin_lock(&pnp_lock);
 	pnp_init_resource_table(&dev->res);
 	dev->rule->depnum = 0;
-	pnp_err("res: Unable to resolve resource conflicts for the device '%s', this device will not be usable.", dev->dev.bus_id);
+	spin_unlock(&pnp_lock);
+	pnp_err("res: Unable to resolve resource conflicts for the device '%s', some devices may not be usable.", dev->dev.bus_id);
 	return 0;
 }
 
-static int pnp_process_active(struct pnp_dev *dev)
+static int pnp_resolve_conflicts(struct pnp_dev *dev)
 {
 	int i;
 	struct pnp_dev * cdev;
@@ -1079,24 +1084,6 @@
 	return 1;
 }
 
-/**
- * pnp_configure_device - determines the best possible resource configuration based on available information
- * @dev: pointer to the desired device
- */
-
-int pnp_configure_device(struct pnp_dev *dev)
-{
-	int error;
-	if(!dev)
-		return -EINVAL;
-	if(dev->active) {
-		error = pnp_process_active(dev);
-	} else {
-		error = pnp_generate_config(dev);
-	}
-	return error;
-}
-
 static int pnp_compare_resources(struct pnp_resource_table * resa, struct pnp_resource_table * resb)
 {
 	int idx;
@@ -1128,6 +1115,92 @@
  * PnP Device Resource Management
  */
 
+
+/**
+ * pnp_resource_change - change one resource
+ * @resource: pointer to resource to be changed
+ * @start: start of region
+ * @size: size of region
+ *
+ */
+
+void pnp_resource_change(struct resource *resource, unsigned long start, unsigned long size)
+{
+	if (resource == NULL)
+		return;
+	resource->flags &= ~IORESOURCE_AUTO;
+	resource->start = start;
+	resource->end = start + size - 1;
+}
+
+/**
+ * pnp_auto_config_dev - determines the best possible resource configuration based on available information
+ * @dev: pointer to the desired device
+ */
+
+int pnp_auto_config_dev(struct pnp_dev *dev)
+{
+	int error;
+	if(!dev)
+		return -EINVAL;
+
+	dev->config_mode = PNP_CONFIG_AUTO;
+
+	if(dev->active)
+		error = pnp_resolve_conflicts(dev);
+	else
+		error = pnp_generate_config(dev);
+	return error;
+}
+
+/**
+ * pnp_manual_config_dev - Disables Auto Config and Manually sets the resource table
+ * @dev: pointer to the desired device
+ * @res: pointer to the new resource config
+ */
+
+int pnp_manual_config_dev(struct pnp_dev *dev, struct pnp_resource_table * res, int mode)
+{
+	int i;
+	struct pnp_resource_table bak = dev->res;
+	if (!dev || !res)
+		return -EINVAL;
+	if (dev->active)
+		return -EBUSY;
+	spin_lock(&pnp_lock);
+	dev->res = *res;
+
+	if (!(mode & PNP_CONFIG_FORCE)) {
+		for (i = 0; i < PNP_MAX_PORT; i++) {
+			if(pnp_check_port(dev,i))
+				goto fail;
+		}
+		for (i = 0; i < PNP_MAX_MEM; i++) {
+			if(pnp_check_mem(dev,i))
+				goto fail;
+		}
+		for (i = 0; i < PNP_MAX_IRQ; i++) {
+			if(pnp_check_irq(dev,i))
+				goto fail;
+		}
+		for (i = 0; i < PNP_MAX_DMA; i++) {
+			if(pnp_check_dma(dev,i))
+				goto fail;
+		}
+	}
+	spin_unlock(&pnp_lock);
+
+	if (mode & PNP_CONFIG_RESOLVE)
+		pnp_resolve_conflicts(dev);
+
+	dev->config_mode = PNP_CONFIG_MANUAL;
+
+fail:
+	dev->res = bak;
+	spin_unlock(&pnp_lock);
+	return -EINVAL;
+}
+
 /**
  * pnp_activate_dev - activates a PnP device for use
  * @dev: pointer to the desired device
@@ -1141,11 +1214,27 @@
 		return -EINVAL;
 	if (dev->active) {
 		pnp_info("res: The PnP device '%s' is already active.", dev->dev.bus_id);
+		return -EBUSY;
+	}
+	/* if the auto config failed, try again now, because this device was requested before others it's best to find a config at all costs */
+	if (!pnp_is_active(dev)) {
+		spin_lock(&pnp_lock);
+		if (!pnp_next_config(dev,1,NULL)) {
+			pnp_init_resource_table(&dev->res);
+			dev->rule->depnum = 0;
+			pnp_err("res: Unable to resolve resource conflicts for the device '%s'", dev->dev.bus_id);
+			spin_unlock(&pnp_lock);
+			return -EBUSY;
+		}
+		spin_unlock(&pnp_lock);
+	}
+	if (dev->config_mode & PNP_CONFIG_INVALID) {
+		pnp_info("res: Unable to activate the PnP device '%s' because its resource configuration is invalid", dev->dev.bus_id);
 		return -EINVAL;
 	}
 	if (dev->status != PNP_READY && dev->status != PNP_ATTACHED){
 		pnp_err("res: Activation failed because the PnP device '%s' is busy", dev->dev.bus_id);
-		return -EINVAL;
+		return -EBUSY;
 	}
 	if (!pnp_can_write(dev)) {
 		pnp_info("res: Unable to activate the PnP device '%s' because this feature is not supported", dev->dev.bus_id);
@@ -1165,7 +1254,10 @@
 	} else
 		dev->active = pnp_is_active(dev);
 	pnp_dbg("res: the device '%s' has been activated", dev->dev.bus_id);
-	kfree(dev->rule);
+	if (dev->rule) {
+		kfree(dev->rule);
+		dev->rule = NULL;
+	}
 	return 0;
 }
 
@@ -1200,22 +1292,6 @@
 	return 0;
 }
 
-/**
- * pnp_resource_change - change one resource
- * @resource: pointer to resource to be changed
- * @start: start of region
- * @size: size of region
- *
- */
-
-void pnp_resource_change(struct resource *resource, unsigned long start, unsigned long size)
-{
-	if (resource == NULL)
-		return;
-	resource->flags &= ~IORESOURCE_AUTO;
-	resource->start = start;
-	resource->end = start + size - 1;
-}
 
 EXPORT_SYMBOL(pnp_build_resource);
 EXPORT_SYMBOL(pnp_find_resources);
diff -urN a/drivers/pnp/support.c b/drivers/pnp/support.c
--- a/drivers/pnp/support.c	Sun Feb  2 18:43:34 2003
+++ b/drivers/pnp/support.c	Mon Feb  3 15:49:22 2003
@@ -168,7 +168,7 @@
 				break;
 			}
 			} /* switch */
-                        p += len + 3;
+			p += len + 3;
 			continue;
 		} /* end large tag */
 
@@ -410,8 +410,8 @@
 			possible_dma(p,len,depnum,dev);
 			break;
 		}
-                case SMALL_TAG_STARTDEP:
-                {
+		case SMALL_TAG_STARTDEP:
+		{
 			if (len > 1)
 				goto sm_err;
 			dependent = 0x100 | PNP_RES_PRIORITY_ACCEPTABLE;
@@ -419,15 +419,15 @@
 				dependent = 0x100 | p[1];
 			pnp_build_resource(dev,dependent);
 			depnum = pnp_get_max_depnum(dev);
-                        break;
-                }
-                case SMALL_TAG_ENDDEP:
-                {
+			break;
+		}
+		case SMALL_TAG_ENDDEP:
+		{
 			if (len != 0)
 				goto sm_err;
 			depnum = 0;
-                        break;
-                }
+			break;
+		}
 		case SMALL_TAG_PORT:
 		{
 			if (len != 7)
diff -urN a/include/linux/pnp.h b/include/linux/pnp.h
--- a/include/linux/pnp.h	Sun Feb  2 18:43:34 2003
+++ b/include/linux/pnp.h	Mon Feb  3 19:07:00 2003
@@ -253,7 +253,7 @@
 	struct pnp_resource_table	res;		/* contains the currently chosen resources */
 	struct pnp_resources	      * possible;	/* a list of possible resources */
 	struct pnp_rule_table	      * rule;		/* the current possible resource set */
-	int 				lock_resources;	/* resources are locked */
+	int 				config_mode;	/* flags that determine how the device's resources should be configured */
 
 	void * protocol_data;		/* Used to store protocol specific data */
 	unsigned short	regs;		/* ISAPnP: supported registers */
@@ -300,6 +300,13 @@
 	void (*quirk_function)(struct pnp_dev *dev);	/* fixup function */
 };
 
+/* config modes */
+#define PNP_CONFIG_AUTO		0x0001	/* Use the Resource Configuration Engine to determine resource settings */
+#define PNP_CONFIG_MANUAL	0x0002	/* the config has been manually specified */
+#define PNP_CONFIG_FORCE	0x0003	/* disables validity checking */
+#define PNP_CONFIG_RESOLVE	0x0008	/* moves other configs out of the way, use only when absolutely necessary */
+#define PNP_CONFIG_INVALID	0x0010	/* If this flag is set, the pnp layer will refuse to activate the device */
+
 /* capabilities */
 #define PNP_READ		0x0001
 #define PNP_WRITE		0x0002
@@ -313,7 +320,7 @@
 				 ((dev)->capabilities & PNP_WRITE))
 #define pnp_can_disable(dev)	(((dev)->protocol) && ((dev)->protocol->disable) && \
 				 ((dev)->capabilities & PNP_DISABLE))
-#define pnp_can_configure(dev)	((!(dev)->active) && (!(dev)->lock_resources) && \
+#define pnp_can_configure(dev)	((!(dev)->active) && ((dev)->config_mode & PNP_CONFIG_AUTO) && \
 				 ((dev)->capabilities & PNP_CONFIGURABLE))
 
 /* status */
@@ -377,6 +384,8 @@
 int pnp_activate_dev(struct pnp_dev *dev);
 int pnp_disable_dev(struct pnp_dev *dev);
 void pnp_resource_change(struct resource *resource, unsigned long start, unsigned long size);
+int pnp_manual_config_dev(struct pnp_dev *dev, struct pnp_resource_table *res, int mode);
+int pnp_auto_config_dev(struct pnp_dev *dev);
 
 /* driver */
 int compare_pnp_id(struct pnp_id * pos, const char * id);
@@ -415,6 +424,8 @@
 static inline int pnp_activate_dev(struct pnp_dev *dev) { return -ENODEV; }
 static inline int pnp_disable_dev(struct pnp_dev *dev) { return -ENODEV; }
 static inline void pnp_resource_change(struct resource *resource, unsigned long start, unsigned long size) { ; }
+static inline int pnp_manual_config_dev(struct pnp_dev *dev, struct pnp_resource_table *res, int mode) { return -ENODEV; }
+static inline int pnp_auto_config_dev(struct pnp_dev *dev) { return -ENODEV; }
 
 /* driver */
 static inline int compare_pnp_id(struct list_head * id_list, const char * id) { return -ENODEV; }

Re: PnP model

[Russell King]

On Mon, Feb 03, 2003 at 08:43:25PM +0000, Adam Belay wrote:
> In many cases, Auto configuration can be better then manual configuration.
> 1.) The auto configuration engine in my patch is able to resolve almost any
> resource conflict and provides the greatest chance for all devices to have
> resources allocated.

There is a nice problem with ISA PNP serial ports generated by this
type of thing in 2.5.59.

On boot, we probe for the usual serial ports, and discover two at
0x3f8 and 0x2f8, and we request these resources.  Please note that
one port could be in use as a serial console from earlier in the
bootup.

Then, we move on to the PNP probes.  The PNP layer gives us two
serial ports, and we initialise them.  The PNP layer notices that
the resources for 0x3f8 and 0x2f8, and re-assigns the ports to
0x3e8 and 0x2e8.  The serial layer finds two extra ports at
0x3e8 and 0x2e8.

However, the ports at 0x3f8 and 0x2f8 are now gone, along with the
serial console, and the boot messages claim that we have four serial
ports at ttyS0, ttyS1, ttyS2, and ttyS3, when the machine in fact
only has two serial ports.  The serial layer likewise believes we
have four serial ports.

I look forward to your thoughts on getting around this problem.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: PnP model

[Jaroslav Kysela]

On Tue, 4 Feb 2003, Russell King wrote:

> On Mon, Feb 03, 2003 at 08:43:25PM +0000, Adam Belay wrote:
> > In many cases, Auto configuration can be better then manual configuration.
> > 1.) The auto configuration engine in my patch is able to resolve almost any
> > resource conflict and provides the greatest chance for all devices to have
> > resources allocated.
> 
> There is a nice problem with ISA PNP serial ports generated by this
> type of thing in 2.5.59.
> 
> On boot, we probe for the usual serial ports, and discover two at
> 0x3f8 and 0x2f8, and we request these resources.  Please note that
> one port could be in use as a serial console from earlier in the
> bootup.
> 
> Then, we move on to the PNP probes.  The PNP layer gives us two
> serial ports, and we initialise them.  The PNP layer notices that
> the resources for 0x3f8 and 0x2f8, and re-assigns the ports to
> 0x3e8 and 0x2e8.  The serial layer finds two extra ports at
> 0x3e8 and 0x2e8.
> 
> However, the ports at 0x3f8 and 0x2f8 are now gone, along with the
> serial console, and the boot messages claim that we have four serial
> ports at ttyS0, ttyS1, ttyS2, and ttyS3, when the machine in fact
> only has two serial ports.  The serial layer likewise believes we
> have four serial ports.
> 
> I look forward to your thoughts on getting around this problem.

I think that legacy devices must be probed after PnP ones, otherwise 
you'll get these duplications.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs

Re: PnP model

[Jaroslav Kysela]

On Mon, 3 Feb 2003, Adam Belay wrote:

> On Mon, Feb 03, 2003 at 02:55:37PM +0100, Jaroslav Kysela wrote:
> > Hi all,
> >
> > 	I think that we need to discuss deeply the right PnP model. The
> 
> I'm confident this is the right pnp model.
> 
> > actual changes proposed by Adam are going to be more and more complex
> > without allowing the user interactions inside the "auto" steps. The
> > auto-configuration might be good and bad as we all know, but having an
> > method to skip it is necessary.
> 
> In many cases, Auto configuration can be better then manual configuration.

Autoconfiguration is better in perfect world... There are always troubles 
with some hardware incompatibilities.

> 1.) The auto configuration engine in my patch is able to resolve almost any
> resource conflict and provides the greatest chance for all devices to have
> resources allocated.
> 2.) Certainly some driver developers would like to manually set resources
> but many may prefer the option to auto config.
> 3.) Drivers under the existing system are not aware of many forms of
> resource conflicts and could set resources incorrectly.
> 4.) Some users do not want to worry about manual configuration and would
> welcome an auto configuration system that makes intelligent choices
> without user input.  This autoconfiguration system monitors many variables
> that a user would have a hard time keeping track of and never overlooks any
> potential conflicts in its analysis.
> 
> The above stated reasons are why I introduced these auto configuration
> (Resource Management) improvements.
> 
> I feel that one solution is to support both manual and auto configuration so
> the user can use what he or she prefers, however, I am confident that in most
> cases the auto configuration will be the best option.

But do we have to enable the auto configuration at boot implicitly?
I don't think so. Again, it's better if driver decides if auto or manual 
configuration will be used. The step by step configuration makes the 
implemenation more rebust (user will know which driver fails). Also, 
having legacy devices in the system, you cannot determine which resources 
are required for them before appropriate driver detects and initializes 
the device. I don't think that this algorithm will be worse than your 
proposed one. There are usually no resource conflicts for PnP devices so 
it will work at least on the same number of machines like implicit 
autoconfiguration at boot.

Also, imagine that PnP driver is build into the kernel: you'll no chance
select the right configuration over sysfs, but there are __setup() macros
allowing to pass the right resources to a driver.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs

Re: PnP model

[Russell King]

On Tue, Feb 04, 2003 at 11:18:36AM +0100, Jaroslav Kysela wrote:
> I think that legacy devices must be probed after PnP ones, otherwise 
> you'll get these duplications.

Unfortunately, this isn't easy to do, while keeping stuff like serial
consoles working from early at bootup.  Alan Cox definitely does not
want to see the serial console initialised any later than it is today,
and he's not the only one.

In addition, the "legacy" devices are part of the 8250.c module - they
have to be for serial console.  The PNP devices are probed as part of
the 8250_pnp.c module, and since 8250_pnp.c depends on 8250.c, the
serial PNP ports will _always_ be initialised after the ISA probes.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: PnP Model

[James Bottomley]

On Mon, 2003-02-03 at 10:41, Alan Cox wrote:
> On Mon, 2003-02-03 at 15:31, James Bottomley wrote:
> > The last issue is probably that we'd like the ISA probes to be run after
> > all the rest of the busses so that all resources in use in the system
> 
> They need to run very early on in some ways. We don't want to assign a
> PnP device over something we didnt know exists. We can scan the other
> busses first safely but we can't activate devices or do anything else
> until the ISA unsafe probes run. Those also have some very careful
> ordering especially in networking. NE2000 must run early, other probes
> can make some cards move around so must also be ordered


It strikes me that we can make use of the bus matching logic for this. 
For ISA, since we have no concept of a card id, we could instead match
on when the binding should occur (i.e. ISA_BIND_EARLY and
ISA_BIND_LATE).  Thus each driver picks its binding type and we run bind
early before all other busses and bind late after them (probably by
simulating a hotplug event that says hey I suddenly found a bunch of ISA
cards of type ISA_BIND_LATE).  It's a bit of an abuse of what bus
matching is supposed to do, but it should work for the purpose

James

RE: PnP model

[Grover, Andrew]

> From: Adam Belay [mailto:ambx1@neo.rr.com] 
> In many cases, Auto configuration can be better then manual 
> configuration.
> 1.) The auto configuration engine in my patch is able to 
> resolve almost any
> resource conflict and provides the greatest chance for all 
> devices to have
> resources allocated.
> 2.) Certainly some driver developers would like to manually 
> set resources
> but many may prefer the option to auto config.

I think the people who want to manually configure their device's
resources need to step up and justify why this is really necessary.

If someone is manually configuring something, that means the automatic
config *failed*. Why did it fail? It should never fail. Manual config is
only giving the user to opportunity to get something wrong.

Regards -- Andy

Re: PnP model

[John Bradford]

> > In many cases, Auto configuration can be better then manual 
> > configuration.
> > 1.) The auto configuration engine in my patch is able to 
> > resolve almost any
> > resource conflict and provides the greatest chance for all 
> > devices to have
> > resources allocated.
> > 2.) Certainly some driver developers would like to manually 
> > set resources
> > but many may prefer the option to auto config.
> 
> I think the people who want to manually configure their device's
> resources need to step up and justify why this is really necessary.

Prototyping an embedded system, maybe, where you have devices in the
test box that won't be in the production machine.  You would want them
to use resources other than those that you want the hardware which
will be present to use.

> If someone is manually configuring something, that means the automatic
> config *failed*.

Not necessarily.

> Why did it fail? It should never fail. Manual config is only giving
> the user to opportunity to get something wrong.

Agreed, auto configuration should never fail, but that doesn't mean
that you shouldn't have manual configuration as an option.

John.

RE: PnP model

[Grover, Andrew]

> From: John Bradford [mailto:john@grabjohn.com] 
> > I think the people who want to manually configure their device's
> > resources need to step up and justify why this is really necessary.
> 
> Prototyping an embedded system, maybe, where you have devices in the
> test box that won't be in the production machine.  You would want them
> to use resources other than those that you want the hardware which
> will be present to use.

Ok fair enough. But I think the drivers should always think things are
handled in a PnP manner, even if they really aren't. ;-) For example,
between the stages where PnP enumerates the devices and the stage where
drivers get device_add notifications as a result of that, we will be
assigning the system resources to each device, but we could also
implement a way at this stage for people to manually alter things. I
think this is the right place to do this, as opposed to having all the
drivers implement code to probe for themselves.

Thoughts?

Regards -- Andy

RE: PnP model

[Alan Cox]

On Tue, 2003-02-04 at 19:53, Grover, Andrew wrote:
> Ok fair enough. But I think the drivers should always think things are
> handled in a PnP manner, even if they really aren't. ;-) For example,

If this was a post 2.6 argument I would agree. The notion of removing all the
horribly resource code and having ISA legacy drivers probe code do

	my_dev = isa_create_device(&resourceinfo, "soundblaster16");
	my_dev->set_irq = foo;

etc type stuff is nice

Re: PnP model

[Adam Belay]

On Tue, Feb 04, 2003 at 10:49:37AM +0000, Russell King wrote:
> On Tue, Feb 04, 2003 at 11:18:36AM +0100, Jaroslav Kysela wrote:
> > I think that legacy devices must be probed after PnP ones, otherwise
> > you'll get these duplications.
> 
> Unfortunately, this isn't easy to do, while keeping stuff like serial
> consoles working from early at bootup.  Alan Cox definitely does not
> want to see the serial console initialised any later than it is today,
> and he's not the only one.
> 
> In addition, the "legacy" devices are part of the 8250.c module - they
> have to be for serial console.  The PNP devices are probed as part of
> the 8250_pnp.c module, and since 8250_pnp.c depends on 8250.c, the
> serial PNP ports will _always_ be initialised after the ISA probes.
>

I think I have an alternative solution.  If we add support for current
resource configs and don't reset the cards, we can determine what the
active configuration was when the serial driver detects the isapnp card.
Because the device will remain active the resources will not be changed.
Below is a patch against my previous 5 patches.  This patch also
contains a few fixes and cleanups.  Could you please test this and let
me know if it solves the problem.

Also I noticed that the serial driver legacy probe, independent of my
changes, sometimes detects the incorrect irq, is proper irq detection
needed by the serial driver or will it still work properly even if
that information is detected incorrectly?

Thanks,
Adam


P.S.: ISAPnP MEM32 writing and reading still needs a little work.
P.S.: PNPBIOS MEM32s are fully supported.

diff -urN a/drivers/pnp/interface.c b/drivers/pnp/interface.c
--- a/drivers/pnp/interface.c	Mon Feb  3 19:44:01 2003
+++ b/drivers/pnp/interface.c	Tue Feb  4 20:58:01 2003
@@ -265,7 +265,6 @@
 			str += sprintf(str," %ld \n", pnp_dma(dev, i));
 		}
 	}
-	done:
 	return (str - buf);
 }
 
diff -urN a/drivers/pnp/isapnp/core.c b/drivers/pnp/isapnp/core.c
--- a/drivers/pnp/isapnp/core.c	Sun Feb  2 18:43:34 2003
+++ b/drivers/pnp/isapnp/core.c	Wed Feb  5 21:06:09 2003
@@ -53,7 +53,7 @@
 
 int isapnp_disable;			/* Disable ISA PnP */
 int isapnp_rdp;				/* Read Data Port */
-int isapnp_reset = 1;			/* reset all PnP cards (deactivate) */
+int isapnp_reset = 0;			/* reset all PnP cards (deactivate) */
 int isapnp_skip_pci_scan;		/* skip PCI resource scanning */
 int isapnp_verbose = 1;			/* verbose mode */
 
@@ -259,7 +259,7 @@
 		 *	We cannot use NE2000 probe spaces for ISAPnP or we
 		 *	will lock up machines.
 		 */
-		if ((rdp < 0x280 || rdp >  0x380) && !check_region(rdp, 1)) 
+		if ((rdp < 0x280 || rdp >  0x380) && !check_region(rdp, 1))
 		{
 			isapnp_rdp = rdp;
 			return 0;
@@ -435,6 +435,7 @@
 /*
  *  Parse logical device tag.
  */
+static int isapnp_get_resources(struct pnp_dev *dev, struct pnp_resource_table * res);
 
 static struct pnp_dev * __init isapnp_parse_device(struct pnp_card *card, int size, int number)
 {
@@ -765,7 +766,7 @@
 /*
  *  Parse resource map for ISA PnP card.
  */
- 
+
 static void __init isapnp_parse_resource_map(struct pnp_card *card)
 {
 	unsigned char type, tmp[17];
@@ -822,7 +823,7 @@
 {
 	int i, j;
 	unsigned char checksum = 0x6a, bit, b;
-	
+
 	for (i = 0; i < 8; i++) {
 		b = data[i];
 		for (j = 0; j < 8; j++) {
@@ -855,6 +856,63 @@
 	pnpc_add_id(id,card);
 }
 
+
+static int isapnp_parse_current_resources(struct pnp_dev *dev, struct pnp_resource_table * res)
+{
+	int tmp, ret;
+	struct pnp_rule_table rule;
+	if (dev->rule)
+		rule = *dev->rule;
+	else {
+		if (!pnp_generate_rule(dev,1,&rule))
+			return -EINVAL;
+	}
+
+	dev->active = isapnp_read_byte(ISAPNP_CFG_ACTIVATE);
+	if (dev->active) {
+		for (tmp = 0; tmp < PNP_MAX_PORT; tmp++) {
+			ret = isapnp_read_word(ISAPNP_CFG_PORT + (tmp << 1));
+			if (!ret)
+				continue;
+			res->port_resource[tmp].start = ret;
+			if (rule.port[tmp])
+				res->port_resource[tmp].end = ret + rule.port[tmp]->size - 1;
+			else
+				res->port_resource[tmp].end = ret + 1; /* all we can do is assume 1 :-( */
+			res->port_resource[tmp].flags = IORESOURCE_IO;
+		}
+		for (tmp = 0; tmp < PNP_MAX_MEM; tmp++) {
+			ret = isapnp_read_dword(ISAPNP_CFG_MEM + (tmp << 3));
+			if (!ret)
+				continue;
+			res->mem_resource[tmp].start = ret;
+			if (rule.mem[tmp])
+				res->mem_resource[tmp].end = ret + rule.mem[tmp]->size - 1;
+			else
+				res->mem_resource[tmp].end = ret + 1; /* all we can do is assume 1 :-( */
+			res->mem_resource[tmp].flags = IORESOURCE_MEM;
+		}
+		for (tmp = 0; tmp < PNP_MAX_IRQ; tmp++) {
+			ret = (isapnp_read_word(ISAPNP_CFG_IRQ + (tmp << 1)) >> 8);
+			if (!ret)
+				continue;
+			res->irq_resource[tmp].start = res->irq_resource[tmp].end = ret;
+			res->irq_resource[tmp].flags = IORESOURCE_IRQ;
+		}
+		for (tmp = 0; tmp < PNP_MAX_DMA; tmp++) {
+			ret = isapnp_read_byte(ISAPNP_CFG_DMA + tmp);
+			if (ret == 4)
+				continue;
+			if (rule.dma[tmp]) { /* some isapnp systems forget to set this to 4 so we have to check */
+				res->dma_resource[tmp].start = res->dma_resource[tmp].end = ret;
+				res->dma_resource[tmp].flags = IORESOURCE_DMA;
+			}
+		}
+	}
+	return 0;
+}
+
+
 /*
  *  Build device list for all present ISA PnP devices.
  */
@@ -864,6 +922,7 @@
 	int csn;
 	unsigned char header[9], checksum;
 	struct pnp_card *card;
+	struct pnp_dev *dev;
 
 	isapnp_wait();
 	isapnp_key();
@@ -896,8 +955,17 @@
 			printk(KERN_ERR "isapnp: checksum for device %i is not valid (0x%x)\n", csn, isapnp_checksum_value);
 		card->checksum = isapnp_checksum_value;
 		card->protocol = &isapnp_protocol;
+
+		/* read the current resource data */
+		card_for_each_dev(card,dev) {
+			isapnp_device(dev->number);
+		pnp_init_resource_table(&dev->res);
+			isapnp_parse_current_resources(dev, &dev->res);
+		}
+
 		pnpc_add_card(card);
 	}
+	isapnp_wait();
 	return 0;
 }
 
@@ -971,16 +1039,12 @@
 
 static int isapnp_get_resources(struct pnp_dev *dev, struct pnp_resource_table * res)
 {
-	/* We don't need to do anything but this, the rest is taken care of */
-	if (pnp_port_valid(dev, 0) == 0 &&
-	    pnp_mem_valid(dev, 0) == 0 &&
-	    pnp_irq_valid(dev, 0) == 0 &&
-	    pnp_dma_valid(dev, 0) == 0)
-		dev->active = 0;
-	else
-		dev->active = 1;
-	*res = dev->res;
-	return 0;
+	int ret;
+	pnp_init_resource_table(res);
+	isapnp_cfg_begin(dev->card->number, dev->number);
+	ret = isapnp_parse_current_resources(dev, res);
+	isapnp_cfg_end();
+	return ret;
 }
 
 static int isapnp_set_resources(struct pnp_dev *dev, struct pnp_resource_table * res)
@@ -989,18 +1053,19 @@
 
 	isapnp_cfg_begin(dev->card->number, dev->number);
 	dev->active = 1;
-	for (tmp = 0; tmp < 8 && res->port_resource[tmp].flags & IORESOURCE_IO; tmp++)
+	for (tmp = 0; tmp < PNP_MAX_PORT && res->port_resource[tmp].flags & IORESOURCE_IO; tmp++)
 		isapnp_write_word(ISAPNP_CFG_PORT+(tmp<<1), res->port_resource[tmp].start);
-	for (tmp = 0; tmp < 2 && res->irq_resource[tmp].flags & IORESOURCE_IRQ; tmp++) {
+	for (tmp = 0; tmp < PNP_MAX_IRQ && res->irq_resource[tmp].flags & IORESOURCE_IRQ; tmp++) {
 		int irq = res->irq_resource[tmp].start;
 		if (irq == 2)
 			irq = 9;
 		isapnp_write_byte(ISAPNP_CFG_IRQ+(tmp<<1), irq);
 	}
-	for (tmp = 0; tmp < 2 && res->dma_resource[tmp].flags & IORESOURCE_DMA; tmp++)
+	for (tmp = 0; tmp < PNP_MAX_DMA && res->dma_resource[tmp].flags & IORESOURCE_DMA; tmp++)
 		isapnp_write_byte(ISAPNP_CFG_DMA+tmp, res->dma_resource[tmp].start);
-	for (tmp = 0; tmp < 4 && res->mem_resource[tmp].flags & IORESOURCE_MEM; tmp++)
+	for (tmp = 0; tmp < PNP_MAX_MEM && res->mem_resource[tmp].flags & IORESOURCE_MEM; tmp++)
 		isapnp_write_word(ISAPNP_CFG_MEM+(tmp<<2), (res->mem_resource[tmp].start >> 8) & 0xffff);
+	/* FIXME: We aren't handling 32bit mems properly here */
 	isapnp_activate(dev->number);
 	isapnp_cfg_end();
 	return 0;
@@ -1010,7 +1075,7 @@
 {
 	if (!dev || !dev->active)
 		return -EINVAL;
-      	isapnp_cfg_begin(dev->card->number, dev->number);
+	isapnp_cfg_begin(dev->card->number, dev->number);
 	isapnp_deactivate(dev->number);
 	dev->active = 0;
 	isapnp_cfg_end();
diff -urN a/drivers/pnp/resource.c b/drivers/pnp/resource.c
--- a/drivers/pnp/resource.c	Mon Feb  3 19:44:02 2003
+++ b/drivers/pnp/resource.c	Wed Feb  5 20:01:16 2003
@@ -725,6 +725,12 @@
 	return 0;
 }
 
+/**
+ * pnp_init_resource_table - Resets a resource table to default values.
+ * @table: pointer to the desired resource table
+ *
+ */
+
 void pnp_init_resource_table(struct pnp_resource_table *table)
 {
 	int idx;
@@ -787,6 +793,8 @@
 static struct pnp_change * pnp_add_change(struct pnp_change * parent, struct pnp_dev * dev)
 {
 	struct pnp_change * change = pnp_alloc(sizeof(struct pnp_change));
+	if (!change)
+		return NULL;
 	change->res_bak = dev->res;
 	change->rule_bak = *dev->rule;
 	change->dev = dev;
@@ -805,7 +813,15 @@
 		list_splice_init(&change->changes, &parent->changes);
 }
 
-static int pnp_generate_rule(struct pnp_dev *dev, int depnum)
+/**
+ * pnp_generate_rule - Creates a rule table structure based on depnum and device.
+ * @dev: pointer to the desired device
+ * @depnum: dependent function, if not valid will return an error
+ * @rule: pointer to a rule structure to record data to
+ *
+ */
+
+int pnp_generate_rule(struct pnp_dev * dev, int depnum, struct pnp_rule_table * rule)
 {
 	int nport = 0, nirq = 0, ndma = 0, nmem = 0;
 	struct pnp_resources * res;
@@ -813,7 +829,6 @@
 	struct pnp_mem * mem;
 	struct pnp_irq * irq;
 	struct pnp_dma * dma;
-	struct pnp_rule_table * rule = dev->rule;
 
 	if (depnum <= 0 || !rule)
 		return -EINVAL;
@@ -905,7 +920,7 @@
 		for (; depnum <= max; depnum++) {
 			struct pnp_resources * res = pnp_find_resources(dev, depnum);
 			if (res->priority == priority) {
-				if(pnp_generate_rule(dev, depnum)) {
+				if(pnp_generate_rule(dev, depnum, dev->rule)) {
 					dev->rule->depnum = depnum;
 					return 1;
 				}
@@ -923,7 +938,7 @@
 	struct pnp_dev * cdev;
 
 	for (i = 0; i < PNP_MAX_PORT; i++) {
-		if (dev->res.port_resource[i].start == 0) {
+		if (dev->res.port_resource[i].start == 0 || pnp_check_port_conflicts(dev,i,SEARCH_WARM)) {
 			if (!pnp_next_port(dev,i))
 				return 0;
 		}
@@ -938,7 +953,7 @@
 		pnp_commit_changes(parent, change);
 	}
 	for (i = 0; i < PNP_MAX_MEM; i++) {
-		if (dev->res.mem_resource[i].start == 0) {
+		if (dev->res.mem_resource[i].start == 0 || pnp_check_mem_conflicts(dev,i,SEARCH_WARM)) {
 			if (!pnp_next_mem(dev,i))
 				return 0;
 		}
@@ -953,7 +968,7 @@
 		pnp_commit_changes(parent, change);
 	}
 	for (i = 0; i < PNP_MAX_IRQ; i++) {
-		if (dev->res.irq_resource[i].start == -1) {
+		if (dev->res.irq_resource[i].start == -1 || pnp_check_irq_conflicts(dev,i,SEARCH_WARM)) {
 			if (!pnp_next_irq(dev,i))
 				return 0;
 		}
@@ -968,7 +983,7 @@
 		pnp_commit_changes(parent, change);
 	}
 	for (i = 0; i < PNP_MAX_DMA; i++) {
-		if (dev->res.dma_resource[i].start == -1) {
+		if (dev->res.dma_resource[i].start == -1 || pnp_check_dma_conflicts(dev,i,SEARCH_WARM)) {
 			if (!pnp_next_dma(dev,i))
 				return 0;
 		}
@@ -989,6 +1004,8 @@
 {
 	struct pnp_change * change = pnp_add_change(parent,dev);
 	move--;
+	if (!change)
+		return 0;
 	if (!dev->rule)
 		goto fail;
 	if (!pnp_can_configure(dev))
@@ -1014,7 +1031,8 @@
 	return 0;
 }
 
-static int pnp_generate_config(struct pnp_dev * dev)
+/* this advanced algorithm will shuffle other configs to make room and ensure that the most possible devices have configs */
+static int pnp_advanced_config(struct pnp_dev * dev)
 {
 	int move;
 	/* if the device cannot be configured skip it */
@@ -1036,7 +1054,6 @@
 		spin_unlock(&pnp_lock);
 	}
 
-	spin_lock(&pnp_lock);
 	pnp_init_resource_table(&dev->res);
 	dev->rule->depnum = 0;
 	spin_unlock(&pnp_lock);
@@ -1054,7 +1071,7 @@
 		do {
 			cdev = pnp_check_port_conflicts(dev,i,SEARCH_COLD);
 			if (cdev)
-				pnp_generate_config(cdev);
+				pnp_advanced_config(cdev);
 		} while (cdev);
 	}
 	for (i = 0; i < PNP_MAX_MEM; i++)
@@ -1062,7 +1079,7 @@
 		do {
 			cdev = pnp_check_mem_conflicts(dev,i,SEARCH_COLD);
 			if (cdev)
-				pnp_generate_config(cdev);
+				pnp_advanced_config(cdev);
 		} while (cdev);
 	}
 	for (i = 0; i < PNP_MAX_IRQ; i++)
@@ -1070,7 +1087,7 @@
 		do {
 			cdev = pnp_check_irq_conflicts(dev,i,SEARCH_COLD);
 			if (cdev)
-				pnp_generate_config(cdev);
+				pnp_advanced_config(cdev);
 		} while (cdev);
 	}
 	for (i = 0; i < PNP_MAX_DMA; i++)
@@ -1078,12 +1095,59 @@
 		do {
 			cdev = pnp_check_dma_conflicts(dev,i,SEARCH_COLD);
 			if (cdev)
-				pnp_generate_config(cdev);
+				pnp_advanced_config(cdev);
 		} while (cdev);
 	}
 	return 1;
 }
 
+/* this is a much faster algorithm but it may not leave resources for other devices to use */
+static int pnp_simple_config(struct pnp_dev * dev)
+{
+	int i;
+	spin_lock(&pnp_lock);
+	if (dev->active) {
+		spin_unlock(&pnp_lock);
+		return 1;
+	}
+	dev->rule->depnum = 0;
+	pnp_init_resource_table(&dev->res);
+	if (!dev->rule) {
+		dev->rule = pnp_alloc(sizeof(struct pnp_rule_table));
+		if (!dev->rule) {
+			spin_unlock(&pnp_lock);
+			return -ENOMEM;
+		}
+	}
+	while (pnp_next_rule(dev)) {
+		for (i = 0; i < PNP_MAX_PORT; i++) {
+			if (!pnp_next_port(dev,i))
+				continue;
+		}
+		for (i = 0; i < PNP_MAX_MEM; i++) {
+			if (!pnp_next_mem(dev,i))
+				continue;
+		}
+		for (i = 0; i < PNP_MAX_IRQ; i++) {
+			if (!pnp_next_irq(dev,i))
+				continue;
+		}
+		for (i = 0; i < PNP_MAX_DMA; i++) {
+			if (!pnp_next_dma(dev,i))
+				continue;
+		}
+		goto done;
+	}
+	pnp_init_resource_table(&dev->res);
+	dev->rule->depnum = 0;
+	spin_unlock(&pnp_lock);
+	return 0;
+
+done:
+	pnp_resolve_conflicts(dev);	/* this is required or we will break the advanced configs */
+	return 1;
+}
+
 static int pnp_compare_resources(struct pnp_resource_table * resa, struct pnp_resource_table * resb)
 {
 	int idx;
@@ -1136,6 +1200,7 @@
 /**
  * pnp_auto_config_dev - determines the best possible resource configuration based on available information
  * @dev: pointer to the desired device
+ *
  */
 
 int pnp_auto_config_dev(struct pnp_dev *dev)
@@ -1149,7 +1214,7 @@
 	if(dev->active)
 		error = pnp_resolve_conflicts(dev);
 	else
-		error = pnp_generate_config(dev);
+		error = pnp_advanced_config(dev);
 	return error;
 }
 
@@ -1157,6 +1222,8 @@
  * pnp_manual_config_dev - Disables Auto Config and Manually sets the resource table
  * @dev: pointer to the desired device
  * @res: pointer to the new resource config
+ *
+ * This function can be used by drivers that want to manually set thier resources.
  */
 
 int pnp_manual_config_dev(struct pnp_dev *dev, struct pnp_resource_table * res, int mode)
@@ -1190,9 +1257,7 @@
 	}
 	spin_unlock(&pnp_lock);
 
-	if (mode & PNP_CONFIG_RESOLVE)
-		pnp_resolve_conflicts(dev);
-
+	pnp_resolve_conflicts(dev);
 	dev->config_mode = PNP_CONFIG_MANUAL;
 
 fail:
@@ -1216,49 +1281,51 @@
 		pnp_info("res: The PnP device '%s' is already active.", dev->dev.bus_id);
 		return -EBUSY;
 	}
-	/* if the auto config failed, try again now, because this device was requested before others it's best to find a config at all costs */
+	spin_lock(&pnp_lock);	/* we lock just in case the device is being configured during this call */
+	dev->active = 1;
+	spin_unlock(&pnp_lock); /* once the device is claimed active we know it won't be configured so we can unlock */
+
+	/* If this condition is true, advanced configuration failed, we need to get this device up and running
+	 * so we use the simple config engine which ignores cold conflicts, this of course may lead to new failures */
 	if (!pnp_is_active(dev)) {
-		spin_lock(&pnp_lock);
-		if (!pnp_next_config(dev,1,NULL)) {
-			pnp_init_resource_table(&dev->res);
-			dev->rule->depnum = 0;
-			pnp_err("res: Unable to resolve resource conflicts for the device '%s'", dev->dev.bus_id);
-			spin_unlock(&pnp_lock);
-			return -EBUSY;
+		if (!pnp_simple_config(dev)) {
+			pnp_err("res: Unable to resolve resource conflicts for the device '%s'.", dev->dev.bus_id);
+			goto fail;
 		}
-		spin_unlock(&pnp_lock);
 	}
 	if (dev->config_mode & PNP_CONFIG_INVALID) {
-		pnp_info("res: Unable to activate the PnP device '%s' because its resource configuration is invalid", dev->dev.bus_id);
-		return -EINVAL;
+		pnp_info("res: Unable to activate the PnP device '%s' because its resource configuration is invalid.", dev->dev.bus_id);
+		goto fail;
 	}
 	if (dev->status != PNP_READY && dev->status != PNP_ATTACHED){
-		pnp_err("res: Activation failed because the PnP device '%s' is busy", dev->dev.bus_id);
-		return -EBUSY;
+		pnp_err("res: Activation failed because the PnP device '%s' is busy.", dev->dev.bus_id);
+		goto fail;
 	}
 	if (!pnp_can_write(dev)) {
-		pnp_info("res: Unable to activate the PnP device '%s' because this feature is not supported", dev->dev.bus_id);
-		return -EINVAL;
+		pnp_info("res: Unable to activate the PnP device '%s' because this feature is not supported.", dev->dev.bus_id);
+		goto fail;
 	}
 	if (!dev->protocol->set(dev, &dev->res)<0) {
-		pnp_err("res: The protocol '%s' reports that activating the PnP device '%s' has failed", dev->protocol->name, dev->dev.bus_id);
-		return -1;
+		pnp_err("res: The protocol '%s' reports that activating the PnP device '%s' has failed.", dev->protocol->name, dev->dev.bus_id);
+		goto fail;
 	}
 	if (pnp_can_read(dev)) {
 		struct pnp_resource_table res;
 		dev->protocol->get(dev, &res);
-		if (pnp_compare_resources(&dev->res, &res)) {
-			pnp_err("res: The resources requested do not match those actually set for the PnP device '%s'", dev->dev.bus_id);
-			return -1;
-		}
+		if (pnp_compare_resources(&dev->res, &res)) /* if this happens we may be in big trouble but it's best just to continue */
+			pnp_err("res: The resources requested do not match those set for the PnP device '%s', please report.", dev->dev.bus_id);
 	} else
 		dev->active = pnp_is_active(dev);
-	pnp_dbg("res: the device '%s' has been activated", dev->dev.bus_id);
+	pnp_dbg("res: the device '%s' has been activated.", dev->dev.bus_id);
 	if (dev->rule) {
 		kfree(dev->rule);
 		dev->rule = NULL;
 	}
 	return 0;
+
+fail:
+	dev->active = 0; /* fixes incorrect active state */
+	return -EINVAL;
 }
 
 /**
@@ -1277,18 +1344,19 @@
 		return -EINVAL;
 	}
 	if (dev->status != PNP_READY){
-		pnp_info("res: Disable failed becuase the PnP device '%s' is busy", dev->dev.bus_id);
+		pnp_info("res: Disable failed becuase the PnP device '%s' is busy.", dev->dev.bus_id);
 		return -EINVAL;
 	}
-	if (!pnp_can_disable(dev)<0) {
-		pnp_info("res: Unable to disable the PnP device '%s' because this feature is not supported", dev->dev.bus_id);
+	if (!pnp_can_disable(dev)) {
+		pnp_info("res: Unable to disable the PnP device '%s' because this feature is not supported.", dev->dev.bus_id);
 		return -EINVAL;
 	}
-	if (!dev->protocol->disable(dev)) {
-		pnp_err("res: The protocol '%s' reports that disabling the PnP device '%s' has failed", dev->protocol->name, dev->dev.bus_id);
+	if (dev->protocol->disable(dev)<0) {
+		pnp_err("res: The protocol '%s' reports that disabling the PnP device '%s' has failed.", dev->protocol->name, dev->dev.bus_id);
 		return -1;
 	}
-	pnp_dbg("the device '%s' has been disabled", dev->dev.bus_id);
+	dev->active = 0; /* just in case the protocol doesn't do this */
+	pnp_dbg("the device '%s' has been disabled.", dev->dev.bus_id);
 	return 0;
 }
 
diff -urN a/include/linux/pnp.h b/include/linux/pnp.h
--- a/include/linux/pnp.h	Mon Feb  3 19:44:02 2003
+++ b/include/linux/pnp.h	Tue Feb  4 21:41:01 2003
@@ -303,9 +303,8 @@
 /* config modes */
 #define PNP_CONFIG_AUTO		0x0001	/* Use the Resource Configuration Engine to determine resource settings */
 #define PNP_CONFIG_MANUAL	0x0002	/* the config has been manually specified */
-#define PNP_CONFIG_FORCE	0x0003	/* disables validity checking */
-#define PNP_CONFIG_RESOLVE	0x0008	/* moves other configs out of the way, use only when absolutely necessary */
-#define PNP_CONFIG_INVALID	0x0010	/* If this flag is set, the pnp layer will refuse to activate the device */
+#define PNP_CONFIG_FORCE	0x0004	/* disables validity checking */
+#define PNP_CONFIG_INVALID	0x0008	/* If this flag is set, the pnp layer will refuse to activate the device */
 
 /* capabilities */
 #define PNP_READ		0x0001
@@ -381,6 +380,7 @@
 int pnp_add_port_resource(struct pnp_dev *dev, int depnum, struct pnp_port *data);
 int pnp_add_mem_resource(struct pnp_dev *dev, int depnum, struct pnp_mem *data);
 void pnp_init_resource_table(struct pnp_resource_table *table);
+int pnp_generate_rule(struct pnp_dev * dev, int depnum, struct pnp_rule_table * rule);
 int pnp_activate_dev(struct pnp_dev *dev);
 int pnp_disable_dev(struct pnp_dev *dev);
 void pnp_resource_change(struct resource *resource, unsigned long start, unsigned long size);
@@ -421,6 +421,7 @@
 static inline int pnp_add_port_resource(struct pnp_dev *dev, int depnum, struct pnp_irq *data) { return -ENODEV; }
 static inline int pnp_add_mem_resource(struct pnp_dev *dev, int depnum, struct pnp_irq *data) { return -ENODEV; }
 static inline void pnp_init_resource_table(struct pnp_resource_table *table) { ; }
+static inline int pnp_generate_rule(struct pnp_dev * dev, int depnum, struct pnp_rule_table * rule) { return -ENODEV; }
 static inline int pnp_activate_dev(struct pnp_dev *dev) { return -ENODEV; }
 static inline int pnp_disable_dev(struct pnp_dev *dev) { return -ENODEV; }
 static inline void pnp_resource_change(struct resource *resource, unsigned long start, unsigned long size) { ; }

Re: PnP model

[Adam Belay]

On Tue, Feb 04, 2003 at 11:53:40AM -0800, Grover, Andrew wrote:
> > From: John Bradford [mailto:john@grabjohn.com] 
> > > I think the people who want to manually configure their device's
> > > resources need to step up and justify why this is really necessary.
> > 
> > Prototyping an embedded system, maybe, where you have devices in the
> > test box that won't be in the production machine.  You would want them
> > to use resources other than those that you want the hardware which
> > will be present to use.
>
> Ok fair enough. But I think the drivers should always think things are
> handled in a PnP manner, even if they really aren't. ;-) For example,
> between the stages where PnP enumerates the devices and the stage where
> drivers get device_add notifications as a result of that, we will be
> assigning the system resources to each device, but we could also
> implement a way at this stage for people to manually alter things. I
> think this is the right place to do this, as opposed to having all the
> drivers implement code to probe for themselves.
> 
> Thoughts?

I agree.  Actually the isapnp specifications (see Figure 2. Plug and Play ISA 
Configuration Flow for Plug and Play BIOS located in Plug and Play ISA
Specification Version 1.0a) recommend that the operating system configures
and activates all devices before drivers are loaded.  For the most part
linux plug and play follows this standard with the exception of manual
configuration support which is included in my latest patches.

Regards,
Adam

Re: PnP model

[Jaroslav Kysela]

On Wed, 5 Feb 2003, Adam Belay wrote:

> On Tue, Feb 04, 2003 at 10:49:37AM +0000, Russell King wrote:
> > On Tue, Feb 04, 2003 at 11:18:36AM +0100, Jaroslav Kysela wrote:
> > > I think that legacy devices must be probed after PnP ones, otherwise
> > > you'll get these duplications.
> > 
> > Unfortunately, this isn't easy to do, while keeping stuff like serial
> > consoles working from early at bootup.  Alan Cox definitely does not
> > want to see the serial console initialised any later than it is today,
> > and he's not the only one.
> > 
> > In addition, the "legacy" devices are part of the 8250.c module - they
> > have to be for serial console.  The PNP devices are probed as part of
> > the 8250_pnp.c module, and since 8250_pnp.c depends on 8250.c, the
> > serial PNP ports will _always_ be initialised after the ISA probes.
> >
> 
> I think I have an alternative solution.  If we add support for current
> resource configs and don't reset the cards, we can determine what the
> active configuration was when the serial driver detects the isapnp card.
> Because the device will remain active the resources will not be changed.
> Below is a patch against my previous 5 patches.  This patch also
> contains a few fixes and cleanups.  Could you please test this and let
> me know if it solves the problem.
> 
> Also I noticed that the serial driver legacy probe, independent of my
> changes, sometimes detects the incorrect irq, is proper irq detection
> needed by the serial driver or will it still work properly even if
> that information is detected incorrectly?

> -int isapnp_reset = 1;			/* reset all PnP cards (deactivate) */
> +int isapnp_reset = 0;			/* reset all PnP cards (deactivate) */

Please, don't do this. The default value is quite ok. I think that it's
more PnP BIOS problem than ISA PnP one (I don't know about any motherboard
where the standard serial ports are ISA PnP). But the parsing of active
configuration when isapnp_reset == 0 is ok...

> +	for (tmp = 0; tmp < PNP_MAX_PORT && res->port_resource[tmp].flags & IORESOURCE_IO; tmp++)

Again, pnp_valid_* macros are your fried. You deleted them from your 
tree during merge.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs