Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 2/2] usb: ulpi: Call of_node_put correctly
From:   Sean Anderson <sean.anderson@seco.com>
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220124173344.874885-1-sean.anderson@seco.com>
 <20220124173344.874885-2-sean.anderson@seco.com>
 <Ye/AUUlnuHBoGxab@kuha.fi.intel.com>
 <dc30490b-15e8-1b10-beb2-eaaa10190649@seco.com>
Message-ID: <284ed076-ac57-c90c-751d-0ced6d0c7c61@seco.com>
Date:   Tue, 25 Jan 2022 12:01:00 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <dc30490b-15e8-1b10-beb2-eaaa10190649@seco.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UnhhRkcyY1RvcVgvYzU1MDdRaDNOY3pIcE1jT1hGUzFsVjVheFNnMGVrck5x?=
 =?utf-8?B?N3YrQnQxU2Nnd0UyTzhJbEtBRVFIbXh3ZEFYcXpIVTZCNmxubHAvWUM5ekxt?=
 =?utf-8?B?WFVKOFpnbHZxQTRyWjRiU1hEbGtaK1ArbEU5dnFQRkFiVk02TE54M1Rnd0RX?=
 =?utf-8?B?eVFFYjJ5TnplNVZ6RElYSHRHNGY4WFJEVTBwOWJTV2lYbzZzbFhxQ3huMEpE?=
 =?utf-8?B?WkZ0ak5NaGFYZEthWTZFbEZOaXFRU1pzbkVtQS9QUzh3Tmt2SFd6MjdIMVhW?=
 =?utf-8?B?dHpNYm5MM3lhMG1UUzY0MVE5c0paY2dhbVBPMmdmQ3ZLU2l1MlJQK3ROeHRM?=
 =?utf-8?B?MlpPNHlIRDlRcmxoSWhtNCtmNmtPakx4eUxtcHNDd2c2NjhvVUkvWDY2TVF5?=
 =?utf-8?B?L2lqYUpENWh1cGYzV3JqZTVYM01jZ05qeHNsN2l1QThlRXNVaXNNUWFJMjdI?=
 =?utf-8?B?L0J2MWpMZU5vRkRTOHVnNE1lYm5yTnR4SFpUQUs0Y0VwT0ZvWmNpWm1Fb2or?=
 =?utf-8?B?aDhOdjM5Z3Zsa3hXMDg3VTVkRGZ6ZnlKWlN6bm9NTWsxYStDeFJDT0gzSnE2?=
 =?utf-8?B?MzA2aEdQVXJQTVdjUUcyT3RxU3ZFaTFueXh3KzhVNVprRjJDeVMrUzZkK1RB?=
 =?utf-8?B?LzVaNlc0ZldyTURtMFZnMjBJd2diRjd4NGhkUnZlajFqWGNGWlVYcTdLeEpD?=
 =?utf-8?B?SHZHUkgyYWpZZC91bXd0SEJrenpLcHFNeWJGZ1QyUUdabGUxL20yaS9Zc2pM?=
 =?utf-8?B?L2JBNzhTWStvT3hOUEdJdk5OelpVVjRYNkFnOUJTRTY5YWwrTXZZOE9LTXFC?=
 =?utf-8?B?VXkyM2pIamVsc0hHYnk1VXhadnllQ0tHcUdsbTZheDFNZEJnSEkxY1JCczJL?=
 =?utf-8?B?ajYwTUdQd21tZExRRHBJQ29GM3c5bjB6ZC8vVnU5VGNhdG5hVERWRFFLc2dX?=
 =?utf-8?B?NSs1dDV5NTVXNkdZUGlNRitjYk5oZksvUVhNYytqMS85RXJXTWdERmdyRGdQ?=
 =?utf-8?B?YXhJWitJckhqbmtEcjdaTko2dzJjSUUxbWxBNTZ0Y2kzWUVzV2xYTnBzbDY0?=
 =?utf-8?B?bS9wS2VrcHFYNHFEeU1FbGY3NERKalYxT0g2bGpXbWtPckxGb2ZITm5DY0dI?=
 =?utf-8?B?czVnUUl2ZGxmZEJ4eGFVVE9ENWtjWWpyR1RpcUZKR2ZGaVVNRFBPZUhJMElr?=
 =?utf-8?B?U3IyM1MxNHhrVElLbHFOeHg1c3dmR3ZJZ3Z4ZUJnWnRHbTliZXN4YnR2a09N?=
 =?utf-8?B?dStVVCtyU3dNZGhiRjZ5cVloVU9RMVc3YnB5Y2VBQ1NvU2ZuRERwYjBJcHRR?=
 =?utf-8?B?WDFnNlhxektNM2tQcjc1VXdEV2N5ZFhmRkVRakVTMWlWdlgzTlVRazdiN2dV?=
 =?utf-8?B?V3dFZlNvTXBPSkdmcmZUanQrSkZlNHVIRWtRLytEWXp2R09yZnRoN0ZpQitJ?=
 =?utf-8?B?REdaWWJkbzFBODVIZHM0Nlh1L0w0K3F3UkR0UXdZeFFsUlNBWnRYNHg3U3ll?=
 =?utf-8?B?TXhNa1VERzJLUlFSWlczdmRIMm5MQ1RJbnpnblY3V2RqT2t4V2JWdm9qNUtS?=
 =?utf-8?B?TnFpaEtVdTJnRTJtbGlURW1qYXFVRFVCUU1RNDNvcFFUVk5RRmhOL1E2RTUy?=
 =?utf-8?B?QUI2T25Md1lMdkxmTzZsaFhNa1k2YUNTWG1Xa2tBcHpNeC83dnFXRVNsSEt2?=
 =?utf-8?B?NTlibW1jQk5TUGl5QlcyaUtMRWJQU2VYM1pzalRZZWlzdXZRV1Q2VjVXWG9x?=
 =?utf-8?B?UjhxSUNpQjg4NXVGTlZpM091MHRFdTM2RnBkK0FhbklWU2xjeDVtVE02RjNZ?=
 =?utf-8?B?RVprdGM4dDZCbXJ4SUc5VXUxMmJFcWxwS1dnUzZjb2hDYjRpSnZWY1pVMHlU?=
 =?utf-8?B?OE9OT3NCckg0VTBvd2pkTW9wd2d4T3I4M2wrajR4SmhBaTdadnpsay9aYm9u?=
 =?utf-8?B?dkxaRkc2a3RtcUVHVXlOaE4vS0x1YURwYmNqTDR4WlFBMVFpM1RtUVJoMGdE?=
 =?utf-8?B?cXJKYUxiQ1ZjU2JYSE5nNzZtNzc1OS92QkxpNmRkVXo5ODZvWkdueVBWbTdG?=
 =?utf-8?B?Y3puYzYzRitqMlpWaHFTZUxCYy9NMENyTUd2RUtmQ1JIckVGQUJvQW16eWR3?=
 =?utf-8?B?SWhiSGpMUS9uQUJkRTB5SUREbWVpKzQzaWZoRmZXVERLTVZmMnlocjEwemk2?=
 =?utf-8?Q?0lpP/RDpo5OFDkepMtyphZo=3D?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: fd545595-9d58-423f-4344-08d9e0244c0e
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4523.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2022 17:01:15.0177
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: SSui/5rcd+cgi8BVYpzjcmbWHtP+vcqz4pCCl9gGVCkJOkwvVtTS+Av8rxmFCqaWiiWO5u5ZCfdpD1soQW1QOg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR03MB4316
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/25/22 11:53 AM, Sean Anderson wrote:
> Hi Heikki,
> 
> On 1/25/22 4:18 AM, Heikki Krogerus wrote:
>> On Mon, Jan 24, 2022 at 12:33:44PM -0500, Sean Anderson wrote:
>>> of_node_put should always be called on device nodes gotten from
>>> of_get_*. Additionally, it should only be called after there are no
>>> remaining users. To address the first issue, call of_node_put if later
>>> steps in ulpi_register fail. To address the latter, call of_node_put
>>> only after calling device_unregister.
>> 
>> This looks like a fix, but you don't have the fix tag.
> 
> You're right this should have
> 
> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT")
> 
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> ---
>>> 
>>> Changes in v2:
>>> - New
>>> 
>>>  drivers/usb/common/ulpi.c | 10 +++++++---
>>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c
>>> index 87deb514eb78..c6ba72544f2b 100644
>>> --- a/drivers/usb/common/ulpi.c
>>> +++ b/drivers/usb/common/ulpi.c
>>> @@ -301,11 +301,11 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>>>  
>>>  	ret = ulpi_read_id(ulpi);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto err;
>>>  
>>>  	ret = device_register(&ulpi->dev);
>>>  	if (ret)
>>> -		return ret;
>>> +		goto err;
>> 
>> I think there is another bug in the code here. Missing put_device().
> 
> So what is the correct way to create a device? Shouldn't device_register
> be paired with device_unregister? And from what I can tell,
> device_unregister does not put the of_node.
> 
>> If you first fix that, you should then be able to call
>> fwnode_handle_put() (instead of of_node_put())
> 
> Well, we currently only have a ulpi_of_register, so I don't think we
> will have a fwnode here. But I can use that if you wish.

