Re: [PROPOSED PATCH] ATM refcount + firestream

[Andrew Morton <andrewm@uow.edu.au>]

Patrick van de Lageweg wrote:
> 
> Hi all,
> 
> Here is the second try for the atm refcount problem. I've made made
> several enhancement over the previos patch. Can you take a look at it if
> I've missed anything? (This time it also includes the driver for the
> firestream card. That's why the patch is so large. It's gziped and
> uuencoded).

Patrick, I looked at the modules stuff and you do not
appear to be actually _using_ it anywhere:

bix:/home/morton> grep owner patch
+  owner:       THIS_MODULE,
+       owner:          THIS_MODULE
+       owner:          THIS_MODULE,
+       owner:        THIS_MODULE,
+  owner:       THIS_MODULE,
+       owner:          THIS_MODULE,
+   owner:      THIS_MODULE,
+       struct module *owner;
+       struct module *owner;
bix:/home/morton>


It looks like you'll need something like the following:
(warning: uncompiled ATM-ignoramus code)

Index: net/atm/common.c
===================================================================
RCS file: /opt/cvs/lk/net/atm/common.c,v
retrieving revision 1.3.2.1
diff -u -u -r1.3.2.1 common.c
--- net/atm/common.c	2000/07/08 06:26:43	1.3.2.1
+++ net/atm/common.c	2000/10/27 11:17:45
@@ -144,6 +144,8 @@
 			    "rx_inuse == %d after closing\n",
 			    atomic_read(&vcc->rx_inuse));
+		if (vcc->dev->ops->owner)
+			__MOD_DEC_USE_COUNT(vcc->dev->ops->owner);
 		bind_vcc(vcc,NULL);
 	}
 	if (free_sk) free_atm_vcc_sk(sk);
 }
@@ -199,13 +201,22 @@
 {
 	int error;
 
+	if (try_inc_mod_count(dev->ops->owner) == 0) {
+		return -ENODEV;
+	}
+
+	error = 0;
+
 	if ((vpi != ATM_VPI_UNSPEC && vpi != ATM_VPI_ANY &&
 	    vpi >> dev->ci_range.vpi_bits) || (vci != ATM_VCI_UNSPEC &&
-	    vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits))
-		return -EINVAL;
-	if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE))
-		return -EPERM;
-	error = 0;
+	    vci != ATM_VCI_ANY && vci >> dev->ci_range.vci_bits)) {
+		error = -EINVAL;
+		goto out;
+	}
+	if (vci > 0 && vci < ATM_NOT_RSV_VCI && !capable(CAP_NET_BIND_SERVICE)) {
+		error = -EPERM;
+		goto out;
+	}
 	bind_vcc(vcc,dev);
 	switch (vcc->qos.aal) {
 		case ATM_AAL0:
@@ -231,19 +242,26 @@
 	if (!error) error = adjust_tp(&vcc->qos.rxtp,vcc->qos.aal);
 	if (error) {
 		bind_vcc(vcc,NULL);
-		return error;
+		goto out;
 	}
 	DPRINTK("VCC %d.%d, AAL %d\n",vpi,vci,vcc->qos.aal);
 	DPRINTK("  TX: %d, PCR %d..%d, SDU %d\n",vcc->qos.txtp.traffic_class,
 	    vcc->qos.txtp.min_pcr,vcc->qos.txtp.max_pcr,vcc->qos.txtp.max_sdu);
 	DPRINTK("  RX: %d, PCR %d..%d, SDU %d\n",vcc->qos.rxtp.traffic_class,
 	    vcc->qos.rxtp.min_pcr,vcc->qos.rxtp.max_pcr,vcc->qos.rxtp.max_sdu);
+
 	if (dev->ops->open) {
 		error = dev->ops->open(vcc,vpi,vci);
 		if (error) {
 			bind_vcc(vcc,NULL);
-			return error;
+			goto out;
 		}
+	}
+
+out:
+	if (error) {
+		if (dev->ops->owner)
+			__MOD_DEC_USE_COUNT(dev->ops->owner);
 	}
 	return 0;
 }


Something similar will be need to be wrapped around the usage of
`struct atm_tcp_ops()' as well.  Let me know if you'd like me to
prototype a patch for that.

The other thing you need to watch out for is atmdev_ops.ioctl().
Can this be called when the device is not open?

In other words, can atmdev_ops.ioctl() be called prior to
atmdev_ops.open()?  In more other words, can ioctl() be
called after close()?

If so then the above patch is not sufficient - it only increments
the module use count on the open() path.

If this is the case then you're fairly severely screwed.  This is
because the atm_dev handling has the same design flaw as the
netdevice handling: the logical place to inc/dec the module
refcount is within atm_dev_[de]register().  But this doesn't
work because you can never _get_ to the deregister point
through sys_delete_module() to drop the refcount.

Like netdevices, ATM needs to be able to separate the act
of loading the module from the act of registering the driver.

netdevices manage to get away with it because of ANK's funky
dev_hold()/dev_put() refcounting.  It looks like ATM devices
aren't that lucky.

One workaround would be to refuse to allow the device to be
accessed at all if it isn't open.  This may be unacceptable.


Look, this modules stuff is really bad.  Phillip Rumpf proposed
a radical alternative a while back which I felt was not given
sufficient consideration.  The idea was to make sys_delete_module()
grab all the other CPUs and leave them spinning on a flag while
the unload was proceeding.  This was very similar to
arch/i386/kernel/apm.c:apm_power_off().

As far as I can recall, the only restriction was that you are
not allowed to call module functions when the module refcount
is zero if those functions can call schedule().

prumpf, please dig out that patch.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/