* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Joe Perches @ 2012-08-24 18:27 UTC (permalink / raw)
To: Naresh Kumar Inna
Cc: Nicholas A. Bellinger, JBottomley@parallels.com,
linux-scsi@vger.kernel.org, Dimitrios Michailidis,
netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <5037BC94.1050201@chelsio.com>
On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
> Is there a TRUE/FALSE define in a standard header file?
include/linux/stddef.h:
enum {
false = 0,
true = 1
};
> I see a lot of
> kernel code defining their own TRUE/FALSE.
Those should be changed eventually.
^ permalink raw reply
* Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Stephen Warren @ 2012-08-24 18:29 UTC (permalink / raw)
To: Timur Tabi
Cc: Fleming Andy-AFLEMING, David Miller, ddaney.cavm@gmail.com,
netdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
Scott Wood
In-Reply-To: <5037AB67.6030106@freescale.com>
On 08/24/2012 10:27 AM, Timur Tabi wrote:
> Stephen Warren wrote:
>>>>>> Then, that'd have to be <9 1>;
>>>>
>>>> Actually, I had #size-cells = <0>.
>
>> I think that if you have #size-cells=<0>, then you'll see the following
>> error message when attempting to translate the address into the parent's
>> address space:
>>
>> prom_parse: Bad cell count for /board-control@3,0/mdio-mux-emi2
>
> It doesn't appear to be working. Here's my tree:
>
> / {
> model = "fsl,P5020DS";
> compatible = "fsl,P5020DS";
> #address-cells = <2>;
> #size-cells = <2>;
> interrupt-parent = <&mpic>;
>
> lbc: localbus@ffe124000 {
> reg = <0xf 0xfe124000 0 0x1000>;
> ranges = <0 0 0xf 0xe8000000 0x08000000
> 2 0 0xf 0xffa00000 0x00040000
> 3 0 0xf 0xffdf0000 0x00008000>;
>
> fpga: board-control@3,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> reg = <3 0 0x30>;
>
> mdio-mux-emi1 {
> compatible = "mdio-mux-mmioreg";
> mdio-parent-bus = <&mdio0>;
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <9 1>; // BRDCFG1
> mux-mask = <0x78>; // EMI1
>
> That means that the physical address that I need is fffdf0009. However,
> when I call of_address_to_resource(), the returned address I get is fe8000009.
>
> So it's not picking up the "3" in the 'reg' property of the
> board-control@3,0 node. What am I missing? Do I need a 'ranges' property
> in the board-control@3,0 node?
Yes.
When translating the child node's reg property into the parent's address
space, the parent's reg property shouldn't even be used at all; all the
mapping is done through the ranges property.
I thought the code error-checked for a missing ranges property, but I
guess not...
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Naresh Kumar Inna @ 2012-08-24 18:36 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <1345751929.10190.83.camel@haakon2.linux-iscsi.org>
On 8/24/2012 1:28 AM, Nicholas A. Bellinger wrote:
> On Fri, 2012-08-24 at 03:57 +0530, Naresh Kumar Inna wrote:
>> This patch contains the first set of the header files for csiostor driver.
>>
>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>> ---
>> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
>> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
>> drivers/scsi/csiostor/csio_hw.h | 668 ++++++++++++++++++++++++
>> drivers/scsi/csiostor/csio_init.h | 158 ++++++
>> 4 files changed, 1812 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/scsi/csiostor/csio_defs.h
>> create mode 100644 drivers/scsi/csiostor/csio_fcoe_proto.h
>> create mode 100644 drivers/scsi/csiostor/csio_hw.h
>> create mode 100644 drivers/scsi/csiostor/csio_init.h
>>
>
> Hi Naresh,
>
> Just commenting on csio_defs.h bits here... As Robert mentioned, you'll
> need to convert the driver to use (or add to) upstream protocol
> definitions and drop the csio_fcoe_proto.h bits..
>
Hi Nicholas,
I would like take up the discussion of the protocol header file in that
email thread. Please find the rest of my replies inline.
Thanks for reviewing,
Naresh.
>> diff --git a/drivers/scsi/csiostor/csio_defs.h b/drivers/scsi/csiostor/csio_defs.h
>> new file mode 100644
>> index 0000000..4f1c713
>> --- /dev/null
>> +++ b/drivers/scsi/csiostor/csio_defs.h
>> @@ -0,0 +1,143 @@
>> +/*
>> + * This file is part of the Chelsio FCoE driver for Linux.
>> + *
>> + * Copyright (c) 2008-2012 Chelsio Communications, Inc. All rights reserved.
>> + *
>> + * This software is available to you under a choice of one of two
>> + * licenses. You may choose to be licensed under the terms of the GNU
>> + * General Public License (GPL) Version 2, available from the file
>> + * COPYING in the main directory of this source tree, or the
>> + * OpenIB.org BSD license below:
>> + *
>> + * Redistribution and use in source and binary forms, with or
>> + * without modification, are permitted provided that the following
>> + * conditions are met:
>> + *
>> + * - Redistributions of source code must retain the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer.
>> + *
>> + * - Redistributions in binary form must reproduce the above
>> + * copyright notice, this list of conditions and the following
>> + * disclaimer in the documentation and/or other materials
>> + * provided with the distribution.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>> + * SOFTWARE.
>> + */
>> +
>> +#ifndef __CSIO_DEFS_H__
>> +#define __CSIO_DEFS_H__
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/timer.h>
>> +#include <linux/list.h>
>> +#include <linux/bug.h>
>> +#include <linux/pci.h>
>> +#include <linux/jiffies.h>
>> +
>> +/* Function returns */
>> +enum csio_retval {
>> + CSIO_SUCCESS = 0,
>> + CSIO_INVAL = 1,
>> + CSIO_BUSY = 2,
>> + CSIO_NOSUPP = 3,
>> + CSIO_TIMEOUT = 4,
>> + CSIO_NOMEM = 5,
>> + CSIO_NOPERM = 6,
>> + CSIO_RETRY = 7,
>> + CSIO_EPROTO = 8,
>> + CSIO_EIO = 9,
>> + CSIO_CANCELLED = 10,
>> +};
>> +
>
> Please don't assign macros for errno's, and give them positive values.
>
Although some of these return values appear to be mapped to errno
values, there are others that do not have an errno equivalent (example
CSIO_CANCELLED). We may have future needs to have more driver/protocol
specific return values as well. What do you suggest?
>> +#define csio_retval_t enum csio_retval
>
> Please get rid of this csio_retval_t nonsense.
I can get rid of the typedef and use enum csio_retval instead.
>
>> +
>> +enum {
>> + CSIO_FALSE = 0,
>> + CSIO_TRUE = 1,
>> +};
>> +
>
> Same here, please use normal Boolean macros
>
>> +#define CSIO_ROUNDUP(__v, __r) (((__v) + (__r) - 1) / (__r))
>> +#define CSIO_INVALID_IDX 0xFFFFFFFF
>> +#define csio_inc_stats(elem, val) ((elem)->stats.val++)
>> +#define csio_dec_stats(elem, val) ((elem)->stats.val--)
>
> No reason for either of this stats inc+dec macros. Please drop them.
I will get rid of them.
>
>> +#define csio_valid_wwn(__n) ((*__n >> 4) == 0x5 ? CSIO_TRUE : \
>> + CSIO_FALSE)
>> +#define CSIO_WORD_TO_BYTE 4
>> +
>> +static inline int
>> +csio_list_deleted(struct list_head *list)
>> +{
>> + return ((list->next == list) && (list->prev == list));
>> +}
>> +
>> +#define csio_list_next(elem) (((struct list_head *)(elem))->next)
>> +#define csio_list_prev(elem) (((struct list_head *)(elem))->prev)
>> +
>> +#define csio_deq_from_head(head, elem) \
>> +do { \
>> + if (!list_empty(head)) { \
>> + *((struct list_head **)(elem)) = csio_list_next((head)); \
>> + csio_list_next((head)) = \
>> + csio_list_next(csio_list_next((head))); \
>> + csio_list_prev(csio_list_next((head))) = (head); \
>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
>> + } else \
>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
>> +} while (0)
>> +
>
> This code is confusing as hell.. Why can't you just use normal list.h
> macros for this..?
I have not found an equivalent function in list.h that does the above
and the following macro. Could you please point me to it? I have seen a
couple of other drivers define their own macros to achieve what this
macro does, hence I assumed there isnt a list.h macro that does this.
>> +#define csio_deq_from_tail(head, elem) \
>> +do { \
>> + if (!list_empty(head)) { \
>> + *((struct list_head **)(elem)) = csio_list_prev((head)); \
>> + csio_list_prev((head)) = \
>> + csio_list_prev(csio_list_prev((head))); \
>> + csio_list_next(csio_list_prev((head))) = (head); \
>> + INIT_LIST_HEAD(*((struct list_head **)(elem))); \
>> + } else \
>> + *((struct list_head **)(elem)) = (struct list_head *)NULL;\
>> +} while (0)
>> +
>
> Same here.. Please don't use macros like this.
>
>> +/* State machine */
>> +typedef void (*csio_sm_state_t)(void *, uint32_t);
>> +
>> +struct csio_sm {
>> + struct list_head sm_list;
>> + csio_sm_state_t sm_state;
>> +};
>> +
>> +#define csio_init_state(__smp, __state) \
>> + (((struct csio_sm *)(__smp))->sm_state = (csio_sm_state_t)(__state))
>> +
>> +#define csio_set_state(__smp, __state) \
>> + (((struct csio_sm *)(__smp))->sm_state = (csio_sm_state_t)(__state))
>> +
>> +
>> +#define csio_post_event(__smp, __evt) \
>> + (((struct csio_sm *)(__smp))->sm_state((__smp), (uint32_t)(__evt)))
>> +
>> +#define csio_get_state(__smp) (((struct csio_sm *)(__smp))->sm_state)
>> +
>> +#define csio_match_state(__smp, __state) \
>> + (csio_get_state((__smp)) == (csio_sm_state_t)(__state))
>> +
>
> Why does any of the sm_state stuff need to be in macros..? Please
> inline all of this code.
I will change them to static inline.
>
>> +#define CSIO_ASSERT(cond) \
>> +do { \
>> + if (unlikely(!((cond)))) \
>> + BUG(); \
>> +} while (0)
>> +
>> +#ifdef __CSIO_DEBUG__
>> +#define CSIO_DB_ASSERT(__c) CSIO_ASSERT((__c))
>> +#else
>> +#define CSIO_DB_ASSERT(__c)
>> +#endif
>> +
>> +#endif /* ifndef __CSIO_DEFS_H__ */
>
>
^ permalink raw reply
* Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Timur Tabi @ 2012-08-24 18:36 UTC (permalink / raw)
To: Stephen Warren
Cc: Fleming Andy-AFLEMING, David Miller, ddaney.cavm@gmail.com,
netdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
Scott Wood
In-Reply-To: <5037C815.4020906@wwwdotorg.org>
Stephen Warren wrote:
> When translating the child node's reg property into the parent's address
> space, the parent's reg property shouldn't even be used at all; all the
> mapping is done through the ranges property.
>
> I thought the code error-checked for a missing ranges property, but I
> guess not...
I don't think 'ranges' is always necessary, because sometimes the child
nodes have a different address space that's not mapped to the parent. For
instance, I2C devices have addresses that are not mapped to the I2C
controller itself.
Anyway, thanks to Scott for helping me figure this out. I was missing a
ranges property:
fpga: board-control@3,0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
reg = <3 0 0x30>;
ranges = <0 3 0 0x30>;
This maps the child address of 0 to the parent address of 3 0. It seems
obvious now, but it was driving me crazy. We've never put child devices
under our FPGA nodes, so there was no prior use case of a 'ranges'
property in any of the localbus devices that I could learn from. Plus,
this is the first time we're probing directly on a child of a localbus device.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Scott Wood @ 2012-08-24 18:43 UTC (permalink / raw)
To: Timur Tabi
Cc: Stephen Warren, Fleming Andy-AFLEMING, David Miller,
ddaney.cavm@gmail.com, netdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org
In-Reply-To: <5037C9AF.3060903@freescale.com>
On 08/24/2012 01:36 PM, Timur Tabi wrote:
> Stephen Warren wrote:
>> When translating the child node's reg property into the parent's address
>> space, the parent's reg property shouldn't even be used at all; all the
>> mapping is done through the ranges property.
>>
>> I thought the code error-checked for a missing ranges property, but I
>> guess not...
>
> I don't think 'ranges' is always necessary, because sometimes the child
> nodes have a different address space that's not mapped to the parent. For
> instance, I2C devices have addresses that are not mapped to the I2C
> controller itself.
>
> Anyway, thanks to Scott for helping me figure this out. I was missing a
> ranges property:
>
> fpga: board-control@3,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> reg = <3 0 0x30>;
> ranges = <0 3 0 0x30>;
>
> This maps the child address of 0 to the parent address of 3 0. It seems
> obvious now, but it was driving me crazy. We've never put child devices
> under our FPGA nodes, so there was no prior use case of a 'ranges'
> property in any of the localbus devices that I could learn from. Plus,
> this is the first time we're probing directly on a child of a localbus device.
There's ep8248e.dts, not that I'd have expected you to look there. :-)
-Scott
^ permalink raw reply
* Re: [PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (headers part 1).
From: Naresh Kumar Inna @ 2012-08-24 18:45 UTC (permalink / raw)
To: Love, Robert W
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <50367309.3080306@intel.com>
On 8/23/2012 11:45 PM, Love, Robert W wrote:
> On 8/23/2012 3:27 PM, Naresh Kumar Inna wrote:
>> This patch contains the first set of the header files for csiostor driver.
>>
>> Signed-off-by: Naresh Kumar Inna <naresh@chelsio.com>
>> ---
>> drivers/scsi/csiostor/csio_defs.h | 143 ++++++
>> drivers/scsi/csiostor/csio_fcoe_proto.h | 843 +++++++++++++++++++++++++++++++
>
> I think most of these protocol definitions can be found in
> include/scsi/fc/ and possibly incluse/scsi/scsi_transport_fc.h. I think
> any protocol stuff missing should be added to the common header files.
>
> Thanks, //Rob--
Hi Rob,
Thanks for reviewing this patch. I will look into how best I can re-use
the headers in include/scsi/fc and get back if I have questions.
Regards,
Naresh.
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [patch net-next 0/3] team: couple of fixes
From: Jiri Pirko @ 2012-08-24 18:53 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, brouer@redhat.com, ogerlitz@mellanox.com
In-Reply-To: <20120824.134705.178022240623528140.davem@davemloft.net>
24. 8. 2012 v 19:47, David Miller <davem@davemloft.net>:
> From: Jiri Pirko <jiri@resnulli.us>
> Date: Thu, 23 Aug 2012 15:26:50 +0200
>
>> + vlan helper
>>
>> Jiri Pirko (3):
>> teamd: don't print warn message on -ESRCH during event send
>> vlan: add helper which can be called to se it device is used by vlan
>> team: do not allow to add VLAN challenged port when vlan is used
>
> Applied with typos corrected :-)
I owe you a beer :)
> --
> 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
* Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Stephen Warren @ 2012-08-24 18:56 UTC (permalink / raw)
To: Timur Tabi
Cc: Fleming Andy-AFLEMING, David Miller, ddaney.cavm@gmail.com,
netdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
Scott Wood
In-Reply-To: <5037C9AF.3060903@freescale.com>
On 08/24/2012 12:36 PM, Timur Tabi wrote:
> Stephen Warren wrote:
>> When translating the child node's reg property into the parent's address
>> space, the parent's reg property shouldn't even be used at all; all the
>> mapping is done through the ranges property.
>>
>> I thought the code error-checked for a missing ranges property, but I
>> guess not...
>
> I don't think 'ranges' is always necessary, because sometimes the child
> nodes have a different address space that's not mapped to the parent. For
> instance, I2C devices have addresses that are not mapped to the I2C
> controller itself.
In the I2C case, the address spaces are disjoint, so there's never any
mapping between them, so there's no need for ranges.
Any time the child address space is intended to be part of the parent's
address space, I believe ranges is supposed to be specified, perhaps
even mandatory, even if the translation is 1:1.
> Anyway, thanks to Scott for helping me figure this out. I was missing a
> ranges property:
>
> fpga: board-control@3,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
> reg = <3 0 0x30>;
> ranges = <0 3 0 0x30>;
>
> This maps the child address of 0 to the parent address of 3 0.
Yes, that looks reasonable.
^ permalink raw reply
* Re: [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
From: Naresh Kumar Inna @ 2012-08-24 19:04 UTC (permalink / raw)
To: David Miller
Cc: JBottomley@parallels.com, linux-scsi@vger.kernel.org,
Dimitrios Michailidis, netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <20120824.130406.1059160453285824849.davem@davemloft.net>
On 8/24/2012 10:34 PM, David Miller wrote:
> From: Naresh Kumar Inna <naresh@chelsio.com>
> Date: Fri, 24 Aug 2012 03:57:45 +0530
>
>> This is the initial submission of the Chelsio FCoE offload driver (csiostor)
>> to the upstream kernel. This driver currently supports FCoE offload
>> functionality over Chelsio T4-based 10Gb Converged Network Adapters.
>>
>> The following patches contain the driver sources for csiostor driver and
>> updates to firmware/hardware header files shared between csiostor and
>> cxgb4 (Chelsio T4-based NIC driver). The csiostor driver is dependent on these
>> header updates. These patches have been generated against scsi 'misc' branch.
>>
>> csiostor is a low level SCSI driver that interfaces with PCI, SCSI midlayer and
>> FC transport subsystems. This driver claims the FCoE PCIe function on the
>> Chelsio Converged Network Adapter. It relies on firmware events for slow path
>> operations like discovery, thereby offloading session management. The driver
>> programs firmware via Work Request interfaces for fast path I/O offload
>> features.
>
> You are going to have to get rid of these module parameters.
>
> That have to do with things that are in no way specific to your device,
> and therefore should be configured using generic kernel facilities.
>
> Using driver specific module parameters results in a poor user
> experience, because in order to make a configuration change the user
> has to know exactly what kind of device and driver is underneath,
> and then learn what the unique method is to make that configuration
> change.
>
> If you use a generic facility, the user only needs to learn one way to
> make a configuration change, regardless of device type and driver.
>
Hi Dave,
Thanks for reviewing. Is your comment with respect to any particular
module parameter[s] in this driver or all of them?
Regards,
Naresh.
^ permalink raw reply
* Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: David Miller @ 2012-08-24 19:07 UTC (permalink / raw)
To: swarren; +Cc: timur, afleming, ddaney.cavm, netdev, devicetree-discuss,
scottwood
In-Reply-To: <5037CE45.4010309@wwwdotorg.org>
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Fri, 24 Aug 2012 12:56:05 -0600
> In the I2C case, the address spaces are disjoint, so there's never any
> mapping between them, so there's no need for ranges.
>
> Any time the child address space is intended to be part of the parent's
> address space, I believe ranges is supposed to be specified, perhaps
> even mandatory, even if the translation is 1:1.
Regardless, you really can't just generically translate ranges
in some universal way and expect it to work in all cases.
You need bus specific drivers to deal with various special
cases.
See the *_map() methods implemented in:
arch/sparc/kernel/of_device_64.c
for example.
^ permalink raw reply
* Re: [PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 5).
From: Naresh Kumar Inna @ 2012-08-24 19:07 UTC (permalink / raw)
To: Joe Perches
Cc: Nicholas A. Bellinger, JBottomley@parallels.com,
linux-scsi@vger.kernel.org, Dimitrios Michailidis,
netdev@vger.kernel.org, Chethan Seshadri
In-Reply-To: <1345832854.32359.11.camel@joe2Laptop>
On 8/24/2012 11:57 PM, Joe Perches wrote:
> On Fri, 2012-08-24 at 23:10 +0530, Naresh Kumar Inna wrote:
>> Is there a TRUE/FALSE define in a standard header file?
>
> include/linux/stddef.h:
> enum {
> false = 0,
> true = 1
> };
>
>> I see a lot of
>> kernel code defining their own TRUE/FALSE.
>
> Those should be changed eventually.
>
Thanks! I will switch over to these macros.
^ permalink raw reply
* Re: [PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
From: David Miller @ 2012-08-24 19:10 UTC (permalink / raw)
To: naresh; +Cc: JBottomley, linux-scsi, dm, netdev, chethan
In-Reply-To: <5037D043.7000302@chelsio.com>
From: Naresh Kumar Inna <naresh@chelsio.com>
Date: Sat, 25 Aug 2012 00:34:35 +0530
> Thanks for reviewing. Is your comment with respect to any particular
> module parameter[s] in this driver or all of them?
All of them.
^ permalink raw reply
* [PATCH] [v3] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Timur Tabi @ 2012-08-24 19:10 UTC (permalink / raw)
To: Andy Fleming, David Miller, swarren-3lzwWm7+Weoh9ZMKESR00Q,
ddaney.cavm-Re5JQEeQqe8AvxtiuMwx3w,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
netdev-u79uwXL29TY76Z2rM5mHXA
Add support for an MDIO bus multiplexer controlled by a simple memory-mapped
device, like an FPGA. The device must be memory-mapped and contain only
8-bit registers (which keeps things simple).
Tested on a Freescale P5020DS board which uses the "PIXIS" FPGA attached
to the localbus.
Signed-off-by: Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
.../devicetree/bindings/net/mdio-mux-mmioreg.txt | 77 +++++++++
drivers/net/phy/Kconfig | 13 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/mdio-mux-mmioreg.c | 170 ++++++++++++++++++++
4 files changed, 261 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
create mode 100644 drivers/net/phy/mdio-mux-mmioreg.c
diff --git a/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
new file mode 100644
index 0000000..aff8fd9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio-mux-mmioreg.txt
@@ -0,0 +1,77 @@
+Properties for an MDIO bus multiplexer controlled by a memory-mapped device
+
+This is a special case of a MDIO bus multiplexer. A memory-mapped device,
+like an FPGA, is used to control which child bus is connected. The mdio-mux
+node must be a child of the memory-mapped device. The driver currently only
+supports devices with eight-bit registers.
+
+Required properties in addition to the generic multiplexer properties:
+
+- compatible : string, must contain "mdio-mux-mmioreg"
+
+- reg : integer, contains the offset of the register that controls the bus
+ multiplexer. The size field in the 'reg' property is the size of
+ register, and must therefore be 1.
+
+- mux-mask : integer, contains an eight-bit mask that specifies which
+ bits in the register control the actual bus multiplexer. The
+ 'reg' property of each child mdio-mux node must be constrained by
+ this mask.
+
+Example:
+
+The FPGA node defines a memory-mapped FPGA with a register space of 0x30 bytes.
+For the "EMI2" MDIO bus, register 9 (BRDCFG1) controls the mux on that bus.
+A bitmask of 0x6 means that bits 1 and 2 (bit 0 is lsb) are the bits on
+BRDCFG1 that control the actual mux.
+
+ /* The FPGA node */
+ fpga: board-control@3,0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "fsl,p5020ds-fpga", "fsl,fpga-ngpixis";
+ reg = <3 0 0x30>;
+ ranges = <0 3 0 0x30>;
+
+ mdio-mux-emi2 {
+ compatible = "mdio-mux-mmioreg", "mdio-mux";
+ mdio-parent-bus = <&xmdio0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <9 1>; // BRDCFG1
+ mux-mask = <0x6>; // EMI2
+
+ emi2_slot1: mdio@0 { // Slot 1 XAUI (FM2)
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy_xgmii_slot1: ethernet-phy@0 {
+ compatible = "ethernet-phy-ieee802.3-c45";
+ reg = <4>;
+ };
+ };
+
+ emi2_slot2: mdio@2 { // Slot 2 XAUI (FM1)
+ reg = <2>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ phy_xgmii_slot2: ethernet-phy@4 {
+ compatible = "ethernet-phy-ieee802.3-c45";
+ reg = <0>;
+ };
+ };
+ };
+ };
+
+ /* The parent MDIO bus. */
+ xmdio0: mdio@f1000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,fman-xmdio";
+ reg = <0xf1000 0x1000>;
+ interrupts = <100 1 0 0>;
+ };
+
+
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3090dc6..0268edd 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -159,6 +159,19 @@ config MDIO_BUS_MUX_GPIO
several child MDIO busses to a parent bus. Child bus
selection is under the control of GPIO lines.
+config MDIO_BUS_MUX_MMIOREG
+ tristate "Support for MMIO device-controlled MDIO bus multiplexers"
+ depends on OF_MDIO
+ select MDIO_BUS_MUX
+ help
+ This module provides a driver for MDIO bus multiplexers that
+ are controlled via a simple memory-mapped device, like an FPGA.
+ The multiplexer connects one of several child MDIO busses to a
+ parent bus. Child bus selection is under the control of one of
+ the FPGA's registers.
+
+ Currently, only 8-bit registers are supported.
+
endif # PHYLIB
config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 6d2dc6c..426674d 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
obj-$(CONFIG_AMD_PHY) += amd.o
obj-$(CONFIG_MDIO_BUS_MUX) += mdio-mux.o
obj-$(CONFIG_MDIO_BUS_MUX_GPIO) += mdio-mux-gpio.o
+obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o
diff --git a/drivers/net/phy/mdio-mux-mmioreg.c b/drivers/net/phy/mdio-mux-mmioreg.c
new file mode 100644
index 0000000..098239a
--- /dev/null
+++ b/drivers/net/phy/mdio-mux-mmioreg.c
@@ -0,0 +1,170 @@
+/*
+ * Simple memory-mapped device MDIO MUX driver
+ *
+ * Author: Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+ *
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/of_mdio.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/phy.h>
+#include <linux/mdio-mux.h>
+
+struct mdio_mux_mmioreg_state {
+ void *mux_handle;
+ phys_addr_t phys;
+ uint8_t mask;
+};
+
+/*
+ * MDIO multiplexing switch function
+ *
+ * This function is called by the mdio-mux layer when it thinks the mdio bus
+ * multiplexer needs to switch.
+ *
+ * 'current_child' is the current value of the mux register (masked via
+ * s->mask).
+ *
+ * 'desired_child' is the value of the 'reg' property of the target child MDIO
+ * node.
+ *
+ * The first time this function is called, current_child == -1.
+ *
+ * If current_child == desired_child, then the mux is already set to the
+ * correct bus.
+ */
+static int mdio_mux_mmioreg_switch_fn(int current_child, int desired_child,
+ void *data)
+{
+ struct mdio_mux_mmioreg_state *s = data;
+
+ if (current_child ^ desired_child) {
+ void *p = ioremap(s->phys, 1);
+ uint8_t x, y;
+
+ if (!p)
+ return -ENOMEM;
+
+ x = ioread8(p);
+ y = (x & ~s->mask) | desired_child;
+ if (x != y) {
+ iowrite8((x & ~s->mask) | desired_child, p);
+ pr_debug("%s: %02x -> %02x\n", __func__, x, y);
+ }
+
+ iounmap(p);
+ }
+
+ return 0;
+}
+
+static int __devinit mdio_mux_mmioreg_probe(struct platform_device *pdev)
+{
+ struct device_node *np2, *np = pdev->dev.of_node;
+ struct mdio_mux_mmioreg_state *s;
+ struct resource res;
+ const __be32 *iprop;
+ int len, ret;
+
+ dev_dbg(&pdev->dev, "probing node %s\n", np->full_name);
+
+ s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
+ if (!s)
+ return -ENOMEM;
+
+ ret = of_address_to_resource(np, 0, &res);
+ if (ret) {
+ dev_err(&pdev->dev, "could not obtain memory map for node %s\n",
+ np->full_name);
+ return ret;
+ }
+ s->phys = res.start;
+
+ if (resource_size(&res) != sizeof(uint8_t)) {
+ dev_err(&pdev->dev, "only 8-bit registers are supported\n");
+ return -EINVAL;
+ }
+
+ iprop = of_get_property(np, "mux-mask", &len);
+ if (!iprop || len != sizeof(uint32_t)) {
+ dev_err(&pdev->dev, "missing or invalid mux-mask property\n");
+ return -ENODEV;
+ }
+ if (be32_to_cpup(iprop) > 255) {
+ dev_err(&pdev->dev, "only 8-bit registers are supported\n");
+ return -EINVAL;
+ }
+ s->mask = be32_to_cpup(iprop);
+
+ /*
+ * Verify that the 'reg' property of each child MDIO bus does not
+ * set any bits outside of the 'mask'.
+ */
+ for_each_available_child_of_node(np, np2) {
+ iprop = of_get_property(np2, "reg", &len);
+ if (!iprop || len != sizeof(uint32_t)) {
+ dev_err(&pdev->dev, "mdio-mux child node %s is "
+ "missing a 'reg' property\n", np2->full_name);
+ return -ENODEV;
+ }
+ if (be32_to_cpup(iprop) & ~s->mask) {
+ dev_err(&pdev->dev, "mdio-mux child node %s has "
+ "a 'reg' value with unmasked bits\n",
+ np2->full_name);
+ return -ENODEV;
+ }
+ }
+
+ ret = mdio_mux_init(&pdev->dev, mdio_mux_mmioreg_switch_fn,
+ &s->mux_handle, s);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register mdio-mux bus %s\n",
+ np->full_name);
+ return ret;
+ }
+
+ pdev->dev.platform_data = s;
+
+ return 0;
+}
+
+static int __devexit mdio_mux_mmioreg_remove(struct platform_device *pdev)
+{
+ struct mdio_mux_mmioreg_state *s = dev_get_platdata(&pdev->dev);
+
+ mdio_mux_uninit(s->mux_handle);
+
+ return 0;
+}
+
+static struct of_device_id mdio_mux_mmioreg_match[] = {
+ {
+ .compatible = "mdio-mux-mmioreg",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mdio_mux_mmioreg_match);
+
+static struct platform_driver mdio_mux_mmioreg_driver = {
+ .driver = {
+ .name = "mdio-mux-mmioreg",
+ .owner = THIS_MODULE,
+ .of_match_table = mdio_mux_mmioreg_match,
+ },
+ .probe = mdio_mux_mmioreg_probe,
+ .remove = __devexit_p(mdio_mux_mmioreg_remove),
+};
+
+module_platform_driver(mdio_mux_mmioreg_driver);
+
+MODULE_AUTHOR("Timur Tabi <timur-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_DESCRIPTION("Memory-mapped device MDIO MUX driver");
+MODULE_LICENSE("GPL v2");
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH] [v2] netdev/phy: add MDIO bus multiplexer driven by a memory-mapped device
From: Scott Wood @ 2012-08-24 19:18 UTC (permalink / raw)
To: David Miller
Cc: swarren, timur, afleming, ddaney.cavm, netdev, devicetree-discuss
In-Reply-To: <20120824.150740.9047511945034908.davem@davemloft.net>
On 08/24/2012 02:07 PM, David Miller wrote:
> From: Stephen Warren <swarren@wwwdotorg.org>
> Date: Fri, 24 Aug 2012 12:56:05 -0600
>
>> In the I2C case, the address spaces are disjoint, so there's never any
>> mapping between them, so there's no need for ranges.
>>
>> Any time the child address space is intended to be part of the parent's
>> address space, I believe ranges is supposed to be specified, perhaps
>> even mandatory, even if the translation is 1:1.
Yes, it's mandatory (even if the kernel lets you get away without it for
the sake of some broken Apple firmware, IIRC). If the translation is an
identity map you can use an empty "ranges;".
> Regardless, you really can't just generically translate ranges
> in some universal way and expect it to work in all cases.
>
> You need bus specific drivers to deal with various special
> cases.
>
> See the *_map() methods implemented in:
>
> arch/sparc/kernel/of_device_64.c
>
> for example.
We don't expect it to work in all cases. We expect it to work if the
bus node is on the whitelist for which we create devices on
platform_bus, there's a platform driver that binds against it, and that
driver calls of_iomap() or equivalent because the binding says that reg
refers to something that is memory mapped.
-Scott
^ permalink raw reply
* Re: pull request: wireless 2012-08-24
From: David Miller @ 2012-08-24 19:18 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20120824161330.GC27147@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 24 Aug 2012 12:13:31 -0400
> This batch of fixes is intended for 3.6...
>
> Johannes Berg gives us a pair of iwlwifi fixes. One corrects some
> improperly defined ifdefs that lead to crashes and BUG_ONs. The other
> prevents attempts to read SRAM for devices that aren't actually started.
>
> Julia Lawall provides an ipw2100 fix to properly set the return code
> from a function call before testing it! :-)
>
> Thomas Huehn corrects the improper use of a constant related to a power
> setting in ath5k.
>
> Thomas Pedersen offers a mac80211 fix to properly handle destination
> addresses of unicast frames passing though a mesh gate.
>
> Vladimir Zapolskiy provides a brcmsmac fix to properly mark the
> interface state when the device goes down.
Pulled, thanks a lot John.
^ permalink raw reply
* Re: pull request: wireless-next 2012-08-24
From: David Miller @ 2012-08-24 19:18 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, netdev
In-Reply-To: <20120824173138.GD27147@tuxdriver.com>
From: "John W. Linville" <linville@tuxdriver.com>
Date: Fri, 24 Aug 2012 13:31:38 -0400
> This is a batch of updates intended for 3.7. The bulk of it is
> mac80211 changes, including some mesh work from Thomas Pederson and
> some multi-channel work from Johannes. A variety of driver updates
> and other bits are scattered in there as well.
Pulled, thanks!
^ permalink raw reply
* Re: pull-request: can 2012-08-24
From: David Miller @ 2012-08-24 19:21 UTC (permalink / raw)
To: mkl; +Cc: netdev, linux-can
In-Reply-To: <1345800643-29456-1-git-send-email-mkl@pengutronix.de>
From: Marc Kleine-Budde <mkl@pengutronix.de>
Date: Fri, 24 Aug 2012 11:30:41 +0200
> here are two fixes for the v3.6 release cycle. Alexey Khoroshilov submitted a
> fix for a memory leak in the softing driver (in softing_load_fw()) in case a
> krealloc() fails. Sven Schmitt fixed the misuse of the IRQF_SHARED flag in the
> irq resouce of the sja1000 platform driver, now the correct flag is used. There
> are no mainline users of this feature which need to be converted.
Pulled, thanks Marc.
^ permalink raw reply
* RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
From: Keller, Jacob E @ 2012-08-24 19:38 UTC (permalink / raw)
To: Ben Hutchings, Richard Cochran
Cc: Vick, Matthew, Kirsher, Jeffrey T, davem@davemloft.net,
netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <1345827088.2694.11.camel@bwh-desktop.uk.solarflarecom.com>
> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Friday, August 24, 2012 9:51 AM
> To: Richard Cochran
> Cc: Vick, Matthew; Keller, Jacob E; Kirsher, Jeffrey T;
> davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.
>
> On Fri, 2012-08-24 at 12:10 +0200, Richard Cochran wrote:
> > On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:
> > >
> > > I tend to agree with Jake here--I like having the information. I'm
> fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping
> if we're going to do it. What do you think, Richard?
> >
> > Come to think of it, I never liked the CONFIG_IGB_PTP very much in the
> > first place. These were added after the fact by Jeff Kirsher. He had
> > said off list that there was some issue with CONFIG_PTP_1588_CLOCK and
> > igb as a module, or something like that. At that time I said, just go
> > ahead and fix it up.
>
> If there is some feature of a driver that depends on an optional
> modularisable subsystem, and you want the driver to be built without that
> feature when the subsystem is missing, then the feature has to be disabled
> when the driver is built-in and the subsystem is a module. So in general
> you need:
>
> config DRIVER_FEATURE
> bool "This is a really neat feature for the driver"
> depends on DRIVER && SUBSYSTEM && !(DRIVER=y && SUBYSTEM=m)
>
> > I think it would be better if the "time stamp all Rx packets" of the
> > 82580 were always available, and that the PHC feature always be
> > compiled when CONFIG_PTP_1588_CLOCK is selected.
> >
> > Maybe you could ask Jeff what the issue was, and then see if there is
> > a way to remove CONFIG_IGB_PTP altogether.
>
> Even if they are to be enabled automatically, CONFIG_IGB_PTP and similar
> config symbols are important as shorthand. So I think what you want is:
>
> config IGB_PTP
> def_bool y
> depends on IGB && PTP_1588_CLOCK && !(IGB=y && PTP_1588_CLOCK=m)
>
> (But currently it's actually *selecting* PTP_1588_CLOCK. We should
> probably have drivers consistently select or depend on it, not a
> mixture.)
>
> Ben.
>
> --
> Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer;
> that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
The IGP_PTP is necessary otherwise you have to do something like #if defined(CONFIG_PTP_1588_CLOCK), or #ifdef CONFIG_PTP_1588_CLOCK \ #ifdef CONFIG_PTP_1588_CLOCK_MODULE
The main reason that I wouldn't want to un-wrap the timestamping is because the hwtstamp ioctl is somewhat problematic because it is almost all ptp only. Also, some of the parts for igb driver don't support timestamp all, they only support ptp only packets, and this would be a lot more confusing since I would say still only allow that if ptp is on.. (since those values are useless except with the PHC clock)
- Jake
^ permalink raw reply
* Re: [PATCH 14/15] netpoll: re-enable irq in poll_napi()
From: Andrew Morton @ 2012-08-24 19:43 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David Miller
In-Reply-To: <1344597891-32242-15-git-send-email-amwang@redhat.com>
On Fri, 10 Aug 2012 19:24:50 +0800
Cong Wang <amwang@redhat.com> wrote:
> napi->poll() needs IRQ enabled, so we have to re-enable IRQ before
> calling it.
>
> Cc: David Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <amwang@redhat.com>
> ---
> net/core/netpoll.c | 10 +++++++++-
> 1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index e4ba3e7..346b1eb 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -168,16 +168,24 @@ static void poll_napi(struct net_device *dev)
> struct napi_struct *napi;
> int budget = 16;
>
> + WARN_ON_ONCE(!irqs_disabled());
> +
> list_for_each_entry(napi, &dev->napi_list, dev_list) {
> + local_irq_enable();
> if (napi->poll_owner != smp_processor_id() &&
> spin_trylock(&napi->poll_lock)) {
> + rcu_read_lock_bh();
> budget = poll_one_napi(rcu_dereference_bh(dev->npinfo),
> napi, budget);
> + rcu_read_unlock_bh();
> spin_unlock(&napi->poll_lock);
>
> - if (!budget)
> + if (!budget) {
> + local_irq_disable();
> break;
> + }
> }
> + local_irq_disable();
> }
> }
This commit (6bdb7fe3104 in mainline) makes my netconsole-using x86_64
box lock up during boot. Dunno why, but I do have a cellphone:
http://ozlabs.org/~akpm/stuff/IMG_20120824_122054.jpg
^ permalink raw reply
* pull request: sfc-next 2012-08-24
From: Ben Hutchings @ 2012-08-24 19:46 UTC (permalink / raw)
To: David Miller; +Cc: linux-net-drivers, netdev
The following changes since commit 8f4cccbbd92f2ad0ddbbc498ef7cee2a1c3defe9:
net: Set device operstate at registration time (2012-08-24 12:46:13 -0400)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc-next.git for-davem
(commit 8f8b3d518999fd1c342310910aa1e49112c86d05)
1. Change the TX path to stop queues earlier and avoid returning
NETDEV_TX_BUSY.
2. Remove some inefficiencies in soft-TSO.
3. Fix various bugs involving device state transitions and/or reset
scheduling by error handlers.
4. Take advantage of my previous change to operstate initialisation.
5. Miscellaneous cleanup.
Ben.
Ben Hutchings (16):
sfc: Refactor struct efx_tx_buffer to use a flags field
sfc: Stop TX queues before they fill up
sfc: Simplify TSO header buffer allocation
sfc: Replace tso_state::full_packet_space with ip_base_len
sfc: Stash header offsets for TSO in struct tso_state
sfc: Change state names to be clearer, and comment them
sfc: Hold the RTNL lock for more of the suspend/resume cycle
sfc: Keep disabled NICs quiescent during suspend/resume
sfc: Hold RTNL lock (only) when calling efx_stop_interrupts()
sfc: Never try to stop and start a NIC that is disabled
sfc: Improve log messages in case we abort probe due to a pending reset
sfc: Fix reset vs probe/remove/PM races involving efx_nic::state
sfc: Remove overly paranoid locking assertions from netdev operations
sfc: Remove bogus comment about MTU change and RX buffer overrun
sfc: Assign efx and efx->type as early as possible in efx_pci_probe()
sfc: Fix the initial device operstate
drivers/net/ethernet/sfc/efx.c | 235 +++++++-----
drivers/net/ethernet/sfc/ethtool.c | 4 +-
drivers/net/ethernet/sfc/falcon_boards.c | 2 +-
drivers/net/ethernet/sfc/net_driver.h | 49 ++--
drivers/net/ethernet/sfc/nic.c | 6 +-
drivers/net/ethernet/sfc/tx.c | 621 ++++++++++++------------------
6 files changed, 410 insertions(+), 507 deletions(-)
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH v3 01/17] hashtable: introduce a small and naive hashtable
From: Sasha Levin @ 2012-08-24 19:47 UTC (permalink / raw)
To: Tejun Heo
Cc: torvalds, akpm, linux-kernel, linux-mm, paul.gortmaker, davem,
rostedt, mingo, ebiederm, aarcange, ericvh, netdev, josh,
eric.dumazet, mathieu.desnoyers, axboe, agk, dm-devel, neilb,
ccaulfie, teigland, Trond.Myklebust, bfields, fweisbec, jesse,
venkat.x.venkatsubra, ejt, snitzer, edumazet, linux-nfs, dev,
rds-devel, lw
In-Reply-To: <20120823200456.GD14962@google.com>
On 08/23/2012 10:04 PM, Tejun Heo wrote:
> Hello, Sasha.
>
> On Thu, Aug 23, 2012 at 02:24:32AM +0200, Sasha Levin wrote:
>>> I think the almost trivial nature of hlist hashtables makes this a bit
>>> tricky and I'm not very sure but having this combinatory explosion is
>>> a bit dazzling when the same functionality can be achieved by simply
>>> combining operations which are already defined and named considering
>>> hashtable. I'm not feeling too strong about this tho. What do others
>>> think?
>>
>> I'm thinking that this hashtable API will have 2 purposes: First, it would
>> prevent the excessive duplication of hashtable implementations all around the code.
>>
>> Second, it will allow more easily interchangeable hashtable implementations to
>> find their way into the kernel. There are several maintainers who would be happy
>> to see dynamically sized RCU hashtable, and I'm guessing that several more
>> variants could be added based on needs in specific modules.
>>
>> The second reason is why several things you've mentioned look the way they are:
>>
>> - No DEFINE_HASHTABLE(): I wanted to force the use of hash_init() since
>> initialization for other hashtables may be more complicated than the static
>> initialization for this implementation, which means that any place that used
>> DEFINE_HASHTABLE() and didn't do hash_init() will be buggy.
>
> I think this is problematic. It looks exactly like other existing
> DEFINE macros yet what its semantics is different. I don't think
> that's a good idea.
I can switch that to be DECLARE_HASHTABLE() if the issue is semantics.
>> I'm actually tempted in hiding hlist completely from hashtable users, probably
>> by simply defining a hash_head/hash_node on top of the hlist_ counterparts.
>
> I think that it would be best to keep this one simple & obvious, which
> already has enough in-kernel users to justify its existence. There
> are significant benefits in being trivially understandable and
> expectable. If we want more advanced ones - say resizing, hybrid or
> what not, let's make that a separate one. No need to complicate the
> common straight-forward case for that.
>
> So, I think it would be best to keep this one as straight-forward and
> trivial as possible. Helper macros to help its users are fine but
> let's please not go for full encapsulation.
What if we cut off the dynamic allocated (but not resizable) hashtable out for
the moment, and focus on the most common statically allocated hashtable case?
The benefits would be:
- Getting rid of all the _size() macros, which will make the amount of helpers
here reasonable.
- Dynamically allocated hashtable can be easily added as a separate
implementation using the same API. We already have some of those in the kernel...
- When that's ready, I feel it's a shame to lose full encapsulation just due to
hash_hashed().
Thanks,
Sasha
>
> Thanks.
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [PATCH net-next 01/16] sfc: Refactor struct efx_tx_buffer to use a flags field
From: Ben Hutchings @ 2012-08-24 19:47 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
Add a flags field to struct efx_tx_buffer, replacing the
continuation and map_single booleans.
Since a single descriptor cannot be both a TSO header and the last
descriptor for an skb, unionise efx_tx_buffer::{skb,tsoh} and add
flags for validity of these fields.
Clear all flags in free buffers (whereas previously the continuation
flag would be set).
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/net_driver.h | 27 ++++++-----
drivers/net/ethernet/sfc/nic.c | 4 +-
drivers/net/ethernet/sfc/tx.c | 78 +++++++++++++++------------------
3 files changed, 53 insertions(+), 56 deletions(-)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index cd9c0a9..0ac01fa 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -91,29 +91,30 @@ struct efx_special_buffer {
};
/**
- * struct efx_tx_buffer - An Efx TX buffer
- * @skb: The associated socket buffer.
- * Set only on the final fragment of a packet; %NULL for all other
- * fragments. When this fragment completes, then we can free this
- * skb.
- * @tsoh: The associated TSO header structure, or %NULL if this
- * buffer is not a TSO header.
+ * struct efx_tx_buffer - buffer state for a TX descriptor
+ * @skb: When @flags & %EFX_TX_BUF_SKB, the associated socket buffer to be
+ * freed when descriptor completes
+ * @tsoh: When @flags & %EFX_TX_BUF_TSOH, the associated TSO header structure.
* @dma_addr: DMA address of the fragment.
+ * @flags: Flags for allocation and DMA mapping type
* @len: Length of this fragment.
* This field is zero when the queue slot is empty.
- * @continuation: True if this fragment is not the end of a packet.
- * @unmap_single: True if dma_unmap_single should be used.
* @unmap_len: Length of this fragment to unmap
*/
struct efx_tx_buffer {
- const struct sk_buff *skb;
- struct efx_tso_header *tsoh;
+ union {
+ const struct sk_buff *skb;
+ struct efx_tso_header *tsoh;
+ };
dma_addr_t dma_addr;
+ unsigned short flags;
unsigned short len;
- bool continuation;
- bool unmap_single;
unsigned short unmap_len;
};
+#define EFX_TX_BUF_CONT 1 /* not last descriptor of packet */
+#define EFX_TX_BUF_SKB 2 /* buffer is last part of skb */
+#define EFX_TX_BUF_TSOH 4 /* buffer is TSO header */
+#define EFX_TX_BUF_MAP_SINGLE 8 /* buffer was mapped with dma_map_single() */
/**
* struct efx_tx_queue - An Efx TX queue
diff --git a/drivers/net/ethernet/sfc/nic.c b/drivers/net/ethernet/sfc/nic.c
index 326d799..aa11370 100644
--- a/drivers/net/ethernet/sfc/nic.c
+++ b/drivers/net/ethernet/sfc/nic.c
@@ -401,8 +401,10 @@ void efx_nic_push_buffers(struct efx_tx_queue *tx_queue)
++tx_queue->write_count;
/* Create TX descriptor ring entry */
+ BUILD_BUG_ON(EFX_TX_BUF_CONT != 1);
EFX_POPULATE_QWORD_4(*txd,
- FSF_AZ_TX_KER_CONT, buffer->continuation,
+ FSF_AZ_TX_KER_CONT,
+ buffer->flags & EFX_TX_BUF_CONT,
FSF_AZ_TX_KER_BYTE_COUNT, buffer->len,
FSF_AZ_TX_KER_BUF_REGION, 0,
FSF_AZ_TX_KER_BUF_ADDR, buffer->dma_addr);
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 1871343..24c82f3 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -39,25 +39,25 @@ static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
struct device *dma_dev = &tx_queue->efx->pci_dev->dev;
dma_addr_t unmap_addr = (buffer->dma_addr + buffer->len -
buffer->unmap_len);
- if (buffer->unmap_single)
+ if (buffer->flags & EFX_TX_BUF_MAP_SINGLE)
dma_unmap_single(dma_dev, unmap_addr, buffer->unmap_len,
DMA_TO_DEVICE);
else
dma_unmap_page(dma_dev, unmap_addr, buffer->unmap_len,
DMA_TO_DEVICE);
buffer->unmap_len = 0;
- buffer->unmap_single = false;
}
- if (buffer->skb) {
+ if (buffer->flags & EFX_TX_BUF_SKB) {
(*pkts_compl)++;
(*bytes_compl) += buffer->skb->len;
dev_kfree_skb_any((struct sk_buff *) buffer->skb);
- buffer->skb = NULL;
netif_vdbg(tx_queue->efx, tx_done, tx_queue->efx->net_dev,
"TX queue %d transmission id %x complete\n",
tx_queue->queue, tx_queue->read_count);
}
+
+ buffer->flags &= EFX_TX_BUF_TSOH;
}
/**
@@ -89,14 +89,14 @@ static void efx_tsoh_heap_free(struct efx_tx_queue *tx_queue,
static void efx_tsoh_free(struct efx_tx_queue *tx_queue,
struct efx_tx_buffer *buffer)
{
- if (buffer->tsoh) {
+ if (buffer->flags & EFX_TX_BUF_TSOH) {
if (likely(!buffer->tsoh->unmap_len)) {
buffer->tsoh->next = tx_queue->tso_headers_free;
tx_queue->tso_headers_free = buffer->tsoh;
} else {
efx_tsoh_heap_free(tx_queue, buffer->tsoh);
}
- buffer->tsoh = NULL;
+ buffer->flags &= ~EFX_TX_BUF_TSOH;
}
}
@@ -163,7 +163,7 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
unsigned int len, unmap_len = 0, fill_level, insert_ptr;
dma_addr_t dma_addr, unmap_addr = 0;
unsigned int dma_len;
- bool unmap_single;
+ unsigned short dma_flags;
int q_space, i = 0;
netdev_tx_t rc = NETDEV_TX_OK;
@@ -190,7 +190,7 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
* since this is more efficient on machines with sparse
* memory.
*/
- unmap_single = true;
+ dma_flags = EFX_TX_BUF_MAP_SINGLE;
dma_addr = dma_map_single(dma_dev, skb->data, len, PCI_DMA_TODEVICE);
/* Process all fragments */
@@ -234,10 +234,8 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
insert_ptr = tx_queue->insert_count & tx_queue->ptr_mask;
buffer = &tx_queue->buffer[insert_ptr];
efx_tsoh_free(tx_queue, buffer);
- EFX_BUG_ON_PARANOID(buffer->tsoh);
- EFX_BUG_ON_PARANOID(buffer->skb);
+ EFX_BUG_ON_PARANOID(buffer->flags);
EFX_BUG_ON_PARANOID(buffer->len);
- EFX_BUG_ON_PARANOID(!buffer->continuation);
EFX_BUG_ON_PARANOID(buffer->unmap_len);
dma_len = efx_max_tx_len(efx, dma_addr);
@@ -247,13 +245,14 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
/* Fill out per descriptor fields */
buffer->len = dma_len;
buffer->dma_addr = dma_addr;
+ buffer->flags = EFX_TX_BUF_CONT;
len -= dma_len;
dma_addr += dma_len;
++tx_queue->insert_count;
} while (len);
/* Transfer ownership of the unmapping to the final buffer */
- buffer->unmap_single = unmap_single;
+ buffer->flags = EFX_TX_BUF_CONT | dma_flags;
buffer->unmap_len = unmap_len;
unmap_len = 0;
@@ -264,14 +263,14 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
len = skb_frag_size(fragment);
i++;
/* Map for DMA */
- unmap_single = false;
+ dma_flags = 0;
dma_addr = skb_frag_dma_map(dma_dev, fragment, 0, len,
DMA_TO_DEVICE);
}
/* Transfer ownership of the skb to the final buffer */
buffer->skb = skb;
- buffer->continuation = false;
+ buffer->flags = EFX_TX_BUF_SKB | dma_flags;
netdev_tx_sent_queue(tx_queue->core_txq, skb->len);
@@ -302,7 +301,7 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
/* Free the fragment we were mid-way through pushing */
if (unmap_len) {
- if (unmap_single)
+ if (dma_flags & EFX_TX_BUF_MAP_SINGLE)
dma_unmap_single(dma_dev, unmap_addr, unmap_len,
DMA_TO_DEVICE);
else
@@ -340,7 +339,6 @@ static void efx_dequeue_buffers(struct efx_tx_queue *tx_queue,
}
efx_dequeue_buffer(tx_queue, buffer, pkts_compl, bytes_compl);
- buffer->continuation = true;
buffer->len = 0;
++tx_queue->read_count;
@@ -484,7 +482,7 @@ int efx_probe_tx_queue(struct efx_tx_queue *tx_queue)
{
struct efx_nic *efx = tx_queue->efx;
unsigned int entries;
- int i, rc;
+ int rc;
/* Create the smallest power-of-two aligned ring */
entries = max(roundup_pow_of_two(efx->txq_entries), EFX_MIN_DMAQ_SIZE);
@@ -500,8 +498,6 @@ int efx_probe_tx_queue(struct efx_tx_queue *tx_queue)
GFP_KERNEL);
if (!tx_queue->buffer)
return -ENOMEM;
- for (i = 0; i <= tx_queue->ptr_mask; ++i)
- tx_queue->buffer[i].continuation = true;
/* Allocate hardware ring */
rc = efx_nic_probe_tx(tx_queue);
@@ -546,7 +542,6 @@ void efx_release_tx_buffers(struct efx_tx_queue *tx_queue)
unsigned int pkts_compl = 0, bytes_compl = 0;
buffer = &tx_queue->buffer[tx_queue->read_count & tx_queue->ptr_mask];
efx_dequeue_buffer(tx_queue, buffer, &pkts_compl, &bytes_compl);
- buffer->continuation = true;
buffer->len = 0;
++tx_queue->read_count;
@@ -631,7 +626,7 @@ void efx_remove_tx_queue(struct efx_tx_queue *tx_queue)
* @in_len: Remaining length in current SKB fragment
* @unmap_len: Length of SKB fragment
* @unmap_addr: DMA address of SKB fragment
- * @unmap_single: DMA single vs page mapping flag
+ * @dma_flags: TX buffer flags for DMA mapping - %EFX_TX_BUF_MAP_SINGLE or 0
* @protocol: Network protocol (after any VLAN header)
* @header_len: Number of bytes of header
* @full_packet_size: Number of bytes to put in each outgoing segment
@@ -651,7 +646,7 @@ struct tso_state {
unsigned in_len;
unsigned unmap_len;
dma_addr_t unmap_addr;
- bool unmap_single;
+ unsigned short dma_flags;
__be16 protocol;
unsigned header_len;
@@ -833,9 +828,7 @@ static int efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
efx_tsoh_free(tx_queue, buffer);
EFX_BUG_ON_PARANOID(buffer->len);
EFX_BUG_ON_PARANOID(buffer->unmap_len);
- EFX_BUG_ON_PARANOID(buffer->skb);
- EFX_BUG_ON_PARANOID(!buffer->continuation);
- EFX_BUG_ON_PARANOID(buffer->tsoh);
+ EFX_BUG_ON_PARANOID(buffer->flags);
buffer->dma_addr = dma_addr;
@@ -845,7 +838,8 @@ static int efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
if (dma_len >= len)
break;
- buffer->len = dma_len; /* Don't set the other members */
+ buffer->len = dma_len;
+ buffer->flags = EFX_TX_BUF_CONT;
dma_addr += dma_len;
len -= dma_len;
}
@@ -873,12 +867,11 @@ static void efx_tso_put_header(struct efx_tx_queue *tx_queue,
efx_tsoh_free(tx_queue, buffer);
EFX_BUG_ON_PARANOID(buffer->len);
EFX_BUG_ON_PARANOID(buffer->unmap_len);
- EFX_BUG_ON_PARANOID(buffer->skb);
- EFX_BUG_ON_PARANOID(!buffer->continuation);
- EFX_BUG_ON_PARANOID(buffer->tsoh);
+ EFX_BUG_ON_PARANOID(buffer->flags);
buffer->len = len;
buffer->dma_addr = tsoh->dma_addr;
buffer->tsoh = tsoh;
+ buffer->flags = EFX_TX_BUF_TSOH | EFX_TX_BUF_CONT;
++tx_queue->insert_count;
}
@@ -896,11 +889,11 @@ static void efx_enqueue_unwind(struct efx_tx_queue *tx_queue)
buffer = &tx_queue->buffer[tx_queue->insert_count &
tx_queue->ptr_mask];
efx_tsoh_free(tx_queue, buffer);
- EFX_BUG_ON_PARANOID(buffer->skb);
+ EFX_BUG_ON_PARANOID(buffer->flags & EFX_TX_BUF_SKB);
if (buffer->unmap_len) {
unmap_addr = (buffer->dma_addr + buffer->len -
buffer->unmap_len);
- if (buffer->unmap_single)
+ if (buffer->flags & EFX_TX_BUF_MAP_SINGLE)
dma_unmap_single(&tx_queue->efx->pci_dev->dev,
unmap_addr, buffer->unmap_len,
DMA_TO_DEVICE);
@@ -911,7 +904,7 @@ static void efx_enqueue_unwind(struct efx_tx_queue *tx_queue)
buffer->unmap_len = 0;
}
buffer->len = 0;
- buffer->continuation = true;
+ buffer->flags = 0;
}
}
@@ -938,7 +931,7 @@ static void tso_start(struct tso_state *st, const struct sk_buff *skb)
st->out_len = skb->len - st->header_len;
st->unmap_len = 0;
- st->unmap_single = false;
+ st->dma_flags = 0;
}
static int tso_get_fragment(struct tso_state *st, struct efx_nic *efx,
@@ -947,7 +940,7 @@ static int tso_get_fragment(struct tso_state *st, struct efx_nic *efx,
st->unmap_addr = skb_frag_dma_map(&efx->pci_dev->dev, frag, 0,
skb_frag_size(frag), DMA_TO_DEVICE);
if (likely(!dma_mapping_error(&efx->pci_dev->dev, st->unmap_addr))) {
- st->unmap_single = false;
+ st->dma_flags = 0;
st->unmap_len = skb_frag_size(frag);
st->in_len = skb_frag_size(frag);
st->dma_addr = st->unmap_addr;
@@ -965,7 +958,7 @@ static int tso_get_head_fragment(struct tso_state *st, struct efx_nic *efx,
st->unmap_addr = dma_map_single(&efx->pci_dev->dev, skb->data + hl,
len, DMA_TO_DEVICE);
if (likely(!dma_mapping_error(&efx->pci_dev->dev, st->unmap_addr))) {
- st->unmap_single = true;
+ st->dma_flags = EFX_TX_BUF_MAP_SINGLE;
st->unmap_len = len;
st->in_len = len;
st->dma_addr = st->unmap_addr;
@@ -990,7 +983,7 @@ static int tso_fill_packet_with_fragment(struct efx_tx_queue *tx_queue,
struct tso_state *st)
{
struct efx_tx_buffer *buffer;
- int n, end_of_packet, rc;
+ int n, rc;
if (st->in_len == 0)
return 0;
@@ -1008,17 +1001,18 @@ static int tso_fill_packet_with_fragment(struct efx_tx_queue *tx_queue,
rc = efx_tx_queue_insert(tx_queue, st->dma_addr, n, &buffer);
if (likely(rc == 0)) {
- if (st->out_len == 0)
+ if (st->out_len == 0) {
/* Transfer ownership of the skb */
buffer->skb = skb;
-
- end_of_packet = st->out_len == 0 || st->packet_space == 0;
- buffer->continuation = !end_of_packet;
+ buffer->flags = EFX_TX_BUF_SKB;
+ } else if (st->packet_space != 0) {
+ buffer->flags = EFX_TX_BUF_CONT;
+ }
if (st->in_len == 0) {
/* Transfer ownership of the DMA mapping */
buffer->unmap_len = st->unmap_len;
- buffer->unmap_single = st->unmap_single;
+ buffer->flags |= st->dma_flags;
st->unmap_len = 0;
}
}
@@ -1195,7 +1189,7 @@ static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
unwind:
/* Free the DMA mapping we were in the process of writing out */
if (state.unmap_len) {
- if (state.unmap_single)
+ if (state.dma_flags & EFX_TX_BUF_MAP_SINGLE)
dma_unmap_single(&efx->pci_dev->dev, state.unmap_addr,
state.unmap_len, DMA_TO_DEVICE);
else
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 02/16] sfc: Stop TX queues before they fill up
From: Ben Hutchings @ 2012-08-24 19:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
We now have a definite upper bound on the number of descriptors per
skb; use that to stop the queue when the next packet might not fit.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 10 ++
drivers/net/ethernet/sfc/net_driver.h | 5 +
drivers/net/ethernet/sfc/tx.c | 212 +++++++++++++++------------------
3 files changed, 112 insertions(+), 115 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 65a8d49..3b3f084 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -630,6 +630,16 @@ static void efx_start_datapath(struct efx_nic *efx)
efx->rx_buffer_order = get_order(efx->rx_buffer_len +
sizeof(struct efx_rx_page_state));
+ /* We must keep at least one descriptor in a TX ring empty.
+ * We could avoid this when the queue size does not exactly
+ * match the hardware ring size, but it's not that important.
+ * Therefore we stop the queue when one more skb might fill
+ * the ring completely. We wake it when half way back to
+ * empty.
+ */
+ efx->txq_stop_thresh = efx->txq_entries - efx_tx_max_skb_descs(efx);
+ efx->txq_wake_thresh = efx->txq_stop_thresh / 2;
+
/* Initialise the channels */
efx_for_each_channel(channel, efx) {
efx_for_each_channel_tx_queue(tx_queue, channel)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 0ac01fa..28a6d62 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -665,6 +665,8 @@ struct vfdi_status;
* should be allocated for this NIC
* @rxq_entries: Size of receive queues requested by user.
* @txq_entries: Size of transmit queues requested by user.
+ * @txq_stop_thresh: TX queue fill level at or above which we stop it.
+ * @txq_wake_thresh: TX queue fill level at or below which we wake it.
* @tx_dc_base: Base qword address in SRAM of TX queue descriptor caches
* @rx_dc_base: Base qword address in SRAM of RX queue descriptor caches
* @sram_lim_qw: Qword address limit of SRAM
@@ -775,6 +777,9 @@ struct efx_nic {
unsigned rxq_entries;
unsigned txq_entries;
+ unsigned int txq_stop_thresh;
+ unsigned int txq_wake_thresh;
+
unsigned tx_dc_base;
unsigned rx_dc_base;
unsigned sram_lim_qw;
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 24c82f3..330d911 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -22,14 +22,6 @@
#include "nic.h"
#include "workarounds.h"
-/*
- * TX descriptor ring full threshold
- *
- * The tx_queue descriptor ring fill-level must fall below this value
- * before we restart the netif queue
- */
-#define EFX_TXQ_THRESHOLD(_efx) ((_efx)->txq_entries / 2u)
-
static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
struct efx_tx_buffer *buffer,
unsigned int *pkts_compl,
@@ -138,6 +130,56 @@ unsigned int efx_tx_max_skb_descs(struct efx_nic *efx)
return max_descs;
}
+/* Get partner of a TX queue, seen as part of the same net core queue */
+static struct efx_tx_queue *efx_tx_queue_partner(struct efx_tx_queue *tx_queue)
+{
+ if (tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD)
+ return tx_queue - EFX_TXQ_TYPE_OFFLOAD;
+ else
+ return tx_queue + EFX_TXQ_TYPE_OFFLOAD;
+}
+
+static void efx_tx_maybe_stop_queue(struct efx_tx_queue *txq1)
+{
+ /* We need to consider both queues that the net core sees as one */
+ struct efx_tx_queue *txq2 = efx_tx_queue_partner(txq1);
+ struct efx_nic *efx = txq1->efx;
+ unsigned int fill_level;
+
+ fill_level = max(txq1->insert_count - txq1->old_read_count,
+ txq2->insert_count - txq2->old_read_count);
+ if (likely(fill_level < efx->txq_stop_thresh))
+ return;
+
+ /* We used the stale old_read_count above, which gives us a
+ * pessimistic estimate of the fill level (which may even
+ * validly be >= efx->txq_entries). Now try again using
+ * read_count (more likely to be a cache miss).
+ *
+ * If we read read_count and then conditionally stop the
+ * queue, it is possible for the completion path to race with
+ * us and complete all outstanding descriptors in the middle,
+ * after which there will be no more completions to wake it.
+ * Therefore we stop the queue first, then read read_count
+ * (with a memory barrier to ensure the ordering), then
+ * restart the queue if the fill level turns out to be low
+ * enough.
+ */
+ netif_tx_stop_queue(txq1->core_txq);
+ smp_mb();
+ txq1->old_read_count = ACCESS_ONCE(txq1->read_count);
+ txq2->old_read_count = ACCESS_ONCE(txq2->read_count);
+
+ fill_level = max(txq1->insert_count - txq1->old_read_count,
+ txq2->insert_count - txq2->old_read_count);
+ EFX_BUG_ON_PARANOID(fill_level >= efx->txq_entries);
+ if (likely(fill_level < efx->txq_stop_thresh)) {
+ smp_mb();
+ if (likely(!efx->loopback_selftest))
+ netif_tx_start_queue(txq1->core_txq);
+ }
+}
+
/*
* Add a socket buffer to a TX queue
*
@@ -151,7 +193,7 @@ unsigned int efx_tx_max_skb_descs(struct efx_nic *efx)
* This function is split out from efx_hard_start_xmit to allow the
* loopback test to direct packets via specific TX queues.
*
- * Returns NETDEV_TX_OK or NETDEV_TX_BUSY
+ * Returns NETDEV_TX_OK.
* You must hold netif_tx_lock() to call this function.
*/
netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
@@ -160,12 +202,11 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
struct device *dma_dev = &efx->pci_dev->dev;
struct efx_tx_buffer *buffer;
skb_frag_t *fragment;
- unsigned int len, unmap_len = 0, fill_level, insert_ptr;
+ unsigned int len, unmap_len = 0, insert_ptr;
dma_addr_t dma_addr, unmap_addr = 0;
unsigned int dma_len;
unsigned short dma_flags;
- int q_space, i = 0;
- netdev_tx_t rc = NETDEV_TX_OK;
+ int i = 0;
EFX_BUG_ON_PARANOID(tx_queue->write_count != tx_queue->insert_count);
@@ -183,9 +224,6 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
return NETDEV_TX_OK;
}
- fill_level = tx_queue->insert_count - tx_queue->old_read_count;
- q_space = efx->txq_entries - 1 - fill_level;
-
/* Map for DMA. Use dma_map_single rather than dma_map_page
* since this is more efficient on machines with sparse
* memory.
@@ -205,32 +243,6 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
/* Add to TX queue, splitting across DMA boundaries */
do {
- if (unlikely(q_space-- <= 0)) {
- /* It might be that completions have
- * happened since the xmit path last
- * checked. Update the xmit path's
- * copy of read_count.
- */
- netif_tx_stop_queue(tx_queue->core_txq);
- /* This memory barrier protects the
- * change of queue state from the access
- * of read_count. */
- smp_mb();
- tx_queue->old_read_count =
- ACCESS_ONCE(tx_queue->read_count);
- fill_level = (tx_queue->insert_count
- - tx_queue->old_read_count);
- q_space = efx->txq_entries - 1 - fill_level;
- if (unlikely(q_space-- <= 0)) {
- rc = NETDEV_TX_BUSY;
- goto unwind;
- }
- smp_mb();
- if (likely(!efx->loopback_selftest))
- netif_tx_start_queue(
- tx_queue->core_txq);
- }
-
insert_ptr = tx_queue->insert_count & tx_queue->ptr_mask;
buffer = &tx_queue->buffer[insert_ptr];
efx_tsoh_free(tx_queue, buffer);
@@ -277,6 +289,8 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
/* Pass off to hardware */
efx_nic_push_buffers(tx_queue);
+ efx_tx_maybe_stop_queue(tx_queue);
+
return NETDEV_TX_OK;
dma_err:
@@ -288,7 +302,6 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
/* Mark the packet as transmitted, and free the SKB ourselves */
dev_kfree_skb_any(skb);
- unwind:
/* Work backwards until we hit the original insert pointer value */
while (tx_queue->insert_count != tx_queue->write_count) {
unsigned int pkts_compl = 0, bytes_compl = 0;
@@ -309,7 +322,7 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
DMA_TO_DEVICE);
}
- return rc;
+ return NETDEV_TX_OK;
}
/* Remove packets from the TX queue
@@ -448,6 +461,7 @@ void efx_xmit_done(struct efx_tx_queue *tx_queue, unsigned int index)
{
unsigned fill_level;
struct efx_nic *efx = tx_queue->efx;
+ struct efx_tx_queue *txq2;
unsigned int pkts_compl = 0, bytes_compl = 0;
EFX_BUG_ON_PARANOID(index > tx_queue->ptr_mask);
@@ -455,15 +469,18 @@ void efx_xmit_done(struct efx_tx_queue *tx_queue, unsigned int index)
efx_dequeue_buffers(tx_queue, index, &pkts_compl, &bytes_compl);
netdev_tx_completed_queue(tx_queue->core_txq, pkts_compl, bytes_compl);
- /* See if we need to restart the netif queue. This barrier
- * separates the update of read_count from the test of the
- * queue state. */
+ /* See if we need to restart the netif queue. This memory
+ * barrier ensures that we write read_count (inside
+ * efx_dequeue_buffers()) before reading the queue status.
+ */
smp_mb();
if (unlikely(netif_tx_queue_stopped(tx_queue->core_txq)) &&
likely(efx->port_enabled) &&
likely(netif_device_present(efx->net_dev))) {
- fill_level = tx_queue->insert_count - tx_queue->read_count;
- if (fill_level < EFX_TXQ_THRESHOLD(efx))
+ txq2 = efx_tx_queue_partner(tx_queue);
+ fill_level = max(tx_queue->insert_count - tx_queue->read_count,
+ txq2->insert_count - txq2->read_count);
+ if (fill_level <= efx->txq_wake_thresh)
netif_tx_wake_queue(tx_queue->core_txq);
}
@@ -776,47 +793,19 @@ efx_tsoh_heap_free(struct efx_tx_queue *tx_queue, struct efx_tso_header *tsoh)
* @len: Length of fragment
* @final_buffer: The final buffer inserted into the queue
*
- * Push descriptors onto the TX queue. Return 0 on success or 1 if
- * @tx_queue full.
+ * Push descriptors onto the TX queue.
*/
-static int efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
- dma_addr_t dma_addr, unsigned len,
- struct efx_tx_buffer **final_buffer)
+static void efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
+ dma_addr_t dma_addr, unsigned len,
+ struct efx_tx_buffer **final_buffer)
{
struct efx_tx_buffer *buffer;
struct efx_nic *efx = tx_queue->efx;
- unsigned dma_len, fill_level, insert_ptr;
- int q_space;
+ unsigned dma_len, insert_ptr;
EFX_BUG_ON_PARANOID(len <= 0);
- fill_level = tx_queue->insert_count - tx_queue->old_read_count;
- /* -1 as there is no way to represent all descriptors used */
- q_space = efx->txq_entries - 1 - fill_level;
-
while (1) {
- if (unlikely(q_space-- <= 0)) {
- /* It might be that completions have happened
- * since the xmit path last checked. Update
- * the xmit path's copy of read_count.
- */
- netif_tx_stop_queue(tx_queue->core_txq);
- /* This memory barrier protects the change of
- * queue state from the access of read_count. */
- smp_mb();
- tx_queue->old_read_count =
- ACCESS_ONCE(tx_queue->read_count);
- fill_level = (tx_queue->insert_count
- - tx_queue->old_read_count);
- q_space = efx->txq_entries - 1 - fill_level;
- if (unlikely(q_space-- <= 0)) {
- *final_buffer = NULL;
- return 1;
- }
- smp_mb();
- netif_tx_start_queue(tx_queue->core_txq);
- }
-
insert_ptr = tx_queue->insert_count & tx_queue->ptr_mask;
buffer = &tx_queue->buffer[insert_ptr];
++tx_queue->insert_count;
@@ -847,7 +836,6 @@ static int efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
EFX_BUG_ON_PARANOID(!len);
buffer->len = len;
*final_buffer = buffer;
- return 0;
}
@@ -975,20 +963,19 @@ static int tso_get_head_fragment(struct tso_state *st, struct efx_nic *efx,
* @st: TSO state
*
* Form descriptors for the current fragment, until we reach the end
- * of fragment or end-of-packet. Return 0 on success, 1 if not enough
- * space in @tx_queue.
+ * of fragment or end-of-packet.
*/
-static int tso_fill_packet_with_fragment(struct efx_tx_queue *tx_queue,
- const struct sk_buff *skb,
- struct tso_state *st)
+static void tso_fill_packet_with_fragment(struct efx_tx_queue *tx_queue,
+ const struct sk_buff *skb,
+ struct tso_state *st)
{
struct efx_tx_buffer *buffer;
- int n, rc;
+ int n;
if (st->in_len == 0)
- return 0;
+ return;
if (st->packet_space == 0)
- return 0;
+ return;
EFX_BUG_ON_PARANOID(st->in_len <= 0);
EFX_BUG_ON_PARANOID(st->packet_space <= 0);
@@ -999,26 +986,24 @@ static int tso_fill_packet_with_fragment(struct efx_tx_queue *tx_queue,
st->out_len -= n;
st->in_len -= n;
- rc = efx_tx_queue_insert(tx_queue, st->dma_addr, n, &buffer);
- if (likely(rc == 0)) {
- if (st->out_len == 0) {
- /* Transfer ownership of the skb */
- buffer->skb = skb;
- buffer->flags = EFX_TX_BUF_SKB;
- } else if (st->packet_space != 0) {
- buffer->flags = EFX_TX_BUF_CONT;
- }
+ efx_tx_queue_insert(tx_queue, st->dma_addr, n, &buffer);
- if (st->in_len == 0) {
- /* Transfer ownership of the DMA mapping */
- buffer->unmap_len = st->unmap_len;
- buffer->flags |= st->dma_flags;
- st->unmap_len = 0;
- }
+ if (st->out_len == 0) {
+ /* Transfer ownership of the skb */
+ buffer->skb = skb;
+ buffer->flags = EFX_TX_BUF_SKB;
+ } else if (st->packet_space != 0) {
+ buffer->flags = EFX_TX_BUF_CONT;
+ }
+
+ if (st->in_len == 0) {
+ /* Transfer ownership of the DMA mapping */
+ buffer->unmap_len = st->unmap_len;
+ buffer->flags |= st->dma_flags;
+ st->unmap_len = 0;
}
st->dma_addr += n;
- return rc;
}
@@ -1112,13 +1097,13 @@ static int tso_start_new_packet(struct efx_tx_queue *tx_queue,
*
* Add socket buffer @skb to @tx_queue, doing TSO or return != 0 if
* @skb was not enqueued. In all cases @skb is consumed. Return
- * %NETDEV_TX_OK or %NETDEV_TX_BUSY.
+ * %NETDEV_TX_OK.
*/
static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
struct sk_buff *skb)
{
struct efx_nic *efx = tx_queue->efx;
- int frag_i, rc, rc2 = NETDEV_TX_OK;
+ int frag_i, rc;
struct tso_state state;
/* Find the packet protocol and sanity-check it */
@@ -1150,11 +1135,7 @@ static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
goto mem_err;
while (1) {
- rc = tso_fill_packet_with_fragment(tx_queue, skb, &state);
- if (unlikely(rc)) {
- rc2 = NETDEV_TX_BUSY;
- goto unwind;
- }
+ tso_fill_packet_with_fragment(tx_queue, skb, &state);
/* Move onto the next fragment? */
if (state.in_len == 0) {
@@ -1178,6 +1159,8 @@ static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
/* Pass off to hardware */
efx_nic_push_buffers(tx_queue);
+ efx_tx_maybe_stop_queue(tx_queue);
+
tx_queue->tso_bursts++;
return NETDEV_TX_OK;
@@ -1186,7 +1169,6 @@ static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
"Out of memory for TSO headers, or DMA mapping error\n");
dev_kfree_skb_any(skb);
- unwind:
/* Free the DMA mapping we were in the process of writing out */
if (state.unmap_len) {
if (state.dma_flags & EFX_TX_BUF_MAP_SINGLE)
@@ -1198,7 +1180,7 @@ static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
}
efx_enqueue_unwind(tx_queue);
- return rc2;
+ return NETDEV_TX_OK;
}
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 03/16] sfc: Simplify TSO header buffer allocation
From: Ben Hutchings @ 2012-08-24 19:50 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
TSO header buffers contain a control structure immediately followed by
the packet headers, and are kept on a free list when not in use. This
complicates buffer management and tends to result in cache read misses
when we recycle such buffers (particularly if DMA-coherent memory
requires caches to be disabled).
Replace the free list with a simple mapping by descriptor index. We
know that there is always a payload descriptor between any two
descriptors with TSO header buffers, so we can allocate only one
such buffer for each two descriptors.
While we're at it, use a standard error code for allocation failure,
not -1.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/net_driver.h | 13 +-
drivers/net/ethernet/sfc/nic.c | 2 +-
drivers/net/ethernet/sfc/tx.c | 315 +++++++++++----------------------
3 files changed, 112 insertions(+), 218 deletions(-)
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 28a6d62..a4fe9a7 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -94,7 +94,8 @@ struct efx_special_buffer {
* struct efx_tx_buffer - buffer state for a TX descriptor
* @skb: When @flags & %EFX_TX_BUF_SKB, the associated socket buffer to be
* freed when descriptor completes
- * @tsoh: When @flags & %EFX_TX_BUF_TSOH, the associated TSO header structure.
+ * @heap_buf: When @flags & %EFX_TX_BUF_HEAP, the associated heap buffer to be
+ * freed when descriptor completes.
* @dma_addr: DMA address of the fragment.
* @flags: Flags for allocation and DMA mapping type
* @len: Length of this fragment.
@@ -104,7 +105,7 @@ struct efx_special_buffer {
struct efx_tx_buffer {
union {
const struct sk_buff *skb;
- struct efx_tso_header *tsoh;
+ void *heap_buf;
};
dma_addr_t dma_addr;
unsigned short flags;
@@ -113,7 +114,7 @@ struct efx_tx_buffer {
};
#define EFX_TX_BUF_CONT 1 /* not last descriptor of packet */
#define EFX_TX_BUF_SKB 2 /* buffer is last part of skb */
-#define EFX_TX_BUF_TSOH 4 /* buffer is TSO header */
+#define EFX_TX_BUF_HEAP 4 /* buffer was allocated with kmalloc() */
#define EFX_TX_BUF_MAP_SINGLE 8 /* buffer was mapped with dma_map_single() */
/**
@@ -134,6 +135,7 @@ struct efx_tx_buffer {
* @channel: The associated channel
* @core_txq: The networking core TX queue structure
* @buffer: The software buffer ring
+ * @tsoh_page: Array of pages of TSO header buffers
* @txd: The hardware descriptor ring
* @ptr_mask: The size of the ring minus 1.
* @initialised: Has hardware queue been initialised?
@@ -157,9 +159,6 @@ struct efx_tx_buffer {
* variable indicates that the queue is full. This is to
* avoid cache-line ping-pong between the xmit path and the
* completion path.
- * @tso_headers_free: A list of TSO headers allocated for this TX queue
- * that are not in use, and so available for new TSO sends. The list
- * is protected by the TX queue lock.
* @tso_bursts: Number of times TSO xmit invoked by kernel
* @tso_long_headers: Number of packets with headers too long for standard
* blocks
@@ -176,6 +175,7 @@ struct efx_tx_queue {
struct efx_channel *channel;
struct netdev_queue *core_txq;
struct efx_tx_buffer *buffer;
+ struct efx_buffer *tsoh_page;
struct efx_special_buffer txd;
unsigned int ptr_mask;
bool initialised;
@@ -188,7 +188,6 @@ struct efx_tx_queue {
unsigned int insert_count ____cacheline_aligned_in_smp;
unsigned int write_count;
unsigned int old_read_count;
- struct efx_tso_header *tso_headers_free;
unsigned int tso_bursts;
unsigned int tso_long_headers;
unsigned int tso_packets;
diff --git a/drivers/net/ethernet/sfc/nic.c b/drivers/net/ethernet/sfc/nic.c
index aa11370..cdff40b 100644
--- a/drivers/net/ethernet/sfc/nic.c
+++ b/drivers/net/ethernet/sfc/nic.c
@@ -298,7 +298,7 @@ efx_free_special_buffer(struct efx_nic *efx, struct efx_special_buffer *buffer)
/**************************************************************************
*
* Generic buffer handling
- * These buffers are used for interrupt status and MAC stats
+ * These buffers are used for interrupt status, MAC stats, etc.
*
**************************************************************************/
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 330d911..61bc0ed 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -47,51 +47,16 @@ static void efx_dequeue_buffer(struct efx_tx_queue *tx_queue,
netif_vdbg(tx_queue->efx, tx_done, tx_queue->efx->net_dev,
"TX queue %d transmission id %x complete\n",
tx_queue->queue, tx_queue->read_count);
+ } else if (buffer->flags & EFX_TX_BUF_HEAP) {
+ kfree(buffer->heap_buf);
}
- buffer->flags &= EFX_TX_BUF_TSOH;
+ buffer->len = 0;
+ buffer->flags = 0;
}
-/**
- * struct efx_tso_header - a DMA mapped buffer for packet headers
- * @next: Linked list of free ones.
- * The list is protected by the TX queue lock.
- * @dma_unmap_len: Length to unmap for an oversize buffer, or 0.
- * @dma_addr: The DMA address of the header below.
- *
- * This controls the memory used for a TSO header. Use TSOH_DATA()
- * to find the packet header data. Use TSOH_SIZE() to calculate the
- * total size required for a given packet header length. TSO headers
- * in the free list are exactly %TSOH_STD_SIZE bytes in size.
- */
-struct efx_tso_header {
- union {
- struct efx_tso_header *next;
- size_t unmap_len;
- };
- dma_addr_t dma_addr;
-};
-
static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
struct sk_buff *skb);
-static void efx_fini_tso(struct efx_tx_queue *tx_queue);
-static void efx_tsoh_heap_free(struct efx_tx_queue *tx_queue,
- struct efx_tso_header *tsoh);
-
-static void efx_tsoh_free(struct efx_tx_queue *tx_queue,
- struct efx_tx_buffer *buffer)
-{
- if (buffer->flags & EFX_TX_BUF_TSOH) {
- if (likely(!buffer->tsoh->unmap_len)) {
- buffer->tsoh->next = tx_queue->tso_headers_free;
- tx_queue->tso_headers_free = buffer->tsoh;
- } else {
- efx_tsoh_heap_free(tx_queue, buffer->tsoh);
- }
- buffer->flags &= ~EFX_TX_BUF_TSOH;
- }
-}
-
static inline unsigned
efx_max_tx_len(struct efx_nic *efx, dma_addr_t dma_addr)
@@ -245,7 +210,6 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
do {
insert_ptr = tx_queue->insert_count & tx_queue->ptr_mask;
buffer = &tx_queue->buffer[insert_ptr];
- efx_tsoh_free(tx_queue, buffer);
EFX_BUG_ON_PARANOID(buffer->flags);
EFX_BUG_ON_PARANOID(buffer->len);
EFX_BUG_ON_PARANOID(buffer->unmap_len);
@@ -309,7 +273,6 @@ netdev_tx_t efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb)
insert_ptr = tx_queue->insert_count & tx_queue->ptr_mask;
buffer = &tx_queue->buffer[insert_ptr];
efx_dequeue_buffer(tx_queue, buffer, &pkts_compl, &bytes_compl);
- buffer->len = 0;
}
/* Free the fragment we were mid-way through pushing */
@@ -352,7 +315,6 @@ static void efx_dequeue_buffers(struct efx_tx_queue *tx_queue,
}
efx_dequeue_buffer(tx_queue, buffer, pkts_compl, bytes_compl);
- buffer->len = 0;
++tx_queue->read_count;
read_ptr = tx_queue->read_count & tx_queue->ptr_mask;
@@ -495,6 +457,21 @@ void efx_xmit_done(struct efx_tx_queue *tx_queue, unsigned int index)
}
}
+/* Size of page-based TSO header buffers. Larger blocks must be
+ * allocated from the heap.
+ */
+#define TSOH_STD_SIZE 128
+#define TSOH_PER_PAGE (PAGE_SIZE / TSOH_STD_SIZE)
+
+/* At most half the descriptors in the queue at any time will refer to
+ * a TSO header buffer, since they must always be followed by a
+ * payload descriptor referring to an skb.
+ */
+static unsigned int efx_tsoh_page_count(struct efx_tx_queue *tx_queue)
+{
+ return DIV_ROUND_UP(tx_queue->ptr_mask + 1, 2 * TSOH_PER_PAGE);
+}
+
int efx_probe_tx_queue(struct efx_tx_queue *tx_queue)
{
struct efx_nic *efx = tx_queue->efx;
@@ -516,14 +493,27 @@ int efx_probe_tx_queue(struct efx_tx_queue *tx_queue)
if (!tx_queue->buffer)
return -ENOMEM;
+ if (tx_queue->queue & EFX_TXQ_TYPE_OFFLOAD) {
+ tx_queue->tsoh_page =
+ kcalloc(efx_tsoh_page_count(tx_queue),
+ sizeof(tx_queue->tsoh_page[0]), GFP_KERNEL);
+ if (!tx_queue->tsoh_page) {
+ rc = -ENOMEM;
+ goto fail1;
+ }
+ }
+
/* Allocate hardware ring */
rc = efx_nic_probe_tx(tx_queue);
if (rc)
- goto fail;
+ goto fail2;
return 0;
- fail:
+fail2:
+ kfree(tx_queue->tsoh_page);
+ tx_queue->tsoh_page = NULL;
+fail1:
kfree(tx_queue->buffer);
tx_queue->buffer = NULL;
return rc;
@@ -559,7 +549,6 @@ void efx_release_tx_buffers(struct efx_tx_queue *tx_queue)
unsigned int pkts_compl = 0, bytes_compl = 0;
buffer = &tx_queue->buffer[tx_queue->read_count & tx_queue->ptr_mask];
efx_dequeue_buffer(tx_queue, buffer, &pkts_compl, &bytes_compl);
- buffer->len = 0;
++tx_queue->read_count;
}
@@ -580,13 +569,12 @@ void efx_fini_tx_queue(struct efx_tx_queue *tx_queue)
efx_nic_fini_tx(tx_queue);
efx_release_tx_buffers(tx_queue);
-
- /* Free up TSO header cache */
- efx_fini_tso(tx_queue);
}
void efx_remove_tx_queue(struct efx_tx_queue *tx_queue)
{
+ int i;
+
if (!tx_queue->buffer)
return;
@@ -594,6 +582,14 @@ void efx_remove_tx_queue(struct efx_tx_queue *tx_queue)
"destroying TX queue %d\n", tx_queue->queue);
efx_nic_remove_tx(tx_queue);
+ if (tx_queue->tsoh_page) {
+ for (i = 0; i < efx_tsoh_page_count(tx_queue); i++)
+ efx_nic_free_buffer(tx_queue->efx,
+ &tx_queue->tsoh_page[i]);
+ kfree(tx_queue->tsoh_page);
+ tx_queue->tsoh_page = NULL;
+ }
+
kfree(tx_queue->buffer);
tx_queue->buffer = NULL;
}
@@ -616,17 +612,6 @@ void efx_remove_tx_queue(struct efx_tx_queue *tx_queue)
#define TSOH_OFFSET NET_IP_ALIGN
#endif
-#define TSOH_BUFFER(tsoh) ((u8 *)(tsoh + 1) + TSOH_OFFSET)
-
-/* Total size of struct efx_tso_header, buffer and padding */
-#define TSOH_SIZE(hdr_len) \
- (sizeof(struct efx_tso_header) + TSOH_OFFSET + hdr_len)
-
-/* Size of blocks on free list. Larger blocks must be allocated from
- * the heap.
- */
-#define TSOH_STD_SIZE 128
-
#define PTR_DIFF(p1, p2) ((u8 *)(p1) - (u8 *)(p2))
#define ETH_HDR_LEN(skb) (skb_network_header(skb) - (skb)->data)
#define SKB_TCP_OFF(skb) PTR_DIFF(tcp_hdr(skb), (skb)->data)
@@ -699,91 +684,43 @@ static __be16 efx_tso_check_protocol(struct sk_buff *skb)
return protocol;
}
-
-/*
- * Allocate a page worth of efx_tso_header structures, and string them
- * into the tx_queue->tso_headers_free linked list. Return 0 or -ENOMEM.
- */
-static int efx_tsoh_block_alloc(struct efx_tx_queue *tx_queue)
+static u8 *efx_tsoh_get_buffer(struct efx_tx_queue *tx_queue,
+ struct efx_tx_buffer *buffer, unsigned int len)
{
- struct device *dma_dev = &tx_queue->efx->pci_dev->dev;
- struct efx_tso_header *tsoh;
- dma_addr_t dma_addr;
- u8 *base_kva, *kva;
+ u8 *result;
- base_kva = dma_alloc_coherent(dma_dev, PAGE_SIZE, &dma_addr, GFP_ATOMIC);
- if (base_kva == NULL) {
- netif_err(tx_queue->efx, tx_err, tx_queue->efx->net_dev,
- "Unable to allocate page for TSO headers\n");
- return -ENOMEM;
- }
-
- /* dma_alloc_coherent() allocates pages. */
- EFX_BUG_ON_PARANOID(dma_addr & (PAGE_SIZE - 1u));
-
- for (kva = base_kva; kva < base_kva + PAGE_SIZE; kva += TSOH_STD_SIZE) {
- tsoh = (struct efx_tso_header *)kva;
- tsoh->dma_addr = dma_addr + (TSOH_BUFFER(tsoh) - base_kva);
- tsoh->next = tx_queue->tso_headers_free;
- tx_queue->tso_headers_free = tsoh;
- }
-
- return 0;
-}
-
-
-/* Free up a TSO header, and all others in the same page. */
-static void efx_tsoh_block_free(struct efx_tx_queue *tx_queue,
- struct efx_tso_header *tsoh,
- struct device *dma_dev)
-{
- struct efx_tso_header **p;
- unsigned long base_kva;
- dma_addr_t base_dma;
+ EFX_BUG_ON_PARANOID(buffer->len);
+ EFX_BUG_ON_PARANOID(buffer->flags);
+ EFX_BUG_ON_PARANOID(buffer->unmap_len);
- base_kva = (unsigned long)tsoh & PAGE_MASK;
- base_dma = tsoh->dma_addr & PAGE_MASK;
+ if (likely(len <= TSOH_STD_SIZE - TSOH_OFFSET)) {
+ unsigned index =
+ (tx_queue->insert_count & tx_queue->ptr_mask) / 2;
+ struct efx_buffer *page_buf =
+ &tx_queue->tsoh_page[index / TSOH_PER_PAGE];
+ unsigned offset =
+ TSOH_STD_SIZE * (index % TSOH_PER_PAGE) + TSOH_OFFSET;
- p = &tx_queue->tso_headers_free;
- while (*p != NULL) {
- if (((unsigned long)*p & PAGE_MASK) == base_kva)
- *p = (*p)->next;
- else
- p = &(*p)->next;
- }
+ if (unlikely(!page_buf->addr) &&
+ efx_nic_alloc_buffer(tx_queue->efx, page_buf, PAGE_SIZE))
+ return NULL;
- dma_free_coherent(dma_dev, PAGE_SIZE, (void *)base_kva, base_dma);
-}
+ result = (u8 *)page_buf->addr + offset;
+ buffer->dma_addr = page_buf->dma_addr + offset;
+ buffer->flags = EFX_TX_BUF_CONT;
+ } else {
+ tx_queue->tso_long_headers++;
-static struct efx_tso_header *
-efx_tsoh_heap_alloc(struct efx_tx_queue *tx_queue, size_t header_len)
-{
- struct efx_tso_header *tsoh;
-
- tsoh = kmalloc(TSOH_SIZE(header_len), GFP_ATOMIC | GFP_DMA);
- if (unlikely(!tsoh))
- return NULL;
-
- tsoh->dma_addr = dma_map_single(&tx_queue->efx->pci_dev->dev,
- TSOH_BUFFER(tsoh), header_len,
- DMA_TO_DEVICE);
- if (unlikely(dma_mapping_error(&tx_queue->efx->pci_dev->dev,
- tsoh->dma_addr))) {
- kfree(tsoh);
- return NULL;
+ buffer->heap_buf = kmalloc(TSOH_OFFSET + len, GFP_ATOMIC);
+ if (unlikely(!buffer->heap_buf))
+ return NULL;
+ result = (u8 *)buffer->heap_buf + TSOH_OFFSET;
+ buffer->flags = EFX_TX_BUF_CONT | EFX_TX_BUF_HEAP;
}
- tsoh->unmap_len = header_len;
- return tsoh;
-}
+ buffer->len = len;
-static void
-efx_tsoh_heap_free(struct efx_tx_queue *tx_queue, struct efx_tso_header *tsoh)
-{
- dma_unmap_single(&tx_queue->efx->pci_dev->dev,
- tsoh->dma_addr, tsoh->unmap_len,
- DMA_TO_DEVICE);
- kfree(tsoh);
+ return result;
}
/**
@@ -814,7 +751,6 @@ static void efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
tx_queue->read_count >=
efx->txq_entries);
- efx_tsoh_free(tx_queue, buffer);
EFX_BUG_ON_PARANOID(buffer->len);
EFX_BUG_ON_PARANOID(buffer->unmap_len);
EFX_BUG_ON_PARANOID(buffer->flags);
@@ -846,53 +782,42 @@ static void efx_tx_queue_insert(struct efx_tx_queue *tx_queue,
* a single fragment, and we know it doesn't cross a page boundary. It
* also allows us to not worry about end-of-packet etc.
*/
-static void efx_tso_put_header(struct efx_tx_queue *tx_queue,
- struct efx_tso_header *tsoh, unsigned len)
+static int efx_tso_put_header(struct efx_tx_queue *tx_queue,
+ struct efx_tx_buffer *buffer, u8 *header)
{
- struct efx_tx_buffer *buffer;
-
- buffer = &tx_queue->buffer[tx_queue->insert_count & tx_queue->ptr_mask];
- efx_tsoh_free(tx_queue, buffer);
- EFX_BUG_ON_PARANOID(buffer->len);
- EFX_BUG_ON_PARANOID(buffer->unmap_len);
- EFX_BUG_ON_PARANOID(buffer->flags);
- buffer->len = len;
- buffer->dma_addr = tsoh->dma_addr;
- buffer->tsoh = tsoh;
- buffer->flags = EFX_TX_BUF_TSOH | EFX_TX_BUF_CONT;
+ if (unlikely(buffer->flags & EFX_TX_BUF_HEAP)) {
+ buffer->dma_addr = dma_map_single(&tx_queue->efx->pci_dev->dev,
+ header, buffer->len,
+ DMA_TO_DEVICE);
+ if (unlikely(dma_mapping_error(&tx_queue->efx->pci_dev->dev,
+ buffer->dma_addr))) {
+ kfree(buffer->heap_buf);
+ buffer->len = 0;
+ buffer->flags = 0;
+ return -ENOMEM;
+ }
+ buffer->unmap_len = buffer->len;
+ buffer->flags |= EFX_TX_BUF_MAP_SINGLE;
+ }
++tx_queue->insert_count;
+ return 0;
}
-/* Remove descriptors put into a tx_queue. */
+/* Remove buffers put into a tx_queue. None of the buffers must have
+ * an skb attached.
+ */
static void efx_enqueue_unwind(struct efx_tx_queue *tx_queue)
{
struct efx_tx_buffer *buffer;
- dma_addr_t unmap_addr;
/* Work backwards until we hit the original insert pointer value */
while (tx_queue->insert_count != tx_queue->write_count) {
--tx_queue->insert_count;
buffer = &tx_queue->buffer[tx_queue->insert_count &
tx_queue->ptr_mask];
- efx_tsoh_free(tx_queue, buffer);
- EFX_BUG_ON_PARANOID(buffer->flags & EFX_TX_BUF_SKB);
- if (buffer->unmap_len) {
- unmap_addr = (buffer->dma_addr + buffer->len -
- buffer->unmap_len);
- if (buffer->flags & EFX_TX_BUF_MAP_SINGLE)
- dma_unmap_single(&tx_queue->efx->pci_dev->dev,
- unmap_addr, buffer->unmap_len,
- DMA_TO_DEVICE);
- else
- dma_unmap_page(&tx_queue->efx->pci_dev->dev,
- unmap_addr, buffer->unmap_len,
- DMA_TO_DEVICE);
- buffer->unmap_len = 0;
- }
- buffer->len = 0;
- buffer->flags = 0;
+ efx_dequeue_buffer(tx_queue, buffer, NULL, NULL);
}
}
@@ -1014,35 +939,24 @@ static void tso_fill_packet_with_fragment(struct efx_tx_queue *tx_queue,
* @st: TSO state
*
* Generate a new header and prepare for the new packet. Return 0 on
- * success, or -1 if failed to alloc header.
+ * success, or -%ENOMEM if failed to alloc header.
*/
static int tso_start_new_packet(struct efx_tx_queue *tx_queue,
const struct sk_buff *skb,
struct tso_state *st)
{
- struct efx_tso_header *tsoh;
+ struct efx_tx_buffer *buffer =
+ &tx_queue->buffer[tx_queue->insert_count & tx_queue->ptr_mask];
struct tcphdr *tsoh_th;
unsigned ip_length;
u8 *header;
+ int rc;
- /* Allocate a DMA-mapped header buffer. */
- if (likely(TSOH_SIZE(st->header_len) <= TSOH_STD_SIZE)) {
- if (tx_queue->tso_headers_free == NULL) {
- if (efx_tsoh_block_alloc(tx_queue))
- return -1;
- }
- EFX_BUG_ON_PARANOID(!tx_queue->tso_headers_free);
- tsoh = tx_queue->tso_headers_free;
- tx_queue->tso_headers_free = tsoh->next;
- tsoh->unmap_len = 0;
- } else {
- tx_queue->tso_long_headers++;
- tsoh = efx_tsoh_heap_alloc(tx_queue, st->header_len);
- if (unlikely(!tsoh))
- return -1;
- }
+ /* Allocate and insert a DMA-mapped header buffer. */
+ header = efx_tsoh_get_buffer(tx_queue, buffer, st->header_len);
+ if (!header)
+ return -ENOMEM;
- header = TSOH_BUFFER(tsoh);
tsoh_th = (struct tcphdr *)(header + SKB_TCP_OFF(skb));
/* Copy and update the headers. */
@@ -1078,12 +992,13 @@ static int tso_start_new_packet(struct efx_tx_queue *tx_queue,
tsoh_iph->payload_len = htons(ip_length - sizeof(*tsoh_iph));
}
+ rc = efx_tso_put_header(tx_queue, buffer, header);
+ if (unlikely(rc))
+ return rc;
+
st->packet_space = skb_shinfo(skb)->gso_size;
++tx_queue->tso_packets;
- /* Form a descriptor for this header. */
- efx_tso_put_header(tx_queue, tsoh, st->header_len);
-
return 0;
}
@@ -1182,23 +1097,3 @@ static int efx_enqueue_skb_tso(struct efx_tx_queue *tx_queue,
efx_enqueue_unwind(tx_queue);
return NETDEV_TX_OK;
}
-
-
-/*
- * Free up all TSO datastructures associated with tx_queue. This
- * routine should be called only once the tx_queue is both empty and
- * will no longer be used.
- */
-static void efx_fini_tso(struct efx_tx_queue *tx_queue)
-{
- unsigned i;
-
- if (tx_queue->buffer) {
- for (i = 0; i <= tx_queue->ptr_mask; ++i)
- efx_tsoh_free(tx_queue, &tx_queue->buffer[i]);
- }
-
- while (tx_queue->tso_headers_free != NULL)
- efx_tsoh_block_free(tx_queue, tx_queue->tso_headers_free,
- &tx_queue->efx->pci_dev->dev);
-}
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next 04/16] sfc: Replace tso_state::full_packet_space with ip_base_len
From: Ben Hutchings @ 2012-08-24 19:50 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1345837574.2694.22.camel@bwh-desktop.uk.solarflarecom.com>
We only use tso_state::full_packet_space to calculate the IPv4 tot_len
or IPv6 payload_len, not to set tso_state::packet_space. Replace it
with an ip_base_len field holding the value of tot_len or payload_len
before including the TCP payload, which is much more useful when
constructing the new headers.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/tx.c | 20 +++++++++++---------
1 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 61bc0ed..d9dbebc 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -631,7 +631,7 @@ void efx_remove_tx_queue(struct efx_tx_queue *tx_queue)
* @dma_flags: TX buffer flags for DMA mapping - %EFX_TX_BUF_MAP_SINGLE or 0
* @protocol: Network protocol (after any VLAN header)
* @header_len: Number of bytes of header
- * @full_packet_size: Number of bytes to put in each outgoing segment
+ * @ip_base_len: IPv4 tot_len or IPv6 payload_len, before TCP payload
*
* The state used during segmentation. It is put into this data structure
* just to make it easy to pass into inline functions.
@@ -652,7 +652,7 @@ struct tso_state {
__be16 protocol;
unsigned header_len;
- int full_packet_size;
+ unsigned int ip_base_len;
};
@@ -830,12 +830,14 @@ static void tso_start(struct tso_state *st, const struct sk_buff *skb)
*/
st->header_len = ((tcp_hdr(skb)->doff << 2u)
+ PTR_DIFF(tcp_hdr(skb), skb->data));
- st->full_packet_size = st->header_len + skb_shinfo(skb)->gso_size;
- if (st->protocol == htons(ETH_P_IP))
+ if (st->protocol == htons(ETH_P_IP)) {
+ st->ip_base_len = st->header_len - ETH_HDR_LEN(skb);
st->ipv4_id = ntohs(ip_hdr(skb)->id);
- else
+ } else {
+ st->ip_base_len = tcp_hdr(skb)->doff << 2u;
st->ipv4_id = 0;
+ }
st->seqnum = ntohl(tcp_hdr(skb)->seq);
EFX_BUG_ON_PARANOID(tcp_hdr(skb)->urg);
@@ -966,15 +968,16 @@ static int tso_start_new_packet(struct efx_tx_queue *tx_queue,
st->seqnum += skb_shinfo(skb)->gso_size;
if (st->out_len > skb_shinfo(skb)->gso_size) {
/* This packet will not finish the TSO burst. */
- ip_length = st->full_packet_size - ETH_HDR_LEN(skb);
+ st->packet_space = skb_shinfo(skb)->gso_size;
tsoh_th->fin = 0;
tsoh_th->psh = 0;
} else {
/* This packet will be the last in the TSO burst. */
- ip_length = st->header_len - ETH_HDR_LEN(skb) + st->out_len;
+ st->packet_space = st->out_len;
tsoh_th->fin = tcp_hdr(skb)->fin;
tsoh_th->psh = tcp_hdr(skb)->psh;
}
+ ip_length = st->ip_base_len + st->packet_space;
if (st->protocol == htons(ETH_P_IP)) {
struct iphdr *tsoh_iph =
@@ -989,14 +992,13 @@ static int tso_start_new_packet(struct efx_tx_queue *tx_queue,
struct ipv6hdr *tsoh_iph =
(struct ipv6hdr *)(header + SKB_IPV6_OFF(skb));
- tsoh_iph->payload_len = htons(ip_length - sizeof(*tsoh_iph));
+ tsoh_iph->payload_len = htons(ip_length);
}
rc = efx_tso_put_header(tx_queue, buffer, header);
if (unlikely(rc))
return rc;
- st->packet_space = skb_shinfo(skb)->gso_size;
++tx_queue->tso_packets;
return 0;
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox