Re: [RFC][PATCH] move dma_mask into struct device

Re: [RFC][PATCH] move dma_mask into struct device

[Arnd Bergmann]

J.E.J. Bottomley [James.Bottomley@steeleye.com] wrote:

> Attached is a patch which moves dma_mask into struct device and cleans up the 
> scsi mid-layer to use it (instead of using struct pci_dev).  The advantage to 
> doing this is probably most apparent on non-pci bus architectures where 
> currently you have to construct a fake pci_dev just so you can get the bounce 
> buffers to work correctly.

That does not sound like the right way to me. If you need to have the dma_mask
for the Scsi_Host, you should store it in Scsi_Host itself. A struct device
must never know about obscure architecture specific stuff like dma.

	Arnd <><

Re: [RFC][PATCH] move dma_mask into struct device

[J.E.J. Bottomley]

arndb@de.ibm.com said:
> That does not sound like the right way to me. If you need to have the
> dma_mask for the Scsi_Host, you should store it in Scsi_Host itself. A
> struct device must never know about obscure architecture specific
> stuff like dma.

The SCSI host itself has no need of a DMA mask.  What we need the mask for is 
to set up the bounce limits for the block queue, we don't actually ever use it 
again.  Unfortunately, dma_mask isn't architecture specific, its a universal 
property of the block queues used to determine when to bounce memory regions.

The dma_mask is a property of the connection of the Scsi_Host to the machine 
bus, not of the Scsi_Host itself, so it does properly belong in the generic 
device which is used to reflect machine bus attachments.

Think of it this way: we have two struct device's per SCSI host: one for the 
actual HBA card or bus attachment, which contains all of the bus specific 
pieces, and one for the host itself reflecting the fact that it is a bridge 
from the machine bus to the scsi bus.

James

Re: [RFC][PATCH] move dma_mask into struct device

[Arnd Bergmann]

On Saturday 16 November 2002 16:33, J.E.J. Bottomley wrote:

> The SCSI host itself has no need of a DMA mask.  What we need the mask for
> is to set up the bounce limits for the block queue, we don't actually ever
> use it again.  Unfortunately, dma_mask isn't architecture specific, its a
> universal property of the block queues used to determine when to bounce
> memory regions.

On my s390 system, I can have many thousand devices and none of them is
doing DMA, so I would indeed call it architecture specific. Note that 
even in a normal PC system, most devices (e.g. CPUs, input devices or
the disks attached to the host adapter) don't have any concept of
DMA.

> The dma_mask is a property of the connection of the Scsi_Host to the
> machine bus, not of the Scsi_Host itself, so it does properly belong in the
> generic device which is used to reflect machine bus attachments.
Maybe, but if you put dma_mask in struct device, you are making it a property
of every single device in the system.

> Think of it this way: we have two struct device's per SCSI host: one for
> the actual HBA card or bus attachment, which contains all of the bus
> specific pieces, and one for the host itself reflecting the fact that it is
> a bridge from the machine bus to the scsi bus.
That does not sound right either, but I don't care so much about this because
you are not messing with my devices here. The problem is having two 'struct
device's for one physical device. The Scsi_Host should really be the
*driver_data of the bus devices, with the scsi devices being direct children
of that device.

	Arnd <><

Re: [RFC][PATCH] move dma_mask into struct device

[J.E.J. Bottomley]

arndb@de.ibm.com said:
> On my s390 system, I can have many thousand devices and none of them
> is doing DMA, so I would indeed call it architecture specific. Note
> that  even in a normal PC system, most devices (e.g. CPUs, input
> devices or the disks attached to the host adapter) don't have any
> concept of DMA. 

DMA itself is pervasive to almost every architecture, that's why we have the 
DMA API.  That some devices don't do DMA, I agree with (the struct scsi_device 
is another example).  However, in order to divorce DMA from the PCI bus, it 
has to be obtainable from the generic device, without requiring knowledge of 
the bus.  In OO terms, it would be in a dmaable_device which inherits from 
device, but for expediency in layering all this into the kernel means I'd have 
to break almost every driver and introduce them to the concept of 
dmaable_device, so it's just easier to expand device by a pointer.

James

