| 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
| ||
|
Date: Thu, 17 Sep 2020 12:50:27 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Shannon Nelson <snelson@...sando.io>, netdev@...r.kernel.org,
davem@...emloft.net
Subject: Re: [PATCH v4 net-next 1/5] devlink: add timeout information to
status_notify
On 9/16/2020 8:02 PM, Shannon Nelson wrote:
> Add a timeout element to the DEVLINK_CMD_FLASH_UPDATE_STATUS
> netlink message for use by a userland utility to show that
> a particular firmware flash activity may take a long but
> bounded time to finish. Also add a handy helper for drivers
> to make use of the new timeout value.
>
> UI usage hints:
> - if non-zero, add timeout display to the end of the status line
> [component] status_msg ( Xm Ys : Am Bs )
> using the timeout value for Am Bs and updating the Xm Ys
> every second
> - if the timeout expires while awaiting the next update,
> display something like
> [component] status_msg ( timeout reached : Am Bs )
> - if new status notify messages are received, remove
> the timeout and start over
>
> Signed-off-by: Shannon Nelson <snelson@...sando.io>
> ---
This one looks good to me. I think the only other things that I saw from
previous discussion are:
a) we could convert the internal helper devlink_nl_flash_update_fill and
__devlink_flash_update_notify to use structs for their fields, and..
b) Jakub pointed out that most drivers don't currently use the component
field so we could remove that from the helpers.
However, I don't have strong feelings about those either way, so:
Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> include/net/devlink.h | 4 ++++
> include/uapi/linux/devlink.h | 3 +++
> net/core/devlink.c | 29 +++++++++++++++++++++++------
> 3 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index eaec0a8cc5ef..f206accf80ad 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -1400,6 +1400,10 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
> const char *component,
> unsigned long done,
> unsigned long total);
> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> + const char *status_msg,
> + const char *component,
> + unsigned long timeout);
>
> int devlink_traps_register(struct devlink *devlink,
> const struct devlink_trap *traps,
> diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
> index 40d35145c879..4a6e213cfa04 100644
> --- a/include/uapi/linux/devlink.h
> +++ b/include/uapi/linux/devlink.h
> @@ -460,6 +460,9 @@ enum devlink_attr {
>
> DEVLINK_ATTR_PORT_EXTERNAL, /* u8 */
> DEVLINK_ATTR_PORT_CONTROLLER_NUMBER, /* u32 */
> +
> + DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT, /* u64 */
> +
> /* add new attributes above here, update the policy in devlink.c */
>
> __DEVLINK_ATTR_MAX,
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 19037f114307..01f855e53e06 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -3024,7 +3024,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
> enum devlink_command cmd,
> const char *status_msg,
> const char *component,
> - unsigned long done, unsigned long total)
> + unsigned long done,
> + unsigned long total,
> + unsigned long timeout)
Number of paramters here is getting rather large. Does it make sense to
convert this one to a struct now?
> {
> void *hdr;
>
> @@ -3052,6 +3054,9 @@ static int devlink_nl_flash_update_fill(struct sk_buff *msg,
> if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TOTAL,
> total, DEVLINK_ATTR_PAD))
> goto nla_put_failure;
> + if (nla_put_u64_64bit(msg, DEVLINK_ATTR_FLASH_UPDATE_STATUS_TIMEOUT,
> + timeout, DEVLINK_ATTR_PAD))
> + goto nla_put_failure;
>
> out:
> genlmsg_end(msg, hdr);
> @@ -3067,7 +3072,8 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
> const char *status_msg,
> const char *component,
> unsigned long done,
> - unsigned long total)
> + unsigned long total,
> + unsigned long timeout)
> {
> struct sk_buff *msg;
> int err;
> @@ -3081,7 +3087,7 @@ static void __devlink_flash_update_notify(struct devlink *devlink,
> return;
>
> err = devlink_nl_flash_update_fill(msg, devlink, cmd, status_msg,
> - component, done, total);
> + component, done, total, timeout);
> if (err)
> goto out_free_msg;
>
> @@ -3097,7 +3103,7 @@ void devlink_flash_update_begin_notify(struct devlink *devlink)
> {
> __devlink_flash_update_notify(devlink,
> DEVLINK_CMD_FLASH_UPDATE,
> - NULL, NULL, 0, 0);
> + NULL, NULL, 0, 0, 0);
> }
> EXPORT_SYMBOL_GPL(devlink_flash_update_begin_notify);
>
> @@ -3105,7 +3111,7 @@ void devlink_flash_update_end_notify(struct devlink *devlink)
> {
> __devlink_flash_update_notify(devlink,
> DEVLINK_CMD_FLASH_UPDATE_END,
> - NULL, NULL, 0, 0);
> + NULL, NULL, 0, 0, 0);
> }
> EXPORT_SYMBOL_GPL(devlink_flash_update_end_notify);
>
> @@ -3117,10 +3123,21 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
> {
> __devlink_flash_update_notify(devlink,
> DEVLINK_CMD_FLASH_UPDATE_STATUS,
> - status_msg, component, done, total);
> + status_msg, component, done, total, 0);
> }
> EXPORT_SYMBOL_GPL(devlink_flash_update_status_notify);
>
> +void devlink_flash_update_timeout_notify(struct devlink *devlink,
> + const char *status_msg,
> + const char *component,
> + unsigned long timeout)
So we dropped the done and total here since in most cases we expect not
to need both a timeout and a done/total. I think that make sense. Most
of the time I think a command will benefit from one or the other.
> +{
> + __devlink_flash_update_notify(devlink,
> + DEVLINK_CMD_FLASH_UPDATE_STATUS,
> + status_msg, component, 0, 0, timeout);
> +}
> +EXPORT_SYMBOL_GPL(devlink_flash_update_timeout_notify);
> +
> static int devlink_nl_cmd_flash_update(struct sk_buff *skb,
> struct genl_info *info)
> {
>
Powered by blists - more mailing lists