Message-ID: <3f683628-6f4b-c678-9137-38bdbe4a68ea@linux.intel.com>
Date:   Thu, 8 Sep 2022 20:38:19 +0530
From:   "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
To:     Loic Poulain <loic.poulain@...aro.org>,
        Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        "Sudi, Krishna C" <krishna.c.sudi@...el.com>,
        Intel Corporation <linuxwwan@...el.com>,
        Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>,
        Mishra Soumya Prakash <soumya.prakash.mishra@...el.com>
Subject: Re: [PATCH net-next 4/5] net: wwan: t7xx: Enable devlink based fw
 flashing and coredump collection

On 9/7/2022 8:53 PM, Loic Poulain wrote:
> On Wed, 7 Sept 2022 at 01:25, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>>
>> On Sat, Sep 3, 2022 at 11:32 AM Kumar, M Chetan
>> <m.chetan.kumar@...ux.intel.com> wrote:
>>> On 8/30/2022 7:51 AM, Sergey Ryazanov wrote:
>>>> On Tue, Aug 16, 2022 at 7:12 AM <m.chetan.kumar@...el.com> wrote:
>>>>> From: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
>>>>>
>>>>> This patch brings-in support for t7xx wwan device firmware flashing &
>>>>> coredump collection using devlink.
>>>>>
>>>>> Driver Registers with Devlink framework.
>>>>> Implements devlink ops flash_update callback that programs modem firmware.
>>>>> Creates region & snapshot required for device coredump log collection.
>>>>> On early detection of wwan device in fastboot mode driver sets up CLDMA0 HW
>>>>> tx/rx queues for raw data transfer then registers with devlink framework.
>>>>> Upon receiving firmware image & partition details driver sends fastboot
>>>>> commands for flashing the firmware.
>>>>>
>>>>> In this flow the fastboot command & response gets exchanged between driver
>>>>> and device. Once firmware flashing is success completion status is reported
>>>>> to user space application.
>>>>>
>>>>> Below is the devlink command usage for firmware flashing
>>>>>
>>>>> $devlink dev flash pci/$BDF file ABC.img component ABC
>>>>>
>>>>> Note: ABC.img is the firmware to be programmed to "ABC" partition.
>>>>>
>>>>> In case of coredump collection when wwan device encounters an exception
>>>>> it reboots & stays in fastboot mode for coredump collection by host driver.
>>>>> On detecting exception state driver collects the core dump, creates the
>>>>> devlink region & reports an event to user space application for dump
>>>>> collection. The user space application invokes devlink region read command
>>>>> for dump collection.
>>>>>
>>>>> Below are the devlink commands used for coredump collection.
>>>>>
>>>>> devlink region new pci/$BDF/mr_dump
>>>>> devlink region read pci/$BDF/mr_dump snapshot $ID address $ADD length $LEN
>>>>> devlink region del pci/$BDF/mr_dump snapshot $ID
>>>>>
>>>>> Signed-off-by: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
>>>>> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>
>>>>> Signed-off-by: Mishra Soumya Prakash <soumya.prakash.mishra@...el.com>
>>>>
>>>> [skipped]
>>>>
>>>>> diff --git a/drivers/net/wwan/t7xx/t7xx_state_monitor.c b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>> index 9c222809371b..00e143c8d568 100644
>>>>> --- a/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>> +++ b/drivers/net/wwan/t7xx/t7xx_state_monitor.c
>>>>
>>>> [skipped]
>>>>
>>>>> @@ -239,8 +252,16 @@ static void t7xx_lk_stage_event_handling(struct t7xx_fsm_ctl *ctl, unsigned int
>>>>>                           return;
>>>>>                   }
>>>>>
>>>>> +               if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>>>> +                       port->dl->mode = T7XX_FB_DUMP_MODE;
>>>>> +               else
>>>>> +                       port->dl->mode = T7XX_FB_DL_MODE;
>>>>>                   port->port_conf->ops->enable_chl(port);
>>>>>                   t7xx_cldma_start(md_ctrl);
>>>>> +               if (lk_event == LK_EVENT_CREATE_PD_PORT)
>>>>> +                       t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DUMP_MODE);
>>>>> +               else
>>>>> +                       t7xx_uevent_send(dev, T7XX_UEVENT_MODEM_FASTBOOT_DL_MODE);
>>>>>                   break;
>>>>>
>>>>>           case LK_EVENT_RESET:
>>>>
>>>> [skipped]
>>>>
>>>>> @@ -318,6 +349,7 @@ static void fsm_routine_ready(struct t7xx_fsm_ctl *ctl)
>>>>>
>>>>>           ctl->curr_state = FSM_STATE_READY;
>>>>>           t7xx_fsm_broadcast_ready_state(ctl);
>>>>> +       t7xx_uevent_send(&md->t7xx_dev->pdev->dev, T7XX_UEVENT_MODEM_READY);
>>>>>           t7xx_md_event_notify(md, FSM_READY);
>>>>>    }
>>>>
>>>> These UEVENT things look at least unrelated to the patch. If the
>>>> deriver is really need it, please factor out it into a separate patch
>>>> with a comment describing why userspace wants to see these events.
>>>>
>>>> On the other hand, this looks like a custom tracing implementation. It
>>>> might be better to use simple debug messages instead or even the
>>>> tracing API, which is much more powerful than any uevent.
>>>
>>> Driver is reporting modem status (up, down, exception, etc) via uevent.
>>> The wwan user space services use these states for taking some action.
>>> So we have choose uevent for reporting modem status to user space.
>>>
>>> Is it ok we retain this logic ? I will drop it from this patch and send
>>> it as a separate patch for review.
>>
>> Usually some subsystem generates common events for served devices. And
>> it is quite unusual for drivers to generate custom uevents. I found
>> only a few examples of such drivers.
>>
>> I am not against the uevent usage, I just doubt that some userspace
>> software could benefit from custom driver uevents. If this case is
>> special, then please send these uevent changes as a separate patch
>> with a comment describing why userspace wants to see them.
> 
> Yes, would be good to avoid new custom uevents if possible. I'm not
> familiar with devlink but in the case of firmware flashing I assume
> the device state is fully managed internally by the driver, and the
> command terminates with success (or not), so we don't really need to
> report async events. For firmware state, maybe having a 'state' sysfs
> prop would be a good start (as for remoteproc), with generic state
> names like "running", "crashed"...

Driver needs a way to report custom events to user space.
If sysfs is a way to report custom events will change the
implementation to it.

new 'state' entry will be created under dev and modem status
will be written to it. So user space could get the modem status.

> 
> BTW, don't remember if we already mention/address this, but isn't the
> device coredump framework more appropriate for such dump?

Devlink region provides similar capability as dev coredump.
It allows the driver to collect the dump snapshot and can be
easily accessed via devlink region read or dump commands.

Since for firmware flashing we are using devlink interface we
also choose devlink region for dump collection.

I hope it is ok to continue with devlink region.

-- 
Chetan

Powered by blists - more mailing lists