Re: [PATCH] hwmon: xgene: Fix crash when alarm occurs before driver probe

From: Hoan Tran
Date: Wed Sep 07 2016 - 14:55:13 EST

Next message: Kevin Hilman: "Re: [PATCH V3 0/5] firmware: Add support for TI System Control Interface (TI-SCI) protocol driver"
Previous message: Sinclair Yeh: "Re: [PATCH v2] usercopy: remove page-spanning test for now"
In reply to: Guenter Roeck: "Re: [PATCH] hwmon: xgene: Fix crash when alarm occurs before driver probe"
Next in thread: Guenter Roeck: "Re: [PATCH] hwmon: xgene: Fix crash when alarm occurs before driver probe"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Guenter,

On Tue, Sep 6, 2016 at 11:39 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 09/06/2016 11:07 PM, Hoan Tran wrote:
>>
>> Hi Guenter,
>>
>> On Tue, Sep 6, 2016 at 10:50 PM, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>>
>>> On 09/06/2016 10:21 PM, Hoan Tran wrote:
>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>> Thank for your quick review !
>>>>
>>>> On Tue, Sep 6, 2016 at 9:35 PM, Guenter Roeck <linux@xxxxxxxxxxxx>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 09/06/2016 08:46 PM, Hoan Tran wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> The system crashes during probing xgene-hwmon driver when temperature
>>>>>> alarm interrupt occurs before.
>>>>>> It's because
>>>>>> - xgene_hwmon_probe() requests PCC mailbox channel which also enables
>>>>>> the mailbox interrupt.
>>>>>> - As temperature alarm interrupt is pending, ISR runs and crashes
>>>>>> when
>>>>>> accesses
>>>>>> into invalid resource as unmapped PCC shared memory.
>>>>>>
>>>>>> This patch fixes this issue by saving this alarm message and
>>>>>> scheduling
>>>>>> a
>>>>>> bottom handler after xgene_hwmon_probe() finish.
>>>>>>
>>>>>
>>>>> I am not completely happy with this fix. Main problem I have is that
>>>>> the
>>>>> processing associated with resp_pending doesn't happen until init_flag
>>>>> is
>>>>> set.
>>>>> Since the hwmon functions can be called right after
>>>>> hwmon_device_register_with_groups(),
>>>>> there is now a new race condition between that call and setting
>>>>> init_flag.
>>>>
>>>>
>>>>
>>>> I think it's still good if hwmon functions are called right after
>>>> hwmon_device_register_with_groups().
>>>> The response message will be queued into FIFO and be processed later.
>>>>
>>> Yes, but the call to complete() won't happen in this case, or am I
>>> missing
>>> something ?
>>
>>
>> Yes, I think xgene_hwmon_rd() and xgene_hwmon_pcc_rd() functions have
>> to check "init_flag == true" before issue the read command.
>>
>
> This is getting more and more messy :-(
>
>> Thanks
>> Hoan
>>
>>>
>>> Guenter
>>>
>>>
>>>>>
>>>>> I am also a bit concerned with init_flag and rx_pending not being
>>>>> atomic
>>>>> and
>>>>> protected.
>>>>> What happens if a second interrupt occurs right after init_flag is set
>>>>> but
>>>>> before
>>>>> (or while) rx_pending is evaluated ?
>>>>
>>>>
>>>>
>>>> Yah, that's a good catch. I can re-use the "kfifo_lock" spinlock for
>>>> this atomic protection.
>>>>
>>>>>
>>>>> On top of that, init_flag and thus the added complexity is (unless I am
>>>>> missing
>>>>> something) only needed if acpi is enabled. Penaltizing non-acpi code
>>>>> doesn't
>>>>> seem
>>>>> to be optimal.
>>>>
>>>>
>>>>
>>>> I think, with DT, we still need this flag. In a case of temperature
>>>> alarm, the driver needs to set "temp1_critical_alarm" sysfs.
>>>> This "temp1_critical_alarm" should be created before "init_flag" = true.
>>>>
>
> I don't know. After all, any such alarm would be lost if it occurred
> before the driver was loaded, no ? Those mailboxes should really have
> a means to be informed that the initiator is ready to handle
> interrupts/callbacks.

We don't want to miss an alarm message. User should be warned.

As
ctx->hwmon_dev = hwmon_device_register_with_groups(ctx->dev,

"apm_xgene",
ctx,
xgene_hwmon_groups);
How about, callback functions check the ctx->hwmon_dev validation. If
not, they just save msg into FIFO.
Beside of that, as hwmon functions can be called before ctx->hwmon_dev
is assigned. Callback functions check if there is a mailbox response
pending before saving msg into FIFO as below

static int xgene_hwmon_rx_ready(struct xgene_hwmon_dev *ctx, void *msg)
{
if (IS_ERR(ctx->hwmon_dev) && !ctx->resp_pending) {
/* Enqueue to the FIFO */
kfifo_in_spinlocked(&ctx->async_msg_fifo, msg,
sizeof(struct slimpro_resp_msg),
&ctx->kfifo_lock);

return -EBUSY;
}

return 0;
}

At the end of probe function, driver always schedules a bottom handler
to handle FIFO msg.

Then we can remove the init_flag and rx_pending.

Thanks
Hoan

>
> Guenter
>
>
>>>> Thanks
>>>> Hoan
>>>>
>>>>>
>>>>> How do other drivers handle this situation ? This must be a common
>>>>> problem
>>>>> with all mbox users.
>>>>>
>>>>> Thanks,
>>>>> Guenter
>>>>>
>>>>>
>>>>>> Signed-off-by: Hoan Tran <hotran@xxxxxxx>
>>>>>> Reported-by: Itaru Kitayama <itaru.kitayama@xxxxxxxx>
>>>>>> ---
>>>>>> drivers/hwmon/xgene-hwmon.c | 75
>>>>>> +++++++++++++++++++++++++++++++++------------
>>>>>> 1 file changed, 56 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
>>>>>> index bc78a5d..e3b4e84 100644
>>>>>> --- a/drivers/hwmon/xgene-hwmon.c
>>>>>> +++ b/drivers/hwmon/xgene-hwmon.c
>>>>>> @@ -107,6 +107,8 @@ struct xgene_hwmon_dev {
>>>>>> struct completion rd_complete;
>>>>>> int resp_pending;
>>>>>> struct slimpro_resp_msg sync_msg;
>>>>>> + bool init_flag;
>>>>>> + bool rx_pending;
>>>>>>
>>>>>> struct work_struct workq;
>>>>>> struct kfifo_rec_ptr_1 async_msg_fifo;
>>>>>> @@ -465,13 +467,35 @@ static void xgene_hwmon_evt_work(struct
>>>>>> work_struct
>>>>>> *work)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +static int xgene_hwmon_rx_ready(struct xgene_hwmon_dev *ctx, void
>>>>>> *msg)
>>>>>> +{
>>>>>> + if (!ctx->init_flag) {
>>>>>> + ctx->rx_pending = true;
>>>>>> + /* Enqueue to the FIFO */
>>>>>> + kfifo_in_spinlocked(&ctx->async_msg_fifo, msg,
>>>>>> + sizeof(struct slimpro_resp_msg),
>>>>>> + &ctx->kfifo_lock);
>>>>>> + return -EBUSY;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> /*
>>>>>> * This function is called when the SLIMpro Mailbox received a
>>>>>> message
>>>>>> */
>>>>>> static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg)
>>>>>> {
>>>>>> struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl);
>>>>>> - struct slimpro_resp_msg amsg;
>>>>>> +
>>>>>> + /*
>>>>>> + * While the driver registers with the mailbox framework, an
>>>>>> interrupt
>>>>>> + * can be pending before the probe function completes its
>>>>>> + * initialization. If such condition occurs, just queue up the
>>>>>> message
>>>>>> + * as the driver is not ready for servicing the callback.
>>>>>> + */
>>>>>> + if (xgene_hwmon_rx_ready(ctx, msg) < 0)
>>>>>> + return;
>>>>>>
>>>>>> /*
>>>>>> * Response message format:
>>>>>> @@ -500,12 +524,8 @@ static void xgene_hwmon_rx_cb(struct mbox_client
>>>>>> *cl,
>>>>>> void *msg)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> - amsg.msg = ((u32 *)msg)[0];
>>>>>> - amsg.param1 = ((u32 *)msg)[1];
>>>>>> - amsg.param2 = ((u32 *)msg)[2];
>>>>>> -
>>>>>> /* Enqueue to the FIFO */
>>>>>> - kfifo_in_spinlocked(&ctx->async_msg_fifo, &amsg,
>>>>>> + kfifo_in_spinlocked(&ctx->async_msg_fifo, msg,
>>>>>> sizeof(struct slimpro_resp_msg),
>>>>>> &ctx->kfifo_lock);
>>>>>> /* Schedule the bottom handler */
>>>>>> schedule_work(&ctx->workq);
>>>>>> @@ -520,6 +540,15 @@ static void xgene_hwmon_pcc_rx_cb(struct
>>>>>> mbox_client
>>>>>> *cl, void *msg)
>>>>>> struct acpi_pcct_shared_memory *generic_comm_base =
>>>>>> ctx->pcc_comm_addr;
>>>>>> struct slimpro_resp_msg amsg;
>>>>>>
>>>>>> + /*
>>>>>> + * While the driver registers with the mailbox framework, an
>>>>>> interrupt
>>>>>> + * can be pending before the probe function completes its
>>>>>> + * initialization. If such condition occurs, just queue up the
>>>>>> message
>>>>>> + * as the driver is not ready for servicing the callback.
>>>>>> + */
>>>>>> + if (xgene_hwmon_rx_ready(ctx, &amsg) < 0)
>>>>>> + return;
>>>>>> +
>>>>>> msg = generic_comm_base + 1;
>>>>>> /* Check if platform sends interrupt */
>>>>>> if (!xgene_word_tst_and_clr(&generic_comm_base->status,
>>>>>> @@ -596,6 +625,17 @@ static int xgene_hwmon_probe(struct
>>>>>> platform_device
>>>>>> *pdev)
>>>>>> platform_set_drvdata(pdev, ctx);
>>>>>> cl = &ctx->mbox_client;
>>>>>>
>>>>>> + spin_lock_init(&ctx->kfifo_lock);
>>>>>> + mutex_init(&ctx->rd_mutex);
>>>>>> +
>>>>>> + rc = kfifo_alloc(&ctx->async_msg_fifo,
>>>>>> + sizeof(struct slimpro_resp_msg) *
>>>>>> ASYNC_MSG_FIFO_SIZE,
>>>>>> + GFP_KERNEL);
>>>>>> + if (rc)
>>>>>> + goto out_mbox_free;
>>>>>> +
>>>>>> + INIT_WORK(&ctx->workq, xgene_hwmon_evt_work);
>>>>>> +
>>>>>> /* Request mailbox channel */
>>>>>> cl->dev = &pdev->dev;
>>>>>> cl->tx_done = xgene_hwmon_tx_done;
>>>>>> @@ -676,17 +716,6 @@ static int xgene_hwmon_probe(struct
>>>>>> platform_device
>>>>>> *pdev)
>>>>>> ctx->usecs_lat = PCC_NUM_RETRIES * cppc_ss->latency;
>>>>>> }
>>>>>>
>>>>>> - spin_lock_init(&ctx->kfifo_lock);
>>>>>> - mutex_init(&ctx->rd_mutex);
>>>>>> -
>>>>>> - rc = kfifo_alloc(&ctx->async_msg_fifo,
>>>>>> - sizeof(struct slimpro_resp_msg) *
>>>>>> ASYNC_MSG_FIFO_SIZE,
>>>>>> - GFP_KERNEL);
>>>>>> - if (rc)
>>>>>> - goto out_mbox_free;
>>>>>> -
>>>>>> - INIT_WORK(&ctx->workq, xgene_hwmon_evt_work);
>>>>>> -
>>>>>> ctx->hwmon_dev = hwmon_device_register_with_groups(ctx->dev,
>>>>>>
>>>>>> "apm_xgene",
>>>>>> ctx,
>>>>>> @@ -697,17 +726,25 @@ static int xgene_hwmon_probe(struct
>>>>>> platform_device
>>>>>> *pdev)
>>>>>> goto out;
>>>>>> }
>>>>>>
>>>>>> + ctx->init_flag = true;
>>>>>> + if (ctx->rx_pending) {
>>>>>> + /*
>>>>>> + * If there is a pending message, schedule the bottom
>>>>>> handler
>>>>>> + */
>>>>>> + schedule_work(&ctx->workq);
>>>>>> + }
>>>>>> +
>>>>>> dev_info(&pdev->dev, "APM X-Gene SoC HW monitor driver
>>>>>> registered\n");
>>>>>>
>>>>>> return 0;
>>>>>>
>>>>>> out:
>>>>>> - kfifo_free(&ctx->async_msg_fifo);
>>>>>> -out_mbox_free:
>>>>>> if (acpi_disabled)
>>>>>> mbox_free_channel(ctx->mbox_chan);
>>>>>> else
>>>>>> pcc_mbox_free_channel(ctx->mbox_chan);
>>>>>> +out_mbox_free:
>>>>>> + kfifo_free(&ctx->async_msg_fifo);
>>>>>>
>>>>>> return rc;
>>>>>> }
>>>>>>
>>>>>
>>>>
>>>
>>
>

Next message: Kevin Hilman: "Re: [PATCH V3 0/5] firmware: Add support for TI System Control Interface (TI-SCI) protocol driver"
Previous message: Sinclair Yeh: "Re: [PATCH v2] usercopy: remove page-spanning test for now"
In reply to: Guenter Roeck: "Re: [PATCH] hwmon: xgene: Fix crash when alarm occurs before driver probe"
Next in thread: Guenter Roeck: "Re: [PATCH] hwmon: xgene: Fix crash when alarm occurs before driver probe"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]