Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1278404 > unrolled thread

[PATCH] base/platform: fix panic when probe function is NULL

Back to article view | Back to linux.kernel

Contents

  [PATCH] base/platform: fix panic when probe function is NULL martin.wilck@ts.fujitsu.com - 2015-11-26 20:20 +0100
    Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-11-26 21:40 +0100
      Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-11-27 08:40 +0100
      Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-11-28 17:50 +0100
      Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-11-28 17:50 +0100
        Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-11-29 00:00 +0100
          Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-11-30 14:00 +0100
    Re: [PATCH] base/platform: fix panic when probe function is NULL Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-11-27 11:20 +0100
      Re: [PATCH] base/platform: fix panic when probe function is NULL "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-11-30 08:50 +0100
      [PATCH v2] base/platform: return success when probe function is NULL martin.wilck@ts.fujitsu.com - 2015-11-30 13:00 +0100
        Re: [PATCH v2] base/platform: return success when probe function is  NULL Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 21:40 +0100
      Re: [PATCH] base/platform: fix panic when probe function is NULL "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-01 11:50 +0100
        Re: [PATCH] base/platform: fix panic when probe function is NULL Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-12-01 14:30 +0100
          Re: [PATCH] base/platform: fix panic when probe function is NULL "Wilck, Martin" <martin.wilck@ts.fujitsu.com> - 2015-12-01 16:20 +0100
            Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 18:30 +0100
              Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL Peter Huewe <peterhuewe@gmx.de> - 2015-12-01 19:40 +0100
                Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 19:50 +0100
                  Aw: Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL "Peter Huewe" <PeterHuewe@gmx.de> - 2015-12-01 20:00 +0100
                    Re: Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2015-12-01 20:10 +0100
    Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-11-28 17:40 +0100
      Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-11-29 11:00 +0100
        Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-11-30 14:00 +0100
          Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe  function is NULL Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-11-30 14:10 +0100

Page 1 of 2  [1] 2  Next page →

#1278404 — [PATCH] base/platform: fix panic when probe function is NULL

From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

Since b8b2c7d845d5, platform_drv_probe() is called for all platform
devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
platform_drv_probe() will return the error code from dev_pm_domain_attach().

This causes real_probe() to enter the "probe_failed" path and set
dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
success if both dev->bus->probe and drv->probe are missing.

This may cause a panic later. For example, inserting the tpm_tis
driver with parameter "force=1" (i.e. registering tpm_tis as a platform
driver) will panic in tpmm_chip_alloc() because dev->driver is NULL:

     chip->cdev.owner = chip->pdev->driver->owner;

This patch fixes this by returning success in platform_drv_probe() if
"just" dev_pm_domain_attach() had failed. This restores the semantics
of platform_device_register_XXX() if the associated platform driver has
no "probe" function.

Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
callbacks are called unconditionally")

Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
---
 drivers/base/platform.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1dd6d3b..c994e76 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -513,10 +513,14 @@ static int platform_drv_probe(struct device *_dev)
 		return ret;
 
 	ret = dev_pm_domain_attach(_dev, true);
