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(-)
--
2.39.2
vsc_tp_wakeup_request() called wait_event_timeout() with
gpiod_get_value_cansleep() which may sleep, and does so as the
implementation is that of gpio-ljca.
Move the GPIO state check outside the call.
Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/misc/mei/vsc-tp.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 264ece72f0bf..200af14490d7 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -25,7 +25,8 @@
#define VSC_TP_ROM_BOOTUP_DELAY_MS 10
#define VSC_TP_ROM_XFER_POLL_TIMEOUT_US (500 * USEC_PER_MSEC)
#define VSC_TP_ROM_XFER_POLL_DELAY_US (20 * USEC_PER_MSEC)
-#define VSC_TP_WAIT_FW_ASSERTED_TIMEOUT (2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_TIMEOUT (2 * HZ)
+#define VSC_TP_WAIT_FW_POLL_DELAY_US (20 * USEC_PER_MSEC)
#define VSC_TP_MAX_XFER_COUNT 5
#define VSC_TP_PACKET_SYNC 0x31
@@ -103,13 +104,15 @@ static int vsc_tp_wakeup_request(struct vsc_tp *tp)
gpiod_set_value_cansleep(tp->wakeupfw, 0);
ret = wait_event_timeout(tp->xfer_wait,
- atomic_read(&tp->assert_cnt) &&
- gpiod_get_value_cansleep(tp->wakeuphost),
- VSC_TP_WAIT_FW_ASSERTED_TIMEOUT);
+ atomic_read(&tp->assert_cnt),
+ VSC_TP_WAIT_FW_POLL_TIMEOUT);
if (!ret)
return -ETIMEDOUT;
- return 0;
+ return read_poll_timeout(gpiod_get_value_cansleep, ret, ret,
+ VSC_TP_WAIT_FW_POLL_DELAY_US,
+ VSC_TP_WAIT_FW_POLL_TIMEOUT, false,
+ tp->wakeuphost);
}
static void vsc_tp_wakeup_release(struct vsc_tp *tp)
--
2.39.2
Assign all possible fields of pinfo in variable declaration, instead of
just zeroing it there.
Signed-off-by: Sakari Ailus <[email protected]>
---
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,
+ };
struct device *dev = &spi->dev;
struct platform_device *pdev;
struct acpi_device *adev;
- struct vsc_tp *tp;
int ret;
tp = devm_kzalloc(dev, sizeof(*tp), GFP_KERNEL);
@@ -508,13 +513,8 @@ static int vsc_tp_probe(struct spi_device *spi)
ret = -ENODEV;
goto err_destroy_lock;
}
- pinfo.fwnode = acpi_fwnode_handle(adev);
-
- pinfo.name = "intel_vsc";
- pinfo.data = &tp;
- pinfo.size_data = sizeof(tp);
- pinfo.id = PLATFORM_DEVID_NONE;
+ pinfo.fwnode = acpi_fwnode_handle(adev);
pdev = platform_device_register_full(&pinfo);
if (IS_ERR(pdev)) {
ret = PTR_ERR(pdev);
--
2.39.2
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> >> ---
> >> 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
Hi Greg,
Thanks for the review.
On Mon, Feb 12, 2024 at 11:02:04AM +0100, 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 <[email protected]>
> > ---
> > 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?
Hmm. I'm not quite sure what you mean.
The patch changes where the fields are assigned but the variable is only
used when the assignment is done in any case, so this doesn't change
anything.
>
> 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.
The only caveat in initialising a struct is that the possible holes due to
ABI aren't initialised, which generally is a concern with UAPI or network
(i.e. not here, and the patch doesn't change that anyway).
--
Regards,
Sakari Ailus