1. 12 Jan, 2021 13 commits
    • Vladimir Oltean's avatar
      net: dsa: felix: the switch does not support DMA · 537e2b88
      Vladimir Oltean authored
      The code that sets the DMA mask to 64 bits is bogus, it is taken from
      the enetc driver together with the rest of the PCI probing boilerplate.
      
      Since this patch is touching the error path to delete err_dma, let's
      also change the err_alloc_felix label which was incorrect. The kzalloc
      failure does not need a kfree, but it doesn't hurt either, since kfree
      works with NULL pointers.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Link: https://lore.kernel.org/r/20210109203415.2120142-1-olteanv@gmail.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      537e2b88
    • Jakub Kicinski's avatar
      Merge branch 'get-rid-of-the-switchdev-transactional-model' · d1c8b6a3
      Jakub Kicinski authored
      Vladimir Oltean says:
      
      ====================
      Get rid of the switchdev transactional model
      
      Changes in v4:
      - Fixed build error in dsa_loop and build warning in hellcreek driver.
      - Scheduling the mlxsw SPAN work item regardless of the VLAN add return
        code, as per Ido's and Petr's request.
      
      Changes in v3:
      - Resolved a build warning in mv88e6xxx and tested that it actually
        works properly, which resulted in an extra patch (02/11).
      - Addressed Ido's minor feedback in commit 10/11 relating to a comment.
      
      Changes in v2:
      - Got rid of the vid_begin -> vid_end range too from the switchdev API.
      - Actually propagating errors from DSA MDB and VLAN notifiers.
      
      This series comes after the late realization that the prepare/commit
      separation imposed by switchdev does not help literally anybody:
      https://patchwork.kernel.org/project/netdevbpf/patch/20201212203901.351331-1-vladimir.oltean@nxp.com/
      
      We should kill it before it inflicts even more damage to the error
      handling logic in drivers.
      
      Also remove the unused VLAN ranges feature from the switchdev VLAN
      objects, which simplifies all drivers by quite a bit.
      ====================
      
      Link: https://lore.kernel.org/r/20210109000156.1246735-1-olteanv@gmail.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      d1c8b6a3
    • Vladimir Oltean's avatar
      net: switchdev: delete the transaction object · 8f73cc50
      Vladimir Oltean authored
      Now that all users of struct switchdev_trans have been modified to do
      without it, we can remove this structure and the two helpers to determine
      the phase.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Reviewed-by: default avatarIdo Schimmel <idosch@nvidia.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      8f73cc50
    • Vladimir Oltean's avatar
      mlxsw: spectrum_switchdev: remove transactional logic for VLAN objects · 4b400fea
      Vladimir Oltean authored
      As of commit 457e20d6 ("mlxsw: spectrum_switchdev: Avoid returning
      errors in commit phase"), the mlxsw driver performs the VLAN object
      offloading during the prepare phase. So conversion just seems to be a
      matter of removing the code that was running in the commit phase.
      
      Ido Schimmel explains that the reason why mlxsw_sp_span_respin is called
      unconditionally is because the bridge driver will ignore -EOPNOTSUPP and
      actually add the VLAN on the bridge device - see commit 9c86ce2c
      ("net: bridge: Notify about bridge VLANs") and commit ea472175
      ("mlxsw: spectrum_switchdev: Ignore bridge VLAN events"). Since the VLAN
      was successfully added on the bridge device, mlxsw_sp_span_respin_work()
      should be able to resolve the egress port for a packet that is mirrored
      to a gre tap and passes through the bridge device. Therefore keep the
      logic as it is.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Reviewed-by: default avatarIdo Schimmel <idosch@nvidia.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      4b400fea
    • Vladimir Oltean's avatar
      net: dsa: remove obsolete comments about switchdev transactions · 417b99bf
      Vladimir Oltean authored
      Now that all port object notifiers were converted to be non-transactional,
      we can remove the comments that say otherwise.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      417b99bf
    • Vladimir Oltean's avatar
      net: dsa: remove the transactional logic from VLAN objects · 1958d581
      Vladimir Oltean authored
      It should be the driver's business to logically separate its VLAN
      offloading into a preparation and a commit phase, and some drivers don't
      need / can't do this.
      
      So remove the transactional shim from DSA and let drivers propagate
      errors directly from the .port_vlan_add callback.
      
      It would appear that the code has worse error handling now than it had
      before. DSA is the only in-kernel user of switchdev that offloads one
      switchdev object to more than one port: for every VLAN object offloaded
      to a user port, that VLAN is also offloaded to the CPU port. So the
      "prepare for user port -> check for errors -> prepare for CPU port ->
      check for errors -> commit for user port -> commit for CPU port"
      sequence appears to make more sense than the one we are using now:
      "offload to user port -> check for errors -> offload to CPU port ->
      check for errors", but it is really a compromise. In the new way, we can
      catch errors from the commit phase that we previously had to ignore.
      But we have our hands tied and cannot do any rollback now: if we add a
      VLAN on the CPU port and it fails, we can't do the rollback by simply
      deleting it from the user port, because the switchdev API is not so nice
      with us: it could have simply been there already, even with the same
      flags. So we don't even attempt to rollback anything on addition error,
      just leave whatever VLANs managed to get offloaded right where they are.
      This should not be a problem at all in practice.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      1958d581
    • Vladimir Oltean's avatar
      net: dsa: remove the transactional logic from MDB entries · a52b2da7
      Vladimir Oltean authored
      For many drivers, the .port_mdb_prepare callback was not a good opportunity
      to avoid any error condition, and they would suppress errors found during
      the actual commit phase.
      
      Where a logical separation between the prepare and the commit phase
      existed, the function that used to implement the .port_mdb_prepare
      callback still exists, but now it is called directly from .port_mdb_add,
      which was modified to return an int code.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> # hellcreek
      Reviewed-by: Linus Wallei <linus.walleij@linaro.org> # RTL8366
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      a52b2da7
    • Vladimir Oltean's avatar
      net: dsa: remove the transactional logic from ageing time notifiers · 77b61365
      Vladimir Oltean authored
      Remove the shim introduced in DSA for offloading the bridge ageing time
      from switchdev, by first checking whether the ageing time is within the
      range limits requested by the driver.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      77b61365
    • Vladimir Oltean's avatar
      net: switchdev: remove the transaction structure from port attributes · bae33f2b
      Vladimir Oltean authored
      Since the introduction of the switchdev API, port attributes were
      transmitted to drivers for offloading using a two-step transactional
      model, with a prepare phase that was supposed to catch all errors, and a
      commit phase that was supposed to never fail.
      
      Some classes of failures can never be avoided, like hardware access, or
      memory allocation. In the latter case, merely attempting to move the
      memory allocation to the preparation phase makes it impossible to avoid
      memory leaks, since commit 91cf8ece ("switchdev: Remove unused
      transaction item queue") which has removed the unused mechanism of
      passing on the allocated memory between one phase and another.
      
      It is time we admit that separating the preparation from the commit
      phase is something that is best left for the driver to decide, and not
      something that should be baked into the API, especially since there are
      no switchdev callers that depend on this.
      
      This patch removes the struct switchdev_trans member from switchdev port
      attribute notifier structures, and converts drivers to not look at this
      member.
      
      In part, this patch contains a revert of my previous commit 2e554a7a
      ("net: dsa: propagate switchdev vlan_filtering prepare phase to
      drivers").
      
      For the most part, the conversion was trivial except for:
      - Rocker's world implementation based on Broadcom OF-DPA had an odd
        implementation of ofdpa_port_attr_bridge_flags_set. The conversion was
        done mechanically, by pasting the implementation twice, then only
        keeping the code that would get executed during prepare phase on top,
        then only keeping the code that gets executed during the commit phase
        on bottom, then simplifying the resulting code until this was obtained.
      - DSA's offloading of STP state, bridge flags, VLAN filtering and
        multicast router could be converted right away. But the ageing time
        could not, so a shim was introduced and this was left for a further
        commit.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> # hellcreek
      Reviewed-by: Linus Walleij <linus.walleij@linaro.org> # RTL8366RB
      Reviewed-by: default avatarIdo Schimmel <idosch@nvidia.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      bae33f2b
    • Vladimir Oltean's avatar
      net: switchdev: delete switchdev_port_obj_add_now · cf6def51
      Vladimir Oltean authored
      After the removal of the transactional model inside
      switchdev_port_obj_add_now, it has no added value and we can just call
      switchdev_port_obj_notify directly, bypassing this function. Let's
      delete it.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Reviewed-by: default avatarIdo Schimmel <idosch@nvidia.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      cf6def51
    • Vladimir Oltean's avatar
      net: switchdev: remove the transaction structure from port object notifiers · ffb68fc5
      Vladimir Oltean authored
      Since the introduction of the switchdev API, port objects were
      transmitted to drivers for offloading using a two-step transactional
      model, with a prepare phase that was supposed to catch all errors, and a
      commit phase that was supposed to never fail.
      
      Some classes of failures can never be avoided, like hardware access, or
      memory allocation. In the latter case, merely attempting to move the
      memory allocation to the preparation phase makes it impossible to avoid
      memory leaks, since commit 91cf8ece ("switchdev: Remove unused
      transaction item queue") which has removed the unused mechanism of
      passing on the allocated memory between one phase and another.
      
      It is time we admit that separating the preparation from the commit
      phase is something that is best left for the driver to decide, and not
      something that should be baked into the API, especially since there are
      no switchdev callers that depend on this.
      
      This patch removes the struct switchdev_trans member from switchdev port
      object notifier structures, and converts drivers to not look at this
      member.
      
      Where driver conversion is trivial (like in the case of the Marvell
      Prestera driver, NXP DPAA2 switch, TI CPSW, and Rocker drivers), it is
      done in this patch.
      
      Where driver conversion needs more attention (DSA, Mellanox Spectrum),
      the conversion is left for subsequent patches and here we only fake the
      prepare/commit phases at a lower level, just not in the switchdev
      notifier itself.
      
      Where the code has a natural structure that is best left alone as a
      preparation and a commit phase (as in the case of the Ocelot switch),
      that structure is left in place, just made to not depend upon the
      switchdev transactional model.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Acked-by: default avatarLinus Walleij <linus.walleij@linaro.org>
      Acked-by: default avatarJiri Pirko <jiri@nvidia.com>
      Reviewed-by: default avatarIdo Schimmel <idosch@nvidia.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      ffb68fc5
    • Vladimir Oltean's avatar
      net: dsa: mv88e6xxx: deny vid 0 on the CPU port and DSA links too · 3e85f580
      Vladimir Oltean authored
      mv88e6xxx apparently has a problem offloading VID 0, which the 8021q
      module tries to install as part of commit ad1afb00 ("vlan_dev: VLAN
      0 should be treated as "no vlan tag" (802.1p packet)"). That mv88e6xxx
      restriction seems to have been introduced by the "VTU GetNext VID-1
      trick to retrieve a single entry" - see commit 2fb5ef09 ("net: dsa:
      mv88e6xxx: extract single VLAN retrieval").
      
      There is one more problem. The mv88e6xxx CPU port and DSA links do not
      report properly in the prepare phase what are the VLANs that they can
      offload. They'll say they can offload everything:
      
      mv88e6xxx_port_vlan_prepare
      -> mv88e6xxx_port_check_hw_vlan:
      
      	/* DSA and CPU ports have to be members of multiple vlans */
      	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
      		return 0;
      
      Except that if you actually try to commit to it, they'll error out and
      print this message:
      
      [   32.802438] mv88e6085 d0032004.mdio-mii:12: p9: failed to add VLAN 0t
      
      which comes from:
      
      mv88e6xxx_port_vlan_add
      -> mv88e6xxx_port_vlan_join:
      
      	if (!vid)
      		return -EOPNOTSUPP;
      
      What prevents this condition from triggering in real life? The fact that
      when a DSA_NOTIFIER_VLAN_ADD is emitted, it never targets a DSA link
      directly. Instead, the notifier will always target either a user port or
      a CPU port. DSA links just happen to get dragged in by:
      
      static bool dsa_switch_vlan_match(struct dsa_switch *ds, int port,
      				  struct dsa_notifier_vlan_info *info)
      {
      	...
      	if (dsa_is_dsa_port(ds, port))
      		return true;
      	...
      }
      
      So for every DSA VLAN notifier, during the prepare phase, it will just
      so happen that there will be somebody to say "no, don't do that".
      
      This will become a problem when the switchdev prepare/commit transactional
      model goes away. Every port needs to think on its own. DSA links can no
      longer bluff and rely on the fact that the prepare phase will not go
      through to the end, because there will be no prepare phase any longer.
      
      Fix this issue before it becomes a problem, by having the "vid == 0"
      check earlier than the check whether we are a CPU port / DSA link or not.
      Also, the "vid == 0" check becomes unnecessary in the .port_vlan_add
      callback, so we can remove it.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      3e85f580
    • Vladimir Oltean's avatar
      net: switchdev: remove vid_begin -> vid_end range from VLAN objects · b7a9e0da
      Vladimir Oltean authored
      The call path of a switchdev VLAN addition to the bridge looks something
      like this today:
      
              nbp_vlan_init
              |  __br_vlan_set_default_pvid
              |  |                       |
              |  |    br_afspec          |
              |  |        |              |
              |  |        v              |
              |  | br_process_vlan_info  |
              |  |        |              |
              |  |        v              |
              |  |   br_vlan_info        |
              |  |       / \            /
              |  |      /   \          /
              |  |     /     \        /
              |  |    /       \      /
              v  v   v         v    v
            nbp_vlan_add   br_vlan_add ------+
             |              ^      ^ |       |
             |             /       | |       |
             |            /       /  /       |
             \ br_vlan_get_master/  /        v
              \        ^        /  /  br_vlan_add_existing
               \       |       /  /          |
                \      |      /  /          /
                 \     |     /  /          /
                  \    |    /  /          /
                   \   |   /  /          /
                    v  |   | v          /
                    __vlan_add         /
                       / |            /
                      /  |           /
                     v   |          /
         __vlan_vid_add  |         /
                     \   |        /
                      v  v        v
            br_switchdev_port_vlan_add
      
      The ranges UAPI was introduced to the bridge in commit bdced7ef
      ("bridge: support for multiple vlans and vlan ranges in setlink and
      dellink requests") (Jan 10 2015). But the VLAN ranges (parsed in br_afspec)
      have always been passed one by one, through struct bridge_vlan_info
      tmp_vinfo, to br_vlan_info. So the range never went too far in depth.
      
      Then Scott Feldman introduced the switchdev_port_bridge_setlink function
      in commit 47f8328b ("switchdev: add new switchdev bridge setlink").
      That marked the introduction of the SWITCHDEV_OBJ_PORT_VLAN, which made
      full use of the range. But switchdev_port_bridge_setlink was called like
      this:
      
      br_setlink
      -> br_afspec
      -> switchdev_port_bridge_setlink
      
      Basically, the switchdev and the bridge code were not tightly integrated.
      Then commit 41c498b9 ("bridge: restore br_setlink back to original")
      came, and switchdev drivers were required to implement
      .ndo_bridge_setlink = switchdev_port_bridge_setlink for a while.
      
      In the meantime, commits such as 0944d6b5 ("bridge: try switchdev op
      first in __vlan_vid_add/del") finally made switchdev penetrate the
      br_vlan_info() barrier and start to develop the call path we have today.
      But remember, br_vlan_info() still receives VLANs one by one.
      
      Then Arkadi Sharshevsky refactored the switchdev API in 2017 in commit
      29ab586c ("net: switchdev: Remove bridge bypass support from
      switchdev") so that drivers would not implement .ndo_bridge_setlink any
      longer. The switchdev_port_bridge_setlink also got deleted.
      This refactoring removed the parallel bridge_setlink implementation from
      switchdev, and left the only switchdev VLAN objects to be the ones
      offloaded from __vlan_vid_add (basically RX filtering) and  __vlan_add
      (the latter coming from commit 9c86ce2c ("net: bridge: Notify about
      bridge VLANs")).
      
      That is to say, today the switchdev VLAN object ranges are not used in
      the kernel. Refactoring the above call path is a bit complicated, when
      the bridge VLAN call path is already a bit complicated.
      
      Let's go off and finish the job of commit 29ab586c by deleting the
      bogus iteration through the VLAN ranges from the drivers. Some aspects
      of this feature never made too much sense in the first place. For
      example, what is a range of VLANs all having the BRIDGE_VLAN_INFO_PVID
      flag supposed to mean, when a port can obviously have a single pvid?
      This particular configuration _is_ denied as of commit 6623c60d
      ("bridge: vlan: enforce no pvid flag in vlan ranges"), but from an API
      perspective, the driver still has to play pretend, and only offload the
      vlan->vid_end as pvid. And the addition of a switchdev VLAN object can
      modify the flags of another, completely unrelated, switchdev VLAN
      object! (a VLAN that is PVID will invalidate the PVID flag from whatever
      other VLAN had previously been offloaded with switchdev and had that
      flag. Yet switchdev never notifies about that change, drivers are
      supposed to guess).
      
      Nonetheless, having a VLAN range in the API makes error handling look
      scarier than it really is - unwinding on errors and all of that.
      When in reality, no one really calls this API with more than one VLAN.
      It is all unnecessary complexity.
      
      And despite appearing pretentious (two-phase transactional model and
      all), the switchdev API is really sloppy because the VLAN addition and
      removal operations are not paired with one another (you can add a VLAN
      100 times and delete it just once). The bridge notifies through
      switchdev of a VLAN addition not only when the flags of an existing VLAN
      change, but also when nothing changes. There are switchdev drivers out
      there who don't like adding a VLAN that has already been added, and
      those checks don't really belong at driver level. But the fact that the
      API contains ranges is yet another factor that prevents this from being
      addressed in the future.
      
      Of the existing switchdev pieces of hardware, it appears that only
      Mellanox Spectrum supports offloading more than one VLAN at a time,
      through mlxsw_sp_port_vlan_set. I have kept that code internal to the
      driver, because there is some more bookkeeping that makes use of it, but
      I deleted it from the switchdev API. But since the switchdev support for
      ranges has already been de facto deleted by a Mellanox employee and
      nobody noticed for 4 years, I'm going to assume it's not a biggie.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: Ido Schimmel <idosch@nvidia.com> # switchdev and mlxsw
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> # hellcreek
      Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      b7a9e0da
  2. 11 Jan, 2021 1 commit
  3. 10 Jan, 2021 24 commits
  4. 09 Jan, 2021 2 commits