From: Zijun Hu <quic_zijuhu@quicinc.com>

Use dedicated API device_is_bound() instead of 'dev->driver' to check
device binding state in faux_device_create_with_groups().

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/base/faux.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 407c1d1aad50b7969a6dab9d2027d8beab66a754..76fe494a128254aaaf1339386ab37817a732781f 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -154,7 +154,8 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 	 * if not, let's fail the creation as trying to guess if probe was
 	 * successful is almost impossible to determine by the caller.
 	 */
-	if (!dev->driver) {
+	/* Do not need to device_lock(dev) due to no race here */
+	if (!device_is_bound(dev)) {
 		dev_err(dev, "probe did not succeed, tearing down the device\n");
 		faux_device_destroy(faux_dev);
 		faux_dev = NULL;

---
base-commit: 21b0dc55bed6d9b5dd5d1ad22b75d9d1c7426bbc
change-id: 20250307-fix_faux-0420f1d8eca6

Best regards,
-- 
Zijun Hu <quic_zijuhu@quicinc.com>

On Fri, Mar 07, 2025 at 08:47:16PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@quicinc.com>
> 
> Use dedicated API device_is_bound() instead of 'dev->driver' to check
> device binding state in faux_device_create_with_groups().
> 
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/base/faux.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/faux.c b/drivers/base/faux.c
> index 407c1d1aad50b7969a6dab9d2027d8beab66a754..76fe494a128254aaaf1339386ab37817a732781f 100644
> --- a/drivers/base/faux.c
> +++ b/drivers/base/faux.c
> @@ -154,7 +154,8 @@ struct faux_device *faux_device_create_with_groups(const char *name,
>  	 * if not, let's fail the creation as trying to guess if probe was
>  	 * successful is almost impossible to determine by the caller.
>  	 */
> -	if (!dev->driver) {
> +	/* Do not need to device_lock(dev) due to no race here */
> +	if (!device_is_bound(dev)) {

Ick, this feels wrong.  This is the driver core code, it can poke in
dev->driver if it wants to, and as the lock is not held, and there is no
lock even mentioned in this driver, the comment is confusing unless you
dig and go read that device_is_bound() requires this.

Also, when we start to add locking requirements to functions like
device_is_bound() (which we really should) this call will fail the
check, right?  How are you going to explain that?  :)

thanks,

greg k-h

On 2025/3/7 22:04, Greg Kroah-Hartman wrote:
>> -	if (!dev->driver) {
>> +	/* Do not need to device_lock(dev) due to no race here */
>> +	if (!device_is_bound(dev)) {
> Ick, this feels wrong.  This is the driver core code, it can poke in

both 'dev->driver' and device_is_bound() is okay with this very special
context.

'dev->driver'    :     is binding or have bound successfully.
device_is_bound():     have bound successfully.

device_is_bound() may be more accurate here.

> dev->driver if it wants to, and as the lock is not held, and there is no
> lock even mentioned in this driver, the comment is confusing unless you
> dig and go read that device_is_bound() requires this.
> 

agree.

> Also, when we start to add locking requirements to functions like
> device_is_bound() (which we really should) this call will fail the
> check, right?  How are you going to explain that?  🙂

Generically, for 'dev->driver' and device_is_bound(), device_lock()
should be hold firstly to avoid race as driver core codes do.

this patch may be still okay if device_is_bound() is changed to
bool device_is_bound(struct device *dev)
{
	bool ret;
	
	device_lock(dev);
	ret = dev->p && klist_node_attached(&dev->p->knode_driver);
	device_unlock(dev);

	return ret;
}