[PATCH 01/12] kernel.h: Add for_each_if()

[PATCH 01/12] kernel.h: Add for_each_if()

[Daniel Vetter]

To avoid compilers complainig about ambigious else blocks when putting
an if condition into a for_each macro one needs to invert the
condition and add a dummy else. We have a nice little convenience
macro for that in drm headers, let's move it out. Subsequent patches
will roll it out to other places.

Motivated by a discussion with Andy and Yisheng, who want to add
another for_each_macro which would benefit from for_each_if() instead
of hand-rolling it.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: NeilBrown <neilb@suse.com>
Cc: Wei Wang <wvw@google.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Andrei Vagin <avagin@openvz.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Yisheng Xie <ysxie@foxmail.com>
---
 include/drm/drmP.h     | 3 ---
 include/linux/kernel.h | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f7a19c2a7a80..05350424a4d3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -110,7 +110,4 @@ static inline bool drm_can_sleep(void)
 	return true;
 }
 
-/* helper for handling conditionals in various for_each macros */
-#define for_each_if(condition) if (!(condition)) {} else
-
 #endif
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 941dc0a5a877..4cb95ab9a5bc 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -71,6 +71,9 @@
  */
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
+/* helper for handling conditionals in various for_each macros */
+#define for_each_if(condition) if (!(condition)) {} else
+
 #define u64_to_user_ptr(x) (		\
 {					\
 	typecheck(u64, x);		\
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/12] blk: use for_each_if

[Daniel Vetter]

Makes the macros resilient against if {} else {} blocks right
afterwards.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Shaohua Li <shli@fb.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/blk-cgroup.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 6c666fd7de3c..f1c3afe42c26 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -382,7 +382,7 @@ static inline void blkg_put(struct blkcg_gq *blkg)
  */
 #define blkg_for_each_descendant_pre(d_blkg, pos_css, p_blkg)		\
 	css_for_each_descendant_pre((pos_css), &(p_blkg)->blkcg->css)	\
-		if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css),	\
+		for_each_if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css), \
 					      (p_blkg)->q, false)))
 
 /**
@@ -397,7 +397,7 @@ static inline void blkg_put(struct blkcg_gq *blkg)
  */
 #define blkg_for_each_descendant_post(d_blkg, pos_css, p_blkg)		\
 	css_for_each_descendant_post((pos_css), &(p_blkg)->blkcg->css)	\
