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

[Jiri Pirko <jiri@resnulli.us>]

Mon, Aug 07, 2023 at 02:26:53PM CEST, songjinjian@hotmail.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.


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