From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from vps0.lunn.ch (vps0.lunn.ch [156.67.10.101]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1D9C98635D; Wed, 5 Mar 2025 15:14:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=156.67.10.101 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741187675; cv=none; b=c4pxQgUbF0QA7VDJFRuJmQzwrWia26Z+vtR8l2CFuKpMQqv9JJ9Zxo6z3co1N4qwWQ7Fwc8mwZFZEW78Gm5tpPaokhc3/NqH2LltYS2+tLZ7mNcW02P95rQY4mt7LENHaZ990hzgCmFH4iadItjkMOW7yO/BM1y8S3LaTf0iPfM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1741187675; c=relaxed/simple; bh=oUCJHhTLkIkZYGtUC5LeznhRIkU1WBXDpdv7zgGdYkE=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=UnnzYTJV7iyqKWz+d6FcxhIq4WKi7T0PvSfcNmhyrZi3I0QhDYK70TQlRJuDZo66aa50QKkAqEKnCU/rPzCQWReMBesn6lh+pgsRTs73xTWoPT6n+K5jz0BROkDVtb9Eaq/hnWK18YG1lg5DkMv0klgWsaciVWj5T7cbtzKJSGI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch; spf=pass smtp.mailfrom=lunn.ch; dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b=hkfcbV6e; arc=none smtp.client-ip=156.67.10.101 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=lunn.ch Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=lunn.ch Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=lunn.ch header.i=@lunn.ch header.b="hkfcbV6e" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lunn.ch; s=20171124; h=In-Reply-To:Content-Disposition:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:From:Sender:Reply-To:Subject: Date:Message-ID:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding: Content-ID:Content-Description:Content-Disposition:In-Reply-To:References; bh=tdeNOf9UKgSka2fVLsKE2/PskNhMSwr2fUM0473Ddao=; b=hkfcbV6eMLZ4tDrXgg1MvCQFUM aRdyQaT18pOCXfCzvZGlMWpE6SSrJxRKlYMH3PFJvGGOtUGPvnrddmy2WuJlDGc0mC4H+fSSw9ERr KvfhT1+sI27asFVYASQBKiJF9VXAI/WwK2fngp8pU4FAzYzTA5+xfb2PwL8GbhV3S4fg=; Received: from andrew by vps0.lunn.ch with local (Exim 4.94.2) (envelope-from ) id 1tpqRu-002Vtc-LE; Wed, 05 Mar 2025 16:14:18 +0100 Date: Wed, 5 Mar 2025 16:14:18 +0100 From: Andrew Lunn To: Joseph Huang Cc: netdev@vger.kernel.org, Joseph Huang , Vladimir Oltean , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Guenter Roeck , linux-kernel@vger.kernel.org Subject: Re: [PATCH net] net: dsa: mv88e6xxx: Verify after ATU Load ops Message-ID: <2ea7cde2-2aa1-4ef4-a3ea-9991c1928d68@lunn.ch> References: <20250304235352.3259613-1-Joseph.Huang@garmin.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250304235352.3259613-1-Joseph.Huang@garmin.com> On Tue, Mar 04, 2025 at 06:53:51PM -0500, Joseph Huang wrote: > ATU Load operations could fail silently if there's not enough space > on the device to hold the new entry. > > Do a Read-After-Write verification after each fdb/mdb add operation > to make sure that the operation was really successful, and return > -ENOSPC otherwise. Please could you add a description of what the user sees when the ATU is full. What makes this a bug which needs fixing? I would of thought at least for unicast addresses, the switch has no entry for the destination, so sends the packet to the CPU. The CPU will then software bridge it out the correct port. Reporting ENOSPC will not change that. > @@ -2845,7 +2866,8 @@ static int mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port, > > mv88e6xxx_reg_lock(chip); > err = mv88e6xxx_port_db_load_purge(chip, port, addr, vid, > - MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC); > + MV88E6XXX_G1_ATU_DATA_STATE_UC_STATIC, > + true); > mv88e6xxx_reg_unlock(chip); > > return err; > @@ -6613,7 +6635,8 @@ static int mv88e6xxx_port_mdb_add(struct dsa_switch *ds, int port, > > mv88e6xxx_reg_lock(chip); > err = mv88e6xxx_port_db_load_purge(chip, port, mdb->addr, mdb->vid, > - MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC); > + MV88E6XXX_G1_ATU_DATA_STATE_MC_STATIC, > + true); > mv88e6xxx_reg_unlock(chip); This change seems bigger than what it needs to be. Rather than modify mv88e6xxx_port_db_load_purge(), why not perform the lookup just in these two functions via a helper? Andrew --- pw-bot: cr