Hi Bart,
i broke ide-floppy for Iomega ZIP drives with the last round of generic patches
and now it works only sometimes during write requests. The reason for it is that
the command issue path is not being delayed with a 50msec timeout, for details
see the comment in idefloppy_start_pc(). Anyway, attached is a fix that should
go into the -stable kernel too since the driver is now broken in 2.6.26.
On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
up here with the following error:
[ 4.296729] Uniform Multi-Platform E-IDE driver
[ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
[ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
[ 4.298153] ICH4: not 100% native mode: will probe irqs later
[ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
[ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
[ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
[ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
[ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
[ 4.868027] hda: UDMA/33 mode selected
[ 4.868441] hdb: UDMA/100 mode selected
[ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
[ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
[ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
[ 5.847362] hdd: UDMA/33 mode selected
[ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
[ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
[ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
[ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
[ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
[ 11.342504] hde: no response (status = 0xa1), resetting drive
[ 17.206535] hdf: no response (status = 0xa1), resetting drive
[ 17.614474] ------------[ cut here ]------------
[ 17.614528] WARNING: at lib/kref.c:43 kref_get+0x1a/0x20()
[ 17.614586] Modules linked in:
[ 17.614681] Pid: 1, comm: swapper Not tainted 2.6.26 #33
[ 17.614738] [<c01220e9>] warn_on_slowpath+0x41/0x7b
[ 17.614839] [<c02efa9a>] ? _spin_unlock_irq+0x2d/0x42
[ 17.614980] [<c011d1e7>] ? finish_task_switch+0x47/0x94
[ 17.615118] [<c011d1cb>] ? finish_task_switch+0x2b/0x94
[ 17.615257] [<c02effa8>] ? __reacquire_kernel_lock+0x33/0x37
[ 17.615396] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.615552] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.615693] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.615830] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.615968] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.616107] [<c01d86ae>] kref_get+0x1a/0x20
[ 17.616204] [<c01d7c39>] kobject_get+0x12/0x17
[ 17.616301] [<c01d7ce0>] kobject_add_internal+0x44/0x14f
[ 17.616399] [<c01d7e69>] kobject_add_varg+0x4a/0x4c
[ 17.617153] [<c01d7ed0>] kobject_add+0x43/0x49
[ 17.617252] [<c022a5dd>] device_add+0x91/0x48e
[ 17.617353] [<c022a2aa>] ? device_initialize+0xd7/0xf8
[ 17.617510] [<c022a9ec>] device_register+0x12/0x15
[ 17.617606] [<c02376b1>] ide_host_register+0x284/0x537
[ 17.617706] [<c0237af8>] ? ide_host_alloc_all+0x123/0x178
[ 17.617845] [<c04227ba>] ide_generic_init+0x142/0x1e7
[ 17.617946] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.618084] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.618226] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.618363] [<c02efa9a>] ? _spin_unlock_irq+0x2d/0x42
[ 17.618516] [<c011d1e7>] ? finish_task_switch+0x47/0x94
[ 17.618655] [<c02effa8>] ? __reacquire_kernel_lock+0x33/0x37
[ 17.618796] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.618938] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.619077] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.619218] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.619357] [<c02eeb4d>] ? mutex_unlock+0x8/0xa
[ 17.619512] [<c01a0334>] ? sysfs_addrm_finish+0x17/0x1cd
[ 17.619649] [<c02efb2e>] ? _spin_unlock+0x27/0x3c
[ 17.619788] [<c017bad9>] ? ifind+0x7e/0x88
[ 17.619926] [<c019fdd8>] ? sysfs_ilookup_test+0x0/0x11
[ 17.620068] [<c019ffa7>] ? sysfs_find_dirent+0x16/0x27
[ 17.620206] [<c01a00a0>] ? sysfs_add_one+0x14/0x85
[ 17.620344] [<c019fc8e>] ? sysfs_add_file_mode+0x4e/0x6d
[ 17.620502] [<c019fcbb>] ? sysfs_add_file+0xe/0x13
[ 17.620637] [<c040d2eb>] kernel_init+0x127/0x257
[ 17.620739] [<c0422678>] ? ide_generic_init+0x0/0x1e7
[ 17.620879] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.621019] [<c01dbe18>] ? trace_hardirqs_on_thunk+0xc/0x10
[ 17.621159] [<c0102eb6>] ? restore_nocheck_notrace+0x0/0xe
[ 17.621298] [<c040d1c4>] ? kernel_init+0x0/0x257
[ 17.621438] [<c040d1c4>] ? kernel_init+0x0/0x257
[ 17.621591] [<c0103a8f>] kernel_thread_helper+0x7/0x10
[ 17.621691] =======================
[ 17.621759] ---[ end trace 01bb572fb1fb92e8 ]---
[ 17.621835] BUG: unable to handle kernel paging request at 6f690074
[ 17.621968] IP: [<6f690074>]
[ 17.622060] *pde = 00000000
[ 17.622153] Oops: 0000 [#1] PREEMPT SMP
[ 17.622370] Modules linked in:
[ 17.622458]
[ 17.622506] Pid: 1, comm: swapper Tainted: G W (2.6.26 #33)
[ 17.622617] EIP: 0060:[<6f690074>] EFLAGS: 00010206 CPU: 0
[ 17.622670] EIP is at 0x6f690074
[ 17.622720] EAX: dfa1b5c8 EBX: c03e630c ECX: 6f690074 EDX: c01ec367
[ 17.622774] ESI: dfa1b5c8 EDI: c069bd4c EBP: df82fc24 ESP: df82fc14
[ 17.622884] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[ 17.622937] Process swapper (pid: 1, ti=df82e000 task=df830000 task.ti=df82e000)
[ 17.622992] Stack: c01ec382 c03e630c dfa1b614 00000000 df82fc34 c02e11c6 c03e630c dfa1b614
[ 17.623379] df82fc44 c02e1255 dfa1b5e8 dfa1b5e8 df82fc74 c022a8cf dfa1b6e4 dfa1b6c0
[ 17.623379] c03e630c dfa1b5e8 00000000 00000000 c022a2aa dfa1b5e8 dfa1b5e8 00000006
[ 17.623379] Call Trace:
[ 17.623379] [<c01ec382>] ? pci_device_suspend+0x1b/0x4d
[ 17.623379] [<c02e11c6>] ? klist_node_init+0x36/0x3a
[ 17.623379] [<c02e1255>] ? klist_add_tail+0x12/0x38
[ 17.623379] [<c022a8cf>] ? device_add+0x383/0x48e
[ 17.623379] [<c022a2aa>] ? device_initialize+0xd7/0xf8
[ 17.623379] [<c022a9ec>] ? device_register+0x12/0x15
[ 17.623379] [<c02376b1>] ? ide_host_register+0x284/0x537
[ 17.623379] [<c0237af8>] ? ide_host_alloc_all+0x123/0x178
[ 17.623379] [<c04227ba>] ? ide_generic_init+0x142/0x1e7
[ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.623379] [<c02efa9a>] ? _spin_unlock_irq+0x2d/0x42
[ 17.623379] [<c011d1e7>] ? finish_task_switch+0x47/0x94
[ 17.623379] [<c02effa8>] ? __reacquire_kernel_lock+0x33/0x37
[ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
[ 17.623379] [<c02eeb4d>] ? mutex_unlock+0x8/0xa
[ 17.623379] [<c01a0334>] ? sysfs_addrm_finish+0x17/0x1cd
[ 17.623379] [<c02efb2e>] ? _spin_unlock+0x27/0x3c
[ 17.623379] [<c017bad9>] ? ifind+0x7e/0x88
[ 17.623379] [<c019fdd8>] ? sysfs_ilookup_test+0x0/0x11
[ 17.623379] [<c019ffa7>] ? sysfs_find_dirent+0x16/0x27
[ 17.623379] [<c01a00a0>] ? sysfs_add_one+0x14/0x85
[ 17.623379] [<c019fc8e>] ? sysfs_add_file_mode+0x4e/0x6d
[ 17.623379] [<c019fcbb>] ? sysfs_add_file+0xe/0x13
[ 17.623379] [<c040d2eb>] ? kernel_init+0x127/0x257
[ 17.623379] [<c0422678>] ? ide_generic_init+0x0/0x1e7
[ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
[ 17.623379] [<c01dbe18>] ? trace_hardirqs_on_thunk+0xc/0x10
[ 17.623379] [<c0102eb6>] ? restore_nocheck_notrace+0x0/0xe
[ 17.623379] [<c040d1c4>] ? kernel_init+0x0/0x257
[ 17.623379] [<c040d1c4>] ? kernel_init+0x0/0x257
[ 17.623379] [<c0103a8f>] ? kernel_thread_helper+0x7/0x10
[ 17.623379] =======================
[ 17.623379] Code: Bad EIP value.
[ 17.623379] EIP: [<6f690074>] 0x6f690074 SS:ESP 0068:df82fc14
[ 17.630502] ---[ end trace 01bb572fb1fb92e8 ]---
[ 17.630557] Kernel panic - not syncing: Attempted to kill init!
I tracked the error down to the call to ide_register_port(hwif) in
ide-probe.c:ide_host_register() which does device_register(&hwif->gendev) and
the hwif->gendev->kobj seems unitialized thus the WARN_ON on its refcount in
kref_get(). Will look into it more when i get some free time.
--
From: Borislav Petkov <[email protected]>
Check the correct flags-location for set features.
Signed-off-by: Borislav Petkov <[email protected]>
---
diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
index 97cabfd..ddabad9 100644
--- a/drivers/ide/ide-atapi.c
+++ b/drivers/ide/ide-atapi.c
@@ -257,7 +257,7 @@ ide_startstop_t ide_transfer_pc(ide_drive_t *drive, struct ide_atapi_pc *pc,
}
/* Send the actual packet */
- if ((pc->flags & IDE_DFLAG_ZIP_DRIVE) == 0)
+ if ((drive->dev_flags & IDE_DFLAG_ZIP_DRIVE) == 0)
hwif->tp_ops->output_data(drive, NULL, rq->cmd, 12);
return ide_started;
--
Regards/Gru?,
Boris.
On Tue, Jul 15, 2008 at 07:33:56AM +0200, Borislav Petkov wrote:
> Hi Bart,
>
> i broke ide-floppy for Iomega ZIP drives with the last round of generic patches
> and now it works only sometimes during write requests. The reason for it is that
> the command issue path is not being delayed with a 50msec timeout, for details
> see the comment in idefloppy_start_pc(). Anyway, attached is a fix that should
> go into the -stable kernel too since the driver is now broken in 2.6.26.
>
> On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> up here with the following error:
>
>
> [ 4.296729] Uniform Multi-Platform E-IDE driver
> [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> [ 4.868027] hda: UDMA/33 mode selected
> [ 4.868441] hdb: UDMA/100 mode selected
> [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> [ 5.847362] hdd: UDMA/33 mode selected
> [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> [ 11.342504] hde: no response (status = 0xa1), resetting drive
> [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> [ 17.614474] ------------[ cut here ]------------
> [ 17.614528] WARNING: at lib/kref.c:43 kref_get+0x1a/0x20()
> [ 17.614586] Modules linked in:
> [ 17.614681] Pid: 1, comm: swapper Not tainted 2.6.26 #33
> [ 17.614738] [<c01220e9>] warn_on_slowpath+0x41/0x7b
> [ 17.614839] [<c02efa9a>] ? _spin_unlock_irq+0x2d/0x42
> [ 17.614980] [<c011d1e7>] ? finish_task_switch+0x47/0x94
> [ 17.615118] [<c011d1cb>] ? finish_task_switch+0x2b/0x94
> [ 17.615257] [<c02effa8>] ? __reacquire_kernel_lock+0x33/0x37
> [ 17.615396] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.615552] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.615693] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.615830] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.615968] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.616107] [<c01d86ae>] kref_get+0x1a/0x20
> [ 17.616204] [<c01d7c39>] kobject_get+0x12/0x17
> [ 17.616301] [<c01d7ce0>] kobject_add_internal+0x44/0x14f
> [ 17.616399] [<c01d7e69>] kobject_add_varg+0x4a/0x4c
> [ 17.617153] [<c01d7ed0>] kobject_add+0x43/0x49
> [ 17.617252] [<c022a5dd>] device_add+0x91/0x48e
> [ 17.617353] [<c022a2aa>] ? device_initialize+0xd7/0xf8
> [ 17.617510] [<c022a9ec>] device_register+0x12/0x15
> [ 17.617606] [<c02376b1>] ide_host_register+0x284/0x537
> [ 17.617706] [<c0237af8>] ? ide_host_alloc_all+0x123/0x178
> [ 17.617845] [<c04227ba>] ide_generic_init+0x142/0x1e7
> [ 17.617946] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.618084] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.618226] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.618363] [<c02efa9a>] ? _spin_unlock_irq+0x2d/0x42
> [ 17.618516] [<c011d1e7>] ? finish_task_switch+0x47/0x94
> [ 17.618655] [<c02effa8>] ? __reacquire_kernel_lock+0x33/0x37
> [ 17.618796] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.618938] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.619077] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.619218] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.619357] [<c02eeb4d>] ? mutex_unlock+0x8/0xa
> [ 17.619512] [<c01a0334>] ? sysfs_addrm_finish+0x17/0x1cd
> [ 17.619649] [<c02efb2e>] ? _spin_unlock+0x27/0x3c
> [ 17.619788] [<c017bad9>] ? ifind+0x7e/0x88
> [ 17.619926] [<c019fdd8>] ? sysfs_ilookup_test+0x0/0x11
> [ 17.620068] [<c019ffa7>] ? sysfs_find_dirent+0x16/0x27
> [ 17.620206] [<c01a00a0>] ? sysfs_add_one+0x14/0x85
> [ 17.620344] [<c019fc8e>] ? sysfs_add_file_mode+0x4e/0x6d
> [ 17.620502] [<c019fcbb>] ? sysfs_add_file+0xe/0x13
> [ 17.620637] [<c040d2eb>] kernel_init+0x127/0x257
> [ 17.620739] [<c0422678>] ? ide_generic_init+0x0/0x1e7
> [ 17.620879] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.621019] [<c01dbe18>] ? trace_hardirqs_on_thunk+0xc/0x10
> [ 17.621159] [<c0102eb6>] ? restore_nocheck_notrace+0x0/0xe
> [ 17.621298] [<c040d1c4>] ? kernel_init+0x0/0x257
> [ 17.621438] [<c040d1c4>] ? kernel_init+0x0/0x257
> [ 17.621591] [<c0103a8f>] kernel_thread_helper+0x7/0x10
> [ 17.621691] =======================
> [ 17.621759] ---[ end trace 01bb572fb1fb92e8 ]---
> [ 17.621835] BUG: unable to handle kernel paging request at 6f690074
> [ 17.621968] IP: [<6f690074>]
> [ 17.622060] *pde = 00000000
> [ 17.622153] Oops: 0000 [#1] PREEMPT SMP
> [ 17.622370] Modules linked in:
> [ 17.622458]
> [ 17.622506] Pid: 1, comm: swapper Tainted: G W (2.6.26 #33)
> [ 17.622617] EIP: 0060:[<6f690074>] EFLAGS: 00010206 CPU: 0
> [ 17.622670] EIP is at 0x6f690074
> [ 17.622720] EAX: dfa1b5c8 EBX: c03e630c ECX: 6f690074 EDX: c01ec367
> [ 17.622774] ESI: dfa1b5c8 EDI: c069bd4c EBP: df82fc24 ESP: df82fc14
> [ 17.622884] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> [ 17.622937] Process swapper (pid: 1, ti=df82e000 task=df830000 task.ti=df82e000)
> [ 17.622992] Stack: c01ec382 c03e630c dfa1b614 00000000 df82fc34 c02e11c6 c03e630c dfa1b614
> [ 17.623379] df82fc44 c02e1255 dfa1b5e8 dfa1b5e8 df82fc74 c022a8cf dfa1b6e4 dfa1b6c0
> [ 17.623379] c03e630c dfa1b5e8 00000000 00000000 c022a2aa dfa1b5e8 dfa1b5e8 00000006
> [ 17.623379] Call Trace:
> [ 17.623379] [<c01ec382>] ? pci_device_suspend+0x1b/0x4d
> [ 17.623379] [<c02e11c6>] ? klist_node_init+0x36/0x3a
> [ 17.623379] [<c02e1255>] ? klist_add_tail+0x12/0x38
> [ 17.623379] [<c022a8cf>] ? device_add+0x383/0x48e
> [ 17.623379] [<c022a2aa>] ? device_initialize+0xd7/0xf8
> [ 17.623379] [<c022a9ec>] ? device_register+0x12/0x15
> [ 17.623379] [<c02376b1>] ? ide_host_register+0x284/0x537
> [ 17.623379] [<c0237af8>] ? ide_host_alloc_all+0x123/0x178
> [ 17.623379] [<c04227ba>] ? ide_generic_init+0x142/0x1e7
> [ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.623379] [<c02efa9a>] ? _spin_unlock_irq+0x2d/0x42
> [ 17.623379] [<c011d1e7>] ? finish_task_switch+0x47/0x94
> [ 17.623379] [<c02effa8>] ? __reacquire_kernel_lock+0x33/0x37
> [ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.623379] [<c013d1d9>] ? trace_hardirqs_on+0xb/0xd
> [ 17.623379] [<c02eeb4d>] ? mutex_unlock+0x8/0xa
> [ 17.623379] [<c01a0334>] ? sysfs_addrm_finish+0x17/0x1cd
> [ 17.623379] [<c02efb2e>] ? _spin_unlock+0x27/0x3c
> [ 17.623379] [<c017bad9>] ? ifind+0x7e/0x88
> [ 17.623379] [<c019fdd8>] ? sysfs_ilookup_test+0x0/0x11
> [ 17.623379] [<c019ffa7>] ? sysfs_find_dirent+0x16/0x27
> [ 17.623379] [<c01a00a0>] ? sysfs_add_one+0x14/0x85
> [ 17.623379] [<c019fc8e>] ? sysfs_add_file_mode+0x4e/0x6d
> [ 17.623379] [<c019fcbb>] ? sysfs_add_file+0xe/0x13
> [ 17.623379] [<c040d2eb>] ? kernel_init+0x127/0x257
> [ 17.623379] [<c0422678>] ? ide_generic_init+0x0/0x1e7
> [ 17.623379] [<c013d1ad>] ? trace_hardirqs_on_caller+0xe1/0x102
> [ 17.623379] [<c01dbe18>] ? trace_hardirqs_on_thunk+0xc/0x10
> [ 17.623379] [<c0102eb6>] ? restore_nocheck_notrace+0x0/0xe
> [ 17.623379] [<c040d1c4>] ? kernel_init+0x0/0x257
> [ 17.623379] [<c040d1c4>] ? kernel_init+0x0/0x257
> [ 17.623379] [<c0103a8f>] ? kernel_thread_helper+0x7/0x10
> [ 17.623379] =======================
> [ 17.623379] Code: Bad EIP value.
> [ 17.623379] EIP: [<6f690074>] 0x6f690074 SS:ESP 0068:df82fc14
> [ 17.630502] ---[ end trace 01bb572fb1fb92e8 ]---
> [ 17.630557] Kernel panic - not syncing: Attempted to kill init!
>
> I tracked the error down to the call to ide_register_port(hwif) in
> ide-probe.c:ide_host_register() which does device_register(&hwif->gendev) and
> the hwif->gendev->kobj seems unitialized thus the WARN_ON on its refcount in
i mean hwif->gendev->kobj->parent here.
> kref_get(). Will look into it more when i get some free time.
>
> --
> From: Borislav Petkov <[email protected]>
>
> Check the correct flags-location for set features.
>
> Signed-off-by: Borislav Petkov <[email protected]>
>
> ---
>
> diff --git a/drivers/ide/ide-atapi.c b/drivers/ide/ide-atapi.c
> index 97cabfd..ddabad9 100644
> --- a/drivers/ide/ide-atapi.c
> +++ b/drivers/ide/ide-atapi.c
> @@ -257,7 +257,7 @@ ide_startstop_t ide_transfer_pc(ide_drive_t *drive, struct ide_atapi_pc *pc,
> }
>
> /* Send the actual packet */
> - if ((pc->flags & IDE_DFLAG_ZIP_DRIVE) == 0)
> + if ((drive->dev_flags & IDE_DFLAG_ZIP_DRIVE) == 0)
> hwif->tp_ops->output_data(drive, NULL, rq->cmd, 12);
>
> return ide_started;
> --
> Regards/Gru?,
> Boris.
--
Regards/Gru?,
Boris.
Hi,
On Tuesday 15 July 2008, Borislav Petkov wrote:
[...]
> On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> up here with the following error:
>
> [ 4.296729] Uniform Multi-Platform E-IDE driver
> [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> [ 4.868027] hda: UDMA/33 mode selected
> [ 4.868441] hdb: UDMA/100 mode selected
> [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> [ 5.847362] hdd: UDMA/33 mode selected
> [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> [ 11.342504] hde: no response (status = 0xa1), resetting drive
> [ 17.206535] hdf: no response (status = 0xa1), resetting drive
hde? hdf?
[...]
> I tracked the error down to the call to ide_register_port(hwif) in
> ide-probe.c:ide_host_register() which does device_register(&hwif->gendev) and
> the hwif->gendev->kobj seems unitialized thus the WARN_ON on its refcount in
> kref_get(). Will look into it more when i get some free time.
Unfortunately I couldn't reproduce this problem here (2.6.26 + pata tree)
so please try to debug it and/or narrow it down to the guilty change.
> --
> From: Borislav Petkov <[email protected]>
>
> Check the correct flags-location for set features.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Thanks, I folded the fix into original patch (->dev_flags is not yet
upstream so -stable fix shouldn't be necessary).
While on it: I later noticed that there will be also need for common
ATA/ATAPI ->dev_flags in the future so I wonder whether current
->dev_flags should be renamed to ->atapi_flags (& s/*DFLAG*/*AFLAG*/).
If there is agreement on this I'll fix it in pata tree.
On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Tuesday 15 July 2008, Borislav Petkov wrote:
>
> [...]
>
> > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > up here with the following error:
> >
> > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > [ 4.868027] hda: UDMA/33 mode selected
> > [ 4.868441] hdb: UDMA/100 mode selected
> > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > [ 5.847362] hdd: UDMA/33 mode selected
> > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
>
> hde? hdf?
>
> [...]
yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
>
> > I tracked the error down to the call to ide_register_port(hwif) in
> > ide-probe.c:ide_host_register() which does device_register(&hwif->gendev) and
> > the hwif->gendev->kobj seems unitialized thus the WARN_ON on its refcount in
> > kref_get(). Will look into it more when i get some free time.
>
> Unfortunately I couldn't reproduce this problem here (2.6.26 + pata tree)
> so please try to debug it and/or narrow it down to the guilty change.
Will do.
> > --
> > From: Borislav Petkov <[email protected]>
> >
> > Check the correct flags-location for set features.
> >
> > Signed-off-by: Borislav Petkov <[email protected]>
>
> Thanks, I folded the fix into original patch (->dev_flags is not yet
> upstream so -stable fix shouldn't be necessary).
>
> While on it: I later noticed that there will be also need for common
> ATA/ATAPI ->dev_flags in the future so I wonder whether current
> ->dev_flags should be renamed to ->atapi_flags (& s/*DFLAG*/*AFLAG*/).
>
> If there is agreement on this I'll fix it in pata tree.
... or if there's room, use a single ->dev_flags for all possible flag settings?
--
Regards/Gru?,
Boris.
On Tuesday 15 July 2008, Borislav Petkov wrote:
> On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > Hi,
> >
> > On Tuesday 15 July 2008, Borislav Petkov wrote:
> >
> > [...]
> >
> > > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > > up here with the following error:
> > >
> > > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > > [ 4.868027] hda: UDMA/33 mode selected
> > > [ 4.868441] hdb: UDMA/100 mode selected
> > > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > > [ 5.847362] hdd: UDMA/33 mode selected
> > > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> >
> > hde? hdf?
> >
> > [...]
>
> yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
Close, it is related to MAX_HWIFS being upper bound on hws[]
in ide_generic.c but now we loop for ARRAY_SIZE(legacy_bases).
IOW there is a memset(hws, 0, MAX_HWIFS) missing at the top of
ide_generic_init() (please re-test after adding it).
This may also explain the later problems with ide_host_register().
> > > From: Borislav Petkov <[email protected]>
> > >
> > > Check the correct flags-location for set features.
> > >
> > > Signed-off-by: Borislav Petkov <[email protected]>
> >
> > Thanks, I folded the fix into original patch (->dev_flags is not yet
> > upstream so -stable fix shouldn't be necessary).
> >
> > While on it: I later noticed that there will be also need for common
> > ATA/ATAPI ->dev_flags in the future so I wonder whether current
> > ->dev_flags should be renamed to ->atapi_flags (& s/*DFLAG*/*AFLAG*/).
> >
> > If there is agreement on this I'll fix it in pata tree.
>
> ... or if there's room, use a single ->dev_flags for all possible flag settings?
Unfortunately there isn't enough room left (27 bits are occupied ATM)
and having u64 ->dev_flags sucks...
On Tue, Jul 15, 2008 at 10:58:48PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Tuesday 15 July 2008, Borislav Petkov wrote:
> > On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > >
> > > Hi,
> > >
> > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > >
> > > [...]
> > >
> > > > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > > > up here with the following error:
> > > >
> > > > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > > > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > > > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > > > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > > > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > > > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > > > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > > > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > > > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > [ 4.868027] hda: UDMA/33 mode selected
> > > > [ 4.868441] hdb: UDMA/100 mode selected
> > > > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > > > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > > > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > [ 5.847362] hdd: UDMA/33 mode selected
> > > > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > > > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > > > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > > > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> > >
> > > hde? hdf?
> > >
> > > [...]
> >
> > yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
>
> Close, it is related to MAX_HWIFS being upper bound on hws[]
> in ide_generic.c but now we loop for ARRAY_SIZE(legacy_bases).
>
> IOW there is a memset(hws, 0, MAX_HWIFS) missing at the top of
> ide_generic_init() (please re-test after adding it).
>
> This may also explain the later problems with ide_host_register().
>
> > > > From: Borislav Petkov <[email protected]>
> > > >
> > > > Check the correct flags-location for set features.
> > > >
> > > > Signed-off-by: Borislav Petkov <[email protected]>
> > >
> > > Thanks, I folded the fix into original patch (->dev_flags is not yet
> > > upstream so -stable fix shouldn't be necessary).
> > >
> > > While on it: I later noticed that there will be also need for common
> > > ATA/ATAPI ->dev_flags in the future so I wonder whether current
> > > ->dev_flags should be renamed to ->atapi_flags (& s/*DFLAG*/*AFLAG*/).
> > >
> > > If there is agreement on this I'll fix it in pata tree.
> >
> > ... or if there's room, use a single ->dev_flags for all possible flag settings?
>
> Unfortunately there isn't enough room left (27 bits are occupied ATM)
> and having u64 ->dev_flags sucks...
I'm pretty sure some of the 27 will be removed later but yeah, u64 flags is
kinda bad since it has to be always atomically updated and this has to be
explicitly staged on 32bit cpus due to the wordsize. I guess two flags members
are the easiest thing to do for now, you might add some comments to both so we
know which is which.
--
Regards/Gru?,
Boris.
On Wednesday 16 July 2008, Borislav Petkov wrote:
> On Tue, Jul 15, 2008 at 10:58:48PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > >
> > > > [...]
> > > >
> > > > > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > > > > up here with the following error:
> > > > >
> > > > > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > > > > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > > > > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > > > > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > > > > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > > > > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > > > > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > > > > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > > > > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > [ 4.868027] hda: UDMA/33 mode selected
> > > > > [ 4.868441] hdb: UDMA/100 mode selected
> > > > > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > > > > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > > > > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > [ 5.847362] hdd: UDMA/33 mode selected
> > > > > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > > > > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > > > > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > > > > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> > > >
> > > > hde? hdf?
> > > >
> > > > [...]
> > >
> > > yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
> >
> > Close, it is related to MAX_HWIFS being upper bound on hws[]
> > in ide_generic.c but now we loop for ARRAY_SIZE(legacy_bases).
> >
> > IOW there is a memset(hws, 0, MAX_HWIFS) missing at the top of
> > ide_generic_init() (please re-test after adding it).
> >
> > This may also explain the later problems with ide_host_register().
I fixed this in pata tree now with:
"ide-generic: remove ide_default_{io_base,irq}() inlines (take 2)" patch
> > > > > From: Borislav Petkov <[email protected]>
> > > > >
> > > > > Check the correct flags-location for set features.
> > > > >
> > > > > Signed-off-by: Borislav Petkov <[email protected]>
> > > >
> > > > Thanks, I folded the fix into original patch (->dev_flags is not yet
> > > > upstream so -stable fix shouldn't be necessary).
> > > >
> > > > While on it: I later noticed that there will be also need for common
> > > > ATA/ATAPI ->dev_flags in the future so I wonder whether current
> > > > ->dev_flags should be renamed to ->atapi_flags (& s/*DFLAG*/*AFLAG*/).
> > > >
> > > > If there is agreement on this I'll fix it in pata tree.
> > >
> > > ... or if there's room, use a single ->dev_flags for all possible flag settings?
> >
> > Unfortunately there isn't enough room left (27 bits are occupied ATM)
> > and having u64 ->dev_flags sucks...
>
> I'm pretty sure some of the 27 will be removed later but yeah, u64 flags is
> kinda bad since it has to be always atomically updated and this has to be
> explicitly staged on 32bit cpus due to the wordsize. I guess two flags members
->dev_flags -> ->atapi_flags & co. are also in pata tree now
> are the easiest thing to do for now, you might add some comments to both so we
> know which is which.
Sure, I will remember about this when it comes time for ->dev_flags.
Thanks,
Bart
Hi Bart,
On Wed, Jul 16, 2008 at 07:56:46PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 16 July 2008, Borislav Petkov wrote:
> > On Tue, Jul 15, 2008 at 10:58:48PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > > On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > > >
> > > > > [...]
> > > > >
> > > > > > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > > > > > up here with the following error:
> > > > > >
> > > > > > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > > > > > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > > > > > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > > > > > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > > > > > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > > > > > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > > > > > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > > > > > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > > > > > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > > [ 4.868027] hda: UDMA/33 mode selected
> > > > > > [ 4.868441] hdb: UDMA/100 mode selected
> > > > > > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > > > > > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > > > > > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > > [ 5.847362] hdd: UDMA/33 mode selected
> > > > > > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > > > > > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > > > > > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > > > > > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> > > > >
> > > > > hde? hdf?
> > > > >
> > > > > [...]
> > > >
> > > > yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
> > >
> > > Close, it is related to MAX_HWIFS being upper bound on hws[]
> > > in ide_generic.c but now we loop for ARRAY_SIZE(legacy_bases).
> > >
> > > IOW there is a memset(hws, 0, MAX_HWIFS) missing at the top of
> > > ide_generic_init() (please re-test after adding it).
> > >
> > > This may also explain the later problems with ide_host_register().
>
> I fixed this in pata tree now with:
>
> "ide-generic: remove ide_default_{io_base,irq}() inlines (take 2)" patch
here's the root cause for the problem: I had both Intel ICH chipset
(BLK_DEV_PIIX) und generic ide (BLK_DEV_GENERIC) selected in Kconfig and since
ICH4 uses the generic driver detection routine, the second(!) generic detection after the
ICH4 one failed and died. This is why request_region()-resources are shown as
not being free above:
> > > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
One of the possible fixes is adding
depends on !BLK_DEV_GENERIC
after each IDE chipset driver using the generic detection in drivers/ide/Kconfig
but it's a not-that-elegant one. Another thing would be using a dummy one like
BLK_DEV_IDEDMA_PCI, but I'm not that sure. Will look into it. I'm pretty sure
you have a better idea...
>
> > > > > > From: Borislav Petkov <[email protected]>
> > > > > >
> > > > > > Check the correct flags-location for set features.
> > > > > >
> > > > > > Signed-off-by: Borislav Petkov <[email protected]>
> > > > >
> > > > > Thanks, I folded the fix into original patch (->dev_flags is not yet
> > > > > upstream so -stable fix shouldn't be necessary).
> > > > >
> > > > > While on it: I later noticed that there will be also need for common
> > > > > ATA/ATAPI ->dev_flags in the future so I wonder whether current
> > > > > ->dev_flags should be renamed to ->atapi_flags (& s/*DFLAG*/*AFLAG*/).
> > > > >
> > > > > If there is agreement on this I'll fix it in pata tree.
> > > >
> > > > ... or if there's room, use a single ->dev_flags for all possible flag settings?
> > >
> > > Unfortunately there isn't enough room left (27 bits are occupied ATM)
> > > and having u64 ->dev_flags sucks...
> >
> > I'm pretty sure some of the 27 will be removed later but yeah, u64 flags is
> > kinda bad since it has to be always atomically updated and this has to be
> > explicitly staged on 32bit cpus due to the wordsize. I guess two flags members
>
> ->dev_flags -> ->atapi_flags & co. are also in pata tree now
>
> > are the easiest thing to do for now, you might add some comments to both so we
> > know which is which.
>
> Sure, I will remember about this when it comes time for ->dev_flags.
>
> Thanks,
> Bart
--
Regards/Gru?,
Boris.
On Sunday 20 July 2008, Borislav Petkov wrote:
> Hi Bart,
>
> On Wed, Jul 16, 2008 at 07:56:46PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 16 July 2008, Borislav Petkov wrote:
> > > On Tue, Jul 15, 2008 at 10:58:48PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > > > On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > > > > > > up here with the following error:
> > > > > > >
> > > > > > > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > > > > > > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > > > > > > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > > > > > > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > > > > > > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > > > > > > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > > > > > > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > > > > > > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > > > > > > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > > > [ 4.868027] hda: UDMA/33 mode selected
> > > > > > > [ 4.868441] hdb: UDMA/100 mode selected
> > > > > > > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > > > > > > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > > > > > > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > > > [ 5.847362] hdd: UDMA/33 mode selected
> > > > > > > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > > > > > > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > > > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > > > > > > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > > > > > > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> > > > > >
> > > > > > hde? hdf?
> > > > > >
> > > > > > [...]
> > > > >
> > > > > yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
> > > >
> > > > Close, it is related to MAX_HWIFS being upper bound on hws[]
> > > > in ide_generic.c but now we loop for ARRAY_SIZE(legacy_bases).
> > > >
> > > > IOW there is a memset(hws, 0, MAX_HWIFS) missing at the top of
> > > > ide_generic_init() (please re-test after adding it).
> > > >
> > > > This may also explain the later problems with ide_host_register().
> >
> > I fixed this in pata tree now with:
> >
> > "ide-generic: remove ide_default_{io_base,irq}() inlines (take 2)" patch
>
> here's the root cause for the problem: I had both Intel ICH chipset
> (BLK_DEV_PIIX) und generic ide (BLK_DEV_GENERIC) selected in Kconfig and since
> ICH4 uses the generic driver detection routine, the second(!) generic detection after the
> ICH4 one failed and died. This is why request_region()-resources are shown as
> not being free above:
>
> > > > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
I also use BLK_DEV_PIIX + BLK_DEV_GENERIC configuration
(for testing purposes) and it works fine here.
[ Besides it shouldn't result in phantom hde & hdf devices
and ide_generic blowing up on failure. ]
Have you tried the memset() fix that I proposed
(pata tree contains the revised patch now)?
> One of the possible fixes is adding
>
> depends on !BLK_DEV_GENERIC
>
> after each IDE chipset driver using the generic detection in drivers/ide/Kconfig
> but it's a not-that-elegant one. Another thing would be using a dummy one like
> BLK_DEV_IDEDMA_PCI, but I'm not that sure. Will look into it. I'm pretty sure
> you have a better idea...
pata_legacy.c has a proper fix which needs porting into ide-generic.c
(it should be pretty easy thing to do).
[ The fix is to skip automatic-probing of primary port and/or secondary
one if a PCI controller using legacy I/O bases is detected:
for_each_pci_dev(p) {
int r;
/* Check for any overlap of the system ATA mappings. Native
mode controllers stuck on these addresses or some devices
in 'raid' mode won't be found by the storage class test */
for (r = 0; r < 6; r++) {
if (pci_resource_start(p, r) == 0x1f0)
primary = 1;
if (pci_resource_start(p, r) == 0x170)
secondary = 1;
}
/* Check for special cases */
legacy_check_special_cases(p, &primary, &secondary); ]
Thanks,
Bart
On Mon, Jul 21, 2008 at 09:03:36PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Sunday 20 July 2008, Borislav Petkov wrote:
> > Hi Bart,
> >
> > On Wed, Jul 16, 2008 at 07:56:46PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > On Wednesday 16 July 2008, Borislav Petkov wrote:
> > > > On Tue, Jul 15, 2008 at 10:58:48PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > > > > On Wed, Jul 16, 2008 at 05:59:16PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Tuesday 15 July 2008, Borislav Petkov wrote:
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > On a different note, the current pata tree on top of v2.6.25-2125-g50515af blows
> > > > > > > > up here with the following error:
> > > > > > > >
> > > > > > > > [ 4.296729] Uniform Multi-Platform E-IDE driver
> > > > > > > > [ 4.297905] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
> > > > > > > > [ 4.297986] ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 18
> > > > > > > > [ 4.298153] ICH4: not 100% native mode: will probe irqs later
> > > > > > > > [ 4.298213] ide0: BM-DMA at 0xfc00-0xfc07
> > > > > > > > [ 4.298282] ide1: BM-DMA at 0xfc08-0xfc0f
> > > > > > > > [ 4.561768] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
> > > > > > > > [ 4.816724] hdb: SAMSUNG SP2014N, ATA DISK drive
> > > > > > > > [ 4.867959] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > > > > [ 4.868027] hda: UDMA/33 mode selected
> > > > > > > > [ 4.868441] hdb: UDMA/100 mode selected
> > > > > > > > [ 5.540683] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
> > > > > > > > [ 5.795564] hdd: IC35L120AVV207-0, ATA DISK drive
> > > > > > > > [ 5.847295] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
> > > > > > > > [ 5.847362] hdd: UDMA/33 mode selected
> > > > > > > > [ 5.847715] ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> > > > > > > > [ 5.855487] ide1 at 0x170-0x177,0x376 on irq 15
> > > > > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
> > > > > > > > [ 11.342504] hde: no response (status = 0xa1), resetting drive
> > > > > > > > [ 17.206535] hdf: no response (status = 0xa1), resetting drive
> > > > > > >
> > > > > > > hde? hdf?
> > > > > > >
> > > > > > > [...]
> > > > > >
> > > > > > yep, looks strange to me too. Isn't that the MAX_HWIFS upper bound of a loop.. ?
> > > > >
> > > > > Close, it is related to MAX_HWIFS being upper bound on hws[]
> > > > > in ide_generic.c but now we loop for ARRAY_SIZE(legacy_bases).
> > > > >
> > > > > IOW there is a memset(hws, 0, MAX_HWIFS) missing at the top of
> > > > > ide_generic_init() (please re-test after adding it).
> > > > >
> > > > > This may also explain the later problems with ide_host_register().
> > >
> > > I fixed this in pata tree now with:
> > >
> > > "ide-generic: remove ide_default_{io_base,irq}() inlines (take 2)" patch
> >
> > here's the root cause for the problem: I had both Intel ICH chipset
> > (BLK_DEV_PIIX) und generic ide (BLK_DEV_GENERIC) selected in Kconfig and since
> > ICH4 uses the generic driver detection routine, the second(!) generic detection after the
> > ICH4 one failed and died. This is why request_region()-resources are shown as
> > not being free above:
> >
> > > > > > > > [ 5.875927] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > > > > > > > [ 5.876012] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > > > > > > > [ 5.876074] ide_generic: I/O resource 0x170-0x177 not free.
>
> I also use BLK_DEV_PIIX + BLK_DEV_GENERIC configuration
> (for testing purposes) and it works fine here.
Well, I added some more debug printk's to see how the hosts get initialized and here's
what it looks like here:
[ 4.297031] netconsole: network logging started
[ 4.297086] Uniform Multi-Platform E-IDE driver
[ 4.297457] ICH4: IDE controller (0x8086:0x24cb rev 0x02) at PCI slot 0000:00:1f.1
[ 4.297547] pci 0000:00:1f.1: PCI INT A -> GSI 18 (level, low) -> IRQ 18
[ 4.297658] ICH4: not 100% native mode: will probe irqs later
[ 4.297714] ide_host_register: loop0: i=0, hwif=dfa19000
[ 4.297774] ide0: BM-DMA at 0xfc00-0xfc07
[ 4.297835] ide_host_register: loop0: i=1, hwif=dfa19800
[ 4.298041] ide1: BM-DMA at 0xfc08-0xfc0f
[ 4.298102] ide_host_register: loop0: i=2, hwif=00000000
[ 4.298157] ide_host_register: loop0: i=3, hwif=00000000
[ 4.298211] ide_host_register: loop0: i=4, hwif=00000000
[ 4.298267] ide_host_register: loop0: i=5, hwif=00000000
[ 4.298321] ide_host_register: loop0: i=6, hwif=00000000
[ 4.298375] ide_host_register: loop0: i=7, hwif=00000000
[ 4.298429] ide_host_register: loop0: i=8, hwif=00000000
[ 4.298483] ide_host_register: loop0: i=9, hwif=00000000
[ 4.298538] ide_host_register: loop1: i=0, hwif=dfa19000
[ 4.562086] hda: QUANTUM FIREBALLlct10 20, ATA DISK drive
[ 4.817043] hdb: SAMSUNG SP2014N, ATA DISK drive
[ 4.867892] ide_host_register: hwif=dfa19000
[ 4.868350] hda: drive side 80-wire cable detection failed, limiting max speed to UDMA33
[ 4.868419] hda: UDMA/33 mode selected
[ 4.868827] hdb: UDMA/100 mode selected
[ 4.868974] ide_host_register: loop1: i=1, hwif=dfa19800
[ 5.541001] hdc: IOMEGA ZIP 100 ATAPI, ATAPI FLOPPY drive
[ 5.795879] hdd: IC35L120AVV207-0, ATA DISK drive
[ 5.846729] ide_host_register: hwif=dfa19800
[ 5.847673] hdd: host side 80-wire cable detection failed, limiting max speed to UDMA33
[ 5.847756] hdd: UDMA/33 mode selected
[ 5.848002] ide_host_register: loop1: i=2, hwif=00000000
[ 5.848056] ide_host_register: loop1: i=3, hwif=00000000
[ 5.848111] ide_host_register: loop1: i=4, hwif=00000000
[ 5.848165] ide_host_register: loop1: i=5, hwif=00000000
[ 5.848219] ide_host_register: loop1: i=6, hwif=00000000
[ 5.848925] ide_host_register: loop1: i=7, hwif=00000000
[ 5.848981] ide_host_register: loop1: i=8, hwif=00000000
...
and this is where ide_host_register() goes through all the loops in there doing
all inits until it finishes. Then it is called again from ide_generic_init() and
here's what happens:
[ 5.879917] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
[ 5.879997] probe_mask=0x3, i=0x0, io_addr=0x1f0
[ 5.880057] ide_generic: I/O resource 0x1F0-0x1F7 not free.
[ 5.880115] probe_mask=0x3, i=0x1, io_addr=0x170
[ 5.880173] ide_generic: I/O resource 0x170-0x177 not free.
[ 5.880246] ide_host_register: loop0: i=0, hwif=00000000
[ 5.880299] ide_host_register: loop0: i=1, hwif=00000000
[ 5.880357] ide_host_register: loop0: i=2, hwif=00000000
and then it goes KABOOM!
I tested both with BLK_DEV_GENERIC on and off and the
error happens only when it is on:
--- config.ok 2008-07-22 06:58:48.000000000 +0200
+++ config.b0rked 2008-07-22 06:59:31.000000000 +0200
@@ -1,7 +1,7 @@
#
# Automatically generated make config: don't edit
# Linux kernel version: 2.6.26
-# Tue Jul 22 06:58:28 2008
+# Tue Jul 22 06:59:24 2008
#
# CONFIG_64BIT is not set
CONFIG_X86_32=y
@@ -600,7 +600,7 @@ CONFIG_IDE_PROC_FS=y
#
# IDE chipset support/bugfixes
#
-# CONFIG_IDE_GENERIC is not set
+CONFIG_IDE_GENERIC=y
# CONFIG_BLK_DEV_PLATFORM is not set
# CONFIG_BLK_DEV_CMD640 is not set
# CONFIG_BLK_DEV_IDEPNP is not set
I've also attached the .config that breaks the machine. Please take a look in
case i'm missing something.
> [ Besides it shouldn't result in phantom hde & hdf devices
> and ide_generic blowing up on failure. ]
>
> Have you tried the memset() fix that I proposed
> (pata tree contains the revised patch now)?
yep, test runs ontop of your tree from Sunday which already has the fix.
>
> > One of the possible fixes is adding
> >
> > depends on !BLK_DEV_GENERIC
> >
> > after each IDE chipset driver using the generic detection in drivers/ide/Kconfig
> > but it's a not-that-elegant one. Another thing would be using a dummy one like
> > BLK_DEV_IDEDMA_PCI, but I'm not that sure. Will look into it. I'm pretty sure
> > you have a better idea...
>
> pata_legacy.c has a proper fix which needs porting into ide-generic.c
> (it should be pretty easy thing to do).
(is this a hint^^? :))
> [ The fix is to skip automatic-probing of primary port and/or secondary
> one if a PCI controller using legacy I/O bases is detected:
>
> for_each_pci_dev(p) {
> int r;
> /* Check for any overlap of the system ATA mappings. Native
> mode controllers stuck on these addresses or some devices
> in 'raid' mode won't be found by the storage class test */
> for (r = 0; r < 6; r++) {
> if (pci_resource_start(p, r) == 0x1f0)
> primary = 1;
> if (pci_resource_start(p, r) == 0x170)
> secondary = 1;
> }
> /* Check for special cases */
> legacy_check_special_cases(p, &primary, &secondary); ]
>
> Thanks,
> Bart
--
Regards/Gru?,
Boris.
Hi,
On Tuesday 22 July 2008, Borislav Petkov wrote:
[...]
> [ 5.879917] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> [ 5.879997] probe_mask=0x3, i=0x0, io_addr=0x1f0
> [ 5.880057] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> [ 5.880115] probe_mask=0x3, i=0x1, io_addr=0x170
> [ 5.880173] ide_generic: I/O resource 0x170-0x177 not free.
> [ 5.880246] ide_host_register: loop0: i=0, hwif=00000000
> [ 5.880299] ide_host_register: loop0: i=1, hwif=00000000
> [ 5.880357] ide_host_register: loop0: i=2, hwif=00000000
>
> and then it goes KABOOM!
>
>
>
> I tested both with BLK_DEV_GENERIC on and off and the
> error happens only when it is on:
>
> --- config.ok 2008-07-22 06:58:48.000000000 +0200
> +++ config.b0rked 2008-07-22 06:59:31.000000000 +0200
> @@ -1,7 +1,7 @@
> #
> # Automatically generated make config: don't edit
> # Linux kernel version: 2.6.26
> -# Tue Jul 22 06:58:28 2008
> +# Tue Jul 22 06:59:24 2008
> #
> # CONFIG_64BIT is not set
> CONFIG_X86_32=y
> @@ -600,7 +600,7 @@ CONFIG_IDE_PROC_FS=y
> #
> # IDE chipset support/bugfixes
> #
> -# CONFIG_IDE_GENERIC is not set
> +CONFIG_IDE_GENERIC=y
> # CONFIG_BLK_DEV_PLATFORM is not set
> # CONFIG_BLK_DEV_CMD640 is not set
> # CONFIG_BLK_DEV_IDEPNP is not set
>
> I've also attached the .config that breaks the machine. Please take a look in
> case i'm missing something.
Thanks, with this config I can reproduce the problem.
> > [ Besides it shouldn't result in phantom hde & hdf devices
> > and ide_generic blowing up on failure. ]
> >
> > Have you tried the memset() fix that I proposed
> > (pata tree contains the revised patch now)?
>
> yep, test runs ontop of your tree from Sunday which already has the fix.
*sigh*
The previous fix was garbage and contained brown-paper-bag bug:
diff -u b/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
--- b/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -114,7 +114,7 @@
printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
"parameter for probing all legacy ISA IDE ports\n");
- memset(hws, 0, MAX_HWIFS);
+ memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
io_addr = legacy_bases[i];
Now it should be finally fixed.
> > > One of the possible fixes is adding
> > >
> > > depends on !BLK_DEV_GENERIC
> > >
> > > after each IDE chipset driver using the generic detection in drivers/ide/Kconfig
> > > but it's a not-that-elegant one. Another thing would be using a dummy one like
> > > BLK_DEV_IDEDMA_PCI, but I'm not that sure. Will look into it. I'm pretty sure
> > > you have a better idea...
> >
> > pata_legacy.c has a proper fix which needs porting into ide-generic.c
> > (it should be pretty easy thing to do).
>
> (is this a hint^^? :))
It has *HINT* written all over it. ;)
Thanks,
Bart
On Tue, Jul 22, 2008 at 09:49:19PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> Hi,
>
> On Tuesday 22 July 2008, Borislav Petkov wrote:
>
> [...]
>
> > [ 5.879917] ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports
> > [ 5.879997] probe_mask=0x3, i=0x0, io_addr=0x1f0
> > [ 5.880057] ide_generic: I/O resource 0x1F0-0x1F7 not free.
> > [ 5.880115] probe_mask=0x3, i=0x1, io_addr=0x170
> > [ 5.880173] ide_generic: I/O resource 0x170-0x177 not free.
> > [ 5.880246] ide_host_register: loop0: i=0, hwif=00000000
> > [ 5.880299] ide_host_register: loop0: i=1, hwif=00000000
> > [ 5.880357] ide_host_register: loop0: i=2, hwif=00000000
> >
> > and then it goes KABOOM!
> >
> >
> >
> > I tested both with BLK_DEV_GENERIC on and off and the
> > error happens only when it is on:
> >
> > --- config.ok 2008-07-22 06:58:48.000000000 +0200
> > +++ config.b0rked 2008-07-22 06:59:31.000000000 +0200
> > @@ -1,7 +1,7 @@
> > #
> > # Automatically generated make config: don't edit
> > # Linux kernel version: 2.6.26
> > -# Tue Jul 22 06:58:28 2008
> > +# Tue Jul 22 06:59:24 2008
> > #
> > # CONFIG_64BIT is not set
> > CONFIG_X86_32=y
> > @@ -600,7 +600,7 @@ CONFIG_IDE_PROC_FS=y
> > #
> > # IDE chipset support/bugfixes
> > #
> > -# CONFIG_IDE_GENERIC is not set
> > +CONFIG_IDE_GENERIC=y
> > # CONFIG_BLK_DEV_PLATFORM is not set
> > # CONFIG_BLK_DEV_CMD640 is not set
> > # CONFIG_BLK_DEV_IDEPNP is not set
> >
> > I've also attached the .config that breaks the machine. Please take a look in
> > case i'm missing something.
>
> Thanks, with this config I can reproduce the problem.
>
> > > [ Besides it shouldn't result in phantom hde & hdf devices
> > > and ide_generic blowing up on failure. ]
> > >
> > > Have you tried the memset() fix that I proposed
> > > (pata tree contains the revised patch now)?
> >
> > yep, test runs ontop of your tree from Sunday which already has the fix.
>
> *sigh*
>
> The previous fix was garbage and contained brown-paper-bag bug:
>
> diff -u b/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> --- b/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
> @@ -114,7 +114,7 @@
> printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
> "parameter for probing all legacy ISA IDE ports\n");
>
> - memset(hws, 0, MAX_HWIFS);
> + memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
>
> for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
> io_addr = legacy_bases[i];
>
>
> Now it should be finally fixed.
True story. Works here too.
>
> > > > One of the possible fixes is adding
> > > >
> > > > depends on !BLK_DEV_GENERIC
> > > >
> > > > after each IDE chipset driver using the generic detection in drivers/ide/Kconfig
> > > > but it's a not-that-elegant one. Another thing would be using a dummy one like
> > > > BLK_DEV_IDEDMA_PCI, but I'm not that sure. Will look into it. I'm pretty sure
> > > > you have a better idea...
> > >
> > > pata_legacy.c has a proper fix which needs porting into ide-generic.c
> > > (it should be pretty easy thing to do).
> >
> > (is this a hint^^? :))
>
> It has *HINT* written all over it. ;)
Hm, let's see whether there's time during the weekend. I already have something
stolen from pata_legacy but I'll do some more testing first. By the way, what
are the chances of exporting those pieces of code from drivers/ata/pata_legacy.c
and adding the function def into some header instead of duplicating the code into
ide_generic.c?
--
Regards/Gru?,
Boris.
On Wednesday 23 July 2008, Borislav Petkov wrote:
[...]
> > Now it should be finally fixed.
>
> True story. Works here too.
Thanks for verifying it.
> Hm, let's see whether there's time during the weekend. I already have something
> stolen from pata_legacy but I'll do some more testing first. By the way, what
> are the chances of exporting those pieces of code from drivers/ata/pata_legacy.c
> and adding the function def into some header instead of duplicating the code into
> ide_generic.c?
Good idea (<linux/ata.h> sounds like a perfect spot).
Thanks,
Bart
[ removed [email protected] from the CC-list ]
On Wed, Jul 23, 2008 at 08:51:11PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday 23 July 2008, Borislav Petkov wrote:
>
> [...]
>
> > > Now it should be finally fixed.
> >
> > True story. Works here too.
>
> Thanks for verifying it.
>
> > Hm, let's see whether there's time during the weekend. I already have something
> > stolen from pata_legacy but I'll do some more testing first. By the way, what
> > are the chances of exporting those pieces of code from drivers/ata/pata_legacy.c
> > and adding the function def into some header instead of duplicating the code into
> > ide_generic.c?
>
> Good idea (<linux/ata.h> sounds like a perfect spot).
Hi Bart,
i finally found some time to work on the iobase-exclusion. Actually, i dropped
the original idea of reusing pata_legacy code without duplicating it since this
got the whole SATA pulled in in Kconfig, which, imo, outweighs the savings from
not duplicating one function. I ended up refitting the pata_legacy iobase checks
into ide-generic.
As a result, i have now a new bool-Kconfig option BLK_DEV_GENERIC_ONLY which
gets reverse-selected only when no pci ide controller which is using the generic
ide_host_register() from within ide_pci_init_one() is selected in Kconfig. This
is tested both with and without a pci ide driver selected in addition to
ide-generic.
---
From: Borislav Petkov <[email protected]>
Date: Fri, 1 Aug 2008 07:33:13 +0200
Subject: [PATCH] ide-generic: skip automatic probing of legacy iobases
A number of pci ide controllers use legacy IO bases for their primary
and secondary ports. Skip probing those when both a specific host
driver _and_ ide-generic are enabled. The checking code originates from
drivers/ata/pata_legacy.c and is only reorganized into ide-generic.
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/Kconfig | 4 +++
drivers/ide/ide-generic.c | 49 +++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 51 insertions(+), 2 deletions(-)
diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
index 611319b..f103f5f 100644
--- a/drivers/ide/Kconfig
+++ b/drivers/ide/Kconfig
@@ -386,10 +386,14 @@ config BLK_DEV_OFFBOARD
config BLK_DEV_GENERIC
tristate "Generic PCI IDE Chipset Support"
select BLK_DEV_IDEPCI
+ select BLK_DEV_GENERIC_ONLY if !(BLK_DEV_AEC62XX || BLK_DEV_ALI15X3 || BLK_DEV_AMD74XX || BLK_DEV_ATIIXP || BLK_DEV_CMD64X || BLK_DEV_CS5530 || BLK_DEV_CS5535 || BLK_DEV_HPT34X || BLK_DEV_HPT366 || BLK_DEV_IT821X || BLK_DEV_IT8213 || BLK_DEV_JMICRON || BLK_DEV_NS87415 || BLK_DEV_OPTI621 || BLK_DEV_PDC202XX_OLD || BLK_DEV_PDC202XX_NEW || BLK_DEV_PIIX || BLK_DEV_RZ1000 || BLK_DEV_SC1200 || BLK_DEV_SVWKS || BLK_DEV_SIIMAGE || BLK_DEV_SIS5513 || BLK_DEV_SL82C105 || BLK_DEV_SLC90E66 || BLK_DEV_TC86C001 || BLK_DEV_TRIFLEX || BLK_DEV_TRM290 || BLK_DEV_VIA82CXXX)
help
This option provides generic support for various PCI IDE Chipsets
which otherwise might not be supported.
+config BLK_DEV_GENERIC_ONLY
+ bool
+
config BLK_DEV_OPTI621
tristate "OPTi 82C621 chipset enhanced support (EXPERIMENTAL)"
depends on EXPERIMENTAL
diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
index 8fe8b5b..3ce78f9 100644
--- a/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -100,12 +100,55 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
#endif
+static void ide_generic_check_pci_uses_legacy_iobases(int *primary,
+ int *secondary)
+{
+
+#if !defined(CONFIG_BLK_DEV_GENERIC_ONLY)
+ struct pci_dev *p = NULL;
+ u16 val;
+
+ for_each_pci_dev(p) {
+ int r;
+
+ for (r = 0; r < 6; r++) {
+ if (pci_resource_start(p, r) == 0x1f0)
+ *primary = 1;
+ if (pci_resource_start(p, r) == 0x170)
+ *secondary = 1;
+ }
+
+ /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == 0x1078 && p->device == 0x0000)
+ *primary = *secondary = 1;
+
+ /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == 0x1078 && p->device == 0x0002)
+ *primary = *secondary = 1;
+
+ /* Intel MPIIX - PIO ATA on non PCI side of bridge */
+ if (p->vendor == 0x8086 && p->device == 0x1234) {
+
+ pci_read_config_word(p, 0x6C, &val);
+ if (val & 0x8000) {
+ /* ATA port enabled */
+ if (val & 0x4000)
+ *secondary = 1;
+ else
+ *primary = 1;
+ }
+ }
+ }
+#endif
+
+}
+
static int __init ide_generic_init(void)
{
hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
struct ide_host *host;
unsigned long io_addr;
- int i, rc;
+ int i, rc, primary = 0, secondary = 0;
#ifdef CONFIG_MIPS
if (!ide_probe_legacy())
@@ -116,7 +159,9 @@ static int __init ide_generic_init(void)
memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
- for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
+ ide_generic_check_pci_uses_legacy_iobases(&primary, &secondary);
+
+ for (i = primary + secondary; i < ARRAY_SIZE(legacy_bases); i++) {
io_addr = legacy_bases[i];
hws[i] = NULL;
--
1.5.5.4
--
Regards/Gruss,
Boris.
Hi,
On Friday 01 August 2008, Borislav Petkov wrote:
>
> [ removed [email protected] from the CC-list ]
>
> On Wed, Jul 23, 2008 at 08:51:11PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 23 July 2008, Borislav Petkov wrote:
> >
> > [...]
> >
> > > > Now it should be finally fixed.
> > >
> > > True story. Works here too.
> >
> > Thanks for verifying it.
> >
> > > Hm, let's see whether there's time during the weekend. I already have something
> > > stolen from pata_legacy but I'll do some more testing first. By the way, what
> > > are the chances of exporting those pieces of code from drivers/ata/pata_legacy.c
> > > and adding the function def into some header instead of duplicating the code into
> > > ide_generic.c?
> >
> > Good idea (<linux/ata.h> sounds like a perfect spot).
>
> Hi Bart,
>
> i finally found some time to work on the iobase-exclusion. Actually, i dropped
> the original idea of reusing pata_legacy code without duplicating it since this
> got the whole SATA pulled in in Kconfig, which, imo, outweighs the savings from
> not duplicating one function. I ended up refitting the pata_legacy iobase checks
> into ide-generic.
Why not try <linux/ata.h> + inline trick instead?
[ <linux/ata.h> is shared by both stacks so by moving the function there
+ making it inline it can also be shared without the need for dependency
on libata. ]
> As a result, i have now a new bool-Kconfig option BLK_DEV_GENERIC_ONLY which
> gets reverse-selected only when no pci ide controller which is using the generic
> ide_host_register() from within ide_pci_init_one() is selected in Kconfig. This
> is tested both with and without a pci ide driver selected in addition to
> ide-generic.
How's about just leaving the final decision up to the user with changing
probe_mask in ide_generic from 0x3 to 0x0 and automatically probing for
ports 0-1 iff there is no IDE PCI controller present (otherwise check
probe_mask).
This is should remove the need for Kconfig magic and is a sane default
since a lot of people get caught using ide_generic by mistake and not by
intent (IOW they forgot to enable the right IDE PCI host driver).
[ The small minority which may use it by intent (I don't see any practical
reasons for doing it though) would still be able to override the default
with ide_generic.probe_mask=0x3 kernel parameter. ]
> ---
> From: Borislav Petkov <[email protected]>
> Date: Fri, 1 Aug 2008 07:33:13 +0200
> Subject: [PATCH] ide-generic: skip automatic probing of legacy iobases
>
> A number of pci ide controllers use legacy IO bases for their primary
> and secondary ports. Skip probing those when both a specific host
> driver _and_ ide-generic are enabled. The checking code originates from
> drivers/ata/pata_legacy.c and is only reorganized into ide-generic.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> drivers/ide/Kconfig | 4 +++
> drivers/ide/ide-generic.c | 49 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> index 611319b..f103f5f 100644
> --- a/drivers/ide/Kconfig
> +++ b/drivers/ide/Kconfig
> @@ -386,10 +386,14 @@ config BLK_DEV_OFFBOARD
> config BLK_DEV_GENERIC
> tristate "Generic PCI IDE Chipset Support"
> select BLK_DEV_IDEPCI
> + select BLK_DEV_GENERIC_ONLY if !(BLK_DEV_AEC62XX || BLK_DEV_ALI15X3 || BLK_DEV_AMD74XX || BLK_DEV_ATIIXP || BLK_DEV_CMD64X || BLK_DEV_CS5530 || BLK_DEV_CS5535 || BLK_DEV_HPT34X || BLK_DEV_HPT366 || BLK_DEV_IT821X || BLK_DEV_IT8213 || BLK_DEV_JMICRON || BLK_DEV_NS87415 || BLK_DEV_OPTI621 || BLK_DEV_PDC202XX_OLD || BLK_DEV_PDC202XX_NEW || BLK_DEV_PIIX || BLK_DEV_RZ1000 || BLK_DEV_SC1200 || BLK_DEV_SVWKS || BLK_DEV_SIIMAGE || BLK_DEV_SIS5513 || BLK_DEV_SL82C105 || BLK_DEV_SLC90E66 || BLK_DEV_TC86C001 || BLK_DEV_TRIFLEX || BLK_DEV_TRM290 || BLK_DEV_VIA82CXXX)
> help
> This option provides generic support for various PCI IDE Chipsets
> which otherwise might not be supported.
>
> +config BLK_DEV_GENERIC_ONLY
> + bool
> +
> config BLK_DEV_OPTI621
> tristate "OPTi 82C621 chipset enhanced support (EXPERIMENTAL)"
> depends on EXPERIMENTAL
> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> index 8fe8b5b..3ce78f9 100644
> --- a/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
> @@ -100,12 +100,55 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
> #endif
>
> +static void ide_generic_check_pci_uses_legacy_iobases(int *primary,
> + int *secondary)
> +{
> +
> +#if !defined(CONFIG_BLK_DEV_GENERIC_ONLY)
> + struct pci_dev *p = NULL;
> + u16 val;
> +
> + for_each_pci_dev(p) {
> + int r;
> +
> + for (r = 0; r < 6; r++) {
> + if (pci_resource_start(p, r) == 0x1f0)
> + *primary = 1;
> + if (pci_resource_start(p, r) == 0x170)
> + *secondary = 1;
> + }
> +
> + /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
> + if (p->vendor == 0x1078 && p->device == 0x0000)
> + *primary = *secondary = 1;
> +
> + /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
> + if (p->vendor == 0x1078 && p->device == 0x0002)
> + *primary = *secondary = 1;
> +
> + /* Intel MPIIX - PIO ATA on non PCI side of bridge */
> + if (p->vendor == 0x8086 && p->device == 0x1234) {
> +
> + pci_read_config_word(p, 0x6C, &val);
> + if (val & 0x8000) {
> + /* ATA port enabled */
> + if (val & 0x4000)
> + *secondary = 1;
> + else
> + *primary = 1;
> + }
> + }
> + }
> +#endif
> +
> +}
> +
> static int __init ide_generic_init(void)
> {
> hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
> struct ide_host *host;
> unsigned long io_addr;
> - int i, rc;
> + int i, rc, primary = 0, secondary = 0;
>
> #ifdef CONFIG_MIPS
> if (!ide_probe_legacy())
> @@ -116,7 +159,9 @@ static int __init ide_generic_init(void)
>
> memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
>
> - for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
> + ide_generic_check_pci_uses_legacy_iobases(&primary, &secondary);
> +
> + for (i = primary + secondary; i < ARRAY_SIZE(legacy_bases); i++) {
> io_addr = legacy_bases[i];
>
> hws[i] = NULL;
On Sat, Aug 02, 2008 at 07:02:12PM +0200, Bartlomiej Zolnierkiewicz wrote:
[.. ]
> Why not try <linux/ata.h> + inline trick instead?
>
> [ <linux/ata.h> is shared by both stacks so by moving the function there
> + making it inline it can also be shared without the need for dependency
> on libata. ]
I hadn't thought of that, will try it out later.
> > As a result, i have now a new bool-Kconfig option BLK_DEV_GENERIC_ONLY which
> > gets reverse-selected only when no pci ide controller which is using the generic
> > ide_host_register() from within ide_pci_init_one() is selected in Kconfig. This
> > is tested both with and without a pci ide driver selected in addition to
> > ide-generic.
>
> How's about just leaving the final decision up to the user with changing
> probe_mask in ide_generic from 0x3 to 0x0 and automatically probing for
> ports 0-1 iff there is no IDE PCI controller present (otherwise check
> probe_mask).
Wait, let me get this straight: you want to set probe_mask to 0x0 as a default,
which skips probing of the primary and secondary ports, and to do the checking
whether the IDE PCI controller uses legacy iobases only when the user has
enforced it by setting probe_mask to 0x3? At least this is how i understand
it...
> This is should remove the need for Kconfig magic and is a sane default
> since a lot of people get caught using ide_generic by mistake and not by
> intent (IOW they forgot to enable the right IDE PCI host driver).
[.. ]
--
Regards/Gruss,
Boris.
On Saturday 02 August 2008, Borislav Petkov wrote:
> On Sat, Aug 02, 2008 at 07:02:12PM +0200, Bartlomiej Zolnierkiewicz wrote:
[...]
> > > As a result, i have now a new bool-Kconfig option BLK_DEV_GENERIC_ONLY which
> > > gets reverse-selected only when no pci ide controller which is using the generic
> > > ide_host_register() from within ide_pci_init_one() is selected in Kconfig. This
> > > is tested both with and without a pci ide driver selected in addition to
> > > ide-generic.
> >
> > How's about just leaving the final decision up to the user with changing
> > probe_mask in ide_generic from 0x3 to 0x0 and automatically probing for
> > ports 0-1 iff there is no IDE PCI controller present (otherwise check
> > probe_mask).
>
> Wait, let me get this straight: you want to set probe_mask to 0x0 as a default,
> which skips probing of the primary and secondary ports, and to do the checking
Yes.
> whether the IDE PCI controller uses legacy iobases only when the user has
> enforced it by setting probe_mask to 0x3? At least this is how i understand
> it...
Nope, always do the checking and if there is no IDE PCI controller do the
probing (& if there is IDE PCI controller present check probe_mask bits).
Ok, here's a definitely better solution:
---
From: Borislav Petkov <[email protected]>
Date: Sun, 3 Aug 2008 08:31:20 +0200
Subject: [PATCH 1/2] pata_legacy: export functionality to ide
export the legacy iobases checking code to other
users (ide) by pushing it up into the header.
CC: Alan Cox <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ata/pata_legacy.c | 63 +-----------------------------------------
include/linux/ata.h | 67 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+), 62 deletions(-)
diff --git a/drivers/ata/pata_legacy.c b/drivers/ata/pata_legacy.c
index bc037ff..14d187e 100644
--- a/drivers/ata/pata_legacy.c
+++ b/drivers/ata/pata_legacy.c
@@ -50,7 +50,6 @@
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/pci.h>
#include <linux/init.h>
#include <linux/blkdev.h>
#include <linux/delay.h>
@@ -1040,47 +1039,6 @@ fail:
return ret;
}
-/**
- * legacy_check_special_cases - ATA special cases
- * @p: PCI device to check
- * @master: set this if we find an ATA master
- * @master: set this if we find an ATA secondary
- *
- * A small number of vendors implemented early PCI ATA interfaces
- * on bridge logic without the ATA interface being PCI visible.
- * Where we have a matching PCI driver we must skip the relevant
- * device here. If we don't know about it then the legacy driver
- * is the right driver anyway.
- */
-
-static void __init legacy_check_special_cases(struct pci_dev *p, int *primary,
- int *secondary)
-{
- /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
- if (p->vendor == 0x1078 && p->device == 0x0000) {
- *primary = *secondary = 1;
- return;
- }
- /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
- if (p->vendor == 0x1078 && p->device == 0x0002) {
- *primary = *secondary = 1;
- return;
- }
- /* Intel MPIIX - PIO ATA on non PCI side of bridge */
- if (p->vendor == 0x8086 && p->device == 0x1234) {
- u16 r;
- pci_read_config_word(p, 0x6C, &r);
- if (r & 0x8000) {
- /* ATA port enabled */
- if (r & 0x4000)
- *secondary = 1;
- else
- *primary = 1;
- }
- return;
- }
-}
-
static __init void probe_opti_vlb(void)
{
/* If an OPTI 82C46X is present find out where the channels are */
@@ -1210,26 +1168,7 @@ static __init int legacy_init(void)
struct legacy_probe *pl = &probe_list[0];
int slot = 0;
- struct pci_dev *p = NULL;
-
- for_each_pci_dev(p) {
- int r;
- /* Check for any overlap of the system ATA mappings. Native
- mode controllers stuck on these addresses or some devices
- in 'raid' mode won't be found by the storage class test */
- for (r = 0; r < 6; r++) {
- if (pci_resource_start(p, r) == 0x1f0)
- primary = 1;
- if (pci_resource_start(p, r) == 0x170)
- secondary = 1;
- }
- /* Check for special cases */
- legacy_check_special_cases(p, &primary, &secondary);
-
- /* If PCI bus is present then don't probe for tertiary
- legacy ports */
- pci_present = 1;
- }
+ ata_legacy_check_iobases(&primary, &secondary, &pci_present);
if (winbond == 1)
winbond = 0x130; /* Default port, alt is 1B0 */
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 11de32c..0470562 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -30,6 +30,7 @@
#define __LINUX_ATA_H__
#include <linux/types.h>
+#include <linux/pci.h>
/* defines only for the constants which don't work well as enums */
#define ATA_DMA_BOUNDARY 0xffffUL
@@ -776,4 +777,70 @@ static inline int lba_48_ok(u64 block, u32 n_block)
#define sata_pmp_gscr_rev(gscr) (((gscr)[SATA_PMP_GSCR_REV] >> 8) & 0xff)
#define sata_pmp_gscr_ports(gscr) ((gscr)[SATA_PMP_GSCR_PORT_INFO] & 0xf)
+/**
+ * legacy_check_special_cases - ATA special cases
+ * @p: PCI device to check
+ * @master: set this if we find an ATA master
+ * @master: set this if we find an ATA secondary
+ *
+ * A small number of vendors implemented early PCI ATA interfaces
+ * on bridge logic without the ATA interface being PCI visible.
+ * Where we have a matching PCI driver we must skip the relevant
+ * device here. If we don't know about it then the legacy driver
+ * is the right driver anyway.
+ */
+static inline void __init ata_legacy_check_special_cases(struct pci_dev *p,
+ int *primary,
+ int *secondary)
+{
+ /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == 0x1078 && p->device == 0x0000) {
+ *primary = *secondary = 1;
+ return;
+ }
+ /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == 0x1078 && p->device == 0x0002) {
+ *primary = *secondary = 1;
+ return;
+ }
+ /* Intel MPIIX - PIO ATA on non PCI side of bridge */
+ if (p->vendor == 0x8086 && p->device == 0x1234) {
+ u16 r;
+ pci_read_config_word(p, 0x6C, &r);
+ if (r & 0x8000) {
+ /* ATA port enabled */
+ if (r & 0x4000)
+ *secondary = 1;
+ else
+ *primary = 1;
+ }
+ return;
+ }
+}
+
+static inline void __init ata_legacy_check_iobases(int *primary, int *secondary,
+ int *pci_present)
+{
+ struct pci_dev *p = NULL;
+
+ for_each_pci_dev(p) {
+ int r;
+ /* Check for any overlap of the system ATA mappings. Native
+ mode controllers stuck on these addresses or some devices
+ in 'raid' mode won't be found by the storage class test */
+ for (r = 0; r < 6; r++) {
+ if (pci_resource_start(p, r) == 0x1f0)
+ *primary = 1;
+ if (pci_resource_start(p, r) == 0x170)
+ *secondary = 1;
+ }
+ /* Check for special cases */
+ ata_legacy_check_special_cases(p, primary, secondary);
+
+ /* If PCI bus is present then don't probe for tertiary
+ legacy ports */
+ *pci_present = 1;
+ }
+}
+
#endif /* __LINUX_ATA_H__ */
--
1.5.5.4
--
Regards/Gruss,
Boris.
From: Borislav Petkov <[email protected]>
Date: Sun, 3 Aug 2008 09:28:53 +0200
Subject: [PATCH 2/2] ide-generic: handle probing of legacy io-ports
Avoid probing the io-ports in case an IDE PCI controller is present and it uses
the legacy iobases. If we still want to enforce the probing, we do
ide_generic.probe_mask=0x3f
on the kernel command line.
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-generic.c | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
index 8fe8b5b..7d79616 100644
--- a/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -27,7 +27,7 @@
#define DRV_NAME "ide_generic"
-static int probe_mask = 0x03;
+static int probe_mask = 0x00;
module_param(probe_mask, int, 0);
MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
@@ -105,18 +105,31 @@ static int __init ide_generic_init(void)
hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
struct ide_host *host;
unsigned long io_addr;
- int i, rc;
+ int i, rc, dummy, primary = 0, secondary = 0;
#ifdef CONFIG_MIPS
if (!ide_probe_legacy())
return -ENODEV;
#endif
- printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
- "parameter for probing all legacy ISA IDE ports\n");
+ ata_legacy_check_iobases(&primary, &secondary, &dummy);
+
+ if (primary) {
+ if (probe_mask) {
+ printk(KERN_WARNING "%s: enforcing probing of io ports "
+ "upon user request.\n", DRV_NAME);
+ primary = 0;
+ secondary = 0;
+ } else
+ printk(KERN_INFO DRV_NAME ": please use "
+ \"probe_mask=0x3f\" module parameter for probing"
+ "all legacy ISA IDE ports\n");
+
+ } else
+ probe_mask = 0x3;
memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
- for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
+ for (i = primary + secondary; i < ARRAY_SIZE(legacy_bases); i++) {
io_addr = legacy_bases[i];
hws[i] = NULL;
--
1.5.5.4
--
Regards/Gruss,
Boris.
On Sun, 3 Aug 2008 09:37:56 +0200
Borislav Petkov <[email protected]> wrote:
> Ok, here's a definitely better solution:
Please don't stuff large important pieces of code in header files where
they will be overlooked
NAK this.
I'm happy to have a shared library directory for ATA stuff, containing
useful C code, but hiding stuff in headers like that is just plain wrong.
On Sunday 03 August 2008, Alan Cox wrote:
> On Sun, 3 Aug 2008 09:37:56 +0200
> Borislav Petkov <[email protected]> wrote:
>
> > Ok, here's a definitely better solution:
>
> Please don't stuff large important pieces of code in header files where
> they will be overlooked
>
> NAK this.
>
> I'm happy to have a shared library directory for ATA stuff, containing
> useful C code, but hiding stuff in headers like that is just plain wrong.
The code in question is 65 LOC total (43 LOC without counting comments)
so having a shared library just for it sounds like an overkill and we may
just copy that one function from pata_legacy to ide_generic instead.
Jeff, what is your stance here?
> The code in question is 65 LOC total (43 LOC without counting comments)
> so having a shared library just for it sounds like an overkill and we may
People expect code in C files, so in headers it gets missed as well as
dumped in a directory with no correlation between file name and subsystem.
> just copy that one function from pata_legacy to ide_generic instead.
If you are going to #include two copies you might as well just copy it.
Alan
On Sunday 03 August 2008, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Date: Sun, 3 Aug 2008 09:28:53 +0200
> Subject: [PATCH 2/2] ide-generic: handle probing of legacy io-ports
>
> Avoid probing the io-ports in case an IDE PCI controller is present and it uses
> the legacy iobases. If we still want to enforce the probing, we do
>
> ide_generic.probe_mask=0x3f
>
> on the kernel command line.
>
> Signed-off-by: Borislav Petkov <[email protected]>
Thanks for reworking the patch, looks much better now.
There are still some issues to address though.
> ---
> drivers/ide/ide-generic.c | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> index 8fe8b5b..7d79616 100644
> --- a/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
> @@ -27,7 +27,7 @@
>
> #define DRV_NAME "ide_generic"
>
> -static int probe_mask = 0x03;
> +static int probe_mask = 0x00;
No need to initialize it now.
> module_param(probe_mask, int, 0);
> MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
>
> @@ -105,18 +105,31 @@ static int __init ide_generic_init(void)
> hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
> struct ide_host *host;
> unsigned long io_addr;
> - int i, rc;
> + int i, rc, dummy, primary = 0, secondary = 0;
>
> #ifdef CONFIG_MIPS
> if (!ide_probe_legacy())
> return -ENODEV;
> #endif
> - printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
> - "parameter for probing all legacy ISA IDE ports\n");
> + ata_legacy_check_iobases(&primary, &secondary, &dummy);
> +
> + if (primary) {
Shouldn't this also check for secondary?
> + if (probe_mask) {
> + printk(KERN_WARNING "%s: enforcing probing of io ports "
> + "upon user request.\n", DRV_NAME);
> + primary = 0;
> + secondary = 0;
> + } else
> + printk(KERN_INFO DRV_NAME ": please use "
> + \"probe_mask=0x3f\" module parameter for probing"
> + "all legacy ISA IDE ports\n");
Help message is no longer printed for !primary
(we always want to have it unless probe_mask is set).
> +
> + } else
> + probe_mask = 0x3;
I think this was meant to be 'probe_mask |= 3;'?
> memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
>
> - for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
> + for (i = primary + secondary; i < ARRAY_SIZE(legacy_bases); i++) {
No need for primary/secondary checking here now as everything
is controlled by probe_mask.
Thus we can check for probe_mask first in the previous chunk
and it can be rewritten/simplified to something like:
ata_legacy_check_iobases(&primary, &secondary, &dummy);
if (probe_mask == 0) {
printk(KERN_INFO DRV_NAME ": please use "
\"probe_mask=0x3f\" module parameter for probing"
"all legacy ISA IDE ports\n");
if (primary == 0)
probe_mask |= 1;
if (secondary == 0)
probe_mask |= 2;
}
On Sun, Aug 03, 2008 at 12:59:07PM +0100, Alan Cox wrote:
> On Sun, 3 Aug 2008 09:37:56 +0200
> Borislav Petkov <[email protected]> wrote:
>
> > Ok, here's a definitely better solution:
>
> Please don't stuff large important pieces of code in header files where
> they will be overlooked
What do you mean by "overlooked"? If you're looking for the function defintion,
any sensible code indexing tool will point you to the right place.
And linux/ata.h already contains several c one liners/helpers. What is the
difference between the two new functions and the ones already present there?
Although the solution i propose is not adhering to some header/c file
conventions, it is still the best one considering the other possibilities:
a) code duplication: dumb idea, bloated kernel for no reason
b) evil Kconfig SELECT pulling in core libata just so that ide might be calling
a function or two.
[.. ]
--
Regards/Gruss,
Boris.
On Sun, Aug 03, 2008 at 04:11:10PM +0200, Bartlomiej Zolnierkiewicz wrote:
>
> On Sunday 03 August 2008, Borislav Petkov wrote:
> > From: Borislav Petkov <[email protected]>
> > Date: Sun, 3 Aug 2008 09:28:53 +0200
> > Subject: [PATCH 2/2] ide-generic: handle probing of legacy io-ports
> >
> > Avoid probing the io-ports in case an IDE PCI controller is present and it uses
> > the legacy iobases. If we still want to enforce the probing, we do
> >
> > ide_generic.probe_mask=0x3f
> >
> > on the kernel command line.
> >
> > Signed-off-by: Borislav Petkov <[email protected]>
>
> Thanks for reworking the patch, looks much better now.
>
> There are still some issues to address though.
>
> > ---
> > drivers/ide/ide-generic.c | 23 ++++++++++++++++++-----
> > 1 files changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> > index 8fe8b5b..7d79616 100644
> > --- a/drivers/ide/ide-generic.c
> > +++ b/drivers/ide/ide-generic.c
> > @@ -27,7 +27,7 @@
> >
> > #define DRV_NAME "ide_generic"
> >
> > -static int probe_mask = 0x03;
> > +static int probe_mask = 0x00;
>
> No need to initialize it now.
right, static.
> > module_param(probe_mask, int, 0);
> > MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
> >
> > @@ -105,18 +105,31 @@ static int __init ide_generic_init(void)
> > hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
> > struct ide_host *host;
> > unsigned long io_addr;
> > - int i, rc;
> > + int i, rc, dummy, primary = 0, secondary = 0;
> >
> > #ifdef CONFIG_MIPS
> > if (!ide_probe_legacy())
> > return -ENODEV;
> > #endif
> > - printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
> > - "parameter for probing all legacy ISA IDE ports\n");
> > + ata_legacy_check_iobases(&primary, &secondary, &dummy);
> > +
> > + if (primary) {
>
> Shouldn't this also check for secondary?
you don't have to since primary is set to one in all cases.
> > + if (probe_mask) {
> > + printk(KERN_WARNING "%s: enforcing probing of io ports "
> > + "upon user request.\n", DRV_NAME);
> > + primary = 0;
> > + secondary = 0;
> > + } else
> > + printk(KERN_INFO DRV_NAME ": please use "
> > + \"probe_mask=0x3f\" module parameter for probing"
> > + "all legacy ISA IDE ports\n");
>
> Help message is no longer printed for !primary
> (we always want to have it unless probe_mask is set).
How about moving that info to Documentation/ide/ide.txt instead? Do we want to
issue that on every boot, seems like a too unimportant message to be in the
bootlog to me...
> > +
> > + } else
> > + probe_mask = 0x3;
>
> I think this was meant to be 'probe_mask |= 3;'?
true.
> > memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
> >
> > - for (i = 0; i < ARRAY_SIZE(legacy_bases); i++) {
> > + for (i = primary + secondary; i < ARRAY_SIZE(legacy_bases); i++) {
>
> No need for primary/secondary checking here now as everything
> is controlled by probe_mask.
>
> Thus we can check for probe_mask first in the previous chunk
> and it can be rewritten/simplified to something like:
>
> ata_legacy_check_iobases(&primary, &secondary, &dummy);
>
> if (probe_mask == 0) {
> printk(KERN_INFO DRV_NAME ": please use "
> \"probe_mask=0x3f\" module parameter for probing"
> "all legacy ISA IDE ports\n");
>
> if (primary == 0)
> probe_mask |= 1;
>
> if (secondary == 0)
> probe_mask |= 2;
True, this version is more readable. Reworked one coming up :) ...
--
Regards/Gruss,
Boris.
On Sun, Aug 03, 2008 at 04:45:39PM +0200, Borislav Petkov wrote:
> On Sun, Aug 03, 2008 at 04:11:10PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >
> > On Sunday 03 August 2008, Borislav Petkov wrote:
> > > From: Borislav Petkov <[email protected]>
> > > Date: Sun, 3 Aug 2008 09:28:53 +0200
> > > Subject: [PATCH 2/2] ide-generic: handle probing of legacy io-ports
> > >
> > > Avoid probing the io-ports in case an IDE PCI controller is present and it uses
> > > the legacy iobases. If we still want to enforce the probing, we do
> > >
> > > ide_generic.probe_mask=0x3f
> > >
> > > on the kernel command line.
> > >
> > > Signed-off-by: Borislav Petkov <[email protected]>
> >
> > Thanks for reworking the patch, looks much better now.
> >
> > There are still some issues to address though.
> >
> > > ---
> > > drivers/ide/ide-generic.c | 23 ++++++++++++++++++-----
> > > 1 files changed, 18 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> > > index 8fe8b5b..7d79616 100644
> > > --- a/drivers/ide/ide-generic.c
> > > +++ b/drivers/ide/ide-generic.c
> > > @@ -27,7 +27,7 @@
> > >
> > > #define DRV_NAME "ide_generic"
> > >
> > > -static int probe_mask = 0x03;
> > > +static int probe_mask = 0x00;
> >
> > No need to initialize it now.
>
> right, static.
>
> > > module_param(probe_mask, int, 0);
> > > MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
> > >
> > > @@ -105,18 +105,31 @@ static int __init ide_generic_init(void)
> > > hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
> > > struct ide_host *host;
> > > unsigned long io_addr;
> > > - int i, rc;
> > > + int i, rc, dummy, primary = 0, secondary = 0;
> > >
> > > #ifdef CONFIG_MIPS
> > > if (!ide_probe_legacy())
> > > return -ENODEV;
> > > #endif
> > > - printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
> > > - "parameter for probing all legacy ISA IDE ports\n");
> > > + ata_legacy_check_iobases(&primary, &secondary, &dummy);
> > > +
> > > + if (primary) {
> >
> > Shouldn't this also check for secondary?
>
> you don't have to since primary is set to one in all cases.
crap, forget what i said here ^ :(.
--
Regards/Gruss,
Boris.
On Sunday 03 August 2008, Borislav Petkov wrote:
[...]
> > > + if (probe_mask) {
> > > + printk(KERN_WARNING "%s: enforcing probing of io ports "
> > > + "upon user request.\n", DRV_NAME);
> > > + primary = 0;
> > > + secondary = 0;
> > > + } else
> > > + printk(KERN_INFO DRV_NAME ": please use "
> > > + \"probe_mask=0x3f\" module parameter for probing"
> > > + "all legacy ISA IDE ports\n");
> >
> > Help message is no longer printed for !primary
> > (we always want to have it unless probe_mask is set).
>
> How about moving that info to Documentation/ide/ide.txt instead? Do we want to
> issue that on every boot, seems like a too unimportant message to be in the
> bootlog to me...
Some time ago we've changed the default from probing all ports to probe
only safe ones (we had to do it because probing all ports may break other
ISA devices) so there may be very unlikely cases when this bugfix caused
regression and we would like people to be able to deduce the workaround
from just reading kernel messages.
On Sun, Aug 03, 2008 at 04:11:10PM +0200, Bartlomiej Zolnierkiewicz wrote:
[.. ]
> There are still some issues to address though.
Issues addressed. Here's v2:
---
From: Borislav Petkov <[email protected]>
Date: Sun, 3 Aug 2008 09:28:53 +0200
Subject: [PATCH] ide-generic: handle probing of legacy io-ports v2
Avoid probing the io-ports in case an IDE PCI controller is present and it uses
the legacy iobases. If we still want to enforce the probing, we do
ide_generic.probe_mask=0x3f
on the kernel command line.
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-generic.c | 21 +++++++++++++++++----
1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
index 8fe8b5b..1beb51b 100644
--- a/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -27,7 +27,7 @@
#define DRV_NAME "ide_generic"
-static int probe_mask = 0x03;
+static int probe_mask;
module_param(probe_mask, int, 0);
MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
@@ -105,14 +105,27 @@ static int __init ide_generic_init(void)
hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
struct ide_host *host;
unsigned long io_addr;
- int i, rc;
+ int i, rc, dummy, primary = 0, secondary = 0;
#ifdef CONFIG_MIPS
if (!ide_probe_legacy())
return -ENODEV;
#endif
- printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
- "parameter for probing all legacy ISA IDE ports\n");
+ ata_legacy_check_iobases(&primary, &secondary, &dummy);
+
+ if (!probe_mask) {
+ printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" "
+ "module parameter for probing all legacy ISA IDE ports\n");
+
+ if (primary == 0)
+ probe_mask |= 0x1;
+
+ if (secondary == 0)
+ probe_mask |= 0x2;
+ } else {
+ printk(KERN_WARNING "%s: enforcing probing of io ports upon "
+ "user request.\n", DRV_NAME);
+ }
memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
--
1.5.5.4
--
Regards/Gruss,
Boris.
> conventions, it is still the best one considering the other possibilities:
>
> a) code duplication: dumb idea, bloated kernel for no reason
I think you might want to start somewhere else if that worries you. Its
also a mostly bogus reasoning as almost nobody builds with both, in fact
you have to be pretty careful if you do that or it all falls over in a
heap.
For previous cases (eg ide timing) it has actually made more sense to
split the code. The moment you get future different behaviour between
libata and old IDE on any of these devices the sharing will just break
again (eg if one or the other drops chipset support for one of those
devices or adds one for another device)
> b) evil Kconfig SELECT pulling in core libata just so that ide might be calling
> a function or two.
That would be stunningly dumb. I happen to think we have developers whose
minds extent to adding libata-common.c and pulling in a single file if we
do that. Yes people have the past done stupid stuff like pulling all of
CONFIG_IDE in for a single USB device but that was because nobody noticed
and fixed it promptly not because it was a good idea.
Alan
[shortened up CC list]
On Sun, Aug 03, 2008 at 04:22:14PM +0100, Alan Cox wrote:
> > conventions, it is still the best one considering the other possibilities:
> >
> > a) code duplication: dumb idea, bloated kernel for no reason
>
> I think you might want to start somewhere else if that worries you. Its
> also a mostly bogus reasoning as almost nobody builds with both, in fact
> you have to be pretty careful if you do that or it all falls over in a
> heap.
>
> For previous cases (eg ide timing) it has actually made more sense to
> split the code. The moment you get future different behaviour between
> libata and old IDE on any of these devices the sharing will just break
> again (eg if one or the other drops chipset support for one of those
> devices or adds one for another device)
>
> > b) evil Kconfig SELECT pulling in core libata just so that ide might be calling
> > a function or two.
>
> That would be stunningly dumb. I happen to think we have developers whose
> minds extent to adding libata-common.c and pulling in a single file if we
> do that. Yes people have the past done stupid stuff like pulling all of
> CONFIG_IDE in for a single USB device but that was because nobody noticed
> and fixed it promptly not because it was a good idea.
>
> Alan
Ok then, so we duplicate. This seems like the easiest solution. Sharing code
between libata and IDE is not that smart in case the two development directions
divert, as you said. Bart, here's v3:
---
From: Borislav Petkov <[email protected]>
Date: Sun, 3 Aug 2008 18:46:35 +0200
Subject: [PATCH] ide-generic: handle probing of legacy io-ports v3
Avoid probing the io-ports in case an IDE PCI controller is present and it uses
the legacy iobases. If we still want to enforce the probing, we do
ide_generic.probe_mask=0x3f
on the kernel command line. The iobase checking code is adapted from
drivers/ata/pata_legacy.c
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-generic.c | 60 ++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
index 8fe8b5b..e9b7b69 100644
--- a/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -27,7 +27,7 @@
#define DRV_NAME "ide_generic"
-static int probe_mask = 0x03;
+static int probe_mask;
module_param(probe_mask, int, 0);
MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
@@ -100,19 +100,71 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
#endif
+
+static void ide_generic_check_pci_legacy_iobases(int *primary, int *secondary)
+{
+ struct pci_dev *p = NULL;
+ u16 val;
+
+ for_each_pci_dev(p) {
+ int r;
+
+ for (r = 0; r < 6; r++) {
+ if (pci_resource_start(p, r) == 0x1f0)
+ *primary = 1;
+ if (pci_resource_start(p, r) == 0x170)
+ *secondary = 1;
+ }
+
+ /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == 0x1078 && p->device == 0x0000)
+ *primary = *secondary = 1;
+
+ /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == 0x1078 && p->device == 0x0002)
+ *primary = *secondary = 1;
+
+ /* Intel MPIIX - PIO ATA on non PCI side of bridge */
+ if (p->vendor == 0x8086 && p->device == 0x1234) {
+
+ pci_read_config_word(p, 0x6C, &val);
+ if (val & 0x8000) {
+ /* ATA port enabled */
+ if (val & 0x4000)
+ *secondary = 1;
+ else
+ *primary = 1;
+ }
+ }
+ }
+}
+
static int __init ide_generic_init(void)
{
hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
struct ide_host *host;
unsigned long io_addr;
- int i, rc;
+ int i, rc, primary = 0, secondary = 0;
#ifdef CONFIG_MIPS
if (!ide_probe_legacy())
return -ENODEV;
#endif
- printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
- "parameter for probing all legacy ISA IDE ports\n");
+ ide_generic_check_pci_legacy_iobases(&primary, &secondary);
+
+ if (!probe_mask) {
+ printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" "
+ "module parameter for probing all legacy ISA IDE ports\n");
+
+ if (primary == 0)
+ probe_mask |= 0x1;
+
+ if (secondary == 0)
+ probe_mask |= 0x2;
+ } else {
+ printk(KERN_WARNING "%s: enforcing probing of io ports upon "
+ "user request.\n", DRV_NAME);
+ }
memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
--
1.5.5.4
--
Regards/Gruss,
Boris.
Alan Cox wrote:
>> The code in question is 65 LOC total (43 LOC without counting comments)
>> so having a shared library just for it sounds like an overkill and we may
>
> People expect code in C files, so in headers it gets missed as well as
> dumped in a directory with no correlation between file name and subsystem.
>
>> just copy that one function from pata_legacy to ide_generic instead.
>
> If you are going to #include two copies you might as well just copy it.
That's pretty much my feeling... just copy the code.
If the shared code grows larger, create a kernel module with the stuff
shared by both libata and drivers/ide.
liblibata? libata-core-core? :)
Jeff
Hello.
Borislav Petkov wrote:
> Avoid probing the io-ports in case an IDE PCI controller is present and it uses
> the legacy iobases. If we still want to enforce the probing, we do
> ide_generic.probe_mask=0x3f
> on the kernel command line. The iobase checking code is adapted from
> drivers/ata/pata_legacy.c
> Signed-off-by: Borislav Petkov <[email protected]>
> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> index 8fe8b5b..e9b7b69 100644
> --- a/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
[...]
> @@ -100,19 +100,71 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
> #endif
>
> +
Extra newline...
> +static void ide_generic_check_pci_legacy_iobases(int *primary, int *secondary)
> +{
> + struct pci_dev *p = NULL;
> + u16 val;
> +
> + for_each_pci_dev(p) {
> + int r;
> +
> + for (r = 0; r < 6; r++) {
> + if (pci_resource_start(p, r) == 0x1f0)
> + *primary = 1;
> + if (pci_resource_start(p, r) == 0x170)
> + *secondary = 1;
> + }
> +
> + /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
> + if (p->vendor == 0x1078 && p->device == 0x0000)
> + *primary = *secondary = 1;
> +
> + /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
> + if (p->vendor == 0x1078 && p->device == 0x0002)
> + *primary = *secondary = 1;
I think the above two if statements should be collapsed into a single one.
> +
> + /* Intel MPIIX - PIO ATA on non PCI side of bridge */
> + if (p->vendor == 0x8086 && p->device == 0x1234) {
Also, perhaps it makes sense to #include <linux/pci_ids.h> and use the
macros defined there...
MBR, Sergei
On Tue, Aug 5, 2008 at 4:26 PM, Sergei Shtylyov <[email protected]> wrote:
> Hello.
>
> Borislav Petkov wrote:
>
>> Avoid probing the io-ports in case an IDE PCI controller is present and it
>> uses
>> the legacy iobases. If we still want to enforce the probing, we do
>
>> ide_generic.probe_mask=0x3f
>
>> on the kernel command line. The iobase checking code is adapted from
>> drivers/ata/pata_legacy.c
>
>> Signed-off-by: Borislav Petkov <[email protected]>
>
>> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
>> index 8fe8b5b..e9b7b69 100644
>> --- a/drivers/ide/ide-generic.c
>> +++ b/drivers/ide/ide-generic.c
>
> [...]
>>
>> @@ -100,19 +100,71 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170,
>> 0x1e8, 0x168, 0x1e0, 0x160 };
>> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
>> #endif
>> +
>
> Extra newline...
>
>> +static void ide_generic_check_pci_legacy_iobases(int *primary, int
>> *secondary)
>> +{
>> + struct pci_dev *p = NULL;
>> + u16 val;
>> +
>> + for_each_pci_dev(p) {
>> + int r;
>> +
>> + for (r = 0; r < 6; r++) {
>> + if (pci_resource_start(p, r) == 0x1f0)
>> + *primary = 1;
>> + if (pci_resource_start(p, r) == 0x170)
>> + *secondary = 1;
>> + }
>> +
>> + /* Cyrix CS5510 pre SFF MWDMA ATA on the bridge */
>> + if (p->vendor == 0x1078 && p->device == 0x0000)
>> + *primary = *secondary = 1;
>> +
>> + /* Cyrix CS5520 pre SFF MWDMA ATA on the bridge */
>> + if (p->vendor == 0x1078 && p->device == 0x0002)
>> + *primary = *secondary = 1;
>
> I think the above two if statements should be collapsed into a single one.
This is code is actually from the pata_legacy.c but yep, you're right,
those can merge.
>
>> +
>> + /* Intel MPIIX - PIO ATA on non PCI side of bridge */
>> + if (p->vendor == 0x8086 && p->device == 0x1234) {
>
> Also, perhaps it makes sense to #include <linux/pci_ids.h> and use the
> macros defined there...
Will look into it later and redo the patch, thanks for reviewing. I
still haven't heard from
Bart, though, whether he's OK with the code duplication...?
--
Regards/Gru?,
Boris
On Tue, Aug 5, 2008 at 4:32 PM, Boris Petkov <[email protected]> wrote:
[...]
> Will look into it later and redo the patch, thanks for reviewing. I
> still haven't heard from
> Bart, though, whether he's OK with the code duplication...?
Given Alan & Jeff concerns about moving it to <linux/ata.h>, just
copying the function to ide_generic seems like a best solution for now.
On Tue, Aug 05, 2008 at 04:41:54PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Tue, Aug 5, 2008 at 4:32 PM, Boris Petkov <[email protected]> wrote:
>
> [...]
>
> > Will look into it later and redo the patch, thanks for reviewing. I
> > still haven't heard from
> > Bart, though, whether he's OK with the code duplication...?
>
> Given Alan & Jeff concerns about moving it to <linux/ata.h>, just
> copying the function to ide_generic seems like a best solution for now.
---
From: Borislav Petkov <[email protected]>
Date: Sun, 3 Aug 2008 18:46:35 +0200
Subject: [PATCH] ide-generic: handle probing of legacy io-ports v4
Avoid probing the io-ports in case an IDE PCI controller is present and it uses
the legacy iobases. If we still want to enforce the probing, we do
ide_generic.probe_mask=0x3f
on the kernel command line. The iobase checking code is adapted from
drivers/ata/pata_legacy.c after converting hex pci ids into their corresponding
macros in <linux/pci_ids.h>.
CC: Sergei Shtylyov <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-generic.c | 59 +++++++++++++++++++++++++++++++++++++++++---
1 files changed, 55 insertions(+), 4 deletions(-)
diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
index 8fe8b5b..efce159 100644
--- a/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -19,6 +19,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/ide.h>
+#include <linux/pci_ids.h>
/* FIXME: convert m32r to use ide_platform host driver */
#ifdef CONFIG_M32R
@@ -27,7 +28,7 @@
#define DRV_NAME "ide_generic"
-static int probe_mask = 0x03;
+static int probe_mask;
module_param(probe_mask, int, 0);
MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
@@ -100,19 +101,69 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
#endif
+static void ide_generic_check_pci_legacy_iobases(int *primary, int *secondary)
+{
+ struct pci_dev *p = NULL;
+ u16 val;
+
+ for_each_pci_dev(p) {
+ int r;
+
+ for (r = 0; r < 6; r++) {
+ if (pci_resource_start(p, r) == 0x1f0)
+ *primary = 1;
+ if (pci_resource_start(p, r) == 0x170)
+ *secondary = 1;
+ }
+
+ /* Cyrix CS55{1,2}0 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == PCI_VENDOR_ID_CYRIX &&
+ (p->device == PCI_DEVICE_ID_CYRIX_5510 ||
+ p->device == PCI_DEVICE_ID_CYRIX_5520))
+ *primary = *secondary = 1;
+
+ /* Intel MPIIX - PIO ATA on non PCI side of bridge */
+ if (p->vendor == PCI_VENDOR_ID_INTEL &&
+ p->device == PCI_DEVICE_ID_INTEL_82371MX) {
+
+ pci_read_config_word(p, 0x6C, &val);
+ if (val & 0x8000) {
+ /* ATA port enabled */
+ if (val & 0x4000)
+ *secondary = 1;
+ else
+ *primary = 1;
+ }
+ }
+ }
+}
+
static int __init ide_generic_init(void)
{
hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
struct ide_host *host;
unsigned long io_addr;
- int i, rc;
+ int i, rc, primary = 0, secondary = 0;
#ifdef CONFIG_MIPS
if (!ide_probe_legacy())
return -ENODEV;
#endif
- printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
- "parameter for probing all legacy ISA IDE ports\n");
+ ide_generic_check_pci_legacy_iobases(&primary, &secondary);
+
+ if (!probe_mask) {
+ printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" "
+ "module parameter for probing all legacy ISA IDE ports\n");
+
+ if (primary == 0)
+ probe_mask |= 0x1;
+
+ if (secondary == 0)
+ probe_mask |= 0x2;
+ } else {
+ printk(KERN_WARNING "%s: enforcing probing of io ports upon "
+ "user request.\n", DRV_NAME);
+ }
memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
--
1.5.5.4
--
Regards/Gruss,
Boris.
Hello.
Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Date: Sun, 3 Aug 2008 18:46:35 +0200
> Subject: [PATCH] ide-generic: handle probing of legacy io-ports v4
>
> Avoid probing the io-ports in case an IDE PCI controller is present and it uses
> the legacy iobases. If we still want to enforce the probing, we do
>
> ide_generic.probe_mask=0x3f
>
> on the kernel command line. The iobase checking code is adapted from
> drivers/ata/pata_legacy.c after converting hex pci ids into their corresponding
> macros in <linux/pci_ids.h>.
>
> CC: Sergei Shtylyov <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
>
Acked-by: Sergei Shtylyov <[email protected]>
> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> index 8fe8b5b..efce159 100644
> --- a/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
>
[...]
> @@ -100,19 +101,69 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
> #endif
>
> +static void ide_generic_check_pci_legacy_iobases(int *primary, int *secondary)
> +{
> + struct pci_dev *p = NULL;
> + u16 val;
> +
> + for_each_pci_dev(p) {
> + int r;
> +
> + for (r = 0; r < 6; r++) {
> + if (pci_resource_start(p, r) == 0x1f0)
> + *primary = 1;
> + if (pci_resource_start(p, r) == 0x170)
> + *secondary = 1;
> + }
>
Would have been probably enough to test only BAR0/2, don't you think?
MBR, Sergei
On Wed, Aug 6, 2008 at 1:34 PM, Sergei Shtylyov <[email protected]> wrote:
> Hello.
>
> Borislav Petkov wrote:
>>
>> From: Borislav Petkov <[email protected]>
>> Date: Sun, 3 Aug 2008 18:46:35 +0200
>> Subject: [PATCH] ide-generic: handle probing of legacy io-ports v4
>>
>> Avoid probing the io-ports in case an IDE PCI controller is present and it
>> uses
>> the legacy iobases. If we still want to enforce the probing, we do
>>
>> ide_generic.probe_mask=0x3f
>>
>> on the kernel command line. The iobase checking code is adapted from
>> drivers/ata/pata_legacy.c after converting hex pci ids into their
>> corresponding
>> macros in <linux/pci_ids.h>.
>>
>> CC: Sergei Shtylyov <[email protected]>
>> Signed-off-by: Borislav Petkov <[email protected]>
>>
>
> Acked-by: Sergei Shtylyov <[email protected]>
>
>> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
>> index 8fe8b5b..efce159 100644
>> --- a/drivers/ide/ide-generic.c
>> +++ b/drivers/ide/ide-generic.c
>>
>
> [...]
>>
>> @@ -100,19 +101,69 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170,
>> 0x1e8, 0x168, 0x1e0, 0x160 };
>> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
>> #endif
>> +static void ide_generic_check_pci_legacy_iobases(int *primary, int
>> *secondary)
>> +{
>> + struct pci_dev *p = NULL;
>> + u16 val;
>> +
>> + for_each_pci_dev(p) {
>> + int r;
>> +
>> + for (r = 0; r < 6; r++) {
>> + if (pci_resource_start(p, r) == 0x1f0)
>> + *primary = 1;
>> + if (pci_resource_start(p, r) == 0x170)
>> + *secondary = 1;
>> + }
>>
>
> Would have been probably enough to test only BAR0/2, don't you think?
I assume you're referring to the legacy ioports fixup in
drivers/pci/probe.c:pci_setup_device(). Yes, there's no need to go all the way
to BAR5 since those are guaranteed unused in compatibility mode, so actually the
loop should go till 4. Bart, can you please change that when applying?
--
Regards/Gru?,
Boris
Boris Petkov wrote:
>>>From: Borislav Petkov <[email protected]>
>>>Date: Sun, 3 Aug 2008 18:46:35 +0200
>>>Subject: [PATCH] ide-generic: handle probing of legacy io-ports v4
>>>Avoid probing the io-ports in case an IDE PCI controller is present and it uses
>>>the legacy iobases. If we still want to enforce the probing, we do
>>>ide_generic.probe_mask=0x3f
>>>on the kernel command line. The iobase checking code is adapted from
>>>drivers/ata/pata_legacy.c after converting hex pci ids into their
>>>corresponding
>>>macros in <linux/pci_ids.h>.
>>>CC: Sergei Shtylyov <[email protected]>
>>>Signed-off-by: Borislav Petkov <[email protected]>
>>Acked-by: Sergei Shtylyov <[email protected]>
>>>diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
>>>index 8fe8b5b..efce159 100644
>>>--- a/drivers/ide/ide-generic.c
>>>+++ b/drivers/ide/ide-generic.c
>>[...]
>>>@@ -100,19 +101,69 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170,
>>>0x1e8, 0x168, 0x1e0, 0x160 };
>>> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
>>> #endif
>>> +static void ide_generic_check_pci_legacy_iobases(int *primary, int
>>>*secondary)
>>>+{
>>>+ struct pci_dev *p = NULL;
>>>+ u16 val;
>>>+
>>>+ for_each_pci_dev(p) {
>>>+ int r;
>>>+
>>>+ for (r = 0; r < 6; r++) {
>>>+ if (pci_resource_start(p, r) == 0x1f0)
>>>+ *primary = 1;
>>>+ if (pci_resource_start(p, r) == 0x170)
>>>+ *secondary = 1;
>>>+ }
>> Would have been probably enough to test only BAR0/2, don't you think?
> I assume you're referring to the legacy ioports fixup in
> drivers/pci/probe.c:pci_setup_device().
And to the fact that the value 0x1f0 should only be ever seen in BAR0 and
0x170 in BAR2 even if they would have been read off the chips (some chips have
these values reading back even in legacy mode, and even could malfunction if
other values are written there), not fixed up there, and certainly not in BAR1
or BAR3, so it's quite pointless to look in these BARs too.
WBR, Sergei
On Wed, Aug 06, 2008 at 07:57:40PM +0400, Sergei Shtylyov wrote:
> Boris Petkov wrote:
>
>>>> From: Borislav Petkov <[email protected]>
>>>> Date: Sun, 3 Aug 2008 18:46:35 +0200
>>>> Subject: [PATCH] ide-generic: handle probing of legacy io-ports v4
>
>>>> Avoid probing the io-ports in case an IDE PCI controller is present and it uses
>>>> the legacy iobases. If we still want to enforce the probing, we do
>
>>>> ide_generic.probe_mask=0x3f
>
>>>> on the kernel command line. The iobase checking code is adapted from
>>>> drivers/ata/pata_legacy.c after converting hex pci ids into their
>>>> corresponding
>>>> macros in <linux/pci_ids.h>.
>
>>>> CC: Sergei Shtylyov <[email protected]>
>>>> Signed-off-by: Borislav Petkov <[email protected]>
>
>>> Acked-by: Sergei Shtylyov <[email protected]>
>
>>>> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
>>>> index 8fe8b5b..efce159 100644
>>>> --- a/drivers/ide/ide-generic.c
>>>> +++ b/drivers/ide/ide-generic.c
>
>>> [...]
>
>>>> @@ -100,19 +101,69 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170,
>>>> 0x1e8, 0x168, 0x1e0, 0x160 };
>>>> static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
>>>> #endif
>>>> +static void ide_generic_check_pci_legacy_iobases(int *primary, int
>>>> *secondary)
>>>> +{
>>>> + struct pci_dev *p = NULL;
>>>> + u16 val;
>>>> +
>>>> + for_each_pci_dev(p) {
>>>> + int r;
>>>> +
>>>> + for (r = 0; r < 6; r++) {
>>>> + if (pci_resource_start(p, r) == 0x1f0)
>>>> + *primary = 1;
>>>> + if (pci_resource_start(p, r) == 0x170)
>>>> + *secondary = 1;
>>>> + }
>
>>> Would have been probably enough to test only BAR0/2, don't you think?
>
>> I assume you're referring to the legacy ioports fixup in
>> drivers/pci/probe.c:pci_setup_device().
>
> And to the fact that the value 0x1f0 should only be ever seen in BAR0
> and 0x170 in BAR2 even if they would have been read off the chips (some
> chips have these values reading back even in legacy mode, and even could
> malfunction if other values are written there), not fixed up there, and
> certainly not in BAR1 or BAR3, so it's quite pointless to look in these
> BARs too.
So the comment in there saying that in some cases BAR0-3 could contain junk is a
bogus? In other words, can we assume that one will always read 0x1f0 from BAR0
and 0x170 from BAR2 in compatibility mode. If so, the check is even simpler:
if (pci_resource_start(p, 0) == 0x1f0)
*primary = 1;
if (pci_resource_start(p, 2) == 0x170)
*secondary = 1;
--
Regards/Gruss,
Boris.
> So the comment in there saying that in some cases BAR0-3 could contain junk is a
> bogus? In other words, can we assume that one will always read 0x1f0 from BAR0
> and 0x170 from BAR2 in compatibility mode. If so, the check is even simpler:
>
> if (pci_resource_start(p, 0) == 0x1f0)
> *primary = 1;
> if (pci_resource_start(p, 2) == 0x170)
> *secondary = 1;
In theory, but for the sake of about 30 bytes of code in an obscure 'last
resort work everywhere' driver it seemed a bit pointless.
Alan
Borislav Petkov wrote:
>>>>>From: Borislav Petkov <[email protected]>
>>>>>Date: Sun, 3 Aug 2008 18:46:35 +0200
>>>>>Subject: [PATCH] ide-generic: handle probing of legacy io-ports v4
>>>>>Avoid probing the io-ports in case an IDE PCI controller is present and it uses
>>>>>the legacy iobases. If we still want to enforce the probing, we do
>>>>>ide_generic.probe_mask=0x3f
>>>>>on the kernel command line. The iobase checking code is adapted from
>>>>>drivers/ata/pata_legacy.c after converting hex pci ids into their
>>>>>corresponding
>>>>>macros in <linux/pci_ids.h>.
>>>>>CC: Sergei Shtylyov <[email protected]>
>>>>>Signed-off-by: Borislav Petkov <[email protected]>
>>>>Acked-by: Sergei Shtylyov <[email protected]>
>>>>>diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
>>>>>index 8fe8b5b..efce159 100644
>>>>>--- a/drivers/ide/ide-generic.c
>>>>>+++ b/drivers/ide/ide-generic.c
>>>>[...]
>>>>>@@ -100,19 +101,69 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170,
>>>>>0x1e8, 0x168, 0x1e0, 0x160 };
>>>>>static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
>>>>>#endif
>>>>>+static void ide_generic_check_pci_legacy_iobases(int *primary, int
>>>>>*secondary)
>>>>>+{
>>>>>+ struct pci_dev *p = NULL;
>>>>>+ u16 val;
>>>>>+
>>>>>+ for_each_pci_dev(p) {
>>>>>+ int r;
>>>>>+
>>>>>+ for (r = 0; r < 6; r++) {
>>>>>+ if (pci_resource_start(p, r) == 0x1f0)
>>>>>+ *primary = 1;
>>>>>+ if (pci_resource_start(p, r) == 0x170)
>>>>>+ *secondary = 1;
>>>>>+ }
>>>>Would have been probably enough to test only BAR0/2, don't you think?
>>>I assume you're referring to the legacy ioports fixup in
>>>drivers/pci/probe.c:pci_setup_device().
>> And to the fact that the value 0x1f0 should only be ever seen in BAR0
>>and 0x170 in BAR2 even if they would have been read off the chips (some
>>chips have these values reading back even in legacy mode, and even could
>>malfunction if other values are written there), not fixed up there, and
>>certainly not in BAR1 or BAR3, so it's quite pointless to look in these
>>BARs too.
> So the comment in there saying that in some cases BAR0-3 could contain junk is a
> bogus?
Don't know. The PCI IDE spec. said that those should be 0 in compatibility
mode but I know that some controllers require the standard values of 0x1[f7]0
to be in BAR0/2 in compatibility mode. If my memory serves, NatSemi PC8741x
were ones of those...
> In other words, can we assume that one will always read 0x1f0 from BAR0
> and 0x170 from BAR2 in compatibility mode.
Certainly not. We can only rely on the workaround putting 0x1[f7]0 in the
resources 0/2.
> If so, the check is even simpler:
> if (pci_resource_start(p, 0) == 0x1f0)
> *primary = 1;
> if (pci_resource_start(p, 2) == 0x170)
> *secondary = 1;
Yes, I think that should be enough...
WBR, Sergei
On Thu, Aug 07, 2008 at 12:04:19AM +0400, Sergei Shtylyov wrote:
[.. ]
>>>>> Would have been probably enough to test only BAR0/2, don't you think?
>
>>>> I assume you're referring to the legacy ioports fixup in
>>>> drivers/pci/probe.c:pci_setup_device().
>
>>> And to the fact that the value 0x1f0 should only be ever seen in
>>> BAR0 and 0x170 in BAR2 even if they would have been read off the
>>> chips (some chips have these values reading back even in legacy mode,
>>> and even could malfunction if other values are written there), not
>>> fixed up there, and certainly not in BAR1 or BAR3, so it's quite
>>> pointless to look in these BARs too.
>
>> So the comment in there saying that in some cases BAR0-3 could contain junk is a
>> bogus?
>
> Don't know. The PCI IDE spec. said that those should be 0 in
> compatibility mode but I know that some controllers require the standard
> values of 0x1[f7]0 to be in BAR0/2 in compatibility mode. If my memory
> serves, NatSemi PC8741x were ones of those...
>
>> In other words, can we assume that one will always read 0x1f0 from BAR0
>> and 0x170 from BAR2 in compatibility mode.
>
> Certainly not. We can only rely on the workaround putting 0x1[f7]0 in
> the resources 0/2.
>
>> If so, the check is even simpler:
>
>> if (pci_resource_start(p, 0) == 0x1f0)
>> *primary = 1;
>> if (pci_resource_start(p, 2) == 0x170)
>> *secondary = 1;
>
> Yes, I think that should be enough...
Here's v5:
---
From: Borislav Petkov <[email protected]>
Date: Sun, 3 Aug 2008 18:46:35 +0200
Subject: [PATCH] ide-generic: handle probing of legacy io-ports v5
Avoid probing the io-ports in case an IDE PCI controller is present and it
uses the legacy iobases. If we still want to enforce the probing, we do
ide_generic.probe_mask=0x3f
on the kernel command line. The iobase checking code is
adapted from drivers/ata/pata_legacy.c after converting hex
pci ids into their corresponding macros in <linux/pci_ids.h>.
Also, check only BAR0/2 resources since those are guaranteed
by the workaround in drivers/pci/probe.c:pci_setup_device().
CC: Sergei Shtylyov <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
drivers/ide/ide-generic.c | 56 +++++++++++++++++++++++++++++++++++++++++---
1 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
index 8fe8b5b..608f353 100644
--- a/drivers/ide/ide-generic.c
+++ b/drivers/ide/ide-generic.c
@@ -19,6 +19,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/ide.h>
+#include <linux/pci_ids.h>
/* FIXME: convert m32r to use ide_platform host driver */
#ifdef CONFIG_M32R
@@ -27,7 +28,7 @@
#define DRV_NAME "ide_generic"
-static int probe_mask = 0x03;
+static int probe_mask;
module_param(probe_mask, int, 0);
MODULE_PARM_DESC(probe_mask, "probe mask for legacy ISA IDE ports");
@@ -100,19 +101,66 @@ static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168, 0x1e0, 0x160 };
static const int legacy_irqs[] = { 14, 15, 11, 10, 8, 12 };
#endif
+static void ide_generic_check_pci_legacy_iobases(int *primary, int *secondary)
+{
+ struct pci_dev *p = NULL;
+ u16 val;
+
+ for_each_pci_dev(p) {
+
+ if (pci_resource_start(p, 0) == 0x1f0)
+ *primary = 1;
+ if (pci_resource_start(p, 2) == 0x170)
+ *secondary = 1;
+
+ /* Cyrix CS55{1,2}0 pre SFF MWDMA ATA on the bridge */
+ if (p->vendor == PCI_VENDOR_ID_CYRIX &&
+ (p->device == PCI_DEVICE_ID_CYRIX_5510 ||
+ p->device == PCI_DEVICE_ID_CYRIX_5520))
+ *primary = *secondary = 1;
+
+ /* Intel MPIIX - PIO ATA on non PCI side of bridge */
+ if (p->vendor == PCI_VENDOR_ID_INTEL &&
+ p->device == PCI_DEVICE_ID_INTEL_82371MX) {
+
+ pci_read_config_word(p, 0x6C, &val);
+ if (val & 0x8000) {
+ /* ATA port enabled */
+ if (val & 0x4000)
+ *secondary = 1;
+ else
+ *primary = 1;
+ }
+ }
+ }
+}
+
static int __init ide_generic_init(void)
{
hw_regs_t hw[MAX_HWIFS], *hws[MAX_HWIFS];
struct ide_host *host;
unsigned long io_addr;
- int i, rc;
+ int i, rc, primary = 0, secondary = 0;
#ifdef CONFIG_MIPS
if (!ide_probe_legacy())
return -ENODEV;
#endif
- printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" module "
- "parameter for probing all legacy ISA IDE ports\n");
+ ide_generic_check_pci_legacy_iobases(&primary, &secondary);
+
+ if (!probe_mask) {
+ printk(KERN_INFO DRV_NAME ": please use \"probe_mask=0x3f\" "
+ "module parameter for probing all legacy ISA IDE ports\n");
+
+ if (primary == 0)
+ probe_mask |= 0x1;
+
+ if (secondary == 0)
+ probe_mask |= 0x2;
+ } else {
+ printk(KERN_WARNING "%s: enforcing probing of io ports upon "
+ "user request.\n", DRV_NAME);
+ }
memset(hws, 0, sizeof(hw_regs_t *) * MAX_HWIFS);
--
1.5.5.4
--
Regards/Gruss,
Boris.
On Thursday 07 August 2008, Borislav Petkov wrote:
[...]
> Here's v5:
>
> ---
> From: Borislav Petkov <[email protected]>
> Date: Sun, 3 Aug 2008 18:46:35 +0200
> Subject: [PATCH] ide-generic: handle probing of legacy io-ports v5
>
> Avoid probing the io-ports in case an IDE PCI controller is present and it
> uses the legacy iobases. If we still want to enforce the probing, we do
>
> ide_generic.probe_mask=0x3f
>
> on the kernel command line. The iobase checking code is
> adapted from drivers/ata/pata_legacy.c after converting hex
> pci ids into their corresponding macros in <linux/pci_ids.h>.
>
> Also, check only BAR0/2 resources since those are guaranteed
> by the workaround in drivers/pci/probe.c:pci_setup_device().
>
> CC: Sergei Shtylyov <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
applied, thanks
Hello.
Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
> Date: Sun, 3 Aug 2008 18:46:35 +0200
> Subject: [PATCH] ide-generic: handle probing of legacy io-ports v5
>
> Avoid probing the io-ports in case an IDE PCI controller is present and it
> uses the legacy iobases. If we still want to enforce the probing, we do
>
> ide_generic.probe_mask=0x3f
>
> on the kernel command line. The iobase checking code is
> adapted from drivers/ata/pata_legacy.c after converting hex
> pci ids into their corresponding macros in <linux/pci_ids.h>.
>
> Also, check only BAR0/2 resources since those are guaranteed
> by the workaround in drivers/pci/probe.c:pci_setup_device().
>
> CC: Sergei Shtylyov <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
Acked-by: Sergei Shtylyov <[email protected]>
MBR, Sergei