Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Jul 2023 19:19:04 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Andrew Lunn <andrew@lunn.ch>,
        Eric Dumazet <edumazet@google.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        Woojung Huh <woojung.huh@microchip.com>,
        Arun Ramadoss <arun.ramadoss@microchip.com>,
        "Russell King (Oracle)" <linux@armlinux.org.uk>,
        kernel@pengutronix.de, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, UNGLinuxDriver@microchip.com,
        Petr Machata <petrm@nvidia.com>
Subject: Re: [PATCH net-next v2 1/1] net: dsa: microchip: Add partial ACL
 support for ksz9477 switches
Message-ID: <ZLl6mF8iKBD/OeHb@corigine.com>
References: <20230719111930.515070-1-o.rempel@pengutronix.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230719111930.515070-1-o.rempel@pengutronix.de>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?xs6MIFLC4PS9ySRFdonrXT5YFbUVtY5ZuS5sxnhNNfOxF9ri3Rsyc4v6GqxB?=
 =?us-ascii?Q?/yIfCjvsycIAMm9D5WMFhQo8RTljGipQJs0ZEo/DAjFmkIErF53F8DxP7QWo?=
 =?us-ascii?Q?jIDdps6epiXmAXIqi0uD+bnxBN2HrRDJxRSINd1Qbvx2TO8ReqUPF3iuDwJ/?=
 =?us-ascii?Q?prHCBWjxOXfCNwYvv6kAXPlE4aKNU1Cbrhj/if7jP7pXA4XwvGTYjsjq4ca1?=
 =?us-ascii?Q?rU52SeWHsXEgqw4v64sz4ytxGaHXt6aN+/9j4czkfngRbbweAFZREtoisfZ8?=
 =?us-ascii?Q?RUYvPAjWpyYn/YbzfCCQe8Ll3ICOecVtMLRtfn7tEsqhp6KyP7F1UXFI1nPx?=
 =?us-ascii?Q?rq1W7D7YRDjst8ZNJm0D2DbmfPz9SZAyYYzZyYVaoMWbdvsbqBWDL0+gDUCY?=
 =?us-ascii?Q?RAH4OfnsbtcUgSK5kZHFugY/La0vOo40a1YnkvFY67ujgzB5Dmf+MPZ6qZns?=
 =?us-ascii?Q?zolRJiZ4L+KhtNv+N4ipz2lQwT6M2oZvAjApw84bBlsFxaTwaEnss1T/UiQO?=
 =?us-ascii?Q?ed2bywgrCwcq4h1QLH1+wevxG6A75gjB0yItdgme5Xol/BI1SDehWztyLU4g?=
 =?us-ascii?Q?eisPErfPHUHSWvPCTN/7+W0NzWo21Dz6QZLve1aFhy+59RT3GxgIe7nArWs1?=
 =?us-ascii?Q?d54s+8N9nErcpptD6Lr+nHtQbTAckWr041aarKOU1Mxfx3iddN89GuAlAidz?=
 =?us-ascii?Q?f/wUHQPF9RZ1ZgbJuZMr2XtDIbJGGQ4G+gEBwks2ID5zhHM+sqtfIwVs7cDN?=
 =?us-ascii?Q?dpWPzRYjARi0vy9dp0m3upcJ9mdNFsWAHpoGt/aEtZqE/E0Sc/cjcv/G+/mY?=
 =?us-ascii?Q?L6Kop9oRH+3qmd6FjZKB0UdHscQp4fgqkIiLdmotxBFJZTAPTcyfxfpt0Nlz?=
 =?us-ascii?Q?367vtd9ox2ltrG+v6MyAiHtgq6iSUN0QYzSYYqmhFKl8MgFFGSKc5RGi9piC?=
 =?us-ascii?Q?HYSZivYbdK+lPyac6eVjms8E+7NzFB4VLWiQnfe1IsG7mWAmuuiFhWFtCv2/?=
 =?us-ascii?Q?iqv4Z95qb8IwqQiudvIdvxfNDWoYosoDlpj+67fxzKiWa04faXf/JVvJJcZD?=
 =?us-ascii?Q?bUcdde3ER1HGFvwdK8gdcCsGCYewzsz+wqg0z+a1DQA6JGdWYvE+BLjlnTiu?=
 =?us-ascii?Q?eAyZDaMqVc0MCrCve9E0+xgekWu/Kzg5M29qu1okAKsio+c+dS7paw55CMlU?=
 =?us-ascii?Q?2CdcDPOjrDv2X6TJD8yWI2RU0sEpW/d4fweXk9u2t/b06HQEJcJ8q5sxvLCm?=
 =?us-ascii?Q?D7Ehr+cYTJ0Cscnx+pWXVej+BfXEop97rJaYlfimCWzbGo6RwjTMhCEN7Dnz?=
 =?us-ascii?Q?XPLWxWF62PZNAOZVdi00ixwqKZdV8UBxGyZho1r6rcnYmuQVumph9qFwzmyD?=
 =?us-ascii?Q?ECe8NT6jYmX5yzFHrLd+zEBKyx6hFvX4eKpaowm8/BiHy/PGVfn6WqHHBr88?=
 =?us-ascii?Q?B2XYZZvKYiqmXT0AcD3kX7Et1g+d136qcPRF3xPm/gKxK12M3I4lmDFd7f0f?=
 =?us-ascii?Q?XmIGSL6Rh5DW4jsNPah9/i3nD+tJSbwUFQ7KRDBRS7sd7vgfuVymgsNwVOgV?=
 =?us-ascii?Q?Yvi/RmdEpujOkPUnPEVLczJg12gJavqiZj50kC/RH98cC6/FsFX0IxIEF/hj?=
 =?us-ascii?Q?fg=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b0c49f2e-9014-4c71-cc4c-08db894dd1f0
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jul 2023 18:19:13.3992
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: poeMcFDkTim8C4uUy0P48mLSmUaOojiM/FalYsCGXVE0XLvsf9V2tngUAZavrCG2mcoQbUc4YzINCVFFIrLfv+HDNIaDBmlSRDdxknhFuBs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR13MB3617
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Jul 19, 2023 at 01:19:29PM +0200, Oleksij Rempel wrote:

