| 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