* Re: [PATCH net] devlink: double free in devlink_resource_fill()
From: Jiri Pirko @ 2018-09-23 17:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Jiri Pirko, David S. Miller, netdev, kernel-janitors
In-Reply-To: <20180921080755.GA16307@mwanda>
Fri, Sep 21, 2018 at 10:07:55AM CEST, dan.carpenter@oracle.com wrote:
>Smatch reports that devlink_dpipe_send_and_alloc_skb() frees the skb
>on error so this is a double free. We fixed a bunch of these bugs in
>commit 7fe4d6dcbcb4 ("devlink: Remove redundant free on error path") but
>we accidentally overlooked this one.
>
>Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
>Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path
From: Janusz Krzysztofik @ 2018-09-23 23:53 UTC (permalink / raw)
To: Linus Walleij
Cc: Andrew Lunn, Ulf Hansson, linux-doc, Tony Lindgren,
Dominik Brodowski, Peter Rosin, netdev, linux-i2c,
Peter Meerwald-Stadler, Marek Szyprowski, devel, Florian Fainelli,
Jonathan Corbet, Janusz Krzysztofik, Krzysztof Kozlowski,
Kishon Vijay Abraham I, linux-iio, Peter Korsgaard,
Geert Uytterhoeven, linux-serial, Jiri Slaby, Michael Hennerich,
Uwe Kleine-König, linux-gpio
In-Reply-To: <2785169.v6aIfS3K2k@z50>
While investigating possible reasons of GPIO fast bitmap processing
related boot hang on Samsung Snow Chromebook, reported by Marek
Szyprowski (thanks!), I've discovered one coding bug, addressed by
PATCH 1/2 of this series, and one potential regression introduced at
design level of the solution, hopefully fixed by PATCH 2/2. See
commit messages for details.
Janusz Krzysztofik (2):
gpiolib: Fix missing updates of bitmap index
gpiolib: Fix array members of same chip processed separately
The fixes should resolve the boot hang observed by Marek, however the
second change excludes that particular case from fast bitmap processing
and restores the old behaviour. Hence, it is possible still another
issue which have had an influence on that boot hang exists in the code.
In order to fully verify the fix, it would have to be tested on a
platform where an array of GPIO descriptors is used which starts from
at least two consecutive pins of one GPIO chip in hardware order,
starting ftom 0, followed by one or more pins belonging to other
chip(s).
In order to verify if separate calls to .set() chip callback for each
pin instead of one call to .set_multiple() is actually the reason of
boot hang on Samsung Snow Chromebook, the affected driver -
drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
modified for testing purposes so it calls gpiod_set_value() for each
pin instead of gpiod_set_array_value() for all of them. If that would
also result in boot hang, we could be sure the issue was really the
one addressed by the second fix. Marek, could you please try to
perform such test?
Thanks,
Janusz
diffstat:
Documentation/driver-api/gpio/board.rst | 19 +++++++++----
drivers/gpio/gpiolib.c | 46 +++++++++++++++++++++-----------
2 files changed, 45 insertions(+), 20 deletions(-)
^ permalink raw reply
* [PATCH 1/2] gpiolib: Fix missing updates of bitmap index
From: Janusz Krzysztofik @ 2018-09-23 23:53 UTC (permalink / raw)
To: Linus Walleij
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg Kroah-Hartman,
Kishon Vijay Abraham I, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
Jiri Slaby, Willy Tarreau, Geert Uytterhoeven
In-Reply-To: <20180923235336.22148-1-jmkrzyszt@gmail.com>
In new code introduced by commit b17566a6b08b ("gpiolib: Implement fast
processing path in get/set array"), bitmap index is not updated with
next found zero bit position as it should while skipping over pins
already processed via fast bitmap path, possibly resulting in an
infinite loop. Fix it.
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
drivers/gpio/gpiolib.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a53d17745d21..7d9536a79a66 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2880,8 +2880,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
__set_bit(hwgpio, mask);
if (array_info)
- find_next_zero_bit(array_info->get_mask,
- array_size, i);
+ i = find_next_zero_bit(array_info->get_mask,
+ array_size, i);
else
i++;
} while ((i < array_size) &&
@@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
trace_gpio_value(desc_to_gpio(desc), 1, value);
if (array_info)
- find_next_zero_bit(array_info->get_mask, i, j);
+ j = find_next_zero_bit(array_info->get_mask, i,
+ j);
else
j++;
}
@@ -3192,8 +3193,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
}
if (array_info)
- find_next_zero_bit(array_info->set_mask,
- array_size, i);
+ i = find_next_zero_bit(array_info->set_mask,
+ array_size, i);
else
i++;
} while ((i < array_size) &&
--
2.16.4
^ permalink raw reply related
* [PATCH 2/2] gpiolib: Fix array members of same chip processed separately
From: Janusz Krzysztofik @ 2018-09-23 23:53 UTC (permalink / raw)
To: Linus Walleij
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg Kroah-Hartman,
Kishon Vijay Abraham I, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
Jiri Slaby, Willy Tarreau, Geert Uytterhoeven
In-Reply-To: <20180923235336.22148-1-jmkrzyszt@gmail.com>
New code introduced by commit bf9346f5d47b ("gpiolib: Identify arrays
matching GPIO hardware") forcibly tries to find an array member which
has its array index number equal to its hardware pin number and set
up an array info for possible fast bitmap processing of all arrray
pins belonging to that chip which also satisfy that numbering rule.
Depending on array content, it may happen that consecutive array
members which belong to the same chip but don't have array indexes
equal to their pin hardware numbers will be split into groups, some of
them processed together via the fast bitmap path, and rest of them
separetely. However, applications may expect all those pins being
processed together with a single call to .set_multiple() chip callback,
like that was done before the change.
Limit applicability of fast bitmap processing path to cases where all
pins of consecutive array members starting from 0 which belong to the
same chip have their hardware numbers equal to their corresponding
array indexes. That should still speed up processing of applications
using whole GPIO banks as I/O ports, while not breaking simultaneous
manipulation of consecutive pins of the same chip which don't follow
the equal numbering rule.
Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
Documentation/driver-api/gpio/board.rst | 19 +++++++++++++-----
drivers/gpio/gpiolib.c | 35 +++++++++++++++++++++++----------
2 files changed, 39 insertions(+), 15 deletions(-)
diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst
index c66821e033c2..a0f294e2e250 100644
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -202,9 +202,18 @@ mapped to the device determines if the array qualifies for fast bitmap
processing. If yes, a bitmap is passed over get/set array functions directly
between a caller and a respective .get/set_multiple() callback of a GPIO chip.
-In order to qualify for fast bitmap processing, the pin mapping must meet the
+In order to qualify for fast bitmap processing, the array must meet the
following requirements:
-- it must belong to the same chip as other 'fast' pins of the function,
-- its index within the function must match its hardware number within the chip.
-
-Open drain and open source pins are excluded from fast bitmap output processing.
+- pin hardware number of array member 0 must also be 0,
+- pin hardware numbers of consecutive array members which belong to the same
+ chip as member 0 does must also match their array indexes.
+
+Otherwise fast bitmap processing path is not used in order to avoid consecutive
+pins which belong to the same chip but are not in hardware order being processed
+separately.
+
+If the array applies for fast bitmap processing path, pins which belong to
+different chips than member 0 does, as well as those with indexes different from
+their hardware pin numbers, are excluded from the fast path, both input and
+output. Moreover, open drain and open source pins are excluded from fast bitmap
+output processing.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7d9536a79a66..6ae13e3e05f1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4376,11 +4376,10 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
chip = gpiod_to_chip(desc);
/*
- * Select a chip of first array member
- * whose index matches its pin hardware number
- * as a candidate for fast bitmap processing.
+ * If pin hardware number of array member 0 is also 0, select
+ * its chip as a candidate for fast bitmap processing path.
*/
- if (!array_info && gpio_chip_hwgpio(desc) == descs->ndescs) {
+ if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
struct gpio_descs *array;
bitmap_size = BITS_TO_LONGS(chip->ngpio > count ?
@@ -4414,14 +4413,30 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
count - descs->ndescs);
descs->info = array_info;
}
- /*
- * Unmark members which don't qualify for fast bitmap
- * processing (different chip, not in hardware order)
- */
- if (array_info && (chip != array_info->chip ||
- gpio_chip_hwgpio(desc) != descs->ndescs)) {
+ /* Unmark array members which don't belong to the 'fast' chip */
+ if (array_info && array_info->chip != chip) {
__clear_bit(descs->ndescs, array_info->get_mask);
__clear_bit(descs->ndescs, array_info->set_mask);
+ }
+ /*
+ * Detect array members which belong to the 'fast' chip
+ * but their pins are not in hardware order.
+ */
+ else if (array_info &&
+ gpio_chip_hwgpio(desc) != descs->ndescs) {
+ /*
+ * Don't use fast path if all array members processed so
+ * far belong to the same chip as this one but its pin
+ * hardware number is different from its array index.
+ */
+ if (bitmap_full(array_info->get_mask, descs->ndescs)) {
+ array_info = NULL;
+ } else {
+ __clear_bit(descs->ndescs,
+ array_info->get_mask);
+ __clear_bit(descs->ndescs,
+ array_info->set_mask);
+ }
} else if (array_info) {
/* Exclude open drain or open source from fast output */
if (gpiochip_line_is_open_drain(chip, descs->ndescs) ||
--
2.16.4
^ permalink raw reply related
* linux-next: manual merge of the bpf-next tree with Linus' tree
From: Stephen Rothwell @ 2018-09-24 0:19 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
Arnaldo Carvalho de Melo, Yonghong Song
[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]
Hi all,
Today's linux-next merge of the bpf-next tree got a conflict in:
tools/lib/bpf/Build
between commit:
6d41907c630d ("tools lib bpf: Provide wrapper for strerror_r to build in !_GNU_SOURCE systems")
from Linus' tree and commit:
f7010770fbac ("tools/bpf: move bpf/lib netlink related functions into a new file")
from the bpf-next tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc tools/lib/bpf/Build
index 6eb9bacd1948,512b2c0ba0d2..000000000000
--- a/tools/lib/bpf/Build
+++ b/tools/lib/bpf/Build
@@@ -1,1 -1,1 +1,1 @@@
- libbpf-y := libbpf.o bpf.o nlattr.o btf.o libbpf_errno.o str_error.o
-libbpf-y := libbpf.o bpf.o nlattr.o btf.o libbpf_errno.o netlink.o
++libbpf-y := libbpf.o bpf.o nlattr.o btf.o libbpf_errno.o str_error.o netlink.o
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH v3 1/2] netfilter: nf_tables: add SECMARK support
From: Christian Göttsche @ 2018-09-23 18:26 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, netfilter-devel, coreteam, netdev,
linux-kernel, paul, sds, eparis, jmorris, serge, selinux,
linux-security-module
Add the ability to set the security context of packets within the nf_tables framework.
Add a nft_object for holding security contexts in the kernel and manipulating packets on the wire.
Convert the security context strings at rule addition time to security identifiers.
This is the same behavior like in xt_SECMARK and offers better performance than computing it per packet.
Set the maximum security context length to 256.
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3: switch context string from char[] to char *
rename function to nft_secmark_compute_secid()
v2: convert security context strings to ids on rule addition time
Based on nf-next
Tested with v4.18.8
include/net/netfilter/nf_tables_core.h | 4 +
include/uapi/linux/netfilter/nf_tables.h | 18 +++-
net/netfilter/nf_tables_core.c | 28 +++++-
net/netfilter/nft_meta.c | 107 +++++++++++++++++++++++
4 files changed, 152 insertions(+), 5 deletions(-)
diff --git a/include/net/netfilter/nf_tables_core.h b/include/net/netfilter/nf_tables_core.h
index 8da837d2a..2046d104f 100644
--- a/include/net/netfilter/nf_tables_core.h
+++ b/include/net/netfilter/nf_tables_core.h
@@ -16,6 +16,10 @@ extern struct nft_expr_type nft_meta_type;
extern struct nft_expr_type nft_rt_type;
extern struct nft_expr_type nft_exthdr_type;
+#ifdef CONFIG_NETWORK_SECMARK
+extern struct nft_object_type nft_secmark_obj_type;
+#endif
+
int nf_tables_core_module_init(void);
void nf_tables_core_module_exit(void);
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 702e4f0be..5444e7687 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -1176,6 +1176,21 @@ enum nft_quota_attributes {
};
#define NFTA_QUOTA_MAX (__NFTA_QUOTA_MAX - 1)
+/**
+ * enum nft_secmark_attributes - nf_tables secmark object netlink attributes
+ *
+ * @NFTA_SECMARK_CTX: security context (NLA_STRING)
+ */
+enum nft_secmark_attributes {
+ NFTA_SECMARK_UNSPEC,
+ NFTA_SECMARK_CTX,
+ __NFTA_SECMARK_MAX,
+};
+#define NFTA_SECMARK_MAX (__NFTA_SECMARK_MAX - 1)
+
+/* Max security context length */
+#define NFT_SECMARK_CTX_MAXLEN 256
+
/**
* enum nft_reject_types - nf_tables reject expression reject types
*
@@ -1432,7 +1447,8 @@ enum nft_ct_timeout_timeout_attributes {
#define NFT_OBJECT_CONNLIMIT 5
#define NFT_OBJECT_TUNNEL 6
#define NFT_OBJECT_CT_TIMEOUT 7
-#define __NFT_OBJECT_MAX 8
+#define NFT_OBJECT_SECMARK 8
+#define __NFT_OBJECT_MAX 9
#define NFT_OBJECT_MAX (__NFT_OBJECT_MAX - 1)
/**
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index ffd5c0f94..3fbce3b9c 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -249,12 +249,24 @@ static struct nft_expr_type *nft_basic_types[] = {
&nft_exthdr_type,
};
+static struct nft_object_type *nft_basic_objects[] = {
+#ifdef CONFIG_NETWORK_SECMARK
+ &nft_secmark_obj_type,
+#endif
+};
+
int __init nf_tables_core_module_init(void)
{
- int err, i;
+ int err, i, j = 0;
+
+ for (i = 0; i < ARRAY_SIZE(nft_basic_objects); i++) {
+ err = nft_register_obj(nft_basic_objects[i]);
+ if (err)
+ goto err;
+ }
- for (i = 0; i < ARRAY_SIZE(nft_basic_types); i++) {
- err = nft_register_expr(nft_basic_types[i]);
+ for (j = 0; j < ARRAY_SIZE(nft_basic_types); j++) {
+ err = nft_register_expr(nft_basic_types[j]);
if (err)
goto err;
}
@@ -262,8 +274,12 @@ int __init nf_tables_core_module_init(void)
return 0;
err:
+ while (j-- > 0)
+ nft_unregister_expr(nft_basic_types[j]);
+
while (i-- > 0)
- nft_unregister_expr(nft_basic_types[i]);
+ nft_unregister_obj(nft_basic_objects[i]);
+
return err;
}
@@ -274,4 +290,8 @@ void nf_tables_core_module_exit(void)
i = ARRAY_SIZE(nft_basic_types);
while (i-- > 0)
nft_unregister_expr(nft_basic_types[i]);
+
+ i = ARRAY_SIZE(nft_basic_objects);
+ while (i-- > 0)
+ nft_unregister_obj(nft_basic_objects[i]);
}
diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
index 297fe7d97..c8ac0ef4b 100644
--- a/net/netfilter/nft_meta.c
+++ b/net/netfilter/nft_meta.c
@@ -543,3 +543,110 @@ struct nft_expr_type nft_meta_type __read_mostly = {
.maxattr = NFTA_META_MAX,
.owner = THIS_MODULE,
};
+
+#ifdef CONFIG_NETWORK_SECMARK
+
+struct nft_secmark {
+ u32 secid;
+ char *ctx;
+};
+
+static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
+ [NFTA_SECMARK_CTX] = { .type = NLA_STRING, .len = NFT_SECMARK_CTX_MAXLEN },
+};
+
+static int nft_secmark_compute_secid(struct nft_secmark *priv)
+{
+ int err;
+ u32 tmp_secid = 0;
+
+ err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
+ if (err)
+ return err;
+
+ if (!tmp_secid)
+ return -ENOENT;
+
+ err = security_secmark_relabel_packet(tmp_secid);
+ if (err)
+ return err;
+
+ priv->secid = tmp_secid;
+ return 0;
+}
+
+static void nft_secmark_obj_eval(struct nft_object *obj, struct nft_regs *regs, const struct nft_pktinfo *pkt)
+{
+ const struct nft_secmark *priv = nft_obj_data(obj);
+ struct sk_buff *skb = pkt->skb;
+
+ skb->secmark = priv->secid;
+}
+
+
+static int nft_secmark_obj_init(const struct nft_ctx *ctx, const struct nlattr * const tb[], struct nft_object *obj)
+{
+ int err;
+ struct nft_secmark *priv = nft_obj_data(obj);
+
+ if (tb[NFTA_SECMARK_CTX] == NULL)
+ return -EINVAL;
+
+ priv->ctx = nla_strdup(tb[NFTA_SECMARK_CTX], GFP_KERNEL);
+ if (!priv->ctx)
+ return -ENOMEM;
+
+ err = nft_secmark_compute_secid(priv);
+ if (err) {
+ kfree(priv->ctx);
+ return err;
+ }
+
+ security_secmark_refcount_inc();
+
+ return 0;
+}
+
+static int nft_secmark_obj_dump(struct sk_buff *skb, struct nft_object *obj, bool reset)
+{
+ int err;
+ struct nft_secmark *priv = nft_obj_data(obj);
+
+ if (nla_put_string(skb, NFTA_SECMARK_CTX, priv->ctx))
+ return -1;
+
+ if (reset) {
+ err = nft_secmark_compute_secid(priv);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static void nft_secmark_obj_destroy(const struct nft_ctx *ctx, struct nft_object *obj)
+{
+ struct nft_secmark *priv = nft_obj_data(obj);
+
+ security_secmark_refcount_dec();
+
+ kfree(priv->ctx);
+}
+
+static const struct nft_object_ops nft_secmark_obj_ops = {
+ .type = &nft_secmark_obj_type,
+ .size = sizeof(struct nft_secmark),
+ .init = nft_secmark_obj_init,
+ .eval = nft_secmark_obj_eval,
+ .dump = nft_secmark_obj_dump,
+ .destroy = nft_secmark_obj_destroy,
+};
+struct nft_object_type nft_secmark_obj_type __read_mostly = {
+ .type = NFT_OBJECT_SECMARK,
+ .ops = &nft_secmark_obj_ops,
+ .maxattr = NFTA_SECMARK_MAX,
+ .policy = nft_secmark_policy,
+ .owner = THIS_MODULE,
+};
+
+#endif /* CONFIG_NETWORK_SECMARK */
--
2.19.0
^ permalink raw reply related
* Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers
From: David Miller @ 2018-09-23 19:29 UTC (permalink / raw)
To: edumazet
Cc: netdev, michael.chan, ariel.elior, eric.dumazet, tariqt, saeedm,
jeffrey.t.kirsher, jakub.kicinski, songliubraving, j.vosburgh,
vfalico, andy
In-Reply-To: <20180921222752.101307-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 21 Sep 2018 15:27:37 -0700
> As diagnosed by Song Liu, ndo_poll_controller() can
> be very dangerous on loaded hosts, since the cpu
> calling ndo_poll_controller() might steal all NAPI
> contexts (for all RX/TX queues of the NIC).
>
> This capture, showing one ksoftirqd eating all cycles
> can last for unlimited amount of time, since one
> cpu is generally not able to drain all the queues under load.
>
> It seems that all networking drivers that do use NAPI
> for their TX completions, should not provide a ndo_poll_controller() :
>
> Most NAPI drivers have netpoll support already handled
> in core networking stack, since netpoll_poll_dev()
> uses poll_napi(dev) to iterate through registered
> NAPI contexts for a device.
I'm having trouble understanding the difference.
If the drivers are processing all of the RX/TX queue draining by hand
in their ndo_poll_controller() method, how is that different from the
generic code walking all of the registererd NAPI instances one by one?
^ permalink raw reply
* Re: [PATCH net v2] net: phy: fix WoL handling when suspending the PHY
From: Heiner Kallweit @ 2018-09-23 19:30 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <75BE6B3C-DB93-4892-9059-E263374D934B@gmail.com>
On 23.09.2018 18:49, Florian Fainelli wrote:
>
>
> On September 23, 2018 6:38:21 AM PDT, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Actually there's nothing wrong with the two changes marked as "Fixes",
>> they just revealed a problem which has been existing before.
>> After having switched r8169 to phylib it was reported that WoL from
>> shutdown doesn't work any longer (WoL from suspend isn't affected).
>> Reason is that during shutdown phy_disconnect()->phy_detach()->
>> phy_suspend() is called.
>> A similar issue occurs when the phylib state machine calls
>> phy_suspend() when handling state PHY_HALTED.
>>
>> Core of the problem is that phy_suspend() suspends the PHY when it
>> should not due to WoL. phy_suspend() checks for WoL already, but this
>> works only if the PHY driver handles WoL (what is rarely the case).
>> Typically WoL is handled by the MAC driver.
>>
>> phylib knows about this and handles it in mdio_bus_phy_may_suspend(),
>> but that's used only when suspending the system, not in other cases
>> like shutdown.
>>
>> Therefore factor out the relevant check from
>> mdio_bus_phy_may_suspend() to a new function phy_may_suspend() and
>> use it in phy_suspend().
>>
>> Last but not least change phy_detach() to call phy_suspend() before
>> attached_dev is set to NULL. phy_suspend() accesses attached_dev
>> when checking whether the MAC driver activated WoL.
>
> The rationale for the change makes sense but I am worried about a couple of things:
>
> - we have seen drivers fail to call SET_NETDEV_DEV() correctly, or sometimes it is made possible because they are Device Tree only objects without a backing parent struct device (CONFIG_OF makes it possible), so we could be missing some cases here but this is not a big deal
>
> - most Ethernet controller implementations typically set the device as a wake-up enabled device when an user issues the appropriate ethtool -s iface wol <parameters> and not at driver probe or ndo_open() time. There are exceptions like ASIX USB Ethernet adapters that AFAIR are wake-up enabled all the time. The main concern here is that at the time of suspend device_may_wakeup() evaluates to true and we keep the PHY on even though the driver/user was not requesting for WoL per-se
>
> What you encode here is definitely the behavior that Ethernet controllers should have, but we should also audit drivers that use PHYLIB and implement WoL that this is not going to regresses their power management or ability to wake up from PHY, and finally, that there is not redundant code already in place.
>
Good points. It's somehow unfortunate that we don't have a clear info
whether WoL is enabled for a netdevice or not. Instead we have to
evaluate different hints and make an educated guess (which can be
wrong like in the potential cases you described).
One idea that comes to my mind: Add a flag wol_enabled to struct net_device
and set it in ethtool_set_wol. Then we could use this new flag in phy_suspend().
This would transparently fix the issue for the most common cases.
Still we would have to think about cases like WoL being enabled w/o explicitly
configuring it (e.g. BIOS enables WoL).
What do you think?
>>
>> Fixes: f1e911d5d0df ("r8169: add basic phylib support")
>> Fixes: e8cfd9d6c772 ("net: phy: call state machine synchronously in
>> phy_stop")
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - improved commit message
>> - reduced scope of patch, don't touch functionality of
>> mdio_bus_phy_suspend and mdio_bus_phy_resume
>> ---
>> drivers/net/phy/phy_device.c | 42 ++++++++++++++++++++++--------------
>> 1 file changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c
>> b/drivers/net/phy/phy_device.c
>> index af64a9320..4cab94bae 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -75,6 +75,26 @@ extern struct phy_driver genphy_10g_driver;
>> static LIST_HEAD(phy_fixup_list);
>> static DEFINE_MUTEX(phy_fixup_lock);
>>
>> +static bool phy_may_suspend(struct phy_device *phydev)
>> +{
>> + struct net_device *netdev = phydev->attached_dev;
>> +
>> + if (!netdev)
>> + return true;
>> +
>> + /* Don't suspend PHY if the attached netdev parent may wakeup.
>> + * The parent may point to a PCI device, as in tg3 driver.
>> + */
>> + if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
>> + return false;
>> +
>> + /* Also don't suspend PHY if the netdev itself may wakeup. This
>> + * is the case for devices w/o underlaying pwr. mgmt. aware bus,
>> + * e.g. SoC devices.
>> + */
>> + return !device_may_wakeup(&netdev->dev);
>> +}
>> +
>> #ifdef CONFIG_PM
>> static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
>> {
>> @@ -93,20 +113,7 @@ static bool mdio_bus_phy_may_suspend(struct
>> phy_device *phydev)
>> if (!netdev)
>> return !phydev->suspended;
>>
>> - /* Don't suspend PHY if the attached netdev parent may wakeup.
>> - * The parent may point to a PCI device, as in tg3 driver.
>> - */
>> - if (netdev->dev.parent && device_may_wakeup(netdev->dev.parent))
>> - return false;
>> -
>> - /* Also don't suspend PHY if the netdev itself may wakeup. This
>> - * is the case for devices w/o underlaying pwr. mgmt. aware bus,
>> - * e.g. SoC devices.
>> - */
>> - if (device_may_wakeup(&netdev->dev))
>> - return false;
>> -
>> - return true;
>> + return phy_may_suspend(phydev);
>> }
>>
>> static int mdio_bus_phy_suspend(struct device *dev)
>> @@ -1132,9 +1139,9 @@ void phy_detach(struct phy_device *phydev)
>> sysfs_remove_link(&dev->dev.kobj, "phydev");
>> sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev");
>> }
>> + phy_suspend(phydev);
>> phydev->attached_dev->phydev = NULL;
>> phydev->attached_dev = NULL;
>> - phy_suspend(phydev);
>> phydev->phylink = NULL;
>>
>> phy_led_triggers_unregister(phydev);
>> @@ -1171,9 +1178,12 @@ int phy_suspend(struct phy_device *phydev)
>> struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
>> int ret = 0;
>>
>> + if (phydev->suspended)
>> + return 0;
>> +
>> /* If the device has WOL enabled, we cannot suspend the PHY */
>> phy_ethtool_get_wol(phydev, &wol);
>> - if (wol.wolopts)
>> + if (wol.wolopts || !phy_may_suspend(phydev))
>> return -EBUSY;
>>
>> if (phydev->drv && phydrv->suspend)
>
^ permalink raw reply
* Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers
From: Eric Dumazet @ 2018-09-23 19:47 UTC (permalink / raw)
To: David Miller
Cc: netdev, michael.chan, Ariel Elior, Eric Dumazet, Tariq Toukan,
Saeed Mahameed, Jeff Kirsher, jakub.kicinski, songliubraving,
Jay Vosburgh, Veaceslav Falico, Andy Gospodarek
In-Reply-To: <20180923.122915.2174284095784692286.davem@davemloft.net>
On Sun, Sep 23, 2018 at 12:29 PM David Miller <davem@davemloft.net> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Fri, 21 Sep 2018 15:27:37 -0700
>
> > As diagnosed by Song Liu, ndo_poll_controller() can
> > be very dangerous on loaded hosts, since the cpu
> > calling ndo_poll_controller() might steal all NAPI
> > contexts (for all RX/TX queues of the NIC).
> >
> > This capture, showing one ksoftirqd eating all cycles
> > can last for unlimited amount of time, since one
> > cpu is generally not able to drain all the queues under load.
> >
> > It seems that all networking drivers that do use NAPI
> > for their TX completions, should not provide a ndo_poll_controller() :
> >
> > Most NAPI drivers have netpoll support already handled
> > in core networking stack, since netpoll_poll_dev()
> > uses poll_napi(dev) to iterate through registered
> > NAPI contexts for a device.
>
> I'm having trouble understanding the difference.
>
> If the drivers are processing all of the RX/TX queue draining by hand
> in their ndo_poll_controller() method, how is that different from the
> generic code walking all of the registererd NAPI instances one by one?
(resent in plain text mode this time)
Reading poll_napi() and poll_one_napi() I thought that we were using
NAPI_STATE_NPSVC
and cmpxchg(&napi->poll_owner, -1, cpu) to _temporary_ [1] own each
napi at a time.
But I do see we also have this part at the beginning of poll_one_napi() :
if (!test_bit(NAPI_STATE_SCHED, &napi->state))
return;
So we probably should remove it. (The normal napi->poll() calls would
not proceed since napi->poll_owner would not be -1)
[1]
While if a cpu succeeds into setting NAPI_STATE_SCHED, it means it has
to own it as long as the
napi->poll() does not call napi_complete_done(), and this can be
forever (the capture effect)
Basically calling napi_schedule() is the dangerous part.
I believe busy_polling and netpoll are the same intruders (as they can
run on arbitrary cpus).
But netpoll is far more problematic since it iterates through all RX/TX queues.
^ permalink raw reply
* Syrian Rescue
From: Syrian Rescue @ 2018-09-23 20:09 UTC (permalink / raw)
To: Recipients
Dear Sir/Madam,
My name is Rybak Ahmed from Syria. I am a miner and i wish to introduce my Gold/Diamonds funds which I and my partner sold. Because of the constant war going on in our country my partner and his family are all dead.Hence i take this upon myself to find trustworthy partners who will be able to partner with me to obtain the document needed, in order for us to receive our $6,200,000.00 which is currently in Bank of America escrow account.
Please note that my attorney/broker will discuss further with you as to what is needed to obtain the change of name, from my partner's to yours.
Your share will be 30% and send me 70% to my account that I will give you after you have receive the US$6,200,000.00 (six million and two hundred thousand dollars).You have nothing to lose. Just indicate your readiness by writing me back to my e-mail
Looking forward to your interest.
Sincerely,
Rybak Ahmed
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: Make MLXSW_SP1_FWREV_MINOR a hard requirement
From: Andrew Lunn @ 2018-09-23 20:58 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, davem, jiri, petrm, mlxsw
In-Reply-To: <20180923144855.26444-1-idosch@mellanox.com>
> Therefore tweak the check to accept any FW version that is:
>
> - on the same branch as the preferred version, and
> - the same as or newer than the preferred version.
Hi Ido
Do you print this information out? If the check fails, it would be
useful to know what the minimal version is.
Andrew
^ permalink raw reply
* Re: [PATCH 2/2] netfilter: nf_tables: add requirements for connsecmark support
From: kbuild test robot @ 2018-09-24 3:03 UTC (permalink / raw)
To: Christian Göttsche
Cc: kbuild-all, pablo, kadlec, fw, davem, netfilter-devel, coreteam,
netdev, linux-kernel, paul, sds, eparis, jmorris, serge, selinux,
linux-security-module
In-Reply-To: <20180923091611.19815-2-cgzones@googlemail.com>
[-- Attachment #1: Type: text/plain, Size: 3101 bytes --]
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on nf-next/master]
[also build test ERROR on v4.19-rc5 next-20180921]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Christian-G-ttsche/netfilter-nf_tables-add-SECMARK-support/20180923-213820
base: https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git master
config: x86_64-randconfig-s3-09241007 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
net//netfilter/nft_ct.c: In function 'nft_ct_set_eval':
>> net//netfilter/nft_ct.c:303:22: error: 'value' undeclared (first use in this function); did you mean 'false'?
if (ct->secmark != value) {
^~~~~
false
net//netfilter/nft_ct.c:303:22: note: each undeclared identifier is reported only once for each function it appears in
vim +303 net//netfilter/nft_ct.c
275
276 static void nft_ct_set_eval(const struct nft_expr *expr,
277 struct nft_regs *regs,
278 const struct nft_pktinfo *pkt)
279 {
280 const struct nft_ct *priv = nft_expr_priv(expr);
281 struct sk_buff *skb = pkt->skb;
282 #ifdef CONFIG_NF_CONNTRACK_MARK
283 u32 value = regs->data[priv->sreg];
284 #endif
285 enum ip_conntrack_info ctinfo;
286 struct nf_conn *ct;
287
288 ct = nf_ct_get(skb, &ctinfo);
289 if (ct == NULL || nf_ct_is_template(ct))
290 return;
291
292 switch (priv->key) {
293 #ifdef CONFIG_NF_CONNTRACK_MARK
294 case NFT_CT_MARK:
295 if (ct->mark != value) {
296 ct->mark = value;
297 nf_conntrack_event_cache(IPCT_MARK, ct);
298 }
299 break;
300 #endif
301 #ifdef CONFIG_NF_CONNTRACK_SECMARK
302 case NFT_CT_SECMARK:
> 303 if (ct->secmark != value) {
304 ct->secmark = value;
305 nf_conntrack_event_cache(IPCT_SECMARK, ct);
306 }
307 break;
308 #endif
309 #ifdef CONFIG_NF_CONNTRACK_LABELS
310 case NFT_CT_LABELS:
311 nf_connlabels_replace(ct,
312 ®s->data[priv->sreg],
313 ®s->data[priv->sreg],
314 NF_CT_LABELS_MAX_SIZE / sizeof(u32));
315 break;
316 #endif
317 #ifdef CONFIG_NF_CONNTRACK_EVENTS
318 case NFT_CT_EVENTMASK: {
319 struct nf_conntrack_ecache *e = nf_ct_ecache_find(ct);
320 u32 ctmask = regs->data[priv->sreg];
321
322 if (e) {
323 if (e->ctmask != ctmask)
324 e->ctmask = ctmask;
325 break;
326 }
327
328 if (ctmask && !nf_ct_is_confirmed(ct))
329 nf_ct_ecache_ext_add(ct, ctmask, 0, GFP_ATOMIC);
330 break;
331 }
332 #endif
333 default:
334 break;
335 }
336 }
337
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31606 bytes --]
^ permalink raw reply
* [net-next:master 13/221] drivers/net//ethernet/ni/nixge.c:145:2: note: in expansion of macro 'nixge_hw_dma_bd_set_addr'
From: kbuild test robot @ 2018-09-23 21:22 UTC (permalink / raw)
To: Moritz Fischer; +Cc: kbuild-all, netdev
[-- Attachment #1: Type: text/plain, Size: 18932 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git master
head: 12ba7e1045521ec9f251c93ae0a6735cc3f42337
commit: 7e8d5755be0e6c92d3b86a85e54c6a550b1910c5 [13/221] net: nixge: Add support for 64-bit platforms
config: i386-randconfig-x075-09240403 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout 7e8d5755be0e6c92d3b86a85e54c6a550b1910c5
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
drivers/net//ethernet/ni/nixge.c: In function 'nixge_hw_dma_bd_release':
drivers/net//ethernet/ni/nixge.c:254:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
skb = (struct sk_buff *)
^
In file included from include/linux/skbuff.h:17:0,
from include/linux/if_ether.h:23,
from include/linux/etherdevice.h:25,
from drivers/net//ethernet/ni/nixge.c:7:
drivers/net//ethernet/ni/nixge.c: In function 'nixge_hw_dma_bd_init':
drivers/net//ethernet/ni/nixge.c:130:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
(bd)->field##_lo = lower_32_bits(((u64)addr)); \
^
include/linux/kernel.h:234:33: note: in definition of macro 'lower_32_bits'
#define lower_32_bits(n) ((u32)(n))
^
>> drivers/net//ethernet/ni/nixge.c:145:2: note: in expansion of macro 'nixge_hw_dma_bd_set_addr'
nixge_hw_dma_bd_set_addr((bd), sw_id_offset, (addr))
^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net//ethernet/ni/nixge.c:326:3: note: in expansion of macro 'nixge_hw_dma_bd_set_offset'
nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], skb);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net//ethernet/ni/nixge.c:131:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
(bd)->field##_hi = upper_32_bits(((u64)addr)); \
^
include/linux/kernel.h:228:35: note: in definition of macro 'upper_32_bits'
#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
^
>> drivers/net//ethernet/ni/nixge.c:145:2: note: in expansion of macro 'nixge_hw_dma_bd_set_addr'
nixge_hw_dma_bd_set_addr((bd), sw_id_offset, (addr))
^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net//ethernet/ni/nixge.c:326:3: note: in expansion of macro 'nixge_hw_dma_bd_set_offset'
nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], skb);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net//ethernet/ni/nixge.c: In function 'nixge_recv':
drivers/net//ethernet/ni/nixge.c:604:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
skb = (struct sk_buff *)nixge_hw_dma_bd_get_addr(cur_p,
^
In file included from include/linux/skbuff.h:17:0,
from include/linux/if_ether.h:23,
from include/linux/etherdevice.h:25,
from drivers/net//ethernet/ni/nixge.c:7:
drivers/net//ethernet/ni/nixge.c:130:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
(bd)->field##_lo = lower_32_bits(((u64)addr)); \
^
include/linux/kernel.h:234:33: note: in definition of macro 'lower_32_bits'
#define lower_32_bits(n) ((u32)(n))
^
>> drivers/net//ethernet/ni/nixge.c:145:2: note: in expansion of macro 'nixge_hw_dma_bd_set_addr'
nixge_hw_dma_bd_set_addr((bd), sw_id_offset, (addr))
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net//ethernet/ni/nixge.c:646:3: note: in expansion of macro 'nixge_hw_dma_bd_set_offset'
nixge_hw_dma_bd_set_offset(cur_p, new_skb);
^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net//ethernet/ni/nixge.c:131:37: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
(bd)->field##_hi = upper_32_bits(((u64)addr)); \
^
include/linux/kernel.h:228:35: note: in definition of macro 'upper_32_bits'
#define upper_32_bits(n) ((u32)(((n) >> 16) >> 16))
^
>> drivers/net//ethernet/ni/nixge.c:145:2: note: in expansion of macro 'nixge_hw_dma_bd_set_addr'
nixge_hw_dma_bd_set_addr((bd), sw_id_offset, (addr))
^~~~~~~~~~~~~~~~~~~~~~~~
drivers/net//ethernet/ni/nixge.c:646:3: note: in expansion of macro 'nixge_hw_dma_bd_set_offset'
nixge_hw_dma_bd_set_offset(cur_p, new_skb);
^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/nixge_hw_dma_bd_set_addr +145 drivers/net//ethernet/ni/nixge.c
> 7 #include <linux/etherdevice.h>
8 #include <linux/module.h>
9 #include <linux/netdevice.h>
10 #include <linux/of_address.h>
11 #include <linux/of_mdio.h>
12 #include <linux/of_net.h>
13 #include <linux/of_platform.h>
14 #include <linux/of_irq.h>
15 #include <linux/skbuff.h>
16 #include <linux/phy.h>
17 #include <linux/mii.h>
18 #include <linux/nvmem-consumer.h>
19 #include <linux/ethtool.h>
20 #include <linux/iopoll.h>
21
22 #define TX_BD_NUM 64
23 #define RX_BD_NUM 128
24
25 /* Axi DMA Register definitions */
26 #define XAXIDMA_TX_CR_OFFSET 0x00 /* Channel control */
27 #define XAXIDMA_TX_SR_OFFSET 0x04 /* Status */
28 #define XAXIDMA_TX_CDESC_OFFSET 0x08 /* Current descriptor pointer */
29 #define XAXIDMA_TX_TDESC_OFFSET 0x10 /* Tail descriptor pointer */
30
31 #define XAXIDMA_RX_CR_OFFSET 0x30 /* Channel control */
32 #define XAXIDMA_RX_SR_OFFSET 0x34 /* Status */
33 #define XAXIDMA_RX_CDESC_OFFSET 0x38 /* Current descriptor pointer */
34 #define XAXIDMA_RX_TDESC_OFFSET 0x40 /* Tail descriptor pointer */
35
36 #define XAXIDMA_CR_RUNSTOP_MASK 0x1 /* Start/stop DMA channel */
37 #define XAXIDMA_CR_RESET_MASK 0x4 /* Reset DMA engine */
38
39 #define XAXIDMA_BD_CTRL_LENGTH_MASK 0x007FFFFF /* Requested len */
40 #define XAXIDMA_BD_CTRL_TXSOF_MASK 0x08000000 /* First tx packet */
41 #define XAXIDMA_BD_CTRL_TXEOF_MASK 0x04000000 /* Last tx packet */
42 #define XAXIDMA_BD_CTRL_ALL_MASK 0x0C000000 /* All control bits */
43
44 #define XAXIDMA_DELAY_MASK 0xFF000000 /* Delay timeout counter */
45 #define XAXIDMA_COALESCE_MASK 0x00FF0000 /* Coalesce counter */
46
47 #define XAXIDMA_DELAY_SHIFT 24
48 #define XAXIDMA_COALESCE_SHIFT 16
49
50 #define XAXIDMA_IRQ_IOC_MASK 0x00001000 /* Completion intr */
51 #define XAXIDMA_IRQ_DELAY_MASK 0x00002000 /* Delay interrupt */
52 #define XAXIDMA_IRQ_ERROR_MASK 0x00004000 /* Error interrupt */
53 #define XAXIDMA_IRQ_ALL_MASK 0x00007000 /* All interrupts */
54
55 /* Default TX/RX Threshold and waitbound values for SGDMA mode */
56 #define XAXIDMA_DFT_TX_THRESHOLD 24
57 #define XAXIDMA_DFT_TX_WAITBOUND 254
58 #define XAXIDMA_DFT_RX_THRESHOLD 24
59 #define XAXIDMA_DFT_RX_WAITBOUND 254
60
61 #define XAXIDMA_BD_STS_ACTUAL_LEN_MASK 0x007FFFFF /* Actual len */
62 #define XAXIDMA_BD_STS_COMPLETE_MASK 0x80000000 /* Completed */
63 #define XAXIDMA_BD_STS_DEC_ERR_MASK 0x40000000 /* Decode error */
64 #define XAXIDMA_BD_STS_SLV_ERR_MASK 0x20000000 /* Slave error */
65 #define XAXIDMA_BD_STS_INT_ERR_MASK 0x10000000 /* Internal err */
66 #define XAXIDMA_BD_STS_ALL_ERR_MASK 0x70000000 /* All errors */
67 #define XAXIDMA_BD_STS_RXSOF_MASK 0x08000000 /* First rx pkt */
68 #define XAXIDMA_BD_STS_RXEOF_MASK 0x04000000 /* Last rx pkt */
69 #define XAXIDMA_BD_STS_ALL_MASK 0xFC000000 /* All status bits */
70
71 #define NIXGE_REG_CTRL_OFFSET 0x4000
72 #define NIXGE_REG_INFO 0x00
73 #define NIXGE_REG_MAC_CTL 0x04
74 #define NIXGE_REG_PHY_CTL 0x08
75 #define NIXGE_REG_LED_CTL 0x0c
76 #define NIXGE_REG_MDIO_DATA 0x10
77 #define NIXGE_REG_MDIO_ADDR 0x14
78 #define NIXGE_REG_MDIO_OP 0x18
79 #define NIXGE_REG_MDIO_CTRL 0x1c
80
81 #define NIXGE_ID_LED_CTL_EN BIT(0)
82 #define NIXGE_ID_LED_CTL_VAL BIT(1)
83
84 #define NIXGE_MDIO_CLAUSE45 BIT(12)
85 #define NIXGE_MDIO_CLAUSE22 0
86 #define NIXGE_MDIO_OP(n) (((n) & 0x3) << 10)
87 #define NIXGE_MDIO_OP_ADDRESS 0
88 #define NIXGE_MDIO_C45_WRITE BIT(0)
89 #define NIXGE_MDIO_C45_READ (BIT(1) | BIT(0))
90 #define NIXGE_MDIO_C22_WRITE BIT(0)
91 #define NIXGE_MDIO_C22_READ BIT(1)
92 #define NIXGE_MDIO_ADDR(n) (((n) & 0x1f) << 5)
93 #define NIXGE_MDIO_MMD(n) (((n) & 0x1f) << 0)
94
95 #define NIXGE_REG_MAC_LSB 0x1000
96 #define NIXGE_REG_MAC_MSB 0x1004
97
98 /* Packet size info */
99 #define NIXGE_HDR_SIZE 14 /* Size of Ethernet header */
100 #define NIXGE_TRL_SIZE 4 /* Size of Ethernet trailer (FCS) */
101 #define NIXGE_MTU 1500 /* Max MTU of an Ethernet frame */
102 #define NIXGE_JUMBO_MTU 9000 /* Max MTU of a jumbo Eth. frame */
103
104 #define NIXGE_MAX_FRAME_SIZE (NIXGE_MTU + NIXGE_HDR_SIZE + NIXGE_TRL_SIZE)
105 #define NIXGE_MAX_JUMBO_FRAME_SIZE \
106 (NIXGE_JUMBO_MTU + NIXGE_HDR_SIZE + NIXGE_TRL_SIZE)
107
108 struct nixge_hw_dma_bd {
109 u32 next_lo;
110 u32 next_hi;
111 u32 phys_lo;
112 u32 phys_hi;
113 u32 reserved3;
114 u32 reserved4;
115 u32 cntrl;
116 u32 status;
117 u32 app0;
118 u32 app1;
119 u32 app2;
120 u32 app3;
121 u32 app4;
122 u32 sw_id_offset_lo;
123 u32 sw_id_offset_hi;
124 u32 reserved6;
125 };
126
127 #ifdef CONFIG_PHYS_ADDR_T_64BIT
128 #define nixge_hw_dma_bd_set_addr(bd, field, addr) \
129 do { \
> 130 (bd)->field##_lo = lower_32_bits(((u64)addr)); \
131 (bd)->field##_hi = upper_32_bits(((u64)addr)); \
132 } while (0)
133 #else
134 #define nixge_hw_dma_bd_set_addr(bd, field, addr) \
135 ((bd)->field##_lo = lower_32_bits((addr)))
136 #endif
137
138 #define nixge_hw_dma_bd_set_phys(bd, addr) \
139 nixge_hw_dma_bd_set_addr((bd), phys, (addr))
140
141 #define nixge_hw_dma_bd_set_next(bd, addr) \
142 nixge_hw_dma_bd_set_addr((bd), next, (addr))
143
144 #define nixge_hw_dma_bd_set_offset(bd, addr) \
> 145 nixge_hw_dma_bd_set_addr((bd), sw_id_offset, (addr))
146
147 #ifdef CONFIG_PHYS_ADDR_T_64BIT
148 #define nixge_hw_dma_bd_get_addr(bd, field) \
149 (dma_addr_t)((((u64)(bd)->field##_hi) << 32) | ((bd)->field##_lo))
150 #else
151 #define nixge_hw_dma_bd_get_addr(bd, field) \
152 (dma_addr_t)((bd)->field##_lo)
153 #endif
154
155 struct nixge_tx_skb {
156 struct sk_buff *skb;
157 dma_addr_t mapping;
158 size_t size;
159 bool mapped_as_page;
160 };
161
162 struct nixge_priv {
163 struct net_device *ndev;
164 struct napi_struct napi;
165 struct device *dev;
166
167 /* Connection to PHY device */
168 struct device_node *phy_node;
169 phy_interface_t phy_mode;
170
171 int link;
172 unsigned int speed;
173 unsigned int duplex;
174
175 /* MDIO bus data */
176 struct mii_bus *mii_bus; /* MII bus reference */
177
178 /* IO registers, dma functions and IRQs */
179 void __iomem *ctrl_regs;
180 void __iomem *dma_regs;
181
182 struct tasklet_struct dma_err_tasklet;
183
184 int tx_irq;
185 int rx_irq;
186
187 /* Buffer descriptors */
188 struct nixge_hw_dma_bd *tx_bd_v;
189 struct nixge_tx_skb *tx_skb;
190 dma_addr_t tx_bd_p;
191
192 struct nixge_hw_dma_bd *rx_bd_v;
193 dma_addr_t rx_bd_p;
194 u32 tx_bd_ci;
195 u32 tx_bd_tail;
196 u32 rx_bd_ci;
197
198 u32 coalesce_count_rx;
199 u32 coalesce_count_tx;
200 };
201
202 static void nixge_dma_write_reg(struct nixge_priv *priv, off_t offset, u32 val)
203 {
204 writel(val, priv->dma_regs + offset);
205 }
206
207 static void nixge_dma_write_desc_reg(struct nixge_priv *priv, off_t offset,
208 dma_addr_t addr)
209 {
210 writel(lower_32_bits(addr), priv->dma_regs + offset);
211 #ifdef CONFIG_PHYS_ADDR_T_64BIT
212 writel(upper_32_bits(addr), priv->dma_regs + offset + 4);
213 #endif
214 }
215
216 static u32 nixge_dma_read_reg(const struct nixge_priv *priv, off_t offset)
217 {
218 return readl(priv->dma_regs + offset);
219 }
220
221 static void nixge_ctrl_write_reg(struct nixge_priv *priv, off_t offset, u32 val)
222 {
223 writel(val, priv->ctrl_regs + offset);
224 }
225
226 static u32 nixge_ctrl_read_reg(struct nixge_priv *priv, off_t offset)
227 {
228 return readl(priv->ctrl_regs + offset);
229 }
230
231 #define nixge_ctrl_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us) \
232 readl_poll_timeout((priv)->ctrl_regs + (addr), (val), (cond), \
233 (sleep_us), (timeout_us))
234
235 #define nixge_dma_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us) \
236 readl_poll_timeout((priv)->dma_regs + (addr), (val), (cond), \
237 (sleep_us), (timeout_us))
238
239 static void nixge_hw_dma_bd_release(struct net_device *ndev)
240 {
241 struct nixge_priv *priv = netdev_priv(ndev);
242 dma_addr_t phys_addr;
243 struct sk_buff *skb;
244 int i;
245
246 for (i = 0; i < RX_BD_NUM; i++) {
247 phys_addr = nixge_hw_dma_bd_get_addr(&priv->rx_bd_v[i],
248 phys);
249
250 dma_unmap_single(ndev->dev.parent, phys_addr,
251 NIXGE_MAX_JUMBO_FRAME_SIZE,
252 DMA_FROM_DEVICE);
253
254 skb = (struct sk_buff *)
255 nixge_hw_dma_bd_get_addr(&priv->rx_bd_v[i],
256 sw_id_offset);
257 dev_kfree_skb(skb);
258 }
259
260 if (priv->rx_bd_v)
261 dma_free_coherent(ndev->dev.parent,
262 sizeof(*priv->rx_bd_v) * RX_BD_NUM,
263 priv->rx_bd_v,
264 priv->rx_bd_p);
265
266 if (priv->tx_skb)
267 devm_kfree(ndev->dev.parent, priv->tx_skb);
268
269 if (priv->tx_bd_v)
270 dma_free_coherent(ndev->dev.parent,
271 sizeof(*priv->tx_bd_v) * TX_BD_NUM,
272 priv->tx_bd_v,
273 priv->tx_bd_p);
274 }
275
276 static int nixge_hw_dma_bd_init(struct net_device *ndev)
277 {
278 struct nixge_priv *priv = netdev_priv(ndev);
279 struct sk_buff *skb;
280 dma_addr_t phys;
281 u32 cr;
282 int i;
283
284 /* Reset the indexes which are used for accessing the BDs */
285 priv->tx_bd_ci = 0;
286 priv->tx_bd_tail = 0;
287 priv->rx_bd_ci = 0;
288
289 /* Allocate the Tx and Rx buffer descriptors. */
290 priv->tx_bd_v = dma_zalloc_coherent(ndev->dev.parent,
291 sizeof(*priv->tx_bd_v) * TX_BD_NUM,
292 &priv->tx_bd_p, GFP_KERNEL);
293 if (!priv->tx_bd_v)
294 goto out;
295
296 priv->tx_skb = devm_kcalloc(ndev->dev.parent,
297 TX_BD_NUM, sizeof(*priv->tx_skb),
298 GFP_KERNEL);
299 if (!priv->tx_skb)
300 goto out;
301
302 priv->rx_bd_v = dma_zalloc_coherent(ndev->dev.parent,
303 sizeof(*priv->rx_bd_v) * RX_BD_NUM,
304 &priv->rx_bd_p, GFP_KERNEL);
305 if (!priv->rx_bd_v)
306 goto out;
307
308 for (i = 0; i < TX_BD_NUM; i++) {
309 nixge_hw_dma_bd_set_next(&priv->tx_bd_v[i],
310 priv->tx_bd_p +
311 sizeof(*priv->tx_bd_v) *
312 ((i + 1) % TX_BD_NUM));
313 }
314
315 for (i = 0; i < RX_BD_NUM; i++) {
316 nixge_hw_dma_bd_set_next(&priv->rx_bd_v[i],
317 priv->rx_bd_p
318 + sizeof(*priv->rx_bd_v) *
319 ((i + 1) % RX_BD_NUM));
320
321 skb = netdev_alloc_skb_ip_align(ndev,
322 NIXGE_MAX_JUMBO_FRAME_SIZE);
323 if (!skb)
324 goto out;
325
> 326 nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], skb);
327 phys = dma_map_single(ndev->dev.parent, skb->data,
328 NIXGE_MAX_JUMBO_FRAME_SIZE,
329 DMA_FROM_DEVICE);
330
331 nixge_hw_dma_bd_set_phys(&priv->rx_bd_v[i], phys);
332
333 priv->rx_bd_v[i].cntrl = NIXGE_MAX_JUMBO_FRAME_SIZE;
334 }
335
336 /* Start updating the Rx channel control register */
337 cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
338 /* Update the interrupt coalesce count */
339 cr = ((cr & ~XAXIDMA_COALESCE_MASK) |
340 ((priv->coalesce_count_rx) << XAXIDMA_COALESCE_SHIFT));
341 /* Update the delay timer count */
342 cr = ((cr & ~XAXIDMA_DELAY_MASK) |
343 (XAXIDMA_DFT_RX_WAITBOUND << XAXIDMA_DELAY_SHIFT));
344 /* Enable coalesce, delay timer and error interrupts */
345 cr |= XAXIDMA_IRQ_ALL_MASK;
346 /* Write to the Rx channel control register */
347 nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET, cr);
348
349 /* Start updating the Tx channel control register */
350 cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET);
351 /* Update the interrupt coalesce count */
352 cr = (((cr & ~XAXIDMA_COALESCE_MASK)) |
353 ((priv->coalesce_count_tx) << XAXIDMA_COALESCE_SHIFT));
354 /* Update the delay timer count */
355 cr = (((cr & ~XAXIDMA_DELAY_MASK)) |
356 (XAXIDMA_DFT_TX_WAITBOUND << XAXIDMA_DELAY_SHIFT));
357 /* Enable coalesce, delay timer and error interrupts */
358 cr |= XAXIDMA_IRQ_ALL_MASK;
359 /* Write to the Tx channel control register */
360 nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET, cr);
361
362 /* Populate the tail pointer and bring the Rx Axi DMA engine out of
363 * halted state. This will make the Rx side ready for reception.
364 */
365 nixge_dma_write_desc_reg(priv, XAXIDMA_RX_CDESC_OFFSET, priv->rx_bd_p);
366 cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
367 nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
368 cr | XAXIDMA_CR_RUNSTOP_MASK);
369 nixge_dma_write_desc_reg(priv, XAXIDMA_RX_TDESC_OFFSET, priv->rx_bd_p +
370 (sizeof(*priv->rx_bd_v) * (RX_BD_NUM - 1)));
371
372 /* Write to the RS (Run-stop) bit in the Tx channel control register.
373 * Tx channel is now ready to run. But only after we write to the
374 * tail pointer register that the Tx channel will start transmitting.
375 */
376 nixge_dma_write_desc_reg(priv, XAXIDMA_TX_CDESC_OFFSET, priv->tx_bd_p);
377 cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET);
378 nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
379 cr | XAXIDMA_CR_RUNSTOP_MASK);
380
381 return 0;
382 out:
383 nixge_hw_dma_bd_release(ndev);
384 return -ENOMEM;
385 }
386
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32795 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: Make MLXSW_SP1_FWREV_MINOR a hard requirement
From: Ido Schimmel @ 2018-09-23 21:51 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Ido Schimmel, netdev, davem, jiri, petrm, mlxsw
In-Reply-To: <20180923205809.GA30338@lunn.ch>
On Sun, Sep 23, 2018 at 10:58:09PM +0200, Andrew Lunn wrote:
> > Therefore tweak the check to accept any FW version that is:
> >
> > - on the same branch as the preferred version, and
> > - the same as or newer than the preferred version.
>
> Hi Ido
>
> Do you print this information out? If the check fails, it would be
> useful to know what the minimal version is.
Hi Andrew,
Yes, we do print it. It is the version the driver will try to load
during initialization in case current version is incompatible:
dev_info(mlxsw_sp->bus_info->dev, "Flashing firmware using file %s\n",
fw_filename);
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: Make MLXSW_SP1_FWREV_MINOR a hard requirement
From: Andrew Lunn @ 2018-09-23 22:04 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Ido Schimmel, netdev, davem, jiri, petrm, mlxsw
In-Reply-To: <20180923215100.GA24149@splinter>
On Mon, Sep 24, 2018 at 12:51:00AM +0300, Ido Schimmel wrote:
> On Sun, Sep 23, 2018 at 10:58:09PM +0200, Andrew Lunn wrote:
> > > Therefore tweak the check to accept any FW version that is:
> > >
> > > - on the same branch as the preferred version, and
> > > - the same as or newer than the preferred version.
> >
> > Hi Ido
> >
> > Do you print this information out? If the check fails, it would be
> > useful to know what the minimal version is.
>
> Hi Andrew,
>
> Yes, we do print it. It is the version the driver will try to load
> during initialization in case current version is incompatible:
>
> dev_info(mlxsw_sp->bus_info->dev, "Flashing firmware using file %s\n",
> fw_filename);
Ah. O.K. Thanks.
But doesn't that mean you reflash the device with the minimum version,
when in fact there could be a much newer version in /lib/firmware?
Andrew
^ permalink raw reply
* Re: KMSAN: uninit-value in memcmp (2)
From: Vladis Dronov @ 2018-09-23 22:09 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: syzbot, syzkaller-bugs, David Miller, Eric Dumazet, LKML, netdev,
sunlianwen
In-Reply-To: <CACT4Y+Z1G_QMG_PQu_ZwHsyhwyh_DBOG4mX9n=va08w3Z7j02Q@mail.gmail.com>
Hello, Dmirty,
Thank you for the reply. Can we please, discuss this further?
> You can see on dashboard that the last crash
> for the second version (2) happened just few days ago. So this is a
> different bug.
Well... yes and no. When I was looking at this bug (bug?id=088efeac32fd) I was looking
at the report at "2018/05/09 18:55" (https://syzkaller.appspot.com/text?tag=CrashReport&x=141b707b800000),
since it was the only report with a reproducer. This was my error.
The error and the call trace in this report are:
>>>
BUG: KMSAN: uninit-value in memcmp+0x119/0x180 lib/string.c:861
CPU: 0 PID: 38 Comm: kworker/0:1 Not tainted 4.17.0-rc3+ #88
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: ipv6_addrconf addrconf_dad_work
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x185/0x1d0 lib/dump_stack.c:113
kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
__msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
memcmp+0x119/0x180 lib/string.c:861
__hw_addr_add_ex net/core/dev_addr_lists.c:61 [inline]
__dev_mc_add+0x1fc/0x900 net/core/dev_addr_lists.c:670
dev_mc_add+0x6d/0x80 net/core/dev_addr_lists.c:687
igmp6_group_added+0x2db/0xa00 net/ipv6/mcast.c:662
ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914
addrconf_join_solict net/ipv6/addrconf.c:2103 [inline]
addrconf_dad_begin net/ipv6/addrconf.c:3853 [inline]
addrconf_dad_work+0x462/0x2a20 net/ipv6/addrconf.c:3979
process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
kthread+0x539/0x720 kernel/kthread.c:239
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412
Local variable description: ----buf@igmp6_group_added
Variable was created at:
igmp6_group_added+0x4a/0xa00 net/ipv6/mcast.c:650
ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914
<<<
It is the same like in bug?id=3887c0d99aecb27d085180c5222d245d08a30806
which, after some more test, made me believe these bugs are duplicate
and are fixed by the same commit.
But let's look at another report at "2018/09/12 21:00"
(https://syzkaller.appspot.com/text?tag=CrashReport&x=14f99b71400000)
at the bug (bug?id=088efeac32fd), the one you've mentioned as
"the last crash for the second version (2) happened just few days ago".
Its error and the call trace are completely different:
>>>
BUG: KMSAN: uninit-value in memcmp+0x11d/0x180 lib/string.c:863
CPU: 0 PID: 6107 Comm: syz-executor4 Not tainted 4.19.0-rc3+ #45
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x14b/0x190 lib/dump_stack.c:113
kmsan_report+0x183/0x2b0 mm/kmsan/kmsan.c:956
__msan_warning+0x70/0xc0 mm/kmsan/kmsan_instr.c:645
memcmp+0x11d/0x180 lib/string.c:863
dev_uc_add_excl+0x165/0x7b0 net/core/dev_addr_lists.c:464
ndo_dflt_fdb_add net/core/rtnetlink.c:3463 [inline]
rtnl_fdb_add+0x1081/0x1270 net/core/rtnetlink.c:3558
rtnetlink_rcv_msg+0xa0b/0x1530 net/core/rtnetlink.c:4715
netlink_rcv_skb+0x36e/0x5f0 net/netlink/af_netlink.c:2454
rtnetlink_rcv+0x50/0x60 net/core/rtnetlink.c:4733
netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
netlink_unicast+0x1638/0x1720 net/netlink/af_netlink.c:1343
netlink_sendmsg+0x1205/0x1290 net/netlink/af_netlink.c:1908
sock_sendmsg_nosec net/socket.c:621 [inline]
sock_sendmsg net/socket.c:631 [inline]
...
Uninit was created at:
...
slab_post_alloc_hook mm/slab.h:446 [inline]
slab_alloc_node mm/slub.c:2718 [inline]
__kmalloc_node_track_caller+0x9e7/0x1160 mm/slub.c:4351
__kmalloc_reserve net/core/skbuff.c:138 [inline]
__alloc_skb+0x2f5/0x9e0 net/core/skbuff.c:206
alloc_skb include/linux/skbuff.h:996 [inline]
netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
netlink_sendmsg+0xb49/0x1290 net/netlink/af_netlink.c:1883
sock_sendmsg_nosec net/socket.c:621 [inline]
sock_sendmsg net/socket.c:631 [inline]
___sys_sendmsg+0xe70/0x1290 net/socket.c:2114
<<<
This is a different bug. How come these 2 different reports for 2 different
bugs have ended in the same syzkaller report (bug?id=088efeac32fd) ?
One bug is fixed by the "net: fix uninit-value in __hw_addr_add_ex()" commit,
the second one is not, but they are still in the same syzkaller report.
This was the reason of my confusion. I'm not sure how to fix this. If it is possible,
probably we need to cancel/revoke "#syz fix: net: fix uninit-value in __hw_addr_add_ex()"
for this syzkaller report (bug?id=088efeac32fd). And then "split" it into 2 or
more different reports, but I'm not sure if this is possible.
Probably, syzkaller needs to look deeper into the KMSAN reports to differentiate
KMSAN errors happening because of different reasons.
Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer
----- Original Message -----
> From: "Dmitry Vyukov" <dvyukov@google.com>
> To: "Vladis Dronov" <vdronov@redhat.com>
> Cc: "syzbot" <syzbot+d3402c47f680ff24b29c@syzkaller.appspotmail.com>, "syzkaller-bugs"
> <syzkaller-bugs@googlegroups.com>, "David Miller" <davem@davemloft.net>, "Eric Dumazet" <edumazet@google.com>,
> "LKML" <linux-kernel@vger.kernel.org>, "netdev" <netdev@vger.kernel.org>, "sunlianwen" <sunlw.fnst@cn.fujitsu.com>
> Sent: Sunday, September 23, 2018 6:22:36 PM
> Subject: Re: KMSAN: uninit-value in memcmp (2)
>
> On Sun, Sep 23, 2018 at 6:02 PM, Vladis Dronov <vdronov@redhat.com> wrote:
> > #syz fix: net: fix uninit-value in __hw_addr_add_ex()
>
> Hi Vladis,
>
> This can be fixed with "net: fix uninit-value in __hw_addr_add_ex()".
> That commit landed in April, syzbot waited till the commit reached all
> tested trees, and then closed the bug.
> But the similar bug continued to happen, so syzbot created second
> version of this bug (2). You can see on dashboard that the last crash
> for the second version (2) happened just few days ago. So this is a
> different bug.
>
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: Make MLXSW_SP1_FWREV_MINOR a hard requirement
From: Ido Schimmel @ 2018-09-23 22:28 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Ido Schimmel, netdev, davem, jiri, petrm, mlxsw
In-Reply-To: <20180923220417.GA31923@lunn.ch>
On Mon, Sep 24, 2018 at 12:04:17AM +0200, Andrew Lunn wrote:
> On Mon, Sep 24, 2018 at 12:51:00AM +0300, Ido Schimmel wrote:
> > On Sun, Sep 23, 2018 at 10:58:09PM +0200, Andrew Lunn wrote:
> > > > Therefore tweak the check to accept any FW version that is:
> > > >
> > > > - on the same branch as the preferred version, and
> > > > - the same as or newer than the preferred version.
> > >
> > > Hi Ido
> > >
> > > Do you print this information out? If the check fails, it would be
> > > useful to know what the minimal version is.
> >
> > Hi Andrew,
> >
> > Yes, we do print it. It is the version the driver will try to load
> > during initialization in case current version is incompatible:
> >
> > dev_info(mlxsw_sp->bus_info->dev, "Flashing firmware using file %s\n",
> > fw_filename);
>
> Ah. O.K. Thanks.
>
> But doesn't that mean you reflash the device with the minimum version,
> when in fact there could be a much newer version in /lib/firmware?
No, because we always enforce the latest version we post to
linux-firmware. We try to keep firmware updates at a minimum, so if we
decided to post a new version it's either because the driver now
requires a feature from this version (which makes older versions
incompatible) or because a critical bug was fixed in that version.
^ permalink raw reply
* Re: KMSAN: uninit-value in memcmp (2)
From: Alexander Potapenko @ 2018-09-24 6:53 UTC (permalink / raw)
To: Vladis Dronov
Cc: Dmitriy Vyukov, syzbot+d3402c47f680ff24b29c, syzkaller-bugs,
David Miller, Eric Dumazet, LKML, Networking, sunlw.fnst
In-Reply-To: <1040580049.15456466.1537740558279.JavaMail.zimbra@redhat.com>
On Mon, Sep 24, 2018 at 12:09 AM Vladis Dronov <vdronov@redhat.com> wrote:
>
> Hello, Dmirty,
>
> Thank you for the reply. Can we please, discuss this further?
Hi Vladis,
> > You can see on dashboard that the last crash
> > for the second version (2) happened just few days ago. So this is a
> > different bug.
FWIW I've just double-checked that the reproducer provided by
syzkaller in the original message still triggers the report from the
original message in the latest KMSAN tree (which already contains the
__hw_addr_add_ex() fix from April).
> Well... yes and no. When I was looking at this bug (bug?id=088efeac32fd) I was looking
> at the report at "2018/05/09 18:55" (https://syzkaller.appspot.com/text?tag=CrashReport&x=141b707b800000),
> since it was the only report with a reproducer. This was my error.
>
> The error and the call trace in this report are:
>
> >>>
> BUG: KMSAN: uninit-value in memcmp+0x119/0x180 lib/string.c:861
> CPU: 0 PID: 38 Comm: kworker/0:1 Not tainted 4.17.0-rc3+ #88
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: ipv6_addrconf addrconf_dad_work
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x185/0x1d0 lib/dump_stack.c:113
> kmsan_report+0x142/0x240 mm/kmsan/kmsan.c:1067
> __msan_warning_32+0x6c/0xb0 mm/kmsan/kmsan_instr.c:683
> memcmp+0x119/0x180 lib/string.c:861
> __hw_addr_add_ex net/core/dev_addr_lists.c:61 [inline]
> __dev_mc_add+0x1fc/0x900 net/core/dev_addr_lists.c:670
> dev_mc_add+0x6d/0x80 net/core/dev_addr_lists.c:687
> igmp6_group_added+0x2db/0xa00 net/ipv6/mcast.c:662
> ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914
> addrconf_join_solict net/ipv6/addrconf.c:2103 [inline]
> addrconf_dad_begin net/ipv6/addrconf.c:3853 [inline]
> addrconf_dad_work+0x462/0x2a20 net/ipv6/addrconf.c:3979
> process_one_work+0x12c6/0x1f60 kernel/workqueue.c:2145
> worker_thread+0x113c/0x24f0 kernel/workqueue.c:2279
> kthread+0x539/0x720 kernel/kthread.c:239
> ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:412
>
> Local variable description: ----buf@igmp6_group_added
> Variable was created at:
> igmp6_group_added+0x4a/0xa00 net/ipv6/mcast.c:650
> ipv6_dev_mc_inc+0xe9e/0x1130 net/ipv6/mcast.c:914
> <<<
>
> It is the same like in bug?id=3887c0d99aecb27d085180c5222d245d08a30806
> which, after some more test, made me believe these bugs are duplicate
> and are fixed by the same commit.
>
> But let's look at another report at "2018/09/12 21:00"
> (https://syzkaller.appspot.com/text?tag=CrashReport&x=14f99b71400000)
> at the bug (bug?id=088efeac32fd), the one you've mentioned as
> "the last crash for the second version (2) happened just few days ago".
>
> Its error and the call trace are completely different:
>
> >>>
> BUG: KMSAN: uninit-value in memcmp+0x11d/0x180 lib/string.c:863
> CPU: 0 PID: 6107 Comm: syz-executor4 Not tainted 4.19.0-rc3+ #45
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x14b/0x190 lib/dump_stack.c:113
> kmsan_report+0x183/0x2b0 mm/kmsan/kmsan.c:956
> __msan_warning+0x70/0xc0 mm/kmsan/kmsan_instr.c:645
> memcmp+0x11d/0x180 lib/string.c:863
> dev_uc_add_excl+0x165/0x7b0 net/core/dev_addr_lists.c:464
> ndo_dflt_fdb_add net/core/rtnetlink.c:3463 [inline]
> rtnl_fdb_add+0x1081/0x1270 net/core/rtnetlink.c:3558
> rtnetlink_rcv_msg+0xa0b/0x1530 net/core/rtnetlink.c:4715
> netlink_rcv_skb+0x36e/0x5f0 net/netlink/af_netlink.c:2454
> rtnetlink_rcv+0x50/0x60 net/core/rtnetlink.c:4733
> netlink_unicast_kernel net/netlink/af_netlink.c:1317 [inline]
> netlink_unicast+0x1638/0x1720 net/netlink/af_netlink.c:1343
> netlink_sendmsg+0x1205/0x1290 net/netlink/af_netlink.c:1908
> sock_sendmsg_nosec net/socket.c:621 [inline]
> sock_sendmsg net/socket.c:631 [inline]
> ...
> Uninit was created at:
> ...
> slab_post_alloc_hook mm/slab.h:446 [inline]
> slab_alloc_node mm/slub.c:2718 [inline]
> __kmalloc_node_track_caller+0x9e7/0x1160 mm/slub.c:4351
> __kmalloc_reserve net/core/skbuff.c:138 [inline]
> __alloc_skb+0x2f5/0x9e0 net/core/skbuff.c:206
> alloc_skb include/linux/skbuff.h:996 [inline]
> netlink_alloc_large_skb net/netlink/af_netlink.c:1189 [inline]
> netlink_sendmsg+0xb49/0x1290 net/netlink/af_netlink.c:1883
> sock_sendmsg_nosec net/socket.c:621 [inline]
> sock_sendmsg net/socket.c:631 [inline]
> ___sys_sendmsg+0xe70/0x1290 net/socket.c:2114
> <<<
>
> This is a different bug. How come these 2 different reports for 2 different
> bugs have ended in the same syzkaller report (bug?id=088efeac32fd) ?
I suspect this is because syzbot used the top stack frame as the
report signature.
There's a mechanism to ignore frames like memcmp() in the reports, not
sure why didn't it work in this case (maybe it just wasn't in place at
the time the report happened).
> One bug is fixed by the "net: fix uninit-value in __hw_addr_add_ex()" commit,
> the second one is not, but they are still in the same syzkaller report.
>
> This was the reason of my confusion. I'm not sure how to fix this. If it is possible,
> probably we need to cancel/revoke "#syz fix: net: fix uninit-value in __hw_addr_add_ex()"
> for this syzkaller report (bug?id=088efeac32fd). And then "split" it into 2 or
> more different reports, but I'm not sure if this is possible.
>
> Probably, syzkaller needs to look deeper into the KMSAN reports to differentiate
> KMSAN errors happening because of different reasons.
>
> Best regards,
> Vladis Dronov | Red Hat, Inc. | Product Security Engineer
>
> ----- Original Message -----
> > From: "Dmitry Vyukov" <dvyukov@google.com>
> > To: "Vladis Dronov" <vdronov@redhat.com>
> > Cc: "syzbot" <syzbot+d3402c47f680ff24b29c@syzkaller.appspotmail.com>, "syzkaller-bugs"
> > <syzkaller-bugs@googlegroups.com>, "David Miller" <davem@davemloft.net>, "Eric Dumazet" <edumazet@google.com>,
> > "LKML" <linux-kernel@vger.kernel.org>, "netdev" <netdev@vger.kernel.org>, "sunlianwen" <sunlw.fnst@cn.fujitsu.com>
> > Sent: Sunday, September 23, 2018 6:22:36 PM
> > Subject: Re: KMSAN: uninit-value in memcmp (2)
> >
> > On Sun, Sep 23, 2018 at 6:02 PM, Vladis Dronov <vdronov@redhat.com> wrote:
> > > #syz fix: net: fix uninit-value in __hw_addr_add_ex()
> >
> > Hi Vladis,
> >
> > This can be fixed with "net: fix uninit-value in __hw_addr_add_ex()".
> > That commit landed in April, syzbot waited till the commit reached all
> > tested trees, and then closed the bug.
> > But the similar bug continued to happen, so syzbot created second
> > version of this bug (2). You can see on dashboard that the last crash
> > for the second version (2) happened just few days ago. So this is a
> > different bug.
> >
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/1040580049.15456466.1537740558279.JavaMail.zimbra%40redhat.com.
> For more options, visit https://groups.google.com/d/optout.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
^ permalink raw reply
* RE: [PATCH] bonding: avoid repeated display of same link status change
From: Manish Kumar Singh @ 2018-09-24 7:05 UTC (permalink / raw)
To: Eric Dumazet, netdev
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David S. Miller,
linux-kernel
In-Reply-To: <05f57c08-3ebb-7ee5-7ab2-519cb5a70bd8@gmail.com>
> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: 18 सितम्बर 2018 19:30
> To: Manish Kumar Singh; Eric Dumazet; netdev@vger.kernel.org
> Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] bonding: avoid repeated display of same link status
> change
>
>
>
> On 09/17/2018 10:05 PM, Manish Kumar Singh wrote:
> >
> >
> >> -----Original Message-----
> >> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> >> Sent: 17 सितम्बर 2018 20:08
> >> To: Manish Kumar Singh; netdev@vger.kernel.org
> >> Cc: Jay Vosburgh; Veaceslav Falico; Andy Gospodarek; David S. Miller;
> linux-
> >> kernel@vger.kernel.org
> >> Subject: Re: [PATCH] bonding: avoid repeated display of same link status
> >> change
> >>
> >>
> >>
> >> On 09/17/2018 12:20 AM, mk.singh@oracle.com wrote:
> >>> From: Manish Kumar Singh <mk.singh@oracle.com>
> >>>
> >>> When link status change needs to be committed and rtnl lock couldn't be
> >>> taken, avoid redisplay of same link status change message.
> >>>
> >>> Signed-off-by: Manish Kumar Singh <mk.singh@oracle.com>
> >>> ---
> >>> drivers/net/bonding/bond_main.c | 6 ++++--
> >>> include/net/bonding.h | 1 +
> >>> 2 files changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bonding/bond_main.c
> >> b/drivers/net/bonding/bond_main.c
> >>> index 217b790d22ed..fb4e3aff1677 100644
> >>> --- a/drivers/net/bonding/bond_main.c
> >>> +++ b/drivers/net/bonding/bond_main.c
> >>> @@ -2087,7 +2087,7 @@ static int bond_miimon_inspect(struct bonding
> >> *bond)
> >>> bond_propose_link_state(slave, BOND_LINK_FAIL);
> >>> commit++;
> >>> slave->delay = bond->params.downdelay;
> >>> - if (slave->delay) {
> >>> + if (slave->delay && !bond->rtnl_needed) {
> >>> netdev_info(bond->dev, "link status down
> for
> >> %sinterface %s, disabling it in %d ms\n",
> >>> (BOND_MODE(bond) ==
> >>> BOND_MODE_ACTIVEBACKUP) ?
> >>> @@ -2127,7 +2127,7 @@ static int bond_miimon_inspect(struct bonding
> >> *bond)
> >>> commit++;
> >>> slave->delay = bond->params.updelay;
> >>>
> >>> - if (slave->delay) {
> >>> + if (slave->delay && !bond->rtnl_needed) {
> >>> netdev_info(bond->dev, "link status up for
> >> interface %s, enabling it in %d ms\n",
> >>> slave->dev->name,
> >>> ignore_updelay ? 0 :
> >>> @@ -2301,9 +2301,11 @@ static void bond_mii_monitor(struct
> >> work_struct *work)
> >>> if (!rtnl_trylock()) {
> >>> delay = 1;
> >>> should_notify_peers = false;
> >>> + bond->rtnl_needed = true;
> >>
> >> How can you set a shared variable with no synchronization ?
> > Thanks Eric for reviewing the patch. rtnl_needed is not a shared variable, it
> is part of bonding structure, that is one per bonding driver instance. There
> can't be two parallel instances of bond_miimon_inspect for a single bonding
> driver instance at any given point of time. and only bond_miimon_inspect
> updates it. That’s why I think there is no need of any synchronization here.
> >
> >
>
> If rtnl_trylock() can not grab RTNL,
> there is no way the current thread can set the variable without a race, if the
> word including rtnl_needed is shared by other fields in the structure.
>
> Your patch adds a subtle possibility of future bugs, even if it runs fine today.
>
> Do not pave the way for future bugs, make your code robust, please.
Thankyou Eric, we are making the changes and will repost the patch after testing it.
-Manish
>
>
>
>
>
>
>
^ permalink raw reply
* Re: [PATCH 1/2] gpiolib: Fix missing updates of bitmap index
From: Linus Walleij @ 2018-09-24 8:11 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg KH, kishon,
Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald, Jiri Slaby, Willy Tarreau,
Geert Uytterhoeven, Sebastien Bourdelin
In-Reply-To: <20180923235336.22148-2-jmkrzyszt@gmail.com>
On Mon, Sep 24, 2018 at 1:52 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> In new code introduced by commit b17566a6b08b ("gpiolib: Implement fast
> processing path in get/set array"), bitmap index is not updated with
> next found zero bit position as it should while skipping over pins
> already processed via fast bitmap path, possibly resulting in an
> infinite loop. Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Patch applied!
Thanks for working on getting this into shape!
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH net-next 0/2] net: mvpp2: Add txq to CPU mapping
From: Maxime Chevallier @ 2018-09-24 9:11 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Antoine Tenart,
thomas.petazzoni, gregory.clement, miquel.raynal, nadavh, stefanc,
ymarkman, mw
Hi everyone,
This short series adds XPS support to the mvpp2 driver, by mapping
txqs and CPUs. This comes with a patch using round-robin scheduling
for the HW to pick the next txq to transmit from, instead of the default
fixed-priority scheduling.
Maxime Chevallier (2):
net: mvpp2: support XPS by mapping TX queues to CPUs
net: mvpp2: use round-robin scheduling for TX queues on the same CPU
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 1 +
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 9 ++++++++-
2 files changed, 9 insertions(+), 1 deletion(-)
--
2.11.0
^ permalink raw reply
* [PATCH net-next 1/2] net: mvpp2: support XPS by mapping TX queues to CPUs
From: Maxime Chevallier @ 2018-09-24 9:11 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Antoine Tenart,
thomas.petazzoni, gregory.clement, miquel.raynal, nadavh, stefanc,
ymarkman, mw
In-Reply-To: <20180924091106.15094-1-maxime.chevallier@bootlin.com>
Since the PPv2 controller has multiple TX queues, we can spread traffic
by assining TX queues to CPUs, allowing to use XPS to balance egress
traffic between CPUs.
Suggested-by : Yan Markman <ymarkman@marvell.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index d30ccc515bb7..bdacb9577216 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -2423,13 +2423,17 @@ static int mvpp2_setup_rxqs(struct mvpp2_port *port)
static int mvpp2_setup_txqs(struct mvpp2_port *port)
{
struct mvpp2_tx_queue *txq;
- int queue, err;
+ int queue, err, cpu;
for (queue = 0; queue < port->ntxqs; queue++) {
txq = port->txqs[queue];
err = mvpp2_txq_init(port, txq);
if (err)
goto err_cleanup;
+
+ /* Assign this queue to a CPU */
+ cpu = queue % num_present_cpus();
+ netif_set_xps_queue(port->dev, cpumask_of(cpu), queue);
}
if (port->has_tx_irqs) {
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 2/2] net: mvpp2: use round-robin scheduling for TX queues on the same CPU
From: Maxime Chevallier @ 2018-09-24 9:11 UTC (permalink / raw)
To: davem
Cc: Maxime Chevallier, netdev, linux-kernel, Antoine Tenart,
thomas.petazzoni, gregory.clement, miquel.raynal, nadavh, stefanc,
ymarkman, mw
In-Reply-To: <20180924091106.15094-1-maxime.chevallier@bootlin.com>
This commit allows each TXQ to be picked in a round-robin fashion by
the PPv2 transmit scheduling mechanism. This is opposed to the default
behaviour that prioritizes the highest numbered queues.
Suggested-by: Yan Markman <ymarkman@marvell.com>
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 1 +
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 3 +++
2 files changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
index f5dceef60b0e..176c6b56fdcc 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
@@ -331,6 +331,7 @@
#define MVPP2_TXP_SCHED_ENQ_MASK 0xff
#define MVPP2_TXP_SCHED_DISQ_OFFSET 8
#define MVPP2_TXP_SCHED_CMD_1_REG 0x8010
+#define MVPP2_TXP_SCHED_FIXED_PRIO_REG 0x8014
#define MVPP2_TXP_SCHED_PERIOD_REG 0x8018
#define MVPP2_TXP_SCHED_MTU_REG 0x801c
#define MVPP2_TXP_MTU_MAX 0x7FFFF
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index bdacb9577216..c2ed71788e4f 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
@@ -1448,6 +1448,9 @@ static void mvpp2_defaults_set(struct mvpp2_port *port)
tx_port_num);
mvpp2_write(port->priv, MVPP2_TXP_SCHED_CMD_1_REG, 0);
+ /* Set TXQ scheduling to Round-Robin */
+ mvpp2_write(port->priv, MVPP2_TXP_SCHED_FIXED_PRIO_REG, 0);
+
/* Close bandwidth for all queues */
for (queue = 0; queue < MVPP2_MAX_TXQ; queue++) {
ptxq = mvpp2_txq_phys(port->id, queue);
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers
From: David Miller @ 2018-09-24 5:04 UTC (permalink / raw)
To: edumazet
Cc: netdev, michael.chan, ariel.elior, eric.dumazet, tariqt, saeedm,
jeffrey.t.kirsher, jakub.kicinski, songliubraving, j.vosburgh,
vfalico, andy
In-Reply-To: <20180921222752.101307-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 21 Sep 2018 15:27:37 -0700
> As diagnosed by Song Liu, ndo_poll_controller() can
> be very dangerous on loaded hosts, since the cpu
> calling ndo_poll_controller() might steal all NAPI
> contexts (for all RX/TX queues of the NIC).
>
> This capture, showing one ksoftirqd eating all cycles
> can last for unlimited amount of time, since one
> cpu is generally not able to drain all the queues under load.
>
> It seems that all networking drivers that do use NAPI
> for their TX completions, should not provide a ndo_poll_controller() :
>
> Most NAPI drivers have netpoll support already handled
> in core networking stack, since netpoll_poll_dev()
> uses poll_napi(dev) to iterate through registered
> NAPI contexts for a device.
>
> This patch series take care of the first round, we will
> handle other drivers in future rounds.
Series applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path
From: Janusz Krzysztofik @ 2018-09-24 11:08 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Andrew Lunn, Ulf Hansson, linux-doc, Tony Lindgren, Linus Walleij,
Dominik Brodowski, Peter Rosin, netdev, linux-i2c,
Peter Meerwald-Stadler, devel, Florian Fainelli, Jonathan Corbet,
Krzysztof Kozlowski, Kishon Vijay Abraham I, linux-iio,
Peter Korsgaard, Geert Uytterhoeven, linux-serial, Jiri Slaby,
Michael Hennerich, Uwe Kleine-König, linux-gpio,
Russell King
In-Reply-To: <20180924094339eucas1p282f2f7cb627c183fe87da044edb90fa5~XTMgQ4GFs1339913399eucas1p2N@eucas1p2.samsung.com>
Hi Marek,
2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
> Hi Janusz,
>
> On 2018-09-24 01:53, Janusz Krzysztofik wrote:
>> While investigating possible reasons of GPIO fast bitmap processing
>> related boot hang on Samsung Snow Chromebook, reported by Marek
>> Szyprowski (thanks!), I've discovered one coding bug, addressed by
>> PATCH 1/2 of this series, and one potential regression introduced at
>> design level of the solution, hopefully fixed by PATCH 2/2. See
>> commit messages for details.
>>
>> Janusz Krzysztofik (2):
>> gpiolib: Fix missing updates of bitmap index
>> gpiolib: Fix array members of same chip processed separately
>>
>> The fixes should resolve the boot hang observed by Marek, however the
>> second change excludes that particular case from fast bitmap processing
>> and restores the old behaviour.
>
> I confirm, that the above 2 patches fixes boot issue on Samsung Snow
> Chromebook with next-20180920.
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
>> Hence, it is possible still another
>> issue which have had an influence on that boot hang exists in the code.
>> In order to fully verify the fix, it would have to be tested on a
>> platform where an array of GPIO descriptors is used which starts from
>> at least two consecutive pins of one GPIO chip in hardware order,
>> starting ftom 0, followed by one or more pins belonging to other
>> chip(s).
>>
>> In order to verify if separate calls to .set() chip callback for each
>> pin instead of one call to .set_multiple() is actually the reason of
>> boot hang on Samsung Snow Chromebook, the affected driver -
>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
>> modified for testing purposes so it calls gpiod_set_value() for each
>> pin instead of gpiod_set_array_value() for all of them. If that would
>> also result in boot hang, we could be sure the issue was really the
>> one addressed by the second fix. Marek, could you please try to
>> perform such test?
>
> Yes, I've just tested next-20180920 only with the first patch from this
> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
> It boots fine, so indeed the issue is in handling of arrays of gpios.
>
> Just to be sure I did it right, this is my change to the mentioned file:
Yeah, that's what I had on mind. However, I'd be more lucky if it didn't work
for you. Setting the pins sequentially, not simultaneously as before, was
exactly what I hoped was the reason of the hang.
> diff --git a/drivers/mmc/core/pwrseq_simple.c
> b/drivers/mmc/core/pwrseq_simple.c
> index 7f882a2bb872..9397dc1f2e38 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct
> mmc_pwrseq_simple *pwrseq,
> int value)
> {
> struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
> + int i;
>
> - if (!IS_ERR(reset_gpios)) {
> - DECLARE_BITMAP(values, BITS_PER_TYPE(value));
> - int nvalues = reset_gpios->ndescs;
> -
> - values[0] = value;
> -
> - gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
> - reset_gpios->info, values);
> - }
> + if (!IS_ERR(reset_gpios))
> + for (i = 0; i < reset_gpios->ndescs; i++)
The only difference from the behaviour when the hang was occurring is now
the order the pins are manipulated. Maybe that matters?
Could you please retry the same with the order of pins reversed, either in
the .dts file or here inside this for loop?
Thanks,
Janusz
> + gpiod_set_value_cansleep(reset_gpios->desc[i], value);
> }
>
> static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>
^ 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