* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: David Miller @ 2012-08-07 23:29 UTC (permalink / raw)
To: ian.molton
Cc: linux-arm-kernel, andrew, thomas.petazzoni, ben.dooks, arnd,
netdev
In-Reply-To: <1344350092-24050-1-git-send-email-ian.molton@codethink.co.uk>
From: Ian Molton <ian.molton@codethink.co.uk>
Date: Tue, 7 Aug 2012 15:34:45 +0100
> Fixed all comments.
>
> * Dropped csb1724 defconfig.
> * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
> * Dropped un-necessary D-T irq fixup code
Who is going to take this series?
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Matt Sealey @ 2012-08-08 0:31 UTC (permalink / raw)
To: David Miller
Cc: ian.molton, thomas.petazzoni, andrew, arnd, netdev, ben.dooks,
linux-arm-kernel
In-Reply-To: <20120807.162923.34400427265666163.davem@davemloft.net>
On Tue, Aug 7, 2012 at 6:29 PM, David Miller <davem@davemloft.net> wrote:
> From: Ian Molton <ian.molton@codethink.co.uk>
> Date: Tue, 7 Aug 2012 15:34:45 +0100
>
>> Fixed all comments.
>>
>> * Dropped csb1724 defconfig.
>> * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
>> * Dropped un-necessary D-T irq fixup code
>
> Who is going to take this series?
Would anyone mind too much if I *didn't* break out a Pegasos II and
test it? Our platform has a Marvell northbridge (Discovery II)
implementing this, with a Marvell PHY, and it's OpenFirmware (as in,
REAL OpenFirmware) so the device tree isn't about to change to fit new
bindings. But I'm not sure we even have one in the office that boots
anymore.. there may be users out there but they're well beyond
warranty support (early 2005 or so was the last time we sold one).
If anyone needs the original device tree entries to compare and
contrast I may be able to provide them such that any parsing and
initializing of the driver take into account this old
board/northbridge/implementation. I'm just curious if anyone cares
enough..
--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply
* RE: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver
From: Arun MURTHY @ 2012-08-08 3:03 UTC (permalink / raw)
To: Alan Cox
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, gregkh@linuxfoundation.org,
Sjur BRENDELAND
In-Reply-To: <20120807110156.4d0c2571@pyramind.ukuu.org.uk>
> > +#define SIZE_OF_FIFO (512*1024)
> > +
> > +static u8 message_fifo[ISA_DEVICES][SIZE_OF_FIFO];
>
> Thats a huge amount of static memory that gets allocated regardless of
> whether the device is open or being used ?
Yes, but all of these will be used, in any system which includes APE-Modem
communication.
>
> > +static int major;
> > +module_param(major, int, 0);
> > +MODULE_PARM_DESC(major, "Major device number");
>
> Is this really needed still - it looks like an escaped debugging aid
Yup, will remove this:-)
>
> > +/* global fops mutex */
> > +static DEFINE_MUTEX(isa_lock);
> > +
> > +/**
> > + * shrm_get_cdev_index() - return the index mapped to l2 header
> > + * @l2_header: L2 header
> > + *
> > + * struct map_device maps the index(count) with the device L2
> header.
> > + * This function returns the index for the provided L2 header in
> case
> > + * of success else -ve value.
> > + */
> > +int shrm_get_cdev_index(u8 l2_header)
> > +{
> > + u8 cnt;
> > + for (cnt = 0; cnt < ISA_DEVICES; cnt++) {
> > + if (map_dev[cnt].l2_header == l2_header)
> > + return map_dev[cnt].idx;
> > + }
> > + return -EINVAL;
> > +}
>
> Would be good to document the locking assumptions on this lot
Sure will add a note on this locking mechanism.
>
> > + /* reset the msg queue pointers */
> > + q->size = SIZE_OF_FIFO;
> > + q->readptr = 0;
> > + q->writeptr = 0;
>
> Is there a reason for not using the existing kfifo layer for this work
> or
> does it need to handle other things kfifo cannot.
Basically it doesn't suit our protocol of having base addr, read/write
pointer, locking etc as the same set of structures and protocol will be
used on the modem side implementation.
>
> > +static u32 isa_select(struct file *filp,
> > + struct poll_table_struct *wait)
> > +{
> > + struct isadev_context *isadev = filp->private_data;
> > + struct shrm_dev *shrm = isadev->dl_queue.shrm;
> > + struct message_queue *q;
> > + u32 mask = 0;
> > + u32 m = iminor(filp->f_path.dentry->d_inode);
> > + u8 idx = shrm_get_cdev_index(m);
> > +
> > + dev_dbg(shrm->dev, "%s IN\n", __func__);
> > +
> > + if (shrm->msr_flag)
> > + return -ENODEV;
>
> This test has no associated locking. Please explain what stop sit
> becoming true during the rest of this function ?
Locking in this file has been used only for protecting the handling
of queues.
When modem is reset, due to some crash or some unknown event, in
that case, this flag will be set and we are not suppose to communicate
any further as modem is in reset state.
>
> + struct shrm_dev *shrm = isadev->dl_queue.shrm;
> > + struct message_queue *q;
> > + u32 msgsize;
> > +
> > + dev_dbg(shrm->dev, "%s IN\n", __func__);
> > +
> > + if (len <= 0)
> > + return -EFAULT;
>
> How can this occur ?
Check for error condition
> > + spin_lock_bh(&q->update_lock);
> > + if (list_empty(&q->msg_list)) {
> > + spin_unlock_bh(&q->update_lock);
> > + dev_dbg(shrm->dev, "Waiting for Data\n");
> > + if (wait_event_interruptible(q->wq_readable,
> > + atomic_read(&q->q_rp) == 1))
> > + return -ERESTARTSYS;
> > + } else
> > + spin_unlock_bh(&q->update_lock);
> > +
> > + if (shrm->msr_flag) {
> > + atomic_set(&q->q_rp, 0);
> > + return -ENODEV;
> > + }
> > +
> > + msgsize = get_size_of_new_msg(q);
> > +
> > + if (len < msgsize)
> > + return -EINVAL;
>
> What happens with two parallel reads - I don't see what prevents
> corruption if that occurs or one racing read freeing the message before
> another has finished processing it.
Two parallel reads for different L2 headers can happen, but within the
same L2 header is out of the scope. Since the client using this in
user space will not know about the message. i.e which msg is for which
client. Hence so that scenario is not considered.
>
> > +
> > + if ((q->readptr+msgsize) >= q->size) {
>
> Minor style comment - you seen to randomly switch between a+b and a + b
> styles 8)
Will correct it.
> > + size = (q->size-q->readptr);
> > + /* Copy First Part of msg */
> > + if (copy_to_user(psrc,
> > + (u8 *)(q->fifo_base+q->readptr),
> > + size)) {
> > + dev_err(shrm->dev, "copy_to_user failed\n");
> > + return -EFAULT;
> > + }
> > + psrc += size;
> > + /* Copy Second Part of msg at the top of fifo */
> > + if (copy_to_user(psrc,
> > + (u8 *)(q->fifo_base),
> > + (msgsize-size))) {
> > + dev_err(shrm->dev, "copy_to_user failed\n");
> > + return -EFAULT;
> > + }
> > + } else {
> > + if (copy_to_user(buf,
> > + (u8 *)(q->fifo_base + q->readptr),
> > + msgsize)) {
> > + dev_err(shrm->dev, "copy_to_user failed\n");
> > + return -EFAULT;
> > + }
>
> Pedantically you should return the number of bytes successfully copied
> if
> you consume data. As this code stands it's not a big deal as far as I
> can
> see since you don't remove the message in this situation. I suspect
> that
> may need to change ?
In error case, if the number of messages copied are returned, there is
no way to communicate the error and hence only that partial message
will be considered and on decoding that message, will be a flaw.
> > +/**
> > + * isa_write() - Write to shrm char device
> > + * @filp: file descriptor
> > + * @buf: user buffer pointer
> > + * @len: size of requested data transfer
> > + * @ppos: not used
> > + *
> > + * It checks if there is space available in queue, and copies the
> message
> > + * inside queue. If there is no space, it blocks until space becomes
> available.
> > + * It also schedules transfer thread to transmit the newly added
> message.
> > + */
> > +ssize_t isa_write(struct file *filp, const char __user *buf,
> > + size_t len, loff_t *ppos)
> > +{
> > + struct isadev_context *isadev = filp->private_data;
> > + struct shrm_dev *shrm = isadev->dl_queue.shrm;
> > + struct message_queue *q;
> > + void *addr = 0;
> > + int err, l2_header;
> > + int ret = 0;
> > +
> > + dev_dbg(shrm->dev, "%s IN\n", __func__);
> > +
> > + if (len <= 0 || buf == NULL)
> > + return -EFAULT;
>
> len < 0 cannot occur, buf == NULL is not an error
Error handling is for what which is not expected.
>
> > + q = &isadev->dl_queue;
> > + l2_header = shrm_get_cdev_l2header(isadev->device_id);
> > + if (l2_header < 0) {
> > + dev_err(shrm->dev, "failed to get L2 header\n");
> > + return l2_header;
> > + }
> > +
> > + switch (l2_header) {
>
> Why keep going through switches and lookup tables - can't you cache
> this
> in the isadev or in some file private data ?
This is a write request coming from user space, hence the identification
has to be done based on the write request.
>
> > + if (copy_from_user(addr, buf, len)) {
> > + dev_err(shrm->dev, "copy_from_user failed\n");
>
> This allows an user to fill the logs with crap - should be dev_dbg
>
> Also what happens on two parallel writers ?
Explained above.
>
>
> > +/**
> > + * isa_ioctl() - To handle different ioctl commands supported by
> driver.
> > + * @inode: structure is used by the kernel internally to
> represent files
> > + * @filp: file descriptor pointer
> > + * @cmd: ioctl command
> > + * @arg: input param
>
> This appears incomplete - it's just debug statements
Yes, will remove this.
>
>
> > +}
> > +/**
> > + * isa_mmap() - Maps kernel queue memory to user space.
> > + * @filp: file descriptor pointer
> > + * @vma: virtual area memory structure.
> > + *
> > + * This function maps kernel FIFO into user space. This function
> > + * shall be called twice to map both uplink and downlink buffers.
>
> Again this appears incomplete
Will work on this.
> > + dev_err(shrm->dev, "Device not opened yet\n");
> > + mutex_unlock(&isa_lock);
> > + return -ENODEV;
> > + }
> > + atomic_set(&isa_context->is_open[idx], 1);
>
> How do you know it will always be one. Also given it's within the mutex
> in all uses I can see why is it an atomic ?
>
As per our assumptions/protocol only one client per L2 header.
> > +
> > + switch (m) {
> > + case RPC_MESSAGING:
> > + dev_info(shrm->dev, "Close RPC_MESSAGING Device\n");
> > + break;
> > + case AUDIO_MESSAGING:
> > + dev_info(shrm->dev, "Close AUDIO_MESSAGING Device\n");
> > + break;
> > + case SECURITY_MESSAGING:
> > + dev_info(shrm->dev, "CLose SECURITY_MESSAGING Device\n");
> > + break;
> > + case COMMON_LOOPBACK_MESSAGING:
> > + kfree(isadev->addr);
> > + dev_info(shrm->dev, "Close COMMON_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case AUDIO_LOOPBACK_MESSAGING:
> > + kfree(isadev->addr);
> > + dev_info(shrm->dev, "Close AUDIO_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case CIQ_MESSAGING:
> > + kfree(isadev->addr);
> > + dev_info(shrm->dev, "Close CIQ_MESSAGING Device\n");
> > + break;
> > + case RTC_CAL_MESSAGING:
> > + dev_info(shrm->dev, "Close RTC_CAL_MESSAGING Device\n");
> > + break;
>
> kfree(NULL) is a no-op and guaranteed so, providing you see isadev-
> >addr
> = NULL properly in the open path you can delete all of this code and
> just
> do
> kfree(isadev->addr);
> mutex_unlock(...)
> return 0;
Ok, sounds good!
>
> > + if (get_boot_state() != BOOT_DONE) {
> > + dev_err(shrm->dev, "Boot is not done\n");
> > + return -EBUSY;
> > + }
>
> Is it guaranteed that this is a one way path - ie a device never goes
> back into BOOT state ?
No, on modem reset, everything happens from first.
>
> > + if (!atomic_dec_and_test(&isa_context->is_open[idx])) {
> > + atomic_inc(&isa_context->is_open[idx]);
> > + dev_err(shrm->dev, "Device already opened\n");
> > + mutex_unlock(&isa_lock);
> > + return -EBUSY;
>
> See comments earlier about the atomics
Replied above
>
> > + }
> > + isadev = &isa_context->isadev[idx];
> > + if (filp != NULL)
> > + filp->private_data = isadev;
>
> How can filp be NULL ?
:-) just a error condition check
>
> > +
> > + switch (m) {
> > + case RPC_MESSAGING:
> > + dev_info(shrm->dev, "Open RPC_MESSAGING Device\n");
> > + break;
> > + case AUDIO_MESSAGING:
> > + dev_info(shrm->dev, "Open AUDIO_MESSAGING Device\n");
> > + break;
> > + case SECURITY_MESSAGING:
> > + dev_info(shrm->dev, "Open SECURITY_MESSAGING Device\n");
> > + break;
> > + case COMMON_LOOPBACK_MESSAGING:
> > + isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
> > + if (!isadev->addr) {
> > + mutex_unlock(&isa_lock);
> > + return -ENOMEM;
> > + }
> > + dev_info(shrm->dev, "Open COMMON_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case AUDIO_LOOPBACK_MESSAGING:
> > + isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
> > + if (!isadev->addr) {
> > + mutex_unlock(&isa_lock);
> > + return -ENOMEM;
> > + }
> > + dev_info(shrm->dev, "Open AUDIO_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case CIQ_MESSAGING:
> > + isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
> > + if (!isadev->addr) {
> > + mutex_unlock(&isa_lock);
> > + return -ENOMEM;
> > + }
> > + dev_info(shrm->dev, "Open CIQ_MESSAGING Device\n");
> > + break;
> > + case RTC_CAL_MESSAGING:
> > + dev_info(shrm->dev, "Open RTC_CAL_MESSAGING Device\n");
> > + break;
> > + };
>
> So all of this could just be
>
> dev_dbg("Open device %d\n", m);
> switch (m) {
> case a
> case b
> case c
> isadev->addr = ....
> break;
> case e
> case f
> isadev->addr = NULL;
> break;
> default:
> error path
>
> also in your error case right now if you run out of memory you don't
> fix
> up the open counters.
Ok sounds good!
>
> > +const struct file_operations isa_fops = {
>
> static
>
>
> > + for (no_dev = 0; no_dev < ISA_DEVICES; no_dev++) {
> > + atomic_set(&isa_context->is_open[no_dev], 1);
> > + device_create(isa_context->shm_class, NULL,
> > + MKDEV(MAJOR(dev_id),
> > + map_dev[no_dev].l2_header), NULL,
> > + map_dev[no_dev].name);
> > + }
>
> What happens if I open the device right here... ?
It can be opened, but nothing thereafter, since modem is not booted.
>
> > +
> > + isa_context->isadev = kzalloc(sizeof
> > + (struct isadev_context)*ISA_DEVICES,
> > + GFP_KERNEL);
> > + if (isa_context->isadev == NULL) {
> > + dev_err(shrm->dev, "Failed to alloc memory\n");
> > + return -ENOMEM;
>
> You just leaked all the other stuff you created, left device pointers
> to
> invalid memory registered.
>
> There is a reason kernel drivers use the goto type error handling - it
> avoids missing these things.
Oops missed it will correct.
>
>
> > diff --git a/drivers/modem_shm/u8500_shm/shrm_fifo.c
> b/drivers/modem_shm/u8500_shm/shrm_fifo.c
> > new file mode 100644
> > index 0000000..ad63cc4
> > --- /dev/null
> > +++ b/drivers/modem_shm/u8500_shm/shrm_fifo.c
> > @@ -0,0 +1,837 @@
> > +/*
> > + * Copyright (C) ST-Ericsson SA 2010
> > + *
> > + * Author: Arun Murthy <arun.murthy@stericsson.com>
> > + * Kumar Sanghvi for ST-Ericsson
> > + *
> > + * License terms: GNU General Public License (GPL) version 2
> > + */
> > +
> > +#include <linux/modem_shm/u8500_shm/shrm.h>
> > +#include <linux/modem_shm/u8500_shm/shrm_driver.h>
> > +#include <linux/modem_shm/u8500_shm/shrm_private.h>
> > +#include <linux/modem_shm/u8500_shm/shrm_net.h>
> > +#include <linux/mfd/dbx500-prcmu.h>
> > +
> > +#define L1_BOOT_INFO_REQ 1
> > +#define L1_BOOT_INFO_RESP 2
> > +#define L1_NORMAL_MSG 3
> > +#define L1_HEADER_MASK 28
> > +#define L1_MAPID_MASK 0xF0000000
> > +#define CONFIG_OFFSET 8
> > +#define COUNTER_OFFSET 20
> > +#define L2_HEADER_SIZE 4
> > +#define L2_HEADER_OFFSET 24
> > +#define MASK_0_15_BIT 0xFF
> > +#define MASK_16_31_BIT 0xFF00
> > +#define MASK_16_27_BIT 0xFFF0000
> > +#define MASK_0_39_BIT 0xFFFFF
> > +#define MASK_40_55_BIT 0xFF00000
> > +#define MASK_8_16_BIT 0x0000FF00
> > +#define MSG_LEN_OFFSET 16
> > +#define SHRM_VER 2
> > +#define ca_ist_inactivity_timer 25 /*25ms */
> > +#define ca_csc_inactivity_timer 25 /*25ms */
> > +
> > +static u8 msg_audio_counter;
> > +static u8 msg_common_counter;
> > +
> > +struct fifo_write_params ape_shm_fifo_0;
> > +struct fifo_write_params ape_shm_fifo_1;
> > +struct fifo_read_params cmt_shm_fifo_0;
> > +struct fifo_read_params cmt_shm_fifo_1;
>
> static or fix the naming
Done!
> > +struct sock *shrm_nl_sk;
>
> Better to make globals visible than bury them in a mass of statics
>
> (actually all of this raises a big question - what happens if you ever
> need to have two modems - you are basically hardcoding one per system.
> That's fine if you are *very* sure it will remain true)
Atleast as per the present protocol, two modem is not supported, and if
so in future, then the protocol will have to be affected. The interrupts
part is the major part.
> > + spin_lock_irqsave(&boot_lock, flags);
> > + boot_state = BOOT_INIT;
>
> Your open method locking earlier assumes this cannot go backwards...
This can go backward, locking is done only while modifying this flag.
>
>
> > + dev_info(shm_dev->dev, "Initiating Modem silent reset\n");
>
> Seems an odd use of dev_info.. if its a bug workaround then perhaps
> dev_warn, and if its not important dev_dbg ?
Would prefer dev_dbg
>
> > +DECLARE_TASKLET(shrm_sw_reset_callback, shrm_modem_reset_callback,
> > + IRQ_PRCMU_MODEM_SW_RESET_REQ);
> > +
>
> static
>
> Lots of other stuff here that should be shrm_ or static...
>
> There also seems to be places where you copy or touch the fifo memory
> which was ioremapped but don't use readb/writeb/memcpy_toio etc, or
> handle endianness. While endianness might be the kind of thing you can
> be
> sure of in some embedded platforms not using readb and the like means
> the
> compiler can do interesting optimisations you won't want and is
> something
> that will break on some architectures and potentially on others in
> future.
Sure will look into that again and implement this in the next version
of the patch.
Thanks and Regards,
Arun R Murthy
--------------
^ permalink raw reply
* RE: [PATCHv2 1/4] modem_shm: Add Modem Access Framework
From: Arun MURTHY @ 2012-08-08 3:36 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, Sjur BRENDELAND
In-Reply-To: <20120807183809.GC26990@kroah.com>
> On Tue, Aug 07, 2012 at 12:24:28PM +0530, Arun Murthy wrote:
> > Adds Modem Access Framework, which allows for registering platform
> specific
> > modem access mechanisms. The framework also exposes APIs for client
> drivers
> > for getting and releasing access to modem, regardless of the
> underlying
> > platform specific access mechanism.
>
> The term "modems" here has a lot of legacy connotations. First of
> which
> is, userspace handles this today as tty devices, why aren't you doing
> the same here? Why does this have to be something "special"?
>
The main focus over there the modem IPC. In doing so, there are some
functionality like waking the modem, or releasing the modem etc. These
will be used by the modem IPC drivers and also few others like sim driver
and security drivers.
Since this is a shared call and hence has to be synchronized. Hence so a
small framework like is being done to monitor the modem access related only
operations.
> >
> > Signed-off-by: Arun Murthy <arun.murthy@stericsson.com>
> > Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> > ---
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/modem_shm/Kconfig | 9 +
> > drivers/modem_shm/Makefile | 1 +
> > drivers/modem_shm/modem_access.c | 419
> ++++++++++++++++++++++++++++++++
>
> Any reason why this can't be under drivers/tty/ ?
No specific reason, other than to have a separate place for the modem
access related drivers. Many of the platforms have these functionality
and hence all of them can reside over here using a common framework(MAF)
>
> > include/linux/modem_shm/modem.h | 64 +++++
> > include/linux/modem_shm/modem_client.h | 55 +++++
>
> Why are both of these "public" like this? Why not just make one file?
> Would someone ever only need to include one of these?
modem.h is a header file used by the MAF, but modem_client.h includes
a structure that will be used by all clients registering to this MAF.
Hence the clients can only include only this header.
>
> > +config MODEM_SHM
> > + bool "Modem Access Framework"
> > + default y
>
> Unless it is needed to boot your machine, NEVER make the default y.
Oops,.. will change this.
>
> > +struct modem {
> > + struct device *dev;
> > + struct list_head list;
> > + char *modem_name;
>
> You already have a name in the struct device, why do you need another
> one?
>
> > + struct device_attribute dev_attr;
>
> Why is this in the structure?
>
> > + struct modem_dev *mdev;
> > + atomic_t use;
>
> What is this variable for?
This is used to monitor the number of requests received. Just to
balance the enable/disable process. Something like the regulator
framework.
>
> Why isn't this a 'struct device' itself?
>
> > +/**
> > + * modem_is_requested - check if modem access is requested
> > + * @modem: modem device
> > + *
> > + * Checks whether modem is accessed or not by querying
> > + * the underlying platform specific modem access
> > + * implementation.
> > + */
> > +int modem_is_requested(struct modem *modem)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&modem->mdev->mutex);
> > + ret = _modem_is_requested(modem->mdev);
> > + mutex_unlock(&modem->mdev->mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(modem_is_requested);
>
> EXPORT_SYMBOL_GPL() for this, and the other apis perhaps?
OK.
>
> > +static struct modem *_modem_get(struct device *dev, const char *id,
> > + int exclusive)
> > +{
> > + struct modem_dev *mdev_ptr;
> > + struct modem *modem = ERR_PTR(-ENODEV);
> > + int ret;
> > +
> > + if (id == NULL) {
> > + pr_err("modem_get with no identifier\n");
> > + return modem;
> > + }
> > +
> > + mutex_lock(&modem_list_mutex);
> > + list_for_each_entry(mdev_ptr, &modem_list, modem_list) {
> > + if (strcmp(mdev_get_name(mdev_ptr), id) == 0)
> > + goto found;
> > + }
> > +
> > + goto out;
> > +
> > +found:
> > + if (!try_module_get(mdev_ptr->owner))
> > + goto out;
> > +
> > + modem = create_modem(mdev_ptr, dev, id);
> > + if (modem == NULL) {
> > + modem = ERR_PTR(-ENOMEM);
> > + module_put(mdev_ptr->owner);
> > + }
> > +
> > + mdev_ptr->open_count++;
> > + ret = _modem_is_requested(mdev_ptr);
> > + if (ret)
> > + mdev_ptr->use_count = 1;
> > + else
> > + mdev_ptr->use_count = 0;
> > +
> > +out:
> > + mutex_unlock(&modem_list_mutex);
> > + return modem;
> > +
> > +}
>
> Calling this function does a lot more than just incrementing the
> reference count of an object. It sets it up, and creates things. That
> should be way more documented than this one line says:
Sure will elaborate the documentation.
>
> > +/**
> > + * modem_get - Get reference to a particular platform specific modem
> > + * @dev: device
> > + * @id: modem device name
> > + *
> > + * Get reference to a particular modem device.
> > + */
>
> See, that's really not true.
>
> > +static ssize_t modem_print_state(char *buf, int state)
> > +{
> > + if (state > 0)
> > + return sprintf(buf, "accessed\n");
> > + else if (state == 0)
> > + return sprintf(buf, "released\n");
> > + else
> > + return sprintf(buf, "unknown\n");
> > +}
>
> Why not use an enumerated type for your state, to make it easier to
> understand than 0, -, and +?
Done
>
> > +static ssize_t modem_state_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct modem_dev *mdev = dev_get_drvdata(dev);
> > + ssize_t ret;
> > +
> > + mutex_lock(&mdev->mutex);
> > + ret = modem_print_state(buf, _modem_is_requested(mdev));
>
> Why not just embed the function here? It's only ever called once.
This function will be called by the modem IPC drivers also.
>
> > + mutex_unlock(&mdev->mutex);
> > +
> > + return ret;
> > +}
> > +static DEVICE_ATTR(state, 0444, modem_state_show, NULL);
> > +
> > +static ssize_t modem_use_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct modem_dev *mdev = dev_get_drvdata(dev);
> > + struct modem *mod;
> > + size_t size = 0;
> > +
> > + list_for_each_entry(mod, &mdev->client_list, list) {
> > + if (mod->dev != NULL)
> > + size += sprintf((buf + size), "%s (%d)\n",
> > + dev_name(mod->dev), atomic_read(&mod->use));
> > + else
> > + size += sprintf((buf + size), "unknown (%d)\n",
> > + atomic_read(&mod->use));
> > + }
> > + size += sprintf((buf + size), "\n");
> > +
> > + return size;
> > +}
> > +static DEVICE_ATTR(use, 0444, modem_use_show, NULL);
>
> You have sysfs files with no matching Documentation/ABI entries showing
> how they are to be used, and what they contain, in this patch. Please
> fix this.
>
> And why are you reporting an atomic value, that's 2 values per sysfs
> file, not acceptable.
>
Will change it accordingly.
> > +static ssize_t modem_name_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct modem_dev *mdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%s\n", mdev_get_name(mdev));
> > +}
> > +static DEVICE_ATTR(name, 0444, modem_name_show, NULL);
>
> The name is in the struct device, which is the directory in sysfs,
> don't
> include it again in a sysfs file, that's redundant.
Ok.
>
> > +static int add_modem_attributes(struct modem_dev *mdev)
> > +{
> > + struct device *dev = &mdev->dev;
> > + struct modem_ops *ops = mdev->desc->ops;
> > + int status = 0;
> > +
> > + status = device_create_file(dev, &dev_attr_use);
> > + if (status < 0)
> > + return status;
> > +
> > + status = device_create_file(dev, &dev_attr_name);
> > + if (status < 0)
> > + return status;
> > +
> > + status = device_create_file(dev, &dev_attr_num_active_users);
> > + if (status < 0)
> > + return status;
> > +
> > + if (ops->is_requested) {
> > + status = device_create_file(dev, &dev_attr_state);
> > + if (status < 0)
> > + return status;
> > + }
> > +
> > + return 0;
> > +}
>
> Please use a default attribute group, as you just raced with userspace,
> and now userspace tried to look for these files when the device was
> created, yet they were not present yet, causing all sorts of problems
> with your tools. This must be fixed.
>
Done. Will implement this in the next version of the patch.
Thanks and Regards,
Arun R Murthy
--------------
^ permalink raw reply
* RE: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver
From: Arun MURTHY @ 2012-08-08 3:36 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, Sjur BRENDELAND
In-Reply-To: <20120807183925.GD26990@kroah.com>
> > drivers/modem_shm/Kconfig | 2 +
> > drivers/modem_shm/Makefile | 1 +
> > drivers/modem_shm/u8500_shm/Kconfig | 43 +
> > drivers/modem_shm/u8500_shm/Makefile | 7 +
> > drivers/modem_shm/u8500_shm/shrm_char.c | 895 ++++++++++++
> > drivers/modem_shm/u8500_shm/shrm_driver.c | 732 ++++++++++
> > drivers/modem_shm/u8500_shm/shrm_fifo.c | 837 ++++++++++++
> > drivers/modem_shm/u8500_shm/shrm_net.c | 312 +++++
> > drivers/modem_shm/u8500_shm/shrm_protocol.c | 1590
> ++++++++++++++++++++++
> > include/linux/modem_shm/u8500_shm/shrm.h | 23 +
> > include/linux/modem_shm/u8500_shm/shrm_config.h | 114 ++
> > include/linux/modem_shm/u8500_shm/shrm_driver.h | 225 +++
> > include/linux/modem_shm/u8500_shm/shrm_ioctl.h | 43 +
> > include/linux/modem_shm/u8500_shm/shrm_net.h | 46 +
> > include/linux/modem_shm/u8500_shm/shrm_private.h | 183 +++
>
> Why do any of those .h files need to be in include/linux/*? Shouldn't
> they just be in the same directory as the driver itself?
Can be moved to the same dir. Will do that in the next ver of the patch.
Thanks and Regards,
Arun R Murthy
-------------
^ permalink raw reply
* What is the significance of coherent_dma_mask?
From: Venu Byravarasu @ 2012-08-08 3:58 UTC (permalink / raw)
To: gregkh, rdunlap, lars, grant.likely, joe; +Cc: netdev, linux-kernel, swarren
Hi,
Some of the USB drivers are setting coherent_dma_mask & many others are
not doing it.
From the description present in the comment beside this field in
device.h, I could not understand its real significance.
Can some one plz let me know its real need and use case where it is
mandatory to use it?
Thanks,
Venu
^ permalink raw reply
* RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Jain Priyanka-B32167 @ 2012-08-08 4:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20120807.162138.5000306557864030.davem@davemloft.net>
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, August 08, 2012 4:52 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Priyanka Jain <Priyanka.Jain@freescale.com>
Date: Tue, 7 Aug 2012 10:51:44 +0530
> xfrm_policy_afinfo is read mosly data structure.
> Write on xfrm_policy_afinfo is done only at the time of configuration.
> So rwlocks can be safely replaced with RCU.
>
> RCUs usage optimizes the performance.
>
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
This patch doesn't apply to the net-next tree, please respin.
[Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
Also:
> - xfrm_policy_afinfo[afinfo->family] = NULL;
> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
> + NULL);
Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
[Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
Please let me know your final say on this. I will make changes accordingly if required.
Thanks
Priyanka
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-08 5:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric W. Biederman, Or Gerlitz, davem, roland, netdev, ali,
sean.hefty, Erez Shitrit, Doug Ledford
In-Reply-To: <20120805185031.GA18640@redhat.com>
On Sun, Aug 5, 2012 at 9:50 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
[...]
> So it seems that a sane solution would involve an extra level of
> indirection, with guest addresses being translated to host IB addresses.
> As long as you do this, maybe using an ethernet frame format makes sense.
[...]
Yep, that's among the points we're trying to make, the way you've put
it makes it clearer.
> So far the things that make sense. Here are some that don't, to me:
> - Is a pdf presentation all you have in terms of documentation?
> We are talking communication protocols here - I would expect a
> proper spec, and some effort to standardize, otherwise where's the
> guarantee it won't change in an incompatible way?
To be precise, the solution uses 100% IPoIB wire-protocol, so we don't
see a need
for any spec change / standardization effort. This might go to the 1st
point you've
brought... improve the documentation, will do that. The pdf you looked
at was presented
in a conference.
> Other things that I would expect to be addressed in such a spec is
> interaction with other IPoIB features, such as connected
> mode, checksum offloading etc, and IB features such as multipath etc.
For the eipoib interface, it doesn't really matters if the underlyind
ipoib clones used by it (we call them VIFs) use connected or datagram
mode, what does matter is the MTU and offload features supported by
these VIFs, for which the eipoib interface will have the min among all
these VIFs. Since for a given eipoib nic, all its VIFs must originated
from the same IPoIB PIF (e.g ib0) its easy admin job to make sure they
all have the same mtu / features which are needed for that eipoib nic,
e.g by using the same mode (connected/datagram for all of them), hope
this is clear.
> - The way you encode LID/QPN in the MAC seems questionable. IIRC there's
> more to IB addressing than just the LID. Since everyone on the subnet
> need access to this translation, I think it makes sense to store it in
> the SM. I think this would also obviate some IPv4 specific hacks in kernel.
The idead beyond the encoding was uniqueness, LID/QPN is unique per IB
HCA end-node. I wasn't sure to understand the comment re the IPv4 hacks.
> - IGMP/MAC snooping in a driver is just too hairy.
mmm, any rough idea/direction how to do that otherwise?
> As you point out, bridge currently needs the uplink in promisc mode.
> I don't think a driver should work around that limitation.
> For some setups, it might be interesting to remove the
> promisc mode requirement, failing that,
> I think you could use macvtap passthrough.
That's in the plans, the current code doesn't assume that the eipoib
has bridge on top, for VM networking it works with bridge + tap,
bridge + macvtap, but it would easily work with passthrough when we
allow to create multiple eipoib interfaces on the same ipoib PIF (e.g
today for the ib0 PIF we create eipoib eth0, and then two VIFs ib0.1
and ib0.2 that are enslaved by eth0, but next we will create eth1 and
eth2 which will use ib0.1 and ib0.2
respectively.
> - Currently migration works without host kernel help, would be
> preferable to keep it that way.
OK
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie @ 2012-08-08 5:28 UTC (permalink / raw)
To: Oliver Neukum
Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-usb,
netdev
In-Reply-To: <1377311.1NXjxk1Tbx@linux-lqwf.site>
Hi Oliver,
On Mon, Aug 06, 2012 at 10:10:43AM +0200, Oliver Neukum wrote:
> On Monday 06 August 2012 07:21:29 Olivier Sobrie wrote:
> > This driver provides support for several Kvaser CAN/USB devices.
> > Such kind of devices supports up to three can network interfaces.
> >
> > It has been tested with a Kvaser USB Leaf Light (one network interface)
> > connected to a pch_can interface.
> > The firmware version of the Kvaser device was 2.5.205.
>
> > +static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> > + struct net_device *netdev)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > + struct kvaser_usb *dev = priv->dev;
> > + struct net_device_stats *stats = &netdev->stats;
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > + struct kvaser_usb_tx_urb_context *context = NULL;
> > + struct urb *urb;
> > + void *buf;
> > + struct kvaser_msg *msg;
> > + int i, err;
> > + int ret = NETDEV_TX_OK;
> > +
> > + if (can_dropped_invalid_skb(netdev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + urb = usb_alloc_urb(0, GFP_ATOMIC);
> > + if (!urb) {
> > + netdev_err(netdev, "No memory left for URBs\n");
> > + stats->tx_dropped++;
> > + dev_kfree_skb(skb);
> > + return NETDEV_TX_OK;
> > + }
> > +
> > + buf = usb_alloc_coherent(dev->udev, sizeof(struct kvaser_msg),
> > + GFP_ATOMIC, &urb->transfer_dma);
>
> usb_alloc_coherent() as a rule only makes sense if you reuse the buffer
> and in some cases not even then. Please use a simple kmalloc()
Ok thanks. I'll change it.
>
> > + if (!buf) {
> > + netdev_err(netdev, "No memory left for USB buffer\n");
> > + stats->tx_dropped++;
> > + dev_kfree_skb(skb);
> > + goto nobufmem;
> > + }
> > +
> > + msg = (struct kvaser_msg *)buf;
> > + msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
> > + msg->u.tx_can.flags = 0;
> > + msg->u.tx_can.channel = priv->channel;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + msg->id = CMD_TX_EXT_MESSAGE;
> > + msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
> > + msg->u.tx_can.msg[1] = (cf->can_id >> 18) & 0x3f;
> > + msg->u.tx_can.msg[2] = (cf->can_id >> 14) & 0x0f;
> > + msg->u.tx_can.msg[3] = (cf->can_id >> 6) & 0xff;
> > + msg->u.tx_can.msg[4] = cf->can_id & 0x3f;
> > + } else {
> > + msg->id = CMD_TX_STD_MESSAGE;
> > + msg->u.tx_can.msg[0] = (cf->can_id >> 6) & 0x1f;
> > + msg->u.tx_can.msg[1] = cf->can_id & 0x3f;
> > + }
> > +
> > + msg->u.tx_can.msg[5] = cf->can_dlc;
> > + memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
> > +
> > + for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
> > + if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> > + context = &priv->tx_contexts[i];
> > + break;
> > + }
> > + }
> > +
> > + if (!context) {
> > + netdev_warn(netdev, "cannot find free context\n");
> > + ret = NETDEV_TX_BUSY;
> > + goto releasebuf;
> > + }
> > +
> > + context->priv = priv;
> > + context->echo_index = i;
> > + context->dlc = cf->can_dlc;
> > +
> > + msg->u.tx_can.tid = context->echo_index;
> > +
> > + usb_fill_bulk_urb(urb, dev->udev,
> > + usb_sndbulkpipe(dev->udev,
> > + dev->bulk_out->bEndpointAddress),
> > + buf, msg->len,
> > + kvaser_usb_write_bulk_callback, context);
> > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > + usb_anchor_urb(urb, &priv->tx_submitted);
> > +
> > + can_put_echo_skb(skb, netdev, context->echo_index);
> > +
> > + atomic_inc(&priv->active_tx_urbs);
> > +
> > + if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
> > + netif_stop_queue(netdev);
> > +
> > + err = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (unlikely(err)) {
> > + can_free_echo_skb(netdev, context->echo_index);
> > +
> > + atomic_dec(&priv->active_tx_urbs);
> > + usb_unanchor_urb(urb);
> > +
> > + stats->tx_dropped++;
> > +
> > + if (err == -ENODEV)
> > + netif_device_detach(netdev);
> > + else
> > + netdev_warn(netdev, "Failed tx_urb %d\n", err);
> > +
> > + goto releasebuf;
> > + }
> > +
> > + netdev->trans_start = jiffies;
> > +
> > + usb_free_urb(urb);
> > +
> > + return NETDEV_TX_OK;
> > +
> > +releasebuf:
> > + usb_free_coherent(dev->udev, sizeof(struct kvaser_msg),
> > + buf, urb->transfer_dma);
> > +nobufmem:
> > + usb_free_urb(urb);
> > + return ret;
> > +}
>
> > +static int kvaser_usb_init_one(struct usb_interface *intf,
> > + const struct usb_device_id *id, int channel)
> > +{
> > + struct kvaser_usb *dev = usb_get_intfdata(intf);
> > + struct net_device *netdev;
> > + struct kvaser_usb_net_priv *priv;
> > + int i, err;
> > +
> > + netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> > + if (!netdev) {
> > + dev_err(&intf->dev, "Cannot alloc candev\n");
> > + return -ENOMEM;
> > + }
> > +
> > + priv = netdev_priv(netdev);
> > +
> > + init_completion(&priv->start_comp);
> > + init_completion(&priv->stop_comp);
> > +
> > + init_usb_anchor(&priv->tx_submitted);
> > + atomic_set(&priv->active_tx_urbs, 0);
> > +
> > + for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
> > + priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> > +
> > + priv->dev = dev;
> > + priv->netdev = netdev;
> > + priv->channel = channel;
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > + priv->can.clock.freq = CAN_USB_CLOCK;
> > + priv->can.bittiming_const = &kvaser_usb_bittiming_const;
> > + priv->can.do_set_bittiming = kvaser_usb_set_bittiming;
> > + priv->can.do_set_mode = kvaser_usb_set_mode;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> > + if (id->driver_info & KVASER_HAS_SILENT_MODE)
> > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
> > +
> > + netdev->flags |= IFF_ECHO;
> > +
> > + netdev->netdev_ops = &kvaser_usb_netdev_ops;
> > +
> > + SET_NETDEV_DEV(netdev, &intf->dev);
> > +
> > + err = register_candev(netdev);
> > + if (err) {
> > + dev_err(&intf->dev, "Failed to register can device\n");
> > + free_candev(netdev);
> > + return err;
> > + }
> > +
>
> Here the device is usable.
>
> > + dev->nets[channel] = priv;
>
> And only know you init this field. Looks like a race condition.
Argh... Indeed.
Thanks
>
> > + netdev_dbg(netdev, "device registered\n");
> > +
> > + return 0;
> > +}
> > +
>
> > +static void kvaser_usb_disconnect(struct usb_interface *intf)
> > +{
> > + struct kvaser_usb *dev = usb_get_intfdata(intf);
> > + int i;
> > +
> > + usb_set_intfdata(intf, NULL);
> > +
> > + if (!dev)
> > + return;
> > +
> > + for (i = 0; i < dev->nchannels; i++) {
> > + if (!dev->nets[i])
> > + continue;
> > +
> > + unregister_netdev(dev->nets[i]->netdev);
> > + free_candev(dev->nets[i]->netdev);
> > + }
> > +
> > + kvaser_usb_unlink_all_urbs(dev);
>
> So what happens if an URB completes between freeing the candev
> and unlinking and proceeds to push data to upper layers?
In this case I should avoid using netdev context, i.e. pointer to "struct
kvaser_usb_net_priv" in urb callbacks.
Is that what you mean?
I see it is well done in kvaser_usb_read_bulk_callback()
but not in kvaser_usb_write_bulk_callback().
I'll better protect the callbacks and move the free_candev after the
unlink.
Thank you,
--
Olivier
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: David Miller @ 2012-08-08 5:55 UTC (permalink / raw)
To: B32167; +Cc: netdev
In-Reply-To: <AC83832D6324604BB08FAE5A33553F1907A96EA6@039-SN2MPN1-012.039d.mgd.msft.net>
From: Jain Priyanka-B32167 <B32167@freescale.com>
Date: Wed, 8 Aug 2012 04:53:42 +0000
>
>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, August 08, 2012 4:52 AM
> To: Jain Priyanka-B32167
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
>
> From: Priyanka Jain <Priyanka.Jain@freescale.com>
> Date: Tue, 7 Aug 2012 10:51:44 +0530
>
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>>
>> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>
> This patch doesn't apply to the net-next tree, please respin.
> [Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
>
> Also:
>
>> - xfrm_policy_afinfo[afinfo->family] = NULL;
>> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
>> + NULL);
>
> Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
> [Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
> Please let me know your final say on this. I will make changes accordingly if required.
What in the world are you talking about?
The openning parenthesis of the rcu_assign_pointer() statement is not anywhere
close to the 80th column.
You're doing this:
x(A,
B);
and I want you to do this:
x(A,
B);
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-08 6:04 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford
In-Reply-To: <871ujjfkb1.fsf@xmission.com>
Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Ali Ayoub <ali@mellanox.com> writes:
>> Among other things, the main benefit we're targeting is to allow IPoE
>> traffic within the VM to go through the (Ethernet) vBridge down to the
>> eIPoIB PIF, and eventually to IPoIB and to the IB network.
> Oh yes. It just occurred to me there is huge problem with eIPoIB as
> currently presented in these patches. It breaks DHCPv4 the same way
> it breaks ARP, but DHCPv4 is not fixed up.
To put things in place, DHCPv4 is supported with eIPoIB, the DHCP
UDP/IP payload isn't touched, only need to set the BOOTP broadcast
flag in the dhcp server config file.
Or.
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie @ 2012-08-08 6:14 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: Marc Kleine-Budde, linux-can, linux-usb, netdev
In-Reply-To: <5020B51E.8060600@grandegger.com>
Hi Wolfgang,
On Tue, Aug 07, 2012 at 08:26:38AM +0200, Wolfgang Grandegger wrote:
> On 08/06/2012 07:21 AM, Olivier Sobrie wrote:
> > This driver provides support for several Kvaser CAN/USB devices.
> > Such kind of devices supports up to three can network interfaces.
>
> s/can/CAN/
>
> > It has been tested with a Kvaser USB Leaf Light (one network interface)
> > connected to a pch_can interface.
> > The firmware version of the Kvaser device was 2.5.205.
> >
> > List of Kvaser devices supported by the driver:
> > - Kvaser Leaf prototype (P010v2 and v3)
> > - Kvaser Leaf Light (P010v3)
> > - Kvaser Leaf Professional HS
> > - Kvaser Leaf SemiPro HS
> > - Kvaser Leaf Professional LS
> > - Kvaser Leaf Professional SWC
> > - Kvaser Leaf Professional LIN
> > - Kvaser Leaf SemiPro LS
> > - Kvaser Leaf SemiPro SWC
> > - Kvaser Memorator II, Prototype
> > - Kvaser Memorator II HS/HS
> > - Kvaser USBcan Professional HS/HS
> > - Kvaser Leaf Light GI
> > - Kvaser Leaf Professional HS (OBD-II connector)
> > - Kvaser Memorator Professional HS/LS
> > - Kvaser Leaf Light "China"
> > - Kvaser BlackBird SemiPro
> > - Kvaser OEM Mercury
> > - Kvaser OEM Leaf
> > - Kvaser USBcan R
>
> Impressive list! What CAN controllers are used inside the devices? SJA1000?
I took this list from the Kvaser driver. However I only have a Kvaser
Leaf Light device thus I'm not sure it will work with other ones.
If you prefer I can only let Kvaser Leaf Light instead of the full list.
In my device it looks to be a Renesas M16C controller.
>
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> > Changes since v1:
> > - added copyrights
> > - kvaser_usb.h merged into kvaser.c
> > - added kvaser_usb_get_endpoints to find eindpoints instead of
> > hardcoding their address
> > - some cleanup and comestic changes
> > - fixed issues with errors handling
> > - fixed restart-ms == 0 case
> > - removed do_get_berr_counter method since the hardware doens't return
> > good values for txerr and rxerr.
> >
> > If someone in the linux-usb mailing can review it, it would be nice.
> >
> > Concerning the errors, it behaves like that now:
> >
> > 1) Short-circuit CAN-H and CAN-L and restart-ms = 0
> >
> > t0: short-circuit + 'cansend can1 123#112233'
> >
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-off
> > bus-error
> >
> > t1: remove short-circuit + 'ip link set can1 type can restart'
> >
> > can1 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
> > restarted-after-bus-off
> > can1 20000004 [8] 00 0C 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
>
> Why do we get the last error message? Maybe the firmware does it that
> way (going down passive->warning->active).
It goes in that order: warning -> passive -> bus off -> warning
-> passive -> ...
>
> > 2) Short-circuit CAN-H and CAN-L and restart-ms = 100
> >
> > t0: short-circuit + cansend can1 123#112233
> >
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-off
> > bus-error
> > can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > restarted-after-bus-off
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-off
> > bus-error
> > ...
> >
> > can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > restarted-after-bus-off
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> >
> > t1: remove short-circuit
> >
> > can1 123 [3] 11 22 33
> > can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{back-to-error-active}{}}
>
> The order is still inverted but likely the firmware is doing it that way.
Indeed the firmware does it that way: it sends the acknwledge of the
frame beofre the state change. I can avoid that behavior by checking the
state in the acknowledge frame and send the restart frame if the bus was
off.
>
> > 3) CAN-H and CAN-L disconnected
> >
> > t0: CAN-H and CAN-L disconnected + cansend can1 123#112233
> >
> > can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > can1 2000008C [8] 00 30 80 1B 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{error-on-tx}{acknowledge-delimiter}}
> > bus-error
> >
> > t1: CAN-H and CAN-L reconnected
> >
> > can1 123 [3] 11 22 33
> > can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
>
> Why do we get an error-passive message? Now I will have a closer look to
> the code...
The firmware sends a CMD_CAN_ERROR_EVENT with the passive bit set...
> > +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct net_device_stats *stats;
> > + struct kvaser_usb_net_priv *priv;
> > + unsigned int new_state;
> > + u8 channel, status;
> > +
> > + if (msg->id == CMD_CAN_ERROR_EVENT) {
> > + channel = msg->u.error_event.channel;
> > + status = msg->u.error_event.status;
> > + } else {
> > + channel = msg->u.chip_state_event.channel;
> > + status = msg->u.chip_state_event.status;
> > + }
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > + stats = &priv->netdev->stats;
> > +
> > + if (status & M16C_STATE_BUS_RESET) {
> > + kvaser_usb_unlink_tx_urbs(priv);
> > + return;
> > + }
> > + skb = alloc_can_err_skb(priv->netdev, &cf);
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
>
> Cleanup? kvaser_usb_unlink_tx_urbs()?
If I get the M16C_STATE_BUS_RESET I'll not receive the ack frames anymore.
I need to set the context->echo_index back to MAX_TX_URBS to not loose tx
urbs.
By the way I think a can_free_echo_skb() is missing...
>
> > + }
> > +
> > + if (status & M16C_STATE_BUS_OFF) {
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > +
> > + if (!priv->can.restart_ms)
> > + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> > +
> > + if (!priv->can.state != CAN_ERR_BUSOFF) {
> > + priv->can.can_stats.bus_off++;
> > + netif_carrier_off(priv->netdev);
> > + }
> > +
> > + new_state = CAN_STATE_BUS_OFF;
> > + } else if (status & M16C_STATE_BUS_PASSIVE) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
> > + CAN_ERR_CRTL_RX_PASSIVE;
>
> State changes should only be report when the state really changes.
> Therefore it should go under the if block below.
Ok. Is it possible to get such sequence:
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 10 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
2 questions:
1) If the bus is still in CAN_ERR_CRTL_RX_PASSIVE after the second frame,
shouldn't we let the corresponding bit set, 0x10?
2) Can we send multiple times same frame wih same error bits set?
>
> > + if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> > + priv->can.can_stats.error_passive++;
> > +
> > + new_state = CAN_STATE_ERROR_PASSIVE;
> > + } else if (status & M16C_STATE_BUS_ERROR) {
>
> Hm, strange, a bus error is not a state change. You use here if...else
> if... Isn't it possible that more than one bit is set.
Indeed it is possible to have multiple bits set.
e.g. M16C_STATE_BUS_PASSIVE + M16C_STATE_BUS_ERROR or M16C_STATE_BUS_OFF + M16C_STATE_BUS_ERROR.
What error should I report in case of M16C_STATE_BUS_ERROR?
>
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = CAN_ERR_CRTL_TX_WARNING |
> > + CAN_ERR_CRTL_RX_WARNING;
>
> See above.
>
> > + if (priv->can.state != CAN_STATE_ERROR_WARNING)
> > + priv->can.can_stats.error_warning++;
> > +
> > + new_state = CAN_STATE_ERROR_WARNING;
> > + } else {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_ACTIVE;
> > +
> > + new_state = CAN_STATE_ERROR_ACTIVE;
> > + }
> > +
> > + if (priv->can.restart_ms &&
> > + (priv->can.state >= CAN_STATE_BUS_OFF) &&
> > + (new_state < CAN_STATE_BUS_OFF)) {
> > + cf->can_id |= CAN_ERR_RESTARTED;
> > + priv->can.can_stats.restarts++;
> > + netif_carrier_on(priv->netdev);
> > + }
> > +
> > + if (msg->id == CMD_CAN_ERROR_EVENT) {
> > + u8 error_factor = msg->u.error_event.error_factor;
> > +
> > + priv->can.can_stats.bus_error++;
> > + stats->rx_errors++;
> > +
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > + if (error_factor & M16C_EF_ACKE)
> > + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
> > + CAN_ERR_PROT_LOC_ACK_DEL);
> > + if (error_factor & M16C_EF_CRCE)
> > + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL);
> > + if (error_factor & M16C_EF_FORME)
> > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > + if (error_factor & M16C_EF_STFE)
> > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > + if (error_factor & M16C_EF_BITE0)
> > + cf->data[2] |= CAN_ERR_PROT_BIT0;
> > + if (error_factor & M16C_EF_BITE1)
> > + cf->data[2] |= CAN_ERR_PROT_BIT1;
> > + if (error_factor & M16C_EF_TRE)
> > + cf->data[2] |= CAN_ERR_PROT_TX;
> > + }
> > +
> > + priv->can.state = new_state;
> > +
> > + if (!memcmp(cf, &priv->cf_err_old, sizeof(*cf))) {
> > + kfree_skb(skb);
> > + return;
> > + }
>
> Hm, the firmware seems not to clear error conditions? Anyway, state
> change and error reporting is magic on many CAN controllers. Just the
> SJA1000 is doing it nicely.
I added it to prevent sending two times the same error. It happens that
the firmware sends multiple times the same error message.
>
> > + netif_rx(skb);
> > +
> > + priv->cf_err_old = *cf;
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
> > +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct kvaser_usb_net_priv *priv;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct net_device_stats *stats;
> > + u8 channel = msg->u.rx_can.channel;
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > + stats = &priv->netdev->stats;
> > +
> > + skb = alloc_can_skb(priv->netdev, &cf);
> > + if (skb == NULL) {
>
> s/skb == NULL)/!skb/ ?
>
> > + stats->tx_dropped++;
> > + return;
> > + }
> > +
> > + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
> > + (msg->u.rx_can.msg[1] & 0x3f);
> > + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
> > +
> > + if (msg->id == CMD_RX_EXT_MESSAGE) {
> > + cf->can_id <<= 18;
> > + cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
> > + ((msg->u.rx_can.msg[3] & 0xff) << 6) |
> > + (msg->u.rx_can.msg[4] & 0x3f);
> > + cf->can_id |= CAN_EFF_FLAG;
> > + }
> > +
> > + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> > + MSG_FLAG_NERR)) {
> > + cf->can_id |= CAN_ERR_FLAG;
> > + cf->can_dlc = CAN_ERR_DLC;
> > + cf->data[1] = CAN_ERR_CRTL_UNSPEC;
>
> What is the meaning of such errors? A comment, netdev_err() or
> netdev_dbg() would be nice.
I never reached this error with the hardware I've... I don't know what's
the meaning of this flag... I will add a trace.
>
> > +
> > + stats->rx_errors++;
> > + } else if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> > + cf->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> > + cf->can_dlc = CAN_ERR_DLC;
> > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > +
> > + stats->rx_over_errors++;
> > + stats->rx_errors++;
> > + } else if (!msg->u.rx_can.flag) {
> > + memcpy(cf->data, &msg->u.rx_can.msg[6], cf->can_dlc);
> > + } else {
> > + kfree_skb(skb);
> > + return;
> > + }
> > +
> > + netif_rx(skb);
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
I'll fix all others small things you mentionned.
Thank you!
--
Olivier
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Eric Dumazet @ 2012-08-08 6:25 UTC (permalink / raw)
To: Priyanka Jain; +Cc: netdev
In-Reply-To: <1344316904-2544-1-git-send-email-Priyanka.Jain@freescale.com>
On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:
> xfrm_policy_afinfo is read mosly data structure.
> Write on xfrm_policy_afinfo is done only at the
> time of configuration.
> So rwlocks can be safely replaced with RCU.
>
> RCUs usage optimizes the performance.
> static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
> @@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
> struct xfrm_policy_afinfo *afinfo;
> if (unlikely(family >= NPROTO))
> return NULL;
> - read_lock(&xfrm_policy_afinfo_lock);
> - afinfo = xfrm_policy_afinfo[family];
> + rcu_read_lock();
> + afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
> if (unlikely(!afinfo))
> - read_unlock(&xfrm_policy_afinfo_lock);
> + rcu_read_unlock();
This makes no sense to me : We cant safely return afinfo here.
Note the current code is buggy as well, this is worrying.
As soon as we exit from xfrm_policy_get_afinfo(), pointer might be
invalid.
Really, RCU conversion should be the right moment to spot those bugs and
first fix them (for stable trees)
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Fan Du @ 2012-08-08 6:44 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Priyanka Jain, netdev
In-Reply-To: <1344407147.28967.212.camel@edumazet-glaptop>
First, sorry to jump in.
On 2012年08月08日 14:25, Eric Dumazet wrote:
> On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the
>> time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>
>
>> static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
>> @@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
>> struct xfrm_policy_afinfo *afinfo;
>> if (unlikely(family>= NPROTO))
>> return NULL;
>> - read_lock(&xfrm_policy_afinfo_lock);
>> - afinfo = xfrm_policy_afinfo[family];
>> + rcu_read_lock();
>> + afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
>> if (unlikely(!afinfo))
>> - read_unlock(&xfrm_policy_afinfo_lock);
>> + rcu_read_unlock();
>
> This makes no sense to me : We cant safely return afinfo here.
>
> Note the current code is buggy as well, this is worrying.
>
> As soon as we exit from xfrm_policy_get_afinfo(), pointer might be
> invalid.
>
Yes, it might be invalid, but all the callers have checked the return
value, thus use it in a sane way.
So I don't follow "Note the current code is buggy as well".
Am I missing something here?
> Really, RCU conversion should be the right moment to spot those bugs and
> first fix them (for stable trees)
>
>
>
> --
> 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
>
--
Love each day!
--fan
^ permalink raw reply
* Re: [PATCH] cdc-phonet: Don't leak in usbpn_open
From: Rémi Denis-Courmont @ 2012-08-08 7:12 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel, netdev, linux-usb, Greg Kroah-Hartman
In-Reply-To: <alpine.LNX.2.00.1208072354030.3227@swampdragon.chaosbits.net>
Le mercredi 8 août 2012 00:56:26 Jesper Juhl, vous avez écrit :
> We allocate memory for 'req' with usb_alloc_urb() and then test
> 'if (!req || rx_submit(pnd, req, GFP_KERNEL | __GFP_COLD))'.
> If we enter that branch due to '!req' then there is no problem. But if
> we enter the branch due to 'req' being != 0 and the 'rx_submit()' call
> being false, then we'll leak the memory we allocated.
> Deal with the leak by always calling 'usb_free_urb(req)' when entering
> the branch. If 'req' happens to be 0 then the call is harmless, if it
> is not 0 then we free the memory we allocated but don't need.
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
Acked-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
> drivers/net/usb/cdc-phonet.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Only compile tested due to lack of hardware.
Hardware won't help you much with testing the error case anyway.
--
Rémi Denis-Courmont, looking for a job
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-08 7:32 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford, Michael S. Tsirkin
In-Reply-To: <87obmnfs4p.fsf@xmission.com>
Eric W. Biederman <ebiederm@xmission.com> wrote:
> Ali Ayoub <ali@mellanox.com> writes:
[...]
>> I don't see in other alternatives a solution for the problem we're
>> trying to solve. If there are changes/suggestions to improve eIPoIB
>> netdev driver to avoid "messing with the link layer" and make it
>> acceptable, we can discuss and apply them.
> Nothing needs to be applied the code is done. Routing from
> IPoE to IPoIB works. There is nothing in what anyone has posted as requirements
> that needs work to implement.
> I totally fail to see how getting packets of of the VM as ethernet
> frames, and then IP layer routing those packets over IP is not an
> option. What requirement am I missing.
As you've indicated routing w/w.o using proxy-arp is an option, however,
> All VMs should suport that mode of operation, and certainly the kernel does.
> Implementations involving bridges like macvlan and macvtap are
> performance optimizations, and the optimizations don't even apply in
> areas like 802.11, where only one mac address is supported per adapter.
> Bridging can ocassionally also be an administrative simplification as
> well, but you should be able to achieve the a similar simplification
> with a dhcprelay and proxy arp.
as you wrote here, when performance and ease-of-use is under the spot,
VM deployments tend to not to use routing.
This is b/c it involves more over-head on the packet forwarding, and
more administration work, for example, for setting routing rules that
involve the VM IP address, something which AFAIK the hypervisor have
no clue on, also its unclear to me if/how live migration can work in
such setting.
>From this exact reason, there's a bunch of use-cases by tools and
cloud stacks (such as open stack, ovirt, more) which do use bridged
mode and the rest of the Ethernet envelope, such as using virtual L2
vlan domains, ebtables based rules, etc etc. Where they and are not
application to ipoib, but are working file ith eipoib.
You mentioned that bridging mode doesn't apply to environment such as
802.11, and hence routing mode is used, we are trying to make a point
here that bridging mode applies to ipoib with the approach suggested
by eipoib.
Also, if we extend the discussion a bit, there are two more aspects to throw in:
The first is the performance thing we have already started to mention
-- specifically, the approach for RX zero copy (into the VM buffer),
use designs such as vhost + macvtap NIC in passthrough mode which is
likey to be set over a per VM hypervisor NIC, e.g such as the ones
provided by VMDQ patches John Fastabend started to post (see
http://marc.info/?l=linux-netdev&m=134264998405581&w=2) -- the ib0.N
clone child are IPoIB VMDQ NICs if you like, and setting an eipoib NIC
on top of each they can be plugged to that design.
The 2nd aspect, is NON VM environments where a NIC with Ethernet look
and feel is required for IP traffic, but this have to live within an
echo-system that fully uses IPoIB.
In other words, a use case where IPoIB has to be below the cover for
set of some specific apps, or nodes but do IP interaction with other
apps/nodes and gateways who use IPoIB, the eIPoIB driver provides that
functionality.
So, to sum up, routing / proxy-arp seems to be off where we are targeting.
Or.
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Arnd Bergmann @ 2012-08-08 8:16 UTC (permalink / raw)
To: David Miller
Cc: ian.molton, linux-arm-kernel, andrew, thomas.petazzoni, ben.dooks,
netdev
In-Reply-To: <20120807.162923.34400427265666163.davem@davemloft.net>
On Tuesday 07 August 2012, David Miller wrote:
> From: Ian Molton <ian.molton@codethink.co.uk>
> Date: Tue, 7 Aug 2012 15:34:45 +0100
>
> > Fixed all comments.
> >
> > * Dropped csb1724 defconfig.
> > * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
> > * Dropped un-necessary D-T irq fixup code
>
> Who is going to take this series?
I'd prefer to take the entire series through the arm-soc tree from
the kirkwood maintainers. We first have to work out the bindings
though, since the current patch introduces a new one that is
incompatible with the one we were using on powerpc with
open firmware before.
Arnd
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Wolfgang Grandegger @ 2012-08-08 8:25 UTC (permalink / raw)
To: Olivier Sobrie; +Cc: Marc Kleine-Budde, linux-can, linux-usb, netdev
In-Reply-To: <20120808061419.GA16678@hposo>
Hi Oliver,
On 08/08/2012 08:14 AM, Olivier Sobrie wrote:
> Hi Wolfgang,
>
> On Tue, Aug 07, 2012 at 08:26:38AM +0200, Wolfgang Grandegger wrote:
>> On 08/06/2012 07:21 AM, Olivier Sobrie wrote:
>>> This driver provides support for several Kvaser CAN/USB devices.
>>> Such kind of devices supports up to three can network interfaces.
>>
>> s/can/CAN/
>>
>>> It has been tested with a Kvaser USB Leaf Light (one network interface)
>>> connected to a pch_can interface.
>>> The firmware version of the Kvaser device was 2.5.205.
>>>
>>> List of Kvaser devices supported by the driver:
>>> - Kvaser Leaf prototype (P010v2 and v3)
>>> - Kvaser Leaf Light (P010v3)
>>> - Kvaser Leaf Professional HS
>>> - Kvaser Leaf SemiPro HS
>>> - Kvaser Leaf Professional LS
>>> - Kvaser Leaf Professional SWC
>>> - Kvaser Leaf Professional LIN
>>> - Kvaser Leaf SemiPro LS
>>> - Kvaser Leaf SemiPro SWC
>>> - Kvaser Memorator II, Prototype
>>> - Kvaser Memorator II HS/HS
>>> - Kvaser USBcan Professional HS/HS
>>> - Kvaser Leaf Light GI
>>> - Kvaser Leaf Professional HS (OBD-II connector)
>>> - Kvaser Memorator Professional HS/LS
>>> - Kvaser Leaf Light "China"
>>> - Kvaser BlackBird SemiPro
>>> - Kvaser OEM Mercury
>>> - Kvaser OEM Leaf
>>> - Kvaser USBcan R
>>
>> Impressive list! What CAN controllers are used inside the devices? SJA1000?
>
> I took this list from the Kvaser driver. However I only have a Kvaser
> Leaf Light device thus I'm not sure it will work with other ones.
> If you prefer I can only let Kvaser Leaf Light instead of the full list.
> In my device it looks to be a Renesas M16C controller.
OK. Checking the manual, if available, could help to understand how the
firmware handles bus errors and state changes.
>>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>>> ---
>>> Changes since v1:
>>> - added copyrights
>>> - kvaser_usb.h merged into kvaser.c
>>> - added kvaser_usb_get_endpoints to find eindpoints instead of
>>> hardcoding their address
>>> - some cleanup and comestic changes
>>> - fixed issues with errors handling
>>> - fixed restart-ms == 0 case
>>> - removed do_get_berr_counter method since the hardware doens't return
>>> good values for txerr and rxerr.
>>>
>>> If someone in the linux-usb mailing can review it, it would be nice.
>>>
>>> Concerning the errors, it behaves like that now:
>>>
>>> 1) Short-circuit CAN-H and CAN-L and restart-ms = 0
>>>
>>> t0: short-circuit + 'cansend can1 123#112233'
>>>
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-off
>>> bus-error
>>>
>>> t1: remove short-circuit + 'ip link set can1 type can restart'
>>>
>>> can1 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
>>> restarted-after-bus-off
>>> can1 20000004 [8] 00 0C 00 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>
>> Why do we get the last error message? Maybe the firmware does it that
>> way (going down passive->warning->active).
>
> It goes in that order: warning -> passive -> bus off -> warning
> -> passive -> ...
Just for curiosity? You don't see back to "error active"?
>>> 2) Short-circuit CAN-H and CAN-L and restart-ms = 100
>>>
>>> t0: short-circuit + cansend can1 123#112233
>>>
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-off
>>> bus-error
>>> can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> restarted-after-bus-off
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-off
>>> bus-error
>>> ...
>>>
>>> can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> restarted-after-bus-off
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>>
>>> t1: remove short-circuit
>>>
>>> can1 123 [3] 11 22 33
>>> can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{back-to-error-active}{}}
>>
>> The order is still inverted but likely the firmware is doing it that way.
>
> Indeed the firmware does it that way: it sends the acknwledge of the
> frame beofre the state change. I can avoid that behavior by checking the
> state in the acknowledge frame and send the restart frame if the bus was
> off.
Well, if the firmware does it wrong, I would not really care. Also,
could you use timestamping to see if they come close together.
>>> 3) CAN-H and CAN-L disconnected
>>>
>>> t0: CAN-H and CAN-L disconnected + cansend can1 123#112233
>>>
>>> can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> can1 2000008C [8] 00 30 80 1B 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{error-on-tx}{acknowledge-delimiter}}
>>> bus-error
>>>
>>> t1: CAN-H and CAN-L reconnected
>>>
>>> can1 123 [3] 11 22 33
>>> can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>
>> Why do we get an error-passive message? Now I will have a closer look to
>> the code...
>
> The firmware sends a CMD_CAN_ERROR_EVENT with the passive bit set...
Maybe the order is again inverted. Do they come at the same time
(visiable with candump -td).
>>> +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct net_device_stats *stats;
>>> + struct kvaser_usb_net_priv *priv;
>>> + unsigned int new_state;
>>> + u8 channel, status;
>>> +
>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>> + channel = msg->u.error_event.channel;
>>> + status = msg->u.error_event.status;
>>> + } else {
>>> + channel = msg->u.chip_state_event.channel;
>>> + status = msg->u.chip_state_event.status;
>>> + }
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> + stats = &priv->netdev->stats;
>>> +
>>> + if (status & M16C_STATE_BUS_RESET) {
>>> + kvaser_usb_unlink_tx_urbs(priv);
>>> + return;
>>> + }
>>> + skb = alloc_can_err_skb(priv->netdev, &cf);
>>> + if (!skb) {
>>> + stats->rx_dropped++;
>>> + return;
>>
>> Cleanup? kvaser_usb_unlink_tx_urbs()?
>
> If I get the M16C_STATE_BUS_RESET I'll not receive the ack frames anymore.
> I need to set the context->echo_index back to MAX_TX_URBS to not loose tx
> urbs.
> By the way I think a can_free_echo_skb() is missing...
>
>>
>>> + }
>>> +
>>> + if (status & M16C_STATE_BUS_OFF) {
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> +
>>> + if (!priv->can.restart_ms)
>>> + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
>>> +
>>> + if (!priv->can.state != CAN_ERR_BUSOFF) {
>>> + priv->can.can_stats.bus_off++;
>>> + netif_carrier_off(priv->netdev);
>>> + }
>>> +
>>> + new_state = CAN_STATE_BUS_OFF;
>>> + } else if (status & M16C_STATE_BUS_PASSIVE) {
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
>>> + CAN_ERR_CRTL_RX_PASSIVE;
>>
>> State changes should only be report when the state really changes.
>> Therefore it should go under the if block below.
>
> Ok. Is it possible to get such sequence:
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>
> 2 questions:
> 1) If the bus is still in CAN_ERR_CRTL_RX_PASSIVE after the second frame,
> shouldn't we let the corresponding bit set, 0x10?
No, see below.
> 2) Can we send multiple times same frame wih same error bits set?
Yes, because that's what the hardware reports.
>>
>>> + if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
>>> + priv->can.can_stats.error_passive++;
>>> +
>>> + new_state = CAN_STATE_ERROR_PASSIVE;
>>> + } else if (status & M16C_STATE_BUS_ERROR) {
>>
>> Hm, strange, a bus error is not a state change. You use here if...else
>> if... Isn't it possible that more than one bit is set.
>
> Indeed it is possible to have multiple bits set.
> e.g. M16C_STATE_BUS_PASSIVE + M16C_STATE_BUS_ERROR or M16C_STATE_BUS_OFF + M16C_STATE_BUS_ERROR.
OK, that's the normal behaviour. Obviously they send bus errors together
with the *actual* state. The hardware does usuallly report bus errors
frequently while the error condition persists.
> What error should I report in case of M16C_STATE_BUS_ERROR?
To make that clear, I have added an (old) output from the SJA1000, which
is the defacto reference. Bus error reporting is enabled and no cable is
connected. Watch the TX error count increasing and how the state changes:
$ ./candump -e 0xffff any
can0 20000088 [8] 00 00 80 19 00 08 00 00 ERRORFRAME
\ \ \-- ACK slot.
\ \-- error occured on transmission
\-- Bus-error | Protocol violations (data[2], data[3]).
can0 20000088 [8] 00 00 80 19 00 10 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 18 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 20 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 28 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 30 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 38 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 40 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 48 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 50 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 58 00 00 ERRORFRAME
can0 2000008C [8] 00 08 80 19 00 60 00 00 ERRORFRAME
\ \-- reached warning level for TX errors
\-- | Controller problems (see data[1]).
can0 20000088 [8] 00 00 80 19 00 68 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 70 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 78 00 00 ERRORFRAME
can0 2000008C [8] 00 20 80 19 00 80 00 00 ERRORFRAME
\ \-- reached passive level for TX errors
\-- | Controller problems (see data[1]).
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
\ \-- RXerror count
\-- TXerror count
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
...
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
>
>>
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_TX_WARNING |
>>> + CAN_ERR_CRTL_RX_WARNING;
>>
>> See above.
>>
>>> + if (priv->can.state != CAN_STATE_ERROR_WARNING)
>>> + priv->can.can_stats.error_warning++;
>>> +
>>> + new_state = CAN_STATE_ERROR_WARNING;
>>> + } else {
>>> + cf->can_id |= CAN_ERR_PROT;
>>> + cf->data[2] = CAN_ERR_PROT_ACTIVE;
>>> +
>>> + new_state = CAN_STATE_ERROR_ACTIVE;
>>> + }
>>> +
>>> + if (priv->can.restart_ms &&
>>> + (priv->can.state >= CAN_STATE_BUS_OFF) &&
>>> + (new_state < CAN_STATE_BUS_OFF)) {
>>> + cf->can_id |= CAN_ERR_RESTARTED;
>>> + priv->can.can_stats.restarts++;
>>> + netif_carrier_on(priv->netdev);
>>> + }
>>> +
>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>> + u8 error_factor = msg->u.error_event.error_factor;
>>> +
>>> + priv->can.can_stats.bus_error++;
>>> + stats->rx_errors++;
>>> +
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +
>>> + if (error_factor & M16C_EF_ACKE)
>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
>>> + CAN_ERR_PROT_LOC_ACK_DEL);
>>> + if (error_factor & M16C_EF_CRCE)
>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
>>> + CAN_ERR_PROT_LOC_CRC_DEL);
>>> + if (error_factor & M16C_EF_FORME)
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + if (error_factor & M16C_EF_STFE)
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + if (error_factor & M16C_EF_BITE0)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT0;
>>> + if (error_factor & M16C_EF_BITE1)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT1;
>>> + if (error_factor & M16C_EF_TRE)
>>> + cf->data[2] |= CAN_ERR_PROT_TX;
>>> + }
>>> +
>>> + priv->can.state = new_state;
>>> +
>>> + if (!memcmp(cf, &priv->cf_err_old, sizeof(*cf))) {
>>> + kfree_skb(skb);
>>> + return;
>>> + }
>>
>> Hm, the firmware seems not to clear error conditions? Anyway, state
>> change and error reporting is magic on many CAN controllers. Just the
>> SJA1000 is doing it nicely.
>
> I added it to prevent sending two times the same error. It happens that
> the firmware sends multiple times the same error message.
Can then be removed, I think, see above.
>>
>>> + netif_rx(skb);
>>> +
>>> + priv->cf_err_old = *cf;
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +}
>>> +
>>> +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct kvaser_usb_net_priv *priv;
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct net_device_stats *stats;
>>> + u8 channel = msg->u.rx_can.channel;
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> + stats = &priv->netdev->stats;
>>> +
>>> + skb = alloc_can_skb(priv->netdev, &cf);
>>> + if (skb == NULL) {
>>
>> s/skb == NULL)/!skb/ ?
>>
>>> + stats->tx_dropped++;
>>> + return;
>>> + }
>>> +
>>> + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
>>> + (msg->u.rx_can.msg[1] & 0x3f);
>>> + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
>>> +
>>> + if (msg->id == CMD_RX_EXT_MESSAGE) {
>>> + cf->can_id <<= 18;
>>> + cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
>>> + ((msg->u.rx_can.msg[3] & 0xff) << 6) |
>>> + (msg->u.rx_can.msg[4] & 0x3f);
>>> + cf->can_id |= CAN_EFF_FLAG;
>>> + }
>>> +
>>> + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
>>> + cf->can_id |= CAN_RTR_FLAG;
>>> + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
>>> + MSG_FLAG_NERR)) {
>>> + cf->can_id |= CAN_ERR_FLAG;
>>> + cf->can_dlc = CAN_ERR_DLC;
>>> + cf->data[1] = CAN_ERR_CRTL_UNSPEC;
>>
>> What is the meaning of such errors? A comment, netdev_err() or
>> netdev_dbg() would be nice.
>
> I never reached this error with the hardware I've... I don't know what's
> the meaning of this flag... I will add a trace.
Then add a netdev_err().
Wolfgang.
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Eric W. Biederman @ 2012-08-08 8:36 UTC (permalink / raw)
To: Or Gerlitz
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford
In-Reply-To: <CAJZOPZ+W0wfa38LejXt0gvBNa8Q6pWVZOs7xctHGRQXHAeMYPQ@mail.gmail.com>
Or Gerlitz <or.gerlitz@gmail.com> writes:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> Ali Ayoub <ali@mellanox.com> writes:
>
>>> Among other things, the main benefit we're targeting is to allow IPoE
>>> traffic within the VM to go through the (Ethernet) vBridge down to the
>>> eIPoIB PIF, and eventually to IPoIB and to the IB network.
>
>> Oh yes. It just occurred to me there is huge problem with eIPoIB as
>> currently presented in these patches. It breaks DHCPv4 the same way
>> it breaks ARP, but DHCPv4 is not fixed up.
>
> To put things in place, DHCPv4 is supported with eIPoIB, the DHCP
> UDP/IP payload isn't touched, only need to set the BOOTP broadcast
> flag in the dhcp server config file.
Wrong. DHCPv4 is broken over eIPoIB.
Coming from ethernet
htype == 1 not 32 as required by RFC4390
hlen == 6 not 0 as required by RFC4390
The chaddr field is has 6 bytes of the ethernet mac address not the
required 16 bytes of 0.
The client-identifier field is optional over ethernet.
An ethernet DHCPv4 client simply does not generate a dhcp packet that
conforms to RFC4390.
Therefore DHCPv4 over eIPoIB is broken, and a dhcp server or relay
may reasonably look at the DHCP packet and drop it because it is
garbage.
You might find a forgiving dhcp server that doesn't drop insane packets
on the floor and tries to make things work.
I am sorry. eIPoIB is broken as designed. eIPoIB most assuredly is not
compatible with ethernet. eIPoIB most definitely does not work even for
the general case of transporing IP traffic. Claiming that eIPoIB is any
else is a lie.
Eric
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: David Miller @ 2012-08-08 8:59 UTC (permalink / raw)
To: arnd
Cc: ian.molton, linux-arm-kernel, andrew, thomas.petazzoni, ben.dooks,
netdev
In-Reply-To: <201208080816.29218.arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 8 Aug 2012 08:16:28 +0000
> On Tuesday 07 August 2012, David Miller wrote:
>> From: Ian Molton <ian.molton@codethink.co.uk>
>> Date: Tue, 7 Aug 2012 15:34:45 +0100
>>
>> > Fixed all comments.
>> >
>> > * Dropped csb1724 defconfig.
>> > * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
>> > * Dropped un-necessary D-T irq fixup code
>>
>> Who is going to take this series?
>
> I'd prefer to take the entire series through the arm-soc tree from
> the kirkwood maintainers. We first have to work out the bindings
> though, since the current patch introduces a new one that is
> incompatible with the one we were using on powerpc with
> open firmware before.
Ok.
^ permalink raw reply
* Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
From: Pavel Emelyanov @ 2012-08-08 9:00 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Miller, Eric Dumazet, Eric W. Biederman, Linux Netdev List
In-Reply-To: <1344364573.2688.13.camel@bwh-desktop.uk.solarflarecom.com>
On 08/07/2012 10:36 PM, Ben Hutchings wrote:
> On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
>> The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
>> so take the given ifidex and register netdev with it.
>>
>> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
>> ---
>> drivers/net/veth.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index 5852361..496c026 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>> if (tbp[IFLA_ADDRESS] == NULL)
>> eth_hw_addr_random(peer);
>>
>> + if (ifmp)
>> + peer->ifindex = ifmp->ifi_index;
>> +
>> err = register_netdevice(peer);
>> put_net(net);
>> net = NULL;
>
> Is this safe, given that this code path previously ignored
> ifmp->ifi_index? Userland could be passing in garbage and may now fail
> occasionally because the value clashes with an existing interface.
You're right, I've missed that fact :( The good news is that we still can
use the ifmp->ifi_index for the peer index configuration. We just need to
assume that if the caller specified the ifindex for the veth master device,
then it's aware of this possibility and should explicitly configure (or set
to 0) the peer's ifindex as well. Like this:
if (ifmp && (dev->ifindex != 0))
peer->ifindex = ifmp->ifi_index;
Does this assumption work from you POV?
> Ben.
>
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Eric W. Biederman @ 2012-08-08 9:17 UTC (permalink / raw)
To: Or Gerlitz
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford, Michael S. Tsirkin
In-Reply-To: <CAJZOPZLApfvgd1wrM2HseNrWh-egaixjhGfB7xBJU1FxFhBdzg@mail.gmail.com>
Or Gerlitz <or.gerlitz@gmail.com> writes:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Ali Ayoub <ali@mellanox.com> writes:
> [...]
>>> I don't see in other alternatives a solution for the problem we're
>>> trying to solve. If there are changes/suggestions to improve eIPoIB
>>> netdev driver to avoid "messing with the link layer" and make it
>>> acceptable, we can discuss and apply them.
>
>> Nothing needs to be applied the code is done. Routing from
>> IPoE to IPoIB works. There is nothing in what anyone has posted as requirements
>> that needs work to implement.
>
>> I totally fail to see how getting packets of of the VM as ethernet
>> frames, and then IP layer routing those packets over IP is not an
>> option. What requirement am I missing.
>
>
> As you've indicated routing w/w.o using proxy-arp is an option, however,
>
>> All VMs should suport that mode of operation, and certainly the kernel does.
>> Implementations involving bridges like macvlan and macvtap are
>> performance optimizations, and the optimizations don't even apply in
>> areas like 802.11, where only one mac address is supported per adapter.
>> Bridging can ocassionally also be an administrative simplification as
>> well, but you should be able to achieve the a similar simplification
>> with a dhcprelay and proxy arp.
>
> as you wrote here, when performance and ease-of-use is under the spot,
> VM deployments tend to not to use routing.
>
> This is b/c it involves more over-head on the packet forwarding, and
> more administration work, for example, for setting routing rules that
> involve the VM IP address, something which AFAIK the hypervisor have
> no clue on, also its unclear to me if/how live migration can work in
> such setting.
All you need to make proxy-arp essentially pain free is a smart dhcp
relay, that sets up the routes.
> From this exact reason, there's a bunch of use-cases by tools and
> cloud stacks (such as open stack, ovirt, more) which do use bridged
> mode and the rest of the Ethernet envelope, such as using virtual L2
> vlan domains, ebtables based rules, etc etc. Where they and are not
> application to ipoib, but are working file ith eipoib.
Yes I am certain all of their IPv6 traffic works fine.
Regardless those are open source projects and can be modified to add
support to cleanly support inifinibnad.
> You mentioned that bridging mode doesn't apply to environment such as
> 802.11, and hence routing mode is used, we are trying to make a pointn
> here that bridging mode applies to ipoib with the approach suggested
> by eipoib.
You are completely failing. Every time I look I see something about
eIPoIB that is even more broken. Given that eIPoIB is a NAT
implementation that isn't really a surprise but still.
eIPoIB imposes enough overhead that I expect that routing is cheaper,
so your performance advantges go right out the window.
eIPoIB is seriously incompatible with ethernet breaking almost
everything and barely allowing IPv4 to work.
> Also, if we extend the discussion a bit, there are two more aspects to throw in:
>
> The first is the performance thing we have already started to mention
> -- specifically, the approach for RX zero copy (into the VM buffer),
> use designs such as vhost + macvtap NIC in passthrough mode which is
> likey to be set over a per VM hypervisor NIC, e.g such as the ones
> provided by VMDQ patches John Fastabend started to post (see
> http://marc.info/?l=linux-netdev&m=134264998405581&w=2) -- the ib0.N
> clone child are IPoIB VMDQ NICs if you like, and setting an eipoib NIC
> on top of each they can be plugged to that design.
If you care about performance link-layer NAT is not the way to go.
Teach the pieces you care about how to talk infiniband.
> The 2nd aspect, is NON VM environments where a NIC with Ethernet look
> and feel is required for IP traffic, but this have to live within an
> echo-system that fully uses IPoIB.
> In other words, a use case where IPoIB has to be below the cover for
> set of some specific apps, or nodes but do IP interaction with other
> apps/nodes and gateways who use IPoIB, the eIPoIB driver provides that
> functionality.
ip link add type dummy.
There now you have an interface with ethernet look and feel, and
routing can happily avoid it.
> So, to sum up, routing / proxy-arp seems to be off where we are
> targeting.
My condolences.
The existence of router / proxy-arp means that solutions do exist
(unlike your previous claim) you just don't like the idea of deploying
them.
Infiniband is standard enough you could quite easily implement virtual
infiniband bridging as an alternative to ethernet bridging.
At this stage of the game eIPoIB is interesting the same way a zombie is
interesting. It is fascinating to see it still moving as chunks of
flesh fall to the floor removing any doubt that it is dead.
Eric
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-08 9:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Miller, linux-arm-kernel, andrew, thomas.petazzoni,
ben.dooks, netdev
In-Reply-To: <201208080816.29218.arnd@arndb.de>
On 08/08/12 09:16, Arnd Bergmann wrote:
> I'd prefer to take the entire series through the arm-soc tree from
> the kirkwood maintainers. We first have to work out the bindings
> though, since the current patch introduces a new one that is
> incompatible with the one we were using on powerpc with open firmware
> before.
Looking at the ethernet-group stuff, specifically from
arch/powerpc/boot/dts/prpmc2800.dts, which I've taken as a base for the
below:
I think we can (and should) do something similar.
Sadly, there is no code present to describe marvell,mv64360-mdio,
however the device tree looks basically sane.
mdio@2000 {
#address-cells = <1>;
#size-cells = <0>;
device_type = "mdio";
compatible = "marvell,mv643xx-mdio";
PHY0: ethernet-phy@1 {
device_type = "ethernet-phy";
compatible = "broadcom,bcm5421";
interrupts = <76>; /* GPP 12 */
interrupt-parent = <&PIC>;
reg = <1>;
};
PHY1: ethernet-phy@3 {
device_type = "ethernet-phy";
compatible = "broadcom,bcm5421";
interrupts = <76>; /* GPP 12 */
interrupt-parent = <&PIC>;
reg = <3>;
};
};
ethernet-group@2400 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "marvell,mv64360-eth-group";
reg = <0x2400 0x2000>;
ethernet@0 {
device_type = "network";
compatible = "marvell,mv64360-eth";
reg = <0>;
interrupts = <32>;
interrupt-parent = <&mpic>;
phy = <&phy0>;
local-mac-address = [ 00 00 00 00 00 00 ];
};
ethernet@1 {
device_type = "network";
compatible = "marvell,mv64360-eth";
reg = <1>;
interrupts = <33>;
interrupt-parent = <&mpic>;
phy = <&phy1>;
local-mac-address = [ 00 00 00 00 00 00 ];
};
};
^ permalink raw reply
* RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Jain Priyanka-B32167 @ 2012-08-08 9:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20120807.225531.618417929491214091.davem@davemloft.net>
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, August 08, 2012 11:26 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Jain Priyanka-B32167 <B32167@freescale.com>
Date: Wed, 8 Aug 2012 04:53:42 +0000
>
>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, August 08, 2012 4:52 AM
> To: Jain Priyanka-B32167
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with
> rcu
>
> From: Priyanka Jain <Priyanka.Jain@freescale.com>
> Date: Tue, 7 Aug 2012 10:51:44 +0530
>
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>>
>> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>
> This patch doesn't apply to the net-next tree, please respin.
> [Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
>
> Also:
>
>> - xfrm_policy_afinfo[afinfo->family] = NULL;
>> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
>> + NULL);
>
> Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
> [Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
> Please let me know your final say on this. I will make changes accordingly if required.
What in the world are you talking about?
The openning parenthesis of the rcu_assign_pointer() statement is not anywhere close to the 80th column.
You're doing this:
x(A,
B);
and I want you to do this:
x(A,
B);
[Priyanka] Got it. Thanks. Will correct this.
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-08 9:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Miller, linux-arm-kernel, andrew, thomas.petazzoni,
ben.dooks, netdev
In-Reply-To: <50223428.6030506@codethink.co.uk>
Ignore, Hit send whilst editing, by mistake. Sorry for the noise guys.
-Ian
^ 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