Latest version of em28xx / em28xx-dvb patch for PCTV 290e

  Hi,

Here's my latest patch for the em28xx / em28xx-dvb modules, which addresses the 
following problems:

a) Locking problem when unplugging and then replugging USB adapter.
b) Race conditions between adding/removing devices from the devlist, while 
simultaneously loading/unloading extension modules.
c) Resource leaks on error paths.
d) Preventing the DVB framework from trying to put the adapter into sleep mode 
when the adapter has been physically unplugged. (This results in occasional 
"-19" errors from I2C functions when disconnecting.)
e) Use atomic bit operations to manage "device in use" slots, and enforce upper 
limit of EM28XX_MAXBOARDS slots.

BTW, was there a reason why the em28xx-dvb module doesn't use dvb-usb?

Any review comments welcome,

Cheers,
Chris
  

On Thu, Aug 18, 2011 at 1:52 PM, Chris Rankin <rankincj@yahoo.com> wrote:
> Hi,
>
> Here's my latest patch for the em28xx / em28xx-dvb modules, which addresses
> the following problems:
>
> a) Locking problem when unplugging and then replugging USB adapter.
> b) Race conditions between adding/removing devices from the devlist, while
> simultaneously loading/unloading extension modules.
> c) Resource leaks on error paths.
> d) Preventing the DVB framework from trying to put the adapter into sleep
> mode when the adapter has been physically unplugged. (This results in
> occasional "-19" errors from I2C functions when disconnecting.)
> e) Use atomic bit operations to manage "device in use" slots, and enforce
> upper limit of EM28XX_MAXBOARDS slots.

Hi Chris,

You would be well served to break this into a patch series, as it
tends to be difficult to review a whole series of changes in a single
patch.

You seem to be mixed in a bunch of "useless" changes alongside
functional changes.  For example, if you're trying to add in a missing
goto inside an exception block, doing that at the same time as
renaming instances of "errCode" to "retval" just creates confusion.

And finally, the mutex structure used for the modules is somewhat
complicated due to to the need to keep the analog side of the board
locked while initializing the digital side.  This code was added
specifically to prevent race conditions that were seen during
initialization as things like udev and dbus attempted to connect to
the newly created V4L device while the em28xx-dvb module was still
coming up.

In other words, I don't doubt there are bugs, and I cannot say whether
your fixes are appropriate without giving a hard look at the logic.
But you should be aware of the thinking behind the way it was done and
it would be very worthwhile if you could test with at least one hybrid
product to ensure the changes you are making don't break anything (the
em2874 used in the 290e is a poor test case since it doesn't have
analog support).

> BTW, was there a reason why the em28xx-dvb module doesn't use dvb-usb?

This is largely a product of the history of the devices using the
framework.  The em28xx driver was originally analog only, and DVB
support was added later as new chips came out which needed it.  The
dvb-usb driver came from dedicated DVB devices that had no analog
support.  In fact, even today the lack of analog support is a huge
deficiency in that framework which is why we don't support the analog
side of any hybrid devices that use dvb-usb.

In other words, if we were reinventing this stuff today, there would
probably be only a single framework shared by dvb-usb and em28xx.  But
at this point it's too much cost and too little benefit to go through
the work to attempt to merge them.

Devin
  

On Thu, Aug 18, 2011 at 2:43 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Thu, Aug 18, 2011 at 1:52 PM, Chris Rankin <rankincj@yahoo.com> wrote:
>> Hi,
>>
>> Here's my latest patch for the em28xx / em28xx-dvb modules, which addresses
>> the following problems:
>>
>> a) Locking problem when unplugging and then replugging USB adapter.
>> b) Race conditions between adding/removing devices from the devlist, while
>> simultaneously loading/unloading extension modules.
>> c) Resource leaks on error paths.
>> d) Preventing the DVB framework from trying to put the adapter into sleep
>> mode when the adapter has been physically unplugged. (This results in
>> occasional "-19" errors from I2C functions when disconnecting.)
>> e) Use atomic bit operations to manage "device in use" slots, and enforce
>> upper limit of EM28XX_MAXBOARDS slots.
>
> Hi Chris,
>
> You would be well served to break this into a patch series, as it
> tends to be difficult to review a whole series of changes in a single
> patch.
>
> You seem to be mixed in a bunch of "useless" changes alongside
> functional changes.  For example, if you're trying to add in a missing
> goto inside an exception block, doing that at the same time as
> renaming instances of "errCode" to "retval" just creates confusion.
>
> And finally, the mutex structure used for the modules is somewhat
> complicated due to to the need to keep the analog side of the board
> locked while initializing the digital side.  This code was added
> specifically to prevent race conditions that were seen during
> initialization as things like udev and dbus attempted to connect to
> the newly created V4L device while the em28xx-dvb module was still
> coming up.
>
> In other words, I don't doubt there are bugs, and I cannot say whether
> your fixes are appropriate without giving a hard look at the logic.
> But you should be aware of the thinking behind the way it was done and
> it would be very worthwhile if you could test with at least one hybrid
> product to ensure the changes you are making don't break anything (the
> em2874 used in the 290e is a poor test case since it doesn't have
> analog support).
>
>> BTW, was there a reason why the em28xx-dvb module doesn't use dvb-usb?
>
> This is largely a product of the history of the devices using the
> framework.  The em28xx driver was originally analog only, and DVB
> support was added later as new chips came out which needed it.  The
> dvb-usb driver came from dedicated DVB devices that had no analog
> support.  In fact, even today the lack of analog support is a huge
> deficiency in that framework which is why we don't support the analog
> side of any hybrid devices that use dvb-usb.
>
> In other words, if we were reinventing this stuff today, there would
> probably be only a single framework shared by dvb-usb and em28xx.  But
> at this point it's too much cost and too little benefit to go through
> the work to attempt to merge them.
>
> Devin
>
> --
> Devin J. Heitmueller - Kernel Labs
> http://www.kernellabs.com
>

