Skip to contentSkip to conversation

[KJ] [PATCH]

PATCH In review Patrik Kullman 2 participants 3 messages Jul 28, 2006 – Jul 31, 2006

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 [KJ] [PATCH]

This is my first try to submit a patch here, so go gentle on me.. ;) At least [email protected] thought it seemed ok..

This little patch moves from pci_find_device() in alim1535_wdt.c and alim7101_wdt.c to pci_get_device()/pci_dev_put() I also found that alim1535.c uses 0x1535/0x7101 instead of the PCI_DEVICE_ID_AL_* defines.

I compared my usage of pci_dev_put() with other drivers and I hope that I've understood it right.

drivers/char/watchdog/alim1535_wdt.c +15 -6
@@ -312,7 +312,7 @@
  */
 
 static struct pci_device_id ali_pci_tbl[] = {
-	{ PCI_VENDOR_ID_AL, 0x1535, PCI_ANY_ID, PCI_ANY_ID,},
+	{ PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, PCI_ANY_ID, PCI_ANY_ID,},
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, ali_pci_tbl);
@@ -330,17 +330,25 @@
 	u32 wdog;
 
 	/* Check for a 1535 series bridge */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x1535, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, NULL);
+	if(!pdev) {
+		pci_dev_put(pdev);
 		return -ENODEV;
+	} else {
+		pci_dev_put(pdev);
+	}
 
 	/* Check for the a 7101 PMU */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x7101, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, NULL);
+	if(!pdev) {
+		pci_dev_put(pdev);
 		return -ENODEV;
+	}
 
-	if(pci_enable_device(pdev))
+	if(pci_enable_device(pdev)) {
+		pci_dev_put(pdev);
 		return -EIO;
+	}
 
 	ali_pci = pdev;
 
@@ -447,6 +455,7 @@
 	/* Deregister */
 	unregister_reboot_notifier(&ali_notifier);
 	misc_deregister(&ali_miscdev);
+	pci_dev_put(ali_pci);
 }
 
 module_init(watchdog_init);
drivers/char/watchdog/alim7101_wdt.c +13 -2
@@ -332,6 +332,7 @@
 	wdt_turnoff();
 	/* Deregister */
 	misc_deregister(&wdt_miscdev);
+	pci_dev_put(alim7101_pmu);
 	unregister_reboot_notifier(&wdt_notifier);
 }
 
@@ -342,17 +343,20 @@
 	char tmp;
 
 	printk(KERN_INFO PFX "Steve Hill <[email protected]>.\n");
-	alim7101_pmu = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
+	alim7101_pmu = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
 	if (!alim7101_pmu) {
 		printk(KERN_INFO PFX "ALi M7101 PMU not present - WDT not set\n");
+		pci_dev_put(alim7101_pmu);
 		return -EBUSY;
 	}
 
 	/* Set the WDT in the PMU to 1 second */
 	pci_write_config_byte(alim7101_pmu, ALI_7101_WDT, 0x02);
 
-	ali1543_south = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
+	ali1543_south = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
 	if (!ali1543_south) {
+		pci_dev_put(ali1543_south);
+		pci_dev_put(alim7101_pmu);
 		printk(KERN_INFO PFX "ALi 1543 South-Bridge not present - WDT not set\n");
 		return -EBUSY;
 	}
@@ -360,11 +364,15 @@
 	if ((tmp & 0x1e) = 0x00) {
 		if (!use_gpio) {
 			printk(KERN_INFO PFX "Detected old alim7101 revision 'a1d'.  If this is a cobalt board, set the 'use_gpio' module parameter.\n");
+			pci_dev_put(ali1543_south);
+			pci_dev_put(alim7101_pmu);
 			return -EBUSY;
 		} 
 		nowayout = 1;
 	} else if ((tmp & 0x1e) != 0x12 && (tmp & 0x1e) != 0x00) {
 		printk(KERN_INFO PFX "ALi 1543 South-Bridge does not have the correct revision number (???1001?) - WDT not set\n");
+		pci_dev_put(ali1543_south);
+		pci_dev_put(alim7101_pmu);
 		return -EBUSY;
 	}
 
