[PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

HPT374 BIOS seems to only save f_CNT register value for the function #0 before
re-tuning DPLL causing the driver to report obviously distorted f_CNT for the
function #1 -- fix this by always reading the saved f_CNT register value from
in the init_chipset() method the function #0 of HPT374 chip.
While at it, introduce 'chip_type' for the copy of the 'struct hpt_info' member
and replace the structure assignment by memcpy()...

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
This is against the current Linus tree and unfortunately I was able to only
compile test it since that tree gives MODPOST warning and dies early.
Bob, please test it if/when you'll be able to and report the results...

 drivers/ide/pci/hpt366.c |   27 +++++++++++++++++++--------
 1 files changed, 19 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/ide/pci/hpt366.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/hpt366.c
+++ linux-2.6/drivers/ide/pci/hpt366.c
@@ -1,5 +1,5 @@
 /*
- * linux/drivers/ide/pci/hpt366.c		Version 1.10	Jun 29, 2007
+ * linux/drivers/ide/pci/hpt366.c		Version 1.11	Aug 4, 2007
  *
  * Copyright (C) 1999-2003		Andre Hedrick <andre@linux-ide.org>
  * Portions Copyright (C) 2001	        Sun Microsystems, Inc.
@@ -981,6 +981,7 @@ static unsigned int __devinit init_chips
 	struct hpt_info *info	= kmalloc(sizeof(struct hpt_info), GFP_KERNEL);
 	unsigned long io_base	= pci_resource_start(dev, 4);
 	u8 pci_clk,  dpll_clk	= 0;	/* PCI and DPLL clock in MHz */
+	u8 chip_type;
 	enum ata_clock	clock;
 
 	if (info == NULL) {
@@ -992,7 +993,8 @@ static unsigned int __devinit init_chips
 	 * Copy everything from a static "template" structure
 	 * to just allocated per-chip hpt_info structure.
 	 */
-	*info = *(struct hpt_info *)pci_get_drvdata(dev);
+	memcpy(info, pci_get_drvdata(dev), sizeof(struct hpt_info));
+	chip_type = info->chip_type;
 
 	pci_write_config_byte(dev, PCI_CACHE_LINE_SIZE, (L1_CACHE_BYTES / 4));
 	pci_write_config_byte(dev, PCI_LATENCY_TIMER, 0x78);
@@ -1002,7 +1004,7 @@ static unsigned int __devinit init_chips
 	/*
 	 * First, try to estimate the PCI clock frequency...
 	 */
-	if (info->chip_type >= HPT370) {
+	if (chip_type >= HPT370) {
 		u8  scr1  = 0;
 		u16 f_cnt = 0;
 		u32 temp  = 0;
@@ -1016,7 +1018,7 @@ static unsigned int __devinit init_chips
 		 * HighPoint does this for HPT372A.
 		 * NOTE: This register is only writeable via I/O space.
 		 */
-		if (info->chip_type == HPT372A)
+		if (chip_type == HPT372A)
 			outb(0x0e, io_base + 0x9c);
 
 		/*
@@ -1040,7 +1042,16 @@ static unsigned int __devinit init_chips
 		 * reading the f_CNT register itself in hopes that nobody has
 		 * touched the DPLL yet...
 		 */
-		temp = inl(io_base + 0x90);
+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
+							     dev->devfn - 1);
+			unsigned long io_base = pci_resource_start(dev1, 4);
+
+			temp =	inl(io_base + 0x90);
+			pci_dev_put(dev1);
+		} else
+			temp =	inl(io_base + 0x90);
+
 		if ((temp & 0xFFFFF000) != 0xABCDE000) {
 			int i;
 
@@ -1120,7 +1131,7 @@ static unsigned int __devinit init_chips
 	 * We also  don't like using  the DPLL because this causes glitches
 	 * on PRST-/SRST- when the state engine gets reset...
 	 */
-	if (info->chip_type >= HPT374 || info->settings[clock] == NULL) {
+	if (chip_type >= HPT374 || info->settings[clock] == NULL) {
 		u16 f_low, delta = pci_clk < 50 ? 2 : 4;
 		int adjust;
 
@@ -1190,7 +1201,7 @@ static unsigned int __devinit init_chips
 	/* Point to this chip's own instance of the hpt_info structure. */
 	pci_set_drvdata(dev, info);
 
-	if (info->chip_type >= HPT370) {
+	if (chip_type >= HPT370) {
 		u8  mcr1, mcr4;
 
 		/*
@@ -1209,7 +1220,7 @@ static unsigned int __devinit init_chips
 	 * the MISC. register to stretch the UltraDMA Tss timing.
 	 * NOTE: This register is only writeable via I/O space.
 	 */
-	if (info->chip_type == HPT371N && clock == ATA_CLOCK_66MHZ)
+	if (chip_type == HPT371N && clock == ATA_CLOCK_66MHZ)
 
 		outb(inb(io_base + 0x9c) | 0x04, io_base + 0x9c);
 

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bob Ham]

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

On Mon, 2007-08-06 at 00:06 +0400, Sergei Shtylyov wrote: 
> HPT374 BIOS seems to only save f_CNT register value for the function #0 before
> re-tuning DPLL causing the driver to report obviously distorted f_CNT for the
> function #1 -- fix this by always reading the saved f_CNT register value from
> in the init_chipset() method the function #0 of HPT374 chip.
> While at it, introduce 'chip_type' for the copy of the 'struct hpt_info' member
> and replace the structure assignment by memcpy()...

HPT374: IDE controller at PCI slot 0000:00:0d.0
ACPI: PCI Interrupt 0000:00:0d.0[A] -> GSI 16 (level, low) -> IRQ 16
HPT374: chipset revision 7
HPT374: DPLL base: 48 MHz, f_CNT: 142, assuming 33 MHz PCI
HPT374: using 50 MHz DPLL clock
HPT374: 100% native mode on irq 16
    ide2: BM-DMA at 0xec00-0xec07, BIOS settings: hde:DMA, hdf:pio
    ide3: BM-DMA at 0xec08-0xec0f, BIOS settings: hdg:DMA, hdh:pio
ACPI: PCI Interrupt 0000:00:0d.1[A] -> GSI 16 (level, low) -> IRQ 16
HPT374: DPLL base: 48 MHz, f_CNT: 142, assuming 33 MHz PCI
HPT374: using 50 MHz DPLL clock
    ide4: BM-DMA at 0xed00-0xed07, BIOS settings: hdi:DMA, hdj:pio
    ide5: BM-DMA at 0xed08-0xed0f, BIOS settings: hdk:DMA, hdl:pio


again, followed by a hard lock

-- 
Bob Ham <rah@bash.sh>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Bob Ham wrote:

>>HPT374 BIOS seems to only save f_CNT register value for the function #0 before
>>re-tuning DPLL causing the driver to report obviously distorted f_CNT for the
>>function #1 -- fix this by always reading the saved f_CNT register value from
>>in the init_chipset() method the function #0 of HPT374 chip.
>>While at it, introduce 'chip_type' for the copy of the 'struct hpt_info' member
>>and replace the structure assignment by memcpy()...

> HPT374: IDE controller at PCI slot 0000:00:0d.0
> ACPI: PCI Interrupt 0000:00:0d.0[A] -> GSI 16 (level, low) -> IRQ 16
> HPT374: chipset revision 7
> HPT374: DPLL base: 48 MHz, f_CNT: 142, assuming 33 MHz PCI
> HPT374: using 50 MHz DPLL clock
> HPT374: 100% native mode on irq 16
>     ide2: BM-DMA at 0xec00-0xec07, BIOS settings: hde:DMA, hdf:pio
>     ide3: BM-DMA at 0xec08-0xec0f, BIOS settings: hdg:DMA, hdh:pio
> ACPI: PCI Interrupt 0000:00:0d.1[A] -> GSI 16 (level, low) -> IRQ 16
> HPT374: DPLL base: 48 MHz, f_CNT: 142, assuming 33 MHz PCI
> HPT374: using 50 MHz DPLL clock
>     ide4: BM-DMA at 0xed00-0xed07, BIOS settings: hdi:DMA, hdj:pio
>     ide5: BM-DMA at 0xed08-0xed0f, BIOS settings: hdk:DMA, hdl:pio

> again, followed by a hard lock

    Hm, well, this is indeed tough case but at least it prodded me to fix some 
issues.  Maybe it's worth for you to file a bug at bugzilla.kernel.org...

MBR, Sergei

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Alan Cox]

> +		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
> +			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
> +							     dev->devfn - 1);

Can be NULL
> +			unsigned long io_base = pci_resource_start(dev1, 4);

Kaboom

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bob Ham]

On Mon, 2007-08-06 at 21:18 +0400, Sergei Shtylyov wrote:
>     Hm, well, this is indeed tough case but at least it prodded me to fix some 
> issues.  Maybe it's worth for you to file a bug at bugzilla.kernel.org...

I've raised bug 8851:

  http://bugzilla.kernel.org/show_bug.cgi?id=8851

Bob

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bartlomiej Zolnierkiewicz]

On Sunday 05 August 2007, Sergei Shtylyov wrote:
> HPT374 BIOS seems to only save f_CNT register value for the function #0 before
> re-tuning DPLL causing the driver to report obviously distorted f_CNT for the
> function #1 -- fix this by always reading the saved f_CNT register value from
> in the init_chipset() method the function #0 of HPT374 chip.
> While at it, introduce 'chip_type' for the copy of the 'struct hpt_info' member
> and replace the structure assignment by memcpy()...
> 
> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

applied but the issue raised by Alan needs addressing

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Alan Cox wrote:

>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
>>+							     dev->devfn - 1);

> Can be NULL

    Not really. This may not be called if it's NULL -- see hpt374_init_setup().
Maybe worth a comment though...

>>+			unsigned long io_base = pci_resource_start(dev1, 4);

> Kaboom

    That was a dud bomb. ;-)

