* Re: AF_BUS socket address family
From: Vincent Sanders @ 2012-06-30 0:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20120629.165023.1605284574408858612.davem@davemloft.net>
On Fri, Jun 29, 2012 at 04:50:23PM -0700, David Miller wrote:
> From: Vincent Sanders <vincent.sanders@collabora.co.uk>
> Date: Sat, 30 Jun 2012 00:42:30 +0100
>
> > Basically you are indicating you would be completely opposed to any
> > mechanism involving D-Bus IPC and the kernel?
>
> I would not oppose existing mechanisms, which I do not believe is
> impossible to use in your scenerio.
>
You keep saying that yet have offered no concrete way to achive the
semantics we require. To pass fd and credentials currently *requires*
the use of AF_UNIX does it not? And D-Bus already emulates its IPC
over AF_UNIX because of that.
> What you really don't get is that packet drops and event losses are
> absolutely fundamental.
not within an IPC surely? there cannot be packet drops within AF_BUS
we simply do not do it. The rrecive queues are checked for capability
of reciving the message before it is delivered to them all or none.
>
> As long as receivers lack infinite receive queue this will always be
> the case.
Indeed, I would not question that.
>
> Multicast operates in non-reliable transports only so that one stuck
> or malfunctioning receiver doesn't screw things over for everyone nor
> unduly brudon the sender.
>
We have addressed this within AF_BUS by the reciver and bus master
being told if all recepients cannot receive the message (and therefor
it cannot be sent).
The policy decision of how to handle this situation is therefore
handled by the userspace clients on a protocol level. D-Bus *already*
has to handle this situation, its just currently done over AF_UNIX
sockets so once it occours the problem is harder to rectify as the
ordering constraint is broken (which causes even more issues).
I am afraid it is rather late here and I may not be able to continue
this conversation untill the morning, I apologise if this is
inconveniant, but I must sleep.
--
Regards Vincent
^ permalink raw reply
* Re: AF_BUS socket address family
From: David Miller @ 2012-06-29 23:50 UTC (permalink / raw)
To: vincent.sanders; +Cc: netdev, linux-kernel
In-Reply-To: <20120629234230.GA11480@kyllikki.org>
From: Vincent Sanders <vincent.sanders@collabora.co.uk>
Date: Sat, 30 Jun 2012 00:42:30 +0100
> Basically you are indicating you would be completely opposed to any
> mechanism involving D-Bus IPC and the kernel?
I would not oppose existing mechanisms, which I do not believe is
impossible to use in your scenerio.
What you really don't get is that packet drops and event losses are
absolutely fundamental.
As long as receivers lack infinite receive queue this will always be
the case.
Multicast operates in non-reliable transports only so that one stuck
or malfunctioning receiver doesn't screw things over for everyone nor
unduly brudon the sender.
^ permalink raw reply
* Re: [PATCH 0/2] [net-next] Netlink updates
From: David Miller @ 2012-06-29 23:42 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <1340986522-3442-1-git-send-email-pablo@netfilter.org>
From: pablo@netfilter.org
Date: Fri, 29 Jun 2012 18:15:20 +0200
> If you like them, please manually apply. I wanted to know if you are
> happy with these before pushing them into my tree, as they include
> netlink changes.
Looks great, both applied.
The interface can probably be extended further, if you notice all
callers pass in THIS_MODULE. So you can have an inline wrapper
"netlink_kernel_create()" and passes the THIS_MODULE value down
into "__netlink_kernel_create()"
^ permalink raw reply
* Re: AF_BUS socket address family
From: Vincent Sanders @ 2012-06-29 23:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20120629.161821.948325645333976311.davem@davemloft.net>
On Fri, Jun 29, 2012 at 04:18:21PM -0700, David Miller wrote:
> From: Vincent Sanders <vincent.sanders@collabora.co.uk>
> Date: Sat, 30 Jun 2012 00:12:37 +0100
>
> > I had hoped you would have at least read the opening list where I
> > outlined the underlying features which explain why none of the
> > existing IPC match the requirements.
>
> I had hoped that you had read the part we told you last time where
> we explained why multicast and "reliable delivery" are fundamentally
> incompatible attributes.
>
I do not beleive we indicated reliable delivery, mearly ordered and
idempotent. eitehr everyone gets the message in the same order or
noone gets it.
> We are not creating a full address family in the kernel which exists
> for one, and only one, specific and difficult user.
Basically you are indicating you would be completely opposed to any
mechanism involving D-Bus IPC and the kernel?
Is there were a way to convince you that this is of real value to a
great many of the users of Linux systems in use today. I can assert
with some confidence that there are many, many more users of D-Bus IPC
than there are for several of the other address families that are
present within the kernel already.
The current users are suffering from the issues outlined in my
introductory mail all the time. These issues are caused by emulating an
IPC system over AF_UNIX in userspace.
All we are trying to do is make things better for our users, is there
a way to do that which will satisfy you technically and them? Honestly
I am just looking for a viable solution here.
--
Regards Vincent
^ permalink raw reply
* Re: [PATCH 0/5] netfilter fixes for 3.5-rc4
From: David Miller @ 2012-06-29 23:37 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, netdev
In-Reply-To: <1340984255-738-1-git-send-email-pablo@netfilter.org>
From: pablo@netfilter.org
Date: Fri, 29 Jun 2012 17:37:30 +0200
> * One refcount leak fix in IPVS IPv6 support from Eric Dumazet.
>
> * One fix for interface comparison in ipset hash-netiface sets
> from Florian Westphal.
>
> * One fix for a missing rcu_read_unlock in nfnetlink from
> Tomasz Bursztyka.
>
> * One fix for a kernel crash if IPSET_CMD_NONE is set to ipset via
> nfnetlink, again from Tomasz Bursztyka.
>
> You can pull these changes from:
>
> git://1984.lsi.us.es/nf master
Pulled, thanks Pablo.
^ permalink raw reply
* Re: [PATCH v5] sctp: be more restrictive in transport selection on bundled sacks
From: David Miller @ 2012-06-29 23:34 UTC (permalink / raw)
To: nhorman; +Cc: netdev, vyasevich, linux-sctp
In-Reply-To: <1341000929-22933-1-git-send-email-nhorman@tuxdriver.com>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 29 Jun 2012 16:15:29 -0400
> + /* Has this transport moved the ctsn since we last sacked */
> + __u32 sack_generation;
> +
...
> + __u32 sack_generation;
These are __u32 but they only take on the value '1' or '0'. Please
use bool and give it a more reasonable name, a name that describes
how it is really a predicate.
> - struct sctp_association *asoc;
> struct timer_list *timer;
> - asoc = pkt->transport->asoc;
> + struct sctp_association *asoc = pkt->transport->asoc;
> +
Please leave asoc where it was, on the first line. We encourage
listing local variables such that the longest lines come first,
then gradually shorter and short lines.
> + /*
> + * Once we have a sack generated, check to see what our sack
> + * generation is, if its 0, reset the transports to 0, and reset
Please format:
/* Like
* this.
*/
Thanks.
^ permalink raw reply
* Re: pull request: wireless-next 2012-06-29
From: David Miller @ 2012-06-29 23:30 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev
In-Reply-To: <20120629172836.GB30528@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 29 Jun 2012 13:28:37 -0400
> Here is another batch of updates intended for 3.6. This includes a
> number of pulls, including ones from the mac80211, iwlwifi, ath6kl, and
> wl12xx trees. I also pulled from the wireless tree to avoid potential
> build conflicts. There are a number of other patches applied directly,
> including a number for the Broadcom drivers and the mwifiex driver.
>
> The updates cover the usual variety of new hardware support and feature
> enhancements. It's all good work, but there aren't any big headliners.
> This does resolve a net-next/wireless-next merge conflict reported
> by Stephen.
Pulled, thanks John.
^ permalink raw reply
* Re: AF_BUS socket address family
From: Vincent Sanders @ 2012-06-29 23:22 UTC (permalink / raw)
To: Casey Schaufler; +Cc: netdev, linux-kernel, David S. Miller
In-Reply-To: <4FEDF7B6.3020107@schaufler-ca.com>
On Fri, Jun 29, 2012 at 11:45:10AM -0700, Casey Schaufler wrote:
> On 6/29/2012 9:45 AM, Vincent Sanders wrote:
<snip>
> >
> > A socket created using BUS_PROTO_DBUS indicates that the messages
> > passed will be in the D-Bus format. The userspace libraries have been
> > updated to use this transport with an updated D-Bus daemon [2] as a bus
> > master.
>
> Why don't you go whole hog and put all of D-Bus into the kernel?
>
That would be ridiculously excessive. This work represents what we
feel is the minimum required functionlity for the underlying IPC
mechanism.
The minimal filtering performed by the netfilter module is what is
required to enforce security as used in existing deployments and no more.
<snip>
> >
> > The tools for testing these assertions are available [3] and
> > consistently show a doubling in throughput and better than halving of
> > latency.
>
> Please cross-post Patches 04/15 and 05/15 to the linux-security-module list.
> Please cross-post Patch 05/15 to the selinux list.
>
> Where is the analogous patch for the Smack LSM?
we have not tested or built this with the Smack LSM, I would, of
course, be pleased to accept a patch to add this functionality if you
are knowladgeable in this area.
<snip>
^ permalink raw reply
* Re: AF_BUS socket address family
From: David Miller @ 2012-06-29 23:18 UTC (permalink / raw)
To: vincent.sanders; +Cc: netdev, linux-kernel
In-Reply-To: <20120629231236.GA28593@mail.collabora.co.uk>
From: Vincent Sanders <vincent.sanders@collabora.co.uk>
Date: Sat, 30 Jun 2012 00:12:37 +0100
> I had hoped you would have at least read the opening list where I
> outlined the underlying features which explain why none of the
> existing IPC match the requirements.
I had hoped that you had read the part we told you last time where
we explained why multicast and "reliable delivery" are fundamentally
incompatible attributes.
We are not creating a full address family in the kernel which exists
for one, and only one, specific and difficult user.
^ permalink raw reply
* Re: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.
From: Francois Romieu @ 2012-06-29 23:02 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: David Miller, netdev, Dept-NX Linux NIC Driver
In-Reply-To: <5E4F49720D0BAD499EE1F01232234BA877435B23DA@AVEXMB1.qlogic.org>
(linux-zupk.site removed)
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
[...]
> - broken indent
> - netif_err(... "cleared successfully" ...)
> [JK] I don't think so, %s will be replace with "cleared successfully" or with "clear failed" depend upon status value.
I would not netif_err when everything is ok.
[...]
> If I read things correctly, WAKE_MAGIC can not be cleared and it makes no
> difference on the status code. It should be fixed.
>
> [JK] You right, here we are not clearing WAKE_MAGIC but we are making sure that wol is disable when no option is set.
Imho wol should be disabled as well when neither WAKE_MAGIC nor any option
is set at all.
--
Ueimor
^ permalink raw reply
* Re: AF_BUS socket address family
From: Vincent Sanders @ 2012-06-29 23:12 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel
In-Reply-To: <20120629.153656.1141845894730637434.davem@davemloft.net>
On Fri, Jun 29, 2012 at 03:36:56PM -0700, David Miller wrote:
>
> There is no extensive text describing why using IPv4 for this cannot
> be done. I can almost bet that nobody really, honestly, tried.
>
I can assure you that the team has tried no fewer than six differing
approaches, including using IP and attempting to bend several of the
existing address families.
> Basically this means all of our feedback from the last time we had
> discussions on kernel IPC for DBUS are being completely ignored.
Absolutely not, we listened hard and did extensive research, please do
not ascribe thoughtlessness to our actions. Certainly I would not
presume to waste your time and present something which has not been
thoroughly considered.
I had hoped you would have at least read the opening list where I
outlined the underlying features which explain why none of the
existing IPC match the requirements.
Firstly it is intended is an interprocess mechanism and not to rely on
a configured IP system, indeed one of its primary usages is to
provide mechanism for various tools to set up IP networking.
Leaving that aside the requirements for multicast, strict ordering, fd
passing and credential passing are simply not available in any other
single transport. It was made plain to us that AF_UNIX would not be
expanded to encompass multicast so we are left with adding AF_BUS.
If we are wrong I hope you will explain to me how we can achieve fd and
credential passing to multicast groups within existing protocols.
>
> Therefore, I will completely ignore this patch submission.
>
I do hope you will reconsider, or at least educate us appropriately.
I understand you are a busy maintainer and appreciate your time in this matter.
Best regards
--
Vincent Sanders <vincent.sanders@collabora.co.uk>
^ permalink raw reply
* Re: [PATCH] net: Update netdev_alloc_frag to work more efficiently with TCP and GRO
From: Alexander Duyck @ 2012-06-29 23:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Alexander Duyck, netdev, davem, jeffrey.t.kirsher
In-Reply-To: <1340368430.4604.10280.camel@edumazet-glaptop>
On 06/22/2012 05:33 AM, Eric Dumazet wrote:
> Here is the patch I tested here.
>
> Using 32768 bytes allocations is actually nice for MTU=9000 traffic,
> since we can fit 3 frames per 32KB instead of only 2 frames (using
> kmalloc-16384 slab))
>
> Also, I prefill page->_count with a high bias value, to avoid the
> get_page() we did for each allocated frag.
>
> In my profiles, the get_page() cost was dominant, because of false
> sharing with skb consumers (as they might run on different cpus)
>
> This way, when 32768 bytes are filled, we perform a single
> atomic_sub_return() and can recycle the page if we find we are the last
> user (this is what you did in your patch, when testing page->_count
> being 1)
>
> Note : If I used max(PAGE_SIZE, 32678) for MAX_NETDEV_FRAGSIZE,
> gcc was not able to optimise get_order(MAX_NETDEV_FRAGSIZE), strange...
>
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 5b21522..d31efa2 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -296,9 +296,18 @@ EXPORT_SYMBOL(build_skb);
> struct netdev_alloc_cache {
> struct page *page;
> unsigned int offset;
> + unsigned int pagecnt_bias;
> };
> static DEFINE_PER_CPU(struct netdev_alloc_cache, netdev_alloc_cache);
>
> +#if PAGE_SIZE > 32768
> +#define MAX_NETDEV_FRAGSIZE PAGE_SIZE
> +#else
> +#define MAX_NETDEV_FRAGSIZE 32768
> +#endif
> +
> +#define NETDEV_PAGECNT_BIAS (MAX_NETDEV_FRAGSIZE / \
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
> /**
> * netdev_alloc_frag - allocate a page fragment
> * @fragsz: fragment size
> @@ -316,18 +325,25 @@ void *netdev_alloc_frag(unsigned int fragsz)
> nc = &__get_cpu_var(netdev_alloc_cache);
> if (unlikely(!nc->page)) {
> refill:
> - nc->page = alloc_page(GFP_ATOMIC | __GFP_COLD);
> + nc->page = alloc_pages(GFP_ATOMIC | __GFP_COLD | __GFP_COMP,
> + get_order(MAX_NETDEV_FRAGSIZE));
> + if (unlikely(!nc->page))
> + goto end;
> +recycle:
> + atomic_set(&nc->page->_count, NETDEV_PAGECNT_BIAS);
> + nc->pagecnt_bias = NETDEV_PAGECNT_BIAS;
> nc->offset = 0;
> }
> - if (likely(nc->page)) {
> - if (nc->offset + fragsz > PAGE_SIZE) {
> - put_page(nc->page);
> - goto refill;
> - }
> - data = page_address(nc->page) + nc->offset;
> - nc->offset += fragsz;
> - get_page(nc->page);
> + if (nc->offset + fragsz > MAX_NETDEV_FRAGSIZE) {
> + if (!atomic_sub_return(nc->pagecnt_bias,
> + &nc->page->_count))
> + goto recycle;
> + goto refill;
> }
> + data = page_address(nc->page) + nc->offset;
> + nc->offset += fragsz;
> + nc->pagecnt_bias--; /* avoid get_page()/get_page() false sharing */
> +end:
> local_irq_restore(flags);
> return data;
> }
> @@ -353,7 +369,7 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
> unsigned int fragsz = SKB_DATA_ALIGN(length + NET_SKB_PAD) +
> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>
> - if (fragsz <= PAGE_SIZE && !(gfp_mask & __GFP_WAIT)) {
> + if (fragsz <= MAX_NETDEV_FRAGSIZE && !(gfp_mask & __GFP_WAIT)) {
> void *data = netdev_alloc_frag(fragsz);
>
> if (likely(data)) {
>
>
I was wondering if there were any plans to clean this patch up and
submit it to net-next? If not, I can probably work on that since this
addressed the concerns I had in my original patch.
Thanks,
Alex
^ permalink raw reply
* Source-specific multicast socket options documentation
From: Radek Pazdera @ 2012-06-29 22:52 UTC (permalink / raw)
To: mtk.manpages; +Cc: linux-man, netdev
Hi,
I found out, that some source-specific multicast socket options are
missing in the IPv4 manpage (man 7 ip) and I'm trying to write some
documentation for them.
This is what I have now (without the formatting). I'd like to ask,
if you could spare a minute and have just a quick look over it
to make sure there are not some errors (either technical or language -
I'm not native speaker), before I post the patch.
IP_ADD_SOURCE_MEMBERSHIP (since Linux 2.5.68)
Join a multicast group and allow receiving data only from a specific
source. Argument is an ip_mreq_source structure.
struct ip_mreq_source {
struct in_addr imr_multiaddr; /* IP multicast group
address */
struct in_addr imr_interface; /* IP address of local
interface */
struct in_addr imr_sourceaddr; /* IP address of
multicast source */
};
The ip_mreq_source structure is similar to ip_mreqn structure.
imr_multiaddr contains the address of the multicast group
the application wants to join or leave. imr_address is the
address of the local interface which the system will join
the multicast group with. imr_sourceaddr is the address of
multicast source the application wants to receive data
from.
Application can use this option multiple times to receive
data from more than one source.
IP_DROP_SOURCE_MEMBERHSIP (since Linux 2.5.68)
Leave a source-specific group, i.e. stop receiving data from
a given multicast group, coming from a given source.
If the application has subscribed to multiple sources within
the same group, data from the remaining sources will still
be delivered. To stop receiving data from all sources at once
use IP_LEAVE_GROUP.
Argument is an ip_mreq_source structure as described at
IP_ADD_SOURCE_MEMBERSHIP.
IP_BLOCK_SOURCE (since Linux 2.5.68)
Block receiving multicast data from a specific source in a given
group. This is valid only after the application have subscribed
to the multicast group using either IP_ADD_MEMBERSHIP or
IP_ADD_SOURCE_MEMBERSHIP.
Argument is an ip_mreq_source structure as described at
IP_ADD_SOURCE_MEMBERSHIP.
IP_UNBLOCK_SOURCE (since Linux 2.5.68)
Unblock previously blocked multicast source. Returns EADDRNOTAVAIL
when given source was not being blocked.
Argument is an ip_mreq_source structure as described at
IP_ADD_SOURCE_MEMBERSHIP.
IP_MSFILTER (since Linux 2.5.68)
This option provides access to the advanced full-state filtering API.
Argument is an ip_msfilter structure.
struct ip_msfilter {
struct in_addr imsf_multiaddr; /* IP multicast group
address */
struct in_addr imsf_interface; /* IP address of local
interface */
uint32_t imsf_fmode; /* Filter-mode */
uint32_t imsf_numsrc; /* Number of sources in
the following array */
struct in_addr imsf_slist[1]; /* Array of source
addresses */
};
There are two macros, MCAST_INCLUDE and MCAST_EXCLUDE, which can be
used to specify the filtering mode. Additionaly, IP_MSFILTER_SIZE(n)
macro exists to determine how much memory is needed to store
ip_msfilter structure with n sources in the source list.
For full description of multicast source filtering refer to RFC 3376.
My sources for these information were:
* http://tools.ietf.org/html/rfc3678
* http://tools.ietf.org/html/rfc3376
* code (http://lxr.free-electrons.com/source/net/ipv4/ip_sockglue.c)
Thank you!
Radek Pazdera :)
^ permalink raw reply
* RE: [PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.
From: Jitendra Kalsaria @ 2012-06-29 22:48 UTC (permalink / raw)
To: Francois Romieu
Cc: David Miller, netdev, Ron Mercer, Dept-NX Linux NIC Driver
In-Reply-To: <20120629213755.GC19152@electric-eye.fr.zoreil.com>
Francois,
See comments inline.
Jiten
-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com]
Sent: Friday, June 29, 2012 2:38 PM
To: Jitendra Kalsaria
Cc: David Miller; netdev; Ron@linux-zupk.site; Dept-NX Linux NIC Driver
Subject: Re: [PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
[...]
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index cdbc860..aa514c5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -2701,11 +2701,9 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
> pci_alloc_consistent(qdev->pdev, tx_ring->wq_size,
> &tx_ring->wq_base_dma);
>
> - if ((tx_ring->wq_base == NULL) ||
> - tx_ring->wq_base_dma & WQ_ADDR_ALIGN) {
> - netif_err(qdev, ifup, qdev->ndev, "tx_ring alloc failed.\n");
> - return -ENOMEM;
> - }
> + if ((tx_ring->wq_base == NULL) || tx_ring->wq_base_dma & WQ_ADDR_ALIGN)
> + goto err;
> +
> tx_ring->q =
> kmalloc(tx_ring->wq_len * sizeof(struct tx_ring_desc), GFP_KERNEL);
> if (tx_ring->q == NULL)
> @@ -2713,8 +2711,12 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
>
> return 0;
> err:
> - pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> + if (tx_ring->wq_base) {
> + pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> tx_ring->wq_base, tx_ring->wq_base_dma);
> + tx_ring->wq_base = NULL;
> + }
You do not need a test: use a second label + goto.
Nit: You can replace 'if ((tx_ring->wq_base == NULL)' with
'if (!tx_ring->wq_base' and add a local variable for qdev->pdev.
You may consider replacing pci_alloc_consistent with dma_alloc_coherent
in a future patch.
[JK] Will, definitely take care of this in future patch.
--
Ueimor
^ permalink raw reply
* RE: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.
From: Jitendra Kalsaria @ 2012-06-29 22:40 UTC (permalink / raw)
To: Francois Romieu
Cc: David Miller, netdev, Ron@linux-zupk.site,
Dept-NX Linux NIC Driver
In-Reply-To: <20120629213747.GB19152@electric-eye.fr.zoreil.com>
Francois,
See comments inline.
Jiten
-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com]
Sent: Friday, June 29, 2012 2:38 PM
To: Jitendra Kalsaria
Cc: David Miller; netdev; Ron@linux-zupk.site; Dept-NX Linux NIC Driver
Subject: Re: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
>
> Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c | 43 ++++++++++++++--------
> 1 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index 8e2c2a7..81672f5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -388,10 +388,14 @@ static void ql_get_drvinfo(struct net_device *ndev,
> static void ql_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> {
> struct ql_adapter *qdev = netdev_priv(ndev);
> - /* What we support. */
> - wol->supported = WAKE_MAGIC;
> - /* What we've currently got set. */
> - wol->wolopts = qdev->wol;
> +
> + if (qdev->pdev->subsystem_device == 0x0068 ||
> + qdev->pdev->subsystem_device == 0x0180) {
> + /* What we support. */
> + wol->supported = WAKE_MAGIC;
> + /* What we've currently got set. */
> + wol->wolopts = qdev->wol;
> + }
unsigned short ssys_dev = qdev->pdev->subsystem_device;
if (ssys_dev == 0x0068 || ssys_dev == 0x0180) {
wol->supported = WAKE_MAGIC;
wol->wolopts = qdev->wol;
}
[JK] Will do this.
> }
>
> static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> @@ -399,19 +403,26 @@ static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> struct ql_adapter *qdev = netdev_priv(ndev);
> int status;
>
> - if (wol->wolopts & ~WAKE_MAGIC)
> - return -EINVAL;
> - qdev->wol = wol->wolopts;
> -
> - netif_info(qdev, drv, qdev->ndev, "Set wol option 0x%x\n", qdev->wol);
> - if (!qdev->wol) {
> - u32 wol = 0;
> - status = ql_mb_wol_mode(qdev, wol);
> - netif_err(qdev, drv, qdev->ndev, "WOL %s (wol code 0x%x)\n",
> - status == 0 ? "cleared successfully" : "clear failed",
> - wol);
> + if (qdev->pdev->subsystem_device == 0x0068 ||
> + qdev->pdev->subsystem_device == 0x0180) {
See above.
> + if (wol->wolopts & ~WAKE_MAGIC)
> + return -EINVAL;
> + qdev->wol = wol->wolopts;
> +
> + netif_info(qdev, drv, qdev->ndev,
> + "Set wol option 0x%x\n", qdev->wol);
qdev->ndev == ndev, right ?
[JK] Yes
> + if (!qdev->wol) {
> + u32 wol = 0;
> + status = ql_mb_wol_mode(qdev, wol);
Missing empty line.
status should be declared here.
[JK] Will fix this
> + netif_err(qdev, drv, qdev->ndev,
> + "WOL %s (wol code 0x%x)\n",
> + status == 0 ? "cleared successfully" : "clear failed",
> + wol);
- broken indent
- netif_err(... "cleared successfully" ...)
[JK] I don't think so, %s will be replace with "cleared successfully" or with "clear failed" depend upon status value.
> + }
> + } else {
> + netif_info(qdev, drv, qdev->ndev,
> + "WOL is not supported on stand-up card\n");
> }
> -
> return 0;
If I read things correctly, WAKE_MAGIC can not be cleared and it makes no
difference on the status code. It should be fixed.
[JK] You right, here we are not clearing WAKE_MAGIC but we are making sure that wol is disable when no option is set.
--
Ueimor
^ permalink raw reply
* Re: AF_BUS socket address family
From: David Miller @ 2012-06-29 22:36 UTC (permalink / raw)
To: vincent.sanders; +Cc: netdev, linux-kernel
In-Reply-To: <1340988354-26981-1-git-send-email-vincent.sanders@collabora.co.uk>
There is no extensive text describing why using IPv4 for this cannot
be done. I can almost bet that nobody really, honestly, tried.
Basically this means all of our feedback from the last time we had
discussions on kernel IPC for DBUS are being completely ignored.
Therefore, I will completely ignore this patch submission.
^ permalink raw reply
* Re: [BUG, regression, bisected] Marvell 88E8055 NIC (sky2) fails to detect link after resume from S3
From: Michal Zatloukal @ 2012-06-29 22:36 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, Mirko Lindner, Stephen Hemminger
In-Reply-To: <20120629215840.GA21533@electric-eye.fr.zoreil.com>
Sorry, not a developer, but I assume you mean something like:
if (!hw)
return 0;
+ mdelay(800);
/* Re-enable all clocks */
Right? In that case, I'm building the kernel now and should be able to
report by tomorrow.
MZ
On Fri, Jun 29, 2012 at 11:58 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> (maintainers Cced)
>
> Michal Zatloukal <myxal.mxl@gmail.com> :
> [7afe1845dd1e7c90828c942daed7e57ffa7c38d6 induced regression]
>> My uneducated guess is that by making the resume from S3 shorter, the
>> driver catches the hardware with its pants down and freaks out.
>> You can find all details/files (dmesg, lspci, dmidecode, config...)
>> collected by apport in the ubuntu bug linked above. Let me know if I
>> should supply any more info.
>> Note: Please CC me into replies, I'm not subscribed. Thank you.
>
> Can you workaround it by enforcing some mdelay() in sky2_resume before
> it does any real work ?
>
> --
> Ueimor
^ permalink raw reply
* Re: [PATCH net-next] cnic: Fix mmap regression.
From: David Miller @ 2012-06-29 22:34 UTC (permalink / raw)
To: mchan; +Cc: netdev
In-Reply-To: <1340998365-15753-1-git-send-email-mchan@broadcom.com>
From: "Michael Chan" <mchan@broadcom.com>
Date: Fri, 29 Jun 2012 12:32:45 -0700
> commit 1f85d58cdf15354a7120fc9ccc9bb9c45b53af88
> cnic: Remove uio mem[0].
>
> introduced a regression as older versions of userspace app still rely
> on this mmap. Restore the mmap functionality and get the base address
> from pci_resource_start() as the nedev->base_addr has been deprecated for
> PCI devices.
>
> Update version to 2.5.12.
>
> Signed-off-by: Michael Chan <mchan@broadocm.com>
I really couldn't believe what you guys were doing in the original
commit, but I decided to let you do stupid things and find out the
hard way that removing any user visible interface is basically
impossible.
Applied, thanks.
^ permalink raw reply
* RE: [PATCH net-next 0/6] qlge: bug fix
From: Jitendra Kalsaria @ 2012-06-29 22:05 UTC (permalink / raw)
To: Francois Romieu; +Cc: David Miller, netdev, Dept-NX Linux NIC Driver
In-Reply-To: <20120629214015.GE19152@electric-eye.fr.zoreil.com>
Francois,
I have already fixed my cc list. Will resubmit to net as major of them where bug fixes and thanks for your suggestion.
Jiten
-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com]
Sent: Friday, June 29, 2012 2:40 PM
To: Jitendra Kalsaria
Cc: David Miller; netdev; Dept-NX Linux NIC Driver
Subject: Re: [PATCH net-next 0/6] qlge: bug fix
Please fix your Cc:.
Ron@linux-zupk.site does not exist.
--
Ueimor
^ permalink raw reply
* Re: [BUG, regression, bisected] Marvell 88E8055 NIC (sky2) fails to detect link after resume from S3
From: Francois Romieu @ 2012-06-29 21:58 UTC (permalink / raw)
To: Michal Zatloukal; +Cc: netdev, Mirko Lindner, Stephen Hemminger
In-Reply-To: <op.wgon84tw16tawo@esprimo>
(maintainers Cced)
Michal Zatloukal <myxal.mxl@gmail.com> :
[7afe1845dd1e7c90828c942daed7e57ffa7c38d6 induced regression]
> My uneducated guess is that by making the resume from S3 shorter, the
> driver catches the hardware with its pants down and freaks out.
> You can find all details/files (dmesg, lspci, dmidecode, config...)
> collected by apport in the ubuntu bug linked above. Let me know if I
> should supply any more info.
> Note: Please CC me into replies, I'm not subscribed. Thank you.
Can you workaround it by enforcing some mdelay() in sky2_resume before
it does any real work ?
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next 0/6] qlge: bug fix
From: Francois Romieu @ 2012-06-29 21:40 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: davem, netdev, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1340994290-28832-1-git-send-email-jitendra.kalsaria@qlogic.com>
Please fix your Cc:.
Ron@linux-zupk.site does not exist.
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next 5/6] qlge: Categorize receive frame errors from firmware.
From: Francois Romieu @ 2012-06-29 21:38 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: davem, netdev, Ron, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1340994290-28832-6-git-send-email-jitendra.kalsaria@qlogic.com>
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
>
> Signed-off-by: Sritej Velaga <sritej.velaga@qlogic.com>
> Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 8 ++++
> drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c | 14 +++++++
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 46 ++++++++++++++++++----
> 3 files changed, 59 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 5a639df..e81bbb7 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1535,6 +1535,14 @@ struct nic_stats {
> u64 rx_1024_to_1518_pkts;
> u64 rx_1519_to_max_pkts;
> u64 rx_len_err_pkts;
> + /* Receive Mac Err stats */
> + u64 rx_code_err;
> + u64 rx_oversize_err;
> + u64 rx_undersize_err;
> + u64 rx_preamble_err;
> + u64 rx_frame_len_err;
> + u64 rx_crc_err;
> + u64 rx_err_count;
> /*
> * These stats come from offset 500h to 5C8h
> * in the XGMAC register.
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index 966bd96..bbc4136 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -226,6 +226,13 @@ static char ql_stats_str_arr[][ETH_GSTRING_LEN] = {
> {"rx_1024_to_1518_pkts"},
> {"rx_1519_to_max_pkts"},
> {"rx_len_err_pkts"},
> + {"rx_code_err"},
> + {"rx_oversize_err"},
> + {"rx_undersize_err"},
> + {"rx_preamble_err"},
> + {"rx_frame_len_err"},
> + {"rx_crc_err"},
> + {"rx_err_count"},
> {"tx_cbfc_pause_frames0"},
> {"tx_cbfc_pause_frames1"},
> {"tx_cbfc_pause_frames2"},
> @@ -320,6 +327,13 @@ ql_get_ethtool_stats(struct net_device *ndev,
> *data++ = s->rx_1024_to_1518_pkts;
> *data++ = s->rx_1519_to_max_pkts;
> *data++ = s->rx_len_err_pkts;
> + *data++ = s->rx_code_err;
> + *data++ = s->rx_oversize_err;
> + *data++ = s->rx_undersize_err;
> + *data++ = s->rx_preamble_err;
> + *data++ = s->rx_frame_len_err;
> + *data++ = s->rx_crc_err;
> + *data++ = s->rx_err_count;
> *data++ = s->tx_cbfc_pause_frames0;
> *data++ = s->tx_cbfc_pause_frames1;
> *data++ = s->tx_cbfc_pause_frames2;
The stats seem to be 64 bits integers only. Couldn't you use a plain loop
with ARRAY_SIZE(ql_stats_str_arr) ?
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index aa514c5..0f56148 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1433,6 +1433,34 @@ map_error:
> return NETDEV_TX_BUSY;
> }
>
> +/* Categorizing receive firmware frame errors */
> +static void ql_categorize_rx_err(struct ql_adapter *qdev, u8 rx_err)
> +{
> + qdev->nic_stats.rx_err_count++;
Use a local variable for &qdev->nic_stats ?
> +
> + switch (rx_err & IB_MAC_IOCB_RSP_ERR_MASK) {
> + case IB_MAC_IOCB_RSP_ERR_CODE_ERR:
> + qdev->nic_stats.rx_code_err++;
> + break;
> + case IB_MAC_IOCB_RSP_ERR_OVERSIZE:
> + qdev->nic_stats.rx_oversize_err++;
> + break;
> + case IB_MAC_IOCB_RSP_ERR_UNDERSIZE:
> + qdev->nic_stats.rx_undersize_err++;
> + break;
> + case IB_MAC_IOCB_RSP_ERR_PREAMBLE:
> + qdev->nic_stats.rx_preamble_err++;
> + break;
> + case IB_MAC_IOCB_RSP_ERR_FRAME_LEN:
> + qdev->nic_stats.rx_frame_len_err++;
> + break;
> + case IB_MAC_IOCB_RSP_ERR_CRC:
> + qdev->nic_stats.rx_crc_err++;
> + default:
> + break;
> + }
> +}
> +
> /* Process an inbound completion from an rx ring. */
> static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
> struct rx_ring *rx_ring,
> @@ -1446,6 +1474,12 @@ static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
>
> napi->dev = qdev->ndev;
>
> + if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) {
> + ql_categorize_rx_err(qdev, ib_mac_rsp->flags2);
> + put_page(lbq_desc->p.pg_chunk.page);
> + return;
> + }
I did not expect a put_page here from the commit description.
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.
From: Francois Romieu @ 2012-06-29 21:37 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: davem, netdev, Ron, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1340994290-28832-5-git-send-email-jitendra.kalsaria@qlogic.com>
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
[...]
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index cdbc860..aa514c5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -2701,11 +2701,9 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
> pci_alloc_consistent(qdev->pdev, tx_ring->wq_size,
> &tx_ring->wq_base_dma);
>
> - if ((tx_ring->wq_base == NULL) ||
> - tx_ring->wq_base_dma & WQ_ADDR_ALIGN) {
> - netif_err(qdev, ifup, qdev->ndev, "tx_ring alloc failed.\n");
> - return -ENOMEM;
> - }
> + if ((tx_ring->wq_base == NULL) || tx_ring->wq_base_dma & WQ_ADDR_ALIGN)
> + goto err;
> +
> tx_ring->q =
> kmalloc(tx_ring->wq_len * sizeof(struct tx_ring_desc), GFP_KERNEL);
> if (tx_ring->q == NULL)
> @@ -2713,8 +2711,12 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
>
> return 0;
> err:
> - pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> + if (tx_ring->wq_base) {
> + pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> tx_ring->wq_base, tx_ring->wq_base_dma);
> + tx_ring->wq_base = NULL;
> + }
You do not need a test: use a second label + goto.
Nit: You can replace 'if ((tx_ring->wq_base == NULL)' with
'if (!tx_ring->wq_base' and add a local variable for qdev->pdev.
You may consider replacing pci_alloc_consistent with dma_alloc_coherent
in a future patch.
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.
From: Francois Romieu @ 2012-06-29 21:37 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: davem, netdev, Ron, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1340994290-28832-3-git-send-email-jitendra.kalsaria@qlogic.com>
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
>
> Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c | 43 ++++++++++++++--------
> 1 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index 8e2c2a7..81672f5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -388,10 +388,14 @@ static void ql_get_drvinfo(struct net_device *ndev,
> static void ql_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> {
> struct ql_adapter *qdev = netdev_priv(ndev);
> - /* What we support. */
> - wol->supported = WAKE_MAGIC;
> - /* What we've currently got set. */
> - wol->wolopts = qdev->wol;
> +
> + if (qdev->pdev->subsystem_device == 0x0068 ||
> + qdev->pdev->subsystem_device == 0x0180) {
> + /* What we support. */
> + wol->supported = WAKE_MAGIC;
> + /* What we've currently got set. */
> + wol->wolopts = qdev->wol;
> + }
unsigned short ssys_dev = qdev->pdev->subsystem_device;
if (ssys_dev == 0x0068 || ssys_dev == 0x0180) {
wol->supported = WAKE_MAGIC;
wol->wolopts = qdev->wol;
}
> }
>
> static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> @@ -399,19 +403,26 @@ static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> struct ql_adapter *qdev = netdev_priv(ndev);
> int status;
>
> - if (wol->wolopts & ~WAKE_MAGIC)
> - return -EINVAL;
> - qdev->wol = wol->wolopts;
> -
> - netif_info(qdev, drv, qdev->ndev, "Set wol option 0x%x\n", qdev->wol);
> - if (!qdev->wol) {
> - u32 wol = 0;
> - status = ql_mb_wol_mode(qdev, wol);
> - netif_err(qdev, drv, qdev->ndev, "WOL %s (wol code 0x%x)\n",
> - status == 0 ? "cleared successfully" : "clear failed",
> - wol);
> + if (qdev->pdev->subsystem_device == 0x0068 ||
> + qdev->pdev->subsystem_device == 0x0180) {
See above.
> + if (wol->wolopts & ~WAKE_MAGIC)
> + return -EINVAL;
> + qdev->wol = wol->wolopts;
> +
> + netif_info(qdev, drv, qdev->ndev,
> + "Set wol option 0x%x\n", qdev->wol);
qdev->ndev == ndev, right ?
> + if (!qdev->wol) {
> + u32 wol = 0;
> + status = ql_mb_wol_mode(qdev, wol);
Missing empty line.
status should be declared here.
> + netif_err(qdev, drv, qdev->ndev,
> + "WOL %s (wol code 0x%x)\n",
> + status == 0 ? "cleared successfully" : "clear failed",
> + wol);
- broken indent
- netif_err(... "cleared successfully" ...)
> + }
> + } else {
> + netif_info(qdev, drv, qdev->ndev,
> + "WOL is not supported on stand-up card\n");
> }
> -
> return 0;
If I read things correctly, WAKE_MAGIC can not be cleared and it makes no
difference on the status code. It should be fixed.
--
Ueimor
^ permalink raw reply
* Re: [PATCH net-next 0/6] qlge: bug fix
From: Francois Romieu @ 2012-06-29 21:36 UTC (permalink / raw)
To: Jitendra Kalsaria; +Cc: davem, netdev, Ron, Dept_NX_Linux_NIC_Driver
In-Reply-To: <1340994290-28832-1-git-send-email-jitendra.kalsaria@qlogic.com>
Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
>
> Please apply it to net-next.
Bug fixes are supposed to be applied to current net, and fast-tracked
to stable.
Why net-next ?
--
Ueimor
^ 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