Re: [PATCH 7/7] usb: phy: registering tegra USB PHY as platform driver

[Stephen Warren]

On 03/20/2013 06:43 AM, Venu Byravarasu wrote:
> Stephen Warren wrote at Wednesday, March 20, 2013 1:51 AM:
>> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>>> Registered tegra USB PHY as a separate platform driver.

>>> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c

>>> +static int tegra_usb_phy_probe(struct platform_device *pdev)
>>
>> Hmmm. Note that in order to make deferred probe work correctly, all the
>> gpio_request(), clk_get(), etc. calls that acquire resources from other
>> drivers must happen here in probe() and not in tegra_usb_phy_open().
>  
> In present code tegra_usb_phy_open() is indirectly called via usb_phy_init() from ehci-tegra.c.
> Between obtaining PHY handle (and hence getting into deferred probe, when it is not available)
> and calling usb_phy_init, no other USB registers were accessed. Hence I was doing this way.
> 
> Do you still think it would be better to move gpio and clk APIs to probe?

Yes.

What's happening right now is that the PHY driver potentially completes
probe() before its resources are available.

This just happens not to break anything because the EHCI driver's probe
ends up getting deferred until some other PHY driver function can
acquire those resources, so the USB port as a whole isn't registered
until the resources are available.

However, this still means that the PHY driver could be suspended after
e.g. a driver that provides a GPIO it uses, since the PHY's probe
completed before the GPIO driver's probe.

Hence, this could easily cause some form of suspend/resume or perhaps
even shutdown/reboot problem.

>>> +struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
>>> +{
>>> +	struct device *dev;
>>> +	struct tegra_usb_phy *tegra_phy;
>>> +
>>> +	dev = driver_find_device(&tegra_usb_phy_driver.driver, NULL, dn,
>>> +				 tegra_usb_phy_match);
>>> +	if (!dev)
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +	tegra_phy = dev_get_drvdata(dev);
>>> +
>>> +	return &tegra_phy->u_phy;
>>> +}
>>
>> I think you need a module_get() somewhere in there, and also need to add
>> a tegra_usb_put_phy() function too, so you can call module_put() from it.
> 
> Am not clear on why to add module_get here. Can you plz provide more details?

Actually, I guess it isn't needed, although slightly by accident perhaps:

If ehci-tegra.c and phy-usb-tegra.c are built as modules, they'll be
separate modules. Since ehci-tegra.c calls into phy-usb-tegra.c, you
need to make sure that phy-usb-tegra.c stays loaded any time
ehci-tegra.c is loaded.

Right now, this is ensured because ehci-tegra.c references functions in
phy-usb-tegra.c by symbol name, so a dependency exists. So, I guess
everything actually works out without the module_get(). So, no changes
are needed.

After this series is applied, I believe that tegra_usb_get_phy() is the
last function that ehci-tegra.c references by symbol name. Eventually,
that function will be replaced by devm_of_phy_get_byname() (see Kishon's
generic PHY API patch series), so there won't be any symbol name
dependencies, so some other mechanism must be used to ensure the PHY
driver stays loaded while the EHCI driver is using it. At that point,
the implementation of devm_of_phy_get_byname() should be calling
module_get(), so again no changes should be required to your patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/