* Re: [PATCH 2/7] phy: miphy365x: Pass sysconfig register offsets via syscfg dt property.
From: Lee Jones @ 2014-11-19 10:42 UTC (permalink / raw)
To: Peter Griffin
Cc: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd,
netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-3-git-send-email-peter.griffin@linaro.org>
On Wed, 19 Nov 2014, Peter Griffin wrote:
> Based on Arnds review comments here https://lkml.org/lkml/2014/11/13/161, update
> the miphy365 phy driver to access sysconfig register offsets via syscfg dt property.
>
> This is because the reg property should not be mixing address spaces like it does
> currently for miphy365. This change then also aligns us to how other platforms such
> as keystone and bcm7445 pass there syscon offsets via DT.
>
> This patch breaks DT compatibility, but this platform is considered WIP, and is only
> used by a few developers who are upstreaming support for it. This change has been done
> as a single atomic commit to ensure it is bisectable.
You should wrap your lines sooner (70-75 chars).
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> .../devicetree/bindings/phy/phy-miphy365x.txt | 15 +++++------
> arch/arm/boot/dts/stih416.dtsi | 10 ++++----
> drivers/phy/phy-miphy365x.c | 29 ++++++++--------------
> 3 files changed, 23 insertions(+), 31 deletions(-)
[...]
> diff --git a/drivers/phy/phy-miphy365x.c b/drivers/phy/phy-miphy365x.c
> index 801afaf..7308afe 100644
> --- a/drivers/phy/phy-miphy365x.c
> +++ b/drivers/phy/phy-miphy365x.c
[...]
> phy_set_drvdata(phy, miphy_dev->phys[port]);
> +
> port++;
> + /*sysconfig offsets are not indexed from zero */
You're missing a ' ' afrer '/*'.
Probably better do say what they _are_ indexed from, rather than what
they're not.
> + ret = of_property_read_u32_index(np, "st,syscfg", port,
> + &miphy_phy->ctrlreg);
> + if (ret) {
> + dev_err(&pdev->dev, "No sysconfig offset found\n");
> + return ret;
> + }
> }
>
> provider = devm_of_phy_provider_register(&pdev->dev, miphy365x_xlate);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v3] fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Jiri Bohac @ 2014-11-19 10:38 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: jbohac, Arnaldo Carvalho de Melo, netdev, David Miller
In-Reply-To: <20141119103413.GA19092@midget.suse.cz>
This fixes an old regression introduced by commit
b0d0d915 (ipx: remove the BKL).
When a recvmsg syscall blocks waiting for new data, no data can be sent on the
same socket with sendmsg because ipx_recvmsg() sleeps with the socket locked.
This breaks mars-nwe (NetWare emulator):
- the ncpserv process reads the request using recvmsg
- ncpserv forks and spawns nwconn
- ncpserv calls a (blocking) recvmsg and waits for new requests
- nwconn deadlocks in sendmsg on the same socket
Commit b0d0d915 has simply replaced BKL locking with
lock_sock/release_sock. Unlike now, BKL got unlocked while
sleeping, so a blocking recvmsg did not block a concurrent
sendmsg.
Only keep the socket locked while actually working with the socket data and
release it prior to calling skb_recv_datagram().
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index a0c7536..d0725d9 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1764,6 +1764,7 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
struct ipxhdr *ipx = NULL;
struct sk_buff *skb;
int copied, rc;
+ int locked = 1;
lock_sock(sk);
/* put the autobinding in */
@@ -1790,6 +1791,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
if (sock_flag(sk, SOCK_ZAPPED))
goto out;
+ release_sock(sk);
+ locked = 0;
skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
flags & MSG_DONTWAIT, &rc);
if (!skb) {
@@ -1825,7 +1828,8 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
out_free:
skb_free_datagram(sk, skb);
out:
- release_sock(sk);
+ if (locked)
+ release_sock(sk);
return rc;
}
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
* Re: ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Jiri Bohac @ 2014-11-19 10:34 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Jiri Bohac, Arnaldo Carvalho de Melo, netdev, David Miller
In-Reply-To: <1609909.1jJeqOzgZ8@wuerfel>
On Wed, Nov 19, 2014 at 09:32:54AM +0100, Arnd Bergmann wrote:
> I'm more interested in the code structure, in particular this bit
>
> @@ -1807,8 +1812,10 @@ static int ipx_recvmsg(struct kiocb *iocb, struct socket *sock,
>
> rc = skb_copy_datagram_iovec(skb, sizeof(struct ipxhdr), msg->msg_iov,
> copied);
> - if (rc)
> - goto out_free;
> + if (rc) {
> + skb_free_datagram(sk, skb);
> + goto out;
> + }
>
>
> after your change mixes coding styles: in some failure cases you just goto
> a cleanup part, in other cases you do the cleanup before the goto.
>
> If I'm reading the patch correctly, this change has introduced a leak
Very lame of me - thanks so much for spotitng this!
Sending a restructured v3.
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply
* Re: net: xfrm: Deletion of an unnecessary check before the function call "ipcomp_free_tfms"
From: Dan Carpenter @ 2014-11-19 10:10 UTC (permalink / raw)
To: SF Markus Elfring
Cc: David S. Miller, Herbert Xu, Steffen Klassert, netdev, LKML,
kernel-janitors, Julia Lawall
In-Reply-To: <546C6823.50900@users.sourceforge.net>
I have come to view you as a very clever troll. You will say
infuriating things like "That is an interesting background information
but the function implementation update suggestion is correct" and
pretend to not see the bug for nine emails. Then you'll say something
"Ha ha, I audited all the call trees and it can't actually be NULL so
you were getting angry for no reason." Except you'll say it in a
more obfuscated way than that. This is what you did last time.
regards,
dan carpenter
^ permalink raw reply
* Re: [patch net-next v3 8/9] net: move vlan pop/push functions into common code
From: Jiri Pirko @ 2014-11-19 10:03 UTC (permalink / raw)
To: Pravin Shelar
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <CALnjE+rJEtyL-QDrp0x+N_pCxDuR6soHkaY-OrswWrdZwfE2sQ@mail.gmail.com>
Wed, Nov 19, 2014 at 09:06:19AM CET, pshelar@nicira.com wrote:
>On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> So it can be used from out of openvswitch code.
>> Did couple of cosmetic changes on the way, namely variable naming and
>> adding support for 8021AD proto.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>
>> v2->v3
>> - used previously introduced __vlan_insert_tag helper
>> - used skb_push/pull to get skb->data into needed point
>> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
>> v1->v2:
>> - adjusted to fix recent ovs changes
>> - included change to use make_writable suggested by Eric
>>
>> include/linux/skbuff.h | 2 +
>> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>> net/openvswitch/actions.c | 88 ++++++-------------------------------------
>> 3 files changed, 109 insertions(+), 76 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index e045516..78c299f 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>> int skb_ensure_writable(struct sk_buff *skb, int write_len);
>> +int skb_vlan_pop(struct sk_buff *skb);
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>
>> struct skb_checksum_ops {
>> __wsum (*update)(const void *mem, int len, __wsum wsum);
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index d11bbe0..2f72e62 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
>> }
>> EXPORT_SYMBOL(skb_ensure_writable);
>>
>> +/* remove VLAN header from packet and update csum accordingly. */
>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>> +{
>> + struct vlan_hdr *vhdr;
>> + unsigned int offset = skb->data - skb_mac_header(skb);
>> + int err;
>> +
>> + __skb_push(skb, offset);
>> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>> + if (unlikely(err))
>> + goto pull;
>> +
>> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>> +
>> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>> +
>> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> + __skb_pull(skb, VLAN_HLEN);
>> +
>> + vlan_set_encap_proto(skb, vhdr);
>> + skb->mac_header += VLAN_HLEN;
>> +
>> + if (skb_network_offset(skb) < ETH_HLEN)
>> + skb_set_network_header(skb, ETH_HLEN);
>> +
>> + skb_reset_mac_len(skb);
>
>skb_reset_mac_len() sets length according to ethernet and network
>offsets, but mpls expects mac-length to be offset to mpls header (ref.
>skb_mpls_header()). Therefore rather than reset we need to subtract
>VLAN_HLEN from mac_len. Same goes for vlan-push(), we can add
>VLAN_HLEN.
Hmm, I copied this bits from the original openvswitch code. So you say
that it is broken?
For the push case, __vlan_insert_tag is called which does:
skb->mac_header -= VLAN_HLEN;
then skb_reset_mac_len does:
skb->mac_len = skb->network_header - skb->mac_header
Should be the same as skb->mac_len += VLAN_HLEN, but nicer.
So for push, I think this is okay.
>
>> +pull:
>> + __skb_pull(skb, offset);
>> +
>> + return err;
>> +}
>> +
>> +int skb_vlan_pop(struct sk_buff *skb)
>> +{
>> + u16 vlan_tci;
>> + __be16 vlan_proto;
>> + int err;
>> +
>> + if (likely(vlan_tx_tag_present(skb))) {
>> + skb->vlan_tci = 0;
>> + } else {
>> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>> + skb->protocol != htons(ETH_P_8021AD)) ||
>> + skb->len < VLAN_ETH_HLEN))
>> + return 0;
>> +
>> + err = __skb_vlan_pop(skb, &vlan_tci);
>> + if (err)
>> + return err;
>> + }
>> + /* move next vlan tag to hw accel tag */
>> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>> + skb->protocol != htons(ETH_P_8021AD)) ||
>> + skb->len < VLAN_ETH_HLEN))
>> + return 0;
>> +
>> + vlan_proto = skb->protocol;
>> + err = __skb_vlan_pop(skb, &vlan_tci);
>> + if (unlikely(err))
>> + return err;
>> +
>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_pop);
>> +
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>> +{
>> + if (vlan_tx_tag_present(skb)) {
>> + unsigned int offset = skb->data - skb_mac_header(skb);
>> + int err;
>> +
>> + /* __vlan_insert_tag expect skb->data pointing to mac header.
>> + * So change skb->data before calling it and change back to
>> + * original position later
>> + */
>> + __skb_push(skb, offset);
>> + err = __vlan_insert_tag(skb, skb->vlan_proto,
>> + vlan_tx_tag_get(skb));
>> + if (err)
>> + return err;
>> + skb->protocol = skb->vlan_proto;
>> + skb_reset_mac_len(skb);
>> + __skb_pull(skb, offset);
>> +
>> + if (skb->ip_summed == CHECKSUM_COMPLETE)
>> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
>> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> + }
>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_push);
>> +
>> /**
>> * alloc_skb_with_frags - allocate skb with page frags
>> *
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 7ffa377..ae7e1b2 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
>> return 0;
>> }
>>
>> -/* remove VLAN header from packet and update csum accordingly. */
>> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>> -{
>> - struct vlan_hdr *vhdr;
>> - int err;
>> -
>> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>> - if (unlikely(err))
>> - return err;
>> -
>> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>> -
>> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> - *current_tci = vhdr->h_vlan_TCI;
>> -
>> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> - __skb_pull(skb, VLAN_HLEN);
>> -
>> - vlan_set_encap_proto(skb, vhdr);
>> - skb->mac_header += VLAN_HLEN;
>> -
>> - if (skb_network_offset(skb) < ETH_HLEN)
>> - skb_set_network_header(skb, ETH_HLEN);
>> -
>> - /* Update mac_len for subsequent MPLS actions */
>> - skb_reset_mac_len(skb);
>> - return 0;
>> -}
>> -
>> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>> {
>> - __be16 tci;
>> int err;
>>
>> - if (likely(vlan_tx_tag_present(skb))) {
>> - skb->vlan_tci = 0;
>> - } else {
>> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN))
>> - return 0;
>> -
>> - err = __pop_vlan_tci(skb, &tci);
>> - if (err)
>> - return err;
>> - }
>> - /* move next vlan tag to hw accel tag */
>> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN)) {
>> + err = skb_vlan_pop(skb);
>> + if (vlan_tx_tag_present(skb))
>> + invalidate_flow_key(key);
>> + else
>> key->eth.tci = 0;
>> - return 0;
>> - }
>> -
>> - invalidate_flow_key(key);
>> - err = __pop_vlan_tci(skb, &tci);
>> - if (unlikely(err))
>> - return err;
>> -
>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
>> - return 0;
>> + return err;
>> }
>>
>> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
>> const struct ovs_action_push_vlan *vlan)
>> {
>> - if (unlikely(vlan_tx_tag_present(skb))) {
>> - u16 current_tag;
>> -
>> - /* push down current VLAN tag */
>> - current_tag = vlan_tx_tag_get(skb);
>> -
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - current_tag);
>> - if (!skb)
>> - return -ENOMEM;
>> - /* Update mac_len for subsequent MPLS actions */
>> - skb->mac_len += VLAN_HLEN;
>> -
>> - if (skb->ip_summed == CHECKSUM_COMPLETE)
>> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
>> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> -
>> + if (vlan_tx_tag_present(skb))
>> invalidate_flow_key(key);
>> - } else {
>> + else
>> key->eth.tci = vlan->vlan_tci;
>> - }
>> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>> - return 0;
>> + return skb_vlan_push(skb, vlan->vlan_tpid,
>> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>> }
>>
>> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
>> @@ -858,8 +792,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>
>> case OVS_ACTION_ATTR_PUSH_VLAN:
>> err = push_vlan(skb, key, nla_data(a));
>> - if (unlikely(err)) /* skb already freed. */
>> + if (unlikely(err)) {
>> + dev_kfree_skb_any(skb);
>> return err;
>> + }
>There is no need to check for error. Just break is sufficient. Error
>checking is done after switch case.
Fixed.
>> break;
>>
>> case OVS_ACTION_ATTR_POP_VLAN:
>> --
>> 1.9.3
>>
^ permalink raw reply
* [PATCH net-next] net: use skb_get_hash_raw instead of skb_get_hash in set_rps_cpu
From: Govindarajulu Varadarajan @ 2014-11-19 10:00 UTC (permalink / raw)
To: davem, netdev; +Cc: Govindarajulu Varadarajan
In set_rps_cpu, we call skb_get_hash to get the hash of skb. The caller
get_rps_cpu has already determined the hash of skb by calling skb_get_hash.
Since get_rps_cpu is the only caller of set_rps_cpu, we can use skb_get_hash_raw
Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 1ab168e..b4ad4d1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3055,7 +3055,7 @@ set_rps_cpu(struct net_device *dev, struct sk_buff *skb,
flow_table = rcu_dereference(rxqueue->rps_flow_table);
if (!flow_table)
goto out;
- flow_id = skb_get_hash(skb) & flow_table->mask;
+ flow_id = skb_get_hash_raw(skb) & flow_table->mask;
rc = dev->netdev_ops->ndo_rx_flow_steer(dev, skb,
rxq_index, flow_id);
if (rc < 0)
--
2.1.0
^ permalink raw reply related
* Re: net: xfrm: Deletion of an unnecessary check before the function call "ipcomp_free_tfms"
From: Julia Lawall @ 2014-11-19 9:58 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Dan Carpenter, David S. Miller, Herbert Xu, Steffen Klassert,
netdev, LKML, kernel-janitors
In-Reply-To: <546C6823.50900@users.sourceforge.net>
On Wed, 19 Nov 2014, SF Markus Elfring wrote:
> >> The ipcomp_free_tfms() function tests whether its argument is NULL and then
> >> returns immediately. Thus the test around the call is not needed.
> >
> > It doesn't though...
>
> You are right that this function implementation does a bit more before
> returning because of a detected null pointer.
> https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/net/xfrm/xfrm_ipcomp.c?id=394efd19d5fcae936261bd48e5b33b21897aacf8#n247
>
> Can you agree that input parameter validation is also performed there?
> Do you want that I resend my patch with a corrected commit message?
This is completely crazy. The function performs a side effect on a data
structure. If the call site doesn't want that done in a certain case,
then it should not be done.
julia
^ permalink raw reply
* Re: [net-next 12/15] i40evf: make checkpatch happy
From: Jeff Kirsher @ 2014-11-19 9:55 UTC (permalink / raw)
To: Joe Perches; +Cc: davem, Mitch Williams, netdev, nhorman, sassmann, jogreene
In-Reply-To: <1416373185.6651.12.camel@perches.com>
[-- Attachment #1: Type: text/plain, Size: 2056 bytes --]
On Tue, 2014-11-18 at 20:59 -0800, Joe Perches wrote:
> On Tue, 2014-11-18 at 20:10 -0800, Jeff Kirsher wrote:
> > This patch is the result of running checkpatch on the i40evf driver with
> > the --strict option. The vast majority of changes are adding/removing
> > blank lines, aligning function parameters, and correcting over-long
> > lines.
>
> Hey Mitch, Jeff:
>
> > diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
> []
> > @@ -1265,8 +1269,8 @@ int i40evf_init_interrupt_scheme(struct i40evf_adapter *adapter)
> > }
> >
> > dev_info(&adapter->pdev->dev, "Multiqueue %s: Queue pair count = %u",
> > - (adapter->num_active_queues > 1) ? "Enabled" :
> > - "Disabled", adapter->num_active_queues);
> > + (adapter->num_active_queues > 1) ? "Enabled" : "Disabled",
> > + adapter->num_active_queues);
>
> You could add a newline to that format one day.
>
> > diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c b/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c
> []
> > @@ -711,7 +712,6 @@ void i40evf_virtchnl_completion(struct i40evf_adapter *adapter,
> > "%s: Unknown event %d from pf\n",
> > __func__, vpe->event);
> > break;
> > -
> > }
> > return;
> > }
> > @@ -776,7 +776,7 @@ void i40evf_virtchnl_completion(struct i40evf_adapter *adapter,
> > break;
> > default:
> > dev_warn(&adapter->pdev->dev, "%s: Received unexpected message %d from PF\n",
> > - __func__, v_opcode);
> > + __func__, v_opcode);
> > break;
> > } /* switch v_opcode */
> > adapter->current_op = I40E_VIRTCHNL_OP_UNKNOWN;
>
> And be consistent with PF vs pf usage too.
Thanks for noticing, I had not caught that there was inconsistent use.
I try to catch acronyms that are not capitalized, and after looking
through the code, I see that there is inconsistent use of VF vs vf as
well. I am putting together a patch now that will make the use of both
acronyms consistent throughout our code.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: net: xfrm: Deletion of an unnecessary check before the function call "ipcomp_free_tfms"
From: SF Markus Elfring @ 2014-11-19 9:51 UTC (permalink / raw)
To: Dan Carpenter
Cc: David S. Miller, Herbert Xu, Steffen Klassert, netdev, LKML,
kernel-janitors, Julia Lawall
In-Reply-To: <20141119084542.GK4905@mwanda>
>> The ipcomp_free_tfms() function tests whether its argument is NULL and then
>> returns immediately. Thus the test around the call is not needed.
>
> It doesn't though...
You are right that this function implementation does a bit more before
returning because of a detected null pointer.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/net/xfrm/xfrm_ipcomp.c?id=394efd19d5fcae936261bd48e5b33b21897aacf8#n247
Can you agree that input parameter validation is also performed there?
Do you want that I resend my patch with a corrected commit message?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.
From: Jeff Kirsher @ 2014-11-19 8:43 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev, David Miller, bruce.w.allan, Jacob Keller, John Ronciak,
Matthew Vick
In-Reply-To: <20141119063103.GA4109@localhost.localdomain>
[-- Attachment #1: Type: text/plain, Size: 396 bytes --]
On Wed, 2014-11-19 at 07:31 +0100, Richard Cochran wrote:
> On Mon, Nov 17, 2014 at 03:15:18PM -0800, Jeff Kirsher wrote:
> > Thanks Richard, I have added your series of igb patches to my queue.
>
> Jeff, please hold off on these. I found a bug in the PPS code. I will
> re-submit with a fix.
Ok, I will drop the series that I currently have in my queue and will
await your v2 series.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* guten Morgen
From: Loans Engine® @ 2014-11-19 9:43 UTC (permalink / raw)
Guten Morgen
Erhalten Sie ein Darlehen bei 3 %, in kurz-und langfristig. Darlehen-Engine ist eine globale Kreditvergabe-Gruppe, die geschaffen wurde, um auf die Bedürfnisse der wirtschaftlich angeschlagenen Clients. Wir arbeiten in allen Kategorien von Krediten. Geben Sie die folgende Informationen, wenn Sie interessiert sind.
Vollständiger Name:
Geschlecht:
Land:
Den erforderlichen Betrag:
Dauer:
Mission:
Wir brauchen diese vollständige Informationen für Kreditbearbeitung.
Mailen Sie uns: loan.engine@outlook.com
Alles Gute
Ana weiß
--
Esta mensagem foi verificada pelo sistema de antivirus e
acredita-se estar livre de perigo.
^ permalink raw reply
* Re: [PATCH net] virtio-net: validate features during probe
From: Michael S. Tsirkin @ 2014-11-19 9:39 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <546C63E6.7040600@redhat.com>
On Wed, Nov 19, 2014 at 05:33:26PM +0800, Jason Wang wrote:
> On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
> > On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
> >> This patch validates feature dependencies during probe and fail the probing
> >> if a dependency is missed. This fixes the issues of hitting BUG()
> >> when qemu fails to advertise features correctly. One example is booting
> >> guest with ctrl_vq=off through qemu.
> >>
> >> Cc: Rusty Russell <rusty@rustcorp.com.au>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> >> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> ---
> >> drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 93 insertions(+)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index ec2a8b4..4a0ad46 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> >> };
> >> #endif
> >>
> >> +static int virtnet_validate_features(struct virtio_device *dev,
> >> + unsigned int *table,
> >> + int table_size,
> >> + unsigned int feature)
> >> +{
> >> + int i;
> >> +
> >> + if (!virtio_has_feature(dev, feature)) {
> >> + for (i = 0; i < table_size; i++) {
> >> + unsigned int f = table[i];
> >> +
> >> + if (virtio_has_feature(dev, f)) {
> >> + dev_err(&dev->dev,
> >> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
> >
> > This line's way too long.
>
> Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
> split)
> >
> >> + f, feature);
> >> + return -EINVAL;
> >> + }
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int virtnet_check_features(struct virtio_device *dev)
> >> +{
> >> + unsigned int features_for_ctrl_vq[] = {
> >> + VIRTIO_NET_F_CTRL_RX,
> >> + VIRTIO_NET_F_CTRL_VLAN,
> >> + VIRTIO_NET_F_GUEST_ANNOUNCE,
> >> + VIRTIO_NET_F_MQ,
> >> + VIRTIO_NET_F_CTRL_MAC_ADDR
> >> + };
> >> + unsigned int features_for_guest_csum[] = {
> >> + VIRTIO_NET_F_GUEST_TSO4,
> >> + VIRTIO_NET_F_GUEST_TSO6,
> >> + VIRTIO_NET_F_GUEST_ECN,
> >> + VIRTIO_NET_F_GUEST_UFO,
> >> + };
> >> + unsigned int features_for_host_csum[] = {
> >> + VIRTIO_NET_F_HOST_TSO4,
> >> + VIRTIO_NET_F_HOST_TSO6,
> >> + VIRTIO_NET_F_HOST_ECN,
> >> + VIRTIO_NET_F_HOST_UFO,
> >> + };
> >> + int err;
> >> +
> >> + err = virtnet_validate_features(dev, features_for_ctrl_vq,
> >> + ARRAY_SIZE(features_for_ctrl_vq),
> >> + VIRTIO_NET_F_CTRL_VQ);
> >> + if (err)
> >> + return err;
> >> +
> >> + err = virtnet_validate_features(dev, features_for_guest_csum,
> >> + ARRAY_SIZE(features_for_guest_csum),
> >> + VIRTIO_NET_F_GUEST_CSUM);
> >> + if (err)
> >> + return err;
> >> +
> >> + err = virtnet_validate_features(dev, features_for_host_csum,
> >> + ARRAY_SIZE(features_for_host_csum),
> >> + VIRTIO_NET_F_CSUM);
> >> + if (err)
> >> + return err;
> >> +
> >> + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
> >> + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
> >> + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
> >> + dev_err(&dev->dev,
> >> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> >> + VIRTIO_NET_F_GUEST_ECN,
> >> + VIRTIO_NET_F_GUEST_TSO4,
> >> + VIRTIO_NET_F_GUEST_TSO6);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
> >> + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
> >> + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
> >> + dev_err(&dev->dev,
> >> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> >> + VIRTIO_NET_F_HOST_ECN,
> >> + VIRTIO_NET_F_HOST_TSO4,
> >> + VIRTIO_NET_F_HOST_TSO6);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int virtnet_probe(struct virtio_device *vdev)
> >> {
> >> int i, err;
> >> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> >> struct virtnet_info *vi;
> >> u16 max_queue_pairs;
> >>
> >> + err = virtnet_check_features(vdev);
> >> + if (err)
> >> + return -EINVAL;
> >> +
> >> /* Find if host supports multiqueue virtio_net device */
> >> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> >> struct virtio_net_config,
> > The API seems too complex, and you still had to open-code ECN logic.
> > Just open-code most of it.
>
> Yes, the ECN could be done through the same way as ctrl_vq did.
>
> How about move those checking into virtio core by just letting device
> export its dependency table?
So far we only have this for net, let's not add
one-off APIs.
> > You can use a helper macro to output a
> > friendly message without code duplication.
> > For example like the below (completely untested)?
> >
> >
> > I would also like to split things: dependencies on
> > VIRTIO_NET_F_CTRL_VQ might go into this kernel,
> > since they help fix BUG.
> >
> > Others should wait since they don't fix any crashes, and there's a small
> > chance of a regression for some hypervisor in the field.
>
> Probably ok but not sure, since the rest features are all related to
> csum and offloading, we are in fact depends on network core to fix them.
Well it does fix them so ... there's no bug, is there?
> >
> > -->
> >
> > virtio-net: friendlier handling of misconfigured hypervisors
> >
> > We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
> > is not set but one of features depending on it is.
> > That's not a friendly way to report errors to
> > hypervisors.
> > Let's check, and fail probe instead.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 26e1330..7a7d1a3 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> > };
> > #endif
> >
> > +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
> > + const char *fname)
> > +{
> > + if (!virtio_has_feature(vdev, fbit))
> > + return false;
> > +
> > + dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
> > + fbit, fname);
> > +
> > + return true;
> > +}
> > +
> > +#define VIRTNET_FAIL_ON(vdev, fbit) \
> > + __virtnet_fail_on_feature(vdev, fbit, #fbit)
> > +
> > static int virtnet_probe(struct virtio_device *vdev)
> > {
> > int i, err;
> > @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> > struct virtnet_info *vi;
> > u16 max_queue_pairs;
> >
> > + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
> > + (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
> > + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
> > + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
> > + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
> > + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
> > + return -EINVAL;
> > +
> > /* Find if host supports multiqueue virtio_net device */
> > err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> > struct virtio_net_config,
> >
>
> Patch looks good, but consider we may check more dependencies in the
> future, may need a helper instead. Or just use this and switch to
> dependency table in 3.19.
Pls note this is just pseudo-code - I didn't even compile it.
If you want something like this merged, go ahead and
post it, probably addressing Cornelia's request to
print the dependency too. Maybe:
> > (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX, "ctrl_vq") ||
> > VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN, "ctrl_vq") ||
> > VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE, "ctrl_vq") ||
> > VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ, "ctrl_vq") ||
> > VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR, "ctrl_vq")))
--
MST
^ permalink raw reply
* Re: [PATCH net] virtio-net: validate features during probe
From: Jason Wang @ 2014-11-19 9:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20141119085939.GB24827@redhat.com>
On 11/19/2014 04:59 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
>> This patch validates feature dependencies during probe and fail the probing
>> if a dependency is missed. This fixes the issues of hitting BUG()
>> when qemu fails to advertise features correctly. One example is booting
>> guest with ctrl_vq=off through qemu.
>>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 93 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index ec2a8b4..4a0ad46 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>> };
>> #endif
>>
>> +static int virtnet_validate_features(struct virtio_device *dev,
>> + unsigned int *table,
>> + int table_size,
>> + unsigned int feature)
>> +{
>> + int i;
>> +
>> + if (!virtio_has_feature(dev, feature)) {
>> + for (i = 0; i < table_size; i++) {
>> + unsigned int f = table[i];
>> +
>> + if (virtio_has_feature(dev, f)) {
>> + dev_err(&dev->dev,
>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
>
> This line's way too long.
Yes. (Anyway it pass checkpatch.pl since it forbids quoted string to be
split)
>
>> + f, feature);
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int virtnet_check_features(struct virtio_device *dev)
>> +{
>> + unsigned int features_for_ctrl_vq[] = {
>> + VIRTIO_NET_F_CTRL_RX,
>> + VIRTIO_NET_F_CTRL_VLAN,
>> + VIRTIO_NET_F_GUEST_ANNOUNCE,
>> + VIRTIO_NET_F_MQ,
>> + VIRTIO_NET_F_CTRL_MAC_ADDR
>> + };
>> + unsigned int features_for_guest_csum[] = {
>> + VIRTIO_NET_F_GUEST_TSO4,
>> + VIRTIO_NET_F_GUEST_TSO6,
>> + VIRTIO_NET_F_GUEST_ECN,
>> + VIRTIO_NET_F_GUEST_UFO,
>> + };
>> + unsigned int features_for_host_csum[] = {
>> + VIRTIO_NET_F_HOST_TSO4,
>> + VIRTIO_NET_F_HOST_TSO6,
>> + VIRTIO_NET_F_HOST_ECN,
>> + VIRTIO_NET_F_HOST_UFO,
>> + };
>> + int err;
>> +
>> + err = virtnet_validate_features(dev, features_for_ctrl_vq,
>> + ARRAY_SIZE(features_for_ctrl_vq),
>> + VIRTIO_NET_F_CTRL_VQ);
>> + if (err)
>> + return err;
>> +
>> + err = virtnet_validate_features(dev, features_for_guest_csum,
>> + ARRAY_SIZE(features_for_guest_csum),
>> + VIRTIO_NET_F_GUEST_CSUM);
>> + if (err)
>> + return err;
>> +
>> + err = virtnet_validate_features(dev, features_for_host_csum,
>> + ARRAY_SIZE(features_for_host_csum),
>> + VIRTIO_NET_F_CSUM);
>> + if (err)
>> + return err;
>> +
>> + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
>> + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
>> + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
>> + dev_err(&dev->dev,
>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
>> + VIRTIO_NET_F_GUEST_ECN,
>> + VIRTIO_NET_F_GUEST_TSO4,
>> + VIRTIO_NET_F_GUEST_TSO6);
>> + return -EINVAL;
>> + }
>> +
>> + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
>> + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
>> + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
>> + dev_err(&dev->dev,
>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
>> + VIRTIO_NET_F_HOST_ECN,
>> + VIRTIO_NET_F_HOST_TSO4,
>> + VIRTIO_NET_F_HOST_TSO6);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int virtnet_probe(struct virtio_device *vdev)
>> {
>> int i, err;
>> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>> struct virtnet_info *vi;
>> u16 max_queue_pairs;
>>
>> + err = virtnet_check_features(vdev);
>> + if (err)
>> + return -EINVAL;
>> +
>> /* Find if host supports multiqueue virtio_net device */
>> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
>> struct virtio_net_config,
> The API seems too complex, and you still had to open-code ECN logic.
> Just open-code most of it.
Yes, the ECN could be done through the same way as ctrl_vq did.
How about move those checking into virtio core by just letting device
export its dependency table?
> You can use a helper macro to output a
> friendly message without code duplication.
> For example like the below (completely untested)?
>
>
> I would also like to split things: dependencies on
> VIRTIO_NET_F_CTRL_VQ might go into this kernel,
> since they help fix BUG.
>
> Others should wait since they don't fix any crashes, and there's a small
> chance of a regression for some hypervisor in the field.
Probably ok but not sure, since the rest features are all related to
csum and offloading, we are in fact depends on network core to fix them.
>
> -->
>
> virtio-net: friendlier handling of misconfigured hypervisors
>
> We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
> is not set but one of features depending on it is.
> That's not a friendly way to report errors to
> hypervisors.
> Let's check, and fail probe instead.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 26e1330..7a7d1a3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> };
> #endif
>
> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
> + const char *fname)
> +{
> + if (!virtio_has_feature(vdev, fbit))
> + return false;
> +
> + dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
> + fbit, fname);
> +
> + return true;
> +}
> +
> +#define VIRTNET_FAIL_ON(vdev, fbit) \
> + __virtnet_fail_on_feature(vdev, fbit, #fbit)
> +
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
> @@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> struct virtnet_info *vi;
> u16 max_queue_pairs;
>
> + if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
> + (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
> + VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
> + return -EINVAL;
> +
> /* Find if host supports multiqueue virtio_net device */
> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> struct virtio_net_config,
>
Patch looks good, but consider we may check more dependencies in the
future, may need a helper instead. Or just use this and switch to
dependency table in 3.19.
^ permalink raw reply
* Re: [PATCH V2 net] virtio-net: validate features during probe
From: Jason Wang @ 2014-11-19 9:21 UTC (permalink / raw)
To: Cornelia Huck; +Cc: mst, netdev, linux-kernel, virtualization
In-Reply-To: <20141119095445.32d94d5d.cornelia.huck@de.ibm.com>
On 11/19/2014 04:54 PM, Cornelia Huck wrote:
> On Wed, 19 Nov 2014 15:21:29 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> This patch validates feature dependencies during probe and fail the probing
>> if a dependency is missed. This fixes the issues of hitting BUG()
>> when qemu fails to advertise features correctly. One example is booting
>> guest with ctrl_vq=off through qemu.
>>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> Changes from V1:
>> - Drop VIRTIO_NET_F_*_UFO from the checklist, since it was disabled
>> ---
>> drivers/net/virtio_net.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 91 insertions(+)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index ec2a8b4..b16a761 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1673,6 +1673,93 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
>> };
>> #endif
>>
>> +static int virtnet_validate_features(struct virtio_device *dev,
>> + unsigned int *table,
>> + int table_size,
>> + unsigned int feature)
>> +{
>> + int i;
>> +
>> + if (!virtio_has_feature(dev, feature)) {
> Do an early return, get rid of one indentation level?
This sounds good.
>> + for (i = 0; i < table_size; i++) {
>> + unsigned int f = table[i];
>> +
>> + if (virtio_has_feature(dev, f)) {
>> + dev_err(&dev->dev,
>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
> s/hyperviser/hypervisor/ (also below)
Yes, thanks for the catching.
>
>> + f, feature);
>> + return -EINVAL;
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int virtnet_check_features(struct virtio_device *dev)
>> +{
>> + unsigned int features_for_ctrl_vq[] = {
>> + VIRTIO_NET_F_CTRL_RX,
>> + VIRTIO_NET_F_CTRL_VLAN,
>> + VIRTIO_NET_F_GUEST_ANNOUNCE,
>> + VIRTIO_NET_F_MQ,
>> + VIRTIO_NET_F_CTRL_MAC_ADDR
>> + };
>> + unsigned int features_for_guest_csum[] = {
>> + VIRTIO_NET_F_GUEST_TSO4,
>> + VIRTIO_NET_F_GUEST_TSO6,
>> + VIRTIO_NET_F_GUEST_ECN,
>> + };
>> + unsigned int features_for_host_csum[] = {
>> + VIRTIO_NET_F_HOST_TSO4,
>> + VIRTIO_NET_F_HOST_TSO6,
>> + VIRTIO_NET_F_HOST_ECN,
>> + };
> I'm wondering whether it would be easier to read if you listed all
> prereqs per feature instead of all features that depend on a feature?
> It would still be hard to express the v4/v6 or conditions below in
> tables, though.
For v4 and v6, we could use something like
unsigned int feature_for_host_tso6[] = {
VIRTIO_NET_F_HOST_ECN,
};
unsigned int feature_for_host_tso4[] = {
VIRTIO_NET_F_HOST_ECN,
}
To avoid the following open-coding for ECN. And probably we need another
device specific dependency table and let virtio core do this instead.
>
> Or call the arrays features_depending_on_foo?
Yes.
>
>> + int err;
>> +
>> + err = virtnet_validate_features(dev, features_for_ctrl_vq,
>> + ARRAY_SIZE(features_for_ctrl_vq),
>> + VIRTIO_NET_F_CTRL_VQ);
>> + if (err)
>> + return err;
> If you already print a message that a user may use to fix their
> hypervisor (or bug someone about it), would it make sense to check all
> dependencies and print a full list of everything that is broken in the
> advertised feature bits? I usually hate it if I fix one thing only to
> hit the next bug when the program could have already told me about
> everything I need to fix :)
>
Right this sounds good.
>> +
>> + err = virtnet_validate_features(dev, features_for_guest_csum,
>> + ARRAY_SIZE(features_for_guest_csum),
>> + VIRTIO_NET_F_GUEST_CSUM);
>> + if (err)
>> + return err;
>> +
>> + err = virtnet_validate_features(dev, features_for_host_csum,
>> + ARRAY_SIZE(features_for_host_csum),
>> + VIRTIO_NET_F_CSUM);
>> + if (err)
>> + return err;
>> +
>> + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
>> + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
>> + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
>> + dev_err(&dev->dev,
>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
>> + VIRTIO_NET_F_GUEST_ECN,
>> + VIRTIO_NET_F_GUEST_TSO4,
>> + VIRTIO_NET_F_GUEST_TSO6);
>> + return -EINVAL;
>> + }
>> +
>> + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
>> + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
>> + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
>> + dev_err(&dev->dev,
>> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> "Hypervisor bug: advertised feature <foo> but not <bar> or <baz>"
>
> ?
More compact, looks good. Thanks
^ permalink raw reply
* Re: [PATCH v2 net 1/2] drivers/net: Disable UFO through virtio
From: Michael S. Tsirkin @ 2014-11-19 9:14 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, Hannes Frederic Sowa, virtualization
In-Reply-To: <1414693632.16849.62.camel@decadent.org.uk>
On Thu, Oct 30, 2014 at 06:27:12PM +0000, Ben Hutchings wrote:
> IPv6 does not allow fragmentation by routers, so there is no
> fragmentation ID in the fixed header. UFO for IPv6 requires the ID to
> be passed separately, but there is no provision for this in the virtio
> net protocol.
>
> Until recently our software implementation of UFO/IPv6 generated a new
> ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet
> passed through a tap, which is even worse.
>
> Unfortunately there is no distinction between UFO/IPv4 and v6
> features, so disable UFO on taps and virtio_net completely until we
> have a proper solution.
>
> We cannot depend on VM managers respecting the tap feature flags, so
> keep accepting UFO packets but log a warning the first time we do
> this.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Fixes: 916e4cf46d02 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data")
There's something I don't understand here. I see:
NETIF_F_UFO_BIT, /* ... UDPv4 fragmentation */
this comment is wrong then?
The patches drastically regress performance for UDPv4 for VMs only, but
isn't it likely many other devices based their code on this comment?
How about we disable UFO for IPv6 globally, and put the
flag back in?
We can then gradually add NETIF_F_UFO6_BIT for devices that
actually support UFO for IPv6.
Thoughts?
> ---
> drivers/net/macvtap.c | 13 +++++--------
> drivers/net/tun.c | 19 +++++++++++--------
> drivers/net/virtio_net.c | 24 ++++++++++++++----------
> 3 files changed, 30 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index 65e2892..2aeaa61 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
> static const struct proto_ops macvtap_socket_ops;
>
> #define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
> - NETIF_F_TSO6 | NETIF_F_UFO)
> + NETIF_F_TSO6)
> #define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
> #define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
>
> @@ -569,6 +569,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
> gso_type = SKB_GSO_TCPV6;
> break;
> case VIRTIO_NET_HDR_GSO_UDP:
> + pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
> + current->comm);
> gso_type = SKB_GSO_UDP;
> break;
> default:
> @@ -614,8 +616,6 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
> vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> else if (sinfo->gso_type & SKB_GSO_TCPV6)
> vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> - else if (sinfo->gso_type & SKB_GSO_UDP)
> - vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
> else
> BUG();
> if (sinfo->gso_type & SKB_GSO_TCP_ECN)
> @@ -950,9 +950,6 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> if (arg & TUN_F_TSO6)
> feature_mask |= NETIF_F_TSO6;
> }
> -
> - if (arg & TUN_F_UFO)
> - feature_mask |= NETIF_F_UFO;
> }
>
> /* tun/tap driver inverts the usage for TSO offloads, where
> @@ -963,7 +960,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
> * When user space turns off TSO, we turn off GSO/LRO so that
> * user-space will not receive TSO frames.
> */
> - if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
> + if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
> features |= RX_OFFLOADS;
> else
> features &= ~RX_OFFLOADS;
> @@ -1064,7 +1061,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
> case TUNSETOFFLOAD:
> /* let the user check for future flags */
> if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
> - TUN_F_TSO_ECN | TUN_F_UFO))
> + TUN_F_TSO_ECN))
> return -EINVAL;
>
> rtnl_lock();
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 186ce54..280d3d2 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -174,7 +174,7 @@ struct tun_struct {
> struct net_device *dev;
> netdev_features_t set_features;
> #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
> - NETIF_F_TSO6|NETIF_F_UFO)
> + NETIF_F_TSO6)
>
> int vnet_hdr_sz;
> int sndbuf;
> @@ -1149,8 +1149,18 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> break;
> case VIRTIO_NET_HDR_GSO_UDP:
> + {
> + static bool warned;
> +
> + if (!warned) {
> + warned = true;
> + netdev_warn(tun->dev,
> + "%s: using disabled UFO feature; please fix this program\n",
> + current->comm);
> + }
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> break;
> + }
> default:
> tun->dev->stats.rx_frame_errors++;
> kfree_skb(skb);
> @@ -1251,8 +1261,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> else if (sinfo->gso_type & SKB_GSO_TCPV6)
> gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> - else if (sinfo->gso_type & SKB_GSO_UDP)
> - gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
> else {
> pr_err("unexpected GSO type: "
> "0x%x, gso_size %d, hdr_len %d\n",
> @@ -1762,11 +1770,6 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
> features |= NETIF_F_TSO6;
> arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
> }
> -
> - if (arg & TUN_F_UFO) {
> - features |= NETIF_F_UFO;
> - arg &= ~TUN_F_UFO;
> - }
> }
>
> /* This gives the user a way to test for new features in future by
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d75256bd..ec2a8b4 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -491,8 +491,17 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> break;
> case VIRTIO_NET_HDR_GSO_UDP:
> + {
> + static bool warned;
> +
> + if (!warned) {
> + warned = true;
> + netdev_warn(dev,
> + "host using disabled UFO feature; please fix it\n");
> + }
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> break;
> + }
> case VIRTIO_NET_HDR_GSO_TCPV6:
> skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> break;
> @@ -881,8 +890,6 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
> hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> - else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
> - hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
> else
> BUG();
> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
> @@ -1705,7 +1712,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
> - dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
> + dev->hw_features |= NETIF_F_TSO
> | NETIF_F_TSO_ECN | NETIF_F_TSO6;
> }
> /* Individual feature bits: what can host handle? */
> @@ -1715,11 +1722,9 @@ static int virtnet_probe(struct virtio_device *vdev)
> dev->hw_features |= NETIF_F_TSO6;
> if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
> dev->hw_features |= NETIF_F_TSO_ECN;
> - if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
> - dev->hw_features |= NETIF_F_UFO;
>
> if (gso)
> - dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO);
> + dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
> /* (!csum && gso) case will be fixed by register_netdev() */
> }
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> @@ -1757,8 +1762,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* If we can receive ANY GSO packets, we must allocate large ones. */
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> - virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
> - virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
> + virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
> vi->big_packets = true;
>
> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> @@ -1952,9 +1956,9 @@ static struct virtio_device_id id_table[] = {
> static unsigned int features[] = {
> VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM,
> VIRTIO_NET_F_GSO, VIRTIO_NET_F_MAC,
> - VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
> + VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6,
> VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> - VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
> + VIRTIO_NET_F_GUEST_ECN,
> VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
>
>
> --
> Ben Hutchings
> The program is absolutely right; therefore, the computer must be wrong.
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net] virtio-net: validate features during probe
From: Cornelia Huck @ 2014-11-19 9:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20141119085939.GB24827@redhat.com>
On Wed, 19 Nov 2014 10:59:39 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> +bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
> + const char *fname)
> +{
> + if (!virtio_has_feature(vdev, fbit))
> + return false;
> +
> + dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
> + fbit, fname);
I'd like the message to point out that this is a hypervisor problem: I
can imagine that a user would be stumped about what to do about this.
And printing what requirements are missing would probably be helpful to
someone trying to fix the hypervisor.
> +
> + return true;
> +}
> +
>
^ permalink raw reply
* [PATCH net-next] sky2: use new netdev_rss_key_fill() helper
From: Ian Morris @ 2014-11-19 9:06 UTC (permalink / raw)
To: netdev; +Cc: Ian Morris, Mirko Lindner, Stephen Hemminger, Eric Dumazet
Switch to a random RSS key rather than a fixed one.
Using netdev_rss_key_fill helper also ensures that all ports share
a common key.
See also commit 960fb622f85180f36d3aff82af53e2be3db2f888.
Signed-off-by: Ian Morris <ipm@chirality.org.uk>
Cc: Mirko Lindner <mlindner@marvell.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Eric Dumazet <edumazet@google.com>
---
drivers/net/ethernet/marvell/sky2.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index bd33662..53a1cc5 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -1290,14 +1290,6 @@ static void rx_set_checksum(struct sky2_port *sky2)
? BMU_ENA_RX_CHKSUM : BMU_DIS_RX_CHKSUM);
}
-/*
- * Fixed initial key as seed to RSS.
- */
-static const uint32_t rss_init_key[10] = {
- 0x7c3351da, 0x51c5cf4e, 0x44adbdd1, 0xe8d38d18, 0x48897c43,
- 0xb1d60e7e, 0x6a3dd760, 0x01a2e453, 0x16f46f13, 0x1a0e7b30
-};
-
/* Enable/disable receive hash calculation (RSS) */
static void rx_set_rss(struct net_device *dev, netdev_features_t features)
{
@@ -1313,9 +1305,12 @@ static void rx_set_rss(struct net_device *dev, netdev_features_t features)
/* Program RSS initial values */
if (features & NETIF_F_RXHASH) {
+ u32 rss_key[10];
+
+ netdev_rss_key_fill(rss_key, sizeof(rss_key));
for (i = 0; i < nkeys; i++)
sky2_write32(hw, SK_REG(sky2->port, RSS_KEY + i * 4),
- rss_init_key[i]);
+ rss_key[i]);
/* Need to turn on (undocumented) flag to make hashing work */
sky2_write32(hw, SK_REG(sky2->port, RX_GMF_CTRL_T),
--
1.9.1
^ permalink raw reply related
* Re: BCM4313 & brcmsmac & 3.12: only semi-working?
From: Michael Tokarev @ 2014-11-19 9:04 UTC (permalink / raw)
To: Maximilian Engelhardt, Rafał Miłecki, Arend van Spriel
Cc: Seth Forshee, brcm80211 development,
linux-wireless@vger.kernel.org, Network Development
In-Reply-To: <3543341.FmUQFH9nrl@eisbaer>
18.11.2014 01:36, Maximilian Engelhardt wrote:
[]
> I just wanted to ask if there is any progress on this issue since I haven't
> heard anything for a month. Please let me know if you need any additional
> information.
I've no idea if there's any progress. Meanwhile I've an easy way of
testing of my brcm4313 card in a mini-itx board with mini-PCIe slot.
It works rather nicely and the stalls are easy to trigger.
Running 3.16 kernel right now, tried to d/load a file from the
AP, -- boom, it stalled after 77Kb.
Since the previous discussion apparently ended prematurely and no patches
to try emerged, I don't have anything to try on it...
Thanks,
/mjt
^ permalink raw reply
* Re: [PATCH V2 net] virtio-net: validate features during probe
From: Michael S. Tsirkin @ 2014-11-19 9:01 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1416381689-2025-1-git-send-email-jasowang@redhat.com>
On Wed, Nov 19, 2014 at 03:21:29PM +0800, Jason Wang wrote:
> This patch validates feature dependencies during probe and fail the probing
> if a dependency is missed. This fixes the issues of hitting BUG()
> when qemu fails to advertise features correctly. One example is booting
> guest with ctrl_vq=off through qemu.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - Drop VIRTIO_NET_F_*_UFO from the checklist, since it was disabled
In this form, it's not 3.18 material anyway.
Let's just focus on fixing BUG that you see for now,
and for 3.19, let's fix UFO.
> ---
> drivers/net/virtio_net.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 91 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ec2a8b4..b16a761 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1673,6 +1673,93 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> };
> #endif
>
> +static int virtnet_validate_features(struct virtio_device *dev,
> + unsigned int *table,
> + int table_size,
> + unsigned int feature)
> +{
> + int i;
> +
> + if (!virtio_has_feature(dev, feature)) {
> + for (i = 0; i < table_size; i++) {
> + unsigned int f = table[i];
> +
> + if (virtio_has_feature(dev, f)) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
> + f, feature);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int virtnet_check_features(struct virtio_device *dev)
> +{
> + unsigned int features_for_ctrl_vq[] = {
> + VIRTIO_NET_F_CTRL_RX,
> + VIRTIO_NET_F_CTRL_VLAN,
> + VIRTIO_NET_F_GUEST_ANNOUNCE,
> + VIRTIO_NET_F_MQ,
> + VIRTIO_NET_F_CTRL_MAC_ADDR
> + };
> + unsigned int features_for_guest_csum[] = {
> + VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6,
> + VIRTIO_NET_F_GUEST_ECN,
> + };
> + unsigned int features_for_host_csum[] = {
> + VIRTIO_NET_F_HOST_TSO4,
> + VIRTIO_NET_F_HOST_TSO6,
> + VIRTIO_NET_F_HOST_ECN,
> + };
> + int err;
> +
> + err = virtnet_validate_features(dev, features_for_ctrl_vq,
> + ARRAY_SIZE(features_for_ctrl_vq),
> + VIRTIO_NET_F_CTRL_VQ);
> + if (err)
> + return err;
> +
> + err = virtnet_validate_features(dev, features_for_guest_csum,
> + ARRAY_SIZE(features_for_guest_csum),
> + VIRTIO_NET_F_GUEST_CSUM);
> + if (err)
> + return err;
> +
> + err = virtnet_validate_features(dev, features_for_host_csum,
> + ARRAY_SIZE(features_for_host_csum),
> + VIRTIO_NET_F_CSUM);
> + if (err)
> + return err;
> +
> + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
> + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
> + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> + VIRTIO_NET_F_GUEST_ECN,
> + VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6);
> + return -EINVAL;
> + }
> +
> + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
> + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
> + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> + VIRTIO_NET_F_HOST_ECN,
> + VIRTIO_NET_F_HOST_TSO4,
> + VIRTIO_NET_F_HOST_TSO6);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
> @@ -1680,6 +1767,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> struct virtnet_info *vi;
> u16 max_queue_pairs;
>
> + err = virtnet_check_features(vdev);
> + if (err)
> + return -EINVAL;
> +
> /* Find if host supports multiqueue virtio_net device */
> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> struct virtio_net_config,
> --
> 1.9.1
^ permalink raw reply
* Re: [PATCH net] virtio-net: validate features during probe
From: Michael S. Tsirkin @ 2014-11-19 8:59 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1416378939-28821-1-git-send-email-jasowang@redhat.com>
On Wed, Nov 19, 2014 at 02:35:39PM +0800, Jason Wang wrote:
> This patch validates feature dependencies during probe and fail the probing
> if a dependency is missed. This fixes the issues of hitting BUG()
> when qemu fails to advertise features correctly. One example is booting
> guest with ctrl_vq=off through qemu.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ec2a8b4..4a0ad46 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> };
> #endif
>
> +static int virtnet_validate_features(struct virtio_device *dev,
> + unsigned int *table,
> + int table_size,
> + unsigned int feature)
> +{
> + int i;
> +
> + if (!virtio_has_feature(dev, feature)) {
> + for (i = 0; i < table_size; i++) {
> + unsigned int f = table[i];
> +
> + if (virtio_has_feature(dev, f)) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
This line's way too long.
> + f, feature);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int virtnet_check_features(struct virtio_device *dev)
> +{
> + unsigned int features_for_ctrl_vq[] = {
> + VIRTIO_NET_F_CTRL_RX,
> + VIRTIO_NET_F_CTRL_VLAN,
> + VIRTIO_NET_F_GUEST_ANNOUNCE,
> + VIRTIO_NET_F_MQ,
> + VIRTIO_NET_F_CTRL_MAC_ADDR
> + };
> + unsigned int features_for_guest_csum[] = {
> + VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6,
> + VIRTIO_NET_F_GUEST_ECN,
> + VIRTIO_NET_F_GUEST_UFO,
> + };
> + unsigned int features_for_host_csum[] = {
> + VIRTIO_NET_F_HOST_TSO4,
> + VIRTIO_NET_F_HOST_TSO6,
> + VIRTIO_NET_F_HOST_ECN,
> + VIRTIO_NET_F_HOST_UFO,
> + };
> + int err;
> +
> + err = virtnet_validate_features(dev, features_for_ctrl_vq,
> + ARRAY_SIZE(features_for_ctrl_vq),
> + VIRTIO_NET_F_CTRL_VQ);
> + if (err)
> + return err;
> +
> + err = virtnet_validate_features(dev, features_for_guest_csum,
> + ARRAY_SIZE(features_for_guest_csum),
> + VIRTIO_NET_F_GUEST_CSUM);
> + if (err)
> + return err;
> +
> + err = virtnet_validate_features(dev, features_for_host_csum,
> + ARRAY_SIZE(features_for_host_csum),
> + VIRTIO_NET_F_CSUM);
> + if (err)
> + return err;
> +
> + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
> + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
> + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> + VIRTIO_NET_F_GUEST_ECN,
> + VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6);
> + return -EINVAL;
> + }
> +
> + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
> + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
> + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> + VIRTIO_NET_F_HOST_ECN,
> + VIRTIO_NET_F_HOST_TSO4,
> + VIRTIO_NET_F_HOST_TSO6);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
> @@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> struct virtnet_info *vi;
> u16 max_queue_pairs;
>
> + err = virtnet_check_features(vdev);
> + if (err)
> + return -EINVAL;
> +
> /* Find if host supports multiqueue virtio_net device */
> err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
> struct virtio_net_config,
The API seems too complex, and you still had to open-code ECN logic.
Just open-code most of it. You can use a helper macro to output a
friendly message without code duplication.
For example like the below (completely untested)?
I would also like to split things: dependencies on
VIRTIO_NET_F_CTRL_VQ might go into this kernel,
since they help fix BUG.
Others should wait since they don't fix any crashes, and there's a small
chance of a regression for some hypervisor in the field.
-->
virtio-net: friendlier handling of misconfigured hypervisors
We currently trigger BUG when VIRTIO_NET_F_CTRL_VQ
is not set but one of features depending on it is.
That's not a friendly way to report errors to
hypervisors.
Let's check, and fail probe instead.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 26e1330..7a7d1a3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1673,6 +1673,21 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
};
#endif
+bool __virtnet_fail_on_feature(struct virtio_device *vdev, unsigned int fbit,
+ const char *fname)
+{
+ if (!virtio_has_feature(vdev, fbit))
+ return false;
+
+ dev_err(&dev->dev, "missing requirements for feature bit %d: %s\n",
+ fbit, fname);
+
+ return true;
+}
+
+#define VIRTNET_FAIL_ON(vdev, fbit) \
+ __virtnet_fail_on_feature(vdev, fbit, #fbit)
+
static int virtnet_probe(struct virtio_device *vdev)
{
int i, err;
@@ -1680,6 +1695,14 @@ static int virtnet_probe(struct virtio_device *vdev)
struct virtnet_info *vi;
u16 max_queue_pairs;
+ if (!virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ) &&
+ (VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_RX) ||
+ VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_VLAN) ||
+ VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) ||
+ VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_MQ) ||
+ VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_CTRL_MAC_ADDR)))
+ return -EINVAL;
+
/* Find if host supports multiqueue virtio_net device */
err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
struct virtio_net_config,
^ permalink raw reply related
* Re: [PATCH V2 net] virtio-net: validate features during probe
From: Cornelia Huck @ 2014-11-19 8:54 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, netdev, linux-kernel, virtualization
In-Reply-To: <1416381689-2025-1-git-send-email-jasowang@redhat.com>
On Wed, 19 Nov 2014 15:21:29 +0800
Jason Wang <jasowang@redhat.com> wrote:
> This patch validates feature dependencies during probe and fail the probing
> if a dependency is missed. This fixes the issues of hitting BUG()
> when qemu fails to advertise features correctly. One example is booting
> guest with ctrl_vq=off through qemu.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> Changes from V1:
> - Drop VIRTIO_NET_F_*_UFO from the checklist, since it was disabled
> ---
> drivers/net/virtio_net.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 91 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ec2a8b4..b16a761 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1673,6 +1673,93 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
> };
> #endif
>
> +static int virtnet_validate_features(struct virtio_device *dev,
> + unsigned int *table,
> + int table_size,
> + unsigned int feature)
> +{
> + int i;
> +
> + if (!virtio_has_feature(dev, feature)) {
Do an early return, get rid of one indentation level?
> + for (i = 0; i < table_size; i++) {
> + unsigned int f = table[i];
> +
> + if (virtio_has_feature(dev, f)) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
s/hyperviser/hypervisor/ (also below)
> + f, feature);
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int virtnet_check_features(struct virtio_device *dev)
> +{
> + unsigned int features_for_ctrl_vq[] = {
> + VIRTIO_NET_F_CTRL_RX,
> + VIRTIO_NET_F_CTRL_VLAN,
> + VIRTIO_NET_F_GUEST_ANNOUNCE,
> + VIRTIO_NET_F_MQ,
> + VIRTIO_NET_F_CTRL_MAC_ADDR
> + };
> + unsigned int features_for_guest_csum[] = {
> + VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6,
> + VIRTIO_NET_F_GUEST_ECN,
> + };
> + unsigned int features_for_host_csum[] = {
> + VIRTIO_NET_F_HOST_TSO4,
> + VIRTIO_NET_F_HOST_TSO6,
> + VIRTIO_NET_F_HOST_ECN,
> + };
I'm wondering whether it would be easier to read if you listed all
prereqs per feature instead of all features that depend on a feature?
It would still be hard to express the v4/v6 or conditions below in
tables, though.
Or call the arrays features_depending_on_foo?
> + int err;
> +
> + err = virtnet_validate_features(dev, features_for_ctrl_vq,
> + ARRAY_SIZE(features_for_ctrl_vq),
> + VIRTIO_NET_F_CTRL_VQ);
> + if (err)
> + return err;
If you already print a message that a user may use to fix their
hypervisor (or bug someone about it), would it make sense to check all
dependencies and print a full list of everything that is broken in the
advertised feature bits? I usually hate it if I fix one thing only to
hit the next bug when the program could have already told me about
everything I need to fix :)
> +
> + err = virtnet_validate_features(dev, features_for_guest_csum,
> + ARRAY_SIZE(features_for_guest_csum),
> + VIRTIO_NET_F_GUEST_CSUM);
> + if (err)
> + return err;
> +
> + err = virtnet_validate_features(dev, features_for_host_csum,
> + ARRAY_SIZE(features_for_host_csum),
> + VIRTIO_NET_F_CSUM);
> + if (err)
> + return err;
> +
> + if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
> + (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
> + !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
> + VIRTIO_NET_F_GUEST_ECN,
> + VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6);
> + return -EINVAL;
> + }
> +
> + if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
> + (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
> + !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
> + dev_err(&dev->dev,
> + "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
"Hypervisor bug: advertised feature <foo> but not <bar> or <baz>"
?
> + VIRTIO_NET_F_HOST_ECN,
> + VIRTIO_NET_F_HOST_TSO4,
> + VIRTIO_NET_F_HOST_TSO6);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
^ permalink raw reply
* Re: [PATCH 7/7] stmmac: dwmac-sti: Pass sysconfig register offset via syscon dt property.
From: Lee Jones @ 2014-11-19 8:51 UTC (permalink / raw)
To: Peter Griffin
Cc: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd,
netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-8-git-send-email-peter.griffin@linaro.org>
On Wed, 19 Nov 2014, Peter Griffin wrote:
> Based on Arnds review comments here https://lkml.org/lkml/2014/11/13/161,
> we should not be mixing address spaces in the reg property like this driver
> currently does. This patch updates the driver, dt docs and also the existing
> dt nodes to pass the sysconfig offset in the syscon dt property.
>
> This patch breaks DT compatibility! But this platform is considered WIP, and is only
> used by a few developers who are upstreaming support for it. This change has been done
> as a single atomic commit to ensure it is bisectable.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> Documentation/devicetree/bindings/net/sti-dwmac.txt | 14 +++++---------
> arch/arm/boot/dts/stih415.dtsi | 12 ++++++------
> arch/arm/boot/dts/stih416.dtsi | 12 ++++++------
> drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 13 +++++++------
> 4 files changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/sti-dwmac.txt b/Documentation/devicetree/bindings/net/sti-dwmac.txt
> index 6762a6b..d05c1e1 100644
> --- a/Documentation/devicetree/bindings/net/sti-dwmac.txt
> +++ b/Documentation/devicetree/bindings/net/sti-dwmac.txt
> @@ -9,14 +9,10 @@ The device node has following properties.
> Required properties:
> - compatible : Can be "st,stih415-dwmac", "st,stih416-dwmac",
> "st,stih407-dwmac", "st,stid127-dwmac".
> - - reg : Offset of the glue configuration register map in system
> - configuration regmap pointed by st,syscon property and size.
Looks like you are removing the reg property description completely?
> - - st,syscon : Should be phandle to system configuration node which
> - encompases this glue registers.
> + - st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
> + encompases the glue register, and the offset of the control register.
> - st,gmac_en: this is to enable the gmac into a dedicated sysctl control
> register available on STiH407 SoC.
> - - sti-ethconf: this is the gmac glue logic register to enable the GMAC,
> - select among the different modes and program the clk retiming.
> - pinctrl-0: pin-control for all the MII mode supported.
[...]
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
> @@ -123,7 +123,7 @@ struct sti_dwmac {
> bool ext_phyclk; /* Clock from external PHY */
> u32 tx_retime_src; /* TXCLK Retiming*/
> struct clk *clk; /* PHY clock */
> - int ctrl_reg; /* GMAC glue-logic control register */
> + u32 ctrl_reg; /* GMAC glue-logic control register */
> int clk_sel_reg; /* GMAC ext clk selection register */
> struct device *dev;
> struct regmap *regmap;
> @@ -286,11 +286,6 @@ static int sti_dwmac_parse_data(struct sti_dwmac *dwmac,
> if (!np)
> return -EINVAL;
>
> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sti-ethconf");
> - if (!res)
> - return -ENODATA;
> - dwmac->ctrl_reg = res->start;
> -
> /* clk selection from extra syscfg register */
> dwmac->clk_sel_reg = -ENXIO;
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sti-clkconf");
> @@ -301,6 +296,12 @@ static int sti_dwmac_parse_data(struct sti_dwmac *dwmac,
> if (IS_ERR(regmap))
> return PTR_ERR(regmap);
>
> + err = of_property_read_u32_index(np, "st,syscon", 1, &dwmac->ctrl_reg);
A few platforms have this format for sysconn now. We should toy with
the idea of either making this a standard call, and/or defining '1'.
> + if (err) {
> + dev_err(dev, "Can't get sysconfig ctrl offset (%d)\n", err);
> + return err;
> + }
> +
> dwmac->dev = dev;
> dwmac->interface = of_get_phy_mode(np);
> dwmac->regmap = regmap;
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 2/2] bonding: Introduce 4 AD link speed to fix agg_bandwidth
From: Xie Jianhua @ 2014-11-19 8:48 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jianhua Xie, Jianhua Xie
In-Reply-To: <1416386939-24591-1-git-send-email-Jianhua.Xie@freescale.com>
From: Jianhua Xie <Jianhua.Xie@freescale.com>
This patch adds [2.5|20|40|56] Gbps enum definition, and fixes
aggregated bandwidth calculation based on above slave links.
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: David S. Miller <davem@davemloft.net>
Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
---
drivers/net/bonding/bond_3ad.c | 38 +++++++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6bc27d9..8baa87d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -88,7 +88,11 @@ enum ad_link_speed_type {
AD_LINK_SPEED_10MBPS,
AD_LINK_SPEED_100MBPS,
AD_LINK_SPEED_1000MBPS,
- AD_LINK_SPEED_10000MBPS
+ AD_LINK_SPEED_2500MBPS,
+ AD_LINK_SPEED_10000MBPS,
+ AD_LINK_SPEED_20000MBPS,
+ AD_LINK_SPEED_40000MBPS,
+ AD_LINK_SPEED_56000MBPS
};
/* compare MAC addresses */
@@ -247,7 +251,11 @@ static inline int __check_agg_selection_timer(struct port *port)
* %AD_LINK_SPEED_10MBPS,
* %AD_LINK_SPEED_100MBPS,
* %AD_LINK_SPEED_1000MBPS,
+ * %AD_LINK_SPEED_2500MBPS,
* %AD_LINK_SPEED_10000MBPS
+ * %AD_LINK_SPEED_20000MBPS
+ * %AD_LINK_SPEED_40000MBPS
+ * %AD_LINK_SPEED_56000MBPS
*/
static u16 __get_link_speed(struct port *port)
{
@@ -275,10 +283,26 @@ static u16 __get_link_speed(struct port *port)
speed = AD_LINK_SPEED_1000MBPS;
break;
+ case SPEED_2500:
+ speed = AD_LINK_SPEED_2500MBPS;
+ break;
+
case SPEED_10000:
speed = AD_LINK_SPEED_10000MBPS;
break;
+ case SPEED_20000:
+ speed = AD_LINK_SPEED_20000MBPS;
+ break;
+
+ case SPEED_40000:
+ speed = AD_LINK_SPEED_40000MBPS;
+ break;
+
+ case SPEED_56000:
+ speed = AD_LINK_SPEED_56000MBPS;
+ break;
+
default:
/* unknown speed value from ethtool. shouldn't happen */
speed = 0;
@@ -639,9 +663,21 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
case AD_LINK_SPEED_1000MBPS:
bandwidth = aggregator->num_of_ports * 1000;
break;
+ case AD_LINK_SPEED_2500MBPS:
+ bandwidth = aggregator->num_of_ports * 2500;
+ break;
case AD_LINK_SPEED_10000MBPS:
bandwidth = aggregator->num_of_ports * 10000;
break;
+ case AD_LINK_SPEED_20000MBPS:
+ bandwidth = aggregator->num_of_ports * 20000;
+ break;
+ case AD_LINK_SPEED_40000MBPS:
+ bandwidth = aggregator->num_of_ports * 40000;
+ break;
+ case AD_LINK_SPEED_56000MBPS:
+ bandwidth = aggregator->num_of_ports * 56000;
+ break;
default:
bandwidth = 0; /* to silence the compiler */
}
--
2.1.0.27.g96db324
^ permalink raw reply related
* [PATCH 1/2] bonding: change AD_LINK_SPEED_BITMASK to enum to suport more speed
From: Xie Jianhua @ 2014-11-19 8:48 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jianhua Xie, Jianhua Xie
In-Reply-To: <1416386939-24591-1-git-send-email-Jianhua.Xie@freescale.com>
From: Jianhua Xie <Jianhua.Xie@freescale.com>
Port Key was determined as 16 bits according to the link speed,
duplex and user key (which is yet not supported). In the old
speed field, 5 bits are for speed [1|10|100|1000|10000]Mbps as
below:
--------------------------------------------------------------
Port key :| User key | Speed | Duplex|
--------------------------------------------------------------
16 6 1 0
This patch keeps the old layout, but changes AD_LINK_SPEED_BITMASK
from bit type to an enum type. In this way, the speed field can
expand speed type from 5 to 32.
CC: Jay Vosburgh <j.vosburgh@gmail.com>
CC: Veaceslav Falico <vfalico@gmail.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: David S. Miller <davem@davemloft.net>
Signed-off-by: Jianhua Xie <jianhua.xie@freescale.com>
---
drivers/net/bonding/bond_3ad.c | 66 ++++++++++++++++++++++--------------------
1 file changed, 34 insertions(+), 32 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 0a32143..6bc27d9 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -79,15 +79,17 @@
* --------------------------------------------------------------
* 16 6 1 0
*/
-#define AD_DUPLEX_KEY_BITS 0x1
-#define AD_SPEED_KEY_BITS 0x3E
-#define AD_USER_KEY_BITS 0xFFC0
-
-#define AD_LINK_SPEED_BITMASK_1MBPS 0x1
-#define AD_LINK_SPEED_BITMASK_10MBPS 0x2
-#define AD_LINK_SPEED_BITMASK_100MBPS 0x4
-#define AD_LINK_SPEED_BITMASK_1000MBPS 0x8
-#define AD_LINK_SPEED_BITMASK_10000MBPS 0x10
+#define AD_DUPLEX_KEY_MASKS 0x1
+#define AD_SPEED_KEY_MASKS 0x3E
+#define AD_USER_KEY_MASKS 0xFFC0
+
+enum ad_link_speed_type {
+ AD_LINK_SPEED_1MBPS = 1,
+ AD_LINK_SPEED_10MBPS,
+ AD_LINK_SPEED_100MBPS,
+ AD_LINK_SPEED_1000MBPS,
+ AD_LINK_SPEED_10000MBPS
+};
/* compare MAC addresses */
#define MAC_ADDRESS_EQUAL(A, B) \
@@ -240,12 +242,12 @@ static inline int __check_agg_selection_timer(struct port *port)
* __get_link_speed - get a port's speed
* @port: the port we're looking at
*
- * Return @port's speed in 802.3ad bitmask format. i.e. one of:
+ * Return @port's speed in 802.3ad enum format. i.e. one of:
* 0,
- * %AD_LINK_SPEED_BITMASK_10MBPS,
- * %AD_LINK_SPEED_BITMASK_100MBPS,
- * %AD_LINK_SPEED_BITMASK_1000MBPS,
- * %AD_LINK_SPEED_BITMASK_10000MBPS
+ * %AD_LINK_SPEED_10MBPS,
+ * %AD_LINK_SPEED_100MBPS,
+ * %AD_LINK_SPEED_1000MBPS,
+ * %AD_LINK_SPEED_10000MBPS
*/
static u16 __get_link_speed(struct port *port)
{
@@ -262,19 +264,19 @@ static u16 __get_link_speed(struct port *port)
else {
switch (slave->speed) {
case SPEED_10:
- speed = AD_LINK_SPEED_BITMASK_10MBPS;
+ speed = AD_LINK_SPEED_10MBPS;
break;
case SPEED_100:
- speed = AD_LINK_SPEED_BITMASK_100MBPS;
+ speed = AD_LINK_SPEED_100MBPS;
break;
case SPEED_1000:
- speed = AD_LINK_SPEED_BITMASK_1000MBPS;
+ speed = AD_LINK_SPEED_1000MBPS;
break;
case SPEED_10000:
- speed = AD_LINK_SPEED_BITMASK_10000MBPS;
+ speed = AD_LINK_SPEED_10000MBPS;
break;
default:
@@ -625,19 +627,19 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
if (aggregator->num_of_ports) {
switch (__get_link_speed(aggregator->lag_ports)) {
- case AD_LINK_SPEED_BITMASK_1MBPS:
+ case AD_LINK_SPEED_1MBPS:
bandwidth = aggregator->num_of_ports;
break;
- case AD_LINK_SPEED_BITMASK_10MBPS:
+ case AD_LINK_SPEED_10MBPS:
bandwidth = aggregator->num_of_ports * 10;
break;
- case AD_LINK_SPEED_BITMASK_100MBPS:
+ case AD_LINK_SPEED_100MBPS:
bandwidth = aggregator->num_of_ports * 100;
break;
- case AD_LINK_SPEED_BITMASK_1000MBPS:
+ case AD_LINK_SPEED_1000MBPS:
bandwidth = aggregator->num_of_ports * 1000;
break;
- case AD_LINK_SPEED_BITMASK_10000MBPS:
+ case AD_LINK_SPEED_10000MBPS:
bandwidth = aggregator->num_of_ports * 10000;
break;
default:
@@ -1011,7 +1013,7 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
port->sm_rx_state);
switch (port->sm_rx_state) {
case AD_RX_INITIALIZE:
- if (!(port->actor_oper_port_key & AD_DUPLEX_KEY_BITS))
+ if (!(port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS))
port->sm_vars &= ~AD_PORT_LACP_ENABLED;
else
port->sm_vars |= AD_PORT_LACP_ENABLED;
@@ -1318,7 +1320,7 @@ static void ad_port_selection_logic(struct port *port, bool *update_slave_arr)
/* update the new aggregator's parameters
* if port was responsed from the end-user
*/
- if (port->actor_oper_port_key & AD_DUPLEX_KEY_BITS)
+ if (port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS)
/* if port is full duplex */
port->aggregator->is_individual = false;
else
@@ -1846,7 +1848,7 @@ void bond_3ad_bind_slave(struct slave *slave)
/* if the port is not full duplex, then the port should be not
* lacp Enabled
*/
- if (!(port->actor_oper_port_key & AD_DUPLEX_KEY_BITS))
+ if (!(port->actor_oper_port_key & AD_DUPLEX_KEY_MASKS))
port->sm_vars &= ~AD_PORT_LACP_ENABLED;
/* actor system is the bond's system */
port->actor_system = BOND_AD_INFO(bond).system.sys_mac_addr;
@@ -2214,7 +2216,7 @@ void bond_3ad_adapter_speed_changed(struct slave *slave)
spin_lock_bh(&slave->bond->mode_lock);
- port->actor_admin_port_key &= ~AD_SPEED_KEY_BITS;
+ port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
port->actor_oper_port_key = port->actor_admin_port_key |=
(__get_link_speed(port) << 1);
netdev_dbg(slave->bond->dev, "Port %d changed speed\n", port->actor_port_number);
@@ -2247,7 +2249,7 @@ void bond_3ad_adapter_duplex_changed(struct slave *slave)
spin_lock_bh(&slave->bond->mode_lock);
- port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
+ port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
port->actor_oper_port_key = port->actor_admin_port_key |=
__get_duplex(port);
netdev_dbg(slave->bond->dev, "Port %d changed duplex\n", port->actor_port_number);
@@ -2289,18 +2291,18 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
*/
if (link == BOND_LINK_UP) {
port->is_enabled = true;
- port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
+ port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
port->actor_oper_port_key = port->actor_admin_port_key |=
__get_duplex(port);
- port->actor_admin_port_key &= ~AD_SPEED_KEY_BITS;
+ port->actor_admin_port_key &= ~AD_SPEED_KEY_MASKS;
port->actor_oper_port_key = port->actor_admin_port_key |=
(__get_link_speed(port) << 1);
} else {
/* link has failed */
port->is_enabled = false;
- port->actor_admin_port_key &= ~AD_DUPLEX_KEY_BITS;
+ port->actor_admin_port_key &= ~AD_DUPLEX_KEY_MASKS;
port->actor_oper_port_key = (port->actor_admin_port_key &=
- ~AD_SPEED_KEY_BITS);
+ ~AD_SPEED_KEY_MASKS);
}
netdev_dbg(slave->bond->dev, "Port %d changed link status to %s\n",
port->actor_port_number,
--
2.1.0.27.g96db324
^ permalink raw reply related
* [PATCH v2 net-next 0/2] bonding: Introduce 4 AD link speed
From: Xie Jianhua @ 2014-11-19 8:48 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Jay Vosburgh, Veaceslav Falico, Andy Gospodarek,
David S. Miller, Jianhua Xie
From: Jianhua Xie <Jianhua.Xie@freescale.com>
The speed field of AD Port Key was based on bitmask, it supported 5
kinds of link speed at most, as there were only 5 bits in the speed
field of the AD Port Key. This patches series change the speed type
(AD_LINK_SPEED_BITMASK) from bitmask to enum type in order to enhance
speed type from 5 to 32, and then introduce 4 AD link speed to fix
agg_bandwidth.
Jianhua Xie (2):
bonding: change AD_LINK_SPEED_BITMASK to enum to suport more speed
bonding: Introduce 4 AD link speed to fix agg_bandwidth
drivers/net/bonding/bond_3ad.c | 102 ++++++++++++++++++++++++++++-------------
1 file changed, 70 insertions(+), 32 deletions(-)
--
v2:
* Use an enum type to instead of the bitmask, not expand speed field,
v1:
* Compress RFC patches #2 to #5 into one single patch.
* Fix inexact commit information.
2.1.0.27.g96db324
^ 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