Skip to content
Snippets Groups Projects
  1. Jul 26, 2011
  2. Jul 15, 2011
    • Stefan Richter's avatar
      firewire: cdev: prevent race between first get_info ioctl and bus reset event queuing · 93b37905
      Stefan Richter authored
      
      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: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      Cc: <stable@kernel.org>
      93b37905
    • Stefan Richter's avatar
      firewire: cdev: return -ENOTTY for unimplemented ioctls, not -EINVAL · d873d794
      Stefan Richter authored
      
      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: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      Cc: <stable@kernel.org>
      d873d794
  3. Jul 13, 2011
  4. Jul 10, 2011
  5. Jul 09, 2011
    • Stefan Richter's avatar
      firewire: ohci: skip soft reset retries after card ejection · 9f426173
      Stefan Richter authored
      
      The software reset in firewire-ohci's pci_remove does not have a great
      prospect of success if the card was already physically removed at this
      point.  So let's skip the 500 ms that were spent in retries here.
      
      Also, replace a defined constant by its open-coded value.  This is not a
      constant from a specification but an arbitrarily chosen retry limit.  It
      was only used in this single place.
      
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      9f426173
    • Stefan Richter's avatar
      firewire: ohci: fix PHY reg access after card ejection · 215fa444
      Stefan Richter authored
      
      Detect and handle ejection of FireWire CardBus cards in PHY register
      accesses:
      
        - The last attempt of firewire-core to reset the bus during shutdown
          caused a spurious "firewire_ohci: failed to write phy reg" error
          message in the log.  Skip this message as well as the prior retry
          loop that needlessly took 100 milliseconds.
      
        - In the unlikely case that a PHY register was read right after card
          ejection, a bogus value was obtained and possibly acted upon.
          Instead, fail the read attempt.
      
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      215fa444
    • Stefan Richter's avatar
    • Stefan Richter's avatar
      firewire: ohci: reduce potential context_stop latency · 9ef28ccd
      Stefan Richter authored
      
      Stopping an isochronous reception DMA context takes two loop iterations
      in context_stop on several controllers (JMicron, NEC, VIA).  But there
      is no extra delay necessary between these two reg_read trials; the MMIO
      reads themselves are slow enough.  Hence bring back the behavior from
      before commit dd6254e5 "firewire: ohci:
      remove superfluous posted write flushes" on these controllers by means
      of an "if (i)" condition.
      
      Isochronous context stop is performed in preemptible contexts (and only
      rarely), hence this change is of little impact.  (Besides, Agere and TI
      controllers always, or almost always, have the context stopped already
      at the first ContextControl read.)
      
      More important is asynchronous transmit context stop, which is performed
      while local interrupts are disabled (on the two AT DMAs in
      bus_reset_tasklet, i.e. after a self-ID-complete event).  In my
      experience with several controllers, tested with a usermode AT-request
      transmitter as well as with FTP transmission over firewire-net, the AT
      contexts were luckily already stopped at the first ContextControl read,
      i.e. never required another MMIO read let alone mdelay.  A possible
      explanation for this is that the controllers which I tested perhaps stop
      AT DMA before they perform the self-ID reception DMA.
      
      But we cannot be sure about that and should keep the interrupts-disabled
      busy loop as short as possible.  Hence, query the ContextControl
      register in 1000 udelay(10) intervals instead of 10 udelay(1000)
      intervals.  I understand from an estimation by Clemens Ladisch that
      stopping a busy DMA context should take microseconds or at worst tens of
      microseconds, not milliseconds.
      
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      9ef28ccd
  6. Jun 02, 2011
  7. May 10, 2011
    • Stefan Richter's avatar
      firewire: sbp2: parallelize login, reconnect, logout · 105e53f8
      Stefan Richter authored
      
      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: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      105e53f8
    • Stefan Richter's avatar
      firewire: sbp2: octlet AT payloads can be stack-allocated · 81bf52d8
      Stefan Richter authored
      
      We do not need slab allocations for ORB pointer write transactions
      anymore in order to satisfy streaming DMA mapping constraints, thanks to
      commit da28947e "firewire: ohci: avoid separate DMA mapping for
      small AT payloads".
      
      (Besides, the slab-allocated buffers that firewire-sbp2 used to provide
      for 8-byte write requests were still not fully portable since they
      shared a cacheline with unrelated CPU-accessed data.)
      
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      81bf52d8
    • Stefan Richter's avatar
      firewire: sbp2: omit Scsi_Host lock from queuecommand · b75ca5ea
      Stefan Richter authored
      
      firewire-sbp2 already takes care for internal serialization where
      required (ORB list accesses), and it does not use cmd->serial_number
      internally.  Hence it is safe to not grab the shost lock around
      queuecommand.
      
      While we are at housekeeping, drop a redundant struct member:
      sbp2_command_orb.done is set once in a hot path and dereferenced once in
      a hot path.  We can as well dereference sbp2_command_orb.cmd->scsi_done
      instead.
      
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      b75ca5ea
    • Stefan Richter's avatar
      firewire: core: use non-reentrant workqueue with rescuer · 6ea9e7bb
      Stefan Richter authored
      
      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: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      Reviewed-by: default avatarTejun Heo <tj@kernel.org>
      6ea9e7bb
    • Clemens Ladisch's avatar
      firewire: optimize iso queueing by setting wake only after the last packet · 13882a82
      Clemens Ladisch authored
      
      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: default avatarClemens Ladisch <clemens@ladisch.de>
      Signed-off-by: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      13882a82
    • Stefan Richter's avatar
      firewire: octlet AT payloads can be stack-allocated · f30e6d3e
      Stefan Richter authored
      
      We do not need slab allocations anymore in order to satisfy
      streaming DMA mapping constraints, thanks to commit da28947e
      "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: default avatarStefan Richter <stefanr@s5r6.in-berlin.de>
      Acked-by: default avatarClemens Ladisch <clemens@ladisch.de>
      f30e6d3e
  8. May 02, 2011
    • B.J. Buchalter's avatar
      firewire: Fix for broken configrom updates in quick succession · 2e053a27
      B.J. Buchalter authored
      
      Current implementation of ohci_set_config_rom() uses a deferred
      bus reset via fw_schedule_bus_reset(). If clients add multiple
      unit descriptors to the config_rom in quick succession, the
      deferred bus reset may not have fired before succeeding update
      requests have come in. This can lead to an incorrect partial
      update of the config_rom for both addition and removal of
      config_rom descriptors, as the ohci_set_config_rom() routine
      will return -EBUSY if a previous pending update has not been
      completed yet; the requested update just gets dropped on the floor.
      
      This patch recognizes that the "in-flight" update can be modified
      until it has been processed by the bus-reset, and the locking
      in the bus_reset_tasklet ensures that the update is done atomically
      with respect to modifications made by ohci_set_config_rom(). The
      -EBUSY error case is simply removed.
      
      [Stefan R:  The bug always existed at least theoretically.  But it
      became easy to trigger since 2.6.36 commit 02d37bed "firewire: core:
      integrate software-forced bus resets with bus management" which
      introduced long mandatory delays between janitorial bus resets.]
      
      Signed-off-by: default avatarBenjamin Buchalter <bj@mhlabs.com>
      Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de> (trivial style changes)
      Cc: <stable@kernel.org> # 2.6.36.y and newer
      2e053a27
  9. Apr 19, 2011
  10. Mar 31, 2011
  11. Mar 20, 2011
  12. Mar 15, 2011
  13. Mar 14, 2011
  14. Feb 26, 2011
  15. Jan 23, 2011
  16. Jan 20, 2011
Loading