Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-25 Thread Miguel Ojeda 
On Wed, Nov 25, 2020 at 5:24 PM Jakub Kicinski  wrote:
>
> And just to spell it out,
>
> case ENUM_VALUE1:
> bla();
> break;
> case ENUM_VALUE2:
> bla();
> default:
> break;
>
> is a fairly idiomatic way of indicating that not all values of the enum
> are expected to be handled by the switch statement.

It looks like a benign typo to me -- `ENUM_VALUE2` does not follow the
same pattern like `ENUM_VALUE1`. To me, the presence of the `default`
is what indicates (explicitly) that not everything is handled.

> Applying a real patch set and then getting a few follow ups the next day
> for trivial coding things like fallthrough missing or static missing,
> just because I didn't have the full range of compilers to check with
> before applying makes me feel pretty shitty, like I'm not doing a good
> job. YMMV.

The number of compilers, checkers, static analyzers, tests, etc. we
use keeps going up. That, indeed, means maintainers will miss more
things (unless maintainers do more work than before). But catching
bugs before they happen is *not* a bad thing.

Perhaps we could encourage more rebasing in -next (while still giving
credit to bots and testers) to avoid having many fixing commits
afterwards, but that is orthogonal.

I really don't think we should encourage the feeling that a maintainer
is doing a bad job if they don't catch everything on their reviews.
Any review is worth it. Maintainers, in the end, are just the
"guaranteed" reviewers that decide when the code looks reasonable
enough. They should definitely not feel pressured to be perfect.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-24 Thread Miguel Ojeda 
On Mon, Nov 23, 2020 at 9:38 PM James Bottomley
 wrote:
>
> So you think a one line patch should take one minute to produce ... I
> really don't think that's grounded in reality.

No, I have not said that. Please don't put words in my mouth (again).

I have said *authoring* lines of *this* kind takes a minute per line.
Specifically: lines fixing the fallthrough warning mechanically and
repeatedly where the compiler tells you to, and doing so full-time for
a month.

For instance, take the following one from Gustavo. Are you really
saying it takes 12 minutes (your number) to write that `break;`?

diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c
index 24cc445169e2..a3e0fb5b8671 100644
--- a/drivers/gpu/drm/via/via_irq.c
+++ b/drivers/gpu/drm/via/via_irq.c
@@ -364,6 +364,7 @@ int via_wait_irq(struct drm_device *dev, void
*data, struct drm_file *file_priv)
irqwait->request.sequence +=
atomic_read(_irq->irq_received);
irqwait->request.type &= ~_DRM_VBLANK_RELATIVE;
+   break;
case VIA_IRQ_ABSOLUTE:
break;
default:

>  I suppose a one line
> patch only takes a minute to merge with b4 if no-one reviews or tests
> it, but that's not really desirable.

I have not said that either. I said reviewing and merging those are
noise compared to any complex patch. Testing should be done by the
author comparing codegen.

> Part of what I'm trying to measure is the "and useful" bit because
> that's not a given.

It is useful since it makes intent clear. It also catches actual bugs,
which is even more valuable.

> Well, you know, subsystems are very different in terms of the amount of
> patches a maintainer has to process per release cycle of the kernel.
> If a maintainer is close to capacity, additional patches, however
> trivial, become a problem.  If a maintainer has spare cycles, trivial
> patches may look easy.

First of all, voluntary maintainers choose their own workload.
Furthermore, we already measure capacity in the `MAINTAINERS` file:
maintainers can state they can only handle a few patches. Finally, if
someone does not have time for a trivial patch, they are very unlikely
to have any time to review big ones.

> You seem to be saying that because you find it easy to merge trivial
> patches, everyone should.

Again, I have not said anything of the sort.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-23 Thread Miguel Ojeda 
On Mon, Nov 23, 2020 at 4:58 PM James Bottomley
 wrote:
>
> Well, I used git.  It says that as of today in Linus' tree we have 889
> patches related to fall throughs and the first series went in in
> october 2017 ... ignoring a couple of outliers back to February.

I can see ~10k insertions over ~1k commits and 15 years that mention a
fallthrough in the entire repo. That is including some commits (like
the biggest one, 960 insertions) that have nothing to do with C
fallthrough. A single kernel release has an order of magnitude more
changes than this...

But if we do the math, for an author, at even 1 minute per line change
and assuming nothing can be automated at all, it would take 1 month of
work. For maintainers, a couple of trivial lines is noise compared to
many other patches.

In fact, this discussion probably took more time than the time it
would take to review the 200 lines. :-)

