[dpdk-dev,v3,02/15] ethdev: add linkstatus get/set helper functions
Checks
Commit Message
Many drivers are all doing copy/paste of the same code to atomically
update the link status. Reduce duplication, and allow for future
changes by having common function for this.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
2 files changed, 64 insertions(+)
Comments
On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
> Many drivers are all doing copy/paste of the same code to atomically
> update the link status. Reduce duplication, and allow for future
> changes by having common function for this.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> 2 files changed, 64 insertions(+)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index a524af740f4a..6674500bbc4a 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1495,6 +1495,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
> }
> }
>
> +int
> +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> + const struct rte_eth_link *new_link)
> +{
> + volatile uint64_t *dev_link
> + = (volatile uint64_t *)&(dev->data->dev_link);
> + union {
> + uint64_t val64;
> + struct rte_eth_link link;
> + } orig;
> +
> + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> +
> + orig.val64 = rte_atomic64_exchange(dev_link,
> + *(const uint64_t *)new_link);
> +
> + return (orig.link.link_status == new_link->link_status) ? -1 : 0;
It still contradicts to: -1 if link state has changed, 0 if the same.
BTW, it looks like the return value of the link_update callback is not
specified (described) and not used. It explains why different drivers
behave differently and nobody notices it.
> +}
> +
> +void
> +_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> + struct rte_eth_link *link)
> +{
> + volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link);
> + uint64_t tmp;
> +
> + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> + /* can't use rte_atomic64_read because it returns signed int */
> + do {
> + tmp = *src;
> + } while (!rte_atomic64_cmpset(src, tmp, tmp));
> +
> + memcpy(link, &tmp, sizeof(tmp));
> +}
> +
> int
> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index b10e2a92da71..b47402b797c3 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2218,6 +2218,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
> */
> void rte_eth_dev_close(uint16_t port_id);
>
> +
> +/**
> + * @internal
> + * Atomically set the link status for the specific device.
> + * It is for use by DPDK device driver use only.
> + * User applications should not call it
> + *
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param link
> + * New link status value.
> + * @return
> + * -1 if link state has changed, 0 if the same.
> + */
> +int _rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> + const struct rte_eth_link *link);
> +
> +/**
> + * @internal
> + * Atomically get the link speed and status.
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param link
> + * link status value.
> + */
> +void _rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> + struct rte_eth_link *link);
> +
> /**
> * Reset a Ethernet device and keep its port id.
> *
On Tue, 9 Jan 2018 13:30:46 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
> > Many drivers are all doing copy/paste of the same code to atomically
> > update the link status. Reduce duplication, and allow for future
> > changes by having common function for this.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> > lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 64 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > index a524af740f4a..6674500bbc4a 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1495,6 +1495,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
> > }
> > }
> >
> > +int
> > +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> > + const struct rte_eth_link *new_link)
> > +{
> > + volatile uint64_t *dev_link
> > + = (volatile uint64_t *)&(dev->data->dev_link);
> > + union {
> > + uint64_t val64;
> > + struct rte_eth_link link;
> > + } orig;
> > +
> > + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> > +
> > + orig.val64 = rte_atomic64_exchange(dev_link,
> > + *(const uint64_t *)new_link);
> > +
> > + return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>
> It still contradicts to: -1 if link state has changed, 0 if the same.
> BTW, it looks like the return value of the link_update callback is not
> specified (described) and not used. It explains why different drivers
> behave differently and nobody notices it.
It looks like link_status callback could be void.
The only places that use return value from set are code that wants
to log something if status changed.
On 1/9/2018 4:29 PM, Stephen Hemminger wrote:
> On Tue, 9 Jan 2018 13:30:46 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 01/08/2018 08:45 PM, Stephen Hemminger wrote:
>>> Many drivers are all doing copy/paste of the same code to atomically
>>> update the link status. Reduce duplication, and allow for future
>>> changes by having common function for this.
>>>
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
<...>
>>> +int
>>> +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
>>> + const struct rte_eth_link *new_link)
I see "_rte" prefix is used to say this is internal only but this is not the
syntax for other internal APIs, only there is one using this syntax, so may be
confusing to have a mix of them.
"@internal" marker in function comment already saying that this is internal.
Also I send a patch to separate public APIs and PMD APIs to different files,
hopefully that will also help clarifying target of the APIs.
And although these two new APIs are internal, they need to be in .map file for
shared library case.
>>> +{
>>> + volatile uint64_t *dev_link
>>> + = (volatile uint64_t *)&(dev->data->dev_link);
>>> + union {
>>> + uint64_t val64;
>>> + struct rte_eth_link link;
>>> + } orig;
>>> +
>>> + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
>>> +
>>> + orig.val64 = rte_atomic64_exchange(dev_link,
>>> + *(const uint64_t *)new_link);
>>> +
>>> + return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>>
>> It still contradicts to: -1 if link state has changed, 0 if the same.
+1, function documentation in header file and this conflicts, one needs fixing.
>> BTW, it looks like the return value of the link_update callback is not
>> specified (described) and not used. It explains why different drivers
>> behave differently and nobody notices it.
>
> It looks like link_status callback could be void.
> The only places that use return value from set are code that wants
> to log something if status changed.
When a set function returns -1/0, it looks like it returns set status. Returning
if link status changes seems odd to me. What about returning original link
struct, so PMD may get this information if they want.
@@ -1495,6 +1495,42 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
}
}
+int
+_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
+ const struct rte_eth_link *new_link)
+{
+ volatile uint64_t *dev_link
+ = (volatile uint64_t *)&(dev->data->dev_link);
+ union {
+ uint64_t val64;
+ struct rte_eth_link link;
+ } orig;
+
+ RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+
+ orig.val64 = rte_atomic64_exchange(dev_link,
+ *(const uint64_t *)new_link);
+
+ return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+}
+
+void
+_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
+ struct rte_eth_link *link)
+{
+ volatile uint64_t *src = (uint64_t *)&(dev->data->dev_link);
+ uint64_t tmp;
+
+ RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+ /* can't use rte_atomic64_read because it returns signed int */
+ do {
+ tmp = *src;
+ } while (!rte_atomic64_cmpset(src, tmp, tmp));
+
+ memcpy(link, &tmp, sizeof(tmp));
+}
+
int
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
{
@@ -2218,6 +2218,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
*/
void rte_eth_dev_close(uint16_t port_id);
+
+/**
+ * @internal
+ * Atomically set the link status for the specific device.
+ * It is for use by DPDK device driver use only.
+ * User applications should not call it
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param link
+ * New link status value.
+ * @return
+ * -1 if link state has changed, 0 if the same.
+ */
+int _rte_eth_linkstatus_set(struct rte_eth_dev *dev,
+ const struct rte_eth_link *link);
+
+/**
+ * @internal
+ * Atomically get the link speed and status.
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param link
+ * link status value.
+ */
+void _rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
+ struct rte_eth_link *link);
+
/**
* Reset a Ethernet device and keep its port id.
*