[PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Malcolm Priestley]

Hi antti

There some issues with dvb_usb_v2 with the lmedm04 driver.

The first being this patch, no return value from dvb_usb_download_firmware
causes system wide dead lock with COLD disconnect as system attempts to continue
to warm state.

The second is to do with d->props.bInterfaceNumber in patch 2.

In get_usb_stream_config there no handing of the struct dvb_frontend.
...
int (*get_usb_stream_config) (struct dvb_frontend *,
			struct usb_data_stream_properties *);

...
I wonder if it would be better to use adapter instead?

I also have a draft patch to use delayed work.

Regards


Malcolm


Signed-off-by: Malcolm Priestley <tvboxspy@gmail.com>
---
 drivers/media/dvb/dvb-usb/dvb_usb_init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/dvb/dvb-usb/dvb_usb_init.c b/drivers/media/dvb/dvb-usb/dvb_usb_init.c
index 60aa944..c16a28a 100644
--- a/drivers/media/dvb/dvb-usb/dvb_usb_init.c
+++ b/drivers/media/dvb/dvb-usb/dvb_usb_init.c
@@ -61,7 +61,7 @@ static int dvb_usb_download_firmware(struct dvb_usb_device *d)
 	if (ret < 0)
 		goto err;
 
-	return 0;
+	return ret;
 err:
 	pr_debug("%s: failed=%d\n", __func__, ret);
 	return ret;
-- 
1.7.10

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Antti Palosaari]

Hi Malcolm,
I was really surprised someone has had interest to test that stuff at 
that phase as I did not even advertised it yet :) It is likely happen 
next Monday or so as there is some issues I would like to check / solve.


On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
> Hi antti
>
> There some issues with dvb_usb_v2 with the lmedm04 driver.
>
> The first being this patch, no return value from dvb_usb_download_firmware
> causes system wide dead lock with COLD disconnect as system attempts to continue
> to warm state.

Hmm, I did not understand what you mean. What I looked lmedm04 driver I 
think it uses single USB ID (no cold + warm IDs). So it downloads 
firmware and then reconnects itself from the USB bus?
For that scenario you should "return RECONNECTS_USB;" from the driver 
.download_firmware().

I tested it using one non-public Cypress FX2 device - it was changing 
USB ID after the FX download, but from the driver perspective it does 
not matter. It is always new device if it reconnects USB.

PS. as I looked that driver I saw many different firmwares. That is now 
supported and you should use .get_firmware_name() (maybe you already did 
it).

> The second is to do with d->props.bInterfaceNumber in patch 2.

See the other mail.

> In get_usb_stream_config there no handing of the struct dvb_frontend.
> ...
> int (*get_usb_stream_config) (struct dvb_frontend *,
> 			struct usb_data_stream_properties *);
>
> ...
> I wonder if it would be better to use adapter instead?

That was something I think many times how to implement. The motivation 
of whole callback is mainly from the MFE. You have one stream (per 
adapter) which is needed to configure dynamically per use case. I left 
still old static stream configuration as it was, but moved it to the 
adapter property. So if you need only one stream configuration use old 
style as it looks simpler.

But back to actual question. It is simple - we need to know used 
frontend that we could return correct settings. Imagine MFE device, FE0 
uses bulk stream and FE1 uses isochronous. Knowing frontend makes it 
possible to reconfigure.

There is one "hack" I did intentionally. At the attach() it is called 
frontend == NULL to signal we need to get maximum buffer size in order 
to alloc big enough USB DMA buffers. Maybe I will add macro for that, 
like BUFFERS(count, size) to clear. Or even add another callback.

>
> I also have a draft patch to use delayed work.
>
> Regards
>
>
> Malcolm
>
>
> Signed-off-by: Malcolm Priestley<tvboxspy@gmail.com>
> ---
>   drivers/media/dvb/dvb-usb/dvb_usb_init.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/dvb/dvb-usb/dvb_usb_init.c b/drivers/media/dvb/dvb-usb/dvb_usb_init.c
> index 60aa944..c16a28a 100644
> --- a/drivers/media/dvb/dvb-usb/dvb_usb_init.c
> +++ b/drivers/media/dvb/dvb-usb/dvb_usb_init.c
> @@ -61,7 +61,7 @@ static int dvb_usb_download_firmware(struct dvb_usb_device *d)
>   	if (ret<  0)
>   		goto err;
>
> -	return 0;
> +	return ret;
>   err:
>   	pr_debug("%s: failed=%d\n", __func__, ret);
>   	return ret;

