* Re: [PATCH net 06/10] net: hns: fix return value of the function about rss
From: David Miller @ 2016-03-21 15:42 UTC (permalink / raw)
To: Yisen.Zhuang
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <1458558401-190165-7-git-send-email-Yisen.Zhuang@huawei.com>
From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
Date: Mon, 21 Mar 2016 19:06:37 +0800
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> index 3c4a3bc..f3a5e05 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
...
> @@ -1235,12 +1226,11 @@ hns_set_rss(struct net_device *netdev, const u32 *indir, const u8 *key,
> {
> struct hns_nic_priv *priv = netdev_priv(netdev);
> struct hnae_ae_ops *ops;
> - int ret;
>
> if (AE_IS_VER1(priv->enet_ver)) {
> netdev_err(netdev,
> "RSS feature is not supported on this hardware\n");
> - return -EOPNOTSUPP;
> + return (u32)-EOPNOTSUPP;
> }
>
This is incredibly broken, you cannot cast this negative error return value to
an unsigned integer.
^ permalink raw reply
* [PATCH net] ipv4: fix broadcast packets reception
From: Paolo Abeni @ 2016-03-21 15:42 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Alexey Kuznetsov, Shawn Bohrer,
Hannes Frederic Sowa
Currently, ingress ipv4 broadcast datagrams are dropped if the
ingress interface lacks an ipv4 address. This is caused by
multiple issues:
- in udp_v4_early_demux() ip_check_mc_rcu is invoked even on
bcast packets
- ip_route_input_slow() always try to validate the source
This patch tries to address both issues, invoking ip_check_mc_rcu()
only for mcast packets and calling fib_validate_source() only
if the in_device has an address, at least.
Fixes: 6e5403093261 ("ipv4/udp: Verify multicast group is ours in upd_v4_early_demux()")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/ipv4/route.c | 6 +++++-
net/ipv4/udp.c | 12 ++++++++----
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 02c6229..e86d33d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1867,7 +1867,11 @@ brd_input:
if (skb->protocol != htons(ETH_P_IP))
goto e_inval;
- if (!ipv4_is_zeronet(saddr)) {
+ /* Do not validate the source if this device not not have an
+ * IPv4 address yet and the destination is the broadcast address.
+ */
+ if (!ipv4_is_zeronet(saddr) && (in_dev->ifa_list ||
+ !ipv4_is_lbcast(daddr))) {
err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
in_dev, &itag);
if (err < 0)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 836abe5..08eed5e 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2070,10 +2070,14 @@ void udp_v4_early_demux(struct sk_buff *skb)
if (!in_dev)
return;
- ours = ip_check_mc_rcu(in_dev, iph->daddr, iph->saddr,
- iph->protocol);
- if (!ours)
- return;
+ /* we are supposed to accept bcast packets */
+ if (skb->pkt_type == PACKET_MULTICAST) {
+ ours = ip_check_mc_rcu(in_dev, iph->daddr, iph->saddr,
+ iph->protocol);
+ if (!ours)
+ return;
+ }
+
sk = __udp4_lib_mcast_demux_lookup(net, uh->dest, iph->daddr,
uh->source, iph->saddr, dif);
} else if (skb->pkt_type == PACKET_HOST) {
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net 03/10] net: hns: add uc match for debug ports
From: David Miller @ 2016-03-21 15:40 UTC (permalink / raw)
To: Yisen.Zhuang
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <1458558401-190165-4-git-send-email-Yisen.Zhuang@huawei.com>
From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
Date: Mon, 21 Mar 2016 19:06:34 +0800
> + (void)hns_mac_set_promisc(mac_cb, (u8)!!en);
This cast to void is unnecssary.
> +static void hns_gmac_set_uc_match(void *mac_drv, u16 en)
> +{
> + struct mac_driver *drv = (struct mac_driver *)mac_drv;
Casts from void pointers are unnecessary.
> +{
> + struct mac_driver *drv = (struct mac_driver *)mac_drv;
Likewise.
^ permalink raw reply
* Re: [PATCH net 01/10] net: hns: bug fix about ping6
From: David Miller @ 2016-03-21 15:39 UTC (permalink / raw)
To: Yisen.Zhuang
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <1458558401-190165-2-git-send-email-Yisen.Zhuang@huawei.com>
From: Yisen Zhuang <Yisen.Zhuang@huawei.com>
Date: Mon, 21 Mar 2016 19:06:32 +0800
> From: Kejian Yan <yankejian@huawei.com>
>
> The current upstreaming code fails to ping other IPv6 net device, because
> the enet receives the multicast packets with the src mac addr whick is the
^^^^^
"which"
> @@ -670,6 +671,14 @@ out_bnum_err:
> return -EFAULT;
> }
>
> + /* filter out multicast pkt with the same src mac as this port */
> + eh = (struct ethhdr *)skb->data;
Please use the "eth_hdr()" helper.
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Sebastian Frias @ 2016-03-21 15:36 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Daniel Mack, David S. Miller, netdev, lkml, mason,
Florian Fainelli, Mans Rullgard, Fabio Estevam,
Martin Blumenstingl, Linus Walleij
In-Reply-To: <20160321135457.GX4292@pengutronix.de>
Hi Uwe,
On 03/21/2016 02:54 PM, Uwe Kleine-König wrote:
>>
>> Two things:
>> - I suppose that in such hypothetical case the dependency on GPIOLIB
>> would be required and thus be there
>
> I don't agree. There are bugs out there, and maybe the reason is as
> simple as "the implementor of the reset-gpio handling had GPIOLIB on and
> didn't test without GPIOLIB".
:-) fair enough, let's wait for his comments then.
>
>> - We'd also need to check that 'gpiod' is not NULL
>
> That is the optional part. If gpiod is NULL, you have one of the devices
> that don't need to handle the enable line.
gpiod=NULL (because the key is not there) or gpiod=ERR (because
GPIOLIB=n + my patch) will result in the same behaviour, ie: driver
binds, but fails to reset.
>
>> In the scenario at hand only some devices require a hack and
>> gpiod_reset=NULL is valid (ie: device will not fail probing)
>
> With your patch and GPIOLIB=n you bind the driver even for the devices
> that need the hack. This is broken!
It is a degraded mode I agree and had proposed to print a warning.
However, I fail to see how is GPIOLIB=n different from just not having
"reset" present.
The fact that GPIOLIB=y does not means that the "reset" key will be there.
I mean, you assume that "reset" will be present for devices that need
the hack, yet there is no such guarantee and the code clearly assumes
that it can be missing.
For all we know, the key is actually missing on systems that do use the
faulty PHY (all systems designed prior to the hack being implemented)
And that, regardless of GPIOLIB status.
Since the reset line can be missing, it can be missing for whatever reason.
Whether it is because the "reset" key is not present or because
GPIOLIB=n, it does not matter, it will result on the same outcome.
Furthermore, if "reset" is really required for certain devices that need
the hack, then both cases:
- GPIOLIB=n
- "reset" not present
should be handled the same way (ie: not bind the driver) for such devices.
By the way, I think not binding the driver is too strong too.
Having a GPIO free and being able to route it to the PHY for reset may
not even be possible on some systems.
Are they supposed to stop working?
>
>>> For consistency I'd recommend to do the
>>> same for reset even though there is a chance to get a working device.
>>
>> I'm not so sure, because then information would be lost, handling both
>> ("enable" and "reset") the same way aliases different intents and
>> requirements.
>> Indeed, only "enable" would be really mandatory, "reset" is essentially
>> optional (only 1 of 3 devices of the family require the hack).
>>
>> Besides, if "reset" was really mandatory, we would also fail the probe
>> if the pointer for the reset GPIO is NULL.
>
> No, if reset was mandatory you'd use gpiod_get instead of
> gpiod_get_optional and fail iff that call fails, too.
Ok, but the current code for "reset" is using devm_gpiod_get_optional()
so "reset" is clearly optional.
And that call will return NULL if "reset" is not present, even with
GPIOLIB=y
>
>> Indeed, even with GPIOLIB=y the pointer can be NULL if the "reset" (or
>> "enable" in your scenario) is not present.
>> From a functionality perspective, a NULL pointer for "reset" will result
>> in the same behaviour as GPIOLIB=n, ie: not being able to reset the PHY.
>
> Right, but you only get reset=NULL iff the device tree (or whatever is
> the source of information for gpiod_get) doesn't specify a reset gpio
> which then means "This device doesn't need a reset gpio.". This is
> different from the GPIOLIB=n case, where the return code signals "It's
> unknown if the device needs a reset gpio.".
I think somehow you try to make a difference on the reason why the
reset=INVALID (NULL or ERR), but IMHO there's none.
If somebody using AT8030 PHY (the one that requires the hack) either
does not configure a "reset" key on the DT, either does not enable
GPIOLIB, the result will be the same.
There is no warning in either case and it will run on a degraded mode.
>
>> So, the current code (your commit) and previous code (Daniel's commit)
>> clearly points to "reset" being optional.
>>
>> Furthermore, I think we should not even register the
>> "link_change_notify" callback when dealing with AT803x PHYs that do not
>> require the hack.
>> Another solution (considering the callback is statically registered in
>> the "phy_driver" structs for all AT803x PHYs) would be for the callback
>> to disable itself.
>> Once it detects that the PHY does not require the hack, it could just
>> perform:
>>
>> phydev->drv->link_change_notify = NULL;
>>
>> or call a new generic function to do the same if encapsulation is required.
>>
>> If everybody agrees I can make such change as well, but I really think
>> Daniel should give his opinion first.
>>
>>>
>>>> What would you think of making at803x_link_change_notify() print a
>>>> message every time it should do a reset but does not has a way to do it?
>>>
>>> Then this question is obsolete because the device doesn't probe.
>>
>> I think you assume that "reset" is mandatory for all AT803x devices, but
>> that's not what the code says.
>
> No, you're wrong here. I'm aware that the code supports devices without
> reset.
Ok, then I did not understand what you meant with "the question is
obsolete because the device doesn't probe".
Unless I'm missing something, the only way the driver won't bind
correctly with current code is if GPIOLIB=n
Systems using the faulty PHY and having GPIOLIB=y but with an outdated
DT that does not has a "reset" key would have the PHY driver bind yet
have it fail due to missing "reset".
>
>> As such, my proposal was to:
>> - keep my proposed patch
>
> I don't agree.
>
>> - make another patch to print a warning when gpiod_reset is NULL (which
>> can happen, even without my patch)
>
> This only happens if no reset gpio is needed and in this case the driver
> does the right thing. So if you ask me, there is no need to modify the
> driver. Better add a dependency to GPIOLIB. This is necessary even for
> devices which don't need a reset gpio to answer the question "does this
> driver need a reset gpio?" correctly.
I don't see how the dependency on GPIOLIB=y improves the situation in
any useful way.
To put an example: in our case we don't use the faulty PHY.
So, the DT does not has the "reset" key.
Why should then the PHY driver be dependent on GPIOLIB?
Best regards,
Sebastian
^ permalink raw reply
* Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
From: Joachim Eastwood @ 2016-03-21 15:36 UTC (permalink / raw)
To: Rob Herring
Cc: Chen-Yu Tsai, Alexandre TORGUE, Maxime Coquelin,
Giuseppe Cavallaro, netdev, devicetree, linux-kernel,
linux-arm-kernel
In-Reply-To: <20160321124043.GA26873@rob-hp-laptop>
On 21 March 2016 at 13:40, Rob Herring <robh@kernel.org> wrote:
> On Sat, Mar 19, 2016 at 12:00:22AM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE
>> <alexandre.torgue@gmail.com> wrote:
>> > +- clocks: Must contain a phandle for each entry in clock-names.
>> > +- clock-names: Should be "stmmaceth" for the host clock.
>
> This doesn't sound like the clock input signal name...
>
>> > + Should be "tx-clk" for the MAC TX clock.
>> > + Should be "rx-clk" for the MAC RX clock.
>
> How can other DWMAC blocks not have these clocks? The glue can't really
> add these clocks. It could combine them into one or a new version of
> DWMAC could have a different number of clock inputs. So if there is
> variation here, then some of the bindings are probably wrong. I guess
> the only change I'm suggesting is possibly moving these into common
> binding doc.
The LPC18xx implementation probably have these clocks as well but the
LPC1850 user manual only documents the main clock. Someone with access
to the IP block doc from Synopsys should be able to check which clocks
the MAC really needs.
Rockchip bindings have two clocks named "mac_clk_rx" and "mac_clk_tx".
These are probably the same as stm32 needs so maybe use these names
and move them into the main doc and update the rockchip binding.
regards,
Joachim Eastwood
^ permalink raw reply
* Re: [patch] mdio-sun4i: oops in error handling in probe
From: David Miller @ 2016-03-21 15:30 UTC (permalink / raw)
To: dan.carpenter; +Cc: f.fainelli, maxime.ripard, wens, netdev, kernel-janitors
In-Reply-To: <20160321090231.GA31670@mwanda>
From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 21 Mar 2016 12:02:31 +0300
> We could end up dereferencing an error pointer when we call
> regulator_disable().
>
> Fixes: 4bdcb1dd9feb ('net: Add MDIO bus driver for the Allwinner EMAC')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied, thanks Dan.
^ permalink raw reply
* Re: [PATCH] net: smc911x: avoid unused variable warnings
From: David Miller @ 2016-03-21 15:27 UTC (permalink / raw)
To: arnd; +Cc: netdev, robert.jarzmik, linux-kernel
In-Reply-To: <1458549072-1914517-1-git-send-email-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Mon, 21 Mar 2016 09:30:59 +0100
> The change to use the generic DMA engine API in the smc911x
> driver has led to a harmless warning about unused local variables:
>
> smsc/smc911x.c: In function 'smc911x_probe':
> smsc/smc911x.c:1796:20: error: unused variable 'param'
> smsc/smc911x.c:1795:17: error: unused variable 'mask'
> smsc/smc911x.c:1794:26: error: unused variable 'config'
>
> This puts the variable declarations inside of the same #ifdef
> that protects their use.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 79d3b59a93ba ("net: smc911x: convert pxa dma to dmaengine")
Applied.
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: add the AF_KCM entries to family name tables
From: David Miller @ 2016-03-21 15:27 UTC (permalink / raw)
To: decui; +Cc: olaf, gregkh, jasowang, linux-kernel, netdev, apw, devel,
haiyangz
In-Reply-To: <1458553869-26756-1-git-send-email-decui@microsoft.com>
Two things wrong with this submission:
1) You need to provide an initial "[PATCH net-next 0/3] ..." header posting
explaining at a high level what this patch series is about and how it is
implemented and why.
2) The net-next tree is closed at this time because we are in the merge window,
therefore no new feature patches should be submitted to the netdev mailing
list at this time. Please wait until some (reasonable) amount of time after
the merge window closes to resubmit this.
Thanks.
^ permalink raw reply
* RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM
From: Woojung.Huh @ 2016-03-21 14:59 UTC (permalink / raw)
To: linux, geert, UNGLinuxDriver, davem
Cc: rjw, netdev, linux-usb, linux-pm, linux-kernel
In-Reply-To: <56EF2B44.9040807@roeck-us.net>
> -----Original Message-----
> From: Guenter Roeck [mailto:linux@roeck-us.net]
> Sent: Sunday, March 20, 2016 6:59 PM
> To: Geert Uytterhoeven; Woojung Huh - C21699; UNGLinuxDriver; David S.
> Miller
> Cc: Rafael J. Wysocki; netdev@vger.kernel.org; linux-usb@vger.kernel.org;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM
>
> Not that it matters anymore since David reverted the original patch,
> but my reason for not sending a similar patch was that I wasn't sure
> if .runtime_auto should be accessed from drivers in the first place,
> or if there is some logical problem with the code.
>
Because driver can enable/disable autosuspend by usb_enable_autosuspend() and
usb_disable_autosuspend(), it would be nice to have helper to find out autosuspend status.
The code path was to utilize autosuspend power saving, when it is enabled.
Woojung
^ permalink raw reply
* Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM
From: Alan Stern @ 2016-03-21 14:57 UTC (permalink / raw)
To: Oliver Neukum
Cc: Geert Uytterhoeven, Woojung Huh, Microchip Linux Driver Support,
David S. Miller, Guenter Roeck, Rafael J. Wysocki, netdev,
linux-usb, linux-pm, linux-kernel
In-Reply-To: <1458549361.2299.1.camel@suse.com>
On Mon, 21 Mar 2016, Oliver Neukum wrote:
> On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
> > If CONFIG_PM=n:
> >
> > drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
> > drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no
> > member named ‘runtime_auto’
> >
> > If PM is disabled, the runtime_auto flag is not available, but auto
> > suspend is not enabled anyway. Hence protect the check for
> > runtime_auto
> > by #ifdef CONFIG_PM to fix this.
> >
> > Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper
> > to
> > include/linux/pm.h, which always return false if CONFIG_PM is
> > disabled.
>
> That is what we do for almost everything else in include/pm_runtime.h
> So it is the best solution to add the stub.
Guenter's question about whether drivers should try to access
runtime_auto in the first place was a good one. A similar problem
arises in the block layer: When a block device has removable media, the
block layer probes at 1-second intervals looking for media changes.
This defeats autosuspend in the same way as we're talking about here.
One possible solution is to export a sysfs parameter to prevent
statistics collection (or more generally, to change the interval at
which it occurs).
But checking the runtime_auto flag is probably not a great idea. Even
if it isn't set, collecting statistics is likely to wait up a device
that otherwise would have remained suspended.
Perhaps the best solution is to collect the statistics only when the
device is not suspended or is about to suspend.
Alan Stern
^ permalink raw reply
* RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM
From: Woojung.Huh-UWL1GkI3JZL3oGB3hsPCZA @ 2016-03-21 14:41 UTC (permalink / raw)
To: oneukum-IBi9RG/b67k, geert-Td1EMuHUCqxL1ZNQvxDV9g
Cc: UNGLinuxDriver-UWL1GkI3JZL3oGB3hsPCZA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-0h96xk9xTtrk1uMJSBkQmQ,
rjw-LthD3rsA81gm4RdzfppkhA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1458549361.2299.1.camel-IBi9RG/b67k@public.gmane.org>
> -----Original Message-----
> From: Oliver Neukum [mailto:oneukum@suse.com]
> Sent: Monday, March 21, 2016 4:36 AM
> To: Geert Uytterhoeven
> Cc: Woojung Huh - C21699; UNGLinuxDriver; David S. Miller; Guenter Roeck;
> Rafael J. Wysocki; netdev@vger.kernel.org; linux-usb@vger.kernel.org;
> linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM
Thanks for all comments.
Will look into it and submit new patch.
- Woojung
^ permalink raw reply
* Re: [PATCH 4/5] ath9k: fix misleading indentation
From: Kalle Valo @ 2016-03-21 14:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, netdev, QCA ath9k Development, linux-wireless,
ath9k-devel, linux-kernel
In-Reply-To: <1457965120-3155420-5-git-send-email-arnd@arndb.de>
Arnd Bergmann <arnd@arndb.de> writes:
> A cleanup patch in linux-3.18 moved around some code in the ath9k
> driver and left some code to be indented in a misleading way,
> made worse by the addition of some new code for p2p mode, as
> discovered by a new gcc-6 warning:
>
> drivers/net/wireless/ath/ath9k/init.c: In function 'ath9k_set_hw_capab':
> drivers/net/wireless/ath/ath9k/init.c:851:4: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
> hw->wiphy->iface_combinations = if_comb;
> ^~
> drivers/net/wireless/ath/ath9k/init.c:847:3: note: ...this 'if' clause, but it is not
> if (ath9k_is_chanctx_enabled())
> ^~
>
> The code is in fact correct, but the indentation is not, so I'm
> reformatting it as it should have been after the original cleanup.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 499afaccf6f3 ("ath9k: Isolate ath9k_use_chanctx module parameter")
> Fixes: eb61f9f623f7 ("ath9k: advertise p2p dev support when chanctx")
Dave already applied this so I can skip this.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 3/5] ath9k: fix buffer overrun for ar9287
From: Kalle Valo @ 2016-03-21 14:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA,
QCA ath9k Development, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
ath9k-devel-xDcbHBWguxHbcTqmT+pZeQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1457965120-3155420-4-git-send-email-arnd-r2nGTMty4D4@public.gmane.org>
Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> writes:
> Code that was added back in 2.6.38 has an obvious overflow
> when accessing a static array, and at the time it was added
> only a code comment was put in front of it as a reminder
> to have it reviewed properly.
>
> This has not happened, but gcc-6 now points to the specific
> overflow:
>
> drivers/net/wireless/ath/ath9k/eeprom.c: In function 'ath9k_hw_get_gain_boundaries_pdadcs':
> drivers/net/wireless/ath/ath9k/eeprom.c:483:44: error: array subscript is above array bounds [-Werror=array-bounds]
> maxPwrT4[i] = data_9287[idxL].pwrPdg[i][4];
> ~~~~~~~~~~~~~~~~~~~~~~~~~^~~
>
> It turns out that the correct array length exists in the local
> 'intercepts' variable of this function, so we can just use that
> instead of hardcoding '4', so this patch changes all three
> instances to use that variable. The other two instances were
> already correct, but it's more consistent this way.
>
> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Fixes: 940cd2c12ebf ("ath9k_hw: merge the ar9287 version of ath9k_hw_get_gain_boundaries_pdadcs")
Dave already applies this so I can skip this.
--
Kalle Valo
--
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
* Re: [PATCH net 09/10] net: hns: adds limitation for debug port mtu
From: Sergei Shtylyov @ 2016-03-21 14:17 UTC (permalink / raw)
To: Yisen Zhuang, davem
Cc: yankejian, huangdaode, salil.mehta, lisheng011, lipeng321,
liguozhu, arnd, xieqianqian, andrew, ivecera, netdev,
linux-kernel, linuxarm
In-Reply-To: <1458558401-190165-10-git-send-email-Yisen.Zhuang@huawei.com>
Hello.
On 03/21/2016 02:06 PM, Yisen Zhuang wrote:
> From: Kejian Yan <yankejian@huawei.com>
>
> If mtu for debug port is set more than 1500, it may cause that packets
> are dropped by ppe. So maximum value for debug port should be 1500.
>
> Signed-off-by: Kejian Yan <yankejian@huawei.com>
> Signed-off-by: Yisen Zhuang <Yisen.Zhuang@huawei.com>
[...]
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h
> index a69c8af..a68efd6 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h
> @@ -28,6 +28,7 @@ struct dsaf_device;
> #define MAC_MAX_MTU 9600
> #define MAC_MAX_MTU_V2 9728
> #define MAC_MIN_MTU 68
> +#define MAC_MAX_MTU_DBG MAC_DEFAULT_MTU
Please use tabs, not spaces to indent the value, like above
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Uwe Kleine-König @ 2016-03-21 13:54 UTC (permalink / raw)
To: Sebastian Frias
Cc: Daniel Mack, David S. Miller, netdev, lkml, mason,
Florian Fainelli, Mans Rullgard, Fabio Estevam,
Martin Blumenstingl, Linus Walleij
In-Reply-To: <56EFEDAD.9030903@laposte.net>
Hello Sebastian,
On Mon, Mar 21, 2016 at 01:48:45PM +0100, Sebastian Frias wrote:
> On 03/18/2016 08:12 PM, Uwe Kleine-König wrote:
> > On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
> >> On 03/18/2016 01:54 PM, Uwe Kleine-König wrote:
> >>> From a driver perspecitive, it would be nice if devm_gpiod_get_optional
> >>> returned NULL iff the respective gpio isn't specified even with
> >>> GPIOLIB=n, but this isn't sensible either because it would result in
> >>> quite some gpiolib code to not being conditionally compiled on
> >>> CONFIG_GPIOLIB any more.
> >>
> >> Let's say that was the case, what would the PHY code do?
> >
> > With reset gpios it might not be that critical, but consider an optional
> > enable gpio. (Optional in the sense, that some device have it and others
> > don't, e.g. because the pin is pulled into active level by hardware.)
> >
> > Now you do:
> >
> > gpiod = gpiod_get_optional("enable");
> >
> > and if gpiod now is an error pointer, you must assume that you cannot
> > operate the device. And even with GPIOLIB=n (and gpiod = ERR_PTR(-ENOSYS))
> > you cannot ignore the error.
>
> Two things:
> - I suppose that in such hypothetical case the dependency on GPIOLIB
> would be required and thus be there
I don't agree. There are bugs out there, and maybe the reason is as
simple as "the implementor of the reset-gpio handling had GPIOLIB on and
didn't test without GPIOLIB".
> - We'd also need to check that 'gpiod' is not NULL
That is the optional part. If gpiod is NULL, you have one of the devices
that don't need to handle the enable line.
> In the scenario at hand only some devices require a hack and
> gpiod_reset=NULL is valid (ie: device will not fail probing)
With your patch and GPIOLIB=n you bind the driver even for the devices
that need the hack. This is broken!
> >For consistency I'd recommend to do the
> > same for reset even though there is a chance to get a working device.
>
> I'm not so sure, because then information would be lost, handling both
> ("enable" and "reset") the same way aliases different intents and
> requirements.
> Indeed, only "enable" would be really mandatory, "reset" is essentially
> optional (only 1 of 3 devices of the family require the hack).
>
> Besides, if "reset" was really mandatory, we would also fail the probe
> if the pointer for the reset GPIO is NULL.
No, if reset was mandatory you'd use gpiod_get instead of
gpiod_get_optional and fail iff that call fails, too.
> Indeed, even with GPIOLIB=y the pointer can be NULL if the "reset" (or
> "enable" in your scenario) is not present.
> From a functionality perspective, a NULL pointer for "reset" will result
> in the same behaviour as GPIOLIB=n, ie: not being able to reset the PHY.
Right, but you only get reset=NULL iff the device tree (or whatever is
the source of information for gpiod_get) doesn't specify a reset gpio
which then means "This device doesn't need a reset gpio.". This is
different from the GPIOLIB=n case, where the return code signals "It's
unknown if the device needs a reset gpio.".
> So, the current code (your commit) and previous code (Daniel's commit)
> clearly points to "reset" being optional.
>
> Furthermore, I think we should not even register the
> "link_change_notify" callback when dealing with AT803x PHYs that do not
> require the hack.
> Another solution (considering the callback is statically registered in
> the "phy_driver" structs for all AT803x PHYs) would be for the callback
> to disable itself.
> Once it detects that the PHY does not require the hack, it could just
> perform:
>
> phydev->drv->link_change_notify = NULL;
>
> or call a new generic function to do the same if encapsulation is required.
>
> If everybody agrees I can make such change as well, but I really think
> Daniel should give his opinion first.
>
> >
> >> What would you think of making at803x_link_change_notify() print a
> >> message every time it should do a reset but does not has a way to do it?
> >
> > Then this question is obsolete because the device doesn't probe.
>
> I think you assume that "reset" is mandatory for all AT803x devices, but
> that's not what the code says.
No, you're wrong here. I'm aware that the code supports devices without
reset.
> As such, my proposal was to:
> - keep my proposed patch
I don't agree.
> - make another patch to print a warning when gpiod_reset is NULL (which
> can happen, even without my patch)
This only happens if no reset gpio is needed and in this case the driver
does the right thing. So if you ask me, there is no need to modify the
driver. Better add a dependency to GPIOLIB. This is necessary even for
devices which don't need a reset gpio to answer the question "does this
driver need a reset gpio?" correctly.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
From: Giuseppe CAVALLARO @ 2016-03-21 14:02 UTC (permalink / raw)
To: Alexandre Torgue, Chen-Yu Tsai
Cc: Maxime Coquelin, netdev, devicetree, Joachim Eastwood,
linux-kernel, linux-arm-kernel
In-Reply-To: <CAJgp7zxyQGrSSvNszawBQfS+omgKi1i35uW-eDyLdW-jP7oRjg@mail.gmail.com>
On 3/21/2016 11:45 AM, Alexandre Torgue wrote:
> Hi,
>
> 2016-03-18 17:00 GMT+01:00 Chen-Yu Tsai <wens@csie.org>:
>> Hi,
>>
>> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE
>> <alexandre.torgue@gmail.com> wrote:
>>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>>> new file mode 100644
>>> index 0000000..ada2aa4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>>> @@ -0,0 +1,32 @@
>>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>>> +
>>> +This file documents platform glue layer for stmmac.
>>> +Please see stmmac.txt for the other unchanged properties.
>>> +
>>> +The device node has following properties.
>>> +
>>> +Required properties:
>>> +- compatible: Should be "st,stm32-dwmac" to select glue, and
>>> + "snps,dwmac-3.50a" to select IP vesrion.
Almost all the synp gmac chips have the HW capability register that is
used for setting all the parameters at probe time.
This will override fields passed from DT. In theory, it is not
necessary to pass: "snps,dwmac-3.50a" from device tree at least there
is either no HW cap reg or the glue has some w/a for a specific chip
revision.
To be honest, I like to see the "snps,dwmac-3.50a" as compatibility
to also have a better readability (that's my personal view ;-) ).
Peppe
>>
>> If you need have sort of hardware glue, then it is not compatible.
>>
>
> We could have the case where the glue is set by a bootloader.
> In this case, we will select IP version in compatible and we will use
> generic dwmac glue to probe stmmac driver.
>
> Regards
>
> Alex.
>
>> ChenYu
>>
>>> +- clocks: Must contain a phandle for each entry in clock-names.
>>> +- clock-names: Should be "stmmaceth" for the host clock.
>>> + Should be "tx-clk" for the MAC TX clock.
>>> + Should be "rx-clk" for the MAC RX clock.
>>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>>> + encompases the glue register, and the offset of the control register.
>>> +Example:
>>> +
>>> + ethernet0: dwmac@40028000 {
>>> + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>>> + status = "disabled";
>>> + reg = <0x40028000 0x8000>;
>>> + reg-names = "stmmaceth";
>>> + interrupts = <0 61 0>, <0 62 0>;
>>> + interrupt-names = "macirq", "eth_wake_irq";
>>> + clock-names = "stmmaceth", "tx-clk", "rx-clk";
>>> + clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>>> + st,syscon = <&syscfg 0x4>;
>>> + snps,pbl = <8>;
>>> + snps,mixed-burst;
>>> + dma-ranges;
>>> + };
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
^ permalink raw reply
* Re: [PATCH net-next] net/mlx4_core: Fix backward compatibility on VFs
From: Eli Cohen @ 2016-03-21 13:56 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <56EF8058.2060203-sLpHqDYs0B2HXe+LvDLADg@public.gmane.org>
On Mon, Mar 21, 2016 at 04:02:16PM +1100, Alexey Kardashevskiy wrote:
>
> After more tries, I found that if for whatever reason mlx4_core
> fails to stop while shutting the guest down (last message is
> "mlx4_core 0000:00:00.0: mlx4_shutdown was called"), then next time
> VF in guest won't start.
>
> Example #1:
>
> mlx4_core: Mellanox ConnectX core driver v2.2-1 (Feb, 2014)
> mlx4_core: Initializing 0000:00:00.0
> mlx4_core 0000:00:00.0: enabling device (0000 -> 0002)
> mlx4_core 0000:00:00.0: Detected virtual function - running in slave mode
> mlx4_core 0000:00:00.0: Sending reset
> mlx4_core 0000:00:00.0: Sending vhcr0
> mlx4_core 0000:00:00.0: HCA minimum page size:1
> mlx4_core 0000:00:00.0: UAR size:4096 != kernel PAGE_SIZE of 65536
> mlx4_core 0000:00:00.0: Failed to obtain slave caps
Alexey, can you verify that the value of the enable_4k_uar parameter
is false?
>
> Example #2:
>
> root@le-dbg:~# dhclient eth0
> NETDEV WATCHDOG: eth0 (mlx4_core): transmit queue 11 timed out
> ------------[ cut here ]------------
> WARNING: at /home/aik/p/guest-kernel/net/sched/sch_generic.c:303
>
> and no IP assigned, timed out.
>
>
> This is fixed by the guest restart, first restart might not help,
> then the second restart will.
>
> The host is running the latest upstream plus the patch I am replying
> to. The guest is using initramdisk from debian bootstrap and vanilla
> v4.2 kernel, ppc64le arch, POWER8 chip, QEMU is running with 1 CPU
> and 2GB of RAM.
>
> Does this look any familiar?
>
This is completely unrelated to the compatibility problem you reported
and which this patch addresses. We will reproduce in house and post a
fix.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] net: phy: at803x: don't depend on GPIOLIB
From: Sebastian Frias @ 2016-03-21 12:48 UTC (permalink / raw)
To: Uwe Kleine-König, Daniel Mack
Cc: David S. Miller, netdev, lkml, mason, Florian Fainelli,
Mans Rullgard, Fabio Estevam, Martin Blumenstingl, Linus Walleij
In-Reply-To: <20160318191242.GQ4292@pengutronix.de>
Hi Uwe,
On 03/18/2016 08:12 PM, Uwe Kleine-König wrote:
> Hello Sebastian,
>
> On Fri, Mar 18, 2016 at 04:56:21PM +0100, Sebastian Frias wrote:
>> On 03/18/2016 01:54 PM, Uwe Kleine-König wrote:
>>> From a driver perspecitive, it would be nice if devm_gpiod_get_optional
>>> returned NULL iff the respective gpio isn't specified even with
>>> GPIOLIB=n, but this isn't sensible either because it would result in
>>> quite some gpiolib code to not being conditionally compiled on
>>> CONFIG_GPIOLIB any more.
>>
>> Let's say that was the case, what would the PHY code do?
>
> With reset gpios it might not be that critical, but consider an optional
> enable gpio. (Optional in the sense, that some device have it and others
> don't, e.g. because the pin is pulled into active level by hardware.)
>
> Now you do:
>
> gpiod = gpiod_get_optional("enable");
>
> and if gpiod now is an error pointer, you must assume that you cannot
> operate the device. And even with GPIOLIB=n (and gpiod = ERR_PTR(-ENOSYS))
> you cannot ignore the error.
Two things:
- I suppose that in such hypothetical case the dependency on GPIOLIB
would be required and thus be there
- We'd also need to check that 'gpiod' is not NULL
In the scenario at hand only some devices require a hack and
gpiod_reset=NULL is valid (ie: device will not fail probing)
>For consistency I'd recommend to do the
> same for reset even though there is a chance to get a working device.
I'm not so sure, because then information would be lost, handling both
("enable" and "reset") the same way aliases different intents and
requirements.
Indeed, only "enable" would be really mandatory, "reset" is essentially
optional (only 1 of 3 devices of the family require the hack).
Besides, if "reset" was really mandatory, we would also fail the probe
if the pointer for the reset GPIO is NULL.
Indeed, even with GPIOLIB=y the pointer can be NULL if the "reset" (or
"enable" in your scenario) is not present.
From a functionality perspective, a NULL pointer for "reset" will result
in the same behaviour as GPIOLIB=n, ie: not being able to reset the PHY.
So, the current code (your commit) and previous code (Daniel's commit)
clearly points to "reset" being optional.
Furthermore, I think we should not even register the
"link_change_notify" callback when dealing with AT803x PHYs that do not
require the hack.
Another solution (considering the callback is statically registered in
the "phy_driver" structs for all AT803x PHYs) would be for the callback
to disable itself.
Once it detects that the PHY does not require the hack, it could just
perform:
phydev->drv->link_change_notify = NULL;
or call a new generic function to do the same if encapsulation is required.
If everybody agrees I can make such change as well, but I really think
Daniel should give his opinion first.
>
>> What would you think of making at803x_link_change_notify() print a
>> message every time it should do a reset but does not has a way to do it?
>
> Then this question is obsolete because the device doesn't probe.
I think you assume that "reset" is mandatory for all AT803x devices, but
that's not what the code says.
As such, my proposal was to:
- keep my proposed patch
- make another patch to print a warning when gpiod_reset is NULL (which
can happen, even without my patch)
What do you think?
Best regards,
Sebastian
^ permalink raw reply
* Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
From: Rob Herring @ 2016-03-21 12:40 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Alexandre TORGUE, Maxime Coquelin, Giuseppe Cavallaro, netdev,
devicetree, Joachim Eastwood, linux-kernel, linux-arm-kernel
In-Reply-To: <CAGb2v65SvVi0cJbaaD9cYrqC3pJ9eumwhTVk11_N7r_nTZhS9g@mail.gmail.com>
On Sat, Mar 19, 2016 at 12:00:22AM +0800, Chen-Yu Tsai wrote:
> Hi,
>
> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE
> <alexandre.torgue@gmail.com> wrote:
> > Signed-off-by: Alexandre TORGUE <alexandre.torgue@gmail.com>
> >
> > diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> > new file mode 100644
> > index 0000000..ada2aa4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
> > @@ -0,0 +1,32 @@
> > +STMicroelectronics STM32 / MCU DWMAC glue layer controller
> > +
> > +This file documents platform glue layer for stmmac.
> > +Please see stmmac.txt for the other unchanged properties.
> > +
> > +The device node has following properties.
> > +
> > +Required properties:
> > +- compatible: Should be "st,stm32-dwmac" to select glue, and
> > + "snps,dwmac-3.50a" to select IP vesrion.
>
> If you need have sort of hardware glue, then it is not compatible.
Hence, the more specific compatible. Having the generic one is still
useful.
>
> ChenYu
>
> > +- clocks: Must contain a phandle for each entry in clock-names.
> > +- clock-names: Should be "stmmaceth" for the host clock.
This doesn't sound like the clock input signal name...
> > + Should be "tx-clk" for the MAC TX clock.
> > + Should be "rx-clk" for the MAC RX clock.
How can other DWMAC blocks not have these clocks? The glue can't really
add these clocks. It could combine them into one or a new version of
DWMAC could have a different number of clock inputs. So if there is
variation here, then some of the bindings are probably wrong. I guess
the only change I'm suggesting is possibly moving these into common
binding doc.
> > +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
> > + encompases the glue register, and the offset of the control register.
> > +Example:
> > +
> > + ethernet0: dwmac@40028000 {
ethernet@...
> > + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
> > + status = "disabled";
> > + reg = <0x40028000 0x8000>;
> > + reg-names = "stmmaceth";
> > + interrupts = <0 61 0>, <0 62 0>;
> > + interrupt-names = "macirq", "eth_wake_irq";
> > + clock-names = "stmmaceth", "tx-clk", "rx-clk";
> > + clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
> > + st,syscon = <&syscfg 0x4>;
> > + snps,pbl = <8>;
> > + snps,mixed-burst;
> > + dma-ranges;
> > + };
> > --
> > 1.9.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" 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] change nfqueue failopen to apply also to receive message buffer in addition to queue size
From: Florian Westphal @ 2016-03-21 12:22 UTC (permalink / raw)
To: Yigal Reiss (yreiss)
Cc: 'netdev@vger.kernel.org', netfilter-devel@vger.kernel.org,
Florian Westphal (fw@strlen.de), stephen
In-Reply-To: <2ba8dceec36a41149598e43f09af048e@XCH-RTP-014.cisco.com>
Yigal Reiss (yreiss) <yreiss@cisco.com> wrote:
[ CC shemminger ]
This is the place where the commit message should go.
The Subject: line alone isn't enough for a large patch like this.
> Signed-off-by: Yigal Reiss <yreiss@cisco.com>
> ---
>
> NOTE: this is a re-send after being bounced by the test robot for a compiler warning. I hope I'm doing the right thing in resubmitting rather than replying to the original message. Recompiled w/o warnings and re-tested.
Resubmit is ok.
> This is follow-up on http://marc.info/?l=netfilter-devel&m=145765526817347&w=2
Perhaps you could summarize the discussion in the commit message.
> Existing fail-open mechanism for nfqueue only applies for message that cannot be sent due to queue size (queue_maxlen). It does not apply when the failure is due to socket receive buffer size. This seems to be quite arbitrary since different packet sizes for the same queue and buffer sizes yield failure for different reasons.
>
> This patch makes both behave the same.
This should go into the commit log, not here.
> There is also a change in the proc file (/proc/net/netfilter/nfnetlink_queue) to account for the fail-opened packets.
> One change to existing behavior which I would like to stress is in the function netlink_unicast (now in netlink_unicast_nofree). In case where a call to sk_filter() returned non-zero value, netlink_unicast would set its returned error value to skb->len. I don't see how this ever made sense and I couldn't find anyone looking at this value. I changed this in order to have a consistent (err<0) return value on errors which was required for my changes. If anyone sees a problem with this change I'd like to know.
Should be done in a separate change.
It looks like a bug -- AFAICS if a sk filter is active on the nfnetlink
sk we will believe sk got queued and will put the (free'd) skb ptr on
the reinject list.
Added here:
commit b1153f29ee07dc1a788964409255a4b4fae50b98
Author: Stephen Hemminger <shemminger@vyatta.com>
netlink: make socket filters work on netlink
It looks like the intent is to hide the error from writes happening
on netlink sk, but doing so also hides it from nfnetlink_queue which
relies on skb having been attached to the sk when we don't see an error.
Maybe Stephen remembers details?
Is the error masking intentional/needed?
If so it seems we will need to refactor this a bit to
differentiate between kernel-internal caller and "called
on behalf of userspace"....
> -
> + unsigned int queue_failopened;
> + unsigned int nobuf_failopened;
Is this useful...?
Userspace already should get -ENOBUFS errors on netlink overrun.
> - seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %2d\n",
> + seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %5u %5u %2d\n",
Problematic since it changes layout of a file we unfortunately have to
view as uapi.
I would prefer if we could leave the proc file alone and not
add any new stats counters for this, unless there is a good argument
for doing so.
> + long *timeo, struct sock *ssk)
> +{
> + int ret = netlink_attachskb_nofree(sk, skb, timeo, ssk);
> + if (ret < 0)
> + kfree_skb(skb);
> + return ret;
> +}
Seems this patch is whitespace damaged.
Please send a patch to yourself and make sure it applies cleanly via git-am.
> @@ -1752,7 +1760,6 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
> sock_put(sk);
>
> if (signal_pending(current)) {
> - kfree_skb(skb);
> return sock_intr_errno(*timeo);
> }
This would make the {} unneded so these should be zapped too.
^ permalink raw reply
* Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
From: Joachim Eastwood @ 2016-03-21 12:11 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Alexandre Torgue, Maxime Coquelin, Giuseppe Cavallaro, netdev,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <CAGb2v64V_Km+i62L=L3nuzANX1eMhC2eBvKCZuVqEFdWqeTHLg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Chen-Yu,
On 21 March 2016 at 12:08, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> On Mon, Mar 21, 2016 at 6:45 PM, Alexandre Torgue
> <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi,
>>
>> 2016-03-18 17:00 GMT+01:00 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>> Hi,
>>>
>>> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE
>>> <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> Signed-off-by: Alexandre TORGUE <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>>>> new file mode 100644
>>>> index 0000000..ada2aa4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>>>> @@ -0,0 +1,32 @@
>>>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>>>> +
>>>> +This file documents platform glue layer for stmmac.
>>>> +Please see stmmac.txt for the other unchanged properties.
>>>> +
>>>> +The device node has following properties.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should be "st,stm32-dwmac" to select glue, and
>>>> + "snps,dwmac-3.50a" to select IP vesrion.
s/vesrion/version
>>>
>>> If you need have sort of hardware glue, then it is not compatible.
>>>
>>
>> We could have the case where the glue is set by a bootloader.
>> In this case, we will select IP version in compatible and we will use
>> generic dwmac glue to probe stmmac driver.
>
> It seems most platforms using DWMAC follow this design set by
> the original stmmac bindings. I'm arguing that the requirement
> of setting up the glue makes them incompatible.
>
> What happens when the bootloader didn't setup the glue? And one
> forgets to build the STM32 driver, only the generic one? The
> generic driver even matches to some, but not all, version
> strings.
>
> Maybe it would've been better if the versioned strings were
> only used to indicate functionality, and not used to bind
> the drivers. But the bindings were set some time ago.
Since Alexandre has not added "snps,dwmac-3.50a" to dwmac-generic
doesn't he use it as you suggest here?
Note that we can not remove all the generic compatible strings from
dwmac-generic because there is one platform that depend on one of
them.
(see arch/arm/boot/dts/exynos5440.dtsi:190)
So we can not remove "snps,dwmac-3.70a" from the dwmac-generic driver
if we want to keep backwards compatibility with exynos5440. But I
guess we could remove the others if we want to.
regards,
Joachim Eastwood
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFCv2 0/3] mac80211: implement fq codel
From: Michal Kazior @ 2016-03-21 11:57 UTC (permalink / raw)
To: Dave Taht
Cc: Jasmine Strong, Network Development, linux-wireless,
ath10k@lists.infradead.org, codel@lists.bufferbloat.net,
make-wifi-fast
In-Reply-To: <CAA93jw7TuBLVi9k-+2=J27j-Qs+sS3UNaQNTMF9D2MQ13fQY9A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5516 bytes --]
On 17 March 2016 at 18:00, Dave Taht <dave.taht@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 1:55 AM, Michal Kazior <michal.kazior@tieto.com> wrote:
>
>> I suspect the BK/BE latency difference has to do with the fact that
>> there's bulk traffic going on BE queues (this isn't reflected
>> explicitly in the plots). The `bursts` flent test includes short
>> bursts of traffic on tid0 (BE) which is shared with ICMP and BE UDP_RR
>> (seen as green and blue lines on the plot). Due to (intended) limited
>> outflow (6mbps) BE queues build up and don't drain for the duration of
>> the entire test creating more opportunities for aggregating BE traffic
>> while other queues are near-empty and very short (time wise as well).
>
> I agree with your explanation. Access to the media and queue length
> are the two variables at play here.
>
> I just committed a new flent test that should exercise the vo,vi,be,
> and bk queues, "bursts_11e". I dropped the conventional ping from it
> and just rely on netperf's udp_rr for each queue. It seems to "do the
> right thing" on the ath9k....
[...]
> I long for regular "rrul" and "rrul_be" tests against the new stuff to
> blow it up thoroughly as references along the way.
> (tcp_upload, tcp_download, (and several of the rtt_fair tests also
> between stations)). Will get formal about it here as soon as we end up
> on the same kernel trees....
[...]
> simple example of the damage having all 4 queues always contending is
> exemplified by running the rrul and rrul_be tests against nearly any
> given AP.
Thanks! I've run more tests and am attaching results.
A couple of words on the test naming:
- "fast" means 1x1 station with good RF conditions
- "slow" means 1x1 station with bad RF conditions (antenna unplugged)
- "fast+slow" means traffic is directed to both "fast" and "slow" stations
- "verfast" means 4x4 station for peak tput measurement
- "autorate" means rate control is enabled
- "rate6m" means 6mbps fixed tx rate on DUT
- the DUT is acting as AP in all tests
- other devices in the setup *do not* have any extra patches (so
bidirectional tests must be carefully analyzed)
- 4 sets of software patches:
- fullpatch contains all codel patches (mac80211+ath10k)
- macpatch contains only mac80211 changes (so ath10k at least gets
to use per-txq fq-codel like queuing)
- pre-waketx is ath10k with some patches reverted (before
pull-push/wake-tx-queue stuff was applied)
- waketx is current ath10k (i.e. with simple wake_tx_queue implementation)
Observations/ notes:
- "slow" case proves my naive get_expected_throughput() for ath10k is
highly inaccurate due to not considering retries. because of that
latency gets bad as mac80211's tx scheduling is queuing up more than
necessary; ath9k should do a lot better with minstrel
- i kept netperf2.6 (which has no udp-rr recovery) for now as it's
easier to spot glitches
Please let me know if you see anything interesting or worrying in these plots.
>> I've modified traffic-gen and re-run tests with bursts on all tested
>> tids/ACs (tid0, tid1, tid5). I'm attaching the results.
>>
>> With bursts on all tids you can clearly see BK has much higher latency than BE.
>
> The long term goal here, of course, is for BK (or the other queues) to
> not have seconds of queuing latency but something more bounded to 2x
> media access time...
My patch already tries to maintain txop-based in-flight tx queue
depth. Current defaults are to keep between 3-4 txops per hardware and
roughly 2txops per tid. You could argue these are too big but I wanted
to keep them conservative, at least initially, to make sure to not
affect peak throughput badly. All of these are knobs you can play with
via debugfs.
This requires drivers to use ieee80211_tx_schedule(). If driver merely
uses wake_tx_queue it will only benefit from flow fairness (albeit
limited) but it will not keep queues at N txop fill level (unless
driver does that on it's own).
This means that Tim's ath9k patch will need to be adjusted a bit to
make use of this new API prototype for full effect. Unfortunately I
didn't have time to play on this front yet.
>> (Note, I've changed my AP to QCA988X with oldie firmware 10.1.467 for
>> this test; it doesn't have the weird hiccups I was seeing on QCA99X0
>> and newer QCA988X firmware reports bogus expected throughput which is
>> most likely a result of my sloppy proof-of-concept change in ath10k).
>
> So I should avoid ben greer's firmware for now?
I'm guessing his 10.1 fork should work fine. Not sure about the 10.2.4 though.
Anyway, keep in mind you'll get mixed results with ath10k. The
throughput estimation I've done for now is an ugly hack. It works in
fixed-rate conditions (which I use to prove a point that given
adequate rate estimation you can keep fw/hw tx queues at a reasonable
latency). It doesn't consider tx retries and unstable RF conditions
(rate control is in firmware and there's limited information available
to the driver) though which leads to more frames being queued than
necessary (and therefore increasing latency). This becomes apparent
with real-life interference and tx retries (just compare
"autorate,slow" against "rate6m,fast").
ath9k should do a lot better job at this (although that requires Tim's
patches; I haven't tested that myself) because it uses minstrel which
and should predict throughput a lot more reliably.
Michał
[-- Attachment #2: flent-2016-03-21.tar.gz --]
[-- Type: application/x-gzip, Size: 2029295 bytes --]
^ permalink raw reply
* [PATCH net-next] change nfqueue failopen to apply also to receive message buffer in addition to queue size
From: Yigal Reiss (yreiss) @ 2016-03-21 11:23 UTC (permalink / raw)
To: 'netdev@vger.kernel.org', netfilter-devel@vger.kernel.org
Cc: Florian Westphal (fw@strlen.de)
Signed-off-by: Yigal Reiss <yreiss@cisco.com>
---
NOTE: this is a re-send after being bounced by the test robot for a compiler warning. I hope I'm doing the right thing in resubmitting rather than replying to the original message. Recompiled w/o warnings and re-tested.
This is follow-up on http://marc.info/?l=netfilter-devel&m=145765526817347&w=2
Existing fail-open mechanism for nfqueue only applies for message that cannot be sent due to queue size (queue_maxlen). It does not apply when the failure is due to socket receive buffer size. This seems to be quite arbitrary since different packet sizes for the same queue and buffer sizes yield failure for different reasons.
This patch makes both behave the same.
There is also a change in the proc file (/proc/net/netfilter/nfnetlink_queue) to account for the fail-opened packets.
One change to existing behavior which I would like to stress is in the function netlink_unicast (now in netlink_unicast_nofree). In case where a call to sk_filter() returned non-zero value, netlink_unicast would set its returned error value to skb->len. I don't see how this ever made sense and I couldn't find anyone looking at this value. I changed this in order to have a consistent (err<0) return value on errors which was required for my changes. If anyone sees a problem with this change I'd like to know.
include/linux/netfilter/nfnetlink.h | 2 ++
include/linux/netlink.h | 3 +++
net/netfilter/nfnetlink.c | 7 +++++++
net/netfilter/nfnetlink_queue.c | 25 ++++++++++++++++++-------
net/netlink/af_netlink.c | 37 +++++++++++++++++++++++++------------
5 files changed, 55 insertions(+), 19 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink.h b/include/linux/netfilter/nfnetlink.h
index ba0d978..eb477d4 100644
--- a/include/linux/netfilter/nfnetlink.h
+++ b/include/linux/netfilter/nfnetlink.h
@@ -39,6 +39,8 @@ struct sk_buff *nfnetlink_alloc_skb(struct net *net, unsigned int size,
int nfnetlink_send(struct sk_buff *skb, struct net *net, u32 portid,
unsigned int group, int echo, gfp_t flags);
int nfnetlink_set_err(struct net *net, u32 portid, u32 group, int error);
+int nfnetlink_unicast_nofree(struct sk_buff *skb, struct net *net, u32 portid,
+ int flags);
int nfnetlink_unicast(struct sk_buff *skb, struct net *net, u32 portid,
int flags);
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 0b41959..9f7a819 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -79,6 +79,7 @@ netlink_alloc_skb(struct sock *ssk, unsigned int size, u32 dst_portid,
return __netlink_alloc_skb(ssk, size, 0, dst_portid, gfp_mask);
}
+extern int netlink_unicast_nofree(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 portid, int nonblock);
extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 portid,
__u32 group, gfp_t allocation);
@@ -92,6 +93,8 @@ extern int netlink_unregister_notifier(struct notifier_block *nb);
/* finegrained unicast helpers: */
struct sock *netlink_getsockbyfilp(struct file *filp);
+int netlink_attachskb_nofree(struct sock *sk, struct sk_buff *skb,
+ long *timeo, struct sock *ssk);
int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
long *timeo, struct sock *ssk);
void netlink_detachskb(struct sock *sk, struct sk_buff *skb);
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 857ae89..28f7e2d 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -154,6 +154,13 @@ int nfnetlink_unicast(struct sk_buff *skb, struct net *net, u32 portid,
}
EXPORT_SYMBOL_GPL(nfnetlink_unicast);
+int nfnetlink_unicast_nofree(struct sk_buff *skb, struct net *net, u32 portid,
+ int flags)
+{
+ return netlink_unicast_nofree(net->nfnl, skb, portid, flags);
+}
+EXPORT_SYMBOL_GPL(nfnetlink_unicast_nofree);
+
/* Process one complete nfnetlink message. */
static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
{
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 1d39365..3d32153 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -60,7 +60,8 @@ struct nfqnl_instance {
unsigned int copy_range;
unsigned int queue_dropped;
unsigned int queue_user_dropped;
-
+ unsigned int queue_failopened;
+ unsigned int nobuf_failopened;
u_int16_t queue_num; /* number of this queue */
u_int8_t copy_mode;
@@ -551,6 +552,7 @@ nla_put_failure:
return NULL;
}
+
static int
__nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
struct nf_queue_entry *entry)
@@ -569,6 +571,7 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
if (queue->queue_total >= queue->queue_maxlen) {
if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
+ queue->queue_failopened++;
failopen = 1;
err = 0;
} else {
@@ -582,10 +585,17 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
*packet_id_ptr = htonl(entry->id);
/* nfnetlink_unicast will either free the nskb or add it to a socket */
- err = nfnetlink_unicast(nskb, net, queue->peer_portid, MSG_DONTWAIT);
+ err = nfnetlink_unicast_nofree(nskb, net, queue->peer_portid, MSG_DONTWAIT);
if (err < 0) {
- queue->queue_user_dropped++;
- goto err_out_unlock;
+ if (queue->flags & NFQA_CFG_F_FAIL_OPEN) {
+ queue->nobuf_failopened++;
+ failopen = 1;
+ err = 0;
+ }
+ else {
+ queue->queue_user_dropped++;
+ }
+ goto err_out_free_nskb;
}
__enqueue_entry(queue, entry);
@@ -595,7 +605,6 @@ __nfqnl_enqueue_packet(struct net *net, struct nfqnl_instance *queue,
err_out_free_nskb:
kfree_skb(nskb);
-err_out_unlock:
spin_unlock_bh(&queue->lock);
if (failopen)
nf_reinject(entry, NF_ACCEPT);
@@ -1327,12 +1336,14 @@ static int seq_show(struct seq_file *s, void *v)
{
const struct nfqnl_instance *inst = v;
- seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %2d\n",
+ seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %5u %5u %2d\n",
inst->queue_num,
inst->peer_portid, inst->queue_total,
inst->copy_mode, inst->copy_range,
inst->queue_dropped, inst->queue_user_dropped,
- inst->id_sequence, 1);
+ inst->id_sequence,
+ inst->queue_failopened, inst->nobuf_failopened,
+ 2);
return 0;
}
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index f1ffb34..0191cdf 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1710,17 +1710,26 @@ static struct sk_buff *netlink_alloc_large_skb(unsigned int size,
return skb;
}
+int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
+ long *timeo, struct sock *ssk)
+{
+ int ret = netlink_attachskb_nofree(sk, skb, timeo, ssk);
+ if (ret < 0)
+ kfree_skb(skb);
+ return ret;
+}
+
/*
* Attach a skb to a netlink socket.
* The caller must hold a reference to the destination socket. On error, the
* reference is dropped. The skb is not send to the destination, just all
* all error checks are performed and memory in the queue is reserved.
* Return values:
- * < 0: error. skb freed, reference to sock dropped.
+ * < 0: error. reference to sock dropped.
* 0: continue
* 1: repeat lookup - reference dropped while waiting for socket memory.
*/
-int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
+int netlink_attachskb_nofree(struct sock *sk, struct sk_buff *skb,
long *timeo, struct sock *ssk)
{
struct netlink_sock *nlk;
@@ -1735,7 +1744,6 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
if (!ssk || netlink_is_kernel(ssk))
netlink_overrun(sk);
sock_put(sk);
- kfree_skb(skb);
return -EAGAIN;
}
@@ -1752,7 +1760,6 @@ int netlink_attachskb(struct sock *sk, struct sk_buff *skb,
sock_put(sk);
if (signal_pending(current)) {
- kfree_skb(skb);
return sock_intr_errno(*timeo);
}
return 1;
@@ -1833,8 +1840,6 @@ static int netlink_unicast_kernel(struct sock *sk, struct sk_buff *skb,
netlink_deliver_tap_kernel(sk, ssk, skb);
nlk->netlink_rcv(skb);
consume_skb(skb);
- } else {
- kfree_skb(skb);
}
sock_put(sk);
return ret;
@@ -1843,6 +1848,16 @@ static int netlink_unicast_kernel(struct sock *sk, struct sk_buff *skb,
int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
u32 portid, int nonblock)
{
+ int ret = netlink_unicast_nofree(ssk, skb, portid, nonblock);
+ if (ret < 0)
+ kfree_skb(skb);
+ return ret;
+}
+EXPORT_SYMBOL(netlink_unicast);
+
+int netlink_unicast_nofree(struct sock *ssk, struct sk_buff *skb,
+ u32 portid, int nonblock)
+{
struct sock *sk;
int err;
long timeo;
@@ -1853,20 +1868,18 @@ int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
retry:
sk = netlink_getsockbyportid(ssk, portid);
if (IS_ERR(sk)) {
- kfree_skb(skb);
return PTR_ERR(sk);
}
if (netlink_is_kernel(sk))
return netlink_unicast_kernel(sk, skb, ssk);
- if (sk_filter(sk, skb)) {
- err = skb->len;
- kfree_skb(skb);
+ err = sk_filter(sk, skb);
+ if (err) {
sock_put(sk);
return err;
}
- err = netlink_attachskb(sk, skb, &timeo, ssk);
+ err = netlink_attachskb_nofree(sk, skb, &timeo, ssk);
if (err == 1)
goto retry;
if (err)
@@ -1874,7 +1887,7 @@ retry:
return netlink_sendskb(sk, skb);
}
-EXPORT_SYMBOL(netlink_unicast);
+EXPORT_SYMBOL(netlink_unicast_nofree);
struct sk_buff *__netlink_alloc_skb(struct sock *ssk, unsigned int size,
unsigned int ldiff, u32 dst_portid,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v5 2/4] Documentation: Bindings: Add STM32 DWMAC glue
From: Chen-Yu Tsai @ 2016-03-21 11:08 UTC (permalink / raw)
To: Alexandre Torgue
Cc: Chen-Yu Tsai, Maxime Coquelin, Giuseppe Cavallaro, netdev,
devicetree, Joachim Eastwood, linux-kernel, linux-arm-kernel
In-Reply-To: <CAJgp7zxyQGrSSvNszawBQfS+omgKi1i35uW-eDyLdW-jP7oRjg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, Mar 21, 2016 at 6:45 PM, Alexandre Torgue
<alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi,
>
> 2016-03-18 17:00 GMT+01:00 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>> Hi,
>>
>> On Fri, Mar 18, 2016 at 11:37 PM, Alexandre TORGUE
>> <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Signed-off-by: Alexandre TORGUE <alexandre.torgue-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>>> new file mode 100644
>>> index 0000000..ada2aa4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt
>>> @@ -0,0 +1,32 @@
>>> +STMicroelectronics STM32 / MCU DWMAC glue layer controller
>>> +
>>> +This file documents platform glue layer for stmmac.
>>> +Please see stmmac.txt for the other unchanged properties.
>>> +
>>> +The device node has following properties.
>>> +
>>> +Required properties:
>>> +- compatible: Should be "st,stm32-dwmac" to select glue, and
>>> + "snps,dwmac-3.50a" to select IP vesrion.
>>
>> If you need have sort of hardware glue, then it is not compatible.
>>
>
> We could have the case where the glue is set by a bootloader.
> In this case, we will select IP version in compatible and we will use
> generic dwmac glue to probe stmmac driver.
It seems most platforms using DWMAC follow this design set by
the original stmmac bindings. I'm arguing that the requirement
of setting up the glue makes them incompatible.
What happens when the bootloader didn't setup the glue? And one
forgets to build the STM32 driver, only the generic one? The
generic driver even matches to some, but not all, version
strings.
Maybe it would've been better if the versioned strings were
only used to indicate functionality, and not used to bind
the drivers. But the bindings were set some time ago.
Regards
ChenYu
> Regards
>
> Alex.
>
>> ChenYu
>>
>>> +- clocks: Must contain a phandle for each entry in clock-names.
>>> +- clock-names: Should be "stmmaceth" for the host clock.
>>> + Should be "tx-clk" for the MAC TX clock.
>>> + Should be "rx-clk" for the MAC RX clock.
>>> +- st,syscon : Should be phandle/offset pair. The phandle to the syscon node which
>>> + encompases the glue register, and the offset of the control register.
>>> +Example:
>>> +
>>> + ethernet0: dwmac@40028000 {
>>> + compatible = "st,stm32-dwmac", "snps,dwmac-3.50a";
>>> + status = "disabled";
>>> + reg = <0x40028000 0x8000>;
>>> + reg-names = "stmmaceth";
>>> + interrupts = <0 61 0>, <0 62 0>;
>>> + interrupt-names = "macirq", "eth_wake_irq";
>>> + clock-names = "stmmaceth", "tx-clk", "rx-clk";
>>> + clocks = <&rcc 0 25>, <&rcc 0 26>, <&rcc 0 27>;
>>> + st,syscon = <&syscfg 0x4>;
>>> + snps,pbl = <8>;
>>> + snps,mixed-burst;
>>> + dma-ranges;
>>> + };
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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
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