[PATCH] phy: phy-uclass: Add a missing error handling path

[Bhupesh Sharma]

Hi Jonas,

On Mon, 3 Jul 2023 at 01:18, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> On 2023-07-02 20:47, Bhupesh Sharma wrote:
> > Since function 'phy_get_counts()' can return NULL,
> > add handling for that error path inside callers of
> > this function.
>
> Do you have any example where this can/has happened?

Yes, it happened while I was writing a UFS Host Controller driver for
u-boot and tried to initialize the UFS PHY via the generic u-boot PHY
CLASS utility functions, namely:

/* get phy */
ret = generic_phy_get_by_name(hba->dev, "ufsphy", &phy);
...

/* phy initialization */
ret = generic_phy_init(&phy);
...
 /* power on phy */
ret = generic_phy_power_on(&phy);

> Counts should never be NULL in a normal working call flow. I am a bit
> worried that this patch may hide some other bug or use of an
> uninitialized phy struct.

I agree, but I found that if the UFS Host Controller driver would mess
up the phy call sequences while it was in a 'power_up_sequence',
instead of using the standard UCLASS_PHY
'generic_phy_get_by_index_nodev' flow, it might get a counts value
NULL and then the PHY driver would silently crash while setting /
accessing members of 'counts'.

int generic_phy_init(struct phy *phy)
{
    counts = phy_get_counts(phy);
     ...
    if (counts->init_count > 0) {
        // crash
    ..

> The phy struct is initialized in generic_phy_get_by_index_nodev. That
> function should fail when counts cannot be allocated.
>
> Maybe generic_phy_get_by_index_nodev should unset phy->dev if it fails,
> or generic_phy_valid should be extended to also check phy->counts.
> That way generic_phy_valid would return false and these functions
> should return earlier before trying to use counts.

I agree, this error handling should be here for counts being
uninitialized, whether we do it per-function or at the top level.. As
I can see several users of the flow I described in the u-boot code
itself (for setting up a PHY), for e.g.:

 board/sunxi/board.c
 drivers/usb/cdns3/core.c

etc..

Thanks,
Bhupesh