Hi Jonas, On Mon, 3 Jul 2023 at 01:18, Jonas Karlman <jo...@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