[PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[Yi Zhang]

the driver_data may be used for sanity check, it fails the
probe() if driver_data is NULL after it is re-triggered.
for example, soc_probe() in sound/soc/soc-core.c

Signed-off-by: Yi Zhang <yizhang_hust@163.com>
---
 drivers/base/dd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index c4da2df..30c53d3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -402,7 +402,8 @@ pinctrl_bind_failed:
 	devres_release_all(dev);
 	driver_sysfs_remove(dev);
 	dev->driver = NULL;
-	dev_set_drvdata(dev, NULL);
+	if (ret != -EPROBE_DEFER)
+		dev_set_drvdata(dev, NULL);
 	if (dev->pm_domain && dev->pm_domain->dismiss)
 		dev->pm_domain->dismiss(dev);
 	pm_runtime_reinit(dev);
-- 
1.9.1

Re: [PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On Tue, Mar 08, 2016 at 11:15:22PM +0800, Yi Zhang wrote:
> the driver_data may be used for sanity check, it fails the
> probe() if driver_data is NULL after it is re-triggered.
> for example, soc_probe() in sound/soc/soc-core.c
> 
> Signed-off-by: Yi Zhang <yizhang_hust-9Onoh4P/yGk@public.gmane.org>
> ---
>  drivers/base/dd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Hi Greg,

This causes a boot regression on at least one board, caused by one of
the drivers looking at driver data to check whether or not the driver
has properly loaded. If the code encounters a non-NULL pointer it
tries to dereference it, but because it's already been freed there is
no memory backing it and things crash.

I don't think keeping stale pointers around is a good idea. The whole
point of setting this to NULL in the core is so that probe failures
result in the same starting conditions no matter what.

Can we please get this reverted?

Cc'ing linux-tegra for visibility since that's where the boot regression
is observed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

On Tue, Mar 08, 2016 at 11:15:22PM +0800, Yi Zhang wrote:
> the driver_data may be used for sanity check, it fails the
> probe() if driver_data is NULL after it is re-triggered.
> for example, soc_probe() in sound/soc/soc-core.c
> 
> Signed-off-by: Yi Zhang <yizhang_hust@163.com>
> ---
>  drivers/base/dd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Hi Greg,

This causes a boot regression on at least one board, caused by one of
the drivers looking at driver data to check whether or not the driver
has properly loaded. If the code encounters a non-NULL pointer it
tries to dereference it, but because it's already been freed there is
no memory backing it and things crash.

I don't think keeping stale pointers around is a good idea. The whole
point of setting this to NULL in the core is so that probe failures
result in the same starting conditions no matter what.

Can we please get this reverted?

Cc'ing linux-tegra for visibility since that's where the boot regression
is observed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[Greg KH]

On Tue, May 03, 2016 at 03:11:26PM +0200, Thierry Reding wrote:
> On Tue, Mar 08, 2016 at 11:15:22PM +0800, Yi Zhang wrote:
> > the driver_data may be used for sanity check, it fails the
> > probe() if driver_data is NULL after it is re-triggered.
> > for example, soc_probe() in sound/soc/soc-core.c
> > 
> > Signed-off-by: Yi Zhang <yizhang_hust-9Onoh4P/yGk@public.gmane.org>
> > ---
> >  drivers/base/dd.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Hi Greg,
> 
> This causes a boot regression on at least one board, caused by one of
> the drivers looking at driver data to check whether or not the driver
> has properly loaded. If the code encounters a non-NULL pointer it
> tries to dereference it, but because it's already been freed there is
> no memory backing it and things crash.
> 
> I don't think keeping stale pointers around is a good idea. The whole
> point of setting this to NULL in the core is so that probe failures
> result in the same starting conditions no matter what.
> 
> Can we please get this reverted?
> 
> Cc'ing linux-tegra for visibility since that's where the boot regression
> is observed.

Now reverted, thanks for letting me know.

greg k-h

Re: [PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[Greg KH]

On Tue, May 03, 2016 at 03:11:26PM +0200, Thierry Reding wrote:
> On Tue, Mar 08, 2016 at 11:15:22PM +0800, Yi Zhang wrote:
> > the driver_data may be used for sanity check, it fails the
> > probe() if driver_data is NULL after it is re-triggered.
> > for example, soc_probe() in sound/soc/soc-core.c
> > 
> > Signed-off-by: Yi Zhang <yizhang_hust@163.com>
> > ---
> >  drivers/base/dd.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Hi Greg,
> 
> This causes a boot regression on at least one board, caused by one of
> the drivers looking at driver data to check whether or not the driver
> has properly loaded. If the code encounters a non-NULL pointer it
> tries to dereference it, but because it's already been freed there is
> no memory backing it and things crash.
> 
> I don't think keeping stale pointers around is a good idea. The whole
> point of setting this to NULL in the core is so that probe failures
> result in the same starting conditions no matter what.
> 
> Can we please get this reverted?
> 
> Cc'ing linux-tegra for visibility since that's where the boot regression
> is observed.

Now reverted, thanks for letting me know.

greg k-h

Re: [PATCH] base: dd: don't remove driver_data in -EPROBE_DEFER case

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1338 bytes --]

On Tue, May 03, 2016 at 08:07:39AM -0700, Greg KH wrote:
> On Tue, May 03, 2016 at 03:11:26PM +0200, Thierry Reding wrote:
> > On Tue, Mar 08, 2016 at 11:15:22PM +0800, Yi Zhang wrote:
> > > the driver_data may be used for sanity check, it fails the
> > > probe() if driver_data is NULL after it is re-triggered.
> > > for example, soc_probe() in sound/soc/soc-core.c
> > > 
> > > Signed-off-by: Yi Zhang <yizhang_hust@163.com>
> > > ---
> > >  drivers/base/dd.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Hi Greg,
> > 
> > This causes a boot regression on at least one board, caused by one of
> > the drivers looking at driver data to check whether or not the driver
> > has properly loaded. If the code encounters a non-NULL pointer it
> > tries to dereference it, but because it's already been freed there is
> > no memory backing it and things crash.
> > 
> > I don't think keeping stale pointers around is a good idea. The whole
> > point of setting this to NULL in the core is so that probe failures
> > result in the same starting conditions no matter what.
> > 
> > Can we please get this reverted?
> > 
> > Cc'ing linux-tegra for visibility since that's where the boot regression
> > is observed.
> 
> Now reverted, thanks for letting me know.

Thanks Greg.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]