[KJ] changed from pci_find_device to pci_get_device

[KJ] changed from pci_find_device to pci_get_device

[Daniel Marjamäki]

Hello friends!

I've made some changes and are interested in feedback (drivers/ide/pci/via82cxxx.c).

Some notes:
* The main change was to switch from pci_find_device to pci_get_device.
* I unlock "isa" when it won't be used anymore
* Changed a return value because init_chipset_via82cxxx returns an unsigned value.

This is the patch in a readable format:

static unsigned int __devinit init_chipset_via82cxxx(struct pci_dev *dev, const char *name)
{
	struct pci_dev *isa = NULL;

	for (via_config = via_isa_bridges; via_config->id; via_config++)
-		if ((isa = pci_find_device(PCI_VENDOR_ID_VIA +
+		if ((isa = pci_get_device(PCI_VENDOR_ID_VIA +
			!!(via_config->flags & VIA_BAD_ID),
			via_config->id, NULL))) {

			pci_read_config_byte(isa, PCI_REVISION_ID, &t);
			if (t >= via_config->rev_min &&
			    t <= via_config->rev_max)
				break;
+
+			pci_dev_put(isa);
		}

	if (!via_config->id) {
		printk(KERN_WARNING "VP_IDE: Unknown VIA SouthBridge, disabling DMA.\n");
-		return -ENODEV;
+		return ENODEV;
	}

	/* DO STUFF (I don't include it here for your convenience) */

#if defined(DISPLAY_VIA_TIMINGS) && defined(CONFIG_PROC_FS)
-	if (!via_proc) {
+	if (via_proc) {
+		isa_dev = NULL;
+		pci_dev_put(isa);
+	} else {
		via_base = pci_resource_start(dev, 4);
		bmide_dev = dev;
		isa_dev = isa;
		ide_pci_create_host_proc("via", via_get_info);
		via_proc = 1;
	}
+#else
+	pci_dev_put(isa);	
#endif /* DISPLAY_VIA_TIMINGS && CONFIG_PROC_FS */

	return 0;
}


+#if defined(DISPLAY_VIA_TIMINGS) && defined(CONFIG_PROC_FS)
+static void via_ide_exit(void)
+{
+	if (isa_dev)
+		pci_dev_put(isa_dev);
+}
+module_exit(via_ide_exit);
+#endif /* DISPLAY_VIA_TIMINGS && CONFIG_PROC_FS */


Best regards,
Daniel Marjamäki




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

Re: [KJ] changed from pci_find_device to pci_get_device

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

On Sun, Nov 27, 2005 at 08:41:20PM +0100, Daniel Marjam?ki wrote:
> 
> 
> Hello friends!
> 
> I've made some changes and are interested in feedback 
> (drivers/ide/pci/via82cxxx.c).
> 
> Some notes:
> * The main change was to switch from pci_find_device to pci_get_device.
> * I unlock "isa" when it won't be used anymore
> * Changed a return value because init_chipset_via82cxxx returns an unsigned 
> value.
> 
> This is the patch in a readable format:

But it's not in a format that could be applied :(

Also, try splitting this up into 3 different patches, as you are doing
three different things.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Daniel Marjamäki]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]


The patch I posted was made against the wrong kernel version. Sorry.

The patch below is made against linux-2.6.15-rc2..



--- a/drivers/ide/pci/via82cxxx.c	2005-11-28 18:14:54.000000000 +0100
+++ b/drivers/ide/pci/via82cxxx.c	2005-11-28 18:20:51.000000000 +0100
@@ -247,7 +247,7 @@ static struct via_isa_bridge *via_config
  	u8 t;

  	for (via_config = via_isa_bridges; via_config->id; via_config++)
-		if ((*isa = pci_find_device(PCI_VENDOR_ID_VIA +
+		if ((*isa = pci_get_device(PCI_VENDOR_ID_VIA +
  			!!(via_config->flags & VIA_BAD_ID),
  			via_config->id, NULL))) {

@@ -255,6 +255,9 @@ static struct via_isa_bridge *via_config
  			if (t >= via_config->rev_min &&
  			    t <= via_config->rev_max)
  				break;
+
+			pci_dev_put(*isa);
+			*isa = NULL;
  		}

  	return via_config;
@@ -282,7 +285,7 @@ static unsigned int __devinit init_chips
  	via_config = via_config_find(&isa);
  	if (!via_config->id) {
  		printk(KERN_WARNING "VP_IDE: Unknown VIA SouthBridge, disabling DMA.\n");
-		return -ENODEV;
+		return ENODEV;
  	}

  	/*
@@ -360,6 +363,8 @@ static unsigned int __devinit init_chips
  		via_dma[via_config->flags & VIA_UDMA],
  		pci_name(dev));

+	pci_dev_put(isa);
+
  	return 0;
  }

@@ -430,6 +435,7 @@ static void __devinit init_hwif_via82cxx
  	ide_set_hwifdata(hwif, vdev);

  	vdev->via_config = via_config_find(&isa);
+	if (isa) pci_dev_put(isa);
  	via_cable_detect(hwif->pci_dev, vdev);

  	hwif->autodma = 0;





[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Alexey Dobriyan]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On Mon, Nov 28, 2005 at 10:43:58PM +0100, Daniel Marjam?ki wrote:
> --- a/drivers/ide/pci/via82cxxx.c
> +++ b/drivers/ide/pci/via82cxxx.c
> @@ -282,7 +285,7 @@ static unsigned int __devinit init_chips
>  	via_config = via_config_find(&isa);
>  	if (!via_config->id) {
>  		printk(KERN_WARNING "VP_IDE: Unknown VIA SouthBridge,
>  		disabling DMA.\n");
> -		return -ENODEV;
> +		return ENODEV;
>  	}

I do not understand this chunk.

> @@ -430,6 +435,7 @@ static void __devinit init_hwif_via82cxx
>  	ide_set_hwifdata(hwif, vdev);
>
>  	vdev->via_config = via_config_find(&isa);
> +	if (isa) pci_dev_put(isa);
>  	via_cable_detect(hwif->pci_dev, vdev);

	if (isa)
		pci_...


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Daniel Marjamäki]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]


Thank you Alexey for reviewing my patch.

About the chunk you didn't understand.
The function must return an unsigned int, therefore "return -ENODEV;" is wrong.



Alexey Dobriyan wrote:
> On Mon, Nov 28, 2005 at 10:43:58PM +0100, Daniel Marjam?ki wrote:
> 
>>--- a/drivers/ide/pci/via82cxxx.c
>>+++ b/drivers/ide/pci/via82cxxx.c
>>@@ -282,7 +285,7 @@ static unsigned int __devinit init_chips
>> 	via_config = via_config_find(&isa);
>> 	if (!via_config->id) {
>> 		printk(KERN_WARNING "VP_IDE: Unknown VIA SouthBridge,
>> 		disabling DMA.\n");
>>-		return -ENODEV;
>>+		return ENODEV;
>> 	}
> 
> 
> I do not understand this chunk.
> 
> 
>>@@ -430,6 +435,7 @@ static void __devinit init_hwif_via82cxx
>> 	ide_set_hwifdata(hwif, vdev);
>>
>> 	vdev->via_config = via_config_find(&isa);
>>+	if (isa) pci_dev_put(isa);
>> 	via_cable_detect(hwif->pci_dev, vdev);
> 
> 
> 	if (isa)
> 		pci_...
> 
> 


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Alexey Dobriyan]

[-- Attachment #1: Type: text/plain, Size: 822 bytes --]

On Tue, Nov 29, 2005 at 12:51:27PM +0100, Daniel Marjam?ki wrote:
> About the chunk you didn't understand.
> The function must return an unsigned int, therefore "return -ENODEV;" is
> wrong.

->init_chipset method returns either small positive number (irq) or BIG
positive number in case of error ((unsigned int) -ENODEV). In
do_ide_setup_pci_device() function ->init_chipset return value is casted to int
and checked for "< 0".

> >>--- a/drivers/ide/pci/via82cxxx.c
> >>+++ b/drivers/ide/pci/via82cxxx.c
> >>@@ -282,7 +285,7 @@ static unsigned int __devinit init_chips
> >>	via_config = via_config_find(&isa);
> >>	if (!via_config->id) {
> >>		printk(KERN_WARNING "VP_IDE: Unknown VIA SouthBridge,
> >>		disabling DMA.\n");
> >>-		return -ENODEV;
> >>+		return ENODEV;
> >>	}
> >
> >
> >I do not understand this chunk.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Daniel Marjamäki]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]


>>About the chunk you didn't understand.
>>The function must return an unsigned int, therefore "return -ENODEV;" is
>>wrong.
> 
> 
> ->init_chipset method returns either small positive number (irq) or BIG
> positive number in case of error ((unsigned int) -ENODEV). In
> do_ide_setup_pci_device() function ->init_chipset return value is casted to int
> and checked for "< 0".
> 

I made a HUGE mistake then. Sorry.
Thank you Alexey!!


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Daniel Marjamäki]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]


Hello friends!

I have reworked my patch that I sent a few days ago.
I made huge mistakes in the previous patch but I hope this patch is perfect.


--- a/drivers/ide/pci/via82cxxx.c	2005-11-29 14:45:00.000000000 +0100
+++ b/drivers/ide/pci/via82cxxx.c	2005-11-29 14:49:32.000000000 +0100
@@ -247,7 +247,7 @@ static struct via_isa_bridge *via_config
  	u8 t;

  	for (via_config = via_isa_bridges; via_config->id; via_config++)
-		if ((*isa = pci_find_device(PCI_VENDOR_ID_VIA +
+		if ((*isa = pci_get_device(PCI_VENDOR_ID_VIA +
  			!!(via_config->flags & VIA_BAD_ID),
  			via_config->id, NULL))) {

@@ -255,6 +255,9 @@ static struct via_isa_bridge *via_config
  			if (t >= via_config->rev_min &&
  			    t <= via_config->rev_max)
  				break;
+
+			pci_dev_put(*isa);
+			*isa = NULL;
  		}

  	return via_config;
@@ -360,6 +363,8 @@ static unsigned int __devinit init_chips
  		via_dma[via_config->flags & VIA_UDMA],
  		pci_name(dev));

+	pci_dev_put(isa);
+
  	return 0;
  }

@@ -430,6 +435,8 @@ static void __devinit init_hwif_via82cxx
  	ide_set_hwifdata(hwif, vdev);

  	vdev->via_config = via_config_find(&isa);
+	if (isa)
+		pci_dev_put(isa);
  	via_cable_detect(hwif->pci_dev, vdev);

  	hwif->autodma = 0;



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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

Re: [KJ] changed from pci_find_device to pci_get_device

[Greg KH]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Tue, Nov 29, 2005 at 02:56:09PM +0100, Daniel Marjam?ki wrote:
> 
> Hello friends!
> 
> I have reworked my patch that I sent a few days ago.
> I made huge mistakes in the previous patch but I hope this patch is perfect.

Please include the whole list of what you are changing in the patch, as
sometimes we don't remember what is supposed to be happening here :)

Also, you forgot the "Signed-off-by:" line...

> @@ -430,6 +435,8 @@ static void __devinit init_hwif_via82cxx
>  	ide_set_hwifdata(hwif, vdev);
> 
>  	vdev->via_config = via_config_find(&isa);
> +	if (isa)
> +		pci_dev_put(isa);
>  	via_cable_detect(hwif->pci_dev, vdev);

You don't have to check for NULL before calling pci_dev_put(), so you
can drop the if() check.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

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