* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: Joe Perches @ 2007-10-23 3:39 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Arnaldo Carvalho de Melo, netdev, David S. Miller
In-Reply-To: <20071022203052.23725719@freepuppy.rosehill>
On Mon, 2007-10-22 at 20:30 -0700, Stephen Hemminger wrote:
> > Corrected fast code is:
> >
> > while (len--) {
> > sum += *data++;
> > sum <<= 1;
> > sum = (((sum & 0x10000) >> 16) + sum) & 0xffff;
> > }
> >
> > At least it is correct on the standalone random data test, and the
> > new code is 30% faster for the cached memory case (13.7 clks/byte vs 18
> > clks/byte).
> Your code looks different...
Both are 16 bit rotate lefts.
Which looks clearer?
^ permalink raw reply
* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: Herbert Xu @ 2007-10-23 3:51 UTC (permalink / raw)
To: Joe Perches; +Cc: davem, acme, netdev
In-Reply-To: <1193104400.5132.45.camel@localhost>
Joe Perches <joe@perches.com> wrote:
> On Mon, 2007-10-22 at 18:43 -0700, David Miller wrote:
>> Ok, but again did you test it?
>
> Nope. Stephen Hemminger did in 2003.
But your code differs significantly from Stephen's version.
However, if it is correct it does look like a good improvement.
So please write a simple test program. It can't that bad since
there are only 65536 values to test :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* RE: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings
From: Li Yang-r58472 @ 2007-10-23 3:38 UTC (permalink / raw)
To: Medve Emilian-EMMEDVE1, David Miller; +Cc: jgarzik, netdev, linuxppc-dev
In-Reply-To: <598D5675D34BE349929AF5EDE9B03E2701685446@az33exm24.fsl.freescale.net>
> -----Original Message-----
> From: Medve Emilian-EMMEDVE1
> Sent: Monday, October 22, 2007 9:48 PM
> To: David Miller
> Cc: jgarzik@pobox.com; Li Yang-r58472;
> netdev@vger.kernel.org; linuxppc-dev@ozlabs.org
> Subject: RE: [PATCH] [POWERPC] ucc_geth: Eliminate compile warnings
>
> Hello David,
>
>
> > No piece of code in the kernel should live in a vacuum.
> >
> > In order to improve overall code quality, every piece of
> driver code
> > should avoid assuming things about pointer sizes and things of this
> > nature.
>
> I'm afraid we might be talking about orthogonal issues here.
> I actively agree that all code (not only kernel) should be
> written up to the coding/quality standards you mention above,
> but I see a difference between fixing a warning and making a
> driver portable (to 64-bit PowerPCs, to other platforms,
> etc.). If there is a kernel todo list somewhere lets add to
> it the task to make the ucc_geth more portable.
>
> > Then the driver can get enabled into the build on every
> platform, and
> > therefore nobody will break the build of this driver again since it
> > will get hit by "allmodconfig"
> > et al. builds even on platforms other than the one it is meant for.
> >
> > This hack fix is not acceptable, really.
>
> Are you suggesting we leave those warnings there until
> somebody decides to fix all the portability issues of this
> driver? My patch is a small and insignificant improvement and
> not the revolution you're asking for, but is an small
> improvement today (I dislike warnings) vs. an improbable big
> one in the future.
I'd say we can not use our way of doing things while working with the
community. The community has to consider the kernel as a whole and thus
has its own virtue. The warning has been there for some time. It stays
as an indicator that we have something to do to improve the portability.
I will work on a patch to fix this portability issue.
- Leo
^ permalink raw reply
* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: Stephen Hemminger @ 2007-10-23 3:30 UTC (permalink / raw)
To: Joe Perches; +Cc: Arnaldo Carvalho de Melo, netdev, David S. Miller
In-Reply-To: <1193081779.5132.24.camel@localhost>
On Mon, 22 Oct 2007 12:36:19 -0700
Joe Perches <joe@perches.com> wrote:
> It's a bit after 2.6.1 now...
>
> Removes unnecessary if, uses 16 bit rotate left.
> Performance improves ~30%
>
> Signed-off-by: Joe Perches <joe@perches.com>
>
> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 7c0b515..1c50f4c 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -925,15 +925,9 @@ static int atrtr_ioctl(unsigned int cmd, void __user *arg)
> static unsigned long atalk_sum_partial(const unsigned char *data,
> int len, unsigned long sum)
> {
> - /* This ought to be unwrapped neatly. I'll trust gcc for now */
> while (len--) {
> - sum += *data;
> - sum <<= 1;
> - if (sum & 0x10000) {
> - sum++;
> - sum &= 0xffff;
> - }
> - data++;
> + sum += *data++;
> + sum = ((sum & 0x8000)>>15) | ((sum & 0x7fff)<<1);
> }
> return sum;
> }
>
The end of the message you quoted was:
> Corrected fast code is:
>
> while (len--) {
> sum += *data++;
> sum <<= 1;
> sum = (((sum & 0x10000) >> 16) + sum) & 0xffff;
> }
>
> At least it is correct on the standalone random data test, and the
> new code is 30% faster for the cached memory case (13.7 clks/byte vs 18
> clks/byte).
Your code looks different...
--
Stephen Hemminger <shemminger@linux-foundation.org>
^ permalink raw reply
* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: Joe Perches @ 2007-10-23 1:36 UTC (permalink / raw)
To: David Miller; +Cc: acme, netdev
In-Reply-To: <20071022.173545.74561992.davem@davemloft.net>
On Mon, 2007-10-22 at 17:35 -0700, David Miller wrote:
> Your code is rotating bit 15 down by one bit and bits 0-14 up by one
> bit.
Yes, a 16 bit rotate left.
There was a discussion a few years ago:
http://oss.sgi.com/archives/netdev/2003-10/msg00734.html
>From the spec:
Implementers of DDP should treat generating the checksum as an optional
feature. The 16-bit DDP checksum is computed as follows:
CkSum := 0 ;
FOR each datagram byte starting with the byte immediately following this
Checksum field
REPEAT the following algorithm:
CkSum := CkSum + byte; (unsigned addition)
Rotate CkSum left one bit, rotating the
most significant bit in least significant bit;
IF, at the end, CkSum = 0 THEN
CkSum := $FFFF (all ones).
Reception of a datagram with CkSum equal to 0 implies that a checksum is
not performed.
^ permalink raw reply
* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: Joe Perches @ 2007-10-23 1:53 UTC (permalink / raw)
To: David Miller; +Cc: acme, netdev
In-Reply-To: <20071022.184340.102771527.davem@davemloft.net>
On Mon, 2007-10-22 at 18:43 -0700, David Miller wrote:
> Ok, but again did you test it?
Nope. Stephen Hemminger did in 2003.
^ permalink raw reply
* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: David Miller @ 2007-10-23 1:43 UTC (permalink / raw)
To: joe; +Cc: acme, netdev
In-Reply-To: <1193103388.5132.38.camel@localhost>
From: Joe Perches <joe@perches.com>
Date: Mon, 22 Oct 2007 18:36:28 -0700
> On Mon, 2007-10-22 at 17:35 -0700, David Miller wrote:
> > Your code is rotating bit 15 down by one bit and bits 0-14 up by one
> > bit.
>
> Yes, a 16 bit rotate left.
>
> There was a discussion a few years ago:
> http://oss.sgi.com/archives/netdev/2003-10/msg00734.html
>
> >From the spec:
...
> Reception of a datagram with CkSum equal to 0 implies that a checksum is
> not performed.
Ok, but again did you test it?
^ permalink raw reply
* Re: [PATCH] wan: new driver retina
From: Krzysztof Halasa @ 2007-10-23 0:41 UTC (permalink / raw)
To: Matti Linnanvuori; +Cc: akpm, jgarzik, netdev
In-Reply-To: <720714.25776.qm@web52011.mail.re2.yahoo.com>
A quick look only:
Matti Linnanvuori <mattilinnanvuori@yahoo.com> writes:
> +++ linux-2.6.24/drivers/net/wan/retina.c
> + CHANGES
> + -------
> +
> + v1.0.0 (JK) - May 27, 2003:
> + * Original driver.
> +
> + v1.1.0 (JK) - June, 2003:
> + * final Flexibilis driver
> +
> + v1.2.0: NO_ARP option back again
> +
> + v1.2.1: (JT) - Aug 21, 2003:
> + * Added support for Retina C5400 card including PROC stuff.
> +
> + v1.2.2: (Petri Ahonen) - Sep 19, 2003:
And so on - I'm not sure such logs belong here.
> +#define DRV_NAME "retina"
> +#define DRV_VERSION "1.2.5"
> +#define DRV_RELDATE "November 14, 2003"
Hmm...
> +/* obsolete
> + see retina_noarp_with_ptp
> +define FEPCI_NO_ARP */
If it's obsolete (or rather unused), just drop it.
> +#undef inb
> +#undef inw
> +#undef inl
> +#undef outb
> +#undef outw
> +#undef outl
> +#define inb nonexistent /* force using only 32bit access */
> +#define inw nonexistent /* force using only 32bit access */
> +#define inl(x) le32_to_cpu(readl(x))
> +#define outb nonexistent /* force using only 32bit access */
> +#define outw nonexistent /* force using only 32bit access */
> +#define outl(value, address) writel(cpu_to_le32(value), address)
Any code like that is write-only, why don't just use readl()/
writel() in the actual code?
Are you sure about this cpu_to_le32? readl()/writel() already
preserve the value.
> +#define VMA_OFFSET(vma) ((vma)->vm_pgoff << PAGE_SHIFT)
Not sure about such things in a driver.
> +enum pci_id_flags_bits {
> + /* Set PCI command register bits before calling probe */
> + PCI_USES_IO = 1, PCI_USES_MEM = 2, PCI_USES_MASTER = 4,
> + /* Read and map the single following PCI BAR */
> + PCI_ADDR0 = 0 << 4, PCI_ADDR1 = 1 << 4, PCI_ADDR2 =
> + 2 << 4, PCI_ADDR3 = 3 << 4,
> + PCI_ADDR_64BITS = 0x100, PCI_NO_ACPI_WAKE =
> + 0x200, PCI_NO_MIN_LATENCY = 0x400,
> + PCI_UNUSED_IRQ = 0x800,
> +};
We already have such things in PCI headers, don't we?
> +/* Linux 2.4 appears to drop POINTOPOINT,BROADCAST and NOARP flags
Linux 2.4?
> +/* proc filesystem functions introduced: */
I'm not sure we're adding new /proc files.
Perhaps you should investigate sysfs and friends?
> + case FEPCI_IOCTL_R_SHARED_MEM:
> + DBG_PRINT(" %s: ioctl read shared mem commanded.\n",
> + fepci_NAME);
> + fepci_copy_to_user(arg, ioaddr + FEPCI_SHARED_MEM_OFFSETT,
> + _IOC_SIZE(cmd), 0);
> + break;
> + case FEPCI_IOCTL_W_SHARED_MEM:
> + DBG_PRINT(" %s: ioctl write shared mem commanded.\n",
> + fepci_NAME);
> + fepci_copy_from_user(ioaddr + FEPCI_SHARED_MEM_OFFSETT,
> + arg, _IOC_SIZE(cmd), 0);
> + break;
> + case FEPCI_IOCTL_G_IDENTIFICATION:
> + DBG_PRINT(" %s: IOCTL_G_IDENTIFICATION commanded.\n",
> + fepci_NAME);
> + fepci_copy_to_user(arg,
> + ioaddr + FEPCI_IDENTIFICATION_OFFSETT,
> + _IOC_SIZE(cmd), 1);
> + break;
> + case FEPCI_IOCTL_G_FEATURES:
> + DBG_PRINT(" %s: IOCTL_G_FEATURES commanded.\n", fepci_NAME);
> + fepci_copy_to_user(arg, ioaddr + FEPCI_FEATURES_OFFSETT,
> + _IOC_SIZE(cmd), 1);
> + break;
Are you sure these ioctls are a good idea? Perhaps sysfs attributes
would be much better?
> + if (length == 0) {
> + fp->rx_packets_of_size_0_stream++;
> + } else if (length == 1) {
> + fp->rx_packets_of_size_1_stream++;
> + } else if (length == 2) {
> + fp->rx_packets_of_size_2_stream++;
> + } else if (length == 3) {
> + fp->rx_packets_of_size_3_stream++;
> + } else if (length < 8) {
> + fp->rx_packets_of_size_4_7_stream++;
> + } else if (length < 16) {
...
I think style details are really a personal thing but this would
look much better without the braces.
> + }
> + temp_tx = (temp_tx + 1) & (TX_RING_SIZE - 1);
> + temp_tx_unit = (temp_tx_unit + 1);
> + temp_tx_unit *= temp_tx_unit < fp->units;
> + }
> +
> + return IRQ_HANDLED;
No unhandled IRQ protection anymore?
> +#ifdef FEPCI_POINT_TO_POINT
> +static int is_ptp_interface(struct net_device *dev)
> +{
> + char **p_ptp_if_name = retina_ptp_interfaces;
> + unsigned int i = interfaces;
> + while (i > 0 && *p_ptp_if_name != NULL) {
> + if (!strncmp(dev->name, *p_ptp_if_name, sizeof(dev->name))) {
> + return 1;
> + } else {
> + }
> + p_ptp_if_name++;
> + i--;
> + }
> + return 0;
> +}
A bit weird, isn't it?
> +static int __devinit fepci_init_one(struct pci_dev *pdev,
> + const struct pci_device_id *ent)
> +{
> + struct net_device *dev = NULL;
> + struct fepci_ch_private *fp = NULL;
> + int chip_idx = ent->driver_data;
> + int drv_flags = pci_id_tbl[chip_idx].drv_flags;
> + int i = pci_enable_device(pdev);
> + u32 j;
> + resource_size_t real_ioaddr;
> + void *ioaddr;
> + unsigned position;
> +
> + if (i) {
> + printk(KERN_WARNING "%s: pci_enable_device returned %x.\n",
> + fepci_NAME, i);
> + return i;
> + }
Didn't spot that pci_enable_device() at first. I think we shouldn't
put state-changing functions in variable declarations, especially
when there are many variables.
> + goto err_2;
> + FOUND:
The labels can be hard to spot, too.
> + /* dev->name[3]= j+0x30; channel number -> ascii */
> + /* minor number -> ascii */
> + dev->name[4] = ((fp->minor * CHANNELS + j) % 10) + 0x30;
> + /* minor number -> ascii */
> + dev->name[3] = ((fp->minor * CHANNELS + j) / 10) + 0x30;
That 0x30 could be written as plain '0'.
> + /* HW_ADDR: 00:rnd:rnd:rnd:rnd:05 */
> + dev->dev_addr[0] = 0;
> + get_random_bytes(&(dev->dev_addr[1]), 4);
> + dev->dev_addr[5] = 5;
I think we already have a function for this.
> +
> +static int fepci_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> + struct fepci_ch_private *fp = dev->priv;
> + const unsigned cur_tx = fp->cur_tx;
> +
> + fp->tx_skbuffs_in++;
> +
> + {
> + u32 tx_length = skb->len;
> + u32 bus_address;
> + struct sk_buff *old;
> + if (unlikely(tx_length < ETH_ZLEN)) {
> + struct sk_buff *bigger =
Why don't you just move the variables to the beginning of this function?
The extra indentation isn't good for readability.
> + if (intr_status & IntrFrameTransmitted) {
> + fp->tx_interrupts_since_last_timer++;
> + if (netif_tx_trylock(dev)) {
> + unsigned i = TX_RING_SIZE - 1;
> + do {
> + u32 desc_b;
> + if ((fp->tx_skbuff[i] != NULL)
> + &&
> + (((desc_b =
> + inl(&fp->tx_desc[i].
> + desc_b)) & transfer_not_done) ==
> + 0)) {
> + /* has been sent */
> + pci_unmap_single(fp->
> + this_card_priv->
> + pci_dev,
> + inl(&fp->
> + tx_desc[i].
> + desc_a),
> + desc_b &
> + frame_length,
> + PCI_DMA_TODEVICE);
The above looks like a hardcopy printout with missing CRs (carriage
returns) :-)
Why not split it?
> + [line]++;
> + /* small packet counters */
> + if (length == 0)
> + fp->rx_packets_of_size_0++;
> + else if (length == 1)
> + fp->rx_packets_of_size_1++;
> + else if (length == 2)
> + fp->rx_packets_of_size_2++;
> + else if (length == 3)
> + fp->rx_packets_of_size_3++;
> + else if (length < 8)
> + fp->rx_packets_of_size_4_7++;
> + else if (length < 16)
> + fp->rx_packets_of_size_8_15++;
> + else if (length < 32)
> + fp->rx_packets_of_size_16_31++;
Is there a specific reason to have such detailed counters?
> +int get_line_data_rate_value(unsigned char line_rate)
> +{
> + switch (line_rate) {
> + case 0x00:
> + return 0;
> + case 0x01:
> + return 1;
> + case 0x05:
> + return 8;
> + case 0x06:
> + return 10;
> + case 0x14:
> + return 256;
> + case 0x15:
> + return 300;
> + case 0x20:
> + return 1;
> + case 0x28:
> + return 8;
> + case 0x29:
> + return 10;
> + case 0x30:
> + return 32;
> + case 0x34:
> + return 56;
> + case 0x36:
> + return 64;
> + case 0x60:
> + return 1;
> + case 0xa0:
> + return 1;
> + default:
> + return -1;
> + }
> +}
> +
> +char get_line_data_rate_unit(unsigned char line_rate)
> +{
> + switch (line_rate) {
> + case 0x00:
> + return 0;
> + case 0x01:
> + return 0;
> + case 0x05:
> + return 0;
> + case 0x06:
> + return 0;
> + case 0x14:
> + return 0;
> + case 0x15:
> + return 0;
> + case 0x20:
> + return 'k';
> + case 0x28:
> + return 'k';
> + case 0x29:
> + return 'k';
> + case 0x30:
> + return 'k';
> + case 0x34:
> + return 'k';
> + case 0x36:
> + return 'k';
> + case 0x60:
> + return 'M';
> + case 0xa0:
> + return 'G';
> + default:
> + return 0;
> + }
> +}
Are you sure you really want this?
> +int print_line_type(unsigned char type, char *buf, int pos)
> +{
> + DBG_PRINT("print_line_type %c\n", type);
> + switch (type) {
> + case 0:
> + pos += sprintf(buf + pos, "NONE");
> + break;
> + case 1:
> + pos += sprintf(buf + pos, "DCombus management bus");
> + break;
> + case 2:
> + pos += sprintf(buf + pos, "V.24");
> + break;
> + case 3:
> + pos += sprintf(buf + pos, "X.21");
> + break;
> + case 4:
> + pos += sprintf(buf + pos, "V.35");
> + break;
> + case 5:
> + pos += sprintf(buf + pos, "V.11");
> + break;
> + case 6:
> + pos += sprintf(buf + pos, "IDSL (ISDN Basic Rate");
> + break;
> + case 7:
> + pos += sprintf(buf + pos, "E1 nonframed/framed");
> + break;
> + case 8:
> + pos += sprintf(buf + pos, "E2 nonframed/framed");
> + break;
> + case 9:
A table would be more readable I think.
> +++ linux-2.6.24/drivers/net/wan/retina.h
> @@ -0,0 +1,164 @@
Short header file included by single .c - is it worth it?
Wow, really long.
--
Krzysztof Halasa
^ permalink raw reply
* Re: [PATCH] - trivial - Improve appletalk checksum calculation
From: David Miller @ 2007-10-23 0:35 UTC (permalink / raw)
To: joe; +Cc: acme, netdev
In-Reply-To: <1193081779.5132.24.camel@localhost>
From: Joe Perches <joe@perches.com>
Date: Mon, 22 Oct 2007 12:36:19 -0700
> It's a bit after 2.6.1 now...
>
> Removes unnecessary if, uses 16 bit rotate left.
> Performance improves ~30%
>
> Signed-off-by: Joe Perches <joe@perches.com>
I'm not sure your transformation is equivalent. Did you
actually test this with appletalk traffic to make sure
the sum is computed correctly? Or more simply, did you
write a test program to send some test data through the
old and new functions?
Here is what I think the problem is:
> static unsigned long atalk_sum_partial(const unsigned char *data,
> int len, unsigned long sum)
> {
> - /* This ought to be unwrapped neatly. I'll trust gcc for now */
> while (len--) {
> - sum += *data;
> - sum <<= 1;
> - if (sum & 0x10000) {
> - sum++;
> - sum &= 0xffff;
> - }
> - data++;
> + sum += *data++;
> + sum = ((sum & 0x8000)>>15) | ((sum & 0x7fff)<<1);
> }
> return sum;
> }
The old code is handling overflow, so that when bit 16 gets set by and
addition, that overflow bit gets cleared and then added back into the
sum.
Your code is rotating bit 15 down by one bit and bits 0-14 up by one
bit, my remedial math knowledge tell me that's likely not the same
thing. :-)
^ permalink raw reply
* Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: Benjamin Herrenschmidt @ 2007-10-23 0:32 UTC (permalink / raw)
To: David Miller; +Cc: linas, netdev, mcarlson, linuxppc-dev, mchan, linux-pci
In-Reply-To: <20071022.172332.112621497.davem@davemloft.net>
On Mon, 2007-10-22 at 17:23 -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Mon, 22 Oct 2007 14:54:52 -0500
>
> > As discussed in the other thread, I'll try to set up a patch
> > for an arch callback for restoring msi state.
>From what it looks like at this stage, pSeries might need to
differenciate restoring MSI state after a device reset (PCI error
recovery) from restoring MSI state after suspend/resume (if we ever
implement that one).
The former apparently require manual saving & restoring of the config
space bits. (Linas, do you have a pointer to the bit of PAPR spec that
specifies that we need to save & restore the MSI message ourselves ?)
For the later (suspend/resume), that will definitely not work, or at
least, will not be enough, especially with something like suspend to
disk, where we'll need to have the firmware reconfigure the MSIs for us
(to make sure, among others, that the interrupt controllers are properly
configured for MSIs etc...).
Ben.
^ permalink raw reply
* Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: Benjamin Herrenschmidt @ 2007-10-23 0:29 UTC (permalink / raw)
To: Linas Vepstas
Cc: Michael Ellerman, netdev, mcarlson, linuxppc-dev list, mchan,
linux-pci, David Miller
In-Reply-To: <20071023001307.GF4280@austin.ibm.com>
> > I don't know why you keep talking about powerpc laptops here ...
>
> Well, there are Apple laptops, right? Aren't those the "powermac"
> platform? Now, I don't know if they support MSI, but if they do,
> I get the impression that they might not restore msi state correctly,
> after being put into hardware suspend. But perhaps I'm mistaken;
> I was simply grepping for various msi-related functions in various
> arch subdirectories, comparing x86 to other arches, and noticed
> that code that would restore msi state seems to be missing for
> most arches and most powerpc platforms.
Ah ok, i see. Well, platforms that use write_msi_msg() shouldn't need
anything special right ? So only pSeries is an issue here....
PowerBooks don't indeed have MSI support, though G5's do and some people
have been toying around with suspend/resume on them (hibernation only at
that stage) but it doesn't matter at this stage. We are specifically
talking about pSeries which is the "special" case here.
Ben.
^ permalink raw reply
* Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: David Miller @ 2007-10-23 0:23 UTC (permalink / raw)
To: linas; +Cc: linuxppc-dev, mchan, mcarlson, netdev, linux-pci, michael
In-Reply-To: <20071022195451.GE4280@austin.ibm.com>
From: linas@austin.ibm.com (Linas Vepstas)
Date: Mon, 22 Oct 2007 14:54:52 -0500
> As discussed in the other thread, I'll try to set up a patch
> for an arch callback for restoring msi state.
Thank you.
^ permalink raw reply
* Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: Linas Vepstas @ 2007-10-23 0:13 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Michael Ellerman, netdev, mcarlson, linuxppc-dev list, mchan,
linux-pci, David Miller
In-Reply-To: <1193088267.6745.108.camel@pasglop>
On Tue, Oct 23, 2007 at 07:24:27AM +1000, Benjamin Herrenschmidt wrote:
>
> On Mon, 2007-10-22 at 13:13 -0500, Linas Vepstas wrote:
> > On Mon, Oct 22, 2007 at 11:49:24AM +1000, Michael Ellerman wrote:
> > >
> > > On pseries there's a chance it will work for PCI error recovery, but if
> > > so it's just lucky that firmware has left everything configured the same
> > > way.
> >
> > ? The papr is quite clear that i is up to the OS to restore the msi
> > state after an eeh error.
>
> Via direct config space access or via firmware change-msi calls ?
Direct config space access. It says that the OS is supposed to read the
MSI config (after its been set up), save it, and restore it, (via direct
config space writes) if the device is ever reset.
> I don't know why you keep talking about powerpc laptops here ...
Well, there are Apple laptops, right? Aren't those the "powermac"
platform? Now, I don't know if they support MSI, but if they do,
I get the impression that they might not restore msi state correctly,
after being put into hardware suspend. But perhaps I'm mistaken;
I was simply grepping for various msi-related functions in various
arch subdirectories, comparing x86 to other arches, and noticed
that code that would restore msi state seems to be missing for
most arches and most powerpc platforms.
--linas
^ permalink raw reply
* [MIPS] MIPSnet: Delete all the useless debugging printks.
From: Ralf Baechle @ 2007-10-22 23:35 UTC (permalink / raw)
To: Andrew Morton, Jeff Garzik; +Cc: netdev
Plus minor formatting fixes.
Signed-off-by: Ralf Baechle <ralf@linux-mips.org>
---
drivers/net/mipsnet.c | 44 ++++----------------------------------------
1 file changed, 4 insertions(+), 40 deletions(-)
diff --git a/drivers/net/mipsnet.c b/drivers/net/mipsnet.c
index 37707a0..aafc3ce 100644
--- a/drivers/net/mipsnet.c
+++ b/drivers/net/mipsnet.c
@@ -30,6 +30,7 @@ static int ioiocpy_frommipsnet(struct net_device *dev, unsigned char *kdata,
int len)
{
uint32_t available_len = inl(mipsnet_reg_address(dev, rxDataCount));
+
if (available_len < len)
return -EFAULT;
@@ -45,14 +46,8 @@ static inline ssize_t mipsnet_put_todevice(struct net_device *dev,
int count_to_go = skb->len;
char *buf_ptr = skb->data;
- pr_debug("%s: %s(): telling MIPSNET txDataCount(%d)\n",
- dev->name, __FUNCTION__, skb->len);
-
outl(skb->len, mipsnet_reg_address(dev, txDataCount));
- pr_debug("%s: %s(): sending data to MIPSNET txDataBuffer(%d)\n",
- dev->name, __FUNCTION__, skb->len);
-
for (; count_to_go; buf_ptr++, count_to_go--)
outb(*buf_ptr, mipsnet_reg_address(dev, txDataBuffer));
@@ -64,10 +59,8 @@ static inline ssize_t mipsnet_put_todevice(struct net_device *dev,
static int mipsnet_xmit(struct sk_buff *skb, struct net_device *dev)
{
- pr_debug("%s:%s(): transmitting %d bytes\n",
- dev->name, __FUNCTION__, skb->len);
-
- /* Only one packet at a time. Once TXDONE interrupt is serviced, the
+ /*
+ * Only one packet at a time. Once TXDONE interrupt is serviced, the
* queue will be restarted.
*/
netif_stop_queue(dev);
@@ -94,8 +87,6 @@ static inline ssize_t mipsnet_get_fromdev(struct net_device *dev, size_t count)
skb->protocol = eth_type_trans(skb, dev);
skb->ip_summed = CHECKSUM_UNNECESSARY;
- pr_debug("%s:%s(): pushing RXed data to kernel\n",
- dev->name, __FUNCTION__);
netif_rx(skb);
dev->stats.rx_packets++;
@@ -112,44 +103,29 @@ static irqreturn_t mipsnet_interrupt(int irq, void *dev_id)
uint64_t interruptFlags;
if (irq == dev->irq) {
- pr_debug("%s:%s(): irq %d for device\n",
- dev->name, __FUNCTION__, irq);
-
retval = IRQ_HANDLED;
interruptFlags =
inl(mipsnet_reg_address(dev, interruptControl));
- pr_debug("%s:%s(): intCtl=0x%016llx\n", dev->name,
- __FUNCTION__, interruptFlags);
if (interruptFlags & MIPSNET_INTCTL_TXDONE) {
- pr_debug("%s:%s(): got TXDone\n",
- dev->name, __FUNCTION__);
outl(MIPSNET_INTCTL_TXDONE,
mipsnet_reg_address(dev, interruptControl));
/* only one packet at a time, we are done. */
netif_wake_queue(dev);
} else if (interruptFlags & MIPSNET_INTCTL_RXDONE) {
- pr_debug("%s:%s(): got RX data\n",
- dev->name, __FUNCTION__);
mipsnet_get_fromdev(dev,
inl(mipsnet_reg_address(dev, rxDataCount)));
- pr_debug("%s:%s(): clearing RX int\n",
- dev->name, __FUNCTION__);
outl(MIPSNET_INTCTL_RXDONE,
mipsnet_reg_address(dev, interruptControl));
} else if (interruptFlags & MIPSNET_INTCTL_TESTBIT) {
- pr_debug("%s:%s(): got test interrupt\n",
- dev->name, __FUNCTION__);
/*
* TESTBIT is cleared on read.
* And takes effect after a write with 0
*/
outl(0, mipsnet_reg_address(dev, interruptControl));
} else {
- pr_debug("%s:%s(): no valid fags 0x%016llx\n",
- dev->name, __FUNCTION__, interruptFlags);
/* Maybe shared IRQ, just ignore, no clearing. */
retval = IRQ_NONE;
}
@@ -165,22 +141,15 @@ static irqreturn_t mipsnet_interrupt(int irq, void *dev_id)
static int mipsnet_open(struct net_device *dev)
{
int err;
- pr_debug("%s: mipsnet_open\n", dev->name);
err = request_irq(dev->irq, &mipsnet_interrupt,
IRQF_SHARED, dev->name, (void *) dev);
if (err) {
- pr_debug("%s: %s(): can't get irq %d\n",
- dev->name, __FUNCTION__, dev->irq);
release_region(dev->base_addr, MIPSNET_IO_EXTENT);
return err;
}
- pr_debug("%s: %s(): got IO region at 0x%04lx and irq %d for dev.\n",
- dev->name, __FUNCTION__, dev->base_addr, dev->irq);
-
-
netif_start_queue(dev);
/* test interrupt handler */
@@ -193,8 +162,8 @@ static int mipsnet_open(struct net_device *dev)
static int mipsnet_close(struct net_device *dev)
{
- pr_debug("%s: %s()\n", dev->name, __FUNCTION__);
netif_stop_queue(dev);
+
return 0;
}
@@ -229,9 +198,6 @@ static int __init mipsnet_probe(struct device *dev)
/* Get the io region now, get irq on open() */
if (!request_region(netdev->base_addr, MIPSNET_IO_EXTENT, "mipsnet")) {
- pr_debug("%s: %s(): IO region {start: 0x%04lux, len: %d} "
- "for dev is not availble.\n", netdev->name,
- __FUNCTION__, netdev->base_addr, MIPSNET_IO_EXTENT);
err = -EBUSY;
goto out_free_netdev;
}
@@ -295,8 +261,6 @@ static int __init mipsnet_init_module(void)
static void __exit mipsnet_exit_module(void)
{
- pr_debug("MIPSNet Ethernet driver exiting\n");
-
driver_unregister(&mipsnet_driver);
}
^ permalink raw reply related
* rfkill causes oops with NULL parent device
From: Jonathan McDowell @ 2007-10-22 22:10 UTC (permalink / raw)
To: netdev
I'm trying to add support for the bluetooth device on Toshiba laptops
using rfkill. The device is controlled only via software and doesn't
exist at all unless enabled, at which point it appears on the USB bus.
However, doing something along the lines of:
toshiba_bluetooth_rfkill = rfkill_allocate(NULL, RFKILL_TYPE_BLUETOOTH);
if (toshiba_bluetooth_rfkill != NULL) {
result = rfkill_register(toshiba_bluetooth_rfkill);
}
causes an oops; the laptop I'm testing on doesn't have serial so it's
hard to capture the backtrace, but it's the call to device_add in
rfkill_register and there's mention of get_device_parent.
This is with 2.6.23. If rfkill *requires* a parent device then I would
have thought it should return an error if passed NULL in _allocate?
J.
--
] http://www.earth.li/~noodles/ [] I am Elmer of Borg, be vewy quiet, [
] PGP/GPG Key @ the.earth.li [] I'm assimilating wabbits. [
] via keyserver, web or email. [] [
] RSA: 4DC4E7FD / DSA: 5B430367 [] [
^ permalink raw reply
* Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: Benjamin Herrenschmidt @ 2007-10-22 21:24 UTC (permalink / raw)
To: Linas Vepstas
Cc: Michael Ellerman, netdev, mcarlson, linuxppc-dev list, mchan,
linux-pci, David Miller
In-Reply-To: <20071022181336.GC4280@austin.ibm.com>
On Mon, 2007-10-22 at 13:13 -0500, Linas Vepstas wrote:
> On Mon, Oct 22, 2007 at 11:49:24AM +1000, Michael Ellerman wrote:
> >
> > On pseries there's a chance it will work for PCI error recovery, but if
> > so it's just lucky that firmware has left everything configured the same
> > way.
>
> ? The papr is quite clear that i is up to the OS to restore the msi
> state after an eeh error.
Via direct config space access or via firmware change-msi calls ?
> > Yes I think so. That way we can properly reconfigure via the firmware
> > interface. The other option would be to design some new arch hook to do
> > resume, but just doing a disable/enable seems simpler to me.
>
> Err, If you read the code for suspend/resume, it never actually calls
> disable/enable (and thus doesn't go to the firmware); it calls
> restore_msi_state() function!
>
> If suspend/resume needs to call firmware to restore the state, then,
> at the moment, suspend/resume is broken. As I mentioned earlier,
> I presumed that no powerpc laptops currently use msi-enabled devices,
> as otherwise, this would have been flushed out.
I don't know why you keep talking about powerpc laptops here ...
Ben.
^ permalink raw reply
* Re: attach additional value to skb
From: Wenhua Zhao @ 2007-10-22 21:18 UTC (permalink / raw)
To: netdev
In-Reply-To: <f36b08ee0710221303y5a846a27v1ebdc919db0f00ac@mail.gmail.com>
On 10/22/07, Yakov Lerner <iler.ml@gmail.com> wrote:
> 1. How can I attach my 32-bit value to the skb in the
> PRE_ROUTNIG hook such that FORWARD and POST_ROUTING
> hooks can later access this value from the skb ?
>
> Can I use sk_buff->cb for that ?
In IP layer cb is used by IP as structure inet_skb_param. It is
written by ip_rcv_options() (in function ip_rcv_finish()) between
PRE_ROUTING and FORWARD/POST_ROUTING.
In my opinion, cb is only used when one exclusively owns the skb.
Regards,
Wenhua
^ permalink raw reply
* Re: kernel panic when running tcpdump
From: Mariusz Kozlowski @ 2007-10-22 21:16 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jeff Garzik, linux-kernel, netdev, linux-ppp
In-Reply-To: <20071022120359.26403bf0.akpm@linux-foundation.org>
> > I'm seeing reproducible oops on 2.6.23-mm1 when trying to run tcpdump
> > over ppp0 interface.
>
> Can you please test the latest Linus kernel from
> ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots/?
Sure.
> Because all netwrking things which were in 2.6.23-mm1 are now in mainline.
> So if mainline is OK then that bug presumably got fixed.
You're right. 2.6.23-git17 runs fine so the bug must have been fixed.
Regards,
Mariusz
^ permalink raw reply
* [PATCH] sky2: crash on remove
From: Stephen Hemminger @ 2007-10-22 20:39 UTC (permalink / raw)
To: Andrew Morton, Jeff Garzik; +Cc: netdev, jkarlson
In-Reply-To: <20071022111235.266a10df.akpm@linux-foundation.org>
Fix off-by one in remove logic that just got introduced.
Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
---
This only occurs in new post 2.6.23 code.
--- a/drivers/net/sky2.c 2007-10-22 09:38:11.000000000 -0700
+++ b/drivers/net/sky2.c 2007-10-22 12:11:22.000000000 -0700
@@ -4271,7 +4271,7 @@ static void __devexit sky2_remove(struct
del_timer_sync(&hw->watchdog_timer);
cancel_work_sync(&hw->restart_work);
- for (i = hw->ports; i >= 0; --i)
+ for (i = hw->ports-1; i >= 0; --i)
unregister_netdev(hw->dev[i]);
sky2_write32(hw, B0_IMSK, 0);
@@ -4289,7 +4289,7 @@ static void __devexit sky2_remove(struct
pci_release_regions(pdev);
pci_disable_device(pdev);
- for (i = hw->ports; i >= 0; --i)
+ for (i = hw->ports-1; i >= 0; --i)
free_netdev(hw->dev[i]);
iounmap(hw->regs);
^ permalink raw reply
* attach additional value to skb
From: Yakov Lerner @ 2007-10-22 20:03 UTC (permalink / raw)
To: netdev
1. How can I attach my 32-bit value to the skb in the
PRE_ROUTNIG hook such that FORWARD and POST_ROUTING
hooks can later access this value from the skb ?
Can I use sk_buff->cb for that ?
2.
I want to add my custom destructor to the skb, the function
pointer to be called at skb destruction time. Will the following
work: if I push address of my_destructor to the
sk-buff->destructor field, and then when my_destructor
is called, I call the (saved) old value of sk-buff->destructor ?
Will this work ?
Yakov
^ permalink raw reply
* Re: [BUG] powerpc does not save msi state [was Re: [PATCH 5/7] pci: Export the pci_restore_msi_state() function
From: Linas Vepstas @ 2007-10-22 19:54 UTC (permalink / raw)
To: David Miller; +Cc: linuxppc-dev, mchan, mcarlson, netdev, linux-pci, michael
In-Reply-To: <20071019.175308.54212640.davem@davemloft.net>
On Fri, Oct 19, 2007 at 05:53:08PM -0700, David Miller wrote:
> From: linas@austin.ibm.com (Linas Vepstas)
> Date: Fri, 19 Oct 2007 19:46:10 -0500
>
> > FWIW, it looks like not all that many arches do this; the output
> > for grep -r address_hi * is pretty thin. Then, looking at
> > i386/kernel/io_apic.c as an example, one can see that the
> > msi state save happens "by accident" if CONFIG_SMP is enabled;
> > and so its surely broekn on uniprocesor machines.
>
> I don't see this, in all cases write_msi_msg() will transfer
> the given "*msg" to entry->msg by this assignment in
> drivers/pci/msi.c:
>
> void write_msi_msg(unsigned int irq, struct msi_msg *msg)
> {
> ...
> entry->msg = *msg;
> }
>
> So as long as write_msi_msg() is invoked, it will be saved
> properly.
As Michael Ellerman points out, the pseries msi setup is done
by firmware, and so this bit never happens.
As discussed in the other thread, I'll try to set up a patch
for an arch callback for restoring msi state.
-linas
^ permalink raw reply
* Re: appletalk bugs
From: Gabriel C @ 2007-10-22 19:45 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Linus Torvalds, Andrew Morton, Linux Kernel Mailing List, netdev
In-Reply-To: <m18x5vasif.fsf@ebiederm.dsl.xmission.com>
Eric W. Biederman wrote:
> Gabriel C <nix.or.die@googlemail.com> writes:
>
>> Hi,
>>
>> modprobing appletalk on current git gives a warning in dmesg :
>>
>> [38506.600269] sysctl table check failed: /net/appletalk .3.7 procname does not
>> match binary path procname
>
> Oops. My apologies it appears I made a mistake when creating my
> table to check up on sysctl values.
>
> This should fix it.
Yes it does, thx.
>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> ---
>
> diff --git a/kernel/sysctl_check.c b/kernel/sysctl_check.c
> index 3c9ef5a..ed6fe51 100644
> --- a/kernel/sysctl_check.c
> +++ b/kernel/sysctl_check.c
> @@ -731,7 +731,7 @@ static struct trans_ctl_table trans_net_table[] = {
> { NET_UNIX, "unix", trans_net_unix_table },
> { NET_IPV4, "ipv4", trans_net_ipv4_table },
> { NET_IPX, "ipx", trans_net_ipx_table },
> - { NET_ATALK, "atalk", trans_net_atalk_table },
> + { NET_ATALK, "appletalk", trans_net_atalk_table },
> { NET_NETROM, "netrom", trans_net_netrom_table },
> { NET_AX25, "ax25", trans_net_ax25_table },
> { NET_BRIDGE, "bridge", trans_net_bridge_table },
>
^ permalink raw reply
* [PATCH] - trivial - Improve appletalk checksum calculation
From: Joe Perches @ 2007-10-22 19:36 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo; +Cc: netdev, David S. Miller
It's a bit after 2.6.1 now...
Removes unnecessary if, uses 16 bit rotate left.
Performance improves ~30%
Signed-off-by: Joe Perches <joe@perches.com>
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 7c0b515..1c50f4c 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -925,15 +925,9 @@ static int atrtr_ioctl(unsigned int cmd, void __user *arg)
static unsigned long atalk_sum_partial(const unsigned char *data,
int len, unsigned long sum)
{
- /* This ought to be unwrapped neatly. I'll trust gcc for now */
while (len--) {
- sum += *data;
- sum <<= 1;
- if (sum & 0x10000) {
- sum++;
- sum &= 0xffff;
- }
- data++;
+ sum += *data++;
+ sum = ((sum & 0x8000)>>15) | ((sum & 0x7fff)<<1);
}
return sum;
}
^ permalink raw reply related
* Re: [PATCH] [net/ipv4]: fib_seq_show function adjustment to get a more sensable output of /proc/net/route
From: Eric Dumazet @ 2007-10-22 19:31 UTC (permalink / raw)
To: Denis Cheng; +Cc: David S. Miller, Jeff Garzik, netdev, linux-kernel
In-Reply-To: <1193077582-4790-1-git-send-email-crquan@gmail.com>
Denis Cheng a écrit :
> the temporary bf[127] char array is redundant, and the specified width 127 make the output of /proc/net/route include many trailing spaces;
> since most terminal's cols are less than 127, this made every fib entry occupy two lines,
>
> after applied this patch, the output of /proc/net/route is more sensable like this:
>
> Iface Destination Gateway Flags RefCnt Use Metric Mask MTU Window IRTT
> eth0 0001A8C0 00000000 0001 0 0 0 00FFFFFF 0 0 0
> lo 0000007F 00000000 0001 0 0 0 000000FF 0 0 0
> eth0 00000000 0101A8C0 0003 0 0 0 00000000 0 0 0
>
> Signed-off-by: Denis Cheng <crquan@gmail.com>
Hum... did you test your patch with many routes declared ? (more than 32 on
i386/x86_64)
127 is not a random value, but chosen as a power of two minus 1.
PAGE_SIZE is garanted to be a multiple of 128 (127 chars + line_feed) on all
arches.
So each read() on /proc/net/route delivers PAGE_SIZE/128 lines.
With your patch, some lines might be truncated (one every 32 on i386)
^ permalink raw reply
* Re: [PATCH] USB: net: Fix asix read transfer buffer allocations.
From: Valentine Barshak @ 2007-10-22 19:22 UTC (permalink / raw)
To: Oliver Neukum; +Cc: netdev, linux-usb-devel
In-Reply-To: <200710221929.04035.oliver@neukum.org>
Oliver Neukum wrote:
> Am Montag 22 Oktober 2007 schrieb Valentine Barshak:
>> static int asix_mdio_read(struct net_device *netdev, int phy_id, int loc)
>> {
>> struct usbnet *dev = netdev_priv(netdev);
>> + void *buf;
>> u16 res;
>>
>> mutex_lock(&dev->phy_mutex);
>> asix_set_sw_mii(dev);
>> +
>> + buf = kmalloc(2, GFP_KERNEL);
>
> This is done under lock. Can you allocate the buffer once and reuse it?
>
> Regards
> Oliver
>
I think we can use 2 bytes of the usbnet data buffer for this.
I'll submit a new patch soon.
Thanks,
Valentine.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox