Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

On Wed, Feb 09, 2011 at 04:10:55PM -0800, Ira W. Snyder wrote:

On Wed, Feb 09, 2011 at 03:42:31PM -0800, Dmitry Torokhov wrote:

quoted

On Wed, Feb 09, 2011 at 03:35:45PM -0800, Ira W. Snyder wrote:

quoted

On Wed, Feb 09, 2011 at 10:27:40AM -0800, Dmitry Torokhov wrote:

[ snip stuff I've already fixed in the next version ]

quoted

quoted

The requirement is that the device stay open during reconfiguration.
This provides for that. Readers just block for as long as the device is
not producing data.

OK, you still need to make sure you do not touch free/used buffer while
device is disabled. Also, you need to kick readers if you unbind the
driver, so maybe a new flag priv->exists should be introduced and
checked.

I don't understand what you mean by "kick readers if you unbind the
driver". The kernel automatically increases the refcount on a module
when a process is using the module. This shows up in the "Used by"
column of lsmod's output.

The kernel will not let you rmmod a module with a non-zero refcount. You
cannot get into the situation where you have rmmod'ed the module and a
reader is still blocking in read()/poll().

However you can still unbind the driver from the device by writing into
driver's sysfs 'unbind' attribute.

See drivers/base/bus.c::driver_unbind().

I was completely unaware of that "feature". I hunch that many drivers
are incapable of dealing with an unbind while they are still open.

Hmm, maybe older drivers... Anythig hotpluggable (USB, PCI, etc) should
be in a better shape because they expect to be yanked at any time.

Matter of fact, I don't see how this can EVER be safe. The driver core
automatically calls the data_of_remove() routine while there are still
blocked readers. This kfree()s the private data structure, which
contains the suggested priv->exists flag. What happens if the memory
allocator re-allocates that memory to a different driver before the
reader process is woken up to check the priv->exists flag?

The only way to solve this is to count the number of open()s and
close()s, and block the unbind until all users have close()d the device.

Yes, you can kick readers and wait, or you can refcount that private
structure and have readers grab a reference when they open your device
and drop it in their fops->release() method. Your remove() should also
drop reference instead of doing kfree() outright.

Thanks.

-- 
Dmitry