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
J.E.J. Bottomley [[email protected]] 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
[email protected]
[email protected] 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
J.E.J. Bottomley [[email protected]] 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 <><
[email protected] 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
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 <><
[email protected] 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
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 <><
[email protected] 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
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 <><
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
[email protected] 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