Date: Sat,  5 Aug 2023 20:05:35 +0800
From: Jinjian Song <songjinjian@...mail.com>
To: jiri@...nulli.us
Cc: chandrashekar.devegowda@...el.com,
	chiranjeevi.rapolu@...ux.intel.com,
	danielwinkler@...gle.com,
	davem@...emloft.net,
	edumazet@...gle.com,
	haijun.liu@...iatek.com,
	ilpo.jarvinen@...ux.intel.com,
	jesse.brandeburg@...el.com,
	jinjian.song@...ocom.com,
	johannes@...solutions.net,
	kuba@...nel.org,
	linuxwwan@...el.com,
	linuxwwan_5g@...el.com,
	loic.poulain@...aro.org,
	m.chetan.kumar@...ux.intel.com,
	netdev@...r.kernel.org,
	pabeni@...hat.com,
	ricardo.martinez@...ux.intel.com,
	ryazanov.s.a@...il.com,
	songjinjian@...mail.com,
	soumya.prakash.mishra@...el.com
Subject: Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing

Thu, Aug 03, 2023 at 04:18:09AM CEST, songjinjian@...mail.com wrote:
>>From: Jinjian Song <jinjian.song@...ocom.com>
>>
>>Adds support for t7xx wwan device firmware flashing using devlink.
>>
>>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.
>>
>>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.
>>
>>Base on the v5 patch version of follow series:
>>'net: wwan: t7xx: fw flashing & coredump support'
>>(https://patchwork.kernel.org/project/netdevbpf/patch/fc8bbb0b66a5ff3a489ea9857d79b374508090ef.1674307425.git.m.chetan.kumar@linux.intel.com/)
>>
>>Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>
>
>Overall, this patch/patchset is very ugly and does some wrong or
>questionable things that make my head hurt. Ugh.

Thanks for your review, this is my first time to do this and when I git send-email,
my email account locked so the patchset break at 3/6 and I resend the remaining 3 patches again.

I will reorganize and prepare v2 version, sorry for that.

>> #include "t7xx_port_devlink.h"
>> 
>>+static int t7xx_devlink_port_read(struct t7xx_port *port, char *buf, size_t count)
>
>You have "devlink" in lot of the function and struct field names. Does
>not make sense to me as for example this function does not have anything
>to do with devlink. Could you please rename them all?

Thanks for your review, I think I can rename them all to flash_dump port read, this functions used
to send or recieve data(firmware/coredump/command) with modem

>>+	set_bit(T7XX_FLASH_STATUS, &dl->status);
>>+	port = dl->port;
>>+	dev_dbg(port->dev, "flash partition name:%s binary size:%zu\n", component, fw->size);
>>+	ret = t7xx_devlink_fb_flash_partition(port, component, fw->data, fw->size);
>>+
>>+	sprintf(flash_status, "%s %s", "flashing", !ret ? "success" : "failure");
>
>Don't put return status in to the flash_status. Function returns error
>value which is propagated to the user.
>
>In fact, in your case, usage of devlink_flash_update_status_notify()
>does not make much sense as you don't have multiple flash stages.

Thanks for your review, yes 'success' and 'failure', Function can returns error I will remove 
status notify and flash_status
 
>>+	devlink_flash_update_status_notify(devlink, flash_status, params->component, 0, 0);
>>+	clear_bit(T7XX_FLASH_STATUS, &dl->status);
>>+
>>+err_out:
>>+	return ret;
>> 	return 0;
>> }

>> static int t7xx_devlink_reload_up(struct devlink *devlink,
>>@@ -50,13 +266,114 @@ static int t7xx_devlink_reload_up(struct devlink *devlink,
>> 				  u32 *actions_performed,
>> 				  struct netlink_ext_ack *extack)
>> {
>>-	return 0;
>>+	*actions_performed = BIT(action);
>>+	switch (action) {
>>+	case DEVLINK_RELOAD_ACTION_DRIVER_REINIT:
>
>Your driver reinit does not do anything. Please remove it from supported
>actions.

I want to register reinit action and fastboot param with it, so it work like follow:
1.devlink param set fastboot 1 driver_reinit
2.devlink driver_reinit
3.use space command remove driver, then driver remove function get the fastboot param 
true, then send message to modem to let modem reboot to fastboot download mode.
4.use space command rescan device, driver probe and export flash port.
5.devlink flash firmware image.

if don't suggest use devlink param fastboot driver_reinit, I think set 
fastboot flag during this action, but Intel colleague Kumar have drop that way in the old 
v2 patch version.
https://patchwork.kernel.org/project/netdevbpf/patch/20230105154300.198873-1-m.chetan.kumar@linux.intel.com/ 

>>+	case DEVLINK_RELOAD_ACTION_FW_ACTIVATE:
>>+		return 0;
>>+	default:
>>+		/* Unsupported action should not get to this function */
>>+		return -EOPNOTSUPP;
>>+	}
>>+}

>>+struct devlink_info_req {
>>+	struct sk_buff *msg;
>>+	void (*version_cb)(const char *version_name,
>>+			   enum devlink_info_version_type version_type,
>>+			   void *version_cb_priv);
>>+	void *version_cb_priv;
>>+};
>
>Ah! No. Remove this. If you need to touch internal of the struct, this
>is definitelly not the way to do it.

Thanks for your review, got it.

>>+
>>+struct devlink_flash_component_lookup_ctx {
>>+	const char *lookup_name;
>>+	bool lookup_name_found;
>>+};
>
>Same here.

Thanks for your review, got it.

>>+
>>+static int t7xx_devlink_info_get_loopback(struct devlink *devlink, struct devlink_info_req *req,
>>+					  struct netlink_ext_ack *extack)
>>+{
>>+	struct devlink_flash_component_lookup_ctx *lookup_ctx = req->version_cb_priv;
>>+	int ret;
>>+
>>+	if (!req)
>>+		return t7xx_devlink_info_get(devlink, req, extack);
>>+
>>+	ret = devlink_info_version_running_put_ext(req, lookup_ctx->lookup_name,
>
>It actually took me a while why you are doing this. You try to overcome
>the limitation for drivers to expose version for all components that are
>valid for flashing. That is not nice
>
>Please don't do things like this!
>
>Expose the versions for all valid components, or don't flash them.

For the old modem firmware, it don't support the info_get function, so add the logic here to 
compatible with old modem firmware update during devlink flash.

>>+						   "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>+
>>+	return ret;
>> }
>> 

Powered by blists - more mailing lists