linux-kernel - Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <51ecc509-e277-591d-3db5-fc9d46eee326@gmail.com>
Date:   Sat, 4 Jan 2020 02:19:01 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     Peter Chen <peter.chen@....com>, Rob Herring <robh-dt@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Felipe Balbi <balbi@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module
 to driver's dependencies

03.01.2020 10:25, Michał Mirosław пишет:
> On Thu, Jan 02, 2020 at 06:17:47PM +0300, Dmitry Osipenko wrote:
>> 31.12.2019 00:02, Michał Mirosław пишет:
>>> On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
>>>> 24.12.2019 00:32, Michał Mirosław пишет:
>>>>> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
>>>>>> 20.12.2019 06:56, Peter Chen пишет:
>>>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>>>> [...]
>>>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>>>>>>  	struct tegra_udc *udc;
>>>>>>>>  	int err;
>>>>>>>>  
>>>>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>>>>>> +		err = request_module("phy_tegra_usb");
>>>>>>>> +		if (err)
>>>>>>>> +			return err;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> Why you do this dependency, if this controller driver can't
>>>>>>> get USB PHY, it should return error. What's the return value
>>>>>>> after calling below:
>>>>>>>
>>>>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
>>>>>>
>>>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>>>>
>>>>> How are other driver modules autoloaded? Isn't there an appropriate
>>>>> MODALIAS or MODULE_DEVICE_TABLE in there?
>>>>
>>>> Hello Michał,
>>>>
>>>> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
>>>>
>>>> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
>>>> module and thus the PHY module should be loaded before the CI module,
>>>> otherwise CI driver fails with the EPROBE_DEFER.
>>>
>>> Why, then, is CI driver not being probed again after PHY driver loads?
>>> EPROBE_DEFER is what should cause driver core to re-probe a device after
>>> other devices appear (PHY in this case).
>>
>> CI driver is getting re-probed just fine if PHY's driver module is
>> loaded manually after loading the CI's module. This patch removes this
>> necessity to manually load PHY's module.
>>
>> This is just a minor convenience change that brings the CI's driver
>> loading behaviour on par with the behaviour of loading Tegra's EHCI
>> driver module.
> 
> I fully understand the goal, but what I'm missing is that why this
> doesn't work out of the box? If the PHY module is autoloaded, and so is
> CI driver, and (as I understand) the driver's probe() correctly returns
> EPROBE_DEFER when PHY is not probed yet, then I guess that means bug
> somewhere else and the patch just covers it up.

It works out of the box, but it also could work a bit better in a case
of manually reloading modules. Perhaps it should be possible to derive
module dependencies from the Kconfig dependencies, apparently kernel
doesn't support it yet or maybe there is some reason why it can't be done.