Message-ID: <ZNCew+BPDITsZdY1@nanopsycho>
Date: Mon, 7 Aug 2023 09:35:31 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jinjian Song <songjinjian@...mail.com>
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, soumya.prakash.mishra@...el.com
Subject: Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of
 firmware flashing

Sat, Aug 05, 2023 at 02:05:35PM CEST, songjinjian@...mail.com wrote:
>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.

What do you mean by this? I don't follow. What's "command remove
driver"?


>4.use space command rescan device, driver probe and export flash port.

Again, what's "command rescan device" ?

Could you perhaps rather use command line examples?


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

No! Don't do this. I don't care about your firmware. We enforce info_get
and flash component parity, obey it. Either provide the version info for
all components you want to flash with proper versions, or don't
implement the flash.


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

Powered by blists - more mailing lists