-	if (ret != -EPROBE_DEFER && drv->probe) {
-		ret = drv->probe(dev);
-		if (ret)
-			dev_pm_domain_detach(_dev, true);
+	if (ret != -EPROBE_DEFER) {
+		if (drv->probe) {
+			ret = drv->probe(dev);
+			if (ret)
+				dev_pm_domain_detach(_dev, true);
+		} else
+			/* don't fail if just dev_pm_domain_attach failed */
+			ret = 0;
 	}
 
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1278430 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Thu, Nov 26, 2015 at 08:01:34PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> platform_drv_probe() will return the error code from dev_pm_domain_attach().
> 
> This causes real_probe() to enter the "probe_failed" path and set
> dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> success if both dev->bus->probe and drv->probe are missing.
> 
> This may cause a panic later. For example, inserting the tpm_tis
> driver with parameter "force=1" (i.e. registering tpm_tis as a platform
> driver) will panic in tpmm_chip_alloc() because dev->driver is NULL:
> 
>      chip->cdev.owner = chip->pdev->driver->owner;

Is this happening because tpm_tis is not creating the platform device
properly? ie it just calls platform_device_register_simple and then
force initializes it via tpm_tis_init, which expects to be called from
a probe function with an attached driver.

Instead we should setup a proper platform device with the default
IO range for x86 and let the driver core call tpm_tis_init via
tis_drv.probe.

Would changing things in this way fix the problem you've observed?

I have some patches to do this that are part of my OF enablement
series, but I can make something simpler that would deal with this
fairly quickly if you can test.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278581 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

T24gRG8sIDIwMTUtMTEtMjYgYXQgMTM6MzAgLTA3MDAsIEphc29uIEd1bnRob3JwZSB3cm90ZToK
PiBPbiBUaHUsIE5vdiAyNiwgMjAxNSBhdCAwODowMTozNFBNICswMTAwLCBtYXJ0aW4ud2lsY2tA
dHMuZnVqaXRzdS5jb20gd3JvdGU6Cj4gPiBGcm9tOiBNYXJ0aW4gV2lsY2sgPE1hcnRpbi5XaWxj
a0B0cy5mdWppdHN1LmNvbT4KPiA+IAo+ID4gU2luY2UgYjhiMmM3ZDg0NWQ1LCBwbGF0Zm9ybV9k
cnZfcHJvYmUoKSBpcyBjYWxsZWQgZm9yIGFsbCBwbGF0Zm9ybQo+ID4gZGV2aWNlcy4gSWYgZHJ2
LT5wcm9iZSBpcyBOVUxMLCBhbmQgZGV2X3BtX2RvbWFpbl9hdHRhY2goKSBmYWlscywKPiA+IHBs
YXRmb3JtX2Rydl9wcm9iZSgpIHdpbGwgcmV0dXJuIHRoZSBlcnJvciBjb2RlIGZyb20gZGV2X3Bt
X2RvbWFpbl9hdHRhY2goKS4KPiA+IAo+ID4gVGhpcyBjYXVzZXMgcmVhbF9wcm9iZSgpIHRvIGVu
dGVyIHRoZSAicHJvYmVfZmFpbGVkIiBwYXRoIGFuZCBzZXQKPiA+IGRldi0+ZHJpdmVyIHRvIE5V
TEwuIEJlZm9yZSBiOGIyYzdkODQ1ZDUsIHJlYWxfcHJvYmUoKSB3b3VsZCBhc3N1bWUKPiA+IHN1
Y2Nlc3MgaWYgYm90aCBkZXYtPmJ1cy0+cHJvYmUgYW5kIGRydi0+cHJvYmUgYXJlIG1pc3Npbmcu
Cj4gPiAKPiA+IFRoaXMgbWF5IGNhdXNlIGEgcGFuaWMgbGF0ZXIuIEZvciBleGFtcGxlLCBpbnNl
cnRpbmcgdGhlIHRwbV90aXMKPiA+IGRyaXZlciB3aXRoIHBhcmFtZXRlciAiZm9yY2U9MSIgKGku
ZS4gcmVnaXN0ZXJpbmcgdHBtX3RpcyBhcyBhIHBsYXRmb3JtCj4gPiBkcml2ZXIpIHdpbGwgcGFu
aWMgaW4gdHBtbV9jaGlwX2FsbG9jKCkgYmVjYXVzZSBkZXYtPmRyaXZlciBpcyBOVUxMOgo+ID4g
Cj4gPiAgICAgIGNoaXAtPmNkZXYub3duZXIgPSBjaGlwLT5wZGV2LT5kcml2ZXItPm93bmVyOwo+
IAo+IElzIHRoaXMgaGFwcGVuaW5nIGJlY2F1c2UgdHBtX3RpcyBpcyBub3QgY3JlYXRpbmcgdGhl
IHBsYXRmb3JtIGRldmljZQo+IHByb3Blcmx5PyBpZSBpdCBqdXN0IGNhbGxzIHBsYXRmb3JtX2Rl
dmljZV9yZWdpc3Rlcl9zaW1wbGUgYW5kIHRoZW4KPiBmb3JjZSBpbml0aWFsaXplcyBpdCB2aWEg
dHBtX3Rpc19pbml0LCB3aGljaCBleHBlY3RzIHRvIGJlIGNhbGxlZCBmcm9tCj4gYSBwcm9iZSBm
dW5jdGlvbiB3aXRoIGFuIGF0dGFjaGVkIGRyaXZlci4KPiAKPiBJbnN0ZWFkIHdlIHNob3VsZCBz
ZXR1cCBhIHByb3BlciBwbGF0Zm9ybSBkZXZpY2Ugd2l0aCB0aGUgZGVmYXVsdAo+IElPIHJhbmdl
IGZvciB4ODYgYW5kIGxldCB0aGUgZHJpdmVyIGNvcmUgY2FsbCB0cG1fdGlzX2luaXQgdmlhCj4g
dGlzX2Rydi5wcm9iZS4KPiAKPiBXb3VsZCBjaGFuZ2luZyB0aGluZ3MgaW4gdGhpcyB3YXkgZml4
IHRoZSBwcm9ibGVtIHlvdSd2ZSBvYnNlcnZlZD8KCkkgdGhpbmsgc28uIE5vbmV0aGVsZXNzLCBw
YXRjaCBiOGIyYzdkODQ1ZDUgaW50cm9kdWNlZCBhIGNoYW5nZSBpbiB0aGUKd2F5IHBsYXRmb3Jt
IGRldmljZSByZWdpc3RyYXRpb24gYmVoYXZlcy4gVGhlIHBsYXRmb3JtIGRldmljZSBjb2RlIHNl
ZW1zCnRvIGJlIHByZXBhcmVkIGZvciBjYXNlcyB3aGVyZSBwbGF0Zm9ybV9kcml2ZXItPnByb2Jl
ID09IE5VTEwsIHNvIHRoYXQKY2FzZSBzaG91bGQgYmUgaGFuZGxlZCBncmFjZWZ1bGx5LiBPdGhl
cndpc2UsIGZhaWx1cmUgc2hvdWxkIG9jY3VyCmVhcmxpZXIsIGUuZy4gd2hlbiBwbGF0Zm9ybV9k
cml2ZXJfcmVnaXN0ZXIoKSBpcyBjYWxsZWQgd2l0aCAKcGxhdGZvcm1fZHJpdmVyLT5wcm9iZSA9
PSBOVUxMLiB0cG1fdGlzIG1heSBub3QgYmUgdGhlIG9ubHkgZHJpdmVyIHRoYXQKdXNlcyBwbGF0
Zm9ybV9kZXZpY2VfcmVnaXN0ZXJfc2ltcGxlKCkgaW4gdGhpcyB3YXkuCgo+IEkgaGF2ZSBzb21l
IHBhdGNoZXMgdG8gZG8gdGhpcyB0aGF0IGFyZSBwYXJ0IG9mIG15IE9GIGVuYWJsZW1lbnQKPiBz
ZXJpZXMsIGJ1dCBJIGNhbiBtYWtlIHNvbWV0aGluZyBzaW1wbGVyIHRoYXQgd291bGQgZGVhbCB3
aXRoIHRoaXMKPiBmYWlybHkgcXVpY2tseSBpZiB5b3UgY2FuIHRlc3QuCgpMZXQncyBmaXJzdCB3
YWl0IHdoYXQgdGhlIHBsYXRmb3JtIGd1eXMgc2F5LgoKTWFydGluCgo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279176 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Sat, Nov 28, 2015 at 06:40:03PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 26, 2015 at 01:30:31PM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 26, 2015 at 08:01:34PM +0100, martin.wilck@ts.fujitsu.com wrote:
> > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> > > 
> > > Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> > > devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> > > platform_drv_probe() will return the error code from dev_pm_domain_attach().
> > > 
> > > This causes real_probe() to enter the "probe_failed" path and set
> > > dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> > > success if both dev->bus->probe and drv->probe are missing.
> > > 
> > > This may cause a panic later. For example, inserting the tpm_tis
> > > driver with parameter "force=1" (i.e. registering tpm_tis as a platform
> > > driver) will panic in tpmm_chip_alloc() because dev->driver is NULL:
> > > 
> > >      chip->cdev.owner = chip->pdev->driver->owner;
> > 
> > Is this happening because tpm_tis is not creating the platform device
> > properly? ie it just calls platform_device_register_simple and then
> > force initializes it via tpm_tis_init, which expects to be called from
> > a probe function with an attached driver.
> 
> Agreed. We should have a probe callback.
> 
> > Instead we should setup a proper platform device with the default
> > IO range for x86 and let the driver core call tpm_tis_init via
> > tis_drv.probe.
> > 
> > Would changing things in this way fix the problem you've observed?
> > 
> > I have some patches to do this that are part of my OF enablement
> > series, but I can make something simpler that would deal with this
> > fairly quickly if you can test.
> 
> Does the patch set that you sent include the fix or not? I haven't yet
> reviewed them properly.

Another question: does you patch series include an alternative fix for
the probe bug or should I just pick Martins fix? As I sad previously I
was seriously lost with the race but now I understand what you and
Martin were saying (and feel utterly stupid + ashamed!). Now I'm just
thinking, which fix I should pick.

Anyway, I'll try to go through your code ASAP.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279177 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Thu, Nov 26, 2015 at 01:30:31PM -0700, Jason Gunthorpe wrote:
> On Thu, Nov 26, 2015 at 08:01:34PM +0100, martin.wilck@ts.fujitsu.com wrote:
> > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> > 
> > Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> > devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> > platform_drv_probe() will return the error code from dev_pm_domain_attach().
> > 
> > This causes real_probe() to enter the "probe_failed" path and set
> > dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> > success if both dev->bus->probe and drv->probe are missing.
> > 
> > This may cause a panic later. For example, inserting the tpm_tis
> > driver with parameter "force=1" (i.e. registering tpm_tis as a platform
> > driver) will panic in tpmm_chip_alloc() because dev->driver is NULL:
> > 
> >      chip->cdev.owner = chip->pdev->driver->owner;
> 
> Is this happening because tpm_tis is not creating the platform device
> properly? ie it just calls platform_device_register_simple and then
> force initializes it via tpm_tis_init, which expects to be called from
> a probe function with an attached driver.

Agreed. We should have a probe callback.

> Instead we should setup a proper platform device with the default
> IO range for x86 and let the driver core call tpm_tis_init via
> tis_drv.probe.
> 
> Would changing things in this way fix the problem you've observed?
> 
> I have some patches to do this that are part of my OF enablement
> series, but I can make something simpler that would deal with this
> fairly quickly if you can test.

Does the patch set that you sent include the fix or not? I haven't yet
reviewed them properly.

> Jason

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279243 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Sat, Nov 28, 2015 at 06:40:03PM +0200, Jarkko Sakkinen wrote:
> > I have some patches to do this that are part of my OF enablement
> > series, but I can make something simpler that would deal with this
> > fairly quickly if you can test.
> 
> Does the patch set that you sent include the fix or not? I haven't yet
> reviewed them properly.

No fixing probe is another task. I can send some patches for that when
we are done with the IRQ stuff. That is something we should fix no
matter what..

BTW, please test my IRQ series, I forgot to mention I was unable to
test it properly here...

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279845 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Sat, Nov 28, 2015 at 03:52:51PM -0700, Jason Gunthorpe wrote:
> On Sat, Nov 28, 2015 at 06:40:03PM +0200, Jarkko Sakkinen wrote:
> > > I have some patches to do this that are part of my OF enablement
> > > series, but I can make something simpler that would deal with this
> > > fairly quickly if you can test.
> > 
> > Does the patch set that you sent include the fix or not? I haven't yet
> > reviewed them properly.
> 
> No fixing probe is another task. I can send some patches for that when
> we are done with the IRQ stuff. That is something we should fix no
> matter what..
> 
> BTW, please test my IRQ series, I forgot to mention I was unable to
> test it properly here...

Got you. I need to at least test insmod/rmod (maybe couple of times in a
cycle).  Do you see any other code paths that could easily break because
of your patches?

> Jason

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1278700

Hello Martin,

On Thu, Nov 26, 2015 at 08:01:34PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> platform_drv_probe() will return the error code from dev_pm_domain_attach().

Correct, this is an unintended change of behaviour introduced in
b8b2c7d845d5.

> This causes real_probe() to enter the "probe_failed" path and set
> dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> success if both dev->bus->probe and drv->probe are missing.
> 
> This may cause a panic later. For example, inserting the tpm_tis
> driver with parameter "force=1" (i.e. registering tpm_tis as a platform
> driver) will panic in tpmm_chip_alloc() because dev->driver is NULL:
> 
>      chip->cdev.owner = chip->pdev->driver->owner;

This sounds like a separate issue though. Looking at init_tis there is:

        rc = platform_driver_register(&tis_drv);
        if (rc < 0)
                return rc;
        pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
        if (IS_ERR(pdev)) {
                rc = PTR_ERR(pdev);
                goto err_dev;
        }
        rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);