> We're also complaining about the inability to recruit maintainers:
>
> https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/
>
> And burn out:
>
> http://antirez.com/news/129

Accepting trivial and useful 1-line patches is not what makes a
voluntary maintainer quit... Thankless work with demanding deadlines is.

> The whole crux of your argument seems to be maintainers' time isn't
> important so we should accept all trivial patches

I have not said that, at all. In fact, I am a voluntary one and I
welcome patches like this. It takes very little effort on my side to
review and it helps the kernel overall. Paid maintainers are the ones
that can take care of big features/reviews.

> What I'm actually trying to articulate is a way of measuring value of
> the patch vs cost ... it has nothing really to do with who foots the
> actual bill.

I understand your point, but you were the one putting it in terms of a
junior FTE. In my view, 1 month-work (worst case) is very much worth
removing a class of errors from a critical codebase.

> One thesis I'm actually starting to formulate is that this continual
> devaluing of maintainers is why we have so much difficulty keeping and
> recruiting them.

That may very well be true, but I don't feel anybody has devalued
maintainers in this discussion.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-23 Thread Miguel Ojeda 
On Sun, Nov 22, 2020 at 11:36 PM James Bottomley
 wrote:
>
> Well, it seems to be three years of someone's time plus the maintainer
> review time and series disruption of nearly a thousand patches.  Let's
> be conservative and assume the producer worked about 30% on the series
> and it takes about 5-10 minutes per patch to review, merge and for
> others to rework existing series.  So let's say it's cost a person year
> of a relatively junior engineer producing the patches and say 100h of
> review and application time.  The latter is likely the big ticket item
> because it's what we have in least supply in the kernel (even though
> it's 20x vs the producer time).

How are you arriving at such numbers? It is a total of ~200 trivial lines.

> It's not about the risk of the changes it's about the cost of
> implementing them.  Even if you discount the producer time (which
> someone gets to pay for, and if I were the engineering manager, I'd be
> unhappy about), the review/merge/rework time is pretty significant in
> exchange for six minor bug fixes.  Fine, when a new compiler warning
> comes along it's certainly reasonable to see if we can benefit from it
> and the fact that the compiler people think it's worthwhile is enough
> evidence to assume this initially.  But at some point you have to ask
> whether that assumption is supported by the evidence we've accumulated
> over the time we've been using it.  And if the evidence doesn't support
> it perhaps it is time to stop the experiment.

Maintainers routinely review 1-line trivial patches, not to mention
internal API changes, etc.

If some company does not want to pay for that, that's fine, but they
don't get to be maintainers and claim `Supported`.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-22 Thread Miguel Ojeda 
On Sun, Nov 22, 2020 at 7:22 PM James Bottomley
 wrote:
>
> Well, it's a problem in an error leg, sure, but it's not a really
> compelling reason for a 141 patch series, is it?  All that fixing this
> error will do is get the driver to print "oh dear there's a problem"
> under four more conditions than it previously did.
>
> We've been at this for three years now with nearly a thousand patches,
> firstly marking all the fall throughs with /* fall through */ and later
> changing it to fallthrough.  At some point we do have to ask if the
> effort is commensurate with the protection afforded.  Please tell me
> our reward for all this effort isn't a single missing error print.

It isn't that much effort, isn't it? Plus we need to take into account
the future mistakes that it might prevent, too. So even if there were
zero problems found so far, it is still a positive change.

I would agree if these changes were high risk, though; but they are
almost trivial.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 000/141] Fix fall-through warnings for Clang

2020-11-20 Thread Miguel Ojeda 
Hi Gustavo,

On Fri, Nov 20, 2020 at 7:21 PM Gustavo A. R. Silva
 wrote:
>
> Hi all,
>
> This series aims to fix almost all remaining fall-through warnings in
> order to enable -Wimplicit-fallthrough for Clang.

Thanks for this.

Since this warning is reliable in both/all compilers and we are
eventually getting rid of all the cases, what about going even further
and making it an error right after?

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

2018-10-20 Thread Miguel Ojeda 
Hi Dan,

