Re: [PATCH] PCI: Fix pcibios_update_irq misuse of irq number

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Russell,

On 02/02/15 16:33, Russell King - ARM Linux wrote:
> On Wed, Jan 28, 2015 at 02:51:23PM +0000, Marc Zyngier wrote:
>>  void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
>>  {
>> -	dev_dbg(&dev->dev, "assigning IRQ %02d\n", irq);
>> -	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>> +	struct irq_data *d;
>> +
>> +	d = irq_get_irq_data(irq);
>> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>> +	while (d->parent_data)
>> +		d = d->parent_data;
>> +#endif
>> +	dev_dbg(&dev->dev, "assigning IRQ %02ld\n", d->hwirq);
>> +	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, d->hwirq);
> 
> I'm really not convinced about this being the correct thing to do.
> 
> Let's take an older ARM system, such as a Footbridge based system with a
> PCI southbridge.
> 
> Such a system has IRQs 0-15 as the PCI southbridge ISA interrupts.  Then
> there are four PCI interrupts provided by the on-board Footbridge.
> 
> Right now, PCI devices are programmed with the OS specific interrupt
> number - eg:
> 
> 00:06.1 IDE interface: Contaq Microsystems 82c693 (prog-if 80 [Master])
>         Flags: medium devsel, IRQ 14
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0e 01 00 00
> 
> 00:06.2 IDE interface: Contaq Microsystems 82c693 (prog-if 00 [])
>         Flags: medium devsel, IRQ 15
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0f 02 00 00
> 
> 00:06.3 USB Controller: Contaq Microsystems 82c693 (prog-if 10 [OHCI])
>         Flags: medium devsel, IRQ 12
> 30: 00 00 00 00 00 00 00 00 00 00 00 00 0c 01 00 00
> 
> 00:07.0 Mass storage controller: Integrated Technology Express, Inc. IT/ITE8212
> Dual channel ATA RAID controller (rev 13)
>         Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 24
> 30: 00 00 02 04 80 00 00 00 00 00 00 00 18 01 08 08
> 
> 00:08.0 Ethernet controller: 3Com Corporation 3c905C-TX/TX-M [Tornado] (rev 74)
>         Flags: bus master, medium devsel, latency 32, IRQ 22
> 30: 00 00 06 04 dc 00 00 00 00 00 00 00 16 01 0a 0a
> 
> 00:09.0 VGA compatible controller: S3 Inc. 86c775/86c785 [Trio 64V2/DX or /GX] (rev 16) (prog-if 00 [VGA controller])
>         Flags: medium devsel, IRQ 21
> 30: 00 00 00 0c 00 00 00 00 00 00 00 00 15 01 00 00
> 
> What your change would mean is that the IRQs currently being programmed
>> = 16 would be programmed into with numbers with 16 removed from them.
> This means that legacy stuff (eg on the Southbridge which really do signal
> via the ISA IRQ controller) end up using the same number range as those
> which take PCI specific IRQs.
> 
> This surely is not sane.

I suppose this is ebsa285? I must confess I don't see how to distinguish
the two cases (the GIC case uses a purely virtual number, and the
footbridge case uses something that seems to be physical).

A very easy fix would be to entirely contain this change within #ifdef
CONFIG_IRQ_DOMAIN_HIERARCHY/#endif, but that doesn't fill me with
confidence.

What I don't get is how the hwirq field is set in this case. It probably
isn't very useful (as there is no domain lookup), so I would have hoped
to see irq == hwirq. Obviously, this is not the case. What am I missing?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...