-		if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css),	\
+		for_each_if (((d_blkg) = __blkg_lookup(css_to_blkcg(pos_css), \
 					      (p_blkg)->q, false)))
 
 /**
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] blk: use for_each_if

[Tejun Heo]

On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
> Makes the macros resilient against if {} else {} blocks right
> afterwards.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Cc: Shaohua Li <shli@fb.com>
> Cc: Kate Stewart <kstewart@linuxfoundation.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Arnd Bergmann <arnd@arndb.de>

Acked-by: Tejun Heo <tj@kernel.org>

Jens, it'd probably be best to route this through block tree.

Thanks.

-- 
tejun

Re: [PATCH 02/12] blk: use for_each_if

[Tejun Heo]

On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
> > Makes the macros resilient against if {} else {} blocks right
> > afterwards.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Tejun Heo <tj@kernel.org>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Cc: Shaohua Li <shli@fb.com>
> > Cc: Kate Stewart <kstewart@linuxfoundation.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> Jens, it'd probably be best to route this through block tree.

Oops, this requires an earlier patch to move the for_each_if def to a
common header and should be routed together.

Thanks.

-- 
tejun

Re: [PATCH 02/12] blk: use for_each_if

[Jens Axboe]

On 7/11/18 10:45 AM, Tejun Heo wrote:
> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>> Makes the macros resilient against if {} else {} blocks right
>>> afterwards.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: Jens Axboe <axboe@kernel.dk>
>>> Cc: Shaohua Li <shli@fb.com>
>>> Cc: Kate Stewart <kstewart@linuxfoundation.org>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>
>> Acked-by: Tejun Heo <tj@kernel.org>
>>
>> Jens, it'd probably be best to route this through block tree.
> 
> Oops, this requires an earlier patch to move the for_each_if def to a
> common header and should be routed together.

Yeah, this is a problem with the submission.

Always (ALWAYS) CC folks on at least the cover letter and generic
earlier patches. Getting just one patch sent like this is mostly
useless, and causes more harm than good.

-- 
Jens Axboe

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] blk: use for_each_if

[Daniel Vetter]

On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 7/11/18 10:45 AM, Tejun Heo wrote:
>> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>>> Makes the macros resilient against if {} else {} blocks right
>>>> afterwards.
>>>>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> Cc: Tejun Heo <tj@kernel.org>
>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>> Cc: Shaohua Li <shli@fb.com>
>>>> Cc: Kate Stewart <kstewart@linuxfoundation.org>
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>
>>> Acked-by: Tejun Heo <tj@kernel.org>
>>>
>>> Jens, it'd probably be best to route this through block tree.
>>
>> Oops, this requires an earlier patch to move the for_each_if def to a
>> common header and should be routed together.
>
> Yeah, this is a problem with the submission.
>
> Always (ALWAYS) CC folks on at least the cover letter and generic
> earlier patches. Getting just one patch sent like this is mostly
> useless, and causes more harm than good.

Ime sending a patch with more than 20 or so recipients means it gets
stuck everywhere in moderation queues. Or outright spam filters. I
thought the correct way to do this is to cc: mailing lists (lkml has
them all), but apparently that's not how it's done. Despite that all
the patch series I get never have the cover letter addressed to me
either.

So what's the magic way to make this possible?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 02/12] blk: use for_each_if

[Jens Axboe]

On 7/11/18 12:50 PM, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 7/11/18 10:45 AM, Tejun Heo wrote:
>>> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>>>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>>>> Makes the macros resilient against if {} else {} blocks right
>>>>> afterwards.
>>>>>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>> Cc: Tejun Heo <tj@kernel.org>
>>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>>> Cc: Shaohua Li <shli@fb.com>
>>>>> Cc: Kate Stewart <kstewart@linuxfoundation.org>
>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>
>>>> Acked-by: Tejun Heo <tj@kernel.org>
>>>>
>>>> Jens, it'd probably be best to route this through block tree.
>>>
>>> Oops, this requires an earlier patch to move the for_each_if def to a
>>> common header and should be routed together.
>>
>> Yeah, this is a problem with the submission.
>>
>> Always (ALWAYS) CC folks on at least the cover letter and generic
>> earlier patches. Getting just one patch sent like this is mostly
>> useless, and causes more harm than good.
> 
> Ime sending a patch with more than 20 or so recipients means it gets
> stuck everywhere in moderation queues. Or outright spam filters. I
> thought the correct way to do this is to cc: mailing lists (lkml has
> them all), but apparently that's not how it's done. Despite that all
> the patch series I get never have the cover letter addressed to me
> either.
> 
> So what's the magic way to make this possible?

I don't think there's a git easy way of sending it out outside of
just ensuring that everybody is CC'ed on everything. I don't mind
that at all. I don't subscribe to lkml, and the patches weren't
sent to linux-block. Hence all I see is this stand-alone patch,
and logic would dictate that it's stand-alone (but it isn't).

-- 
Jens Axboe

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] blk: use for_each_if

[Tejun Heo]

On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
> I don't think there's a git easy way of sending it out outside of
> just ensuring that everybody is CC'ed on everything. I don't mind
> that at all. I don't subscribe to lkml, and the patches weren't
> sent to linux-block. Hence all I see is this stand-alone patch,
> and logic would dictate that it's stand-alone (but it isn't).

What I sometimes do is including a short blurb on each patch giving
the overview and action hints (e.g. this is part of patchset doing XYZ
and should be routed such and such).  It's a bit redundant but has
worked pretty well for patchsets with dependenat & sweeping changes.

Thanks.

-- 
tejun

Re: [PATCH 02/12] blk: use for_each_if

[Daniel Vetter]

On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
>> I don't think there's a git easy way of sending it out outside of
>> just ensuring that everybody is CC'ed on everything. I don't mind
>> that at all. I don't subscribe to lkml, and the patches weren't
>> sent to linux-block. Hence all I see is this stand-alone patch,
>> and logic would dictate that it's stand-alone (but it isn't).

Hm yeah I forgot to add linux-block. But others where there's no
dedicated list (or get_maintainers.pl didn't have one) also complained
about not getting Cc'ed, and I can't Cc everyone for sweeping changes.

> What I sometimes do is including a short blurb on each patch giving
> the overview and action hints (e.g. this is part of patchset doing XYZ
> and should be routed such and such).  It's a bit redundant but has
> worked pretty well for patchsets with dependenat & sweeping changes.

Yeah I guess I can just copypaste/summarize patch 1 to all the
subsequent patches, sounds like the best option.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] blk: use for_each_if

[Jens Axboe]

On 7/11/18 3:08 PM, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo <tj@kernel.org> wrote:
>> On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
>>> I don't think there's a git easy way of sending it out outside of
>>> just ensuring that everybody is CC'ed on everything. I don't mind
>>> that at all. I don't subscribe to lkml, and the patches weren't
>>> sent to linux-block. Hence all I see is this stand-alone patch,
>>> and logic would dictate that it's stand-alone (but it isn't).
> 
> Hm yeah I forgot to add linux-block. But others where there's no
> dedicated list (or get_maintainers.pl didn't have one) also complained
> about not getting Cc'ed, and I can't Cc everyone for sweeping changes.

I don't personally see a problem with just CC'ing everyone.

>> What I sometimes do is including a short blurb on each patch giving
>> the overview and action hints (e.g. this is part of patchset doing XYZ
>> and should be routed such and such).  It's a bit redundant but has
>> worked pretty well for patchsets with dependenat & sweeping changes.
> 
> Yeah I guess I can just copypaste/summarize patch 1 to all the
> subsequent patches, sounds like the best option.

Another approach might be to submit the first independent patch
separately. Once that's in the kernel, you can send out the rest
as independent patches instead of doing a cross-kernel series that
all depend on one single patch. Seems to me that's where you run
into issues, and it can be avoided quite easily.

-- 
Jens Axboe

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] blk: use for_each_if

[Daniel Vetter]

On Wed, Jul 11, 2018 at 03:13:00PM -0600, Jens Axboe wrote:
> On 7/11/18 3:08 PM, Daniel Vetter wrote:
> > On Wed, Jul 11, 2018 at 10:06 PM, Tejun Heo <tj@kernel.org> wrote:
> >> On Wed, Jul 11, 2018 at 01:31:51PM -0600, Jens Axboe wrote:
> >>> I don't think there's a git easy way of sending it out outside of
> >>> just ensuring that everybody is CC'ed on everything. I don't mind
> >>> that at all. I don't subscribe to lkml, and the patches weren't
> >>> sent to linux-block. Hence all I see is this stand-alone patch,
> >>> and logic would dictate that it's stand-alone (but it isn't).
> > 
> > Hm yeah I forgot to add linux-block. But others where there's no
> > dedicated list (or get_maintainers.pl didn't have one) also complained
> > about not getting Cc'ed, and I can't Cc everyone for sweeping changes.
> 
> I don't personally see a problem with just CC'ing everyone.
> 
> >> What I sometimes do is including a short blurb on each patch giving
> >> the overview and action hints (e.g. this is part of patchset doing XYZ
> >> and should be routed such and such).  It's a bit redundant but has
> >> worked pretty well for patchsets with dependenat & sweeping changes.
> > 
> > Yeah I guess I can just copypaste/summarize patch 1 to all the
> > subsequent patches, sounds like the best option.
> 
> Another approach might be to submit the first independent patch
> separately. Once that's in the kernel, you can send out the rest
> as independent patches instead of doing a cross-kernel series that
> all depend on one single patch. Seems to me that's where you run
> into issues, and it can be avoided quite easily.

Well patch 1 died in a bikeshed (or is on live support at most). I kinda
hoped that showing it's somewhat widely used pattern in the kernel would
help it's cause, but alas not going to happen.

Anyway for next time around I'll crank up the Cc: knob to the max :-)

Thanks anyway for comments and stuff.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/12] blk: use for_each_if

[Joe Perches]

On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
> On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
> > On 7/11/18 10:45 AM, Tejun Heo wrote:
> > > On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
> > > > On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
> > > > > Makes the macros resilient against if {} else {} blocks right
> > > > > afterwards.
> > > > > 
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Tejun Heo <tj@kernel.org>
> > > > > Cc: Jens Axboe <axboe@kernel.dk>
> > > > > Cc: Shaohua Li <shli@fb.com>
> > > > > Cc: Kate Stewart <kstewart@linuxfoundation.org>
> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Cc: Arnd Bergmann <arnd@arndb.de>
> > > > 
> > > > Acked-by: Tejun Heo <tj@kernel.org>
> > > > 
> > > > Jens, it'd probably be best to route this through block tree.
> > > 
> > > Oops, this requires an earlier patch to move the for_each_if def to a
> > > common header and should be routed together.
> > 
> > Yeah, this is a problem with the submission.
> > 
> > Always (ALWAYS) CC folks on at least the cover letter and generic
> > earlier patches. Getting just one patch sent like this is mostly
> > useless, and causes more harm than good.
> 
> Ime sending a patch with more than 20 or so recipients means it gets
> stuck everywhere in moderation queues. Or outright spam filters. I
> thought the correct way to do this is to cc: mailing lists (lkml has
> them all), but apparently that's not how it's done. Despite that all
> the patch series I get never have the cover letter addressed to me
> either.
> 
> So what's the magic way to make this possible?

Jens' advice is crap.

There is no generic way to make this possible.

BCC's don't work, series that touch multiple subsystems
get rejected when the recipient list is too large.

I think you did it correctly.

Re: [PATCH 02/12] blk: use for_each_if

[Jens Axboe]

On 7/12/18 12:45 AM, Joe Perches wrote:
> On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
>> On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 7/11/18 10:45 AM, Tejun Heo wrote:
>>>> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>>>>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>>>>> Makes the macros resilient against if {} else {} blocks right
>>>>>> afterwards.
>>>>>>
>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>>> Cc: Tejun Heo <tj@kernel.org>
>>>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>>>> Cc: Shaohua Li <shli@fb.com>
>>>>>> Cc: Kate Stewart <kstewart@linuxfoundation.org>
>>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>>
>>>>> Acked-by: Tejun Heo <tj@kernel.org>
>>>>>
>>>>> Jens, it'd probably be best to route this through block tree.
>>>>
>>>> Oops, this requires an earlier patch to move the for_each_if def to a
>>>> common header and should be routed together.
>>>
>>> Yeah, this is a problem with the submission.
>>>
>>> Always (ALWAYS) CC folks on at least the cover letter and generic
>>> earlier patches. Getting just one patch sent like this is mostly
>>> useless, and causes more harm than good.
>>
>> Ime sending a patch with more than 20 or so recipients means it gets
>> stuck everywhere in moderation queues. Or outright spam filters. I
>> thought the correct way to do this is to cc: mailing lists (lkml has
>> them all), but apparently that's not how it's done. Despite that all
>> the patch series I get never have the cover letter addressed to me
>> either.
>>
>> So what's the magic way to make this possible?
> 
> Jens' advice is crap.
> 
> There is no generic way to make this possible.

Nobody claimed there was. And the advice is perfectly fine,
sending out patches to folks that have hidden dependencies on other
patches is a no-go.

> BCC's don't work, series that touch multiple subsystems
> get rejected when the recipient list is too large.
> 
> I think you did it correctly.

Clearly that's not the case, regardless of what you think.

Thanks for your invaluable and useful feedback, sharing your vast
experience in patchsets with dependencies.

-- 
Jens Axboe

Re: [PATCH 02/12] blk: use for_each_if

[Joe Perches]

On Thu, 2018-07-12 at 07:54 -0600, Jens Axboe wrote:
> 
> Thanks for your invaluable and useful feedback, sharing your vast
> experience in patchsets with dependencies.

I've probably more experience sending patchsets
with dependencies across subsystems than anyone.

There is no single style that works and I've
probably tried them all.

It's actually a somewhat significant issue within
this community that could use some arbitration.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] blk: use for_each_if

[Vlastimil Babka]

On 07/12/2018 08:45 AM, Joe Perches wrote:
> On Wed, 2018-07-11 at 20:50 +0200, Daniel Vetter wrote:
>> On Wed, Jul 11, 2018 at 8:30 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 7/11/18 10:45 AM, Tejun Heo wrote:
>>>> On Wed, Jul 11, 2018 at 09:40:58AM -0700, Tejun Heo wrote:
>>>>> On Mon, Jul 09, 2018 at 10:36:40AM +0200, Daniel Vetter wrote:
>>>>>> Makes the macros resilient against if {} else {} blocks right
>>>>>> afterwards.
>>>>>>
>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>>> Cc: Tejun Heo <tj@kernel.org>
>>>>>> Cc: Jens Axboe <axboe@kernel.dk>
>>>>>> Cc: Shaohua Li <shli@fb.com>
>>>>>> Cc: Kate Stewart <kstewart@linuxfoundation.org>
>>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>> Cc: Joseph Qi <joseph.qi@linux.alibaba.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>>
>>>>> Acked-by: Tejun Heo <tj@kernel.org>
>>>>>
>>>>> Jens, it'd probably be best to route this through block tree.
>>>>
>>>> Oops, this requires an earlier patch to move the for_each_if def to a
>>>> common header and should be routed together.
>>>
>>> Yeah, this is a problem with the submission.
>>>
>>> Always (ALWAYS) CC folks on at least the cover letter and generic
>>> earlier patches. Getting just one patch sent like this is mostly
>>> useless, and causes more harm than good.
>>
>> Ime sending a patch with more than 20 or so recipients means it gets
>> stuck everywhere in moderation queues. Or outright spam filters. I
>> thought the correct way to do this is to cc: mailing lists (lkml has
>> them all), but apparently that's not how it's done. Despite that all
>> the patch series I get never have the cover letter addressed to me
>> either.
>>
>> So what's the magic way to make this possible?
> 
> Jens' advice is crap.

This statement was rather offensive and totally uncalled for, AFAICS.
Why did you write it like that?

> There is no generic way to make this possible.
> 
> BCC's don't work, series that touch multiple subsystems
> get rejected when the recipient list is too large.

I don't know what's the usual limit for recipient list, probably never
hit it myself, but for series that are not so large, I use this approach
to make sure the cover letter is CC'd to everyone that's CC'd in any
patch in the series:

- add per-patch Cc:'s to the git commit logs
- clear out *.patch from the working dir
- git format-patch --cover-letter ...
- edit cover letter
- git send-email ... --cc-cmd=./cc.sh ...

where cc.sh contains this:

#/bin/sh
if [[ $1 == *cover-letter* ]]; then
        grep '<.*@.*>' -h *.patch | sed 's/^.*: //' | sort | uniq
else
        grep '<.*@.*>' -h $1 | sed 's/^.*: //' | sort | uniq
fi

That proceses all tags besides Cc (acked-by, reported-by etc) and turns
them to Cc's for each patch (or does git now do that by itself as well?)
and for cover letter, it accumulates that from all the patches.

Vlastimil

> I think you did it correctly.
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/12] cgroup: use for_each_if

[Daniel Vetter]

Avoids the need to invert the condition instead of the open-coded
version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: cgroups@vger.kernel.org
---
 include/linux/cgroup.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c9fdf6f57913..666c6200d61d 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -287,9 +287,7 @@ void css_task_iter_end(struct css_task_iter *it);
 	for ((leader) = cgroup_taskset_first((tset), &(dst_css));	\
 	     (leader);							\
 	     (leader) = cgroup_taskset_next((tset), &(dst_css)))	\
-		if ((leader) != (leader)->group_leader)			\
-			;						\
-		else
+		for_each_if ((leader) == (leader)->group_leader)
 
 /*
  * Inline functions.
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/12] cgroup: use for_each_if

[Tejun Heo]

On Mon, Jul 09, 2018 at 10:36:41AM +0200, Daniel Vetter wrote:
> Avoids the need to invert the condition instead of the open-coded
> version.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: cgroups@vger.kernel.org

Acked-by: Tejun Heo <tj@kernel.org>

Please feel free to route as you see fit.

Thanks.

-- 
tejun
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/12] cpufreq: use for_each_if

[Daniel Vetter]

Avoids the inverted condition compared to the open coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
---
 include/linux/cpufreq.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 882a9b9e34bc..f2028c229b96 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
 
 #define cpufreq_for_each_valid_entry(pos, table)			\
 	for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++)	\
-		if (pos->frequency == CPUFREQ_ENTRY_INVALID)		\
-			continue;					\
-		else
+		for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
 
 /*
  * cpufreq_for_each_valid_entry_idx -     iterate with index over a cpufreq
@@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
 
 #define cpufreq_for_each_valid_entry_idx(pos, table, idx)		\
 	cpufreq_for_each_entry_idx(pos, table, idx)			\
-		if (pos->frequency == CPUFREQ_ENTRY_INVALID)		\
-			continue;					\
-		else
+		for_each_if (pos->frequency == CPUFREQ_ENTRY_INVALID)
 
 
 int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
-- 
2.18.0

Re: [PATCH 04/12] cpufreq: use for_each_if

[Eric Engestrom]

On Monday, 2018-07-09 10:36:42 +0200, Daniel Vetter wrote:
> Avoids the inverted condition compared to the open coded version.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> ---
>  include/linux/cpufreq.h | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 882a9b9e34bc..f2028c229b96 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
>  
>  #define cpufreq_for_each_valid_entry(pos, table)			\
>  	for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++)	\
> -		if (pos->frequency == CPUFREQ_ENTRY_INVALID)		\
> -			continue;					\
> -		else
> +		for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
>  
>  /*
>   * cpufreq_for_each_valid_entry_idx -     iterate with index over a cpufreq
> @@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
>  
>  #define cpufreq_for_each_valid_entry_idx(pos, table, idx)		\
>  	cpufreq_for_each_entry_idx(pos, table, idx)			\
> -		if (pos->frequency == CPUFREQ_ENTRY_INVALID)		\
> -			continue;					\
> -		else
> +		for_each_if (pos->frequency == CPUFREQ_ENTRY_INVALID)

Should be `!=` there ^

>  
>  
>  int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] cpufreq: use for_each_if

[Daniel Vetter]

Avoids the inverted condition compared to the open coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Cc: Eric Engestrom <eric.engestrom@intel.com>
--
v2: Fix the logic fumble in the 2nd hunk, spotted by Eric.
---
 include/linux/cpufreq.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 882a9b9e34bc..3a774f523d00 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
 
 #define cpufreq_for_each_valid_entry(pos, table)			\
 	for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++)	\
-		if (pos->frequency == CPUFREQ_ENTRY_INVALID)		\
-			continue;					\
-		else
+		for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
 
 /*
  * cpufreq_for_each_valid_entry_idx -     iterate with index over a cpufreq
@@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
 
 #define cpufreq_for_each_valid_entry_idx(pos, table, idx)		\
 	cpufreq_for_each_entry_idx(pos, table, idx)			\
-		if (pos->frequency == CPUFREQ_ENTRY_INVALID)		\
-			continue;					\
-		else
+		for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
 
 
 int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] cpufreq: use for_each_if

[Rafael J. Wysocki]

On Mon, Jul 9, 2018 at 6:11 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Avoids the inverted condition compared to the open coded version.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Cc: Eric Engestrom <eric.engestrom@intel.com>
> --
> v2: Fix the logic fumble in the 2nd hunk, spotted by Eric.

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Or do you want me to apply it?

> ---
>  include/linux/cpufreq.h | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 882a9b9e34bc..3a774f523d00 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -649,9 +649,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
>
>  #define cpufreq_for_each_valid_entry(pos, table)                       \
>         for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++)   \
> -               if (pos->frequency == CPUFREQ_ENTRY_INVALID)            \
> -                       continue;                                       \
> -               else
> +               for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
>
>  /*
>   * cpufreq_for_each_valid_entry_idx -     iterate with index over a cpufreq
> @@ -663,9 +661,7 @@ static inline void dev_pm_opp_free_cpufreq_table(struct device *dev,
>
>  #define cpufreq_for_each_valid_entry_idx(pos, table, idx)              \
>         cpufreq_for_each_entry_idx(pos, table, idx)                     \
> -               if (pos->frequency == CPUFREQ_ENTRY_INVALID)            \
> -                       continue;                                       \
> -               else
> +               for_each_if (pos->frequency != CPUFREQ_ENTRY_INVALID)
>
>
>  int cpufreq_frequency_table_cpuinfo(struct cpufreq_policy *policy,
> --
> 2.18.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/12] dmar: Use for_each_If

[Daniel Vetter]

Avoids the inverted conditions compared to the open coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Joerg Roedel <jroedel@suse.de>
---
 include/linux/dmar.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index e2433bc50210..99397504e75e 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -85,15 +85,15 @@ extern struct list_head dmar_drhd_units;
 
 #define for_each_active_drhd_unit(drhd)					\
 	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
-		if (drhd->ignored) {} else
+		for_each_if (!drhd->ignored)
 
 #define for_each_active_iommu(i, drhd)					\
 	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
-		if (i=drhd->iommu, drhd->ignored) {} else
+		for_each_if ((i=drhd->iommu, !drhd->ignored))
 
 #define for_each_iommu(i, drhd)						\
 	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
-		if (i=drhd->iommu, 0) {} else 
+		for_each_if ((i=drhd->iommu, true))
 
 static inline bool dmar_rcu_check(void)
 {
@@ -108,7 +108,8 @@ static inline bool dmar_rcu_check(void)
 			NULL, (p) < (c)); (p)++)
 
 #define	for_each_active_dev_scope(a, c, p, d)	\
-	for_each_dev_scope((a), (c), (p), (d))	if (!(d)) { continue; } else
+	for_each_dev_scope((a), (c), (p), (d))	\
+		for_each_if (d)
 
 extern int dmar_table_init(void);
 extern int dmar_dev_scope_init(void);
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/12] dmar: Use for_each_If

[Joerg Roedel]

On Mon, Jul 09, 2018 at 10:36:43AM +0200, Daniel Vetter wrote:
>  #define for_each_active_drhd_unit(drhd)					\
>  	list_for_each_entry_rcu(drhd, &dmar_drhd_units, list)		\
> -		if (drhd->ignored) {} else
> +		for_each_if (!drhd->ignored)

Hmm, in my tree the macro comes from

	include/drm/drmP.h:#define for_each_if(condition) if (!(condition)) {} else

Please re-submit when the macro is in a generic header upstream.


	Joerg

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/12] mm: use for_each_if

[Daniel Vetter]

Avoids the inverted condition of the open-coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: David Rientjes <rientjes@google.com>
Cc: Kemi Wang <kemi.wang@intel.com>
Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
Cc: Petr Tesarik <ptesarik@suse.com>
Cc: YASUAKI ISHIMATSU <yasu.isimatu@gmail.com>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Nikolay Borisov <nborisov@suse.com>
Cc: linux-mm@kvack.org
---
 include/linux/mmzone.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 32699b2dc52a..1bd5f4c72c8b 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -940,9 +940,7 @@ extern struct zone *next_zone(struct zone *zone);
 	for (zone = (first_online_pgdat())->node_zones; \
 	     zone;					\
 	     zone = next_zone(zone))			\
-		if (!populated_zone(zone))		\
-			; /* do nothing */		\
-		else
+		for_each_if (populated_zone(zone))
 
 static inline struct zone *zonelist_zone(struct zoneref *zoneref)
 {
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/12] mm: use for_each_if

