- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes

by Christian König

Am 30.11.21 um 13:31 schrieb Thomas Hellström: > [SNIP] >> Other than that, I didn't investigate the nesting fails enough to say >> I can accurately review this. :) > > Basically the problem is that within enable_signaling() which is > called with the dma_fence lock held, we take the dma_fence lock of > another fence. If that other fence is a dma_fence_array, or a > dma_fence_chain which in turn tries to lock a dma_fence_array we hit a > splat. Yeah, I already thought that you constructed something like that. You get the splat because what you do here is illegal, you can't mix dma_fence_array and dma_fence_chain like this or you can end up in a stack corruption. Regards, Christian. > > But I'll update the commit message with a typical splat. > > /Thomas

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC PATCH 0/2] Attempt to avoid dma-fence-[chain|array] lockdep splats

by Christian König

Am 30.11.21 um 13:19 schrieb Thomas Hellström: > Introducing more usage of dma-fence-chain and dma-fence-array in the > i915 driver we start to hit lockdep splats due to the recursive fence > locking in the dma-fence-chain and dma-fence-array containers. > This is a humble suggestion to try to establish a dma-fence locking order > (patch 1) and to avoid excessive recursive locking in these containers > (patch 2) Well completely NAK to this. This splats are intentional notes that something in the driver code is wrong (or we messed up the chain and array containers somehow). Those two containers are intentionally crafted in a way which allows to avoid any dependency between their spinlocks. So as long as you correctly use them you should never see a splat. Please provide the lockdep splat so that we can analyze the problem. Thanks, Christian. > > Thomas Hellström (2): > dma-fence: Avoid establishing a locking order between fence classes > dma-fence: Avoid excessive recursive fence locking from > enable_signaling() callbacks > > drivers/dma-buf/dma-fence-array.c | 23 +++++++-- > drivers/dma-buf/dma-fence-chain.c | 12 ++++- > drivers/dma-buf/dma-fence.c | 79 +++++++++++++++++++++---------- > include/linux/dma-fence.h | 4 ++ > 4 files changed, 89 insertions(+), 29 deletions(-) > > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: dri-devel(a)lists.freedesktop.org > Cc: Christian König <christian.koenig(a)amd.com> >

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC PATCH 1/2] dma-fence: Avoid establishing a locking order between fence classes

by Maarten Lankhorst