MBR, Sergei

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Alan Cox]

On Fri, 10 Aug 2007 19:41:59 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> Alan Cox wrote:
> 
> >>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
> >>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
> >>+							     dev->devfn - 1);
> 
> > Can be NULL
> 
>     Not really. This may not be called if it's NULL -- see hpt374_init_setup().
> Maybe worth a comment though...
> 
> >>+			unsigned long io_base = pci_resource_start(dev1, 4);
> 
> > Kaboom
> 
>     That was a dud bomb. ;-)


What stops a hot unplug of a 374 from causing that to occur. I don't see
where you have the other pci_dev pinned on a hotplug on a box set to scan
the devices in reverse order

(yes its an extremely obscure case ;))

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Alan Cox wrote:

>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
>>>>+							     dev->devfn - 1);

>>>Can be NULL

>>    Not really. This may not be called if it's NULL -- see hpt374_init_setup().
>>Maybe worth a comment though...

>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);
>>
>>>Kaboom
>>
>>    That was a dud bomb. ;-)

> What stops a hot unplug of a 374 from causing that to occur. I don't see

    Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
The IDE core does that for me.

> where you have the other pci_dev pinned on a hotplug on a box set to scan
> the devices in reverse order

    Function 1 will always be skipped, regardless of the scan order.

> (yes its an extremely obscure case ;))

    "Security through obscurity". :-)

