Date:   Thu, 8 Sep 2022 21:18:56 +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/8/2022 8:38 PM, Kumar, M Chetan wrote:
> 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.

Also, during 4G driver (IOSM) submission reviewer[1] suggested us
to consider using devlink region and health for coredump collection.
This is also another reason why we choose devlink region interface for 
dump collection.

The same is followed for 5G driver.

[1]
Andrew LunnJan. 7, 2021, 7:35 p.m. UTC | #1
On Thu, Jan 07, 2021 at 10:35:12PM +0530, M Chetan Kumar wrote:
 > Implements a char device for flashing Modem FW image while Device
 > is in boot rom phase and for collecting traces on modem crash.

Since this is a network device, you might want to take a look at
devlink support for flashing devices.

https://www.kernel.org/doc/html/latest/networking/devlink/devlink-flash.html

And for collecting crashes and other health information, consider
devlink region and devlink health.

It is much better to reuse existing infrastructure than do something
proprietary with a char dev.

Andrew


-- 
Chetan

Powered by blists - more mailing lists