From: Ferruh Yigit <ferruh.yigit@xilinx.com>
To: Chaoyong He <chaoyong.he@corigine.com>,
"dev@dpdk.org" <dev@dpdk.org>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Cc: oss-drivers <oss-drivers@corigine.com>,
Niklas Soderlund <niklas.soderlund@corigine.com>
Subject: Re: [PATCH v7 05/12] net/nfp: add flower PF setup and mempool init logic
Date: Tue, 6 Sep 2022 11:18:39 +0100 [thread overview]
Message-ID: <8a38e1ca-ecae-0a38-9d06-841fb29387e6@xilinx.com> (raw)
In-Reply-To: <SJ0PR13MB5545DCA0F8A73CF2333DAE3E9E7E9@SJ0PR13MB5545.namprd13.prod.outlook.com>
On 9/6/2022 9:45 AM, Chaoyong He wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
>> Sent: Monday, September 5, 2022 11:42 PM
>> To: Chaoyong He <chaoyong.he@corigine.com>; dev@dpdk.org
>> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
>> <niklas.soderlund@corigine.com>
>> Subject: Re: [PATCH v7 05/12] net/nfp: add flower PF setup and mempool
>> init logic
>>
>> On 8/12/2022 11:22 AM, Chaoyong He wrote:
>>> Adds the vNIC initialization logic for the flower PF vNIC. The flower
>>> firmware exposes this vNIC for the purposes of fallback traffic in the
>>> switchdev use-case. The logic of setting up this vNIC is similar to
>>> the logic seen in nfp_net_init() and nfp_net_start().
>>>
>>> Adds minimal dev_ops for this PF device. Because the device is being
>>> exposed externally to DPDK it should also be configured using DPDK
>>> helpers like rte_eth_configure(). For these helpers to work the flower
>>> logic needs to implements a minimal set of dev_ops. The Rx and Tx
>>> logic for this vNIC will be added in a subsequent commit.
>>>
>>> OVS expects incoming packets coming into the OVS datapath to be
>>> allocated from a mempool that contains objects of type "struct
>>> dp_packet". For the PF handling the slowpath into OVS it should use a
>>> mempool that is compatible with OVS. This commit adds the logic to
>>> create the OVS compatible mempool. It adds certain OVS specific
>>> structs to be able to instantiate the mempool.
>>>
>>
>> Can you please elaborate what is OVS compatible mempool?
>>
>> <...>
>>
>>> +static inline struct nfp_app_flower * nfp_app_flower_priv_get(struct
>>> +nfp_pf_dev *pf_dev) {
>>> + if (pf_dev == NULL)
>>> + return NULL;
>>> + else if (pf_dev->app_id != NFP_APP_FLOWER_NIC)
>>> + return NULL;
>>> + else
>>> + return (struct nfp_app_flower *)pf_dev->app_priv; }
>>> +
>>
>> What do you think to unify functions to get private data, instead of having a
>> function for each FW, it can be possible to have single one?
>>
>
> At first, we use two macros for this, and Andrew advice change them to functions.
> ```
> #define NFP_APP_PRIV_TO_APP_NIC(app_priv)\
> ((struct nfp_app_nic *)app_priv)
>
> #define NFP_APP_PRIV_TO_APP_FLOWER(app_priv)\
> ((struct nfp_app_flower *)app_priv)
> ```
> So your advice is we unify the functions into:
> ```
> static inline struct nfp_app_nic *
> nfp_app_priv_get(struct nfp_pf_dev *pf_dev)
> {
> if (pf_dev == NULL)
> return NULL;
> else if (pf_dev->app_id == NFP_APP_CORE_NIC ||
> pf_dev->app_id == NFP_APP_FLOWER_NIC)
> return pf_dev->app_priv;
> else
> return NULL;
> }
> ```
> and convert the pointer type at where this function been called?
Since return pointer types are different, it should return "void *",
```
static inline void *
nfp_app_priv_get(struct nfp_pf_dev *pf_dev)
{
if (pf_dev == NULL)
return NULL;
else if (pf_dev->app_id == NFP_APP_CORE_NIC ||
pf_dev->app_id == NFP_APP_FLOWER_NIC)
return pf_dev->app_priv;
else
return NULL;
}
```
And when assigning a pointer from "void *", no explicit cast is required.
```
struct nfp_app_flower *app_flower;
app_flower = nfp_app_priv_get(pf_dev);
```
I think this is better to have single function, instead of different
helper function for each FW, but I would like to get @Andrew's comment too.
Btw, since 'nfp_app_nic*_priv_get' return 'NULL' now, should callers
check for NULL, this may introduce too many checks, and if checks are
not necessary, what is the benefit of the function against macro?
next prev parent reply other threads:[~2022-09-06 10:18 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-12 10:22 [PATCH v7 00/12] preparation for the rte_flow offload of nfp PMD Chaoyong He
2022-08-12 10:22 ` [PATCH v7 01/12] net/nfp: move app specific attributes to own struct Chaoyong He
2022-09-05 15:38 ` Ferruh Yigit
2022-09-06 9:20 ` Chaoyong He
2022-09-06 9:35 ` Ferruh Yigit
2022-08-12 10:22 ` [PATCH v7 02/12] net/nfp: simplify initialization and remove dead code Chaoyong He
2022-08-12 10:22 ` [PATCH v7 03/12] net/nfp: move app specific init logic to own function Chaoyong He
2022-09-05 15:38 ` Ferruh Yigit
2022-09-06 8:29 ` Chaoyong He
2022-09-06 9:47 ` Ferruh Yigit
2022-08-12 10:22 ` [PATCH v7 04/12] net/nfp: add initial flower firmware support Chaoyong He
2022-09-05 15:39 ` Ferruh Yigit
2022-09-07 8:22 ` Chaoyong He
2022-09-07 13:24 ` Ferruh Yigit
2022-08-12 10:22 ` [PATCH v7 05/12] net/nfp: add flower PF setup and mempool init logic Chaoyong He
2022-09-05 15:42 ` Ferruh Yigit
2022-09-06 8:45 ` Chaoyong He
2022-09-06 10:18 ` Ferruh Yigit [this message]
2022-08-12 10:22 ` [PATCH v7 06/12] net/nfp: add flower PF related routines Chaoyong He
2022-08-12 10:22 ` [PATCH v7 07/12] net/nfp: add flower ctrl VNIC related logics Chaoyong He
2022-08-12 10:22 ` [PATCH v7 08/12] net/nfp: move common rxtx function for flower use Chaoyong He
2022-08-12 10:22 ` [PATCH v7 09/12] net/nfp: add flower ctrl VNIC rxtx logic Chaoyong He
2022-09-05 15:42 ` Ferruh Yigit
2022-08-12 10:22 ` [PATCH v7 10/12] net/nfp: add flower representor framework Chaoyong He
2022-08-12 10:22 ` [PATCH v7 11/12] net/nfp: move rxtx function to header file Chaoyong He
2022-08-12 10:22 ` [PATCH v7 12/12] net/nfp: add flower PF rxtx logic Chaoyong He
2022-09-01 22:10 ` [PATCH v7 00/12] preparation for the rte_flow offload of nfp PMD Niklas Söderlund
2022-09-05 15:40 ` Ferruh Yigit
Reply instructions:
You may reply publicly 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=8a38e1ca-ecae-0a38-9d06-841fb29387e6@xilinx.com \
--to=ferruh.yigit@xilinx.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=chaoyong.he@corigine.com \
--cc=dev@dpdk.org \
--cc=niklas.soderlund@corigine.com \
--cc=oss-drivers@corigine.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).