MBR, Sergei

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bartlomiej Zolnierkiewicz]

On Friday 10 August 2007, Sergei Shtylyov wrote:
> Alan Cox wrote:
> 
> >>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
> >>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
> >>>>+							     dev->devfn - 1);
> 
> >>>Can be NULL
> 
> >>    Not really. This may not be called if it's NULL -- see hpt374_init_setup().
> >>Maybe worth a comment though...
> 
> >>>>+			unsigned long io_base = pci_resource_start(dev1, 4);
> >>
> >>>Kaboom
> >>
> >>    That was a dud bomb. ;-)
> 
> > What stops a hot unplug of a 374 from causing that to occur. I don't see
> 
>     Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
> The IDE core does that for me.

ide_scan_pcibus() is used iff IDE is built-in.

Moreover pci_get_device() holds reference _only_ to the current PCI device
(the reference count to @from PCI device is _always_ decremented).

> > where you have the other pci_dev pinned on a hotplug on a box set to scan
> > the devices in reverse order
> 
>     Function 1 will always be skipped, regardless of the scan order.

Yes, but init_chipset_hpt366() will still try to access Function 1
even if earlier init_setup_hpt374() failed to obtain reference to it.

> > (yes its an extremely obscure case ;))
> 
>     "Security through obscurity". :-)

