Message ID | 20221215144536.3810578-1-lukma@denx.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:e747:0:0:0:0:0 with SMTP id c7csp403059wrn; Thu, 15 Dec 2022 06:52:37 -0800 (PST) X-Google-Smtp-Source: AA0mqf71X/h8pgBm1hpNXy05wcmoyB4DFaAlCJxjn3fJSgo69NDP6FS+fMJrhlKQ4RXfSrVpCXg7 X-Received: by 2002:a05:6402:378c:b0:45c:835b:ac66 with SMTP id et12-20020a056402378c00b0045c835bac66mr23368878edb.33.1671115957459; Thu, 15 Dec 2022 06:52:37 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1671115957; cv=none; d=google.com; s=arc-20160816; b=INkvg042tOAld6rGRg3Qbz+j/gNza0Y5vgU55xMG3C18sU9l2KPxH4fdXb+46dFoc1 PymYZCBFKhJd2hEe1QHyxeMbsjFGrkarm2n3bIl6+yiQKm+ONUBlBjhvtoDBtZ9x4O+o YoIg1IVmyYf49+QmQjh0OLdfoGIXbqevBRbqJwcGM38Qqm3wA3ElyOUOmjBMrlBztD/0 vwaZbe2h7SgHSN24UvtqW4YUKbum4fGGZrYchC78KtJxAWFpbj3QzwI+PbgbhigjyEwf bUJa5m/5qinJo2QskEj3ajPwaFrIi9fw8RsGkBfbZaVmlejvkHfB+svd6Ku6/Ofq7ZYq TXEg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=D6p/BoRStqcnYa2Wb2pX3TOnQiiewV652kz213LodFw=; b=qLqgA8Rvs6yxmorLn3SZ+tklTYOEFUc+BsBej+ThwYAnq9qfQNyvKP3r/e6ASxw0M3 6ePMB0DTtseJm1XnIvZRtE8yXPZrvEsvXDFBLaEln7C7tTf9inaLC0wtegsPwEmdxlDh Ys5keRU50yn3i41kPSwxTwngc8SgMSR/v0oX84iORca+yqJnmO8oDDQ7b6T2KuHYAIjl Gn9nUWKM8nH93iloIdB377RXbbUBu5vwdWZnBQ5rYfhJG49QQHaG0XNSK42XqJyCjW3q e5pIAKwTIT35ymkj8B3AsP6Hab8FiFzRPReWGztL4ATKDUZrfgCakNSolV2IjHgmyTsr iT9w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@denx.de header.s=phobos-20191101 header.b=t1PLd21w; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id dr14-20020a170907720e00b0078d484e0e7esi14460960ejc.488.2022.12.15.06.52.13; Thu, 15 Dec 2022 06:52:37 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@denx.de header.s=phobos-20191101 header.b=t1PLd21w; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229854AbiLOOp7 (ORCPT <rfc822;jeantsuru.cumc.mandola@gmail.com> + 99 others); Thu, 15 Dec 2022 09:45:59 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56286 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229591AbiLOOpz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 15 Dec 2022 09:45:55 -0500 Received: from phobos.denx.de (phobos.denx.de [IPv6:2a01:238:438b:c500:173d:9f52:ddab:ee01]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2CB56DB7; Thu, 15 Dec 2022 06:45:54 -0800 (PST) Received: from localhost.localdomain (85-222-111-42.dynamic.chello.pl [85.222.111.42]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: lukma@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id CCDA684EC2; Thu, 15 Dec 2022 15:45:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1671115552; bh=D6p/BoRStqcnYa2Wb2pX3TOnQiiewV652kz213LodFw=; h=From:To:Cc:Subject:Date:From; b=t1PLd21w/M31Q7JwTcARq2C0azSKCcIypJ7fO89oRB/KxMygNa3zorGmSoZFWmowT ja5T1UuDwLVjf5kgSWxIpJghyBvA67nLS0IOxIv9QHN3dlOCHUPh0GbTKrVQ3PnzrX 8rk75nbegTRW3MyCktlzF50ALdQKujkdE5CltPdxuL8CCxR3T2Pvpp0HSS1QEWQNsl xEnvzjBNvWXE8ozR37dzy8cCN5K/ehL7c5q2TWKulLscUKfgai2WiMYdR0CyTLlDLw EfSHe8lO0aINZwHMQtdYD7SwerczYmqEBm0A5Zn+Cg2Ce52vAf3tEQbnOZrybo3fUS n99ayP1PFsDgw== From: Lukasz Majewski <lukma@denx.de> To: Andrew Lunn <andrew@lunn.ch>, Vladimir Oltean <olteanv@gmail.com> Cc: Vivien Didelot <vivien.didelot@gmail.com>, Florian Fainelli <f.fainelli@gmail.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Lukasz Majewski <lukma@denx.de> Subject: [PATCH v2 1/3] dsa: marvell: Provide per device information about max frame size Date: Thu, 15 Dec 2022 15:45:34 +0100 Message-Id: <20221215144536.3810578-1-lukma@denx.de> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_NONE, SPF_PASS 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1752292086378520205?= X-GMAIL-MSGID: =?utf-8?q?1752292086378520205?= |
Series |
[v2,1/3] dsa: marvell: Provide per device information about max frame size
|
|
Commit Message
Lukasz Majewski
Dec. 15, 2022, 2:45 p.m. UTC
Different Marvell DSA switches support different size of max frame
bytes to be sent.
For example mv88e6185 supports max 1632 bytes, which is now in-driver
standard value. On the other hand - mv88e6250 supports 2048 bytes.
As this value is internal and may be different for each switch IC,
new entry in struct mv88e6xxx_info has been added to store it.
Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Define max_frame_size with default value of 1632 bytes,
- Set proper value for the mv88e6250 switch SoC (linkstreet) family
---
drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
drivers/net/dsa/mv88e6xxx/chip.h | 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
Comments
On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote: > Different Marvell DSA switches support different size of max frame > bytes to be sent. > > For example mv88e6185 supports max 1632 bytes, which is now in-driver > standard value. On the other hand - mv88e6250 supports 2048 bytes. > > As this value is internal and may be different for each switch IC, > new entry in struct mv88e6xxx_info has been added to store it. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > Changes for v2: > - Define max_frame_size with default value of 1632 bytes, > - Set proper value for the mv88e6250 switch SoC (linkstreet) family > --- > drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- > drivers/net/dsa/mv88e6xxx/chip.h | 1 + > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 2ca3cbba5764..7ae4c389ce50 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) > if (chip->info->ops->port_set_jumbo_size) > return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > else if (chip->info->ops->set_max_frame_size) > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > + return (chip->info->max_frame_size - VLAN_ETH_HLEN > + - EDSA_HLEN - ETH_FCS_LEN); > + > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > } > > Is there any specific reason for triggering this based on the existance of the function call? Why not just replace: else if (chip->info->ops->set_max_frame_size) with: else if (chip->info->max_frame_size) Otherwise my concern is one gets defined without the other leading to a future issue as 0 - extra headers will likely wrap and while the return value may be a signed int, it is usually stored in an unsigned int so it would effectively uncap the MTU. Actually you could take this one step further since all values should be 1522 or greater you could just drop the else/if and replace the last line with "max_t(int, chip->info->max_frame_size, 1522) - (headers)".
On Thu, 15 Dec 2022 15:45:34 +0100 Lukasz Majewski wrote: > Different Marvell DSA switches support different size of max frame > bytes to be sent. > > For example mv88e6185 supports max 1632 bytes, which is now in-driver > standard value. On the other hand - mv88e6250 supports 2048 bytes. > > As this value is internal and may be different for each switch IC, > new entry in struct mv88e6xxx_info has been added to store it. # Form letter - net-next is closed We have already submitted the networking pull request to Linus for v6.2 and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after Jan 2nd. RFC patches sent for review only are obviously welcome at any time.
Hi Jakub, > On Thu, 15 Dec 2022 15:45:34 +0100 Lukasz Majewski wrote: > > Different Marvell DSA switches support different size of max frame > > bytes to be sent. > > > > For example mv88e6185 supports max 1632 bytes, which is now > > in-driver standard value. On the other hand - mv88e6250 supports > > 2048 bytes. > > > > As this value is internal and may be different for each switch IC, > > new entry in struct mv88e6xxx_info has been added to store it. > > # Form letter - net-next is closed > I see.... > We have already submitted the networking pull request to Linus > for v6.2 and therefore net-next is closed for new drivers, features, > code refactoring and optimizations. We are currently accepting > bug fixes only. Ok. > > Please repost when net-next reopens after Jan 2nd. > > RFC patches sent for review only are obviously welcome at any time. I hope that the discussion regarding those patches will be done by this time. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Alexander, > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote: > > Different Marvell DSA switches support different size of max frame > > bytes to be sent. > > > > For example mv88e6185 supports max 1632 bytes, which is now > > in-driver standard value. On the other hand - mv88e6250 supports > > 2048 bytes. > > > > As this value is internal and may be different for each switch IC, > > new entry in struct mv88e6xxx_info has been added to store it. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > Changes for v2: > > - Define max_frame_size with default value of 1632 bytes, > > - Set proper value for the mv88e6250 switch SoC (linkstreet) family > > --- > > drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- > > drivers/net/dsa/mv88e6xxx/chip.h | 1 + > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50 > > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct > > dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size) > > return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - > > ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size) > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - > > ETH_FCS_LEN; > > + return (chip->info->max_frame_size - VLAN_ETH_HLEN > > + - EDSA_HLEN - ETH_FCS_LEN); > > + > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > } > > > > > > Is there any specific reason for triggering this based on the > existance of the function call? This was the original code in this driver. This value (1632 or 2048 bytes) is SoC (family) specific. By checking which device defines set_max_frame_size callback, I could fill the chip->info->max_frame_size with 1632 value. > Why not just replace: > else if (chip->info->ops->set_max_frame_size) > with: > else if (chip->info->max_frame_size) > I think that the callback check is a bit "defensive" approach -> 1522B is the default value and 1632 (or 10240 - jumbo) can be set only when proper callback is defined. > Otherwise my concern is one gets defined without the other leading to > a future issue as 0 - extra headers will likely wrap and while the > return value may be a signed int, it is usually stored in an unsigned > int so it would effectively uncap the MTU. Please correct me if I misunderstood something: The problem is with new mv88eXXXX devices, which will not provide max_frame_size information to their chip->info struct? Or is there any other issue? > > Actually you could take this one step further since all values should > be 1522 or greater you could just drop the else/if and replace the > last line with "max_t(int, chip->info->max_frame_size, 1522) - > (headers)". This would be possible, yes. However, then we will not check if the proper callback (e.g. chip->info->ops->set_max_frame_size) is available for specific SoC. If this is OK for maintainers for this driver, then I don't mind. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de> wrote: > > Hi Alexander, > > > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote: > > > Different Marvell DSA switches support different size of max frame > > > bytes to be sent. > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > in-driver standard value. On the other hand - mv88e6250 supports > > > 2048 bytes. > > > > > > As this value is internal and may be different for each switch IC, > > > new entry in struct mv88e6xxx_info has been added to store it. > > > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > --- > > > Changes for v2: > > > - Define max_frame_size with default value of 1632 bytes, > > > - Set proper value for the mv88e6250 switch SoC (linkstreet) family > > > --- > > > drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- > > > drivers/net/dsa/mv88e6xxx/chip.h | 1 + > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > > b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50 > > > 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c > > > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > > > @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct > > > dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size) > > > return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - > > > ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size) > > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - > > > ETH_FCS_LEN; > > > + return (chip->info->max_frame_size - VLAN_ETH_HLEN > > > + - EDSA_HLEN - ETH_FCS_LEN); > > > + > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > } > > > > > > > > > > Is there any specific reason for triggering this based on the > > existance of the function call? > > This was the original code in this driver. > > This value (1632 or 2048 bytes) is SoC (family) specific. > > By checking which device defines set_max_frame_size callback, I could > fill the chip->info->max_frame_size with 1632 value. > > > Why not just replace: > > else if (chip->info->ops->set_max_frame_size) > > with: > > else if (chip->info->max_frame_size) > > > > I think that the callback check is a bit "defensive" approach -> 1522B > is the default value and 1632 (or 10240 - jumbo) can be set only when > proper callback is defined. > > > Otherwise my concern is one gets defined without the other leading to > > a future issue as 0 - extra headers will likely wrap and while the > > return value may be a signed int, it is usually stored in an unsigned > > int so it would effectively uncap the MTU. > > Please correct me if I misunderstood something: > > The problem is with new mv88eXXXX devices, which will not provide > max_frame_size information to their chip->info struct? > > Or is there any other issue? That was mostly my concern. I was adding a bit of my own defensive programming in the event that somebody forgot to fill out the chip->info. If nothing else it might make sense to add a check to verify that the max_frame_size is populated before blindly using it. So perhaps you could do something similar to the max_t approach I had called out earlier but instead of applying it on the last case you could apply it for the "set_max_frame_size" case with 1632 being the minimum and being overwritten by 2048 if it is set in max_frame_size.
Hi Alexander, > On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > Hi Alexander, > > > > > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote: > > > > Different Marvell DSA switches support different size of max > > > > frame bytes to be sent. > > > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > > in-driver standard value. On the other hand - mv88e6250 supports > > > > 2048 bytes. > > > > > > > > As this value is internal and may be different for each switch > > > > IC, new entry in struct mv88e6xxx_info has been added to store > > > > it. > > > > > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > --- > > > > Changes for v2: > > > > - Define max_frame_size with default value of 1632 bytes, > > > > - Set proper value for the mv88e6250 switch SoC (linkstreet) > > > > family --- > > > > drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- > > > > drivers/net/dsa/mv88e6xxx/chip.h | 1 + > > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > > > b/drivers/net/dsa/mv88e6xxx/chip.c index > > > > 2ca3cbba5764..7ae4c389ce50 100644 --- > > > > a/drivers/net/dsa/mv88e6xxx/chip.c +++ > > > > b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3093,7 +3093,9 @@ static > > > > int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) if > > > > (chip->info->ops->port_set_jumbo_size) return 10240 - > > > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; else if > > > > (chip->info->ops->set_max_frame_size) > > > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - > > > > ETH_FCS_LEN; > > > > + return (chip->info->max_frame_size - VLAN_ETH_HLEN > > > > + - EDSA_HLEN - ETH_FCS_LEN); > > > > + > > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > } > > > > > > > > > > > > > > Is there any specific reason for triggering this based on the > > > existance of the function call? > > > > This was the original code in this driver. > > > > This value (1632 or 2048 bytes) is SoC (family) specific. > > > > By checking which device defines set_max_frame_size callback, I > > could fill the chip->info->max_frame_size with 1632 value. > > > > > Why not just replace: > > > else if (chip->info->ops->set_max_frame_size) > > > with: > > > else if (chip->info->max_frame_size) > > > > > > > I think that the callback check is a bit "defensive" approach -> > > 1522B is the default value and 1632 (or 10240 - jumbo) can be set > > only when proper callback is defined. > > > > > Otherwise my concern is one gets defined without the other > > > leading to a future issue as 0 - extra headers will likely wrap > > > and while the return value may be a signed int, it is usually > > > stored in an unsigned int so it would effectively uncap the MTU. > > > > Please correct me if I misunderstood something: > > > > The problem is with new mv88eXXXX devices, which will not provide > > max_frame_size information to their chip->info struct? > > > > Or is there any other issue? > > That was mostly my concern. I was adding a bit of my own defensive > programming in the event that somebody forgot to fill out the > chip->info. If nothing else it might make sense to add a check to > verify that the max_frame_size is populated before blindly using it. > So perhaps you could do something similar to the max_t approach I had > called out earlier but instead of applying it on the last case you > could apply it for the "set_max_frame_size" case with 1632 being the > minimum and being overwritten by 2048 if it is set in max_frame_size. I think that I shall add: else if (chip->info->ops->set_max_frame_size) return max_t(int, chip->info->max_frame_size, 1632) - (headers) So then the "default" value of 1632 will be overwritten by 2048 bytes. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Alexander, > Hi Alexander, > > > On Fri, Dec 16, 2022 at 5:05 AM Lukasz Majewski <lukma@denx.de> > > wrote: > > > > > > Hi Alexander, > > > > > > > On Thu, 2022-12-15 at 15:45 +0100, Lukasz Majewski wrote: > > > > > Different Marvell DSA switches support different size of max > > > > > frame bytes to be sent. > > > > > > > > > > For example mv88e6185 supports max 1632 bytes, which is now > > > > > in-driver standard value. On the other hand - mv88e6250 > > > > > supports 2048 bytes. > > > > > > > > > > As this value is internal and may be different for each switch > > > > > IC, new entry in struct mv88e6xxx_info has been added to store > > > > > it. > > > > > > > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > > --- > > > > > Changes for v2: > > > > > - Define max_frame_size with default value of 1632 bytes, > > > > > - Set proper value for the mv88e6250 switch SoC (linkstreet) > > > > > family --- > > > > > drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++- > > > > > drivers/net/dsa/mv88e6xxx/chip.h | 1 + > > > > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c > > > > > b/drivers/net/dsa/mv88e6xxx/chip.c index > > > > > 2ca3cbba5764..7ae4c389ce50 100644 --- > > > > > a/drivers/net/dsa/mv88e6xxx/chip.c +++ > > > > > b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3093,7 +3093,9 @@ > > > > > static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int > > > > > port) if (chip->info->ops->port_set_jumbo_size) return 10240 - > > > > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; else if > > > > > (chip->info->ops->set_max_frame_size) > > > > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - > > > > > ETH_FCS_LEN; > > > > > + return (chip->info->max_frame_size - > > > > > VLAN_ETH_HLEN > > > > > + - EDSA_HLEN - ETH_FCS_LEN); > > > > > + > > > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; > > > > > } > > > > > > > > > > > > > > > > > > Is there any specific reason for triggering this based on the > > > > existance of the function call? > > > > > > This was the original code in this driver. > > > > > > This value (1632 or 2048 bytes) is SoC (family) specific. > > > > > > By checking which device defines set_max_frame_size callback, I > > > could fill the chip->info->max_frame_size with 1632 value. > > > > > > > Why not just replace: > > > > else if (chip->info->ops->set_max_frame_size) > > > > with: > > > > else if (chip->info->max_frame_size) > > > > > > > > > > I think that the callback check is a bit "defensive" approach -> > > > 1522B is the default value and 1632 (or 10240 - jumbo) can be set > > > only when proper callback is defined. > > > > > > > Otherwise my concern is one gets defined without the other > > > > leading to a future issue as 0 - extra headers will likely wrap > > > > and while the return value may be a signed int, it is usually > > > > stored in an unsigned int so it would effectively uncap the > > > > MTU. > > > > > > Please correct me if I misunderstood something: > > > > > > The problem is with new mv88eXXXX devices, which will not provide > > > max_frame_size information to their chip->info struct? > > > > > > Or is there any other issue? > > > > That was mostly my concern. I was adding a bit of my own defensive > > programming in the event that somebody forgot to fill out the > > chip->info. If nothing else it might make sense to add a check to > > verify that the max_frame_size is populated before blindly using it. > > So perhaps you could do something similar to the max_t approach I > > had called out earlier but instead of applying it on the last case > > you could apply it for the "set_max_frame_size" case with 1632 > > being the minimum and being overwritten by 2048 if it is set in > > max_frame_size. > > I think that I shall add: > > else if (chip->info->ops->set_max_frame_size) > return max_t(int, chip->info->max_frame_size, 1632) - > (headers) > > So then the "default" value of 1632 will be overwritten by 2048 bytes. > Is this approach acceptable for you? > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 2ca3cbba5764..7ae4c389ce50 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3093,7 +3093,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size) return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size) - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; + return (chip->info->max_frame_size - VLAN_ETH_HLEN + - EDSA_HLEN - ETH_FCS_LEN); + return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; } @@ -4461,6 +4463,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = { .avb_ops = &mv88e6352_avb_ops, .ptp_ops = &mv88e6250_ptp_ops, .phylink_validate = mv88e6065_phylink_validate, + .set_max_frame_size = mv88e6185_g1_set_max_frame_size, }; static const struct mv88e6xxx_ops mv88e6290_ops = { @@ -5060,6 +5063,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .atu_move_port_mask = 0xf, .multi_chip = true, .ops = &mv88e6095_ops, + .max_frame_size = 1632, }, [MV88E6097] = { @@ -5083,6 +5087,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ops = &mv88e6097_ops, + .max_frame_size = 1632, }, [MV88E6123] = { @@ -5106,6 +5111,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ops = &mv88e6123_ops, + .max_frame_size = 1632, }, [MV88E6131] = { @@ -5174,6 +5180,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ptp_support = true, .ops = &mv88e6161_ops, + .max_frame_size = 1632, }, [MV88E6165] = { @@ -5197,6 +5204,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .ptp_support = true, .ops = &mv88e6165_ops, + .max_frame_size = 1632, }, [MV88E6171] = { @@ -5312,6 +5320,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .multi_chip = true, .edsa_support = MV88E6XXX_EDSA_SUPPORTED, .ops = &mv88e6185_ops, + .max_frame_size = 1632, }, [MV88E6190] = { @@ -5440,6 +5449,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_internal_phys = 2, .invalid_port_mask = BIT(2) | BIT(3) | BIT(4), .max_vid = 4095, + .max_frame_size = 2048, .port_base_addr = 0x08, .phy_base_addr = 0x00, .global1_addr = 0x0f, @@ -5486,6 +5496,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = { .num_ports = 7, .num_internal_phys = 5, .max_vid = 4095, + .max_frame_size = 2048, .port_base_addr = 0x08, .phy_base_addr = 0x00, .global1_addr = 0x0f, diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h index 80dc7b549e81..9712b10fc4ed 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.h +++ b/drivers/net/dsa/mv88e6xxx/chip.h @@ -130,6 +130,7 @@ struct mv88e6xxx_info { unsigned int num_internal_phys; unsigned int num_gpio; unsigned int max_vid; + unsigned int max_frame_size; unsigned int port_base_addr; unsigned int phy_base_addr; unsigned int global1_addr;