Message-ID: <CAMZdPi_xNyt9JAihaRSPUgUy4h43oMxw-JjA_K3VVMmEuS55Og@mail.gmail.com>
Date:   Wed, 7 Sep 2022 17:23:28 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc:     "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>,
        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 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"...

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

Regards,
Loic

Powered by blists - more mailing lists