2008-10-01 09:01:09

by Martin Schwidefsky

[permalink] [raw]
Subject: [patch 21/21] Add ioctl support for EMC Symmetrix Subsystem Control I/O

From: Nigel Hislop <[email protected]>

EMC Symmetrix Subsystem Control I/O through CKD dasd requires a
specific parameter list sent to the array via a Perform Subsystem
Function CCW. The Symmetrix response is retrieved from the array
via a Read Subsystem Data CCW.

Signed-off-by: Nigel Hislop <[email protected]>
Signed-off-by: Hannes Reinecke <[email protected]>
Signed-off-by: Martin Schwidefsky <[email protected]>
---

arch/s390/include/asm/dasd.h | 13 +++++
drivers/s390/block/dasd_eckd.c | 98 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+)

Index: quilt-2.6/arch/s390/include/asm/dasd.h
===================================================================
--- quilt-2.6.orig/arch/s390/include/asm/dasd.h
+++ quilt-2.6/arch/s390/include/asm/dasd.h
@@ -3,6 +3,8 @@
* Author(s)......: Holger Smolinski <[email protected]>
* Bugreports.to..: <[email protected]>
* (C) IBM Corporation, IBM Deutschland Entwicklung GmbH, 1999,2000
+ * EMC Symmetrix ioctl Copyright EMC Corporation, 2008
+ * Author.........: Nigel Hislop <[email protected]>
*
* This file is the interface of the DASD device driver, which is exported to user space
* any future changes wrt the API will result in a change of the APIVERSION reported
@@ -202,6 +204,16 @@ typedef struct attrib_data_t {
#define DASD_SEQ_PRESTAGE 0x4
#define DASD_REC_ACCESS 0x5

+/*
+ * Perform EMC Symmetrix I/O
+ */
+typedef struct dasd_symmio_parms {
+ unsigned char reserved[8]; /* compat with older releases */
+ unsigned long long psf_data; /* char * cast to u64 */
+ unsigned long long rssd_result; /* char * cast to u64 */
+ int psf_data_len;
+ int rssd_result_len;
+} __attribute__ ((packed)) dasd_symmio_parms_t;

/********************************************************************************
* SECTION: Definition of IOCTLs
@@ -247,6 +259,7 @@ typedef struct attrib_data_t {
/* Set Attributes (cache operations) */
#define BIODASDSATTR _IOW(DASD_IOCTL_LETTER,2,attrib_data_t)

+#define BIODASDSYMMIO _IOWR(DASD_IOCTL_LETTER, 240, dasd_symmio_parms_t)

#endif /* DASD_H */

Index: quilt-2.6/drivers/s390/block/dasd_eckd.c
===================================================================
--- quilt-2.6.orig/drivers/s390/block/dasd_eckd.c
+++ quilt-2.6/drivers/s390/block/dasd_eckd.c
@@ -6,6 +6,8 @@
* Martin Schwidefsky <[email protected]>
* Bugreports.to..: <[email protected]>
* (C) IBM Corporation, IBM Deutschland Entwicklung GmbH, 1999,2000
+ * EMC Symmetrix ioctl Copyright EMC Corporation, 2008
+ * Author.........: Nigel Hislop <[email protected]>
*
*/

@@ -2083,6 +2085,100 @@ dasd_eckd_set_attrib(struct dasd_device
return 0;
}

+/*
+ * Issue syscall I/O to EMC Symmetrix array.
+ * CCWs are PSF and RSSD
+ */
+static int dasd_symm_io(struct dasd_device *device, void __user *argp)
+{
+ struct dasd_symmio_parms usrparm;
+ char *psf_data, *rssd_result;
+ struct dasd_ccw_req *cqr;
+ struct ccw1 *ccw;
+ int rc;
+
+ /* Copy parms from caller */
+ rc = copy_from_user(&usrparm, argp, sizeof(usrparm)) ? -EFAULT : 0;
+ if (rc)
+ goto out;
+#ifndef CONFIG_64BIT
+ /* Make sure pointers are sane even on 31 bit. */
+ if ((usrparm.psf_data >> 32) != 0 || (usrparm.rssd_result >> 32) != 0) {
+ rc = -EINVAL;
+ goto out;
+ }
+#endif
+ /* alloc I/O data area */
+ psf_data = kzalloc(usrparm.psf_data_len, GFP_KERNEL | GFP_DMA);
+ rssd_result = kzalloc(usrparm.rssd_result_len, GFP_KERNEL | GFP_DMA);
+ if (!psf_data || !rssd_result) {
+ rc = -ENOMEM;
+ goto out_free;
+ }
+
+ /* get syscall header from user space */
+ rc = copy_from_user(psf_data,
+ (void __user *)(unsigned long) usrparm.psf_data,
+ usrparm.psf_data_len) ? -EFAULT : 0;
+ if (rc)
+ goto out_free;
+
+ /* sanity check on syscall header */
+ if (psf_data[0] != 0x17 && psf_data[1] != 0xce) {
+ rc = -EINVAL;
+ goto out_free;
+ }
+
+ /* setup CCWs for PSF + RSSD */
+ cqr = dasd_smalloc_request("ECKD", 2 , 0, device);
+ if (IS_ERR(cqr)) {
+ DEV_MESSAGE(KERN_WARNING, device, "%s",
+ "Could not allocate initialization request");
+ rc = PTR_ERR(cqr);
+ goto out_free;
+ }
+
+ cqr->startdev = device;
+ cqr->memdev = device;
+ cqr->retries = 3;
+ cqr->expires = 10 * HZ;
+ cqr->buildclk = get_clock();
+ cqr->status = DASD_CQR_FILLED;
+
+ /* Build the ccws */
+ ccw = cqr->cpaddr;
+
+ /* PSF ccw */
+ ccw->cmd_code = DASD_ECKD_CCW_PSF;
+ ccw->count = usrparm.psf_data_len;
+ ccw->flags |= CCW_FLAG_CC;
+ ccw->cda = (__u32)(addr_t) psf_data;
+
+ ccw++;
+
+ /* RSSD ccw */
+ ccw->cmd_code = DASD_ECKD_CCW_RSSD;
+ ccw->count = usrparm.rssd_result_len;
+ ccw->flags = CCW_FLAG_SLI ;
+ ccw->cda = (__u32)(addr_t) rssd_result;
+
+ rc = dasd_sleep_on(cqr);
+ if (rc)
+ goto out_sfree;
+
+ rc = copy_to_user((void __user *)(unsigned long) usrparm.rssd_result,
+ rssd_result, usrparm.rssd_result_len) ? -EFAULT : 0;
+
+out_sfree:
+ dasd_sfree_request(cqr, cqr->memdev);
+out_free:
+ kfree(rssd_result);
+ kfree(psf_data);
+out:
+ DBF_DEV_EVENT(DBF_WARNING, device, "Symmetrix ioctl: rc=%d", rc);
+ return rc;
+}
+
static int
dasd_eckd_ioctl(struct dasd_block *block, unsigned int cmd, void __user *argp)
{
@@ -2101,6 +2197,8 @@ dasd_eckd_ioctl(struct dasd_block *block
return dasd_eckd_reserve(device);
case BIODASDSLCK:
return dasd_eckd_steal_lock(device);
+ case BIODASDSYMMIO:
+ return dasd_symm_io(device, argp);
default:
return -ENOIOCTLCMD;
}

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.


2008-10-01 11:03:17

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [patch 21/21] Add ioctl support for EMC Symmetrix Subsystem Control I/O

> +static int dasd_symm_io(struct dasd_device *device, void __user *argp)
> +{
> + struct dasd_symmio_parms usrparm;
> + char *psf_data, *rssd_result;
> + struct dasd_ccw_req *cqr;
> + struct ccw1 *ccw;
> + int rc;
> +
> + /* Copy parms from caller */
> + rc = copy_from_user(&usrparm, argp, sizeof(usrparm)) ? -EFAULT : 0;
> + if (rc)
> + goto out;

That style is quite odd, what about the more normal

rc = -EFAULT;
if (copy_from_user(&usrparm, argp, sizeof(usrparm)))
goto out;

instead?

> + /* alloc I/O data area */
> + psf_data = kzalloc(usrparm.psf_data_len, GFP_KERNEL | GFP_DMA);
> + rssd_result = kzalloc(usrparm.rssd_result_len, GFP_KERNEL | GFP_DMA);
> + if (!psf_data || !rssd_result) {
> + rc = -ENOMEM;
> + goto out_free;
> + }

just check each on individually and do individual unwinding. Makes the
code a little more easily readable.

> + /* get syscall header from user space */
> + rc = copy_from_user(psf_data,
> + (void __user *)(unsigned long) usrparm.psf_data,
> + usrparm.psf_data_len) ? -EFAULT : 0;
> + if (rc)
> + goto out_free;

Same as the first copy_from_user

> + rc = copy_to_user((void __user *)(unsigned long) usrparm.rssd_result,
> + rssd_result, usrparm.rssd_result_len) ? -EFAULT : 0;

And here again.

2008-10-01 11:21:32

by Martin Schwidefsky

[permalink] [raw]
Subject: Re: [patch 21/21] Add ioctl support for EMC Symmetrix Subsystem Control I/O

On Wed, 2008-10-01 at 07:03 -0400, Christoph Hellwig wrote:
> > +static int dasd_symm_io(struct dasd_device *device, void __user *argp)
> > +{
> > + struct dasd_symmio_parms usrparm;
> > + char *psf_data, *rssd_result;
> > + struct dasd_ccw_req *cqr;
> > + struct ccw1 *ccw;
> > + int rc;
> > +
> > + /* Copy parms from caller */
> > + rc = copy_from_user(&usrparm, argp, sizeof(usrparm)) ? -EFAULT : 0;
> > + if (rc)
> > + goto out;
>
> That style is quite odd, what about the more normal
>
> rc = -EFAULT;
> if (copy_from_user(&usrparm, argp, sizeof(usrparm)))
> goto out;
>
> instead?

Ok, I will change that.

> > + /* alloc I/O data area */
> > + psf_data = kzalloc(usrparm.psf_data_len, GFP_KERNEL | GFP_DMA);
> > + rssd_result = kzalloc(usrparm.rssd_result_len, GFP_KERNEL | GFP_DMA);
> > + if (!psf_data || !rssd_result) {
> > + rc = -ENOMEM;
> > + goto out_free;
> > + }
>
> just check each on individually and do individual unwinding. Makes the
> code a little more easily readable.

Well, matter of taste I would say. Two kzallocs, one check for the
results and just two kfrees if one of the kzallocs failed. I find this
version to be a little more compact and therefore easier to read. If you
insist I will change it ..

> > + /* get syscall header from user space */
> > + rc = copy_from_user(psf_data,
> > + (void __user *)(unsigned long) usrparm.psf_data,
> > + usrparm.psf_data_len) ? -EFAULT : 0;
> > + if (rc)
> > + goto out_free;
>
> Same as the first copy_from_user

Ok.

> > + rc = copy_to_user((void __user *)(unsigned long) usrparm.rssd_result,
> > + rssd_result, usrparm.rssd_result_len) ? -EFAULT : 0;
>
> And here again.

Ok.

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.