Probably one more point worth making:  I definitely appreciate that
you've take the time to focus on these particular problems.  I've been
complaining about them for months but just never got around to rolling
up my sleeves to debug them myself.

In other words, don't interpret anything in my previous email as discouragement.

Devin
  

On 18/08/11 19:43, Devin Heitmueller wrote:
> You would be well served to break this into a patch series, as it tends to be 
> difficult to review a whole series of changes in a single patch. You seem to 
> be mixed in a bunch of "useless" changes alongside functional changes. For 
> example, if you're trying to add in a missing goto inside an exception block, 
> doing that at the same time as renaming instances of "errCode" to "retval" 
> just creates confusion.

Actually, those two particular changes go together because I'm replacing "return 
errCode" with a goto to "return retval". Ultimately, errCode becomes an unused 
variable.

> And finally, the mutex structure used for the modules is somewhat complicated 
> due to to the need to keep the analog side of the board locked while 
> initializing the digital side. This code was added specifically to prevent 
> race conditions that were seen during initialization as things like udev and 
> dbus attempted to connect to the newly created V4L device while the em28xx-dvb 
> module was still coming up.

OK, thanks. I've been tackling this problem from the "We must always take lock A 
before lock B, and never vice versa" point of view. So the order is:

- take device mutex
- enter em28xx_init_dev()
- enter em28xx_init_extension()
- take device list mutex
- call init() function for every extension with this device

Since dvb_init() is the init() function for the em28xx-dvb extension, it follows 
that it cannot take the device's mutex again. The problem is therefore moved to 
em28xx_dvb_register(), which takes the device list mutex and yet MUST not take 
the mutex for any device in the list.

Combining em28xx_add_into_devlist() with em28xx_init_extension() (and similarly 
em28xx_remove_from_devlist() with em28xx_close_extension()) means that the 
device list must always contain a list of devices that has been initialised 
against every extension in the extension list.

I can probably factor out the simpler patches first, such as using the bit 
operations on em28xx_devused, and the memory leak in em28xx_v4l2_close(). And 
the spelling fixes...

Cheers,
Chris

--
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
  

Em 18-08-2011 12:56, Chris Rankin escreveu:
> On 18/08/11 19:43, Devin Heitmueller wrote:
>> You would be well served to break this into a patch series, as it tends to be difficult to review a whole series of changes in a single patch. You seem to be mixed in a bunch of "useless" changes alongside functional changes. For example, if you're trying to add in a missing goto inside an exception block, doing that at the same time as renaming instances of "errCode" to "retval" just creates confusion.
> 
> Actually, those two particular changes go together because I'm replacing "return errCode" with a goto to "return retval". Ultimately, errCode becomes an unused variable.
> 
>> And finally, the mutex structure used for the modules is somewhat complicated due to to the need to keep the analog side of the board locked while initializing the digital side. This code was added specifically to prevent race conditions that were seen during initialization as things like udev and dbus attempted to connect to the newly created V4L device while the em28xx-dvb module was still coming up.
> 
> OK, thanks. I've been tackling this problem from the "We must always take lock A before lock B, and never vice versa" point of view. So the order is:
> 
> - take device mutex
> - enter em28xx_init_dev()
> - enter em28xx_init_extension()
> - take device list mutex
> - call init() function for every extension with this device
> 
> Since dvb_init() is the init() function for the em28xx-dvb extension, it follows that it cannot take the device's mutex again. The problem is therefore moved to em28xx_dvb_register(), which takes the device list mutex and yet MUST not take the mutex for any device in the list.
> 
> Combining em28xx_add_into_devlist() with em28xx_init_extension() (and similarly em28xx_remove_from_devlist() with em28xx_close_extension()) means that the device list must always contain a list of devices that has been initialised against every extension in the extension list.
> 
> I can probably factor out the simpler patches first, such as using the bit operations on em28xx_devused, and the memory leak in em28xx_v4l2_close(). And the spelling fixes...

I agree with Devin: please break it into one patch with just the mutex
fixes, and another one(s) with the cleanups. The basic rule is one patch
per logic change. 

For the mutex patch, please add:
"Cc: stable@kernel.org" at the line before your signed-off-by, as such
patch should be backported to stable kernels.

With respect to return errCode, maybe the better would be to
rename it as just "rc". It is very common to call the return
code as that on several places, and it is smaller.

PS.: I didn't make a full review of your patches yet. I'll probably
do it by the next week, after returning from LinuxCon.

Thanks!
Mauro
--
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