netdev - Re: [PATCH net-next v5 3/3] tsnep: Add TSN endpoint Ethernet MAC driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 16 Nov 2021 23:16:23 +0100
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v5 3/3] tsnep: Add TSN endpoint Ethernet MAC driver

> > +static int tsnep_ethtool_get_regs_len(struct net_device *netdev)
> > +{
> > +     struct tsnep_adapter *adapter = netdev_priv(netdev);
> > +     int len;
> > +     int num_queues;
> > +
> > +     len = TSNEP_MAC_SIZE;
> > +     num_queues = max(adapter->num_tx_queues, adapter->num_rx_queues);
> > +     len += TSNEP_QUEUE_SIZE * (num_queues - 1);
>
> Why the num_queues - 1 ? A comment here might be good explaining it.

First queue is within TSNEP_MAC_SIZE. I will add a comment.

>
> > +
> > +     return len;
> > +}
> > +
> > +static void tsnep_ethtool_get_regs(struct net_device *netdev,
> > +                                struct ethtool_regs *regs,
> > +                                void *p)
> > +{
> > +     struct tsnep_adapter *adapter = netdev_priv(netdev);
> > +
> > +     regs->version = 1;
> > +
> > +     memcpy_fromio(p, adapter->addr, regs->len);
> > +}
>
> So the registers and the queues are contiguous?

Yes, unused addresses in between are defined to be readable (zero).

>
> > +static int tsnep_ethtool_get_ts_info(struct net_device *dev,
> > +                                  struct ethtool_ts_info *info)
> > +{
> > +     struct tsnep_adapter *adapter = netdev_priv(dev);
> > +
> > +     info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> > +                             SOF_TIMESTAMPING_RX_SOFTWARE |
> > +                             SOF_TIMESTAMPING_SOFTWARE |
> > +                             SOF_TIMESTAMPING_TX_HARDWARE |
> > +                             SOF_TIMESTAMPING_RX_HARDWARE |
> > +                             SOF_TIMESTAMPING_RAW_HARDWARE;
> > +
> > +     if (adapter->ptp_clock)
> > +             info->phc_index = ptp_clock_index(adapter->ptp_clock);
> > +     else
> > +             info->phc_index = -1;
> > +
> > +     info->tx_types = BIT(HWTSTAMP_TX_OFF) |er is 484kb with test c
> > +                      BIT(HWTSTAMP_TX_ON);
> > +     info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > +                        BIT(HWTSTAMP_FILTER_ALL);
> > +
> > +     return 0;
> > +}
>
> You should Cc: Richard Cochran <richardcochran@...il.com> for the PTP
> parts.

Thanks for the hint. I will add him.

>
> > +static int tsnep_mdio_init(struct tsnep_adapter *adapter)
> > +{
> > +     struct device_node *np = adapter->pdev->dev.of_node;
> > +     int retval;
> > +
> > +     if (np) {
> > +             np = of_get_child_by_name(np, "mdio");
> > +             if (!np)
> > +                     return 0;
> > +
> > +             adapter->suppress_preamble =
> > +                     of_property_read_bool(np, "suppress-preamble");
> > +     }
> > +
> > +     adapter->mdiobus = devm_mdiobus_alloc(&adapter->pdev->dev);
> > +     if (!adapter->mdiobus) {
> > +             retval = -ENOMEM;
> > +
> > +             goto out;
> > +     }
> > +
> > +     adapter->mdiobus->priv = (void *)adapter;
> > +     adapter->mdiobus->parent = &adapter->pdev->dev;
> > +     adapter->mdiobus->read = tsnep_mdiobus_read;
> > +     adapter->mdiobus->write = tsnep_mdiobus_write;
> > +     adapter->mdiobus->name = TSNEP "-mdiobus";
> > +     snprintf(adapter->mdiobus->id, MII_BUS_ID_SIZE, "%s",
> > +              adapter->pdev->name);
> > +
> > +     if (np) {
> > +             retval = of_mdiobus_register(adapter->mdiobus, np);
> > +
> > +             of_node_put(np);
> > +     } else {
> > +             /* do not scan broadcast address */
> > +             adapter->mdiobus->phy_mask = 0x0000001;
> > +
> > +             retval = mdiobus_register(adapter->mdiobus);
>
> You can probably simply this. of_mdiobus_register() is happy to take a
> NULL pointer for np, and will fall back to mdiobus_register().

I will think about it. Driver is also in use with v4.9 and this fallback
does not exist there.

> > diff --git a/drivers/net/ethernet/engleder/tsnep_test.c b/drivers/net/ethernet/engleder/tsnep_test.c
>
> You have quite a lot of code in this file. Could it either be
>
> 1) A loadable module which extends the base driver?
> 2) A build time configuration option?
>
> What percentage of the overall driver binary does this test code take
> up?

Driver is 484kB with test code and 396kB without. So test code is roughly
20% currently. In my opinion a configuration option makes more sense,
because a loadable module would require exported symbols. Shall I add a
build time configuration option?

> Apart from the minor comments above, ethtool, mdio, phy all looks
> good.

Thanks!

Gerhard