| Message ID | 20240809044738.4347-1-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: octeon_ep_vf: use ethtool_sprintf/puts | expand |
On Thu, Aug 08, 2024 at 09:47:27PM -0700, Rosen Penev wrote: > Simplifies the function and avoids manual pointer manipulation. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> Thanks, The code changes look good to me and my local testing shows that it compiles. So, from that point of view: Reviewed-by: Simon Horman <horms@kernel.org> But I do have a few points about process, which I hope you will give due consideration: 1. It would be good if the patch description was a bit more verbose. And indicated how you found this problem (by inspection?) and how it was tested (compile tested only?). This helps set expectations for reviewers of this patch, both now and in the future. 2. You have posed a number of similar patches. To aid review it would be best to group these, say in batches of no more than 10. F.e. Point 1, above, doesn't just apply to this patch. 3. Please do review the Networking subsystem process document https://docs.kernel.org/process/maintainer-netdev.html ...
On Mon, Aug 12, 2024 at 5:43 AM Simon Horman <horms@kernel.org> wrote: > > On Thu, Aug 08, 2024 at 09:47:27PM -0700, Rosen Penev wrote: > > Simplifies the function and avoids manual pointer manipulation. > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > Thanks, > > The code changes look good to me and my local testing shows that it compiles. > So, from that point of view: > > Reviewed-by: Simon Horman <horms@kernel.org> > > But I do have a few points about process, which I hope you will > give due consideration: > > 1. It would be good if the patch description was a bit more verbose. > And indicated how you found this problem (by inspection?) > and how it was tested (compile tested only?). Right. This is just an API change. There should be no actual change in functionality. > > This helps set expectations for reviewers of this patch, > both now and in the future. > > 2. You have posed a number of similar patches. > To aid review it would be best to group these, say in batches of > no more than 10. I plan to do a treewide commit with a coccinelle script but would like to manually fix the problematic ones before doing so. Having said that I still need to figure out how to do a cover letter... > > F.e. Point 1, above, doesn't just apply to this patch. > > 3. Please do review the Networking subsystem process document > > https://docs.kernel.org/process/maintainer-netdev.html > > ...
> > 2. You have posed a number of similar patches. > > To aid review it would be best to group these, say in batches of > > no more than 10. > I plan to do a treewide commit with a coccinelle script but would like > to manually fix the problematic ones before doing so. Having said that > I still need to figure out how to do a cover letter... git format-patch --cover-letter HEAD~42 I also find `b4 prep` good for managing patchsets, and it will produce a cover letter by default if there is more than one patch in the set: https://b4.docs.kernel.org/en/latest/contributor/prep.html Andrew
On Mon, Aug 12, 2024 at 10:04:51AM -0700, Rosen Penev wrote: > On Mon, Aug 12, 2024 at 5:43 AM Simon Horman <horms@kernel.org> wrote: > > > > On Thu, Aug 08, 2024 at 09:47:27PM -0700, Rosen Penev wrote: > > > Simplifies the function and avoids manual pointer manipulation. > > > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > > > Thanks, > > > > The code changes look good to me and my local testing shows that it compiles. > > So, from that point of view: > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > > But I do have a few points about process, which I hope you will > > give due consideration: > > > > 1. It would be good if the patch description was a bit more verbose. > > And indicated how you found this problem (by inspection?) > > and how it was tested (compile tested only?). > Right. This is just an API change. There should be no actual change in > functionality. Right, but adding this kind of information to the commit message is useful. > > > > This helps set expectations for reviewers of this patch, > > both now and in the future. > > > > 2. You have posed a number of similar patches. > > To aid review it would be best to group these, say in batches of > > no more than 10. > I plan to do a treewide commit with a coccinelle script but would like > to manually fix the problematic ones before doing so. Having said that > I still need to figure out how to do a cover letter... I suggest using at b4. Else git format-patch (to prepare) + git send-email (to send). FWIIW, I was not suggesting a large tree-wide patchset. But rather, groups of related patches, in batches of 10 or less. > > > > F.e. Point 1, above, doesn't just apply to this patch. > > > > 3. Please do review the Networking subsystem process document > > > > https://docs.kernel.org/process/maintainer-netdev.html > > > > ... >
diff --git a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c index a1979b45e355..c7622159d195 100644 --- a/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c +++ b/drivers/net/ethernet/marvell/octeon_ep_vf/octep_vf_ethtool.c @@ -58,32 +58,24 @@ static void octep_vf_get_strings(struct net_device *netdev, { struct octep_vf_device *oct = netdev_priv(netdev); u16 num_queues = CFG_GET_PORTS_ACTIVE_IO_RINGS(oct->conf); - char *strings = (char *)data; int i, j; switch (stringset) { case ETH_SS_STATS: - for (i = 0; i < OCTEP_VF_GLOBAL_STATS_CNT; i++) { - snprintf(strings, ETH_GSTRING_LEN, - octep_vf_gstrings_global_stats[i]); - strings += ETH_GSTRING_LEN; - } - - for (i = 0; i < num_queues; i++) { - for (j = 0; j < OCTEP_VF_TX_Q_STATS_CNT; j++) { - snprintf(strings, ETH_GSTRING_LEN, - octep_vf_gstrings_tx_q_stats[j], i); - strings += ETH_GSTRING_LEN; - } - } - - for (i = 0; i < num_queues; i++) { - for (j = 0; j < OCTEP_VF_RX_Q_STATS_CNT; j++) { - snprintf(strings, ETH_GSTRING_LEN, - octep_vf_gstrings_rx_q_stats[j], i); - strings += ETH_GSTRING_LEN; - } - } + for (i = 0; i < OCTEP_VF_GLOBAL_STATS_CNT; i++) + ethtool_puts(&data, octep_vf_gstrings_global_stats[i]); + + for (i = 0; i < num_queues; i++) + for (j = 0; j < OCTEP_VF_TX_Q_STATS_CNT; j++) + ethtool_sprintf(&data, + octep_vf_gstrings_tx_q_stats[j], + i); + + for (i = 0; i < num_queues; i++) + for (j = 0; j < OCTEP_VF_RX_Q_STATS_CNT; j++) + ethtool_sprintf(&data, + octep_vf_gstrings_rx_q_stats[j], + i); break; default: break;
Simplifies the function and avoids manual pointer manipulation. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- .../marvell/octeon_ep_vf/octep_vf_ethtool.c | 36 ++++++++----------- 1 file changed, 14 insertions(+), 22 deletions(-)