On 30-11-2021 13:19, Thomas Hellström wrote: > The locking order for taking two fence locks is implicitly defined in > at least two ways in the code: > > 1) Fence containers first and other fences next, which is defined by > the enable_signaling() callbacks of dma_fence_chain and > dma_fence_array. > 2) Reverse signal order, which is used by __i915_active_fence_set(). > > Now 1) implies 2), except for the signal_on_any mode of dma_fence_array > and 2) does not imply 1), and also 1) makes locking order between > different containers confusing. > > Establish 2) and fix up the signal_on_any mode by calling > enable_signaling() on such fences unlocked at creation. > > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: dri-devel(a)lists.freedesktop.org > Cc: Christian König <christian.koenig(a)amd.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom(a)linux.intel.com> > --- > drivers/dma-buf/dma-fence-array.c | 13 +++-- > drivers/dma-buf/dma-fence-chain.c | 3 +- > drivers/dma-buf/dma-fence.c | 79 +++++++++++++++++++++---------- > include/linux/dma-fence.h | 3 ++ > 4 files changed, 69 insertions(+), 29 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index 3e07f961e2f3..0322b92909fe 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -84,8 +84,8 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > * insufficient). > */ > dma_fence_get(&array->base); > - if (dma_fence_add_callback(array->fences[i], &cb[i].cb, > - dma_fence_array_cb_func)) { > + if (dma_fence_add_callback_nested(array->fences[i], &cb[i].cb, > + dma_fence_array_cb_func)) { > int error = array->fences[i]->error; > > dma_fence_array_set_pending_error(array, error); > @@ -158,6 +158,7 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > { > struct dma_fence_array *array; > size_t size = sizeof(*array); > + struct dma_fence *fence; > > /* Allocate the callback structures behind the array. */ > size += num_fences * sizeof(struct dma_fence_array_cb); > @@ -165,8 +166,9 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > if (!array) > return NULL; > > + fence = &array->base; > spin_lock_init(&array->lock); > - dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, > + dma_fence_init(fence, &dma_fence_array_ops, &array->lock, > context, seqno); > init_irq_work(&array->work, irq_dma_fence_array_work); > > @@ -174,7 +176,10 @@ struct dma_fence_array *dma_fence_array_create(int num_fences, > atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); > array->fences = fences; > > - array->base.error = PENDING_ERROR; > + fence->error = PENDING_ERROR; > + > + if (signal_on_any) > + dma_fence_enable_sw_signaling(fence); > > return array; > } > diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c > index 1b4cb3e5cec9..0518e53880f6 100644 > --- a/drivers/dma-buf/dma-fence-chain.c > +++ b/drivers/dma-buf/dma-fence-chain.c > @@ -152,7 +152,8 @@ static bool dma_fence_chain_enable_signaling(struct dma_fence *fence) > struct dma_fence *f = chain ? chain->fence : fence; > > dma_fence_get(f); > - if (!dma_fence_add_callback(f, &head->cb, dma_fence_chain_cb)) { > + if (!dma_fence_add_callback_nested(f, &head->cb, > + dma_fence_chain_cb)) { > dma_fence_put(fence); > return true; > } > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 066400ed8841..90a3d5121746 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -610,6 +610,37 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) > } > EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > > +static int __dma_fence_add_callback(struct dma_fence *fence, > + struct dma_fence_cb *cb, > + dma_fence_func_t func, > + int nest_level) > +{ > + unsigned long flags; > + int ret = 0; > + > + if (WARN_ON(!fence || !func)) > + return -EINVAL; > + > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > + INIT_LIST_HEAD(&cb->node); > + return -ENOENT; > + } > + > + spin_lock_irqsave_nested(fence->lock, flags, 0); Forgot to hook up nest_level here? > + > + if (__dma_fence_enable_signaling(fence)) { > + cb->func = func; > + list_add_tail(&cb->node, &fence->cb_list); > + } else { > + INIT_LIST_HEAD(&cb->node); > + ret = -ENOENT; > + } > + > + spin_unlock_irqrestore(fence->lock, flags); > + > + return ret; > +} > + > /** > * dma_fence_add_callback - add a callback to be called when the fence > * is signaled > @@ -635,33 +666,33 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, > dma_fence_func_t func) > { > - unsigned long flags; > - int ret = 0; > - > - if (WARN_ON(!fence || !func)) > - return -EINVAL; > - > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > - INIT_LIST_HEAD(&cb->node); > - return -ENOENT; > - } > - > - spin_lock_irqsave(fence->lock, flags); > - > - if (__dma_fence_enable_signaling(fence)) { > - cb->func = func; > - list_add_tail(&cb->node, &fence->cb_list); > - } else { > - INIT_LIST_HEAD(&cb->node); > - ret = -ENOENT; > - } > - > - spin_unlock_irqrestore(fence->lock, flags); > - > - return ret; > + return __dma_fence_add_callback(fence, cb, func, 0); > } > EXPORT_SYMBOL(dma_fence_add_callback); > Other than that, I didn't investigate the nesting fails enough to say I can accurately review this. :) ~Maarten

3 years, 6 months

1
0
0 0

completely rework the dma_resv semantic

by Christian König

Hi everyone, compared to the last version I've dropped the pruning as suggested by Maarten, split the new DMA_RESV_USAGE_* patches from the general introduction as suggeted by Daniel and renamed OTEHRS to BOOKKEEP as suggested by Pekka. Please take a look and review, Christian.

3 years, 6 months

2
30
0 0

Re: [Linaro-mm-sig] [PATCH] dma_fence_array: Fix PENDING_ERROR leak in dma_fence_array_signaled()

by Christian König

Am 29.11.21 um 13:46 schrieb Thomas Hellström: > On Mon, 2021-11-29 at 13:33 +0100, Christian König wrote: >> Am 29.11.21 um 13:23 schrieb Thomas Hellström: >>> Hi, Christian, >>> >>> On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: >>>> Am 29.11.21 um 08:35 schrieb Thomas Hellström: >>>>> If a dma_fence_array is reported signaled by a call to >>>>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status. >>>>> >>>>> Fix this by clearing the PENDING_ERROR status if we return true >>>>> in >>>>> dma_fence_array_signaled(). >>>>> >>>>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- >>>>> array container") >>>>> Cc: linaro-mm-sig(a)lists.linaro.org >>>>> Cc: Christian König <ckoenig.leichtzumerken(a)gmail.com> >>>>> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> >>>>> Signed-off-by: Thomas Hellström >>>>> <thomas.hellstrom(a)linux.intel.com> >>>> Reviewed-by: Christian König <christian.koenig(a)amd.com> >>> How are the dma-buf / dma-fence patches typically merged? If i915 >>> is >>> the only fence->error user, could we take this through drm-intel to >>> avoid a backmerge for upcoming i915 work? >> Well that one here looks like a bugfix to me, so either through >> drm-misc-fixes ore some i915 -fixes branch sounds fine to me. >> >> If you have any new development based on that a backmerge of the - >> fixes >> into your -next branch is unavoidable anyway. > Ok, I'll check with Joonas if I can take it through > drm-intel-gt-next, since fixes are cherry-picked from that one. Patch > will then appear in both the -fixes and the -next branch. Well exactly that's the stuff Daniel told me to avoid :) But maybe your i915 workflow is somehow better handling that than the AMD workflow. Christian. > > Thanks, > /Thomas > > >> Regards, >> Christian. >> >>> /Thomas >>> >>> >>>>> --- >>>>> drivers/dma-buf/dma-fence-array.c | 6 +++++- >>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- >>>>> buf/dma-fence-array.c >>>>> index d3fbd950be94..3e07f961e2f3 100644 >>>>> --- a/drivers/dma-buf/dma-fence-array.c >>>>> +++ b/drivers/dma-buf/dma-fence-array.c >>>>> @@ -104,7 +104,11 @@ static bool >>>>> dma_fence_array_signaled(struct >>>>> dma_fence *fence) >>>>> { >>>>> struct dma_fence_array *array = >>>>> to_dma_fence_array(fence); >>>>> >>>>> - return atomic_read(&array->num_pending) <= 0; >>>>> + if (atomic_read(&array->num_pending) > 0) >>>>> + return false; >>>>> + >>>>> + dma_fence_array_clear_pending_error(array); >>>>> + return true; >>>>> } >>>>> >>>>> static void dma_fence_array_release(struct dma_fence *fence) >

3 years, 6 months

3
2
0 0

Re: [Linaro-mm-sig] [PATCH 22/26] dma-buf: add enum dma_resv_usage

by Daniel Vetter

On Tue, Nov 23, 2021 at 03:21:07PM +0100, Christian König wrote: > This change adds the dma_resv_usage enum and allows us to specify why a > dma_resv object is queried for its containing fences. > > Additional to that a dma_resv_usage_rw() helper function is added to aid > retrieving the fences for a read or write userspace submission. > > This is then deployed to the different query functions of the dma_resv > object and all of their users. > > Signed-off-by: Christian König <christian.koenig(a)amd.com> Just a few comments on the kenreldoc while I scroll through. > EXPORT_SYMBOL(ib_umem_dmabuf_map_pages); > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > index 062571c04bca..37552935bca6 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -49,6 +49,86 @@ extern struct ww_class reservation_ww_class; > > struct dma_resv_list; > > +/** > + * enum dma_resv_usage - how the fences from a dma_resv obj are used > + * > + * This enum describes the different use cases for a dma_resv object and > + * controls which fences are returned when queried. > + * > + * An important fact is that there is the order KERNEL<WRITE<READ<OTHER and > + * when the dma_resv object is asked for fences for one use case the fences > + * for the lower use case are returned as well. Might be good to replicate this to all functions that take a dma_resv_usage flag, and then also add a "See enum dma_resv_usage for more information." so we get a clickable hyperlink too. > + * > + * For example when asking for WRITE fences then the KERNEL fences are returned > + * as well. Similar when asked for READ fences then both WRITE and KERNEL > + * fences are returned as well. > + */ > +enum dma_resv_usage { > + /** > + * @DMA_RESV_USAGE_KERNEL: For in kernel memory management only. > + * > + * This should only be used for things like copying or clearing memory > + * with a DMA hardware engine for the purpose of kernel memory > + * management. > + * > + * Drivers *always* need to wait for those fences before accessing the > + * resource protected by the dma_resv object. The only exception for > + * that is when the resource is known to be locked down in place by > + * pinning it previously. Should dma_buf_pin also do the wait for kernel fences? I think that would also ba e bit clearer semantics in the dma-buf patch which does these waits for us. Or should dma_buf_pin be pipelined and it's up to callers to wait? For kms that's definitely the semantics we want, but it's a bit playing with fire situation, so maybe dma-buf should get the more idiot proof semantics? > + */ > + DMA_RESV_USAGE_KERNEL, > + > + /** > + * @DMA_RESV_USAGE_WRITE: Implicit write synchronization. > + * > + * This should only be used for userspace command submissions which add > + * an implicit write dependency. > + */ > + DMA_RESV_USAGE_WRITE, > + > + /** > + * @DMA_RESV_USAGE_READ: Implicit read synchronization. > + * > + * This should only be used for userspace command submissions which add > + * an implicit read dependency. > + */ > + DMA_RESV_USAGE_READ, > + > + /** > + * @DMA_RESV_USAGE_OTHER: No implicit sync. > + * > + * This should be used for operations which don't want to add an > + * implicit dependency at all, but still have a dependency on memory > + * management. > + * > + * This might include things like preemption fences as well as device > + * page table updates or even userspace command submissions. I think we should highlight a bit more that for explicitly synchronized userspace like vk OTHER is the normal case. So really not an exception. Ofc aside from amdkgf there's currently no driver doing this, but really we should have lots of them ... > + * > + * The kernel memory management *always* need to wait for those fences > + * before moving or freeing the resource protected by the dma_resv > + * object. > + */ > + DMA_RESV_USAGE_OTHER > +}; > + > +/** > + * dma_resv_usage_rw - helper for implicit sync > + * @write: true if we create a new implicit sync write > + * > + * This returns the implicit synchronization usage for write or read accesses. Pls add "See enum dma_resv_usage for more details." or so. Never hurts to be plentiful with links :-) > + */ > +static inline enum dma_resv_usage dma_resv_usage_rw(bool write) > +{ > + /* This looks confusing at first sight, but is indeed correct. > + * > + * The rational is that new write operations needs to wait for the > + * existing read and write operations to finish. > + * But a new read operation only needs to wait for the existing write > + * operations to finish. > + */ > + return write ? DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE; > +} > + > /** > * struct dma_resv - a reservation object manages fences for a buffer > * > @@ -147,8 +227,8 @@ struct dma_resv_iter { > /** @obj: The dma_resv object we iterate over */ > struct dma_resv *obj; > > - /** @all_fences: If all fences should be returned */ > - bool all_fences; > + /** @usage: Controls which fences are returned */ > + enum dma_resv_usage usage; > > /** @fence: the currently handled fence */ > struct dma_fence *fence; > @@ -178,14 +258,14 @@ struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor); > * dma_resv_iter_begin - initialize a dma_resv_iter object > * @cursor: The dma_resv_iter object to initialize > * @obj: The dma_resv object which we want to iterate over > - * @all_fences: If all fences should be returned or just the exclusive one > + * @usage: controls which fences to return Please add the blurb here I mentioned above. Maybe adjust the text to use the neatly highlighted @usage. > */ > static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor, > struct dma_resv *obj, > - bool all_fences) > + enum dma_resv_usage usage) > { > cursor->obj = obj; > - cursor->all_fences = all_fences; > + cursor->usage = usage; > cursor->fence = NULL; > } > > @@ -242,7 +322,7 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) > * dma_resv_for_each_fence - fence iterator > * @cursor: a struct dma_resv_iter pointer > * @obj: a dma_resv object pointer > - * @all_fences: true if all fences should be returned > + * @usage: controls which fences to return > * @fence: the current fence > * Same, another place that needs the @usage clarification. > * Iterate over the fences in a struct dma_resv object while holding the > @@ -251,8 +331,8 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) > * valid as long as the lock is held and so no extra reference to the fence is > * taken. > */ > -#define dma_resv_for_each_fence(cursor, obj, all_fences, fence) \ > - for (dma_resv_iter_begin(cursor, obj, all_fences), \ > +#define dma_resv_for_each_fence(cursor, obj, usage, fence) \ > + for (dma_resv_iter_begin(cursor, obj, usage), \ > fence = dma_resv_iter_first(cursor); fence; \ > fence = dma_resv_iter_next(cursor)) > > @@ -421,14 +501,14 @@ void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context, > void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); > void dma_resv_prune(struct dma_resv *obj); > void dma_resv_prune_unlocked(struct dma_resv *obj); > -int dma_resv_get_fences(struct dma_resv *obj, bool write, > +int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage, > unsigned int *num_fences, struct dma_fence ***fences); > -int dma_resv_get_singleton(struct dma_resv *obj, bool write, > +int dma_resv_get_singleton(struct dma_resv *obj, enum dma_resv_usage usage, > struct dma_fence **fence); > int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); > -long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr, > - unsigned long timeout); > -bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all); > +long dma_resv_wait_timeout(struct dma_resv *obj, enum dma_resv_usage usage, > + bool intr, unsigned long timeout); > +bool dma_resv_test_signaled(struct dma_resv *obj, enum dma_resv_usage usage); > void dma_resv_describe(struct dma_resv *obj, struct seq_file *seq); I took endless amounts of discussions, but I think we're arriving at something really neat and tiny here now finally. Both semantics, and how drivers use them. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 6 months

