| Message ID | 20241220025241.1522781-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | eth: fbnic: support basic RSS config and setting channel count | expand |
On Thu, Dec 19, 2024 at 06:52:32PM -0800, Jakub Kicinski wrote: > Define ethtool callback handlers in order in which they are defined > in the ops struct. It doesn't really matter what the order is, > but it's good to have an order. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > .../net/ethernet/meta/fbnic/fbnic_ethtool.c | 160 +++++++++--------- > 1 file changed, 80 insertions(+), 80 deletions(-) > > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c > index cc8ca94529ca..777e083acae9 100644 > --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c > @@ -40,6 +40,68 @@ static const struct fbnic_stat fbnic_gstrings_hw_stats[] = { > #define FBNIC_HW_FIXED_STATS_LEN ARRAY_SIZE(fbnic_gstrings_hw_stats) > #define FBNIC_HW_STATS_LEN FBNIC_HW_FIXED_STATS_LEN > > +static void I thought type and name on separate lines are not desirable, it could be moved to a single line in this commit. Assuming such adjustment, Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> Also, this would be a little bit out of scope for this commit, but seeing relatively new code that does not use `for (int i = 0,...)` is surprising. > +fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) > +{ > + struct fbnic_net *fbn = netdev_priv(netdev); > + struct fbnic_dev *fbd = fbn->fbd; > + > + fbnic_get_fw_ver_commit_str(fbd, drvinfo->fw_version, > + sizeof(drvinfo->fw_version)); > +} > + > +static int fbnic_get_regs_len(struct net_device *netdev) > +{ > + struct fbnic_net *fbn = netdev_priv(netdev); > + > + return fbnic_csr_regs_len(fbn->fbd) * sizeof(u32); > +} > + > +static void fbnic_get_regs(struct net_device *netdev, > + struct ethtool_regs *regs, void *data) > +{ > + struct fbnic_net *fbn = netdev_priv(netdev); > + > + fbnic_csr_get_regs(fbn->fbd, data, ®s->version); > +} > + > +static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data) > +{ > + int i; > + > + switch (sset) { > + case ETH_SS_STATS: > + for (i = 0; i < FBNIC_HW_STATS_LEN; i++) > + ethtool_puts(&data, fbnic_gstrings_hw_stats[i].string); > + break; > + } > +} > + > +static void fbnic_get_ethtool_stats(struct net_device *dev, > + struct ethtool_stats *stats, u64 *data) > +{ > + struct fbnic_net *fbn = netdev_priv(dev); > + const struct fbnic_stat *stat; > + int i; > + > + fbnic_get_hw_stats(fbn->fbd); > + > + for (i = 0; i < FBNIC_HW_STATS_LEN; i++) { > + stat = &fbnic_gstrings_hw_stats[i]; > + data[i] = *(u64 *)((u8 *)&fbn->fbd->hw_stats + stat->offset); > + } > +} > + > +static int fbnic_get_sset_count(struct net_device *dev, int sset) > +{ > + switch (sset) { > + case ETH_SS_STATS: > + return FBNIC_HW_STATS_LEN; > + default: > + return -EOPNOTSUPP; > + } > +} > + > static int > fbnic_get_ts_info(struct net_device *netdev, > struct kernel_ethtool_ts_info *tsinfo) > @@ -69,14 +131,27 @@ fbnic_get_ts_info(struct net_device *netdev, > return 0; > } > > -static void > -fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) > +static void fbnic_get_ts_stats(struct net_device *netdev, > + struct ethtool_ts_stats *ts_stats) > { > struct fbnic_net *fbn = netdev_priv(netdev); > - struct fbnic_dev *fbd = fbn->fbd; > + u64 ts_packets, ts_lost; > + struct fbnic_ring *ring; > + unsigned int start; > + int i; > > - fbnic_get_fw_ver_commit_str(fbd, drvinfo->fw_version, > - sizeof(drvinfo->fw_version)); > + ts_stats->pkts = fbn->tx_stats.ts_packets; > + ts_stats->lost = fbn->tx_stats.ts_lost; > + for (i = 0; i < fbn->num_tx_queues; i++) { > + ring = fbn->tx[i]; > + do { > + start = u64_stats_fetch_begin(&ring->stats.syncp); > + ts_packets = ring->stats.ts_packets; > + ts_lost = ring->stats.ts_lost; > + } while (u64_stats_fetch_retry(&ring->stats.syncp, start)); > + ts_stats->pkts += ts_packets; > + ts_stats->lost += ts_lost; > + } > } > > static void fbnic_set_counter(u64 *stat, struct fbnic_stat_counter *counter) > @@ -85,43 +160,6 @@ static void fbnic_set_counter(u64 *stat, struct fbnic_stat_counter *counter) > *stat = counter->value; > } > > -static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data) > -{ > - int i; > - > - switch (sset) { > - case ETH_SS_STATS: > - for (i = 0; i < FBNIC_HW_STATS_LEN; i++) > - ethtool_puts(&data, fbnic_gstrings_hw_stats[i].string); > - break; > - } > -} > - > -static int fbnic_get_sset_count(struct net_device *dev, int sset) > -{ > - switch (sset) { > - case ETH_SS_STATS: > - return FBNIC_HW_STATS_LEN; > - default: > - return -EOPNOTSUPP; > - } > -} > - > -static void fbnic_get_ethtool_stats(struct net_device *dev, > - struct ethtool_stats *stats, u64 *data) > -{ > - struct fbnic_net *fbn = netdev_priv(dev); > - const struct fbnic_stat *stat; > - int i; > - > - fbnic_get_hw_stats(fbn->fbd); > - > - for (i = 0; i < FBNIC_HW_STATS_LEN; i++) { > - stat = &fbnic_gstrings_hw_stats[i]; > - data[i] = *(u64 *)((u8 *)&fbn->fbd->hw_stats + stat->offset); > - } > -} > - > static void > fbnic_get_eth_mac_stats(struct net_device *netdev, > struct ethtool_eth_mac_stats *eth_mac_stats) > @@ -164,44 +202,6 @@ fbnic_get_eth_mac_stats(struct net_device *netdev, > &mac_stats->eth_mac.FrameTooLongErrors); > } > > -static void fbnic_get_ts_stats(struct net_device *netdev, > - struct ethtool_ts_stats *ts_stats) > -{ > - struct fbnic_net *fbn = netdev_priv(netdev); > - u64 ts_packets, ts_lost; > - struct fbnic_ring *ring; > - unsigned int start; > - int i; > - > - ts_stats->pkts = fbn->tx_stats.ts_packets; > - ts_stats->lost = fbn->tx_stats.ts_lost; > - for (i = 0; i < fbn->num_tx_queues; i++) { > - ring = fbn->tx[i]; > - do { > - start = u64_stats_fetch_begin(&ring->stats.syncp); > - ts_packets = ring->stats.ts_packets; > - ts_lost = ring->stats.ts_lost; > - } while (u64_stats_fetch_retry(&ring->stats.syncp, start)); > - ts_stats->pkts += ts_packets; > - ts_stats->lost += ts_lost; > - } > -} > - > -static void fbnic_get_regs(struct net_device *netdev, > - struct ethtool_regs *regs, void *data) > -{ > - struct fbnic_net *fbn = netdev_priv(netdev); > - > - fbnic_csr_get_regs(fbn->fbd, data, ®s->version); > -} > - > -static int fbnic_get_regs_len(struct net_device *netdev) > -{ > - struct fbnic_net *fbn = netdev_priv(netdev); > - > - return fbnic_csr_regs_len(fbn->fbd) * sizeof(u32); > -} > - > static const struct ethtool_ops fbnic_ethtool_ops = { > .get_drvinfo = fbnic_get_drvinfo, > .get_regs_len = fbnic_get_regs_len, > -- > 2.47.1 > >
On Fri, 20 Dec 2024 15:53:18 +0100 Larysa Zaremba wrote: > I thought type and name on separate lines are not desirable, it could be moved > to a single line in this commit. Assuming such adjustment, I find this style more readable, TBH. I tend to break the line after return type if the function declaration doesn't fit on a line and the type is longer than 3 chars (IOW int/u32/u8 are exceptions). Functions which end up looking like: static struct some_type_struct *some_function_name_here(struct another *ptr, int argument); are really ugly. And break if > 3 chars is a simple rule to follow. You will find that most of fbnic does not follow this style, because I didn't write most of it. But most of the nfp driver does. I think I learned the breaking after return type from Felix Fietkau. Not that he specified the 3 character thing as pedantically as I do. > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> > > Also, this would be a little bit out of scope for this commit, but seeing > relatively new code that does not use `for (int i = 0,...)` is surprising. I think you at Intel try to adopt the novelties much more than the rest of us. Let us old timers be, please.
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c index cc8ca94529ca..777e083acae9 100644 --- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c +++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c @@ -40,6 +40,68 @@ static const struct fbnic_stat fbnic_gstrings_hw_stats[] = { #define FBNIC_HW_FIXED_STATS_LEN ARRAY_SIZE(fbnic_gstrings_hw_stats) #define FBNIC_HW_STATS_LEN FBNIC_HW_FIXED_STATS_LEN +static void +fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) +{ + struct fbnic_net *fbn = netdev_priv(netdev); + struct fbnic_dev *fbd = fbn->fbd; + + fbnic_get_fw_ver_commit_str(fbd, drvinfo->fw_version, + sizeof(drvinfo->fw_version)); +} + +static int fbnic_get_regs_len(struct net_device *netdev) +{ + struct fbnic_net *fbn = netdev_priv(netdev); + + return fbnic_csr_regs_len(fbn->fbd) * sizeof(u32); +} + +static void fbnic_get_regs(struct net_device *netdev, + struct ethtool_regs *regs, void *data) +{ + struct fbnic_net *fbn = netdev_priv(netdev); + + fbnic_csr_get_regs(fbn->fbd, data, ®s->version); +} + +static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data) +{ + int i; + + switch (sset) { + case ETH_SS_STATS: + for (i = 0; i < FBNIC_HW_STATS_LEN; i++) + ethtool_puts(&data, fbnic_gstrings_hw_stats[i].string); + break; + } +} + +static void fbnic_get_ethtool_stats(struct net_device *dev, + struct ethtool_stats *stats, u64 *data) +{ + struct fbnic_net *fbn = netdev_priv(dev); + const struct fbnic_stat *stat; + int i; + + fbnic_get_hw_stats(fbn->fbd); + + for (i = 0; i < FBNIC_HW_STATS_LEN; i++) { + stat = &fbnic_gstrings_hw_stats[i]; + data[i] = *(u64 *)((u8 *)&fbn->fbd->hw_stats + stat->offset); + } +} + +static int fbnic_get_sset_count(struct net_device *dev, int sset) +{ + switch (sset) { + case ETH_SS_STATS: + return FBNIC_HW_STATS_LEN; + default: + return -EOPNOTSUPP; + } +} + static int fbnic_get_ts_info(struct net_device *netdev, struct kernel_ethtool_ts_info *tsinfo) @@ -69,14 +131,27 @@ fbnic_get_ts_info(struct net_device *netdev, return 0; } -static void -fbnic_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo) +static void fbnic_get_ts_stats(struct net_device *netdev, + struct ethtool_ts_stats *ts_stats) { struct fbnic_net *fbn = netdev_priv(netdev); - struct fbnic_dev *fbd = fbn->fbd; + u64 ts_packets, ts_lost; + struct fbnic_ring *ring; + unsigned int start; + int i; - fbnic_get_fw_ver_commit_str(fbd, drvinfo->fw_version, - sizeof(drvinfo->fw_version)); + ts_stats->pkts = fbn->tx_stats.ts_packets; + ts_stats->lost = fbn->tx_stats.ts_lost; + for (i = 0; i < fbn->num_tx_queues; i++) { + ring = fbn->tx[i]; + do { + start = u64_stats_fetch_begin(&ring->stats.syncp); + ts_packets = ring->stats.ts_packets; + ts_lost = ring->stats.ts_lost; + } while (u64_stats_fetch_retry(&ring->stats.syncp, start)); + ts_stats->pkts += ts_packets; + ts_stats->lost += ts_lost; + } } static void fbnic_set_counter(u64 *stat, struct fbnic_stat_counter *counter) @@ -85,43 +160,6 @@ static void fbnic_set_counter(u64 *stat, struct fbnic_stat_counter *counter) *stat = counter->value; } -static void fbnic_get_strings(struct net_device *dev, u32 sset, u8 *data) -{ - int i; - - switch (sset) { - case ETH_SS_STATS: - for (i = 0; i < FBNIC_HW_STATS_LEN; i++) - ethtool_puts(&data, fbnic_gstrings_hw_stats[i].string); - break; - } -} - -static int fbnic_get_sset_count(struct net_device *dev, int sset) -{ - switch (sset) { - case ETH_SS_STATS: - return FBNIC_HW_STATS_LEN; - default: - return -EOPNOTSUPP; - } -} - -static void fbnic_get_ethtool_stats(struct net_device *dev, - struct ethtool_stats *stats, u64 *data) -{ - struct fbnic_net *fbn = netdev_priv(dev); - const struct fbnic_stat *stat; - int i; - - fbnic_get_hw_stats(fbn->fbd); - - for (i = 0; i < FBNIC_HW_STATS_LEN; i++) { - stat = &fbnic_gstrings_hw_stats[i]; - data[i] = *(u64 *)((u8 *)&fbn->fbd->hw_stats + stat->offset); - } -} - static void fbnic_get_eth_mac_stats(struct net_device *netdev, struct ethtool_eth_mac_stats *eth_mac_stats) @@ -164,44 +202,6 @@ fbnic_get_eth_mac_stats(struct net_device *netdev, &mac_stats->eth_mac.FrameTooLongErrors); } -static void fbnic_get_ts_stats(struct net_device *netdev, - struct ethtool_ts_stats *ts_stats) -{ - struct fbnic_net *fbn = netdev_priv(netdev); - u64 ts_packets, ts_lost; - struct fbnic_ring *ring; - unsigned int start; - int i; - - ts_stats->pkts = fbn->tx_stats.ts_packets; - ts_stats->lost = fbn->tx_stats.ts_lost; - for (i = 0; i < fbn->num_tx_queues; i++) { - ring = fbn->tx[i]; - do { - start = u64_stats_fetch_begin(&ring->stats.syncp); - ts_packets = ring->stats.ts_packets; - ts_lost = ring->stats.ts_lost; - } while (u64_stats_fetch_retry(&ring->stats.syncp, start)); - ts_stats->pkts += ts_packets; - ts_stats->lost += ts_lost; - } -} - -static void fbnic_get_regs(struct net_device *netdev, - struct ethtool_regs *regs, void *data) -{ - struct fbnic_net *fbn = netdev_priv(netdev); - - fbnic_csr_get_regs(fbn->fbd, data, ®s->version); -} - -static int fbnic_get_regs_len(struct net_device *netdev) -{ - struct fbnic_net *fbn = netdev_priv(netdev); - - return fbnic_csr_regs_len(fbn->fbd) * sizeof(u32); -} - static const struct ethtool_ops fbnic_ethtool_ops = { .get_drvinfo = fbnic_get_drvinfo, .get_regs_len = fbnic_get_regs_len,
Define ethtool callback handlers in order in which they are defined in the ops struct. It doesn't really matter what the order is, but it's good to have an order. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- .../net/ethernet/meta/fbnic/fbnic_ethtool.c | 160 +++++++++--------- 1 file changed, 80 insertions(+), 80 deletions(-)