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:

quoted

Hi Ilpo,
Thank you for the feedback.

On 1/17/2023 7:37 PM, Ilpo Järvinen wrote:

quoted

On Mon, 16 Jan 2023, m.chetan.kumar@linux.intel.com wrote:

quoted

From: M Chetan Kumar <redacted>

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 <redacted>
Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
Signed-off-by: Mishra Soumya Prakash <redacted>
Reviewed-by: Jesse Brandeburg <redacted>
--

quoted

quoted

quoted

+	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.

quoted

quoted

+		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.

quoted

quoted

+		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)

quoted

quoted

quoted

+		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;
+}

quoted

quoted

quoted

+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