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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210520084817.6bd770e5@kicinski-fedora-PC1C0HJN>
Date:   Thu, 20 May 2021 08:48:17 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Saeed Mahameed <saeed@...nel.org>
Cc:     Lijun Pan <ljp@...ux.vnet.ibm.com>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] mlx5: count all link events

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.

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

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