| Message ID | 20240730125856.7321-1-ubizjak@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/chelsio/libcxgb: Add __percpu annotations to libcxgb_ppm.c | expand |
On Tue, Jul 30, 2024 at 02:58:19PM +0200, Uros Bizjak wrote: > Compiling libcxgb_ppm.c results in several sparse warnings: > > libcxgb_ppm.c:368:15: warning: incorrect type in assignment (different address spaces) > libcxgb_ppm.c:368:15: expected struct cxgbi_ppm_pool *pools > libcxgb_ppm.c:368:15: got void [noderef] __percpu *_res > libcxgb_ppm.c:374:48: warning: incorrect type in initializer (different address spaces) > libcxgb_ppm.c:374:48: expected void const [noderef] __percpu *__vpp_verify > libcxgb_ppm.c:374:48: got struct cxgbi_ppm_pool * > libcxgb_ppm.c:484:19: warning: incorrect type in assignment (different address spaces) > libcxgb_ppm.c:484:19: expected struct cxgbi_ppm_pool [noderef] __percpu *pool > libcxgb_ppm.c:484:19: got struct cxgbi_ppm_pool *[assigned] pool > libcxgb_ppm.c:511:21: warning: incorrect type in argument 1 (different address spaces) > libcxgb_ppm.c:511:21: expected void [noderef] __percpu *__pdata > libcxgb_ppm.c:511:21: got struct cxgbi_ppm_pool *[assigned] pool > > Add __percpu annotation to *pools and *pool percpu pointers and to > ppm_alloc_cpu_pool() function that returns percpu pointer to fix > these warnings. > > Compile tested only, but there is no difference in the resulting object file. > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Paolo Abeni <pabeni@redhat.com> > --- > drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c > index 854d87e1125c..01d776113500 100644 > --- a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c > +++ b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c > @@ -342,10 +342,10 @@ int cxgbi_ppm_release(struct cxgbi_ppm *ppm) > } > EXPORT_SYMBOL(cxgbi_ppm_release); > > -static struct cxgbi_ppm_pool *ppm_alloc_cpu_pool(unsigned int *total, > - unsigned int *pcpu_ppmax) > +static struct cxgbi_ppm_pool __percpu *ppm_alloc_cpu_pool(unsigned int *total, > + unsigned int *pcpu_ppmax) Let's keep to less than 80 columns wide, as is still preferred for Networking code. Perhaps in this case: static struct cxgbi_ppm_pool __percpu * ppm_alloc_cpu_pool(unsigned int *total, unsigned int *pcpu_ppmax) Also, I do observe that this is an old driver, so the value of cleaning it is perhaps limited. But the above aside, this looks good to me. > { > - struct cxgbi_ppm_pool *pools; > + struct cxgbi_ppm_pool __percpu *pools; > unsigned int ppmax = (*total) / num_possible_cpus(); > unsigned int max = (PCPU_MIN_UNIT_SIZE - sizeof(*pools)) << 3; > unsigned int bmap;
On Wed, Jul 31, 2024 at 11:35 AM Simon Horman <horms@kernel.org> wrote: > > On Tue, Jul 30, 2024 at 02:58:19PM +0200, Uros Bizjak wrote: > > Compiling libcxgb_ppm.c results in several sparse warnings: > > > > libcxgb_ppm.c:368:15: warning: incorrect type in assignment (different address spaces) > > libcxgb_ppm.c:368:15: expected struct cxgbi_ppm_pool *pools > > libcxgb_ppm.c:368:15: got void [noderef] __percpu *_res > > libcxgb_ppm.c:374:48: warning: incorrect type in initializer (different address spaces) > > libcxgb_ppm.c:374:48: expected void const [noderef] __percpu *__vpp_verify > > libcxgb_ppm.c:374:48: got struct cxgbi_ppm_pool * > > libcxgb_ppm.c:484:19: warning: incorrect type in assignment (different address spaces) > > libcxgb_ppm.c:484:19: expected struct cxgbi_ppm_pool [noderef] __percpu *pool > > libcxgb_ppm.c:484:19: got struct cxgbi_ppm_pool *[assigned] pool > > libcxgb_ppm.c:511:21: warning: incorrect type in argument 1 (different address spaces) > > libcxgb_ppm.c:511:21: expected void [noderef] __percpu *__pdata > > libcxgb_ppm.c:511:21: got struct cxgbi_ppm_pool *[assigned] pool > > > > Add __percpu annotation to *pools and *pool percpu pointers and to > > ppm_alloc_cpu_pool() function that returns percpu pointer to fix > > these warnings. > > > > Compile tested only, but there is no difference in the resulting object file. > > > > Signed-off-by: Uros Bizjak <ubizjak@gmail.com> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Paolo Abeni <pabeni@redhat.com> > > --- > > drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c > > index 854d87e1125c..01d776113500 100644 > > --- a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c > > +++ b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c > > @@ -342,10 +342,10 @@ int cxgbi_ppm_release(struct cxgbi_ppm *ppm) > > } > > EXPORT_SYMBOL(cxgbi_ppm_release); > > > > -static struct cxgbi_ppm_pool *ppm_alloc_cpu_pool(unsigned int *total, > > - unsigned int *pcpu_ppmax) > > +static struct cxgbi_ppm_pool __percpu *ppm_alloc_cpu_pool(unsigned int *total, > > + unsigned int *pcpu_ppmax) > > Let's keep to less than 80 columns wide, as is still preferred for > Networking code. Perhaps in this case: > > static struct cxgbi_ppm_pool __percpu * > ppm_alloc_cpu_pool(unsigned int *total, unsigned int *pcpu_ppmax) Hm, the original is exactly what sparse dumped, IMO code dumps should be left as they were dumped. > Also, I do observe that this is an old driver, so the value > of cleaning it is perhaps limited. It broke my strict percpu checker with allyesconfig. Fixing it was trivial and resulted in no object code changes. > But the above aside, this looks good to me. Thanks, Uros.
On Wed, 31 Jul 2024 12:06:18 +0200 Uros Bizjak wrote: > > Let's keep to less than 80 columns wide, as is still preferred for > > Networking code. Perhaps in this case: > > > > static struct cxgbi_ppm_pool __percpu * > > ppm_alloc_cpu_pool(unsigned int *total, unsigned int *pcpu_ppmax) > > Hm, the original is exactly what sparse dumped, IMO code dumps should > be left as they were dumped. Unclear to me what you mean by "dumped" here. Please follow Simon's advice.
On Thu, Aug 1, 2024 at 3:11 AM Jakub Kicinski <kuba@kernel.org> wrote: > > On Wed, 31 Jul 2024 12:06:18 +0200 Uros Bizjak wrote: > > > Let's keep to less than 80 columns wide, as is still preferred for > > > Networking code. Perhaps in this case: > > > > > > static struct cxgbi_ppm_pool __percpu * > > > ppm_alloc_cpu_pool(unsigned int *total, unsigned int *pcpu_ppmax) > > > > Hm, the original is exactly what sparse dumped, IMO code dumps should > > be left as they were dumped. > > Unclear to me what you mean by "dumped" here. > Please follow Simon's advice. Oh, sorry, I totally misunderstood Simon's email. I have sent v2 that includes requested change. Thanks, Uros.
diff --git a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c index 854d87e1125c..01d776113500 100644 --- a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c +++ b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c @@ -342,10 +342,10 @@ int cxgbi_ppm_release(struct cxgbi_ppm *ppm) } EXPORT_SYMBOL(cxgbi_ppm_release); -static struct cxgbi_ppm_pool *ppm_alloc_cpu_pool(unsigned int *total, - unsigned int *pcpu_ppmax) +static struct cxgbi_ppm_pool __percpu *ppm_alloc_cpu_pool(unsigned int *total, + unsigned int *pcpu_ppmax) { - struct cxgbi_ppm_pool *pools; + struct cxgbi_ppm_pool __percpu *pools; unsigned int ppmax = (*total) / num_possible_cpus(); unsigned int max = (PCPU_MIN_UNIT_SIZE - sizeof(*pools)) << 3; unsigned int bmap; @@ -392,7 +392,7 @@ int cxgbi_ppm_init(void **ppm_pp, struct net_device *ndev, unsigned int iscsi_edram_size) { struct cxgbi_ppm *ppm = (struct cxgbi_ppm *)(*ppm_pp); - struct cxgbi_ppm_pool *pool = NULL; + struct cxgbi_ppm_pool __percpu *pool = NULL; unsigned int pool_index_max = 0; unsigned int ppmax_pool = 0; unsigned int ppod_bmap_size;
Compiling libcxgb_ppm.c results in several sparse warnings: libcxgb_ppm.c:368:15: warning: incorrect type in assignment (different address spaces) libcxgb_ppm.c:368:15: expected struct cxgbi_ppm_pool *pools libcxgb_ppm.c:368:15: got void [noderef] __percpu *_res libcxgb_ppm.c:374:48: warning: incorrect type in initializer (different address spaces) libcxgb_ppm.c:374:48: expected void const [noderef] __percpu *__vpp_verify libcxgb_ppm.c:374:48: got struct cxgbi_ppm_pool * libcxgb_ppm.c:484:19: warning: incorrect type in assignment (different address spaces) libcxgb_ppm.c:484:19: expected struct cxgbi_ppm_pool [noderef] __percpu *pool libcxgb_ppm.c:484:19: got struct cxgbi_ppm_pool *[assigned] pool libcxgb_ppm.c:511:21: warning: incorrect type in argument 1 (different address spaces) libcxgb_ppm.c:511:21: expected void [noderef] __percpu *__pdata libcxgb_ppm.c:511:21: got struct cxgbi_ppm_pool *[assigned] pool Add __percpu annotation to *pools and *pool percpu pointers and to ppm_alloc_cpu_pool() function that returns percpu pointer to fix these warnings. Compile tested only, but there is no difference in the resulting object file. Signed-off-by: Uros Bizjak <ubizjak@gmail.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Eric Dumazet <edumazet@google.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Paolo Abeni <pabeni@redhat.com> --- drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)