IBM Power RAID driver (ipr)

http://www-124.ibm.com/storageio/ipr/patch-2.6.1-ipr-2.0.0-1.gz

+static int ipr_unsafe = 1; /* xxx set to 0 for ship */

Hi, I took a look and I have some comments / questions. I will try to go over all of it in the next few days, but here's what I went over so far:

Quoted 1 line

+static int ipr_unsafe = 1; /* xxx set to 0 for ship */

Should it be set to 0 or to 1 for inclusion in the kernel? any driver that is first included is considered experimental, so just how unsafe is ipr_unsafe?

Review

Quoted 6 lines

(quoted diff) +6 -0

+static void ipr_sleep_no_lock(signed long delay)
+{
+	DECLARE_WAIT_QUEUE_HEAD(internal_wait);
+
+	sleep_on_timeout(&internal_wait, (delay * HZ) / 1000);
+	return;

just a nitpick, but the Linux coding style says no 'return' from void functions.

Review

Quoted 5 lines

(quoted diff) +5 -0

+static void ipr_sleep(struct ipr_ioa_cfg *ioa_cfg, signed long delay)
+{
+	spin_unlock_irq(ioa_cfg->host->host_lock);
+	ipr_sleep_no_lock(delay);
+	spin_lock_irq(ioa_cfg->host->host_lock);

Hmpf... it's called from ipr_diagnostic_ioctl(), which does spin_lock_irqsave()
ipr_sleep()
spin_unlock_irqrestore()

So I guess you really do intend to enable interrupts before going to sleep? it's somewhat fragile... also, is the dropping and reacquisition of the lock safe here (in the sense that none of the data the lock is protecting that we care about can change while the lock is not held?)

Same questions wrt ipr_interruptible_sleep_on(), which does the same thing. By the way, ipr_dump_ioctl does not check the return value from ipr_interruptible_sleep_on(). If this is intentional, perhaps call it using (void)ipr_interruptible_sleep_on()?

Review

Quoted 6 lines

(quoted diff) +6 -0

+static void ipr_block_requests(struct ipr_ioa_cfg *ioa_cfg)
+{
+	ENTER;
+
+	if (0 == ioa_cfg->block_host_ops++) {
+	   ioa_cfg->allow_cmds = 0;

This is racy unless a lock is held. I think a lock is held here in all callers of this function, but a comment to that effect would be nice before the function. This also applies to every other function which either must be called with a lock taken, or modifies the "locking status" - takes a lock which is released elsewhere, enables interrupts unconditionally, etc.

(quoted diff) +9 -0

+static void ipr_unblock_requests(struct ipr_ioa_cfg *ioa_cfg)
+{
+	ENTER;
+
+	if (0 == --ioa_cfg->block_host_ops) {
+	   ioa_cfg->allow_cmds = 1;
+	   spin_unlock_irq(ioa_cfg->host->host_lock);
+	   scsi_unblock_requests(ioa_cfg->host);
+	   spin_lock_irq(ioa_cfg->host->host_lock);

Eeek! What happens if on another CPU a call comes in that does ipr_block_requests() after the lock is released and before it is taken again?

I have just written a LLD for the IBM Power RAID family of adapters. This includes several IBM iSeries and pSeries SCSI adapters. Please review for 2.6 inclusion.

- Since ipr_sleep_no_lock/ipr_sleep is used once, we can open code a schedule_timeout there instead.

- Add a comment to explain why the wakeup strategy isnt racy. I automatically thought it was (just like the kernel version of sleep_on) until I could verify all wake ups are done under the scsi host lock. I also have a trivial change to make the two sleep functions alike.

- Im a bit worried about memory barriers wrt the allow_* things. As an example I added some mb()s wherever ->allow_interrupts is set.

Actually, do we need this flag at all? Cant we just set the bits on the card to disable interrupts, then do a synchronize_irq()? Similarly it would be nice to get rid of some of the other allow_ flags if we can get around them.

- The forcing of the pci cacheline size worries me, could we have problems with enabling MWI etc after doing this?

- Do we need the extra ipr_set_pcix_cmd_reg during init? We are going to do it after a reset arent we?

- It looks like the kmalloc in ipr_sdt_copy is called from process context without any locks held, so we dont need an atomic allocation.

Hi,

I have just written a LLD for the IBM Power RAID family of adapters. This includes several IBM iSeries and pSeries SCSI adapters. Please review for 2.6 inclusion.

(quoted diff) +14 -0

+/**
+ * ipr_version_show - Show the driver version
+ * @dd:	device driver struct
+ * @buf:	buffer
+ * 
+ * Return value:
+ * 	number of bytes printed to buffer
+ **/
+static ssize_t ipr_version_show(struct device_driver *dd, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n", IPR_DRIVER_VERSION);
+}
+
+static DRIVER_ATTR(version, S_IRUGO, ipr_version_show, NULL);

Please don't do this in sysfs. We're still hoping for a MODULE_VERSION macro, but every driver crafting it's own version telling mechanism doesn't scale.

This is my fault. We recommended not to include procfs info in this driver and pointed to sysfs. Do we have a timeline on MODULE_VERSION?

Quoted 15 lines · Christoph Hellwig [[email protected]] wrote:

(quoted diff) +14 -0

+/**
+ * ipr_version_show - Show the driver version
+ * @dd:	device driver struct
+ * @buf:	buffer
+ * 
+ * Return value:
+ * 	number of bytes printed to buffer
+ **/
+static ssize_t ipr_version_show(struct device_driver *dd, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n", IPR_DRIVER_VERSION);
+}
+
+static DRIVER_ATTR(version, S_IRUGO, ipr_version_show, NULL);

Quoted 4 lines

Please don't do this in sysfs. We're still hoping for a MODULE_VERSION macro, but every driver crafting it's own version telling mechanism doesn't scale.

This is my fault. We recommended not to include procfs info in this driver and pointed to sysfs. Do we have a timeline on MODULE_VERSION?

... and will MODULE_VERSION be defined when the driver is built in?

+#include <asm/processor.h>

Why do you need this one?

(quoted diff) +3 -0

+#ifdef CONFIG_COMPAT
+#include <linux/ioctl32.h>
+#endif

Shouldn't hurt to include this unconditionally.

A single source-file driver shouldn't ever have non-static entry points.

You have lots of forward-declarations. This is usually a sign of a bad source file structure as code reads much easier if functions called by another function are above them in the source. Also there's some other strangeness in the code, e.g. modern drivers usually have the module_init/exit routines at the very end, before that the pci driver methods and objects, then the scsi host template and before it it's methods.

(quoted diff) +14 -0

+/**
+ * ipr_version_show - Show the driver version
+ * @dd:	device driver struct
+ * @buf:	buffer
+ * 
+ * Return value:
+ * 	number of bytes printed to buffer
+ **/
+static ssize_t ipr_version_show(struct device_driver *dd, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n", IPR_DRIVER_VERSION);
+}
+
+static DRIVER_ATTR(version, S_IRUGO, ipr_version_show, NULL);

Please don't do this in sysfs. We're still hoping for a MODULE_VERSION macro, but every driver crafting it's own version telling mechanism doesn't scale.

What about a MODULE_DESCRIPTION?

(quoted diff) +16 -0

+unsigned int ipr_ioctls[] =
+{
+	IPR_IOCTL_PASSTHRU,
+	IPR_IOCTL_RUN_DIAGNOSTICS,
+	IPR_IOCTL_DUMP_IOA,
+	IPR_IOCTL_RESET_IOA,
+	IPR_IOCTL_READ_DRIVER_CFG,
+	IPR_IOCTL_WRITE_DRIVER_CFG,
+	IPR_IOCTL_GET_BUS_CAPABILTIES,
+	IPR_IOCTL_SET_BUS_ATTRIBUTES,
+	IPR_IOCTL_GET_TRACE,
+	IPR_IOCTL_RECLAIM_CACHE,
+	IPR_IOCTL_QUERY_CONFIGURATION,
+	IPR_IOCTL_UCODE_DOWNLOAD,
+	IPR_IOCTL_CHANGE_ADAPTER_ASSIGNMENT
+};

Please provide a brief explanation of every ioctl and why you need it as ioctl. At least the GET_BUS_CAPABILTIES / SET_BUS_ATTRIBUTES seem to be better expressed in the transport sysfs API discussed on linux-scsi

(quoted diff) +11 -0

+/**
+ * ipr_get_mode_page - Locate specified mode page
+ * @mode_pages:	mode page buffer
+ * @page_code:	page code to find
+ * @len:		minimum required length for mode page
+ *
+ * Return value:
+ * 	pointer to mode page / NULL on failure
+ **/
+static void *ipr_get_mode_page(struct ipr_mode_pages *mode_pages,
+			       u32 page_code, u32 len)

Okay, this isn't good at all. A LLDD driver shouldn't care about mode pages and all the stuff you do with it. Please do it from userspace using the sg driver.

(quoted diff) +2 -0

+	spin_lock_init(&ipr_driver_lock);
+	INIT_LIST_HEAD(&ipr_ioa_head);

These could be initialized at compile-time (but shouldn't be needed at all without the character devices)

(quoted diff) +2 -0

+	ipr_regs = (unsigned long)ioremap(ipr_regs_pci,
+					  pci_resource_len(pdev, 0));

You need to check the return value.

(quoted diff) +18 -0

+/**
+ * ipr_scan_vsets - Scans for VSET devices
+ * @ioa_cfg:	ioa config struct
+ *
+ * Description: Since the VSET resources do not follow SAM in that we can have
+ * sparse LUNs with no LUN 0, we have to scan for these ourselves.
+ *
+ * Return value:
+ * 	none
+ **/
+static void ipr_scan_vsets(struct ipr_ioa_cfg *ioa_cfg)
+{
+	int target, lun;
+
+	for (target = 0; target < IPR_MAX_NUM_TARGETS_PER_BUS; target++)
+		for (lun = 0; lun < IPR_MAX_NUM_VSET_LUNS_PER_TARGET; lun++ )
+			scsi_add_device(ioa_cfg->host, IPR_VSET_BUS, target, lun);
+}

Can you explain what these vsets are supposed to do? And why a scsi_scan quirk can't be done instead of doing this in a LLDD?

(quoted diff) +11 -0

+/**
+ * ipr_inquiry - Send an Inquiry to the adapter.
+ * @ipr_cmd:	ipr command struct
+ *
+ * This utility function sends an inquiry to the adapter.
+ *
+ * Return value:
+ * 	none
+ **/
+static void ipr_inquiry(struct ipr_cmnd *ipr_cmd, u8 flags, u8 page,
+			u32 dma_addr, u8 xfer_len)

Doing inquiries from a LLDD looks very, very wrong. Explanations please.

+/**
+ * ipr_find_ses_entry - Find matching SES in SES table
+ * @res: resource entry struct of SES
+ *
+ * Return value:
+ * pointer to SES table entry / NULL on failure
+ **/

This is an extreme layering violation. Any SES handling belongs into an upper level scsi driver (or userspace via sg)

+/**
+ * ipr_do_req - Send driver initiated requests.
+ * @ipr_cmd: ipr command struct
+ * @done: done function
+ * @timeout_func: timeout function
+ * @timeout: timeout value
+ *
+ * This function sends the specified command to the adapter with the
+ * timeout given. The done function is invoked on command completion.

This looks very wrong. And fortunately it seems to be only called from code that is totally misplaced in a LLDD anyway. All scsi commands should go through the midlayer queueing.

+/**
+ * ipr_vset_stop_unit - Send a STOP UNIT to a VSET resource
+ * @ioa_cfg: ioa config struct
+ * @res: resource entry struct
+ *
+ * This function sends a STOP UNIT to a VSET resource to
+ * flush the write cache..
+ *
+ * Return value:
+ * IOASC of the command
+ **/

Again, a LLDD is the very wrong place for something like that. Is there a spec for that VSET stuff somewhere?

(quoted diff) +23 -0

+/**
+ * ipr_send_cmd - Build and send a mid-layer command
+ * @ioa_cfg:	ioa config struct
+ * @scsi_cmd:	scsi command struct
+ * @res:		ipr resource entry
+ *
+ * Return value:
+ * 	0 on success / SCSI_MLQUEUE_HOST_BUSY if DMA mapping fails
+ **/
+static int ipr_send_cmd(struct ipr_ioa_cfg *ioa_cfg,
+			struct scsi_cmnd *scsi_cmd,
+			struct ipr_resource_entry *res)
+{
+	struct ipr_cmnd *ipr_cmd;
+	int rc = 0;
+	void (*timeout_func) (struct scsi_cmnd *);
+	int timeout;
+	struct ipr_ioarcb *ioarcb;
+
+	ipr_cmd = ipr_get_free_ipr_cmnd(ioa_cfg);
+	ioarcb = &ipr_cmd->ioarcb;
+
+	list_add_tail(&ipr_cmd->queue, &ioa_cfg->pending_q);

Can you explain the per-command list mess please? In theory a properly written driver shouldn't need lists like that, just start a command in ->queuecommand if possible, else return an error and immediately call ->done when you're done with a scsi command.

(quoted diff) +8 -0

+	/*
+	 * Double the timeout value to use as we will use the adapter
+	 * as the primary timing mechanism
+	 */
+	timeout_func = (void (*)(struct scsi_cmnd *)) scsi_cmd->eh_timeout.function;
+	timeout = scsi_cmd->timeout_per_command;
+
+	scsi_add_timer(scsi_cmd, timeout * IPR_TIMEOUT_MULTIPLIER, timeout_func);

Okay, another driver that would like hooks into the EH timer. We should probabl better fix this correctly by adding a host method for it.

+ **/
+int ipr_queue(struct scsi_cmnd *scsi_cmd, void (*done) (struct scsi_cmnd *))

ipr_queuecommand would be a better name.

(quoted diff) +8 -0

+
+	/* xxx can I remove some of these checks? */
+	if (unlikely(ioa_cfg->ioa_is_dead || !res || res->del_from_ml)) {
+		memset(scsi_cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		scsi_cmd->result = (DID_NO_CONNECT << 16);
+		scsi_cmd->scsi_done(scsi_cmd);
+		return 0;
+	}

this should be handled by the midlayer per-device and per-host state bits.

(quoted diff) +1 -0

+	rc = ipr_send_cmd(ioa_cfg, scsi_cmd, res);

merge this into the quecommand routine?

(quoted diff) +10 -0

+int ipr_biosparam(struct scsi_device *scsi_device,
+		  struct block_device *block_device,
+		  sector_t capacity, int *parm)
+{
+	int heads, sectors, cylinders;
+
+	heads = 128;
+	sectors = 32;
+
+	cylinders = (capacity / (128 * 32));

You need to use sector_div here to avoid blowing up on 32bit systems.

Would it be ok to rename the sysfs file to ipr_version so I have a way to get this information until a better way comes along?

IPR_IOCTL_READ_DRIVER_CFG, IPR_IOCTL_WRITE_DRIVER_CFG, IPR_IOCTL_RUN_DIAGNOSTICS, IPR_IOCTL_RESET_IOA, IPR_IOCTL_CHANGE_ADAPTER_ASSIGNMENT: these could be sysfs files.

IPR_IOCTL_PASSTHRU: Allows user space to build adapter/device commands to send to the adapter. This includes RAID configuration, cache recovery commands, etc. This ioctl allows me to push all the RAID configuration complexity into userspace.

IPR_IOCTL_RUN_DIAGNOSTICS: This one could be a sysfs file.

IPR_IOCTL_DUMP_IOA: This ioctl is called by a userspace daemon. When called, it goes to sleep and waits (interruptibly) for a severe adapter error to occur. When this happens, the driver "dumps" the adapter's memory. After the adapter is reset, the daemon wakes up and writes the dump file to the filesystem. The dump file is usually around 2-4Meg in size.

IPR_IOCTL_GET_TRACE: This returns the driver's internal trace. This one needs a buffer > 4k. I could create a binary sysfs file for it since, AFAIK, this size limitation does not exist for binary sysfs files.

IPR_IOCTL_RECLAIM_CACHE: This is used to perform cache storage reclaimation on the adapter. It takes parameters from userspace, sends a command to the adapter, resets the adapter, then returns data to the user.

IPR_IOCTL_UCODE_DOWNLOAD: Used to update the microcode on the adapter and on the physical disks in RAID arrays (these devices are not seen by the SCSI midlayer). These microcode images are generally between 500k and 1M in size.

Quoted 3 lines

Okay, this isn't good at all. A LLDD driver shouldn't care about mode pages and all the stuff you do with it. Please do it from userspace using the sg driver.

Ok. Here are my reasons:

1. First of all, this driver is setting up mode pages for devices according to how the adapter requires them to be setup to run properly. Arguably, the adapter should do this, but it doesn't.

2. There are 2 types of devices that I setup mode pages for. The first are generic SCSI disk devices. These are seen by the mid-layer and could feasibly have their mode pages setup by userspace. But, the adapter requires qerr=1 in order to run TCQing. So I am setting that up. If I moved that into userspace, then I couldn't run TCQing until userspace setup mode page 0x0A.

The other type of device is a physical disk in a
RAID array. These devices are not seen by the mid-layer. The only way for userspace to talk to them (at least in the current implementation) is through the adapter ioctl interface discussed above. For these devices, the adapter requires the mode pages to be setup as I am doing in order to run reliably.

Quoted 2 lines

Can you explain what these vsets are supposed to do? And why a scsi_scan quirk can't be done instead of doing this in a LLDD?

I was expecting this one;) VSET stands for Volume Set. A VSET is a logical disk created by the adapter firmware representing a disk array. The reason a scsi_scan quirk does not work is because they also do not have any reasonable product ID in their standard inquiry data to parse on.

because I need to (ipr_std_inquiry). This stems from the fact that the adapter will not start any new advanced function (RAID, adapter write caching, etc) to a device until it is told by the system that the device is a "supported device" by using the "set supported devices" command.

Complicated example: You have a RAID 5 disk array with n devices. One of them fails. You put in a replacement device of a different type. In order to be able to rebuild the disk array using the new device, the adapter needs to have a set supported devices command sent down with the vendor/product ID of the replacement device. An inquiry has to be done to get this data.

Quoted 3 lines

This looks very wrong. And fortunately it seems to be only called from code that is totally misplaced in a LLDD anyway. All scsi commands should go through the midlayer queueing.

It is primarily used when talking to devices not known to the mid-layer, like the adapter itself, and devices that are in a disk array.

Quoted 2 lines

Again, a LLDD is the very wrong place for something like that. Is there a spec for that VSET stuff somewhere?

Unfortunately no. But I can answer any questions you might have. I wasn't sure if this was an ok thing to do or not. The midlayer is unconfiguring the device, it seemed reasonable that I should have the adapter flush the write cache and stop talking to that logical device.

Build a command in DMA'able host memory (struct ipr_ioarcb), write the PCI address to a register on the adapter, the adapter DMAs down the control block, executes the command, writes the ipr_ioarcb.host_response_handle in the command control block to the host request/response queue in host memory, and signals an interrupt.

Does that help at all?

Quoted 2 lines

this should be handled by the midlayer per-device and per-host state bits.

I didn't realize a LLD was allowed to modify these.

Quoted 1 line

Would it be ok to rename the sysfs file to ipr_version so I have a way to get this information until a better way comes along?

I'm not too happy. Can't you just grep dmesg output like we do for other drivers?

Quoted 1 line

IPR_IOCTL_READ_DRIVER_CFG, IPR_IOCTL_WRITE_DRIVER_CFG, IPR_IOCTL_RUN_DIAGNOSTICS, IPR_IOCTL_RESET_IOA, IPR_IOCTL_CHANGE_ADAPTER_ASSIGNMENT: these could be sysfs files.

Please change them over.

Quoted 1 line

IPR_IOCTL_GET_TRACE: This returns the driver's internal trace. This one needs a buffer > 4k. I could create a binary sysfs file for it since, AFAIK, this size limitation does not exist for binary sysfs files.

Please do. Also what about making this and the above one conditional on some debugging option?

Quoted 1 line

IPR_IOCTL_UCODE_DOWNLOAD: Used to update the microcode on the adapter and on the physical disks in RAID arrays (these devices are not seen by the SCSI midlayer). These microcode images are generally between 500k and 1M in size.

Can't you use the request_firmware interface for that?

Quoted 4 lines

Quoted 3 lines

Okay, this isn't good at all. A LLDD driver shouldn't care about mode pages and all the stuff you do with it. Please do it from userspace using the sg driver.

Quoted 5 lines

Ok. Here are my reasons:

1. First of all, this driver is setting up mode pages for devices according to how the adapter requires them to be setup to run properly. Arguably, the adapter should do this, but it doesn't.

2. There are 2 types of devices that I setup mode pages for. The first are generic SCSI disk devices. These are seen by the mid-layer and could feasibly have their mode pages setup by userspace. But, the adapter requires qerr=1 in order to run TCQing. So I am setting that up. If I moved that into userspace, then I couldn't run TCQing until userspace setup mode page 0x0A.

Doesn't sound to bad, does it? Bootup ir hardly performance critical.

Quoted 1 line

The other type of device is a physical disk in a RAID array. These devices are not seen by the mid-layer. The only way for userspace to talk to them (at least in the current implementation) is through the adapter ioctl interface discussed above. For these devices, the adapter requires the mode pages to be setup as I am doing in order to run reliably.

Given all the mess these invisble devices create I think they shouldn't be invisble but rather exported as a second pseudo-channel on the host.

Quoted 3 lines

because I need to (ipr_std_inquiry). This stems from the fact that the adapter will not start any new advanced function (RAID, adapter write caching, etc) to a device until it is told by the system that the device is a "supported device" by using the "set supported devices" command.

Complicated example: You have a RAID 5 disk array with n devices. One of them fails. You put in a replacement device of a different type. In order to be able to rebuild the disk array using the new device, the adapter needs to have a set supported devices command sent down with the vendor/product ID of the replacement device. An inquiry has to be done to get this data.

Yikes. I'd love to see designer of this hardware face by face..

Well, whats done is done and we can fix these fuckups now. Can't you at least do the inquiries from userland?

Quoted 5 lines

Quoted 3 lines

This looks very wrong. And fortunately it seems to be only called from code that is totally misplaced in a LLDD anyway. All scsi commands should go through the midlayer queueing.

It is primarily used when talking to devices not known to the mid-layer, like the adapter itself, and devices that are in a disk array.

Well, you shouldn't do that. We do in fact have interfaces to allocate proper scsi device without registering them with the midlayer, currently it's only used for the host itself (scsi_get_host_dev/scsi_free_host_dev), but I don't see why this can't work for other devices.

Quoted 4 lines

Quoted 2 lines

Again, a LLDD is the very wrong place for something like that. Is there a spec for that VSET stuff somewhere?

Unfortunately no. But I can answer any questions you might have. I wasn't sure if this was an ok thing to do or not. The midlayer is unconfiguring the device, it seemed reasonable that I should have the adapter flush the write cache and stop talking to that logical device.

We should probably always send a STOP UNIT when unconfiguring devices from the midlayer or and upper driver.

Quoted 3 lines

Build a command in DMA'able host memory (struct ipr_ioarcb), write the PCI address to a register on the adapter, the adapter DMAs down the control block, executes the command, writes the ipr_ioarcb.host_response_handle in the command control block to the host request/response queue in host memory, and signals an interrupt.

Does that help at all?

What's the relation of all these command lists to that?

Quoted 3 lines

Quoted 1 line

this should be handled by the midlayer per-device and per-host state bits.

I didn't realize a LLD was allowed to modify these.

Not directly. But we have function to e.g. set a device offline.

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

this should be handled by the midlayer per-device and per-host state bits.

I didn't realize a LLD was allowed to modify these.

Not directly. But we have function to e.g. set a device offline.

Not in 2.6.1. I think this was in, then removed at some point. Anyone know the story on why it was removed? Should we have a scsi_set_host_offline as well?

Quoted 2 lines

Please do. Also what about making this and the above one conditional on some debugging option?

How about a kernel config option for this? I don't want to make it too difficult for people to enable it as it is exceedingly valuable for failure analysis.

Quoted 2 lines

Given all the mess these invisble devices create I think they shouldn't be invisble but rather exported as a second pseudo-channel on the host.

Ok. That would solve a lot of problems. I will report these devices in as scsi disk devices. They will respond like normal SCSI disks, except when issued media commands (read/write) it will fail with data protect
(07) sense data.

Yes. Should be able to. By getting rid of all this code, one problem we will need to solve is the following:

1. Driver is up and running with a disk array.
2. Adapter gets reset for some reason (could be due to an adapter error)
3. Disk array device now needs a START_UNIT command.

The current mid-layer/sd will not handle this. Would you accept a patch to add this function?

Ok, but I will still need that interface to send commands directed to the adapter.

Quoted 6 lines

Quoted 6 lines

Build a command in DMA'able host memory (struct ipr_ioarcb), write the PCI address to a register on the adapter, the adapter DMAs down the control block, executes the command, writes the ipr_ioarcb.host_response_handle in the command control block to the host request/response queue in host memory, and signals an interrupt.

Quoted 3 lines

Quoted 1 line

Does that help at all?

What's the relation of all these command lists to that?

Sorry, I guess I'm missing something here. Are you saying a LLD shouldn't need to maintain of queue of its command blocks (like my pending_q and free_q)?

Quoted 1 line

Ok, but I will still need that interface to send commands directed to the adapter.

commands as in scsi commands or as in internal commands?

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

Build a command in DMA'able host memory (struct ipr_ioarcb), write the PCI address to a register on the adapter, the adapter DMAs down the control block, executes the command, writes the ipr_ioarcb.host_response_handle in the command control block to the host request/response queue in host memory, and signals an interrupt.

Does that help at all?

What's the relation of all these command lists to that?

Quoted 1 line

Sorry, I guess I'm missing something here. Are you saying a LLD shouldn't need to maintain of queue of its command blocks (like my pending_q and free_q)?

Unless you allocate the blocks in ->queuecommand (like most modern drivers do) you'll need some form of freelist of course. What you shouldn't need iß the pendig_q as the midlayer already keeps track of pending commands. This of course only works if you actually do send all commands via the midlayer (which you should).

Quoted 1 line

commands as in scsi commands or as in internal commands?

Internal commands. As you look through the driver, they will look very similar, as the interface to the adapter maps to scsi pretty close. I could use the mid-layer for these commands, but I would need some way to send them when the adapter is self blocked and would probably need to call scsi_add_host before the adapter is functional so I could send the adapter initialization sequence.

The reason I have a pending_q is so that when I reset the adapter without the mid-layer knowing about it, I need to fail back any outstanding ops so they get retried as appropriate. I suppose a way around this would be a mid-layer interface that an LLD could call which would cause a host reset. Then the midlayer could drive the reset and take care of doing the right thing with any outstanding ops.

Quoted 1 line

The reason I have a pending_q is so that when I reset the adapter without the mid-layer knowing about it, I need to fail back any outstanding ops so they get retried as appropriate. I suppose a way around this would be a mid-layer interface that an LLD could call which would cause a host reset. Then the midlayer could drive the reset and take care of doing the right thing with any outstanding ops.

Does scsi_reset_provider fit your needs?

Quoted 4 lines

Quoted 2 lines

Given all the mess these invisble devices create I think they shouldn't be invisble but rather exported as a second pseudo-channel on the host.

Ok. That would solve a lot of problems. I will report these devices in as scsi disk devices. They will respond like normal SCSI disks, except when issued media commands (read/write) it will fail with data protect (07) sense data.

Now reading this again it might be better to use the "fake" as in not registered with the device model devices I mentioned below for this.

How about a flag in the scsi_device struct that an LLD could set in slave_configure, which would prevent upper layer drivers from binding to it?

Then these devices could be found through the normal scsi scan and would have nice sysfs entries and sg devices for them. Not sure what we would call the flag... psuedo_device, fake_device?

Quoted 3 lines · Brian King

How about a flag in the scsi_device struct that an LLD could set in slave_configure, which would prevent upper layer drivers from binding to it?

I'm not that happy about this. But the other solutions I could think of are ven worse.

Quoted 3 lines

Then these devices could be found through the normal scsi scan and would have nice sysfs entries and sg devices for them. Not sure what we would call the flag... psuedo_device, fake_device?

ghost? hidden?

Could you cook up a patch?

Also if we can tell via sysfs attributes that a bus (host:channel or maybe just a host) has physical disks attached, udev can be used to create entries in their own directory, like:

/dev/physical/sdN

Making the node read only can also be done with udev, but that does not help with root write access.

Quoted 14 lines · Christoph Hellwig

Quoted 3 lines · Brian King

How about a flag in the scsi_device struct that an LLD could set in slave_configure, which would prevent upper layer drivers from binding to it?

I'm not that happy about this. But the other solutions I could think of are ven worse.

Quoted 3 lines

Then these devices could be found through the normal scsi scan and would have nice sysfs entries and sg devices for them. Not sure what we would call the flag... psuedo_device, fake_device?

Quoted 3 lines

ghost? hidden?

Could you cook up a patch?

Why not just have the sd's show up? This is not much different then use of a volume manager. Finding the /dev/sg of the physical disk is the same problem as finding the sd.

We will still get errors logged when the partition table is read, as the devices are also read protected.

Quoted 2 lines · Brian King

We will still get errors logged when the partition table is read, as the devices are also read protected.

So it is the hardware and not the driver enforcing the read protection? But other SCSI commands are OK?

Quoted 2 lines · Christoph Hellwig

We should probably always send a STOP UNIT when unconfiguring devices from the midlayer or and upper driver.

Sounds reasonable.

Quoted 7 lines

+/**
+ * ipr_find_ses_entry - Find matching SES in SES table
+ * @res: resource entry struct of SES
+ * + * Return value:
+ * pointer to SES table entry / NULL on failure
+ **/

Quoted 2 lines

This is an extreme layering violation. Any SES handling belongs into an upper level scsi driver (or userspace via sg)

I'm not actually talking to the SES, I'm only looking at the adapter's device configuration table. What I am trying to accomplish here is I am looking at each SCSI bus and trying to determine the max scsi speed that is safe to run on them. There are certain older backplanes that do not run reliably at faster speeds. Hence, the SES table.

Did my explanation help, or would you still like this code removed? I don't have a huge problem moving it into userspace, the only downside is that I will only be able to run at 80MB/sec until the configuration tool runs.