Received-SPF: pass (google.com: domain of linux-kernel+bounces-61337-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 12 Feb 2024 11:41:18 +0100
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	linux-kernel@vger.kernel.org, Hans de Goede <hdegoede@redhat.com>,
	Tomas Winkler <tomas.winkler@intel.com>,
	Wentong Wu <wentong.wu@intel.com>
Subject: Re: [PATCH 3/3] mei: vsc: Assign pinfo fields in variable declaration
Message-ID: <2024021240-encode-disjoin-c8c2@gregkh>
References: <20240212094618.344921-1-sakari.ailus@linux.intel.com>
 <20240212094618.344921-4-sakari.ailus@linux.intel.com>
 <2024021210-freeway-unblessed-d966@gregkh>
 <d14e8a0c-f510-4c98-9bb6-1728eceb61aa@app.fastmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d14e8a0c-f510-4c98-9bb6-1728eceb61aa@app.fastmail.com>

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@linux.intel.com>
> >> ---
> >>  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