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