Not in this case. :-)

Bart

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
>>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
>>>>>>+							     dev->devfn - 1);

>>>>>Can be NULL

>>>>   Not really. This may not be called if it's NULL -- see hpt374_init_setup().
>>>>Maybe worth a comment though...

>>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);

>>>>>Kaboom

>>>>   That was a dud bomb. ;-)

>>>What stops a hot unplug of a 374 from causing that to occur. I don't see

>>    Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
>>The IDE core does that for me.

> ide_scan_pcibus() is used iff IDE is built-in.

> Moreover pci_get_device() holds reference _only_ to the current PCI device
> (the reference count to @from PCI device is _always_ decremented).

    Indeed... doesn't it look like a buglet in the IDE core?

>>>where you have the other pci_dev pinned on a hotplug on a box set to scan
>>>the devices in reverse order

>>    Function 1 will always be skipped, regardless of the scan order.

> Yes, but init_chipset_hpt366() will still try to access Function 1

    No!  Re-read the code please: init_chipset_hpt366() won't be called for 
function 1 if that one is not detected, and only in this case it does function 
0 access to read the saved f_CNT value.

> even if earlier init_setup_hpt374() failed to obtain reference to it.

>>>(yes its an extremely obscure case ;))

>>    "Security through obscurity". :-)

> Not in this case. :-)

    Yeah, here we have another case. ;-)

> Bart

WBR, Sergei

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
> >>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
> >>>>>>+							     dev->devfn - 1);
> 
> >>>>>Can be NULL
> 
> >>>>   Not really. This may not be called if it's NULL -- see hpt374_init_setup().
> >>>>Maybe worth a comment though...
> 
> >>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);
> 
> >>>>>Kaboom
> 
> >>>>   That was a dud bomb. ;-)
> 
> >>>What stops a hot unplug of a 374 from causing that to occur. I don't see
> 
> >>    Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
> >>The IDE core does that for me.
> 
> > ide_scan_pcibus() is used iff IDE is built-in.
> 
> > Moreover pci_get_device() holds reference _only_ to the current PCI device
> > (the reference count to @from PCI device is _always_ decremented).
> 
>     Indeed... doesn't it look like a buglet in the IDE core?

It is OK, when ide_scan_pcibus() is not used probing is done by PCI layer
and it keeps the reference to the PCI device in pci_device_probe().

> >>>where you have the other pci_dev pinned on a hotplug on a box set to scan
> >>>the devices in reverse order
> 
> >>    Function 1 will always be skipped, regardless of the scan order.
> 
> > Yes, but init_chipset_hpt366() will still try to access Function 1
> 
>     No!  Re-read the code please: init_chipset_hpt366() won't be called for 
> function 1 if that one is not detected, and only in this case it does function 
> 0 access to read the saved f_CNT value.

Argh, thinko on my side.

Unfortunately patch needs fixing anyway since the new comment is incorrect
because the reference is kept by hpt366 driver itself - pci_get_slot() in
init_setup_hpt374() - not the IDE core...