Thank you for the comments and testing, I am happy to answer other 
questions too.

regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Malcolm Priestley]

On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
> Hi Malcolm,
> I was really surprised someone has had interest to test that stuff at 
> that phase as I did not even advertised it yet :) It is likely happen 
> next Monday or so as there is some issues I would like to check / solve.
> 
> 
> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
> > Hi antti
> >
> > There some issues with dvb_usb_v2 with the lmedm04 driver.
> >
> > The first being this patch, no return value from dvb_usb_download_firmware
> > causes system wide dead lock with COLD disconnect as system attempts to continue
> > to warm state.
> 
> Hmm, I did not understand what you mean. What I looked lmedm04 driver I 
> think it uses single USB ID (no cold + warm IDs). So it downloads 
> firmware and then reconnects itself from the USB bus?
> For that scenario you should "return RECONNECTS_USB;" from the driver 
> .download_firmware().
> 
If the device disconnects from the USB bus after the firmware download.

In most cases the device is already gone.

There is currently no way to insert RECONNECTS_USB into the return.


> I tested it using one non-public Cypress FX2 device - it was changing 
> USB ID after the FX download, but from the driver perspective it does 
> not matter. It is always new device if it reconnects USB.
> 

Have double checked that the thread is not continuing to write on the
old ID?

The zero condition will lead to dvb_usb_init.

> PS. as I looked that driver I saw many different firmwares. That is now 
> supported and you should use .get_firmware_name() (maybe you already did 
> it).
> 
Yes, I have supported this in the driver.




Regards


Malcolm

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Antti Palosaari]

On 06/14/2012 03:44 AM, Malcolm Priestley wrote:
> On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
>> Hi Malcolm,
>> I was really surprised someone has had interest to test that stuff at
>> that phase as I did not even advertised it yet :) It is likely happen
>> next Monday or so as there is some issues I would like to check / solve.
>>
>>
>> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
>>> Hi antti
>>>
>>> There some issues with dvb_usb_v2 with the lmedm04 driver.
>>>
>>> The first being this patch, no return value from dvb_usb_download_firmware
>>> causes system wide dead lock with COLD disconnect as system attempts to continue
>>> to warm state.
>>
>> Hmm, I did not understand what you mean. What I looked lmedm04 driver I
>> think it uses single USB ID (no cold + warm IDs). So it downloads
>> firmware and then reconnects itself from the USB bus?
>> For that scenario you should "return RECONNECTS_USB;" from the driver
>> .download_firmware().
>>
> If the device disconnects from the USB bus after the firmware download.
>
> In most cases the device is already gone.
>
> There is currently no way to insert RECONNECTS_USB into the return.

Argh, I was blind! You are absolutely correct. It never returns value 1 
(RECONNECTS_USB) from the .download_firmware().

That patch is fine, I will apply it, thanks!

I think that must be also changed to return immediately without 
releasing the interface. Let the USB core release it when it detects 
disconnect - otherwise it could crash as it tries to access potentially 
resources that are already freed. Just for the timing issue if it 
happens or not.

} else if (ret == RECONNECTS_USB) {
	ret = 0;
	goto exit_usb_driver_release_interface;

add return 0 here without releasing interface and test.


>> I tested it using one non-public Cypress FX2 device - it was changing
>> USB ID after the FX download, but from the driver perspective it does
>> not matter. It is always new device if it reconnects USB.
>>
>
> Have double checked that the thread is not continuing to write on the
> old ID?

Nope, but likely delayed probe() is finished until it reconnects so I 
cannot see problem. You device disconnects faster and thus USB core 
traps .disconnect() earlier...

Could you test returning 0 and if it works sent new patch.

> The zero condition will lead to dvb_usb_init.
>
>> PS. as I looked that driver I saw many different firmwares. That is now
>> supported and you should use .get_firmware_name() (maybe you already did
>> it).
>>
> Yes, I have supported this in the driver.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Antti Palosaari]