2
2
0 0

Re: [Linaro-mm-sig] [PATCH] dma_fence_array: Fix PENDING_ERROR leak in dma_fence_array_signaled()

by Christian König

Am 29.11.21 um 13:23 schrieb Thomas Hellström: > Hi, Christian, > > On Mon, 2021-11-29 at 09:21 +0100, Christian König wrote: >> Am 29.11.21 um 08:35 schrieb Thomas Hellström: >>> If a dma_fence_array is reported signaled by a call to >>> dma_fence_is_signaled(), it may leak the PENDING_ERROR status. >>> >>> Fix this by clearing the PENDING_ERROR status if we return true in >>> dma_fence_array_signaled(). >>> >>> Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence- >>> array container") >>> Cc: linaro-mm-sig(a)lists.linaro.org >>> Cc: Christian König <ckoenig.leichtzumerken(a)gmail.com> >>> Cc: Chris Wilson <chris(a)chris-wilson.co.uk> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom(a)linux.intel.com> >> Reviewed-by: Christian König <christian.koenig(a)amd.com> > How are the dma-buf / dma-fence patches typically merged? If i915 is > the only fence->error user, could we take this through drm-intel to > avoid a backmerge for upcoming i915 work? Well that one here looks like a bugfix to me, so either through drm-misc-fixes ore some i915 -fixes branch sounds fine to me. If you have any new development based on that a backmerge of the -fixes into your -next branch is unavoidable anyway. Regards, Christian. > > /Thomas > > >>> --- >>> drivers/dma-buf/dma-fence-array.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma- >>> buf/dma-fence-array.c >>> index d3fbd950be94..3e07f961e2f3 100644 >>> --- a/drivers/dma-buf/dma-fence-array.c >>> +++ b/drivers/dma-buf/dma-fence-array.c >>> @@ -104,7 +104,11 @@ static bool dma_fence_array_signaled(struct >>> dma_fence *fence) >>> { >>> struct dma_fence_array *array = to_dma_fence_array(fence); >>> >>> - return atomic_read(&array->num_pending) <= 0; >>> + if (atomic_read(&array->num_pending) > 0) >>> + return false; >>> + >>> + dma_fence_array_clear_pending_error(array); >>> + return true; >>> } >>> >>> static void dma_fence_array_release(struct dma_fence *fence) >

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma_fence_array: Fix PENDING_ERROR leak in dma_fence_array_signaled()

by Christian König

