On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
<ricardo.martinez@xxxxxxxxxxxxxxx> wrote:
> Port-proxy provides a common interface to interact with different types
> of ports. Ports export their configuration via `struct t7xx_port` and
> operate as defined by `struct port_ops`.
[skipped]
> diff --git a/drivers/net/wwan/t7xx/t7xx_port.h b/drivers/net/wwan/t7xx/t7xx_port.h
> ...
> +struct t7xx_port {
> + /* members used for initialization, do not change the order */
As already suggested by Andy, use C99 initializers to initialize the
md_ccci_ports array and drop the above comment about the strict order
requirements.
> + enum ccci_ch tx_ch;
> + enum ccci_ch rx_ch;
> + unsigned char txq_index;
> + unsigned char rxq_index;
> + unsigned char txq_exp_index;
> + unsigned char rxq_exp_index;
> + enum cldma_id path_id;
> + unsigned int flags;
> + struct port_ops *ops;
> + unsigned int minor;
> + char *name;
Why did you need these two fields with the port name and minor number?
The WWAN subsystem will care about these data for you. It is its
purpose.
> + enum wwan_port_type mtk_port_type;
> +
> + /* members un-initialized in definition */
> + struct wwan_port *mtk_wwan_port;
> + struct mtk_pci_dev *mtk_dev;
> + struct device *dev;
> + short seq_nums[2];
> + struct port_proxy *port_proxy;
> + atomic_t usage_cnt;
> + struct list_head entry;
> + struct list_head queue_entry;
> + unsigned int major;
> + unsigned int minor_base;
> + /* TX and RX flows are asymmetric since ports are multiplexed on
> + * queues.
> + *
> + * TX: data blocks are sent directly to a queue. Each port
> + * does not maintain a TX list; instead, they only provide
> + * a wait_queue_head for blocking writes.
> + *
> + * RX: Each port uses a RX list to hold packets,
> + * allowing the modem to dispatch RX packet as quickly as possible.
> + */
> + struct sk_buff_head rx_skb_list;
> + bool skb_from_pool;
> + spinlock_t port_update_lock; /* protects port configuration */
> + wait_queue_head_t rx_wq;
> + int rx_length_th;
> + port_skb_handler skb_handler;
> + unsigned char chan_enable;
> + unsigned char chn_crt_stat;
> + struct cdev *cdev;
> + struct task_struct *thread;
> + struct mutex tx_mutex_lock; /* protects the seq number operation */
> +};
You should split the t7xx_port structure at least for two parts. A
first part with static configuration can remain in the structure and
statically initialized in the md_ccci_ports array. All non-shareable
runtime state fields (e.g. SKB lists, pointers to dynamically
allocated device instance structures) should be moved to a device
state container.
[skipped]
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.c b/drivers/net/wwan/t7xx/t7xx_port_proxy.c
> ...
> +#define PORT_NETLINK_MSG_MAX_PAYLOAD 32
> +#define PORT_NOTIFY_PROTOCOL NETLINK_USERSOCK
There is a clear statement in the include/uapi/linux/netlink.h file
that NETLINK_USERSOCK is reserved for user mode socket protocols.
Please do not abuse netlink protocol numbers.
If you really need a special Netlink interface to communicate with
userspace, consider creating a new generic netlink family. But it
looks like all the Netlink stuff here is a leftover of a debug
interface that was used at an earlier driver development stage. So I
suggest to just remove all Netlink usage here and consider using
dynamic debug logging, or switch to the kernel tracing.
> ...
> +static struct port_proxy *port_prox;
This is another one pointer that should be placed into a device
runtime state structure to avoid driver crash with multiple modems.
> ...
> +static struct port_ops dummy_port_ops;
Why do you need this dummy ops structure? You anyway remove it in the
next patch. If you need it as a placeholder for the md_ccci_ports
array below, then it is safe to define an empty md_ccci_ports array
and then just fill it. Please consider removing this structure to
avoid ping-pong changes.
> +static struct t7xx_port md_ccci_ports[] = {
> + {0, 0, 0, 0, 0, 0, ID_CLDMA1, 0, &dummy_port_ops, 0xff, "dummy_port",},
As already suggested by Andy, use C99 initializers here. E.g.
{
.tx_ch = 0,
.rx_ch = 0,
.txq_index = 0,
.rxq_index = 0,
.txq_exp_index = 0,
.rxq_exp_index = 0,
.path_id = ID_CLDMA1,
.ops = &dummy_port_ops,
.name = "dummy_port",
}, {
...
}, {
...
}
> ...
> +/* Sequence numbering to track for lost packets */
> +void port_proxy_set_seq_num(struct t7xx_port *port, struct ccci_header *ccci_h)
> +{
> + if (ccci_h && port) {
> + ccci_h->status &= ~HDR_FLD_SEQ;
> + ccci_h->status |= FIELD_PREP(HDR_FLD_SEQ, port->seq_nums[MTK_OUT]);
> + ccci_h->status &= ~HDR_FLD_AST;
> + ccci_h->status |= FIELD_PREP(HDR_FLD_AST, 1);
> + }
Endians handling required here.
> +}
> +
> +static u16 port_check_rx_seq_num(struct t7xx_port *port, struct ccci_header *ccci_h)
> +{
> + u16 channel, seq_num, assert_bit;
> +
> + channel = FIELD_GET(HDR_FLD_CHN, ccci_h->status);
> + seq_num = FIELD_GET(HDR_FLD_SEQ, ccci_h->status);
> + assert_bit = FIELD_GET(HDR_FLD_AST, ccci_h->status);
Field endians handling is missed here. Probably you should first
convert status field data to CPU endians and only then parse it. E.g.
u32 status = le32_to_cpu(ccci_h->status);
channel = FIELD_GET(HDR_FLD_CHN, status);
seq_num = FIELD_GET(HDR_FLD_SEQ, status);
assert_bit = FIELD_GET(HDR_FLD_AST, status);
> + if (assert_bit && port->seq_nums[MTK_IN] &&
> + ((seq_num - port->seq_nums[MTK_IN]) & CHECK_RX_SEQ_MASK) != 1) {
> + dev_err(port->dev, "channel %d seq number out-of-order %d->%d (data: %X, %X)\n",
> + channel, seq_num, port->seq_nums[MTK_IN],
> + ccci_h->data[0], ccci_h->data[1]);
dev_err_ratelimited() ?
> + }
> +
> + return seq_num;
> +}
> ...
> +int port_recv_skb(struct t7xx_port *port, struct sk_buff *skb)
> +{
> ...
> + if (port->flags & PORT_F_RX_ALLOW_DROP) {
> + dev_err(port->dev, "port %s RX full, drop packet\n", port->name);
Should the ratelimited variant be used here? And why is so high
message level used?
> + return -ENETDOWN;
Why ENETDOWN on buffer space exhaustion?
> + }
> +
> + return -ENOBUFS;
> +}
> ...
> +int port_proxy_send_skb(struct t7xx_port *port, struct sk_buff *skb, bool from_pool)
> +{
> + struct ccci_header *ccci_h;
> ...
> + ccci_h = (struct ccci_header *)(skb->data);
> ...
> + port_proxy_set_seq_num(port, (struct ccci_header *)ccci_h);
cchi_h is already of type ccci_header, no casting is required here.
> ...
> +
> +/* inject CCCI message to modem */
> +void port_proxy_send_msg_to_md(int ch, unsigned int msg, unsigned int resv)
This function is not called by any code in this patch. Should the
function be moved to the "net: wwan: t7xx: Add control port" patch
along with the ctrl_msg_header structure definition?
> +{
> + struct ctrl_msg_header *ctrl_msg_h;
> + struct ccci_header *ccci_h;
> + struct t7xx_port *port;
> + struct sk_buff *skb;
> + int ret;
> +
> + port = port_get_by_ch(ch);
> + if (!port)
> + return;
> +
> + skb = ccci_alloc_skb_from_pool(&port->mtk_dev->pools, sizeof(struct ccci_header),
> + GFS_BLOCKING);
> + if (!skb)
> + return;
> +
> + if (ch == CCCI_CONTROL_TX) {
> + ccci_h = (struct ccci_header *)(skb->data);
> + ccci_h->data[0] = CCCI_HEADER_NO_DATA;
> + ccci_h->data[1] = sizeof(struct ctrl_msg_header) + CCCI_H_LEN;
> + ccci_h->status &= ~HDR_FLD_CHN;
> + ccci_h->status |= FIELD_PREP(HDR_FLD_CHN, ch);
> + ccci_h->reserved = 0;
> + ctrl_msg_h = (struct ctrl_msg_header *)(skb->data + CCCI_H_LEN);
> + ctrl_msg_h->data_length = 0;
> + ctrl_msg_h->reserved = resv;
> + ctrl_msg_h->ctrl_msg_id = msg;
> + skb_put(skb, CCCI_H_LEN + sizeof(struct ctrl_msg_header));
> + } else {
> + ccci_h = skb_put(skb, sizeof(struct ccci_header));
> + ccci_h->data[0] = CCCI_HEADER_NO_DATA;
> + ccci_h->data[1] = msg;
> + ccci_h->status &= ~HDR_FLD_CHN;
> + ccci_h->status |= FIELD_PREP(HDR_FLD_CHN, ch);
> + ccci_h->reserved = resv;
> + }
Endians handling missed here as well.
> + ret = port_proxy_send_skb(port, skb, port->skb_from_pool);
> + if (ret) {
> + dev_err(port->dev, "port%s send to MD fail\n", port->name);
> + ccci_free_skb(&port->mtk_dev->pools, skb);
> + }
> +}
> ...
> +static int proxy_register_char_dev(void)
> +{
> + dev_t dev = 0;
> + int ret;
> +
> + if (port_prox->major) {
> + dev = MKDEV(port_prox->major, port_prox->minor_base);
> + ret = register_chrdev_region(dev, TTY_IPC_MINOR_BASE, MTK_DEV_NAME);
> + } else {
> + ret = alloc_chrdev_region(&dev, port_prox->minor_base,
> + TTY_IPC_MINOR_BASE, MTK_DEV_NAME);
> + if (ret)
> + dev_err(port_prox->dev, "failed to alloc chrdev region, ret=%d\n", ret);
> +
> + port_prox->major = MAJOR(dev);
> + }
For what do you need these character devices? The WWAN subsystem
already handle all these tasks.
> + return ret;
> +}
> ...
> +static int proxy_alloc(struct mtk_modem *md)
> +{
> + int ret;
> +
> + port_prox = devm_kzalloc(&md->mtk_dev->pdev->dev, sizeof(*port_prox), GFP_KERNEL);
> + if (!port_prox)
> + return -ENOMEM;
This pointer should be placed into the mtk_modem, not to a global variable.
> ...
> +int port_proxy_broadcast_state(struct t7xx_port *port, int state)
> +{
> + char msg[PORT_NETLINK_MSG_MAX_PAYLOAD];
> +
> + if (state >= MTK_PORT_STATE_INVALID)
> + return -EINVAL;
> +
> + switch (state) {
> + case MTK_PORT_STATE_ENABLE:
> + snprintf(msg, PORT_NETLINK_MSG_MAX_PAYLOAD, "enable %s", port->name);
> + break;
> +
> + case MTK_PORT_STATE_DISABLE:
> + snprintf(msg, PORT_NETLINK_MSG_MAX_PAYLOAD, "disable %s", port->name);
> + break;
> +
> + default:
> + snprintf(msg, PORT_NETLINK_MSG_MAX_PAYLOAD, "invalid operation");
> + break;
> + }
> +
> + return port_netlink_send_msg(port, PORT_STATE_BROADCAST_GROUP, msg, strlen(msg) + 1);
Netlink by nature is a binary protocol. You need to emit messages of
different types per port state with a port name attribute inside. Or
emit a single type message with two separate attributes: one to carry
a port state and a separate attribute to carry a port name.
Or, as I suggested above, just drop this Netlink abuse and switch to
dynamic debug logging. Or even better, consider switching to the
kernel tracing API.
[skipped]
> diff --git a/drivers/net/wwan/t7xx/t7xx_port_proxy.h b/drivers/net/wwan/t7xx/t7xx_port_proxy.h
> ...
> +struct ctrl_msg_header {
> + u32 ctrl_msg_id;
> + u32 reserved;
> + u32 data_length;
All three of these fields should be of type __be32, since the
structure is passed to the modem as is.
> + u8 data[0];
> +};
> +
> +struct port_msg {
> + u32 head_pattern;
> + u32 info;
> + u32 tail_pattern;
Same here.
> + u8 data[0]; /* port set info */
> +};
[skipped]
> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
> ...
> @@ -202,11 +208,29 @@ static void fsm_routine_exception(struct ccci_fsm_ctl *ctl, struct ccci_fsm_comm
> ...
> spin_unlock_irqrestore(&ctl->event_lock, flags);
> + if (pass) {
> + log_port = port_get_by_name("ttyCMdLog");
> + if (log_port)
> + log_port->ops->enable_chl(log_port);
> + else
> + dev_err(dev, "ttyCMdLog port not found\n");
> +
> + meta_port = port_get_by_name("ttyCMdMeta");
> + if (meta_port)
> + meta_port->ops->enable_chl(meta_port);
> + else
> + dev_err(dev, "ttyCMdMeta port not found\n");
Looks like this change does not belong to this patch. The "ttyCMdLog"
port entry will be created only by the last patch.
--
Sergey