On Sat, Oct 20, 2018 at 9:22 PM Dan Carpenter  wrote:
>
> On Sat, Oct 20, 2018 at 04:42:07PM +0200, Miguel Ojeda wrote:
> > Using an attribute is indeed better whenever possible. In C++17 it is
> > an standard attribute and there have been proposals to include some of
> > them for C as well since 2016 at least, e.g. the latest for
> > fallthrough at:
> >
> >   http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf
> >
> > I have taken a quick look into supporting it (typing it here to save
> > it on the mailing list :-), and we have:
> >
> >   * gcc >= 7.1 supports -Wimplicit-fallthrough with
> > __attribute__((fallthrough)), the comment syntax and the C++
> > [[fallthrough]] syntax.
> >   * gcc < 7.1 complains about empty declarations (it does not know
> > about attributes for null statements) and also
> > -Wdeclaration-after-statement.
>
> I'm not sure I understand about empty decalarations.  The idea is that
> we would do:

That paragraph tried to explain that gcc < 7.1 did not know about
__attribute__((fallthrough)); i.e. that everything was introduced in
gcc 7.1.

Anyway, the conclusion was a neuron misfiring of mine -- in my mind I
was thinking clang supported the comment syntax (when I clearly wrote
that it didn't). Never mind --- thanks for pointing it out!

>
> case 3:
> frob();
> __fall_through();
> case 4:
> frob();
>
> #if GCC > 7.1
> #define __fall_through() __attribute__((fallthrough))
> #else
> #define __fall_through()
> #endif

Yes, of course, that is what we do for other attributes -- actually in
-next we have pending a better way for checking, using
__has_attribute:

  #if __has_attribute(fallthrough)
  #define __fallthrough __attribute__((fallthrough))
  #else
  #define __fallthrough
  #endif

>
> So long as GCC and static analysis tools understand about the attribute
> that's enought to catch the bugs. No one cares what clang and icc do.

Not so sure about that -- there are quite some people looking forward
to building Linux with clang, even if only to have another compiler to
check for warnings and to use the clang/llvm-related tools (and to
write new ones).

> We would just disable the fall through warnings on those until they are
> updated to support the fallthrough attribute.

You mean if they start supporting the warning but not the attribute? I
don't think that would be likely (i.e. if clang enables the warning on
C mode, they will have to introduce a way to suppress it; which should
be the attribute (at least), since they typically like to be
compatible with gcc and since they already support it in C++ >= 11),
but everything is possible.

>
> We wouldn't update all the fall through comments until later, but going
> forward people could use the __fall_through() macro if they want.

Agreed. I will introduce it in the compiler-attributes tree -- should
be there for -rc1 if no one complains. CC'ing all of you in the patch.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/5] Drivers: hv: kvp: Fix the recent regression caused by incorrect clean-up

2018-10-20 Thread Miguel Ojeda 
+On Wed, Oct 17, 2018 at 8:25 AM Dan Carpenter  wrote:
>
> It's not common at all.  It should be wrapped in a macro and put into
> compiler.h.
>
> But I hope it does become adopted.  It's better than randomly grepping
> for non-standard comments.

Using an attribute is indeed better whenever possible. In C++17 it is
an standard attribute and there have been proposals to include some of
them for C as well since 2016 at least, e.g. the latest for
fallthrough at:

  http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2268.pdf

I have taken a quick look into supporting it (typing it here to save
it on the mailing list :-), and we have:

  * gcc >= 7.1 supports -Wimplicit-fallthrough with
__attribute__((fallthrough)), the comment syntax and the C++
[[fallthrough]] syntax.
  * gcc < 7.1 complains about empty declarations (it does not know
about attributes for null statements) and also
-Wdeclaration-after-statement.
  * clang 7 supports -Wimplicit-fallthrough (not enabled in
-Wall/extra/pedantic like gcc, though) but *only* in C++ mode and with
the C++ syntax [[fallthrough]]. In other words, in C mode, no syntax
works and no diagnostics are emitted. It complains about
Wmissing-declarations. [IMO they should allow the __attribute__ syntax
for fallthrough (and enable it on C mode) to be compatible with gcc.
Maybe they are simply waiting for the C2x attributes... :-)]
  * icc 19 does not know about -Wimplicit-fallthrough at all (but
seems to allow [[fallthrough]] on C++17 mode to comply with the
standard).

Therefore, the only improvement we could do right now is starting to
use the attribute for gcc > 7.1, and a comment for everybody else.
However, even if that was worth the trouble of changing the 2500+
instances of fall through markings that we have, comments are replaced
before the preprocessor stage, so we would need some (probably
non-portable) macro magic.

So, I would say, let's revisit this again in a few years. Possibly,
when we move the minimum version to gcc 7.1, clang and icc may both
support the fallthrough warning in C mode; so that we can always use
the __attribute__((fallthrough)) unconditionally under a __fallthrough
#define.

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array

2018-08-30 Thread Miguel Ojeda 
Hi Janusz,

