Re: [PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration

[Greg Kroah-Hartman]

On Mon, Feb 12, 2024 at 11:14:29AM +0100, Arnd Bergmann wrote:
> On Mon, Feb 12, 2024, at 11:02, Greg Kroah-Hartman wrote:
> > On Mon, Feb 12, 2024 at 11:46:18AM +0200, Sakari Ailus wrote:
> >> Assign all possible fields of pinfo in variable declaration, instead of
> >> just zeroing it there.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/misc/mei/vsc-tp.c | 16 ++++++++--------
> >>  1 file changed, 8 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> >> index 200af14490d7..1eda2860f63b 100644
> >> --- a/drivers/misc/mei/vsc-tp.c
> >> +++ b/drivers/misc/mei/vsc-tp.c
> >> @@ -447,11 +447,16 @@ static int vsc_tp_match_any(struct acpi_device *adev, void *data)
> >>  
> >>  static int vsc_tp_probe(struct spi_device *spi)
> >>  {
> >> -	struct platform_device_info pinfo = { 0 };
> >> +	struct vsc_tp *tp;
> >> +	struct platform_device_info pinfo = {
> >> +		.name = "intel_vsc",
> >> +		.data = &tp,
> >> +		.size_data = sizeof(tp),
> >> +		.id = PLATFORM_DEVID_NONE,
> >> +	};
> >
> > But now you have potential stack data in the structure for the fields
> > that you aren't assigning here, right?  Is that acceptable, or will it
> > leak somewhere?
> >
> > This is why we generally do not do this type of style.  So unless you
> > are fixing an issue here, please don't do it.
> 
> If you have any initializer, all named fields in the structure
> are zeroed. The only bits of the structure that may contain
> stack data are for padding between fields, but that doesn't
> actually change here from the previous version.

I thought we had looked into that before and it would 0 out everything
if you just had the {0} initializer, including holes?  Or was it not, or
did it depend on the compiler/version?
Sorry, I never remember and so just recommend a memset which should be
the same overall.

> The old version you have here just skips the named fields
> and otherwise would end up lookingn like
> 
> struct platform_device_info pinfo = {
>       .parent = 0,
> };
> 
> which is still a partial initializer and has the added
> problem of relying on a literal '0' as a NULL pointer.
> In modern compilers, one can write this as
> struct platform_device_info pinfo = {}, but Sakari's
> version looks best to me.

Ok, as long as there's no stale stack data, I'm ok with it.

thanks,

greg k-h