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

[Antti Palosaari <crope@iki.fi>]

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/