[PATCH] [SERIAL] add TP560 data/fax/modem support

[PATCH] [SERIAL] add TP560 data/fax/modem support

[Bjorn Helgaas]

Claim Topic TP560 data/fax/voice modem.  This device reports as class 0x0780,
so we don't claim it by default:

	00:0d.0 Class 0780: 151f:0000
		Subsystem: 151f:0000
		Interrupt: pin A routed to IRQ 11
		Region 0: I/O ports at a400 [size=8]
	00: 1f 15 00 00 01 00 00 02 00 00 80 07 00 00 00 00
	10: 01 a4 00 00 00 00 00 00 00 00 00 00 00 00 00 00
	20: 00 00 00 00 00 00 00 00 00 00 00 00 1f 15 00 00
	30: 00 00 00 00 00 00 00 00 00 00 00 00 0b 01 00 00

Some rc.serial scripts extract IRQ and I/O port information from
/proc/pci and stuff it into an unused port using setserial.  That
doesn't work reliably anymore because pci_enable_device() is never
called, so the IRQ may not be enabled.

Thanks to Evan Clarke for reporting and helping debug this problem.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

===== drivers/serial/8250_pci.c 1.48 vs edited =====
--- 1.48/drivers/serial/8250_pci.c	2004-11-21 23:42:29 -07:00
+++ edited/drivers/serial/8250_pci.c	2005-02-07 12:00:32 -07:00
@@ -2212,6 +2212,13 @@
 		0, pbn_exar_XR17C158 },
 
 	/*
+	 * Topic TP560 Data/Fax/Voice 56k modem (reported by Evan Clarke)
+	 */
+	{	PCI_VENDOR_ID_TOPIC, PCI_DEVICE_ID_TOPIC_TP560,
+		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+		pbn_b0_1_115200 },
+
+	/*
 	 * These entries match devices with class COMMUNICATION_SERIAL,
 	 * COMMUNICATION_MODEM or COMMUNICATION_MULTISERIAL
 	 */
===== include/linux/pci_ids.h 1.200 vs edited =====
--- 1.200/include/linux/pci_ids.h	2005-01-30 23:33:43 -07:00
+++ edited/include/linux/pci_ids.h	2005-02-07 11:56:14 -07:00
@@ -1972,6 +1972,9 @@
 #define PCI_DEVICE_ID_BCM4401		0x4401
 #define PCI_DEVICE_ID_BCM4401B0		0x4402
 
+#define PCI_VENDOR_ID_TOPIC		0x151f
+#define PCI_DEVICE_ID_TOPIC_TP560	0x0000
+
 #define PCI_VENDOR_ID_ENE		0x1524
 #define PCI_DEVICE_ID_ENE_1211		0x1211
 #define PCI_DEVICE_ID_ENE_1225		0x1225

Re: [PATCH] [SERIAL] add TP560 data/fax/modem support

[linux-os]

I thought somebody promised to add a pci_route_irq(dev) or some
such so that the device didn't have to be enabled before
the IRQ was correct.

I first reported this bad IRQ problem back in December of 2004.
Has the new function been added?


On Mon, 7 Feb 2005, Bjorn Helgaas wrote:

