* Re: [PATCH] xen-netback: count number required slots for an skb more carefully
From: Wei Liu @ 2013-09-04 13:14 UTC (permalink / raw)
To: David Vrabel
Cc: Wei Liu, xen-devel, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Ian Campbell, netdev, msw, annie.li
In-Reply-To: <52271DFF.3070008@citrix.com>
On Wed, Sep 04, 2013 at 12:48:15PM +0100, David Vrabel wrote:
> On 03/09/13 22:53, Wei Liu wrote:
> > On Tue, Sep 03, 2013 at 06:29:50PM +0100, David Vrabel wrote:
> >> From: David Vrabel <david.vrabel@citrix.com>
> >>
> >> When a VM is providing an iSCSI target and the LUN is used by the
> >> backend domain, the generated skbs for direct I/O writes to the disk
> >> have large, multi-page skb->data but no frags.
> >>
> >> With some lengths and starting offsets, xen_netbk_count_skb_slots()
> >> would be one short because the simple calculation of
> >> DIV_ROUND_UP(skb_headlen(), PAGE_SIZE) was not accounting for the
> >> decisions made by start_new_rx_buffer() which does not guarantee
> >> responses are fully packed.
> >>
> >> For example, a skb with length < 2 pages but which spans 3 pages would
> >> be counted as requiring 2 slots but would actually use 3 slots.
> >>
> >> skb->data:
> >>
> >> | 1111|222222222222|3333 |
> >>
> >> Fully packed, this would need 2 slots:
> >>
> >> |111122222222|22223333 |
> >>
> >> But because the 2nd page wholy fits into a slot it is not split across
> >> slots and goes into a slot of its own:
> >>
> >> |1111 |222222222222|3333 |
> >>
> >> Miscounting the number of slots means netback may push more responses
> >> than the number of available requests. This will cause the frontend
> >> to get very confused and report "Too many frags/slots". The frontend
> >> never recovers and will eventually BUG.
> >>
> >> Fix this by counting the number of required slots more carefully. In
> >> xen_netbk_count_skb_slots(), more closely follow the algorithm used by
> >> xen_netbk_gop_skb() by introducing xen_netbk_count_frag_slots() which
> >> is the dry-run equivalent of netbk_gop_frag_copy().
> >>
> >
> > Phew! So this is backend miscounting bug. I thought it was a frontend
> > bug so it didn't ring a bell when we had our face-to-face discussion,
> > sorry. :-(
> >
> > This bug was discussed back in July among Annie, Matt, Ian and I. We
> > finally agreed to take Matt's solution. Matt agreed to post final
> > version within a week but obviously he's too busy to do so. I was away
> > so I didn't follow closely. Eventually it fell through the crack. :-(
>
> I think I prefer fixing the counting for backporting to stable kernels.
The original patch has coding style change. Sans that contextual change
it's not a very long patch.
> Xi's approach of packing the ring differently is a change in frontend
> visible behaviour and seems more risky. e.g., possible performance
> impact so I would like to see some performance analysis of that approach.
>
With Xi's approach it is more efficient for backend to process. As we
now use one less grant copy operation which means we copy the same
amount of data with less grant ops.
>From frontend's PoV I think the impact is minimal. Frontend is involved
in assembling the packets. It only takes what's in the ring and chain
them together. The operation involves copying so far is the
__pskb_pull_tail which happens a) in rare case when there's more frags
than frontend's MAX_SKB_FRAGS, b) when pull_to > skb_headlen which
happens. With Xi's change the rare case a) will even be rarer than
before as we use less slots. b) happens the same as it happens before
Xi's change, because the pull is guarded by "if (pull_to >
skb_headlen(skb))" and Xi's change doesn't affect skb_headlen.
So overall I don't see obvious downside.
Wei.
> David
^ permalink raw reply
* Re: [PATCH net-next 2/2] tuntap: orphan frags before trying to set tx timestamp
From: Sergei Shtylyov @ 2013-09-04 13:27 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst, Richard Cochran
In-Reply-To: <1378269226-5969-2-git-send-email-jasowang@redhat.com>
Hello.
On 04-09-2013 8:33, Jason Wang wrote:
> sock_tx_timestamp() will clear all zerocopy flags of skb which may lead the
> frags never to be orphaned. This will break guest to guest traffic when zerocopy
> is enabled. Fix this by orphaning the frags before trying to set tx time stamp.
> The issue were introduced by commit eda297729171fe16bf34fe5b0419dfb69060f623
> (tun: Support software transmit time stamping).
> Cc: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/tun.c | 9 +++++----
> 1 files changed, 5 insertions(+), 4 deletions(-)
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 2dddb1b..af9a096 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -749,15 +749,16 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> >= dev->tx_queue_len / tun->numqueues)
> goto drop;
>
> + /* Orphan the skb - required as we might hang on to it
> + * for indefinite time. */
You could fix the comment style to the networking code default, while at: it:
/* bla
* bla
*/
> + if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
> + goto drop;
> +
> if (skb->sk) {
> sock_tx_timestamp(skb->sk, &skb_shinfo(skb)->tx_flags);
> sw_tx_timestamp(skb);
> }
>
> - /* Orphan the skb - required as we might hang on to it
> - * for indefinite time. */
> - if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
> - goto drop;
WBR, Sergei
^ permalink raw reply
* Re: [PATCH] xen-netback: count number required slots for an skb more carefully
From: Wei Liu @ 2013-09-04 13:35 UTC (permalink / raw)
To: Ian Campbell
Cc: David Vrabel, Wei Liu, xen-devel, Konrad Rzeszutek Wilk,
Boris Ostrovsky, netdev, msw, annie.li
In-Reply-To: <1378298485.17510.109.camel@kazak.uk.xensource.com>
On Wed, Sep 04, 2013 at 01:41:25PM +0100, Ian Campbell wrote:
> On Wed, 2013-09-04 at 12:48 +0100, David Vrabel wrote:
> > On 03/09/13 22:53, Wei Liu wrote:
> > > On Tue, Sep 03, 2013 at 06:29:50PM +0100, David Vrabel wrote:
> > >> From: David Vrabel <david.vrabel@citrix.com>
> > >>
> > >> When a VM is providing an iSCSI target and the LUN is used by the
> > >> backend domain, the generated skbs for direct I/O writes to the disk
> > >> have large, multi-page skb->data but no frags.
> > >>
> > >> With some lengths and starting offsets, xen_netbk_count_skb_slots()
> > >> would be one short because the simple calculation of
> > >> DIV_ROUND_UP(skb_headlen(), PAGE_SIZE) was not accounting for the
> > >> decisions made by start_new_rx_buffer() which does not guarantee
> > >> responses are fully packed.
> > >>
> > >> For example, a skb with length < 2 pages but which spans 3 pages would
> > >> be counted as requiring 2 slots but would actually use 3 slots.
> > >>
> > >> skb->data:
> > >>
> > >> | 1111|222222222222|3333 |
> > >>
> > >> Fully packed, this would need 2 slots:
> > >>
> > >> |111122222222|22223333 |
> > >>
> > >> But because the 2nd page wholy fits into a slot it is not split across
> > >> slots and goes into a slot of its own:
> > >>
> > >> |1111 |222222222222|3333 |
> > >>
> > >> Miscounting the number of slots means netback may push more responses
> > >> than the number of available requests. This will cause the frontend
> > >> to get very confused and report "Too many frags/slots". The frontend
> > >> never recovers and will eventually BUG.
> > >>
> > >> Fix this by counting the number of required slots more carefully. In
> > >> xen_netbk_count_skb_slots(), more closely follow the algorithm used by
> > >> xen_netbk_gop_skb() by introducing xen_netbk_count_frag_slots() which
> > >> is the dry-run equivalent of netbk_gop_frag_copy().
> > >>
> > >
> > > Phew! So this is backend miscounting bug. I thought it was a frontend
> > > bug so it didn't ring a bell when we had our face-to-face discussion,
> > > sorry. :-(
> > >
> > > This bug was discussed back in July among Annie, Matt, Ian and I. We
> > > finally agreed to take Matt's solution. Matt agreed to post final
> > > version within a week but obviously he's too busy to do so. I was away
> > > so I didn't follow closely. Eventually it fell through the crack. :-(
> >
> > I think I prefer fixing the counting for backporting to stable kernels.
>
> That's a good argument. I think we should take this patch, or something
> very like it, now and then rebase the more complex thing on top.
>
It's not as complex as you first see it. David's and Xi's approaches are
different routes to the same destination.
David's approach makes counting fits what netbk_gop_frag_copy actually
does, while Xi's approach is the other way around -- makes
netbk_gop_frag_copy fits what counting returns.
As long as we have them agreed with each other we're fine.
> > Xi's approach of packing the ring differently is a change in frontend
> > visible behaviour and seems more risky. e.g., possible performance
> > impact so I would like to see some performance analysis of that approach.
>
> Yes.
The performance impact is more concerning. I have a short analysis in
the reply to David's email.
Wei.
^ permalink raw reply
* Re: [PATCH] xen-netback: count number required slots for an skb more carefully
From: David Vrabel @ 2013-09-04 14:02 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel, Konrad Rzeszutek Wilk, Boris Ostrovsky, Ian Campbell,
netdev, msw, annie.li
In-Reply-To: <20130904131459.GO14104@zion.uk.xensource.com>
On 04/09/13 14:14, Wei Liu wrote:
> On Wed, Sep 04, 2013 at 12:48:15PM +0100, David Vrabel wrote:
>> On 03/09/13 22:53, Wei Liu wrote:
>>> On Tue, Sep 03, 2013 at 06:29:50PM +0100, David Vrabel wrote:
>>>> From: David Vrabel <david.vrabel@citrix.com>
>>>>
>>>> When a VM is providing an iSCSI target and the LUN is used by the
>>>> backend domain, the generated skbs for direct I/O writes to the disk
>>>> have large, multi-page skb->data but no frags.
>>>>
>>>> With some lengths and starting offsets, xen_netbk_count_skb_slots()
>>>> would be one short because the simple calculation of
>>>> DIV_ROUND_UP(skb_headlen(), PAGE_SIZE) was not accounting for the
>>>> decisions made by start_new_rx_buffer() which does not guarantee
>>>> responses are fully packed.
>>>>
>>>> For example, a skb with length < 2 pages but which spans 3 pages would
>>>> be counted as requiring 2 slots but would actually use 3 slots.
>>>>
>>>> skb->data:
>>>>
>>>> | 1111|222222222222|3333 |
>>>>
>>>> Fully packed, this would need 2 slots:
>>>>
>>>> |111122222222|22223333 |
>>>>
>>>> But because the 2nd page wholy fits into a slot it is not split across
>>>> slots and goes into a slot of its own:
>>>>
>>>> |1111 |222222222222|3333 |
>>>>
>>>> Miscounting the number of slots means netback may push more responses
>>>> than the number of available requests. This will cause the frontend
>>>> to get very confused and report "Too many frags/slots". The frontend
>>>> never recovers and will eventually BUG.
>>>>
>>>> Fix this by counting the number of required slots more carefully. In
>>>> xen_netbk_count_skb_slots(), more closely follow the algorithm used by
>>>> xen_netbk_gop_skb() by introducing xen_netbk_count_frag_slots() which
>>>> is the dry-run equivalent of netbk_gop_frag_copy().
>>>>
>>>
>>> Phew! So this is backend miscounting bug. I thought it was a frontend
>>> bug so it didn't ring a bell when we had our face-to-face discussion,
>>> sorry. :-(
>>>
>>> This bug was discussed back in July among Annie, Matt, Ian and I. We
>>> finally agreed to take Matt's solution. Matt agreed to post final
>>> version within a week but obviously he's too busy to do so. I was away
>>> so I didn't follow closely. Eventually it fell through the crack. :-(
>>
>> I think I prefer fixing the counting for backporting to stable kernels.
>
> The original patch has coding style change. Sans that contextual change
> it's not a very long patch.
The size of the patch isn't the main concern for backport-ability. It's
the frontend visible changes and thus any (unexpected) impacts on
frontends -- this is especially important as only a small fraction of
frontends in use will be tested with these changes.
>> Xi's approach of packing the ring differently is a change in frontend
>> visible behaviour and seems more risky. e.g., possible performance
>> impact so I would like to see some performance analysis of that approach.
>>
>
> With Xi's approach it is more efficient for backend to process. As we
> now use one less grant copy operation which means we copy the same
> amount of data with less grant ops.
It think it uses more grant ops because the copies of the linear
portion are in chunks that do not cross source page boundaries.
i.e., in netbk_gop_skb():
data = skb->data;
while (data < skb_tail_pointer(skb)) {
unsigned int offset = offset_in_page(data);
unsigned int len = PAGE_SIZE - offset;
[...]
It wasn't clear from the patch that this had been considered and that
any extra space needed in the grant op array was made available.
> From frontend's PoV I think the impact is minimal. Frontend is involved
> in assembling the packets. It only takes what's in the ring and chain
> them together. The operation involves copying so far is the
> __pskb_pull_tail which happens a) in rare case when there's more frags
> than frontend's MAX_SKB_FRAGS, b) when pull_to > skb_headlen which
> happens. With Xi's change the rare case a) will even be rarer than
> before as we use less slots. b) happens the same as it happens before
> Xi's change, because the pull is guarded by "if (pull_to >
> skb_headlen(skb))" and Xi's change doesn't affect skb_headlen.
>
> So overall I don't see obvious downside.
The obvious downside is it doesn't exist (in a form that can be applied
now), it hasn't been tested and I think there may well be a subtle bug
that would need a careful review or testing to confirm/deny.
You are free to work on this as a future improvements but I really don't
see why this critical bug fix needs to be delayed any further.
David
^ permalink raw reply
* [PATCH] net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
From: Thomas Petazzoni @ 2013-09-04 14:21 UTC (permalink / raw)
To: David S. Miller, netdev
Cc: linux-arm-kernel, Lior Amsalem, Gregory Clement, Ezequiel Garcia,
Willy Tarreau, Jochen De Smet, Peter Sanford, Ethan Tuttle,
Chény Yves-Gael, Ryan Press, Simon Guinot, vdonnefort,
stable
This commit fixes a long-standing bug that has been reported by many
users: on some Armada 370 platforms, only the network interface that
has been used in U-Boot to tftp the kernel works properly in
Linux. The other network interfaces can see a 'link up', but are
unable to transmit data. The reports were generally made on the Armada
370-based Mirabox, but have also been given on the Armada 370-RD
board.
The network MAC in the Armada 370/XP (supported by the mvneta driver
in Linux) has a functionality that allows it to continuously poll the
PHY and directly update the MAC configuration accordingly (speed,
duplex, etc.). The very first versions of the driver submitted for
review were using this hardware mechanism, but due to this, the driver
was not integrated with the kernel phylib. Following reviews, the
driver was changed to use the phylib, and therefore a software based
polling. In software based polling, Linux regularly talks to the PHY
over the MDIO bus, and sees if the link status has changed. If it's
the case then the adjust_link() callback of the driver is called to
update the MAC configuration accordingly.
However, it turns out that the adjust_link() callback was not
configuring the hardware in a completely correct way: while it was
setting the speed and duplex bits correctly, it wasn't telling the
hardware to actually take into account those bits rather than what the
hardware-based PHY polling mechanism has concluded. So, in fact the
adjust_link() callback was basically a no-op.
However, the network happened to be working because on the network
interfaces used by U-Boot for tftp on Armada 370 platforms because the
hardware PHY polling was enabled by the bootloader, and left enabled
by Linux. However, the second network interface not used for tftp (or
both network interfaces if the kernel is loaded from USB, NAND or SD
card) didn't had the hardware PHY polling enabled.
This patch fixes this situation by:
(1) Making sure that the hardware PHY polling is disabled by clearing
the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
register in the driver ->probe() function.
(2) Making sure that the duplex and speed selections made by the
adjust_link() callback are taken into account by clearing the
MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
MVNETA_GMAC_AUTONEG_CONFIG register.
This patch has been tested on Armada 370 Mirabox, and now both network
interfaces are usable after boot.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Willy Tarreau <w@1wt.eu>
Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
Cc: Peter Sanford <psanford@nearbuy.io>
Cc: Ethan Tuttle <ethan@ethantuttle.com>
Cc: Chény Yves-Gael <yves@cheny.fr>
Cc: Ryan Press <ryan@presslab.us>
Cc: Simon Guinot <simon.guinot@sequanux.org>
Cc: vdonnefort@lacie.com
Cc: stable@vger.kernel.org
---
David, this patch is a fix for a problem that has been here since 3.8
(when the mvneta driver was introduced), so I've Cc'ed stable@ and if
possible I'd like to patch to be included for 3.12.
---
drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index b017818..90ab292 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -138,7 +138,9 @@
#define MVNETA_GMAC_FORCE_LINK_PASS BIT(1)
#define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)
#define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)
+#define MVNETA_GMAC_AN_SPEED_EN BIT(7)
#define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)
+#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)
#define MVNETA_MIB_COUNTERS_BASE 0x3080
#define MVNETA_MIB_LATE_COLLISION 0x7c
#define MVNETA_DA_FILT_SPEC_MCAST 0x3400
@@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
/* Assign port SDMA configuration */
mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
+ /* Disable PHY polling in hardware, since we're using the
+ * kernel phylib to do this.
+ */
+ val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
+ val &= ~MVNETA_PHY_POLLING_ENABLE;
+ mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
+
mvneta_set_ucast_table(pp, -1);
mvneta_set_special_mcast_table(pp, -1);
mvneta_set_other_mcast_table(pp, -1);
@@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
MVNETA_GMAC_CONFIG_GMII_SPEED |
- MVNETA_GMAC_CONFIG_FULL_DUPLEX);
+ MVNETA_GMAC_CONFIG_FULL_DUPLEX |
+ MVNETA_GMAC_AN_SPEED_EN |
+ MVNETA_GMAC_AN_DUPLEX_EN);
if (phydev->duplex)
val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
--
1.8.1.2
^ permalink raw reply related
* [PATCH] net: mvneta: implement ->ndo_do_ioctl() to support PHY ioctls
From: Thomas Petazzoni @ 2013-09-04 14:26 UTC (permalink / raw)
To: David S. Miller, netdev
Cc: Gregory Clement, Ezequiel Garcia, Lior Amsalem, linux-arm-kernel,
Jason Cooper, Andrew Lunn
This commit implements the ->ndo_do_ioctl() operation so that the
PHY-related ioctl() calls can work from userspace, which allows
applications like mii-tool or mii-diag to do their job.
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
drivers/net/ethernet/marvell/mvneta.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 90ab292..389a854 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -2451,6 +2451,21 @@ static int mvneta_stop(struct net_device *dev)
return 0;
}
+static int mvneta_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+ struct mvneta_port *pp = netdev_priv(dev);
+ int ret;
+
+ if (!pp->phy_dev)
+ return -ENOTSUPP;
+
+ ret = phy_mii_ioctl(pp->phy_dev, ifr, cmd);
+ if (!ret)
+ mvneta_adjust_link(dev);
+
+ return ret;
+}
+
/* Ethtool methods */
/* Get settings (phy address, speed) for ethtools */
@@ -2569,6 +2584,7 @@ static const struct net_device_ops mvneta_netdev_ops = {
.ndo_change_mtu = mvneta_change_mtu,
.ndo_tx_timeout = mvneta_tx_timeout,
.ndo_get_stats64 = mvneta_get_stats64,
+ .ndo_do_ioctl = mvneta_ioctl,
};
const struct ethtool_ops mvneta_eth_tool_ops = {
--
1.8.1.2
^ permalink raw reply related
* Re: [patch] sfc: check for allocation failure
From: Ben Hutchings @ 2013-09-04 14:41 UTC (permalink / raw)
To: dan.carpenter; +Cc: linux-net-drivers, netdev, kernel-janitors, David Miller
In-Reply-To: <20130903.224939.866623192858217581.davem@davemloft.net>
On Tue, 2013-09-03 at 22:49 -0400, David Miller wrote:
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Date: Mon, 2 Sep 2013 12:04:42 +0300
>
> > It upsets static analyzers when we don't check for allocation failure.
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> This applies cleanly neither to net nor net-next, please send me a
> version that does.
Oh yes, I failed to spot the filename. That function has since been
moved to falcon.c.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v3 0/3] Send audit/procinfo/cgroup data in socket-level control message
From: Tejun Heo @ 2013-09-04 14:45 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Jan Kaluza, rgb-H+wXaHxf7aLQT0dZR+AlfA,
netdev-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
eparis-H+wXaHxf7aLQT0dZR+AlfA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn,
cgroups-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <878uzdf2xp.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
Hello, Eric.
On Wed, Sep 04, 2013 at 12:42:26AM -0700, Eric W. Biederman wrote:
> Can I just say ick, blech, barf, gag.
Gees, an awesome way to start the conversation. If your gag response
is hyper-sensitive, go see a frigging doctor. It's annoying because
you tend to go over the top while getting things wrong often enough.
Even if you don't agree, you don't have to start things this way.
> You don't require this information to be passed. You are asking people
> to suport a lot of new code for the forseeable future. The only advantage
> appears to be for short lived racy processes that don't even bother to
> make certain their message was acknowleged before exiting.
While I'm not sure whether this is *the* right appraoch, how is "we
have some pretty visible race conditions but it's probably okay" an
answer? This affects auditing and logging directly and you're saying
"ah well, the program wasn't running long enough"?
> You sent this during the merge window which is the time for code
> integration and testing not new code.
What are you talking about? It is *okay* to send new patches during
merge window even if it's headed for the next merge window. Sending
patches to maintainers doesn't mean "this should go in right now".
Maintainers are of course free to delay response or ask for
pinging/resending later but it's just stupid to accuse patch
submitters for sending patches. What the hell is that?
> By my count you have overflowed cb in struct sk_buff and are stomping on
> _skb_refdest.
>
> If you are going to go crazy and pass things is there a reason you do
> not add a patch to pass the bsd SCM_CREDS? That information seems more
> relevant in a security context and for making security decisions than
> about half the information you are passing.
You could have lost all the other paragraphs and just responded with
the above. I don't think we can extend an existing struct but maybe
how information is packed can be adjusted. That said, the proposed
split makes sense to me.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
From: Jason Cooper @ 2013-09-04 14:50 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: David S. Miller, netdev, Lior Amsalem, Jochen De Smet,
Simon Guinot, Ryan Press, vdonnefort, Ethan Tuttle, stable,
Ezequiel Garcia, Chény Yves-Gael, Gregory Clement,
Peter Sanford, Willy Tarreau, linux-arm-kernel
In-Reply-To: <1378304478-21237-1-git-send-email-thomas.petazzoni@free-electrons.com>
On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
> This commit fixes a long-standing bug that has been reported by many
> users: on some Armada 370 platforms, only the network interface that
> has been used in U-Boot to tftp the kernel works properly in
> Linux. The other network interfaces can see a 'link up', but are
> unable to transmit data. The reports were generally made on the Armada
> 370-based Mirabox, but have also been given on the Armada 370-RD
> board.
>
> The network MAC in the Armada 370/XP (supported by the mvneta driver
> in Linux) has a functionality that allows it to continuously poll the
> PHY and directly update the MAC configuration accordingly (speed,
> duplex, etc.). The very first versions of the driver submitted for
> review were using this hardware mechanism, but due to this, the driver
> was not integrated with the kernel phylib. Following reviews, the
> driver was changed to use the phylib, and therefore a software based
> polling. In software based polling, Linux regularly talks to the PHY
> over the MDIO bus, and sees if the link status has changed. If it's
> the case then the adjust_link() callback of the driver is called to
> update the MAC configuration accordingly.
>
> However, it turns out that the adjust_link() callback was not
> configuring the hardware in a completely correct way: while it was
> setting the speed and duplex bits correctly, it wasn't telling the
> hardware to actually take into account those bits rather than what the
> hardware-based PHY polling mechanism has concluded. So, in fact the
> adjust_link() callback was basically a no-op.
>
> However, the network happened to be working because on the network
> interfaces used by U-Boot for tftp on Armada 370 platforms because the
> hardware PHY polling was enabled by the bootloader, and left enabled
> by Linux. However, the second network interface not used for tftp (or
> both network interfaces if the kernel is loaded from USB, NAND or SD
> card) didn't had the hardware PHY polling enabled.
>
> This patch fixes this situation by:
>
> (1) Making sure that the hardware PHY polling is disabled by clearing
> the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
> register in the driver ->probe() function.
>
> (2) Making sure that the duplex and speed selections made by the
> adjust_link() callback are taken into account by clearing the
> MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
> MVNETA_GMAC_AUTONEG_CONFIG register.
>
> This patch has been tested on Armada 370 Mirabox, and now both network
> interfaces are usable after boot.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Willy Tarreau <w@1wt.eu>
> Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
> Cc: Peter Sanford <psanford@nearbuy.io>
> Cc: Ethan Tuttle <ethan@ethantuttle.com>
> Cc: Chény Yves-Gael <yves@cheny.fr>
> Cc: Ryan Press <ryan@presslab.us>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> Cc: vdonnefort@lacie.com
> Cc: stable@vger.kernel.org
> ---
> David, this patch is a fix for a problem that has been here since 3.8
> (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
> possible I'd like to patch to be included for 3.12.
David,
Offending patch is:
c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and v3.11
Acked-by: Jason Cooper <jason@lakedaemon.net>
thx,
Jason.
> ---
> drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b017818..90ab292 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -138,7 +138,9 @@
> #define MVNETA_GMAC_FORCE_LINK_PASS BIT(1)
> #define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)
> #define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)
> +#define MVNETA_GMAC_AN_SPEED_EN BIT(7)
> #define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)
> +#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)
> #define MVNETA_MIB_COUNTERS_BASE 0x3080
> #define MVNETA_MIB_LATE_COLLISION 0x7c
> #define MVNETA_DA_FILT_SPEC_MCAST 0x3400
> @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
> /* Assign port SDMA configuration */
> mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
>
> + /* Disable PHY polling in hardware, since we're using the
> + * kernel phylib to do this.
> + */
> + val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
> + val &= ~MVNETA_PHY_POLLING_ENABLE;
> + mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
> +
> mvneta_set_ucast_table(pp, -1);
> mvneta_set_special_mcast_table(pp, -1);
> mvneta_set_other_mcast_table(pp, -1);
> @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
> val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
> val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
> MVNETA_GMAC_CONFIG_GMII_SPEED |
> - MVNETA_GMAC_CONFIG_FULL_DUPLEX);
> + MVNETA_GMAC_CONFIG_FULL_DUPLEX |
> + MVNETA_GMAC_AN_SPEED_EN |
> + MVNETA_GMAC_AN_DUPLEX_EN);
>
> if (phydev->duplex)
> val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
> --
> 1.8.1.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next 4/5] driver/net: enic: Exposing symbols for Cisco's low latency driver
From: Ben Hutchings @ 2013-09-04 14:52 UTC (permalink / raw)
To: Govindarajulu Varadarajan
Cc: davem, netdev, linux-kernel, benve, ssujith, nistrive, umalhi
In-Reply-To: <1378273638-7780-5-git-send-email-govindarajulu90@gmail.com>
On Wed, 2013-09-04 at 11:17 +0530, Govindarajulu Varadarajan wrote:
> This patch exposes symbols for usnic low latency driver that can be used to
> register and unregister vNics as well to traverse the resources on vNics.
>
> Signed-off-by: Upinder Malhi <umalhi@cisco.com>
> Signed-off-by: Nishank Trivedi <nistrive@cisco.com>
> Signed-off-by: Christian Benvenuti <benve@cisco.com>
Will usnic, or any other user of these symbols, be submitted for
inclusion in-tree as well? It is generally expected that exported
functions do have an in-tree user.
Also, header files under drivers/ generally won't be included in
distribution -devel packages, so to support an out-of-tree module the
function prototypes would need to be included in a header under include/
(or else you have to repeat them and hope the types never change).
Ben.
> ---
> drivers/net/ethernet/cisco/enic/vnic_dev.c | 10 ++++++++++
> drivers/net/ethernet/cisco/enic/vnic_dev.h | 1 +
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.c b/drivers/net/ethernet/cisco/enic/vnic_dev.c
> index 97455c5..69dd925 100644
> --- a/drivers/net/ethernet/cisco/enic/vnic_dev.c
> +++ b/drivers/net/ethernet/cisco/enic/vnic_dev.c
> @@ -175,6 +175,7 @@ unsigned int vnic_dev_get_res_count(struct vnic_dev *vdev,
> {
> return vdev->res[type].count;
> }
> +EXPORT_SYMBOL(vnic_dev_get_res_count);
>
> void __iomem *vnic_dev_get_res(struct vnic_dev *vdev, enum vnic_res_type type,
> unsigned int index)
> @@ -193,6 +194,7 @@ void __iomem *vnic_dev_get_res(struct vnic_dev *vdev, enum vnic_res_type type,
> return (char __iomem *)vdev->res[type].vaddr;
> }
> }
> +EXPORT_SYMBOL(vnic_dev_get_res);
>
> static unsigned int vnic_dev_desc_ring_size(struct vnic_dev_ring *ring,
> unsigned int desc_count, unsigned int desc_size)
> @@ -942,6 +944,7 @@ void vnic_dev_unregister(struct vnic_dev *vdev)
> kfree(vdev);
> }
> }
> +EXPORT_SYMBOL(vnic_dev_unregister);
>
> struct vnic_dev *vnic_dev_register(struct vnic_dev *vdev,
> void *priv, struct pci_dev *pdev, struct vnic_dev_bar *bar,
> @@ -969,6 +972,13 @@ err_out:
> vnic_dev_unregister(vdev);
> return NULL;
> }
> +EXPORT_SYMBOL(vnic_dev_register);
> +
> +struct pci_dev *vnic_dev_get_pdev(struct vnic_dev *vdev)
> +{
> + return vdev->pdev;
> +}
> +EXPORT_SYMBOL(vnic_dev_get_pdev);
>
> int vnic_dev_init_prov2(struct vnic_dev *vdev, u8 *buf, u32 len)
> {
> diff --git a/drivers/net/ethernet/cisco/enic/vnic_dev.h b/drivers/net/ethernet/cisco/enic/vnic_dev.h
> index f3d9b79..e670029 100644
> --- a/drivers/net/ethernet/cisco/enic/vnic_dev.h
> +++ b/drivers/net/ethernet/cisco/enic/vnic_dev.h
> @@ -127,6 +127,7 @@ int vnic_dev_set_ig_vlan_rewrite_mode(struct vnic_dev *vdev,
> struct vnic_dev *vnic_dev_register(struct vnic_dev *vdev,
> void *priv, struct pci_dev *pdev, struct vnic_dev_bar *bar,
> unsigned int num_bars);
> +struct pci_dev *vnic_dev_get_pdev(struct vnic_dev *vdev);
> int vnic_dev_init_prov2(struct vnic_dev *vdev, u8 *buf, u32 len);
> int vnic_dev_enable2(struct vnic_dev *vdev, int active);
> int vnic_dev_enable2_done(struct vnic_dev *vdev, int *status);
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v3 0/3] Send audit/procinfo/cgroup data in socket-level control message
From: Richard Guy Briggs @ 2013-09-04 14:58 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Jan Kaluza, netdev-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
eparis-H+wXaHxf7aLQT0dZR+AlfA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, tj-DgEjT+Ai2ygdnm+yROfE0A,
cgroups-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <878uzdf2xp.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
On Wed, Sep 04, 2013 at 12:42:26AM -0700, Eric W. Biederman wrote:
> Jan Kaluza <jkaluza-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > Hi,
> >
> > this patchset against net-next (applies also to linux-next) adds 3 new types
> > of "Socket"-level control message (SCM_AUDIT, SCM_PROCINFO and SCM_CGROUP).
> >
> > Server-like processes in many cases need credentials and other
> > metadata of the peer, to decide if the calling process is allowed to
> > request a specific action, or the server just wants to log away this
> > type of information for auditing tasks.
> >
> > The current practice to retrieve such process metadata is to look that
> > information up in procfs with the $PID received over SCM_CREDENTIALS.
> > This is sufficient for long-running tasks, but introduces a race which
> > cannot be worked around for short-living processes; the calling
> > process and all the information in /proc/$PID/ is gone before the
> > receiver of the socket message can look it up.
>
> > Changes introduced in this patchset can also increase performance
> > of such server-like processes, because current way of opening and
> > parsing /proc/$PID/* files is much more expensive than receiving these
> > metadata using SCM.
>
> Can I just say ick, blech, barf, gag.
/me hands ebiederman an air sickness bag.
> You don't require this information to be passed. You are asking people
> to suport a lot of new code for the forseeable future. The only advantage
> appears to be for short lived racy processes that don't even bother to
> make certain their message was acknowleged before exiting.
>
> You sent this during the merge window which is the time for code
> integration and testing not new code.
This is an RFC. How is this important?
> By my count you have overflowed cb in struct sk_buff and are stomping on
> _skb_refdest.
For patch1/3 I count 56/48, then for patch3 I get 48/48. Jan, you might
do the conversion to a pointer in patch1/3 to avoid bisect breakage.
> If you are going to go crazy and pass things is there a reason you do
> not add a patch to pass the bsd SCM_CREDS? That information seems more
> relevant in a security context and for making security decisions than
> about half the information you are passing.
>
> Eric
- RGB
--
Richard Guy Briggs <rbriggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH net-next v2 1/6] bonding: simplify and use RCU protection for 3ad xmit path
From: Ding Tianhong @ 2013-09-04 14:53 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
In-Reply-To: <20130904101823.GO1992@redhat.com>
于 2013/9/4 18:18, Veaceslav Falico 写道:
> On Wed, Sep 04, 2013 at 05:43:45PM +0800, Ding Tianhong wrote:
> ...snip...
>> +/**
>> + * IMPORTANT: bond_first/last_slave_rcu can return NULL in case of
>> an empty list
>> + * Caller must hold rcu_read_lock
>> + */
>> +#define bond_first_slave_rcu(bond) \
>> + list_first_or_null_rcu(&(bond)->slave_list, struct slave, list)
>> +#define bond_last_slave_rcu(bond) \
>> + (list_empty(&(bond)->slave_list) ? NULL : \
>> + bond_to_slave_rcu((bond)->slave_list.prev))
>
> Here, bond_last_slave_rcu() is racy. The list can be non-empty when
> list_empty() is verified, however afterwards it might become empty, when
> you call bond_to_slave_rcu(), and thus you'll get
> bond_to_slave(bond->slave_list) in the result, which is not a slave.
>
> Take a look at list_first_or_null_rcu() for a reference. The main idea is
> that it first gets the ->next pointer, with RCU protection, and then
> verifies if it's the list head or not, and if not - it gets the container
> already. This way the ->next pointer won't get away.
>
> These kind of bugs are really rare, but are *EXTREMELY* hard to debug.
Thanks for your response and opinions, but I think your miss something,
the slave_list will not changed in the rcu_read_lock, so ,the bugs will not
happen.
>
>> +
>> #define bond_is_first_slave(bond, pos) ((pos)->list.prev ==
>> &(bond)->slave_list)
>> #define bond_is_last_slave(bond, pos) ((pos)->list.next ==
>> &(bond)->slave_list)
>>
>> @@ -93,6 +106,15 @@
>> (bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
>> bond_to_slave((pos)->list.prev))
>>
>> +/* Since bond_first/last_slave_rcu can return NULL, these can return
>> NULL too */
>> +#define bond_next_slave_rcu(bond, pos) \
>> + (bond_is_last_slave(bond, pos) ? bond_first_slave_rcu(bond) : \
>> + bond_to_slave_rcu((pos)->list.next))
>> +
>> +#define bond_prev_slave_rcu(bond, pos) \
>> + (bond_is_first_slave(bond, pos) ? bond_last_slave_rcu(bond) : \
>> + bond_to_slave_rcu((pos)->list.prev))
>> +
>
> These two are also racy. bond_is_last/first_slave() is not rcu-ified, and
> thus you can't rely on it without proper locking. Same ideas apply as per
> bond_first_slave_rcu().
> --
refer to the above answer.
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v3 0/3] Send audit/procinfo/cgroup data in socket-level control message
From: Jan Kaluža @ 2013-09-04 15:04 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Eric W. Biederman, davem-fT/PcQaiUtIeIZ0/mPfg9Q, LKML,
netdev-u79uwXL29TY76Z2rM5mHXA, eparis-H+wXaHxf7aLQT0dZR+AlfA,
tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <20130904145830.GC28517-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On 09/04/2013 04:58 PM, Richard Guy Briggs wrote:
> On Wed, Sep 04, 2013 at 12:42:26AM -0700, Eric W. Biederman wrote:
>> Jan Kaluza <jkaluza-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>>> Hi,
>>>
>>> this patchset against net-next (applies also to linux-next) adds 3 new types
>>> of "Socket"-level control message (SCM_AUDIT, SCM_PROCINFO and SCM_CGROUP).
>>>
>>> Server-like processes in many cases need credentials and other
>>> metadata of the peer, to decide if the calling process is allowed to
>>> request a specific action, or the server just wants to log away this
>>> type of information for auditing tasks.
>>>
>>> The current practice to retrieve such process metadata is to look that
>>> information up in procfs with the $PID received over SCM_CREDENTIALS.
>>> This is sufficient for long-running tasks, but introduces a race which
>>> cannot be worked around for short-living processes; the calling
>>> process and all the information in /proc/$PID/ is gone before the
>>> receiver of the socket message can look it up.
>>
>>> Changes introduced in this patchset can also increase performance
>>> of such server-like processes, because current way of opening and
>>> parsing /proc/$PID/* files is much more expensive than receiving these
>>> metadata using SCM.
>>
>> Can I just say ick, blech, barf, gag.
>
> /me hands ebiederman an air sickness bag.
>
>> You don't require this information to be passed. You are asking people
>> to suport a lot of new code for the forseeable future. The only advantage
>> appears to be for short lived racy processes that don't even bother to
>> make certain their message was acknowleged before exiting.
>>
>> You sent this during the merge window which is the time for code
>> integration and testing not new code.
>
> This is an RFC. How is this important?
>
>> By my count you have overflowed cb in struct sk_buff and are stomping on
>> _skb_refdest.
>
> For patch1/3 I count 56/48, then for patch3 I get 48/48. Jan, you might
> do the conversion to a pointer in patch1/3 to avoid bisect breakage.
Yes, this is valid point. I will do the conversion in patch1. Thanks all
for reviewing and pointing that out.
Jan Kaluza
>> If you are going to go crazy and pass things is there a reason you do
>> not add a patch to pass the bsd SCM_CREDS? That information seems more
>> relevant in a security context and for making security decisions than
>> about half the information you are passing.
>>
>> Eric
>
> - RGB
>
> --
> Richard Guy Briggs <rbriggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Senior Software Engineer
> Kernel Security
> AMER ENG Base Operating Systems
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635
> Internal: (81) 32635
> Alt: +1.613.693.0684x3545
>
^ permalink raw reply
* [patch v2] sfc: check for allocation failure
From: Dan Carpenter @ 2013-09-04 15:07 UTC (permalink / raw)
To: Solarflare linux maintainers; +Cc: Ben Hutchings, netdev, kernel-janitors
In-Reply-To: <20130903.224939.866623192858217581.davem@davemloft.net>
It upsets static analyzers when we don't check for allocation failure.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: rebased on latest linux-next
diff --git a/drivers/net/ethernet/sfc/falcon.c b/drivers/net/ethernet/sfc/falcon.c
index 38d179c..75799f8 100644
--- a/drivers/net/ethernet/sfc/falcon.c
+++ b/drivers/net/ethernet/sfc/falcon.c
@@ -894,6 +894,8 @@ static int falcon_mtd_probe(struct efx_nic *efx)
/* Allocate space for maximum number of partitions */
parts = kcalloc(2, sizeof(*parts), GFP_KERNEL);
+ if (!parts)
+ return -ENOMEM;
n_parts = 0;
spi = &nic_data->spi_flash;
^ permalink raw reply related
* Re: [PATCH net-next v2 5/6] bonding: restructure and add rcu for bond_for_each_slave_next()
From: Ding Tianhong @ 2013-09-04 15:02 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Ding Tianhong, Jay Vosburgh, Andy Gospodarek, David S. Miller,
Nikolay Aleksandrov, Netdev
In-Reply-To: <20130904103540.GP1992@redhat.com>
于 2013/9/4 18:35, Veaceslav Falico 写道:
> On Wed, Sep 04, 2013 at 05:44:15PM +0800, Ding Tianhong wrote:
> ...snip...
>> +/* Check whether the slave is the only one in bond */
>> +#define bond_is_only_slave(bond, pos) \
>> + (((pos)->list.prev == &(bond)->slave_list) && \
>> + ((pos)->list.next == &(bond)->slave_list))
>
> Could be done without pos at all -
>
> !list_empty(&(bond)->slave_list) && \
> &(bond)->slave_list.next == &(bond)->slave_list.prev
>
> If we have only one slave and pos is NOT our slave then... well.. we have
> big troubles.
>
yes, more simple more beautiful, thanks.
but if the pos is not our slave, it is the mistake, not bug. :)
>> +
>> /**
>> * bond_for_each_slave_from - iterate the slaves list from a starting
>> point
>> * @bond: the bond holding this list.
>> * @pos: current slave.
>> - * @cnt: counter for max number of moves
>> * @start: starting point.
>> *
>> * Caller must hold bond->lock
>> */
>> -#define bond_for_each_slave_from(bond, pos, cnt, start) \
>> - for (cnt = 0, pos = start; pos && cnt < (bond)->slave_cnt; \
>> - cnt++, pos = bond_next_slave(bond, pos))
>> +#define bond_for_each_slave_from(bond, pos, start) \
>> + for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
>> + &pos->list != &bond->slave_list : \
>> + &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
>> + (pos = list_entry(pos->list.next, typeof(*pos), list)) : \
>> + (pos = bond_next_slave(bond, pos)))
>
> Did you check that?
>
> pos = slave1 (bond has more than one slave);
> pos && &pos->list != &slave1->list - false.
>
> We won't ever enter this loop if we have >1 slaves.
>
> I don't understand this at all.
>
ok, the logic is : if slaves == 1, run once for the slave.
if slaves > 1, run loops until reach the end list.
I could not get a better way to simplify the function, I think it is a
suitable scheme.
by the way, I test the function and works well. :)
>> +
>> +/**
>> + * bond_for_each_slave_from_rcu - iterate the slaves list from a
>> starting point
>> + * @bond: the bond holding this list.
>> + * @pos: current slave.
>> + * @start: starting point.
>> + *
>> + * Caller must hold rcu_read_lock
>> + */
>> +#define bond_for_each_slave_from_rcu(bond, pos, start) \
>> + for (pos = start; pos && (bond_is_only_slave(bond, start) ? \
>> + &pos->list != &bond->slave_list : \
>> + &pos->list != &start->list); bond_is_only_slave(bond, start) ? \
>> + (pos = list_entry_rcu(pos->list.next, typeof(*pos), list)) : \
>> + (pos = bond_next_slave_rcu(bond, pos)))
>
> Ditto as bond_for_each_slave_from() and, also, see my comment about RCU
> from patch 1.
>
>>
>> /**
>> * bond_for_each_slave - iterate over all slaves
>> --
>> 1.8.2.1
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH] net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
From: Vincent Donnefort @ 2013-09-04 15:20 UTC (permalink / raw)
To: Jason Cooper
Cc: Thomas Petazzoni, Lior Amsalem, Jochen De Smet, Simon Guinot,
Ryan Press, netdev, stable, Willy Tarreau, Ethan Tuttle,
Ezequiel Garcia, Chény Yves-Gael, Gregory Clement,
Peter Sanford, David S. Miller, linux-arm-kernel
In-Reply-To: <20130904145051.GO19598@titan.lakedaemon.net>
On Wed, Sep 04, 2013 at 10:50:51AM -0400, Jason Cooper wrote:
> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
> > This commit fixes a long-standing bug that has been reported by many
> > users: on some Armada 370 platforms, only the network interface that
> > has been used in U-Boot to tftp the kernel works properly in
> > Linux. The other network interfaces can see a 'link up', but are
> > unable to transmit data. The reports were generally made on the Armada
> > 370-based Mirabox, but have also been given on the Armada 370-RD
> > board.
> >
> > The network MAC in the Armada 370/XP (supported by the mvneta driver
> > in Linux) has a functionality that allows it to continuously poll the
> > PHY and directly update the MAC configuration accordingly (speed,
> > duplex, etc.). The very first versions of the driver submitted for
> > review were using this hardware mechanism, but due to this, the driver
> > was not integrated with the kernel phylib. Following reviews, the
> > driver was changed to use the phylib, and therefore a software based
> > polling. In software based polling, Linux regularly talks to the PHY
> > over the MDIO bus, and sees if the link status has changed. If it's
> > the case then the adjust_link() callback of the driver is called to
> > update the MAC configuration accordingly.
> >
> > However, it turns out that the adjust_link() callback was not
> > configuring the hardware in a completely correct way: while it was
> > setting the speed and duplex bits correctly, it wasn't telling the
> > hardware to actually take into account those bits rather than what the
> > hardware-based PHY polling mechanism has concluded. So, in fact the
> > adjust_link() callback was basically a no-op.
> >
> > However, the network happened to be working because on the network
> > interfaces used by U-Boot for tftp on Armada 370 platforms because the
> > hardware PHY polling was enabled by the bootloader, and left enabled
> > by Linux. However, the second network interface not used for tftp (or
> > both network interfaces if the kernel is loaded from USB, NAND or SD
> > card) didn't had the hardware PHY polling enabled.
> >
> > This patch fixes this situation by:
> >
> > (1) Making sure that the hardware PHY polling is disabled by clearing
> > the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
> > register in the driver ->probe() function.
> >
> > (2) Making sure that the duplex and speed selections made by the
> > adjust_link() callback are taken into account by clearing the
> > MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
> > MVNETA_GMAC_AUTONEG_CONFIG register.
> >
> > This patch has been tested on Armada 370 Mirabox, and now both network
> > interfaces are usable after boot.
> >
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Cc: Willy Tarreau <w@1wt.eu>
> > Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
> > Cc: Peter Sanford <psanford@nearbuy.io>
> > Cc: Ethan Tuttle <ethan@ethantuttle.com>
> > Cc: Chény Yves-Gael <yves@cheny.fr>
> > Cc: Ryan Press <ryan@presslab.us>
> > Cc: Simon Guinot <simon.guinot@sequanux.org>
> > Cc: vdonnefort@lacie.com
> > Cc: stable@vger.kernel.org
> > ---
> > David, this patch is a fix for a problem that has been here since 3.8
> > (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
> > possible I'd like to patch to be included for 3.12.
>
> David,
>
> Offending patch is:
>
> c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
>
> Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and v3.11
>
> Acked-by: Jason Cooper <jason@lakedaemon.net>
>
> thx,
>
> Jason.
Works with the armada370-rd board.
Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
Thank you!
Vincent.
>
> > ---
> > drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> > index b017818..90ab292 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -138,7 +138,9 @@
> > #define MVNETA_GMAC_FORCE_LINK_PASS BIT(1)
> > #define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)
> > #define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)
> > +#define MVNETA_GMAC_AN_SPEED_EN BIT(7)
> > #define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)
> > +#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)
> > #define MVNETA_MIB_COUNTERS_BASE 0x3080
> > #define MVNETA_MIB_LATE_COLLISION 0x7c
> > #define MVNETA_DA_FILT_SPEC_MCAST 0x3400
> > @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
> > /* Assign port SDMA configuration */
> > mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
> >
> > + /* Disable PHY polling in hardware, since we're using the
> > + * kernel phylib to do this.
> > + */
> > + val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
> > + val &= ~MVNETA_PHY_POLLING_ENABLE;
> > + mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
> > +
> > mvneta_set_ucast_table(pp, -1);
> > mvneta_set_special_mcast_table(pp, -1);
> > mvneta_set_other_mcast_table(pp, -1);
> > @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
> > val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
> > val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
> > MVNETA_GMAC_CONFIG_GMII_SPEED |
> > - MVNETA_GMAC_CONFIG_FULL_DUPLEX);
> > + MVNETA_GMAC_CONFIG_FULL_DUPLEX |
> > + MVNETA_GMAC_AN_SPEED_EN |
> > + MVNETA_GMAC_AN_DUPLEX_EN);
> >
> > if (phydev->duplex)
> > val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
> > --
> > 1.8.1.2
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Vincent
LaCie will welcome you at IBC Amsterdam (13-17 Sept) on booth 7.G17 (Hall7). Come and visit us.
^ permalink raw reply
* Re: [PATCH v3 0/3] Send audit/procinfo/cgroup data in socket-level control message
From: Richard Guy Briggs @ 2013-09-04 15:20 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Jan Kaluza, davem-fT/PcQaiUtIeIZ0/mPfg9Q, LKML,
netdev-u79uwXL29TY76Z2rM5mHXA, eparis-H+wXaHxf7aLQT0dZR+AlfA,
tj-DgEjT+Ai2ygdnm+yROfE0A, lizefan-hv44wF8Li93QT0dZR+AlfA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <20130904145830.GC28517-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On Wed, Sep 04, 2013 at 10:58:30AM -0400, Richard Guy Briggs wrote:
> On Wed, Sep 04, 2013 at 12:42:26AM -0700, Eric W. Biederman wrote:
> > Jan Kaluza <jkaluza-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > > this patchset against net-next (applies also to linux-next) adds 3 new types
> > > of "Socket"-level control message (SCM_AUDIT, SCM_PROCINFO and SCM_CGROUP).
>
> > By my count you have overflowed cb in struct sk_buff and are stomping on
> > _skb_refdest.
>
> For patch1/3 I count 56/48, then for patch3 I get 48/48. Jan, you might
> do the conversion to a pointer in patch1/3 to avoid bisect breakage.
Wait, that __aligned(8) is for cb[48], not for the contents.
For patch1/3 I count 28/48 on 32-bit, 36/48 on 64-bit (or would that be
56 by default on 64-bit arches without aligned specified?), then for
patch3 I get 24/48 on 32 and 40/48 on 64 (or again 48/48 by default?).
> > Eric
>
> - RGB
- RGB
--
Richard Guy Briggs <rbriggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH v3 0/3] Send audit/procinfo/cgroup data in socket-level control message
From: Eric Dumazet @ 2013-09-04 15:30 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Jan Kaluza, netdev-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
eparis-H+wXaHxf7aLQT0dZR+AlfA, Eric W. Biederman,
tj-DgEjT+Ai2ygdnm+yROfE0A, cgroups-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <20130904152022.GD28517-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On Wed, 2013-09-04 at 11:20 -0400, Richard Guy Briggs wrote:
> On Wed, Sep 04, 2013 at 10:58:30AM -0400, Richard Guy Briggs wrote:
> > On Wed, Sep 04, 2013 at 12:42:26AM -0700, Eric W. Biederman wrote:
> > > Jan Kaluza <jkaluza-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
> > > > this patchset against net-next (applies also to linux-next) adds 3 new types
> > > > of "Socket"-level control message (SCM_AUDIT, SCM_PROCINFO and SCM_CGROUP).
> >
> > > By my count you have overflowed cb in struct sk_buff and are stomping on
> > > _skb_refdest.
> >
> > For patch1/3 I count 56/48, then for patch3 I get 48/48. Jan, you might
> > do the conversion to a pointer in patch1/3 to avoid bisect breakage.
>
> Wait, that __aligned(8) is for cb[48], not for the contents.
>
> For patch1/3 I count 28/48 on 32-bit, 36/48 on 64-bit (or would that be
> 56 by default on 64-bit arches without aligned specified?), then for
> patch3 I get 24/48 on 32 and 40/48 on 64 (or again 48/48 by default?).
Do not count, just add this :
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 86de99a..5b61320 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1329,6 +1329,7 @@ static void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb)
{
int i;
+ BUILD_BUG_ON(sizeof(struct unix_skb_parms) > sizeof(skb->cb));
scm->fp = UNIXCB(skb).fp;
UNIXCB(skb).fp = NULL;
^ permalink raw reply related
* Re: [PATCH v3 0/3] Send audit/procinfo/cgroup data in socket-level control message
From: Jan Kaluža @ 2013-09-04 15:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Richard Guy Briggs, Eric W. Biederman,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, LKML, netdev-u79uwXL29TY76Z2rM5mHXA,
eparis-H+wXaHxf7aLQT0dZR+AlfA, tj-DgEjT+Ai2ygdnm+yROfE0A,
lizefan-hv44wF8Li93QT0dZR+AlfA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <1378308621.7360.110.camel@edumazet-glaptop>
On 09/04/2013 05:30 PM, Eric Dumazet wrote:
> On Wed, 2013-09-04 at 11:20 -0400, Richard Guy Briggs wrote:
>> On Wed, Sep 04, 2013 at 10:58:30AM -0400, Richard Guy Briggs wrote:
>>> On Wed, Sep 04, 2013 at 12:42:26AM -0700, Eric W. Biederman wrote:
>>>> Jan Kaluza <jkaluza-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>>>>> this patchset against net-next (applies also to linux-next) adds 3 new types
>>>>> of "Socket"-level control message (SCM_AUDIT, SCM_PROCINFO and SCM_CGROUP).
>>>
>>>> By my count you have overflowed cb in struct sk_buff and are stomping on
>>>> _skb_refdest.
>>>
>>> For patch1/3 I count 56/48, then for patch3 I get 48/48. Jan, you might
>>> do the conversion to a pointer in patch1/3 to avoid bisect breakage.
>>
>> Wait, that __aligned(8) is for cb[48], not for the contents.
>>
>> For patch1/3 I count 28/48 on 32-bit, 36/48 on 64-bit (or would that be
>> 56 by default on 64-bit arches without aligned specified?), then for
>> patch3 I get 24/48 on 32 and 40/48 on 64 (or again 48/48 by default?).
>
> Do not count, just add this :
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 86de99a..5b61320 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1329,6 +1329,7 @@ static void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb)
> {
> int i;
>
> + BUILD_BUG_ON(sizeof(struct unix_skb_parms) > sizeof(skb->cb));
> scm->fp = UNIXCB(skb).fp;
> UNIXCB(skb).fp = NULL;
>
That's already in af_unix.c:
BUILD_BUG_ON(sizeof(struct unix_skb_parms) > FIELD_SIZEOF(struct
sk_buff, cb));
This test is passing for me even with only patch1/3 applied when
building 64bit kernel.
Jan Kaluza
^ permalink raw reply
* Re: [PATCH] xen-netback: count number required slots for an skb more carefully
From: Wei Liu @ 2013-09-04 15:44 UTC (permalink / raw)
To: David Vrabel
Cc: Wei Liu, xen-devel, Konrad Rzeszutek Wilk, Boris Ostrovsky,
Ian Campbell, netdev, msw, annie.li
In-Reply-To: <52273D59.2020205@citrix.com>
On Wed, Sep 04, 2013 at 03:02:01PM +0100, David Vrabel wrote:
[...]
> >>
> >> I think I prefer fixing the counting for backporting to stable kernels.
> >
> > The original patch has coding style change. Sans that contextual change
> > it's not a very long patch.
>
> The size of the patch isn't the main concern for backport-ability. It's
> the frontend visible changes and thus any (unexpected) impacts on
> frontends -- this is especially important as only a small fraction of
> frontends in use will be tested with these changes.
>
> >> Xi's approach of packing the ring differently is a change in frontend
> >> visible behaviour and seems more risky. e.g., possible performance
> >> impact so I would like to see some performance analysis of that approach.
> >>
> >
> > With Xi's approach it is more efficient for backend to process. As we
> > now use one less grant copy operation which means we copy the same
> > amount of data with less grant ops.
>
> It think it uses more grant ops because the copies of the linear
> portion are in chunks that do not cross source page boundaries.
>
> i.e., in netbk_gop_skb():
>
> data = skb->data;
> while (data < skb_tail_pointer(skb)) {
> unsigned int offset = offset_in_page(data);
> unsigned int len = PAGE_SIZE - offset;
> [...]
>
> It wasn't clear from the patch that this had been considered and that
> any extra space needed in the grant op array was made available.
>
If I'm not mistaken the grant op array is already enormous. See the
comment in struct xen_netbk for grant_copy_op. The case that a buffer
straddles two slots was taken into consideration long ago -- that's
why you don't see any comment or code change WRT that...
> > From frontend's PoV I think the impact is minimal. Frontend is involved
> > in assembling the packets. It only takes what's in the ring and chain
> > them together. The operation involves copying so far is the
> > __pskb_pull_tail which happens a) in rare case when there's more frags
> > than frontend's MAX_SKB_FRAGS, b) when pull_to > skb_headlen which
> > happens. With Xi's change the rare case a) will even be rarer than
> > before as we use less slots. b) happens the same as it happens before
> > Xi's change, because the pull is guarded by "if (pull_to >
> > skb_headlen(skb))" and Xi's change doesn't affect skb_headlen.
> >
> > So overall I don't see obvious downside.
>
> The obvious downside is it doesn't exist (in a form that can be applied
> now), it hasn't been tested and I think there may well be a subtle bug
> that would need a careful review or testing to confirm/deny.
>
It does exist and apply cleanly on top of net tree. I haven't posted
it yet because we haven't reached concensus which path to take. :-)
The only reason that last version didn't get upstreamed is that the
commit message wasn't clear enough. From the technical PoV it's quite
sound and I believe Amazon has been using it for a long time -- the
older reference dates back to Aug 2012 IIRC. It's just never properly
upstreamed.
> You are free to work on this as a future improvements but I really don't
> see why this critical bug fix needs to be delayed any further.
>
True. I don't mean to hold off critical fix. Just want to make sure that
every option is presented and considered.
Wei.
> David
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] random: add prandom_u32_range and prandom_u32_max helpers
From: Joe Perches @ 2013-09-04 15:54 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, netdev, linux-kernel, Theodore Ts'o
In-Reply-To: <1378298247-29364-2-git-send-email-dborkman@redhat.com>
On Wed, 2013-09-04 at 14:37 +0200, Daniel Borkmann wrote:
> We have implemented the same function over and over, so introduce
> generic helpers that unify these implementations in order to migrate
> such code to use them. Make the API similarly to randomize_range()
> for consistency. prandom_u32_range() generates numbers in [start, end]
> interval and prandom_u32_max() generates numbers in [0, end] interval.
I think these helpers can in many cases cause
poorer compiler generated object code.
> +/**
> + * prandom_u32_range - return a random number in interval [start, end]
> + * @start: lower interval endpoint
> + * @end: higher interval endpoint
> + *
> + * Returns a number that is in the given interval:
> + *
> + * [...... <range> .....]
> + * start end
> + *
> + * Callers need to make sure that start <= end. Note that the result
> + * depends on PRNG being well distributed in [0, ~0U] space. Here we
> + * use maximally equidistributed combined Tausworthe generator.
> + */
> +static inline u32 prandom_u32_range(u32 start, u32 end)
> +{
> + return (u32)(((u64) prandom_u32() * (end + 1 - start)) >> 32) + start;
> +}
This is effectively:
return (prandom_u32() % (end - start)) + start;
and if start and end are constant, gcc can optimize the
division by constant to a 32 bit multiply/shift/add.
I think if you add __builtin_constant_p tests for start
and end and expand the code a little you can still get
the optimizations done.
^ permalink raw reply
* Re: [PATCH net-next 1/2] tuntap: purge socket error queue on detach
From: Richard Cochran @ 2013-09-04 15:56 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst
In-Reply-To: <1378269226-5969-1-git-send-email-jasowang@redhat.com>
On Wed, Sep 04, 2013 at 12:33:45PM +0800, Jason Wang wrote:
> Commit eda297729171fe16bf34fe5b0419dfb69060f623
> (tun: Support software transmit time stamping) will queue skbs into error queue
> when tx stamping is enabled. But it forgets to purge the error queue during
> detach. This patch fixes this.
>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 2/2] tuntap: orphan frags before trying to set tx timestamp
From: Richard Cochran @ 2013-09-04 15:57 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst
In-Reply-To: <1378269226-5969-2-git-send-email-jasowang@redhat.com>
On Wed, Sep 04, 2013 at 12:33:46PM +0800, Jason Wang wrote:
> sock_tx_timestamp() will clear all zerocopy flags of skb which may lead the
> frags never to be orphaned. This will break guest to guest traffic when zerocopy
> is enabled. Fix this by orphaning the frags before trying to set tx time stamp.
>
> The issue were introduced by commit eda297729171fe16bf34fe5b0419dfb69060f623
> (tun: Support software transmit time stamping).
>
> Cc: Richard Cochran <richardcochran@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* Re: [PATCH] net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
From: yves @ 2013-09-04 16:00 UTC (permalink / raw)
To: Vincent Donnefort
Cc: Thomas Petazzoni, Lior Amsalem, Jochen De Smet, Jason Cooper,
Ryan Press, netdev, stable, Willy Tarreau, Ethan Tuttle,
Ezequiel Garcia, Gregory Clement, Peter Sanford, David S. Miller,
linux-arm-kernel, Simon Guinot
In-Reply-To: <20130904152018.GA21032@grp-vdonnefort>
Le 2013-09-04 17:20, Vincent Donnefort a écrit :
> On Wed, Sep 04, 2013 at 10:50:51AM -0400, Jason Cooper wrote:
>> On Wed, Sep 04, 2013 at 04:21:18PM +0200, Thomas Petazzoni wrote:
>> > This commit fixes a long-standing bug that has been reported by many
>> > users: on some Armada 370 platforms, only the network interface that
>> > has been used in U-Boot to tftp the kernel works properly in
>> > Linux. The other network interfaces can see a 'link up', but are
>> > unable to transmit data. The reports were generally made on the Armada
>> > 370-based Mirabox, but have also been given on the Armada 370-RD
>> > board.
>> >
>> > The network MAC in the Armada 370/XP (supported by the mvneta driver
>> > in Linux) has a functionality that allows it to continuously poll the
>> > PHY and directly update the MAC configuration accordingly (speed,
>> > duplex, etc.). The very first versions of the driver submitted for
>> > review were using this hardware mechanism, but due to this, the driver
>> > was not integrated with the kernel phylib. Following reviews, the
>> > driver was changed to use the phylib, and therefore a software based
>> > polling. In software based polling, Linux regularly talks to the PHY
>> > over the MDIO bus, and sees if the link status has changed. If it's
>> > the case then the adjust_link() callback of the driver is called to
>> > update the MAC configuration accordingly.
>> >
>> > However, it turns out that the adjust_link() callback was not
>> > configuring the hardware in a completely correct way: while it was
>> > setting the speed and duplex bits correctly, it wasn't telling the
>> > hardware to actually take into account those bits rather than what the
>> > hardware-based PHY polling mechanism has concluded. So, in fact the
>> > adjust_link() callback was basically a no-op.
>> >
>> > However, the network happened to be working because on the network
>> > interfaces used by U-Boot for tftp on Armada 370 platforms because the
>> > hardware PHY polling was enabled by the bootloader, and left enabled
>> > by Linux. However, the second network interface not used for tftp (or
>> > both network interfaces if the kernel is loaded from USB, NAND or SD
>> > card) didn't had the hardware PHY polling enabled.
>> >
>> > This patch fixes this situation by:
>> >
>> > (1) Making sure that the hardware PHY polling is disabled by clearing
>> > the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
>> > register in the driver ->probe() function.
>> >
>> > (2) Making sure that the duplex and speed selections made by the
>> > adjust_link() callback are taken into account by clearing the
>> > MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
>> > MVNETA_GMAC_AUTONEG_CONFIG register.
>> >
>> > This patch has been tested on Armada 370 Mirabox, and now both network
>> > interfaces are usable after boot.
>> >
>> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> > Cc: Willy Tarreau <w@1wt.eu>
>> > Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
>> > Cc: Peter Sanford <psanford@nearbuy.io>
>> > Cc: Ethan Tuttle <ethan@ethantuttle.com>
>> > Cc: Chény Yves-Gael <yves@cheny.fr>
>> > Cc: Ryan Press <ryan@presslab.us>
>> > Cc: Simon Guinot <simon.guinot@sequanux.org>
>> > Cc: vdonnefort@lacie.com
>> > Cc: stable@vger.kernel.org
>> > ---
>> > David, this patch is a fix for a problem that has been here since 3.8
>> > (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
>> > possible I'd like to patch to be included for 3.12.
>>
>> David,
>>
>> Offending patch is:
>>
>> c5aff18 net: mvneta: driver for Marvell Armada 370/XP network unit
>>
>> Applies and builds cleanly against v3.8.13, v3.9.11, v3.10.10, and
>> v3.11
>>
>> Acked-by: Jason Cooper <jason@lakedaemon.net>
>>
>> thx,
>>
>> Jason.
>
> Works with the armada370-rd board.
>
> Tested-by: Vincent Donnefort <vdonnefort@gmail.com>
>
> Thank you!
>
> Vincent.
>
Works with the mirabox armada370 devkit.
Tested-by: Yves-Gael Cheny <yves@cheny.fr>
Many thx,
Yves-Gaël .
>>
>> > ---
>> > drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
>> > 1 file changed, 12 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
>> > index b017818..90ab292 100644
>> > --- a/drivers/net/ethernet/marvell/mvneta.c
>> > +++ b/drivers/net/ethernet/marvell/mvneta.c
>> > @@ -138,7 +138,9 @@
>> > #define MVNETA_GMAC_FORCE_LINK_PASS BIT(1)
>> > #define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)
>> > #define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)
>> > +#define MVNETA_GMAC_AN_SPEED_EN BIT(7)
>> > #define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)
>> > +#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)
>> > #define MVNETA_MIB_COUNTERS_BASE 0x3080
>> > #define MVNETA_MIB_LATE_COLLISION 0x7c
>> > #define MVNETA_DA_FILT_SPEC_MCAST 0x3400
>> > @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
>> > /* Assign port SDMA configuration */
>> > mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
>> >
>> > + /* Disable PHY polling in hardware, since we're using the
>> > + * kernel phylib to do this.
>> > + */
>> > + val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
>> > + val &= ~MVNETA_PHY_POLLING_ENABLE;
>> > + mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
>> > +
>> > mvneta_set_ucast_table(pp, -1);
>> > mvneta_set_special_mcast_table(pp, -1);
>> > mvneta_set_other_mcast_table(pp, -1);
>> > @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
>> > val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
>> > val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
>> > MVNETA_GMAC_CONFIG_GMII_SPEED |
>> > - MVNETA_GMAC_CONFIG_FULL_DUPLEX);
>> > + MVNETA_GMAC_CONFIG_FULL_DUPLEX |
>> > + MVNETA_GMAC_AN_SPEED_EN |
>> > + MVNETA_GMAC_AN_DUPLEX_EN);
>> >
>> > if (phydev->duplex)
>> > val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
>> > --
>> > 1.8.1.2
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] net: mvneta: properly disable HW PHY polling and ensure adjust_link() works
From: Gregory CLEMENT @ 2013-09-04 16:08 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: David S. Miller, netdev, Lior Amsalem, Jochen De Smet,
Simon Guinot, Ryan Press, vdonnefort, Ethan Tuttle, stable,
Ezequiel Garcia, Chény Yves-Gael, Peter Sanford,
Willy Tarreau, linux-arm-kernel
In-Reply-To: <1378304478-21237-1-git-send-email-thomas.petazzoni@free-electrons.com>
On 04/09/2013 16:21, Thomas Petazzoni wrote:
> This commit fixes a long-standing bug that has been reported by many
> users: on some Armada 370 platforms, only the network interface that
> has been used in U-Boot to tftp the kernel works properly in
> Linux. The other network interfaces can see a 'link up', but are
> unable to transmit data. The reports were generally made on the Armada
> 370-based Mirabox, but have also been given on the Armada 370-RD
> board.
>
> The network MAC in the Armada 370/XP (supported by the mvneta driver
> in Linux) has a functionality that allows it to continuously poll the
> PHY and directly update the MAC configuration accordingly (speed,
> duplex, etc.). The very first versions of the driver submitted for
> review were using this hardware mechanism, but due to this, the driver
> was not integrated with the kernel phylib. Following reviews, the
> driver was changed to use the phylib, and therefore a software based
> polling. In software based polling, Linux regularly talks to the PHY
> over the MDIO bus, and sees if the link status has changed. If it's
> the case then the adjust_link() callback of the driver is called to
> update the MAC configuration accordingly.
>
> However, it turns out that the adjust_link() callback was not
> configuring the hardware in a completely correct way: while it was
> setting the speed and duplex bits correctly, it wasn't telling the
> hardware to actually take into account those bits rather than what the
> hardware-based PHY polling mechanism has concluded. So, in fact the
> adjust_link() callback was basically a no-op.
>
> However, the network happened to be working because on the network
> interfaces used by U-Boot for tftp on Armada 370 platforms because the
> hardware PHY polling was enabled by the bootloader, and left enabled
> by Linux. However, the second network interface not used for tftp (or
> both network interfaces if the kernel is loaded from USB, NAND or SD
> card) didn't had the hardware PHY polling enabled.
>
> This patch fixes this situation by:
>
> (1) Making sure that the hardware PHY polling is disabled by clearing
> the MVNETA_PHY_POLLING_ENABLE bit in the MVNETA_UNIT_CONTROL
> register in the driver ->probe() function.
>
> (2) Making sure that the duplex and speed selections made by the
> adjust_link() callback are taken into account by clearing the
> MVNETA_GMAC_AN_SPEED_EN and MVNETA_GMAC_AN_DUPLEX_EN bits in the
> MVNETA_GMAC_AUTONEG_CONFIG register.
>
> This patch has been tested on Armada 370 Mirabox, and now both network
> interfaces are usable after boot.
>
Well done Thomas!
I have successfully tested it on Armada 370 Mirabox:
Tested-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Thanks
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Willy Tarreau <w@1wt.eu>
> Cc: Jochen De Smet <jochen.armkernel@leahnim.org>
> Cc: Peter Sanford <psanford@nearbuy.io>
> Cc: Ethan Tuttle <ethan@ethantuttle.com>
> Cc: Chény Yves-Gael <yves@cheny.fr>
> Cc: Ryan Press <ryan@presslab.us>
> Cc: Simon Guinot <simon.guinot@sequanux.org>
> Cc: vdonnefort@lacie.com
> Cc: stable@vger.kernel.org
> ---
> David, this patch is a fix for a problem that has been here since 3.8
> (when the mvneta driver was introduced), so I've Cc'ed stable@ and if
> possible I'd like to patch to be included for 3.12.
> ---
> drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index b017818..90ab292 100644
> --- a/drivers/net/ethernet/marvell/mvneta.c
> +++ b/drivers/net/ethernet/marvell/mvneta.c
> @@ -138,7 +138,9 @@
> #define MVNETA_GMAC_FORCE_LINK_PASS BIT(1)
> #define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)
> #define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)
> +#define MVNETA_GMAC_AN_SPEED_EN BIT(7)
> #define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)
> +#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)
> #define MVNETA_MIB_COUNTERS_BASE 0x3080
> #define MVNETA_MIB_LATE_COLLISION 0x7c
> #define MVNETA_DA_FILT_SPEC_MCAST 0x3400
> @@ -915,6 +917,13 @@ static void mvneta_defaults_set(struct mvneta_port *pp)
> /* Assign port SDMA configuration */
> mvreg_write(pp, MVNETA_SDMA_CONFIG, val);
>
> + /* Disable PHY polling in hardware, since we're using the
> + * kernel phylib to do this.
> + */
> + val = mvreg_read(pp, MVNETA_UNIT_CONTROL);
> + val &= ~MVNETA_PHY_POLLING_ENABLE;
> + mvreg_write(pp, MVNETA_UNIT_CONTROL, val);
> +
> mvneta_set_ucast_table(pp, -1);
> mvneta_set_special_mcast_table(pp, -1);
> mvneta_set_other_mcast_table(pp, -1);
> @@ -2307,7 +2316,9 @@ static void mvneta_adjust_link(struct net_device *ndev)
> val = mvreg_read(pp, MVNETA_GMAC_AUTONEG_CONFIG);
> val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |
> MVNETA_GMAC_CONFIG_GMII_SPEED |
> - MVNETA_GMAC_CONFIG_FULL_DUPLEX);
> + MVNETA_GMAC_CONFIG_FULL_DUPLEX |
> + MVNETA_GMAC_AN_SPEED_EN |
> + MVNETA_GMAC_AN_DUPLEX_EN);
>
> if (phydev->duplex)
> val |= MVNETA_GMAC_CONFIG_FULL_DUPLEX;
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ 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