[PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Pawel Moll]

The host devices without a parent were "forcefully adopted"
by platform bus. This patch removes this assignment. In
effect the dev_dev may be NULL now, which means ISA.

Cc: James E.J. Bottomley <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---

This patch is a part of effort to remove references to platform_bus
and make it static.

James, could you please have a look and advice if the change is
correct? Would you happen to know the "real reasons" behind
using the root platform_bus device a parent?

 drivers/scsi/hosts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 3cbb57a..0c7389f 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -218,7 +218,7 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 
 	if (!shost->shost_gendev.parent)
-		shost->shost_gendev.parent = dev ? dev : &platform_bus;
+		shost->shost_gendev.parent = dev;
 	if (!dma_dev)
 		dma_dev = shost->shost_gendev.parent;
 
-- 
1.9.1

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[James Bottomley]

On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> The host devices without a parent were "forcefully adopted"
> by platform bus. This patch removes this assignment. In
> effect the dev_dev may be NULL now, which means ISA.
> 
> Cc: James E.J. Bottomley <JBottomley@parallels.com>
> Cc: linux-scsi@vger.kernel.org
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
> 
> This patch is a part of effort to remove references to platform_bus
> and make it static.
> 
> James, could you please have a look and advice if the change is
> correct? Would you happen to know the "real reasons" behind
> using the root platform_bus device a parent?

Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
in the DMA transfers if it is.  A lot of the legacy ISA device on x86
and I thought some ARM SOC devices don't pass in the parent device, so
we hang them off a known parent.

You can grep for it; these are the devices that will begin to panic if
you apply this patch:

arch/ia64/hp/sim/simscsi.c:     error = scsi_add_host(host, NULL);
drivers/scsi/a2091.c:   error = scsi_add_host(instance, NULL);
drivers/scsi/a3000.c:   error = scsi_add_host(instance, NULL);
drivers/scsi/aha152x.c: if( scsi_add_host(shpnt, NULL) ) {
drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
drivers/scsi/gvp11.c:   error = scsi_add_host(instance, NULL);
drivers/scsi/imm.c:     err = scsi_add_host(host, NULL);
drivers/scsi/pcmcia/fdomain_stub.c:    if (scsi_add_host(host, NULL))
drivers/scsi/pcmcia/nsp_cs.c:   ret = scsi_add_host (host, NULL);
drivers/scsi/pcmcia/qlogic_stub.c:      if (scsi_add_host(shost, NULL))
drivers/scsi/pcmcia/sym53c500_cs.c:     if (scsi_add_host(host, NULL))
drivers/scsi/ppa.c:     err = scsi_add_host(host, NULL);
drivers/scsi/qlogicfas.c:       if (scsi_add_host(hreg, NULL))
drivers/scsi/scsi_module.c:             error = scsi_add_host(shost, NULL);
drivers/scsi/sgiwd93.c: err = scsi_add_host(host, NULL);

Note I've picked up scsi_module, so anything that uses the SCSI module
interface also has this problem.

James

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Pawel Moll]

On Fri, 2014-07-25 at 15:46 +0100, James Bottomley wrote:
> On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > The host devices without a parent were "forcefully adopted"
> > by platform bus. This patch removes this assignment. In
> > effect the dev_dev may be NULL now, which means ISA.
> > 
> > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > Cc: linux-scsi@vger.kernel.org
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> > 
> > This patch is a part of effort to remove references to platform_bus
> > and make it static.
> > 
> > James, could you please have a look and advice if the change is
> > correct? Would you happen to know the "real reasons" behind
> > using the root platform_bus device a parent?
> 
> Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> in the DMA transfers if it is.  

That's what I though at the beginning as well, but then I crawled
through get_dma_ops(struct device *dev) and it seems that on most
architectures (all but two, if I remember correctly) it will return a
default set of DMA ops if the dev == NULL, eg.

static inline struct dma_map_ops *get_dma_ops(struct device *dev)
{
#ifndef CONFIG_X86_DEV_DMA_OPS
        return dma_ops;
#else
        if (unlikely(!dev) || !dev->archdata.dma_ops)
                return dma_ops;
        else
                return dev->archdata.dma_ops;
#endif 
}

in arch/x86/include/asm/dma-mapping.h. Now, there seem to be only a
handful of places where dev_dma is dereferenced: scsi_dma_map and
scsi_dma_unmap in drivers/scsi/scsi_lib_dma.c, where all the calls seem
to be "NULL resistant" and __scsi_alloc_queue in __scsi_alloc_queue
which will oops as you said.

