From: Subash Abhinov Kasiviswanathan <subashab@codeaurora•org>
To: Dan Williams <dcbw@redhat•com>
Cc: netdev@vger•kernel.org, davem@davemloft•net,
fengguang.wu@intel•com, jiri@resnulli•us,
stephen@networkplumber•org, David.Laight@aculab•com,
marcel@holtmann•org, andrew@lunn•ch
Subject: Re: [PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation
Date: Wed, 30 Aug 2017 15:19:19 -0600 [thread overview]
Message-ID: <c64415aa8d1184303f5916bdd2fd6ce3@codeaurora.org> (raw)
In-Reply-To: <1504103947.21231.5.camel@redhat.com>
> General comment; other drivers that do similar things (macvlan, ipvlan)
> use the term "port" to refer to what I think you're calling a
> "rmnet_real_dev_info". Maybe that's a shorter or less confusing term.
> Could be renamed later too, if you wanted to do so.
>
Hi Dan
I'll rename it to rmnet_port.
> Maybe this got elided during the revisions, but now I can't find
> anywhere that sets RMNET_LOCAL_LOGICAL_ENDPOINT. Looking at the
> callchain, there are two places that LOCAL_LOGICAL_ENDPOINT matters:
>
> rmnet_get_endpoint(): only ever called by __rmnet_set_endpoint_config()
>
> __rmnet_set_endpoint_config(): only called from
> rmnet_set_endpoint_config(); which itself is only called from
> rmnet_newlink().
>
> So the only place that 'config_id' is set, and thus that it could be
> LOCAL_LOGICAL_ENDPOINT, is rmnet_newlink() via 'mux_id'. But
> IFLA_VLAN_ID is a u16, and so I don't see anywhere that
> config_id/mux_id will ever be < 0, and thus anywhere that it could be
> LOCAL_LOGICAL_ENDPOINT.
>
> I could well just not be seeing it though...
>
> This function (__rmnet_set_endpoint_config) seems to only be called
> from rmnet_set_endpoint_config(). Perhaps just combine them?
>
> But that brings up another point; can the rmnet "mode" or egress_dev
> change at runtime, after the rmnet child has been created? I forget if
> that was possible with your original patchset that used ioctls.
>
The original series with IOCTL was able to change it.
With the current netlink based configuration, we are using a fixed
config
of muxing and the egress dev is fixed for its lifetime. Practically,
these
should never change for a set of rmnet devices attached to a real dev.
I will remove LOCAL_LOGICAL_ENDPOINT since it is unused.
> Why not set the mux_id in rmnet_vnd_newlink()?
>
> Also, bigger problem. r->rmnet_devices[] is only 32 items in size.
> But mux_id (which is used as an index into rmnet_devices in a few
> places) can be up to 255 (RMNET_MAX_LOGICAL_EP).
>
> So if you try to create an rmnet for mux ID 32, you panic the kernel.
> See below my comments about rmnet_real_dev_info...
>
I'll fix this.
> I can't see anywhere that the egress/ingress data get set except for
> this function, so perhaps you could just skip these functions and
> (since you already have 'r' from above) set r-
>> [egress|ingress]_data_format directly?
>
Yes, till this is made configurable, this need not be set separately.
> This means that the first time you add an rmnet dev to a netdev, it'll
> create a structure that's quite large (at least 255 * 6, but more due
> to padding), when in most cases few of these items will be used. Most
> of the time you'd have only a couple PDNs active, but this will
> allocate memory for MAX_LOGICAL_EP of them, no?
>
> ipvlan uses a list to track the child devices attached to a physical
> device so that it doesn't have to allocate them all at once and waste
> memory; that technique could replace the 'rmnet_devices' member below.
>
> It also uses a hash to find the actual ipvlan upperdev from the
> rx_handler of the lowerdev, which is probably what would replace
> muxed_ep[] here.
>
> Is the relationship between rmnet "child"/upper devs and mux_ids 1:1?
> Or can you have multiple rmnet devs for the same mux_id?
>
> Dan
We can have multiple rmnet devices having the same mux_id. They will
need to be attached to different real_dev though. I'll look into the
creation of hash for the lookup. Once I have the hash up, I should
be able to get rid of some of the structures.
The other main functionality which I am unsure is the
bridge handling - passing on MAP data from one real_dev to another.
Is there some to achieve this using any existing netlink attributes?
Any suggestions would be appreciated.
> Please implement ndo_get_iflink as well, so that it's easy to find out
> what the "parent"/lowerdev for a given rmnet interface is.
>
> That might mean adding a "phy_dev" member to rmnet_priv, but that might
> help you clean up a lot of other stuff too
>
Sure, I'll implement this. Let me know if you have more comments.
next prev parent reply other threads:[~2017-08-30 21:19 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-30 4:44 [PATCH net-next 0/3 v11] Add support for rmnet driver Subash Abhinov Kasiviswanathan
2017-08-30 4:44 ` [PATCH net-next 1/3 v11] net: ether: Add support for multiplexing and aggregation type Subash Abhinov Kasiviswanathan
2017-08-30 4:44 ` [PATCH net-next 2/3 v11] net: arp: Add support for raw IP device Subash Abhinov Kasiviswanathan
2017-08-30 4:44 ` [PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation Subash Abhinov Kasiviswanathan
2017-08-30 14:39 ` Dan Williams
2017-08-30 21:19 ` Subash Abhinov Kasiviswanathan [this message]
2017-08-30 21:27 ` David Miller
2017-08-30 21:40 ` Subash Abhinov Kasiviswanathan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c64415aa8d1184303f5916bdd2fd6ce3@codeaurora.org \
--to=subashab@codeaurora$(echo .)org \
--cc=David.Laight@aculab$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=dcbw@redhat$(echo .)com \
--cc=fengguang.wu@intel$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=marcel@holtmann$(echo .)org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=stephen@networkplumber$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox