Re: 2.6.18-mm2

Re: 2.6.18-mm2

[J.A. Magallón]

On Thu, 28 Sep 2006 01:46:23 -0700, Andrew Morton <akpm@osdl.org> wrote:

> 
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18/2.6.18-mm2/
> 
> 

aic7xxx oopses on boot:

PCI: Setting latency timer of device 0000:00:0e.0 to 64
IRQ handler type mismatch for IRQ 0
 [<c013c697>] setup_irq+0xb7/0x1b0
 [<c0274770>] ahc_linux_isr+0x0/0x50
 [<c013c833>] request_irq+0xa3/0xc0
 [<c027605c>] ahc_pci_map_int+0x2c/0x50
 [<c027167a>] ahc_pci_config+0x5ea/0xcf0
 [<c0208c00>] pci_bus_write_config_byte+0x30/0x70
 [<c02761dc>] ahc_linux_pci_dev_probe+0xec/0x1e0
 [<c01983b5>] sysfs_dirent_exist+0x45/0x70
 [<c019927b>] sysfs_create_link+0x7b/0x180
 [<c020d643>] pci_match_device+0x13/0xd0
 [<c0202b2f>] kobject_get+0xf/0x20
 [<c020d776>] pci_device_probe+0x56/0x80
 [<c024ea7b>] really_probe+0x3b/0xe0
 [<c024eb5f>] driver_probe_device+0x3f/0xa0
 [<c030c7a3>] klist_next+0x53/0xa0
 [<c024ecba>] __driver_attach+0x7a/0x80
 [<c024e01a>] bus_for_each_dev+0x3a/0x60
 [<c024e986>] driver_attach+0x16/0x20
 [<c024ec40>] __driver_attach+0x0/0x80
 [<c024e39c>] bus_add_driver+0x7c/0x1a0
 [<c020d935>] __pci_register_driver+0x65/0x90
 [<c0405749>] ahc_linux_init+0x79/0x90
 [<c01004b0>] init+0x120/0x330
 [<c0102eca>] ret_from_fork+0x6/0x1c
 [<c0100390>] init+0x0/0x330
 [<c0100390>] init+0x0/0x330
 [<c0103b13>] kernel_thread_helper+0x7/0x14
 =======================
aic7xxx: probe of 0000:00:0e.0 failed with error -16

lspci:

leda:~# lspci
00:00.0 Host bridge: Intel Corporation 440BX/ZX/DX - 82443BX/ZX/DX Host bridge (rev 03)
00:01.0 PCI bridge: Intel Corporation 440BX/ZX/DX - 82443BX/ZX/DX AGP bridge (rev 03)
00:07.0 ISA bridge: Intel Corporation 82371AB/EB/MB PIIX4 ISA (rev 02)
00:07.1 IDE interface: Intel Corporation 82371AB/EB/MB PIIX4 IDE (rev 01)
00:07.2 USB Controller: Intel Corporation 82371AB/EB/MB PIIX4 USB (rev 01)
00:07.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 02)
00:0d.0 SCSI storage controller: Adaptec AIC-7892A U160/m (rev 02)
00:0e.0 SCSI storage controller: Adaptec AHA-2940U2/U2W / 7890/7891 (rev 01)
00:0f.0 Ethernet controller: 3Com Corporation 3c905B 100BaseTX [Cyclone] (rev 64)
00:12.0 Multimedia audio controller: Creative Labs SB Live! EMU10k1 (rev 07)
00:12.1 Input device controller: Creative Labs SB Live! Game Port (rev 07)
01:00.0 VGA compatible controller: nVidia Corporation NV34 [GeForce FX 5200] (rev a1)

(the 2940 is onboard and the U160 is a PCI card).

Full dmesg follows:

Linux version 2.6.18-jam02 (root@rescue) (gcc version 4.1.1 20060724 (prerelease) (4.1.1-3mdk)) #1 SMP Fri Sep 29 12:31:45 CEST 2006
BIOS-provided physical RAM map:
sanitize start
sanitize end
copy_e820_map() start: 0000000000000000 size: 000000000009fc00 end: 000000000009fc00 type: 1
copy_e820_map() type is E820_RAM
copy_e820_map() start: 000000000009fc00 size: 0000000000000400 end: 00000000000a0000 type: 2
copy_e820_map() start: 00000000000e0000 size: 0000000000020000 end: 0000000000100000 type: 2
copy_e820_map() start: 0000000000100000 size: 000000001ff00000 end: 0000000020000000 type: 1
copy_e820_map() type is E820_RAM
copy_e820_map() start: 00000000fec00000 size: 0000000000001000 end: 00000000fec01000 type: 2
copy_e820_map() start: 00000000fee00000 size: 0000000000001000 end: 00000000fee01000 type: 2
copy_e820_map() start: 00000000fffc0000 size: 0000000000040000 end: 0000000100000000 type: 2
 BIOS-e820: 0000000000000000 - 000000000009fc00 (usable)
 BIOS-e820: 000000000009fc00 - 00000000000a0000 (reserved)
 BIOS-e820: 00000000000e0000 - 0000000000100000 (reserved)
 BIOS-e820: 0000000000100000 - 0000000020000000 (usable)
 BIOS-e820: 00000000fec00000 - 00000000fec01000 (reserved)
 BIOS-e820: 00000000fee00000 - 00000000fee01000 (reserved)
 BIOS-e820: 00000000fffc0000 - 0000000100000000 (reserved)
0MB HIGHMEM available.
512MB LOWMEM available.
found SMP MP-table at 000fb4c0
Entering add_active_range(0, 0, 131072) 0 entries of 256 used
Zone PFN ranges:
  DMA             0 ->     4096
  Normal       4096 ->   131072
  HighMem    131072 ->   131072
early_node_map[1] active PFN ranges
    0:        0 ->   131072
On node 0 totalpages: 131072
  DMA zone: 32 pages used for memmap
  DMA zone: 0 pages reserved
  DMA zone: 4064 pages, LIFO batch:0
  Normal zone: 992 pages used for memmap
  Normal zone: 125984 pages, LIFO batch:31
  HighMem zone: 0 pages used for memmap
DMI 2.1 present.
ACPI: Unable to locate RSDP
Intel MultiProcessor Specification v1.4
    Virtual Wire compatibility mode.
OEM ID: INTEL    Product ID: 440BX        APIC at: 0xFEE00000
Processor #0 6:7 APIC version 17
Processor #1 6:7 APIC version 17
I/O APIC #2 Version 17 at 0xFEC00000.
Enabling APIC mode:  Flat.  Using 1 I/O APICs
Processors: 2
Allocating PCI resources starting at 30000000 (gap: 20000000:dec00000)
Detected 501.164 MHz processor.
Built 1 zonelists.  Total pages: 130048
Kernel command line: vga=6 root=/dev/sda1
mapped APIC to ffffd000 (fee00000)
mapped IOAPIC to ffffc000 (fec00000)
Enabling fast FPU save and restore... done.
Enabling unmasked SIMD FPU exception support... done.
Initializing CPU#0
PID hash table entries: 2048 (order: 11, 8192 bytes)
Console: colour VGA+ 80x60
Dentry cache hash table entries: 65536 (order: 6, 262144 bytes)
Inode-cache hash table entries: 32768 (order: 5, 131072 bytes)
Memory: 515792k/524288k available (2111k kernel code, 8076k reserved, 845k data, 204k init, 0k highmem)
virtual kernel memory layout:
    fixmap  : 0xfff9d000 - 0xfffff000   ( 392 kB)
    pkmap   : 0xff800000 - 0xffc00000   (4096 kB)
    vmalloc : 0xe0800000 - 0xff7fe000   ( 495 MB)
    lowmem  : 0xc0000000 - 0xe0000000   ( 512 MB)
      .init : 0xc03ea000 - 0xc041d000   ( 204 kB)
      .data : 0xc030fe3a - 0xc03e33a4   ( 845 kB)
      .text : 0xc0100000 - 0xc030fe3a   (2111 kB)
Checking if this processor honours the WP bit even in supervisor mode... Ok.
Calibrating delay using timer specific routine.. 1003.12 BogoMIPS (lpj=5015604)
Mount-cache hash table entries: 512
CPU: After generic identify, caps: 0383fbff 00000000 00000000 00000000 00000000 00000000 00000000
CPU: L1 I cache: 16K, L1 D cache: 16K
CPU: L2 cache: 512K
CPU: After all inits, caps: 0383fbff 00000000 00000000 00000040 00000000 00000000 00000000
Checking 'hlt' instruction... OK.
Freeing SMP alternatives: 16k freed
CPU0: Intel Pentium III (Katmai) stepping 03
Booting processor 1/1 eip 2000
Initializing CPU#1
Calibrating delay using timer specific routine.. 1002.31 BogoMIPS (lpj=5011557)
CPU: After generic identify, caps: 0383fbff 00000000 00000000 00000000 00000000 00000000 00000000
CPU: L1 I cache: 16K, L1 D cache: 16K
CPU: L2 cache: 512K
CPU: After all inits, caps: 0383fbff 00000000 00000000 00000040 00000000 00000000 00000000
CPU1: Intel Pentium III (Katmai) stepping 03
Total of 2 processors activated (2005.43 BogoMIPS).
ExtINT not setup in hardware but reported by MP table
ENABLING IO-APIC IRQs
..TIMER: vector=0x31 apic1=0 pin1=2 apic2=0 pin2=0
checking TSC synchronization across 2 CPUs: passed.
Brought up 2 CPUs
migration_cost=2850
NET: Registered protocol family 16
PCI: PCI BIOS revision 2.10 entry at 0xfdb81, last bus=1
PCI: Using configuration type 1
Setting up standard PCI resources
ACPI: Interpreter disabled.
SCSI subsystem initialized
PCI: Probing PCI hardware
PCI: Probing PCI hardware (bus 00)
* Found PM-Timer Bug on the chipset. Due to workarounds for a bug,
* this clock source is slow. Consider trying other clock sources
PCI quirk: region 0400-043f claimed by PIIX4 ACPI
PCI quirk: region 0440-044f claimed by PIIX4 SMB
PIIX4 devres B PIO at 0290-0297
Boot video device is 0000:01:00.0
PCI: Cannot allocate resource region 0 of device 0000:00:0e.0
PCI: Bridge: 0000:00:01.0
  IO window: d000-dfff
  MEM window: fca00000-feafffff
  PREFETCH window: e4800000-f48fffff
