| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 10 May 2024 13:05:33 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
jacob.e.keller@...el.com, michal.kubiak@...el.com,
maciej.fijalkowski@...el.com, sridhar.samudrala@...el.com,
przemyslaw.kitszel@...el.com, wojciech.drewek@...el.com,
pio.raczynski@...il.com, jiri@...dia.com,
mateusz.polchlopek@...el.com, shayd@...dia.com
Subject: Re: [iwl-next v1 03/14] ice: add basic devlink subfunctions support
Fri, May 10, 2024 at 09:13:51AM CEST, michal.swiatkowski@...ux.intel.com wrote:
>On Thu, May 09, 2024 at 01:06:52PM +0200, Jiri Pirko wrote:
>> Tue, May 07, 2024 at 01:45:04PM CEST, michal.swiatkowski@...ux.intel.com wrote:
>> >From: Piotr Raczynski <piotr.raczynski@...el.com>
>> >
>> >Implement devlink port handlers responsible for ethernet type devlink
>> >subfunctions. Create subfunction devlink port and setup all resources
>> >needed for a subfunction netdev to operate. Configure new VSI for each
>> >new subfunction, initialize and configure interrupts and Tx/Rx resources.
>> >Set correct MAC filters and create new netdev.
>> >
>> >For now, subfunction is limited to only one Tx/Rx queue pair.
>> >
>> >Only allocate new subfunction VSI with devlink port new command.
>> >This makes sure that real resources are configured only when a new
>> >subfunction gets activated. Allocate and free subfunction MSIX
>>
>> Interesting. You talk about actitation, yet you don't implement it here.
>>
>
>I will rephrase it. I meant that new VSI needs to be created even before
>any activation or configuration.
>
>>
>>
>> >interrupt vectors using new API calls with pci_msix_alloc_irq_at
>> >and pci_msix_free_irq.
>> >
>> >Support both automatic and manual subfunction numbers. If no subfunction
>> >number is provided, use xa_alloc to pick a number automatically. This
>> >will find the first free index and use that as the number. This reduces
>> >burden on users in the simple case where a specific number is not
>> >required. It may also be slightly faster to check that a number exists
>> >since xarray lookup should be faster than a linear scan of the dyn_ports
>> >xarray.
>> >
>> >Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
>> >Co-developed-by: Jacob Keller <jacob.e.keller@...el.com>
>> >Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>> >Signed-off-by: Piotr Raczynski <piotr.raczynski@...el.com>
>> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
>> >---
>> > .../net/ethernet/intel/ice/devlink/devlink.c | 3 +
>> > .../ethernet/intel/ice/devlink/devlink_port.c | 357 ++++++++++++++++++
>> > .../ethernet/intel/ice/devlink/devlink_port.h | 33 ++
>> > drivers/net/ethernet/intel/ice/ice.h | 4 +
>> > drivers/net/ethernet/intel/ice/ice_lib.c | 5 +-
>> > drivers/net/ethernet/intel/ice/ice_lib.h | 2 +
>> > drivers/net/ethernet/intel/ice/ice_main.c | 9 +-
>> > 7 files changed, 410 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> >index 10073342e4f0..3fb3a7e828a4 100644
>> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
>> >@@ -6,6 +6,7 @@
>> > #include "ice.h"
>> > #include "ice_lib.h"
>> > #include "devlink.h"
>> >+#include "devlink_port.h"
>> > #include "ice_eswitch.h"
>> > #include "ice_fw_update.h"
>> > #include "ice_dcb_lib.h"
>> >@@ -1277,6 +1278,8 @@ static const struct devlink_ops ice_devlink_ops = {
>> >
>> > .rate_leaf_parent_set = ice_devlink_set_parent,
>> > .rate_node_parent_set = ice_devlink_set_parent,
>> >+
>> >+ .port_new = ice_devlink_port_new,
>> > };
>> >
>> >+
>
>[...]
>
>> >+/**
>> >+ * ice_devlink_port_fn_hw_addr_set - devlink handler for mac address set
>> >+ * @port: pointer to devlink port
>> >+ * @hw_addr: hw address to set
>> >+ * @hw_addr_len: hw address length
>> >+ * @extack: extack for reporting error messages
>> >+ *
>> >+ * Sets mac address for the port, verifies arguments and copies address
>> >+ * to the subfunction structure.
>> >+ *
>> >+ * Return: zero on success or an error code on failure.
>> >+ */
>> >+static int
>> >+ice_devlink_port_fn_hw_addr_set(struct devlink_port *port, const u8 *hw_addr,
>> >+ int hw_addr_len,
>> >+ struct netlink_ext_ack *extack)
>> >+{
>> >+ struct ice_dynamic_port *dyn_port;
>> >+
>> >+ dyn_port = ice_devlink_port_to_dyn(port);
>> >+
>> >+ if (hw_addr_len != ETH_ALEN || !is_valid_ether_addr(hw_addr)) {
>> >+ NL_SET_ERR_MSG_MOD(extack, "Invalid ethernet address");
>> >+ return -EADDRNOTAVAIL;
>> >+ }
>> >+
>> >+ ether_addr_copy(dyn_port->hw_addr, hw_addr);
>>
>> How does this work? You copy the address to the internal structure, but
>> where you update the HW?
>>
>
>When the basic MAC filter is added in HW.
When is that. My point is, user can all this function at any time, and
when he calls it, he expect it's applied right away. In case it can't be
for example applied on activated SF, you should block the request.
>
>>
>> >+
>> >+ return 0;
>> >+}
>
>[...]
>
>> >
>> >@@ -5383,6 +5389,7 @@ static void ice_remove(struct pci_dev *pdev)
>> > ice_remove_arfs(pf);
>> >
>> > devl_lock(priv_to_devlink(pf));
>> >+ ice_dealloc_all_dynamic_ports(pf);
>> > ice_deinit_devlink(pf);
>> >
>> > ice_unload(pf);
>> >@@ -6677,7 +6684,7 @@ static int ice_up_complete(struct ice_vsi *vsi)
>> >
>> > if (vsi->port_info &&
>> > (vsi->port_info->phy.link_info.link_info & ICE_AQ_LINK_UP) &&
>> >- vsi->netdev && vsi->type == ICE_VSI_PF) {
>> >+ ((vsi->netdev && vsi->type == ICE_VSI_PF))) {
>>
>> I think this is some leftover, remove.
>>
>
>Yeah, thanks, will remove it.
>
>>
>> > ice_print_link_msg(vsi, true);
>> > netif_tx_start_all_queues(vsi->netdev);
>> > netif_carrier_on(vsi->netdev);
>> >--
>> >2.42.0
>> >
>> >
Powered by blists - more mailing lists