LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Alex Elder <elder@linaro.org>
Cc: David Miller <davem@davemloft.net>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Networking <netdev@vger.kernel.org>,
	DTML <devicetree@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	syadagir@codeaurora.org, mjavid@codeaurora.org,
	Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>
Subject: Re: [RFC PATCH 11/12] soc: qcom: ipa: IPA rmnet interface
Date: Wed, 7 Nov 2018 14:30:53 +0100
Message-ID: <CAK8P3a3vB6wF7KYHpk+ULYGFhEe-pyWYC6fHaaBYeSuWCNooRA@mail.gmail.com> (raw)
In-Reply-To: <20181107003250.5832-12-elder@linaro.org>

On Wed, Nov 7, 2018 at 1:33 AM Alex Elder <elder@linaro.org> wrote:

> Note:  This portion of the driver will be heavily affected by
> planned rework on the data path code.

Ok. I don't think the ioctl interface has a real chance of getting merged
into the kernel. You should generally not require any custom user space
tools for a driver like this.

> diff --git a/drivers/net/ipa/msm_rmnet.h b/drivers/net/ipa/msm_rmnet.h
> new file mode 100644
> index 000000000000..042380fd53fb
> --- /dev/null
> +++ b/drivers/net/ipa/msm_rmnet.h

Just for the record: if we really wanted to define ioctls, this would go
into 'include/linux/uapi/msm_rmnet.h' and get installed into the
/usr/include hierarchy on all machines.

> +
> +#define RMNET_IOCTL_SET_LLP_ETHERNET 0x000089f1 /* Set Ethernet protocol  */
> +#define RMNET_IOCTL_SET_LLP_IP      0x000089f2 /* Set RAWIP protocol     */
> +#define RMNET_IOCTL_GET_LLP         0x000089f3 /* Get link protocol      */
> +#define RMNET_IOCTL_SET_QOS_ENABLE   0x000089f4 /* Set QoS header enabled */
> +#define RMNET_IOCTL_SET_QOS_DISABLE  0x000089f5 /* Set QoS header disabled*/
> +#define RMNET_IOCTL_GET_QOS         0x000089f6 /* Get QoS header state   */
> +#define RMNET_IOCTL_GET_OPMODE      0x000089f7 /* Get operation mode     */

And the commands would be defined using _IOC/_IOR/_IOW macros that
document which arguments they take


> +#define RMNET_IOCTL_OPEN            0x000089f8 /* Open transport port    */
> +#define RMNET_IOCTL_CLOSE           0x000089f9 /* Close transport port   */
> +#define RMNET_IOCTL_FLOW_ENABLE             0x000089fa /* Flow enable            */
> +#define RMNET_IOCTL_FLOW_DISABLE     0x000089fb /* Flow disable                  */
> +#define RMNET_IOCTL_FLOW_SET_HNDL    0x000089fc /* Set flow handle       */
> +#define RMNET_IOCTL_EXTENDED        0x000089fd /* Extended IOCTLs        */

'extended' interfaces are obviously out of the question entirely, those
would all need to be separate commands.


> +/* User space may not have this defined. */
> +#ifndef IFNAMSIZ
> +#define IFNAMSIZ 16
> +#endif

This is in <linux/if.h>