NET: Registered protocol family 2
IP route cache hash table entries: 16384 (order: 4, 65536 bytes)
TCP established hash table entries: 65536 (order: 7, 524288 bytes)
TCP bind hash table entries: 32768 (order: 6, 262144 bytes)
TCP: Hash tables configured (established 65536 bind 32768)
TCP reno registered
Installing knfsd (copyright (C) 1996 okir@monad.swb.de).
io scheduler noop registered
io scheduler anticipatory registered (default)
io scheduler deadline registered
io scheduler cfq registered
Limiting direct PCI/PCI transfers.
Real Time Clock Driver v1.12ac
Non-volatile memory driver v1.2
Serial: 8250/16550 driver $Revision: 1.90 $ 4 ports, IRQ sharing disabled
serial8250: ttyS0 at I/O 0x3f8 (irq = 4) is a 16550A
serial8250: ttyS1 at I/O 0x2f8 (irq = 3) is a 16550A
Floppy drive(s): fd0 is 1.44M
FDC 0 is a post-1991 82077
scsi0 : Adaptec AIC7XXX EISA/VLB/PCI SCSI HBA DRIVER, Rev 7.0
        <Adaptec 29160 Ultra160 SCSI adapter>
        aic7892: Ultra160 Wide Channel A, SCSI Id=7, 32/253 SCBs

scsi 0:0:0:0: Direct-Access     IBM      IC35L018UWD210-0 S5BS PQ: 0 ANSI: 3
scsi0:A:0:0: Tagged Queuing enabled.  Depth 32
 target0:0:0: Beginning Domain Validation
 target0:0:0: wide asynchronous
 target0:0:0: FAST-80 WIDE SCSI 160.0 MB/s DT (12.5 ns, offset 63)
 target0:0:0: Ending Domain Validation
scsi 0:0:5:0: CD-ROM            TOSHIBA  CD-ROM XM-6401TA 1015 PQ: 0 ANSI: 2
 target0:0:5: Beginning Domain Validation
 target0:0:5: FAST-20 SCSI 20.0 MB/s ST (50 ns, offset 16)
 target0:0:5: Domain Validation skipping write tests
 target0:0:5: Ending Domain Validation
PCI: Enabling device 0000:00:0e.0 (0000 -> 0003)
PCI: No IRQ known for interrupt pin A of device 0000:00:0e.0. Probably buggy MP table.
PCI: Setting latency timer of device 0000:00:0e.0 to 64
IRQ handler type mismatch for IRQ 0
 [<c013c697>] setup_irq+0xb7/0x1b0
 [<c0274770>] ahc_linux_isr+0x0/0x50
 [<c013c833>] request_irq+0xa3/0xc0
 [<c027605c>] ahc_pci_map_int+0x2c/0x50
 [<c027167a>] ahc_pci_config+0x5ea/0xcf0
 [<c0208c00>] pci_bus_write_config_byte+0x30/0x70
 [<c02761dc>] ahc_linux_pci_dev_probe+0xec/0x1e0
 [<c01983b5>] sysfs_dirent_exist+0x45/0x70
 [<c019927b>] sysfs_create_link+0x7b/0x180
 [<c020d643>] pci_match_device+0x13/0xd0
 [<c0202b2f>] kobject_get+0xf/0x20
 [<c020d776>] pci_device_probe+0x56/0x80
 [<c024ea7b>] really_probe+0x3b/0xe0
 [<c024eb5f>] driver_probe_device+0x3f/0xa0
 [<c030c7a3>] klist_next+0x53/0xa0
 [<c024ecba>] __driver_attach+0x7a/0x80
 [<c024e01a>] bus_for_each_dev+0x3a/0x60
 [<c024e986>] driver_attach+0x16/0x20
 [<c024ec40>] __driver_attach+0x0/0x80
 [<c024e39c>] bus_add_driver+0x7c/0x1a0
 [<c020d935>] __pci_register_driver+0x65/0x90
 [<c0405749>] ahc_linux_init+0x79/0x90
 [<c01004b0>] init+0x120/0x330
 [<c0102eca>] ret_from_fork+0x6/0x1c
 [<c0100390>] init+0x0/0x330
 [<c0100390>] init+0x0/0x330
 [<c0103b13>] kernel_thread_helper+0x7/0x14
 =======================
aic7xxx: probe of 0000:00:0e.0 failed with error -16
SCSI device sda: 35843670 512-byte hdwr sectors (18352 MB)
sda: Write Protect is off
sda: Mode Sense: cb 00 00 08
SCSI device sda: drive cache: write through
SCSI device sda: 35843670 512-byte hdwr sectors (18352 MB)
sda: Write Protect is off
sda: Mode Sense: cb 00 00 08
SCSI device sda: drive cache: write through
 sda: sda1 sda2 < sda5 >
sd 0:0:0:0: Attached scsi disk sda
serio: i8042 AUX port at 0x60,0x64 irq 12
serio: i8042 KBD port at 0x60,0x64 irq 1
mice: PS/2 mouse device common for all mice
md: linear personality registered for level -1
md: raid0 personality registered for level 0
md: raid1 personality registered for level 1
md: raid10 personality registered for level 10
input: AT Translated Set 2 keyboard as /class/input/input0
raid6: int32x1     95 MB/s
logips2pp: Detected unknown logitech mouse model 1
raid6: int32x2     98 MB/s
raid6: int32x4    114 MB/s
raid6: int32x8    117 MB/s
input: PS/2 Logitech Mouse as /class/input/input1
raid6: mmxx1      217 MB/s
raid6: mmxx2      323 MB/s
raid6: sse1x1     245 MB/s
raid6: sse1x2     329 MB/s
raid6: using algorithm sse1x2 (329 MB/s)
md: raid6 personality registered for level 6
md: raid5 personality registered for level 5
md: raid4 personality registered for level 4
raid5: automatically using best checksumming function: pIII_sse
   pIII_sse  :  1014.400 MB/sec
raid5: using function: pIII_sse (1014.400 MB/sec)
md: md driver 0.90.3 MAX_MD_DEVS=256, MD_SB_DISKS=27
md: bitmap version 4.39
TCP cubic registered
NET: Registered protocol family 1
NET: Registered protocol family 17
Using IPI Shortcut mode
md: Autodetecting RAID arrays.
md: autorun ...
md: ... autorun DONE.
Time: tsc clocksource has been installed.
EXT3-fs: INFO: recovery required on readonly filesystem.
EXT3-fs: write access will be enabled during recovery.
kjournald starting.  Commit interval 5 seconds
EXT3-fs: recovery complete.
EXT3-fs: mounted filesystem with ordered data mode.
VFS: Mounted root (ext3 filesystem) readonly.
Freeing unused kernel memory: 204k freed
usbcore: registered new interface driver usbfs
usbcore: registered new interface driver hub
usbcore: registered new device driver usb
USB Universal Host Controller Interface driver v3.0
uhci_hcd 0000:00:07.2: UHCI Host Controller
uhci_hcd 0000:00:07.2: new USB bus registered, assigned bus number 1
uhci_hcd 0000:00:07.2: irq 9, io base 0x0000ef80
usb usb1: new device found, idVendor=0000, idProduct=0000
usb usb1: new device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: UHCI Host Controller
usb usb1: Manufacturer: Linux 2.6.18-jam02 uhci_hcd
usb usb1: SerialNumber: 0000:00:07.2
usb usb1: configuration #1 chosen from 1 choice
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 2 ports detected
sr0: scsi-1 drive
Uniform CD-ROM driver Revision: 3.20
sr 0:0:5:0: Attached scsi CD-ROM sr0
EXT3 FS on sda1, internal journal
libata version 2.00 loaded.
ata_piix 0000:00:07.1: version 2.00ac7
ata1: PATA max UDMA/33 cmd 0x1F0 ctl 0x3F6 bmdma 0xFFA0 irq 14
ata2: PATA max UDMA/33 cmd 0x170 ctl 0x376 bmdma 0xFFA8 irq 15
scsi1 : ata_piix
scsi2 : ata_piix
ATA: abnormal status 0x7F on port 0x177
ATA: abnormal status 0x7F on port 0x177
ata2.00: ATAPI, max MWDMA0, CDB intr
ata2.00: configured for PIO3
scsi 2:0:0:0: Direct-Access     IOMEGA   ZIP 250          51.G PQ: 0 ANSI: 5
sd 2:0:0:0: Attached scsi removable disk sdb
Adding 1148608k swap on /dev/sda5.  Priority:-1 extents:1 across:1148608k
loop: loaded (max 8 devices)
Uniform Multi-Platform E-IDE driver Revision: 7.00alpha2
ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx
3c59x: Donald Becker and others. www.scyld.com/network/vortex.html
0000:00:0f.0: 3Com PCI 3c905B Cyclone 100baseTx at e0804f80.
eth0:  setting full-duplex.
nfsd: last server has exited
nfsd: unexporting all filesystems
Linux agpgart interface v0.101 (c) Dave Jones
nvidia: module license 'NVIDIA' taints kernel.
NVRM: loading NVIDIA Linux x86 Kernel Module  1.0-9625  Thu Sep 14 15:33:21 PDT 2006

--
J.A. Magallon <jamagallon()ono!com>     \               Software is like sex:
                                         \         It's better when it's free
