* Re: [PATCH net-next 0/4] liquidio: improve soft command/response handling
From: David Miller @ 2018-08-30 3:07 UTC (permalink / raw)
To: felix.manlunas
Cc: netdev, raghu.vatsavayi, derek.chickles, satananda.burla,
weilin.chang
In-Reply-To: <20180829015058.GA7898@felix-thinkpad.cavium.com>
From: Felix Manlunas <felix.manlunas@cavium.com>
Date: Tue, 28 Aug 2018 18:50:58 -0700
> From: Weilin Chang <weilin.chang@cavium.com>
>
> Change soft command handling to fix the possible race condition when the
> process handles a response of a soft command that was already freed by an
> application which got timeout for this request.
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH 3/4] clk: x86: Stop marking clocks as CLK_IS_CRITICAL
From: Stephen Boyd @ 2018-08-30 3:46 UTC (permalink / raw)
To: David S . Miller, Andy Shevchenko, Hans de Goede, Heiner Kallweit,
Irina Tirdea, Michael Turquette
Cc: Hans de Goede, netdev, Johannes Stezenbach, Carlo Caione,
linux-clk
In-Reply-To: <20180827143200.8597-4-hdegoede@redhat.com>
Quoting Hans de Goede (2018-08-27 07:31:59)
> Commit d31fd43c0f9a ("clk: x86: Do not gate clocks enabled by the
> firmware"), which added the code to mark clocks as CLK_IS_CRITICAL, causes
> all unclaimed PMC clocks on Cherry Trail devices to be on all the time,
> resulting on the device not being able to reach S0i3 when suspended.
>
> The reason for this commit is that on some Bay Trail / Cherry Trail devices
> the r8169 ethernet controller uses pmc_plt_clk_4. Now that the clk-pmc-atom
> driver exports an "ether_clk" alias for pmc_plt_clk_4 and the r8169 driver
> has been modified to get and enable this clock (if present) the marking of
> the clocks as CLK_IS_CRITICAL is no longer necessary.
>
> This commit removes the CLK_IS_CRITICAL marking, fixing Cherry Trail
> devices not being able to reach S0i3 greatly decreasing their battery
> drain when suspended.
>
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=193891#c102
> Cc: Johannes Stezenbach <js@sig21.net>
> Cc: Carlo Caione <carlo@endlessm.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
Acked-by: Stephen Boyd <sboyd@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
From: Peter Rosin @ 2018-08-30 4:30 UTC (permalink / raw)
To: Janusz Krzysztofik, Linus Walleij
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
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, linux-doc, linux-i2
In-Reply-To: <20180829204900.19390-2-jmkrzyszt@gmail.com>
On 2018-08-29 22:48, Janusz Krzysztofik wrote:
> Most users of get/set array functions iterate consecutive bits of data,
> usually a single integer, while processing array of results obtained
> from, or building an array of values to be passed to those functions.
> Save time wasted on those iterations by changing the functions' API to
> accept bitmaps.
>
> All current users are updated as well.
>
> More benefits from the change are expected as soon as planned support
> for accepting/passing those bitmaps directly from/to respective GPIO
> chip callbacks if applicable is implemented.
> diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
> index 401308e3d036..4e36e0eac7a3 100644
> --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> @@ -22,18 +22,16 @@ struct gpiomux {
> struct i2c_mux_gpio_platform_data data;
> unsigned gpio_base;
> struct gpio_desc **gpios;
> - int *values;
> + int *values; /* FIXME: no longer needed */
That's a half-measure...
> };
>
> static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
> {
> + unsigned long value_bitmap[1] = { val, };
> int i;
...and i is no longer needed. Hmm, I'd expect a warning about that?
>
> - for (i = 0; i < mux->data.n_gpios; i++)
> - mux->values[i] = (val >> i) & 1;
> -
> gpiod_set_array_value_cansleep(mux->data.n_gpios,
> - mux->gpios, mux->values);
> + mux->gpios, value_bitmap);
> }
>
> static int i2c_mux_gpio_select(struct i2c_mux_core *muxc, u32 chan)
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 6fdd9316db8b..734e1b43aed6 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -17,20 +17,17 @@
>
> struct mux_gpio {
> struct gpio_descs *gpios;
> - int *val;
> + int *val; /* FIXME: no longer needed */
> };
>
> static int mux_gpio_set(struct mux_control *mux, int state)
> {
> struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
> + unsigned long value_bitmap[1] = { state, };
> int i;
>
> - for (i = 0; i < mux_gpio->gpios->ndescs; i++)
> - mux_gpio->val[i] = (state >> i) & 1;
> -
> gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
> - mux_gpio->gpios->desc,
> - mux_gpio->val);
> + mux_gpio->gpios->desc, value_bitmap);
>
> return 0;
> }
Dito for this driver. Just squash the following and be done with
it, no attribution needed...
With that (for the changes in i2c-mux-gpio.c and mux/gpio.c)
Reviewed-by: Peter Rosin <peda@axentia.se>
Linus, I expect this will will end up in some immutable branch for me
to pick up, in case I need to? Not that I expect further work to clash
in these two drivers, but...
Cheers,
Peter
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4e36e0eac7a3..06a89a29250a 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -22,13 +22,11 @@ struct gpiomux {
struct i2c_mux_gpio_platform_data data;
unsigned gpio_base;
struct gpio_desc **gpios;
- int *values; /* FIXME: no longer needed */
};
static void i2c_mux_gpio_set(const struct gpiomux *mux, unsigned val)
{
unsigned long value_bitmap[1] = { val, };
- int i;
gpiod_set_array_value_cansleep(mux->data.n_gpios,
mux->gpios, value_bitmap);
@@ -180,15 +178,13 @@ static int i2c_mux_gpio_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
muxc = i2c_mux_alloc(parent, &pdev->dev, mux->data.n_values,
- mux->data.n_gpios * sizeof(*mux->gpios) +
- mux->data.n_gpios * sizeof(*mux->values), 0,
+ mux->data.n_gpios * sizeof(*mux->gpios), 0,
i2c_mux_gpio_select, NULL);
if (!muxc) {
ret = -ENOMEM;
goto alloc_failed;
}
mux->gpios = muxc->priv;
- mux->values = (int *)(mux->gpios + mux->data.n_gpios);
muxc->priv = mux;
platform_set_drvdata(pdev, muxc);
diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
index 734e1b43aed6..eb1798677dfd 100644
--- a/drivers/mux/gpio.c
+++ b/drivers/mux/gpio.c
@@ -17,14 +17,12 @@
struct mux_gpio {
struct gpio_descs *gpios;
- int *val; /* FIXME: no longer needed */
};
static int mux_gpio_set(struct mux_control *mux, int state)
{
struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
unsigned long value_bitmap[1] = { state, };
- int i;
gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
mux_gpio->gpios->desc, value_bitmap);
@@ -55,13 +53,11 @@ static int mux_gpio_probe(struct platform_device *pdev)
if (pins < 0)
return pins;
- mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
- pins * sizeof(*mux_gpio->val));
+ mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio));
if (IS_ERR(mux_chip))
return PTR_ERR(mux_chip);
mux_gpio = mux_chip_priv(mux_chip);
- mux_gpio->val = (int *)(mux_gpio + 1);
mux_chip->ops = &mux_gpio_ops;
mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
^ permalink raw reply related
* Re: [PATCH net-next 0/5] rtnetlink: add IFA_IF_NETNSID for RTM_GETADDR
From: Kirill Tkhai @ 2018-08-30 8:49 UTC (permalink / raw)
To: Christian Brauner
Cc: netdev, linux-kernel, davem, kuznet, yoshfuji, pombredanne,
kstewart, gregkh, dsahern, fw, lucien.xin, jakub.kicinski, jbenc,
nicolas.dichtel
In-Reply-To: <20180829181303.4sacopk7y3p5xyou@gmail.com>
On 29.08.2018 21:13, Christian Brauner wrote:
> Hi Kirill,
>
> Thanks for the question!
>
> On Wed, Aug 29, 2018 at 11:30:37AM +0300, Kirill Tkhai wrote:
>> Hi, Christian,
>>
>> On 29.08.2018 02:18, Christian Brauner wrote:
>>> From: Christian Brauner <christian@brauner.io>
>>>
>>> Hey,
>>>
>>> A while back we introduced and enabled IFLA_IF_NETNSID in
>>> RTM_{DEL,GET,NEW}LINK requests (cf. [1], [2], [3], [4], [5]). This has led
>>> to signficant performance increases since it allows userspace to avoid
>>> taking the hit of a setns(netns_fd, CLONE_NEWNET), then getting the
>>> interfaces from the netns associated with the netns_fd. Especially when a
>>> lot of network namespaces are in use, using setns() becomes increasingly
>>> problematic when performance matters.
>>
>> could you please give a real example, when setns()+socket(AF_NETLINK) cause
>> problems with the performance? You should do this only once on application
>> startup, and then you have created netlink sockets in any net namespaces you
>> need. What is the problem here?
>
> So we have a daemon (LXD) that is often running thousands of containers.
> When users issue a lxc list request against the daemon it returns a list
> of all containers including all of the interfaces and addresses for each
> container. To retrieve those addresses we currently rely on setns() +
> getifaddrs() for each of those containers. That has horrible
> performance.
Could you please provide some numbers showing that setns()
introduces signify performance decrease in the application?
I worry about all this just because of netlink interface is
already overloaded, while this patch makes it even less modular.
In case of one day we finally reach rtnl unscalability trap,
every common interface like this may be a decisive nail in
a coffin lid of possibility to overwrite everything.
> The problem with what you're proposing is that the daemon would need to
> cache a socket file descriptor for each container which is something
> that we unfortunately cannot do since we can't excessively cache file
> descriptors because we can easily hit the open file limit. We also
> refrain from caching file descriptors for a long time for security
> reasons.
>
> For the case where users just request a list of the interfaces we
> can already use RTM_GETLINK + IFLA_IF_NETNS which has way better
> performance. But we can't do the same with RTM_GETADDR requests which
> was an oversight on my part when I wrote the original patchset for the
> RTM_*LINK requests. This just rectifies this and aligns RTM_GETLINK +
> RTM_GETADDR.
> Based on this patchset I have written a userspace POC that is basically
> a netns namespace aware getifaddr() or - as I like to call it -
> netns_getifaddr().
>
>>
>>> Usually, RTML_GETLINK requests are followed by RTM_GETADDR requests (cf.
>>> getifaddrs() style functions and friends). But currently, RTM_GETADDR
>>> requests do not support a similar property like IFLA_IF_NETNSID for
>>> RTM_*LINK requests.
>>> This is problematic since userspace can retrieve interfaces from another
>>> network namespace by sending a IFLA_IF_NETNSID property along but
>>> RTM_GETLINK request but is still forced to use the legacy setns() style of
>>> retrieving interfaces in RTM_GETADDR requests.
>>>
>>> The goal of this series is to make it possible to perform RTM_GETADDR
>>> requests on different network namespaces. To this end a new IFA_IF_NETNSID
>>> property for RTM_*ADDR requests is introduced. It can be used to send a
>>> network namespace identifier along in RTM_*ADDR requests. The network
>>> namespace identifier will be used to retrieve the target network namespace
>>> in which the request is supposed to be fulfilled. This aligns the behavior
>>> of RTM_*ADDR requests with the behavior of RTM_*LINK requests.
>>>
>>> Security:
>>> - The caller must have assigned a valid network namespace identifier for
>>> the target network namespace.
>>> - The caller must have CAP_NET_ADMIN in the owning user namespace of the
>>> target network namespace.
>>>
>>> Thanks!
>>> Christian
>>>
>>> [1]: commit 7973bfd8758d ("rtnetlink: remove check for IFLA_IF_NETNSID")
>>> [2]: commit 5bb8ed075428 ("rtnetlink: enable IFLA_IF_NETNSID for RTM_NEWLINK")
>>> [3]: commit b61ad68a9fe8 ("rtnetlink: enable IFLA_IF_NETNSID for RTM_DELLINK")
>>> [4]: commit c310bfcb6e1b ("rtnetlink: enable IFLA_IF_NETNSID for RTM_SETLINK")
>>> [5]: commit 7c4f63ba8243 ("rtnetlink: enable IFLA_IF_NETNSID in do_setlink()")
>>>
>>> Christian Brauner (5):
>>> rtnetlink: add rtnl_get_net_ns_capable()
>>> if_addr: add IFA_IF_NETNSID
>>> ipv4: enable IFA_IF_NETNSID for RTM_GETADDR
>>> ipv6: enable IFA_IF_NETNSID for RTM_GETADDR
>>> rtnetlink: move type calculation out of loop
>>>
>>> include/net/rtnetlink.h | 1 +
>>> include/uapi/linux/if_addr.h | 1 +
>>> net/core/rtnetlink.c | 15 +++++---
>>> net/ipv4/devinet.c | 38 +++++++++++++++-----
>>> net/ipv6/addrconf.c | 70 ++++++++++++++++++++++++++++--------
>>> 5 files changed, 97 insertions(+), 28 deletions(-)
>>>
^ permalink raw reply
* Re: [PATCH][net-next] vxlan: reduce dirty cache line in vxlan_find_mac
From: David Miller @ 2018-08-30 5:08 UTC (permalink / raw)
To: lirongqing; +Cc: netdev
In-Reply-To: <1535514730-21032-1-git-send-email-lirongqing@baidu.com>
From: Li RongQing <lirongqing@baidu.com>
Date: Wed, 29 Aug 2018 11:52:10 +0800
> vxlan_find_mac() unconditionally set f->used for every packet,
> this causes a cache miss for every packet, since remote, hlist
> and used of vxlan_fdb share the same cache line, which are
> accessed when send every packets.
>
> so f->used is set only if not equal to jiffies, to reduce dirty
> cache line times, this gives 3% speed-up with small packets.
>
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
Applied.
^ permalink raw reply
* Re: [PATCH v2] net: mvpp2: initialize port of_node pointer
From: David Miller @ 2018-08-30 5:09 UTC (permalink / raw)
To: baruch
Cc: maxime.chevallier, antoine.tenart, jason, andrew, gregory.clement,
sebastian.hesselbarth, linux, ori.shemtov, netdev
In-Reply-To: <2a0c56bcd61ad29b6318af95c9be06a58c610d80.1535525079.git.baruch@tkos.co.il>
From: Baruch Siach <baruch@tkos.co.il>
Date: Wed, 29 Aug 2018 09:44:39 +0300
> Without a valid of_node in struct device we can't find the mvpp2 port
> device by its DT node. Specifically, this breaks
> of_find_net_device_by_node().
>
> For example, the Armada 8040 based Clearfog GT-8K uses Marvell 88E6141
> switch connected to the &cp1_eth2 port:
>
> &cp1_mdio {
> ...
>
> switch0: switch0@4 {
> compatible = "marvell,mv88e6085";
> ...
>
> ports {
> ...
>
> port@5 {
> reg = <5>;
> label = "cpu";
> ethernet = <&cp1_eth2>;
> };
> };
> };
> };
>
> Without this patch, dsa_register_switch() returns -EPROBE_DEFER because
> of_find_net_device_by_node() can't find the device_node of the &cp1_eth2
> device.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
Applied.
^ permalink raw reply
* Re: [PATCH net v2 0/2] net_sched: reject unknown tcfa_action values
From: David Miller @ 2018-08-30 5:11 UTC (permalink / raw)
To: pabeni; +Cc: netdev, jhs, xiyou.wangcong, jiri, dcaratti, lucasb
In-Reply-To: <cover.1535527298.git.pabeni@redhat.com>
From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 29 Aug 2018 10:22:32 +0200
> As agreed some time ago, this changeset reject unknown tcfa_action values,
> instead of changing such values under the hood.
>
> A tdc test is included to verify the new behavior.
>
> v1 -> v2:
> - helper is now static and renamed according to act_* convention
> - updated extack message, according to the new behavior
Series applied, thank you.
^ permalink raw reply
* Re: pull-request: mac80211-next 2018-08-29
From: David Miller @ 2018-08-30 5:14 UTC (permalink / raw)
To: johannes; +Cc: netdev, linux-wireless
In-Reply-To: <20180829115353.31679-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 29 Aug 2018 13:53:52 +0200
> Here's a small update for net-next, there's not much but since we
> still don't have a shared tree, the ieee80211_send_layer2_update()
> should go to Kalle through your tree :)
>
> Please pull and let me know if there's any problem.
Pulled, thanks Johannes.
^ permalink raw reply
* Re: [PATCH] rtnetlink: expose value from SET_NETDEV_DEVTYPE via IFLA_DEVTYPE attribute
From: Jiri Pirko @ 2018-08-30 5:12 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: netdev, David S. Miller
In-Reply-To: <2E0F93BA-74CD-4E5E-8FDC-CC2B8A373B69@holtmann.org>
Wed, Aug 29, 2018 at 05:23:58PM CEST, marcel@holtmann.org wrote:
>Hi Jiri,
>
>>> The name value from SET_NETDEV_DEVTYPE only ended up in the uevent sysfs
>>> file as DEVTYPE= information. To avoid any kind of race conditions
>>> between netlink messages and reading from sysfs, it is useful to add the
>>> same string as new IFLA_DEVTYPE attribute included in the RTM_NEWLINK
>>> messages.
>>>
>>> For network managing daemons that have to classify ARPHRD_ETHER network
>>> devices into different types (like Wireless LAN, Bluetooth etc.), this
>>> avoids the extra round trip to sysfs and parsing of the uevent file.
>>>
>>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>> ---
>>> include/uapi/linux/if_link.h | 2 ++
>>> net/core/rtnetlink.c | 12 ++++++++++++
>>> 2 files changed, 14 insertions(+)
>>>
>>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>> index 43391e2d1153..781294972bb4 100644
>>> --- a/include/uapi/linux/if_link.h
>>> +++ b/include/uapi/linux/if_link.h
>>> @@ -166,6 +166,8 @@ enum {
>>> IFLA_NEW_IFINDEX,
>>> IFLA_MIN_MTU,
>>> IFLA_MAX_MTU,
>>> + IFLA_DEVTYPE, /* Name value from SET_NETDEV_DEVTYPE */
>>
>> This is not something netdev-related. dev->dev.type is struct device_type.
>> This is a generic "device" thing. Incorrect to expose over
>> netdev-specific API. Please use "device" API for this.
>
>it is not just "device" related since this is a sub-classification of ARPHRD_ETHER type as a wrote above. Don't get hang up that this information is part of struct device. The dev->dev.type contains strings like "wlan", "bluetooth", "wimax", "gadget" etc. so that you can tell what kind of Ethernet card you have.
I understand. But using dev->dev.type for that purpose seems like an
abuse. If this is subtype of netdev, it should not be stored in parent
device. I believe that you need to re-introduce this as a part of struct
net_device - preferably an enum - and that you can expose over rtnl (and
net-sysfs).
>
>We can revive the patches for adding this information as IFLA_INFO_KIND, but last time they where reverted since NetworkManager did all the wrong decisions based on that. That part is fixed now and we can put that back and declare the sub-type classification of Ethernet device in two places. Or we just take the information that we added a long time ago for exactly this sub-classification and provide them to userspace via RTNL.
IFLA_INFO_KIND serves for different purpose. It is for drivers that
register rtnl_link_ops. I don't think it would be wise to mix it with
this.
Btw, could you wrap the sentences at 72 cols? Would be easier to read
that way.
>
>Regards
>
>Marcel
>
^ permalink raw reply
* Re: [PATCH] rtnetlink: expose value from SET_NETDEV_DEVTYPE via IFLA_DEVTYPE attribute
From: Jiri Pirko @ 2018-08-30 5:14 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Marcel Holtmann, netdev, davem
In-Reply-To: <20180829082428.0da2d748@xeon-e3>
Wed, Aug 29, 2018 at 05:24:28PM CEST, stephen@networkplumber.org wrote:
>On Wed, 29 Aug 2018 09:18:55 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Tue, Aug 28, 2018 at 10:58:11PM CEST, marcel@holtmann.org wrote:
>> >The name value from SET_NETDEV_DEVTYPE only ended up in the uevent sysfs
>> >file as DEVTYPE= information. To avoid any kind of race conditions
>> >between netlink messages and reading from sysfs, it is useful to add the
>> >same string as new IFLA_DEVTYPE attribute included in the RTM_NEWLINK
>> >messages.
>> >
>> >For network managing daemons that have to classify ARPHRD_ETHER network
>> >devices into different types (like Wireless LAN, Bluetooth etc.), this
>> >avoids the extra round trip to sysfs and parsing of the uevent file.
>> >
>> >Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>> >---
>> > include/uapi/linux/if_link.h | 2 ++
>> > net/core/rtnetlink.c | 12 ++++++++++++
>> > 2 files changed, 14 insertions(+)
>> >
>> >diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> >index 43391e2d1153..781294972bb4 100644
>> >--- a/include/uapi/linux/if_link.h
>> >+++ b/include/uapi/linux/if_link.h
>> >@@ -166,6 +166,8 @@ enum {
>> > IFLA_NEW_IFINDEX,
>> > IFLA_MIN_MTU,
>> > IFLA_MAX_MTU,
>> >+ IFLA_DEVTYPE, /* Name value from SET_NETDEV_DEVTYPE */
>>
>> This is not something netdev-related. dev->dev.type is struct device_type.
>> This is a generic "device" thing. Incorrect to expose over
>> netdev-specific API. Please use "device" API for this.
>
>There is no device API in netlink. The whole point of this patch is to
>do it in one message. It might be a performance optimization, but I can't
>see how it could be a race condition. Devices set type before registering.
I understand. My point is to avoid exposing generic "struct device"
values over rtnetlink. It mixes apples and oranges.
^ permalink raw reply
* Re: [PATCH] Revert "net: stmmac: Do not keep rearming the coalesce timer in stmmac_xmit"
From: Jerome Brunet @ 2018-08-30 9:21 UTC (permalink / raw)
To: Martin Blumenstingl, Jose.Abreu
Cc: peppe.cavallaro, alexandre.torgue, netdev, linux-amlogic,
Joao.Pinto, clabbe, linux-kernel, Vitor.Soares
In-Reply-To: <CAFBinCA+Q6vLzutdMLOgeTDg5GBkqvp3YzBQiUd_y-qyiz=dsQ@mail.gmail.com>
On Wed, 2018-08-29 at 20:03 +0200, Martin Blumenstingl wrote:
> Hi Jose,
>
> On Tue, Aug 28, 2018 at 10:12 AM Jose Abreu <Jose.Abreu@synopsys.com> wrote:
> >
> > Hi Jerome,
> >
> > On 24-08-2018 10:04, Jerome Brunet wrote:
> > > This reverts commit 4ae0169fd1b3c792b66be58995b7e6b629919ecf.
> > >
> > > This change in the handling of the coalesce timer is causing regression on
> > > (at least) amlogic platforms.
> > >
> > > Network will break down very quickly (a few seconds) after starting
> > > a download. This can easily be reproduced using iperf3 for example.
> > >
> > > The problem has been reported on the S805, S905, S912 and A113 SoCs
> > > (Realtek and Micrel PHYs) and it is likely impacting all Amlogics
> > > platforms using Gbit ethernet
> > >
> > > No problem was seen with the platform using 10/100 only PHYs (GXL internal)
> > >
> > > Reverting change brings things back to normal and allows to use network
> > > again until we better understand the problem with the coalesce timer.
> > >
> > >
> >
> > Apologies for the delayed answer but I was in FTO.
>
> I hope you were able to enjoy your time off
>
> > I'm not sure what can be causing this but I have some questions
> > for you:
> > - What do you mean by "network will break down"? Do you see
> > queue timeout?
>
> in case of iperf3 traffic just stops
>
> > - What do you see in ethtool/ifconfig stats? Can you send me
> > the stats before and after network break?
>
> see below for my exact steps. let me know if you need more information
> (but be prepared for some delay until I respond)
>
> > - Is your setup multi-queue/channel?
>
> we don't specify anything in the .dtsi files of our platform, so the
> "default" is being used
>
> > - Can you point me to the DT bindings of your setup?
>
> in this specific case I used
> arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dts which inherits
> the platform settings from arch/arm64/boot/dts/amlogic/meson-gx.dtsi
problem is also confirmed of the following devices:
* arch/arm64/boot/dts/amlogic/meson-axg-s400.dts
* arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
* arch/arm64/boot/dts/amlogic/meson-gxbb-p200.dts
* arch/arm/boot/dts/meson8b-odroidc1.dts
But AFAIK, all amlogic platforms with GBe PHY are impacted
> ("ethmac" node)
>
> so here's my exact steps to reproduce:
> - boot the board
> - IP address is auto-assigned via DHCP
> - iperf3 -c 192.168.1.100
> - wait until network breaks, CTRL+C
> - ifconfig eth0 down
> - ifconfig eth0 up
> - (now Ethernet is working again)
>
> from that point on:
> # dmesg | tail -n 4
> [ 47.782778] RTL8211F Gigabit Ethernet 0.2009087f:00: attached PHY
> driver [RTL8211F Gigabit Ethernet] (mii_bus:phy_addr=0.2009087f:00,
> irq=30)
> [ 47.803540] meson8b-dwmac c9410000.ethernet eth0: No Safety
> Features support found
> [ 47.805505] meson8b-dwmac c9410000.ethernet eth0: PTP not supported by HW
> [ 50.311975] meson8b-dwmac c9410000.ethernet eth0: Link is Up -
> 1Gbps/Full - flow control rx/tx
>
> # ethtool -S eth0
> NIC statistics:
> mmc_tx_octetcount_gb: 0
> mmc_tx_framecount_gb: 0
> mmc_tx_broadcastframe_g: 0
> mmc_tx_multicastframe_g: 0
> mmc_tx_64_octets_gb: 0
> mmc_tx_65_to_127_octets_gb: 0
> mmc_tx_128_to_255_octets_gb: 0
> mmc_tx_256_to_511_octets_gb: 0
> mmc_tx_512_to_1023_octets_gb: 0
> mmc_tx_1024_to_max_octets_gb: 0
> mmc_tx_unicast_gb: 0
> mmc_tx_multicast_gb: 0
> mmc_tx_broadcast_gb: 0
> mmc_tx_underflow_error: 0
> mmc_tx_singlecol_g: 0
> mmc_tx_multicol_g: 0
> mmc_tx_deferred: 0
> mmc_tx_latecol: 0
> mmc_tx_exesscol: 0
> mmc_tx_carrier_error: 0
> mmc_tx_octetcount_g: 0
> mmc_tx_framecount_g: 0
> mmc_tx_excessdef: 0
> mmc_tx_pause_frame: 0
> mmc_tx_vlan_frame_g: 0
> mmc_rx_framecount_gb: 43
> mmc_rx_octetcount_gb: 3096
> mmc_rx_octetcount_g: 3096
> mmc_rx_broadcastframe_g: 38
> mmc_rx_multicastframe_g: 0
> mmc_rx_crc_error: 0
> mmc_rx_align_error: 0
> mmc_rx_run_error: 0
> mmc_rx_jabber_error: 0
> mmc_rx_undersize_g: 0
> mmc_rx_oversize_g: 0
> mmc_rx_64_octets_gb: 29
> mmc_rx_65_to_127_octets_gb: 13
> mmc_rx_128_to_255_octets_gb: 0
> mmc_rx_256_to_511_octets_gb: 1
> mmc_rx_512_to_1023_octets_gb: 0
> mmc_rx_1024_to_max_octets_gb: 0
> mmc_rx_unicast_g: 5
> mmc_rx_length_error: 0
> mmc_rx_autofrangetype: 0
> mmc_rx_pause_frames: 0
> mmc_rx_fifo_overflow: 0
> mmc_rx_vlan_frames_gb: 0
> mmc_rx_watchdog_error: 0
> mmc_rx_ipc_intr_mask: 1073692671
> mmc_rx_ipc_intr: 0
> mmc_rx_ipv4_gd: 15
> mmc_rx_ipv4_hderr: 0
> mmc_rx_ipv4_nopay: 0
> mmc_rx_ipv4_frag: 0
> mmc_rx_ipv4_udsbl: 0
> mmc_rx_ipv4_gd_octets: 1028
> mmc_rx_ipv4_hderr_octets: 0
> mmc_rx_ipv4_nopay_octets: 0
> mmc_rx_ipv4_frag_octets: 0
> mmc_rx_ipv4_udsbl_octets: 0
> mmc_rx_ipv6_gd_octets: 0
> mmc_rx_ipv6_hderr_octets: 0
> mmc_rx_ipv6_nopay_octets: 0
> mmc_rx_ipv6_gd: 0
> mmc_rx_ipv6_hderr: 0
> mmc_rx_ipv6_nopay: 0
> mmc_rx_udp_gd: 11
> mmc_rx_udp_err: 0
> mmc_rx_tcp_gd: 4
> mmc_rx_tcp_err: 0
> mmc_rx_icmp_gd: 0
> mmc_rx_icmp_err: 0
> mmc_rx_udp_gd_octets: 592
> mmc_rx_udp_err_octets: 0
> mmc_rx_tcp_gd_octets: 136
> mmc_rx_tcp_err_octets: 0
> mmc_rx_icmp_gd_octets: 0
> mmc_rx_icmp_err_octets: 0
> tx_underflow: 0
> tx_carrier: 0
> tx_losscarrier: 0
> vlan_tag: 0
> tx_deferred: 0
> tx_vlan: 0
> tx_jabber: 0
> tx_frame_flushed: 0
> tx_payload_error: 0
> tx_ip_header_error: 0
> rx_desc: 0
> sa_filter_fail: 0
> overflow_error: 0
> ipc_csum_error: 0
> rx_collision: 0
> rx_crc_errors: 0
> dribbling_bit: 0
> rx_length: 0
> rx_mii: 0
> rx_multicast: 0
> rx_gmac_overflow: 0
> rx_watchdog: 0
> da_rx_filter_fail: 0
> sa_rx_filter_fail: 0
> rx_missed_cntr: 0
> rx_overflow_cntr: 0
> rx_vlan: 0
> tx_undeflow_irq: 0
> tx_process_stopped_irq: 0
> tx_jabber_irq: 0
> rx_overflow_irq: 0
> rx_buf_unav_irq: 0
> rx_process_stopped_irq: 0
> rx_watchdog_irq: 0
> tx_early_irq: 0
> fatal_bus_error_irq: 0
> rx_early_irq: 0
> threshold: 1
> tx_pkt_n: 8
> rx_pkt_n: 43
> normal_irq_n: 38
> rx_normal_irq_n: 34
> napi_poll: 38
> tx_normal_irq_n: 4
> tx_clean: 43
> tx_set_ic_bit: 4
> irq_receive_pmt_irq_n: 0
> mmc_tx_irq_n: 0
> mmc_rx_irq_n: 0
> mmc_rx_csum_offload_irq_n: 0
> irq_tx_path_in_lpi_mode_n: 0
> irq_tx_path_exit_lpi_mode_n: 0
> irq_rx_path_in_lpi_mode_n: 0
> irq_rx_path_exit_lpi_mode_n: 0
> phy_eee_wakeup_error_n: 0
> ip_hdr_err: 0
> ip_payload_err: 0
> ip_csum_bypassed: 0
> ipv4_pkt_rcvd: 0
> ipv6_pkt_rcvd: 0
> no_ptp_rx_msg_type_ext: 0
> ptp_rx_msg_type_sync: 0
> ptp_rx_msg_type_follow_up: 0
> ptp_rx_msg_type_delay_req: 0
> ptp_rx_msg_type_delay_resp: 0
> ptp_rx_msg_type_pdelay_req: 0
> ptp_rx_msg_type_pdelay_resp: 0
> ptp_rx_msg_type_pdelay_follow_up: 0
> ptp_rx_msg_type_announce: 0
> ptp_rx_msg_type_management: 0
> ptp_rx_msg_pkt_reserved_type: 0
> ptp_frame_type: 0
> ptp_ver: 0
> timestamp_dropped: 0
> av_pkt_rcvd: 0
> av_tagged_pkt_rcvd: 0
> vlan_tag_priority_val: 0
> l3_filter_match: 0
> l4_filter_match: 0
> l3_l4_filter_no_match: 0
> irq_pcs_ane_n: 0
> irq_pcs_link_n: 0
> irq_rgmii_n: 0
> mtl_tx_status_fifo_full: 0
> mtl_tx_fifo_not_empty: 0
> mmtl_fifo_ctrl: 0
> mtl_tx_fifo_read_ctrl_write: 0
> mtl_tx_fifo_read_ctrl_wait: 0
> mtl_tx_fifo_read_ctrl_read: 0
> mtl_tx_fifo_read_ctrl_idle: 0
> mac_tx_in_pause: 0
> mac_tx_frame_ctrl_xfer: 0
> mac_tx_frame_ctrl_idle: 0
> mac_tx_frame_ctrl_wait: 0
> mac_tx_frame_ctrl_pause: 0
> mac_gmii_tx_proto_engine: 0
> mtl_rx_fifo_fill_level_full: 0
> mtl_rx_fifo_fill_above_thresh: 0
> mtl_rx_fifo_fill_below_thresh: 0
> mtl_rx_fifo_fill_level_empty: 0
> mtl_rx_fifo_read_ctrl_flush: 0
> mtl_rx_fifo_read_ctrl_read_data: 0
> mtl_rx_fifo_read_ctrl_status: 0
> mtl_rx_fifo_read_ctrl_idle: 0
> mtl_rx_fifo_ctrl_active: 0
> mac_rx_frame_ctrl_fifo: 0
> mac_gmii_rx_proto_engine: 0
> tx_tso_frames: 0
> tx_tso_nfrags: 0
> # ethtool eth0
> Settings for eth0:
> Supported ports: [ TP MII ]
> Supported link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Supported pause frame use: Symmetric Receive-only
> Supports auto-negotiation: Yes
> Supported FEC modes: Not reported
> Advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Advertised pause frame use: No
> Advertised auto-negotiation: Yes
> Advertised FEC modes: Not reported
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: Symmetric Receive-only
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: Not reported
> Speed: 1000Mb/s
> Duplex: Full
> Port: MII
> PHYAD: 0
> Transceiver: internal
> Auto-negotiation: on
> Supports Wake-on: ug
> Wake-on: d
> Current message level: 0x0000003f (63)
> drv probe link timer ifdown ifup
> Link detected: yes
> # ifconfig eth0
> eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
> inet 192.168.1.116 netmask 255.255.255.0 broadcast 192.168.1.255
> ether 66:d8:d0:34:f1:cb txqueuelen 1000 (Ethernet)
> RX packets 4420 bytes 292864 (286.0 KiB)
> RX errors 0 dropped 0 overruns 0 frame 0
> TX packets 19165 bytes 28975001 (27.6 MiB)
> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
> device interrupt 21
>
> # iperf3 -c 192.168.1.100
> Connecting to host 192.168.1.100, port 5201
> [ 5] local 192.168.1.116 port 50608 connected to 192.168.1.100 port 5201
> [ ID] Interval Transfer Bitrate Retr Cwnd
> [ 5] 0.00-1.00 sec 111 MBytes 932 Mbits/sec 1 338 KBytes
> [ 5] 1.00-2.00 sec 111 MBytes 930 Mbits/sec 0 351 KBytes
> [ 5] 2.00-3.00 sec 112 MBytes 938 Mbits/sec 0 375 KBytes
> [ 5] 3.00-4.00 sec 112 MBytes 943 Mbits/sec 0 390 KBytes
> [ 5] 4.00-5.00 sec 50.3 MBytes 421 Mbits/sec 1 1.41 KBytes
> [ 5] 5.00-6.00 sec 0.00 Bytes 0.00 bits/sec 1 1.41 KBytes
> [ 5] 6.00-7.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
> [ 5] 7.00-8.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
> [ 5] 8.00-9.00 sec 0.00 Bytes 0.00 bits/sec 0 1.41 KBytes
> [ 5] 10.00-29.11 sec 0.00 Bytes 0.00 bits/sec 2 1.41 KBytes
> <CTRL+C after ~30 seconds>
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate Retr
> [ 5] 0.00-29.11 sec 496 MBytes 143 Mbits/sec 5 sender
> [ 5] 0.00-29.11 sec 0.00 Bytes 0.00 bits/sec receiver
> iperf3: interrupt - the client has terminated
>
> # ethtool -S eth0
> NIC statistics:
> mmc_tx_octetcount_gb: 0
> mmc_tx_framecount_gb: 0
> mmc_tx_broadcastframe_g: 0
> mmc_tx_multicastframe_g: 0
> mmc_tx_64_octets_gb: 0
> mmc_tx_65_to_127_octets_gb: 0
> mmc_tx_128_to_255_octets_gb: 0
> mmc_tx_256_to_511_octets_gb: 0
> mmc_tx_512_to_1023_octets_gb: 0
> mmc_tx_1024_to_max_octets_gb: 0
> mmc_tx_unicast_gb: 0
> mmc_tx_multicast_gb: 0
> mmc_tx_broadcast_gb: 0
> mmc_tx_underflow_error: 0
> mmc_tx_singlecol_g: 0
> mmc_tx_multicol_g: 0
> mmc_tx_deferred: 0
> mmc_tx_latecol: 0
> mmc_tx_exesscol: 0
> mmc_tx_carrier_error: 0
> mmc_tx_octetcount_g: 0
> mmc_tx_framecount_g: 0
> mmc_tx_excessdef: 0
> mmc_tx_pause_frame: 0
> mmc_tx_vlan_frame_g: 0
> mmc_rx_framecount_gb: 16766
> mmc_rx_octetcount_gb: 1173986
> mmc_rx_octetcount_g: 1173986
> mmc_rx_broadcastframe_g: 98
> mmc_rx_multicastframe_g: 0
> mmc_rx_crc_error: 0
> mmc_rx_align_error: 0
> mmc_rx_run_error: 0
> mmc_rx_jabber_error: 0
> mmc_rx_undersize_g: 0
> mmc_rx_oversize_g: 0
> mmc_rx_64_octets_gb: 96
> mmc_rx_65_to_127_octets_gb: 16669
> mmc_rx_128_to_255_octets_gb: 0
> mmc_rx_256_to_511_octets_gb: 1
> mmc_rx_512_to_1023_octets_gb: 0
> mmc_rx_1024_to_max_octets_gb: 0
> mmc_rx_unicast_g: 16668
> mmc_rx_length_error: 0
> mmc_rx_autofrangetype: 0
> mmc_rx_pause_frames: 0
> mmc_rx_fifo_overflow: 0
> mmc_rx_vlan_frames_gb: 0
> mmc_rx_watchdog_error: 0
> mmc_rx_ipc_intr_mask: 2147385342
> mmc_rx_ipc_intr: 0
> mmc_rx_ipv4_gd: 16671
> mmc_rx_ipv4_hderr: 0
> mmc_rx_ipv4_nopay: 0
> mmc_rx_ipv4_frag: 0
> mmc_rx_ipv4_udsbl: 0
> mmc_rx_ipv4_gd_octets: 867822
> mmc_rx_ipv4_hderr_octets: 0
> mmc_rx_ipv4_nopay_octets: 0
> mmc_rx_ipv4_frag_octets: 0
> mmc_rx_ipv4_udsbl_octets: 0
> mmc_rx_ipv6_gd_octets: 0
> mmc_rx_ipv6_hderr_octets: 0
> mmc_rx_ipv6_nopay_octets: 0
> mmc_rx_ipv6_gd: 0
> mmc_rx_ipv6_hderr: 0
> mmc_rx_ipv6_nopay: 0
> mmc_rx_udp_gd: 28
> mmc_rx_udp_err: 0
> mmc_rx_tcp_gd: 16643
> mmc_rx_tcp_err: 0
> mmc_rx_icmp_gd: 0
> mmc_rx_icmp_err: 0
> mmc_rx_udp_gd_octets: 1068
> mmc_rx_udp_err_octets: 0
> mmc_rx_tcp_gd_octets: 533334
> mmc_rx_tcp_err_octets: 0
> mmc_rx_icmp_gd_octets: 0
> mmc_rx_icmp_err_octets: 0
> tx_underflow: 0
> tx_carrier: 0
> tx_losscarrier: 0
> vlan_tag: 0
> tx_deferred: 0
> tx_vlan: 0
> tx_jabber: 0
> tx_frame_flushed: 0
> tx_payload_error: 0
> tx_ip_header_error: 0
> rx_desc: 0
> sa_filter_fail: 0
> overflow_error: 0
> ipc_csum_error: 0
> rx_collision: 0
> rx_crc_errors: 0
> dribbling_bit: 0
> rx_length: 0
> rx_mii: 0
> rx_multicast: 0
> rx_gmac_overflow: 0
> rx_watchdog: 0
> da_rx_filter_fail: 0
> sa_rx_filter_fail: 0
> rx_missed_cntr: 0
> rx_overflow_cntr: 0
> rx_vlan: 0
> tx_undeflow_irq: 0
> tx_process_stopped_irq: 0
> tx_jabber_irq: 0
> rx_overflow_irq: 0
> rx_buf_unav_irq: 0
> rx_process_stopped_irq: 0
> rx_watchdog_irq: 0
> tx_early_irq: 0
> fatal_bus_error_irq: 0
> rx_early_irq: 11717
> threshold: 1
> tx_pkt_n: 359107
> rx_pkt_n: 16660
> normal_irq_n: 108987
> rx_normal_irq_n: 7449
> napi_poll: 107154
> tx_normal_irq_n: 105918
> tx_clean: 107179
> tx_set_ic_bit: 179554
> irq_receive_pmt_irq_n: 0
> mmc_tx_irq_n: 0
> mmc_rx_irq_n: 0
> mmc_rx_csum_offload_irq_n: 0
> irq_tx_path_in_lpi_mode_n: 0
> irq_tx_path_exit_lpi_mode_n: 0
> irq_rx_path_in_lpi_mode_n: 0
> irq_rx_path_exit_lpi_mode_n: 0
> phy_eee_wakeup_error_n: 0
> ip_hdr_err: 0
> ip_payload_err: 0
> ip_csum_bypassed: 0
> ipv4_pkt_rcvd: 0
> ipv6_pkt_rcvd: 0
> no_ptp_rx_msg_type_ext: 0
> ptp_rx_msg_type_sync: 0
> ptp_rx_msg_type_follow_up: 0
> ptp_rx_msg_type_delay_req: 0
> ptp_rx_msg_type_delay_resp: 0
> ptp_rx_msg_type_pdelay_req: 0
> ptp_rx_msg_type_pdelay_resp: 0
> ptp_rx_msg_type_pdelay_follow_up: 0
> ptp_rx_msg_type_announce: 0
> ptp_rx_msg_type_management: 0
> ptp_rx_msg_pkt_reserved_type: 0
> ptp_frame_type: 0
> ptp_ver: 0
> timestamp_dropped: 0
> av_pkt_rcvd: 0
> av_tagged_pkt_rcvd: 0
> vlan_tag_priority_val: 0
> l3_filter_match: 0
> l4_filter_match: 0
> l3_l4_filter_no_match: 0
> irq_pcs_ane_n: 0
> irq_pcs_link_n: 0
> irq_rgmii_n: 0
> mtl_tx_status_fifo_full: 0
> mtl_tx_fifo_not_empty: 0
> mmtl_fifo_ctrl: 0
> mtl_tx_fifo_read_ctrl_write: 0
> mtl_tx_fifo_read_ctrl_wait: 0
> mtl_tx_fifo_read_ctrl_read: 0
> mtl_tx_fifo_read_ctrl_idle: 0
> mac_tx_in_pause: 0
> mac_tx_frame_ctrl_xfer: 0
> mac_tx_frame_ctrl_idle: 0
> mac_tx_frame_ctrl_wait: 0
> mac_tx_frame_ctrl_pause: 0
> mac_gmii_tx_proto_engine: 0
> mtl_rx_fifo_fill_level_full: 0
> mtl_rx_fifo_fill_above_thresh: 0
> mtl_rx_fifo_fill_below_thresh: 0
> mtl_rx_fifo_fill_level_empty: 0
> mtl_rx_fifo_read_ctrl_flush: 0
> mtl_rx_fifo_read_ctrl_read_data: 0
> mtl_rx_fifo_read_ctrl_status: 0
> mtl_rx_fifo_read_ctrl_idle: 0
> mtl_rx_fifo_ctrl_active: 0
> mac_rx_frame_ctrl_fifo: 0
> mac_gmii_rx_proto_engine: 0
> tx_tso_frames: 0
> tx_tso_nfrags: 0
> # ethtool eth0
> Settings for eth0:
> Supported ports: [ TP MII ]
> Supported link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Supported pause frame use: Symmetric Receive-only
> Supports auto-negotiation: Yes
> Supported FEC modes: Not reported
> Advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Advertised pause frame use: No
> Advertised auto-negotiation: Yes
> Advertised FEC modes: Not reported
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: Symmetric Receive-only
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: Not reported
> Speed: 1000Mb/s
> Duplex: Full
> Port: MII
> PHYAD: 0
> Transceiver: internal
> Auto-negotiation: on
> Supports Wake-on: ug
> Wake-on: d
> Current message level: 0x0000003f (63)
> drv probe link timer ifdown ifup
> Link detected: yes
> # ifconfig eth0
> eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
> inet 192.168.1.116 netmask 255.255.255.0 broadcast 192.168.1.255
> ether 66:d8:d0:34:f1:cb txqueuelen 1000 (Ethernet)
> RX packets 21021 bytes 1389130 (1.3 MiB)
> RX errors 0 dropped 0 overruns 0 frame 0
> TX packets 378272 bytes 572598814 (546.0 MiB)
> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
> device interrupt 21
>
>
> Regards
> Martin
^ permalink raw reply
* Re: [PATCH 00/15] soc: octeontx2: Add RVU admin function driver
From: Sunil Kovvuri @ 2018-08-30 9:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: LKML, olof, LAKML, linux-soc, Sunil Goutham, Linux Netdev List,
David S. Miller
In-Reply-To: <CA+sq2Cd3R0V5hcaF+ESfkOEBHgBwOB6b8Uwx7K+CeUN6gLFTOg@mail.gmail.com>
On Tue, Aug 28, 2018 at 6:54 PM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
>
> On Tue, Aug 28, 2018 at 5:53 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Aug 28, 2018 at 12:57 PM <sunil.kovvuri@gmail.com> wrote:
> > >
> > > From: Sunil Goutham <sgoutham@marvell.com>
> > >
> > > Resource virtualization unit (RVU) on Marvell's OcteonTX2 SOC supports
> > > multiple PCIe SRIOV physical functions (PFs) and virtual functions (VFs).
> > > PF0 is called administrative / admin function (AF) and has privilege access
> > > to registers to provision different RVU functional blocks to each of
> > > PF/VF.
> > >
> > > This admin function (AF) driver acts as a configuration / administrative
> > > software which provisions functional blocks to a PF/VF on demand for them
> > > to work as one of the following
> > > - A basic network controller (i.e NIC).
> > > - NIC with packet filtering, shaping and scheduling capabilities.
> > > - A crypto device.
> > > - A combination of above etc.
> > >
> > > PF/VFs communicate with admin function via a shared memory region.
> > > This patch series adds logic for the following
> > > - RVU AF driver with functional blocks provisioning support
> > > - Mailbox infrastructure for communication between AF and PFs.
> > > - CGX driver which provides information about physcial network
> > > interfaces which AF processes and forwards required info to
> > > PF/VF drivers.
> > >
> > > This is the first set of patches out of 70 odd patches.
> > >
> > > Note: This driver neither receives any data nor processes it i.e no I/O,
> > > just does the hardware configuration.
> >
> > Hi Sunil,
> >
> > Thanks for posting this first series, I'm glad we're seeing support for this
> > chip family making some progress.
>
> Thanks.
>
> >
> > My feeling overall is that we need a review from the network driver
> > folks more than the arm-soc team etc, and that maybe the driver
> > as a whole should go into drivers/net/ethernet.
>
> This driver doesn't handle any network IO and moreever this driver has to handle
> configuration requests from crypto driver as well. There will be
> separate network and
> crypto drivers which will be upstreamed into drivers/net/ethernet and
> drivers/crypto.
> And in future silicons there will be different types of functional
> blocks which will be
> added into this resource virtualization unit (RVU). Hence i thought
> this driver is not a
> right fit in drivers/net/ethernet.
>
> >
> > We support some couple of similar hardware already that has
> > both support for virtual functions and for crypto offload, including
> > the Chelsio cxgb4, Mellanox mlx5, NXP DPAA and probably others,
>
> I agree, but i guess that is done to support inline crypto.
> Here this driver has to handle requests from normal crypto driver
> (drivers/crypto) as well.
>
> > and we need to ensure that the exposed interfaces are all
> > compatible, and that you use the correct subsystems and
> > in-kernel abstractions for thing that are common.
> >
> > Arnd
Hi Arnd,
I hope i have answered your queries.
Let us know if you have any more queries or feedback w.r.t to a
individual patch or the patchset on the whole.
Thanks,
Sunil.
^ permalink raw reply
* Re: [PATCH 0/3] IB/ipoib: Use dev_port to disambiguate port numbers
From: Leon Romanovsky @ 2018-08-30 5:43 UTC (permalink / raw)
To: Arseny Maslennikov; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, netdev
In-Reply-To: <20180828210117.6437-1-ar@cs.msu.ru>
[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]
On Wed, Aug 29, 2018 at 12:01:14AM +0300, Arseny Maslennikov wrote:
> Pre-3.15 userspace had trouble distinguishing different ports
> of a NIC on a single PCI bus/device/function. To solve this,
> a sysfs field `dev_port' was introduced quite a while ago
> (commit v3.14-rc3-739-g3f85944fe207), and some relevant device
> drivers were fixed to use it, but not in case of IPoIB.
>
> The convention for some reason never got documented in the kernel, but
> was immediately adopted by userspace (notably udev[1][2], biosdevname[3])
>
> 3/3 documents the sysfs field — that's why I'm CC-ing netdev.
>
> This series was tested on and applies to 4.19-rc1.
>
> [1] https://lists.freedesktop.org/archives/systemd-devel/2014-June/020788.html
> [2] https://lists.freedesktop.org/archives/systemd-devel/2014-July/020804.html
> [3] https://github.com/CloudAutomationNTools/biosdevname/blob/c795d51dd93a5309652f0d635f12a3ecfabfaa72/src/eths.c#L38
>
> Arseny Maslennikov (3):
> IB/ipoib: Use dev_port to expose network interface port numbers
> IB/ipoib: Stop using dev_id to expose port numbers
I completely agree with previous Yuval's comment, it makes no sense to
start separate commits for every line.
Please decide what is best and right behavior and do it, instead of
pushing it up to be the maintainer's problem.
Thanks
> Documentation/ABI: document /sys/class/net/*/dev_port
>
> Documentation/ABI/testing/sysfs-class-net | 10 ++++++++++
> drivers/infiniband/ulp/ipoib/ipoib_main.c | 2 +-
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> --
> 2.18.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: KASAN: stack-out-of-bounds Read in __schedule
From: Daniel Borkmann @ 2018-08-30 9:52 UTC (permalink / raw)
To: Dmitry Vyukov, Alexander Potapenko, Alexei Starovoitov, netdev
Cc: Jan Kara, syzbot+45a34334c61a8ecf661d, Jan Kara, linux-ext4, LKML,
syzkaller-bugs, Theodore Ts'o
In-Reply-To: <CACT4Y+a2bwVvDRfL=wg=5bnOv6S7RrJYoh4ygaizqVm3JpNu1A@mail.gmail.com>
On 08/30/2018 06:11 AM, Dmitry Vyukov wrote:
> On Wed, Aug 29, 2018 at 7:03 AM, 'Alexander Potapenko' via
> syzkaller-bugs <syzkaller-bugs@googlegroups.com> wrote:
>> On Wed, Aug 29, 2018 at 3:46 PM Jan Kara <jack@suse.cz> wrote:
>>> On Tue 28-08-18 08:30:02, syzbot wrote:
>>>> Hello,
>>>>
>>>> syzbot found the following crash on:
>>>>
>>>> HEAD commit: 5b394b2ddf03 Linux 4.19-rc1
>>>> git tree: upstream
>>>> console output: https://syzkaller.appspot.com/x/log.txt?x=14f4d8e1400000
>>>> kernel config: https://syzkaller.appspot.com/x/.config?x=49927b422dcf0b29
>>>> dashboard link: https://syzkaller.appspot.com/bug?extid=45a34334c61a8ecf661d
>>>> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>>>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13127e5a400000
>>>>
>>>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>>>> Reported-by: syzbot+45a34334c61a8ecf661d@syzkaller.appspotmail.com
>>>>
>>>> IPv6: ADDRCONF(NETDEV_UP): veth1: link is not ready
>>>> IPv6: ADDRCONF(NETDEV_CHANGE): veth1: link becomes ready
>>>> IPv6: ADDRCONF(NETDEV_CHANGE): veth0: link becomes ready
>>>> 8021q: adding VLAN 0 to HW filter on device team0
>>>> ==================================================================
>>>> BUG: KASAN: stack-out-of-bounds in schedule_debug kernel/sched/core.c:3285
>>>> [inline]
>>>> BUG: KASAN: stack-out-of-bounds in __schedule+0x1977/0x1df0
>>>> kernel/sched/core.c:3395
>>>> Read of size 8 at addr ffff8801ad090000 by task syz-executor0/4718
>>>
>>> Weird, can you please help me decipher this? So here KASAN complains about
>>> wrong memory access in the scheduler.
>
> This looks like a result of a previous bad silent memory corruption.
>
> The KASAN report says there is a stack out-of-bounds in scheduler. And
> that if followed by slab corruption report in another task.
>
> fs/jbd2/transaction.c happens to be the first meaningful file in this
> crash, and so that's where it is attributed to.
>
> Rerunning the reproducer several times can maybe give some better
> glues, or maybe not, maybe they all will look equally puzzling.
>
> This part of the repro looks familiar:
>
> r1 = bpf$MAP_CREATE(0x0, &(0x7f0000002e40)={0x12, 0x0, 0x4, 0x6e, 0x0,
> 0x1}, 0x68)
> bpf$MAP_UPDATE_ELEM(0x2, &(0x7f0000000180)={r1, &(0x7f0000000000),
> &(0x7f0000000140)}, 0x20)
>
> We had exactly such consequences of a bug in bpf map very recently,
> but that was claimed to be fixed. Maybe not completely?
> +bpf maintainers
Looks like syzbot found this in Linus tree with HEAD commit 5b394b2ddf03 ("Linux 4.19-rc1")
one day later net PR got merged via 050cdc6c9501 ("Merge git://git.kernel.org/pub/...").
This PR contained a couple of fixes I did on sockmap code during audit such as:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b845c898b2f1ea458d5453f0fa1da6e2dfce3bb4
Looking at the reproducer syzkaller found it contains:
r1 = bpf$MAP_CREATE(0x0, &(0x7f0000002e40)={0x12, 0x0, 0x4, 0x6e, 0x0, 0x1}, 0x68)
^^^
So it found the crash with map type of sock hash and key size of 0x0 (which is invalid),
where subsequent map update triggered the corruption. I just did a 'syz test' and it
wasn't able to trigger the crash anymore.
#syz fix: bpf, sockmap: fix sock_hash_alloc and reject zero-sized keys
^ permalink raw reply
* [PATCH] net/rds: RDS is not Radio Data System
From: Pavel Machek @ 2018-08-30 10:23 UTC (permalink / raw)
To: Trivial patch monkey, Netdev list, sowmini.varadhan,
santosh.shilimkar, ka-cheong.poon, linux-kernel
Cc: David S. Miller
[-- Attachment #1: Type: text/plain, Size: 816 bytes --]
Getting prompt "The RDS Protocol" (RDS) is not too helpful, and it is
easily confused with Radio Data System (which we may want to support
in kernel, too).
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
diff --git a/net/rds/Kconfig b/net/rds/Kconfig
index 41f7556..2738f14 100644
--- a/net/rds/Kconfig
+++ b/net/rds/Kconfig
@@ -1,6 +1,6 @@
config RDS
- tristate "The RDS Protocol"
+ tristate "The (Reliable Datagram Sockets) Protocol"
depends on INET
---help---
The RDS (Reliable Datagram Sockets) protocol provides reliable,
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply related
* Answer me.
From: Mr.David Keller @ 2018-08-30 6:26 UTC (permalink / raw)
My Greeting,Did you receive the letter i sent to you. Please answer me.
Best Regard,
Mr. David Keller.
^ permalink raw reply
* [PATCH] can: ucan: remove redundant pointer 'udev'
From: Colin King @ 2018-08-30 10:27 UTC (permalink / raw)
To: Wolfgang Grandegger, Marc Kleine-Budde, David S . Miller,
linux-can, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Pointer 'udev' is being assigned but is never used hence it is
redundant and can be removed.
Cleans up clang warning:
variable 'udev' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/can/usb/ucan.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
index 0678a38b1af4..c9fd83e8d947 100644
--- a/drivers/net/can/usb/ucan.c
+++ b/drivers/net/can/usb/ucan.c
@@ -1575,11 +1575,8 @@ static int ucan_probe(struct usb_interface *intf,
/* disconnect the device */
static void ucan_disconnect(struct usb_interface *intf)
{
- struct usb_device *udev;
struct ucan_priv *up = usb_get_intfdata(intf);
- udev = interface_to_usbdev(intf);
-
usb_set_intfdata(intf, NULL);
if (up) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] net/rds: RDS is not Radio Data System
From: Sowmini Varadhan @ 2018-08-30 10:30 UTC (permalink / raw)
To: Pavel Machek
Cc: Trivial patch monkey, Netdev list, santosh.shilimkar,
ka-cheong.poon, linux-kernel, David S. Miller
In-Reply-To: <20180830102336.GA21063@amd>
On (08/30/18 12:23), Pavel Machek wrote:
> config RDS
> - tristate "The RDS Protocol"
> + tristate "The (Reliable Datagram Sockets) Protocol"
Can we drop the extra parentheses (i.e., the ())?
^ permalink raw reply
* [PATCH] 9p/rdma: do not disconnect on down_interruptible EAGAIN
From: Dominique Martinet @ 2018-08-30 10:35 UTC (permalink / raw)
To: Eric Van Hensbergen, Latchesar Ionkov
Cc: Dominique Martinet, v9fs-developer, netdev, linux-kernel,
linux-rdma
From: Dominique Martinet <dominique.martinet@cea.fr>
9p/rdma would sometimes drop the connection and display errors in
recv_done when the user does ^C.
The errors were IB_WC_WR_FLUSH_ERR caused by recv buffers that were
posted at the time of disconnect, and we just do not want to disconnect
when down_interruptible is... interrupted.
As a bonus, re-evaluate the log importance of post_recv() failing: we
want to see that one if it ever happens.
Signed-off-by: Dominique Martinet <dominique.martinet@cea.fr>
---
this had actually been bugging me forever, but I couldn't test the refcount
stuff properly around flush with that so I had to finally figure it out..
net/9p/trans_rdma.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 9cc9b3a19ee7..9719bc4d9424 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -477,7 +477,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
err = post_recv(client, rpl_context);
if (err) {
- p9_debug(P9_DEBUG_FCALL, "POST RECV failed\n");
+ p9_debug(P9_DEBUG_ERROR, "POST RECV failed: %d\n", err);
goto recv_error;
}
/* remove posted receive buffer from request structure */
@@ -546,7 +546,7 @@ static int rdma_request(struct p9_client *client, struct p9_req_t *req)
recv_error:
kfree(rpl_context);
spin_lock_irqsave(&rdma->req_lock, flags);
- if (rdma->state < P9_RDMA_CLOSING) {
+ if (err != -EINTR && rdma->state < P9_RDMA_CLOSING) {
rdma->state = P9_RDMA_CLOSING;
spin_unlock_irqrestore(&rdma->req_lock, flags);
rdma_disconnect(rdma->cm_id);
--
2.17.1
^ permalink raw reply related
* [PATCH v4] 9p: Add refcount to p9_req_t
From: Dominique Martinet @ 2018-08-30 10:52 UTC (permalink / raw)
To: Tomas Bortoli, Eric Van Hensbergen, Latchesar Ionkov
Cc: v9fs-developer, netdev, linux-kernel, syzkaller,
Dominique Martinet
In-Reply-To: <1535518779-28551-1-git-send-email-asmadeus@codewreck.org>
From: Tomas Bortoli <tomasbortoli@gmail.com>
To avoid use-after-free(s), use a refcount to keep track of the
usable references to any instantiated struct p9_req_t.
This commit adds p9_req_put(), p9_req_get() and p9_req_try_get() as
wrappers to kref_put(), kref_get() and kref_get_unless_zero().
These are used by the client and the transports to keep track of
valid requests' references.
p9_free_req() is added back and used as callback by kref_put().
Add SLAB_TYPESAFE_BY_RCU as it ensures that the memory freed by
kmem_cache_free() will not be reused for another type until the rcu
synchronisation period is over, so an address gotten under rcu read
lock is safe to inc_ref() without corrupting random memory while
the lock is held.
Co-developed-by: Dominique Martinet <dominique.martinet@cea.fr>
Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com
Signed-off-by: Dominique Martinet <dominique.martinet@cea.fr>
---
v3:
- add req put if virtio zc request fails
- add req put if cancelled callback is not defined for virtio
- (incorrectly) add req put in rdma cancelled callback
v4:
- removed rdma's cancelled callback put again
- changed the else if no cancelled callback into actually giving virtio
a callback, xen does not need to call put in that case either because
both function rely on tag_lookup to find the request. trans_fd only
needs to put in cancelled because it also keeps the req in a list around
for cancel.
- add req put for trans xen's request(), I'm not sure why that one was
missing either..
And with that I believe I am done testing all four transports.
I'll do a second round of tests next week just to make sure, but it
should be good enough™
Sorry for the multiple iterations.
include/net/9p/client.h | 14 ++++++++++
net/9p/client.c | 57 ++++++++++++++++++++++++++++++++++++-----
net/9p/trans_fd.c | 11 +++++++-
net/9p/trans_rdma.c | 1 +
net/9p/trans_virtio.c | 26 ++++++++++++++++---
net/9p/trans_xen.c | 1 +
6 files changed, 98 insertions(+), 12 deletions(-)
diff --git a/include/net/9p/client.h b/include/net/9p/client.h
index 735f3979d559..947a570307a6 100644
--- a/include/net/9p/client.h
+++ b/include/net/9p/client.h
@@ -94,6 +94,7 @@ enum p9_req_status_t {
struct p9_req_t {
int status;
int t_err;
+ struct kref refcount;
wait_queue_head_t wq;
struct p9_fcall tc;
struct p9_fcall rc;
@@ -233,6 +234,19 @@ int p9_client_lock_dotl(struct p9_fid *fid, struct p9_flock *flock, u8 *status);
int p9_client_getlock_dotl(struct p9_fid *fid, struct p9_getlock *fl);
void p9_fcall_fini(struct p9_fcall *fc);
struct p9_req_t *p9_tag_lookup(struct p9_client *, u16);
+
+static inline void p9_req_get(struct p9_req_t *r)
+{
+ kref_get(&r->refcount);
+}
+
+static inline int p9_req_try_get(struct p9_req_t *r)
+{
+ return kref_get_unless_zero(&r->refcount);
+}
+
+int p9_req_put(struct p9_req_t *r);
+
void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status);
int p9_parse_header(struct p9_fcall *, int32_t *, int8_t *, int16_t *, int);
diff --git a/net/9p/client.c b/net/9p/client.c
index 7942c0bfcc5b..aeeb6d8515d4 100644
--- a/net/9p/client.c
+++ b/net/9p/client.c
@@ -310,6 +310,18 @@ p9_tag_alloc(struct p9_client *c, int8_t type, unsigned int max_size)
if (tag < 0)
goto free;
+ /* Init ref to two because in the general case there is one ref
+ * that is put asynchronously by a writer thread, one ref
+ * temporarily given by p9_tag_lookup and put by p9_client_cb
+ * in the recv thread, and one ref put by p9_tag_remove in the
+ * main thread. The only exception is virtio that does not use
+ * p9_tag_lookup but does not have a writer thread either
+ * (the write happens synchronously in the request/zc_request
+ * callback), so p9_client_cb eats the second ref there
+ * as the pointer is duplicated directly by virtqueue_add_sgs()
+ */
+ refcount_set(&req->refcount.refcount, 2);
+
return req;
free:
@@ -333,10 +345,21 @@ struct p9_req_t *p9_tag_lookup(struct p9_client *c, u16 tag)
struct p9_req_t *req;
rcu_read_lock();
+again:
req = idr_find(&c->reqs, tag);
- /* There's no refcount on the req; a malicious server could cause
- * us to dereference a NULL pointer
- */
+ if (req) {
+ /* We have to be careful with the req found under rcu_read_lock
+ * Thanks to SLAB_TYPESAFE_BY_RCU we can safely try to get the
+ * ref again without corrupting other data, then check again
+ * that the tag matches once we have the ref
+ */
+ if (!p9_req_try_get(req))
+ goto again;
+ if (req->tc.tag != tag) {
+ p9_req_put(req);
+ goto again;
+ }
+ }
rcu_read_unlock();
return req;
@@ -350,7 +373,7 @@ EXPORT_SYMBOL(p9_tag_lookup);
*
* Context: Any context.
*/
-static void p9_tag_remove(struct p9_client *c, struct p9_req_t *r)
+static int p9_tag_remove(struct p9_client *c, struct p9_req_t *r)
{
unsigned long flags;
u16 tag = r->tc.tag;
@@ -359,11 +382,23 @@ static void p9_tag_remove(struct p9_client *c, struct p9_req_t *r)
spin_lock_irqsave(&c->lock, flags);
idr_remove(&c->reqs, tag);
spin_unlock_irqrestore(&c->lock, flags);
+ return p9_req_put(r);
+}
+
+static void p9_req_free(struct kref *ref)
+{
+ struct p9_req_t *r = container_of(ref, struct p9_req_t, refcount);
p9_fcall_fini(&r->tc);
p9_fcall_fini(&r->rc);
kmem_cache_free(p9_req_cache, r);
}
+int p9_req_put(struct p9_req_t *r)
+{
+ return kref_put(&r->refcount, p9_req_free);
+}
+EXPORT_SYMBOL(p9_req_put);
+
/**
* p9_tag_cleanup - cleans up tags structure and reclaims resources
* @c: v9fs client struct
@@ -379,7 +414,9 @@ static void p9_tag_cleanup(struct p9_client *c)
rcu_read_lock();
idr_for_each_entry(&c->reqs, req, id) {
pr_info("Tag %d still in use\n", id);
- p9_tag_remove(c, req);
+ if (p9_tag_remove(c, req) == 0)
+ pr_warn("Packet with tag %d has still references",
+ req->tc.tag);
}
rcu_read_unlock();
}
@@ -403,6 +440,7 @@ void p9_client_cb(struct p9_client *c, struct p9_req_t *req, int status)
wake_up(&req->wq);
p9_debug(P9_DEBUG_MUX, "wakeup: %d\n", req->tc.tag);
+ p9_req_put(req);
}
EXPORT_SYMBOL(p9_client_cb);
@@ -643,9 +681,10 @@ static int p9_client_flush(struct p9_client *c, struct p9_req_t *oldreq)
* if we haven't received a response for oldreq,
* remove it from the list
*/
- if (oldreq->status == REQ_STATUS_SENT)
+ if (oldreq->status == REQ_STATUS_SENT) {
if (c->trans_mod->cancelled)
c->trans_mod->cancelled(c, oldreq);
+ }
p9_tag_remove(c, req);
return 0;
@@ -682,6 +721,8 @@ static struct p9_req_t *p9_client_prepare_req(struct p9_client *c,
return req;
reterr:
p9_tag_remove(c, req);
+ /* We have to put also the 2nd reference as it won't be used */
+ p9_req_put(req);
return ERR_PTR(err);
}
@@ -716,6 +757,8 @@ p9_client_rpc(struct p9_client *c, int8_t type, const char *fmt, ...)
err = c->trans_mod->request(c, req);
if (err < 0) {
+ /* write won't happen */
+ p9_req_put(req);
if (err != -ERESTARTSYS && err != -EFAULT)
c->status = Disconnected;
goto recalc_sigpending;
@@ -2241,7 +2284,7 @@ EXPORT_SYMBOL(p9_client_readlink);
int __init p9_client_init(void)
{
- p9_req_cache = KMEM_CACHE(p9_req_t, 0);
+ p9_req_cache = KMEM_CACHE(p9_req_t, SLAB_TYPESAFE_BY_RCU);
return p9_req_cache ? 0 : -ENOMEM;
}
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 20f46f13fe83..686e24e355d0 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -132,6 +132,7 @@ struct p9_conn {
struct list_head req_list;
struct list_head unsent_req_list;
struct p9_req_t *req;
+ struct p9_req_t *wreq;
char tmp_buf[7];
struct p9_fcall rc;
int wpos;
@@ -383,6 +384,7 @@ static void p9_read_work(struct work_struct *work)
m->rc.sdata = NULL;
m->rc.offset = 0;
m->rc.capacity = 0;
+ p9_req_put(m->req);
m->req = NULL;
}
@@ -472,6 +474,8 @@ static void p9_write_work(struct work_struct *work)
m->wbuf = req->tc.sdata;
m->wsize = req->tc.size;
m->wpos = 0;
+ p9_req_get(req);
+ m->wreq = req;
spin_unlock(&m->client->lock);
}
@@ -492,8 +496,11 @@ static void p9_write_work(struct work_struct *work)
}
m->wpos += err;
- if (m->wpos == m->wsize)
+ if (m->wpos == m->wsize) {
m->wpos = m->wsize = 0;
+ p9_req_put(m->wreq);
+ m->wreq = NULL;
+ }
end_clear:
clear_bit(Wworksched, &m->wsched);
@@ -694,6 +701,7 @@ static int p9_fd_cancel(struct p9_client *client, struct p9_req_t *req)
if (req->status == REQ_STATUS_UNSENT) {
list_del(&req->req_list);
req->status = REQ_STATUS_FLSHD;
+ p9_req_put(req);
ret = 0;
}
spin_unlock(&client->lock);
@@ -711,6 +719,7 @@ static int p9_fd_cancelled(struct p9_client *client, struct p9_req_t *req)
spin_lock(&client->lock);
list_del(&req->req_list);
spin_unlock(&client->lock);
+ p9_req_put(req);
return 0;
}
diff --git a/net/9p/trans_rdma.c b/net/9p/trans_rdma.c
index 5b0cda1aaa7a..9cc9b3a19ee7 100644
--- a/net/9p/trans_rdma.c
+++ b/net/9p/trans_rdma.c
@@ -365,6 +365,7 @@ send_done(struct ib_cq *cq, struct ib_wc *wc)
c->busa, c->req->tc.size,
DMA_TO_DEVICE);
up(&rdma->sq_sem);
+ p9_req_put(c->req);
kfree(c);
}
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 3dd6ce1c0f2d..eb596c2ed546 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -207,6 +207,13 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
return 1;
}
+/* Reply won't come, so drop req ref */
+static int p9_virtio_cancelled(struct p9_client *client, struct p9_req_t *req)
+{
+ p9_req_put(req);
+ return 0;
+}
+
/**
* pack_sg_list_p - Just like pack_sg_list. Instead of taking a buffer,
* this takes a list of pages.
@@ -404,6 +411,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
struct scatterlist *sgs[4];
size_t offs;
int need_drop = 0;
+ int kicked = 0;
p9_debug(P9_DEBUG_TRANS, "virtio request\n");
@@ -411,8 +419,10 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
__le32 sz;
int n = p9_get_mapped_pages(chan, &out_pages, uodata,
outlen, &offs, &need_drop);
- if (n < 0)
- return n;
+ if (n < 0) {
+ err = n;
+ goto err_out;
+ }
out_nr_pages = DIV_ROUND_UP(n + offs, PAGE_SIZE);
if (n != outlen) {
__le32 v = cpu_to_le32(n);
@@ -428,8 +438,10 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
} else if (uidata) {
int n = p9_get_mapped_pages(chan, &in_pages, uidata,
inlen, &offs, &need_drop);
- if (n < 0)
- return n;
+ if (n < 0) {
+ err = n;
+ goto err_out;
+ }
in_nr_pages = DIV_ROUND_UP(n + offs, PAGE_SIZE);
if (n != inlen) {
__le32 v = cpu_to_le32(n);
@@ -498,6 +510,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
}
virtqueue_kick(chan->vq);
spin_unlock_irqrestore(&chan->lock, flags);
+ kicked = 1;
p9_debug(P9_DEBUG_TRANS, "virtio request kicked\n");
err = wait_event_killable(req->wq, req->status >= REQ_STATUS_RCVD);
/*
@@ -518,6 +531,10 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
}
kvfree(in_pages);
kvfree(out_pages);
+ if (!kicked) {
+ /* reply won't come */
+ p9_req_put(req);
+ }
return err;
}
@@ -750,6 +767,7 @@ static struct p9_trans_module p9_virtio_trans = {
.request = p9_virtio_request,
.zc_request = p9_virtio_zc_request,
.cancel = p9_virtio_cancel,
+ .cancelled = p9_virtio_cancelled,
/*
* We leave one entry for input and one entry for response
* headers. We also skip one more entry to accomodate, address
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 782a07f2ad0c..e2fbf3677b9b 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -185,6 +185,7 @@ static int p9_xen_request(struct p9_client *client, struct p9_req_t *p9_req)
ring->intf->out_prod = prod;
spin_unlock_irqrestore(&ring->lock, flags);
notify_remote_via_irq(ring->irq);
+ p9_req_put(p9_req);
return 0;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 1/5] pppoe: fix PPPOEIOCSFWD compat handling
From: Guillaume Nault @ 2018-08-30 11:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: paulus, linux-ppp, netdev, mitch, mostrows, jchapman, xeb, davem,
viro, y2038, linux-kernel
In-Reply-To: <20180829140409.833488-1-arnd@arndb.de>
On Wed, Aug 29, 2018 at 04:03:26PM +0200, Arnd Bergmann wrote:
> Support for handling the PPPOEIOCSFWD ioctl in compat mode was added in
> linux-2.5.69 along with hundreds of other commands, but was always broken
> sincen only the structure is compatible, but the command number is not,
> due to the size being sizeof(size_t), or at first sizeof(sizeof((struct
> sockaddr_pppox)), which is different on 64-bit architectures.
>
And the implementation was broken until 2016 (see 29e73269aa4d ("pppoe:
fix reference counting in PPPoE proxy")), and nobody ever noticed. I
should probably have removed this ioctl entirely instead of fixing it.
Clearly, it has never been used.
If you think it's worth fixing (as opposed to dropping this ioctl or
its compat mode), then,
Acked-by: Guillaume Nault <g.nault@alphalink.fr>
^ permalink raw reply
* Re: [PATCH net-next 3/5] ppp: move PPPIOCSCOMPRESS32 to ppp-generic.c
From: Guillaume Nault @ 2018-08-30 11:04 UTC (permalink / raw)
To: Arnd Bergmann
Cc: paulus, linux-ppp, netdev, mitch, mostrows, jchapman, xeb, davem,
viro, y2038, linux-kernel, Kirill Tkhai
In-Reply-To: <20180829140409.833488-3-arnd@arndb.de>
On Wed, Aug 29, 2018 at 04:03:28PM +0200, Arnd Bergmann wrote:
> PPPIOCSCOMPRESS is only implemented in ppp_generic, so it's best to move
> the compat handling there. My first approach was to keep it in a new
> ppp_compat_ioctl() function, but it turned out to be much simpler to do
> it in the regular ioctl handler, by allowing both structure layouts to
> be handled directly there.
>
Acked-by: Guillaume Nault <g.nault@alphalink.fr>
^ permalink raw reply
* Re: [PATCH net-next 4/5] ppp: move PPPIOCSPASS32/PPPIOCSACTIVE32 to ppp_generic.c
From: Guillaume Nault @ 2018-08-30 11:05 UTC (permalink / raw)
To: Arnd Bergmann
Cc: paulus, linux-ppp, netdev, mitch, mostrows, jchapman, xeb, davem,
viro, y2038, linux-kernel
In-Reply-To: <20180829140409.833488-4-arnd@arndb.de>
On Wed, Aug 29, 2018 at 04:03:29PM +0200, Arnd Bergmann wrote:
> PPPIOCSPASS and PPPIOCSACTIVE are implemented in ppp_generic and isdn_ppp,
> but the latter one doesn't work for compat mode in general, so we can
> move these two into the generic code.
>
> Again, the best implementation I could come up with was to merge
> the compat handling into the regular ppp_ioctl() function and
> treating all ioctl commands as compatible.
>
Acked-by: Guillaume Nault <g.nault@alphalink.fr>
^ permalink raw reply
* Re: [PATCH net-next 5/5] ppp: handle PPPIOCGIDLE for 64-bit time_t
From: Guillaume Nault @ 2018-08-30 11:06 UTC (permalink / raw)
To: Arnd Bergmann
Cc: paulus, linux-ppp, netdev, mitch, mostrows, jchapman, xeb, davem,
viro, y2038, linux-kernel, Karsten Keil, linux-doc
In-Reply-To: <20180829140409.833488-5-arnd@arndb.de>
On Wed, Aug 29, 2018 at 04:03:30PM +0200, Arnd Bergmann wrote:
> The ppp_idle structure is defined in terms of __kernel_time_t, which is
> defined as 'long' on all architectures, and this usage is not affected
> by the y2038 problem since it transports a time interval rather than an
> absolute time.
>
> However, the ppp user space defines the same structure as time_t, which
> may be 64-bit wide on new libc versions even on 32-bit architectures.
>
> It's easy enough to just handle both possible structure layouts on
> all architectures, to deal with the possibility that a user space ppp
> implementation comes with its own ppp_idle structure definition, as well
> as to document the fact that the driver is y2038-safe.
>
> Doing this also avoids the need for a special compat mode translation,
> since 32-bit and 64-bit kernels now support the same interfaces.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Documentation/networking/ppp_generic.txt | 2 ++
> drivers/isdn/i4l/isdn_ppp.c | 14 ++++++++---
> drivers/net/ppp/ppp_generic.c | 18 ++++++++++----
> fs/compat_ioctl.c | 31 ------------------------
> include/uapi/linux/ppp-ioctl.h | 2 ++
> include/uapi/linux/ppp_defs.h | 14 +++++++++++
> 6 files changed, 42 insertions(+), 39 deletions(-)
>
> diff --git a/Documentation/networking/ppp_generic.txt b/Documentation/networking/ppp_generic.txt
> index 61daf4b39600..fd563aff5fc9 100644
> --- a/Documentation/networking/ppp_generic.txt
> +++ b/Documentation/networking/ppp_generic.txt
> @@ -378,6 +378,8 @@ an interface unit are:
> CONFIG_PPP_FILTER option is enabled, the set of packets which reset
> the transmit and receive idle timers is restricted to those which
> pass the `active' packet filter.
> + Two versions of this command exist, to deal with user space
> + expecting times as either 32-bit or 64-bit time_t seconds.
>
> * PPPIOCSMAXCID sets the maximum connection-ID parameter (and thus the
> number of connection slots) for the TCP header compressor and
> diff --git a/drivers/isdn/i4l/isdn_ppp.c b/drivers/isdn/i4l/isdn_ppp.c
> index a7b275ea5de1..1f17126c5fa4 100644
> --- a/drivers/isdn/i4l/isdn_ppp.c
> +++ b/drivers/isdn/i4l/isdn_ppp.c
> @@ -543,11 +543,19 @@ isdn_ppp_ioctl(int min, struct file *file, unsigned int cmd, unsigned long arg)
> }
> is->pppcfg = val;
> break;
> - case PPPIOCGIDLE: /* get idle time information */
> + case PPPIOCGIDLE32: /* get idle time information */
> if (lp) {
> - struct ppp_idle pidle;
> + struct ppp_idle32 pidle;
> pidle.xmit_idle = pidle.recv_idle = lp->huptimer;
> - if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle))))
> + if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle32))))
> + return r;
> + }
> + break;
> + case PPPIOCGIDLE64: /* get idle time information */
> + if (lp) {
> + struct ppp_idle64 pidle;
> + pidle.xmit_idle = pidle.recv_idle = lp->huptimer;
> + if ((r = set_arg(argp, &pidle, sizeof(struct ppp_idle64))))
> return r;
> }
> break;
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 3a7aa2eed415..c8b8aa071140 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -619,7 +619,8 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> struct ppp_file *pf;
> struct ppp *ppp;
> int err = -EFAULT, val, val2, i;
> - struct ppp_idle idle;
> + struct ppp_idle32 idle32;
> + struct ppp_idle64 idle64;
> struct npioctl npi;
> int unit, cflags;
> struct slcompress *vj;
> @@ -743,10 +744,17 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> err = 0;
> break;
>
> - case PPPIOCGIDLE:
> - idle.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
> - idle.recv_idle = (jiffies - ppp->last_recv) / HZ;
> - if (copy_to_user(argp, &idle, sizeof(idle)))
> + case PPPIOCGIDLE32:
> + idle32.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
> + idle32.recv_idle = (jiffies - ppp->last_recv) / HZ;
> + if (copy_to_user(argp, &idle32, sizeof(idle32)))
>
Missing 'break;'
> + err = 0;
> + break;
> +
> + case PPPIOCGIDLE64:
> + idle64.xmit_idle = (jiffies - ppp->last_xmit) / HZ;
> + idle64.recv_idle = (jiffies - ppp->last_recv) / HZ;
> + if (copy_to_user(argp, &idle32, sizeof(idle32)))
>
I guess you meant 'idle64' instead of 'idle32'.
^ permalink raw reply
* Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array
From: Miguel Ojeda @ 2018-08-30 11:10 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Linus Walleij, Jonathan Corbet, 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, Linux
In-Reply-To: <20180829204900.19390-2-jmkrzyszt@gmail.com>
Hi Janusz,
On Wed, Aug 29, 2018 at 10:48 PM, Janusz Krzysztofik
<jmkrzyszt@gmail.com> wrote:
> Most users of get/set array functions iterate consecutive bits of data,
> usually a single integer, while processing array of results obtained
> from, or building an array of values to be passed to those functions.
> Save time wasted on those iterations by changing the functions' API to
> accept bitmaps.
>
> All current users are updated as well.
>
> More benefits from the change are expected as soon as planned support
> for accepting/passing those bitmaps directly from/to respective GPIO
> chip callbacks if applicable is implemented.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Miguel Ojeda Sandonis <miguel.ojeda.sandonis@gmail.com>
> Cc: Peter Korsgaard <peter.korsgaard@barco.com>
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Michael Hennerich <Michael.Hennerich@analog.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> Documentation/driver-api/gpio/consumer.rst | 22 ++++----
> drivers/auxdisplay/hd44780.c | 52 +++++++++--------
> drivers/bus/ts-nbus.c | 19 ++-----
> drivers/gpio/gpio-max3191x.c | 17 +++---
> drivers/gpio/gpiolib.c | 86 +++++++++++++++--------------
> drivers/gpio/gpiolib.h | 4 +-
> drivers/i2c/muxes/i2c-mux-gpio.c | 8 +--
> drivers/mmc/core/pwrseq_simple.c | 13 ++---
> drivers/mux/gpio.c | 9 +--
> drivers/net/phy/mdio-mux-gpio.c | 3 +-
> drivers/pcmcia/soc_common.c | 11 ++--
> drivers/phy/motorola/phy-mapphone-mdm6600.c | 17 +++---
> drivers/staging/iio/adc/ad7606.c | 9 +--
> drivers/tty/serial/serial_mctrl_gpio.c | 7 ++-
> include/linux/gpio/consumer.h | 18 +++---
> 15 files changed, 140 insertions(+), 155 deletions(-)
>
> diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
> index aa03f389d41d..ed68042ddccf 100644
> --- a/Documentation/driver-api/gpio/consumer.rst
> +++ b/Documentation/driver-api/gpio/consumer.rst
> @@ -323,29 +323,29 @@ The following functions get or set the values of an array of GPIOs::
>
> int gpiod_get_array_value(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array);
> + unsigned long *value_bitmap);
> int gpiod_get_raw_array_value(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array);
> + unsigned long *value_bitmap);
> int gpiod_get_array_value_cansleep(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array);
> + unsigned long *value_bitmap);
> int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array);
> + unsigned long *value_bitmap);
>
> void gpiod_set_array_value(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array)
> + unsigned long *value_bitmap)
> void gpiod_set_raw_array_value(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array)
> + unsigned long *value_bitmap)
> void gpiod_set_array_value_cansleep(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array)
> + unsigned long *value_bitmap)
> void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
> struct gpio_desc **desc_array,
> - int *value_array)
> + unsigned long *value_bitmap)
>
> The array can be an arbitrary set of GPIOs. The functions will try to access
> GPIOs belonging to the same bank or chip simultaneously if supported by the
> @@ -356,8 +356,8 @@ accessed sequentially.
> The functions take three arguments:
> * array_size - the number of array elements
> * desc_array - an array of GPIO descriptors
> - * value_array - an array to store the GPIOs' values (get) or
> - an array of values to assign to the GPIOs (set)
> + * value_bitmap - a bitmap to store the GPIOs' values (get) or
> + a bitmap of values to assign to the GPIOs (set)
>
> The descriptor array can be obtained using the gpiod_get_array() function
> or one of its variants. If the group of descriptors returned by that function
> @@ -366,7 +366,7 @@ the struct gpio_descs returned by gpiod_get_array()::
>
> struct gpio_descs *my_gpio_descs = gpiod_get_array(...);
> gpiod_set_array_value(my_gpio_descs->ndescs, my_gpio_descs->desc,
> - my_gpio_values);
> + my_gpio_value_bitmap);
>
> It is also possible to access a completely arbitrary array of descriptors. The
> descriptors may be obtained using any combination of gpiod_get() and
> diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
> index f1a42f0f1ded..bbbd6a29bf01 100644
> --- a/drivers/auxdisplay/hd44780.c
> +++ b/drivers/auxdisplay/hd44780.c
> @@ -62,20 +62,19 @@ static void hd44780_strobe_gpio(struct hd44780 *hd)
> /* write to an LCD panel register in 8 bit GPIO mode */
> static void hd44780_write_gpio8(struct hd44780 *hd, u8 val, unsigned int rs)
> {
> - int values[10]; /* for DATA[0-7], RS, RW */
> - unsigned int i, n;
> + unsigned long value_bitmap[1]; /* for DATA[0-7], RS, RW */
(I read your comments in the other email)
I still find this odd, but if everyone is going to have this change
done like this, consistency is better.
> + unsigned int n;
>
> - for (i = 0; i < 8; i++)
> - values[PIN_DATA0 + i] = !!(val & BIT(i));
> - values[PIN_CTRL_RS] = rs;
> + value_bitmap[0] = val;
> + __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 9;
> if (hd->pins[PIN_CTRL_RW]) {
> - values[PIN_CTRL_RW] = 0;
> + __clear_bit(PIN_CTRL_RW, value_bitmap);
> n++;
> }
>
> /* Present the data to the port */
> - gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA0], values);
> + gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA0], value_bitmap);
>
> hd44780_strobe_gpio(hd);
> }
> @@ -83,32 +82,31 @@ static void hd44780_write_gpio8(struct hd44780 *hd, u8 val, unsigned int rs)
> /* write to an LCD panel register in 4 bit GPIO mode */
> static void hd44780_write_gpio4(struct hd44780 *hd, u8 val, unsigned int rs)
> {
> - int values[10]; /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
> - unsigned int i, n;
> + /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
> + unsigned long value_bitmap[1];
Ditto.
> + unsigned int n;
>
> /* High nibble + RS, RW */
> - for (i = 4; i < 8; i++)
> - values[PIN_DATA0 + i] = !!(val & BIT(i));
> - values[PIN_CTRL_RS] = rs;
> + value_bitmap[0] = val;
> + __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 5;
> if (hd->pins[PIN_CTRL_RW]) {
> - values[PIN_CTRL_RW] = 0;
> + __clear_bit(PIN_CTRL_RW, value_bitmap);
> n++;
> }
> + value_bitmap[0] >>= PIN_DATA4;
>
> /* Present the data to the port */
> - gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4],
> - &values[PIN_DATA4]);
> + gpiod_set_array_value_cansleep(n, &hd->pins[PIN_DATA4], value_bitmap);
>
> hd44780_strobe_gpio(hd);
>
> /* Low nibble */
> - for (i = 0; i < 4; i++)
> - values[PIN_DATA4 + i] = !!(val & BIT(i));
> + value_bitmap[0] &= ~((1 << PIN_DATA4) - 1);
> + value_bitmap[0] |= val & ~((1 << PIN_DATA4) - 1);
This is still wrong! What I originally meant in my v4 review is that
there is an extra ~ in the second line.
Also, a couple of general comments:
- Please review the list of CCs (I was not CC'd originally, so maybe
there are other maintainers that aren't, either)
- In general, the new code seems harder to read than the original one
(but that is my impression).
Thanks for your effort! :-)
Cheers,
Miguel
^ 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