| Message ID | 1312442059-23935-17-git-send-email-thierry.reding@avionic-design.de (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Em 04-08-2011 04:14, Thierry Reding escreveu: > Instead of selecting the default interface setting when preparing > isochronous transfers, select it on the first call to open() to make > sure it is available earlier. Hmm... I fail to see what this is needed earlier. The ISOC endpont is used only when the device is streaming. Did you get any bug related to it? If so, please describe it better. > --- > drivers/staging/tm6000/tm6000-video.c | 17 ++++++++++++----- > 1 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c > index 70fc19e..b59a0da 100644 > --- a/drivers/staging/tm6000/tm6000-video.c > +++ b/drivers/staging/tm6000/tm6000-video.c > @@ -595,11 +595,6 @@ static int tm6000_prepare_isoc(struct tm6000_core *dev) > tm6000_uninit_isoc(dev); > /* Stop interrupt USB pipe */ > tm6000_ir_int_stop(dev); > - > - usb_set_interface(dev->udev, > - dev->isoc_in.bInterfaceNumber, > - dev->isoc_in.bAlternateSetting); > - > /* Start interrupt USB pipe */ > tm6000_ir_int_start(dev); > > @@ -1484,6 +1479,18 @@ static int tm6000_open(struct file *file) > break; > } > > + if (dev->users == 0) { > + int err = usb_set_interface(dev->udev, > + dev->isoc_in.bInterfaceNumber, > + dev->isoc_in.bAlternateSetting); > + if (err < 0) { > + dev_err(&vdev->dev, "failed to select interface %d, " > + "alt. setting %d\n", > + dev->isoc_in.bInterfaceNumber, > + dev->isoc_in.bAlternateSetting); > + } > + } > + > /* If more than one user, mutex should be added */ > dev->users++; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Mauro Carvalho Chehab wrote: > Em 04-08-2011 04:14, Thierry Reding escreveu: > > Instead of selecting the default interface setting when preparing > > isochronous transfers, select it on the first call to open() to make > > sure it is available earlier. > > Hmm... I fail to see what this is needed earlier. The ISOC endpont is used > only when the device is streaming. > > Did you get any bug related to it? If so, please describe it better. I'm not sure whether this really fixes a bug, but it seems a little wrong to me to selecting the interface so late in the process when in fact the device is already being configured before (video standard, audio mode, firmware upload, ...). Thinking about it, this may actually be part of the fix for the "device hangs sometimes for inexplicable reasons" bug that this whole patch series seems to fix. Thierry
Em 01-09-2011 02:19, Thierry Reding escreveu: > * Mauro Carvalho Chehab wrote: >> Em 04-08-2011 04:14, Thierry Reding escreveu: >>> Instead of selecting the default interface setting when preparing >>> isochronous transfers, select it on the first call to open() to make >>> sure it is available earlier. >> >> Hmm... I fail to see what this is needed earlier. The ISOC endpont is used >> only when the device is streaming. >> >> Did you get any bug related to it? If so, please describe it better. > > I'm not sure whether this really fixes a bug, but it seems a little wrong to > me to selecting the interface so late in the process when in fact the device > is already being configured before (video standard, audio mode, firmware > upload, ...). Some applications may open the device just to change the controls. All other drivers only set alternates/interfaces when the streaming is requested, as alternates/interfaces are needed only there. > Thinking about it, this may actually be part of the fix for the "device hangs > sometimes for inexplicable reasons" bug that this whole patch series seems to > fix. It is unlikely, except if the firmware inside the chip is broken (unfortunately, we have serious reasons to believe that the internal firmware on this chipset has serious bugs). I prefer to not apply this patch, except if we have a good reason for that, as otherwise this driver will behave different than the others. Regards, Mauro. > > Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Mauro Carvalho Chehab wrote: > Em 01-09-2011 02:19, Thierry Reding escreveu: > > * Mauro Carvalho Chehab wrote: > >> Em 04-08-2011 04:14, Thierry Reding escreveu: > >>> Instead of selecting the default interface setting when preparing > >>> isochronous transfers, select it on the first call to open() to make > >>> sure it is available earlier. > >> > >> Hmm... I fail to see what this is needed earlier. The ISOC endpont is used > >> only when the device is streaming. > >> > >> Did you get any bug related to it? If so, please describe it better. > > > > I'm not sure whether this really fixes a bug, but it seems a little wrong to > > me to selecting the interface so late in the process when in fact the device > > is already being configured before (video standard, audio mode, firmware > > upload, ...). > > Some applications may open the device just to change the controls. All other drivers > only set alternates/interfaces when the streaming is requested, as alternates/interfaces > are needed only there. Okay, I didn't know that it was only necessary for streaming. > > Thinking about it, this may actually be part of the fix for the "device hangs > > sometimes for inexplicable reasons" bug that this whole patch series seems to > > fix. > > It is unlikely, except if the firmware inside the chip is broken (unfortunately, > we have serious reasons to believe that the internal firmware on this chipset has > serious bugs). Indeed! =) > I prefer to not apply this patch, except if we have a good reason for that, > as otherwise this driver will behave different than the others. Okay, it's your call. Unfortunately I no longer have the hardware available to test if this is really related to the bug. I'll have to check again when I have the hardware. Thierry
diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c index 70fc19e..b59a0da 100644 --- a/drivers/staging/tm6000/tm6000-video.c +++ b/drivers/staging/tm6000/tm6000-video.c @@ -595,11 +595,6 @@ static int tm6000_prepare_isoc(struct tm6000_core *dev) tm6000_uninit_isoc(dev); /* Stop interrupt USB pipe */ tm6000_ir_int_stop(dev); - - usb_set_interface(dev->udev, - dev->isoc_in.bInterfaceNumber, - dev->isoc_in.bAlternateSetting); - /* Start interrupt USB pipe */ tm6000_ir_int_start(dev); @@ -1484,6 +1479,18 @@ static int tm6000_open(struct file *file) break; } + if (dev->users == 0) { + int err = usb_set_interface(dev->udev, + dev->isoc_in.bInterfaceNumber, + dev->isoc_in.bAlternateSetting); + if (err < 0) { + dev_err(&vdev->dev, "failed to select interface %d, " + "alt. setting %d\n", + dev->isoc_in.bInterfaceNumber, + dev->isoc_in.bAlternateSetting); + } + } + /* If more than one user, mutex should be added */ dev->users++;