On Wed, Aug 29, 2018 at 10:48 PM, Janusz Krzysztofik
 wrote:
> Most users of get/set array functions iterate consecutive bits of data,
> usually a single integer, while processing array of results obtained
> from, or building an array of values to be passed to those functions.
> Save time wasted on those iterations by changing the functions' API to
> accept bitmaps.
>
> All current users are updated as well.
>
> More benefits from the change are expected as soon as planned support
> for accepting/passing those bitmaps directly from/to respective GPIO
> chip callbacks if applicable is implemented.
>
> Cc: Jonathan Corbet 
> Cc: Miguel Ojeda Sandonis 
> Cc: Peter Korsgaard 
> Cc: Peter Rosin 
> Cc: Andrew Lunn 
> Cc: Florian Fainelli 
> Cc: "David S. Miller" 
> Cc: Dominik Brodowski 
> Cc: Kishon Vijay Abraham I 
> Cc: Lars-Peter Clausen 
> Cc: Michael Hennerich 
> Cc: Jonathan Cameron 
> Cc: Hartmut Knaack 
> Cc: Peter Meerwald-Stadler 
> Cc: Greg Kroah-Hartman 
> Cc: Jiri Slaby 
> Signed-off-by: Janusz Krzysztofik 
> Acked-by: Ulf Hansson 
> ---
>  Documentation/driver-api/gpio/consumer.rst  | 22 
>  drivers/auxdisplay/hd44780.c| 52 +
>  drivers/bus/ts-nbus.c   | 19 ++-
>  drivers/gpio/gpio-max3191x.c| 17 +++---
>  drivers/gpio/gpiolib.c  | 86 
> +++--
>  drivers/gpio/gpiolib.h  |  4 +-
>  drivers/i2c/muxes/i2c-mux-gpio.c|  8 +--
>  drivers/mmc/core/pwrseq_simple.c| 13 ++---
>  drivers/mux/gpio.c  |  9 +--
>  drivers/net/phy/mdio-mux-gpio.c |  3 +-
>  drivers/pcmcia/soc_common.c | 11 ++--
>  drivers/phy/motorola/phy-mapphone-mdm6600.c | 17 +++---
>  drivers/staging/iio/adc/ad7606.c|  9 +--
>  drivers/tty/serial/serial_mctrl_gpio.c  |  7 ++-
>  include/linux/gpio/consumer.h   | 18 +++---
>  15 files changed, 140 insertions(+), 155 deletions(-)
>
> diff --git a/Documentation/driver-api/gpio/consumer.rst 
> b/Documentation/driver-api/gpio/consumer.rst
> index aa03f389d41d..ed68042ddccf 100644
> --- a/Documentation/driver-api/gpio/consumer.rst
> +++ b/Documentation/driver-api/gpio/consumer.rst
> @@ -323,29 +323,29 @@ The following functions get or set the values of an 
> array of GPIOs::
>
> int gpiod_get_array_value(unsigned int array_size,
>   struct gpio_desc **desc_array,
> - int *value_array);
> + unsigned long *value_bitmap);
> int gpiod_get_raw_array_value(unsigned int array_size,
>   struct gpio_desc **desc_array,
> - int *value_array);
> + unsigned long *value_bitmap);
> int gpiod_get_array_value_cansleep(unsigned int array_size,
>struct gpio_desc **desc_array,
> -  int *value_array);
> +  unsigned long *value_bitmap);
> int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
>struct gpio_desc **desc_array,
> -  int *value_array);
> +  unsigned long *value_bitmap);
>
> void gpiod_set_array_value(unsigned int array_size,
>struct gpio_desc **desc_array,
> -  int *value_array)
> +  unsigned long *value_bitmap)
> void gpiod_set_raw_array_value(unsigned int array_size,
>struct gpio_desc **desc_array,
> -  int *value_array)
> +  unsigned long *value_bitmap)
> void gpiod_set_array_value_cansleep(unsigned int array_size,
> struct gpio_desc **desc_array,
> -   int *value_array)
> +   unsigned long *value_bitmap)
> void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
> struct gpio_desc **desc_array,
> -   int *value_array)
> +   unsigned long *value_bitmap)
>
>  The array can be an arbitrary set of GPIOs. The functions will try to access
>  GPIOs belonging to the same bank or chip

Re: [RFC RFT PATCH v4 1/4] gpiolib: Pass bitmaps, not integer arrays, to get/set array

2018-08-29 Thread Miguel Ojeda 
Hi Janusz,