Re: [RFC][PATCH] move dma_mask into struct device

[Arnd Bergmann]

On Saturday 16 November 2002 18:01, J.E.J. Bottomley wrote:

> However, in order to divorce DMA from the
> PCI bus, it has to be obtainable from the generic device, without requiring
> knowledge of the bus.  In OO terms, it would be in a dmaable_device which
> inherits from device, but for expediency in layering all this into the
> kernel means I'd have to break almost every driver and introduce them to
> the concept of
> dmaable_device, so it's just easier to expand device by a pointer.

The Scsi_Host is already derived from struct device and since you need the 
dma_mask only for Scsi_Host, there is no need to expand the base class.

You can easily keep out the pci stuff if you do something like
this:

static inline void scsi_set_device(struct Scsi_Host *shost,
                                   struct device *dev)
{
        shost->dev = dev;
        shost->host_driverfs_dev.parent = dev;
}

static inline void scsi_set_pci_device(struct Scsi_Host *shost,
                                       struct pci_dev *pdev)
{
	scsi_set_device(shost, &pdev->dev);
	shost->dma_mask = pdev->dma_mask;
}

static inline void scsi_set_foobus_device(struct Scsi_Host *shost,
					  struct foo_dev *fdev)
{
	scsi_set_device(shost, fdev->dev);
	shost->dma_mask = fdev->foo_dma_mask;
}

You can even avoid Scsi_Host::dev completely if you make its only
remaining user (scsi_ioctl_get_pci) use host_sysfs_dev->parent.bus_id
instead.

	Arnd <><

Re: [RFC][PATCH] move dma_mask into struct device

[J.E.J. Bottomley]

arndb@de.ibm.com said:
> You can easily keep out the pci stuff if you do something like this: 

No...look at what you've done.  Now SCSI has to know about every bus type on 
every architecture; that's an extreme layering violation.  architecture/bus 
types are generally only defined for the arch (PCI being the exception), so 
now the additions have to be #ifdef'd just so it will compile..

You've done this because you effectively have to pull a common but differently 
located structure element out of each of these bus specific devices.  That 
implies to me that dma_mask should be in a common structure, which was the 
whole basis for the dmaable_device that I outlined previously.  As I said, the 
only reason I haven't implemented dmaable_device is for expediency.

James

Re: [RFC][PATCH] move dma_mask into struct device

[Arnd Bergmann]

On Saturday 16 November 2002 19:12, J.E.J. Bottomley wrote:

> No...look at what you've done.  Now SCSI has to know about every bus type
> on every architecture; that's an extreme layering violation. 
> architecture/bus types are generally only defined for the arch (PCI being
> the exception), so now the additions have to be #ifdef'd just so it will
> compile..

Right, the definitions for how to get the dma_mask out of a bus specific
device don't belong into the generic header file.
Still, each host driver knows how to find the dma_mask if any, so
it can easily set the field in the Scsi_Host. Existing pci host
adapter drivers can keep using scsi_set_pci_device(), others
can just as well do it themselves.

	Arnd <><

[RFC][PATCH] move dma_mask into struct device

[J.E.J. Bottomley]

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

Attached is a patch which moves dma_mask into struct device and cleans up the 
scsi mid-layer to use it (instead of using struct pci_dev).  The advantage to 
doing this is probably most apparent on non-pci bus architectures where 
currently you have to construct a fake pci_dev just so you can get the bounce 
buffers to work correctly.

The patch tries to perturb the minimum amount of code, so dma_mask in struct 
device is simply a pointer to the one in pci_dev.  However, it will make it 
easy for me now to add generic device to MCA without having to go the fake pci 
route.

This patch completely removes knowledge of pci devices from the SCSI mid-layer.

I have compiled and tested this, but obviously, since I have an MCA machine, 
it's not of much value to the pci code changes, so if someone with a PCI 
machine could check those out, I'd be grateful.

The main problem SCSI has with this is the scsi_ioctl_get_pci which is used to 
get the pci slot name.  Although, this can be fixed up afterwards with Matthew 
Wilcox's name change patch.

I'd like to see this as the beginning of a move away from bus specific code to 
using generic device code (where possible).  Comments and feedback welcome.

