* Re: [PATCH 2/8] netback: add module unload function
From: Wei Liu @ 2013-03-05 13:30 UTC (permalink / raw)
To: Stephen Hemminger
Cc: wei.liu2, xen-devel@lists.xen.org, netdev@vger.kernel.org,
Ian Campbell, konrad.wilk@oracle.com, annie.li@oracle.com
In-Reply-To: <20130304135831.4d52eddc@nehalam.linuxnetplumber.net>
On Mon, 2013-03-04 at 21:58 +0000, Stephen Hemminger wrote:
> On Fri, 15 Feb 2013 16:00:03 +0000
> Wei Liu <wei.liu2@citrix.com> wrote:
>
> > Enable users to unload netback module. Users should make sure there is not vif
> > runnig.
>
>
> Isn't it likely that some admin might be trying to cleanup or do shutdown and there
> is an ordering problem. What happens if vif's are still running?
>
> Why depend on users? You should allow it anytime and do refcounting and auto-destroy the vif's for them.
Sure, we should not depend on users. I will move my vif get/put module
patch before this one.
Wei.
^ permalink raw reply
* Re: [PATCH 0/7] USB: don't recover device if suspend fails in system sleep
From: Oliver Neukum @ 2013-03-05 13:28 UTC (permalink / raw)
To: Ming Lei
Cc: Bjørn Mork, David S. Miller, Greg Kroah-Hartman, Jiri Kosina,
Alan Stern, netdev, linux-usb, linux-input
In-Reply-To: <CACVXFVMkztqc_K+8gvPJhU-rC+c_STDyZLDmJZ2p1hLnADDXvg@mail.gmail.com>
On Tuesday 05 March 2013 21:08:09 Ming Lei wrote:
> On Tue, Mar 5, 2013 at 8:50 PM, Oliver Neukum <oneukum@suse.de> wrote:
> > In other words, if we don't handle errors, there must be no errors,
> > otherwise it doesn't matter what we do in the error case. We'd leave
> > the problem to generic layers.
>
> Generic layers can't handle the driver's specific failure.
We depend on stopping the HC forcing all devices into suspend.
We know this is problematic. For example some disk enclosures
need to flush cache. Fortunately for us this is done in the SCSI
layer.
> If driver records its suspend failure state in suspend(), resume()
> should and can deal with it without much difficulty.
Yes, but why bother? Either we can safely suspend in any state or
we must not ignore errors.
> > Furthermore there is a small chance that although the device tree
> > is walked, teh system suspend fails for another later reason that
> > is not ignored. In that case the drivers need to do error recovery,
> > albeit in resume().
>
> Yes, resume() need to handle the USB system suspend failure
> either in normal resume or error recovery, both are basically same.
In theory yes, in practice usually power is cut.
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 0/7] USB: don't recover device if suspend fails in system sleep
From: Bjørn Mork @ 2013-03-05 13:18 UTC (permalink / raw)
To: Ming Lei
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <CACVXFVM=PcxTLb1-Bo-11Lz0FzthQJTRKt5=sn6W85K9p8xE4A@mail.gmail.com>
Ming Lei <ming.lei@canonical.com> writes:
> On Tue, Mar 5, 2013 at 3:03 PM, Bjørn Mork <bjorn@mork.no> wrote:
>> Ming Lei <ming.lei@canonical.com> writes:
>>
>>> Hi,
>>>
>>> This patch adds comments on interface driver suspend callback
>>> to emphasize that the failure return value is ignored by
>>> USB core in system sleep context, so do not try to recover
>>> device for this case, otherwise the URB traffic scheduled
>>> in recovery of failure path may cross system sleep, and may
>>> cause problems.
>>
>> Well, an unexpected error did happen so problems are to be expected,
>> yes.
>>
>>> Also fixes the USB serial, HID and several usbnet drivers
>>> which may recover device in suspend failure path of system sleep.
>>
>> I believe all of these are wrong unless you have any real bug which is
>> fixed by this.
>
> It is really a bug if one driver submits URBs and keeps them scheduled
> on bus before system sleep, because the bus transaction can't be kept
> across system sleep cycle.
1. You do not fix that. You just ignore that the driver failed to
cancel *one* of its URBs and return to the caller in an unknown
state. That's not even papering over the bug, it is just adding a
new one without solving anything
2. I asked for a *real* bug, as opposed to theoretical bug. Working
around real issues by papering over them may sometimes be acceptable
if the real issue is bad enough. Working around theoretical bugs this
way never is.
Yes, I can also see the theoretical issue. Feel free to fix that if you
know how. I don't.
>> All these drivers suspend in multiple steps, where each step can
>> fail. If a later step fails then they revert any previously successful
>> step before returning the failure, thereby ensuring that the
>> device/driver state when suspend returns is consistently either
>> suspended or resumed.
>
> IMO, for autosuspend, that is right, but it is not for system suspend,
> and the driver's suspend callback can't return in resumed state
OK. You do realize that the code you are changing tries to deal with
the case where suspend already failed, so returning in suspended state
is impossible? If you cannot return in resumed state, then you have two
options:
a) define a new state
b) don't return
You seem to want to do a). But that is far more work than just ignoring
partial errors. You need to add the state, make the caller deal with
it, make resume deal with it etc.
Personally I believe that alternative
c) revert to resumed state and return error
like these drivers do is a better option.
> because the USB core will ignore the failure return value and force
> to suspend the device.
Yes, I know that. So fix that then if it is an issue. My claim is that
it is not, and that is why the USB core can ignore the failure. If it
were an issue then the core could not ignore it. Simple as that.
You cannot keep pushing this down to "Do not allow operation to ever
fail" for every small task involved in a suspend. It involves
hardware. It can and will fail.
>> I am going to NAK the cdc_mbim and qmi_wwan pacthes unless you can
>> convince me that we need to add a "partly-suspended" state for the
>> system suspend error case. In which case the patch will need to include
>> the corresponding resume fix for the "partly-suspended" state.
>>
>
> Yes, you may argue that the device might be in partly-suspended state,
> but it doesn't matter since the device will be put into suspend later and
Where will it do that? AFAICS, usb_suspend_both() will resume all
already suspended interfaces if a driver fails suspend. From the
function docs:
* This is the central routine for suspending USB devices. It calls the
* suspend methods for all the interface drivers in @udev and then calls
* the suspend method for @udev itself. If an error occurs at any stage,
* all the interfaces which were suspended are resumed so that they remain
* in the same state as the device.
You do want the *failing* interface driver/function to have the same
state as the rest of the device functions, i.e. resumed, on return from
suspend.
> the resume callback can recover from the partly-suspend state.
It can. But it won't magically just do that by ignoring errors in
suspend.
> These patches doesn't break previous failure path of system suspend
> and just avoid to submit new URBs, so how about letting later delta
> patches fix for the corresponding resume?
From Documentation/usb/power-management.txt :
The suspend method is called to warn the driver that the
device is going to be suspended. If the driver returns a
negative error code, the suspend will be aborted. Normally
the driver will return 0, in which case it must cancel all
outstanding URBs (usb_kill_urb()) and not submit any more.
The driver need only cancel outstanding URBs if it returns 0. How the
USB core handles this is up to the USB core. It is not for the
drivers to deal with. Unless you modify the API to say that suspend
cannot fail in the !PMSG_IS_AUTO(message) case.
But I really think all that ugly PMSG_IS_AUTO(message) testing in the
USB drivers is contradicting the simple design of the USB core, using a
single common suspend for both runtime and system suspend and leaving it
up to the USB core to handle the differences.
AFAICS, it does that just fine. If you think it does not, then please
fix that in the USB core or redefine the API. But don't just randomly
make drivers ignore errors.
Bjørn
^ permalink raw reply
* Re: [PATCH 1/7] USB: adds comment on suspend callback
From: Ming Lei @ 2013-03-05 13:16 UTC (permalink / raw)
To: David S. Miller, Greg Kroah-Hartman, Jiri Kosina
Cc: Alan Stern, Oliver Neukum, netdev, linux-usb, linux-input,
Ming Lei
In-Reply-To: <1362456103-24956-2-git-send-email-ming.lei@canonical.com>
On Tue, Mar 5, 2013 at 12:01 PM, Ming Lei <ming.lei@canonical.com> wrote:
> This patch adds comments on interface driver suspend callback
> to emphasize that the failure return value is ignored by
> USB core in system sleep context, so do not try to recover
> device for this case.
>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
> include/linux/usb.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 4d22d0f..ea9d7cb 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -978,7 +978,10 @@ struct usbdrv_wrap {
> * the "usbfs" filesystem. This lets devices provide ways to
> * expose information to user space regardless of where they
> * do (or don't) show up otherwise in the filesystem.
> - * @suspend: Called when the device is going to be suspended by the system.
> + * @suspend: Called when the device is going to be suspended by the
> + * system either from system sleep or runtime suspend context, and
> + * its failed return value will be ignored in system sleep context,
> + * so do NOT try to recover device for this case.
> * @resume: Called when the device is being resumed by the system.
> * @reset_resume: Called when the suspended device has been reset instead
> * of being resumed.
On the other hand, resume()/reset_resume() should deal with the previous
suspend failure if the policy to ignore suspend failure is kept.
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [PATCH 0/7] USB: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 13:08 UTC (permalink / raw)
To: Oliver Neukum
Cc: Bjørn Mork, David S. Miller, Greg Kroah-Hartman, Jiri Kosina,
Alan Stern, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3703451.5FViJ58GpZ-7ztolUikljGernLeA6q8OA@public.gmane.org>
On Tue, Mar 5, 2013 at 8:50 PM, Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org> wrote:
>>
>> IMO, for autosuspend, that is right, but it is not for system suspend,
>> and the driver's suspend callback can't return in resumed state
>> because the USB core will ignore the failure return value and force
>> to suspend the device.
>
> It seems to me that in this case you just need to make sure that
> suspend() not fail for system suspend. Or revisit the decision to
> ignore failures.
IMO, the current policy is correct.
> In other words, if we don't handle errors, there must be no errors,
> otherwise it doesn't matter what we do in the error case. We'd leave
> the problem to generic layers.
Generic layers can't handle the driver's specific failure.
If driver records its suspend failure state in suspend(), resume()
should and can deal with it without much difficulty.
>
> Furthermore there is a small chance that although the device tree
> is walked, teh system suspend fails for another later reason that
> is not ignored. In that case the drivers need to do error recovery,
> albeit in resume().
Yes, resume() need to handle the USB system suspend failure
either in normal resume or error recovery, both are basically same.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/7] USB: don't recover device if suspend fails in system sleep
From: Oliver Neukum @ 2013-03-05 12:50 UTC (permalink / raw)
To: Ming Lei
Cc: Bjørn Mork, David S. Miller, Greg Kroah-Hartman, Jiri Kosina,
Alan Stern, netdev, linux-usb, linux-input
In-Reply-To: <CACVXFVM=PcxTLb1-Bo-11Lz0FzthQJTRKt5=sn6W85K9p8xE4A@mail.gmail.com>
On Tuesday 05 March 2013 18:55:42 Ming Lei wrote:
> > All these drivers suspend in multiple steps, where each step can
> > fail. If a later step fails then they revert any previously successful
> > step before returning the failure, thereby ensuring that the
> > device/driver state when suspend returns is consistently either
> > suspended or resumed.
>
> IMO, for autosuspend, that is right, but it is not for system suspend,
> and the driver's suspend callback can't return in resumed state
> because the USB core will ignore the failure return value and force
> to suspend the device.
It seems to me that in this case you just need to make sure that
suspend() not fail for system suspend. Or revisit the decision to
ignore failures.
In other words, if we don't handle errors, there must be no errors,
otherwise it doesn't matter what we do in the error case. We'd leave
the problem to generic layers.
Furthermore there is a small chance that although the device tree
is walked, teh system suspend fails for another later reason that
is not ignored. In that case the drivers need to do error recovery,
albeit in resume().
Regards
Oliver
^ permalink raw reply
* tipc: MTU discovery
From: Sebastian Pöhn @ 2013-03-05 12:43 UTC (permalink / raw)
To: netdev; +Cc: jon.maloy, allan.stephens
I am trying to run TIPC over a link with a MTU higher than standard
1500 (actually 2k or more).
I have a Ethernet Device configured with this MTU and an overlying
VLAN with the same MTU. Link Layer looks ok, as MTU sized ICMP traffic
is sent and received unfragmented.
TIPC does not discover the higher MTU, it always keeps at 1500.
Smaller ones (1.4k) are detected correctly.
Doing some deeper investigation into the TIPC stack leaded to this observation:
State Messages larger than 1500 which as used for the MTU negotiation
do not appear in the TIPC stack. But I am able to seen them entering
the correct device.
Because of that no reply with the correct max_packet field set can be
send and the negotiation will always end up at 1.5k.
So my questions are:
# Is TIPC meant to detect and use the MTU larger than 1.5k?
# Why are the packets not passed to the TIPC stack?
^ permalink raw reply
* Re: [PATCH 7/7] usbnet: qmi_wwan: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 12:27 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <877glmqq4w.fsf@nemi.mork.no>
On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> If suspend callback fails in system sleep context, usb core will
>> ignore the failure and let system sleep go ahead further, so
>> this patch doesn't recover device under this situation.
>>
>> Cc: Bjørn Mork <bjorn@mork.no>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>> drivers/net/usb/qmi_wwan.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
>> index efb5c7c..fce18be 100644
>> --- a/drivers/net/usb/qmi_wwan.c
>> +++ b/drivers/net/usb/qmi_wwan.c
>> @@ -294,7 +294,7 @@ static int qmi_wwan_suspend(struct usb_interface *intf, pm_message_t message)
>>
>> if (intf == info->control && info->subdriver && info->subdriver->suspend)
>> ret = info->subdriver->suspend(intf, message);
>> - if (ret < 0)
>> + if (ret < 0 && PMSG_IS_AUTO(msg))
>> usbnet_resume(intf);
>> err:
>> return ret;
>
>
> NAK. We if the device cannot suspend, then it cannot do suspend halfways
> either. Whether the caller will ignore the error or not, is irrelevant.
See my same comments on 4/7.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 1/1] net: ipv4: Remove redundant NULL check
From: Sachin Kamat @ 2013-03-05 11:27 UTC (permalink / raw)
To: netdev; +Cc: davem, sachin.kamat
kfree on a null pointer is a NO-OP. Hence null check is not necessary.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
Compile tested on linux-next tree (20130305).
---
net/ipv4/tcp.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 47e854f..3aa5d54 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -992,10 +992,8 @@ static inline int select_size(const struct sock *sk, bool sg)
void tcp_free_fastopen_req(struct tcp_sock *tp)
{
- if (tp->fastopen_req != NULL) {
- kfree(tp->fastopen_req);
- tp->fastopen_req = NULL;
- }
+ kfree(tp->fastopen_req);
+ tp->fastopen_req = NULL;
}
static int tcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg, int *size)
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH 4/7] usbnet: cdc_mbim: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 11:07 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <87boayqq5c.fsf@nemi.mork.no>
On Tue, Mar 5, 2013 at 3:09 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> If suspend callback fails in system sleep context, usb core will
>> ignore the failure and let system sleep go ahead further, so
>> this patch doesn't recover device under this situation.
>>
>> Cc: Bjørn Mork <bjorn@mork.no>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>> drivers/net/usb/cdc_mbim.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
>> index 248d2dc..da83546 100644
>> --- a/drivers/net/usb/cdc_mbim.c
>> +++ b/drivers/net/usb/cdc_mbim.c
>> @@ -338,7 +338,7 @@ static int cdc_mbim_suspend(struct usb_interface *intf, pm_message_t message)
>>
>> if (intf == ctx->control && info->subdriver && info->subdriver->suspend)
>> ret = info->subdriver->suspend(intf, message);
>> - if (ret < 0)
>> + if (ret < 0 && PMSG_IS_AUTO(msg))
>> usbnet_resume(intf);
>>
>> error:
>
> NAK. We if the device cannot suspend, then it cannot do suspend halfways
Sorry, what do you mean that the device cannot suspend? If you mean
usb_suspend_device(), its failure is still ignored, and generally it is OK
since the suspend action is driven by upstream port.
> either. Whether the caller will ignore the error or not, is irrelevant.
Anyway, usbnet_resume() can't be called to submit new URBs in
the failure path of system suspend, so the patch should be correct.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 0/7] USB: don't recover device if suspend fails in system sleep
From: Ming Lei @ 2013-03-05 10:55 UTC (permalink / raw)
To: Bjørn Mork
Cc: David S. Miller, Greg Kroah-Hartman, Jiri Kosina, Alan Stern,
Oliver Neukum, netdev, linux-usb, linux-input
In-Reply-To: <87fw0aqqel.fsf@nemi.mork.no>
On Tue, Mar 5, 2013 at 3:03 PM, Bjørn Mork <bjorn@mork.no> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> Hi,
>>
>> This patch adds comments on interface driver suspend callback
>> to emphasize that the failure return value is ignored by
>> USB core in system sleep context, so do not try to recover
>> device for this case, otherwise the URB traffic scheduled
>> in recovery of failure path may cross system sleep, and may
>> cause problems.
>
> Well, an unexpected error did happen so problems are to be expected,
> yes.
>
>> Also fixes the USB serial, HID and several usbnet drivers
>> which may recover device in suspend failure path of system sleep.
>
> I believe all of these are wrong unless you have any real bug which is
> fixed by this.
It is really a bug if one driver submits URBs and keeps them scheduled
on bus before system sleep, because the bus transaction can't be kept
across system sleep cycle.
>
> All these drivers suspend in multiple steps, where each step can
> fail. If a later step fails then they revert any previously successful
> step before returning the failure, thereby ensuring that the
> device/driver state when suspend returns is consistently either
> suspended or resumed.
IMO, for autosuspend, that is right, but it is not for system suspend,
and the driver's suspend callback can't return in resumed state
because the USB core will ignore the failure return value and force
to suspend the device.
>
> The error recovery they do in suspend is not about preventing suspend at
> all. It is about ensuring that that the driver and device is in a
> consistent state, which is "resumed" if suspend fails.
No, only putting the device as "suspended" in system suspend failure
can keep the state as consistent, see the above explanation.
>
> Your patch set make the drivers return from suspend in some intermediate
> state, where the device and/or driver is neither suspended nor resumed.
> This is wrong. You still did not necessarily kill all URBs, but you
> killed some of them. What is resume() going to do then?
These patches try to avoid submitting new URBs in the failure path of
system suspend, and we can comments these fixes in each patch one
by one.
>
> I am going to NAK the cdc_mbim and qmi_wwan pacthes unless you can
> convince me that we need to add a "partly-suspended" state for the
> system suspend error case. In which case the patch will need to include
> the corresponding resume fix for the "partly-suspended" state.
>
Yes, you may argue that the device might be in partly-suspended state,
but it doesn't matter since the device will be put into suspend later and
the resume callback can recover from the partly-suspend state.
These patches doesn't break previous failure path of system suspend
and just avoid to submit new URBs, so how about letting later delta
patches fix for the corresponding resume?
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [Xen-devel] [PATCH 5/8] netback: multi-page ring support
From: David Vrabel @ 2013-03-05 10:41 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, netdev, ian.campbell, konrad.wilk, annie.li
In-Reply-To: <1360944010-15336-6-git-send-email-wei.liu2@citrix.com>
On 15/02/13 16:00, Wei Liu wrote:
> [nothing]
You need to describe the protocol used to negotiate this. What happens
when a frontend without such support connects to a backend with support?
And vice-versa?
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -45,6 +45,12 @@
> #include <xen/grant_table.h>
> #include <xen/xenbus.h>
>
> +#define NETBK_MAX_RING_PAGE_ORDER XENBUS_MAX_RING_PAGE_ORDER
> +#define NETBK_MAX_RING_PAGES (1U << NETBK_MAX_RING_PAGE_ORDER)
> +
> +#define NETBK_MAX_TX_RING_SIZE XEN_NETIF_TX_RING_SIZE(NETBK_MAX_RING_PAGES)
> +#define NETBK_MAX_RX_RING_SIZE XEN_NETIF_RX_RING_SIZE(NETBK_MAX_RING_PAGES)
See comment below.
> @@ -105,15 +113,19 @@ static inline struct xenbus_device *xenvif_to_xenbus_device(struct xenvif *vif)
> return to_xenbus_device(vif->dev->dev.parent);
> }
>
> -#define XEN_NETIF_TX_RING_SIZE __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE)
> -#define XEN_NETIF_RX_RING_SIZE __CONST_RING_SIZE(xen_netif_rx, PAGE_SIZE)
> +#define XEN_NETIF_TX_RING_SIZE(_nr_pages) \
> + __CONST_RING_SIZE(xen_netif_tx, PAGE_SIZE * (_nr_pages))
Ring size is no longer const.
Would these be better as inline functions with a struct xenvif parameter?
Would need to fixup the MAX_XX_RING_SIZE macros above.
> index db638e1..fa4d46d 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -305,10 +305,16 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
> return vif;
> }
>
> -int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
> - unsigned long rx_ring_ref, unsigned int evtchn)
> +int xenvif_connect(struct xenvif *vif,
> + unsigned long *tx_ring_ref, unsigned int tx_ring_ref_count,
> + unsigned long *rx_ring_ref, unsigned int rx_ring_ref_count,
> + unsigned int evtchn)
> {
> int err = -ENOMEM;
> + void *addr;
> + struct xen_netif_tx_sring *txs;
> + struct xen_netif_rx_sring *rxs;
> + int tmp[NETBK_MAX_RING_PAGES], i;
grant_ref_t, and elsewhere probably -- I didn't check.
> @@ -382,7 +413,8 @@ void xenvif_disconnect(struct xenvif *vif)
>
> unregister_netdev(vif->dev);
>
> - xen_netbk_unmap_frontend_rings(vif);
> + xen_netbk_unmap_frontend_rings(vif, (void *)vif->tx.sring);
> + xen_netbk_unmap_frontend_rings(vif, (void *)vif->rx.sring);
Don't need the casts here.
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -47,6 +47,19 @@
> #include <asm/xen/hypercall.h>
> #include <asm/xen/page.h>
>
> +unsigned int MODPARM_netback_max_rx_ring_page_order = NETBK_MAX_RING_PAGE_ORDER;
> +module_param_named(netback_max_rx_ring_page_order,
> + MODPARM_netback_max_rx_ring_page_order, uint, 0);
Please don't prefix new module parameters with "netback",
"max_rx_ring_page_order" is fine.
> +MODULE_PARM_DESC(netback_max_rx_ring_page_order,
> + "Maximum supported receiver ring page order");
> +
> +unsigned int MODPARM_netback_max_tx_ring_page_order = NETBK_MAX_RING_PAGE_ORDER;
> +module_param_named(netback_max_tx_ring_page_order,
> + MODPARM_netback_max_tx_ring_page_order, uint, 0);
Ditto.
> +MODULE_PARM_DESC(netback_max_tx_ring_page_order,
> + "Maximum supported transmitter ring page order");
> +
> +
[...]
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
[...]
> + err = xenbus_scanf(XBT_NIL, dev->otherend, "tx-ring-order", "%u",
> + &tx_ring_order);
> + if (err < 0) {
> + tx_ring_order = 0;
> +
> + err = xenbus_scanf(XBT_NIL, dev->otherend, "tx-ring-ref", "%lu",
> + &tx_ring_ref[0]);
> + if (err < 0) {
> + xenbus_dev_fatal(dev, err, "reading %s/tx-ring-ref",
> + dev->otherend);
> + return err;
> + }
> + } else {
> + unsigned int i;
> +
> + if (tx_ring_order > MODPARM_netback_max_tx_ring_page_order) {
> + err = -EINVAL;
> + xenbus_dev_fatal(dev, err,
> + "%s/tx-ring-page-order too big",
> + dev->otherend);
> + return err;
> + }
> +
> + for (i = 0; i < (1U << tx_ring_order); i++) {
> + char ring_ref_name[sizeof("tx-ring-ref") + 2];
> +
> + snprintf(ring_ref_name, sizeof(ring_ref_name),
> + "tx-ring-ref%u", i);
> +
> + err = xenbus_scanf(XBT_NIL, dev->otherend,
> + ring_ref_name, "%lu",
> + &tx_ring_ref[i]);
> + if (err < 0) {
> + xenbus_dev_fatal(dev, err,
> + "reading %s/%s",
> + dev->otherend,
> + ring_ref_name);
> + return err;
> + }
> + }
Refactor this whole if/else block and the similar code below for rx into
a common library function?
It will be useful for blkback etc. as well.
> @@ -454,11 +566,28 @@ static int connect_rings(struct backend_info *be)
> vif->csum = !val;
>
> /* Map the shared frame, irq etc. */
> - err = xenvif_connect(vif, tx_ring_ref, rx_ring_ref, evtchn);
> + err = xenvif_connect(vif, tx_ring_ref, (1U << tx_ring_order),
> + rx_ring_ref, (1U << rx_ring_order),
> + evtchn);
> if (err) {
> + /* construct 1 2 3 / 4 5 6 */
This comment doesn't make sense to me.
David
^ permalink raw reply
* Re: [Xen-devel] [PATCH 4/8] xenbus_client: Extend interface to support multi-page ring
From: David Vrabel @ 2013-03-05 10:25 UTC (permalink / raw)
To: Wei Liu
Cc: xen-devel, netdev, ian.campbell, konrad.wilk, annie.li,
Stefano Stabellini, Roger Pau Monne
In-Reply-To: <1360944010-15336-5-git-send-email-wei.liu2@citrix.com>
On 15/02/13 16:00, Wei Liu wrote:
> Also bundle fixes for xen frontends and backends in this patch.
When changing APIs you don't need to call out required changes to
callers as "fixes".
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -357,17 +359,39 @@ static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
> /**
> * xenbus_grant_ring
> * @dev: xenbus device
> - * @ring_mfn: mfn of ring to grant
> -
> - * Grant access to the given @ring_mfn to the peer of the given device. Return
> - * 0 on success, or -errno on error. On error, the device will switch to
> + * @vaddr: starting virtual address of the ring
> + * @nr_pages: number of pages to be granted
> + * @grefs: grant reference array to be filled in
> + *
> + * Grant access to the given @vaddr to the peer of the given device.
> + * Then fill in @grefs with grant references. Return 0 on success, or
> + * -errno on error. On error, the device will switch to
> * XenbusStateClosing, and the error will be saved in the store.
> */
> -int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn)
> +int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> + int nr_pages, int *grefs)
This call previously return the grant ref in an int, but grant refs are
really of (unsigned) type grant_ref_t. Make grefs an array of
grant_ref_t's.
> static int xenbus_map_ring_valloc_pv(struct xenbus_device *dev,
> - int gnt_ref, void **vaddr)
> + int *gnt_ref, int nr_grefs, void **vaddr)
[...]
> + /* Issue hypercall for individual entry, rollback if error occurs. */
> + for (i = 0; i < nr_grefs; i++) {
> + op.flags = GNTMAP_host_map | GNTMAP_contains_pte;
> + op.ref = gnt_ref[i];
> + op.dom = dev->otherend_id;
> + op.host_addr = arbitrary_virt_to_machine(pte[i]).maddr;
> +
> + if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
> + BUG();
I think these hypercalls should be batches into one
GNTTABOP_map_grant_ref call. I think this will make your 'rollback'
easier as well.
Similarly, for all the other places you have map/unmap in a loop.
> +int xenbus_map_ring(struct xenbus_device *dev, int *gnt_ref, int nr_grefs,
> + grant_handle_t *handle, void *vaddr, int *vma_leaked)
grant_ref_t for the array.
> @@ -195,15 +200,17 @@ int xenbus_watch_pathfmt(struct xenbus_device *dev, struct xenbus_watch *watch,
> const char *pathfmt, ...);
>
> int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state new_state);
> -int xenbus_grant_ring(struct xenbus_device *dev, unsigned long ring_mfn);
> +int xenbus_grant_ring(struct xenbus_device *dev, void *vaddr,
> + int nr_gages, int *grefs);
nr_pages?
David
^ permalink raw reply
* Fwd: FW: Request guidance for supporting GMII to RGMII conversion in Linux MAC driver
From: Michal Simek @ 2013-03-05 10:15 UTC (permalink / raw)
To: netdev; +Cc: David Miller, eric.dumazet, Anirudha Sarangi
In-Reply-To: <f75b2440-5d94-4c5f-872d-8942f72d1a61@CO9EHSMHS023.ehs.local>
Hi,
just resent email to mailing list from Anirudha.
Hi,
I work for Xilinx and handle the Linux Ethernet controller drivers in Xilinx.
This email is regarding a concern that I have for one of our planned
HW implementation involving GMII to RGMII conversion.
Our HW team have developed a soft IP (will sit in the FPGA) which does
a GMII to RGMII conversion. The output from our Gig controller is GMII
data which is fed to this converter and the RGMII output from the
converter is fed to the PHY. The only concern here is that the
converter must know the link speed to do proper conversion. In a
typical case, the PHY autoneg happens between our PHY and the remote
PHY. At this point, the converter must know the negotiated speed.
In the existing implementation the converter has a register, to which
my Ethernet driver must write the negotiated speed. The Linux PHY
framework (drivers/net/phy) invokes the registered callback in my
driver and my driver is expected to write to this converter register.
We have the following options:
- The converter sits on the same MDIO bus on which the PHY operates.
In my device tree I have a node for my MAC. In this node, I have a
node for mdio. Currently in this mdio node, we have just one entry for
my PHY. Now there will be one more entry added for the converter which
will have a mdio address. The device probe finds out the converter.
Then every time the link status change callback in my driver is called
from the Linux PHY framework, I need to write to this converter
register through the mdio. I have some options here. First option is,
create a dummy PHY driver with only one relevant API that writes to
the register the speed/duplex settings (and no other APIs
implemented). Second is just do a mdio write with speed/duplex setting
without creating a separate dummy PHY driver. The advantage with the
1st option is that the same converter core can be used with other MACs
and is like a separate entity different from the MAC driver. It’s
functionally projected as a separate entity.
- The converter sits on a Axi_Lite interface not a MDIO bus. In the
device tree, it does not show up on a mdio bus. It shows up on the
Axi_Lite bus as a separate IP. Like previous case, we will either
directly write to the converter register over Axi_Lite or write a
small driver and provide read/write interface which the MAC driver can
use to write to the converter.
The MDIO implementation is simpler than Axi_Lite for this particular
implementation and uses lesser FPGA resource.
I just want to get opinion from the Linux maintainers as in future I
need to think of upstreaming the solution. I must know which of the
above will create lesser issues when I upstream.
Please advise me.
regards
Anirudha Sarangi
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
^ permalink raw reply
* Re: [Xen-devel] [PATCH 3/8] netback: get/put module along with vif connect/disconnect
From: David Vrabel @ 2013-03-05 10:02 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, netdev, ian.campbell, konrad.wilk, annie.li
In-Reply-To: <1360944010-15336-4-git-send-email-wei.liu2@citrix.com>
On 15/02/13 16:00, Wei Liu wrote:
> If there is vif running and user unloads netback, guest's network interface
> just mysteriously stops working. So we need to prevent unloading netback
> module if there is vif running.
It's not mysterious -- it is cleanly disconnected, and will reconnect
when the module is reinserted.
Being able to unload modules while they are in use is standard so I
don't think this should be applied.
David
^ permalink raw reply
* [net 3/4] igb: Fix link setup for I210 devices
From: Jeff Kirsher @ 2013-03-05 9:48 UTC (permalink / raw)
To: davem; +Cc: Carolyn Wyborny, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1362476917-5189-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Carolyn Wyborny <carolyn.wyborny@intel.com>
This patch changes the setup copper link function to use a switch
statement for the PHY id's available for the given PHY types. It
also adds a case for the I210 PHY id, so the appropriate setup link
function is called for it.
Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igb/e1000_82575.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 84e7e09..b64542a 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -1361,11 +1361,16 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
switch (hw->phy.type) {
case e1000_phy_i210:
case e1000_phy_m88:
- if (hw->phy.id == I347AT4_E_PHY_ID ||
- hw->phy.id == M88E1112_E_PHY_ID)
+ switch (hw->phy.id) {
+ case I347AT4_E_PHY_ID:
+ case M88E1112_E_PHY_ID:
+ case I210_I_PHY_ID:
ret_val = igb_copper_link_setup_m88_gen2(hw);
- else
+ break;
+ default:
ret_val = igb_copper_link_setup_m88(hw);
+ break;
+ }
break;
case e1000_phy_igp_3:
ret_val = igb_copper_link_setup_igp(hw);
--
1.7.11.7
^ permalink raw reply related
* [net 4/4] igb: Fix for lockdep issue in igb_get_i2c_client
From: Jeff Kirsher @ 2013-03-05 9:48 UTC (permalink / raw)
To: davem; +Cc: Carolyn Wyborny, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1362476917-5189-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Carolyn Wyborny <carolyn.wyborny@intel.com>
This patch fixes a lockdep warning in igb_get_i2c_client by
refactoring the initialization and usage of the i2c_client
completely. There is no on the fly allocation of the single
client needed today.
Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igb/igb.h | 2 +-
drivers/net/ethernet/intel/igb/igb_hwmon.c | 14 ++++++
drivers/net/ethernet/intel/igb/igb_main.c | 69 +-----------------------------
3 files changed, 17 insertions(+), 68 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index d27edbc..2515140 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -447,7 +447,7 @@ struct igb_adapter {
#endif
struct i2c_algo_bit_data i2c_algo;
struct i2c_adapter i2c_adap;
- struct igb_i2c_client_list *i2c_clients;
+ struct i2c_client *i2c_client;
};
#define IGB_FLAG_HAS_MSI (1 << 0)
diff --git a/drivers/net/ethernet/intel/igb/igb_hwmon.c b/drivers/net/ethernet/intel/igb/igb_hwmon.c
index 0a9b073..4623502 100644
--- a/drivers/net/ethernet/intel/igb/igb_hwmon.c
+++ b/drivers/net/ethernet/intel/igb/igb_hwmon.c
@@ -39,6 +39,10 @@
#include <linux/pci.h>
#ifdef CONFIG_IGB_HWMON
+struct i2c_board_info i350_sensor_info = {
+ I2C_BOARD_INFO("i350bb", (0Xf8 >> 1)),
+};
+
/* hwmon callback functions */
static ssize_t igb_hwmon_show_location(struct device *dev,
struct device_attribute *attr,
@@ -188,6 +192,7 @@ int igb_sysfs_init(struct igb_adapter *adapter)
unsigned int i;
int n_attrs;
int rc = 0;
+ struct i2c_client *client = NULL;
/* If this method isn't defined we don't support thermals */
if (adapter->hw.mac.ops.init_thermal_sensor_thresh == NULL)
@@ -198,6 +203,15 @@ int igb_sysfs_init(struct igb_adapter *adapter)
if (rc)
goto exit;
+ /* init i2c_client */
+ client = i2c_new_device(&adapter->i2c_adap, &i350_sensor_info);
+ if (client == NULL) {
+ dev_info(&adapter->pdev->dev,
+ "Failed to create new i2c device..\n");
+ goto exit;
+ }
+ adapter->i2c_client = client;
+
/* Allocation space for max attributes
* max num sensors * values (loc, temp, max, caution)
*/
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 4d53a17..4dbd629 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1923,10 +1923,6 @@ void igb_set_fw_version(struct igb_adapter *adapter)
return;
}
-static const struct i2c_board_info i350_sensor_info = {
- I2C_BOARD_INFO("i350bb", 0Xf8),
-};
-
/* igb_init_i2c - Init I2C interface
* @adapter: pointer to adapter structure
*
@@ -7717,67 +7713,6 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba)
}
}
-static DEFINE_SPINLOCK(i2c_clients_lock);
-
-/* igb_get_i2c_client - returns matching client
- * in adapters's client list.
- * @adapter: adapter struct
- * @dev_addr: device address of i2c needed.
- */
-static struct i2c_client *
-igb_get_i2c_client(struct igb_adapter *adapter, u8 dev_addr)
-{
- ulong flags;
- struct igb_i2c_client_list *client_list;
- struct i2c_client *client = NULL;
- struct i2c_board_info client_info = {
- I2C_BOARD_INFO("igb", 0x00),
- };
-
- spin_lock_irqsave(&i2c_clients_lock, flags);
- client_list = adapter->i2c_clients;
-
- /* See if we already have an i2c_client */
- while (client_list) {
- if (client_list->client->addr == (dev_addr >> 1)) {
- client = client_list->client;
- goto exit;
- } else {
- client_list = client_list->next;
- }
- }
-
- /* no client_list found, create a new one */
- client_list = kzalloc(sizeof(*client_list), GFP_ATOMIC);
- if (client_list == NULL)
- goto exit;
-
- /* dev_addr passed to us is left-shifted by 1 bit
- * i2c_new_device call expects it to be flush to the right.
- */
- client_info.addr = dev_addr >> 1;
- client_info.platform_data = adapter;
- client_list->client = i2c_new_device(&adapter->i2c_adap, &client_info);
- if (client_list->client == NULL) {
- dev_info(&adapter->pdev->dev,
- "Failed to create new i2c device..\n");
- goto err_no_client;
- }
-
- /* insert new client at head of list */
- client_list->next = adapter->i2c_clients;
- adapter->i2c_clients = client_list;
-
- client = client_list->client;
- goto exit;
-
-err_no_client:
- kfree(client_list);
-exit:
- spin_unlock_irqrestore(&i2c_clients_lock, flags);
- return client;
-}
-
/* igb_read_i2c_byte - Reads 8 bit word over I2C
* @hw: pointer to hardware structure
* @byte_offset: byte offset to read
@@ -7791,7 +7726,7 @@ s32 igb_read_i2c_byte(struct e1000_hw *hw, u8 byte_offset,
u8 dev_addr, u8 *data)
{
struct igb_adapter *adapter = container_of(hw, struct igb_adapter, hw);
- struct i2c_client *this_client = igb_get_i2c_client(adapter, dev_addr);
+ struct i2c_client *this_client = adapter->i2c_client;
s32 status;
u16 swfw_mask = 0;
@@ -7828,7 +7763,7 @@ s32 igb_write_i2c_byte(struct e1000_hw *hw, u8 byte_offset,
u8 dev_addr, u8 data)
{
struct igb_adapter *adapter = container_of(hw, struct igb_adapter, hw);
- struct i2c_client *this_client = igb_get_i2c_client(adapter, dev_addr);
+ struct i2c_client *this_client = adapter->i2c_client;
s32 status;
u16 swfw_mask = E1000_SWFW_PHY0_SM;
--
1.7.11.7
^ permalink raw reply related
* [net 2/4] igb: Drop BUILD_BUG_ON check from igb_build_rx_buffer
From: Jeff Kirsher @ 2013-03-05 9:48 UTC (permalink / raw)
To: davem; +Cc: Alexander Duyck, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1362476917-5189-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Alexander Duyck <alexander.h.duyck@intel.com>
On s390 the igb driver was throwing a build error due to the fact that a frame
built using build_skb would be larger than 2K. Since this is not likely to
change at any point in the future we are better off just dropping the check
since we already had a check in igb_set_rx_buffer_len that will just disable
the usage of build_skb anyway.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ed79a1c..4d53a17 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6227,13 +6227,6 @@ static struct sk_buff *igb_build_rx_buffer(struct igb_ring *rx_ring,
/* If we spanned a buffer we have a huge mess so test for it */
BUG_ON(unlikely(!igb_test_staterr(rx_desc, E1000_RXD_STAT_EOP)));
- /* Guarantee this function can be used by verifying buffer sizes */
- BUILD_BUG_ON(SKB_WITH_OVERHEAD(IGB_RX_BUFSZ) < (NET_SKB_PAD +
- NET_IP_ALIGN +
- IGB_TS_HDR_LEN +
- ETH_FRAME_LEN +
- ETH_FCS_LEN));
-
rx_buffer = &rx_ring->rx_buffer_info[rx_ring->next_to_clean];
page = rx_buffer->page;
prefetchw(page);
--
1.7.11.7
^ permalink raw reply related
* [net 1/4] e1000e: workaround DMA unit hang on I218
From: Jeff Kirsher @ 2013-03-05 9:48 UTC (permalink / raw)
To: davem; +Cc: Bruce Allan, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1362476917-5189-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Bruce Allan <bruce.w.allan@intel.com>
At 1000Mbps link speed, one of the MAC's internal clocks can be stopped for
up to 4us when entering K1 (a power mode of the MAC-PHY interconnect). If
the MAC is waiting for completion indications for 2 DMA write requests into
Host memory (e.g. descriptor writeback or Rx packet writing) and the
indications occur while the clock is stopped, both indications will be
missed by the MAC causing the MAC to wait for the completion indications
and be unable to generate further DMA write requests. This results in an
apparent hardware hang.
Work-around the issue by disabling the de-assertion of the clock request
when 1000Mbps link is acquired (K1 must be disabled while doing this).
Signed-off-by: Bruce Allan <bruce.w.allan@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/e1000e/ich8lan.c | 71 ++++++++++++++++++++++++++++-
drivers/net/ethernet/intel/e1000e/ich8lan.h | 2 +
drivers/net/ethernet/intel/e1000e/regs.h | 1 +
3 files changed, 73 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index dff7bff..121a865 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -782,6 +782,59 @@ release:
}
/**
+ * e1000_k1_workaround_lpt_lp - K1 workaround on Lynxpoint-LP
+ * @hw: pointer to the HW structure
+ * @link: link up bool flag
+ *
+ * When K1 is enabled for 1Gbps, the MAC can miss 2 DMA completion indications
+ * preventing further DMA write requests. Workaround the issue by disabling
+ * the de-assertion of the clock request when in 1Gpbs mode.
+ **/
+static s32 e1000_k1_workaround_lpt_lp(struct e1000_hw *hw, bool link)
+{
+ u32 fextnvm6 = er32(FEXTNVM6);
+ s32 ret_val = 0;
+
+ if (link && (er32(STATUS) & E1000_STATUS_SPEED_1000)) {
+ u16 kmrn_reg;
+
+ ret_val = hw->phy.ops.acquire(hw);
+ if (ret_val)
+ return ret_val;
+
+ ret_val =
+ e1000e_read_kmrn_reg_locked(hw, E1000_KMRNCTRLSTA_K1_CONFIG,
+ &kmrn_reg);
+ if (ret_val)
+ goto release;
+
+ ret_val =
+ e1000e_write_kmrn_reg_locked(hw,
+ E1000_KMRNCTRLSTA_K1_CONFIG,
+ kmrn_reg &
+ ~E1000_KMRNCTRLSTA_K1_ENABLE);
+ if (ret_val)
+ goto release;
+
+ usleep_range(10, 20);
+
+ ew32(FEXTNVM6, fextnvm6 | E1000_FEXTNVM6_REQ_PLL_CLK);
+
+ ret_val =
+ e1000e_write_kmrn_reg_locked(hw,
+ E1000_KMRNCTRLSTA_K1_CONFIG,
+ kmrn_reg);
+release:
+ hw->phy.ops.release(hw);
+ } else {
+ /* clear FEXTNVM6 bit 8 on link down or 10/100 */
+ ew32(FEXTNVM6, fextnvm6 & ~E1000_FEXTNVM6_REQ_PLL_CLK);
+ }
+
+ return ret_val;
+}
+
+/**
* e1000_check_for_copper_link_ich8lan - Check for link (Copper)
* @hw: pointer to the HW structure
*
@@ -818,6 +871,14 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw)
return ret_val;
}
+ /* Work-around I218 hang issue */
+ if ((hw->adapter->pdev->device == E1000_DEV_ID_PCH_LPTLP_I218_LM) ||
+ (hw->adapter->pdev->device == E1000_DEV_ID_PCH_LPTLP_I218_V)) {
+ ret_val = e1000_k1_workaround_lpt_lp(hw, link);
+ if (ret_val)
+ return ret_val;
+ }
+
/* Clear link partner's EEE ability */
hw->dev_spec.ich8lan.eee_lp_ability = 0;
@@ -3954,8 +4015,16 @@ void e1000_suspend_workarounds_ich8lan(struct e1000_hw *hw)
phy_ctrl = er32(PHY_CTRL);
phy_ctrl |= E1000_PHY_CTRL_GBE_DISABLE;
+
if (hw->phy.type == e1000_phy_i217) {
- u16 phy_reg;
+ u16 phy_reg, device_id = hw->adapter->pdev->device;
+
+ if ((device_id == E1000_DEV_ID_PCH_LPTLP_I218_LM) ||
+ (device_id == E1000_DEV_ID_PCH_LPTLP_I218_V)) {
+ u32 fextnvm6 = er32(FEXTNVM6);
+
+ ew32(FEXTNVM6, fextnvm6 & ~E1000_FEXTNVM6_REQ_PLL_CLK);
+ }
ret_val = hw->phy.ops.acquire(hw);
if (ret_val)
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h b/drivers/net/ethernet/intel/e1000e/ich8lan.h
index b6d3174..8bf4655 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.h
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h
@@ -92,6 +92,8 @@
#define E1000_FEXTNVM4_BEACON_DURATION_8USEC 0x7
#define E1000_FEXTNVM4_BEACON_DURATION_16USEC 0x3
+#define E1000_FEXTNVM6_REQ_PLL_CLK 0x00000100
+
#define PCIE_ICH8_SNOOP_ALL PCIE_NO_SNOOP_ALL
#define E1000_ICH_RAR_ENTRIES 7
diff --git a/drivers/net/ethernet/intel/e1000e/regs.h b/drivers/net/ethernet/intel/e1000e/regs.h
index 794fe14..a7e6a3e 100644
--- a/drivers/net/ethernet/intel/e1000e/regs.h
+++ b/drivers/net/ethernet/intel/e1000e/regs.h
@@ -42,6 +42,7 @@
#define E1000_FEXTNVM 0x00028 /* Future Extended NVM - RW */
#define E1000_FEXTNVM3 0x0003C /* Future Extended NVM 3 - RW */
#define E1000_FEXTNVM4 0x00024 /* Future Extended NVM 4 - RW */
+#define E1000_FEXTNVM6 0x00010 /* Future Extended NVM 6 - RW */
#define E1000_FEXTNVM7 0x000E4 /* Future Extended NVM 7 - RW */
#define E1000_FCT 0x00030 /* Flow Control Type - RW */
#define E1000_VET 0x00038 /* VLAN Ether Type - RW */
--
1.7.11.7
^ permalink raw reply related
* [net 0/4][pull request] Intel Wired LAN Driver Updates
From: Jeff Kirsher @ 2013-03-05 9:48 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, gospo, sassmann
This series contains fixes to e1000e and igb.
The e1000e fix resolves an issue at 1000Mbps link speed, where one of the
MAC's internal clocks can be stopped for up to 4us when entering K1 (a
power mode of the MAC-PHY interconnect). If the MAC is waiting for
completion indications for 2 DMA write requests into Host memory
(e.g. descriptor writeback or Rx packet writing) and the
indications occur while the clock is stopped, both indications will be
missed by the MAC causing the MAC to wait for the completion indications
and be unable to generate further DMA write requests. This results in an
apparent hardware hang. The patch works-around the issue by disabling
the de-assertion of the clock request when 1000Mbps link is acquired (K1
must be disabled while doing this).
The igb fix to drop BUILD_BUG_ON check from igb_build_rx_buffer resolves
a build error on s390 devices. The igb driver was throwing a build error
due to the fact that a frame built using build_skb would be larger than 2K.
Since this is not likely to change at any point in the future we are better
off just dropping the check since we already had a check in
igb_set_rx_buffer_len that will just disable the usage of build_skb anyway.
The igb fix for i210 link setup changes the setup copper link function
to use a switch statement, so that the appropriate setup link function
is called for the given PHY types.
Lastly, the igb fix for a lockdep issue in igb_get_i2c_client resolves
the issue by re-factoring the initialization and usage of the i2c_client.
The following are changes since commit aab2b4bf224ef8358d262f95b568b8ad0cecf0a0:
tcp: fix double-counted receiver RTT when leaving receiver fast path
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net master
Alexander Duyck (1):
igb: Drop BUILD_BUG_ON check from igb_build_rx_buffer
Bruce Allan (1):
e1000e: workaround DMA unit hang on I218
Carolyn Wyborny (2):
igb: Fix link setup for I210 devices
igb: Fix for lockdep issue in igb_get_i2c_client
drivers/net/ethernet/intel/e1000e/ich8lan.c | 71 +++++++++++++++++++++++++-
drivers/net/ethernet/intel/e1000e/ich8lan.h | 2 +
drivers/net/ethernet/intel/e1000e/regs.h | 1 +
drivers/net/ethernet/intel/igb/e1000_82575.c | 11 ++--
drivers/net/ethernet/intel/igb/igb.h | 2 +-
drivers/net/ethernet/intel/igb/igb_hwmon.c | 14 +++++
drivers/net/ethernet/intel/igb/igb_main.c | 76 +---------------------------
7 files changed, 98 insertions(+), 79 deletions(-)
--
1.7.11.7
^ permalink raw reply
* RE : [PATCH 2/3] net: at91_ether: Use module_platform_driver_probe macro
From: Ferre, Nicolas @ 2013-03-05 9:41 UTC (permalink / raw)
To: Sachin Kamat, netdev@vger.kernel.org; +Cc: davem@davemloft.net
In-Reply-To: <1362457882-12892-2-git-send-email-sachin.kamat@linaro.org>
> module_platform_driver_probe() eliminates the boilerplate and simplifies
> the code.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/net/ethernet/cadence/at91_ether.c | 13 +------------
> 1 files changed, 1 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/at91_ether.c b/drivers/net/ethernet/cadence/at91_ether.c
> index 3becdb2..1a57e16 100644
> --- a/drivers/net/ethernet/cadence/at91_ether.c
> +++ b/drivers/net/ethernet/cadence/at91_ether.c
> @@ -519,18 +519,7 @@ static struct platform_driver at91ether_driver = {
> },
> };
>
> -static int __init at91ether_init(void)
> -{
> - return platform_driver_probe(&at91ether_driver, at91ether_probe);
> -}
> -
> -static void __exit at91ether_exit(void)
> -{
> - platform_driver_unregister(&at91ether_driver);
> -}
> -
> -module_init(at91ether_init)
> -module_exit(at91ether_exit)
> +module_platform_driver_probe(at91ether_driver, at91ether_probe);
>
> MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("AT91RM9200 EMAC Ethernet driver");
> --
> 1.7.4.1
>
^ permalink raw reply
* RE : [PATCH 3/3] net: macb: Use module_platform_driver_probe macro
From: Ferre, Nicolas @ 2013-03-05 9:36 UTC (permalink / raw)
To: Sachin Kamat, netdev@vger.kernel.org; +Cc: davem@davemloft.net
In-Reply-To: <1362457882-12892-3-git-send-email-sachin.kamat@linaro.org>
> module_platform_driver_probe() eliminates the boilerplate and simplifies
> the code.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> drivers/net/ethernet/cadence/macb.c | 13 +------------
> 1 files changed, 1 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 7903943..3a5d680 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1737,18 +1737,7 @@ static struct platform_driver macb_driver = {
> },
> };
>
> -static int __init macb_init(void)
> -{
> - return platform_driver_probe(&macb_driver, macb_probe);
> -}
> -
> -static void __exit macb_exit(void)
> -{
> - platform_driver_unregister(&macb_driver);
> -}
> -
> -module_init(macb_init);
> -module_exit(macb_exit);
> +module_platform_driver_probe(macb_driver, macb_probe);
>
> MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("Cadence MACB/GEM Ethernet driver");
> --
> 1.7.4.1
>
^ permalink raw reply
* Re: [PATCH] drivers/isdn: checkng length to be sure not memory overflow
From: Jiri Slaby @ 2013-03-05 9:34 UTC (permalink / raw)
To: Chen Gang; +Cc: Jiri Kosina, isdn, Greg KH, alan, netdev
In-Reply-To: <51355653.9090404@asianux.com>
On 03/05/2013 03:20 AM, Chen Gang wrote:
> 于 2013年02月28日 21:43, Jiri Slaby 写道:
>>
>> Nah, there *are* still people using ISDN.
>>
>
> ok, thanks.
>
> it seems, we need maintaining ISDN:
> need fix bugs.
> need not add new features.
> need keep current features no touch.
>
>
>> No, we cannot be sure that a user gives us less than that. Your patch
>> just throws the rest to fix that overflow, right? What I'm saying I
>> wouldn't fix more than that.
>
> what you said is: this patch need improving, is it correct ?
> if it is correct.
> I still prefer to throw the rest contents (but need improving, too)
> for maintaining, we need fix bug, but not need add new features
> so what we should do:
> a) fix the bug (should not memory overflow)
> b) keep the original buffer length no touch
> it is not sizeof(cmd.parm.cmsg.para) - 2)
> it should be sizeof(cmd.param) - sizeof(cmd.param.cmsg) + sizeof(cmd.param.cmsg.para) - 2
> c) need complete the relative document to export the limitation.
>
> is it ok ?
Yes, it is -- just fix the bug with minimal effort.
--
js
suse labs
^ permalink raw reply
* Re: [PATCH] SCTP: Free the per-net sysctl table on net exit. v2
From: Martin Mokrejs @ 2013-03-05 9:05 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: David Miller, vyasevich, netdev, linux-sctp, stable
In-Reply-To: <87obgayoo8.fsf_-_@xmission.com>
Hi,
this is to let you know that this patch is still not in stable.
I just fetched 3.7.10 and reproduced the original problem.
I bet this slipped through because there was an initial patch from Vlad Yasevich
on Jan 24, then a follow-up patch from Eric Biedermann which did not compile and
finally a v2 patch from E. B which did compile.
For me the last, v2 patch applies (with some hunk) to 3.7.10 and compiles
fine.
Thanks,
martin
Eric W. Biederman wrote:
> From: Vlad Yasevich <vyasevich@gmail.com>
> Date: Thu, 24 Jan 2013 11:02:47 -0500
>
> Per-net sysctl table needs to be explicitly freed at
> net exit. Otherwise we see the following with kmemleak:
>
> unreferenced object 0xffff880402d08000 (size 2048):
> comm "chrome_sandbox", pid 18437, jiffies 4310887172 (age 9097.630s)
> hex dump (first 32 bytes):
> b2 68 89 81 ff ff ff ff 20 04 04 f8 01 88 ff ff .h...... .......
> 04 00 00 00 a4 01 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<ffffffff815b4aad>] kmemleak_alloc+0x21/0x3e
> [<ffffffff81110352>] slab_post_alloc_hook+0x28/0x2a
> [<ffffffff81113fad>] __kmalloc_track_caller+0xf1/0x104
> [<ffffffff810f10c2>] kmemdup+0x1b/0x30
> [<ffffffff81571e9f>] sctp_sysctl_net_register+0x1f/0x72
> [<ffffffff8155d305>] sctp_net_init+0x100/0x39f
> [<ffffffff814ad53c>] ops_init+0xc6/0xf5
> [<ffffffff814ad5b7>] setup_net+0x4c/0xd0
> [<ffffffff814ada5e>] copy_net_ns+0x6d/0xd6
> [<ffffffff810938b1>] create_new_namespaces+0xd7/0x147
> [<ffffffff810939f4>] copy_namespaces+0x63/0x99
> [<ffffffff81076733>] copy_process+0xa65/0x1233
> [<ffffffff81077030>] do_fork+0x10b/0x271
> [<ffffffff8100a0e9>] sys_clone+0x23/0x25
> [<ffffffff815dda73>] stub_clone+0x13/0x20
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> I fixed the spelling of sysctl_header so the code actually
> compiles. -- EWB.
>
> Reported-by: Martin Mokrejs <mmokrejs@fold.natur.cuni.cz>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> The typo is fixed in the patch this time in addition to my test
> tree.
>
> net/sctp/sysctl.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
> index 043889a..bf3c6e8 100644
> --- a/net/sctp/sysctl.c
> +++ b/net/sctp/sysctl.c
> @@ -366,7 +366,11 @@ int sctp_sysctl_net_register(struct net *net)
>
> void sctp_sysctl_net_unregister(struct net *net)
> {
> + struct ctl_table *table;
> +
> + table = net->sctp.sysctl_header->ctl_table_arg;
> unregister_net_sysctl_table(net->sctp.sysctl_header);
> + kfree(table);
> }
>
> static struct ctl_table_header * sctp_sysctl_header;
>
^ permalink raw reply
* Re: [PATCH 3/5] wl1251: call pm_runtime_put_sync in pm_runtime_get_sync failed case
From: Luciano Coelho @ 2013-03-05 8:51 UTC (permalink / raw)
To: Li Fei
Cc: linville, wfp5p, gregkh, notasas, rjw, linux-wireless, netdev,
linux-kernel, chuansheng.liu
In-Reply-To: <1362037892.6212.13.camel@fli24-HP-Compaq-8100-Elite-CMT-PC>
On Thu, 2013-02-28 at 15:51 +0800, Li Fei wrote:
> Even in failed case of pm_runtime_get_sync, the usage_count
> is incremented. In order to keep the usage_count with correct
> value and runtime power management to behave correctly, call
> pm_runtime_put(_sync) in such case.
>
> Signed-off-by Liu Chuansheng <chuansheng.liu@intel.com>
> Signed-off-by: Li Fei <fei.li@intel.com>
> ---
Applied, thanks!
--
Luca.
^ 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