| 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: <20231120171531.GA245676@kernel.org>
Date: Mon, 20 Nov 2023 17:15:31 +0000
From: Simon Horman <horms@...nel.org>
To: Michal Michalik <michal.michalik@...el.com>
Cc: netdev@...r.kernel.org, vadim.fedorenko@...ux.dev,
arkadiusz.kubalewski@...el.com, jonathan.lemon@...il.com,
pabeni@...hat.com, poros@...hat.com, milena.olech@...el.com,
mschmidt@...hat.com, linux-clk@...r.kernel.org, bvanassche@....org,
kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com
Subject: Re: [PATCH RFC net-next v3 1/2] netdevsim: implement DPLL for
subsystem selftests
On Fri, Nov 17, 2023 at 08:05:04PM +0100, Michal Michalik wrote:
> DPLL subsystem integration tests require a module which mimics the
> behavior of real driver which supports DPLL hardware. To fully test the
> subsystem the netdevsim is amended with DPLL implementation.
>
> Signed-off-by: Michal Michalik <michal.michalik@...el.com>
Hi Michal,
Nice to see tests being added for DPLL.
some minor feedback from my side.
> diff --git a/drivers/net/netdevsim/dpll.c b/drivers/net/netdevsim/dpll.c
...
> +static void nsim_fill_pin_pd(struct nsim_pin_priv_data *pd, u64 frequency, u32 prio,
> + enum dpll_pin_direction direction)
nit: Please consider limiting Networking code to 80 columns wide.
...
> +int nsim_dpll_init_owner(struct nsim_dpll *dpll, int devid,
> + unsigned int ports_count)
> +{
> + u64 clock_id;
> + int err;
> +
> + get_random_bytes(&clock_id, sizeof(clock_id));
> +
> + /* Create EEC DPLL */
> + dpll->dpll_e = dpll_device_get(clock_id, EEC_DPLL_DEV, THIS_MODULE);
> + if (IS_ERR(dpll->dpll_e))
> + goto dpll_e;
Branching to dpll_e will cause the function to return err,
but err is uninitialised here.
As there is nothing to unwind here I would lean towards simply
returning a negative error value directly here. But if not,
I'd suggest setting err to a negative error value inside the
if condition.
Flagged by clang-16 W=1 build, and Smatch.
> +
> + dpll->dpll_e_pd.temperature = EEC_DPLL_TEMPERATURE;
> + dpll->dpll_e_pd.mode = DPLL_MODE_AUTOMATIC;
> + dpll->dpll_e_pd.clock_id = clock_id;
> + dpll->dpll_e_pd.status = DPLL_LOCK_STATUS_UNLOCKED;
> +
> + err = dpll_device_register(dpll->dpll_e, DPLL_TYPE_EEC, &nsim_dds_ops,
> + &dpll->dpll_e_pd);
> + if (err)
> + goto e_reg;
> +
> + /* Create PPS DPLL */
> + dpll->dpll_p = dpll_device_get(clock_id, PPS_DPLL_DEV, THIS_MODULE);
> + if (IS_ERR(dpll->dpll_p))
> + goto dpll_p;
> +
> + dpll->dpll_p_pd.temperature = PPS_DPLL_TEMPERATURE;
> + dpll->dpll_p_pd.mode = DPLL_MODE_MANUAL;
> + dpll->dpll_p_pd.clock_id = clock_id;
> + dpll->dpll_p_pd.status = DPLL_LOCK_STATUS_UNLOCKED;
> +
> + err = dpll_device_register(dpll->dpll_p, DPLL_TYPE_PPS, &nsim_dds_ops,
> + &dpll->dpll_p_pd);
> + if (err)
> + goto p_reg;
> +
> + /* Create first pin (GNSS) */
> + err = nsim_fill_pin_properties(&dpll->pp_gnss, "GNSS",
> + DPLL_PIN_TYPE_GNSS,
> + PIN_GNSS_CAPABILITIES, 1,
> + DPLL_PIN_FREQUENCY_1_HZ,
> + DPLL_PIN_FREQUENCY_1_HZ);
> + if (err)
> + goto pp_gnss;
> + dpll->p_gnss =
> + dpll_pin_get(clock_id, PIN_GNSS, THIS_MODULE, &dpll->pp_gnss);
> + if (IS_ERR(dpll->p_gnss))
> + goto p_gnss;
> + nsim_fill_pin_pd(&dpll->p_gnss_pd, DPLL_PIN_FREQUENCY_1_HZ,
> + PIN_GNSS_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
> + err = dpll_pin_register(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
> + &dpll->p_gnss_pd);
> + if (err)
> + goto e_gnss_reg;
> +
> + /* Create second pin (PPS) */
> + err = nsim_fill_pin_properties(&dpll->pp_pps, "PPS", DPLL_PIN_TYPE_EXT,
> + PIN_PPS_CAPABILITIES, 1,
> + DPLL_PIN_FREQUENCY_1_HZ,
> + DPLL_PIN_FREQUENCY_1_HZ);
> + if (err)
> + goto pp_pps;
> + dpll->p_pps =
> + dpll_pin_get(clock_id, PIN_PPS, THIS_MODULE, &dpll->pp_pps);
> + if (IS_ERR(dpll->p_pps))
> + goto p_pps;
This branch will cause the function to return err.
However, err is set to 0 here. Perhaps it should be set
to a negative error value instead?
Flagged by Smatch.
> + nsim_fill_pin_pd(&dpll->p_pps_pd, DPLL_PIN_FREQUENCY_1_HZ,
> + PIN_PPS_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
> + err = dpll_pin_register(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
> + &dpll->p_pps_pd);
> + if (err)
> + goto e_pps_reg;
> + err = dpll_pin_register(dpll->dpll_p, dpll->p_pps, &nsim_pin_ops,
> + &dpll->p_pps_pd);
> + if (err)
> + goto p_pps_reg;
> +
> + dpll->pp_rclk =
> + kcalloc(ports_count, sizeof(*dpll->pp_rclk), GFP_KERNEL);
> + dpll->p_rclk = kcalloc(ports_count, sizeof(*dpll->p_rclk), GFP_KERNEL);
> + dpll->p_rclk_pd =
> + kcalloc(ports_count, sizeof(*dpll->p_rclk_pd), GFP_KERNEL);
> +
> + return 0;
> +
> +p_pps_reg:
> + dpll_pin_unregister(dpll->dpll_e, dpll->p_pps, &nsim_pin_ops,
> + &dpll->p_pps_pd);
> +e_pps_reg:
> + dpll_pin_put(dpll->p_pps);
> +p_pps:
> + nsim_free_pin_properties(&dpll->pp_pps);
> +pp_pps:
> + dpll_pin_unregister(dpll->dpll_e, dpll->p_gnss, &nsim_pin_ops,
> + &dpll->p_gnss_pd);
> +e_gnss_reg:
> + dpll_pin_put(dpll->p_gnss);
> +p_gnss:
> + nsim_free_pin_properties(&dpll->pp_gnss);
> +pp_gnss:
> + dpll_device_unregister(dpll->dpll_p, &nsim_dds_ops, &dpll->dpll_p_pd);
> +p_reg:
> + dpll_device_put(dpll->dpll_p);
> +dpll_p:
> + dpll_device_unregister(dpll->dpll_e, &nsim_dds_ops, &dpll->dpll_e_pd);
> +e_reg:
> + dpll_device_put(dpll->dpll_e);
> +dpll_e:
> + return err;
> +}
...
> +int nsim_rclk_init(struct netdevsim *ns)
> +{
> + struct nsim_dpll *dpll;
> + unsigned int index;
> + int err, devid;
> + char *name;
> +
> + devid = ns->nsim_dev->nsim_bus_dev->dev.id;
devid is set but otherwise unused in this function.
Flagged gcc-14 and clang-16 W=1 builds.
> + index = ns->nsim_dev_port->port_index;
> + dpll = &ns->nsim_dev->dpll;
> + err = -ENOMEM;
> +
> + name = kasprintf(GFP_KERNEL, "RCLK_%i", index);
> + if (!name)
> + goto err;
> +
> + /* Get EEC DPLL */
> + if (IS_ERR(dpll->dpll_e))
> + goto dpll;
> +
> + /* Get PPS DPLL */
> + if (IS_ERR(dpll->dpll_p))
> + goto dpll;
> +
> + /* Create Recovered clock pin (RCLK) */
> + nsim_fill_pin_properties(&dpll->pp_rclk[index], name,
> + DPLL_PIN_TYPE_SYNCE_ETH_PORT,
> + PIN_RCLK_CAPABILITIES, 1, 1e6, 125e6);
> + dpll->p_rclk[index] = dpll_pin_get(dpll->dpll_e_pd.clock_id,
> + PIN_RCLK + index, THIS_MODULE,
> + &dpll->pp_rclk[index]);
> + kfree(name);
> + if (IS_ERR(dpll->p_rclk[index]))
> + goto p_rclk;
This branch will call kfree(name), but this has already been done above.
Likewise for the other goto branches below.
Flagged by Smatch.
> + nsim_fill_pin_pd(&dpll->p_rclk_pd[index], DPLL_PIN_FREQUENCY_10_MHZ,
> + PIN_RCLK_PRIORITY, DPLL_PIN_DIRECTION_INPUT);
> + err = dpll_pin_register(dpll->dpll_e, dpll->p_rclk[index],
> + &nsim_pin_ops, &dpll->p_rclk_pd[index]);
> + if (err)
> + goto dpll_e_reg;
> + err = dpll_pin_register(dpll->dpll_p, dpll->p_rclk[index],
> + &nsim_pin_ops, &dpll->p_rclk_pd[index]);
> + if (err)
> + goto dpll_p_reg;
> +
> + netdev_dpll_pin_set(ns->netdev, dpll->p_rclk[index]);
> +
> + return 0;
> +
> +dpll_p_reg:
> + dpll_pin_unregister(dpll->dpll_e, dpll->p_rclk[index], &nsim_pin_ops,
> + &dpll->p_rclk_pd[index]);
> +dpll_e_reg:
> + dpll_pin_put(dpll->p_rclk[index]);
> +p_rclk:
> + nsim_free_pin_properties(&dpll->pp_rclk[index]);
> +dpll:
> + kfree(name);
> +err:
> + return err;
> +}
...
Powered by blists - more mailing lists