| 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: <20210530135919.3f64cf33@kicinski-fedora-PC1C0HJN.hsd1.ca.comcast.net>
Date: Sun, 30 May 2021 13:59:19 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Steen Hegelund <steen.hegelund@...rochip.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Andrew Lunn <andrew@...n.ch>,
Russell King <linux@...linux.org.uk>,
"Microchip Linux Driver Support" <UNGLinuxDriver@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Madalin Bucur <madalin.bucur@....nxp.com>,
Mark Einon <mark.einon@...il.com>,
Masahiro Yamada <masahiroy@...nel.org>,
Arnd Bergmann <arnd@...db.de>,
Philipp Zabel <p.zabel@...gutronix.de>,
"Simon Horman" <simon.horman@...ronome.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
Bjarni Jonasson <bjarni.jonasson@...rochip.com>,
Lars Povlsen <lars.povlsen@...rochip.com>
Subject: Re: [PATCH net-next v2 02/10] net: sparx5: add the basic sparx5
driver
On Fri, 28 May 2021 14:34:11 +0200 Steen Hegelund wrote:
> This adds the Sparx5 basic SwitchDev driver framework with IO range
> mapping, switch device detection and core clock configuration.
>
> Support for ports, phylink, netdev, mactable etc. are in the following
> patches.
> + for (idx = 0; idx < 3; idx++) {
> + spx5_rmw(GCB_SIO_CLOCK_SYS_CLK_PERIOD_SET(clk_period / 100),
> + GCB_SIO_CLOCK_SYS_CLK_PERIOD,
> + sparx5,
> + GCB_SIO_CLOCK(idx));
> + }
braces unnecessary, please fix everywhere.
> +
> + spx5_rmw(HSCH_TAS_STATEMACHINE_CFG_REVISIT_DLY_SET
> + ((256 * 1000) / clk_period),
> + HSCH_TAS_STATEMACHINE_CFG_REVISIT_DLY,
> + sparx5,
> + HSCH_TAS_STATEMACHINE_CFG);
> +
> + spx5_rmw(ANA_AC_POL_POL_UPD_INT_CFG_POL_UPD_INT_SET(pol_upd_int),
> + ANA_AC_POL_POL_UPD_INT_CFG_POL_UPD_INT,
> + sparx5,
> + ANA_AC_POL_POL_UPD_INT_CFG);
> +
> + return 0;
> +}
> + /* Default values, some from DT */
> + sparx5->coreclock = SPX5_CORE_CLOCK_DEFAULT;
> +
> + ports = of_get_child_by_name(np, "ethernet-ports");
Don't you need to release the reference you got on @ports?
> + if (!ports) {
> + dev_err(sparx5->dev, "no ethernet-ports child node found\n");
> + return -ENODEV;
> + }
> + sparx5->port_count = of_get_child_count(ports);
> +
> + configs = kcalloc(sparx5->port_count,
> + sizeof(struct initial_port_config), GFP_KERNEL);
> + if (!configs)
> + return -ENOMEM;
> +
> + for_each_available_child_of_node(ports, portnp) {
> + struct sparx5_port_config *conf;
> + struct phy *serdes;
> + u32 portno;
> +
> + err = of_property_read_u32(portnp, "reg", &portno);
> + if (err) {
> + dev_err(sparx5->dev, "port reg property error\n");
> + continue;
> + }
> + config = &configs[idx];
> + conf = &config->conf;
> + err = of_get_phy_mode(portnp, &conf->phy_mode);
> + if (err) {
> + dev_err(sparx5->dev, "port %u: missing phy-mode\n",
> + portno);
> + continue;
> + }
> + err = of_property_read_u32(portnp, "microchip,bandwidth",
> + &conf->bandwidth);
> + if (err) {
> + dev_err(sparx5->dev, "port %u: missing bandwidth\n",
> + portno);
> + continue;
> + }
> + err = of_property_read_u32(portnp, "microchip,sd-sgpio", &conf->sd_sgpio);
> + if (err)
> + conf->sd_sgpio = ~0;
> + else
> + sparx5->sd_sgpio_remapping = true;
> + serdes = devm_of_phy_get(sparx5->dev, portnp, NULL);
> + if (IS_ERR(serdes)) {
> + err = PTR_ERR(serdes);
> + if (err != -EPROBE_DEFER)
> + dev_err(sparx5->dev,
> + "port %u: missing serdes\n",
> + portno);
dev_err_probe()
> + goto cleanup_config;
> + }
> + config->portno = portno;
> + config->node = portnp;
> + config->serdes = serdes;
> +
> + conf->media = PHY_MEDIA_DAC;
> + conf->serdes_reset = true;
> + conf->portmode = conf->phy_mode;
> + if (of_find_property(portnp, "sfp", NULL)) {
> + conf->has_sfp = true;
> + conf->power_down = true;
> + }
> + idx++;
> + }
> +
> + err = sparx5_create_targets(sparx5);
> + if (err)
> + goto cleanup_config;
> +
> + if (of_get_mac_address(np, mac_addr)) {
> + dev_info(sparx5->dev, "MAC addr was not set, use random MAC\n");
> + eth_random_addr(sparx5->base_mac);
> + sparx5->base_mac[5] = 0;
> + } else {
> + ether_addr_copy(sparx5->base_mac, mac_addr);
> + }
> +
> + /* Inj/Xtr IRQ support to be added in later patches */
> + /* Read chip ID to check CPU interface */
> + sparx5->chip_id = spx5_rd(sparx5, GCB_CHIP_ID);
> +
> + sparx5->target_ct = (enum spx5_target_chiptype)
> + GCB_CHIP_ID_PART_ID_GET(sparx5->chip_id);
> +
> + /* Initialize Switchcore and internal RAMs */
> + if (sparx5_init_switchcore(sparx5)) {
> + dev_err(sparx5->dev, "Switchcore initialization error\n");
> + goto cleanup_config;
Should @err be set?
> + }
> +
> + /* Initialize the LC-PLL (core clock) and set affected registers */
> + if (sparx5_init_coreclock(sparx5)) {
> + dev_err(sparx5->dev, "LC-PLL initialization error\n");
> + goto cleanup_config;
ditto
> + }
> +
> + for (idx = 0; idx < sparx5->port_count; ++idx) {
> + config = &configs[idx];
> + if (!config->node)
> + continue;
> +
> + err = sparx5_create_port(sparx5, config);
> + if (err) {
> + dev_err(sparx5->dev, "port create error\n");
> + goto cleanup_ports;
> + }
> + }
> +
> + if (sparx5_start(sparx5)) {
> + dev_err(sparx5->dev, "Start failed\n");
> + goto cleanup_ports;
and here
> + }
> +
> + kfree(configs);
> + return err;
> +
> +cleanup_ports:
> + /* Port cleanup to be added in later patches */
> +cleanup_config:
> + kfree(configs);
> + return err;
> +}
> +struct sparx5_port_config {
Spurious tab before {?
> + phy_interface_t portmode;
> + bool has_sfp;
> + u32 bandwidth;
> + int speed;
> + int duplex;
> + enum phy_media media;
> + bool power_down;
> + bool autoneg;
> + u32 pause;
> + bool serdes_reset;
Group all 4 bools together for better packing?
> + phy_interface_t phy_mode;
> + u32 sd_sgpio;
> +};
> +static inline void spx5_rmw(u32 val, u32 mask, struct sparx5 *sparx5,
> + int id, int tinst, int tcnt,
> + int gbase, int ginst, int gcnt, int gwidth,
> + int raddr, int rinst, int rcnt, int rwidth)
> +{
> + u32 nval;
> + void __iomem *addr =
> + spx5_addr(sparx5->regs, id, tinst, tcnt,
Why try to initialize inline when it results in weird looking code and
no saved lines?
> + gbase, ginst, gcnt, gwidth,
> + raddr, rinst, rcnt, rwidth);
Not to mention that you end up with no new line after variable
declaration.
> + nval = readl(addr);
> + nval = (nval & ~mask) | (val & mask);
> + writel(nval, addr);
> +}
Powered by blists - more mailing lists