| Message ID | 20240910-ti-warn-v1-1-afd1e404abbe@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: ti: Address some warnings | expand |
On Tue, 10 Sep 2024 08:17:56 +0100 Simon Horman wrote: > An alternate, approach would be to create a variant of > devm_add_action_or_reset() which expects __percpu data. This would > avoid discarding the __percpu annotation, and any value it may have > between the casts added by this patch. However, doing so appears to > require a significant amount of plumbing. And, as far as I can see, the > code updated by this patch would be the only user of it. So this patch > takes a simpler approach. Sorry if this was already discussed, but struct am65_cpsw_ndev_stats appears to be identical to struct pcpu_sw_netstats but for ordering. Can we let the core allocate the stats by setting netdev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS?
On Wed, Sep 11, 2024 at 05:06:43PM -0700, Jakub Kicinski wrote: > On Tue, 10 Sep 2024 08:17:56 +0100 Simon Horman wrote: > > An alternate, approach would be to create a variant of > > devm_add_action_or_reset() which expects __percpu data. This would > > avoid discarding the __percpu annotation, and any value it may have > > between the casts added by this patch. However, doing so appears to > > require a significant amount of plumbing. And, as far as I can see, the > > code updated by this patch would be the only user of it. So this patch > > takes a simpler approach. > > Sorry if this was already discussed, but struct am65_cpsw_ndev_stats > appears to be identical to struct pcpu_sw_netstats but for ordering. > Can we let the core allocate the stats by setting > netdev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS? Hi Jakub, Thanks for pointing that out, and sorry for not thinking of it myself. Looking over the code, and taking a first pass at implementing this, I believe the answer is yes :) I also think that, as a second step, by using dev_core_stats, the custom ndo_get_stats64() implementation can be removed. LMKWYT.
On Thu, 12 Sep 2024 10:58:13 +0100 Simon Horman wrote: > Thanks for pointing that out, and sorry for not thinking of it myself. > > Looking over the code, and taking a first pass at implementing this, > I believe the answer is yes :) > > I also think that, as a second step, by using dev_core_stats, > the custom ndo_get_stats64() implementation can be removed. > LMKWYT. Second step or one conversion patch, no preference. But AFAICT you're right, the ndo can be completely removed thanks to the conversion.
On Thu, Sep 12, 2024 at 08:45:15AM -0700, Jakub Kicinski wrote: > On Thu, 12 Sep 2024 10:58:13 +0100 Simon Horman wrote: > > Thanks for pointing that out, and sorry for not thinking of it myself. > > > > Looking over the code, and taking a first pass at implementing this, > > I believe the answer is yes :) > > > > I also think that, as a second step, by using dev_core_stats, > > the custom ndo_get_stats64() implementation can be removed. > > LMKWYT. > > Second step or one conversion patch, no preference. But AFAICT you're > right, the ndo can be completely removed thanks to the conversion. Thanks, I'll see about making it so.
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c index b7e5d0fb5d19..a4b0e4bb7529 100644 --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c @@ -2671,9 +2671,7 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common) static void am65_cpsw_pcpu_stats_free(void *data) { - struct am65_cpsw_ndev_stats __percpu *stats = data; - - free_percpu(stats); + free_percpu((void __percpu *)data); } static void am65_cpsw_nuss_phylink_cleanup(struct am65_cpsw_common *common) @@ -2794,7 +2792,7 @@ am65_cpsw_nuss_init_port_ndev(struct am65_cpsw_common *common, u32 port_idx) return -ENOMEM; ret = devm_add_action_or_reset(dev, am65_cpsw_pcpu_stats_free, - ndev_priv->stats); + (__force void *)ndev_priv->stats); if (ret) dev_err(dev, "failed to add percpu stat free action %d\n", ret);
Sparse flags a mismatch between percpu stats data, and it's use with actions which handle non-percpu data. .../am65-cpsw-nuss.c:2658:55: warning: incorrect type in initializer (different address spaces) .../am65-cpsw-nuss.c:2658:55: expected struct am65_cpsw_ndev_stats [noderef] __percpu *stats .../am65-cpsw-nuss.c:2658:55: got void *data .../am65-cpsw-nuss.c:2781:15: warning: incorrect type in argument 3 (different address spaces) .../am65-cpsw-nuss.c:2781:15: expected void *data .../am65-cpsw-nuss.c:2781:15: got struct am65_cpsw_ndev_stats [noderef] __percpu *stats Address this using casts. An alternate, approach would be to create a variant of devm_add_action_or_reset() which expects __percpu data. This would avoid discarding the __percpu annotation, and any value it may have between the casts added by this patch. However, doing so appears to require a significant amount of plumbing. And, as far as I can see, the code updated by this patch would be the only user of it. So this patch takes a simpler approach. No functional change intended. Compile tested only. Signed-off-by: Simon Horman <horms@kernel.org> --- drivers/net/ethernet/ti/am65-cpsw-nuss.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)