[PATCH] driver core: faux: only create the device if probe() succeeds

[PATCH] driver core: faux: only create the device if probe() succeeds

[Greg Kroah-Hartman]

It's really hard to know if a faux device properly passes the callback
to probe() without having to poke around in the faux_device structure
and then clean up.  Instead of having to have every user of the api do
this logic, just do it in the faux device core itself.

This makes the use of a custom probe() callback for a faux device much
simpler overall.

Suggested-by: Kurt Borja <kuurtb@gmail.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/faux.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/base/faux.c b/drivers/base/faux.c
index 531e9d789ee0..407c1d1aad50 100644
--- a/drivers/base/faux.c
+++ b/drivers/base/faux.c
@@ -102,7 +102,9 @@ static void faux_device_release(struct device *dev)
  *
  * Note, when this function is called, the functions specified in struct
  * faux_ops can be called before the function returns, so be prepared for
- * everything to be properly initialized before that point in time.
+ * everything to be properly initialized before that point in time.  If the
+ * probe callback (if one is present) does NOT succeed, the creation of the
+ * device will fail and NULL will be returned.
  *
  * Return:
  * * NULL if an error happened with creating the device
@@ -147,6 +149,17 @@ struct faux_device *faux_device_create_with_groups(const char *name,
 		return NULL;
 	}
 