...

Hi Oleksij,

> +/**
> + * ksz9477_acl_port_enable - Enables ACL functionality on a given port.
> + * @dev: The ksz_device instance.
> + * @port: The port number on which to enable ACL functionality.
> + *
> + * This function enables ACL functionality on the specified port by configuring
> + * the appropriate control registers. It returns 0 if the operation is
> + * successful, or a negative error code if an error occurs.
> + *
> + * 0xn801 - KSZ9477S 5.2.8.2 Port Priority Control Register
> + *        Bit 7 - Highest Priority
> + *        Bit 6 - OR'ed Priority
> + *        Bit 4 - MAC Address Priority Classification
> + *        Bit 3 - VLAN Priority Classification
> + *        Bit 2 - 802.1p Priority Classification
> + *        Bit 1 - Diffserv Priority Classification
> + *        Bit 0 - ACL Priority Classification
> + *
> + * Note: current driver implementation sets 802.1p priority classification
> + * by default. In this function we add ACL priority classification with OR'ed
> + * priority. According to testing, priority set by ACL will supersede the
> + * 802.1p priority.
> + *
> + * 0xn803 - KSZ9477S 5.2.8.4 Port Authentication Control Register
> + *        Bit 2 - Access Control List (ACL) Enable
> + *        Bits 1:0 - Authentication Mode
> + *                00 = Reserved
> + *                01 = Block Mode. Authentication is enabled. When ACL is
> + *                     enabled, all traffic that misses the ACL rules is
> + *                     blocked; otherwise ACL actions apply.
> + *                10 = Pass Mode. Authentication is disabled. When ACL is
> + *                     enabled, all traffic that misses the ACL rules is
> + *                     forwarded; otherwise ACL actions apply.
> + *                11 = Trap Mode. Authentication is enabled. All traffic is
> + *                     forwarded to the host port. When ACL is enabled, all
> + *                     traffic that misses the ACL rules is blocked; otherwise
> + *                     ACL actions apply.
> + *
> + * Note: we are using Pass Mode here.

kernel-doc gcc-12 W=1 and clang-16 W=1 all complains
that there is a duplicate Note section in the above.

...

> +/**
> + * ksz9477_acl_matching_rule_cfg_l2 - Configure an ACL filtering entry to match
> + *				      L2 types of Ethernet frames
> + * @entry: Pointer to ACL entry buffer
> + * @ethertype: Ethertype value
> + * @eth_addr: Pointer to Ethernet address
> + * @is_src: If true, match the source MAC address; if false, match the
> + *	    destination MAC address
> + *
> + * This function configures an Access Control List (ACL) filtering
> + * entry to match Layer 2 types of Ethernet frames based on the provided
> + * ethertype and Ethernet address. Additionally, it can match either the source
> + * or destination MAC address depending on the value of the is_src parameter.
> + *
> + * Register Descriptions for MD = 01 and ENB != 00 (Layer 2 MAC header
> + * filtering)
> + *
> + * 0x01 - Mode and Enable
> + *        Bits 5:4 - MD (Mode)
> + *                01 = Layer 2 MAC header or counter filtering
> + *        Bits 3:2 - ENB (Enable)
> + *                01 = Comparison is performed only on the TYPE value
> + *                10 = Comparison is performed only on the MAC Address value
> + *                11 = Both the MAC Address and TYPE are tested
> + *        Bit  1   - S/D (Source / Destination)
> + *                0 = Destination address
> + *                1 = Source address
> + *        Bit  0   - EQ (Equal / Not Equal)
> + *                0 = Not Equal produces true result
> + *                1 = Equal produces true result
> + *
> + * 0x02-0x07 - MAC Address
> + *        0x02 - MAC Address [47:40]
> + *        0x03 - MAC Address [39:32]
> + *        0x04 - MAC Address [31:24]
> + *        0x05 - MAC Address [23:16]
> + *        0x06 - MAC Address [15:8]
> + *        0x07 - MAC Address [7:0]
> + *
> + * 0x08-0x09 - EtherType
> + *        0x08 - EtherType [15:8]
> + *        0x09 - EtherType [7:0]
> + */
> +static void ksz9477_acl_matching_rule_cfg_l2(u8 *entry, __be16 ethertype,
> +					     u8 *eth_addr, bool is_src)
> +{
> +	u8 enb = 0;
> +	u8 val;
> +
> +	if (ethertype)
> +		enb |= KSZ9477_ACL_ENB_L2_TYPE;
> +	if (eth_addr)
> +		enb |= KSZ9477_ACL_ENB_L2_MAC;
> +
> +	val = FIELD_PREP(KSZ9477_ACL_MD_MASK, KSZ9477_ACL_MD_L2_MAC) |
> +	      FIELD_PREP(KSZ9477_ACL_ENB_MASK, enb) |
> +	      FIELD_PREP(KSZ9477_ACL_SD_SRC, is_src) | KSZ9477_ACL_EQ_EQUAL;
> +	ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_1, val);
> +
> +	if (eth_addr) {
> +		int i;
> +
> +		for (i = 0; i < ETH_ALEN; i++) {
> +			ksz9477_acl_set_reg(entry,
> +					    KSZ9477_ACL_PORT_ACCESS_2 + i,
> +					    eth_addr[i]);
> +		}
> +	}
> +
> +	ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_8, ethertype & 0xff);
> +	ksz9477_acl_set_reg(entry, KSZ9477_ACL_PORT_ACCESS_9, ethertype >> 8);

