| Message ID | 20171127124450.28799-2-jeremy@azazel.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 27, 2017 at 12:44:48PM +0000, Jeremy Sowden wrote: > The "address" member of struct ia_css_host_data is a pointer-to-char, so define default as NULL. > > Signed-off-by: Jeremy Sowden <jeremy@azazel.net> > --- > .../css2400/runtime/isp_param/interface/ia_css_isp_param_types.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > index 8e651b80345a..6fee3f7fd184 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets { > }; > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ > - { { { { 0, 0 } } } } > + { { { { NULL, 0 } } } } This define is way ugly and instead of making superficial changes, you should try to eliminate it. People look at warnings as a bad thing but they are actually a valuable resource which call attention to bad code. By making this change you're kind of wasting the warning. The bad code is still there, it's just swept under the rug but like a dead mouse carcass it's still stinking up the living room. We should leave the warning there until it irritates someone enough to fix it properly. regards, dan carpenter
On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote: > On Mon, Nov 27, 2017 at 12:44:48PM +0000, Jeremy Sowden wrote: > > The "address" member of struct ia_css_host_data is a > > pointer-to-char, so define default as NULL. > > > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > > @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets { > > }; > > > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ > > - { { { { 0, 0 } } } } > > + { { { { NULL, 0 } } } } > > This define is way ugly and instead of making superficial changes, you > should try to eliminate it. > > People look at warnings as a bad thing but they are actually a > valuable resource which call attention to bad code. By making this > change you're kind of wasting the warning. The bad code is still > there, it's just swept under the rug but like a dead mouse carcass > it's still stinking up the living room. We should leave the warning > there until it irritates someone enough to fix it properly. Tracking down the offending initializer was definitely a pain. Compound literals with designated initializers would make this macro (and a number of others) easier to understand and more type-safe: #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ - { { { { 0, 0 } } } } + (struct ia_css_isp_param_host_segments) { \ + .params = { { \ + (struct ia_css_host_data) { \ + .address = NULL, \ + .size = 0 \ + } \ + } } \ + } Unfortunately this default value is one end of a chain of default values used to initialize members of default values of enclosing structs where the outermost values are used to initialize some static variables: static enum ia_css_err init_pipe_defaults(enum ia_css_pipe_mode mode, struct ia_css_pipe *pipe, bool copy_pipe) { static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE; static struct ia_css_preview_settings prev = IA_CSS_DEFAULT_PREVIEW_SETTINGS; static struct ia_css_capture_settings capt = IA_CSS_DEFAULT_CAPTURE_SETTINGS; static struct ia_css_video_settings video = IA_CSS_DEFAULT_VIDEO_SETTINGS; static struct ia_css_yuvpp_settings yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; if (pipe == NULL) { IA_CSS_ERROR("NULL pipe parameter"); return IA_CSS_ERR_INVALID_ARGUMENTS; } /* Initialize pipe to pre-defined defaults */ *pipe = default_pipe; /* TODO: JB should not be needed, but temporary backward reference */ switch (mode) { case IA_CSS_PIPE_MODE_PREVIEW: pipe->mode = IA_CSS_PIPE_ID_PREVIEW; pipe->pipe_settings.preview = prev; break; case IA_CSS_PIPE_MODE_CAPTURE: if (copy_pipe) { pipe->mode = IA_CSS_PIPE_ID_COPY; } else { pipe->mode = IA_CSS_PIPE_ID_CAPTURE; } pipe->pipe_settings.capture = capt; break; case IA_CSS_PIPE_MODE_VIDEO: pipe->mode = IA_CSS_PIPE_ID_VIDEO; pipe->pipe_settings.video = video; break; case IA_CSS_PIPE_MODE_ACC: pipe->mode = IA_CSS_PIPE_ID_ACC; break; case IA_CSS_PIPE_MODE_COPY: pipe->mode = IA_CSS_PIPE_ID_CAPTURE; break; case IA_CSS_PIPE_MODE_YUVPP: pipe->mode = IA_CSS_PIPE_ID_YUVPP; pipe->pipe_settings.yuvpp = yuvpp; break; default: return IA_CSS_ERR_INVALID_ARGUMENTS; } return IA_CSS_SUCCESS; } and GCC's limited support for using compound literals to initialize static variables doesn't stretch this far. I'm not convinced, however, that those variables actually achieve very much. If I change the code to assign the defaults directly, the problem goes away: diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c index f92b6a9f77eb..671b2c732a46 100644 --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c @@ -2291,25 +2291,19 @@ init_pipe_defaults(enum ia_css_pipe_mode mode, struct ia_css_pipe *pipe, bool copy_pipe) { - static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE; - static struct ia_css_preview_settings prev = IA_CSS_DEFAULT_PREVIEW_SETTINGS; - static struct ia_css_capture_settings capt = IA_CSS_DEFAULT_CAPTURE_SETTINGS; - static struct ia_css_video_settings video = IA_CSS_DEFAULT_VIDEO_SETTINGS; - static struct ia_css_yuvpp_settings yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; - if (pipe == NULL) { IA_CSS_ERROR("NULL pipe parameter"); return IA_CSS_ERR_INVALID_ARGUMENTS; } /* Initialize pipe to pre-defined defaults */ - *pipe = default_pipe; + *pipe = IA_CSS_DEFAULT_PIPE; /* TODO: JB should not be needed, but temporary backward reference */ switch (mode) { case IA_CSS_PIPE_MODE_PREVIEW: pipe->mode = IA_CSS_PIPE_ID_PREVIEW; - pipe->pipe_settings.preview = prev; + pipe->pipe_settings.preview = IA_CSS_DEFAULT_PREVIEW_SETTINGS; break; case IA_CSS_PIPE_MODE_CAPTURE: if (copy_pipe) { @@ -2317,11 +2311,11 @@ init_pipe_defaults(enum ia_css_pipe_mode mode, } else { pipe->mode = IA_CSS_PIPE_ID_CAPTURE; } - pipe->pipe_settings.capture = capt; + pipe->pipe_settings.capture = IA_CSS_DEFAULT_CAPTURE_SETTINGS; break; case IA_CSS_PIPE_MODE_VIDEO: pipe->mode = IA_CSS_PIPE_ID_VIDEO; - pipe->pipe_settings.video = video; + pipe->pipe_settings.video = IA_CSS_DEFAULT_VIDEO_SETTINGS; break; case IA_CSS_PIPE_MODE_ACC: pipe->mode = IA_CSS_PIPE_ID_ACC; @@ -2331,7 +2325,7 @@ init_pipe_defaults(enum ia_css_pipe_mode mode, break; case IA_CSS_PIPE_MODE_YUVPP: pipe->mode = IA_CSS_PIPE_ID_YUVPP; - pipe->pipe_settings.yuvpp = yuvpp; + pipe->pipe_settings.yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; break; default: return IA_CSS_ERR_INVALID_ARGUMENTS; Does this seem reasonable or am I barking up the wrong tree? J.
On Tue, Nov 28, 2017 at 11:33:37PM +0000, Jeremy Sowden wrote: > On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote: > > On Mon, Nov 27, 2017 at 12:44:48PM +0000, Jeremy Sowden wrote: > > > The "address" member of struct ia_css_host_data is a > > > pointer-to-char, so define default as NULL. > > > > > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > > > @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets { > > > }; > > > > > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ > > > - { { { { 0, 0 } } } } > > > + { { { { NULL, 0 } } } } > > > > This define is way ugly and instead of making superficial changes, you > > should try to eliminate it. > > > > People look at warnings as a bad thing but they are actually a > > valuable resource which call attention to bad code. By making this > > change you're kind of wasting the warning. The bad code is still > > there, it's just swept under the rug but like a dead mouse carcass > > it's still stinking up the living room. We should leave the warning > > there until it irritates someone enough to fix it properly. > > Tracking down the offending initializer was definitely a pain. > > Compound literals with designated initializers would make this macro > (and a number of others) easier to understand and more type-safe: > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ > - { { { { 0, 0 } } } } > + (struct ia_css_isp_param_host_segments) { \ > + .params = { { \ > + (struct ia_css_host_data) { \ > + .address = NULL, \ > + .size = 0 \ > + } \ > + } } \ > + } Using designated initializers is good, yes. Can't we just use an empty initializer since this is all zeroed memory anyway? (struct ia_css_isp_param_host_segments) { } I haven't tried it. > > Unfortunately this default value is one end of a chain of default values Yeah. A really long chain... > used to initialize members of default values of enclosing structs where > the outermost values are used to initialize some static variables: > > static enum ia_css_err > init_pipe_defaults(enum ia_css_pipe_mode mode, > struct ia_css_pipe *pipe, > bool copy_pipe) > { > static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE; > static struct ia_css_preview_settings prev = IA_CSS_DEFAULT_PREVIEW_SETTINGS; > static struct ia_css_capture_settings capt = IA_CSS_DEFAULT_CAPTURE_SETTINGS; > static struct ia_css_video_settings video = IA_CSS_DEFAULT_VIDEO_SETTINGS; > static struct ia_css_yuvpp_settings yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; > > if (pipe == NULL) { > IA_CSS_ERROR("NULL pipe parameter"); > return IA_CSS_ERR_INVALID_ARGUMENTS; > } > > /* Initialize pipe to pre-defined defaults */ > *pipe = default_pipe; > > /* TODO: JB should not be needed, but temporary backward reference */ > switch (mode) { > case IA_CSS_PIPE_MODE_PREVIEW: > pipe->mode = IA_CSS_PIPE_ID_PREVIEW; > pipe->pipe_settings.preview = prev; > break; > case IA_CSS_PIPE_MODE_CAPTURE: > if (copy_pipe) { > pipe->mode = IA_CSS_PIPE_ID_COPY; > } else { > pipe->mode = IA_CSS_PIPE_ID_CAPTURE; > } > pipe->pipe_settings.capture = capt; > break; > case IA_CSS_PIPE_MODE_VIDEO: > pipe->mode = IA_CSS_PIPE_ID_VIDEO; > pipe->pipe_settings.video = video; > break; > case IA_CSS_PIPE_MODE_ACC: > pipe->mode = IA_CSS_PIPE_ID_ACC; > break; > case IA_CSS_PIPE_MODE_COPY: > pipe->mode = IA_CSS_PIPE_ID_CAPTURE; > break; > case IA_CSS_PIPE_MODE_YUVPP: > pipe->mode = IA_CSS_PIPE_ID_YUVPP; > pipe->pipe_settings.yuvpp = yuvpp; > break; > default: > return IA_CSS_ERR_INVALID_ARGUMENTS; > } > > return IA_CSS_SUCCESS; > } > > and GCC's limited support for using compound literals to initialize > static variables doesn't stretch this far. > > I'm not convinced, however, that those variables actually achieve very > much. If I change the code to assign the defaults directly, the problem > goes away: > > diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c > index f92b6a9f77eb..671b2c732a46 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/sh_css.c > @@ -2291,25 +2291,19 @@ init_pipe_defaults(enum ia_css_pipe_mode mode, > struct ia_css_pipe *pipe, > bool copy_pipe) > { > - static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE; > - static struct ia_css_preview_settings prev = IA_CSS_DEFAULT_PREVIEW_SETTINGS; > - static struct ia_css_capture_settings capt = IA_CSS_DEFAULT_CAPTURE_SETTINGS; > - static struct ia_css_video_settings video = IA_CSS_DEFAULT_VIDEO_SETTINGS; > - static struct ia_css_yuvpp_settings yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; > - > if (pipe == NULL) { > IA_CSS_ERROR("NULL pipe parameter"); > return IA_CSS_ERR_INVALID_ARGUMENTS; > } > > /* Initialize pipe to pre-defined defaults */ > - *pipe = default_pipe; > + *pipe = IA_CSS_DEFAULT_PIPE; > > /* TODO: JB should not be needed, but temporary backward reference */ > switch (mode) { > case IA_CSS_PIPE_MODE_PREVIEW: > pipe->mode = IA_CSS_PIPE_ID_PREVIEW; > - pipe->pipe_settings.preview = prev; > + pipe->pipe_settings.preview = IA_CSS_DEFAULT_PREVIEW_SETTINGS; > break; > case IA_CSS_PIPE_MODE_CAPTURE: > if (copy_pipe) { > @@ -2317,11 +2311,11 @@ init_pipe_defaults(enum ia_css_pipe_mode mode, > } else { > pipe->mode = IA_CSS_PIPE_ID_CAPTURE; > } > - pipe->pipe_settings.capture = capt; > + pipe->pipe_settings.capture = IA_CSS_DEFAULT_CAPTURE_SETTINGS; > break; > case IA_CSS_PIPE_MODE_VIDEO: > pipe->mode = IA_CSS_PIPE_ID_VIDEO; > - pipe->pipe_settings.video = video; > + pipe->pipe_settings.video = IA_CSS_DEFAULT_VIDEO_SETTINGS; > break; > case IA_CSS_PIPE_MODE_ACC: > pipe->mode = IA_CSS_PIPE_ID_ACC; > @@ -2331,7 +2325,7 @@ init_pipe_defaults(enum ia_css_pipe_mode mode, > break; > case IA_CSS_PIPE_MODE_YUVPP: > pipe->mode = IA_CSS_PIPE_ID_YUVPP; > - pipe->pipe_settings.yuvpp = yuvpp; > + pipe->pipe_settings.yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; > break; > default: > return IA_CSS_ERR_INVALID_ARGUMENTS; > > Does this seem reasonable or am I barking up the wrong tree? Yes. Chopping the chain down and deleting as much of this code as possible seems a good thing. regards, dan carpenter
On 2017-11-29, at 03:04:53 +0300, Dan Carpenter wrote: > On Tue, Nov 28, 2017 at 11:33:37PM +0000, Jeremy Sowden wrote: > > On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote: > > > On Mon, Nov 27, 2017 at 12:44:48PM +0000, Jeremy Sowden wrote: > > > > The "address" member of struct ia_css_host_data is a > > > > pointer-to-char, so define default as NULL. > > > > > > > > --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > > > > +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h > > > > @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets { > > > > }; > > > > > > > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ > > > > - { { { { 0, 0 } } } } > > > > + { { { { NULL, 0 } } } } > > > > > > This define is way ugly and instead of making superficial changes, you > > > should try to eliminate it. > > > > > > People look at warnings as a bad thing but they are actually a > > > valuable resource which call attention to bad code. By making this > > > change you're kind of wasting the warning. The bad code is still > > > there, it's just swept under the rug but like a dead mouse carcass > > > it's still stinking up the living room. We should leave the warning > > > there until it irritates someone enough to fix it properly. > > > > Tracking down the offending initializer was definitely a pain. > > > > Compound literals with designated initializers would make this macro > > (and a number of others) easier to understand and more type-safe: > > > > #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ > > - { { { { 0, 0 } } } } > > + (struct ia_css_isp_param_host_segments) { \ > > + .params = { { \ > > + (struct ia_css_host_data) { \ > > + .address = NULL, \ > > + .size = 0 \ > > + } \ > > + } } \ > > + } > > Using designated initializers is good, yes. Can't we just use an > empty initializer since this is all zeroed memory anyway? > > (struct ia_css_isp_param_host_segments) { } > > I haven't tried it. There are 35 defaults defined by macros like this, most of them much more complicated that IA_CSS_DEFAULT_ISP_MEM_PARAMS, and a few members are initialized to non-zero values. My plan, therefore, is to convert everything to use designated initializers, and then start removing the zeroes afterwards. > > > > Unfortunately this default value is one end of a chain of default values > > Yeah. A really long chain... > > > used to initialize members of default values of enclosing structs where > > the outermost values are used to initialize some static variables: > > > > static enum ia_css_err > > init_pipe_defaults(enum ia_css_pipe_mode mode, > > struct ia_css_pipe *pipe, > > bool copy_pipe) > > { > > static struct ia_css_pipe default_pipe = IA_CSS_DEFAULT_PIPE; > > static struct ia_css_preview_settings prev = IA_CSS_DEFAULT_PREVIEW_SETTINGS; > > static struct ia_css_capture_settings capt = IA_CSS_DEFAULT_CAPTURE_SETTINGS; > > static struct ia_css_video_settings video = IA_CSS_DEFAULT_VIDEO_SETTINGS; > > static struct ia_css_yuvpp_settings yuvpp = IA_CSS_DEFAULT_YUVPP_SETTINGS; > > > > if (pipe == NULL) { > > IA_CSS_ERROR("NULL pipe parameter"); > > return IA_CSS_ERR_INVALID_ARGUMENTS; > > } > > > > /* Initialize pipe to pre-defined defaults */ > > *pipe = default_pipe; > > > > [...] > > > > I'm not convinced, however, that those variables actually achieve very > > much. If I change the code to assign the defaults directly, the problem > > goes away: > > > > [...] > > > > Does this seem reasonable or am I barking up the wrong tree? > > Yes. Chopping the chain down and deleting as much of this code as > possible seems a good thing. I'll get chopping. J.
> There are 35 defaults defined by macros like this, most of them much > more complicated that IA_CSS_DEFAULT_ISP_MEM_PARAMS, and a few members > are initialized to non-zero values. My plan, therefore, is to convert > everything to use designated initializers, and then start removing the > zeroes afterwards. Where they are only used once in the tree it might be even cleaner to just do static struct foo = FOO_DEFAULT_BAR; foo.x = 12; foo.bar = &foo; etc in the code. Alan
The CSS API uses a lot of nested anonymous structs defined in object
macros to assign default values to its data-structures. These have been
changed to use compound-literals and designated initializers to make
them more comprehensible and less fragile.
The compound-literals can also be used in assignment, which made it
possible get rid of some temporary variables whose only purpose is to be
initialized by one of these anonymous structs and then serve as the
rvalue in an assignment expression.
The designated initializers also allow the removal of lots of
struct-members initialized to zero values.
I made the changes in three stages: firstly, I converted the default
values to compound-literals and designated initializers and removed the
temporary variables; secondly, I removed the zero-valued struct-members;
finally, I removed some structs which had become empty.
Jeremy Sowden (3):
media: atomisp: convert default struct values to use compound-literals
with designated initializers.
media: atomisp: delete zero-valued struct members.
media: atomisp: delete empty default struct values.
.../hive_isp_css_common/input_formatter_global.h | 16 ---
.../pci/atomisp2/css2400/ia_css_frame_public.h | 29 ++----
.../atomisp/pci/atomisp2/css2400/ia_css_pipe.h | 110 +++++++--------------
.../pci/atomisp2/css2400/ia_css_pipe_public.h | 108 +++-----------------
.../atomisp/pci/atomisp2/css2400/ia_css_types.h | 64 +++---------
.../isp/kernels/s3a/s3a_1.0/ia_css_s3a_types.h | 50 +---------
.../kernels/sdis/common/ia_css_sdis_common_types.h | 31 ++----
.../isp/kernels/sdis/sdis_1.0/ia_css_sdis.host.c | 3 +-
.../runtime/binary/interface/ia_css_binary.h | 88 ++---------------
.../atomisp2/css2400/runtime/binary/src/binary.c | 3 +-
.../isp_param/interface/ia_css_isp_param_types.h | 10 --
.../runtime/pipeline/interface/ia_css_pipeline.h | 24 ++---
.../css2400/runtime/pipeline/src/pipeline.c | 7 +-
.../media/atomisp/pci/atomisp2/css2400/sh_css.c | 31 ++----
.../atomisp/pci/atomisp2/css2400/sh_css_legacy.h | 11 ---
.../atomisp/pci/atomisp2/css2400/sh_css_metrics.h | 21 ----
16 files changed, 114 insertions(+), 492 deletions(-)
base-commit: 37cb8e1f8e10c6e9bd2a1b95cdda0620a21b0551
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h index 8e651b80345a..6fee3f7fd184 100644 --- a/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h +++ b/drivers/staging/media/atomisp/pci/atomisp2/css2400/runtime/isp_param/interface/ia_css_isp_param_types.h @@ -95,7 +95,7 @@ union ia_css_all_memory_offsets { }; #define IA_CSS_DEFAULT_ISP_MEM_PARAMS \ - { { { { 0, 0 } } } } + { { { { NULL, 0 } } } } #define IA_CSS_DEFAULT_ISP_CSS_PARAMS \ { { { { 0, 0 } } } }
The "address" member of struct ia_css_host_data is a pointer-to-char, so define default as NULL. Signed-off-by: Jeremy Sowden <jeremy@azazel.net> --- .../css2400/runtime/isp_param/interface/ia_css_isp_param_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)