> Claim Topic TP560 data/fax/voice modem.  This device reports as class 0x0780,
> so we don't claim it by default:
>
> 	00:0d.0 Class 0780: 151f:0000
> 		Subsystem: 151f:0000
> 		Interrupt: pin A routed to IRQ 11
> 		Region 0: I/O ports at a400 [size=8]
> 	00: 1f 15 00 00 01 00 00 02 00 00 80 07 00 00 00 00
> 	10: 01 a4 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 	20: 00 00 00 00 00 00 00 00 00 00 00 00 1f 15 00 00
> 	30: 00 00 00 00 00 00 00 00 00 00 00 00 0b 01 00 00
>
> Some rc.serial scripts extract IRQ and I/O port information from
> /proc/pci and stuff it into an unused port using setserial.  That
> doesn't work reliably anymore because pci_enable_device() is never
> called, so the IRQ may not be enabled.
>
> Thanks to Evan Clarke for reporting and helping debug this problem.
>
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
>
> ===== drivers/serial/8250_pci.c 1.48 vs edited =====
> --- 1.48/drivers/serial/8250_pci.c	2004-11-21 23:42:29 -07:00
> +++ edited/drivers/serial/8250_pci.c	2005-02-07 12:00:32 -07:00
> @@ -2212,6 +2212,13 @@
> 		0, pbn_exar_XR17C158 },
>
> 	/*
> +	 * Topic TP560 Data/Fax/Voice 56k modem (reported by Evan Clarke)
> +	 */
> +	{	PCI_VENDOR_ID_TOPIC, PCI_DEVICE_ID_TOPIC_TP560,
> +		PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> +		pbn_b0_1_115200 },
> +
> +	/*
> 	 * These entries match devices with class COMMUNICATION_SERIAL,
> 	 * COMMUNICATION_MODEM or COMMUNICATION_MULTISERIAL
> 	 */
> ===== include/linux/pci_ids.h 1.200 vs edited =====
> --- 1.200/include/linux/pci_ids.h	2005-01-30 23:33:43 -07:00
> +++ edited/include/linux/pci_ids.h	2005-02-07 11:56:14 -07:00
> @@ -1972,6 +1972,9 @@
> #define PCI_DEVICE_ID_BCM4401		0x4401
> #define PCI_DEVICE_ID_BCM4401B0		0x4402
>
> +#define PCI_VENDOR_ID_TOPIC		0x151f
> +#define PCI_DEVICE_ID_TOPIC_TP560	0x0000
> +
> #define PCI_VENDOR_ID_ENE		0x1524
> #define PCI_DEVICE_ID_ENE_1211		0x1211
> #define PCI_DEVICE_ID_ENE_1225		0x1225
>
>
> -
> 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/
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [PATCH] [SERIAL] add TP560 data/fax/modem support

[Bjorn Helgaas]

On Mon, 2005-02-07 at 15:12 -0500, linux-os wrote:
> I thought somebody promised to add a pci_route_irq(dev) or some
> such so that the device didn't have to be enabled before
> the IRQ was correct.
>
> I first reported this bad IRQ problem back in December of 2004.
> Has the new function been added?

That's a completely different problem.  The point here is that
the serial driver currently doesn't do anything with the TP560
(no pci_enable_device(), no pci_route_irq(), no nothing).  Then
when setserial comes along and force-feeds the driver with the
IO and IRQ info, there's nothing at that point that does anything
to enable the device or route its interrupt either.

I did raise the idea of adding a pci_route_irq() interface, but
to be honest, I was never convinced of its general usefulness.
I haven't heard of any driver in the tree that requires it,
so it's not clear that it would be accepted even if I (or you)
wrote it.

I think you mentioned a specific PCI interface chip that was
susceptible to the problem; is there a public reference that
would help explicate the situation?

Re: [PATCH] [SERIAL] add TP560 data/fax/modem support

[linux-os]

On Mon, 7 Feb 2005, Bjorn Helgaas wrote:

> On Mon, 2005-02-07 at 15:12 -0500, linux-os wrote:
>> I thought somebody promised to add a pci_route_irq(dev) or some
>> such so that the device didn't have to be enabled before
>> the IRQ was correct.
>>
>> I first reported this bad IRQ problem back in December of 2004.
>> Has the new function been added?
>
> That's a completely different problem.  The point here is that
> the serial driver currently doesn't do anything with the TP560
> (no pci_enable_device(), no pci_route_irq(), no nothing).  Then
> when setserial comes along and force-feeds the driver with the
> IO and IRQ info, there's nothing at that point that does anything
> to enable the device or route its interrupt either.
>
> I did raise the idea of adding a pci_route_irq() interface, but
> to be honest, I was never convinced of its general usefulness.

There  have been two PCI drivers in the past three days where
users have reported that the IRQ was wrong. One patch was
provided to enable the device before requesting the IRQ, this
being inherently dangerous. So, if you are the one who
re-wrote the PCI stuff so that the IRQ is wrong when the
device is claimed, then I think you have a duty to fix the
code that you have broken.