Thanks,
Bart

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
>>>>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
>>>>>>>>+							     dev->devfn - 1);

>>>>>>>Can be NULL

>>>>>>  Not really. This may not be called if it's NULL -- see hpt374_init_setup().
>>>>>>Maybe worth a comment though...

>>>>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);

>>>>>>>Kaboom

>>>>>>  That was a dud bomb. ;-)

>>>>>What stops a hot unplug of a 374 from causing that to occur. I don't see

>>>>   Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
>>>>The IDE core does that for me.

>>>ide_scan_pcibus() is used iff IDE is built-in.

>>>Moreover pci_get_device() holds reference _only_ to the current PCI device
>>>(the reference count to @from PCI device is _always_ decremented).

>>    Indeed... doesn't it look like a buglet in the IDE core?

> It is OK, when ide_scan_pcibus() is not used

    But whan it is used?

> probing is done by PCI layer
> and it keeps the reference to the PCI device in pci_device_probe().

>>>>>where you have the other pci_dev pinned on a hotplug on a box set to scan
>>>>>the devices in reverse order

>>>>   Function 1 will always be skipped, regardless of the scan order.

>>>Yes, but init_chipset_hpt366() will still try to access Function 1

>>    No!  Re-read the code please: init_chipset_hpt366() won't be called for 
>>function 1 if that one is not detected, and only in this case it does function 
>>0 access to read the saved f_CNT value.

> Argh, thinko on my side.

    Alas, it's your thinko, *again*. ;-)

> Unfortunately patch needs fixing anyway since the new comment is incorrect
> because the reference is kept by hpt366 driver itself - pci_get_slot() in
> init_setup_hpt374() - not the IDE core...

    Actually, init_setup_hpt374() grabs function 1 to register pair of devices 
(in hopes that the callers have already "pinned" function 0), and 
init_chipset_hpt366() "pins" function 0 to read the saved f_CNT.

> Thanks,
> Bart

MBR, Sergei

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
> >>>>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
> >>>>>>>>+							     dev->devfn - 1);
> 
> >>>>>>>Can be NULL
> 
> >>>>>>  Not really. This may not be called if it's NULL -- see hpt374_init_setup().
> >>>>>>Maybe worth a comment though...
> 
> >>>>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);
> 
> >>>>>>>Kaboom
> 
> >>>>>>  That was a dud bomb. ;-)
> 
> >>>>>What stops a hot unplug of a 374 from causing that to occur. I don't see
> 
> >>>>   Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
> >>>>The IDE core does that for me.
> 
> >>>ide_scan_pcibus() is used iff IDE is built-in.
> 
> >>>Moreover pci_get_device() holds reference _only_ to the current PCI device
> >>>(the reference count to @from PCI device is _always_ decremented).
> 
> >>    Indeed... doesn't it look like a buglet in the IDE core?
> 
> > It is OK, when ide_scan_pcibus() is not used
> 
>     But whan it is used?

Then it keeps the reference to PCI device itself by using pci_get_device()...

> > probing is done by PCI layer
> > and it keeps the reference to the PCI device in pci_device_probe().
> 
> >>>>>where you have the other pci_dev pinned on a hotplug on a box set to scan
> >>>>>the devices in reverse order
> 
> >>>>   Function 1 will always be skipped, regardless of the scan order.
> 
> >>>Yes, but init_chipset_hpt366() will still try to access Function 1
> 
> >>    No!  Re-read the code please: init_chipset_hpt366() won't be called for 
> >>function 1 if that one is not detected, and only in this case it does function 
> >>0 access to read the saved f_CNT value.
> 
> > Argh, thinko on my side.
> 
>     Alas, it's your thinko, *again*. ;-)
> 
> > Unfortunately patch needs fixing anyway since the new comment is incorrect
> > because the reference is kept by hpt366 driver itself - pci_get_slot() in
> > init_setup_hpt374() - not the IDE core...
> 
>     Actually, init_setup_hpt374() grabs function 1 to register pair of devices 
> (in hopes that the callers have already "pinned" function 0), and 
> init_chipset_hpt366() "pins" function 0 to read the saved f_CNT.

