netdev - Re: [PATCH 1/8] New driver "sfc" for Solarstorm SFC4000 controller (try #8)

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: <20080323.223238.86781188.davem@davemloft.net>
Date:	Sun, 23 Mar 2008 22:32:38 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	bhutchings@...arflare.com
Cc:	netdev@...r.kernel.org, linux-net-drivers@...arflare.com,
	jgarzik@...ox.com
Subject: Re: [PATCH 1/8] New driver "sfc" for Solarstorm SFC4000 controller
 (try #8)

From: Ben Hutchings <bhutchings@...arflare.com>
Date: Wed, 12 Mar 2008 01:22:48 +0000

> +static void efx_link_status_changed(struct efx_nic *efx)
> +{
> +	unsigned long flags __attribute__ ((unused));

Please just delete this variable, it really is never
used not matter what the build configuration.

> +
> +		if (rss_cpus == 0) {
> +#ifdef topology_core_siblings
> +			cpumask_t core_mask;
> +			int cpu;
> +
> +			cpus_clear(core_mask);
> +			efx->rss_queues = 0;
> +			for_each_online_cpu(cpu) {
> +				if (!cpu_isset(cpu, core_mask)) {
> +					++efx->rss_queues;
> +					cpus_or(core_mask, core_mask,
> +						topology_core_siblings(cpu));
> +				}
> +			}
> +#else
> +			efx->rss_queues = num_online_cpus();
> +#endif

Please don't test feature availability this way.

Either use the proper CONFIG_* option ifdef.

Or if this is some feature which is not merged yet, remove
this code and add it back once that necessary feature is
added upstream.

> +/* Allocate the NAPI dev's.
> + * Called after we know how many channels there are.
> + */
> +static int efx_init_napi(struct efx_nic *efx)
> +{
> +	struct efx_channel *channel;
> +	int rc;
> +
> +	/* Allocate the NAPI dev for the port */
> +	efx->net_dev = alloc_etherdev(0);
> +	if (!efx->net_dev) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +	efx->net_dev->priv = efx;
> +	efx->mii.dev = efx->net_dev;

Please use alloc_etherdev() how is was designed, by
specifying sizeof(struct efx_nic) as the size argument
and that way your private area gets setup transparently
and none of these explicit assignments are necessary.

> +static int efx_reset(struct efx_nic *efx)
> +{
> +	struct ethtool_cmd ecmd;
> +	unsigned long flags __attribute__ ((unused));
> +	enum reset_type method = efx->reset_pending;
> +	int rc;

Like the other case, remove this truly unused
"flags" thing.

If you are doing this because some macro references
this local variable without an explicit reference in
the code or the calls to it, you need to fix that.

Otherwise the rest of this part looks OK.
--
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