* Re: [PATCH 05/16] net: phy: adin: configure RGMII/RMII/MII modes on config
From: Andrew Lunn @ 2019-08-05 14:39 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-6-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:42PM +0300, Alexandru Ardelean wrote:
> The ADIN1300 chip supports RGMII, RMII & MII modes. Default (if
> unconfigured) is RGMII.
> This change adds support for configuring these modes via the device
> registers.
>
> For RGMII with internal delays (modes RGMII_ID,RGMII_TXID, RGMII_RXID),
It would be nice to add the missing space.
> the default delay is 2 ns. This can be configurable and will be done in
> a subsequent change.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/net/phy/adin.c | 79 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index 3dd9fe50f4c8..dbdb8f60741c 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -33,14 +33,91 @@
> ADIN1300_INT_HW_IRQ_EN)
> #define ADIN1300_INT_STATUS_REG 0x0019
>
> +#define ADIN1300_GE_RGMII_CFG_REG 0xff23
> +#define ADIN1300_GE_RGMII_RXID_EN BIT(2)
> +#define ADIN1300_GE_RGMII_TXID_EN BIT(1)
> +#define ADIN1300_GE_RGMII_EN BIT(0)
> +
> +#define ADIN1300_GE_RMII_CFG_REG 0xff24
> +#define ADIN1300_GE_RMII_EN BIT(0)
> +
> +static int adin_config_rgmii_mode(struct phy_device *phydev,
> + phy_interface_t intf)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RGMII_CFG_REG);
> + if (reg < 0)
> + return reg;
> +
> + if (!phy_interface_mode_is_rgmii(intf)) {
> + reg &= ~ADIN1300_GE_RGMII_EN;
> + goto write;
> + }
> +
> + reg |= ADIN1300_GE_RGMII_EN;
> +
> + if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> + intf == PHY_INTERFACE_MODE_RGMII_RXID) {
> + reg |= ADIN1300_GE_RGMII_RXID_EN;
> + } else {
> + reg &= ~ADIN1300_GE_RGMII_RXID_EN;
> + }
> +
> + if (intf == PHY_INTERFACE_MODE_RGMII_ID ||
> + intf == PHY_INTERFACE_MODE_RGMII_TXID) {
> + reg |= ADIN1300_GE_RGMII_TXID_EN;
> + } else {
> + reg &= ~ADIN1300_GE_RGMII_TXID_EN;
> + }
Nice. Often driver writers forget to clear the delay, they only set
it. Not so here.
However, is checkpatch happy with this? Each half of the if/else is a
single statement, so the {} are not needed.
> +
> +write:
> + return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> + ADIN1300_GE_RGMII_CFG_REG, reg);
> +}
> +
> +static int adin_config_rmii_mode(struct phy_device *phydev,
> + phy_interface_t intf)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, ADIN1300_GE_RMII_CFG_REG);
> + if (reg < 0)
> + return reg;
> +
> + if (intf != PHY_INTERFACE_MODE_RMII) {
> + reg &= ~ADIN1300_GE_RMII_EN;
> + goto write;
goto? Really?
> + }
> +
> + reg |= ADIN1300_GE_RMII_EN;
> +
> +write:
> + return phy_write_mmd(phydev, MDIO_MMD_VEND1,
> + ADIN1300_GE_RMII_CFG_REG, reg);
> +}
> +
> static int adin_config_init(struct phy_device *phydev)
> {
> - int rc;
> + phy_interface_t interface, rc;
genphy_config_init() does not return a phy_interface_t!
>
> rc = genphy_config_init(phydev);
> if (rc < 0)
> return rc;
>
> + interface = phydev->interface;
> +
> + rc = adin_config_rgmii_mode(phydev, interface);
> + if (rc < 0)
> + return rc;
> +
> + rc = adin_config_rmii_mode(phydev, interface);
> + if (rc < 0)
> + return rc;
> +
> + dev_info(&phydev->mdio.dev, "PHY is using mode '%s'\n",
> + phy_modes(phydev->interface));
phydev_dbg(), or not at all.
Andrew
^ permalink raw reply
* Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces
From: Jiri Pirko @ 2019-08-05 14:49 UTC (permalink / raw)
To: David Ahern; +Cc: Jakub Kicinski, netdev, davem, sthemmin, mlxsw
In-Reply-To: <796ba97c-9915-9a44-e933-4a7e22aaef2e@gmail.com>
Mon, Aug 05, 2019 at 04:10:39PM CEST, dsahern@gmail.com wrote:
>On 8/4/19 11:54 PM, Jiri Pirko wrote:
>> There was implicit devlink instance creation per-namespace. No relation
>> any actual device. It was wrong and misuse of devlink.
>>
>> Now you have 1 devlink instance per 1 device as it should be. Also, you
>> have fib resource control for this device, also as it is done for real
>> devices, like mlxsw.
>>
>> Could you please describe your usecase? Perhaps we can handle
>> it differently.
>
>I have described this before, multiple times.
>
>It is documented in the commit log for the initial fib.c in netdevsim
>(37923ed6b8cea94d7d76038e2f72c57a0b45daab) and
>https://lore.kernel.org/netdev/20180328012200.15175-7-dsa@cumulusnetworks.com/
>
>And this comment in the discussion thread:
>
>https://lore.kernel.org/netdev/e9c59b0c-328e-d343-6e8d-d19f643d2e9d@cumulusnetworks.com/:
>"The intention is to treat the kernel's tables *per namespace* as a
>standalone entity that can be managed very similar to ASIC resources."
>
>
>So, to state this again, the fib.c in the RFC version
>https://lore.kernel.org/netdev/20180322225757.10377-8-dsa@cumulusnetworks.com/
>
>targeted this:
>
> namespace 1 | namespace 2 | ... | namespace N
> | | |
> | | |
> devlink 1 | devlink 2 | ... | devlink N
>
>and each devlink instance has resource limits for the number of fib
>rules and fib entries *for that namespace* only.
>
>You objected to how the devlink instances per namespace was implemented,
>so the non-RFC version limited the devlink instance and resource
>controller to init_net only. Fine. I accepted that limitation until
>someone had time to work on devlink instances per network namespace
>which you are doing now. So, the above goal will be achievable but first
>you need to fix the breakage you put into v5.2 and forward.
>
>Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>per-namepace accounting to all namespaces managed by a single devlink
>instance in init_net - which is completely wrong.
No. Not "all namespaces". Only the one where the devlink is. And that is
always init_net, until this patchset.
>
>Move the fib accounting back to per namespace as the original code
>intended. If you now want the devlink instance to be namespace based
>then it should be trivial for you to fix it and will work going forward.
With this patchset, you can create netdevsim instance in a namespace,
set the resources limits on the devlink instance for this netdevsim
and you have what you need and what you had before. You just need to
create one netdevsim instance per network namespace.
Or multiple netdevsim instances in one namespace with different
limitations. Up to you.
^ permalink raw reply
* Re: [RFC] net: dsa: mv88e6xxx: ptp: improve phc2sys precision for mv88e6xxx switch in combination with imx6-fec
From: Hubert Feurstein @ 2019-08-05 14:50 UTC (permalink / raw)
To: Vladimir Oltean
Cc: mlichvar, Richard Cochran, Andrew Lunn, netdev, lkml,
Vivien Didelot, Florian Fainelli, David S. Miller
In-Reply-To: <CA+h21hr835sdvtgVOA2M9SWeCXDOrDG1S3FnNgJd_9NP2X_TaQ@mail.gmail.com>
Hi Vladimir,
Am Mo., 5. Aug. 2019 um 11:55 Uhr schrieb Vladimir Oltean <olteanv@gmail.com>:
[...]
> You guessed correctly (since you copied me) that I'm battling much of
> the same issues with the sja1105 and its spi-fsl-dspi controller
> driver.
I've copied you, because of this discussion on github:
https://github.com/openil/linuxptp/issues/5
There you said: "In fact any MDIO access will make the latency
unpredictable and hence
throw off the time."
I thought you might be interested in how to make MDIO latency predictable.
[...]
> - You said you patched linuxptp master. Could you share the patch? Is
> there anything else that's needed except compiling against the board's
> real kernel headers (aka point KBUILD_OUTPUT to the extracted location
> of /sys/kernel/kheaders.tar.xz)? I've done that and I do see phc2sys
> probing and using the new ioctl, but I don't see a big improvement in
> my case (that's probably because my SPI interface has real jitter
> caused by peripheral clock instability, but I really need to put a
> scope on it to be sure, so that's a discussion for another time).
My compiler used kernel headers where the ioctl was not yet defined.
So I simply defined it in the linuxptp source directly:
diff --git a/sysoff.c b/sysoff.c
index b993ee9..b23ad2f 100644
--- a/sysoff.c
+++ b/sysoff.c
@@ -27,6 +27,20 @@
#define NS_PER_SEC 1000000000LL
+#ifndef PTP_SYS_OFFSET_EXTENDED
+struct ptp_sys_offset_extended {
+ unsigned int n_samples; /* Desired number of measurements. */
+ unsigned int rsv[3]; /* Reserved for future use. */
+ /*
+ * Array of [system, phc, system] time stamps. The kernel will provide
+ * 3*n_samples time stamps.
+ */
+ struct ptp_clock_time ts[PTP_MAX_SAMPLES][3];
+};
+#define PTP_SYS_OFFSET_EXTENDED \
+ _IOWR(PTP_CLK_MAGIC, 9, struct ptp_sys_offset_extended)
+#endif
+
#ifdef PTP_SYS_OFFSET
static int64_t pctns(struct ptp_clock_time *t)
> - I really wonder what your jitter is caused by. Maybe it is just
> contention on the mii_bus->mdio_lock? If that's the case, maybe you
> don't need to go all the way down to the driver level, and taking the
> ptp_sts at the subsystem (MDIO, SPI, I2C, etc) level is "good enough".
I would say there are many places, where we introduce unpredictable jitter:
- The busy-flag polling
- The locking of the chip- and mdio-bus-mutex
- The mdio_done completion in the imx_fec
- Scheduling latencies
> - Along the lines of the above, I wonder how badly would the
> maintainers shout at the proposal of adding a struct
> ptp_system_timestamp pointer and its associated lock in struct device.
> That way at least you don't have to change the API, like you did with
> mdiobus_write_nested_ptp. Relatively speaking, this is the least
> amount of intrusion (although, again, far from beautiful).
It is important to make sure to hook up the right mdio_write access, that is
why the ptp_sts pointer is passed under the mdio_lock. Of course It would
be nicer, to pass through the pointer as an argument, instead of bypassing it to
the mii_bus struct. In the case of SPI it could be added to the spi_transfer
struct.
> - The software timestamps help you in the particular case of phc2sys,
> but are they enough to cover all your needs?
For now it's all I need.
> I haven't spent even 1
> second looking at Marvell switch datasheets, but is its free-running
> timer only used for PTP timestamping? At least the sja1105 does also
> support time-based egress scheduling and ingress policing, so for that
> scenario, the timecounter/cyclecounter implementation will no longer
> cut it (you do need to synchronize the hardware clock). If your
> hardware supports these PTP-based features as well, I can only assume
> the reason why mv88e6xxx went for a timecounter is the same as the
> reason why I did: the MDIO/SPI jitter while accessing the frequency
> and offset correction registers is bad enough that the servo loop goes
> haywire. And implementing gettimex64 will not solve that.
>
[...]
Hubert
^ permalink raw reply related
* Re: [PATCH] staging: isdn: remove unnecessary parentheses
From: Dan Carpenter @ 2019-08-05 14:50 UTC (permalink / raw)
To: Thiago Bonotto
Cc: Karsten Keil, Greg Kroah-Hartman, netdev, devel, linux-kernel,
lkcamp
In-Reply-To: <20190802202323.27117-1-thbonotto@gmail.com>
This driver is obselete so we're just keeping it around for a couple
kernel releases and then deleting it. We're not taking cleanups for it.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
From: Andrew Lunn @ 2019-08-05 14:51 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-7-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote:
> Sometimes, the connection between a MAC and PHY is done via a
> mode/interface converter. An example is a GMII-to-RGMII converter, which
> would mean that the MAC operates in GMII mode while the PHY operates in
> RGMII. In this case there is a discrepancy between what the MAC expects &
> what the PHY expects and both need to be configured in their respective
> modes.
>
> Sometimes, this converter is specified via a board/system configuration (in
> the device-tree for example). But, other times it can be left unspecified.
> The use of these converters is common in boards that have FPGA on them.
>
> This patch also adds support for a `adi,phy-mode-internal` property that
> can be used in these (implicit convert) cases. The internal PHY mode will
> be used to specify the correct register settings for the PHY.
>
> `fwnode_handle` is used, since this property may be specified via ACPI as
> well in other setups, but testing has been done in DT context.
Looking at the patch, you seems to assume phy-mode is what the MAC is
using? That seems rather odd, given the name. It seems like a better
solution would be to add a mac-mode, which the MAC uses to configure
its side of the link. The MAC driver would then implement this
property.
I don't see a need for this. phy-mode indicates what the PHY should
use. End of story.
Andrew
^ permalink raw reply
* Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces
From: David Ahern @ 2019-08-05 14:51 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Jakub Kicinski, netdev, davem, sthemmin, mlxsw
In-Reply-To: <20190805144927.GD2349@nanopsycho.orion>
On 8/5/19 8:49 AM, Jiri Pirko wrote:
>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>> per-namepace accounting to all namespaces managed by a single devlink
>> instance in init_net - which is completely wrong.
> No. Not "all namespaces". Only the one where the devlink is. And that is
> always init_net, until this patchset.
>
>
Jiri: your change to fib.c does not take into account namespace when
doing rules and routes accounting. you broke it. fix it.
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: dump nested registers
From: Vivien Didelot @ 2019-08-05 14:52 UTC (permalink / raw)
To: Michal Kubecek; +Cc: netdev, f.fainelli, andrew, davem, linville, cphealy
In-Reply-To: <20190805080448.GA31971@unicorn.suse.cz>
Hi Michal!
On Mon, 5 Aug 2019 10:04:48 +0200, Michal Kubecek <mkubecek@suse.cz> wrote:
> On Fri, Aug 02, 2019 at 03:34:54PM -0400, Vivien Didelot wrote:
> > Usually kernel drivers set the regs->len value to the same length as
> > info->regdump_len, which was used for the allocation. In case where
> > regs->len is smaller than the allocated info->regdump_len length,
> > we may assume that the dump contains a nested set of registers.
> >
> > This becomes handy for kernel drivers to expose registers of an
> > underlying network conduit unfortunately not exposed to userspace,
> > as found in network switching equipment for example.
> >
> > This patch adds support for recursing into the dump operation if there
> > is enough room for a nested ethtool_drvinfo structure containing a
> > valid driver name, followed by a ethtool_regs structure like this:
> >
> > 0 regs->len info->regdump_len
> > v v v
> > +--------------+-----------------+--------------+-- - --+
> > | ethtool_regs | ethtool_drvinfo | ethtool_regs | |
> > +--------------+-----------------+--------------+-- - --+
> >
> > Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
> > ---
>
> I'm not sure about this approach. If these additional objects with their
> own registers are represented by a network device, we can query their
> registers directly. If they are not (which, IIUC, is the case in your
> use case), we should use an appropriate interface. AFAIK the CPU ports
> are already represented in devlink, shouldn't devlink be also used to
> query their registers?
Yet another interface wasn't that much appropriate for DSA, making the stack
unnecessarily complex. In fact we are already glueing the statistics of the CPU
port into the master's ethtool ops (both physical ports are wired together).
Adding support for nested registers dump in ethtool makes it simple to
(pretty) dump CPU port's registers without too much userspace addition.
>
> > ethtool.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/ethtool.c b/ethtool.c
> > index 05fe05a08..c0e2903c5 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -1245,7 +1245,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >
> > if (gregs_dump_raw) {
> > fwrite(regs->data, regs->len, 1, stdout);
> > - return 0;
> > + goto nested;
> > }
You're right regarding your comment about raw output. I can keep the return
0 here.
> >
> > if (!gregs_dump_hex)
> > @@ -1253,7 +1253,7 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> > if (!strncmp(driver_list[i].name, info->driver,
> > ETHTOOL_BUSINFO_LEN)) {
> > if (driver_list[i].func(info, regs) == 0)
> > - return 0;
> > + goto nested;
> > /* This version (or some other
> > * variation in the dump format) is
> > * not handled; fall back to hex
> > @@ -1263,6 +1263,15 @@ static int dump_regs(int gregs_dump_raw, int gregs_dump_hex,
> >
> > dump_hex(stdout, regs->data, regs->len, 0);
> >
> > +nested:
> > + /* Recurse dump if some drvinfo and regs structures are nested */
> > + if (info->regdump_len > regs->len + sizeof(*info) + sizeof(*regs)) {
> > + info = (struct ethtool_drvinfo *)(®s->data[0] + regs->len);
> > + regs = (struct ethtool_regs *)(®s->data[0] + regs->len + sizeof(*info));
> > +
> > + return dump_regs(gregs_dump_raw, gregs_dump_hex, info, regs);
> > + }
> > +
> > return 0;
> > }
> >
>
> For raw and hex dumps, this will dump only the payloads without any
> metadata allowing to identify what are the additional blocks for the
> other related objects, i.e. where they start, how long they are and what
> they belong to. That doesn't seem very useful.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH 1/2] usb: serial: option: Add the BroadMobi BM818 card
From: Bob Ham @ 2019-08-05 14:44 UTC (permalink / raw)
To: Johan Hovold, Angus Ainslie (Purism)
Cc: kernel, Bjørn Mork, David S. Miller, Greg Kroah-Hartman,
netdev, linux-usb, linux-kernel
In-Reply-To: <20190805114711.GF3574@localhost>
[-- Attachment #1.1: Type: text/plain, Size: 950 bytes --]
On 05/08/2019 12:47, Johan Hovold wrote:
> On Wed, Jul 24, 2019 at 07:52:26AM -0700, Angus Ainslie (Purism) wrote:
>> From: Bob Ham <bob.ham@puri.sm>
>>
>> Add a VID:PID for the BroadModi BM818 M.2 card
>
> Would you mind posting the output of usb-devices (or lsusb -v) for this
> device?
T: Bus=01 Lev=03 Prnt=40 Port=03 Cnt=01 Dev#= 44 Spd=480 MxCh= 0
D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
P: Vendor=2020 ProdID=2060 Rev=00.00
S: Manufacturer=Qualcomm, Incorporated
S: Product=Qualcomm CDMA Technologies MSM
C: #Ifs= 5 Cfg#= 1 Atr=e0 MxPwr=500mA
I: If#=0x0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#=0x1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#=0x2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I: If#=0x3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=fe Prot=ff Driver=(none)
I: If#=0x4 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH]][next] selftests: nettest: fix spelling mistake: "potocol" -> "protocol"
From: shuah @ 2019-08-05 15:04 UTC (permalink / raw)
To: Colin King, David S . Miller, netdev, linux-kselftest
Cc: kernel-janitors, linux-kernel, shuah
In-Reply-To: <20190805105211.27229-1-colin.king@canonical.com>
On 8/5/19 4:52 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a spelling mistake in an error messgae. Fix it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> tools/testing/selftests/net/nettest.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/net/nettest.c b/tools/testing/selftests/net/nettest.c
> index 9278f8460d75..83515e5ea4dc 100644
> --- a/tools/testing/selftests/net/nettest.c
> +++ b/tools/testing/selftests/net/nettest.c
> @@ -1627,7 +1627,7 @@ int main(int argc, char *argv[])
> args.protocol = pe->p_proto;
> } else {
> if (str_to_uint(optarg, 0, 0xffff, &tmp) != 0) {
> - fprintf(stderr, "Invalid potocol\n");
> + fprintf(stderr, "Invalid protocol\n");
> return 1;
> }
> args.protocol = tmp;
>
Assuming this will go through net tree
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH bpf-next 1/3] selftests/bpf: test_progs: switch to open_memstream
From: Stanislav Fomichev @ 2019-08-05 15:12 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, Stanislav Fomichev, netdev@vger.kernel.org,
bpf@vger.kernel.org, davem@davemloft.net, ast@kernel.org,
daniel@iogearbox.net
In-Reply-To: <CAEf4BzYV31v6ch-k+ZCQr1RaBuGComt9C0dQjFV1Es42qXz-8Q@mail.gmail.com>
On 08/02, Andrii Nakryiko wrote:
> On Fri, Aug 2, 2019 at 1:14 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> >
> > On 08/02, Andrii Nakryiko wrote:
> > > On 8/2/19 10:17 AM, Stanislav Fomichev wrote:
> > > > Use open_memstream to override stdout during test execution.
> > > > The copy of the original stdout is held in env.stdout and used
> > > > to print subtest info and dump failed log.
> > >
> > > I really like the idea. I didn't know about open_memstream, it's awesome. Thanks!
> > One possible downside of using open_memstream is that it's glibc
> > specific. I probably need to wrap it in #ifdef __GLIBC__ to make
> > it work with other libcs and just print everything as it was before :-(.
> > I'm not sure we care though.
>
> Given this is selftests/bpf, it is probably OK.
>
> >
> > > > test_{v,}printf are now simple wrappers around stdout and will be
> > > > removed in the next patch.
> > > >
> > > > Cc: Andrii Nakryiko <andriin@fb.com>
> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > > ---
> > > > tools/testing/selftests/bpf/test_progs.c | 100 ++++++++++-------------
> > > > tools/testing/selftests/bpf/test_progs.h | 2 +-
> > > > 2 files changed, 46 insertions(+), 56 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > > > index db00196c8315..00d1565d01a3 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.c
> > > > +++ b/tools/testing/selftests/bpf/test_progs.c
> > > > @@ -40,14 +40,22 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
> > > >
> > > > static void dump_test_log(const struct prog_test_def *test, bool failed)
> > > > {
> > > > - if (env.verbose || test->force_log || failed) {
> > > > - if (env.log_cnt) {
> > > > - fprintf(stdout, "%s", env.log_buf);
> > > > - if (env.log_buf[env.log_cnt - 1] != '\n')
> > > > - fprintf(stdout, "\n");
> > > > + if (stdout == env.stdout)
> > > > + return;
> > > > +
> > > > + fflush(stdout); /* exports env.log_buf & env.log_cap */
> > > > +
> > > > + if (env.log_cap && (env.verbose || test->force_log || failed)) {
> > > > + int len = strlen(env.log_buf);
> > >
> > > env.log_cap is not really a capacity, it's actual number of bytes (without terminating zero), so there is no need to do strlen and it's probably better to rename env.log_cap into env.log_cnt.
> > I'll rename it to log_size to match open_memstream args.
> > We probably still need to do strlen because open_memstream can allocate
> > bigger buffer to hold the data.
>
> If I read man page correctly, env.log_cnt will be exactly the value
> that strlen will return - number of actual bytes written (omitting
> terminal zero), not number of pre-allocated bytes, thus I'm saying
> that strlen is redundant. Please take a look again.
Yeah, you're right, I've played with it a bit and it does return the
length of the data, not the (possibly bigger) size of the buffer.
Will fix in a v3 and use log_cnt.
> >
> > > > +
> > > > + if (len) {
> > > > + fprintf(env.stdout, "%s", env.log_buf);
> > > > + if (env.log_buf[len - 1] != '\n')
> > > > + fprintf(env.stdout, "\n");
> > > > +
> > > > + fseeko(stdout, 0, SEEK_SET);
> > > Same bug as I already fixed with env.log_cnt = 0 being inside this if. You want to do seek always, not just when you print output log.
> > SG, will move to where we currently clear log_cnt, thanks!
> >
> > > > /* rewind */
> > > > }
> > > > }
> > > > - env.log_cnt = 0;
> > > > }
> > > >
> > > > void test__end_subtest()
> > > > @@ -62,7 +70,7 @@ void test__end_subtest()
> > > >
> > > > dump_test_log(test, sub_error_cnt);
> > > >
> > > > - printf("#%d/%d %s:%s\n",
> > > > + fprintf(env.stdout, "#%d/%d %s:%s\n",
> > > > test->test_num, test->subtest_num,
> > > > test->subtest_name, sub_error_cnt ? "FAIL" : "OK");
> > > > }
> > > > @@ -100,53 +108,7 @@ void test__force_log() {
> > > >
> > > > void test__vprintf(const char *fmt, va_list args)
> > > > {
> > > > - size_t rem_sz;
> > > > - int ret = 0;
> > > > -
> > > > - if (env.verbose || (env.test && env.test->force_log)) {
> > > > - vfprintf(stderr, fmt, args);
> > > > - return;
> > > > - }
> > > > -
> > > > -try_again:
> > > > - rem_sz = env.log_cap - env.log_cnt;
> > > > - if (rem_sz) {
> > > > - va_list ap;
> > > > -
> > > > - va_copy(ap, args);
> > > > - /* we reserved extra byte for \0 at the end */
> > > > - ret = vsnprintf(env.log_buf + env.log_cnt, rem_sz + 1, fmt, ap);
> > > > - va_end(ap);
> > > > -
> > > > - if (ret < 0) {
> > > > - env.log_buf[env.log_cnt] = '\0';
> > > > - fprintf(stderr, "failed to log w/ fmt '%s'\n", fmt);
> > > > - return;
> > > > - }
> > > > - }
> > > > -
> > > > - if (!rem_sz || ret > rem_sz) {
> > > > - size_t new_sz = env.log_cap * 3 / 2;
> > > > - char *new_buf;
> > > > -
> > > > - if (new_sz < 4096)
> > > > - new_sz = 4096;
> > > > - if (new_sz < ret + env.log_cnt)
> > > > - new_sz = ret + env.log_cnt;
> > > > -
> > > > - /* +1 for guaranteed space for terminating \0 */
> > > > - new_buf = realloc(env.log_buf, new_sz + 1);
> > > > - if (!new_buf) {
> > > > - fprintf(stderr, "failed to realloc log buffer: %d\n",
> > > > - errno);
> > > > - return;
> > > > - }
> > > > - env.log_buf = new_buf;
> > > > - env.log_cap = new_sz;
> > > > - goto try_again;
> > > > - }
> > > > -
> > > > - env.log_cnt += ret;
> > > > + vprintf(fmt, args);
> > > > }
> > > >
> > > > void test__printf(const char *fmt, ...)
> > > > @@ -477,6 +439,32 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> > > > return 0;
> > > > }
> > > >
> > > > +static void stdout_hijack(void)
> > > > +{
> > > > + if (env.verbose || (env.test && env.test->force_log)) {
> > > > + /* nothing to do, output to stdout by default */
> > > > + return;
> > > > + }
> > > > +
> > > > + /* stdout -> buffer */
> > > > + fflush(stdout);
> > > > + stdout = open_memstream(&env.log_buf, &env.log_cap);
> > > Check errors and restore original stdout if something went wrong? (And emit some warning to stderr).
> > Good point, will do.
> >
> > > > +}
> > > > +
> > > > +static void stdout_restore(void)
> > > > +{
> > > > + if (stdout == env.stdout)
> > > > + return;
> > > > +
> > > > + fclose(stdout);
> > > > + free(env.log_buf);
> > > > +
> > > > + env.log_buf = NULL;
> > > > + env.log_cap = 0;
> > > > +
> > > > + stdout = env.stdout;
> > > > +}
> > > > +
> > > > int main(int argc, char **argv)
> > > > {
> > > > static const struct argp argp = {
> > > > @@ -495,6 +483,7 @@ int main(int argc, char **argv)
> > > > srand(time(NULL));
> > > >
> > > > env.jit_enabled = is_jit_enabled();
> > > > + env.stdout = stdout;
> > > >
> > > > for (i = 0; i < prog_test_cnt; i++) {
> > > > struct prog_test_def *test = &prog_test_defs[i];
> > > > @@ -508,6 +497,7 @@ int main(int argc, char **argv)
> > > > test->test_num, test->test_name))
> > > > continue;
> > > >
> > > > + stdout_hijack();
> > > Why do you do this for every test? Just do once before all the tests and restore after?
> > We can do that, my thinking was to limit the area of hijacking :-)
>
> But why? We actually want to hijack stdout/stderr for entire duration
> of all the tests. If test_progs needs some "infrastructural" mandatory
> output, we have env.stdout/env.stderr for that.
Oh, I agree, I've done just that for v2 that's already out.
I was just trying to justify my initial thinking :-)
> > But that would work as well, less allocations per test, I guess. Will
> > do.
> >
> > > > test->run_test();
> > > > /* ensure last sub-test is finalized properly */
> > > > if (test->subtest_name)
> > > > @@ -522,6 +512,7 @@ int main(int argc, char **argv)
> > > > env.succ_cnt++;
> > > >
> > > > dump_test_log(test, test->error_cnt);
> > > > + stdout_restore();
> > > >
> > > > printf("#%d %s:%s\n", test->test_num, test->test_name,
> > > > test->error_cnt ? "FAIL" : "OK");
> > > > @@ -529,7 +520,6 @@ int main(int argc, char **argv)
> > > > printf("Summary: %d/%d PASSED, %d FAILED\n",
> > > > env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
> > > >
> > > > - free(env.log_buf);
> > > > free(env.test_selector.num_set);
> > > > free(env.subtest_selector.num_set);
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > > > index afd14962456f..9fd89078494f 100644
> > > > --- a/tools/testing/selftests/bpf/test_progs.h
> > > > +++ b/tools/testing/selftests/bpf/test_progs.h
> > > > @@ -56,8 +56,8 @@ struct test_env {
> > > > bool jit_enabled;
> > > >
> > > > struct prog_test_def *test;
> > > > + FILE *stdout;
> > > > char *log_buf;
> > > > - size_t log_cnt;
> > > > size_t log_cap;
> > > So it's actually log_cnt that's assigned on fflush for memstream, according to man page, so probably keep log_cnt, delete log_cap.
> > Ack. See above, will rename to log_size, let me know if you disagree.
> >
> > > > int succ_cnt; /* successful tests */
^ permalink raw reply
* Re: [PATCH 11/16] net: phy: adin: PHY reset mechanisms
From: Andrew Lunn @ 2019-08-05 15:15 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-12-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:48PM +0300, Alexandru Ardelean wrote:
> The ADIN PHYs supports 4 types of reset:
> 1. The standard PHY reset via BMCR_RESET bit in MII_BMCR reg
> 2. Reset via GPIO
> 3. Reset via reg GeSftRst (0xff0c) & reload previous pin configs
> 4. Reset via reg GeSftRst (0xff0c) & request new pin configs
>
> Resets 2 & 4 are almost identical, with the exception that the crystal
> oscillator is available during reset for 2.
>
> Resetting via GeSftRst or via GPIO is useful when doing a warm reboot. If
> doing various settings via phytool or ethtool, the sub-system registers
> don't reset just via BMCR_RESET.
>
> This change implements resetting the entire PHY subsystem during probe.
> During PHY HW init (phy_hw_init() logic) the PHY core regs will be reset
> again via BMCR_RESET. This will also need to happen during a PM resume.
phylib already has support for GPIO reset. So if possible, you should
not repeat that code here.
What is the difference between a GPIO reset, and a GPIO reset followed
by a subsystem soft reset?
Andrew
^ permalink raw reply
* Re: [PATCH 01/16] net: phy: adin: add support for Analog Devices PHYs
From: Andrew Lunn @ 2019-08-05 15:17 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-2-alexandru.ardelean@analog.com>
> +static struct phy_driver adin_driver[] = {
> + {
> + .phy_id = PHY_ID_ADIN1200,
> + .name = "ADIN1200",
> + .phy_id_mask = 0xfffffff0,
> + .features = PHY_BASIC_FEATURES,
Do you need this? If the device implements the registers correctly,
phylib can determine this from the registers.
> + .config_init = adin_config_init,
> + .config_aneg = genphy_config_aneg,
> + .read_status = genphy_read_status,
> + },
> + {
> + .phy_id = PHY_ID_ADIN1300,
> + .name = "ADIN1300",
> + .phy_id_mask = 0xfffffff0,
> + .features = PHY_GBIT_FEATURES,
same here.
> + .config_init = adin_config_init,
> + .config_aneg = genphy_config_aneg,
> + .read_status = genphy_read_status,
> + },
> +};
> +
> +module_phy_driver(adin_driver);
> +
> +static struct mdio_device_id __maybe_unused adin_tbl[] = {
> + { PHY_ID_ADIN1200, 0xfffffff0 },
> + { PHY_ID_ADIN1300, 0xfffffff0 },
PHY_ID_MATCH_VENDOR().
Andrew
^ permalink raw reply
* Re: [PATCH 12/16] net: phy: adin: read EEE setting from device-tree
From: Andrew Lunn @ 2019-08-05 15:19 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-13-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:49PM +0300, Alexandru Ardelean wrote:
> By default, EEE is not advertised on system init. This change allows the
> user to specify a device property to enable EEE advertisements when the PHY
> initializes.
This patch is not required. If EEE is not being advertised when it
should, it means there is a MAC driver bug.
Andrew
^ permalink raw reply
* Re: [patch net-next v2 1/3] net: devlink: allow to change namespaces
From: Jiri Pirko @ 2019-08-05 15:20 UTC (permalink / raw)
To: David Ahern; +Cc: Jakub Kicinski, netdev, davem, sthemmin, mlxsw
In-Reply-To: <566cdf6c-dafc-fb3e-bd94-b75eba3488b5@gmail.com>
Mon, Aug 05, 2019 at 04:51:22PM CEST, dsahern@gmail.com wrote:
>On 8/5/19 8:49 AM, Jiri Pirko wrote:
>>> Your commit 5fc494225c1eb81309cc4c91f183cd30e4edb674 changed that from a
>>> per-namepace accounting to all namespaces managed by a single devlink
>>> instance in init_net - which is completely wrong.
>> No. Not "all namespaces". Only the one where the devlink is. And that is
>> always init_net, until this patchset.
>>
>>
>
>Jiri: your change to fib.c does not take into account namespace when
>doing rules and routes accounting. you broke it. fix it.
What do you mean by "account namespace"? It's a device resource, why to
tight it with namespace? What if you have 2 netdevsim-devlink instances
in one namespace? Why the setting should be per-namespace?
^ permalink raw reply
* Re: [PATCH 14/16] net: phy: adin: make sure down-speed auto-neg is enabled
From: Andrew Lunn @ 2019-08-05 15:22 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-15-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:51PM +0300, Alexandru Ardelean wrote:
> Down-speed auto-negotiation may not always be enabled, in which case the
> PHY won't down-shift to 100 or 10 during auto-negotiation.
Please look at how the marvell driver enables and configures this
feature. Ideally we want all PHY drivers to use the same configuration
API for features like this.
Andrew
^ permalink raw reply
* Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
From: Andrew Lunn @ 2019-08-05 15:28 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-16-alexandru.ardelean@analog.com>
> +struct adin_hw_stat {
> + const char *string;
> +static void adin_get_strings(struct phy_device *phydev, u8 *data)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(adin_hw_stats); i++) {
> + memcpy(data + i * ETH_GSTRING_LEN,
> + adin_hw_stats[i].string, ETH_GSTRING_LEN);
You define string as a char *. So it will be only as long as it should
be. However memcpy always copies ETH_GSTRING_LEN bytes, doing off the
end of the string and into whatever follows.
> + }
> +}
> +
> +static int adin_read_mmd_stat_regs(struct phy_device *phydev,
> + struct adin_hw_stat *stat,
> + u32 *val)
> +{
> + int ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg1);
> + if (ret < 0)
> + return ret;
> +
> + *val = (ret & 0xffff);
> +
> + if (stat->reg2 == 0)
> + return 0;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, stat->reg2);
> + if (ret < 0)
> + return ret;
> +
> + *val <<= 16;
> + *val |= (ret & 0xffff);
Does the hardware have a snapshot feature? Is there a danger that
between the two reads stat->reg1 rolls over and you end up with too
big a value?
Andrew
^ permalink raw reply
* Re: [PATCH] tools: bpftool: fix reading from /proc/config.gz
From: Stanislav Fomichev @ 2019-08-05 15:29 UTC (permalink / raw)
To: Peter Wu
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, Stanislav Fomichev,
Jakub Kicinski, Quentin Monnet
In-Reply-To: <20190805001541.8096-1-peter@lekensteyn.nl>
On 08/05, Peter Wu wrote:
> /proc/config has never existed as far as I can see, but /proc/config.gz
> is present on Arch Linux. Execute an external gunzip program to avoid
> linking to zlib and rework the option scanning code since a pipe is not
> seekable. This also fixes a file handle leak on some error paths.
Thanks for doing that! One question: why not link against -lz instead?
With fork/execing gunzip you're just hiding this dependency.
You can add something like this to the Makefile:
ifeq ($(feature-zlib),1)
CLFAGS += -DHAVE_ZLIB
endif
And then conditionally add support for config.gz. Thoughts?
>
> Fixes: 4567b983f78c ("tools: bpftool: add probes for kernel configuration options")
> Cc: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
> tools/bpf/bpftool/feature.c | 92 +++++++++++++++++++++----------------
> 1 file changed, 52 insertions(+), 40 deletions(-)
>
> diff --git a/tools/bpf/bpftool/feature.c b/tools/bpf/bpftool/feature.c
> index d672d9086fff..e9e10f582047 100644
> --- a/tools/bpf/bpftool/feature.c
> +++ b/tools/bpf/bpftool/feature.c
> @@ -284,34 +284,34 @@ static void probe_jit_limit(void)
> }
> }
>
> -static char *get_kernel_config_option(FILE *fd, const char *option)
> +static bool get_kernel_config_option(FILE *fd, char **buf_p, size_t *n_p,
> + char **value)
> {
> - size_t line_n = 0, optlen = strlen(option);
> - char *res, *strval, *line = NULL;
> - ssize_t n;
> + char *sep;
> + ssize_t linelen;
>
> - rewind(fd);
> - while ((n = getline(&line, &line_n, fd)) > 0) {
> - if (strncmp(line, option, optlen))
> + while ((linelen = getline(buf_p, n_p, fd)) > 0) {
> + char *line = *buf_p;
> + if (strncmp(line, "CONFIG_", 7))
> continue;
> - /* Check we have at least '=', value, and '\n' */
> - if (strlen(line) < optlen + 3)
> - continue;
> - if (*(line + optlen) != '=')
> +
> + sep = memchr(line, '=', linelen);
> + if (!sep)
> continue;
>
> /* Trim ending '\n' */
> - line[strlen(line) - 1] = '\0';
> + line[linelen - 1] = '\0';
> +
> + /* Split on '=' and ensure that a value is present. */
> + *sep = '\0';
> + if (!sep[1])
> + continue;
>
> - /* Copy and return config option value */
> - strval = line + optlen + 1;
> - res = strdup(strval);
> - free(line);
> - return res;
> + *value = sep + 1;
> + return true;
> }
> - free(line);
>
> - return NULL;
> + return false;
> }
>
> static void probe_kernel_image_config(void)
> @@ -386,31 +386,34 @@ static void probe_kernel_image_config(void)
> /* test_bpf module for BPF tests */
> "CONFIG_TEST_BPF",
> };
> + char *values[ARRAY_SIZE(options)] = { };
> char *value, *buf = NULL;
> struct utsname utsn;
> char path[PATH_MAX];
> size_t i, n;
> ssize_t ret;
> - FILE *fd;
> + FILE *fd = NULL;
> + bool is_pipe = false;
>
> if (uname(&utsn))
> - goto no_config;
> + goto end_parse;
>
> snprintf(path, sizeof(path), "/boot/config-%s", utsn.release);
>
> fd = fopen(path, "r");
> if (!fd && errno == ENOENT) {
> - /* Some distributions put the config file at /proc/config, give
> - * it a try.
> - * Sometimes it is also at /proc/config.gz but we do not try
> - * this one for now, it would require linking against libz.
> + /* Some distributions build with CONFIG_IKCONFIG=y and put the
> + * config file at /proc/config.gz. We try to invoke an external
> + * gzip program to avoid linking to libz.
> + * Hide stderr to avoid interference with the JSON output.
> */
> - fd = fopen("/proc/config", "r");
> + fd = popen("gunzip -c /proc/config.gz 2>/dev/null", "r");
> + is_pipe = true;
> }
> if (!fd) {
> p_info("skipping kernel config, can't open file: %s",
> strerror(errno));
> - goto no_config;
> + goto end_parse;
> }
> /* Sanity checks */
> ret = getline(&buf, &n, fd);
> @@ -418,27 +421,36 @@ static void probe_kernel_image_config(void)
> if (!buf || !ret) {
> p_info("skipping kernel config, can't read from file: %s",
> strerror(errno));
> - free(buf);
> - goto no_config;
> + goto end_parse;
> }
> if (strcmp(buf, "# Automatically generated file; DO NOT EDIT.\n")) {
> p_info("skipping kernel config, can't find correct file");
> - free(buf);
> - goto no_config;
> + goto end_parse;
> + }
> +
> + while (get_kernel_config_option(fd, &buf, &n, &value)) {
> + for (i = 0; i < ARRAY_SIZE(options); i++) {
> + if (values[i] || strcmp(buf, options[i]))
> + continue;
> +
> + values[i] = strdup(value);
> + }
> + }
> +
> +end_parse:
> + if (fd) {
> + if (is_pipe) {
> + if (pclose(fd))
> + p_info("failed to read /proc/config.gz");
> + } else
> + fclose(fd);
> }
> free(buf);
>
> for (i = 0; i < ARRAY_SIZE(options); i++) {
> - value = get_kernel_config_option(fd, options[i]);
> - print_kernel_option(options[i], value);
> - free(value);
> + print_kernel_option(options[i], values[i]);
> + free(values[i]);
> }
> - fclose(fd);
> - return;
> -
> -no_config:
> - for (i = 0; i < ARRAY_SIZE(options); i++)
> - print_kernel_option(options[i], NULL);
> }
>
> static bool probe_bpf_syscall(const char *define_prefix)
> --
> 2.22.0
>
^ permalink raw reply
* Re: [PATCH 15/16] net: phy: adin: add ethtool get_stats support
From: Andrew Lunn @ 2019-08-05 15:30 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, mark.rutland,
f.fainelli, hkallweit1
In-Reply-To: <20190805165453.3989-16-alexandru.ardelean@analog.com>
On Mon, Aug 05, 2019 at 07:54:52PM +0300, Alexandru Ardelean wrote:
> This change implements retrieving all the error counters from the PHY.
> The PHY supports several error counters/stats. The `Mean Square Errors`
> status values are only valie when a link is established, and shouldn't be
> incremented. These values characterize the quality of a signal.
I think you mean accumulated, not incremented?
>
> The rest of the error counters are self-clearing on read.
> Most of them are reports from the Frame Checker engine that the PHY has.
>
> Not retrieving the `LPI Wake Error Count Register` here, since that is used
> by the PHY framework to check for any EEE errors. And that register is
> self-clearing when read (as per IEEE spec).
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/net/phy/adin.c | 108 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 108 insertions(+)
>
> diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
> index a1f3456a8504..04896547dac8 100644
> --- a/drivers/net/phy/adin.c
> +++ b/drivers/net/phy/adin.c
> @@ -103,6 +103,32 @@ static struct clause22_mmd_map clause22_mmd_map[] = {
> { MDIO_MMD_PCS, MDIO_PCS_EEE_WK_ERR, ADIN1300_LPI_WAKE_ERR_CNT_REG },
> };
>
> +struct adin_hw_stat {
> + const char *string;
> + u16 reg1;
> + u16 reg2;
> + bool do_not_inc;
do_not_accumulate? or reverse its meaning, clear_on_read?
Andrew
^ permalink raw reply
* [PATCH v2 0/1] Fix bridge mac_len handling
From: Zahari Doychev @ 2019-08-05 15:37 UTC (permalink / raw)
To: netdev
Cc: bridge, nikolay, roopa, jhs, dsahern, simon.horman,
makita.toshiaki, xiyou.wangcong, jiri, ast, johannes,
Zahari Doychev
After the last discussion about the possible solution of the problem. I have
decided to resend the patches making the discussed corrections. It seems that
the patches are still not the complete solution as there still can be problem
handling skb->data pointing after the VLAN tag but I got the impression that
all agreed that the bridge code should be able to handle mac_len correctly.
Here again the description how to problem can be reproduce.
The Linux bridge does not correctly forward double vlan tagged packets added
using the tc act_vlan action. I am using a bridge with two netdevs and on one
of them a have the clsact qdisc with tc flower rule adding two vlan tags.
ip link add name br0 type bridge vlan_filtering 1
ip link set dev br0 up
ip link set dev net0 up
ip link set dev net0 master br0
ip link set dev net1 up
ip link set dev net1 master br0
bridge vlan add dev net0 vid 100 master
bridge vlan add dev br0 vid 100 self
bridge vlan add dev net1 vid 100 master
tc qdisc add dev net0 handle ffff: clsact
tc qdisc add dev net1 handle ffff: clsact
tc filter add dev net0 ingress pref 1 protocol all flower \
action vlan push id 10 pipe action vlan push id 100
tc filter add dev net0 egress pref 1 protocol 802.1q flower \
vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
action vlan pop pipe action vlan pop
When using the setup above the packets coming on net0 get double tagged but
the MAC headers gets corrupted when the packets go out of net1. The second vlan
header is not considered in br_dev_queue_push_xmit. The skb data pointer is
decremented only by the ETH_HLEN length. This later causes the function
validate_xmit_vlan to insert the outer vlan tag behind the inner vlan tag.
The inner vlan becomes in this way part of the source mac address.
The problem in the bridge forwarding is fixed by using the mac_len when using
skb_push before forwarding the packets which ensures that the skb->data is
set correctly on push/pull.
Changes from v1:
- reset mac_len in br_dev_xmit
Zahari Doychev (1):
net: bridge: use mac_len in bridge forwarding
net/bridge/br_device.c | 3 ++-
net/bridge/br_forward.c | 4 ++--
net/bridge/br_vlan.c | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
--
2.22.0
^ permalink raw reply
* [PATCH v2 1/1] net: bridge: use mac_len in bridge forwarding
From: Zahari Doychev @ 2019-08-05 15:37 UTC (permalink / raw)
To: netdev
Cc: bridge, nikolay, roopa, jhs, dsahern, simon.horman,
makita.toshiaki, xiyou.wangcong, jiri, ast, johannes,
Zahari Doychev
In-Reply-To: <20190805153740.29627-1-zahari.doychev@linux.com>
The bridge code cannot forward packets from various paths that set up the
SKBs in different ways. Some of these packets get corrupted during the
forwarding as not always is just ETH_HLEN pulled at the front. This happens
e.g. when VLAN tags are pushed bu using tc act_vlan on ingress.
The problem is fixed by using skb->mac_len instead of ETH_HLEN, which makes
sure that the skb headers are correctly restored. This usually does not
change anything, execpt the local bridge transmits which now need to set
the skb->mac_len correctly in br_dev_xmit, as well as the broken case noted
above.
Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
net/bridge/br_device.c | 3 ++-
net/bridge/br_forward.c | 4 ++--
net/bridge/br_vlan.c | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 681b72862c16..aeb77ff60311 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -55,8 +55,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
BR_INPUT_SKB_CB(skb)->frag_max_size = 0;
skb_reset_mac_header(skb);
+ skb_reset_mac_len(skb);
eth = eth_hdr(skb);
- skb_pull(skb, ETH_HLEN);
+ skb_pull(skb, skb->mac_len);
if (!br_allowed_ingress(br, br_vlan_group_rcu(br), skb, &vid))
goto out;
diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
index 86637000f275..edb4f3533f05 100644
--- a/net/bridge/br_forward.c
+++ b/net/bridge/br_forward.c
@@ -32,7 +32,7 @@ static inline int should_deliver(const struct net_bridge_port *p,
int br_dev_queue_push_xmit(struct net *net, struct sock *sk, struct sk_buff *skb)
{
- skb_push(skb, ETH_HLEN);
+ skb_push(skb, skb->mac_len);
if (!is_skb_forwardable(skb->dev, skb))
goto drop;
@@ -94,7 +94,7 @@ static void __br_forward(const struct net_bridge_port *to,
net = dev_net(indev);
} else {
if (unlikely(netpoll_tx_running(to->br->dev))) {
- skb_push(skb, ETH_HLEN);
+ skb_push(skb, skb->mac_len);
if (!is_skb_forwardable(skb->dev, skb))
kfree_skb(skb);
else
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 021cc9f66804..88244c9cc653 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -466,13 +466,14 @@ static bool __allowed_ingress(const struct net_bridge *br,
/* Tagged frame */
if (skb->vlan_proto != br->vlan_proto) {
/* Protocol-mismatch, empty out vlan_tci for new tag */
- skb_push(skb, ETH_HLEN);
+ skb_push(skb, skb->mac_len);
skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
skb_vlan_tag_get(skb));
if (unlikely(!skb))
return false;
skb_pull(skb, ETH_HLEN);
+ skb_reset_network_header(skb);
skb_reset_mac_len(skb);
*vid = 0;
tagged = false;
--
2.22.0
^ permalink raw reply related
* [PATCH bpf-next v3 0/3] selftests/bpf: switch test_progs back to stdio
From: Stanislav Fomichev @ 2019-08-05 15:40 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko
I was looking into converting test_sockops* to test_progs framework
and that requires using cgroup_helpers.c which rely on stdio/stderr.
Let's use open_memstream to override stdout into buffer during
subtests instead of custom test_{v,}printf wrappers. That lets
us continue to use stdio in the subtests and dump it on failure
if required.
That would also fix bpf_find_map which currently uses printf to
signal failure (missed during test_printf conversion).
Cc: Andrii Nakryiko <andriin@fb.com>
Stanislav Fomichev (3):
selftests/bpf: test_progs: switch to open_memstream
selftests/bpf: test_progs: test__printf -> printf
selftests/bpf: test_progs: drop extra trailing tab
.../bpf/prog_tests/bpf_verif_scale.c | 4 +-
.../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
.../selftests/bpf/prog_tests/map_lock.c | 10 +-
.../selftests/bpf/prog_tests/send_signal.c | 4 +-
.../selftests/bpf/prog_tests/spinlock.c | 2 +-
.../bpf/prog_tests/stacktrace_build_id.c | 4 +-
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
.../selftests/bpf/prog_tests/xdp_noinline.c | 4 +-
tools/testing/selftests/bpf/test_progs.c | 131 ++++++++----------
tools/testing/selftests/bpf/test_progs.h | 12 +-
10 files changed, 83 insertions(+), 94 deletions(-)
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply
* [PATCH bpf-next v3 1/3] selftests/bpf: test_progs: switch to open_memstream
From: Stanislav Fomichev @ 2019-08-05 15:40 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko
In-Reply-To: <20190805154055.197664-1-sdf@google.com>
Use open_memstream to override stdout during test execution.
The copy of the original stdout is held in env.stdout and used
to print subtest info and dump failed log.
test_{v,}printf are now simple wrappers around stdout and will be
removed in the next patch.
v3:
* don't do strlen over log_buf, log_cnt has it already (Andrii Nakryiko)
v2:
* add ifdef __GLIBC__ around open_memstream (maybe pointless since
we already depend on glibc for argp_parse)
* hijack stderr as well (Andrii Nakryiko)
* don't hijack for every test, do it once (Andrii Nakryiko)
* log_cap -> log_size (Andrii Nakryiko)
* do fseeko in a proper place (Andrii Nakryiko)
* check open_memstream returned value (Andrii Nakryiko)
Cc: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/test_progs.c | 115 ++++++++++++-----------
tools/testing/selftests/bpf/test_progs.h | 2 +-
2 files changed, 61 insertions(+), 56 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index db00196c8315..9556439c607c 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -40,14 +40,20 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
static void dump_test_log(const struct prog_test_def *test, bool failed)
{
+ if (stdout == env.stdout)
+ return;
+
+ fflush(stdout); /* exports env.log_buf & env.log_cnt */
+
if (env.verbose || test->force_log || failed) {
if (env.log_cnt) {
- fprintf(stdout, "%s", env.log_buf);
+ fprintf(env.stdout, "%s", env.log_buf);
if (env.log_buf[env.log_cnt - 1] != '\n')
- fprintf(stdout, "\n");
+ fprintf(env.stdout, "\n");
}
}
- env.log_cnt = 0;
+
+ fseeko(stdout, 0, SEEK_SET); /* rewind */
}
void test__end_subtest()
@@ -62,7 +68,7 @@ void test__end_subtest()
dump_test_log(test, sub_error_cnt);
- printf("#%d/%d %s:%s\n",
+ fprintf(env.stdout, "#%d/%d %s:%s\n",
test->test_num, test->subtest_num,
test->subtest_name, sub_error_cnt ? "FAIL" : "OK");
}
@@ -79,7 +85,8 @@ bool test__start_subtest(const char *name)
test->subtest_num++;
if (!name || !name[0]) {
- fprintf(stderr, "Subtest #%d didn't provide sub-test name!\n",
+ fprintf(env.stderr,
+ "Subtest #%d didn't provide sub-test name!\n",
test->subtest_num);
return false;
}
@@ -100,53 +107,7 @@ void test__force_log() {
void test__vprintf(const char *fmt, va_list args)
{
- size_t rem_sz;
- int ret = 0;
-
- if (env.verbose || (env.test && env.test->force_log)) {
- vfprintf(stderr, fmt, args);
- return;
- }
-
-try_again:
- rem_sz = env.log_cap - env.log_cnt;
- if (rem_sz) {
- va_list ap;
-
- va_copy(ap, args);
- /* we reserved extra byte for \0 at the end */
- ret = vsnprintf(env.log_buf + env.log_cnt, rem_sz + 1, fmt, ap);
- va_end(ap);
-
- if (ret < 0) {
- env.log_buf[env.log_cnt] = '\0';
- fprintf(stderr, "failed to log w/ fmt '%s'\n", fmt);
- return;
- }
- }
-
- if (!rem_sz || ret > rem_sz) {
- size_t new_sz = env.log_cap * 3 / 2;
- char *new_buf;
-
- if (new_sz < 4096)
- new_sz = 4096;
- if (new_sz < ret + env.log_cnt)
- new_sz = ret + env.log_cnt;
-
- /* +1 for guaranteed space for terminating \0 */
- new_buf = realloc(env.log_buf, new_sz + 1);
- if (!new_buf) {
- fprintf(stderr, "failed to realloc log buffer: %d\n",
- errno);
- return;
- }
- env.log_buf = new_buf;
- env.log_cap = new_sz;
- goto try_again;
- }
-
- env.log_cnt += ret;
+ vprintf(fmt, args);
}
void test__printf(const char *fmt, ...)
@@ -477,6 +438,48 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
return 0;
}
+static void stdio_hijack(void)
+{
+#ifdef __GLIBC__
+ if (env.verbose || (env.test && env.test->force_log)) {
+ /* nothing to do, output to stdout by default */
+ return;
+ }
+
+ /* stdout and stderr -> buffer */
+ fflush(stdout);
+
+ env.stdout = stdout;
+ env.stderr = stderr;
+
+ stdout = open_memstream(&env.log_buf, &env.log_cnt);
+ if (!stdout) {
+ stdout = env.stdout;
+ perror("open_memstream");
+ return;
+ }
+
+ stderr = stdout;
+#endif
+}
+
+static void stdio_restore(void)
+{
+#ifdef __GLIBC__
+ if (stdout == env.stdout)
+ return;
+
+ fclose(stdout);
+ free(env.log_buf);
+
+ env.log_buf = NULL;
+ env.log_cnt = 0;
+
+ stdout = env.stdout;
+ stderr = env.stderr;
+#endif
+}
+
int main(int argc, char **argv)
{
static const struct argp argp = {
@@ -496,6 +499,7 @@ int main(int argc, char **argv)
env.jit_enabled = is_jit_enabled();
+ stdio_hijack();
for (i = 0; i < prog_test_cnt; i++) {
struct prog_test_def *test = &prog_test_defs[i];
int old_pass_cnt = pass_cnt;
@@ -523,13 +527,14 @@ int main(int argc, char **argv)
dump_test_log(test, test->error_cnt);
- printf("#%d %s:%s\n", test->test_num, test->test_name,
- test->error_cnt ? "FAIL" : "OK");
+ fprintf(env.stdout, "#%d %s:%s\n",
+ test->test_num, test->test_name,
+ test->error_cnt ? "FAIL" : "OK");
}
+ stdio_restore();
printf("Summary: %d/%d PASSED, %d FAILED\n",
env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
- free(env.log_buf);
free(env.test_selector.num_set);
free(env.subtest_selector.num_set);
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index afd14962456f..4c00fc79ac5f 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -56,9 +56,9 @@ struct test_env {
bool jit_enabled;
struct prog_test_def *test;
+ FILE *stdout, *stderr;
char *log_buf;
size_t log_cnt;
- size_t log_cap;
int succ_cnt; /* successful tests */
int sub_succ_cnt; /* successful sub-tests */
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH bpf-next v3 2/3] selftests/bpf: test_progs: test__printf -> printf
From: Stanislav Fomichev @ 2019-08-05 15:40 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko
In-Reply-To: <20190805154055.197664-1-sdf@google.com>
Now that test__printf is a simple wraper around printf, let's drop it
(and test__vprintf as well).
Cc: Andrii Nakryiko <andriin@fb.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/prog_tests/bpf_verif_scale.c | 4 ++--
.../testing/selftests/bpf/prog_tests/l4lb_all.c | 2 +-
.../testing/selftests/bpf/prog_tests/map_lock.c | 10 +++++-----
.../selftests/bpf/prog_tests/send_signal.c | 4 ++--
.../testing/selftests/bpf/prog_tests/spinlock.c | 2 +-
.../bpf/prog_tests/stacktrace_build_id.c | 4 ++--
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 ++--
.../selftests/bpf/prog_tests/xdp_noinline.c | 4 ++--
tools/testing/selftests/bpf/test_progs.c | 16 +---------------
tools/testing/selftests/bpf/test_progs.h | 10 ++++------
10 files changed, 22 insertions(+), 38 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index b4be96162ff4..3548ba2f24a8 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -5,13 +5,13 @@ static int libbpf_debug_print(enum libbpf_print_level level,
const char *format, va_list args)
{
if (level != LIBBPF_DEBUG) {
- test__vprintf(format, args);
+ vprintf(format, args);
return 0;
}
if (!strstr(format, "verifier log"))
return 0;
- test__vprintf("%s", args);
+ vprintf("%s", args);
return 0;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
index 5ce572c03a5f..20ddca830e68 100644
--- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
+++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
@@ -74,7 +74,7 @@ static void test_l4lb(const char *file)
}
if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
error_cnt++;
- test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
+ printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
}
out:
bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index 2e78217ed3fd..ee99368c595c 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
for (i = 0; i < 10000; i++) {
err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
if (err) {
- test__printf("lookup failed\n");
+ printf("lookup failed\n");
error_cnt++;
goto out;
}
if (vars[0] != 0) {
- test__printf("lookup #%d var[0]=%d\n", i, vars[0]);
+ printf("lookup #%d var[0]=%d\n", i, vars[0]);
error_cnt++;
goto out;
}
@@ -22,8 +22,8 @@ static void *parallel_map_access(void *arg)
for (j = 2; j < 17; j++) {
if (vars[j] == rnd)
continue;
- test__printf("lookup #%d var[1]=%d var[%d]=%d\n",
- i, rnd, j, vars[j]);
+ printf("lookup #%d var[1]=%d var[%d]=%d\n",
+ i, rnd, j, vars[j]);
error_cnt++;
goto out;
}
@@ -43,7 +43,7 @@ void test_map_lock(void)
err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
if (err) {
- test__printf("test_map_lock:bpf_prog_load errno %d\n", errno);
+ printf("test_map_lock:bpf_prog_load errno %d\n", errno);
goto close_prog;
}
map_fd[0] = bpf_find_map(__func__, obj, "hash_map");
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 461b423d0584..1575f0a1f586 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -202,8 +202,8 @@ static int test_send_signal_nmi(void)
-1 /* cpu */, -1 /* group_fd */, 0 /* flags */);
if (pmu_fd == -1) {
if (errno == ENOENT) {
- test__printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
- __func__);
+ printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
+ __func__);
return 0;
}
/* Let the test fail with a more informative message */
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index deb2db5b85b0..114ebe6a438e 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -12,7 +12,7 @@ void test_spinlock(void)
err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
if (err) {
- test__printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
+ printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
goto close_prog;
}
for (i = 0; i < 4; i++)
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
index 356d2c017a9c..ac44fda84833 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
@@ -109,8 +109,8 @@ void test_stacktrace_build_id(void)
if (build_id_matches < 1 && retry--) {
bpf_link__destroy(link);
bpf_object__close(obj);
- test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
- __func__);
+ printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+ __func__);
goto retry;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index f44f2c159714..9557b7dfb782 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -140,8 +140,8 @@ void test_stacktrace_build_id_nmi(void)
if (build_id_matches < 1 && retry--) {
bpf_link__destroy(link);
bpf_object__close(obj);
- test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
- __func__);
+ printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+ __func__);
goto retry;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
index b5404494b8aa..15f7c272edb0 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
@@ -75,8 +75,8 @@ void test_xdp_noinline(void)
}
if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
error_cnt++;
- test__printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
- bytes, pkts);
+ printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
+ bytes, pkts);
}
out:
bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 9556439c607c..963912008042 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -105,20 +105,6 @@ void test__force_log() {
env.test->force_log = true;
}
-void test__vprintf(const char *fmt, va_list args)
-{
- vprintf(fmt, args);
-}
-
-void test__printf(const char *fmt, ...)
-{
- va_list args;
-
- va_start(args, fmt);
- test__vprintf(fmt, args);
- va_end(args);
-}
-
struct ipv4_packet pkt_v4 = {
.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
.iph.ihl = 5,
@@ -310,7 +296,7 @@ static int libbpf_print_fn(enum libbpf_print_level level,
{
if (!env.very_verbose && level == LIBBPF_DEBUG)
return 0;
- test__vprintf(format, args);
+ vprintf(format, args);
return 0;
}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 4c00fc79ac5f..f346ea213140 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -69,8 +69,6 @@ extern int error_cnt;
extern int pass_cnt;
extern struct test_env env;
-extern void test__printf(const char *fmt, ...);
-extern void test__vprintf(const char *fmt, va_list args);
extern void test__force_log();
extern bool test__start_subtest(const char *name);
@@ -96,12 +94,12 @@ extern struct ipv6_packet pkt_v6;
int __ret = !!(condition); \
if (__ret) { \
error_cnt++; \
- test__printf("%s:FAIL:%s ", __func__, tag); \
- test__printf(format); \
+ printf("%s:FAIL:%s ", __func__, tag); \
+ printf(format); \
} else { \
pass_cnt++; \
- test__printf("%s:PASS:%s %d nsec\n", \
- __func__, tag, duration); \
+ printf("%s:PASS:%s %d nsec\n", \
+ __func__, tag, duration); \
} \
__ret; \
})
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH bpf-next v3 3/3] selftests/bpf: test_progs: drop extra trailing tab
From: Stanislav Fomichev @ 2019-08-05 15:40 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev, Andrii Nakryiko
In-Reply-To: <20190805154055.197664-1-sdf@google.com>
Small (un)related cleanup.
Cc: Andrii Nakryiko <andriin@fb.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/test_progs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 963912008042..beed74043933 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -278,7 +278,7 @@ enum ARG_KEYS {
ARG_VERIFIER_STATS = 's',
ARG_VERBOSE = 'v',
};
-
+
static const struct argp_option opts[] = {
{ "num", ARG_TEST_NUM, "NUM", 0,
"Run test number NUM only " },
--
2.22.0.770.g0f2c4a37fd-goog
^ permalink raw reply related
* [PATCH] xen/netback: Reset nr_frags before freeing skb
From: Ross Lagerwall @ 2019-08-05 15:34 UTC (permalink / raw)
To: netdev; +Cc: xen-devel, David S. Miller, Paul Durrant, Wei Liu, Ross Lagerwall
At this point nr_frags has been incremented but the frag does not yet
have a page assigned so freeing the skb results in a crash. Reset
nr_frags before freeing the skb to prevent this.
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
drivers/net/xen-netback/netback.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index 1d9940d4e8c7..c9262ffeefe4 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -925,6 +925,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
skb_shinfo(skb)->nr_frags = MAX_SKB_FRAGS;
nskb = xenvif_alloc_skb(0);
if (unlikely(nskb == NULL)) {
+ skb_shinfo(skb)->nr_frags = 0;
kfree_skb(skb);
xenvif_tx_err(queue, &txreq, extra_count, idx);
if (net_ratelimit())
@@ -940,6 +941,7 @@ static void xenvif_tx_build_gops(struct xenvif_queue *queue,
if (xenvif_set_skb_gso(queue->vif, skb, gso)) {
/* Failure in xenvif_set_skb_gso is fatal. */
+ skb_shinfo(skb)->nr_frags = 0;
kfree_skb(skb);
kfree_skb(nskb);
break;
--
2.17.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox