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

[Saeed Mahameed <saeed@kernel.org>]

On Thu, 2021-05-20 at 08:48 -0700, Jakub Kicinski wrote:
> On Wed, 19 May 2021 22:36:10 -0700 Saeed Mahameed wrote:
> > On Wed, 2021-05-19 at 13:56 -0700, Jakub Kicinski wrote:
> > > On Wed, 19 May 2021 13:18:36 -0700 Saeed Mahameed wrote:  
> > > > 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
> > 
> > >  - 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.

> > > 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.