* [PATCH net] net/mlx4_en: Limit the RFS filter IDs to be < RPS_NO_FILTER
From: Or Gerlitz @ 2012-07-25 14:35 UTC (permalink / raw)
To: davem; +Cc: netdev, oren, Amir Vadai, Ben Hutchings, Or Gerliz
From: Amir Vadai <amirv@mellanox.com>
RFS filter id can't have the special value RPS_NO_FILTER,
need to skip it when allocating id's.
Also, changed an ifdef into a more elegant IS_DEFINED.
CC: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
Signed-off-by: Or Gerliz <ogerlitz@mellanox.com>
---
Addressing feedback from Ben Hutchings
drivers/net/ethernet/mellanox/mlx4/en_cq.c | 8 ++------
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
index aa9c2f6..866829b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
@@ -77,12 +77,8 @@ int mlx4_en_activate_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq *cq,
struct mlx4_en_dev *mdev = priv->mdev;
int err = 0;
char name[25];
- struct cpu_rmap *rmap =
-#ifdef CONFIG_RFS_ACCEL
- priv->dev->rx_cpu_rmap;
-#else
- NULL;
-#endif
+ struct cpu_rmap *rmap = IS_ENABLED(CONFIG_RFS_ACCEL) ?
+ priv->dev->rx_cpu_rmap : NULL;
cq->dev = mdev->pndev[priv->port];
cq->mcq.set_ci_db = cq->wqres.db.db;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 8864d8b..edd9cb8 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -201,7 +201,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip,
filter->flow_id = flow_id;
- filter->id = priv->last_filter_id++;
+ filter->id = priv->last_filter_id++ % RPS_NO_FILTER;
list_add_tail(&filter->next, &priv->filters);
hlist_add_head(&filter->filter_chain,
--
1.7.8.2
^ permalink raw reply related
* [PATCH RESEND net] net/mlx4_en: Limit the RFS filter IDs to be < RPS_NO_FILTER
From: Or Gerlitz @ 2012-07-25 14:36 UTC (permalink / raw)
To: davem; +Cc: netdev, oren, Amir Vadai, Ben Hutchings, Or Gerlitz
From: Amir Vadai <amirv@mellanox.com>
RFS filter id can't have the special value RPS_NO_FILTER,
need to skip it when allocating id's.
Also, changed an ifdef into a more elegant IS_DEFINED.
CC: Ben Hutchings <bhutchings@solarflare.com>
Signed-off-by: Amir Vadai <amirv@mellanox.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
Addressing feedback from Ben Hutchings
resending as of typo in my signature... sorry
drivers/net/ethernet/mellanox/mlx4/en_cq.c | 8 ++------
drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
index aa9c2f6..866829b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
@@ -77,12 +77,8 @@ int mlx4_en_activate_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq *cq,
struct mlx4_en_dev *mdev = priv->mdev;
int err = 0;
char name[25];
- struct cpu_rmap *rmap =
-#ifdef CONFIG_RFS_ACCEL
- priv->dev->rx_cpu_rmap;
-#else
- NULL;
-#endif
+ struct cpu_rmap *rmap = IS_ENABLED(CONFIG_RFS_ACCEL) ?
+ priv->dev->rx_cpu_rmap : NULL;
cq->dev = mdev->pndev[priv->port];
cq->mcq.set_ci_db = cq->wqres.db.db;
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 8864d8b..edd9cb8 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -201,7 +201,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip,
filter->flow_id = flow_id;
- filter->id = priv->last_filter_id++;
+ filter->id = priv->last_filter_id++ % RPS_NO_FILTER;
list_add_tail(&filter->next, &priv->filters);
hlist_add_head(&filter->filter_chain,
--
1.7.8.2
^ permalink raw reply related
* Re: [PATCH] mlx4: Add support for EEH error recovery
From: Shlomo Pongartz @ 2012-07-25 14:38 UTC (permalink / raw)
To: Or Gerlitz
Cc: David Miller, klebers, netdev, jackm, yevgenyp, ogerlitz,
cascardo, brking
In-Reply-To: <CAJZOPZKUw7DQQH5mFq4XK5GOZ_Wj6JmjFYi6no7aL_UrpO10vw@mail.gmail.com>
On 7/25/2012 1:30 AM, Or Gerlitz wrote:
> On Wed, Jul 25, 2012 at 12:03 AM, David Miller <davem@davemloft.net> wrote:
>
>> Or, you promised an ACK today, I still haven't seen it.
> It turned out that reacted we did, but not the ACK way.
>
> Again, code review wise, we intended to ack it, but Shlomo has set
> testing environment, under which he had some issues with the patch, as
> such he preferred not to ACK it but rather bring up the issues on the
> list and sort them out 1st. I thought it would be wrong to over-rule
> this preference of him, and this way is fair-enough with the author
> and your guide-lines, maybe I had to be more aggressive with ACKing
> this, as of the merge window closing coming. So tomorrow.
>
> Or.
>
>> There is no reason Kleber should be penalized and have his work
>> miss the merge window just because you guys can't be bothered
>> to approve this patch in a reasonable amount of time.
>>
>> Therefore I'm just going to apply it later today, and don't do this
>> with someone's submission ever again, it impedes progress and
>> frustrates contributors.
> .
>
The code looks fine and can be merged.
We will continue to test it under various conditions.
Sorry for the delay.
Thank you.
Shlomo Pongratz.
^ permalink raw reply
* Re: open sockets preventing unregister_netdevice from completing in linux-next (next-20120724)
From: Eric Dumazet @ 2012-07-25 14:38 UTC (permalink / raw)
To: Bjørn Mork; +Cc: netdev
In-Reply-To: <87boj4hs3k.fsf@nemi.mork.no>
On Wed, 2012-07-25 at 15:45 +0200, Bjørn Mork wrote:
> I am currently researching several power management regressions in
> linux-next as of next-20120724, spread over the PCI, USB and net
> subsystems. This one I believe belongs to the net subsystem, although I
> definitely may be wrong, mixing these together.
>
> My test case is:
>
> - open a ssh connection over a USB network device (qmi_wwan - which is why
> I am looking at this, but I really don't think it's the driver this time)
> - suspend laptop with netdev and ssh connection up
> - attempt to resume
>
> The USB device will be gone on resume because it is power cycled, so the
> drivers need to clean up, to let the device be rediscovered and bound
> again. But this does not happen anymore in linux-next as long as some
> socket is open. Instead we have these messages:
>
> Jul 25 15:13:11 nemi kernel: [ 7704.560306] unregister_netdevice: waiting for wwan0 to become free. Usage count = 1
> Jul 25 15:13:21 nemi kernel: [ 7714.800308] unregister_netdevice: waiting for wwan0 to become free. Usage count = 1
> Jul 25 15:13:31 nemi kernel: [ 7725.040316] unregister_netdevice: waiting for wwan0 to become free. Usage count = 1
>
>
> There are quite a few problems with the system in this state. Any write
> to the power/control associated with that USB device will hang,
> presumably because the USB device does not exist anymore. This will
> also make new attempts to suspend fail. And the USB device is of course
> not functional. The driver has not yet had a chance to clean up any of
> the other devices associated with the dead USB device (wwan1,
> /dev/cdc-wdm0 and /dev/cdc-wdm1), so these ghost devices will appear as
> non- functional. And new devices cannot be registered until the
> previous USB device is deleted and a new one created.
>
> Killing the ssh session let the unregister_netdevice continue and
> everything will be cleaned up and go back to normal.
>
> This is a regression compared to 3.5, where unregister_netdevice would
> succeed regardless of any open sockets. Or maybe the sockets were
> auto-reaped? I don't know the inner details - just observing the
> results.
>
> The test case above is quite normal operational mode for me. I often
> leave open sessions while suspending (because I intend to continue using
> them after resuming). And I always forget that this won't work for the
> USB modem case. I don't really care either. I expect the netdev to be
> removed, routes deleted and any sockets referencing either should just
> die or live on as zombies or whatever. The important part is that they
> should not prevent deletion of a netdev when e.g. the physical device is
> gone. That's the way things used to work.
>
>
Yes, we miss what was done with rt_cache_flush() : find all cached
routes and release all dev references...
^ permalink raw reply
* Re: [PATCH] ppp: add 64 bit stats
From: Kevin Groeneveld @ 2012-07-25 14:43 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1343189061.2626.11073.camel@edumazet-glaptop>
On Wed, Jul 25, 2012 at 12:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> It there are spinlocks already, why even adding u64_stats_sync ?
That is a good question I have already wondered about myself.
>include/linux/u64_stats_sync.h
>
>* 3) Write side must ensure mutual exclusion or one seqcount update could
> * be lost, thus blocking readers forever.
> * If this synchronization point is not a mutex, but a spinlock or
> * spinlock_bh() or disable_bh() :
It seems the u64_stats_sync requires some form of mutual exclusion.
So why bother ever using it at all? Maybe there are cases where the
required mutual exclusion can be cheaper than a spinlock? Maybe it is
just to avoid the spinlocks on the read side of things?
I hope you don't mind all my questions...
^ permalink raw reply
* Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
From: Ben Hutchings @ 2012-07-25 14:47 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
olaf@aepfle.de, apw@canonical.com, netdev@vger.kernel.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF9236A8B61@SN2PRD0310MB382.namprd03.prod.outlook.com>
On Wed, Jul 25, 2012 at 02:10:05PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > Sent: Tuesday, July 24, 2012 9:11 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> > apw@canonical.com; netdev@vger.kernel.org
> > Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
> >
> > On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > > In preparation to implementing IP injection, cleanup the way we propagate
> > > and handle errors both in the driver as well as in the user level daemon.
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > ---
> > > drivers/hv/hv_kvp.c | 112 +++++++++++++++++++++++++++++++++++++-
> > --------
> > > include/linux/hyperv.h | 17 +++++---
> > > tools/hv/hv_kvp_daemon.c | 70 +++++++++++++++-------------
> > > 3 files changed, 138 insertions(+), 61 deletions(-)
> > >
> > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > > index 0012eed..9b7fc4a 100644
> > > --- a/drivers/hv/hv_kvp.c
> > > +++ b/drivers/hv/hv_kvp.c
> > [...]
> > > @@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct
> > netlink_skb_parms *nsp)
> > > {
> > > struct hv_kvp_msg *message;
> > > struct hv_kvp_msg_enumerate *data;
> > > + int error = 0;
> > >
> > > message = (struct hv_kvp_msg *)msg->data;
> > > - switch (message->kvp_hdr.operation) {
> > > +
> > > + /*
> > > + * If we are negotiating the version information
> > > + * with the daemon; handle that first.
> > > + */
> > > +
> > > + if (in_hand_shake) {
> > > + if (kvp_handle_handshake(message))
> > > + in_hand_shake = false;
> > > + return;
> > > + }
> > > +
> > > + /*
> > > + * Based on the version of the daemon, we propagate errors from the
> > > + * daemon differently.
> > > + */
> > > +
> > > + data = &message->body.kvp_enum_data;
> > > +
> > > + switch (dm_reg_value) {
> > > case KVP_OP_REGISTER:
> > > - pr_info("KVP: user-mode registering done.\n");
> > > - kvp_register();
> > > - kvp_transaction.active = false;
> > > - hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
> > > + /*
> > > + * Null string is used to pass back error condition.
> > > + */
> > > + if (!strlen(data->data.key))
> >
> > Do we know that the key is null-terminated here? Shouldn't we just
> > check whether data->data.key[0] == 0?
>
> Yes, currently we do return null string to indicate error.
[...]
So the kernel should assume userland input is always valid?
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus
^ permalink raw reply
* RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: KY Srinivasan @ 2012-07-25 14:48 UTC (permalink / raw)
To: Ben Hutchings
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
olaf@aepfle.de, apw@canonical.com, netdev@vger.kernel.org
In-Reply-To: <1343179491.5132.112.camel@deadeye.wl.decadent.org.uk>
Ben,
At the outset I want to thank you for taking the time to review this code. Given that Greg
has indicated that he will not be able to look at this patch set till 3.6 and the nature of review
comments I have gotten from you and others, I will re-spin this patch set to address all the
comments I have gotten to date. Specific responses to your comments are in-line.
Regards,
K. Y
> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Tuesday, July 24, 2012 9:25 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
>
> On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > Implement the KVP verb - KVP_OP_SET_IP_INFO. This operation configures
> the
> > specified interface based on the given configuration. Since configuring
> > an interface is very distro specific, we invoke an external script to
> > configure the interface.
> [...]
> > +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> > +{
> > + char str[256];
> > + int error;
> > +
> > + memset(str, 0, sizeof(str));
> > + strcat(str, s1);
> > + if (s2 != NULL)
> > + strcat(str, s2);
> > + strcat(str, "=");
> > + strcat(str, s3);
> > + strcat(str, "\n");
> > +
> > + error = fputs(str, f);
>
> This style of string pasting is crazy; have you never heard of
> fprintf()?
>
> [...]
> > + /*
> > + * Set the configuration for the specified interface with
> > + * the information provided. Since there is no standard
> > + * way to configure an interface, we will have an external
> > + * script that does the job of configuring the interface and
> > + * flushing the configuration.
> > + *
> > + * The parameters passed to this external script are:
> > + * 1. A configuration file that has the specified configuration.
> > + *
> > + * We will embed the name of the interface in the configuration
> > + * file: ifcfg-ethx (where ethx is the interface name).
> > + *
> > + * Here is the format of the ip configuration file:
> > + *
> > + * HWADDR=macaddr
>
> Is the interface supposed to be matched by name or by MAC address?
I do not dictate that. My plan was to package all the information I have about
the interface and the desired configuration in a file and invoke the external
distro specific script to do its magic. This external script is free to ignore
what it does not need.
>
> > + * BOOTPROTO=dhcp (dhcp enabled for the interface)
>
> The BOOTPROTO line may or may not appear.
>
> > + * NM_CONTROLLED=no (this interface will not be controlled by NM)
> > + * PEERDNS=yes
>
> I wonder what the point is of including constant lines in the file.
> What is the external script supposed to do if it these apparent
> constants change in future?
As you can see, I did my testing on a RHEL system and I was too lazy to create
a RHEL specific config file in the external script and so I ended up creating pretty much
the config file needed by RHEL in this daemon. All the external script had to do was to
simply copy this file into the right location and bring up the interface. So, if you prefer
that we not populate the config file with these constant lines, I can get rid of them.
As I noted earlier external scripts may not choose to use all the information in the file
that this daemon generates. What I have done here, simplifies the external script
at least for one distro.
>
> > + * IPADDR_x=ipaddr
> > + * NETMASK_x=netmask
> > + * GATEWAY_x=gateway
> > + * DNSx=dns
>
> A strangely familiar format...
>
> > + * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> > + * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> > + * IPV6NETMASK.
> > + */
> > +
> > + memset(if_file, 0, sizeof(if_file));
> > + strcat(if_file, "/var/opt/hyperv/ifcfg-");
>
> Like I said before about the key-value files, this should be under
> /var/lib if the daemon is included in a distribution. You should
> perhaps use a macro for the "/var/opt" part so it can be overridden
> depending on whether it's built as a distribution or add-on package.
I will make this a macro.
>
> > + strcat(if_file, if_name);
> > +
> > + file = fopen(if_file, "w");
> > +
> > + if (file == NULL) {
> > + syslog(LOG_ERR, "Failed to open config file");
> > + return HV_E_FAIL;
> > + }
> > +
> > + /*
> > + * First write out the MAC address.
> > + */
> > +
> > + mac_addr = kvp_if_name_to_mac(if_name);
> > + if (mac_addr == NULL) {
> > + error = HV_E_FAIL;
> > + goto setval_error;
> > + }
> > +
> > + error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> > + if (error)
> > + goto setval_error;
> [...]
>
> This line isn't mentioned in the above comment.
>
> Ben.
>
> --
> Ben Hutchings
> If more than one person is responsible for a bug, no one is at fault.
^ permalink raw reply
* Re: [PATCH] ppp: add 64 bit stats
From: Eric Dumazet @ 2012-07-25 14:50 UTC (permalink / raw)
To: Kevin Groeneveld; +Cc: netdev
In-Reply-To: <CABF+-6VAWVM=Cpyjk4iTbG-bBfHUs_CfRhe7rXmxxiF_ZX7_OA@mail.gmail.com>
On Wed, 2012-07-25 at 10:43 -0400, Kevin Groeneveld wrote:
> On Wed, Jul 25, 2012 at 12:04 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > It there are spinlocks already, why even adding u64_stats_sync ?
>
> That is a good question I have already wondered about myself.
>
> >include/linux/u64_stats_sync.h
> >
> >* 3) Write side must ensure mutual exclusion or one seqcount update could
> > * be lost, thus blocking readers forever.
> > * If this synchronization point is not a mutex, but a spinlock or
> > * spinlock_bh() or disable_bh() :
>
> It seems the u64_stats_sync requires some form of mutual exclusion.
> So why bother ever using it at all? Maybe there are cases where the
> required mutual exclusion can be cheaper than a spinlock? Maybe it is
> just to avoid the spinlocks on the read side of things?
>
> I hope you don't mind all my questions...
u64_stats_sync is good for write sides without any locking,
for example using percpu data.
So if you use shared counters, u64_stats_sync has no value.
^ permalink raw reply
* RE: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
From: KY Srinivasan @ 2012-07-25 14:51 UTC (permalink / raw)
To: Ben Hutchings
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
olaf@aepfle.de, apw@canonical.com, netdev@vger.kernel.org
In-Reply-To: <20120725144724.GD1894@decadent.org.uk>
> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Wednesday, July 25, 2012 10:47 AM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> apw@canonical.com; netdev@vger.kernel.org
> Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
>
> On Wed, Jul 25, 2012 at 02:10:05PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > > Sent: Tuesday, July 24, 2012 9:11 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; olaf@aepfle.de;
> > > apw@canonical.com; netdev@vger.kernel.org
> > > Subject: Re: [PATCH 03/17] Drivers: hv: kvp: Cleanup error handling in KVP
> > >
> > > On Tue, 2012-07-24 at 09:01 -0700, K. Y. Srinivasan wrote:
> > > > In preparation to implementing IP injection, cleanup the way we propagate
> > > > and handle errors both in the driver as well as in the user level daemon.
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > ---
> > > > drivers/hv/hv_kvp.c | 112
> +++++++++++++++++++++++++++++++++++++-
> > > --------
> > > > include/linux/hyperv.h | 17 +++++---
> > > > tools/hv/hv_kvp_daemon.c | 70 +++++++++++++++-------------
> > > > 3 files changed, 138 insertions(+), 61 deletions(-)
> > > >
> > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> > > > index 0012eed..9b7fc4a 100644
> > > > --- a/drivers/hv/hv_kvp.c
> > > > +++ b/drivers/hv/hv_kvp.c
> > > [...]
> > > > @@ -109,27 +154,52 @@ kvp_cn_callback(struct cn_msg *msg, struct
> > > netlink_skb_parms *nsp)
> > > > {
> > > > struct hv_kvp_msg *message;
> > > > struct hv_kvp_msg_enumerate *data;
> > > > + int error = 0;
> > > >
> > > > message = (struct hv_kvp_msg *)msg->data;
> > > > - switch (message->kvp_hdr.operation) {
> > > > +
> > > > + /*
> > > > + * If we are negotiating the version information
> > > > + * with the daemon; handle that first.
> > > > + */
> > > > +
> > > > + if (in_hand_shake) {
> > > > + if (kvp_handle_handshake(message))
> > > > + in_hand_shake = false;
> > > > + return;
> > > > + }
> > > > +
> > > > + /*
> > > > + * Based on the version of the daemon, we propagate errors from the
> > > > + * daemon differently.
> > > > + */
> > > > +
> > > > + data = &message->body.kvp_enum_data;
> > > > +
> > > > + switch (dm_reg_value) {
> > > > case KVP_OP_REGISTER:
> > > > - pr_info("KVP: user-mode registering done.\n");
> > > > - kvp_register();
> > > > - kvp_transaction.active = false;
> > > > - hv_kvp_onchannelcallback(kvp_transaction.kvp_context);
> > > > + /*
> > > > + * Null string is used to pass back error condition.
> > > > + */
> > > > + if (!strlen(data->data.key))
> > >
> > > Do we know that the key is null-terminated here? Shouldn't we just
> > > check whether data->data.key[0] == 0?
> >
> > Yes, currently we do return null string to indicate error.
> [...]
>
> So the kernel should assume userland input is always valid?
Good point! This is the existing code and this patch-set cleans up all
the error handling and would not have this problem.
K. Y
^ permalink raw reply
* Re: [PATCH RESEND net] net/mlx4_en: Limit the RFS filter IDs to be < RPS_NO_FILTER
From: Ben Hutchings @ 2012-07-25 14:57 UTC (permalink / raw)
To: Or Gerlitz; +Cc: davem, netdev, oren, Amir Vadai
In-Reply-To: <1343226979-32004-1-git-send-email-ogerlitz@mellanox.com>
On Wed, 2012-07-25 at 17:36 +0300, Or Gerlitz wrote:
> From: Amir Vadai <amirv@mellanox.com>
>
> RFS filter id can't have the special value RPS_NO_FILTER,
> need to skip it when allocating id's.
>
> Also, changed an ifdef into a more elegant IS_DEFINED.
>
> CC: Ben Hutchings <bhutchings@solarflare.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>
> Addressing feedback from Ben Hutchings
>
> resending as of typo in my signature... sorry
--signoff is so much easier than typing it every time :-)
> drivers/net/ethernet/mellanox/mlx4/en_cq.c | 8 ++------
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
> index aa9c2f6..866829b 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
> @@ -77,12 +77,8 @@ int mlx4_en_activate_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq *cq,
> struct mlx4_en_dev *mdev = priv->mdev;
> int err = 0;
> char name[25];
> - struct cpu_rmap *rmap =
> -#ifdef CONFIG_RFS_ACCEL
> - priv->dev->rx_cpu_rmap;
> -#else
> - NULL;
> -#endif
> + struct cpu_rmap *rmap = IS_ENABLED(CONFIG_RFS_ACCEL) ?
> + priv->dev->rx_cpu_rmap : NULL;
This is a separate change.
> cq->dev = mdev->pndev[priv->port];
> cq->mcq.set_ci_db = cq->wqres.db.db;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 8864d8b..edd9cb8 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -201,7 +201,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip,
>
> filter->flow_id = flow_id;
>
> - filter->id = priv->last_filter_id++;
> + filter->id = priv->last_filter_id++ % RPS_NO_FILTER;
This should do for now.
Ideally the filter ID would be based on the index used in hardware. We
would need to change the API to allow for devices where this cannot be
determined synchronously, as in this driver. (And the filter ID might
also need to be wider.)
Ben.
>
> list_add_tail(&filter->next, &priv->filters);
> hlist_add_head(&filter->filter_chain,
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH RESEND net] net/mlx4_en: Limit the RFS filter IDs to be < RPS_NO_FILTER
From: Ben Hutchings @ 2012-07-25 14:57 UTC (permalink / raw)
To: Or Gerlitz; +Cc: davem, netdev, oren, Amir Vadai
In-Reply-To: <1343226979-32004-1-git-send-email-ogerlitz@mellanox.com>
On Wed, 2012-07-25 at 17:36 +0300, Or Gerlitz wrote:
> From: Amir Vadai <amirv@mellanox.com>
>
> RFS filter id can't have the special value RPS_NO_FILTER,
> need to skip it when allocating id's.
>
> Also, changed an ifdef into a more elegant IS_DEFINED.
>
> CC: Ben Hutchings <bhutchings@solarflare.com>
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>
> Addressing feedback from Ben Hutchings
>
> resending as of typo in my signature... sorry
--signoff is so much easier than typing it every time :-)
> drivers/net/ethernet/mellanox/mlx4/en_cq.c | 8 ++------
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
> 2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
> index aa9c2f6..866829b 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
> @@ -77,12 +77,8 @@ int mlx4_en_activate_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq *cq,
> struct mlx4_en_dev *mdev = priv->mdev;
> int err = 0;
> char name[25];
> - struct cpu_rmap *rmap =
> -#ifdef CONFIG_RFS_ACCEL
> - priv->dev->rx_cpu_rmap;
> -#else
> - NULL;
> -#endif
> + struct cpu_rmap *rmap = IS_ENABLED(CONFIG_RFS_ACCEL) ?
> + priv->dev->rx_cpu_rmap : NULL;
This ought to be a separate change really.
> cq->dev = mdev->pndev[priv->port];
> cq->mcq.set_ci_db = cq->wqres.db.db;
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 8864d8b..edd9cb8 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -201,7 +201,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip,
>
> filter->flow_id = flow_id;
>
> - filter->id = priv->last_filter_id++;
> + filter->id = priv->last_filter_id++ % RPS_NO_FILTER;
This should do for now.
Ideally the filter ID would be based on the index used in hardware. We
would need to change the API to allow for devices where this cannot be
determined synchronously, as in this driver. (And the filter ID might
also need to be wider.)
Ben.
>
> list_add_tail(&filter->next, &priv->filters);
> hlist_add_head(&filter->filter_chain,
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] sctp: Make "Invalid Stream Identifier" ERROR follows SACK when bundling
From: Vlad Yasevich @ 2012-07-25 15:00 UTC (permalink / raw)
To: Xufeng Zhang
Cc: Neil Horman, xufeng zhang, sri, davem, linux-sctp, netdev,
linux-kernel
In-Reply-To: <CA+=dFzgXq3Sx-Ny+6oDZ3BEdsMQCgswNEVPVnhmhedHFvazzWw@mail.gmail.com>
On 07/25/2012 05:22 AM, Xufeng Zhang wrote:
> On 7/25/12, Xufeng Zhang <xufengzhang.main@gmail.com> wrote:
>> On 7/25/12, Vlad Yasevich <vyasevich@gmail.com> wrote:
>>>
>>> Actually not true. AUTH can be before SACK. So can any addip chunks
>>> that
>>> aid in locating an association.
>>>
>>> Now AUTH isn't a big issue since its autogenerated to the packet but
>>> ADDIP
>>> is since it could be queued up for retransmission.
>>>
>>> There could be other extensions as well. It really needs to be done
>>> either
>>> through side effects or making error chunks go at the end of other
>>> control
>>> chunks. Need to audit the spec to see if that's ok.
>> You are right, I just found SHUTDOWN chunks are also before SACK based on
>> your commit "[SCTP]: Fix SACK sequence during shutdown".
>> Maybe the only solution is to do some work on side effects just as you
>> said.
>> Thanks for your explanation!
>
> And after take a moment to look into the relative codes, I think we
> can implement it
> by below way:
> 1). Add a flag(isi_err_needed) in the embedded struct peer of struct
> struct sctp_association
> just like sack_needed flag.
> 2). When "invalid stream identifier" ERROR happens in sctp_eat_data()
> function, we just
> set isi_err_needed flag and don't create ERROR chunk and also don't
> insert SCTP_CMD_REPLY command.
> 3). In sctp_gen_sack() function, we create ERROR chunk and also insert
> SCTP_CMD_REPLY command if isi_err_needed flag is set.
>
> Is this way proper?
>
So, I looked at the code, and it looks very simple to do. We already
return a specific status from sctp_eat_data() when the error was
generated. All you have to do is take the code that generates the error
and adds it to the command list and give it its own small function that
you can then call if SCTP_IERROR_BAD_STREAM error was returned.
-vlad
>
> Thanks,
> Xufeng Zhang
>>
>>
>>
>> Thanks,
>> Xufeng Zhang
>>>
>>> -vlad
>>>>
>>>> Thanks,
>>>> Xufeng Zhang
>>>>>
>>>>> -vlad
>>>
>>>
>>> --
>>> Sent from my Android phone with SkitMail. Please excuse my brevity.
>>>
>>
^ permalink raw reply
* Re: [PATCH] mlx4: Add support for EEH error recovery
From: Kleber Sacilotto de Souza @ 2012-07-25 15:02 UTC (permalink / raw)
To: Shlomo Pongartz
Cc: Or Gerlitz, David Miller, netdev, jackm, yevgenyp, ogerlitz,
cascardo, brking
In-Reply-To: <5010070B.5040405@mellanox.com>
On 07/25/2012 11:47 AM, Shlomo Pongartz wrote:
> On 7/25/2012 1:30 AM, Or Gerlitz wrote:
>> On Wed, Jul 25, 2012 at 12:03 AM, David Miller <davem@davemloft.net> wrote:
>>
>>> Or, you promised an ACK today, I still haven't seen it.
>> It turned out that reacted we did, but not the ACK way.
>>
>> Again, code review wise, we intended to ack it, but Shlomo has set
>> testing environment, under which he had some issues with the patch, as
>> such he preferred not to ACK it but rather bring up the issues on the
>> list and sort them out 1st. I thought it would be wrong to over-rule
>> this preference of him, and this way is fair-enough with the author
>> and your guide-lines, maybe I had to be more aggressive with ACKing
>> this, as of the merge window closing coming. So tomorrow.
>>
>> Or.
>>
>>> There is no reason Kleber should be penalized and have his work
>>> miss the merge window just because you guys can't be bothered
>>> to approve this patch in a reasonable amount of time.
>>>
>>> Therefore I'm just going to apply it later today, and don't do this
>>> with someone's submission ever again, it impedes progress and
>>> frustrates contributors.
>> .
>>
>
> Hi Kleber,
>
>
>
> I reviewed the patch and it seems fine, here's my ACK
>
>
>
> Acked-by: Shlomo Pongratz <shlomop@mellanox.com
> <mailto:shlomop@mellanox.com>>
>
>
Hi Shlomo,
Thank you for reviewing my patch.
--
Kleber Sacilotto de Souza
IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH RESEND net] net/mlx4_en: Limit the RFS filter IDs to be < RPS_NO_FILTER
From: Or Gerlitz @ 2012-07-25 15:04 UTC (permalink / raw)
To: Ben Hutchings; +Cc: davem, netdev, oren, Amir Vadai
In-Reply-To: <1343228230.2694.12.camel@bwh-desktop.uk.solarflarecom.com>
On 25/07/2012 17:57, Ben Hutchings wrote:
>
> resending as of typo in my signature... sorry
>
> --signoff is so much easier than typing it every time :-)
I know and I do that on the regular basis, today did that differently
and here's the result...
Or.
>
>
>> drivers/net/ethernet/mellanox/mlx4/en_cq.c | 8 ++------
>> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
>> 2 files changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_cq.c b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
>> index aa9c2f6..866829b 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_cq.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_cq.c
>> @@ -77,12 +77,8 @@ int mlx4_en_activate_cq(struct mlx4_en_priv *priv, struct mlx4_en_cq *cq,
>> struct mlx4_en_dev *mdev = priv->mdev;
>> int err = 0;
>> char name[25];
>> - struct cpu_rmap *rmap =
>> -#ifdef CONFIG_RFS_ACCEL
>> - priv->dev->rx_cpu_rmap;
>> -#else
>> - NULL;
>> -#endif
>> + struct cpu_rmap *rmap = IS_ENABLED(CONFIG_RFS_ACCEL) ?
>> + priv->dev->rx_cpu_rmap : NULL;
>
> This is a separate change.
OK, will send two patches
>
>
>> cq->dev = mdev->pndev[priv->port];
>> cq->mcq.set_ci_db = cq->wqres.db.db;
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> index 8864d8b..edd9cb8 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
>> @@ -201,7 +201,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip,
>>
>> filter->flow_id = flow_id;
>>
>> - filter->id = priv->last_filter_id++;
>> + filter->id = priv->last_filter_id++ % RPS_NO_FILTER;
>
> This should do for now.
thanks
Or.
>
>
> Ideally the filter ID would be based on the index used in hardware. We
> would need to change the API to allow for devices where this cannot be
> determined synchronously, as in this driver. (And the filter ID might
> also need to be wider.)
>
> Ben.
>
>>
>> list_add_tail(&filter->next,&priv->filters);
>> hlist_add_head(&filter->filter_chain,
>
^ permalink raw reply
* [PATCH] ipv4: rt_cache_valid must check expired routes
From: Eric Dumazet @ 2012-07-25 15:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
commit d2d68ba9fe8 (ipv4: Cache input routes in fib_info nexthops.)
introduced rt_cache_valid() helper. It unfortunately doesn't check if
route is expired before caching it.
I noticed sk_setup_caps() was constantly called on a tcp workload.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
This patch is bringed by an European guy, but its still July after
all ;)
net/ipv4/route.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 6bcb8fc..30e7021 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -444,7 +444,7 @@ static inline int ip_rt_proc_init(void)
}
#endif /* CONFIG_PROC_FS */
-static inline int rt_is_expired(struct rtable *rth)
+static inline bool rt_is_expired(const struct rtable *rth)
{
return rth->rt_genid != rt_genid(dev_net(rth->dst.dev));
}
@@ -1222,9 +1222,11 @@ static void rt_cache_route(struct fib_nh *nh, struct rtable *rt)
}
}
-static bool rt_cache_valid(struct rtable *rt)
+static bool rt_cache_valid(const struct rtable *rt)
{
- return (rt && rt->dst.obsolete == DST_OBSOLETE_FORCE_CHK);
+ return rt &&
+ rt->dst.obsolete == DST_OBSOLETE_FORCE_CHK &&
+ !rt_is_expired(rt);
}
static void rt_set_nexthop(struct rtable *rt, __be32 daddr,
^ permalink raw reply related
* Re: [PATCH RESEND net-next V2] IB/ipoib: break linkage to neighbouring system
From: Or Gerlitz @ 2012-07-25 16:19 UTC (permalink / raw)
To: Roland Dreier; +Cc: David Miller, netdev, eric.dumazet, cl, shlomop
In-Reply-To: <CAG4TOxNrywFPj_91RzkPikPoWP4hwe+2oninuF7zkrBFo6Q3Kg@mail.gmail.com>
On 25/07/2012 10:52, Roland Dreier wrote:
> On Tue, Jul 24, 2012 at 2:00 PM, David Miller<davem@davemloft.net> wrote:
>> Looks good, Roland you got this?
>
> Yes, I'll read it over and pick it up, thanks.
great, lets us know if anything need to be clarified, waiting to hear
from you...
^ permalink raw reply
* broken nat on linus' latest tree.
From: Dave Jones @ 2012-07-25 17:44 UTC (permalink / raw)
To: netdev
I put Linus' latest tree on my firewall, and found that none
of the machines in my house can talk to the net any more.
The setup is pretty basic. eth0 talks to the cable modem,
eth1 talks to lan. ip masq between them.
Running tcpdump, when I run a ping from another machine to the internet,
I see icmp requests arriving on eth1.
I see those requests and their replies going in/out of eth0, but I don't
see the replies being forwarded back to eth1.
(It happens with all traffic, not just icmp).
3.5 works, this is a post-merge regression.
Any ideas ?
Dave
^ permalink raw reply
* [PATCH net-next,2/2] hyperv: Add error handling to rndis_filter_device_add()
From: Haiyang Zhang @ 2012-07-25 18:08 UTC (permalink / raw)
To: davem, netdev; +Cc: haiyangz, kys, olaf, linux-kernel, devel
In-Reply-To: <1343239722-21328-1-git-send-email-haiyangz@microsoft.com>
Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/net/hyperv/rndis_filter.c | 11 ++++-------
1 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index fbf5394..e5d6146 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -804,18 +804,15 @@ int rndis_filter_device_add(struct hv_device *dev,
/* Send the rndis initialization message */
ret = rndis_filter_init_device(rndis_device);
if (ret != 0) {
- /*
- * TODO: If rndis init failed, we will need to shut down the
- * channel
- */
+ rndis_filter_device_remove(dev);
+ return ret;
}
/* Get the mac address */
ret = rndis_filter_query_device_mac(rndis_device);
if (ret != 0) {
- /*
- * TODO: shutdown rndis device and the channel
- */
+ rndis_filter_device_remove(dev);
+ return ret;
}
memcpy(device_info->mac_adr, rndis_device->hw_mac_adr, ETH_ALEN);
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 1/6] ipv4: Delete routing cache.
From: Ben Greear @ 2012-07-25 18:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20120712.104253.1298961759089179385.davem@davemloft.net>
I just ran a quick test on yesterday's net-next, and our
virtual-router logic appears to work fine.
This logic sets up lots of routing rules and routing tables
and basically routes the same packet through multiple
VETH links.
We'll do some more extensive testing, including performance related stuff
when we move to newer upstream kernels with this feature.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* [PATCH net-next,1/2] hyperv: Add a check for ring_size value
From: Haiyang Zhang @ 2012-07-25 18:08 UTC (permalink / raw)
To: davem, netdev; +Cc: haiyangz, kys, olaf, linux-kernel, devel
It prevents ring_size being set to a too small value.
Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Reviewed-by: K. Y. Srinivasan <kys@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 8e23c08..8c5a1c4 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -47,7 +47,7 @@ struct net_device_context {
struct work_struct work;
};
-
+#define RING_SIZE_MIN 64
static int ring_size = 128;
module_param(ring_size, int, S_IRUGO);
MODULE_PARM_DESC(ring_size, "Ring buffer size (# of pages)");
@@ -518,6 +518,11 @@ static void __exit netvsc_drv_exit(void)
static int __init netvsc_drv_init(void)
{
+ if (ring_size < RING_SIZE_MIN) {
+ ring_size = RING_SIZE_MIN;
+ pr_info("Increased ring_size to %d (min allowed)\n",
+ ring_size);
+ }
return vmbus_driver_register(&netvsc_drv);
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH] igb: correct hardware type (i210/i211) check in igb_loopback_test()
From: Jesper Juhl @ 2012-07-25 19:05 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, e1000-devel, Carolyn Wyborny, Jeff Pieper, Jeff Kirsher,
Rick Jones, John Ronciak, Jesse Brandeburg, Bruce Allan,
Don Skidmore, Greg Rose, Peter P Waskiewicz Jr, Alex Duyck,
David S. Miller
In the original code
...
if ((adapter->hw.mac.type == e1000_i210)
|| (adapter->hw.mac.type == e1000_i210)) {
...
the second check of 'adapter->hw.mac.type' is pointless since it tests
for the exact same value as the first.
After reading through a few other parts of the driver I believe that
the second check was actually intended to check for 'e1000_i211'
rather than 'e1000_i210', but I admit that I'm not certain so someone
with more knowledge about this driver should ACK the patch before it
gets merged.
Unfortunately I have no hardware to actually test this on, so it is
compile tested only.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index a19c84c..ad489b7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1783,7 +1783,7 @@ static int igb_loopback_test(struct igb_adapter *adapter, u64 *data)
goto out;
}
if ((adapter->hw.mac.type == e1000_i210)
- || (adapter->hw.mac.type == e1000_i210)) {
+ || (adapter->hw.mac.type == e1000_i211)) {
dev_err(&adapter->pdev->dev,
"Loopback test not supported "
"on this part at this time.\n");
--
1.7.11.3
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply related
* Re: [PATCH] igb: correct hardware type (i210/i211) check in igb_loopback_test()
From: Wyborny, Carolyn @ 2012-07-25 19:13 UTC (permalink / raw)
To: Jesper Juhl, linux-kernel@vger.kernel.org
Cc: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org,
Rick Jones, Allan, Bruce W, Brandeburg, Jesse, Ronciak, John,
David S. Miller
In-Reply-To: <alpine.LNX.2.00.1207252059320.11754@swampdragon.chaosbits.net>
-----Original Message-----
From: Jesper Juhl [mailto:jj@chaosbits.net]
Sent: Wednesday, July 25, 2012 12:06 PM
To: linux-kernel@vger.kernel.org
Cc: netdev@vger.kernel.org; e1000-devel@lists.sourceforge.net; Wyborny, Carolyn; Pieper, Jeffrey E; Kirsher, Jeffrey T; Rick Jones; Ronciak, John; Brandeburg, Jesse; Allan, Bruce W; Skidmore, Donald C; Rose, Gregory V; Waskiewicz Jr, Peter P; Duyck, Alexander H; David S. Miller
Subject: [PATCH] igb: correct hardware type (i210/i211) check in igb_loopback_test()
In the original code
...
if ((adapter->hw.mac.type == e1000_i210)
|| (adapter->hw.mac.type == e1000_i210)) { ...
the second check of 'adapter->hw.mac.type' is pointless since it tests for the exact same value as the first.
After reading through a few other parts of the driver I believe that the second check was actually intended to check for 'e1000_i211'
rather than 'e1000_i210', but I admit that I'm not certain so someone with more knowledge about this driver should ACK the patch before it gets merged.
Unfortunately I have no hardware to actually test this on, so it is compile tested only.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index a19c84c..ad489b7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1783,7 +1783,7 @@ static int igb_loopback_test(struct igb_adapter *adapter, u64 *data)
goto out;
}
if ((adapter->hw.mac.type == e1000_i210)
- || (adapter->hw.mac.type == e1000_i210)) {
+ || (adapter->hw.mac.type == e1000_i211)) {
dev_err(&adapter->pdev->dev,
"Loopback test not supported "
"on this part at this time.\n");
--
1.7.11.3
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
ACK.
Good catch. Thanks Jesper!
Carolyn
Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH] igb: don't break user visible strings over multiple lines in igb_ethtool.c
From: Jesper Juhl @ 2012-07-25 19:19 UTC (permalink / raw)
To: linux-kernel
Cc: e1000-devel, netdev, Rick Jones, Bruce Allan, Jesse Brandeburg,
David S. Miller
Even when they go beyond 80 characters, user visible strings should be
on one line to make them easy to grep for.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
drivers/net/ethernet/intel/igb/igb_ethtool.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
This patch was generated on top of another patch I submitted a few minutes ago:
Subject: [PATCH] igb: correct hardware type (i210/i211) check in igb_loopback_test()
Message-ID: <alpine.LNX.2.00.1207252059320.11754@swampdragon.chaosbits.net>
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index ad489b7..90550f5 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -209,8 +209,8 @@ static int igb_set_settings(struct net_device *netdev, struct ethtool_cmd *ecmd)
/* When SoL/IDER sessions are active, autoneg/speed/duplex
* cannot be changed */
if (igb_check_reset_block(hw)) {
- dev_err(&adapter->pdev->dev, "Cannot change link "
- "characteristics when SoL/IDER is active.\n");
+ dev_err(&adapter->pdev->dev,
+ "Cannot change link characteristics when SoL/IDER is active.\n");
return -EINVAL;
}
@@ -1089,8 +1089,8 @@ static bool reg_pattern_test(struct igb_adapter *adapter, u64 *data,
wr32(reg, (_test[pat] & write));
val = rd32(reg) & mask;
if (val != (_test[pat] & write & mask)) {
- dev_err(&adapter->pdev->dev, "pattern test reg %04X "
- "failed: got 0x%08X expected 0x%08X\n",
+ dev_err(&adapter->pdev->dev,
+ "pattern test reg %04X failed: got 0x%08X expected 0x%08X\n",
reg, val, (_test[pat] & write & mask));
*data = reg;
return 1;
@@ -1108,8 +1108,8 @@ static bool reg_set_and_check(struct igb_adapter *adapter, u64 *data,
wr32(reg, write & mask);
val = rd32(reg);
if ((write & mask) != (val & mask)) {
- dev_err(&adapter->pdev->dev, "set/check reg %04X test failed:"
- " got 0x%08X expected 0x%08X\n", reg,
+ dev_err(&adapter->pdev->dev,
+ "set/check reg %04X test failed: got 0x%08X expected 0x%08X\n", reg,
(val & mask), (write & mask));
*data = reg;
return 1;
@@ -1171,8 +1171,9 @@ static int igb_reg_test(struct igb_adapter *adapter, u64 *data)
wr32(E1000_STATUS, toggle);
after = rd32(E1000_STATUS) & toggle;
if (value != after) {
- dev_err(&adapter->pdev->dev, "failed STATUS register test "
- "got: 0x%08X expected: 0x%08X\n", after, value);
+ dev_err(&adapter->pdev->dev,
+ "failed STATUS register test got: 0x%08X expected: 0x%08X\n",
+ after, value);
*data = 1;
return 1;
}
@@ -1777,16 +1778,14 @@ static int igb_loopback_test(struct igb_adapter *adapter, u64 *data)
* sessions are active */
if (igb_check_reset_block(&adapter->hw)) {
dev_err(&adapter->pdev->dev,
- "Cannot do PHY loopback test "
- "when SoL/IDER is active.\n");
+ "Cannot do PHY loopback test when SoL/IDER is active.\n");
*data = 0;
goto out;
}
if ((adapter->hw.mac.type == e1000_i210)
|| (adapter->hw.mac.type == e1000_i211)) {
dev_err(&adapter->pdev->dev,
- "Loopback test not supported "
- "on this part at this time.\n");
+ "Loopback test not supported on this part at this time.\n");
*data = 0;
goto out;
}
--
1.7.11.3
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* RE: [PATCH 1/4] pch_gbe: Fix the checksum fill to the error location
From: Andy Cress @ 2012-07-25 20:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Zhong Hongbo
In-Reply-To: <1342510387.2626.174.camel@edumazet-glaptop>
Eric,
For the checksum patch, that needs to be tabled until Hongbo has a chance to evaluate/revise it, but that isn't vital to the goal of avoiding transmit timeouts, so the other 3 patches should still go forward. I'll submit a revised patch set with just those 3.
I tried applying the dont-copy-payload patch you outlined below, and for some reason it won't send successfully with that change. I'm not sure why though. This approach seems sound, and should greatly help transmit performance, if I could figure out what else it needs.
Andy
-----Original Message-----
From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
Sent: Tuesday, July 17, 2012 3:33 AM
To: Andy Cress
Cc: netdev@vger.kernel.org; Zhong Hongbo
Subject: Re: [PATCH 1/4] pch_gbe: Fix the checksum fill to the error location
On Tue, 2012-07-17 at 09:09 +0200, Eric Dumazet wrote:
> Hmm... I fail to understand why you care about NIC doing checksums,
> while pch_gbe_tx_queue() make a _copy_ of each outgoing
> packets.
>
> There _must_ be a way to avoid most of these copies (ie not touching
> payload), only mess with the header to insert these 2 nul bytes ?
>
> /* [Header:14][payload] ---> [Header:14][paddong:2][payload] */
>
> So at device setup : dev->needed_headroom = 2;
>
> and in xmit,
>
> if (skb_headroom(skb) < 2) {
> struct sk_buff *skb_new;
>
> skb_new = skb_realloc_headroom(skb, 2);
> if (!skb_new) { handle error }
> consume_skb(skb);
> skb = skb_new;
> }
> ptr = skb_push(skb, 2);
> memmove(ptr, ptr + 2, ETH_HLEN);
> ptr[ETH_HLEN] = 0;
> ptr[ETH_HLEN + 1] = 0;
>
>
Something like the following (untested) patch
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c | 55 +++++-----
1 file changed, 29 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
index b100656..2d3d982 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
@@ -1163,7 +1163,7 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
struct pch_gbe_hw *hw = &adapter->hw;
struct pch_gbe_tx_desc *tx_desc;
struct pch_gbe_buffer *buffer_info;
- struct sk_buff *tmp_skb;
+ char *ptr;
unsigned int frame_ctrl;
unsigned int ring_num;
@@ -1221,18 +1221,27 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
buffer_info = &tx_ring->buffer_info[ring_num];
- tmp_skb = buffer_info->skb;
+ if (skb_headroom(skb) < 2) {
+ struct sk_buff *skb_new;
+
+ skb_new = skb_realloc_headroom(skb, 2);
+ if (!skb_new) {
+ tx_ring->next_to_use = ring_num;
+ dev_kfree_skb_any(skb);
+ return;
+ }
+ consume_skb(skb);
+ skb = skb_new;
+ }
/* [Header:14][payload] ---> [Header:14][paddong:2][payload] */
- memcpy(tmp_skb->data, skb->data, ETH_HLEN);
- tmp_skb->data[ETH_HLEN] = 0x00;
- tmp_skb->data[ETH_HLEN + 1] = 0x00;
- tmp_skb->len = skb->len;
- memcpy(&tmp_skb->data[ETH_HLEN + 2], &skb->data[ETH_HLEN],
- (skb->len - ETH_HLEN));
+ ptr = skb_push(skb, 2);
+ memmove(ptr, ptr + 2, ETH_HLEN);
+ ptr[ETH_HLEN] = 0x00;
+ ptr[ETH_HLEN + 1] = 0x00;
/*-- Set Buffer information --*/
- buffer_info->length = tmp_skb->len;
- buffer_info->dma = dma_map_single(&adapter->pdev->dev, tmp_skb->data,
+ buffer_info->length = skb->len;
+ buffer_info->dma = dma_map_single(&adapter->pdev->dev, skb->data,
buffer_info->length,
DMA_TO_DEVICE);
if (dma_mapping_error(&adapter->pdev->dev, buffer_info->dma)) {
@@ -1240,18 +1249,20 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
buffer_info->dma = 0;
buffer_info->time_stamp = 0;
tx_ring->next_to_use = ring_num;
+ dev_kfree_skb_any(skb);
return;
}
buffer_info->mapped = true;
buffer_info->time_stamp = jiffies;
+ buffer_info->skb = skb;
/*-- Set Tx descriptor --*/
tx_desc = PCH_GBE_TX_DESC(*tx_ring, ring_num);
- tx_desc->buffer_addr = (buffer_info->dma);
- tx_desc->length = (tmp_skb->len);
- tx_desc->tx_words_eob = ((tmp_skb->len + 3));
+ tx_desc->buffer_addr = buffer_info->dma;
+ tx_desc->length = skb->len;
+ tx_desc->tx_words_eob = skb->len + 3;
tx_desc->tx_frame_ctrl = (frame_ctrl);
- tx_desc->gbec_status = (DSC_INIT16);
+ tx_desc->gbec_status = DSC_INIT16;
if (unlikely(++ring_num == tx_ring->count))
ring_num = 0;
@@ -1265,7 +1276,6 @@ static void pch_gbe_tx_queue(struct pch_gbe_adapter *adapter,
pch_tx_timestamp(adapter, skb);
#endif
- dev_kfree_skb_any(skb);
}
/**
@@ -1543,19 +1553,12 @@ static void pch_gbe_alloc_tx_buffers(struct pch_gbe_adapter *adapter,
struct pch_gbe_tx_ring *tx_ring)
{
struct pch_gbe_buffer *buffer_info;
- struct sk_buff *skb;
unsigned int i;
- unsigned int bufsz;
struct pch_gbe_tx_desc *tx_desc;
- bufsz =
- adapter->hw.mac.max_frame_size + PCH_GBE_DMA_ALIGN + NET_IP_ALIGN;
-
for (i = 0; i < tx_ring->count; i++) {
buffer_info = &tx_ring->buffer_info[i];
- skb = netdev_alloc_skb(adapter->netdev, bufsz);
- skb_reserve(skb, PCH_GBE_DMA_ALIGN);
- buffer_info->skb = skb;
+ buffer_info->skb = NULL;
tx_desc = PCH_GBE_TX_DESC(*tx_ring, i);
tx_desc->gbec_status = (DSC_INIT16);
}
@@ -1622,9 +1625,9 @@ pch_gbe_clean_tx(struct pch_gbe_adapter *adapter,
buffer_info->length, DMA_TO_DEVICE);
buffer_info->mapped = false;
}
- if (buffer_info->skb) {
- pr_debug("trim buffer_info->skb : %d\n", i);
- skb_trim(buffer_info->skb, 0);
+ if (skb) {
+ dev_kfree_skb_any(skb);
+ buffer_info->skb = NULL;
}
tx_desc->gbec_status = DSC_INIT16;
if (unlikely(++i == tx_ring->count))
^ permalink raw reply related
* [RFC NET-NEXT PATCH 0/6] implement MDI-X set support in Intel drivers
From: Jesse Brandeburg @ 2012-07-25 17:59 UTC (permalink / raw)
To: netdev; +Cc: bhutchings, jesse.brandeburg
This is an update (v2) to the patches sent previously.
This series implements MDI-X set support in the Intel gigabit ethernet
drivers. It specifically allows a user to control the MDI state,
in order for them to work around issues with broken switches.
The only change to this series is updated descriptions and an
addition of reporting the current mdix_ctrl value in the
"get_settings" functions, which allows the new extended reporting
in the ethtool app's MDI-X: line
sent as RFC for now to get feedback while internal testing
completes. (and net-next is closed)
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
Bruce W Allan (1):
e1000e: implement 82577/579 MDI setting support
Jesse Brandeburg (5):
igb: update to allow reading/setting MDI state
e1000e: implement MDI/MDI-X control
e1000: configure and read MDI settings
igb: implement 580 MDI setting support
ethtool.h: MDI setting support
drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 39 ++++++++++++++++++++
drivers/net/ethernet/intel/e1000/e1000_main.c | 4 ++
drivers/net/ethernet/intel/e1000e/ethtool.c | 41 ++++++++++++++++++++-
drivers/net/ethernet/intel/e1000e/phy.c | 31 +++++++++++++++-
drivers/net/ethernet/intel/igb/e1000_phy.c | 29 ++++++++++++++-
drivers/net/ethernet/intel/igb/e1000_phy.h | 5 ++-
drivers/net/ethernet/intel/igb/igb_ethtool.c | 42 ++++++++++++++++++++++
drivers/net/ethernet/intel/igb/igb_main.c | 4 ++
include/linux/ethtool.h | 17 ++++++---
9 files changed, 198 insertions(+), 14 deletions(-)
--
Jesse Brandeburg
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox