Re: [PATCH net-next] mlx5: count all link events

On Thu, 2021-05-20 at 08:48 -0700, Jakub Kicinski wrote:

On Wed, 19 May 2021 22:36:10 -0700 Saeed Mahameed wrote:

quoted

On Wed, 2021-05-19 at 13:56 -0700, Jakub Kicinski wrote:

quoted

On Wed, 19 May 2021 13:18:36 -0700 Saeed Mahameed wrote:  

quoted

then according to the above assumption it is safe to make
netif_carrier_event() do everything.

netif_carrier_event(netdev, up) {
        if (dev->reg_state == NETREG_UNINITIALIZED)
                return;

        if (up == netif_carrier_ok(netdev) {
                atomic_inc(&netdev->carrier_up_count);
                atomic_inc(&netdev->carrier_down_count);
                linkwatch_fire_event(netdev);
        }

        if (up) {
                netdev_info(netdev, "Link up\n");
                netif_carrier_on(netdev);
        } else {
                netdev_info(netdev, "Link down\n");
                netif_carrier_off(netdev);
        }
}  

Two things to consider are:
 - some drivers print more info than just "link up/link down" so
they'd
   have to drop that extra stuff (as much as I'd like the
consistency)  

+1 for the consistency

quoted

 - again with the unnecessary events I was afraid that drivers
reuse 
   the same handler for device events and to read the state in
which
   case we may do something like:

        if (from_event && up == netif_carrier_ok(netdev)
  

I don't actually understand your point here .. what kind of
scenarios
it is wrong to use this function ? 

But anyway, the name of the function makes it very clear this is
from
event.. also we can document this.

I don't have any proof of this but drivers may check link state
periodically from a service job or such.

I see.

quoted

quoted

Maybe we can revisit when there's more users?  

goes both ways :), we can do what fits the requirement for mlx5 now
and
revisit in the future, if we do believe this should be general
behavior
for all/most vendors of-course!

I think it'd be more of a "add this function so the future drivers
can
use it". I've scanned the drivers I'm familiar with and none of them
seemed like they could make use of the "wider" version of the helper.
Does mlx4 need it?

No, mlx4 relies on the event type.

The problem seems slightly unusual, I feel like targeted helper would
lead to a cleaner API, but can change if we really need to..

Sure, I have no strong opinion on the matter.