Date:	Tue, 26 Aug 2008 09:51:42 -0700
From:	Ron Mercer <ron.mercer@...gic.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	jeff@...zik.org, netdev@...r.kernel.org
Subject: Re: [PATCH 2/6] [RFC] qlge: New Driver: Adding main driver file qlge_main.c.

Hi Ben,
Thank you for your input. I guess it's obvious I forgot to run
checkpatch.  I will run it before I repost with the changes you suggest.
I am still working on the receive buffer copy/realignment so those
changes won't be in the next post. There are a few points of discussion
in-line below.
Regards,
Ron Mercer
Qlogic Corporation


On Tue, Aug 26, 2008 at 12:24:21PM +0100, Ben Hutchings wrote:
> Ron Mercer wrote:
> > 
> > Signed-off-by: Ron Mercer <ron.mercer@...gic.com>
> > ---
> >  drivers/net/qlge/qlge_main.c | 4350 ++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 4350 insertions(+), 0 deletions(-)
> >  create mode 100755 drivers/net/qlge/qlge_main.c
> > 
> > diff --git a/drivers/net/qlge/qlge_main.c b/drivers/net/qlge/qlge_main.c
> > new file mode 100755
> > index 0000000..0aa472f
> > --- /dev/null
> > +++ b/drivers/net/qlge/qlge_main.c
> [...]
> > +#include <linux/version.h>
> 
> Should never be used in an in-tree driver.  (Doesn't checkpatch.pl warn
> about this?)
> 
> [...]
> > +#if 0
> > +static int ql_update_ring_coalescing(struct ql_adapter *qdev)
> > +{
> 
> Why are all the ethtool operation implementations disabled?  If they are
> broken in some way, I think they should be left out now and added later.
> 
> [...]
> > +static void ql_update_buffer_queues(struct ql_adapter *qdev,
> > +				    struct rx_ring *rx_ring)
> > +{
> > +	if (rx_ring->sbq_len) ql_update_sbq(qdev, rx_ring);
> > +	if (rx_ring->lbq_len) ql_update_lbq(qdev, rx_ring);
> > +}
> 
> The body of an if-statement always starts on the next line, as checkpatch.pl
> will tell you.
> 
> [...]
> > +static void ql_enable_msix(struct ql_adapter *qdev)
> > +{
> > +	int i;
> > +
> > +	qdev->intr_count = 1;
> > +	/* Get the MSIX vectors. */
> > +	if (irq_type == MSIX_IRQ) {
> > +		/* Try to alloc space for the msix struct,
> > +		 * if it fails then go to MSI/legacy.
> > +		 */
> > +		qdev->msi_x_entry = kcalloc(qdev->rx_ring_count,
> > +					    sizeof(struct msix_entry),
> > +					    GFP_KERNEL);
> > +		if (!qdev->msi_x_entry) {
> > +			irq_type = MSI_IRQ;
> > +			goto msi;
> > +		}
> > +
> > +		for (i = 0; i < qdev->rx_ring_count; i++)
> > +			qdev->msi_x_entry[i].entry = i;
> > +
> > +		if (!pci_enable_msix
> > +		    (qdev->pdev, qdev->msi_x_entry, qdev->rx_ring_count)) {
> > +			set_bit(QL_MSIX_ENABLED, &qdev->flags);
> > +			qdev->intr_count = qdev->rx_ring_count;
> > +			QPRINTK(IFUP, INFO,
> > +				"MSI-X Enabled, got %d vectors.\n",
> > +				qdev->intr_count);
> > +			return;
> > +		} else {
> > +			kfree(qdev->msi_x_entry);
> > +			qdev->msi_x_entry = NULL;
> > +			QPRINTK(IFUP, WARNING,
> > +				"MSI-X Enable failed, trying MSI.\n");
> > +			irq_type = MSI_IRQ;
> 
> If pci_enable_msix() returns a positive number, that's a hint as to how
> many IRQs you should be able to allocate.  It may be worth retrying with
> that number.

I coded it that way originally but it was so messy I pulled it out. I
felt that if a system can't supply the number of interrupt vectors
requested it's just as well to run in MSI single vector mode. I don't
think this is likely to happen. I am open to change this if anyone
insists, but it didn't seem worth the complexity to me.


> 
> [...]
> > +static int ql_adapter_initialize(struct ql_adapter *qdev)
> > +{
> > +	u32 value, mask;
> > +	int i;
> > +	int status = 0;
> > +
> > +	/*
> > +	 * Set up the System register to halt on errors.
> > +	 */
> > +	value = SYS_EFE | SYS_FAE;
> > +	mask = value << 16;
> > +	ql_write32(qdev, SYS, mask | value);
> > +
> > +	/* Set the default queue. */
> > +	value = NIC_RCV_CFG_DFQ;
> > +	mask = NIC_RCV_CFG_DFQ_MASK;
> > +	ql_write32(qdev, NIC_RCV_CFG, (mask | value));
> > +
> > +	/* Set the MPI interrupt to enabled. */
> > +	ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16) | INTR_MASK_PI);
> > +
> > +	/* Enable the function, set pagesize, enable error checking. */
> > +	value = FSC_FE | FSC_EPC_INBOUND | FSC_EPC_OUTBOUND | FSC_EC;
> > +
> > +	if (PAGE_SHIFT == 12)	/* 4k pages */
> > +		value |= FSC_VM_PAGE_4K;
> > +	else if (PAGE_SHIFT == 13)	/* 8k pages */
> > +		value |= FSC_VM_PAGE_8K;
> > +	else if (PAGE_SHIFT == 16)	/* 64k pages */
> > +		value |= FSC_VM_PAGE_64K;
> > +	else {
> > +		QPRINTK(IFUP, ERR, "Invalid page size of %d.\n",
> > +			1 << PAGE_SHIFT);
> 
> You can use
> 	BUILD_BUG_ON(PAGE_SHIFT != 12 && PAGE_SHIFT != 13 && PAGE_SHIFT != 16);
> to detect this problem at compile time.
> 
> Why doesn't this function do any cleanup in case of failure part way
> through?
> 
> [...]

I'm going to get rid of this. What we're trying to do is tell the chip
how to interpret the memory in BAR 3. It doesn't have to match the
actual PAGE_SIZE from the OS as I originally assumed.  I will change it to 4k and leave it at
that.     

> > +static int ql_configure_rings(struct ql_adapter *qdev)
> > +{
> > +	int i;
> > +	struct rx_ring *rx_ring;
> > +	struct tx_ring *tx_ring;
> > +	int cpu_cnt = num_online_cpus();
> > +
> > +	/*
> > +	 * For each processor present we allocate one
> > +	 * rx_ring for outbound completions, and one
> 
> Typo: should be "tx_ring".
> 
> [...]

It should be rx_ring.  This chip uses tx_rings to send packets to the
chip and their completions are added to a separate rx_ring. I've coded
this to use two types for rx_rings: those that handle outbound
completions and those that handle inbound.

> > +static int qlge_change_mtu(struct net_device *ndev, int new_mtu)
> > +{
> > +	struct ql_adapter *qdev = netdev_priv(ndev);
> > +
> > +	if (ndev->mtu == 1500 && new_mtu == 9000) {
> > +		QPRINTK(IFUP, ERR, "Turning on split headers.\n");
> > +		ql_write32(qdev, FSC, (FSC_SH << 16) | FSC_SH);
> > +		if (ql_set_framesize(qdev, JUMBO_FRAME_SIZE))
> > +			return -EIO;
> > +	} else if (ndev->mtu == 9000 && new_mtu == 1500) {
> > +		QPRINTK(IFUP, ERR, "Turning off split headers.\n");
> > +		ql_write32(qdev, FSC, (FSC_SH << 16));
> > +		if (ql_set_framesize(qdev, NORMAL_FRAME_SIZE))
> > +			return -EIO;
> > +	} else if ((ndev->mtu == 1500 && new_mtu == 1500) ||
> > +		   (ndev->mtu == 9000 && new_mtu == 9000)) {
> > +		return 0;
> > +	} else
> > +		return -EINVAL;
> [...]
> 
> This would be a whole lot easier to follow if written as:
> 
> 	if (ndev->mtu == new_mtu)
> 		return 0;
> 	else if (new_mtu == 1500)
> 		...
> 	else if (new_mtu == 9000)
> 		...
> 	else
> 		return -EINVAL;
> 
> But why don't you support MTUs other than 1500 and 9000?
> 
> Ben.
>
Since I have to touch the hardware to change the max frame size I
wanted to verify that a change was actually taking place.  The change
requires grabbing a semaphore that is shared with firmware and it's all
kind of involved.  I thought it would be good to skip that if nothing
was changing.
As far as the ==1500 or  ==9000 I will get back to you later. This was a
requirement passed down for a previous chip at Qlogic, but it may not
apply here.  More on this later...



> -- 
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
--
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

Powered by blists - more mailing lists