[PATCH] info to atm layer about broken connection

[Duncan Sands]

Hi Stanislaw,

> > > simply after disconnect modem, ppp demon still works 
> > > and ifconfig command show me connection is ok.
> > > Using vcc->push(vcc, NULL) kill pppd with
> > > Modem hangup message in logs.
> > 
> > that sounds like a bug in the pppoatm driver.  I will look into this.
> I don't know, however it seems pushing NULL is ok.
> I found in net/atm/pppoatm.c :
> 
> /* Called when an AAL5 PDU comes in */
> static void pppoatm_push(struct atm_vcc *atmvcc, struct sk_buff *skb)
> {
> 	struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
> 	DPRINTK("pppoatm push\n");
> 	if (skb == NULL) {			/* VCC was closed */
> 		DPRINTK("removing ATMPPP VCC %p\n", pvcc);
> 		pppoatm_unassign_vcc(atmvcc);
> 		atmvcc->push(atmvcc, NULL);	/* Pass along bad news */
> 		return;
> 	}
> 
> 
> and in net/atm/raw.c :
> 
> /*
>  * SKB == NULL indicates that the link is being closed
>  */
> 
> static void atm_push_raw(struct atm_vcc *vcc,struct sk_buff *skb)
> {
> 	if (skb) {
> 		struct sock *sk = sk_atm(vcc);
> 
> 		skb_queue_tail(&sk->sk_receive_queue, skb);
> 		sk->sk_data_ready(sk, skb->len);
> 	}
> }

Here's what Chas Williams (ATM maintainer told me):

"... I believe that all
you should need to do is go through the list of vcc's that belong to your
card and call vcc_release_async().  the next time the user space process
that has control of the socket belonging to the vcc will get an error
and close the socket.  the socket release() is responsible for sending
the NULL skb to the next protocol layer."

So indeed a NULL skb should be sent, but not by the driver.  That's why
I say it looks like a bug in the ATM layer.

> > > > Also, as far as I can see the use of try_module_get is pointless.  Why do
> > > > you think it is needed?
> > > It depends on approach, someone would like to finish connection when 
> > > net driver module is unloaded, other one would like to prevent unloading 
> > > module. I preffer second approach as is in (for example) pppoatm .
> > >
> > > Adding try_module_get and module_put pair in subdriver atm_start() and atm_stop() 
> > > propably block unload module anyway.
> > 
> > It would, which is why I don't want it.  Can you please explain why it is
> > useful to have the connection be gone before the module unload completes?
> Remain connection when device is gone give some problems (at least on pppoatm): 
> - you can read/write to not working interface and there is no indication 
> for user the link is broken

This has nothing to do with module unloading.  You are explaining why you want
the connection to be shut down when the device is removed.  I agree that the
connection should be shutdown.  I guess I wasn't clear: my question was: why
is it important that the connection be completely shutdown before the rmmod
completes, rather than (maybe) soon afterwards.  This is the difference between
doing a synchronous shutdown (in which you tell the ATM layer to shutdown, and
you wait for the shutdown to complete before allowing the module unload to finish),
and an asynchronous shutdown (in which you tell the ATM layer to shutdown, but
don't wait for it to complete).

> - you must reload atm module before you can establish new connection, 
> even when old one was disable. 	

Same remark as above.

> > I don't really have anything against it, except that this kind of
> > synchronous unloading can easily cause rmmod to hang indefinitely (for
> > example: the output of rmmod is piped to a socket that goes through the
> > ATM device; I don't know if this is really possible, just giving you
> > the idea), so better to avoid it if possible.  
> I hope higher layers properly handle this race condition possibility.

It's not a race condition, and in a sense there's nothing that can be done
about it.  If A is waiting for B, but B is waiting for A, you get a deadlock,
that's all.  The trick is to make sure this never happens, by making sure that
A or B doesn't wait.  For example, if, on module unload, the driver code doesn't
wait for anything, there is no problem.  That is the case now, and I'd rather
not change it.  But it seems to me you don't really want synchronous shutdown
anyway: you just want the connection to be killed.  

> > For example, the proc 
> > filesystem entries /proc/net/atm/speed* can also continue to exist after 
> > the speedtch module is unloaded, or the device is removed, because it
> > would be much harder to do it differently.
> 
> Ok, but if we call read on unloaded procfs entries we get error 
> (propably -ENOENT),  with internet connection there is no error,
> only sleeping forever.

Sure, I agree there should be an error in the internet connection case too
(this has been on my TODO list for an awfully long time).  But I don't see
what this has to do with adding calls to try_module_get.

> Summary:
>  - Info to atm layer that link is broken when device is removed or module unloaded is necessary. 

I agree.

>  - Prevent subdriver module unloading when there is working connection is nice (lets root 
> disable connection (kill pppd or atmarpd) before unload module).

You still haven't explained why this is useful.  You've only explained why breaking
the link is a good thing.

Ciao,

D.