Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent
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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent
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 jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org Signed-off-by: Pawel Moll pawel.m...@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); -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent
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 jbottom...@parallels.com Cc: linux-scsi@vger.kernel.org Signed-off-by: Pawel Moll pawel.m...@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 -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent
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 jbottom...@parallels.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Pawel Moll pawel.m...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] [SCSI] Do not use platform_bus as a parent
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 jbottom...@parallels.com
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Pawel Moll pawel.m...@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ł
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
