[KJ] [PATCH]

[KJ] [PATCH]

[Patrik Kullman]

This is my first try to submit a patch here, so go gentle on me.. ;)
At least patch-tester@coderock.org 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.

Signed-off-by: Patrik Kullman <patrik@yes.nu>
--- linux-2.6.17/drivers/char/watchdog/alim1535_wdt.c	2006-06-18 03:49:35.000000000 +0200
+++ linux/drivers/char/watchdog/alim1535_wdt.c	2006-07-28 07:15:00.000000000 +0200
@@ -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);
--- linux-2.6.17/drivers/char/watchdog/alim7101_wdt.c	2006-06-18 03:49:35.000000000 +0200
+++ linux/drivers/char/watchdog/alim7101_wdt.c	2006-07-28 18:19:52.000000000 +0200
@@ -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 <steve@navaho.co.uk>.\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
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Domen Puncer]

On 28/07/06 18:31 +0200, Patrik Kullman wrote:
> This is my first try to submit a patch here, so go gentle on me.. ;)
> At least patch-tester@coderock.org 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.
> 
> Signed-off-by: Patrik Kullman <patrik@yes.nu>
> --- linux-2.6.17/drivers/char/watchdog/alim1535_wdt.c	2006-06-18 03:49:35.000000000 +0200
> +++ linux/drivers/char/watchdog/alim1535_wdt.c	2006-07-28 07:15:00.000000000 +0200
> @@ -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);

pci_dev_put(NULL)?

>  		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?

> +	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);
> --- linux-2.6.17/drivers/char/watchdog/alim7101_wdt.c	2006-06-18 03:49:35.000000000 +0200
> +++ linux/drivers/char/watchdog/alim7101_wdt.c	2006-07-28 18:19:52.000000000 +0200
> @@ -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 <steve@navaho.co.uk>.\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(NULL)

> +		pci_dev_put(alim7101_pmu);

Goto err_out (or something appropriate).

>  		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

>  			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

>  		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
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Domen Puncer]

On 31/07/06 21:51 +0200, Patrik Kullman wrote:
> > >  		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?
> 
> 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
> 
> I guess I shouldn't have.
> 
> Will fix.
> 
> 
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Alan Cox]

Ar Sad, 2006-10-14 am 11:49 -0600, ysgrifennodd Matthew Wilcox:
> Only broken on SMP ...
> 
> I wouldn't mind writing a new driver (using the serial core) if someone
> wants to send me one.  I need a multiport serial card anyway ...

You still have ISA bus ?

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH]

[Amol Lad]

On Mon, 2006-10-16 at 07:23 -0600, Matthew Wilcox wrote:
> On Mon, Oct 16, 2006 at 12:58:52PM +0530, Amol Lad wrote:
> > Replaced save_flags()/cli()/restore_flags() pair with spin_lock
> > alternatives.
> > 
> > For this case, I believe spin lock plays no role but I also do not have
> > a better way.
> 
> I think there's a better way.  Here's the full stretch covered by that:
> 
>         save_flags(flags);
>         cli();
> 
> #ifndef CONFIG_SERIAL_DZ_CONSOLE
>         /* reset the chip */
>         dz_reset(&dz_ports[0]);
> #endif
> 
>         /* order matters here... the trick is that flags
>            is updated... in request_irq - to immediatedly obliterate
>            it is unwise. */
>         restore_flags(flags);
> 
> Now, we can obviously move the junk inside the ifdef:
> 
> #ifndef CONFIG_SERIAL_DZ_CONSOLE
>         save_flags(flags);
>         cli();
> 
>         /* reset the chip */
>         dz_reset(&dz_ports[0]);
> 
>         restore_flags(flags);
> #endif
> 
> Now, there's only one other place that dz_reset is called from, and to
> be honest, it looks like it's missing some locking too.  Looking at the
> other uses of spin_lock within this file, we can see that it's used to
> protect the DZ_ ports.
> 
> So I think a better patch would look like this:
> 
> diff --git a/drivers/serial/dz.c b/drivers/serial/dz.c
> index 8a98aae..de7a0b1 100644
> --- a/drivers/serial/dz.c
> +++ b/drivers/serial/dz.c
> @@ -661,6 +661,8 @@ static void __init dz_init_ports(void)
>  
>  static void dz_reset(struct dz_port *dport)
>  {
> +       unsigned long flags;
> +       spin_lock_irqsave((&dport->port.lock, flags);
>         dz_out(dport, DZ_CSR, DZ_CLR);
>  
>         while (dz_in(dport, DZ_CSR) & DZ_CLR);
> @@ -670,6 +672,7 @@ static void dz_reset(struct dz_port *dpo
>  
>         /* enable scanning */
>         dz_out(dport, DZ_CSR, DZ_MSE);
> +       spin_unlock_irqrestore((&dport->port.lock, flags);
>  }
>  
>  #ifdef CONFIG_SERIAL_DZ_CONSOLE
> @@ -783,19 +786,11 @@ int __init dz_init(void)
>  
>         dz_init_ports();
>  
> -       save_flags(flags);
> -       cli();
> -
>  #ifndef CONFIG_SERIAL_DZ_CONSOLE
>         /* reset the chip */
>         dz_reset(&dz_ports[0]);
>  #endif
>  
> -       /* order matters here... the trick is that flags
> -          is updated... in request_irq - to immediatedly obliterate
> -          it is unwise. */
> -       restore_flags(flags);
> -
>         if (request_irq(dz_ports[0].port.irq, dz_interrupt,
>                         IRQF_DISABLED, "DZ", &dz_ports[0]))
>                 panic("Unable to register DZ interrupt");
> 
> But looking at the driver, there's some places we're missing locking.
> 
> In dz_set_mctrl(), we read-modify-write DZ_TCR without holding a lock.
> We also do that in dz_console_setup(), but I suspect we're guaranteed
> by higher levels not to race with anything.
> 
> I suspect it can't hit us in practice (due to the dz driver being for
> hardware that's UP only, but maybe with preemption, it could bite
> us ...), but there's no locking in the interrupt handler.  I think
> dz_transmit_chars() needs locking against dz_console_putchar(), for
> example.
> 
> Anyway, that's enough to be going on with.

Thanks for that insight. Why not just include your this patch initally
and we add more locking later (as it will surely need run time testing)

If you think it's fine then please sign-off the patch.

Thanks again !

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors