Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220407223629.21487-1-ricardo.martinez@linux.intel.com>
 <20220407223629.21487-5-ricardo.martinez@linux.intel.com> <CAHNKnsQWThUnMnk6ruGek9cuDKOAcPa18nA7-CgLf58=iEgE0g@mail.gmail.com>
 <MWHPR1101MB231920A2152DC2FC7B2FBB3CE0FB9@MWHPR1101MB2319.namprd11.prod.outlook.com>
In-Reply-To: <MWHPR1101MB231920A2152DC2FC7B2FBB3CE0FB9@MWHPR1101MB2319.namprd11.prod.outlook.com>
From:   Sergey Ryazanov <ryazanov.s.a@gmail.com>
Date:   Wed, 27 Apr 2022 02:06:16 +0300
Message-ID: <CAHNKnsQLwMMpFjOVvJUt5927g5dGUKAe_rNRuEBgCRR=KuipPg@mail.gmail.com>
Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy infrastructure
To:     "Veleta, Moises" <moises.veleta@intel.com>
Cc:     Ricardo Martinez <ricardo.martinez@linux.intel.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-wireless@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>,
        "Kumar, M Chetan" <m.chetan.kumar@intel.com>,
        "Devegowda, Chandrashekar" <chandrashekar.devegowda@intel.com>,
        linuxwwan <linuxwwan@intel.com>,
        "chiranjeevi.rapolu@linux.intel.com" 
        <chiranjeevi.rapolu@linux.intel.com>,
        "Liu, Haijun" <haijun.liu@mediatek.com>,
        "Hanania, Amir" <amir.hanania@intel.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "Sharma, Dinesh" <dinesh.sharma@intel.com>,
        "Lee, Eliot" <eliot.lee@intel.com>,
        "Jarvinen, Ilpo Johannes" <ilpo.johannes.jarvinen@intel.com>,
        "Bossart, Pierre-louis" <pierre-louis.bossart@intel.com>,
        "Sethuraman, Muralidharan" <muralidharan.sethuraman@intel.com>,
        "Mishra, Soumya Prakash" <soumya.prakash.mishra@intel.com>,
        "Kancharla, Sreehari" <sreehari.kancharla@intel.com>,
        "Sahu, Madhusmita" <madhusmita.sahu@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Moises,

