- 25 Oct, 2021 5 commits
-
-
Vladimir Oltean authored
The sja1105 hardware seems as concurrent as can be, but when we create a background script that adds/removes a rain of FDB entries without the rtnl_mutex taken, then in parallel we do another operation like run 'bridge fdb show', we can notice these errors popping up: sja1105 spi2.0: port 2 failed to read back entry for 00:01:02:03:00:40 vid 0: -ENOENT sja1105 spi2.0: port 2 failed to add 00:01:02:03:00:40 vid 0 to fdb: -2 sja1105 spi2.0: port 2 failed to read back entry for 00:01:02:03:00:46 vid 0: -ENOENT sja1105 spi2.0: port 2 failed to add 00:01:02:03:00:46 vid 0 to fdb: -2 Luckily what is going on does not require a major rework in the driver. The sja1105_dynamic_config_read() function sends multiple SPI buffers to the peripheral until the operation completes. We should not do anything until the hardware clears the VALID bit. But since there is no locking (i.e. right now we are implicitly serialized by the rtnl_mutex, but if we remove that), it might be possible that the process which performs the dynamic config read is preempted and another one performs a dynamic config write. What will happen in that case is that sja1105_dynamic_config_read(), when it resumes, expects to see VALIDENT set for the entry it reads back. But it won't. This can be corrected by introducing a mutex for serializing SPI accesses to the dynamic config interface which should be atomic with respect to each other. Signed-off-by:
Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by:
Florian Fainelli <f.fainelli@gmail.com> Signed-off-by:
David S. Miller <davem@davemloft.net>
-
Vladimir Oltean authored
The hardware manual says that software should attempt a new dynamic config access (be it a a write or a read-back) only while the VALID bit is cleared. The VALID bit is set by software to 1, and it remains set as long as the hardware is still processing the request. Currently the driver only polls for the command completion only for reads, because that's when we need the actual data read back. Writes have been more or less "asynchronous", although this has never been an observable issue. This change makes sja1105_dynamic_config_write poll the VALID bit as well, to absolutely ensure that a follow-up access to the static config finds the VALID bit cleared. So VALID means "work in progress", while VALIDENT means "entry being read is valid". On reads we check the VALIDENT bit too, while on writes that bit is not always defined. So we need to factor it out of the loop, and make the loop provide back the unpacked command structure, so that sja1105_dynamic_config_read can check the VALIDENT bit. The change also attempts to convert the open-coded loop to use the read_poll_timeout macro, since I know this will come up during review. It's more code, but hey, it uses read_poll_timeout! Tested on SJA1105T, SJA1105S, SJA1110A. Signed-off-by:
-
Vladimir Oltean authored
At present, when either of ds->ops->port_fdb_del() or ds->ops->port_mdb_del() return a non-zero error code, we attempt to save the day and keep the data structure associated with that switchdev object, as the deletion procedure did not complete. However, the way in which we do this is suspicious to the checker in lib/refcount.c, who thinks it is buggy to increment a refcount that became zero, and that this is indicative of a use-after-free. Fixes: 161ca59d ("net: dsa: reference count the MDB entries at the cross-chip notifier level") Signed-off-by:
-
David S. Miller authored
This reverts commit 965e6b26, reversing changes made to 4d98bb0d.
-
David S. Miller authored
Merge tag 'linux-can-next-for-5.16-20211024' of git://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next Marc Kleine-Budde says: ==================== pull-request: can-next 2021-10-24 this is a pull request of 15 patches for net-next/master. The first patch is by Thomas Gleixner and makes use of hrtimer_forward_now() in the CAN broad cast manager (bcm). The next patch is by me and changes the type of the variables used in the CAN bit timing calculation can_fixup_bittiming() to unsigned int. Vincent Mailhol provides 6 patches targeting the CAN device infrastructure. The CAN-FD specific Transmitter Delay Compensation (TDC) is updated and configuration via the CAN netlink interface is added. Qing Wang's patch updates the at91 and janz-ican3 drivers to use sysfs_emit() instead of snprintf() in the sysfs show functions. Geert Uytterhoeven's patch drops the unneeded ARM dependency from the rar Kconfig. Cai Huoqing's patch converts the mscan driver to make use of the dev_err_probe() helper function. A patch by me against the gsusb driver changes the printf format strings to use %u to print unsigned values. Stephane Grosjean's patch updates the peak_usb CAN-FD driver to use the 64 bit timestamps provided by the hardware. The last 2 patches target the xilinx_can driver. Michal Simek provides a patch that removes repeated word from the kernel-doc and Dongliang Mu's patch removes a redundant netif_napi_del() from the xcan_remove() function. ==================== Signed-off-by:
-
- 24 Oct, 2021 35 commits
-
-
Dongliang Mu authored
Since netif_napi_del() is already done in the free_candev(), we remove this redundant netif_napi_del() invocation. In addition, this patch can match the operations in the xcan_probe() and xcan_remove() functions. Link: https://lore.kernel.org/all/20211017125022.3100329-1-mudongliangabcd@gmail.comSigned-off-by:
Dongliang Mu <mudongliangabcd@gmail.com> Signed-off-by:
Marc Kleine-Budde <mkl@pengutronix.de>
-
Michal Simek authored
Trivial patch. Issue is reported by checkpatch. Link: https://lore.kernel.org/all/267c11097c90debbb5b1efebbeabc98161177def.1632306843.git.michal.simek@xilinx.comSigned-off-by:
Michal Simek <michal.simek@xilinx.com> Signed-off-by:
-
Stephane Grosjean authored
This patch allows to use the whole 64-bit timestamps received from the CAN-FD device (expressed in µs) rather than only its low part, in the hwtstamp structure of the skb transferred to the network layer, when a CAN/CANFD frame has been received. Link: https://lore.kernel.org/all/20210930094603.23134-1-s.grosjean@peak-system.comSigned-off-by:
Stephane Grosjean <s.grosjean@peak-system.com> Signed-off-by:
-
Marc Kleine-Budde authored
This patch changes printf modifier to an unsigned int, as the printed variables are unsigned. Link: https://lore.kernel.org/all/20210914101005.84394-1-mkl@pengutronix.de Cc: Roman Valls <brainstorm@nopcode.org> Signed-off-by:
-
Cai Huoqing authored
When possible use dev_err_probe help to properly deal with the PROBE_DEFER error, the benefit is that DEFER issue will be logged in the devices_deferred debugfs file. And using dev_err_probe() can reduce code size, and simplify the code. Link: https://lore.kernel.org/all/20210915145726.7092-1-caihuoqing@baidu.comSigned-off-by:
Cai Huoqing <caihuoqing@baidu.com> Signed-off-by:
-
Geert Uytterhoeven authored
The dependency on ARM predates the dependency on ARCH_RENESAS. The latter was introduced for Renesas arm64 SoCs first, and later extended to cover Renesas ARM SoCs, too. Link: https://lore.kernel.org/all/362d9ced19f3524ee8917df5681b3880c13cac85.1630416373.git.geert+renesas@glider.beSigned-off-by:
Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by:
-
Qing Wang authored
The sysfs show() functions must not use snprintf() when formatting the value to be returned to user space. Fix the following coccicheck warning: | drivers/net/can/at91_can.c:1185: WARNING: use scnprintf or sprintf. | drivers/net/can/janz-ican3.c:1834: WARNING: use scnprintf or sprintf. | | Use sysfs_emit instead of scnprintf or sprintf makes more sense. Link: https://lore.kernel.org/all/1634280624-4816-1-git-send-email-wangqing@vivo.comSigned-off-by:
Qing Wang <wangqing@vivo.com> Signed-off-by:
-
Vincent Mailhol authored
struct can_tdc::tdco represents the absolute offset from TDCV. Some controllers use instead an offset relative to the Sample Point (SP) such that: | SSP = TDCV + absolute TDCO | = TDCV + SP + relative TDCO Consequently: | relative TDCO = absolute TDCO - SP The function can_tdc_get_relative_tdco() allow to retrieve this relative TDCO value. Link: https://lore.kernel.org/all/20210918095637.20108-7-mailhol.vincent@wanadoo.fr CC: Stefan Mätje <Stefan.Maetje@esd.eu> Signed-off-by:
Vincent Mailhol <mailhol.vincent@wanadoo.fr> Signed-off-by:
-
Vincent Mailhol authored
Some CAN device can measure the TDCV (Transmission Delay Compensation Value) automatically for each transmitted CAN frames. A callback function do_get_auto_tdcv() is added to retrieve that value. This function is used only if CAN_CTRLMODE_TDC_AUTO is enabled (if CAN_CTRLMODE_TDC_MANUAL is selected, the TDCV value is provided by the user). If the device does not support reporting of TDCV, do_get_auto_tdcv() should be set to NULL and TDCV will not be reported by the netlink interface. On success, do_get_auto_tdcv() shall return 0. If the value can not be measured by the device, for example because network is down or because no frames were transmitted yet, can_priv::do_get_auto_tdcv() shall return a negative error code (e.g. -EINVAL) to signify that the value is not yet available. In such cases, TDCV is not reported by the netlink interface. Link: https://lore.kernel.org/all/20210918095637.20108-6-mailhol.vincent@wanadoo.fr CC: Stefan Mätje <stefan.maetje@esd.eu> Signed-off-by:
-
Vincent Mailhol authored
Add the netlink interface for TDC parameters of struct can_tdc_const and can_tdc. Contrary to the can_bittiming(_const) structures for which there is just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure, here, we create a nested entry IFLA_CAN_TDC. Within this nested entry, additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC parameters of the newly introduced struct can_tdc_const and struct can_tdc. For struct can_tdc_const, these are: IFLA_CAN_TDC_TDCV_MIN IFLA_CAN_TDC_TDCV_MAX IFLA_CAN_TDC_TDCO_MIN IFLA_CAN_TDC_TDCO_MAX IFLA_CAN_TDC_TDCF_MIN IFLA_CAN_TDC_TDCF_MAX For struct can_tdc, these are: IFLA_CAN_TDC_TDCV IFLA_CAN_TDC_TDCO IFLA_CAN_TDC_TDCF This is done so that changes can be applied in the future to the structures without breaking the netlink interface. The TDC netlink logic works as follow: * CAN_CTRLMODE_FD is not provided: - if any TDC parameters are provided: error. - TDC parameters not provided: TDC parameters unchanged. * CAN_CTRLMODE_FD is provided and is false: - TDC is deactivated: both the structure and the CAN_CTRLMODE_TDC_{AUTO,MANUAL} flags are flushed. * CAN_CTRLMODE_FD provided and is true: - CAN_CTRLMODE_TDC_{AUTO,MANUAL} and tdc{v,o,f} not provided: call can_calc_tdco() to automatically decide whether TDC should be activated and, if so, set CAN_CTRLMODE_TDC_AUTO and uses the calculated tdco value. - CAN_CTRLMODE_TDC_AUTO and tdco provided: set CAN_CTRLMODE_TDC_AUTO and use the provided tdco value. Here, tdcv is illegal and tdcf is optional. - CAN_CTRLMODE_TDC_MANUAL and both of tdcv and tdco provided: set CAN_CTRLMODE_TDC_MANUAL and use the provided tdcv and tdco value. Here, tdcf is optional. - CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive. Whenever one flag is turned on, the other will automatically be turned off. Providing both returns an error. - Combination other than the one listed above are illegal and will return an error. N.B. above rules mean that whenever CAN_CTRLMODE_FD is provided, the previous TDC values will be overwritten. The only option to reuse previous TDC value is to not provide CAN_CTRLMODE_FD. All the new parameters are defined as u32. This arbitrary choice is done to mimic the other bittiming values with are also all of type u32. An u16 would have been sufficient to hold the TDC values. This patch completes below series (c.f. [1]): - commit 289ea9e4 ("can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)") - commit c25cc799 ("can: bittiming: add calculation for CAN FD Transmitter Delay Compensation (TDC)") [1] https://lore.kernel.org/linux-can/20210224002008.4158-1-mailhol.vincent@wanadoo.fr/T/#t Link: https://lore.kernel.org/all/20210918095637.20108-5-mailhol.vincent@wanadoo.frSigned-off-by: -
Vincent Mailhol authored
The function can_calc_tdco() directly retrieves can_priv from the net_device and directly modifies it. This is annoying for the upcoming patch. In drivers/net/can/dev/netlink.c:can_changelink(), the data bittiming are written to a temporary structure and memcpyed to can_priv only after everything succeeded. In the next patch, where we will introduce the netlink interface for TDC parameters, we will add a new TDC block which can potentially fail. For this reason, the data bittiming temporary structure has to be copied after that to-be-introduced TDC block. However, TDC also needs to access data bittiming information. We change the prototype so that the data bittiming structure is passed to can_calc_tdco() as an argument instead of retrieving it from priv. This way can_calc_tdco() can access the data bittiming before it gets memcpyed to priv. Link: https://lore.kernel.org/all/20210918095637.20108-4-mailhol.vincent@wanadoo.frSigned-off-by:
-
Vincent Mailhol authored
In the current implementation, all Transmission Delay Compensation (TDC) parameters are expressed in time quantum. However, ISO 11898-1 actually specifies that these should be expressed in *minimum* time quantum. Furthermore, the minimum time quantum is specified to be "one node clock period long" (c.f. paragraph 11.3.1.1 "Bit time"). For sake of simplicity, we prefer to use the "clock period" term instead of "minimum time quantum" because we believe that it is more broadly understood. This patch fixes that discrepancy by updating the documentation and the formula for TDCO calculation. N.B. In can_calc_tdco(), the sample point (in time quantum) was calculated using a division, thus introducing a risk of rounding and truncation errors. On top of changing the unit to clock period, we also modified the formula to use only additions. Link: https://lore.kernel.org/all/20210918095637.20108-3-mailhol.vincent@wanadoo.frSuggested-by:
Stefan Mätje <Stefan.Maetje@esd.eu> Signed-off-by:
-
Vincent Mailhol authored
ISO 11898-1 specifies in section 11.3.3 "Transmitter delay compensation" that "the configuration range for [the] SSP position shall be at least 0 to 63 minimum time quanta." Because SSP = TDCV + TDCO, it means that we should allow both TDCV and TDCO to hold zero value in order to honor SSP's minimum possible value. However, current implementation assigned special meaning to TDCV and TDCO's zero values: * TDCV = 0 -> TDCV is automatically measured by the transceiver. * TDCO = 0 -> TDC is off. In order to allow for those values to really be zero and to maintain current features, we introduce two new flags: * CAN_CTRLMODE_TDC_AUTO indicates that the controller support automatic measurement of TDCV. * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support manual configuration of TDCV. N.B.: current implementation failed to provide an option for the driver to indicate that only manual mode was supported. TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function can_tdc_is_enabled() which is also introduced in this patch. Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to struct can_tdc_const. While we are not convinced that those three fields could be anything else than zero, we can imagine that some controllers might specify a lower bound on these. Thus, those minimums are really added "just in case". Comments of struct can_tdc and can_tdc_const are updated accordingly. Finally, the changes are applied to the etas_es58x driver. Link: https://lore.kernel.org/all/20210918095637.20108-2-mailhol.vincent@wanadoo.frSigned-off-by: -
Marc Kleine-Budde authored
All timing calculation is done with unsigned integers, so change type of tseg1 and alltseg to unsigned int, too. Link: https://lore.kernel.org/all/20211013130653.1513627-1-mkl@pengutronix.de Link: https://github.com/linux-can/can-utils/pull/314Reported-by:
Gary Bisson <bisson.gary@gmail.com> Signed-off-by:
-
Thomas Gleixner authored
hrtimer_forward_now() provides the same functionality as the open coded hrimer_forward() invocation. Prepares for removal of hrtimer_forward() from the public interfaces. Link: https://lore.kernel.org/all/20210923153339.684546907@linutronix.deSigned-off-by:
Thomas Gleixner <tglx@linutronix.de> Cc: Oliver Hartkopp <socketcan@hartkopp.net> Cc: linux-can@vger.kernel.org Cc: Marc Kleine-Budde <mkl@pengutronix.de> Cc: netdev@vger.kernel.org Cc: Jakub Kicinski <kuba@kernel.org> Cc: "David S. Miller" <davem@davemloft.net> Signed-off-by:
-
David S. Miller authored
Jakub Kicinski says: ==================== don't write directly to netdev->dev_addr Constify references to netdev->dev_addr and use appropriate helpers. v2: s/got/go/ ==================== Signed-off-by: -
Jakub Kicinski authored
Get it ready for constant netdev->dev_addr. Signed-off-by:
Jakub Kicinski <kuba@kernel.org> Signed-off-by:
-
Jakub Kicinski authored
Commit 406f42fa ("net-next: When a bond have a massive amount of VLANs...") introduced a rbtree for faster Ethernet address look up. To maintain netdev->dev_addr in this tree we need to make all the writes to it go through appropriate helpers. We will make netdev->dev_addr a const. Make sure local references to netdev->dev_addr are constant. Signed-off-by:
-
Jakub Kicinski authored
Get it ready for constant netdev->dev_addr. Signed-off-by:
-
Jakub Kicinski authored
hsr_create_self_node() may get netdev->dev_addr passed as argument, netdev->dev_addr will be const soon. Signed-off-by:
-
Jakub Kicinski authored
Commit 406f42fa ("net-next: When a bond have a massive amount of VLANs...") introduced a rbtree for faster Ethernet address look up. To maintain netdev->dev_addr in this tree we need to make all the writes to it go through appropriate helpers. Make sure local references to netdev->dev_addr are constant. Signed-off-by:
-
Jakub Kicinski authored
netdev->dev_addr will become a const soon(ish), constify the local variables referring to it. Reviewed-by:
Andrew Lunn <andrew@lunn.ch> Signed-off-by:
-
Jakub Kicinski authored
Get it ready for constant netdev->dev_addr. Signed-off-by:
-
Jakub Kicinski authored
Get it ready for constant netdev->dev_addr. Signed-off-by:
-
Sean Anderson authored
This converts instances of bitmap_foo(args..., __ETHTOOL_LINK_MODE_MASK_NBITS) to linkmode_foo(args...) I manually fixed up some lines to prevent them from being excessively long. Otherwise, this change was generated with the following semantic patch: // Generated with // echo linux/linkmode.h > includes // git grep -Flf includes include/ | cut -f 2- -d / | cat includes - \ // | sort | uniq | tee new_includes | wc -l && mv new_includes includes // and repeating until the number stopped going up @i@ @@ ( #include <linux/acpi_mdio.h> | #include <linux/brcmphy.h> | #include <linux/dsa/loop.h> | #include <linux/dsa/sja1105.h> | #include <linux/ethtool.h> | #include <linux/ethtool_netlink.h> | #include <linux/fec.h> | #include <linux/fs_enet_pd.h> | #include <linux/fsl/enetc_mdio.h> | #include <linux/fwnode_mdio.h> | #include <linux/linkmode.h> | #include <linux/lsm_audit.h> | #include <linux/mdio-bitbang.h> | #include <linux/mdio.h> | #include <linux/mdio-mux.h> | #include <linux/mii.h> | #include <linux/mii_timestamper.h> | #include <linux/mlx5/accel.h> | #include <linux/mlx5/cq.h> | #include <linux/mlx5/device.h> | #include <linux/mlx5/driver.h> | #include <linux/mlx5/eswitch.h> | #include <linux/mlx5/fs.h> | #include <linux/mlx5/port.h> | #include <linux/mlx5/qp.h> | #include <linux/mlx5/rsc_dump.h> | #include <linux/mlx5/transobj.h> | #include <linux/mlx5/vport.h> | #include <linux/of_mdio.h> | #include <linux/of_net.h> | #include <linux/pcs-lynx.h> | #include <linux/pcs/pcs-xpcs.h> | #include <linux/phy.h> | #include <linux/phy_led_triggers.h> | #include <linux/phylink.h> | #include <linux/platform_data/bcmgenet.h> | #include <linux/platform_data/xilinx-ll-temac.h> | #include <linux/pxa168_eth.h> | #include <linux/qed/qed_eth_if.h> | #include <linux/qed/qed_fcoe_if.h> | #include <linux/qed/qed_if.h> | #include <linux/qed/qed_iov_if.h> | #include <linux/qed/qed_iscsi_if.h> | #include <linux/qed/qed_ll2_if.h> | #include <linux/qed/qed_nvmetcp_if.h> | #include <linux/qed/qed_rdma_if.h> | #include <linux/sfp.h> | #include <linux/sh_eth.h> | #include <linux/smsc911x.h> | #include <linux/soc/nxp/lpc32xx-misc.h> | #include <linux/stmmac.h> | #include <linux/sunrpc/svc_rdma.h> | #include <linux/sxgbe_platform.h> | #include <net/cfg80211.h> | #include <net/dsa.h> | #include <net/mac80211.h> | #include <net/selftests.h> | #include <rdma/ib_addr.h> | #include <rdma/ib_cache.h> | #include <rdma/ib_cm.h> | #include <rdma/ib_hdrs.h> | #include <rdma/ib_mad.h> | #include <rdma/ib_marshall.h> | #include <rdma/ib_pack.h> | #include <rdma/ib_pma.h> | #include <rdma/ib_sa.h> | #include <rdma/ib_smi.h> | #include <rdma/ib_umem.h> | #include <rdma/ib_umem_odp.h> | #include <rdma/ib_verbs.h> | #include <rdma/iw_cm.h> | #include <rdma/mr_pool.h> | #include <rdma/opa_addr.h> | #include <rdma/opa_port_info.h> | #include <rdma/opa_smi.h> | #include <rdma/opa_vnic.h> | #include <rdma/rdma_cm.h> | #include <rdma/rdma_cm_ib.h> | #include <rdma/rdmavt_cq.h> | #include <rdma/rdma_vt.h> | #include <rdma/rdmavt_qp.h> | #include <rdma/rw.h> | #include <rdma/tid_rdma_defs.h> | #include <rdma/uverbs_ioctl.h> | #include <rdma/uverbs_named_ioctl.h> | #include <rdma/uverbs_std_types.h> | #include <rdma/uverbs_types.h> | #include <soc/mscc/ocelot.h> | #include <soc/mscc/ocelot_ptp.h> | #include <soc/mscc/ocelot_vcap.h> | #include <trace/events/ib_mad.h> | #include <trace/events/rdma_core.h> | #include <trace/events/rdma.h> | #include <trace/events/rpcrdma.h> | #include <uapi/linux/ethtool.h> | #include <uapi/linux/ethtool_netlink.h> | #include <uapi/linux/mdio.h> | #include <uapi/linux/mii.h> ) @depends on i@ expression list args; @@ ( - bitmap_zero(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_zero(args) | - bitmap_copy(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_copy(args) | - bitmap_and(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_and(args) | - bitmap_or(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_or(args) | - bitmap_empty(args, ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_empty(args) | - bitmap_andnot(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_andnot(args) | - bitmap_equal(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_equal(args) | - bitmap_intersects(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_intersects(args) | - bitmap_subset(args, __ETHTOOL_LINK_MODE_MASK_NBITS) + linkmode_subset(args) ) Add missing linux/mii.h include to mellanox. -DaveM Signed-off-by:
Sean Anderson <sean.anderson@seco.com> Signed-off-by:
-
David S. Miller authored
Vladimir Oltean says: ==================== Drop rtnl_lock from DSA .port_fdb_{add,del} As mentioned in the RFC posted 2 months ago: https://patchwork.kernel.org/project/netdevbpf/cover/20210824114049.3814660-1-vladimir.oltean@nxp.com/ DSA is transitioning to a driver API where the rtnl_lock is not held when calling ds->ops->port_fdb_add() and ds->ops->port_fdb_del(). Drivers cannot take that lock privately from those callbacks either. This change is required so that DSA can wait for switchdev FDB work items to finish before leaving the bridge. That change will be made in a future patch series. A small selftest is provided with the patch set in the hope that concurrency issues uncovered by this series, but not spotted by me by code inspection, will be caught. A status of the existing drivers: - mv88e6xxx_port_fdb_add() and mv88e6xxx_port_fdb_del() take mv88e6xxx_reg_lock() so they should be safe. - qca8k_fdb_add() and qca8k_fdb_del() take mutex_lock(&priv->reg_mutex) so they should be safe. - hellcreek_fdb_add() and hellcreek_fdb_add() take mutex_lock(&hellcreek->reg_lock) so they should be safe. - ksz9477_port_fdb_add() and ksz9477_port_fdb_del() take mutex_lock(&dev->alu_mutex) so they should be safe. - b53_fdb_add() and b53_fdb_del() did not have locking, so I've added a scheme based on my own judgement there (not tested). - felix_fdb_add() and felix_fdb_del() did not have locking, I've added and tested a locking scheme there. - mt7530_port_fdb_add() and mt7530_port_fdb_del() take mutex_lock(&priv->reg_mutex), so they should be safe. - gswip_port_fdb() did not have locking, so I've added a non-expert locking scheme based on my own judgement (not tested). - lan9303_alr_add_port() and lan9303_alr_del_port() take mutex_lock(&chip->alr_mutex) so they should be safe. - sja1105_fdb_add() and sja1105_fdb_del() did not have locking, I've added and tested a locking scheme. ==================== Signed-off-by: -
Vladimir Oltean authored
This test is a bit strange in that it is perhaps more manual than others: it does not transmit a clear OK/FAIL verdict, because user space does not have synchronous feedback from the kernel. If a hardware access fails, it is in deferred context. Nonetheless, on sja1105 I have used it successfully to find and solve a concurrency issue, so it can be used as a starting point for other driver maintainers too. Signed-off-by:
-
Vladimir Oltean authored
These programs are useful, but not all selftests require them. Additionally, on embedded boards without package management (things like buildroot), installing mausezahn or jq is not always as trivial as downloading a package from the web. So it is actually a bit annoying to require programs that are not used. Introduce options that can be set by scripts to not enforce these dependencies. For compatibility, default to "yes". Cc: Nikolay Aleksandrov <nikolay@nvidia.com> Cc: Ido Schimmel <idosch@nvidia.com> Cc: Guillaume Nault <gnault@redhat.com> Cc: Po-Hsu Lin <po-hsu.lin@canonical.com> Signed-off-by:
-
Vladimir Oltean authored
After talking with Ido Schimmel, it became clear that rtnl_lock is not actually required for anything that is done inside the SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE deferred work handlers. The reason why it was probably added by Arkadi Sharshevsky in commit c9eb3e0f ("net: dsa: Add support for learning FDB through notification") was to offer the same locking/serialization guarantees as .ndo_fdb_{add,del} and avoid reworking any drivers. DSA has implemented .ndo_fdb_add and .ndo_fdb_del until commit b117e1e8 ("net: dsa: delete dsa_legacy_fdb_add and dsa_legacy_fdb_del") - that is to say, until fairly recently. But those methods have been deleted, so now we are free to drop the rtnl_lock as well. Note that exposing DSA switch drivers to an unlocked method which was previously serialized by the rtnl_mutex is a potentially dangerous affair. Driver writers couldn't ensure that their internal locking scheme does the right thing even if they wanted. We could err on the side of paranoia and introduce a switch-wide lock inside the DSA framework, but that seems way overreaching. Instead, we could check as many drivers for regressions as we can, fix those first, then let this change go in once it is assumed to be fairly safe. Signed-off-by: -
Vladimir Oltean authored
Now that the rtnl_mutex is going away for dsa_port_{host_,}fdb_{add,del}, no one is serializing access to the address lists that DSA keeps for the purpose of reference counting on shared ports (CPU and cascade ports). It can happen for one dsa_switch_do_fdb_del to do list_del on a dp->fdbs element while another dsa_switch_do_fdb_{add,del} is traversing dp->fdbs. We need to avoid that. Currently dp->mdbs is not at risk, because dsa_switch_do_mdb_{add,del} still runs under the rtnl_mutex. But it would be nice if it would not depend on that being the case. So let's introduce a mutex per port (the address lists are per port too) and share it between dp->mdbs and dp->fdbs. The place where we put the locking is interesting. It could be tempting to put a DSA-level lock which still serializes calls to .port_fdb_{add,del}, but it would still not avoid concurrency with other driver code paths that are currently under rtnl_mutex (.port_fdb_dump, .port_fast_age). So it would add a very false sense of security (and adding a global switch-wide lock in DSA to resynchronize with the rtnl_lock is also counterproductive and hard). So the locking is intentionally done only where the dp->fdbs and dp->mdbs lists are traversed. That means, from a driver perspective, that .port_fdb_add will be called with the dp->addr_lists_lock mutex held on the CPU port, but not held on user ports. This is done so that driver writers are not encouraged to rely on any guarantee offered by dp->addr_lists_lock. Signed-off-by: -
Vladimir Oltean authored
Looking at the code, the GSWIP switch appears to hold bridging service structures (VLANs, FDBs, forwarding rules) in PCE table entries. Hardware access to the PCE table is non-atomic, and is comprised of several register reads and writes. These accesses are currently serialized by the rtnl_lock, but DSA is changing its driver API and that lock will no longer be held when calling ->port_fdb_add() and ->port_fdb_del(). So this driver needs to serialize the access to the PCE table using its own locking scheme. This patch adds that. Signed-off-by:
Hauke Mehrtens <hauke@hauke-m.de> Signed-off-by:
-
Vladimir Oltean authored
The b53 driver performs non-atomic transactions to the ARL table when adding, deleting and reading FDB and MDB entries. Traditionally these were all serialized by the rtnl_lock(), but now it is possible that DSA calls ->port_fdb_add and ->port_fdb_del without holding that lock. So the driver must have its own serialization logic. Add a mutex and hold it from all entry points (->port_fdb_{add,del,dump}, ->port_mdb_{add,del}). Signed-off-by: -
Vladimir Oltean authored
DSA would like to remove the rtnl_lock from its SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE handlers, and the felix driver uses the same MAC table functions as ocelot. This means that the MAC table functions will no longer be implicitly serialized with respect to each other by the rtnl_mutex, we need to add a dedicated lock in ocelot for the non-atomic operations of selecting a MAC table row, reading/writing what we want and polling for completion. Signed-off-by: -
Vladimir Oltean authored
The sja1105 hardware seems as concurrent as can be, but when we create a background script that adds/removes a rain of FDB entries without the rtnl_mutex taken, then in parallel we do another operation like run 'bridge fdb show', we can notice these errors popping up: sja1105 spi2.0: port 2 failed to read back entry for 00:01:02:03:00:40 vid 0: -ENOENT sja1105 spi2.0: port 2 failed to add 00:01:02:03:00:40 vid 0 to fdb: -2 sja1105 spi2.0: port 2 failed to read back entry for 00:01:02:03:00:46 vid 0: -ENOENT sja1105 spi2.0: port 2 failed to add 00:01:02:03:00:46 vid 0 to fdb: -2 Luckily what is going on does not require a major rework in the driver. The sja1105_dynamic_config_read() function sends multiple SPI buffers to the peripheral until the operation completes. We should not do anything until the hardware clears the VALID bit. But since there is no locking (i.e. right now we are implicitly serialized by the rtnl_mutex, but if we remove that), it might be possible that the process which performs the dynamic config read is preempted and another one performs a dynamic config write. What will happen in that case is that sja1105_dynamic_config_read(), when it resumes, expects to see VALIDENT set for the entry it reads back. But it won't. This can be corrected by introducing a mutex for serializing SPI accesses to the dynamic config interface which should be atomic with respect to each other. Signed-off-by:
-
Vladimir Oltean authored
The hardware manual says that software should attempt a new dynamic config access (be it a a write or a read-back) only while the VALID bit is cleared. The VALID bit is set by software to 1, and it remains set as long as the hardware is still processing the request. Currently the driver only polls for the command completion only for reads, because that's when we need the actual data read back. Writes have been more or less "asynchronous", although this has never been an observable issue. This change makes sja1105_dynamic_config_write poll the VALID bit as well, to absolutely ensure that a follow-up access to the static config finds the VALID bit cleared. So VALID means "work in progress", while VALIDENT means "entry being read is valid". On reads we check the VALIDENT bit too, while on writes that bit is not always defined. So we need to factor it out of the loop, and make the loop provide back the unpacked command structure, so that sja1105_dynamic_config_read can check the VALIDENT bit. The change also attempts to convert the open-coded loop to use the read_poll_timeout macro, since I know this will come up during review. It's more code, but hey, it uses read_poll_timeout! Tested on SJA1105T, SJA1105S, SJA1110A. Signed-off-by:
-