Yep, this would make a nice code comment instead the current one. :)

Bart

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>>>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
>>>>>>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
>>>>>>>>>>+							     dev->devfn - 1);

>>>>>>>>>Can be NULL

>>>>>>>> Not really. This may not be called if it's NULL -- see hpt374_init_setup().
>>>>>>>>Maybe worth a comment though...

>>>>>>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);

>>>>>>>>>Kaboom

>>>>>>>> That was a dud bomb. ;-)

>>>>>>>What stops a hot unplug of a 374 from causing that to occur. I don't see

>>>>>>  Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
>>>>>>The IDE core does that for me.

>>>>>ide_scan_pcibus() is used iff IDE is built-in.

>>>>>Moreover pci_get_device() holds reference _only_ to the current PCI device
>>>>>(the reference count to @from PCI device is _always_ decremented).

>>>>   Indeed... doesn't it look like a buglet in the IDE core?

>>>It is OK, when ide_scan_pcibus() is not used

>>    But whan it is used?

> Then it keeps the reference to PCI device itself by using pci_get_device()...

    Well, that I know. What I was asking is where the reference is kept after 
the driver init time -- we're still working with the PCI device, so unplagging 
it would be wrong thing to do. Actually, we should never call pci_dev_put() 
since IDE drivers are not unloadable, right?

>>>probing is done by PCI layer
>>>and it keeps the reference to the PCI device in pci_device_probe().

>>>>>>>where you have the other pci_dev pinned on a hotplug on a box set to scan
>>>>>>>the devices in reverse order

>>>>>>  Function 1 will always be skipped, regardless of the scan order.

>>>>>Yes, but init_chipset_hpt366() will still try to access Function 1

>>>>   No!  Re-read the code please: init_chipset_hpt366() won't be called for 
>>>>function 1 if that one is not detected, and only in this case it does function 
>>>>0 access to read the saved f_CNT value.

>>>Argh, thinko on my side.

>>    Alas, it's your thinko, *again*. ;-)

>>>Unfortunately patch needs fixing anyway since the new comment is incorrect
>>>because the reference is kept by hpt366 driver itself - pci_get_slot() in
>>>init_setup_hpt374() - not the IDE core...

>>    Actually, init_setup_hpt374() grabs function 1 to register pair of devices 
>>(in hopes that the callers have already "pinned" function 0), and 
>>init_chipset_hpt366() "pins" function 0 to read the saved f_CNT.

> Yep, this would make a nice code comment instead the current one. :)

   Ugh, will try to reword it... :-|

> Bart

MBR, Sergei

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Bartlomiej Zolnierkiewicz]

On Saturday 11 August 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>>>>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
> >>>>>>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
> >>>>>>>>>>+							     dev->devfn - 1);
> 
> >>>>>>>>>Can be NULL
> 
> >>>>>>>> Not really. This may not be called if it's NULL -- see hpt374_init_setup().
> >>>>>>>>Maybe worth a comment though...
> 
> >>>>>>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);
> 
> >>>>>>>>>Kaboom
> 
> >>>>>>>> That was a dud bomb. ;-)
> 
> >>>>>>>What stops a hot unplug of a 374 from causing that to occur. I don't see
> 
> >>>>>>  Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
> >>>>>>The IDE core does that for me.
> 
> >>>>>ide_scan_pcibus() is used iff IDE is built-in.
> 
> >>>>>Moreover pci_get_device() holds reference _only_ to the current PCI device
> >>>>>(the reference count to @from PCI device is _always_ decremented).
> 
> >>>>   Indeed... doesn't it look like a buglet in the IDE core?
> 
> >>>It is OK, when ide_scan_pcibus() is not used
> 
> >>    But whan it is used?
> 
> > Then it keeps the reference to PCI device itself by using pci_get_device()...
> 
>     Well, that I know. What I was asking is where the reference is kept after 
> the driver init time -- we're still working with the PCI device, so unplagging 
> it would be wrong thing to do. Actually, we should never call pci_dev_put() 
> since IDE drivers are not unloadable, right?

