Subject: Re: [RFC PATCH net-next 1/3] net: rtnetlink: Add link-down reason to RTNL messages

From: Petr Machata <[email-masked]>
Date: Mon, 18 Mar 2019 12:15:56 +0000

Roopa Prabhu <[email-masked]> writes:

> This looks good and will be use-full. But i have some comments on the
> implementation below.
> Also, carrier can go down due to protocol down (IFLA_PROTODOWN). And I
> get asked about supporting
> reason codes or protocol owner for such protodown reason (I have not
> given it much thought yet. I will see if there is a way
> to use your series for that as well).

My thinking was that since protocol down is set from userspace, it's
under admin control, and that's where the reason signalling should be.

>> ---
>>  include/net/rtnetlink.h      |  3 +++
>>  include/uapi/linux/if_link.h | 16 ++++++++++++++++
>>  net/core/rtnetlink.c         | 22 ++++++++++++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
>> index e2091bb2b3a8..cfd9e86ff0ca 100644
>> --- a/include/net/rtnetlink.h
>> +++ b/include/net/rtnetlink.h
>> @@ -110,6 +110,9 @@ struct rtnl_link_ops {
>>         int                     (*fill_linkxstats)(struct sk_buff *skb,
>>                                                    const struct net_device *dev,
>>                                                    int *prividx, int attr);
>> +       size_t                  (*link_down_reason_get_size)(const struct net_device *dev);
>> +       int                     (*fill_link_down_reason)(struct sk_buff *skb,
>> +                                                        const struct net_device *dev);
>>  };
>
> Any reason to restrict this to network interfaces which support rtnl_link_ops ?.
> I also saw that you added rtnl_link_ops to mlxsw for this. Which also
> means every driver will now have to declare rtnl_link_ops to use this
> ?.
> I think we should keep rtnl_link_ops to logical links  like bridge,
> bonds etc (ie which support newlink and dellink).
>
> Can't we use netdev_ops for this ?. That will allow any driver to just
> support this readily.

I guess you are right.

>> +enum rtnl_link_down_reason_major {
>> +       RTNL_LDR_OTHER,
>> +       RTNL_LDR_NO_CABLE,
>> +       RTNL_LDR_UNSUPPORTED_CABLE,
>> +       RTNL_LDR_AUTONEG_FAILURE,
>> +       RTNL_LDR_NO_LINK_PARTNER,
>> +       RTNL_LDR_LINK_TRAINING_FAILURE,
>> +       RTNL_LDR_LOGICAL_MISMATCH,
>> +       RTNL_LDR_REMOTE_FAULT,
>> +       RTNL_LDR_BAD_SIGNAL_INTEGRITY,
>> +       RTNL_LDR_CALIBRATION_FAILURE,
>> +       RTNL_LDR_POWER_BUDGET_EXCEEDED,
>> +};
>
> prefer LINK_DOWN_REASON_* or LINKDOWN_REASON_*
> (Though there is no predefined convention, the prefix RTNL makes it
> feel like a top-level attribute when its really a value for an IFLA_*
> attribute.)

OK.