| 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: <296aa172-404e-414a-a56a-ca82b3e90397@microchip.com>
Date: Tue, 31 Oct 2023 11:04:12 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <andrew@...n.ch>
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
Hi Andrew,
On 24/10/23 8:03 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> +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?
Ah yes, this check is not needed at all. Will correct it.
>
>
>> + 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.
Yes, noted. Will do 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.
Ah ok, then it is supposed to be like below, isn't it?
static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
{
struct sockaddr *address = addr;
int ret;
if (netif_running(netdev))
return -EBUSY;
ret = lan865x_set_hw_macaddr(netdev);
if (ret)
return ret;
eth_hw_addr_set(netdev, address->sa_data);
return 0;
}
>
>> +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.
Sure, will split into multiple functions.
>
>> +/* 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?
oa_tc6_init() function internally calls oa_tc6_configure() function to
configure different settings. At the end it writes SYNC bit to CONFIG0
register which enables the MAC-PHY to start the data transfer. So the
buffer configuration should be done prior to that. Since this is part of
the configuration this callback is implemented.
>
>> +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.
Ah yes, will correct it.
>
>> +#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.
Going to remove all the OA specific configurations in the next revisions.
Best Regards,
Parthiban V
>
> Andrew
>
Powered by blists - more mailing lists