James


[-- Attachment #2: tmp.diff --]
[-- Type: text/plain , Size: 3432 bytes --]

===== drivers/pci/probe.c 1.17 vs edited =====
--- 1.17/drivers/pci/probe.c	Fri Nov  1 12:33:02 2002
+++ edited/drivers/pci/probe.c	Fri Nov 15 14:00:46 2002
@@ -449,6 +449,7 @@
 	/* now put in global tree */
 	strcpy(dev->dev.name,dev->name);
 	strcpy(dev->dev.bus_id,dev->slot_name);
+	dev->dev->dma_mask = &dev->dma_mask;
 
 	device_register(&dev->dev);
 	return dev;
===== drivers/scsi/hosts.h 1.36 vs edited =====
--- 1.36/drivers/scsi/hosts.h	Thu Nov 14 13:07:27 2002
+++ edited/drivers/scsi/hosts.h	Fri Nov 15 14:43:59 2002
@@ -468,10 +468,10 @@
     unsigned int max_host_blocked;
 
     /*
-     * For SCSI hosts which are PCI devices, set pci_dev so that
-     * we can do BIOS EDD 3.0 mappings
+     * This is a pointer to the generic device for this host (i.e. the
+     * device on the bus);
      */
-    struct pci_dev *pci_dev;
+    struct device *dev;
 
     /* 
      * Support for driverfs filesystem
@@ -521,11 +521,17 @@
 	shost->host_lock = lock;
 }
 
+static inline void scsi_set_device(struct Scsi_Host *shost,
+                                   struct device *dev)
+{
+        shost->dev = dev;
+        shost->host_driverfs_dev.parent = dev;
+}
+
 static inline void scsi_set_pci_device(struct Scsi_Host *shost,
                                        struct pci_dev *pdev)
 {
-	shost->pci_dev = pdev;
-	shost->host_driverfs_dev.parent=&pdev->dev;
+        scsi_set_device(shost, &pdev->dev);
 }
 
 
===== drivers/scsi/scsi_ioctl.c 1.12 vs edited =====
--- 1.12/drivers/scsi/scsi_ioctl.c	Thu Oct 17 13:52:39 2002
+++ edited/drivers/scsi/scsi_ioctl.c	Fri Nov 15 14:08:19 2002
@@ -396,9 +396,9 @@
 scsi_ioctl_get_pci(Scsi_Device * dev, void *arg)
 {
 
-        if (!dev->host->pci_dev) return -ENXIO;
-        return copy_to_user(arg, dev->host->pci_dev->slot_name,
-                            sizeof(dev->host->pci_dev->slot_name));
+        if (!dev->host->dev) return -ENXIO;
+        return copy_to_user(arg, dev->host->dev->name,
+                            sizeof(dev->host->dev->name));
 }
 
 
===== drivers/scsi/scsi_scan.c 1.35 vs edited =====
--- 1.35/drivers/scsi/scsi_scan.c	Thu Nov 14 12:34:35 2002
+++ edited/drivers/scsi/scsi_scan.c	Fri Nov 15 14:25:41 2002
@@ -436,8 +436,8 @@
 	u64 bounce_limit;
 
 	if (sh->highmem_io) {
-		if (sh->pci_dev && PCI_DMA_BUS_IS_PHYS) {
-			bounce_limit = sh->pci_dev->dma_mask;
+		if (sh->dev && sh->dev->dma_mask && PCI_DMA_BUS_IS_PHYS) {
+			bounce_limit = *sh->dev->dma_mask;
 		} else {
 			/*
 			 * Platforms with virtual-DMA translation
===== drivers/scsi/st.c 1.42 vs edited =====
--- 1.42/drivers/scsi/st.c	Mon Nov 11 03:32:34 2002
+++ edited/drivers/scsi/st.c	Fri Nov 15 14:25:58 2002
@@ -3786,8 +3786,8 @@
 			 * hardware have no practical limit.
 			 */
 			bounce_limit = BLK_BOUNCE_ANY;
-		else if (SDp->host->pci_dev)
-			bounce_limit = SDp->host->pci_dev->dma_mask;
+		else if (SDp->host->dev && SDp->host->dev->dma_mask)
+			bounce_limit = *SDp->host->dev->dma_mask;
 	} else if (SDp->host->unchecked_isa_dma)
 		bounce_limit = BLK_BOUNCE_ISA;
 	bounce_limit >>= PAGE_SHIFT;
