|SUCCESS| [v2] dts: clean cryptodev environment after a test run

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163906

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-11

Thank you for the patch. I have reviewed it for correctness, coding style,
and compliance with DPDK guidelines. Here are my findings:

commit [patch under review]
Author: Andrew Bailey <abailey at iol.unh.edu>

dts: clean cryptodev environment after a test run

Prior to this patch, the virtual functions created by DTS for crypto
devices were not properly cleanedu up. This could lead to a system to be
left in a bad state after running the cryptodev throughput test suite.
This patch properly cleans up the environment after cryptodev testing.

Bugzilla ID: 1923
Fixes: 8ee2df9da125 ("dts: add cryptodev package")

> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
> index ee943462c2..6e40552d21 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -181,6 +181,22 @@ def get_port_info(self, pci_address: str) -> PortInfo:
>  
>          return PortInfo(mac_address, logical_name, driver, is_link_up)
>  
> +    def unbind_ports(self, ports):
> +        """Overrides :meth:`~.os_session.OSSession.unbind_ports`.
> +
> +        The :attr:`~.devbind_script_path` property must be setup in order to call this method.
> +        """
> +        ports_pci_addrs = " ".join(port.pci for port in ports)
> +
> +        self.send_command(
> +            f"{self.devbind_script_path} -u --force {ports_pci_addrs}",
> +            privileged=True,
> +            verify=True,
> +        )
> +
> +        if self._lshw_net_info:
> +            del self._lshw_net_info

Does this implicit boolean comparison violate the coding style
requirements? The DPDK coding style requires explicit comparisons with
None for non-boolean types. Is _lshw_net_info a boolean or a collection
type?

If it's a collection or object, this should be:

    if self._lshw_net_info is not None:

Also, does this function handle the case when ports is an empty list? If
ports_pci_addrs becomes an empty string, does the devbind script handle
that gracefully or could it cause unexpected behavior?

> +
>      def bind_ports_to_driver(self, ports: list[Port], driver_name: str) -> None:
>          """Overrides :meth:`~.os_session.OSSession.bind_ports_to_driver`.

[ ... ]

> @@ -289,7 +305,18 @@ def create_vfs(self, pf_port: Port) -> None:
>              self.refresh_lshw()
>  
>      def delete_crypto_vfs(self, pf_port: Port) -> None:
> -        """Overrides :meth:`~.os_session.OSSession.delete_crypto_vfs`."""
> +        """Overrides :meth:`~.os_session.OSSession.delete_crypto_vfs`.
> +
> +        The :attr:`~.devbind_script_path` property must be setup in order to call this method.
> +        """
> +        if vfs := self.get_pci_addr_of_crypto_vfs(pf_port):
> +            vf_pci_addrs = " ".join(vf for vf in vfs)
> +            self.send_command(
> +                f"{self.devbind_script_path} -u --force {vf_pci_addrs}",
> +                privileged=True,
> +                verify=True,
> +            )
> +        self.unbind_ports([pf_port])

Does this code properly handle errors if unbind_ports() fails after the
VFs have been unbound? If send_command() with verify=True raises an
exception in the VF unbind block, does that prevent the pf_port from
being unbound, potentially leaving the system in an inconsistent state?

>          self.send_command(
>              f"echo 1 | sudo tee /sys/bus/pci/devices/{pf_port.pci}/remove".replace(":", "\\:"),
>              privileged=True,

Does this ordering make sense? The code unbinds the PF port and then
attempts to remove it via sysfs. Can the remove operation succeed if the
port is already unbound? Does the unbind operation need to happen before
the remove, or could removing the device while still bound cause issues?

> diff --git a/dts/framework/testbed_model/os_session.py b/dts/framework/testbed_model/os_session.py
> index 2c267afed1..f2dc9b20a9 100644
> --- a/dts/framework/testbed_model/os_session.py
> +++ b/dts/framework/testbed_model/os_session.py
> @@ -573,6 +573,14 @@ def get_port_info(self, pci_address: str) -> PortInfo:
>              ConfigurationError: If the port could not be found.
>          """
>  
> +    @abstractmethod
> +    def unbind_ports(self, ports: list[Port]) -> None:
> +        """Unbind `ports` from any driver.
> +
> +        Args:
> +            ports: The list of ports to unbind.
> +        """

Does this abstract method declaration match the implementation signature
in linux_session.py? The implementation has:

    def unbind_ports(self, ports):

without type annotations, while the abstract method has:

    def unbind_ports(self, ports: list[Port]) -> None:

Should the implementation include the type annotations to match the
abstract method signature?