Anyway, if you are saying that dev_dma must not be NULL at any
circumstances, I'll either have to find some kind of replacement for
platform_bus or convince Greg that platform_bus must not be made
static ;-)

> A lot of the legacy ISA device on x86
> and I thought some ARM SOC devices don't pass in the parent device, so
> we hang them off a known parent.

That's another thing I'm not sure - once assigned, is the dma_dev
related to the parent in any way? Even more - is the shost_gendev.parent
used at all? Doesn't seem to be. If it's only about a place in the
device model hierarchy, leaving parent as NULL will make such device a
virtual one, which it probably should be...

Paweł


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Greg Kroah-Hartman]

On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote:
> On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > The host devices without a parent were "forcefully adopted"
> > by platform bus. This patch removes this assignment. In
> > effect the dev_dev may be NULL now, which means ISA.
> > 
> > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > Cc: linux-scsi@vger.kernel.org
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > ---
> > 
> > This patch is a part of effort to remove references to platform_bus
> > and make it static.
> > 
> > James, could you please have a look and advice if the change is
> > correct? Would you happen to know the "real reasons" behind
> > using the root platform_bus device a parent?
> 
> Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> and I thought some ARM SOC devices don't pass in the parent device, so
> we hang them off a known parent.

The "generic" platform bus device is not a "known parent".  I don't
understand the difference between just setting the parent to be NULL,
which will then have a "proper" parent pointer filled in by the driver
core when the device is registered, or faking it out here.  What is the
difference?

In the end, the device always ends up with a parent pointer, right?

thanks,

greg k-h

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[James Bottomley]

On Sat, 2014-07-26 at 13:11 -0700, Greg Kroah-Hartman wrote:
> On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote:
> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > > The host devices without a parent were "forcefully adopted"
> > > by platform bus. This patch removes this assignment. In
> > > effect the dev_dev may be NULL now, which means ISA.
> > > 
> > > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > > Cc: linux-scsi@vger.kernel.org
> > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > ---
> > > 
> > > This patch is a part of effort to remove references to platform_bus
> > > and make it static.
> > > 
> > > James, could you please have a look and advice if the change is
> > > correct? Would you happen to know the "real reasons" behind
> > > using the root platform_bus device a parent?
> > 
> > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> > in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> > and I thought some ARM SOC devices don't pass in the parent device, so
> > we hang them off a known parent.
> 
> The "generic" platform bus device is not a "known parent".  I don't
> understand the difference between just setting the parent to be NULL,
> which will then have a "proper" parent pointer filled in by the driver
> core when the device is registered, or faking it out here.  What is the
> difference?

If you set the parent to NULL, the host template dma_dev will end up
NULL as well and that will trigger a NULL deref panic in the dma segment
routines.

If you want to remove platform_bus, we have to have a well known device
to set dma_dev to at scsi_host_add time.

> In the end, the device always ends up with a parent pointer, right?

The parent pointer isn't the problem ... assigning the correct dma
device is.

James

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Greg Kroah-Hartman]

On Sun, Jul 27, 2014 at 07:52:57AM +0400, James Bottomley wrote:
> On Sat, 2014-07-26 at 13:11 -0700, Greg Kroah-Hartman wrote:
> > On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote:
> > > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote:
> > > > The host devices without a parent were "forcefully adopted"
> > > > by platform bus. This patch removes this assignment. In
> > > > effect the dev_dev may be NULL now, which means ISA.
> > > > 
> > > > Cc: James E.J. Bottomley <JBottomley@parallels.com>
> > > > Cc: linux-scsi@vger.kernel.org
> > > > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> > > > ---
> > > > 
> > > > This patch is a part of effort to remove references to platform_bus
> > > > and make it static.
> > > > 
> > > > James, could you please have a look and advice if the change is
> > > > correct? Would you happen to know the "real reasons" behind
> > > > using the root platform_bus device a parent?
> > > 
> > > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> > > in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> > > and I thought some ARM SOC devices don't pass in the parent device, so
> > > we hang them off a known parent.
> > 
> > The "generic" platform bus device is not a "known parent".  I don't
> > understand the difference between just setting the parent to be NULL,
> > which will then have a "proper" parent pointer filled in by the driver
> > core when the device is registered, or faking it out here.  What is the
> > difference?
> 
> If you set the parent to NULL, the host template dma_dev will end up
> NULL as well and that will trigger a NULL deref panic in the dma segment
> routines.
>
> If you want to remove platform_bus, we have to have a well known device
> to set dma_dev to at scsi_host_add time.

