Re: [PATCH 1/1] auxdisplay: Add I2C gpio expander example
On Wed, Jan 6, 2021 at 12:39 PM Ralf Schlatterbeck wrote: > > The hd44780 displays are often used with pcf8574 based I/O expanders. > Add example to documentation. > > Signed-off-by: Ralf Schlatterbeck Since Geert suggested it, it is customary to write Suggested-by: Geert Uytterhoeven above your signature. Rob, if you are taking this on your tree: Acked-by: Miguel Ojeda Otherwise, I will pick it up. Cheers, Miguel
Re: [PATCH 0/1] auxdisplay: Add I2C gpio expander example
On Wed, Jan 6, 2021 at 1:12 PM Miguel Ojeda wrote: > > - Normally, you will want to use scripts/get_maintainer.pl to know > to whom send a given change. In this case, I am Cc'ing Rob, Geert and > the devicetree mailing list which were missing. (Actually Cc'ing Rob.) Cheers, Miguel
Re: [PATCH 0/1] auxdisplay: Add I2C gpio expander example
On Wed, Jan 6, 2021 at 12:37 PM Ralf Schlatterbeck wrote: > > The Hitachi HD44780 is often used together with a PCF8574 based I2C > I/O expander. It was non-obvious to me that the existing parallel > connected version of the auxdisplay driver can already handle this > configuration with appropriate device tree magic. This patch documents > the necessary incantations in an example. > > That this is not only non-obvious to me is documented by at least two > I2C kernel implementations for the display out there. > > Thanks to Geert Uytterhoeven for pointing out how this is done. > Thanks to Miguel Ojeda for extensively commenting on my previous patch. You're welcome! A couple tips: - Normally, you will want to use scripts/get_maintainer.pl to know to whom send a given change. In this case, I am Cc'ing Rob, Geert and the devicetree mailing list which were missing. - Also, for single patches, typically you would want to send the patch without a cover letter. If you want to put comments that shouldn't go into the commit, you can write them just below the "---" line. Thank you for contributing to the kernel and welcome! Cheers, Miguel
Re: [PATCH] fs: binfmt_em86: check return code of remove_arg_zero
On Tue, Dec 22, 2020 at 10:03 PM Nick Desaulniers
wrote:
>
> remove_arg_zero is declared as __must_check. Looks like it can return
> -EFAULT on failure.
>
> Cc: Masahiro Yamada
> Cc: Miguel Ojeda
> Reported-by: Guenter Roeck
> Signed-off-by: Nick Desaulniers
Cc'ing Alpha list too.
Alexander, fs-devel: pinging about this... We removed
ENABLE_MUST_CHECK in 196793946264 ("Compiler Attributes: remove
CONFIG_ENABLE_MUST_CHECK"), so this should be giving a warning now
unconditionally. In 5.12 it will likely become a build error.
Nick: thanks for the patch! (I missed it in December, sorry)
Reviewed-by: Miguel Ojeda
Cheers,
Miguel
Re: [PATCH 2/2] auxdisplay: devicetree doc for HD44780/PCF8574
Hi Ralf, Something quick I noticed... On Tue, Jan 5, 2021 at 4:05 PM Ralf Schlatterbeck wrote: > > + display-height-chars: > +description: Height of the display, in character cells, Spurious comma instead of period. Cheers, Miguel
Re: [PATCH 1/2] auxdisplay: HD44780 connected to I2C via PCF8574
Hi Ralf,
On Tue, Jan 5, 2021 at 4:04 PM Ralf Schlatterbeck wrote:
>
> Add HD44780 character display connected via I2C I/O expander.
> Re-uses the high-level interface of the existing HD44780 driver.
>
> Signed-off-by: Ralf Schlatterbeck
Thanks for the driver! See a first quick review below.
Also, Cc'ing others related to hd44780/charlcd that may be interested
(there is another patch in the series, too).
> +config HD44780_PCF8574
> + tristate "HD44780 Character LCD support, I2C-connection"
There is no hyphen in the "parallel connection" one, perhaps remove it?
> +#define DEBUG
Spurious line from development?
> +/*
> + * The display uses 4-bit mode (the I/O expander has only 8 bits)
> + * The control signals RS, R/W, E are on three bits and on many displays
> + * the backlight is on bit 3. The upper 4 bits are data.
> + */
> +#define HD44780_RS_SHIFT 0
> +#define HD44780_RW_SHIFT 1
> +#define HD44780_E_SHIFT2
> +#define HD44780_BACKLIGHT_SHIFT3
These are always used in BIT() from a quick look, so perhaps you can
directly define the bits themselves instead.
> +struct hd44780 {
> + const struct i2c_client *i2c_client;
> + int backlight;
> +};
Even though the struct is internal to the driver, I'd avoid calling it
the same as the one in hd44780.c. I guess hd44780_pcf8574 would be
best, following the name of the file and the CONFIG_ option.
Same for the #define names, the functions' names, etc.
> +static void hd44780_backlight(struct charlcd *lcd, int on)
> +{
> + struct hd44780 *hd = lcd->drvdata;
> + u8 b = BIT(HD44780_RW_SHIFT); /* Read-bit */
> +
> + hd->backlight = on;
Newline here. In general, please add newlines to try to separate
blocks of related code, similar to paragraphs in text. (I will give
some examples below).
> + (void)i2c_master_send(hd->i2c_client, , 1);
I wonder if we should make charlcd_ops' backlight return an int rather
than void, so that we can properly return the error here (similarly in
lcd2s etc.), even if we ignore it so far in charlcd.c... Thoughts Lars
et. al.?
> +static int hd44780_send_nibble(struct hd44780 *hd, u8 outbyte)
> +{
> + const struct i2c_client *i2c_client = hd->i2c_client;
> + u8 backlight = hd->backlight ? BIT(HD44780_BACKLIGHT_SHIFT) : 0;
const?
> + u8 b = outbyte;
> + int err;
Newline.
> + /*
> +* Transfers first send the output byte with the E-bit 0
> +* Then the spec says to wait for 20us, then we set the E-bit to 1
> +* and wait for 40us, then reset the E-bit again.
> +* A max-speed (400kbit/s) I2C transfer for a single byte will
> +* already take 25us. So the first delay of 20us can be skipped.
> +* The second delay becomes 40us - 25us = 15us.
> +*/
> + b = (outbyte & ~BIT(HD44780_E_SHIFT)) | backlight;
> + dev_dbg(_client->dev, "I2C send: 0x%x", b);
> + err = i2c_master_send(i2c_client, , 1);
> + if (err < 0)
> + goto errout;
Newline (same for the others).
> + b = (outbyte | BIT(HD44780_E_SHIFT)) | backlight;
> + dev_dbg(_client->dev, "I2C send: 0x%x", b);
> + err = i2c_master_send(i2c_client, , 1);
> + if (err < 0)
> + goto errout;
> + udelay(15);
> + b = (outbyte & ~BIT(HD44780_E_SHIFT)) | backlight;
> + dev_dbg(_client->dev, "I2C send: 0x%x", b);
> + err = i2c_master_send(i2c_client, , 1);
> + if (err < 0)
> + goto errout;
> + return 0;
Newline.
> +errout:
> + dev_err(_client->dev, "Error sending to display: %d", err);
> + return err;
> +}
> +
> +static void hd44780_write_cmd_raw_nibble(struct charlcd *lcd, int cmd)
> +{
> + struct hd44780 *hd = lcd->drvdata;
> +
> + (void)hd44780_send_nibble(hd, (cmd & 0x0F) << 4);
Similarly, should we make hd44780_write_cmd_raw_nibble(),
hd44780_write_data() etc. return the error, even if ignored later on?
> + ret = hd44780_send_nibble(hd, (cmd & 0xF0));
No parenthesis needed in the argument (I guess you wrote them for
consistency with the other send*()s, but they are far apart).
> + if (ret)
> + return;
> + ret = hd44780_send_nibble(hd, (cmd << 4));
Ditto.
> + if (ret)
> + return;
Newline.
> +static int hd44780_i2c_remove(struct i2c_client *client)
> +{
> + struct charlcd *lcd = i2c_get_clientdata(client);
> +
> + charlcd_unregister(lcd);
charlcd_unregister() may fail (it doesn't right now in any path, but
it returns an int, so it should be checked and returned if so).
Cheers,
Miguel
[GIT PULL] Compiler Attributes for v5.11
Hi Linus, This one is not a bug fix; but it is not a new feature either... If you prefer changes like this one to be sent in the merge window, I will re-send it later on. If this gets into v5.11, then Masahiro is planning to try to make the warning an error for v5.12. This commit has already triggered the discussion/fix of a couple of likely mistakes (or at least the addition of explanations for the omissions), which is a good thing. I left it in -next for a few more weeks than planned to see if (more) people complained about the warnings, but it has been quiet since then. Cheers, Miguel The following changes since commit b65054597872ce3aefbc6a666385eabdf9e288da: Linux 5.10-rc6 (2020-11-29 15:50:50 -0800) are available in the Git repository at: https://github.com/ojeda/linux.git tags/compiler-attributes-for-linus-v5.11 for you to fetch changes up to 1967939462641d8b36bcb3fcf06d48e66cd67a4f: Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK (2020-12-02 13:47:17 +0100) An addition to compiler_attributes.h thanks to: - Remove CONFIG_ENABLE_MUST_CHECK (Masahiro Yamada) Masahiro Yamada (1): Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK include/linux/compiler_attributes.h | 6 ++ include/linux/compiler_types.h | 6 -- lib/Kconfig.debug | 8 tools/testing/selftests/wireguard/qemu/debug.config | 1 - 4 files changed, 6 insertions(+), 15 deletions(-)
Re: [PATCH] sh: check return code of request_irq
On Fri, Jan 1, 2021 at 2:50 PM John Paul Adrian Glaubitz wrote: > > Verified on my SH-7785LCR board. Boots fine. > > Tested-by: John Paul Adrian Glaubitz Thanks for testing, John! I think Masahiro was concerned about the error case (I assume you tested the happy path). In any case, if no maintainer suggests something else, looks good to me (and it is no worse than the status quo unless the `pr_err()` can somehow kill it), so: Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH] ARM: avoid cpuidle link warning
On Wed, Dec 30, 2020 at 4:55 PM Arnd Bergmann wrote: > > Change the definition of CPUIDLE_METHOD_OF_DECLARE() to silently > drop the table and all code referenced from it when CONFIG_CPU_IDLE > is disabled. Re-Cc'ing Nick (Arnd's email had a wrong address). Cheers, Miguel
Re: [PATCH] ARM: avoid cpuidle link warning
On Wed, Dec 30, 2020 at 4:55 PM Arnd Bergmann wrote: > > Change the definition of CPUIDLE_METHOD_OF_DECLARE() to silently > drop the table and all code referenced from it when CONFIG_CPU_IDLE > is disabled. Looks good, Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Mon, Dec 21, 2020 at 11:02 AM Guenter Roeck wrote: > > On 12/20/20 10:18 PM, Masahiro Yamada wrote: > With a change like this, I'd have expected that there is a coccinelle > script or similar to ensure that claims made in the commit message > are true. It is only a warning -- the compiler already tells us what is wrong. Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Mon, Dec 21, 2020 at 7:20 AM Masahiro Yamada wrote: > > Sorry for the delay. No problem! > Now I sent out the fix for lantiq_etop.c > > https://lore.kernel.org/patchwork/patch/1355595/ I saw it, thanks for the Cc! > The reason of the complication was > I was trying to merge the following patch in the same development cycle: > https://patchwork.kernel.org/project/linux-kbuild/patch/20201117104736.24997-1-o...@aepfle.de/ Ah, then that explains why Guenter's had an error instead of a warning. > Tomorrow's linux-next should be OK > and, you can send my patch in this merge window. Thanks! Cheers, Miguel
Re: [PATCH] net: lantiq_etop: check the result of request_irq()
On Mon, Dec 21, 2020 at 6:01 PM Andrew Lunn wrote: > > So please leave the warning in place, and maybe somebody else will > fully fix it. For context: the plan is to enable the warning unconditionally starting with 5.11. After that, the idea is making it an error as soon as reasonable (e.g. 5.12 if no warnings remain by then). However, if there is nobody planning to fix a given warning, then I'd say documenting the problem with a `FIXME` comment (plus a change like this or simply ignoring the return value) would be the best approach. Cheers, Miguel
Re: Linux kernel in-tree Rust support
On Thu, Dec 17, 2020 at 10:45 PM Pavel Machek wrote: > > Well.. not everyone has 32 cores in their notebook. It is true that complex Rust, like complex C++, does have high compilation times. But it all depends on how much one relies on certain language features. Straightforward Rust code is quick to compile. For reference, some quick stats :-) On a given machine, building v5.10 with a minimal config under -j3 takes 3 minutes. With Rust support enabled and 4 trivial Rust modules, it takes 50 seconds more. "A big increase!", you may indeed claim, but those 50 seconds are basically all spent on the shared Rust support. The actual Rust modules compile very quickly afterwards. For example, touching either `drivers/char/mem.c` or one of the trivial Rust modules takes the same time in that machine: 10 seconds. This is for a minimal config -- when you start dealing with `allmodconfig` builds, or when you start having a hundred Rust modules instead of 4, the upfront cost becomes very small per Rust module. > Okay. I did some refactoring recently and I really wished kernel was > in Rust (and not in C)... so lets see what happens. Indeed, I think it is definitely worth it. Cheers, Miguel
Re: [PATCH] leds: Use DEVICE_ATTR_{RW, RO, WO} macros
On Sun, Dec 13, 2020 at 7:21 PM Joe Perches wrote: > > clang-format is not a tool to rewrite code only neaten its layout. > > coccinelle _might_ be able to do this for limited cases where the > show function is in the same compilation unit/file, but even then > it would not be a trivial script. +1 The most robust approach, but the one that is most involved, would be a clang-tidy check. Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Sun, Dec 13, 2020 at 4:38 PM 'Matthias Urlichs' via Clang Built Linux wrote: > > If your change to a function breaks its callers, it's your job to fix No function has changed. This patch enables a warning (that for some reason is an error in the case of Guenter). Even if this was a hard error, the same applies: the function hasn't changed. It just means callers never tested with `CONFIG_ENABLE_MUST_CHECK` for *years*. > the callers proactively instead of waiting for "as they come" bug > reports. (Assuming, of course, that you know about the breakage. Which > you do when you tell us that the bad pattern can simply be grepped for.) No, *we don't know about the breakage*. The grep was for the particular function Guenter reported, and done to validate his concern. If you want to manually inspect every caller of every `__must_check` function, or to write a cocci patch or a clang-tidy check or similar (that would be obsolete as soon as `__must_check` is enabled), you are welcome to do so. But a much better usage of our time would be letting machines do their job. > If nothing else, that's far more efficient than [number_of_callers] > separate patches by other people who each need to find the offending > change, figure out what to change and/or who to report the problem to, > and so on until the fix lands in the kernel. This change is not in Linus' tree, it is on -next. > Moreover, this wouldn't leave the kernel sources in a non-bisect-able > state during that time. Again, the change is in -next. That is the point: to do integration testing and let the bots run against it. Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Sun, Dec 13, 2020 at 4:16 PM Greg KH wrote: > > Because if you get a report of something breaking for your change, you > need to work to resolve it, not argue about it. Otherwise it needs to > be dropped/reverted. Nobody has argued that. In fact, I explicitly said the opposite: "So I think we can fix them as they come.". I am expecting Masahiro to follow up. It has been less than 24 hours since the report, on a weekend. Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Sun, Dec 13, 2020 at 1:55 PM Guenter Roeck wrote: > > Witz komm raus, Du bist umzingelt. Please, explain this reference. :-) > The key here is "if nobody complains". I would argue that it is _your_ > responsibility to do those builds, and not the reponsibility of others > to do it for you. Testing allmodconfig for a popular architecture, agreed, it is due diligence to avoid messing -next that day. Testing a matrix of configs * arches * gcc/clang * compiler versions? No, sorry, that is what CI/-next/-rcs are for and that is where the "if nobody complains" comes from. If you think building a set of code for a given arch/config/etc. is particularly important, then it is _your_ responsibility to build it once in a while in -next (as you have done). If it is not that important, somebody will speak up in one -rc. If not, is anyone actually building that code at all? Otherwise, changing core/shared code would be impossible. Please don't blame the author for making a sensible change that will improve code quality for everyone. > But, sure, your call. Please feel free to ignore my report. I'm not ignoring the report, quite the opposite. I am trying to understand why you think reverting is needed for something that has been more than a week in -next without any major breakage and still has a long road to v5.11. Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck wrote: > > This patch results in: > > arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus': > arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of > 'request_irq' declared with attribute 'warn_unused_result' > > when building sh:defconfig. Checking for calls to request_irq() > suggests that there will be other similar errors in various builds. > Reverting the patch fixes the problem. Which ones? From a quick grep and some filtering I could only find one file with wrong usage apart from this one: drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv); drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv); Of course, this does not cover other functions, but it means there aren't many issues and/or people building the code if nobody complains within a few weeks. So I think we can fix them as they come. Cheers, Miguel
[GIT PULL] auxdisplay for v5.11
Hi Linus, This time there have been quite a few changes in auxdisplay thanks to a refactor by Lars Poeschel to share code in order to introduce a new driver. I am sending these earlier than usual. Please also note I am using a different email address than usual, too. Cheers, Miguel The following changes since commit 3cea11cd5e3b00d91caf0b4730194039b45c5891: Linux 5.10-rc2 (2020-11-01 14:43:51 -0800) are available in the Git repository at: https://github.com/ojeda/linux.git tags/auxdisplay-for-linus-v5.11 for you to fetch changes up to 351dcacc6d774258be9fec6f51c14f8ff38243f6: auxdisplay: panel: Remove redundant charlcd_ops structures (2020-11-16 17:13:37 +0100) A bigger set of changes than usual for auxdisplay: - Significant refactor work to make charlcd independent of device, i.e. hd44780 (Lars Poeschel) - New driver: lcd2s (Lars Poeschel) - Fixes on top of the rework while being tested in -next (Lars Poeschel, Dan Carpenter and kernel test robot) Dan Carpenter (1): auxdisplay: fix use after free in lcd2s_i2c_remove() Lars Poeschel (28): auxdisplay: Use an enum for charlcd backlight on/off ops auxdisplay: Introduce hd44780_common.[ch] auxdisplay: Move hwidth and bwidth to struct hd44780_common auxdisplay: Move ifwidth to struct hd44780_common auxdisplay: Move write_data pointer to hd44780_common auxdisplay: Move write_cmd pointers to hd44780 drivers auxdisplay: Move addr out of charlcd_priv auxdisplay: hd44780_common_print auxdisplay: provide hd44780_common_gotoxy auxdisplay: add home to charlcd_ops auxdisplay: Move clear_display to hd44780_common auxdisplay: make charlcd_backlight visible to hd44780_common auxdisplay: Make use of enum for backlight on / off auxdisplay: Move init_display to hd44780_common auxdisplay: implement various hd44780_common_ functions auxdisplay: cleanup unnecessary hd44780 code in charlcd auxdisplay: Move char redefine code to hd44780_common auxdisplay: Call charlcd_backlight in place auxdisplay: hd44780_common: Reduce clear_display timeout auxdisplay: hd44780: Remove clear_fast auxdisplay: charlcd: replace last device specific stuff auxdisplay: Change gotoxy calling interface auxdisplay: charlcd: Do not print chars at end of line auxdisplay: lcd2s DT binding doc auxdisplay: add a driver for lcd2s character display auxdisplay: hd44780_common: Fix build error auxdisplay: panel: Fix missing print function pointer auxdisplay: panel: Remove redundant charlcd_ops structures kernel test robot (1): auxdisplay: fix platform_no_drv_owner.cocci warnings .../bindings/auxdisplay/modtronix,lcd2s.yaml | 58 +++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + drivers/auxdisplay/Kconfig | 33 +- drivers/auxdisplay/Makefile| 2 + drivers/auxdisplay/charlcd.c | 412 ++--- drivers/auxdisplay/charlcd.h | 86 - drivers/auxdisplay/hd44780.c | 120 -- drivers/auxdisplay/hd44780_common.c| 361 ++ drivers/auxdisplay/hd44780_common.h| 33 ++ drivers/auxdisplay/lcd2s.c | 402 drivers/auxdisplay/panel.c | 173 - 11 files changed, 1218 insertions(+), 464 deletions(-) create mode 100644 Documentation/devicetree/bindings/auxdisplay/modtronix,lcd2s.yaml create mode 100644 drivers/auxdisplay/hd44780_common.c create mode 100644 drivers/auxdisplay/hd44780_common.h create mode 100644 drivers/auxdisplay/lcd2s.c
Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
On Thu, Dec 10, 2020 at 11:35 AM Marco Elver wrote: > > It looks like there's no clear MAINTAINER for this. :-/ > It'd still be good to fix this for 5.11. Richard seems to be the author, not sure if he picks patches (CC'd). I guess Masahiro or akpm (Cc'd) would be two options; otherwise, I could pick it up through compiler attributes (stretching the definition...). Cheers, Miguel
Re: [PATCH] netfilter: conntrack: fix -Wformat
On Thu, Dec 3, 2020 at 8:26 AM Lukas Bulwahn wrote: > > It is not a competition between checkpatch and clang-format, but if it would > be: Please note that clang-tidy is a different tool, it is designed to write lints based on the AST rather than formatting. > But jokes aside: Dwaipayan Ray, a mentee Joe and I are working with, > has already submitted a patch to checkpatch that identifies those > patterns and provides a fix: > > https://lore.kernel.org/lkml/20201128200046.78739-1-dwaipayanr...@gmail.com/ That is very good! However, it does not hurt to have it repeated in clang-tidy too: it is a very good thing to have a full C parser behind when writing lints! Cheers, Miguel
Re: [PATCH] Documentation: fix typos found in process, dev-tools, and doc-guide subdirectories
On Wed, Dec 2, 2020 at 8:56 AM Andrew Klychkov wrote: > > Fix four typos in kcov.rst, sphinx.rst, clang-format.rst, and > embargoed-hardware-issues.rst > > Signed-off-by: Andrew Klychkov > --- > Documentation/dev-tools/kcov.rst| 2 +- > Documentation/doc-guide/sphinx.rst | 2 +- > Documentation/process/clang-format.rst | 2 +- Thanks! Acked-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH] userfaultfd: selftests: make __{s,u}64 format specifiers portable
On Thu, Dec 3, 2020 at 12:55 AM Axel Rasmussen wrote: > > This is what clang-format yields. Are you thinking it would be better > to line everything up with the ( in uffd_error( ? Yeah, sometimes clang-format cannot do a good job with the 80 column limit + 8 tabs. You are definitely not forced to follow clang-format output by any means. Subsystem maintainers decide what style they prefer anyway, which could range from a manual approach to following clang-format strictly. Clang-format implements the general kernel style as closely as we could get it so far (it will improve more in the future when we raise the minimum clang-format version required). See Doc/process/clang-format.rst. > Or, perhaps this case is a good reason to make uffd_error() a variadic > macro so we can insert "-EEXIST" || "error" with a "%s". ...and indeed, sometimes it is a hint that simplifying things could help :-) Cheers, Miguel
Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK
On Sat, Nov 28, 2020 at 8:34 PM Masahiro Yamada wrote:
>
> Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
>
> A lot of warn_unused_result warnings existed in 2006, but until now
> they have been fixed thanks to people doing allmodconfig tests.
>
> Our goal is to always enable __must_check where appropriate, so this
> CONFIG option is no longer needed.
>
> I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
>
> # CONFIG_ENABLE_MUST_CHECK is not set
>
> I did not touch them for now since it would be a big churn. If arch
> maintainers want to clean them up, please go ahead.
>
> While I was here, I also moved __must_check to compiler_attributes.h
> from compiler_types.h
>
> Signed-off-by: Masahiro Yamada
> Acked-by: Jason A. Donenfeld
Picked this new version with the Acks etc., plus I moved it within
compiler_attributes.h to keep it sorted (it's sorted by the second
column, rather than the first).
Thanks a lot!
Cheers,
Miguel
Re: [PATCH] MAINTAINERS add D: tag for subsystem commit prefix
On Fri, Nov 27, 2020 at 10:51 PM wrote: > > From > RFC MAINTAINERS tag for cleanup robot > https://lkml.org/lkml/2020/11/21/190 Please use lore.kernel.org for links. > The prefix used by subsystems in their commit log is arbitrary. > To elimitate the time and effort to manually find a reasonable Typo in "eliminate". > prefix, store the preferred prefix in the MAINTAINERS file. > > Populate with reasonable prefixes using this algorithm. > What did the maintainers use in their commits? > If there were no maintainers commits then what did everyone > else use in their commits. Why is this in the form of a question? I am not sure I understand -- is it rhetorical? > The results were manually reviewed and about 25% were rejected. What do you mean by rejected? > Add 'D' tag to checkpatch.pl > > Use 'D' tag by adding --subsystem_commit_prefix option > get_maintainer.pl This looks also out of place, as if it was squashed from other commits. I would suggest trying to reword the message in prose, explaining the rationale and why it was done like this. > AUXILIARY DISPLAY DRIVERS Missing entry for auxdisplay. Cheers, Miguel
Re: [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
On Sat, Nov 21, 2020 at 8:44 PM Masahiro Yamada wrote:
>
> Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK").
>
> A lot of warn_unused_result warnings existed in 2006, but until now
> they have been fixed thanks to people doing allmodconfig tests.
>
> Our goal is to always enable __must_check where appreciate, so this
> CONFIG option is no longer needed.
>
> I see a lot of defconfig (arch/*/configs/*_defconfig) files having:
>
> # CONFIG_ENABLE_MUST_CHECK is not set
>
> I did not touch them for now since it would be a big churn. If arch
> maintainers want to clean them up, please go ahead.
>
> Signed-off-by: Masahiro Yamada
Picked it up through compiler-attributes with the "appreciate" typo
fixed on my end. I did a quick compile-test with a minimal config.
Let's see if the -next bots complain...
Thanks!
Cheers,
Miguel
Re: [PATCH 01/16] mfd: bcm590xx: drop of_match_ptr from of_device_id table
On Fri, Nov 27, 2020 at 10:21 AM Lee Jones wrote: > > It's a per-subsystem convention thing. I think some allow both, too. For people that send tree-wide patches, it would be if we agreed on the convention... Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven wrote: > > The maintainer is not necessarily the owner/author of the code, and > thus may not know the intent of the code. Agreed, I was not blaming maintainers -- just trying to point out that the problem is there :-) In those cases, it is still very useful: we add the `fallthrough` and a comment saying `FIXME: fallthrough intended? Figure this out...`. Thus a previous unknown unknown is now a known unknown. And no new unknown unknowns will be introduced since we enabled the warning globally. > BTW, you cannot mindlessly fix the latter, as you cannot know if > "(a == b)" or "((a = b))" was intended, without understanding the code > (and the (possibly unavailable) data sheet, and the hardware, ...). That's right, I was referring to the cases where the compiler saves someone time from a typo they just made. Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 11:44 PM Edward Cree wrote: > > To make the intent clear, you have to first be certain that you > understand the intent; otherwise by adding either a break or a > fallthrough to suppress the warning you are just destroying the > information that "the intent of this code is unknown". If you don't know what the intent of your own code is, then you *already* have a problem in your hands. > Figuring out the intent of a piece of unfamiliar code takes more > than 1 minute; just because > case foo: > thing; > case bar: > break; > produces identical code to > case foo: > thing; > break; > case bar: > break; > doesn't mean that *either* is correct — maybe the author meant What takes 1 minute is adding it *mechanically* by the author, i.e. so that you later compare whether codegen is the same. > to write > case foo: > return thing; > case bar: > break; > and by inserting that break you've destroyed the marker that > would direct someone who knew what the code was about to look > at that point in the code and spot the problem. Then it means you already have a bug. This patchset gives the maintainer a chance to notice it, which is a good thing. The "you've destroyed the market" claim is bogus, because: 1. you were not looking into it 2. you didn't notice the bug so far 3. is implicit -- harder to spot 4. is only useful if you explicitly take a look at this kind of bug. So why don't you do it now? > Thus, you *always* have to look at more than just the immediate > mechanical context of the code, to make a proper judgement that > yes, this was the intent. I find that is the responsibility of the maintainers and reviewers for tree-wide patches like this, assuming they want. They can also keep the behavior (and the bugs) without spending time. Their choice. > If you think that that sort of thing > can be done in an *average* time of one minute, then I hope you > stay away from code I'm responsible for! Please don't accuse others of recklessness or incompetence, especially if you didn't understand what they said. > A warning is only useful because it makes you *think* about the > code. If you suppress the warning without doing that thinking, > then you made the warning useless; and if the warning made you > think about code that didn't *need* it, then the warning was > useless from the start. We are not suppressing the warning. Quite the opposite, in fact. > So make your mind up: does Clang's stricter -Wimplicit-fallthrough > flag up code that needs thought (in which case the fixes take > effort both to author and to review) As I said several times already, it does take time to review if the maintainer wants to take the chance to see if they had a bug to begin with, but it does not require thought for the author if they just go for equivalent codegen. > or does it flag up code > that can be mindlessly "fixed" (in which case the warning is > worthless)? Proponents in this thread seem to be trying to > have it both ways. A warning is not worthless just because you can mindlessly fix it. There are many counterexamples, e.g. many checkpatch/lint/lang-format/indentation warnings, functional ones like the `if (a = b)` warning... Cheers, Miguel
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
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
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 12:53 AM Finn Thain wrote: > > I'm saying that supporting the official language spec makes more sense > than attempting to support a multitude of divergent interpretations of the > spec (i.e. gcc, clang, coverity etc.) Making the kernel strictly conforming is a ship that sailed long ago, for several reasons. Anyway, supporting several compilers and other tools, regardless of extensions, is valuable. > I'm also saying that the reason why we use -std=gnu89 is that existing > code was written in that language, not in ad hoc languages comprised of > collections of extensions that change with every release. No, we aren't particularly tied to `gnu89` or anything like that. We could actually go for `gnu11` already, since the minimum GCC and Clang support it. Even if a bit of code needs fixing, that shouldn't be a problem if someone puts the work. In other words, the kernel code is not frozen, nor are the features it uses from compilers. They do, in fact, change from time to time. > Thank you for checking. I found a free version that's only 6 weeks old: You're welcome! There are quite a few new attributes coming, mostly following C++ ones. > It will be interesting to see whether 6.7.11.5 changes once the various > implementations reach agreement. Not sure what you mean. The standard does not evolve through implementations' agreement (although standardizing existing practice is one of the best arguments to back a change). Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
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
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 1:58 AM Finn Thain wrote:
>
> What I meant was that you've used pessimism as if it was fact.
"future mistakes that it might prevent" is neither pessimism nor states a fact.
> For example, "There is no way to guess what the effect would be if the
> compiler trained programmers to add a knee-jerk 'break' statement to avoid
> a warning".
It is only knee-jerk if you think you are infallible.
> Moreover, what I meant was that preventing programmer mistakes is a
> problem to be solved by development tools
This warning comes from a development tool -- the compiler.
> The idea that retro-fitting new
> language constructs onto mature code is somehow necessary to "prevent
> future mistakes" is entirely questionable.
The kernel is not a frozen codebase.
Further, "mature code vs. risk of change" arguments don't apply here
because the semantics of the program and binary output isn't changing.
> Sure. And if you put -Wimplicit-fallthrough into the Makefile and if that
> leads to well-intentioned patches that cause regressions, it is partly on
> you.
Again: adding a `fallthrough` does not change the program semantics.
If you are a maintainer and want to cross-check, compare the codegen.
> Have you ever considered the overall cost of the countless
> -Wpresume-incompetence flags?
Yeah: negative. On the other hand, the overall cost of the countless
-fI-am-infallible flags is very noticeable.
> Perhaps you pay the power bill for a build farm that produces logs that
> no-one reads? Perhaps you've run git bisect, knowing that the compiler
> messages are not interesting? Or compiled software in using a language
> that generates impenetrable messages? If so, here's a tip:
>
> # grep CFLAGS /etc/portage/make.conf
> CFLAGS="... -Wno-all -Wno-extra ..."
> CXXFLAGS="${CFLAGS}"
>
> Now allow me some pessimism: the hardware upgrades, gigawatt hours and
> wait time attributable to obligatory static analyses are a net loss.
If you really believe compiler warnings and static analysis are
useless and costly, I think there is not much point in continuing the
discussion.
> No, it's not for me to prove that such patches don't affect code
> generation. That's for the patch author and (unfortunately) for reviewers.
I was not asking you to prove it. I am stating that proving it is very easy.
Cheers,
Miguel
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:24 PM Finn Thain wrote: > > These statements are not "missing" unless you presume that code written > before the latest de facto language spec was written should somehow be > held to that spec. There is no "language spec" the kernel adheres to. Even if it did, kernel code is not frozen. If an improvement is found, it should be applied. > If the 'fallthrough' statement is not part of the latest draft spec then > we should ask why not before we embrace it. Being that the kernel still > prefers -std=gnu89 you might want to consider what has prevented > -std=gnu99 or -std=gnu2x etc. The C standard has nothing to do with this. We use compiler extensions of several kinds, for many years. Even discounting those extensions, the kernel is not even conforming to C due to e.g. strict aliasing. I am not sure what you are trying to argue here. But, since you insist: yes, the `fallthrough` attribute is in the current C2x draft. Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
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
Re: [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
On Mon, Nov 23, 2020 at 4:37 AM Masahiro Yamada wrote: > > I can move it to compiler_attribute.h > > This attribute is supported by gcc, clang, and icc. > https://godbolt.org/z/ehd6so > > I can send v2. > > I do not mind renaming, but it should be done in a separate patch. Of course -- sorry, I didn't mean we had to do them now, i.e. we can do the move and the rename later on, if you prefer. Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
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
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 11:54 PM Finn Thain wrote: > > We should also take into account optimisim about future improvements in > tooling. Not sure what you mean here. There is no reliable way to guess what the intention was with a missing fallthrough, even if you parsed whitespace and indentation. > It is if you want to spin it that way. How is that a "spin"? It is a fact that we won't get *implicit* fallthrough mistakes anymore (in particular if we make it a hard error). > But what we inevitably get is changes like this: > > case 3: > this(); > + break; > case 4: > hmmm(); > > Why? Mainly to silence the compiler. Also because the patch author argued > successfully that they had found a theoretical bug, often in mature code. If someone changes control flow, that is on them. Every kernel developer knows what `break` does. > But is anyone keeping score of the regressions? If unreported bugs count, > what about unreported regressions? Introducing `fallthrough` does not change semantics. If you are really keen, you can always compare the objects because the generated code shouldn't change. Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
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
Re: [PATCH] compiler_attribute: remove CONFIG_ENABLE_MUST_CHECK
On Sat, Nov 21, 2020 at 8:44 PM Masahiro Yamada wrote: > > Our goal is to always enable __must_check where appreciate, so this > CONFIG option is no longer needed. This would be great. It also implies we can then move it to `compiler_attributes.h` since it does not depend on config options anymore. We should also rename it to `__nodiscard`, since that is the standardized name (coming soon to C2x and in C++ for years). Cc'ing the Clang folks too to make them aware. Cheers, Miguel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
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
Re: [PATCH v2 1/3] powerpc: boot: include compiler_attributes.h
On Wed, Nov 18, 2020 at 1:08 AM Nick Desaulniers wrote: > > It was also noted in 6a9dc5fd6170 that we could -D__KERNEL__ and > -include compiler_types.h like the main kernel does, though testing that > produces a whole sea of warnings to cleanup. (Re; for Gustavo to consider since he took it now): I would add a comment noting this as a reminder -- it also helps to entice a cleanup. Cheers, Miguel
Re: [PATCH] auxdisplay: panel: Remove redundant charlcd_ops structures
On Mon, Nov 16, 2020 at 2:41 PM wrote: > > From: Lars Poeschel > > The three struct charlcd_ops contain the same data, so we only need one > of this structures. The other two are removed. > > Signed-off-by: Lars Poeschel Thanks Lars, picking this up. Cheers, Miguel
Re: [PATCH] auxdisplay: panel: Fix missing print function pointer
On Mon, Nov 16, 2020 at 2:22 PM wrote:
>
> From: Lars Poeschel
>
> charlcd drivers need to provide some print function to charlcd. For
> hd44780 based panel driver this function was missing. We provide the
> generic hd44780_common_print function which should be suitable.
>
> Fixes: b26deabb1d915fe87d395081bbd3058b938dee89 ("auxdisplay:
> hd44780_common_print")
> Reported-by: kernel test robot
> Signed-off-by: Lars Poeschel
Thanks Lars, picking this up.
Cheers,
Miguel
Re: [PATCH 3/3] powerpc: fix -Wimplicit-fallthrough
On Mon, Nov 16, 2020 at 5:35 AM Nick Desaulniers wrote: > > The "fallthrough" pseudo-keyword was added as a portable way to denote > intentional fallthrough. Clang will still warn on cases where there is a > fallthrough to an immediate break. Add explicit breaks for those cases. > > Link: https://github.com/ClangBuiltLinux/linux/issues/236 > Signed-off-by: Nick Desaulniers It makes things clearer having a `break` added, so I like that warning. Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH 2/3] Revert "lib: Revert use of fallthrough pseudo-keyword in lib/"
On Mon, Nov 16, 2020 at 5:35 AM Nick Desaulniers
wrote:
>
> This reverts commit 6a9dc5fd6170 ("lib: Revert use of fallthrough
> pseudo-keyword in lib/")
>
> Now that we can build arch/powerpc/boot/ free of -Wimplicit-fallthrough,
> re-enable these fixes for lib/.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/236
> Signed-off-by: Nick Desaulniers
Looks fine on visual inspection:
Reviewed-by: Miguel Ojeda
Cheers,
Miguel
Re: [PATCH 1/3] powerpc: boot: include compiler_attributes.h
On Mon, Nov 16, 2020 at 5:35 AM Nick Desaulniers wrote: > > It was also noted in 6a9dc5fd6170 that we could -D__KERNEL__ and > -include compiler_types.h like the main kernel does, though testing that > produces a whole sea of warnings to cleanup. This approach is minimally > invasive. I would add a comment noting this as a reminder -- it also helps to entice a cleanup. Acked-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH 2/3] Revert "lib: Revert use of fallthrough pseudo-keyword in lib/"
On Mon, Nov 16, 2020 at 7:26 AM Gustavo A. R. Silva wrote: > > Reviewed-by: Gustavo A. R. Silva .org :-) Cheers, Miguel
Re: [PATCH] perf test: Fix dwarf unwind for optimized builds.
On Mon, Nov 16, 2020 at 7:48 AM Ian Rogers wrote: > > GCC [0-9]+ :-) Perhaps just a reference to the GCC bug rather than a date. That would be very good. > In linux/compiler_attributes.h add: > #define __GCC4_has_attribute_disable_tail_calls 0 > to the #ifndef __has_attribute block. We can't do this locally here as after > that #include __has_attribute will be defined. As far as I know, `tools/` use their own `compiler*` files, which is why I was suggesting creating the equivalent there. > In terms of lines of code, there's not much difference. Arguably there is a > bit more cognitive load from the #include and that disable_tail_call needs > the funny handling that's here but won't obviously be hinted at by placing it > in a shared header. I'm a little concerned that someone will come across this > in shared code and then go and break this test again with well intentioned > cleanup. Fewer lines, fewer conditions :-) The `#include` is hardly important given kernel developers already know and use compiler attributes in many places (they are included in the majority of compilation units). Actually, we can simplify further. The attribute itself should be pulled from the `compiler_attributes.h` (a `tools/` one, if needed), and the barrier should likely be the `barrier()` macro (ditto). Then, we just need: #if __has_attribute(disable_tail_calls) # define NO_TAIL_CALL_BARRIER #else # define NO_TAIL_CALL_BARRIER barrier() #endif because using the attribute directly just works -- the only special thing here is the conditional barrier. And this conditional barrier should probably be shared, too, defining it wherever `barrier()` (or equivalent) is defined for `tools/`. And the name could be `barrier_for_tail_call()` or something like that. Of course, we don't need to do all this for this patch, but we should always attempt to minimize/simplify the diffs later on -- that is why I suggested using the unconditional `__has_attribute` as if it was already properly defined (we had to untangle similar stuff when I added `compiler_attributes.h`). Cheers, Miguel
Re: [PATCH] perf test: Fix dwarf unwind for optimized builds.
On Sat, Nov 14, 2020 at 9:14 PM Ian Rogers wrote: > > Unfortunately no GCC version actually has this fixed. Then we can say GCC <= 11 does not support it yet or something like that. > This seems overly complex and unnecessary. How is 1 condition more complex than 3 different ones? Cheers, Miguel
Re: [PATCH] auxdisplay: fix platform_no_drv_owner.cocci warnings
On Wed, Nov 11, 2020 at 10:26 AM kernel test robot wrote:
>
> From: kernel test robot
>
> drivers/auxdisplay/lcd2s.c:373:3-8: No need to set .owner here. The core will
> do it.
>
> Remove .owner field if calls are used which set it automatically
>
> Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
>
> Fixes: 8c9108d014c5 ("auxdisplay: add a driver for lcd2s character display")
> CC: Lars Poeschel
> Reported-by: kernel test robot
> Signed-off-by: kernel test robot
Lars, I am picking this one, just so that you know.
Cheers,
Miguel
Re: [PATCH -next] auxdisplay: fix platform_no_drv_owner.cocci warning
Hi Zou, On Sat, Nov 14, 2020 at 9:11 AM Zou Wei wrote: > > ./drivers/auxdisplay/lcd2s.c:373:3-8: No need to set .owner here. The core > will do it. Thanks a lot for the patch. This patch was already submitted by the kernel test bot, please see https://lore.kernel.org/lkml/2020092559.GA67395@c88ae2e89e59/ Cheers, Miguel
Re: [PATCH] perf test: Fix dwarf unwind for optimized builds.
On Sat, Nov 14, 2020 at 1:08 AM 'Ian Rogers' via Clang Built Linux
wrote:
>
> To ensure the stack frames are on the stack tail calls optimizations
> need to be inhibited. If your compiler supports an attribute use it,
> otherwise use an asm volatile barrier.
>
> The barrier fix was suggested here:
> https://lore.kernel.org/lkml/20201028081123.gt2...@hirez.programming.kicks-ass.net/
>
> Fixes: 9ae1e990f1ab ("perf tools: Remove broken __no_tail_call
>attribute")
> ---
> tools/perf/tests/dwarf-unwind.c | 39 +++--
> 1 file changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/tests/dwarf-unwind.c b/tools/perf/tests/dwarf-unwind.c
> index 83638097c3bc..c8ce86bceea8 100644
> --- a/tools/perf/tests/dwarf-unwind.c
> +++ b/tools/perf/tests/dwarf-unwind.c
> @@ -24,6 +24,23 @@
> /* For bsearch. We try to unwind functions in shared object. */
> #include
>
> +/*
> + * The test will assert frames are on the stack but tail call optimizations
> lose
> + * the frame of the caller. Clang can disable this optimization on a called
> + * function but GCC currently (11/2020) lacks this attribute. The barrier is
> + * used to inhibit tail calls in these cases.
> + */
It would be nice to put the GCC version rather than the date.
> +#ifdef __has_attribute
> +#if __has_attribute(disable_tail_calls)
> +#define NO_TAIL_CALL_ATTRIBUTE __attribute__((disable_tail_calls))
> +#define NO_TAIL_CALL_BARRIER
> +#endif
> +#endif
> +#ifndef NO_TAIL_CALL_ATTRIBUTE
> +#define NO_TAIL_CALL_ATTRIBUTE
> +#define NO_TAIL_CALL_BARRIER __asm__ __volatile__("" : : : "memory");
> +#endif
I would try avoid this nest of conditions and instead do it like in
`compiler_attributes.h`, i.e. make use of `__has_attribute`
unconditional by making sure it works for all versions/compilers, and
then just:
#if __has_attribute(disable_tail_calls)
# define NO_TAIL_CALL_ATTRIBUTE __attribute__((disable_tail_calls))
# define NO_TAIL_CALL_BARRIER
#else
# define NO_TAIL_CALL_ATTRIBUTE
# define NO_TAIL_CALL_BARRIER __asm__ __volatile__("" : : : "memory");
#endif
In fact, I think it would be best to simply have a mimic of
`compiler_attributes.h` suitable for `tools/`.
Cheers,
Miguel
Re: [PATCH] ACPICA: fix -Wfallthrough
On Fri, Nov 13, 2020 at 1:09 AM Nick Desaulniers wrote: > > Thank you for the explicit diagnostics observed. Something fishy is > going on though, https://godbolt.org/z/Gbxbxa is how I expect MSVC to > handle include/linux/compiler_attributes.h. > > The C preprocessor should make it such that MSVC never sees > `__attribute__` or `__fallthrough__`; that it does begs the question. > That would seem to imply that `#if __has_attribute(__fallthrough__)` > somehow evaluates to true on MSVC, but my godbolt link shows it does > not. > > Could the upstream ACPICA project be #define'ing something that could > be altering this? (Or not #define'ing something?) > > Worst case, we could do as Joe Perches suggested and disable > -Wfallthrough for drivers/acpi/acpica/. I agree, something is fishy. MSVC has several flags for conformance and extensions support, including two full C preprocessors in newer versions; which means we might be missing something, but I don't see how the code in compiler_attributes.h could be confusing MSVC even in older non-conforming versions. Cheers, Miguel
Re: [PATCH] ACPICA: fix -Wfallthrough
On Thu, Nov 12, 2020 at 10:49 PM Moore, Robert wrote: > > 1>c:\acpica\source\components\utilities\utdelete.c(270): warning C4013: > '__attribute__' undefined; assuming extern returning int > 1>c:\acpica\source\components\utilities\utdelete.c(270): error C2065: > '__fallthrough__': undeclared identifier > 1>c:\acpica\source\components\utilities\utdelete.c(272): error C2143: syntax > error: missing ';' before 'case' Can you share a minimized sample with the `cl` version and command-line options? Cheers, Miguel
Re: [PATCH -next] treewide: Remove stringification from __alias macro definition
On Wed, Nov 11, 2020 at 8:19 AM Ard Biesheuvel wrote: > > I am still not convinced we need this change, as I don't see how the > concerns regarding __section apply to __alias. But if we do, can we > please use the same approach, i.e., revert the current patch, and > queue it again after v5.11-rc1 with all new occurrences covered as > well? In general, it would be nice to move all compiler attributes to use the `__` syntax, which is independent of compiler vendor, gives us a level of indirection to modify behavior between compilers and is shorter/nicer for users. But it is low priority, so it should go in whenever it causes the least amount of trouble. Cheers, Miguel
Re: [PATCH v6 00/25] Make charlcd device independent
Hi Randy, On Fri, Nov 6, 2020 at 5:35 PM Randy Dunlap wrote: > > I'm not sure that I understand the question... > > Include > Reported-by: Randy Dunlap > if possible. If not, then don't. It's not a big deal. > > Integrate the fix from Lars in whatever way works for you. Thanks Randy -- I asked Stephen Rothwell and he told me even for -next patches on top are preferred, unless the bug is bad enough. In which case, the [] notation can still be used to give credit. Cheers, Miguel
Re: [PATCH] auxdisplay: fix use after free in lcd2s_i2c_remove()
On Fri, Nov 6, 2020 at 8:26 PM Dan Carpenter wrote: > > The kfree() needs to be moved down a line to prevent a use after free. Thanks Dan for catching this one up while in -next. I'll pick it up. Cheers, Miguel
Re: [PATCH] auxdisplay: hd44780_common: Fix build error
On Mon, Nov 9, 2020 at 10:32 AM wrote: > > From: Lars Poeschel > > When building the hd44780_common driver without a driver that actually > uses it like panel or hd44780 you got a build error, because > hd44780_common uses charlcd, but did not select it. It's users did > select it. > This is fixed now. hd4478_common now selects charlcd in Kconfig and > panel and hd44780 do not. They only select hd44780_common. > > Reported-by: Randy Dunlap > Signed-off-by: Lars Poeschel Thanks Lars, I'm picking it up. Cheers, Miguel
Re: [PATCH] Kbuild: enable -Wfallthrough for clang
On Sat, Nov 7, 2020 at 8:08 AM Nick Desaulniers wrote:
>
> Partial revert of commit e2079e93f562 ("kbuild: Do not enable
> -Wimplicit-fallthrough for clang for now")
Wait, it says partial revert because it is one, but doing it this way
does not enable the option back for GCC (and Clang).
Shouldn't it be a full revert?
Cheers,
Miguel
Re: [PATCH] Kbuild: enable -Wfallthrough for clang
On Sat, Nov 7, 2020 at 8:08 AM Nick Desaulniers wrote:
>
> Partial revert of commit e2079e93f562 ("kbuild: Do not enable
> -Wimplicit-fallthrough for clang for now")
>
> This has been fixed up over time thanks to the addition of "fallthrough"
> pseudo-keyword in
> commit 294f69e662d1 ("compiler_attributes.h: Add 'fallthrough' pseudo
> keyword for switch/case use")
I think the title is missing the "implicit"?
Acked-by: Miguel Ojeda
Cheers,
Miguel
Re: [PATCH v6 00/25] Make charlcd device independent
On Fri, Nov 6, 2020 at 11:11 AM Lars Poeschel wrote: > > I got an email [1] with a report about a build failure in > hd44780_common. The fix is simple but I don't know the process from here > on. Should I post a v7 of the whole patchset or only a follow-up patch > for the fix ? Either would work (I can rebase it on my side). However, in order to give credit to Randy, if the fix is integrated into a previous patch, then I am not sure where we would put the Reported-by. Randy, what people usually do for your reports on -next (or what do you prefer)? Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Thu, Nov 5, 2020 at 1:35 AM Nick Desaulniers wrote: > > I'll help you paint your bikeshed. Oh, but what color? I see a red > door, and I want it painted black. > > Sounds to me like Miguel could win over a critic by addressing some of > your (quite valid) concerns. ;) I am happy to take the patch, but we need to at least enable other features that were added meanwhile -- that is the advantage of increasing the minimum! If someone wants to take a look (wink, wink -- Joe, you are very experienced with all the different styles used around the kernel and would be great to have you on board with clang-format), they can use the following list for starters. There are a few important new features: - AlignConsecutiveMacros is probably one of the biggest one for the kernel that we were missing so far. - IndentPPDirectives and AlignEscapedNewlines would be very good to use too -- but the kernel style is not consistent AFAIK, so we would need to decide. Then there are a few others that pertain to us too: - SpaceBeforeSquareBrackets - SpacesInConditionalStatement - SpaceAfterLogicalNot - SpaceInEmptyBlock - IndentGotoLabels Others are also worth checking to see if we can take advantage of them: - IncludeBlocks (and configuring IncludeCategories etc.) - StatementMacros Then there are others that are not related to us, but to be consistent we would explicitly set them in the file. Finally, for extra points, we could already document the new ones in LLVM 11 if any, for the future, but that is optional. If no one is up for the task, I will eventually do it... :-) Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Thu, Nov 5, 2020 at 1:33 AM Nick Desaulniers wrote: > > Yeah, that's annoying. Why don't you send me a patch that downgrades > it from hard error to polite warning (in case someone made a typo), > and I'll review it? Sure! Cheers, Miguel
Re: [PATCH] compiler-clang: remove version check for BPF Tracing
On Wed, Nov 4, 2020 at 8:11 PM Nick Desaulniers wrote: > > bpftrace parses the kernel headers and uses Clang under the hood. Remove > the version check when __BPF_TRACING__ is defined (as bpftrace does) so > that this tool can continue to parse kernel headers, even with older > clang sources. Acked-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH v6 00/25] Make charlcd device independent
On Tue, Nov 3, 2020 at 10:58 AM wrote: > > Changes in v6: > - patch 2: Fix Kconfig so that auxdisplay is now a submenu again > - patch 23: Fix some typos in commit message Thanks a lot for all the work, Lars. Queued in -next. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
Hi Joe, First of all, thanks for taking the time to write your reasoning. On Wed, Nov 4, 2020 at 5:17 AM Joe Perches wrote: > > The current kernel is v5.10 which requires clang 10.0 or higher. For building, yes. > This patch is not to be applied or backported to old kernels so no > person is going to use this patch on any old or backported kernel. Agreed (see my answer to Nick). > If a person is going to use clang-format on the current kernel sources > unless they are developing for the current kernel. > > They are going to have to be using clang 10.0 or higher and therefore > also will have and be using clang-format 10.0 or higher. No, they might be using GCC as usual and installed clang-format from their distro. In fact, I'd expect most developers accustomed to GCC to try it out that way, and also most of them to install compilers from their distro, not from the webpage, unless they need a newer version for some reason (e.g. new warnings, new debugging features in the kernel, etc.). In principle, clang-format (as a tool) is not related to building the kernel. We may call it "x-format" and think about it as a statically linked binary. What I am saying is that aligning clang-format to LLVM (now that LLVM has a minimum supported version) is not a necessity. We can still do it, of course, since there are new features for everyone and anybody that complains can install a newer version from the webpage. But there is nothing that forces us to require it. It is a decision that we balance w.r.t. new features. To put it concretely: if there were 0 new features or big fixes in clang-format 10 compared to 4, there would be no reason whatsoever to require users to download a new version. On the other side of the spectrum, some projects require a concrete version (not just a minimum), because they automatically format their entire codebase and want to avoid differences in output between clang-format versions. But we are far from automatically formatting the entire codebase. > Take it or not, apply it or not. I don't use clang-format and unless > there are improvements to it, I imagine I'll continue to use emacs > indent-region and a few other reformatting tools instead. Again, I am not opposed to the change. In fact, I am eager to improve the output of clang-format so that more people get engaged with it. I was just surprised you asserted so strongly that nobody is using clang-format from their distro. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, Nov 4, 2020 at 4:15 AM Joe Perches wrote: > > No one ever will use clang-format on the current kernel sources > without having a recent version of clang and clang-format. Why? Many distros come with clang-format pre-packaged, and in fact the original patch (that you commented on) argued for the >= 4 requirement that way. I am not saying we cannot change it, in fact I'd prefer if we do it (see my answer to Nick); but I don't understand your basis to claim nobody is installing their distro clang-format package. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, Nov 4, 2020 at 2:08 AM Nick Desaulniers wrote: > > Miguel, > Really? :P I'd bet if you picked up this patch no one would notice. > > I recommend a simpler approach to multiple version support, which is > just matching the one version recommended for the rest of LLVM tools. > Sure, technically you can use older tools, but do so at your own peril > and don't complain to us if it doesn't work. Otherwise trying to Originally, the .clang-format file was made to work with old versions in order to make it easy for people to use (just install it from your distro etc.). One of the reasons for that was to help adoption, as well as because clang-format gives a hard error on encountering an unknown option :-( I am not opposed to changing those requirements now and say it is part of the LLVM toolchain (even if it is independent from building), but somebody might be annoyed. > explain different versions and even for different directories gets way > too complex for anyone to take seriously. That is just an escape hatch for developers that really need the latest formatting options (e.g. to minimize the exceptions in fully formatted files) or temporarily deal with some bits of kernel code with a different style. I definitely wouldn't want people adding their own .clang-format files without good reason... > It's not like we backport raising the minimum version. That is a good point. In fact, we can just do it very early in the cycle and wait to see who complains. If there are too many complaints, we can always revert it. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
On Wed, Nov 4, 2020 at 1:56 AM Joe Perches wrote: > > Do remember that this patch is for the current kernel and > not any old version that someone might be compiling. > > The current kernel _requires_ clang 10.0+ and that would > obviously provide clang-format 10+ as well. You can use clang-format without having ever built a kernel with Clang. Cheers, Miguel
Re: [RFC PATCH] .clang-format: Remove conditional comments
Hi Joe, On Tue, Nov 3, 2020 at 7:29 PM Joe Perches wrote: > > Now that the clang minimum supported version is > 10.0, enable the > commented out conditional reformatting key:value lines in the file. > > Signed-off-by: Joe Perches > --- > > Hey Miguel. > > I don't use this, but on its face it seems a reasonable change > if the commented out key:value lines are correct. It is, yeah; however, the concern is that there may be developers running an old clang-format from their distro (i.e. not using it for compiling the kernel). We need to compare the functionality advantage vs. the inconvenience of installing a current LLVM. The best would be to ask whoever is using it right now, but there is no easy way to do that -- many will only notice when the change is actually pushed :-) So far, I have avoided upgrading the requirement until clang-format could match the kernel style even better (i.e. so that when the upgrade happens, there is a reason for it). Also, the configuration can be overridden in subfolders, thus a maintainer can push things forward in a subsystem meanwhile. Cheers, Miguel
Re: [PATCH 00/25] Make charlcd device independent
Hi Lars, On Thu, Oct 29, 2020 at 10:52 AM wrote: > > Changes in v5: > - patch 1: Fix a commit message typo: of -> on > - patch 2: Remove some unnecessary newlines > - patch 8: Fix some typos > - patch 14: Fix commit message typo: it's -> its > - patch 15: this patch is squashed together from the former individual > hd44780_common_ function patches > - patch 16: combined two cleanup patches > - patch 17: I did previously undo commit 3f03b6498 which was a mistake. > This is now corrected. > - patch 24: Picked up Robs Reviewed-by > - patch 25: use hex_to_bin like in commit 3f03b6498 but for the lcd2s.c > file Thanks a lot for all that! Please take a look at my other two messages for v5. We are almost there... Cheers, Miguel
Re: [PATCH v5 02/25] auxdisplay: Introduce hd44780_common.[ch]
Hi Lars, On Thu, Oct 29, 2020 at 10:57 AM wrote: > > diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig > index 81757eeded68..a56171d1a1ba 100644 > --- a/drivers/auxdisplay/Kconfig > +++ b/drivers/auxdisplay/Kconfig > @@ -14,12 +14,31 @@ menuconfig AUXDISPLAY > > If you say N, all options in this submenu will be skipped and > disabled. > > +config CHARLCD > + tristate "Character LCD core support" if COMPILE_TEST > + help > + This is the base system for character-based LCD displays. > + It makes no sense to have this alone, you select your display driver > + and if it needs the charlcd core, it will select it automatically. > + This is some character LCD core interface that multiple drivers can > + use. > + > +config HD44780_COMMON > + tristate "Common functions for HD44780 (and compatibles) LCD > displays" if COMPILE_TEST > + help > + This is a module with the common symbols for HD44780 (and > compatibles) > + displays. This is the code that multiple other modules use. It is > not > + useful alone. If you have some sort of HD44780 compatible display, > + you very likely use this. It is selected automatically by selecting > + your concrete display. > + > if AUXDISPLAY > These two should be after `if AUXDISPLAY`, no? I noticed the menu is broken when I went to compile test this (the options appear outside and the auxdisplay menu is empty). Perhaps you don't use menuconfig so you didn't see it? Sorry I missed this in previous iterations... Cheers, Miguel
Re: [PATCH v5 23/25] auxdisplay: charlcd: Do not print chars at end of line
Hi Lars, A few extra typos in this commit message. On Thu, Oct 29, 2020 at 10:58 AM wrote: > > Skip printing characters at the end of a display line. This fits to the > behaviour we already had, that the cursor is nailed to last position of to last -> to the last > a line. > This might slightly change behaviour. > On hd44780 displays with one or two lines the previous implementation > did still write characters to the buffer of the display even if they are > currently not visible. The shift_display command could be used so set so -> to > the "viewing window" to a new position in the buffer and then you could > see the characters previously written. > This described behaviour does not work for hd44780 displays with more > than two display lines. There simply is not enough buffer. > So the behaviour was a bit inconsistens across different displays. inconsistent -> inconsistent > The new behaviour is to stop writing character at the end of a visible character -> characters > line, even if there would be room in the buffer. This allows us to have > an easy implementation, that should behave equal on all supported > displays. This is not hd44780 hardware dependents anymore. dependents -> dependent Cheers, Miguel
Re: [PATCH] treewide: Remove stringification from __alias macro definition
On Fri, Oct 30, 2020 at 4:07 AM Joe Perches wrote: > > Like the old __section macro, the __alias macro uses macro # stringification > to create quotes around the symbol name used in the __attribute__. Hmm... isn't this V2? It seems none of the Acks/Reviews were picked up, did something major change? Cheers, Miguel
Re: [PATCH] tools/perf: Remove broken __no_tail_call attribute
On Wed, Oct 28, 2020 at 9:11 AM Peter Zijlstra wrote: > > Subject: tools/perf: Remove broken __no_tail_call attribute Acked-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH v2] ctype.h: remove duplicate isdigit() helper
On Tue, Oct 27, 2020 at 11:37 PM Arnd Bergmann wrote: > > Sounds good, I'll take that. Are the clang and icc version numbers > the actual ones we should list here, or is this just an example? Actual ones -- well, the best approximation I could get from the available versions in Compiler Explorer :-) Cheers, Miguel
Re: [PATCH v2] ctype.h: remove duplicate isdigit() helper
On Tue, Oct 27, 2020 at 12:57 AM Arnd Bergmann wrote: > > +#ifdef __has_builtin > +#define has_builtin(x) __has_builtin(x) > +#else > +#define has_builtin(x) (0) > +#endif Could this be #ifndef __has_builtin # define __has_builtin(x) 0 #endif ? i.e. mimicking what we do for `__has_attribute`. It would also be a nice idea to put a reminder comment like: /* * __has_builtin is supported on gcc >= 10, clang >= 3 and icc >= 21. * In the meantime, to support gcc < 10, we implement __has_builtin * by hand. */ Cheers, Miguel
Re: [PATCH] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Fri, Oct 23, 2020 at 10:03 AM Joe Perches wrote: > > Thanks Miguel, but IMO it doesn't need time in next. You're welcome! It never hurts to keep things for a bit there. > Applying it just before an rc1 minimizes conflicts. There shouldn't be many conflicts after -rc1. The amount of changes is reasonable too, so no need to apply the script directly. In any case, if you prefer that Linus picks it up himself right away for this -rc1, it looks good to me (with the caveat that it isn't tested): Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: [PATCH] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Thu, Oct 22, 2020 at 4:36 AM Joe Perches wrote:
>
> Use a more generic form for __section that requires quotes to avoid
> complications with clang and gcc differences.
I performed visual inspection (one by one...) and the only thing I saw
is that sometimes the `__attribute__` has a whitespace afterwards and
sometimes it doesn't, same for the commas inside, e.g.:
- __used __attribute__((section(".modinfo"), unused, aligned(1))) \
+ __used __section(".modinfo") __attribute__((unused, aligned(1))) \
and:
-__attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void * \
+__section("__param") __attribute__ ((unused, aligned(sizeof(void * \
I think the patch tries to follow the style of the replaced line, but
for the commas in this last case it didn't. Anyway, it is not
important.
I can pick it up in my queue along with the __alias one and keep it
for a few weeks in -next.
Cheers,
Miguel
Re: GCC section alignment, and GCC-4.9 being a weird one
On Wed, Oct 21, 2020 at 8:35 PM Joe Perches wrote: > > Perhaps something to add as a generic test in checkpatch. Agreed! It would be nice to check all of them. Even checking for `attribute` and `__attribute__` could be potentially reasonable (i.e. so that people are reminded to consider adding whatever attributes they need into `compiler_attributes.h`), although perhaps too annoying/noisy for some (e.g. for `arch/*` devs...). Cheers, Miguel
Re: [PATCH -next] treewide: Remove stringification from __alias macro definition
On Wed, Oct 21, 2020 at 9:07 PM Joe Perches wrote: > > Using quotes in __section caused/causes differences > between clang and gcc. Yeah, it is a good cleanup get. Thanks! > https://lkml.org/lkml/2020/9/29/2187 Can you please put this in a Link: like Ard suggested? (and ideally find the message in lore.kernel.org instead). Cheers, Miguel
Re: GCC section alignment, and GCC-4.9 being a weird one
On Wed, Oct 21, 2020 at 7:42 PM Nick Desaulniers wrote: > > If you used some of the macros from > include/linux/compiler_attributes.h like __section and __aligned, I > would prefer it. Please consider spelling out __attribute__(()) an > antipattern. +1, the shorthands should be used unless there is a reason not to (and please write the reason in a comment in that case). Cheers, Miguel
Re: [PATCH v4 00/32] Make charlcd device independent
On Fri, Oct 16, 2020 at 4:33 AM Miguel Ojeda wrote: > > Picking these for linux-next (including Rob's Reviewed-by). I have > spotted a few typos that I corrected -- I will note them by email. Hmm, I think we should do another round instead, since I found what looks to be an unintended revert of a previous commit in patch 24. Lars, can you please take a look? Also, please take the chance to apply my comments given we have a new round (and Rob's Reviewed-by too). By the way, I think you could simplify by squashing the "implement hd44780_*" commits together (i.e. from 15 to 22 except 20), and the two cleanups together too (i.e. 20 and 23). I know we asked you to split things up before, but those two sets are quite similar (including in their commit message) and easy to understand all together. Cheers, Miguel
Re: [PATCH v4 08/32] auxdisplay: hd44780_common_print
On Mon, Oct 5, 2020 at 2:27 PM wrote: > > We create a hd44780_common_print function. It is derived from the > original charlcd_print. charlcd_print becomes a device independent print > function, that then only calles via it's ops function pointers, into the Typos: calles -> calls, it's -> its > + * @clear_fast: Clear the whole display and set cursor to position 0, 0. This one is optional, but the comment seems to say it isn't (it is later removed, so in the end it doesn't matter, but probably we should write "Optional." here too). > + * @backlight: Turn backlight on or off. Optional. > + * @print: just Print one character to the display at current cursor > position. Typo: remove "just" Cheers, Miguel
Re: [PATCH v4 32/32] auxdisplay: add a driver for lcd2s character display
On Mon, Oct 5, 2020 at 3:01 PM wrote:
>
> + while (*esc && i < LCD2S_CHARACTER_SIZE + 2) {
> + shift ^= 4;
> + if (*esc >= '0' && *esc <= '9') {
> + value |= (*esc - '0') << shift;
> + } else if (*esc >= 'A' && *esc <= 'Z') {
> + value |= (*esc - 'A' + 10) << shift;
> + } else if (*esc >= 'a' && *esc <= 'z') {
> + value |= (*esc - 'a' + 10) << shift;
This should also probably use hex_to_bin() or similar (see my other
comment on patch 24) and/or share the implementation as much as
possible.
Cheers,
Miguel
Re: [PATCH v4 24/32] auxdisplay: Move char redefine code to hd44780_common
On Mon, Oct 5, 2020 at 3:01 PM wrote:
>
> + while (*esc && cgoffset < 8) {
> + shift ^= 4;
> + if (*esc >= '0' && *esc <= '9') {
> + value |= (*esc - '0') << shift;
> + } else if (*esc >= 'A' && *esc <= 'F') {
> + value |= (*esc - 'A' + 10) << shift;
> + } else if (*esc >= 'a' && *esc <= 'f') {
> + value |= (*esc - 'a' + 10) << shift;
I just noticed this is undoing commit 3f03b6498 ("auxdisplay: charlcd:
Reuse hex_to_bin() instead of custom code"). Lars?
Cheers,
Miguel
Re: [PATCH v4 14/32] auxdisplay: Move init_display to hd44780_common
On Mon, Oct 5, 2020 at 3:01 PM wrote: > > The init_display function is moved over to hd44780_common. charlcd uses > it via it's ops function pointer and drivers initialize the ops with the it's -> its (Already corrected in my queue) Cheers, Miguel
Re: [PATCH v4 02/32] auxdisplay: Introduce hd44780_common.[ch]
On Mon, Oct 5, 2020 at 2:27 PM wrote:
>
> diff --git a/drivers/auxdisplay/hd44780_common.c
> b/drivers/auxdisplay/hd44780_common.c
> new file mode 100644
> index ..073f47397f7d
> --- /dev/null
> +++ b/drivers/auxdisplay/hd44780_common.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include
> +#include
> +
> +#include "hd44780_common.h"
> +
> +struct hd44780_common *hd44780_common_alloc(void)
> +{
> + struct hd44780_common *hd;
> +
> + hd = kzalloc(sizeof(*hd), GFP_KERNEL);
> + if (!hd)
> + return NULL;
> +
> + return hd;
> +
Typo: spurious newline.
> +}
> +EXPORT_SYMBOL_GPL(hd44780_common_alloc);
> +
> +MODULE_LICENSE("GPL");
> +
Spurious EOF newline.
> diff --git a/drivers/auxdisplay/hd44780_common.h
> b/drivers/auxdisplay/hd44780_common.h
> new file mode 100644
> index ..767bbda91744
> --- /dev/null
> +++ b/drivers/auxdisplay/hd44780_common.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +struct hd44780_common {
> + void *hd44780;
> +};
> +
> +struct hd44780_common *hd44780_common_alloc(void);
> +
Spurious EOF newline.
(All already corrected in my queue).
Cheers,
Miguel
Re: [PATCH v4 01/32] auxdisplay: Use an enum for charlcd backlight on/off ops
On Mon, Oct 5, 2020 at 2:27 PM wrote: > > We use an enum for calling the functions in charlcd, that turn the > backlight on or off. This enum is generic and can be used for other > charlcd turn of / turn off operations as well. Typo: of -> on (Already corrected in my queue) Cheers, Miguel
Re: [PATCH v4 00/32] Make charlcd device independent
Hi Lars, On Tue, Oct 6, 2020 at 10:38 AM Lars Poeschel wrote: > > Changes in v4: > - modtronix -> Modtronix in new lcd2s driver > - Kconfig: remove "default n" in new lcd2s driver > > Changes in v3: > - Fix some typos in Kconfig stuff > - Fix kernel test robot reported error: Missed EXPORT_SYMBOL_GPL > - new patch to reduce display timeout > - better patch description to why not move cursor beyond end of a line > - Fixed make dt_binding_doc errors Picking these for linux-next (including Rob's Reviewed-by). I have spotted a few typos that I corrected -- I will note them by email. Cheers, Miguel
Re: [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS
On Wed, Oct 14, 2020 at 8:05 PM Joe Perches wrote: > > Any 'formatting off/on' marker should be tool agnostic. Agreed, they should have used a compiler-agnostic name for the marker. Cheers, Miguel
Re: [RFC PATCH v2] checkpatch: add shebang check to EXECUTE_PERMISSIONS
On Wed, Oct 14, 2020 at 10:40 AM Joe Perches wrote: > > Eek no. > > Mindless use of either tool isn't a great thing. That is up to opinion. I (and others) definitely want to get to the point the kernel sources are automatically formatted, because it has significant advantages. The biggest is that it saves a lot of time for everyone involved. > Linux source code has generally be created with > human readability in mind by humans, not scripts. Code readability is by definition for humans, and any code formatter (like clang-format) is written for them. > Please don't try to replace human readable code > with mindless tools. Please do :-) > There is a _lot_ of relatively inappropriate > output in how clang-format changes existing code > in the kernel. Sure, but note that: - Code that should be specially-formatted should be in a clang-format-off section to begin with, so it doesn't count. - Some people like to tweak identifiers or refactor code to make it fit in the line limit beautifully. If you are doing that, then you should do that for clang-format too. It is not fair to compare both outputs when the tool is restricted on the transformations it can perform. You can help the tool, too, the same way you can help yourself. - Some style differences may be worth ignoring if the advantage is getting automatic formatting. Cheers, Miguel
Re: [PATCH v4 00/32] Make charlcd device independent
Hi Lars, On Mon, Oct 5, 2020 at 2:27 PM wrote: > > This tries to make charlcd device independent. Thanks a lot for the work! I see you have written the differences between versions in each patch, but given there are 32 patches, it'd be nice to comment which ones have changed so that folks that already did a review can take a look at those. Cheers, Miguel
Re: [PATCH net-next v2 3/6] bonding: rename slave to port where possible
Hi Jarod, On Fri, Oct 2, 2020 at 7:44 PM Jarod Wilson wrote: > > .clang-format |4 +-> #ifdef > CONFIG_NET_POLL_CONTROLLER Acked-by: Miguel Ojeda Cheers, Miguel
Re: [RFC PATCH next-20200930] treewide: Convert macro and uses of __section(foo) to __section("foo")
Hi Joe, On Thu, Oct 1, 2020 at 12:56 AM Joe Perches wrote: > > So I installed the powerpc cross compiler, and > nope, that doesn't work, it makes a mess. Thanks a lot for reviving the script and sending the treewide cleanup! > So it looks like the best option is to exclude these > 2 files from conversion. Agreed. Nevertheless, is there any reason arch/powerpc/* should not be compiling cleanly with compiler.h? (CC'ing the rest of the PowerPC reviewers and ML). Cheers, Miguel
Re: [PATCH] compiler.h: avoid escaped section names
Hi Nick, On Tue, Sep 29, 2020 at 9:43 PM Nick Desaulniers wrote: > > The stringification operator, `#`, in the preprocessor escapes strings. > For example, `# "foo"` becomes `"\"foo\""`. GCC and Clang differ in how > they treat section names that contain \". > > The portable solution is to not use a string literal with the > preprocessor stringification operator. > > In this case, since __section unconditionally uses the stringification > operator, we actually want the more verbose > __attribute__((__section__())). Let's add a comment about this in the code -- otherwise we/someone will convert it back without noticing. Also we could add another on `__section` itself warning about this. > Link: https://bugs.llvm.org/show_bug.cgi?id=42950 Is there a link / have we opened a bug on GCC's side too? Thanks! Cheers, Miguel
Re: [PATCH v2 00/33] Make charlcd device independent
Hi Lars, On Mon, Sep 21, 2020 at 4:47 PM wrote: > > This tries to make charlcd device independent. Thanks a lot for the series! > [1] https://marc.info/?l=linux-kernel=157121432124507 Nit: please use lore.kernel.org and the Link: tag instead. Cheers, Miguel
Re: [PATCH v2 31/32] auxdisplay: charlcd: Do not print chars at end of line
Hi Lars, Willy, On Tue, Sep 22, 2020 at 12:04 PM Willy Tarreau wrote: > > The points you mention are good enough indicators that it's extremely > unlikely anyone has ever used that feature with these drivers. So I > think it's best to keep your changes and wait to see if anyone pops > up with an obscure use case, in which case their explanation might > help figure another approach. Agreed -- Lars, please add an explanation for the removed feature in the appropriate commit message and also mention it in the cover letter so that such user-facing change (and any other) is documented. Cheers, Miguel