tpm_tis_init calls tpmm_chip_alloc which barfs when pdev (i.e. the return value
of platform_device_register_simple above) isn't bound. It is not allowed
to assume that the device is bound after the above function calls.

So I'd say drop the paragraph about tpm_tis and the change is fine.

> This patch fixes this by returning success in platform_drv_probe() if
> "just" dev_pm_domain_attach() had failed. This restores the semantics
> of platform_device_register_XXX() if the associated platform driver has
> no "probe" function.
> 
> Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> callbacks are called unconditionally")
> 

I think line breaks in the Fixes: line are frowned on. Also usually
there is no empty line between Fixes: and S-o-b:.

> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> ---
>  drivers/base/platform.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1dd6d3b..c994e76 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -513,10 +513,14 @@ static int platform_drv_probe(struct device *_dev)
>  		return ret;
>  
>  	ret = dev_pm_domain_attach(_dev, true);
> -	if (ret != -EPROBE_DEFER && drv->probe) {
> -		ret = drv->probe(dev);
> -		if (ret)
> -			dev_pm_domain_detach(_dev, true);
> +	if (ret != -EPROBE_DEFER) {
> +		if (drv->probe) {
> +			ret = drv->probe(dev);
> +			if (ret)
> +				dev_pm_domain_detach(_dev, true);
> +		} else
> +			/* don't fail if just dev_pm_domain_attach failed */
> +			ret = 0;

An else that has a } should also have a {, according to 
checkpatch and Documentation/CodingStyle. You can write it
alternatively as:

	if (ret != -EPROBE_DEFER) {
		if (drv->probe)
			ret = drv->probe(dev);
		else
			ret = 0;

		if (ret)
			dev_pm_domain_detach(_dev, true);
	}

.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279650

SGVsbG8gVXdlLAoKdGhhbmtzIGZvciB5b3VyIHJldmlldy4KCj4gVGhpcyBtYXkgY2F1c2UgYSBw
YW5pYyBsYXRlci4gRm9yIGV4YW1wbGUsIGluc2VydGluZyB0aGUgdHBtX3Rpcwo+ID4gZHJpdmVy
IHdpdGggcGFyYW1ldGVyICJmb3JjZT0xIiAoaS5lLiByZWdpc3RlcmluZyB0cG1fdGlzIGFzIGEg
cGxhdGZvcm0KPiA+IGRyaXZlcikgd2lsbCBwYW5pYyBpbiB0cG1tX2NoaXBfYWxsb2MoKSBiZWNh
dXNlIGRldi0+ZHJpdmVyIGlzIE5VTEw6Cj4gPiAKPiA+ICAgICAgY2hpcC0+Y2Rldi5vd25lciA9
IGNoaXAtPnBkZXYtPmRyaXZlci0+b3duZXI7Cj4gCj4gVGhpcyBzb3VuZHMgbGlrZSBhIHNlcGFy
YXRlIGlzc3VlIHRob3VnaC4gTG9va2luZyBhdCBpbml0X3RpcyB0aGVyZSBpczoKPiAKPiAgICAg
ICAgIHJjID0gcGxhdGZvcm1fZHJpdmVyX3JlZ2lzdGVyKCZ0aXNfZHJ2KTsKPiAgICAgICAgIGlm
IChyYyA8IDApCj4gICAgICAgICAgICAgICAgIHJldHVybiByYzsKPiAgICAgICAgIHBkZXYgPSBw
bGF0Zm9ybV9kZXZpY2VfcmVnaXN0ZXJfc2ltcGxlKCJ0cG1fdGlzIiwgLTEsIE5VTEwsIDApOwo+
ICAgICAgICAgaWYgKElTX0VSUihwZGV2KSkgewo+ICAgICAgICAgICAgICAgICByYyA9IFBUUl9F
UlIocGRldik7Cj4gICAgICAgICAgICAgICAgIGdvdG8gZXJyX2RldjsKPiAgICAgICAgIH0KPiAg
ICAgICAgIHJjID0gdHBtX3Rpc19pbml0KCZwZGV2LT5kZXYsICZ0aXNfZGVmYXVsdF9pbmZvLCBO
VUxMKTsKPiAKPiB0cG1fdGlzX2luaXQgY2FsbHMgdHBtbV9jaGlwX2FsbG9jIHdoaWNoIGJhcmZz
IHdoZW4gcGRldiAoaS5lLiB0aGUgcmV0dXJuIHZhbHVlCj4gb2YgcGxhdGZvcm1fZGV2aWNlX3Jl
Z2lzdGVyX3NpbXBsZSBhYm92ZSkgaXNuJ3QgYm91bmQuIEl0IGlzIG5vdCBhbGxvd2VkCj4gdG8g
YXNzdW1lIHRoYXQgdGhlIGRldmljZSBpcyBib3VuZCBhZnRlciB0aGUgYWJvdmUgZnVuY3Rpb24g
Y2FsbHMuCgpJIGFncmVlIHRoYXQgdGhlIFRQTSBwbGF0Zm9ybSBkZXZpY2UgY29kZSBkZXNlcnZl
cyBpbXByb3ZlbWVudC4gSmFzb24Kd3JvdGUgdGhhdCBoZSBoYXMgYWxyZWFkeSBzb21lIHBhdGNo
ZXMgYXZhaWxhYmxlIGZvciB0aGF0LgoKSSBsYWNrIHRoZSBrbm93bGVkZ2UgdG8ganVkZ2Ugd2hl
dGhlciBvciBub3QgdHBtX2lzX2luaXQncyBhc3N1bXB0aW9uCndhcyBjb3JyZWN0LiBCdXQsIG1h
eWJlIGp1c3QgYnkgbHVjaywgdGhpcyBhc3N1bXB0aW9uIHVzZWQgdG8gYmUgKnRydWUqCnVudGls
IHBhdGNoIGI4YjJjN2Q4NDVkNS4gRHJpdmVyIGFuZCBkZXZpY2Ugd2VyZSBtYXRjaGVkIGJ5IG5h
bWUKKCJ0cG1fdGlzIikgYnkgdGhlIHBsYXRmb3JtIGRyaXZlciBwcm9iaW5nIGNvZGUsIGFuZCBk
ZXZpY2UgYW5kIGRyaXZlcgp3ZXJlIGFjdHVhbGx5IGJvdW5kIHRvIGVhY2ggb3RoZXIgYWZ0ZXIg
dGhpcyBzZXF1ZW5jZSBvZiBjYWxscy4gCgo+IFNvIEknZCBzYXkgZHJvcCB0aGUgcGFyYWdyYXBo
IGFib3V0IHRwbV90aXMgYW5kIHRoZSBjaGFuZ2UgaXMgZmluZS4KCkkgZGlkbid0IG1lYW4gdG8g
YmxhbWUgeW91ciBwYXRjaC4gQnV0IGEgbm90ZSBhYm91dCB0aGUgcGFuaWMgbWlnaHQgYmUKaGVs
cGZ1bCBqdXN0IGluIGNhc2Ugc29tZW9uZSBlbHNlIHJ1bnMgaW50byB0aGUgc2FtZSBwcm9ibGVt
LiBUaGUKY29ubmVjdGlvbiBiZXR3ZWVuIHlvdXIgcGF0Y2ggYW5kIHRwbV90aXMgbG9hZGluZyBp
cyBmYXIgZnJvbSBvYnZpb3VzLgpJIG1lbnRpb25lZCB0aGUgcGFuaWMgaW4gb3JkZXIgdG8gY2xh
cmlmeSB0aGF0IHRoaXMgd2Fzbid0IGp1c3QgYQp0aGVvcmV0aWNhbCBpc3N1ZS4KCkFueXdheSwg
SSdsbCByZXN1Ym1pdCB3aXRoIHlvdXIgc3R5bGUgaGludHMgYXBwbGllZCBhbmQgd2lsbCB0cnkg
dG8gZmluZAphIHdvcmRpbmcgZm9yIHRoZSBjb21taXQgbWVzc2FnZSB0aGF0IHdlIGNhbiBhZ3Jl
ZSB1cG9uLgoKQmVzdCBSZWdhcmRzLApNYXJ0aW4KCj4gCj4gPiBUaGlzIHBhdGNoIGZpeGVzIHRo
aXMgYnkgcmV0dXJuaW5nIHN1Y2Nlc3MgaW4gcGxhdGZvcm1fZHJ2X3Byb2JlKCkgaWYKPiA+ICJq
dXN0IiBkZXZfcG1fZG9tYWluX2F0dGFjaCgpIGhhZCBmYWlsZWQuIFRoaXMgcmVzdG9yZXMgdGhl
IHNlbWFudGljcwo+ID4gb2YgcGxhdGZvcm1fZGV2aWNlX3JlZ2lzdGVyX1hYWCgpIGlmIHRoZSBh
c3NvY2lhdGVkIHBsYXRmb3JtIGRyaXZlciBoYXMKPiA+IG5vICJwcm9iZSIgZnVuY3Rpb24uCj4g
PiAKPiA+IEZpeGVzOiBiOGIyYzdkODQ1ZDUgKCJiYXNlL3BsYXRmb3JtOiBhc3NlcnQgdGhhdCBk
ZXZfcG1fZG9tYWluCj4gPiBjYWxsYmFja3MgYXJlIGNhbGxlZCB1bmNvbmRpdGlvbmFsbHkiKQo+
ID4gCj4gCj4gSSB0aGluayBsaW5lIGJyZWFrcyBpbiB0aGUgRml4ZXM6IGxpbmUgYXJlIGZyb3du
ZWQgb24uIEFsc28gdXN1YWxseQo+IHRoZXJlIGlzIG5vIGVtcHR5IGxpbmUgYmV0d2VlbiBGaXhl
czogYW5kIFMtby1iOi4KPiAKPiA+IFNpZ25lZC1vZmYtYnk6IE1hcnRpbiBXaWxjayA8TWFydGlu
LldpbGNrQHRzLmZ1aml0c3UuY29tPgo+ID4gLS0tCj4gPiAgZHJpdmVycy9iYXNlL3BsYXRmb3Jt
LmMgfCAxMiArKysrKysrKy0tLS0KPiA+ICAxIGZpbGUgY2hhbmdlZCwgOCBpbnNlcnRpb25zKCsp
LCA0IGRlbGV0aW9ucygtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9iYXNlL3BsYXRm
b3JtLmMgYi9kcml2ZXJzL2Jhc2UvcGxhdGZvcm0uYwo+ID4gaW5kZXggMWRkNmQzYi4uYzk5NGU3
NiAxMDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvYmFzZS9wbGF0Zm9ybS5jCj4gPiArKysgYi9kcml2
ZXJzL2Jhc2UvcGxhdGZvcm0uYwo+ID4gQEAgLTUxMywxMCArNTEzLDE0IEBAIHN0YXRpYyBpbnQg
cGxhdGZvcm1fZHJ2X3Byb2JlKHN0cnVjdCBkZXZpY2UgKl9kZXYpCj4gPiAgCQlyZXR1cm4gcmV0
Owo+ID4gIAo+ID4gIAlyZXQgPSBkZXZfcG1fZG9tYWluX2F0dGFjaChfZGV2LCB0cnVlKTsKPiA+
IC0JaWYgKHJldCAhPSAtRVBST0JFX0RFRkVSICYmIGRydi0+cHJvYmUpIHsKPiA+IC0JCXJldCA9
IGRydi0+cHJvYmUoZGV2KTsKPiA+IC0JCWlmIChyZXQpCj4gPiAtCQkJZGV2X3BtX2RvbWFpbl9k
ZXRhY2goX2RldiwgdHJ1ZSk7Cj4gPiArCWlmIChyZXQgIT0gLUVQUk9CRV9ERUZFUikgewo+ID4g
KwkJaWYgKGRydi0+cHJvYmUpIHsKPiA+ICsJCQlyZXQgPSBkcnYtPnByb2JlKGRldik7Cj4gPiAr
CQkJaWYgKHJldCkKPiA+ICsJCQkJZGV2X3BtX2RvbWFpbl9kZXRhY2goX2RldiwgdHJ1ZSk7Cj4g
PiArCQl9IGVsc2UKPiA+ICsJCQkvKiBkb24ndCBmYWlsIGlmIGp1c3QgZGV2X3BtX2RvbWFpbl9h
dHRhY2ggZmFpbGVkICovCj4gPiArCQkJcmV0ID0gMDsKPiAKPiBBbiBlbHNlIHRoYXQgaGFzIGEg
fSBzaG91bGQgYWxzbyBoYXZlIGEgeywgYWNjb3JkaW5nIHRvIAo+IGNoZWNrcGF0Y2ggYW5kIERv
Y3VtZW50YXRpb24vQ29kaW5nU3R5bGUuIFlvdSBjYW4gd3JpdGUgaXQKPiBhbHRlcm5hdGl2ZWx5
IGFzOgo+IAo+IAlpZiAocmV0ICE9IC1FUFJPQkVfREVGRVIpIHsKPiAJCWlmIChkcnYtPnByb2Jl
KQo+IAkJCXJldCA9IGRydi0+cHJvYmUoZGV2KTsKPiAJCWVsc2UKPiAJCQlyZXQgPSAwOwo+IAo+
IAkJaWYgKHJldCkKPiAJCQlkZXZfcG1fZG9tYWluX2RldGFjaChfZGV2LCB0cnVlKTsKPiAJfQo+
IAo+IC4KPiAKPiBCZXN0IHJlZ2FyZHMKPiBVd2UKPiAK
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279802 — [PATCH v2] base/platform: return success when probe function is NULL

From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

Since b8b2c7d845d5, platform_drv_probe() is called for all platform
devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
platform_drv_probe() will return the error code from dev_pm_domain_attach().

This causes real_probe() to enter the "probe_failed" path and set
dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
success if both dev->bus->probe and drv->probe were missing. As a result,
a device and driver could be "bound" together just by matching their names;
this doesn't work any more after b8b2c7d845d5.

This may cause problems later for certain usage of platform_driver_register()
and platform_device_register_simple(). I observed a panic while loading
the tpm_tis driver with parameter "force=1" (i.e. registering tpm_tis as
a platform driver), because tpm_tis_init's assumption that the device
returned by platform_device_register_simple() was bound didn't hold any more
(tpmm_chip_alloc() dereferences chip->pdev->driver, causing panic).

This patch restores the previous (4.3.0 and earlier) behavior of
platform_drv_probe() in the case when the associated platform driver has
no "probe" function.

v2: fixed style issues, rephrased commit message.

Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally")
Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
---
 drivers/base/platform.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 1dd6d3b..176b59f 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -513,10 +513,15 @@ static int platform_drv_probe(struct device *_dev)
 		return ret;
 
 	ret = dev_pm_domain_attach(_dev, true);
-	if (ret != -EPROBE_DEFER && drv->probe) {
-		ret = drv->probe(dev);
-		if (ret)
-			dev_pm_domain_detach(_dev, true);
+	if (ret != -EPROBE_DEFER) {
+		if (drv->probe) {
+			ret = drv->probe(dev);
+			if (ret)
+				dev_pm_domain_detach(_dev, true);
+		} else {
+			/* don't fail if just dev_pm_domain_attach failed */
+			ret = 0;
+		}
 	}
 
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281188 — Re: [PATCH v2] base/platform: return success when probe function is NULL

Hello,

On Mon, Nov 30, 2015 at 12:50:05PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> platform_drv_probe() will return the error code from dev_pm_domain_attach().
> 
> This causes real_probe() to enter the "probe_failed" path and set
> dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> success if both dev->bus->probe and drv->probe were missing. As a result,
> a device and driver could be "bound" together just by matching their names;
> this doesn't work any more after b8b2c7d845d5.
> 
> This may cause problems later for certain usage of platform_driver_register()
> and platform_device_register_simple(). I observed a panic while loading
> the tpm_tis driver with parameter "force=1" (i.e. registering tpm_tis as
> a platform driver), because tpm_tis_init's assumption that the device
> returned by platform_device_register_simple() was bound didn't hold any more
> (tpmm_chip_alloc() dereferences chip->pdev->driver, causing panic).

I'm a bit uncertain if I should be happy with this wording or not. While
b8b2c7d845d5 has a bug which made the tpm_tis driver provoke a panic and
which needs fixing, this doesn't mean that the tpm_tis driver was right.

So "This may cause problems later ..." isn't a correct justification
because even in the presence of the bug introduced by b8b2c7d845d5, the
tpm_tis driver must not assume that it's device is bound on return of
platform_device_register_simple.

It's just that the bug in b8b2c7d845d5 made a wrong assumption of the
tpm_tis driver obvious, and now both need fixing.

If this were my patch, I'd not talk about tpm_tis at all. b8b2c7d845d5
broke binding for platform drivers with no probe function in certain
situations. This is corrected here.

> This patch restores the previous (4.3.0 and earlier) behavior of
> platform_drv_probe() in the case when the associated platform driver has
> no "probe" function.
> 
> v2: fixed style issues, rephrased commit message.

This must go after the triple-dash below to not be included in the
commit.

> Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain callbacks are called unconditionally")
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> ---

Best regards
Uwe
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280754

SGVsbG8gVXdlLAoKPiBUaGlzIHNvdW5kcyBsaWtlIGEgc2VwYXJhdGUgaXNzdWUgdGhvdWdoLiBM
b29raW5nIGF0IGluaXRfdGlzIHRoZXJlIGlzOgo+IAo+ICAgICAgICAgcmMgPSBwbGF0Zm9ybV9k
cml2ZXJfcmVnaXN0ZXIoJnRpc19kcnYpOwo+ICAgICAgICAgaWYgKHJjIDwgMCkKPiAgICAgICAg
ICAgICAgICAgcmV0dXJuIHJjOwo+ICAgICAgICAgcGRldiA9IHBsYXRmb3JtX2RldmljZV9yZWdp
c3Rlcl9zaW1wbGUoInRwbV90aXMiLCAtMSwgTlVMTCwgMCk7Cj4gICAgICAgICBpZiAoSVNfRVJS
KHBkZXYpKSB7Cj4gICAgICAgICAgICAgICAgIHJjID0gUFRSX0VSUihwZGV2KTsKPiAgICAgICAg
ICAgICAgICAgZ290byBlcnJfZGV2Owo+ICAgICAgICAgfQo+ICAgICAgICAgcmMgPSB0cG1fdGlz
X2luaXQoJnBkZXYtPmRldiwgJnRpc19kZWZhdWx0X2luZm8sIE5VTEwpOwo+IAo+IHRwbV90aXNf
aW5pdCBjYWxscyB0cG1tX2NoaXBfYWxsb2Mgd2hpY2ggYmFyZnMgd2hlbiBwZGV2IChpLmUuIHRo
ZSByZXR1cm4gdmFsdWUKPiBvZiBwbGF0Zm9ybV9kZXZpY2VfcmVnaXN0ZXJfc2ltcGxlIGFib3Zl
KSBpc24ndCBib3VuZC4gSXQgaXMgbm90IGFsbG93ZWQKPiB0byBhc3N1bWUgdGhhdCB0aGUgZGV2
aWNlIGlzIGJvdW5kIGFmdGVyIHRoZSBhYm92ZSBmdW5jdGlvbiBjYWxscy4KCkNhbiB5b3UgcGxl
YXNlIGV4cGxhaW4gYWdhaW4gd2h5IHlvdSB0aGluayB0aGF0IGFzc3VtcHRpb24gaXMgaW52YWxp
ZD8gCkFzIGZhciBhcyBJIHVuZGVyc3RhbmQgdGhlIGNvZGUsIHRoZSBhc3N1bXB0aW9uIHdvdWxk
IGJlIGNvcnJlY3QgaW4KNC4zLjAgYW5kIGVhcmxpZXI6CgpwbGF0Zm9ybV9kcml2ZXJfcmVnaXN0
ZXIoKSByZWdpc3RlcnMgYSBwbGF0Zm9ybSBkcml2ZXIgd2l0aCBuYW1lCiJ0cG1fdGlzIi4gcGxh
dGZvcm1fZGV2aWNlX3JlZ2lzdGVyX3NpbXBsZSgpIHJlZ2lzdGVycyBhIGRldmljZSB3aXRoIHRo
ZQpzYW1lIG5hbWUuIFRoaXMgd2lsbCBjYWxsIHBsYXRmb3JtX2RldmljZV9hZGQoKS9kZXZpY2Vf
YWRkKCkgYW5kIHN0YXJ0CnByb2JpbmcgZm9yIGEgcGxhdGZvcm0gZGV2aWNlLiBQbGF0Zm9ybSBi
dXMgcHJvYmluZyBpbiBwbGF0Zm9ybV9tYXRjaCgpCmZhbGxzIGJhY2sgdG8gYSBzaW1wbGUgbWF0
Y2ggYmV0d2VlbiBkcml2ZXIgYW5kIGRldmljZSBuYW1lIGlmIGFsbCBlbHNlCmZhaWxzLiBUaGF0
IG1hdGNoIHN1Y2NlZWRzIGZvciB0aGUgInRwbV90aXMiIGRyaXZlci4gVGh1cwpkcml2ZXJfcHJv
YmVfZGV2aWNlKCkgd2lsbCBiZSBjYWxsZWQsIGFuZCBpbiB0aGUgYWJzZW5jZSBvZiBhCmRyaXZl
ci1zcGVjaWZpYyBwcm9iZSByb3V0aW5lLCB3aWxsIHN1Y2NlZWQuIFRodXMgYWZ0ZXIKcGxhdGZv
cm1fZGV2aWNlX3JlZ2lzdGVyX3NpbXBsZSgpIHJldHVybnMsIGRldmljZSBhbmQgZHJpdmVyIHdp
bGwgYmUKYm91bmQuIFRoaXMgbWF0Y2hlcyBhbHNvIGFjdHVhbCBiZWhhdmlvciBvZiB0aGUgcHJl
LTQuNCBjb2RlLiAKClBsZWFzZSBleHBsYWluIHdoYXQgSSBhbSBvdmVybG9va2luZy4gSSBhbSBq
dXN0IHRyeWluZyB0byB1bmRlcnN0YW5kLgpBcyBmYXIgYXMgdHBtX3RpcyBpcyBjb25jZXJuZWQs
IEphc29uJ3MgY3VycmVudCBwYXRjaCBzZXQgaXMgZ29pbmcgdG8KZml4IHRoaXMgZm9yIGdvb2Qg
YW55d2F5LgoKUmVnYXJkcwpNYXJ0aW4KCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280863

Hello Martin,

On Tue, Dec 01, 2015 at 11:41:53AM +0100, Wilck, Martin wrote:
> > This sounds like a separate issue though. Looking at init_tis there is:
> > 
> >         rc = platform_driver_register(&tis_drv);
> >         if (rc < 0)
> >                 return rc;
> >         pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
> >         if (IS_ERR(pdev)) {
> >                 rc = PTR_ERR(pdev);
> >                 goto err_dev;
> >         }
> >         rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
> > 
> > tpm_tis_init calls tpmm_chip_alloc which barfs when pdev (i.e. the return value
> > of platform_device_register_simple above) isn't bound. It is not allowed
> > to assume that the device is bound after the above function calls.
> 
> Can you please explain again why you think that assumption is invalid? 

You can unbind a device from a driver via sysfs, you can also prevent
binding somehow I think, probing can fail for different reasons, probing
might wait for userspace interaction to load firmware which wasn't
scheduled yet. I'm sure there are still more things that break the
assumption.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1280939

PiA+ID4gdHBtX3Rpc19pbml0IGNhbGxzIHRwbW1fY2hpcF9hbGxvYyB3aGljaCBiYXJmcyB3aGVu
IHBkZXYgKGkuZS4gdGhlIHJldHVybiB2YWx1ZQo+ID4gPiBvZiBwbGF0Zm9ybV9kZXZpY2VfcmVn
aXN0ZXJfc2ltcGxlIGFib3ZlKSBpc24ndCBib3VuZC4gSXQgaXMgbm90IGFsbG93ZWQKPiA+ID4g
dG8gYXNzdW1lIHRoYXQgdGhlIGRldmljZSBpcyBib3VuZCBhZnRlciB0aGUgYWJvdmUgZnVuY3Rp
b24gY2FsbHMuCj4gPiAKPiA+IENhbiB5b3UgcGxlYXNlIGV4cGxhaW4gYWdhaW4gd2h5IHlvdSB0
aGluayB0aGF0IGFzc3VtcHRpb24gaXMgaW52YWxpZD8gCj4gCj4gWW91IGNhbiB1bmJpbmQgYSBk
ZXZpY2UgZnJvbSBhIGRyaXZlciB2aWEgc3lzZnMsIHlvdSBjYW4gYWxzbyBwcmV2ZW50Cj4gYmlu
ZGluZyBzb21laG93IEkgdGhpbmssIHByb2JpbmcgY2FuIGZhaWwgZm9yIGRpZmZlcmVudCByZWFz
b25zLCBwcm9iaW5nCj4gbWlnaHQgd2FpdCBmb3IgdXNlcnNwYWNlIGludGVyYWN0aW9uIHRvIGxv
YWQgZmlybXdhcmUgd2hpY2ggd2Fzbid0Cj4gc2NoZWR1bGVkIHlldC4gSSdtIHN1cmUgdGhlcmUg
YXJlIHN0aWxsIG1vcmUgdGhpbmdzIHRoYXQgYnJlYWsgdGhlCj4gYXNzdW1wdGlvbi4KClRoYW5r
cy4gT3V0IG9mIHRoZXNlLCAicHJldmVudCBiaW5kaW5nIHNvbWVob3ciIHdvdWxkIGJlIHRoZSBv
bmx5CnByb2JsZW0gdGhhdCBhcHBsaWVzIHRvIHRwbV90aXMsIGFzIHByb2JpbmcgY2FuJ3QgZmFp
bCAobm8gcHJvYmUoKQpyb3V0aW5lKSwgdGhlcmUncyBubyBGVyB0byBsb2FkLCBhbmQgdW5iaW5k
aW5nIHZpYSBzeXNmcyB3b3VsZCByZXF1aXJlCm5lYXJseSBpbXBvc3NpYmxlIHRpbWluZyAobm90
IHN1cmUgaWYgaXQgY291bGQgYmUgZG9uZSB3aXRoIHVkZXYpLgoKQW55d2F5LCB0aGUgUmlnaHQg
VGhpbmcgdG8gZG8gaXMgdG8gY3JlYXRlIGEgcHJvYmUoKSByb3V0aW5lIGFuZCB0aGF0J3MKd2hh
dCBKYXNvbiBkaWQuCgpUaGFua3MgYWdhaW4sCk1hcnRpbgoK
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281071 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Tue, Dec 01, 2015 at 04:19:25PM +0100, Wilck, Martin wrote:
> > > > tpm_tis_init calls tpmm_chip_alloc which barfs when pdev (i.e. the return value
> > > > of platform_device_register_simple above) isn't bound. It is not allowed
> > > > to assume that the device is bound after the above function calls.
> > > 
> > > Can you please explain again why you think that assumption is invalid? 
> > 
> > You can unbind a device from a driver via sysfs, you can also prevent
> > binding somehow I think, probing can fail for different reasons, probing
> > might wait for userspace interaction to load firmware which wasn't
> > scheduled yet. I'm sure there are still more things that break the
> > assumption.
> 
> Thanks. Out of these, "prevent binding somehow" would be the only
> problem that applies to tpm_tis, as probing can't fail (no probe()
> routine), there's no FW to load, and unbinding via sysfs would require
> nearly impossible timing (not sure if it could be done with udev).
> 
> Anyway, the Right Thing to do is to create a probe() routine and that's
> what Jason did.

That fixes tpm_tis, but there are other ancient TPM drivers that use
the old, now broken way.

So, we still need to do something here. Either fixup b8b2c7d845d5 as
you have proposed, remove the now broken obsolete TPM drivers, or try
and fix them..

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281108 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

Am 1. Dezember 2015 09:25:37 PST, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
>On Tue, Dec 01, 2015 at 04:19:25PM +0100, Wilck, Martin wrote:
>> > > > tpm_tis_init calls tpmm_chip_alloc which barfs when pdev (i.e.
>the return value
>> > > > of platform_device_register_simple above) isn't bound. It is
>not allowed
>> > > > to assume that the device is bound after the above function
>calls.
>> > > 
>> > > Can you please explain again why you think that assumption is
>invalid? 
>> > 
>> > You can unbind a device from a driver via sysfs, you can also
>prevent
>> > binding somehow I think, probing can fail for different reasons,
>probing
>> > might wait for userspace interaction to load firmware which wasn't
>> > scheduled yet. I'm sure there are still more things that break the
>> > assumption.
>> 
>> Thanks. Out of these, "prevent binding somehow" would be the only
>> problem that applies to tpm_tis, as probing can't fail (no probe()
>> routine), there's no FW to load, and unbinding via sysfs would
>require
>> nearly impossible timing (not sure if it could be done with udev).
>> 
>> Anyway, the Right Thing to do is to create a probe() routine and
>that's
>> what Jason did.
>
>That fixes tpm_tis, but there are other ancient TPM drivers that use
>the old, now broken way.
>
>So, we still need to do something here. Either fixup b8b2c7d845d5 as
>you have proposed, remove the now broken obsolete TPM drivers, or try
>and fix them..

How broken are they and since when?

I thought multiple times about deprecating and finally removing the 1.1b stuff - tpm 1.2 is out for 10? years now? With an expected life span of a TPM of roughly 5years... 
And also unfortunately the 1.1b legacy drivers usually get loaded first :( (atleast for slb9635)

Mark them as obsolete , default them to No and remove them by 4.10 if there are no objections?

Peter
-- 
Sent from my mobile
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281111 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Tue, Dec 01, 2015 at 10:26:17AM -0800, Peter Huewe wrote:
> 
> 
> Am 1. Dezember 2015 09:25:37 PST, schrieb Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
> >On Tue, Dec 01, 2015 at 04:19:25PM +0100, Wilck, Martin wrote:
> >> > > > tpm_tis_init calls tpmm_chip_alloc which barfs when pdev (i.e.
> >the return value
> >> > > > of platform_device_register_simple above) isn't bound. It is
> >not allowed
> >> > > > to assume that the device is bound after the above function
> >calls.
> >> > > 
> >> > > Can you please explain again why you think that assumption is
> >invalid? 
> >> > 
> >> > You can unbind a device from a driver via sysfs, you can also
> >prevent
> >> > binding somehow I think, probing can fail for different reasons,
> >probing
> >> > might wait for userspace interaction to load firmware which wasn't
> >> > scheduled yet. I'm sure there are still more things that break the
> >> > assumption.
> >> 
> >> Thanks. Out of these, "prevent binding somehow" would be the only
> >> problem that applies to tpm_tis, as probing can't fail (no probe()
> >> routine), there's no FW to load, and unbinding via sysfs would
> >require
> >> nearly impossible timing (not sure if it could be done with udev).
> >> 
> >> Anyway, the Right Thing to do is to create a probe() routine and
> >that's
> >> what Jason did.
> >
> >That fixes tpm_tis, but there are other ancient TPM drivers that use
> >the old, now broken way.
> >
> >So, we still need to do something here. Either fixup b8b2c7d845d5 as
> >you have proposed, remove the now broken obsolete TPM drivers, or try
> >and fix them..
> 
> How broken are they and since when?

oops the kernel broken, since 4.4-rc1 apparently, so not released yet.

> Mark them as obsolete , default them to No and remove them by 4.10
> if there are no objections?

Greg KH has been advising just to delete stuff right away. It is easy
to undelete something if someone comes around with hardware and is
willing to test

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281122 — Aw: Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

> > >That fixes tpm_tis, but there are other ancient TPM drivers that use
> > >the old, now broken way.
> > >
> > >So, we still need to do something here. Either fixup b8b2c7d845d5 as
> > >you have proposed, remove the now broken obsolete TPM drivers, or try
> > >and fix them..
> >
> > How broken are they and since when?

> oops the kernel broken, since 4.4-rc1 apparently, so not released yet.
damn, I was hoping MUCH longer :)

