23 Jul, 2010
7 commits
-
region->end is defined as an upper bound of the requested address range,
exclusive --- i.e. as an address outside of the range in which the
requested CSR is to be placed.Hence 0x0001,0000,0000,0000 is the biggest valid region->end, not
0x0000,ffff,ffff,fffc like the current check asserted.For simplicity, the fix drops the region->end & 3 test because there is
no actual problem with these bits set in region->end. The allocated
address range will be quadlet aligned and of a size of multiple quadlets
due to the checks for region->start & 3 and handler->length & 3 alone.Signed-off-by: Stefan Richter
-
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
-
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
-
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
-
to make the correspondence of ioctl numbers and handlers more obvious.
Signed-off-by: Stefan Richter
-
Response events:
- are generated on more occasions than their documentation claimed.CSR allocation:
- An already occupied CSR can be determined from errno==EBUSY.Bus resets:
- Note that FW_CDEV_IOC_INITIATE_BUS_RESET is nonblocking and that the
client is not required to observe a grace period since kernels
2.6.36+ will enforce it now (commit 02d37bed).- The possible values of fw_cdev_initiate_bus_reset.type are listed in
the kerneldoc comment already.- Clarify that an application that uses FW_CDEV_IOC_ADD_DESCRIPTOR and
FW_CDEV_IOC_REMOVE_DESCRIPTOR does not have to issue a bus reset.Isochronous I/O contexts:
- At most one can be created per open file descriptor.Signed-off-by: Stefan Richter
-
core-transaction.c transmit_complete_callback() and close_transaction()
expect packet callback status to be an ACK or RCODE, and ACKs get
translated to RCODEs for transaction callbacks.An old comment on the packet callback API (been there from the initial
submission of the stack) and the dummy_driver implementation of
send_request/send_response deviated from this as they also included
-ERRNO in the range of status values.Let's narrow status values down to ACK and RCODE to prevent surprises.
RCODE_CANCELLED is chosen as the dummy_driver's RCODE as its meaning of
"transaction timed out" comes closest to what happens when a transaction
coincides with card removal.Signed-off-by: Stefan Richter
13 Jul, 2010
6 commits
-
Signed-off-by: Joe Perches
Signed-off-by: Stefan Richter -
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
-
When a descriptor was added or removed to the local node's config ROM,
userspace clients which had a local node's /dev/fw* open did not receive
any fw_cdev_event_bus_reset for poll()/read() consumption.The cause was that the core-device.c facility which re-reads the config
ROM of the bus reset initiator node missed to call the fw_device update
function. The fw_units are destroyed and newly added, but their parent
stays and needs to be updated.Reported-by: Jay Fenlason
Signed-off-by: Stefan Richter -
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
8 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