firewire: cdev: return -ENOTTY for unimplemented ioctls, not -EINVAL
PATCH 1/1 firewire: cdev: return -ENOTTY for unimplemented ioctls, not -EINVAL
Quoted 5 lines · On Jun 27 Linus Torvalds wrote:
The correct error code for "I don't understand this ioctl" is ENOTTY. The naming may be odd, but you should think of that error value as a "unrecognized ioctl number, you're feeding me random numbers that I don't understand and I assume for historical reasons that you tried to do some tty operation on me".
[...]
Quoted 13 lines
The EINVAL thing goes way back, and is a disaster. It predates Linux itself, as far as I can tell. You'll find lots of man-pages that have this line in it:
EINVAL Request or argp is not valid.
and it shows up in POSIX etc. And sadly, it generally shows up _before_ the line that says
ENOTTY The specified request does not apply to the kind of object
that the descriptor d references.so a lot of people get to the EINVAL, and never even notice the ENOTTY.
[...]
Quoted 3 lines
At least glibc (and hopefully other C libraries) use a _string_ that makes much more sense: strerror(ENOTTY) is "Inappropriate ioctl for device"
So let's correct this in the <linux/firewire-cdev.h> ABI while it is still young, relative to distributor adoption.
Side note: We return -ENOTTY not only on _IOC_TYPE or _IOC_NR mismatch, but also on _IOC_SIZE mismatch. An ioctl with an unsupported size of argument structure can be seen as an unsupported version of that ioctl.
--- drivers/firewire/core-cdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firewire/core-cdev.c +1 -1
@@ -1583,7 +1583,7 @@ static int dispatch_ioctl(struct clientif (_IOC_TYPE (cmd) !='#' ||_IOC_NR (cmd) >=ARRAY_SIZE (ioctl_handlers) ||_IOC_SIZE (cmd) >sizeof (buffer)) -return -EINVAL ; +return -ENOTTY ;if (_IOC_DIR (cmd) == _IOC_READ)memset (&buffer,0 ,_IOC_SIZE (cmd));
Signature · 3 lines
Stefan Richter -=====-==-== -=== -=--= http://arcgraph.de/sr/
PATCH 1/1 firewire: cdev: prevent race between first get_info ioctl and bus reset event queuing
Between open(2) of a /dev/fw* and the first FW_CDEV_IOC_GET_INFO ioctl(2) on it, the kernel already queues FW_CDEV_EVENT_BUS_RESET events to be read(2) by the client. The get_info ioctl is practically always issued right away after open, hence this condition only occurs if the client opens during a bus reset, especially during a rapid series of bus resets.
The problem with this condition is two-fold:
- These bus reset events carry the (as yet undocumented) @closure value of 0. But it is not the kernel's place to choose closures; they are privat to the client. E.g., this 0 value forced from the kernel makes it unsafe for clients to dereference it as a pointer to a closure object without NULL pointer check.
- It is impossible for clients to determine the relative order of bus reset events from get_info ioctl(2) versus those from read(2), except in one way: By comparison of closure values. Again, such a procedure imposes complexity on clients and reduces freedom in use of the bus reset closure.
So, change the ABI to suppress queuing of bus reset events before the first FW_CDEV_IOC_GET_INFO ioctl was issued by the client.
Note, this ABI change cannot be version-controlled. The kernel cannot distinguish old from new clients before the first FW_CDEV_IOC_GET_INFO ioctl.
We will try to back-merge this change into currently maintained stable/ longterm series, and we only document the new behaviour. The old behavior is now considered a kernel bug, which it basically is.
--- drivers/firewire/core-cdev.c | 18 ++++++++++-------- include/linux/firewire-cdev.h | 3 +++ 2 files changed, 13 insertions(+), 8 deletions(-)
drivers/firewire/core-cdev.c +10 -8
@@ -253,14 +253,11 @@ static int fw_device_op_open(struct inodinit_waitqueue_head (&client->wait);init_waitqueue_head (&client->tx_flush_wait);INIT_LIST_HEAD (&client->phy_receiver_link); +INIT_LIST_HEAD (&client->link);kref_init (&client->kref); file->private_data = client; -mutex_lock (&device->client_list_mutex); -list_add_tail (&client->link, &device->client_list); -mutex_unlock (&device->client_list_mutex); -return nonseekable_open (inode, file); }
@@ -451,15 +448,20 @@ static int ioctl_get_info(struct clientif (ret !=0 )return -EFAULT ; +mutex_lock (&client->device->client_list_mutex); + client->bus_reset_closure = a->bus_reset_closure;if (a->bus_reset !=0 ) {fill_bus_reset_event (&bus_reset, client); -if (copy_to_user (u64_to_uptr (a->bus_reset), - &bus_reset,sizeof (bus_reset))) -return -EFAULT ; + ret =copy_to_user (u64_to_uptr (a->bus_reset), + &bus_reset,sizeof (bus_reset)); } +if (ret ==0 &&list_empty (&client->link)) +list_add_tail (&client->link, &client->device->client_list); -return 0 ; +mutex_unlock (&client->device->client_list_mutex); + +return ret ? -EFAULT :0 ; } static int add_client_resource(struct client *client,
include/linux/firewire-cdev.h +3 -0
@@ -475,6 +475,9 @@ union fw_cdev_event {* of the bus. This does not cause a bus reset to happen. * @bus_reset_closure: Value of &closure in this and subsequent bus reset events * @card: The index of the card this device belongs to +* +* As a side effect, reception of %FW_CDEV_EVENT_BUS_RESET events to be read(2) +* is started by this ioctl. */ struct fw_cdev_get_info {__u32 version;
Signature · 3 lines
Stefan Richter -=====-==-== -=== -=--= http://arcgraph.de/sr/
PATCH 1/1 firewire: cdev: ABI documentation enhancements
Add overview documentation in Documentation/ABI/stable/firewire-cdev.
Improve the inline reference documentation in firewire-cdev.h:
- Add /* available since kernel... */ comments to event numbers consistent with the comments on ioctl numbers.
- Shorten some documentation on an event and an ioctl that are less interesting to current programming because there are newer preferable variants.
- Spell Configuration ROM (name of an IEEE 1212 register) in upper case.
- Move the dummy FW_CDEV_VERSION out of the reader's field of vision. We should remove it from the header next year or so.
---
applicable after patch
"firewire: cdev: prevent race between first get_info ioctl and bus reset event queuing"
Documentation/ABI/stable/firewire-cdev | 103 +++++++++++++++++++++++++ include/linux/firewire-cdev.h | 77 +++++++++---------- 2 files changed, 138 insertions(+), 42 deletions(-)
Documentation/ABI/stable/firewire-cdev new file +103 -0
@@ -0,0 +1,103 @@ +What: /dev/fw[0-9]+ +Date: May 2007 +KernelVersion: 2.6.22 +Contact: [email protected] +Description: + The character device files /dev/fw* are the interface between + firewire-core and IEEE 1394 device drivers implemented in + userspace. The ioctl(2)- and read(2)-based ABI is defined and + documented in <linux/firewire-cdev.h>. + + This ABI offers most of the features which firewire-core also + exposes to kernelspace IEEE 1394 drivers. + + Each /dev/fw* is associated with one IEEE 1394 node, which can + be remote or local nodes. Operations on a /dev/fw* file have + different scope: + - The 1394 node which is associated with the file: + - Asynchronous request transmission + - Get the Configuration ROM + - Query node ID + - Query maximum speed of the path between this node + and local node + - The 1394 bus (i.e. "card") to which the node is attached to: + - Isochronous stream transmission and reception + - Asynchronous stream transmission and reception + - Asynchronous broadcast request transmission + - PHY packet transmission and reception + - Allocate, reallocate, deallocate isochronous + resources (channels, bandwidth) at the bus's IRM + - Query node IDs of local node, root node, IRM, bus + manager + - Query cycle time + - Bus reset initiation, bus reset event reception + - All 1394 buses: + - Allocation of IEEE 1212 address ranges on the local + link layers, reception of inbound requests to such + an address range, asynchronous response transmission + to inbound requests + - Addition of descriptors or directories to the local + nodes' Configuration ROM + + Due to the different scope of operations and in order to let + userland implement different access permission models, some + operations are restricted to /dev/fw* files that are associated + with a local node: + - Addition of descriptors or directories to the local + nodes' Configuration ROM + - PHY packet transmission and reception + + A /dev/fw* file remains associated with one particular node + during its entire life time. Bus topology changes, and hence + node ID changes, are tracked by firewire-core. ABI users do not + need to be aware of topology. + + The following file operations are supported: + + open(2) + Currently the only useful flags are O_RDWR. + + ioctl(2) + Initiate various actions. Some take immediate effect, others + are performed asynchronously while or after the ioctl returns. + See the inline documentation in <linux/firewire-cdev.h> for + descriptions of all ioctls. + + poll(2), epoll(2) + Watch for events to become available to be read. + + read(2) + Receive various events. There are solicited events like + outbound asynchronous transaction completion, and unsolicited + events such as bus reset events, isochronous DMA buffer + completion, or PHY packet reception. Always use a read buffer + which is large enough to receive the largest event that could + ever arrive. See the documentation in <linux/firewire-cdev.h> + for descriptions of all event types and for which ioctls affect + reception of events. + + mmap(2) + Allocate a DMA buffer for isochronous reception or transmission + and map it into the process address space. The arguments should + be used as follows: addr = NULL, length = the desired buffer + size, i.e. number of packets times size of largest packet, + prot = at least PROT_READ for reception and at least PROT_WRITE + for transmission, flags = MAP_SHARED, fd = the handle to the + /dev/fw*, offset = 0. + + Isochronous reception works in packet-per-buffer fashion except + for multichannel reception which works in buffer-fill mode. + + munmap(2) + Unmap the isochronous I/O buffer from the process address space. + + close(2) + Besides stopping and freeing I/O contexts that were associated + with the file descriptor, back out any changes to the local + nodes' Configuration ROM. Deallocate isochronous channels and + bandwidth at the IRM that were marked for kernel-assisted + re- and deallocation. + +Users: libraw1394 + libdc1394 + tools like jujuutils, fwhack, ...
include/linux/firewire-cdev.h +35 -42
162 lines not on this page · raw message
Signature · 3 lines
Stefan Richter -=====-==-== -=== -=--= http://arcgraph.de/sr/
PATCH 1/1 firewire: document the sysfs ABIs
of firewire-core and firewire-sbp2.
--- Documentation/ABI/stable/sysfs-bus-firewire | 122 ++++++++++++++++++++ 1 file changed, 122 insertions(+)
Documentation/ABI/stable/sysfs-bus-firewire new file +122 -0
@@ -0,0 +1,122 @@ +What: /sys/bus/firewire/devices/fw[0-9]+/ +Date: May 2007 +KernelVersion: 2.6.22 +Contact: [email protected] +Description: + IEEE 1394 node device attributes. + Read-only. Mutable during the node device's lifetime. + See IEEE 1212 for semantic definitions. + + config_rom + Contents of the Configuration ROM register. + Binary attribute; an array of host-endian u32. + + guid + The node's EUI-64 in the bus information block of + Configuration ROM. + Hexadecimal string representation of an u64. + + +What: /sys/bus/firewire/devices/fw[0-9]+/units +Date: June 2009 +KernelVersion: 2.6.31 +Contact: [email protected] +Description: + IEEE 1394 node device attribute. + Read-only. Mutable during the node device's lifetime. + See IEEE 1212 for semantic definitions. + + units + Summary of all units present in an IEEE 1394 node. + Contains space-separated tuples of specifier_id and + version of each unit present in the node. Specifier_id + and version are hexadecimal string representations of + u24 of the respective unit directory entries. + Specifier_id and version within each tuple are separated + by a colon. + +Users: udev rules to set ownership and access permissions or ACLs of + /dev/fw[0-9]+ character device files + + +What: /sys/bus/firewire/devices/fw[0-9]+[.][0-9]+/ +Date: May 2007 +KernelVersion: 2.6.22 +Contact: [email protected] +Description: + IEEE 1394 unit device attributes. + Read-only. Immutable during the unit device's lifetime. + See IEEE 1212 for semantic definitions. + + modalias + Same as MODALIAS in the uevent at device creation. + + rom_index + Offset of the unit directory within the parent device's + (node device's) Configuration ROM, in quadlets. + Decimal string representation. + + +What: /sys/bus/firewire/devices/*/ +Date: May 2007 +KernelVersion: 2.6.22 +Contact: [email protected] +Description: + Attributes common to IEEE 1394 node devices and unit devices. + Read-only. Mutable during the node device's lifetime. + Immutable during the unit device's lifetime. + See IEEE 1212 for semantic definitions. + + These attributes are only created if the root directory of an + IEEE 1394 node or the unit directory of an IEEE 1394 unit + actually contains according entries. + + hardware_version + Hexadecimal string representation of an u24. + + hardware_version_name + Contents of a respective textual descriptor leaf. + + model + Hexadecimal string representation of an u24. + + model_name + Contents of a respective textual descriptor leaf. + + specifier_id + Hexadecimal string representation of an u24. + Mandatory in unit directories according to IEEE 1212. + + vendor + Hexadecimal string representation of an u24. + Mandatory in the root directory according to IEEE 1212. + + vendor_name + Contents of a respective textual descriptor leaf. + + version + Hexadecimal string representation of an u24. + Mandatory in unit directories according to IEEE 1212. + + +What: /sys/bus/firewire/drivers/sbp2/fw*/host*/target*/*:*:*:*/ieee1394_id + formerly + /sys/bus/ieee1394/drivers/sbp2/fw*/host*/target*/*:*:*:*/ieee1394_id +Date: Feb 2004 +KernelVersion: 2.6.4 +Contact: [email protected] +Description: + SCSI target port identifier and logical unit identifier of a + logical unit of an SBP-2 target. The identifiers are specified + in SAM-2...SAM-4 annex A. They are persistent and world-wide + unique properties the SBP-2 attached target. + + Read-only attribute, immutable during the target's lifetime. + Format, as exposed by firewire-sbp2 since 2.6.22, May 2007: + Colon-separated hexadecimal string representations of + u64 EUI-64 : u24 directory_ID : u16 LUN + without 0x prefixes, without whitespace. The former sbp2 driver + (removed in 2.6.37 after being superseded by firewire-sbp2) used + a somewhat shorter format which was not as close to SAM. + +Users: udev rules to create /dev/disk/by-id/ symlinks
Signature · 3 lines
Stefan Richter -=====-==-== -=== -=--= http://arcgraph.de/sr/
Re: [PATCH] firewire: cdev: ABI documentation enhancements
In reply to Stefan Richterinclude/linux/firewire-cdev.h struct fw_cdev_remove_descriptor { · line 667
Quoted 9 lines · On Jul 09 Stefan Richter wrote:
include/linux/firewire-cdev.h +1
-1@@ -672,7 +661,7 @@ struct fw_cdev_remove_descriptor {* @handle: Handle to context, written back by kernel * * Prior to sending or receiving isochronous I/O, a context must be created. -* The context records information about the transmit or receive configuration +* The context records information about the transmit or receive Configuration * and typically maps to an underlying hardware resource. A context is set up * for either sending or receiving. It is bound to a specific isochronous * @channel.
Disregard this hunk.
Signature · 3 lines
Stefan Richter -=====-==-== -=== -=--= http://arcgraph.de/sr/