Re: [PATCH] Watchdog: i8xx_tco remove pci_find_device

Re: [PATCH] Watchdog: i8xx_tco remove pci_find_device

[Wim Van Sebroeck]

Hi Jiri,

Just back from a small holiday, but:

> Watchdog: i8xx_tco remove pci_find_device.
> 
> Use refcounting for pci device obtaining. Use PCI_DEVICE macro.
> 
> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

Why the pci_dev_put's? We aren't registering the PCI devices. See
the comment above the MODULE_DEVICE_TABLE:
/*
 * Data for PCI driver interface
 *
 * This data only exists for exporting the supported
 * PCI ids via MODULE_DEVICE_TABLE.  We do not actually
 * register a pci_driver, because someone else might one day
 * want to register another driver on the same PCI id.
 */

Since the I/O controller Hub has several functions we explicitely
do not register the PCI device...

PS: In the -mm tree there is allready a replacement for this driver...
Plan is to get this one into linus tree soon.

Greetings,
Wim.

Re: [PATCH] Watchdog: i8xx_tco remove pci_find_device

[Jiri Slaby]

Wim Van Sebroeck napsal(a):
> Hi Jiri,

Hi Wim.

> 
> Just back from a small holiday, but:
> 
>> Watchdog: i8xx_tco remove pci_find_device.
>>
>> Use refcounting for pci device obtaining. Use PCI_DEVICE macro.
>>
>> Signed-off-by: Jiri Slaby <jirislaby@gmail.com>
> 
> Why the pci_dev_put's? We aren't registering the PCI devices. See
> the comment above the MODULE_DEVICE_TABLE:
> /*
>  * Data for PCI driver interface
>  *
>  * This data only exists for exporting the supported
>  * PCI ids via MODULE_DEVICE_TABLE.  We do not actually
>  * register a pci_driver, because someone else might one day
>  * want to register another driver on the same PCI id.
>  */

Sure, but it's not registering, but telling the subsystem, we use the device, so
that user can't hotunplug it since some driver uses it and reads and writes its
registers. It's purpose of refcounting in pci_dev_{put,get}() (pci_dev_get is
called in pci_get_device()).

> Since the I/O controller Hub has several functions we explicitely
> do not register the PCI device...
> 
> PS: In the -mm tree there is allready a replacement for this driver...
> Plan is to get this one into linus tree soon.

This patch is against 2.6.18-rc1-mm2. (Maybe you mean there are some patches
coming to -rc2-mm1?)

thanks,
-- 
<a href="http://www.fi.muni.cz/~xslaby/">Jiri Slaby</a>
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: [PATCH] Watchdog: i8xx_tco remove pci_find_device

[Wim Van Sebroeck]

Hi Jiri,

> > Why the pci_dev_put's? We aren't registering the PCI devices. See
> > the comment above the MODULE_DEVICE_TABLE:
> > /*
> >  * Data for PCI driver interface
> >  *
> >  * This data only exists for exporting the supported
> >  * PCI ids via MODULE_DEVICE_TABLE.  We do not actually
> >  * register a pci_driver, because someone else might one day
> >  * want to register another driver on the same PCI id.
> >  */
> 
> Sure, but it's not registering, but telling the subsystem, we use the device, so
> that user can't hotunplug it since some driver uses it and reads and writes its
> registers. It's purpose of refcounting in pci_dev_{put,get}() (pci_dev_get is
> called in pci_get_device()).

I see now; it's the: 
#define for_each_pci_dev(d) while ((d = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, d)) != NULL)
that does this. Need to fix that in iTCO_wdt...

> > Since the I/O controller Hub has several functions we explicitely
> > do not register the PCI device...
> > 
> > PS: In the -mm tree there is allready a replacement for this driver...
> > Plan is to get this one into linus tree soon.
> 
> This patch is against 2.6.18-rc1-mm2. (Maybe you mean there are some patches
> coming to -rc2-mm1?)

Nope. iTCO_wdt is going to replace i8xx_tco after testing.

Greetings,
Wim.