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