On Tue, Apr 26, 2022 at 10:46 PM Veleta, Moises <moises.veleta@intel.com> w=
rote:
> From: Sergey Ryazanov <ryazanov.s.a@gmail.com>
> Sent: Monday, April 25, 2022 4:53 PM
> To: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> Cc: netdev@vger.kernel.org <netdev@vger.kernel.org>; linux-wireless@vger.=
kernel.org <linux-wireless@vger.kernel.org>; Jakub Kicinski <kuba@kernel.or=
g>; David Miller <davem@davemloft.net>; Johannes Berg <johannes@sipsolution=
s.net>; Loic Poulain <loic.poulain@linaro.org>; Kumar, M Chetan <m.chetan.k=
umar@intel.com>; Devegowda, Chandrashekar <chandrashekar.devegowda@intel.co=
m>; linuxwwan <linuxwwan@intel.com>; chiranjeevi.rapolu@linux.intel.com <ch=
iranjeevi.rapolu@linux.intel.com>; Liu, Haijun <haijun.liu@mediatek.com>; H=
anania, Amir <amir.hanania@intel.com>; Andy Shevchenko <andriy.shevchenko@l=
inux.intel.com>; Sharma, Dinesh <dinesh.sharma@intel.com>; Lee, Eliot <elio=
t.lee@intel.com>; Jarvinen, Ilpo Johannes <ilpo.johannes.jarvinen@intel.com=
>; Veleta, Moises <moises.veleta@intel.com>; Bossart, Pierre-louis <pierre-=
louis.bossart@intel.com>; Sethuraman, Muralidharan <muralidharan.sethuraman=
@intel.com>; Mishra, Soumya Prakash <soumya.prakash.mishra@intel.com>; Kanc=
harla, Sreehari <sreehari.kancharla@intel.com>; Sahu, Madhusmita <madhusmit=
a.sahu@intel.com>
> Subject: Re: [PATCH net-next v6 04/13] net: wwan: t7xx: Add port proxy in=
frastructure
>
> On Fri, Apr 8, 2022 at 1:37 AM Ricardo Martinez
> <ricardo.martinez@linux.intel.com> 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`.
>>
>> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com>
>> Co-developed-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.=
com>
>> Signed-off-by: Chandrashekar Devegowda <chandrashekar.devegowda@intel.co=
m>
>> Co-developed-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
>>
>> From a WWAN framework perspective:
>> Reviewed-by: Loic Poulain <loic.poulain@linaro.org>
>
> [skipped]
>
>> +int t7xx_port_enqueue_skb(struct t7xx_port *port, struct sk_buff *skb)
>> +{
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&port->rx_wq.lock, flags);
>> +       if (port->rx_skb_list.qlen >=3D port->rx_length_th) {
>> +               spin_unlock_irqrestore(&port->rx_wq.lock, flags);
>
> Probably skb should be freed here before returning. The caller assumes
> that skb will be consumed even in case of error.
>
> [MV] We do not drop port ctrl messages. We keep them and try again later.
> Whereas WWAN skbs are dropped if conditions are met.

I missed that the WWAN port returns no error when it drops the skb.
And then I concluded that any failure to process the CCCI message
should be accomplished with the skb freeing. Now the handling of CCCI
messages is more clear to me. Thank you for the clarifications!

To avoid similar misinterpretation in the future, I thought that the
skb freeing in the WWAN port worth a comment. Something to describe
that despite dropping the message, the return code is zero, indicating
skb consumption. Similarly in this (t7xx_port_enqueue_skb) function.
Something like: "return an error here, the caller will try again
later". And maybe in t7xx_cldma_gpd_rx_from_q() near the loop break
after the .skb_recv() failure test. Something like: "break message
processing for now will try this message later".

What do you think?

>> +               return -ENOBUFS;
>> +       }
>> +       __skb_queue_tail(&port->rx_skb_list, skb);
>> +       spin_unlock_irqrestore(&port->rx_wq.lock, flags);
>> +
>> +       wake_up_all(&port->rx_wq);
>> +       return 0;
>> +}
>
> [skipped]
>
>> +static int t7xx_port_proxy_recv_skb(struct cldma_queue *queue, struct s=
k_buff *skb)
>> +{
>> +       struct ccci_header *ccci_h =3D (struct ccci_header *)skb->data;
>> +       struct t7xx_pci_dev *t7xx_dev =3D queue->md_ctrl->t7xx_dev;
>> +       struct t7xx_fsm_ctl *ctl =3D t7xx_dev->md->fsm_ctl;
>> +       struct device *dev =3D queue->md_ctrl->dev;
>> +       struct t7xx_port_conf *port_conf;
>> +       struct t7xx_port *port;
>> +       u16 seq_num, channel;
>> +       int ret;
>> +
>> +       if (!skb)
>> +               return -EINVAL;
>> +
>> +       channel =3D FIELD_GET(CCCI_H_CHN_FLD, le32_to_cpu(ccci_h->status=
));
>> +       if (t7xx_fsm_get_md_state(ctl) =3D=3D MD_STATE_INVALID) {
>> +               dev_err_ratelimited(dev, "Packet drop on channel 0x%x, m=
odem not ready\n", channel);
>> +               goto drop_skb;
>> +       }
>> +
>> +       port =3D t7xx_port_proxy_find_port(t7xx_dev, queue, channel);
>> +       if (!port) {
>> +               dev_err_ratelimited(dev, "Packet drop on channel 0x%x, p=
ort not found\n", channel);
>> +               goto drop_skb;
>> +       }
>> +
>> +       seq_num =3D t7xx_port_next_rx_seq_num(port, ccci_h);
>> +       port_conf =3D port->port_conf;
>> +       skb_pull(skb, sizeof(*ccci_h));
>> +
>> +       ret =3D port_conf->ops->recv_skb(port, skb);
>> +       if (ret) {
>> +               skb_push(skb, sizeof(*ccci_h));
>
> Header can not be pushed back here, since the .recv_skb() callback
> consumes (frees) skb even in case of error. See
> t7xx_port_wwan_recv_skb() and my comment in t7xx_port_enqueue_skb().
> Pushing the header back after failure will trigger the use-after-free
> error.
>
> [MV] Since t7xx_port_wwan_recv_skb returns 0 when dropping a skb, it skip=
s this push operation and
> will not trigger the error stated. Inside of that function an error is pr=
inted.
>
>> +               return ret;
>> +       }
>> +
>> +       port->seq_nums[MTK_RX] =3D seq_num;
>
> The expected sequence number updated only on successful .recv_skb()
> exit. This will trigger the out-of-order seqno warning on a next
> message after a .recv_skb() failure. Is this intentional behaviour?
>
> Maybe the expected sequence number should be updated before the
> .recv_skb() call? Or even the sequence number update should be moved
> to t7xx_port_next_rx_seq_num()?
>
> [MV] For t7xx_port_wwan_recv_skb, it drops skb and seq_nums is updated.
> for t7xx_port_enqueue_skb, it retains skb and can be processed again late=
r, skipping this update upon error.

Now this is clear to me. Thanks.

>> +       return 0;
>> +
>> +drop_skb:
>> +       dev_kfree_skb_any(skb);
>> +       return 0;
>> +}

--=20
Sergey