* (unknown)
From: Loan Company Ltd @ 2013-02-17 12:47 UTC (permalink / raw)
DO YOU NEED A LEGIT LOAN OF 4%? E-MAIL US WITH FULL DETAILS:1.Name:2.Age:3.Phone Number:4.Country:5.Loan Amount.6.Duration.7.Sex
^ permalink raw reply
* Re: tuntap: Overload handling
From: Michael S. Tsirkin @ 2013-02-17 13:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Sebastian Pöhn, netdev
In-Reply-To: <1360861290.6884.55.camel@edumazet-glaptop>
On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
>
> > Hmm so ~1000 packets in the tun queue is not enough?
> > You always have the option to increase it some more ...
> >
> > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > (tun: only queue packets on device)
> > >
> >
> > Eric in the past you said the following things
> > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > In your case I would just not use qdisc at all, like other virtual
> > > > devices.
> > ...
> > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > be always empty. Whats the point storing more than 500 packets for a
> > > > device ? Thats a latency killer.
> > you don't think this applies, anymore?
> >
>
> Users have the choice to setup a qdisc or not.
>
> Having no qdisc can help raw performance, at the expense of bufferbloat.
> Thats all I was saying.
>
> It seems tun.c has no longer the possibility to effectively use a qdisc,
> (allowing the queue to buildup at qdisc layer)
>
But, userspace is in no position to decide whether using
the qdisc is a good or a bad thing.
The issue I tried to solve is that with tun, it's trivially easy for
userspace to lock up resources forever.
Simply not stopping the qdisc is probably the simplest solution.
An alternative is to orphan the skbs before we queue them.
At some point I posted a proposal doing exactly this
subj of "net: orphan queued skbs if device tx can stall".
Do you think it's worth revisiting this?
Also - does anyone know of a testcase showing there's a problem
with the simplest solution we now have in place?
--
MST
^ permalink raw reply
* [PATCH v2 2/2] net: fec: Improve logging
From: Fabio Estevam @ 2013-02-17 13:36 UTC (permalink / raw)
To: davem; +Cc: s.hauer, shawn.guo, marex, netdev, Fabio Estevam
In-Reply-To: <1361108204-9498-1-git-send-email-festevam@gmail.com>
From: Fabio Estevam <fabio.estevam@freescale.com>
Replace pr_debug with dev_dbg to improve the logging style.
dev_dbg indicates the device name in the log, which is useful for debugging
purposes when we have a dual FEC system, such as mx28.
Also use "phy-reset-gpios" in the message to correlate with the device tree
property.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Use dev_dbg instead of dev_err
drivers/net/ethernet/freescale/fec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 5864a67..29d82cf 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1695,7 +1695,7 @@ static void fec_reset_phy(struct platform_device *pdev)
err = devm_gpio_request_one(&pdev->dev, phy_reset,
GPIOF_OUT_INIT_LOW, "phy-reset");
if (err) {
- pr_debug("FEC: failed to get gpio phy-reset: %d\n", err);
+ dev_dbg(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
return;
}
msleep(msec);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 2/2] net: fec: Improve logging
From: Fabio Estevam @ 2013-02-17 13:41 UTC (permalink / raw)
To: Shawn Guo; +Cc: davem, s.hauer, marex, netdev, Fabio Estevam
In-Reply-To: <20130217115555.GA6782@S2101-09.ap.freescale.net>
On Sun, Feb 17, 2013 at 8:55 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>> --- a/drivers/net/ethernet/freescale/fec.c
>> +++ b/drivers/net/ethernet/freescale/fec.c
>> @@ -1695,7 +1695,7 @@ static void fec_reset_phy(struct platform_device *pdev)
>> err = devm_gpio_request_one(&pdev->dev, phy_reset,
>> GPIOF_OUT_INIT_LOW, "phy-reset");
>> if (err) {
>> - pr_debug("FEC: failed to get gpio phy-reset: %d\n", err);
>> + dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
>
> Shouldn't dev_dbg be more like a equivalent of pr_debug? The reason
> why it's taken as a debug message rather than an error is that some
> board design may not have reset line for phy.
If a board does not have a reset line for the PHY then it will not try
to request the GPIO as per patch 1/2 of this series.
I sent a v2 with dev_dbg, but I think that the first version with
dev_err is more useful.
^ permalink raw reply
* Re: [net-next 13/15] ixgbe: implement SFF diagnostic monitoring via ethtool
From: Michał Mirosław @ 2013-02-17 13:41 UTC (permalink / raw)
To: Tantilov, Emil S
Cc: Kirsher, Jeffrey T, davem@davemloft.net, Aurélien Guillaume,
netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com
In-Reply-To: <87618083B2453E4A8714035B62D67992337B5051@FMSMSX102.amr.corp.intel.com>
2013/2/17 Tantilov, Emil S <emil.s.tantilov@intel.com>:
>>> @@ -2839,6 +2840,117 @@ static int ixgbe_set_channels(struct net_device
>>*dev,
>>> return ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
>>> }
>>>
>>> +static int ixgbe_get_module_info(struct net_device *dev,
>>> + struct ethtool_modinfo *modinfo)
>>> +{
>>> + struct ixgbe_adapter *adapter = netdev_priv(dev);
>>> + struct ixgbe_hw *hw = &adapter->hw;
>>> + u32 status;
>>> + u8 sff8472_rev, addr_mode;
>>> + int ret_val = 0;
>>> + bool page_swap = false;
>>> +
>>> + /* avoid concurent i2c reads */
>>> + while (test_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
>>> + msleep(100);
>>> +
>>> + /* used by the service task */
>>> + set_bit(__IXGBE_READ_I2C, &adapter->state);
>>
>>This is racy. Why do you need another bit?
>
> The I2C bit helps to reduce the delay in the service task relative to the initialization of the SFP modules.
>
>>
>> while (test_and_set_bit(__IXGBE_IN_SFP_INIT, &adapter->state))
>> msleep(100);
>>...
>> clear_bit(__IXGBE_IN_SFP_INIT, &adapter->state)
>
> This is what I had initially, but the i2c reads can take a long time on some parts and __IXGBE_IN_SFP_INIT protects portions of the code that have nothing to do with I2C reads. Setting __IXGBE_IN_SFP_INIT in ethtool while dumping the SFF data can introduce needlessly long delays in the SFP initialization path.
Maybe it would be enough to protect the body of read_i2c_eeprom()
usign a mutex? It seems you want __IXGBE_READ_I2C to work like a
mutex, but you test it in non-atomic way.
Best Regards,
Michał Mirosław
^ permalink raw reply
* [PATCH v2 1/2] net: fec: Do a sanity check on the gpio number
From: Fabio Estevam @ 2013-02-17 13:36 UTC (permalink / raw)
To: davem; +Cc: s.hauer, shawn.guo, marex, netdev, Fabio Estevam
From: Fabio Estevam <fabio.estevam@freescale.com>
Since commit 372e722ea4d (gpiolib: use descriptors internally) the following
warning is seen on a mx28evk board:
[ 5.116291] ------------[ cut here ]------------
[ 5.121306] WARNING: at drivers/gpio/gpiolib.c:125 gpio_to_desc+0x30/0x44()
[ 5.128491] invalid GPIO -2
[ 5.131563] Modules linked in:
[ 5.134846] [<c0014e20>] (unwind_backtrace+0x0/0xf4) from [<c001d428>] (warn_slowpath_common+0x4c/0x68)
[ 5.144682] [<c001d428>] (warn_slowpath_common+0x4c/0x68) from [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40)
[ 5.154693] [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40) from [<c0283434>] (gpio_to_desc+0x30/0x44)
[ 5.164002] [<c0283434>] (gpio_to_desc+0x30/0x44) from [<c0285470>] (gpio_request_one+0x10/0xe8)
[ 5.173294] [<c0285470>] (gpio_request_one+0x10/0xe8) from [<c0282f50>] (devm_gpio_request_one+0x40/0x74)
[ 5.183332] [<c0282f50>] (devm_gpio_request_one+0x40/0x74) from [<c0319be0>] (fec_probe+0x2d0/0x99c)
[ 5.192923] [<c0319be0>] (fec_probe+0x2d0/0x99c) from [<c02c8114>] (platform_drv_probe+0x14/0x18)
[ 5.202228] [<c02c8114>] (platform_drv_probe+0x14/0x18) from [<c02c6e00>] (driver_probe_device+0x90/0x224)
[ 5.212332] [<c02c6e00>] (driver_probe_device+0x90/0x224) from [<c02c7028>] (__driver_attach+0x94/0x98)
[ 5.222162] [<c02c7028>] (__driver_attach+0x94/0x98) from [<c02c5750>] (bus_for_each_dev+0x78/0x98)
[ 5.231642] [<c02c5750>] (bus_for_each_dev+0x78/0x98) from [<c02c5fd0>] (bus_add_driver+0x1a4/0x240)
[ 5.241207] [<c02c5fd0>] (bus_add_driver+0x1a4/0x240) from [<c02c7608>] (driver_register+0x78/0x140)
[ 5.250768] [<c02c7608>] (driver_register+0x78/0x140) from [<c00087a4>] (do_one_initcall+0x30/0x17c)
[ 5.260347] [<c00087a4>] (do_one_initcall+0x30/0x17c) from [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0)
[ 5.270381] [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0) from [<c044dfd4>] (kernel_init+0x8/0xe4)
[ 5.279886] [<c044dfd4>] (kernel_init+0x8/0xe4) from [<c000f248>] (ret_from_fork+0x14/0x2c)
[ 5.288740] ---[ end trace c15c72a22979d58d ]---
mx28evk has two ethernet controllers. The GPIO that performs the
ethernet reset on both ports is the same GPIO, so on the board dts file, only in
one ethernet instance is passed the GPIO reset property.
Validate the gpio number prior to requesting it in order to avoid such warning.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v1:
- Added Shawn's ack
drivers/net/ethernet/freescale/fec.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 0fe68c4..5864a67 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1689,6 +1689,9 @@ static void fec_reset_phy(struct platform_device *pdev)
msec = 1;
phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+ if (!gpio_is_valid(phy_reset))
+ return;
+
err = devm_gpio_request_one(&pdev->dev, phy_reset,
GPIOF_OUT_INIT_LOW, "phy-reset");
if (err) {
--
1.7.9.5
^ permalink raw reply related
* Re: Universal tap device
From: Arnd Bergmann @ 2013-02-17 13:58 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jan Engelhardt, netdev, ebiederm, stephen, eric.dumazet, davem
In-Reply-To: <20130217093520.GA1931@minipsycho.orion>
On Sunday 17 February 2013, Jiri Pirko wrote:
>
> Sat, Feb 16, 2013 at 11:25:06PM CET, arnd@arndb.de wrote:
> >On Saturday 16 February 2013, Jiri Pirko wrote:
> >> Sat, Feb 16, 2013 at 05:46:25PM CET, jengelh@inai.de wrote:
> >> >On Saturday 2013-02-16 15:01, Jiri Pirko wrote:
> >> >
> >> >>Hi all.
> >> >>
> >> >>Looking at macvtap and thinking about the tap solutions in general,
> >> >>I think it would be handly to have some universal tap device.
> >> >
> >> >As in, allowing tuntap to register as an rx_handler for arbitrary
> >> >devices?
> >>
> >> Essentially yes, but I was thinking without the actual netdev:
> >> not eth0 -> tap0 -> chardev
> >> but eth0 -> chardev
> >>
> >> Not sure though if that is easily doable..
> >
> >Isn't that the same as setting up macvtap in PASSTHRU mode?
>
> Yep, but without netdev instance. That brings some issues. Maybe the
> netdev instance should be there. Not sure.
>
The idea of macvtap is at least that there should be no run-time
from using a separate netdev. Having the device is useful mostly
for configuration, especially with the non-passthrough modes
where you want to bind to a new mac address.
Arnd
^ permalink raw reply
* Re: [PATCH] ethernet: neterion: vxge: vxge-traffic.c: fix for a potential NULL pointer dereference
From: walter harms @ 2013-02-17 14:49 UTC (permalink / raw)
To: Kumar Amit Mehta
Cc: jdmason, davem, joe, netdev, linux-kernel, kernel-janitors
In-Reply-To: <1361037394-14731-1-git-send-email-gmate.amit@gmail.com>
Am 16.02.2013 18:56, schrieb Kumar Amit Mehta:
> fix for a potential NULL pointer dereference and removal of a redundant
> assignment operation. Found using smatch.
>
> Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
> ---
> drivers/net/ethernet/neterion/vxge/vxge-traffic.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/neterion/vxge/vxge-traffic.c b/drivers/net/ethernet/neterion/vxge/vxge-traffic.c
> index 99749bd..adb05a8 100644
> --- a/drivers/net/ethernet/neterion/vxge/vxge-traffic.c
> +++ b/drivers/net/ethernet/neterion/vxge/vxge-traffic.c
> @@ -611,11 +611,8 @@ __vxge_hw_vpath_alarm_process(struct __vxge_hw_virtualpath *vpath,
> struct vxge_hw_vpath_stats_sw_info *sw_stats;
> struct vxge_hw_vpath_reg __iomem *vp_reg;
>
> - if (vpath == NULL) {
> - alarm_event = VXGE_HW_SET_LEVEL(VXGE_HW_EVENT_UNKNOWN,
> - alarm_event);
> + if (vpath == NULL)
> goto out2;
> - }
>
> hldev = vpath->hldev;
> vp_reg = vpath->vp_reg;
> @@ -852,13 +849,12 @@ __vxge_hw_vpath_alarm_process(struct __vxge_hw_virtualpath *vpath,
> }
> out:
> hldev->stats.sw_dev_err_stats.vpath_alarms++;
> + __vxge_hw_device_handle_error(hldev, vpath->vp_id, alarm_event);
> out2:
> if ((alarm_event == VXGE_HW_EVENT_ALARM_CLEARED) ||
> (alarm_event == VXGE_HW_EVENT_UNKNOWN))
> return VXGE_HW_OK;
>
> - __vxge_hw_device_handle_error(hldev, vpath->vp_id, alarm_event);
> -
> if (alarm_event == VXGE_HW_EVENT_SERR)
> return VXGE_HW_ERR_CRITICAL;
>
the patch looks ok.
it would be nice if someone would rewrite the if (alarm_event == x ) stuff
using switch or more if()'s it is hardly readable this way.
just my 2 cents,
re,
wh
^ permalink raw reply
* Re: [PATCH 2/2] net: fec: Improve logging
From: Shawn Guo @ 2013-02-17 15:08 UTC (permalink / raw)
To: Fabio Estevam; +Cc: davem, s.hauer, marex, netdev, Fabio Estevam
In-Reply-To: <CAOMZO5AT4=cYrQBG0Or7PDw18hrYYuNyg17Bt-TU2jNSg5GXWg@mail.gmail.com>
On Sun, Feb 17, 2013 at 10:41:11AM -0300, Fabio Estevam wrote:
> On Sun, Feb 17, 2013 at 8:55 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
>
> >> --- a/drivers/net/ethernet/freescale/fec.c
> >> +++ b/drivers/net/ethernet/freescale/fec.c
> >> @@ -1695,7 +1695,7 @@ static void fec_reset_phy(struct platform_device *pdev)
> >> err = devm_gpio_request_one(&pdev->dev, phy_reset,
> >> GPIOF_OUT_INIT_LOW, "phy-reset");
> >> if (err) {
> >> - pr_debug("FEC: failed to get gpio phy-reset: %d\n", err);
> >> + dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
> >
> > Shouldn't dev_dbg be more like a equivalent of pr_debug? The reason
> > why it's taken as a debug message rather than an error is that some
> > board design may not have reset line for phy.
>
> If a board does not have a reset line for the PHY then it will not try
> to request the GPIO as per patch 1/2 of this series.
Argh, yes, you're right. The pr_debug makes no sense any more with the
first patch in place, and it should become a error message. But they
should be one patch rather than a series to make the most sense, IMO.
Shawn
^ permalink raw reply
* [PATCH v3] net: fec: Do a sanity check on the gpio number
From: Fabio Estevam @ 2013-02-17 15:29 UTC (permalink / raw)
To: davem; +Cc: s.hauer, shawn.guo, marex, netdev, Fabio Estevam
From: Fabio Estevam <fabio.estevam@freescale.com>
Since commit 372e722ea4d (gpiolib: use descriptors internally) the following
warning is seen on a mx28evk board:
[ 5.116291] ------------[ cut here ]------------
[ 5.121306] WARNING: at drivers/gpio/gpiolib.c:125 gpio_to_desc+0x30/0x44()
[ 5.128491] invalid GPIO -2
[ 5.131563] Modules linked in:
[ 5.134846] [<c0014e20>] (unwind_backtrace+0x0/0xf4) from [<c001d428>] (warn_slowpath_common+0x4c/0x68)
[ 5.144682] [<c001d428>] (warn_slowpath_common+0x4c/0x68) from [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40)
[ 5.154693] [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40) from [<c0283434>] (gpio_to_desc+0x30/0x44)
[ 5.164002] [<c0283434>] (gpio_to_desc+0x30/0x44) from [<c0285470>] (gpio_request_one+0x10/0xe8)
[ 5.173294] [<c0285470>] (gpio_request_one+0x10/0xe8) from [<c0282f50>] (devm_gpio_request_one+0x40/0x74)
[ 5.183332] [<c0282f50>] (devm_gpio_request_one+0x40/0x74) from [<c0319be0>] (fec_probe+0x2d0/0x99c)
[ 5.192923] [<c0319be0>] (fec_probe+0x2d0/0x99c) from [<c02c8114>] (platform_drv_probe+0x14/0x18)
[ 5.202228] [<c02c8114>] (platform_drv_probe+0x14/0x18) from [<c02c6e00>] (driver_probe_device+0x90/0x224)
[ 5.212332] [<c02c6e00>] (driver_probe_device+0x90/0x224) from [<c02c7028>] (__driver_attach+0x94/0x98)
[ 5.222162] [<c02c7028>] (__driver_attach+0x94/0x98) from [<c02c5750>] (bus_for_each_dev+0x78/0x98)
[ 5.231642] [<c02c5750>] (bus_for_each_dev+0x78/0x98) from [<c02c5fd0>] (bus_add_driver+0x1a4/0x240)
[ 5.241207] [<c02c5fd0>] (bus_add_driver+0x1a4/0x240) from [<c02c7608>] (driver_register+0x78/0x140)
[ 5.250768] [<c02c7608>] (driver_register+0x78/0x140) from [<c00087a4>] (do_one_initcall+0x30/0x17c)
[ 5.260347] [<c00087a4>] (do_one_initcall+0x30/0x17c) from [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0)
[ 5.270381] [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0) from [<c044dfd4>] (kernel_init+0x8/0xe4)
[ 5.279886] [<c044dfd4>] (kernel_init+0x8/0xe4) from [<c000f248>] (ret_from_fork+0x14/0x2c)
[ 5.288740] ---[ end trace c15c72a22979d58d ]---
mx28evk has two ethernet controllers. The GPIO that performs the
ethernet reset on both ports is the same GPIO, so on the board dts file, only in
one ethernet instance is passed the GPIO reset property.
Validate the gpio number prior to requesting it in order to avoid such warning.
With such gpio validation in place, it is also safe to change from pr_debug to
dev_err in the case the gpio request fails.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v2:
- Merge the two patches into a single one, and use dev_err
Changes since v1:
- Added Shawn's ack
drivers/net/ethernet/freescale/fec.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 0fe68c4..1a2071b 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1689,10 +1689,13 @@ static void fec_reset_phy(struct platform_device *pdev)
msec = 1;
phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+ if (!gpio_is_valid(phy_reset))
+ return;
+
err = devm_gpio_request_one(&pdev->dev, phy_reset,
GPIOF_OUT_INIT_LOW, "phy-reset");
if (err) {
- pr_debug("FEC: failed to get gpio phy-reset: %d\n", err);
+ dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
return;
}
msleep(msec);
--
1.7.9.5
^ permalink raw reply related
* Re: tuntap: Overload handling
From: Sebastian Pöhn @ 2013-02-17 16:08 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Eric Dumazet, netdev
In-Reply-To: <20130217132404.GA22552@redhat.com>
On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> > On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> >
> > > Hmm so ~1000 packets in the tun queue is not enough?
> > > You always have the option to increase it some more ...
> > >
> > > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > > (tun: only queue packets on device)
> > > >
> > >
> > > Eric in the past you said the following things
> > > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > > In your case I would just not use qdisc at all, like other virtual
> > > > > devices.
> > > ...
> > > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > > be always empty. Whats the point storing more than 500 packets for a
> > > > > device ? Thats a latency killer.
> > > you don't think this applies, anymore?
> > >
> >
> > Users have the choice to setup a qdisc or not.
> >
> > Having no qdisc can help raw performance, at the expense of bufferbloat.
> > Thats all I was saying.
> >
> > It seems tun.c has no longer the possibility to effectively use a qdisc,
> > (allowing the queue to buildup at qdisc layer)
> >
>
> But, userspace is in no position to decide whether using
> the qdisc is a good or a bad thing.
> The issue I tried to solve is that with tun, it's trivially easy for
> userspace to lock up resources forever.
> Simply not stopping the qdisc is probably the simplest solution.
>
> An alternative is to orphan the skbs before we queue them.
> At some point I posted a proposal doing exactly this
> subj of "net: orphan queued skbs if device tx can stall".
> Do you think it's worth revisiting this?
>
> Also - does anyone know of a testcase showing there's a problem
> with the simplest solution we now have in place?
>
I think the solution is good as it is. Of course if you want to do odd
things with it like me - it's not, but that's not its usual use-case.
^ permalink raw reply
* Re: [patch net-next v5 10/11] tbf: take into account gso skbs
From: Jiri Pirko @ 2013-02-17 16:18 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <1360687182.6884.5.camel@edumazet-glaptop>
Tue, Feb 12, 2013 at 05:39:42PM CET, eric.dumazet@gmail.com wrote:
>On Tue, 2013-02-12 at 11:12 +0100, Jiri Pirko wrote:
>> Ignore max_size check for gso skbs. This check made bigger packets
>> incorrectly dropped. Remove this limitation for gso skbs.
>>
>> Also for peaks, ignore mtu for gso skbs.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>> net/sched/sch_tbf.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>> index c8388f3..8973e93 100644
>> --- a/net/sched/sch_tbf.c
>> +++ b/net/sched/sch_tbf.c
>> @@ -121,7 +121,7 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc *sch)
>> struct tbf_sched_data *q = qdisc_priv(sch);
>> int ret;
>>
>> - if (qdisc_pkt_len(skb) > q->max_size)
>> + if (qdisc_pkt_len(skb) > q->max_size && !skb_is_gso(skb))
>> return qdisc_reshape_fail(skb, sch);
>>
>> ret = qdisc_enqueue(skb, q->qdisc);
>> @@ -165,7 +165,7 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch)
>>
>> if (q->peak_present) {
>> ptoks = toks + q->ptokens;
>> - if (ptoks > q->mtu)
>> + if (ptoks > q->mtu && !skb_is_gso(skb))
>> ptoks = q->mtu;
>> ptoks -= (s64) psched_l2t_ns(&q->peak, len);
>> }
>
>
>I guess this part is wrong.
>
>If we dont cap ptoks to q->mtu we allow bigger bursts.
I'm going through this issue back and front and on the second thought,
I think this patch might not be so wrong after all.
"Accumulating" time in ptoks would effectively cause the skb to be sent
only in case time for whole skb is available (accumulated).
The re-segmenting will only cause the skb fragments sent in each time frame.
I can't see how the bigger bursts you are reffering to can happen.
Or am I missing something?
>
>Ideally we could re-segment the skb if psched_l2t_ns(&q->peak, len) is
>bigger than q->mtu
>
>
>
>
>
>
^ permalink raw reply
* Re: tuntap: Overload handling
From: Michael S. Tsirkin @ 2013-02-17 16:18 UTC (permalink / raw)
To: Sebastian Pöhn; +Cc: Eric Dumazet, netdev
In-Reply-To: <1361117293.1748.1.camel@alpha.Speedport_W723_V_Typ_A_1_00_096>
On Sun, Feb 17, 2013 at 05:08:13PM +0100, Sebastian Pöhn wrote:
> On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> > On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> > > On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> > >
> > > > Hmm so ~1000 packets in the tun queue is not enough?
> > > > You always have the option to increase it some more ...
> > > >
> > > > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > > > (tun: only queue packets on device)
> > > > >
> > > >
> > > > Eric in the past you said the following things
> > > > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > > > In your case I would just not use qdisc at all, like other virtual
> > > > > > devices.
> > > > ...
> > > > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > > > be always empty. Whats the point storing more than 500 packets for a
> > > > > > device ? Thats a latency killer.
> > > > you don't think this applies, anymore?
> > > >
> > >
> > > Users have the choice to setup a qdisc or not.
> > >
> > > Having no qdisc can help raw performance, at the expense of bufferbloat.
> > > Thats all I was saying.
> > >
> > > It seems tun.c has no longer the possibility to effectively use a qdisc,
> > > (allowing the queue to buildup at qdisc layer)
> > >
> >
> > But, userspace is in no position to decide whether using
> > the qdisc is a good or a bad thing.
> > The issue I tried to solve is that with tun, it's trivially easy for
> > userspace to lock up resources forever.
> > Simply not stopping the qdisc is probably the simplest solution.
> >
> > An alternative is to orphan the skbs before we queue them.
> > At some point I posted a proposal doing exactly this
> > subj of "net: orphan queued skbs if device tx can stall".
> > Do you think it's worth revisiting this?
> >
> > Also - does anyone know of a testcase showing there's a problem
> > with the simplest solution we now have in place?
> >
>
> I think the solution is good as it is. Of course if you want to do odd
> things with it like me - it's not, but that's not its usual use-case.
Tap+UIO seems actually pretty close to a VM case.
Do you know it's not good for your usecase, or do you speculate?
What's the tx queue length in your setup?
--
MST
^ permalink raw reply
* Re: tuntap: Overload handling
From: Eric Dumazet @ 2013-02-17 17:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Sebastian Pöhn, netdev
In-Reply-To: <20130217132404.GA22552@redhat.com>
On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> But, userspace is in no position to decide whether using
> the qdisc is a good or a bad thing.
> The issue I tried to solve is that with tun, it's trivially easy for
> userspace to lock up resources forever.
> Simply not stopping the qdisc is probably the simplest solution.
>
> An alternative is to orphan the skbs before we queue them.
> At some point I posted a proposal doing exactly this
> subj of "net: orphan queued skbs if device tx can stall".
> Do you think it's worth revisiting this?
Its trivially easy for userspace to consume all resources, with or
without tuntap.
Say, a regular Gbps ethernet device.
We need some flow control at some point, unless we deal with device
with unlimited bandwidth.
If we orphan skbs too soon, how can we limit a single sender to flood
the device ? Even a single TCP flow could do that. TCP Small Queues
prevent this, but relies on proper skb orphaning.
If the tuntap problem is that skb can sit there and are never consumed,
its not a bug in the producer (the sender), but a problem with the
receiver (the consumer of the queue). Some kind of cleanup is needed.
Lets take the qdisc analogy :
Codel for instance is able to drop packets that are sitting too long in
queue. skbs are not orphaned until they are freed.
^ permalink raw reply
* Re: [patch net-next v5 10/11] tbf: take into account gso skbs
From: Eric Dumazet @ 2013-02-17 17:54 UTC (permalink / raw)
To: Jiri Pirko; +Cc: netdev, davem, edumazet, jhs, kuznet, j.vimal
In-Reply-To: <20130217161803.GB1931@minipsycho.orion>
On Sun, 2013-02-17 at 17:18 +0100, Jiri Pirko wrote:
> I'm going through this issue back and front and on the second thought,
> I think this patch might not be so wrong after all.
>
> "Accumulating" time in ptoks would effectively cause the skb to be sent
> only in case time for whole skb is available (accumulated).
>
> The re-segmenting will only cause the skb fragments sent in each time frame.
>
> I can't see how the bigger bursts you are reffering to can happen.
>
> Or am I missing something?
Token Bucket Filter doesnt allow to accumulate tokens above a given
threshold. Thats the whole point of the algo.
After a one hour idle time, you don't want to allow your device sending
a burst exceeding the constraint.
This is all about avoiding packet drops in a device with a very small
queue.
Your patch was pretty close to solve the problem.
^ permalink raw reply
* SPECIAL OFFER! APPLY NOW FOR 3% LOAN (PERSONAL, BUSINESS & PROJECT FINANCING LOANS)
From: GFS @ 2013-02-17 15:51 UTC (permalink / raw)
Good Day
Apply for personal, business project financing and any type loan at 3%
interest rate. Contact Ari Monfared of the Global Financial Solution
via email: arimonfared@blumail.org
In business, it takes money to make money. We are ready to entrust our money on you and your business ideas if you meet the necessary requirement.
Best Regards
Ari Monfared
Global Financial Solution
^ permalink raw reply
* Re: tuntap: Overload handling
From: Sebastian Pöhn @ 2013-02-17 20:54 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Eric Dumazet, netdev
In-Reply-To: <20130217161836.GA24375@redhat.com>
On Sun, 2013-02-17 at 18:18 +0200, Michael S. Tsirkin wrote:
> On Sun, Feb 17, 2013 at 05:08:13PM +0100, Sebastian Pöhn wrote:
> > On Sun, 2013-02-17 at 15:24 +0200, Michael S. Tsirkin wrote:
> > > On Thu, Feb 14, 2013 at 09:01:30AM -0800, Eric Dumazet wrote:
> > > > On Thu, 2013-02-14 at 18:42 +0200, Michael S. Tsirkin wrote:
> > > >
> > > > > Hmm so ~1000 packets in the tun queue is not enough?
> > > > > You always have the option to increase it some more ...
> > > > >
> > > > > > You should ask Michael S. Tsirkin, as he removed the flow control
> > > > > > in commit 5d097109257c03a71845729f8db6b5770c4bbedc
> > > > > > (tun: only queue packets on device)
> > > > > >
> > > > >
> > > > > Eric in the past you said the following things
> > > > > (http://lkml.indiana.edu/hypermail/linux/kernel/1204.1/00784.html)
> > > > > > > In your case I would just not use qdisc at all, like other virtual
> > > > > > > devices.
> > > > > ...
> > > > > > > Anyway, with a 500 packet limit in TUN queue itself, qdisc layer should
> > > > > > > be always empty. Whats the point storing more than 500 packets for a
> > > > > > > device ? Thats a latency killer.
> > > > > you don't think this applies, anymore?
> > > > >
> > > >
> > > > Users have the choice to setup a qdisc or not.
> > > >
> > > > Having no qdisc can help raw performance, at the expense of bufferbloat.
> > > > Thats all I was saying.
> > > >
> > > > It seems tun.c has no longer the possibility to effectively use a qdisc,
> > > > (allowing the queue to buildup at qdisc layer)
> > > >
> > >
> > > But, userspace is in no position to decide whether using
> > > the qdisc is a good or a bad thing.
> > > The issue I tried to solve is that with tun, it's trivially easy for
> > > userspace to lock up resources forever.
> > > Simply not stopping the qdisc is probably the simplest solution.
> > >
> > > An alternative is to orphan the skbs before we queue them.
> > > At some point I posted a proposal doing exactly this
> > > subj of "net: orphan queued skbs if device tx can stall".
> > > Do you think it's worth revisiting this?
> > >
> > > Also - does anyone know of a testcase showing there's a problem
> > > with the simplest solution we now have in place?
> > >
> >
> > I think the solution is good as it is. Of course if you want to do odd
> > things with it like me - it's not, but that's not its usual use-case.
>
> Tap+UIO seems actually pretty close to a VM case.
In this case, no. What I have is a over-provisioned wan line. If you
have double the load you can handle - having a QoS system which is
slowing down your clients is essential, dropping in a device driver is
the last you may want.
> Do you know it's not good for your usecase, or do you speculate?
Well the scheme I work in is like this: The wan line says 'Hey I have
some bandwidth, give me some traffic.' So it's not a good idea that the
network subsystem is blowing in a lot of traffic.
> What's the tx queue length in your setup?
No decided yet.
>
But again I think it's not the aim of tuntap to satisfy my exotic usage.
I'll gonna do some changes in it anyway.
^ permalink raw reply
* [PATCH] bonding: set sysfs device_type to 'bond'
From: Doug Goldstein @ 2013-02-18 2:38 UTC (permalink / raw)
To: netdev; +Cc: Jay Vosburgh, Andy Gospodarek, Doug Goldstein
Sets the sysfs device_type to 'bond' for udev. This allows udev rules to
be created for bond devices. This is similar to how other network
devices set their device_type.
Signed-off-by: Doug Goldstein <cardoe@cardoe.com>
---
drivers/net/bonding/bond_main.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b7d45f3..34a6e96 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -4350,6 +4350,10 @@ static const struct net_device_ops bond_netdev_ops = {
.ndo_add_slave = bond_enslave,
.ndo_del_slave = bond_release,
.ndo_fix_features = bond_fix_features,
+};
+
+static const struct device_type bond_type = {
+ .name = "bond",
};
static void bond_destructor(struct net_device *bond_dev)
@@ -4382,6 +4386,8 @@ static void bond_setup(struct net_device *bond_dev)
bond_dev->destructor = bond_destructor;
+ SET_NETDEV_DEVTYPE(bond_dev, &bond_type);
+
/* Initialize the device options */
bond_dev->tx_queue_len = 0;
bond_dev->flags |= IFF_MASTER|IFF_MULTICAST;
--
1.7.12.4
^ permalink raw reply related
* Re: [Patch net-next v4] netpoll: fix some sparse warnings in various places
From: Cong Wang @ 2013-02-18 2:55 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, Eric Dumazet, Vlad Yasevich, Stephen Hemminger,
David S. Miller
In-Reply-To: <20130216091808.GA1536@minipsycho.orion>
On Sat, 2013-02-16 at 10:18 +0100, Jiri Pirko wrote:
>
> Hmm. I'm just wondering if there wouldn't be nicer for these functions
> to
> return bool if you just want to know if npinfo is null or not. This
> notation seems a bit confusing to me.
Yes, makes sense.
^ permalink raw reply
* [PATCH] b43: Increase number of RX DMA slots
From: Larry Finger @ 2013-02-18 3:01 UTC (permalink / raw)
To: linville-2XuSBdqkA4R54TAoqtyWWQ
Cc: linux-wireless-u79uwXL29TY76Z2rM5mHXA, Larry Finger,
netdev-u79uwXL29TY76Z2rM5mHXA, Bastian Bittorf, Stable
Bastian Bittorf reported that some of the silent freezes on a Linksys WRT54G
were due to overflow of the RX DMA ring buffer, which was created with 64
slots. That finding reminded me that I was seeing similar crashed on a netbook,
which also has a relatively slow processor. After increasing the number of
slots to 128, runs on the netbook that previously failed now worked; however,
I found that 109 slots had been used in one test. For that reason, the number
of slots is being increased to 256.
Signed-off-by: Larry Finger <Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
Cc: Bastian Bittorf <bittorf-yxHw+vPQXZDZJqsBc5GL+g@public.gmane.org>
Cc: Stable <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
---
---
drivers/net/wireless/b43/dma.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/b43/dma.h b/drivers/net/wireless/b43/dma.h
index 315b96e..9fdd198 100644
--- a/drivers/net/wireless/b43/dma.h
+++ b/drivers/net/wireless/b43/dma.h
@@ -169,7 +169,7 @@ struct b43_dmadesc_generic {
/* DMA engine tuning knobs */
#define B43_TXRING_SLOTS 256
-#define B43_RXRING_SLOTS 64
+#define B43_RXRING_SLOTS 256
#define B43_DMA0_RX_FW598_BUFSIZE (B43_DMA0_RX_FW598_FO + IEEE80211_MAX_FRAME_LEN)
#define B43_DMA0_RX_FW351_BUFSIZE (B43_DMA0_RX_FW351_FO + IEEE80211_MAX_FRAME_LEN)
--
1.8.1.1
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [Patch net-next v5] netpoll: fix some sparse warnings in various places
From: Cong Wang @ 2013-02-18 3:22 UTC (permalink / raw)
To: netdev
Cc: Eric Dumazet, Jiri Pirko, Vlad Yasevich, Stephen Hemminger,
David S. Miller, Cong Wang
From: Cong Wang <amwang@redhat.com>
This fixes sparse warnings like the one below:
drivers/net/team/team.c:953:25: warning: incorrect type in return expression (different address spaces)
drivers/net/team/team.c:953:25: expected struct netpoll_info *
drivers/net/team/team.c:953:25: got struct netpoll_info [noderef] <asn:4>*npinfo
and make these functions return bool as suggested by Jiri.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: Vlad Yasevich <vyasevic@redhat.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 94c1534..2263bf2 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1300,9 +1300,9 @@ static int bond_netpoll_setup(struct net_device *dev, struct netpoll_info *ni, g
return err;
}
-static struct netpoll_info *bond_netpoll_info(struct bonding *bond)
+static bool bond_netpoll_info(struct bonding *bond)
{
- return bond->dev->npinfo;
+ return rcu_access_pointer(bond->dev->npinfo) != NULL;
}
#else
@@ -1862,8 +1862,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
bond_set_carrier(bond);
#ifdef CONFIG_NET_POLL_CONTROLLER
- slave_dev->npinfo = bond_netpoll_info(bond);
- if (slave_dev->npinfo) {
+ rcu_assign_pointer(slave_dev->npinfo, bond->dev->npinfo);
+ if (bond_netpoll_info(bond)) {
if (slave_enable_netpoll(new_slave)) {
read_unlock(&bond->lock);
pr_info("Error, %s: master_dev is using netpoll, "
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 05c5efe..26abee3 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -948,9 +948,9 @@ static void team_port_disable_netpoll(struct team_port *port)
kfree(np);
}
-static struct netpoll_info *team_netpoll_info(struct team *team)
+static bool team_netpoll_info(struct team *team)
{
- return team->dev->npinfo;
+ return rcu_access_pointer(team->dev->npinfo) != NULL;
}
#else
@@ -962,9 +962,9 @@ static int team_port_enable_netpoll(struct team *team, struct team_port *port,
static void team_port_disable_netpoll(struct team_port *port)
{
}
-static struct netpoll_info *team_netpoll_info(struct team *team)
+static bool team_netpoll_info(struct team *team)
{
- return NULL;
+ return false;
}
#endif
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 6d314c4..f69cdda 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -328,9 +328,9 @@ extern void br_dev_delete(struct net_device *dev, struct list_head *list);
extern netdev_tx_t br_dev_xmit(struct sk_buff *skb,
struct net_device *dev);
#ifdef CONFIG_NET_POLL_CONTROLLER
-static inline struct netpoll_info *br_netpoll_info(struct net_bridge *br)
+static inline bool br_netpoll_info(struct net_bridge *br)
{
- return br->dev->npinfo;
+ return rcu_access_pointer(br->dev->npinfo) != NULL;
}
static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
@@ -345,9 +345,9 @@ static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
extern int br_netpoll_enable(struct net_bridge_port *p, gfp_t gfp);
extern void br_netpoll_disable(struct net_bridge_port *p);
#else
-static inline struct netpoll_info *br_netpoll_info(struct net_bridge *br)
+static inline bool br_netpoll_info(struct net_bridge *br)
{
- return NULL;
+ return false;
}
static inline void br_netpoll_send_skb(const struct net_bridge_port *p,
^ permalink raw reply related
* Re: [PATCH firmware] rtl_nic: update firmware for RTL8168G
From: Ben Hutchings @ 2013-02-18 3:28 UTC (permalink / raw)
To: Hayes Wang; +Cc: dwmw2, romieu, netdev, linux-kernel
In-Reply-To: <1360163939-3625-1-git-send-email-hayeswang@realtek.com>
[-- Attachment #1: Type: text/plain, Size: 611 bytes --]
On Wed, 2013-02-06 at 23:18 +0800, Hayes Wang wrote:
> File: rtl_nic/rtl8168g-2.fw
> Version: 0.0.1
>
> This firmware supports new fw setting mothod for linux kernel.
>
> Add MAC OCP settings for:
> -phy reset
> -PXE in ALDPS
> -pwron sequence
> -OBFF
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
> WHENCE | 3 +++
> rtl_nic/rtl8168g-2.fw | Bin 0 -> 4896 bytes
> 2 files changed, 3 insertions(+)
> create mode 100644 rtl_nic/rtl8168g-2.fw
[...]
Applied, thanks.
Ben.
--
Ben Hutchings
Sturgeon's Law: Ninety percent of everything is crap.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* linux-next: build warning after merge of the net-next tree
From: Stephen Rothwell @ 2013-02-18 6:00 UTC (permalink / raw)
To: David Miller, netdev; +Cc: linux-next, linux-kernel, Pravin B Shelar
[-- Attachment #1: Type: text/plain, Size: 468 bytes --]
Hi all,
After merging the net-next tree, today's linux-next build (powerpc
ppc44x_defconfig) produced this warning:
net/ipv6/xfrm6_mode_tunnel.c: In function 'xfrm6_mode_tunnel_input':
net/ipv6/xfrm6_mode_tunnel.c:72:2: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
Introduced by commit 14bbd6a565e1 ("net: Add skb_unclone() helper
function").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3] net: fec: Do a sanity check on the gpio number
From: Shawn Guo @ 2013-02-18 6:16 UTC (permalink / raw)
To: Fabio Estevam; +Cc: davem, s.hauer, marex, netdev, Fabio Estevam
In-Reply-To: <1361114964-10517-1-git-send-email-festevam@gmail.com>
On Sun, Feb 17, 2013 at 12:29:24PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
>
> Since commit 372e722ea4d (gpiolib: use descriptors internally) the following
> warning is seen on a mx28evk board:
>
> [ 5.116291] ------------[ cut here ]------------
> [ 5.121306] WARNING: at drivers/gpio/gpiolib.c:125 gpio_to_desc+0x30/0x44()
> [ 5.128491] invalid GPIO -2
> [ 5.131563] Modules linked in:
> [ 5.134846] [<c0014e20>] (unwind_backtrace+0x0/0xf4) from [<c001d428>] (warn_slowpath_common+0x4c/0x68)
> [ 5.144682] [<c001d428>] (warn_slowpath_common+0x4c/0x68) from [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40)
> [ 5.154693] [<c001d4d8>] (warn_slowpath_fmt+0x30/0x40) from [<c0283434>] (gpio_to_desc+0x30/0x44)
> [ 5.164002] [<c0283434>] (gpio_to_desc+0x30/0x44) from [<c0285470>] (gpio_request_one+0x10/0xe8)
> [ 5.173294] [<c0285470>] (gpio_request_one+0x10/0xe8) from [<c0282f50>] (devm_gpio_request_one+0x40/0x74)
> [ 5.183332] [<c0282f50>] (devm_gpio_request_one+0x40/0x74) from [<c0319be0>] (fec_probe+0x2d0/0x99c)
> [ 5.192923] [<c0319be0>] (fec_probe+0x2d0/0x99c) from [<c02c8114>] (platform_drv_probe+0x14/0x18)
> [ 5.202228] [<c02c8114>] (platform_drv_probe+0x14/0x18) from [<c02c6e00>] (driver_probe_device+0x90/0x224)
> [ 5.212332] [<c02c6e00>] (driver_probe_device+0x90/0x224) from [<c02c7028>] (__driver_attach+0x94/0x98)
> [ 5.222162] [<c02c7028>] (__driver_attach+0x94/0x98) from [<c02c5750>] (bus_for_each_dev+0x78/0x98)
> [ 5.231642] [<c02c5750>] (bus_for_each_dev+0x78/0x98) from [<c02c5fd0>] (bus_add_driver+0x1a4/0x240)
> [ 5.241207] [<c02c5fd0>] (bus_add_driver+0x1a4/0x240) from [<c02c7608>] (driver_register+0x78/0x140)
> [ 5.250768] [<c02c7608>] (driver_register+0x78/0x140) from [<c00087a4>] (do_one_initcall+0x30/0x17c)
> [ 5.260347] [<c00087a4>] (do_one_initcall+0x30/0x17c) from [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0)
> [ 5.270381] [<c05fa29c>] (kernel_init_freeable+0xe8/0x1b0) from [<c044dfd4>] (kernel_init+0x8/0xe4)
> [ 5.279886] [<c044dfd4>] (kernel_init+0x8/0xe4) from [<c000f248>] (ret_from_fork+0x14/0x2c)
> [ 5.288740] ---[ end trace c15c72a22979d58d ]---
>
> mx28evk has two ethernet controllers. The GPIO that performs the
> ethernet reset on both ports is the same GPIO, so on the board dts file, only in
> one ethernet instance is passed the GPIO reset property.
>
> Validate the gpio number prior to requesting it in order to avoid such warning.
>
> With such gpio validation in place, it is also safe to change from pr_debug to
> dev_err in the case the gpio request fails.
>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Reviewed-by: Shawn Guo <shawn.guo@linaro.org>
^ permalink raw reply
* Re: i.Mx6Quad - eth0: tx queue full!
From: Frank Li @ 2013-02-18 7:29 UTC (permalink / raw)
To: Troy Kisky; +Cc: Vikram Narayanan, netdev, LAK, Fabio Estevam
In-Reply-To: <51098AE4.5030305@boundarydevices.com>
Do you have a way to duplicate this issue consistently?
best regards
Frank Li
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox