13 Jul, 2010
3 commits
-
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
-
The present inline documentation of the fw_send_request() in-kernel API
refers to userland code that is not applicable to kernel drivers at all.Reported-by: Ben Gamari
While we are at fixing the whole documentation of fw_send_request(),
also improve the rest of firewire-core's kerneldoc comments:
- Add a bit of text concerning fw_run_transaction()'s call parameters.
- Append () to function names and tab-align parameter descriptions as
suggested by the example in Documentation/kernel-doc-nano-HOWTO.txt.
- Remove kerneldoc markers from comments on static functions.
- Remove outdated parameter descriptions at build_tree().Signed-off-by: Stefan Richter
-
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
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
08 Jul, 2010
2 commits
-
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
-
This is a workqueue job and always entered with IRQs enabled.
Signed-off-by: Stefan Richter
21 Jun, 2010
8 commits
-
Add information regarding the 2.6.32 update to the xmit variant of
fw_cdev_event_iso_interrupt.Signed-off-by: Stefan Richter
-
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 FenlasonLibffado + libraw1394 + firewire-core is currently unable to drive two
or more audio devices on the same bus.
Reported-by: Arnold KrilleThis 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 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 -
When a remote device does a LOCK_REQUEST, the core does not pass
the extended tcode to userspace. This patch makes it use the
juju-specific tcodes listed in firewire-constants.h for incoming
requests.Signed-off-by: Jay Fenlason
This matches how tcode in the API for outbound requests is treated.
Affects kernelspace and userspace drivers alike, but at the moment there
are no kernespace drivers that receive lock requests.Split out from a combo patch, slightly reordered, changelog reworded.
Signed-off-by: Stefan Richter
-
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
-
If a request comes in to an address range managed by a userspace driver
i.e. 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
-
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
Reference counting to be addressed in a separate change.
Signed-off-by: Stefan Richter (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
Signed-off-by: Stefan Richter -
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
20 Jun, 2010
1 commit
-
All of the fields of the iso_interrupt_event instance are overwritten
right after it was allocated.Signed-off-by: Stefan Richter
19 Jun, 2010
8 commits
-
which caused gcc 4.6 to warn about
variable 'destination' set but not used.Since the hardware ensures that we receive only response packets with
proper destination node ID (in a given bus generation), we have no use
for destination here in the core as well as in upper layers.(This is different with request packets. There we pass destination node
ID to upper layers because they may for example need to check whether
this was an unicast or broadcast request.)Reported-and-Tested-By: Justin P. Mattock
Signed-off-by: Stefan Richter -
which caused gcc 4.6 to warn about
variable 'XYZ' set but not used.sbp2.c, unit_characteristics:
The underlying problem which was spotted here --- an incomplete
implementation --- is already 50% fixed in drivers/firewire/sbp2.c which
observes mgt_ORB_timeout but not yet ORB_size.raw1394.c, length_conflict; dv1394.c, ts_off:
Impossible to tell why these variables are there. We can safely remove
them though because we don't need a compiler warning to realize that we
are dealing with (at least stylistically) flawed code here.dv1394.c, packet_time:
This was used in debug macro that is only compiled in with
DV1394_DEBUG_LEVEL >= 2 defined at compile-time. Just drop it since
nobody debugs dv1394 anymore. Avoids noise in regular kernel builds.dv1394.c, ohci; eth1394.c, priv:
These variables clearly can go away. Somebody wanted to use them but
then didn't (or not anymore).Note, all of this code is considered to be at its end of life and is
thus not really meant to receive janitorial updates anymore. But if we
can easily remove noisy warnings from kernel builds, we should.Reported-by: Justin P. Mattock
Signed-off-by: Stefan Richter -
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
-
All of these CSRs have the same read/ write/ aynthing-else handling,
except for CSR_PRIORITY_BUDGET which might not be implemented.The CSR_CYCLE_TIME read handler implementation accepted 4-byte-sized
block write requests before this change but this is just silly; the
register is only required to support quadlet read and write requests
like the other r/w CSR core and Serial-Bus-dependent registers.Signed-off-by: Stefan Richter
-
Push the maintenance of STATE_CLEAR/SET.abdicate down into the card
driver. This way, the read/write_csr_reg driver method works uniformly
across all CSR offsets.Signed-off-by: Stefan Richter
-
by feature variables in the fw_card struct. The hook appeared to be an
unnecessary abstraction in the card driver interface.Cleaner would be to pass those feature flags as arguments to
fw_card_initialize() or fw_card_add(), but the FairnessControl register
is in the SCLK domain and may therefore not be accessible while Link
Power Status is off, i.e. before the card->driver->enable call from
fw_card_add().Signed-off-by: Stefan Richter
-
In case of fw_card_bm_work()'s lock request, the present sizeof
expression is going to be wrong if somebody changes the fw_card's DMA
scratch buffer's size in the future.In case of quadlet write requests, sizeof(u32) is just silly; it's 4.
In case of SBP-2 ORB pointer write requests, 8 is arguably quicker to
understand as the correct and only possible value than
sizeof(some_datum).Signed-off-by: Stefan Richter
-
Add a comment on which of the conflicting NODE_IDS specifications we
implement. Reduce a comment on rather irrelevant register bits that can
all be looked up in the spec (or from now on in the code history).
Directly include the required indirectly included bug.h.Signed-off-by: Stefan Richter
10 Jun, 2010
18 commits
-
As part of the bus manager responsibilities, make sure that the cycle
master sends cycle start packets. This is needed when the old bus
manager disabled the cycle master's cmstr bit and there are iso-capable
nodes on the new bus.Signed-off-by: Clemens Ladisch
-
On OHCI 1.1 controllers, let the hardware allocate the broadcast channel
automatically. This removes a theoretical race condition directly after
a bus reset where it could be possible to read the channel allocation
register with channel 31 still being unallocated.Signed-off-by: Clemens Ladisch
-
Implement the abdicate bit, which is required for bus manager
capable nodes and tested by the Base 1394 Test Suite.Finally, something to do at a command reset! :-)
Signed-off-by: Clemens Ladisch
-
Implement the cmstr bit, which is required for cycle master capable
nodes and tested for by the Base 1394 Test Suite.This bit allows the bus master to disable cycle start packets; there are
bus master implementations that actually do this.Signed-off-by: Clemens Ladisch
-
Implement the MAIN_UTILITY register, which is utterly optional
but useful as a safe target for diagnostic read/write/broadcast
transactions.Signed-off-by: Clemens Ladisch
-
If supported by the OHCI controller, implement the PRIORITY_BUDGET
register, which is required for nodes that can use asynchronous
priority arbitration.To allow the core to determine what features the lowlevel device
supports, add a new card driver callback.Signed-off-by: Clemens Ladisch
-
Implement the BUSY_TIMEOUT register, which is required for nodes that
support retries.Signed-off-by: Clemens Ladisch
-
Implement the BUS_TIME register, which is required for cycle master
capable nodes and tested for by the Base 1393 Test Suite. Even when
there is not yet bus master initialization support, this register allows
us to work together with other bus masters.Signed-off-by: Clemens Ladisch
-
The specification requires that CYCLE_TIME is writable so that it can be
initialized, so we better implement it.Signed-off-by: Clemens Ladisch
-
Implement the SPLIT_TIMEOUT registers. Besides being required by the
spec, this is desirable for some IIDC devices and necessary for many
audio devices to be able to increase the timeout from userspace.Signed-off-by: Clemens Ladisch
-
This implements the RESET_START register (as a dummy) to make the Base
1394 Test Suite happy.Signed-off-by: Clemens Ladisch
-
The NODE_IDS register, and especially its bus_id field, is quite
useless because 1394.1 requires that the bus_id field always stays
0x3ff. However, the 1394 specification requires this register on all
transaction capable nodes, and the Base 1394 Test Suite tests for it,
so we better implement it.Signed-off-by: Clemens Ladisch
-
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
-
The state registers are zero and read-only in this implementation, so
they are not of much use. However, the specification requires that they
are present for transaction capable nodes, and the Base 1394 Test Suite
tests for them, so we better implement them.Signed-off-by: Clemens Ladisch
-
When the candidate bus manager fails to do the lock request with which
it tries to become bus manager, it assumes that the current IRM is not
actually IRM capable and forces itself to become root. However, if that
lock request failed because the local node itself was not able to send
it, then we cannot blame the current IRM and should not steal its
rootness.In this case, RCODE_SEND_ERROR is likely to indicate a temporary error
condition such as exhausted tlabels or low memory, so we better try
again later.Signed-off-by: Clemens Ladisch
-
Most PHY chips, when idle, can complete a register access in the time
needed for two or three PCI read transactions; bigger delays occur only
when data is currently being moved over the link/PHY interface. So if
we busy-wait a few times when waiting for the register access to finish,
it is likely that we can finish without having to sleep.Signed-off-by: Clemens Ladisch
-
WARN's format string argument should not carry a printk level prefix.
Signed-off-by: Stefan Richter
-
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
Signed-off-by: Stefan Richter