| Message ID | 1406298233-27876-4-git-send-email-pawel.moll@arm.com |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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ł -- 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/
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 -- 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/
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 -- 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/
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-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/
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;
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(-)