Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
> 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
Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
On 2017-11-29, at 03:04:53 +0300, Dan Carpenter wrote:
> On Tue, Nov 28, 2017 at 11:33:37PM +, Jeremy Sowden wrote:
> > On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote:
> > > On Mon, Nov 27, 2017 at 12:44:48PM +, 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.
signature.asc
Description: PGP signature
Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
On Tue, Nov 28, 2017 at 11:33:37PM +, Jeremy Sowden wrote:
> On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote:
> > On Mon, Nov 27, 2017 at 12:44:48PM +, 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;
Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
On 2017-11-28, at 17:15:24 +0300, Dan Carpenter wrote:
> On Mon, Nov 27, 2017 at 12:44:48PM +, 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;
Re: [PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
On Mon, Nov 27, 2017 at 12:44:48PM +, 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
> ---
> .../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
[PATCH v2 1/3] media: staging: atomisp: fix for sparse "using plain integer as NULL pointer" warnings.
The "address" member of struct ia_css_host_data is a pointer-to-char, so define
default as NULL.
Signed-off-by: Jeremy Sowden
---
.../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 } } } }
#define IA_CSS_DEFAULT_ISP_CSS_PARAMS \
{ { { { 0, 0 } } } }
--
2.15.0
