Re: [PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

2017-08-30 Thread Subash Abhinov Kasiviswanathan 

Subash, keep in mind that since I applied your v11 patches already you
will need to send me relative fixes and changes at this point, rather
than resubmit the series.

Thank you.


Thanks for the heads up David. Will do.
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

2017-08-30 Thread David Miller 
From: Subash Abhinov Kasiviswanathan 
Date: Wed, 30 Aug 2017 15:19:19 -0600

> Sure, I'll implement this. Let me know if you have more comments.

Subash, keep in mind that since I applied your v11 patches already you
will need to send me relative fixes and changes at this point, rather
than resubmit the series.

Thank you.

Re: [PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

2017-08-30 Thread Subash Abhinov Kasiviswanathan 

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.

--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

2017-08-30 Thread Dan Williams 
On Tue, 2017-08-29 at 22:44 -0600, Subash Abhinov Kasiviswanathan
wrote:
> RmNet driver provides a transport agnostic MAP (multiplexing and
> aggregation protocol) support in embedded module. Module provides
> virtual network devices which can be attached to any IP-mode
> physical device. This will be used to provide all MAP functionality
> on future hardware in a single consistent location.

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.

> Signed-off-by: Subash Abhinov Kasiviswanathan
> 
> ---
>  Documentation/networking/rmnet.txt |  82 
>  drivers/net/ethernet/qualcomm/Kconfig  |   2 +
>  drivers/net/ethernet/qualcomm/Makefile |   2 +
>  drivers/net/ethernet/qualcomm/rmnet/Kconfig|  12 +
>  drivers/net/ethernet/qualcomm/rmnet/Makefile   |  10 +
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 419
> +
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h |  56 +++
>  .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c   | 271
> +
>  .../net/ethernet/qualcomm/rmnet/rmnet_handlers.h   |  26 ++
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h|  88 +
>  .../ethernet/qualcomm/rmnet/rmnet_map_command.c| 107 ++
>  .../net/ethernet/qualcomm/rmnet/rmnet_map_data.c   | 105 ++
>  .../net/ethernet/qualcomm/rmnet/rmnet_private.h|  45 +++
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c| 170 +
>  drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h|  29 ++
>  15 files changed, 1424 insertions(+)
>  create mode 100644 Documentation/networking/rmnet.txt
>  create mode 100644 drivers/net/ethernet/qualcomm/rmnet/Kconfig
>  create mode 100644 drivers/net/ethernet/qualcomm/rmnet/Makefile
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.h
>  create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>  create mode 100644
> drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h
>  create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
>  create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h
> 
> diff --git a/Documentation/networking/rmnet.txt
> b/Documentation/networking/rmnet.txt
> new file mode 100644
> index 000..6b341ea
> --- /dev/null
> +++ b/Documentation/networking/rmnet.txt
> @@ -0,0 +1,82 @@
> +1. Introduction
> +
> +rmnet driver is used for supporting the Multiplexing and aggregation
> +Protocol (MAP). This protocol is used by all recent chipsets using
> Qualcomm
> +Technologies, Inc. modems.
> +
> +This driver can be used to register onto any physical network device
> in
> +IP mode. Physical transports include USB, HSIC, PCIe and IP
> accelerator.
> +
> +Multiplexing allows for creation of logical netdevices (rmnet
> devices) to
> +handle multiple private data networks (PDN) like a default internet,
> tethering,
> +multimedia messaging service (MMS) or IP media subsystem (IMS).
> Hardware sends
> +packets with MAP headers to rmnet. Based on the multiplexer id,
> rmnet
> +routes to the appropriate PDN after removing the MAP header.
> +
> +Aggregation is required to achieve high data rates. This involves
> hardware
> +sending aggregated bunch of MAP frames. rmnet driver will de-
> aggregate
> +these MAP frames and send them to appropriate PDN's.
> +
> +2. Packet format
> +
> +a. MAP packet (data / control)
> +
> +MAP header has the same endianness of the IP packet.
> +
> +Packet format -
> +
> +Bit 0 1   2-7  8 -
> 15   16 - 31
> +Function   Command / Data   Reserved Pad   Multiplexer
> IDPayload length
> +Bit32 - x
> +Function Raw  Bytes
> +
> +Command (1)/ Data (0) bit value is to indicate if the packet is a
> MAP command
> +or data packet. Control packet is used for transport level flow
> control. Data
> +packets are standard IP packets.
> +
> +Reserved bits are usually zeroed out and to be ignored by receiver.
> +
> +Padding is number of bytes to be added for 4 byte alignment if
> required by
> +hardware.
> +
> +Multiplexer ID is to indicate the PDN on which data has to be sent.
> +
> +Payload length includes the padding length but does not include MAP
> header
> +length.
> +
> +b. MAP packet (command specific)
> +
> +Bit 0 1   2-7  8 -
> 15   16 - 31
> +Function   Command Reserved Pad   

[PATCH net-next 3/3 v11] drivers: net: ethernet: qualcomm: rmnet: Initial implementation

2017-08-29 Thread Subash Abhinov Kasiviswanathan 
RmNet driver provides a transport agnostic MAP (multiplexing and
aggregation protocol) support in embedded module. Module provides
virtual network devices which can be attached to any IP-mode
physical device. This will be used to provide all MAP functionality
on future hardware in a single consistent location.

Signed-off-by: Subash Abhinov Kasiviswanathan 
---
 Documentation/networking/rmnet.txt |  82 
 drivers/net/ethernet/qualcomm/Kconfig  |   2 +
 drivers/net/ethernet/qualcomm/Makefile |   2 +
 drivers/net/ethernet/qualcomm/rmnet/Kconfig|  12 +
 drivers/net/ethernet/qualcomm/rmnet/Makefile   |  10 +
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c | 419 +
 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h |  56 +++
 .../net/ethernet/qualcomm/rmnet/rmnet_handlers.c   | 271 +
 .../net/ethernet/qualcomm/rmnet/rmnet_handlers.h   |  26 ++
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h|  88 +
 .../ethernet/qualcomm/rmnet/rmnet_map_command.c| 107 ++
 .../net/ethernet/qualcomm/rmnet/rmnet_map_data.c   | 105 ++
 .../net/ethernet/qualcomm/rmnet/rmnet_private.h|  45 +++
 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c| 170 +
 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h|  29 ++
 15 files changed, 1424 insertions(+)
 create mode 100644 Documentation/networking/rmnet.txt
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/Kconfig
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/Makefile
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.h
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_private.h
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
 create mode 100644 drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.h

diff --git a/Documentation/networking/rmnet.txt 
b/Documentation/networking/rmnet.txt
new file mode 100644
index 000..6b341ea
--- /dev/null
+++ b/Documentation/networking/rmnet.txt
@@ -0,0 +1,82 @@
+1. Introduction
+
+rmnet driver is used for supporting the Multiplexing and aggregation
+Protocol (MAP). This protocol is used by all recent chipsets using Qualcomm
+Technologies, Inc. modems.
+
+This driver can be used to register onto any physical network device in
+IP mode. Physical transports include USB, HSIC, PCIe and IP accelerator.
+
+Multiplexing allows for creation of logical netdevices (rmnet devices) to
+handle multiple private data networks (PDN) like a default internet, tethering,
+multimedia messaging service (MMS) or IP media subsystem (IMS). Hardware sends
+packets with MAP headers to rmnet. Based on the multiplexer id, rmnet
+routes to the appropriate PDN after removing the MAP header.
+
+Aggregation is required to achieve high data rates. This involves hardware
+sending aggregated bunch of MAP frames. rmnet driver will de-aggregate
+these MAP frames and send them to appropriate PDN's.
+
+2. Packet format
+
+a. MAP packet (data / control)
+
+MAP header has the same endianness of the IP packet.
+
+Packet format -
+
+Bit 0 1   2-7  8 - 15   16 - 31
+Function   Command / Data   Reserved Pad   Multiplexer IDPayload length
+Bit32 - x
+Function Raw  Bytes
+
+Command (1)/ Data (0) bit value is to indicate if the packet is a MAP command
+or data packet. Control packet is used for transport level flow control. Data
+packets are standard IP packets.
+
+Reserved bits are usually zeroed out and to be ignored by receiver.
+
+Padding is number of bytes to be added for 4 byte alignment if required by
+hardware.
+
+Multiplexer ID is to indicate the PDN on which data has to be sent.
+
+Payload length includes the padding length but does not include MAP header
+length.
+
+b. MAP packet (command specific)
+
+Bit 0 1   2-7  8 - 15   16 - 31
+Function   Command Reserved Pad   Multiplexer IDPayload length
+Bit  32 - 3940 - 4546 - 47   48 - 63
+Function   Command nameReserved   Command Type   Reserved
+Bit  64 - 95
+Function   Transaction ID
+Bit  96 - 127
+Function   Command data
+
+Command 1 indicates disabling flow while 2 is enabling flow
+
+Command types -
+0 for MAP command request
+1 is to acknowledge the receipt of a command
+2 is for unsupported commands
+3 is for error during processing of commands
+
+c. Aggregation
+
+Aggregation is multiple MAP packets (can be data or command) delivered