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: <eee6df3d-5e6b-4b4c-bfcc-55b31814fb82@lunn.ch> Date: Tue, 24 Oct 2023 04:33:44 +0200 From: Andrew Lunn <andrew@...n.ch> To: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com> Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, corbet@....net, steen.hegelund@...rochip.com, rdunlap@...radead.org, horms@...nel.org, casper.casan@...il.com, netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, horatiu.vultur@...rochip.com, Woojung.Huh@...rochip.com, Nicolas.Ferre@...rochip.com, UNGLinuxDriver@...rochip.com, Thorsten.Kummermehr@...rochip.com Subject: Re: [PATCH net-next v2 8/9] microchip: lan865x: add driver support for Microchip's LAN865X MACPHY > +static int lan865x_set_hw_macaddr(struct net_device *netdev) > +{ > + u32 regval; > + bool ret; > + struct lan865x_priv *priv = netdev_priv(netdev); > + const u8 *mac = netdev->dev_addr; > + > + ret = oa_tc6_read_register(priv->tc6, LAN865X_MAC_NCR, ®val); > + if (ret) > + goto error_mac; > + if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) { Would testing netif_running(netdev) be enough? That is a more common test you see. In fact, you already have that in lan865x_set_mac_address(). And in lan865x_probe() why would the hardware be enabled? > + if (netif_msg_drv(priv)) > + netdev_warn(netdev, "Hardware must be disabled for MAC setting\n"); > + return -EBUSY; > + } > + /* MAC address setting */ > + regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) | mac[0]; > + ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAB1, regval); > + if (ret) > + goto error_mac; > + > + regval = (mac[5] << 8) | mac[4]; > + ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval); > + if (ret) > + goto error_mac; > + > + return 0; > + > +error_mac: > + return -ENODEV; oa_tc6_write_register() should return an error code, so return it. > +static int lan865x_set_mac_address(struct net_device *netdev, void *addr) > +{ > + struct sockaddr *address = addr; > + > + if (netif_running(netdev)) > + return -EBUSY; > + > + eth_hw_addr_set(netdev, address->sa_data); > + > + return lan865x_set_hw_macaddr(netdev); You should ideally only update the netdev MAC address, if you managed to change the value in the hardware. > +static void lan865x_set_multicast_list(struct net_device *netdev) > +{ > + struct lan865x_priv *priv = netdev_priv(netdev); > + u32 regval = 0; > + > + if (netdev->flags & IFF_PROMISC) { > + /* Enabling promiscuous mode */ > + regval |= MAC_PROMISCUOUS_MODE; > + regval &= (~MAC_MULTICAST_MODE); > + regval &= (~MAC_UNICAST_MODE); > + } else if (netdev->flags & IFF_ALLMULTI) { > + /* Enabling all multicast mode */ > + regval &= (~MAC_PROMISCUOUS_MODE); > + regval |= MAC_MULTICAST_MODE; > + regval &= (~MAC_UNICAST_MODE); > + } else if (!netdev_mc_empty(netdev)) { > + /* Enabling specific multicast addresses */ > + struct netdev_hw_addr *ha; > + u32 hash_lo = 0; > + u32 hash_hi = 0; > + > + netdev_for_each_mc_addr(ha, netdev) { > + u32 bit_num = lan865x_hash(ha->addr); > + u32 mask = 1 << (bit_num & 0x1f); > + > + if (bit_num & 0x20) > + hash_hi |= mask; > + else > + hash_lo |= mask; > + } > + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRT, hash_hi)) { > + if (netif_msg_timer(priv)) > + netdev_err(netdev, "Failed to write reg_hashh"); > + return; > + } > + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) { > + if (netif_msg_timer(priv)) > + netdev_err(netdev, "Failed to write reg_hashl"); > + return; > + } > + regval &= (~MAC_PROMISCUOUS_MODE); > + regval &= (~MAC_MULTICAST_MODE); > + regval |= MAC_UNICAST_MODE; > + } else { > + /* enabling local mac address only */ > + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRT, regval)) { > + if (netif_msg_timer(priv)) > + netdev_err(netdev, "Failed to write reg_hashh"); > + return; > + } > + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) { > + if (netif_msg_timer(priv)) > + netdev_err(netdev, "Failed to write reg_hashl"); > + return; > + } > + } > + if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) { > + if (netif_msg_timer(priv)) > + netdev_err(netdev, "Failed to enable promiscuous mode"); > + } > +} Another of those big functions which could be lots of simple functions each doing one thing. > +/* Configures the number of bytes allocated to each buffer in the > + * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64 > + * is the default value. So it is enough to configure the queue buffer size only > + * for 32 bytes. Generally cps can't be changed during run time and also it is > + * configured in the device tree. The values for the Tx/Rx queue buffer size are > + * taken from the LAN865x datasheet. > + */ Its unclear why this needs to be a callback. Why not just set it after oa_tc6_init() returns? > +static void lan865x_remove(struct spi_device *spi) > +{ > + struct lan865x_priv *priv = spi_get_drvdata(spi); > + > + oa_tc6_exit(priv->tc6); > + unregister_netdev(priv->netdev); Is that the correct order? Seems like you should unregister the netdev first. > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id lan865x_acpi_ids[] = { > + { .id = "LAN865X", > + }, > + {}, Does this work? You don't have access to the device tree properties. Andrew
Powered by blists - more mailing lists