| 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: <CH3PR11MB8414A2B45B06EC5ACEB3A525E3BAA@CH3PR11MB8414.namprd11.prod.outlook.com>
Date: Wed, 22 Nov 2023 16:49:33 +0000
From: "Michalik, Michal" <michal.michalik@...el.com>
To: Simon Horman <horms@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"vadim.fedorenko@...ux.dev" <vadim.fedorenko@...ux.dev>, "Kubalewski,
Arkadiusz" <arkadiusz.kubalewski@...el.com>, "jonathan.lemon@...il.com"
<jonathan.lemon@...il.com>, "pabeni@...hat.com" <pabeni@...hat.com>, poros
<poros@...hat.com>, "Olech, Milena" <milena.olech@...el.com>, mschmidt
<mschmidt@...hat.com>, "linux-clk@...r.kernel.org"
<linux-clk@...r.kernel.org>, "bvanassche@....org" <bvanassche@....org>,
"kuba@...nel.org" <kuba@...nel.org>, "davem@...emloft.net"
<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>
Subject: RE: [PATCH RFC net-next v3 1/2] netdevsim: implement DPLL for
subsystem selftests
On 20 November 2023 6:16 PM CET, Simon Horman wrote:
>
> 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.
>
Hi Simon - much thanks for taking the time to do such a great review.
>> 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.
>
Ohh - yeah, I overlooked that - fixing.
> ...
>
>> +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.
Fully agree, will return immediately with - EFAULT.
>
>> +
>> + 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.
>
It seems I need to start using Smatch - thanks for pointing this error, fixing.
>> + 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.
>
Yes, I noticed that and already fixed it in my branch.
>> + 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.
>
Good catch - will move the free above just before "return 0".
>> + 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