Re: [PATCH v2 3/4] phy: core: Add phy bulk data helper functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] of_phy_bulk_get_all() returns early without initializing the `phys` pointer, leading to uninitialized pointer dereference
- [Medium] Inconsistent error code (-EOPNOTSUPP instead of -ENOSYS) returned for disabled Generic PHY subsystem
--

commit b95d19650b85be78835c779ebfd45c729010493a
Author: Inochi Amaoto <inochiama@gmail.com>

phy: core: Add phy bulk data helper functions

Add several helper functions that allow drivers to get several phy
consumers in one operation. If any of the phy cannot be acquired then
any phys that were got will be put before returning to the caller.

> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
[ ... ]
> +int of_phy_bulk_get_all(struct device_node *np, struct phy_bulk_data **phys)
> +{
> +	struct phy_bulk_data *phy_bulk;
> +	int num_phys;
> +	int ret;
> +
> +	num_phys = of_phy_get_count(np);
> +	if (num_phys <= 0)
> +		return num_phys;

[Severity: High]
Does this code return without initializing the phys pointer?

If a driver calls of_phy_bulk_get_all() with an uninitialized pointer
and the device node specifies 0 phys, it will return 0 (success)
without initializing *phys. If the driver later calls
of_phy_bulk_put_all(), it could pass the uninitialized pointer to
kfree().

> +
> +	phy_bulk = kmalloc_objs(*phy_bulk, num_phys);
> +	if (!phy_bulk)
> +		return -ENOMEM;
> +
> +	ret = of_phy_bulk_get_by_index(np, num_phys, phy_bulk);
> +	if (ret) {
> +		kfree(phy_bulk);
> +		return ret;

[Severity: High]
Does this code also leave phys uninitialized on this error path?

Even though callers typically do not call put on error, if they do,
it would also lead to an uninitialized pointer dereference.

> +	}
[ ... ]
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
[ ... ]
> +static inline int phy_bulk_get(struct device *dev, unsigned int num_phys,
> +			       struct phy_bulk_data *phys)
> +{
> +	return -EOPNOTSUPP;
> +}

[Severity: Medium]
Does this code return -EOPNOTSUPP instead of -ENOSYS when the phy
subsystem is disabled?

The established convention in include/linux/phy/phy.h is to consistently
return -ENOSYS for disabled stubs. Drivers checking for -ENOSYS to
gracefully handle a disabled phy subsystem might fail to probe or behave
incorrectly when receiving -EOPNOTSUPP.

[ ... ]
> +static inline int phy_bulk_check_disabled(unsigned int num_phys,
> +					  struct phy_bulk_data *phys)
> +{
> +	if (!phys)
> +		return 0;
> +
> +	for (unsigned int i = 0; i < num_phys; i++)
> +		if (phys[i].phy)
> +			return -EOPNOTSUPP;

[Severity: Medium]
Does this helper incorrectly return -EOPNOTSUPP, propagating the wrong error
code to bulk initialization operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904083709.425893-1-inochiama@gmail.com?part=3

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy