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 6D6BF4576F; Thu, 8 Aug 2024 23:40:26 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 307A942D8C; Thu, 8 Aug 2024 23:40:26 +0200 (CEST) Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) by mails.dpdk.org (Postfix) with ESMTP id A1F5F42D89 for ; Thu, 8 Aug 2024 23:40:25 +0200 (CEST) Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-2cb566d528aso1214463a91.1 for ; Thu, 08 Aug 2024 14:40:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1723153225; x=1723758025; 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=RN1YyoDGX0iuAZu1wGNsi1hjQbCkkHe/6tT5hhnohWI=; b=NAHiNDY5WnzwXtl0stOE4dkWvGu4ABsFkbcNI6c+tr2m9uRz1klCc0wflu6Lq9DOcA JGfRmgelCM3wH0jcW/gK2ozqElALTXdPsb+scZ73M/2A2dFjZ3Hi1IUN7IrLvoxJ27rm b9aE5DmZvH5kAwK/J7ySSa0hjoC229SvkvRdU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723153225; x=1723758025; 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=RN1YyoDGX0iuAZu1wGNsi1hjQbCkkHe/6tT5hhnohWI=; b=W5WcBkIDdTN7c2H+333JVh4doQrpmX3IFuF+lV+pr/FcvofiP8+n3Lx8KhW9qBtyXd 6OVHdpuxXozsQvgWc/Jyk96bboeJ8fKiJn46kAwMUmQoxbQokmBUVGzyj2x4ONf7XYkD apYXRxSb41l1iKXOUKlx6uHhG6jNMGHB9k3oRTmtglPxEECD679ZyW8PZEIVMWwOzAeF VjOnsHHMM0JXAOOsUKV7lb3Rn2rdGNim/uWyq5PxRXjZ32mzn5r6LDGWrsCTS/UvVlAF PF1U6GL1ZjbS9TlppDDs/VtJUmIQr8n3EnMwB9APSUcHluLsZpbSgvLFjV7sABufnl05 tIWg== X-Forwarded-Encrypted: i=1; AJvYcCVpMrtiGWXp05KCsvRWCxFsaAiCu5OrOrBtFOLZJvD0Z5KPRcsQsni4U9246Do46hhq8pZ6U7i+JSqAPiY= X-Gm-Message-State: AOJu0Yyq4XD2gnfCkceQQm2zajz8LEuwETW4yTnVrb0qoNIMKfvUInxB 2BS7cqVQdon8DNTY1v5b0rE+Anndv3WVFe+2EucpNeKNWf6ETbbnz2jiwtgraaATE1ofaAtMxG6 GbpOOKrJtAZr3asvcp2qF5N3lOqP5jWPRfVR/oCbNBNylABn+ X-Google-Smtp-Source: AGHT+IELvSIwQTMzgTP08vKV9oL7Mw69wwjibHKQUN8+mkHhLZ1S76/g8jFY9aBCzVE/l4mJL8aYZ+wUXw43naYJ2rE= X-Received: by 2002:a17:90a:cc05:b0:2c9:7aea:2188 with SMTP id 98e67ed59e1d1-2d1c336ed3emr4020645a91.2.1723153224617; Thu, 08 Aug 2024 14:40:24 -0700 (PDT) MIME-Version: 1.0 References: <20240805171246.18580-1-npratte@iol.unh.edu> <20240805171246.18580-2-npratte@iol.unh.edu> In-Reply-To: <20240805171246.18580-2-npratte@iol.unh.edu> From: Jeremy Spewock Date: Thu, 8 Aug 2024 17:40:12 -0400 Message-ID: Subject: Re: [RFC v1 1/2] dts: add additional vlan configuration to testpmd shell class To: Nicholas Pratte Cc: Honnappa.Nagarahalli@arm.com, yoan.picchi@foss.arm.com, paul.szczepanek@arm.com, juraj.linkes@pantheon.tech, luca.vizzarro@arm.com, probb@iol.unh.edu, dmarx@iol.unh.edu, 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 Nick, Patch looks pretty good to me, just a few minor comments below. On Mon, Aug 5, 2024 at 1:13=E2=80=AFPM Nicholas Pratte wrote: > > The ethertype test suite requires many different internal runtime > vlan offload options to test ethertype configuration. The following patch > adds a new RxVlanOffloadOptions class that contains all the different > vlan configuration options available within testpmd. Additionally, an > extra method has beena added to adjust both the inner and outer tpids of Extra "a" here at the end of "been." > ingressing and egressing packets. > > Bugzilla ID: 1505 > > Signed-off-by: Nicholas Pratte > --- > + def set_vlan_tpid(self, port_id: int, tpid: int, inner_id: bool) -> = None: Is this one of those things that you can't really verify from testpmd output? That makes sense if it is, but we should probably note why this method is different in the doc-string. > + """Set ethertype tpid values using the ethdev api. > + > + Args: > + port_id: the ID of the port the tpid is being changed on. "the" should be capitalized here. > + tpid: The tpid value being changed on the port. > + inner_id: If :data:`True`, set the inner tpid to the specifi= ed value. If > + :data:`False`, change the outer tpid to the specified va= lue. > + > + Raises: > + InteractiveCommandExecutionError: If either `port_id` or `tp= id` value is invalid. > + """ > + if tpid < 0 or tpid > 65535: > + raise InteractiveCommandExecutionError("Invalid TPID value g= iven.") > + output =3D self.send_command( > + f'vlan set {"inner" if inner_id else "outer"} tpid {tpid} {p= ort_id}' > + ) > + if output.startswith("Invalid port"): > + raise InteractiveCommandExecutionError(f"Invalid port ID {po= rt_id} given.") > + > > + def set_vlan_offload_option( > + self, port: int, offload_option: VLANOffloadFlag, on: bool, veri= fy: bool =3D True > + ) -> None: > + """Enable extended vlans on the specified port. > + > + Args: > + port: The port number to use, should be within 0-32. > + on: If :data:`True`, extended vlan turn on, otherwise it tur= ns off. > + offload_options: The rx vlan offload option to be set. > + verify: If :data:`True`, the output of the command and show = port info > + is scanned to verify that vlan stripping was enabled on = the specified port. > + If not, it is considered an error. > + > + Raises: > + InteractiveCommandExecutionError: If `verify` is :data:`True= ` and stripping > + fails to update. > + """ > + offload_output =3D self.send_command( > + f"vlan set {str(offload_option).lower()} {'on' if on else 'o= ff'} {port}" Because flags can be combinations of multiple values, this might not work if you passed in a flag that had STRIP and FILTER turned on at the same time, for example. I think the string version would come out to be something like STRIP|FILTER which definitely wouldn't be what you want. Unfortunately, I think the only way around that though is to have conditional values for each option and then checking to see if the flag contains that option. Something like if VLANOffloadFlag.STRIP in options: self.send_command(vlan set strip on 0) Obviously with proper string formatting. Although, another thing you might be able to do is if you can iterate through the members of the VLANOffloadFlag class, you could do something more like: for offload in VLANOffloadFlag.__members__: if offload in options: self.send_command(f"vlan set {offload.name.lower()} on 0") And that would be a shorter way that covers you for any combination of offloads. Could be fancy and make it a one-liner too if you wanted to, but that's up to you. > + ) > + if verify: > + if on ^ (str(offload_option) in str(self.show_port_info(port= _id=3Dport).vlan_offload)): Is there a reason you can't directly compare the flags? The different possible combinations of options will all have unique values, so I think you should just be able to do `offload_option =3D=3D self.show_port_info(port_id=3Dport).vlan_offload)` > + self._logger.debug( > + f"""Failed to set {offload_option} {'on' if on else = 'off'} port {port}: > + \n{offload_output} > + """ > + ) > + raise InteractiveCommandExecutionError( > + f"Testpmd failed to set {offload_option} {'on' if on= else 'off'} port {port}." > + ) > + > def port_stop_all(self, verify: bool =3D True) -> None: > """Stop all ports. > > -- > 2.44.0 >