On Tue, Nov 17, 2020 at 12:26:38PM +0000, Tom Parkin wrote: > On Sun, Nov 15, 2020 at 17:28:38 +0100, Guillaume Nault wrote: > > On Tue, Nov 10, 2020 at 12:04:29PM +0000, Tom Parkin wrote: > > > On Tue, Nov 10, 2020 at 00:24:01 +0100, Guillaume Nault wrote: > > > However, my primary motivation for using ppp_channel_push was actually > > > the handling for managing dropping the packet if the channel was > > > deregistered. > > > > I might be missing something, but I don't see what ppp_channel_push() > > does appart from holding the xmit lock and handling the xmit queue. > > If we agree that there's no need to use the xmit queue, all > > ppp_channel_push() does for us is taking pch->downl, which we probably > > can do on our own. > > > > > It'd be simple enough to add another function which performed the same > > > deregistration check but didn't transmit via. the queue. > > > > That's probably what I'm missing: what do you mean by "deregistration > > check"? I can't see anything like this in ppp_channel_push(). > > It's literally just the check on pch->chan once pch->downl is held. > So it would be trivial to do the same thing in a different codepath: I > just figured why reinvent the wheel :-)
Okay, I was thinking of something more complex. I agree with not reinventing existing functions, but in this case, I think that ppp_channel_push() does too many unecessary operations (like recursion handling and processing the parent unit's queue). Also, a helper function would be the natural place for calling skb_scrub_packet() and for handling concurent access or modification of the ->bridge pointer (as discussed earlier in this thread). > > Sorry for the confusion. No problem. It's nice to see some work being done in this area :).
