| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <672de8c0.050a0220.a7227.c2c7@mx.google.com>
Date: Fri, 8 Nov 2024 11:32:28 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Matthias Brugger <matthias.bgg@...il.com>,
"AngeloGioacchino Del Regno," <angelogioacchino.delregno@...labora.com>,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
upstream@...oha.com
Subject: Re: [net-next PATCH v3 2/3] net: dsa: Add Airoha AN8855 5-Port
Gigabit DSA Switch driver
On Thu, Nov 07, 2024 at 06:53:53PM +0100, Christophe JAILLET wrote:
> Le 06/11/2024 à 13:22, Christian Marangi a écrit :
> > Add Airoha AN8855 5-Port Gigabit DSA switch.
> >
> > The switch is also a nvmem-provider as it does have EFUSE to calibrate
> > the internal PHYs.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth-Re5JQEeQqe8AvxtiuMwx3w@...lic.gmane.org>
> > ---
>
> Hi,
>
> ...
>
> > +#include <linux/bitfield.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/if_bridge.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/mdio.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/of_mdio.h>
> > +#include <linux/of_net.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/nvmem-provider.h>
>
> Could be moved a few lines above to keep order.
>
> > +#include <linux/phylink.h>
> > +#include <linux/regmap.h>
> > +#include <net/dsa.h>
> ...
>
> > +static int an8855_port_fdb_dump(struct dsa_switch *ds, int port,
> > + dsa_fdb_dump_cb_t *cb, void *data)
> > +{
> > + struct an8855_priv *priv = ds->priv;
> > + struct an8855_fdb _fdb = { };
>
> Should it but reseted in the do loop below, instead of only once here?
>
Common practice is to fill EVERY value in _fdb to not have to reset, but
yes just as extra caution, I will move the define in the for loop.
> > + int banks, count = 0;
> > + u32 rsp;
> > + int ret;
> > + int i;
> > +
> > + mutex_lock(&priv->reg_mutex);
> > +
> > + /* Load search port */
> > + ret = regmap_write(priv->regmap, AN8855_ATWD2,
> > + FIELD_PREP(AN8855_ATWD2_PORT, port));
> > + if (ret)
> > + goto exit;
> > + ret = an8855_fdb_cmd(priv, AN8855_ATC_MAT(AND8855_FDB_MAT_MAC_PORT) |
> > + AN8855_FDB_START, &rsp);
> > + if (ret < 0)
> > + goto exit;
> > +
> > + do {
> > + /* From response get the number of banks to read, exit if 0 */
> > + banks = FIELD_GET(AN8855_ATC_HIT, rsp);
> > + if (!banks)
> > + break;
> > +
> > + /* Each banks have 4 entry */
> > + for (i = 0; i < 4; i++) {
> > + count++;
> > +
> > + /* Check if bank is present */
> > + if (!(banks & BIT(i)))
> > + continue;
> > +
> > + /* Select bank entry index */
> > + ret = regmap_write(priv->regmap, AN8855_ATRDS,
> > + FIELD_PREP(AN8855_ATRD_SEL, i));
> > + if (ret)
> > + break;
> > + /* wait 1ms for the bank entry to be filled */
> > + usleep_range(1000, 1500);
> > + an8855_fdb_read(priv, &_fdb);
> > +
> > + if (!_fdb.live)
> > + continue;
> > + ret = cb(_fdb.mac, _fdb.vid, _fdb.noarp, data);
> > + if (ret < 0)
> > + break;
> > + }
> > +
> > + /* Stop if reached max FDB number */
> > + if (count >= AN8855_NUM_FDB_RECORDS)
> > + break;
> > +
> > + /* Read next bank */
> > + ret = an8855_fdb_cmd(priv, AN8855_ATC_MAT(AND8855_FDB_MAT_MAC_PORT) |
> > + AN8855_FDB_NEXT, &rsp);
> > + if (ret < 0)
> > + break;
> > + } while (true);
> > +
> > +exit:
> > + mutex_unlock(&priv->reg_mutex);
> > + return ret;
> > +}
>
> ...
>
> > + ret = regmap_set_bits(priv->regmap, AN8855_RG_RATE_ADAPT_CTRL_0,
> > + AN8855_RG_RATE_ADAPT_RX_BYPASS |
> > + AN8855_RG_RATE_ADAPT_TX_BYPASS |
> > + AN8855_RG_RATE_ADAPT_RX_EN |
> > + AN8855_RG_RATE_ADAPT_TX_EN);
> > + if (ret)
> > + return ret;
> > +
> > + /* Disable AN if not in autoneg */
> > + ret = regmap_update_bits(priv->regmap, AN8855_SGMII_REG_AN0, BMCR_ANENABLE,
> > + neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED ? BMCR_ANENABLE :
> > + 0);
>
> Should 'ret' be tested here?
>
Sorry forgot to add.
> > +
> > + if (interface == PHY_INTERFACE_MODE_SGMII &&
> > + neg_mode == PHYLINK_PCS_NEG_INBAND_DISABLED) {
> > + ret = regmap_set_bits(priv->regmap, AN8855_PHY_RX_FORCE_CTRL_0,
> > + AN8855_RG_FORCE_TXC_SEL);
> > + if (ret)
> > + return ret;
> > + }
>
> ...
>
> > + priv->ds = devm_kzalloc(&mdiodev->dev, sizeof(*priv->ds), GFP_KERNEL);
> > + if (!priv->ds)
> > + return -ENOMEM;
> > +
> > + priv->ds->dev = &mdiodev->dev;
> > + priv->ds->num_ports = AN8855_NUM_PORTS;
> > + priv->ds->priv = priv;
> > + priv->ds->ops = &an8855_switch_ops;
> > + mutex_init(&priv->reg_mutex);
>
> devm_mutex_init() to slightly simplify the remove function?
>
Wonder if there is a variant also for dsa_unregister_switch. That
would effectively remove the need for a remove function.
> > + priv->ds->phylink_mac_ops = &an8855_phylink_mac_ops;
> > +
> > + priv->pcs.ops = &an8855_pcs_ops;
> > + priv->pcs.neg_mode = true;
> > + priv->pcs.poll = true;
> > +
> > + ret = an8855_sw_register_nvmem(priv);
> > + if (ret)
> > + return ret;
> > +
> > + dev_set_drvdata(&mdiodev->dev, priv);
> > +
> > + return dsa_register_switch(priv->ds);
> > +}
> > +
> > +static void
> > +an8855_sw_remove(struct mdio_device *mdiodev)
> > +{
> > + struct an8855_priv *priv = dev_get_drvdata(&mdiodev->dev);
> > +
> > + dsa_unregister_switch(priv->ds);
> > + mutex_destroy(&priv->reg_mutex);
> > +}
> > +
> > +static const struct of_device_id an8855_of_match[] = {
> > + { .compatible = "airoha,an8855" },
> > + { /* sentinel */ },
>
> Ending comma is usually not needed after a terminator.
>
> > +};
> > +
> > +static struct mdio_driver an8855_mdio_driver = {
> > + .probe = an8855_sw_probe,
> > + .remove = an8855_sw_remove,
> > + .mdiodrv.driver = {
> > + .name = "an8855",
> > + .of_match_table = an8855_of_match,
> > + },
> > +};
>
> ...
>
> > +#define AN8855_VA0_VTAG_EN BIT(10) /* Per VLAN Egress Tag Control */
> > +#define AN8855_VA0_IVL_MAC BIT(5) /* Independent VLAN Learning */
> > +#define AN8855_VA0_VLAN_VALID BIT(0) /* VLAN Entry Valid */
> > +#define AN8855_VAWD1 0x10200608
> > +#define AN8855_VA1_PORT_STAG BIT(1)
> > +
> > +/* Same register map of VAWD0 */
>
> Not sure to follow. AN8855_VAWD0 above is 0x10200604, not 0x10200618
>
Confusing comment. The meaning here is not "same register" but same
register fields aka bits and mask for the register are the same of
..604.
> > +#define AN8855_VARD0 0x10200618
> > +
> > +enum an8855_vlan_egress_attr {
> > + AN8855_VLAN_EGRESS_UNTAG = 0,
> > + AN8855_VLAN_EGRESS_TAG = 2,
> > + AN8855_VLAN_EGRESS_STACK = 3,
> > +};
> > +
> > +/* Register for port STP state control */
> > +#define AN8855_SSP_P(x) (0x10208000 + ((x) * 0x200))
> > +#define AN8855_FID_PST GENMASK(1, 0)
> > +
> > +enum an8855_stp_state {
> > + AN8855_STP_DISABLED = 0,
> > + AN8855_STP_BLOCKING = 1,
> > + AN8855_STP_LISTENING = 1,
>
> Just wondering if this 0, 1, *1*, 2, 3 was intentional?
>
Yes blocking and listening is the same, I will follow suggestion by
Andrew and make blocking = listening.
> > + AN8855_STP_LEARNING = 2,
> > + AN8855_STP_FORWARDING = 3
> > +};
> > +
> > +/* Register for port control */
> > +#define AN8855_PCR_P(x) (0x10208004 + ((x) * 0x200))
> > +#define AN8855_EG_TAG GENMASK(29, 28)
> > +#define AN8855_PORT_PRI GENMASK(26, 24)
> > +#define AN8855_PORT_TX_MIR BIT(20)
> > +#define AN8855_PORT_RX_MIR BIT(16)
> > +#define AN8855_PORT_VLAN GENMASK(1, 0)
> > +
> > +enum an8855_port_mode {
> > + /* Port Matrix Mode: Frames are forwarded by the PCR_MATRIX members. */
> > + AN8855_PORT_MATRIX_MODE = 0,
> > +
> > + /* Fallback Mode: Forward received frames with ingress ports that do
> > + * not belong to the VLAN member. Frames whose VID is not listed on
> > + * the VLAN table are forwarded by the PCR_MATRIX members.
> > + */
> > + AN8855_PORT_FALLBACK_MODE = 1,
> > +
> > + /* Check Mode: Forward received frames whose ingress do not
> > + * belong to the VLAN member. Discard frames if VID ismiddes on the
> > + * VLAN table.
> > + */
> > + AN8855_PORT_CHECK_MODE = 1,
>
> Just wondering if this 0, 1, *1*, 3 was intentional?
>
Nope a typo. Thanks for noticing this.
> > +
> > + /* Security Mode: Discard any frame due to ingress membership
> > + * violation or VID missed on the VLAN table.
> > + */
> > + AN8855_PORT_SECURITY_MODE = 3,
> > +};
> ...
>
> CJ
--
Ansuel
Powered by blists - more mailing lists