Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
On Mon, Sep 14, 2020 at 11:07 PM Jakub Kicinski wrote:
>
> On Mon, 14 Sep 2020 03:24:13 +0200 Andrew Lunn wrote:
> > > +static void gaudi_nic_get_internal_stats(struct net_device *netdev, u64
> > > *data)
> > > +{
> > > + struct gaudi_nic_device **ptr = netdev_priv(netdev);
> > > + struct gaudi_nic_device *gaudi_nic = *ptr;
> > > + struct hl_device *hdev = gaudi_nic->hdev;
> > > + u32 port = gaudi_nic->port;
> > > + u32 num_spmus;
> > > + int i;
> > > +
> > > + num_spmus = (port & 1) ? NIC_SPMU1_STATS_LEN : NIC_SPMU0_STATS_LEN;
> > > +
> > > + gaudi_sample_spmu_nic(hdev, port, num_spmus, data);
> > > + data += num_spmus;
> > > +
> > > + /* first entry is title */
> > > + data[0] = 0;
> >
> > You have been looking at statistics names recently. What do you think
> > of this data[0]?
>
> Highly counter-productive, users will commonly grep for statistics.
> Header which says "TX stats:" is a bad idea.
ok, thanks for the input, we will fix that.
Oded
Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
On Mon, 14 Sep 2020 03:24:13 +0200 Andrew Lunn wrote:
> > +static void gaudi_nic_get_internal_stats(struct net_device *netdev, u64
> > *data)
> > +{
> > + struct gaudi_nic_device **ptr = netdev_priv(netdev);
> > + struct gaudi_nic_device *gaudi_nic = *ptr;
> > + struct hl_device *hdev = gaudi_nic->hdev;
> > + u32 port = gaudi_nic->port;
> > + u32 num_spmus;
> > + int i;
> > +
> > + num_spmus = (port & 1) ? NIC_SPMU1_STATS_LEN : NIC_SPMU0_STATS_LEN;
> > +
> > + gaudi_sample_spmu_nic(hdev, port, num_spmus, data);
> > + data += num_spmus;
> > +
> > + /* first entry is title */
> > + data[0] = 0;
>
> You have been looking at statistics names recently. What do you think
> of this data[0]?
Highly counter-productive, users will commonly grep for statistics.
Header which says "TX stats:" is a bad idea.
Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
On Mon, Sep 14, 2020 at 4:37 AM Andrew Lunn wrote:
>
> > +static int gaudi_nic_get_module_eeprom(struct net_device *netdev,
> > + struct ethtool_eeprom *ee, u8 *data)
> > +{
> > + struct gaudi_nic_device **ptr = netdev_priv(netdev);
> > + struct gaudi_nic_device *gaudi_nic = *ptr;
> > + struct hl_device *hdev = gaudi_nic->hdev;
> > +
> > + if (!ee->len)
> > + return -EINVAL;
> > +
> > + memset(data, 0, ee->len);
> > + memcpy(data, hdev->asic_prop.cpucp_nic_info.qsfp_eeprom, ee->len);
> > +
>
> You memset and then memcpy the same number of bytes?
Thanks for catching this, we will fix it.
>
> You also need to validate ee->offset, and ee->len. Otherwise this is a
> vector for user space to read kernel memory after
> hdev->asic_prop.cpucp_nic_info.qsfp_eeprom. See drivers/net/phy/sfp.c:
> sfp_module_eeprom() as a good example of this validation.
>
> Andrew
Thanks for the pointer, we will take a look and fix it.
Oded
Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
On Mon, Sep 14, 2020 at 4:39 AM Florian Fainelli wrote: > > > > On 9/12/2020 7:41 AM, Oded Gabbay wrote: > > From: Omer Shpigelman > > > > The driver supports ethtool callbacks and provides statistics using the > > device's profiling infrastructure (coresight). > > Is there any relationship near or far with ARM's CoreSight: > > https://developer.arm.com/ip-products/system-ip/coresight-debug-and-trace > > if not, should you rename this? > -- > Florian We have a cortex A53 inside our ASIC and we use other ARM IPs. One of those IPs is the CoreSight infrastructure for trace and profiling. It also provides us with something they call SPMU (performance monitoring). Those units provide us counters per port with which we provide the statistics. Thanks, Oded
Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
On 9/12/2020 7:41 AM, Oded Gabbay wrote: From: Omer Shpigelman The driver supports ethtool callbacks and provides statistics using the device's profiling infrastructure (coresight). Is there any relationship near or far with ARM's CoreSight: https://developer.arm.com/ip-products/system-ip/coresight-debug-and-trace if not, should you rename this? -- Florian
Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
> +static int gaudi_nic_get_module_eeprom(struct net_device *netdev,
> + struct ethtool_eeprom *ee, u8 *data)
> +{
> + struct gaudi_nic_device **ptr = netdev_priv(netdev);
> + struct gaudi_nic_device *gaudi_nic = *ptr;
> + struct hl_device *hdev = gaudi_nic->hdev;
> +
> + if (!ee->len)
> + return -EINVAL;
> +
> + memset(data, 0, ee->len);
> + memcpy(data, hdev->asic_prop.cpucp_nic_info.qsfp_eeprom, ee->len);
> +
You memset and then memcpy the same number of bytes?
You also need to validate ee->offset, and ee->len. Otherwise this is a
vector for user space to read kernel memory after
hdev->asic_prop.cpucp_nic_info.qsfp_eeprom. See drivers/net/phy/sfp.c:
sfp_module_eeprom() as a good example of this validation.
Andrew
Re: [PATCH v2 12/14] habanalabs/gaudi: Add ethtool support using coresight
> +static struct gaudi_nic_ethtool_stats gaudi_nic_mac_stats_rx[] = {
> + {"Rx MAC counters", 0},
> + {" etherStatsOctets", 0x0},
> + {" OctetsReceivedOK", 0x4},
> + {" aAlignmentErrors", 0x8},
> + {" aPAUSEMACCtrlFramesReceived", 0xC},
> + {" aFrameTooLongErrors", 0x10},
> + {" aInRangeLengthErrors", 0x14},
> + {" aFramesReceivedOK", 0x18},
> + {" VLANReceivedOK", 0x1C},
> + {" aFrameCheckSequenceErrors", 0x20},
> + {" ifInErrors", 0x24},
> + {" ifInUcastPkts", 0x28},
> + {" ifInMulticastPkts", 0x2C},
> + {" ifInBroadcastPkts", 0x30},
> + {" etherStatsDropEvents", 0x34},
> + {" etherStatsUndersizePkts", 0x38},
> + {" etherStatsPkts", 0x3C},
> + {" etherStatsPkts64Octets", 0x40},
> + {" etherStatsPkts65to127Octets", 0x44},
> + {" etherStatsPkts128to255Octets", 0x48},
> + {" etherStatsPkts256to511Octets", 0x4C},
> + {" etherStatsPkts512to1023Octets", 0x50},
> + {" etherStatsPkts1024to1518Octets", 0x54},
> + {" etherStatsPkts1519toMaxOctets", 0x58},
> + {" etherStatsOversizePkts", 0x5C},
> + {" etherStatsJabbers", 0x60},
> + {" etherStatsFragments", 0x64},
> + {" aCBFCPAUSEFramesReceived_0", 0x68},
> + {" aCBFCPAUSEFramesReceived_1", 0x6C},
> + {" aCBFCPAUSEFramesReceived_2", 0x70},
> + {" aCBFCPAUSEFramesReceived_3", 0x74},
> + {" aCBFCPAUSEFramesReceived_4", 0x78},
> + {" aCBFCPAUSEFramesReceived_5", 0x7C},
> + {" aCBFCPAUSEFramesReceived_6", 0x80},
> + {" aCBFCPAUSEFramesReceived_7", 0x84},
> + {" aMACControlFramesReceived", 0x88}
> +};
> +
> +static struct gaudi_nic_ethtool_stats gaudi_nic_mac_stats_tx[] = {
> + {"Tx MAC counters", 0},
> + {" etherStatsOctets", 0x0},
> + {" OctetsTransmittedOK", 0x4},
> + {" aPAUSEMACCtrlFramesTransmitted", 0x8},
> + {" aFramesTransmittedOK", 0xC},
> + {" VLANTransmittedOK", 0x10},
> + {" ifOutErrors", 0x14},
> + {" ifOutUcastPkts", 0x18},
> + {" ifOutMulticastPkts", 0x1C},
> + {" ifOutBroadcastPkts", 0x20},
> + {" etherStatsPkts64Octets", 0x24},
> + {" etherStatsPkts65to127Octets", 0x28},
> + {" etherStatsPkts128to255Octets", 0x2C},
> + {" etherStatsPkts256to511Octets", 0x30},
> + {" etherStatsPkts512to1023Octets", 0x34},
> + {" etherStatsPkts1024to1518Octets", 0x38},
> + {" etherStatsPkts1519toMaxOctets", 0x3C},
> + {" aCBFCPAUSEFramesTransmitted_0", 0x40},
> + {" aCBFCPAUSEFramesTransmitted_1", 0x44},
> + {" aCBFCPAUSEFramesTransmitted_2", 0x48},
> + {" aCBFCPAUSEFramesTransmitted_3", 0x4C},
> + {" aCBFCPAUSEFramesTransmitted_4", 0x50},
> + {" aCBFCPAUSEFramesTransmitted_5", 0x54},
> + {" aCBFCPAUSEFramesTransmitted_6", 0x58},
> + {" aCBFCPAUSEFramesTransmitted_7", 0x5C},
> + {" aMACControlFramesTransmitted", 0x60},
> + {" etherStatsPkts", 0x64}
> +};
...
> +static void gaudi_nic_get_internal_stats(struct net_device *netdev, u64
> *data)
> +{
> + struct gaudi_nic_device **ptr = netdev_priv(netdev);
> + struct gaudi_nic_device *gaudi_nic = *ptr;
> + struct hl_device *hdev = gaudi_nic->hdev;
> + u32 port = gaudi_nic->port;
> + u32 num_spmus;
> + int i;
> +
> + num_spmus = (port & 1) ? NIC_SPMU1_STATS_LEN : NIC_SPMU0_STATS_LEN;
> +
> + gaudi_sample_spmu_nic(hdev, port, num_spmus, data);
> + data += num_spmus;
> +
> + /* first entry is title */
> + data[0] = 0;
Hi Jakub
You have been looking at statistics names recently. What do you think
of this data[0]?
Andrew
