| Message ID | 1398433530-13136-50-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9b1ee0b2cb8bffdbb3003b1d5205f3ae0592c15a |
| Delegated to: | Takashi Iwai |
| Headers | show |
At Fri, 25 Apr 2014 22:45:30 +0900, Takashi Sakamoto wrote: > > In post commit, a quirk of this firmware about transactions is reported. > This commit apply a workaround for this quirk. > > They often fail transactions due to gap_count mismatch. This state is changed > by generating bus reset. > > The fw_schedule_bus_reset() is an exported symbol in firewire-core. But there > are no header for public. This commit moves its prototype from > drivers/firewire/core.h to include/linux/firewire.h. > > This mismatch still affects bus management before generating this bus reset. > It still takes a time to call driver's probe() because transactions are still > often failed. > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> Hrm, I should have taken a deeper review. This change was slipped to my tree without Stefan's ack. Stefan, is it OK to keep this change as is? thanks, Takashi > --- > drivers/firewire/core.h | 1 - > include/linux/firewire.h | 3 +++ > sound/firewire/bebob/bebob.c | 32 ++++++++++++++++++++++++++++---- > sound/firewire/bebob/bebob.h | 1 + > 4 files changed, 32 insertions(+), 5 deletions(-) > > diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h > index c98764a..870044e 100644 > --- a/drivers/firewire/core.h > +++ b/drivers/firewire/core.h > @@ -118,7 +118,6 @@ int fw_card_add(struct fw_card *card, > u32 max_receive, u32 link_speed, u64 guid); > void fw_core_remove_card(struct fw_card *card); > int fw_compute_block_crc(__be32 *block); > -void fw_schedule_bus_reset(struct fw_card *card, bool delayed, bool short_reset); > void fw_schedule_bm_work(struct fw_card *card, unsigned long delay); > > /* -cdev */ > diff --git a/include/linux/firewire.h b/include/linux/firewire.h > index c3683bd..d4b7683 100644 > --- a/include/linux/firewire.h > +++ b/include/linux/firewire.h > @@ -367,6 +367,9 @@ static inline int fw_stream_packet_destination_id(int tag, int channel, int sy) > return tag << 14 | channel << 8 | sy; > } > > +void fw_schedule_bus_reset(struct fw_card *card, bool delayed, > + bool short_reset); > + > struct fw_descriptor { > struct list_head link; > size_t length; > diff --git a/sound/firewire/bebob/bebob.c b/sound/firewire/bebob/bebob.c > index e1dd421..31b96b7 100644 > --- a/sound/firewire/bebob/bebob.c > +++ b/sound/firewire/bebob/bebob.c > @@ -247,10 +247,26 @@ bebob_probe(struct fw_unit *unit, > if (err < 0) > goto error; > > - err = snd_card_register(card); > - if (err < 0) { > - snd_bebob_stream_destroy_duplex(bebob); > - goto error; > + if (!bebob->maudio_special_quirk) { > + err = snd_card_register(card); > + if (err < 0) { > + snd_bebob_stream_destroy_duplex(bebob); > + goto error; > + } > + } else { > + /* > + * This is a workaround. This bus reset seems to have an effect > + * to make devices correctly handling transactions. Without > + * this, the devices have gap_count mismatch. This causes much > + * failure of transaction. > + * > + * Just after registration, user-land application receive > + * signals from dbus and starts I/Os. To avoid I/Os till the > + * future bus reset, registration is done in next update(). > + */ > + bebob->deferred_registration = true; > + fw_schedule_bus_reset(fw_parent_device(bebob->unit)->card, > + false, true); > } > > dev_set_drvdata(&unit->device, bebob); > @@ -273,6 +289,14 @@ bebob_update(struct fw_unit *unit) > > fcp_bus_reset(bebob->unit); > snd_bebob_stream_update_duplex(bebob); > + > + if (bebob->deferred_registration) { > + if (snd_card_register(bebob->card) < 0) { > + snd_bebob_stream_destroy_duplex(bebob); > + snd_card_free(bebob->card); > + } > + bebob->deferred_registration = false; > + } > } > > static void bebob_remove(struct fw_unit *unit) > diff --git a/sound/firewire/bebob/bebob.h b/sound/firewire/bebob/bebob.h > index 4a54e74..91b26b0 100644 > --- a/sound/firewire/bebob/bebob.h > +++ b/sound/firewire/bebob/bebob.h > @@ -109,6 +109,7 @@ struct snd_bebob { > > /* for M-Audio special devices */ > void *maudio_special_quirk; > + bool deferred_registration; > }; > > static inline int > -- > 1.8.3.2 >
On May 30 Takashi Iwai wrote: > At Fri, 25 Apr 2014 22:45:30 +0900, > Takashi Sakamoto wrote: > > > > In post commit, a quirk of this firmware about transactions is reported. > > This commit apply a workaround for this quirk. > > > > They often fail transactions due to gap_count mismatch. This state is changed > > by generating bus reset. > > > > The fw_schedule_bus_reset() is an exported symbol in firewire-core. But there > > are no header for public. This commit moves its prototype from > > drivers/firewire/core.h to include/linux/firewire.h. > > > > This mismatch still affects bus management before generating this bus reset. > > It still takes a time to call driver's probe() because transactions are still > > often failed. > > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > Hrm, I should have taken a deeper review. This change was slipped to > my tree without Stefan's ack. > Stefan, is it OK to keep this change as is? Yes, the implementation of this patch and its routing through the ALSA tree is OK with me. I should have sent Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de> right away when this revision of the patch series showed up on the list. (I think I only sent an informal agreement a time ago for an earlier revision.) Longer term we should look into improving this workaround to address 1. the above mentioned transaction failures which happen before the bebob driver's .probe is called (which are merely transient recoverable failures, apparently), 2. the theoretical possibility of the very same problem occurring with other IEEE 1394 devices. If we indeed come up with such a more general solution, its implementation will of course move out of the bebob driver into firewire-core. But this is really longer term since we need to be careful with that, and I might want to recreate an environment which reproduces the issue before I attempt such generalization or before I take a respective patch from somebody else. --- So in short, let's proceed with this patch through ALSA git. > thanks, > > Takashi > > > --- > > drivers/firewire/core.h | 1 - > > include/linux/firewire.h | 3 +++ > > sound/firewire/bebob/bebob.c | 32 ++++++++++++++++++++++++++++---- > > sound/firewire/bebob/bebob.h | 1 + > > 4 files changed, 32 insertions(+), 5 deletions(-) [...]
At Fri, 30 May 2014 22:22:59 +0200, Stefan Richter wrote: > > On May 30 Takashi Iwai wrote: > > At Fri, 25 Apr 2014 22:45:30 +0900, > > Takashi Sakamoto wrote: > > > > > > In post commit, a quirk of this firmware about transactions is reported. > > > This commit apply a workaround for this quirk. > > > > > > They often fail transactions due to gap_count mismatch. This state is changed > > > by generating bus reset. > > > > > > The fw_schedule_bus_reset() is an exported symbol in firewire-core. But there > > > are no header for public. This commit moves its prototype from > > > drivers/firewire/core.h to include/linux/firewire.h. > > > > > > This mismatch still affects bus management before generating this bus reset. > > > It still takes a time to call driver's probe() because transactions are still > > > often failed. > > > > > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > > > Hrm, I should have taken a deeper review. This change was slipped to > > my tree without Stefan's ack. > > Stefan, is it OK to keep this change as is? > > Yes, the implementation of this patch and its routing through the ALSA > tree is OK with me. I should have sent > Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de> > right away when this revision of the patch series showed up on the list. > (I think I only sent an informal agreement a time ago for an earlier > revision.) OK, thanks for confirmation! > Longer term we should look into improving this workaround to address > 1. the above mentioned transaction failures which happen before the bebob > driver's .probe is called (which are merely transient recoverable > failures, apparently), > 2. the theoretical possibility of the very same problem occurring with > other IEEE 1394 devices. > If we indeed come up with such a more general solution, its implementation > will of course move out of the bebob driver into firewire-core. But this > is really longer term since we need to be careful with that, and I might > want to recreate an environment which reproduces the issue before I > attempt such generalization or before I take a respective patch from > somebody else. --- So in short, let's proceed with this patch through ALSA > git. Sounds reasonable. thanks, Takashi > > > thanks, > > > > Takashi > > > > > --- > > > drivers/firewire/core.h | 1 - > > > include/linux/firewire.h | 3 +++ > > > sound/firewire/bebob/bebob.c | 32 ++++++++++++++++++++++++++++---- > > > sound/firewire/bebob/bebob.h | 1 + > > > 4 files changed, 32 insertions(+), 5 deletions(-) > [...] > > -- > Stefan Richter > -=====-====- -=-= ====- > http://arcgraph.de/sr/ >
diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index c98764a..870044e 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -118,7 +118,6 @@ int fw_card_add(struct fw_card *card, u32 max_receive, u32 link_speed, u64 guid); void fw_core_remove_card(struct fw_card *card); int fw_compute_block_crc(__be32 *block); -void fw_schedule_bus_reset(struct fw_card *card, bool delayed, bool short_reset); void fw_schedule_bm_work(struct fw_card *card, unsigned long delay); /* -cdev */ diff --git a/include/linux/firewire.h b/include/linux/firewire.h index c3683bd..d4b7683 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -367,6 +367,9 @@ static inline int fw_stream_packet_destination_id(int tag, int channel, int sy) return tag << 14 | channel << 8 | sy; } +void fw_schedule_bus_reset(struct fw_card *card, bool delayed, + bool short_reset); + struct fw_descriptor { struct list_head link; size_t length; diff --git a/sound/firewire/bebob/bebob.c b/sound/firewire/bebob/bebob.c index e1dd421..31b96b7 100644 --- a/sound/firewire/bebob/bebob.c +++ b/sound/firewire/bebob/bebob.c @@ -247,10 +247,26 @@ bebob_probe(struct fw_unit *unit, if (err < 0) goto error; - err = snd_card_register(card); - if (err < 0) { - snd_bebob_stream_destroy_duplex(bebob); - goto error; + if (!bebob->maudio_special_quirk) { + err = snd_card_register(card); + if (err < 0) { + snd_bebob_stream_destroy_duplex(bebob); + goto error; + } + } else { + /* + * This is a workaround. This bus reset seems to have an effect + * to make devices correctly handling transactions. Without + * this, the devices have gap_count mismatch. This causes much + * failure of transaction. + * + * Just after registration, user-land application receive + * signals from dbus and starts I/Os. To avoid I/Os till the + * future bus reset, registration is done in next update(). + */ + bebob->deferred_registration = true; + fw_schedule_bus_reset(fw_parent_device(bebob->unit)->card, + false, true); } dev_set_drvdata(&unit->device, bebob); @@ -273,6 +289,14 @@ bebob_update(struct fw_unit *unit) fcp_bus_reset(bebob->unit); snd_bebob_stream_update_duplex(bebob); + + if (bebob->deferred_registration) { + if (snd_card_register(bebob->card) < 0) { + snd_bebob_stream_destroy_duplex(bebob); + snd_card_free(bebob->card); + } + bebob->deferred_registration = false; + } } static void bebob_remove(struct fw_unit *unit) diff --git a/sound/firewire/bebob/bebob.h b/sound/firewire/bebob/bebob.h index 4a54e74..91b26b0 100644 --- a/sound/firewire/bebob/bebob.h +++ b/sound/firewire/bebob/bebob.h @@ -109,6 +109,7 @@ struct snd_bebob { /* for M-Audio special devices */ void *maudio_special_quirk; + bool deferred_registration; }; static inline int
In post commit, a quirk of this firmware about transactions is reported. This commit apply a workaround for this quirk. They often fail transactions due to gap_count mismatch. This state is changed by generating bus reset. The fw_schedule_bus_reset() is an exported symbol in firewire-core. But there are no header for public. This commit moves its prototype from drivers/firewire/core.h to include/linux/firewire.h. This mismatch still affects bus management before generating this bus reset. It still takes a time to call driver's probe() because transactions are still often failed. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- drivers/firewire/core.h | 1 - include/linux/firewire.h | 3 +++ sound/firewire/bebob/bebob.c | 32 ++++++++++++++++++++++++++++---- sound/firewire/bebob/bebob.h | 1 + 4 files changed, 32 insertions(+), 5 deletions(-)