The correct way to fix the broken code is to make sure that
the IRQ, reported in the structure, is correct. Alternate
methods might be a pci_route_irq() function.

The existing PCI code is broken. The fact that an invalid
IRQ is reported in the structure is PROOF that it is broken
and requires no further discussion. Many of us have to
use this stuff in a professional environment, we can't
enable devices without an interrupt handler in place
because it is not allowed in code that is subject to
peer review. We also can't use code when such problems
as invalid values in returned data are discovered.

Two months ago I discovered this problem and reported it, hoping
that the person who broke existing code would fix it. It has
not been fixed.

Cheers,
Dick Johnson
Penguin : Linux version 2.6.10 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [PATCH] [SERIAL] add TP560 data/fax/modem support

[Bjorn Helgaas]

On Tue, 2005-02-08 at 07:25 -0500, linux-os wrote:
> On Mon, 7 Feb 2005, Bjorn Helgaas wrote:
> 
> > On Mon, 2005-02-07 at 15:12 -0500, linux-os wrote:
> >> I thought somebody promised to add a pci_route_irq(dev) or some
> >> such so that the device didn't have to be enabled before
> >> the IRQ was correct.
> >>
> >> I first reported this bad IRQ problem back in December of 2004.
> >> Has the new function been added?
> >
> > That's a completely different problem.  The point here is that
> > the serial driver currently doesn't do anything with the TP560
> > (no pci_enable_device(), no pci_route_irq(), no nothing).  Then
> > when setserial comes along and force-feeds the driver with the
> > IO and IRQ info, there's nothing at that point that does anything
> > to enable the device or route its interrupt either.
> >
> > I did raise the idea of adding a pci_route_irq() interface, but
> > to be honest, I was never convinced of its general usefulness.
> 
> There  have been two PCI drivers in the past three days where
> users have reported that the IRQ was wrong.

If you've got pointers to these two reports, please provide them.

> One patch was
> provided to enable the device before requesting the IRQ, this
> being inherently dangerous. So, if you are the one who
> re-wrote the PCI stuff so that the IRQ is wrong when the
> device is claimed, then I think you have a duty to fix the
> code that you have broken.

Most drivers in the tree call pci_enable_device() before
request_irq().  That was the case even before my changes.
Dangerous?  Maybe, but I didn't make it more so.

> The correct way to fix the broken code is to make sure that
> the IRQ, reported in the structure, is correct. Alternate
> methods might be a pci_route_irq() function.

Or perhaps, the IRQ could be routed in pci_setup_device().
But, by the principle of "if you don't use it, don't touch
it", there *is* something to be said for the fact that the
current code doesn't touch IRQ routing unless a driver
actually claims the device.

> The existing PCI code is broken. The fact that an invalid
> IRQ is reported in the structure is PROOF that it is broken
> and requires no further discussion.

Then I'm sure your patch to fix it will be accepted without
much dissent.

> Many of us have to
> use this stuff in a professional environment, we can't
> enable devices without an interrupt handler in place
> because it is not allowed in code that is subject to
> peer review. We also can't use code when such problems
> as invalid values in returned data are discovered.

Nobody's forcing you to use "this stuff."

> Two months ago I discovered this problem and reported it, hoping
> that the person who broke existing code would fix it. It has
> not been fixed.

The fact is, yours is the only report of an issue with the
structure of the new code.  And it apparently only concerns
an out-of-tree driver.  Still, I agree that it might be good
to do something about it.  But you haven't provided pointers
to the hardware specs or driver involved.  So I'm not going to
go too much out of my way to fix it.

Re: [PATCH] [SERIAL] add TP560 data/fax/modem support

[Russell King]

On Mon, Feb 07, 2005 at 12:39:42PM -0700, Bjorn Helgaas wrote:
> Claim Topic TP560 data/fax/voice modem.  This device reports as class 0x0780,
> so we don't claim it by default:

Applied, thanks.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core