netdev - Re: [net-next 2/8] e1000e: Use rtnl_lock to prevent race conditions between net and pci/pm

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: <cf197ef61703cbaa64ac522cf5d191b4b74f64d6.camel@linux.intel.com>
Date:   Wed, 30 Oct 2019 08:48:24 -0700
From:   Alexander Duyck <alexander.h.duyck@...ux.intel.com>
To:     Joe Perches <joe@...ches.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        Morumuri Srivalli <smorumu1@...ibm.com>,
        David Dai <zdai@...ux.vnet.ibm.com>,
        Aaron Brown <aaron.f.brown@...el.com>
Subject: Re: [net-next 2/8] e1000e: Use rtnl_lock to prevent race conditions
 between net and pci/pm

On Tue, 2019-10-29 at 22:50 -0700, Joe Perches wrote:
> On Tue, 2019-10-29 at 21:36 -0700, Jeff Kirsher wrote:
> > From: Alexander Duyck <alexander.h.duyck@...ux.intel.com>
> > 
> > This patch is meant to address possible race conditions that can exist
> > between network configuration and power management. A similar issue was
> > fixed for igb in commit 9474933caf21 ("igb: close/suspend race in
> > netif_device_detach").
> []
> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> []
> > @@ -4715,12 +4715,12 @@ int e1000e_close(struct net_device *netdev)
> >  
> >  	pm_runtime_get_sync(&pdev->dev);
> >  
> > -	if (!test_bit(__E1000_DOWN, &adapter->state)) {
> > +	if (netif_device_present(netdev)) {
> >  		e1000e_down(adapter, true);
> >  		e1000_free_irq(adapter);
> >  
> >  		/* Link status message must follow this format */
> > -		pr_info("%s NIC Link is Down\n", adapter->netdev->name);
> > +		pr_info("%s NIC Link is Down\n", netdev->name);
> 
> This change is already a minor optimization, but perhaps
> this should instead be:
> 
> 	netdev_info(netdev, "NIC Link is Down\n");
> 
> And whatever pr_<level> uses should use netdev_<level>
> where possible in the driver.

I don't view it as an optimization. The test to enter this section was
based on adapter and is now based on netdev. So the info message was
changed from being adapter based and made netdev based. I suspect changing
to netdev_info is going to change the messaging since it dumps more than
just the netdevice name and adds a colon.

If we are going to change the messaging I think it would be better to do
it as a separate patch. There are a couple spots that output the message
"NIC Link is Down" so we should probably update both of those and the
"Link is UP" at the same time. Since you mentioned it do you want to patch
that or should I?

Thanks.

- Alex