Am 29.11.21 um 08:35 schrieb Thomas Hellström: > If a dma_fence_array is reported signaled by a call to > dma_fence_is_signaled(), it may leak the PENDING_ERROR status. > > Fix this by clearing the PENDING_ERROR status if we return true in > dma_fence_array_signaled(). > > Fixes: 1f70b8b812f3 ("dma-fence: Propagate errors to dma-fence-array container") > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: Christian König <ckoenig.leichtzumerken(a)gmail.com> > Cc: Chris Wilson <chris(a)chris-wilson.co.uk> > Signed-off-by: Thomas Hellström <thomas.hellstrom(a)linux.intel.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/dma-fence-array.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index d3fbd950be94..3e07f961e2f3 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -104,7 +104,11 @@ static bool dma_fence_array_signaled(struct dma_fence *fence) > { > struct dma_fence_array *array = to_dma_fence_array(fence); > > - return atomic_read(&array->num_pending) <= 0; > + if (atomic_read(&array->num_pending) > 0) > + return false; > + > + dma_fence_array_clear_pending_error(array); > + return true; > } > > static void dma_fence_array_release(struct dma_fence *fence)

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 11/15] iio: buffer-dma: Boost performance using write-combine cache setting

by Lars-Peter Clausen

On 11/27/21 5:05 PM, Jonathan Cameron wrote: >> Non-coherent mapping with no cache sync: >> - fileio: >> read: 156 MiB/s >> write: 123 MiB/s >> - dmabuf: >> read: 234 MiB/s (capped by sample rate) >> write: 182 MiB/s >> >> Non-coherent reads with no cache sync + write-combine writes: >> - fileio: >> read: 156 MiB/s >> write: 140 MiB/s >> - dmabuf: >> read: 234 MiB/s (capped by sample rate) >> write: 210 MiB/s >> >> >> A few things we can deduce from this: >> >> * Write-combine is not available on Zynq/ARM? If it was working, it >> should give a better performance than the coherent mapping, but it >> doesn't seem to do anything at all. At least it doesn't harm >> performance. > I'm not sure it's very relevant to this sort of streaming write. > If you write a sequence of addresses then nothing stops them getting combined > into a single write whether or not it is write-combining. There is a difference at which point they can get combined. With write-combine they can be coalesced into a single transaction anywhere in the interconnect, as early as the CPU itself. Without write-cobmine the DDR controller might decide to combine them, but not earlier. This can make a difference especially if the write is a narrow write, i.e. the access size is smaller than the buswidth. Lets say you do 32-bit writes, but your bus is 64 bits wide. With WC two 32-bits can be combined into a 64-bit write. Without WC that is not possible and you are potentially not using the bus to its fullest capacity. This is especially true if the memory bus is wider than the widest access size of the CPU.

3 years, 6 months

1
0
0 0

completely rework the dma_resv semantic

by Christian König

Hi guys, as discussed before this set of patches completely rework the dma_resv semantic and spreads the new handling over all the existing drivers and users. First of all this drops the DAG approach because it requires that every single driver implements those relatively complicated rules correctly and any violation of that immediately leads to either corruption of freed memory or even more severe security problems. Instead we just keep all fences around all the time until they are signaled. Only fences with the same context are assumed to be signaled in the correct order since this is exercised elsewhere as well. Replacing fences is now only supported for hardware mechanism like VM page table updates where the hardware can guarantee that the resource can't be accessed any more. Then the concept of a single exclusive fence and multiple shared fences is dropped as well. Instead the dma_resv object is now just a container for dma_fence objects where each fence has associated usage flags. Those use flags describe how the operation represented by the dma_fence object is using the resource protected by the dma_resv object. This allows us to add multiple fences for each usage type. Additionally to the existing WRITE/READ usages this patch set also adds the new KERNEL and OTHER usages. The KERNEL usages is used in cases where the kernel needs to do some operation with the resource protected by the dma_resv object, like copies or clears. Those are mandatory to wait for when dynamic memory management is used. The OTHER usage is for cases where we don't want that the operation represented by the dma_fence object participate in any implicit sync but needs to be respected by the kernel memory management. Examples for those are VM page table updates and preemption fences. While doing this the new implementation cleans up existing workarounds all over the place, but especially amdgpu and TTM. Surprisingly I also found two use cases for the KERNEL/OTHER usage in i915 and Nouveau, those might need more thoughts. In general the existing functionality should been preserved, the only downside is that we now always need to reserve a slot before adding a fence. The newly added call to the reservation function can probably use some more cleanup. TODOs: Testing, testing, testing, doublechecking the newly added kerneldoc for any typos. Please review and/or comment, Christian.

3 years, 6 months

3
30
0 0

Re: [Linaro-mm-sig] [PATCH v4] dma-buf: system_heap: Use 'for_each_sgtable_sg' in pages free flow

by Christian König

Am 26.11.21 um 08:49 schrieb guangming.cao(a)mediatek.com: > From: Guangming <Guangming.Cao(a)mediatek.com> > > For previous version, it uses 'sg_table.nent's to traverse sg_table in pages > free flow. > However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of > created entries in the DMA adderess space. > So, use 'sg_table.nents' in pages free flow will case some pages can't be freed. > > Here we should use sg_table.orig_nents to free pages memory, but use the > sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common > helper 'for_each_sg' which maybe cause memory leak) is much better. > > Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available") > Signed-off-by: Guangming <Guangming.Cao(a)mediatek.com> > Reviewed-by: Robin Murphy <robin.murphy(a)arm.com> Reviewed-by: Christian König <christian.koenig(a)amd.com> > Cc: <stable(a)vger.kernel.org> # 5.11.* > --- > v4: Correct commit message > 1. Cc stable(a)vger.kernel.org in commit message and add required kernel version. > 2. Add reviewed-by since patch V2 and V4 are same and V2 is reviewed by Robin. > 3. There is no new code change in V4. > V3: Cc stable(a)vger.kernel.org > 1. This patch needs to be merged stable branch, add stable(a)vger.kernel.org > in mail list. > 2. Correct some spelling mistake. > 3. There is No new code change in V3. > V2: use 'for_each_sgtable_sg' to 'replece for_each_sg' as suggested by Robin. > > --- > drivers/dma-buf/heaps/system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..8660508f3684 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) > int i; > > table = &buffer->sg_table; > - for_each_sg(table->sgl, sg, table->nents, i) { > + for_each_sgtable_sg(table, sg, i) { > struct page *page = sg_page(sg); > > __free_pages(page, compound_order(page));

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3] dma-buf: system_heap: Use 'for_each_sgtable_sg' in pages free flow

by Greg KH

On Fri, Nov 26, 2021 at 11:16:05AM +0800, guangming.cao(a)mediatek.com wrote: > From: Guangming <Guangming.Cao(a)mediatek.com> > > For previous version, it uses 'sg_table.nent's to traverse sg_table in pages > free flow. > However, 'sg_table.nents' is reassigned in 'dma_map_sg', it means the number of > created entries in the DMA adderess space. > So, use 'sg_table.nents' in pages free flow will case some pages can't be freed. > > Here we should use sg_table.orig_nents to free pages memory, but use the > sgtable helper 'for each_sgtable_sg'(, instead of the previous rather common > helper 'for_each_sg' which maybe cause memory leak) is much better. > > Fixes: d963ab0f15fb0 ("dma-buf: system_heap: Allocate higher order pages if available") > > Signed-off-by: Guangming <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/heaps/system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..8660508f3684 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) > int i; > > table = &buffer->sg_table; > - for_each_sg(table->sgl, sg, table->nents, i) { > + for_each_sgtable_sg(table, sg, i) { > struct page *page = sg_page(sg); > > __free_pages(page, compound_order(page)); > -- > 2.17.1 > <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter>

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4] i2c: tegra: Add the ACPI support

