* Re: [PATCH] mlx4: Fixing Ethernet unicast packet steering
From: Roland Dreier @ 2011-08-04 2:53 UTC (permalink / raw)
To: Yevgeny Petrilin
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4E395A18.8070600-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
Hi Yevgeny!
Actually on further testing it seems that things are still not perfect
with my old firmware at least.
If I do "ifconfig eth6 mtu 1450" (where eth6 is a mlx4_en interface)
then I am no longer able to get traffic through the interface; if I
have a ping running it stops immediately. It seems any mtu change
triggers this (increasing the mtu above the default has the same
effect, and restoring the mtu to the default doesn't fix it).
Haven't tried to debug further.
- R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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] slip: cleanup statistics generation
From: David Miller @ 2011-08-04 2:12 UTC (permalink / raw)
To: matvejchikov; +Cc: netdev
In-Reply-To: <CAKh5naZiFHjQ_q00iBk1kz5XCU9E4wC+7Pxd-8w=v96z-f0Vig@mail.gmail.com>
From: Matvejchikov Ilya <matvejchikov@gmail.com>
Date: Wed, 3 Aug 2011 15:02:18 +0400
> OMG, sorry for the typo. Here is the correct patch.
>
> Subject: [PATCH] slip: cleanup statistics generation
>
> Remove unused tx_compressed, tx_compressed and tx_misses fields from
> the slip structure. Also, make some device stats generation cleanups.
>
> Signed-off-by: Matvejchikov Ilya <matvejchikov@gmail.com>
Looks better, applied, thanks.
^ permalink raw reply
* Re: [PATCH ] cdc_ncm: fixes for big-endian architecture / MIPS
From: David Miller @ 2011-08-04 2:11 UTC (permalink / raw)
To: giuseppe
Cc: alexey.orishko, netdev, oliver, linux-usb, gregkh, alexey.orishko
In-Reply-To: <87ipqeebvg.fsf@gnu.org>
From: Giuseppe Scrivano <giuseppe@southpole.se>
Date: Wed, 03 Aug 2011 17:46:43 +0200
>>From 7ba49d858103acb2ce4043127e3512ea29dff307 Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <giuseppe@southpole.se>
> Date: Fri, 15 Jul 2011 15:34:14 +0200
> Subject: [PATCH] cdc_ncm: fix endianess problem.
>
> Signed-off-by: Giuseppe Scrivano <giuseppe@southpole.se>
The patch looks great, but please repost this fresh with a more
vebose commit log message explaining all the things we discussed
to get the change to it's current form.
Thanks!
^ permalink raw reply
* Re: [resend PATCH] irda: use PCI_VENDOR_ID_*
From: David Miller @ 2011-08-04 2:04 UTC (permalink / raw)
To: jdmason; +Cc: netdev, samuel
In-Reply-To: <1312389762-9728-1-git-send-email-jdmason@kudzu.us>
From: Jon Mason <jdmason@kudzu.us>
Date: Wed, 3 Aug 2011 11:42:42 -0500
> Use PCI_VENDOR_ID_* from pci_ids.h instead of creating #define locally.
>
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
Applied, thanks.
^ permalink raw reply
* Re: [resend PATCH] ixgb: use PCI_VENDOR_ID_*
From: Jeff Kirsher @ 2011-08-04 0:09 UTC (permalink / raw)
To: Jon Mason
Cc: David S. Miller, netdev@vger.kernel.org, Brandeburg, Jesse,
Allan, Bruce W, Wyborny, Carolyn, Skidmore, Donald C,
Rose, Gregory V, Waskiewicz Jr, Peter P, Duyck, Alexander H,
Ronciak, John, e1000-devel@lists.sourceforge.net
In-Reply-To: <1312389773-9761-1-git-send-email-jdmason@kudzu.us>
[-- Attachment #1: Type: text/plain, Size: 436 bytes --]
On Wed, 2011-08-03 at 09:42 -0700, Jon Mason wrote:
> Use PCI_VENDOR_ID_* from pci_ids.h instead of creating #define
> locally.
>
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
> ---
> drivers/net/ixgb/ixgb_hw.c | 5 +++--
> drivers/net/ixgb/ixgb_ids.h | 5 -----
> drivers/net/ixgb/ixgb_main.c | 10 +++++-----
> 3 files changed, 8 insertions(+), 12 deletions(-)
Thanks Jon, I will add the patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [resend PATCH] ixgbe: remove unused #define
From: Jeff Kirsher @ 2011-08-04 0:09 UTC (permalink / raw)
To: Jon Mason
Cc: David S. Miller, netdev@vger.kernel.org, Brandeburg, Jesse,
Allan, Bruce W, Wyborny, Carolyn, Skidmore, Donald C,
Rose, Gregory V, Waskiewicz Jr, Peter P, Duyck, Alexander H,
Ronciak, John, e1000-devel@lists.sourceforge.net
In-Reply-To: <1312389773-9761-2-git-send-email-jdmason@kudzu.us>
[-- Attachment #1: Type: text/plain, Size: 311 bytes --]
On Wed, 2011-08-03 at 09:42 -0700, Jon Mason wrote:
>
> Remove unused IXGBE_INTEL_VENDOR_ID #define
>
> Signed-off-by: Jon Mason <jdmason@kudzu.us>
> ---
> drivers/net/ixgbe/ixgbe_type.h | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
Thanks Jon, I will add the patch to my queue.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: pull request: wireless 2011-08-03
From: David Miller @ 2011-08-03 23:38 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20110803144754.GB30439@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Wed, 3 Aug 2011 10:47:55 -0400
> The following changes since commit 28f4881cbf9ce285edfc245a8990af36d21c062f:
>
> bnx2x: Clear MDIO access warning during first driver load (2011-08-03 03:22:18 -0700)
>
> are available in the git repository at:
> git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git for-davem
>
Looks a lot better, pulled, thanks!
^ permalink raw reply
* Re: [PATCH] mlx4: Fixing Ethernet unicast packet steering
From: David Miller @ 2011-08-03 23:39 UTC (permalink / raw)
To: yevgenyp-VPRAkNaXOzVS1MOuV/RT9w
Cc: rdreier-FYB4Gu1CFyUAvxtiuMwx3w, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4E39594C.4090808-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
From: Yevgeny Petrilin <yevgenyp-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
Date: Wed, 3 Aug 2011 17:21:00 +0300
>
> For older FW versions, fixing the usage of per port Mac table.
> For each port we must define the base QP number, which is passed
> to the HW.
> Setting the correct value in SET_PORT FW command to enable the steering.
>
> Reported-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> Signed-off-by: Yevgeny Petrilin <yevgenyp-VPRAkNaXOzVS1MOuV/RT9w@public.gmane.org>
Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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 net-next 0/5] qlcnic: Fixes and debug support
From: David Miller @ 2011-08-03 23:30 UTC (permalink / raw)
To: anirban.chakraborty; +Cc: netdev, Dept_NX_Linux_NIC_Driver
In-Reply-To: <EA95D2DB-162E-4FBD-8DC1-F4DE41EB7ACE@qlogic.com>
From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
Date: Wed, 3 Aug 2011 13:20:00 -0700
> Do you want me to resend the patch series for net-2.6? It was my
> mistake to specify net-next instead of net-2.6
The debug support patches are not appropriate for plain 'net',
only the pure most critical bug fixes are.
^ permalink raw reply
* Re: [PATCH] mlx4: Fixing Ethernet unicast packet steering
From: David Miller @ 2011-08-03 23:28 UTC (permalink / raw)
To: roland-BHEL68pLQRGGvPXPguhicg
Cc: yevgenyp-VPRAkNaXOzVS1MOuV/RT9w,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAL1RGDVHnd2x2pWgaYG2vKKJk-qYfQp3T5pgJ-48HeaMA=9_EA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
From: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Date: Wed, 3 Aug 2011 12:06:56 -0700
> On Wed, Aug 3, 2011 at 11:58 AM, Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> wrote:
>>> For older FW versions, fixing the usage of per port Mac table.
>>> For each port we must define the base QP number, which is passed
>>> to the HW.
>>> Setting the correct value in SET_PORT FW command to enable the steering.
>
>> Thanks, testing this now.
>
> Yep, works well on my backrev FW boards. Dave, you want to merge this
> or should I?
I'll take it, thanks for testing.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" 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] net: Fix security_socket_sendmsg() bypass problem.
From: Tetsuo Handa @ 2011-08-03 21:50 UTC (permalink / raw)
To: anton; +Cc: davem, eparis, casey, mjt, netdev, linux-security-module
In-Reply-To: <20110803232957.5e7a5d0a@kryten>
Anton Blanchard wrote:
> [PATCH] net: Cap number of elements for recvmmsg and sendmmsg
>
> To limit the amount of time we can spend in recvmmsg and sendmmsg,
> cap the number of elements to UIO_MAXIOV (currently 1024).
Looks reasonable value. But it will return less than requested without setting
error code. Programmers would needlessly call getsockopt(SO_ERROR) and get 0.
Maybe -EINVAL or something is better than returning less than requested?
^ permalink raw reply
* Re: bonding and ifenslave version.
From: Stephen Hemminger @ 2011-08-03 21:33 UTC (permalink / raw)
To: Nicolas de Pesloüan; +Cc: Jay Vosburgh, Andy Gospodarek, netdev
In-Reply-To: <4E39B1AB.7050502@gmail.com>
On Wed, 03 Aug 2011 22:38:03 +0200
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
> Le 03/08/2011 22:07, Jay Vosburgh a écrit :
> > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote:
> >> Le 03/08/2011 21:03, Andy Gospodarek a écrit :
>
> >>> Distributions benefit from version numbers on userspace utils. It
> >>> would probably be better to keep ifenslave's version number as it is
> >>> to help those maintaining those distro packages.
> >>
> >> As one of the maintainers for the ifenslave package on Debian, I perfectly
> >> understand the need for an upstream version, but as such, I expected the
> >> upstream version number to change when the file change... Version numbers
> >> in Debian use upstream version numbers when available and add a subversion
> >> number for Debian specific changes. I would expect to change the version
> >> number and not only the Debian subversion when the only change is a new
> >> upstream version.
> >
> > One thing to remember here is that currently very few (perhaps
> > no) distros use the ifenslave.c that comes with mainline. The distros
> > I'm familiar with configure bonding via sysfs, either directly in
> > initscripts / sysconfig, or via a shell script ifenslave (which I
> > believe is what Debian has). Many distros still install it in
> > /sbin/ifenslave, but it isn't used by the network configuration stuff.
>
> The ifenslave package on Debian provide two things:
>
> - The binary ifenslave, simply compiled from mainline ifenslave.c.
> - A plug-in for ifupdown to allow for bonding related options in /etc/network/interfaces. I can
> confirm that this plug-in doesn't use the ifenslave command, but sysfs, since version 1.1.0-12
> (current stable version of this package is 1.1.0-17).
>
> > The ifenslave.c in mainline is pretty much just a legacy for
> > backwards compatibility; it has not had a bug fix since 2005 (a few typo
> > repairs since then), and no major functional changes since before the
> > git era.
> >
> > I was considering proposing feature removal for ifenslave.c and
> > the ioctl API to add and remove slaves, but some discussion a few months
> > ago indicated that there are apparently still some users out there (I'd
> > guess embedded of some variety).
>
> Unfortunately, there exist *many* how-to that suggest to use ifenslave, causing many users to use it
> instead of sysfs for bonding setup.
>
> At least, we can:
> - update the bonding documentation to clarify that ifenslave is deprecated (and move most ifenslave
> related stuffs at the end of the documentation);
> - possibly issue a warning when the API is used, suggesting to use sysfs instead.
>
> I can take care of the documentation update if appropriate.
>
> Nicolas.
Or I could just put a shell script that does the same thing in iproute.
^ permalink raw reply
* Re: [PATCH] bonding: document two undocumented options.
From: Jay Vosburgh @ 2011-08-03 20:59 UTC (permalink / raw)
To: =?ISO-8859-1?Q?Nicolas_de_Peslo=FCan?=
Cc: David Miller, Stephen Hemminger, nicolas.2p.debian, andy,
netdev@vger.kernel.org
In-Reply-To: <4E39A907.5040408@gmail.com>
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>Le 03/08/2011 12:44, David Miller a écrit :
>> From: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>> Date: Tue, 2 Aug 2011 22:06:55 +0200
>>
>>> Commit 655f8919d549ad1872e24d826b6ce42530516d2e
>>> bonding: add min links parameter to 802.3ad
>>>
>>> and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f
>>> bonding: add all_slaves_active parameter
>>>
>>> introduced new options to bonding, but didn't provide the documentation
>>> for those options.
>>>
>>> Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>>
>> Please explicitly mention in each new entry what the default
>> setting is.
>
>Unfortunately, I failed to find a place in the bonding code where the
>max_links option is initialized with a default value. So I must assume
>default value is zero which should cause carrier to always be asserted, or
>undefined, which should cause interesting side effects...
>
>The obvious default value should be 1, but I cannot confirm it is.
Looking at it now, I see no initialization, and it's a static,
so I believe it will end up being zero. From the code, zero seems like
the proper default, since it will make this test never pass:
/* are enough slaves available to consider link up? */
if (active->num_of_ports < bond->params.min_links) {
if (netif_carrier_ok(bond->dev)) {
netif_carrier_off(bond->dev);
return 1;
}
This will cause carrier to be asserted (for 802.3ad mode)
whenever there is an active aggregator, regardless of the number of
available links in that aggregator.
>Stephen, as the author of this feature, can you please clarify what the default value for min_links is?
>
>V2 will follow, giving the real default value for all_slaves_active and
>what I consider the sensible default value for max_links, even if the
>technical real default value is currently unclear.
I think the actual and sensible default are both zero, although
the documentation should probably mention that a value of zero is magic
and won't ever set the bond down due to too few ports (links) active.
Or, perhaps describe it how it actually works: if there are
fewer than "min_links" ports in the active aggregator, the bond is set
carrier down. The default min_links value of zero means that the bond
will never be set down due to having too few ports active.
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* Dear Esteemed Email User,
From: Webmaster Customer Care Support @ 2011-08-03 20:24 UTC (permalink / raw)
Dear Esteemed User,
Click on the link below and fill the details to improve the spam filter
services in our webmail systems for better online services to avoid virus
and spam mails.
http://h1cc.net/help/use/WEBADMIN/form1.html
Do not ignore this message.
--
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.
^ permalink raw reply
* Re: bonding and ifenslave version.
From: Nicolas de Pesloüan @ 2011-08-03 20:38 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Andy Gospodarek, netdev
In-Reply-To: <1718.1312402077@death>
Le 03/08/2011 22:07, Jay Vosburgh a écrit :
> Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote:
>> Le 03/08/2011 21:03, Andy Gospodarek a écrit :
>>> Distributions benefit from version numbers on userspace utils. It
>>> would probably be better to keep ifenslave's version number as it is
>>> to help those maintaining those distro packages.
>>
>> As one of the maintainers for the ifenslave package on Debian, I perfectly
>> understand the need for an upstream version, but as such, I expected the
>> upstream version number to change when the file change... Version numbers
>> in Debian use upstream version numbers when available and add a subversion
>> number for Debian specific changes. I would expect to change the version
>> number and not only the Debian subversion when the only change is a new
>> upstream version.
>
> One thing to remember here is that currently very few (perhaps
> no) distros use the ifenslave.c that comes with mainline. The distros
> I'm familiar with configure bonding via sysfs, either directly in
> initscripts / sysconfig, or via a shell script ifenslave (which I
> believe is what Debian has). Many distros still install it in
> /sbin/ifenslave, but it isn't used by the network configuration stuff.
The ifenslave package on Debian provide two things:
- The binary ifenslave, simply compiled from mainline ifenslave.c.
- A plug-in for ifupdown to allow for bonding related options in /etc/network/interfaces. I can
confirm that this plug-in doesn't use the ifenslave command, but sysfs, since version 1.1.0-12
(current stable version of this package is 1.1.0-17).
> The ifenslave.c in mainline is pretty much just a legacy for
> backwards compatibility; it has not had a bug fix since 2005 (a few typo
> repairs since then), and no major functional changes since before the
> git era.
>
> I was considering proposing feature removal for ifenslave.c and
> the ioctl API to add and remove slaves, but some discussion a few months
> ago indicated that there are apparently still some users out there (I'd
> guess embedded of some variety).
Unfortunately, there exist *many* how-to that suggest to use ifenslave, causing many users to use it
instead of sysfs for bonding setup.
At least, we can:
- update the bonding documentation to clarify that ifenslave is deprecated (and move most ifenslave
related stuffs at the end of the documentation);
- possibly issue a warning when the API is used, suggesting to use sysfs instead.
I can take care of the documentation update if appropriate.
Nicolas.
^ permalink raw reply
* Re: [PATCH RFC net-next] virtio_net: refill buffer right after being used
From: Michael S. Tsirkin @ 2011-08-03 20:30 UTC (permalink / raw)
To: Shirley Ma; +Cc: Mike Waychison, Rusty Russell, kvm, virtualization, netdev
In-Reply-To: <1312089375.23194.11.camel@localhost.localdomain>
On Sat, Jul 30, 2011 at 10:16:15PM -0700, Shirley Ma wrote:
> It averages the latency between each receive by filling only one set of
> buffers vs. either none buffers or 1/2 ring size buffers fill between
> receives.
I see how the overhead of allocating memory is spread more evenly. Does this
actually help some workloads?
^ permalink raw reply
* Re: [PATCH RFC net-next] virtio_net: refill buffer right after being used
From: Michael S. Tsirkin @ 2011-08-03 20:26 UTC (permalink / raw)
To: Shirley Ma; +Cc: Rusty Russell, kvm, virtualization, netdev
In-Reply-To: <1311980131.24300.30.camel@localhost.localdomain>
On Fri, Jul 29, 2011 at 03:55:31PM -0700, Shirley Ma wrote:
> Resubmit it with a typo fix.
>
> Signed-off-by: Shirley Ma <xma@us.ibm.com>
> ---
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 0c7321c..c8201d4 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -429,6 +429,22 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi, gfp_t gfp)
> return err;
> }
>
> +static int fill_one(struct virtnet_info *vi, gfp_t gfp)
> +{
> + int err;
> +
> + if (vi->mergeable_rx_bufs)
> + err = add_recvbuf_mergeable(vi, gfp);
> + else if (vi->big_packets)
> + err = add_recvbuf_big(vi, gfp);
> + else
> + err = add_recvbuf_small(vi, gfp);
> +
> + if (err >= 0)
> + ++vi->num;
> + return err;
> +}
> +
> /* Returns false if we couldn't fill entirely (OOM). */
> static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
> {
> @@ -436,17 +452,10 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp)
> bool oom;
>
> do {
> - if (vi->mergeable_rx_bufs)
> - err = add_recvbuf_mergeable(vi, gfp);
> - else if (vi->big_packets)
> - err = add_recvbuf_big(vi, gfp);
> - else
> - err = add_recvbuf_small(vi, gfp);
> -
> + err = fill_one(vi, gfp);
> oom = err == -ENOMEM;
> if (err < 0)
> break;
> - ++vi->num;
> } while (err > 0);
> if (unlikely(vi->num > vi->max))
> vi->max = vi->num;
> @@ -506,13 +515,13 @@ again:
> receive_buf(vi->dev, buf, len);
> --vi->num;
> received++;
> - }
> -
> - if (vi->num < vi->max / 2) {
> - if (!try_fill_recv(vi, GFP_ATOMIC))
> + if (fill_one(vi, GFP_ATOMIC) < 0)
> schedule_delayed_work(&vi->refill, 0);
If we get a large packet, we might add less that what we removed from
the ring. Isn't this a problem?
> }
>
> + /* notify buffers are refilled */
> + virtqueue_kick(vi->rvq);
> +
> /* Out of packets? */
> if (received < budget) {
> napi_complete(napi);
>
>
--
MST
^ permalink raw reply
* Váš webmail kvótu překročila stanovené kvóty
From: s l u ž b a @ 2011-08-03 17:42 UTC (permalink / raw)
Váš webmail kvótu překročila stanovené kvóty, která je 2 GB. Nacházíte se
v současné době běží na 2,3 GB.
Chcete-li znovu aktivovat a zvýšit své kvóty, webmail, prosím ověřit a
aktualizovat webmail účet
Ve snaze znovu aktivovat a zvýšit své kvóty webmail klikněte na odkaz níže.
http://fd8.formdesk.com/cunicz/statsqq
Pokud tak neučiníte, může mít za následek zrušení vašeho účtu webmail.
Díky, a omlouvám se za nepříjemnosti
Admin / webmaster / Místní host
^ permalink raw reply
* Re: [PATCH net-next 0/5] qlcnic: Fixes and debug support
From: Anirban Chakraborty @ 2011-08-03 20:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Dept_NX_Linux_NIC_Driver
In-Reply-To: <20110801.173840.1120019952254994551.davem@davemloft.net>
On Aug 1, 2011, at 5:38 PM, David Miller wrote:
> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
> Date: Mon, 1 Aug 2011 08:24:04 -0700
>
>>
>> On Aug 1, 2011, at 1:57 AM, David Miller wrote:
>>
>>> From: Anirban Chakraborty <anirban.chakraborty@qlogic.com>
>>> Date: Fri, 29 Jul 2011 16:30:30 -0700
>>>
>>>> Please apply the series to net-next. Thanks.
>>>
>>> Queued up for net-next
>>
>> Would it be too much of a trouble to push these to net-2.6, as these are minor bug fixes anyway?
>
> No.
Do you want me to resend the patch series for net-2.6? It was my mistake to specify net-next instead of net-2.6
Thanks.
-Anirban
^ permalink raw reply
* Re: bonding and ifenslave version.
From: Jay Vosburgh @ 2011-08-03 20:07 UTC (permalink / raw)
To: =?ISO-8859-1?Q?Nicolas_de_Peslo=FCan?=; +Cc: Andy Gospodarek, netdev
In-Reply-To: <4E39A3A7.1080905@gmail.com>
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>Le 03/08/2011 21:03, Andy Gospodarek a écrit :
>
>>> I thought introducing a new option should cause the driver version to
>>> change. Am I right?
>>
>> When a significant change happens, we try to change the version
>> number. The version number probably should have been changed when
>> those were added. Inspecting the module options or sysfs parameters
>> indicate whether or not these patches were added, so it is less of a
>> priority than when some internal infrastructure (like moving to use
>> rx_handler) changes.
>>
>> I consider it more critical to change the bonding module version when
>> something changes that cannot be detected by inspecting the module or
>> sysfs parameters. This is more helpful to users reporting problems.
>>
>
>Ok, 'sounds perfectly sensible to me, thanks.
>
>>> On a different but related topic, the version in
>>> Documentation/networking/ifenslave.c (1.1.0) didn't change since the git
>>> origin and probably since 2003.
>>>
>>> Arguably, none of the commit regarding this file introduced a significant
>>> change (with the possible exception of commit
>>> e6d184e33109010412ad1d59719af74755a935f4, [NET]: Fix ifenslave to not fail
>>> on lack of IP information). But if we never change a 3-level version number,
>>> whatever the level of change, this version number might be useless. Any
>>> comment?
>>>
>>> Nicolas.
>>>
>>
>> Distributions benefit from version numbers on userspace utils. It
>> would probably be better to keep ifenslave's version number as it is
>> to help those maintaining those distro packages.
>
>As one of the maintainers for the ifenslave package on Debian, I perfectly
>understand the need for an upstream version, but as such, I expected the
>upstream version number to change when the file change... Version numbers
>in Debian use upstream version numbers when available and add a subversion
>number for Debian specific changes. I would expect to change the version
>number and not only the Debian subversion when the only change is a new
>upstream version.
One thing to remember here is that currently very few (perhaps
no) distros use the ifenslave.c that comes with mainline. The distros
I'm familiar with configure bonding via sysfs, either directly in
initscripts / sysconfig, or via a shell script ifenslave (which I
believe is what Debian has). Many distros still install it in
/sbin/ifenslave, but it isn't used by the network configuration stuff.
The ifenslave.c in mainline is pretty much just a legacy for
backwards compatibility; it has not had a bug fix since 2005 (a few typo
repairs since then), and no major functional changes since before the
git era.
I was considering proposing feature removal for ifenslave.c and
the ioctl API to add and remove slaves, but some discussion a few months
ago indicated that there are apparently still some users out there (I'd
guess embedded of some variety).
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply
* [PATCH] bonding: document two undocumented options.
From: Nicolas de Pesloüan @ 2011-08-03 20:02 UTC (permalink / raw)
To: davem; +Cc: fubar, andy, netdev, shemminger, Nicolas de Pesloüan
In-Reply-To: <20110803.034425.1421983987148421662.davem@davemloft.net>
Commit 655f8919d549ad1872e24d826b6ce42530516d2e
bonding: add min links parameter to 802.3ad
and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f
bonding: add all_slaves_active parameter
introduced new options to bonding, but didn't provide the documentation
for those options.
V2 : add the default value for both options.
Signed-off-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>
---
Documentation/networking/bonding.txt | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index 675612f..480af6e 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -238,6 +238,18 @@ ad_select
This option was added in bonding version 3.4.0.
+all_slaves_active
+
+ Specifies that duplicate frames (received on inactive ports) should be
+ dropped (0) or delivered (1).
+
+ Normally, bonding will drop duplicate frames (received on inactive
+ ports), which is desirable for most users. But there are some times
+ it is nice to allow duplicate frames to be delivered.
+
+ The default value is 0 (drop duplicate frames received on inactive
+ ports).
+
arp_interval
Specifies the ARP link monitoring frequency in milliseconds.
@@ -433,6 +445,18 @@ miimon
determined. See the High Availability section for additional
information. The default value is 0.
+min_links
+
+ Specifies the minimum number of links that must be active before
+ asserting carrier. It is similar to the Cisco EtherChannel min-links
+ feature. This allows setting the minimum number of member ports that
+ must be up (link-up state) before marking the bond device as up
+ (carrier on). This is useful for situations where higher level services
+ such as clustering want to ensure a minimum number of low bandwidth
+ links are active before switchover. The default value is 1.
+
+ This option only affect 802.3ad mode.
+
mode
Specifies one of the bonding policies. The default is
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] bonding: document two undocumented options.
From: Nicolas de Pesloüan @ 2011-08-03 20:01 UTC (permalink / raw)
To: David Miller, Stephen Hemminger
Cc: nicolas.2p.debian, fubar, andy, netdev@vger.kernel.org
In-Reply-To: <20110803.034425.1421983987148421662.davem@davemloft.net>
Le 03/08/2011 12:44, David Miller a écrit :
> From: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
> Date: Tue, 2 Aug 2011 22:06:55 +0200
>
>> Commit 655f8919d549ad1872e24d826b6ce42530516d2e
>> bonding: add min links parameter to 802.3ad
>>
>> and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f
>> bonding: add all_slaves_active parameter
>>
>> introduced new options to bonding, but didn't provide the documentation
>> for those options.
>>
>> Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>
> Please explicitly mention in each new entry what the default
> setting is.
Unfortunately, I failed to find a place in the bonding code where the max_links option is
initialized with a default value. So I must assume default value is zero which should cause carrier
to always be asserted, or undefined, which should cause interesting side effects...
The obvious default value should be 1, but I cannot confirm it is.
Stephen, as the author of this feature, can you please clarify what the default value for min_links is?
V2 will follow, giving the real default value for all_slaves_active and what I consider the sensible
default value for max_links, even if the technical real default value is currently unclear.
Nicolas.
^ permalink raw reply
* Re: bonding and ifenslave version.
From: Nicolas de Pesloüan @ 2011-08-03 19:38 UTC (permalink / raw)
To: Andy Gospodarek; +Cc: Jay Vosburgh, netdev
In-Reply-To: <CAHashqCrRq3z_bwddy3Y3+t+hmA4oN3hndmsTKUDNc73U=c3Gg@mail.gmail.com>
Le 03/08/2011 21:03, Andy Gospodarek a écrit :
>> I thought introducing a new option should cause the driver version to
>> change. Am I right?
>
> When a significant change happens, we try to change the version
> number. The version number probably should have been changed when
> those were added. Inspecting the module options or sysfs parameters
> indicate whether or not these patches were added, so it is less of a
> priority than when some internal infrastructure (like moving to use
> rx_handler) changes.
>
> I consider it more critical to change the bonding module version when
> something changes that cannot be detected by inspecting the module or
> sysfs parameters. This is more helpful to users reporting problems.
>
Ok, 'sounds perfectly sensible to me, thanks.
>> On a different but related topic, the version in
>> Documentation/networking/ifenslave.c (1.1.0) didn't change since the git
>> origin and probably since 2003.
>>
>> Arguably, none of the commit regarding this file introduced a significant
>> change (with the possible exception of commit
>> e6d184e33109010412ad1d59719af74755a935f4, [NET]: Fix ifenslave to not fail
>> on lack of IP information). But if we never change a 3-level version number,
>> whatever the level of change, this version number might be useless. Any
>> comment?
>>
>> Nicolas.
>>
>
> Distributions benefit from version numbers on userspace utils. It
> would probably be better to keep ifenslave's version number as it is
> to help those maintaining those distro packages.
As one of the maintainers for the ifenslave package on Debian, I perfectly understand the need for
an upstream version, but as such, I expected the upstream version number to change when the file
change... Version numbers in Debian use upstream version numbers when available and add a subversion
number for Debian specific changes. I would expect to change the version number and not only the
Debian subversion when the only change is a new upstream version.
Anyway, it is not that important and I can leave with 1.1.0 for long :-D
Thanks again.
Nicolas.
^ permalink raw reply
* Re: [net-next v2 17/71] myri*: Move the Myricom drivers
From: Jon Mason @ 2011-08-03 19:37 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: davem@davemloft.net, netdev@vger.kernel.org, gospo@redhat.com,
sassmann@redhat.com, Andrew Gallatin, Brice Goglin
In-Reply-To: <1312351360.2294.95.camel@jtkirshe-mobl>
On Wed, Aug 3, 2011 at 1:02 AM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> On Mon, 2011-08-01 at 10:09 -0700, Jon Mason wrote:
>> On Sat, Jul 30, 2011 at 10:26 PM, Jeff Kirsher
>> <jeffrey.t.kirsher@intel.com> wrote:
>> > Move the Myricom drivers into drivers/net/ethernet/myricom/ and make
>> > the necessary Kconfig and Makefile changes.
>>
>> Acked-by: Jon Mason <mason@myri.com>
>>
>> > CC: Andrew Gallatin <gallatin@myri.com>
>> > CC: Brice Goglin <brice@myri.com>
>> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> Jon-
>
> I made some minor changes to the Kconfig based on feedback from others
> on other Kconfig's which made sense to apply to the other driver
> Kconfig's. Here is the Kconfig:
>
> +config NET_VENDOR_MYRI
> + bool "Myricom devices"
> + depends on PCI && INET
> + ---help---
> + If you have a network (Ethernet) card belonging to this class,
> say
> + Y and read the Ethernet-HOWTO, available from
> + <http://www.tldp.org/docs.html#howto>.
> +
> + Note that the answer to this question doesn't directly affect
> the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about Myricom cards. If you say Y, you will be
> asked for
> + your specific card in the following questions.
> +
> +if NET_VENDOR_MYRI
> +
> +config MYRI10GE
> + tristate "Myricom Myri-10G Ethernet support"
> + depends on PCI && INET
> + select FW_LOADER
> + select CRC32
> + select INET_LRO
> + ---help---
> + This driver supports Myricom Myri-10G Dual Protocol interface
> in
> + Ethernet mode. If the eeprom on your board is not recent
> enough,
> + you will need a newer firmware image.
> + You may get this image or more information, at:
> +
> + <http://www.myri.com/scs/download-Myri10GE.html>
> +
> + To compile this driver as a module, choose M here. The module
> + will be called myri10ge.
> +
> +config MYRI10GE_DCA
> + bool "Direct Cache Access (DCA) Support"
> + default y
> + depends on MYRI10GE && DCA && !(MYRI10GE=y && DCA=m)
> + ---help---
> + Say Y here if you want to use Direct Cache Access (DCA) in the
> + driver. DCA is a method for warming the CPU cache before data
> + is used, with the intent of lessening the impact of cache
> misses.
> +
> +endif # NET_VENDOR_MYRI
>
> Let me know if these changes are ok and I can add your ACK.
Looks fine to me.
Thanks,
Jon
^ permalink raw reply
* Re: [PATCH] Add pch ieee1588 driver for Intel EG20T PCH
From: Richard Cochran @ 2011-08-03 19:23 UTC (permalink / raw)
To: Toshiharu Okada
Cc: richard.cochran, netdev, linux-kernel, qi.wang, yong.y.wang,
joel.clark, kok.howg.ewe, tomoya-linux
In-Reply-To: <1312352861-10955-1-git-send-email-toshiharu-linux@dsn.okisemi.com>
On Wed, Aug 03, 2011 at 03:27:41PM +0900, Toshiharu Okada wrote:
> This patch is for IEEE1588 driver of Intel EG20T PCH.
> EG20T PCH is the platform controller hub that is used in Intel's
> general embedded platform.
> This driver adds support for using the EG20T PCH as a PTP clock.
>
> Would you review this patch, although this driver has not been tested yet?
Thanks for offering this patch. Please see my comments, below.
>
> Signed-off-by: Toshiharu Okada <toshiharu-linux@dsn.okisemi.com>
> ---
> drivers/ptp/Kconfig | 13 +
> drivers/ptp/Makefile | 1 +
> drivers/ptp/ptp_pch.c | 629 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 643 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ptp/ptp_pch.c
>
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 68d7201..cd9bc3b 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -72,4 +72,17 @@ config DP83640_PHY
> In order for this to work, your MAC driver must also
> implement the skb_tx_timetamp() function.
>
> +config PTP_1588_CLOCK_PCH
> + tristate "Intel PCH EG20T as PTP clock"
> + depends on PTP_1588_CLOCK
> + depends on PCH_GBE
> + help
> + This driver adds support for using the PCH EG20T as a PTP
> + clock. This clock is only useful if your PTP programs are
> + getting hardware time stamps on the PTP Ethernet packets
> + using the SO_TIMESTAMPING API.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called ptp_pch.
> +
> endmenu
> diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
> index f6933e8..8b58597 100644
> --- a/drivers/ptp/Makefile
> +++ b/drivers/ptp/Makefile
> @@ -5,3 +5,4 @@
> ptp-y := ptp_clock.o ptp_chardev.o ptp_sysfs.o
> obj-$(CONFIG_PTP_1588_CLOCK) += ptp.o
> obj-$(CONFIG_PTP_1588_CLOCK_IXP46X) += ptp_ixp46x.o
> +obj-$(CONFIG_PTP_1588_CLOCK_PCH) += ptp_pch.o
> diff --git a/drivers/ptp/ptp_pch.c b/drivers/ptp/ptp_pch.c
> new file mode 100644
> index 0000000..0a804dc
> --- /dev/null
> +++ b/drivers/ptp/ptp_pch.c
> @@ -0,0 +1,629 @@
> +/*
> + * PTP 1588 clock using the EG20T PCH
> + *
> + * Copyright (C) 2010 OMICRON electronics GmbH
> + * Copyright (C) 2010 OKI SEMICONDUCTOR Co., LTD.
> + *
> + * This code was derived from the IXP46X driver.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307, USA.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/ptp_clock_kernel.h>
> +
> +#define STATION_ADDR_LEN 20
> +#define PCI_DEVICE_ID_PCH_1588 0x8819
> +#define IO_MEM_BAR 1
> +
> +
> +/* Register read/write macros */
> +#define PCH_BIT_SET_CHECK(addr, bitmask) \
> + ((ioread32(addr) & (bitmask)) == (bitmask))
> +#define PCH_SET_ADDR_BIT(addr, bitmask)\
> + iowrite32((ioread32(addr) | (bitmask)), (addr))
> +#define PCH_CLR_ADDR_BIT(addr, bitmask)\
> + iowrite32((ioread32(addr) & ~(bitmask)), (addr))
I don't really like macros of this sort, just to set and clear
register bits. It doesn't make any fewer lines nor does it clarify the
code. Every driver author will know what is meant by the plain old C,
like:
x = x | mask;
x = x & ~mask;
Also, you should consider whether you need to protect against
concurrent access on a register.
> +
> +#define DEFAULT_ADDEND 0xF0000029
> +#define TICKS_NS_SHIFT 4
This driver is based on my ixp46x driver, which is fine because,
looking at the data sheet, it appears that the time stamping unit in
the EG20T PCH is fairly similar.
However, I doubt that these ADDEND and SHIFT values will work for you,
since they were calculated for the frequency compensated clock on the
IXP. They reflect an input clock frequency of 66.666666 MHz and a
nominal frequency of 62.5 MHz (or a period of 16 nanoseconds, thus
SHIFT is 4).
Section 14.6.1.10 in the data sheet seems to imply that the input
clock is 50 MHz. In that case you could use a nominal frequency of
31.25 MHz (period 32 ns, SHIFT 5). However, you need to find out
the actual input clock frequency. If this can vary, then the driver
should allow changing these values.
> +#define N_EXT_TS 2
> +
> +enum pch_status {
> + PCH_SUCCESS,
> + PCH_INVALIDPARAM,
> + PCH_NOTIMESTAMP,
> + PCH_INTERRUPTMODEINUSE,
> + PCH_FAILED,
> + PCH_UNSUPPORTED,
> +};
> +/**
> + * struct pch_ts_regs - IEEE 1588 registers
> + */
> +struct pch_ts_regs {
> + u32 control;
> + u32 event;
> + u32 addend;
> + u32 accum;
> + u32 test;
> + u32 ts_compare;
> + u32 rsystime_lo;
> + u32 rsystime_hi;
> + u32 systime_lo;
> + u32 systime_hi;
> + u32 trgt_lo;
> + u32 trgt_hi;
> + u32 asms_lo;
> + u32 asms_hi;
> + u32 amms_lo;
> + u32 amms_hi;
> + u32 ch_control;
You never program this register. But I think the "Mode" field should
be set to 2. The other settings really make no sense at all. Or do
you set this in the MAC driver?
(I wonder why Intel retained the very limited PTP V1 modes from the
IXP time stamping unit.)
> + u32 ch_event;
> + u32 tx_snap_lo;
> + u32 tx_snap_hi;
> + u32 rx_snap_lo;
> + u32 rx_snap_hi;
> + u32 src_uuid_lo;
> + u32 src_uuid_hi;
> + u32 can_status;
> + u32 can_snap_lo;
> + u32 can_snap_hi;
> + u32 ts_sel;
> + u32 ts_st[6];
> + u32 reserve1[15];
> + u32 stl_max_set_en;
> + u32 stl_max_set;
> + u32 reserve2[13];
> + u32 srst;
> +};
> +
> +#define PCH_TSC_RESET (1 << 0)
> +#define PCH_TSC_TTM_MASK (1 << 1)
> +#define PCH_TSC_ASMS_MASK (1 << 2)
> +#define PCH_TSC_AMMS_MASK (1 << 3)
> +#define PCH_TSC_PPSM_MASK (1 << 4)
> +#define PCH_TSE_TTIPEND (1 << 1)
> +#define PCH_TSE_SNS (1 << 2)
> +#define PCH_TSE_SNM (1 << 3)
> +#define PCH_TSE_PPS (1 << 4)
> +#define PCH_CC_MM (1 << 0)
> +#define PCH_CC_TA (1 << 1)
> +
> +#define PCH_CC_MODE_SHIFT 16
> +#define PCH_CC_MODE_MASK 0x001F0000
> +#define PCH_CC_VERSION (1 << 31)
> +#define PCH_CE_TXS (1 << 0)
> +#define PCH_CE_RXS (1 << 1)
> +#define PCH_CE_OVR (1 << 0)
> +#define PCH_CE_VAL (1 << 1)
> +#define PCH_ECS_ETH (1 << 0)
> +
> +#define PCH_ECS_CAN (1 << 1)
> +#define PCH_STATION_BYTES 6
> +
> +#define PCH_IEEE1588_ETH (1 << 0)
> +#define PCH_IEEE1588_CAN (1 << 1)
> +/**
> + * struct pch_dev - Driver private data
> + */
> +struct pch_dev {
> + struct pch_ts_regs *regs;
> + struct ptp_clock *ptp_clock;
> + struct ptp_clock_info caps;
> + int exts0_enabled;
> + int exts1_enabled;
> +
> + u32 mem_base;
> + u32 mem_size;
> + u32 irq;
> + u32 suspend:1;
> + u32 initialized:1;
> + struct pci_dev *pdev;
> + spinlock_t lock;
It would be nice to have a short comment telling what this lock
protects. (Yes, I know what it protects, but still that is good
practice to have a comment.)
> +};
> +
> +static inline void pch_eth_enable_set(struct pch_dev *chip)
> +{
> + /* SET the eth_enable bit */
> + PCH_SET_ADDR_BIT(&chip->regs->ts_sel, PCH_ECS_ETH);
> +}
I really don't like this or the other similar helper functions,
below. They don't make the driver more understandable or shorter.
This function is only used in one place. You need at least two callers
to justify this.
> +
> +/*
> + * Register access functions
> + */
> +
> +static u64 pch_systime_read(struct pch_ts_regs *regs)
> +{
> + u64 ns;
> + u32 lo, hi;
> +
> + lo = ioread32(®s->systime_lo);
> + hi = ioread32(®s->systime_hi);
> +
> + ns = ((u64) hi) << 32;
> + ns |= lo;
> + ns <<= TICKS_NS_SHIFT;
Here 'ns' will only be in nanoseconds when the ADDEND and SHIFT macros
are correct for the actual machine, as I mentioned above.
> +
> + return ns;
> +}
> +
> +static void pch_systime_write(struct pch_ts_regs *regs, u64 ns)
> +{
> + u32 hi, lo;
> +
> + ns >>= TICKS_NS_SHIFT;
> + hi = ns >> 32;
> + lo = ns & 0xffffffff;
> +
> + iowrite32(lo, ®s->systime_lo);
> + iowrite32(hi, ®s->systime_hi);
> +}
> +
I think the following helper functions ...
> +static inline u32 pch_pps_evt_get(struct pch_dev *chip)
> +{
> + /* Poll for PPS event */
> + return PCH_BIT_SET_CHECK(&chip->regs->event, PCH_TSE_PPS);
> +}
> +
> +static inline u32 pch_amms_evt_get(struct pch_dev *chip)
> +{
> + /* Poll for Auxiliary Master Mode Snapshot Captured event */
> + return PCH_BIT_SET_CHECK(&chip->regs->event, PCH_TSE_SNM);
> +}
> +
> +static inline u32 pch_asms_evt_get(struct pch_dev *chip)
> +{
> + /* Poll for Auxiliary Slave Mode Snapshot Captured event */
> + return PCH_BIT_SET_CHECK(&chip->regs->event, PCH_TSE_SNS);
> +}
> +
> +static inline u32 pch_ttm_evt_get(struct pch_dev *chip)
> +{
> + /* Poll for Target Time Reached event */
> + return PCH_BIT_SET_CHECK(&chip->regs->event, PCH_TSE_TTIPEND);
> +}
> +
> +static inline void pch_pps_evt_clear(struct pch_dev *chip)
> +{
> + /* Clear PPS event */
> + PCH_SET_ADDR_BIT(&chip->regs->event, PCH_TSE_PPS);
> +}
> +
> +static inline void pch_amms_evt_clear(struct pch_dev *chip)
> +{
> + /* Clear Auxiliary Master Mode Snapshot Captured event */
> + PCH_SET_ADDR_BIT(&chip->regs->event, PCH_TSE_SNM);
> +}
> +
> +static inline void pch_asms_evt_clear(struct pch_dev *chip)
> +{
> + /* Clear Auxiliary Slave Mode Snapshot Captured event */
> + PCH_SET_ADDR_BIT(&chip->regs->event, PCH_TSE_SNS);
> +}
> +
> +static inline void pch_ttm_evt_clear(struct pch_dev *chip)
> +{
> + /* Clear Target Time Reached event */
> + PCH_SET_ADDR_BIT(&chip->regs->event, PCH_TSE_TTIPEND);
> +}
... are just noise and add nothing useful to the driver.
> +
> +static inline void pch_block_reset(struct pch_dev *chip)
> +{
> + /* Reset Hardware Assist block */
> + PCH_SET_ADDR_BIT(&chip->regs->control, PCH_TSC_RESET);
> + PCH_CLR_ADDR_BIT(&chip->regs->control, PCH_TSC_RESET);
> +}
> +
> +/* This function enables all 64 bits in system time registers [high & low].
> +This is a work-around for non continuous value in the SystemTime Register*/
> +static void pch_set_system_time_count(struct pch_dev *chip)
> +{
> + iowrite32(0x01, &chip->regs->stl_max_set_en);
> + iowrite32(0xFFFFFFFF, &chip->regs->stl_max_set);
> + iowrite32(0x00, &chip->regs->stl_max_set_en);
> +}
> +
> +static void pch_reset(struct pch_dev *chip)
> +{
> + /* Reset Hardware Assist */
> + pch_block_reset(chip);
> +
> + /* enable all 32 bits in system time registers */
> + pch_set_system_time_count(chip);
> +}
These three, above, are okay, since they encapsulate one logical
operation that takes more than one register access.
However, you might consider whether you need locking here.
> +
> +static void pch_eth_enable(struct pch_dev *chip)
> +{
> + pch_eth_enable_set(chip);
> +}
Again, this helper only has one caller. Why not just set the bit that
you need in line?
> +
> +/*
> + * Interrupt service routine
> + */
> +static irqreturn_t isr(int irq, void *priv)
> +{
> + struct pch_dev *pch_dev = priv;
> + struct pch_ts_regs *regs = pch_dev->regs;
> + struct ptp_clock_event event;
> + u32 ack = 0, lo, hi, val;
> +
> + val = ioread32(®s->event);
> +
> + if (val & PCH_TSE_SNS) {
> + ack |= PCH_TSE_SNS;
> + if (pch_dev->exts0_enabled) {
> + hi = ioread32(®s->asms_hi);
> + lo = ioread32(®s->asms_lo);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 0;
> + event.timestamp = ((u64) hi) << 32;
> + event.timestamp |= lo;
> + event.timestamp <<= TICKS_NS_SHIFT;
> + ptp_clock_event(pch_dev->ptp_clock, &event);
> + }
> + }
> +
> + if (val & PCH_TSE_SNM) {
> + ack |= PCH_TSE_SNM;
> + if (pch_dev->exts1_enabled) {
> + hi = ioread32(®s->amms_hi);
> + lo = ioread32(®s->amms_lo);
> + event.type = PTP_CLOCK_EXTTS;
> + event.index = 1;
> + event.timestamp = ((u64) hi) << 32;
> + event.timestamp |= lo;
> + event.timestamp <<= TICKS_NS_SHIFT;
> + ptp_clock_event(pch_dev->ptp_clock, &event);
> + }
> + }
> +
> + if (val & PCH_TSE_TTIPEND)
> + ack |= PCH_TSE_TTIPEND; /* this bit seems to be always set */
This ISR code (and much of the rest of the driver) is copied from the
IXP driver. It would be nice to know if it actually works on the atom
hardware. Do have any hardware to try it on?
> +
> + if (ack) {
> + iowrite32(ack, ®s->event);
> + return IRQ_HANDLED;
> + } else
> + return IRQ_NONE;
> +}
> +
> +/*
> + * PTP clock operations
> + */
> +
> +static int ptp_pch_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> + u64 adj;
> + u32 diff, addend;
> + int neg_adj = 0;
> + struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> + struct pch_ts_regs *regs = pch_dev->regs;
> +
> + if (ppb < 0) {
> + neg_adj = 1;
> + ppb = -ppb;
> + }
> + addend = DEFAULT_ADDEND;
> + adj = addend;
> + adj *= ppb;
> + diff = div_u64(adj, 1000000000ULL);
> +
> + addend = neg_adj ? addend - diff : addend + diff;
> +
> + iowrite32(addend, ®s->addend);
> +
> + return 0;
> +}
> +
> +static int ptp_pch_adjtime(struct ptp_clock_info *ptp, s64 delta)
> +{
> + s64 now;
> + unsigned long flags;
> + struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> + struct pch_ts_regs *regs = pch_dev->regs;
> +
> + spin_lock_irqsave(&pch_dev->lock, flags);
> + now = pch_systime_read(regs);
> + now += delta;
> + pch_systime_write(regs, now);
> + spin_unlock_irqrestore(&pch_dev->lock, flags);
> +
> + return 0;
> +}
> +
> +static int ptp_pch_gettime(struct ptp_clock_info *ptp, struct timespec *ts)
> +{
> + u64 ns;
> + u32 remainder;
> + unsigned long flags;
> + struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> + struct pch_ts_regs *regs = pch_dev->regs;
> +
> + spin_lock_irqsave(&pch_dev->lock, flags);
> + ns = pch_systime_read(regs);
> + spin_unlock_irqrestore(&pch_dev->lock, flags);
> +
> + ts->tv_sec = div_u64_rem(ns, 1000000000, &remainder);
> + ts->tv_nsec = remainder;
> + return 0;
> +}
> +
> +static int ptp_pch_settime(struct ptp_clock_info *ptp,
> + const struct timespec *ts)
> +{
> + u64 ns;
> + unsigned long flags;
> + struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> + struct pch_ts_regs *regs = pch_dev->regs;
> +
> + ns = ts->tv_sec * 1000000000ULL;
> + ns += ts->tv_nsec;
> +
> + spin_lock_irqsave(&pch_dev->lock, flags);
> + pch_systime_write(regs, ns);
> + spin_unlock_irqrestore(&pch_dev->lock, flags);
> +
> + return 0;
> +}
> +
> +static int ptp_pch_enable(struct ptp_clock_info *ptp,
> + struct ptp_clock_request *rq, int on)
> +{
> + struct pch_dev *pch_dev = container_of(ptp, struct pch_dev, caps);
> +
> + switch (rq->type) {
> + case PTP_CLK_REQ_EXTTS:
> + switch (rq->extts.index) {
> + case 0:
> + pch_dev->exts0_enabled = on ? 1 : 0;
> + break;
> + case 1:
> + pch_dev->exts1_enabled = on ? 1 : 0;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> + default:
> + break;
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
> +static struct ptp_clock_info ptp_pch_caps = {
> + .owner = THIS_MODULE,
> + .name = "PCH timer",
> + .max_adj = 66666655,
This should be recalculated once you figure out the input clock and
nominal frequency.
> + .n_ext_ts = N_EXT_TS,
> + .pps = 0,
> + .adjfreq = ptp_pch_adjfreq,
> + .adjtime = ptp_pch_adjtime,
> + .gettime = ptp_pch_gettime,
> + .settime = ptp_pch_settime,
> + .enable = ptp_pch_enable,
> +};
> +
> +
> +#ifdef CONFIG_PM
> +static s32 pch_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> + struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> + chip->suspend = 1;
You set this flag here ...
> + pci_disable_device(pdev);
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> +
> + if (pci_save_state(pdev) != 0) {
> + dev_err(&pdev->dev,
> + "%s: could not save PCI config state\n", __func__);
> + return -ENOMEM;
> + }
> + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +
> + return 0;
> +}
> +
> +static s32 pch_resume(struct pci_dev *pdev)
> +{
> + s32 ret;
> + struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + dev_err(&pdev->dev, "%s: pci_enable_device failed\n", __func__);
> + return ret;
> + }
> + pci_enable_wake(pdev, PCI_D3hot, 0);
> + chip->suspend = 0;
... and clear it again here. Why?
> + return 0;
> +}
> +#else
> +#define pch_suspend NULL
> +#define pch_resume NULL
> +#endif
> +
> +static void __devexit pch_remove(struct pci_dev *pdev)
> +{
> + struct pch_dev *chip = pci_get_drvdata(pdev);
> +
> + ptp_clock_unregister(chip->ptp_clock);
> + /* free the interrupt */
> + if (pdev->irq != 0)
> + free_irq(pdev->irq, chip);
> +
> + /* unmap the virtual IO memory space */
> + if (chip->regs != 0) {
> + iounmap(chip->regs);
> + chip->regs = 0;
> + }
> + /* release the reserved IO memory space */
> + if (chip->mem_base != 0) {
> + release_mem_region(chip->mem_base, chip->mem_size);
> + chip->mem_base = 0;
> + }
> + pci_disable_device(pdev);
> + kfree(chip);
> + dev_info(&pdev->dev, "%s: complete\n", __func__);
> +}
> +
> +static s32 __devinit
> +pch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + s32 ret;
> + struct pch_dev *chip;
> +
> + chip = kzalloc(sizeof(struct pch_dev), GFP_KERNEL);
> + if (chip == NULL)
> + return -ENOMEM;
> +
> + /* enable the 1588 pci device */
> + ret = pci_enable_device(pdev);
> + if (ret != 0) {
> + dev_err(&pdev->dev,
> + "%s:could not enable the pci device\n", __func__);
> + goto err_pci_en;
> + }
> +
> + chip->mem_base = pci_resource_start(pdev, IO_MEM_BAR);
> + if (!chip->mem_base) {
> + dev_err(&pdev->dev,
> + "%s: could not locate IO memory address\n", __func__);
> + ret = -ENODEV;
> + goto err_pci_start;
> + }
> +
> + /* retreive the available length of the IO memory space */
> + chip->mem_size = pci_resource_len(pdev, IO_MEM_BAR);
> +
> + /* allocate the memory for the device registers */
> + if (!request_mem_region
> + (chip->mem_base, chip->mem_size, "1588_regs")) {
Poor statement break (and this would fit all on one line).
> + dev_err(&pdev->dev,
> + "%s: could not allocate register memory space\n", __func__);
Bad indentation.
> + ret = -EBUSY;
> + goto err_req_mem_region;
> + }
> +
> + /* get the virtual address to the 1588 registers */
> + chip->regs = ioremap(chip->mem_base, chip->mem_size);
> +
> + if (!chip->regs) {
> + dev_err(&pdev->dev,
> + "%s: Could not get virtual address\n", __func__);
> + ret = -ENOMEM;
> + goto err_ioremap;
> + }
> +
> + chip->caps = ptp_pch_caps;
> + chip->ptp_clock = ptp_clock_register(&chip->caps);
> +
> + if (IS_ERR(chip->ptp_clock))
> + return PTR_ERR(chip->ptp_clock);
> +
> + spin_lock_init(&chip->lock);
> +
> + ret = request_irq(pdev->irq, &isr, IRQF_SHARED, KBUILD_MODNAME, chip);
> + if (ret != 0) {
> + dev_err(&pdev->dev,
> + "%s: failed to get irq %d\n", __func__, pdev->irq);
> + goto err_req_irq;
> + }
> +
> + chip->initialized = 1;
You set this flag, but never use it.
> + /* indicate success */
> + chip->irq = pdev->irq;
> + chip->pdev = pdev;
> + pci_set_drvdata(pdev, chip);
> +
> + /* reset the ieee1588 h/w */
> + pch_reset(chip);
> +
> + iowrite32(DEFAULT_ADDEND, &chip->regs->addend);
> + iowrite32(1, &chip->regs->trgt_lo);
> + iowrite32(0, &chip->regs->trgt_hi);
> + iowrite32(PCH_TSE_TTIPEND, &chip->regs->event);
> + pch_eth_enable(chip);
> +
> + return 0;
> +
> +err_req_irq:
> + ptp_clock_unregister(chip->ptp_clock);
> + iounmap(chip->regs);
> + chip->regs = 0;
> +
> +err_ioremap:
> + release_mem_region(chip->mem_base, chip->mem_size);
> +
> +err_req_mem_region:
> + chip->mem_base = 0;
> +
> +err_pci_start:
> + pci_disable_device(pdev);
> +
> +err_pci_en:
> + kfree(chip);
> + dev_err(&pdev->dev, "%s: probe failed(ret=0x%x)\n", __func__, ret);
> +
> + return ret;
> +}
> +
> +static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
> + {.vendor = PCI_VENDOR_ID_INTEL,
Needs a space (or newline) before the dot.
> + .device = PCI_DEVICE_ID_PCH_1588
> + },
> + {0}
> +};
> +
> +static struct pci_driver pch_pcidev = {
> + .name = KBUILD_MODNAME,
> + .id_table = pch_pcidev_id,
Here you meant "pch_gbe_pcidev_id" instead (or no "gbe" in the PCI
device table).
> + .probe = pch_probe,
> + .remove = pch_remove,
> + .suspend = pch_suspend,
> + .resume = pch_resume,
> +};
> +
> +static void __exit ptp_pch_exit(void)
> +{
> + pci_unregister_driver(&pch_pcidev);
> +}
> +
> +static s32 __init ptp_pch_init(void)
> +{
> + s32 ret;
> +
> + /* register the driver with the pci core */
> + ret = pci_register_driver(&pch_pcidev);
> +
> + return ret;
> +}
> +
> +module_init(ptp_pch_init);
> +module_exit(ptp_pch_exit);
> +
> +MODULE_AUTHOR("OKI SEMICONDUCTOR, <toshiharu-linux@dsn.okisemi.com>");
> +MODULE_DESCRIPTION("PTP clock using the EG20T timer");
> +MODULE_LICENSE("GPL");
Overall, the driver looks okay. I would appreciate if you would take a
look at the comments and submit a revised patch.
I would also like to see how the time stamps are done in the MAC
driver. Have you already posted that?
Feature request: I notice in the data sheet that the time stamping
unit can produce a PPS output. Any chance that you could program this
feature?
Thanks,
Richard
> +
> --
> 1.7.4.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
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