netdev - Re: [net-next PATCH 5/6] enic: feature add: add ethtool -c/C support

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: <C7526AA2.1AFF9%scofeldm@cisco.com>
Date:	Sat, 19 Dec 2009 11:33:54 -0800
From:	Scott Feldman <scofeldm@...co.com>
To:	Simon Horman <horms@...ge.net.au>
CC:	<davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: [net-next PATCH 5/6] enic: feature add: add ethtool -c/C support

Thanks for the review comments Simon.

On 12/19/09 2:10 AM, "Simon Horman" <horms@...ge.net.au> wrote:

> On Fri, Dec 18, 2009 at 06:10:02PM -0800, Scott Feldman wrote:
>> + /* Only rx-usecs and tx-usecs can be set */
>> + if (ecmd->rx_max_coalesced_frames || ecmd->rx_coalesce_usecs_irq ||
>> +     ecmd->rx_max_coalesced_frames_irq ||
>> +     ecmd->tx_max_coalesced_frames || ecmd->tx_coalesce_usecs_irq ||
>> +     ecmd->tx_max_coalesced_frames_irq ||
>> +     ecmd->stats_block_coalesce_usecs ||
>> +     ecmd->use_adaptive_rx_coalesce || ecmd->use_adaptive_tx_coalesce ||
>> +     ecmd->pkt_rate_low ||
>> +     ecmd->rx_coalesce_usecs_low || ecmd->rx_max_coalesced_frames_low ||
>> +     ecmd->tx_coalesce_usecs_low || ecmd->tx_max_coalesced_frames_low ||
>> +     ecmd->pkt_rate_high ||
>> +     ecmd->rx_coalesce_usecs_high ||
>> +     ecmd->rx_max_coalesced_frames_high ||
>> +     ecmd->tx_coalesce_usecs_high ||
>> +     ecmd->tx_max_coalesced_frames_high ||
>> +     ecmd->rate_sample_interval)
>> +  return -EINVAL;
> 
> This seems rather verbose.  Does it really matter if there is junk in other
> ecmd fields?  They seem to be otherwise ignored. Removing the check above
> would seem to be consistent with other enic_set_*() functions.

Ya, we can do that.
 
>> if (mtu && mtu != enic->port_mtu) {
>> +  enic->port_mtu = mtu;
>> if (mtu < enic->netdev->mtu)
>> printk(KERN_WARNING PFX
>> "%s: interface MTU (%d) set higher "
>> "than switch port MTU (%d)\n",
>> enic->netdev->name, enic->netdev->mtu, mtu);
>> -  enic->port_mtu = mtu;
>> }
>>  }
> 
> This hunk seems unrelated to the rest of the patch.
> Am I missing something?

Oops, that change snuck in there.  That line got moved due to some backward
compat changes, which have been stripped away, so it looks like that line is
moving for no reason.  It's a benign change so let's make the change to be
consistent with our internal code base.
 
>> + enic->tx_coalesce_usecs = enic->config.intr_timer_usec;
>> + enic->rx_coalesce_usecs =  enic->tx_coalesce_usecs;
>> +
> 
> It looks like there is an space after =

Got it.

-scott

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