21 Jun, 2010
4 commits
-
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
20 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 -
This patch adds support for message-signaled interrupts.
Any native PCI-Express OHCI controller should support MSI, but most are
just PCI cores behind a PCI-E/PCI bridge. The only chips that are known
to claim to support MSI are the Lucent/Agere/LSI FW643 and the VIA
VT6315, none of which I have been able to test.Due to the high level of trust I have in the competence of these and any
future chip makers, I thought it a good idea to add a disable-MSI quirk.Signed-off-by: Clemens Ladisch
Tested Agere FW643 rev 07 [11c1:5901] and JMicron JMB381 [197b:2380].
Added a quirks list entry for JMB38X since it kept its count of MSI
events consistently at zero.Signed-off-by: Stefan Richter
-
On 26 Apr 2010, Clemens Ladisch wrote:
> In theory, none of the interrupts should occur before the link is
> enabled. In practice, I'd rather make sure to not set the master
> interrupt enable bit until we have installed the interrupt handler.and proposed to move OHCI1394_masterIntEnable out of the present
reg_write() into a new one before the HCControl.linkEnable reg_write().Why not defer setting /all/ of the bits until right before linkEnable?
Reviewed-by: Clemens Ladisch
Signed-off-by: Stefan Richter
01 Jun, 2010
1 commit
-
...when user data is immediately copied into the allocated region.
Signed-off-by: Julia Lawall
Signed-off-by: Stefan Richter (changelog)
26 May, 2010
1 commit
-
All application domains that are supported by the old ieee1394 driver
stack are supported by the newer firewire driver stack too. There is
now good and extensive experience with the newer stack from deployment
in Fedora since F7 as well as by enthusiast users of other
distributions.The new drivers have consequently been recommended as the default ones
since 2.6.33, in order to fix some severe usability problems of FireWire
on Linux due to limitations of the old stack. It is now high time to
announce when the obsolete drivers will be removed.Signed-off-by: Stefan Richter
Acked-by: Jarod Wilson
19 May, 2010
2 commits
-
Using a single timeout for all transaction that need to be flushed does
not work if the submission of new transactions can defer the timeout
indefinitely into the future. We need to have timeouts that do not
change due to other transactions; the simplest way to do this is with a
separate timer for each transaction.Signed-off-by: Clemens Ladisch
Signed-off-by: Stefan Richter (+ one lockdep annotation) -
fw_core_handle_response() was not properly clearing tlabel_mask. This
was resulting in premature tlabel exhaustion.Signed-off-by: Peter Hurley
This fixes an omission in 2.6.31-rc1 commit 1e626fdc "firewire: core:
use more outbound tlabels" which prevented to really use 64 instead of
32 transaction labels, as soon as split transactions occurred that had
their AR-resp tasklet run after the AT-req tasklet.Signed-off-by: Stefan Richter
20 Apr, 2010
2 commits
-
If one request is so long-lived that it does not get a response before
the following 63 requests, its bit in tlabel_mask is still set when the
next request tries to allocate a transaction label for that number. In
this state, while the first request is not completed or timed out, no
new requests can be submitted.To fix this, skip over any label still in use, and do not error out
unless we have entirely run out of labels.Signed-off-by: Clemens Ladisch
Signed-off-by: Stefan Richter -
Clemens Ladisch pointed out that
- BIB_IMC is not named like the field is called in the standard,
- readers of the code may get worried about the magic 0x0c0083c0,
- a CSR_NODE_CAPABILITIES key is there in the header but not put to
good use.So let's rename BIB_IMC, add a defined constant for Node_Capabilities
and a comment which reassures people that somebody thought about it and
they don't have to (or if they still do, tell them where they have to
look for confirmation), and prune our incomplete and arbitrary set of
defined constants of CSR key IDs. And there is a nother magic number,
that of Bus_Information_Block.Bus_Name, to be defined and commented.Signed-off-by: Stefan Richter
10 Apr, 2010
1 commit
-
The
- raw1394 (/dev/raw1394),
- video1394 (/dev/video1394/*),
- dv1394 (/dev/dv1394/*)
character device file ABIs do not make any use of lseek(), pread(), or
pwrite(). Therefore 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.Although all this is legacy code which should be left in peace until it
is eventually removed (as it is superseded by firewire-core's
ABI), this change seems still worth doing to
further minimize the presence of BKL usage in the kernel.Signed-off-by: Stefan Richter