Mandriva Linux release 2007.0 (Cooker) for i586
Linux 2.6.18-jam02 (gcc 4.1.1 20060724 (prerelease) (4.1.1-3mdk)) #1 SMP PREEMPT

Re: 2.6.18-mm2

[Matthew Wilcox]

On Fri, Sep 29, 2006 at 03:57:38PM +0200, J.A. Magall??n wrote:
> aic7xxx oopses on boot:
> 
> PCI: Setting latency timer of device 0000:00:0e.0 to 64
> IRQ handler type mismatch for IRQ 0

Of course, this isn't a scsi problem, it's a peecee hardware problem.
Or maybe a PCI subsystem problem.  But it's clearly not aic7xxx's fault.

> PCI: Cannot allocate resource region 0 of device 0000:00:0e.0

That's not good.  Might be part of the problem.

> PCI: Enabling device 0000:00:0e.0 (0000 -> 0003)
> PCI: No IRQ known for interrupt pin A of device 0000:00:0e.0. Probably buggy MP table.

This is the direct problem.  You've got no irq.

Re: 2.6.18-mm2

[Alan Cox]

Ar Gwe, 2006-09-29 am 08:39 -0600, ysgrifennodd Matthew Wilcox:
> On Fri, Sep 29, 2006 at 03:57:38PM +0200, J.A. Magall??n wrote:
> > aic7xxx oopses on boot:
> > 
> > PCI: Setting latency timer of device 0000:00:0e.0 to 64
> > IRQ handler type mismatch for IRQ 0
> 
> Of course, this isn't a scsi problem, it's a peecee hardware problem.
> Or maybe a PCI subsystem problem.  But it's clearly not aic7xxx's fault.

AIC7xxx finding it has no IRQ configured is valid (annoying, stupid and
valid) so the driver should check before requesting "no IRQ"

Re: 2.6.18-mm2

[J.A. Magallón]

On Fri, 29 Sep 2006 08:39:49 -0600, Matthew Wilcox <matthew@wil.cx> wrote:

> On Fri, Sep 29, 2006 at 03:57:38PM +0200, J.A. Magall??n wrote:
> > aic7xxx oopses on boot:
> > 
> > PCI: Setting latency timer of device 0000:00:0e.0 to 64
> > IRQ handler type mismatch for IRQ 0
> 
> Of course, this isn't a scsi problem, it's a peecee hardware problem.
> Or maybe a PCI subsystem problem.  But it's clearly not aic7xxx's fault.
> 
> > PCI: Cannot allocate resource region 0 of device 0000:00:0e.0
> 
> That's not good.  Might be part of the problem.
> 
> > PCI: Enabling device 0000:00:0e.0 (0000 -> 0003)
> > PCI: No IRQ known for interrupt pin A of device 0000:00:0e.0. Probably buggy MP table.
> 
> This is the direct problem.  You've got no irq.
> 

Thanks...

Now I have just realized this:

00:0d.0 SCSI storage controller: Adaptec AIC-7892A U160/m (rev 02)
00:0e.0 SCSI storage controller: Adaptec AHA-2940U2/U2W / 7890/7891 (rev 01)

leda:~# lsscsi -Hv
[0]    aic7xxx     
  dir: /sys/class/scsi_host/host0
  device dir: /sys/devices/pci0000:00/0000:00:0d.0/host0
[1]    ata_piix    
  dir: /sys/class/scsi_host/host1
  device dir: /sys/devices/pci0000:00/0000:00:07.1/host1
[2]    ata_piix    
  dir: /sys/class/scsi_host/host2
  device dir: /sys/devices/pci0000:00/0000:00:07.1/host2

leda:~# lsscsi
[0:0:0:0]    disk    IBM      IC35L018UWD210-0 S5BS  /dev/sda
[0:0:5:0]    cd/dvd  TOSHIBA  CD-ROM XM-6401TA 1015  /dev/sr0
[2:0:0:0]    disk    IOMEGA   ZIP 250          51.G  /dev/sdb

Device 00:0e.0 is the 2940, which has nothing hung.
Who's to blame ? the bios because is assigns no interupts as no devices are
connected to the bus ? Or the kernel that should understand something like
'this device is disabled' ?

I can try to change the cdrom to the 2940 and see what happens...

Thanks, I will try the patch posted, it looks something like what I said
above, disable the device.

--
J.A. Magallon <jamagallon()ono!com>     \               Software is like sex:
                                         \         It's better when it's free
Mandriva Linux release 2007.0 (Cooker) for i586
Linux 2.6.18-jam02 (gcc 4.1.1 20060724 (prerelease) (4.1.1-3mdk)) #1 SMP PREEMPT

Re: 2.6.18-mm2

[Alan Cox]

Ar Gwe, 2006-09-29 am 23:50 +0000, ysgrifennodd Frederik Deweerdt:
> Does this patch makes sense in that case? If yes, I'll put up a patch
> for the remaining cases in the drivers/scsi/aic7xxx/ directory.
> Also, aic7xxx's coding style would put parenthesis around the returned
> value, should I follow it?

Yes - but perhaps with a warning message so users know why ?

As to coding style - kernel style is unbracketed so I wouldnt worry
about either.

Re: 2.6.18-mm2

[Frederik Deweerdt]

On Fri, Sep 29, 2006 at 06:15:42PM +0100, Alan Cox wrote:
> Ar Gwe, 2006-09-29 am 08:39 -0600, ysgrifennodd Matthew Wilcox:
> > On Fri, Sep 29, 2006 at 03:57:38PM +0200, J.A. Magall??n wrote:
> > > aic7xxx oopses on boot:
> > > 
> > > PCI: Setting latency timer of device 0000:00:0e.0 to 64
> > > IRQ handler type mismatch for IRQ 0
> > 
> > Of course, this isn't a scsi problem, it's a peecee hardware problem.
> > Or maybe a PCI subsystem problem.  But it's clearly not aic7xxx's fault.
> 
> AIC7xxx finding it has no IRQ configured is valid (annoying, stupid and
> valid) so the driver should check before requesting "no IRQ"
> 
Alan,

Does this patch makes sense in that case? If yes, I'll put up a patch
for the remaining cases in the drivers/scsi/aic7xxx/ directory.
Also, aic7xxx's coding style would put parenthesis around the returned
value, should I follow it?

Regards,
Frederik

diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index ea5687d..38f5ca7 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -185,6 +185,9 @@ ahc_linux_pci_dev_probe(struct pci_dev *
 	int		 error;
 	struct device	*dev = &pdev->dev;
 
+	if (!pdev->irq)
+		return -ENODEV;
+
 	pci = pdev;
 	entry = ahc_find_pci_device(pci);
 	if (entry == NULL)

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Willy Tarreau]

On Sat, Sep 30, 2006 at 03:19:14PM +0100, Alan Cox wrote:
> Ar Sad, 2006-09-30 am 14:09 +0000, ysgrifennodd Frederik Deweerdt:
> > Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>
> 
> Acked-by: Alan Cox <alan@redhat.com>

It seems to me that it's also valid for 2.4. Has someone any objection ?

Willy

[-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Frederik Deweerdt]

On Sat, Sep 30, 2006 at 12:43:24AM +0100, Alan Cox wrote:
> Ar Gwe, 2006-09-29 am 23:50 +0000, ysgrifennodd Frederik Deweerdt:
> > Does this patch makes sense in that case? If yes, I'll put up a patch
> > for the remaining cases in the drivers/scsi/aic7xxx/ directory.
> > Also, aic7xxx's coding style would put parenthesis around the returned
> > value, should I follow it?
> 
> Yes - but perhaps with a warning message so users know why ?
> 
> As to coding style - kernel style is unbracketed so I wouldnt worry
> about either.
> 
Thanks for the advices. 

The following patch checks whenever the irq is valid before issuing a
request_irq() for AIC7XXX and AIC79XX. An error message is displayed to
let the user know what went wrong.

Regards,
Frederik

Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>

diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
index 2001fe8..8279122 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
@@ -132,6 +132,11 @@ ahd_linux_pci_dev_probe(struct pci_dev *
 	char		*name;
 	int		 error;
 
+	if (!pdev->irq) {
+		printk(KERN_WARNING "aic79xx: No irq line set\n");
+		return -ENODEV;
+	}
+
 	pci = pdev;
 	entry = ahd_find_pci_device(pci);
 	if (entry == NULL)
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index ea5687d..ca61cdb 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -185,6 +185,11 @@ ahc_linux_pci_dev_probe(struct pci_dev *
 	int		 error;
 	struct device	*dev = &pdev->dev;
 
+	if (!pdev->irq) {
+		printk(KERN_WARNING "aic7xxx: No irq line set\n");
+		return -ENODEV;
+	}
+
 	pci = pdev;
 	entry = ahc_find_pci_device(pci);
 	if (entry == NULL)

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Alan Cox]

Ar Sad, 2006-09-30 am 14:09 +0000, ysgrifennodd Frederik Deweerdt:
> Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>

Acked-by: Alan Cox <alan@redhat.com>

Re: 2.6.18-mm2

[James Bottomley]

On Fri, 2006-09-29 at 23:50 +0000, Frederik Deweerdt wrote:
> +       if (!pdev->irq)
> +               return -ENODEV;
> +

Don't I remember that 0 is a valid IRQ on some platforms?

i.e. shouldn't this be

if (pdev->irq == NO_IRQ)
	return -ENODEV;

?

I think this won't quite work because only the platforms that actually
have a valid zero irq define it, but there must be something else that
works.

James

Re: 2.6.18-mm2

[Matthew Wilcox]

On Sat, Sep 30, 2006 at 10:26:22AM -0500, James Bottomley wrote:
> On Fri, 2006-09-29 at 23:50 +0000, Frederik Deweerdt wrote:
> > +       if (!pdev->irq)
> > +               return -ENODEV;
> > +
> 
> Don't I remember that 0 is a valid IRQ on some platforms?
> 
> i.e. shouldn't this be
> 
> if (pdev->irq == NO_IRQ)
> 	return -ENODEV;
> 
> ?
> 
> I think this won't quite work because only the platforms that actually
> have a valid zero irq define it, but there must be something else that
> works.

Linus threw a hissy fit and declared that platforms which use 0 as a
valid IRQ are broken and wrong.  Despite PCI using 255 to mean no IRQ
and 0 as a valid IRQ ;-)