by Dmitry Osipenko

25.11.2021 19:53, Akhil R пишет: > Add support for the ACPI based device registration so that the driver > can be also enabled through ACPI table. > > This does not include the ACPI support for Tegra VI and DVC I2C. > > Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 12 deletions(-) > > v4 changes: > * changed has_acpi_companion to ACPI_HANDLE for consistency across > all functions > v3 - https://lkml.org/lkml/2021/11/25/173 > v2 - https://lkml.org/lkml/2021/11/23/82 > v1 - https://lkml.org/lkml/2021/11/19/393 Andy suggested to make v5 with extra patch that will make use of the generic i2c_timings, but it should be a separate change. This patch is good to me. Please provide the full changelog for each version next time, instead of the links. Thanks! If you'll make v5, then feel free to add my r-b: Reviewed-by: Dmitry Osipenko <digetx(a)gmail.com>

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v4] i2c: tegra: Add the ACPI support

by Dmitry Osipenko

25.11.2021 22:28, Andy Shevchenko пишет: >> - err = reset_control_reset(i2c_dev->rst); >> + if (handle) >> + err = acpi_evaluate_object(handle, "_RST", NULL, NULL); > Does it compile for CONFIG_ACPI=n case? > It compiles and works fine with CONFIG_ACPI=n.

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] dma_heap: use for_each_sgtable_sg in sg_table release flow

by Robin Murphy

On 2021-11-25 13:49, guangming.cao(a)mediatek.com wrote: > From: Guangming <Guangming.Cao(a)mediatek.com> > > Use (for_each_sgtable_sg) rather than (for_each_sg) to traverse > sg_table to free sg_table. > Use (for_each_sg) maybe will casuse some pages can't be freed > when send wrong nents number. It's still worth spelling out that this is fixing a bug where the current code should have been using table->orig_nents - it's just that switching to the sgtable helper is the best way to make the fix, since it almost entirely removes the possibility of making that (previously rather common) mistake. If it helps, for the change itself: Reviewed-by: Robin Murphy <robin.murphy(a)arm.com> Thanks, Robin. > Signed-off-by: Guangming <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/heaps/system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..8660508f3684 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) > int i; > > table = &buffer->sg_table; > - for_each_sg(table->sgl, sg, table->nents, i) { > + for_each_sgtable_sg(table, sg, i) { > struct page *page = sg_page(sg); > > __free_pages(page, compound_order(page)); >

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma_heap: use sg_table.orig_nents in sg_table release flow

by Robin Murphy

On 2021-11-25 12:46, guangming.cao(a)mediatek.com wrote: > From: Guangming <Guangming.Cao(a)mediatek.com> > > Use (sg_table.orig_nents) rather than (sg_table.nents) to traverse > sg_table to free sg_table. > Use (sg_table.nents) maybe will casuse some pages can't be freed. ...and this sort of bug is precisely why we have the for_each_sgtable_sg() helper ;) Robin. > Signed-off-by: Guangming <Guangming.Cao(a)mediatek.com> > --- > drivers/dma-buf/heaps/system_heap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..ce10d4eb674c 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -289,7 +289,7 @@ static void system_heap_dma_buf_release(struct dma_buf *dmabuf) > int i; > > table = &buffer->sg_table; > - for_each_sg(table->sgl, sg, table->nents, i) { > + for_each_sg(table->sgl, sg, table->orig_nents, i) { > struct page *page = sg_page(sg); > > __free_pages(page, compound_order(page)); >

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3] i2c: tegra: Add the ACPI support

by Dmitry Osipenko

25.11.2021 12:07, Akhil R пишет: > Add support for the ACPI based device registration so that the driver > can be also enabled through ACPI table. > > This does not include the ACPI support for Tegra VI and DVC I2C. > > Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 12 deletions(-) > > v3 changes > * removed acpi_has_method check. > * moved dev_err_probe to init_reset function to make it consistent with > init_clocks. > * Updates in commit message as suggested. > > v2 - https://lkml.org/lkml/2021/11/23/82 > v1 - https://lkml.org/lkml/2021/11/19/393 Akhil, the patch looks almost good, thank you. Please see one minor question below. > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index c883044..b889eb3 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -6,6 +6,7 @@ > * Author: Colin Cross <ccross(a)android.com> > */ > > +#include <linux/acpi.h> > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > @@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > { > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > + acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); ... > +static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev) > +{ > + if (has_acpi_companion(i2c_dev->dev)) > + return 0; Can we use ACPI_HANDLE() everywhere instead of has_acpi_companion()? For consistency. I guess that's what Andy was asking about in v1?

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by Dmitry Osipenko

24.11.2021 19:40, Akhil R пишет: >> 24.11.2021 10:18, Akhil R пишет: >>>> *i2c_dev) >>>>> i2c_dev->is_vi = true; } >>>> How are you going to differentiate the VI I2C from a non-VI? This >>>> doesn't look right. >>> This patch adds the ACPI support to only non-VI I2C. The device_ids in >>> match table are added accordingly. I suppose, of_device_is_compatible >>> always returns false as there is no device tree. >>> Agree with the other comments. >> >> Will the VI I2C have a different ACPI ID or how it's going to work? > As there is a different compatible for VI I2C in device tree, I suppose the ACPI > would have a different ID as well. I think the logic would also need an update > if to have VI I2C using the ACPI. But that wasn't actually considered in this patch. Thanks, you could reflected it in the commit message.

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by Dmitry Osipenko

24.11.2021 10:18, Akhil R пишет: >> *i2c_dev) >>> i2c_dev->is_vi = true; >>> } >> How are you going to differentiate the VI I2C from a non-VI? This doesn't look >> right. > This patch adds the ACPI support to only non-VI I2C. The device_ids in match table > are added accordingly. I suppose, of_device_is_compatible always returns false as > there is no device tree. > Agree with the other comments. Will the VI I2C have a different ACPI ID or how it's going to work?

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by kernel test robot

