* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: David Miller @ 2012-08-08 5:55 UTC (permalink / raw)
To: B32167; +Cc: netdev
In-Reply-To: <AC83832D6324604BB08FAE5A33553F1907A96EA6@039-SN2MPN1-012.039d.mgd.msft.net>
From: Jain Priyanka-B32167 <B32167@freescale.com>
Date: Wed, 8 Aug 2012 04:53:42 +0000
>
>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, August 08, 2012 4:52 AM
> To: Jain Priyanka-B32167
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
>
> From: Priyanka Jain <Priyanka.Jain@freescale.com>
> Date: Tue, 7 Aug 2012 10:51:44 +0530
>
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>>
>> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>
> This patch doesn't apply to the net-next tree, please respin.
> [Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
>
> Also:
>
>> - xfrm_policy_afinfo[afinfo->family] = NULL;
>> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
>> + NULL);
>
> Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
> [Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
> Please let me know your final say on this. I will make changes accordingly if required.
What in the world are you talking about?
The openning parenthesis of the rcu_assign_pointer() statement is not anywhere
close to the 80th column.
You're doing this:
x(A,
B);
and I want you to do this:
x(A,
B);
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-08 6:04 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford
In-Reply-To: <871ujjfkb1.fsf@xmission.com>
Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Ali Ayoub <ali@mellanox.com> writes:
>> Among other things, the main benefit we're targeting is to allow IPoE
>> traffic within the VM to go through the (Ethernet) vBridge down to the
>> eIPoIB PIF, and eventually to IPoIB and to the IB network.
> Oh yes. It just occurred to me there is huge problem with eIPoIB as
> currently presented in these patches. It breaks DHCPv4 the same way
> it breaks ARP, but DHCPv4 is not fixed up.
To put things in place, DHCPv4 is supported with eIPoIB, the DHCP
UDP/IP payload isn't touched, only need to set the BOOTP broadcast
flag in the dhcp server config file.
Or.
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie @ 2012-08-08 6:14 UTC (permalink / raw)
To: Wolfgang Grandegger; +Cc: Marc Kleine-Budde, linux-can, linux-usb, netdev
In-Reply-To: <5020B51E.8060600@grandegger.com>
Hi Wolfgang,
On Tue, Aug 07, 2012 at 08:26:38AM +0200, Wolfgang Grandegger wrote:
> On 08/06/2012 07:21 AM, Olivier Sobrie wrote:
> > This driver provides support for several Kvaser CAN/USB devices.
> > Such kind of devices supports up to three can network interfaces.
>
> s/can/CAN/
>
> > It has been tested with a Kvaser USB Leaf Light (one network interface)
> > connected to a pch_can interface.
> > The firmware version of the Kvaser device was 2.5.205.
> >
> > List of Kvaser devices supported by the driver:
> > - Kvaser Leaf prototype (P010v2 and v3)
> > - Kvaser Leaf Light (P010v3)
> > - Kvaser Leaf Professional HS
> > - Kvaser Leaf SemiPro HS
> > - Kvaser Leaf Professional LS
> > - Kvaser Leaf Professional SWC
> > - Kvaser Leaf Professional LIN
> > - Kvaser Leaf SemiPro LS
> > - Kvaser Leaf SemiPro SWC
> > - Kvaser Memorator II, Prototype
> > - Kvaser Memorator II HS/HS
> > - Kvaser USBcan Professional HS/HS
> > - Kvaser Leaf Light GI
> > - Kvaser Leaf Professional HS (OBD-II connector)
> > - Kvaser Memorator Professional HS/LS
> > - Kvaser Leaf Light "China"
> > - Kvaser BlackBird SemiPro
> > - Kvaser OEM Mercury
> > - Kvaser OEM Leaf
> > - Kvaser USBcan R
>
> Impressive list! What CAN controllers are used inside the devices? SJA1000?
I took this list from the Kvaser driver. However I only have a Kvaser
Leaf Light device thus I'm not sure it will work with other ones.
If you prefer I can only let Kvaser Leaf Light instead of the full list.
In my device it looks to be a Renesas M16C controller.
>
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> > Changes since v1:
> > - added copyrights
> > - kvaser_usb.h merged into kvaser.c
> > - added kvaser_usb_get_endpoints to find eindpoints instead of
> > hardcoding their address
> > - some cleanup and comestic changes
> > - fixed issues with errors handling
> > - fixed restart-ms == 0 case
> > - removed do_get_berr_counter method since the hardware doens't return
> > good values for txerr and rxerr.
> >
> > If someone in the linux-usb mailing can review it, it would be nice.
> >
> > Concerning the errors, it behaves like that now:
> >
> > 1) Short-circuit CAN-H and CAN-L and restart-ms = 0
> >
> > t0: short-circuit + 'cansend can1 123#112233'
> >
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-off
> > bus-error
> >
> > t1: remove short-circuit + 'ip link set can1 type can restart'
> >
> > can1 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
> > restarted-after-bus-off
> > can1 20000004 [8] 00 0C 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
>
> Why do we get the last error message? Maybe the firmware does it that
> way (going down passive->warning->active).
It goes in that order: warning -> passive -> bus off -> warning
-> passive -> ...
>
> > 2) Short-circuit CAN-H and CAN-L and restart-ms = 100
> >
> > t0: short-circuit + cansend can1 123#112233
> >
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-off
> > bus-error
> > can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > restarted-after-bus-off
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-off
> > bus-error
> > ...
> >
> > can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > restarted-after-bus-off
> > can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-warning,tx-error-warning}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> > can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
> > bus-error
> >
> > t1: remove short-circuit
> >
> > can1 123 [3] 11 22 33
> > can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
> > protocol-violation{{back-to-error-active}{}}
>
> The order is still inverted but likely the firmware is doing it that way.
Indeed the firmware does it that way: it sends the acknwledge of the
frame beofre the state change. I can avoid that behavior by checking the
state in the acknowledge frame and send the restart frame if the bus was
off.
>
> > 3) CAN-H and CAN-L disconnected
> >
> > t0: CAN-H and CAN-L disconnected + cansend can1 123#112233
> >
> > can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > can1 2000008C [8] 00 30 80 1B 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
> > protocol-violation{{error-on-tx}{acknowledge-delimiter}}
> > bus-error
> >
> > t1: CAN-H and CAN-L reconnected
> >
> > can1 123 [3] 11 22 33
> > can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
> > controller-problem{rx-error-passive,tx-error-passive}
>
> Why do we get an error-passive message? Now I will have a closer look to
> the code...
The firmware sends a CMD_CAN_ERROR_EVENT with the passive bit set...
> > +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct net_device_stats *stats;
> > + struct kvaser_usb_net_priv *priv;
> > + unsigned int new_state;
> > + u8 channel, status;
> > +
> > + if (msg->id == CMD_CAN_ERROR_EVENT) {
> > + channel = msg->u.error_event.channel;
> > + status = msg->u.error_event.status;
> > + } else {
> > + channel = msg->u.chip_state_event.channel;
> > + status = msg->u.chip_state_event.status;
> > + }
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > + stats = &priv->netdev->stats;
> > +
> > + if (status & M16C_STATE_BUS_RESET) {
> > + kvaser_usb_unlink_tx_urbs(priv);
> > + return;
> > + }
> > + skb = alloc_can_err_skb(priv->netdev, &cf);
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
>
> Cleanup? kvaser_usb_unlink_tx_urbs()?
If I get the M16C_STATE_BUS_RESET I'll not receive the ack frames anymore.
I need to set the context->echo_index back to MAX_TX_URBS to not loose tx
urbs.
By the way I think a can_free_echo_skb() is missing...
>
> > + }
> > +
> > + if (status & M16C_STATE_BUS_OFF) {
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > +
> > + if (!priv->can.restart_ms)
> > + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
> > +
> > + if (!priv->can.state != CAN_ERR_BUSOFF) {
> > + priv->can.can_stats.bus_off++;
> > + netif_carrier_off(priv->netdev);
> > + }
> > +
> > + new_state = CAN_STATE_BUS_OFF;
> > + } else if (status & M16C_STATE_BUS_PASSIVE) {
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
> > + CAN_ERR_CRTL_RX_PASSIVE;
>
> State changes should only be report when the state really changes.
> Therefore it should go under the if block below.
Ok. Is it possible to get such sequence:
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 10 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
2 questions:
1) If the bus is still in CAN_ERR_CRTL_RX_PASSIVE after the second frame,
shouldn't we let the corresponding bit set, 0x10?
2) Can we send multiple times same frame wih same error bits set?
>
> > + if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
> > + priv->can.can_stats.error_passive++;
> > +
> > + new_state = CAN_STATE_ERROR_PASSIVE;
> > + } else if (status & M16C_STATE_BUS_ERROR) {
>
> Hm, strange, a bus error is not a state change. You use here if...else
> if... Isn't it possible that more than one bit is set.
Indeed it is possible to have multiple bits set.
e.g. M16C_STATE_BUS_PASSIVE + M16C_STATE_BUS_ERROR or M16C_STATE_BUS_OFF + M16C_STATE_BUS_ERROR.
What error should I report in case of M16C_STATE_BUS_ERROR?
>
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = CAN_ERR_CRTL_TX_WARNING |
> > + CAN_ERR_CRTL_RX_WARNING;
>
> See above.
>
> > + if (priv->can.state != CAN_STATE_ERROR_WARNING)
> > + priv->can.can_stats.error_warning++;
> > +
> > + new_state = CAN_STATE_ERROR_WARNING;
> > + } else {
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_ACTIVE;
> > +
> > + new_state = CAN_STATE_ERROR_ACTIVE;
> > + }
> > +
> > + if (priv->can.restart_ms &&
> > + (priv->can.state >= CAN_STATE_BUS_OFF) &&
> > + (new_state < CAN_STATE_BUS_OFF)) {
> > + cf->can_id |= CAN_ERR_RESTARTED;
> > + priv->can.can_stats.restarts++;
> > + netif_carrier_on(priv->netdev);
> > + }
> > +
> > + if (msg->id == CMD_CAN_ERROR_EVENT) {
> > + u8 error_factor = msg->u.error_event.error_factor;
> > +
> > + priv->can.can_stats.bus_error++;
> > + stats->rx_errors++;
> > +
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > + if (error_factor & M16C_EF_ACKE)
> > + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
> > + CAN_ERR_PROT_LOC_ACK_DEL);
> > + if (error_factor & M16C_EF_CRCE)
> > + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL);
> > + if (error_factor & M16C_EF_FORME)
> > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > + if (error_factor & M16C_EF_STFE)
> > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > + if (error_factor & M16C_EF_BITE0)
> > + cf->data[2] |= CAN_ERR_PROT_BIT0;
> > + if (error_factor & M16C_EF_BITE1)
> > + cf->data[2] |= CAN_ERR_PROT_BIT1;
> > + if (error_factor & M16C_EF_TRE)
> > + cf->data[2] |= CAN_ERR_PROT_TX;
> > + }
> > +
> > + priv->can.state = new_state;
> > +
> > + if (!memcmp(cf, &priv->cf_err_old, sizeof(*cf))) {
> > + kfree_skb(skb);
> > + return;
> > + }
>
> Hm, the firmware seems not to clear error conditions? Anyway, state
> change and error reporting is magic on many CAN controllers. Just the
> SJA1000 is doing it nicely.
I added it to prevent sending two times the same error. It happens that
the firmware sends multiple times the same error message.
>
> > + netif_rx(skb);
> > +
> > + priv->cf_err_old = *cf;
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
> > +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct kvaser_usb_net_priv *priv;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct net_device_stats *stats;
> > + u8 channel = msg->u.rx_can.channel;
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > + stats = &priv->netdev->stats;
> > +
> > + skb = alloc_can_skb(priv->netdev, &cf);
> > + if (skb == NULL) {
>
> s/skb == NULL)/!skb/ ?
>
> > + stats->tx_dropped++;
> > + return;
> > + }
> > +
> > + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
> > + (msg->u.rx_can.msg[1] & 0x3f);
> > + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
> > +
> > + if (msg->id == CMD_RX_EXT_MESSAGE) {
> > + cf->can_id <<= 18;
> > + cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
> > + ((msg->u.rx_can.msg[3] & 0xff) << 6) |
> > + (msg->u.rx_can.msg[4] & 0x3f);
> > + cf->can_id |= CAN_EFF_FLAG;
> > + }
> > +
> > + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> > + MSG_FLAG_NERR)) {
> > + cf->can_id |= CAN_ERR_FLAG;
> > + cf->can_dlc = CAN_ERR_DLC;
> > + cf->data[1] = CAN_ERR_CRTL_UNSPEC;
>
> What is the meaning of such errors? A comment, netdev_err() or
> netdev_dbg() would be nice.
I never reached this error with the hardware I've... I don't know what's
the meaning of this flag... I will add a trace.
>
> > +
> > + stats->rx_errors++;
> > + } else if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> > + cf->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> > + cf->can_dlc = CAN_ERR_DLC;
> > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > +
> > + stats->rx_over_errors++;
> > + stats->rx_errors++;
> > + } else if (!msg->u.rx_can.flag) {
> > + memcpy(cf->data, &msg->u.rx_can.msg[6], cf->can_dlc);
> > + } else {
> > + kfree_skb(skb);
> > + return;
> > + }
> > +
> > + netif_rx(skb);
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
I'll fix all others small things you mentionned.
Thank you!
--
Olivier
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Eric Dumazet @ 2012-08-08 6:25 UTC (permalink / raw)
To: Priyanka Jain; +Cc: netdev
In-Reply-To: <1344316904-2544-1-git-send-email-Priyanka.Jain@freescale.com>
On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:
> xfrm_policy_afinfo is read mosly data structure.
> Write on xfrm_policy_afinfo is done only at the
> time of configuration.
> So rwlocks can be safely replaced with RCU.
>
> RCUs usage optimizes the performance.
> static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
> @@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
> struct xfrm_policy_afinfo *afinfo;
> if (unlikely(family >= NPROTO))
> return NULL;
> - read_lock(&xfrm_policy_afinfo_lock);
> - afinfo = xfrm_policy_afinfo[family];
> + rcu_read_lock();
> + afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
> if (unlikely(!afinfo))
> - read_unlock(&xfrm_policy_afinfo_lock);
> + rcu_read_unlock();
This makes no sense to me : We cant safely return afinfo here.
Note the current code is buggy as well, this is worrying.
As soon as we exit from xfrm_policy_get_afinfo(), pointer might be
invalid.
Really, RCU conversion should be the right moment to spot those bugs and
first fix them (for stable trees)
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Fan Du @ 2012-08-08 6:44 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Priyanka Jain, netdev
In-Reply-To: <1344407147.28967.212.camel@edumazet-glaptop>
First, sorry to jump in.
On 2012年08月08日 14:25, Eric Dumazet wrote:
> On Tue, 2012-08-07 at 10:51 +0530, Priyanka Jain wrote:
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the
>> time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>
>
>> static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
>> @@ -2530,16 +2535,16 @@ static struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short family)
>> struct xfrm_policy_afinfo *afinfo;
>> if (unlikely(family>= NPROTO))
>> return NULL;
>> - read_lock(&xfrm_policy_afinfo_lock);
>> - afinfo = xfrm_policy_afinfo[family];
>> + rcu_read_lock();
>> + afinfo = rcu_dereference(xfrm_policy_afinfo[family]);
>> if (unlikely(!afinfo))
>> - read_unlock(&xfrm_policy_afinfo_lock);
>> + rcu_read_unlock();
>
> This makes no sense to me : We cant safely return afinfo here.
>
> Note the current code is buggy as well, this is worrying.
>
> As soon as we exit from xfrm_policy_get_afinfo(), pointer might be
> invalid.
>
Yes, it might be invalid, but all the callers have checked the return
value, thus use it in a sane way.
So I don't follow "Note the current code is buggy as well".
Am I missing something here?
> Really, RCU conversion should be the right moment to spot those bugs and
> first fix them (for stable trees)
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Love each day!
--fan
^ permalink raw reply
* Re: [PATCH] cdc-phonet: Don't leak in usbpn_open
From: Rémi Denis-Courmont @ 2012-08-08 7:12 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel, netdev, linux-usb, Greg Kroah-Hartman
In-Reply-To: <alpine.LNX.2.00.1208072354030.3227@swampdragon.chaosbits.net>
Le mercredi 8 août 2012 00:56:26 Jesper Juhl, vous avez écrit :
> We allocate memory for 'req' with usb_alloc_urb() and then test
> 'if (!req || rx_submit(pnd, req, GFP_KERNEL | __GFP_COLD))'.
> If we enter that branch due to '!req' then there is no problem. But if
> we enter the branch due to 'req' being != 0 and the 'rx_submit()' call
> being false, then we'll leak the memory we allocated.
> Deal with the leak by always calling 'usb_free_urb(req)' when entering
> the branch. If 'req' happens to be 0 then the call is harmless, if it
> is not 0 then we free the memory we allocated but don't need.
>
> Signed-off-by: Jesper Juhl <jj@chaosbits.net>
Acked-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
> drivers/net/usb/cdc-phonet.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Only compile tested due to lack of hardware.
Hardware won't help you much with testing the error case anyway.
--
Rémi Denis-Courmont, looking for a job
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-08 7:32 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford, Michael S. Tsirkin
In-Reply-To: <87obmnfs4p.fsf@xmission.com>
Eric W. Biederman <ebiederm@xmission.com> wrote:
> Ali Ayoub <ali@mellanox.com> writes:
[...]
>> I don't see in other alternatives a solution for the problem we're
>> trying to solve. If there are changes/suggestions to improve eIPoIB
>> netdev driver to avoid "messing with the link layer" and make it
>> acceptable, we can discuss and apply them.
> Nothing needs to be applied the code is done. Routing from
> IPoE to IPoIB works. There is nothing in what anyone has posted as requirements
> that needs work to implement.
> I totally fail to see how getting packets of of the VM as ethernet
> frames, and then IP layer routing those packets over IP is not an
> option. What requirement am I missing.
As you've indicated routing w/w.o using proxy-arp is an option, however,
> All VMs should suport that mode of operation, and certainly the kernel does.
> Implementations involving bridges like macvlan and macvtap are
> performance optimizations, and the optimizations don't even apply in
> areas like 802.11, where only one mac address is supported per adapter.
> Bridging can ocassionally also be an administrative simplification as
> well, but you should be able to achieve the a similar simplification
> with a dhcprelay and proxy arp.
as you wrote here, when performance and ease-of-use is under the spot,
VM deployments tend to not to use routing.
This is b/c it involves more over-head on the packet forwarding, and
more administration work, for example, for setting routing rules that
involve the VM IP address, something which AFAIK the hypervisor have
no clue on, also its unclear to me if/how live migration can work in
such setting.
>From this exact reason, there's a bunch of use-cases by tools and
cloud stacks (such as open stack, ovirt, more) which do use bridged
mode and the rest of the Ethernet envelope, such as using virtual L2
vlan domains, ebtables based rules, etc etc. Where they and are not
application to ipoib, but are working file ith eipoib.
You mentioned that bridging mode doesn't apply to environment such as
802.11, and hence routing mode is used, we are trying to make a point
here that bridging mode applies to ipoib with the approach suggested
by eipoib.
Also, if we extend the discussion a bit, there are two more aspects to throw in:
The first is the performance thing we have already started to mention
-- specifically, the approach for RX zero copy (into the VM buffer),
use designs such as vhost + macvtap NIC in passthrough mode which is
likey to be set over a per VM hypervisor NIC, e.g such as the ones
provided by VMDQ patches John Fastabend started to post (see
http://marc.info/?l=linux-netdev&m=134264998405581&w=2) -- the ib0.N
clone child are IPoIB VMDQ NICs if you like, and setting an eipoib NIC
on top of each they can be plugged to that design.
The 2nd aspect, is NON VM environments where a NIC with Ethernet look
and feel is required for IP traffic, but this have to live within an
echo-system that fully uses IPoIB.
In other words, a use case where IPoIB has to be below the cover for
set of some specific apps, or nodes but do IP interaction with other
apps/nodes and gateways who use IPoIB, the eIPoIB driver provides that
functionality.
So, to sum up, routing / proxy-arp seems to be off where we are targeting.
Or.
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Arnd Bergmann @ 2012-08-08 8:16 UTC (permalink / raw)
To: David Miller
Cc: ian.molton, linux-arm-kernel, andrew, thomas.petazzoni, ben.dooks,
netdev
In-Reply-To: <20120807.162923.34400427265666163.davem@davemloft.net>
On Tuesday 07 August 2012, David Miller wrote:
> From: Ian Molton <ian.molton@codethink.co.uk>
> Date: Tue, 7 Aug 2012 15:34:45 +0100
>
> > Fixed all comments.
> >
> > * Dropped csb1724 defconfig.
> > * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
> > * Dropped un-necessary D-T irq fixup code
>
> Who is going to take this series?
I'd prefer to take the entire series through the arm-soc tree from
the kirkwood maintainers. We first have to work out the bindings
though, since the current patch introduces a new one that is
incompatible with the one we were using on powerpc with
open firmware before.
Arnd
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Wolfgang Grandegger @ 2012-08-08 8:25 UTC (permalink / raw)
To: Olivier Sobrie; +Cc: Marc Kleine-Budde, linux-can, linux-usb, netdev
In-Reply-To: <20120808061419.GA16678@hposo>
Hi Oliver,
On 08/08/2012 08:14 AM, Olivier Sobrie wrote:
> Hi Wolfgang,
>
> On Tue, Aug 07, 2012 at 08:26:38AM +0200, Wolfgang Grandegger wrote:
>> On 08/06/2012 07:21 AM, Olivier Sobrie wrote:
>>> This driver provides support for several Kvaser CAN/USB devices.
>>> Such kind of devices supports up to three can network interfaces.
>>
>> s/can/CAN/
>>
>>> It has been tested with a Kvaser USB Leaf Light (one network interface)
>>> connected to a pch_can interface.
>>> The firmware version of the Kvaser device was 2.5.205.
>>>
>>> List of Kvaser devices supported by the driver:
>>> - Kvaser Leaf prototype (P010v2 and v3)
>>> - Kvaser Leaf Light (P010v3)
>>> - Kvaser Leaf Professional HS
>>> - Kvaser Leaf SemiPro HS
>>> - Kvaser Leaf Professional LS
>>> - Kvaser Leaf Professional SWC
>>> - Kvaser Leaf Professional LIN
>>> - Kvaser Leaf SemiPro LS
>>> - Kvaser Leaf SemiPro SWC
>>> - Kvaser Memorator II, Prototype
>>> - Kvaser Memorator II HS/HS
>>> - Kvaser USBcan Professional HS/HS
>>> - Kvaser Leaf Light GI
>>> - Kvaser Leaf Professional HS (OBD-II connector)
>>> - Kvaser Memorator Professional HS/LS
>>> - Kvaser Leaf Light "China"
>>> - Kvaser BlackBird SemiPro
>>> - Kvaser OEM Mercury
>>> - Kvaser OEM Leaf
>>> - Kvaser USBcan R
>>
>> Impressive list! What CAN controllers are used inside the devices? SJA1000?
>
> I took this list from the Kvaser driver. However I only have a Kvaser
> Leaf Light device thus I'm not sure it will work with other ones.
> If you prefer I can only let Kvaser Leaf Light instead of the full list.
> In my device it looks to be a Renesas M16C controller.
OK. Checking the manual, if available, could help to understand how the
firmware handles bus errors and state changes.
>>> Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
>>> ---
>>> Changes since v1:
>>> - added copyrights
>>> - kvaser_usb.h merged into kvaser.c
>>> - added kvaser_usb_get_endpoints to find eindpoints instead of
>>> hardcoding their address
>>> - some cleanup and comestic changes
>>> - fixed issues with errors handling
>>> - fixed restart-ms == 0 case
>>> - removed do_get_berr_counter method since the hardware doens't return
>>> good values for txerr and rxerr.
>>>
>>> If someone in the linux-usb mailing can review it, it would be nice.
>>>
>>> Concerning the errors, it behaves like that now:
>>>
>>> 1) Short-circuit CAN-H and CAN-L and restart-ms = 0
>>>
>>> t0: short-circuit + 'cansend can1 123#112233'
>>>
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-off
>>> bus-error
>>>
>>> t1: remove short-circuit + 'ip link set can1 type can restart'
>>>
>>> can1 20000100 [8] 00 00 00 00 00 00 00 00 ERRORFRAME
>>> restarted-after-bus-off
>>> can1 20000004 [8] 00 0C 00 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>
>> Why do we get the last error message? Maybe the firmware does it that
>> way (going down passive->warning->active).
>
> It goes in that order: warning -> passive -> bus off -> warning
> -> passive -> ...
Just for curiosity? You don't see back to "error active"?
>>> 2) Short-circuit CAN-H and CAN-L and restart-ms = 100
>>>
>>> t0: short-circuit + cansend can1 123#112233
>>>
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-off
>>> bus-error
>>> can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> restarted-after-bus-off
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 200000C8 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-off
>>> bus-error
>>> ...
>>>
>>> can1 2000018C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> restarted-after-bus-off
>>> can1 2000008C [8] 00 0C 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-warning,tx-error-warning}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>> can1 2000008C [8] 00 30 90 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{tx-recessive-bit-error,error-on-tx}{}}
>>> bus-error
>>>
>>> t1: remove short-circuit
>>>
>>> can1 123 [3] 11 22 33
>>> can1 20000008 [8] 00 00 40 00 00 00 00 00 ERRORFRAME
>>> protocol-violation{{back-to-error-active}{}}
>>
>> The order is still inverted but likely the firmware is doing it that way.
>
> Indeed the firmware does it that way: it sends the acknwledge of the
> frame beofre the state change. I can avoid that behavior by checking the
> state in the acknowledge frame and send the restart frame if the bus was
> off.
Well, if the firmware does it wrong, I would not really care. Also,
could you use timestamping to see if they come close together.
>>> 3) CAN-H and CAN-L disconnected
>>>
>>> t0: CAN-H and CAN-L disconnected + cansend can1 123#112233
>>>
>>> can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> can1 2000008C [8] 00 30 80 1B 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>> protocol-violation{{error-on-tx}{acknowledge-delimiter}}
>>> bus-error
>>>
>>> t1: CAN-H and CAN-L reconnected
>>>
>>> can1 123 [3] 11 22 33
>>> can1 20000004 [8] 00 30 00 00 00 00 00 00 ERRORFRAME
>>> controller-problem{rx-error-passive,tx-error-passive}
>>
>> Why do we get an error-passive message? Now I will have a closer look to
>> the code...
>
> The firmware sends a CMD_CAN_ERROR_EVENT with the passive bit set...
Maybe the order is again inverted. Do they come at the same time
(visiable with candump -td).
>>> +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct net_device_stats *stats;
>>> + struct kvaser_usb_net_priv *priv;
>>> + unsigned int new_state;
>>> + u8 channel, status;
>>> +
>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>> + channel = msg->u.error_event.channel;
>>> + status = msg->u.error_event.status;
>>> + } else {
>>> + channel = msg->u.chip_state_event.channel;
>>> + status = msg->u.chip_state_event.status;
>>> + }
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> + stats = &priv->netdev->stats;
>>> +
>>> + if (status & M16C_STATE_BUS_RESET) {
>>> + kvaser_usb_unlink_tx_urbs(priv);
>>> + return;
>>> + }
>>> + skb = alloc_can_err_skb(priv->netdev, &cf);
>>> + if (!skb) {
>>> + stats->rx_dropped++;
>>> + return;
>>
>> Cleanup? kvaser_usb_unlink_tx_urbs()?
>
> If I get the M16C_STATE_BUS_RESET I'll not receive the ack frames anymore.
> I need to set the context->echo_index back to MAX_TX_URBS to not loose tx
> urbs.
> By the way I think a can_free_echo_skb() is missing...
>
>>
>>> + }
>>> +
>>> + if (status & M16C_STATE_BUS_OFF) {
>>> + cf->can_id |= CAN_ERR_BUSOFF;
>>> +
>>> + if (!priv->can.restart_ms)
>>> + kvaser_usb_simple_msg_async(priv, CMD_STOP_CHIP);
>>> +
>>> + if (!priv->can.state != CAN_ERR_BUSOFF) {
>>> + priv->can.can_stats.bus_off++;
>>> + netif_carrier_off(priv->netdev);
>>> + }
>>> +
>>> + new_state = CAN_STATE_BUS_OFF;
>>> + } else if (status & M16C_STATE_BUS_PASSIVE) {
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_TX_PASSIVE |
>>> + CAN_ERR_CRTL_RX_PASSIVE;
>>
>> State changes should only be report when the state really changes.
>> Therefore it should go under the if block below.
>
> Ok. Is it possible to get such sequence:
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 10 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
> can1 20000088 [8] 00 00 90 00 00 00 00 00 ERRORFRAME
>
> 2 questions:
> 1) If the bus is still in CAN_ERR_CRTL_RX_PASSIVE after the second frame,
> shouldn't we let the corresponding bit set, 0x10?
No, see below.
> 2) Can we send multiple times same frame wih same error bits set?
Yes, because that's what the hardware reports.
>>
>>> + if (priv->can.state != CAN_STATE_ERROR_PASSIVE)
>>> + priv->can.can_stats.error_passive++;
>>> +
>>> + new_state = CAN_STATE_ERROR_PASSIVE;
>>> + } else if (status & M16C_STATE_BUS_ERROR) {
>>
>> Hm, strange, a bus error is not a state change. You use here if...else
>> if... Isn't it possible that more than one bit is set.
>
> Indeed it is possible to have multiple bits set.
> e.g. M16C_STATE_BUS_PASSIVE + M16C_STATE_BUS_ERROR or M16C_STATE_BUS_OFF + M16C_STATE_BUS_ERROR.
OK, that's the normal behaviour. Obviously they send bus errors together
with the *actual* state. The hardware does usuallly report bus errors
frequently while the error condition persists.
> What error should I report in case of M16C_STATE_BUS_ERROR?
To make that clear, I have added an (old) output from the SJA1000, which
is the defacto reference. Bus error reporting is enabled and no cable is
connected. Watch the TX error count increasing and how the state changes:
$ ./candump -e 0xffff any
can0 20000088 [8] 00 00 80 19 00 08 00 00 ERRORFRAME
\ \ \-- ACK slot.
\ \-- error occured on transmission
\-- Bus-error | Protocol violations (data[2], data[3]).
can0 20000088 [8] 00 00 80 19 00 10 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 18 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 20 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 28 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 30 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 38 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 40 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 48 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 50 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 58 00 00 ERRORFRAME
can0 2000008C [8] 00 08 80 19 00 60 00 00 ERRORFRAME
\ \-- reached warning level for TX errors
\-- | Controller problems (see data[1]).
can0 20000088 [8] 00 00 80 19 00 68 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 70 00 00 ERRORFRAME
can0 20000088 [8] 00 00 80 19 00 78 00 00 ERRORFRAME
can0 2000008C [8] 00 20 80 19 00 80 00 00 ERRORFRAME
\ \-- reached passive level for TX errors
\-- | Controller problems (see data[1]).
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
\ \-- RXerror count
\-- TXerror count
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
...
can0 20000088 [8] 00 00 80 19 00 80 00 00 ERRORFRAME
>
>>
>>> + cf->can_id |= CAN_ERR_CRTL;
>>> + cf->data[1] = CAN_ERR_CRTL_TX_WARNING |
>>> + CAN_ERR_CRTL_RX_WARNING;
>>
>> See above.
>>
>>> + if (priv->can.state != CAN_STATE_ERROR_WARNING)
>>> + priv->can.can_stats.error_warning++;
>>> +
>>> + new_state = CAN_STATE_ERROR_WARNING;
>>> + } else {
>>> + cf->can_id |= CAN_ERR_PROT;
>>> + cf->data[2] = CAN_ERR_PROT_ACTIVE;
>>> +
>>> + new_state = CAN_STATE_ERROR_ACTIVE;
>>> + }
>>> +
>>> + if (priv->can.restart_ms &&
>>> + (priv->can.state >= CAN_STATE_BUS_OFF) &&
>>> + (new_state < CAN_STATE_BUS_OFF)) {
>>> + cf->can_id |= CAN_ERR_RESTARTED;
>>> + priv->can.can_stats.restarts++;
>>> + netif_carrier_on(priv->netdev);
>>> + }
>>> +
>>> + if (msg->id == CMD_CAN_ERROR_EVENT) {
>>> + u8 error_factor = msg->u.error_event.error_factor;
>>> +
>>> + priv->can.can_stats.bus_error++;
>>> + stats->rx_errors++;
>>> +
>>> + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>> +
>>> + if (error_factor & M16C_EF_ACKE)
>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
>>> + CAN_ERR_PROT_LOC_ACK_DEL);
>>> + if (error_factor & M16C_EF_CRCE)
>>> + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
>>> + CAN_ERR_PROT_LOC_CRC_DEL);
>>> + if (error_factor & M16C_EF_FORME)
>>> + cf->data[2] |= CAN_ERR_PROT_FORM;
>>> + if (error_factor & M16C_EF_STFE)
>>> + cf->data[2] |= CAN_ERR_PROT_STUFF;
>>> + if (error_factor & M16C_EF_BITE0)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT0;
>>> + if (error_factor & M16C_EF_BITE1)
>>> + cf->data[2] |= CAN_ERR_PROT_BIT1;
>>> + if (error_factor & M16C_EF_TRE)
>>> + cf->data[2] |= CAN_ERR_PROT_TX;
>>> + }
>>> +
>>> + priv->can.state = new_state;
>>> +
>>> + if (!memcmp(cf, &priv->cf_err_old, sizeof(*cf))) {
>>> + kfree_skb(skb);
>>> + return;
>>> + }
>>
>> Hm, the firmware seems not to clear error conditions? Anyway, state
>> change and error reporting is magic on many CAN controllers. Just the
>> SJA1000 is doing it nicely.
>
> I added it to prevent sending two times the same error. It happens that
> the firmware sends multiple times the same error message.
Can then be removed, I think, see above.
>>
>>> + netif_rx(skb);
>>> +
>>> + priv->cf_err_old = *cf;
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> +}
>>> +
>>> +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
>>> + const struct kvaser_msg *msg)
>>> +{
>>> + struct kvaser_usb_net_priv *priv;
>>> + struct can_frame *cf;
>>> + struct sk_buff *skb;
>>> + struct net_device_stats *stats;
>>> + u8 channel = msg->u.rx_can.channel;
>>> +
>>> + if (channel >= dev->nchannels) {
>>> + dev_err(dev->udev->dev.parent,
>>> + "Invalid channel number (%d)\n", channel);
>>> + return;
>>> + }
>>> +
>>> + priv = dev->nets[channel];
>>> + stats = &priv->netdev->stats;
>>> +
>>> + skb = alloc_can_skb(priv->netdev, &cf);
>>> + if (skb == NULL) {
>>
>> s/skb == NULL)/!skb/ ?
>>
>>> + stats->tx_dropped++;
>>> + return;
>>> + }
>>> +
>>> + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
>>> + (msg->u.rx_can.msg[1] & 0x3f);
>>> + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
>>> +
>>> + if (msg->id == CMD_RX_EXT_MESSAGE) {
>>> + cf->can_id <<= 18;
>>> + cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
>>> + ((msg->u.rx_can.msg[3] & 0xff) << 6) |
>>> + (msg->u.rx_can.msg[4] & 0x3f);
>>> + cf->can_id |= CAN_EFF_FLAG;
>>> + }
>>> +
>>> + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
>>> + cf->can_id |= CAN_RTR_FLAG;
>>> + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
>>> + MSG_FLAG_NERR)) {
>>> + cf->can_id |= CAN_ERR_FLAG;
>>> + cf->can_dlc = CAN_ERR_DLC;
>>> + cf->data[1] = CAN_ERR_CRTL_UNSPEC;
>>
>> What is the meaning of such errors? A comment, netdev_err() or
>> netdev_dbg() would be nice.
>
> I never reached this error with the hardware I've... I don't know what's
> the meaning of this flag... I will add a trace.
Then add a netdev_err().
Wolfgang.
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Eric W. Biederman @ 2012-08-08 8:36 UTC (permalink / raw)
To: Or Gerlitz
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford
In-Reply-To: <CAJZOPZ+W0wfa38LejXt0gvBNa8Q6pWVZOs7xctHGRQXHAeMYPQ@mail.gmail.com>
Or Gerlitz <or.gerlitz@gmail.com> writes:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> Ali Ayoub <ali@mellanox.com> writes:
>
>>> Among other things, the main benefit we're targeting is to allow IPoE
>>> traffic within the VM to go through the (Ethernet) vBridge down to the
>>> eIPoIB PIF, and eventually to IPoIB and to the IB network.
>
>> Oh yes. It just occurred to me there is huge problem with eIPoIB as
>> currently presented in these patches. It breaks DHCPv4 the same way
>> it breaks ARP, but DHCPv4 is not fixed up.
>
> To put things in place, DHCPv4 is supported with eIPoIB, the DHCP
> UDP/IP payload isn't touched, only need to set the BOOTP broadcast
> flag in the dhcp server config file.
Wrong. DHCPv4 is broken over eIPoIB.
Coming from ethernet
htype == 1 not 32 as required by RFC4390
hlen == 6 not 0 as required by RFC4390
The chaddr field is has 6 bytes of the ethernet mac address not the
required 16 bytes of 0.
The client-identifier field is optional over ethernet.
An ethernet DHCPv4 client simply does not generate a dhcp packet that
conforms to RFC4390.
Therefore DHCPv4 over eIPoIB is broken, and a dhcp server or relay
may reasonably look at the DHCP packet and drop it because it is
garbage.
You might find a forgiving dhcp server that doesn't drop insane packets
on the floor and tries to make things work.
I am sorry. eIPoIB is broken as designed. eIPoIB most assuredly is not
compatible with ethernet. eIPoIB most definitely does not work even for
the general case of transporing IP traffic. Claiming that eIPoIB is any
else is a lie.
Eric
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: David Miller @ 2012-08-08 8:59 UTC (permalink / raw)
To: arnd
Cc: ian.molton, linux-arm-kernel, andrew, thomas.petazzoni, ben.dooks,
netdev
In-Reply-To: <201208080816.29218.arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, 8 Aug 2012 08:16:28 +0000
> On Tuesday 07 August 2012, David Miller wrote:
>> From: Ian Molton <ian.molton@codethink.co.uk>
>> Date: Tue, 7 Aug 2012 15:34:45 +0100
>>
>> > Fixed all comments.
>> >
>> > * Dropped csb1724 defconfig.
>> > * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
>> > * Dropped un-necessary D-T irq fixup code
>>
>> Who is going to take this series?
>
> I'd prefer to take the entire series through the arm-soc tree from
> the kirkwood maintainers. We first have to work out the bindings
> though, since the current patch introduces a new one that is
> incompatible with the one we were using on powerpc with
> open firmware before.
Ok.
^ permalink raw reply
* Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
From: Pavel Emelyanov @ 2012-08-08 9:00 UTC (permalink / raw)
To: Ben Hutchings
Cc: David Miller, Eric Dumazet, Eric W. Biederman, Linux Netdev List
In-Reply-To: <1344364573.2688.13.camel@bwh-desktop.uk.solarflarecom.com>
On 08/07/2012 10:36 PM, Ben Hutchings wrote:
> On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
>> The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
>> so take the given ifidex and register netdev with it.
>>
>> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
>> ---
>> drivers/net/veth.c | 3 +++
>> 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index 5852361..496c026 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>> if (tbp[IFLA_ADDRESS] == NULL)
>> eth_hw_addr_random(peer);
>>
>> + if (ifmp)
>> + peer->ifindex = ifmp->ifi_index;
>> +
>> err = register_netdevice(peer);
>> put_net(net);
>> net = NULL;
>
> Is this safe, given that this code path previously ignored
> ifmp->ifi_index? Userland could be passing in garbage and may now fail
> occasionally because the value clashes with an existing interface.
You're right, I've missed that fact :( The good news is that we still can
use the ifmp->ifi_index for the peer index configuration. We just need to
assume that if the caller specified the ifindex for the veth master device,
then it's aware of this possibility and should explicitly configure (or set
to 0) the peer's ifindex as well. Like this:
if (ifmp && (dev->ifindex != 0))
peer->ifindex = ifmp->ifi_index;
Does this assumption work from you POV?
> Ben.
>
Thanks,
Pavel
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Eric W. Biederman @ 2012-08-08 9:17 UTC (permalink / raw)
To: Or Gerlitz
Cc: Ali Ayoub, David Miller, ogerlitz, roland, netdev, sean.hefty,
erezsh, dledford, Michael S. Tsirkin
In-Reply-To: <CAJZOPZLApfvgd1wrM2HseNrWh-egaixjhGfB7xBJU1FxFhBdzg@mail.gmail.com>
Or Gerlitz <or.gerlitz@gmail.com> writes:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Ali Ayoub <ali@mellanox.com> writes:
> [...]
>>> I don't see in other alternatives a solution for the problem we're
>>> trying to solve. If there are changes/suggestions to improve eIPoIB
>>> netdev driver to avoid "messing with the link layer" and make it
>>> acceptable, we can discuss and apply them.
>
>> Nothing needs to be applied the code is done. Routing from
>> IPoE to IPoIB works. There is nothing in what anyone has posted as requirements
>> that needs work to implement.
>
>> I totally fail to see how getting packets of of the VM as ethernet
>> frames, and then IP layer routing those packets over IP is not an
>> option. What requirement am I missing.
>
>
> As you've indicated routing w/w.o using proxy-arp is an option, however,
>
>> All VMs should suport that mode of operation, and certainly the kernel does.
>> Implementations involving bridges like macvlan and macvtap are
>> performance optimizations, and the optimizations don't even apply in
>> areas like 802.11, where only one mac address is supported per adapter.
>> Bridging can ocassionally also be an administrative simplification as
>> well, but you should be able to achieve the a similar simplification
>> with a dhcprelay and proxy arp.
>
> as you wrote here, when performance and ease-of-use is under the spot,
> VM deployments tend to not to use routing.
>
> This is b/c it involves more over-head on the packet forwarding, and
> more administration work, for example, for setting routing rules that
> involve the VM IP address, something which AFAIK the hypervisor have
> no clue on, also its unclear to me if/how live migration can work in
> such setting.
All you need to make proxy-arp essentially pain free is a smart dhcp
relay, that sets up the routes.
> From this exact reason, there's a bunch of use-cases by tools and
> cloud stacks (such as open stack, ovirt, more) which do use bridged
> mode and the rest of the Ethernet envelope, such as using virtual L2
> vlan domains, ebtables based rules, etc etc. Where they and are not
> application to ipoib, but are working file ith eipoib.
Yes I am certain all of their IPv6 traffic works fine.
Regardless those are open source projects and can be modified to add
support to cleanly support inifinibnad.
> You mentioned that bridging mode doesn't apply to environment such as
> 802.11, and hence routing mode is used, we are trying to make a pointn
> here that bridging mode applies to ipoib with the approach suggested
> by eipoib.
You are completely failing. Every time I look I see something about
eIPoIB that is even more broken. Given that eIPoIB is a NAT
implementation that isn't really a surprise but still.
eIPoIB imposes enough overhead that I expect that routing is cheaper,
so your performance advantges go right out the window.
eIPoIB is seriously incompatible with ethernet breaking almost
everything and barely allowing IPv4 to work.
> Also, if we extend the discussion a bit, there are two more aspects to throw in:
>
> The first is the performance thing we have already started to mention
> -- specifically, the approach for RX zero copy (into the VM buffer),
> use designs such as vhost + macvtap NIC in passthrough mode which is
> likey to be set over a per VM hypervisor NIC, e.g such as the ones
> provided by VMDQ patches John Fastabend started to post (see
> http://marc.info/?l=linux-netdev&m=134264998405581&w=2) -- the ib0.N
> clone child are IPoIB VMDQ NICs if you like, and setting an eipoib NIC
> on top of each they can be plugged to that design.
If you care about performance link-layer NAT is not the way to go.
Teach the pieces you care about how to talk infiniband.
> The 2nd aspect, is NON VM environments where a NIC with Ethernet look
> and feel is required for IP traffic, but this have to live within an
> echo-system that fully uses IPoIB.
> In other words, a use case where IPoIB has to be below the cover for
> set of some specific apps, or nodes but do IP interaction with other
> apps/nodes and gateways who use IPoIB, the eIPoIB driver provides that
> functionality.
ip link add type dummy.
There now you have an interface with ethernet look and feel, and
routing can happily avoid it.
> So, to sum up, routing / proxy-arp seems to be off where we are
> targeting.
My condolences.
The existence of router / proxy-arp means that solutions do exist
(unlike your previous claim) you just don't like the idea of deploying
them.
Infiniband is standard enough you could quite easily implement virtual
infiniband bridging as an alternative to ethernet bridging.
At this stage of the game eIPoIB is interesting the same way a zombie is
interesting. It is fascinating to see it still moving as chunks of
flesh fall to the floor removing any doubt that it is dead.
Eric
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-08 9:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Miller, linux-arm-kernel, andrew, thomas.petazzoni,
ben.dooks, netdev
In-Reply-To: <201208080816.29218.arnd@arndb.de>
On 08/08/12 09:16, Arnd Bergmann wrote:
> I'd prefer to take the entire series through the arm-soc tree from
> the kirkwood maintainers. We first have to work out the bindings
> though, since the current patch introduces a new one that is
> incompatible with the one we were using on powerpc with open firmware
> before.
Looking at the ethernet-group stuff, specifically from
arch/powerpc/boot/dts/prpmc2800.dts, which I've taken as a base for the
below:
I think we can (and should) do something similar.
Sadly, there is no code present to describe marvell,mv64360-mdio,
however the device tree looks basically sane.
mdio@2000 {
#address-cells = <1>;
#size-cells = <0>;
device_type = "mdio";
compatible = "marvell,mv643xx-mdio";
PHY0: ethernet-phy@1 {
device_type = "ethernet-phy";
compatible = "broadcom,bcm5421";
interrupts = <76>; /* GPP 12 */
interrupt-parent = <&PIC>;
reg = <1>;
};
PHY1: ethernet-phy@3 {
device_type = "ethernet-phy";
compatible = "broadcom,bcm5421";
interrupts = <76>; /* GPP 12 */
interrupt-parent = <&PIC>;
reg = <3>;
};
};
ethernet-group@2400 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "marvell,mv64360-eth-group";
reg = <0x2400 0x2000>;
ethernet@0 {
device_type = "network";
compatible = "marvell,mv64360-eth";
reg = <0>;
interrupts = <32>;
interrupt-parent = <&mpic>;
phy = <&phy0>;
local-mac-address = [ 00 00 00 00 00 00 ];
};
ethernet@1 {
device_type = "network";
compatible = "marvell,mv64360-eth";
reg = <1>;
interrupts = <33>;
interrupt-parent = <&mpic>;
phy = <&phy1>;
local-mac-address = [ 00 00 00 00 00 00 ];
};
};
^ permalink raw reply
* RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Jain Priyanka-B32167 @ 2012-08-08 9:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20120807.225531.618417929491214091.davem@davemloft.net>
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, August 08, 2012 11:26 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Jain Priyanka-B32167 <B32167@freescale.com>
Date: Wed, 8 Aug 2012 04:53:42 +0000
>
>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, August 08, 2012 4:52 AM
> To: Jain Priyanka-B32167
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with
> rcu
>
> From: Priyanka Jain <Priyanka.Jain@freescale.com>
> Date: Tue, 7 Aug 2012 10:51:44 +0530
>
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>>
>> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>
> This patch doesn't apply to the net-next tree, please respin.
> [Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
>
> Also:
>
>> - xfrm_policy_afinfo[afinfo->family] = NULL;
>> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
>> + NULL);
>
> Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
> [Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
> Please let me know your final say on this. I will make changes accordingly if required.
What in the world are you talking about?
The openning parenthesis of the rcu_assign_pointer() statement is not anywhere close to the 80th column.
You're doing this:
x(A,
B);
and I want you to do this:
x(A,
B);
[Priyanka] Got it. Thanks. Will correct this.
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Ian Molton @ 2012-08-08 9:42 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Miller, linux-arm-kernel, andrew, thomas.petazzoni,
ben.dooks, netdev
In-Reply-To: <50223428.6030506@codethink.co.uk>
Ignore, Hit send whilst editing, by mistake. Sorry for the noise guys.
-Ian
^ permalink raw reply
* [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
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