Message-ID: <CAHNKnsRY2cRS8LggQbpFaPGoOT_hSZSecT8QtKxW=D7Gq7Ug+A@mail.gmail.com>
Date:   Wed, 7 Sep 2022 02:24:59 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...emloft.net>,
        Johannes Berg <johannes@...solutions.net>,
        Loic Poulain <loic.poulain@...aro.org>,
        "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 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.

-- 
Sergey

Powered by blists - more mailing lists