| Message ID | 20230622130507.606713-4-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/3] net: macb: check constant to define and fix __be32 warnings | expand |
On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote: > The netdev_dbg() calls in gem_add_flow_filter() and gem_del_flow_filter() > call ntohl() on the ipv4 addresses, which will put them into the host order > but not the right type (returns a __be32, not an u32 as would be expected). > > Chaning the htonl() to nthol() should still do the right conversion, return > the correct u32 type and should not change any functional to remove the > following sparse warnings: > > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types) > drivers/net/ethernet/cadence/macb_main.c:3568:9: expected unsigned int [usertype] val > drivers/net/ethernet/cadence/macb_main.c:3568:9: got restricted __be32 [usertype] ip4src > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types) > drivers/net/ethernet/cadence/macb_main.c:3568:9: expected unsigned int [usertype] val > drivers/net/ethernet/cadence/macb_main.c:3568:9: got restricted __be32 [usertype] ip4dst > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 > d > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types) > drivers/net/ethernet/cadence/macb_main.c:3622:25: expected unsigned int [usertype] val > drivers/net/ethernet/cadence/macb_main.c:3622:25: got restricted __be32 [usertype] ip4src > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types) > drivers/net/ethernet/cadence/macb_main.c:3622:25: expected unsigned int [usertype] val > drivers/net/ethernet/cadence/macb_main.c:3622:25: got restricted __be32 [usertype] ip4dst > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Hi Ben, this code-change looks good to me, but I have a few minor nits for your consideration. 1. Please specify the target tree, in this case net-next, for patch sets for Networking code. Subject: [PATCH net-next ...] ... 2. It might be nicer to write '.../macb_main.c' or similar, rather tha nthe full path, in the patch description. 3. checkpatch --codespell says: 'Chaning' -> 'Chaining' > --- > drivers/net/ethernet/cadence/macb_main.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 56e202b74bd7..59a90c2b307f 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device *netdev, > netdev_dbg(netdev, > "Adding flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n", > fs->flow_type, (int)fs->ring_cookie, fs->location, > - htonl(fs->h_u.tcp_ip4_spec.ip4src), > - htonl(fs->h_u.tcp_ip4_spec.ip4dst), > + ntohl(fs->h_u.tcp_ip4_spec.ip4src), > + ntohl(fs->h_u.tcp_ip4_spec.ip4dst), > be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc), > be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst)); > > @@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device *netdev, > netdev_dbg(netdev, > "Deleting flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n", > fs->flow_type, (int)fs->ring_cookie, fs->location, > - htonl(fs->h_u.tcp_ip4_spec.ip4src), > - htonl(fs->h_u.tcp_ip4_spec.ip4dst), > + ntohl(fs->h_u.tcp_ip4_spec.ip4src), > + ntohl(fs->h_u.tcp_ip4_spec.ip4dst), > be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc), > be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst)); > > -- > 2.40.1 > >
On 2023-06-22 16:44, Simon Horman wrote: > On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote: >> The netdev_dbg() calls in gem_add_flow_filter() and >> gem_del_flow_filter() >> call ntohl() on the ipv4 addresses, which will put them into the host >> order >> but not the right type (returns a __be32, not an u32 as would be >> expected). >> >> Chaning the htonl() to nthol() should still do the right conversion, >> return >> the correct u32 type and should not change any functional to remove >> the >> following sparse warnings: >> >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect >> type in argument 1 (different base types) >> drivers/net/ethernet/cadence/macb_main.c:3568:9: expected unsigned >> int [usertype] val >> drivers/net/ethernet/cadence/macb_main.c:3568:9: got restricted >> __be32 [usertype] ip4src >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect >> type in argument 1 (different base types) >> drivers/net/ethernet/cadence/macb_main.c:3568:9: expected unsigned >> int [usertype] val >> drivers/net/ethernet/cadence/macb_main.c:3568:9: got restricted >> __be32 [usertype] ip4dst >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from >> restricted __be32 >> d >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect >> type in argument 1 (different base types) >> drivers/net/ethernet/cadence/macb_main.c:3622:25: expected unsigned >> int [usertype] val >> drivers/net/ethernet/cadence/macb_main.c:3622:25: got restricted >> __be32 [usertype] ip4src >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect >> type in argument 1 (different base types) >> drivers/net/ethernet/cadence/macb_main.c:3622:25: expected unsigned >> int [usertype] val >> drivers/net/ethernet/cadence/macb_main.c:3622:25: got restricted >> __be32 [usertype] ip4dst >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from >> restricted __be32 >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > Hi Ben, > > this code-change looks good to me, but I have a few minor nits for your > consideration. > > 1. Please specify the target tree, in this case net-next, for patch > sets > for Networking code. > > Subject: [PATCH net-next ...] ... Ah, was using net, but I assume net-next is probably ok > 2. It might be nicer to write '.../macb_main.c' or similar, > rather tha nthe full path, in the patch description. > > 3. checkpatch --codespell says: 'Chaning' -> 'Chaining' Ok, thank you. I didn't know about that. Since there's another patch that needs work I'll re-send this early next week with the fixes in. >> --- >> drivers/net/ethernet/cadence/macb_main.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c >> b/drivers/net/ethernet/cadence/macb_main.c >> index 56e202b74bd7..59a90c2b307f 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device >> *netdev, >> netdev_dbg(netdev, >> "Adding flow filter >> entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n", >> fs->flow_type, (int)fs->ring_cookie, fs->location, >> - htonl(fs->h_u.tcp_ip4_spec.ip4src), >> - htonl(fs->h_u.tcp_ip4_spec.ip4dst), >> + ntohl(fs->h_u.tcp_ip4_spec.ip4src), >> + ntohl(fs->h_u.tcp_ip4_spec.ip4dst), >> be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc), >> be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst)); >> >> @@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device >> *netdev, >> netdev_dbg(netdev, >> "Deleting flow filter >> entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n", >> fs->flow_type, (int)fs->ring_cookie, fs->location, >> - htonl(fs->h_u.tcp_ip4_spec.ip4src), >> - htonl(fs->h_u.tcp_ip4_spec.ip4dst), >> + ntohl(fs->h_u.tcp_ip4_spec.ip4src), >> + ntohl(fs->h_u.tcp_ip4_spec.ip4dst), >> be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc), >> be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst)); >> >> -- >> 2.40.1 >> >>
On Fri, Jun 23, 2023 at 10:43:12AM +0100, Ben Dooks wrote: > > > On 2023-06-22 16:44, Simon Horman wrote: > > On Thu, Jun 22, 2023 at 02:05:07PM +0100, Ben Dooks wrote: ... > > Hi Ben, > > > > this code-change looks good to me, but I have a few minor nits for your > > consideration. > > > > 1. Please specify the target tree, in this case net-next, for patch sets > > for Networking code. > > > > Subject: [PATCH net-next ...] ... > > Ah, was using net, but I assume net-next is probably ok I think net-next is best for this kind of change. FWIIW, this series did apply there. > > 2. It might be nicer to write '.../macb_main.c' or similar, > > rather tha nthe full path, in the patch description. > > > > 3. checkpatch --codespell says: 'Chaning' -> 'Chaining' > > Ok, thank you. I didn't know about that. It is quite handy :) > Since there's another patch that needs work I'll re-send this early next > week > with the fixes in. I think a repost of the series is a good plan. Please note that when v6.4 is released, which may well be over the weekend, then net-next will be closed until after rc1 is released - approximately two weeks. If it is closed then you'll need to wait until it reopens before posting v2.
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 56e202b74bd7..59a90c2b307f 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -3568,8 +3568,8 @@ static int gem_add_flow_filter(struct net_device *netdev, netdev_dbg(netdev, "Adding flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n", fs->flow_type, (int)fs->ring_cookie, fs->location, - htonl(fs->h_u.tcp_ip4_spec.ip4src), - htonl(fs->h_u.tcp_ip4_spec.ip4dst), + ntohl(fs->h_u.tcp_ip4_spec.ip4src), + ntohl(fs->h_u.tcp_ip4_spec.ip4dst), be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc), be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst)); @@ -3622,8 +3622,8 @@ static int gem_del_flow_filter(struct net_device *netdev, netdev_dbg(netdev, "Deleting flow filter entry,type=%u,queue=%u,loc=%u,src=%08X,dst=%08X,ps=%u,pd=%u\n", fs->flow_type, (int)fs->ring_cookie, fs->location, - htonl(fs->h_u.tcp_ip4_spec.ip4src), - htonl(fs->h_u.tcp_ip4_spec.ip4dst), + ntohl(fs->h_u.tcp_ip4_spec.ip4src), + ntohl(fs->h_u.tcp_ip4_spec.ip4dst), be16_to_cpu(fs->h_u.tcp_ip4_spec.psrc), be16_to_cpu(fs->h_u.tcp_ip4_spec.pdst));
The netdev_dbg() calls in gem_add_flow_filter() and gem_del_flow_filter() call ntohl() on the ipv4 addresses, which will put them into the host order but not the right type (returns a __be32, not an u32 as would be expected). Chaning the htonl() to nthol() should still do the right conversion, return the correct u32 type and should not change any functional to remove the following sparse warnings: drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types) drivers/net/ethernet/cadence/macb_main.c:3568:9: expected unsigned int [usertype] val drivers/net/ethernet/cadence/macb_main.c:3568:9: got restricted __be32 [usertype] ip4src drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: incorrect type in argument 1 (different base types) drivers/net/ethernet/cadence/macb_main.c:3568:9: expected unsigned int [usertype] val drivers/net/ethernet/cadence/macb_main.c:3568:9: got restricted __be32 [usertype] ip4dst drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3568:9: warning: cast from restricted __be32 d drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types) drivers/net/ethernet/cadence/macb_main.c:3622:25: expected unsigned int [usertype] val drivers/net/ethernet/cadence/macb_main.c:3622:25: got restricted __be32 [usertype] ip4src drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: incorrect type in argument 1 (different base types) drivers/net/ethernet/cadence/macb_main.c:3622:25: expected unsigned int [usertype] val drivers/net/ethernet/cadence/macb_main.c:3622:25: got restricted __be32 [usertype] ip4dst drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 drivers/net/ethernet/cadence/macb_main.c:3622:25: warning: cast from restricted __be32 Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> --- drivers/net/ethernet/cadence/macb_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)