netdev - Re: [net-next 10/10] ixgbe: convert to ndo_fix

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] [day] [month] [year] [list]

Message-ID: <20110726173419.GA21903@rere.qmqm.pl>
Date:	Tue, 26 Jul 2011 19:34:19 +0200
From:	Michal Miroslaw <mirq-linux@...e.qmqm.pl>
To:	"Skidmore, Donald C" <donald.c.skidmore@...el.com>
Cc:	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: Re: [net-next 10/10] ixgbe: convert to ndo_fix_features

On Mon, Jul 25, 2011 at 03:42:09PM -0700, Skidmore, Donald C wrote:
>From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
>On Behalf Of Michal Miroslaw
> >On Thu, Jul 21, 2011 at 11:09:11PM -0700, Jeff Kirsher wrote:
> >> From: Don Skidmore <donald.c.skidmore@...el.com>
> >>
> >> Private rx_csum flags are now duplicate of netdev->features &
> >> NETIF_F_RXCSUM.  We remove those duplicates and now use the
> >net_device_ops
> >> ndo_set_features.  This was based on the original patch submitted by
> >> Michal Miroslaw <mirq-linux@...e.qmqm.pl>.  I also removed the special
> >> case not requiring a reset for X540 hardware.  It is needed just as it
> >is
> >> in 82599 hardware.
> >
> >Looks mostly good now. Minor hints below.
> >
> >[...]
> >> +static u32 ixgbe_fix_features(struct net_device *netdev, u32 data)
> >> +{
> >[...]
> >> +	/* Turn off LRO if not RSC capable or invalid ITR settings */
> >> +	if (!(adapter->flags2 & IXGBE_FLAG2_RSC_CAPABLE)) {
> >> +		data &= ~NETIF_F_LRO;
> >> +	} else if (!(adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) &&
> >> +		   (adapter->rx_itr_setting != 1 &&
> >> +		    adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE)) {
> >> +		data &= ~NETIF_F_LRO;
> >> +		e_info(probe, "rx-usecs set too low, not enabling RSC\n");
> >> +	}
> >
> >Better:
> >
> >... else if (data & NETIF_F_LRO && adapter->rx_itr_setting != 1 &&
> >adapter->rx_itr_setting > IXGBE_MAX_RSC_INT_RATE) {
> >	e_info(...)
> >	data &= ~NETIF_F_LRO;
> >}
> >
> 
> I see your point here, the added complexity (checking IXGBE_FLAG2_RSC_ENABLED) is there to cover cases that come up in our out of tree driver's kcompat code.  This is why we mirror these feature flags to begin with.  I would like to modify the driver to use only the feature flags but that will require some interesting kcompat code as well as touch large parts of the driver so I was thinking would probably be best as another patch.  
> 
> My concern in this conditional is when RSC is not just possible but actually turned on.  Currently the NETIF_F_LRO flag is enabled in probe if we are RSC capable.  But it is possible to be RSC capable but not have it enabled due to setting interrupts throttling rates too high.

You should base decisions in ndo_fix_features on what will be enabled in
following ndo_set_features call, not what is enabled at the time of
ndo_fix_features, unless the dependency is not controlled in ndo_set_features
(e.g. MTU).

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html