move dma_mask into struct device
Quoted 4 lines · [email protected] said:
That does not sound like the right way to me. If you need to have the
dma_maskfor theScsi_Host, you should store it inScsi_Hostitself. 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
Quoted 5 lines · J.E.J. Bottomley
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_maskisn'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.
Quoted 3 lines
The
dma_maskis a property of the connection of theScsi_Hostto the machine bus, not of theScsi_Hostitself, 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.
Quoted 4 lines
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 <><
Quoted 5 lines · [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
Quoted 7 lines · J.E.J. Bottomley
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 admaable_devicewhich 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 ofdmaable_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 <><
Quoted 1 line · [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
Quoted 5 lines · J.E.J. Bottomley
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 <><