| Age | Commit message (Collapse) | Author |
|---|
|
Clemens points out that we need to use compat_ptr() in order to safely
cast from u64 to addresses of a 32-bit usermode client.
Before, our conversion went wrong
- in practice if the client cast from pointer to integer such that
sign-extension happened, (libraw1394 and libdc1394 at least were not
doing that, IOW were not affected)
or
- in theory on s390 (which doesn't have FireWire though) and on the
tile architecture, regardless of what the client does.
The bug would usually be observed as the initial get_info ioctl failing
with "Bad address" (EFAULT).
Reported-by: Carl Karsten <carl@personnelware.com>
Reported-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
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 twofold:
- 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.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: <stable@kernel.org>
|
|
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".
[...]
> 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.
[...]
> 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.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: <stable@kernel.org>
|
|
The struct sbp2_logical_unit.work items can all be executed in parallel
but are not reentrant. Furthermore, reconnect or re-login work must be
executed in a WQ_MEM_RECLAIM workqueue.
Hence replace the old single-threaded firewire-sbp2 workqueue by a
concurrency-managed but non-reentrant workqueue with rescuer.
firewire-core already maintains one, hence use this one.
In earlier versions of this change, I observed occasional failures of
parallel INQUIRY to an Initio INIC-2430 FireWire 800 to dual IDE bridge.
More testing indicates that parallel INQUIRY is not actually a problem,
but too quick successions of logout and login + INQUIRY, e.g. a quick
sequence of cable plugout and plugin, can result in failed INQUIRY.
This does not seem to be something that should or could be addressed by
serialization.
Another dual-LU device to which I currently have access to, an
OXUF924DSB FireWire 800 to dual SATA bridge with firmware from MacPower,
has been successfully tested with this too.
This change is beneficial to environments with two or more FireWire
storage devices, especially if they are located on the same bus.
Management tasks that should be performed as soon and as quickly as
possible, especially reconnect, are no longer held up by tasks on other
devices that may take a long time, especially login with INQUIRY and sd
or sr driver probe.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
firewire-core manages the following types of work items:
fw_card.br_work:
- resets the bus on a card and possibly sends a PHY packet before that
- does not sleep for long or not at all
- is scheduled via fw_schedule_bus_reset() by
- firewire-ohci's pci_probe method
- firewire-ohci's set_config_rom method, called by kernelspace
protocol drivers and userspace drivers which add/remove
Configuration ROM descriptors
- userspace drivers which use the bus reset ioctl
- itself if the last reset happened less than 2 seconds ago
fw_card.bm_work:
- performs bus management duties
- usually does not (but may in corner cases) sleep for long
- is scheduled via fw_schedule_bm_work() by
- firewire-ohci's self-ID-complete IRQ handler tasklet
- firewire-core's fw_device.work instances whenever the root node
device was (successfully or unsuccessfully) discovered,
refreshed, or rediscovered
- itself in case of resource allocation failures or in order to
obey the 125ms bus manager arbitration interval
fw_device.work:
- performs node probe, update, shutdown, revival, removal; including
kernel driver probe, update, shutdown and bus reset notification to
userspace drivers
- usually sleeps moderately long, in corner cases very long
- is scheduled by
- firewire-ohci's self-ID-complete IRQ handler tasklet via the
core's fw_node_event
- firewire-ohci's pci_remove method via core's fw_destroy_nodes/
fw_node_event
- itself during retries, e.g. while a node is powering up
iso_resource.work:
- accesses registers at the Isochronous Resource Manager node
- usually does not (but may in corner cases) sleep for long
- is scheduled via schedule_iso_resource() by
- the owning userspace driver at addition and removal of the
resource
- firewire-core's fw_device.work instances after bus reset
- itself in case of resource allocation if necessary to obey the
1000ms reallocation period after bus reset
fw_card.br_work instances should not, and instances of the others must
not, be executed in parallel by multiple CPUs -- but were not protected
against that. Hence allocate a non-reentrant workqueue for them.
fw_device.work may be used in the memory reclaim path in case of SBP-2
device updates. Hence we need a workqueue with rescuer and cannot use
system_nrt_wq.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Reviewed-by: Tejun Heo <tj@kernel.org>
|
|
When queueing iso packets, the run time is dominated by the two
MMIO accesses that set the DMA context's wake bit. Because most
drivers submit packets in batches, we can save much time by
removing all but the last wakeup.
The internal kernel API is changed to require a call to
fw_iso_context_queue_flush() after a batch of queued packets.
The user space API does not change, so one call to
FW_CDEV_IOC_QUEUE_ISO must specify multiple packets to take
advantage of this optimization.
In my measurements, this patch reduces the time needed to queue
fifty skip packets from userspace to one sixth on a 2.5 GHz CPU,
or to one third at 800 MHz.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
We do not need slab allocations anymore in order to satisfy
streaming DMA mapping constraints, thanks to commit da28947e7e36
"firewire: ohci: avoid separate DMA mapping for small AT payloads".
(Besides, the slab-allocated buffers that firewire-core, firewire-sbp2,
and firedtv used to provide for 8-byte write and lock requests were
still not fully portable since they crossed cacheline boundaries or
shared a cacheline with unrelated CPU-accessed data. snd-firewire-lib
got this aspect right by using an extra kmalloc/ kfree just for the
8-byte transaction buffer.)
This change replaces kmalloc'ed lock transaction scratch buffers in
firewire-core, firedtv, and snd-firewire-lib by local stack allocations.
Perhaps the most notable result of the change is simpler locking because
there is no need to serialize usages of preallocated per-device buffers
anymore. Also, allocations and deallocations are simpler.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Acked-by: Clemens Ladisch <clemens@ladisch.de>
|
|
On a 32-bit machine with, e.g., HZ=1000, jiffies will overflow after
about 50 days, so if there are between 25 and 50 days between bus
resets, the card->reset_jiffies comparisons can get wrong results.
To fix this, ensure that this timestamp always uses 64 bits.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
|
|
For outbound transactions, the IDR's and the callback's references now
have exactly the same lifetime, so we do not need both of them.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
|
|
We must not use fw_cancel_transaction() because it cannot correctly
abort still-active transactions. The only place in core-cdev where this
matters is when the file is released. Instead of trying to abort the
transactions, we wait for them to complete normally, i.e., until all
outbound transaction resources have been removed from the IDR tree.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
|
|
Outbound transactions are never aborted with release_client_resource(),
so it is not necessary for complete_transaction() to check whether the
resource is still registered. Only shutdown_resource() can abort such
an transaction, and this is already handled with the in_shutdown check.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: "Stefan Richter" <stefanr@s5r6.in-berlin.de>
|
|
Change the header of PHY packets to be sent to include a pseudo
transaction code. This makes the header consistent with that of
received PHY packets, and allows at_context_queue_packet() and
log_ar_at_event() to see the packet type directly instead of having
to deduce it from the header length or even from the header contents.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Conflicts:
drivers/firewire/core-card.c
drivers/firewire/core-cdev.c
and forgotten #include <linux/time.h> in drivers/firewire/ohci.c
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
This adds the DMA context programming and userspace ABI for multichannel
reception, i.e. for listening on multiple channel numbers by means of a
single DMA context.
The use case is reception of more streams than there are IR DMA units
offered by the link layer. This is already implemented by the older
ohci1394 + ieee1394 + raw1394 stack. And as discussed recently on
linux1394-devel, this feature is occasionally used in practice.
The big drawbacks of this mode are that buffer layout and interrupt
generation necessarily differ from single-channel reception: Headers
and trailers are not stripped from packets, packets are not aligned with
buffer chunks, interrupts are per buffer chunk, not per packet.
These drawbacks also cause a rather hefty code footprint to support this
rarely used OHCI-1394 feature. (367 lines added, among them 94 lines of
added userspace ABI documentation.)
This implementation enforces that a multichannel reception context may
only listen to channels to which no single-channel context on the same
link layer is presently listening to. OHCI-1394 would allow to overlay
single-channel contexts by the multi-channel context, but this would be
a departure from the present first-come-first-served policy of IR
context creation.
The implementation is heavily based on an earlier one by Jay Fenlason.
Thanks Jay.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Make a note on the seemingly unused linux/sched.h.
Rename an irritatingly named variable.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
ioctl_create_iso_context enforces ctx->header_size >= 4.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
In both the ieee1394 stack and the firewire stack, the core treats
kernelspace drivers better than userspace drivers when it comes to
CSR address range allocation: The former may request a register to be
placed automatically at a free spot anywhere inside a specified address
range. The latter may only request a register at a fixed offset.
Hence, userspace drivers which do not require a fixed offset potentially
need to implement a retry loop with incremented offset in each retry
until the kernel does not fail allocation with EBUSY. This awkward
procedure is not fundamentally necessary as the core already provides a
superior allocation API to kernelspace drivers.
Therefore change the ioctl() ABI by addition of a region_end member in
the existing struct fw_cdev_allocate. Userspace and kernelspace APIs
work the same way now.
There is a small cost to pay by clients though: If client source code
is required to compile with older kernel headers too, then any use of
the new member fw_cdev_allocate.region_end needs to be enclosed by
#ifdef/#endif directives. However, any client program that seriously
wants to use address range allocations will require a kernel of cdev ABI
version >= 4 at runtime and a linux/firewire-cdev.h header of >= 4
anyway. This is because v4 brings FW_CDEV_EVENT_REQUEST2. The only
client program in which build-time compatibility with struct
fw_cdev_allocate as found in older kernel headers makes sense is
libraw1394.
(libraw1394 uses the older broken FW_CDEV_EVENT_REQUEST to implement a
makeshift, incorrect transaction responder that does at least work
somewhat in many simple scenarios, relying on guesswork by libraw1394
and by libraw1394 based applications. Plus, address range allocation
and transaction responder is only one of many features that libraw1394
needs to provide, and these other features need to work with kernel and
kernel-headers as old as possible. Any new linux/firewire-cdev.h based
client that implements a transaction responder should never attempt to
do it like libraw1394; instead it should make a header and kernel of v4
or later a hard requirement.)
While we are at it, update the struct fw_cdev_allocate documentation to
better reflect the recent fw_cdev_event_request2 ABI addition.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
This extends the FW_CDEV_IOC_SEND_PHY_PACKET ioctl() for /dev/fw* to be
useful for ping time measurements. One application for it would be gap
count optimization in userspace that is based on ping times rather than
hop count. (The latter is implemented in firewire-core itself but is
not applicable to beta PHYs that act as repeater.)
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Add an FW_CDEV_IOC_RECEIVE_PHY_PACKETS ioctl() and
FW_CDEV_EVENT_PHY_PACKET_RECEIVED poll()/read() event for /dev/fw*.
This can be used to get information from remote PHYs by remote access
PHY packets.
This is also the 2nd half of the functionality (the receive part) to
support a userspace implementation of a VersaPHY transaction layer.
Safety considerations:
- PHY packets are generally broadcasts, hence some kind of elevated
privileges should be required of a process to be able to listen in
on PHY packets. This implementation assumes that a process that is
allowed to open the /dev/fw* of a local node does have this
privilege.
There was an inconclusive discussion about introducing POSIX
capabilities as a means to check for user privileges for these
kinds of operations.
Other limitations:
- PHY packet reception may be switched on by ioctl() but cannot be
switched off again. It would be trivial to provide an off switch,
but this is not worth the code. The client should simply close()
the fd then, or just ignore further events.
- For sake of simplicity of API and kernel-side implementation, no
filter per packet content is provided.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Add an FW_CDEV_IOC_SEND_PHY_PACKET ioctl() for /dev/fw* which can be
used to implement bus management related functionality in userspace.
This is also half of the functionality (the transmit part) that is
needed to support a userspace implementation of a VersaPHY transaction
layer.
Safety considerations:
- PHY packets are generally broadcasts and may have interesting
effects on PHYs and the bus, e.g. make asynchronous arbitration
impossible due to too low gap count. Hence some kind of elevated
privileges should be required of a process to be able to send
PHY packets. This implementation assumes that a process that is
allowed to open the /dev/fw* of a local node does have this
privilege.
There was an inconclusive discussion about introducing POSIX
capabilities as a means to check for user privileges for these
kinds of operations.
- The kernel does not check integrity of the supplied packet data.
That would be far too much code, considering the many kinds of
PHY packets. A process which got the privilege to send these
packets is trusted to do it correctly.
Just like with the other "send packet" ioctls, a non-blocking API is
chosen; i.e. the ioctl may return even before AT DMA started. After
transmission, an event for poll()/read() is enqueued. Most users are
going to need a blocking API, but a blocking userspace wrapper is easy
to implement, and the second of the two existing libraw1394 calls
raw1394_phy_packet_write() and raw1394_start_phy_packet_write() can be
better supported that way.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
to make the correspondence of ioctl numbers and handlers more obvious.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Bus resets which are triggered
- by the kernel drivers after updates of the local nodes' config ROM,
- by userspace software via ioctl
shall be deferred until after >=2 seconds after the last bus reset.
If multiple modifications of the local nodes' config ROM happen in a row,
only a single bus reset should happen after them.
When the local node's link goes from inactive to active or vice versa,
and at the two occasions of bus resets mentioned above --- and if the
current gap count differs from 63 --- the bus reset should be preceded
by a PHY configuration packet that reaffirms the gap count. Otherwise a
bus manager would have to reset the bus again right after that.
This is necessary to promote bus stability, e.g. leave grace periods for
allocations and reallocations of isochronous channels and bandwidth,
SBP-2 reconnections etc.; see IEEE 1394 clause 8.2.1.
This change implements all of the above by moving bus reset initiation
into a delayed work (except for bus resets which are triggered by the
bus manager workqueue job and are performed there immediately). It
comes with a necessary addition to the card driver methods that allows
to get the current gap count from PHY registers.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
kernel API constants
The FW_ISO_ constants of the in-kernel API of firewire-core and
FW_CDEV_ISO_ constants of the userspace API of firewire-core have
nothing to do with each other --- except that the core-cdev.c
implementation relies on them having the same values.
Hence put some compile-time assertions into core-cdev.c. It's lame but
I prefer it over including the userspace API header into the kernelspace
API header and defining kernelspace API constants from userspace API
constants. Nor do I want to expose the kernelspace constants in one of
the two firewire headers that are exported to userland since this only
concerns the core-cdev.c implementation.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Check that the data length of a write quadlet request actually is large
enough for a quadlet. Otherwise, fw_fill_request could access the four
bytes after the end of the outbound_transaction_event structure.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Modification of Clemens' change: Consolidate the check into
init_request() which is used by the affected ioctl_send_request() and
ioctl_send_broadcast_request() and the unaffected
ioctl_send_stream_packet(), to save a few lines of code.
Note, since struct outbound_transaction_event *e is slab-allocated, such
an out-of-bounds access won't hit unallocated memory but may result in a
(virtually impossible to exploit) information disclosure.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Fix an obscure ABI feature that is a bit of a hassle to implement.
However, somebody put it into the ABI, so let's fill in a sensible
value there.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
fw_cdev_event_request2
The problem:
A target-like userspace driver, e.g. AV/C target or SBP-2/3 target,
needs to be able to act as responder and requester. In the latter role,
it needs to send requests to nods from which it received requests. This
is currently impossible because fw_cdev_event_request lacks information
about sender node ID.
Reported-by: Jay Fenlason <fenlason@redhat.com>
Libffado + libraw1394 + firewire-core is currently unable to drive two
or more audio devices on the same bus.
Reported-by: Arnold Krille <arnold@arnoldarts.de>
This is because libffado requires destination node ID of FCP requests
and sender node ID of FCP responses to match. It even prohibits
libffado from working with a bus on which libraw1394 opens a /dev/fw* as
default ioctl device that does not correspond with the audio device.
This is because libraw1394 does not receive the sender node ID from the
kernel.
Moreover, fw_cdev_event_request makes it impossible to tell unicast and
broadcast write requests apart.
The fix:
Add a replacement of struct fw_cdev_event_request request, boringly
called struct fw_cdev_event_request2. The new event will be sent to a
userspace client instead of the old one if the client claims
compatibility with <linux/firewire-cdev.h> ABI version 4 or later.
libraw1394 needs to be extended to make use of the new event, in order
to properly support libffado and other FCP or address range mapping
users who require correct sender node IDs.
Further notes:
While we are at it, change back the range of possible values of
fw_cdev_event_request.tcode to 0x0...0xb like in ABI version <= 3.
The preceding change "firewire: expose extended tcode of incoming lock
requests to (userspace) drivers" expanded it to 0x0...0x17 which could
catch sloppily coded clients by surprise. The extended range of codes
is only used in the new fw_cdev_event_request2.tcode.
Jay and I also suggested an alternative approach to fix the ABI for
incoming requests: Add an FW_CDEV_IOC_GET_REQUEST_INFO ioctl which can
be called after reception of an fw_cdev_event_request, before issuing of
the closing FW_CDEV_IOC_SEND_RESPONSE ioctl. The new ioctl would reveal
the vital information about a request that fw_cdev_event_request lacks.
Jay showed an implementation of this approach.
The former event approach adds 27 LOC of rather trivial code to
core-cdev.c, the ioctl approach 34 LOC, some of which is nontrivial.
The ioctl approach would certainly also add more LOC to userspace
programs which require the expanded information on inbound requests.
This approach is probably only on the lighter-weight side in case of
clients that want to be compatible with kernels that lack the new
capability, like libraw1394. However, the code to be added to such
libraw1394-like clients in case of the event approach is a straight-
forward additional switch () case in its event handler.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
libraw1394 v2.0.0...v2.0.5 takes FW_CDEV_VERSION from an externally
installed header file and uses it to declare its own implementation
level in FW_CDEV_IOC_GET_INFO. This is wrong; it should set the real
version for which it was actually written.
If we add features to the kernel ABI that require the kernel to check
a client's implementation level, we can not trust the client version if
it was set from FW_CDEV_VERSION.
Hence freeze FW_CDEV_VERSION at the current value (no damage has been
done yet), clearly document FW_CDEV_VERSION as a dummy version and what
clients are expected to do with fw_cdev_get_info.version, and use a new
defined constant (which is not placed into the exported header file) as
kernel implementation level.
Note, in order to check in client program source code which features are
present in an externally installed linux/firewire-cdev.h, use
preprocessor directives like
#ifdef FW_CDEV_IOC_ALLOCATE_ISO_RESOURCE
or
#ifdef FW_CDEV_EVENT_ISO_RESOURCE_ALLOCATED
instead of a check of FW_CDEV_VERSION.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
If a request comes in to an address range managed by a userspace driver
i.e. <linux/firewire-cdev.h> client, the card instance of request and
response may differ from the card instance of the client device.
Therefore we need to take a reference of the card until the response was
sent.
I thought about putting the reference counting into core-transaction.c,
but the various high-level drivers besides cdev clients (firewire-net,
firewire-sbp2, firedtv) use the card pointer in their fw_address_handler
address_callback method only to look up devices of which they already
hold the necessary references. So this seems to be a specific
firewire-cdev issue which is better addressed locally.
We do not need the reference
- in case of FCP_REQUEST or FCP_RESPONSE requests because then the
firewire-core will send the split transaction response for us
already in the context of the request handler,
- if it is the same card as the client device's because we hold a
card reference indirectly via teh client->device reference.
To keep things simple, we take the reference nevertheless.
Jay Fenlason wrote:
> there's no way for the core to tell cdev "this card is gone,
> kill any inbound transactions on it", while cdev holds the transaction
> open until userspace issues a SEND_RESPONSE ioctl, which may be a very,
> very long time. But when it does, it calls fw_send_response(), which
> will dereference the card...
>
> So how unhappy are we about userspace potentially holding a fw_card
> open forever?
While termination of inbound transcations at card removal could be
implemented, it is IMO not worth the effort. Currently, the effect of
holding a reference of a card that has been removed is to block the
process that called the pci_remove of the card. This is
- either a user process ran by root. Root can find and kill processes
that have /dev/fw* open, if desired.
- a kernel thread (which one?) in case of hot removal of a PCCard or
ExpressCard.
The latter case could be a problem indeed. firewire-core's card
shutdown and card release should probably be improved not to block in
shutdown, just to defer freeing of memory until release.
This is not a new problem though; the same already always happens with
the client->device->card without the need of inbound transactions or
other special conditions involved, other than the client not closing the
file.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
My box has two firewire cards in it: card0 and card1.
My application opens /dev/fw0 (card 0) and allocates an address space.
The core makes the address space available on both cards.
Along comes the remote device, which sends a READ_QUADLET_REQUEST to
card1. The request gets passed up to my application, which calls
ioctl_send_response().
ioctl_send_response() then calls fw_send_response() with card0,
because that's the card it's bound to.
Card0's driver drops the response, because it isn't part of
a transaction that it has outstanding.
So in core-cdev: handle_request(), we need to stash the
card of the inbound request in the struct inbound_transaction_resource and
use that card to send the response to.
The hard part will be refcounting the card correctly
so it can't get deallocated while we hold a pointer to it.
Here's a trivial patch, which does not do the card refcounting, but at
least demonstrates what the problem is.
Note that we can't depend on the fact that the core-cdev:client
structure holds a card open, because in this case the card it holds
open is not the card the request came in on.
..and there's no way for the core to tell cdev "this card is gone,
kill any inbound transactions on it", while cdev holds the transaction
open until userspace issues a SEND_RESPONSE ioctl, which may be a very,
very long time. But when it does, it calls fw_send_response(), which
will dereference the card...
So how unhappy are we about userspace potentially holding a fw_card
open forever?
Signed-off-by: Jay Fenlason <fenlason@redhat.com>
Reference counting to be addressed in a separate change.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (whitespace)
|
|
Protect the client's iso context pointer against a race that can happen
when more than one creation call is executed at the same time.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
void (*fw_address_callback_t)(..., int speed, ...) is the speed that a
remote node chose to transmit a request to us. In case of split
transactions, firewire-core will transmit the response at that speed.
Upper layer drivers on the other hand (firewire-net, -sbp2, firedtv, and
userspace drivers) cannot do anything useful with that speed datum,
except log it for debug purposes. But data that is merely potentially
(not even actually) used for debug purposes does not belong into the API.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
All of the fields of the iso_interrupt_event instance are overwritten
right after it was allocated.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Rather than "read a Control and Status Registers (CSR) Architecture
register" I prefer to say "read a Control and Status Register".
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
To prepare for the following additions of more OHCI-implemented CSR
registers, replace the get_cycle_time driver callback with a generic
CSR register callback.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
|
|
Add a check that the data length in the SEND_RESPONSE ioctl is correct.
Incidentally, this also fixes the previously wrong response length of
software-handled lock requests.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6
* 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6:
ieee1394: schedule for removal
firewire: core: use separate timeout for each transaction
firewire: core: Fix tlabel exhaustion problem
firewire: core: make transaction label allocation more robust
firewire: core: clean up config ROM related defined constants
ieee1394: mark char device files as not seekable
firewire: cdev: mark char device files as not seekable
firewire: ohci: cleanups and fix for nonstandard build without debug facility
firewire: ohci: wait for PHY register accesses to complete
firewire: ohci: fix up configuration of TI chips
firewire: ohci: enable 1394a enhancements
firewire: ohci: do not clear PHY interrupt status inadvertently
firewire: ohci: add a function for reading PHY registers
Trivial conflicts in Documentation/feature-removal-schedule.txt
|
|
git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6
* 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6:
firewire: cdev: change license of exported header files to MIT license
firewire: cdev: comment fixlet
firewire: cdev: iso packet documentation
firewire: cdev: fix information leak
firewire: cdev: require quadlet-aligned headers for transmit packets
firewire: cdev: disallow receive packets without header
|
|
The <linux/firewire-cdev.h> character device file ABI (i.e. /dev/fw*
character device file interface) does not make any use of lseek(),
pread(), pwrite() (or any kind of write() at all).
Use nonseekable_open() and, redundantly, set file_operations.llseek to
no_llseek to remove any doubt whether the BKL-grabbing default_llseek
handler is used. (Also shuffle file_operations initialization according
to the order of handler definitions.)
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
A userspace client got to see uninitialized stack-allocated memory if it
specified an _IOC_READ type of ioctl and an argument size larger than
expected by firewire-core's ioctl handlers (but not larger than the
core's union ioctl_arg).
Fix this by clearing the requested buffer size to zero, but only at _IOR
ioctls. This way, there is almost no runtime penalty to legitimate
ioctls. The only legitimate _IOR is FW_CDEV_IOC_GET_CYCLE_TIMER with 12
or 16 bytes to memset.
[Another way to fix this would be strict checking of argument size (and
possibly direction) vs. command number. However, we then need a lookup
table, and we need to allow for slight size deviations in case of 32bit
userland on 64bit kernel.]
Reported-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
The definition of struct fw_cdev_iso_packet seems to imply that the
header_length must be quadlet-aligned, and in fact, specifying an
unaligned header has never really worked when using multiple packet
structures, because the position of the next control word is computed by
rounding the header_length _down_, so the last one to three bytes of the
header would overlap the next control word.
To avoid this problem, check that the header length is properly aligned.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
In receive contexts, reject packets with header_length==0. This would
be an instruction to queue zero packets which would not make sense.
This prevents a division by zero in the OHCI driver.
Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
implicit slab.h inclusion from percpu.h
percpu.h is included by sched.h and module.h and thus ends up being
included when building most .c files. percpu.h includes slab.h which
in turn includes gfp.h making everything defined by the two files
universally available and complicating inclusion dependencies.
percpu.h -> slab.h dependency is about to be removed. Prepare for
this change by updating users of gfp and slab facilities include those
headers directly instead of assuming availability. As this conversion
needs to touch large number of source files, the following script is
used as the basis of conversion.
http://userweb.kernel.org/~tj/misc/slabh-sweep.py
The script does the followings.
* Scan files for gfp and slab usages and update includes such that
only the necessary includes are there. ie. if only gfp is used,
gfp.h, if slab is used, slab.h.
* When the script inserts a new include, it looks at the include
blocks and try to put the new include such that its order conforms
to its surrounding. It's put in the include block which contains
core kernel includes, in the same order that the rest are ordered -
alphabetical, Christmas tree, rev-Xmas-tree or at the end if there
doesn't seem to be any matching order.
* If the script can't find a place to put a new include (mostly
because the file doesn't have fitting include block), it prints out
an error message indicating which .h file needs to be added to the
file.
The conversion was done in the following steps.
1. The initial automatic conversion of all .c files updated slightly
over 4000 files, deleting around 700 includes and adding ~480 gfp.h
and ~3000 slab.h inclusions. The script emitted errors for ~400
files.
2. Each error was manually checked. Some didn't need the inclusion,
some needed manual addition while adding it to implementation .h or
embedding .c file was more appropriate for others. This step added
inclusions to around 150 files.
3. The script was run again and the output was compared to the edits
from #2 to make sure no file was left behind.
4. Several build tests were done and a couple of problems were fixed.
e.g. lib/decompress_*.c used malloc/free() wrappers around slab
APIs requiring slab.h to be added manually.
5. The script was run on all .h files but without automatically
editing them as sprinkling gfp.h and slab.h inclusions around .h
files could easily lead to inclusion dependency hell. Most gfp.h
inclusion directives were ignored as stuff from gfp.h was usually
wildly available and often used in preprocessor macros. Each
slab.h inclusion directive was examined and added manually as
necessary.
6. percpu.h was updated not to include slab.h.
7. Build test were done on the following configurations and failures
were fixed. CONFIG_GCOV_KERNEL was turned off for all tests (as my
distributed build env didn't work with gcov compiles) and a few
more options had to be turned off depending on archs to make things
build (like ipr on powerpc/64 which failed due to missing writeq).
* x86 and x86_64 UP and SMP allmodconfig and a custom test config.
* powerpc and powerpc64 SMP allmodconfig
* sparc and sparc64 SMP allmodconfig
* ia64 SMP allmodconfig
* s390 SMP allmodconfig
* alpha SMP allmodconfig
* um on x86_64 SMP allmodconfig
8. percpu.h modifications were reverted so that it could be applied as
a separate patch and serve as bisection point.
Given the fact that I had only a couple of failures from tests on step
6, I'm fairly confident about the coverage of this conversion patch.
If there is a breakage, it's likely to be something in one of the arch
headers which should be easily discoverable easily on most builds of
the specific arch.
Signed-off-by: Tejun Heo <tj@kernel.org>
Guess-its-ok-by: Christoph Lameter <cl@linux-foundation.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Lee Schermerhorn <Lee.Schermerhorn@hp.com>
|
|
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
from array of char to union of structs. I already used a union to size
the buffer which holds ioctl arguments; more consequent is to define it
as an instance of this union in the first place.
Also rename several local variables from "request" to "a"(rgument) since
the term request can be mistaken to mean a transaction subaction, e.g.
an instance of struct fw_request.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
The system time from CLOCK_REALTIME is not monotonic, hence problematic
for the main user of the FW_CDEV_IOC_GET_CYCLE_TIMER ioctl. This issue
exists in its successor ABI, i.e. raw1394, too.
http://subversion.ffado.org/ticket/242
We now offer an alternative ioctl which lets the caller choose between
CLOCK_REALTIME, CLOCK_MONOTONIC, and CLOCK_MONOTONIC_RAW as source of
the local time, very similar to the clock_gettime libc function. The
format of the local time return value matches that of clock_gettime
(seconds and nanoseconds, instead of a single microseconds value from
the existing ioctl).
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
The current implementation of Bus_Time read access was buggy since it
did not ensure that Bus_Time.second_count_hi and second_count_lo came
from the same 128 seconds period.
Reported-by: Håkan Johansson <f96hajo@chalmers.se>
Instead of a fix, remove Bus_Time register support altogether. The spec
requires all cycle master capable nodes to implement this (all Linux
nodes are cycle master capable) while it also says that it "may" be
initialized by the bus manager or by the IRM standing in for a bus
manager. (Neither Linux' firewire-core nor ieee1394 nodemgr implement
this.)
Since we cannot rely on Bus_Time having been initialized by a bus
manager, it is better to return an error instead of a nonsensical value
on a read request to Bus_Time.
Alternatively, we could fix the Bus_Time read integrity bug _and_
implement (a) cycle master's write support of the register as well as
(b) bus manager's Bus_Time initialization service, i.e. preservation of
the Bus_Time when the cycle master node of a bus changes. However, that
would be quite some code for a feature that is unreliable to begin with
and very likely unused in practice.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
ohci: Break out of the retry loop if too many attempts were necessary.
This may theoretically happen if the chip is fatally defective or if the
get_cycle_timer ioctl was performed after a CardBus controller was
ejected.
Also micro-optimize the loop by re-using the last two register reads in
the next iteration, remove a questionable inline keyword, and shuffle a
comment around.
core: ioctl_get_cycle_timer() is always called with interrupts on,
therefore local_irq_save() can be replaced by local_irq_disable().
Disabled local IRQs imply disabled preemption, hence preempt_disable()
can be removed.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
Commit db5d247a "firewire: fix use of multiple AV/C devices, allow
multiple FCP listeners" introduced a regression into 2.6.33-rc3:
The core freed payloads of incoming requests to FCP_Request or
FCP_Response before a userspace driver accessed them.
We need to copy such payloads for each registered userspace client
and free the copies according to the lifetime rules of non-FCP client
request resources.
(This could possibly be optimized by reference counts instead of
copies.)
The presently only kernelspace driver which listens for FCP requests,
firedtv, was not affected because it already copies FCP frames into an
own buffer before returning to firewire-core's FCP handler dispatcher.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
|
If copy_from_user in an FW_CDEV_IOC_SEND_RESPONSE ioctl failed, the
fw_request pointed to by the inbound_transaction_resource is no
longer referenced and needs to be freed.
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
|