> +struct rmnet_ioctl_extended_s {
> +       u32     extended_ioctl;
> +       union {

And unions in the ioctl interfaces also wouldn't work.

> +static bool initialized;       /* Avoid duplicate initialization */
> +
> +static struct rmnet_ipa_context rmnet_ipa_ctx_struct;
> +static struct rmnet_ipa_context *rmnet_ipa_ctx = &rmnet_ipa_ctx_struct;

Global variables like these should be removed.

> +/** ipa_wwan_xmit() - Transmits an skb.
> + *
> + * @skb: skb to be transmitted
> + * @dev: network device
> + *
> + * Return codes:
> + * NETDEV_TX_OK: Success
> + * NETDEV_TX_BUSY: Error while transmitting the skb. Try again later
> + */
> +static int ipa_wwan_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +       struct ipa_wwan_private *wwan_ptr = netdev_priv(dev);
> +       unsigned int skb_len;
> +       int outstanding;
> +
> +       if (skb->protocol != htons(ETH_P_MAP)) {
> +               dev_kfree_skb_any(skb);
> +               dev->stats.tx_dropped++;
> +               return NETDEV_TX_OK;
> +       }
> +
> +       /* Control packets are sent even if queue is stopped.  We
> +        * always honor the data and control high-water marks.
> +        */
> +       outstanding = atomic_read(&wwan_ptr->outstanding_pkts);
> +       if (!RMNET_MAP_GET_CD_BIT(skb)) {       /* Data packet? */
> +               if (netif_queue_stopped(dev))
> +                       return NETDEV_TX_BUSY;
> +               if (outstanding >= wwan_ptr->outstanding_high)
> +                       return NETDEV_TX_BUSY;
> +       } else if (outstanding >= wwan_ptr->outstanding_high_ctl) {
> +               return NETDEV_TX_BUSY;
> +       }

This seems to be a poor reimplementation of BQL. Better
use netdev_sent_queue() and netdev_completed_queue()
to do the same thing better.

> +/** apps_ipa_packet_receive_notify() - Rx notify
> + *
> + * @priv: driver context
> + * @evt: event type
> + * @data: data provided with event
> + *
> + * IPA will pass a packet to the Linux network stack with skb->data
> + */
> +static void apps_ipa_packet_receive_notify(void *priv, enum ipa_dp_evt_type evt,
> +                                          unsigned long data)
> +{
> +       struct ipa_wwan_private *wwan_ptr;
> +       struct net_device *dev = priv;
> +
> +       wwan_ptr = netdev_priv(dev);
> +       if (evt == IPA_RECEIVE) {
> +               struct sk_buff *skb = (struct sk_buff *)data;
> +               int ret;
> +               unsigned int packet_len = skb->len;
> +
> +               skb->dev = rmnet_ipa_ctx->dev;
> +               skb->protocol = htons(ETH_P_MAP);
> +
> +               ret = netif_receive_skb(skb);
> +               if (ret) {
> +                       pr_err_ratelimited("fail on netif_receive_skb\n");
> +                       dev->stats.rx_dropped++;
> +               }
> +               dev->stats.rx_packets++;
> +               dev->stats.rx_bytes += packet_len;
> +       } else if (evt == IPA_CLIENT_START_POLL) {
> +               napi_schedule(&wwan_ptr->napi);
> +       } else if (evt == IPA_CLIENT_COMP_NAPI) {
> +               napi_complete(&wwan_ptr->napi);
> +       } else {
> +               ipa_err("Invalid evt %d received in wan_ipa_receive\n", evt);
> +       }
> +}

I don't understand the logic here. Why is this a callback function?
You normally want the data path to be as fast as possible, and the
indirection seems like it would get in the way of that.

Since the function doesn't do much interesting work, could
it be moved into the caller?

> +/** handle_ingress_format() - Ingress data format configuration */
> +static int handle_ingress_format(struct net_device *dev,
> +                                struct rmnet_ioctl_extended_s *in)
> +{

Can you describe how this would be called from user space?
I.e. what is the reason we have to configure anything here?


> +
> +       /* Unsupported requests */
> +       case RMNET_IOCTL_SET_MRU:                       /* Set MRU */
> +       case RMNET_IOCTL_GET_MRU:                       /* Get MRU */
> +       case RMNET_IOCTL_GET_AGGREGATION_COUNT:         /* Get agg count */
> +       case RMNET_IOCTL_SET_AGGREGATION_COUNT:         /* Set agg count */
> +       case RMNET_IOCTL_GET_AGGREGATION_SIZE:          /* Get agg size */
> +       case RMNET_IOCTL_SET_AGGREGATION_SIZE:          /* Set agg size */
> +       case RMNET_IOCTL_FLOW_CONTROL:                  /* Do flow control */
> +       case RMNET_IOCTL_GET_DFLT_CONTROL_CHANNEL:      /* For legacy use */
> +       case RMNET_IOCTL_GET_HWSW_MAP:                  /* Get HW/SW map */
> +       case RMNET_IOCTL_SET_RX_HEADROOM:               /* Set RX Headroom */
> +       case RMNET_IOCTL_SET_QOS_VERSION:               /* Set 8/6 byte QoS */
> +       case RMNET_IOCTL_GET_QOS_VERSION:               /* Get 8/6 byte QoS */
> +       case RMNET_IOCTL_GET_SUPPORTED_QOS_MODES:       /* Get QoS modes */
> +       case RMNET_IOCTL_SET_SLEEP_STATE:               /* Set sleep state */
> +       case RMNET_IOCTL_SET_XLAT_DEV_INFO:             /* xlat dev name */
> +       case RMNET_IOCTL_DEREGISTER_DEV:                /* Deregister netdev */
> +               return -ENOTSUPP;       /* Defined, but unsupported command */
> +
> +       default:
> +               return -EINVAL;         /* Invalid (unrecognized) command */
> +       }
> +
> +copy_out:
> +       return copy_to_user(data, &edata, size) ? -EFAULT : 0;
> +}
> +
> +/** ipa_wwan_ioctl() - I/O control for wwan network driver */
> +static int ipa_wwan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +       struct rmnet_ioctl_data_s ioctl_data = { };
> +       void __user *data;
> +       size_t size;
> +
> +       data = ifr->ifr_ifru.ifru_data;
> +       size = sizeof(ioctl_data);
> +
> +       switch (cmd) {
> +       /* These features are implied; alternatives are not supported */
> +       case RMNET_IOCTL_SET_LLP_IP:            /* RAW IP protocol */
> +       case RMNET_IOCTL_SET_QOS_DISABLE:       /* QoS header disabled */
> +               return 0;
> +
> +       /* These features are not supported; use alternatives */
> +       case RMNET_IOCTL_SET_LLP_ETHERNET:      /* Ethernet protocol */
> +       case RMNET_IOCTL_SET_QOS_ENABLE:        /* QoS header enabled */
> +       case RMNET_IOCTL_GET_OPMODE:            /* Get operation mode */
> +       case RMNET_IOCTL_FLOW_ENABLE:           /* Flow enable */
> +       case RMNET_IOCTL_FLOW_DISABLE:          /* Flow disable */
> +       case RMNET_IOCTL_FLOW_SET_HNDL:         /* Set flow handle */
> +               return -ENOTSUPP;
> +
> +       case RMNET_IOCTL_GET_LLP:               /* Get link protocol */
> +               ioctl_data.u.operation_mode = RMNET_MODE_LLP_IP;
> +               goto copy_out;
> +
> +       case RMNET_IOCTL_GET_QOS:               /* Get QoS header state */
> +               ioctl_data.u.operation_mode = RMNET_MODE_NONE;
> +               goto copy_out;
> +
> +       case RMNET_IOCTL_OPEN:                  /* Open transport port */
> +       case RMNET_IOCTL_CLOSE:                 /* Close transport port */
> +               return 0;
> +
> +       case RMNET_IOCTL_EXTENDED:              /* Extended IOCTLs */
> +               return ipa_wwan_ioctl_extended(dev, data);
> +
> +       default:
> +               return -EINVAL;
> +       }

It would help to remove everything that is a nop or not implemented
or that returns a constant value here, those are clearly not
relevant for the submission here.

> +
> +static const struct of_device_id rmnet_ipa_dt_match[] = {
> +       {.compatible = "qcom,rmnet-ipa"},
> +       {},
> +};

The match string looks overly generic, surely there must be plans
to have future versions of this that might require identification.

     Arnd

  reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-07  0:32 [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Alex Elder
2018-11-07  0:32 ` [RFC PATCH 01/12] dt-bindings: soc: qcom: add IPA bindings Alex Elder
2018-11-07 11:50   ` Arnd Bergmann
2018-11-09 22:38     ` Alex Elder
2018-11-07 14:59   ` Rob Herring
2018-11-09 22:38     ` Alex Elder
2018-11-11  1:40       ` Rob Herring
2018-11-13 16:28     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 02/12] soc: qcom: ipa: DMA helpers Alex Elder
2018-11-07 12:17   ` Arnd Bergmann
2018-11-13 16:33     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 03/12] soc: qcom: ipa: generic software interface Alex Elder
2018-11-07  0:32 ` [RFC PATCH 04/12] soc: qcom: ipa: immediate commands Alex Elder
2018-11-07 14:36   ` Arnd Bergmann
2018-11-13 16:58     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 05/12] soc: qcom: ipa: IPA interrupts and the microcontroller Alex Elder
2018-11-07  0:32 ` [RFC PATCH 06/12] soc: qcom: ipa: QMI modem communication Alex Elder
2018-11-07  0:32 ` [RFC PATCH 07/12] soc: qcom: ipa: IPA register abstraction Alex Elder
2018-11-07 15:00   ` Arnd Bergmann
2018-11-15  2:48     ` Alex Elder
2018-11-15 14:42       ` Arnd Bergmann
2018-11-07  0:32 ` [RFC PATCH 08/12] soc: qcom: ipa: utility functions Alex Elder
2018-11-07  0:32 ` [RFC PATCH 09/12] soc: qcom: ipa: main IPA source file Alex Elder
2018-11-07 14:08   ` Arnd Bergmann
2018-11-15  3:11     ` Alex Elder
2018-11-07  0:32 ` [RFC PATCH 10/12] soc: qcom: ipa: data path Alex Elder
2018-11-07 14:55   ` Arnd Bergmann
2018-11-15  3:31     ` Alex Elder
2018-11-15 14:48       ` Arnd Bergmann
2018-11-07  0:32 ` [RFC PATCH 11/12] soc: qcom: ipa: IPA rmnet interface Alex Elder
2018-11-07 13:30   ` Arnd Bergmann [this message]
2018-11-07 15:26   ` Dan Williams
2018-11-07  0:32 ` [RFC PATCH 12/12] soc: qcom: ipa: build and "ipa_i.h" Alex Elder
2018-11-07  0:40   ` Randy Dunlap
2018-11-08 16:22     ` Alex Elder
2018-11-07 12:34   ` Arnd Bergmann
2018-11-07 15:46 ` [RFC PATCH 00/12] net: introduce Qualcomm IPA driver Arnd Bergmann

Reply instructions:

You may reply publically 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=CAK8P3a3vB6wF7KYHpk+ULYGFhEe-pyWYC6fHaaBYeSuWCNooRA@mail.gmail.com \
    --to=arnd@arndb.de \
    --cc=bjorn.andersson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=elder@linaro.org \
    --cc=ilias.apalodimas@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mjavid@codeaurora.org \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=syadagir@codeaurora.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git