Signed-off-by: Jan-Bernd Themann <[email protected]>
drivers/net/ehea/ehea_ethtool.c | 264 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 264 insertions(+)
--- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_ethtool.c 1969-12-31 16:00:00.000000000 -0800
+++ kernel/drivers/net/ehea/ehea_ethtool.c 2006-08-18 00:01:02.841367875 -0700
@@ -0,0 +1,264 @@
+/*
+ * linux/drivers/net/ehea/ehea_ethtool.c
+ *
+ * eHEA ethernet device driver for IBM eServer System p
+ *
+ * (C) Copyright IBM Corp. 2006
+ *
+ * Authors:
+ * Christoph Raisch <[email protected]>
+ * Jan-Bernd Themann <[email protected]>
+ * Thomas Klein <[email protected]>
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include "ehea.h"
+#include "ehea_phyp.h"
+
+
+static int netdev_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+ u64 hret = H_HARDWARE;
+ struct ehea_port *port = netdev_priv(dev);
+ struct ehea_adapter *adapter = port->adapter;
+ struct hcp_query_ehea_port_cb_4 *cb4 = NULL;
+
+ cb4 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
+ if(!cb4) {
+ ehea_error("no mem for cb4");
+ goto get_settings_exit;
+ }
+
+ hret = ehea_h_query_ehea_port(adapter->handle,
+ port->logical_port_id,
+ H_PORT_CB4,
+ H_PORT_CB4_ALL,
+ cb4);
+
+ if (hret != H_SUCCESS) {
+ ehea_error("query_ehea_port failed");
+ kfree(cb4);
+ goto get_settings_exit;
+ }
+
+ if (netif_msg_hw(port))
+ ehea_dump(cb4, sizeof(struct hcp_query_ehea_port_cb_4),
+ "netdev_get_settings");
+
+ if (netif_carrier_ok(dev)) {
+ switch(cb4->port_speed){
+ case H_PORT_SPEED_10M_H:
+ cmd->speed = SPEED_10;
+ cmd->duplex = DUPLEX_HALF;
+ break;
+ case H_PORT_SPEED_10M_F:
+ cmd->speed = SPEED_10;
+ cmd->duplex = DUPLEX_FULL;
+ break;
+ case H_PORT_SPEED_100M_H:
+ cmd->speed = SPEED_100;
+ cmd->duplex = DUPLEX_HALF;
+ break;
+ case H_PORT_SPEED_100M_F:
+ cmd->speed = SPEED_100;
+ cmd->duplex = DUPLEX_FULL;
+ break;
+ case H_PORT_SPEED_1G_F:
+ cmd->speed = SPEED_1000;
+ cmd->duplex = DUPLEX_FULL;
+ break;
+ case H_PORT_SPEED_10G_F:
+ cmd->speed = SPEED_10000;
+ cmd->duplex = DUPLEX_FULL;
+ break;
+ }
+ } else {
+ cmd->speed = -1;
+ cmd->duplex = -1;
+ }
+
+ cmd->supported = (SUPPORTED_10000baseT_Full | SUPPORTED_1000baseT_Full
+ | SUPPORTED_100baseT_Full | SUPPORTED_100baseT_Half
+ | SUPPORTED_10baseT_Full | SUPPORTED_10baseT_Half
+ | SUPPORTED_Autoneg | SUPPORTED_FIBRE);
+
+ cmd->advertising = (ADVERTISED_10000baseT_Full | ADVERTISED_Autoneg
+ | ADVERTISED_FIBRE);
+
+ cmd->port = PORT_FIBRE;
+ cmd->autoneg = AUTONEG_ENABLE;
+
+ kfree(cb4);
+ return 0;
+
+get_settings_exit:
+ return 1;
+}
+
+static void netdev_get_drvinfo(struct net_device *dev,
+ struct ethtool_drvinfo *info)
+{
+ strncpy(info->driver, DRV_NAME, sizeof(info->driver) - 1);
+ strncpy(info->version, DRV_VERSION, sizeof(info->version) - 1);
+}
+
+static u32 netdev_get_msglevel(struct net_device *dev)
+{
+ struct ehea_port *port = netdev_priv(dev);
+ return port->msg_enable;
+}
+
+static void netdev_set_msglevel(struct net_device *dev, u32 value)
+{
+ struct ehea_port *port = netdev_priv(dev);
+ port->msg_enable = value;
+}
+
+static char ehea_ethtool_stats_keys[][ETH_GSTRING_LEN] = {
+ {"poll_max_processed"},
+ {"queue_stopped"},
+ {"min_swqe_avail"},
+ {"poll_receive_err"},
+ {"pkt_send"},
+ {"pkt_xmit"},
+ {"send_tasklet"},
+ {"ehea_poll"},
+ {"nwqe"},
+ {"swqe_available_0"},
+ {"rxo"},
+ {"rx64"},
+ {"rx65"},
+ {"rx128"},
+ {"rx256"},
+ {"rx512"},
+ {"rx1024"},
+ {"txo"},
+ {"tx64"},
+ {"tx65"},
+ {"tx128"},
+ {"tx256"},
+ {"tx512"},
+ {"tx1024"},
+};
+
+static void netdev_get_strings(struct net_device *dev,
+ u32 stringset, u8 * data)
+{
+ switch (stringset) {
+ case ETH_SS_TEST:
+ break;
+ case ETH_SS_STATS:
+ memcpy(data, &ehea_ethtool_stats_keys,
+ sizeof(ehea_ethtool_stats_keys));
+ }
+}
+
+static int netdev_get_stats_count(struct net_device *dev)
+{
+ return ARRAY_SIZE(ehea_ethtool_stats_keys);
+}
+
+static void netdev_get_ethtool_stats(struct net_device *dev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ int i = 0;
+ u64 hret = H_HARDWARE;
+ struct ehea_port *port = netdev_priv(dev);
+ struct ehea_adapter *adapter = port->adapter;
+ struct ehea_port_res *pr = &port->port_res[0];
+ struct port_state *p_state = &pr->p_state;
+ struct hcp_query_ehea_port_cb_6 *cb6 = NULL;
+
+ cb6 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
+ if(!cb6) {
+ ehea_error("no mem for cb6");
+ goto stats_exit;
+ }
+
+ hret = ehea_h_query_ehea_port(adapter->handle,
+ port->logical_port_id,
+ H_PORT_CB6,
+ H_PORT_CB6_ALL,
+ cb6);
+ if (hret != H_SUCCESS)
+ ehea_error("query_ehea_port failed");
+
+ if (netif_msg_hw(port))
+ ehea_dump(cb6, sizeof(struct hcp_query_ehea_port_cb_6),
+ "netdev_get_ethtool_stats");
+
+ data[i++] = p_state->poll_max_processed;
+ data[i++] = p_state->queue_stopped;
+ data[i++] = p_state->min_swqe_avail;
+ data[i++] = p_state->poll_receive_errors;
+ data[i++] = p_state->pkt_send;
+ data[i++] = p_state->pkt_xmit;
+ data[i++] = p_state->send_tasklet;
+ data[i++] = p_state->ehea_poll;
+ data[i++] = p_state->nwqe;
+ data[i++] = atomic_read(&port->port_res[0].swqe_avail);
+
+ data[i++] = cb6->rxo;
+ data[i++] = cb6->rx64;
+ data[i++] = cb6->rx65;
+ data[i++] = cb6->rx128;
+ data[i++] = cb6->rx256;
+ data[i++] = cb6->rx512;
+ data[i++] = cb6->rx1024;
+ data[i++] = cb6->txo;
+ data[i++] = cb6->tx64;
+ data[i++] = cb6->tx65;
+ data[i++] = cb6->tx128;
+ data[i++] = cb6->tx256;
+ data[i++] = cb6->tx512;
+ data[i++] = cb6->tx1024;
+
+ kfree(cb6);
+stats_exit:
+ return;
+}
+
+struct ethtool_ops ehea_ethtool_ops = {
+ .get_settings = netdev_get_settings,
+ .get_drvinfo = netdev_get_drvinfo,
+ .get_msglevel = netdev_get_msglevel,
+ .set_msglevel = netdev_set_msglevel,
+ .get_link = ethtool_op_get_link,
+ .get_tx_csum = ethtool_op_get_tx_csum,
+ .set_tx_csum = ethtool_op_set_tx_csum,
+ .get_sg = ethtool_op_get_sg,
+ .set_sg = ethtool_op_set_sg,
+ .get_tso = ethtool_op_get_tso,
+ .set_tso = ethtool_op_set_tso,
+ .get_strings = netdev_get_strings,
+ .get_stats_count = netdev_get_stats_count,
+ .get_ethtool_stats = netdev_get_ethtool_stats,
+ .set_settings = NULL,
+ .nway_reset = NULL,
+ .get_pauseparam = NULL,
+ .set_pauseparam = NULL,
+ .get_rx_csum = NULL,
+ .set_rx_csum = NULL,
+ .phys_id = NULL,
+ .self_test = NULL,
+ .self_test_count = NULL
+};
+
+void ehea_set_ethtool_ops(struct net_device *netdev)
+{
+ SET_ETHTOOL_OPS(netdev, &ehea_ethtool_ops);
+}
On Fri, Aug 18, 2006 at 01:33:22PM +0200, Jan-Bernd Themann wrote:
> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_ethtool.c
> +++ kernel/drivers/net/ehea/ehea_ethtool.c
> +static int netdev_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> + u64 hret = H_HARDWARE;
useless assignment;
> + struct ehea_port *port = netdev_priv(dev);
> + struct ehea_adapter *adapter = port->adapter;
> + struct hcp_query_ehea_port_cb_4 *cb4 = NULL;
> +
> + cb4 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> + if(!cb4) {
> + ehea_error("no mem for cb4");
> + goto get_settings_exit;
> + }
> +
> + hret = ehea_h_query_ehea_port(adapter->handle,
> + port->logical_port_id,
> + H_PORT_CB4,
> + H_PORT_CB4_ALL,
> + cb4);
> +static void netdev_get_drvinfo(struct net_device *dev,
> + struct ethtool_drvinfo *info)
> +{
> + strncpy(info->driver, DRV_NAME, sizeof(info->driver) - 1);
> + strncpy(info->version, DRV_VERSION, sizeof(info->version) - 1);
Use strlcpy() to not forget -1 accidently.
> +static u32 netdev_get_msglevel(struct net_device *dev)
^^^^^^^^
> +{
> + struct ehea_port *port = netdev_priv(dev);
> + return port->msg_enable;
^^^^^^
Something is mis-named here.
> +}
> +
> +static void netdev_set_msglevel(struct net_device *dev, u32 value)
> +{
> + struct ehea_port *port = netdev_priv(dev);
> + port->msg_enable = value;
> +}
And here.
> +static void netdev_get_ethtool_stats(struct net_device *dev,
> + struct ethtool_stats *stats, u64 *data)
> +{
> + int i = 0;
> + u64 hret = H_HARDWARE;
Useless assignment.
> + struct ehea_port *port = netdev_priv(dev);
> + struct ehea_adapter *adapter = port->adapter;
> + struct ehea_port_res *pr = &port->port_res[0];
> + struct port_state *p_state = &pr->p_state;
> + struct hcp_query_ehea_port_cb_6 *cb6 = NULL;
Ditto.
> + cb6 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> + if(!cb6) {
> + ehea_error("no mem for cb6");
> + goto stats_exit;
> + }
> +
> + hret = ehea_h_query_ehea_port(adapter->handle,
> + port->logical_port_id,
> + H_PORT_CB6,
> + H_PORT_CB6_ALL,
> + cb6);
> +struct ethtool_ops ehea_ethtool_ops = {
> + .get_settings = netdev_get_settings,
> + .get_drvinfo = netdev_get_drvinfo,
> + .get_msglevel = netdev_get_msglevel,
> + .set_msglevel = netdev_set_msglevel,
> + .get_link = ethtool_op_get_link,
> + .get_tx_csum = ethtool_op_get_tx_csum,
Whitespace breakage.
> + .set_tx_csum = ethtool_op_set_tx_csum,
> + .get_sg = ethtool_op_get_sg,
> + .set_sg = ethtool_op_set_sg,
> + .get_tso = ethtool_op_get_tso,
> + .set_tso = ethtool_op_set_tso,
> + .get_strings = netdev_get_strings,
> + .get_stats_count = netdev_get_stats_count,
> + .get_ethtool_stats = netdev_get_ethtool_stats,
> + .set_settings = NULL,
> + .nway_reset = NULL,
> + .get_pauseparam = NULL,
> + .set_pauseparam = NULL,
> + .get_rx_csum = NULL,
> + .set_rx_csum = NULL,
> + .phys_id = NULL,
> + .self_test = NULL,
> + .self_test_count = NULL
If you don't use them, don't mention them at all. Compiler will DTRT.
Hi Alexey,
first of all thanks a lot for the extensive review.
Alexey Dobriyan wrote:
>> + u64 hret = H_HARDWARE;
>
> Useless assignment here and everywhere.
>
Initializing returncodes to errorstate is a cheap way to prevent
accidentally returning (uninitalized) success returncodes which
can lead to catastrophic misbehaviour.
>> +static void netdev_get_drvinfo(struct net_device *dev,
>> + struct ethtool_drvinfo *info)
>> +{
>> + strncpy(info->driver, DRV_NAME, sizeof(info->driver) - 1);
>> + strncpy(info->version, DRV_VERSION, sizeof(info->version) - 1);
>
> Use strlcpy() to not forget -1 accidently.
I agree.
Kind regards
Thomas
On Fri, 18 Aug 2006 17:41:26 +0200
Thomas Klein <[email protected]> wrote:
> Hi Alexey,
>
> first of all thanks a lot for the extensive review.
>
>
> Alexey Dobriyan wrote:
> >> + u64 hret = H_HARDWARE;
> >
> > Useless assignment here and everywhere.
> >
>
> Initializing returncodes to errorstate is a cheap way to prevent
> accidentally returning (uninitalized) success returncodes which
> can lead to catastrophic misbehaviour.
That is old thinking. Current compilers do live/dead analysis
and tell you about this at compile time which is better than relying
on default behavior at runtime.
On Fri, 2006-08-18 at 17:41 +0200, Thomas Klein wrote:
> Hi Alexey,
>
> first of all thanks a lot for the extensive review.
>
>
> Alexey Dobriyan wrote:
> >> + u64 hret = H_HARDWARE;
> >
> > Useless assignment here and everywhere.
> >
>
> Initializing returncodes to errorstate is a cheap way to prevent
> accidentally returning (uninitalized) success returncodes which
> can lead to catastrophic misbehaviour.
If you try to return an uninitialized value the compiler will warn you,
you'll then look at the code and realise you missed a case, you might
save yourself a bug. By unconditionally initialising you are lying to
the compiler, and it can no longer help you.
cheers
--
Michael Ellerman
IBM OzLabs
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
On Sat, 2006-08-19 at 16:18 +1000, Michael Ellerman wrote:
>
> If you try to return an uninitialized value the compiler will warn you,
> you'll then look at the code and realise you missed a case, you might
> save yourself a bug.
You *should* look at the code :)
So should we be reporting these as bugs?
andy@cx02:~/linux/linux-2.6.17.6$ script make.script
Script started, file is make.script
andy@cx02:~/linux/linux-2.6.17.6$ make
...
Script done, file is make.script
andy@cx02:~/linux/linux-2.6.17.6$ fgrep warning make.script
arch/i386/kernel/cpu/transmeta.c:12: warning: 'cpu_freq' may be used uninitialized in this function
fs/bio.c:169: warning: 'idx' may be used uninitialized in this function
fs/eventpoll.c:500: warning: 'fd' may be used uninitialized in this function
fs/isofs/namei.c:162: warning: 'offset' may be used uninitialized in this function
fs/isofs/namei.c:162: warning: 'block' may be used uninitialized in this function
fs/nfsd/nfsctl.c:292: warning: 'maxsize' may be used uninitialized in this function
fs/udf/balloc.c:751: warning: 'goal_eloc.logicalBlockNum' may be used uninitialized in this function
fs/udf/super.c:1358: warning: 'ino.partitionReferenceNum' may be used uninitialized in this function
fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_startblock' may be used uninitialized in this function
fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_blockcount' may be used uninitialized in this function
fs/xfs/xfs_bmap.c:2498: warning: 'rtx' is used uninitialized in this function
fs/xfs/xfs_bmap_btree.c:753: warning: 'nkey.br_startoff' may be used uninitialized in this function
fs/xfs/xfs_da_btree.c:151: warning: 'action' may be used uninitialized in this function
fs/xfs/xfs_dir.c:363: warning: 'totallen' may be used uninitialized in this function
fs/xfs/xfs_dir.c:363: warning: 'count' may be used uninitialized in this function
fs/xfs/xfs_ialloc_btree.c:545: warning: 'nkey.ir_startino' may be used uninitialized in this function
fs/xfs/xfs_inode.c:1958: warning: 'last_dip' may be used uninitialized in this function
fs/xfs/xfs_inode.c:1960: warning: 'last_offset' may be used uninitialized in this function
fs/xfs/xfs_log.c:1749: warning: 'iclog' may be used uninitialized in this function
fs/xfs/xfs_log_recover.c:523: warning: 'first_blk' may be used uninitialized in this function
ipc/msg.c:338: warning: 'setbuf.qbytes' may be used uninitialized in this function
ipc/msg.c:338: warning: 'setbuf.uid' may be used uninitialized in this function
ipc/msg.c:338: warning: 'setbuf.gid' may be used uninitialized in this function
ipc/msg.c:338: warning: 'setbuf.mode' may be used uninitialized in this function
ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function
ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function
ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function
drivers/md/dm-table.c:431: warning: 'dev' may be used uninitialized in this function
drivers/md/dm-ioctl.c:1388: warning: 'param' may be used uninitialized in this function
net/sched/sch_cbq.c:409: warning: 'ret' may be used uninitialized in this function
lib/zlib_inflate/inftrees.c:121: warning: 'r.base' may be used uninitialized in this function
On Sat, 2006-08-19 at 02:48 -0400, Andy Gay wrote:
> On Sat, 2006-08-19 at 16:18 +1000, Michael Ellerman wrote:
>
> >
> > If you try to return an uninitialized value the compiler will warn you,
> > you'll then look at the code and realise you missed a case, you might
> > save yourself a bug.
>
> You *should* look at the code :)
>
> So should we be reporting these as bugs?
No you're better off sending patches ;)
A lot of these have started appearing recently, which I think is due to
GCC becoming more vocal. Unfortunately many of them are false positives
caused by GCC not seeming to grok that this is ok:
void foo(int *x) { *x = 1; }
...
int x;
foo(&x);
return x;
It's a pity because it creates noise, but still it's beside the point.
New code going into the kernel should be 100% warning free, and so if
the eHEA guys had missed an error case they'd spot the warning before
they submitted it.
Doing the initialise-to-some-value "trick" means you only spot the bug
via testing.
cheers
--
Michael Ellerman
IBM OzLabs
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
On Saturday 19 August 2006 10:41, Michael Ellerman wrote:
> A lot of these have started appearing recently, which I think is due to
> GCC becoming more vocal. Unfortunately many of them are false positives
> caused by GCC not seeming to grok that this is ok:
>
> void foo(int *x) { *x = 1; }
> ...
> int x;
> foo(&x);
> return x;
>
It's more subtle than this, gcc only gets it wrong when multiple
things come together, the most common one seems to be:
- it tries to inline foo()
- foo has a path where it initializes *x and another one where it
doesn't
- x is accessed after foo() returns, but only when foo indeed has
initialized it.
The problem is that gcc now is more aggressive about inlining
functions. It used to assume that all functions initialize their
pointer arguments, now it does some more checking, but not enough,
so there are lots of false positives. Every gcc-4.x release seems
to fix some of these cases, but a few others remain.
Arnd <><
Andy Gay wrote:
> fs/bio.c:169: warning: 'idx' may be used uninitialized in this function
> fs/eventpoll.c:500: warning: 'fd' may be used uninitialized in this function
> fs/isofs/namei.c:162: warning: 'offset' may be used uninitialized in this function
> fs/isofs/namei.c:162: warning: 'block' may be used uninitialized in this function
> fs/nfsd/nfsctl.c:292: warning: 'maxsize' may be used uninitialized in this function
> fs/udf/balloc.c:751: warning: 'goal_eloc.logicalBlockNum' may be used uninitialized in this function
> fs/udf/super.c:1358: warning: 'ino.partitionReferenceNum' may be used uninitialized in this function
> fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_startblock' may be used uninitialized in this function
> fs/xfs/xfs_alloc_btree.c:611: warning: 'nkey.ar_blockcount' may be used uninitialized in this function
> fs/xfs/xfs_bmap.c:2498: warning: 'rtx' is used uninitialized in this function
> fs/xfs/xfs_bmap_btree.c:753: warning: 'nkey.br_startoff' may be used uninitialized in this function
> fs/xfs/xfs_da_btree.c:151: warning: 'action' may be used uninitialized in this function
> fs/xfs/xfs_dir.c:363: warning: 'totallen' may be used uninitialized in this function
> fs/xfs/xfs_dir.c:363: warning: 'count' may be used uninitialized in this function
> fs/xfs/xfs_ialloc_btree.c:545: warning: 'nkey.ir_startino' may be used uninitialized in this function
> fs/xfs/xfs_inode.c:1958: warning: 'last_dip' may be used uninitialized in this function
> fs/xfs/xfs_inode.c:1960: warning: 'last_offset' may be used uninitialized in this function
> fs/xfs/xfs_log.c:1749: warning: 'iclog' may be used uninitialized in this function
> fs/xfs/xfs_log_recover.c:523: warning: 'first_blk' may be used uninitialized in this function
> ipc/msg.c:338: warning: 'setbuf.qbytes' may be used uninitialized in this function
> ipc/msg.c:338: warning: 'setbuf.uid' may be used uninitialized in this function
> ipc/msg.c:338: warning: 'setbuf.gid' may be used uninitialized in this function
> ipc/msg.c:338: warning: 'setbuf.mode' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.uid' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.gid' may be used uninitialized in this function
> ipc/sem.c:810: warning: 'setbuf.mode' may be used uninitialized in this function
> drivers/md/dm-table.c:431: warning: 'dev' may be used uninitialized in this function
> drivers/md/dm-ioctl.c:1388: warning: 'param' may be used uninitialized in this function
> net/sched/sch_cbq.c:409: warning: 'ret' may be used uninitialized in this function
> lib/zlib_inflate/inftrees.c:121: warning: 'r.base' may be used uninitialized in this function
These are gcc bugs. We don't patch the kernel for gcc bugs.
Jeff
Stephen Hemminger wrote:
> On Fri, 18 Aug 2006 17:41:26 +0200
> Thomas Klein <[email protected]> wrote:
>
>> Hi Alexey,
>>
>> first of all thanks a lot for the extensive review.
>>
>>
>> Alexey Dobriyan wrote:
>>>> + u64 hret = H_HARDWARE;
>>> Useless assignment here and everywhere.
>>>
>> Initializing returncodes to errorstate is a cheap way to prevent
>> accidentally returning (uninitalized) success returncodes which
>> can lead to catastrophic misbehaviour.
>
> That is old thinking. Current compilers do live/dead analysis
> and tell you about this at compile time which is better than relying
> on default behavior at runtime.
Understood. I reworked the returncode handling and removed the
unnecessary initializations.
Thanks for pointing this out.
Thomas
Alexey Dobriyan wrote:
> On Fri, Aug 18, 2006 at 01:33:22PM +0200, Jan-Bernd Themann wrote:
>> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_ethtool.c
>> +++ kernel/drivers/net/ehea/ehea_ethtool.c
>> +{
>> + strncpy(info->driver, DRV_NAME, sizeof(info->driver) - 1);
>> + strncpy(info->version, DRV_VERSION, sizeof(info->version) - 1);
>
> Use strlcpy() to not forget -1 accidently.
Done.
>
>> +static u32 netdev_get_msglevel(struct net_device *dev)
> ^^^^^^^^
>> +{
>> + struct ehea_port *port = netdev_priv(dev);
>> + return port->msg_enable;
> ^^^^^^
>
> Something is mis-named here.
The kernel requires a structure member of that name.
>> +struct ethtool_ops ehea_ethtool_ops = {
>> + .get_settings = netdev_get_settings,
>> + .get_drvinfo = netdev_get_drvinfo,
>> + .get_msglevel = netdev_get_msglevel,
>> + .set_msglevel = netdev_set_msglevel,
>> + .get_link = ethtool_op_get_link,
>> + .get_tx_csum = ethtool_op_get_tx_csum,
>
> Whitespace breakage.
Fixed.
>> + .set_settings = NULL,
>> + .nway_reset = NULL,
>> + .get_pauseparam = NULL,
>> + .set_pauseparam = NULL,
>> + .get_rx_csum = NULL,
>> + .set_rx_csum = NULL,
>> + .phys_id = NULL,
>> + .self_test = NULL,
>> + .self_test_count = NULL
>
> If you don't use them, don't mention them at all. Compiler will DTRT.
Agreed. Assignments removed.
Thanks again :-)
Thomas