ethertype is big endian.
But host byte order math is done on it in the two lines above.
This doesn't seem right.

...

> +/**
> + * ksz9477_flower_parse_action - Parse flow rule actions for a specified port
> + *				 on a ksz_device.
> + * @dev: The ksz_device instance.
> + * @port: The port number to parse the flow rule actions for.
> + * @extack: The netlink extended ACK for reporting errors.
> + * @cls: The flow_cls_offload instance containing the flow rule.
> + * @entry_idx: The index of the ACL entry to store the action.
> + *
> + * This function checks if the actions in the flow rule are supported by
> + * the device. Currently, only actions that change priorities are supported.
> + * If unsupported actions are encountered, an error message is set in the
> + * extended ACK.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +static int ksz9477_flower_parse_action(struct ksz_device *dev, int port,
> +				       struct netlink_ext_ack *extack,
> +				       struct flow_cls_offload *cls,
> +				       int entry_idx)
> +{
> +	struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
> +	struct ksz9477_acl_priv *acl = dev->ports[port].acl_priv;
> +	const struct flow_action_entry *act;
> +	struct ksz9477_acl_entry *entry;
> +	bool prio_force = false;
> +	u8 prio_val;
> +	int i;
> +
> +	if (TC_H_MIN(cls->classid)) {
> +		NL_SET_ERR_MSG_MOD(extack, "hw_tc is not supported. Use: action skbedit prio");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	flow_action_for_each(i, act, &rule->action) {
> +		switch (act->id) {
> +		case FLOW_ACTION_PRIORITY:
> +			if (act->priority > KSZ9477_MAX_TC) {
> +				NL_SET_ERR_MSG_MOD(extack, "Priority value is too high");
> +				return -EOPNOTSUPP;
> +			}
> +			prio_force = true;
> +			prio_val = act->priority;
> +			break;
> +		default:
> +			NL_SET_ERR_MSG_MOD(extack, "action not supported");
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	/* pick entry to store action */
> +	entry = &acl->acles.entries[entry_idx];
> +
> +	ksz9477_acl_action_rule_cfg(entry->entry, prio_force, prio_val);

If the flow_action_for_each loop iterates zero times then
prio_val will be uninitialised here. gcc-12 [-Wmaybe-uninitialized]
and Smatch warn about this. But I'm unsure if it can actually happen.

> +	ksz9477_acl_processing_rule_set_action(entry->entry, entry_idx);
> +
> +	return 0;
> +}

...