Hm, looks like I missed the ACPI_COMPANION_SET

So this should probably be fwnode_handle_put.

But ACPI_COMPANION_SET doesn't seem to get the fwnode?

> --Sean
> 
>> from
>> ulpi_dev_release(), and that should cover all cases.
>> 
>>>  	root = debugfs_create_dir(dev_name(dev), ULPI_ROOT);
>>>  	debugfs_create_file("regs", 0444, root, ulpi, &ulpi_regs_ops);
>>> @@ -314,6 +314,10 @@ static int ulpi_register(struct device *dev, struct ulpi *ulpi)
>>>  		ulpi->id.vendor, ulpi->id.product);
>>>  
>>>  	return 0;
>>> +
>>> +err:
>>> +	of_node_put(ulpi->dev.of_node);
>>> +	return ret;
>> 
>> So no need for that.
>> 
>>>  }
>>>  
>>>  /**
>>> @@ -357,8 +361,8 @@ void ulpi_unregister_interface(struct ulpi *ulpi)
>>>  {
>>>  	debugfs_remove_recursive(debugfs_lookup(dev_name(&ulpi->dev),
>>>  						ULPI_ROOT));
>>> -	of_node_put(ulpi->dev.of_node);
>>>  	device_unregister(&ulpi->dev);
>>> +	of_node_put(ulpi->dev.of_node);
>>>  }
>> 
>> And here you can just remove that of_node_put() call.
>> 
>> thanks,
>> 
>