* Re: [PATCH] [CAIF-RFC 7/8-v2] CAIF Protocol Stack
From: Stefano Babic @ 2009-10-12 13:43 UTC (permalink / raw)
To: sjur.brandeland
Cc: netdev, kim.xx.lilliestierna, christian.bejram, daniel.martensson
In-Reply-To: <1255095571-6501-8-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com wrote:
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>
>
Hi Sjur,
> +ST-Ericsson modems support a number of transports between modem
> +and host,
> +currently Uart and Shared Memory are available for Linux.
Shared Memory was removed in this patchset.
> +== CAIF structure ==
> +
> +The goal is to have caif as system independent as possible.
> +All caif code can be found under GenCaif/src and GenCaif/inc.
The path are wrong, I think you mean net/caif/generic and include/net/caif.
> +The actual linux module implementation is under src/kernel.
> +There is also a user space program that is not up to date to run the stack in
> +user space for testing.
I think you can remove these phrases.
> +
> +We have tested the kernel implementation on the emulator with a modem and we
> +are able to enumerate and make a link setup.
Only as comment: I tested this patchset again on an ARM platform and I
am able to send successfully AT commands to the Ericsson Test Device "B26".
> + - CFSHML CAIF Shared Memory layer.
Again, this layer was removed and there is no track about SPI Layer.
> +To achieve this we need the help of a daemon program called ldiscd.
> +The benefit is that we can hook up to any TTY, the downside is that we need
> +an extra operation in order to install the line discipline.
I understand the reason, it looks only a quite odd that we need to start
only for this purpose a user space program. And there is no hint if the
daemon is not started, simply caif does not work. What do you think to
set up the line discipline directly in kernel ?
> +Install the line discipline (daemon)
> +$ ldisc
> +
> +Install the VEI channel (this will enumerate and do the linksetup of the first VEI channel. If this goes well you should see /dev/chn*) (There are printks logging all buffers that can be checked with dmesg):
> +$ modprobe chnl_chr
> +
> +The AT (VEI) channel is ready to use (you can now send AT commands on it):
Not really. at this point, the channels are not configured if we do not
use chardevconfig (or we do the same in kernel).
> diff --git a/Documentation/CAIF/caif_user_config.dox b/Documentation/CAIF/caif_user_config.dox
> +The hardware drivers for the physical links to the modems is configured by \b ACTIVATE or \b DEACTIVATE commands.
> +CAIF PHY Drivers and Channel configuration must be done before any CAIF Channels can be accessed from Linux User Space.
> +
> +\section Defined commands
> +The following commands are defined:
> +- \b CREATE - Create a new Net or Character device.
> +- \b DELETE - Delete a Net or Character device
> +- \b ACTIVATE - Activate a CAIF PHY interface.
> +- \b DEACTIVATE - De-activate a CAIF PHY interface.
This file seems obsolete. There is no track about ACTIVATE/DEACTIVATE.
> +
> +\remark
> +Character type devices will show up in /dev/ once they are configured.
If you have udev running on your system....
> +Note also that configured channels are not opened before a file handle is opened from user space.
> +
> +\section Examples
> +The examples below uses the chardevconfig utility as an example on creating channels
> +
> +\subsection Configuration of an AT type Channel:
> +\code
> +$ echo "CREATE TYPE=AT DEV=CHAR NAME=chnlat1" | chrdevconfig /dev/caifconfig
This does not work, PHYPREF is missing.
> +$ echo "CREATE TYPE=RFM DEV=CHAR NAME=rfm01 CONNID=1 VOLUME=/rfm" | chrdevconfig /dev/caifconfig
This does not work, too
> +$ echo "CONFIG-PHY TYPE=MSL NAME=PHYMSL8 INSTANCE=1 CHECKSUM=no HEAD-ALIGN=2 TAIL-ALIGN=2 TRANSFER-ALIGN=16
...and this one does not work.
> +ACTIVATE Activates a new CAIF PHY Instance
> +PHYTYPE=[UART|SPI|MSL|SHM|LOOP] Type of CAIF PHY Instance to configure
> +NAME=<phy-name> Name of the CAIF PHY Device
> +INSTANCE=<id> Instance ID of the device
> +CHECKSUM=[yes|no] Frame Checksum is used for this PHY
> +PARAM=... Other PHY Specific configuration parameters
> +
> +
> +DEACTIVATE NAME=<phy-name> De-activates a PHY Instance
Not implemented ?
> diff --git a/Documentation/CAIF/ldiscd/ldiscd.c b/Documentation/CAIF/ldiscd/ldiscd.c
> +#define CAIF_LDISC_TTY "/dev/ttyS0"
I think it should not be bad to avoid a fixed device name and take it as
program parameter.
Stefano
--
stefano <stefano.babic@babic.homelinux.org>
GPG Key: 0x55814DDE
Fingerprint 4E85 2A66 4CBA 497A 2A7B D3BF 5973 F216 5581 4DDE
^ permalink raw reply
* Re: kernel mode pppoe ppp if + ifb + mirred redirect, ethernet packets in ifb?!
From: jamal @ 2009-10-12 13:10 UTC (permalink / raw)
To: Denys Fedoryschenko; +Cc: netdev
In-Reply-To: <200910121143.39924.denys@visp.net.lb>
On Mon, 2009-10-12 at 11:43 +0300, Denys Fedoryschenko wrote:
> Is it expected that redirecting ppp interface, that supposed to be clean IP
> traffic is becoming eth encapsulated traffic?
No. Imagine if there were other types of packets non-ip for example,
what do you do then?
this feature is as close as you can get when you do switch level
mirroring or redirection. If you want to edit header before redirect
etc, use pedit (refer to recent discussion with someone who wanted to
replicate packets for redundant routing purposes);
cheers,
jamal
^ permalink raw reply
* Re: [PATCH 1/3] iwmc3200top: Add Intel Wireless MultiCom 3200 top driver.
From: John W. Linville @ 2009-10-12 13:17 UTC (permalink / raw)
To: David Miller
Cc: tomasw, netdev, linux-wireless, linux-mmc, yi.zhu,
inaky.perez-gonzalez, cindy.h.kao, guy.cohen, ron.rindjunsky
In-Reply-To: <20091011.033647.66923614.davem@davemloft.net>
On Sun, Oct 11, 2009 at 03:36:47AM -0700, David Miller wrote:
> From: Tomas Winkler <tomasw@gmail.com>
> Date: Sun, 11 Oct 2009 10:05:20 +0200
>
> > Just close my eyes and there is new game to play. :)
> > It's not in the patchwork, so is there any reason you are not planning
> > to add it. The patch intention was for net-next, it looks like
> > I didn't mark it as such, my fault.
>
> Because there still seems to be some confusion between which of these
> bits go through John Linville as a wireless driver and which bits
> go directly through me.
>
> By default I assume John picks up "wireless" drivers and send them
> to me in his wireless merges to me.
>
> If that's not the case, explicitly do a fresh submission of this patch
> and explicitly ask me to merge it.
I was afraid this might get lost in the confusion. I had other things
to worry about and since it crossed so many lines and was sent to
netdev, I figured I'd worry about it later if it got lost... :-(
FWIW, I think the wimax/bluetooth/wifi connection makes net-next-2.6
the right tree for this.
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: [PATCH] [CAIF-RFC 6/8-v2] CAIF Protocol Stack
From: Stefano Babic @ 2009-10-12 13:06 UTC (permalink / raw)
To: sjur.brandeland
Cc: netdev, kim.xx.lilliestierna, christian.bejram, daniel.martensson
In-Reply-To: <1255095571-6501-7-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com wrote:
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>
Hi Sjur,
> +config CAIF_TTY
> + tristate "CAIF TTY transport driver"
> + default n
> + ---help---
> + The CAIF TTY transport driver.
> + If you say yes here you will also need to build a userspace utility to set the line disicpline on the
s/disicpline/discipline/
> diff --git a/drivers/net/caif/phyif_loop.c b/drivers/net/caif/phyif_loop.c
> +/* Start ring buffer */
> +#define RING_MAX_BUFFERS 16384
> +
> +struct ring_buffer_element {
> + struct _cfpkt_t *cfpkt;
> +};
> +
> +static struct {
> + struct ring_buffer_element ring_buffer[RING_MAX_BUFFERS];
> + int head_index;
> + int tail_index;
> +} my_ring_buffer;
> +
> +#define ring_buffer_index_plus_one(index) \
> + ((index+1) < RING_MAX_BUFFERS ? (index + 1) : 0)
> +
> +#define ring_buffer_increment_tail(rb) \
> + ((rb)->tail_index = ring_buffer_index_plus_one((rb)->tail_index))
> +
> +#define ring_buffer_increment_head(rb) \
> + ((rb)->head_index = ring_buffer_index_plus_one((rb)->head_index))
> +
> +#define ring_buffer_empty(rb) ((rb)->head_index == (rb)->tail_index)
> +#define ring_buffer_full(rb) (ring_buffer_index_plus_one((rb)->head_index)\
> + == (rb)->tail_index)
> +#define ring_buffer_tail_element(rb) ((rb)->ring_buffer[(rb)->tail_index])
> +#define ring_buffer_head_element(rb) ((rb)->ring_buffer[(rb)->head_index])
> +#define ring_buffer_size(rb) (((rb)->head_index >= (rb)->tail_index)) ?\
> + ((rb)->head_index - (rb)->tail_index) : \
> + (RING_MAX_BUFFERS - ((rb)->tail_index - (rb)->head_index))
> +/* End ring buffer */
Is there a reason why do you prefer to implement your own ring buffer
management else to use the circ_buf already implemented in kernel ?
> +
> +
> +
> +static void work_func(struct work_struct *work);
> +static struct workqueue_struct *ploop_work_queue;
> +static DECLARE_WORK(loop_work, work_func);
> +static wait_queue_head_t buf_available;
> +
> +
> +#define phyif_assert(assert) BUG_ON(!(assert))
Specialized assert function for this module, really needed ?
> +
> + result = tty_register_ldisc(N_MOUSE, &phyif_ldisc);
I think it is time to set up your own discipline include/linux/tty.h,
inserting a N_CAIF line discipline. Reusing other discipline conflicts
with other drivers.
Stefano
--
stefano <stefano.babic@babic.homelinux.org>
GPG Key: 0x55814DDE
Fingerprint 4E85 2A66 4CBA 497A 2A7B D3BF 5973 F216 5581 4DDE
^ permalink raw reply
* Re: [PATCH] [CAIF-RFC 5/8-v2] CAIF Protocol Stack
From: Stefano Babic @ 2009-10-12 12:51 UTC (permalink / raw)
To: sjur.brandeland
Cc: netdev, kim.xx.lilliestierna, christian.bejram, daniel.martensson
In-Reply-To: <1255095571-6501-6-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com wrote:
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>
Hi Sjur,
> diff --git a/net/caif/caif_chr.c b/net/caif/caif_chr.c
> +#define caif_assert(assert) BUG_ON(!(assert))
Do we need special assert for each module (cfglu_assert,
caif_assert,...) ? They are all defined in the same way.
At this point I have already set a comment about using BUG_ON in a
previous patch.
I see a mixed policy in this patch, using sometimes _assert and
sometimes directly BUG_ON, too.
> diff --git a/net/caif/chnl_chr.c b/net/caif/chnl_chr.c
> + /* Lock in order to try to stop someone from opening the device
> + too early. The misc device has its own lock. We cannot take our
> + lock until misc_register() is finished, because in open() the
> + locks are taken in this order (misc first and then dev).
> + So anyone managing to open the device between the misc_register
> + and the mutex_lock will get a "device not found" error. Don't
> + think it can be avoided.
> + */
> + mutex_lock_interruptible(&dev->mutex);
The return value is not checked and it must be, as in all other cases.
> + /* Find device from name */
> + dev = find_device(-1, name, 0);
> + if (!dev)
> + return -EBADF;
> +
> +
> + mutex_lock_interruptible(&dev->mutex);
The return value of mutex_lock_interruptible() must be checked.
Stefano
--
stefano <stefano.babic@babic.homelinux.org>
GPG Key: 0x55814DDE
Fingerprint 4E85 2A66 4CBA 497A 2A7B D3BF 5973 F216 5581 4DDE
^ permalink raw reply
* Re: [PATCH] Fix IXP 2000 network driver building.
From: Vincent Sanders @ 2009-10-12 12:45 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev
In-Reply-To: <1255349459.2931.7.camel@achroite>
[-- Attachment #1: Type: text/plain, Size: 1005 bytes --]
On Mon, Oct 12, 2009 at 01:10:59PM +0100, Ben Hutchings wrote:
> On Sun, 2009-10-11 at 16:55 +0100, Vincent Sanders wrote:
> > The IXP 2000 network driver was failing to build as it has its own
> > statistics gathering which was not compatible with the recent network
> > device operations changes. This patch fixes the driver in the obvious
> > way and has been compile tested. I have been unable to get the ixp2000
> > maintainer to comment or test this fix.
<snip>
>
> Is there any reason you can't use dev->stats instead of this private
> field?
I have absolutely no idea ;-) my interest in this is fixing the build
faliure and making the target useful again, unfortunately the
maintainer is MIA, I performed the minimal obvious change.
I could tidy this futher, but my version of this hardware will not run
the kernel at all (hardware is old broken dev. board) so I am worried
I may break something with a larger blind change.
--
Vincent Sanders
Simtec Electronics
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply
* Re: 2.6.32-rc4: Reported regressions 2.6.30 -> 2.6.31
From: Frederik Deweerdt @ 2009-10-12 12:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux Kernel Mailing List, Andrew Morton, Linus Torvalds,
Natalie Protasevich, Kernel Testers List, Network Development,
Linux ACPI, Linux PM List, Linux SCSI List, Linux Wireless List,
DRI
In-Reply-To: <56acieJJ2fF.A.nEB.Hzl0KB@chimera>
Hi Rafael,
On Mon, Oct 12, 2009 at 12:41:30AM +0200, Rafael J. Wysocki wrote:
> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=14185
> Subject : Oops in driversbasefirmware_class
> Submitter : <lars_ericsson-zq6IREYz3ykAvxtiuMwx3w@public.gmane.org>
> Date : 2009-09-17 05:09 (25 days old)
> First-Bad-Commit: http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=6e03a201bbe8137487f340d26aa662110e324b20
>
>
> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=14253
> Subject : Oops in driversbasefirmware_class
> Submitter : Lars Ericsson <Lars_Ericsson-zq6IREYz3ykAvxtiuMwx3w@public.gmane.org>
> Date : 2009-09-16 20:44 (26 days old)
> References : http://lkml.org/lkml/2009/9/16/461
> Handled-By : Frederik Deweerdt <frederik.deweerdt-kjvbsxwSFqI@public.gmane.org>
> Patch : http://patchwork.kernel.org/patch/49914/
>
Those two are refering to the same bug.
Regards,
Frederik
^ permalink raw reply
* Re: [PATCH] [CAIF-RFC 4/8-v2] CAIF Protocol Stack
From: Stefano Babic @ 2009-10-12 12:20 UTC (permalink / raw)
To: sjur.brandeland
Cc: netdev, kim.xx.lilliestierna, christian.bejram, daniel.martensson
In-Reply-To: <1255095571-6501-5-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com wrote:
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>
Hi Sjur,
>
> diff --git a/net/caif/generic/cfcnfg.c b/net/caif/generic/cfcnfg.c
> new file mode 100644
> index 0000000..3aad201
> + * NOTE: What happends destroy failure:
s/happends/happens/, there are other occurencies.
> +
> +bool cfcnfg_del_adapt_layer(struct _cfcnfg_t *cnfg, layer_t *adap_layer)
> +{
> + uint8 channel_id = 0;
> + struct cfcnfg_phyinfo *phyinfo = NULL;
> + uint8 phyid = 0;
> + CFLOG_TRACE(("cfcnfg: enter del_adaptation_layer\n"));
> +
> + cfglu_assert(adap_layer != NULL);
> + channel_id = adap_layer->id;
> + cfglu_assert(channel_id != 0);
My two cents about using assert in the code, but I prefer to get some
info from system when something goes wrong as to call in some way panic
(the assert calls BUG_ON) and blocks forever.
This line is not different as checking the adapt_layer->dn pointer some
lines after and I think an error is better recognized in that case. So
IMHO should be better something like:
if (channel_id == 0) {
CFLOG_ERROR(("cfcnfg:adap_layer->dn is NULL\n"));
return CFGLU_EINVAL;
}
> diff --git a/net/caif/generic/cfpkt_plain.c b/net/caif/generic/cfpkt_plain.c
> +#define CHECK_MEM 1
Probably you plan to use always the magic number in your buffer
management. Then should be better to remove all the #if CHECK_MEM stuff.
> +#if CHECK_MEM
> +
> +#endif
This three lines seem not needed.... ;)
Stefano
--
stefano <stefano.babic@babic.homelinux.org>
GPG Key: 0x55814DDE
Fingerprint 4E85 2A66 4CBA 497A 2A7B D3BF 5973 F216 5581 4DDE
^ permalink raw reply
* Re: [PATCH net-next-2.6] ixgbe: Use the instance of net_device_stats from net_device.
From: Ajit Khaparde @ 2009-10-12 12:10 UTC (permalink / raw)
To: Tantilov, Emil S
Cc: davem@davemloft.net, netdev@vger.kernel.org, Kirsher, Jeffrey T
In-Reply-To: <EA929A9653AAE14F841771FB1DE5A1365F98A89A0B@rrsmsx501.amr.corp.intel.com>
On 08/10/09 15:55 -0600, Tantilov, Emil S wrote:
> Ajit Khaparde wrote:
> > Since net_device has an instance of net_device_stats,
> > we can remove the instance of this from the private adapter structure.
> >
> > Signed-off-by: Ajit Khaparde <ajitk@serverengines.com>
>
> I am seeing an issues with these patches where some counters are messed up:
>
Thanks for testing the changes.
There was an error in the way I was trying to offset for the netdev stats.
I have a fix for it now. I would like to know if you would prefer an
incremental patch or a patch with the previous changes too.
I will mail the patches accordingly. Sorry for the delay in sending this out.
-Ajit
> --- ixgbe ---
>
> NIC statistics:
> rx_packets: 18446612147046614784
> tx_packets: 18446612147046614784
> rx_bytes: 18446744072099799523
> tx_bytes: 18446744072099857640
> rx_pkts_nic: 12
> tx_pkts_nic: 19
> rx_bytes_nic: 1148
> tx_bytes_nic: 1762
> lsc_int: 4
> tx_busy: 0
> non_eop_descs: 0
> rx_errors: 0
> tx_errors: 18446744072099850983
> rx_dropped: 18446612140591825160
> tx_dropped: 18446612140591825424
> multicast: 18446612140591825952
> broadcast: 0
> rx_no_buffer_count: 0
> collisions: 18446612140591825688
> rx_over_errors: 18446612140591827008
> rx_crc_errors: 18446612140591827272
> rx_frame_errors: 18446612140591827536
> hw_rsc_count: 0
> fdir_match: 0
> fdir_miss: 10
> rx_fifo_errors: 18446612140591827800
> rx_missed_errors: 18446612140591828064
> tx_aborted_errors: 18446612140591828328
> tx_carrier_errors: 18446612140591828592
> tx_fifo_errors: 18446612140591828856
> tx_heartbeat_errors: 18446612140591829120
> ...
>
> --- igb ---
> NIC statistics:
> rx_packets: 0
> tx_packets: 0
> rx_bytes: 0
> tx_bytes: 0
> rx_broadcast: 0
> tx_broadcast: 0
> rx_multicast: 0
> tx_multicast: 0
> rx_errors: 0
> tx_errors: 0
> tx_dropped: 18446744072099036908
> multicast: 0
> collisions: 0
> rx_length_errors: 0
> rx_over_errors: 65535
> rx_crc_errors: 0
> rx_frame_errors: 0
> rx_no_buffer_count: 0
> rx_queue_drop_packet_count: 0
> rx_missed_errors: 0
> tx_aborted_errors: 0
> tx_carrier_errors: 0
> tx_fifo_errors: 0
> tx_heartbeat_errors: 18446612146842011544
> ...
>
> --- e1000 ---
> NIC statistics:
> rx_packets: 0
> tx_packets: 0
> rx_bytes: 0
> tx_bytes: 0
> rx_broadcast: 0
> tx_broadcast: 0
> rx_multicast: 0
> tx_multicast: 0
> rx_errors: 0
> tx_errors: 0
> tx_dropped: 18446744072099165544
> multicast: 0
> collisions: 0
> rx_length_errors: 0
> rx_over_errors: 0
> rx_crc_errors: 0
> rx_frame_errors: 18446744072099094496
> rx_no_buffer_count: 0
> rx_missed_errors: 0
> tx_aborted_errors: 0
> tx_carrier_errors: 0
> tx_fifo_errors: 0
> tx_heartbeat_errors: 4294967295
> ...
>
> --- e1000e ---
> NIC statistics:
> rx_packets: 0
> tx_packets: 7
> rx_bytes: 0
> tx_bytes: 614
> rx_broadcast: 0
> tx_broadcast: 0
> rx_multicast: 0
> tx_multicast: 7
> rx_errors: 0
> tx_errors: 0
> tx_dropped: 18446744072099315636
>
> Thanks,
> Emil--
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Fix IXP 2000 network driver building.
From: Ben Hutchings @ 2009-10-12 12:10 UTC (permalink / raw)
To: Vincent Sanders; +Cc: netdev
In-Reply-To: <1255276546-31903-1-git-send-email-vince@simtec.co.uk>
On Sun, 2009-10-11 at 16:55 +0100, Vincent Sanders wrote:
> The IXP 2000 network driver was failing to build as it has its own
> statistics gathering which was not compatible with the recent network
> device operations changes. This patch fixes the driver in the obvious
> way and has been compile tested. I have been unable to get the ixp2000
> maintainer to comment or test this fix.
[...]
> diff --git a/drivers/net/ixp2000/ixpdev.c
> b/drivers/net/ixp2000/ixpdev.c
> index 1272434..79cb6fa 100644
> --- a/drivers/net/ixp2000/ixpdev.c
> +++ b/drivers/net/ixp2000/ixpdev.c
> @@ -21,6 +21,7 @@
> #include "ixp2400_tx.ucode"
> #include "ixpdev_priv.h"
> #include "ixpdev.h"
> +#include "pm3386.h"
>
> #define DRV_MODULE_VERSION "0.2"
>
> @@ -271,6 +272,15 @@ static int ixpdev_close(struct net_device *dev)
> return 0;
> }
>
> +static struct net_device_stats *ixpdev_get_stats(struct net_device
> *dev)
> +{
> + struct ixpdev_priv *ip = netdev_priv(dev);
> +
> + pm3386_get_stats(ip->channel, &(ip->stats));
> +
> + return &(ip->stats);
> +}
> +
[...]
Is there any reason you can't use dev->stats instead of this private
field?
Ben.
--
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.
^ permalink raw reply
* Re: [Bug #14252] WARNING: at include/linux/skbuff.h:1382 w/ e1000
From: Stephan von Krawczynski @ 2009-10-12 11:44 UTC (permalink / raw)
To: David Miller
Cc: rjw, linux-kernel, kernel-testers, netdev, jeffrey.t.kirsher,
jesse.brandeburg, peter.p.waskiewicz.jr
In-Reply-To: <20091012.034919.98011643.davem@davemloft.net>
On Mon, 12 Oct 2009 03:49:19 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: "Rafael J. Wysocki" <rjw@sisk.pl>
> Date: Mon, 12 Oct 2009 01:01:06 +0200 (CEST)
>
> > This message has been generated automatically as a part of a report
> > of regressions introduced between 2.6.30 and 2.6.31.
> >
> > The following bug entry is on the current list of known regressions
> > introduced between 2.6.30 and 2.6.31. Please verify if it still should
> > be listed and let me know (either way).
> >
> >
> > Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=14252
> > Subject : WARNING: at include/linux/skbuff.h:1382 w/ e1000
> > Submitter : Stephan von Krawczynski <skraw@ithnet.com>
> > Date : 2009-09-20 11:26 (22 days old)
> > References : http://marc.info/?l=linux-kernel&m=125344599006033&w=4
>
> Hmmm... e1000 calls skb_trim() on both jumbo and non-jumbo ring
> buffers which get recycled.
>
> At least for the Jumbo case, that's illegal as you cannot call
> skb_trim() on an SKB with paged data.
>
> But this assertion is triggering for the non-jumbo ring where
> only linear packets should be present as far as I can tell.
>
> Some Intel folks need to take a look, CC:'d, and people need
> to CC: their networking bug reports to netdev@vger.kernel.org
> so that the proper folks see it.
>
> Thanks.
Really, this was a lucky catch, because most of the time the box goes dead right away.
Don't interpret "most of the time" as "continously every day". It just happens sometimes. I am not that surprised because it's a box on the "frontline", you can find a lot of trash going on there, like:
Oct 12 12:21:01 box kernel: TCP: Peer 217.231.204.133:61124/80 unexpectedly shrunk window 2348821413:2348838837 (repaired)
Oct 12 12:21:02 box kernel: TCP: Peer 217.231.204.133:61124/80 unexpectedly shrunk window 2348821413:2348838837 (repaired)
--
Regards,
Stephan
^ permalink raw reply
* [PATCH] Fix IXP 2000 network driver building.
From: Vincent Sanders @ 2009-10-11 15:55 UTC (permalink / raw)
To: netdev; +Cc: Vincent Sanders
The IXP 2000 network driver was failing to build as it has its own
statistics gathering which was not compatible with the recent network
device operations changes. This patch fixes the driver in the obvious
way and has been compile tested. I have been unable to get the ixp2000
maintainer to comment or test this fix.
Signed-off-by: Vincent Sanders <vince@simtec.co.uk>
---
drivers/net/ixp2000/enp2611.c | 18 +-----------------
drivers/net/ixp2000/ixpdev.c | 11 +++++++++++
drivers/net/ixp2000/ixpdev.h | 1 +
3 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ixp2000/enp2611.c b/drivers/net/ixp2000/enp2611.c
index b02a981..34a6cfd 100644
--- a/drivers/net/ixp2000/enp2611.c
+++ b/drivers/net/ixp2000/enp2611.c
@@ -119,24 +119,9 @@ static struct ixp2400_msf_parameters enp2611_msf_parameters =
}
};
-struct enp2611_ixpdev_priv
-{
- struct ixpdev_priv ixpdev_priv;
- struct net_device_stats stats;
-};
-
static struct net_device *nds[3];
static struct timer_list link_check_timer;
-static struct net_device_stats *enp2611_get_stats(struct net_device *dev)
-{
- struct enp2611_ixpdev_priv *ip = netdev_priv(dev);
-
- pm3386_get_stats(ip->ixpdev_priv.channel, &(ip->stats));
-
- return &(ip->stats);
-}
-
/* @@@ Poll the SFP moddef0 line too. */
/* @@@ Try to use the pm3386 DOOL interrupt as well. */
static void enp2611_check_link_status(unsigned long __dummy)
@@ -203,14 +188,13 @@ static int __init enp2611_init_module(void)
ports = pm3386_port_count();
for (i = 0; i < ports; i++) {
- nds[i] = ixpdev_alloc(i, sizeof(struct enp2611_ixpdev_priv));
+ nds[i] = ixpdev_alloc(i, sizeof(struct ixpdev_priv));
if (nds[i] == NULL) {
while (--i >= 0)
free_netdev(nds[i]);
return -ENOMEM;
}
- nds[i]->get_stats = enp2611_get_stats;
pm3386_init_port(i);
pm3386_get_mac(i, nds[i]->dev_addr);
}
diff --git a/drivers/net/ixp2000/ixpdev.c b/drivers/net/ixp2000/ixpdev.c
index 1272434..79cb6fa 100644
--- a/drivers/net/ixp2000/ixpdev.c
+++ b/drivers/net/ixp2000/ixpdev.c
@@ -21,6 +21,7 @@
#include "ixp2400_tx.ucode"
#include "ixpdev_priv.h"
#include "ixpdev.h"
+#include "pm3386.h"
#define DRV_MODULE_VERSION "0.2"
@@ -271,6 +272,15 @@ static int ixpdev_close(struct net_device *dev)
return 0;
}
+static struct net_device_stats *ixpdev_get_stats(struct net_device *dev)
+{
+ struct ixpdev_priv *ip = netdev_priv(dev);
+
+ pm3386_get_stats(ip->channel, &(ip->stats));
+
+ return &(ip->stats);
+}
+
static const struct net_device_ops ixpdev_netdev_ops = {
.ndo_open = ixpdev_open,
.ndo_stop = ixpdev_close,
@@ -278,6 +288,7 @@ static const struct net_device_ops ixpdev_netdev_ops = {
.ndo_change_mtu = eth_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = eth_mac_addr,
+ .ndo_get_stats = ixpdev_get_stats,
#ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller = ixpdev_poll_controller,
#endif
diff --git a/drivers/net/ixp2000/ixpdev.h b/drivers/net/ixp2000/ixpdev.h
index 391ece6..bf235e9 100644
--- a/drivers/net/ixp2000/ixpdev.h
+++ b/drivers/net/ixp2000/ixpdev.h
@@ -18,6 +18,7 @@ struct ixpdev_priv
struct napi_struct napi;
int channel;
int tx_queue_entries;
+ struct net_device_stats stats;
};
struct net_device *ixpdev_alloc(int channel, int sizeof_priv);
--
1.6.0.4
^ permalink raw reply related
* Re: PATCH: Network Device Naming mechanism and policy
From: Ben Hutchings @ 2009-10-12 11:31 UTC (permalink / raw)
To: Scott James Remnant
Cc: Matt Domsch, Narendra K, netdev, linux-hotplug, jordan_hargrave
In-Reply-To: <1255344075.2143.1.camel@warcraft>
On Mon, 2009-10-12 at 11:41 +0100, Scott James Remnant wrote:
> On Fri, 2009-10-09 at 09:51 -0500, Matt Domsch wrote:
>
> > As has been noted here, MAC addresses are not necessarily unique to an
> > interface. As such, we are not proposing a net/by-mac/* symlink to
> > /dev/netdev/*.
> >
> On the other hand, they *tend* to be unique for a wide range of systems.
> This makes them pretty comparable to LABELs on disks, and we have
> a /dev/disk/by-label
[...]
MAC addresses are normally assigned automatically but can be overridden
if necessary. In that respect they are more like UUIDs for disks.
I don't see any analogue of disk labels, though labels could conceivably
be added to some NICs using VPD.
Ben.
--
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.
^ permalink raw reply
* Re: [PATCH] pasemi_mac: ethtool set settings support
From: David Miller @ 2009-10-12 11:25 UTC (permalink / raw)
To: olof; +Cc: linuxppc-dev, jgarzik, netdev
In-Reply-To: <20091006161123.GC29195@lixom.net>
From: Olof Johansson <olof@lixom.net>
Date: Tue, 6 Oct 2009 11:11:23 -0500
> On Mon, Oct 05, 2009 at 05:31:24PM +0400, Valentine Barshak wrote:
>> Add ethtool set settings to pasemi_mac_ethtool.
>>
>> Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
>
> Acked-by: Olof Johansson <olof@lixom.net>
Applied to net-next-2.6, thanks.
^ permalink raw reply
* Re: NOHZ: local_softirq_pending 08
From: Tilman Schmidt @ 2009-10-12 11:25 UTC (permalink / raw)
To: David Miller
Cc: tilman, johannes, hidave.darkstar, linux-kernel, tglx,
linux-wireless, linux-ppp, netdev, paulus
In-Reply-To: <20091012.033246.56701176.davem@davemloft.net>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Mon, 12 Oct 2009 03:32:46 -0700 (PDT), David Miller wrote:
> The PPP receive paths in ppp_generic.c do a local_bh_disable()/
> local_bh_enable() around packet receiving (via ppp_recv_lock()/
> ppp_recv_unlock() in ppp_do_recv).
>
> So at least that part is perfectly fine.
>
> ppp_input(), as called from ppp_sync_process(), also disables BH's
> around ppp_do_recv() calls (via read_lock_bh()/read_unlock_bh()).
>
> So that's fine too.
>
> Do you have a bug report or are you just scanning around looking
> for trouble? :-)
I have encountered the message in the subject during a test of
the Gigaset CAPI driver, and would like to determine whether
it's a bug in the driver, a bug somewhere else, or no bug at
all. The test scenario was PPP over ISDN with pppd+capiplugin.
In an alternative scenario, also PPP over ISDN but with
smpppd+capidrv, the message did not occur.
Johannes' answer pointed me to the netif_rx() function.
The Gigaset driver itself doesn't call that function at all.
In the scenario where I saw the message, it was the SYNC_PPP
line discipline that did. But from your explanation I gather
that the cause cannot lie there.
So now I'm looking for other possible causes of that message.
- --
Tilman Schmidt E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.4 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFK0xITQ3+did9BuFsRAmXGAKCIiqJffUnuKw9rPjxHwbj9AnXOigCdGgS9
MpxRNGs0A4GTydYJaylbjyo=
=LFxi
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [net-next-2.6 PATCH] be2net:patch to implement ethtool get_phys_id function.
From: David Miller @ 2009-10-12 11:24 UTC (permalink / raw)
To: sarveshwarb; +Cc: netdev
In-Reply-To: <20091009131306.GA32311@serverengines.com>
From: Sarveshwar Bandi <sarveshwarb@serverengines.com>
Date: Fri, 9 Oct 2009 18:43:16 +0530
> please accept patch, implements port beacon functionality for be2net driver.
>
> Signed-off-by: sarveshwarb <sarveshwarb@serverengines.com>
Applied, but please use your real name as it appears in your
From: email field in signoffs. Like this:
Signed-off-by: Sarveshwar Bandi <sarveshwarb@serverengines.com>
I fixed that up when applying this.
Thanks.
^ permalink raw reply
* Re: [PATCH] acenic: Pass up error code from ace_load_firmware()
From: David Miller @ 2009-10-12 11:19 UTC (permalink / raw)
To: ben; +Cc: jes, linux-acenic, netdev
In-Reply-To: <1255311286.2488.11.camel@localhost>
From: Ben Hutchings <ben@decadent.org.uk>
Date: Mon, 12 Oct 2009 02:34:46 +0100
> If ace_load_firmware() fails, ace_init() cleans up but still returns
> 0, leading to an oops as seen in <http://bugs.debian.org/521383>.
> It should pass the error code up.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Applied, and I'll queue this up for -stable, thanks!
^ permalink raw reply
* Re: [Bug #14265] ifconfig: page allocation failure. order:5, mode:0x8020 w/ e100
From: David Miller @ 2009-10-12 11:05 UTC (permalink / raw)
To: rjw; +Cc: linux-kernel, kernel-testers, karol.k.lewandowski, mel, netdev
In-Reply-To: <L6tb-VyKHZK.A.YoC.lzl0KB@chimera>
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Mon, 12 Oct 2009 01:01:08 +0200 (CEST)
[ Netdev CC:'d ]
> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=14265
> Subject : ifconfig: page allocation failure. order:5, mode:0x8020 w/ e100
> Submitter : Karol Lewandowski <karol.k.lewandowski@gmail.com>
> Date : 2009-09-15 12:05 (27 days old)
> References : http://marc.info/?l=linux-kernel&m=125301636509517&w=4
A 128K memory allocation fails after resume, film at 11.
That e100 driver code has been that way forever, so likely it's
something in the page allocator or similar that is making this happen
more likely now. Perhaps it's related to the iwlagn allocation
failures being tracked down in another thread.
It's a shame that pci_alloc_consistent() has to always use GFP_ATOMIC
for compatability.
As far as I can tell, these code paths can sleep. So maybe the
following hack would fix this for now. Could someone test this?
diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index 679965c..c71729f 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -1780,9 +1780,9 @@ static void e100_clean_cbs(struct nic *nic)
nic->cb_to_clean = nic->cb_to_clean->next;
nic->cbs_avail++;
}
- pci_free_consistent(nic->pdev,
- sizeof(struct cb) * nic->params.cbs.count,
- nic->cbs, nic->cbs_dma_addr);
+ dma_free_coherent(&nic->pdev->dev,
+ sizeof(struct cb) * nic->params.cbs.count,
+ nic->cbs, nic->cbs_dma_addr);
nic->cbs = NULL;
nic->cbs_avail = 0;
}
@@ -1800,8 +1800,10 @@ static int e100_alloc_cbs(struct nic *nic)
nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = NULL;
nic->cbs_avail = 0;
- nic->cbs = pci_alloc_consistent(nic->pdev,
- sizeof(struct cb) * count, &nic->cbs_dma_addr);
+ nic->cbs = dma_alloc_coherent(&nic->pdev->dev,
+ sizeof(struct cb) * count,
+ &nic->cbs_dma_addr,
+ GFP_KERNEL);
if (!nic->cbs)
return -ENOMEM;
@@ -2655,16 +2657,16 @@ static int e100_do_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
static int e100_alloc(struct nic *nic)
{
- nic->mem = pci_alloc_consistent(nic->pdev, sizeof(struct mem),
- &nic->dma_addr);
+ nic->mem = dma_alloc_coherent(&nic->pdev->dev, sizeof(struct mem),
+ &nic->dma_addr, GFP_KERNEL);
return nic->mem ? 0 : -ENOMEM;
}
static void e100_free(struct nic *nic)
{
if (nic->mem) {
- pci_free_consistent(nic->pdev, sizeof(struct mem),
- nic->mem, nic->dma_addr);
+ dma_free_coherent(&nic->pdev->dev, sizeof(struct mem),
+ nic->mem, nic->dma_addr);
nic->mem = NULL;
}
}
^ permalink raw reply related
* Re: [Bug #14252] WARNING: at include/linux/skbuff.h:1382 w/ e1000
From: David Miller @ 2009-10-12 10:49 UTC (permalink / raw)
To: rjw
Cc: linux-kernel, kernel-testers, skraw, netdev, jeffrey.t.kirsher,
jesse.brandeburg, peter.p.waskiewicz.jr
In-Reply-To: <L6tb-VyKHZK.A.9UD.zzl0KB@chimera>
From: "Rafael J. Wysocki" <rjw@sisk.pl>
Date: Mon, 12 Oct 2009 01:01:06 +0200 (CEST)
> This message has been generated automatically as a part of a report
> of regressions introduced between 2.6.30 and 2.6.31.
>
> The following bug entry is on the current list of known regressions
> introduced between 2.6.30 and 2.6.31. Please verify if it still should
> be listed and let me know (either way).
>
>
> Bug-Entry : http://bugzilla.kernel.org/show_bug.cgi?id=14252
> Subject : WARNING: at include/linux/skbuff.h:1382 w/ e1000
> Submitter : Stephan von Krawczynski <skraw@ithnet.com>
> Date : 2009-09-20 11:26 (22 days old)
> References : http://marc.info/?l=linux-kernel&m=125344599006033&w=4
Hmmm... e1000 calls skb_trim() on both jumbo and non-jumbo ring
buffers which get recycled.
At least for the Jumbo case, that's illegal as you cannot call
skb_trim() on an SKB with paged data.
But this assertion is triggering for the non-jumbo ring where
only linear packets should be present as far as I can tell.
Some Intel folks need to take a look, CC:'d, and people need
to CC: their networking bug reports to netdev@vger.kernel.org
so that the proper folks see it.
Thanks.
^ permalink raw reply
* Re: PATCH: Network Device Naming mechanism and policy
From: Scott James Remnant @ 2009-10-12 10:41 UTC (permalink / raw)
To: Matt Domsch; +Cc: Narendra K, netdev, linux-hotplug, jordan_hargrave
In-Reply-To: <20091009145137.GD19218@mock.linuxdev.us.dell.com>
[-- Attachment #1: Type: text/plain, Size: 693 bytes --]
On Fri, 2009-10-09 at 09:51 -0500, Matt Domsch wrote:
> As has been noted here, MAC addresses are not necessarily unique to an
> interface. As such, we are not proposing a net/by-mac/* symlink to
> /dev/netdev/*.
>
On the other hand, they *tend* to be unique for a wide range of systems.
This makes them pretty comparable to LABELs on disks, and we have
a /dev/disk/by-label
Remember that udev already supports symlink stacking, and priorities and
such.
I don't think there's any danger of supporting a /dev/netdev/by-mac by
default, it'll be a benefit to most and those who don't have unique MACs
will just ignore it.
Scott
--
Scott James Remnant
scott@ubuntu.com
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: NOHZ: local_softirq_pending 08
From: David Miller @ 2009-10-12 10:32 UTC (permalink / raw)
To: tilman
Cc: johannes, hidave.darkstar, linux-kernel, tglx, linux-wireless,
linux-ppp, netdev, paulus
In-Reply-To: <4AD2E8C8.4060205@imap.cc>
From: Tilman Schmidt <tilman@imap.cc>
Date: Mon, 12 Oct 2009 10:28:56 +0200
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> [CCing PPP people]
>
> Am 11.10.2009 13:40 schrieb Johannes Berg:
>> On Sun, 2009-10-11 at 13:18 +0200, Tilman Schmidt wrote:
>>
>>> Can you explain a bit more what that message is about?
>>> I am encountering it in a completely different context
>>> (PPP over ISDN) [...]
>>
>> Basically, calling netif_rx() with softirqs enabled.
>
> AFAICS that would have to be the netif_rx() call in
> ppp_receive_nonmp_frame() [drivers/net/ppp_generic.c#L1791],
> called (via others) from the tasklet work function
> ppp_sync_process() [drivers/net/ppp_synctty.c#L545].
> Should that be changed to the
> "if (in_interrupt()) netif_rx(skb) else netif_rx_ni(skb)"
> stanza from the linux.kernel.wireless.general thread then?
The PPP receive paths in ppp_generic.c do a local_bh_disable()/
local_bh_enable() around packet receiving (via ppp_recv_lock()/
ppp_recv_unlock() in ppp_do_recv).
So at least that part is perfectly fine.
ppp_input(), as called from ppp_sync_process(), also disables BH's
around ppp_do_recv() calls (via read_lock_bh()/read_unlock_bh()).
So that's fine too.
Do you have a bug report or are you just scanning around looking
for trouble? :-)
^ permalink raw reply
* Re: [PATCH] Generalize socket rx gap / receive queue overflow cmsg (v4)
From: David Miller @ 2009-10-12 10:01 UTC (permalink / raw)
To: eric.dumazet; +Cc: nhorman, netdev, socketcan
In-Reply-To: <4AD2B2BF.8040706@gmail.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 12 Oct 2009 06:38:23 +0200
> Neil Horman a écrit :
>> 3) This applies cleanly to net-next assuming that commit
>> 977750076d98c7ff6cbda51858bb5a5894a9d9ab (my af packet cmsg patch) is reverted
>>
>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>
> Thanks Neil
>
> I found no obvious error in this v4, except two long lines.
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
I reverted the AF_PACKET commit and applied this patch,
thanks!
^ permalink raw reply
* Re: Ping Is Broken
From: Jarek Poplawski @ 2009-10-12 9:47 UTC (permalink / raw)
To: CentOS mailing list
Cc: public-netdev-u79uwXL29TY76Z2rM5mHXA, Omaha Linux User Group
In-Reply-To: <7e84ed60910090944q5c66ea0w63ed55a72482bf2f@mail.gmail.com>
On 09-10-2009 18:44, Rob Townley wrote:
> ping -I is broken
>
> The following deals with bug in ping that made it very difficult to set up a
> system with two gateways.
>
> Demonstration that *ping -I is broken*. When specifying the source
> interface using -I with an *ethX* alias and that interface is not the
> default gateway
> interface, then ping fails. When specifying the interface as an ip address,
> ping works. Search for "Destination Host Unreachable" to find the bug.
>
>
> eth*0* = 4.3.2.8 and the default gateway is accessed through a different
> interface eth*1*.
> eth*1* = 192.168.168.155 is used as the device to get to the default
> gateway.
> *FAILS *: ping *-I eth0* 208.67.222.222
> *WORKS*: ping *-I 4.3.2.8* 208.67.222.222
> *WORKS*: ping *-I eth1* 208.67.222.222
> *WORKS*: ping *-I 192.168.168.155* 208.67.222.222
...
> man ping:
> -I interface address
> Set source address to specified interface address.
> Argument may be *numeric IP address or name of device*.
> When pinging IPv6 link-local address this option is required.
It seems this description might be misleading that IP address and name
of device are equivalent here, while they are treated a bit different.
The device name is additionally used in a sendmsg message, probably to
guarantee the device is really used (not its address only), so it
looks like intended.
> ping -V returns the latest available on CentOS and Fedora and the
> maintainers website:
> ping utility, iputils-ss020927
I guess the patch below could do what you expect in this case, but
rather "man" should be fixed...
Jarek P.
---
--- ping.c.orig 2002-09-20 15:08:11.000000000 +0000
+++ ping.c 2009-10-12 08:51:25.000000000 +0000
@@ -323,7 +323,7 @@ main(int argc, char **argv)
perror("ping: icmp open socket");
exit(2);
}
-
+#if 0
if (device) {
struct ifreq ifr;
@@ -336,7 +336,7 @@ main(int argc, char **argv)
cmsg.ipi.ipi_ifindex = ifr.ifr_ifindex;
cmsg_len = sizeof(cmsg);
}
-
+#endif
if (broadcast_pings || IN_MULTICAST(ntohl(whereto.sin_addr.s_addr))) {
if (uid) {
if (interval < 1000) {
^ permalink raw reply
* Re: [PATCH] [CAIF-RFC 0/8-v2] CAIF Protocol Stack
From: David Miller @ 2009-10-12 9:41 UTC (permalink / raw)
To: sjur.brandeland
Cc: netdev, stefano.babic, randy.dunlap, kim.xx.lilliestierna,
christian.bejram, daniel.martensson
In-Reply-To: <1255095571-6501-1-git-send-email-sjur.brandeland@stericsson.com>
From: sjur.brandeland@stericsson.com
Date: Fri, 09 Oct 2009 15:39:23 +0200
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>
>
> This is the second version of the CAIF patch set.
> The patch set is compiled on 386 for 2.6.31.
> All feedback is apreciated.
There is a lot of coding style issues here I'd like you to
address in your next submission.
Comments are formatted in several places like this:
/*
*
*
*/
It should be:
/*
*
*
*/
Also often there are tons of extraneous empty lines in the source
files. Particularly right after the comment at the top of the
source file. For example include/net/caif/generic/cfcnfg.h
there is the top comment (which needs to be fixed as described
above) and then 8 empty lines before the #ifndef CFCNFG_H_
That's rediculious, just have one empty line there seperating things.
Seriously, you should be able to just scan over your patch and see all
of these oddities. They jumped right out at me. Please clean them
up.
Thank you.
^ permalink raw reply
* Re: [PATCH] [CAIF-RFC 2/8-v2] CAIF Protocol Stack
From: Stefano Babic @ 2009-10-12 9:28 UTC (permalink / raw)
To: sjur.brandeland
Cc: netdev, kim.xx.lilliestierna, christian.bejram, daniel.martensson
In-Reply-To: <1255095571-6501-3-git-send-email-sjur.brandeland@stericsson.com>
sjur.brandeland@stericsson.com wrote:
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>
>
Hi Sjur,
> diff --git a/include/net/caif/generic/cfcnfg.h b/include/net/caif/generic/cfcnfg.h
> +/** Types of Physical Layers defined in CAIF Stack */
> +typedef enum _cfcnfg_phy_type_t {
> + CFPHYTYPE_UNKNOWN = 0,
> + CFPHYTYPE_SERIAL = 1, /*!< Serial Physical Interface */
> + CFPHYTYPE_SPI = 2, /*!< SPI Physical Interface */
> + CFPHYTYPE_MSL = 3, /*!< MSL Physical Interface */
> + CFPHYTYPE_SHM = 4, /*!< Shared Memory Physical Interface */
You actually removed the shared memory driver. Do you plan to insert it
again ?
> +/**
> + * Adds a Adaptation Layer to the CAIF Stack.
> + * The Adaptation Layer is where the interface to application or higher-level
> + * driver functionality is implemented.
> + * \image html AddVeiCaifConfig.jpg "Add an Adaptation layer to CAIF Stack."
There are references to pictures that are not provided.
> diff --git a/include/net/caif/generic/cfglue.h b/include/net/caif/generic/cfglue.h
[snip]
> +/* ASSERT */
> +#define cfglu_assert(exp) BUG_ON(!(exp))
I do not why, but even GENERIC_BUG is not defined for all architectures.
This means that BUG_ON is simply removed by the compiler and we get no
track if the assert fails.
It should be better to add at least an internal trace (adding for
example a CFLOG_FATAL call).
> diff --git a/include/net/caif/generic/cfshml.h b/include/net/caif/generic/cfshml.h
Is thif file probably obsolete ?
> diff --git a/include/net/caif/generic/cfspil.h b/include/net/caif/generic/cfspil.h
> +/** @page SPI PHY Layer description.
> + *
> + * SPI Physical layer is not implemented in GenCaif. The SPI PHY Layer
> + * is HW dependent. But the CFSPIL (Caif SPI Layer) provides support for
> + * implementing the SPI Layer Protocol.
Not sure I have understood. There is an abstraction layer for SPI in
kernel and some generic purpose functions are provided (spy_sync,
spi_async, etc.) that are HW independent. I know they provide only the
methods for the data transfer, but I have imagined that we need at this
point some kind of "caif_spi_device" that is able to talk with the caif
stack on one side and uses the functions in the SPI framework. In any
case, HW independent. Have I missed something ?
Stefano
--
stefano <stefano.babic@babic.homelinux.org>
GPG Key: 0x55814DDE
Fingerprint 4E85 2A66 4CBA 497A 2A7B D3BF 5973 F216 5581 4DDE
^ 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