From mboxrd@z Thu Jan 1 00:00:00 1970 From: Subash Abhinov Kasiviswanathan Subject: Re: [PATCH net-next] net: qmi_wwan: Add pass through mode Date: Wed, 27 Jun 2018 18:38:56 -0600 Message-ID: References: <1530066614-24995-1-git-send-email-subashab@codeaurora.org> <87a7rg8xqv.fsf@miraculix.mork.no> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Cc: dnlplm@gmail.com, dcbw@redhat.com, davem@davemloft.net, netdev@vger.kernel.org, Aleksander Morgado To: =?UTF-8?Q?Bj=C3=B8rn_Mork?= Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:39210 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752178AbeF1Ai5 (ORCPT ); Wed, 27 Jun 2018 20:38:57 -0400 In-Reply-To: <87a7rg8xqv.fsf@miraculix.mork.no> Sender: netdev-owner@vger.kernel.org List-ID: > The concepte looks fine to me, but I have a few comments to the > implementation below. > > First: I missed the last part of the discussion around automatic > detection of passthrough mode. Could you give us a short summary of > the > alternatives you tried and why they were dropped? Hi Bjørn The other approach was to explicitly check for the rx_handler of the qmi_wwan netdev and if it was attached by rmnet driver (rmnet_rx_handler). If it was indeed attached, then the packet would be queued to stack. I had dropped this option since it would mean that atleast the rmnet rx_handler would have to be exported and exposed through a new header. This also brings a tight coupling between qmi_wwan and rmnet driver. > IIUC, userspace will be responsible for doing something like this to > set > up a rmnet map interface: > > 1) set the qmi_wwan netdev mode to raw-ip using sysfs > 2) set the qmi_wwan netdev mode to pass-through using syfs > 3) bind an rmnet netdev to the qmi_wwan netdev using netlink > 4) configure the device for raw-ip using qmi > 5) configure the device for map using qmi > > I've just had a quick glance at this, but this function looks like an > almost exact copy of the raw_ip_store(). Why not share that code > instead of copying it? > > And while you're at it: There is nothing preventing us from turning on > raw-ip here instead of failing if it is off, us there? I.e. why not > set QMI_WWAN_FLAG_RAWIP when QMI_WWAN_FLAG_PASS_THROUGH is being set, > and clear QMI_WWAN_FLAG_PASS_THROUGH when QMI_WWAN_FLAG_RAWIP is being > cleared? > > You're missing the inverse relationship, aren't you? There is nothing > preventing the user from turning off raw-ip again after setting > pass-through. > > Do we really need all the notifier stuff here? You don't change the > qmi_wwan netdev since that's already taken care of when setting > QMI_WWAN_FLAG_RAWIP. > > AFAICS, QMI_WWAN_FLAG_PASS_THROUGH can be changed without any locking, > notifications or netdev state restrictions. All it does is change the > behaviour on rx, and there is no reason that can't be applied from the > next packet even on a running interface. > > We could make pass_through_store just call raw_ip_store (or the part > of it which matters, factored out into a separate function), if > necessary. The rest of pass_through_store just sets or clears the flag. > There is no need to do more than that, is there? > > And as noted above, raw_ip_store must also clear > QMI_WWAN_FLAG_PASS_THROUGH if clearing QMI_WWAN_FLAG_RAWIP. > > There is no need testing for rawip here, since you enforce that in > pass_through_store(). I can make these changes. With this, steps 1 and 2 are combined. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project