===== include/linux/device.h 1.58 vs edited =====
--- 1.58/include/linux/device.h	Thu Oct 31 15:25:58 2002
+++ edited/include/linux/device.h	Fri Nov 15 13:52:55 2002
@@ -300,6 +300,7 @@
 					   being off. */
 
 	unsigned char *saved_state;	/* saved device state */
+	u64		*dma_mask;	/* dma mask (if dma'able device) */
 
 	void	(*release)(struct device * dev);
 };

Re: [RFC][PATCH] move dma_mask into struct device

[Mike Anderson]

J.E.J. Bottomley [James.Bottomley@steeleye.com] wrote:

> ===== drivers/pci/probe.c 1.17 vs edited =====
> --- 1.17/drivers/pci/probe.c	Fri Nov  1 12:33:02 2002
> +++ edited/drivers/pci/probe.c	Fri Nov 15 14:00:46 2002
> @@ -449,6 +449,7 @@
>  	/* now put in global tree */
>  	strcpy(dev->dev.name,dev->name);
>  	strcpy(dev->dev.bus_id,dev->slot_name);
> +	dev->dev->dma_mask = &dev->dma_mask;

I got a compile error here. This should be.
	dev->dev.dma_mask = &dev->dma_mask;

I did not have a current bk handy on the lab machine, but I ran it on
a 2.5.47 view with a few offset warnings.

The machine is a 2x pci systems with the following drivers loaded:
	aic7xxx, ips, qlogicisp.

It just booted and ran a little IO.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [RFC][PATCH] move dma_mask into struct device

[J.E.J. Bottomley]

andmike@us.ibm.com said:
> I got a compile error here. This should be.
> 	dev->dev.dma_mask = &dev->dma_mask; 

Oops, yes, that's what comes of making changes in code you don't compile.

> The machine is a 2x pci systems with the following drivers loaded:
> 	aic7xxx, ips, qlogicisp. 

Thanks for testing it.

James

Re: [RFC][PATCH] move dma_mask into struct device

[Patrick Mansfield]

On Fri, Nov 15, 2002 at 03:34:12PM -0500, J.E.J. Bottomley wrote:
> --- 1.36/drivers/scsi/hosts.h	Thu Nov 14 13:07:27 2002
> +++ edited/drivers/scsi/hosts.h	Fri Nov 15 14:43:59 2002
> @@ -468,10 +468,10 @@
>      unsigned int max_host_blocked;
>  
>      /*
> -     * For SCSI hosts which are PCI devices, set pci_dev so that
> -     * we can do BIOS EDD 3.0 mappings
> +     * This is a pointer to the generic device for this host (i.e. the
> +     * device on the bus);
>       */
> -    struct pci_dev *pci_dev;
> +    struct device *dev;
>  
>      /* 
>       * Support for driverfs filesystem
> @@ -521,11 +521,17 @@
>  	shost->host_lock = lock;
>  }
>  
> +static inline void scsi_set_device(struct Scsi_Host *shost,
> +                                   struct device *dev)
> +{
> +        shost->dev = dev;
> +        shost->host_driverfs_dev.parent = dev;
> +}
> +
>  static inline void scsi_set_pci_device(struct Scsi_Host *shost,
>                                         struct pci_dev *pdev)
>  {
> -	shost->pci_dev = pdev;
> -	shost->host_driverfs_dev.parent=&pdev->dev;
> +        scsi_set_device(shost, &pdev->dev);
>  }

Can shost->host_driverfs_dev.parent be used instead of adding and
using a duplicate shost->dev?

-- Patrick Mansfield

Re: [RFC][PATCH] move dma_mask into struct device

[J.E.J. Bottomley]

patmans@us.ibm.com said:
> Can shost->host_driverfs_dev.parent be used instead of adding and
> using a duplicate shost->dev? 

I think so. I believe the parent is always the device we're looking for.  I'll 
make the fix.

James