Message-ID: <c6b25a60-5c30-bd49-f4f2-0adb7b0e9061@linux.intel.com>
Date:   Thu, 19 Jan 2023 21:31:10 +0530
From:   "Kumar, M Chetan" <m.chetan.kumar@...ux.intel.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Netdev <netdev@...r.kernel.org>, kuba@...nel.org,
        davem@...emloft.net, johannes@...solutions.net,
        ryazanov.s.a@...il.com, loic.poulain@...aro.org,
        ricardo.martinez@...ux.intel.com,
        chiranjeevi.rapolu@...ux.intel.com, haijun.liu@...iatek.com,
        edumazet@...gle.com, pabeni@...hat.com,
        chandrashekar.devegowda@...el.com, linuxwwan@...el.com,
        linuxwwan_5g@...el.com,
        Mishra Soumya Prakash <soumya.prakash.mishra@...el.com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>
Subject: Re: [PATCH v4 net-next 4/5] net: wwan: t7xx: Enable devlink based fw
 flashing and coredump collection

On 1/19/2023 12:55 AM, Ilpo Järvinen wrote:
> On Wed, 18 Jan 2023, Kumar, M Chetan wrote:
> 
>> Hi Ilpo,
>> Thank you for the feedback.
>>
>> On 1/17/2023 7:37 PM, Ilpo Järvinen wrote:
>>> On Mon, 16 Jan 2023, m.chetan.kumar@...ux.intel.com wrote:
>>>
>>>> From: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
>>>>
>>>> Adds support for t7xx wwan device firmware flashing & coredump collection
>>>> using devlink.
>>>>
>>>> 1> Driver Registers with Devlink framework.
>>>> 2> Implements devlink ops flash_update callback that programs modem fw.
>>>> 3> 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 and then registers to devlink
>>>> framework.
>>>> On user space application issuing command for firmware update the driver
>>>> sends fastboot flash command & firmware to program NAND.
>>>>
>>>> In flashing procedure the fastboot command & response are 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>
>>>> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
>>>> --
> 
>>>> +	for (i = 0; i < total_part; i++) {
>>>
>>> The whole operation below is quite fancy, I'd add some comment telling the
>>> intent.
>>
>> Device returns firmware name & version in string format. Using below logic to
>> decode it.
>>
>> Will add some comment.
>>
>>>
>>>> +		part_name = strsep(&data, ",");
>>>> +		ver = strsep(&data, ",");
>>>
>>> Can ver become NULL here?
>>
>> It should not be the case. As part of component fw version query device is
>> expected to send the complete list for fw components.
>>
>> On safer note will add NULL check.
>>
>>>
>>>> +		ver_len = strlen(ver);
>>>> +		if (ver[ver_len - 2] == 0x5C && ver[ver_len - 1] == 0x6E)
>>>> +			ver[ver_len - 4] = '\0';
>>>
>>> Is ver_len guaranteed to be large enough?
>>
>> fw version query response message will not cross 512 bytes.
>> It is aligned with device implementation.
> 
> I meant the other way around, is ver_len guaranteed to large enough that
> it is safe to do ver_len - 4 (or even -1).

For negative cases, it is not guaranteed.
Will guard with below check.

if ((i == total_part - 1) && ver_len >= 4)


> 
>>>> +		ret = devlink_info_version_running_put_ext(req, part_name,
>>>> ver,
>>>> +
>>>> DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>> +	}
>>>> +
>>>> +err_clear_bit:
>>>> +	clear_bit(T7XX_GET_INFO, &dl->status);
>>>> +	kfree(data);
>>>> +	return ret;
>>>> +}
> 
>>>> +static void t7xx_devlink_uninit(struct t7xx_port *port)
>>>> +{
>>>> +	struct t7xx_devlink *dl = port->t7xx_dev->dl;
>>>> +	int i;
>>>> +
>>>> +	vfree(dl->regions[T7XX_MRDUMP_INDEX].buf);
>>>> +
>>>> +	dl->mode = T7XX_NORMAL_MODE;
>>>> +	destroy_workqueue(dl->wq);
>>>> +
>>>> +	BUILD_BUG_ON(ARRAY_SIZE(t7xx_devlink_region_infos) >
>>>> ARRAY_SIZE(dl->regions));
>>>
>>> The same BUILD_BUG_ON again? Maybe just make a single static_assert()
>>> outside of the functions.
>>
>> Should i change it as below ? please suggest.
>>
>> static_assert(ARRAY_SIZE(t7xx_devlink_region_infos) ==
>>                (sizeof(typeof_member(struct t7xx_devlink, regions)) /
>>                 sizeof(struct t7xx_devlink_region)));
>> static void t7xx_devlink_uninit(struct t7xx_port *port)
>> {
>> ..
> 
> I see, it's not that easy so perhaps just leave it as is.
> 
> I guess something like this might work but seems bit hacky to me
> (untested):
> 
> static_assert(ARRAY_SIZE(t7xx_devlink_region_infos) ==
> 	      ARRAY_SIZE(((struct t7xx_devlink *)NULL)->regions));
>

I tested your code, it works.
If you are OK, I will keep it.
  --
Chetan

Powered by blists - more mailing lists