* [dpdk-dev] [PATCH] ethdev: move non null judgment of ops function pointer ahead @ 2020-01-16 12:27 Wei Hu (Xavier) 2020-01-16 21:08 ` Ferruh Yigit 0 siblings, 1 reply; 3+ messages in thread From: Wei Hu (Xavier) @ 2020-01-16 12:27 UTC (permalink / raw) To: dev From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com> This patch moves the following judgement statement to the first half of the function named rte_eth_dev_set_vlan_offload, so we can avoid changing the content of dev->data->dev_conf.rxmode.offloads even when the pointer named dev->dev_ops->vlan_offload_set is NULL: RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com> Signed-off-by: Min Wang (Jushui) <wangmin3@huawei.com> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> --- lib/librte_ethdev/rte_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c index aec2d0f70..21f24cd7f 100644 --- a/lib/librte_ethdev/rte_ethdev.c +++ b/lib/librte_ethdev/rte_ethdev.c @@ -3257,6 +3257,7 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); dev = &rte_eth_devices[port_id]; + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); /* save original values in case of failure */ orig_offloads = dev->data->dev_conf.rxmode.offloads; @@ -3307,7 +3308,6 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) if (mask == 0) return ret; - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); if (ret) { /* hit an error restore original values */ -- 2.23.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: move non null judgment of ops function pointer ahead 2020-01-16 12:27 [dpdk-dev] [PATCH] ethdev: move non null judgment of ops function pointer ahead Wei Hu (Xavier) @ 2020-01-16 21:08 ` Ferruh Yigit 2020-01-17 3:04 ` Wei Hu (Xavier) 0 siblings, 1 reply; 3+ messages in thread From: Ferruh Yigit @ 2020-01-16 21:08 UTC (permalink / raw) To: Wei Hu (Xavier), dev; +Cc: Andrew Rybchenko, Thomas Monjalon On 1/16/2020 12:27 PM, Wei Hu (Xavier) wrote: > From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com> > > This patch moves the following judgement statement to the first half of > the function named rte_eth_dev_set_vlan_offload, so we can avoid changing > the content of dev->data->dev_conf.rxmode.offloads even when the pointer > named dev->dev_ops->vlan_offload_set is NULL: > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > > Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> > Signed-off-by: Chunsong Feng <fengchunsong@huawei.com> > Signed-off-by: Min Wang (Jushui) <wangmin3@huawei.com> > Signed-off-by: Min Hu (Connor) <humin29@huawei.com> > --- > lib/librte_ethdev/rte_ethdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c > index aec2d0f70..21f24cd7f 100644 > --- a/lib/librte_ethdev/rte_ethdev.c > +++ b/lib/librte_ethdev/rte_ethdev.c > @@ -3257,6 +3257,7 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > dev = &rte_eth_devices[port_id]; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > > /* save original values in case of failure */ > orig_offloads = dev->data->dev_conf.rxmode.offloads; > @@ -3307,7 +3308,6 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) > if (mask == 0) > return ret; > > - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); > ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); > if (ret) { > /* hit an error restore original values */ > I agree this side affect is a defect, thanks for spotting. Above approach would work, but also I think it was good approach to return success (0) when no change at all requested, we are loosing it here. What do you think, - Update a local 'dev_offloads' variable, instead of directly updating the device config data - if no change requested, "mask == 0", return success - if change requested, check the 'vlan_offload_set' dev_ops in this stage - assign the local 'dev_offloads' to 'dev->data->dev_conf.rxmode.offloads' and call the dev_ops - On error recover the 'dev->data->dev_conf.rxmode.offloads' to 'orig_offloads' This way both side affect can be prevented and API returns success if no change requested. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH] ethdev: move non null judgment of ops function pointer ahead 2020-01-16 21:08 ` Ferruh Yigit @ 2020-01-17 3:04 ` Wei Hu (Xavier) 0 siblings, 0 replies; 3+ messages in thread From: Wei Hu (Xavier) @ 2020-01-17 3:04 UTC (permalink / raw) To: Ferruh Yigit, Wei Hu (Xavier), dev; +Cc: Andrew Rybchenko, Thomas Monjalon Hi, Ferruh Yigit On 2020/1/17 5:08, Ferruh Yigit wrote: > On 1/16/2020 12:27 PM, Wei Hu (Xavier) wrote: >> From: "Wei Hu (Xavier)" <xavier.huwei@huawei.com> >> >> This patch moves the following judgement statement to the first half of >> the function named rte_eth_dev_set_vlan_offload, so we can avoid changing >> the content of dev->data->dev_conf.rxmode.offloads even when the pointer >> named dev->dev_ops->vlan_offload_set is NULL: >> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> >> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com> >> Signed-off-by: Chunsong Feng <fengchunsong@huawei.com> >> Signed-off-by: Min Wang (Jushui) <wangmin3@huawei.com> >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com> >> --- >> lib/librte_ethdev/rte_ethdev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c >> index aec2d0f70..21f24cd7f 100644 >> --- a/lib/librte_ethdev/rte_ethdev.c >> +++ b/lib/librte_ethdev/rte_ethdev.c >> @@ -3257,6 +3257,7 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >> >> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); >> dev = &rte_eth_devices[port_id]; >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> >> /* save original values in case of failure */ >> orig_offloads = dev->data->dev_conf.rxmode.offloads; >> @@ -3307,7 +3308,6 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask) >> if (mask == 0) >> return ret; >> >> - RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP); >> ret = (*dev->dev_ops->vlan_offload_set)(dev, mask); >> if (ret) { >> /* hit an error restore original values */ >> > > I agree this side affect is a defect, thanks for spotting. > > Above approach would work, but also I think it was good approach to return > success (0) when no change at all requested, we are loosing it here. > > What do you think, > - Update a local 'dev_offloads' variable, instead of directly updating the > device config data > - if no change requested, "mask == 0", return success > - if change requested, check the 'vlan_offload_set' dev_ops in this stage > - assign the local 'dev_offloads' to 'dev->data->dev_conf.rxmode.offloads' and > call the dev_ops > - On error recover the 'dev->data->dev_conf.rxmode.offloads' to 'orig_offloads' > > This way both side affect can be prevented and API returns success if no change > requested. > I agree your opinion and will send V2. Thanks Xavier ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-17 3:04 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-16 12:27 [dpdk-dev] [PATCH] ethdev: move non null judgment of ops function pointer ahead Wei Hu (Xavier) 2020-01-16 21:08 ` Ferruh Yigit 2020-01-17 3:04 ` Wei Hu (Xavier)
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).