Re: [PATCH net] qmi_wwan: Clone the skb when in pass-through mode

From: Kristian Evensen <kristian.evensen@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "Bjørn Mork" <bjorn@mork.no>,
	"Network Development" <netdev@vger.kernel.org>,
	subashab@codeaurora.org
Subject: Re: [PATCH net] qmi_wwan: Clone the skb when in pass-through mode
Date: Tue, 15 Jun 2021 11:03:24 +0200	[thread overview]
Message-ID: <CAKfDRXgQLvTpeowOe=17xLqYbVRcem9N2anJRSjMcQm6=OnH1A@mail.gmail.com> (raw)
In-Reply-To: <20210614130530.7a422f27@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

Hi Jakub,

On Mon, Jun 14, 2021 at 10:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
> Agreed on the cloning being a strange solution. Kristian, were you able
> to reproduce the problem on upstream kernels?

Yes, after Bjørn's comment I realized that cloning was not a good
solution. I should have taken a closer look at the usbnet code, so
sorry about that. The most recent kernel I have managed to work well
with my boards is 5.4.123, but I see that 5.10 is available as well
(OpenWrt). However, I have backported all changes made to rmnet and
qmi_wwan between 5.4 and net-next as of yesterday. I was hoping that a
backport of the changes to those two drivers would be enough, but
perhaps there is something I have missed. I will try to get 5.10 to
run and see if that helps.

However, I have spent some more time looking into the code today.
Bjørn is right that calling netif_rx() from qmi_wwan is strange, at
least when in passthrough mode. The rx_fixup function will return 1
(assuming netif_rx() is successful), which in turn will lead to
usbnet_skb_return() being called and netif_rx() being called a second
time for the same skb. I have to admit that I don't know what will
happen when netif_rx() is called twice, but either call seems
redundant. I will submit a patch modifying the qmi_wwan rx_fixup
function to return 1 when the QMI_WWAN_FLAG_PASS_THROUGH. I believe it
is a nice clean-up and that is better to use as much of the existing
infrastructure as possible.

> It does look pretty strange that qmimux_rx_fixup() copies out all
> packets and receives them, and then let's usbnet to process the
> multi-frame skb without even fulling off the qmimux_hdr. I'm probably
> missing something.. otherwise sth like FLAG_MULTI_PACKET may be in
> order?

qmimux_rx_fixup() is different from what we are discussing here.
qmimux_rx_fixup() is used when the de-aggregation is performed by the
qmi_wwan driver, while the passthrough flag is set when the
de-aggregation is done by the rmnet driver. The logic in
qmimux_rx_fixup() is very similar to how the other usbnet mini-drivers
handles de-aggregation and also how de-aggregation is handled by for
example rmnet. I have no opinion on if the logic makes sens or not,
but at least the origin can be traced :)

Kristian