Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211101035635.26999-1-ricardo.martinez@linux.intel.com> <20211101035635.26999-6-ricardo.martinez@linux.intel.com>
In-Reply-To: <20211101035635.26999-6-ricardo.martinez@linux.intel.com>
From:   Sergey Ryazanov <ryazanov.s.a@gmail.com>
Date:   Sat, 6 Nov 2021 21:07:03 +0300
Message-ID: <CAHNKnsTZhtmt6DNsEb+EvnFBo9mNPd4pPG=qjvkxyD3pH+KqrA@mail.gmail.com>
Subject: Re: [PATCH v2 05/14] net: wwan: t7xx: Add control port
To:     Ricardo Martinez <ricardo.martinez@linux.intel.com>
Cc:     netdev@vger.kernel.org, linux-wireless@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        David Miller <davem@davemloft.net>,
        Johannes Berg <johannes@sipsolutions.net>,
        Loic Poulain <loic.poulain@linaro.org>,
        M Chetan Kumar <m.chetan.kumar@intel.com>,
        chandrashekar.devegowda@intel.com,
        Intel Corporation <linuxwwan@intel.com>,
        chiranjeevi.rapolu@linux.intel.com, haijun.liu@mediatek.com,
        amir.hanania@intel.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        dinesh.sharma@intel.com, eliot.lee@intel.com,
        mika.westerberg@linux.intel.com, moises.veleta@intel.com,
        pierre-louis.bossart@intel.com, muralidharan.sethuraman@intel.com,
        Soumya.Prakash.Mishra@intel.com, sreehari.kancharla@intel.com,
        suresh.nagaraj@intel.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 1, 2021 at 6:57 AM Ricardo Martinez
<ricardo.martinez@linux.intel.com> wrote:
> Control Port implements driver control messages such as modem-host
> handshaking, controls port enumeration, and handles exception messages.
>
> The handshaking process between the driver and the modem happens during
> the init sequence. The process involves the exchange of a list of
> supported runtime features to make sure that modem and host are ready
> to provide proper feature lists including port enumeration. Further
> features can be enabled and controlled in this handshaking process.

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
> ...
> +struct feature_query {
> +       u32 head_pattern;

Looks like this field should be __le32 since it is sent to modem as is.

> +       u8 feature_set[FEATURE_COUNT];
> +       u32 tail_pattern;

Ditto.

> +};
> +
> +static void prepare_host_rt_data_query(struct core_sys_info *core)
> +{
> ...
> +       ft_query->head_pattern = MD_FEATURE_QUERY_ID;

This should be a

  ft_query->head_pattern = cpu_to_le32(MD_FEATURE_QUERY_ID);

to run on the big-endians CPU. strace will notify you about each
endians mismatch as soon as you change head_pattern field type to
__le32.

> +       memcpy(ft_query->feature_set, core->feature_set, FEATURE_COUNT);
> +       ft_query->tail_pattern = MD_FEATURE_QUERY_ID;

Ditto.

> +       /* send HS1 message to device */
> +       port_proxy_send_skb(core->ctl_port, skb, 0);
> +}

[skipped]

> diff --git a/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c b/drivers/net/wwan/t7xx/t7xx_port_ctrl_msg.c
> ...
> +static void fsm_ee_message_handler(struct sk_buff *skb)
> +{
> ...
> +       ctrl_msg_h = (struct ctrl_msg_header *)skb->data;
> ...
> +       switch (ctrl_msg_h->ctrl_msg_id) {

This should be:

        switch (le32_to_cpu(ctrl_msg_h->ctrl_msg_id)) {

> +       case CTL_ID_MD_EX:
> +               if (ctrl_msg_h->reserved != MD_EX_CHK_ID) {

Why is this field called 'reserved', but used to perform message validation?

> ...
> +static void control_msg_handler(struct t7xx_port *port, struct sk_buff *skb)
> +{
> +       struct ctrl_msg_header *ctrl_msg_h;
> ...
> +       ctrl_msg_h = (struct ctrl_msg_header *)skb->data;
> ..
> +       switch (ctrl_msg_h->ctrl_msg_id) {

This should be something like this:

        switch (le32_to_cpu(ctrl_msg_h->ctrl_msg_id)) {

--
Sergey