On 06/14/2012 04:15 AM, Antti Palosaari wrote:
> On 06/14/2012 03:44 AM, Malcolm Priestley wrote:
>> On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
>>> Hi Malcolm,
>>> I was really surprised someone has had interest to test that stuff at
>>> that phase as I did not even advertised it yet :) It is likely happen
>>> next Monday or so as there is some issues I would like to check / solve.
>>>
>>>
>>> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
>>>> Hi antti
>>>>
>>>> There some issues with dvb_usb_v2 with the lmedm04 driver.
>>>>
>>>> The first being this patch, no return value from
>>>> dvb_usb_download_firmware
>>>> causes system wide dead lock with COLD disconnect as system attempts
>>>> to continue
>>>> to warm state.
>>>
>>> Hmm, I did not understand what you mean. What I looked lmedm04 driver I
>>> think it uses single USB ID (no cold + warm IDs). So it downloads
>>> firmware and then reconnects itself from the USB bus?
>>> For that scenario you should "return RECONNECTS_USB;" from the driver
>>> .download_firmware().
>>>
>> If the device disconnects from the USB bus after the firmware download.
>>
>> In most cases the device is already gone.
>>
>> There is currently no way to insert RECONNECTS_USB into the return.
>
> Argh, I was blind! You are absolutely correct. It never returns value 1
> (RECONNECTS_USB) from the .download_firmware().
>
> That patch is fine, I will apply it, thanks!
>
> I think that must be also changed to return immediately without
> releasing the interface. Let the USB core release it when it detects
> disconnect - otherwise it could crash as it tries to access potentially
> resources that are already freed. Just for the timing issue if it
> happens or not.
>
> } else if (ret == RECONNECTS_USB) {
> ret = 0;
> goto exit_usb_driver_release_interface;
>
> add return 0 here without releasing interface and test.
>
>
>>> I tested it using one non-public Cypress FX2 device - it was changing
>>> USB ID after the FX download, but from the driver perspective it does
>>> not matter. It is always new device if it reconnects USB.
>>>
>>
>> Have double checked that the thread is not continuing to write on the
>> old ID?
>
> Nope, but likely delayed probe() is finished until it reconnects so I
> cannot see problem. You device disconnects faster and thus USB core
> traps .disconnect() earlier...
>
> Could you test returning 0 and if it works sent new patch.
>
>> The zero condition will lead to dvb_usb_init.
>>
>>> PS. as I looked that driver I saw many different firmwares. That is now
>>> supported and you should use .get_firmware_name() (maybe you already did
>>> it).
>>>
>> Yes, I have supported this in the driver.

Malcolm,

could you just test if returning from the routines after fw download is 
enough to fix all your problems?

I mean those two fixes:
dvb_usb_download_firmware()
* return RECONNECTS_USB correctly

dvb_usbv2_init_work()
* return without releasing USB interface if RECONNECTS_USB

regardss
Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Malcolm Priestley]

