* 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
* Re: [PATCH 1/1] net: xfrm: Deletion of an unnecessary check before the function call "ipcomp_free_tfms"
From: Dan Carpenter @ 2014-11-19 8:45 UTC (permalink / raw)
To: SF Markus Elfring
Cc: David S. Miller, Herbert Xu, Steffen Klassert, netdev, LKML,
kernel-janitors, Coccinelle
In-Reply-To: <546BAFF5.5020603@users.sourceforge.net>
On Tue, Nov 18, 2014 at 09:45:41PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 18 Nov 2014 21:41:26 +0100
>
> 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...
regards,
dan carpenter
^ permalink raw reply
* Re: [patch net-next v3 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers
From: Jiri Pirko @ 2014-11-19 8:38 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+qp2SiKPFaRQDWcBxR-kiryfnsttT902iUnTi7=DZT6JQ@mail.gmail.com>
Wed, Nov 19, 2014 at 09:05:34AM CET, pshelar@nicira.com wrote:
>On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Use them to push skb->vlan_tci into the payload and avoid code
>> duplication.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>> drivers/net/vxlan.c | 22 ++++++----------------
>> include/linux/if_vlan.h | 34 ++++++++++++++++++++++++++++++++++
>> net/core/dev.c | 8 ++------
>> net/core/netpoll.c | 4 +---
>> net/ipv4/geneve.c | 11 +++--------
>> net/openvswitch/datapath.c | 6 +-----
>> net/openvswitch/vport-gre.c | 12 ++++--------
>> 7 files changed, 51 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index bb8fbab..64d45fa 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -1599,14 +1599,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
>> if (unlikely(err))
>> return err;
>>
>> - if (vlan_tx_tag_present(skb)) {
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - vlan_tx_tag_get(skb));
>> - if (WARN_ON(!skb))
>> - return -ENOMEM;
>> -
>> - skb->vlan_tci = 0;
>> - }
>> + skb = vlan_hwaccel_push_inside(skb);
>> + if (WARN_ON(!skb))
>> + return -ENOMEM;
>>
>> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
>> vxh->vx_flags = htonl(VXLAN_FLAGS);
>> @@ -1643,14 +1638,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
>> if (unlikely(err))
>> return err;
>>
>> - if (vlan_tx_tag_present(skb)) {
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - vlan_tx_tag_get(skb));
>> - if (WARN_ON(!skb))
>> - return -ENOMEM;
>> -
>> - skb->vlan_tci = 0;
>> - }
>> + skb = vlan_hwaccel_push_inside(skb);
>> + if (WARN_ON(!skb))
>> + return -ENOMEM;
>>
>> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
>> vxh->vx_flags = htonl(VXLAN_FLAGS);
>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> index 46e4a15..291e670 100644
>> --- a/include/linux/if_vlan.h
>> +++ b/include/linux/if_vlan.h
>> @@ -341,6 +341,40 @@ static inline struct sk_buff *vlan_insert_tag_set_proto(struct sk_buff *skb,
>> return skb;
>> }
>>
>> +/*
>> + * __vlan_hwaccel_push_inside - pushes vlan tag to the payload
>> + * @skb: skbuff to tag
>> + *
>> + * Pushes the VLAN tag from @skb->vlan_tci inside to the payload.
>> + *
>> + * Following the skb_unshare() example, in case of error, the calling function
>> + * doesn't have to worry about freeing the original skb.
>> + */
>> +static inline struct sk_buff *__vlan_hwaccel_push_inside(struct sk_buff *skb)
>> +{
>> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> + vlan_tx_tag_get(skb));
>> + if (likely(skb))
>> + skb->vlan_tci = 0;
>> + return skb;
>> +}
>> +/*
>> + * vlan_hwaccel_push_inside - pushes vlan tag to the payload
>> + * @skb: skbuff to tag
>> + *
>> + * Checks is tag is present in @skb->vlan_tci and if it is, it pushes the
>> + * VLAN tag from @skb->vlan_tci inside to the payload.
>> + *
>> + * Following the skb_unshare() example, in case of error, the calling function
>> + * doesn't have to worry about freeing the original skb.
>> + */
>> +static inline struct sk_buff *vlan_hwaccel_push_inside(struct sk_buff *skb)
>> +{
>> + if (vlan_tx_tag_present(skb))
>> + skb = __vlan_hwaccel_push_inside(skb);
>> + return skb;
>> +}
>> +
>> /**
>> * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
>> * @skb: skbuff to tag
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 3611e60..ac48362 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2644,12 +2644,8 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
>> netdev_features_t features)
>> {
>> if (vlan_tx_tag_present(skb) &&
>> - !vlan_hw_offload_capable(features, skb->vlan_proto)) {
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - vlan_tx_tag_get(skb));
>> - if (skb)
>> - skb->vlan_tci = 0;
>> - }
>> + !vlan_hw_offload_capable(features, skb->vlan_proto))
>> + skb = __vlan_hwaccel_push_inside(skb);
>> return skb;
>> }
>>
>> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
>> index 65d3723..e0ad5d1 100644
>> --- a/net/core/netpoll.c
>> +++ b/net/core/netpoll.c
>> @@ -79,8 +79,7 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
>>
>> if (vlan_tx_tag_present(skb) &&
>> !vlan_hw_offload_capable(features, skb->vlan_proto)) {
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - vlan_tx_tag_get(skb));
>> + skb = __vlan_hwaccel_push_inside(skb);
>> if (unlikely(!skb)) {
>> /* This is actually a packet drop, but we
>> * don't want the code that calls this
>> @@ -88,7 +87,6 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
>> */
>> goto out;
>> }
>> - skb->vlan_tci = 0;
>> }
>>
>> status = netdev_start_xmit(skb, dev, txq, false);
>> diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
>> index fd430a6..a457232 100644
>> --- a/net/ipv4/geneve.c
>> +++ b/net/ipv4/geneve.c
>> @@ -131,14 +131,9 @@ int geneve_xmit_skb(struct geneve_sock *gs, struct rtable *rt,
>> if (unlikely(err))
>> return err;
>>
>> - if (vlan_tx_tag_present(skb)) {
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - vlan_tx_tag_get(skb));
>> - if (unlikely(!skb)
>> - return -ENOMEM;
>> -
>> - skb->vlan_tci = 0;
>> - }
>> + skb = vlan_hwaccel_push_inside(skb);
>> + if (unlikely(!skb))
>> + return -ENOMEM;
>>
>> gnvh = (struct genevehdr *)__skb_push(skb, sizeof(*gnvh) + opt_len);
>> geneve_build_header(gnvh, tun_flags, vni, opt_len, opt);
>> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
>> index c63e60e..0cb9d4f 100644
>> --- a/net/openvswitch/datapath.c
>> +++ b/net/openvswitch/datapath.c
>> @@ -425,12 +425,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>> if (!nskb)
>> return -ENOMEM;
>>
>> - nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto,
>> - vlan_tx_tag_get(nskb));
>> - if (!nskb)
>> - return -ENOMEM;
>> + nskb = __vlan_hwaccel_push_inside(nskb);
>>
>> - nskb->vlan_tci = 0;
>
>Need to check for returned nskb for NULL case.
Oops, Fixed. Thanks.
>
>> skb = nskb;
>> }
>>
>> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
>> index 777cd8c..6b69df5 100644
>> --- a/net/openvswitch/vport-gre.c
>> +++ b/net/openvswitch/vport-gre.c
>> @@ -175,14 +175,10 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff *skb)
>> goto err_free_rt;
>> }
>>
>> - if (vlan_tx_tag_present(skb)) {
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - vlan_tx_tag_get(skb));
>> - if (unlikely(!skb) {
>> - err = -ENOMEM;
>> - goto err_free_rt;
>> - }
>> - skb->vlan_tci = 0;
>> + skb = vlan_hwaccel_push_inside(skb);
>> + if (unlikely(!skb)) {
>> + err = -ENOMEM;
>> + goto err_free_rt;
>> }
>>
>> /* Push Tunnel header. */
>> --
>> 1.9.3
>>
^ permalink raw reply
* Re: ipx: fix locking regression in ipx_sendmsg and ipx_recvmsg
From: Arnd Bergmann @ 2014-11-19 8:32 UTC (permalink / raw)
To: Jiri Bohac; +Cc: Arnaldo Carvalho de Melo, netdev, David Miller
In-Reply-To: <20141118221057.GA13473@midget.suse.cz>
On Tuesday 18 November 2014 23:10:57 Jiri Bohac wrote:
> On Tue, Nov 18, 2014 at 02:37:26PM +0100, Arnd Bergmann wrote:
> > Does ipxrtr_route_packet() actually sleep while waiting for the network,
> > or is it possible that you only need to change the recvmsg path?
>
> You're right.
> In fact, it can sleep in sock_alloc_send_skb(), but my patch does
> not fix this - it releases the lock after that.
> So let's ignore that for now, I'll send a V2 modifying only
> ipx_recvmsg().
Ok.
> > > if (sock_flag(sk, SOCK_ZAPPED))
> > > - goto out;
> > > + goto out_release;
> > >
> > > + release_sock(sk);
> > > skb = skb_recv_datagram(sk, flags & ~MSG_DONTWAIT,
> > > flags & MSG_DONTWAIT, &rc);
> > > if (!skb) {
> >
> > Same thing here: I think your patch could be simplified if you just
> > release the socket lock before calling skb_recv_datagram and get
> > it back afterwards,
>
> It would simplify the code a little to just get the lock again.
> But do we really want to optimize for source code size at the cost of
> taking locks that are not necessarry?
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
because you no longer call skb_free_datagram() in the success case.
Changing the locking only around the skb_recv_datagram() call would
have avoided this problem or at least (if I'm reading it wrong and
the patch is indeed correct) have made it easier to review what the
new code flow and what the change is.
> > and it would be much simpler if you could show that the lock is
> > not needed at all.
>
> At least the ipxitf_insert_socket() inside __ipx_bind() looks
> like it must be protected not to corrupt the intrfc->if_sklist.
> I am not familiar with the code, so there may be other things.
Ok. Better to do just a less invasive change then. Clearly this code
is not getting much testing, so leaving the locking in place (aside
from fixing the bug) seems the better choice.
Arnd
^ permalink raw reply
* [PATCH 7/7] stmmac: dwmac-sti: Pass sysconfig register offset via syscon dt property.
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd
Cc: peter.griffin, lee.jones, netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin@linaro.org>
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.
- - 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.
Optional properties:
@@ -40,10 +36,10 @@ ethernet0: dwmac@9630000 {
device_type = "network";
status = "disabled";
compatible = "st,stih407-dwmac", "snps,dwmac", "snps,dwmac-3.710";
- reg = <0x9630000 0x8000>, <0x80 0x4>;
- reg-names = "stmmaceth", "sti-ethconf";
+ reg = <0x9630000 0x8000>;
+ reg-names = "stmmaceth";
- st,syscon = <&syscfg_sbc_reg>;
+ st,syscon = <&syscfg_sbc_reg 0x80>;
st,gmac_en;
resets = <&softreset STIH407_ETH1_SOFTRESET>;
reset-names = "stmmaceth";
diff --git a/arch/arm/boot/dts/stih415.dtsi b/arch/arm/boot/dts/stih415.dtsi
index 9198c12..b346a1a 100644
--- a/arch/arm/boot/dts/stih415.dtsi
+++ b/arch/arm/boot/dts/stih415.dtsi
@@ -153,8 +153,8 @@
compatible = "st,stih415-dwmac", "snps,dwmac", "snps,dwmac-3.610";
status = "disabled";
- reg = <0xfe810000 0x8000>, <0x148 0x4>;
- reg-names = "stmmaceth", "sti-ethconf";
+ reg = <0xfe810000 0x8000>;
+ reg-names = "stmmaceth";
interrupts = <0 147 0>, <0 148 0>, <0 149 0>;
interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
@@ -165,7 +165,7 @@
snps,mixed-burst;
snps,force_sf_dma_mode;
- st,syscon = <&syscfg_rear>;
+ st,syscon = <&syscfg_rear 0x148>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_mii0>;
@@ -177,8 +177,8 @@
device_type = "network";
compatible = "st,stih415-dwmac", "snps,dwmac", "snps,dwmac-3.610";
status = "disabled";
- reg = <0xfef08000 0x8000>, <0x74 0x4>;
- reg-names = "stmmaceth", "sti-ethconf";
+ reg = <0xfef08000 0x8000>;
+ reg-names = "stmmaceth";
interrupts = <0 150 0>, <0 151 0>, <0 152 0>;
interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
@@ -186,7 +186,7 @@
snps,mixed-burst;
snps,force_sf_dma_mode;
- st,syscon = <&syscfg_sbc>;
+ st,syscon = <&syscfg_sbc 0x74>;
resets = <&softreset STIH415_ETH1_SOFTRESET>;
reset-names = "stmmaceth";
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 6fbde39..acce07d 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -163,8 +163,8 @@
device_type = "network";
compatible = "st,stih416-dwmac", "snps,dwmac", "snps,dwmac-3.710";
status = "disabled";
- reg = <0xfe810000 0x8000>, <0x8bc 0x4>;
- reg-names = "stmmaceth", "sti-ethconf";
+ reg = <0xfe810000 0x8000>;
+ reg-names = "stmmaceth";
interrupts = <0 133 0>, <0 134 0>, <0 135 0>;
interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
@@ -172,7 +172,7 @@
snps,pbl = <32>;
snps,mixed-burst;
- st,syscon = <&syscfg_rear>;
+ st,syscon = <&syscfg_rear 0x8bc>;
resets = <&softreset STIH416_ETH0_SOFTRESET>;
reset-names = "stmmaceth";
pinctrl-names = "default";
@@ -185,15 +185,15 @@
device_type = "network";
compatible = "st,stih416-dwmac", "snps,dwmac", "snps,dwmac-3.710";
status = "disabled";
- reg = <0xfef08000 0x8000>, <0x7f0 0x4>;
- reg-names = "stmmaceth", "sti-ethconf";
+ reg = <0xfef08000 0x8000>;
+ reg-names = "stmmaceth";
interrupts = <0 136 0>, <0 137 0>, <0 138 0>;
interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
snps,pbl = <32>;
snps,mixed-burst;
- st,syscon = <&syscfg_sbc>;
+ st,syscon = <&syscfg_sbc 0x7f0>;
resets = <&softreset STIH416_ETH1_SOFTRESET>;
reset-names = "stmmaceth";
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c
index ccfe7e5..ca2303c 100644
--- 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);
+ 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;
--
1.9.1
^ permalink raw reply related
* [PATCH 6/7] ARM: multi_v7_defconfig: Enable stih407 usb picophy
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd
Cc: peter.griffin, lee.jones, netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin@linaro.org>
This patch enables the picoPHY usb phy which is used by
the usb2 and usb3 host controllers when controlling usb2/1.1
devices. It is found in stih407 family SoC's from STMicroelectronics.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index e13ab7e..ac7b536 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -427,6 +427,7 @@ CONFIG_OMAP_USB2=y
CONFIG_TI_PIPE3=y
CONFIG_PHY_MIPHY365X=y
CONFIG_PHY_STIH41X_USB=y
+CONFIG_PHY_STIH407_USB=y
CONFIG_PHY_SUN4I_USB=y
CONFIG_EXT4_FS=y
CONFIG_VFAT_FS=y
--
1.9.1
^ permalink raw reply related
* [PATCH 5/7] ARM: STi: DT: STiH410: Add DT nodes for the ehci and ohci usb controllers.
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd
Cc: peter.griffin, lee.jones, netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin@linaro.org>
This patch adds the DT nodes for the extra ehci and ohci usb controllers
on the stih410 SoC.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stih410.dtsi | 52 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/arch/arm/boot/dts/stih410.dtsi b/arch/arm/boot/dts/stih410.dtsi
index 7fd9d57..e929d36 100644
--- a/arch/arm/boot/dts/stih410.dtsi
+++ b/arch/arm/boot/dts/stih410.dtsi
@@ -28,5 +28,57 @@
<&picophyreset STIH407_PICOPHY1_RESET>;
reset-names = "global", "port";
};
+
+ ohci0: usb@9a03c00 {
+ compatible = "st,st-ohci-300x";
+ reg = <0x9a03c00 0x100>;
+ interrupts = <GIC_SPI 180 IRQ_TYPE_NONE>;
+ clocks = <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>;
+ resets = <&powerdown STIH407_USB2_PORT0_POWERDOWN>,
+ <&softreset STIH407_USB2_PORT0_SOFTRESET>;
+ reset-names = "power", "softreset";
+ phys = <&usb2_picophy1>;
+ phy-names = "usb";
+ };
+
+ ehci0: usb@9a03e00 {
+ compatible = "st,st-ehci-300x";
+ reg = <0x9a03e00 0x100>;
+ interrupts = <GIC_SPI 151 IRQ_TYPE_NONE>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb0>;
+ clocks = <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>;
+ resets = <&powerdown STIH407_USB2_PORT0_POWERDOWN>,
+ <&softreset STIH407_USB2_PORT0_SOFTRESET>;
+ reset-names = "power", "softreset";
+ phys = <&usb2_picophy1>;
+ phy-names = "usb";
+ };
+
+ ohci1: usb@9a83c00 {
+ compatible = "st,st-ohci-300x";
+ reg = <0x9a83c00 0x100>;
+ interrupts = <GIC_SPI 181 IRQ_TYPE_NONE>;
+ clocks = <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>;
+ resets = <&powerdown STIH407_USB2_PORT1_POWERDOWN>,
+ <&softreset STIH407_USB2_PORT1_SOFTRESET>;
+ reset-names = "power", "softreset";
+ phys = <&usb2_picophy2>;
+ phy-names = "usb";
+ };
+
+ ehci1: usb@9a83e00 {
+ compatible = "st,st-ehci-300x";
+ reg = <0x9a83e00 0x100>;
+ interrupts = <GIC_SPI 153 IRQ_TYPE_NONE>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb1>;
+ clocks = <&clk_s_c0_flexgen CLK_TX_ICN_DISP_0>;
+ resets = <&powerdown STIH407_USB2_PORT1_POWERDOWN>,
+ <&softreset STIH407_USB2_PORT1_SOFTRESET>;
+ reset-names = "power", "softreset";
+ phys = <&usb2_picophy2>;
+ phy-names = "usb";
+ };
};
};
--
1.9.1
^ permalink raw reply related
* [PATCH 4/7] ARM: STi: DT: STiH410: Add usb2 picophy dt nodes
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd
Cc: peter.griffin, lee.jones, netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin@linaro.org>
This patch adds the dt nodes for the extra usb2 picophys found on the stih410.
These two picophys are used in conjunction with the extra ehci/ohci usb
controllers also found on the stih410.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stih410.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/arm/boot/dts/stih410.dtsi b/arch/arm/boot/dts/stih410.dtsi
index c05627e..7fd9d57 100644
--- a/arch/arm/boot/dts/stih410.dtsi
+++ b/arch/arm/boot/dts/stih410.dtsi
@@ -10,5 +10,23 @@
#include "stih407-family.dtsi"
#include "stih410-pinctrl.dtsi"
/ {
+ soc {
+ usb2_picophy1: phy@1 {
+ compatible = "st,stih407-usb2-phy";
+ #phy-cells = <0>;
+ st,syscfg = <&syscfg_core 0xf8 0xf4>;
+ resets = <&softreset STIH407_PICOPHY_SOFTRESET>,
+ <&picophyreset STIH407_PICOPHY0_RESET>;
+ reset-names = "global", "port";
+ };
+ usb2_picophy2: phy@2 {
+ compatible = "st,stih407-usb2-phy";
+ #phy-cells = <0>;
+ st,syscfg = <&syscfg_core 0xfc 0xf4>;
+ resets = <&softreset STIH407_PICOPHY_SOFTRESET>,
+ <&picophyreset STIH407_PICOPHY1_RESET>;
+ reset-names = "global", "port";
+ };
+ };
};
--
1.9.1
^ permalink raw reply related
* [PATCH 3/7] ARM: STi: DT: STiH407: Add usb2 picophy dt nodes
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd
Cc: peter.griffin, lee.jones, netdev, devicetree, alexandre.torgue
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin@linaro.org>
This patch adds the dt nodes for the usb2 picophy found on the stih407
device family. It is used on stih407 by the dwc3 usb3 controller when
controlling usb2/1.1 devices.
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
arch/arm/boot/dts/stih407-family.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index 3e31d32..e908477 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -274,5 +274,14 @@
status = "disabled";
};
+
+ usb2_picophy0: phy@0 {
+ compatible = "st,stih407-usb2-phy";
+ #phy-cells = <0>;
+ st,syscfg = <&syscfg_core 0x100 0xf4>;
+ resets = <&softreset STIH407_PICOPHY_SOFTRESET>,
+ <&picophyreset STIH407_PICOPHY0_RESET>;
+ reset-names = "global", "port";
+ };
};
};
--
1.9.1
^ permalink raw reply related
* [PATCH 2/7] phy: miphy365x: Pass sysconfig register offsets via syscfg dt property.
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, srinivas.kandagatla,
maxime.coquelin, patrice.chotard, peppe.cavallaro, kishon, arnd
Cc: peter.griffin, netdev, lee.jones, alexandre.torgue, devicetree
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin@linaro.org>
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.
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/Documentation/devicetree/bindings/phy/phy-miphy365x.txt b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
index 42c8808..9802d5d 100644
--- a/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
+++ b/Documentation/devicetree/bindings/phy/phy-miphy365x.txt
@@ -6,8 +6,10 @@ for SATA and PCIe.
Required properties (controller (parent) node):
- compatible : Should be "st,miphy365x-phy"
-- st,syscfg : Should be a phandle of the system configuration register group
- which contain the SATA, PCIe mode setting bits
+- st,syscfg : Phandle / integer array property. Phandle of sysconfig group
+ containing the miphy registers and integer array should contain
+ an entry for each port sub-node, specifying the control
+ register offset inside the sysconfig group.
Required nodes : A sub-node is required for each channel the controller
provides. Address range information including the usual
@@ -26,7 +28,6 @@ Required properties (port (child) node):
registers filled in "reg":
- sata: For SATA devices
- pcie: For PCIe devices
- - syscfg: To specify the syscfg based config register
Optional properties (port (child) node):
- st,sata-gen : Generation of locally attached SATA IP. Expected values
@@ -39,20 +40,20 @@ Example:
miphy365x_phy: miphy365x@fe382000 {
compatible = "st,miphy365x-phy";
- st,syscfg = <&syscfg_rear>;
+ st,syscfg = <&syscfg_rear 0x824 0x828>;
#address-cells = <1>;
#size-cells = <1>;
ranges;
phy_port0: port@fe382000 {
- reg = <0xfe382000 0x100>, <0xfe394000 0x100>, <0x824 0x4>;
- reg-names = "sata", "pcie", "syscfg";
+ reg = <0xfe382000 0x100>, <0xfe394000 0x100>;
+ reg-names = "sata", "pcie";
#phy-cells = <1>;
st,sata-gen = <3>;
};
phy_port1: port@fe38a000 {
- reg = <0xfe38a000 0x100>, <0xfe804000 0x100>, <0x828 0x4>;;
+ reg = <0xfe38a000 0x100>, <0xfe804000 0x100>;;
reg-names = "sata", "pcie", "syscfg";
#phy-cells = <1>;
st,pcie-tx-pol-inv;
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index fad9073..6fbde39 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -283,21 +283,21 @@
miphy365x_phy: phy@fe382000 {
compatible = "st,miphy365x-phy";
- st,syscfg = <&syscfg_rear>;
+ st,syscfg = <&syscfg_rear 0x824 0x828>;
#address-cells = <1>;
#size-cells = <1>;
ranges;
phy_port0: port@fe382000 {
#phy-cells = <1>;
- reg = <0xfe382000 0x100>, <0xfe394000 0x100>, <0x824 0x4>;
- reg-names = "sata", "pcie", "syscfg";
+ reg = <0xfe382000 0x100>, <0xfe394000 0x100>;
+ reg-names = "sata", "pcie";
};
phy_port1: port@fe38a000 {
#phy-cells = <1>;
- reg = <0xfe38a000 0x100>, <0xfe804000 0x100>, <0x828 0x4>;
- reg-names = "sata", "pcie", "syscfg";
+ reg = <0xfe38a000 0x100>, <0xfe804000 0x100>;
+ reg-names = "sata", "pcie";
};
};
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
@@ -141,7 +141,7 @@ struct miphy365x_phy {
bool pcie_tx_pol_inv;
bool sata_tx_pol_inv;
u32 sata_gen;
- u64 ctrlreg;
+ u32 ctrlreg;
u8 type;
};
@@ -179,7 +179,7 @@ static int miphy365x_set_path(struct miphy365x_phy *miphy_phy,
bool sata = (miphy_phy->type == MIPHY_TYPE_SATA);
return regmap_update_bits(miphy_dev->regmap,
- (unsigned int)miphy_phy->ctrlreg,
+ miphy_phy->ctrlreg,
SYSCFG_SELECT_SATA_MASK,
sata << SYSCFG_SELECT_SATA_POS);
}
@@ -445,7 +445,6 @@ int miphy365x_get_addr(struct device *dev, struct miphy365x_phy *miphy_phy,
{
struct device_node *phynode = miphy_phy->phy->dev.of_node;
const char *name;
- const __be32 *taddr;
int type = miphy_phy->type;
int ret;
@@ -455,22 +454,6 @@ int miphy365x_get_addr(struct device *dev, struct miphy365x_phy *miphy_phy,
return ret;
}
- if (!strncmp(name, "syscfg", 6)) {
- taddr = of_get_address(phynode, index, NULL, NULL);
- if (!taddr) {
- dev_err(dev, "failed to fetch syscfg address\n");
- return -EINVAL;
- }
-
- miphy_phy->ctrlreg = of_translate_address(phynode, taddr);
- if (miphy_phy->ctrlreg == OF_BAD_ADDR) {
- dev_err(dev, "failed to translate syscfg address\n");
- return -EINVAL;
- }
-
- return 0;
- }
-
if (!((!strncmp(name, "sata", 4) && type == MIPHY_TYPE_SATA) ||
(!strncmp(name, "pcie", 4) && type == MIPHY_TYPE_PCIE)))
return 0;
@@ -606,7 +589,15 @@ static int miphy365x_probe(struct platform_device *pdev)
return ret;
phy_set_drvdata(phy, miphy_dev->phys[port]);
+
port++;
+ /*sysconfig offsets are not indexed from zero */
+ 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);
--
1.9.1
^ permalink raw reply related
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