[PATCH] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[PATCH] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[Stefan Richter]

Unlike the ohci1394 driver, fw-ohci uses the selfIDGeneration field of
bus reset packets to determine the generation of incoming requests as
per OHCI 1.1 clause 8.4.2.3.  This is more precise --- provided that the
controller inserts the correct generation.  Texas Instruments chips
often don't.

This prevented the transmission of response packets, which for example
broke AV/C transactions as used when communicating with miniDV cameras
and any other AV/C devices.

There is apparently no way to detect and adjust incorrect generations.
Therefore we ignore the generation of bus reset packets from TI chips
and use the generation of the self ID buffer instead.  Alas this is
received at a slightly wrong time.  In rare cases, this could cause us
to not respond to legitimate requests in rare cases, or to respond to
expired requests.  (The latter is less likely because the bus reset
packet AR event is typically handled before the self ID complete event.)

Bug reported by Mladen Kuntner, who was extraordinarily patient while
dealing with the driver maintainers.
https://bugzilla.redhat.com/show_bug.cgi?id=243081

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-ohci.c |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -571,14 +571,22 @@ static __le32 *handle_ar_packet(struct a
 	 * generation.  We only need this for requests; for responses
 	 * we use the unique tlabel for finding the matching
 	 * request.
+	 *
+	 * Alas TI chips often emit bus reset packets with a wrong
+	 * generation.  As a trade-off, we set the correct generation
+	 * at a slightly incorrect time (in bus_reset_tasklet).
 	 */
+	if (evt == OHCI1394_evt_bus_reset) {
+		struct pci_dev *dev = to_pci_dev(ohci->card.device);
+
+		if (dev->vendor != PCI_VENDOR_ID_TI)
+			ohci->request_generation = (p.header[2] >> 16) & 0xff;
 
-	if (evt == OHCI1394_evt_bus_reset)
-		ohci->request_generation = (p.header[2] >> 16) & 0xff;
-	else if (ctx == &ohci->ar_request_ctx)
+	} else if (ctx == &ohci->ar_request_ctx) {
 		fw_core_handle_request(&ohci->card, &p);
-	else
+	} else {
 		fw_core_handle_response(&ohci->card, &p);
+	}
 
 	return buffer + length + 1;
 }