+	/*
+	 * Verify that we did bind the driver to the device (i.e. probe worked),
+	 * 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) {
+		dev_err(dev, "probe did not succeed, tearing down the device\n");
+		faux_device_destroy(faux_dev);
+		faux_dev = NULL;
+	}
+
 	return faux_dev;
 }
 EXPORT_SYMBOL_GPL(faux_device_create_with_groups);

Re: [PATCH] driver core: faux: only create the device if probe() succeeds

[Danilo Krummrich]

On Tue, Feb 25, 2025 at 07:35:46AM +0100, Greg Kroah-Hartman wrote:
> It's really hard to know if a faux device properly passes the callback
> to probe() without having to poke around in the faux_device structure
> and then clean up.  Instead of having to have every user of the api do
> this logic, just do it in the faux device core itself.
> 
> This makes the use of a custom probe() callback for a faux device much
> simpler overall.
> 
> Suggested-by: Kurt Borja <kuurtb@gmail.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/faux.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/faux.c b/drivers/base/faux.c
> index 531e9d789ee0..407c1d1aad50 100644
> --- a/drivers/base/faux.c
> +++ b/drivers/base/faux.c
> @@ -102,7 +102,9 @@ static void faux_device_release(struct device *dev)
>   *
>   * Note, when this function is called, the functions specified in struct
>   * faux_ops can be called before the function returns, so be prepared for
> - * everything to be properly initialized before that point in time.
> + * everything to be properly initialized before that point in time.  If the
> + * probe callback (if one is present) does NOT succeed, the creation of the
> + * device will fail and NULL will be returned.
>   *
>   * Return:
>   * * NULL if an error happened with creating the device
> @@ -147,6 +149,17 @@ struct faux_device *faux_device_create_with_groups(const char *name,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * Verify that we did bind the driver to the device (i.e. probe worked),
> +	 * 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) {

Seems like really_probe() cleans things up properly through
device_unbind_cleanup(), such that dev->driver is guaranteed to be NULL on
failure.

> +		dev_err(dev, "probe did not succeed, tearing down the device\n");
> +		faux_device_destroy(faux_dev);
> +		faux_dev = NULL;

NIT: Maybe return NULL directly (like above) in case a subsequent change adds
more code to the end of this function and does not consider that we fall
through.

Either way,

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

> +	}
> +
>  	return faux_dev;
>  }
>  EXPORT_SYMBOL_GPL(faux_device_create_with_groups);

Re: [PATCH] driver core: faux: only create the device if probe() succeeds

[Kurt Borja]

On Tue Feb 25, 2025 at 1:35 AM -05, Greg Kroah-Hartman wrote:
> It's really hard to know if a faux device properly passes the callback
> to probe() without having to poke around in the faux_device structure
> and then clean up.  Instead of having to have every user of the api do
> this logic, just do it in the faux device core itself.
>
> This makes the use of a custom probe() callback for a faux device much
> simpler overall.
>
> Suggested-by: Kurt Borja <kuurtb@gmail.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for considering this!

Reviewed-by: Kurt Borja <kuurtb@gmail.com>

> ---
>  drivers/base/faux.c |   15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/faux.c b/drivers/base/faux.c
> index 531e9d789ee0..407c1d1aad50 100644
> --- a/drivers/base/faux.c
> +++ b/drivers/base/faux.c
> @@ -102,7 +102,9 @@ static void faux_device_release(struct device *dev)
>   *
>   * Note, when this function is called, the functions specified in struct
>   * faux_ops can be called before the function returns, so be prepared for
> - * everything to be properly initialized before that point in time.
> + * everything to be properly initialized before that point in time.  If the
> + * probe callback (if one is present) does NOT succeed, the creation of the
> + * device will fail and NULL will be returned.
>   *
>   * Return:
>   * * NULL if an error happened with creating the device
> @@ -147,6 +149,17 @@ struct faux_device *faux_device_create_with_groups(const char *name,
>  		return NULL;
>  	}
>  
> +	/*
> +	 * Verify that we did bind the driver to the device (i.e. probe worked),
> +	 * 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) {
> +		dev_err(dev, "probe did not succeed, tearing down the device\n");
> +		faux_device_destroy(faux_dev);
> +		faux_dev = NULL;
> +	}
> +
>  	return faux_dev;
>  }
>  EXPORT_SYMBOL_GPL(faux_device_create_with_groups);
-- 
 ~ Kurt

Re: [PATCH] driver core: faux: only create the device if probe() succeeds

[Greg Kroah-Hartman]

On Tue, Feb 25, 2025 at 12:24:33PM +0100, Danilo Krummrich wrote:
> On Tue, Feb 25, 2025 at 07:35:46AM +0100, Greg Kroah-Hartman wrote:
> > It's really hard to know if a faux device properly passes the callback
> > to probe() without having to poke around in the faux_device structure
> > and then clean up.  Instead of having to have every user of the api do
> > this logic, just do it in the faux device core itself.
> > 
> > This makes the use of a custom probe() callback for a faux device much
> > simpler overall.
> > 
> > Suggested-by: Kurt Borja <kuurtb@gmail.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/base/faux.c |   15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/faux.c b/drivers/base/faux.c
> > index 531e9d789ee0..407c1d1aad50 100644
> > --- a/drivers/base/faux.c
> > +++ b/drivers/base/faux.c
> > @@ -102,7 +102,9 @@ static void faux_device_release(struct device *dev)
> >   *
> >   * Note, when this function is called, the functions specified in struct
> >   * faux_ops can be called before the function returns, so be prepared for
> > - * everything to be properly initialized before that point in time.
> > + * everything to be properly initialized before that point in time.  If the
> > + * probe callback (if one is present) does NOT succeed, the creation of the
> > + * device will fail and NULL will be returned.
> >   *
> >   * Return:
> >   * * NULL if an error happened with creating the device
> > @@ -147,6 +149,17 @@ struct faux_device *faux_device_create_with_groups(const char *name,
> >  		return NULL;
> >  	}
> >  
> > +	/*
> > +	 * Verify that we did bind the driver to the device (i.e. probe worked),
> > +	 * 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) {
> 
> Seems like really_probe() cleans things up properly through
> device_unbind_cleanup(), such that dev->driver is guaranteed to be NULL on
> failure.

Yes.

> > +		dev_err(dev, "probe did not succeed, tearing down the device\n");
> > +		faux_device_destroy(faux_dev);
> > +		faux_dev = NULL;
> 
> NIT: Maybe return NULL directly (like above) in case a subsequent change adds
> more code to the end of this function and does not consider that we fall
> through.
> 
> Either way,
> 
> Reviewed-by: Danilo Krummrich <dakr@kernel.org>

Thanks for the review!

greg k-h