Re: [Freedreno] [PATCH v3 00/23] drm/msm: de-struct_mutex-ification
On Mon, Oct 19, 2020 at 10:45 PM Rob Clark wrote: > > From: Rob Clark > > This doesn't remove *all* the struct_mutex, but it covers the worst > of it, ie. shrinker/madvise/free/retire. The submit path still uses > struct_mutex, but it still needs *something* serialize a portion of > the submit path, and lock_stat mostly just shows the lock contention > there being with other submits. And there are a few other bits of > struct_mutex usage in less critical paths (debugfs, etc). But this > seems like a reasonable step in the right direction. > > v2: teach lockdep about shrinker locking patters (danvet) and > convert to obj->resv locking (danvet) > v3: fix get_vaddr locking for legacy userspace (relocs), devcoredump, > and rd/hangrd For the series: Reviewed-by: Kristian H. Kristensen > Rob Clark (23): > drm/msm: Fix a couple incorrect usages of get_vaddr_active() > drm/msm/gem: Add obj->lock wrappers > drm/msm/gem: Rename internal get_iova_locked helper > drm/msm/gem: Move prototypes to msm_gem.h > drm/msm/gem: Add some _locked() helpers > drm/msm/gem: Move locking in shrinker path > drm/msm/submit: Move copy_from_user ahead of locking bos > drm/msm: Do rpm get sooner in the submit path > drm/msm/gem: Switch over to obj->resv for locking > drm/msm: Use correct drm_gem_object_put() in fail case > drm/msm: Drop chatty trace > drm/msm: Move update_fences() > drm/msm: Add priv->mm_lock to protect active/inactive lists > drm/msm: Document and rename preempt_lock > drm/msm: Protect ring->submits with it's own lock > drm/msm: Refcount submits > drm/msm: Remove obj->gpu > drm/msm: Drop struct_mutex from the retire path > drm/msm: Drop struct_mutex in free_object() path > drm/msm: Remove msm_gem_free_work > drm/msm: Drop struct_mutex in madvise path > drm/msm: Drop struct_mutex in shrinker path > drm/msm: Don't implicit-sync if only a single ring > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +- > drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +- > drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + > drivers/gpu/drm/msm/dsi/dsi_host.c| 1 + > drivers/gpu/drm/msm/msm_debugfs.c | 7 + > drivers/gpu/drm/msm/msm_drv.c | 21 +- > drivers/gpu/drm/msm/msm_drv.h | 73 +- > drivers/gpu/drm/msm/msm_fbdev.c | 1 + > drivers/gpu/drm/msm/msm_gem.c | 266 +++--- > drivers/gpu/drm/msm/msm_gem.h | 133 +-- > drivers/gpu/drm/msm/msm_gem_shrinker.c| 81 ++- > drivers/gpu/drm/msm/msm_gem_submit.c | 158 - > drivers/gpu/drm/msm/msm_gpu.c | 110 + > drivers/gpu/drm/msm/msm_gpu.h | 5 +- > drivers/gpu/drm/msm/msm_rd.c | 2 +- > drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +- > drivers/gpu/drm/msm/msm_ringbuffer.h | 13 +- > 19 files changed, 495 insertions(+), 405 deletions(-) > > -- > 2.26.2 > > ___ > Freedreno mailing list > freedr...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v3 07/23] drm/msm/submit: Move copy_from_user ahead of locking bos
On Mon, Oct 19, 2020 at 10:45 PM Rob Clark wrote:
>
> From: Rob Clark
>
> We cannot switch to using obj->resv for locking without first moving all
> the copy_from_user() ahead of submit_lock_objects(). Otherwise in the
> mm fault path we aquire mm->mmap_sem before obj lock, but in the submit
> path the order is reversed.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem.h| 3 +
> drivers/gpu/drm/msm/msm_gem_submit.c | 121 ---
> 2 files changed, 76 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index c5232b8da794..0b7dda312992 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -240,7 +240,10 @@ struct msm_gem_submit {
> uint32_t type;
> uint32_t size; /* in dwords */
> uint64_t iova;
> + uint32_t offset;/* in dwords */
> uint32_t idx; /* cmdstream buffer idx in bos[] */
> + uint32_t nr_relocs;
> + struct drm_msm_gem_submit_reloc *relocs;
> } *cmd; /* array of size nr_cmds */
> struct {
> uint32_t flags;
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index aa5c60a7132d..002130d826aa 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -62,11 +62,16 @@ static struct msm_gem_submit *submit_create(struct
> drm_device *dev,
>
> void msm_gem_submit_free(struct msm_gem_submit *submit)
> {
> + unsigned i;
> +
> dma_fence_put(submit->fence);
> list_del(>node);
> put_pid(submit->pid);
> msm_submitqueue_put(submit->queue);
>
> + for (i = 0; i < submit->nr_cmds; i++)
> + kfree(submit->cmd[i].relocs);
> +
> kfree(submit);
> }
>
> @@ -150,6 +155,60 @@ static int submit_lookup_objects(struct msm_gem_submit
> *submit,
> return ret;
> }
>
> +static int submit_lookup_cmds(struct msm_gem_submit *submit,
> + struct drm_msm_gem_submit *args, struct drm_file *file)
> +{
> + unsigned i, sz;
> + int ret = 0;
> +
> + for (i = 0; i < args->nr_cmds; i++) {
> + struct drm_msm_gem_submit_cmd submit_cmd;
> + void __user *userptr =
> + u64_to_user_ptr(args->cmds + (i *
> sizeof(submit_cmd)));
> +
> + ret = copy_from_user(_cmd, userptr,
> sizeof(submit_cmd));
> + if (ret) {
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + /* validate input from userspace: */
> + switch (submit_cmd.type) {
> + case MSM_SUBMIT_CMD_BUF:
> + case MSM_SUBMIT_CMD_IB_TARGET_BUF:
> + case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
> + break;
> + default:
> + DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
> + return -EINVAL;
> + }
> +
> + if (submit_cmd.size % 4) {
> + DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
> + submit_cmd.size);
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + submit->cmd[i].type = submit_cmd.type;
> + submit->cmd[i].size = submit_cmd.size / 4;
> + submit->cmd[i].offset = submit_cmd.submit_offset / 4;
> + submit->cmd[i].idx = submit_cmd.submit_idx;
> + submit->cmd[i].nr_relocs = submit_cmd.nr_relocs;
> +
> + sz = sizeof(struct drm_msm_gem_submit_reloc) *
> submit_cmd.nr_relocs;
> + submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL);
kmalloc_array() or check_mul_overflow() here for the integer overflow check.
> + ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
> + if (ret) {
> + ret = -EFAULT;
> + goto out;
> + }
> + }
> +
> +out:
> + return ret;
> +}
> +
> static void submit_unlock_unpin_bo(struct msm_gem_submit *submit,
> int i, bool backoff)
> {
> @@ -301,7 +360,7 @@ static int submit_bo(struct msm_gem_submit *submit,
> uint32_t idx,
>
> /* process the reloc's and patch up the cmdstream as needed: */
> static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object
> *obj,
> - uint32_t offset, uint32_t nr_relocs, uint64_t relocs)
> + uint32_t offset, uint32_t nr_relocs, struct
> drm_msm_gem_submit_reloc *relocs)
> {
> uint32_t i, last_offset = 0;
> uint32_t *ptr;
> @@ -327,18 +386,11 @@ static int submit_reloc(struct msm_gem_submit *submit,
> struct msm_gem_object *ob
> }
>
> for (i = 0; i < nr_relocs; i++) {
> -
Re: [Freedreno] [PATCH v3 06/23] drm/msm/gem: Move locking in shrinker path
On Mon, Oct 19, 2020 at 10:45 PM Rob Clark wrote:
>
> From: Rob Clark
>
> Move grabbing the bo lock into shrinker, with a msm_gem_trylock() to
> skip over bo's that are already locked. This gets rid of the nested
> lock classes.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem.c | 24 +
> drivers/gpu/drm/msm/msm_gem.h | 29 ++
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 27 +---
> 3 files changed, 35 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index e0d8d739b068..1195847714ba 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -17,8 +17,6 @@
> #include "msm_gpu.h"
> #include "msm_mmu.h"
>
> -static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
> -
>
> static dma_addr_t physaddr(struct drm_gem_object *obj)
> {
> @@ -693,20 +691,19 @@ int msm_gem_madvise(struct drm_gem_object *obj,
> unsigned madv)
> return (madv != __MSM_MADV_PURGED);
> }
>
> -void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass)
> +void msm_gem_purge(struct drm_gem_object *obj)
> {
> struct drm_device *dev = obj->dev;
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> WARN_ON(!mutex_is_locked(>struct_mutex));
> + WARN_ON(!msm_gem_is_locked(obj));
> WARN_ON(!is_purgeable(msm_obj));
> WARN_ON(obj->import_attach);
>
> - mutex_lock_nested(_obj->lock, subclass);
> -
> put_iova(obj);
>
> - msm_gem_vunmap_locked(obj);
> + msm_gem_vunmap(obj);
>
> put_pages(obj);
>
> @@ -724,11 +721,9 @@ void msm_gem_purge(struct drm_gem_object *obj, enum
> msm_gem_lock subclass)
>
> invalidate_mapping_pages(file_inode(obj->filp)->i_mapping,
> 0, (loff_t)-1);
> -
> - msm_gem_unlock(obj);
> }
>
> -static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
> +void msm_gem_vunmap(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
>
> @@ -741,15 +736,6 @@ static void msm_gem_vunmap_locked(struct drm_gem_object
> *obj)
> msm_obj->vaddr = NULL;
> }
>
> -void msm_gem_vunmap(struct drm_gem_object *obj, enum msm_gem_lock subclass)
> -{
> - struct msm_gem_object *msm_obj = to_msm_bo(obj);
> -
> - mutex_lock_nested(_obj->lock, subclass);
> - msm_gem_vunmap_locked(obj);
> - msm_gem_unlock(obj);
> -}
> -
> /* must be called before _move_to_active().. */
> int msm_gem_sync_object(struct drm_gem_object *obj,
> struct msm_fence_context *fctx, bool exclusive)
> @@ -986,7 +972,7 @@ static void free_object(struct msm_gem_object *msm_obj)
>
> drm_prime_gem_destroy(obj, msm_obj->sgt);
> } else {
> - msm_gem_vunmap_locked(obj);
> + msm_gem_vunmap(obj);
> put_pages(obj);
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index d55d5401a2d2..c5232b8da794 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -162,6 +162,13 @@ msm_gem_lock(struct drm_gem_object *obj)
> mutex_lock(_obj->lock);
> }
>
> +static inline bool __must_check
> +msm_gem_trylock(struct drm_gem_object *obj)
> +{
> + struct msm_gem_object *msm_obj = to_msm_bo(obj);
> + return mutex_trylock_recursive(_obj->lock) ==
> MUTEX_TRYLOCK_SUCCESS;
This can just be
return mutex_trylock(_obj->lock) == 1;
now, right?
> +}
> +
> static inline int
> msm_gem_lock_interruptible(struct drm_gem_object *obj)
> {
> @@ -190,6 +197,7 @@ static inline bool is_active(struct msm_gem_object
> *msm_obj)
>
> static inline bool is_purgeable(struct msm_gem_object *msm_obj)
> {
> + WARN_ON(!msm_gem_is_locked(_obj->base));
> WARN_ON(!mutex_is_locked(_obj->base.dev->struct_mutex));
> return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
> !msm_obj->base.dma_buf &&
> !msm_obj->base.import_attach;
> @@ -197,27 +205,12 @@ static inline bool is_purgeable(struct msm_gem_object
> *msm_obj)
>
> static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
> {
> + WARN_ON(!msm_gem_is_locked(_obj->base));
> return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
> }
>
> -/* The shrinker can be triggered while we hold objA->lock, and need
> - * to grab objB->lock to purge it. Lockdep just sees these as a single
> - * class of lock, so we use subclasses to teach it the difference.
> - *
> - * OBJ_LOCK_NORMAL is implicit (ie. normal mutex_lock() call), and
> - * OBJ_LOCK_SHRINKER is used by shrinker.
> - *
> - * It is *essential* that we never go down paths that could trigger the
> - * shrinker for a purgable object. This is ensured by checking that
> - * msm_obj->madv == MSM_MADV_WILLNEED.
> - */
> -enum msm_gem_lock {
> - OBJ_LOCK_NORMAL,
>
Re: [Freedreno] [PATCH 00/14] drm/msm: de-struct_mutex-ification
On Sun, Oct 4, 2020 at 9:21 PM Rob Clark wrote: > > From: Rob Clark > > This doesn't remove *all* the struct_mutex, but it covers the worst > of it, ie. shrinker/madvise/free/retire. The submit path still uses > struct_mutex, but it still needs *something* serialize a portion of > the submit path, and lock_stat mostly just shows the lock contention > there being with other submits. And there are a few other bits of > struct_mutex usage in less critical paths (debugfs, etc). But this > seems like a reasonable step in the right direction. What a great patch set. Daniel has some good points and nothing that requires big changes, but on the other hand, I'm not sure it's something that needs to block this set either. Either way, for the series Reviewed-by: Kristian H. Kristensen > Rob Clark (14): > drm/msm: Use correct drm_gem_object_put() in fail case > drm/msm: Drop chatty trace > drm/msm: Move update_fences() > drm/msm: Add priv->mm_lock to protect active/inactive lists > drm/msm: Document and rename preempt_lock > drm/msm: Protect ring->submits with it's own lock > drm/msm: Refcount submits > drm/msm: Remove obj->gpu > drm/msm: Drop struct_mutex from the retire path > drm/msm: Drop struct_mutex in free_object() path > drm/msm: remove msm_gem_free_work > drm/msm: drop struct_mutex in madvise path > drm/msm: Drop struct_mutex in shrinker path > drm/msm: Don't implicit-sync if only a single ring > > drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +- > drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +-- > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 +- > drivers/gpu/drm/msm/msm_debugfs.c | 7 ++ > drivers/gpu/drm/msm/msm_drv.c | 15 +--- > drivers/gpu/drm/msm/msm_drv.h | 19 +++-- > drivers/gpu/drm/msm/msm_gem.c | 76 ++ > drivers/gpu/drm/msm/msm_gem.h | 53 + > drivers/gpu/drm/msm/msm_gem_shrinker.c| 58 ++ > drivers/gpu/drm/msm/msm_gem_submit.c | 17 ++-- > drivers/gpu/drm/msm/msm_gpu.c | 96 ++- > drivers/gpu/drm/msm/msm_gpu.h | 5 +- > drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +- > drivers/gpu/drm/msm/msm_ringbuffer.h | 13 ++- > 14 files changed, 188 insertions(+), 194 deletions(-) > > -- > 2.26.2 > > ___ > Freedreno mailing list > freedr...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH 13/14] drm/msm: Drop struct_mutex in shrinker path
On Mon, Oct 5, 2020 at 4:02 PM Daniel Vetter wrote:
>
> On Mon, Oct 05, 2020 at 05:24:19PM +0800, Hillf Danton wrote:
> >
> > On Sun, 4 Oct 2020 12:21:45
> > > From: Rob Clark
> > >
> > > Now that the inactive_list is protected by mm_lock, and everything
> > > else on per-obj basis is protected by obj->lock, we no longer depend
> > > on struct_mutex.
> > >
> > > Signed-off-by: Rob Clark
> > > ---
> > > drivers/gpu/drm/msm/msm_gem.c | 1 -
> > > drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --
> > > 2 files changed, 55 deletions(-)
> > >
> > [...]
> >
> > > @@ -71,13 +33,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker,
> > > struct shrink_control *sc)
> > > {
> > > struct msm_drm_private *priv =
> > > container_of(shrinker, struct msm_drm_private, shrinker);
> > > - struct drm_device *dev = priv->dev;
> > > struct msm_gem_object *msm_obj;
> > > unsigned long freed = 0;
> > > - bool unlock;
> > > -
> > > - if (!msm_gem_shrinker_lock(dev, ))
> > > - return SHRINK_STOP;
> > >
> > > mutex_lock(>mm_lock);
> >
> > Better if the change in behavior is documented that SHRINK_STOP will
> > no longer be needed.
>
> btw I read through this and noticed you have your own obj lock, plus
> mutex_lock_nested. I strongly recommend to just cut over to dma_resv_lock
> for all object lock needs (soc drivers have been terrible with this
> unfortuntaly), and in the shrinker just use dma_resv_trylock instead of
> trying to play clever games outsmarting lockdep.
>
> I recently wrote an entire blog length rant on why I think
> mutex_lock_nested is too dangerous to be useful:
>
> https://blog.ffwll.ch/2020/08/lockdep-false-positives.html
>
> Not anything about this here, just general comment. The problem extends to
> shmem helpers and all that also having their own locks for everything.
This is definitely a tangible improvement though - very happy to see
msm_gem_shrinker_lock() go.
Reviewed-by: Kristian H. Kristensen
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Freedreno] [PATCH 2/3] drm/msm: Convert shrinker msgs to tracepoints
On Tue, Sep 1, 2020 at 8:41 AM Rob Clark wrote:
>
> From: Rob Clark
>
> This reduces the spam in dmesg when we start hitting the shrinker, and
> replaces it with something we can put on a timeline while profiling or
> debugging system issues.
That is a good solution,
Reviewed-by: Kristian H. Kristensen
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem_shrinker.c | 5 +++--
> drivers/gpu/drm/msm/msm_gpu_trace.h| 26 ++
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> index 722d61668a97..482576d7a39a 100644
> --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
> +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
> @@ -6,6 +6,7 @@
>
> #include "msm_drv.h"
> #include "msm_gem.h"
> +#include "msm_gpu_trace.h"
>
> static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
> {
> @@ -87,7 +88,7 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
> shrink_control *sc)
> mutex_unlock(>struct_mutex);
>
> if (freed > 0)
> - pr_info_ratelimited("Purging %lu bytes\n", freed <<
> PAGE_SHIFT);
> + trace_msm_gem_purge(freed << PAGE_SHIFT);
>
> return freed;
> }
> @@ -123,7 +124,7 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned
> long event, void *ptr)
> *(unsigned long *)ptr += unmapped;
>
> if (unmapped > 0)
> - pr_info_ratelimited("Purging %u vmaps\n", unmapped);
> + trace_msm_gem_purge_vmaps(unmapped);
>
> return NOTIFY_DONE;
> }
> diff --git a/drivers/gpu/drm/msm/msm_gpu_trace.h
> b/drivers/gpu/drm/msm/msm_gpu_trace.h
> index 07572ab179fa..1079fe551279 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_trace.h
> +++ b/drivers/gpu/drm/msm/msm_gpu_trace.h
> @@ -114,6 +114,32 @@ TRACE_EVENT(msm_gmu_freq_change,
> TP_printk("freq=%u, perf_index=%u", __entry->freq,
> __entry->perf_index)
> );
>
> +
> +TRACE_EVENT(msm_gem_purge,
> + TP_PROTO(u32 bytes),
> + TP_ARGS(bytes),
> + TP_STRUCT__entry(
> + __field(u32, bytes)
> + ),
> + TP_fast_assign(
> + __entry->bytes = bytes;
> + ),
> + TP_printk("Purging %u bytes", __entry->bytes)
> +);
> +
> +
> +TRACE_EVENT(msm_gem_purge_vmaps,
> + TP_PROTO(u32 unmapped),
> + TP_ARGS(unmapped),
> + TP_STRUCT__entry(
> + __field(u32, unmapped)
> + ),
> + TP_fast_assign(
> + __entry->unmapped = unmapped;
> + ),
> + TP_printk("Purging %u vmaps", __entry->unmapped)
> +);
> +
> #endif
>
> #undef TRACE_INCLUDE_PATH
> --
> 2.26.2
>
> ___
> Freedreno mailing list
> freedr...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v1 2/3] drm/msm: Print all 64 bits of the faulting IOMMU address
On Tue, May 7, 2019 at 11:02 AM Jordan Crouse wrote:
>
> When we move to 64 bit addressing for a5xx and a6xx targets we will start
> seeing pagefaults at larger addresses so format them appropriately in the
> log message for easier debugging.
Yes please, this has confused me more than once.
Reviewed-by: Kristian H. Kristensen
> Signed-off-by: Jordan Crouse
> ---
>
> drivers/gpu/drm/msm/msm_iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 12bb54c..1926329 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -30,7 +30,7 @@ static int msm_fault_handler(struct iommu_domain *domain,
> struct device *dev,
> struct msm_iommu *iommu = arg;
> if (iommu->base.handler)
> return iommu->base.handler(iommu->base.arg, iova, flags);
> - pr_warn_ratelimited("*** fault: iova=%08lx, flags=%d\n", iova, flags);
> + pr_warn_ratelimited("*** fault: iova=%16lx, flags=%d\n", iova, flags);
> return 0;
> }
>
> --
> 2.7.4
>
> ___
> Freedreno mailing list
> freedr...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394
On 10/1/07, Pieter Palmers <[EMAIL PROTECTED]> wrote:
> Stefan Richter wrote:
> >> This duplicates the read cycle timer feature of raw1394 (added in Linux
> >> 2.6.21) in firewire-core's userspace ABI.
> >
> > Kristian and Pieter, does this simple duplication of the ioctl make
> > sense on its own? AFAIU rawiso's iso packet buffers look different from
> > fw-cdevs's. It seems to me as if rawiso always put the cycle into a user
> > buffer for each iso packet received...
> >
> > raw1394.h::struct raw1394_iso_packet_info {
> > __u32 offset;
> > __u16 len;
> > __u16 cycle; /* recv only */
> > __u8 channel; /* recv only */
> > __u8 tag;
> > __u8 sy;
> > };
> >
> > raw1394.c::raw1394_iso_recv_packets()
> >
> > /* copy the packet_infos out */
> > for (i = 0; i < upackets.n_packets; i++) {
> > if (__copy_to_user([i],
> > >iso_handle->infos[packet],
> > sizeof(struct raw1394_iso_packet_info)))
> > return -EFAULT;
> >
> > packet = (packet + 1) % fi->iso_handle->buf_packets;
> > }
> >
> > ...while the Juju ABI returns the cycle only for those packets whose
> > fw_cdev_iso_packet.control had the FW_CDEV_ISO_INTERRUPT flag set.
> > The cycle is then written out in the fw_cdev_event_iso_interrupt event
> > which happens when this particular packet was received. Right?
> >
> > Pieter, do applications like yours need the cycle counter only for a few
> > predetermined packets or for each and every packet?
>
> We need it for every packet for two reasons:
> 1) it's the only way to determine how many packets were dropped when
> packet drops are flagged in the callback
Your application should know what the timestamp should be for each iso
receive callback and if you see a larger value than expected you can
use that to calculate how many cycles were lost.
> 2) we convert the 16-bit SYT timestamp of a packet to a full 32-bit
> cycle counter value. This because the range of the 16-bit SYT is too
> small (only 16 packets) for systems that have large buffering.
If you get the timestamp for the first packet in a receive batch, you
can still do this, right?
cheers,
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] firewire: adopt read cycle timer ABI from raw1394
On 10/1/07, Pieter Palmers [EMAIL PROTECTED] wrote:
Stefan Richter wrote:
This duplicates the read cycle timer feature of raw1394 (added in Linux
2.6.21) in firewire-core's userspace ABI.
Kristian and Pieter, does this simple duplication of the ioctl make
sense on its own? AFAIU rawiso's iso packet buffers look different from
fw-cdevs's. It seems to me as if rawiso always put the cycle into a user
buffer for each iso packet received...
raw1394.h::struct raw1394_iso_packet_info {
__u32 offset;
__u16 len;
__u16 cycle; /* recv only */
__u8 channel; /* recv only */
__u8 tag;
__u8 sy;
};
raw1394.c::raw1394_iso_recv_packets()
/* copy the packet_infos out */
for (i = 0; i upackets.n_packets; i++) {
if (__copy_to_user(upackets.infos[i],
fi-iso_handle-infos[packet],
sizeof(struct raw1394_iso_packet_info)))
return -EFAULT;
packet = (packet + 1) % fi-iso_handle-buf_packets;
}
...while the Juju ABI returns the cycle only for those packets whose
fw_cdev_iso_packet.control had the FW_CDEV_ISO_INTERRUPT flag set.
The cycle is then written out in the fw_cdev_event_iso_interrupt event
which happens when this particular packet was received. Right?
Pieter, do applications like yours need the cycle counter only for a few
predetermined packets or for each and every packet?
We need it for every packet for two reasons:
1) it's the only way to determine how many packets were dropped when
packet drops are flagged in the callback
Your application should know what the timestamp should be for each iso
receive callback and if you see a larger value than expected you can
use that to calculate how many cycles were lost.
2) we convert the 16-bit SYT timestamp of a packet to a full 32-bit
cycle counter value. This because the range of the 16-bit SYT is too
small (only 16 packets) for systems that have large buffering.
If you get the timestamp for the first packet in a receive batch, you
can still do this, right?
cheers,
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 00/23] drm: introduce drm_zalloc
On Tue, 2007-08-28 at 21:50 +0100, Dave Airlie wrote: > On Tue, 28 Aug 2007, Christoph Hellwig wrote: > > > On Mon, Aug 27, 2007 at 10:57:50PM +0200, [EMAIL PROTECTED] wrote: > >> Hello, > >> > >>As there are many places in drm code where drm_alloc + memset is used > >> this patch series introduces drm_zalloc and also makes use of drm_calloc > >> where > >> needed. Most of these patches save some bytes so the benefit is a few kB > >> saved > >> (gcc 4.1.2) with patch applied. Also some small (style, etc.) things are > >> fixed. > >> This patch series does the conversion drm tree-wide. All patches were > >> compile > >> tested. > > > > Please just convert it to plain kzalloc/kcalloc and kill these utterly > > useless > > wrappers instead. > > > > > > The wrappers aren't useless the drm alloc/free passes in a memory space > for debugging purposes so we can track memory abuse when developing, Do we ever use that, though? Having to pass in the pointer, the size and the area just to free memory, sure is a bitch. > but drm_zalloc shouldjust alias to drm_calloc really.. drm_calloc calls kcalloc which performs an integer overflow check on the 'n' and 'size' arguments, which isn't needed for drm_zalloc. Small detail, of course, but I don't see the problem with aliasing to kzalloc. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 00/23] drm: introduce drm_zalloc
On Tue, 2007-08-28 at 21:50 +0100, Dave Airlie wrote: On Tue, 28 Aug 2007, Christoph Hellwig wrote: On Mon, Aug 27, 2007 at 10:57:50PM +0200, [EMAIL PROTECTED] wrote: Hello, As there are many places in drm code where drm_alloc + memset is used this patch series introduces drm_zalloc and also makes use of drm_calloc where needed. Most of these patches save some bytes so the benefit is a few kB saved (gcc 4.1.2) with patch applied. Also some small (style, etc.) things are fixed. This patch series does the conversion drm tree-wide. All patches were compile tested. Please just convert it to plain kzalloc/kcalloc and kill these utterly useless wrappers instead. The wrappers aren't useless the drm alloc/free passes in a memory space for debugging purposes so we can track memory abuse when developing, Do we ever use that, though? Having to pass in the pointer, the size and the area just to free memory, sure is a bitch. but drm_zalloc shouldjust alias to drm_calloc really.. drm_calloc calls kcalloc which performs an integer overflow check on the 'n' and 'size' arguments, which isn't needed for drm_zalloc. Small detail, of course, but I don't see the problem with aliasing to kzalloc. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
On Mon, 2007-06-25 at 16:31 -0400, Steven Rostedt wrote: > On Mon, 2007-06-25 at 16:07 -0400, Kristian Høgsberg wrote: > > > > Maybe we should be looking at something like GENERIC_SOFTIRQ to run > > > functions that a driver could add. But they would run only on the CPU > > > that scheduled them, and do not guarantee non-reentrant as tasklets do > > > today. > > > > Sounds like this will fill the gap. Of course, this won't reduce the > > number of delayed-execution mechanisms available... > > I disagree. Adding a generic softirq is not really adding another > delayed-execution, it's just extending the sofitrq. It would not have > any different semantics as a normal softirq, except that it would be > dynamic for modules to use. A tasklet has different concepts than > softirq. It adds non-reentrancy and that the tasklet function can run > where it wasn't scheduled. Hmm, yeah, true. Ok, sounds useful, let me know when you have a patch. I'll try porting the firewire stack and let you know how it works. Just to make sure I got this right: softirqs will always be scheduled on the irq handling CPU and if an interrupt schedules multiple softirqs from one handler invocation, the softirqs will be executed in the order they are scheduled? And the reentrancy that softirqs does not protect against (as opposed to tasklets) is the case where different CPUs handle the interrupt and each schedule the same softirq for execution? Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
On Mon, 2007-06-25 at 15:11 -0400, Steven Rostedt wrote: > On Mon, 2007-06-25 at 14:48 -0400, Kristian Høgsberg wrote: > ... > > However, I don't really understand how you can discuss a wholesale > > replacing of tasklets with workqueues, given the very different > > execution sematics of the two mechanisms. I would think that others > > have used tasklets for similar purposes as I have and moving that to > > workqueues just has to break a bunch of stuff. I don't know the various > > places tasklets are used as well as other people in this thread, but I > > think deprecating them and moving code to either softirqs or workqueues > > on a case by case basis is a better approach. That way we also avoid > > the gross wrappers. > > The gross wrappers were a perfect way to shed light on something that is > overused, and should most likely be replaced. > > Does your system need to have these functions that are in tasklets need > to be non-reentrant? I wonder how many "irq critical" functions used > tasklets just because adding a softirq requires too much (no generic > softirq code). A tasklet is constrained to run on one CPU at a time, > and it is not guaranteed to run on the CPU it was scheduled on. When I started the firewire work, I wasn't aware that tasklets were going away, but I knew that doing too much work in the interrupt handler was frowned upon, for good reasons. So I was looking at softirqs vs taslkets, and since using softirqs means you have to go add yourself to the big bitmask, I opted for tasklets. The comment in interrupt.h directly recommends this. As it stands, the firewire stack does actaully rely on the non-reentrancy of tasklets, but that's not a deal-breaker, I can add the necessary locking. > Perhaps it's time to add a new functionality while removing tasklets. > Things that are ok to bounce around CPUs (like tasklets do) can most > likely be replaced by a work queue. But these highly critical tasks > probably would benefit from being a softirq. > > Maybe we should be looking at something like GENERIC_SOFTIRQ to run > functions that a driver could add. But they would run only on the CPU > that scheduled them, and do not guarantee non-reentrant as tasklets do > today. Sounds like this will fill the gap. Of course, this won't reduce the number of delayed-execution mechanisms available... Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
On Fri, 2007-06-22 at 23:59 +0200, Ingo Molnar wrote: > so how about the following, different approach: anyone who has a tasklet > in any performance-sensitive codepath, please yell now. We'll also do a > proactive search for such places. We can convert those places to > softirqs, or move them back into hardirq context. Once this is done - > and i doubt it will go beyond 1-2 places - we can just mass-convert the > other 110 places to the lame but compatible solution of doing them in a > global thread context. OK, here's a yell. I'm using tasklets in the new firewire stack for all interrupt handling. All my interrupt handler does is read out the event mask and schedule the appropriate tasklets. Most of these tasklets typically just end up scheduling work or completing a completion, so moving it to a workqueue is pretty pointless. In particular, the isochronous DMA events must be handled with as little latency as possible, so a workqueue in that code path would be pretty bad. I'm not strongly attached to tasklets, and it sounds like I got it wrong and used the wrong delayed execution mechanism. But that's just another data point that suggests that there are too many of these. I guess I need to sit down and look into porting that to softirqs? However, I don't really understand how you can discuss a wholesale replacing of tasklets with workqueues, given the very different execution sematics of the two mechanisms. I would think that others have used tasklets for similar purposes as I have and moving that to workqueues just has to break a bunch of stuff. I don't know the various places tasklets are used as well as other people in this thread, but I think deprecating them and moving code to either softirqs or workqueues on a case by case basis is a better approach. That way we also avoid the gross wrappers. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
On Fri, 2007-06-22 at 23:59 +0200, Ingo Molnar wrote: so how about the following, different approach: anyone who has a tasklet in any performance-sensitive codepath, please yell now. We'll also do a proactive search for such places. We can convert those places to softirqs, or move them back into hardirq context. Once this is done - and i doubt it will go beyond 1-2 places - we can just mass-convert the other 110 places to the lame but compatible solution of doing them in a global thread context. OK, here's a yell. I'm using tasklets in the new firewire stack for all interrupt handling. All my interrupt handler does is read out the event mask and schedule the appropriate tasklets. Most of these tasklets typically just end up scheduling work or completing a completion, so moving it to a workqueue is pretty pointless. In particular, the isochronous DMA events must be handled with as little latency as possible, so a workqueue in that code path would be pretty bad. I'm not strongly attached to tasklets, and it sounds like I got it wrong and used the wrong delayed execution mechanism. But that's just another data point that suggests that there are too many of these. I guess I need to sit down and look into porting that to softirqs? However, I don't really understand how you can discuss a wholesale replacing of tasklets with workqueues, given the very different execution sematics of the two mechanisms. I would think that others have used tasklets for similar purposes as I have and moving that to workqueues just has to break a bunch of stuff. I don't know the various places tasklets are used as well as other people in this thread, but I think deprecating them and moving code to either softirqs or workqueues on a case by case basis is a better approach. That way we also avoid the gross wrappers. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
On Mon, 2007-06-25 at 15:11 -0400, Steven Rostedt wrote: On Mon, 2007-06-25 at 14:48 -0400, Kristian Høgsberg wrote: ... However, I don't really understand how you can discuss a wholesale replacing of tasklets with workqueues, given the very different execution sematics of the two mechanisms. I would think that others have used tasklets for similar purposes as I have and moving that to workqueues just has to break a bunch of stuff. I don't know the various places tasklets are used as well as other people in this thread, but I think deprecating them and moving code to either softirqs or workqueues on a case by case basis is a better approach. That way we also avoid the gross wrappers. The gross wrappers were a perfect way to shed light on something that is overused, and should most likely be replaced. Does your system need to have these functions that are in tasklets need to be non-reentrant? I wonder how many irq critical functions used tasklets just because adding a softirq requires too much (no generic softirq code). A tasklet is constrained to run on one CPU at a time, and it is not guaranteed to run on the CPU it was scheduled on. When I started the firewire work, I wasn't aware that tasklets were going away, but I knew that doing too much work in the interrupt handler was frowned upon, for good reasons. So I was looking at softirqs vs taslkets, and since using softirqs means you have to go add yourself to the big bitmask, I opted for tasklets. The comment in interrupt.h directly recommends this. As it stands, the firewire stack does actaully rely on the non-reentrancy of tasklets, but that's not a deal-breaker, I can add the necessary locking. Perhaps it's time to add a new functionality while removing tasklets. Things that are ok to bounce around CPUs (like tasklets do) can most likely be replaced by a work queue. But these highly critical tasks probably would benefit from being a softirq. Maybe we should be looking at something like GENERIC_SOFTIRQ to run functions that a driver could add. But they would run only on the CPU that scheduled them, and do not guarantee non-reentrant as tasklets do today. Sounds like this will fill the gap. Of course, this won't reduce the number of delayed-execution mechanisms available... Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 0/6] Convert all tasklets to workqueues
On Mon, 2007-06-25 at 16:31 -0400, Steven Rostedt wrote: On Mon, 2007-06-25 at 16:07 -0400, Kristian Høgsberg wrote: Maybe we should be looking at something like GENERIC_SOFTIRQ to run functions that a driver could add. But they would run only on the CPU that scheduled them, and do not guarantee non-reentrant as tasklets do today. Sounds like this will fill the gap. Of course, this won't reduce the number of delayed-execution mechanisms available... I disagree. Adding a generic softirq is not really adding another delayed-execution, it's just extending the sofitrq. It would not have any different semantics as a normal softirq, except that it would be dynamic for modules to use. A tasklet has different concepts than softirq. It adds non-reentrancy and that the tasklet function can run where it wasn't scheduled. Hmm, yeah, true. Ok, sounds useful, let me know when you have a patch. I'll try porting the firewire stack and let you know how it works. Just to make sure I got this right: softirqs will always be scheduled on the irq handling CPU and if an interrupt schedules multiple softirqs from one handler invocation, the softirqs will be executed in the order they are scheduled? And the reentrancy that softirqs does not protect against (as opposed to tasklets) is the case where different CPUs handle the interrupt and each schedule the same softirq for execution? Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] lib: add idr_remove_all
Remove all ids from the given idr tree. idr_destroy() only frees up
unused, cached idp_layers, but this function will remove all id mappings
and leave all idp_layers unused.
A typical clean-up sequence for objects stored in an idr tree, will
use idr_for_each() to free all objects, if necessay, then
idr_remove_all() to remove all ids, and idr_destroy() to free
up the cached idr_layers.
Signed-off-by: Kristian Hoegsberg <[EMAIL PROTECTED]>
---
include/linux/idr.h |1 +
lib/idr.c | 47 +++
2 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index d71735e..2d1e105 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -82,6 +82,7 @@ int idr_for_each(struct idr *idp,
int (*fn)(int id, void *p, void *data), void *data);
void *idr_replace(struct idr *idp, void *ptr, int id);
void idr_remove(struct idr *idp, int id);
+void idr_remove_all(struct idr *idp);
void idr_destroy(struct idr *idp);
void idr_init(struct idr *idp);
diff --git a/lib/idr.c b/lib/idr.c
index dd43be5..b4d18d4 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -358,6 +358,53 @@ void idr_remove(struct idr *idp, int id)
EXPORT_SYMBOL(idr_remove);
/**
+ * idr_remove_all - remove all ids from the given idr tree
+ * @idp: idr handle
+ *
+ * idr_destroy() only frees up unused, cached idp_layers, but this
+ * function will remove all id mappings and leave all idp_layers
+ * unused.
+ *
+ * A typical clean-up sequence for objects stored in an idr tree, will
+ * use idr_for_each() to free all objects, if necessay, then
+ * idr_remove_all() to remove all ids, and idr_destroy() to free
+ * up the cached idr_layers.
+ */
+void idr_remove_all(struct idr *idp)
+{
+ int n, id, max, error = 0;
+ struct idr_layer *p;
+ struct idr_layer *pa[MAX_LEVEL];
+ struct idr_layer **paa = [0];
+
+ n = idp->layers * IDR_BITS;
+ p = idp->top;
+ max = 1 << n;
+
+ id = 0;
+ while (id < max && !error) {
+ while (n > IDR_BITS && p) {
+ n -= IDR_BITS;
+ *paa++ = p;
+ p = p->ary[(id >> n) & IDR_MASK];
+ }
+
+ id += 1 << n;
+ while (n < fls(id)) {
+ if (p) {
+ memset(p, 0, sizeof *p);
+ free_layer(idp, p);
+ }
+ n += IDR_BITS;
+ p = *--paa;
+ }
+ }
+ idp->top = NULL;
+ idp->layers = 0;
+}
+EXPORT_SYMBOL(idr_remove_all);
+
+/**
* idr_destroy - release all cached layers within an idr tree
* idp: idr handle
*/
--
1.5.0.6
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] lib: add idr_for_each()
This patch adds an iterator function for the idr data structure. Compared
to just iterating through the idr with an integer and idr_find, this
iterator is (almost, but not quite) linear in the number of elements,
as opposed to the number of integers in the range covered by the idr. This
makes a difference for sparse idrs, but more importantly, it's a nicer
way to iterate through the elements.
The drm subsystem is moving to idr for tracking contexts and drawables, and
with this change, we can use the idr exclusively for tracking these
resources.
Signed-off-by: Kristian Hoegsberg <[EMAIL PROTECTED]>
---
include/linux/idr.h |2 +
lib/idr.c | 55 +++
2 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 8268034..d71735e 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -78,6 +78,8 @@ void *idr_find(struct idr *idp, int id);
int idr_pre_get(struct idr *idp, gfp_t gfp_mask);
int idr_get_new(struct idr *idp, void *ptr, int *id);
int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id);
+int idr_for_each(struct idr *idp,
+int (*fn)(int id, void *p, void *data), void *data);
void *idr_replace(struct idr *idp, void *ptr, int id);
void idr_remove(struct idr *idp, int id);
void idr_destroy(struct idr *idp);
diff --git a/lib/idr.c b/lib/idr.c
index 305117c..dd43be5 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -404,6 +404,61 @@ void *idr_find(struct idr *idp, int id)
EXPORT_SYMBOL(idr_find);
/**
+ * idr_for_each - iterate through all stored pointers
+ * @idp: idr handle
+ * @fn: function to be called for each pointer
+ * @data: data passed back to callback function
+ *
+ * Iterate over the pointers registered with the given idr. The
+ * callback function will be called for each pointer currently
+ * registered, passing the id, the pointer and the data pointer passed
+ * to this function. It is not safe to modify the idr tree while in
+ * the callback, so functions such as idr_get_new and idr_remove are
+ * not allowed.
+ *
+ * We check the return of @fn each time. If it returns anything other
+ * than 0, we break out and return that value.
+ *
+ * The caller must serialize idr_find() vs idr_get_new() and idr_remove().
+ */
+int idr_for_each(struct idr *idp,
+int (*fn)(int id, void *p, void *data), void *data)
+{
+ int n, id, max, error = 0;
+ struct idr_layer *p;
+ struct idr_layer *pa[MAX_LEVEL];
+ struct idr_layer **paa = [0];
+
+ n = idp->layers * IDR_BITS;
+ p = idp->top;
+ max = 1 << n;
+
+ id = 0;
+ while (id < max) {
+ while (n > 0 && p) {
+ n -= IDR_BITS;
+ *paa++ = p;
+ p = p->ary[(id >> n) & IDR_MASK];
+ }
+
+ if (p) {
+ error = fn(id, (void *)p, data);
+ if (error)
+ break;
+ }
+
+ id += 1 << n;
+ while (n < fls(id)) {
+ n += IDR_BITS;
+ p = *--paa;
+ }
+ }
+
+ return error;
+}
+EXPORT_SYMBOL(idr_for_each);
+
+/**
* idr_replace - replace pointer for given id
* @idp: idr handle
* @ptr: pointer you want associated with the id
--
1.5.0.6
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] lib: add idr_remove_all
Remove all ids from the given idr tree. idr_destroy() only frees up
unused, cached idp_layers, but this function will remove all id mappings
and leave all idp_layers unused.
A typical clean-up sequence for objects stored in an idr tree, will
use idr_for_each() to free all objects, if necessay, then
idr_remove_all() to remove all ids, and idr_destroy() to free
up the cached idr_layers.
Signed-off-by: Kristian Hoegsberg [EMAIL PROTECTED]
---
include/linux/idr.h |1 +
lib/idr.c | 47 +++
2 files changed, 48 insertions(+), 0 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index d71735e..2d1e105 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -82,6 +82,7 @@ int idr_for_each(struct idr *idp,
int (*fn)(int id, void *p, void *data), void *data);
void *idr_replace(struct idr *idp, void *ptr, int id);
void idr_remove(struct idr *idp, int id);
+void idr_remove_all(struct idr *idp);
void idr_destroy(struct idr *idp);
void idr_init(struct idr *idp);
diff --git a/lib/idr.c b/lib/idr.c
index dd43be5..b4d18d4 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -358,6 +358,53 @@ void idr_remove(struct idr *idp, int id)
EXPORT_SYMBOL(idr_remove);
/**
+ * idr_remove_all - remove all ids from the given idr tree
+ * @idp: idr handle
+ *
+ * idr_destroy() only frees up unused, cached idp_layers, but this
+ * function will remove all id mappings and leave all idp_layers
+ * unused.
+ *
+ * A typical clean-up sequence for objects stored in an idr tree, will
+ * use idr_for_each() to free all objects, if necessay, then
+ * idr_remove_all() to remove all ids, and idr_destroy() to free
+ * up the cached idr_layers.
+ */
+void idr_remove_all(struct idr *idp)
+{
+ int n, id, max, error = 0;
+ struct idr_layer *p;
+ struct idr_layer *pa[MAX_LEVEL];
+ struct idr_layer **paa = pa[0];
+
+ n = idp-layers * IDR_BITS;
+ p = idp-top;
+ max = 1 n;
+
+ id = 0;
+ while (id max !error) {
+ while (n IDR_BITS p) {
+ n -= IDR_BITS;
+ *paa++ = p;
+ p = p-ary[(id n) IDR_MASK];
+ }
+
+ id += 1 n;
+ while (n fls(id)) {
+ if (p) {
+ memset(p, 0, sizeof *p);
+ free_layer(idp, p);
+ }
+ n += IDR_BITS;
+ p = *--paa;
+ }
+ }
+ idp-top = NULL;
+ idp-layers = 0;
+}
+EXPORT_SYMBOL(idr_remove_all);
+
+/**
* idr_destroy - release all cached layers within an idr tree
* idp: idr handle
*/
--
1.5.0.6
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] lib: add idr_for_each()
This patch adds an iterator function for the idr data structure. Compared
to just iterating through the idr with an integer and idr_find, this
iterator is (almost, but not quite) linear in the number of elements,
as opposed to the number of integers in the range covered by the idr. This
makes a difference for sparse idrs, but more importantly, it's a nicer
way to iterate through the elements.
The drm subsystem is moving to idr for tracking contexts and drawables, and
with this change, we can use the idr exclusively for tracking these
resources.
Signed-off-by: Kristian Hoegsberg [EMAIL PROTECTED]
---
include/linux/idr.h |2 +
lib/idr.c | 55 +++
2 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/include/linux/idr.h b/include/linux/idr.h
index 8268034..d71735e 100644
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -78,6 +78,8 @@ void *idr_find(struct idr *idp, int id);
int idr_pre_get(struct idr *idp, gfp_t gfp_mask);
int idr_get_new(struct idr *idp, void *ptr, int *id);
int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id);
+int idr_for_each(struct idr *idp,
+int (*fn)(int id, void *p, void *data), void *data);
void *idr_replace(struct idr *idp, void *ptr, int id);
void idr_remove(struct idr *idp, int id);
void idr_destroy(struct idr *idp);
diff --git a/lib/idr.c b/lib/idr.c
index 305117c..dd43be5 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -404,6 +404,61 @@ void *idr_find(struct idr *idp, int id)
EXPORT_SYMBOL(idr_find);
/**
+ * idr_for_each - iterate through all stored pointers
+ * @idp: idr handle
+ * @fn: function to be called for each pointer
+ * @data: data passed back to callback function
+ *
+ * Iterate over the pointers registered with the given idr. The
+ * callback function will be called for each pointer currently
+ * registered, passing the id, the pointer and the data pointer passed
+ * to this function. It is not safe to modify the idr tree while in
+ * the callback, so functions such as idr_get_new and idr_remove are
+ * not allowed.
+ *
+ * We check the return of @fn each time. If it returns anything other
+ * than 0, we break out and return that value.
+ *
+ * The caller must serialize idr_find() vs idr_get_new() and idr_remove().
+ */
+int idr_for_each(struct idr *idp,
+int (*fn)(int id, void *p, void *data), void *data)
+{
+ int n, id, max, error = 0;
+ struct idr_layer *p;
+ struct idr_layer *pa[MAX_LEVEL];
+ struct idr_layer **paa = pa[0];
+
+ n = idp-layers * IDR_BITS;
+ p = idp-top;
+ max = 1 n;
+
+ id = 0;
+ while (id max) {
+ while (n 0 p) {
+ n -= IDR_BITS;
+ *paa++ = p;
+ p = p-ary[(id n) IDR_MASK];
+ }
+
+ if (p) {
+ error = fn(id, (void *)p, data);
+ if (error)
+ break;
+ }
+
+ id += 1 n;
+ while (n fls(id)) {
+ n += IDR_BITS;
+ p = *--paa;
+ }
+ }
+
+ return error;
+}
+EXPORT_SYMBOL(idr_for_each);
+
+/**
* idr_replace - replace pointer for given id
* @idp: idr handle
* @ptr: pointer you want associated with the id
--
1.5.0.6
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc patch] firewire: prefix modules with firewire- instead of fw-
On 5/25/07, Stefan Richter <[EMAIL PROTECTED]> wrote: Of course everybody immediately associates "fw-" with FireWire, not firmware or firewall or whatever. But "firewire-" has a nice ring to it too. It's fine with me. I like the less ambiguous names better, and I don't think the length of the names is a problem. If you end up typing the module names too much, you're doing something wrong or working on the firewire stack, in which case you're screwed anyway :) Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [rfc patch] firewire: prefix modules with firewire- instead of fw-
On 5/25/07, Stefan Richter [EMAIL PROTECTED] wrote: Of course everybody immediately associates fw- with FireWire, not firmware or firewall or whatever. But firewire- has a nice ring to it too. It's fine with me. I like the less ambiguous names better, and I don't think the length of the names is a problem. If you end up typing the module names too much, you're doing something wrong or working on the firewire stack, in which case you're screwed anyway :) Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Race free attributes in sysfs
On 5/22/07, Pierre Ossman <[EMAIL PROTECTED]> wrote: Kay Sievers wrote: > We could change the driver-core to suppress the creation of an attribute > if the attribute's show() or store() method returns something like > -ENOENT at registration time? > The driver would pass _all_ possible attributes of the device at > registration time, but the core would only create the attributes which > are implemented for this particular device? Would that work for you? > Not sure. Not in an obvious way at least. It also doesn't feel like "the kernel way". Generally you can create/allocate an object, assign attributes to it, then activate it. Couldn't it be done so that I can add sysfs stuff to a device after I just initialized it? (but before I add it). I agree here - if it was just possible to do this between create and add, this discussion would be moot. > You can assign any number of attribute groups to the device. If they > don't have a group name, they will all be created directly at the device > level. Would that work for you? I've had a look at sysfs groups and the biggest beef I have with those is that they're too low level. In order to use them I first need to create device attributes, then create an array of pointers to each attr member. It would be nice if I could just feed an array of device attributes (i.e. I want wrappers). I did a little helper struct for the firewire subsystem that might be useful on a more general level. It's struct fw_attribute_group in drivers/firewire/fw-device.h and the implementation is init_fw_attribute_group in drivers/firewire/fw-device.c. But I agree, attribute groups require a fair bit of boiler plate code. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Race free attributes in sysfs
On 5/22/07, Pierre Ossman [EMAIL PROTECTED] wrote: Kay Sievers wrote: We could change the driver-core to suppress the creation of an attribute if the attribute's show() or store() method returns something like -ENOENT at registration time? The driver would pass _all_ possible attributes of the device at registration time, but the core would only create the attributes which are implemented for this particular device? Would that work for you? Not sure. Not in an obvious way at least. It also doesn't feel like the kernel way. Generally you can create/allocate an object, assign attributes to it, then activate it. Couldn't it be done so that I can add sysfs stuff to a device after I just initialized it? (but before I add it). I agree here - if it was just possible to do this between create and add, this discussion would be moot. You can assign any number of attribute groups to the device. If they don't have a group name, they will all be created directly at the device level. Would that work for you? I've had a look at sysfs groups and the biggest beef I have with those is that they're too low level. In order to use them I first need to create device attributes, then create an array of pointers to each attr member. It would be nice if I could just feed an array of device attributes (i.e. I want wrappers). I did a little helper struct for the firewire subsystem that might be useful on a more general level. It's struct fw_attribute_group in drivers/firewire/fw-device.h and the implementation is init_fw_attribute_group in drivers/firewire/fw-device.c. But I agree, attribute groups require a fair bit of boiler plate code. cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/6] firewire: OHCI-1394 lowlevel driver
Christoph Hellwig wrote:
+ if (pci_enable_device(dev)) {
+ fw_error("Failed to enable OHCI hardware.\n");
+ return cleanup(ohci, CLEANUP_PUT_CARD, -ENODEV);
Please use normal goto unwinding so the driver follows the same model
as almost all other pci drivers and allows people to follow your driver
more easily. Also it's not a alot of cleanup code, so removing this
might actually be a net decrease in lines of code.
Sure. The patch I have here says 42 insertions, 44 deletions :)
+ if (software_reset(ohci)) {
Please give all your function a nice prefix so that oops messages are
more readable.
I don't see a strong precedence for this for static functions. For example,
if you grep for 'static' in drivers/usb/core, there's a mix of usb_* prefixed
functions and functions without a consistent prefix. Plus, when the drivers
are loaded as modules, the module name will be in the stack trace. I can
track down the most generic sounding functions and give them a fw_ prefix, but
doing a whole-sale prefixing of static functions will make the source more
noisy and reduce readabilty - I'm not sure it's worth it.
cheers,
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] firewire: SBP-2 highlevel driver
Stefan Richter wrote: Kristian Høgsberg wrote: I was trying to be clever and only allocate the host once the device had been discovered and initialized. I have now changed the code to just allocate the host up front and use the hostdata mechanism for the sbp2_device struct, which also addresses the host life cycle comments below. I have doubts. IMO the previous code is 100% correct as long as 1 SBP-2 target LU maps to 1 Scsi_Host. The lifetime of the Scsi_Host would only be longer than that of the LU if all LUs (or all LUs at the same initiator port) would be added beneath the same instance of Scsi_Host. Then the lifetime of the Scsi_Host would be that of the fw-sbp2 driver, or that of fw-sbp2's representation of a FireWire bus. In the patch, the sbp2_device is now allocated with the scsi_host and is the hostdata part of the host struct. This mean we have to add all the LUs from the unit directory corresponding to the sbp2_device struct to that host. Is that a problem? I think we had this discussion before, but I still don't understand why this approach isn't feasible. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/6] firewire: SBP-2 highlevel driver
Christoph Hellwig wrote:
+ sg = (struct scatterlist *)orb->cmd->request_buffer;
+ count = dma_map_sg(device->card->device, sg, orb->cmd->use_sg,
+ orb->cmd->sc_data_direction);
you need to handle the error case (count == 0)
Yup, done.
+ /* Convert the scatterlist to an sbp2 page table. If any
+* scatterlist entries are too big for sbp2 we split the as we go. */
Please set the max_sectors value in your host template so that the
block layer doesn't build sg entries too big for you.
As Stefan, said, dma_map_sg() breaks the limit guarantee, so we have to split
things manually if sg entries got merged. I've added a comment explaining this.
Isn't max_sectors the overall size limit of the request, though? The SBP-2
protocol imposes a maximum size of 65535 bytes per sg entry, but the total
size of a request can be larger. I guess, setting dma_boundary to 2^15 could
work.
+ orb->page_table_bus =
+ dma_map_single(device->card->device, orb->page_table,
+ size, DMA_TO_DEVICE);
This needs handling of mapping errors (dma_mapping_error())
Done.
+ orb = kzalloc(sizeof *orb, GFP_ATOMIC);
Normal kernel style is sizeof(*orb)
Oh, hmm... I though the kernel style typically was to avoid excess parens :)
But, sure, I see the comment about preferring parens with sizeof in CodingStyle.
+ if (cmd->use_sg) {
+ sbp2_command_orb_map_scatterlist(orb);
+ } else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) {
+ /* FIXME: Need to split this into a sg list... but
+* could we get the scsi or blk layer to do that by
+* reporting our max supported block size? */
+ fw_error("command > 64k\n");
+ goto fail_bufflen;
+ } else if (cmd->request_bufflen > 0) {
+ sbp2_command_orb_map_buffer(orb);
+ }
The use_sg == 0, request_bufflen != 0 case can't happen anymore.
That should simplify the code a bit. How long has that been the case?
+ fail_mapping:
+ kfree(orb);
+ fail_alloc:
+ cmd->result = DID_ERROR << 16;
+ done(cmd);
Failure due to ressource shortage should not complete the command
but return SCSI_MLQUEUE_HOST_BUSY/SCSI_MLQUEUE_DEVICE_BUSY.
Ok, I've changed that.
+ return 0;
+}
+static struct scsi_host_template scsi_driver_template = {
+ .module = THIS_MODULE,
+ .name = "SBP-2 IEEE-1394",
+ .proc_name = (char *)sbp2_driver_name,
Please don't use casrs here. Either fix up the definition so it
accepts const strings or pass a non-const one.
Ok, I'll patch the scsi host template definition.
+static int add_scsi_devices(struct fw_unit *unit)
+{
+ struct sbp2_device *sd = unit->device.driver_data;
+ int retval, lun;
+
+ if (sd->scsi_host != NULL)
+ return 0;
+
+ sd->scsi_host = scsi_host_alloc(_driver_template,
+ sizeof(unsigned long));
+ if (sd->scsi_host == NULL) {
+ fw_error("failed to register scsi host\n");
+ return -1;
+ }
+
+ sd->scsi_host->hostdata[0] = (unsigned long)unit;
Please take a look ar ther other scsi drivers how this is supposed
to be used.
I was trying to be clever and only allocate the host once the device had been
discovered and initialized. I have now changed the code to just allocate the
host up front and use the hostdata mechanism for the sbp2_device struct, which
also addresses the host life cycle comments below.
+ retval = scsi_add_host(sd->scsi_host, >device);
+ if (retval < 0) {
+ fw_error("failed to add scsi host\n");
+ scsi_host_put(sd->scsi_host);
+ sd->scsi_host = NULL;
+ return retval;
+ }
+
+ /* FIXME: Loop over luns here. */
+ lun = 0;
+ retval = scsi_add_device(sd->scsi_host, 0, 0, lun);
+ if (retval < 0) {
+ fw_error("failed to add scsi device\n");
+ scsi_remove_host(sd->scsi_host);
+ scsi_host_put(sd->scsi_host);
+ sd->scsi_host = NULL;
+ return retval;
+ }
+
+ return 0;
+}
Do we really need another scanning algorithm? Can't you use
scsi_scan_target instead and let the core scsi code handle the
scanning?
Stefan addressed this one.
+
+static void remove_scsi_devices(struct fw_unit *unit)
+{
+ struct sbp2_device *sd = unit->device.driver_data;
+
+ if (sd->scsi_host != NULL) {
+ scsi_remove_host(sd->scsi_host);
+ scsi_host_put(sd->scsi_host);
+ }
+ sd->scsi_host = NULL;
+}
This function seems rather oddly named. And the checking and
setting of scsi_host looks like you have some lifetime rule
problems.
Now fixed as described above.
Thanks for the review, will
Re: [PATCH 5/6] firewire: SBP-2 highlevel driver
Christoph Hellwig wrote:
+ sg = (struct scatterlist *)orb-cmd-request_buffer;
+ count = dma_map_sg(device-card-device, sg, orb-cmd-use_sg,
+ orb-cmd-sc_data_direction);
you need to handle the error case (count == 0)
Yup, done.
+ /* Convert the scatterlist to an sbp2 page table. If any
+* scatterlist entries are too big for sbp2 we split the as we go. */
Please set the max_sectors value in your host template so that the
block layer doesn't build sg entries too big for you.
As Stefan, said, dma_map_sg() breaks the limit guarantee, so we have to split
things manually if sg entries got merged. I've added a comment explaining this.
Isn't max_sectors the overall size limit of the request, though? The SBP-2
protocol imposes a maximum size of 65535 bytes per sg entry, but the total
size of a request can be larger. I guess, setting dma_boundary to 2^15 could
work.
+ orb-page_table_bus =
+ dma_map_single(device-card-device, orb-page_table,
+ size, DMA_TO_DEVICE);
This needs handling of mapping errors (dma_mapping_error())
Done.
+ orb = kzalloc(sizeof *orb, GFP_ATOMIC);
Normal kernel style is sizeof(*orb)
Oh, hmm... I though the kernel style typically was to avoid excess parens :)
But, sure, I see the comment about preferring parens with sizeof in CodingStyle.
+ if (cmd-use_sg) {
+ sbp2_command_orb_map_scatterlist(orb);
+ } else if (cmd-request_bufflen SBP2_MAX_SG_ELEMENT_LENGTH) {
+ /* FIXME: Need to split this into a sg list... but
+* could we get the scsi or blk layer to do that by
+* reporting our max supported block size? */
+ fw_error(command 64k\n);
+ goto fail_bufflen;
+ } else if (cmd-request_bufflen 0) {
+ sbp2_command_orb_map_buffer(orb);
+ }
The use_sg == 0, request_bufflen != 0 case can't happen anymore.
That should simplify the code a bit. How long has that been the case?
+ fail_mapping:
+ kfree(orb);
+ fail_alloc:
+ cmd-result = DID_ERROR 16;
+ done(cmd);
Failure due to ressource shortage should not complete the command
but return SCSI_MLQUEUE_HOST_BUSY/SCSI_MLQUEUE_DEVICE_BUSY.
Ok, I've changed that.
+ return 0;
+}
+static struct scsi_host_template scsi_driver_template = {
+ .module = THIS_MODULE,
+ .name = SBP-2 IEEE-1394,
+ .proc_name = (char *)sbp2_driver_name,
Please don't use casrs here. Either fix up the definition so it
accepts const strings or pass a non-const one.
Ok, I'll patch the scsi host template definition.
+static int add_scsi_devices(struct fw_unit *unit)
+{
+ struct sbp2_device *sd = unit-device.driver_data;
+ int retval, lun;
+
+ if (sd-scsi_host != NULL)
+ return 0;
+
+ sd-scsi_host = scsi_host_alloc(scsi_driver_template,
+ sizeof(unsigned long));
+ if (sd-scsi_host == NULL) {
+ fw_error(failed to register scsi host\n);
+ return -1;
+ }
+
+ sd-scsi_host-hostdata[0] = (unsigned long)unit;
Please take a look ar ther other scsi drivers how this is supposed
to be used.
I was trying to be clever and only allocate the host once the device had been
discovered and initialized. I have now changed the code to just allocate the
host up front and use the hostdata mechanism for the sbp2_device struct, which
also addresses the host life cycle comments below.
+ retval = scsi_add_host(sd-scsi_host, unit-device);
+ if (retval 0) {
+ fw_error(failed to add scsi host\n);
+ scsi_host_put(sd-scsi_host);
+ sd-scsi_host = NULL;
+ return retval;
+ }
+
+ /* FIXME: Loop over luns here. */
+ lun = 0;
+ retval = scsi_add_device(sd-scsi_host, 0, 0, lun);
+ if (retval 0) {
+ fw_error(failed to add scsi device\n);
+ scsi_remove_host(sd-scsi_host);
+ scsi_host_put(sd-scsi_host);
+ sd-scsi_host = NULL;
+ return retval;
+ }
+
+ return 0;
+}
Do we really need another scanning algorithm? Can't you use
scsi_scan_target instead and let the core scsi code handle the
scanning?
Stefan addressed this one.
+
+static void remove_scsi_devices(struct fw_unit *unit)
+{
+ struct sbp2_device *sd = unit-device.driver_data;
+
+ if (sd-scsi_host != NULL) {
+ scsi_remove_host(sd-scsi_host);
+ scsi_host_put(sd-scsi_host);
+ }
+ sd-scsi_host = NULL;
+}
This function seems rather oddly named. And the checking and
setting of scsi_host looks like you have some lifetime rule
problems.
Now fixed as described above.
Thanks for the review, will send out new patches.
Kristian
-
To
Re: [PATCH 5/6] firewire: SBP-2 highlevel driver
Stefan Richter wrote: Kristian Høgsberg wrote: I was trying to be clever and only allocate the host once the device had been discovered and initialized. I have now changed the code to just allocate the host up front and use the hostdata mechanism for the sbp2_device struct, which also addresses the host life cycle comments below. I have doubts. IMO the previous code is 100% correct as long as 1 SBP-2 target LU maps to 1 Scsi_Host. The lifetime of the Scsi_Host would only be longer than that of the LU if all LUs (or all LUs at the same initiator port) would be added beneath the same instance of Scsi_Host. Then the lifetime of the Scsi_Host would be that of the fw-sbp2 driver, or that of fw-sbp2's representation of a FireWire bus. In the patch, the sbp2_device is now allocated with the scsi_host and is the hostdata part of the host struct. This mean we have to add all the LUs from the unit directory corresponding to the sbp2_device struct to that host. Is that a problem? I think we had this discussion before, but I still don't understand why this approach isn't feasible. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/6] firewire: OHCI-1394 lowlevel driver
Christoph Hellwig wrote:
+ if (pci_enable_device(dev)) {
+ fw_error(Failed to enable OHCI hardware.\n);
+ return cleanup(ohci, CLEANUP_PUT_CARD, -ENODEV);
Please use normal goto unwinding so the driver follows the same model
as almost all other pci drivers and allows people to follow your driver
more easily. Also it's not a alot of cleanup code, so removing this
might actually be a net decrease in lines of code.
Sure. The patch I have here says 42 insertions, 44 deletions :)
+ if (software_reset(ohci)) {
Please give all your function a nice prefix so that oops messages are
more readable.
I don't see a strong precedence for this for static functions. For example,
if you grep for 'static' in drivers/usb/core, there's a mix of usb_* prefixed
functions and functions without a consistent prefix. Plus, when the drivers
are loaded as modules, the module name will be in the stack trace. I can
track down the most generic sounding functions and give them a fw_ prefix, but
doing a whole-sale prefixing of static functions will make the source more
noisy and reduce readabilty - I'm not sure it's worth it.
cheers,
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Olaf Hering wrote: On Thu, May 03, Olaf Hering wrote: On Thu, May 03, Stefan Richter wrote: ieee1394-old Noone will seriously ship two firewire stacks, so that cant be the issue (for distributors). Once there is a way to easily switch between kernel releases, I'm ok with whatever module names you pick. This patch loads fw-sbp2 if sbp2 is still in the config file. So one can go back and forth between releases without worry about the root filesystem drivers. That's a good solution, that should work. I've committed it locally, will push out changes soon. thanks, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] firewire: char device interface
Christoph Hellwig wrote: On Wed, May 02, 2007 at 05:11:45PM -0400, Kristian H??gsberg wrote: The firewire-cdev.h file is meant to be a self-contained userspace header file and shouldn't include other kernel header files. All duplicated values are standardized ieee1394 values and won't ever change. I should put a #ifndef __FW_COMMON_DEFINES protection around the duplicate values, I guess, but I'm just wondering why I never saw a "symbol redefined" warning... No, defining things in two places is not okay. Just add a new header that defines these protocol constants, which needs to be included by userspace that wants to use them. Ok, I split out the shared constants into linux/firewire-constants.h which gets included by linux/firewire-cdev.h. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] firewire: add it all to kbuild
Christoph Hellwig wrote: On Thu, May 03, 2007 at 01:01:08AM +0200, Stefan Richter wrote: Christoph Hellwig wrote: +fw-core-objs := fw-card.o fw-topology.o fw-transaction.o fw-iso.o \ + fw-device.o fw-cdev.o fw-core-y += .. Like such? Exactly. OK, changed that here, will push out new patches soon. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] firewire: char device interface
Christoph Hellwig wrote:
On Wed, May 02, 2007 at 02:17:26PM +0200, Stefan Richter wrote:
+#include
+#include
Always use the
Fixed.
+struct fw_cdev_get_info {
+ /* The version field is just a running serial number. We
+* never break backwards compatibility. Userspace passes in
+* the version it expects and the kernel passes back the
+* highest version it can provide. Even if the structs in
+* this interface are extended in a later version, the kernel
+* will not copy back more data than what was present in the
+* interface version userspace expects. */
+ __u32 version;
Please don't even try to build interfaces this complicated. If your
current interface needs changes at some point just introduce new ioctls.
Sure, I don't expect a lot of changes anyway, and the few features I know that
will show up down the road are strictly additions anyway. I think it makes
sense to keep the version number there, though, as a way to advertise which
ioctls are present.
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] firewire: handling of cards, buses, nodes
Christoph Hellwig wrote:
On Wed, May 02, 2007 at 02:15:42PM +0200, Stefan Richter wrote:
+/* -*- c-basic-offset: 8 -*-
Please don't pollute the code with annotation for some editors.
OK.
+ * fw-card.c - card level functions
Please don't put the filename into a comment inside the filename.
It's not actually useful and it gets out of date very easily.
Yeah, true.
+static DECLARE_RWSEM(card_rwsem);
this one is only ever used with down_read/up_read so a normal
mutex would probably better suited.
down_write/up_write, but yes, I'll change that to a mutex. The card_rwsem was
previously subsystem.rwsem, but that went away.
+static LIST_HEAD(card_list);
+#define bib_pmc((1) << 27)
+#define bib_bmc((1) << 28)
+#define bib_isc((1) << 29)
+#define bib_cmc((1) << 30)
+#define bib_imc((1) << 31)
These are rather odd names for constants. They should at least
start with an uppercase letter if not be all uppercase. Also
an enum might be right here.
Sure, I'll do that... all those uppercase letters hurt my eyes, though.
+static u32 *
+generate_config_rom (struct fw_card *card, size_t *config_rom_length)
Please don't put white spaces after the function identifier.
Ugh, yeah, I don't know why I keep doing that...
+ /* Initialize contents of config rom buffer. On the OHCI
+* controller, block reads to the config rom accesses the host
+* memory, but quadlet read access the hardware bus info block
+* registers. That's just crack, but it means we should make
+* sure the contents of bus info block in host memory mathces
+* the version stored in the OHCI registers. */
Normal style for block comments is:
/*
* Initialize contents of config rom buffer. On the OHCI
* controller, block reads to the config rom accesses the host
* memory, but quadlet read access the hardware bus info block
* registers. That's just crack, but it means we should make
* sure the contents of bus info block in host memory mathces
* the version stored in the OHCI registers.
*/
Oh, ok, I can clean that up.
+struct fw_node {
+ u16 node_id;
+ u8 color;
+ u8 port_count;
+ unsigned link_on : 1;
+ unsigned initiated_reset : 1;
+ unsigned b_path : 1;
+ u8 phy_speed : 3; /* As in the self ID packet. */
+ u8 max_speed : 5; /* Minimum of all phy-speeds and port speeds on
+ * the path from the local node to this node. */
+ u8 max_depth : 4; /* Maximum depth to any leaf node */
+ u8 max_hops : 4; /* Max hops in this sub tree */
I don't think we need to save the few bits here and can just use
u8 instead of bitfields.
These bitfields aren't used on the wire or anywhere outside the driver... what
the problem? Perfomance?
+ /* Pop the child nodes off the stack and push the new node. */
+ __list_del(h->prev, );
Please don't ever use __list_del directly, it's an interna implementation
detail.
Do you have an alternative suggestion here? I need to take a sub-list of the
list, which is exactly what __list_del does. I could loop through the
sublist, but it's pretty pointless since I have both ends of the sublist.
I also notices that close to none of the export functions have kerneldoc
comment blocks. I think we really should have them on something that
is a driver API.
Yeah... I'll sit down and try to document it better.
thanks
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] firewire: handling of cards, buses, nodes
Pekka Enberg wrote: On 5/2/07, Stefan Richter <[EMAIL PROTECTED]> wrote: I looked around a bit with grep -R and a few search terms but didn't find something definite. Is there any other user of a crc16_itu_t or crc_ccitt or whatever which operates on a (CPU byte ordered) u32[] instead of on a (network byte ordered) u8[]? I was referring to this: http://lkml.org/lkml/2006/6/12/137 That works. If you look closely, you can see that the ITU-T table is just crc_itu_t_table[i] = bitrev16(crc_ccitt_table[bitrev8(i)]) but that's a expensive enough operation that I think it deserves its own table. Especially if there's another driver out there that needs this. On 5/2/07, Stefan Richter <[EMAIL PROTECTED]> wrote: The only value in having a shared implementation would be a potentially smaller kernel. Sharing it to ensure correctness is not an issue; fw-topology.c::crc16_itu_t is simply the one in IEEE 1212 table 5. Performance is also not an issue (if better algorithms exist) because the FireWire stack uses it only infrequently on a moderate amount of data. Yeah, it's not a biggie, but we do have a tradition of putting generally useful things into lib/ so that everyone doesn't invent their own. I'll pull in Ivo's patch and add it to the firewire branch. thanks, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] firewire: handling of cards, buses, nodes
Pekka Enberg wrote: On 5/2/07, Stefan Richter [EMAIL PROTECTED] wrote: I looked around a bit with grep -R and a few search terms but didn't find something definite. Is there any other user of a crc16_itu_t or crc_ccitt or whatever which operates on a (CPU byte ordered) u32[] instead of on a (network byte ordered) u8[]? I was referring to this: http://lkml.org/lkml/2006/6/12/137 That works. If you look closely, you can see that the ITU-T table is just crc_itu_t_table[i] = bitrev16(crc_ccitt_table[bitrev8(i)]) but that's a expensive enough operation that I think it deserves its own table. Especially if there's another driver out there that needs this. On 5/2/07, Stefan Richter [EMAIL PROTECTED] wrote: The only value in having a shared implementation would be a potentially smaller kernel. Sharing it to ensure correctness is not an issue; fw-topology.c::crc16_itu_t is simply the one in IEEE 1212 table 5. Performance is also not an issue (if better algorithms exist) because the FireWire stack uses it only infrequently on a moderate amount of data. Yeah, it's not a biggie, but we do have a tradition of putting generally useful things into lib/ so that everyone doesn't invent their own. I'll pull in Ivo's patch and add it to the firewire branch. thanks, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] firewire: handling of cards, buses, nodes
Christoph Hellwig wrote:
On Wed, May 02, 2007 at 02:15:42PM +0200, Stefan Richter wrote:
+/* -*- c-basic-offset: 8 -*-
Please don't pollute the code with annotation for some editors.
OK.
+ * fw-card.c - card level functions
Please don't put the filename into a comment inside the filename.
It's not actually useful and it gets out of date very easily.
Yeah, true.
+static DECLARE_RWSEM(card_rwsem);
this one is only ever used with down_read/up_read so a normal
mutex would probably better suited.
down_write/up_write, but yes, I'll change that to a mutex. The card_rwsem was
previously subsystem.rwsem, but that went away.
+static LIST_HEAD(card_list);
+#define bib_pmc((1) 27)
+#define bib_bmc((1) 28)
+#define bib_isc((1) 29)
+#define bib_cmc((1) 30)
+#define bib_imc((1) 31)
These are rather odd names for constants. They should at least
start with an uppercase letter if not be all uppercase. Also
an enum might be right here.
Sure, I'll do that... all those uppercase letters hurt my eyes, though.
+static u32 *
+generate_config_rom (struct fw_card *card, size_t *config_rom_length)
Please don't put white spaces after the function identifier.
Ugh, yeah, I don't know why I keep doing that...
+ /* Initialize contents of config rom buffer. On the OHCI
+* controller, block reads to the config rom accesses the host
+* memory, but quadlet read access the hardware bus info block
+* registers. That's just crack, but it means we should make
+* sure the contents of bus info block in host memory mathces
+* the version stored in the OHCI registers. */
Normal style for block comments is:
/*
* Initialize contents of config rom buffer. On the OHCI
* controller, block reads to the config rom accesses the host
* memory, but quadlet read access the hardware bus info block
* registers. That's just crack, but it means we should make
* sure the contents of bus info block in host memory mathces
* the version stored in the OHCI registers.
*/
Oh, ok, I can clean that up.
+struct fw_node {
+ u16 node_id;
+ u8 color;
+ u8 port_count;
+ unsigned link_on : 1;
+ unsigned initiated_reset : 1;
+ unsigned b_path : 1;
+ u8 phy_speed : 3; /* As in the self ID packet. */
+ u8 max_speed : 5; /* Minimum of all phy-speeds and port speeds on
+ * the path from the local node to this node. */
+ u8 max_depth : 4; /* Maximum depth to any leaf node */
+ u8 max_hops : 4; /* Max hops in this sub tree */
I don't think we need to save the few bits here and can just use
u8 instead of bitfields.
These bitfields aren't used on the wire or anywhere outside the driver... what
the problem? Perfomance?
+ /* Pop the child nodes off the stack and push the new node. */
+ __list_del(h-prev, stack);
Please don't ever use __list_del directly, it's an interna implementation
detail.
Do you have an alternative suggestion here? I need to take a sub-list of the
list, which is exactly what __list_del does. I could loop through the
sublist, but it's pretty pointless since I have both ends of the sublist.
I also notices that close to none of the export functions have kerneldoc
comment blocks. I think we really should have them on something that
is a driver API.
Yeah... I'll sit down and try to document it better.
thanks
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] firewire: char device interface
Christoph Hellwig wrote:
On Wed, May 02, 2007 at 02:17:26PM +0200, Stefan Richter wrote:
+#include asm/ioctl.h
+#include asm/types.h
Always use the linux/ versions.
Fixed.
+struct fw_cdev_get_info {
+ /* The version field is just a running serial number. We
+* never break backwards compatibility. Userspace passes in
+* the version it expects and the kernel passes back the
+* highest version it can provide. Even if the structs in
+* this interface are extended in a later version, the kernel
+* will not copy back more data than what was present in the
+* interface version userspace expects. */
+ __u32 version;
Please don't even try to build interfaces this complicated. If your
current interface needs changes at some point just introduce new ioctls.
Sure, I don't expect a lot of changes anyway, and the few features I know that
will show up down the road are strictly additions anyway. I think it makes
sense to keep the version number there, though, as a way to advertise which
ioctls are present.
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6] firewire: add it all to kbuild
Christoph Hellwig wrote: On Thu, May 03, 2007 at 01:01:08AM +0200, Stefan Richter wrote: Christoph Hellwig wrote: +fw-core-objs := fw-card.o fw-topology.o fw-transaction.o fw-iso.o \ + fw-device.o fw-cdev.o fw-core-y += .. Like such? Exactly. OK, changed that here, will push out new patches soon. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] firewire: char device interface
Christoph Hellwig wrote: On Wed, May 02, 2007 at 05:11:45PM -0400, Kristian H??gsberg wrote: The firewire-cdev.h file is meant to be a self-contained userspace header file and shouldn't include other kernel header files. All duplicated values are standardized ieee1394 values and won't ever change. I should put a #ifndef __FW_COMMON_DEFINES protection around the duplicate values, I guess, but I'm just wondering why I never saw a symbol redefined warning... No, defining things in two places is not okay. Just add a new header that defines these protocol constants, which needs to be included by userspace that wants to use them. Ok, I split out the shared constants into linux/firewire-constants.h which gets included by linux/firewire-cdev.h. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Olaf Hering wrote: On Thu, May 03, Olaf Hering wrote: On Thu, May 03, Stefan Richter wrote: ieee1394-old Noone will seriously ship two firewire stacks, so that cant be the issue (for distributors). Once there is a way to easily switch between kernel releases, I'm ok with whatever module names you pick. This patch loads fw-sbp2 if sbp2 is still in the config file. So one can go back and forth between releases without worry about the root filesystem drivers. That's a good solution, that should work. I've committed it locally, will push out changes soon. thanks, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Adrian Bunk wrote: | An advantage of changing the names is that they are now prefixed. Is the opportunity to clean up module names compelling enough, vs. (the wish for) minimized trouble with scripts which refer to module names? ... How big is the trouble actually? Exactly. In Fedora we've just added a fw-sbp2 case to mkinitrd, it's only a couple of lines of extra shell code: elif [ "$modName" = "fw-sbp2" ]; then findmodule fw-core findmodule fw-ohci modName="fw-sbp2" and that's the extent of the changes. The sbp2 case for the old drivers is still in there and in the end mkinitrd works with either stack. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Adrian Bunk wrote: | An advantage of changing the names is that they are now prefixed. Is the opportunity to clean up module names compelling enough, vs. (the wish for) minimized trouble with scripts which refer to module names? ... How big is the trouble actually? Exactly. In Fedora we've just added a fw-sbp2 case to mkinitrd, it's only a couple of lines of extra shell code: elif [ $modName = fw-sbp2 ]; then findmodule fw-core findmodule fw-ohci modName=fw-sbp2 and that's the extent of the changes. The sbp2 case for the old drivers is still in there and in the end mkinitrd works with either stack. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/6] firewire: isochronous and asynchronous I/O
Christoph Hellwig wrote:
+ for (i = 0; i < buffer->page_count; i++) {
+ buffer->pages[i] = alloc_page(GFP_KERNEL | GFP_DMA32 |
__GFP_ZERO);
+ if (buffer->pages[i] == NULL)
+ goto out_pages;
+
+ address = dma_map_page(card->device, buffer->pages[i],
+ 0, PAGE_SIZE, direction);
+ if (dma_mapping_error(address)) {
+ __free_page(buffer->pages[i]);
+ goto out_pages;
+ }
Are you sure using streaming dma mapping is safe here? I don't see
actual user in this patch, but doing the proper ownership protocol
for them is quite difficult if you reuse them, and allocating them
in kernelspace usually means you want to keep reusing them.
What other options are there? The only user in the stack is the userspace
interface, which lets you mmap the pages in the iso_buffer from an
application. The pages need to stay around and stay mapped as long as that
buffer is mmapped by userspace. The buffer can be several megabytes and I
don't want to set up a kernel side virtual mapping for it. The pages are only
used for either outgoing or incoming data, never both, so device/driver
ownership isn't too difficult to handle.
+#include
You don't actually seem to use this one ..
+#include
.. or this one ..
+#include
.. or this one.
Ah, right, I'll get rid of those.
+ retval = fw_core_add_address_handler(_map,
+_map_region);
+ BUG_ON(retval < 0);
+
+ retval = fw_core_add_address_handler(,
+_region);
+ BUG_ON(retval < 0);
+
+ /* Add the vendor textual descriptor. */
+ retval = fw_core_add_descriptor(_id_descriptor);
+ BUG_ON(retval < 0);
+ retval = fw_core_add_descriptor(_id_descriptor);
+ BUG_ON(retval < 0);
These kinds of bug checks look wrong. Either the operations
can't fail in which case they should not return an error value
or you should handle them properly.
The fw_core_add_descriptor() checks that the descriptor block it's passed is
internally consistent and is used for blocks passed in from userspace too. In
these two cases, the blocks are static const arrays in the driver and if
fw_core_add_descriptor returns < 0 it's a bug in the driver.
Both the previous and this patch contain quite a lot of GFP_ATOMIC
allocation which are a sign of not having a very good layering.
Looking through the GFP_ATOMIC allocations, I see a couple that could be
rolled back to GFP_KERNEL. But I don't know that it means bad layering, I'm
just typically using a GFP_ATOMIC kmalloc than, preallocating some fixed
number of, say, packets or nodes or whatever. For example, the old SBP-2
(storage) driver uses a free-list of packets and grabs one from that list when
the SCSI stacks asks it to send a request. If that list is empty, it fails
and lets the SCSI stack retry the command. I'm using a GFP_ATOMIC kmalloc
instead in that case, and I believe it's a better approach than implementing
ad-hoc allocation data structures.
Thanks for the reviews, I'll look through your other emails.
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] firewire: handling of cards, buses, nodes
Pekka Enberg wrote:
On 5/2/07, Stefan Richter <[EMAIL PROTECTED]> wrote:
+/* The lib/crc16.c implementation uses the standard (0x8005)
+ * polynomial, but we need the ITU-T (or CCITT) polynomial (0x1021).
+ * The implementation below works on an array of host-endian u32
+ * words, assuming they'll be transmited msb first. */
+u16
+crc16_itu_t(const u32 *buffer, size_t length)
+{
[snip]
So put it in lib/crc-itu-t.c? Btw, there's apparently another ITU-T
implementation floating around by Ivo van Doorn.
I looked into this a while back, and the kernel does have a CCITT based crc16
function. The problem is that it works on the opposite bit order from what I
need, so to use it I'll have to bitswap every byte as it goes in and then
bitswap the resulting crc16. That's more code and probably less efficient
than just implementing the crc itself. I wouldn't mind moving it to lib/
though, but nobody else in the kernel is using this bit order.
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] firewire: char device interface
John Stoffel wrote: "Stefan" == Stefan Richter <[EMAIL PROTECTED]> writes: Stefan> Signed-off-by: Stefan Richter <[EMAIL PROTECTED]> Stefan> --- Stefan> drivers/firewire/fw-cdev.c| 954 ++ Stefan> include/linux/firewire-cdev.h | 268 + Stefan> 2 files changed, 1222 insertions(+) Stefan> Index: linux_juju/include/linux/firewire-cdev.h ... Stefan> +#define RCODE_SEND_ERROR0x10 Stefan> +#define RCODE_CANCELLED 0x11 Stefan> +#define RCODE_BUSY 0x12 Stefan> +#define RCODE_GENERATION0x13 Stefan> +#define RCODE_NO_ACK0x14 Stefan> + Stefan> +#define SCODE_100 0x0 Stefan> +#define SCODE_200 0x1 Stefan> +#define SCODE_400 0x2 Stefan> +#define SCODE_800 0x3 Stefan> +#define SCODE_1600 0x4 Stefan> +#define SCODE_3200 0x5 These are also defined in fw-transaction.h, though that file doesn't have all the values. Can these just be combined into a single fw-constants.h file instead? I honestly haven't checked all your defines The firewire-cdev.h file is meant to be a self-contained userspace header file and shouldn't include other kernel header files. All duplicated values are standardized ieee1394 values and won't ever change. I should put a #ifndef __FW_COMMON_DEFINES protection around the duplicate values, I guess, but I'm just wondering why I never saw a "symbol redefined" warning... Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Adrian Bunk wrote: On Wed, May 02, 2007 at 02:48:11PM +0200, Stefan Richter wrote: Olaf Hering wrote: On Tue, May 01, Kristian Høgsberg wrote: drivers/firewire/Kconfig | 60 ++ NACK. Upgrade the current drivers/ieee1394/ with the new code, Last time I believe I was the only one who asked whether to put it into drivers/ieee1394 instead of another directory. Of course I acknowledge that everytime a new review round is started, people do reconsider. Especially since we had a gap of a few months since the last LKML review. and keep all existing module names. I'm impartial to that. Using same names might ease the transition from the userspace side, as far as there is userland which relies on module names. A certain drawback of same names would be that geeks cannot install both stacks at once during the transition period. Therefore, checking whether eventual problems are in fact regressions involves a module unload/ configure/ build/ install/ reload cycle, instead of just module unload/ reload. This especially means we can only get help from testers who are able to build kernels. Other opinions? An advantage of changing the names is that they are now prefixed. But looking at them, there will again be the point whether everyone will think that "fw" is firmware, and perhaps switching to the (although longer) prefix "firewire" might make sense? I like "firewire" better, I'm already using that for the userspace header file. Renaming the modules to firewire-core, firewire-ohci and firewire-sbp2 sounds good to me. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Stefan Richter wrote: Christoph Hellwig wrote: .. Please send out patches for review first. Yes, it's been a while since the last submission for review [1], and most of the changes went over linux1394-devel only. And to put it mildly, there aren't a lot of capable reviewers watching that list. ... (If this division seems odd, don't blame Kristian, blame me. :-) I'm looking forward to comments. Looks good to me, thanks Stefan. The first three patches don't compile on their own as they are, but it's a good split of the core stack. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Olaf Hering wrote: On Tue, May 01, Kristian Høgsberg wrote: drivers/firewire/Kconfig | 60 ++ NACK. Upgrade the current drivers/ieee1394/ with the new code, and keep all existing module names. What's your reasoning here? Having different module names allows people to compile both stacks and switch between them as they wish. Another point in favour of different module names is that the new stack doesn't actually provide the same user space interfaces as the old stack. Basically, no applications use the raw kernel interfaces and the new stack is only compatible at the library level. In the light of this, I think it's fair to change the module names. As for putting the new stack in drivers/ieee1394 - I don't know, I think it makes sense to keep the new stack in it's own directory. If it's a deal breaker for inclusion, let's talk about moving it, but it would be nice if you could elaborate just a little bit beyond "NACK". thanks, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Olaf Hering wrote: On Tue, May 01, Kristian Høgsberg wrote: drivers/firewire/Kconfig | 60 ++ NACK. Upgrade the current drivers/ieee1394/ with the new code, and keep all existing module names. What's your reasoning here? Having different module names allows people to compile both stacks and switch between them as they wish. Another point in favour of different module names is that the new stack doesn't actually provide the same user space interfaces as the old stack. Basically, no applications use the raw kernel interfaces and the new stack is only compatible at the library level. In the light of this, I think it's fair to change the module names. As for putting the new stack in drivers/ieee1394 - I don't know, I think it makes sense to keep the new stack in it's own directory. If it's a deal breaker for inclusion, let's talk about moving it, but it would be nice if you could elaborate just a little bit beyond NACK. thanks, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Adrian Bunk wrote: On Wed, May 02, 2007 at 02:48:11PM +0200, Stefan Richter wrote: Olaf Hering wrote: On Tue, May 01, Kristian Høgsberg wrote: drivers/firewire/Kconfig | 60 ++ NACK. Upgrade the current drivers/ieee1394/ with the new code, Last time I believe I was the only one who asked whether to put it into drivers/ieee1394 instead of another directory. Of course I acknowledge that everytime a new review round is started, people do reconsider. Especially since we had a gap of a few months since the last LKML review. and keep all existing module names. I'm impartial to that. Using same names might ease the transition from the userspace side, as far as there is userland which relies on module names. A certain drawback of same names would be that geeks cannot install both stacks at once during the transition period. Therefore, checking whether eventual problems are in fact regressions involves a module unload/ configure/ build/ install/ reload cycle, instead of just module unload/ reload. This especially means we can only get help from testers who are able to build kernels. Other opinions? An advantage of changing the names is that they are now prefixed. But looking at them, there will again be the point whether everyone will think that fw is firmware, and perhaps switching to the (although longer) prefix firewire might make sense? I like firewire better, I'm already using that for the userspace header file. Renaming the modules to firewire-core, firewire-ohci and firewire-sbp2 sounds good to me. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [git pull] New firewire stack
Stefan Richter wrote: Christoph Hellwig wrote: .. Please send out patches for review first. Yes, it's been a while since the last submission for review [1], and most of the changes went over linux1394-devel only. And to put it mildly, there aren't a lot of capable reviewers watching that list. ... (If this division seems odd, don't blame Kristian, blame me. :-) I'm looking forward to comments. Looks good to me, thanks Stefan. The first three patches don't compile on their own as they are, but it's a good split of the core stack. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/6] firewire: char device interface
John Stoffel wrote: Stefan == Stefan Richter [EMAIL PROTECTED] writes: Stefan Signed-off-by: Stefan Richter [EMAIL PROTECTED] Stefan --- Stefan drivers/firewire/fw-cdev.c| 954 ++ Stefan include/linux/firewire-cdev.h | 268 + Stefan 2 files changed, 1222 insertions(+) Stefan Index: linux_juju/include/linux/firewire-cdev.h ... Stefan +#define RCODE_SEND_ERROR0x10 Stefan +#define RCODE_CANCELLED 0x11 Stefan +#define RCODE_BUSY 0x12 Stefan +#define RCODE_GENERATION0x13 Stefan +#define RCODE_NO_ACK0x14 Stefan + Stefan +#define SCODE_100 0x0 Stefan +#define SCODE_200 0x1 Stefan +#define SCODE_400 0x2 Stefan +#define SCODE_800 0x3 Stefan +#define SCODE_1600 0x4 Stefan +#define SCODE_3200 0x5 These are also defined in fw-transaction.h, though that file doesn't have all the values. Can these just be combined into a single fw-constants.h file instead? I honestly haven't checked all your defines The firewire-cdev.h file is meant to be a self-contained userspace header file and shouldn't include other kernel header files. All duplicated values are standardized ieee1394 values and won't ever change. I should put a #ifndef __FW_COMMON_DEFINES protection around the duplicate values, I guess, but I'm just wondering why I never saw a symbol redefined warning... Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/6] firewire: handling of cards, buses, nodes
Pekka Enberg wrote:
On 5/2/07, Stefan Richter [EMAIL PROTECTED] wrote:
+/* The lib/crc16.c implementation uses the standard (0x8005)
+ * polynomial, but we need the ITU-T (or CCITT) polynomial (0x1021).
+ * The implementation below works on an array of host-endian u32
+ * words, assuming they'll be transmited msb first. */
+u16
+crc16_itu_t(const u32 *buffer, size_t length)
+{
[snip]
So put it in lib/crc-itu-t.c? Btw, there's apparently another ITU-T
implementation floating around by Ivo van Doorn.
I looked into this a while back, and the kernel does have a CCITT based crc16
function. The problem is that it works on the opposite bit order from what I
need, so to use it I'll have to bitswap every byte as it goes in and then
bitswap the resulting crc16. That's more code and probably less efficient
than just implementing the crc itself. I wouldn't mind moving it to lib/
though, but nobody else in the kernel is using this bit order.
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/6] firewire: isochronous and asynchronous I/O
Christoph Hellwig wrote:
+ for (i = 0; i buffer-page_count; i++) {
+ buffer-pages[i] = alloc_page(GFP_KERNEL | GFP_DMA32 |
__GFP_ZERO);
+ if (buffer-pages[i] == NULL)
+ goto out_pages;
+
+ address = dma_map_page(card-device, buffer-pages[i],
+ 0, PAGE_SIZE, direction);
+ if (dma_mapping_error(address)) {
+ __free_page(buffer-pages[i]);
+ goto out_pages;
+ }
Are you sure using streaming dma mapping is safe here? I don't see
actual user in this patch, but doing the proper ownership protocol
for them is quite difficult if you reuse them, and allocating them
in kernelspace usually means you want to keep reusing them.
What other options are there? The only user in the stack is the userspace
interface, which lets you mmap the pages in the iso_buffer from an
application. The pages need to stay around and stay mapped as long as that
buffer is mmapped by userspace. The buffer can be several megabytes and I
don't want to set up a kernel side virtual mapping for it. The pages are only
used for either outgoing or incoming data, never both, so device/driver
ownership isn't too difficult to handle.
+#include linux/kthread.h
You don't actually seem to use this one ..
+#include asm/uaccess.h
.. or this one ..
+#include asm/semaphore.h
.. or this one.
Ah, right, I'll get rid of those.
+ retval = fw_core_add_address_handler(topology_map,
+topology_map_region);
+ BUG_ON(retval 0);
+
+ retval = fw_core_add_address_handler(registers,
+registers_region);
+ BUG_ON(retval 0);
+
+ /* Add the vendor textual descriptor. */
+ retval = fw_core_add_descriptor(vendor_id_descriptor);
+ BUG_ON(retval 0);
+ retval = fw_core_add_descriptor(model_id_descriptor);
+ BUG_ON(retval 0);
These kinds of bug checks look wrong. Either the operations
can't fail in which case they should not return an error value
or you should handle them properly.
The fw_core_add_descriptor() checks that the descriptor block it's passed is
internally consistent and is used for blocks passed in from userspace too. In
these two cases, the blocks are static const arrays in the driver and if
fw_core_add_descriptor returns 0 it's a bug in the driver.
Both the previous and this patch contain quite a lot of GFP_ATOMIC
allocation which are a sign of not having a very good layering.
Looking through the GFP_ATOMIC allocations, I see a couple that could be
rolled back to GFP_KERNEL. But I don't know that it means bad layering, I'm
just typically using a GFP_ATOMIC kmalloc than, preallocating some fixed
number of, say, packets or nodes or whatever. For example, the old SBP-2
(storage) driver uses a free-list of packets and grabs one from that list when
the SCSI stacks asks it to send a request. If that list is empty, it fails
and lets the SCSI stack retry the command. I'm using a GFP_ATOMIC kmalloc
instead in that case, and I believe it's a better approach than implementing
ad-hoc allocation data structures.
Thanks for the reviews, I'll look through your other emails.
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[git pull] New firewire stack
Hi Linus, As you may know, we've been working on a new FireWire stack over on linux1394-devel. The main driver behind this work is to get a small, maintainable and supportable FireWire stack, with an acceptable backwards compatibility story. I've been talking to Stefan Richter about it and we feel that the new stack is ready for inclusion into mainline. What I'd like to propose is that we carry both the new and the old stack in mainline for a few releases. Once we've reached a satisfactory level of stability and worked through what regressions there may be, we can consider deprecating the old stack. Carrying two FireWire stacks in the kernel at the same time is not ideal, but it allows for wider testing of the new stack, while keeping the old stack as a fallback for cases where regressions make the new stack not usable. There's a lot of good reasons to switch to the new stack and a lot of reasons to switch away from the old one. Highlights: - Has been in Fedora rawhide (development branch) and -mm for 3 months, will be shipping in Fedora 7. - Backwards compatible at the library level; existing user space libraries have been ported to use the new user space interface. - Less than 8k lines of code compared to 30k lines of code in the old stack, and a similar size reduction in the sizes of the .ko's. - No kernel threads, compared to one subsystem thread and one thread per FireWire controller in the old stack. - One user space interface to support zero-copy scatter-gather streaming, as opposed to the old stacks 4 (was 5) different streaming interfaces. - Per-device device files, letting userspace set up more finegrained access control, such as preventing direct access to FireWire storage devices. Regressions: - eth1394 not ported over. There is nothing preventing this from being done, though, but there's a couple of infrastructure bits that aren't done yet. - No support for the PCILynx chipset. Nobody has this chipset anymore, and the pcilynx driver in the old stack is bit-rotting anyway. - Some SBP-2 (storage) devices fail after significant amounts of IO. Not clear what the problem is, but I can reproduce it here and am working on fixing it. Please pull from the juju branch in Stefans repo: git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git juju thanks, Kristian drivers/Makefile |1 + drivers/firewire/Kconfig | 60 ++ drivers/firewire/Makefile | 10 + drivers/firewire/fw-card.c| 544 +++ drivers/firewire/fw-cdev.c| 954 +++ drivers/firewire/fw-device.c | 781 +++ drivers/firewire/fw-device.h | 149 +++ drivers/firewire/fw-iso.c | 163 drivers/firewire/fw-ohci.c| 1896 + drivers/firewire/fw-ohci.h| 153 +++ drivers/firewire/fw-sbp2.c| 1165 +++ drivers/firewire/fw-topology.c| 519 ++ drivers/firewire/fw-topology.h| 94 ++ drivers/firewire/fw-transaction.c | 889 + drivers/firewire/fw-transaction.h | 505 ++ drivers/ieee1394/Kconfig |2 + include/linux/firewire-cdev.h | 268 ++ 17 files changed, 8153 insertions(+), 0 deletions(-) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[git pull] New firewire stack
Hi Linus, As you may know, we've been working on a new FireWire stack over on linux1394-devel. The main driver behind this work is to get a small, maintainable and supportable FireWire stack, with an acceptable backwards compatibility story. I've been talking to Stefan Richter about it and we feel that the new stack is ready for inclusion into mainline. What I'd like to propose is that we carry both the new and the old stack in mainline for a few releases. Once we've reached a satisfactory level of stability and worked through what regressions there may be, we can consider deprecating the old stack. Carrying two FireWire stacks in the kernel at the same time is not ideal, but it allows for wider testing of the new stack, while keeping the old stack as a fallback for cases where regressions make the new stack not usable. There's a lot of good reasons to switch to the new stack and a lot of reasons to switch away from the old one. Highlights: - Has been in Fedora rawhide (development branch) and -mm for 3 months, will be shipping in Fedora 7. - Backwards compatible at the library level; existing user space libraries have been ported to use the new user space interface. - Less than 8k lines of code compared to 30k lines of code in the old stack, and a similar size reduction in the sizes of the .ko's. - No kernel threads, compared to one subsystem thread and one thread per FireWire controller in the old stack. - One user space interface to support zero-copy scatter-gather streaming, as opposed to the old stacks 4 (was 5) different streaming interfaces. - Per-device device files, letting userspace set up more finegrained access control, such as preventing direct access to FireWire storage devices. Regressions: - eth1394 not ported over. There is nothing preventing this from being done, though, but there's a couple of infrastructure bits that aren't done yet. - No support for the PCILynx chipset. Nobody has this chipset anymore, and the pcilynx driver in the old stack is bit-rotting anyway. - Some SBP-2 (storage) devices fail after significant amounts of IO. Not clear what the problem is, but I can reproduce it here and am working on fixing it. Please pull from the juju branch in Stefans repo: git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git juju thanks, Kristian drivers/Makefile |1 + drivers/firewire/Kconfig | 60 ++ drivers/firewire/Makefile | 10 + drivers/firewire/fw-card.c| 544 +++ drivers/firewire/fw-cdev.c| 954 +++ drivers/firewire/fw-device.c | 781 +++ drivers/firewire/fw-device.h | 149 +++ drivers/firewire/fw-iso.c | 163 drivers/firewire/fw-ohci.c| 1896 + drivers/firewire/fw-ohci.h| 153 +++ drivers/firewire/fw-sbp2.c| 1165 +++ drivers/firewire/fw-topology.c| 519 ++ drivers/firewire/fw-topology.h| 94 ++ drivers/firewire/fw-transaction.c | 889 + drivers/firewire/fw-transaction.h | 505 ++ drivers/ieee1394/Kconfig |2 + include/linux/firewire-cdev.h | 268 ++ 17 files changed, 8153 insertions(+), 0 deletions(-) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ieee1394: remove usage of skb_queue as packet queue
On 3/17/07, Stefan Richter <[EMAIL PROTECTED]> wrote: This considerably reduces the memory requirements for a packet and eliminates ieee1394's dependency on CONFIG_NET. Nice work Stefan, the skb rewrite was one of the most pointless rewrites in the history of the linux1394 stack. TODO: - Double-check if there are any drivers whose packet complete routine really needs process context. If there are none, get rid of khpsbpkt and execute the complete routine in the low-level driver's bottom half's context. Yup, mandatory running in process context is braindead, since most complete routines just need to complete a completion or schedule some work. - Check whether the complete packet really has to be zeroed when allocated. - Allocate small frequently used packets (e.g. quadlet read requests and 4...8 bytes write requests) from a kmem_cache. Append separately allocated data sections only if necessary. Not sure if this is worth it, kmalloc is pretty fast these days. Hehe, you're reverting the bad decisions that made me tune out from linux1394 development a few years back. But the question is, is it worth it? One of the primary reasons for me to write an alternative stack was to be able to leave linux1394 in maintenence mode. This way I wont screw up existing functionality in the old stack, and will be able to make big changes without worrying about porting over every single driver. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ieee1394: remove usage of skb_queue as packet queue
On 3/17/07, Stefan Richter [EMAIL PROTECTED] wrote: This considerably reduces the memory requirements for a packet and eliminates ieee1394's dependency on CONFIG_NET. Nice work Stefan, the skb rewrite was one of the most pointless rewrites in the history of the linux1394 stack. TODO: - Double-check if there are any drivers whose packet complete routine really needs process context. If there are none, get rid of khpsbpkt and execute the complete routine in the low-level driver's bottom half's context. Yup, mandatory running in process context is braindead, since most complete routines just need to complete a completion or schedule some work. - Check whether the complete packet really has to be zeroed when allocated. - Allocate small frequently used packets (e.g. quadlet read requests and 4...8 bytes write requests) from a kmem_cache. Append separately allocated data sections only if necessary. Not sure if this is worth it, kmalloc is pretty fast these days. Hehe, you're reverting the bad decisions that made me tune out from linux1394 development a few years back. But the question is, is it worth it? One of the primary reasons for me to write an alternative stack was to be able to leave linux1394 in maintenence mode. This way I wont screw up existing functionality in the old stack, and will be able to make big changes without worrying about porting over every single driver. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Juju
Pete Zaitcev wrote: On Thu, 25 Jan 2007 16:18:35 -0500, Kristian Høgsberg <[EMAIL PROTECTED]> wrote: Indeed, I've just moved to an in-tree development model now. I still think the out-off-tree model is a good way to prototype, get started and reach "critical mass" with your driver. But as I'm starting to integrate and sync with Stefans tree now, an in-tree model is a lot easier to work with. The tree is here git://people.freedesktop.org/~krh/linux-2.6 This seems to have disappeared. Was it moved or dropped? No, it's still there, and I just did a git clone on it. How does it fail for you? In any case, you're probably better of pulling from Stefans kernel.org tree: git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git since that's where new work is going to show up. I'm still trying to figure out a good workflow for this, but if all patches are going to through linux1394-devel, there's not much point in me publishing a branch in the freedesktop.org repo, if the patches are going to be edited/tweaked and the committed to Stefans repo. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Juju
Pete Zaitcev wrote: On Thu, 25 Jan 2007 16:18:35 -0500, Kristian Høgsberg [EMAIL PROTECTED] wrote: Indeed, I've just moved to an in-tree development model now. I still think the out-off-tree model is a good way to prototype, get started and reach critical mass with your driver. But as I'm starting to integrate and sync with Stefans tree now, an in-tree model is a lot easier to work with. The tree is here git://people.freedesktop.org/~krh/linux-2.6 This seems to have disappeared. Was it moved or dropped? No, it's still there, and I just did a git clone on it. How does it fail for you? In any case, you're probably better of pulling from Stefans kernel.org tree: git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394-2.6.git since that's where new work is going to show up. I'm still trying to figure out a good workflow for this, but if all patches are going to through linux1394-devel, there's not much point in me publishing a branch in the freedesktop.org repo, if the patches are going to be edited/tweaked and the committed to Stefans repo. Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Juju
Greg KH wrote: On Thu, Jan 25, 2007 at 03:38:24PM -0800, Pete Zaitcev wrote: On Thu, 25 Jan 2007 16:18:35 -0500, Kristian H??gsberg <[EMAIL PROTECTED]> wrote: I see that ORBs are always allocated with a call (like SKB) and not embedded into drivers (like URBs). It's great, keep it up. Also, never allow drivers to pass DMA-mapped buffers into fw_send_request and friends. We made both of these mistakes in USB, and it hurts. Oh, the ORBs are SBP-2 specific data structures, struct fw_transaction is probably what corresponds to USB URBs. This struct is defined in fw-transaction.h and is available for embedding into other structs, such as struct sbp2_orb in fw-sbp2. Is that what you're suggesting against, and what are the problems with this approach? Fortunately we do not care about out-of-tree drivers, which are most affected, you may even call it a feature ^_^. My main problem is, we can't refcount URBs, so usbmon can't tap them and must copy. urbs are reference counted, it's just that not all drivers who create them use them that way :( Perhaps you can inforce this in the new codebase... It's a small change to make the fw_transaction struct opaque and ref-counted, and it's definitely still doable. But the nice thing about embedding the struct is that you have one memory allocation failure path less to worry about. And I haven't yet, and don't expect to see a use case that will need ref-counted struct fw_transaction, the ownership is always clearly defined. But I can go either way on this and if there is a good reason to ref count them it's a pretty small change. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Juju
Greg KH wrote: On Thu, Jan 25, 2007 at 03:38:24PM -0800, Pete Zaitcev wrote: On Thu, 25 Jan 2007 16:18:35 -0500, Kristian H??gsberg [EMAIL PROTECTED] wrote: I see that ORBs are always allocated with a call (like SKB) and not embedded into drivers (like URBs). It's great, keep it up. Also, never allow drivers to pass DMA-mapped buffers into fw_send_request and friends. We made both of these mistakes in USB, and it hurts. Oh, the ORBs are SBP-2 specific data structures, struct fw_transaction is probably what corresponds to USB URBs. This struct is defined in fw-transaction.h and is available for embedding into other structs, such as struct sbp2_orb in fw-sbp2. Is that what you're suggesting against, and what are the problems with this approach? Fortunately we do not care about out-of-tree drivers, which are most affected, you may even call it a feature ^_^. My main problem is, we can't refcount URBs, so usbmon can't tap them and must copy. urbs are reference counted, it's just that not all drivers who create them use them that way :( Perhaps you can inforce this in the new codebase... It's a small change to make the fw_transaction struct opaque and ref-counted, and it's definitely still doable. But the nice thing about embedding the struct is that you have one memory allocation failure path less to worry about. And I haven't yet, and don't expect to see a use case that will need ref-counted struct fw_transaction, the ownership is always clearly defined. But I can go either way on this and if there is a good reason to ref count them it's a pretty small change. cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Juju
Stefan Richter wrote:
Pete Zaitcev wrote:
On Thu, 25 Jan 2007 16:18:35 -0500, Kristian Høgsberg <[EMAIL PROTECTED]> wrote:
...
will do a status write to the status address specified in the ORB, at which
point the SBP-2 transaction is complete.
You know, I wanted to use this picture for a long time:
http://www.flickr.com/photos/zaitcev/369269557/
Haha, sure :)
The fundamental thing about SBP-2 is that ORBs ( = SCSI command blocks
plus SBP-2 header) and data buffers all reside in the memory of the
initiator (or of a 3rd party on the FireWire bus). The target peeks and
pokes them when and how it sees fit. The initiator pushes only tiny
notifications about availability of new ORBs to the target. The target
eventually completes SCSI commands in-order or out-of-order and signals
so by pushing a status block per one or more completed commands.
(Juju's fw-sbp2 gives only one command at a time to the target.
Mainline's sbp2 can optionally give more commands in a row, but the
implementation is subtly broken in several ways and therefore disabled
by default until I fix it right after hell froze over.)
Another important thing to know in order to understand fw-sbp2 and sbp2
is that they currently rely on OHCI-1394's physical DMA feature, which
I'll not explain here. It means two things: 1. FireWire bus addresses of
ORBs and buffers are directly derived from the DMA mapped address.
(FireWire bus addresses are the addresses used in communication between
SBP-2 initiator and target.) 2. Almost all of the transfers done by the
target do not generate interrupts. (Just the status write generates an
interrupt.)
Another thing that probably makes my explanation a little confusing is that
there are two types of transactions: FireWire transactions which consists of a
request followed by a response and are pretty much the smallest interaction
you can have with a remote device. Then there are SBP-2 transactions, which
are a higer level sequence layered on top of FireWire transactions. An SBP-2
transaction consists of a sequence of FireWire transactions, the first of
which is initiated by the initiator. This is the FireWire transaction that
complete_transaction handles. When this first FireWire transaction finishes
succesfully, we know that the SBP-2 transaction has been started and we sit
back and wait for the target to do it's part. If that initial FireWire
transaction fails, we need to fail the SBP-2 transaction we we're trying to start.
...
Now that you drew my attention to sbp2_status_write(), this looks wrong:
/* Lookup the orb corresponding to this status write. */
spin_lock_irqsave(>lock, flags);
list_for_each_entry(orb, >orb_list, link) {
if (status_get_orb_high(status) == 0 &&
status_get_orb_low(status) == orb->request_bus) {
list_del(>link);
break;
}
}
spin_unlock_irqrestore(>lock, flags);
Why is it that fw_request can't carry a pointer?
The target wrote an SBP-2 status block into our memory. The status block
contains the FireWire bus address of the ORB to which it belongs. Juju's
fw-sbp2 does the same as mainline's sbp2: Looking through the pile of
unfinished ORBs for one with the same FireWire bus address, which was
previously derived from the DMA mapped address.
But the status write actually does carry the address of the ORB it signals the
completion of. So in theory, we could just read out the ORB address from the
status write packet and map that back to kernel virtual memory and do an
appropriate container_of() call and we should have the struct sbp2_orb
pointer. The reason I still search through the list is of course that this is
way to much trust to put into hardware as buggy as external storage devices.
Blindly dereferencing a pointer returned by storage driver firmware is
probably a very bad idea.
One thing I want to do (though very low priority) is to allocate the ORBs out
of a preallocated circular buffer. We can then check that the ORB pointer
returned in the status write points into this buffer and that it's a multiple
of the ORB size, at which point it should be safe to dereference it.
> Since there aren't many
mapped ORBs per target, a linked list is a reasonable data structure to
search over. That said --- Kristian, doesn't fw-sbp2 have at most 1 ORB
in sd->orb_list?
Yes, there is only ever one pending ORB in the list, so looking through the
list is not exactly a time sink :)
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Juju
Pete Zaitcev wrote:
Hi, Kristian:
I only looked briefly at SBP-2, and at submit/callback paths it pulled,
because I do not understand most of the other issues.
Great, thanks for giving this a look-over, much appreciated.
Executive summary: please implement proper ORB cancellation. This is
how you verify that your locking model is worth anything: by interaction
of normal transactions with ->remove, ->suspend and SCSI aborts.
Currently, there's no telling if the code is sane or not.
Good catch, my ORB cancellation is a bit rough, and I believe there are a
couple of races there, though I haven't been able to actually trigger these.
When cancelling an ORB I need to cancel the ORB, the pending transaction and
potentially the packet queued for DMA in the fw-ohci driver. This is not
fully in place yet and I need to sit down and audit this path.
I see that ORBs are always allocated with a call (like SKB) and not
embedded into drivers (like URBs). It's great, keep it up. Also,
never allow drivers to pass DMA-mapped buffers into fw_send_request
and friends. We made both of these mistakes in USB, and it hurts.
Oh, the ORBs are SBP-2 specific data structures, struct fw_transaction is
probably what corresponds to USB URBs. This struct is defined in
fw-transaction.h and is available for embedding into other structs, such as
struct sbp2_orb in fw-sbp2. Is that what you're suggesting against, and what
are the problems with this approach?
As for passing DMA-mapped buffers, do you mean that fw_send_request (or code
below that function) should map the payload as it currently does as opposed to
callers pre-mapping DMA buffers?
Now, about small things:
diff --git a/fw-sbp2.c b/fw-sbp2.c
index f27..3a65095 100644
--- a/fw-sbp2.c
+++ b/fw-sbp2.c
@@ -303,7 +303,7 @@ complete_transaction(struct fw_card *card, int rcode,
unsigned long flags;
orb->rcode = rcode;
- if (rcode != RCODE_COMPLETE) {
+ if (rcode != RCODE_COMPLETE) { /* Huh? */
spin_lock_irqsave(>lock, flags);
list_del(>link);
spin_unlock_irqrestore(>lock, flags);
This looks like an inverted test. Who does remove ORB from the list
if it's completed normally?
An SBP-2 ORB transaction sequence consists of a number of FireWire
transactions. First the initiator prepares an SBP-2 ORB (object request
block) in host memory and maps this for DMA. Then the initiators sends its
bus address and the physical address of the ORB to the SBP-2 device. This is
the request that complete_transaction deals with. If we get rcode ==
RCODE_COMPLETE here, we successfully wrote the ORB address to the SBP-2 device
and we leave the ORB in the list so the SBP-2 transaction can continue. Any
other rcode means that the write failed and we need to back out the
transaction and leave it to upper layers to retry.
The rest of the SBP-2 transaction continues with the SBP-2 device issuing a
FireWire read transaction to read out the ORB from host memory and then carry
out the instructions in the ORB. Once the device has completed the ORB it
will do a status write to the status address specified in the ORB, at which
point the SBP-2 transaction is complete.
@@ -320,8 +320,7 @@ sbp2_send_orb(struct sbp2_orb *orb, struct fw_unit *unit,
unsigned long flags;
orb->pointer.high = 0;
- orb->pointer.low = orb->request_bus;
- fw_memcpy_to_be32(>pointer, >pointer, sizeof orb->pointer);
+ orb->pointer.low = cpu_to_be32(orb->request_bus);
spin_lock_irqsave(>card->lock, flags);
list_add_tail(>link, >orb_list);
I'll whine later about this
@@ -347,6 +346,7 @@ static void sbp2_cancel_orbs(struct fw_unit *unit)
list_splice_init(>orb_list, );
spin_unlock_irqrestore(>card->lock, flags);
+ /* XXX Notify the hardware -- show-stopper. */
Yes, agreed.
list_for_each_entry_safe(orb, next, , link) {
orb->rcode = RCODE_CANCELLED;
orb->callback(orb, NULL);
@@ -364,6 +364,9 @@ complete_management_orb(struct sbp2_orb *base_orb, struct
sbp2_status *status)
complete(>done);
}
+/*
+ * This function must be called from a process context.
+ */
static int
sbp2_send_management_orb(struct fw_unit *unit, int node_id, int generation,
int function, int lun, void *response)
@@ -374,9 +377,9 @@ sbp2_send_management_orb(struct fw_unit *unit, int node_id,
int generation,
unsigned long timeout;
int retval = -ENOMEM;
- orb = kzalloc(sizeof *orb, GFP_ATOMIC);
+ orb = kzalloc(sizeof *orb, GFP_KERNEL);
if (orb == NULL)
- return -ENOMEM;
+ goto out_orballoc;
/* The sbp2 device is going to send a block read request to
* read out the request from host memory, so map it for
No reason to use the atomic pool, this function sleeps later anyway.
Oh, true.
@@ -384,17 +387,22 @@
Re: Juju
Pete Zaitcev wrote:
Hi, Kristian:
I only looked briefly at SBP-2, and at submit/callback paths it pulled,
because I do not understand most of the other issues.
Great, thanks for giving this a look-over, much appreciated.
Executive summary: please implement proper ORB cancellation. This is
how you verify that your locking model is worth anything: by interaction
of normal transactions with -remove, -suspend and SCSI aborts.
Currently, there's no telling if the code is sane or not.
Good catch, my ORB cancellation is a bit rough, and I believe there are a
couple of races there, though I haven't been able to actually trigger these.
When cancelling an ORB I need to cancel the ORB, the pending transaction and
potentially the packet queued for DMA in the fw-ohci driver. This is not
fully in place yet and I need to sit down and audit this path.
I see that ORBs are always allocated with a call (like SKB) and not
embedded into drivers (like URBs). It's great, keep it up. Also,
never allow drivers to pass DMA-mapped buffers into fw_send_request
and friends. We made both of these mistakes in USB, and it hurts.
Oh, the ORBs are SBP-2 specific data structures, struct fw_transaction is
probably what corresponds to USB URBs. This struct is defined in
fw-transaction.h and is available for embedding into other structs, such as
struct sbp2_orb in fw-sbp2. Is that what you're suggesting against, and what
are the problems with this approach?
As for passing DMA-mapped buffers, do you mean that fw_send_request (or code
below that function) should map the payload as it currently does as opposed to
callers pre-mapping DMA buffers?
Now, about small things:
diff --git a/fw-sbp2.c b/fw-sbp2.c
index f27..3a65095 100644
--- a/fw-sbp2.c
+++ b/fw-sbp2.c
@@ -303,7 +303,7 @@ complete_transaction(struct fw_card *card, int rcode,
unsigned long flags;
orb-rcode = rcode;
- if (rcode != RCODE_COMPLETE) {
+ if (rcode != RCODE_COMPLETE) { /* Huh? */
spin_lock_irqsave(card-lock, flags);
list_del(orb-link);
spin_unlock_irqrestore(card-lock, flags);
This looks like an inverted test. Who does remove ORB from the list
if it's completed normally?
An SBP-2 ORB transaction sequence consists of a number of FireWire
transactions. First the initiator prepares an SBP-2 ORB (object request
block) in host memory and maps this for DMA. Then the initiators sends its
bus address and the physical address of the ORB to the SBP-2 device. This is
the request that complete_transaction deals with. If we get rcode ==
RCODE_COMPLETE here, we successfully wrote the ORB address to the SBP-2 device
and we leave the ORB in the list so the SBP-2 transaction can continue. Any
other rcode means that the write failed and we need to back out the
transaction and leave it to upper layers to retry.
The rest of the SBP-2 transaction continues with the SBP-2 device issuing a
FireWire read transaction to read out the ORB from host memory and then carry
out the instructions in the ORB. Once the device has completed the ORB it
will do a status write to the status address specified in the ORB, at which
point the SBP-2 transaction is complete.
@@ -320,8 +320,7 @@ sbp2_send_orb(struct sbp2_orb *orb, struct fw_unit *unit,
unsigned long flags;
orb-pointer.high = 0;
- orb-pointer.low = orb-request_bus;
- fw_memcpy_to_be32(orb-pointer, orb-pointer, sizeof orb-pointer);
+ orb-pointer.low = cpu_to_be32(orb-request_bus);
spin_lock_irqsave(device-card-lock, flags);
list_add_tail(orb-link, sd-orb_list);
I'll whine later about this
@@ -347,6 +346,7 @@ static void sbp2_cancel_orbs(struct fw_unit *unit)
list_splice_init(sd-orb_list, list);
spin_unlock_irqrestore(device-card-lock, flags);
+ /* XXX Notify the hardware -- show-stopper. */
Yes, agreed.
list_for_each_entry_safe(orb, next, list, link) {
orb-rcode = RCODE_CANCELLED;
orb-callback(orb, NULL);
@@ -364,6 +364,9 @@ complete_management_orb(struct sbp2_orb *base_orb, struct
sbp2_status *status)
complete(orb-done);
}
+/*
+ * This function must be called from a process context.
+ */
static int
sbp2_send_management_orb(struct fw_unit *unit, int node_id, int generation,
int function, int lun, void *response)
@@ -374,9 +377,9 @@ sbp2_send_management_orb(struct fw_unit *unit, int node_id,
int generation,
unsigned long timeout;
int retval = -ENOMEM;
- orb = kzalloc(sizeof *orb, GFP_ATOMIC);
+ orb = kzalloc(sizeof *orb, GFP_KERNEL);
if (orb == NULL)
- return -ENOMEM;
+ goto out_orballoc;
/* The sbp2 device is going to send a block read request to
* read out the request from host memory, so map it for
No reason to use the atomic pool, this function sleeps later anyway.
Oh, true.
@@ -384,17
Re: Juju
Stefan Richter wrote:
Pete Zaitcev wrote:
On Thu, 25 Jan 2007 16:18:35 -0500, Kristian Høgsberg [EMAIL PROTECTED] wrote:
...
will do a status write to the status address specified in the ORB, at which
point the SBP-2 transaction is complete.
You know, I wanted to use this picture for a long time:
http://www.flickr.com/photos/zaitcev/369269557/
Haha, sure :)
The fundamental thing about SBP-2 is that ORBs ( = SCSI command blocks
plus SBP-2 header) and data buffers all reside in the memory of the
initiator (or of a 3rd party on the FireWire bus). The target peeks and
pokes them when and how it sees fit. The initiator pushes only tiny
notifications about availability of new ORBs to the target. The target
eventually completes SCSI commands in-order or out-of-order and signals
so by pushing a status block per one or more completed commands.
(Juju's fw-sbp2 gives only one command at a time to the target.
Mainline's sbp2 can optionally give more commands in a row, but the
implementation is subtly broken in several ways and therefore disabled
by default until I fix it right after hell froze over.)
Another important thing to know in order to understand fw-sbp2 and sbp2
is that they currently rely on OHCI-1394's physical DMA feature, which
I'll not explain here. It means two things: 1. FireWire bus addresses of
ORBs and buffers are directly derived from the DMA mapped address.
(FireWire bus addresses are the addresses used in communication between
SBP-2 initiator and target.) 2. Almost all of the transfers done by the
target do not generate interrupts. (Just the status write generates an
interrupt.)
Another thing that probably makes my explanation a little confusing is that
there are two types of transactions: FireWire transactions which consists of a
request followed by a response and are pretty much the smallest interaction
you can have with a remote device. Then there are SBP-2 transactions, which
are a higer level sequence layered on top of FireWire transactions. An SBP-2
transaction consists of a sequence of FireWire transactions, the first of
which is initiated by the initiator. This is the FireWire transaction that
complete_transaction handles. When this first FireWire transaction finishes
succesfully, we know that the SBP-2 transaction has been started and we sit
back and wait for the target to do it's part. If that initial FireWire
transaction fails, we need to fail the SBP-2 transaction we we're trying to start.
...
Now that you drew my attention to sbp2_status_write(), this looks wrong:
/* Lookup the orb corresponding to this status write. */
spin_lock_irqsave(card-lock, flags);
list_for_each_entry(orb, sd-orb_list, link) {
if (status_get_orb_high(status) == 0
status_get_orb_low(status) == orb-request_bus) {
list_del(orb-link);
break;
}
}
spin_unlock_irqrestore(card-lock, flags);
Why is it that fw_request can't carry a pointer?
The target wrote an SBP-2 status block into our memory. The status block
contains the FireWire bus address of the ORB to which it belongs. Juju's
fw-sbp2 does the same as mainline's sbp2: Looking through the pile of
unfinished ORBs for one with the same FireWire bus address, which was
previously derived from the DMA mapped address.
But the status write actually does carry the address of the ORB it signals the
completion of. So in theory, we could just read out the ORB address from the
status write packet and map that back to kernel virtual memory and do an
appropriate container_of() call and we should have the struct sbp2_orb
pointer. The reason I still search through the list is of course that this is
way to much trust to put into hardware as buggy as external storage devices.
Blindly dereferencing a pointer returned by storage driver firmware is
probably a very bad idea.
One thing I want to do (though very low priority) is to allocate the ORBs out
of a preallocated circular buffer. We can then check that the ORB pointer
returned in the status write points into this buffer and that it's a multiple
of the ORB size, at which point it should be safe to dereference it.
Since there aren't many
mapped ORBs per target, a linked list is a reasonable data structure to
search over. That said --- Kristian, doesn't fw-sbp2 have at most 1 ORB
in sd-orb_list?
Yes, there is only ever one pending ORB in the list, so looking through the
list is not exactly a time sink :)
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
In-tree version of new FireWire drivers available
Hi, I've moved the new FireWire stack to an in-tree git repository and moved over the missing patches from my out-of-tree version. The tree is available over here: git://people.freedesktop.org/~krh/linux-2.6 with gitweb avialable here: http://gitweb.freedesktop.org/?p=users/krh/linux-2.6.git;a=summary There's only one branch there which is branched of off Stefan Richters master from the linux1394 repo, and has the most recent work from my out-of -tree repo. Hosting this on freedesktop.org should be fine, though kernel.org may be more appropriate :) Changes since the merge into the linux1394 tree include: - gap count optimization - full bus management - loopback for async requests to the local node - a bug fix for a problem exposed by VIA 6306 controllers - a typo fix from the bitfield -> mask+shift conversion. Plus I've merged Stefans recent fixes and resolved the few conflicts from that. Stefan, I'm still not sure what the work flow should be here, do you want to just pull these changes or should I send the 13 patches to linux1394-devel? cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
In-tree version of new FireWire drivers available
Hi, I've moved the new FireWire stack to an in-tree git repository and moved over the missing patches from my out-of-tree version. The tree is available over here: git://people.freedesktop.org/~krh/linux-2.6 with gitweb avialable here: http://gitweb.freedesktop.org/?p=users/krh/linux-2.6.git;a=summary There's only one branch there which is branched of off Stefan Richters master from the linux1394 repo, and has the most recent work from my out-of -tree repo. Hosting this on freedesktop.org should be fine, though kernel.org may be more appropriate :) Changes since the merge into the linux1394 tree include: - gap count optimization - full bus management - loopback for async requests to the local node - a bug fix for a problem exposed by VIA 6306 controllers - a typo fix from the bitfield - mask+shift conversion. Plus I've merged Stefans recent fixes and resolved the few conflicts from that. Stefan, I'm still not sure what the work flow should be here, do you want to just pull these changes or should I send the 13 patches to linux1394-devel? cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [-mm patch] drivers/firewire/: cleanups
Adrian Bunk wrote: On Mon, Jan 22, 2007 at 02:41:29PM -0500, Kristian Høgsberg wrote: Adrian Bunk wrote: On Thu, Jan 11, 2007 at 10:26:27PM -0800, Andrew Morton wrote: ... Changes since 2.6.20-rc3-mm1: ... git-ieee1394.patch ... git trees ... This patch contains the following cleanups: - "extern inline" -> "static inline" I guess that's fine, I do like how extern inline will never generate code Exactly the opposite will be true when gcc will switch to the C99 semantics... Besides this, "static inline" forces inlining in the kernel due to #define inline inline __attribute__((always_inline)) In that case I have no reservations at all. thanks, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [-mm patch] drivers/firewire/: cleanups
Adrian Bunk wrote: On Thu, Jan 11, 2007 at 10:26:27PM -0800, Andrew Morton wrote: ... Changes since 2.6.20-rc3-mm1: ... git-ieee1394.patch ... git trees ... This patch contains the following cleanups: - "extern inline" -> "static inline" I guess that's fine, I do like how extern inline will never generate code and that gcc warns on missing prototype for these case is more of a gcc bug. Not a big deal though, it's more worthwhile to track down real missing prototype offenders. - fw-topology.c: make struct fw_node_create static Looks good. Signed-off-by: Kristian Høgsberg <[EMAIL PROTECTED]> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [-mm patch] drivers/firewire/: cleanups
Adrian Bunk wrote: On Thu, Jan 11, 2007 at 10:26:27PM -0800, Andrew Morton wrote: ... Changes since 2.6.20-rc3-mm1: ... git-ieee1394.patch ... git trees ... This patch contains the following cleanups: - extern inline - static inline I guess that's fine, I do like how extern inline will never generate code and that gcc warns on missing prototype for these case is more of a gcc bug. Not a big deal though, it's more worthwhile to track down real missing prototype offenders. - fw-topology.c: make struct fw_node_create static Looks good. Signed-off-by: Kristian Høgsberg [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [-mm patch] drivers/firewire/: cleanups
Adrian Bunk wrote: On Mon, Jan 22, 2007 at 02:41:29PM -0500, Kristian Høgsberg wrote: Adrian Bunk wrote: On Thu, Jan 11, 2007 at 10:26:27PM -0800, Andrew Morton wrote: ... Changes since 2.6.20-rc3-mm1: ... git-ieee1394.patch ... git trees ... This patch contains the following cleanups: - extern inline - static inline I guess that's fine, I do like how extern inline will never generate code Exactly the opposite will be true when gcc will switch to the C99 semantics... Besides this, static inline forces inlining in the kernel due to #define inline inline __attribute__((always_inline)) In that case I have no reservations at all. thanks, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: allocation failed: out of vmalloc space error treating and VIDEO1394 IOC LISTEN CHANNEL ioctl failed problem
On 1/15/07, Arjan van de Ven <[EMAIL PROTECTED]> wrote: > However, what I'd really like to do is to leave it to user space to > allocate the memory as David describes. In the transmit case, user > space allocates memory (malloc or mmap) and loads the payload into > that buffer. there is a lot of pain involved with doing things this way, it is a TON better if YOU provide the memory via a custom mmap handler for a device driver. (there are a lot of security nightmares involved with the opposite model, like the user can put any kind of memory there, even pci mmio space) OK, point taken. I don't have a strong preference for the opposite model, it just seems elegant that you can let user space handle allocation and pin and map the pages as needed. But you're right, it certainly is easier to give safe memory to user space in the first place rather than try to make sure user space isn't trying to trick us. > Then is does an ioctl() on the firewire control device ioctls are evil ;) esp an "mmap me" ioctl Ah, I'm not mmap'ing it from the ioctl, I do implement the mma file operation for this. However, you have to do an ioctl before mapping the device to configure the dma context. Other than that what is the problem with ioctls, and more interesting, what is the alternative? I don't expect (or want) a bunch of syscalls to be added for this, so I don't really see what other mechanism I should use for this. > It's not too difficult from what I'm doing now, I'd just like to give > user space more control over the buffers it uses for streaming (i.e. > letting user space allocate them). What I'm missing here is: how do I > actually pin a page in memory? I'm sure it's not too difficult, but I > haven't yet figured it out and I'm sure somebody knows it off the top > of his head. again the best way is for you to provide an mmap method... you can then fill in the pages and keep that in some sort of array; this is for example also what the DRI/DRM layer does for textures etc... That sounds a lot like what I have now (mmap method, array of pages) so I'll just stick with that. thanks, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: allocation failed: out of vmalloc space error treating and VIDEO1394 IOC LISTEN CHANNEL ioctl failed problem
On 1/15/07, David Moore <[EMAIL PROTECTED]> wrote: On Mon, 2007-01-15 at 10:20 -0800, Arjan van de Ven wrote: > if you need that much you probably should redesign your algorithms to > not need vmalloc in the first place I think you've convinced me that vmalloc is not a good choice when a driver needs a large buffer (many megabytes) for DMA. In this case, we need a large ring buffer for reception of isochronous packets from a firewire device. If I understand you correctly, you are suggesting that this buffer be obtained as followed: 1. Application performs malloc() in user-space and mmap()s it. 2. Driver uses vmalloc_to_page() on every page of the malloc'ed memory and constructs a scatter-gather list. 3. Map the sg list with pci_map_sg(). 4. Commence DMA. Is that correct? In particular, does it do the right thing in terms of pinning the memory and dealing with high memory? I notice that the block I/O API has some convenience functions for this, but this is not a block device. Are there some other convenience functions that can be used? Forgive me if these are obvious questions -- I'm not the developer of video1394, but I'd still like get it right for the new firewire stack that's being developed. David, thanks for bringing this up. Indeed if vmalloc is not the right way for allocating big buffers, we need to figure something else out. My impression was that the vmalloc group of functions could be used for big allocations since they don't require the underlying memory to be physically contiguous. What I'm doing currently in the new firewire stack is to vmalloc the memory to be used for isochronous payload and then use remap_vmalloc_range() to map the memory to the user. I never access the contents from the kernel side, I just use vmalloc so I can pass the pointer to remap_vmalloc_range(). Maybe this is overkill and a better way to do this is to call get_page() a number of times and manually add these pages to the process address space without ever setting up a kernel side mapping for these. However, what I'd really like to do is to leave it to user space to allocate the memory as David describes. In the transmit case, user space allocates memory (malloc or mmap) and loads the payload into that buffer. Then is does an ioctl() on the firewire control device to indicate the location of this buffer, describe how that payload is to be split into packets, and optionally a header per packet to prepend. The kernel side driver then converts the user space addresses to pages, pins the pages in question, and sets up dma programs to transmit the packets. Likewise for reception, user space allocates buffers for receiving the data and then instructs the kernel the receive a certain amount of data into this buffer. The kernel pins the pages backing the user space buffer and sets up dma to received into those pages. Once a page it full, it's unpinned and userspace is notified. It's not too difficult from what I'm doing now, I'd just like to give user space more control over the buffers it uses for streaming (i.e. letting user space allocate them). What I'm missing here is: how do I actually pin a page in memory? I'm sure it's not too difficult, but I haven't yet figured it out and I'm sure somebody knows it off the top of his head. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: ieee1394 feature needed: overwrite SPLIT_TIMEOUT from userspace
On 1/15/07, Philipp Beyer <[EMAIL PROTECTED]> wrote: Thanks for your input. My post was based on the (wrong) idea that the kernel already uses different timeout values per node. Therefore, having read your answer, I have a different opinion about how to solve this now. About your suggestions: Unfortunately sending an early response and using a secondary register as indication for completed flash writes doesnt work. In short, the device isn't able to process packets while writing to flash and an early answer followed by a period of non-responsiveness might lead to problems on the windows side. Also I dont like the idea of having such a big timeout for every bus transaction. In case of 'normal' operation the device runs fine with a standard timeout value. I read the thread briefly, so I may be off here, but another solution is to implement an FCP-style protocol. That is, instead of trying to cram a long operation into the SPLIT_TIMEOUT window, just use two write transactions to device specific address areas: one for the request from the PC and one for the response from the device. Or you could even use a vendor specific FCP frame. If you use unified transactions (i.e. your devices sends ACK_COMPLETE when it receives the write) it doesn't even generate more traffic on the bus. And since the device will send the response write request once it has completed programming the flash, it doesn't need to respond to packets while it is programming. But even if the write transactions themselves are split transactions, it is still a low overheads solution to your problem that avoids messing with SPLIT_TIMEOUT. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: ieee1394 feature needed: overwrite SPLIT_TIMEOUT from userspace
On 1/15/07, Philipp Beyer [EMAIL PROTECTED] wrote: Thanks for your input. My post was based on the (wrong) idea that the kernel already uses different timeout values per node. Therefore, having read your answer, I have a different opinion about how to solve this now. About your suggestions: Unfortunately sending an early response and using a secondary register as indication for completed flash writes doesnt work. In short, the device isn't able to process packets while writing to flash and an early answer followed by a period of non-responsiveness might lead to problems on the windows side. Also I dont like the idea of having such a big timeout for every bus transaction. In case of 'normal' operation the device runs fine with a standard timeout value. I read the thread briefly, so I may be off here, but another solution is to implement an FCP-style protocol. That is, instead of trying to cram a long operation into the SPLIT_TIMEOUT window, just use two write transactions to device specific address areas: one for the request from the PC and one for the response from the device. Or you could even use a vendor specific FCP frame. If you use unified transactions (i.e. your devices sends ACK_COMPLETE when it receives the write) it doesn't even generate more traffic on the bus. And since the device will send the response write request once it has completed programming the flash, it doesn't need to respond to packets while it is programming. But even if the write transactions themselves are split transactions, it is still a low overheads solution to your problem that avoids messing with SPLIT_TIMEOUT. cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: allocation failed: out of vmalloc space error treating and VIDEO1394 IOC LISTEN CHANNEL ioctl failed problem
On 1/15/07, David Moore [EMAIL PROTECTED] wrote: On Mon, 2007-01-15 at 10:20 -0800, Arjan van de Ven wrote: if you need that much you probably should redesign your algorithms to not need vmalloc in the first place I think you've convinced me that vmalloc is not a good choice when a driver needs a large buffer (many megabytes) for DMA. In this case, we need a large ring buffer for reception of isochronous packets from a firewire device. If I understand you correctly, you are suggesting that this buffer be obtained as followed: 1. Application performs malloc() in user-space and mmap()s it. 2. Driver uses vmalloc_to_page() on every page of the malloc'ed memory and constructs a scatter-gather list. 3. Map the sg list with pci_map_sg(). 4. Commence DMA. Is that correct? In particular, does it do the right thing in terms of pinning the memory and dealing with high memory? I notice that the block I/O API has some convenience functions for this, but this is not a block device. Are there some other convenience functions that can be used? Forgive me if these are obvious questions -- I'm not the developer of video1394, but I'd still like get it right for the new firewire stack that's being developed. David, thanks for bringing this up. Indeed if vmalloc is not the right way for allocating big buffers, we need to figure something else out. My impression was that the vmalloc group of functions could be used for big allocations since they don't require the underlying memory to be physically contiguous. What I'm doing currently in the new firewire stack is to vmalloc the memory to be used for isochronous payload and then use remap_vmalloc_range() to map the memory to the user. I never access the contents from the kernel side, I just use vmalloc so I can pass the pointer to remap_vmalloc_range(). Maybe this is overkill and a better way to do this is to call get_page() a number of times and manually add these pages to the process address space without ever setting up a kernel side mapping for these. However, what I'd really like to do is to leave it to user space to allocate the memory as David describes. In the transmit case, user space allocates memory (malloc or mmap) and loads the payload into that buffer. Then is does an ioctl() on the firewire control device to indicate the location of this buffer, describe how that payload is to be split into packets, and optionally a header per packet to prepend. The kernel side driver then converts the user space addresses to pages, pins the pages in question, and sets up dma programs to transmit the packets. Likewise for reception, user space allocates buffers for receiving the data and then instructs the kernel the receive a certain amount of data into this buffer. The kernel pins the pages backing the user space buffer and sets up dma to received into those pages. Once a page it full, it's unpinned and userspace is notified. It's not too difficult from what I'm doing now, I'd just like to give user space more control over the buffers it uses for streaming (i.e. letting user space allocate them). What I'm missing here is: how do I actually pin a page in memory? I'm sure it's not too difficult, but I haven't yet figured it out and I'm sure somebody knows it off the top of his head. cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: allocation failed: out of vmalloc space error treating and VIDEO1394 IOC LISTEN CHANNEL ioctl failed problem
On 1/15/07, Arjan van de Ven [EMAIL PROTECTED] wrote: However, what I'd really like to do is to leave it to user space to allocate the memory as David describes. In the transmit case, user space allocates memory (malloc or mmap) and loads the payload into that buffer. there is a lot of pain involved with doing things this way, it is a TON better if YOU provide the memory via a custom mmap handler for a device driver. (there are a lot of security nightmares involved with the opposite model, like the user can put any kind of memory there, even pci mmio space) OK, point taken. I don't have a strong preference for the opposite model, it just seems elegant that you can let user space handle allocation and pin and map the pages as needed. But you're right, it certainly is easier to give safe memory to user space in the first place rather than try to make sure user space isn't trying to trick us. Then is does an ioctl() on the firewire control device ioctls are evil ;) esp an mmap me ioctl Ah, I'm not mmap'ing it from the ioctl, I do implement the mma file operation for this. However, you have to do an ioctl before mapping the device to configure the dma context. Other than that what is the problem with ioctls, and more interesting, what is the alternative? I don't expect (or want) a bunch of syscalls to be added for this, so I don't really see what other mechanism I should use for this. It's not too difficult from what I'm doing now, I'd just like to give user space more control over the buffers it uses for streaming (i.e. letting user space allocate them). What I'm missing here is: how do I actually pin a page in memory? I'm sure it's not too difficult, but I haven't yet figured it out and I'm sure somebody knows it off the top of his head. again the best way is for you to provide an mmap method... you can then fill in the pages and keep that in some sort of array; this is for example also what the DRI/DRM layer does for textures etc... That sounds a lot like what I have now (mmap method, array of pages) so I'll just stick with that. thanks, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] New firewire stack - updated patches
Stefan Richter wrote: ... And Windows Vista will drop the IP over 1394 functionality, which is another data point about how widely used this standard is. If we cared what Windows supports or does not support, we would have gap count optimization by now, but no support of IEEE 1394b-2002. Yeah, my point wasn't that we need to drop it because Windows dropped it, it was just a data point backing up the claim that IP 1394 isn't very widely used. And as for gap count optimization, I just added that to my git repo. I thought about adding it before sending out these patches, but I was running late on getting them out -- I ended up spending too much time chasing down a stupid spinlock recursion. It is pretty simple to implement in the new stack, since we have all the topology information available. I did a quick, unscientific benchmark (md5summing 10 32M files) and the optimization is definitely noticable. This is a setup where the box and the disk are both connected to a hub so the max hops is 2, so we're using gap count 4: real0m10.021s user0m1.435s sys 0m0.356s compared to no optimization, ie gap count 63: real0m14.537s user0m1.390s sys 0m0.402s Though I see that Mac OS X uses a more conservative setting for a similiar topology, so maybe we need to add a bit or "margin" to the numbers from the table from 1394. I'm not planning to port the pcilynx driver either. I think it's a sore point for the old stack as it is - nobody uses it or tests it and it's continously bit-rotting. So I'd rather not support it. Here I agree. However, it can perform as well as an OHCI card for SBP-2. If you set up a self-modifying DMA program it can read and write system memory without CPU intervention too. OK, I didn't know that although I suspected something like this might be possible. Of course this remains a potential feature as long as there is no manpower to actually implement it. (Nor is there a userbase to speak of to appreciate such an effort.) Exactly. It's a cool hack (it's mentioned briefly in appendix E.1 of the PCILynx functional specification) and it would be fun to make it work, but I don't really see a userbase here. And if somebody has a PCILynx card and want to use the new stack, I'll trade them for a OHCI controller :) I have a much more useful way to put PCILynx cards to work using my firewire sniffer (http://bitplanet.net/nosy). cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] New firewire stack - updated patches
Pieter Palmers wrote: Kristian Høgsberg wrote: Hi, Here's a new set of patches for the new firewire stack. The changes since the last set of patches address the issues that were raised on the list and can be reviewed in detail here: .. for some reason I didn't get patch 3/4 and 4/4 on the linux1394-devel list, so I'll reply to this one. I would suggest a reordering of the interrupt flag checks. Currently the interrupts that are least likely to occur are checked first. I don't see why. I would reorder the check such that ISO interrupts are checked first, as they have the most stringent timing constraints and are most likely to occur (when using ISO traffic). All the interrupt handler does is schedule tasklets and they are all handled before returning to userspace. So not matter how you order them it's going to take the same time. If you want to defer handling of async traffic to after your userspace handler has run, you need to schedule a work_struct for handling the async events. Having said that, I doubt that the latency between iso interrupt and user space handler imposed by the irq handler and the tasklets will be a problem. All the async tasklets do is copy the data out of the DMA buffers, possibly lookup a corresponding request and eventually call complete() on some struct completion. The worst case is the bus reset tasklet which does the topology walk, but even that is pretty fast. After processing the ISO interrupts (and maybe the Async ones), a bypass could be inserted to exit the interrupt handler when there are no other interrupts to be handled. All other interrupts are to report relatively rare events or errors (error handling still to be added I assume). The effective run-length of the interrupt handler would be shorter using such a bypass, especially in the case where there is a lot of ISO traffic. I'm looking forward to your ISO implementation, and I hope to incorporate it into FreeBob really fast. Sounds great, I'll get to the isochronous receive feature as soon as possible. I'm open to changing the interrupt handling if we can acheive lower latency, but we need to be able to measure it before we start making changes. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] Add driver for OHCI firewire host controllers.
Robert Hancock wrote:
Kristian Høgsberg wrote:
...
+static struct pci_driver fw_ohci_pci_driver = {
+.name= ohci_driver_name,
+.id_table= pci_table,
+.probe= pci_probe,
+.remove= pci_remove,
+};
How about suspend/resume support? Lots of laptops have OHCI 1394 and
full suspend/resume support is something that the current ohci1394
driver lacks.
Yes, good point, that needs to work too. What I'm thinking here is that we
need to turn off the link-on bit in the self ID packets we send out and then
generate a bus reset before we suspend. It shouldn't be necessary to change
upper level drivers, the SBP-2 driver should just remaind logged in to the
storage device. When resuming, we re-enable the link-on bit and do a bus
reset again, and should be back in operation.
Kristian
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] New firewire stack - updated patches
Stefan Richter wrote: Kristian Høgsberg wrote: ... to sum up the changes: - Got rid of bitfields. - Tested on ppc, ppc64 x86-64 and x86. - ioctl interface tested on 32-bit userspace / 64-bit kernels. - ASCIIfied sources. - Incorporated Jeff Garziks comments. - Updated to work with the new workqueue API changes. - Moved subsystem to drivers/firewire from drivers/fw. plus a number of bug fixes. Congrats. WRT the 1st, 3rd, and 5th item you are now ahead of mainline's stack. :-) Hehe, yeah, I saw the big FIXME in raw1394.c about compat_ioctl. But raw1394.c shouldn't be that hard to fix, the ioctl structs are using explicitly sized types and u64 for userspace pointers, the one problem I see is that some of the 64-bit fields aren't 64-bit aligend. As mentioned last time, the stack still lacks isochronous receive functionality to be on par with the old stack, feature-wise. This is the one remaining piece of feature work kernel-side. When that is done, I have a couple of TODO items in user space: Actually there are also eth1394 and pcilynx to be pulled over. Eth1394 should be quite easy to do for anybody after iso reception is settled in your stack. Pcilynx could follow depending on developer interest. It's increasingly rare hardware and the few old machines which have it can be cheaply upgraded to OHCI (which performs better for SBP-2 anyway). Well... I don't think eth1394 was ever used much and it's not something I plan to port over. The only thing I've ever heard people say about it is that it's annoying because it screws up their network interface ordering. And Windows Vista will drop the IP over 1394 functionality, which is another data point about how widely used this standard is. I'm not planning to port the pcilynx driver either. I think it's a sore point for the old stack as it is - nobody uses it or tests it and it's continously bit-rotting. So I'd rather not support it. However, it can perform as well as an OHCI card for SBP-2. If you set up a self-modifying DMA program it can read and write system memory without CPU intervention too. - Make a libraw1394 compatibility library Consider using libraw1394 right from the start of this porting project. If there is only one libraw1394 (which works with raw1394 and with fw-device-cdev), enthusiasts might have an easier time to test your stack. Hmm, maybe. There is going to be completely different code paths for each API entry point and not a lot of code sharing. But there is definitely some merit to only having one library, and if it could detect the kernel interface automatically and just work that would be even better. cheers, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] New firewire stack - updated patches
Stefan Richter wrote: Kristian Høgsberg wrote: ... to sum up the changes: - Got rid of bitfields. - Tested on ppc, ppc64 x86-64 and x86. - ioctl interface tested on 32-bit userspace / 64-bit kernels. - ASCIIfied sources. - Incorporated Jeff Garziks comments. - Updated to work with the new workqueue API changes. - Moved subsystem to drivers/firewire from drivers/fw. plus a number of bug fixes. Congrats. WRT the 1st, 3rd, and 5th item you are now ahead of mainline's stack. :-) Hehe, yeah, I saw the big FIXME in raw1394.c about compat_ioctl. But raw1394.c shouldn't be that hard to fix, the ioctl structs are using explicitly sized types and u64 for userspace pointers, the one problem I see is that some of the 64-bit fields aren't 64-bit aligend. As mentioned last time, the stack still lacks isochronous receive functionality to be on par with the old stack, feature-wise. This is the one remaining piece of feature work kernel-side. When that is done, I have a couple of TODO items in user space: Actually there are also eth1394 and pcilynx to be pulled over. Eth1394 should be quite easy to do for anybody after iso reception is settled in your stack. Pcilynx could follow depending on developer interest. It's increasingly rare hardware and the few old machines which have it can be cheaply upgraded to OHCI (which performs better for SBP-2 anyway). Well... I don't think eth1394 was ever used much and it's not something I plan to port over. The only thing I've ever heard people say about it is that it's annoying because it screws up their network interface ordering. And Windows Vista will drop the IP over 1394 functionality, which is another data point about how widely used this standard is. I'm not planning to port the pcilynx driver either. I think it's a sore point for the old stack as it is - nobody uses it or tests it and it's continously bit-rotting. So I'd rather not support it. However, it can perform as well as an OHCI card for SBP-2. If you set up a self-modifying DMA program it can read and write system memory without CPU intervention too. - Make a libraw1394 compatibility library Consider using libraw1394 right from the start of this porting project. If there is only one libraw1394 (which works with raw1394 and with fw-device-cdev), enthusiasts might have an easier time to test your stack. Hmm, maybe. There is going to be completely different code paths for each API entry point and not a lot of code sharing. But there is definitely some merit to only having one library, and if it could detect the kernel interface automatically and just work that would be even better. cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] Add driver for OHCI firewire host controllers.
Robert Hancock wrote:
Kristian Høgsberg wrote:
...
+static struct pci_driver fw_ohci_pci_driver = {
+.name= ohci_driver_name,
+.id_table= pci_table,
+.probe= pci_probe,
+.remove= pci_remove,
+};
How about suspend/resume support? Lots of laptops have OHCI 1394 and
full suspend/resume support is something that the current ohci1394
driver lacks.
Yes, good point, that needs to work too. What I'm thinking here is that we
need to turn off the link-on bit in the self ID packets we send out and then
generate a bus reset before we suspend. It shouldn't be necessary to change
upper level drivers, the SBP-2 driver should just remaind logged in to the
storage device. When resuming, we re-enable the link-on bit and do a bus
reset again, and should be back in operation.
Kristian
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] New firewire stack - updated patches
Pieter Palmers wrote: Kristian Høgsberg wrote: Hi, Here's a new set of patches for the new firewire stack. The changes since the last set of patches address the issues that were raised on the list and can be reviewed in detail here: .. for some reason I didn't get patch 3/4 and 4/4 on the linux1394-devel list, so I'll reply to this one. I would suggest a reordering of the interrupt flag checks. Currently the interrupts that are least likely to occur are checked first. I don't see why. I would reorder the check such that ISO interrupts are checked first, as they have the most stringent timing constraints and are most likely to occur (when using ISO traffic). All the interrupt handler does is schedule tasklets and they are all handled before returning to userspace. So not matter how you order them it's going to take the same time. If you want to defer handling of async traffic to after your userspace handler has run, you need to schedule a work_struct for handling the async events. Having said that, I doubt that the latency between iso interrupt and user space handler imposed by the irq handler and the tasklets will be a problem. All the async tasklets do is copy the data out of the DMA buffers, possibly lookup a corresponding request and eventually call complete() on some struct completion. The worst case is the bus reset tasklet which does the topology walk, but even that is pretty fast. After processing the ISO interrupts (and maybe the Async ones), a bypass could be inserted to exit the interrupt handler when there are no other interrupts to be handled. All other interrupts are to report relatively rare events or errors (error handling still to be added I assume). The effective run-length of the interrupt handler would be shorter using such a bypass, especially in the case where there is a lot of ISO traffic. I'm looking forward to your ISO implementation, and I hope to incorporate it into FreeBob really fast. Sounds great, I'll get to the isochronous receive feature as soon as possible. I'm open to changing the interrupt handling if we can acheive lower latency, but we need to be able to measure it before we start making changes. cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/4] New firewire stack - updated patches
Stefan Richter wrote: ... And Windows Vista will drop the IP over 1394 functionality, which is another data point about how widely used this standard is. If we cared what Windows supports or does not support, we would have gap count optimization by now, but no support of IEEE 1394b-2002. Yeah, my point wasn't that we need to drop it because Windows dropped it, it was just a data point backing up the claim that IP 1394 isn't very widely used. And as for gap count optimization, I just added that to my git repo. I thought about adding it before sending out these patches, but I was running late on getting them out -- I ended up spending too much time chasing down a stupid spinlock recursion. It is pretty simple to implement in the new stack, since we have all the topology information available. I did a quick, unscientific benchmark (md5summing 10 32M files) and the optimization is definitely noticable. This is a setup where the box and the disk are both connected to a hub so the max hops is 2, so we're using gap count 4: real0m10.021s user0m1.435s sys 0m0.356s compared to no optimization, ie gap count 63: real0m14.537s user0m1.390s sys 0m0.402s Though I see that Mac OS X uses a more conservative setting for a similiar topology, so maybe we need to add a bit or margin to the numbers from the table from 1394. I'm not planning to port the pcilynx driver either. I think it's a sore point for the old stack as it is - nobody uses it or tests it and it's continously bit-rotting. So I'd rather not support it. Here I agree. However, it can perform as well as an OHCI card for SBP-2. If you set up a self-modifying DMA program it can read and write system memory without CPU intervention too. OK, I didn't know that although I suspected something like this might be possible. Of course this remains a potential feature as long as there is no manpower to actually implement it. (Nor is there a userbase to speak of to appreciate such an effort.) Exactly. It's a cool hack (it's mentioned briefly in appendix E.1 of the PCILynx functional specification) and it would be fun to make it work, but I don't really see a userbase here. And if somebody has a PCILynx card and want to use the new stack, I'll trade them for a OHCI controller :) I have a much more useful way to put PCILynx cards to work using my firewire sniffer (http://bitplanet.net/nosy). cheers, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/4] Add device probing and sysfs integration.
Signed-off-by: Kristian Hoegsberg <[EMAIL PROTECTED]>
---
drivers/firewire/Makefile |3
drivers/firewire/fw-card.c| 56 +++
drivers/firewire/fw-device-cdev.c | 617 +
drivers/firewire/fw-device-cdev.h | 146 +
drivers/firewire/fw-device.c | 613 +
drivers/firewire/fw-device.h | 127
drivers/firewire/fw-iso.c |1
drivers/firewire/fw-topology.c| 10 -
drivers/firewire/fw-transaction.c |5
drivers/firewire/fw-transaction.h |4
10 files changed, 1573 insertions(+), 9 deletions(-)
diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index db7020d..da77bc0 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -2,6 +2,7 @@ #
# Makefile for the Linux IEEE 1394 implementation
#
-fw-core-objs := fw-card.o fw-topology.o fw-transaction.o fw-iso.o
+fw-core-objs := fw-card.o fw-topology.o fw-transaction.o fw-iso.o \
+ fw-device.o fw-device-cdev.o
obj-$(CONFIG_FW) += fw-core.o
diff --git a/drivers/firewire/fw-card.c b/drivers/firewire/fw-card.c
index d8abd70..7977390 100644
--- a/drivers/firewire/fw-card.c
+++ b/drivers/firewire/fw-card.c
@@ -24,6 +24,7 @@ #include
#include
#include "fw-transaction.h"
#include "fw-topology.h"
+#include "fw-device.h"
/* The lib/crc16.c implementation uses the standard (0x8005)
* polynomial, but we need the ITU-T (or CCITT) polynomial (0x1021).
@@ -186,6 +187,59 @@ fw_core_remove_descriptor (struct fw_des
EXPORT_SYMBOL(fw_core_remove_descriptor);
static void
+fw_card_irm_work(struct work_struct *work)
+{
+ struct fw_card *card =
+ container_of(work, struct fw_card, work.work);
+ struct fw_device *root;
+ unsigned long flags;
+ int new_irm_id, generation;
+
+ /* FIXME: This simple bus management unconditionally picks a
+* cycle master if the current root can't do it. We need to
+* not do this if there is a bus manager already. Also, some
+* hubs set the contender bit, which is bogus, so we should
+* probably do a little sanity check on the IRM (like, read
+* the bandwidth register) if it's not us. */
+
+ spin_lock_irqsave(>lock, flags);
+
+ generation = card->generation;
+ root = card->root_node->data;
+
+ if (root == NULL)
+ /* Either link_on is false, or we failed to read the
+* config rom. In either case, pick another root. */
+ new_irm_id = card->local_node->node_id;
+ else if (root->state != FW_DEVICE_RUNNING)
+ /* If we haven't probed this device yet, bail out now
+* and let's try again once that's done. */
+ new_irm_id = -1;
+ else if (root->config_rom[2] & bib_cmc)
+ /* FIXME: I suppose we should set the cmstr bit in the
+* STATE_CLEAR register of this node, as described in
+* 1394-1995, 8.4.2.6. Also, send out a force root
+* packet for this node. */
+ new_irm_id = -1;
+ else
+ /* Current root has an active link layer and we
+* successfully read the config rom, but it's not
+* cycle master capable. */
+ new_irm_id = card->local_node->node_id;
+
+ if (card->irm_retries++ > 5)
+ new_irm_id = -1;
+
+ spin_unlock_irqrestore(>lock, flags);
+
+ if (new_irm_id > 0) {
+ fw_notify("Trying to become root (card %d)\n", card->index);
+ fw_send_force_root(card, new_irm_id, generation);
+ fw_core_initiate_bus_reset(card, 1);
+ }
+}
+
+static void
release_card(struct device *device)
{
struct fw_card *card =
@@ -222,6 +276,8 @@ fw_card_initialize(struct fw_card *card,
card->local_node = NULL;
+ INIT_DELAYED_WORK(>work, fw_card_irm_work);
+
card->card_device.bus = _bus_type;
card->card_device.release = release_card;
card->card_device.parent = card->device;
diff --git a/drivers/firewire/fw-device-cdev.c
b/drivers/firewire/fw-device-cdev.c
new file mode 100644
index 000..c10e332
--- /dev/null
+++ b/drivers/firewire/fw-device-cdev.c
@@ -0,0 +1,617 @@
+/* -*- c-basic-offset: 8 -*-
+ *
+ * fw-device-cdev.c - Char device for device raw access
+ *
+ * Copyright (C) 2005-2006 Kristian Hoegsberg <[EMAIL PROTECTED]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
[PATCH 3/4] Add driver for OHCI firewire host controllers.
Signed-off-by: Kristian Hoegsberg <[EMAIL PROTECTED]>
---
drivers/firewire/Kconfig | 11
drivers/firewire/Makefile |1
drivers/firewire/fw-ohci.c | 1394
drivers/firewire/fw-ohci.h | 152 +
4 files changed, 1558 insertions(+), 0 deletions(-)
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index bdd6303..b386334 100644
--- a/drivers/firewire/Kconfig
+++ b/drivers/firewire/Kconfig
@@ -20,4 +20,15 @@ config FW
To compile this driver as a module, say M here: the
module will be called fw-core.
+config FW_OHCI
+ tristate "Support for OHCI firewire host controllers"
+ depends on PCI && FW
+ help
+ Enable this driver if you have an firewire controller based
+ on the OHCI specification. For all practical purposes, this
+ is the only chipset in use, so say Y here.
+
+ To compile this driver as a module, say M here: the
+ module will be called fw-ohci.
+
endmenu
diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index da77bc0..add3b98 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -6,3 +6,4 @@ fw-core-objs := fw-card.o fw-topology.o
fw-device.o fw-device-cdev.o
obj-$(CONFIG_FW) += fw-core.o
+obj-$(CONFIG_FW_OHCI) += fw-ohci.o
diff --git a/drivers/firewire/fw-ohci.c b/drivers/firewire/fw-ohci.c
new file mode 100644
index 000..5392a2b
--- /dev/null
+++ b/drivers/firewire/fw-ohci.c
@@ -0,0 +1,1394 @@
+/* -*- c-basic-offset: 8 -*-
+ *
+ * fw-ohci.c - Driver for OHCI 1394 boards
+ * Copyright (C) 2003-2006 Kristian Hoegsberg <[EMAIL PROTECTED]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "fw-transaction.h"
+#include "fw-ohci.h"
+
+#define descriptor_output_more 0
+#define descriptor_output_last (1 << 12)
+#define descriptor_input_more (2 << 12)
+#define descriptor_input_last (3 << 12)
+#define descriptor_status (1 << 11)
+#define descriptor_key_immediate (2 << 8)
+#define descriptor_ping(1 << 7)
+#define descriptor_yy (1 << 6)
+#define descriptor_no_irq (0 << 4)
+#define descriptor_irq_error (1 << 4)
+#define descriptor_irq_always (3 << 4)
+#define descriptor_branch_always (3 << 2)
+
+struct descriptor {
+ __le16 req_count;
+ __le16 control;
+ __le32 data_address;
+ __le32 branch_address;
+ __le16 res_count;
+ __le16 transfer_status;
+} __attribute__((aligned(16)));
+
+struct ar_context {
+ struct fw_ohci *ohci;
+ struct descriptor descriptor;
+ __le32 buffer[512];
+ dma_addr_t descriptor_bus;
+ dma_addr_t buffer_bus;
+
+ u32 command_ptr;
+ u32 control_set;
+ u32 control_clear;
+
+ struct tasklet_struct tasklet;
+};
+
+struct at_context {
+ struct fw_ohci *ohci;
+ dma_addr_t descriptor_bus;
+ dma_addr_t buffer_bus;
+
+ struct list_head list;
+
+ struct {
+ struct descriptor more;
+ __le32 header[4];
+ struct descriptor last;
+ } d;
+
+ u32 command_ptr;
+ u32 control_set;
+ u32 control_clear;
+
+ struct tasklet_struct tasklet;
+};
+
+#define it_header_sy(v) ((v) << 0)
+#define it_header_tcode(v) ((v) << 4)
+#define it_header_channel(v) ((v) << 8)
+#define it_header_tag(v) ((v) << 14)
+#define it_header_speed(v) ((v) << 16)
+#define it_header_data_length(v) ((v) << 16)
+
+struct iso_context {
+ struct fw_iso_context base;
+ struct tasklet_struct tasklet;
+ u32 control_set;
+ u32 control_clear;
+ u32 command_ptr;
+ u32 context_match;
+
+ struct descriptor *buffer;
+ dma_addr_t buffer_bus;
+ struct descriptor *head_descriptor;
+ struct descriptor *tail_descriptor;
+ struct descriptor *tail_descriptor_last;
+ struct descriptor *prev_descriptor;
+};
+
+#define CONFIG_ROM_SIZE 1024
+
+struct fw_ohci {
+ struct fw_card card;
+
+ __iomem char
[PATCH 4/4] Add SBP-2 protocol driver for storage devices.
Signed-off-by: Kristian Hoegsberg <[EMAIL PROTECTED]>
---
drivers/firewire/Kconfig | 12
drivers/firewire/Makefile |1
drivers/firewire/fw-sbp2.c | 1073
3 files changed, 1086 insertions(+), 0 deletions(-)
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index b386334..bfab4b3 100644
--- a/drivers/firewire/Kconfig
+++ b/drivers/firewire/Kconfig
@@ -31,4 +31,16 @@ config FW_OHCI
To compile this driver as a module, say M here: the
module will be called fw-ohci.
+config FW_SBP2
+ tristate "Support for storage devices (SBP-2 protocol driver)"
+ depends on FW && SCSI
+ help
+ This option enables you to use SBP-2 devices connected to an
+ firewire bus. SBP-2 devices include storage devices like
+ harddisks and DVD drives, also some other FireWire devices
+ like scanners.
+
+ You should also enable support for disks, CD-ROMs, etc. in the SCSI
+ configuration section.
+
endmenu
diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index add3b98..b955c99 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -7,3 +7,4 @@ fw-core-objs := fw-card.o fw-topology.o
obj-$(CONFIG_FW) += fw-core.o
obj-$(CONFIG_FW_OHCI) += fw-ohci.o
+obj-$(CONFIG_FW_SBP2) += fw-sbp2.o
\ No newline at end of file
diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
new file mode 100644
index 000..2756e0c
--- /dev/null
+++ b/drivers/firewire/fw-sbp2.c
@@ -0,0 +1,1073 @@
+/* -*- c-basic-offset: 8 -*-
+ * fw-sbp2.c -- SBP2 driver (SCSI over IEEE1394)
+ *
+ * Copyright (C) 2005-2006 Kristian Hoegsberg <[EMAIL PROTECTED]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+#include "fw-transaction.h"
+#include "fw-topology.h"
+#include "fw-device.h"
+
+/* I don't know why the SCSI stack doesn't define something like this... */
+typedef void (*scsi_done_fn_t) (struct scsi_cmnd *);
+
+static const char sbp2_driver_name[] = "sbp2";
+
+struct sbp2_device {
+ struct fw_unit *unit;
+ struct fw_address_handler address_handler;
+ struct list_head orb_list;
+ u64 management_agent_address;
+ u64 command_block_agent_address;
+ u32 workarounds;
+ int login_id;
+
+ /* We cache these addresses and only update them once we've
+* logged in or reconnected to the sbp2 device. That way, any
+* IO to the device will automatically fail and get retried if
+* it happens in a window where the device is not ready to
+* handle it (e.g. after a bus reset but before we reconnect). */
+ int node_id;
+ int address_high;
+ int generation;
+
+ struct work_struct work;
+ struct Scsi_Host *scsi_host;
+};
+
+#define SBP2_MAX_SG_ELEMENT_LENGTH 0xf000
+#define SBP2_MAX_SECTORS 255 /* Max sectors supported */
+#define SBP2_MAX_CMDS 8 /* This should be safe */
+
+#define SBP2_ORB_NULL 0x8000
+
+#define SBP2_DIRECTION_TO_MEDIA0x0
+#define SBP2_DIRECTION_FROM_MEDIA 0x1
+
+/* Unit directory keys */
+#define SBP2_COMMAND_SET_SPECIFIER 0x38
+#define SBP2_COMMAND_SET 0x39
+#define SBP2_COMMAND_SET_REVISION 0x3b
+#define SBP2_FIRMWARE_REVISION 0x3c
+
+/* Flags for detected oddities and brokeness */
+#define SBP2_WORKAROUND_128K_MAX_TRANS 0x1
+#define SBP2_WORKAROUND_INQUIRY_36 0x2
+#define SBP2_WORKAROUND_MODE_SENSE_8 0x4
+#define SBP2_WORKAROUND_FIX_CAPACITY 0x8
+#define SBP2_WORKAROUND_OVERRIDE 0x100
+
+/* Management orb opcodes */
+#define SBP2_LOGIN_REQUEST 0x0
+#define SBP2_QUERY_LOGINS_REQUEST 0x1
+#define SBP2_RECONNECT_REQUEST 0x3
+#define SBP2_SET_PASSWORD_REQUEST 0x4
+#define SBP2_LOGOUT_REQUEST0x7
+#define SBP2_ABORT_TASK_REQUEST0xb
+#define SBP2_ABORT_TASK_SET0xc
+#define SBP2_LOGICAL_UNIT_RESET0xe
+#define SBP2_TARGET_RESET_REQUEST 0xf
+
+/* Offsets for command block agent registers */
+#define SBP2_AGENT_STATE
[PATCH 0/4] New firewire stack - updated patches
Hi, Here's a new set of patches for the new firewire stack. The changes since the last set of patches address the issues that were raised on the list and can be reviewed in detail here: http://gitweb.freedesktop.org/?p=users/krh/juju.git but to sum up the changes: - Got rid of bitfields. - Tested on ppc, ppc64 x86-64 and x86. - ioctl interface tested on 32-bit userspace / 64-bit kernels. - ASCIIfied sources. - Incorporated Jeff Garziks comments. - Updated to work with the new workqueue API changes. - Moved subsystem to drivers/firewire from drivers/fw. plus a number of bug fixes. As mentioned last time, the stack still lacks isochronous receive functionality to be on par with the old stack, feature-wise. This is the one remaining piece of feature work kernel-side. When that is done, I have a couple of TODO items in user space: - Make a libraw1394 compatibility library - Port libdv1394 to new isochronous API. which will allow us to move most user space applications to the new stack. That is, even if the new stack provides a new interface for asynchronous and isochronous IO, a lot of applications can still work since the changes are isolated to a couple of libraries. This is still in development and is being discussed on the linux1394-devel list. It will likely require a few changes kernel side in the stack as we figure out how to do this. It is still work in progress, but at least now it should work across all architectures and endianesses. Happy Holidays, Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 0/4] New firewire stack - updated patches
Hi, Here's a new set of patches for the new firewire stack. The changes since the last set of patches address the issues that were raised on the list and can be reviewed in detail here: http://gitweb.freedesktop.org/?p=users/krh/juju.git but to sum up the changes: - Got rid of bitfields. - Tested on ppc, ppc64 x86-64 and x86. - ioctl interface tested on 32-bit userspace / 64-bit kernels. - ASCIIfied sources. - Incorporated Jeff Garziks comments. - Updated to work with the new workqueue API changes. - Moved subsystem to drivers/firewire from drivers/fw. plus a number of bug fixes. As mentioned last time, the stack still lacks isochronous receive functionality to be on par with the old stack, feature-wise. This is the one remaining piece of feature work kernel-side. When that is done, I have a couple of TODO items in user space: - Make a libraw1394 compatibility library - Port libdv1394 to new isochronous API. which will allow us to move most user space applications to the new stack. That is, even if the new stack provides a new interface for asynchronous and isochronous IO, a lot of applications can still work since the changes are isolated to a couple of libraries. This is still in development and is being discussed on the linux1394-devel list. It will likely require a few changes kernel side in the stack as we figure out how to do this. It is still work in progress, but at least now it should work across all architectures and endianesses. Happy Holidays, Kristian - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/4] Add SBP-2 protocol driver for storage devices.
Signed-off-by: Kristian Hoegsberg [EMAIL PROTECTED]
---
drivers/firewire/Kconfig | 12
drivers/firewire/Makefile |1
drivers/firewire/fw-sbp2.c | 1073
3 files changed, 1086 insertions(+), 0 deletions(-)
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index b386334..bfab4b3 100644
--- a/drivers/firewire/Kconfig
+++ b/drivers/firewire/Kconfig
@@ -31,4 +31,16 @@ config FW_OHCI
To compile this driver as a module, say M here: the
module will be called fw-ohci.
+config FW_SBP2
+ tristate Support for storage devices (SBP-2 protocol driver)
+ depends on FW SCSI
+ help
+ This option enables you to use SBP-2 devices connected to an
+ firewire bus. SBP-2 devices include storage devices like
+ harddisks and DVD drives, also some other FireWire devices
+ like scanners.
+
+ You should also enable support for disks, CD-ROMs, etc. in the SCSI
+ configuration section.
+
endmenu
diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index add3b98..b955c99 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -7,3 +7,4 @@ fw-core-objs := fw-card.o fw-topology.o
obj-$(CONFIG_FW) += fw-core.o
obj-$(CONFIG_FW_OHCI) += fw-ohci.o
+obj-$(CONFIG_FW_SBP2) += fw-sbp2.o
\ No newline at end of file
diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
new file mode 100644
index 000..2756e0c
--- /dev/null
+++ b/drivers/firewire/fw-sbp2.c
@@ -0,0 +1,1073 @@
+/* -*- c-basic-offset: 8 -*-
+ * fw-sbp2.c -- SBP2 driver (SCSI over IEEE1394)
+ *
+ * Copyright (C) 2005-2006 Kristian Hoegsberg [EMAIL PROTECTED]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/device.h
+#include linux/dma-mapping.h
+
+#include scsi/scsi.h
+#include scsi/scsi_cmnd.h
+#include scsi/scsi_dbg.h
+#include scsi/scsi_device.h
+#include scsi/scsi_host.h
+
+#include fw-transaction.h
+#include fw-topology.h
+#include fw-device.h
+
+/* I don't know why the SCSI stack doesn't define something like this... */
+typedef void (*scsi_done_fn_t) (struct scsi_cmnd *);
+
+static const char sbp2_driver_name[] = sbp2;
+
+struct sbp2_device {
+ struct fw_unit *unit;
+ struct fw_address_handler address_handler;
+ struct list_head orb_list;
+ u64 management_agent_address;
+ u64 command_block_agent_address;
+ u32 workarounds;
+ int login_id;
+
+ /* We cache these addresses and only update them once we've
+* logged in or reconnected to the sbp2 device. That way, any
+* IO to the device will automatically fail and get retried if
+* it happens in a window where the device is not ready to
+* handle it (e.g. after a bus reset but before we reconnect). */
+ int node_id;
+ int address_high;
+ int generation;
+
+ struct work_struct work;
+ struct Scsi_Host *scsi_host;
+};
+
+#define SBP2_MAX_SG_ELEMENT_LENGTH 0xf000
+#define SBP2_MAX_SECTORS 255 /* Max sectors supported */
+#define SBP2_MAX_CMDS 8 /* This should be safe */
+
+#define SBP2_ORB_NULL 0x8000
+
+#define SBP2_DIRECTION_TO_MEDIA0x0
+#define SBP2_DIRECTION_FROM_MEDIA 0x1
+
+/* Unit directory keys */
+#define SBP2_COMMAND_SET_SPECIFIER 0x38
+#define SBP2_COMMAND_SET 0x39
+#define SBP2_COMMAND_SET_REVISION 0x3b
+#define SBP2_FIRMWARE_REVISION 0x3c
+
+/* Flags for detected oddities and brokeness */
+#define SBP2_WORKAROUND_128K_MAX_TRANS 0x1
+#define SBP2_WORKAROUND_INQUIRY_36 0x2
+#define SBP2_WORKAROUND_MODE_SENSE_8 0x4
+#define SBP2_WORKAROUND_FIX_CAPACITY 0x8
+#define SBP2_WORKAROUND_OVERRIDE 0x100
+
+/* Management orb opcodes */
+#define SBP2_LOGIN_REQUEST 0x0
+#define SBP2_QUERY_LOGINS_REQUEST 0x1
+#define SBP2_RECONNECT_REQUEST 0x3
+#define SBP2_SET_PASSWORD_REQUEST 0x4
+#define SBP2_LOGOUT_REQUEST0x7
+#define SBP2_ABORT_TASK_REQUEST0xb
+#define SBP2_ABORT_TASK_SET0xc
+#define SBP2_LOGICAL_UNIT_RESET0xe
[PATCH 2/4] Add device probing and sysfs integration.
Signed-off-by: Kristian Hoegsberg [EMAIL PROTECTED]
---
drivers/firewire/Makefile |3
drivers/firewire/fw-card.c| 56 +++
drivers/firewire/fw-device-cdev.c | 617 +
drivers/firewire/fw-device-cdev.h | 146 +
drivers/firewire/fw-device.c | 613 +
drivers/firewire/fw-device.h | 127
drivers/firewire/fw-iso.c |1
drivers/firewire/fw-topology.c| 10 -
drivers/firewire/fw-transaction.c |5
drivers/firewire/fw-transaction.h |4
10 files changed, 1573 insertions(+), 9 deletions(-)
diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index db7020d..da77bc0 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -2,6 +2,7 @@ #
# Makefile for the Linux IEEE 1394 implementation
#
-fw-core-objs := fw-card.o fw-topology.o fw-transaction.o fw-iso.o
+fw-core-objs := fw-card.o fw-topology.o fw-transaction.o fw-iso.o \
+ fw-device.o fw-device-cdev.o
obj-$(CONFIG_FW) += fw-core.o
diff --git a/drivers/firewire/fw-card.c b/drivers/firewire/fw-card.c
index d8abd70..7977390 100644
--- a/drivers/firewire/fw-card.c
+++ b/drivers/firewire/fw-card.c
@@ -24,6 +24,7 @@ #include linux/errno.h
#include linux/device.h
#include fw-transaction.h
#include fw-topology.h
+#include fw-device.h
/* The lib/crc16.c implementation uses the standard (0x8005)
* polynomial, but we need the ITU-T (or CCITT) polynomial (0x1021).
@@ -186,6 +187,59 @@ fw_core_remove_descriptor (struct fw_des
EXPORT_SYMBOL(fw_core_remove_descriptor);
static void
+fw_card_irm_work(struct work_struct *work)
+{
+ struct fw_card *card =
+ container_of(work, struct fw_card, work.work);
+ struct fw_device *root;
+ unsigned long flags;
+ int new_irm_id, generation;
+
+ /* FIXME: This simple bus management unconditionally picks a
+* cycle master if the current root can't do it. We need to
+* not do this if there is a bus manager already. Also, some
+* hubs set the contender bit, which is bogus, so we should
+* probably do a little sanity check on the IRM (like, read
+* the bandwidth register) if it's not us. */
+
+ spin_lock_irqsave(card-lock, flags);
+
+ generation = card-generation;
+ root = card-root_node-data;
+
+ if (root == NULL)
+ /* Either link_on is false, or we failed to read the
+* config rom. In either case, pick another root. */
+ new_irm_id = card-local_node-node_id;
+ else if (root-state != FW_DEVICE_RUNNING)
+ /* If we haven't probed this device yet, bail out now
+* and let's try again once that's done. */
+ new_irm_id = -1;
+ else if (root-config_rom[2] bib_cmc)
+ /* FIXME: I suppose we should set the cmstr bit in the
+* STATE_CLEAR register of this node, as described in
+* 1394-1995, 8.4.2.6. Also, send out a force root
+* packet for this node. */
+ new_irm_id = -1;
+ else
+ /* Current root has an active link layer and we
+* successfully read the config rom, but it's not
+* cycle master capable. */
+ new_irm_id = card-local_node-node_id;
+
+ if (card-irm_retries++ 5)
+ new_irm_id = -1;
+
+ spin_unlock_irqrestore(card-lock, flags);
+
+ if (new_irm_id 0) {
+ fw_notify(Trying to become root (card %d)\n, card-index);
+ fw_send_force_root(card, new_irm_id, generation);
+ fw_core_initiate_bus_reset(card, 1);
+ }
+}
+
+static void
release_card(struct device *device)
{
struct fw_card *card =
@@ -222,6 +276,8 @@ fw_card_initialize(struct fw_card *card,
card-local_node = NULL;
+ INIT_DELAYED_WORK(card-work, fw_card_irm_work);
+
card-card_device.bus = fw_bus_type;
card-card_device.release = release_card;
card-card_device.parent = card-device;
diff --git a/drivers/firewire/fw-device-cdev.c
b/drivers/firewire/fw-device-cdev.c
new file mode 100644
index 000..c10e332
--- /dev/null
+++ b/drivers/firewire/fw-device-cdev.c
@@ -0,0 +1,617 @@
+/* -*- c-basic-offset: 8 -*-
+ *
+ * fw-device-cdev.c - Char device for device raw access
+ *
+ * Copyright (C) 2005-2006 Kristian Hoegsberg [EMAIL PROTECTED]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A
[PATCH 3/4] Add driver for OHCI firewire host controllers.
Signed-off-by: Kristian Hoegsberg [EMAIL PROTECTED]
---
drivers/firewire/Kconfig | 11
drivers/firewire/Makefile |1
drivers/firewire/fw-ohci.c | 1394
drivers/firewire/fw-ohci.h | 152 +
4 files changed, 1558 insertions(+), 0 deletions(-)
diff --git a/drivers/firewire/Kconfig b/drivers/firewire/Kconfig
index bdd6303..b386334 100644
--- a/drivers/firewire/Kconfig
+++ b/drivers/firewire/Kconfig
@@ -20,4 +20,15 @@ config FW
To compile this driver as a module, say M here: the
module will be called fw-core.
+config FW_OHCI
+ tristate Support for OHCI firewire host controllers
+ depends on PCI FW
+ help
+ Enable this driver if you have an firewire controller based
+ on the OHCI specification. For all practical purposes, this
+ is the only chipset in use, so say Y here.
+
+ To compile this driver as a module, say M here: the
+ module will be called fw-ohci.
+
endmenu
diff --git a/drivers/firewire/Makefile b/drivers/firewire/Makefile
index da77bc0..add3b98 100644
--- a/drivers/firewire/Makefile
+++ b/drivers/firewire/Makefile
@@ -6,3 +6,4 @@ fw-core-objs := fw-card.o fw-topology.o
fw-device.o fw-device-cdev.o
obj-$(CONFIG_FW) += fw-core.o
+obj-$(CONFIG_FW_OHCI) += fw-ohci.o
diff --git a/drivers/firewire/fw-ohci.c b/drivers/firewire/fw-ohci.c
new file mode 100644
index 000..5392a2b
--- /dev/null
+++ b/drivers/firewire/fw-ohci.c
@@ -0,0 +1,1394 @@
+/* -*- c-basic-offset: 8 -*-
+ *
+ * fw-ohci.c - Driver for OHCI 1394 boards
+ * Copyright (C) 2003-2006 Kristian Hoegsberg [EMAIL PROTECTED]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include linux/kernel.h
+#include linux/module.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/pci.h
+#include linux/delay.h
+#include linux/poll.h
+#include asm/uaccess.h
+#include asm/semaphore.h
+
+#include fw-transaction.h
+#include fw-ohci.h
+
+#define descriptor_output_more 0
+#define descriptor_output_last (1 12)
+#define descriptor_input_more (2 12)
+#define descriptor_input_last (3 12)
+#define descriptor_status (1 11)
+#define descriptor_key_immediate (2 8)
+#define descriptor_ping(1 7)
+#define descriptor_yy (1 6)
+#define descriptor_no_irq (0 4)
+#define descriptor_irq_error (1 4)
+#define descriptor_irq_always (3 4)
+#define descriptor_branch_always (3 2)
+
+struct descriptor {
+ __le16 req_count;
+ __le16 control;
+ __le32 data_address;
+ __le32 branch_address;
+ __le16 res_count;
+ __le16 transfer_status;
+} __attribute__((aligned(16)));
+
+struct ar_context {
+ struct fw_ohci *ohci;
+ struct descriptor descriptor;
+ __le32 buffer[512];
+ dma_addr_t descriptor_bus;
+ dma_addr_t buffer_bus;
+
+ u32 command_ptr;
+ u32 control_set;
+ u32 control_clear;
+
+ struct tasklet_struct tasklet;
+};
+
+struct at_context {
+ struct fw_ohci *ohci;
+ dma_addr_t descriptor_bus;
+ dma_addr_t buffer_bus;
+
+ struct list_head list;
+
+ struct {
+ struct descriptor more;
+ __le32 header[4];
+ struct descriptor last;
+ } d;
+
+ u32 command_ptr;
+ u32 control_set;
+ u32 control_clear;
+
+ struct tasklet_struct tasklet;
+};
+
+#define it_header_sy(v) ((v) 0)
+#define it_header_tcode(v) ((v) 4)
+#define it_header_channel(v) ((v) 8)
+#define it_header_tag(v) ((v) 14)
+#define it_header_speed(v) ((v) 16)
+#define it_header_data_length(v) ((v) 16)
+
+struct iso_context {
+ struct fw_iso_context base;
+ struct tasklet_struct tasklet;
+ u32 control_set;
+ u32 control_clear;
+ u32 command_ptr;
+ u32 context_match;
+
+ struct descriptor *buffer;
+ dma_addr_t buffer_bus;
+ struct descriptor *head_descriptor;
+ struct descriptor *tail_descriptor;
+ struct descriptor *tail_descriptor_last;
+ struct descriptor *prev_descriptor;
+};
+
+#define CONFIG_ROM_SIZE 1024
[PATCH] Add PCI class ID for firewire OHCI controllers.
Pull this define out of drivers/ieee1394/ohci1394.c and rename to match
other PCI class defines.
---
drivers/ieee1394/ohci1394.c |4 +---
include/linux/pci_ids.h |1 +
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/ieee1394/ohci1394.c b/drivers/ieee1394/ohci1394.c
index 6e8ea91..3bc55d4 100644
--- a/drivers/ieee1394/ohci1394.c
+++ b/drivers/ieee1394/ohci1394.c
@@ -3584,11 +3584,9 @@ #endif /* CONFIG_PPC_PMAC */
}
#endif /* CONFIG_PM */
-#define PCI_CLASS_FIREWIRE_OHCI ((PCI_CLASS_SERIAL_FIREWIRE << 8) | 0x10)
-
static struct pci_device_id ohci1394_pci_tbl[] = {
{
- .class =PCI_CLASS_FIREWIRE_OHCI,
+ .class =PCI_CLASS_SERIAL_FIREWIRE_OHCI,
.class_mask = PCI_ANY_ID,
.vendor = PCI_ANY_ID,
.device = PCI_ANY_ID,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index c09da1e..4849b26 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -95,6 +95,7 @@ #define PCI_CLASS_PROCESSOR_CO0x0b40
#define PCI_BASE_CLASS_SERIAL 0x0c
#define PCI_CLASS_SERIAL_FIREWIRE 0x0c00
+#define PCI_CLASS_SERIAL_FIREWIRE_OHCI 0x0c0010
#define PCI_CLASS_SERIAL_ACCESS0x0c01
#define PCI_CLASS_SERIAL_SSA 0x0c02
#define PCI_CLASS_SERIAL_USB 0x0c03
--
1.4.2.4
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Add PCI class ID for firewire OHCI controllers.
Pull this define out of drivers/ieee1394/ohci1394.c and rename to match
other PCI class defines.
---
drivers/ieee1394/ohci1394.c |4 +---
include/linux/pci_ids.h |1 +
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/ieee1394/ohci1394.c b/drivers/ieee1394/ohci1394.c
index 6e8ea91..3bc55d4 100644
--- a/drivers/ieee1394/ohci1394.c
+++ b/drivers/ieee1394/ohci1394.c
@@ -3584,11 +3584,9 @@ #endif /* CONFIG_PPC_PMAC */
}
#endif /* CONFIG_PM */
-#define PCI_CLASS_FIREWIRE_OHCI ((PCI_CLASS_SERIAL_FIREWIRE 8) | 0x10)
-
static struct pci_device_id ohci1394_pci_tbl[] = {
{
- .class =PCI_CLASS_FIREWIRE_OHCI,
+ .class =PCI_CLASS_SERIAL_FIREWIRE_OHCI,
.class_mask = PCI_ANY_ID,
.vendor = PCI_ANY_ID,
.device = PCI_ANY_ID,
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index c09da1e..4849b26 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -95,6 +95,7 @@ #define PCI_CLASS_PROCESSOR_CO0x0b40
#define PCI_BASE_CLASS_SERIAL 0x0c
#define PCI_CLASS_SERIAL_FIREWIRE 0x0c00
+#define PCI_CLASS_SERIAL_FIREWIRE_OHCI 0x0c0010
#define PCI_CLASS_SERIAL_ACCESS0x0c01
#define PCI_CLASS_SERIAL_SSA 0x0c02
#define PCI_CLASS_SERIAL_USB 0x0c03
--
1.4.2.4
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] Import fw-sbp2 driver.
Stefan Richter wrote: Kristian Høgsberg wrote: Jeff Garzik wrote: doesn't allowing the stack to issue REPORT LUNS take care of this? Possibly, I don't have firewire multi-LUN devices to test with here. The LUNs are also discoverable from the firewire config rom, which is why I put the comment there. This doesn't mean that the SCSI commands for discovering LUNs doesn't also work. I expect REPORT LUNS won't work for many SBP-2 devices. It is not included in RBC. We discover LUs properly from the information in the ISO 13213 ROM. We just don't map multiple LUs of the same target to scsi_device's beneath a single scsi_target. (We instantiate one Scsi_Host for each LU. I might implement a respective mapping some day, but there is no bigger benefit of doing so.) Yeah, I saw that the stack creates a struct device per LUN, which is kinda gross in my opinion. It's easy enough to discover the LUNs from the rom, I just need to figure out how to tell the SCSI stack about multiple LUNs. Kristian - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
