Hello Dan,
This patch makes raw1394 in current Linux git tree (2.6.21-1570) usable to 32bit
applications running on 64bit kernel (tested on i386 app using x86_64 kernel). I
had to make following changes:
* read() always failed with -EFAULT. This was happening due to
raw1394_compat_read copying data to wrong location - access_ok always
failed as 'r' is kernel address, not user. Whole function just tried to
copy data from 'r' to 'r', which is not good.
* write(fd, buf, 52) from 32bit app was returning 56. Most of callers did not
care, but some (arm registration) did, and anyway it looks bad if request for
writing 52 bytes returns 56. And returning sizeof anything in 'int' is not
good as well. So all functions now return '0' instead of
sizeof(struct raw1394_request) on success, and write() itself provides correct
return value (it just returns value it was asked to write on success as raw1394
does not do any partial writes at all).
* Related to this was problem that write() could have returned 0 when kernel
state would become corrupted and moved to different state than
opened/initialized/connected. Now it returns -EBADFD which seemed appropriate.
* And add compat_ioctl. Although all structures are more or less same,
raw1394_iso_packets got pointer inside, and raw1394_cycle_timer got unwanted
padding in the middle. I did not add any translation for ioctls passing array
of integers around as integers seem to have same size (32 bits) on all
architectures supported by Linux.
With this in place I was able to run my test app and grab some mpegs, so I believe
that I got everything necessary done. If you believe I have missed some ioctl, yell
at me.
Thanks,
Petr Vandrovec
Signed-off-by: Petr Vandrovec <[email protected]>
diff -uprdN linux/drivers/ieee1394/raw1394.c linux/drivers/ieee1394/raw1394.c
--- linux/drivers/ieee1394/raw1394.c 2007-05-06 17:45:47.000000000 -0700
+++ linux/drivers/ieee1394/raw1394.c 2007-05-06 18:08:05.000000000 -0700
@@ -460,7 +460,7 @@ static const char __user *raw1394_compat
static int
raw1394_compat_read(const char __user *buf, struct raw1394_request *r)
{
- struct compat_raw1394_req __user *cr = (typeof(cr)) r;
+ struct compat_raw1394_req __user *cr = (typeof(cr)) buf;
if (!access_ok(VERIFY_WRITE, cr, sizeof(struct compat_raw1394_req)) ||
P(type) ||
P(error) ||
@@ -588,7 +588,7 @@ static int state_opened(struct file_info
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
static int state_initialized(struct file_info *fi, struct pending_request *req)
@@ -602,7 +602,7 @@ static int state_initialized(struct file
req->req.generation = atomic_read(&internal_generation);
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
switch (req->req.type) {
@@ -674,7 +674,7 @@ out_set_card:
}
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
static void handle_iso_listen(struct file_info *fi, struct pending_request *req)
@@ -866,7 +866,7 @@ static int handle_async_request(struct f
if (req->req.error) {
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
hpsb_set_packet_complete_task(packet,
@@ -884,7 +884,7 @@ static int handle_async_request(struct f
hpsb_free_tlabel(packet);
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int handle_iso_send(struct file_info *fi, struct pending_request *req,
@@ -908,7 +908,7 @@ static int handle_iso_send(struct file_i
req->req.error = RAW1394_ERROR_MEMFAULT;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
req->req.length = 0;
@@ -928,7 +928,7 @@ static int handle_iso_send(struct file_i
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int handle_async_send(struct file_info *fi, struct pending_request *req)
@@ -943,7 +943,7 @@ static int handle_async_send(struct file
req->req.error = RAW1394_ERROR_INVALID_ARG;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
packet = hpsb_alloc_packet(req->req.length - header_length);
@@ -956,7 +956,7 @@ static int handle_async_send(struct file
req->req.error = RAW1394_ERROR_MEMFAULT;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
if (copy_from_user
@@ -965,7 +965,7 @@ static int handle_async_send(struct file
req->req.error = RAW1394_ERROR_MEMFAULT;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
packet->type = hpsb_async;
@@ -993,7 +993,7 @@ static int handle_async_send(struct file
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int arm_read(struct hpsb_host *host, int nodeid, quadlet_t * buffer,
@@ -1868,7 +1868,7 @@ static int arm_register(struct file_info
spin_lock_irqsave(&host_info_lock, flags);
list_add_tail(&addr->addr_list, &fi->addr_list);
spin_unlock_irqrestore(&host_info_lock, flags);
- return sizeof(struct raw1394_request);
+ return 0;
}
retval =
hpsb_register_addrspace(&raw1394_highlevel, fi->host, &arm_ops,
@@ -1886,7 +1886,7 @@ static int arm_register(struct file_info
return (-EALREADY);
}
free_pending_request(req); /* immediate success or fail */
- return sizeof(struct raw1394_request);
+ return 0;
}
static int arm_unregister(struct file_info *fi, struct pending_request *req)
@@ -1954,7 +1954,7 @@ static int arm_unregister(struct file_in
vfree(addr->addr_space_buffer);
kfree(addr);
free_pending_request(req); /* immediate success or fail */
- return sizeof(struct raw1394_request);
+ return 0;
}
retval =
hpsb_unregister_addrspace(&raw1394_highlevel, fi->host,
@@ -1970,7 +1970,7 @@ static int arm_unregister(struct file_in
vfree(addr->addr_space_buffer);
kfree(addr);
free_pending_request(req); /* immediate success or fail */
- return sizeof(struct raw1394_request);
+ return 0;
}
/* Copy data from ARM buffer(s) to user buffer. */
@@ -2012,7 +2012,7 @@ static int arm_get_buf(struct file_info
* queue no response, and therefore nobody
* will free it. */
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
} else {
DBGMSG("arm_get_buf request exceeded mapping");
spin_unlock_irqrestore(&host_info_lock, flags);
@@ -2064,7 +2064,7 @@ static int arm_set_buf(struct file_info
* queue no response, and therefore nobody
* will free it. */
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
} else {
DBGMSG("arm_set_buf request exceeded mapping");
spin_unlock_irqrestore(&host_info_lock, flags);
@@ -2085,7 +2085,7 @@ static int reset_notification(struct fil
(req->req.misc == RAW1394_NOTIFY_ON)) {
fi->notification = (u8) req->req.misc;
free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
- return sizeof(struct raw1394_request);
+ return 0;
}
/* error EINVAL (22) invalid argument */
return (-EINVAL);
@@ -2118,12 +2118,12 @@ static int write_phypacket(struct file_i
req->req.length = 0;
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int get_config_rom(struct file_info *fi, struct pending_request *req)
{
- int ret = sizeof(struct raw1394_request);
+ int ret = 0;
quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
int status;
@@ -2153,7 +2153,7 @@ static int get_config_rom(struct file_in
static int update_config_rom(struct file_info *fi, struct pending_request *req)
{
- int ret = sizeof(struct raw1394_request);
+ int ret = 0;
quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
if (!data)
return -ENOMEM;
@@ -2220,7 +2220,7 @@ static int modify_config_rom(struct file
hpsb_update_config_rom_image(fi->host);
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
}
@@ -2285,7 +2285,7 @@ static int modify_config_rom(struct file
/* we have to free the request, because we queue no response,
* and therefore nobody will free it */
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
} else {
for (dentry =
fi->csr1212_dirs[dr]->value.directory.dentries_head;
@@ -2310,7 +2310,7 @@ static int state_connected(struct file_i
case RAW1394_REQ_ECHO:
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
case RAW1394_REQ_ISO_SEND:
print_old_iso_deprecation();
@@ -2334,24 +2334,24 @@ static int state_connected(struct file_i
case RAW1394_REQ_ISO_LISTEN:
print_old_iso_deprecation();
handle_iso_listen(fi, req);
- return sizeof(struct raw1394_request);
+ return 0;
case RAW1394_REQ_FCP_LISTEN:
handle_fcp_listen(fi, req);
- return sizeof(struct raw1394_request);
+ return 0;
case RAW1394_REQ_RESET_BUS:
if (req->req.misc == RAW1394_LONG_RESET) {
DBGMSG("busreset called (type: LONG)");
hpsb_reset_bus(fi->host, LONG_RESET);
free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
- return sizeof(struct raw1394_request);
+ return 0;
}
if (req->req.misc == RAW1394_SHORT_RESET) {
DBGMSG("busreset called (type: SHORT)");
hpsb_reset_bus(fi->host, SHORT_RESET);
free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
- return sizeof(struct raw1394_request);
+ return 0;
}
/* error EINVAL (22) invalid argument */
return (-EINVAL);
@@ -2370,7 +2370,7 @@ static int state_connected(struct file_i
req->req.generation = get_hpsb_generation(fi->host);
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
switch (req->req.type) {
@@ -2383,7 +2383,7 @@ static int state_connected(struct file_i
if (req->req.length == 0) {
req->req.error = RAW1394_ERROR_INVALID_ARG;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
return handle_async_request(fi, req, node);
@@ -2394,7 +2394,7 @@ static ssize_t raw1394_write(struct file
{
struct file_info *fi = (struct file_info *)file->private_data;
struct pending_request *req;
- ssize_t retval = 0;
+ ssize_t retval = -EBADFD;
#ifdef CONFIG_COMPAT
if (count == sizeof(struct compat_raw1394_req) &&
@@ -2436,6 +2436,9 @@ static ssize_t raw1394_write(struct file
if (retval < 0) {
free_pending_request(req);
+ } else {
+ BUG_ON(retval);
+ retval = count;
}
return retval;
@@ -2801,6 +2804,99 @@ static int raw1394_ioctl(struct inode *i
return -EINVAL;
}
+#ifdef CONFIG_COMPAT
+struct raw1394_iso_packets32 {
+ __u32 n_packets;
+ compat_uptr_t infos;
+} __attribute__((packed));
+
+struct raw1394_cycle_timer32 {
+ __u32 cycle_timer;
+ __u64 local_time;
+} __attribute__((packed));
+
+#define RAW1394_IOC_ISO_RECV_PACKETS32 \
+ _IOW ('#', 0x25, struct raw1394_iso_packets32)
+#define RAW1394_IOC_ISO_XMIT_PACKETS32 \
+ _IOW ('#', 0x27, struct raw1394_iso_packets32)
+#define RAW1394_IOC_GET_CYCLE_TIMER32 \
+ _IOR ('#', 0x30, struct raw1394_cycle_timer32)
+
+static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd,
+ struct raw1394_iso_packets32 __user *arg)
+{
+ compat_uptr_t infos32;
+ void *infos;
+ long err = -EFAULT;
+ struct raw1394_iso_packets __user *dst = compat_alloc_user_space(sizeof(struct raw1394_iso_packets));
+
+ if (!copy_in_user(&dst->n_packets, &arg->n_packets, sizeof arg->n_packets) &&
+ !copy_from_user(&infos32, &arg->infos, sizeof infos32)) {
+ infos = compat_ptr(infos32);
+ if (!copy_to_user(&dst->infos, &infos, sizeof infos))
+ err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst);
+ }
+ return err;
+}
+
+static long raw1394_read_cycle_timer32(struct file_info *fi, void __user * uaddr)
+{
+ struct raw1394_cycle_timer32 ct;
+ int err;
+
+ err = hpsb_read_cycle_timer(fi->host, &ct.cycle_timer, &ct.local_time);
+ if (!err)
+ if (copy_to_user(uaddr, &ct, sizeof(ct)))
+ err = -EFAULT;
+ return err;
+}
+
+static long raw1394_compat_ioctl(struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct file_info *fi = file->private_data;
+ void __user *argp = (void __user *)arg;
+ long err;
+
+ lock_kernel();
+ switch (cmd) {
+ /* These requests have same format as long as 'int' has same size. */
+ case RAW1394_IOC_ISO_RECV_INIT:
+ case RAW1394_IOC_ISO_RECV_START:
+ case RAW1394_IOC_ISO_RECV_LISTEN_CHANNEL:
+ case RAW1394_IOC_ISO_RECV_UNLISTEN_CHANNEL:
+ case RAW1394_IOC_ISO_RECV_SET_CHANNEL_MASK:
+ case RAW1394_IOC_ISO_RECV_RELEASE_PACKETS:
+ case RAW1394_IOC_ISO_RECV_FLUSH:
+ case RAW1394_IOC_ISO_XMIT_RECV_STOP:
+ case RAW1394_IOC_ISO_XMIT_INIT:
+ case RAW1394_IOC_ISO_XMIT_START:
+ case RAW1394_IOC_ISO_XMIT_SYNC:
+ case RAW1394_IOC_ISO_GET_STATUS:
+ case RAW1394_IOC_ISO_SHUTDOWN:
+ case RAW1394_IOC_ISO_QUEUE_ACTIVITY:
+ err = raw1394_ioctl(NULL, file, cmd, arg);
+ break;
+ /* These request have different format. */
+ case RAW1394_IOC_ISO_RECV_PACKETS32:
+ err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_RECV_PACKETS, argp);
+ break;
+ case RAW1394_IOC_ISO_XMIT_PACKETS32:
+ err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_XMIT_PACKETS, argp);
+ break;
+ case RAW1394_IOC_GET_CYCLE_TIMER32:
+ err = raw1394_read_cycle_timer32(fi, argp);
+ break;
+ default:
+ err = -EINVAL;
+ break;
+ }
+ unlock_kernel();
+
+ return err;
+}
+#endif
+
static unsigned int raw1394_poll(struct file *file, poll_table * pt)
{
struct file_info *fi = file->private_data;
@@ -3040,7 +3136,9 @@ static const struct file_operations raw1
.write = raw1394_write,
.mmap = raw1394_mmap,
.ioctl = raw1394_ioctl,
- // .compat_ioctl = ... someone needs to do this
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = raw1394_compat_ioctl,
+#endif
.poll = raw1394_poll,
.open = raw1394_open,
.release = raw1394_release,
Petr Vandrovec wrote:
[...]
> * read() always failed with -EFAULT. This was happening due to
> raw1394_compat_read copying data to wrong location - access_ok always
> failed as 'r' is kernel address, not user. Whole function just tried to
> copy data from 'r' to 'r', which is not good.
>
> * write(fd, buf, 52) from 32bit app was returning 56. Most of callers did not
> care, but some (arm registration) did, and anyway it looks bad if request for
> writing 52 bytes returns 56. And returning sizeof anything in 'int' is not
> good as well. So all functions now return '0' instead of
> sizeof(struct raw1394_request) on success, and write() itself provides correct
> return value (it just returns value it was asked to write on success as raw1394
> does not do any partial writes at all).
>
> * Related to this was problem that write() could have returned 0 when kernel
> state would become corrupted and moved to different state than
> opened/initialized/connected. Now it returns -EBADFD which seemed appropriate.
>
> * And add compat_ioctl. Although all structures are more or less same,
> raw1394_iso_packets got pointer inside, and raw1394_cycle_timer got unwanted
> padding in the middle. I did not add any translation for ioctls passing array
> of integers around as integers seem to have same size (32 bits) on all
> architectures supported by Linux.
[...]
Thanks for these fixes. They look good at first glance but I will look
at them in more detail during the week (and hope that Dan can have a
look at them too). I will get back to you once more before I commit
because I would like to split it into three patches (for
raw1394_compat_read, for write, and for compat_ioctl).
--
Stefan Richter
-=====-=-=== -=-= --===
http://arcgraph.de/sr/
On Sunday 06 May 2007 19:14, Petr Vandrovec wrote:
> Hello Dan,
>
> This patch makes raw1394 in current Linux git tree (2.6.21-1570) usable to 32bit
> applications running on 64bit kernel (tested on i386 app using x86_64 kernel). I
> had to make following changes:
>
> * read() always failed with -EFAULT. This was happening due to
> raw1394_compat_read copying data to wrong location - access_ok always
> failed as 'r' is kernel address, not user. Whole function just tried to
> copy data from 'r' to 'r', which is not good.
>
> * write(fd, buf, 52) from 32bit app was returning 56. Most of callers did not
> care, but some (arm registration) did, and anyway it looks bad if request for
> writing 52 bytes returns 56. And returning sizeof anything in 'int' is not
> good as well. So all functions now return '0' instead of
> sizeof(struct raw1394_request) on success, and write() itself provides correct
> return value (it just returns value it was asked to write on success as raw1394
> does not do any partial writes at all).
>
> * Related to this was problem that write() could have returned 0 when kernel
> state would become corrupted and moved to different state than
> opened/initialized/connected. Now it returns -EBADFD which seemed appropriate.
>
> * And add compat_ioctl. Although all structures are more or less same,
> raw1394_iso_packets got pointer inside, and raw1394_cycle_timer got unwanted
> padding in the middle. I did not add any translation for ioctls passing array
> of integers around as integers seem to have same size (32 bits) on all
> architectures supported by Linux.
>
> With this in place I was able to run my test app and grab some mpegs, so I believe
> that I got everything necessary done. If you believe I have missed some ioctl, yell
> at me.
> Thanks,
> Petr Vandrovec
>
> Signed-off-by: Petr Vandrovec <[email protected]>
Reviewed and been testing out fine here on i386.
Acked-by: Dan Dennedy <[email protected]>
> diff -uprdN linux/drivers/ieee1394/raw1394.c linux/drivers/ieee1394/raw1394.c
> --- linux/drivers/ieee1394/raw1394.c 2007-05-06 17:45:47.000000000 -0700
> +++ linux/drivers/ieee1394/raw1394.c 2007-05-06 18:08:05.000000000 -0700
> @@ -460,7 +460,7 @@ static const char __user *raw1394_compat
> static int
> raw1394_compat_read(const char __user *buf, struct raw1394_request *r)
> {
> - struct compat_raw1394_req __user *cr = (typeof(cr)) r;
> + struct compat_raw1394_req __user *cr = (typeof(cr)) buf;
> if (!access_ok(VERIFY_WRITE, cr, sizeof(struct compat_raw1394_req)) ||
> P(type) ||
> P(error) ||
> @@ -588,7 +588,7 @@ static int state_opened(struct file_info
>
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static int state_initialized(struct file_info *fi, struct pending_request *req)
> @@ -602,7 +602,7 @@ static int state_initialized(struct file
> req->req.generation = atomic_read(&internal_generation);
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> switch (req->req.type) {
> @@ -674,7 +674,7 @@ out_set_card:
> }
>
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static void handle_iso_listen(struct file_info *fi, struct pending_request *req)
> @@ -866,7 +866,7 @@ static int handle_async_request(struct f
> if (req->req.error) {
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> hpsb_set_packet_complete_task(packet,
> @@ -884,7 +884,7 @@ static int handle_async_request(struct f
> hpsb_free_tlabel(packet);
> queue_complete_req(req);
> }
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static int handle_iso_send(struct file_info *fi, struct pending_request *req,
> @@ -908,7 +908,7 @@ static int handle_iso_send(struct file_i
> req->req.error = RAW1394_ERROR_MEMFAULT;
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> req->req.length = 0;
> @@ -928,7 +928,7 @@ static int handle_iso_send(struct file_i
> queue_complete_req(req);
> }
>
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static int handle_async_send(struct file_info *fi, struct pending_request *req)
> @@ -943,7 +943,7 @@ static int handle_async_send(struct file
> req->req.error = RAW1394_ERROR_INVALID_ARG;
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> packet = hpsb_alloc_packet(req->req.length - header_length);
> @@ -956,7 +956,7 @@ static int handle_async_send(struct file
> req->req.error = RAW1394_ERROR_MEMFAULT;
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> if (copy_from_user
> @@ -965,7 +965,7 @@ static int handle_async_send(struct file
> req->req.error = RAW1394_ERROR_MEMFAULT;
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> packet->type = hpsb_async;
> @@ -993,7 +993,7 @@ static int handle_async_send(struct file
> queue_complete_req(req);
> }
>
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static int arm_read(struct hpsb_host *host, int nodeid, quadlet_t * buffer,
> @@ -1868,7 +1868,7 @@ static int arm_register(struct file_info
> spin_lock_irqsave(&host_info_lock, flags);
> list_add_tail(&addr->addr_list, &fi->addr_list);
> spin_unlock_irqrestore(&host_info_lock, flags);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
> retval =
> hpsb_register_addrspace(&raw1394_highlevel, fi->host, &arm_ops,
> @@ -1886,7 +1886,7 @@ static int arm_register(struct file_info
> return (-EALREADY);
> }
> free_pending_request(req); /* immediate success or fail */
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static int arm_unregister(struct file_info *fi, struct pending_request *req)
> @@ -1954,7 +1954,7 @@ static int arm_unregister(struct file_in
> vfree(addr->addr_space_buffer);
> kfree(addr);
> free_pending_request(req); /* immediate success or fail */
> - return sizeof(struct raw1394_request);
> + return 0;
> }
> retval =
> hpsb_unregister_addrspace(&raw1394_highlevel, fi->host,
> @@ -1970,7 +1970,7 @@ static int arm_unregister(struct file_in
> vfree(addr->addr_space_buffer);
> kfree(addr);
> free_pending_request(req); /* immediate success or fail */
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> /* Copy data from ARM buffer(s) to user buffer. */
> @@ -2012,7 +2012,7 @@ static int arm_get_buf(struct file_info
> * queue no response, and therefore nobody
> * will free it. */
> free_pending_request(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> } else {
> DBGMSG("arm_get_buf request exceeded mapping");
> spin_unlock_irqrestore(&host_info_lock, flags);
> @@ -2064,7 +2064,7 @@ static int arm_set_buf(struct file_info
> * queue no response, and therefore nobody
> * will free it. */
> free_pending_request(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> } else {
> DBGMSG("arm_set_buf request exceeded mapping");
> spin_unlock_irqrestore(&host_info_lock, flags);
> @@ -2085,7 +2085,7 @@ static int reset_notification(struct fil
> (req->req.misc == RAW1394_NOTIFY_ON)) {
> fi->notification = (u8) req->req.misc;
> free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
> - return sizeof(struct raw1394_request);
> + return 0;
> }
> /* error EINVAL (22) invalid argument */
> return (-EINVAL);
> @@ -2118,12 +2118,12 @@ static int write_phypacket(struct file_i
> req->req.length = 0;
> queue_complete_req(req);
> }
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> static int get_config_rom(struct file_info *fi, struct pending_request *req)
> {
> - int ret = sizeof(struct raw1394_request);
> + int ret = 0;
> quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
> int status;
>
> @@ -2153,7 +2153,7 @@ static int get_config_rom(struct file_in
>
> static int update_config_rom(struct file_info *fi, struct pending_request *req)
> {
> - int ret = sizeof(struct raw1394_request);
> + int ret = 0;
> quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
> if (!data)
> return -ENOMEM;
> @@ -2220,7 +2220,7 @@ static int modify_config_rom(struct file
>
> hpsb_update_config_rom_image(fi->host);
> free_pending_request(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
> }
>
> @@ -2285,7 +2285,7 @@ static int modify_config_rom(struct file
> /* we have to free the request, because we queue no response,
> * and therefore nobody will free it */
> free_pending_request(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> } else {
> for (dentry =
> fi->csr1212_dirs[dr]->value.directory.dentries_head;
> @@ -2310,7 +2310,7 @@ static int state_connected(struct file_i
>
> case RAW1394_REQ_ECHO:
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
>
> case RAW1394_REQ_ISO_SEND:
> print_old_iso_deprecation();
> @@ -2334,24 +2334,24 @@ static int state_connected(struct file_i
> case RAW1394_REQ_ISO_LISTEN:
> print_old_iso_deprecation();
> handle_iso_listen(fi, req);
> - return sizeof(struct raw1394_request);
> + return 0;
>
> case RAW1394_REQ_FCP_LISTEN:
> handle_fcp_listen(fi, req);
> - return sizeof(struct raw1394_request);
> + return 0;
>
> case RAW1394_REQ_RESET_BUS:
> if (req->req.misc == RAW1394_LONG_RESET) {
> DBGMSG("busreset called (type: LONG)");
> hpsb_reset_bus(fi->host, LONG_RESET);
> free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
> - return sizeof(struct raw1394_request);
> + return 0;
> }
> if (req->req.misc == RAW1394_SHORT_RESET) {
> DBGMSG("busreset called (type: SHORT)");
> hpsb_reset_bus(fi->host, SHORT_RESET);
> free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
> - return sizeof(struct raw1394_request);
> + return 0;
> }
> /* error EINVAL (22) invalid argument */
> return (-EINVAL);
> @@ -2370,7 +2370,7 @@ static int state_connected(struct file_i
> req->req.generation = get_hpsb_generation(fi->host);
> req->req.length = 0;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> switch (req->req.type) {
> @@ -2383,7 +2383,7 @@ static int state_connected(struct file_i
> if (req->req.length == 0) {
> req->req.error = RAW1394_ERROR_INVALID_ARG;
> queue_complete_req(req);
> - return sizeof(struct raw1394_request);
> + return 0;
> }
>
> return handle_async_request(fi, req, node);
> @@ -2394,7 +2394,7 @@ static ssize_t raw1394_write(struct file
> {
> struct file_info *fi = (struct file_info *)file->private_data;
> struct pending_request *req;
> - ssize_t retval = 0;
> + ssize_t retval = -EBADFD;
>
> #ifdef CONFIG_COMPAT
> if (count == sizeof(struct compat_raw1394_req) &&
> @@ -2436,6 +2436,9 @@ static ssize_t raw1394_write(struct file
>
> if (retval < 0) {
> free_pending_request(req);
> + } else {
> + BUG_ON(retval);
> + retval = count;
> }
>
> return retval;
> @@ -2801,6 +2804,99 @@ static int raw1394_ioctl(struct inode *i
> return -EINVAL;
> }
>
> +#ifdef CONFIG_COMPAT
> +struct raw1394_iso_packets32 {
> + __u32 n_packets;
> + compat_uptr_t infos;
> +} __attribute__((packed));
> +
> +struct raw1394_cycle_timer32 {
> + __u32 cycle_timer;
> + __u64 local_time;
> +} __attribute__((packed));
> +
> +#define RAW1394_IOC_ISO_RECV_PACKETS32 \
> + _IOW ('#', 0x25, struct raw1394_iso_packets32)
> +#define RAW1394_IOC_ISO_XMIT_PACKETS32 \
> + _IOW ('#', 0x27, struct raw1394_iso_packets32)
> +#define RAW1394_IOC_GET_CYCLE_TIMER32 \
> + _IOR ('#', 0x30, struct raw1394_cycle_timer32)
> +
> +static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd,
> + struct raw1394_iso_packets32 __user *arg)
> +{
> + compat_uptr_t infos32;
> + void *infos;
> + long err = -EFAULT;
> + struct raw1394_iso_packets __user *dst = compat_alloc_user_space(sizeof(struct raw1394_iso_packets));
> +
> + if (!copy_in_user(&dst->n_packets, &arg->n_packets, sizeof arg->n_packets) &&
> + !copy_from_user(&infos32, &arg->infos, sizeof infos32)) {
> + infos = compat_ptr(infos32);
> + if (!copy_to_user(&dst->infos, &infos, sizeof infos))
> + err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst);
> + }
> + return err;
> +}
> +
> +static long raw1394_read_cycle_timer32(struct file_info *fi, void __user * uaddr)
> +{
> + struct raw1394_cycle_timer32 ct;
> + int err;
> +
> + err = hpsb_read_cycle_timer(fi->host, &ct.cycle_timer, &ct.local_time);
> + if (!err)
> + if (copy_to_user(uaddr, &ct, sizeof(ct)))
> + err = -EFAULT;
> + return err;
> +}
> +
> +static long raw1394_compat_ioctl(struct file *file,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct file_info *fi = file->private_data;
> + void __user *argp = (void __user *)arg;
> + long err;
> +
> + lock_kernel();
> + switch (cmd) {
> + /* These requests have same format as long as 'int' has same size. */
> + case RAW1394_IOC_ISO_RECV_INIT:
> + case RAW1394_IOC_ISO_RECV_START:
> + case RAW1394_IOC_ISO_RECV_LISTEN_CHANNEL:
> + case RAW1394_IOC_ISO_RECV_UNLISTEN_CHANNEL:
> + case RAW1394_IOC_ISO_RECV_SET_CHANNEL_MASK:
> + case RAW1394_IOC_ISO_RECV_RELEASE_PACKETS:
> + case RAW1394_IOC_ISO_RECV_FLUSH:
> + case RAW1394_IOC_ISO_XMIT_RECV_STOP:
> + case RAW1394_IOC_ISO_XMIT_INIT:
> + case RAW1394_IOC_ISO_XMIT_START:
> + case RAW1394_IOC_ISO_XMIT_SYNC:
> + case RAW1394_IOC_ISO_GET_STATUS:
> + case RAW1394_IOC_ISO_SHUTDOWN:
> + case RAW1394_IOC_ISO_QUEUE_ACTIVITY:
> + err = raw1394_ioctl(NULL, file, cmd, arg);
> + break;
> + /* These request have different format. */
> + case RAW1394_IOC_ISO_RECV_PACKETS32:
> + err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_RECV_PACKETS, argp);
> + break;
> + case RAW1394_IOC_ISO_XMIT_PACKETS32:
> + err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_XMIT_PACKETS, argp);
> + break;
> + case RAW1394_IOC_GET_CYCLE_TIMER32:
> + err = raw1394_read_cycle_timer32(fi, argp);
> + break;
> + default:
> + err = -EINVAL;
> + break;
> + }
> + unlock_kernel();
> +
> + return err;
> +}
> +#endif
> +
> static unsigned int raw1394_poll(struct file *file, poll_table * pt)
> {
> struct file_info *fi = file->private_data;
> @@ -3040,7 +3136,9 @@ static const struct file_operations raw1
> .write = raw1394_write,
> .mmap = raw1394_mmap,
> .ioctl = raw1394_ioctl,
> - // .compat_ioctl = ... someone needs to do this
> +#ifdef CONFIG_COMPAT
> + .compat_ioctl = raw1394_compat_ioctl,
> +#endif
> .poll = raw1394_poll,
> .open = raw1394_open,
> .release = raw1394_release,
>
On 7 May, Petr Vandrovec wrote:
> This patch makes raw1394 in current Linux git tree (2.6.21-1570) usable to 32bit
> applications running on 64bit kernel (tested on i386 app using x86_64 kernel).
[...]
> With this in place I was able to run my test app and grab some mpegs, so I believe
> that I got everything necessary done. If you believe I have missed some ioctl, yell
> at me.
Petr, thanks once again for the fixes. I took the liberty to split your
patch into the read(), write(), and ioctl() parts but kept your From:
and Signed-off-by: and Dan's ACK. Is this OK with you? I will reply to
this message with
[PATCH 1/3] ieee1394: raw1394: Fix read() for 32bit userland on 64bit kernel
[PATCH 2/3] ieee1394: raw1394: Fix write() for 32bit userland on 64bit kernel
[PATCH 3/3] ieee1394: raw1394: Add ioctl() for 32bit userland on 64bit kernel
I plan to do some own testing on a x86-64 box on Thursday and will then
pass it upstream ASAP.
--
Stefan Richter
-=====-=-=== -=-= =----
http://arcgraph.de/sr/
Date: Mon, 7 May 2007 04:14:47 +0200
From: Petr Vandrovec <[email protected]>
read() always failed with -EFAULT. This was happening due to
raw1394_compat_read copying data to wrong location - access_ok always
failed as 'r' is kernel address, not user. Whole function just tried to
copy data from 'r' to 'r', which is not good.
Signed-off-by: Petr Vandrovec <[email protected]>
Acked-by: Dan Dennedy <[email protected]>
Signed-off-by: Stefan Richter <[email protected]> (split into 3 patches)
---
drivers/ieee1394/raw1394.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6.22-rc1/drivers/ieee1394/raw1394.c
===================================================================
--- linux-2.6.22-rc1.orig/drivers/ieee1394/raw1394.c
+++ linux-2.6.22-rc1/drivers/ieee1394/raw1394.c
@@ -459,7 +459,7 @@ static const char __user *raw1394_compat
static int
raw1394_compat_read(const char __user *buf, struct raw1394_request *r)
{
- struct compat_raw1394_req __user *cr = (typeof(cr)) r;
+ struct compat_raw1394_req __user *cr = (typeof(cr)) buf;
if (!access_ok(VERIFY_WRITE, cr, sizeof(struct compat_raw1394_req)) ||
P(type) ||
P(error) ||
--
Stefan Richter
-=====-=-=== -=-= =----
http://arcgraph.de/sr/
Date: Mon, 7 May 2007 04:14:47 +0200
From: Petr Vandrovec <[email protected]>
* write(fd, buf, 52) from 32bit app was returning 56. Most of callers did not
care, but some (arm registration) did, and anyway it looks bad if request for
writing 52 bytes returns 56. And returning sizeof anything in 'int' is not
good as well. So all functions now return '0' instead of
sizeof(struct raw1394_request) on success, and write() itself provides correct
return value (it just returns value it was asked to write on success as raw1394
does not do any partial writes at all).
* Related to this was problem that write() could have returned 0 when kernel
state would become corrupted and moved to different state than
opened/initialized/connected. Now it returns -EBADFD which seemed appropriate.
Signed-off-by: Petr Vandrovec <[email protected]>
Acked-by: Dan Dennedy <[email protected]>
Signed-off-by: Stefan Richter <[email protected]> (split into 3 patches)
---
drivers/ieee1394/raw1394.c | 65 +++++++++++++++++++------------------
1 file changed, 34 insertions(+), 31 deletions(-)
Index: linux-2.6.22-rc1/drivers/ieee1394/raw1394.c
===================================================================
--- linux-2.6.22-rc1.orig/drivers/ieee1394/raw1394.c
+++ linux-2.6.22-rc1/drivers/ieee1394/raw1394.c
@@ -587,7 +587,7 @@ static int state_opened(struct file_info
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
static int state_initialized(struct file_info *fi, struct pending_request *req)
@@ -601,7 +601,7 @@ static int state_initialized(struct file
req->req.generation = atomic_read(&internal_generation);
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
switch (req->req.type) {
@@ -673,7 +673,7 @@ out_set_card:
}
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
static void handle_iso_listen(struct file_info *fi, struct pending_request *req)
@@ -865,7 +865,7 @@ static int handle_async_request(struct f
if (req->req.error) {
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
hpsb_set_packet_complete_task(packet,
@@ -883,7 +883,7 @@ static int handle_async_request(struct f
hpsb_free_tlabel(packet);
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int handle_iso_send(struct file_info *fi, struct pending_request *req,
@@ -907,7 +907,7 @@ static int handle_iso_send(struct file_i
req->req.error = RAW1394_ERROR_MEMFAULT;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
req->req.length = 0;
@@ -927,7 +927,7 @@ static int handle_iso_send(struct file_i
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int handle_async_send(struct file_info *fi, struct pending_request *req)
@@ -942,7 +942,7 @@ static int handle_async_send(struct file
req->req.error = RAW1394_ERROR_INVALID_ARG;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
packet = hpsb_alloc_packet(req->req.length - header_length);
@@ -955,7 +955,7 @@ static int handle_async_send(struct file
req->req.error = RAW1394_ERROR_MEMFAULT;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
if (copy_from_user
@@ -964,7 +964,7 @@ static int handle_async_send(struct file
req->req.error = RAW1394_ERROR_MEMFAULT;
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
packet->type = hpsb_async;
@@ -992,7 +992,7 @@ static int handle_async_send(struct file
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int arm_read(struct hpsb_host *host, int nodeid, quadlet_t * buffer,
@@ -1867,7 +1867,7 @@ static int arm_register(struct file_info
spin_lock_irqsave(&host_info_lock, flags);
list_add_tail(&addr->addr_list, &fi->addr_list);
spin_unlock_irqrestore(&host_info_lock, flags);
- return sizeof(struct raw1394_request);
+ return 0;
}
retval =
hpsb_register_addrspace(&raw1394_highlevel, fi->host, &arm_ops,
@@ -1885,7 +1885,7 @@ static int arm_register(struct file_info
return (-EALREADY);
}
free_pending_request(req); /* immediate success or fail */
- return sizeof(struct raw1394_request);
+ return 0;
}
static int arm_unregister(struct file_info *fi, struct pending_request *req)
@@ -1953,7 +1953,7 @@ static int arm_unregister(struct file_in
vfree(addr->addr_space_buffer);
kfree(addr);
free_pending_request(req); /* immediate success or fail */
- return sizeof(struct raw1394_request);
+ return 0;
}
retval =
hpsb_unregister_addrspace(&raw1394_highlevel, fi->host,
@@ -1969,7 +1969,7 @@ static int arm_unregister(struct file_in
vfree(addr->addr_space_buffer);
kfree(addr);
free_pending_request(req); /* immediate success or fail */
- return sizeof(struct raw1394_request);
+ return 0;
}
/* Copy data from ARM buffer(s) to user buffer. */
@@ -2011,7 +2011,7 @@ static int arm_get_buf(struct file_info
* queue no response, and therefore nobody
* will free it. */
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
} else {
DBGMSG("arm_get_buf request exceeded mapping");
spin_unlock_irqrestore(&host_info_lock, flags);
@@ -2063,7 +2063,7 @@ static int arm_set_buf(struct file_info
* queue no response, and therefore nobody
* will free it. */
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
} else {
DBGMSG("arm_set_buf request exceeded mapping");
spin_unlock_irqrestore(&host_info_lock, flags);
@@ -2084,7 +2084,7 @@ static int reset_notification(struct fil
(req->req.misc == RAW1394_NOTIFY_ON)) {
fi->notification = (u8) req->req.misc;
free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
- return sizeof(struct raw1394_request);
+ return 0;
}
/* error EINVAL (22) invalid argument */
return (-EINVAL);
@@ -2117,12 +2117,12 @@ static int write_phypacket(struct file_i
req->req.length = 0;
queue_complete_req(req);
}
- return sizeof(struct raw1394_request);
+ return 0;
}
static int get_config_rom(struct file_info *fi, struct pending_request *req)
{
- int ret = sizeof(struct raw1394_request);
+ int ret = 0;
quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
int status;
@@ -2152,7 +2152,7 @@ static int get_config_rom(struct file_in
static int update_config_rom(struct file_info *fi, struct pending_request *req)
{
- int ret = sizeof(struct raw1394_request);
+ int ret = 0;
quadlet_t *data = kmalloc(req->req.length, GFP_KERNEL);
if (!data)
return -ENOMEM;
@@ -2219,7 +2219,7 @@ static int modify_config_rom(struct file
hpsb_update_config_rom_image(fi->host);
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
}
@@ -2284,7 +2284,7 @@ static int modify_config_rom(struct file
/* we have to free the request, because we queue no response,
* and therefore nobody will free it */
free_pending_request(req);
- return sizeof(struct raw1394_request);
+ return 0;
} else {
for (dentry =
fi->csr1212_dirs[dr]->value.directory.dentries_head;
@@ -2309,7 +2309,7 @@ static int state_connected(struct file_i
case RAW1394_REQ_ECHO:
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
case RAW1394_REQ_ISO_SEND:
print_old_iso_deprecation();
@@ -2333,24 +2333,24 @@ static int state_connected(struct file_i
case RAW1394_REQ_ISO_LISTEN:
print_old_iso_deprecation();
handle_iso_listen(fi, req);
- return sizeof(struct raw1394_request);
+ return 0;
case RAW1394_REQ_FCP_LISTEN:
handle_fcp_listen(fi, req);
- return sizeof(struct raw1394_request);
+ return 0;
case RAW1394_REQ_RESET_BUS:
if (req->req.misc == RAW1394_LONG_RESET) {
DBGMSG("busreset called (type: LONG)");
hpsb_reset_bus(fi->host, LONG_RESET);
free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
- return sizeof(struct raw1394_request);
+ return 0;
}
if (req->req.misc == RAW1394_SHORT_RESET) {
DBGMSG("busreset called (type: SHORT)");
hpsb_reset_bus(fi->host, SHORT_RESET);
free_pending_request(req); /* we have to free the request, because we queue no response, and therefore nobody will free it */
- return sizeof(struct raw1394_request);
+ return 0;
}
/* error EINVAL (22) invalid argument */
return (-EINVAL);
@@ -2369,7 +2369,7 @@ static int state_connected(struct file_i
req->req.generation = get_hpsb_generation(fi->host);
req->req.length = 0;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
switch (req->req.type) {
@@ -2382,7 +2382,7 @@ static int state_connected(struct file_i
if (req->req.length == 0) {
req->req.error = RAW1394_ERROR_INVALID_ARG;
queue_complete_req(req);
- return sizeof(struct raw1394_request);
+ return 0;
}
return handle_async_request(fi, req, node);
@@ -2393,7 +2393,7 @@ static ssize_t raw1394_write(struct file
{
struct file_info *fi = (struct file_info *)file->private_data;
struct pending_request *req;
- ssize_t retval = 0;
+ ssize_t retval = -EBADFD;
#ifdef CONFIG_COMPAT
if (count == sizeof(struct compat_raw1394_req) &&
@@ -2435,6 +2435,9 @@ static ssize_t raw1394_write(struct file
if (retval < 0) {
free_pending_request(req);
+ } else {
+ BUG_ON(retval);
+ retval = count;
}
return retval;
--
Stefan Richter
-=====-=-=== -=-= =----
http://arcgraph.de/sr/
Date: Mon, 7 May 2007 04:14:47 +0200
From: Petr Vandrovec <[email protected]>
Add compat_ioctl. Although all structures are more or less same,
raw1394_iso_packets got pointer inside, and raw1394_cycle_timer got unwanted
padding in the middle. I did not add any translation for ioctls passing array
of integers around as integers seem to have same size (32 bits) on all
architectures supported by Linux.
Signed-off-by: Petr Vandrovec <[email protected]>
Acked-by: Dan Dennedy <[email protected]>
Signed-off-by: Stefan Richter <[email protected]> (split into 3 patches)
---
drivers/ieee1394/raw1394.c | 97 ++++++++++++++++++++++++++++++++++++-
1 file changed, 96 insertions(+), 1 deletion(-)
patch update: omitted trailing whitespace
Index: linux-2.6.22-rc1/drivers/ieee1394/raw1394.c
===================================================================
--- linux-2.6.22-rc1.orig/drivers/ieee1394/raw1394.c
+++ linux-2.6.22-rc1/drivers/ieee1394/raw1394.c
@@ -2803,6 +2803,99 @@ static int raw1394_ioctl(struct inode *i
return -EINVAL;
}
+#ifdef CONFIG_COMPAT
+struct raw1394_iso_packets32 {
+ __u32 n_packets;
+ compat_uptr_t infos;
+} __attribute__((packed));
+
+struct raw1394_cycle_timer32 {
+ __u32 cycle_timer;
+ __u64 local_time;
+} __attribute__((packed));
+
+#define RAW1394_IOC_ISO_RECV_PACKETS32 \
+ _IOW ('#', 0x25, struct raw1394_iso_packets32)
+#define RAW1394_IOC_ISO_XMIT_PACKETS32 \
+ _IOW ('#', 0x27, struct raw1394_iso_packets32)
+#define RAW1394_IOC_GET_CYCLE_TIMER32 \
+ _IOR ('#', 0x30, struct raw1394_cycle_timer32)
+
+static long raw1394_iso_xmit_recv_packets32(struct file *file, unsigned int cmd,
+ struct raw1394_iso_packets32 __user *arg)
+{
+ compat_uptr_t infos32;
+ void *infos;
+ long err = -EFAULT;
+ struct raw1394_iso_packets __user *dst = compat_alloc_user_space(sizeof(struct raw1394_iso_packets));
+
+ if (!copy_in_user(&dst->n_packets, &arg->n_packets, sizeof arg->n_packets) &&
+ !copy_from_user(&infos32, &arg->infos, sizeof infos32)) {
+ infos = compat_ptr(infos32);
+ if (!copy_to_user(&dst->infos, &infos, sizeof infos))
+ err = raw1394_ioctl(NULL, file, cmd, (unsigned long)dst);
+ }
+ return err;
+}
+
+static long raw1394_read_cycle_timer32(struct file_info *fi, void __user * uaddr)
+{
+ struct raw1394_cycle_timer32 ct;
+ int err;
+
+ err = hpsb_read_cycle_timer(fi->host, &ct.cycle_timer, &ct.local_time);
+ if (!err)
+ if (copy_to_user(uaddr, &ct, sizeof(ct)))
+ err = -EFAULT;
+ return err;
+}
+
+static long raw1394_compat_ioctl(struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ struct file_info *fi = file->private_data;
+ void __user *argp = (void __user *)arg;
+ long err;
+
+ lock_kernel();
+ switch (cmd) {
+ /* These requests have same format as long as 'int' has same size. */
+ case RAW1394_IOC_ISO_RECV_INIT:
+ case RAW1394_IOC_ISO_RECV_START:
+ case RAW1394_IOC_ISO_RECV_LISTEN_CHANNEL:
+ case RAW1394_IOC_ISO_RECV_UNLISTEN_CHANNEL:
+ case RAW1394_IOC_ISO_RECV_SET_CHANNEL_MASK:
+ case RAW1394_IOC_ISO_RECV_RELEASE_PACKETS:
+ case RAW1394_IOC_ISO_RECV_FLUSH:
+ case RAW1394_IOC_ISO_XMIT_RECV_STOP:
+ case RAW1394_IOC_ISO_XMIT_INIT:
+ case RAW1394_IOC_ISO_XMIT_START:
+ case RAW1394_IOC_ISO_XMIT_SYNC:
+ case RAW1394_IOC_ISO_GET_STATUS:
+ case RAW1394_IOC_ISO_SHUTDOWN:
+ case RAW1394_IOC_ISO_QUEUE_ACTIVITY:
+ err = raw1394_ioctl(NULL, file, cmd, arg);
+ break;
+ /* These request have different format. */
+ case RAW1394_IOC_ISO_RECV_PACKETS32:
+ err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_RECV_PACKETS, argp);
+ break;
+ case RAW1394_IOC_ISO_XMIT_PACKETS32:
+ err = raw1394_iso_xmit_recv_packets32(file, RAW1394_IOC_ISO_XMIT_PACKETS, argp);
+ break;
+ case RAW1394_IOC_GET_CYCLE_TIMER32:
+ err = raw1394_read_cycle_timer32(fi, argp);
+ break;
+ default:
+ err = -EINVAL;
+ break;
+ }
+ unlock_kernel();
+
+ return err;
+}
+#endif
+
static unsigned int raw1394_poll(struct file *file, poll_table * pt)
{
struct file_info *fi = file->private_data;
@@ -3042,7 +3135,9 @@ static const struct file_operations raw1
.write = raw1394_write,
.mmap = raw1394_mmap,
.ioctl = raw1394_ioctl,
- // .compat_ioctl = ... someone needs to do this
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = raw1394_compat_ioctl,
+#endif
.poll = raw1394_poll,
.open = raw1394_open,
.release = raw1394_release,
--
Stefan Richter
-=====-=-=== -=-= =----
http://arcgraph.de/sr/
Stefan Richter wrote:
> On 7 May, Petr Vandrovec wrote:
>> This patch makes raw1394 in current Linux git tree (2.6.21-1570) usable to 32bit
>> applications running on 64bit kernel (tested on i386 app using x86_64 kernel).
> [...]
>> With this in place I was able to run my test app and grab some mpegs, so I believe
>> that I got everything necessary done. If you believe I have missed some ioctl, yell
>> at me.
>
> Petr, thanks once again for the fixes. I took the liberty to split your
> patch into the read(), write(), and ioctl() parts but kept your From:
> and Signed-off-by: and Dan's ACK. Is this OK with you? I will reply to
> this message with
Yes, of course. Thanks a lot.
> [PATCH 1/3] ieee1394: raw1394: Fix read() for 32bit userland on 64bit kernel
> [PATCH 2/3] ieee1394: raw1394: Fix write() for 32bit userland on 64bit kernel
> [PATCH 3/3] ieee1394: raw1394: Add ioctl() for 32bit userland on 64bit kernel
>
> I plan to do some own testing on a x86-64 box on Thursday and will then
> pass it upstream ASAP.
Thanks.
Petr
On Monday 07 May 2007, Petr Vandrovec wrote:
> +struct raw1394_cycle_timer32 {
> + ? ? ? ?__u32 cycle_timer;
> + ? ? ? ?__u64 local_time;
> +} __attribute__((packed));
Note that this data structure only needs conversion on x86_64 and ia64, but
not on powerpc and other 64 bit architectures that align __u64 also in
32 bit mode.
I would suggest you introduce a new __compat_u64 type as
typedef __u64 __compat_u64 __attribute__((aligned(4)));
in include/asm-{x86_64,ia64}/compat.h and as
typdef __u64 __compat_u64;
in the other architectures. Other people have hit the same problem
before and found varying workarounds, but I think we should just
do it correctly now.
Arnd <><
Arnd Bergmann wrote:
> On Monday 07 May 2007, Petr Vandrovec wrote:
>> +struct raw1394_cycle_timer32 {
>> + __u32 cycle_timer;
>> + __u64 local_time;
>> +} __attribute__((packed));
>
> Note that this data structure only needs conversion on x86_64 and ia64, but
> not on powerpc and other 64 bit architectures that align __u64 also in
> 32 bit mode.
Is this conversion just unnecessary or actually harmful on ppc64 and others?
--
Stefan Richter
-=====-=-=== -=-= =-=--
http://arcgraph.de/sr/
On Sunday 20 May 2007, Stefan Richter wrote:
>
> > Note that this data structure only needs conversion on x86_64 and ia64, but
> > not on powerpc and other 64 bit architectures that align __u64 also in
> > 32 bit mode.
>
> Is this conversion just unnecessary or actually harmful on ppc64 and others?
With the current patch, the compat_ioctl function does not handle the ppc32
version of the structure at all, so it's broken there, it would at least
need a
case RAW1394_IOC_GET_CYCLE_TIMER:
err = raw1394_ioctl(NULL, file, cmd, arg);
break;
Arnd <><
Arnd Bergmann wrote:
> On Sunday 20 May 2007, Stefan Richter wrote:
>>> Note that this data structure only needs conversion on x86_64 and ia64, but
>>> not on powerpc and other 64 bit architectures that align __u64 also in
>>> 32 bit mode.
>> Is this conversion just unnecessary or actually harmful on ppc64 and others?
>
> With the current patch, the compat_ioctl function does not handle the ppc32
> version of the structure at all, so it's broken there, it would at least
> need a
>
> case RAW1394_IOC_GET_CYCLE_TIMER:
> err = raw1394_ioctl(NULL, file, cmd, arg);
> break;
Dan,
maybe we should change
/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
struct raw1394_cycle_timer {
/* contents of Isochronous Cycle Timer register,
as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
__u32 cycle_timer;
/* local time in microseconds since Epoch,
simultaneously read with cycle timer */
__u64 local_time;
};
to
/* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
struct raw1394_cycle_timer {
/*
* least significant 32 bits are contents of Isochronous Cycle
* Timer register, as in OHCI 1.1 clause 5.13 (also with
* non-OHCI hosts)
*/
__u64 cycle_timer;
/*
* local time in microseconds since Epoch,
* simultaneously read with cycle timer
*/
__u64 local_time;
};
before a libraw1394 with get-cycle-timer support is released.
Shall I prepare according patches for raw1394 and libraw1394?
--
Stefan Richter
-=====-=-=== -=-= =-=--
http://arcgraph.de/sr/
On Sunday 20 May 2007, Stefan Richter wrote:
> maybe we should change
>
> /* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> struct raw1394_cycle_timer {
> ????????/* contents of Isochronous Cycle Timer register,
> ???????? ? as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
> ????????__u32 cycle_timer;
>
> ????????/* local time in microseconds since Epoch,
> ???????? ? simultaneously read with cycle timer */
> ????????__u64 local_time;
> };
>
> to
>
> /* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> struct raw1394_cycle_timer {
> ????????/*
> ???????? * least significant 32 bits are contents of Isochronous Cycle
> ???????? * Timer register, as in OHCI 1.1 clause 5.13 (also with
> ???????? * non-OHCI hosts)
> ???????? */
> ????????__u64 cycle_timer;
>
> ????????/*
> ???????? * local time in microseconds since Epoch,
> ???????? * simultaneously read with cycle timer
> ???????? */
> ????????__u64 local_time;
> };
>
> before a libraw1394 with get-cycle-timer support is released.
Yes, if you still have the chance to change this without breaking
users, that would be ideal.
I assume that struct raw1394_iso_packets is already set in stone,
right? Otherwise it would be good to make that a compatible structure
as well.
Arnd <><
Arnd Bergmann wrote:
> On Sunday 20 May 2007, Stefan Richter wrote:
>> maybe we should change
...
>> struct raw1394_cycle_timer {
...
>> before a libraw1394 with get-cycle-timer support is released.
>
> Yes, if you still have the chance to change this without breaking
> users, that would be ideal.
>
> I assume that struct raw1394_iso_packets is already set in stone,
> right? Otherwise it would be good to make that a compatible structure
> as well.
Alas it's a few years too late for raw1394_iso_packets.
--
Stefan Richter
-=====-=-=== -=-= =-=--
http://arcgraph.de/sr/
I wrote:
> Arnd Bergmann wrote:
>>>> Note that this data structure only needs conversion on x86_64 and ia64, but
>>>> not on powerpc and other 64 bit architectures that align __u64 also in
>>>> 32 bit mode.
...
> maybe we should change
>
> /* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> struct raw1394_cycle_timer {
...
> __u32 cycle_timer;
...
> __u64 local_time;
> };
>
> to
>
> /* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> struct raw1394_cycle_timer {
...
> __u64 cycle_timer;
...
> __u64 local_time;
> };
>
> before a libraw1394 with get-cycle-timer support is released.
On the other hand, we could handle it in the compat code alone and leave
the rest as-is.
/* PPC32 aligns this at 64bit, IA32 packs it */
struct raw1394_cycle_timer32 {
__u32 cycle_timer;
__u64 local_time;
}
#if defined(CONFIG_X86_64) || defined(CONFIG_IA64)
__attribute__((packed))
#endif
;
Eventually, Arnd's suggestion
> I would suggest you introduce a new __compat_u64 type as
>
> typedef __u64 __compat_u64 __attribute__((aligned(4)));
>
> in include/asm-{x86_64,ia64}/compat.h and as
>
> typdef __u64 __compat_u64;
>
> in the other architectures. Other people have hit the same problem
> before and found varying workarounds, but I think we should just
> do it correctly now.
should be put into practice though.
--
Stefan Richter
-=====-=-=== -=-= =-=-=
http://arcgraph.de/sr/
Pointed out by Arnd Bergmann: PPC32 aligns this at 64bit, IA32 packs
it. A kernel-wide available __compat_u64 which is 4-byte aligned on
AMD64 and IA64 would be nicer though.
Signed-off-by: Stefan Richter <[email protected]>
---
drivers/ieee1394/raw1394.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Index: linux/drivers/ieee1394/raw1394.c
===================================================================
--- linux.orig/drivers/ieee1394/raw1394.c
+++ linux/drivers/ieee1394/raw1394.c
@@ -2814,7 +2814,11 @@ struct raw1394_iso_packets32 {
struct raw1394_cycle_timer32 {
__u32 cycle_timer;
__u64 local_time;
-} __attribute__((packed));
+}
+#if defined(CONFIG_X86_64) || defined(CONFIG_IA64)
+__attribute__((packed))
+#endif
+;
#define RAW1394_IOC_ISO_RECV_PACKETS32 \
_IOW ('#', 0x25, struct raw1394_iso_packets32)
--
Stefan Richter
-=====-=-=== -=-= =-=-=
http://arcgraph.de/sr/
On Sunday 20 May 2007 08:28, Stefan Richter wrote:
> Arnd Bergmann wrote:
> > On Sunday 20 May 2007, Stefan Richter wrote:
> >>> Note that this data structure only needs conversion on x86_64 and ia64,
but
> >>> not on powerpc and other 64 bit architectures that align __u64 also in
> >>> 32 bit mode.
> >> Is this conversion just unnecessary or actually harmful on ppc64 and
others?
> >
> > With the current patch, the compat_ioctl function does not handle the
ppc32
> > version of the structure at all, so it's broken there, it would at least
> > need a
> >
> > case RAW1394_IOC_GET_CYCLE_TIMER:
> > err = raw1394_ioctl(NULL, file, cmd, arg);
> > break;
>
> Dan,
>
> maybe we should change
>
> /* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> struct raw1394_cycle_timer {
> /* contents of Isochronous Cycle Timer register,
> as in OHCI 1.1 clause 5.13 (also with non-OHCI hosts) */
> __u32 cycle_timer;
>
> /* local time in microseconds since Epoch,
> simultaneously read with cycle timer */
> __u64 local_time;
> };
>
> to
>
> /* argument to RAW1394_IOC_GET_CYCLE_TIMER ioctl */
> struct raw1394_cycle_timer {
> /*
> * least significant 32 bits are contents of Isochronous Cycle
> * Timer register, as in OHCI 1.1 clause 5.13 (also with
> * non-OHCI hosts)
> */
> __u64 cycle_timer;
>
> /*
> * local time in microseconds since Epoch,
> * simultaneously read with cycle timer
> */
> __u64 local_time;
> };
>
> before a libraw1394 with get-cycle-timer support is released.
> Shall I prepare according patches for raw1394 and libraw1394?
This seems like a good idea, and I have forwarded it to the ffado (formerly
freebob) developers, specifically Pieter Palmers--the submitter of
raw1394_read_cycle_timer--to get his feedback.
Dan Dennedy wrote:
> On Sunday 20 May 2007 08:28, Stefan Richter wrote:
>> maybe we should change
...
>> struct raw1394_cycle_timer {
[to consist of two u64 to get same alignment on all architectures]
...
>> before a libraw1394 with get-cycle-timer support is released.
>> Shall I prepare according patches for raw1394 and libraw1394?
>
> This seems like a good idea, and I have forwarded it to the ffado (formerly
> freebob) developers, specifically Pieter Palmers--the submitter of
> raw1394_read_cycle_timer--to get his feedback.
I'll post the patches tonight to see how it will look like. Even though
I already posted the kernel-only patch which adjusts
raw1394_iso_packets32 per architecture, the 2x u64 variant may overall
be nicer: a) it eliminates the need for raw1394_cycle_timer32, b) the
get-cycle-timer ABI still has to be duplicated in Juju, and Juju so far
did not require different struct types for 32bit compatibility ioctls.
--
Stefan Richter
-=====-=-=== -=-= =-===
http://arcgraph.de/sr/