Re: [PATCH net-next] net: qmi_wwan: Add pass through mode

[Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>]

> 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