Re: 2.6.18-mm2

[Mark Rustad]

On Sep 30, 2006, at 11:21 AM, Matthew Wilcox wrote:

> On Sat, Sep 30, 2006 at 10:26:22AM -0500, James Bottomley wrote:
>> On Fri, 2006-09-29 at 23:50 +0000, Frederik Deweerdt wrote:
>>> +       if (!pdev->irq)
>>> +               return -ENODEV;
>>> +
>>
>> Don't I remember that 0 is a valid IRQ on some platforms?
>>
>> i.e. shouldn't this be
>>
>> if (pdev->irq == NO_IRQ)
>> 	return -ENODEV;
>>
>> ?
>>
>> I think this won't quite work because only the platforms that  
>> actually
>> have a valid zero irq define it, but there must be something else  
>> that
>> works.
>
> Linus threw a hissy fit and declared that platforms which use 0 as a
> valid IRQ are broken and wrong.  Despite PCI using 255 to mean no IRQ
> and 0 as a valid IRQ ;-)

Having gone down the path of creating a platform that had IRQ 0 as a  
valid interrupt some time ago with the 2.4 kernel, all I can say is  
that while it can be made to work, things go much more smoothly if  
you don't use IRQ 0. Every driver added to the environment pretty  
much had to be tweaked. Of course that mainly meant adding to the  
#ifdef's that were already there for other architectures that had  
also made that mistake.

The biggest pain is admitting the mistake (of using IRQ 0) and  
changing it. Making a clear statement on the issue will help prevent  
others from making the same mistake again. I know that I wish that I  
had known not to do that from the beginning. Having been there and  
done that, I don't need any convincing.

-- 
Mark Rustad, MRustad@mac.com

Re: 2.6.18-mm2

[Alan Cox]

Ar Sad, 2006-09-30 am 10:26 -0500, ysgrifennodd James Bottomley:
> On Fri, 2006-09-29 at 23:50 +0000, Frederik Deweerdt wrote:
> > +       if (!pdev->irq)
> > +               return -ENODEV;
> > +
> 
> Don't I remember that 0 is a valid IRQ on some platforms?
> 
> i.e. shouldn't this be
> 
> if (pdev->irq == NO_IRQ)
> 	return -ENODEV;

NO_IRQ is gone. Everyone uses zero and Linus has declared that is how it
shall be.


Alan

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Jeff Garzik]

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

Frederik Deweerdt wrote:
> On Sat, Sep 30, 2006 at 12:43:24AM +0100, Alan Cox wrote:
>> Ar Gwe, 2006-09-29 am 23:50 +0000, ysgrifennodd Frederik Deweerdt:
>>> Does this patch makes sense in that case? If yes, I'll put up a patch
>>> for the remaining cases in the drivers/scsi/aic7xxx/ directory.
>>> Also, aic7xxx's coding style would put parenthesis around the returned
>>> value, should I follow it?
>> Yes - but perhaps with a warning message so users know why ?
>>
>> As to coding style - kernel style is unbracketed so I wouldnt worry
>> about either.
>>
> Thanks for the advices. 
> 
> The following patch checks whenever the irq is valid before issuing a
> request_irq() for AIC7XXX and AIC79XX. An error message is displayed to
> let the user know what went wrong.
> 
> Regards,
> Frederik
> 
> Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>

Actually, rather than adding this check to every driver, I would rather 
do something like the attached patch:  create a pci_request_irq(), and 
pass a struct pci_device to it.  Then the driver author doesn't have to 
worry about such details.

	Jeff



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

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a544997..9743471 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -809,6 +809,40 @@ err_out:
 	return -EBUSY;
 }
 
+#ifndef ARCH_VALIDATE_PCI_IRQ
+int pci_valid_irq(struct pci_dev *pdev)
+{
+	if (pdev->irq == 0)
+		return -EINVAL;
+	
+	return 0;
+}
+EXPORT_SYMBOL(pci_valid_irq);
+#endif /* ARCH_VALIDATE_PCI_IRQ */
+
+int pci_request_irq(struct pci_dev *pdev,
+		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
+		    unsigned long flags, const char *name, void *userdata)
+{
+	int rc;
+
+	rc = pci_valid_irq(pdev);
+	if (rc) {
+		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
+		return rc;
+	}
+
+	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
+			   name, userdata);
+}
+EXPORT_SYMBOL(pci_request_irq);
+
+void pci_release_irq(struct pci_dev *pdev, void *userdata)
+{
+	free_irq(pdev->irq, userdata);
+}
+EXPORT_SYMBOL(pci_release_irq);
+
 /**
  * pci_set_master - enables bus-mastering for device dev
  * @dev: the PCI device to enable
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5c3a417..5e254fc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -52,6 +52,7 @@ #include <linux/list.h>
 #include <linux/compiler.h>
 #include <linux/errno.h>
 #include <linux/device.h>
+#include <linux/interrupt.h>
 
 /* File state for mmap()s on /proc/bus/pci/X/Y */
 enum pci_mmap_state {
@@ -537,6 +538,12 @@ void pci_release_regions(struct pci_dev 
 int __must_check pci_request_region(struct pci_dev *, int, const char *);
 void pci_release_region(struct pci_dev *, int);
 
+int __must_check pci_valid_irq(struct pci_dev *pdev);
+int __must_check pci_request_irq(struct pci_dev *pdev,
+		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
+		    unsigned long flags, const char *name, void *userdata);
+void pci_release_irq(struct pci_dev *pdev, void *userdata);
+
 /* drivers/pci/bus.c */
 int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 			struct resource *res, resource_size_t size,

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Matthew Wilcox]

On Sat, Sep 30, 2006 at 07:58:18PM -0400, Jeff Garzik wrote:
> Actually, rather than adding this check to every driver, I would rather 
> do something like the attached patch:  create a pci_request_irq(), and 
> pass a struct pci_device to it.  Then the driver author doesn't have to 
> worry about such details.

I like pci_request_irq(), but pci_valid_irq is bad.

> +#ifndef ARCH_VALIDATE_PCI_IRQ
> +int pci_valid_irq(struct pci_dev *pdev)
> +{
> +	if (pdev->irq == 0)
> +		return -EINVAL;
> +	
> +	return 0;
> +}
> +EXPORT_SYMBOL(pci_valid_irq);
> +#endif /* ARCH_VALIDATE_PCI_IRQ */

Better would be:

#ifndef ARCH_VALIDATE_IRQ
static inline int valid_irq(unsigned int irq)
{
	return irq ? 1 : 0;
}
#endif

in linux/interrupt.h (around request_irq).

And it doesn't need to be a __must_check.  There's no point -- it has
no side-effects.  The only reason to call it is if you want the answer
to the question.  You had the sense of the return code wrong too; you
want to use it as:

int pci_request_irq(struct pci_dev *pdev, irq_handler_t handler,
			unsigned long flags, const char *name, void *data)
{
	if (!valid_irq(pdev->irq)) {
		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
		return -EINVAL;
	}

	return request_irq(pdev->irq, handler, flags | IRQF_SHARED, name, data);
}

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Arjan van de Ven]

> .
> 
> And it doesn't need to be a __must_check.  There's no point -- it has
> no side-effects.  The only reason to call it is if you want the answer
> to the question.  You had the sense of the return code wrong too; you
> want to use it as:
> 
> int pci_request_irq(struct pci_dev *pdev, irq_handler_t handler,
> 			unsigned long flags, const char *name, void *data)
> {
> 	if (!valid_irq(pdev->irq)) {
> 		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
> 		return -EINVAL;
> 	}
> 
> 	return request_irq(pdev->irq, handler, flags | IRQF_SHARED, name, data);
> }


well... why not go one step further and eliminate the flags argument
entirely? And use pci_name() for the name (so eliminate the argument ;)
and always pass pdev as data, so that that argument can go away too....

that'll cover 99% of the request_irq() users for pci devices.. and makes
it really nicely simple and consistent.

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Jeff Garzik]

Arjan van de Ven wrote:
> well... why not go one step further and eliminate the flags argument
> entirely? And use pci_name() for the name (so eliminate the argument ;)
> and always pass pdev as data, so that that argument can go away too....
> 
> that'll cover 99% of the request_irq() users for pci devices.. and makes
> it really nicely simple and consistent.

Disagree.  That would involve rewriting a lot of drivers.

flags: may or may not need sample-random flag.

name: is always the ethernet interface, for net drivers, or did you 
forget from your irqbalance days?  ;-)

data: in practice, is _rarely_ struct pci_dev.  It's usually a 
driver-private structure which is the structure most frequently 
accessed.  struct pci_dev* is rarely accessed inside the interrupt 
handler, except maybe somewhere deep in an error handling path.

	Jeff

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Arjan van de Ven]

On Sun, 2006-10-01 at 15:19 -0400, Jeff Garzik wrote:
> Arjan van de Ven wrote:
> > well... why not go one step further and eliminate the flags argument
> > entirely? And use pci_name() for the name (so eliminate the argument ;)
> > and always pass pdev as data, so that that argument can go away too....
> > 
> > that'll cover 99% of the request_irq() users for pci devices.. and makes
> > it really nicely simple and consistent.
> 
> Disagree.  That would involve rewriting a lot of drivers.
> 
> flags: may or may not need sample-random flag.