> > Mark them as obsolete , default them to No and remove them by 4.10
> > if there are no objections?

> Greg KH has been advising just to delete stuff right away. It is easy
> to undelete something if someone comes around with hardware and is
> willing to test

Can you point to that discussion or was it offline?
I mean for staging, yes there it is clear, but for stuff that is officially "upstream", I'm not 100% sure.

Thanks,
Peter


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281131 — Re: Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Tue, Dec 01, 2015 at 07:54:59PM +0100, Peter Huewe wrote:
> Can you point to that discussion or was it offline?

Apparently it was brought up at the most recent kernel summit

http://www.spinics.net/lists/linux-rdma/msg29985.html

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1279173 — Re: [tpmdd-devel] [PATCH] base/platform: fix panic when probe function is NULL

On Thu, Nov 26, 2015 at 08:01:34PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> Since b8b2c7d845d5, platform_drv_probe() is called for all platform
> devices. If drv->probe is NULL, and dev_pm_domain_attach() fails,
> platform_drv_probe() will return the error code from dev_pm_domain_attach().
> 
> This causes real_probe() to enter the "probe_failed" path and set
> dev->driver to NULL. Before b8b2c7d845d5, real_probe() would assume
> success if both dev->bus->probe and drv->probe are missing.
> 
> This may cause a panic later. For example, inserting the tpm_tis
> driver with parameter "force=1" (i.e. registering tpm_tis as a platform
> driver) will panic in tpmm_chip_alloc() because dev->driver is NULL:
> 
>      chip->cdev.owner = chip->pdev->driver->owner;
> 
> This patch fixes this by returning success in platform_drv_probe() if
> "just" dev_pm_domain_attach() had failed. This restores the semantics
> of platform_device_register_XXX() if the associated platform driver has
> no "probe" function.
> 
> Fixes: b8b2c7d845d5 ("base/platform: assert that dev_pm_domain
> callbacks are called unconditionally")
> 
> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

> ---
>  drivers/base/platform.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 1dd6d3b..c994e76 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -513,10 +513,14 @@ static int platform_drv_probe(struct device *_dev)
>  		return ret;
>  
>  	ret = dev_pm_domain_attach(_dev, true);
> -	if (ret != -EPROBE_DEFER && drv->probe) {
> -		ret = drv->probe(dev);
> -		if (ret)
> -			dev_pm_domain_detach(_dev, true);
> +	if (ret != -EPROBE_DEFER) {
> +		if (drv->probe) {
> +			ret = drv->probe(dev);
> +			if (ret)
> +				dev_pm_domain_detach(_dev, true);
> +		} else
> +			/* don't fail if just dev_pm_domain_attach failed */
> +			ret = 0;
>  	}
>  
>  	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> -- 
> 1.8.3.1
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web