[Pavel Tatashin]

LGTM:

Reviewed-by: Pavel Tatashin <pasha.tatashin@oracle.com>

[PATCH 07/12] ide: use for_each_if

[Daniel Vetter]

Avoids complaints from gcc about ambigious else clauses. Not that any
of those are likely to show up in ide ...

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-ide@vger.kernel.org
---
 include/linux/ide.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/ide.h b/include/linux/ide.h
index c74b0321922a..1530d81319ef 100644
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1601,7 +1601,7 @@ static inline void ide_set_drivedata(ide_drive_t *drive, void *data)
 
 #define ide_port_for_each_present_dev(i, dev, port) \
 	for ((i) = 0; ((dev) = (port)->devices[i]) || (i) < MAX_DRIVES; (i)++) \
-		if ((dev)->dev_flags & IDE_DFLAG_PRESENT)
+		for_each_if ((dev)->dev_flags & IDE_DFLAG_PRESENT)
 
 #define ide_host_for_each_port(i, port, host) \
 	for ((i) = 0; ((port) = (host)->ports[i]) || (i) < MAX_HOST_PORTS; (i)++)
-- 
2.18.0

[PATCH 08/12] netdev: use for_each_if

[Daniel Vetter]

Avoids complaints from gcc about ambiguous else clauses.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
---
 include/linux/netdevice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3ec9850c7936..cdffe7f88dbe 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2476,7 +2476,7 @@ extern rwlock_t				dev_base_lock;		/* Device list lock */
 	list_for_each_entry_continue_rcu(d, &(net)->dev_base_head, dev_list)
 #define for_each_netdev_in_bond_rcu(bond, slave)	\
 		for_each_netdev_rcu(&init_net, slave)	\
