From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2755545906; Wed, 4 Sep 2024 23:19:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CB6824025C; Wed, 4 Sep 2024 23:19:01 +0200 (CEST) Received: from mail-pj1-f49.google.com (mail-pj1-f49.google.com [209.85.216.49]) by mails.dpdk.org (Postfix) with ESMTP id 8FF0940151 for ; Wed, 4 Sep 2024 23:18:59 +0200 (CEST) Received: by mail-pj1-f49.google.com with SMTP id 98e67ed59e1d1-2d88c0f8e79so8506a91.3 for ; Wed, 04 Sep 2024 14:18:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1725484739; x=1726089539; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=WMIwCNsqGs9pGpfRyRU2rYpeUlmGNCDZ7p0UVOzS7TA=; b=fiPSaWT914QGH5gW81+rd+iaCmn7Hus37Vzy39HgKfCehOM+w0it66Ffs3ln9XaypT c7CTL2Lmb0x1t9g0F7VizNb5lQwoQhIpcxRRmKbWH/NrFfaGQ2Mf6q5/Qs2L1tBU0y6z VZ7occvQQKpx8pZAVU+EoF/h6F5T3CyWOPpfY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725484739; x=1726089539; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=WMIwCNsqGs9pGpfRyRU2rYpeUlmGNCDZ7p0UVOzS7TA=; b=Dbvrek3keHvwXSxPAA2qQlIanJR9AjxbdeHG4IWlFrhgTMJBASp/wP9Ab0eybf0UhM Kcb8t2LmBoQY8enoXrvPUp/wGekzjZtknCX8QvAUcYB8u//vYTlzBHTJeCi6IqLJH+kC sX525fZvaQj8Ru8Ulnt3pJzgye1dX+gdoaum43gL9PyjXgjHJFtnQtR3dN5P5ogg1xIT FSPWHMorSGLzO6+GfdCUbhoryykuTqSMguStLHUBuIxopyf1viRXN9vMnud/54RHlmmL paU8kphgDeO57OK9ooaq9EPHrFOQYYcjXvyiNuz8/1ULncCKp8ez3xtMZvBMbAtjttIg beDg== X-Forwarded-Encrypted: i=1; AJvYcCVEmdPV+8T0Suvi2lr2mpBo2H+QwGrU2IF9AnXTMk5Ld4ZG84tmdK5ffnxwebTp6pAxdAc=@dpdk.org X-Gm-Message-State: AOJu0YwNnPB5dHHiyIdYRQMm33oWtq35zIjiUzYnJVaAJVu1LQHbDWXk C3xuyfDZZZanJU8w6mByv6Ug1yeas3pi6+cRpktu0PpnXjT1A/DNYYSrVC2KVpr/aWmdKMtRD02 s7v1dnDP4TLzCe1BicATIsuemx1nOhcvbLOPJ6Q== X-Google-Smtp-Source: AGHT+IEzBmkIdZK5o0mowag2r10KqMclmL+jcPIN2ejd2nTqbNDsUTXhH4IpYLHoXenA/2uDCzivz38ObUUbiZaZbaI= X-Received: by 2002:a17:90b:3804:b0:2d8:53f8:77c0 with SMTP id 98e67ed59e1d1-2da7482cc94mr6390715a91.7.1725484738517; Wed, 04 Sep 2024 14:18:58 -0700 (PDT) MIME-Version: 1.0 References: <20240821162550.1163-1-dmarx@iol.unh.edu> <20240826212250.26993-1-dmarx@iol.unh.edu> <20240826212250.26993-2-dmarx@iol.unh.edu> In-Reply-To: <20240826212250.26993-2-dmarx@iol.unh.edu> From: Jeremy Spewock Date: Wed, 4 Sep 2024 17:18:47 -0400 Message-ID: Subject: Re: [PATCH v3 1/2] dts: add csum HW offload to testpmd shell To: Dean Marx Cc: probb@iol.unh.edu, npratte@iol.unh.edu, luca.vizzarro@arm.com, yoan.picchi@foss.arm.com, Honnappa.Nagarahalli@arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hey Dean, This patch is looking good, I like the changes from the previous version, there were just a few comments that I left but they should be pretty easy fixes. On Mon, Aug 26, 2024 at 5:22=E2=80=AFPM Dean Marx wrote= : > + def csum_set_hw(self, layer: ChecksumOffloadOptions, port_id: int, v= erify: bool =3D True) -> None: > + """Enables hardware checksum offloading on the specified layer. > + > + Args: > + layer: The layer that checksum offloading should be enabled = on. > + port_id: The port number to enable checksum offloading on, s= hould be within 0-32. > + verify: If :data:`True` the output of the command will be sc= anned in an attempt to > + verify that checksum offloading was enabled on the port. > + > + Raises: > + InteractiveCommandExecutionError: If checksum offload is not= enabled successfully. > + """ > + for name, offload in ChecksumOffloadOptions.__members__.items(): > + if offload in layer: > + name =3D name.replace("_", "-") > + csum_output =3D self.send_command(f"csum set {name} hw {= port_id}") > + if verify: I think this line and all of the ones below it in this method need to be indented one more time so that they are only done if the offload is one that the user is trying to set. csum_output might not exist if the previous statement is false, so I don't think we would be able to even use it in that case, but even if we could I don't think we really need to verify if the offload is set or not if it isn't in `layer`. > + if ("Bad arguments" in csum_output > + or f"Please stop port {port_id} first" in csum_outpu= t > + or f"checksum offload is not supported by port {= port_id}" in csum_output): > + self._logger.debug(f"Csum set hw error:\n{csum_outpu= t}") > + raise InteractiveCommandExecutionError( > + f"Failed to set csum hw {name} mode on port {por= t_id}" > + ) > + success =3D False > + if "-" in name: > + name.title() For both this and the call to upper() below, they return a new string that has the results applied, so `name` here would have to get re-assigned for the changes to take place. So I think this line would need to be `name =3D name.title()`. > + else: > + name.upper() > + if f"{name} checksum offload is hw" in csum_output: Another thing you could do which I should have thought about sooner is you could just change this if-statement to be: if f"{name} checksum offload is hw" in csum_output.lower(): and then you wouldn't need to worry at all about which letters are uppercase or lowercase in the output and it would all be the same so you don't have to do this either upper() or title() call. It might be technically less efficient, but we've mentioned before that we're fine with that as long as it isn't grossly inefficient, so if it is easier to just make it all lowercase then I think it is fine :). > + success =3D True > + if not success and verify: > + self._logger.debug(f"Failed to set csum hw mode on port = {port_id}:\n{csum_output}") Should we also raise the exception here as a failure? > + > def _close(self) -> None: > """Overrides :meth:`~.interactive_shell.close`.""" > self.stop() > -- > 2.44.0 >