| 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: <AM6PR05MB522428D3B12B3AB5651359EAA2900@AM6PR05MB5224.eurprd05.prod.outlook.com>
Date: Wed, 30 Jan 2019 06:24:13 +0000
From: Vadim Pasternak <vadimp@...lanox.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
Liming Sun <lsun@...lanox.com>
CC: Andy Shevchenko <andy@...radead.org>,
Darren Hart <dvhart@...radead.org>,
David Woods <dwoods@...lanox.com>,
Platform Driver <platform-driver-x86@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 1/1] platform/mellanox: Add bootctl driver for Mellanox
BlueField Soc
[...]
Please, be consistent with naming convention.
All the above should have same prefix as others routines.
>
> > +static ssize_t post_reset_wdog_store(struct device_driver *drv,
> > + const char *buf, size_t count) {
> > + int err;
> > + unsigned long watchdog;
> > +
> > + err = kstrtoul(buf, 10, &watchdog);
> > + if (err)
> > + return err;
> > +
>
> > + if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_POST_RESET_WDOG,
> > + watchdog) < 0)
> > + return -EINVAL;
>
> If that call returns an error it shouldn't be shadowed here.
>
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t reset_action_show(struct device_driver *drv, char
> > +*buf) {
>
> > + return sprintf(buf, "%s\n", reset_action_to_string(
> > +
> > + mlxbf_bootctl_smc_call0(MLXBF_BOOTCTL_GET_RESET_ACTION)));
>
> Wouldn't be easy to parse this as
>
> int action = ...call0();
> return sprintf(...);
>
> ?
>
> (int is an arbitrary type here, choose one that suits)
>
> > +}
> > +
> > +static ssize_t reset_action_store(struct device_driver *drv,
> > + const char *buf, size_t count) {
> > + int action = reset_action_to_val(buf, count);
> > +
>
> > + if (action < 0 || action == MLXBF_BOOTCTL_NONE)
> > + return -EINVAL;
>
> Don't shadow an error.
>
> > +
> > + if (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_RESET_ACTION,
> action) < 0)
> > + return -EINVAL;
>
> Same.
>
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t second_reset_action_show(struct device_driver *drv,
> > +char *buf) {
>
> > + return sprintf(buf, "%s\n", reset_action_to_string(
> > + mlxbf_bootctl_smc_call0(
> > + MLXBF_BOOTCTL_GET_SECOND_RESET_ACTION)));
>
> Use temp variable.
>
> > +}
> > +
> > +static ssize_t second_reset_action_store(struct device_driver *drv,
> > + const char *buf, size_t
> > +count) {
> > + int action = reset_action_to_val(buf, count);
> > +
> > + if (action < 0)
> > + return -EINVAL;
>
> Don't shadow an error.
>
> > +
> > + if
> (mlxbf_bootctl_smc_call1(MLXBF_BOOTCTL_SET_SECOND_RESET_ACTION,
> > + action) < 0)
> > + return -EINVAL;
>
> Same.
>
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t lifecycle_state_show(struct device_driver *drv, char
> > +*buf) {
>
> > + int lc_state = mlxbf_bootctl_smc_call1(
> > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> > + MLXBF_BOOTCTL_FUSE_STATUS_LIFECYCLE);
>
> Split it as
>
> int ...;
>
> ... = call1();
> if (...)
>
> > +
> > + if (lc_state < 0)
> > + return -EINVAL;
>
> Don't shadow an error.
>
> > +
> > + lc_state &= (MLXBF_BOOTCTL_SB_MODE_TEST_MASK |
> > + MLXBF_BOOTCTL_SB_MODE_SECURE_MASK);
>
> Better to split like
>
> xxx =
> (A | B);
>
> > + /*
> > + * If the test bits are set, we specify that the current state may be
> > + * due to using the test bits.
> > + */
>
> > + if ((lc_state & MLXBF_BOOTCTL_SB_MODE_TEST_MASK) != 0) {
>
> ' != 0' is redundant.
>
> > +
> > + lc_state &= MLXBF_BOOTCTL_SB_MODE_SECURE_MASK;
> > +
>
> > + return sprintf(buf, "%s(test)\n",
> > +
> > + mlxbf_bootctl_lifecycle_states[lc_state]);
>
> One line?
>
> > + }
> > +
> > + return sprintf(buf, "%s\n",
> > +mlxbf_bootctl_lifecycle_states[lc_state]);
> > +}
> > +
> > +static ssize_t secure_boot_fuse_state_show(struct device_driver *drv,
> > +char *buf) {
> > + int key;
> > + int buf_len = 0;
> > + int upper_key_used = 0;
> > + int sb_key_state = mlxbf_bootctl_smc_call1(
> > + MLXBF_BOOTCTL_GET_TBB_FUSE_STATUS,
> > + MLXBF_BOOTCTL_FUSE_STATUS_KEYS);
> > +
> > + if (sb_key_state < 0)
> > + return -EINVAL;
> > +
>
> > + for (key = MLXBF_SB_KEY_NUM - 1; key >= 0; key--) {
>
> I'm not sure it's a good idea to put several lines in one sysfs attribute.
>
> > + int burnt = ((sb_key_state & (1 << key)) != 0);
>
> Redundant ' != 0', redundant parens.
>
> > + int valid = ((sb_key_state &
> > + (1 << (key + MLXBF_SB_KEY_NUM))) != 0);
>
> Same.
>
> > +
> > + buf_len += sprintf(buf + buf_len, "Ver%d:", key);
> > + if (upper_key_used) {
> > + if (burnt) {
> > + if (valid)
> > + buf_len += sprintf(buf + buf_len,
> > + "Used");
>
> Oh, why not just
>
> const char *status;
>
> if (...) {
> ...
> status = "1";
> ...
> status = "2";
> ...
> }
> len = snprintf(buf + len, ..., status);
>
> ?
>
> > + else
> > + buf_len += sprintf(buf + buf_len,
> > + "Wasted");
> > + } else {
> > + if (valid)
> > + buf_len += sprintf(buf + buf_len,
> > + "Invalid");
> > + else
> > + buf_len += sprintf(buf + buf_len,
> > + "Skipped");
> > + }
> > + } else {
> > + if (burnt) {
> > + if (valid) {
> > + upper_key_used = 1;
> > + buf_len += sprintf(buf + buf_len,
> > + "In use");
> > + } else
> > + buf_len += sprintf(buf + buf_len,
> > + "Burn incomplete");
> > + } else {
> > + if (valid)
> > + buf_len += sprintf(buf + buf_len,
> > + "Invalid");
> > + else
> > + buf_len += sprintf(buf + buf_len,
> > + "Free");
> > + }
> > + }
> > + buf_len += sprintf(buf + buf_len, "\n");
> > + }
> > +
> > + return buf_len;
> > +}
> > +
>
> > +#define MLXBF_BOOTCTL_DRV_ATTR(_name) DRIVER_ATTR_RW(_name)
>
> What the point?
>
> > +static struct attribute_group mlxbf_bootctl_attr_group = {
> > + .attrs = mlxbf_bootctl_dev_attrs
>
> + comma.
>
[...]
Powered by blists - more mailing lists