@@ -1209,6 +1217,7 @@ at_context_transmit(struct context *ctx,
 static void bus_reset_tasklet(unsigned long data)
 {
 	struct fw_ohci *ohci = (struct fw_ohci *)data;
+	struct pci_dev *dev = to_pci_dev(ohci->card.device);
 	int self_id_count, i, j, reg;
 	int generation, new_generation;
 	unsigned long flags;
@@ -1285,6 +1294,9 @@ static void bus_reset_tasklet(unsigned l
 	context_stop(&ohci->at_response_ctx);
 	reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset);
 
+	if (dev->vendor == PCI_VENDOR_ID_TI)
+		ohci->request_generation = generation;
+
 	/*
 	 * This next bit is unrelated to the AT context stuff but we
 	 * have to do it under the spinlock also.  If a new config rom

-- 
Stefan Richter
-=====-==--- -=-- -==--
http://arcgraph.de/sr/

[PATCH update] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[Stefan Richter]

Unlike the ohci1394 driver, fw-ohci uses the selfIDGeneration field of
bus reset packets to determine the generation of incoming requests as
per OHCI 1.1 clause 8.4.2.3.  This is more precise --- provided that the
controller inserts the correct generation.  Texas Instruments chips
often don't.

This prevented the transmission of response packets, which for example
broke AV/C transactions as used when communicating with miniDV cameras
and any other AV/C devices.

There is apparently no way to detect and adjust incorrect generations.
Therefore we ignore the generation of bus reset packets from TI chips
and use the generation of the self ID buffer instead.  Alas this is
received at a slightly wrong time.  In rare cases, this could cause us
to not respond to legitimate requests or to respond to expired requests.
(The latter is less likely because the bus reset packet AR event is
typically handled before the self ID complete event.)

Bug reported by Mladen Kuntner, who was extraordinarily patient while
dealing with the driver maintainers.
https://bugzilla.redhat.com/show_bug.cgi?id=243081

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

update: use a quirk flag for simpler code

 drivers/firewire/fw-ohci.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -181,6 +181,7 @@ struct fw_ohci {
 	int request_generation;	/* for timestamping incoming requests */
 	u32 bus_seconds;
 	bool old_uninorth;
+	bool bus_reset_packet_quirk;
 
 	/*
 	 * Spinlock for accessing fw_ohci data.  Never call out of
@@ -571,14 +572,19 @@ static __le32 *handle_ar_packet(struct a
 	 * generation.  We only need this for requests; for responses
 	 * we use the unique tlabel for finding the matching
 	 * request.
+	 *
+	 * Alas some chips sometimes emit bus reset packets with a
+	 * wrong generation.  We set the correct generation for these
+	 * at a slightly incorrect time (in bus_reset_tasklet).
 	 */
-
-	if (evt == OHCI1394_evt_bus_reset)
-		ohci->request_generation = (p.header[2] >> 16) & 0xff;
-	else if (ctx == &ohci->ar_request_ctx)
+	if (evt == OHCI1394_evt_bus_reset) {
+		if (!ohci->bus_reset_packet_quirk)
+			ohci->request_generation = (p.header[2] >> 16) & 0xff;
+	} else if (ctx == &ohci->ar_request_ctx) {
 		fw_core_handle_request(&ohci->card, &p);
-	else
+	} else {
 		fw_core_handle_response(&ohci->card, &p);
+	}
 
 	return buffer + length + 1;
 }
@@ -1285,6 +1291,9 @@ static void bus_reset_tasklet(unsigned l
 	context_stop(&ohci->at_response_ctx);
 	reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset);
 
+	if (ohci->bus_reset_packet_quirk)
+		ohci->request_generation = generation;
+
 	/*
 	 * This next bit is unrelated to the AT context stuff but we
 	 * have to do it under the spinlock also.  If a new config rom
@@ -2360,6 +2369,8 @@ pci_probe(struct pci_dev *dev, const str
 	ohci->old_uninorth = dev->vendor == PCI_VENDOR_ID_APPLE &&
 			     dev->device == PCI_DEVICE_ID_APPLE_UNI_N_FW;
 #endif
+	ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
+
 	spin_lock_init(&ohci->lock);
 
 	tasklet_init(&ohci->bus_reset_tasklet,

-- 
Stefan Richter
-=====-==--- -=-- -==--
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[Jarod Wilson]

On Saturday 12 April 2008 04:31:25 pm Stefan Richter wrote:
> Unlike the ohci1394 driver, fw-ohci uses the selfIDGeneration field of
> bus reset packets to determine the generation of incoming requests as
> per OHCI 1.1 clause 8.4.2.3.  This is more precise --- provided that the
> controller inserts the correct generation.  Texas Instruments chips
> often don't.
>
> This prevented the transmission of response packets, which for example
> broke AV/C transactions as used when communicating with miniDV cameras
> and any other AV/C devices.
>
> There is apparently no way to detect and adjust incorrect generations.
> Therefore we ignore the generation of bus reset packets from TI chips
> and use the generation of the self ID buffer instead.  Alas this is
> received at a slightly wrong time.  In rare cases, this could cause us
> to not respond to legitimate requests or to respond to expired requests.
> (The latter is less likely because the bus reset packet AR event is
> typically handled before the self ID complete event.)
>
> Bug reported by Mladen Kuntner, who was extraordinarily patient while
> dealing with the driver maintainers.
> https://bugzilla.redhat.com/show_bug.cgi?id=243081
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>
> update: use a quirk flag for simpler code

The work-around looks good to me, just one question.

> @@ -2360,6 +2369,8 @@ pci_probe(struct pci_dev *dev, const str
>  	ohci->old_uninorth = dev->vendor == PCI_VENDOR_ID_APPLE &&
>  			     dev->device == PCI_DEVICE_ID_APPLE_UNI_N_FW;
>  #endif
> +	ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
> +

I have a few cards with PCI_VENDOR_ID_CREATIVE with a TI TSB41AB2 chip on 'em 
(SoundBlaster Audigy w/FireWire port). I've not had any issues on any of the 
cards I've got, but do we want to add them to the work-around list just to be 
safe?


-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH update] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[Stefan Richter]

Jarod Wilson wrote:
> On Saturday 12 April 2008 04:31:25 pm Stefan Richter wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=243081
...
>> @@ -2360,6 +2369,8 @@ pci_probe(struct pci_dev *dev, const str
>>  	ohci->old_uninorth = dev->vendor == PCI_VENDOR_ID_APPLE &&
>>  			     dev->device == PCI_DEVICE_ID_APPLE_UNI_N_FW;
>>  #endif
>> +	ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
>> +
> 
> I have a few cards with PCI_VENDOR_ID_CREATIVE with a TI TSB41AB2 chip on 'em 
> (SoundBlaster Audigy w/FireWire port). I've not had any issues on any of the 
> cards I've got, but do we want to add them to the work-around list just to be 
> safe?

Right now TSB82AA2 is confirmed buggy by two slightly differing reports
(Mladen's and mine) and TSB43AB22 or TSB43AB22A is under suspicion.

Whether TSB41AB2 is affected or not is impossible to say without having
a hardware combination which triggers the bug.  So far it seems the bug
occurs if there are more bus reset events than self ID complete events.
Note, I noticed the bug on my TSB82AA2 only by means of the bus reset
packet generation logging, not because of any subsequent malfunction
(because a bus reset forced by the bus manager code is always saving the
day on my setup).

If you could reproduce these conditions somehow and then check the debug
log for temporary generation mismatches, that would be good.  Note that
the conditions for the bug to occur are almost exclusively influenced by
the PHY layer hardware of all nodes on the bus, while the bug itself is
caused by the link layer controller.

Anyway, if we get confirmation that the proposed patch fixes
TSB43AB22(A) and you don't have the means to prove that TSB41AB2 is not
buggy, then let's add the vendor ID of Creative as well.  As mentioned,
the patch introduces a certain danger of
 1. fw-core missing to send responses to requests which came in
    immediately after bus reset (unlikely border case),
 2. fw-core responding to requests which came in before the last
    bus reset (even less likely border case, because that would only
    happen if the self ID complete event was processed before an older
    AR event, which does not happen according to my observations with
    a bunch of different chips).

1. is to be compensated by the requester by retry.  We have seen that
some simplistic requesters have difficulties with that.  But luckily,
these requesters are AV/C targets and send requests as AV/C transaction
responses.  I think the chances for this to happen are rather low.

2. is to be compensated by the requester by proper matching of
transaction labels and discarding transaction labels at bus reset events.

AFAIU, the ieee1394 stack has always been in danger to trip these border
cases because ohci1394 never looks at the bus reset packet generation.
So this speaks _for_ enabling the bus_reset_packet_quirk if a card is
merely under suspicion of having the bug and we are unable to find
someone who can disprove the suspicion for us.
-- 
Stefan Richter
-=====-==--- -=-- -==-=
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[Stefan Richter]

Jarod Wilson wrote:
> On Saturday 12 April 2008 04:31:25 pm Stefan Richter wrote:
>> +	ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
>> +
> 
> I have a few cards with PCI_VENDOR_ID_CREATIVE with a TI TSB41AB2 chip on 'em 
> (SoundBlaster Audigy w/FireWire port). I've not had any issues on any of the 
> cards I've got, but do we want to add them to the work-around list just to be 
> safe?

No, we don't need to.  TSB41AB2 is only a PHY, not a link layer 
controller.  While the PHYs (the combination of PHYs which are present 
on the bus) influence the conditions under which the bug can happen, the 
cause for the bug is in the link layer controller alone.

So unless Creative used a TI design in their link layer controller or 
otherwise managed to implement this same quirk as TI (Agere, NEC, and 
VIA didn't according to my tests so far), the presence of TSB41AB2 on a 
card does not make it necessary to activate the quirk workaround.
-- 
Stefan Richter
-=====-==--- -=-- =--=-
http://arcgraph.de/sr/

Re: [PATCH update] firewire: fw-ohci: work around generation bug in TI controllers (fix AV/C and more)

[Jarod Wilson]

On Thursday 17 April 2008 06:47:16 pm Stefan Richter wrote:
> Jarod Wilson wrote:
> > On Saturday 12 April 2008 04:31:25 pm Stefan Richter wrote:
> >> +	ohci->bus_reset_packet_quirk = dev->vendor == PCI_VENDOR_ID_TI;
> >> +
> >
> > I have a few cards with PCI_VENDOR_ID_CREATIVE with a TI TSB41AB2 chip on
> > 'em (SoundBlaster Audigy w/FireWire port). I've not had any issues on any
> > of the cards I've got, but do we want to add them to the work-around list
> > just to be safe?
>
> No, we don't need to.  TSB41AB2 is only a PHY, not a link layer
> controller.  While the PHYs (the combination of PHYs which are present
> on the bus) influence the conditions under which the bug can happen, the
> cause for the bug is in the link layer controller alone.
>
> So unless Creative used a TI design in their link layer controller or
> otherwise managed to implement this same quirk as TI (Agere, NEC, and
> VIA didn't according to my tests so far), the presence of TSB41AB2 on a
> card does not make it necessary to activate the quirk workaround.

Ah, I'll have to take a closer look at these cards and see if I can figure out 
what drives the link layer... But barring the discovery of another TI chip, 
I'm assuming an attempt to reproduce the generation issue will be fruitless.

Well, based on the positive results we've seen thus far with TI controllers 
and this patch in Fedora kernels:

Signed-off-by: Jarod Wilson <jwilson@redhat.com>

-- 
Jarod Wilson
jwilson@redhat.com