Why not set the dma_dev after you call device_add()?  That way you will
pick up the right parent no matter what.

> > In the end, the device always ends up with a parent pointer, right?
> 
> The parent pointer isn't the problem ... assigning the correct dma
> device is.

Ah, ok, it's a scsi core thing, not a driver core thing, that's less
confusing now.  For a "fallback" of a platform device, if you switch the
lines around you should be fine, something like this patch perhaps:

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 3cbb57a8b846..d8d3b294f5bc 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -218,16 +218,16 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 
 	if (!shost->shost_gendev.parent)
-		shost->shost_gendev.parent = dev ? dev : &platform_bus;
-	if (!dma_dev)
-		dma_dev = shost->shost_gendev.parent;
-
-	shost->dma_dev = dma_dev;
+		shost->shost_gendev.parent = dev;
 
 	error = device_add(&shost->shost_gendev);
 	if (error)
 		goto out;
 
+	if (!dma_dev)
+		dma_dev = shost->shost_gendev.parent;
+	shost->dma_dev = dma_dev;
+
 	pm_runtime_set_active(&shost->shost_gendev);
 	pm_runtime_enable(&shost->shost_gendev);
 	device_enable_async_suspend(&shost->shost_gendev);

Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent

[Pawel Moll]

On Sun, 2014-07-27 at 16:07 +0100, Greg Kroah-Hartman wrote:
> Ah, ok, it's a scsi core thing, not a driver core thing, that's less
> confusing now.  For a "fallback" of a platform device, if you switch the
> lines around you should be fine, something like this patch perhaps:
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 3cbb57a8b846..d8d3b294f5bc 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -218,16 +218,16 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  		goto fail;
>  
>  	if (!shost->shost_gendev.parent)
> -		shost->shost_gendev.parent = dev ? dev : &platform_bus;
> -	if (!dma_dev)
> -		dma_dev = shost->shost_gendev.parent;
> -
> -	shost->dma_dev = dma_dev;
> +		shost->shost_gendev.parent = dev;
>  
>  	error = device_add(&shost->shost_gendev);
>  	if (error)
>  		goto out;
>  
> +	if (!dma_dev)
> +		dma_dev = shost->shost_gendev.parent;
> +	shost->dma_dev = dma_dev;
> +
>  	pm_runtime_set_active(&shost->shost_gendev);
>  	pm_runtime_enable(&shost->shost_gendev);
>  	device_enable_async_suspend(&shost->shost_gendev);

But this will still make shost->dma_dev == NULL in the cases James
quoted:

On Fri, 2014-07-25 at 15:46 +0100, James Bottomley wrote:
> Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic
> in the DMA transfers if it is.  A lot of the legacy ISA device on x86
> and I thought some ARM SOC devices don't pass in the parent device, so
> we hang them off a known parent.
> 
> You can grep for it; these are the devices that will begin to panic if
> you apply this patch:
> 
> arch/ia64/hp/sim/simscsi.c:     error = scsi_add_host(host, NULL);
> drivers/scsi/a2091.c:   error = scsi_add_host(instance, NULL);
> drivers/scsi/a3000.c:   error = scsi_add_host(instance, NULL);
> drivers/scsi/aha152x.c: if( scsi_add_host(shpnt, NULL) ) {
> drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
> drivers/scsi/gdth.c:    error = scsi_add_host(shp, NULL);
> drivers/scsi/gvp11.c:   error = scsi_add_host(instance, NULL);
> drivers/scsi/imm.c:     err = scsi_add_host(host, NULL);
> drivers/scsi/pcmcia/fdomain_stub.c:    if (scsi_add_host(host, NULL))
> drivers/scsi/pcmcia/nsp_cs.c:   ret = scsi_add_host (host, NULL);
> drivers/scsi/pcmcia/qlogic_stub.c:      if (scsi_add_host(shost, NULL))
> drivers/scsi/pcmcia/sym53c500_cs.c:     if (scsi_add_host(host, NULL))
> drivers/scsi/ppa.c:     err = scsi_add_host(host, NULL);
> drivers/scsi/qlogicfas.c:       if (scsi_add_host(hreg, NULL))
> drivers/scsi/scsi_module.c:             error = scsi_add_host(shost, NULL);
> drivers/scsi/sgiwd93.c: err = scsi_add_host(host, NULL);

Maybe the DMA API should be coping with NULL device? It seems to handle
it in a half of the methods and fails in the other half...

Pawel