Re: Promise FastTrak TX4000 (pdc20619), hdparm support for libata, smart
From: Jeff Garzik <hidden>
Date: 2004-10-21 23:35:49
Tobias Lorenz wrote: (copied to linux-ide@vger.kernel.org, where the ATA hackers hang out)
based on linux-2.6.9 + your 2.6.9-libata1-dev1.patch, we made a patch to support the Promise FastTrak TX4000 4-port PATA hardware raid controller. It is very similar to the sata_promise.c driver, so we added the support there. We also started to support information tranfer via the hd_driveid structure to the hdparm utility in the libata layer. At the moment, we only transfer cylinders, sectors, heads, model and firmware version. Our intention was to display the supported and the used UDMA mode(s), but we didn't find the correct structures yet to get these infos... Finally, we tried to use the smart utilities. That's also the reason, we we choose your development patch. We found out, that smartctl from the smartsuite package, uses scsi-3 smart commands, when trying to access a scsi disk device (/dev/sd?). That makes smart access with that utility not working. Then we used ide-smart from the ide-smart package. It always uses ide smart commands, also to scsi disk devices, and that makes it working. I hope you include that patch and it get's added to the linux kernel. I would be happy to see my name somewhere there. :-)
I would be happy to facilitate that... libata contributors are always welcome. ;-)
Todo is just to change/complete the description of the driver and to add us to the Changelog/Credits/History/... section(s).
This is pretty good stuff. I would request that you work with me a bit, as I would like you to make a few minor changes. Comments: 1) [administrivia] Please submit patches to linux-ide@vger.kernel.org and jgarzik@pobox.com email addresses. 2) [administrivia] There is a standard email format for patches, which makes it easier to merge your Linux kernel changes, that I request you use. Details: http://linux.yyz.us/patch-format.html http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt In particular we request a "signed-off-by: ..." line accompany each patch and patch description.
quoted hunk
--------------------------------------------------------------------------- linux-2.6.9/drivers/scsi/libata-scsi.c 2004-10-20 22:07:50.000000000 +0200 +++ linux-2.6.7-patched/drivers/scsi/libata-scsi.c 2004-10-20 19:25:15.000000000 +0200@@ -215,6 +215,17 @@ struct ata_port *ap; struct ata_device *dev; int val = -EINVAL, rc = -EINVAL; + struct hd_driveid drv_id = { + .cyls = 0, + .sectors = 0, + .heads = 0, + .fw_rev = "", + .model = "", + .cur_cyls = 0, + .cur_heads = 0, + .cur_sectors = 0, + }; + int geom[3]; ap = (struct ata_port *) &scsidev->host->hostdata[0];
if (!ap)
quoted hunk
@@ -249,7 +260,25 @@ return -EACCES; return ata_task_ioctl(scsidev, arg); + case HDIO_GET_MULTCOUNT: + printk("HDIO_GET_MULTCOUNT not yet implemented\n"); + rc = -EOPNOTSUPP; + break; + + case HDIO_GET_IDENTITY: + ata_std_bios_param(scsidev, NULL, dev->n_sectors, geom); + drv_id.cur_heads = drv_id.heads = geom[0]; + drv_id.cur_sectors = drv_id.sectors = geom[1]; + drv_id.cur_cyls = drv_id.cyls = geom[2]; + strncpy((char *) &drv_id.model, scsidev->model, sizeof(drv_id.model)); + strncpy((char *) &drv_id.fw_rev, scsidev->rev, sizeof(drv_id.fw_rev)); + if(copy_to_user((char *) arg, (char *) &drv_id, + sizeof(drv_id))) + return(-EFAULT); + return 0; + default: + printk("command was: %#x\n", cmd); rc = -EOPNOTSUPP; break;
3) Since the default return value is EOPNOTSUPP, I would rather not include a "not implemented yet" implementation :) So, please remove the HDIO_GET_MULTCOUNT part of the patch. 4) Also, per the "split up logical changes" standard, please send this change in a separate patch (and separate email).
-static struct pci_device_id pdc_sata_pci_tbl[] = {
- { PCI_VENDOR_ID_PROMISE, 0x3371, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+static struct pci_device_id pdc_ata_pci_tbl[] = {
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20371, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
board_2037x },
- { PCI_VENDOR_ID_PROMISE, 0x3373, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20373, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
board_2037x },
- { PCI_VENDOR_ID_PROMISE, 0x3375, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20375, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
board_2037x },
- { PCI_VENDOR_ID_PROMISE, 0x3376, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20376, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
board_2037x },
- { PCI_VENDOR_ID_PROMISE, 0x3318, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20318, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
board_20319 },
- { PCI_VENDOR_ID_PROMISE, 0x3319, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20319, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
board_20319 },
+ { PCI_VENDOR_ID_PROMISE, PCI_DEVICE_ID_PROMISE_20619, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ board_20619 },5) [style] While your patch _is_ 100% correct, my own personal feeling is that PCI device ids have very little value as constants in include/linux/pci_ids.h. Therefore, I actually prefer to use the hexidecimal constant rather than a named constant. 6) [summary] Overall, I do not see any bugs or problems in your sata_promise patch. However... I need to ponder a "human" question... does it make sense to add support for a PATA controller to a module named 'sata_promise'? I wish to avoid code duplication of course, but this is an issue I would like to address. Perhaps we could apply your patch, then rename the kernel module to 'ata_promise'. [we can use the MODULE_ALIAS facility to smooth the user transition] Jeff