-			if (netdev_master_upper_dev_get_rcu(slave) == (bond))
+			for_each_if (netdev_master_upper_dev_get_rcu(slave) == (bond))
 #define net_device_entry(lh)	list_entry(lh, struct net_device, dev_list)
 
 static inline struct net_device *next_net_device(struct net_device *dev)
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/12] nubus: use for_each_if

[Daniel Vetter]

Avoids the inverted check compared to the open-coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Finn Thain <fthain@telegraphics.com.au>
Cc: linux-m68k@lists.linux-m68k.org
---
 include/linux/nubus.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/nubus.h b/include/linux/nubus.h
index eba50b057f6f..17fd07578ef7 100644
--- a/include/linux/nubus.h
+++ b/include/linux/nubus.h
@@ -127,7 +127,7 @@ struct nubus_rsrc *nubus_next_rsrc_or_null(struct nubus_rsrc *from);
 	for (f = nubus_first_rsrc_or_null(); f; f = nubus_next_rsrc_or_null(f))
 
 #define for_each_board_func_rsrc(b, f) \
-	for_each_func_rsrc(f) if (f->board != b) {} else
+	for_each_func_rsrc(f) for_each_if (f->board == b)
 
 /* These are somewhat more NuBus-specific.  They all return 0 for
    success and -1 for failure, as you'd expect. */
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] nubus: use for_each_if

[Finn Thain]

On Mon, 9 Jul 2018, Daniel Vetter wrote:

> Avoids the inverted check compared to the open-coded version.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Finn Thain <fthain@telegraphics.com.au>
> Cc: linux-m68k@lists.linux-m68k.org

Acked-by: Finn Thain <fthain@telegraphics.com.au>

> ---
>  include/linux/nubus.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/nubus.h b/include/linux/nubus.h
> index eba50b057f6f..17fd07578ef7 100644
> --- a/include/linux/nubus.h
> +++ b/include/linux/nubus.h
> @@ -127,7 +127,7 @@ struct nubus_rsrc *nubus_next_rsrc_or_null(struct nubus_rsrc *from);
>  	for (f = nubus_first_rsrc_or_null(); f; f = nubus_next_rsrc_or_null(f))
>  
>  #define for_each_board_func_rsrc(b, f) \
> -	for_each_func_rsrc(f) if (f->board != b) {} else
> +	for_each_func_rsrc(f) for_each_if (f->board == b)
>  
>  /* These are somewhat more NuBus-specific.  They all return 0 for
>     success and -1 for failure, as you'd expect. */
> 

Re: [PATCH 09/12] nubus: use for_each_if

[Geert Uytterhoeven]

Hi Daniel,

On Mon, Jul 9, 2018 at 10:44 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Avoids the inverted check compared to the open-coded version.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Finn Thain <fthain@telegraphics.com.au>
> Cc: linux-m68k@lists.linux-m68k.org

Thanks for your patch!

> --- a/include/linux/nubus.h
> +++ b/include/linux/nubus.h
> @@ -127,7 +127,7 @@ struct nubus_rsrc *nubus_next_rsrc_or_null(struct nubus_rsrc *from);
>         for (f = nubus_first_rsrc_or_null(); f; f = nubus_next_rsrc_or_null(f))
>
>  #define for_each_board_func_rsrc(b, f) \
> -       for_each_func_rsrc(f) if (f->board != b) {} else
> +       for_each_func_rsrc(f) for_each_if (f->board == b)

drivers/net/ethernet/8390/mac8390.c: In function ‘mac8390_device_probe’:
drivers/net/ethernet/8390/mac8390.c:402: error: implicit declaration
of function ‘for_each_if’
drivers/net/ethernet/8390/mac8390.c:402: error: expected ‘;’ before ‘{’ token

Apparently this depends on patch [01/12], which wasn't CCed to
linux-m68k@lists.linux-m68k.org?

Please don't split CCs if all patches in the a series are not independent.
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 10/12] pci: use for_each_if

[Daniel Vetter]

Avoids the inverted condition compared to the open-coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 include/linux/pci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 340029b2fb38..1484471ed048 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -601,7 +601,7 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
 
 #define for_each_pci_bridge(dev, bus)				\
 	list_for_each_entry(dev, &bus->devices, bus_list)	\
-		if (!pci_is_bridge(dev)) {} else
+		for_each_if (pci_is_bridge(dev))
 
 static inline struct pci_dev *pci_upstream_bridge(struct pci_dev *dev)
 {
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] pci: use for_each_if

[Bjorn Helgaas]

On Mon, Jul 09, 2018 at 10:36:48AM +0200, Daniel Vetter wrote:
> Avoids the inverted condition compared to the open-coded version.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I assume you'll merge this with the rest of the series.

> ---
>  include/linux/pci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 340029b2fb38..1484471ed048 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -601,7 +601,7 @@ static inline bool pci_is_bridge(struct pci_dev *dev)
>  
>  #define for_each_pci_bridge(dev, bus)				\
>  	list_for_each_entry(dev, &bus->devices, bus_list)	\
> -		if (!pci_is_bridge(dev)) {} else
> +		for_each_if (pci_is_bridge(dev))
>  
>  static inline struct pci_dev *pci_upstream_bridge(struct pci_dev *dev)
>  {
> -- 
> 2.18.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/12] sched: use for_each_if in topology.h

[Daniel Vetter]

Avoids complaints from gcc about ambiguous else clauses.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
---
 include/linux/topology.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/topology.h b/include/linux/topology.h
index cb0775e1ee4b..4fba5a5b148d 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -40,7 +40,7 @@
 
 #define for_each_node_with_cpus(node)			\
 	for_each_online_node(node)			\
