netdev - Re: [PATCH 2/10] cxgb3

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: <1166623330.3365.1397.camel@laptopd505.fenrus.org>
Date:	Wed, 20 Dec 2006 15:02:09 +0100
From:	Arjan van de Ven <arjan@...radead.org>
To:	Divy Le Ray <None@...lsio.com>
Cc:	jeff@...zik.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, swise@...ngridcomputing.com
Subject: Re: [PATCH 2/10] cxgb3 - main source file

> +/*
> + * Interrupt handler for asynchronous events used with MSI-X.
> + */
> +static irqreturn_t t3_async_intr_handler(int irq, void *cookie)
> +{
> +	t3_slow_intr_handler(cookie);
> +	return IRQ_HANDLED;
> +}

this looks very wrong; why is t3_slow_intr_handler a void rather than
returning IRQ_HANDLED etc? And why wrap around it ?

> +
> +static ssize_t attr_show(struct class_device *cd, char *buf,
> +			 ssize_t(*format) (struct adapter *, char *))
> +{
> +	ssize_t len;
> +	struct adapter *adap = to_net_dev(cd)->priv;
> +
> +	/* Synchronize with ioctls that may shut down the device */
> +	rtnl_lock();
> +	len = (*format) (adap, buf);
> +	rtnl_unlock();
> +	return len;
> +}

I'm usually kind of nervous with drivers taking the rtnl_lock; to me
that sounds like a layering violation.. why shouldn't your attributes
etc live in the net layer instead?

> +#ifdef ETHTOOL_GPERMADDR
> +	.get_perm_addr = ethtool_op_get_perm_addr
> +#endif

what is this ifdef for?


> +static int cxgb_extension_ioctl(struct net_device *dev, void __user *useraddr)
> +{
> +	int ret;
> +	u32 cmd;
> +	struct adapter *adapter = dev->priv;
> +
> +	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
> +		return -EFAULT;
> +
> +	switch (cmd) {
> +	case CHELSIO_SETREG:{

what are these for ?

> +
> +	/*
> +	 * Can't use pci_request_regions() here because some kernels want to
> +	 * request the MSI-X BAR in pci_enable_msix.

are these "some kernels" actual current mainline kernels?

> +	if (!pci_set_dma_mask(pdev, DMA_64BIT_MASK)) {
> +		pci_using_dac = 1;
> +		err = pci_set_consistent_dma_mask(pdev, DMA_64BIT_MASK);
> +		if (err) {
> +			dev_err(&pdev->dev, "unable to obtain 64-bit DMA for "
> +			       "coherent allocations\n");
> +			goto out_release_regions;

this looks wrong; if you can't get 64 bit coherent allocs but can get 32
bit ones.. why error out ?




-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

-
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