* 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
* [PATCH] net/core: Fix potential memory leak in dev_set_alias()
From: Alexey Khoroshilov @ 2012-08-08 10:33 UTC (permalink / raw)
To: David S. Miller
Cc: Alexey Khoroshilov, Eric Dumazet, netdev, linux-kernel,
ldv-project
Do not leak memory by updating pointer with potentially NULL realloc return value.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
net/core/dev.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 0cb3fe8..3bcc5da 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1055,6 +1055,8 @@ rollback:
*/
int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
{
+ char *new_ifalias;
+
ASSERT_RTNL();
if (len >= IFALIASZ)
@@ -1068,9 +1070,10 @@ int dev_set_alias(struct net_device *dev, const char *alias, size_t len)
return 0;
}
- dev->ifalias = krealloc(dev->ifalias, len + 1, GFP_KERNEL);
- if (!dev->ifalias)
+ new_ifalias = krealloc(dev->ifalias, len + 1, GFP_KERNEL);
+ if (!new_ifalias)
return -ENOMEM;
+ dev->ifalias = new_ifalias;
strlcpy(dev->ifalias, alias, len+1);
return len;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver
From: Alan Cox @ 2012-08-08 10:46 UTC (permalink / raw)
To: Arun MURTHY
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, gregkh@linuxfoundation.org,
Sjur BRENDELAND
In-Reply-To: <F45880696056844FA6A73F415B568C695B0E7969E3@EXDCVYMBSTM006.EQ1STM.local>
> 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.
Ok. What happens about endianness or is the modem always the same
endianness as the host ?
> > > + if (len <= 0)
> > > + return -EFAULT;
> >
> > How can this occur ?
>
> Check for error condition
So how can it occur ? The kernel char layer will never pass a negative
length to a driver ?
> > 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.
What stops a hostile application (or programmer error) from doing so
deliberately ?
> > > + 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.
Well buf = NULL is not an error (its weird but its not an error)
Also length < 0 is never passed from the char layer to a driver.
> > > + 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.
So why use atomic. Also you can't make that assumption. If you need your
device to have one user per channel and one write call at a time you must
enforce it. There is nothing wrong with enforcing it but it needs to be
done.
That means your open path probably wants to do something (locked) like
if (foo->users)
return -EBUSY;
you still then need to use a mutex or similar in read and write because a
single open can pass to multiple processes (or multiple writes/reads
occur at once in a multi-threaded app).
User/Kernel is the security boundary so the kernel code must be robust
against a hostile user rather than assuming a correctly functioning
library.
I suspect you simply need to wrap the read/write logic (except for a wait
for new message) with a mutex and all will be well
> > > + 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.
So what occurs if this modem reset happens between that test and the next
line. You have no locking on it so you've got no guarantee that it won't
reset during the test. So it covers the initial set up case but not a
reset.
It may not matter providing a reset wakes up things and it is handled
later. It just looks suspicious.
> > > + isadev = &isa_context->isadev[idx];
> > > + if (filp != NULL)
> > > + filp->private_data = isadev;
> >
> > How can filp be NULL ?
>
> :-) just a error condition check
These tests are not useful, if anything they hide bugs. If you have a
real reason to check (eg its a complicated internal path) then use
WARN_ON(condition)
or
BUG_ON(condition)
so it gets noticed. For core kernel things however there is no point
checking. If the kernel ever passes you null as a file pointer the game
is already over.
> > > + 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.
You've not yet set up the isa_context but yes.. looks like it is covered
by the boot check.
(A good rule of thumb is btw to initialise everything, then register
stuff)
Alan
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-08 11:51 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>
On 08/08/12 10:40, Ian Molton wrote:
> 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:
The SMI / PHY stuff should look very similar, so I'm happy with something
like:
mdio@2000 {
#address-cells = <1>;
#size-cells = <1>;
device_type = "mdio";
compatible = "marvell,mv643xx-mdio";
phy0: ethernet-phy@0 {
device_type = "ethernet-phy";
compatible = "marvell,whatever";
interrupts = <76>;
interrupt-parent = <&mpic>;
reg = <0 32>; // Auto probed phy addr
};
phy1: ethernet-phy@3 {
device_type = "ethernet-phy";
compatible = "marvell,whatever";
interrupts = <77>;
interrupt-parent = <&mpic>;
reg = <3 1>; // specified phy addr
};
... and so on.
}
Where we can use the reg parameter to allow auto-probing, by
specifying a size of 32 (32 phy addrs max).
The ethernet driver itself is more complicated:
We have the following considerations:
* we have one MDIO bus, typically, shared between all the MACs / PHYs.
* each ethernet device can multiple ports (up to three), each with its
own MAC/PHY.
* MAC <-> PHY mapping can be specified, probed (ugh!) or a (gah!)
mix of the two.
* existing D-T users, albeit not well documented / code complete.
* some port address ranges overlap (MIB counters, MCAST / UNICAST
tables, etc.
The existing ethernet-group idea only works because the current
platform-device based driver doesnt really do proper resource
management, and thus the MAC registers are actually mapped by
the MDIO driver.
I don't think that preserving this bad behaviour is a good idea, which
leaves us with two choices:
1) My preferred solution - allow each device to specify up to three
interrupts, MACs, and PHYs. This is clean in that it doesnt require
multiply instantiating a driver three times over the same address
space.
ethernet@2400 {
compatible = "marvell,mv643xx-eth";
reg = <0x2400 0x1c00>
interrupt_parent = <&mpic>;
ports = <3>;
interrupts = <4>, <5>, <6>;
phys = <&phy0>, <&phy1>, <&phy2>;
};
ethernet@6400 {
compatible = "marvell,mv643xx-eth";
reg = <0x6400 0x1c00>
interrupt_parent = <&mpic>;
ports = <1>;
interrupts = <4>;
phys = <&phy3>;
};
Note that the address is 2400, not 2000 - since this driver no longer
would share its address range with the MDIO driver.
This method would require a small amount of rework in the driver to
set up <n> ports, rather than just one.
2) Create some kind of pseudo-ethernet group device that manages
all the work for some sort of lightweight ethernet device, one per
port. This can never be done cleanly since the port address ranges
overlap:
pseudo_eth@2400 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "marvell,mv643xx-shared-eth"
reg = <0x2400 0x1c00>;
ethernet@0 {
compatible = "marvell,mv643xx-port";
interrupts = <4>;
interrupt_parent = <&mpic>;
phy = <&phy0>;
};
ethernet@1 {
compatible = "marvell,mv643xx-port";
interrupts = <5>;
interrupt_parent = <&mpic>;
phy = <&phy1>;
};
ethernet@2 {
compatible = "marvell,mv643xx-port";
interrupts = <6>;
interrupt_parent = <&mpic>;
phy = <&phy2>;
};
}
pseudo_eth@6400 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "marvell,mv643xx-shared-eth"
reg = <0x6400 0x1c00>;
ethernet@0 {
compatible = "marvell,mv643xx-port";
interrupts = <4>;
interrupt_parent = <&mpic>;
phy = <&phy3>;
};
};
Thoughts?
-Ian
^ permalink raw reply
* [RFC net-next 1/4] gianfar: Remove redundant programming of [rt]xic registers
From: Claudiu Manoil @ 2012-08-08 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Paul Gortmaker, Claudiu Manoil
In-Reply-To: <1344428810-29923-1-git-send-email-claudiu.manoil@freescale.com>
In Multi Q Multi Group (MQ_MG_MODE) mode, the Rx/Tx colescing registers [rt]xic
are aliased with the [rt]xic0 registers (coalescing setting regs for Q0). This
avoids programming twice in a row the coalescing registers for the Rx/Tx hw Q0.
Also, replaced inconsistent "unlikely" in the process.
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
drivers/net/ethernet/freescale/gianfar.c | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 4605f72..e9feeb9 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1799,20 +1799,9 @@ void gfar_configure_coalescing(struct gfar_private *priv,
{
struct gfar __iomem *regs = priv->gfargrp[0].regs;
u32 __iomem *baddr;
- int i = 0;
-
- /* Backward compatible case ---- even if we enable
- * multiple queues, there's only single reg to program
- */
- gfar_write(®s->txic, 0);
- if (likely(priv->tx_queue[0]->txcoalescing))
- gfar_write(®s->txic, priv->tx_queue[0]->txic);
-
- gfar_write(®s->rxic, 0);
- if (unlikely(priv->rx_queue[0]->rxcoalescing))
- gfar_write(®s->rxic, priv->rx_queue[0]->rxic);
if (priv->mode == MQ_MG_MODE) {
+ int i;
baddr = ®s->txic0;
for_each_set_bit(i, &tx_mask, priv->num_tx_queues) {
gfar_write(baddr + i, 0);
@@ -1826,6 +1815,17 @@ void gfar_configure_coalescing(struct gfar_private *priv,
if (likely(priv->rx_queue[i]->rxcoalescing))
gfar_write(baddr + i, priv->rx_queue[i]->rxic);
}
+ } else {
+ /* Backward compatible case ---- even if we enable
+ * multiple queues, there's only single reg to program
+ */
+ gfar_write(®s->txic, 0);
+ if (likely(priv->tx_queue[0]->txcoalescing))
+ gfar_write(®s->txic, priv->tx_queue[0]->txic);
+
+ gfar_write(®s->rxic, 0);
+ if (likely(priv->rx_queue[0]->rxcoalescing))
+ gfar_write(®s->rxic, priv->rx_queue[0]->rxic);
}
}
--
1.6.6
^ permalink raw reply related
* [RFC net-next 0/4] gianfar: Use separate NAPI for Tx confirmation processing
From: Claudiu Manoil @ 2012-08-08 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Paul Gortmaker
Hi all,
This set of patches basically splits the existing napi poll routine into
two separate napi functions, one for Rx processing (triggered by frame
receive interrupts only) and one for the Tx confirmation path processing
(triggerred by Tx confirmation interrupts only). The polling algorithm
behind remains much the same.
Important throughput improvements have been noted on low power boards with
this set of changes.
For instance, for the following netperf test:
netperf -l 20 -cC -H 192.168.10.1 -t TCP_STREAM -- -m 1500
yields a throughput gain from oscilating ~500-~700 Mbps to steady ~940 Mbps,
(if the Rx/Tx paths are processed on different cores), w/ no increase in CPU%,
on a p1020rdb - 2 core machine featuring etsec2.0 (Multi-Queue Multi-Group
driver mode).
Also, this change, which should ballance Rx and Tx processing, proves to
be effective against Rx busy interrupt occurrences.
Thanks for your review.
Claudiu
Claudiu Manoil (4):
gianfar: Remove redundant programming of [rt]xic registers
gianfar: Clear ievent from interrupt handler for [RT]x int
gianfar: Separate out the Rx and Tx coalescing functions
gianfar: Use separate NAPIs for Tx and Rx processing
drivers/net/ethernet/freescale/gianfar.c | 220 +++++++++++++++++++++--------
drivers/net/ethernet/freescale/gianfar.h | 16 ++-
2 files changed, 171 insertions(+), 65 deletions(-)
^ permalink raw reply
* [RFC net-next 3/4] gianfar: Separate out the Rx and Tx coalescing functions
From: Claudiu Manoil @ 2012-08-08 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Paul Gortmaker, Claudiu Manoil
In-Reply-To: <1344428810-29923-3-git-send-email-claudiu.manoil@freescale.com>
Split the coalescing programming support by Rx and Tx h/w queues, in order to
introduce a separate NAPI for the Tx confirmation path (next patch). This way,
the Rx processing path will handle the coalescing settings for the Rx queues
only, resp. the Tx confirmation processing path will handle the Tx queues.
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
drivers/net/ethernet/freescale/gianfar.c | 36 +++++++++++++++++++++++------
1 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index ddd350a..919acb3 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1794,8 +1794,8 @@ void gfar_start(struct net_device *dev)
dev->trans_start = jiffies; /* prevent tx timeout */
}
-void gfar_configure_coalescing(struct gfar_private *priv,
- unsigned long tx_mask, unsigned long rx_mask)
+static inline void gfar_configure_tx_coalescing(struct gfar_private *priv,
+ unsigned long mask)
{
struct gfar __iomem *regs = priv->gfargrp[0].regs;
u32 __iomem *baddr;
@@ -1803,14 +1803,31 @@ void gfar_configure_coalescing(struct gfar_private *priv,
if (priv->mode == MQ_MG_MODE) {
int i;
baddr = ®s->txic0;
- for_each_set_bit(i, &tx_mask, priv->num_tx_queues) {
+ for_each_set_bit(i, &mask, priv->num_tx_queues) {
gfar_write(baddr + i, 0);
if (likely(priv->tx_queue[i]->txcoalescing))
gfar_write(baddr + i, priv->tx_queue[i]->txic);
}
+ } else {
+ /* Backward compatible case ---- even if we enable
+ * multiple queues, there's only single reg to program
+ */
+ gfar_write(®s->txic, 0);
+ if (likely(priv->tx_queue[0]->txcoalescing))
+ gfar_write(®s->txic, priv->tx_queue[0]->txic);
+ }
+}
+
+static inline void gfar_configure_rx_coalescing(struct gfar_private *priv,
+ unsigned long mask)
+{
+ struct gfar __iomem *regs = priv->gfargrp[0].regs;
+ u32 __iomem *baddr;
+ if (priv->mode == MQ_MG_MODE) {
+ int i;
baddr = ®s->rxic0;
- for_each_set_bit(i, &rx_mask, priv->num_rx_queues) {
+ for_each_set_bit(i, &mask, priv->num_rx_queues) {
gfar_write(baddr + i, 0);
if (likely(priv->rx_queue[i]->rxcoalescing))
gfar_write(baddr + i, priv->rx_queue[i]->rxic);
@@ -1819,16 +1836,19 @@ void gfar_configure_coalescing(struct gfar_private *priv,
/* Backward compatible case ---- even if we enable
* multiple queues, there's only single reg to program
*/
- gfar_write(®s->txic, 0);
- if (likely(priv->tx_queue[0]->txcoalescing))
- gfar_write(®s->txic, priv->tx_queue[0]->txic);
-
gfar_write(®s->rxic, 0);
if (likely(priv->rx_queue[0]->rxcoalescing))
gfar_write(®s->rxic, priv->rx_queue[0]->rxic);
}
}
+void gfar_configure_coalescing(struct gfar_private *priv,
+ unsigned long tx_mask, unsigned long rx_mask)
+{
+ gfar_configure_tx_coalescing(priv, tx_mask);
+ gfar_configure_rx_coalescing(priv, rx_mask);
+}
+
static int register_grp_irqs(struct gfar_priv_grp *grp)
{
struct gfar_private *priv = grp->priv;
--
1.6.6
^ permalink raw reply related
* [RFC net-next 2/4] gianfar: Clear ievent from interrupt handler for [RT]x int
From: Claudiu Manoil @ 2012-08-08 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Paul Gortmaker, Claudiu Manoil
In-Reply-To: <1344428810-29923-2-git-send-email-claudiu.manoil@freescale.com>
It's the interrupt handler's job to clear ievent for the Tx/Rx paths, as soon
as the corresponding interrupt sources have been masked.
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
drivers/net/ethernet/freescale/gianfar.c | 16 ++++++----------
1 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index e9feeb9..ddd350a 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2568,12 +2568,13 @@ static void gfar_schedule_cleanup(struct gfar_priv_grp *gfargrp)
if (napi_schedule_prep(&gfargrp->napi)) {
gfar_write(&gfargrp->regs->imask, IMASK_RTX_DISABLED);
__napi_schedule(&gfargrp->napi);
- } else {
- /* Clear IEVENT, so interrupts aren't called again
- * because of the packets that have already arrived.
- */
- gfar_write(&gfargrp->regs->ievent, IEVENT_RTX_MASK);
}
+
+ /* Clear IEVENT, so interrupts aren't called again
+ * because of the packets that have already arrived.
+ */
+ gfar_write(&gfargrp->regs->ievent, IEVENT_RTX_MASK);
+
spin_unlock_irqrestore(&gfargrp->grplock, flags);
}
@@ -2837,11 +2838,6 @@ static int gfar_poll(struct napi_struct *napi, int budget)
num_queues = gfargrp->num_rx_queues;
budget_per_queue = budget/num_queues;
- /* Clear IEVENT, so interrupts aren't called again
- * because of the packets that have already arrived
- */
- gfar_write(®s->ievent, IEVENT_RTX_MASK);
-
while (num_queues && left_over_budget) {
budget_per_queue = left_over_budget/num_queues;
left_over_budget = 0;
--
1.6.6
^ permalink raw reply related
* [RFC net-next 4/4] gianfar: Use separate NAPIs for Tx and Rx processing
From: Claudiu Manoil @ 2012-08-08 12:26 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Paul Gortmaker, Pankaj Chauhan, Claudiu Manoil
In-Reply-To: <1344428810-29923-4-git-send-email-claudiu.manoil@freescale.com>
Add a separate napi poll routine for Tx cleanup, to be triggerred by Tx
confirmation interrupts only. Existing poll function is modified to handle
only the Rx path processing. This allows parallel processing of Rx and Tx
confirmation paths on a smp machine (2 cores).
The split also results in simpler/cleaner napi poll function implementations,
where each processing path has its own budget, thus improving the fairness b/w
the processing paths at the same time.
Signed-off-by: Pankaj Chauhan <pankaj.chauhan@freescale.com>
Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
---
drivers/net/ethernet/freescale/gianfar.c | 154 +++++++++++++++++++++++-------
drivers/net/ethernet/freescale/gianfar.h | 16 +++-
2 files changed, 130 insertions(+), 40 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 919acb3..2774961 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -128,12 +128,14 @@ static void free_skb_resources(struct gfar_private *priv);
static void gfar_set_multi(struct net_device *dev);
static void gfar_set_hash_for_addr(struct net_device *dev, u8 *addr);
static void gfar_configure_serdes(struct net_device *dev);
-static int gfar_poll(struct napi_struct *napi, int budget);
+static int gfar_poll_rx(struct napi_struct *napi, int budget);
+static int gfar_poll_tx(struct napi_struct *napi, int budget);
#ifdef CONFIG_NET_POLL_CONTROLLER
static void gfar_netpoll(struct net_device *dev);
#endif
int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit);
-static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue);
+static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue,
+ int tx_work_limit);
static int gfar_process_frame(struct net_device *dev, struct sk_buff *skb,
int amount_pull, struct napi_struct *napi);
void gfar_halt(struct net_device *dev);
@@ -543,16 +545,20 @@ static void disable_napi(struct gfar_private *priv)
{
int i;
- for (i = 0; i < priv->num_grps; i++)
- napi_disable(&priv->gfargrp[i].napi);
+ for (i = 0; i < priv->num_grps; i++) {
+ napi_disable(&priv->gfargrp[i].napi_rx);
+ napi_disable(&priv->gfargrp[i].napi_tx);
+ }
}
static void enable_napi(struct gfar_private *priv)
{
int i;
- for (i = 0; i < priv->num_grps; i++)
- napi_enable(&priv->gfargrp[i].napi);
+ for (i = 0; i < priv->num_grps; i++) {
+ napi_enable(&priv->gfargrp[i].napi_rx);
+ napi_enable(&priv->gfargrp[i].napi_tx);
+ }
}
static int gfar_parse_group(struct device_node *np,
@@ -1028,9 +1034,12 @@ static int gfar_probe(struct platform_device *ofdev)
dev->ethtool_ops = &gfar_ethtool_ops;
/* Register for napi ...We are registering NAPI for each grp */
- for (i = 0; i < priv->num_grps; i++)
- netif_napi_add(dev, &priv->gfargrp[i].napi, gfar_poll,
- GFAR_DEV_WEIGHT);
+ for (i = 0; i < priv->num_grps; i++) {
+ netif_napi_add(dev, &priv->gfargrp[i].napi_rx, gfar_poll_rx,
+ GFAR_DEV_RX_WEIGHT);
+ netif_napi_add(dev, &priv->gfargrp[i].napi_tx, gfar_poll_tx,
+ GFAR_DEV_TX_WEIGHT);
+ }
if (priv->device_flags & FSL_GIANFAR_DEV_HAS_CSUM) {
dev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG |
@@ -2465,7 +2474,8 @@ static void gfar_align_skb(struct sk_buff *skb)
}
/* Interrupt Handler for Transmit complete */
-static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
+static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue,
+ int tx_work_limit)
{
struct net_device *dev = tx_queue->dev;
struct netdev_queue *txq;
@@ -2490,7 +2500,7 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
bdp = tx_queue->dirty_tx;
skb_dirtytx = tx_queue->skb_dirtytx;
- while ((skb = tx_queue->tx_skbuff[skb_dirtytx])) {
+ while ((skb = tx_queue->tx_skbuff[skb_dirtytx]) && tx_work_limit--) {
unsigned long flags;
frags = skb_shinfo(skb)->nr_frags;
@@ -2580,29 +2590,50 @@ static int gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue)
return howmany;
}
-static void gfar_schedule_cleanup(struct gfar_priv_grp *gfargrp)
+static void gfar_schedule_rx_cleanup(struct gfar_priv_grp *gfargrp)
{
unsigned long flags;
- spin_lock_irqsave(&gfargrp->grplock, flags);
- if (napi_schedule_prep(&gfargrp->napi)) {
- gfar_write(&gfargrp->regs->imask, IMASK_RTX_DISABLED);
- __napi_schedule(&gfargrp->napi);
+ if (napi_schedule_prep(&gfargrp->napi_rx)) {
+ u32 imask;
+ spin_lock_irqsave(&gfargrp->grplock, flags);
+ imask = gfar_read(&gfargrp->regs->imask);
+ imask &= ~(IMASK_RX_DEFAULT);
+ gfar_write(&gfargrp->regs->imask, imask);
+ __napi_schedule(&gfargrp->napi_rx);
+ spin_unlock_irqrestore(&gfargrp->grplock, flags);
}
/* Clear IEVENT, so interrupts aren't called again
* because of the packets that have already arrived.
*/
- gfar_write(&gfargrp->regs->ievent, IEVENT_RTX_MASK);
+ gfar_write(&gfargrp->regs->ievent, IEVENT_RX_MASK);
+}
- spin_unlock_irqrestore(&gfargrp->grplock, flags);
+static void gfar_schedule_tx_cleanup(struct gfar_priv_grp *gfargrp)
+{
+ unsigned long flags;
+
+ if (napi_schedule_prep(&gfargrp->napi_tx)) {
+ u32 imask;
+ spin_lock_irqsave(&gfargrp->grplock, flags);
+ imask = gfar_read(&gfargrp->regs->imask);
+ imask &= ~(IMASK_TX_DEFAULT);
+ gfar_write(&gfargrp->regs->imask, imask);
+ __napi_schedule(&gfargrp->napi_tx);
+ spin_unlock_irqrestore(&gfargrp->grplock, flags);
+ }
+ /* Clear IEVENT, so interrupts aren't called again
+ * because of the packets that have already arrived.
+ */
+ gfar_write(&gfargrp->regs->ievent, IEVENT_TX_MASK);
}
/* Interrupt Handler for Transmit complete */
static irqreturn_t gfar_transmit(int irq, void *grp_id)
{
- gfar_schedule_cleanup((struct gfar_priv_grp *)grp_id);
+ gfar_schedule_tx_cleanup((struct gfar_priv_grp *)grp_id);
return IRQ_HANDLED;
}
@@ -2683,7 +2714,7 @@ static inline void count_errors(unsigned short status, struct net_device *dev)
irqreturn_t gfar_receive(int irq, void *grp_id)
{
- gfar_schedule_cleanup((struct gfar_priv_grp *)grp_id);
+ gfar_schedule_rx_cleanup((struct gfar_priv_grp *)grp_id);
return IRQ_HANDLED;
}
@@ -2813,7 +2844,7 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
rx_queue->stats.rx_bytes += pkt_len;
skb_record_rx_queue(skb, rx_queue->qindex);
gfar_process_frame(dev, skb, amount_pull,
- &rx_queue->grp->napi);
+ &rx_queue->grp->napi_rx);
} else {
netif_warn(priv, rx_err, dev, "Missing skb!\n");
@@ -2842,21 +2873,19 @@ int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
return howmany;
}
-static int gfar_poll(struct napi_struct *napi, int budget)
+static int gfar_poll_rx(struct napi_struct *napi, int budget)
{
struct gfar_priv_grp *gfargrp =
- container_of(napi, struct gfar_priv_grp, napi);
+ container_of(napi, struct gfar_priv_grp, napi_rx);
struct gfar_private *priv = gfargrp->priv;
struct gfar __iomem *regs = gfargrp->regs;
- struct gfar_priv_tx_q *tx_queue = NULL;
struct gfar_priv_rx_q *rx_queue = NULL;
- int rx_cleaned = 0, budget_per_queue = 0, rx_cleaned_per_queue = 0;
- int tx_cleaned = 0, i, left_over_budget = budget;
+ int rx_cleaned = 0, budget_per_queue, rx_cleaned_per_queue;
+ int i, left_over_budget = budget;
unsigned long serviced_queues = 0;
int num_queues = 0;
num_queues = gfargrp->num_rx_queues;
- budget_per_queue = budget/num_queues;
while (num_queues && left_over_budget) {
budget_per_queue = left_over_budget/num_queues;
@@ -2866,9 +2895,6 @@ static int gfar_poll(struct napi_struct *napi, int budget)
if (test_bit(i, &serviced_queues))
continue;
rx_queue = priv->rx_queue[i];
- tx_queue = priv->tx_queue[rx_queue->qindex];
-
- tx_cleaned += gfar_clean_tx_ring(tx_queue);
rx_cleaned_per_queue =
gfar_clean_rx_ring(rx_queue, budget_per_queue);
rx_cleaned += rx_cleaned_per_queue;
@@ -2882,27 +2908,83 @@ static int gfar_poll(struct napi_struct *napi, int budget)
}
}
- if (tx_cleaned)
- return budget;
-
if (rx_cleaned < budget) {
+ u32 imask;
napi_complete(napi);
/* Clear the halt bit in RSTAT */
gfar_write(®s->rstat, gfargrp->rstat);
- gfar_write(®s->imask, IMASK_DEFAULT);
+ spin_lock_irq(&gfargrp->grplock);
+ imask = gfar_read(®s->imask);
+ imask |= IMASK_RX_DEFAULT;
+ gfar_write(®s->imask, imask);
+ spin_unlock_irq(&gfargrp->grplock);
/* If we are coalescing interrupts, update the timer
* Otherwise, clear it
*/
- gfar_configure_coalescing(priv, gfargrp->rx_bit_map,
- gfargrp->tx_bit_map);
+ gfar_configure_rx_coalescing(priv, gfargrp->rx_bit_map);
}
return rx_cleaned;
}
+static int gfar_poll_tx(struct napi_struct *napi, int budget)
+{
+ struct gfar_priv_grp *gfargrp =
+ container_of(napi, struct gfar_priv_grp, napi_tx);
+ struct gfar_private *priv = gfargrp->priv;
+ struct gfar __iomem *regs = gfargrp->regs;
+ struct gfar_priv_tx_q *tx_queue = NULL;
+ int tx_cleaned = 0, budget_per_queue, tx_cleaned_per_queue;
+ int i, left_over_budget = budget;
+ unsigned long serviced_queues = 0;
+ int num_queues = 0;
+
+ num_queues = gfargrp->num_tx_queues;
+
+ while (num_queues && left_over_budget) {
+ budget_per_queue = left_over_budget/num_queues;
+ left_over_budget = 0;
+
+ for_each_set_bit(i, &gfargrp->tx_bit_map, priv->num_tx_queues) {
+ if (test_bit(i, &serviced_queues))
+ continue;
+ tx_queue = priv->tx_queue[i];
+ tx_cleaned_per_queue =
+ gfar_clean_tx_ring(tx_queue, budget_per_queue);
+ tx_cleaned += tx_cleaned_per_queue;
+ if (tx_cleaned_per_queue < budget_per_queue) {
+ left_over_budget = left_over_budget +
+ (budget_per_queue -
+ tx_cleaned_per_queue);
+ set_bit(i, &serviced_queues);
+ num_queues--;
+ }
+ }
+ }
+
+ if (tx_cleaned < budget) {
+ u32 imask;
+ napi_complete(napi);
+
+ gfar_write(®s->imask, IMASK_DEFAULT);
+ spin_lock_irq(&gfargrp->grplock);
+ imask = gfar_read(®s->imask);
+ imask |= IMASK_TX_DEFAULT;
+ gfar_write(®s->imask, imask);
+ spin_unlock_irq(&gfargrp->grplock);
+
+ /* If we are coalescing interrupts, update the timer
+ * Otherwise, clear it
+ */
+ gfar_configure_tx_coalescing(priv, gfargrp->tx_bit_map);
+ }
+
+ return tx_cleaned;
+}
+
#ifdef CONFIG_NET_POLL_CONTROLLER
/* Polling 'interrupt' - used by things like netconsole to send skbs
* without having to re-enable interrupts. It's not called while
diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h
index 2136c7f..f5be234 100644
--- a/drivers/net/ethernet/freescale/gianfar.h
+++ b/drivers/net/ethernet/freescale/gianfar.h
@@ -57,8 +57,10 @@ struct ethtool_rx_list {
unsigned int count;
};
-/* The maximum number of packets to be handled in one call of gfar_poll */
-#define GFAR_DEV_WEIGHT 64
+/* The maximum number of packets to be handled in one call of gfar_poll_rx */
+#define GFAR_DEV_RX_WEIGHT 64
+/* The maximum number of packets to be handled in one call of gfar_poll_tx */
+#define GFAR_DEV_TX_WEIGHT 64
/* Length for FCB */
#define GMAC_FCB_LEN 8
@@ -366,6 +368,10 @@ extern const char gfar_driver_version[];
| IMASK_PERR)
#define IMASK_RTX_DISABLED ((~(IMASK_RXFEN0 | IMASK_TXFEN | IMASK_BSY)) \
& IMASK_DEFAULT)
+#define IMASK_RX_DEFAULT (IMASK_RXFEN0 | IMASK_BSY)
+#define IMASK_TX_DEFAULT (IMASK_TXFEN)
+#define IMASK_RX_DISABLED ((~(IMASK_RX_DEFAULT)) & IMASK_DEFAULT)
+#define IMASK_TX_DISABLED ((~(IMASK_TX_DEFAULT)) & IMASK_DEFAULT)
/* Fifo management */
#define FIFO_TX_THR_MASK 0x01ff
@@ -993,7 +999,8 @@ struct gfar_priv_rx_q {
/**
* struct gfar_priv_grp - per group structure
- * @napi: the napi poll function
+ * @napi_rx: the RX napi poll function
+ * @napi_tx: the TX confirmation napi poll function
* @priv: back pointer to the priv structure
* @regs: the ioremapped register space for this group
* @grp_id: group id for this group
@@ -1007,7 +1014,8 @@ struct gfar_priv_rx_q {
struct gfar_priv_grp {
spinlock_t grplock __attribute__ ((aligned (SMP_CACHE_BYTES)));
- struct napi_struct napi;
+ struct napi_struct napi_rx;
+ struct napi_struct napi_tx;
struct gfar_private *priv;
struct gfar __iomem *regs;
unsigned int grp_id;
--
1.6.6
^ permalink raw reply related
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Arnd Bergmann @ 2012-08-08 12:39 UTC (permalink / raw)
To: Ian Molton
Cc: David Miller, linux-arm-kernel, andrew, thomas.petazzoni,
ben.dooks, netdev
In-Reply-To: <502252A6.4090409@codethink.co.uk>
On Wednesday 08 August 2012, Ian Molton wrote:
> The SMI / PHY stuff should look very similar, so I'm happy with something
> like:
>
> mdio@2000 {
> #address-cells = <1>;
> #size-cells = <1>;
> device_type = "mdio";
> compatible = "marvell,mv643xx-mdio";
> phy0: ethernet-phy@0 {
> device_type = "ethernet-phy";
> compatible = "marvell,whatever";
> interrupts = <76>;
> interrupt-parent = <&mpic>;
> reg = <0 32>; // Auto probed phy addr
> };
>
> phy1: ethernet-phy@3 {
> device_type = "ethernet-phy";
> compatible = "marvell,whatever";
> interrupts = <77>;
> interrupt-parent = <&mpic>;
> reg = <3 1>; // specified phy addr
> };
>
> ... and so on.
> }
>
> Where we can use the reg parameter to allow auto-probing, by
> specifying a size of 32 (32 phy addrs max).
I don't understand the auto-probed phy address. What is the purpose of that?
If possible, I think we should keep using #size-cells=<0>, which would
make the method you describe impossible. It might still work if you just
leave out the "reg" property for that node.
I also don't understand how the phy driver would locate ethernet-phy@0
on the bus if it does not know the address.
> The ethernet driver itself is more complicated:
>
> We have the following considerations:
>
> * we have one MDIO bus, typically, shared between all the MACs / PHYs.
> * each ethernet device can multiple ports (up to three), each with its
> own MAC/PHY.
> * MAC <-> PHY mapping can be specified, probed (ugh!) or a (gah!)
> mix of the two.
> * existing D-T users, albeit not well documented / code complete.
> * some port address ranges overlap (MIB counters, MCAST / UNICAST
> tables, etc.
>
> The existing ethernet-group idea only works because the current
> platform-device based driver doesnt really do proper resource
> management, and thus the MAC registers are actually mapped by
> the MDIO driver.
>
> I don't think that preserving this bad behaviour is a good idea, which
> leaves us with two choices:
>
> 1) My preferred solution - allow each device to specify up to three
> interrupts, MACs, and PHYs. This is clean in that it doesnt require
> multiply instantiating a driver three times over the same address
> space.
>
> ethernet@2400 {
> compatible = "marvell,mv643xx-eth";
> reg = <0x2400 0x1c00>
> interrupt_parent = <&mpic>;
> ports = <3>;
> interrupts = <4>, <5>, <6>;
> phys = <&phy0>, <&phy1>, <&phy2>;
> };
>
> ethernet@6400 {
> compatible = "marvell,mv643xx-eth";
> reg = <0x6400 0x1c00>
> interrupt_parent = <&mpic>;
> ports = <1>;
> interrupts = <4>;
> phys = <&phy3>;
> };
>
> Note that the address is 2400, not 2000 - since this driver no longer
> would share its address range with the MDIO driver.
>
> This method would require a small amount of rework in the driver to
> set up <n> ports, rather than just one.
This looks quite nice, but it is still very much incompatible with the
existing binding. Obviously we can abandon an existing binding and
introduce a second one for the same hardware, but that should not
be taken lightly.
> 2) Create some kind of pseudo-ethernet group device that manages
> all the work for some sort of lightweight ethernet device, one per
> port. This can never be done cleanly since the port address ranges
> overlap:
>
> pseudo_eth@2400 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "marvell,mv643xx-shared-eth"
> reg = <0x2400 0x1c00>;
>
> ethernet@0 {
> compatible = "marvell,mv643xx-port";
> interrupts = <4>;
> interrupt_parent = <&mpic>;
> phy = <&phy0>;
> };
>
> ethernet@1 {
> compatible = "marvell,mv643xx-port";
> interrupts = <5>;
> interrupt_parent = <&mpic>;
> phy = <&phy1>;
> };
>
> ethernet@2 {
> compatible = "marvell,mv643xx-port";
> interrupts = <6>;
> interrupt_parent = <&mpic>;
> phy = <&phy2>;
> };
> }
> pseudo_eth@6400 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "marvell,mv643xx-shared-eth"
> reg = <0x6400 0x1c00>;
>
> ethernet@0 {
> compatible = "marvell,mv643xx-port";
> interrupts = <4>;
> interrupt_parent = <&mpic>;
> phy = <&phy3>;
> };
> };
This looks almost compatible with the existing binding, which is
good. I would in fact recommend to use the actual "compatible"
strings from the binding. More generally speaking, you should not
use wildcards in those strings anyway, so always use
"marvell,mv64360-eth" instead of "marvell,mv64x60-eth" or
"marvell,mv643xx-eth". If you have multiple chips that are
completely compatible, put use the identifier for the older one.
I don't fully understand your concern with the overlapping
registers, mostly because I still don't know all the combinations
that are actually valid here. Let me try to say what I understood
so far, and you can correct me if that's wrong:
* A system can have multiple instances of an mv64360 ethernet
block, with a register area of 0x2000 bytes.
* Each such block can have three MACs and three PHYs.
* The first 0x400 bytes in the register space control the three
PHYs and the remaining registers control the MACs.
* While this is meant to be used in a way that you assign
the each of the three PHYs to one of the MACs, this is not
always done, and sometimes you use a different PHY (?), or
one from a different instance of the mv64360 ethernet block
on the same SoC?.
Arnd
^ permalink raw reply
* Re: [RFC PATCH] tun: don't zeroize sock->file on detach
From: Stanislav Kinsbursky @ 2012-08-08 12:53 UTC (permalink / raw)
To: davem@davemloft.net
Cc: netdev@vger.kernel.org, ruanzhijie@hotmail.com,
linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk
In-Reply-To: <20120711114753.24395.53193.stgit@localhost6.localdomain6>
Hi, Dave.
What about this patch?
On Wed, Jul 11, 2012 at 03:48:20PM +0400, Stanislav Kinsbursky wrote:
> This is a fix for bug, introduced in 3.4 kernel by commit
> 1ab5ecb90cb6a3df1476e052f76a6e8f6511cb3d, which, among other things, replaced
> simple sock_put() by sk_release_kernel(). Below is sequence, which leads to
> oops for non-persistent devices:
>
> tun_chr_close()
> tun_detach() <== tun->socket.file = NULL
> tun_free_netdev()
> sk_release_sock()
> sock_release(sock->file == NULL)
> iput(SOCK_INODE(sock)) <== dereference on NULL pointer
>
> This patch just removes zeroing of socket's file from __tun_detach().
> sock_release() will do this.
>
> Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>
> ---
> drivers/net/tun.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 987aeef..c1639f3 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -185,7 +185,6 @@ static void __tun_detach(struct tun_struct *tun)
> netif_tx_lock_bh(tun->dev);
> netif_carrier_off(tun->dev);
> tun->tfile = NULL;
> - tun->socket.file = NULL;
> netif_tx_unlock_bh(tun->dev);
^ permalink raw reply
* [PATCH] lpc_eth: remove obsolete ifdefs
From: Roland Stigge @ 2012-08-08 13:18 UTC (permalink / raw)
To: netdev, linux-kernel, davem, edumazet, aletes.xgr, kevin.wells,
srinivas.bakki
Cc: Roland Stigge
The #ifdefs regarding CONFIG_ARCH_LPC32XX_MII_SUPPORT and
CONFIG_ARCH_LPC32XX_IRAM_FOR_NET are obsolete since the symbols have been
removed from Kconfig and replaced by devicetree based configuration.
Signed-off-by: Roland Stigge <stigge@antcom.de>
---
drivers/net/ethernet/nxp/lpc_eth.c | 13 -------------
1 file changed, 13 deletions(-)
--- linux-2.6.orig/drivers/net/ethernet/nxp/lpc_eth.c
+++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c
@@ -346,28 +346,15 @@ static phy_interface_t lpc_phy_interface
"phy-mode", NULL);
if (mode && !strcmp(mode, "mii"))
return PHY_INTERFACE_MODE_MII;
- return PHY_INTERFACE_MODE_RMII;
}
-
- /* non-DT */
-#ifdef CONFIG_ARCH_LPC32XX_MII_SUPPORT
- return PHY_INTERFACE_MODE_MII;
-#else
return PHY_INTERFACE_MODE_RMII;
-#endif
}
static bool use_iram_for_net(struct device *dev)
{
if (dev && dev->of_node)
return of_property_read_bool(dev->of_node, "use-iram");
-
- /* non-DT */
-#ifdef CONFIG_ARCH_LPC32XX_IRAM_FOR_NET
- return true;
-#else
return false;
-#endif
}
/* Receive Status information word */
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-08 13:19 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Miller, linux-arm-kernel, andrew, thomas.petazzoni,
ben.dooks, netdev
In-Reply-To: <201208081239.16778.arnd@arndb.de>
On 08/08/12 13:39, Arnd Bergmann wrote:
> On Wednesday 08 August 2012, Ian Molton wrote:
>> The SMI / PHY stuff should look very similar, so I'm happy with something
>> like:
>>
>> mdio@2000 {
>> #address-cells = <1>;
>> #size-cells = <1>;
>> device_type = "mdio";
>> compatible = "marvell,mv643xx-mdio";
>> phy0: ethernet-phy@0 {
>> device_type = "ethernet-phy";
>> compatible = "marvell,whatever";
>> interrupts = <76>;
>> interrupt-parent = <&mpic>;
>> reg = <0 32>; // Auto probed phy addr
>> };
>>
>> phy1: ethernet-phy@3 {
>> device_type = "ethernet-phy";
>> compatible = "marvell,whatever";
>> interrupts = <77>;
>> interrupt-parent = <&mpic>;
>> reg = <3 1>; // specified phy addr
>> };
>>
>> ... and so on.
>> }
>>
>> Where we can use the reg parameter to allow auto-probing, by
>> specifying a size of 32 (32 phy addrs max).
> I don't understand the auto-probed phy address. What is the purpose of that?
Personally, I think it should die - but the existing driver and a number
of its users actually scan the bus for their PHY.
I doubt the PHY really moves about or is hotplugged by any of them,
and its actually quite a slow process.
> If possible, I think we should keep using #size-cells=<0>, which would
> make the method you describe impossible. It might still work if you just
> leave out the "reg" property for that node.
I can certainly investigate that. I couldn't see any good evidence that
it was a supported mechanism when I looked.
> I also don't understand how the phy driver would locate ethernet-phy@0
> on the bus if it does not know the address.
>
>> The ethernet driver itself is more complicated:
>>
>> We have the following considerations:
>>
>> * we have one MDIO bus, typically, shared between all the MACs / PHYs.
>> * each ethernet device can multiple ports (up to three), each with its
>> own MAC/PHY.
>> * MAC <-> PHY mapping can be specified, probed (ugh!) or a (gah!)
>> mix of the two.
>> * existing D-T users, albeit not well documented / code complete.
>> * some port address ranges overlap (MIB counters, MCAST / UNICAST
>> tables, etc.
>>
>> The existing ethernet-group idea only works because the current
>> platform-device based driver doesnt really do proper resource
>> management, and thus the MAC registers are actually mapped by
>> the MDIO driver.
>>
>> I don't think that preserving this bad behaviour is a good idea, which
>> leaves us with two choices:
>>
>> 1) My preferred solution - allow each device to specify up to three
>> interrupts, MACs, and PHYs. This is clean in that it doesnt require
>> multiply instantiating a driver three times over the same address
>> space.
>>
>> ethernet@2400 {
>> compatible = "marvell,mv643xx-eth";
>> reg = <0x2400 0x1c00>
>> interrupt_parent = <&mpic>;
>> ports = <3>;
>> interrupts = <4>, <5>, <6>;
>> phys = <&phy0>, <&phy1>, <&phy2>;
>> };
>>
>> ethernet@6400 {
>> compatible = "marvell,mv643xx-eth";
>> reg = <0x6400 0x1c00>
>> interrupt_parent = <&mpic>;
>> ports = <1>;
>> interrupts = <4>;
>> phys = <&phy3>;
>> };
>>
>> Note that the address is 2400, not 2000 - since this driver no longer
>> would share its address range with the MDIO driver.
>>
>> This method would require a small amount of rework in the driver to
>> set up <n> ports, rather than just one.
> This looks quite nice, but it is still very much incompatible with the
> existing binding. Obviously we can abandon an existing binding and
> introduce a second one for the same hardware, but that should not
> be taken lightly.
Fair, however the existing users aren't anywhere near as
numerous as the new ones.
>> 2) Create some kind of pseudo-ethernet group device that manages
>> all the work for some sort of lightweight ethernet device, one per
>> port. This can never be done cleanly since the port address ranges
>> overlap:
>>
>> pseudo_eth@2400 {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> compatible = "marvell,mv643xx-shared-eth"
>> reg = <0x2400 0x1c00>;
>>
>> ethernet@0 {
>> compatible = "marvell,mv643xx-port";
>> interrupts = <4>;
>> interrupt_parent = <&mpic>;
>> phy = <&phy0>;
>> };
>>
>> ethernet@1 {
>> compatible = "marvell,mv643xx-port";
>> interrupts = <5>;
>> interrupt_parent = <&mpic>;
>> phy = <&phy1>;
>> };
>>
>> ethernet@2 {
>> compatible = "marvell,mv643xx-port";
>> interrupts = <6>;
>> interrupt_parent = <&mpic>;
>> phy = <&phy2>;
>> };
>> }
>> pseudo_eth@6400 {
>> #address-cells = <1>;
>> #size-cells = <0>;
>> compatible = "marvell,mv643xx-shared-eth"
>> reg = <0x6400 0x1c00>;
>>
>> ethernet@0 {
>> compatible = "marvell,mv643xx-port";
>> interrupts = <4>;
>> interrupt_parent = <&mpic>;
>> phy = <&phy3>;
>> };
>> };
> This looks almost compatible with the existing binding, which is
> good.
Well, I'm not sure about that - if the existing bindings are really
baked into firmware, then "almost" wont be any use at all.
> I would in fact recommend to use the actual "compatible"
> strings from the binding. More generally speaking, you should not
> use wildcards in those strings anyway, so always use
> "marvell,mv64360-eth" instead of "marvell,mv64x60-eth" or
> "marvell,mv643xx-eth". If you have multiple chips that are
> completely compatible, put use the identifier for the older one.
Noted.
> I don't fully understand your concern with the overlapping
> registers, mostly because I still don't know all the combinations
> that are actually valid here. Let me try to say what I understood
> so far, and you can correct me if that's wrong:
>
> * A system can have multiple instances of an mv64360 ethernet
> block, with a register area of 0x2000 bytes.
> * Each such block can have three MACs and three PHYs.
> * The first 0x400 bytes in the register space control the three
> PHYs and the remaining registers control the MACs.
> * While this is meant to be used in a way that you assign
> the each of the three PHYs to one of the MACs, this is not
> always done, and sometimes you use a different PHY (?), or
> one from a different instance of the mv64360 ethernet block
> on the same SoC?.
Nearly - the whole block is 0x2000 in size, yes. And each one
can have 3 MACs and PHYs, as you say.
There is SMI @ 0x2000 - just one for all ports, and in many
(all?) cases, for all all the other controllers on the SoC to
share. On the armadaXP SoC, for example, each ethernet
block has its own alias of the same bas SMI reg. (there are
4 blocks)
ethernet0@ 0x2400
## regs in order: Main regs, MIB counters, Special mcast table, Mcast
table, Unicast table.
port0 has regs at +0x0000 *0x1000 +0x1400 +0x1500 +0x1600
port1 has regs at +0x0400 *0x1080 +0x1800 +0x1900 +0x1a00
port2 has regs at +0x0800 *0x1100 +0x1c00 +0x1d00 +0x1e00
ethernet1@ 0x6400
port0 has regs at +0x0000 *0x1000 +0x1400 +0x1500 +0x1600
...
As you can see, instead of putting port1 at +0x1700 or so,
marvell have overlapped the register files - in fact, doubly
so, since port1 + 0x1080 is right in the middle of
(port0 + 0x1000) -> (port0 + 0x16ff), so one cant simply map two
sets of regs like 0x0000->0x03ff and 0x1000->0x16ff for port one
either.
-Ian
^ permalink raw reply
* Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
From: Ben Hutchings @ 2012-08-08 13:25 UTC (permalink / raw)
To: Pavel Emelyanov
Cc: David Miller, Eric Dumazet, Eric W. Biederman, Linux Netdev List
In-Reply-To: <50222AC3.8090801@parallels.com>
On Wed, 2012-08-08 at 13:00 +0400, Pavel Emelyanov wrote:
> 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?
Yes, that looks like a neat way to do it. Maybe with an explanatory
comment?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie @ 2012-08-08 13:30 UTC (permalink / raw)
To: Wolfgang Grandegger
Cc: Marc Kleine-Budde, linux-can-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5022227F.60800-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>
On Wed, Aug 08, 2012 at 10:25:35AM +0200, Wolfgang Grandegger wrote:
> 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.
Ok I'll try to find it.
>
> >>> Signed-off-by: Olivier Sobrie <olivier-Ui3EtX6WB9GzQB+pC5nmwQ@public.gmane.org>
> >>> ---
> >>> 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"?
No but that's maybe because of my misunderstanding of the
M16C_STATE_BUS_ERROR flag.
What I see is:
t1: M16C_STATE_BUS_ERROR
t2: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_PASSIVE
t3: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_OFF
and then again
t4: M16C_STATE_BUS_ERROR
t2: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_PASSIVE
t3: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_OFF
Thus as you suggested below, the flag M16C_STATE_BUS_ERROR might not mean
CAN_STATE_ERROR_WARNING...
>
> >>> 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.
candump can1,0:0,#FFFFFFFF -td -e:
(001.369850) can1 123 [3] 11 22 33
(004.716034) can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
protocol-violation{{back-to-error-active}{}}
Not so close... I'll add something in the tx acknowledge.
>
> >>> 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).
(002.465349) can1 123 [3] 11 22 33
(004.562670) can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
controller-problem{rx-error-passive,tx-error-passive}
>
> >>> +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:
With my hardware I do not get txerr and rxerr... I only get
M16C_STATE_BUS_ERROR. Thus in fact I can only pass from ACTIVE to PASSIVE
without reaching the WARNING state.
People of Kvaser just told me txerr and rxerr was set with other hardwares
than Kvaser Leaf Lighti... That's the one I've... I'm unlucky :-(.
I'll adapt the code so that txerr and rxerr are reported for working
hardwares..
>
> $ ./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
Ok. I'll only set CAN_ERR_PROT_LOC_ACK and not CAN_ERR_PROT_LOC_DEL.
>
>
> >
> >>
> >>> + 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.
Ok.
>
> >>
> >>> + 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().
Ok.
Thank you,
--
Olivier
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv2 1/4] modem_shm: Add Modem Access Framework
From: Greg KH @ 2012-08-08 13:37 UTC (permalink / raw)
To: Arun MURTHY
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, Sjur BRENDELAND
In-Reply-To: <F45880696056844FA6A73F415B568C695B0E796A03@EXDCVYMBSTM006.EQ1STM.local>
On Wed, Aug 08, 2012 at 05:36:05AM +0200, Arun MURTHY wrote:
> > 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.
Over where?
What "modem IPC"?
You need to really explain what you are doing here, as I have no idea.
And I have no idea why you still can't just use a tty device, why can't
you?
> 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.
What's a "sim driver", and what type of "security drivers" are you
referring to?
> 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.
Again, why can't the tty layer do this for you?
greg k-h
^ permalink raw reply
* Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
From: Pavel Emelyanov @ 2012-08-08 13:38 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Miller, Eric Dumazet, Eric W. Biederman, Linux Netdev List
In-Reply-To: <1344432330.13142.188.camel@deadeye.wl.decadent.org.uk>
>> 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?
>
> Yes, that looks like a neat way to do it. Maybe with an explanatory
> comment?
Yes, sure. I will resend the set shortly.
Thanks for the feedback!
> Ben.
>
Thanks,
Pavel
^ permalink raw reply
* [PATCH 00/11] recently added ARM defconfig warnings
From: Arnd Bergmann @ 2012-08-08 14:47 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Takashi Iwai, Linus Walleij, Grant Likely, Laxman Dewangan,
Philipp Zabel, Giuseppe Cavallaro, Stefan Roese, Lee Jones,
Kukjin Kim, Vincent Guittot, Samuel Ortiz, arm, Alan Stern,
Grazvydas Ignotas, Paul Parsons, Russell King, Peter Zijlstra,
Arnd Bergmann, Thomas Abraham, Rafael J. Wysocki, Jaswinder Singh,
Magnus Damm, Namhyung Kim, Greg
These patches address problems that showed up in new warnings
when building all ARM defconfig files in v3.6-rc1 compared
with v3.5.
I can merge them through the arm-soc tree or have subsystem
maintainers pick them up if they prefer.
Arnd Bergmann (11):
ARM: topology: mark init_cpu_topology as __init
mfd/asic3: fix asic3_mfd_probe return value
usb/ohci-omap: remove unused variable
ARM: ux500: really kill snowball_of_platform_devs
ARM: exynos: exynos_pm_add_dev_to_genpd may be unused
gpio: em: do not discard em_gio_irq_domain_cleanup
net/stmmac: mark probe function as __devinit
mtd/omap2: fix dmaengine_slave_config error handling
regulator/twl: remove fixed resource handling
spi/s3c64xx: improve error handling
pm/drivers: fix use of SIMPLE_DEV_PM_OPS
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: David S. Miller <davem@davemloft.net>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Grazvydas Ignotas <notasas@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jaswinder Singh <jaswinder.singh@linaro.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Laxman Dewangan <ldewangan@nvidia.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Magnus Damm <damm@opensource.se>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Parsons <lost.distance@yahoo.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Philipp Zabel <philipp.zabel@gmail.com>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Richard Zhao <richard.zhao@freescale.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Thomas Abraham <thomas.abraham@linaro.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: netdev@vger.kernel.org
arch/arm/kernel/topology.c | 2 +-
arch/arm/mach-exynos/pm_domains.c | 2 +-
arch/arm/mach-ux500/board-mop500.c | 5 -----
drivers/char/hw_random/omap-rng.c | 2 +-
drivers/gpio/gpio-em.c | 2 +-
drivers/i2c/busses/i2c-tegra.c | 2 +-
drivers/mfd/asic3.c | 1 +
drivers/mtd/nand/omap2.c | 7 +++----
.../net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +-
drivers/regulator/twl-regulator.c | 21 --------------------
drivers/spi/spi-s3c64xx.c | 2 +-
drivers/usb/host/ohci-omap.c | 2 --
sound/drivers/dummy.c | 2 +-
13 files changed, 12 insertions(+), 40 deletions(-)
--
1.7.10
^ permalink raw reply
* [PATCH 07/11] net/stmmac: mark probe function as __devinit
From: Arnd Bergmann @ 2012-08-08 14:47 UTC (permalink / raw)
To: linux-arm-kernel
Cc: arm, linux-kernel, Arnd Bergmann, Stefan Roese,
Giuseppe Cavallaro, David S. Miller, netdev
In-Reply-To: <1344437248-20560-1-git-send-email-arnd@arndb.de>
Driver probe functions are generally __devinit so they will be
discarded after initialization for non-hotplug kernels.
This was found by a new warning after patch 6a228452d "stmmac: Add
device-tree support" adds a new __devinit function that is called
from stmmac_pltfr_probe.
Without this patch, building socfpga_defconfig results in:
WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt()
The function stmmac_pltfr_probe() references
the function __devinit stmmac_probe_config_dt().
This is often because stmmac_pltfr_probe lacks a __devinit
annotation or the annotation of stmmac_probe_config_dt is wrong.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: netdev@vger.kernel.org
---
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index cd01ee7..b93245c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -74,7 +74,7 @@ static int __devinit stmmac_probe_config_dt(struct platform_device *pdev,
* the necessary resources and invokes the main to init
* the net device, register the mdio bus etc.
*/
-static int stmmac_pltfr_probe(struct platform_device *pdev)
+static int __devinit stmmac_pltfr_probe(struct platform_device *pdev)
{
int ret = 0;
struct resource *res;
--
1.7.10
^ permalink raw reply related
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Wolfgang Grandegger @ 2012-08-08 15:02 UTC (permalink / raw)
To: Olivier Sobrie
Cc: Marc Kleine-Budde, linux-can-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120808133005.GA14300@hposo>
On 08/08/2012 03:30 PM, Olivier Sobrie wrote:
> On Wed, Aug 08, 2012 at 10:25:35AM +0200, Wolfgang Grandegger wrote:
>> 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.
>
> Ok I'll try to find it.
>
>>
>>>>> Signed-off-by: Olivier Sobrie <olivier-Ui3EtX6WB9GzQB+pC5nmwQ@public.gmane.org>
>>>>> ---
>>>>> 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"?
>
> No but that's maybe because of my misunderstanding of the
> M16C_STATE_BUS_ERROR flag.
> What I see is:
> t1: M16C_STATE_BUS_ERROR
> t2: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_PASSIVE
> t3: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_OFF
> and then again
> t4: M16C_STATE_BUS_ERROR
> t2: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_PASSIVE
> t3: M16C_STATE_BUS_ERROR | M16C_STATE_BUS_OFF
>
> Thus as you suggested below, the flag M16C_STATE_BUS_ERROR might not mean
> CAN_STATE_ERROR_WARNING...
Do you see bus error bits set? If not, I could mean "error active",
otherwise "error warning". Meaning the device sends such messages
containing bus error information plus the current state.
>>>>> 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.
>
> candump can1,0:0,#FFFFFFFF -td -e:
> (001.369850) can1 123 [3] 11 22 33
> (004.716034) can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
> protocol-violation{{back-to-error-active}{}}
>
> Not so close... I'll add something in the tx acknowledge.
More than three seconds, wired. I would add netdev_dbg() to the state
change and error handling code to better understand what's going on.
....
>>> 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:
>
> With my hardware I do not get txerr and rxerr... I only get
> M16C_STATE_BUS_ERROR. Thus in fact I can only pass from ACTIVE to PASSIVE
> without reaching the WARNING state.
No problem, I just wanted to illustrate how the hardware does usually do
error and state change reporting.
Wolfgang.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 07/11] net/stmmac: mark probe function as __devinit
From: Stefan Roese @ 2012-08-08 15:07 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, arm, linux-kernel, Giuseppe Cavallaro,
David S. Miller, netdev
In-Reply-To: <1344437248-20560-8-git-send-email-arnd@arndb.de>
On 08/08/2012 04:47 PM, Arnd Bergmann wrote:
> Driver probe functions are generally __devinit so they will be
> discarded after initialization for non-hotplug kernels.
> This was found by a new warning after patch 6a228452d "stmmac: Add
> device-tree support" adds a new __devinit function that is called
> from stmmac_pltfr_probe.
>
> Without this patch, building socfpga_defconfig results in:
>
> WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt()
> The function stmmac_pltfr_probe() references
> the function __devinit stmmac_probe_config_dt().
> This is often because stmmac_pltfr_probe lacks a __devinit
> annotation or the annotation of stmmac_probe_config_dt is wrong.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
Acked-by: Stefan Roese <sr@denx.de>
Thanks,
Stefan
^ permalink raw reply
* [PATCH] can/softing: Fix potential memory leak in softing_load_fw()
From: Alexey Khoroshilov @ 2012-08-08 15:15 UTC (permalink / raw)
To: Kurt Van Dijck
Cc: Alexey Khoroshilov, Wolfgang Grandegger, Marc Kleine-Budde,
linux-can, netdev, linux-kernel, ldv-project
Do not leak memory by updating pointer with potentially NULL realloc return value.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/net/can/softing/softing_fw.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c
index 3105961..b595d34 100644
--- a/drivers/net/can/softing/softing_fw.c
+++ b/drivers/net/can/softing/softing_fw.c
@@ -150,7 +150,7 @@ int softing_load_fw(const char *file, struct softing *card,
const uint8_t *mem, *end, *dat;
uint16_t type, len;
uint32_t addr;
- uint8_t *buf = NULL;
+ uint8_t *buf = NULL, *new_buf;
int buflen = 0;
int8_t type_end = 0;
@@ -199,11 +199,12 @@ int softing_load_fw(const char *file, struct softing *card,
if (len > buflen) {
/* align buflen */
buflen = (len + (1024-1)) & ~(1024-1);
- buf = krealloc(buf, buflen, GFP_KERNEL);
- if (!buf) {
+ new_buf = krealloc(buf, buflen, GFP_KERNEL);
+ if (!new_buf) {
ret = -ENOMEM;
goto failed;
}
+ buf = new_buf;
}
/* verify record data */
memcpy_fromio(buf, &dpram[addr + offset], len);
--
1.7.9.5
^ permalink raw reply related
* [PATCH] rndis_wlan: Fix potential memory leak in update_pmkid()
From: Alexey Khoroshilov @ 2012-08-08 15:44 UTC (permalink / raw)
To: Jussi Kivilinna
Cc: Alexey Khoroshilov, John W. Linville, linux-wireless, netdev,
linux-kernel, ldv-project
Do not leak memory by updating pointer with potentially NULL realloc return value.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/net/wireless/rndis_wlan.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 241162e..7a4ae9e 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -1803,6 +1803,7 @@ static struct ndis_80211_pmkid *update_pmkid(struct usbnet *usbdev,
struct cfg80211_pmksa *pmksa,
int max_pmkids)
{
+ struct ndis_80211_pmkid *new_pmkids;
int i, err, newlen;
unsigned int count;
@@ -1833,11 +1834,12 @@ static struct ndis_80211_pmkid *update_pmkid(struct usbnet *usbdev,
/* add new pmkid */
newlen = sizeof(*pmkids) + (count + 1) * sizeof(pmkids->bssid_info[0]);
- pmkids = krealloc(pmkids, newlen, GFP_KERNEL);
- if (!pmkids) {
+ new_pmkids = krealloc(pmkids, newlen, GFP_KERNEL);
+ if (!new_pmkids) {
err = -ENOMEM;
goto error;
}
+ pmkids = new_pmkids;
pmkids->length = cpu_to_le32(newlen);
pmkids->bssid_info_count = cpu_to_le32(count + 1);
--
1.7.9.5
^ permalink raw reply related
* Re: [RFC net-next 3/4] gianfar: Separate out the Rx and Tx coalescing functions
From: Paul Gortmaker @ 2012-08-08 15:44 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: netdev, David S. Miller
In-Reply-To: <1344428810-29923-4-git-send-email-claudiu.manoil@freescale.com>
[[RFC net-next 3/4] gianfar: Separate out the Rx and Tx coalescing functions] On 08/08/2012 (Wed 15:26) Claudiu Manoil wrote:
> Split the coalescing programming support by Rx and Tx h/w queues, in order to
> introduce a separate NAPI for the Tx confirmation path (next patch). This way,
> the Rx processing path will handle the coalescing settings for the Rx queues
> only, resp. the Tx confirmation processing path will handle the Tx queues.
>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
> ---
> drivers/net/ethernet/freescale/gianfar.c | 36 +++++++++++++++++++++++------
> 1 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index ddd350a..919acb3 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -1794,8 +1794,8 @@ void gfar_start(struct net_device *dev)
> dev->trans_start = jiffies; /* prevent tx timeout */
> }
>
> -void gfar_configure_coalescing(struct gfar_private *priv,
> - unsigned long tx_mask, unsigned long rx_mask)
> +static inline void gfar_configure_tx_coalescing(struct gfar_private *priv,
I believe the preference is to not specify inline when all the chunks in
play are present in the one C file -- i.e. let gcc figure it out. Same
for the Rx instance below.
P.
--
> + unsigned long mask)
> {
> struct gfar __iomem *regs = priv->gfargrp[0].regs;
> u32 __iomem *baddr;
> @@ -1803,14 +1803,31 @@ void gfar_configure_coalescing(struct gfar_private *priv,
> if (priv->mode == MQ_MG_MODE) {
> int i;
> baddr = ®s->txic0;
> - for_each_set_bit(i, &tx_mask, priv->num_tx_queues) {
> + for_each_set_bit(i, &mask, priv->num_tx_queues) {
> gfar_write(baddr + i, 0);
> if (likely(priv->tx_queue[i]->txcoalescing))
> gfar_write(baddr + i, priv->tx_queue[i]->txic);
> }
> + } else {
> + /* Backward compatible case ---- even if we enable
> + * multiple queues, there's only single reg to program
> + */
> + gfar_write(®s->txic, 0);
> + if (likely(priv->tx_queue[0]->txcoalescing))
> + gfar_write(®s->txic, priv->tx_queue[0]->txic);
> + }
> +}
> +
> +static inline void gfar_configure_rx_coalescing(struct gfar_private *priv,
> + unsigned long mask)
> +{
> + struct gfar __iomem *regs = priv->gfargrp[0].regs;
> + u32 __iomem *baddr;
>
> + if (priv->mode == MQ_MG_MODE) {
> + int i;
> baddr = ®s->rxic0;
> - for_each_set_bit(i, &rx_mask, priv->num_rx_queues) {
> + for_each_set_bit(i, &mask, priv->num_rx_queues) {
> gfar_write(baddr + i, 0);
> if (likely(priv->rx_queue[i]->rxcoalescing))
> gfar_write(baddr + i, priv->rx_queue[i]->rxic);
> @@ -1819,16 +1836,19 @@ void gfar_configure_coalescing(struct gfar_private *priv,
> /* Backward compatible case ---- even if we enable
> * multiple queues, there's only single reg to program
> */
> - gfar_write(®s->txic, 0);
> - if (likely(priv->tx_queue[0]->txcoalescing))
> - gfar_write(®s->txic, priv->tx_queue[0]->txic);
> -
> gfar_write(®s->rxic, 0);
> if (likely(priv->rx_queue[0]->rxcoalescing))
> gfar_write(®s->rxic, priv->rx_queue[0]->rxic);
> }
> }
>
> +void gfar_configure_coalescing(struct gfar_private *priv,
> + unsigned long tx_mask, unsigned long rx_mask)
> +{
> + gfar_configure_tx_coalescing(priv, tx_mask);
> + gfar_configure_rx_coalescing(priv, rx_mask);
> +}
> +
> static int register_grp_irqs(struct gfar_priv_grp *grp)
> {
> struct gfar_private *priv = grp->priv;
> --
> 1.6.6
>
>
^ permalink raw reply
* Re: [RFC net-next 2/4] gianfar: Clear ievent from interrupt handler for [RT]x int
From: Paul Gortmaker @ 2012-08-08 16:11 UTC (permalink / raw)
To: Claudiu Manoil; +Cc: netdev, David S. Miller
In-Reply-To: <1344428810-29923-3-git-send-email-claudiu.manoil@freescale.com>
[[RFC net-next 2/4] gianfar: Clear ievent from interrupt handler for [RT]x int] On 08/08/2012 (Wed 15:26) Claudiu Manoil wrote:
> It's the interrupt handler's job to clear ievent for the Tx/Rx paths, as soon
> as the corresponding interrupt sources have been masked.
What wasn't clear to me was whether we'd ever have an instance of
gfar_poll run without RTX_MASK being cleared (in less normal conditions,
like netconsole, KGDBoE etc), since the gfar_schedule_cleanup is only
called from rx/tx IRQ threads, and neither of those are used by
gfar_poll, it seems.
Paul.
--
>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
> ---
> drivers/net/ethernet/freescale/gianfar.c | 16 ++++++----------
> 1 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
> index e9feeb9..ddd350a 100644
> --- a/drivers/net/ethernet/freescale/gianfar.c
> +++ b/drivers/net/ethernet/freescale/gianfar.c
> @@ -2568,12 +2568,13 @@ static void gfar_schedule_cleanup(struct gfar_priv_grp *gfargrp)
> if (napi_schedule_prep(&gfargrp->napi)) {
> gfar_write(&gfargrp->regs->imask, IMASK_RTX_DISABLED);
> __napi_schedule(&gfargrp->napi);
> - } else {
> - /* Clear IEVENT, so interrupts aren't called again
> - * because of the packets that have already arrived.
> - */
> - gfar_write(&gfargrp->regs->ievent, IEVENT_RTX_MASK);
> }
> +
> + /* Clear IEVENT, so interrupts aren't called again
> + * because of the packets that have already arrived.
> + */
> + gfar_write(&gfargrp->regs->ievent, IEVENT_RTX_MASK);
> +
> spin_unlock_irqrestore(&gfargrp->grplock, flags);
>
> }
> @@ -2837,11 +2838,6 @@ static int gfar_poll(struct napi_struct *napi, int budget)
> num_queues = gfargrp->num_rx_queues;
> budget_per_queue = budget/num_queues;
>
> - /* Clear IEVENT, so interrupts aren't called again
> - * because of the packets that have already arrived
> - */
> - gfar_write(®s->ievent, IEVENT_RTX_MASK);
> -
> while (num_queues && left_over_budget) {
> budget_per_queue = left_over_budget/num_queues;
> left_over_budget = 0;
> --
> 1.6.6
>
>
^ 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