@@ -397,6 +405,7 @@
 		__module_get(THIS_MODULE);
 	}
 
+	pci_dev_put(ali1543_south);
 	printk(KERN_INFO PFX "WDT driver for ALi M7101 initialised. timeout=%d sec (nowayout=%d)\n",
 		timeout, nowayout);
 	return 0;
@@ -404,6 +413,8 @@
 err_out_miscdev:
 	misc_deregister(&wdt_miscdev);
 err_out:
+	pci_dev_put(alim7101_pmu);
+	pci_dev_put(ali1543_south);
 	return rc;
 }
 

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#1 lore raw
Quoted 21 lines · Patrik Kullman

This is my first try to submit a patch here, so go gentle on me.. ;) At least [email protected] thought it seemed ok..

This little patch moves from pci_find_device() in alim1535_wdt.c and alim7101_wdt.c to pci_get_device()/pci_dev_put() I also found that alim1535.c uses 0x1535/0x7101 instead of the PCI_DEVICE_ID_AL_* defines.

I compared my usage of pci_dev_put() with other drivers and I hope that I've understood it right.

drivers/char/watchdog/alim1535_wdt.c +1 -1
@@ -312,7 +312,7 @@
  */
 
 static struct pci_device_id ali_pci_tbl[] = {
-	{ PCI_VENDOR_ID_AL, 0x1535, PCI_ANY_ID, PCI_ANY_ID,},
+	{ PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, PCI_ANY_ID, PCI_ANY_ID,},
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, ali_pci_tbl);

drivers/char/watchdog/alim1535_wdt.c line 335

Quoted 8 lines
drivers/char/watchdog/alim1535_wdt.c +3 -2
@@ -330,17 +330,25 @@
 	u32 wdog;
 
 	/* Check for a 1535 series bridge */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x1535, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1535, NULL);
+	if(!pdev) {
+		pci_dev_put(pdev);

pci_dev_put(NULL)?

drivers/char/watchdog/alim1535_wdt.c line 342

Quoted 9 lines
drivers/char/watchdog/alim1535_wdt.c +4 -2
 		return -ENODEV;
+	} else {
+		pci_dev_put(pdev);
+	}
 
 	/* Check for the a 7101 PMU */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x7101, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, NULL);

What if the previous _and_ this one succeed?

Quoted 40 lines
drivers/char/watchdog/alim1535_wdt.c +7 -1
+	if(!pdev) {
+		pci_dev_put(pdev);
 		return -ENODEV;
+	}
 
-	if(pci_enable_device(pdev))
+	if(pci_enable_device(pdev)) {
+		pci_dev_put(pdev);
 		return -EIO;
+	}
 
 	ali_pci = pdev;
 
@@ -447,6 +455,7 @@
 	/* Deregister */
 	unregister_reboot_notifier(&ali_notifier);
 	misc_deregister(&ali_miscdev);
+	pci_dev_put(ali_pci);
 }
 
 module_init(watchdog_init);
drivers/char/watchdog/alim7101_wdt.c +3 -1
@@ -332,6 +332,7 @@
 	wdt_turnoff();
 	/* Deregister */
 	misc_deregister(&wdt_miscdev);
+	pci_dev_put(alim7101_pmu);
 	unregister_reboot_notifier(&wdt_notifier);
 }
 
@@ -342,17 +343,20 @@
 	char tmp;
 
 	printk(KERN_INFO PFX "Steve Hill <[email protected]>.\n");