-		if (nr_cpus_node(node))
+		for_each_if (nr_cpus_node(node))
 
 int arch_update_cpu_topology(void);
 
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Peter Zijlstra]

On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote:
> Avoids complaints from gcc about ambiguous else clauses.

Is that a new thing? I'm fairly sure I've never seen it do that,

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> ---
>  include/linux/topology.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e1ee4b..4fba5a5b148d 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -40,7 +40,7 @@
>  
>  #define for_each_node_with_cpus(node)			\
>  	for_each_online_node(node)			\
> -		if (nr_cpus_node(node))
> +		for_each_if (nr_cpus_node(node))

Not having gotten any of the other patches, I'm not really sure what
this does and such, but improve readability it does not :/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Daniel Vetter]

On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote:
>> Avoids complaints from gcc about ambiguous else clauses.
>
> Is that a new thing? I'm fairly sure I've never seen it do that,
>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> ---
>>  include/linux/topology.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/topology.h b/include/linux/topology.h
>> index cb0775e1ee4b..4fba5a5b148d 100644
>> --- a/include/linux/topology.h
>> +++ b/include/linux/topology.h
>> @@ -40,7 +40,7 @@
>>
>>  #define for_each_node_with_cpus(node)                        \
>>       for_each_online_node(node)                      \
>> -             if (nr_cpus_node(node))
>> +             for_each_if (nr_cpus_node(node))
>
> Not having gotten any of the other patches, I'm not really sure what
> this does and such, but improve readability it does not :/

Patch 1 in this series, which I dumped onto lkml as a whole:

https://lkml.org/lkml/2018/7/9/179

Imo it does improve readability for the if (!cond) {} else pattern.
And (assuming my grep fu isn't too badly wrong) most places in the
kernel do use this pattern in for_each macros, so I guess its a real
thing. We've definitely hit it plenty in drm iterators (but we seem to
like if() checks in iterator macros maybe a bit too much).

I'm happy to drop this patch tough if you deem it offensive.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Peter Zijlstra]

On Mon, Jul 09, 2018 at 05:00:07PM +0200, Daniel Vetter wrote:
> On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote:

> >>  #define for_each_node_with_cpus(node)                        \
> >>       for_each_online_node(node)                      \
> >> -             if (nr_cpus_node(node))
> >> +             for_each_if (nr_cpus_node(node))
> >
> > Not having gotten any of the other patches, I'm not really sure what
> > this does and such, but improve readability it does not :/
> 
> Patch 1 in this series, which I dumped onto lkml as a whole:
> 
> https://lkml.org/lkml/2018/7/9/179

Right, so while I don't object to being Cc'ed to the whole series, I do
mind not being Cc'ed to at least the generic bits required to understand
the patch I do have to look at.

> Imo it does improve readability for the if (!cond) {} else pattern.
> And (assuming my grep fu isn't too badly wrong) most places in the
> kernel do use this pattern in for_each macros, so I guess its a real
> thing. We've definitely hit it plenty in drm iterators (but we seem to
> like if() checks in iterator macros maybe a bit too much).
> 
> I'm happy to drop this patch tough if you deem it offensive.

I'd just like to understand it better; what compiler complains about
this and is the warning otherwise useful? These things don't seem
mentioned in that initial patch either.

IOW I suppose I'm asking for the justification of this churn. If it's
really needed and useful so be it, but so far I'm not seeing any.

At a while guess I'd say this is something new in gcc-8 (and while I
have that installed on some machines, it doesn't seem to be the default,
and so I've not actually seen its output). But is the warning actually
useful, should we not just kill the warning like we tend to do some
really silly ones.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Daniel Vetter]

On Mon, Jul 09, 2018 at 05:12:58PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 05:00:07PM +0200, Daniel Vetter wrote:
> > On Mon, Jul 9, 2018 at 12:36 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Mon, Jul 09, 2018 at 10:36:49AM +0200, Daniel Vetter wrote:
> 
> > >>  #define for_each_node_with_cpus(node)                        \
> > >>       for_each_online_node(node)                      \
> > >> -             if (nr_cpus_node(node))
> > >> +             for_each_if (nr_cpus_node(node))
> > >
> > > Not having gotten any of the other patches, I'm not really sure what
> > > this does and such, but improve readability it does not :/
> > 
> > Patch 1 in this series, which I dumped onto lkml as a whole:
> > 
> > https://lkml.org/lkml/2018/7/9/179
> 
> Right, so while I don't object to being Cc'ed to the whole series, I do
> mind not being Cc'ed to at least the generic bits required to understand
> the patch I do have to look at.
> 
> > Imo it does improve readability for the if (!cond) {} else pattern.
> > And (assuming my grep fu isn't too badly wrong) most places in the
> > kernel do use this pattern in for_each macros, so I guess its a real
> > thing. We've definitely hit it plenty in drm iterators (but we seem to
> > like if() checks in iterator macros maybe a bit too much).
> > 
> > I'm happy to drop this patch tough if you deem it offensive.
> 
> I'd just like to understand it better; what compiler complains about
> this and is the warning otherwise useful? These things don't seem
> mentioned in that initial patch either.
> 
> IOW I suppose I'm asking for the justification of this churn. If it's
> really needed and useful so be it, but so far I'm not seeing any.
> 
> At a while guess I'd say this is something new in gcc-8 (and while I
> have that installed on some machines, it doesn't seem to be the default,
> and so I've not actually seen its output). But is the warning actually
> useful, should we not just kill the warning like we tend to do some
> really silly ones.

for_each_something(foo)
	if (foo->bla)
		call_bla(foo);
	else
		call_default(foo);

Totally contrived, but this complains. Liberally sprinkling {} also shuts
up the compiler, but it's a bit confusing given that a plain for {;;} is
totally fine. And it's confusing since at first glance the compiler
complaining about nested if and ambigous else doesn't make sense since
clearly there's only 1 if there.

Wrt this being useful or not: We've had it for a while in drm, and Andy
and Yishen where rolling yet another open coded version of this on a patch
that flew past me on dri-devel. So I pointed them at the for_each_if() we
have and typed this series to move it to kernel.h.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Peter Zijlstra]

On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote:
> for_each_something(foo)
> 	if (foo->bla)
> 		call_bla(foo);
> 	else
> 		call_default(foo);
> 
> Totally contrived, but this complains. Liberally sprinkling {} also shuts
> up the compiler, but it's a bit confusing given that a plain for {;;} is
> totally fine. And it's confusing since at first glance the compiler
> complaining about nested if and ambigous else doesn't make sense since
> clearly there's only 1 if there.