Hi Akhil, Thank you for the patch! Yet something to improve: [auto build test ERROR on tegra/for-next] [also build test ERROR on v5.16-rc2 next-20211123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support… base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next config: riscv-buildonly-randconfig-r005-20211123 (https://download.01.org/0day-ci/archive/20211124/202111240017.bYyZ7knz-lkp@…) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 49e3838145dff1ec91c2e67a2cb562775c8d2a08) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25… git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636 git checkout dec174be801f41a9e42f4381c59c2357c25e40fb # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp(a)intel.com> All errors (new ones prefixed by >>): In file included from drivers/i2c/busses/i2c-tegra.c:13: In file included from include/linux/dmaengine.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __raw_readb(PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) ^ In file included from drivers/i2c/busses/i2c-tegra.c:13: In file included from include/linux/dmaengine.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) ^ In file included from drivers/i2c/busses/i2c-tegra.c:13: In file included from include/linux/dmaengine.h:12: In file included from include/linux/scatterlist.h:9: In file included from arch/riscv/include/asm/io.h:136: include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writeb(value, PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); ~~~~~~~~~~ ^ include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; ~~~~~~~~~~ ^ >> drivers/i2c/busses/i2c-tegra.c:623:16: error: implicit declaration of function 'acpi_has_method' [-Werror,-Wimplicit-function-declaration] if (handle && acpi_has_method(handle, "_RST")) ^ drivers/i2c/busses/i2c-tegra.c:623:16: note: did you mean 'acpi_has_watchdog'? include/linux/acpi.h:1321:20: note: 'acpi_has_watchdog' declared here static inline bool acpi_has_watchdog(void) { return false; } ^ 7 warnings and 1 error generated. vim +/acpi_has_method +623 drivers/i2c/busses/i2c-tegra.c 608 609 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) 610 { 611 u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; 612 acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); 613 int err; 614 615 /* 616 * The reset shouldn't ever fail in practice. The failure will be a 617 * sign of a severe problem that needs to be resolved. Still we don't 618 * want to fail the initialization completely because this may break 619 * kernel boot up since voltage regulators use I2C. Hence, we will 620 * emit a noisy warning on error, which won't stay unnoticed and 621 * won't hose machine entirely. 622 */ > 623 if (handle && acpi_has_method(handle, "_RST")) 624 err = (acpi_evaluate_object(handle, "_RST", NULL, NULL)); 625 else 626 err = reset_control_reset(i2c_dev->rst); 627 628 WARN_ON_ONCE(err); 629 630 if (i2c_dev->is_dvc) 631 tegra_dvc_init(i2c_dev); 632 633 val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN | 634 FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2); 635 636 if (i2c_dev->hw->has_multi_master_mode) 637 val |= I2C_CNFG_MULTI_MASTER_MODE; 638 639 i2c_writel(i2c_dev, val, I2C_CNFG); 640 i2c_writel(i2c_dev, 0, I2C_INT_MASK); 641 642 if (i2c_dev->is_vi) 643 tegra_i2c_vi_init(i2c_dev); 644 645 switch (i2c_dev->bus_clk_rate) { 646 case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ: 647 default: 648 tlow = i2c_dev->hw->tlow_fast_fastplus_mode; 649 thigh = i2c_dev->hw->thigh_fast_fastplus_mode; 650 tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode; 651 652 if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ) 653 non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode; 654 else 655 non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode; 656 break; 657 658 case 0 ... I2C_MAX_STANDARD_MODE_FREQ: 659 tlow = i2c_dev->hw->tlow_std_mode; 660 thigh = i2c_dev->hw->thigh_std_mode; 661 tsu_thd = i2c_dev->hw->setup_hold_time_std_mode; 662 non_hs_mode = i2c_dev->hw->clk_divisor_std_mode; 663 break; 664 } 665 666 /* make sure clock divisor programmed correctly */ 667 clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE, 668 i2c_dev->hw->clk_divisor_hs_mode) | 669 FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode); 670 i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR); 671 672 if (i2c_dev->hw->has_interface_timing_reg) { 673 val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) | 674 FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow); 675 i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0); 676 } 677 678 /* 679 * Configure setup and hold times only when tsu_thd is non-zero. 680 * Otherwise, preserve the chip default values. 681 */ 682 if (i2c_dev->hw->has_interface_timing_reg && tsu_thd) 683 i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1); 684 685 clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1); 686 687 err = clk_set_rate(i2c_dev->div_clk, 688 i2c_dev->bus_clk_rate * clk_multiplier); 689 if (err) { 690 dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err); 691 return err; 692 } 693 694 if (!i2c_dev->is_dvc && !i2c_dev->is_vi) { 695 u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG); 696 697 sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL; 698 i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG); 699 i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1); 700 i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2); 701 } 702 703 err = tegra_i2c_flush_fifos(i2c_dev); 704 if (err) 705 return err; 706 707 if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg) 708 i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE); 709 710 err = tegra_i2c_wait_for_config_load(i2c_dev); 711 if (err) 712 return err; 713 714 return 0; 715 } 716 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by Dmitry Osipenko

23.11.2021 10:15, Akhil R пишет: > Add support for ACPI based device registration so that the driver > can be also enabled through ACPI table. > > Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 40 insertions(+), 12 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index c883044..8e47889 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -6,6 +6,7 @@ > * Author: Colin Cross <ccross(a)android.com> > */ > > +#include <linux/acpi.h> > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/delay.h> > @@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev) > static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > { > u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; > + acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); > int err; > > /* > @@ -618,7 +620,11 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) > * emit a noisy warning on error, which won't stay unnoticed and > * won't hose machine entirely. > */ > - err = reset_control_reset(i2c_dev->rst); > + if (handle && acpi_has_method(handle, "_RST")) Which SoC version doesn't have "_RST" method? If neither, then please remove this check. > + err = (acpi_evaluate_object(handle, "_RST", NULL, NULL)); Please remove parens around acpi_evaluate_object(). Why you added them? > + else > + err = reset_control_reset(i2c_dev->rst); > + > WARN_ON_ONCE(err); > > if (i2c_dev->is_dvc) > @@ -1627,12 +1633,12 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) > bool multi_mode; > int err; > > - err = of_property_read_u32(np, "clock-frequency", > - &i2c_dev->bus_clk_rate); > + err = device_property_read_u32(i2c_dev->dev, "clock-frequency", > + &i2c_dev->bus_clk_rate); > if (err) > i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ; > > - multi_mode = of_property_read_bool(np, "multi-master"); > + multi_mode = device_property_read_bool(i2c_dev->dev, "multi-master"); > i2c_dev->multimaster_mode = multi_mode; > > if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc")) > @@ -1642,10 +1648,25 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev) > i2c_dev->is_vi = true; > } How are you going to differentiate the VI I2C from a non-VI? This doesn't look right. > > +static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev) > +{ > + if (has_acpi_companion(i2c_dev->dev)) > + return 0; > + > + i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > + if (IS_ERR(i2c_dev->rst)) > + return PTR_ERR(i2c_dev->rst); > + > + return 0; > +} > + > static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev) > { > int err; > > + if (has_acpi_companion(i2c_dev->dev)) > + return 0; > + > i2c_dev->clocks[i2c_dev->nclocks++].id = "div-clk"; > > if (i2c_dev->hw == &tegra20_i2c_hw || i2c_dev->hw == &tegra30_i2c_hw) > @@ -1720,7 +1741,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) > init_completion(&i2c_dev->msg_complete); > init_completion(&i2c_dev->dma_complete); > > - i2c_dev->hw = of_device_get_match_data(&pdev->dev); > + i2c_dev->hw = device_get_match_data(&pdev->dev); > i2c_dev->cont_id = pdev->id; > i2c_dev->dev = &pdev->dev; > > @@ -1746,15 +1767,13 @@ static int tegra_i2c_probe(struct platform_device *pdev) > if (err) > return err; > > - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > - if (IS_ERR(i2c_dev->rst)) { > - dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), > - "failed to get reset control\n"); > - return PTR_ERR(i2c_dev->rst); > - } > - > tegra_i2c_parse_dt(i2c_dev); > > + err = tegra_i2c_init_reset(i2c_dev); > + if (err) > + return dev_err_probe(i2c_dev->dev, err, > + "failed to get reset control\n"); This is inconsistent with tegra_i2c_init_clocks() which returns err directly and prints error message within the function. Please move the dev_err_probe() into tegra_i2c_init_reset() to make it consistent, like I suggested before. Please don't reply with a new version of the patch to the old thread, always send a new version separately. Otherwise it's more difficult to follow patches. Lastly, each new version of a patch must contain changelog. I don't see changelog here, please add it to v3 after the " ---" separator of the commit message. Thanks! Example: Commit message. Signed-off-by: Akhil R <akhilrajeev(a)nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 12 deletions(-) Changelog: v3: bla-bla-bla v2: bla-bla-bla diff ...

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v2] i2c: tegra: Add ACPI support

by kernel test robot

Hi Akhil, Thank you for the patch! Yet something to improve: [auto build test ERROR on tegra/for-next] [also build test ERROR on v5.16-rc2 next-20211123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support… base: https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next config: m68k-randconfig-r011-20211123 (https://download.01.org/0day-ci/archive/20211123/202111232153.mpoejDRV-lkp@…) compiler: m68k-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25… git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636 git checkout dec174be801f41a9e42f4381c59c2357c25e40fb # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp(a)intel.com> All errors (new ones prefixed by >>): drivers/i2c/busses/i2c-tegra.c: In function 'tegra_i2c_init': >> drivers/i2c/busses/i2c-tegra.c:623:23: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Werror=implicit-function-declaration] 623 | if (handle && acpi_has_method(handle, "_RST")) | ^~~~~~~~~~~~~~~ | acpi_has_watchdog cc1: some warnings being treated as errors vim +623 drivers/i2c/busses/i2c-tegra.c 608 609 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) 610 { 611 u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode; 612 acpi_handle handle = ACPI_HANDLE(i2c_dev->dev); 613 int err; 614 615 /* 616 * The reset shouldn't ever fail in practice. The failure will be a 617 * sign of a severe problem that needs to be resolved. Still we don't 618 * want to fail the initialization completely because this may break 619 * kernel boot up since voltage regulators use I2C. Hence, we will 620 * emit a noisy warning on error, which won't stay unnoticed and 621 * won't hose machine entirely. 622 */ > 623 if (handle && acpi_has_method(handle, "_RST")) 624 err = (acpi_evaluate_object(handle, "_RST", NULL, NULL)); 625 else 626 err = reset_control_reset(i2c_dev->rst); 627 628 WARN_ON_ONCE(err); 629 630 if (i2c_dev->is_dvc) 631 tegra_dvc_init(i2c_dev); 632 633 val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN | 634 FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2); 635 636 if (i2c_dev->hw->has_multi_master_mode) 637 val |= I2C_CNFG_MULTI_MASTER_MODE; 638 639 i2c_writel(i2c_dev, val, I2C_CNFG); 640 i2c_writel(i2c_dev, 0, I2C_INT_MASK); 641 642 if (i2c_dev->is_vi) 643 tegra_i2c_vi_init(i2c_dev); 644 645 switch (i2c_dev->bus_clk_rate) { 646 case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ: 647 default: 648 tlow = i2c_dev->hw->tlow_fast_fastplus_mode; 649 thigh = i2c_dev->hw->thigh_fast_fastplus_mode; 650 tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode; 651 652 if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ) 653 non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode; 654 else 655 non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode; 656 break; 657 658 case 0 ... I2C_MAX_STANDARD_MODE_FREQ: 659 tlow = i2c_dev->hw->tlow_std_mode; 660 thigh = i2c_dev->hw->thigh_std_mode; 661 tsu_thd = i2c_dev->hw->setup_hold_time_std_mode; 662 non_hs_mode = i2c_dev->hw->clk_divisor_std_mode; 663 break; 664 } 665 666 /* make sure clock divisor programmed correctly */ 667 clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE, 668 i2c_dev->hw->clk_divisor_hs_mode) | 669 FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode); 670 i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR); 671 672 if (i2c_dev->hw->has_interface_timing_reg) { 673 val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) | 674 FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow); 675 i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0); 676 } 677 678 /* 679 * Configure setup and hold times only when tsu_thd is non-zero. 680 * Otherwise, preserve the chip default values. 681 */ 682 if (i2c_dev->hw->has_interface_timing_reg && tsu_thd) 683 i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1); 684 685 clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1); 686 687 err = clk_set_rate(i2c_dev->div_clk, 688 i2c_dev->bus_clk_rate * clk_multiplier); 689 if (err) { 690 dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err); 691 return err; 692 } 693 694 if (!i2c_dev->is_dvc && !i2c_dev->is_vi) { 695 u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG); 696 697 sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL; 698 i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG); 699 i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1); 700 i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2); 701 } 702 703 err = tegra_i2c_flush_fifos(i2c_dev); 704 if (err) 705 return err; 706 707 if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg) 708 i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE); 709 710 err = tegra_i2c_wait_for_config_load(i2c_dev); 711 if (err) 712 return err; 713 714 return 0; 715 } 716 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/22/21 4:16 PM, Paul Cercueil wrote: > Hi Lars, > > Le lun., nov. 22 2021 at 16:08:51 +0100, Lars-Peter Clausen > <lars(a)metafoo.de> a écrit : >> On 11/21/21 9:08 PM, Paul Cercueil wrote: >>> >>> >>> Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen >>> <lars(a)metafoo.de> a écrit : >>>> On 11/21/21 6:52 PM, Paul Cercueil wrote: >>>>> Hi Lars, >>>>> >>>>> Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen >>>>> <lars(a)metafoo.de> a écrit : >>>>>> On 11/15/21 3:19 PM, Paul Cercueil wrote: >>>>>>> The buffer-dma code was using two queues, incoming and outgoing, to >>>>>>> manage the state of the blocks in use. >>>>>>> >>>>>>> While this totally works, it adds some complexity to the code, >>>>>>> especially since the code only manages 2 blocks. It is much >>>>>>> easier to >>>>>>> just check each block's state manually, and keep a counter for >>>>>>> the next >>>>>>> block to dequeue. >>>>>>> >>>>>>> Since the new DMABUF based API wouldn't use these incoming and >>>>>>> outgoing >>>>>>> queues anyway, getting rid of them now makes the upcoming changes >>>>>>> simpler. >>>>>>> >>>>>>> Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> >>>>>> The outgoing queue is going to be replaced by fences, but I think >>>>>> we need to keep the incoming queue. >>>>> >>>>> Blocks are always accessed in sequential order, so we now have a >>>>> "queue->next_dequeue" that cycles between the buffers >>>>> allocated for fileio. >>>>> >>>>>>> [...] >>>>>>> @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); >>>>>>> static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue >>>>>>> *queue, >>>>>>> struct iio_dma_buffer_block *block) >>>>>>> { >>>>>>> - if (block->state == IIO_BLOCK_STATE_DEAD) { >>>>>>> + if (block->state == IIO_BLOCK_STATE_DEAD) >>>>>>> iio_buffer_block_put(block); >>>>>>> - } else if (queue->active) { >>>>>>> + else if (queue->active) >>>>>>> iio_dma_buffer_submit_block(queue, block); >>>>>>> - } else { >>>>>>> + else >>>>>>> block->state = IIO_BLOCK_STATE_QUEUED; >>>>>>> - list_add_tail(&block->head, &queue->incoming); >>>>>> If iio_dma_buffer_enqueue() is called with a dmabuf and the >>>>>> buffer is not active, it will be marked as queued, but we >>>>>> don't actually keep a reference to it anywhere. It will >>>>>> never be submitted to the DMA, and it will never be >>>>>> signaled as completed. >>>>> >>>>> We do keep a reference to the buffers, in the queue->fileio.blocks >>>>> array. When the buffer is enabled, all the blocks in that >>>>> array that are in the "queued" state will be submitted to the >>>>> DMA. >>>>> >>>> But not when used in combination with the DMA buf changes later in >>>> this series. >>>> >>> >>> That's still the case after the DMABUF changes of the series. Or can >>> you point me exactly what you think is broken? >>> >> When you allocate a DMABUF with the allocate IOCTL and then submit it >> with the enqueue IOCTL before the buffer is enabled it will end up >> marked as queued, but not actually be queued anywhere. >> > > Ok, it works for me because I never enqueue blocks before enabling the > buffer. I can add a requirement that blocks must be enqueued only > after the buffer is enabled. I don't think that is a good idea. This way you are going to potentially drop data at the begining of your stream when the DMA isn't ready yet.

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 01/15] iio: buffer-dma: Get rid of incoming/outgoing queues

by Lars-Peter Clausen

On 11/21/21 9:08 PM, Paul Cercueil wrote: > > > Le dim., nov. 21 2021 at 19:49:03 +0100, Lars-Peter Clausen > <lars(a)metafoo.de> a écrit : >> On 11/21/21 6:52 PM, Paul Cercueil wrote: >>> Hi Lars, >>> >>> Le dim., nov. 21 2021 at 17:23:35 +0100, Lars-Peter Clausen >>> <lars(a)metafoo.de> a écrit : >>>> On 11/15/21 3:19 PM, Paul Cercueil wrote: >>>>> The buffer-dma code was using two queues, incoming and outgoing, to >>>>> manage the state of the blocks in use. >>>>> >>>>> While this totally works, it adds some complexity to the code, >>>>> especially since the code only manages 2 blocks. It is much easier to >>>>> just check each block's state manually, and keep a counter for the >>>>> next >>>>> block to dequeue. >>>>> >>>>> Since the new DMABUF based API wouldn't use these incoming and >>>>> outgoing >>>>> queues anyway, getting rid of them now makes the upcoming changes >>>>> simpler. >>>>> >>>>> Signed-off-by: Paul Cercueil <paul(a)crapouillou.net> >>>> The outgoing queue is going to be replaced by fences, but I think >>>> we need to keep the incoming queue. >>> >>> Blocks are always accessed in sequential order, so we now have a >>> "queue->next_dequeue" that cycles between the buffers allocated for >>> fileio. >>> >>>>> [...] >>>>> @@ -442,28 +435,33 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_disable); >>>>> static void iio_dma_buffer_enqueue(struct iio_dma_buffer_queue >>>>> *queue, >>>>> struct iio_dma_buffer_block *block) >>>>> { >>>>> - if (block->state == IIO_BLOCK_STATE_DEAD) { >>>>> + if (block->state == IIO_BLOCK_STATE_DEAD) >>>>> iio_buffer_block_put(block); >>>>> - } else if (queue->active) { >>>>> + else if (queue->active) >>>>> iio_dma_buffer_submit_block(queue, block); >>>>> - } else { >>>>> + else >>>>> block->state = IIO_BLOCK_STATE_QUEUED; >>>>> - list_add_tail(&block->head, &queue->incoming); >>>> If iio_dma_buffer_enqueue() is called with a dmabuf and the buffer >>>> is not active, it will be marked as queued, but we don't actually >>>> keep a reference to it anywhere. It will never be submitted to >>>> the DMA, and it will never be signaled as completed. >>> >>> We do keep a reference to the buffers, in the queue->fileio.blocks >>> array. When the buffer is enabled, all the blocks in that array >>> that are in the "queued" state will be submitted to the DMA. >>> >> But not when used in combination with the DMA buf changes later in >> this series. >> > > That's still the case after the DMABUF changes of the series. Or can > you point me exactly what you think is broken? > When you allocate a DMABUF with the allocate IOCTL and then submit it with the enqueue IOCTL before the buffer is enabled it will end up marked as queued, but not actually be queued anywhere.

3 years, 6 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] i2c: tegra: Add ACPI support

by Dmitry Osipenko

19.11.2021 16:32, Akhil R пишет: > - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > - if (IS_ERR(i2c_dev->rst)) { > - dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), > - "failed to get reset control\n"); > - return PTR_ERR(i2c_dev->rst); > - } > - > tegra_i2c_parse_dt(i2c_dev); > > - err = tegra_i2c_init_clocks(i2c_dev); > - if (err) > - return err; > + if (!has_acpi_companion(&pdev->dev)) { > + i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); > + if (IS_ERR(i2c_dev->rst)) { > + dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), > + "failed to get reset control\n"); > + return PTR_ERR(i2c_dev->rst); > + } > + > + err = tegra_i2c_init_clocks(i2c_dev); > + if (err) > + return err; > + } What about to factor out the reset initialization into a separate function and write it like this: static int tegra_i2c_init_reset(i2c_dev) { if (has_acpi_companion(i2c_dev->dev) return 0; i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c"); if (IS_ERR(i2c_dev->rst)) return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst), "failed to get reset control\n"); return 0; } And then change tegra_i2c_init_clocks() to: static int tegra_i2c_init_clocks(i2c_dev) { int err; if (has_acpi_companion(i2c_dev->dev)) return 0; ... } This will make both reset/clocks initialization to look more consistent.

3 years, 6 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig