| Message ID | 1408926755-12277-1-git-send-email-broonie@kernel.org |
|---|---|
| State | New |
| Headers | show |
From: Mark Brown <broonie@kernel.org> Date: Sun, 24 Aug 2014 19:32:35 -0500 > From: Mark Brown <broonie@linaro.org> > > struct rds_sock is rather large ausing the following warning in an ARM > allmodconfig: > > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=] > > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id > instead of allocating it on the stack. > > Signed-off-by: Mark Brown <broonie@linaro.org> I'd like you to fix this differently. Creating pseudo instances of objects, and partially initializing it, just to satisfy an interface is always a really bad sign. Create a key structure argument for rds_iw_get_device() and initialize that and pass it in instead, update the other caller similarly. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 25, 2014 at 12:57:40PM -0700, David Miller wrote: > From: Mark Brown <broonie@kernel.org> > > From: Mark Brown <broonie@linaro.org> > > struct rds_sock is rather large ausing the following warning in an ARM > > allmodconfig: > > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=] > > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id > > instead of allocating it on the stack. > > Signed-off-by: Mark Brown <broonie@linaro.org> > I'd like you to fix this differently. Creating pseudo instances of > objects, and partially initializing it, just to satisfy an interface > is always a really bad sign. > Create a key structure argument for rds_iw_get_device() and initialize that > and pass it in instead, update the other caller similarly. I agree that the existing code looks like it could be improved even more but please bear in mind that I'm just looking for a clean build (we've got less than 20 warnings in allmodconfig including staging at the minute) rather than actively working on this code in particular - I've no ability to do more than build testing here.
From: Mark Brown <broonie@kernel.org> Date: Tue, 26 Aug 2014 07:54:09 +0100 > On Mon, Aug 25, 2014 at 12:57:40PM -0700, David Miller wrote: >> From: Mark Brown <broonie@kernel.org> > >> > From: Mark Brown <broonie@linaro.org> > >> > struct rds_sock is rather large ausing the following warning in an ARM >> > allmodconfig: > >> > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=] > >> > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id >> > instead of allocating it on the stack. > >> > Signed-off-by: Mark Brown <broonie@linaro.org> > >> I'd like you to fix this differently. Creating pseudo instances of >> objects, and partially initializing it, just to satisfy an interface >> is always a really bad sign. > >> Create a key structure argument for rds_iw_get_device() and initialize that >> and pass it in instead, update the other caller similarly. > > I agree that the existing code looks like it could be improved even more > but please bear in mind that I'm just looking for a clean build (we've > got less than 20 warnings in allmodconfig including staging at the > minute) rather than actively working on this code in particular - I've > no ability to do more than build testing here. I understand that, but please fix this bug properly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 26, 2014 at 08:29:06AM -0700, David Miller wrote: > From: Mark Brown <broonie@kernel.org> > > I agree that the existing code looks like it could be improved even more > > but please bear in mind that I'm just looking for a clean build (we've > > got less than 20 warnings in allmodconfig including staging at the > > minute) rather than actively working on this code in particular - I've > > no ability to do more than build testing here. > I understand that, but please fix this bug properly. Please set the expectation among the networking developers that their code should compile cleanly on all architectures; currently the networking code (mostly drivers rather than the core) is the single biggest source of warnings outside of staging and if we are requring things like this then that presents a substantial barrier to others contributing to addressing the warnings.
diff --git a/net/rds/iw_rdma.c b/net/rds/iw_rdma.c index a817705..cee5daa 100644 --- a/net/rds/iw_rdma.c +++ b/net/rds/iw_rdma.c @@ -180,22 +180,28 @@ int rds_iw_update_cm_id(struct rds_iw_device *rds_iwdev, struct rdma_cm_id *cm_i { struct sockaddr_in *src_addr, *dst_addr; struct rds_iw_device *rds_iwdev_old; - struct rds_sock rs; + struct rds_sock *rs; struct rdma_cm_id *pcm_id; int rc; + rs = kzalloc(sizeof(*rs), GFP_KERNEL); + if (!rs) + return -ENOMEM; + src_addr = (struct sockaddr_in *)&cm_id->route.addr.src_addr; dst_addr = (struct sockaddr_in *)&cm_id->route.addr.dst_addr; - rs.rs_bound_addr = src_addr->sin_addr.s_addr; - rs.rs_bound_port = src_addr->sin_port; - rs.rs_conn_addr = dst_addr->sin_addr.s_addr; - rs.rs_conn_port = dst_addr->sin_port; + rs->rs_bound_addr = src_addr->sin_addr.s_addr; + rs->rs_bound_port = src_addr->sin_port; + rs->rs_conn_addr = dst_addr->sin_addr.s_addr; + rs->rs_conn_port = dst_addr->sin_port; - rc = rds_iw_get_device(&rs, &rds_iwdev_old, &pcm_id); + rc = rds_iw_get_device(rs, &rds_iwdev_old, &pcm_id); if (rc) rds_iw_remove_cm_id(rds_iwdev, cm_id); + kfree(rs); + return rds_iw_add_cm_id(rds_iwdev, cm_id); }