Ah, so the pattern the compiler tries to warn about is:

	if (foo)
		if (bar)
			/* stmts1 */
		else
			/* stmts2 *

Because it might not be immediately obvious with which if the else goes.
Which is fair enough I suppose.

OK, ACK.

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Daniel Vetter]

On Mon, Jul 9, 2018 at 6:03 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote:
>> for_each_something(foo)
>>       if (foo->bla)
>>               call_bla(foo);
>>       else
>>               call_default(foo);
>>
>> Totally contrived, but this complains. Liberally sprinkling {} also shuts
>> up the compiler, but it's a bit confusing given that a plain for {;;} is
>> totally fine. And it's confusing since at first glance the compiler
>> complaining about nested if and ambigous else doesn't make sense since
>> clearly there's only 1 if there.
>
> Ah, so the pattern the compiler tries to warn about is:
>
>         if (foo)
>                 if (bar)
>                         /* stmts1 */
>                 else
>                         /* stmts2 *
>
> Because it might not be immediately obvious with which if the else goes.
> Which is fair enough I suppose.

Yup. I'll augment the commit message of patch 1 to include this as
example, and why it's confusing in context of a for_each_foo macro
containing an if().

> OK, ACK.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Mark Rutland]

On Mon, Jul 09, 2018 at 06:03:42PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote:
> > for_each_something(foo)
> > 	if (foo->bla)
> > 		call_bla(foo);
> > 	else
> > 		call_default(foo);
> > 
> > Totally contrived, but this complains. Liberally sprinkling {} also shuts
> > up the compiler, but it's a bit confusing given that a plain for {;;} is
> > totally fine. And it's confusing since at first glance the compiler
> > complaining about nested if and ambigous else doesn't make sense since
> > clearly there's only 1 if there.
> 
> Ah, so the pattern the compiler tries to warn about is:
> 
> 	if (foo)
> 		if (bar)
> 			/* stmts1 */
> 		else
> 			/* stmts2 *
> 
> Because it might not be immediately obvious with which if the else goes.
> Which is fair enough I suppose.
> 
> OK, ACK.

Just to bikeshed, there could be macros other than for_each_*() macros
that will want to use this internally, so perhaps it would be worth the
generic version being named something like if_noelse().

We could always add that as/when required, though.

Mark.

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Daniel Vetter]

On Mon, Jul 9, 2018 at 6:12 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Jul 09, 2018 at 06:03:42PM +0200, Peter Zijlstra wrote:
>> On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote:
>> > for_each_something(foo)
>> >     if (foo->bla)
>> >             call_bla(foo);
>> >     else
>> >             call_default(foo);
>> >
>> > Totally contrived, but this complains. Liberally sprinkling {} also shuts
>> > up the compiler, but it's a bit confusing given that a plain for {;;} is
>> > totally fine. And it's confusing since at first glance the compiler
>> > complaining about nested if and ambigous else doesn't make sense since
>> > clearly there's only 1 if there.
>>
>> Ah, so the pattern the compiler tries to warn about is:
>>
>>       if (foo)
>>               if (bar)
>>                       /* stmts1 */
>>               else
>>                       /* stmts2 *
>>
>> Because it might not be immediately obvious with which if the else goes.
>> Which is fair enough I suppose.
>>
>> OK, ACK.
>
> Just to bikeshed, there could be macros other than for_each_*() macros
> that will want to use this internally, so perhaps it would be worth the
> generic version being named something like if_noelse().
>
> We could always add that as/when required, though.

I think a better name would be really good, but both when we added it
for i915 and when we move it to drm headers we drew a blank.
if_noelse() describes pretty good what it does, but kinda fails on the
"where should I use it" departement. If there's some consensus I can
sed the patches quickly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/12] sched: use for_each_if in topology.h

[Mark Rutland]

On Mon, Jul 09, 2018 at 07:55:20PM +0200, Daniel Vetter wrote:
> On Mon, Jul 9, 2018 at 6:12 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Jul 09, 2018 at 06:03:42PM +0200, Peter Zijlstra wrote:
> >> On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote:
> >> > for_each_something(foo)
> >> >     if (foo->bla)
> >> >             call_bla(foo);
> >> >     else
> >> >             call_default(foo);
> >> >
> >> > Totally contrived, but this complains. Liberally sprinkling {} also shuts
> >> > up the compiler, but it's a bit confusing given that a plain for {;;} is
> >> > totally fine. And it's confusing since at first glance the compiler
> >> > complaining about nested if and ambigous else doesn't make sense since
> >> > clearly there's only 1 if there.
> >>
> >> Ah, so the pattern the compiler tries to warn about is:
> >>
> >>       if (foo)
> >>               if (bar)
> >>                       /* stmts1 */
> >>               else
> >>                       /* stmts2 *
> >>
> >> Because it might not be immediately obvious with which if the else goes.
> >> Which is fair enough I suppose.
> >>
> >> OK, ACK.
> >
> > Just to bikeshed, there could be macros other than for_each_*() macros
> > that will want to use this internally, so perhaps it would be worth the
> > generic version being named something like if_noelse().
> >
> > We could always add that as/when required, though.
> 
> I think a better name would be really good, but both when we added it
> for i915 and when we move it to drm headers we drew a blank.
> if_noelse() describes pretty good what it does, but kinda fails on the
> "where should I use it" departement. If there's some consensus I can
> sed the patches quickly.

Just to be clear: for_each_if() is fine by me, so no need to change
things.

Sorry for the noise!

Mark.

Re: [PATCH 11/12] sched: use for_each_if in topology.h

[Peter Zijlstra]

On Mon, Jul 09, 2018 at 05:52:04PM +0200, Daniel Vetter wrote:
> for_each_something(foo)
> 	if (foo->bla)
> 		call_bla(foo);
> 	else
> 		call_default(foo);

Note that the kernel coding style 'discourages' this style and would
like you to write:

	for_each_something(foo) {
		if (foo->bla)
			call_bla(foo);
		else
			call_default(foo);
	}

Which avoids the entire problem. See CodingStyle-3:

  Also, use braces when a loop contains more than a single simple statement:

  .. code-block:: c

	  while (condition) {
		  if (test)
			  do_something();
	  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/12] usb: use for_each_if

[Daniel Vetter]

Avoids the inverted condition compared to the open-coded version.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
---
 include/linux/usb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 4cdd515a4385..a9da7237c619 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -732,7 +732,7 @@ extern struct usb_device *usb_hub_find_child(struct usb_device *hdev,
 	for (port1 = 1,	child =	usb_hub_find_child(hdev, port1); \
 			port1 <= hdev->maxchild; \
 			child = usb_hub_find_child(hdev, ++port1)) \
-		if (!child) continue; else
+		for_each_if (child)
 
 /* USB device locking */
 #define usb_lock_device(udev)			device_lock(&(udev)->dev)
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/12] kernel.h: Add for_each_if()

[Andy Shevchenko]

On Mon, 2018-07-09 at 10:36 +0200, Daniel Vetter wrote:
> To avoid compilers complainig about ambigious else blocks when putting
> an if condition into a for_each macro one needs to invert the
> condition and add a dummy else. We have a nice little convenience
> macro for that in drm headers, let's move it out. Subsequent patches
> will roll it out to other places.
> 
> Motivated by a discussion with Andy and Yisheng, who want to add
> another for_each_macro which would benefit from for_each_if() instead
> of hand-rolling it.
> 

Thanks, Daniel!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: NeilBrown <neilb@suse.com>
> Cc: Wei Wang <wvw@google.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Andrei Vagin <avagin@openvz.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Yisheng Xie <ysxie@foxmail.com>
> ---
>  include/drm/drmP.h     | 3 ---
>  include/linux/kernel.h | 3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index f7a19c2a7a80..05350424a4d3 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -110,7 +110,4 @@ static inline bool drm_can_sleep(void)
>  	return true;
>  }
>  
> -/* helper for handling conditionals in various for_each macros */
> -#define for_each_if(condition) if (!(condition)) {} else
> -
>  #endif
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 941dc0a5a877..4cb95ab9a5bc 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -71,6 +71,9 @@
>   */
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
>  
> +/* helper for handling conditionals in various for_each macros */
> +#define for_each_if(condition) if (!(condition)) {} else
> +
>  #define u64_to_user_ptr(x) (		\
>  {					\
>  	typecheck(u64, x);		\

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] kernel.h: Add for_each_if()

[Daniel Vetter]

To avoid compilers complainig about ambigious else blocks when putting
an if condition into a for_each macro one needs to invert the
condition and add a dummy else. We have a nice little convenience
macro for that in drm headers, let's move it out. Subsequent patches
will roll it out to other places.

The issue the compilers complain about are nested if with an else
block and no {} to disambiguate which if the else belongs to. The C
standard is clear, but in practice people forget:

if (foo)
	if (bar)
		/* something */
	else
		/* something else

The same can happen in a for_each macro when it also contains an if
condition at the end, except the compiler message is now really
confusing since there's only 1 if:

for_each_something()
	if (bar)
		/* something */
	else
		/* something else

The for_each_if() macro, by inverting the condition and adding an
else, avoids the compiler warning.

Motivated by a discussion with Andy and Yisheng, who want to add
another for_each_macro which would benefit from for_each_if() instead
of hand-rolling it.

Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: NeilBrown <neilb@suse.com>
Cc: Wei Wang <wvw@google.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Andrei Vagin <avagin@openvz.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Yisheng Xie <ysxie@foxmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
--
v2: Explain a bit better what this is good for, after the discussion
with Peter Z.
---
 include/drm/drmP.h     | 3 ---
 include/linux/kernel.h | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f7a19c2a7a80..05350424a4d3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -110,7 +110,4 @@ static inline bool drm_can_sleep(void)
 	return true;
 }
 
-/* helper for handling conditionals in various for_each macros */
-#define for_each_if(condition) if (!(condition)) {} else
-
 #endif
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 941dc0a5a877..4cb95ab9a5bc 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -71,6 +71,9 @@
  */
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
+/* helper for handling conditionals in various for_each macros */
+#define for_each_if(condition) if (!(condition)) {} else
+
 #define u64_to_user_ptr(x) (		\
 {					\
 	typecheck(u64, x);		\
-- 
2.18.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] kernel.h: Add for_each_if()

[Andy Shevchenko]

On Mon, 2018-07-09 at 18:25 +0200, Daniel Vetter wrote:

> v2: Explain a bit better what this is good for, after the discussion
> with Peter Z.

Since I have not been Cc'ed to your discussion there is another
weirdness which this macro prohibits, i.e.

for_each_blah() {
} else {
 ...blahblah...
}

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] kernel.h: Add for_each_if()

[Andrew Morton]

