Re: [PATCH] livepatch: introduce klp_func called interface
Hi Wardenjohn, To follow up, Red Hat kpatch QE pointed me toward this test: https://gitlab.com/redhat/centos-stream/tests/kernel/kernel-tests/-/tree/main/general/kpatch/kpatch-trace?ref_type=heads which reports a few interesting things via systemd service and ftrace: - Patched functions - Traced functions - Code that was modified, but didn't run - Other code that ftrace saw, but is not listed in the sysfs directory The code looks to be built on top of the same basic ftrace commands that I sent the other day. You may be able to reuse/copy/etc bits from the scripts if they are handy. -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, Jun 04, 2024 at 04:14:51PM +0800, zhang warden wrote: > > > > On Jun 1, 2024, at 03:16, Joe Lawrence wrote: > > > > Adding these attributes to livepatch sysfs would be expedient and > > probably easier for us to use, but imposes a recurring burden on us to > > maintain and test (where is the documentation and kselftest for this new > > interface?). Or, we could let the other tools handle all of that for > > us. > How this attribute imposes a recurring burden to maintain and test? > Perhaps "responsibility" is a better description. This would introduce an attribute that someone's userspace utility is relying on. It should at least have a kselftest to ensure a random patch in 2027 doesn't break it. > > Perhaps if someone already has an off-the-shelf script that is using > > ftrace to monitor livepatched code, it could be donated to > > Documentation/livepatch/? I can ask our QE folks if they have something > > like this. > > My intention to introduce this attitude to sysfs is that user who what to see > if this function is called can just need to show this function attribute in > the livepatch sysfs interface. > > User who have no experience of using ftrace will have problems to get the > calling state of the patched function. After all, ftrace is a professional > kernel tracing tools. > > Adding this attribute will be more easier for us to show if this patched > function is called. Actually, I have never try to use ftrace to trace a > patched function. Is it OK of using ftrace for a livepatched function? > If you build with CONFIG_SAMPLE_LIVEPATCH=m, you can try it out (or with one of your own livepatches): # Convenience variable $ SYSFS=/sys/kernel/debug/tracing # Install the livepatch sample demo module $ insmod samples/livepatch/livepatch-sample.ko # Verify that ftrace can filter on our functions $ grep cmdline_proc_show $SYSFS/available_filter_functions cmdline_proc_show livepatch_cmdline_proc_show [livepatch_sample] # Turn off any existing tracing and filter functions $ echo 0 > $SYSFS/tracing_on $ echo > $SYSFS/set_ftrace_filter # Set up the function tracer and add the kernel's cmdline_proc_show() # and livepatch-sample's livepatch_cmdline_proc_show() $ echo function > $SYSFS/current_tracer $ echo cmdline_proc_show >> $SYSFS/set_ftrace_filter $ echo livepatch_cmdline_proc_show >> $SYSFS/set_ftrace_filter $ cat $SYSFS/set_ftrace_filter cmdline_proc_show livepatch_cmdline_proc_show [livepatch_sample] # Turn on the ftracing and force execution of the original and # livepatched functions $ echo 1 > $SYSFS/tracing_on $ cat /proc/cmdline this has been live patched # Checkout out the trace file results $ cat $SYSFS/trace # tracer: function # # entries-in-buffer/entries-written: 2/2 #P:8 # #_-=> irqs-off/BH-disabled # / _=> need-resched # | / _---=> hardirq/softirq # || / _--=> preempt-depth # ||| / _-=> migrate-disable # / delay # TASK-PID CPU# | TIMESTAMP FUNCTION # | | | | | | cat-254 [002] ...2. 363.043498: cmdline_proc_show <-seq_read_iter cat-254 [002] ...1. 363.043501: livepatch_cmdline_proc_show <-seq_read_iter The kernel docs provide a lot of explanation of the complete ftracing interface. It's pretty power stuff, though you may also go the other direction and look into using the trace-cmd front end to simplify all of the sysfs manipulation. Julia Evans wrote a blog [1] a while back that provides a some more examples. [1] https://jvns.ca/blog/2017/03/19/getting-started-with-ftrace/ -- Joe
Re: [PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs
On 5/31/24 07:23, Miroslav Benes wrote: > Hi, > > On Tue, 21 Jul 2020, Joe Lawrence wrote: > >> In light of [PATCH] Revert "kbuild: use -flive-patching when >> CONFIG_LIVEPATCH is enabled" [1], we should add some loud disclaimers >> and explanation of the impact compiler optimizations have on >> livepatching. >> >> The first commit provides detailed explanations and examples. The list >> was taken mostly from Miroslav's LPC talk a few years back. This is a >> bit rough, so corrections and additional suggestions welcome. Expanding >> upon the source-based patching approach would be helpful, too. >> >> The second commit adds a small README.rst file in the livepatch samples >> directory pointing the reader to the doc introduced in the first commit. >> >> I didn't touch the livepatch kselftests yet as I'm still unsure about >> how to best account for IPA here. We could add the same README.rst >> disclaimer here, too, but perhaps we have a chance to do something more. >> Possibilities range from checking for renamed functions as part of their >> build, or the selftest scripts, or even adding something to the kernel >> API. I think we'll have a better idea after reviewing the compiler >> considerations doc. > > thanks to Marcos for resurrecting this. > > Joe, do you have an updated version by any chance? Some things have > changed since July 2020 so it calls for a new review. If there was an > improved version, it would be easier. If not, no problem at all. > Yea, it's been a little while :) I don't have any newer version than this one. I can rebase, apply all of the v1 suggestions, and see where it stands. LMK if you can think of any specifics that could be added. For example, CONFIG_KERNEL_IBT will be driving some changes soon, whether it be klp-convert for source-based patches or vmlinux.o binary comparison for kpatch-build. I can push a v2 with a few changes, but IIRC, last time we reviewed this, it kinda begged the question of how someone is creating the livepatch in the first place. As long as we're fine holding that thought for a while longer, this doc may still be useful by itself. -- Joe
Re: [PATCH] livepatch: introduce klp_func called interface
On Tue, May 21, 2024 at 08:34:46AM +0200, Miroslav Benes wrote: > Hello, > > On Mon, 20 May 2024, zhang warden wrote: > > > > > > > > On May 20, 2024, at 14:46, Miroslav Benes wrote: > > > > > > Hi, > > > > > > On Mon, 20 May 2024, Wardenjohn wrote: > > > > > >> Livepatch module usually used to modify kernel functions. > > >> If the patched function have bug, it may cause serious result > > >> such as kernel crash. > > >> > > >> This is a kobject attribute of klp_func. Sysfs interface named > > >> "called" is introduced to livepatch which will be set as true > > >> if the patched function is called. > > >> > > >> /sys/kernel/livepatchcalled > > >> > > >> This value "called" is quite necessary for kernel stability > > >> assurance for livepatching module of a running system. > > >> Testing process is important before a livepatch module apply to > > >> a production system. With this interface, testing process can > > >> easily find out which function is successfully called. > > >> Any testing process can make sure they have successfully cover > > >> all the patched function that changed with the help of this interface. > > > > > > Even easier is to use the existing tracing infrastructure in the kernel > > > (ftrace for example) to track the new function. You can obtain much more > > > information with that than the new attribute provides. > > > > > > Regards, > > > Miroslav > > Hi Miroslav > > > > First, in most cases, testing process is should be automated, which make > > using existing tracing infrastructure inconvenient. > > could you elaborate, please? We use ftrace exactly for this purpose and > our testing process is also more or less automated. > > > Second, livepatch is > > already use ftrace for functional replacement, I don’t think it is a > > good choice of using kernel tracing tool to trace a patched function. > > Why? > > > At last, this attribute can be thought of as a state of a livepatch > > function. It is a state, like the "patched" "transition" state of a > > klp_patch. Adding this state will not break the state consistency of > > livepatch. > > Yes, but the information you get is limited compared to what is available > now. You would obtain the information that a patched function was called > but ftrace could also give you the context and more. > > It would not hurt to add a new attribute per se but I am wondering about > the reason and its usage. Once we have it, the commit message should be > improved based on that. > I agree with Miroslav about using ftrace, or Dan in the other thread about using gcov if even more granular coverage is needed. Admittedly, I would have to go find documentation to remember how to do either, but at least I'd be using well-tested facilities designed for this exact purpose. Adding these attributes to livepatch sysfs would be expedient and probably easier for us to use, but imposes a recurring burden on us to maintain and test (where is the documentation and kselftest for this new interface?). Or, we could let the other tools handle all of that for us. Perhaps if someone already has an off-the-shelf script that is using ftrace to monitor livepatched code, it could be donated to Documentation/livepatch/? I can ask our QE folks if they have something like this. Regards, -- Joe
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote: > Livepatches need to access external symbols which can't be handled > by the normal relocation mechanism. It is needed for two types > of symbols: > > + Symbols which can be local for the original livepatched function. > The alternative implementation in the livepatch sees them > as external symbols. > > + Symbols in modules which are exported via EXPORT_SYMBOL*(). They > must be handled special way otherwise the livepatch module would > depend on the livepatched one. Loading such livepatch would cause > loading the other module as well. > > The address of these symbols can be found via kallsyms. Or they can > be relocated using livepatch specific relocation sections as specified > in Documentation/livepatch/module-elf-format.txt. > > Currently, there is no trivial way to embed the required information as > requested in the final livepatch elf object. klp-convert solves this > problem by using annotations in the elf object to convert the relocation > accordingly to the specification, enabling it to be handled by the > livepatch loader. > > Given the above, create scripts/livepatch to hold tools developed for > livepatches and add source files for klp-convert there. > > Allow to annotate such external symbols in the livepatch by a macro > KLP_RELOC_SYMBOL(). It will create symbol with all needed > metadata. For example: > > extern char *saved_command_line \ > KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0); > > would create symbol > > $>readelf -r -W : > Relocation section '.rela.text' at offset 0x32e60 contains 10 entries: > Offset Info Type Symbol's Value > Symbol's Name + Addend > [...] > 0068 003c0002 R_X86_64_PC32 > .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4 > [...] > > Also add scripts/livepatch/klp-convert. The tool transforms symbols > created by KLP_RELOC_SYMBOL() to object specific rela sections > and rela entries which would later be proceed when the livepatch > or the livepatched object is loaded. > > For example, klp-convert would replace the above symbol with: > > $> readelf -r -W > Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 > entry: > Offset Info Type Symbol's Value > Symbol's Name + Addend > 0068 003c0002 R_X86_64_PC32 > .klp.sym.vmlinux.saved_command_line,0 - 4 > > klp-convert relies on libelf and on a list implementation. Add files > scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf > interfacing layer and scripts/livepatch/list.h, which is a list > implementation. > > Update Makefiles to correctly support the compilation of the new tool, > update MAINTAINERS file and add a .gitignore file. > > [jpoim...@redhat.com: initial version] > Signed-off-by: Josh Poimboeuf > [joe.lawre...@redhat.com: clean-up and fixes] > Signed-off-by: Joe Lawrence > [lhru...@suse.cz: klp-convert code, minimal approach] > Signed-off-by: Lukas Hruska > Reviewed-by: Marcos Paulo de Souza > --- > MAINTAINERS | 1 + > include/linux/livepatch.h | 19 + > scripts/Makefile| 1 + > scripts/livepatch/.gitignore| 1 + > scripts/livepatch/Makefile | 5 + > scripts/livepatch/elf.c | 817 > scripts/livepatch/elf.h | 73 +++ > scripts/livepatch/klp-convert.c | 284 +++ > scripts/livepatch/klp-convert.h | 23 + > scripts/livepatch/list.h| 391 +++ > 10 files changed, 1615 insertions(+) > create mode 100644 scripts/livepatch/.gitignore > create mode 100644 scripts/livepatch/Makefile > create mode 100644 scripts/livepatch/elf.c > create mode 100644 scripts/livepatch/elf.h > create mode 100644 scripts/livepatch/klp-convert.c > create mode 100644 scripts/livepatch/klp-convert.h > create mode 100644 scripts/livepatch/list.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 130b8b0bd4f7..d2facc1f4e15 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -12618,6 +12618,7 @@ F:include/uapi/linux/livepatch.h > F: kernel/livepatch/ > F: kernel/module/livepatch.c > F: samples/livepatch/ > +F: scripts/livepatch/ > F: tools/testing/selftests/livepatch/ > > LLC (802.2) > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..83bbcd1c43fd 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -235,6 +235,
Re: [PATCH v2 2/6] livepatch: Add klp-convert tool
truct symbol *sym, *tmpsym;
> + struct rela *rela, *tmprela;
> +
> + list_for_each_entry_safe(sym, tmpsym, >symbols, list) {
> + list_del(>list);
> + free(sym);
> + }
> + list_for_each_entry_safe(sec, tmpsec, >sections, list) {
> + list_for_each_entry_safe(rela, tmprela, >relas, list) {
> + list_del(>list);
> + free(rela);
> + }
> + list_del(>list);
> + free(sec);
> + }
> + if (elf->fd > 0)
> + close(elf->fd);
Alexy found another ELF coding bug here:
"Techically, it is "fd >= 0"."
I had coded fixes for these in a v8-devel that I never finished. It
shouldn't be too hard to fix these up in the minimal version of the
patchset, but lmk if you'd like a patch.
That's all for now. My plan is to try and turn off kpatch-build's
klp-relocation code and see how passing through to klp-convert fares.
That would give us a good comparison of real-world examples that need to
be handled and tested.
--
Joe
Re: [PATCH] livepatch: Add KLP_IDLE state
On 4/4/24 11:17, Petr Mladek wrote:
> On Tue 2024-04-02 09:52:31, Joe Lawrence wrote:
>> On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwar...@gmail.com wrote:
>>> From: Wardenjohn
>>>
>>> In livepatch, using KLP_UNDEFINED is seems to be confused.
>>> When kernel is ready, livepatch is ready too, which state is
>>> idle but not undefined. What's more, if one livepatch process
>>> is finished, the klp state should be idle rather than undefined.
>>>
>>> Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better
>>> in reading and understanding.
>>> ---
>>> include/linux/livepatch.h | 1 +
>>> kernel/livepatch/patch.c | 2 +-
>>> kernel/livepatch/transition.c | 24
>>> 3 files changed, 14 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>>> index 9b9b38e89563..c1c53cd5b227 100644
>>> --- a/include/linux/livepatch.h
>>> +++ b/include/linux/livepatch.h
>>> @@ -19,6 +19,7 @@
>>>
>>> /* task patch states */
>>> #define KLP_UNDEFINED -1
>>> +#define KLP_IDLE -1
>>
>> Hi Wardenjohn,
>>
>> Quick question, does this patch intend to:
>>
>> - Completely replace KLP_UNDEFINED with KLP_IDLE
>> - Introduce KLP_IDLE as an added, fourth potential state
>> - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain
>> conditions
>>
>> I ask because this patch leaves KLP_UNDEFINED defined and used in other
>> parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and
>> continues to use the same -1 enumeration.
>
> Having two names for the same state adds more harm than good.
>
> Honestly, neither "task->patch_state == KLP_UNDEFINED" nor "KLP_IDLE"
> make much sense.
>
> The problem is in the variable name. It is not a state of a patch.
> It is the state of the transition. The right solution would be
> something like:
>
> klp_target_state -> klp_transition_target_state
> task->patch_state -> task->klp_transition_state
> KLP_UNKNOWN -> KLP_IDLE
>
Yes, this is exactly how I think of these when reading the code. The
model starts to make a lot more sense once you look at it thru this lens :)
> But it would also require renaming:
>
> /proc//patch_state -> klp_transition_state
>
> which might break userspace tools => likely not acceptable.
>
>
> My opinion:
>
> It would be nice to clean this up but it does not look worth the
> effort.
>
Agreed. Instead of changing code and the sysfs interface, we could
still add comments like:
/* task patch transition target states */
#define KLP_UNDEFINED -1 /* idle, no transition in progress */
#define KLP_UNPATCHED0 /* transitioning to unpatched state */
#define KLP_PATCHED 1 /* transitioning to patched state */
/* klp transition target state */
static int klp_target_state = KLP_UNDEFINED;
struct task_struct = {
.patch_state= KLP_UNDEFINED, /* klp transition state */
Maybe just one comment is enough? Alternatively, we could elaborate in
Documentation/livepatch/livepatch.rst if it's really confusing.
Wardenjohn, since you're probably reading this code with fresh(er) eyes,
would any of the above be helpful?
--
Joe
Re: [PATCH] livepatch: Add KLP_IDLE state
On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwar...@gmail.com wrote: > From: Wardenjohn > > In livepatch, using KLP_UNDEFINED is seems to be confused. > When kernel is ready, livepatch is ready too, which state is > idle but not undefined. What's more, if one livepatch process > is finished, the klp state should be idle rather than undefined. > > Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better > in reading and understanding. > --- > include/linux/livepatch.h | 1 + > kernel/livepatch/patch.c | 2 +- > kernel/livepatch/transition.c | 24 > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > index 9b9b38e89563..c1c53cd5b227 100644 > --- a/include/linux/livepatch.h > +++ b/include/linux/livepatch.h > @@ -19,6 +19,7 @@ > > /* task patch states */ > #define KLP_UNDEFINED-1 > +#define KLP_IDLE -1 Hi Wardenjohn, Quick question, does this patch intend to: - Completely replace KLP_UNDEFINED with KLP_IDLE - Introduce KLP_IDLE as an added, fourth potential state - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain conditions I ask because this patch leaves KLP_UNDEFINED defined and used in other parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and continues to use the same -1 enumeration. -- Joe
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Fri, 2023-09-29 at 11:07 +0900, Justin Stitt wrote: > On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers > wrote: > > > > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches wrote: > > > > > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > > > > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > > > > Changes in v2: > > > > > > - remove formatting pass (thanks Joe) (but seriously the formatting > > > > > > is > > > > > > bad, is there opportunity to get a formatting pass in here at some > > > > > > point?) > > > > > LG G7 Battery Replacement | Guide | Is it actually a Samsung? I t > > > > > Why? What is it that makes you believe the formatting is bad? > > > > > > > > > > > > > Investigating further, it looked especially bad in my editor. There is > > > > a mixture of > > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting > > > > this to > > > > 8 is a whole lot better. But I still see some weird spacing > > > > > > > > > > Yes, it's a bit odd indentation. > > > It's emacs default perl format. > > > 4 space indent with 8 space tabs, maximal tab fill. > > > > > > > Oh! What?! That's the most surprising convention I've ever heard of > > (after the GNU C coding style). Yet another thing to hold against > > perl I guess. :P > > > > I have my editor setup to highlight tabs vs spaces via visual cues, so > > that I don't mess up kernel coding style. (`git clang-format HEAD~` > > after a commit helps). scripts/get_maintainer.pl has some serious > > inconsistencies to the point where I'm not sure what it should or was > > meant to be. Now that you mention it, I see it, and it does seem > > consistent in that regard. > > > > Justin, is your formatter configurable to match that convention? > > Maybe it's still useful, as long as you configure it to stick to the > > pre-existing convention. > > Negative, all the perl formatters I've tried will convert everything to > spaces. > The best I've seen is perltidy. > > https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6 emacs with cperl mode works fine. I don't know much about vim, but when I open this file in vim it looks perfectly normal and it's apparently properly syntax highlighted.
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > Changes in v2: > > > - remove formatting pass (thanks Joe) (but seriously the formatting is > > > bad, is there opportunity to get a formatting pass in here at some > > > point?) > > > > Why? What is it that makes you believe the formatting is bad? > > > > Investigating further, it looked especially bad in my editor. There is > a mixture of > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting this to > 8 is a whole lot better. But I still see some weird spacing > Yes, it's a bit odd indentation. It's emacs default perl format. 4 space indent with 8 space tabs, maximal tab fill.
Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
On Thu, 2023-09-28 at 14:03 +0900, Justin Stitt wrote: > On Thu, Sep 28, 2023 at 1:46 PM Joe Perches wrote: > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > Add the "D:" type which behaves the same as "K:" but will only match > > > content present in a patch file. [] > > > My opinion: Nack. > > > > I think something like this would be better > > as it avoids duplication of K and D content. > > If I understand correctly, this puts the onus on the get_maintainer users > to select the right argument whereas adding "D:", albeit with some > duplicate code, allows maintainers themselves to decide in exactly > which context they receive mail. Maybe, but I doubt it'll be significantly different. > This could all be a moot point, though, as I believe Konstantin > is trying to separate out the whole idea of a patch-sender needing > to specify the recipients of a patch. As I understand it, by using get_maintainer.
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > Changes in v2: > - remove formatting pass (thanks Joe) (but seriously the formatting is > bad, is there opportunity to get a formatting pass in here at some > point?) Why? What is it that makes you believe the formatting is bad?
Re: [PATCH v2 2/2] MAINTAINERS: migrate some K to D
On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > Let's get the ball rolling with some changes from K to D. > > Ultimately, if it turns out that 100% of K users want to change to D > then really the behavior of K could just be changed. Given my suggestion to 1/2, this would be unnecessary. > > Signed-off-by: Justin Stitt > Original-author: Kees Cook > --- > MAINTAINERS | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 94e431daa7c2..80ffdaa8f044 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5038,7 +5038,7 @@ F: Documentation/kbuild/llvm.rst > F: include/linux/compiler-clang.h > F: scripts/Makefile.clang > F: scripts/clang-tools/ > -K: \b(?i:clang|llvm)\b > +D: \b(?i:clang|llvm)\b > > CLK API > M: Russell King > @@ -8149,7 +8149,7 @@ F: lib/strcat_kunit.c > F: lib/strscpy_kunit.c > F: lib/test_fortify/* > F: scripts/test_fortify.sh > -K: \b__NO_FORTIFY\b > +D: \b__NO_FORTIFY\b > > FPGA DFL DRIVERS > M: Wu Hao > @@ -11405,8 +11405,10 @@ F: > Documentation/ABI/testing/sysfs-kernel-warn_count > F: include/linux/overflow.h > F: include/linux/randomize_kstack.h > F: mm/usercopy.c > -K: \b(add|choose)_random_kstack_offset\b > -K: \b__check_(object_size|heap_object)\b > +D: \b(add|choose)_random_kstack_offset\b > +D: \b__check_(object_size|heap_object)\b > +D: \b__counted_by\b > + > > KERNEL JANITORS > L: kernel-janit...@vger.kernel.org > @@ -17290,7 +17292,7 @@ F:drivers/acpi/apei/erst.c > F: drivers/firmware/efi/efi-pstore.c > F: fs/pstore/ > F: include/linux/pstore* > -K: \b(pstore|ramoops) > +D: \b(pstore|ramoops) > > PTP HARDWARE CLOCK SUPPORT > M: Richard Cochran > @@ -19231,8 +19233,8 @@ F:include/uapi/linux/seccomp.h > F: kernel/seccomp.c > F: tools/testing/selftests/kselftest_harness.h > F: tools/testing/selftests/seccomp/* > -K: \bsecure_computing > -K: \bTIF_SECCOMP\b > +D: \bsecure_computing > +D: \bTIF_SECCOMP\b > > SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER > M: Kamal Dasu >
Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> Add the "D:" type which behaves the same as "K:" but will only match
> content present in a patch file.
>
> To illustrate:
>
> Imagine this entry in MAINTAINERS:
>
> NEW REPUBLIC
> M: Han Solo
> W: https://www.jointheresistance.org
> D: \bstrncpy\b
>
> Our maintainer, Han, will only be added to the recipients if a patch
> file is passed to get_maintainer (like what b4 does):
> $ ./scripts/get_maintainer.pl 0004-some-change.patch
>
> If the above patch has a `strncpy` present in the subject, commit log or
> diff then Han will be to/cc'd.
>
> However, in the event of a file from the tree given like:
> $ ./scripts/get_maintainer.pl ./lib/string.c
>
> Han will not be noisily to/cc'd (like a K: type would in this
> circumstance)
>
> Signed-off-by: Justin Stitt
> ---
> MAINTAINERS | 5 +
> scripts/get_maintainer.pl | 12 ++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..94e431daa7c2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
> matches patches or files that contain one or more of the words
> printk, pr_info or pr_err
> One regex pattern per line. Multiple K: lines acceptable.
> + D: *Diff content regex* (perl extended) pattern match that applies only to
> + patches and not entire files (e.g. when using the get_maintainers.pl
> + script).
> + Usage same as K:.
> +
>
> Maintainers List
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index ab123b498fd9..a3e697926ddd 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
>
> my @typevalue = ();
> my %keyword_hash;
> +my %patch_keyword_hash;
> my @mfiles = ();
> my @self_test_info = ();
>
> @@ -369,8 +370,10 @@ sub read_maintainer_file {
> $value =~ s@([^/])$@$1/@;
> }
> } elsif ($type eq "K") {
> - $keyword_hash{@typevalue} = $value;
> - }
> + $keyword_hash{@typevalue} = $value;
> + } elsif ($type eq "D") {
> + $patch_keyword_hash{@typevalue} = $value;
> + }
> push(@typevalue, "$type:$value");
> } elsif (!(/^\s*$/ || /^\s*\#/)) {
> push(@typevalue, $line);
> @@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
> push(@keyword_tvi, $line);
> }
> }
> +foreach my $line(keys %patch_keyword_hash) {
> + if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
> +push(@keyword_tvi, $line);
> + }
> +}
> }
> }
> close($patch);
>
My opinion: Nack.
I think something like this would be better
as it avoids duplication of K and D content.
---
scripts/get_maintainer.pl | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..07e7d744cadb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -57,6 +57,7 @@ my $subsystem = 0;
my $status = 0;
my $letters = "";
my $keywords = 1;
+my $keywords_in_file = 0;
my $sections = 0;
my $email_file_emails = 0;
my $from_filename = 0;
@@ -272,6 +273,7 @@ if (!GetOptions(
'letters=s' => \$letters,
'pattern-depth=i' => \$pattern_depth,
'k|keywords!' => \$keywords,
+ 'kf|keywords-in-file!' => \$keywords_in_file,
'sections!' => \$sections,
'fe|file-emails!' => \$email_file_emails,
'f|file' => \$from_filename,
@@ -318,6 +320,7 @@ if ($sections || $letters ne "") {
$subsystem = 0;
$web = 0;
$keywords = 0;
+$keywords_in_file = 0;
$interactive = 0;
} else {
my $selections = $email + $scm + $status + $subsystem + $web;
@@ -548,16 +551,14 @@ foreach my $file (@ARGV) {
$file =~ s/^\Q${cur_path}\E//; #strip any absolute path
$file =~ s/^\Q${lk_path}\E//; #or the path to the lk tree
push(@files, $file);
- if ($file ne "MAINTAINERS" && -f $file && $keywords) {
+ if ($file ne "MAINTAINERS" && -f $file && $keywords &&
$keywords_in_file) {
open(my $f, '<', $file)
or die "$P: Can't open $file: $!\n";
my $text = do { local($/) ; <$f> };
close($f);
- if ($keywords) {
- foreach my $line (keys %keyword_hash) {
- if ($text =~ m/$keyword_hash{$line}/x) {
- push(@keyword_tvi, $line);
- }
+ foreach my $line (keys %keyword_hash) {
+ if ($text =~ m/$keyword_hash{$line}/x) {
+ push(@keyword_tvi, $line);
}
}
}
@@
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, 2023-09-27 at 09:15 -0700, Kees Cook wrote: > On Wed, Sep 27, 2023 at 03:19:16AM +, Justin Stitt wrote: > > Add the "D:" type which behaves the same as "K:" but will only match > > content present in a patch file. > > > > To illustrate: > > > > Imagine this entry in MAINTAINERS: > > > > NEW REPUBLIC > > M: Han Solo > > W: https://www.jointheresistance.org > > D: \bstrncpy\b > > > > Our maintainer, Han, will only be added to the recipients if a patch > > file is passed to get_maintainer (like what b4 does): > > $ ./scripts/get_maintainer.pl 0004-some-change.patch > > > > If the above patch has a `strncpy` present in the subject, commit log or > > diff then Han will be to/cc'd. > > > > However, in the event of a file from the tree given like: > > $ ./scripts/get_maintainer.pl ./lib/string.c > > > > Han will not be noisily to/cc'd (like a K: type would in this > > circumstance) > > > > Note that folks really shouldn't be using get_maintainer on tree files > > anyways [1]. > > > > [1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/ > > As Greg suggested, please drop the above paragraph and link. Then this > looks good to me. > > I would immediately want to send this patch too, so please feel free to > add this to your series (and I bet many other hints on "git grep 'K:.\\b'" > would want to switch from K: to D: too): > > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -5057,7 +5057,7 @@ F: Documentation/kbuild/llvm.rst > F: include/linux/compiler-clang.h > F: scripts/Makefile.clang > F: scripts/clang-tools/ > -K: \b(?i:clang|llvm)\b > +D: \b(?i:clang|llvm)\b etc... My assumption is that the K: --file use is just unnecessary and it'd be better to only use the K: lookup on patches. (and I've somehow stuffed up the receiving side of my email configuration so please ignore any emails to me that bounce for a while)
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > Add the "D:" type which behaves the same as "K:" but will only match > content present in a patch file. Likely it'd be less aggravating just to document that K: is only for patches and add a !$file test.
Re: [PATCH 2/3] get_maintainer: run perltidy
On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > I'm a first time contributor to get_maintainer.pl and the formatting is > suspicious. I am not sure if there is a particular reason it is the way > it is but I let my editor format it and submitted the diff here in this > patch. Capital NACK. Completely unnecessary and adds no value.
Re: [PATCH 1/3] MAINTAINERS: add documentation for D:
On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > Document what "D:" does. > > This is more or less the same as what "K:" does but only works for patch > files. Nack. I'd rather just add a !$file test to K: patterns.
Re: [PATCH][next] checkpatch: add a couple new alloc functions to alloc with multiplies check
On Tue, 2023-09-12 at 11:04 -0600, Gustavo A. R. Silva wrote:
> vmalloc() and vzalloc() functions have now 2-factor multiplication
> argument forms vmalloc_array() and vcalloc(), correspondingly.
> Add alloc-with-multiplies checks for these new functions.
>
> Link: https://github.com/KSPP/linux/issues/342
> Signed-off-by: Gustavo A. R. Silva
> ---
> scripts/checkpatch.pl | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7d16f863edf1..45265d0eee1b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7207,17 +7207,19 @@ sub process {
> "Prefer $3(sizeof(*$1)...) over $3($4...)\n" .
> $herecurr);
> }
>
> -# check for (kv|k)[mz]alloc with multiplies that could be
> kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> +# check for (kv|k|v)[mz]alloc with multiplies that could be
> kmalloc_array/kvmalloc_array/vmalloc_array/kvcalloc/kcalloc/vcalloc
> if ($perl_version_ok &&
> defined $stat &&
> - $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> {
> + $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,?/)
> {
> my $oldfunc = $3;
> my $a1 = $4;
> my $a2 = $10;
> my $newfunc = "kmalloc_array";
> $newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> + $newfunc = "vmalloc_array" if ($oldfunc eq "vmalloc");
> $newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
> $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> + $newfunc = "vcalloc" if ($oldfunc eq "vzalloc");
> my $r1 = $a1;
> my $r2 = $a2;
> if ($a1 =~ /^sizeof\s*\S/) {
> @@ -7233,7 +7235,7 @@ sub process {
>"Prefer $newfunc over $oldfunc with
> multiply\n" . $herectx) &&
> $cnt == 1 &&
> $fix) {
> - $fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> + $fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> }
> }
> }
Seems ok. Dunno how many more of these there might be like
devm_ variants, but maybe it'd be better style to use a hash
with replacement instead of the repeated if block
Something like:
---
scripts/checkpatch.pl | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7d16f863edf1c..bacb63518be14 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -854,6 +854,23 @@ foreach my $entry (keys %deprecated_apis) {
}
$deprecated_apis_search = "(?:${deprecated_apis_search})";
+our %alloc_with_multiply_apis = (
+ "kmalloc" => "kmalloc_array",
+ "kvmalloc" => "kvmalloc_array",
+ "vmalloc" => "vmalloc_array",
+ "kvzalloc" => "kvcalloc",
+ "kzalloc" => "kcalloc",
+ "vzalloc" => "vcalloc",
+);
+
+#Create a search pattern for all these strings to speed up a loop below
+our $alloc_with_multiply_search = "";
+foreach my $entry (keys %alloc_with_multiply_apis) {
+ $alloc_with_multiply_search .= '|' if ($alloc_with_multiply_search ne
"");
+ $alloc_with_multiply_search .= $entry;
+}
+$alloc_with_multiply_search = "(?:${alloc_with_multiply_search})";
+
our $mode_perms_world_writable = qr{
S_IWUGO |
S_IWOTH |
@@ -7210,14 +7227,11 @@ sub process {
# check for (kv|k)[mz]alloc with multiplies that could be
kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
if ($perl_version_ok &&
defined $stat &&
- $stat =~
/^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
{
+ $stat =~
/^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
{
my $oldfunc = $3;
+ my $newfunc = $alloc_with_multiply_apis{$oldfunc};
my $a1 = $4;
my $a2 = $10;
- my $newfunc = "kmalloc_array";
-
Re: [PATCH 1/1] video: hyperv_fb: Add ratelimit on error message
On Tue, 2021-04-20 at 08:44 -0700, Michael Kelley wrote:
> Due to a full ring buffer, the driver may be unable to send updates to
> the Hyper-V host. But outputing the error message can make the problem
> worse because console output is also typically written to the frame
> buffer. As a result, in some circumstances the error message is output
> continuously.
>
> Break the cycle by rate limiting the error message. Also output
> the error code for additional diagnosability.
>
> Signed-off-by: Michael Kelley
None of the callers of this function ever check the return status.
Why is important/useful to emit this message at all?
> ---
> drivers/video/fbdev/hyperv_fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 4dc9077..a7e6eea 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -308,7 +308,7 @@ static inline int synthvid_send(struct hv_device *hdev,
> VM_PKT_DATA_INBAND, 0);
>
>
> if (ret)
> - pr_err("Unable to send packet via vmbus\n");
> + pr_err_ratelimited("Unable to send packet via vmbus; error
> %d\n", ret);
>
>
> return ret;
> }
[PATCH] spi: bcm2835: Fix buffer overflow with CS able to go beyond limit.
It was previoulsy possible to have a device tree with more chips than
the driver supports and go off the end of CS arrays.
This patches inforces CS limit but sets that limit to the max of the
default limit and what is in the device tree when driver is loaded.
Signed-off-by: Joe Burmeister
---
drivers/spi/spi-bcm2835.c | 77 +--
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index aab6c7e5c114..cee761bfffe4 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -28,6 +28,7 @@
#include
#include /* FIXME: using chip internals */
#include /* FIXME: using chip internals */
+#include
#include
#include
@@ -134,7 +135,7 @@ struct bcm2835_spi {
int tx_prologue;
int rx_prologue;
unsigned int tx_spillover;
- u32 prepare_cs[BCM2835_SPI_NUM_CS];
+ u32 *prepare_cs;
struct dentry *debugfs_dir;
u64 count_transfer_polling;
@@ -147,9 +148,9 @@ struct bcm2835_spi {
unsigned int rx_dma_active;
struct dma_async_tx_descriptor *fill_tx_desc;
dma_addr_t fill_tx_addr;
- struct dma_async_tx_descriptor *clear_rx_desc[BCM2835_SPI_NUM_CS];
+ struct dma_async_tx_descriptor **clear_rx_desc;
dma_addr_t clear_rx_addr;
- u32 clear_rx_cs[BCM2835_SPI_NUM_CS] cacheline_aligned;
+ u32 *clear_rx_cs;
};
#if defined(CONFIG_DEBUG_FS)
@@ -875,14 +876,14 @@ static void bcm2835_dma_release(struct spi_controller
*ctlr,
if (ctlr->dma_rx) {
dmaengine_terminate_sync(ctlr->dma_rx);
- for (i = 0; i < BCM2835_SPI_NUM_CS; i++)
+ for (i = 0; i < ctlr->num_chipselect; i++)
if (bs->clear_rx_desc[i])
dmaengine_desc_free(bs->clear_rx_desc[i]);
if (bs->clear_rx_addr)
dma_unmap_single(ctlr->dma_rx->device->dev,
bs->clear_rx_addr,
-sizeof(bs->clear_rx_cs),
+sizeof(u32) * ctlr->num_chipselect,
DMA_TO_DEVICE);
dma_release_channel(ctlr->dma_rx);
@@ -978,7 +979,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr,
struct device *dev,
bs->clear_rx_addr = dma_map_single(ctlr->dma_rx->device->dev,
bs->clear_rx_cs,
- sizeof(bs->clear_rx_cs),
+ sizeof(u32) * ctlr->num_chipselect,
DMA_TO_DEVICE);
if (dma_mapping_error(ctlr->dma_rx->device->dev, bs->clear_rx_addr)) {
dev_err(dev, "cannot map clear_rx_cs - not using DMA mode\n");
@@ -987,7 +988,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr,
struct device *dev,
goto err_release;
}
- for (i = 0; i < BCM2835_SPI_NUM_CS; i++) {
+ for (i = 0; i < ctlr->num_chipselect; i++) {
bs->clear_rx_desc[i] = dmaengine_prep_dma_cyclic(ctlr->dma_rx,
bs->clear_rx_addr + i * sizeof(u32),
sizeof(u32), 0,
@@ -1209,6 +1210,12 @@ static int bcm2835_spi_setup(struct spi_device *spi)
struct gpio_chip *chip;
u32 cs;
+ if (spi->chip_select >= ctlr->num_chipselect) {
+ dev_err(>dev, "cs%d >= max %d\n", spi->chip_select,
+ ctlr->num_chipselect);
+ return -EINVAL;
+ }
+
/*
* Precalculate SPI slave's CS register value for ->prepare_message():
* The driver always uses software-controlled GPIO chip select, hence
@@ -1233,7 +1240,7 @@ static int bcm2835_spi_setup(struct spi_device *spi)
BCM2835_SPI_CS_CLEAR_RX;
dma_sync_single_for_device(ctlr->dma_rx->device->dev,
bs->clear_rx_addr,
- sizeof(bs->clear_rx_cs),
+ sizeof(u32) * ctlr->num_chipselect,
DMA_TO_DEVICE);
}
@@ -1286,39 +1293,71 @@ static int bcm2835_spi_setup(struct spi_device *spi)
return 0;
}
+
+#ifdef CONFIG_OF
+static int bcm2835_spi_get_num_chipselect(struct platform_device *pdev)
+{
+ return max_t(int, of_gpio_named_count(pdev->dev.of_node, "cs-gpios"),
+ BCM2835_SPI_NUM_CS);
+}
+#else
+static int bcm2835_spi_get_num_chipselect(struct platform_device *pdev)
+{
+ return BCM2835_SPI_NUM_CS;
+}
+#endi
Re: [PATCH v2] iommu/amd: Fix extended features logging
On Mon, 2021-04-19 at 22:23 +0300, Alexander Monakov wrote:
> On Sun, 11 Apr 2021, Joe Perches wrote:
>
> > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> > > solution
> > >
> > > drivers/iommu/amd/init.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > > index 596d0c413473..62913f82a21f 100644
> > > --- a/drivers/iommu/amd/init.c
> > > +++ b/drivers/iommu/amd/init.c
> > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
> > > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
> > >
> > >
> > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> > > - pci_info(pdev, "Extended features (%#llx):",
> > > - iommu->features);
> > > + pr_info("Extended features (%#llx):", iommu->features);
> > > +
> > > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> > > if (iommu_feature(iommu, (1ULL << i)))
> > > pr_cont(" %s", feat_str[i]);
> >
> > How about avoiding all of this by using a temporary buffer
> > and a single pci_info.
>
> I think it is mostly up to the maintainers, but from my perspective, it's not
> good to conflate such a simple bugfix with the substantial rewrite you are
> proposing (which also increases code complexity).
You and I have _significant_ differences in the definition of substantial.
Buffering the output is the preferred code style in preference to
pr_cont.
Do remember pr_cont should _only_ be used when absolutely necessary
as interleaving of other messages from other processes/threads can
and does occur.
[PATCH] spi: Handle SPI device setup callback failure.
If the setup callback failed, but the controller has auto_runtime_pm
and set_cs, the setup failure could be missed.
Signed-off-by: Joe Burmeister
---
drivers/spi/spi.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 8b283b2c1668..0c39178f4401 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3388,8 +3388,15 @@ int spi_setup(struct spi_device *spi)
mutex_lock(>controller->io_mutex);
- if (spi->controller->setup)
+ if (spi->controller->setup) {
status = spi->controller->setup(spi);
+ if (status) {
+ mutex_unlock(>controller->io_mutex);
+ dev_err(>controller->dev, "Failed to setup device:
%d\n",
+ status);
+ return status;
+ }
+ }
if (spi->controller->auto_runtime_pm && spi->controller->set_cs) {
status = pm_runtime_get_sync(spi->controller->dev.parent);
--
2.30.2
Re: [PATCH v5] printk: Userspace format enumeration support
On Mon, 2021-04-19 at 11:53 +0200, Greg Kroah-Hartman wrote: > Hm, 12734 of the pr_err() calls do live in drivers/, so most of those > should be dev_err(). Might be something good to throw at interns... That depends on how much churn you want to have in old drivers that generally don't have any users because the hardware is ancient or no longer manufactured. I suggest not changing those. But I believe a coccinelle script was written quite awhile ago to convert pr_ to dev_ when a struct device * is available. http://btrlinux.inria.fr/staging-media-replace-pr_-with-dev_/
Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Sun, Apr 18, 2021 at 10:08:30AM +0100, Jonathan Cameron wrote: > On Fri, 16 Apr 2021 18:49:01 +0100 > Joe Sandom wrote: > > > Driver implementation for AMS/TAOS tsl2591 ambient light sensor. > > > > This driver supports configuration via device tree and sysfs. > > Supported channels for raw infrared light intensity, > > raw combined light intensity and illuminance in lux. > > The driver additionally supports iio events on lower and > > upper thresholds. > > > > This is a very-high sensitivity light-to-digital converter that > > transforms light intensity into a digital signal. > > > > Datasheet: https://ams.com/tsl25911#tab/documents > > Signed-off-by: Joe Sandom > > Hi Joe, > > I was in two minds about whether to just apply this and roll in the below > suggestions + those Andy made or whether to ask you to do a v9. > > The naming and units things below are complex enough that I'm not 100% sure > I'd get the right so please take a look and clean up those last few > corners! > > Thanks, > > Jonathan > Thanks for the comments Jonathan. All good points! happy to clean up the last few bits in v9. I'll aim to get this out over the next few days. Thanks, Joe > > > --- > > Changes in v8; > > - tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not > > necessary > > - tsl2591_read_event_value() and tsl2591_write_event_value() - break > > instead of goto in default case > > - Introduced tsl2591_info_no_irq iio_info structure which is used when the > > interrupt is disabled. > > This variant doesn't expose anything that can't be used when the > > interrupt is disabled > > - Corrected ordering of includes for > > - Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME > > - Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC > > - Changed TSL2591_STS_ALS* defines to all use BIT for consistency > > - Use (BIT(4) - 1) instead of value from list for > > TSL2591_PRST_ALS_INT_CYCLE_MAX > > - Cleaned up a few blank lines that were unneccesary > > - Use sysfs_emit() instead of snprintf() in > > tsl2591_in_illuminance_period_available_show() > > - Use err_unlock branch instead of err to be clear on mutex_unlock cases > > - Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent > > with other functions > > > > NOTE; > > - tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold() > > forward declaration needed > > because tsl2591_set_als_lower_threshold() calls > > tsl2591_set_als_upper_threshold() and vice versa > > > > drivers/iio/light/Kconfig | 11 + > > drivers/iio/light/Makefile |1 + > > drivers/iio/light/tsl2591.c | 1227 +++ > > 3 files changed, 1239 insertions(+) > > create mode 100644 drivers/iio/light/tsl2591.c > > > > ... > > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > > index ea376deaca54..d10912faf964 100644 > > --- a/drivers/iio/light/Makefile > > +++ b/drivers/iio/light/Makefile > > @@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o > > obj-$(CONFIG_TCS3414) += tcs3414.o > > obj-$(CONFIG_TCS3472) += tcs3472.o > > obj-$(CONFIG_TSL2583) += tsl2583.o > > +obj-$(CONFIG_TSL2591) += tsl2591.o > > obj-$(CONFIG_TSL2772) += tsl2772.o > > obj-$(CONFIG_TSL4531) += tsl4531.o > > obj-$(CONFIG_US5182D) += us5182d.o > > diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c > > new file mode 100644 > > index ..4f9c5e19ee35 > > --- /dev/null > > +++ b/drivers/iio/light/tsl2591.c > > @@ -0,0 +1,1227 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * Copyright (C) 2020 Joe Sandom > > Update perhaps? > > > + * > > + * Datasheet: https://ams.com/tsl25911#tab/documents > > + * > > + * Device driver for the TAOS TSL2591. This is a very-high sensitivity > > + * light-to-digital converter that transforms light intensity into a > > digital > > + * signal. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#include > > +#include > > +#include > > + > > +/* ADC integration time, field value to time in ms */ >
Re: [PATCH v8 1/2] Added AMS tsl2591 driver implementation
On Sat, Apr 17, 2021 at 03:50:16PM +0300, Andy Shevchenko wrote: > On Fri, Apr 16, 2021 at 8:49 PM Joe Sandom wrote: > > > > Driver implementation for AMS/TAOS tsl2591 ambient light sensor. > > > > This driver supports configuration via device tree and sysfs. > > Supported channels for raw infrared light intensity, > > raw combined light intensity and illuminance in lux. > > The driver additionally supports iio events on lower and > > upper thresholds. > > > > This is a very-high sensitivity light-to-digital converter that > > transforms light intensity into a digital signal. > > Hmm... It's v8 and the subject line is wrongly formatted. > Please add the corresponding prefix "iio: light: ..." > Thanks for pointing that out Andy. I'll be sure to correct this in v9. > Otherwise it's in very good shape. > > ... > > > +/* TSL2591 enable register definitions */ > > +#define TSL2591_PWR_ON 0x01 > > +#define TSL2591_PWR_OFF 0x00 > > > +#define TSL2591_ENABLE_ALS 0x02 > > +#define TSL2591_ENABLE_ALS_INT 0x10 > > +#define TSL2591_ENABLE_SLEEP_INT0x40 > > +#define TSL2591_ENABLE_NP_INT 0x80 > > Is it a bitfield? > > ... > > > + als_lower_l = als_lower_threshold; > > >> 0, but it's up to you. > > > + als_lower_h = als_lower_threshold >> 8; > > ... > > > + als_upper_l = als_upper_threshold; > > + als_upper_h = als_upper_threshold >> 8; > > Ditto. > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH] base: power: runtime.c: Remove a unnecessary space
On Sun, 2021-04-18 at 09:11 +, Sebastian Fricke wrote: > Hey Joe, Hi Sebastian. > On 18.04.2021 00:09, Joe Perches wrote: > > On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote: > > > Remove a redundant space to improve the quality of the comment. > > I think this patch is not useful. > > It's not redundant. > > Thank you, I actually found this pattern a few more times but I wanted > to check first if this is a mistake or chosen consciously. [] > > For drivers/base/power/runtime.c, that 2 space after period style is used > > dozens of times and changing a single instance of it isn't very useful. Even in that single file it's not consistent. It's something like 3:1 for 2 spaces over 1 space after period. I believe it's done more by habit and author age than anything. If you learned to type using a typewriter and not a keyboard, then you likely still use 2 spaces after a period. > True and if I understand you correctly you would rather keep it as is > right? Yes. IMO: Whitespace in comments like this should not be changed unless there's some other significant benefit like better alignment. cheers, Joe > > > --- > > > Side-note: > > > I found this while reading the code, I don't believe it is important but > > > I thought it doesn't hurt to fix it. [] > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c [] > > > @@ -786,7 +786,7 @@ static int rpm_resume(struct device *dev, int > > > rpmflags) > > > } > > > > > > /* > > > - * See if we can skip waking up the parent. This is safe only if > > > + * See if we can skip waking up the parent. This is safe only if > > > * power.no_callbacks is set, because otherwise we don't know whether > > > * the resume will actually succeed. > > > */
[PATCH] net/wireless/bcom: constify ieee80211_get_response_rate return
It's not modified so make it const with the eventual goal of moving
data to text for various static struct ieee80211_rate arrays.
Signed-off-by: Joe Perches
---
drivers/net/wireless/broadcom/b43/main.c | 2 +-
drivers/net/wireless/broadcom/b43legacy/main.c | 2 +-
include/net/cfg80211.h | 2 +-
net/wireless/util.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/main.c
b/drivers/net/wireless/broadcom/b43/main.c
index 150a366e8f62..17bcec5f3ff7 100644
--- a/drivers/net/wireless/broadcom/b43/main.c
+++ b/drivers/net/wireless/broadcom/b43/main.c
@@ -4053,7 +4053,7 @@ static void b43_update_basic_rates(struct b43_wldev *dev,
u32 brates)
{
struct ieee80211_supported_band *sband =
dev->wl->hw->wiphy->bands[b43_current_band(dev->wl)];
- struct ieee80211_rate *rate;
+ const struct ieee80211_rate *rate;
int i;
u16 basic, direct, offset, basic_offset, rateptr;
diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c
b/drivers/net/wireless/broadcom/b43legacy/main.c
index 7692a2618c97..f64ebff68308 100644
--- a/drivers/net/wireless/broadcom/b43legacy/main.c
+++ b/drivers/net/wireless/broadcom/b43legacy/main.c
@@ -2762,7 +2762,7 @@ static void b43legacy_update_basic_rates(struct
b43legacy_wldev *dev, u32 brates
{
struct ieee80211_supported_band *sband =
dev->wl->hw->wiphy->bands[NL80211_BAND_2GHZ];
- struct ieee80211_rate *rate;
+ const struct ieee80211_rate *rate;
int i;
u16 basic, direct, offset, basic_offset, rateptr;
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 911fae42b0c0..ed07590bc72d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5606,7 +5606,7 @@ static inline bool cfg80211_channel_is_psc(struct
ieee80211_channel *chan)
* which is, for this function, given as a bitmap of indices of
* rates in the band's bitrate table.
*/
-struct ieee80211_rate *
+const struct ieee80211_rate *
ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
u32 basic_rates, int bitrate);
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 1bf0200f562a..382c5262d997 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -24,7 +24,7 @@
#include "rdev-ops.h"
-struct ieee80211_rate *
+const struct ieee80211_rate *
ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
u32 basic_rates, int bitrate)
{
Re: [PATCH] brcmsmac: fix shift on 4 bit masked value
On Sun, 2021-04-18 at 06:10 +, Kalle Valo wrote:
> Colin King wrote:
>
> > From: Colin Ian King
> >
> > The calculation of offtune_val seems incorrect, the u16 value in
> > pi->tx_rx_cal_radio_saveregs[2] is being masked with 0xf0 and then
> > shifted 8 places right so that always ends up as a zero result. I
> > believe the intended shift was 4 bits to the right. Fix this.
> >
> > [Note: not tested, I don't have the H/W]
> >
> > Addresses-Coverity: ("Operands don't affect result")
> > Fixes: 5b435de0d786 ("net: wireless: add brcm80211 drivers")
> > Signed-off-by: Colin Ian King
>
> I think this needs review from someone familiar with the hardware.
>
> Patch set to Changes Requested.
What "change" are you requesting here?
Likely there needs to be some other setting for the patch.
Perhaps "deferred" as you seem to be requesting a review
and there's no actual change necessary, just approval from
someone with the hardware and that someone test the patch.
Re: [PATCH] base: power: runtime.c: Remove a unnecessary space
On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote: > Remove a redundant space to improve the quality of the comment. I think this patch is not useful. It's not redundant. Two spaces after a period is commonly used to separate sentences. It's especially common when used with fixed pitch fonts. A trivial grep seems to show it's used about 50K times in comments. Though single space after period may be used about twice as often. $ git grep '^\s*\*.*\. [A-Z]' | wc -l 54439 $ git grep '^\s*\*.*\. [A-Z]' | wc -l 110003 For drivers/base/power/runtime.c, that 2 space after period style is used dozens of times and changing a single instance of it isn't very useful. > --- > Side-note: > I found this while reading the code, I don't believe it is important but > I thought it doesn't hurt to fix it. > --- > drivers/base/power/runtime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 18b82427d0cb..499434b84171 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -786,7 +786,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > } > > > /* > - * See if we can skip waking up the parent. This is safe only if > + * See if we can skip waking up the parent. This is safe only if > * power.no_callbacks is set, because otherwise we don't know whether > * the resume will actually succeed. > */
Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
On Sat, 2021-04-17 at 14:30 -0400, Jes Sorensen wrote: > On 4/17/21 1:52 PM, Kalle Valo wrote: > > "Gustavo A. R. Silva" wrote: > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, fix > > > multiple warnings by replacing /* fall through */ comments with > > > the new pseudo-keyword macro fallthrough; instead of letting the > > > code fall through to the next case. > > > > > > Notice that Clang doesn't recognize /* fall through */ comments as > > > implicit fall-through markings. > > > > > > Link: https://github.com/KSPP/linux/issues/115 > > > Signed-off-by: Gustavo A. R. Silva > > > > Patch applied to wireless-drivers-next.git, thanks. > > > > bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang > > > > Sorry this junk patch should not have been applied. I don't believe it's a junk patch. I believe your characterization of it as such is flawed. You don't like the style, that's fine, but: Any code in the kernel should not be a unique style of your own choosing when it could cause various compilers to emit unnecessary warnings. Please remember the kernel code base is a formed by a community with a nominally generally accepted style. There is a real desire in that community to both enable compiler warnings that might show defects and simultaneously avoid unnecessary compiler warnings. This particular change just avoids a possible compiler warning.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Fri, 2021-04-16 at 14:41 -0600, Khalid Aziz wrote: > On 4/15/21 8:08 PM, Joe Perches wrote: > > And while it's a lot more code, I'd prefer a solution that looks more > > like the other commonly used kernel logging extension mechanisms > > where adapter is placed before the format, ... in the argument list. > > Hi Joe, > > I don't mind making these changes. It is quite a bit of code but > consistency with other kernel code is useful. Would you like to finalize > this patch, or would you prefer that I take this patch as starting point > and finalize it? Probably better to apply Maciej's patches first and then something like this. btw: the coccinelle script was @@ expression a, b; @@ \(blogic_announce\|blogic_info\|blogic_notice\|blogic_warn\|blogic_err\)( - a, b + b, a , ...)
Re: [PATCH v2] checkpatch: Improve ALLOC_ARRAY_ARGS test
On Fri, 2021-04-16 at 19:57 +0200, Christophe JAILLET wrote:
> The devm_ variant of 'kcalloc()' and 'kmalloc_array()' are not tested
> Add the corresponding check.
>
> Signed-off-by: Christophe JAILLET
> ---
> v2: use a cleaner regex as proposed by Joe Perches
Acked-by: Joe Perches
> ---
> scripts/checkpatch.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 44b9dc330ac6..23697a6b1eaa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7006,7 +7006,7 @@ sub process {
> }
>
>
> # check for alloc argument mismatch
> - if ($line =~ /\b(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> + if ($line =~
> /\b((?:devm_)?(?:kcalloc|kmalloc_array))\s*\(\s*sizeof\b/) {
> WARN("ALLOC_ARRAY_ARGS",
> "$1 uses number as first arg, sizeof is generally
> wrong\n" . $herecurr);
> }
[PATCH v8 1/2] Added AMS tsl2591 driver implementation
Driver implementation for AMS/TAOS tsl2591 ambient light sensor. This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds. This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal. Datasheet: https://ams.com/tsl25911#tab/documents Signed-off-by: Joe Sandom --- Changes in v8; - tsl2591_write_raw() - goto after tsl2591_set_als_gain_int_time() not necessary - tsl2591_read_event_value() and tsl2591_write_event_value() - break instead of goto in default case - Introduced tsl2591_info_no_irq iio_info structure which is used when the interrupt is disabled. This variant doesn't expose anything that can't be used when the interrupt is disabled - Corrected ordering of includes for - Renamed TSL2591_FVAL_TO_ATIME to TSL2591_MSEC_TO_ATIME - Renamed TSL2591_ATIME_TO_FVAL to TSL2591_ATIME_TO_MSEC - Changed TSL2591_STS_ALS* defines to all use BIT for consistency - Use (BIT(4) - 1) instead of value from list for TSL2591_PRST_ALS_INT_CYCLE_MAX - Cleaned up a few blank lines that were unneccesary - Use sysfs_emit() instead of snprintf() in tsl2591_in_illuminance_period_available_show() - Use err_unlock branch instead of err to be clear on mutex_unlock cases - Tidied up use of goto err_unlock in tsl2591_read_raw() to be consistent with other functions NOTE; - tsl2591_set_als_lower_threshold() and tsl2591_set_als_upper_threshold() forward declaration needed because tsl2591_set_als_lower_threshold() calls tsl2591_set_als_upper_threshold() and vice versa drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile |1 + drivers/iio/light/tsl2591.c | 1227 +++ 3 files changed, 1239 insertions(+) create mode 100644 drivers/iio/light/tsl2591.c diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 33ad4dd0b5c7..6a69a9a3577a 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -501,6 +501,17 @@ config TSL2583 Provides support for the TAOS tsl2580, tsl2581 and tsl2583 devices. Access ALS data via iio, sysfs. +config TSL2591 +tristate "TAOS TSL2591 ambient light sensor" +depends on I2C +help + Select Y here for support of the AMS/TAOS TSL2591 ambient light sensor, + featuring channels for combined visible + IR intensity and lux illuminance. + Access data via iio and sysfs. Supports iio_events. + + To compile this driver as a module, select M: the + module will be called tsl2591. + config TSL2772 tristate "TAOS TSL/TMD2x71 and TSL/TMD2x72 Family of light and proximity sensors" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index ea376deaca54..d10912faf964 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o obj-$(CONFIG_TCS3414) += tcs3414.o obj-$(CONFIG_TCS3472) += tcs3472.o obj-$(CONFIG_TSL2583) += tsl2583.o +obj-$(CONFIG_TSL2591) += tsl2591.o obj-$(CONFIG_TSL2772) += tsl2772.o obj-$(CONFIG_TSL4531) += tsl4531.o obj-$(CONFIG_US5182D) += us5182d.o diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c new file mode 100644 index ..4f9c5e19ee35 --- /dev/null +++ b/drivers/iio/light/tsl2591.c @@ -0,0 +1,1227 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Joe Sandom + * + * Datasheet: https://ams.com/tsl25911#tab/documents + * + * Device driver for the TAOS TSL2591. This is a very-high sensitivity + * light-to-digital converter that transforms light intensity into a digital + * signal. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +/* ADC integration time, field value to time in ms */ +#define TSL2591_MSEC_TO_ATIME(x) (((x) + 1) * 100) +/* ADC integration time, time in ms to field value */ +#define TSL2591_ATIME_TO_MSEC(x) (((x) / 100) - 1) + +/* TSL2591 register set */ +#define TSL2591_ENABLE 0x00 +#define TSL2591_CONTROL 0x01 +#define TSL2591_AILTL 0x04 +#define TSL2591_AILTH 0x05 +#define TSL2591_AIHTL 0x06 +#define TSL2591_AIHTH 0x07 +#define TSL2591_NP_AILTL0x08 +#define TSL2591_NP_AILTH0x09 +#define TSL2591_NP_AIHTL0x0A +#define TSL2591_NP_AIHTH0x0B +#define TSL2591_PERSIST 0x0C +#define TSL2591_PACKAGE_ID 0x11 +#define TSL2591_DEVICE_ID 0x12 +#define TSL2591_STATUS 0x13 +#define TSL2591_C0_DATAL0x14 +#define TSL2591_C0_DATAH0x15 +#define TSL2591_C1_DATAL0x16 +#define TSL2591_C1_DATAH0x17 + +/* TSL2591 command
[PATCH v8 2/2] Added AMS tsl2591 device tree binding
Device tree binding for AMS/TAOS tsl2591 ambient light sensor.
This driver supports configuration via device tree and sysfs.
Supported channels for raw infrared light intensity,
raw combined light intensity and illuminance in lux.
The driver additionally supports iio events on lower and
upper thresholds.
This is a very-high sensitivity light-to-digital converter that
transforms light intensity into a digital signal.
Signed-off-by: Joe Sandom
Reviewed-by: Rob Herring
---
Changes in v8:
- No changes
Notes:
- Re-submitted to align the version with part 1 of the patch series
.../bindings/iio/light/amstaos,tsl2591.yaml | 50 +++
1 file changed, 50 insertions(+)
create mode 100644
Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
new file mode 100644
index ..596a3bc770f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/amstaos,tsl2591.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS)
+
+maintainers:
+ - Joe Sandom
+
+description: |
+ AMS/TAOS TSL2591 is a very-high sensitivity
+ light-to-digital converter that transforms light intensity into a digital
+ signal.
+
+properties:
+ compatible:
+const: amstaos,tsl2591
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+description:
+ Interrupt (INT:Pin 2) Active low. Should be set to IRQ_TYPE_EDGE_FALLING.
+ interrupt is used to detect if the light intensity has fallen below
+ or reached above the configured threshold values.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+tsl2591@29 {
+compatible = "amstaos,tsl2591";
+reg = <0x29>;
+interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
+ };
+};
+...
--
2.17.1
Re: [PATCH] checkpatch: Improve ALLOC_ARRAY_ARGS test
On Fri, 2021-04-16 at 18:51 +0200, Christophe JAILLET wrote:
> Le 16/04/2021 à 18:11, Joe Perches a écrit :
> > On Fri, 2021-04-16 at 17:58 +0200, Christophe JAILLET wrote:
> > > The devm_ variant of 'kcalloc()' and 'kmalloc_array()' are not tested
> > > Add the corresponding check.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -7006,9 +7006,9 @@ sub process {
> > > }
> > >
> > >
> > >
> > > # check for alloc argument mismatch
> > > - if ($line =~ /\b(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> > > + if ($line =~
> > > /\b(devm_|)(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> >
> > Perhaps nicer using
> I'll send a V2.
>
> Thx for the feedback.
>
> CJ
>
> >
> > if ($line =~
> > /\b((?:devm_)?(?:kcalloc|kmalloc_array))\s*\*\s*sizeof\b/) {
The \* above should be \(.
I can't type and apparently I don't proofread either.
I offer the excuse that the * and ( are adjacent on my keyboard...
cheers, Joe
Re: [PATCH] checkpatch: Improve ALLOC_ARRAY_ARGS test
On Fri, 2021-04-16 at 17:58 +0200, Christophe JAILLET wrote:
> The devm_ variant of 'kcalloc()' and 'kmalloc_array()' are not tested
> Add the corresponding check.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7006,9 +7006,9 @@ sub process {
> }
>
>
> # check for alloc argument mismatch
> - if ($line =~ /\b(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> + if ($line =~
> /\b(devm_|)(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
Perhaps nicer using
if ($line =~
/\b((?:devm_)?(?:kcalloc|kmalloc_array))\s*\*\s*sizeof\b/) {
> WARN("ALLOC_ARRAY_ARGS",
> - "$1 uses number as first arg, sizeof is generally
> wrong\n" . $herecurr);
> + "$1$2 uses number as first arg, sizeof is
> generally wrong\n" . $herecurr);
So there's only one capture group and this line doesn't need to be changed.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Fri, 2021-04-16 at 16:28 +0200, Maciej W. Rozycki wrote: > On Fri, 16 Apr 2021, Joe Perches wrote: > > > > I'm not sure if that complex message > > > routing via `blogic_msg' is worth having even, rather than calling > > > `printk' or suitable variants directly. > > > > It's to allow the message content to be added to the internal > > >msgbuf[adapter->msgbuflen] > > with strcpy for later use with blogic_show_info()/seq_write. > > I know, but it's not clear to me if it's worth it (a potential buffer > overrun there too, BTW). It's seq_ output so it's nominally an ABI. But then again, I don't use this at all so I don't care much either. It's also odd/bad form that one output KERN_ does not match its blogic_ (blogic_info is emitted at KERN_NOTICE)
Re: [PATCH v5] printk: Userspace format enumeration support
On Fri, 2021-04-16 at 14:56 +0100, Chris Down wrote: > Any better suggestions? :-) A gcc plugin that looks for functions marked __printf(fmt, pos) so any const fmt is stored.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Fri, 2021-04-16 at 12:48 +0200, Maciej W. Rozycki wrote: > I'm not sure if that complex message > routing via `blogic_msg' is worth having even, rather than calling > `printk' or suitable variants directly. It's to allow the message content to be added to the internal >msgbuf[adapter->msgbuflen] with strcpy for later use with blogic_show_info()/seq_write.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Thu, 2021-04-15 at 00:39 +0200, Maciej W. Rozycki wrote:
> Update BusLogic driver's messaging system to use `pr_cont' for
> continuation lines, bringing messy output:
>
> pci :00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> scsi: * BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> scsi0: Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
> scsi0: PCI Bus: 0, Device: 19, Address:
> 0xE0012000,
> Host Adapter SCSI ID: 7
> scsi0: Parity Checking: Enabled, Extended Translation: Enabled
> scsi0: Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
> scsi0: Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
> scsi0: Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
> scsi0: Driver Queue Depth: 211, Host Adapter Queue Depth: 192
> scsi0: Tagged Queue Depth:
> Automatic
> , Untagged Queue Depth: 3
> scsi0: SCSI Bus Termination: Both Enabled
> , SCAM: Disabled
>
> scsi0: *** BusLogic BT-958 Initialized Successfully ***
> scsi host0: BusLogic BT-958
>
> back to order:
>
> pci :00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> scsi: * BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> scsi0: Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
> scsi0: PCI Bus: 0, Device: 19, Address: 0xE0012000, Host Adapter SCSI ID: 7
> scsi0: Parity Checking: Enabled, Extended Translation: Enabled
> scsi0: Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
> scsi0: Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
> scsi0: Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
> scsi0: Driver Queue Depth: 211, Host Adapter Queue Depth: 192
> scsi0: Tagged Queue Depth: Automatic, Untagged Queue Depth: 3
> scsi0: SCSI Bus Termination: Both Enabled, SCAM: Disabled
> scsi0: *** BusLogic BT-958 Initialized Successfully ***
> scsi host0: BusLogic BT-958
>
> Also diagnostic output such as with the `BusLogic=TraceConfiguration'
> parameter is affected and becomes vertical and therefore hard to read.
> This has now been corrected, e.g.:
>
> pci :00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> blogic_cmd(86) Status = 30: 4 ==> 4: FF 05 93 00
> blogic_cmd(95) Status = 28: (Modify I/O Address)
> blogic_cmd(91) Status = 30: 1 ==> 1: 01
> blogic_cmd(04) Status = 30: 4 ==> 4: 41 41 35 30
> blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42
> 1D
> scsi: * BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff
> blogic_cmd(04) Status = 30: 4 ==> 4: 41 41 35 30
> blogic_cmd(0B) Status = 30: 3 ==> 3: 00 08 07
> blogic_cmd(0D) Status = 30: 34 ==> 34: 03 01 07 04 00 00 00 00 00 00 00 00 00
> 00 00 00 FF 42 44 46 FF 00 00 00 00 00 00 00 00 00 FF 00 FF 00
> blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42
> 1D
> blogic_cmd(84) Status = 30: 1 ==> 1: 37
> blogic_cmd(8B) Status = 30: 5 ==> 5: 39 35 38 20 20
> blogic_cmd(85) Status = 30: 1 ==> 1: 42
> blogic_cmd(86) Status = 30: 4 ==> 4: FF 05 93 00
> blogic_cmd(91) Status = 30: 64 ==> 64: 41 46 3E 20 39 35 38 20 20 00 C4 00 04
> 01 07 2F 07 04 35 FF FF FF FF FF FF FF FF FF FF 01 00 FE FF 08 FF FF 00 00 00
> 00 00 00 00 01 00 01 00 00 FF FF 00 00 00 00 00 00 00 00 00 00 00 00 00 FC
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
>
> etc.
In patch 2, vscnprintf should probably be used to make sure it's
0 terminated.
And while it's a lot more code, I'd prefer a solution that looks more
like the other commonly used kernel logging extension mechanisms
where adapter is placed before the format, ... in the argument list.
Today it's:
void blogic_msg(enum, fmt, adapter, ...);
without the __printf marking so there is one format/arg mismatch.
fyi: in the suggested patch below it's
- blogic_info("BIOS Address: 0x%lX, ", adapter,
- adapter->bios_addr);
+ blogic_cont(adapter, "BIOS Address: 0x%X, ",
+ adapter->bios_addr);
I'd prefer
__printf(3, 4)
void blogic_msg(enum, adapter, fmt, ...)
(or maybe void blogic_msg(adapter, enum, fmt, ...))
And there's a simple addition of a blogic_cont macro and extension
to blogic_msg to simplify the logic and obviousness of the logging
extension lines too.
I suggest this done with coccinelle and a little typing:
---
drivers/scsi/BusLogic.c | 496 +++-
drivers/scsi/BusLogic.h | 32 ++--
2 files changed, 341 insertions(+), 187 deletions(-)
diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
index
Re: [PATCH 0/3] Detect suspicious indentation after conditional
On Wed, 2021-04-14 at 14:18 -0700, Julius Werner wrote: > *friendly ping* > > Hi Andy, Joe, > > Any comments on this patch series? Are you guys the right point of > contact for checkpatch changes? I don't have any issue with this patch set, but Andy is really the person that should approve any changes to this block of code. > On Thu, Mar 25, 2021 at 8:50 PM Julius Werner wrote: > > > > This patch series is adding functionality to checkpatch.pl to test for > > incorrect code indentation after a conditional statement, like this: > > > > if (a) > > b; > > c; > > > > (Indentation implies that `c;` was guarded by the conditional, but it > > isn't.) The main part is re-sending a patch from Ivo Sieben that was > > already proposed in 2014 [1]. I don't know why it was never merged -- > > it seems that there was no discussion on it. I hope that it was only > > overlooked, because it works great, and I think this is a very important > > class of common error to catch. > > > > I have tested it extensively on the kernel tree and in the course of > > that found a few more edge cases that get fixed by the other two > > patches. With all these applied, the vast majority of hits I get from > > this check on the kernel tree are actual indentation errors or other > > code style violations (e.g. case label and statement on the same line). > > The only significant remaining group of false positives I found are > > cases of macros being defined within a function, which are overall very > > rare. I think the benefit of adding this check would far outweigh the > > remaining amount of noise. > > > > [1]: https://lore.kernel.org/patchwork/patch/465116 > > > > Ivo Sieben (1): > > Suspicious indentation detection after conditional statement > > > > Julius Werner (2): > > checkpatch: ctx_statement_block: Fix preprocessor guard tracking > > checkpatch: Ignore labels when checking indentation > > > > scripts/checkpatch.pl | 56 +++ > > 1 file changed, 52 insertions(+), 4 deletions(-) > > > > -- > > 2.29.2 > >
Re: [PATCH 2/2] Input: evbug - Use 'pr_debug()' instead of hand-writing it
On Thu, 2021-04-15 at 22:58 +0200, Christophe JAILLET wrote:
> 'printk(KERN_DEBUG pr_fmt(...))' can be replaced by a much less verbose
> 'pr_debug()'.
This is not really true because
printk(KERN_DEBUG ...);
will _always_ be emitted if the console level allows
pr_debug(...);
will _only_ be emitted if the console level allows _and_
DEBUG is defined or dynamic_debug is enabled
(and for dynamic_debug, only if specifically enabled)
DEBUG is defined and dynamic_debug is enabled
> diff --git a/drivers/input/evbug.c b/drivers/input/evbug.c
[]
> @@ -21,8 +21,8 @@ MODULE_LICENSE("GPL");
>
>
> static void evbug_event(struct input_handle *handle, unsigned int type,
> unsigned int code, int value)
> {
> - printk(KERN_DEBUG pr_fmt("Event. Dev: %s, Type: %d, Code: %d, Value:
> %d\n"),
> -dev_name(>dev->dev), type, code, value);
> + pr_debug("Event. Dev: %s, Type: %d, Code: %d, Value: %d\n",
> + dev_name(>dev->dev), type, code, value);
> }
Re: [PATCH] staging: greybus: Match parentheses alignment
On Wed, 2021-04-14 at 09:35 -0500, Alex Elder wrote: > On 4/14/21 9:29 AM, Joe Perches wrote: > > On Wed, 2021-04-14 at 08:17 -0500, Alex Elder wrote: > > > Perhaps (like the -W options for GCC) there > > > could be a way to specify in a Makefile which checkpatch > > > messages are reported/not reported? I don't claim that's > > > a good suggestion, but if I could optionally indicate > > > somewhere that "two consecutive blank lines is OK for > > > Greybus" (one example that comes to mind) I might do so. > > > > checkpatch already has --ignore= and --types= > > for the various classes of messages it emits. > > > > see: $ ./scripts/checkpatch.pl --list-types --verbose > > > > Dwaipayan Ray (cc'd) is supposedly working on expanding > > the verbose descriptions of each type. > > > > That's awesome, I wasn't aware of that. > > Any suggestions on a standardized way to say "in this > subtree, please provide these arguments to checkpatch.pl"? > > I can probably stick it in a README file or something, > but is there an existing best practice? There is no standardized mechanism for this checkpatch use. Putting something in a staging README is in general a good way for it to _not_ be read by people doing 'my first kernel patch'. I still think emitting a message for overly long identifiers could be a decent checkpatch test. https://lore.kernel.org/lkml/1518801207.13169.15.ca...@perches.com/
Re: [PATCH] staging: greybus: Match parentheses alignment
On Wed, 2021-04-14 at 08:17 -0500, Alex Elder wrote: > Perhaps (like the -W options for GCC) there > could be a way to specify in a Makefile which checkpatch > messages are reported/not reported? I don't claim that's > a good suggestion, but if I could optionally indicate > somewhere that "two consecutive blank lines is OK for > Greybus" (one example that comes to mind) I might do so. checkpatch already has --ignore= and --types= for the various classes of messages it emits. see: $ ./scripts/checkpatch.pl --list-types --verbose Dwaipayan Ray (cc'd) is supposedly working on expanding the verbose descriptions of each type.
[PATCH] Remove BCM2835 SPI chipselect limit.
The limit of 4 chipselects for the BCM2835 was not required and also was
not inforced. Without inforcement it was possible to make a device tree
over this limit which would trample memory.
The chipselect count is now obtained from the device tree and expanded
if more devices are added.
Signed-off-by: Joe Burmeister
---
drivers/spi/spi-bcm2835.c | 114 +-
1 file changed, 101 insertions(+), 13 deletions(-)
diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index aab6c7e5c114..4f215ec3bd1b 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -28,6 +28,7 @@
#include
#include /* FIXME: using chip internals */
#include /* FIXME: using chip internals */
+#include
#include
#include
@@ -134,7 +135,8 @@ struct bcm2835_spi {
int tx_prologue;
int rx_prologue;
unsigned int tx_spillover;
- u32 prepare_cs[BCM2835_SPI_NUM_CS];
+ unsigned int allocated_cs_num;
+ u32 *prepare_cs;
struct dentry *debugfs_dir;
u64 count_transfer_polling;
@@ -147,9 +149,9 @@ struct bcm2835_spi {
unsigned int rx_dma_active;
struct dma_async_tx_descriptor *fill_tx_desc;
dma_addr_t fill_tx_addr;
- struct dma_async_tx_descriptor *clear_rx_desc[BCM2835_SPI_NUM_CS];
+ struct dma_async_tx_descriptor **clear_rx_desc;
dma_addr_t clear_rx_addr;
- u32 clear_rx_cs[BCM2835_SPI_NUM_CS] cacheline_aligned;
+ u32 *clear_rx_cs;
};
#if defined(CONFIG_DEBUG_FS)
@@ -859,14 +861,18 @@ static void bcm2835_dma_release(struct spi_controller
*ctlr,
if (ctlr->dma_tx) {
dmaengine_terminate_sync(ctlr->dma_tx);
- if (bs->fill_tx_desc)
+ if (bs->fill_tx_desc) {
dmaengine_desc_free(bs->fill_tx_desc);
+ bs->fill_tx_desc = NULL;
+ }
- if (bs->fill_tx_addr)
+ if (bs->fill_tx_addr) {
dma_unmap_page_attrs(ctlr->dma_tx->device->dev,
bs->fill_tx_addr, sizeof(u32),
DMA_TO_DEVICE,
DMA_ATTR_SKIP_CPU_SYNC);
+ bs->fill_tx_addr = 0;
+ }
dma_release_channel(ctlr->dma_tx);
ctlr->dma_tx = NULL;
@@ -875,15 +881,19 @@ static void bcm2835_dma_release(struct spi_controller
*ctlr,
if (ctlr->dma_rx) {
dmaengine_terminate_sync(ctlr->dma_rx);
- for (i = 0; i < BCM2835_SPI_NUM_CS; i++)
- if (bs->clear_rx_desc[i])
+ for (i = 0; i < bs->allocated_cs_num; i++)
+ if (bs->clear_rx_desc[i]) {
dmaengine_desc_free(bs->clear_rx_desc[i]);
+ bs->clear_rx_desc[i] = NULL;
+ }
- if (bs->clear_rx_addr)
+ if (bs->clear_rx_addr) {
dma_unmap_single(ctlr->dma_rx->device->dev,
bs->clear_rx_addr,
-sizeof(bs->clear_rx_cs),
+sizeof(u32) * bs->allocated_cs_num,
DMA_TO_DEVICE);
+ bs->clear_rx_addr = 0;
+ }
dma_release_channel(ctlr->dma_rx);
ctlr->dma_rx = NULL;
@@ -978,7 +988,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr,
struct device *dev,
bs->clear_rx_addr = dma_map_single(ctlr->dma_rx->device->dev,
bs->clear_rx_cs,
- sizeof(bs->clear_rx_cs),
+ sizeof(u32) * bs->allocated_cs_num,
DMA_TO_DEVICE);
if (dma_mapping_error(ctlr->dma_rx->device->dev, bs->clear_rx_addr)) {
dev_err(dev, "cannot map clear_rx_cs - not using DMA mode\n");
@@ -987,7 +997,7 @@ static int bcm2835_dma_init(struct spi_controller *ctlr,
struct device *dev,
goto err_release;
}
- for (i = 0; i < BCM2835_SPI_NUM_CS; i++) {
+ for (i = 0; i < bs->allocated_cs_num; i++) {
bs->clear_rx_desc[i] = dmaengine_prep_dma_cyclic(ctlr->dma_rx,
bs->clear_rx_addr + i * sizeof(u32),
sizeof(u32), 0,
@@ -1209,6 +1219,48 @@ static int bcm2835_spi_setup(struct spi_device *spi)
struct gpio_chip *chip;
u32 cs;
+ if (spi->chip_select >= bs->allocated_cs_num) {
+ unsigned int ne
Re: [PATCH 12/19] staging: rtl8723bs: remove unnecessary bracks on DBG_871X removal sites
On Tue, 2021-04-13 at 17:52 +0300, Dan Carpenter wrote:
> On Wed, Apr 07, 2021 at 03:49:36PM +0200, Fabio Aiuto wrote:
> > @@ -2586,11 +2583,9 @@ static int rtw_dbg_port(struct net_device *dev,
> >
> >
> > plist =
> > get_next(plist);
> >
> >
> > - if (extra_arg
> > == psta->aid) {
> > - for (j
> > = 0; j < 16; j++) {
> > + if (extra_arg
> > == psta->aid)
> > + for (j
> > = 0; j < 16; j++)
> >
> > preorder_ctrl = >recvreorder_ctrl[j];
> > - }
> > - }
>
> I think Greg already applied this so no stress (don't bother fixing),
> but you removed a bit too much on this one. Multi-line indents normally
> get curly braces for readability. In other words:
>
> if (extra_arg == psta->aid) {
> for (j = 0; j < 16; j++)
> preorder_ctrl =
> >recvreorder_ctrl[j];
> }
>
> regards,
> dan carpenter
>
Re: [PATCH] staging: media: tegra-vde: Align line break to match with the open parenthesis in file trace.h
On Tue, 2021-04-13 at 21:35 +0530, Dwaipayan Ray wrote:
> On Tue, Apr 13, 2021 at 8:59 PM Thierry Reding
> wrote:
> >
> > On Mon, Apr 12, 2021 at 07:20:40PM -0300, Aline Santana Cordeiro wrote:
> > > Align line break to match with the open parenthesis.
> > > Issue detected by checkpatch.pl.
> > > It consequently solved a few end lines with a '(',
> > > issue also detected by checkpatch.pl
> > >
> > > Signed-off-by: Aline Santana Cordeiro
> > > ---
> > > drivers/staging/media/tegra-vde/trace.h | 111
> > > ++--
> > > 1 file changed, 50 insertions(+), 61 deletions(-)
> >
> > Ugh... I'd say this is one case where checkpatch.pl really shouldn't be
> > complaining since this isn't a function call or declaration. It's more
> > like a code snippet written with macros, so I don't think the same rules
> > should apply.
> >
> > Adding checkpatch folks (hence quoting in full): is there anything we
> > can do about this without too much effort? Or should we just accept this
> > the way it is?
> >
>
> While it may be possible to add exceptions for trace headers on the
> alignment rules, I don't know how many more such exceptions we will
> end up adding. Such fine grained checks might be unnecessarily complex.
> So I think we should just accept the way it is for now.
>
> Joe might have a different opinion?
Tracing uses a different style.
Maybe just suppress various messages for complete code blocks
of DECLARE_EVENT_CLASS, DEFINE_EVENT, and TRACE_EVENT
Re: [PATCH v4 3/3] staging: rtl8192e: remove unnecessary parentheses
On Mon, 2021-04-12 at 16:52 +0530, Mitali Borkar wrote: > Removed unnecessary parentheses because they must be used only when it > is necessary or they improve readability. > Reported by checkpatch. I gave you feedback about the memset changes. Please use it. https://lore.kernel.org/lkml/f5fe04d62b22eb5e09c299e769d9b9d8917f119c.ca...@perches.com/ > Changes from v3:- No changes. > Changes from v2:- Rectified spelling mistake in subject description. > Changes has been made in v3. > Changes from v1:- No changes. [] > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c > b/drivers/staging/rtl8192e/rtl819x_HTProc.c [] > @@ -646,13 +646,13 @@ void HTInitializeHTInfo(struct rtllib_device *ieee) > pHTInfo->CurrentMPDUDensity = pHTInfo->MPDU_Density; > pHTInfo->CurrentAMPDUFactor = pHTInfo->AMPDU_Factor; > > > - memset((void *)(&(pHTInfo->SelfHTCap)), 0, > + memset((void *)(>SelfHTCap), 0, > sizeof(pHTInfo->SelfHTCap)); memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap); etc...
Re: [PATCH v2] iommu/amd: Fix extended features logging
On Mon, 2021-04-12 at 00:13 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
>
> pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
> PPR X2APIC NX GT IA GA PC GA_vAPIC
>
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
>
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
>
> Printing the decoded features on the same line would make it quite long.
> Instead, change pci_info() to pr_info() to omit PCI bus location info,
> which is shown in the preceding message anyway. This results in:
>
> pci :00:00.2: AMD-Vi: Found IOMMU cap 0x40
> AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC
> GA_vAPIC
> AMD-Vi: Interrupt remapping enabled
>
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
> divergent log levels")
> Link:
> https://lore.kernel.org/lkml/alpine.lnx.2.20.13.2104112326460.11...@monopod.intra.ispras.ru
> Signed-off-by: Alexander Monakov
> Cc: Paul Menzel
> Cc: Joerg Roedel
> Cc: Suravee Suthikulpanit
> Cc: io...@lists.linux-foundation.org
> ---
>
> v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> solution
>
> drivers/iommu/amd/init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..62913f82a21f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
> pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>
>
> if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> - iommu->features);
> + pr_info("Extended features (%#llx):", iommu->features);
> +
> for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> if (iommu_feature(iommu, (1ULL << i)))
> pr_cont(" %s", feat_str[i]);
How about avoiding all of this by using a temporary buffer
and a single pci_info.
Miscellanea:
o Move the feat_str and i declarations into the if block for locality
---
drivers/iommu/amd/init.c | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 321f5906e6ed..0d219044726e 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu
*iommu)
static void print_iommu_info(void)
{
- static const char * const feat_str[] = {
- "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
- "IA", "GA", "HE", "PC"
- };
struct amd_iommu *iommu;
for_each_iommu(iommu) {
struct pci_dev *pdev = iommu->dev;
- int i;
pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pci_info(pdev, "Extended features (%#llx):",
-iommu->features);
+ static const char * const feat_str[] = {
+ "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
+ "IA", "GA", "HE", "PC"
+ };
+ int i;
+ char features[128] = "";
+ int len = 0;
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
- if (iommu_feature(iommu, (1ULL << i)))
- pr_cont(" %s", feat_str[i]);
+ if (!iommu_feature(iommu, BIT_ULL(i)))
+ continue;
+ len += scnprintf(features + len,
+sizeof(features) - len,
+" %s", feat_str[i]);
}
if (iommu->features & FEATURE_GAM_VAPIC)
- pr_cont(" GA_vAPIC");
+ len += scnprintf(features + len,
+sizeof(features) - len,
+" %s", "GA_vPIC");
- pr_cont("\n");
+ pci_info(pdev, "Extended features (%#llx):%s\n",
+iommu->features, features);
}
}
if (irq_remapping_enabled) {
Re: [PATCH] iommu/amd: Fix extended features logging
On Sun, 2021-04-11 at 21:52 +0200, John Ogness wrote: > I'd rather fix dev_info callers to allow pr_cont and then fix any code > that is using this workaround. Assuming you mean all dev_() uses, me too. > And if the print maintainers agree it is OK to encourage > pr_cont(LOGLEVEL "...") usage, then people should really start using > that if the loglevel on those pieces is important. I have no stong feeling about the use of pr_cont( as valuable or not. I think it's just a trivial bit that could be somewhat useful when interleaving occurs. A somewhat better mechanism would be to have an explicit cookie use like: cookie = printk_multipart_init(KERN_LEVEL, fmt, ...); while () printk_multipart_cont(cookie, fmt, ...); printk_multipark_end(cookie, fmt, ...); And separately, there should be a pr_debug_cont or equivalent.
Re: [PATCH] iommu/amd: Fix extended features logging
(cc'ing the printk maintainers)
On Sun, 2021-04-11 at 00:11 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
>
> pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
> PPR X2APIC NX GT IA GA PC GA_vAPIC
>
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
>
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
>
> Restore the newline, and call pr_info with empty format string to set
> the loglevel for subsequent pr_cont calls. The same solution is used in
> EFI and uvesafb drivers.
>
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
> divergent log levels")
> Signed-off-by: Alexander Monakov
> Cc: Paul Menzel
> Cc: Joerg Roedel
> Cc: Suravee Suthikulpanit
> Cc: io...@lists.linux-foundation.org
> ---
> drivers/iommu/amd/init.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..a25e241eff1c 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,11 @@ static void print_iommu_info(void)
> pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>
>
> if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> + pci_info(pdev, "Extended features (%#llx):\n",
> iommu->features);
> +
> + pr_info("");
> +
> for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> if (iommu_feature(iommu, (1ULL << i)))
> pr_cont(" %s", feat_str[i]);
This shouldn't be necessary.
If this is true then a lot of output logging code broke.
Re: drivers/parport/parport_cs.c:147 parport_config() warn: inconsistent indenting
On Sun, 2021-04-11 at 02:02 +0800, kernel test robot wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: d4961772226de3b48a395a26c076d450d7044c76 > commit: decf26f6ec25dac868782dc1751623a87d147831 parport: Convert > printk(KERN_ to pr_( > date: 12 months ago > config: x86_64-randconfig-m001-20210410 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > smatch warnings: > drivers/parport/parport_cs.c:147 parport_config() warn: inconsistent indenting False positive. The whole thing is inconsistently indented.
Re: [PATCH 4/6] staging: rtl8192e: matched alignment with open parenthesis
On Sat, 2021-04-10 at 07:55 +0530, Mitali Borkar wrote: > On Fri, Apr 09, 2021 at 07:07:09PM -0700, Joe Perches wrote: > > On Sat, 2021-04-10 at 07:05 +0530, Mitali Borkar wrote: > > > Matched the alignment with open parenthesis to meet linux kernel coding > > > style. > > > Reported by checkpatch. > > [] > > > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > b/drivers/staging/rtl8192e/rtl819x_HTProc.c > > [] > > > @@ -154,7 +154,7 @@ bool IsHTHalfNmodeAPs(struct rtllib_device *ieee) > > > (net->ralink_cap_exist)) > > > retValue = true; > > > else if (!memcmp(net->bssid, UNKNOWN_BORADCOM, 3) || > > > - !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) || > > > + !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) || > > > !memcmp(net->bssid, LINKSYSWRT350_LINKSYSWRT150_BROADCOM, 3) || > > > (net->broadcom_cap_exist)) > > > > checkpatch is a stupid script. > > Look further at the code not just at what checkpatch reports. > > Align all the contination lines, not just the first one. > > > Sir, I have aligned them in last patch of this patchset. This sort of change should not require an additional patch.
Re: [PATCH 4/6] staging: rtl8192e: matched alignment with open parenthesis
On Sat, 2021-04-10 at 07:05 +0530, Mitali Borkar wrote:
> Matched the alignment with open parenthesis to meet linux kernel coding
> style.
> Reported by checkpatch.
[]
> diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c
> b/drivers/staging/rtl8192e/rtl819x_HTProc.c
[]
> @@ -154,7 +154,7 @@ bool IsHTHalfNmodeAPs(struct rtllib_device *ieee)
> (net->ralink_cap_exist))
> retValue = true;
> else if (!memcmp(net->bssid, UNKNOWN_BORADCOM, 3) ||
> - !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) ||
> + !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) ||
> !memcmp(net->bssid, LINKSYSWRT350_LINKSYSWRT150_BROADCOM, 3) ||
> (net->broadcom_cap_exist))
checkpatch is a stupid script.
Look further at the code not just at what checkpatch reports.
Align all the contination lines, not just the first one.
It might be sensible to add a generic function like
static inline bool ether_oui_equal(const u8 *addr, const u8 *oui)
{
return addr[0] == oui[0] && addr[1] == oui[1] && addr[2] == oui[2];
}
to include/linux/etherdevice.h
(Maybe use & instead of && if it's speed sensitive)
so this would read
else if (ether_oui_equal(net->bssid, UNKNOWN_BORADCOM) ||
ether_oui_equal(net->bssid,
LINKSYSWRT330_LINKSYSWRT300_BROADCOM) ||
ether_oui_equal(net->bssid,
LINKSYSWRT350_LINKSYSWRT150_BROADCOM) ||
net->broacom_cap_exist)
and it'd also be good to correct the typo of UNKNOWN_BORADCOM globally.
> @@ -654,13 +654,13 @@ void HTInitializeHTInfo(struct rtllib_device *ieee)
> pHTInfo->CurrentAMPDUFactor = pHTInfo->AMPDU_Factor;
>
>
> memset((void *)(&(pHTInfo->SelfHTCap)), 0,
> - sizeof(pHTInfo->SelfHTCap));
> +sizeof(pHTInfo->SelfHTCap));
Doesn't need casts or parentheses.
memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHCap));
> memset((void *)(&(pHTInfo->SelfHTInfo)), 0,
> - sizeof(pHTInfo->SelfHTInfo));
> +sizeof(pHTInfo->SelfHTInfo));
etc...
> @@ -815,7 +815,7 @@ void HTUseDefaultSetting(struct rtllib_device *ieee)
> HTFilterMCSRate(ieee, ieee->Regdot11TxHTOperationalRateSet,
> ieee->dot11HTOperationalRateSet);
> ieee->HTHighestOperaRate = HTGetHighestMCSRate(ieee,
> -ieee->dot11HTOperationalRateSet,
> +
> ieee->dot11HTOperationalRateSet,
> MCS_FILTER_ALL);
multi line statement alignment etc...
Re: [PATCH v5 2/6] w1: ds2438: fixed if brackets coding style issue
On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote:
> Since there is only one statement inside the if clause, no brackets are
> required.
[]
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
[]
> @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_current(sl, ) == 0) {
> + if (w1_ds2438_get_current(sl, ) == 0)
> ret = snprintf(buf, count, "%i\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
to me this would look better using a style like the below:
(and it might be better using sysfs_emit and not snprintf too)
---
drivers/w1/slaves/w1_ds2438.c | 36
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
index 5cfb0ae23e91..9115c5a9bc4f 100644
--- a/drivers/w1/slaves/w1_ds2438.c
+++ b/drivers/w1/slaves/w1_ds2438.c
@@ -279,7 +279,6 @@ static ssize_t iad_read(struct file *filp, struct kobject
*kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
int16_t voltage;
if (off != 0)
@@ -287,12 +286,10 @@ static ssize_t iad_read(struct file *filp, struct kobject
*kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_current(sl, ) == 0) {
- ret = snprintf(buf, count, "%i\n", voltage);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_current(sl, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%i\n", voltage);
}
static ssize_t page0_read(struct file *filp, struct kobject *kobj,
@@ -330,7 +327,6 @@ static ssize_t temperature_read(struct file *filp, struct
kobject *kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
int16_t temp;
if (off != 0)
@@ -338,12 +334,10 @@ static ssize_t temperature_read(struct file *filp, struct
kobject *kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_temperature(sl, ) == 0) {
- ret = snprintf(buf, count, "%i\n", temp);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_temperature(sl, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%i\n", temp);
}
static ssize_t vad_read(struct file *filp, struct kobject *kobj,
@@ -351,7 +345,6 @@ static ssize_t vad_read(struct file *filp, struct kobject
*kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
uint16_t voltage;
if (off != 0)
@@ -359,12 +352,10 @@ static ssize_t vad_read(struct file *filp, struct kobject
*kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ) == 0) {
- ret = snprintf(buf, count, "%u\n", voltage);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%u\n", voltage);
}
static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
@@ -372,7 +363,6 @@ static ssize_t vdd_read(struct file *filp, struct kobject
*kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
uint16_t voltage;
if (off != 0)
@@ -380,12 +370,10 @@ static ssize_t vdd_read(struct file *filp, struct kobject
*kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ) == 0) {
- ret = snprintf(buf, count, "%u\n", voltage);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%u\n", voltage);
}
static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0);
Re: [PATCH][next] mlxsw: spectrum_router: remove redundant initialization of variable force
On Mon, 2021-03-29 at 09:04 +0300, Dan Carpenter wrote:
> On Sat, Mar 27, 2021 at 10:33:34PM +, Colin King wrote:
> > From: Colin Ian King
> >
> > The variable force is being initialized with a value that is
> > never read and it is being updated later with a new value. The
> > initialization is redundant and can be removed.
> >
> > Addresses-Coverity: ("Unused value")
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > index 6ccaa194733b..ff240e3c29f7 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct
> > mlxsw_sp *mlxsw_sp,
> > {
> > u16 bucket_index = info->nh_res_bucket->bucket_index;
> > struct netlink_ext_ack *extack = info->extack;
> > - bool force = info->nh_res_bucket->force;
> > + bool force;
> > char ratr_pl_new[MLXSW_REG_RATR_LEN];
> > char ratr_pl[MLXSW_REG_RATR_LEN];
> > u32 adj_index;
>
> Reverse Christmas tree.
Is still terrible style.
Re: [PATCH] staging: media: meson: vdec: matched alignment with parenthesis
On Fri, 2021-04-09 at 09:30 +0200, Hans Verkuil wrote:
> On 09/04/2021 00:19, Mitali Borkar wrote:
> > Matched alignment with open parenthesis to meet linux kernel coding
> > style.
> > Reported by checkpatch
> >
> > Signed-off-by: Mitali Borkar
> > ---
> > drivers/staging/media/meson/vdec/codec_mpeg12.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > b/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > index 48869cc3d973..21e93a13356c 100644
> > --- a/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > +++ b/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > @@ -81,7 +81,7 @@ static int codec_mpeg12_start(struct amvdec_session *sess)
> > }
> >
> >
> > ret = amvdec_set_canvases(sess, (u32[]){ AV_SCRATCH_0, 0 },
> > - (u32[]){ 8, 0 });
> > + (u32[]){ 8, 0 });
>
> The alignment here is because the 2nd and 3rd arguments belong together, so
> the alignment indicates that. In order to keep that I would add a newline
> after 'sess,' as well. Same as is done in meson/vdec/codec_h264.c.
Perhaps better as:
---
drivers/staging/media/meson/vdec/codec_mpeg12.c | 5 +++--
drivers/staging/media/meson/vdec/vdec_helpers.c | 2 +-
drivers/staging/media/meson/vdec/vdec_helpers.h | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/meson/vdec/codec_mpeg12.c
b/drivers/staging/media/meson/vdec/codec_mpeg12.c
index 48869cc3d973..933f1cd16ce1 100644
--- a/drivers/staging/media/meson/vdec/codec_mpeg12.c
+++ b/drivers/staging/media/meson/vdec/codec_mpeg12.c
@@ -65,6 +65,8 @@ static int codec_mpeg12_start(struct amvdec_session *sess)
struct amvdec_core *core = sess->core;
struct codec_mpeg12 *mpeg12;
int ret;
+ static const u32 canvas1[] = { AV_SCRATCH_0, 0 };
+ static const u32 canvas2[] = { 8, 0 };
mpeg12 = kzalloc(sizeof(*mpeg12), GFP_KERNEL);
if (!mpeg12)
@@ -80,8 +82,7 @@ static int codec_mpeg12_start(struct amvdec_session *sess)
goto free_mpeg12;
}
- ret = amvdec_set_canvases(sess, (u32[]){ AV_SCRATCH_0, 0 },
- (u32[]){ 8, 0 });
+ ret = amvdec_set_canvases(sess, canvas1, canvas2);
if (ret)
goto free_workspace;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c
b/drivers/staging/media/meson/vdec/vdec_helpers.c
index 7f07a9175815..df5c27266c44 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -177,7 +177,7 @@ static int set_canvas_nv12m(struct amvdec_session *sess,
}
int amvdec_set_canvases(struct amvdec_session *sess,
- u32 reg_base[], u32 reg_num[])
+ const u32 reg_base[], const u32 reg_num[])
{
struct v4l2_m2m_buffer *buf;
u32 pixfmt = sess->pixfmt_cap;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h
b/drivers/staging/media/meson/vdec/vdec_helpers.h
index cfaed52ab526..ace8897c34fe 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -17,7 +17,7 @@
* @reg_num: number of contiguous registers after each reg_base (including it)
*/
int amvdec_set_canvases(struct amvdec_session *sess,
- u32 reg_base[], u32 reg_num[]);
+ const u32 reg_base[], const u32 reg_num[]);
/* Helpers to read/write to the various IPs (DOS, PARSER) */
u32 amvdec_read_dos(struct amvdec_core *core, u32 reg);
Re: [PATCH v5 2/6] w1: ds2438: fixed if brackets coding style issue
On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote:
> Since there is only one statement inside the if clause, no brackets are
> required.
>
> Signed-off-by: Luiz Sampaio
> ---
> drivers/w1/slaves/w1_ds2438.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
> index 148921fb9702..56e53a748059 100644
> --- a/drivers/w1/slaves/w1_ds2438.c
> +++ b/drivers/w1/slaves/w1_ds2438.c
> @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_current(sl, ) == 0) {
> + if (w1_ds2438_get_current(sl, ) == 0)
> ret = snprintf(buf, count, "%i\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct
> kobject *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_temperature(sl, ) == 0) {
> + if (w1_ds2438_get_temperature(sl, ) == 0)
> ret = snprintf(buf, count, "%i\n", temp);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ) == 0) {
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ) == 0)
> ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ) == 0) {
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ) == 0)
> ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers
Hi Pedro,
On Tue, Apr 6, 2021 at 11:58 AM Pedro Tammela wrote:
>
> In 'bpf_ringbuf_reserve()' we require the flag to '0' at the moment.
>
> For 'bpf_ringbuf_{discard,submit,output}' a flag of '0' might send a
> notification to the process if needed.
>
> Signed-off-by: Pedro Tammela
> ---
> include/uapi/linux/bpf.h | 7 +++
> tools/include/uapi/linux/bpf.h | 7 +++
> 2 files changed, 14 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 49371eba98ba..8c5c7a893b87 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4061,12 +4061,15 @@ union bpf_attr {
> * of new data availability is sent.
> * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*,
> notification
> * of new data availability is sent unconditionally.
> + * If **0** is specified in *flags*, notification
> + * of new data availability is sent if needed.
Maybe a trivial question, but what does "if needed" mean? Does that
mean "when the buffer is full"?
Re: [RESEND PATCH v1] checkpatch: exclude four preprocessor sub-expressions from MACRO_ARG_REUSE
On Wed, 2021-04-07 at 19:50 +0900, Vincent Mailhol wrote:
> __must_be_array, offsetof, sizeof_field and __stringify are all
> preprocessor macros and do not evaluate their arguments. As such, it
> is safe not to warn when arguments are being reused in those four
> sub-expressions.
>
> Exclude those so that they can pass checkpatch.
>
> Signed-off-by: Vincent Mailhol
Acked-by: Joe Perches
> ---
> scripts/checkpatch.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index df8b23dc1eb0..25ee4fd5b118 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5736,7 +5736,7 @@ sub process {
> next if ($arg =~ /\.\.\./);
> next if ($arg =~ /^type$/i);
> my $tmp_stmt = $define_stmt;
> - $tmp_stmt =~
> s/\b(sizeof|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
> + $tmp_stmt =~
> s/\b(__must_be_array|offsetof|sizeof|sizeof_field|__stringify|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
> $tmp_stmt =~ s/\#+\s*$arg\b//g;
> $tmp_stmt =~ s/\b$arg\s*\#\#//g;
> my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop
On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote:
> Hi Colin,
>
> On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote:
> > From: Colin Ian King
> >
> > The while-loop iterates until src is non-null or i is 3, however, the
> > loop counter i is not intinitialied to zero, causing incorrect iteration
> > counts. Fix this by initializing it to zero.
> >
> > Addresses-Coverity: ("Uninitialized scalar variable")
> > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4
> > backend")
> > Signed-off-by: Colin Ian King
>
> Thank you very much for catching this! It looks good to me,
> Reviewed-by: Gao Xiang
>
> (btw, may I fold this into the original patchset? since such big pcluster
> patchset is just applied to for-next for further integration testing, and
> the commit id is not stable yet..)
>
> Thanks,
> Gao Xiang
I think this code is odd and would be more intelligible using
a for loop like:
---
fs/erofs/decompressor.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 27aa6a99b371..5a64f4649414 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct
z_erofs_decompress_req *rq,
}
ret = alg->prepare_destpages(rq, pagepool);
- if (ret < 0) {
+ if (ret < 0)
return ret;
- } else if (ret) {
+ if (ret) {
dst = page_address(*rq->out);
dst_maptype = 1;
goto dstmap_out;
}
- i = 0;
- while (1) {
+ for (i = 0; i < 3; i++) {
dst = vm_map_ram(rq->out, nrpages_out, -1);
-
+ if (dst) {
+ dst_maptype = 2;
+ goto dstmap_out;
+ }
/* retry two more times (totally 3 times) */
- if (dst || ++i >= 3)
- break;
vm_unmap_aliases();
}
-
- if (!dst)
- return -ENOMEM;
-
- dst_maptype = 2;
+ return -ENOMEM;
dstmap_out:
ret = alg->decompress(rq, dst + rq->pageofs_out);
Re: [PATCH] staging: greybus: Match parentheses alignment
On Tue, 2021-04-06 at 15:27 +0200, Greg KH wrote:
> On Tue, Apr 06, 2021 at 06:42:59PM +0600, Zhansaya Bagdauletkyzy wrote:
> > Match next line with open parentheses by adding tabs/spaces
> > to conform with Linux kernel coding style.
> > Reported by checkpatch.
[]
> > diff --git a/drivers/staging/greybus/camera.c
> > b/drivers/staging/greybus/camera.c
[]
> > @@ -378,8 +378,8 @@ struct ap_csi_config_request {
> > #define GB_CAMERA_CSI_CLK_FREQ_MARGIN 15000U
> >
> >
> > static int gb_camera_setup_data_connection(struct gb_camera *gcam,
> > - struct gb_camera_configure_streams_response *resp,
> > - struct gb_camera_csi_params *csi_params)
> > + struct
> > gb_camera_configure_streams_response *resp,
> > + struct gb_camera_csi_params
> > *csi_params)
>
> And now you violate another coding style requirement, which means
> someone will send another patch to fix that up and around and around it
> goes...
None of the coding style document is an actual requirement Greg.
It's all rules of thumb. Useful rules, but not hard and fast right?
To me, the biggest issue with this code isn't whether or not the
code is aligned at open parentheses or stays within 80 columns,
but is the use of 30+ character length identifiers.
Using identifiers of that length makes using 80 column, or even
100 column length lines infeasible.
Perhaps seeing if include/linux/greybus/greybus_protocols.h
could be updated to use shorter length identifiers might be useful.
The median length identifier there is ~25 chars long and the
maximum length identifier is ~50 chars.
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 19:28 +0200, Fabio Aiuto wrote: > On Sat, Apr 03, 2021 at 09:17:37AM -0700, Joe Perches wrote: > > On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > > > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > > > This patchset removes all RT_TRACE usages in core/ files. > > > > > > > > and hal and include and os_dep > > > > > > Hi, > > > > > > I was just about to send the second patchset relative to hal/ files. > > > The whole has been split up in directories in order to reduce the > > > number of patch per patchset > > > > > It's a good idea, but the patches relative to RT_TRACE removal > > > could be huge > > > > That's really not a significant issue. > > Simplicity in review is also important. > > Mechanization of patch creation can reduce review efforts. > > Maybe I wrongly associated simplicity with patch dimensions, but maybe > for patches this simple have expert reviewers some tool for > automatic review? Coccinelle is a relatively trusted tool and using it as a scripting mechanism where the script is shown as part of the commit message gives confidence that the change it produces can be applied without significant doubt. To improve confidence that any change that does not have an output object code delta, comparing the object code produced before and after the change is useful. Showing that the code has been both compiled and compared in the commit message also improves confidence that the change is useful and can be applied.
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > This patchset removes all RT_TRACE usages in core/ files. > > > > and hal and include and os_dep > > Hi, > > I was just about to send the second patchset relative to hal/ files. > The whole has been split up in directories in order to reduce the > number of patch per patchset > It's a good idea, but the patches relative to RT_TRACE removal > could be huge That's really not a significant issue. Simplicity in review is also important. Mechanization of patch creation can reduce review efforts. Few people are actively working on this particular codebase. As far as I can tell no logical defect is being corrected here. None of this is likely to be backported. Applying each individual patch has a 'cost' in maintainer time and review effort. Fewer patches create lower overall costs. Good luck...
Re: [PATCH v3 23/30] staging: rtl8723bs: fix comparison in if condition in core/rtw_recv.c
On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote:
> fix post-commit checkpatch issue:
>
> CHECK: Using comparison to false is error prone
> 27: FILE: drivers/staging/rtl8723bs/core/rtw_recv.c:381:
> + if (psecuritypriv->
> bcheck_grpkey == false &&
>
> Signed-off-by: Fabio Aiuto
> ---
> drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c
> b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index cd4324a93275..21949925ec77 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -378,7 +378,7 @@ static signed int recvframe_chkmic(struct adapter
> *adapter, union recv_frame *p
>
>
> } else {
> /* mic checked ok */
> - if (psecuritypriv->bcheck_grpkey == false &&
> + if (!psecuritypriv->bcheck_grpkey &&
> (IS_MCAST(prxattrib->ra) == true))
> psecuritypriv->bcheck_grpkey = true;
> }
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote:
> This patchset removes all RT_TRACE usages in core/ files.
and hal and include and os_dep
>
> This is the first of a series aimed at removing RT_TRACE macro.
>
> The whole private tracing system is not tied to a configuration
> symbol and the default behaviour is _trace nothing_. It's verbose
> and relies on a private log level tracing doomed to be
> removed.
It's nice, but individual patches per file done by hand are difficult
to review because you are interleaving removal patches with cleanup
patches.
I believe this should be a patch series with a single patch to remove
all RT_TRACE macro uses using coccinelle and then use separate patches
to do whatever cleanups around these removals you want to do.
All of these below should be done for all files in drivers/staging/rtl8723bs
at once instead of submitting per-file patches.
IMO something like:
Cover-letter: Explain why you are doing this
Patch 1 of N: Remove all RT_TRACE macro uses using a coccinelle script
and include the coccinelle script in the commit message
Patch 2 of N: Remove commented out RT_TRACE uses
Patch 3 of N: Remove RT_TRACE macro definition
Patch 4 of N: Cleanup coccinelle generated {} uses, if/else braces and
the now unnecessary if tests around the RT_TRACE removals
Patch 5 of N: Cleanup whitespace
Patcn x of N: Whatever else related to these RT_TRACE sites...
https://lore.kernel.org/lkml/c845d8ea7d0d8e7a613471edb53d780d660142a9.ca...@perches.com/
Using a sequence like the above would be much easier to review and
would be a significant shorter patch set.
Re: [PATCH 14/16] staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_wlan_util.c
On Fri, 2021-04-02 at 19:40 +0200, Fabio Aiuto wrote: > On Fri, Apr 02, 2021 at 08:20:17AM -0700, Joe Perches wrote: > > On Fri, 2021-04-02 at 14:51 +0200, Fabio Aiuto wrote: > > > On Fri, Apr 02, 2021 at 03:37:57AM -0700, Joe Perches wrote: > > > > On Fri, 2021-04-02 at 12:01 +0200, Fabio Aiuto wrote: > > > > > remove all RT_TRACE logs > > > > > > > > > > fix patch-related checkpatch issues [] > > > > Lastly, another suggestion would be to just submit a single patch > > > > removing _ALL_ the RT_TRACE uses not intermixing various other cleanups > > > > with the series and then do those other cleanups. > > > > > > > > Using a coccinelle script like: > > > > > > > > $ cat RT_TRACE.cocci > > > > @@ > > > > expression a, b, c; > > > > @@ > > > > > > > > - RT_TRACE(a, b, (c)); > > > > > > > > $ spatch -sp-file RT_TRACE.cocci drivers/staging/rtl8723bs/ > > > > > > > > And then clean up the various bits you think are inappropriately done. [] > > > thank you Joe, I tried with (RT_TRACE.cocci in parent folder) > > > > > > user@host:~/src/git/kernels/staging$ spatch -sp-file ../RT_TRACE.cocci > > > drivers/staging/rtl8723bs/ > > > init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h > > > 0 files match > > > > Likely you are running the script on the tree after you have > > applied all your patches. > > > > Try running the cocci script on a fresh copy of -next. > > > > Using the script and adding the script in the commit message helps > > others to verify that the changes you make do not have any other effect. > > > > $ cat RT_TRACE.cocci > > @@ > > expression a, b, c; > > @@ > > > > - RT_TRACE(a, b, (c)); > > > > $ git checkout next-20210401 > > $ spatch -sp-file RT_TRACE.cocci --in-place --no-show-diff --very-quiet > > drivers/staging/rtl8723bs/ > > 31 files match > > $ git diff --stat -p > > drivers/staging/rtl8723bs/core/rtw_cmd.c | 34 +-- [] > > 28 files changed, 19 insertions(+), 935 deletions(-) [] > thank you Joe, this mail is so precious ;) I'm not quite sure what you mean by that but you quoted nearly 200k of the previous email. Please remember to trim your replies.
Re: [PATCH 14/16] staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_wlan_util.c
On Fri, 2021-04-02 at 12:01 +0200, Fabio Aiuto wrote:
> remove all RT_TRACE logs
>
> fix patch-related checkpatch issues
>
> Signed-off-by: Fabio Aiuto
> ---
> .../staging/rtl8723bs/core/rtw_wlan_util.c| 26 +--
> 1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
[]
> @@ -1382,25 +1374,19 @@ int rtw_check_bcn_info(struct adapter *Adapter, u8
> *pframe, u32 packet_len)
> if (encryp_protocol == ENCRYP_PROTOCOL_WPA || encryp_protocol ==
> ENCRYP_PROTOCOL_WPA2) {
> pbuf = rtw_get_wpa_ie(>IEs[12], _ielen,
> bssid->IELength-12);
> if (pbuf && (wpa_ielen > 0)) {
> - if (_SUCCESS == rtw_parse_wpa_ie(pbuf, wpa_ielen+2,
> _cipher, _cipher, _8021x)) {
> - RT_TRACE(_module_rtl871x_mlme_c_, _drv_info_,
> - ("%s pnetwork->pairwise_cipher:
> %d, group_cipher is %d, is_8021x is %d\n", __func__,
> - pairwise_cipher, group_cipher,
> is_8021x));
> - }
> + if (rtw_parse_wpa_ie(pbuf, wpa_ielen + 2, _cipher,
> + _cipher, _8021x) ==
> _SUCCESS)
> + ;
This sort of if is a bit silly.
Better would be to remove the test and just use:
rtw_parse_wpa_ie(pbuf, wpa_ielen + 2, _cipher,
_cipher, _8021x);
> } else {
> pbuf = rtw_get_wpa2_ie(>IEs[12], _ielen,
> bssid->IELength-12);
>
>
> if (pbuf && (wpa_ielen > 0)) {
> - if (_SUCCESS == rtw_parse_wpa2_ie(pbuf,
> wpa_ielen+2, _cipher, _cipher, _8021x)) {
> - RT_TRACE(_module_rtl871x_mlme_c_,
> _drv_info_,
> - ("%s
> pnetwork->pairwise_cipher: %d, pnetwork->group_cipher is %d, is_802x is %d\n",
> - __func__,
> pairwise_cipher, group_cipher, is_8021x));
> - }
> + if (rtw_parse_wpa2_ie(pbuf, wpa_ielen + 2,
> _cipher,
> + _cipher,
> _8021x) == _SUCCESS)
> + ;
rtw_parse_wpa2_ie(pbuf, wpa_ielen + 2,
_cipher,
_cipher, _8021x);
etc...
Lastly, another suggestion would be to just submit a single patch
removing _ALL_ the RT_TRACE uses not intermixing various other cleanups
with the series and then do those other cleanups.
Using a coccinelle script like:
$ cat RT_TRACE.cocci
@@
expression a, b, c;
@@
- RT_TRACE(a, b, (c));
$ spatch -sp-file RT_TRACE.cocci drivers/staging/rtl8723bs/
And then clean up the various bits you think are inappropriately done.
[PATCH v7 1/2] Added AMS tsl2591 driver implementation
Driver implementation for AMS/TAOS tsl2591 ambient light sensor. This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds. This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal. Datasheet: https://ams.com/tsl25911#tab/documents Signed-off-by: Joe Sandom --- Changes in v7; - Revert back to using plain numbers in register defines instead of BIT and GENMASK - Changed from pre-increment of 'i' in for loops to post-increment - Use iopoll.h - readx_poll_timeout macro for periodic poll on ALS status flag valid - Remove the 0xFF masks on u8 types as they are redundant drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile |1 + drivers/iio/light/tsl2591.c | 1224 +++ 3 files changed, 1236 insertions(+) create mode 100644 drivers/iio/light/tsl2591.c diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 33ad4dd0b5c7..6a69a9a3577a 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -501,6 +501,17 @@ config TSL2583 Provides support for the TAOS tsl2580, tsl2581 and tsl2583 devices. Access ALS data via iio, sysfs. +config TSL2591 +tristate "TAOS TSL2591 ambient light sensor" +depends on I2C +help + Select Y here for support of the AMS/TAOS TSL2591 ambient light sensor, + featuring channels for combined visible + IR intensity and lux illuminance. + Access data via iio and sysfs. Supports iio_events. + + To compile this driver as a module, select M: the + module will be called tsl2591. + config TSL2772 tristate "TAOS TSL/TMD2x71 and TSL/TMD2x72 Family of light and proximity sensors" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index ea376deaca54..d10912faf964 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o obj-$(CONFIG_TCS3414) += tcs3414.o obj-$(CONFIG_TCS3472) += tcs3472.o obj-$(CONFIG_TSL2583) += tsl2583.o +obj-$(CONFIG_TSL2591) += tsl2591.o obj-$(CONFIG_TSL2772) += tsl2772.o obj-$(CONFIG_TSL4531) += tsl4531.o obj-$(CONFIG_US5182D) += us5182d.o diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c new file mode 100644 index ..14a405a96e3a --- /dev/null +++ b/drivers/iio/light/tsl2591.c @@ -0,0 +1,1224 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Joe Sandom + * + * Datasheet: https://ams.com/tsl25911#tab/documents + * + * Device driver for the TAOS TSL2591. This is a very-high sensitivity + * light-to-digital converter that transforms light intensity into a digital + * signal. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +/* ADC integration time, field value to time in ms*/ +#define TSL2591_FVAL_TO_ATIME(x) (((x) + 1) * 100) +/* ADC integration time, time in ms to field value */ +#define TSL2591_ATIME_TO_FVAL(x) (((x) / 100) - 1) + +/* TSL2591 register set */ +#define TSL2591_ENABLE 0x00 +#define TSL2591_CONTROL 0x01 +#define TSL2591_AILTL 0x04 +#define TSL2591_AILTH 0x05 +#define TSL2591_AIHTL 0x06 +#define TSL2591_AIHTH 0x07 +#define TSL2591_NP_AILTL0x08 +#define TSL2591_NP_AILTH0x09 +#define TSL2591_NP_AIHTL0x0A +#define TSL2591_NP_AIHTH0x0B +#define TSL2591_PERSIST 0x0C +#define TSL2591_PACKAGE_ID 0x11 +#define TSL2591_DEVICE_ID 0x12 +#define TSL2591_STATUS 0x13 +#define TSL2591_C0_DATAL0x14 +#define TSL2591_C0_DATAH0x15 +#define TSL2591_C1_DATAL0x16 +#define TSL2591_C1_DATAH0x17 + +/* TSL2591 command register definitions */ +#define TSL2591_CMD_NOP 0xA0 +#define TSL2591_CMD_SF_INTSET 0xE4 +#define TSL2591_CMD_SF_CALS_I 0xE5 +#define TSL2591_CMD_SF_CALS_NPI 0xE7 +#define TSL2591_CMD_SF_CNP_ALSI 0xEA + +/* TSL2591 enable register definitions */ +#define TSL2591_PWR_ON 0x01 +#define TSL2591_PWR_OFF 0x00 +#define TSL2591_ENABLE_ALS 0x02 +#define TSL2591_ENABLE_ALS_INT 0x10 +#define TSL2591_ENABLE_SLEEP_INT0x40 +#define TSL2591_ENABLE_NP_INT 0x80 + +/* TSL2591 control register definitions */ +#define TSL2591_CTRL_ALS_INTEGRATION_100MS 0x00 +#define TSL2591_CTRL_ALS_INTEGRATION_200MS 0x01 +#define TSL2591_CTRL_ALS_INTEGRATION_300MS 0x02 +#define TSL2591_CTRL_ALS_INTEGRATION_400MS 0x03 +#define TSL2591_CTRL_ALS_INTEGRATION_500MS 0x04 +#define TSL2591_CTRL_ALS_INTEGRATION_600MS 0x05 +#define TSL2591_CTRL_ALS_LOW_GAIN
[PATCH v7 2/2] Added AMS tsl2591 device tree binding
Device tree binding for AMS/TAOS tsl2591 ambient light sensor.
This driver supports configuration via device tree and sysfs.
Supported channels for raw infrared light intensity,
raw combined light intensity and illuminance in lux.
The driver additionally supports iio events on lower and
upper thresholds.
This is a very-high sensitivity light-to-digital converter that
transforms light intensity into a digital signal.
Signed-off-by: Joe Sandom
Reviewed-by: Rob Herring
---
Changes in v7:
- No changes
Notes:
- Re-submitted to align the version with part 1 of the patch series
.../bindings/iio/light/amstaos,tsl2591.yaml | 50 +++
1 file changed, 50 insertions(+)
create mode 100644
Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
new file mode 100644
index ..596a3bc770f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/amstaos,tsl2591.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS)
+
+maintainers:
+ - Joe Sandom
+
+description: |
+ AMS/TAOS TSL2591 is a very-high sensitivity
+ light-to-digital converter that transforms light intensity into a digital
+ signal.
+
+properties:
+ compatible:
+const: amstaos,tsl2591
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+description:
+ Interrupt (INT:Pin 2) Active low. Should be set to IRQ_TYPE_EDGE_FALLING.
+ interrupt is used to detect if the light intensity has fallen below
+ or reached above the configured threshold values.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+tsl2591@29 {
+compatible = "amstaos,tsl2591";
+reg = <0x29>;
+interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
+ };
+};
+...
--
2.17.1
Re: [PATCH v6 1/2] Added AMS tsl2591 driver implementation
On Fri, Mar 26, 2021 at 01:01:57PM +0200, Andy Shevchenko wrote: > On Fri, Mar 26, 2021 at 12:05 AM Joe Sandom wrote: > > > > Driver implementation for AMS/TAOS tsl2591 ambient light sensor. > > > > This driver supports configuration via device tree and sysfs. > > Supported channels for raw infrared light intensity, > > raw combined light intensity and illuminance in lux. > > The driver additionally supports iio events on lower and > > upper thresholds. > > > > This is a very-high sensitivity light-to-digital converter that > > transforms light intensity into a digital signal. > > I'm under the impression that you ignored at least half of my comments The majority of your comments were applied in V5 as far as I can see. Some of them I recognised as optional at the time. I had another sweep through and have seen value in enforcing a few of the other points you mentioned. I've added them to V7 and will release shortly. Thanks for the feedback Andy. > [1]. Have you seen them? > > [1]: > https://lore.kernel.org/linux-iio/cahp75vcsw2xxdh--rxan7xt0ju+qfw9c_va0ggrgpgpbua0...@mail.gmail.com/ > > Please. address and come again. > NAK for this version, sorry. > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH 49/49] staging: rtl8723bs: remove obsolete macro
On Thu, 2021-04-01 at 11:21 +0200, Fabio Aiuto wrote:
> remove obsolete macro RT_TRACE
[]
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
> b/drivers/staging/rtl8723bs/include/rtw_debug.h
[]
> -#ifdef DEBUG_RTL871X
> -
> -#if defined(_dbgdump) && defined(_MODULE_DEFINE_)
> -
> - #undef RT_TRACE
> - #define RT_TRACE(_Comp, _Level, Fmt)\
> - do {\
> - if ((_Comp & GlobalDebugComponents) && (_Level <=
> GlobalDebugLevel)) {\
> - _dbgdump("%s [0x%08x,%d]", DRIVER_PREFIX, (unsigned
> int)_Comp, _Level);\
> - _dbgdump Fmt;\
> - } \
> - } while (0)
> -
> -#endif /* defined(_dbgdump) && defined(_MODULE_DEFINE_) */
> -#endif /* DEBUG_RTL871X */
Maybe remove all of these too
$ git grep DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_debug.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_debug.c:#endif /* DEBUG_RTL871X */
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/include/autoconf.h:/*#define DEBUG_RTL871X */
drivers/staging/rtl8723bs/include/rtw_debug.h:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/include/rtw_debug.h:#endif /* DEBUG_RTL871X */
Re: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board
On Tue, 2021-03-30 at 16:23 +0800, Moriis Ku wrote:
> From: Morris
>
> Signed-off-by: Morris
> ---
> spi_pack.c | 1506
You might try to use scripts/checkpatch.pl on this and see
if there is anything you want to change to have the code look
more like the rest of the kernel.
Using
./scripts/checkpatch.pl --strict --fix
from the top of the kernel tree should help quite a bit with
making the code layout look more like typical kernel style.
> diff --git a/spi_pack.c b/spi_pack.c
[]
> +static void get_info(struct sunix_sdc_spi_channel *spi_chl, unsigned int
> incomeLength, unsigned int * translateLength)
> +{
[]
> + do
> + {
> + Address = spi_chl->info.phy2_base_start +
> spi_chl->info.memoffset;
> +
> + } while (false);
That's an odd and unnecessary use of a do while.
> + memset(TrBuff, 0, SUNIX_SDC_SPI_BUFF);
> + TrLength = 0;
> +
> + pTrHeader->Version = 0x01;
> + pTrHeader->CmdResponseEventData = pRxHeader->CmdResponseEventData |
> 0x8000;
> + pTrHeader->ResponseStatus = nStatus;
> + pTrHeader->Length = (nStatus == SDCSPI_STATUS_SUCCESS)?(31 +
> (cib_info->spi_number_of_device * 12)):0;
> + TrLength = sizeof(SPI_HEADER);
Perhaps reorder the patch submission to define the structs first.
This can't compile as SPI_HEADER isn't defined
SPI_HEADER and PSPI_HEADER should use not use typedefs and use the typical
struct spi_header
and
struct spi_header *
> + if (pTrHeader->ResponseStatus == SDCSPI_STATUS_SUCCESS)
Hungarian isn't generally used in the kernel.
CamelCase isn't generally used either.
> + {
> + memcpy([TrLength], spi_chl->info.model_name, 16);
> + TrLength += 16;
> + TrBuff[TrLength++] = spi_chl->info.bus_number;
> + TrBuff[TrLength++] = spi_chl->info.dev_number;
> + TrBuff[TrLength++] = spi_chl->info.line;
> + TrBuff[TrLength++] = (unsigned char)((Address & 0xff00) >>
> 24);
> + TrBuff[TrLength++] = (unsigned char)((Address & 0x00ff) >>
> 16);
> + TrBuff[TrLength++] = (unsigned char)((Address & 0xff00) >>
> 8);
> + TrBuff[TrLength++] = (unsigned char)((Address & 0x00ff));
> + TrBuff[TrLength++] = (unsigned char)(spi_chl->info.irq);
You might consider using a single char * and increment that and not use
TrLength at all and use p - TrBuff when necessary.
*p++ = spi_chl->info.bus_number;
*p++ = spi_chl->info.dev_number;
...
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0xff00) >> 24);
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0x00ff) >> 16);
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0xff00) >> 8);
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0x00ff));
Perhaps
put_unaligned_le32(cib_info->spi_significant_of_clock, p);
p += 4;
etc...
[PATCH] checkpatch: Warn when missing newline in return sysfs_emit() formats
return sysfs_emit() uses should include a newline.
Suggest adding a newline when one is missing.
Add one using --fix too.
Signed-off-by: Joe Perches
---
scripts/checkpatch.pl | 11 +++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d67146d0b33c..4dbda85fd7e5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7198,6 +7198,17 @@ sub process {
"Using $1 should generally have parentheses around
the comparison\n" . $herecurr);
}
+# return sysfs_emit(foo, fmt, ...) fmt without newline
+ if ($line =~
/\breturn\s+sysfs_emit\s*\(\s*$FuncArg\s*,\s*($String)/ &&
+ substr($rawline, $-[6], $+[6] - $-[6]) !~ /\\n"$/) {
+ my $offset = $+[6] - 1;
+ if (WARN("SYSFS_EMIT",
+"return sysfs_emit(...) formats should include
a terminating newline\n" . $herecurr) &&
+ $fix) {
+ substr($fixed[$fixlinenr], $offset, 0) = '\\n';
+ }
+ }
+
# nested likely/unlikely calls
if ($line =~
/\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
WARN("LIKELY_MISUSE",
Re: [PATCH] mtd: intel-spi: add is_protected and is_bios_locked knobs
On Tue, 2021-03-30 at 18:54 +0300, Tomas Winkler wrote:
> From: Tamar Mashiah
[]
> the region protection status is exposed via sysfs file
> as the manufacturing will need the both files in order to validate
> that the device is properly sealed.
[]
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c
> b/drivers/mtd/spi-nor/controllers/intel-spi.c
[]
> +static ssize_t intel_spi_is_protected_show(struct device *dev,
> +struct device_attribute *attr, char
> *buf)
> +{
> + struct intel_spi *ispi = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d", ispi->is_protected);
These should also include a newline in the format. i.e.:
return sysfs_emit(buf, "%d\n", ispi->is_protected);
> +static ssize_t intel_spi_bios_lock_show(struct device *dev,
> + struct device_attribute *attr, char
> *buf)
> +{
> + struct intel_spi *ispi = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d", ispi->is_bios_locked);
etc...
Re: [PATCH v2 3/5] crypto: hisilicon/sgl - add some dfx logs
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
> Add some dfx logs in some abnormal exit situations.
[]
> diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
> @@ -87,8 +87,10 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
> device *dev,
> block[i].sgl = dma_alloc_coherent(dev, block_size,
> [i].sgl_dma,
> GFP_KERNEL);
> - if (!block[i].sgl)
> + if (!block[i].sgl) {
> + dev_err(dev, "Fail to allocate hw SG buffer!\n");
This doesn't seem useful as dma_alloc_coherent does a dump_stack
by default on OOM.
Re: [PATCH v2 1/5] crypto: hisilicon/sgl - fixup coding style
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
> use a macro replace of a magic number.
Given the use of 32 in the same test, this
seems more obfuscating that useful to me.
> diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
> @@ -9,6 +9,7 @@
> #define HISI_ACC_SGL_NR_MAX 256
> #define HISI_ACC_SGL_ALIGN_SIZE 64
> #define HISI_ACC_MEM_BLOCK_NR5
> +#define HISI_ACC_BLOCK_SIZE_MAX_SHIFT31
>
>
> struct acc_hw_sge {
> dma_addr_t buf;
> @@ -67,7 +68,8 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
> device *dev,
> sgl_size = sizeof(struct acc_hw_sge) * sge_nr +
> sizeof(struct hisi_acc_hw_sgl);
> block_size = 1 << (PAGE_SHIFT + MAX_ORDER <= 32 ?
> -PAGE_SHIFT + MAX_ORDER - 1 : 31);
> +PAGE_SHIFT + MAX_ORDER - 1 :
> +HISI_ACC_BLOCK_SIZE_MAX_SHIFT);
> sgl_num_per_block = block_size / sgl_size;
> block_num = count / sgl_num_per_block;
> remain_sgl = count % sgl_num_per_block;
Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config
On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote:
> On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen
> wrote:
> >
> > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP.
> > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> >
> > Make the gpiolib allow drivers to return both so driver developers
> > can avoid one of the checkpatch complaints.
>
> Internally we are fine to use the ENOTSUPP.
> Checkpatch false positives there.
>
> I doubt we need this change. Rather checkpatch should rephrase this to
> point out that this is only applicable to _user-visible_ error path.
> Cc'ed Joe.
Adding CC for Jakub Kicinski who added that particular rule/test.
And the output message report of the rule is merely a suggestion indicating
a preference. It's always up to an individual to accept/reject.
At best, perhaps wordsmithing the checkpatch message might be an OK option.
+# ENOTSUPP is not a standard error code and should be avoided in new patches.
+# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
+# Similarly to ENOSYS warning a small number of false positives is expected.
+ if (!$file && $line =~ /\bENOTSUPP\b/) {
+ if (WARN("ENOTSUPP",
+"ENOTSUPP is not a SUSV4 error code, prefer
EOPNOTSUPP\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~
s/\bENOTSUPP\b/EOPNOTSUPP/;
+ }
+ }
+
Re: [PATCH] sched,psi: fix typo in comment
On Mon, 2021-03-29 at 09:35 +0200, Peter Zijlstra wrote: > On Sat, Mar 27, 2021 at 08:46:10PM +0800, Xie XiuQi wrote: > > s/exceution/execution/ > > s/possibe/possible/ > > s/manupulations/manipulations/ > > > > Signed-off-by: Xie XiuQi > > Xie, if you'd have bothered to check the development tree of the code > you're patching, you'd have found it's long since fixed. March 18, 2021 doesn't seem a long time ago to me. commit 3b03706fa621ce31a3e9ef6307020fde4e6aae16 Author: Ingo Molnar Date: Thu Mar 18 13:38:50 2021 +0100 sched: Fix various typos > I'm getting sick and tired of people wasting bandwidth with this > nonsense. This seems more like a large overreaction IMO.
Re: [PATCH v2] livepatch: Replace the fake signal sending with TIF_NOTIFY_SIGNAL infrastructure
On 3/29/21 9:28 AM, Miroslav Benes wrote:
Livepatch sends a fake signal to all remaining blocking tasks of a
running transition after a set period of time. It uses TIF_SIGPENDING
flag for the purpose. Commit 12db8b690010 ("entry: Add support for
TIF_NOTIFY_SIGNAL") added a generic infrastructure to achieve the same.
Replace our bespoke solution with the generic one.
Reviewed-by: Jens Axboe
Reviewed-by: Petr Mladek
Signed-off-by: Miroslav Benes
---
v2:
- #include from kernel/signal.c removed [Petr]
kernel/livepatch/transition.c | 5 ++---
kernel/signal.c | 4 +---
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..3a4beb9395c4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include "core.h"
#include "patch.h"
#include "transition.h"
@@ -369,9 +370,7 @@ static void klp_send_signals(void)
* Send fake signal to all non-kthread tasks which are
* still not migrated.
*/
- spin_lock_irq(>sighand->siglock);
- signal_wake_up(task, 0);
- spin_unlock_irq(>sighand->siglock);
+ set_notify_signal(task);
}
}
read_unlock(_lock);
diff --git a/kernel/signal.c b/kernel/signal.c
index f2a1b898da29..604290a8ca89 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -43,7 +43,6 @@
#include
#include
#include
-#include
#include
#include
@@ -181,8 +180,7 @@ void recalc_sigpending_and_wake(struct task_struct *t)
void recalc_sigpending(void)
{
- if (!recalc_sigpending_tsk(current) && !freezing(current) &&
- !klp_patch_pending(current))
+ if (!recalc_sigpending_tsk(current) && !freezing(current))
clear_thread_flag(TIF_SIGPENDING);
}
Looks good to me. Thanks for checking on this and updating.
Acked-by: Joe Lawrence
-- Joe
Re: [PATCH] kconfig: nconf: stop endless search-up loops
On Sun, 2021-03-28 at 11:27 +0200, Mihai Moldovan wrote:
> * On 3/27/21 11:26 PM, Randy Dunlap wrote:
> > There is a test for it in checkpatch.pl but I also used checkpatch.pl
> > without it complaining, so I don't know what it takes to make the script
> > complain.
> >
> > if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> > $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
> > WARN("CONSTANT_COMPARISON",
> > "Comparisons should place the constant on the
> > right side of the test\n" . $herecurr) &&
>
> There are multiple issues, err, "challenges" there:
> - literal "Constant" instead of "$Constant"
> - the left part is misinterpreted as an operation due to the minus sign
> (arithmetic operator)
> - $Constant is defined as "qr{$Float|$Binary|$Octal|$Hex|$Int}" (which is
> okay), but all these types do not include their negative range.
>
> As far as I can tell, the latter is intentional. Making these types compatible
> with negative values causes a lot of other places to break, so I'm not keen on
> changing this.
>
> The minus sign being misinterpreted as an operator is highly difficult to fix
> in a sane manner. The original intention was to avoid misinterpreting
> expressions like (var - CONSTANT real_op ...) as a constant-on-left expression
> (and, more importantly, to not misfix it when --fix is given).
>
> The general idea is sane and we probably shouldn't change that, but it would
> be good to handle negative values as well.
>
> At first, I was thinking of overriding this detection by checking if the
> leading part matches "(-\s*$", which should only be true for negative values,
> assuming that there is always an opening parenthesis as part of a conditional
> statement/loop (if, while). After playing around with this and composing this
> message for a few hours, it dawned on me that there can easily be free-
> standing forms (for instance as part of for loops or assignment lines), so
> that wouldn't cut it.
>
> It really goes downhill from here.
>
> I assume that false negatives are nuisances due to stylistic errors in the
> code, but false positives actually harmful since a lot of them make code
> review by maintainers very tedious.
>
> So far, minus signs were always part of the leading capture group. I'd
> actually like to have them in the constant capture group instead:
>
> - $line =~
> /^\+(.*)\b($Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) {
> + $line =~
> /^\+(.*)(-?\s*$Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) {
>
> With that sorted, the next best thing I could come up with to weed out
> preceding variables was this (which shouldn't influence non-negative
> constants):
>
> - if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> + if ($lead !~ /(?:$Operators|\.|[a-z])\s*$/ &&
>
>
> There still are a lot of expressions that won't match this, like
> "-1 + 0 == var" (i.e., "CONSTANT CONSTANT ...") or
> constellations like a simple "(CONSTANT) ..." (e.g.,
> "(1) == var").
>
> This is all fuzzy and getting this right would involve moving away from
> trying to make sense of C code with regular expressions in Perl, but actually
> parsing it to extract the semantics. Not exactly something I'd like to do...
>
> Thoughts on my workaround for this issue?
Making $Constant not include negative values was very intentional.
> Did I miss anything crucial or introduce a new bug inadvertently?
Not as far as I can tell from a trivial read.
My best guess as to what is best or necessary to do is nothing.
checkpatch isn't a real parser and never will be.
It can miss stuff. It's imperfect. It doesn't bother me.
Re: [PATCH] kconfig: nconf: stop endless search-up loops
On Sat, 2021-03-27 at 15:26 -0700, Randy Dunlap wrote:
> On 3/27/21 3:12 PM, Mihai Moldovan wrote:
> > * On 3/27/21 4:58 PM, Randy Dunlap wrote:
> > > On 3/27/21 5:01 AM, Mihai Moldovan wrote:
> > > > + if ((-1 == index) && (index == match_start))
> > >
> > > checkpatch doesn't complain about this (and I wonder how it's missed), but
> > > kernel style is (mostly) "constant goes on right hand side of comparison",
> > > so
> > > if ((index == -1) &&
> >
> > I can naturally send a V2 with that swapped.
> >
> > To my rationale: I made sure to use checkpatch, saw that it was accepted and
> > even went for a quick git grep -- '-1 ==', which likewise returned enough
> > results for me to call this consistent with the current code style.
> >
> > Maybe those matches were just frowned-upon, but forgotten-to-be-critized
> > examples of this pattern being used.
>
> There is a test for it in checkpatch.pl but I also used checkpatch.pl
> without it complaining, so I don't know what it takes to make the script
> complain.
>
> if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
> WARN("CONSTANT_COMPARISON",
>"Comparisons should place the constant on the
> right side of the test\n" . $herecurr) &&
Negative values aren't parsed well by the silly script as checkpatch
isn't a real parser.
Basically, checkpatch only recognizes positive ints as constants.
So it doesn't recognize uses like:
a * -5 > b
It doesn't parse -5 as a negative constant.
Though here it does seem the line with
$to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
should be
$to !~ /^(?:$Constant|[A-Z_][A-Z0-9_]*)$/ &&
You are welcome to try to improve checkpatch, but it seems non-trivial
and a relatively uncommon use in the kernel, so I won't.
Most all of the existing uses seem to be in drivers/scsi/pm8001/pm8001_hwi.c
$ git grep -P 'if\s*\(\s*\-\d+\s*(?:<=|>=|==|<|>)' -- '*.[ch]'
drivers/media/i2c/msp3400-driver.c: if (-1 == scarts[out][in + 1])
drivers/media/pci/bt8xx/bttv-driver.c: if (-1 == formats[i].fourcc)
drivers/media/pci/saa7134/saa7134-tvaudio.c:if (-1 == secondary)
drivers/media/pci/saa7146/mxb.c:if (-1 ==
mxb_saa7740_init[i].length)
drivers/media/usb/s2255/s2255drv.c: if (-1 == formats[i].fourcc)
drivers/net/ieee802154/mrf24j40.c: } else if (-1000 >= mbm && mbm > -2000)
{
drivers/net/ieee802154/mrf24j40.c: } else if (-2000 >= mbm && mbm > -3000)
{
drivers/net/ieee802154/mrf24j40.c: } else if (-3000 >= mbm && mbm > -4000)
{
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c: if (-1 ==
*gain_index) {
drivers/parisc/eisa_enumerator.c: if (-1==init_slot(i+1, es)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, GSM_SM_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, GSM_SM_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, RB6_ACCESS_REG)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
MBIC_AAP1_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
MBIC_IOP_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GSM_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GPIO_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
SPC_TOP_LEVEL_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GSM_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
SPC_TOP_LEVEL_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm80xx_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
0))
drivers/scsi/pm8001/pm8001_sas.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm80xx_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/pm8001/pm80xx_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/scsi_debug.c: if (-1 == res)
drivers/scsi/scsi_debug.c: if (-1 == ret) {
Re: [PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h
On Sat, 2021-03-27 at 15:24 +0100, Fabio Aiuto wrote:
> fix the following checkpatch warning:
>
> ERROR: Macros starting with if should be enclosed by a
> do - while loop to avoid possible if/else logic defects
> + #define RT_PRINT_DATA(_Comp, _Level,
> _TitleString, _HexData, _HexDataLen)\
>
> Signed-off-by: Fabio Aiuto
It's good to use checkpatch as a guide to improve code, but this
particular code is just a mess to begin with and it makes a
complete mess of the the dmesg log if it's actually enabled.
Try substituting print_hex_dump_debug for this instead.
> ---
> drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
> b/drivers/staging/rtl8723bs/include/rtw_debug.h
> index d1c557818305..b00f8a6c4312 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> @@ -236,19 +236,21 @@
> #if defined(_dbgdump)
> #undef RT_PRINT_DATA
> #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData,
> _HexDataLen) \
> - if (((_Comp) & GlobalDebugComponents) && (_Level <=
> GlobalDebugLevel)) \
> - {
> \
> - int __i;
> \
> - u8 *ptr = (u8 *)_HexData;
> \
> - _dbgdump("%s", DRIVER_PREFIX);
> \
> - _dbgdump(_TitleString);
> \
> - for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> - {
> \
> - _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4)
> == 0)?" ":" "); \
> - if (((__i + 1) % 16) == 0) _dbgdump("\n");
> \
> - }
> \
> - _dbgdump("\n");
> \
> - }
> + do { \
> + if (((_Comp) & GlobalDebugComponents) && (_Level <=
> GlobalDebugLevel)) \
> + {
> \
> + int __i;
> \
> + u8 *ptr = (u8 *)_HexData;
> \
> + _dbgdump("%s", DRIVER_PREFIX);
> \
> + _dbgdump(_TitleString);
> \
> + for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> + {
> \
> + _dbgdump("%02X%s", ptr[__i], (((__i +
> 1) % 4) == 0)?" ":" "); \
> + if (((__i + 1) % 16) == 0)
> _dbgdump("\n"); \
> + }
> \
> + _dbgdump("\n");
> \
> + } \
> + } while (0)
> #endif /* defined(_dbgdump) */
> #endif /* DEBUG_RTL871X */
>
>
Re: [PATCH v2 06/15] ACPI: LPSS: fix some coding style issues
On Sat, 2021-03-27 at 10:19 +0200, Andy Shevchenko wrote: > On Saturday, March 27, 2021, Xiaofei Tan wrote: > > > Fix some coding style issues reported by checkpatch.pl, including > > following types: > > > > WARNING: simple_strtol is obsolete, use kstrtol instead > > > And one more thing, the above message is bogus. Read what the comments in > the code says about use cases for simple_*() vs. kstrto*() ones. > > Joe? This check and message is nearly 10 years old and was appropriate for when it was implemented. kernel.h currently has: * Use these functions if and only if you cannot use kstrto, because So the message could be changed to something like: WARNING: simple_strtol should be used only when kstrtol can not be used.
Re: [PATCH 2/2] streamline_config.pl: Add softtabstop=4 for vim users
On Thu, 2021-03-25 at 09:50 -0400, Steven Rostedt wrote: > On Thu, 25 Mar 2021 15:20:13 +0900 Masahiro Yamada > wrote: > > > > The root cause of inconsistency is that > > you mix up space-indentation and tab-indentation. > > I do not know if it is a standard way either. > > This is the default way emacs has edited perl files for as long as I can > remember (back to 1996). It became my standard of editing perl files just > because of that. For everything else, I use tabs. [] > > For example, scripts/checkpatch.pl uses only tabs, > > which I think is more robust. > > Probably because Joe probably uses vim ;-) I generally use emacs. Maybe Andy Whitcroft uses vim. For checkpatch.pl I just followed Andy's style. get_maintainer.pl uses the 4 spaces then 1 tab style like Steven uses. perl code can be pretty long left to right so using smaller indentation seems useful there.
[RESEND][PATCH v6 2/2] Added AMS tsl2591 device tree binding
Device tree binding for AMS/TAOS tsl2591 ambient light sensor.
This driver supports configuration via device tree and sysfs.
Supported channels for raw infrared light intensity,
raw combined light intensity and illuminance in lux.
The driver additionally supports iio events on lower and
upper thresholds.
This is a very-high sensitivity light-to-digital converter that
transforms light intensity into a digital signal.
Datasheet: https://ams.com/tsl25911#tab/documents
Signed-off-by: Joe Sandom
Reviewed-by: Rob Herring
---
Changes in v6:
- No changes
Notes:
- Re-submitted to align the version with part 1 of the patch series
Reason for resend:
- Correctly pointed out that I forgot to add reviewed-by tag offered by Rob
Herring
.../bindings/iio/light/amstaos,tsl2591.yaml | 50 +++
1 file changed, 50 insertions(+)
create mode 100644
Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
new file mode 100644
index ..596a3bc770f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/amstaos,tsl2591.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS)
+
+maintainers:
+ - Joe Sandom
+
+description: |
+ AMS/TAOS TSL2591 is a very-high sensitivity
+ light-to-digital converter that transforms light intensity into a digital
+ signal.
+
+properties:
+ compatible:
+const: amstaos,tsl2591
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+description:
+ Interrupt (INT:Pin 2) Active low. Should be set to IRQ_TYPE_EDGE_FALLING.
+ interrupt is used to detect if the light intensity has fallen below
+ or reached above the configured threshold values.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+tsl2591@29 {
+compatible = "amstaos,tsl2591";
+reg = <0x29>;
+interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
+ };
+};
+...
--
2.17.1
Re: [PATCH v6 2/2] Added AMS tsl2591 device tree binding
On Thu, Mar 25, 2021 at 05:43:43PM -0600, Rob Herring wrote: > On Thu, 25 Mar 2021 22:05:04 +0000, Joe Sandom wrote: > > Device tree binding for AMS/TAOS tsl2591 ambient light sensor. > > > > This driver supports configuration via device tree and sysfs. > > Supported channels for raw infrared light intensity, > > raw combined light intensity and illuminance in lux. > > The driver additionally supports iio events on lower and > > upper thresholds. > > > > This is a very-high sensitivity light-to-digital converter that > > transforms light intensity into a digital signal. > > > > Datasheet: https://ams.com/tsl25911#tab/documents > > > > Signed-off-by: Joe Sandom > > --- > > Changes in v6: > > - No changes > > > > Notes: > > - Re-submitted to align the version with part 1 of the patch series > > > > .../bindings/iio/light/amstaos,tsl2591.yaml | 50 +++ > > 1 file changed, 50 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml > > > > > Please add Acked-by/Reviewed-by tags when posting new versions. However, > there's no need to repost patches *only* to add the tags. The upstream > maintainer will do that for acks received on the version they apply. > > If a tag was not added on purpose, please state why and what changed. > Thanks for pointing that out Rob, will amend that now and resend for this version.
[PATCH v6 1/2] Added AMS tsl2591 driver implementation
Driver implementation for AMS/TAOS tsl2591 ambient light sensor. This driver supports configuration via device tree and sysfs. Supported channels for raw infrared light intensity, raw combined light intensity and illuminance in lux. The driver additionally supports iio events on lower and upper thresholds. This is a very-high sensitivity light-to-digital converter that transforms light intensity into a digital signal. Datasheet: https://ams.com/tsl25911#tab/documents Signed-off-by: Joe Sandom --- Changes in v6; - Separated tsl2591_set_als_thresholds into tsl2591_set_als_lower_threshold and tsl2591_set_upper_threshold for cleaner handling of forcing values when thresholds are met - Added als persist value as an argument to tsl2591_set_als_persist_cycle to assign to chip from within the function. Cleaner approach. - Cleaned up power_state handling in tsl2591_resume function - Corrected interrupt handler to return IRQ_NONE in case of spurious interrupt drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile |1 + drivers/iio/light/tsl2591.c | 1215 +++ 3 files changed, 1227 insertions(+) create mode 100644 drivers/iio/light/tsl2591.c diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 33ad4dd0b5c7..6a69a9a3577a 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -501,6 +501,17 @@ config TSL2583 Provides support for the TAOS tsl2580, tsl2581 and tsl2583 devices. Access ALS data via iio, sysfs. +config TSL2591 +tristate "TAOS TSL2591 ambient light sensor" +depends on I2C +help + Select Y here for support of the AMS/TAOS TSL2591 ambient light sensor, + featuring channels for combined visible + IR intensity and lux illuminance. + Access data via iio and sysfs. Supports iio_events. + + To compile this driver as a module, select M: the + module will be called tsl2591. + config TSL2772 tristate "TAOS TSL/TMD2x71 and TSL/TMD2x72 Family of light and proximity sensors" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index ea376deaca54..d10912faf964 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -48,6 +48,7 @@ obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o obj-$(CONFIG_TCS3414) += tcs3414.o obj-$(CONFIG_TCS3472) += tcs3472.o obj-$(CONFIG_TSL2583) += tsl2583.o +obj-$(CONFIG_TSL2591) += tsl2591.o obj-$(CONFIG_TSL2772) += tsl2772.o obj-$(CONFIG_TSL4531) += tsl4531.o obj-$(CONFIG_US5182D) += us5182d.o diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c new file mode 100644 index ..d38569ce165e --- /dev/null +++ b/drivers/iio/light/tsl2591.c @@ -0,0 +1,1215 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2020 Joe Sandom + * + * Datasheet: https://ams.com/tsl25911#tab/documents + * + * Device driver for the TAOS TSL2591. This is a very-high sensitivity + * light-to-digital converter that transforms light intensity into a digital + * signal. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include + +/* ADC integration time, field value to time in ms*/ +#define TSL2591_FVAL_TO_ATIME(x) (((x) + 1) * 100) +/* ADC integration time, time in ms to field value */ +#define TSL2591_ATIME_TO_FVAL(x) (((x) / 100) - 1) + +/* TSL2591 register set */ +#define TSL2591_ENABLE 0x00 +#define TSL2591_CONTROL 0x01 +#define TSL2591_AILTL 0x04 +#define TSL2591_AILTH 0x05 +#define TSL2591_AIHTL 0x06 +#define TSL2591_AIHTH 0x07 +#define TSL2591_NP_AILTL0x08 +#define TSL2591_NP_AILTH0x09 +#define TSL2591_NP_AIHTL0x0A +#define TSL2591_NP_AIHTH0x0B +#define TSL2591_PERSIST 0x0C +#define TSL2591_PACKAGE_ID 0x11 +#define TSL2591_DEVICE_ID 0x12 +#define TSL2591_STATUS 0x13 +#define TSL2591_C0_DATAL0x14 +#define TSL2591_C0_DATAH0x15 +#define TSL2591_C1_DATAL0x16 +#define TSL2591_C1_DATAH0x17 + +/* TSL2591 command register definitions */ +#define TSL2591_CMD_NOP (BIT(5) | BIT(7)) +#define TSL2591_CMD_SF GENMASK(7, 5) +#define TSL2591_CMD_SF_INTSET (BIT(2) | TSL2591_CMD_SF) +#define TSL2591_CMD_SF_CALS_I (BIT(0) | BIT(2) | TSL2591_CMD_SF) +#define TSL2591_CMD_SF_CALS_NPI (GENMASK(2, 0) | TSL2591_CMD_SF) +#define TSL2591_CMD_SF_CNP_ALSI (BIT(1) | BIT(3) | TSL2591_CMD_SF) + +/* TSL2591 enable register definitions */ +#define TSL2591_PWR_ON BIT(0) +#define TSL2591_PWR_OFF (0 << 0) +#define TSL2591_ENABLE_ALS BIT(1) +#define TSL2591_ENABLE_ALS_INT BIT(4) +#define TSL2591_ENABLE_SLEEP_INTBIT(6) +#define TSL2591_ENABLE_NP_INT BIT(7) + +/* TSL2591 control regist
[PATCH v6 2/2] Added AMS tsl2591 device tree binding
Device tree binding for AMS/TAOS tsl2591 ambient light sensor.
This driver supports configuration via device tree and sysfs.
Supported channels for raw infrared light intensity,
raw combined light intensity and illuminance in lux.
The driver additionally supports iio events on lower and
upper thresholds.
This is a very-high sensitivity light-to-digital converter that
transforms light intensity into a digital signal.
Datasheet: https://ams.com/tsl25911#tab/documents
Signed-off-by: Joe Sandom
---
Changes in v6:
- No changes
Notes:
- Re-submitted to align the version with part 1 of the patch series
.../bindings/iio/light/amstaos,tsl2591.yaml | 50 +++
1 file changed, 50 insertions(+)
create mode 100644
Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
new file mode 100644
index ..596a3bc770f4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/amstaos,tsl2591.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/amstaos,tsl2591.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS/TAOS TSL2591 Ambient Light Sensor (ALS)
+
+maintainers:
+ - Joe Sandom
+
+description: |
+ AMS/TAOS TSL2591 is a very-high sensitivity
+ light-to-digital converter that transforms light intensity into a digital
+ signal.
+
+properties:
+ compatible:
+const: amstaos,tsl2591
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+description:
+ Interrupt (INT:Pin 2) Active low. Should be set to IRQ_TYPE_EDGE_FALLING.
+ interrupt is used to detect if the light intensity has fallen below
+ or reached above the configured threshold values.
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+i2c {
+#address-cells = <1>;
+#size-cells = <0>;
+
+tsl2591@29 {
+compatible = "amstaos,tsl2591";
+reg = <0x29>;
+interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
+ };
+};
+...
--
2.17.1
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
On 3/25/21 5:26 AM, Miroslav Benes wrote:
On Thu, 25 Mar 2021, Dong Kai wrote:
commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like
PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads
by making the freezer not send them fake signals.
Here live patching consistency model call klp_send_signals to wake up
all tasks by send fake signal to all non-kthread which only check the
PF_KTHREAD flag, so it still send signal to io threads which may lead to
freezeing issue of io threads.
I suppose this could happen, but it will also affect the live patching
transition if the io threads do not react to signals.
Are you able to reproduce it easily? I mean, is there a testcase I could
use to take a closer look?
If repro is only hypothetical at this point, perhaps we can artificially
create it in selftests? And useful to verify the future change you
mentioned in your other reply?
-- Joe
Re: [PATCH] livepatch: klp_send_signal should treat PF_IO_WORKER like PF_KTHREAD
On 3/24/21 9:48 PM, Dong Kai wrote:
commit 15b2219facad ("kernel: freezer should treat PF_IO_WORKER like
PF_KTHREAD for freezing") is to fix the freezeing issue of IO threads
nit: s/freezeing/freezing
by making the freezer not send them fake signals.
Here live patching consistency model call klp_send_signals to wake up
all tasks by send fake signal to all non-kthread which only check the
PF_KTHREAD flag, so it still send signal to io threads which may lead to
freezeing issue of io threads.
Here we take the same fix action by treating PF_IO_WORKERS as PF_KTHREAD
within klp_send_signal function.
Signed-off-by: Dong Kai
---
note:
the io threads freeze issue links:
[1] https://lore.kernel.org/io-uring/yegnip43%2f6kfn...@kevinlocke.name/
[2]
https://lore.kernel.org/io-uring/d7350ce7-17dc-75d7-611b-27ebf2cb5...@kernel.dk/
kernel/livepatch/transition.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
index f6310f848f34..0e1c35c8f4b4 100644
--- a/kernel/livepatch/transition.c
+++ b/kernel/livepatch/transition.c
@@ -358,7 +358,7 @@ static void klp_send_signals(void)
* Meanwhile the task could migrate itself and the action
* would be meaningless. It is not serious though.
*/
- if (task->flags & PF_KTHREAD) {
+ if (task->flags & (PF_KTHREAD | PF_IO_WORKER)) {
/*
* Wake up a kthread which sleeps interruptedly and
* still has not been migrated.
(PF_KTHREAD | PF_IO_WORKER) is open coded in soo many places maybe this
is a silly question, but...
If the livepatch code could use fake_signal_wake_up(), we could
consolidate the pattern in klp_send_signals() with the one in
freeze_task(). Then there would only one place for wake up / fake
signal logic.
I don't fully understand the differences in the freeze_task() version,
so I only pose this as a question and not v2 request.
As it is, this change seems logical to me, so:
Acked-by: Joe Lawrence
Thanks,
-- Joe
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 23:36 +0100, Rasmus Villemoes wrote: > On 24/03/2021 23.18, Joe Perches wrote: > > There's no silly game here. %pe would either print a string or a value. > > A hashed value, that is, never the raw value. There is value in printing the raw value. As discussed, it can simplify the code. The worry about exposing a ptr value is IMO overstated. It's trivial to inspect the uses and _all_ %p uses need inspection and validation at acceptance anyway.
Re: [PATCH] Fix fnic driver to remove bogus ratelimit messages.
On Tue, 2021-03-23 at 10:27 -0700, ldun...@suse.com wrote:
> From: Lee Duncan
>
> Commit b43abcbbd5b1 ("scsi: fnic: Ratelimit printks to avoid
> looding when vlan is not set by the switch.i") added
> printk_ratelimit() in front of a couple of debug-mode
> messages, to reduce logging overrun when debugging the
> driver. The code:
>
> > if (printk_ratelimit())
> > FNIC_FCS_DBG(KERN_DEBUG, fnic->lport->host,
> > "Start VLAN Discovery\n");
>
> ends up calling printk_ratelimit() quite often, triggering
> many kernel messages about callbacks being surpressed.
>
> The fix is to decompose FNIC_FCS_DBG(), then change the order
> of checks so that printk_ratelimit() is only called if
> driver debugging is enabled.
Please make sure to cc the fnic maintainers when submitting patches
to their drivers.
$ ./scripts/get_maintainer.pl drivers/scsi/fnic/
Satish Kharat (supporter:CISCO FCOE HBA DRIVER)
Sesidhar Baddela (supporter:CISCO FCOE HBA DRIVER)
Karan Tilak Kumar (supporter:CISCO FCOE HBA DRIVER)
"James E.J. Bottomley" (maintainer:SCSI SUBSYSTEM)
"Martin K. Petersen" (maintainer:SCSI SUBSYSTEM)
linux-s...@vger.kernel.org (open list:CISCO FCOE HBA DRIVER)
linux-kernel@vger.kernel.org (open list)
My preference would be to rewrite the FNIC__DBG macros to use
a single fnic_dbg macro and add a new fnic_dbg_ratelimited macro.
Something like the below:
This converts a few uses at KERN_INFO to KERN_DEBUG, only uses fnic
and adds a macro concatenation instead of multiple macros.
Miscellanea:
o Add missing newlines
o Coalesce formats
o Align arguments
---
compile tested only
drivers/scsi/fnic/fnic.h | 45 +++
drivers/scsi/fnic/fnic_fcs.c | 92 +-
drivers/scsi/fnic/fnic_isr.c | 9 +-
drivers/scsi/fnic/fnic_main.c | 9 +-
drivers/scsi/fnic/fnic_scsi.c | 280 --
5 files changed, 162 insertions(+), 273 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 69f373b53132..f12cd6ed9296 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -130,36 +130,27 @@
extern unsigned int fnic_log_level;
extern unsigned int io_completions;
-#define FNIC_MAIN_LOGGING 0x01
-#define FNIC_FCS_LOGGING 0x02
-#define FNIC_SCSI_LOGGING 0x04
-#define FNIC_ISR_LOGGING 0x08
-
-#define FNIC_CHECK_LOGGING(LEVEL, CMD) \
-do { \
- if (unlikely(fnic_log_level & LEVEL)) \
- do {\
- CMD;\
- } while (0);\
+#define FNIC_DBG_MAIN 0x01
+#define FNIC_DBG_FCS 0x02
+#define FNIC_DBG_SCSI 0x04
+#define FNIC_DBG_ISR 0x08
+
+#define fnic_dbg(fnic, TYPE, fmt, ...) \
+do { \
+ if (unlikely(fnic_log_level & FNIC_DBG_##TYPE)) \
+ shost_printk(KERN_DEBUG, (fnic)->lport->host, \
+fmt, ##__VA_ARGS__); \
} while (0)
-#define FNIC_MAIN_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_MAIN_LOGGING, \
-shost_printk(kern_level, host, fmt, ##args);)
-
-#define FNIC_FCS_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_FCS_LOGGING,\
-shost_printk(kern_level, host, fmt, ##args);)
-
-#define FNIC_SCSI_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_SCSI_LOGGING, \
-shost_printk(kern_level, host, fmt, ##args);)
-
-#define FNIC_ISR_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_ISR_LOGGING,\
-shost_printk(kern_level, host, fmt, ##args);)
+#define fnic_dbg_ratelimited(fnic, TYPE, fmt, ...) \
+do { \
+ if (unlikely(fnic_log_level & FNIC_DBG_##TYPE) && \
+ printk_ratelimit()) \
+ shost_printk(KERN_DEBUG, (fnic)->lport->host, \
+fmt, ##__VA_ARGS__); \
+} while (0)
-#define FNIC_MAIN_NOTE(kern_level, host, fmt, args...) \
+#define FNIC_MAIN_NOTE(kern_level, host, fmt, args...) \
shost_printk(kern_level, host, fmt, ##args)
extern const char *fnic_state_str[];
diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
index 881c4823d7e2..81eb278ea025 100644
--- a/drivers/scsi/fnic/fnic_fcs.c
+++ b/drivers/scsi/fnic/fnic_fcs.c
@@ -75,9 +75,8 @@ void fnic_handle_link(struct work_struct
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 20.24, Joe Perches wrote:
> > On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> > > On 24/03/2021 18.20, Joe Perches wrote:
> > >
> > > >
> > > > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > > > sort of code would work.
> > >
> > > No, because that would leak the pointer value when somebody has
> > > accidentally passed a real kernel pointer to %pe.
> >
> > I think it's not really an issue.
> >
> > _All_ code that uses %p extensions need inspection anyway.
>
> There are now a bunch of sanity checks in place that catch e.g. an
> ERR_PTR passed to an extension that would derefence the pointer;
> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)
> is another of those safeguards.
>
> > It's already possible to intentionally 'leak' the ptr value
> > by using %pe, -ptr so I think that's not really an issue.
> >
>
> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.
> How would that leak the value if ptr is an ordinary kernel pointer?
> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.
You are confusing ERR_PTR with IS_ERR
ERR_PTR is just
include/linux/err.h:static inline void * __must_check ERR_PTR(long error)
include/linux/err.h-{
include/linux/err.h-return (void *) error;
include/linux/err.h-}f
> If you want to print the pointer value just do %px. No need for silly
> games.
There's no silly game here. %pe would either print a string or a value.
It already does that in 2 cases.
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 18.20, Joe Perches wrote:
> > On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> > > On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > > > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> > > []
> > > > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > > > []
> > > > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > > > drm_encoder *encoder)
> > > > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > > > encoder);
> > > > > >
> > > > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > > + __func__, ERR_PTR(mux));
> > > > >
> > > > > This does not compile without warnings.
> > > > >
> > > > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > | ^~
> > > > >
> > > > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > > > is converting an int a void * to decode the error type and
> > > > > emit it as a string.
> > > >
> > > > Sorry about that.
> > > >
> > > > I decided against using ERR_PTR() in order to also check for
> > > > positive array overflow, but the version I tested was different from
> > > > the version I sent.
> > > >
> > > > v3 coming.
> > >
> > > Thanks. No worries.
> > >
> > > Up to you, vsprintf would emit the positive mux as a funky hashed
> > > hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> > > perhaps %d without the ERR_PTR use makes the most sense.
> > >
>
> >
> > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > sort of code would work.
>
> No, because that would leak the pointer value when somebody has
> accidentally passed a real kernel pointer to %pe.
I think it's not really an issue.
_All_ code that uses %p extensions need inspection anyway.
It's already possible to intentionally 'leak' the ptr value
by using %pe, -ptr so I think that's not really an issue.
>
> If the code wants a cute -EFOO string explaining what's wrong, what
> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
> messages
>
> if (mux < 0)
> ...
> else if (mux >= ARRAY_SIZE())
> ...
Multiple tests, more unnecessary code, multiple format strings, etc...
[RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> []
> > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > []
> > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > drm_encoder *encoder)
> > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > encoder);
> > > >
> > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > + __func__, ERR_PTR(mux));
> > >
> > > This does not compile without warnings.
> > >
> > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > | ^~
> > >
> > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > is converting an int a void * to decode the error type and
> > > emit it as a string.
> >
> > Sorry about that.
> >
> > I decided against using ERR_PTR() in order to also check for
> > positive array overflow, but the version I tested was different from
> > the version I sent.
> >
> > v3 coming.
>
> Thanks. No worries.
>
> Up to you, vsprintf would emit the positive mux as a funky hashed
> hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> perhaps %d without the ERR_PTR use makes the most sense.
>
>
Maybe it's better to output non PTR_ERR %pe uses as decimal so this
sort of code would work.
---
lib/vsprintf.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3600db686fa4..debdd1c62038 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -619,19 +619,23 @@ static char *string_nocheck(char *buf, char *end, const
char *s,
return widen_string(buf, len, end, spec);
}
-static char *err_ptr(char *buf, char *end, void *ptr,
-struct printf_spec spec)
+static noinline_for_stack
+char *err_ptr(char *buf, char *end, void *ptr, struct printf_spec spec)
{
int err = PTR_ERR(ptr);
- const char *sym = errname(err);
- if (sym)
- return string_nocheck(buf, end, sym, spec);
+ if (IS_ERR(ptr)) {
+ const char *sym = errname(err);
+
+ if (sym)
+ return string_nocheck(buf, end, sym, spec);
+ }
/*
-* Somebody passed ERR_PTR(-1234) or some other non-existing
-* Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to
-* printing it as its decimal representation.
+* Somebody passed ERR_PTR(-1234) or some other non-existing -E
+* or perhaps CONFIG_SYMBOLIC_ERRNAME=n
+* or perhaps a positive number like an array index
+* Fall back to printing it as its decimal representation.
*/
spec.flags |= SIGN;
spec.base = 10;
@@ -2407,9 +2411,7 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
case 'x':
return pointer_string(buf, end, ptr, spec);
case 'e':
- /* %pe with a non-ERR_PTR gets treated as plain %p */
- if (!IS_ERR(ptr))
- break;
+ /* %pe with a non-ERR_PTR(ptr) gets treated as %ld */
return err_ptr(buf, end, ptr, spec);
case 'u':
case 'k':
---
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
[]
> > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > []
> > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > drm_encoder *encoder)
> > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> > >
> > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > + __func__, ERR_PTR(mux));
> >
> > This does not compile without warnings.
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> > of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > | ^~
> >
> > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > is converting an int a void * to decode the error type and
> > emit it as a string.
>
> Sorry about that.
>
> I decided against using ERR_PTR() in order to also check for
> positive array overflow, but the version I tested was different from
> the version I sent.
>
> v3 coming.
Thanks. No worries.
Up to you, vsprintf would emit the positive mux as a funky hashed
hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
perhaps %d without the ERR_PTR use makes the most sense.
Re: [PATCH] amdgpu: fix gcc -Wrestrict warning
On Tue, 2021-03-23 at 14:04 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> gcc warns about an sprintf() that uses the same buffer as source
> and destination, which is undefined behavior in C99:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function
> 'amdgpu_securedisplay_debugfs_write':
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf'
> argument 3 overlaps destination object 'i2c_output' [-Werror=restrict]
> 141 | sprintf(i2c_output, "%s 0x%X", i2c_output,
> | ^~
> 142 |
> securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
> |
> ~
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination
> object referenced by 'restrict'-qualified argument 1 was declared here
> 97 | char i2c_output[256];
> | ^~
>
> Rewrite it to remember the current offset into the buffer instead.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> index 834440ab9ff7..69d7f6bff5d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
> file *f, const char __u
> ret = psp_securedisplay_invoke(psp,
> TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
> if (!ret) {
> if (securedisplay_cmd->status ==
> TA_SECUREDISPLAY_STATUS__SUCCESS) {
> + int pos = 0;
> memset(i2c_output, 0, sizeof(i2c_output));
> for (i = 0; i <
> TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
> - sprintf(i2c_output, "%s 0x%X",
> i2c_output,
> + pos += sprintf(i2c_output + pos, "
> 0x%X",
>
> securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
> dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
> out put is :%s\n", i2c_output);
Perhaps use a hex output like:
---
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
index 9cf856c94f94..25bb34c72d20 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
@@ -97,13 +97,12 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
file *f, const char __u
uint32_t op;
int i;
char str[64];
- char i2c_output[256];
int ret;
if (*pos || size > sizeof(str) - 1)
return -EINVAL;
- memset(str, 0, sizeof(str));
+ memset(str, 0, sizeof(str));
ret = copy_from_user(str, buf, size);
if (ret)
return -EFAULT;
@@ -139,11 +138,9 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
file *f, const char __u
ret = psp_securedisplay_invoke(psp,
TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
if (!ret) {
if (securedisplay_cmd->status ==
TA_SECUREDISPLAY_STATUS__SUCCESS) {
- memset(i2c_output, 0, sizeof(i2c_output));
- for (i = 0; i <
TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
- sprintf(i2c_output, "%s 0x%X",
i2c_output,
-
securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
- dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
out put is :%s\n", i2c_output);
+ dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
output is: %*ph\n",
+(int)TA_SECUREDISPLAY_I2C_BUFFER_SIZE,
+
securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf);
} else {
psp_securedisplay_parse_resp_status(psp,
securedisplay_cmd->status);
}
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
>
> The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> but Liu Ying points out that the driver may hit the out-of-bounds
> problem at runtime anyway.
>
> Signed-off-by: Arnd Bergmann
> ---
> v2: fix subject line
> expand patch description
> print mux number
> check upper bound as well
[]
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> + __func__, ERR_PTR(mux));
This does not compile without warnings.
drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument of
type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
| ^~
If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
is converting an int a void * to decode the error type and
emit it as a string.