On Thu, 2012-06-14 at 23:31 +0300, Antti Palosaari wrote:
> On 06/14/2012 04:15 AM, Antti Palosaari wrote:
> > On 06/14/2012 03:44 AM, Malcolm Priestley wrote:
> >> On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
> >>> Hi Malcolm,
> >>> I was really surprised someone has had interest to test that stuff at
> >>> that phase as I did not even advertised it yet :) It is likely happen
> >>> next Monday or so as there is some issues I would like to check / solve.
> >>>
> >>>
> >>> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
> >>>> Hi antti
> >>>>
> >>>> There some issues with dvb_usb_v2 with the lmedm04 driver.
> >>>>
> >>>> The first being this patch, no return value from
> >>>> dvb_usb_download_firmware
> >>>> causes system wide dead lock with COLD disconnect as system attempts
> >>>> to continue
> >>>> to warm state.
> >>>
> >>> Hmm, I did not understand what you mean. What I looked lmedm04 driver I
> >>> think it uses single USB ID (no cold + warm IDs). So it downloads
> >>> firmware and then reconnects itself from the USB bus?
> >>> For that scenario you should "return RECONNECTS_USB;" from the driver
> >>> .download_firmware().
> >>>
> >> If the device disconnects from the USB bus after the firmware download.
> >>
> >> In most cases the device is already gone.
> >>
> >> There is currently no way to insert RECONNECTS_USB into the return.
> >
> > Argh, I was blind! You are absolutely correct. It never returns value 1
> > (RECONNECTS_USB) from the .download_firmware().
> >
> > That patch is fine, I will apply it, thanks!
> >
> > I think that must be also changed to return immediately without
> > releasing the interface. Let the USB core release it when it detects
> > disconnect - otherwise it could crash as it tries to access potentially
> > resources that are already freed. Just for the timing issue if it
> > happens or not.
> >
> > } else if (ret == RECONNECTS_USB) {
> > ret = 0;
> > goto exit_usb_driver_release_interface;
> >
> > add return 0 here without releasing interface and test.
> >
> >
> >>> I tested it using one non-public Cypress FX2 device - it was changing
> >>> USB ID after the FX download, but from the driver perspective it does
> >>> not matter. It is always new device if it reconnects USB.
> >>>
> >>
> >> Have double checked that the thread is not continuing to write on the
> >> old ID?
> >
> > Nope, but likely delayed probe() is finished until it reconnects so I
> > cannot see problem. You device disconnects faster and thus USB core
> > traps .disconnect() earlier...
> >
> > Could you test returning 0 and if it works sent new patch.
> >
> >> The zero condition will lead to dvb_usb_init.
> >>
> >>> PS. as I looked that driver I saw many different firmwares. That is now
> >>> supported and you should use .get_firmware_name() (maybe you already did
> >>> it).
> >>>
> >> Yes, I have supported this in the driver.
> 
> Malcolm,
> 
> could you just test if returning from the routines after fw download is 
> enough to fix all your problems?
> 
> I mean those two fixes:
> dvb_usb_download_firmware()
> * return RECONNECTS_USB correctly
> 
> dvb_usbv2_init_work()
> * return without releasing USB interface if RECONNECTS_USB
Hi Antti,

Yes, I have tested it and there is no difference.

My understanding is, if the interface is no longer bound it returns
anyway.

It is best not to use it, other drivers in the dvb-usb tree may not like
to be forcibly unbound prior to their reset.

Regards

Malcolm

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Antti Palosaari]

On 06/15/2012 12:33 AM, Malcolm Priestley wrote:
> On Thu, 2012-06-14 at 23:31 +0300, Antti Palosaari wrote:
>> On 06/14/2012 04:15 AM, Antti Palosaari wrote:
>>> On 06/14/2012 03:44 AM, Malcolm Priestley wrote:
>>>> On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
>>>>> Hi Malcolm,
>>>>> I was really surprised someone has had interest to test that stuff at
>>>>> that phase as I did not even advertised it yet :) It is likely happen
>>>>> next Monday or so as there is some issues I would like to check / solve.
>>>>>
>>>>>
>>>>> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
>>>>>> Hi antti
>>>>>>
>>>>>> There some issues with dvb_usb_v2 with the lmedm04 driver.
>>>>>>
>>>>>> The first being this patch, no return value from
>>>>>> dvb_usb_download_firmware
>>>>>> causes system wide dead lock with COLD disconnect as system attempts
>>>>>> to continue
>>>>>> to warm state.
>>>>>
>>>>> Hmm, I did not understand what you mean. What I looked lmedm04 driver I
>>>>> think it uses single USB ID (no cold + warm IDs). So it downloads
>>>>> firmware and then reconnects itself from the USB bus?
>>>>> For that scenario you should "return RECONNECTS_USB;" from the driver
>>>>> .download_firmware().
>>>>>
>>>> If the device disconnects from the USB bus after the firmware download.
>>>>
>>>> In most cases the device is already gone.
>>>>
>>>> There is currently no way to insert RECONNECTS_USB into the return.
>>>
>>> Argh, I was blind! You are absolutely correct. It never returns value 1
>>> (RECONNECTS_USB) from the .download_firmware().
>>>
>>> That patch is fine, I will apply it, thanks!
>>>
>>> I think that must be also changed to return immediately without
>>> releasing the interface. Let the USB core release it when it detects
>>> disconnect - otherwise it could crash as it tries to access potentially
>>> resources that are already freed. Just for the timing issue if it
>>> happens or not.
>>>
>>> } else if (ret == RECONNECTS_USB) {
>>> ret = 0;
>>> goto exit_usb_driver_release_interface;
>>>
>>> add return 0 here without releasing interface and test.
>>>
>>>
>>>>> I tested it using one non-public Cypress FX2 device - it was changing
>>>>> USB ID after the FX download, but from the driver perspective it does
>>>>> not matter. It is always new device if it reconnects USB.
>>>>>
>>>>
>>>> Have double checked that the thread is not continuing to write on the
>>>> old ID?
>>>
>>> Nope, but likely delayed probe() is finished until it reconnects so I
>>> cannot see problem. You device disconnects faster and thus USB core
>>> traps .disconnect() earlier...
>>>
>>> Could you test returning 0 and if it works sent new patch.
>>>
>>>> The zero condition will lead to dvb_usb_init.
>>>>
>>>>> PS. as I looked that driver I saw many different firmwares. That is now
>>>>> supported and you should use .get_firmware_name() (maybe you already did
>>>>> it).
>>>>>
>>>> Yes, I have supported this in the driver.
>>
>> Malcolm,
>>
>> could you just test if returning from the routines after fw download is
>> enough to fix all your problems?
>>
>> I mean those two fixes:
>> dvb_usb_download_firmware()
>> * return RECONNECTS_USB correctly
>>
>> dvb_usbv2_init_work()
>> * return without releasing USB interface if RECONNECTS_USB
> Hi Antti,
>
> Yes, I have tested it and there is no difference.
>
> My understanding is, if the interface is no longer bound it returns
> anyway.
>
> It is best not to use it, other drivers in the dvb-usb tree may not like
> to be forcibly unbound prior to their reset.

I don't understand why this logic cannot work. Do you have some crash 
dump I can see likely functions in path?

I draw it here as a skeleton code.

dvb_usbv2_init_work()
   ret = download_firmware()
   if (ret == DEVICE_IS_WARM)
     init_device()
     return
   else if (ret == DEVICE_RECONECTS_USB)
     return
   else (ret == some error code)
     usb_driver_release_interface()
     return

dvb_usbv2_probe()
   state = alloc()
   usb_set_intfdata(state)
   schedule_work(dvb_usbv2_init_work)
   return 0

dvb_usbv2_disconnect()
   state = usb_get_intfdata()
   cancel_work_sync(dvb_usbv2_init_work)
   free(state)

Anyhow, I have devices I know how to force reconnect USB (AF9015, EC168) 
when needed. So I will make some tests here.

regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Malcolm Priestley]