On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> To avoid compilers complainig about ambigious else blocks when putting
> an if condition into a for_each macro one needs to invert the
> condition and add a dummy else. We have a nice little convenience
> macro for that in drm headers, let's move it out. Subsequent patches
> will roll it out to other places.
> 
> The issue the compilers complain about are nested if with an else
> block and no {} to disambiguate which if the else belongs to. The C
> standard is clear, but in practice people forget:
> 
> if (foo)
> 	if (bar)
> 		/* something */
> 	else
> 		/* something else

um, yeah, don't do that.  Kernel coding style is very much to do

	if (foo) {
		if (bar)
			/* something */
		else
			/* something else
	}

And if not doing that generates a warning then, well, do that.

> The same can happen in a for_each macro when it also contains an if
> condition at the end, except the compiler message is now really
> confusing since there's only 1 if:
> 
> for_each_something()
> 	if (bar)
> 		/* something */
> 	else
> 		/* something else
> 
> The for_each_if() macro, by inverting the condition and adding an
> else, avoids the compiler warning.

Ditto.

> Motivated by a discussion with Andy and Yisheng, who want to add
> another for_each_macro which would benefit from for_each_if() instead
> of hand-rolling it.

Ditto.

> v2: Explain a bit better what this is good for, after the discussion
> with Peter Z.

Presumably the above was discussed in whatever-thread-that-was.

Re: [PATCH] kernel.h: Add for_each_if()

[Daniel Vetter]

On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > To avoid compilers complainig about ambigious else blocks when putting
> > an if condition into a for_each macro one needs to invert the
> > condition and add a dummy else. We have a nice little convenience
> > macro for that in drm headers, let's move it out. Subsequent patches
> > will roll it out to other places.
> > 
> > The issue the compilers complain about are nested if with an else
> > block and no {} to disambiguate which if the else belongs to. The C
> > standard is clear, but in practice people forget:
> > 
> > if (foo)
> > 	if (bar)
> > 		/* something */
> > 	else
> > 		/* something else
> 
> um, yeah, don't do that.  Kernel coding style is very much to do
> 
> 	if (foo) {
> 		if (bar)
> 			/* something */
> 		else
> 			/* something else
> 	}
> 
> And if not doing that generates a warning then, well, do that.
> 
> > The same can happen in a for_each macro when it also contains an if
> > condition at the end, except the compiler message is now really
> > confusing since there's only 1 if:
> > 
> > for_each_something()
> > 	if (bar)
> > 		/* something */
> > 	else
> > 		/* something else
> > 
> > The for_each_if() macro, by inverting the condition and adding an
> > else, avoids the compiler warning.
> 
> Ditto.
> 
> > Motivated by a discussion with Andy and Yisheng, who want to add
> > another for_each_macro which would benefit from for_each_if() instead
> > of hand-rolling it.
> 
> Ditto.
> 
> > v2: Explain a bit better what this is good for, after the discussion
> > with Peter Z.
> 
> Presumably the above was discussed in whatever-thread-that-was.

So there's a bunch of open coded versions of this already in kernel
headers (at least the ones I've found). Not counting the big pile of
existing users in drm. They are all wrong and should be reverted to a
plain if? That why there's a bunch more patches in this series.

And yes I made it clear in the discussion that if you sprinkle enough {}
there's no warning, should have probably captured this here.

Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
can stop bothering with this.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel.h: Add for_each_if()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3806 bytes --]

On Tue, Jul 10 2018, Daniel Vetter wrote:

> On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
>> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> 
>> > To avoid compilers complainig about ambigious else blocks when putting
>> > an if condition into a for_each macro one needs to invert the
>> > condition and add a dummy else. We have a nice little convenience
>> > macro for that in drm headers, let's move it out. Subsequent patches
>> > will roll it out to other places.
>> > 
>> > The issue the compilers complain about are nested if with an else
>> > block and no {} to disambiguate which if the else belongs to. The C
>> > standard is clear, but in practice people forget:
>> > 
>> > if (foo)
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> 
>> um, yeah, don't do that.  Kernel coding style is very much to do
>> 
>> 	if (foo) {
>> 		if (bar)
>> 			/* something */
>> 		else
>> 			/* something else
>> 	}
>> 
>> And if not doing that generates a warning then, well, do that.
>> 
>> > The same can happen in a for_each macro when it also contains an if
>> > condition at the end, except the compiler message is now really
>> > confusing since there's only 1 if:
>> > 
>> > for_each_something()
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> > 
>> > The for_each_if() macro, by inverting the condition and adding an
>> > else, avoids the compiler warning.
>> 
>> Ditto.
>> 
>> > Motivated by a discussion with Andy and Yisheng, who want to add
>> > another for_each_macro which would benefit from for_each_if() instead
>> > of hand-rolling it.
>> 
>> Ditto.
>> 
>> > v2: Explain a bit better what this is good for, after the discussion
>> > with Peter Z.
>> 
>> Presumably the above was discussed in whatever-thread-that-was.
>
> So there's a bunch of open coded versions of this already in kernel
> headers (at least the ones I've found). Not counting the big pile of
> existing users in drm. They are all wrong and should be reverted to a
> plain if? That why there's a bunch more patches in this series.
>
> And yes I made it clear in the discussion that if you sprinkle enough {}
> there's no warning, should have probably captured this here.
>
> Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
> can stop bothering with this.

I think is it problematic to have macros like

#define for_each_foo(...) for (......) if (....)

because
   for_each_foo(...)
      if (x) ....; else ......;

is handled badly.
So in that sense, your work seems like a good thing.

However it isn't clear to me that you need a new macro.
The above macro could simply be changed to

#define for_each_foo(...) for (......) if (!....);else

Clearly people don't always think to do this, but would adding a macro
help people to think?

If we were to have a macro, it isn't clear to me that for_each_if() is a
good name.
Every other macro I've seen that starts "for_each_" causes the body to
loop.  This one doesn't.  If someone doesn't know what for_each_if()
does and sees it in code, they are unlikely to jump to the right
conclusion.
I would suggest that "__if" would be a better choice.  I think most
people would guess that means "like 'if', but a bit different", which is
fairly accurate.

I think the only sure way to avoid bad macros being written is to teach
some static checker to warn about any macro with a dangling "if".
Possibly checkpatch.pl could do that (but I'm not volunteering).

I do agree that it would be good to do something, and if people find
for_each_fi() to actually reduce the number of poorly written macros,
then I don't object to it.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] kernel.h: Add for_each_if()

[Daniel Vetter]

On Tue, Jul 10, 2018 at 12:32 PM, NeilBrown <neilb@suse.com> wrote:
> On Tue, Jul 10 2018, Daniel Vetter wrote:
>
>> On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
>>> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>
>>> > To avoid compilers complainig about ambigious else blocks when putting
>>> > an if condition into a for_each macro one needs to invert the
>>> > condition and add a dummy else. We have a nice little convenience
>>> > macro for that in drm headers, let's move it out. Subsequent patches
>>> > will roll it out to other places.
>>> >
>>> > The issue the compilers complain about are nested if with an else
>>> > block and no {} to disambiguate which if the else belongs to. The C
>>> > standard is clear, but in practice people forget:
>>> >
>>> > if (foo)
>>> >    if (bar)
>>> >            /* something */
>>> >    else
>>> >            /* something else
>>>
>>> um, yeah, don't do that.  Kernel coding style is very much to do
>>>
>>>      if (foo) {
>>>              if (bar)
>>>                      /* something */
>>>              else
>>>                      /* something else
>>>      }
>>>
>>> And if not doing that generates a warning then, well, do that.
>>>
>>> > The same can happen in a for_each macro when it also contains an if
>>> > condition at the end, except the compiler message is now really
>>> > confusing since there's only 1 if:
>>> >
>>> > for_each_something()
>>> >    if (bar)
>>> >            /* something */
>>> >    else
>>> >            /* something else
>>> >
>>> > The for_each_if() macro, by inverting the condition and adding an
>>> > else, avoids the compiler warning.
>>>
>>> Ditto.
>>>
>>> > Motivated by a discussion with Andy and Yisheng, who want to add
>>> > another for_each_macro which would benefit from for_each_if() instead
>>> > of hand-rolling it.
>>>
>>> Ditto.
>>>
>>> > v2: Explain a bit better what this is good for, after the discussion
>>> > with Peter Z.
>>>
>>> Presumably the above was discussed in whatever-thread-that-was.
>>
>> So there's a bunch of open coded versions of this already in kernel
>> headers (at least the ones I've found). Not counting the big pile of
>> existing users in drm. They are all wrong and should be reverted to a
>> plain if? That why there's a bunch more patches in this series.
>>
>> And yes I made it clear in the discussion that if you sprinkle enough {}
>> there's no warning, should have probably captured this here.
>>
>> Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
>> can stop bothering with this.
>
> I think is it problematic to have macros like
>
> #define for_each_foo(...) for (......) if (....)
>
> because
>    for_each_foo(...)
>       if (x) ....; else ......;
>
> is handled badly.
> So in that sense, your work seems like a good thing.
>
> However it isn't clear to me that you need a new macro.
> The above macro could simply be changed to
>
> #define for_each_foo(...) for (......) if (!....);else
>
> Clearly people don't always think to do this, but would adding a macro
> help people to think?
>
> If we were to have a macro, it isn't clear to me that for_each_if() is a
> good name.
> Every other macro I've seen that starts "for_each_" causes the body to
> loop.  This one doesn't.  If someone doesn't know what for_each_if()
> does and sees it in code, they are unlikely to jump to the right
> conclusion.
> I would suggest that "__if" would be a better choice.  I think most
> people would guess that means "like 'if', but a bit different", which is
> fairly accurate.
>
> I think the only sure way to avoid bad macros being written is to teach
> some static checker to warn about any macro with a dangling "if".
> Possibly checkpatch.pl could do that (but I'm not volunteering).
>
> I do agree that it would be good to do something, and if people find
> for_each_fi() to actually reduce the number of poorly written macros,
> then I don't object to it.

There's also the proposal of if_noelse() which I think fares a bit
better than the __if().

But I still have the situation that a bunch of maintainers acked this
and Andrew Morton defacto nacked it, which I guess means I'll keep the
macro in drm? The common way to go about this seems to be to just push
the patch series with the ack in some pull request to Linus and ignore
the people who raised questions, but not really my thing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] kernel.h: Add for_each_if()

[Andrew Morton]

On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:

> But I still have the situation that a bunch of maintainers acked this
> and Andrew Morton defacto nacked it, which I guess means I'll keep the
> macro in drm? The common way to go about this seems to be to just push
> the patch series with the ack in some pull request to Linus and ignore
> the people who raised questions, but not really my thing.

Heh.

But, am I wrong?  Code which uses regular kernel style doesn't have
these issues.  We shouldn't be enabling irregular style - we should be
making such sites more regular.  The fact that the compiler generates a
nice warning in some cases simply helps us with that.



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] kernel.h: Add for_each_if()

[Daniel Vetter]

On Thu, Jul 12, 2018 at 1:05 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>> But I still have the situation that a bunch of maintainers acked this
>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>> macro in drm? The common way to go about this seems to be to just push
>> the patch series with the ack in some pull request to Linus and ignore
>> the people who raised questions, but not really my thing.
>
> Heh.
>
> But, am I wrong?  Code which uses regular kernel style doesn't have
> these issues.  We shouldn't be enabling irregular style - we should be
> making such sites more regular.  The fact that the compiler generates a
> nice warning in some cases simply helps us with that.

Yes liberal sprinkling of {} per coding style fixes it too. But given
that the if (!cond) ; else pattern is fairly common even outside of
drm it seems like not just graphics people think that little bit of
added robustness in iterator macros is worth it. Only reason I
bothered with this is because I've seen another open-coded version of
this pattern fly by.

Anyway I'm going on vacations for a few weeks now. Andy/Yisheng, I
guess if you want this it's up to you to haggle for a consensus around
this. If that doesn't happen I'm happy to keep it in drm headers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] kernel.h: Add for_each_if()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Wed, Jul 11 2018, Andrew Morton wrote:

> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> But I still have the situation that a bunch of maintainers acked this
>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>> macro in drm? The common way to go about this seems to be to just push
>> the patch series with the ack in some pull request to Linus and ignore
>> the people who raised questions, but not really my thing.
>
> Heh.
>
> But, am I wrong?  Code which uses regular kernel style doesn't have
> these issues.  We shouldn't be enabling irregular style - we should be
> making such sites more regular.  The fact that the compiler generates a
> nice warning in some cases simply helps us with that.

I think you are wrong .... or at least, not completely correct.

I think it is perfectly acceptable in Linux to have code like:

  for (....)
  	if (x)
        	something();
        else
        	something_else();

Would you agree?  If not, then I'm the one who is wrong.  Otherwise....

The problem is that for certain poorly written for_each_foo() macros,
such as blkg_for_each_descendant_pre() (and several others identified in
this patch series), writing

   blkg_for_each_descendant_pre(...)
     	if (x)
        	something();
        else
        	something_else();

will trigger a compiler warning.  This is inconsistent with the
behaviour of a simple "for".
So I do think that the macros should be fixed, and I don't think that
sprinkling extra braces is an appropriate response.

I'm not personally convinced that writing
   if_no_else(cond)
is easier than just writing
   if (!(cond)); else

in these macros, but I do think that the macros should be fixed and
maybe this is the path-of-least-resistance to getting it done.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] kernel.h: Add for_each_if()

[Randy Dunlap]

On 07/13/2018 04:37 PM, NeilBrown wrote:
> On Wed, Jul 11 2018, Andrew Morton wrote:
> 
>> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>>> But I still have the situation that a bunch of maintainers acked this
>>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>>> macro in drm? The common way to go about this seems to be to just push
>>> the patch series with the ack in some pull request to Linus and ignore
>>> the people who raised questions, but not really my thing.
>>
>> Heh.
>>
>> But, am I wrong?  Code which uses regular kernel style doesn't have
>> these issues.  We shouldn't be enabling irregular style - we should be
>> making such sites more regular.  The fact that the compiler generates a
>> nice warning in some cases simply helps us with that.
> 
> I think you are wrong .... or at least, not completely correct.
> 
> I think it is perfectly acceptable in Linux to have code like:
> 
>   for (....)
>   	if (x)
>         	something();
>         else
>         	something_else();
> 
> Would you agree?  If not, then I'm the one who is wrong.  Otherwise....

coding-style.rst says:
Also, use braces when a loop contains more than a single simple statement:


> The problem is that for certain poorly written for_each_foo() macros,
> such as blkg_for_each_descendant_pre() (and several others identified in
> this patch series), writing
> 
>    blkg_for_each_descendant_pre(...)
>      	if (x)
>         	something();
>         else
>         	something_else();
> 
> will trigger a compiler warning.  This is inconsistent with the
> behaviour of a simple "for".
> So I do think that the macros should be fixed, and I don't think that
> sprinkling extra braces is an appropriate response.
> 
> I'm not personally convinced that writing
>    if_no_else(cond)
> is easier than just writing
>    if (!(cond)); else

agreed.

> in these macros, but I do think that the macros should be fixed and
> maybe this is the path-of-least-resistance to getting it done.

I'm not opposed to fixing some macros, but some of these macros are just
ease-of-less-typing shortcuts.  They don't improve readability at all;
they harm it.  (of course, that is just one opinion :)



-- 
~Randy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] kernel.h: Add for_each_if()

[Andy Shevchenko]

On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
> On 07/13/2018 04:37 PM, NeilBrown wrote:

> 
> coding-style.rst says:
> Also, use braces when a loop contains more than a single simple
> statement:

Independently on a) would we use some macro for condition, or b) fix
macros against this kind of nested conditions, there is another
weirdness we would like to avoid, i.e.

for_each_foo() {
 ...
} else {
 ...
}

It is written according to coding style, but too much weird.

So, summarize this discussion I think we would
- keep for_each_if() in DRM subsystem alone
- fix macros which are using positive condition 'if (cond)' by replacing
with 'if (!cond) {} else' form for sake of robustness.

Do you agree on that?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] kernel.h: Add for_each_if()

[Randy Dunlap]

On 07/16/2018 01:11 AM, Andy Shevchenko wrote:
> On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
>> On 07/13/2018 04:37 PM, NeilBrown wrote:
> 
>>
>> coding-style.rst says:
>> Also, use braces when a loop contains more than a single simple
>> statement:
> 
> Independently on a) would we use some macro for condition, or b) fix
> macros against this kind of nested conditions, there is another
> weirdness we would like to avoid, i.e.
> 
> for_each_foo() {
>  ...
> } else {
>  ...
> }
> 
> It is written according to coding style, but too much weird.

Yeah, that's odd.  Looks like else matches the for loop. (!)

> So, summarize this discussion I think we would
> - keep for_each_if() in DRM subsystem alone
> - fix macros which are using positive condition 'if (cond)' by replacing
> with 'if (!cond) {} else' form for sake of robustness.
> 
> Do you agree on that?

Sure, both of those sound good to me.

thanks,
-- 
~Randy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] kernel.h: Add for_each_if()

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Mon, Jul 16 2018, Andy Shevchenko wrote:

> On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
>> On 07/13/2018 04:37 PM, NeilBrown wrote:
>
>> 
>> coding-style.rst says:
>> Also, use braces when a loop contains more than a single simple
>> statement:
>
> Independently on a) would we use some macro for condition, or b) fix
> macros against this kind of nested conditions, there is another
> weirdness we would like to avoid, i.e.
>
> for_each_foo() {
>  ...
> } else {
>  ...
> }
>
> It is written according to coding style, but too much weird.

Agreed.  Too weird.

>
> So, summarize this discussion I think we would
> - keep for_each_if() in DRM subsystem alone
> - fix macros which are using positive condition 'if (cond)' by replacing
> with 'if (!cond) {} else' form for sake of robustness.
>
> Do you agree on that?

I agree.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]