ok fair.. but I'd then almost call it "samplerandom" not "flags"...


> 
> name: is always the ethernet interface, for net drivers, or did you 
> forget from your irqbalance days?  ;-)

I'd say the "always" isn't quite true .. I remember that well.
If it's always the pci device at least irqbalance can look up the device
type in sysfs ;)


> data: in practice, is _rarely_ struct pci_dev.  It's usually a 
> driver-private structure which is the structure most frequently 
> accessed.  struct pci_dev* is rarely accessed inside the interrupt 
> handler, except maybe somewhere deep in an error handling path.

hmmm could put a pointer to the private data in the pci_dev at least...
that'd be generally useful, and then this can either just pass that,
or have the isr get to it that way (whichever makes more sense)

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Matthew Wilcox]

On Sun, Oct 01, 2006 at 09:05:23PM +0200, Arjan van de Ven wrote:
> > int pci_request_irq(struct pci_dev *pdev, irq_handler_t handler,
> > 			unsigned long flags, const char *name, void *data)
> > {
> > 	if (!valid_irq(pdev->irq)) {
> > 		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
> > 		return -EINVAL;
> > 	}
> > 
> > 	return request_irq(pdev->irq, handler, flags | IRQF_SHARED, name, data);
> > }
> 
> well... why not go one step further and eliminate the flags argument
> entirely? And use pci_name() for the name (so eliminate the argument ;)
> and always pass pdev as data, so that that argument can go away too....
> 
> that'll cover 99% of the request_irq() users for pci devices.. and makes
> it really nicely simple and consistent.

hmm.  $ echo `cut -c34- /proc/interrupts`
timer i8042 cascade acpi yenta, ehci_hcd:usb1, Intel 82801DB-ICH4 yenta,
uhci_hcd:usb2 uhci_hcd:usb4, eth0 ide0 uhci_hcd:usb3, eth1

Network drivers use their eth%d name.  USB drivers use [eu]hci_hcd:usb%d.
Others tend to use the driver name.  Changing them all to be 0000:00:1d.2
isn't really an improvement in the readability of /proc/interrupts, IMO.

Passing pdev as the data is a good idea for practically no device driver.
It's rare to actually want the pci_device down in the interrupt handler;
normally you want the device private data.  Using pci_get_drvdata(pdev)
as the data would make sense for both sym2 and tg3.  I don't feel like
auditing other drivers to see if it'd make sense for them too.

So, current proposal:

int pci_request_irq(struct pci_dev *pdev, irq_handler_t handler,
			const char *name)
{
	if (!valid_irq(pdev->irq)) {
		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
		return -EINVAL;
	}

	return request_irq(pdev->irq, handler, IRQF_SHARED, name,
				pci_get_drvdata(pdev));
}

But what about IRQF_SAMPLE_RANDOM?

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Jeff Garzik]

Matthew Wilcox wrote:
> Others tend to use the driver name.  Changing them all to be 0000:00:1d.2
> isn't really an improvement in the readability of /proc/interrupts, IMO.

agreed


> Passing pdev as the data is a good idea for practically no device driver.

agreed


> It's rare to actually want the pci_device down in the interrupt handler;
> normally you want the device private data.  Using pci_get_drvdata(pdev)
> as the data would make sense for both sym2 and tg3.  I don't feel like

Using pci_get_drvdata() is a pretty good idea


> int pci_request_irq(struct pci_dev *pdev, irq_handler_t handler,
> 			const char *name)
> {
> 	if (!valid_irq(pdev->irq)) {
> 		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
> 		return -EINVAL;
> 	}
> 
> 	return request_irq(pdev->irq, handler, IRQF_SHARED, name,
> 				pci_get_drvdata(pdev));
> }
> 
> But what about IRQF_SAMPLE_RANDOM?

I still like having a flags argument though.  It's enough of an open 
question, and I bet there will be a new flag or two in the future that 
PCI drivers will want to use.

	Jeff

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Frederik Deweerdt]

On Sat, Sep 30, 2006 at 07:58:18PM -0400, Jeff Garzik wrote:
> Frederik Deweerdt wrote:
> >On Sat, Sep 30, 2006 at 12:43:24AM +0100, Alan Cox wrote:
> >>Ar Gwe, 2006-09-29 am 23:50 +0000, ysgrifennodd Frederik Deweerdt:
> >>>Does this patch makes sense in that case? If yes, I'll put up a patch
> >>>for the remaining cases in the drivers/scsi/aic7xxx/ directory.
> >>>Also, aic7xxx's coding style would put parenthesis around the returned
> >>>value, should I follow it?
> >>Yes - but perhaps with a warning message so users know why ?
> >>
> >>As to coding style - kernel style is unbracketed so I wouldnt worry
> >>about either.
> >>
> >Thanks for the advices. The following patch checks whenever the irq is valid before issuing a
> >request_irq() for AIC7XXX and AIC79XX. An error message is displayed to
> >let the user know what went wrong.
> >Regards,
> >Frederik
> >Signed-off-by: Frederik Deweerdt <frederik.deweerdt@gmail.com>
> 
> Actually, rather than adding this check to every driver, I would rather do something like the attached patch:  create a 
> pci_request_irq(), and pass a struct pci_device to it.  Then the driver author doesn't have to worry about such details.
> 
That's better, indeed. 
[...]
> +#ifndef ARCH_VALIDATE_PCI_IRQ
> +int pci_valid_irq(struct pci_dev *pdev)
> +{
> +	if (pdev->irq == 0)
> +		return -EINVAL;
                        ^^^^^^
Woulnd't this rather be ENODEV? Admitedly, from pci_valid_irq() (or
is_irq_valid()) point of view, it _has_ been passed an invalid value. But
from userspace's point of view, it's like the device was not present.

Regards,
Frederik

Re: [-mm patch] aic7xxx: check irq validity (was Re: 2.6.18-mm2)

[Arjan van de Ven]

> Network drivers use their eth%d name.  USB drivers use [eu]hci_hcd:usb%d.
> Others tend to use the driver name.  Changing them all to be 0000:00:1d.2
> isn't really an improvement in the readability of /proc/interrupts, IMO.

hmm ok; how about allowing name to be NULL, and if it's NULL, use the
pci name?

> 
> So, current proposal:
> 
> int pci_request_irq(struct pci_dev *pdev, irq_handler_t handler,
> 			const char *name)
> {
> 	if (!valid_irq(pdev->irq)) {
> 		dev_printk(KERN_ERR, &pdev->dev, "invalid irq\n");
> 		return -EINVAL;
> 	}
> 
> 	return request_irq(pdev->irq, handler, IRQF_SHARED, name,
> 				pci_get_drvdata(pdev));
> }
> 
> But what about IRQF_SAMPLE_RANDOM?

that's a tough question. I'd almost suggest making such things
properties of the pdev, but sample-random is so far away from PCI
related that it makes no sense I suppose ;(

(others do I think)

One other interesting question is if this function can/should be used to
use MSI transparently (after pci_enable_msi() obviously)

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: [RFC PATCH] pci_request_irq (was [-mm patch] aic7xxx: check irq validity)

[Matthew Wilcox]

On Mon, Oct 02, 2006 at 08:00:48PM +0000, Frederik Deweerdt wrote:
>  /**
> + * pci_request_irq - Reserve an IRQ for a PCI device
> + * @pdev: The PCI device whose irq is to be reserved
> + * handler: The interrupt handler function,

> + * pci_get_drvdata(pdev) shall be passed as an argument to that function

I don't think you can (or should) do this.  Move it to the body of the
comment below.

> + * @flags: The flags to be passed to request_irq()
> + * @name: The name of the device to be associated with the irq
> + *
> + * Returns 0 on success, or a negative value on error.  A warning
> + * message is also printed on failure.
> + */
> +int pci_request_irq(struct pci_dev *pdev,
> +		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
> +		    unsigned long flags, const char *name)
> +{
> +	int rc;
> +	const char *actual_name = name;
> +
> +	rc = is_irq_valid(pdev->irq);
> +	if (!rc) {
> +		dev_printk(KERN_ERR, &pdev->dev, "invalid irq #%d\n", pdev->irq);
> +		return -EINVAL;
> +	}

Why is that more readable than

	if (!is_irq_valid(pdev->irq)) {
		dev_err(&pdev->dev, "invalid irq #%d\n", pdev->irq);
		return -EINVAL;
	}

> +	if (!actual_name)
> +		actual_name = pci_name(pdev);
> +
> +	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
> +			   actual_name, pci_get_drvdata(pdev));

The driver name is a far more common usage than the pci_name.

	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
			name ? name : pdev->driver->name,
			pci_get_drvdata(pdev));

Re: [RFC PATCH] move aic7xxx to pci_request_irq

[Matthew Wilcox]

On Mon, Oct 02, 2006 at 08:07:03PM +0000, Frederik Deweerdt wrote:
> +++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> @@ -341,12 +341,12 @@ ahd_pci_map_int(struct ahd_softc *ahd)
>  {
>  	int error;
>  
> -	error = request_irq(ahd->dev_softc->irq, ahd_linux_isr,
> -			    IRQF_SHARED, "aic79xx", ahd);
> +	error = pci_request_irq(ahd->dev_softc, ahd_linux_isr,
> +			    IRQF_SHARED, "aic79xx");
>  	if (!error)
>  		ahd->platform_data->irq = ahd->dev_softc->irq;
>  	
> -	return (-error);
> +	return error;

Seems unsafe to me.  Unless you want to trace through the whole driver
changing its internal conventions to use negative errnos like the rest
of the kernel.

> -	
> -	return (-error);
> -}
>  
> +	return error;
> +}

Ditto.

Re: [RFC PATCH] move tg3 to pci_request_irq

[Matthew Wilcox]

