|SUCCESS| [v1] 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/163793

_AI Code Review_

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

Reviewing patch from Andrew Bailey <abailey at iol.unh.edu>:

Subject: [PATCH v1] dts: clean cryptodev environment after a test run

The patch addresses cleanup of virtual functions created for crypto
devices in the DTS framework to prevent leaving the system in a bad
state after testing.

> diff --git a/dts/framework/testbed_model/linux_session.py b/dts/framework/testbed_model/linux_session.py
> index ee943462c2..a5bb34b4e8 100644
> --- a/dts/framework/testbed_model/linux_session.py
> +++ b/dts/framework/testbed_model/linux_session.py
> @@ -181,6 +181,21 @@ def get_port_info(self, pci_address: str) -> PortInfo:
>  
>          return PortInfo(mac_address, logical_name, driver, is_link_up)
>  
> +    def unbind_ports(self, ports):

The parameter lacks a type annotation. Python typing should specify
ports: list[Port] to match the abstract method signature.

> +        """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,
> +        )
> +
> +        del self._lshw_net_info

Does this code handle the case when ports is an empty list? The
send_command would execute with an empty ports_pci_addrs string, which
might succeed but seems unintended.

> +
>      def bind_ports_to_driver(self, ports: list[Port], driver_name: str) -> None:
>          """Overrides :meth:`~.os_session.OSSession.bind_ports_to_driver`.
>  
> @@ -289,7 +304,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])

If get_pci_addr_of_crypto_vfs() or send_command() raises an exception,
does unbind_ports() still execute? The pf_port unbind happens regardless
of VF unbind success, but if the VF unbind fails with verify=True, does
this prevent cleanup of the PF?

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

If the unbind_ports() call above fails, does this leave the VFs bound to
their driver while attempting to remove the PF device? The send_command
might fail in that scenario.

> 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.
> +        """
> +
>      @abstractmethod
>      def bind_ports_to_driver(self, ports: list[Port], driver_name: str) -> None:
>          """Bind `ports` to the given `driver_name`.

The abstract method definition is fine, but the concrete implementation
in linux_session.py should match this signature exactly with proper type
hints.