* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville @ 2021-02-13 2:32 kernel test robot 0 siblings, 0 replies; 10+ messages in thread From: kernel test robot @ 2021-02-13 2:32 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 3473 bytes --] CC: kbuild-all(a)lists.01.org In-Reply-To: <20210213001412.4154051-10-olteanv@gmail.com> References: <20210213001412.4154051-10-olteanv@gmail.com> TO: Vladimir Oltean <olteanv@gmail.com> TO: "David S . Miller" <davem@davemloft.net> TO: Jakub Kicinski <kuba@kernel.org> TO: netdev(a)vger.kernel.org CC: Andrew Lunn <andrew@lunn.ch> CC: Florian Fainelli <f.fainelli@gmail.com> CC: Vivien Didelot <vivien.didelot@gmail.com> CC: Richard Cochran <richardcochran@gmail.com> CC: Claudiu Manoil <claudiu.manoil@nxp.com> CC: Alexandre Belloni <alexandre.belloni@bootlin.com> CC: Vladimir Oltean <vladimir.oltean@nxp.com> Hi Vladimir, I love your patch! Perhaps something to improve: [auto build test WARNING on net-next/master] url: https://github.com/0day-ci/linux/commits/Vladimir-Oltean/PTP-for-DSA-tag_ocelot_8021q/20210213-081857 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3c5a2fd042d0bfac71a2dfb99515723d318df47b :::::: branch date: 2 hours ago :::::: commit date: 2 hours ago config: i386-randconfig-m031-20210209 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: net/dsa/tag_ocelot.c:59 ocelot_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? net/dsa/tag_ocelot.c:71 seville_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? vim +59 net/dsa/tag_ocelot.c 9d88a16c0fc930 Vladimir Oltean 2021-02-13 51 9d88a16c0fc930 Vladimir Oltean 2021-02-13 52 static struct sk_buff *ocelot_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 53 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 54 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 55 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 56 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 57 9d88a16c0fc930 Vladimir Oltean 2021-02-13 58 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x8880000a), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @59 ocelot_ifh_set_dest(injection, BIT(dp->index)); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 60 9d88a16c0fc930 Vladimir Oltean 2021-02-13 61 return skb; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 62 } 9d88a16c0fc930 Vladimir Oltean 2021-02-13 63 9d88a16c0fc930 Vladimir Oltean 2021-02-13 64 static struct sk_buff *seville_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 65 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 66 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 67 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 68 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 69 9d88a16c0fc930 Vladimir Oltean 2021-02-13 70 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x88800005), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @71 seville_ifh_set_dest(injection, BIT(dp->index)); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 72 8dce89aa5f3274 Vladimir Oltean 2019-11-14 73 return skb; 8dce89aa5f3274 Vladimir Oltean 2019-11-14 74 } 8dce89aa5f3274 Vladimir Oltean 2019-11-14 75 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 42328 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next 00/12] PTP for DSA tag_ocelot_8021q @ 2021-02-13 0:14 Vladimir Oltean 2021-02-13 0:14 ` [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville Vladimir Oltean 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2021-02-13 0:14 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, netdev Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Richard Cochran, Claudiu Manoil, Alexandre Belloni, Vladimir Oltean, Maxim Kochetkov, UNGLinuxDriver From: Vladimir Oltean <vladimir.oltean@nxp.com> This is part two of the errata workaround begun here: https://patchwork.kernel.org/project/netdevbpf/cover/20210129010009.3959398-1-olteanv@gmail.com/ Now that we have basic traffic support when we operate the Ocelot DSA switches without an NPI port, it would be nice to regain some of the features lost due to the lack of the NPI port functionality. An important one is PTP timestamping, which is intimately tied to the DSA frame header added by the NPI port: on TX, we put a "timestamp request ID" in the Injection Frame Header, while on RX, the Extraction Frame Header contains a partial 32-bit PTP timestamp. Get rid of the NPI port and replace it with a VLAN-based tagger, and you lose PTP, right? Well, not quite, this is what this patch series is about. The NPI port is basically a regular Ethernet port configured to service the packets in and out of the switch's CPU port module (which has other non-DSA I/O mechanisms too, such as register-based MMIO and DMA). If we disable the NPI port, we can in theory still access the packets delivered to the CPU port module by doing exactly what the ocelot switchdev driver does: extracting Ethernet packets through registers (yes, it is as icky as it sounds). However, there's a catch. The Felix switch was integrated into NXP LS1028A with the idea in mind that it will operate as DSA, i.e. using the CPU port module connected to the NPI port, not having I/O over register-based MMIO which is painfully slow and CPU intensive. So register-based packet I/O not supposed to work - those registers aren't even documented in the hardware reference manual for Felix. However they kinda do, with the exception of the fact that an RX interrupt was really not wired to the CPU cores - so we don't know when the CPU port module receives a new packet. But we can hack even around that, by replicating every packet that goes to the CPU port module and making it also go to a plain internal Ethernet port. Then drop the Ethernet packet and read the other copy of it from the CPU port module, this time annotated with the much-wanted RX timestamp. This is all fine and it works, but it does raise some questions about what DSA even is anymore, if we start having switches that inject some of their packets over Ethernet and some through registers, where do we draw the line. In principle I believe these concerns are founded, but at the same time, the way that the Felix driver uses register MMIO based packet I/O is fundamentally the same as any other DSA driver capable of PTP makes use of a side-channel for timestamps like a FIFO (just that this one is a lot more complicated, and comes with the entire actual packet, not just the timestamp). Nonetheless, I tried to keep the extra pressure added by this ERR workaround upon the DSA subsystem as small as possible, so some of the patches are just a revisit of some of Andrew's complaints w.r.t. the fact that tag_ocelot already violates any driver <-> tagger boundary, and as a consequence, is not able to be used on testbeds such as dsa_loop (which it now can). So now, the tag_ocelot and tag_ocelot_8021q drivers should be dsa_loop-clean, and have the ERR workarounds as self-contained as possible, using all the designated features for PTP timestamping and nothing more. Comments appreciated. Vladimir Oltean (12): net: mscc: ocelot: stop returning IRQ_NONE in ocelot_xtr_irq_handler net: mscc: ocelot: only drain extraction queue on error net: mscc: ocelot: better error handling in ocelot_xtr_irq_handler net: mscc: ocelot: use DIV_ROUND_UP helper in ocelot_port_inject_frame net: mscc: ocelot: refactor ocelot_port_inject_frame out of ocelot_port_xmit net: dsa: tag_ocelot: avoid accessing ds->priv in ocelot_rcv net: mscc: ocelot: use common tag parsing code with DSA net: dsa: tag_ocelot: single out PTP-related transmit tag processing net: dsa: tag_ocelot: create separate tagger for Seville net: mscc: ocelot: refactor ocelot_xtr_irq_handler into ocelot_xtr_poll net: dsa: felix: setup MMIO filtering rules for PTP when using tag_8021q net: dsa: tag_ocelot_8021q: add support for PTP timestamping drivers/net/dsa/ocelot/felix.c | 224 +++++++++++++++++-- drivers/net/dsa/ocelot/felix.h | 14 +- drivers/net/dsa/ocelot/felix_vsc9959.c | 29 +-- drivers/net/dsa/ocelot/seville_vsc9953.c | 30 +-- drivers/net/ethernet/mscc/ocelot.c | 216 ++++++++++++++++++ drivers/net/ethernet/mscc/ocelot.h | 9 - drivers/net/ethernet/mscc/ocelot_net.c | 81 +------ drivers/net/ethernet/mscc/ocelot_vsc7514.c | 178 +-------------- include/linux/dsa/ocelot.h | 218 ++++++++++++++++++ include/net/dsa.h | 2 + include/soc/mscc/ocelot.h | 13 +- net/dsa/tag_ocelot.c | 244 +++++++-------------- net/dsa/tag_ocelot_8021q.c | 31 +++ 13 files changed, 794 insertions(+), 495 deletions(-) create mode 100644 include/linux/dsa/ocelot.h -- 2.25.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville 2021-02-13 0:14 [PATCH net-next 00/12] PTP for DSA tag_ocelot_8021q Vladimir Oltean @ 2021-02-13 0:14 ` Vladimir Oltean 2021-02-15 13:00 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2021-02-13 0:14 UTC (permalink / raw) To: David S . Miller, Jakub Kicinski, netdev Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, Richard Cochran, Claudiu Manoil, Alexandre Belloni, Vladimir Oltean, Maxim Kochetkov, UNGLinuxDriver From: Vladimir Oltean <vladimir.oltean@nxp.com> The ocelot tagger is a hot mess currently, it relies on memory initialized by the attached driver for basic frame transmission. This is against all that DSA tagging protocols stand for, which is that the transmission and reception of a DSA-tagged frame, the data path, should be independent from the switch control path, because the tag protocol is in principle hot-pluggable and reusable across switches (even if in practice it wasn't until very recently). But if another driver like dsa_loop wants to make use of tag_ocelot, it couldn't. This was done to have common code between Felix and Ocelot, which have one bit difference in the frame header format. Quoting from commit 67c2404922c2 ("net: dsa: felix: create a template for the DSA tags on xmit"): Other alternatives have been analyzed, such as: - Create a separate tag_seville.c: too much code duplication for just 1 bit field difference. - Create a separate DSA_TAG_PROTO_SEVILLE under tag_ocelot.c, just like tag_brcm.c, which would have a separate .xmit function. Again, too much code duplication for just 1 bit field difference. - Allocate the template from the init function of the tag_ocelot.c module, instead of from the driver: couldn't figure out a method of accessing the correct port template corresponding to the correct tagger in the .xmit function. The really interesting part is that Seville should have had its own tagging protocol defined - it is not compatible on the wire with Ocelot, even for that single bit. In principle, a packet generated by DSA_TAG_PROTO_OCELOT when booted on NXP LS1028A would look in a certain way, but when booted on NXP T1040 it would look differently. The reverse is also true: a packet generated by a Seville switch would be interpreted incorrectly by Wireshark if it was told it was generated by an Ocelot switch. Actually things are a bit more nuanced. If we concentrate only on the DSA tag, what I said above is true, but Ocelot/Seville also support an optional DSA tag prefix, which can be short or long, and it is possible to distinguish the two taggers based on an integer constant put in that prefix. Nonetheless, creating a separate tagger is still justified, since the tag prefix is optional, and without it, there is again no way to distinguish. Claiming backwards binary compatibility is a bit more tough, since I've already changed the format of tag_ocelot once, in commit 5124197ce58b ("net: dsa: tag_ocelot: use a short prefix on both ingress and egress"). Therefore I am not very concerned with treating this as a bugfix and backporting it to stable kernels (which would be another mess due to the fact that there would be lots of conflicts with the other DSA_TAG_PROTO* definitions). It's just simpler to say that the string values of the taggers have ABI value starting with kernel 5.12, which will be when the changing of tag protocol via /sys/class/net/<dsa-master>/dsa/tagging goes live. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/ocelot/felix.c | 18 ++----- drivers/net/dsa/ocelot/felix.h | 1 - drivers/net/dsa/ocelot/felix_vsc9959.c | 26 --------- drivers/net/dsa/ocelot/seville_vsc9953.c | 28 +--------- include/linux/dsa/ocelot.h | 10 ++++ include/net/dsa.h | 2 + net/dsa/tag_ocelot.c | 68 ++++++++++++++++++------ 7 files changed, 70 insertions(+), 83 deletions(-) diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 4af1187f4d69..f294f5f62505 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -431,6 +431,7 @@ static int felix_set_tag_protocol(struct dsa_switch *ds, int cpu, int err; switch (proto) { + case DSA_TAG_PROTO_SEVILLE: case DSA_TAG_PROTO_OCELOT: err = felix_setup_tag_npi(ds, cpu); break; @@ -448,6 +449,7 @@ static void felix_del_tag_protocol(struct dsa_switch *ds, int cpu, enum dsa_tag_protocol proto) { switch (proto) { + case DSA_TAG_PROTO_SEVILLE: case DSA_TAG_PROTO_OCELOT: felix_teardown_tag_npi(ds, cpu); break; @@ -471,7 +473,8 @@ static int felix_change_tag_protocol(struct dsa_switch *ds, int cpu, enum dsa_tag_protocol old_proto = felix->tag_proto; int err; - if (proto != DSA_TAG_PROTO_OCELOT && + if (proto != DSA_TAG_PROTO_SEVILLE && + proto != DSA_TAG_PROTO_OCELOT && proto != DSA_TAG_PROTO_OCELOT_8021Q) return -EPROTONOSUPPORT; @@ -1003,7 +1006,6 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports) for (port = 0; port < num_phys_ports; port++) { struct ocelot_port *ocelot_port; struct regmap *target; - u8 *template; ocelot_port = devm_kzalloc(ocelot->dev, sizeof(struct ocelot_port), @@ -1029,22 +1031,10 @@ static int felix_init_structs(struct felix *felix, int num_phys_ports) return PTR_ERR(target); } - template = devm_kzalloc(ocelot->dev, OCELOT_TOTAL_TAG_LEN, - GFP_KERNEL); - if (!template) { - dev_err(ocelot->dev, - "Failed to allocate memory for DSA tag\n"); - kfree(port_phy_modes); - return -ENOMEM; - } - ocelot_port->phy_mode = port_phy_modes[port]; ocelot_port->ocelot = ocelot; ocelot_port->target = target; - ocelot_port->xmit_template = template; ocelot->ports[port] = ocelot_port; - - felix->info->xmit_template_populate(ocelot, port); } kfree(port_phy_modes); diff --git a/drivers/net/dsa/ocelot/felix.h b/drivers/net/dsa/ocelot/felix.h index 9d4459f2fffb..b2ea425c5803 100644 --- a/drivers/net/dsa/ocelot/felix.h +++ b/drivers/net/dsa/ocelot/felix.h @@ -34,7 +34,6 @@ struct felix_info { enum tc_setup_type type, void *type_data); void (*port_sched_speed_set)(struct ocelot *ocelot, int port, u32 speed); - void (*xmit_template_populate)(struct ocelot *ocelot, int port); }; extern const struct dsa_switch_ops felix_switch_ops; diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c index cacc6f9c0113..32b885fcaf90 100644 --- a/drivers/net/dsa/ocelot/felix_vsc9959.c +++ b/drivers/net/dsa/ocelot/felix_vsc9959.c @@ -1339,31 +1339,6 @@ static int vsc9959_port_setup_tc(struct dsa_switch *ds, int port, } } -static void vsc9959_xmit_template_populate(struct ocelot *ocelot, int port) -{ - struct ocelot_port *ocelot_port = ocelot->ports[port]; - u8 *template = ocelot_port->xmit_template; - u64 bypass, dest, src; - __be32 *prefix; - u8 *injection; - - /* Set the source port as the CPU port module and not the - * NPI port - */ - src = ocelot->num_phys_ports; - dest = BIT(port); - bypass = true; - - injection = template + OCELOT_SHORT_PREFIX_LEN; - prefix = (__be32 *)template; - - packing(injection, &bypass, 127, 127, OCELOT_TAG_LEN, PACK, 0); - packing(injection, &dest, 68, 56, OCELOT_TAG_LEN, PACK, 0); - packing(injection, &src, 46, 43, OCELOT_TAG_LEN, PACK, 0); - - *prefix = cpu_to_be32(0x8880000a); -} - static const struct felix_info felix_info_vsc9959 = { .target_io_res = vsc9959_target_io_res, .port_io_res = vsc9959_port_io_res, @@ -1386,7 +1361,6 @@ static const struct felix_info felix_info_vsc9959 = { .prevalidate_phy_mode = vsc9959_prevalidate_phy_mode, .port_setup_tc = vsc9959_port_setup_tc, .port_sched_speed_set = vsc9959_sched_speed_set, - .xmit_template_populate = vsc9959_xmit_template_populate, }; static irqreturn_t felix_irq_handler(int irq, void *data) diff --git a/drivers/net/dsa/ocelot/seville_vsc9953.c b/drivers/net/dsa/ocelot/seville_vsc9953.c index d7348ea4831e..84f93a874d50 100644 --- a/drivers/net/dsa/ocelot/seville_vsc9953.c +++ b/drivers/net/dsa/ocelot/seville_vsc9953.c @@ -1165,31 +1165,6 @@ static void vsc9953_mdio_bus_free(struct ocelot *ocelot) mdiobus_unregister(felix->imdio); } -static void vsc9953_xmit_template_populate(struct ocelot *ocelot, int port) -{ - struct ocelot_port *ocelot_port = ocelot->ports[port]; - u8 *template = ocelot_port->xmit_template; - u64 bypass, dest, src; - __be32 *prefix; - u8 *injection; - - /* Set the source port as the CPU port module and not the - * NPI port - */ - src = ocelot->num_phys_ports; - dest = BIT(port); - bypass = true; - - injection = template + OCELOT_SHORT_PREFIX_LEN; - prefix = (__be32 *)template; - - packing(injection, &bypass, 127, 127, OCELOT_TAG_LEN, PACK, 0); - packing(injection, &dest, 67, 57, OCELOT_TAG_LEN, PACK, 0); - packing(injection, &src, 46, 43, OCELOT_TAG_LEN, PACK, 0); - - *prefix = cpu_to_be32(0x88800005); -} - static const struct felix_info seville_info_vsc9953 = { .target_io_res = vsc9953_target_io_res, .port_io_res = vsc9953_port_io_res, @@ -1206,7 +1181,6 @@ static const struct felix_info seville_info_vsc9953 = { .mdio_bus_free = vsc9953_mdio_bus_free, .phylink_validate = vsc9953_phylink_validate, .prevalidate_phy_mode = vsc9953_prevalidate_phy_mode, - .xmit_template_populate = vsc9953_xmit_template_populate, }; static int seville_probe(struct platform_device *pdev) @@ -1246,7 +1220,7 @@ static int seville_probe(struct platform_device *pdev) ds->ops = &felix_switch_ops; ds->priv = ocelot; felix->ds = ds; - felix->tag_proto = DSA_TAG_PROTO_OCELOT; + felix->tag_proto = DSA_TAG_PROTO_SEVILLE; err = dsa_register_switch(ds); if (err) { diff --git a/include/linux/dsa/ocelot.h b/include/linux/dsa/ocelot.h index add2b38a2c76..59eadd73289a 100644 --- a/include/linux/dsa/ocelot.h +++ b/include/linux/dsa/ocelot.h @@ -195,6 +195,16 @@ static inline void ocelot_ifh_set_qos_class(void *injection, u64 qos_class) packing(injection, &qos_class, 19, 17, OCELOT_TAG_LEN, PACK, 0); } +static inline void seville_ifh_set_dest(void *injection, u64 dest) +{ + packing(injection, &dest, 67, 57, OCELOT_TAG_LEN, PACK, 0); +} + +static inline void ocelot_ifh_set_src(void *injection, u64 src) +{ + packing(injection, &src, 46, 43, OCELOT_TAG_LEN, PACK, 0); +} + static inline void ocelot_ifh_set_tag_type(void *injection, u64 tag_type) { packing(injection, &tag_type, 16, 16, OCELOT_TAG_LEN, PACK, 0); diff --git a/include/net/dsa.h b/include/net/dsa.h index 74457aaffec7..b095ef114fe8 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -48,6 +48,7 @@ struct phylink_link_state; #define DSA_TAG_PROTO_HELLCREEK_VALUE 18 #define DSA_TAG_PROTO_XRS700X_VALUE 19 #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20 +#define DSA_TAG_PROTO_SEVILLE_VALUE 21 enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, @@ -71,6 +72,7 @@ enum dsa_tag_protocol { DSA_TAG_PROTO_HELLCREEK = DSA_TAG_PROTO_HELLCREEK_VALUE, DSA_TAG_PROTO_XRS700X = DSA_TAG_PROTO_XRS700X_VALUE, DSA_TAG_PROTO_OCELOT_8021Q = DSA_TAG_PROTO_OCELOT_8021Q_VALUE, + DSA_TAG_PROTO_SEVILLE = DSA_TAG_PROTO_SEVILLE_VALUE, }; struct packet_type; diff --git a/net/dsa/tag_ocelot.c b/net/dsa/tag_ocelot.c index fe00519229d7..a7dd61c8e005 100644 --- a/net/dsa/tag_ocelot.c +++ b/net/dsa/tag_ocelot.c @@ -24,33 +24,52 @@ static void ocelot_xmit_ptp(struct dsa_port *dp, void *injection, ocelot_ifh_set_rew_op(injection, rew_op); } -static struct sk_buff *ocelot_xmit(struct sk_buff *skb, - struct net_device *netdev) +static void ocelot_xmit_common(struct sk_buff *skb, struct net_device *netdev, + __be32 ifh_prefix, void **ifh) { struct dsa_port *dp = dsa_slave_to_port(netdev); struct sk_buff *clone = DSA_SKB_CB(skb)->clone; struct dsa_switch *ds = dp->ds; - struct ocelot *ocelot = ds->priv; - struct ocelot_port *ocelot_port; - u8 *prefix, *injection; - - ocelot_port = ocelot->ports[dp->index]; + void *injection; + __be32 *prefix; injection = skb_push(skb, OCELOT_TAG_LEN); - prefix = skb_push(skb, OCELOT_SHORT_PREFIX_LEN); - memcpy(prefix, ocelot_port->xmit_template, OCELOT_TOTAL_TAG_LEN); - - /* Fix up the fields which are not statically determined - * in the template - */ + *prefix = ifh_prefix; + memset(injection, 0, OCELOT_TAG_LEN); + ocelot_ifh_set_bypass(injection, 1); + ocelot_ifh_set_src(injection, ds->num_ports); ocelot_ifh_set_qos_class(injection, skb->priority); /* TX timestamping was requested */ if (clone) ocelot_xmit_ptp(dp, injection, clone); + *ifh = injection; +} + +static struct sk_buff *ocelot_xmit(struct sk_buff *skb, + struct net_device *netdev) +{ + struct dsa_port *dp = dsa_slave_to_port(netdev); + void *injection; + + ocelot_xmit_common(skb, netdev, cpu_to_be32(0x8880000a), &injection); + ocelot_ifh_set_dest(injection, BIT(dp->index)); + + return skb; +} + +static struct sk_buff *seville_xmit(struct sk_buff *skb, + struct net_device *netdev) +{ + struct dsa_port *dp = dsa_slave_to_port(netdev); + void *injection; + + ocelot_xmit_common(skb, netdev, cpu_to_be32(0x88800005), &injection); + seville_ifh_set_dest(injection, BIT(dp->index)); + return skb; } @@ -147,7 +166,26 @@ static const struct dsa_device_ops ocelot_netdev_ops = { .promisc_on_master = true, }; -MODULE_LICENSE("GPL v2"); +DSA_TAG_DRIVER(ocelot_netdev_ops); MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_OCELOT); -module_dsa_tag_driver(ocelot_netdev_ops); +static const struct dsa_device_ops seville_netdev_ops = { + .name = "seville", + .proto = DSA_TAG_PROTO_SEVILLE, + .xmit = seville_xmit, + .rcv = ocelot_rcv, + .overhead = OCELOT_TOTAL_TAG_LEN, + .promisc_on_master = true, +}; + +DSA_TAG_DRIVER(seville_netdev_ops); +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_SEVILLE); + +static struct dsa_tag_driver *ocelot_tag_driver_array[] = { + &DSA_TAG_DRIVER_NAME(ocelot_netdev_ops), + &DSA_TAG_DRIVER_NAME(seville_netdev_ops), +}; + +module_dsa_tag_drivers(ocelot_tag_driver_array); + +MODULE_LICENSE("GPL v2"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville 2021-02-13 0:14 ` [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville Vladimir Oltean 2021-02-15 13:00 ` Dan Carpenter @ 2021-02-15 13:00 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-15 13:00 UTC (permalink / raw) To: kbuild, Vladimir Oltean, David S . Miller, Jakub Kicinski, netdev Cc: lkp, kbuild-all, Andrew Lunn, Florian Fainelli, Vivien Didelot, Richard Cochran, Claudiu Manoil, Alexandre Belloni, Vladimir Oltean [-- Attachment #1: Type: text/plain, Size: 2773 bytes --] Hi Vladimir, url: https://github.com/0day-ci/linux/commits/Vladimir-Oltean/PTP-for-DSA-tag_ocelot_8021q/20210213-081857 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3c5a2fd042d0bfac71a2dfb99515723d318df47b config: i386-randconfig-m031-20210209 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: net/dsa/tag_ocelot.c:59 ocelot_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? net/dsa/tag_ocelot.c:71 seville_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? vim +59 net/dsa/tag_ocelot.c 9d88a16c0fc930 Vladimir Oltean 2021-02-13 52 static struct sk_buff *ocelot_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 53 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 54 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 55 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 56 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 57 9d88a16c0fc930 Vladimir Oltean 2021-02-13 58 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x8880000a), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @59 ocelot_ifh_set_dest(injection, BIT(dp->index)); db->index is less than db->num_ports which 32 or less but sometimes it comes from the device tree so who knows. The ocelot_ifh_set_dest() function takes a u64 though and that suggests that BIT() should be changed to BIT_ULL(). 9d88a16c0fc930 Vladimir Oltean 2021-02-13 60 9d88a16c0fc930 Vladimir Oltean 2021-02-13 61 return skb; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 62 } 9d88a16c0fc930 Vladimir Oltean 2021-02-13 63 9d88a16c0fc930 Vladimir Oltean 2021-02-13 64 static struct sk_buff *seville_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 65 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 66 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 67 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 68 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 69 9d88a16c0fc930 Vladimir Oltean 2021-02-13 70 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x88800005), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @71 seville_ifh_set_dest(injection, BIT(dp->index)); Same. 9d88a16c0fc930 Vladimir Oltean 2021-02-13 72 8dce89aa5f3274 Vladimir Oltean 2019-11-14 73 return skb; 8dce89aa5f3274 Vladimir Oltean 2019-11-14 74 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 42328 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville @ 2021-02-15 13:00 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-15 13:00 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2828 bytes --] Hi Vladimir, url: https://github.com/0day-ci/linux/commits/Vladimir-Oltean/PTP-for-DSA-tag_ocelot_8021q/20210213-081857 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3c5a2fd042d0bfac71a2dfb99515723d318df47b config: i386-randconfig-m031-20210209 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: net/dsa/tag_ocelot.c:59 ocelot_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? net/dsa/tag_ocelot.c:71 seville_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? vim +59 net/dsa/tag_ocelot.c 9d88a16c0fc930 Vladimir Oltean 2021-02-13 52 static struct sk_buff *ocelot_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 53 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 54 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 55 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 56 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 57 9d88a16c0fc930 Vladimir Oltean 2021-02-13 58 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x8880000a), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @59 ocelot_ifh_set_dest(injection, BIT(dp->index)); db->index is less than db->num_ports which 32 or less but sometimes it comes from the device tree so who knows. The ocelot_ifh_set_dest() function takes a u64 though and that suggests that BIT() should be changed to BIT_ULL(). 9d88a16c0fc930 Vladimir Oltean 2021-02-13 60 9d88a16c0fc930 Vladimir Oltean 2021-02-13 61 return skb; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 62 } 9d88a16c0fc930 Vladimir Oltean 2021-02-13 63 9d88a16c0fc930 Vladimir Oltean 2021-02-13 64 static struct sk_buff *seville_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 65 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 66 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 67 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 68 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 69 9d88a16c0fc930 Vladimir Oltean 2021-02-13 70 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x88800005), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @71 seville_ifh_set_dest(injection, BIT(dp->index)); Same. 9d88a16c0fc930 Vladimir Oltean 2021-02-13 72 8dce89aa5f3274 Vladimir Oltean 2019-11-14 73 return skb; 8dce89aa5f3274 Vladimir Oltean 2019-11-14 74 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 42328 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville @ 2021-02-15 13:00 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-15 13:00 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 2828 bytes --] Hi Vladimir, url: https://github.com/0day-ci/linux/commits/Vladimir-Oltean/PTP-for-DSA-tag_ocelot_8021q/20210213-081857 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3c5a2fd042d0bfac71a2dfb99515723d318df47b config: i386-randconfig-m031-20210209 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> smatch warnings: net/dsa/tag_ocelot.c:59 ocelot_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? net/dsa/tag_ocelot.c:71 seville_xmit() warn: should '(((1))) << (dp->index)' be a 64 bit type? vim +59 net/dsa/tag_ocelot.c 9d88a16c0fc930 Vladimir Oltean 2021-02-13 52 static struct sk_buff *ocelot_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 53 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 54 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 55 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 56 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 57 9d88a16c0fc930 Vladimir Oltean 2021-02-13 58 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x8880000a), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @59 ocelot_ifh_set_dest(injection, BIT(dp->index)); db->index is less than db->num_ports which 32 or less but sometimes it comes from the device tree so who knows. The ocelot_ifh_set_dest() function takes a u64 though and that suggests that BIT() should be changed to BIT_ULL(). 9d88a16c0fc930 Vladimir Oltean 2021-02-13 60 9d88a16c0fc930 Vladimir Oltean 2021-02-13 61 return skb; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 62 } 9d88a16c0fc930 Vladimir Oltean 2021-02-13 63 9d88a16c0fc930 Vladimir Oltean 2021-02-13 64 static struct sk_buff *seville_xmit(struct sk_buff *skb, 9d88a16c0fc930 Vladimir Oltean 2021-02-13 65 struct net_device *netdev) 9d88a16c0fc930 Vladimir Oltean 2021-02-13 66 { 9d88a16c0fc930 Vladimir Oltean 2021-02-13 67 struct dsa_port *dp = dsa_slave_to_port(netdev); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 68 void *injection; 9d88a16c0fc930 Vladimir Oltean 2021-02-13 69 9d88a16c0fc930 Vladimir Oltean 2021-02-13 70 ocelot_xmit_common(skb, netdev, cpu_to_be32(0x88800005), &injection); 9d88a16c0fc930 Vladimir Oltean 2021-02-13 @71 seville_ifh_set_dest(injection, BIT(dp->index)); Same. 9d88a16c0fc930 Vladimir Oltean 2021-02-13 72 8dce89aa5f3274 Vladimir Oltean 2019-11-14 73 return skb; 8dce89aa5f3274 Vladimir Oltean 2019-11-14 74 } --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 42328 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville 2021-02-15 13:00 ` Dan Carpenter (?) (?) @ 2021-02-15 13:19 ` Vladimir Oltean 2021-02-15 14:15 ` Dan Carpenter -1 siblings, 1 reply; 10+ messages in thread From: Vladimir Oltean @ 2021-02-15 13:19 UTC (permalink / raw) To: Dan Carpenter Cc: kbuild, David S . Miller, Jakub Kicinski, netdev, lkp, kbuild-all, Andrew Lunn, Florian Fainelli, Vivien Didelot, Richard Cochran, Claudiu Manoil, Alexandre Belloni, Vladimir Oltean Hi Dan, On Mon, Feb 15, 2021 at 04:00:04PM +0300, Dan Carpenter wrote: > db->index is less than db->num_ports which 32 or less but sometimes it > comes from the device tree so who knows. The destination port mask is copied into a 12-bit field of the packet, starting at bit offset 67 and ending at 56: static inline void ocelot_ifh_set_dest(void *injection, u64 dest) { packing(injection, &dest, 67, 56, OCELOT_TAG_LEN, PACK, 0); } So this DSA tagging protocol supports at most 12 bits, which is clearly less than 32. Attempting to send to a port number > 12 will cause the packing() call to truncate way before there will be 32-bit truncation due to type promotion of the BIT(port) argument towards u64. > The ocelot_ifh_set_dest() function takes a u64 though and that > suggests that BIT() should be changed to BIT_ULL(). I understand that you want to silence the warning, which fundamentally comes from the packing() API which works with u64 values and nothing of a smaller size. So I can send a patch which replaces BIT(port) with BIT_ULL(port), even if in practice both are equally fine. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville 2021-02-15 13:19 ` Vladimir Oltean 2021-02-15 14:15 ` Dan Carpenter @ 2021-02-15 14:15 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-15 14:15 UTC (permalink / raw) To: Vladimir Oltean Cc: kbuild, David S . Miller, Jakub Kicinski, netdev, lkp, kbuild-all, Andrew Lunn, Florian Fainelli, Vivien Didelot, Richard Cochran, Claudiu Manoil, Alexandre Belloni, Vladimir Oltean On Mon, Feb 15, 2021 at 03:19:31PM +0200, Vladimir Oltean wrote: > Hi Dan, > > On Mon, Feb 15, 2021 at 04:00:04PM +0300, Dan Carpenter wrote: > > db->index is less than db->num_ports which 32 or less but sometimes it > > comes from the device tree so who knows. > > The destination port mask is copied into a 12-bit field of the packet, > starting at bit offset 67 and ending at 56: > > static inline void ocelot_ifh_set_dest(void *injection, u64 dest) > { > packing(injection, &dest, 67, 56, OCELOT_TAG_LEN, PACK, 0); > } > > So this DSA tagging protocol supports at most 12 bits, which is clearly > less than 32. Attempting to send to a port number > 12 will cause the > packing() call to truncate way before there will be 32-bit truncation > due to type promotion of the BIT(port) argument towards u64. > > > The ocelot_ifh_set_dest() function takes a u64 though and that > > suggests that BIT() should be changed to BIT_ULL(). > > I understand that you want to silence the warning, which fundamentally > comes from the packing() API which works with u64 values and nothing of > a smaller size. So I can send a patch which replaces BIT(port) with > BIT_ULL(port), even if in practice both are equally fine. I don't have a strong feeling about this... Generally silencing warnings just to make a checker happy is the wrong idea. To be honest, I normally ignore these warnings. But I have been looking at them recently to try figure out if we could make it so it would only generate a warning where "db->index" was known as possibly being in the 32-63 range. So I looked at this one. And now I see some ways that Smatch could have parsed this better... regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville @ 2021-02-15 14:15 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-15 14:15 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 1731 bytes --] On Mon, Feb 15, 2021 at 03:19:31PM +0200, Vladimir Oltean wrote: > Hi Dan, > > On Mon, Feb 15, 2021 at 04:00:04PM +0300, Dan Carpenter wrote: > > db->index is less than db->num_ports which 32 or less but sometimes it > > comes from the device tree so who knows. > > The destination port mask is copied into a 12-bit field of the packet, > starting at bit offset 67 and ending at 56: > > static inline void ocelot_ifh_set_dest(void *injection, u64 dest) > { > packing(injection, &dest, 67, 56, OCELOT_TAG_LEN, PACK, 0); > } > > So this DSA tagging protocol supports at most 12 bits, which is clearly > less than 32. Attempting to send to a port number > 12 will cause the > packing() call to truncate way before there will be 32-bit truncation > due to type promotion of the BIT(port) argument towards u64. > > > The ocelot_ifh_set_dest() function takes a u64 though and that > > suggests that BIT() should be changed to BIT_ULL(). > > I understand that you want to silence the warning, which fundamentally > comes from the packing() API which works with u64 values and nothing of > a smaller size. So I can send a patch which replaces BIT(port) with > BIT_ULL(port), even if in practice both are equally fine. I don't have a strong feeling about this... Generally silencing warnings just to make a checker happy is the wrong idea. To be honest, I normally ignore these warnings. But I have been looking at them recently to try figure out if we could make it so it would only generate a warning where "db->index" was known as possibly being in the 32-63 range. So I looked at this one. And now I see some ways that Smatch could have parsed this better... regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville @ 2021-02-15 14:15 ` Dan Carpenter 0 siblings, 0 replies; 10+ messages in thread From: Dan Carpenter @ 2021-02-15 14:15 UTC (permalink / raw) To: kbuild [-- Attachment #1: Type: text/plain, Size: 1731 bytes --] On Mon, Feb 15, 2021 at 03:19:31PM +0200, Vladimir Oltean wrote: > Hi Dan, > > On Mon, Feb 15, 2021 at 04:00:04PM +0300, Dan Carpenter wrote: > > db->index is less than db->num_ports which 32 or less but sometimes it > > comes from the device tree so who knows. > > The destination port mask is copied into a 12-bit field of the packet, > starting at bit offset 67 and ending at 56: > > static inline void ocelot_ifh_set_dest(void *injection, u64 dest) > { > packing(injection, &dest, 67, 56, OCELOT_TAG_LEN, PACK, 0); > } > > So this DSA tagging protocol supports at most 12 bits, which is clearly > less than 32. Attempting to send to a port number > 12 will cause the > packing() call to truncate way before there will be 32-bit truncation > due to type promotion of the BIT(port) argument towards u64. > > > The ocelot_ifh_set_dest() function takes a u64 though and that > > suggests that BIT() should be changed to BIT_ULL(). > > I understand that you want to silence the warning, which fundamentally > comes from the packing() API which works with u64 values and nothing of > a smaller size. So I can send a patch which replaces BIT(port) with > BIT_ULL(port), even if in practice both are equally fine. I don't have a strong feeling about this... Generally silencing warnings just to make a checker happy is the wrong idea. To be honest, I normally ignore these warnings. But I have been looking at them recently to try figure out if we could make it so it would only generate a warning where "db->index" was known as possibly being in the 32-63 range. So I looked at this one. And now I see some ways that Smatch could have parsed this better... regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville 2021-02-15 14:15 ` Dan Carpenter (?) (?) @ 2021-02-15 14:49 ` Vladimir Oltean -1 siblings, 0 replies; 10+ messages in thread From: Vladimir Oltean @ 2021-02-15 14:49 UTC (permalink / raw) To: Dan Carpenter Cc: kbuild, David S . Miller, Jakub Kicinski, netdev, lkp, kbuild-all, Andrew Lunn, Florian Fainelli, Vivien Didelot, Richard Cochran, Claudiu Manoil, Alexandre Belloni, Vladimir Oltean On Mon, Feb 15, 2021 at 05:15:21PM +0300, Dan Carpenter wrote: > On Mon, Feb 15, 2021 at 03:19:31PM +0200, Vladimir Oltean wrote: > > Hi Dan, > > > > On Mon, Feb 15, 2021 at 04:00:04PM +0300, Dan Carpenter wrote: > > > db->index is less than db->num_ports which 32 or less but sometimes it > > > comes from the device tree so who knows. > > > > The destination port mask is copied into a 12-bit field of the packet, > > starting at bit offset 67 and ending at 56: > > > > static inline void ocelot_ifh_set_dest(void *injection, u64 dest) > > { > > packing(injection, &dest, 67, 56, OCELOT_TAG_LEN, PACK, 0); > > } > > > > So this DSA tagging protocol supports at most 12 bits, which is clearly > > less than 32. Attempting to send to a port number > 12 will cause the > > packing() call to truncate way before there will be 32-bit truncation > > due to type promotion of the BIT(port) argument towards u64. > > > > > The ocelot_ifh_set_dest() function takes a u64 though and that > > > suggests that BIT() should be changed to BIT_ULL(). > > > > I understand that you want to silence the warning, which fundamentally > > comes from the packing() API which works with u64 values and nothing of > > a smaller size. So I can send a patch which replaces BIT(port) with > > BIT_ULL(port), even if in practice both are equally fine. > > I don't have a strong feeling about this... Generally silencing > warnings just to make a checker happy is the wrong idea. In this case it is a harmless wrong idea. > To be honest, I normally ignore these warnings. But I have been looking > at them recently to try figure out if we could make it so it would only > generate a warning where "db->index" was known as possibly being in the > 32-63 range. So I looked at this one. > > And now I see some ways that Smatch could have parsed this better... For DSA, the dp->index should be lower than ds->num_ports by construction (see dsa_switch_touch_ports). In turn, ds->num_ports is set to constant values smaller than 12 in felix_pci_probe() and in seville_probe(). For ocelot on the other hand, there is a restriction put in mscc_ocelot_init_ports that the port must be <= ocelot->num_phys_ports, which is set to "of_get_child_count(ports)". So there is indeed a possible attack by device tree on the ocelot driver. The number of physical ports does not depend on device tree (arch/mips/boot/dts/mscc/ocelot.dtsi), but should be hardcoded to 11. How many ports there are defined in DT doesn't change how many physical ports there are. For example, the CPU port module is at index 11, and in the code it is referenced as ocelot->ports[ocelot->num_phys_ports]. If num_phys_ports has any other value than 11, the driver malfunctions because the index of the CPU port is misidentified. I'd rather fix this if there was some way in which static analysis could then determine that "port" is bounded by a constant smaller than the truncation threshold. However, I'm not sure how to classify the severity of this problem. There's a million of other ways in which the system can malfunction if it is being "attacked by device tree". ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-02-15 14:50 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-13 2:32 [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2021-02-13 0:14 [PATCH net-next 00/12] PTP for DSA tag_ocelot_8021q Vladimir Oltean 2021-02-13 0:14 ` [PATCH net-next 09/12] net: dsa: tag_ocelot: create separate tagger for Seville Vladimir Oltean 2021-02-15 13:00 ` Dan Carpenter 2021-02-15 13:00 ` Dan Carpenter 2021-02-15 13:00 ` Dan Carpenter 2021-02-15 13:19 ` Vladimir Oltean 2021-02-15 14:15 ` Dan Carpenter 2021-02-15 14:15 ` Dan Carpenter 2021-02-15 14:15 ` Dan Carpenter 2021-02-15 14:49 ` Vladimir Oltean
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.