On Fri, 2012-06-15 at 01:12 +0300, Antti Palosaari wrote:
> On 06/15/2012 12:33 AM, Malcolm Priestley wrote:
> > On Thu, 2012-06-14 at 23:31 +0300, Antti Palosaari wrote:
> >> On 06/14/2012 04:15 AM, Antti Palosaari wrote:
> >>> On 06/14/2012 03:44 AM, Malcolm Priestley wrote:
> >>>> On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
> >>>>> Hi Malcolm,
> >>>>> I was really surprised someone has had interest to test that stuff at
> >>>>> that phase as I did not even advertised it yet :) It is likely happen
> >>>>> next Monday or so as there is some issues I would like to check / solve.
> >>>>>
> >>>>>
> >>>>> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
> >>>>>> Hi antti
> >>>>>>
> >>>>>> There some issues with dvb_usb_v2 with the lmedm04 driver.
> >>>>>>
> >>>>>> The first being this patch, no return value from
> >>>>>> dvb_usb_download_firmware
> >>>>>> causes system wide dead lock with COLD disconnect as system attempts
> >>>>>> to continue
> >>>>>> to warm state.
> >>>>>
> >>>>> Hmm, I did not understand what you mean. What I looked lmedm04 driver I
> >>>>> think it uses single USB ID (no cold + warm IDs). So it downloads
> >>>>> firmware and then reconnects itself from the USB bus?
> >>>>> For that scenario you should "return RECONNECTS_USB;" from the driver
> >>>>> .download_firmware().
> >>>>>
> >>>> If the device disconnects from the USB bus after the firmware download.
> >>>>
> >>>> In most cases the device is already gone.
> >>>>
> >>>> There is currently no way to insert RECONNECTS_USB into the return.
> >>>
> >>> Argh, I was blind! You are absolutely correct. It never returns value 1
> >>> (RECONNECTS_USB) from the .download_firmware().
> >>>
> >>> That patch is fine, I will apply it, thanks!
> >>>
> >>> I think that must be also changed to return immediately without
> >>> releasing the interface. Let the USB core release it when it detects
> >>> disconnect - otherwise it could crash as it tries to access potentially
> >>> resources that are already freed. Just for the timing issue if it
> >>> happens or not.
> >>>
> >>> } else if (ret == RECONNECTS_USB) {
> >>> ret = 0;
> >>> goto exit_usb_driver_release_interface;
> >>>
> >>> add return 0 here without releasing interface and test.
> >>>
> >>>
> >>>>> I tested it using one non-public Cypress FX2 device - it was changing
> >>>>> USB ID after the FX download, but from the driver perspective it does
> >>>>> not matter. It is always new device if it reconnects USB.
> >>>>>
> >>>>
> >>>> Have double checked that the thread is not continuing to write on the
> >>>> old ID?
> >>>
> >>> Nope, but likely delayed probe() is finished until it reconnects so I
> >>> cannot see problem. You device disconnects faster and thus USB core
> >>> traps .disconnect() earlier...
> >>>
> >>> Could you test returning 0 and if it works sent new patch.
> >>>
> >>>> The zero condition will lead to dvb_usb_init.
> >>>>
> >>>>> PS. as I looked that driver I saw many different firmwares. That is now
> >>>>> supported and you should use .get_firmware_name() (maybe you already did
> >>>>> it).
> >>>>>
> >>>> Yes, I have supported this in the driver.
> >>
> >> Malcolm,
> >>
> >> could you just test if returning from the routines after fw download is
> >> enough to fix all your problems?
> >>
> >> I mean those two fixes:
> >> dvb_usb_download_firmware()
> >> * return RECONNECTS_USB correctly
> >>
> >> dvb_usbv2_init_work()
> >> * return without releasing USB interface if RECONNECTS_USB
> > Hi Antti,
> >
> > Yes, I have tested it and there is no difference.
> >
> > My understanding is, if the interface is no longer bound it returns
> > anyway.
> >
> > It is best not to use it, other drivers in the dvb-usb tree may not like
> > to be forcibly unbound prior to their reset.
> 
> I don't understand why this logic cannot work. Do you have some crash 
> dump I can see likely functions in path?
Sorry, you misunderstood me, there is no crash.

I was only talking using usb_driver_release_interface() or not.

The skeleton code below would work fine :-)

