netdev - Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <MEYP282MB2697C334D0476215C8167B13BB13A@MEYP282MB2697.AUSP282.PROD.OUTLOOK.COM>
Date: Thu, 10 Aug 2023 23:32:52 +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,
	vsankar@...ovo.com
Subject: Re: [net-next 3/6] net: wwan: t7xx: Implements devlink ops of firmware flashing

>Mon, Aug 07, 2023 at 02:26:53PM CEST, songjinjian@...mail.com wrote:
>>Thansk for your review.
>>Witch command: echo 1 > /sys/bus/pci/devices/${bdf}/remove, then driver will run the
>>.remove ops, during this steps, driver get the fastboot param then send command to 
>>device let device reset to fastboot download mode.
>
>Ugh.
>
>
>>
>>>
>>>
>>>>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?
>>
>>Thansk for your review.
>>With the command: echo 1 > /sys/bus/pci/rescan, then driver will run the .probe options
>>then driver will follow the fastboot download process to export the download ports.
>
>That is certainly incorrect. No configuration or operation with the
>device instance should require to unbind&bind the device on the bus.

Thanks for your review, this remove rescan logic impliment in V5 patch driver version, as it can't be allowned 
in driver so I move it out driver patch, just now it seemed no other suitable way to reprobe device and identify 
device in download mode after device reset.
By the way,this remove rescan logic works with iosm driver also, so if there is no idea of other way, I hope it
can be allowned without effect the kernel. 

>>
>>>
>>>>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;
>>>>>+	}
>>>>>+}
>>>
>>>>>>+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.
>>
>>Thanks for your review, I will delete the info_get_loopback function.
>>
>>>
>>>
>>>>
>>>>>>+						   "1.0", DEVLINK_INFO_VERSION_TYPE_COMPONENT);
>>>>>>+
>>>>>>+	return ret;
>>>>>> }
>>>>>> 
>>
>