Re: ioctls, etc. (was Re: [PATCH 1/4] sas: add flag for locally attached PHYs)

Luben Tuikov
2005-10-21T18:37:46Z

On 10/21/05 02:19, Jeff Garzik wrote:
> With the inevitable result that most ioctl code is poorly written, and
> causes bugs and special case synchronization problems :) Driver writers
> love to stuff way too much stuff into ioctls, without thinking about
> overall design.

Could you please use adjectives, like "some" or "all" before
"Device writers"? Could you also use a qualifier like, "excluding me",
etc. Thanks.

> Only for the most simple interfaces. sysfs is for exporting kernel data
> structures to userspace, using read(2) and write(2). You can quite
> literally accomplish anything with that. sysfs could export an event
> directory entry, and a response directory entry. The response dirent
> could provide all the error reporting you could ever want. That's just
> a 2-second off-the-cuff example.

For the 3-second "off-the-cuff example" see how the "smp_portal" sysfs
binary attribute works in drivers/scsi/sas/sas_expander.c at the bottom
of the file, whereby user space first writes and then reads as much data
it is expecting.

(Thus there is _no_ hardcoding of the amount of data SMP request can
return (as is done in SDI).)

Here are the tiny functions for refresh:

/* ---------- SMP portal ---------- */

static ssize_t smp_portal_write(struct kobject *kobj, char *buf, loff_t offs,
size_t size)
{
struct domain_device *dev = to_dom_device(kobj);
struct expander_device *ex = &dev->ex_dev;

if (offs != 0)
return -EFBIG;
else if (size == 0)
return 0;

down_interruptible(&ex->smp_sema);
if (ex->smp_req)
kfree(ex->smp_req);
ex->smp_req = kzalloc(size, GFP_USER);
if (!ex->smp_req) {
up(&ex->smp_sema);
return -ENOMEM;
}
memcpy(ex->smp_req, buf, size);
ex->smp_req_size = size;
ex->smp_portal_pid = current->pid;
up(&ex->smp_sema);

return size;
}

static ssize_t smp_portal_read(struct kobject *kobj, char *buf, loff_t offs,
size_t size)
{
struct domain_device *dev = to_dom_device(kobj);
struct expander_device *ex = &dev->ex_dev;
u8 *smp_resp;
int res = -EINVAL;

down_interruptible(&ex->smp_sema);
if (!ex->smp_req || ex->smp_portal_pid != current->pid ||
offs != ex->smp_req_size)
goto out;

res = 0;
if (size == 0)
goto out;

res = -ENOMEM;
smp_resp = alloc_smp_resp(size);
if (!smp_resp)
goto out;
res = smp_execute_task(dev, ex->smp_req, ex->smp_req_size,
smp_resp, size);
if (!res) {
memcpy(buf, smp_resp, size);
res = size;
}

kfree(smp_resp);
out:
kfree(ex->smp_req);
ex->smp_req = NULL;
ex->smp_req_size = 0;
ex->smp_portal_pid = -1;
up(&ex->smp_sema);
return res;
}

> Note, I'm _not_ suggesting this is the best route for an SMP interface,
> just stating sysfs generalities. sysfs is flexible enough that it could
> completely replace SG_IO (though that would not be a wise idea).

Indeed, sysfs _is_ very flexible, and with a single process open(2) it could
satisfy the transaction needed functionality.

Luben
--