1. 19 Sep, 2021 17 commits
    • Krzysztof Kozlowski's avatar
      net: freescale: drop unneeded MODULE_ALIAS · fdb47583
      Krzysztof Kozlowski authored
      The MODULE_DEVICE_TABLE already creates proper alias for platform
      driver.  Having another MODULE_ALIAS causes the alias to be duplicated.
      Signed-off-by: default avatarKrzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      fdb47583
    • David S. Miller's avatar
      Merge branch 'ocelot-phylink-fixes' · d614489f
      David S. Miller authored
      Colin Foster says:
      
      ====================
      ocelot phylink fixes
      
      When the ocelot driver was migrated to phylink, e6e12df6 ("net:
      mscc: ocelot: convert to phylink") there were two additional writes to
      registers that became stale. One write was to DEV_CLOCK_CFG and one was
      to ANA_PFC_PCF_CFG.
      
      Both of these writes referenced the variable "speed" which originally
      was set to OCELOT_SPEED_{10,100,1000,2500}. These macros expand to
      values of 3, 2, 1, or 0, respectively. After the update, the variable
      speed is set to SPEED_{10,100,1000,2500} which expand to 10, 100, 1000,
      and 2500. So invalid values were getting written to the two registers,
      which would lead to either a lack of functionality or undefined
      funcationality.
      
      Fixing these values was the intent of v1 of this patch set - submitted
      as "[PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC
      speed"
      
      During that review it was determined that both writes were actually
      unnecessary. DEV_CLOCK_CFG is a duplicate write, so can be removed
      entirely. This was accidentally submitted as as a new, lone patch titled
      "[PATCH v1 net] net: mscc: ocelot: remove buggy duplicate write to
      DEV_CLOCK_CFG". This is part of what is considered v2 of this patch set.
      
      Additionally, the write to ANA_PFC_PFC_CFG is also unnecessary. Priority
      flow contol is disabled, so configuring it is useless and should be
      removed. This was also submitted as a new, lone patch titled "[PATCH v1
      net] net: mscc: ocelot: remove buggy and useless write to ANA_PFC_PFC_CFG".
      This is the rest of what is considered v2 of this patch set.
      
      v3
      Identical to v2, but fixes the patch numbering to v3 and submitting the
      two changes as a patch set.
      
      v2
      Note: I misunderstood and submitted two new "v1" patches instead of a
      single "v2" patch set.
      - Remove the buggy writes altogher
      ====================
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      d614489f
    • Colin Foster's avatar
      net: mscc: ocelot: remove buggy duplicate write to DEV_CLOCK_CFG · ba68e994
      Colin Foster authored
      When updating ocelot to use phylink, a second write to DEV_CLOCK_CFG was
      mistakenly left in. It used the variable "speed" which, previously, would
      would have been assigned a value of OCELOT_SPEED_1000. In phylink the
      variable is be SPEED_1000, which is invalid for the
      DEV_CLOCK_LINK_SPEED macro. Removing it as unnecessary and buggy.
      
      Fixes: e6e12df6 ("net: mscc: ocelot: convert to phylink")
      Signed-off-by: default avatarColin Foster <colin.foster@in-advantage.com>
      Reviewed-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      ba68e994
    • Colin Foster's avatar
      net: mscc: ocelot: remove buggy and useless write to ANA_PFC_PFC_CFG · 163957c4
      Colin Foster authored
      A useless write to ANA_PFC_PFC_CFG was left in while refactoring ocelot to
      phylink. Since priority flow control is disabled, writing the speed has no
      effect.
      
      Further, it was using ethtool.h SPEED_ instead of OCELOT_SPEED_ macros,
      which are incorrectly offset for GENMASK.
      
      Lastly, for priority flow control to properly function, some scenarios
      would rely on the rate adaptation from the PCS while the MAC speed would
      be fixed. So it isn't used, and even if it was, neither "speed" nor
      "mac_speed" are necessarily the correct values to be used.
      
      Fixes: e6e12df6 ("net: mscc: ocelot: convert to phylink")
      Signed-off-by: default avatarColin Foster <colin.foster@in-advantage.com>
      Reviewed-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      163957c4
    • Thomas Gleixner's avatar
      net: core: Correct the sock::sk_lock.owned lockdep annotations · 2dcb96ba
      Thomas Gleixner authored
      lock_sock_fast() and lock_sock_nested() contain lockdep annotations for the
      sock::sk_lock.owned 'mutex'. sock::sk_lock.owned is not a regular mutex. It
      is just lockdep wise equivalent. In fact it's an open coded trivial mutex
      implementation with some interesting features.
      
      sock::sk_lock.slock is a regular spinlock protecting the 'mutex'
      representation sock::sk_lock.owned which is a plain boolean. If 'owned' is
      true, then some other task holds the 'mutex', otherwise it is uncontended.
      As this locking construct is obviously endangered by lock ordering issues as
      any other locking primitive it got lockdep annotated via a dedicated
      dependency map sock::sk_lock.dep_map which has to be updated at the lock
      and unlock sites.
      
      lock_sock_nested() is a straight forward 'mutex' lock operation:
      
        might_sleep();
        spin_lock_bh(sock::sk_lock.slock)
        while (!try_lock(sock::sk_lock.owned)) {
            spin_unlock_bh(sock::sk_lock.slock);
            wait_for_release();
            spin_lock_bh(sock::sk_lock.slock);
        }
      
      The lockdep annotation for sock::sk_lock.owned is for unknown reasons
      _after_ the lock has been acquired, i.e. after the code block above and
      after releasing sock::sk_lock.slock, but inside the bottom halves disabled
      region:
      
        spin_unlock(sock::sk_lock.slock);
        mutex_acquire(&sk->sk_lock.dep_map, subclass, 0, _RET_IP_);
        local_bh_enable();
      
      The placement after the unlock is obvious because otherwise the
      mutex_acquire() would nest into the spin lock held region.
      
      But that's from the lockdep perspective still the wrong place:
      
       1) The mutex_acquire() is issued _after_ the successful acquisition which
          is pointless because in a dead lock scenario this point is never
          reached which means that if the deadlock is the first instance of
          exposing the wrong lock order lockdep does not have a chance to detect
          it.
      
       2) It only works because lockdep is rather lax on the context from which
          the mutex_acquire() is issued. Acquiring a mutex inside a bottom halves
          and therefore non-preemptible region is obviously invalid, except for a
          trylock which is clearly not the case here.
      
          This 'works' stops working on RT enabled kernels where the bottom halves
          serialization is done via a local lock, which exposes this misplacement
          because the 'mutex' and the local lock nest the wrong way around and
          lockdep complains rightfully about a lock inversion.
      
      The placement is wrong since the initial commit a5b5bb9a ("[PATCH]
      lockdep: annotate sk_locks") which introduced this.
      
      Fix it by moving the mutex_acquire() in front of the actual lock
      acquisition, which is what the regular mutex_lock() operation does as well.
      
      lock_sock_fast() is not that straight forward. It looks at the first glance
      like a convoluted trylock operation:
      
        spin_lock_bh(sock::sk_lock.slock)
        if (!sock::sk_lock.owned)
            return false;
        while (!try_lock(sock::sk_lock.owned)) {
            spin_unlock_bh(sock::sk_lock.slock);
            wait_for_release();
            spin_lock_bh(sock::sk_lock.slock);
        }
        spin_unlock(sock::sk_lock.slock);
        mutex_acquire(&sk->sk_lock.dep_map, subclass, 0, _RET_IP_);
        local_bh_enable();
        return true;
      
      But that's not the case: lock_sock_fast() is an interesting optimization
      for short critical sections which can run with bottom halves disabled and
      sock::sk_lock.slock held. This allows to shortcut the 'mutex' operation in
      the non contended case by preventing other lockers to acquire
      sock::sk_lock.owned because they are blocked on sock::sk_lock.slock, which
      in turn avoids the overhead of doing the heavy processing in release_sock()
      including waking up wait queue waiters.
      
      In the contended case, i.e. when sock::sk_lock.owned == true the behavior
      is the same as lock_sock_nested().
      
      Semantically this shortcut means, that the task acquired the 'mutex' even
      if it does not touch the sock::sk_lock.owned field in the non-contended
      case. Not telling lockdep about this shortcut acquisition is hiding
      potential lock ordering violations in the fast path.
      
      As a consequence the same reasoning as for the above lock_sock_nested()
      case vs. the placement of the lockdep annotation applies.
      
      The current placement of the lockdep annotation was just copied from
      the original lock_sock(), now renamed to lock_sock_nested(),
      implementation.
      
      Fix this by moving the mutex_acquire() in front of the actual lock
      acquisition and adding the corresponding mutex_release() into
      unlock_sock_fast(). Also document the fast path return case with a comment.
      Reported-by: default avatarSebastian Siewior <bigeasy@linutronix.de>
      Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
      Cc: netdev@vger.kernel.org
      Cc: "David S. Miller" <davem@davemloft.net>
      Cc: Jakub Kicinski <kuba@kernel.org>
      Cc: Eric Dumazet <edumazet@google.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      2dcb96ba
    • Alejandro Concepcion-Rodriguez's avatar
      docs: net: dsa: sja1105: fix reference to sja1105.txt · 48e6d083
      Alejandro Concepcion-Rodriguez authored
      The file sja1105.txt was converted to nxp,sja1105.yaml.
      Signed-off-by: default avatarAlejandro Concepcion-Rodriguez <asconcepcion@acoro.eu>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      48e6d083
    • Randy Dunlap's avatar
      igc: fix build errors for PTP · 87758511
      Randy Dunlap authored
      When IGC=y and PTP_1588_CLOCK=m, the ptp_*() interface family is
      not available to the igc driver. Make this driver depend on
      PTP_1588_CLOCK_OPTIONAL so that it will build without errors.
      
      Various igc commits have used ptp_*() functions without checking
      that PTP_1588_CLOCK is enabled. Fix all of these here.
      
      Fixes these build errors:
      
      ld: drivers/net/ethernet/intel/igc/igc_main.o: in function `igc_msix_other':
      igc_main.c:(.text+0x6494): undefined reference to `ptp_clock_event'
      ld: igc_main.c:(.text+0x64ef): undefined reference to `ptp_clock_event'
      ld: igc_main.c:(.text+0x6559): undefined reference to `ptp_clock_event'
      ld: drivers/net/ethernet/intel/igc/igc_ethtool.o: in function `igc_ethtool_get_ts_info':
      igc_ethtool.c:(.text+0xc7a): undefined reference to `ptp_clock_index'
      ld: drivers/net/ethernet/intel/igc/igc_ptp.o: in function `igc_ptp_feature_enable_i225':
      igc_ptp.c:(.text+0x330): undefined reference to `ptp_find_pin'
      ld: igc_ptp.c:(.text+0x36f): undefined reference to `ptp_find_pin'
      ld: drivers/net/ethernet/intel/igc/igc_ptp.o: in function `igc_ptp_init':
      igc_ptp.c:(.text+0x11cd): undefined reference to `ptp_clock_register'
      ld: drivers/net/ethernet/intel/igc/igc_ptp.o: in function `igc_ptp_stop':
      igc_ptp.c:(.text+0x12dd): undefined reference to `ptp_clock_unregister'
      ld: drivers/platform/x86/dell/dell-wmi-privacy.o: in function `dell_privacy_wmi_probe':
      
      Fixes: 64433e5b ("igc: Enable internal i225 PPS")
      Fixes: 60dbede0 ("igc: Add support for ethtool GET_TS_INFO command")
      Fixes: 87938851 ("igc: enable auxiliary PHC functions for the i225")
      Fixes: 5f295805 ("igc: Add basic skeleton for PTP")
      Signed-off-by: default avatarRandy Dunlap <rdunlap@infradead.org>
      Cc: Ederson de Souza <ederson.desouza@intel.com>
      Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
      Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
      Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
      Cc: "David S. Miller" <davem@davemloft.net>
      Cc: Jakub Kicinski <kuba@kernel.org>
      Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
      Cc: intel-wired-lan@lists.osuosl.org
      Acked-by: default avatarVinicius Costa Gomes <vinicius.gomes@intel.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      87758511
    • Claudiu Manoil's avatar
      enetc: Fix uninitialized struct dim_sample field usage · 9f7afa05
      Claudiu Manoil authored
      The only struct dim_sample member that does not get
      initialized by dim_update_sample() is comp_ctr. (There
      is special API to initialize comp_ctr:
      dim_update_sample_with_comps(), and it is currently used
      only for RDMA.) comp_ctr is used to compute curr_stats->cmps
      and curr_stats->cpe_ratio (see dim_calc_stats()) which in
      turn are consumed by the rdma_dim_*() API.  Therefore,
      functionally, the net_dim*() API consumers are not affected.
      Nevertheless, fix the computation of statistics based
      on an uninitialized variable, even if the mentioned statistics
      are not used at the moment.
      
      Fixes: ae0e6a5d ("enetc: Add adaptive interrupt coalescing")
      Signed-off-by: default avatarClaudiu Manoil <claudiu.manoil@nxp.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      9f7afa05
    • Claudiu Manoil's avatar
      enetc: Fix illegal access when reading affinity_hint · 7237a494
      Claudiu Manoil authored
      irq_set_affinity_hit() stores a reference to the cpumask_t
      parameter in the irq descriptor, and that reference can be
      accessed later from irq_affinity_hint_proc_show(). Since
      the cpu_mask parameter passed to irq_set_affinity_hit() has
      only temporary storage (it's on the stack memory), later
      accesses to it are illegal. Thus reads from the corresponding
      procfs affinity_hint file can result in paging request oops.
      
      The issue is fixed by the get_cpu_mask() helper, which provides
      a permanent storage for the cpumask_t parameter.
      
      Fixes: d4fd0404 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
      Signed-off-by: default avatarClaudiu Manoil <claudiu.manoil@nxp.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      7237a494
    • Jason Wang's avatar
      virtio-net: fix pages leaking when building skb in big mode · afd92d82
      Jason Wang authored
      We try to use build_skb() if we had sufficient tailroom. But we forget
      to release the unused pages chained via private in big mode which will
      leak pages. Fixing this by release the pages after building the skb in
      big mode.
      
      Cc: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
      Fixes: fb32856b ("virtio-net: page_to_skb() use build_skb when there's sufficient tailroom")
      Signed-off-by: default avatarJason Wang <jasowang@redhat.com>
      Reviewed-by: default avatarXuan Zhuo <xuanzhuo@linux.alibaba.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      afd92d82
    • Jan Beulich's avatar
      xen-netback: correct success/error reporting for the SKB-with-fraglist case · 3ede7f84
      Jan Beulich authored
      When re-entering the main loop of xenvif_tx_check_gop() a 2nd time, the
      special considerations for the head of the SKB no longer apply. Don't
      mistakenly report ERROR to the frontend for the first entry in the list,
      even if - from all I can tell - this shouldn't matter much as the overall
      transmit will need to be considered failed anyway.
      Signed-off-by: default avatarJan Beulich <jbeulich@suse.com>
      Reviewed-by: default avatarPaul Durrant <paul@xen.org>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      3ede7f84
    • David S. Miller's avatar
      Merge branch 'dsa-shutdown' · 564df7ab
      David S. Miller authored
      Vladimir Oltean says:
      
      ====================
      Make DSA switch drivers compatible with masters which unregister on shutdown
      
      Changes in v2:
      - fix build for b53_mmap
      - use unregister_netdevice_many
      
      It was reported by Lino here:
      
      https://lore.kernel.org/netdev/20210909095324.12978-1-LinoSanfilippo@gmx.de/
      
      that when the DSA master attempts to unregister its net_device on
      shutdown, DSA should prevent that operation from succeeding because it
      holds a reference to it. This hangs the shutdown process.
      
      This issue was essentially introduced in commit 2f1e8ea7 ("net: dsa:
      link interfaces with the DSA master to get rid of lockdep warnings").
      The present series patches all DSA drivers to handle that case,
      depending on whether those drivers were introduced before or after the
      offending commit, a different Fixes: tag is specified for them.
      
      The approach taken by this series solves the issue in essentially the
      same way as Lino's patches, except for three key differences:
      
      - this series takes a more minimal approach in what is done on shutdown,
        we do not attempt a full tree teardown as that is not strictly
        necessary. I might revisit this if there are compelling reasons to do
        otherwise
      
      - this series fixes the issues for all DSA drivers, not just KSZ9897
      
      - this series works even if the ->remove driver method gets called for
        the same device too, not just ->shutdown. This is really possible to
        happen for SPI device drivers, and potentially possible for other bus
        device drivers too.
      ====================
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      564df7ab
    • Vladimir Oltean's avatar
      net: dsa: xrs700x: be compatible with masters which unregister on shutdown · a68e9da4
      Vladimir Oltean authored
      Since commit 2f1e8ea7 ("net: dsa: link interfaces with the DSA
      master to get rid of lockdep warnings"), DSA gained a requirement which
      it did not fulfill, which is to unlink itself from the DSA master at
      shutdown time.
      
      Since the Arrow SpeedChips XRS700x driver was introduced after the bad
      commit, it has never worked with DSA masters which decide to unregister
      their net_device on shutdown, effectively hanging the reboot process.
      To fix that, we need to call dsa_switch_shutdown.
      
      These devices can be connected by I2C or by MDIO, and if I search for
      I2C or MDIO bus drivers that implement their ->shutdown by redirecting
      it to ->remove I don't see any, however this does not mean it would not
      be possible. To be compatible with that pattern, it is necessary to
      implement an "if this then not that" scheme, to avoid ->remove and
      ->shutdown from being called both for the same struct device.
      
      Fixes: ee00b24f ("net: dsa: add Arrow SpeedChips XRS700x driver")
      Link: https://lore.kernel.org/netdev/20210909095324.12978-1-LinoSanfilippo@gmx.de/Reported-by: default avatarLino Sanfilippo <LinoSanfilippo@gmx.de>
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarGeorge McCollister <george.mccollister@gmail.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      a68e9da4
    • Vladimir Oltean's avatar
      net: dsa: microchip: ksz8863: be compatible with masters which unregister on shutdown · fe405307
      Vladimir Oltean authored
      Since commit 2f1e8ea7 ("net: dsa: link interfaces with the DSA
      master to get rid of lockdep warnings"), DSA gained a requirement which
      it did not fulfill, which is to unlink itself from the DSA master at
      shutdown time.
      
      Since the Microchip sub-driver for KSZ8863 was introduced after the bad
      commit, it has never worked with DSA masters which decide to unregister
      their net_device on shutdown, effectively hanging the reboot process.
      To fix that, we need to call dsa_switch_shutdown.
      
      Since this driver expects the MDIO bus to be backed by mdio_bitbang, I
      don't think there is currently any MDIO bus driver which implements its
      ->shutdown by redirecting it to ->remove, but in any case, to be
      compatible with that pattern, it is necessary to implement an "if this
      then not that" scheme, to avoid ->remove and ->shutdown from being
      called both for the same struct device.
      
      Fixes: 60a36476 ("net: dsa: microchip: Add Microchip KSZ8863 SMI based driver support")
      Link: https://lore.kernel.org/netdev/20210909095324.12978-1-LinoSanfilippo@gmx.de/Reported-by: default avatarLino Sanfilippo <LinoSanfilippo@gmx.de>
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      fe405307
    • Vladimir Oltean's avatar
      net: dsa: hellcreek: be compatible with masters which unregister on shutdown · 46baae56
      Vladimir Oltean authored
      Since commit 2f1e8ea7 ("net: dsa: link interfaces with the DSA
      master to get rid of lockdep warnings"), DSA gained a requirement which
      it did not fulfill, which is to unlink itself from the DSA master at
      shutdown time.
      
      Since the hellcreek driver was introduced after the bad commit, it has
      never worked with DSA masters which decide to unregister their
      net_device on shutdown, effectively hanging the reboot process.
      
      Hellcreek is a platform device driver, so we probably cannot have the
      oddities of ->shutdown and ->remove getting both called for the exact
      same struct device. But to be in line with the pattern from the other
      device drivers which are on slow buses, implement the same "if this then
      not that" pattern of either running the ->shutdown or the ->remove hook.
      The driver's current ->remove implementation makes that very easy
      because it already zeroes out its device_drvdata on ->remove.
      
      Fixes: e4b27ebc ("net: dsa: Add DSA driver for Hirschmann Hellcreek switches")
      Link: https://lore.kernel.org/netdev/20210909095324.12978-1-LinoSanfilippo@gmx.de/Reported-by: default avatarLino Sanfilippo <LinoSanfilippo@gmx.de>
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Acked-by: Kurt Kanzenbach's avatarKurt Kanzenbach <kurt@linutronix.de>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      46baae56
    • Vladimir Oltean's avatar
      net: dsa: be compatible with masters which unregister on shutdown · 0650bf52
      Vladimir Oltean authored
      Lino reports that on his system with bcmgenet as DSA master and KSZ9897
      as a switch, rebooting or shutting down never works properly.
      
      What does the bcmgenet driver have special to trigger this, that other
      DSA masters do not? It has an implementation of ->shutdown which simply
      calls its ->remove implementation. Otherwise said, it unregisters its
      network interface on shutdown.
      
      This message can be seen in a loop, and it hangs the reboot process there:
      
      unregister_netdevice: waiting for eth0 to become free. Usage count = 3
      
      So why 3?
      
      A usage count of 1 is normal for a registered network interface, and any
      virtual interface which links itself as an upper of that will increment
      it via dev_hold. In the case of DSA, this is the call path:
      
      dsa_slave_create
      -> netdev_upper_dev_link
         -> __netdev_upper_dev_link
            -> __netdev_adjacent_dev_insert
               -> dev_hold
      
      So a DSA switch with 3 interfaces will result in a usage count elevated
      by two, and netdev_wait_allrefs will wait until they have gone away.
      
      Other stacked interfaces, like VLAN, watch NETDEV_UNREGISTER events and
      delete themselves, but DSA cannot just vanish and go poof, at most it
      can unbind itself from the switch devices, but that must happen strictly
      earlier compared to when the DSA master unregisters its net_device, so
      reacting on the NETDEV_UNREGISTER event is way too late.
      
      It seems that it is a pretty established pattern to have a driver's
      ->shutdown hook redirect to its ->remove hook, so the same code is
      executed regardless of whether the driver is unbound from the device, or
      the system is just shutting down. As Florian puts it, it is quite a big
      hammer for bcmgenet to unregister its net_device during shutdown, but
      having a common code path with the driver unbind helps ensure it is well
      tested.
      
      So DSA, for better or for worse, has to live with that and engage in an
      arms race of implementing the ->shutdown hook too, from all individual
      drivers, and do something sane when paired with masters that unregister
      their net_device there. The only sane thing to do, of course, is to
      unlink from the master.
      
      However, complications arise really quickly.
      
      The pattern of redirecting ->shutdown to ->remove is not unique to
      bcmgenet or even to net_device drivers. In fact, SPI controllers do it
      too (see dspi_shutdown -> dspi_remove), and presumably, I2C controllers
      and MDIO controllers do it too (this is something I have not researched
      too deeply, but even if this is not the case today, it is certainly
      plausible to happen in the future, and must be taken into consideration).
      
      Since DSA switches might be SPI devices, I2C devices, MDIO devices, the
      insane implication is that for the exact same DSA switch device, we
      might have both ->shutdown and ->remove getting called.
      
      So we need to do something with that insane environment. The pattern
      I've come up with is "if this, then not that", so if either ->shutdown
      or ->remove gets called, we set the device's drvdata to NULL, and in the
      other hook, we check whether the drvdata is NULL and just do nothing.
      This is probably not necessary for platform devices, just for devices on
      buses, but I would really insist for consistency among drivers, because
      when code is copy-pasted, it is not always copy-pasted from the best
      sources.
      
      So depending on whether the DSA switch's ->remove or ->shutdown will get
      called first, we cannot really guarantee even for the same driver if
      rebooting will result in the same code path on all platforms. But
      nonetheless, we need to do something minimally reasonable on ->shutdown
      too to fix the bug. Of course, the ->remove will do more (a full
      teardown of the tree, with all data structures freed, and this is why
      the bug was not caught for so long). The new ->shutdown method is kept
      separate from dsa_unregister_switch not because we couldn't have
      unregistered the switch, but simply in the interest of doing something
      quick and to the point.
      
      The big question is: does the DSA switch's ->shutdown get called earlier
      than the DSA master's ->shutdown? If not, there is still a risk that we
      might still trigger the WARN_ON in unregister_netdevice that says we are
      attempting to unregister a net_device which has uppers. That's no good.
      Although the reference to the master net_device won't physically go away
      even if DSA's ->shutdown comes afterwards, remember we have a dev_hold
      on it.
      
      The answer to that question lies in this comment above device_link_add:
      
       * A side effect of the link creation is re-ordering of dpm_list and the
       * devices_kset list by moving the consumer device and all devices depending
       * on it to the ends of these lists (that does not happen to devices that have
       * not been registered when this function is called).
      
      so the fact that DSA uses device_link_add towards its master is not
      exactly for nothing. device_shutdown() walks devices_kset from the back,
      so this is our guarantee that DSA's shutdown happens before the master's
      shutdown.
      
      Fixes: 2f1e8ea7 ("net: dsa: link interfaces with the DSA master to get rid of lockdep warnings")
      Link: https://lore.kernel.org/netdev/20210909095324.12978-1-LinoSanfilippo@gmx.de/Reported-by: default avatarLino Sanfilippo <LinoSanfilippo@gmx.de>
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Tested-by: default avatarAndrew Lunn <andrew@lunn.ch>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      0650bf52
    • Vladimir Oltean's avatar
      net: mdio: introduce a shutdown method to mdio device drivers · cf957997
      Vladimir Oltean authored
      MDIO-attached devices might have interrupts and other things that might
      need quiesced when we kexec into a new kernel. Things are even more
      creepy when those interrupt lines are shared, and in that case it is
      absolutely mandatory to disable all interrupt sources.
      
      Moreover, MDIO devices might be DSA switches, and DSA needs its own
      shutdown method to unlink from the DSA master, which is a new
      requirement that appeared after commit 2f1e8ea7 ("net: dsa: link
      interfaces with the DSA master to get rid of lockdep warnings").
      
      So introduce a ->shutdown method in the MDIO device driver structure.
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarAndrew Lunn <andrew@lunn.ch>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      cf957997
  2. 17 Sep, 2021 3 commits
  3. 16 Sep, 2021 16 commits
    • Linus Torvalds's avatar
      Merge tag 'net-5.15-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net · fc0c0548
      Linus Torvalds authored
      Pull networking fixes from Jakub Kicinski:
       "Including fixes from bpf.
      
        Current release - regressions:
      
         - vhost_net: fix OoB on sendmsg() failure
      
         - mlx5: bridge, fix uninitialized variable usage
      
         - bnxt_en: fix error recovery regression
      
        Current release - new code bugs:
      
         - bpf, mm: fix lockdep warning triggered by stack_map_get_build_id_offset()
      
        Previous releases - regressions:
      
         - r6040: restore MDIO clock frequency after MAC reset
      
         - tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()
      
         - dsa: flush switchdev workqueue before tearing down CPU/DSA ports
      
        Previous releases - always broken:
      
         - ptp: dp83640: don't define PAGE0, avoid compiler warning
      
         - igc: fix tunnel segmentation offloads
      
         - phylink: update SFP selected interface on advertising changes
      
         - stmmac: fix system hang caused by eee_ctrl_timer during suspend/resume
      
         - mlx5e: fix mutual exclusion between CQE compression and HW TS
      
        Misc:
      
         - bpf, cgroups: fix cgroup v2 fallback on v1/v2 mixed mode
      
         - sfc: fallback for lack of xdp tx queues
      
         - hns3: add option to turn off page pool feature"
      
      * tag 'net-5.15-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net: (67 commits)
        mlxbf_gige: clear valid_polarity upon open
        igc: fix tunnel offloading
        net/{mlx5|nfp|bnxt}: Remove unnecessary RTNL lock assert
        net: wan: wanxl: define CROSS_COMPILE_M68K
        selftests: nci: replace unsigned int with int
        net: dsa: flush switchdev workqueue before tearing down CPU/DSA ports
        Revert "net: phy: Uniform PHY driver access"
        net: dsa: destroy the phylink instance on any error in dsa_slave_phy_setup
        ptp: dp83640: don't define PAGE0
        bnx2x: Fix enabling network interfaces without VFs
        Revert "Revert "ipv4: fix memory leaks in ip_cmsg_send() callers""
        tcp: fix tp->undo_retrans accounting in tcp_sacktag_one()
        net-caif: avoid user-triggerable WARN_ON(1)
        bpf, selftests: Add test case for mixed cgroup v1/v2
        bpf, selftests: Add cgroup v1 net_cls classid helpers
        bpf, cgroups: Fix cgroup v2 fallback on v1/v2 mixed mode
        bpf: Add oversize check before call kvcalloc()
        net: hns3: fix the timing issue of VF clearing interrupt sources
        net: hns3: fix the exception when query imp info
        net: hns3: disable mac in flr process
        ...
      fc0c0548
    • Guenter Roeck's avatar
      net: 6pack: Fix tx timeout and slot time · 3c0d2a46
      Guenter Roeck authored
      tx timeout and slot time are currently specified in units of HZ.  On
      Alpha, HZ is defined as 1024.  When building alpha:allmodconfig, this
      results in the following error message.
      
        drivers/net/hamradio/6pack.c: In function 'sixpack_open':
        drivers/net/hamradio/6pack.c:71:41: error:
        	unsigned conversion from 'int' to 'unsigned char'
        	changes value from '256' to '0'
      
      In the 6PACK protocol, tx timeout is specified in units of 10 ms and
      transmitted over the wire:
      
          https://www.linux-ax25.org/wiki/6PACK
      
      Defining a value dependent on HZ doesn't really make sense, and
      presumably comes from the (very historical) situation where HZ was
      originally 100.
      
      Note that the SIXP_SLOTTIME use explicitly is about 10ms granularity:
      
              mod_timer(&sp->tx_t, jiffies + ((when + 1) * HZ) / 100);
      
      and the SIXP_TXDELAY walue is sent as a byte over the wire.
      Signed-off-by: default avatarGuenter Roeck <linux@roeck-us.net>
      Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      3c0d2a46
    • Arnd Bergmann's avatar
      drm/rockchip: cdn-dp-core: Make cdn_dp_core_resume __maybe_unused · 040b8907
      Arnd Bergmann authored
      With the new static annotation, the compiler warns when the functions
      are actually unused:
      
         drivers/gpu/drm/rockchip/cdn-dp-core.c:1123:12: error: 'cdn_dp_resume' defined but not used [-Werror=unused-function]
          1123 | static int cdn_dp_resume(struct device *dev)
               |            ^~~~~~~~~~~~~
      
      Mark them __maybe_unused to suppress that warning as well.
      
      [ Not so 'new' static annotations any more, and I removed the part of
        the patch that added __maybe_unused to cdn_dp_suspend(), because it's
        used by the shutdown/remove code.
      
        So only the resume function ends up possibly unused if CONFIG_PM isn't
        set     - Linus ]
      
      Fixes: 7c49abb4 ("drm/rockchip: cdn-dp-core: Make cdn_dp_core_suspend/resume static")
      Signed-off-by: default avatarArnd Bergmann <arnd@arndb.de>
      Reviewed-by: default avatarEnric Balletbo i Serra <enric.balletbo@collabora.com>
      Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      040b8907
    • Guenter Roeck's avatar
      cpufreq: vexpress: Drop unused variable · b60cee5b
      Guenter Roeck authored
      arm:allmodconfig fails to build with the following error.
      
        drivers/cpufreq/vexpress-spc-cpufreq.c:454:13: error:
      					unused variable 'cur_cluster'
      
      Remove the unused variable.
      
      Fixes: bb8c26d9 ("cpufreq: vexpress: Set CPUFREQ_IS_COOLING_DEV flag")
      Cc: Viresh Kumar <viresh.kumar@linaro.org>
      Signed-off-by: default avatarGuenter Roeck <linux@roeck-us.net>
      Reviewed-by: default avatarKees Cook <keescook@chromium.org>
      Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      b60cee5b
    • Guenter Roeck's avatar
      alpha: Declare virt_to_phys and virt_to_bus parameter as pointer to volatile · 35a3f4ef
      Guenter Roeck authored
      Some drivers pass a pointer to volatile data to virt_to_bus() and
      virt_to_phys(), and that works fine.  One exception is alpha.  This
      results in a number of compile errors such as
      
        drivers/net/wan/lmc/lmc_main.c: In function 'lmc_softreset':
        drivers/net/wan/lmc/lmc_main.c:1782:50: error:
      	passing argument 1 of 'virt_to_bus' discards 'volatile'
      	qualifier from pointer target type
      
        drivers/atm/ambassador.c: In function 'do_loader_command':
        drivers/atm/ambassador.c:1747:58: error:
      	passing argument 1 of 'virt_to_bus' discards 'volatile'
      	qualifier from pointer target type
      
      Declare the parameter of virt_to_phys and virt_to_bus as pointer to
      volatile to fix the problem.
      Signed-off-by: default avatarGuenter Roeck <linux@roeck-us.net>
      Acked-by: default avatarArnd Bergmann <arnd@arndb.de>
      Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      35a3f4ef
    • Linus Torvalds's avatar
      3com 3c515: make it compile on 64-bit architectures · db71f8fb
      Linus Torvalds authored
      This driver isn't enabled most places because of the ISA config
      dependency, but alpha still has it.  And I think the 'Jensen' actually
      did have an ISA slot.
      
      However, it doesn't build cleanly, because the "Vortex bus master" code
      just casts the skb->data pointer to 'int':
      
              outl((int) (skb->data), ioaddr + Wn7_MasterAddr);
      
      which is all kinds of broken.  Even on a good old traditional PC/AT it
      would be broken because the high bits will be random kernel address
      bits, but presumably the hardware ignores those bits.  I mean, it's ISA.
      We're talking 16MB dma limits. The "good old days".
      
      Make the build happy with this kind of craziness by using the proper
      isa_virt_to_bus() handling that the full bus master code uses anyway
      (the Vortex bus mastering is a limited special case).
      
      Who knows, this might even work.
      Reported-by: default avatarGuenter Roeck <linux@roeck-us.net>
      Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      db71f8fb
    • Linus Torvalds's avatar
      Merge tag 'for-5.15/parisc-4' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux · 5fe983d3
      Linus Torvalds authored
      Pull parisc fix from Helge Deller:
       "Fix a build warning when using the PAGE0 pointer"
      
      * tag 'for-5.15/parisc-4' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux:
        parisc: Use absolute_pointer() to define PAGE0
      5fe983d3
    • Linus Torvalds's avatar
      Merge tag 'm68k-for-v5.15-tag2' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k · 077a6ccf
      Linus Torvalds authored
      Pull m68k fixes from Geert Uytterhoeven:
      
       - Warning fixes to mitigate CONFIG_WERROR=y
      
      * tag 'm68k-for-v5.15-tag2' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k:
        m68k: mvme: Remove overdue #warnings in RTC handling
        m68k: Double cast io functions to unsigned long
      077a6ccf
    • David Thompson's avatar
      mlxbf_gige: clear valid_polarity upon open · ee8a9600
      David Thompson authored
      The network interface managed by the mlxbf_gige driver can
      get into a problem state where traffic does not flow.
      In this state, the interface will be up and enabled, but
      will stop processing received packets.  This problem state
      will happen if three specific conditions occur:
          1) driver has received more than (N * RxRingSize) packets but
             less than (N+1 * RxRingSize) packets, where N is an odd number
             Note: the command "ethtool -g <interface>" will display the
             current receive ring size, which currently defaults to 128
          2) the driver's interface was disabled via "ifconfig oob_net0 down"
             during the window described in #1.
          3) the driver's interface is re-enabled via "ifconfig oob_net0 up"
      
      This patch ensures that the driver's "valid_polarity" field is
      cleared during the open() method so that it always matches the
      receive polarity used by hardware.  Without this fix, the driver
      needs to be unloaded and reloaded to correct this problem state.
      
      Fixes: f92e1869 ("Add Mellanox BlueField Gigabit Ethernet driver")
      Reviewed-by: default avatarAsmaa Mnebhi <asmaa@nvidia.com>
      Signed-off-by: default avatarDavid Thompson <davthompson@nvidia.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      ee8a9600
    • Paolo Abeni's avatar
      igc: fix tunnel offloading · 40ee363c
      Paolo Abeni authored
      Checking tunnel offloading, it turns out that offloading doesn't work
      as expected.  The following script allows to reproduce the issue.
      Call it as `testscript DEVICE LOCALIP REMOTEIP NETMASK'
      
      === SNIP ===
      if [ $# -ne 4 ]
      then
        echo "Usage $0 DEVICE LOCALIP REMOTEIP NETMASK"
        exit 1
      fi
      DEVICE="$1"
      LOCAL_ADDRESS="$2"
      REMOTE_ADDRESS="$3"
      NWMASK="$4"
      echo "Driver: $(ethtool -i ${DEVICE} | awk '/^driver:/{print $2}') "
      ethtool -k "${DEVICE}" | grep tx-udp
      echo
      echo "Set up NIC and tunnel..."
      ip addr add "${LOCAL_ADDRESS}/${NWMASK}" dev "${DEVICE}"
      ip link set "${DEVICE}" up
      sleep 2
      ip link add vxlan1 type vxlan id 42 \
      		   remote "${REMOTE_ADDRESS}" \
      		   local "${LOCAL_ADDRESS}" \
      		   dstport 0 \
      		   dev "${DEVICE}"
      ip addr add fc00::1/64 dev vxlan1
      ip link set vxlan1 up
      sleep 2
      rm -f vxlan.pcap
      echo "Running tcpdump and iperf3..."
      ( nohup tcpdump -i any -w vxlan.pcap >/dev/null 2>&1 ) &
      sleep 2
      iperf3 -c fc00::2 >/dev/null
      pkill tcpdump
      echo
      echo -n "Max. Paket Size: "
      tcpdump -r vxlan.pcap -nnle 2>/dev/null \
      | grep "${LOCAL_ADDRESS}.*> ${REMOTE_ADDRESS}.*OTV" \
      | awk '{print $8}' | awk -F ':' '{print $1}' \
      | sort -n | tail -1
      echo
      ip link del vxlan1
      ip addr del ${LOCAL_ADDRESS}/${NWMASK} dev "${DEVICE}"
      === SNAP ===
      
      The expected outcome is
      
        Max. Paket Size: 64904
      
      This is what you see on igb, the code igc has been taken from.
      However, on igc the output is
      
        Max. Paket Size: 1516
      
      so the GSO aggregate packets are segmented by the kernel before calling
      igc_xmit_frame.  Inside the subsequent call to igc_tso, the check for
      skb_is_gso(skb) fails and the function returns prematurely.
      
      It turns out that this occurs because the feature flags aren't set
      entirely correctly in igc_probe.  In contrast to the original code
      from igb_probe, igc_probe neglects to set the flags required to allow
      tunnel offloading.
      
      Setting the same flags as igb fixes the issue on igc.
      
      Fixes: 34428dff ("igc: Add GSO partial support")
      Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
      Tested-by: default avatarCorinna Vinschen <vinschen@redhat.com>
      Acked-by: default avatarSasha Neftin <sasha.neftin@intel.com>
      Tested-by: default avatarNechama Kraus <nechamax.kraus@linux.intel.com>
      Signed-off-by: default avatarTony Nguyen <anthony.l.nguyen@intel.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      40ee363c
    • Eli Cohen's avatar
      net/{mlx5|nfp|bnxt}: Remove unnecessary RTNL lock assert · 7c3a0a01
      Eli Cohen authored
      Remove the assert from the callback priv lookup function since it does
      not require RTNL lock and is already protected by flow_indr_block_lock.
      
      This will avoid warnings from being emitted to dmesg if the driver
      registers its callback after an ingress qdisc was created for a
      netdevice.
      
      The warnings started after the following patch was merged:
      commit 74fc4f82 ("net: Fix offloading indirect devices dependency on qdisc order creation")
      Signed-off-by: default avatarEli Cohen <elic@nvidia.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      7c3a0a01
    • Adam Borowski's avatar
      net: wan: wanxl: define CROSS_COMPILE_M68K · 84fb7dfc
      Adam Borowski authored
      It was used but never set.  The hardcoded value from before the dawn of
      time was non-standard; the usual name for cross-tools is $TRIPLET-$TOOL
      Signed-off-by: default avatarAdam Borowski <kilobyte@angband.pl>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      84fb7dfc
    • Xiang wangx's avatar
      selftests: nci: replace unsigned int with int · 98dc68f8
      Xiang wangx authored
      Should not use comparison of unsigned expressions < 0.
      Signed-off-by: default avatarXiang wangx <wangxiang@cdjrlc.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      98dc68f8
    • Helge Deller's avatar
      parisc: Use absolute_pointer() to define PAGE0 · 90cc7bed
      Helge Deller authored
      Use absolute_pointer() wrapper for PAGE0 to avoid this compiler warning:
      
        arch/parisc/kernel/setup.c: In function 'start_parisc':
        error: '__builtin_memcmp_eq' specified bound 8 exceeds source size 0
      Signed-off-by: default avatarHelge Deller <deller@gmx.de>
      Co-Developed-by: default avatarGuenter Roeck <linux@roeck-us.net>
      Suggested-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      90cc7bed
    • Linus Torvalds's avatar
      Merge tag 'hyperv-fixes-signed-20210915' of... · ff1ffd71
      Linus Torvalds authored
      Merge tag 'hyperv-fixes-signed-20210915' of git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux
      
      Pull hyperv fixes from Wei Liu:
      
       - Fix kernel crash caused by uio driver (Vitaly Kuznetsov)
      
       - Remove on-stack cpumask from HV APIC code (Wei Liu)
      
      * tag 'hyperv-fixes-signed-20210915' of git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux:
        x86/hyperv: remove on-stack cpumask from hv_send_ipi_mask_allbutself
        asm-generic/hyperv: provide cpumask_to_vpset_noself
        Drivers: hv: vmbus: Fix kernel crash upon unbinding a device from uio_hv_generic driver
      ff1ffd71
    • Linus Torvalds's avatar
      Merge tag 'rtc-5.15-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux · 453fa43c
      Linus Torvalds authored
      Pull RTC fix from Alexandre Belloni:
       "Fix a locking issue in the cmos rtc driver"
      
      * tag 'rtc-5.15-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux:
        rtc: cmos: Disable irq around direct invocation of cmos_interrupt()
      453fa43c
  4. 15 Sep, 2021 4 commits
    • Vladimir Oltean's avatar
      net: dsa: flush switchdev workqueue before tearing down CPU/DSA ports · a57d8c21
      Vladimir Oltean authored
      Sometimes when unbinding the mv88e6xxx driver on Turris MOX, these error
      messages appear:
      
      mv88e6085 d0032004.mdio-mii:12: port 1 failed to delete be:79:b4:9e:9e:96 vid 1 from fdb: -2
      mv88e6085 d0032004.mdio-mii:12: port 1 failed to delete be:79:b4:9e:9e:96 vid 0 from fdb: -2
      mv88e6085 d0032004.mdio-mii:12: port 1 failed to delete d8:58:d7:00:ca:6d vid 100 from fdb: -2
      mv88e6085 d0032004.mdio-mii:12: port 1 failed to delete d8:58:d7:00:ca:6d vid 1 from fdb: -2
      mv88e6085 d0032004.mdio-mii:12: port 1 failed to delete d8:58:d7:00:ca:6d vid 0 from fdb: -2
      
      (and similarly for other ports)
      
      What happens is that DSA has a policy "even if there are bugs, let's at
      least not leak memory" and dsa_port_teardown() clears the dp->fdbs and
      dp->mdbs lists, which are supposed to be empty.
      
      But deleting that cleanup code, the warnings go away.
      
      => the FDB and MDB lists (used for refcounting on shared ports, aka CPU
      and DSA ports) will eventually be empty, but are not empty by the time
      we tear down those ports. Aka we are deleting them too soon.
      
      The addresses that DSA complains about are host-trapped addresses: the
      local addresses of the ports, and the MAC address of the bridge device.
      
      The problem is that offloading those entries happens from a deferred
      work item scheduled by the SWITCHDEV_FDB_DEL_TO_DEVICE handler, and this
      races with the teardown of the CPU and DSA ports where the refcounting
      is kept.
      
      In fact, not only it races, but fundamentally speaking, if we iterate
      through the port list linearly, we might end up tearing down the shared
      ports even before we delete a DSA user port which has a bridge upper.
      
      So as it turns out, we need to first tear down the user ports (and the
      unused ones, for no better place of doing that), then the shared ports
      (the CPU and DSA ports). In between, we need to ensure that all work
      items scheduled by our switchdev handlers (which only run for user
      ports, hence the reason why we tear them down first) have finished.
      
      Fixes: 161ca59d ("net: dsa: reference count the MDB entries at the cross-chip notifier level")
      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/20210914134726.2305133-1-vladimir.oltean@nxp.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      a57d8c21
    • Vladimir Oltean's avatar
      Revert "net: phy: Uniform PHY driver access" · 301de697
      Vladimir Oltean authored
      This reverts commit 3ac8eed6, which did
      more than it said on the box, and not only it replaced to_phy_driver
      with phydev->drv, but it also removed the "!drv" check, without actually
      explaining why that is fine.
      
      That patch in fact breaks suspend/resume on any system which has PHY
      devices with no drivers bound.
      
      The stack trace is:
      
      Unable to handle kernel NULL pointer dereference at virtual address 00000000000000e8
      pc : mdio_bus_phy_suspend+0xd8/0xec
      lr : dpm_run_callback+0x38/0x90
      Call trace:
       mdio_bus_phy_suspend+0xd8/0xec
       dpm_run_callback+0x38/0x90
       __device_suspend+0x108/0x3cc
       dpm_suspend+0x140/0x210
       dpm_suspend_start+0x7c/0xa0
       suspend_devices_and_enter+0x13c/0x540
       pm_suspend+0x2a4/0x330
      
      Examples why that assumption is not fine:
      
      - There is an MDIO bus with a PHY device that doesn't have a specific
        PHY driver loaded, because mdiobus_register() automatically creates a
        PHY device for it but there is no specific PHY driver in the system.
        Normally under those circumstances, the generic PHY driver will be
        bound lazily to it (at phy_attach_direct time). But some Ethernet
        drivers attach to their PHY at .ndo_open time. Until then it, the
        to-be-driven-by-genphy PHY device will not have a driver. The blamed
        patch amounts to saying "you need to open all net devices before the
        system can suspend, to avoid the NULL pointer dereference".
      
      - There is any raw MDIO device which has 'plausible' values in the PHY
        ID registers 2 and 3, which is located on an MDIO bus whose driver
        does not set bus->phy_mask = ~0 (which prevents auto-scanning of PHY
        devices). An example could be a MAC's internal MDIO bus with PCS
        devices on it, for serial links such as SGMII. PHY devices will get
        created for those PCSes too, due to that MDIO bus auto-scanning, and
        although those PHY devices are not used, they do not bother anybody
        either. PCS devices are usually managed in Linux as raw MDIO devices.
        Nonetheless, they do not have a PHY driver, nor does anybody attempt
        to connect to them (because they are not a PHY), and therefore this
        patch breaks that.
      
      The goal itself of the patch is questionable, so I am going for a
      straight revert. to_phy_driver does not seem to have a need to be
      replaced by phydev->drv, in fact that might even trigger code paths
      which were not given too deep of a thought.
      
      For instance:
      
      phy_probe populates phydev->drv at the beginning, but does not clean it
      up on any error (including EPROBE_DEFER). So if the phydev driver
      requests probe deferral, phydev->drv will remain populated despite there
      being no driver bound.
      
      If a system suspend starts in between the initial probe deferral request
      and the subsequent probe retry, we will be calling the phydev->drv->suspend
      method, but _before_ any phydev->drv->probe call has succeeded.
      
      That is to say, if the phydev->drv is allocating any driver-private data
      structure in ->probe, it pretty much expects that data structure to be
      available in ->suspend. But it may not. That is a pretty insane
      environment to present to PHY drivers.
      
      In the code structure before the blamed patch, mdio_bus_phy_may_suspend
      would just say "no, don't suspend" to any PHY device which does not have
      a driver pointer _in_the_device_structure_ (not the phydev->drv). That
      would essentially ensure that ->suspend will never get called for a
      device that has not yet successfully completed probe. This is the code
      structure the patch is returning to, via the revert.
      
      Fixes: 3ac8eed6 ("net: phy: Uniform PHY driver access")
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Acked-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Link: https://lore.kernel.org/r/20210914140515.2311548-1-vladimir.oltean@nxp.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      301de697
    • Vladimir Oltean's avatar
      net: dsa: destroy the phylink instance on any error in dsa_slave_phy_setup · 6a52e733
      Vladimir Oltean authored
      DSA supports connecting to a phy-handle, and has a fallback to a non-OF
      based method of connecting to an internal PHY on the switch's own MDIO
      bus, if no phy-handle and no fixed-link nodes were present.
      
      The -ENODEV error code from the first attempt (phylink_of_phy_connect)
      is what triggers the second attempt (phylink_connect_phy).
      
      However, when the first attempt returns a different error code than
      -ENODEV, this results in an unbalance of calls to phylink_create and
      phylink_destroy by the time we exit the function. The phylink instance
      has leaked.
      
      There are many other error codes that can be returned by
      phylink_of_phy_connect. For example, phylink_validate returns -EINVAL.
      So this is a practical issue too.
      
      Fixes: aab9c406 ("net: dsa: Plug in PHYLINK support")
      Signed-off-by: default avatarVladimir Oltean <vladimir.oltean@nxp.com>
      Reviewed-by: default avatarFlorian Fainelli <f.fainelli@gmail.com>
      Reviewed-by: default avatarRussell King (Oracle) <rmk+kernel@armlinux.org.uk>
      Link: https://lore.kernel.org/r/20210914134331.2303380-1-vladimir.oltean@nxp.comSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
      6a52e733
    • Linus Torvalds's avatar
      qnx4: avoid stringop-overread errors · b7213ffa
      Linus Torvalds authored
      The qnx4 directory entries are 64-byte blocks that have different
      contents depending on the a status byte that is in the last byte of the
      block.
      
      In particular, a directory entry can be either a "link info" entry with
      a 48-byte name and pointers to the real inode information, or an "inode
      entry" with a smaller 16-byte name and the full inode information.
      
      But the code was written to always just treat the directory name as if
      it was part of that "inode entry", and just extend the name to the
      longer case if the status byte said it was a link entry.
      
      That work just fine and gives the right results, but now that gcc is
      tracking data structure accesses much more, the code can trigger a
      compiler error about using up to 48 bytes (the long name) in a structure
      that only has that shorter name in it:
      
         fs/qnx4/dir.c: In function ‘qnx4_readdir’:
         fs/qnx4/dir.c:51:32: error: ‘strnlen’ specified bound 48 exceeds source size 16 [-Werror=stringop-overread]
            51 |                         size = strnlen(de->di_fname, size);
               |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~
         In file included from fs/qnx4/qnx4.h:3,
                          from fs/qnx4/dir.c:16:
         include/uapi/linux/qnx4_fs.h:45:25: note: source object declared here
            45 |         char            di_fname[QNX4_SHORT_NAME_MAX];
               |                         ^~~~~~~~
      
      which is because the source code doesn't really make this whole "one of
      two different types" explicit.
      
      Fix this by introducing a very explicit union of the two types, and
      basically explaining to the compiler what is really going on.
      Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
      b7213ffa