On Mon, Oct 02, 2006 at 08:11:34PM +0000, Frederik Deweerdt wrote:
> @@ -6838,9 +6838,9 @@ restart_timer:
>  
>  static int tg3_request_irq(struct tg3 *tp)
>  {
> +	struct net_device *dev = tp->dev;
>  	irqreturn_t (*fn)(int, void *, struct pt_regs *);
>  	unsigned long flags;
> -	struct net_device *dev = tp->dev;
>  
>  	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
>  		fn = tg3_msi;

Is there any reason for this noise?

Re: [RFC PATCH] move drm to pci_request_irq

[Matthew Wilcox]

On Mon, Oct 02, 2006 at 08:12:29PM +0000, Frederik Deweerdt wrote:
>  
> +	pci_set_drvdata(dev, NULL);
> +
>  	DRM_DEBUG("lastclose completed\n");

Not necessary.  pci_devs are allocated initialised to 0.

> @@ -132,8 +132,10 @@ static int drm_irq_install(drm_device_t 
>  	if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
>  		sh_flags = IRQF_SHARED;
>  
> -	ret = request_irq(dev->irq, dev->driver->irq_handler,
> -			  sh_flags, dev->devname, dev);
> +	pci_set_drvdata(dev->pdev, dev);
> +
> +	ret = pci_request_irq(dev->pdev, dev->driver->irq_handler,
> +			  sh_flags, dev->devname);

This seems like the wrong place to be setting the pci_drvdata.  It
should probably be done in each driver.  But then, requesting the IRQ
should also be done by each driver.  You've dragged us into the "wow,
what a mess DRI is" black hole here, I'm afraid.

[RFC PATCH] pci_request_irq (was [-mm patch] aic7xxx: check irq validity)

[Frederik Deweerdt]

Hi all,

I've tried to summarize the different proposals made by Jeff Garzik,
Matthew Wilcox and Arjan van de Ven in the "[-mm patch] aic7xxx: check
irq validity" thread. I've also added:
- some kerneldoc
- renamed valid_irq to is_irq_valid() 
- added pci_release_irq(). 

I'll send a follow-up patch showing the implied modifications for the
following - semi-randomly chosen :) - drivers: aic7xxx, aic79xx, tg3
and drm.

Regards,
Frederik

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a544997..ae20a3a 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -15,6 +15,7 @@ #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/spinlock.h>
+#include <linux/interrupt.h>
 #include <linux/string.h>
 #include <asm/dma.h>	/* isa_dma_bridge_buggy */
 #include "pci.h"
@@ -810,6 +811,49 @@ err_out:
 }
 
 /**
+ * pci_request_irq - Reserve an IRQ for a PCI device
+ * @pdev: The PCI device whose irq is to be reserved
+ * handler: The interrupt handler function,
+ * pci_get_drvdata(pdev) shall be passed as an argument to that function
+ * @flags: The flags to be passed to request_irq()
+ * @name: The name of the device to be associated with the irq
+ *
+ * Returns 0 on success, or a negative value on error.  A warning
+ * message is also printed on failure.
+ */
+int pci_request_irq(struct pci_dev *pdev,
+		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
+		    unsigned long flags, const char *name)
+{
+	int rc;
+	const char *actual_name = name;
+
+	rc = is_irq_valid(pdev->irq);
+	if (!rc) {
+		dev_printk(KERN_ERR, &pdev->dev, "invalid irq #%d\n", pdev->irq);
+		return -EINVAL;
+	}
+
+	if (!actual_name)
+		actual_name = pci_name(pdev);
+
+	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
+			   actual_name, pci_get_drvdata(pdev));
+}
+EXPORT_SYMBOL(pci_request_irq);
+
+/**
+ * pci_free_irq - releases the interrupt line reserved to the PCI
+ * device pointed by @pdev 
+ * @pdev: the PCI device whose interrupt is to be freed
+ */
+void pci_free_irq(struct pci_dev *pdev)
+{
+	free_irq(pdev->irq, pci_get_drvdata(pdev));
+}
+EXPORT_SYMBOL(pci_free_irq);
+
+/**
  * pci_set_master - enables bus-mastering for device dev
  * @dev: the PCI device to enable
  *
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 1f97e3d..c320b50 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -75,6 +75,13 @@ struct irqaction {
 	struct proc_dir_entry *dir;
 };
 
+#ifndef ARCH_VALIDATE_PCI_IRQ
+static inline int is_irq_valid(unsigned int irq)
+{
+	return irq ? 1 : 0;
+}
+#endif /* ARCH_VALIDATE_PCI_IRQ */
+
 extern irqreturn_t no_action(int cpl, void *dev_id, struct pt_regs *regs);
 extern int request_irq(unsigned int,
 		       irqreturn_t (*handler)(int, void *, struct pt_regs *),
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5bc4659..5e0f07a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -52,6 +52,7 @@ #include <linux/list.h>
 #include <linux/compiler.h>
 #include <linux/errno.h>
 #include <linux/device.h>
+#include <linux/interrupt.h>
 
 /* File state for mmap()s on /proc/bus/pci/X/Y */
 enum pci_mmap_state {
@@ -531,6 +532,11 @@ void pci_release_regions(struct pci_dev 
 int __must_check pci_request_region(struct pci_dev *, int, const char *);
 void pci_release_region(struct pci_dev *, int);
 
+int __must_check pci_request_irq(struct pci_dev *pdev,
+		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
+		    unsigned long flags, const char *name);
+void pci_free_irq(struct pci_dev *pdev);
+
 /* drivers/pci/bus.c */
 int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 			struct resource *res, resource_size_t size,

[RFC PATCH] move aic7xxx to pci_request_irq

[Frederik Deweerdt]

Hi,

This proof-of-concept patch converts the aic7xxx drivers to use the
pci_request_irq() function.

Regards,
Frederik


diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
index 2001fe8..c934f30 100644
--- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
@@ -341,12 +341,12 @@ ahd_pci_map_int(struct ahd_softc *ahd)
 {
 	int error;
 
-	error = request_irq(ahd->dev_softc->irq, ahd_linux_isr,
-			    IRQF_SHARED, "aic79xx", ahd);
+	error = pci_request_irq(ahd->dev_softc, ahd_linux_isr,
+			    IRQF_SHARED, "aic79xx");
 	if (!error)
 		ahd->platform_data->irq = ahd->dev_softc->irq;
 	
-	return (-error);
+	return error;
 }
 
 void
diff --git a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
index ea5687d..d5c402e 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_osm_pci.c
@@ -368,16 +368,14 @@ ahc_pci_map_registers(struct ahc_softc *
 	return (error);
 }
 
-int
-ahc_pci_map_int(struct ahc_softc *ahc)
+int ahc_pci_map_int(struct ahc_softc *ahc)
 {
 	int error;
 
-	error = request_irq(ahc->dev_softc->irq, ahc_linux_isr,
-			    IRQF_SHARED, "aic7xxx", ahc);
+	error = pci_request_irq(ahc->dev_softc, ahc_linux_isr, IRQF_SHARED,
+			    	"aic7xxx");
 	if (error == 0)
 		ahc->platform_data->irq = ahc->dev_softc->irq;
-	
-	return (-error);
-}
 
+	return error;
+}

[RFC PATCH] move tg3 to pci_request_irq

[Frederik Deweerdt]

Hi,

This proof-of-concept patch converts the tg3 driver to use the
pci_request_irq() function.

Regards,
Frederik


diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index c25ba27..23660c6 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -6838,9 +6838,9 @@ restart_timer:
 
 static int tg3_request_irq(struct tg3 *tp)
 {
+	struct net_device *dev = tp->dev;
 	irqreturn_t (*fn)(int, void *, struct pt_regs *);
 	unsigned long flags;
-	struct net_device *dev = tp->dev;
 
 	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
 		fn = tg3_msi;
@@ -6853,7 +6853,7 @@ static int tg3_request_irq(struct tg3 *t
 			fn = tg3_interrupt_tagged;
 		flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
 	}
-	return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
+	return pci_request_irq(tp->pdev, fn, flags, dev->name);
 }
 
 static int tg3_test_interrupt(struct tg3 *tp)
@@ -6866,10 +6866,10 @@ static int tg3_test_interrupt(struct tg3
 
 	tg3_disable_ints(tp);
 
-	free_irq(tp->pdev->irq, dev);
+	pci_free_irq(tp->pdev);
 
-	err = request_irq(tp->pdev->irq, tg3_test_isr,
-			  IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name, dev);
+	err = pci_request_irq(tp->pdev, tg3_test_isr, 
+			      IRQF_SHARED | IRQF_SAMPLE_RANDOM, dev->name);
 	if (err)
 		return err;
 
@@ -6897,7 +6897,7 @@ static int tg3_test_interrupt(struct tg3
 
 	tg3_disable_ints(tp);
 
-	free_irq(tp->pdev->irq, dev);
+	pci_free_irq(tp->pdev);
 
 	err = tg3_request_irq(tp);
 
@@ -6915,7 +6915,6 @@ static int tg3_test_interrupt(struct tg3
  */
 static int tg3_test_msi(struct tg3 *tp)
 {
-	struct net_device *dev = tp->dev;
 	int err;
 	u16 pci_cmd;
 
@@ -6946,7 +6945,7 @@ static int tg3_test_msi(struct tg3 *tp)
 	       "the PCI maintainer and include system chipset information.\n",
 		       tp->dev->name);
 
-	free_irq(tp->pdev->irq, dev);
+	pci_free_irq(tp->pdev);
 	pci_disable_msi(tp->pdev);
 
 	tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
@@ -6966,7 +6965,7 @@ static int tg3_test_msi(struct tg3 *tp)
 	tg3_full_unlock(tp);
 
 	if (err)
-		free_irq(tp->pdev->irq, dev);
+		pci_free_irq(tp->pdev);
 
 	return err;
 }
@@ -7051,7 +7050,7 @@ static int tg3_open(struct net_device *d
 	tg3_full_unlock(tp);
 
 	if (err) {
-		free_irq(tp->pdev->irq, dev);
+		pci_free_irq(tp->pdev);
 		if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
 			pci_disable_msi(tp->pdev);
 			tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;
@@ -7363,7 +7362,7 @@ #endif
 
 	tg3_full_unlock(tp);
 
-	free_irq(tp->pdev->irq, dev);
+	pci_free_irq(tp->pdev);
 	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
 		pci_disable_msi(tp->pdev);
 		tp->tg3_flags2 &= ~TG3_FLG2_USING_MSI;

[RFC PATCH] move drm to pci_request_irq

[Frederik Deweerdt]

Hi,

This proof-of-concept patch converts the drm driver to use the
pci_request_irq() function.

Regards,
Frederik



diff --git a/drivers/char/drm/drm_drv.c b/drivers/char/drm/drm_drv.c
index b366c5b..5b000cd 100644
--- a/drivers/char/drm/drm_drv.c
+++ b/drivers/char/drm/drm_drv.c
@@ -234,6 +234,8 @@ int drm_lastclose(drm_device_t * dev)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	pci_set_drvdata(dev, NULL);
+
 	DRM_DEBUG("lastclose completed\n");
 	return 0;
 }
diff --git a/drivers/char/drm/drm_irq.c b/drivers/char/drm/drm_irq.c
index 4553a3a..5dd12cb 100644
--- a/drivers/char/drm/drm_irq.c
+++ b/drivers/char/drm/drm_irq.c
@@ -132,8 +132,10 @@ static int drm_irq_install(drm_device_t 
 	if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
 		sh_flags = IRQF_SHARED;
 
-	ret = request_irq(dev->irq, dev->driver->irq_handler,
-			  sh_flags, dev->devname, dev);
+	pci_set_drvdata(dev->pdev, dev);
+
+	ret = pci_request_irq(dev->pdev, dev->driver->irq_handler,
+			  sh_flags, dev->devname);
 	if (ret < 0) {
 		mutex_lock(&dev->struct_mutex);
 		dev->irq_enabled = 0;
@@ -173,7 +175,7 @@ int drm_irq_uninstall(drm_device_t * dev
 
 	dev->driver->irq_uninstall(dev);
 
-	free_irq(dev->irq, dev);
+	pci_free_irq(dev->pdev);
 
 	return 0;
 }

Re: [RFC PATCH] move drm to pci_request_irq

[Alan Cox]

Ar Llu, 2006-10-02 am 20:12 +0000, ysgrifennodd Frederik Deweerdt:
> Hi,
> 
> This proof-of-concept patch converts the drm driver to use the
> pci_request_irq() function.

0 isn't invalid - it means no IRQ was assigned so wants a different
message.

Re: [RFC PATCH] move aic7xxx to pci_request_irq

[Frederik Deweerdt]

On Mon, Oct 02, 2006 at 12:27:44PM -0600, Matthew Wilcox wrote:
> On Mon, Oct 02, 2006 at 08:07:03PM +0000, Frederik Deweerdt wrote:
> > +++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> > @@ -341,12 +341,12 @@ ahd_pci_map_int(struct ahd_softc *ahd)
> >  {
> >  	int error;
> >  
> > -	error = request_irq(ahd->dev_softc->irq, ahd_linux_isr,
> > -			    IRQF_SHARED, "aic79xx", ahd);
> > +	error = pci_request_irq(ahd->dev_softc, ahd_linux_isr,
> > +			    IRQF_SHARED, "aic79xx");
> >  	if (!error)
> >  		ahd->platform_data->irq = ahd->dev_softc->irq;
> >  	
> > -	return (-error);
> > +	return error;
> 
> Seems unsafe to me.
It is, it slipped through the patches, I didn't mean to send it to the
list :(. Please ignore that.
> 
> > -	
> > -	return (-error);
> > -}
> >  
> > +	return error;
> > +}
> 
> Ditto.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH] move tg3 to pci_request_irq

[Frederik Deweerdt]

On Mon, Oct 02, 2006 at 12:28:47PM -0600, Matthew Wilcox wrote:
> On Mon, Oct 02, 2006 at 08:11:34PM +0000, Frederik Deweerdt wrote:
> > @@ -6838,9 +6838,9 @@ restart_timer:
> >  
> >  static int tg3_request_irq(struct tg3 *tp)
> >  {
> > +	struct net_device *dev = tp->dev;
> >  	irqreturn_t (*fn)(int, void *, struct pt_regs *);
> >  	unsigned long flags;
> > -	struct net_device *dev = tp->dev;
> >  
> >  	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
> >  		fn = tg3_msi;
> 
> Is there any reason for this noise?
You mean, besides my awkwardness ? ;)
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH] move drm to pci_request_irq

[Frederik Deweerdt]

On Mon, Oct 02, 2006 at 12:37:49PM -0600, Matthew Wilcox wrote:
> On Mon, Oct 02, 2006 at 08:12:29PM +0000, Frederik Deweerdt wrote:
> >  
> > +	pci_set_drvdata(dev, NULL);
> > +
> >  	DRM_DEBUG("lastclose completed\n");
> 
> Not necessary.  pci_devs are allocated initialised to 0.
Actually, this is the exit path, I felt like it could be safer if it was
set to NULL before freeing it.
> 
> > @@ -132,8 +132,10 @@ static int drm_irq_install(drm_device_t 
> >  	if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
> >  		sh_flags = IRQF_SHARED;
> >  
> > -	ret = request_irq(dev->irq, dev->driver->irq_handler,
> > -			  sh_flags, dev->devname, dev);
> > +	pci_set_drvdata(dev->pdev, dev);
> > +
> > +	ret = pci_request_irq(dev->pdev, dev->driver->irq_handler,
> > +			  sh_flags, dev->devname);
> 
> This seems like the wrong place to be setting the pci_drvdata.  It
> should probably be done in each driver.  But then, requesting the IRQ
> should also be done by each driver.  You've dragged us into the "wow,
> what a mess DRI is" black hole here, I'm afraid.
I must admit that I had no idea where to initialize it. Do you have a
better place in mind?
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH] pci_request_irq (was [-mm patch] aic7xxx: check irq validity)

[Frederik Deweerdt]

On Mon, Oct 02, 2006 at 12:15:22PM -0600, Matthew Wilcox wrote:
> On Mon, Oct 02, 2006 at 08:00:48PM +0000, Frederik Deweerdt wrote:
> >  /**
> > + * pci_request_irq - Reserve an IRQ for a PCI device
> > + * @pdev: The PCI device whose irq is to be reserved
> > + * handler: The interrupt handler function,
> 
> > + * pci_get_drvdata(pdev) shall be passed as an argument to that function
> 
> I don't think you can (or should) do this.  Move it to the body of the
> comment below.
OK, thanks for pointing this, will do.
> 
> > + * @flags: The flags to be passed to request_irq()
> > + * @name: The name of the device to be associated with the irq
> > + *
> > + * Returns 0 on success, or a negative value on error.  A warning
> > + * message is also printed on failure.
> > + */
> > +int pci_request_irq(struct pci_dev *pdev,
> > +		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
> > +		    unsigned long flags, const char *name)
> > +{
> > +	int rc;
> > +	const char *actual_name = name;
> > +
> > +	rc = is_irq_valid(pdev->irq);
> > +	if (!rc) {
> > +		dev_printk(KERN_ERR, &pdev->dev, "invalid irq #%d\n", pdev->irq);
> > +		return -EINVAL;
> > +	}
> 
> Why is that more readable than
> 
> 	if (!is_irq_valid(pdev->irq)) {
> 		dev_err(&pdev->dev, "invalid irq #%d\n", pdev->irq);
> 		return -EINVAL;
> 	}
> 
Better too.
> > +	if (!actual_name)
> > +		actual_name = pci_name(pdev);
> > +
> > +	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
> > +			   actual_name, pci_get_drvdata(pdev));
> 
> The driver name is a far more common usage than the pci_name.
> 
> 	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
> 			name ? name : pdev->driver->name,
> 			pci_get_drvdata(pdev));
OK, thanks for the feedback,
Frederik
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH] move drm to pci_request_irq

[Frederik Deweerdt]

On Mon, Oct 02, 2006 at 09:36:38PM +0100, Alan Cox wrote:
> Ar Llu, 2006-10-02 am 20:12 +0000, ysgrifennodd Frederik Deweerdt:
> > Hi,
> > 
> > This proof-of-concept patch converts the drm driver to use the
> > pci_request_irq() function.
> 
> 0 isn't invalid - it means no IRQ was assigned so wants a different
> message.
> 
I understand, what about:

("No usable irq line was found (got #%d)\n", irqno)

This is generic enough, so that if on some arches a given irq (other
than 0) is invalid, the message still makes sense.

Re: [RFC PATCH] move drm to pci_request_irq

[Dave Airlie]

On 10/3/06, Frederik Deweerdt <deweerdt@free.fr> wrote:
> Hi,
>
> This proof-of-concept patch converts the drm driver to use the
> pci_request_irq() function.

NAK.
Wow nice CC'list and no DRM maintainer in sight :-)

This will break framebuffer drivers, the DRM is not a proper PCI
device driver as we don't have PCI device sharing, take a look at the
gpu-2.6.git tree on kernel.org for the "correct" solution, which needs
more attention before merging..

Dave.
>
> Regards,
> Frederik
>
>
>
> diff --git a/drivers/char/drm/drm_drv.c b/drivers/char/drm/drm_drv.c
> index b366c5b..5b000cd 100644
> --- a/drivers/char/drm/drm_drv.c
> +++ b/drivers/char/drm/drm_drv.c
> @@ -234,6 +234,8 @@ int drm_lastclose(drm_device_t * dev)
>         }
>         mutex_unlock(&dev->struct_mutex);
>
> +       pci_set_drvdata(dev, NULL);
> +
>         DRM_DEBUG("lastclose completed\n");
>         return 0;
>  }
> diff --git a/drivers/char/drm/drm_irq.c b/drivers/char/drm/drm_irq.c
> index 4553a3a..5dd12cb 100644
> --- a/drivers/char/drm/drm_irq.c
> +++ b/drivers/char/drm/drm_irq.c
> @@ -132,8 +132,10 @@ static int drm_irq_install(drm_device_t
>         if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
>                 sh_flags = IRQF_SHARED;
>
> -       ret = request_irq(dev->irq, dev->driver->irq_handler,
> -                         sh_flags, dev->devname, dev);
> +       pci_set_drvdata(dev->pdev, dev);
> +
> +       ret = pci_request_irq(dev->pdev, dev->driver->irq_handler,
> +                         sh_flags, dev->devname);
>         if (ret < 0) {
>                 mutex_lock(&dev->struct_mutex);
>                 dev->irq_enabled = 0;
> @@ -173,7 +175,7 @@ int drm_irq_uninstall(drm_device_t * dev
>
>         dev->driver->irq_uninstall(dev);
>
> -       free_irq(dev->irq, dev);
> +       pci_free_irq(dev->pdev);
>
>         return 0;
>  }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [RFC PATCH] move aic7xxx to pci_request_irq

[Arjan van de Ven]

On Mon, 2006-10-02 at 20:07 +0000, Frederik Deweerdt wrote:
> Hi,
> 
> This proof-of-concept patch converts the aic7xxx drivers to use the
> pci_request_irq() function.
> 
> Regards,
> Frederik
> 
> 
> diff --git a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> index 2001fe8..c934f30 100644
> --- a/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> +++ b/drivers/scsi/aic7xxx/aic79xx_osm_pci.c
> @@ -341,12 +341,12 @@ ahd_pci_map_int(struct ahd_softc *ahd)
>  {
>  	int error;
>  
> -	error = request_irq(ahd->dev_softc->irq, ahd_linux_isr,
> -			    IRQF_SHARED, "aic79xx", ahd);
> +	error = pci_request_irq(ahd->dev_softc, ahd_linux_isr,
> +			    IRQF_SHARED, "aic79xx");
>  	if (!error)
>  		ahd->platform_data->irq = ahd->dev_softc->irq;
>  	
> -	return (-error);
> +	return error;
>  }

might as well kill this entire wrapper...

Re: [RFC PATCH] pci_request_irq (was [-mm patch] aic7xxx: check irq validity)

[Randy Dunlap]

On Mon, 2 Oct 2006 20:00:48 +0000 Frederik Deweerdt wrote:

> Hi all,
> 
> I've tried to summarize the different proposals made by Jeff Garzik,
> Matthew Wilcox and Arjan van de Ven in the "[-mm patch] aic7xxx: check
> irq validity" thread. I've also added:
> - some kerneldoc

The kernel-doc needs some repair -- see below.

> - renamed valid_irq to is_irq_valid() 
> - added pci_release_irq(). 
> 
> I'll send a follow-up patch showing the implied modifications for the
> following - semi-randomly chosen :) - drivers: aic7xxx, aic79xx, tg3
> and drm.
> 
> Regards,
> Frederik
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a544997..ae20a3a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -15,6 +15,7 @@ #include <linux/init.h>
>  #include <linux/pci.h>
>  #include <linux/module.h>
>  #include <linux/spinlock.h>
> +#include <linux/interrupt.h>
>  #include <linux/string.h>
>  #include <asm/dma.h>	/* isa_dma_bridge_buggy */
>  #include "pci.h"
> @@ -810,6 +811,49 @@ err_out:
>  }
>  
>  /**
> + * pci_request_irq - Reserve an IRQ for a PCI device
> + * @pdev: The PCI device whose irq is to be reserved
> + * handler: The interrupt handler function,

 * @handler: ...

> + * pci_get_drvdata(pdev) shall be passed as an argument to that function
> + * @flags: The flags to be passed to request_irq()
> + * @name: The name of the device to be associated with the irq
> + *
> + * Returns 0 on success, or a negative value on error.  A warning
> + * message is also printed on failure.
> + */
> +int pci_request_irq(struct pci_dev *pdev,
> +		    irqreturn_t (*handler)(int, void *, struct pt_regs *),
> +		    unsigned long flags, const char *name)
> +{
> +	int rc;
> +	const char *actual_name = name;
> +
> +	rc = is_irq_valid(pdev->irq);
> +	if (!rc) {
> +		dev_printk(KERN_ERR, &pdev->dev, "invalid irq #%d\n", pdev->irq);
> +		return -EINVAL;
> +	}
> +
> +	if (!actual_name)
> +		actual_name = pci_name(pdev);
> +
> +	return request_irq(pdev->irq, handler, flags | IRQF_SHARED,
> +			   actual_name, pci_get_drvdata(pdev));
> +}
> +EXPORT_SYMBOL(pci_request_irq);
> +
> +/**
> + * pci_free_irq - releases the interrupt line reserved to the PCI
> + * device pointed by @pdev 

The first line is function name and <<short>> function description.
It cannot extend more than one line (combined).
If you want to use more text for function description,
you can do so after the list of parameters.  See example below.

> + * @pdev: the PCI device whose interrupt is to be freed
 *
 * This froofroo_irq function only does this on odd phases of
 * the moon.

> + */
> +void pci_free_irq(struct pci_dev *pdev)
> +{
> +	free_irq(pdev->irq, pci_get_drvdata(pdev));
> +}
> +EXPORT_SYMBOL(pci_free_irq);
> +
> +/**
>   * pci_set_master - enables bus-mastering for device dev
>   * @dev: the PCI device to enable
>   *

---
~Randy

Re: [RFC PATCH] move drm to pci_request_irq

[Frederik Deweerdt]

On Tue, Oct 03, 2006 at 09:54:07AM +1000, Dave Airlie wrote:
> On 10/3/06, Frederik Deweerdt <deweerdt@free.fr> wrote:
> >Hi,
> >
> >This proof-of-concept patch converts the drm driver to use the
> >pci_request_irq() function.
> 
> NAK.
> Wow nice CC'list and no DRM maintainer in sight :-)
:), this was just meant as an illustration of the needed modifications
to use pci_request_irq.
> 
> This will break framebuffer drivers, the DRM is not a proper PCI
> device driver as we don't have PCI device sharing, take a look at the
> gpu-2.6.git tree on kernel.org for the "correct" solution, which needs
> more attention before merging..
I'll look, thanks,
Frederik
> 
> Dave.
> >
> >Regards,
> >Frederik
> >
> >
> >
> >diff --git a/drivers/char/drm/drm_drv.c b/drivers/char/drm/drm_drv.c
> >index b366c5b..5b000cd 100644
> >--- a/drivers/char/drm/drm_drv.c
> >+++ b/drivers/char/drm/drm_drv.c
> >@@ -234,6 +234,8 @@ int drm_lastclose(drm_device_t * dev)
> >        }
> >        mutex_unlock(&dev->struct_mutex);
> >
> >+       pci_set_drvdata(dev, NULL);
> >+
> >        DRM_DEBUG("lastclose completed\n");
> >        return 0;
> > }
> >diff --git a/drivers/char/drm/drm_irq.c b/drivers/char/drm/drm_irq.c
> >index 4553a3a..5dd12cb 100644
> >--- a/drivers/char/drm/drm_irq.c
> >+++ b/drivers/char/drm/drm_irq.c
> >@@ -132,8 +132,10 @@ static int drm_irq_install(drm_device_t
> >        if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
> >                sh_flags = IRQF_SHARED;
> >
> >-       ret = request_irq(dev->irq, dev->driver->irq_handler,
> >-                         sh_flags, dev->devname, dev);
> >+       pci_set_drvdata(dev->pdev, dev);
> >+
> >+       ret = pci_request_irq(dev->pdev, dev->driver->irq_handler,
> >+                         sh_flags, dev->devname);
> >        if (ret < 0) {
> >                mutex_lock(&dev->struct_mutex);
> >                dev->irq_enabled = 0;
> >@@ -173,7 +175,7 @@ int drm_irq_uninstall(drm_device_t * dev
> >
> >        dev->driver->irq_uninstall(dev);
> >
> >-       free_irq(dev->irq, dev);
> >+       pci_free_irq(dev->pdev);
> >
> >        return 0;
> > }
> >-
> >To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >Please read the FAQ at  http://www.tux.org/lkml/
> >
> 

Re: [RFC PATCH] move tg3 to pci_request_irq

[Arjan van de Ven]

On Mon, 2006-10-02 at 20:11 +0000, Frederik Deweerdt wrote:
> Hi,
> 
> This proof-of-concept patch converts the tg3 driver to use the
> pci_request_irq() function.
> 
> Regards,
> Frederik
> 
> 
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index c25ba27..23660c6 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -6838,9 +6838,9 @@ restart_timer:
>  
>  static int tg3_request_irq(struct tg3 *tp)
>  {
> +	struct net_device *dev = tp->dev;
>  	irqreturn_t (*fn)(int, void *, struct pt_regs *);
>  	unsigned long flags;
> -	struct net_device *dev = tp->dev;
>  
>  	if (tp->tg3_flags2 & TG3_FLG2_USING_MSI) {
>  		fn = tg3_msi;
> @@ -6853,7 +6853,7 @@ static int tg3_request_irq(struct tg3 *t
>  			fn = tg3_interrupt_tagged;
>  		flags = IRQF_SHARED | IRQF_SAMPLE_RANDOM;
>  	}
> -	return (request_irq(tp->pdev->irq, fn, flags, dev->name, dev));
> +	return pci_request_irq(tp->pdev, fn, flags, dev->name);

since pci_request_irq sets IRQF_SHARED... might as well drop that above.