On Tue, Aug 21, 2018 at 1:43 AM, Janusz Krzysztofik  wrote:
> Most users of get/set array functions iterate consecutive bits of data,
> usually a single integer, while or processing array of results obtained
> from or building an array of values to be passed to those functions.
> Save time wasted on those iterations by changing the functions' API to
> accept bitmaps.
>
> All current users are updated as well.
>
> More benefits from the change are expected as soon as planned support
> for accepting/passing those bitmaps directly from/to respective GPIO
> chip callbacks if applicable is implemented.
>
> Signed-off-by: Janusz Krzysztofik 
> ---
>  Documentation/driver-api/gpio/consumer.rst  | 22 
>  drivers/auxdisplay/hd44780.c| 52 +

[CC'ing Willy and Geert for hd44780]

> diff --git a/drivers/auxdisplay/hd44780.c b/drivers/auxdisplay/hd44780.c
> index f1a42f0f1ded..d340473aa142 100644
> --- a/drivers/auxdisplay/hd44780.c
> +++ b/drivers/auxdisplay/hd44780.c
> @@ -62,20 +62,19 @@ static void hd44780_strobe_gpio(struct hd44780 *hd)
>  /* write to an LCD panel register in 8 bit GPIO mode */
>  static void hd44780_write_gpio8(struct hd44780 *hd, u8 val, unsigned int rs)
>  {
> -   int values[10]; /* for DATA[0-7], RS, RW */
> -   unsigned int i, n;
> +   unsigned long value_bitmap[1];  /* for DATA[0-7], RS, RW */

Why [1]? I understand it is because in other cases it may be more than
one, but...

> +   unsigned int n;
>
> -   for (i = 0; i < 8; i++)
> -   values[PIN_DATA0 + i] = !!(val & BIT(i));
> -   values[PIN_CTRL_RS] = rs;
> +   value_bitmap[0] = val;
> +   __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 9;
> if (hd->pins[PIN_CTRL_RW]) {
> -   values[PIN_CTRL_RW] = 0;
> +   __clear_bit(PIN_CTRL_RW, value_bitmap);
> n++;
> }
>
> /* Present the data to the port */
> -   gpiod_set_array_value_cansleep(n, >pins[PIN_DATA0], values);
> +   gpiod_set_array_value_cansleep(n, >pins[PIN_DATA0], value_bitmap);
>
> hd44780_strobe_gpio(hd);
>  }
> @@ -83,32 +82,31 @@ static void hd44780_write_gpio8(struct hd44780 *hd, u8 
> val, unsigned int rs)
>  /* write to an LCD panel register in 4 bit GPIO mode */
>  static void hd44780_write_gpio4(struct hd44780 *hd, u8 val, unsigned int rs)
>  {
> -   int values[10]; /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
> -   unsigned int i, n;
> +   /* for DATA[0-7], RS, RW, but DATA[0-3] is unused */
> +   unsigned long value_bitmap[0];

This one is even more strange... :-)

> +   unsigned int n;
>
> /* High nibble + RS, RW */
> -   for (i = 4; i < 8; i++)
> -   values[PIN_DATA0 + i] = !!(val & BIT(i));
> -   values[PIN_CTRL_RS] = rs;
> +   value_bitmap[0] = val;
> +   __assign_bit(PIN_CTRL_RS, value_bitmap, rs);
> n = 5;
> if (hd->pins[PIN_CTRL_RW]) {
> -   values[PIN_CTRL_RW] = 0;
> +   __clear_bit(PIN_CTRL_RW, value_bitmap);
> n++;
> }
> +   value_bitmap[0] = value_bitmap[0] >> PIN_DATA4;

Maybe >>=?

>
> /* Present the data to the port */
> -   gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4],
> -  [PIN_DATA4]);
> +   gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4], value_bitmap);
>
> hd44780_strobe_gpio(hd);
>
> /* Low nibble */
> -   for (i = 0; i < 4; i++)
> -   values[PIN_DATA4 + i] = !!(val & BIT(i));
> +   value_bitmap[0] &= ~((1 << PIN_DATA4) - 1);
> +   value_bitmap[0] |= val & ~((1 << PIN_DATA4) - 1);

Are you sure this is correct? You are basically doing an or of
value_bitmap and val and clearing the low-nibble.

>
> /* Present the data to the port */
> -   gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4],
> -  [PIN_DATA4]);
> +   gpiod_set_array_value_cansleep(n, >pins[PIN_DATA4], value_bitmap);
>
> hd44780_strobe_gpio(hd);
>  }

Cheers,
Miguel
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel