Date:   Thu, 10 Mar 2022 03:13:39 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     "Martinez, Ricardo" <ricardo.martinez@...ux.intel.com>
Cc:     netdev@...r.kernel.org, linux-wireless@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        Loic Poulain <loic.poulain@...aro.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        chandrashekar.devegowda@...el.com,
        Intel Corporation <linuxwwan@...el.com>,
        chiranjeevi.rapolu@...ux.intel.com,
        Haijun Liu (刘海军) 
        <haijun.liu@...iatek.com>, amir.hanania@...el.com,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        dinesh.sharma@...el.com, eliot.lee@...el.com,
        ilpo.johannes.jarvinen@...el.com, moises.veleta@...el.com,
        pierre-louis.bossart@...el.com, muralidharan.sethuraman@...el.com,
        Soumya.Prakash.Mishra@...el.com, sreehari.kancharla@...el.com,
        madhusmita.sahu@...el.com
Subject: Re: [PATCH net-next v5 06/13] net: wwan: t7xx: Add AT and MBIM WWAN ports

On Wed, Mar 9, 2022 at 3:02 AM Martinez, Ricardo
<ricardo.martinez@...ux.intel.com> wrote:
> On 3/6/2022 6:56 PM, Sergey Ryazanov wrote:
>> On Thu, Feb 24, 2022 at 1:35 AM Ricardo Martinez
>> <ricardo.martinez@...ux.intel.com> wrote:
>>> From: Chandrashekar Devegowda <chandrashekar.devegowda@...el.com>
>>>
>>> Adds AT and MBIM ports to the port proxy infrastructure.
>>> The initialization method is responsible for creating the corresponding
>>> ports using the WWAN framework infrastructure. The implemented WWAN port
>>> operations are start, stop, and TX.
>> [skipped]
>>
>>> +static int t7xx_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
>>> +{
>>> +       struct t7xx_port *port_private = wwan_port_get_drvdata(port);
>>> +       size_t actual_len, alloc_size, txq_mtu = CLDMA_MTU;
>>> +       struct t7xx_port_static *port_static;
>>> +       unsigned int len, i, packets;
>>> +       struct t7xx_fsm_ctl *ctl;
>>> +       enum md_state md_state;
>>> +
>>> +       len = skb->len;
>>> +       if (!len || !port_private->rx_length_th || !port_private->chan_enable)
>>> +               return -EINVAL;
>>> +
>>> +       port_static = port_private->port_static;
>>> +       ctl = port_private->t7xx_dev->md->fsm_ctl;
>>> +       md_state = t7xx_fsm_get_md_state(ctl);
>>> +       if (md_state == MD_STATE_WAITING_FOR_HS1 || md_state == MD_STATE_WAITING_FOR_HS2) {
>>> +               dev_warn(port_private->dev, "Cannot write to %s port when md_state=%d\n",
>>> +                        port_static->name, md_state);
>>> +               return -ENODEV;
>>> +       }
>>> +
>>> +       alloc_size = min_t(size_t, txq_mtu, len + CCCI_HEADROOM);
>>> +       actual_len = alloc_size - CCCI_HEADROOM;
>>> +       packets = DIV_ROUND_UP(len, txq_mtu - CCCI_HEADROOM);
>>> +
>>> +       for (i = 0; i < packets; i++) {
>>> +               struct ccci_header *ccci_h;
>>> +               struct sk_buff *skb_ccci;
>>> +               int ret;
>>> +
>>> +               if (packets > 1 && packets == i + 1) {
>>> +                       actual_len = len % (txq_mtu - CCCI_HEADROOM);
>>> +                       alloc_size = actual_len + CCCI_HEADROOM;
>>> +               }
>>
>> Why do you track the packet number? Why not track the offset in the
>> passed data? E.g.:
>>
>> for (off = 0; off < len; off += chunklen) {
>>      chunklen = min(len - off, CLDMA_MTU - sizeof(struct ccci_header);
>>      skb_ccci = alloc_skb(chunklen + sizeof(struct ccci_header), ...);
>>      skb_put_data(skb_ccci, skb->data + off, chunklen);
>>      /* Send skb_ccci */
>> }
>
> Sure, I'll make that change.
>
>>> +               skb_ccci = __dev_alloc_skb(alloc_size, GFP_KERNEL);
>>> +               if (!skb_ccci)
>>> +                       return -ENOMEM;
>>> +
>>> +               ccci_h = skb_put(skb_ccci, sizeof(*ccci_h));
>>> +               t7xx_ccci_header_init(ccci_h, 0, actual_len + sizeof(*ccci_h),
>>> +                                     port_static->tx_ch, 0);
>>> +               skb_put_data(skb_ccci, skb->data + i * (txq_mtu - CCCI_HEADROOM), actual_len);
>>> +               t7xx_port_proxy_set_tx_seq_num(port_private, ccci_h);
>>> +
>>> +               ret = t7xx_port_send_skb_to_md(port_private, skb_ccci);
>>> +               if (ret) {
>>> +                       dev_kfree_skb_any(skb_ccci);
>>> +                       dev_err(port_private->dev, "Write error on %s port, %d\n",
>>> +                               port_static->name, ret);
>>> +                       return ret;
>>> +               }
>>> +
>>> +               port_private->seq_nums[MTK_TX]++;
>>
>> Sequence number tracking as well as CCCI header construction are
>> common operations, so why not move them to t7xx_port_send_skb_to_md()?
>
> Sequence number should be set as part of CCCI header construction.
>
> I think it's a bit more readable to initialize the CCCI header right
> after the corresponding skb_put(). Not a big deal, any thoughts?

I do not _think_ creating the CCCI header in the WWAN or CTRL port
functions is any good idea. In case of stacked protocols, each layer
should create its own header, pass the packet down the stack, and then
a next layer will create a next header.

In case of the CTRL port, this means that the control port code should
take an opaque data block from an upper layer (e.g. features request),
prepend it with a control msg header, and pass it down the stack to
the port proxy layer, where the CCCI header will be prepended.

In case a WWAN port, all headers are passed from user space, so there
шы nothing to prepend. And the only remaining function is to fragment
a user input, and then pass all  the fragments to the port proxy
layer, where the CCCI header will be prepended.

This way, you do not overload the CTRL/WWAN port with code of other
protocols (i.e. CCCI), reduce code duplication. Which in itself
improves the code maintainability and future development. Creating a
CCCI header at the WWAN port layer is like forcing a user to manually
create IP and UDP headers before writing a data block into a network
socket :)

Anyway, it is up to you to decide exactly how to create headers and
assign sequence numbers. I just wanted to point out the code
inconsistency. It does not make the code wrong, it just makes the code
look stranger.

> Note that the upcoming fw update feature doesn't require a CCCI header,
> so we could rename the TX function as t7xx_port_send_ccci_skb_to_md(),
> this would give a hint that it is taking care of the CCCI header.

Does this mean the firmware upgrade does not utilize the channel id,
and just pushes data directly to a specific CLDMA queue? In that case
it looks like the firmware upgrade code needs to entirely bypass the
port proxy layer and communicate directly with CLDMA. Isn't it?

>>> +       }
>>> +
>>> +       dev_kfree_skb(skb);
>>> +       return 0;
>>> +}

-- 
Sergey

Powered by blists - more mailing lists