Re: [PATCH] firewire: core: send bus reset promptly on gap count error
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2024-01-27 08:37:52
|
Hi, Thanks for the patch. I would like to check some points about the change. On Tue, Jan 23, 2024 at 12:11:40AM -0800, Adam Goldman wrote: > If we are bus manager and the bus has inconsistent gap counts, send a > bus reset immediately instead of trying to read the root node's config > ROM first. Otherwise, we could spend a lot of time trying to read the > config ROM but never succeeding. > > This eliminates a 50+ second delay before the FireWire bus is usable > after a newly connected device is powered on in certain circumstances. At first, would I request you to explain about the certain circumstances in the patch comment? It is really helpful to understand the change itself. > Signed-off-by: Adam Goldman <ad...@po...> > Link: https://sourceforge.net/p/linux1394/mailman/message/58727806/ > --- > > --- linux-source-6.1.orig/drivers/firewire/core-card.c 2023-09-23 02:11:13.000000000 -0700 > +++ linux-source-6.1/drivers/firewire/core-card.c 2024-01-22 04:23:06.000000000 -0800 > @@ -435,6 +435,16 @@ > * config rom. In either case, pick another root. > */ > new_root_id = local_id; > + } else if (card->gap_count == 0) { > + /* > + * If self IDs have inconsistent gap counts, do a > + * bus reset ASAP. The config rom read might never > + * complete, so don't wait for it. However, still > + * send a PHY configuration packet prior to the bus > + * reset, as permitted by IEEE 1394-2008 8.4.5.2. > + */ > + new_root_id = local_id; > + card->bm_retries = 0; > } else if (!root_device_is_running) { > /* > * If we haven't probed this device yet, bail out now Next, after the condition branches, we can see below lines: ``` /* * Finally, figure out if we should do a reset or not. If we have * done less than 5 resets with the same physical topology and we * have either a new root or a new gap count setting, let's do it. */ if (card->bm_retries++ < 5 && (card->gap_count != gap_count || new_root_id != root_id)) do_reset = true; ``` When the value of "card->gap_count" is zero, it would hit the condition of "card->gap_count != gap_count". I think the transmission of phy config packet and scheduling of short bus reset would be done, regardless of the change. Would I ask the main intention to the additional branch? Thanks Takashi Sakamoto |