Now that I look at pci_device_probe() it seems that there should be additional
pci_dev_get() call in ide_scan_pcidev() if d->probe() succeeds.  Care to fix it?

> >>>probing is done by PCI layer
> >>>and it keeps the reference to the PCI device in pci_device_probe().
> 
> >>>>>>>where you have the other pci_dev pinned on a hotplug on a box set to scan
> >>>>>>>the devices in reverse order
> 
> >>>>>>  Function 1 will always be skipped, regardless of the scan order.
> 
> >>>>>Yes, but init_chipset_hpt366() will still try to access Function 1
> 
> >>>>   No!  Re-read the code please: init_chipset_hpt366() won't be called for 
> >>>>function 1 if that one is not detected, and only in this case it does function 
> >>>>0 access to read the saved f_CNT value.
> 
> >>>Argh, thinko on my side.
> 
> >>    Alas, it's your thinko, *again*. ;-)
> 
> >>>Unfortunately patch needs fixing anyway since the new comment is incorrect
> >>>because the reference is kept by hpt366 driver itself - pci_get_slot() in
> >>>init_setup_hpt374() - not the IDE core...
> 
> >>    Actually, init_setup_hpt374() grabs function 1 to register pair of devices 
> >>(in hopes that the callers have already "pinned" function 0), and 
> >>init_chipset_hpt366() "pins" function 0 to read the saved f_CNT.
> 
> > Yep, this would make a nice code comment instead the current one. :)
> 
>    Ugh, will try to reword it... :-|

Thanks.

Bart

Re: [PATCH 1/2] hpt366: fix PCI clock detection for HPT374

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>>>>>>>>>+		if (chip_type == HPT374 && (PCI_FUNC(dev->devfn) & 1)) {
>>>>>>>>>>>>+			struct pci_dev	*dev1 = pci_get_slot(dev->bus,
>>>>>>>>>>>>+							     dev->devfn - 1);

>>>>>>>>>>>Can be NULL

>>>>>>>>>>Not really. This may not be called if it's NULL -- see hpt374_init_setup().
>>>>>>>>>>Maybe worth a comment though...

>>>>>>>>>>>>+			unsigned long io_base = pci_resource_start(dev1, 4);

>>>>>>>>>>>Kaboom

>>>>>>>>>>That was a dud bomb. ;-)

>>>>>>>>>What stops a hot unplug of a 374 from causing that to occur. I don't see

>>>>>>>> Pinned as in pci_get_device()? If so, see setup-ide.c:ide_scan_pcibus().
>>>>>>>>The IDE core does that for me.

>>>>>>>ide_scan_pcibus() is used iff IDE is built-in.

>>>>>>>Moreover pci_get_device() holds reference _only_ to the current PCI device
>>>>>>>(the reference count to @from PCI device is _always_ decremented).

>>>>>>  Indeed... doesn't it look like a buglet in the IDE core?

>>>>>It is OK, when ide_scan_pcibus() is not used

>>>>   But whan it is used?

>>>Then it keeps the reference to PCI device itself by using pci_get_device()...

>>    Well, that I know. What I was asking is where the reference is kept after 
>>the driver init time -- we're still working with the PCI device, so unplagging 
>>it would be wrong thing to do. Actually, we should never call pci_dev_put() 
>>since IDE drivers are not unloadable, right?

> Now that I look at pci_device_probe() it seems that there should be additional
> pci_dev_get() call in ide_scan_pcidev() if d->probe() succeeds.  Care to fix it?

    Yeah. OK to convert it to kernel style by the same patch?

MBR, Sergei