mbox series

[0/3] MEI VSC fixes and cleanups

Message ID 20240212094618.344921-1-sakari.ailus@linux.intel.com
Headers
Series MEI VSC fixes and cleanups |

Message

Sakari Ailus Feb. 12, 2024, 9:46 a.m. UTC 
  Hi folks,

These patches address sleeping in atomic context. It wasn't obvious at
first this was taking place since the callback sleeps while the caller (a
different driver) called it in a threaded IRQ handler.

Sakari Ailus (3):
  mei: vsc: Call wake_up() and event handler in a workqueue
  mei: vsc: Don't use sleeping condition in wait_event_timeout()
  mei: vsc: Assign pinfo fields in variable declaration

 drivers/misc/mei/vsc-tp.c | 64 ++++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 24 deletions(-)

Comments

Greg KH Feb. 12, 2024, 10:02 a.m. UTC | #1 
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.

thanks,

greg k-h
Arnd Bergmann Feb. 12, 2024, 10:14 a.m. UTC | #2 
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.

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.

     Arnd
Greg KH Feb. 12, 2024, 10:41 a.m. UTC | #3 
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