-	alim7101_pmu = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
+	alim7101_pmu = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101,NULL);
 	if (!alim7101_pmu) {
 		printk(KERN_INFO PFX "ALi M7101 PMU not present - WDT not set\n");
+		pci_dev_put(alim7101_pmu);

drivers/char/watchdog/alim7101_wdt.c line 358

Quoted 10 lines
drivers/char/watchdog/alim7101_wdt.c +2 -1
 		return -EBUSY;
 	}
 
 	/* Set the WDT in the PMU to 1 second */
 	pci_write_config_byte(alim7101_pmu, ALI_7101_WDT, 0x02);
 
-	ali1543_south = pci_find_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
+	ali1543_south = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M1533, NULL);
 	if (!ali1543_south) {
+		pci_dev_put(ali1543_south);

pci_dev_put(NULL)

drivers/char/watchdog/alim7101_wdt.c line 359

Quoted 1 line
drivers/char/watchdog/alim7101_wdt.c +1 -0
+		pci_dev_put(alim7101_pmu);

Goto err_out (or something appropriate).

drivers/char/watchdog/alim7101_wdt.c line 368

Quoted 9 lines
drivers/char/watchdog/alim7101_wdt.c +2 -0
 		printk(KERN_INFO PFX "ALi 1543 South-Bridge not present - WDT not set\n");
 		return -EBUSY;
 	}
@@ -360,11 +364,15 @@
 	if ((tmp & 0x1e) = 0x00) {
 		if (!use_gpio) {
 			printk(KERN_INFO PFX "Detected old alim7101 revision 'a1d'.  If this is a cobalt board, set the 'use_gpio' module parameter.\n");
+			pci_dev_put(ali1543_south);
+			pci_dev_put(alim7101_pmu);

ditto

drivers/char/watchdog/alim7101_wdt.c line 375

Quoted 7 lines
drivers/char/watchdog/alim7101_wdt.c +2 -0
 			return -EBUSY;
 		} 
 		nowayout = 1;
 	} else if ((tmp & 0x1e) != 0x12 && (tmp & 0x1e) != 0x00) {
 		printk(KERN_INFO PFX "ALi 1543 South-Bridge does not have the correct revision number (???1001?) - WDT not set\n");
+		pci_dev_put(ali1543_south);
+		pci_dev_put(alim7101_pmu);

ditto

Quoted 22 lines
drivers/char/watchdog/alim7101_wdt.c +3 -0
 		return -EBUSY;
 	}
 
@@ -397,6 +405,7 @@
 		__module_get(THIS_MODULE);
 	}
 
+	pci_dev_put(ali1543_south);
 	printk(KERN_INFO PFX "WDT driver for ALi M7101 initialised. timeout=%d sec (nowayout=%d)\n",
 		timeout, nowayout);
 	return 0;
@@ -404,6 +413,8 @@
 err_out_miscdev:
 	misc_deregister(&wdt_miscdev);
 err_out:
+	pci_dev_put(alim7101_pmu);
+	pci_dev_put(ali1543_south);
 	return rc;
 }
 
Quoted 4 lines

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

#2 lore raw
In reply to Patrik Kullman
Quoted 12 lines · Patrik Kullman
Quoted 11 lines

drivers/char/watchdog/alim1535_wdt.c line 342

Quoted 9 lines
drivers/char/watchdog/alim1535_wdt.c +4 -2
 		return -ENODEV;
+	} else {
+		pci_dev_put(pdev);
+	}
 
 	/* Check for the a 7101 PMU */
-	pdev = pci_find_device(PCI_VENDOR_ID_AL, 0x7101, NULL);
-	if(pdev = NULL)
+	pdev = pci_get_device(PCI_VENDOR_ID_AL, PCI_DEVICE_ID_AL_M7101, NULL);

What if the previous _and_ this one succeed?

Quoted 4 lines

Since the original developer chose to just search for the 1535 and then overwrite the variable with the 7101 (that will be used), I thought I'd clear the memory of the first device before assigning the new device to it.

Guess I misunderstood the meaning.
Still... you only pci_dev_put one device on exit.

btw. nice to know patch-tester still works :-)

Domen

Quoted 6 lines

I guess I shouldn't have.

Will fix.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors