Roopa Prabhu <ro...@cumulusnetworks.com> 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.