Regards

Malcolm

> 
> I draw it here as a skeleton code.
> 
> dvb_usbv2_init_work()
>    ret = download_firmware()
>    if (ret == DEVICE_IS_WARM)
>      init_device()
>      return
>    else if (ret == DEVICE_RECONECTS_USB)
>      return
>    else (ret == some error code)
>      usb_driver_release_interface()
>      return
> 
> dvb_usbv2_probe()
>    state = alloc()
>    usb_set_intfdata(state)
>    schedule_work(dvb_usbv2_init_work)
>    return 0
> 
> dvb_usbv2_disconnect()
>    state = usb_get_intfdata()
>    cancel_work_sync(dvb_usbv2_init_work)
>    free(state)
> 
> Anyhow, I have devices I know how to force reconnect USB (AF9015, EC168) 
> when needed. So I will make some tests here.
> 
> regards
> Antti

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Antti Palosaari]

On 06/15/2012 01:32 AM, Malcolm Priestley wrote:
> On Fri, 2012-06-15 at 01:12 +0300, Antti Palosaari wrote:
>> On 06/15/2012 12:33 AM, Malcolm Priestley wrote:
>>> On Thu, 2012-06-14 at 23:31 +0300, Antti Palosaari wrote:
>>>> On 06/14/2012 04:15 AM, Antti Palosaari wrote:
>>>>> On 06/14/2012 03:44 AM, Malcolm Priestley wrote:
>>>>>> On Thu, 2012-06-14 at 02:29 +0300, Antti Palosaari wrote:
>>>>>>> Hi Malcolm,
>>>>>>> I was really surprised someone has had interest to test that stuff at
>>>>>>> that phase as I did not even advertised it yet :) It is likely happen
>>>>>>> next Monday or so as there is some issues I would like to check / solve.
>>>>>>>
>>>>>>>
>>>>>>> On 06/14/2012 01:24 AM, Malcolm Priestley wrote:
>>>>>>>> Hi antti
>>>>>>>>
>>>>>>>> There some issues with dvb_usb_v2 with the lmedm04 driver.
>>>>>>>>
>>>>>>>> The first being this patch, no return value from
>>>>>>>> dvb_usb_download_firmware
>>>>>>>> causes system wide dead lock with COLD disconnect as system attempts
>>>>>>>> to continue
>>>>>>>> to warm state.
>>>>>>>
>>>>>>> Hmm, I did not understand what you mean. What I looked lmedm04 driver I
>>>>>>> think it uses single USB ID (no cold + warm IDs). So it downloads
>>>>>>> firmware and then reconnects itself from the USB bus?
>>>>>>> For that scenario you should "return RECONNECTS_USB;" from the driver
>>>>>>> .download_firmware().
>>>>>>>
>>>>>> If the device disconnects from the USB bus after the firmware download.
>>>>>>
>>>>>> In most cases the device is already gone.
>>>>>>
>>>>>> There is currently no way to insert RECONNECTS_USB into the return.
>>>>>
>>>>> Argh, I was blind! You are absolutely correct. It never returns value 1
>>>>> (RECONNECTS_USB) from the .download_firmware().
>>>>>
>>>>> That patch is fine, I will apply it, thanks!
>>>>>
>>>>> I think that must be also changed to return immediately without
>>>>> releasing the interface. Let the USB core release it when it detects
>>>>> disconnect - otherwise it could crash as it tries to access potentially
>>>>> resources that are already freed. Just for the timing issue if it
>>>>> happens or not.
>>>>>
>>>>> } else if (ret == RECONNECTS_USB) {
>>>>> ret = 0;
>>>>> goto exit_usb_driver_release_interface;
>>>>>
>>>>> add return 0 here without releasing interface and test.
>>>>>
>>>>>
>>>>>>> I tested it using one non-public Cypress FX2 device - it was changing
>>>>>>> USB ID after the FX download, but from the driver perspective it does
>>>>>>> not matter. It is always new device if it reconnects USB.
>>>>>>>
>>>>>>
>>>>>> Have double checked that the thread is not continuing to write on the
>>>>>> old ID?
>>>>>
>>>>> Nope, but likely delayed probe() is finished until it reconnects so I
>>>>> cannot see problem. You device disconnects faster and thus USB core
>>>>> traps .disconnect() earlier...
>>>>>
>>>>> Could you test returning 0 and if it works sent new patch.
>>>>>
>>>>>> The zero condition will lead to dvb_usb_init.
>>>>>>
>>>>>>> PS. as I looked that driver I saw many different firmwares. That is now
>>>>>>> supported and you should use .get_firmware_name() (maybe you already did
>>>>>>> it).
>>>>>>>
>>>>>> Yes, I have supported this in the driver.
>>>>
>>>> Malcolm,
>>>>
>>>> could you just test if returning from the routines after fw download is
>>>> enough to fix all your problems?
>>>>
>>>> I mean those two fixes:
>>>> dvb_usb_download_firmware()
>>>> * return RECONNECTS_USB correctly
>>>>
>>>> dvb_usbv2_init_work()
>>>> * return without releasing USB interface if RECONNECTS_USB
>>> Hi Antti,
>>>
>>> Yes, I have tested it and there is no difference.
>>>
>>> My understanding is, if the interface is no longer bound it returns
>>> anyway.
>>>
>>> It is best not to use it, other drivers in the dvb-usb tree may not like
>>> to be forcibly unbound prior to their reset.
>>
>> I don't understand why this logic cannot work. Do you have some crash
>> dump I can see likely functions in path?
> Sorry, you misunderstood me, there is no crash.
>
> I was only talking using usb_driver_release_interface() or not.
>
> The skeleton code below would work fine :-)

Hi Malcolm,

Are you fine now with these two patches and no further changes are needed?

http://git.linuxtv.org/anttip/media_tree.git/commit/2ced3f3b5763b4817c5a884b0980a9a7c87e4d96

http://git.linuxtv.org/anttip/media_tree.git/commit/ddd75d554b2b08cb112c756983301cb9418ccd42

regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/2] [BUG] dvb_usb_v2: return the download ret in dvb_usb_download_firmware

[Malcolm Priestley]

On Fri, 2012-06-15 at 02:14 +0300, Antti Palosaari wrote:

> Hi Malcolm,
> 
> Are you fine now with these two patches and no further changes are needed?
> 
> http://git.linuxtv.org/anttip/media_tree.git/commit/2ced3f3b5763b4817c5a884b0980a9a7c87e4d96
> 
> http://git.linuxtv.org/anttip/media_tree.git/commit/ddd75d554b2b08cb112c756983301cb9418ccd42

Yes, these are okay.

Regards


Malcolm