Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines
On 21-02-2019 10:42, Mark Brown wrote:
> On Thu, Feb 21, 2019 at 08:22:53AM +0800, Axel Lin wrote:
>> Olliver Schinagl 於 2019年2月21日 週四 上午6:57寫道：
>>> On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin
>>> wrote:
The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
very hard to check the linear range settings because it needs to check
the defines one-by-one.
The original code without the defines is very good in readability
as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.
>>> Are you suggesting that magic values and hex numbers are more readable?
>> For example:
>> static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
>>REGULATOR_LINEAR_RANGE(50, 0x0, 0x46, 1),
>>REGULATOR_LINEAR_RANGE(122, 0x47, 0x4b, 2),
>> };
>> Above looks very clear to me as it describes the linear ranges:
>> 1st linear range: min_uV is 50, from selector 0 ~ 0x46, the uV_step is
>> 1
>> 2nd linear range: min_uV is 122, from selector 0x47 ~ 0x4b, the
>> uV_step is 2
>> And it's easy to check the min_sel and max_sel in each linear range.
> Frankly I tend to agree with Axel here - the defines are used in exactly
> one place so their main impact is that you can't read the ranges
> directly in the array but have to jump around to read through the
> defines.
The number of times the defines are used, are of course no reason for it
to be a bad thing. Defines should certainly not be thought of as a tool
to only reduce duplicity of course. Also being consistent where others
do have multiple invocations makes sense.
I will not disagree that it may be extra work to look up the define
(especially if there is no tool tip or split view in the editor) but
reading the whole lot of code, with only the magic values, you still
have to look up the meaning of each magic value, have to guess which one
has the same meaning etc.
Further more, I do believe far more people reading will find an define
to be more descriptive to read. Whoever needs to actually go in and
fix/change things.
Also, how many steps are in the first or second regulator ranges? which
bits are these exactly? Etc etc, you still have to use your mind to
figure things out.
Finally, while some people live and breath hexadecimals and bitmasks,
others just see magic values. So having something readable there, may
make it harder for the one that needs to work on it (which hopefully is
almost never, as these things are not very dynamic/commonly changing)
but passers by see readable text and understand what is happening at a
glance. Which is why we where taught not to use magic values in the
first place :)
[PATCH V3] staging: ks7010: removed unused variables
From: Bo Yu
Compiling the kernel with W=1 results in the following warning:
drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
set but not used [-Wunused-but-set-variable]
u16 mib_val_type;
drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
set but not used [-Wunused-but-set-variable]
u16 mib_val_size;
drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
set but not used [-Wunused-but-set-variable]
u16 result_code;
Remove these variables.
Cc: Greg Kroah-Hartman
Cc: Sergio Paracuellos
Cc: Quytelda Kahja
Signed-off-by: Bo Yu
---
Changelog:
V3: fix spell issue in subject line
V2: fix patch format
---
drivers/staging/ks7010/ks_hostif.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index d938b0997a53..913d8996437a 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
struct net_device *dev = priv->net_dev;
u32 mib_status;
u32 mib_attribute;
- u16 mib_val_size;
- u16 mib_val_type;
mib_status = get_dword(priv);
mib_attribute = get_dword(priv);
- mib_val_size = get_word(priv);
- mib_val_type = get_word(priv);
if (mib_status) {
netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
@@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
*priv)
static
void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
{
- u16 result_code;
- result_code = get_word(priv);
priv->infra_status = 1; /* infrastructure mode set */
hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
}
--
2.11.0
Re: [PATCH] staging/ks7070: Removed unused varibale
On Sat, Feb 23, 2019 at 01:39:39AM -0500, Bo YU wrote:
> From: Bo Yu
>
> Compiling the kernel with W=1 results in the following warning:
>
> drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
> set but not used [-Wunused-but-set-variable]
> u16 mib_val_type;
>
> drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
> set but not used [-Wunused-but-set-variable]
> u16 mib_val_size;
>
> drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
> set but not used [-Wunused-but-set-variable]
> u16 result_code;
>
> Remove these variables.
>
> Rebase on next-20190222
>
> Cc: Greg Kroah-Hartman
> Cc: Sergio Paracuellos
> Cc: Quytelda Kahja
>
> Signed-off-by: Bo Yu
> ---
> drivers/staging/ks7010/ks_hostif.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks_hostif.c
> b/drivers/staging/ks7010/ks_hostif.c
> index d938b0997a53..913d8996437a 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
> struct net_device *dev = priv->net_dev;
> u32 mib_status;
> u32 mib_attribute;
> - u16 mib_val_size;
> - u16 mib_val_type;
>
> mib_status = get_dword(priv);
> mib_attribute = get_dword(priv);
> - mib_val_size = get_word(priv);
> - mib_val_type = get_word(priv);
The function get_word() actually does something to the priv structure,
so you just broke the driver :(
>
> if (mib_status) {
> netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
> @@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
> *priv)
> static
> void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
> {
> - u16 result_code;
>
> - result_code = get_word(priv);
> priv->infra_status = 1; /* infrastructure mode set */
> hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
Same here, odds are you just broke things :(
Please be more careful.
greg k-h
Re: [PATCH V2] staging: ks7070: removed unused varibales
Hi Bo,
On Sat, Feb 23, 2019 at 8:27 AM Bo YU wrote:
>
> From: Bo Yu
>
> Compiling the kernel with W=1 results in the following warning:
>
> drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
> set but not used [-Wunused-but-set-variable]
> u16 mib_val_type;
>
> drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
> set but not used [-Wunused-but-set-variable]
> u16 mib_val_size;
>
> drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
> set but not used [-Wunused-but-set-variable]
> u16 result_code;
>
> Remove these variables.
>
> Rebase on next-20190222
> V2: fix patch format
As Dan has just said, this two rebase and v2 stuff is not needed in
the changelog.
Please, put it under the --- cut off line.
>
> Cc: Greg Kroah-Hartman
> Cc: Sergio Paracuellos
> Cc: Quytelda Kahja
>
> Signed-off-by: Bo Yu
> ---
> drivers/staging/ks7010/ks_hostif.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks_hostif.c
> b/drivers/staging/ks7010/ks_hostif.c
> index d938b0997a53..913d8996437a 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
> struct net_device *dev = priv->net_dev;
> u32 mib_status;
> u32 mib_attribute;
> - u16 mib_val_size;
> - u16 mib_val_type;
>
> mib_status = get_dword(priv);
> mib_attribute = get_dword(priv);
> - mib_val_size = get_word(priv);
> - mib_val_type = get_word(priv);
>
> if (mib_status) {
> netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
> @@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
> *priv)
> static
> void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
> {
> - u16 result_code;
>
> - result_code = get_word(priv);
> priv->infra_status = 1; /* infrastructure mode set */
> hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
> }
> --
> 2.11.0
>
The commit message is still wrong. The driver name is not called
ks7070 is called ks7010, right? and
also the typo with "varibales" is still wrong... Let's do all changes
right at once, please :)
Best regards,
Sergio Paracuellos
Re: [PATCH V2] staging: ks7070: removed unused varibales
On Sat, Feb 23, 2019 at 02:27:12AM -0500, Bo YU wrote:
From: Bo Yu
Compiling the kernel with W=1 results in the following warning:
drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
set but not used [-Wunused-but-set-variable]
u16 mib_val_type;
drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
set but not used [-Wunused-but-set-variable]
u16 mib_val_size;
drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
set but not used [-Wunused-but-set-variable]
u16 result_code;
Remove these variables.
Rebase on next-20190222
V2: fix patch format
Please drop it, will send V3
Thanks
Cc: Greg Kroah-Hartman
Cc: Sergio Paracuellos
Cc: Quytelda Kahja
Signed-off-by: Bo Yu
---
drivers/staging/ks7010/ks_hostif.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index d938b0997a53..913d8996437a 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
struct net_device *dev = priv->net_dev;
u32 mib_status;
u32 mib_attribute;
- u16 mib_val_size;
- u16 mib_val_type;
mib_status = get_dword(priv);
mib_attribute = get_dword(priv);
- mib_val_size = get_word(priv);
- mib_val_type = get_word(priv);
if (mib_status) {
netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
@@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
*priv)
static
void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
{
- u16 result_code;
- result_code = get_word(priv);
priv->infra_status = 1; /* infrastructure mode set */
hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
}
--
2.11.0
[PATCH V2] staging: ks7070: removed unused varibales
From: Bo Yu
Compiling the kernel with W=1 results in the following warning:
drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
set but not used [-Wunused-but-set-variable]
u16 mib_val_type;
drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
set but not used [-Wunused-but-set-variable]
u16 mib_val_size;
drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
set but not used [-Wunused-but-set-variable]
u16 result_code;
Remove these variables.
Rebase on next-20190222
V2: fix patch format
Cc: Greg Kroah-Hartman
Cc: Sergio Paracuellos
Cc: Quytelda Kahja
Signed-off-by: Bo Yu
---
drivers/staging/ks7010/ks_hostif.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index d938b0997a53..913d8996437a 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
struct net_device *dev = priv->net_dev;
u32 mib_status;
u32 mib_attribute;
- u16 mib_val_size;
- u16 mib_val_type;
mib_status = get_dword(priv);
mib_attribute = get_dword(priv);
- mib_val_size = get_word(priv);
- mib_val_type = get_word(priv);
if (mib_status) {
netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
@@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
*priv)
static
void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
{
- u16 result_code;
- result_code = get_word(priv);
priv->infra_status = 1; /* infrastructure mode set */
hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
}
--
2.11.0
Re: [PATCH] staging/ks7070: Removed unused varibale
On Sat, Feb 23, 2019 at 01:39:39AM -0500, Bo YU wrote: > From: Bo Yu > > Compiling the kernel with W=1 results in the following warning: > > drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’ > set but not used [-Wunused-but-set-variable] > u16 mib_val_type; > > drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’ > set but not used [-Wunused-but-set-variable] > u16 mib_val_size; > > drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’ > set but not used [-Wunused-but-set-variable] > u16 result_code; > > Remove these variables. > > Rebase on next-20190222 ^^^ Is this a v2 patch or something? Don't include this in the changelog. Put it under the --- cut off line. > > Cc: Greg Kroah-Hartman > Cc: Sergio Paracuellos > Cc: Quytelda Kahja > > Signed-off-by: Bo Yu > --- ^^^ > drivers/staging/ks7010/ks_hostif.c | 6 -- > 1 file changed, 6 deletions(-) > regards, dan carpenter
[PATCH] tools: testing: selftests: Remove duplicate headers
Remove duplicate headers which are included twice. Signed-off-by: Sabyasachi Gupta Signed-off-by: Souptick Joarder --- tools/testing/selftests/gpio/gpio-mockup-chardev.c | 1 - tools/testing/selftests/net/udpgso.c| 1 - tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c | 1 - tools/testing/selftests/proc/proc-self-syscall.c| 1 - tools/testing/selftests/rseq/rseq.h | 1 - tools/testing/selftests/timers/skew_consistency.c | 1 - tools/testing/selftests/x86/mpx-dig.c | 2 -- 7 files changed, 8 deletions(-) diff --git a/tools/testing/selftests/gpio/gpio-mockup-chardev.c b/tools/testing/selftests/gpio/gpio-mockup-chardev.c index aaa1e9f..d587c81 100644 --- a/tools/testing/selftests/gpio/gpio-mockup-chardev.c +++ b/tools/testing/selftests/gpio/gpio-mockup-chardev.c @@ -12,7 +12,6 @@ #include #include #include -#include #include #include #include diff --git a/tools/testing/selftests/net/udpgso.c b/tools/testing/selftests/net/udpgso.c index e279051..b8265ee 100644 --- a/tools/testing/selftests/net/udpgso.c +++ b/tools/testing/selftests/net/udpgso.c @@ -17,7 +17,6 @@ #include #include #include -#include #include #include #include diff --git a/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c b/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c index 167135b..af1b802 100644 --- a/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c +++ b/tools/testing/selftests/powerpc/pmu/ebb/fork_cleanup_test.c @@ -11,7 +11,6 @@ #include #include #include -#include #include "ebb.h" diff --git a/tools/testing/selftests/proc/proc-self-syscall.c b/tools/testing/selftests/proc/proc-self-syscall.c index 5ab5f48..3a4fec3 100644 --- a/tools/testing/selftests/proc/proc-self-syscall.c +++ b/tools/testing/selftests/proc/proc-self-syscall.c @@ -20,7 +20,6 @@ #include #include #include -#include #include #include diff --git a/tools/testing/selftests/rseq/rseq.h b/tools/testing/selftests/rseq/rseq.h index c72eb70..6c1126e7 100644 --- a/tools/testing/selftests/rseq/rseq.h +++ b/tools/testing/selftests/rseq/rseq.h @@ -16,7 +16,6 @@ #include #include #include -#include #include /* diff --git a/tools/testing/selftests/timers/skew_consistency.c b/tools/testing/selftests/timers/skew_consistency.c index 022b711..8066be9 100644 --- a/tools/testing/selftests/timers/skew_consistency.c +++ b/tools/testing/selftests/timers/skew_consistency.c @@ -32,7 +32,6 @@ #include #include #include -#include #include #include #include "../kselftest.h" diff --git a/tools/testing/selftests/x86/mpx-dig.c b/tools/testing/selftests/x86/mpx-dig.c index c13607e..880fbf6 100644 --- a/tools/testing/selftests/x86/mpx-dig.c +++ b/tools/testing/selftests/x86/mpx-dig.c @@ -8,9 +8,7 @@ #include #include #include -#include #include -#include #include #include #include -- 1.9.1
Re: [PATCH RFC v2 2/4] PCI: pciehp: Do not turn off slot if presence comes up after link
On Fri, Feb 22, 2019 at 07:56:28PM +, alex_gagn...@dellteam.com wrote:
> On 2/21/19 1:36 AM, Lukas Wunner wrote:
> > On Tue, Feb 19, 2019 at 07:20:28PM -0600, Alexandru Gagniuc wrote:
> >>mutex_lock(>state_lock);
> >> + present = pciehp_card_present(ctrl);
> >> + link_active = pciehp_check_link_active(ctrl);
> >>switch (ctrl->state) {
> >
> > These two assignments appear to be superfluous as you're also performing
> > them in pciehp_check_link_active().
>
> Not sure. Between the first check, and this check, you can have several
> seconds elapse depending on whether the driver's .probe()/remove() is
> invoked. Whatever you got at the beginning would be stale. If you had a
> picture dictionary and looked up 'bad idea', it would have a picture of
> the above code with the second check removed.
I don't quite follow. You're no longer using the "present" and
"link_active" variables in pciehp_handle_presence_or_link_change(),
the variables are set again further down in the function and you're
*also* reading PDS and DLLLA in is_delayed_presence_up_event().
So the above-quoted assignments are superfluous. Am I missing something?
(Sorry, I had copy-pasted the wrong function name, I meant
is_delayed_presence_up_event() above, not pciehp_check_link_active().
> I've got all the other review comments addressed in my local branch. I'm
> waiting on Lord Helgass' decision on which solution is better.
Can we keep this discussion in a neutral tone please?
Thanks,
Lukas
[PATCH] Use flush tlb last level when change protection
The protection attributes are only kept in last level tlb, so protection changing only need invalidate last level tlb, exclude the PWC entries. Signed-off-by: Xuefeng Wang --- mm/mprotect.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/mprotect.c b/mm/mprotect.c index 36cb358..0c4303d 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -287,7 +287,7 @@ static unsigned long change_protection_range(struct vm_area_struct *vma, /* Only flush the TLB if we actually modified any entries: */ if (pages) - flush_tlb_range(vma, start, end); + __flush_tlb_range(vma, start, end, PAGE_SIZE, true); dec_tlb_flush_pending(mm); return pages; -- 1.7.12.4
Re: [PATCH] staging/ks7070: Removed unused varibale
Hi Bo,
On Sat, Feb 23, 2019 at 7:39 AM Bo YU wrote:
>
> From: Bo Yu
>
> Compiling the kernel with W=1 results in the following warning:
>
> drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
> set but not used [-Wunused-but-set-variable]
> u16 mib_val_type;
>
> drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
> set but not used [-Wunused-but-set-variable]
> u16 mib_val_size;
>
> drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
> set but not used [-Wunused-but-set-variable]
> u16 result_code;
>
> Remove these variables.
>
> Rebase on next-20190222
>
> Cc: Greg Kroah-Hartman
> Cc: Sergio Paracuellos
> Cc: Quytelda Kahja
>
> Signed-off-by: Bo Yu
> ---
> drivers/staging/ks7010/ks_hostif.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks_hostif.c
> b/drivers/staging/ks7010/ks_hostif.c
> index d938b0997a53..913d8996437a 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
> struct net_device *dev = priv->net_dev;
> u32 mib_status;
> u32 mib_attribute;
> - u16 mib_val_size;
> - u16 mib_val_type;
>
> mib_status = get_dword(priv);
> mib_attribute = get_dword(priv);
> - mib_val_size = get_word(priv);
> - mib_val_type = get_word(priv);
>
> if (mib_status) {
> netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
> @@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
> *priv)
> static
> void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
> {
> - u16 result_code;
>
> - result_code = get_word(priv);
> priv->infra_status = 1; /* infrastructure mode set */
> hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
> }
> --
> 2.11.0
>
PATCH commit message is wrong: format, typos and driver name:
[PATCH] staging/ks7070: Removed unused varibale
Should be "staging: ks7010: removed unused variables"
Thanks,
Sergio Paracuellos
Re: [PATCH v2] Fix HS setting in mmc_hs400_to_hs200()
Hi Ulf, this is just a ping, do you have any comments of this series of patches ? Thx! On Fri, 2019-02-15 at 13:59 +0800, Chaotian Jing wrote: > Change vs v1: > do not retry CMD6 in __mmc_switch() > do not move the reduce clock operation after switch HS > > Chaotian Jing (2): > mmc: core: do not retry CMD6 in __mmc_switch() > mmc: mmc: Fix HS setting in mmc_hs400_to_hs200() > > drivers/mmc/core/mmc.c | 21 +++-- > drivers/mmc/core/mmc_ops.c | 2 +- > 2 files changed, 20 insertions(+), 3 deletions(-) >
[PATCH] staging/ks7070: Removed unused varibale
From: Bo Yu
Compiling the kernel with W=1 results in the following warning:
drivers/staging/ks7010/ks_hostif.c:465:6: warning: variable ‘mib_val_type’
set but not used [-Wunused-but-set-variable]
u16 mib_val_type;
drivers/staging/ks7010/ks_hostif.c:464:6: warning: variable ‘mib_val_size’
set but not used [-Wunused-but-set-variable]
u16 mib_val_size;
drivers/staging/ks7010/ks_hostif.c:786:6: warning: variable ‘result_code’
set but not used [-Wunused-but-set-variable]
u16 result_code;
Remove these variables.
Rebase on next-20190222
Cc: Greg Kroah-Hartman
Cc: Sergio Paracuellos
Cc: Quytelda Kahja
Signed-off-by: Bo Yu
---
drivers/staging/ks7010/ks_hostif.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index d938b0997a53..913d8996437a 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -461,13 +461,9 @@ void hostif_mib_get_confirm(struct ks_wlan_private *priv)
struct net_device *dev = priv->net_dev;
u32 mib_status;
u32 mib_attribute;
- u16 mib_val_size;
- u16 mib_val_type;
mib_status = get_dword(priv);
mib_attribute = get_dword(priv);
- mib_val_size = get_word(priv);
- mib_val_type = get_word(priv);
if (mib_status) {
netdev_err(priv->net_dev, "attribute=%08X, status=%08X\n",
@@ -783,9 +779,7 @@ void hostif_ps_adhoc_set_confirm(struct ks_wlan_private
*priv)
static
void hostif_infrastructure_set_confirm(struct ks_wlan_private *priv)
{
- u16 result_code;
- result_code = get_word(priv);
priv->infra_status = 1; /* infrastructure mode set */
hostif_sme_enqueue(priv, SME_MODE_SET_CONFIRM);
}
--
2.11.0
[PATCH] tmpfs: fix uninitialized return value in shmem_link
From: "Darrick J. Wong"
When we made the shmem_reserve_inode call in shmem_link conditional, we
forgot to update the declaration for ret so that it always has a known
value. Dan Carpenter pointed out this deficiency in the original patch.
Fixes: 1062af920c07 ("tmpfs: fix link accounting when a tmpfile is linked in")
Reported-by: Dan Carpenter
Signed-off-by: Darrick J. Wong
Signed-off-by: Hugh Dickins
---
mm/shmem.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 0905215fb016..2c012eee133d 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2848,7 +2848,7 @@ static int shmem_create(struct inode *dir, struct dentry
*dentry, umode_t mode,
static int shmem_link(struct dentry *old_dentry, struct inode *dir, struct
dentry *dentry)
{
struct inode *inode = d_inode(old_dentry);
- int ret;
+ int ret = 0;
/*
* No ordinary (disk based) filesystem counts links as inodes;
[PATCH v2 3/6] sched/cpufreq: Annotate cpufreq_update_util_data pointer with __rcu
Recently I added an RCU annotation check to rcu_assign_pointer(). All pointers assigned to RCU protected data are to be annotated with __rcu inorder to be able to use rcu_assign_pointer() similar to checks in other RCU APIs. This resulted in a sparse error: kernel//sched/cpufreq.c:41:9: sparse: error: incompatible types in comparison expression (different address spaces) Fix this by annotating cpufreq_update_util_data pointer with __rcu. This will also help sparse catch any future RCU misuage bugs. Signed-off-by: Joel Fernandes (Google) --- kernel/sched/cpufreq.c | 2 +- kernel/sched/sched.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/sched/cpufreq.c b/kernel/sched/cpufreq.c index 22bd8980f32f..e316ee7bb2e5 100644 --- a/kernel/sched/cpufreq.c +++ b/kernel/sched/cpufreq.c @@ -7,7 +7,7 @@ */ #include "sched.h" -DEFINE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); +DEFINE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); /** * cpufreq_add_update_util_hook - Populate the CPU's update_util_data pointer. diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index d04530bf251f..2ab545d40381 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -2166,7 +2166,7 @@ static inline u64 irq_time_read(int cpu) #endif /* CONFIG_IRQ_TIME_ACCOUNTING */ #ifdef CONFIG_CPU_FREQ -DECLARE_PER_CPU(struct update_util_data *, cpufreq_update_util_data); +DECLARE_PER_CPU(struct update_util_data __rcu *, cpufreq_update_util_data); /** * cpufreq_update_util - Take a note about CPU utilization changes. -- 2.21.0.rc0.258.g878e2cd30e-goog
[PATCH v2 4/6] sched_domain: Annotate RCU pointers properly
The scheduler uses RCU API in various places to access sched_domain
pointers. These cause sparse errors as below.
Many new errors show up because of an annotation check I added to
rcu_assign_pointer(). Let us annotate the pointers correctly which also
will help sparse catch any potential future bugs.
This fixes the following sparse errors:
rt.c:1681:9: error: incompatible types in comparison expression
deadline.c:1904:9: error: incompatible types in comparison expression
core.c:519:9: error: incompatible types in comparison expression
core.c:1634:17: error: incompatible types in comparison expression
fair.c:6193:14: error: incompatible types in comparison expression
fair.c:9883:22: error: incompatible types in comparison expression
fair.c:9897:9: error: incompatible types in comparison expression
sched.h:1287:9: error: incompatible types in comparison expression
topology.c:612:9: error: incompatible types in comparison expression
topology.c:615:9: error: incompatible types in comparison expression
sched.h:1300:9: error: incompatible types in comparison expression
topology.c:618:9: error: incompatible types in comparison expression
sched.h:1287:9: error: incompatible types in comparison expression
topology.c:621:9: error: incompatible types in comparison expression
sched.h:1300:9: error: incompatible types in comparison expression
topology.c:624:9: error: incompatible types in comparison expression
topology.c:671:9: error: incompatible types in comparison expression
stats.c:45:17: error: incompatible types in comparison expression
fair.c:5998:15: error: incompatible types in comparison expression
fair.c:5989:15: error: incompatible types in comparison expression
fair.c:5998:15: error: incompatible types in comparison expression
fair.c:5989:15: error: incompatible types in comparison expression
fair.c:6120:19: error: incompatible types in comparison expression
fair.c:6506:14: error: incompatible types in comparison expression
fair.c:6515:14: error: incompatible types in comparison expression
fair.c:6623:9: error: incompatible types in comparison expression
fair.c:5970:17: error: incompatible types in comparison expression
fair.c:8642:21: error: incompatible types in comparison expression
fair.c:9253:9: error: incompatible types in comparison expression
fair.c:9331:9: error: incompatible types in comparison expression
fair.c:9519:15: error: incompatible types in comparison expression
fair.c:9533:14: error: incompatible types in comparison expression
fair.c:9542:14: error: incompatible types in comparison expression
fair.c:9567:14: error: incompatible types in comparison expression
fair.c:9597:14: error: incompatible types in comparison expression
fair.c:9421:16: error: incompatible types in comparison expression
fair.c:9421:16: error: incompatible types in comparison expression
Signed-off-by: Joel Fernandes (Google)
---
include/linux/sched/topology.h | 4 ++--
kernel/sched/sched.h | 14 +++---
kernel/sched/topology.c| 10 +-
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
index c31d3a47a47c..4819c9e01e42 100644
--- a/include/linux/sched/topology.h
+++ b/include/linux/sched/topology.h
@@ -76,8 +76,8 @@ struct sched_domain_shared {
struct sched_domain {
/* These fields must be setup */
- struct sched_domain *parent;/* top domain must be null terminated */
- struct sched_domain *child; /* bottom domain must be null
terminated */
+ struct sched_domain __rcu *parent; /* top domain must be null
terminated */
+ struct sched_domain __rcu *child; /* bottom domain must be null
terminated */
struct sched_group *groups; /* the balancing groups of the domain */
unsigned long min_interval; /* Minimum balance interval ms */
unsigned long max_interval; /* Maximum balance interval ms */
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 2ab545d40381..ca6a79f57e7a 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -866,8 +866,8 @@ struct rq {
atomic_tnr_iowait;
#ifdef CONFIG_SMP
- struct root_domain *rd;
- struct sched_domain *sd;
+ struct root_domain *rd;
+ struct sched_domain __rcu *sd;
unsigned long cpu_capacity;
unsigned long cpu_capacity_orig;
@@ -1305,13 +1305,13 @@ static inline struct sched_domain
*lowest_flag_domain(int cpu, int flag)
return sd;
}
-DECLARE_PER_CPU(struct sched_domain *, sd_llc);
+DECLARE_PER_CPU(struct sched_domain __rcu *, sd_llc);
DECLARE_PER_CPU(int, sd_llc_size);
DECLARE_PER_CPU(int, sd_llc_id);
-DECLARE_PER_CPU(struct sched_domain_shared *, sd_llc_shared);
-DECLARE_PER_CPU(struct sched_domain *, sd_numa);
-DECLARE_PER_CPU(struct sched_domain *, sd_asym_packing);
-DECLARE_PER_CPU(struct sched_domain *, sd_asym_cpucapacity);
[PATCH v2 5/6] rcuwait: Annotate task_struct with __rcu
This suppresses sparse error generated due to the recently added
rcu_assign_pointer sparse check.
percpu-rwsem.c:162:9: sparse: error: incompatible types in comparison expression
exit.c:316:16: sparse: error: incompatible types in comparison expression
Signed-off-by: Joel Fernandes (Google)
---
include/linux/rcuwait.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/rcuwait.h b/include/linux/rcuwait.h
index 90bfa3279a01..563290fc194f 100644
--- a/include/linux/rcuwait.h
+++ b/include/linux/rcuwait.h
@@ -18,7 +18,7 @@
* awoken.
*/
struct rcuwait {
- struct task_struct *task;
+ struct task_struct __rcu *task;
};
#define __RCUWAIT_INITIALIZER(name)\
--
2.21.0.rc0.258.g878e2cd30e-goog
[PATCH v2 6/6] sched: Annotate perf_domain pointer with __rcu
This fixes the following sparse errors in sched/fair.c:
fair.c:6506:14: error: incompatible types in comparison expression
fair.c:8642:21: error: incompatible types in comparison expression
Using __rcu will also help sparse catch any future bugs.
Signed-off-by: Joel Fernandes (Google)
---
kernel/sched/sched.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index ca6a79f57e7a..c8e6514433a9 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -780,7 +780,7 @@ struct root_domain {
* NULL-terminated list of performance domains intersecting with the
* CPUs of the rd. Protected by RCU.
*/
- struct perf_domain *pd;
+ struct perf_domain __rcu *pd;
};
extern struct root_domain def_root_domain;
--
2.21.0.rc0.258.g878e2cd30e-goog
[PATCH v2 1/6] net: rtnetlink: Fix incorrect RCU API usage
rtnl_register_internal() and rtnl_unregister_all tries to directly
dereference an RCU protected pointed outside RCU read side section.
While this is Ok to do since a lock is held, let us use the correct
API to avoid programmer bugs in the future.
This also fixes sparse warnings arising from not using RCU API.
net/core/rtnetlink.c:332:13: warning: incorrect type in assignment
(different address spaces) net/core/rtnetlink.c:332:13:expected
struct rtnl_link **tab net/core/rtnetlink.c:332:13:got struct
rtnl_link *[noderef] *
Signed-off-by: Joel Fernandes (Google)
---
net/core/rtnetlink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5ea1bed08ede..98be4b4818a9 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -188,7 +188,7 @@ static int rtnl_register_internal(struct module *owner,
msgindex = rtm_msgindex(msgtype);
rtnl_lock();
- tab = rtnl_msg_handlers[protocol];
+ tab = rtnl_dereference(rtnl_msg_handlers[protocol]);
if (tab == NULL) {
tab = kcalloc(RTM_NR_MSGTYPES, sizeof(void *), GFP_KERNEL);
if (!tab)
@@ -329,7 +329,7 @@ void rtnl_unregister_all(int protocol)
BUG_ON(protocol < 0 || protocol > RTNL_FAMILY_MAX);
rtnl_lock();
- tab = rtnl_msg_handlers[protocol];
+ tab = rtnl_dereference(rtnl_msg_handlers[protocol]);
if (!tab) {
rtnl_unlock();
return;
--
2.21.0.rc0.258.g878e2cd30e-goog
[PATCH v2 2/6] ixgbe: Fix incorrect RCU API usage
Recently, I added an RCU annotation check in rcu_assign_pointer. This
caused a sparse error to be reported by the ixgbe driver.
Further looking, it seems the adapter->xdp_prog pointer is not annotated
with __rcu. Annonating it fixed the error, but caused a bunch of other
warnings.
This patch tries to fix all warnings by using RCU API properly. This
makes sense to do because not using RCU properly can result in various
hard to find bugs. This is a best effort fix and is only build tested.
The sparse errors and warnings go away with the change. I request
maintainers / developers in this area to review / test it properly.
The sparse error fixed is:
ixgbe_main.c:10256:25: error: incompatible types in comparison expression
Signed-off-by: Joel Fernandes (Google)
---
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 15 ++-
2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 08d85e336bd4..3b14daf27516 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -311,7 +311,7 @@ struct ixgbe_ring {
struct ixgbe_ring *next;/* pointer to next ring in q_vector */
struct ixgbe_q_vector *q_vector; /* backpointer to host q_vector */
struct net_device *netdev; /* netdev ring belongs to */
- struct bpf_prog *xdp_prog;
+ struct bpf_prog __rcu *xdp_prog;
struct device *dev; /* device for DMA mapping */
void *desc; /* descriptor ring memory */
union {
@@ -560,7 +560,7 @@ struct ixgbe_adapter {
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
/* OS defined structs */
struct net_device *netdev;
- struct bpf_prog *xdp_prog;
+ struct bpf_prog __rcu *xdp_prog;
struct pci_dev *pdev;
struct mii_bus *mii_bus;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index daff8183534b..408a312aa6ba 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -2199,7 +2199,7 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter
*adapter,
u32 act;
rcu_read_lock();
- xdp_prog = READ_ONCE(rx_ring->xdp_prog);
+ xdp_prog = rcu_dereference(rx_ring->xdp_prog);
if (!xdp_prog)
goto xdp_out;
@@ -6547,7 +6547,7 @@ int ixgbe_setup_rx_resources(struct ixgbe_adapter
*adapter,
rx_ring->queue_index) < 0)
goto err;
- rx_ring->xdp_prog = adapter->xdp_prog;
+ rcu_assign_pointer(rx_ring->xdp_prog, adapter->xdp_prog);
return 0;
err:
@@ -10246,7 +10246,8 @@ static int ixgbe_xdp_setup(struct net_device *dev,
struct bpf_prog *prog)
if (nr_cpu_ids > MAX_XDP_QUEUES)
return -ENOMEM;
- old_prog = xchg(>xdp_prog, prog);
+ old_prog = rcu_access_pointer(adapter->xdp_prog);
+ rcu_assign_pointer(adapter->xdp_prog, prog);
/* If transitioning XDP modes reconfigure rings */
if (!!prog != !!old_prog) {
@@ -10271,13 +10272,17 @@ static int ixgbe_xdp_setup(struct net_device *dev,
struct bpf_prog *prog)
static int ixgbe_xdp(struct net_device *dev, struct netdev_bpf *xdp)
{
struct ixgbe_adapter *adapter = netdev_priv(dev);
+ struct bpf_prog *prog;
switch (xdp->command) {
case XDP_SETUP_PROG:
return ixgbe_xdp_setup(dev, xdp->prog);
case XDP_QUERY_PROG:
- xdp->prog_id = adapter->xdp_prog ?
- adapter->xdp_prog->aux->id : 0;
+ rcu_read_lock();
+ prog = rcu_dereference(adapter->xdp_prog);
+ xdp->prog_id = prog ? prog->aux->id : 0;
+ rcu_read_unlock();
+
return 0;
case XDP_QUERY_XSK_UMEM:
return ixgbe_xsk_umem_query(adapter, >xsk.umem,
--
2.21.0.rc0.258.g878e2cd30e-goog
[PATCH v2 0/6] RCU fixes for rcu_assign_pointer() usage
These patches fix various sparse errors found as a result of the recent check to add rcu_check_sparse() to rcu_assign_pointer(). The errors in some cases seem to either missing API usage, or missing annotations. The annotations added in the series can also help avoid future incorrect usages and bugs so it is a good idea to do in any case. RFC v1 -> Patch v2: Made changes to various scheduler patches (Peter Zijlstra) Joel Fernandes (Google) (6): net: rtnetlink: Fix incorrect RCU API usage ixgbe: Fix incorrect RCU API usage sched/cpufreq: Annotate cpufreq_update_util_data pointer with __rcu sched_domain: Annotate RCU pointers properly rcuwait: Annotate task_struct with __rcu sched: Annotate perf_domain pointer with __rcu drivers/net/ethernet/intel/ixgbe/ixgbe.h | 4 ++-- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 15 ++- include/linux/rcuwait.h | 2 +- include/linux/sched/topology.h| 4 ++-- kernel/sched/cpufreq.c| 2 +- kernel/sched/sched.h | 18 +- kernel/sched/topology.c | 10 +- net/core/rtnetlink.c | 4 ++-- 8 files changed, 32 insertions(+), 27 deletions(-) -- 2.21.0.rc0.258.g878e2cd30e-goog
RE: [PATCH 2/3] net: ethernet: add c45 PHY support in MDIO read/write functions.
Regards, Parshuram Thombare >-Original Message- >From: Andrew Lunn >Sent: Saturday, February 23, 2019 3:12 AM >To: Parshuram Raju Thombare >Cc: nicolas.fe...@microchip.com; da...@davemloft.net; >net...@vger.kernel.org; f.faine...@gmail.com; hkallwe...@gmail.com; linux- >ker...@vger.kernel.org; Rafal Ciepiela ; Piotr Sroka >; Jan Kotas >Subject: Re: [PATCH 2/3] net: ethernet: add c45 PHY support in MDIO read/write >functions. > >EXTERNAL MAIL > > >On Fri, Feb 22, 2019 at 08:12:42PM +, Parshuram Thombare wrote: >> This patch modify MDIO read/write functions to support communication >> with C45 PHY in Cadence ethernet controller driver. > >Hi Parshuram > >Are all versions of the MDIO controller capable of doing C45? > >Andrew Now driver support c22 and c45 PHY. Are you suggesting to add check for C45 PHY using is_c45 in phydev ? Regards, Parshuram Thombare
RE: [PATCH 3/3] net: ethernet: add support for high speed mac and usxgmii pcs
>> if (macb_is_gem(bp)) {
>> -linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
>> -if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
>> -
> linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> - phydev->supported);
>> +if (bp->caps & MACB_CAPS_HIGH_SPEED) {
>> +linkmode_copy(phydev->supported,
>PHY_10GBIT_FEATURES);
>> +} else {
>> +u32 bitmask =
>ETHTOOL_LINK_MODE_2500baseT_Full_BIT;
>> +
>> +linkmode_copy(phydev->supported,
>PHY_GBIT_FEATURES);
>> +if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
>> +linkmode_set_bit(bitmask, phydev->supported);
>> +}
>
>Same issue again. Somebody could be using a 10G MAC with a 2.5G PHY.
>
> Andrew
Hi Andrew,
Ok, I think this should have been logical AND. I will modify to use
phy_set_max_speed()
instead of directly copying linkmodes.
Regards,
Parshuram Thombare
RE: [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed
>> /* mask with MAC supported features */
>> -if (macb_is_gem(bp) && bp->caps &
>MACB_CAPS_GIGABIT_MODE_AVAILABLE)
>> -phy_set_max_speed(phydev, SPEED_1000);
>> -else
>> -phy_set_max_speed(phydev, SPEED_100);
>> +if (macb_is_gem(bp)) {
>> +linkmode_copy(phydev->supported, PHY_GBIT_FEATURES);
>> +if (bp->caps & MACB_CAPS_TWO_PT_FIVE_GIG_SPEED)
>> +
> linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
>> + phydev->supported);
>> +} else {
>> +linkmode_copy(phydev->supported, PHY_BASIC_FEATURES);
>> +}
>> +
>> +linkmode_copy(phydev->advertising, phydev->supported);
>
>This is not correct. Just because the MAC can do 2.5G does not mean the PHY
>can. So you should not be adding links modes. Also, somebody might be using a
>PHY that can do 2.5G with a MAC which can only do 1G.
>
>The correct thing to do is call phy_set_max_speed() with the maximum speed the
>MAC can do.
Hi Andrew,
Ok, I think this should have been logical AND. I will modify to use
phy_set_max_speed()
instead of directly copying linkmodes.
Regards,
Parshuram Thombare
Re: [PATCH net-next] net: dsa: add missing of_node_put
On 22/02/19 8:06 PM, Andrew Lunn wrote: On Fri, Feb 22, 2019 at 04:48:18PM +0530, Himadri Pandya wrote: Decrement the reference count on port while returning out of the loop. Issue identified by Coccinelle. You and Wen Yang are both fixing the same issue. Maybe you can coordinate? Sure. - Himadri Andrew
Re: [PATCH 05/17] perf data: Add perf_data__(create_dir|close_dir) functions
On Thu, Feb 21, 2019 at 6:42 PM Jiri Olsa wrote:
>
> Adding perf_data__create_dir to create nr files inside
> struct perf_data path directory:
> int perf_data__create_dir(struct perf_data *data, int nr);
>
> and function to close that data:
> void perf_data__close_dir(struct perf_data *data);
>
> Link: http://lkml.kernel.org/n/tip-kl4s1f13cg6wycrg367p8...@git.kernel.org
> Signed-off-by: Jiri Olsa
> ---
> tools/perf/util/data.c | 47 ++
> tools/perf/util/data.h | 8 +++
> 2 files changed, 55 insertions(+)
>
> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index 18e5d94b0e97..7f13fbd9c043 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -7,11 +7,58 @@
> #include
> #include
> #include
> +#include
>
> #include "data.h"
> #include "util.h"
> #include "debug.h"
>
> +static void close_dir(struct perf_data_file *files, int nr)
> +{
> + while (--nr >= 1) {
> + close(files[nr].fd);
> + free(files[nr].path);
> + }
> + free(files);
> +}
> +
> +void perf_data__close_dir(struct perf_data *data)
> +{
> + close_dir(data->dir.files, data->dir.nr);
> +}
> +
> +int perf_data__create_dir(struct perf_data *data, int nr)
> +{
> + struct perf_data_file *files = NULL;
> + int i, ret = -1;
> +
> + files = malloc(nr * sizeof(*files));
> + if (!files)
> + return -ENOMEM;
> +
> + data->dir.files = files;
> + data->dir.nr= nr;
> +
> + for (i = 0; i < nr; i++) {
> + struct perf_data_file *file = [i];
> +
> + if (asprintf(>path, "%s/data.%d", data->path, i) < 0)
> + goto out_err;
> +
> + ret = open(file->path, O_RDWR|O_CREAT|O_TRUNC,
> S_IRUSR|S_IWUSR);
> + if (ret < 0)
> + goto out_err;
It seems you need to free the file->path here.
Thanks,
Namhyung
> +
> + file->fd = ret;
> + }
> +
> + return 0;
> +
> +out_err:
> + close_dir(files, i);
> + return ret;
> +}
> +
> static bool check_pipe(struct perf_data *data)
> {
> struct stat st;
> diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
> index 2bce28117ccf..2d0d015a7d4d 100644
> --- a/tools/perf/util/data.h
> +++ b/tools/perf/util/data.h
> @@ -21,6 +21,11 @@ struct perf_data {
> bool is_pipe;
> bool force;
> enum perf_data_mode mode;
> +
> + struct {
> + struct perf_data_file *files;
> + int nr;
> + } dir;
> };
>
> static inline bool perf_data__is_read(struct perf_data *data)
> @@ -64,4 +69,7 @@ ssize_t perf_data_file__write(struct perf_data_file *file,
> int perf_data__switch(struct perf_data *data,
>const char *postfix,
>size_t pos, bool at_exit);
> +
> +int perf_data__create_dir(struct perf_data *data, int nr);
> +void perf_data__close_dir(struct perf_data *data);
> #endif /* __PERF_DATA_H */
> --
> 2.17.2
>
Re: [PATCH 0/3] Support inline symbols in callchains
On Fri, Feb 22, 2019 at 1:07 AM Jonas Rabenstein
wrote:
>
> Hi,
> This patchset supersedes my previous attempt to add inline symbols to
> callchain of perf-script [1] by a more generic attempt to not hook in
> the output stage but directly into the callchain generation. By a matter
> of fact this adds those inline symbols automatically to other commands
> like perf-report.
> Additionally this fixes the regression reported by Jiri Olsa [2] that
> some entries from previous outputs had been vanished and now only new
> lines are added if symbols had been found.
>
> The integration for perf-report is not completely done as there is an
> issue if the root for an hist_entry as for inlined symbols there may be
> multiple instances (for each address-range) with the same name. But in
> util/sort.c:233 only the name is compared for inlined symbols. As a
> consequence the returned hist_entry may hold a reference to another
> instance for this inlined symbol (with another address range than
> requested) we later on fail with -ERANGE in __symbol__inc_addr_samples
> (util/annotate.c:857).
> This issue does still permit perf-report to be executed without any
> problems and the inlined symbols do show up but none of the samples is
> actually accounted to them but to the original symbol :(
>
> To further provide information what this changeset is doing, here is a
> script-session to show the differences in the output:
>
> [jonas@x60s]$ git reset --hard v5.0-rc7; make -C tools/perf >/dev/null; \
> HEAD is now at a3b22b9f11d9 Linux 5.0-rc7
> [jonas@x60s]$ git am *.patch; make -C tools/perf >/dev/null; \
> mv tools /perf/perf perf-new
> Applying: perf map: add function to lookup inline symbols
> Applying: perf machine: use map__inlines in append_inlines
> Applying: perf machine: add inline symbols to callchains
> [jonas@x60s]$ cat test.c
> static int foo(int m){int r=0; for(int i=0;i static int bar(int m){int r=0; for(int i=0;i static int baz(int m){int r=0; for(int i=0;i r;}
> int main() { return baz(421); }
> [jonas@x60s]$ gcc -O2 -fno-omit-frame-pointer -g -o test test.c
> [jonas@x60s]$ ./perf-new record --call-graph fp ./test.c
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.002 MB perf.data (19 samples) ]
> [jonas@x60s]$ time ./perf-old script >old.script
> real0m0.039s
> user0m0.012s
> sys 0m0.022s
> [jonas@x60s]$ time ./perf-new script >new.script
> real0m0.045s
> user0m0.010s
> sys 0m0.032s
> [jonas@x60s]$ diff -u old.script new.script
> --- old.script 2019-02-21 16:30:47.997194180 +0100
> +++ new.script 2019-02-21 16:30:51.757309790 +0100
> @@ -23,66 +23,101 @@
> 7f43b1148090 _start+0x0 (/lib/x86_64-linux-gnu/ld-2.27.so)
>
> test 7579 1470536.968092: 354758 cycles:uppp:
> + 7f43b1148f4b elf_get_dynamic_info+0xab (inlined)
> 7f43b1148f4b _dl_start+0xab (/lib/x86_64-linux-gnu/ld-2.27.so)
> 7f43b1148098 _dl_start_user+0x0 (/lib/x86_64-linux-gnu/ld-2.27.so)
> [...]
>
> test 7579 1470536.969210:1922435 cycles:uppp:
> + 563f0df9c53f foo+0x4f (inlined)
> + 563f0df9c53f bar+0x4f (inlined)
> + 563f0df9c53f baz+0x4f (inlined)
> 563f0df9c53f main+0x4f (/home/jonas/linux/test)
> 7f43b0d77b97 __libc_start_main+0xe7
> (/lib/x86_64-linux-gnu/libc-2.27.so)
> 75e258d4c544155 [unknown] ([unknown])
> [jonas@x60s]$ time ./perf-new script --no-inline >new.noinline.script
> real0m0.035s
> user0m0.012s
> sys 0m0.020s
> [jonas@x60s]$ diff -u old.script new.noinline.script
> [jonas@x60s]$ ./perf-old report --stdio --quiet | sed '/^$/Q'
>88.80%88.80% test test [.] main
> |
> ---0x75e258d4c544155
>__libc_start_main
>main
> [jonas@x60s]$ ./perf-new report --stdio --quiet | sed '/^$/Q'
> 88.80%88.80% test test [.] main
> |
> ---0x75e258d4c544155
>__libc_start_main
>main
>baz (inlined)
>bar (inlined)
>foo (inlined)
> [jonas@x60s]$ ./perf-new report --stdio --quiet --no-inline | sed '/^$/Q'
> 88.80%88.80% test test [.] main
> |
> ---0x75e258d4c544155
>__libc_start_main
>main
>
> I am still trying to find a way that in the new output of report the
> 88.80% 'Self' are not accounted to main itself but split up for the
> inlined baz, bar and foo symbols.
>
> I'm open to any help as well as feedback,
I guess you can set al->sym to a symbol of the last callchain entry in
the iter_add_single_cumulative_entry(). For annotation, al->addr
needs to be updated from the start of the inlined subroutine if DWARF
is used, but not sure for addr2line though.
Thanks,
Namhyung
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, 22 Feb 2019 15:56:20 -0800
Alexei Starovoitov wrote:
> On Fri, Feb 22, 2019 at 03:16:35PM -0800, Linus Torvalds wrote:
> >
> > So a kernel pointer value of 0x12345678 could be a value kernel
> > pointer pointing to some random kmalloc'ed kernel memory, and a user
> > pointer value of 0x12345678 could be a valid _user_ pointer pointing
> > to some user mapping.
> >
> > See?
> >
> > If you access a user pointer, you need to use a user accessor function
> > (eg "get_user()"), while if you access a kernel pointer you need to
> > just dereference it directly (unless you can't trust it, in which case
> > you need to use a _different_ accessor function).
>
> that was clear already.
> Reading 0x12345678 via probe_kernel_read can return valid value
> and via get_user() can return another valid value on _some_ architectures.
>
> > The fact that user and kernel pointers happen to be distinct on x86-64
> > (right now) is just a random implementation detail.
>
> yes and my point that people already rely on this implementation detail.
> Say we implement
> int bpf_probe_read(void *val, void *unsafe_ptr)
> {
> if (probe_kernel_read(val, unsafe_ptr) == OK) {
> return 0;
> } else (get_user(val, unsafe_ptr) == OK) {
> return 0;
> } else {
> *val = 0;
> return -EFAULT;
> }
> }
Note that we can not use get_user() form kprobe handler. If you use it,
you have to prepare fault_handler() and make bpf itself can be aborted.
So, maybe you can use probe_user_read().
Hmm, however, it still doesn't work correctly on "some" architecture,
since whether a pointer (address) points user-space or kernel-space
depends on the context. In kprobe/bpf, the context means where you
put the probe and which pointer you record.
I think only "__user" tag tells us which one is user-space. But
unfortunately, that "__user" tag is only for compiler or checker, not
for runtime binary. Such useful attribute goes away when we execute it.
So, even if we introduce "ustring", ftrace/perf users has to decide to use
it by themselves. As far as I know, DWARF(debuginfo) also doesn't have
that attribute. So perf-probe can not help it from debuginfo.
(Maybe if we introduce C parser, it might be detected...)
> It will preserve existing bpf_probe_read() behavior on x86.
> If x86 implementation changes tomorrow then progs that read user
> addresses may start failing randomly because first probe_kernel_read()
> will be returning random values from kernel memory and that's no good,
> but at least we won't be breaking them today, so we have time to
> introduce bpf_user_read and bpf_kernel_read and folks have time to adopt them.
I see. I think bpf also has to introduce new bpf_probe_read_user() and
keep bpf_probe_read() for kernel dataa only.
> Imo that's much better than making current bpf_probe_read() fail
> on user addresses today and not providing a non disruptive path forward.
Agreed.
Thank you,
--
Masami Hiramatsu
[RFC PATCH] printk: Introduce "store now but print later" prefix.
There are callers which print many lines in $header_text_line $body_text_lines $trailer_text_line format, and some of them print $body_text_lines part from atomic context (or context where spending too much time can result in a lockup). SysRq-t (which might call printk() on all threads from interrupt context) and the OOM killer (which might call printk() on all threads from RCU critical section, and which cannot make forward progress by sending SIGKILL to OOM victims unless all printk() completes) are such callers. For such callers, storing all printk() messages to logbuf as soon as possible and then start actual printing to consoles (after e.g. leaving critical section, sending SIGKILL to OOM victims) is preferable. There has been an effort to offload actual printing to a dedicated kernel thread. But such offloading is not yet accepted due to "print out printk() messages as soon as possible" versus "don't lockup the system by spending too much time inside printk()" collision. Therefore, I propose per printk() flag while utilizing currently available synchronous printing logic. This patch tries to address "don't lockup the system" with minimal risk of failing to "print out printk() messages", by allowing printk() callers to tell printk() "store $body_text_lines lines into logbuf but start actual printing after $trailer_text_line line is stored into logbuf". This patch is different from existing printk_deferred(), for printk_deferred() is intended for scheduler/timekeeping use only. Moreover, what this patch wants to do is "do not try to print out printk() messages as soon as possible", for accumulated stalling period cannot be decreased if printk_deferred() from e.g. dump_tasks() from out_of_memory() immediately prints out the messages. The point of this patch is to defer the stalling duration to after leaving the critical section. Hereafter, I call printk() with "store now but print later" as "async" printk(), and printk() with "store now and print now" as "sync" printk(). Since storing $body_text_lines into logbuf using "async" printk() becomes very fast (latency is reduced from some milliseconds to some microseconds), the risk of failing to loose $body_text_lines lines which would have been printed out if "sync" printk() were used is small. Since actual printing starts as soon as a "sync" printk() is called (which might be from a CPU which is not processing a series of "async" printk()), the risk of failing to print out critical messages won't increase provided that printk() for critical messages (e.g. oops) are "sync" printk(). The possibility of failing to store all printk() messages to logbuf might be increased by using "async" printk(). But since we have a lot of RAM nowadays, allocating large logbuf enough to hold the entire SysRq-t output using log_buf_len= kernel command line parameter won't be difficult. There is a possibility that a thread which called "sync" printk() stalls for unexpectedly long period due to previously called a lot of "async" printk(). Such thread is expected to be the thread itself who did a lot of "async" printk(), and accumulated stalling period will be there even if that thread did "sync" printk(). This patch counts on console_waiter in console_trylock_spinning() for transferring control back from a random thread which did an innocent "sync" printk() to a thread which did a lot of "async" printk() by recommending such thread to use "sync" printk() for $trailer_text_line. Such expectation may fail, but it can't be solved until we get a really asynchronous printing using lockless consoles. Until that time, let's try whether failing such expectation leads to horrible consequences. If it turns out to be horrible consequences, we could add hints for judging which thread (console_owner or console_waiter) should continue printing (e.g. prefer schedulable process context over atomic process context, prefer atomic process context over soft IRQ context, prefer soft IRQ context over hard IRQ context, and/or track number of records stored by "async" printk() into "struct task_struct" and compare it at console_trylock_spinning()). If that still fails, we can temporarily disable "async" printk() inside vprintk_emit(). Signed-off-by: Tetsuo Handa --- include/linux/kern_levels.h | 18 ++ include/linux/printk.h | 28 kernel/printk/internal.h| 4 ++-- kernel/printk/printk.c | 16 +--- kernel/printk/printk_safe.c | 2 +- 5 files changed, 62 insertions(+), 6 deletions(-) diff --git a/include/linux/kern_levels.h b/include/linux/kern_levels.h index bf2389c..dc924a3 100644 --- a/include/linux/kern_levels.h +++ b/include/linux/kern_levels.h @@ -23,6 +23,24 @@ */ #define KERN_CONT KERN_SOH "c" +/* + * Annotation for a "store now but print later" line of log printout. + * Such lines will be printed when printk() without this annotation is called. + * Users of this annotation are encouraged to do
Re: [PATCH 3/3] perf machine: add inline symbols to callchains
Hello,
On Fri, Feb 22, 2019 at 1:07 AM Jonas Rabenstein
wrote:
>
> Use map__inlines to resolve inlined functions for every entry with
> an symbol that should be added to a callchain.
>
> Signed-off-by: Jonas Rabenstein
> ---
> tools/perf/util/machine.c | 115 --
> 1 file changed, 87 insertions(+), 28 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index dce29c21e4ea..070d074482b4 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1908,6 +1908,91 @@ struct iterations {
> u64 cycles;
> };
>
> +static int __add_callchain_location(struct callchain_cursor *cursor,
> + struct symbol **parent,
> + struct addr_location *root_al,
> + u64 addr, struct addr_location *al,
> + bool branch, struct branch_flags *flags,
> + u64 branch_from, struct iterations *iter)
> +{
> + int nr_loop_iter = 0;
> + u64 iter_cycles = 0;
> +
> + if (symbol_conf.hide_unresolved && al->sym == NULL)
> + return 0;
> +
> + if (al->sym) {
> + if (perf_hpp_list.parent && !*parent &&
> + symbol__match_regex(al->sym, _regex))
> + *parent = al->sym;
> + else if (have_ignore_callees && root_al &&
> + symbol__match_regex(al->sym, _callees_regex)) {
> + /* Treat this symbol as the root,
> + forgetting its callees. */
> + *root_al = *al;
> + callchain_cursor_reset(cursor);
> + }
> + }
> +
> + if (iter) {
> + nr_loop_iter = iter->nr_loop_iter;
> + iter_cycles = iter->cycles;
> + }
> + return callchain_cursor_append(cursor, addr, al->map, al->sym, branch,
> + flags, nr_loop_iter, iter_cycles,
> + branch_from, al->srcline);
> +}
> +
> +static int __add_callchain_ip(struct callchain_cursor *cursor, u64 ip,
> + struct addr_location *al, bool branch,
> + struct branch_flags *flags, u64 branch_from,
> + struct iterations *iter, struct symbol **parent,
> + struct addr_location *root_al)
> +{
> + struct inline_node *inline_node;
> + struct inline_list *inline_list;
> + const char *srcline;
> + struct symbol *symbol;
> + int err = 0;
> +
> + al->srcline = callchain_srcline(al->map, al->sym, al->addr);
> + if (callchain_param.order == ORDER_CALLER)
> + err = __add_callchain_location(cursor, parent, root_al, ip,
> al,
> + branch, flags, branch_from,
> iter);
> + if (err || !al->map || !al->sym)
> + goto no_inline;
> +
> + inline_node = map__inlines(al->map, ip, al->sym);
> + if (!inline_node || list_empty(_node->val))
> + goto no_inline;
> +
> + symbol = al->sym;
> + srcline = al->srcline;
> + list_for_each_entry(inline_list, _node->val, list) {
> + if (inline_list->symbol == symbol)
> + continue;
> + al->sym = inline_list->symbol;
> + al->srcline = inline_list->srcline;
> + err = __add_callchain_location(cursor, parent, root_al, ip,
> + al, branch, flags,
> + branch_from, iter);
> + if (err)
> + break;
> + }
Does this loop do the job both for ORDER_CALLER and ORDER_CALLEE?
Thanks,
Namhyung
> +
> + if (callchain_param.order == ORDER_CALLEE) {
> + al->srcline = srcline;
> + al->sym = symbol;
> + }
> +
> +no_inline:
> + if (!err && callchain_param.order == ORDER_CALLEE)
> + err = __add_callchain_location(cursor, parent, root_al, ip,
> al,
> + branch, flags, branch_from,
> iter);
> + return err;
> +}
> +
> +
> static int add_callchain_ip(struct thread *thread,
> struct callchain_cursor *cursor,
> struct symbol **parent,
> @@ -1920,9 +2005,6 @@ static int add_callchain_ip(struct thread *thread,
> u64 branch_from)
> {
> struct addr_location al;
> - int nr_loop_iter = 0;
> - u64 iter_cycles = 0;
> - const char *srcline = NULL;
>
> al.filtered = 0;
> al.sym = NULL;
> @@ -1955,31 +2037,8 @@ static int add_callchain_ip(struct thread *thread,
> thread__find_symbol(thread, *cpumode, ip, );
> }
>
> -
[PATCH] Bluetooth: hci_ldisc: Postpone HCI_UART_PROTO_READY bit set in hci_uart_set_proto()
task A:task B:
hci_uart_set_proto flush_to_ldisc
- p->open(hu) -> h5_open //alloc h5 - receive_buf
- set_bit HCI_UART_PROTO_READY - tty_port_default_receive_buf
- hci_uart_register_dev - tty_ldisc_receive_buf
- hci_uart_tty_receive
- test_bit HCI_UART_PROTO_READY
- h5_recv
- clear_bit HCI_UART_PROTO_READY while() {
- p->open(hu) -> h5_close //free h5
- h5_rx_3wire_hdr
- h5_reset() //use-after-free
}
It could use ioctl to set hci uart proto, but there is
a use-after-free issue when hci_uart_register_dev() fail in
hci_uart_set_proto(), see stack above, fix this by setting
HCI_UART_PROTO_READY bit only when hci_uart_register_dev()
return success.
Reported-by: syzbot+899a33dc0fa0dbaf0...@syzkaller.appspotmail.com
Signed-off-by: Kefeng Wang
---
drivers/bluetooth/hci_ldisc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 4918fefc4a6f..9562e72c1ae5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -696,14 +696,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
return -EPROTONOSUPPORT;
hu->proto = p;
- set_bit(HCI_UART_PROTO_READY, >flags);
err = hci_uart_register_dev(hu);
if (err) {
- clear_bit(HCI_UART_PROTO_READY, >flags);
return err;
}
+ set_bit(HCI_UART_PROTO_READY, >flags);
return 0;
}
--
2.20.1
Re: X450LCP lost abillity to turn the screen off
Hi João, On 2/13/19 7:46 PM, Marcos Paulo de Souza wrote: Hello João, On 2/12/19 2:30 PM, João Paulo Rechi Vita wrote: On Mon, Feb 11, 2019 at 6:31 PM Marcos Paulo de Souza wrote: Hello João, On 2/11/19 5:14 PM, João Paulo Rechi Vita wrote: Hello Marcos, On Sun, Feb 10, 2019 at 5:05 PM Marcos Paulo de Souza wrote: On 2/10/19 9:45 PM, Andy Shevchenko wrote: On Sun, Feb 10, 2019 at 9:24 PM Marcos Paulo de Souza wrote: Hi, Since 5.0.0-rc4 I vefiried that my ASUS laptop Can you be more specific, what model, BIOS version, etc (also would be nice to have dmi strings from it, I guess dmidecode tool would help). dmidecode attached. cannot turn the screen of anymore. There were several commits in 5.0 merge window touching this functionality like: 71b12beaf12f platform/x86: asus-nb-wmi: Drop mapping of 0x33 and 0x34 scan codes b3f2f3799a97 platform/x86: asus-nb-wmi: Map 0x35 to KEY_SCREENLOCK 78f3ac76d9e5 platform/x86: asus-wmi: Tell the EC the OS will handle the display off hotkey Can you bisect or just try to revert one-by-one from above and see which one is a culprit? I already did some primary analysis, and it seems the commit 3f2f3799a97 maps the x035 (which is Alt+f7 in my laptop) to SCREENLOCK, which is wrong because alt+f7 should be Screen Toggle. I will try to revert this commit, or remap to KEY_DISPLAYTOGGLE or KEY_DISPLAY_OFF, and test if it works. User-space does not act on KEY_DISPLAYTOGGLE / KEY_DISPLAY_OFF, these values should be used when the hardware is turning the screen back-light ON and OFF. According to Asus BIOS engineers, the back-light used to be driven by the hardware, but they have changed to the this new approach of telling the OS to drive the back-light for a while now (no specific dates or BIOS / windows driver versions were shared). They we actually surprised when we told the that some machines still have a working implementation (and selected by default unless told otherwise) of the old behavior, which sounds like it is the case for the machine you have at hand. The new behavior, as defined in their spec is to only notify the OS of the keypress with 0x35, and have the OS "close" the screen, with the screen being "opened" on mouse or keyboard activity. This closely matches the screen lock behavior on Linux platforms, so we are mapping it to KEY_SCREENLOCK in the kernel, and it then gets mapped to XF86ScreenSaver by xkeyboard-config, and finally gnome-settings-daemon uses it as a lock screen shortcut (look for "screensaver" in plugins/media-keys/shortcuts-list.h on the gnome-settings-daemon repository). Interesting. But yes, I'll do my best to track the problem ASAP at my side. Please let me know if I can provide any additional information. You can check what is being sent by the kernel with evtest, and what is being sent by X with "xinput test " (and you can find the device id with "xinput list"). And you can re-map it without having to rebuild the kernel using udev's hwdb. But simply re-mapping should not change anything, since userspace does not act on KEY_DISPLAYTOGGLE / KEY_DISPLAY_OFF. If you want to switch back to the old behavior you need to revert "78f3ac76d9e5 platform/x86: asus-wmi: Tell the EC the OS will handle the display off hotkey". I tried reverting the patch and only recompiling/reinstalling the platform/x86 modules, but the problem still happens. My next step will be testing agains't 4.20, since my machine was working with 4.12, so I might try the major releases first. So maybe your desktop environment (KDE) acts on KEY_DISPLAYTOGGLE / KEY_DISPLAY_OFF and this is the only reason why this was working in the first place? It would be sad to find out different DEs behave differently in this situation, but IMO include/uapi/linux/input-event-codes.h is not super clear about whether userspace should act on these values or they are just intended to notify userspace of a change so desktop notifications (like an OSD) can be shown. If that is the case you will need to revert all 3 commits you listed earlier. Also, make sure to check with evtest which values are being sent by the kernel to make sure the correct code is being executed. I think you found the issue. Tried GNOME in the same machine, with the same kernel, and it works. I can revert those 3 commits and try again with a different DE, if you think it would help. Let me elaborate it better: * When using KDE, pressing Alt+F7, KDE only locks the screen * When using GNOME, pressing Alt+F7, GNOME locks the screen and turns the screen off. I hope it explains better what happened. Thanks. Thanks. That being said, I believe it would be more productive to figure out why your userspace stack is not reacting to 0x35 / XF86ScreenSaver and fix that. Which window manager / graphical desktop environment are you using? Well, I'm using KDE Plasma 5 Desktop Environment (20170319-lp150.7.1) of openSUSE Leap 15.0. As a final note, from your dmidecode output I
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, 22 Feb 2019 09:43:14 -0800
Linus Torvalds wrote:
> On Fri, Feb 22, 2019 at 12:35 AM Masami Hiramatsu wrote:
> >
> > Or, can we do this?
> >
> > long __probe_user_read(void *dst, const void *src, size_t size)
> > {
>
> Add a
>
> if (!access_ok(src, size))
> ret = -EFAULT;
> else {
> .. do the pagefault_disable() etc ..
> }
Since kprobes handler runs in IRQ context, we can not use access_ok() in it.
(only on x86 + CONFIG_DEBUG_ATOMIC_SLEEP=y)
In arch/x86/include/asm/uaccess.h:
#define access_ok(addr, size) \
({ \
WARN_ON_IN_IRQ(); \
likely(!__range_not_ok(addr, size, user_addr_max())); \
})
Do we need acccess_ok_inatomic()?
BTW, it seems a bit strange that this WARN_ON_IN_IRQ() is only in x86
access_ok() implementation, since CONFIG_DEBUG_ATOMIC_SLEEP(which defines
WARN_ON_IN_IRQ) doesn't depend on x86, and access_ok() is widely used
in kernel. I think it would be better that each arch provides __access_ok()
and include/linux/uaccess.h provides access_ok() with WARN_ON_IN_IRQ().
> to after the "set_fs()", and it looks good to me. Make it clear that
> yes, this works _only_ for user reads.
>
> Adn that makes all the games with "kernel_uaccess_faults_ok"
> pointless, so you can just remove them.
OK.
>
> (note that the "access_ok()" has to come after we've done "set_fs()",
> because it takes the address limit from that).
>
> Also, since normally we'd expect that we already have USER_DS, it
> might be worthwhile to do this with a wrapper, something along the
> lines of
>
> mm_segment_t old_fs = get_fs();
>
> if (segment_eq(old_fs, USER_DS))
> return __normal_probe_user_read();
> set_fs(USER_DS);
> ret = __normal_probe_user_read();
> set_fs(old_fs);
> return ret;
>
> and have that __normal_probe_user_read() just do
>
> if (!access_ok(src, size))
> return -EFAULT;
> pagefault_disable();
> ret = __copy_from_user_inatomic(dst, ...);
> pagefault_enable();
> return ret ? -EFAULT : 0;
>
> which looks more obvious.
OK.
>
> Also, I would suggest that you just make the argument type be "const
> void __user *", since the whole point is that this takes a user
> pointer, and nothing else.
Ah, right.
> Then we should still probably fix up "__probe_kernel_read()" to not
> allow user accesses. The easiest way to do that is actually likely to
> use the "unsafe_get_user()" functions *without* doing a
> uaccess_begin(), which will mean that modern CPU's will simply fault
> on a kernel access to user space.
Or, use __chk_user_ptr(ptr) to check it?
Thank you,
>
> The nice thing about that is that usually developers will have access
> to exactly those modern boxes, so the people who notice that it
> doesn't work are the right people.
>
> Alternatively, we should just make it be architecture-specific, so
> that architectures can decide "this address cannot be a kernel
> address" and refuse to do it.
>
> Linus
--
Masami Hiramatsu
[PATCH V4 2/2] dt-bindings: imx: add new resources to scu resource table
Add new resources as below according to latest system controller firmware for new features: IMX_SC_R_PERF IMX_SC_R_OCRAM IMX_SC_R_DMA_5_CH0 IMX_SC_R_DMA_5_CH1 IMX_SC_R_DMA_5_CH2 IMX_SC_R_DMA_5_CH3 IMX_SC_R_ATTESTATION Signed-off-by: Anson Huang Reviewed-by: Rob Herring --- No changes since V3. --- include/dt-bindings/firmware/imx/rsrc.h | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/include/dt-bindings/firmware/imx/rsrc.h b/include/dt-bindings/firmware/imx/rsrc.h index d69f934..4e61f64 100644 --- a/include/dt-bindings/firmware/imx/rsrc.h +++ b/include/dt-bindings/firmware/imx/rsrc.h @@ -36,6 +36,7 @@ #define IMX_SC_R_DC_0_BLIT120 #define IMX_SC_R_DC_0_BLIT221 #define IMX_SC_R_DC_0_BLIT_OUT 22 +#define IMX_SC_R_PERF 23 #define IMX_SC_R_DC_0_WARP 25 #define IMX_SC_R_DC_0_VIDEO0 28 #define IMX_SC_R_DC_0_VIDEO1 29 @@ -322,7 +323,7 @@ #define IMX_SC_R_IRQSTR_SCU2 321 #define IMX_SC_R_IRQSTR_DSP322 #define IMX_SC_R_ELCDIF_PLL323 -#define IMX_SC_R_UNUSED6 324 +#define IMX_SC_R_OCRAM 324 #define IMX_SC_R_AUDIO_PLL_0 325 #define IMX_SC_R_PI_0 326 #define IMX_SC_R_PI_0_PWM_0327 @@ -539,6 +540,11 @@ #define IMX_SC_R_VPU_MU_3 538 #define IMX_SC_R_VPU_ENC_1 539 #define IMX_SC_R_VPU 540 -#define IMX_SC_R_LAST 541 +#define IMX_SC_R_DMA_5_CH0 541 +#define IMX_SC_R_DMA_5_CH1 542 +#define IMX_SC_R_DMA_5_CH2 543 +#define IMX_SC_R_DMA_5_CH3 544 +#define IMX_SC_R_ATTESTATION 545 +#define IMX_SC_R_LAST 546 #endif /* __DT_BINDINGS_RSCRC_IMX_H */ -- 2.7.4
[PATCH V2 2/2] arm64: dts: freescale: imx8mq: add clock for GPIO node
i.MX8MQ has clock gate for each GPIO bank, add clock info to GPIO node for clock management. Signed-off-by: Anson Huang --- No change since V1, just drop 1 patch from V1 patch series. --- arch/arm64/boot/dts/freescale/imx8mq.dtsi | 5 + 1 file changed, 5 insertions(+) diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi index 9155bd4..4c1fca9 100644 --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi @@ -160,6 +160,7 @@ reg = <0x3020 0x1>; interrupts = , ; + clocks = < IMX8MQ_CLK_GPIO1_ROOT>; gpio-controller; #gpio-cells = <2>; interrupt-controller; @@ -171,6 +172,7 @@ reg = <0x3021 0x1>; interrupts = , ; + clocks = < IMX8MQ_CLK_GPIO2_ROOT>; gpio-controller; #gpio-cells = <2>; interrupt-controller; @@ -182,6 +184,7 @@ reg = <0x3022 0x1>; interrupts = , ; + clocks = < IMX8MQ_CLK_GPIO3_ROOT>; gpio-controller; #gpio-cells = <2>; interrupt-controller; @@ -193,6 +196,7 @@ reg = <0x3023 0x1>; interrupts = , ; + clocks = < IMX8MQ_CLK_GPIO4_ROOT>; gpio-controller; #gpio-cells = <2>; interrupt-controller; @@ -204,6 +208,7 @@ reg = <0x3024 0x1>; interrupts = , ; + clocks = < IMX8MQ_CLK_GPIO5_ROOT>; gpio-controller; #gpio-cells = <2>; interrupt-controller; -- 2.7.4
[PATCH V2 1/2] clk: imx8mq: add GPIO clocks to clock tree
i.MX8MQ has clock gate for each GPIO bank, add them
into clock tree for GPIO driver to manage.
Signed-off-by: Anson Huang
---
No change since V1, just drop 1 patch from V1 patch series.
---
drivers/clk/imx/clk-imx8mq.c | 5 +
include/dt-bindings/clock/imx8mq-clock.h | 8 +++-
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index 26b57f4..553dcf5 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -500,6 +500,11 @@ static int imx8mq_clocks_probe(struct platform_device
*pdev)
clks[IMX8MQ_CLK_ECSPI2_ROOT] = imx_clk_gate4("ecspi2_root_clk",
"ecspi2", base + 0x4080, 0);
clks[IMX8MQ_CLK_ECSPI3_ROOT] = imx_clk_gate4("ecspi3_root_clk",
"ecspi3", base + 0x4090, 0);
clks[IMX8MQ_CLK_ENET1_ROOT] = imx_clk_gate4("enet1_root_clk",
"enet_axi", base + 0x40a0, 0);
+ clks[IMX8MQ_CLK_GPIO1_ROOT] = imx_clk_gate4("gpio1_root_clk",
"ipg_root", base + 0x40b0, 0);
+ clks[IMX8MQ_CLK_GPIO2_ROOT] = imx_clk_gate4("gpio2_root_clk",
"ipg_root", base + 0x40c0, 0);
+ clks[IMX8MQ_CLK_GPIO3_ROOT] = imx_clk_gate4("gpio3_root_clk",
"ipg_root", base + 0x40d0, 0);
+ clks[IMX8MQ_CLK_GPIO4_ROOT] = imx_clk_gate4("gpio4_root_clk",
"ipg_root", base + 0x40e0, 0);
+ clks[IMX8MQ_CLK_GPIO5_ROOT] = imx_clk_gate4("gpio5_root_clk",
"ipg_root", base + 0x40f0, 0);
clks[IMX8MQ_CLK_GPT1_ROOT] = imx_clk_gate4("gpt1_root_clk", "gpt1",
base + 0x4100, 0);
clks[IMX8MQ_CLK_I2C1_ROOT] = imx_clk_gate4("i2c1_root_clk", "i2c1",
base + 0x4170, 0);
clks[IMX8MQ_CLK_I2C2_ROOT] = imx_clk_gate4("i2c2_root_clk", "i2c2",
base + 0x4180, 0);
diff --git a/include/dt-bindings/clock/imx8mq-clock.h
b/include/dt-bindings/clock/imx8mq-clock.h
index 04f7ac3..aff3335 100644
--- a/include/dt-bindings/clock/imx8mq-clock.h
+++ b/include/dt-bindings/clock/imx8mq-clock.h
@@ -391,5 +391,11 @@
#define IMX8MQ_CLK_NAND_USDHC_BUS_RAWNAND_CLK 267
-#define IMX8MQ_CLK_END 268
+#define IMX8MQ_CLK_GPIO1_ROOT 268
+#define IMX8MQ_CLK_GPIO2_ROOT 269
+#define IMX8MQ_CLK_GPIO3_ROOT 270
+#define IMX8MQ_CLK_GPIO4_ROOT 271
+#define IMX8MQ_CLK_GPIO5_ROOT 272
+
+#define IMX8MQ_CLK_END 273
#endif /* __DT_BINDINGS_CLOCK_IMX8MQ_H */
--
2.7.4
[PATCH V4 1/2] dt-bindings: imx: remove unused resources from scu resource table
Removes below resources which were defined during pre-silicon phase and the real silicons do NOT have them, they have never been used, latest system controller firmware also removed them: IMX_SC_R_DC_0_CAPTURE0 IMX_SC_R_DC_0_CAPTURE1 IMX_SC_R_DC_0_INTEGRAL0 IMX_SC_R_DC_0_INTEGRAL1 IMX_SC_R_DC_0_FRAC1 IMX_SC_R_DC_1_CAPTURE0 IMX_SC_R_DC_1_CAPTURE1 IMX_SC_R_DC_1_INTEGRAL0 IMX_SC_R_DC_1_INTEGRAL1 IMX_SC_R_DC_1_FRAC1 IMX_SC_R_GPU_3_PID0 IMX_SC_R_M4_0_SIM IMX_SC_R_M4_0_WDOG IMX_SC_R_M4_1_SIM IMX_SC_R_M4_1_WDOG Signed-off-by: Anson Huang --- Changes since V3: - just remove those unused resource and no need to define UNUSED resource. -- include/dt-bindings/firmware/imx/rsrc.h | 15 --- 1 file changed, 15 deletions(-) diff --git a/include/dt-bindings/firmware/imx/rsrc.h b/include/dt-bindings/firmware/imx/rsrc.h index 4481f2d..d69f934 100644 --- a/include/dt-bindings/firmware/imx/rsrc.h +++ b/include/dt-bindings/firmware/imx/rsrc.h @@ -36,15 +36,10 @@ #define IMX_SC_R_DC_0_BLIT120 #define IMX_SC_R_DC_0_BLIT221 #define IMX_SC_R_DC_0_BLIT_OUT 22 -#define IMX_SC_R_DC_0_CAPTURE0 23 -#define IMX_SC_R_DC_0_CAPTURE1 24 #define IMX_SC_R_DC_0_WARP 25 -#define IMX_SC_R_DC_0_INTEGRAL026 -#define IMX_SC_R_DC_0_INTEGRAL127 #define IMX_SC_R_DC_0_VIDEO0 28 #define IMX_SC_R_DC_0_VIDEO1 29 #define IMX_SC_R_DC_0_FRAC030 -#define IMX_SC_R_DC_0_FRAC131 #define IMX_SC_R_DC_0 32 #define IMX_SC_R_GPU_2_PID033 #define IMX_SC_R_DC_0_PLL_034 @@ -53,17 +48,11 @@ #define IMX_SC_R_DC_1_BLIT137 #define IMX_SC_R_DC_1_BLIT238 #define IMX_SC_R_DC_1_BLIT_OUT 39 -#define IMX_SC_R_DC_1_CAPTURE0 40 -#define IMX_SC_R_DC_1_CAPTURE1 41 #define IMX_SC_R_DC_1_WARP 42 -#define IMX_SC_R_DC_1_INTEGRAL043 -#define IMX_SC_R_DC_1_INTEGRAL144 #define IMX_SC_R_DC_1_VIDEO0 45 #define IMX_SC_R_DC_1_VIDEO1 46 #define IMX_SC_R_DC_1_FRAC047 -#define IMX_SC_R_DC_1_FRAC148 #define IMX_SC_R_DC_1 49 -#define IMX_SC_R_GPU_3_PID050 #define IMX_SC_R_DC_1_PLL_051 #define IMX_SC_R_DC_1_PLL_152 #define IMX_SC_R_SPI_0 53 @@ -303,8 +292,6 @@ #define IMX_SC_R_M4_0_UART 287 #define IMX_SC_R_M4_0_I2C 288 #define IMX_SC_R_M4_0_INTMUX 289 -#define IMX_SC_R_M4_0_SIM 290 -#define IMX_SC_R_M4_0_WDOG 291 #define IMX_SC_R_M4_0_MU_0B292 #define IMX_SC_R_M4_0_MU_0A0 293 #define IMX_SC_R_M4_0_MU_0A1 294 @@ -323,8 +310,6 @@ #define IMX_SC_R_M4_1_UART 307 #define IMX_SC_R_M4_1_I2C 308 #define IMX_SC_R_M4_1_INTMUX 309 -#define IMX_SC_R_M4_1_SIM 310 -#define IMX_SC_R_M4_1_WDOG 311 #define IMX_SC_R_M4_1_MU_0B312 #define IMX_SC_R_M4_1_MU_0A0 313 #define IMX_SC_R_M4_1_MU_0A1 314 -- 2.7.4
[PATCH] gpio: mxc: add check to return defer probe if clock tree NOT ready
On i.MX8MQ platform, clock driver uses platform driver
model and it is probed after GPIO driver, so when GPIO
driver fails to get clock, it should check the error type
to decide whether to return defer probe or just ignore
the clock operation.
Fixes: 2808801aab8a ("gpio: mxc: add clock operation")
Signed-off-by: Anson Huang
---
drivers/gpio/gpio-mxc.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 2d1dfa1..e86e61d 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -438,8 +438,11 @@ static int mxc_gpio_probe(struct platform_device *pdev)
/* the controller clock is optional */
port->clk = devm_clk_get(>dev, NULL);
- if (IS_ERR(port->clk))
+ if (IS_ERR(port->clk)) {
+ if (PTR_ERR(port->clk) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
port->clk = NULL;
+ }
err = clk_prepare_enable(port->clk);
if (err) {
--
2.7.4
Re: [PATCH] perf hist: fix memory leak if histogram entry is reused
Hi, On Fri, Feb 22, 2019 at 1:29 AM Jiri Olsa wrote: > > On Thu, Feb 21, 2019 at 02:53:20PM +0100, Jonas Rabenstein wrote: > > On Thu, Feb 21, 2019 at 01:39:09PM +0100, Jiri Olsa wrote: > > > On Thu, Feb 21, 2019 at 01:23:06PM +0100, Jonas Rabenstein wrote: > > > > In __hists__add_entry the srcline of the addr_location is duplicated > > > > for the hist_entry. If hists__findnew_entry returns an already existing > > > > hist_entry the srcline has to be freed again as no further reference to > > > > that duplicated srcline would exists anymore. > > > > > > > > Signed-off-by: Jonas Rabenstein > > > > > > > > --- > > > > tools/perf/util/hist.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > > > > index 8aad8330e392..25b8dbdbbe87 100644 > > > > --- a/tools/perf/util/hist.c > > > > +++ b/tools/perf/util/hist.c > > > > @@ -623,6 +623,9 @@ __hists__add_entry(struct hists *hists, > > > > .ops = ops, > > > > }, *he = hists__findnew_entry(hists, , al, sample_self); > > > > > > > > + if (he && he->srcline != entry.srcline) > > > > + free(entry.srcline); > > > > + > > > > if (!hists->has_callchains && he && he->callchain_size != 0) > > > > hists->has_callchains = true; > > > > return he; > > > > > > nice, do we have similar issue in here? > > > > > > jirka > > > > > > > > > --- > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c > > > index 216388003dea..e65e6822c848 100644 > > > --- a/tools/perf/util/hist.c > > > +++ b/tools/perf/util/hist.c > > > @@ -966,7 +966,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter > > > *iter, > > > .map = al->map, > > > .sym = al->sym, > > > }, > > > - .srcline = al->srcline ? strdup(al->srcline) : NULL, > > > + .srcline = al->srcline, > > While this shouldn't leak the memory, we may end up with an al->srcline > > to get free afterwards while still having a reference on it within the > > hist_entry... Also I could not find where/how the hist_entry is freed up > > so we may get an double free if both of al and he clean the srcline. > > Of course, with your solution we could spare the useless strdup/free if > > we find an hist_entry (which should be the typical case for hotspots..). > > Taking a deeper look thus should be beneficial - but I do not have the > > time for that right now because I'm still working on the inline-symbol > > integration which is more important for me... > > ok, I'll check it I think we can defer calling strdup() to hist_entry__init(). Thanks, Namhyung
Re: [PATCH v8 05/26] clocksource: Add driver for the Ingenic JZ47xx OST
Hi, Le jeu. 24 janv. 2019 à 19:53, Paul Cercueil a écrit : Le jeu. 24 janv. 2019 à 19:46, Stephen Boyd a écrit : Quoting Paul Cercueil (2019-01-24 12:46:28) Le jeu. 24 janv. 2019 à 16:28, Stephen Boyd a écrit : > Quoting Guenter Roeck (2019-01-23 10:01:55) >> On Wed, Jan 23, 2019 at 02:25:53PM -0300, Paul Cercueil wrote: >> > Hi, >> > >> > Le mer. 23 janv. 2019 à 11:31, Guenter Roeck >> a écrit : >> > >On 1/23/19 4:58 AM, Mathieu Malaterre wrote: >> > >>On Wed, Dec 12, 2018 at 11:09 PM Paul Cercueil >> >> > >>wrote: >> > >>> >> > >>>From: Maarten ter Huurne >> > >>> >> > >>>OST is the OS Timer, a 64-bit timer/counter with buffered >> reading. >> > >>> >> > >>>SoCs before the JZ4770 had (if any) a 32-bit OST; the JZ4770 >> and >> > >>>JZ4780 have a 64-bit OST. >> > >>> >> > >>>This driver will register both a clocksource and a sched_clock >> to the >> > >>>system. >> > >>> >> > >>>Signed-off-by: Maarten ter Huurne >> > >>>Signed-off-by: Paul Cercueil >> > >>>--- >> > >>> >> > >>>Notes: >> > >>> v5: New patch >> > >>> >> > >>> v6: - Get rid of SoC IDs; pass pointer to >> ingenic_ost_soc_info >> > >>>as >> > >>>devicetree match data instead. >> > >>> - Use device_get_match_data() instead of the of_* >> variant >> > >>> - Handle error of dev_get_regmap() properly >> > >>> >> > >>> v7: Fix section mismatch by using >> > >>>builtin_platform_driver_probe() >> > >>> >> > >>> v8: builtin_platform_driver_probe() does not work >> anymore in >> > >>> 4.20-rc6? The probe function won't be called. Work >> around >> > >>>this >> > >>> for now by using late_initcall. >> > >>> >> > > >> > >Did anyone notice this ? Either something is wrong with the >> driver, or >> > >with the kernel core. Hacking around it seems like the worst >> possible >> > >"solution". >> > >> > I can confirm it still happens on 5.0-rc3. >> > >> > Just to explain what I'm doing: >> > >> > My ingenic-timer driver probes with builtin_platform_driver_probe >> (this >> > works), >> > and then calls of_platform_populate to probe its children. This >> driver, >> > ingenic-ost, is one of them, and will fail to probe with >> > builtin_platform_driver_probe. >> > >> >> The big question is _why_ it fails to probe. >> > > Are you sharing the device tree node between a 'normal' platform > device > driver and something more low level DT that marks the device's backing > DT node as OF_POPULATED early on? That's my only guess why it's not > working. I do, but I clear the OF_POPULATED flag so that it is then probed as a normal platform device, and it's not on this driver's node but its parent. Where do you clear the OF_POPULATED flag? In the ingenic-timer driver introduced in patch [04/26], inside the probe function. Anything new on this? It still happens on 5.0-rc7. It probes with late_initcall, and not with device_initcall. I have no clue what's going on. -Paul
RE: [PATCH 1/3] clk: imx8mq: initialize clock tree earlier
Best Regards!
Anson Huang
> -Original Message-
> From: Anson Huang
> Sent: 2019年2月23日 11:04
> To: Lucas Stach ; robh...@kernel.org;
> mark.rutl...@arm.com; shawn...@kernel.org; s.ha...@pengutronix.de;
> ker...@pengutronix.de; feste...@gmail.com; mturque...@baylibre.com;
> sb...@kernel.org; Abel Vesa ; ccai...@baylibre.com;
> a...@sigxcpu.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> c...@vger.kernel.org
> Cc: dl-linux-imx
> Subject: RE: [PATCH 1/3] clk: imx8mq: initialize clock tree earlier
>
> Hi, Lucas
>
> Best Regards!
> Anson Huang
>
> > -Original Message-
> > From: Lucas Stach [mailto:l.st...@pengutronix.de]
> > Sent: 2019年2月22日 18:55
> > To: Anson Huang ; robh...@kernel.org;
> > mark.rutl...@arm.com; shawn...@kernel.org; s.ha...@pengutronix.de;
> > ker...@pengutronix.de; feste...@gmail.com; mturque...@baylibre.com;
> > sb...@kernel.org; Abel Vesa ;
> ccai...@baylibre.com;
> > a...@sigxcpu.org; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> > c...@vger.kernel.org
> > Cc: dl-linux-imx
> > Subject: Re: [PATCH 1/3] clk: imx8mq: initialize clock tree earlier
> >
> > Am Freitag, den 22.02.2019, 09:42 + schrieb Anson Huang:
> > > Currently on i.MX8MQ platform, clock driver is probed later than
> > > GPIO driver, and GPIO driver does NOT have defer probe mechanism
> > > since the GPIO clock is optional,
> >
> > So this is what should be fixed. If there is a clock reference in the
> > DT, having the clock driver ready is not optional. Optional to the
> > GPIO driver just means it won't fail if there is no clock reference at all.
> > If there is and the clock provider is not yet there, it needs to do a
> > PROBE_DEFER.
> >
> > So that's a NACK on this patch.
>
> OK, I see. Then I will do a patch for GPIO driver, and I just check other
> drivers,
> looks like imx2 watchdog driver also has similar issue, I will do a patch too.
Just double check the watch dog driver, its clock is NOT optional, and just
return
the error value, so it should be OK, sorry for the noise.
Anson.
>
> Thanks,
> Anson.
>
> >
> > Regards,
> > Lucas
> >
> > > some platforms have GPIO clocks and some are NOT. So it is an issue
> > > that on i.MX8MQ platform, there are GPIO clocks defined, but due to
> > > clock tree is NOT ready during GPIO driver probe, the GPIO clock
> > > management will fail and cause system hang if GPIO clocks are OFF by
> > > default.
> > >
> > > This patch changes the i.MX8MQ clock tree initialization using
> > > CLK_OF_DECLARE instead of platform driver model to make clock tree
> > > ready earlier than GPIO driver.
> > >
> > > Signed-off-by: Anson Huang
> > > ---
> > > drivers/clk/imx/clk-imx8mq.c | 33 +++--
> > > 1 file changed, 7 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-
> > > imx8mq.c index 26b57f4..2df1575 100644
> > > --- a/drivers/clk/imx/clk-imx8mq.c
> > > +++ b/drivers/clk/imx/clk-imx8mq.c
> > > @@ -269,10 +269,9 @@ static const char *imx8mq_clko2_sels[] =
> > > {"osc_25m", "sys2_pll_200m", "sys1_pll_
> > >
> > > static struct clk_onecell_data clk_data;
> > >
> > > -static int imx8mq_clocks_probe(struct platform_device *pdev)
> > > +static void __init imx8mq_clocks_init(struct device_node *np)
> > > {
> > > - struct device *dev = >dev;
> > > - struct device_node *np = dev->of_node;
> > > + struct device_node *anatop_np;
> > > void __iomem *base;
> > > int err;
> > > int i;
> > > @@ -286,10 +285,10 @@ static int imx8mq_clocks_probe(struct
> > > platform_device *pdev)
> > > clks[IMX8MQ_CLK_EXT3] = of_clk_get_by_name(np, "clk_ext3");
> > > clks[IMX8MQ_CLK_EXT4] = of_clk_get_by_name(np, "clk_ext4");
> > >
> > > - np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-anatop");
> > > - base = of_iomap(np, 0);
> > > + anatop_np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-
> > > anatop");
> > > + base = of_iomap(anatop_np, 0);
> > > if (WARN_ON(!base))
> > > - return -ENOMEM;
> > > + return;
> > >
> > > clks[IMX8MQ_ARM_PLL_REF_SEL] = imx_clk_mux("arm_pll_ref_sel",
> > base +
> > > 0x28, 16, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> > > clks[IMX8MQ_GPU_PLL_REF_SEL] = imx_clk_mux("gpu_pll_ref_sel",
> > base +
> > > 0x18, 16, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)); @@ -389,10
> > > +388,9 @@ static int imx8mq_clocks_probe(struct platform_device
> > > +*pdev)
> > > clks[IMX8MQ_SYS2_PLL_500M] =
> > > imx_clk_fixed_factor("sys2_pll_500m", "sys2_pll_out", 1, 2);
> > > clks[IMX8MQ_SYS2_PLL_1000M] =
> > > imx_clk_fixed_factor("sys2_pll_1000m", "sys2_pll_out", 1, 1);
> > >
> > > - np = dev->of_node;
> > > base = of_iomap(np, 0);
> > > if (WARN_ON(!base))
> > > - return -ENOMEM;
> > > + return;
> > >
> > > /* CORE */
> > > clks[IMX8MQ_CLK_A53_SRC] = imx_clk_mux2("arm_a53_src", base +
> > >
[GIT PULL v3] Hyper-V commits for 5.0-rc
-BEGIN PGP SIGNED MESSAGE- Hash: SHA512 The following changes since commit 8834f5600cf3c8db365e18a3d5cac2c2780c81e5: Linux 5.0-rc5 (2019-02-03 13:48:04 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed for you to fetch changes up to b2946cd86f3c1b5b1262c0ec06068110c2328fe6: MAINTAINERS: Change mailing list for Hyper-V CORE AND DRIVERS (2019-02-22 21:46:37 -0500) - Two fixes: 1. A fix for a race condition reading sysfs entries while a device is being added, by Kimberly Brown. 2. Update the Hyper-V mailing list to a new one created on vger.kernel.org, by Haiyang Zhang. - Haiyang Zhang (1): MAINTAINERS: Change mailing list for Hyper-V CORE AND DRIVERS Kimberly Brown (2): Drivers: hv: vmbus: Change server monitor_pages index to 0 Drivers: hv: vmbus: Return -EINVAL if monitor_allocated not set Documentation/ABI/stable/sysfs-bus-vmbus | 15 +--- MAINTAINERS | 2 +- drivers/hv/vmbus_drv.c | 39 +++- 3 files changed, 51 insertions(+), 5 deletions(-) -BEGIN PGP SIGNATURE- iQIzBAEBCgAdFiEE4n5dijQDou9mhzu83qZv95d3LNwFAlxwucgACgkQ3qZv95d3 LNxkUg/9GmnREx3P9dvQECTeT/ynuL1kbA6lhbwolimrEjdBcEiPRRhD1UKOW5lb lwBvp7Qj3+rnVBzI1+950VZoDCsmhCHS2DJ+Iy6OKPASYxCHXfSKfG0BWoid56ar VTjG93gY9MmfUv/SkiAE/lYRdhGEM5GwojzPwKZ88+bV7gsgSHnFLeUgf/C5FXfG PhvviKyV9S4MW5OwTcboNaHNSaS1Wf0EeOE3iHoXiE8Ylj3aJswrCgJWbIc9koQF +KcugIIMnzTIhrKsxe9wmisAVAdaJo33suVOSgS50xnsVmIiJYtSgZ93/jH1GCXO 0rGuu1Qs/1dzzbZN5heQNm1KIlvpEaudG8pTQFssb3nmzyTPVFMqIb5pPungWpus h4n/4DHg3pDgZXeTfygqogSkysWwfCoaiBymWfUYEbYJjg0zxgdrGbQvzowu5nOZ mCBGxhrjJLDCMovkZKZggxutzIL+YTpJjPYmxZgNqnNdGHt9HR3syg142ZC3yXNT ALZzQVXNk6cmCoTr9q/G3qbaS9Sv7BwMqC8YwGsqC5QbEP3WuKPBC7ifdXtA9nuo fgAk2g3+vYlRK4kDUhl8EsTWMkH6HDNMe6Hzn4nyl01Qg36lyEwCoY1C32HFg8RR Roi7nWz6IyQfJixYbkgVznhjHAuFZgIRS307a/n5vmwyYny1VZI= =h1IS -END PGP SIGNATURE-
Re: block: be more careful about status in __bio_chain_endio
I'm also worried about the other two versions, though:
memory-barriers.txt#1724:
1724 (*) The compiler is within its rights to invent stores to a variable,
i.e. the compiler is free to decide __bio_chain_endio looks like this:
static struct bio *__bio_chain_endio(struct bio *bio)
{
struct bio *parent = bio->bi_private;
blk_status_t tmp = parent->bi_status;
parent->bi_status = bio->bi_status;
if (!bio->bi_status)
parent->bi_status = tmp;
bio_put(bio);
return parent;
}
In which case, the read and later store on the two different threads
may overlap in such a way that bio_endio sometimes sees success, even
if one child had an error.
As a result, I believe the setting of parent->bi_status needs to be a
WRITE_ONCE() and the later reading needs to be a READ_ONCE()
[although, since the later reading happens in many different
functions, perhaps some other barrier to make sure all readers get the
correct value is in order.]
[GIT PULL] SCSI fixes for 5.0-rc7
Four small fixes: three in drivers and one in the core. The core fix
is also minor in scope since the bug it fixes is only known to affect
systems using SCSI reservations. Of the driver bugs, the libsas one is
the most major because it can lead to multiple disks on the same
expander not being exposed.
The patch is available here:
git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes
The short changelog is:
Anoob Soman (1):
scsi: libiscsi: Fix race between iscsi_xmit_task and iscsi_complete_task
John Garry (1):
scsi: libsas: Fix rphy phy_identifier for PHYs with end devices attached
Martin Wilck (1):
scsi: core: reset host byte in DID_NEXUS_FAILURE case
Masato Suzuki (1):
scsi: sd_zbc: Fix sd_zbc_report_zones() buffer allocation
And the diffstat:
drivers/scsi/libiscsi.c| 6 ++
drivers/scsi/libsas/sas_expander.c | 2 ++
drivers/scsi/scsi_lib.c| 1 +
drivers/scsi/sd_zbc.c | 8 +---
4 files changed, 14 insertions(+), 3 deletions(-)
With full diff below.
James
---
diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index b8d325ce8754..120fc520f27a 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1459,7 +1459,13 @@ static int iscsi_xmit_task(struct iscsi_conn *conn)
if (test_bit(ISCSI_SUSPEND_BIT, >suspend_tx))
return -ENODATA;
+ spin_lock_bh(>session->back_lock);
+ if (conn->task == NULL) {
+ spin_unlock_bh(>session->back_lock);
+ return -ENODATA;
+ }
__iscsi_get_task(task);
+ spin_unlock_bh(>session->back_lock);
spin_unlock_bh(>session->frwd_lock);
rc = conn->session->tt->xmit_task(task);
spin_lock_bh(>session->frwd_lock);
diff --git a/drivers/scsi/libsas/sas_expander.c
b/drivers/scsi/libsas/sas_expander.c
index 17eb4185f29d..f21c93bbb35c 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -828,6 +828,7 @@ static struct domain_device *sas_ex_discover_end_dev(
rphy = sas_end_device_alloc(phy->port);
if (!rphy)
goto out_free;
+ rphy->identify.phy_identifier = phy_id;
child->rphy = rphy;
get_device(>dev);
@@ -854,6 +855,7 @@ static struct domain_device *sas_ex_discover_end_dev(
child->rphy = rphy;
get_device(>dev);
+ rphy->identify.phy_identifier = phy_id;
sas_fill_in_rphy(child, rphy);
list_add_tail(>disco_list_node,
>port->disco_list);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6d65ac584eba..f8d51c3d5582 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -655,6 +655,7 @@ static blk_status_t scsi_result_to_blk_status(struct
scsi_cmnd *cmd, int result)
set_host_byte(cmd, DID_OK);
return BLK_STS_TARGET;
case DID_NEXUS_FAILURE:
+ set_host_byte(cmd, DID_OK);
return BLK_STS_NEXUS;
case DID_ALLOC_FAILURE:
set_host_byte(cmd, DID_OK);
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index fff86940388b..a340af797a85 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -142,10 +142,12 @@ int sd_zbc_report_zones(struct gendisk *disk, sector_t
sector,
return -EOPNOTSUPP;
/*
-* Get a reply buffer for the number of requested zones plus a header.
-* For ATA, buffers must be aligned to 512B.
+* Get a reply buffer for the number of requested zones plus a header,
+* without exceeding the device maximum command size. For ATA disks,
+* buffers must be aligned to 512B.
*/
- buflen = roundup((nrz + 1) * 64, 512);
+ buflen = min(queue_max_hw_sectors(disk->queue) << 9,
+roundup((nrz + 1) * 64, 512));
buf = kmalloc(buflen, gfp_mask);
if (!buf)
return -ENOMEM;
RE: [PATCH 1/3] clk: imx8mq: initialize clock tree earlier
Hi, Lucas
Best Regards!
Anson Huang
> -Original Message-
> From: Lucas Stach [mailto:l.st...@pengutronix.de]
> Sent: 2019年2月22日 18:55
> To: Anson Huang ; robh...@kernel.org;
> mark.rutl...@arm.com; shawn...@kernel.org; s.ha...@pengutronix.de;
> ker...@pengutronix.de; feste...@gmail.com; mturque...@baylibre.com;
> sb...@kernel.org; Abel Vesa ; ccai...@baylibre.com;
> a...@sigxcpu.org; devicet...@vger.kernel.org; linux-arm-
> ker...@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> c...@vger.kernel.org
> Cc: dl-linux-imx
> Subject: Re: [PATCH 1/3] clk: imx8mq: initialize clock tree earlier
>
> Am Freitag, den 22.02.2019, 09:42 + schrieb Anson Huang:
> > Currently on i.MX8MQ platform, clock driver is probed later than GPIO
> > driver, and GPIO driver does NOT have defer probe mechanism since the
> > GPIO clock is optional,
>
> So this is what should be fixed. If there is a clock reference in the DT,
> having
> the clock driver ready is not optional. Optional to the GPIO driver just means
> it won't fail if there is no clock reference at all.
> If there is and the clock provider is not yet there, it needs to do a
> PROBE_DEFER.
>
> So that's a NACK on this patch.
OK, I see. Then I will do a patch for GPIO driver, and I just check other
drivers, looks
like imx2 watchdog driver also has similar issue, I will do a patch too.
Thanks,
Anson.
>
> Regards,
> Lucas
>
> > some platforms have GPIO clocks and some are NOT. So it is an issue
> > that on i.MX8MQ platform, there are GPIO clocks defined, but due to
> > clock tree is NOT ready during GPIO driver probe, the GPIO clock
> > management will fail and cause system hang if GPIO clocks are OFF by
> > default.
> >
> > This patch changes the i.MX8MQ clock tree initialization using
> > CLK_OF_DECLARE instead of platform driver model to make clock tree
> > ready earlier than GPIO driver.
> >
> > Signed-off-by: Anson Huang
> > ---
> > drivers/clk/imx/clk-imx8mq.c | 33 +++--
> > 1 file changed, 7 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-
> > imx8mq.c index 26b57f4..2df1575 100644
> > --- a/drivers/clk/imx/clk-imx8mq.c
> > +++ b/drivers/clk/imx/clk-imx8mq.c
> > @@ -269,10 +269,9 @@ static const char *imx8mq_clko2_sels[] =
> > {"osc_25m", "sys2_pll_200m", "sys1_pll_
> >
> > static struct clk_onecell_data clk_data;
> >
> > -static int imx8mq_clocks_probe(struct platform_device *pdev)
> > +static void __init imx8mq_clocks_init(struct device_node *np)
> > {
> > - struct device *dev = >dev;
> > - struct device_node *np = dev->of_node;
> > + struct device_node *anatop_np;
> > void __iomem *base;
> > int err;
> > int i;
> > @@ -286,10 +285,10 @@ static int imx8mq_clocks_probe(struct
> > platform_device *pdev)
> > clks[IMX8MQ_CLK_EXT3] = of_clk_get_by_name(np, "clk_ext3");
> > clks[IMX8MQ_CLK_EXT4] = of_clk_get_by_name(np, "clk_ext4");
> >
> > - np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-anatop");
> > - base = of_iomap(np, 0);
> > + anatop_np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-
> > anatop");
> > + base = of_iomap(anatop_np, 0);
> > if (WARN_ON(!base))
> > - return -ENOMEM;
> > + return;
> >
> > clks[IMX8MQ_ARM_PLL_REF_SEL] = imx_clk_mux("arm_pll_ref_sel",
> base +
> > 0x28, 16, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> > clks[IMX8MQ_GPU_PLL_REF_SEL] = imx_clk_mux("gpu_pll_ref_sel",
> base +
> > 0x18, 16, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels)); @@ -389,10
> > +388,9 @@ static int imx8mq_clocks_probe(struct platform_device *pdev)
> > clks[IMX8MQ_SYS2_PLL_500M] =
> > imx_clk_fixed_factor("sys2_pll_500m", "sys2_pll_out", 1, 2);
> > clks[IMX8MQ_SYS2_PLL_1000M] =
> > imx_clk_fixed_factor("sys2_pll_1000m", "sys2_pll_out", 1, 1);
> >
> > - np = dev->of_node;
> > base = of_iomap(np, 0);
> > if (WARN_ON(!base))
> > - return -ENOMEM;
> > + return;
> >
> > /* CORE */
> > clks[IMX8MQ_CLK_A53_SRC] = imx_clk_mux2("arm_a53_src", base +
> > 0x8000, 24, 3, imx8mq_a53_sels, ARRAY_SIZE(imx8mq_a53_sels)); @@
> > -568,22 +566,5 @@ static int imx8mq_clocks_probe(struct
> > platform_device *pdev)
> >
> > err = of_clk_add_provider(np, of_clk_src_onecell_get, _data);
> > WARN_ON(err);
> > -
> > - return err;
> > }
> > -
> > -static const struct of_device_id imx8mq_clk_of_match[] = {
> > - { .compatible = "fsl,imx8mq-ccm" },
> > - { /* Sentinel */ },
> > -};
> > -MODULE_DEVICE_TABLE(of, imx8mq_clk_of_match);
> > -
> > -
> > -static struct platform_driver imx8mq_clk_driver = {
> > - .probe = imx8mq_clocks_probe,
> > - .driver = {
> > - .name = "imx8mq-ccm",
> > - .of_match_table = of_match_ptr(imx8mq_clk_of_match),
> > - },
> > -};
> > -module_platform_driver(imx8mq_clk_driver);
> > +CLK_OF_DECLARE(imx8mq, "fsl,imx8mq-ccm", imx8mq_clocks_init);
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, 22 Feb 2019 18:28:53 -0800 Alexei Starovoitov wrote: > First we introduce bpf_probe_kernel_read and bpf_probe_user_read and > introduce clang/gcc tooling to catch the mistakes. > Going over this 400+ places and manually grepping kernel sources > for __user keyword is not a great proposal if we want to keep those users. > Once we have this working we can remove bpf_probe_read() altogether. > Rejecting bpf prog at load time is a clear signal that user has to fix it > (instead of changing run-time behavior). > When the verifier gets even smarter it could potentially replace prob_read > with probe_kernel_read and probe_user_read when it has that type info. I was about to suggest this approach. Document that bpf_probe_read() is known to be buggy and will be deprecated in the future, and that all new bpf scripts should start using bpf_probe_kernel/user_read() instead (after they have been implemented of course). And give time for people to fix their current scripts. Perhaps in the near future, trigger some kind of warning for users that use bpf_probe_read(). -- Steve
Re: block: be more careful about status in __bio_chain_endio
On Fri, Feb 22 2019 at 9:02pm -0500, John Dorminy wrote: > I am perhaps not understanding the intricacies here, or not seeing a > barrier protecting it, so forgive me if I'm off base. I think reading > parent->bi_status here is unsafe. > Consider the following sequence of events on two threads. > > Thread 0 Thread 1 > In __bio_chain_endio:In __bio_chain_endio: > [A] Child 0 reads parent->bi_status, > no error. > Child bio 1 reads parent, no error > seen > It sets parent->bi_status to an error > It calls bio_put. > Child bio 0 calls bio_put > [end __bio_chain_endio] [end __bio_chain_endio] > In bio_chain_endio(), > bio_endio(parent) > is called, calling > bio_remaining_done() > which decrements __bi_remaining to 1 > and returns false, so no further > endio > stuff is done. > In bio_chain_endio(), bio_endio(parent) > is called, calling bio_remaining_done(), > decrementing parent->__bi_remaining to > 0, and continuing to finish parent. > Either for block tracing or for parent's > bi_end_io(), this thread tries to read > parent->bi_status again. > > The compiler or the CPU may cache the read from [A], and since there > are no intervening barriers, parent->bi_status is still believed on > thread 0 to be success. Thus the bio may still be falsely believed to > have completed successfully, even though child 1 set an error in it. > > Am I missing a subtlety here? Either neilb's original or even Jens' suggestion would be fine though. > if (!parent->bi_status && bio->bi_status) > parent->bi_status = bio->bi_status; Even if your scenario did play out (which I agree it looks possible) it'd just degenerate to neilb's version: > if (bio->bi_status) > parent->bi_status = bio->bi_status; Which also accomplishes fixing what Neil originally detailed in his patch header.
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, Feb 22, 2019 at 6:29 PM Alexei Starovoitov wrote: > > imo this kernel release should finish as-is and in the next cycle we can > change probe_kernel_read() to reject user address [..] Absolutely. Nothing is going to change right now for 5.0, which is imminent. It's really a "long-term we really need to fix this", where the only question is how soon "long-term" is. Linus
RE: [PATCH RESEND V6 2/2] clk: imx: scu: add cpu frequency scaling support
Hi, Stehpen
Best Regards!
Anson Huang
> -Original Message-
> From: Stephen Boyd [mailto:sb...@kernel.org]
> Sent: 2019年2月23日 3:08
> To: devicet...@vger.kernel.org; feste...@gmail.com;
> ker...@pengutronix.de; linux-arm-ker...@lists.infradead.org; linux-
> c...@vger.kernel.org; linux-kernel@vger.kernel.org; mark.rutl...@arm.com;
> mturque...@baylibre.com; robh...@kernel.org; s.ha...@pengutronix.de;
> shawn...@kernel.org; Aisheng Dong ; Anson
> Huang ; Daniel Baluta
> Cc: dl-linux-imx
> Subject: Re: [PATCH RESEND V6 2/2] clk: imx: scu: add cpu frequency scaling
> support
>
> Quoting Anson Huang (2019-02-21 18:32:10)
> > On NXP's i.MX SoCs with system controller inside, CPU frequency
> > scaling can ONLY be done by system controller firmware, and it can
> > ONLY be requested from secure mode, so Linux kernel has to call ARM
> > SMC to trap to ARM-Trusted-Firmware to request system controller
> > firmware to do CPU frequency scaling.
> >
> > This patch adds i.MX system controller CPU frequency scaling support,
> > it reuses cpufreq-dt driver and implement the CPU frequency scaling
> > inside SCU clock driver.
> >
> > Signed-off-by: Anson Huang
>
> Ah I missed one thing, see below.
>
> > @@ -180,6 +185,23 @@ static long clk_scu_round_rate(struct clk_hw *hw,
> unsigned long rate,
> > return rate;
> > }
> >
> > +static int clk_scu_atf_set_cpu_rate(struct clk_hw *hw, unsigned long rate,
> > + unsigned long parent_rate) {
> > + struct clk_scu *clk = to_clk_scu(hw);
> > + struct arm_smccc_res res;
> > + unsigned long cluster_id;
> > +
> > + if (clk->rsrc_id == IMX_SC_R_A35)
> > + cluster_id = 0;
>
> Do we still need to check this anymore? Why not just always use cluster_id 0?
The i.MX8QXP ONLY has 1 cluster named A35, while on i.MX8QM there will be
2 clusters, A53 and A72, so we need to use the resource ID to initialize the
cluster_id.
>
> > +
> > + /* CPU frequency scaling can ONLY be done by ARM-Trusted-Firmware
> */
> > + arm_smccc_smc(IMX_SIP_CPUFREQ, IMX_SIP_SET_CPUFREQ,
> > + cluster_id, rate, 0, 0, 0, 0, );
>
> Because not checking would make this work, vs. checking causes this code to
> sometimes use an uninitialized value from the stack.
89 + if (rsrc_id == IMX_SC_R_A35)
90 + init.ops = _scu_cpu_ops;
91 + else
92 + init.ops = _scu_ops;
I think it should be good. Only when plan to support cpu-freq scaling, then the
CPU clock will be switched to use clk_scu_cpu_ops and the clutser_id
initialization
will be done according to CPU resource. For example, when we plan to support
i.MX8QM
cpu-freq scaling, we will add A53 and A72 check here and switch the clock ops
to clk_scu_cpu_ops,
also we will add the cluster_id initialization in the SMC clock set rate.
Thanks,
Anson.
>
> > +
> > + return 0;
> > +}
> > +
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, Feb 22, 2019 at 04:08:59PM -0800, Linus Torvalds wrote: > On Fri, Feb 22, 2019 at 3:56 PM Alexei Starovoitov > wrote: > > > > It will preserve existing bpf_probe_read() behavior on x86. > > ... but that's the worst possible situation. > > It appears that people haven't understood that kernel and user > addresses are distinct, and may have written programs that are > fundamentally buggy. > > And we _want_ to make it clear that they are buggy on x86-64, exactly > because x86-64 is the one that gets the most testing - by far. > > So if x86-64 continues working for buggy programs, then that only > means that those bugs never get fixed. > > It would be much better to try to get those things fixed, and make the > x86-64 implementation stricter, exactly so that people end up > _realizing_ that they can't just think "a pointer is a pointer, and > the context doesn't matter". > > From a pure functional safety standpoint, I thought bpf already knew > what kind of a pointer it had? when bpf verifier knows the type of pointer it allows direct access to it. That's the case for skb, socket, packet data, hash maps, arrays, stack, etc. Networking progs cannot call bpf_probe_read(). It's available to tracing progs only and their goal is to walk the kernel and user memory with addresses that cannot be statically verified at program load time. We are working on adding type information (BTF) to vmlinux. Soon we'll be able to tell the type of every kernel function argument. Right now arg1 and arg2 in a kprobed function are just u64 pt_regs->di, si. Soon we'll be able to precisely identify their C type. I completely agree on the direction that x86 is the architecture that sets an example and users need to learn the difference in pointers. I only disagree on timing. Right now users don't have an alternative. In our repo I counted ~400 calls to bpf_probe_read and about 10 times more 'indirect' calls. What's happening with 'indirect' calls is BCC toolchain using clang to automatically replace struct_a->field_foo access with bpf_probe_read(struct_a + offsetof(typeof(struct_a), field_foo)); If we had __user vs __kernel annotation available to clang we could have taught BCC to replace this '->' dereference with appropriate kernel vs user helper. Also we need to teach GCC to recognize __user and store into dwarf, so we can take it further into BTF and verify later. Also I think disallowing bpf_probe_read() to read user pointer will not make a desired teaching effect. It will only cause painful debugging to folks when their progs will stop working. It's better to remove bpf_probe_read() completely. imo the process of teaching the users of kernel vs user pointer difference needs to be gradual. First we introduce bpf_probe_kernel_read and bpf_probe_user_read and introduce clang/gcc tooling to catch the mistakes. Going over this 400+ places and manually grepping kernel sources for __user keyword is not a great proposal if we want to keep those users. Once we have this working we can remove bpf_probe_read() altogether. Rejecting bpf prog at load time is a clear signal that user has to fix it (instead of changing run-time behavior). When the verifier gets even smarter it could potentially replace prob_read with probe_kernel_read and probe_user_read when it has that type info. imo this kernel release should finish as-is and in the next cycle we can change probe_kernel_read() to reject user address, have temporary workaround in bpf_probe_read() with probe_kernel_read+get_user hack, introduce new bpf helpers, new tooling and eventually remove buggy bpf_probe_read.
[PATCH v2] checkpatch: handle PCI/USB VID,PID in DT compatible
Documentation/devicetree/bindings/usb/usb-device.txt describes the
'usbVID,...' compatible format, where VID is lower-case hexadecimal,
with leading zeroes suppressed. Allow it here without complaining about
lack of documentation (we don't need a new entry for every ID).
PCI has a similar format
Documentation/devicetree/bindings/pci/pci.txt
http://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
For both, I try to detect something that's close to a VID,PID, but I
intentionally don't parse beyond 4 characters of PID, since USB supports
extending with an interface index, and PCI supports additional subystem
IDs.
Signed-off-by: Brian Norris
---
Overall, this got a little more unwieldy, but it works.
v2:
* Include PCI in addition to USB
* Add special warning for leading zeroes and for upper-case
* Move the VID,PID check up higher, so we don't complain about
documentation at all if using the correct format
---
scripts/checkpatch.pl | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b737ca9d7204..9e770a8f5dfa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3001,6 +3001,24 @@ sub process {
my $vp_file = $dt_path . "vendor-prefixes.txt";
foreach my $compat (@compats) {
+ my ($vendor, $device) = ("", "");
+ if ($compat =~ /^([a-zA-Z0-9\-]+)\,(.*)$/) {
+ ($vendor, $device) = ($1, $2);
+ }
+
+ # PCI and USB VIDs/PIDs have special rules.
+ if ($vendor =~ /^(usb|pci)[0-9a-fA-F]{1,4}$/ &&
$device =~ /^[0-9a-fA-F]{1,4}/) {
+ if ($vendor =~ /^(usb|pci).*[A-F]/ ||
$device =~ /[A-F]/) {
+ WARN("UNDOCUMENTED_DT_STRING",
+"VID/PID in DT compatible
string (\"$compat\") should use lower-case hexadecimal\n" . $herecurr);
+ }
+ if ($vendor =~ /^(usb|pci)0/ || $device
=~ /^0/) {
+ WARN("UNDOCUMENTED_DT_STRING",
+"VID/PID in DT compatible
string (\"$compat\") should omit leading zeroes\n" . $herecurr);
+ }
+ next;
+ }
+
my $compat2 = $compat;
$compat2 =~ s/\,[a-zA-Z0-9]*\-/\,<\.\*>\-/;
my $compat3 = $compat;
@@ -3011,8 +3029,7 @@ sub process {
"DT compatible string \"$compat\"
appears un-documented -- check $dt_path\n" . $herecurr);
}
- next if $compat !~ /^([a-zA-Z0-9\-]+)\,/;
- my $vendor = $1;
+ next if "$vendor" eq "";
`grep -Eq "^$vendor\\b" $vp_file`;
if ( $? >> 8 ) {
WARN("UNDOCUMENTED_DT_STRING",
--
2.21.0.rc0.258.g878e2cd30e-goog
Re: [GIT PULL] Power management fixes for final v5.0
The pull request you sent on Fri, 22 Feb 2019 10:42:40 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git pm-5.0 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/ef4edb3ed830cbbb443de9906b8cf16dc0653a74 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: block: be more careful about status in __bio_chain_endio
I am perhaps not understanding the intricacies here, or not seeing a barrier protecting it, so forgive me if I'm off base. I think reading parent->bi_status here is unsafe. Consider the following sequence of events on two threads. Thread 0 Thread 1 In __bio_chain_endio:In __bio_chain_endio: [A] Child 0 reads parent->bi_status, no error. Child bio 1 reads parent, no error seen It sets parent->bi_status to an error It calls bio_put. Child bio 0 calls bio_put [end __bio_chain_endio] [end __bio_chain_endio] In bio_chain_endio(), bio_endio(parent) is called, calling bio_remaining_done() which decrements __bi_remaining to 1 and returns false, so no further endio stuff is done. In bio_chain_endio(), bio_endio(parent) is called, calling bio_remaining_done(), decrementing parent->__bi_remaining to 0, and continuing to finish parent. Either for block tracing or for parent's bi_end_io(), this thread tries to read parent->bi_status again. The compiler or the CPU may cache the read from [A], and since there are no intervening barriers, parent->bi_status is still believed on thread 0 to be success. Thus the bio may still be falsely believed to have completed successfully, even though child 1 set an error in it. Am I missing a subtlety here?
[PATCH] regulator: da9062: Use struct_size() in devm_kzalloc()
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
struct boo entry[];
};
size = sizeof(struct foo) + count * sizeof(struct boo);
instance = alloc(size, GFP_KERNEL)
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = alloc(struct_size(instance, entry, count), GFP_KERNEL)
Notice that, in this case, variable size is not necessary, hence it is
removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
drivers/regulator/da9062-regulator.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/regulator/da9062-regulator.c
b/drivers/regulator/da9062-regulator.c
index 4c5d6ad853f8..b064d8a19d4c 100644
--- a/drivers/regulator/da9062-regulator.c
+++ b/drivers/regulator/da9062-regulator.c
@@ -992,7 +992,6 @@ static int da9062_regulator_probe(struct platform_device
*pdev)
struct regulator_config config = { };
const struct da9062_regulator_info *rinfo;
int irq, n, ret;
- size_t size;
int max_regulators;
switch (chip->chip_type) {
@@ -1010,9 +1009,8 @@ static int da9062_regulator_probe(struct platform_device
*pdev)
}
/* Allocate memory required by usable regulators */
- size = sizeof(struct da9062_regulators) +
- max_regulators * sizeof(struct da9062_regulator);
- regulators = devm_kzalloc(>dev, size, GFP_KERNEL);
+ regulators = devm_kzalloc(>dev, struct_size(regulators, regulator,
+ max_regulators), GFP_KERNEL);
if (!regulators)
return -ENOMEM;
--
2.20.1
Re: [GIT PULL] ARM: SoC fixes for 5.0
The pull request you sent on Fri, 22 Feb 2019 23:23:33 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git tags/armsoc-fixes has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/9053d2db8b04a468ce1ab92693b940b046ea392c Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [GIT PULL] ARC updates for 5.0 final
The pull request you sent on Fri, 22 Feb 2019 09:42:38 -0800: > git://git.kernel.org/pub/scm/linux/kernel/git/vgupta/arc.git/ > tags/arc-5.0-final has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/2cc63b39003913fdf564cde5c646ac8f174e3ac7 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [GIT PULL] parisc architecture fixes for kernel v5.0
The pull request you sent on Fri, 22 Feb 2019 09:17:25 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git > parisc-5.0-1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/8456e98e18f35f4d4376e8ff3110a3342f81ce9b Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [GIT PULL] Kbuild fixes for v5.0(-rc8)
The pull request you sent on Fri, 22 Feb 2019 09:29:27 +0900: > git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git > tags/kbuild-fixes-v5.0-2 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/77dc1181d896c5c3f8e131e341993aef41e16505 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
Re: [PATCH] au0828: minor fix to a misleading comment in _close()
Hi Shuah, Thanks for your patch. On 2019-02-22 10:45:59 -0700, Shuah Khan wrote: > Fix misleading comment in _close() > > Signed-off-by: Shuah Khan > --- > drivers/media/usb/au0828/au0828-video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/usb/au0828/au0828-video.c > b/drivers/media/usb/au0828/au0828-video.c > index 7876c897cc1d..08f566006a1f 100644 > --- a/drivers/media/usb/au0828/au0828-video.c > +++ b/drivers/media/usb/au0828/au0828-video.c > @@ -1074,7 +1074,7 @@ static int au0828_v4l2_close(struct file *filp) >* so the s_power callback are silently ignored. >* So, the current logic here does the following: >* Disable (put tuner to sleep) when > - * - ALSA and DVB aren't not streaming; > + * - ALSA and DVB aren't streaming; Nit-picking, as you are modifying the line anyhow I would s/;/./ With or without this changed, Reviewed-by: Niklas Söderlund >* - the last V4L2 file handler is closed. >* >* FIXME: > -- > 2.17.1 > -- Regards, Niklas Söderlund
[PATCH v4] mm/hugetlb: Fix unsigned overflow in __nr_hugepages_store_common()
User can change a node specific hugetlb count. i.e. /sys/devices/system/node/node1/hugepages/hugepages-2048kB/nr_hugepages the calculated value of count is a total number of huge pages. It could be overflow when a user entering a crazy high value. If so, the total number of huge pages could be a small value which is not user expect. We can simply fix it by setting count to ULONG_MAX, then it goes on. This may be more in line with user's intention of allocating as many huge pages as possible. Signed-off-by: Jing Xiangfeng --- mm/hugetlb.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/mm/hugetlb.c b/mm/hugetlb.c index afef616..6688894 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -2423,7 +2423,14 @@ static ssize_t __nr_hugepages_store_common(bool obey_mempolicy, * per node hstate attribute: adjust count to global, * but restrict alloc/free to the specified node. */ + unsigned long old_count = count; count += h->nr_huge_pages - h->nr_huge_pages_node[nid]; + /* +* If user specified count causes overflow, set to +* largest possible value. +*/ + if (count < old_count) + count = ULONG_MAX; init_nodemask_of_node(nodes_allowed, nid); } else nodes_allowed = _states[N_MEMORY]; -- 2.7.4
Re: [PATCH] [v2] kasan: turn off asan-stack for clang-8 and earlier
On 2/22/19 5:29 PM, Arnd Bergmann wrote: > Building an arm64 allmodconfig kernel with clang results in over 140 warnings > about overly large stack frames, the worst ones being: > > drivers/gpu/drm/panel/panel-sitronix-st7789v.c:196:12: error: stack frame > size of 20224 bytes in function 'st7789v_prepare' > drivers/video/fbdev/omap2/omapfb/displays/panel-tpo-td028ttec1.c:196:12: > error: stack frame size of 13120 bytes in function 'td028ttec1_panel_enable' > drivers/usb/host/max3421-hcd.c:1395:1: error: stack frame size of 10048 bytes > in function 'max3421_spi_thread' > drivers/net/wan/slic_ds26522.c:209:12: error: stack frame size of 9664 bytes > in function 'slic_ds26522_probe' > drivers/crypto/ccp/ccp-ops.c:2434:5: error: stack frame size of 8832 bytes in > function 'ccp_run_cmd' > drivers/media/dvb-frontends/stv0367.c:1005:12: error: stack frame size of > 7840 bytes in function 'stv0367ter_algo' > > None of these happen with gcc today, and almost all of these are the result > of a single known issue in llvm. Hopefully it will eventually get fixed with > the clang-9 release. > > In the meantime, the best idea I have is to turn off asan-stack for clang-8 > and earlier, so we can produce a kernel that is safe to run. > > I have posted three patches that address the frame overflow warnings that are > not addressed by turning off asan-stack, so in combination with this change, > we get much closer to a clean allmodconfig build, which in turn is necessary > to do meaningful build regression testing. > > It is still possible to turn on the CONFIG_ASAN_STACK option on all versions > of clang, and it's always enabled for gcc, but when CONFIG_COMPILE_TEST is > set, the option remains invisible, so allmodconfig and randconfig builds > (which are normally done with a forced CONFIG_COMPILE_TEST) will still result > in a mostly clean build. > > Cc: Andrey Ryabinin > Cc: Dmitry Vyukov > Cc: Nick Desaulniers > Cc: Mark Brown > Cc: Qian Cai > Cc: Kostya Serebryany > Cc: Andrey Konovalov > Link: https://bugs.llvm.org/show_bug.cgi?id=38809 > Signed-off-by: Arnd Bergmann > --- Reviewed-by: Qian Cai
Re: [PATCH v9 14/27] pwm: jz4740: Improve algorithm of clock calculation
Hi, Le jeu. 10 janv. 2019 à 11:04, Paul Cercueil a écrit : Adding Stephen to the discussion. Adding Stephen to the discussion. On Sat, Jan 5, 2019 at 6:27 PM, Uwe Kleine-König wrote: Hello Paul, On Sat, Jan 05, 2019 at 06:05:38PM -0300, Paul Cercueil wrote: On Sat, Jan 5, 2019 at 4:57 PM, Uwe Kleine-König wrote: > You are assuming stuff here about the parent clk which isn't guaranteed > (AFAICT) by the clk framework: If you call clk_round_rate(clk, rate - 1) > this might well return rate even if the clock could run slower than > rate. It may not be guaranteed by the clock framework itself, but it is guaranteed to behave like that on this family of SoCs. You shouldn't rely on that. Experience shows that people will start copying code to machines where this is not guaranteed. Even if they don't copy and only learn from reading this is bad. Also how do you guarantee that this won't change in the future making the pwm code break without noticing? If you use an API better don't assume more things given than are guaranteed by the API. Having said that I would consider it sensible to introduce something like clk_roundup_rate() and clk_rounddown_rate() which would allow calculations like that. @Stephen: Some context: my algorithm makes use of clk_round_rate(clk, rate - 1) to get the next (smaller) clock rate that a clock support. Is it something safe to assume? If not is there a better way? Bump. What should I do here? > Wouldn't it make sense to start iterating with rate = 0x * 1e9 / > period? Otherwise you get bad configurations if rate is considerable > slower than necessary. The algorithm will start with 'rate' being the parent clock's rate, which will always be the highest rate that the child clock will support. Ah right, I missed that bit. Thanks, -Paul
UBSAN: Undefined behaviour in ../drivers/cpuidle/governors/menu.c:229:21
This is 5.0-rc7 on an old Toshiba Portege laptop. [ 51.898454] UBSAN: Undefined behaviour in ../drivers/cpuidle/governors/menu.c:229:21 [ 51.898467] signed integer overflow: [ 51.898478] 3758096350 * 3758096350 cannot be represented in type 'long long int' [ 51.898496] CPU: 0 PID: 0 Comm: swapper/0 Tainted: GB W 5.0.0-rc7mod #3 [ 51.898507] Hardware name: TOSHIBA PORTEGE R835/Portable PC, BIOS Version 4.10 01/08/2013 [ 51.898519] Call Trace: [ 51.898533] dump_stack+0x7b/0xb5 [ 51.898544] ubsan_epilogue+0xd/0x81 [ 51.898553] handle_overflow+0x198/0x1d7 [ 51.898564] ? __ubsan_handle_negate_overflow+0x195/0x195 [ 51.898575] ? tick_nohz_get_sleep_length+0x121/0x1d0 [ 51.898588] ? ipv6_icmp_error+0x755/0x7f0 [ 51.898597] ? tick_nohz_next_event+0x1b3/0x4a0 [ 51.898607] ? can_stop_idle_tick+0x260/0x260 [ 51.898617] ? __switch_to_asm+0x40/0x70 [ 51.898628] ? calibrate_delay+0x25/0x2341 [ 51.898637] ? __switch_to_asm+0x34/0x70 [ 51.898646] ? common_interrupt+0xa/0xf [ 51.898658] __ubsan_handle_mul_overflow+0xe/0x1c [ 51.898669] menu_select+0x13d0/0x1df0 [ 51.898682] ? menu_enable_device+0x170/0x170 [ 51.898691] ? sched_clock+0x9/0x10 [ 51.898701] ? kasan_check_write+0x14/0x20 [ 51.898711] ? rcu_dynticks_eqs_exit+0x2c/0x80 [ 51.898722] cpuidle_select+0x5a/0xa0 [ 51.898732] do_idle+0x2aa/0x3c0 [ 51.898742] ? arch_cpu_idle_exit+0x40/0x40 [ 51.898751] ? kasan_check_read+0x11/0x20 [ 51.898761] ? do_raw_spin_unlock+0x54/0x220 [ 51.898773] cpu_startup_entry+0x18/0x20 [ 51.898782] rest_init+0x165/0x1a0 [ 51.898793] arch_call_rest_init+0x9/0x11 [ 51.898803] start_kernel+0x697/0x6bd [ 51.898813] ? mem_encrypt_init+0x6/0x6 [ 51.898822] ? load_ucode_intel_bsp+0x53/0x8a [ 51.898832] ? init_intel_microcode+0x1a4/0x1a4 [ 51.898844] ? load_ucode_bsp+0x1e8/0x21c [ 51.898854] x86_64_start_reservations+0x24/0x26 [ 51.898865] x86_64_start_kernel+0x74/0x77 [ 51.898874] secondary_startup_64+0xa4/0xb0 [ 51.898887] -- ~Randy
Re: [PATCH] checkpatch: don't complain about "undocumented" USB VID
On Fri, Feb 22, 2019 at 4:52 PM Joe Perches wrote:
>
> On Fri, 2019-02-22 at 14:55 -0800, Brian Norris wrote:
> > Documentation/devicetree/bindings/usb/usb-device.txt describes the
> > 'usbVID,...' compatible format, where VID is hexadecimal, with leading
> > zeroes suppressed. Allow it here without complaining.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3012,6 +3012,8 @@ sub process {
> > }
> >
> > next if $compat !~ /^([a-zA-Z0-9\-]+)\,/;
> > + # USB VIDs don't need special vendor prefix
> > documentation.
> > + next if $compat =~
> > /^usb[1-9a-f][0-9a-f]{0,3},/;
>
> Maybe check the PID too?
>
> next if $compat =~
> /^usb[1-9a-f][0-9a-f]{0,3},[1-9a-f][0-9a-f]{0,3}\b/;
Ah, good point. I shouldn't be allowing, e.g.,
"usb123,something-that's-not-a-PID".
> Maybe add a test for uses of the leading 0?
I suppose that'd be good too. Would help catch problems with the PID,
at least. (VID problems would fall into the "invalid vendor prefix"
check that's already present.)
Thanks,
Brian
Re: [PATCH] checkpatch: don't complain about "undocumented" USB VID
On Fri, 2019-02-22 at 14:55 -0800, Brian Norris wrote:
> Documentation/devicetree/bindings/usb/usb-device.txt describes the
> 'usbVID,...' compatible format, where VID is hexadecimal, with leading
> zeroes suppressed. Allow it here without complaining.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3012,6 +3012,8 @@ sub process {
> }
>
> next if $compat !~ /^([a-zA-Z0-9\-]+)\,/;
> + # USB VIDs don't need special vendor prefix
> documentation.
> + next if $compat =~ /^usb[1-9a-f][0-9a-f]{0,3},/;
Maybe check the PID too?
next if $compat =~
/^usb[1-9a-f][0-9a-f]{0,3},[1-9a-f][0-9a-f]{0,3}\b/;
Maybe add a test for uses of the leading 0?
Re: INFO: task hung in rtnetlink_rcv_msg
On 02/22/2019 10:45 PM, Jakub Kicinski wrote: > On Fri, 22 Feb 2019 12:14:57 -0800, Joe Perches wrote: >> On Fri, 2019-02-22 at 12:01 -0800, Jakub Kicinski wrote: >>> Hi! >>> >>> Seems like something funny is going on with get_maintainer.pl since XDP >>> entry got added. We seem to have been CCed on: >> >> I suggest removing the MAINTAINERS line with >> >> K: xdp >> >> as xdp is a pretty generic file/patch content >> regex match for the K: type >> >> $ git grep --name-only xdp | wc -l >> 236 >> >> Rather more files than desired. >> >> The N: match is dubious too. >> >> It's generally better to have specific lists of >> maintained file patterns rather than using the >> K: and N: pattern matches. >> >> --- >> MAINTAINERS | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index d7ad97b235ec..aa703e2cb882 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -16970,7 +16970,6 @@ F: include/net/xdp.h >> F: kernel/bpf/devmap.c >> F: kernel/bpf/cpumap.c >> F: include/trace/events/xdp.h >> -K: xdp >> N: xdp >> >> XDP SOCKETS (AF_XDP) > > Thanks for the explanation, at least now I know why it happens! :) > I'll leave it to Daniel to decide if we really need it removed, > so far the false positives weren't overwhelming, just surprising. No strong opinion. I've seen this K+N pattern in a number of places in the maintainers file. I'm fine either way if it gets too noisy. :)
Re: [PATCH 1/3] dt-bindings: qcom_spmi: Document pms405 support
On Mon, 28 Jan 2019 12:45:02 +0100, Jorge Ramirez-Ortiz wrote: > The PMS405 supports 5 SMPS and 13 LDO regulators. > > Signed-off-by: Jorge Ramirez-Ortiz > --- > .../bindings/regulator/qcom,spmi-regulator.txt | 24 > ++ > 1 file changed, 24 insertions(+) > Reviewed-by: Rob Herring
Re: [PATCH 1/3] dt-bindings: input: sitronix-st1232: add compatible string for ST1633
On Mon, 28 Jan 2019 09:44:47 +0100, Martin Kepplinger wrote: > From: Martin Kepplinger > > The st1232 driver gains support for the ST1633 controller too; update > the bindings doc accordingly. > > Signed-off-by: Martin Kepplinger > --- > .../bindings/input/touchscreen/sitronix-st1232.txt | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > Reviewed-by: Rob Herring
Re: [PATCH 1/6] dt-bindings: usb: xhci-tegra: Add Tegra186 support
On Fri, 25 Jan 2019 12:30:08 +0100, Thierry Reding wrote: > From: Thierry Reding > > Extend the bindings to cover the set of features found in Tegra186. > > Signed-off-by: Thierry Reding > --- > .../devicetree/bindings/usb/nvidia,tegra124-xusb.txt | 4 > 1 file changed, 4 insertions(+) > Reviewed-by: Rob Herring
Re: [RFC PATCH v1 07/13] devicetree: bindings: Document first ROHM BD70528 bindings
On Tue, Jan 22, 2019 at 11:45:38AM +0200, Matti Vaittinen wrote:
> Document bindings for regulators (3 bucks, 3 LDOs and 2 LED
> drivers) and 4 GPIO pins which can be configured for I/O or
> as interrupt sources withe configurable trigger levels.
'dt-bindings: mfd: ...' for the subject.
>
> Signed-off-by: Matti Vaittinen
> ---
> .../devicetree/bindings/mfd/rohm,bd70528-pmic.txt | 104
> +
> 1 file changed, 104 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> b/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> new file mode 100644
> index ..21801440a53a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/rohm,bd70528-pmic.txt
> @@ -0,0 +1,104 @@
> +* ROHM BD70528 Power Management Integrated Circuit bindings
> +
> +BD70528MWV is an ultra-low Iq general purpose single-chip power management IC
> +for battery-powered portable devices. The IC integrates 3 ultra-low current
> +consumption buck converters, 3 LDOs and 2 LED Drivers. Also included are 4
> +GPIOs, a real-time clock (RTC), a 32kHz clock gate, high-accuracy VREF
> +for use with an external ADC, flexible dual-input power path, 10 bits SAR ADC
> +for battery temperature monitor and 1S battery charger with scalable charge
> +currents.
> +
> +Required properties:
> + - compatible: Should be "rohm,bd70528"
> + - reg : I2C slave address.
> + - interrupt-parent : Phandle to the parent interrupt controller.
Don't need to document this. It is implied and could be in the parent.
> + - interrupts: The interrupt line the device is connected to.
> + - interrupt-controller : To indicate bd70528 acts as an interrupt
> controller.
> + - #interrupt-cells : Should be 2. usage is compliant to the 2 cells
> + variant of ../interrupt-controller/interrupts.txt
> + - gpio-controller : To indicate bd70528 acts as a gpio controller.
> + - #gpio-cells : Should be 2. The first cell is the pin number
> and
> + the second cell is used to specify flags. See
> + ../gpio/gpio.txt for more information.
> + - clock-frequency : Should be 32768
> + - #clock-cells : Should be 0.
> + - regulators: : List of child nodes that specify the
> regulators.
> + Please see ../regulator/rohm,bd70528-regulator.txt
> +
> +Optional properties:
> + - clock-output-names: Should contain name for output clock.
> +
> +Example:
> +/* external oscillator */
> +osc: oscillator {
> + compatible = "fixed-clock";
> + #clock-cells = <1>;
> + clock-frequency = <32768>;
> + clock-output-names = "osc";
> +};
> +
> +pmic: bd70528@4b {
pmic@4b
> + compatible = "rohm,bd70528";
> + reg = <0x4b>;
> + interrupt-parent = <>;
> + interrupts = <29 GPIO_ACTIVE_LOW>;
> + clocks = < 0>;
> + #clock-cells = <0>;
> + clock-output-names = "bd70528-32k-out";
> + #gpio-cells = <2>;
> + gpio-controller;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> +
> + regulators {
> + buck1: BUCK1 {
> + regulator-name = "buck1";
> + regulator-min-microvolt = <120>;
> + regulator-max-microvolt = <340>;
> + regulator-boot-on;
> + regulator-ramp-delay = <125>;
> + };
> + buck2: BUCK2 {
> + regulator-name = "buck2";
> + regulator-min-microvolt = <120>;
> + regulator-max-microvolt = <330>;
> + regulator-boot-on;
> + regulator-ramp-delay = <125>;
> + };
> + buck3: BUCK3 {
> + regulator-name = "buck3";
> + regulator-min-microvolt = <80>;
> + regulator-max-microvolt = <180>;
> + regulator-boot-on;
> + regulator-ramp-delay = <250>;
> + };
> + ldo1: LDO1 {
> + regulator-name = "ldo1";
> + regulator-min-microvolt = <165>;
> + regulator-max-microvolt = <330>;
> + regulator-boot-on;
> + };
> + ldo2: LDO2 {
> + regulator-name = "ldo2";
> + regulator-min-microvolt = <165>;
> + regulator-max-microvolt = <330>;
> + regulator-boot-on;
> + };
> +
> + ldo3: LDO3 {
> + regulator-name = "ldo3";
> + regulator-min-microvolt = <165>;
> + regulator-max-microvolt = <330>;
> + };
> + led_ldo1:
Re: [PATCH 1/6] dt-bindings: soc: qcom: Add interconnect binding for GENI QUP
On Wed, Jan 23, 2019 at 08:41:20PM +0200, Georgi Djakov wrote: > Hi, > > On 1/23/19 19:07, Bjorn Andersson wrote: > > On Mon 21 Jan 22:33 PST 2019, Alok Chauhan wrote: > > > >> Add documentation for the interconnect and interconnect-names bindings > >> for the GENI QUP as detailed by bindings/interconnect/interconnect.txt. > >> > >> Signed-off-by: Alok Chauhan > >> --- > >> Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 10 > >> ++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt > >> b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt > >> index dab7ca9..44d7e02 100644 > >> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt > >> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt > >> @@ -17,6 +17,12 @@ Required properties if child node exists: > >> - #address-cells: Must be <1> for Serial Engine Address > >> - #size-cells:Must be <1> for Serial Engine Address Size > >> - ranges: Must be present > >> +- interconnects: phandle to a interconnect provider. Please refer > >> + ../interconnect/interconnect.txt for details. > >> + Must be 2 paths corresponding to 2 AXI ports. > >> +- interconnect-names: Port names to differentiate between the > > > > s/Port names/Path names/ > > > >> + 2 interconnect paths defined with interconnect > >> + specifier. > > > > These two names are significant in that they must match what the driver > > expects, hence you must actually specify them here. > > > > And as the scope of these strings are local to the QUP node you can omit > > "qup" from them, so make them "memory" and "config" (or perhaps iface, > > to match the clock naming?). > > Actually there was a discussion in the past where we decided include > both the src and dst endpoint names in this property so that there is > some symmetry with the "interconnects" property. It would be nice to be > consistent across different drivers at least for now. > If we want to denote the master and slave ports here, my two cents would > be for "qup-mem" and "cpu-qup" or something similar? Well, there's a proposal from Maxime to add 'dma-memory' or something. You all need to sort this out. I assume config or cpu-qup is for register access? Why is this needed? That should get described thru the DT tree. The interconnect stuff was supposed to be for the non-cpu centric view (i.e. DMA masters). Maybe it's fine, but that's not my initial reaction. Rob
Re: [PATCH v2 3/4] dt-bindings: display: Add bindings for OSD101T2587-53TS panel
On Fri, 22 Feb 2019 15:16:17 +0200, Peter Ujfalusi wrote: > This adds the device-tree bindings for the OSD101T2587-53TS 10.1" > 1920x1200 panel from One Stop Displays. > > Note: the panel is similar to OSD101T2045-53TS, but it needs additional > MIPI_DSI_TURN_ON_PERIPHERAL message from the host. > > Signed-off-by: Peter Ujfalusi > --- > .../display/panel/osd,osd101t2587-53ts.txt | 14 ++ > 1 file changed, 14 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/osd,osd101t2587-53ts.txt > Reviewed-by: Rob Herring
Re: [PATCH v2 1/4] dt-bindings: display: Add bindings for OSD101T2045-53TS
On Fri, 22 Feb 2019 15:16:15 +0200, Peter Ujfalusi wrote: > This adds the device-tree bindings for the OSD101T2045-53TS 10.1" > 1920x1200 panel from One Stop Displays. > > Signed-off-by: Peter Ujfalusi > --- > .../bindings/display/panel/osd,osd101t2045-53ts.txt | 11 +++ > 1 file changed, 11 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/osd,osd101t2045-53ts.txt > Reviewed-by: Rob Herring
Re: [PATCH V2 6/8] dt-bindings: net: stmmac: add syscfg clock property
On Fri, 22 Feb 2019 09:28:07 +0100, Christophe Roullier wrote: > Need syscfg clock for MCU family in case bootloader does not > activate it. > > Signed-off-by: Christophe Roullier > --- > Documentation/devicetree/bindings/net/stm32-dwmac.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Rob Herring
[PATCH] media: replace WARN_ON in __media_pipeline_start()
__media_pipeline_start() does WARN_ON() when active pipe doesn't
match the input arg entity's pipe.
Replace WARN_ON with a conditional and error message that includes
names of both entities.
Signed-off-by: Shuah Khan
---
drivers/media/media-entity.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 0b1cb3559140..757c641b7409 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -436,7 +436,10 @@ __must_check int __media_pipeline_start(struct
media_entity *entity,
entity->stream_count++;
- if (WARN_ON(entity->pipe && entity->pipe != pipe)) {
+ if (entity->pipe && entity->pipe != pipe) {
+ pr_err("Pipe active for %s. Can't start for %s\n",
+ entity->name,
+ entity_err->name);
ret = -EBUSY;
goto error;
}
--
2.17.1
Re: [PATCH V2 3/8] dt-bindings: net: stmmac: add phys config properties
On Fri, Feb 22, 2019 at 09:28:04AM +0100, Christophe Roullier wrote: > Add properties to support all Phy config > PHY_MODE (MII,GMII, RMII, RGMII) and in normal, PHY wo crystal (25Mhz), > PHY wo crystal (50Mhz), No 125Mhz from PHY config. > > Signed-off-by: Christophe Roullier > --- > Documentation/devicetree/bindings/net/stm32-dwmac.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.txt > b/Documentation/devicetree/bindings/net/stm32-dwmac.txt > index 1341012..f42dc68 100644 > --- a/Documentation/devicetree/bindings/net/stm32-dwmac.txt > +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.txt > @@ -24,9 +24,9 @@ Required properties: > encompases the glue register, and the offset of the control > register. > > Optional properties: > -- clock-names: For MPU family "mac-clk-ck" for PHY without quartz > -- st,int-phyclk (boolean) : valid only where PHY do not have quartz and > need to be clock > -by RCC You can't just remove properties. > +- clock-names: For MPU family "eth-ck" for PHY without quartz > +- st,eth_clk_sel (boolean) : set this property in RGMII PHY when you do not > want use 125Mhz > +- st,eth_ref_clk_sel (boolean) : set this property in RMII mode when you > have PHY without crystal 50MHz s/_/-/ 'sel' I assume is short for select, but the naming here and description don't really tell me what I'm getting. Rob
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
> On Feb 22, 2019, at 3:59 PM, Andy Lutomirski wrote: > > On Fri, Feb 22, 2019 at 3:02 PM Jann Horn wrote: >> On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit wrote: On Feb 22, 2019, at 2:21 PM, Nadav Amit wrote: > On Feb 22, 2019, at 2:17 PM, Jann Horn wrote: > > On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit wrote: >>> On Feb 22, 2019, at 1:43 PM, Jann Horn wrote: >>> >>> (adding some people from the text_poke series to the thread, removing >>> stable@) >>> >>> On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski >>> wrote: > On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov > wrote: >> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote: >> On Fri, 22 Feb 2019 11:27:05 -0800 >> Alexei Starovoitov wrote: >> On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote: Then we should still probably fix up "__probe_kernel_read()" to not allow user accesses. The easiest way to do that is actually likely to use the "unsafe_get_user()" functions *without* doing a uaccess_begin(), which will mean that modern CPU's will simply fault on a kernel access to user space. >>> >>> On bpf side the bpf_probe_read() helper just calls >>> probe_kernel_read() >>> and users pass both user and kernel addresses into it and expect >>> that the helper will actually try to read from that address. >>> >>> If __probe_kernel_read will suddenly start failing on all user >>> addresses >>> it will break the expectations. >>> How do we solve it in bpf_probe_read? >>> Call probe_kernel_read and if that fails call unsafe_get_user >>> byte-by-byte >>> in the loop? >>> That's doable, but people already complain that bpf_probe_read() is >>> slow >>> and shows up in their perf report. >> >> We're changing kprobes to add a specific flag to say that we want to >> differentiate between kernel or user reads. Can this be done with >> bpf_probe_read()? If it's showing up in perf report, I doubt a single > > so you're saying you will break existing kprobe scripts? > I don't think it's a good idea. > It's not acceptable to break bpf_probe_read uapi. If so, the uapi is wrong: a long-sized number does not reliably identify an address if you don’t separately know whether it’s a user or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. I have lobbied for RISC-V and future x86_64 to join the crowd. I don’t know whether I’ll win this fight, but the uapi will probably have to change for at least s390x. What to do about existing scripts is a different question. >>> >>> This lack of logical separation between user and kernel addresses >>> might interact interestingly with the text_poke series, specifically >>> "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for >>> patching" >>> (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2Fdata=02%7C01%7Cnamit%40vmware.com%7Cbab53e52cc5c4ac4419008d69921d1f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864767879731941sdata=2tqD7udTCfNbcNLcj5SFpZt8WwK5NwtgaWMKm1Ye1EE%3Dreserved=0) >>> and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text >>> poking" >>> (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2Fdata=02%7C01%7Cnamit%40vmware.com%7Cbab53e52cc5c4ac4419008d69921d1f1%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864767879731941sdata=7%2BLShgLKnra6xzSkxdJrCclCacfdE5IdczwScW83nuE%3Dreserved=0), >>> right? If someone manages to get a tracing BPF program to trigger in a >>> task that has switched to the patching mm, could they use >>> bpf_probe_write_user() - which uses probe_kernel_write() after >>> checking that KERNEL_DS isn't active and that access_ok() passes - to >>> overwrite kernel text that is mapped writable in the patching mm? >> >> Yes, this is a good point. I guess text_poke() should be defined with >> “__kprobes” and open-code memcpy. >> >> Does it sound reasonable? > > Doesn't __text_poke() as implemented in the proposed patch use a > couple other kernel functions, too? Like switch_mm_irqs_off() and > pte_clear() (which can be a call into a separate function on paravirt > kernels)? I will move the pte_clear() to be done after the poking mm was unloaded. Give me a few minutes to send a sketch of what I think should be done. >>> >>> Err.. You are
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, Feb 22, 2019 at 3:56 PM Alexei Starovoitov wrote: > > It will preserve existing bpf_probe_read() behavior on x86. ... but that's the worst possible situation. It appears that people haven't understood that kernel and user addresses are distinct, and may have written programs that are fundamentally buggy. And we _want_ to make it clear that they are buggy on x86-64, exactly because x86-64 is the one that gets the most testing - by far. So if x86-64 continues working for buggy programs, then that only means that those bugs never get fixed. It would be much better to try to get those things fixed, and make the x86-64 implementation stricter, exactly so that people end up _realizing_ that they can't just think "a pointer is a pointer, and the context doesn't matter". >From a pure functional safety standpoint, I thought bpf already knew what kind of a pointer it had? Linus
[PATCH] clk: at91: programmable: remove unneeded register read
From: Nicolas Ferre This register read is a leftover of a previous read/modify/write. We now use regmap_update_bits(), so we don't need it anymore. Signed-off-by: Nicolas Ferre Signed-off-by: Alexandre Belloni --- drivers/clk/at91/clk-programmable.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/clk/at91/clk-programmable.c b/drivers/clk/at91/clk-programmable.c index 5bc68b9c5498..89d6f3736dbf 100644 --- a/drivers/clk/at91/clk-programmable.c +++ b/drivers/clk/at91/clk-programmable.c @@ -132,11 +132,8 @@ static int clk_programmable_set_rate(struct clk_hw *hw, unsigned long rate, struct clk_programmable *prog = to_clk_programmable(hw); const struct clk_programmable_layout *layout = prog->layout; unsigned long div = parent_rate / rate; - unsigned int pckr; int shift = 0; - regmap_read(prog->regmap, AT91_PMC_PCKR(prog->id), ); - if (!div) return -EINVAL; -- 2.20.1
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, Feb 22, 2019 at 03:59:30PM -0800, Andy Lutomirski wrote: > > > > A relatively simple approach might be to teach BPF not to run kprobe > > programs and such in contexts where current->mm isn't the active mm? > > Maybe using nmi_uaccess_okay(), or something like that? It looks like > > perf_callchain_user() also already uses that. Except that a lot of > > this code is x86-specific... > > This sounds like exactly the right solution. If you're running from > some unknown context (like NMI or tracing), then you should check > nmi_uaccess_okay(). I think we should just promote that to be a > non-arch-specific function (that returns true by default) and check it > the relevant bpf_probe_xyz() functions. > > Alexei, does that seem reasonable? yep. I think that should work.
Re: [bpf] 568f196756: BUG:assuming_atomic_context_at_kernel/seccomp.c
On Sat, 23 Feb 2019 00:15:38 +0100, Jann Horn said: > On Fri, Feb 22, 2019 at 11:42 PM wrote: > > Is there a reason this commit appeared in next-20190221 but did not seem to > > be > > in next-20190222 (confirmed via 'git tag --contains')?y ast@, he wanted to > > fix it in a different way: > https://lore.kernel.org/netdev/20190221192916.2mcd4fmxbdj2j...@ast-mbp.dhcp.thefacebook.com/ Aha. That was cc:ed to netdev, but not to lkml. Explains why I missed that piece of the story
Re: [RFC][Patch v8 0/7] KVM: Guest Free Page Hinting
On Mon, Feb 4, 2019 at 1:47 PM Nitesh Narayan Lal wrote: > > The following patch-set proposes an efficient mechanism for handing freed > memory between the guest and the host. It enables the guests with no page > cache to rapidly free and reclaims memory to and from the host respectively. > > Benefit: > With this patch-series, in our test-case, executed on a single system and > single NUMA node with 15GB memory, we were able to successfully launch > atleast 5 guests > when page hinting was enabled and 3 without it. (Detailed explanation of the > test procedure is provided at the bottom). > > Changelog in V8: > In this patch-series, the earlier approach [1] which was used to capture and > scan the pages freed by the guest has been changed. The new approach is > briefly described below: > > The patch-set still leverages the existing arch_free_page() to add this > functionality. It maintains a per CPU array which is used to store the pages > freed by the guest. The maximum number of entries which it can hold is > defined by MAX_FGPT_ENTRIES(1000). When the array is completely filled, it is > scanned and only the pages which are available in the buddy are stored. This > process continues until the array is filled with pages which are part of the > buddy free list. After which it wakes up a kernel per-cpu-thread. > This kernel per-cpu-thread rescans the per-cpu-array for any re-allocation > and if the page is not reallocated and present in the buddy, the kernel > thread attempts to isolate it from the buddy. If it is successfully isolated, > the page is added to another per-cpu array. Once the entire scanning process > is complete, all the isolated pages are reported to the host through an > existing virtio-balloon driver. > > Known Issues: > * Fixed array size: The problem with having a fixed/hardcoded array > size arises when the size of the guest varies. For example when the guest > size increases and it starts making large allocations fixed size limits this > solution's ability to capture all the freed pages. This will result in less > guest free memory getting reported to the host. > > Known code re-work: > * Plan to re-use Wei's work, which communicates the poison value to > the host. > * The nomenclatures used in virtio-balloon needs to be changed so > that the code can easily be distinguished from Wei's Free Page Hint code. > * Sorting based on zonenum, to avoid repetitive zone locks for the > same zone. > > Other required work: > * Run other benchmarks to evaluate the performance/impact of this > approach. > > Test case: > Setup: > Memory-15837 MB > Guest Memory Size-5 GB > Swap-Disabled > Test Program-Simple program which allocates 4GB memory via malloc, touches it > via memset and exits. > Use case-Number of guests that can be launched completely including the > successful execution of the test program. > Procedure: > The first guest is launched and once its console is up, the test allocation > program is executed with 4 GB memory request (Due to this the guest occupies > almost 4-5 GB of memory in the host in a system without page hinting). Once > this program exits at that time another guest is launched in the host and the > same process is followed. We continue launching the guests until a guest gets > killed due to low memory condition in the host. > > Result: > Without Hinting-3 Guests > With Hinting-5 to 7 Guests(Based on the amount of memory freed/captured). > > [1] https://www.spinics.net/lists/kvm/msg170113.html So I tried reproducing your test and I am not having much luck. According to the sysctl in the guest I am seeing "vm.guest-page-hinting = 1" which is supposed to indicate that the hinting is enabled in both QEMU and the guest right? I'm just wanting to verify that this is the case before I start doing any debugging. I'm assuming you never really ran any multi-threaded tests on a multi-CPU guest did you? With the patches applied I am seeing stability issues. If I enable a VM with multiple CPUs and run something like the page_fault1 test from the will-it-scale suite I am seeing multiple traces being generated by the guest kernel and it ultimately just hangs. I have included the traces below. There end up being 3 specific issues, a double free that is detected, the RCU stall, and then starts complaining about a soft lockup. Thanks. - Alex -- This looks like a page complaining about a double add when added to the LRU -- [ 50.479635] list_add double add: new=f6448008, prev=a000fffd50c0, next=f6448008. [ 50.481066] [ cut here ] [ 50.481753] kernel BUG at lib/list_debug.c:31! [ 50.482448] invalid opcode: [#1] SMP PTI [ 50.483108] CPU: 1 PID: 852 Comm: hinting/1 Not tainted 5.0.0-rc7-next-20190219-baseline+ #50 [ 50.486362] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 [ 50.487881] RIP: 0010:__list_add_valid+0x4b/0x70
[PATCH 1/8] switchdev: Add SWITCHDEV_PORT_ATTR_SET
In preparation for allowing switchdev enabled drivers to veto specific
attribute settings from within the context of the caller, introduce a
new switchdev notifier type for port attributes.
Suggested-by: Ido Schimmel
Signed-off-by: Florian Fainelli
---
include/net/switchdev.h | 27 +
net/switchdev/switchdev.c | 51 +++
2 files changed, 78 insertions(+)
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 45310ddf2d7e..ca56b7487540 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -136,6 +136,7 @@ enum switchdev_notifier_type {
SWITCHDEV_PORT_OBJ_ADD, /* Blocking. */
SWITCHDEV_PORT_OBJ_DEL, /* Blocking. */
+ SWITCHDEV_PORT_ATTR_SET, /* May be blocking . */
SWITCHDEV_VXLAN_FDB_ADD_TO_BRIDGE,
SWITCHDEV_VXLAN_FDB_DEL_TO_BRIDGE,
@@ -164,6 +165,13 @@ struct switchdev_notifier_port_obj_info {
bool handled;
};
+struct switchdev_notifier_port_attr_info {
+ struct switchdev_notifier_info info; /* must be first */
+ const struct switchdev_attr *attr;
+ struct switchdev_trans *trans;
+ bool handled;
+};
+
static inline struct net_device *
switchdev_notifier_info_to_dev(const struct switchdev_notifier_info *info)
{
@@ -216,7 +224,15 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
int (*del_cb)(struct net_device *dev,
const struct switchdev_obj *obj));
+int switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans));
+
#define SWITCHDEV_SET_OPS(netdev, ops) ((netdev)->switchdev_ops = (ops))
+
#else
static inline void switchdev_deferred_process(void)
@@ -303,6 +319,17 @@ switchdev_handle_port_obj_del(struct net_device *dev,
return 0;
}
+static inline int
+switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans))
+{
+ return 0;
+}
+
#define SWITCHDEV_SET_OPS(netdev, ops) do {} while (0)
#endif
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 7e1357db33d7..94400f5b8e07 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -697,3 +697,54 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
return err;
}
EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_del);
+
+static int __switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans))
+{
+ struct net_device *lower_dev;
+ struct list_head *iter;
+ int err = -EOPNOTSUPP;
+
+ if (check_cb(dev)) {
+ port_attr_info->handled = true;
+ return set_cb(dev, port_attr_info->attr,
+ port_attr_info->trans);
+ }
+
+ /* Switch ports might be stacked under e.g. a LAG. Ignore the
+* unsupported devices, another driver might be able to handle them. But
+* propagate to the callers any hard errors.
+*
+* If the driver does its own bookkeeping of stacked ports, it's not
+* necessary to go through this helper.
+*/
+ netdev_for_each_lower_dev(dev, lower_dev, iter) {
+ err = __switchdev_handle_port_attr_set(lower_dev,
port_attr_info,
+ check_cb, set_cb);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+ }
+
+ return err;
+}
+
+int switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans))
+{
+ int err;
+
+ err = __switchdev_handle_port_attr_set(dev, port_attr_info, check_cb,
+
[PATCH net-next 0/8] net: Remove switchdev_ops
Hi all, This patch series completes the removal of the switchdev_ops by converting switchdev_port_attr_set() to use either the blocking (process) or non-blocking (atomic) notifier since we typically need to deal with both depending on where in the bridge code we get called from. This was tested with the forwarding selftests and DSA hardware. Ido, Jiri, please review :) Florian Fainelli (8): switchdev: Add SWITCHDEV_PORT_ATTR_SET rocker: Handle SWITCHDEV_PORT_ATTR_SET net: dsa: Handle SWITCHDEV_PORT_ATTR_SET mlxsw: spectrum_switchdev: Handle SWITCHDEV_PORT_ATTR_SET net: mscc: ocelot: Handle SWITCHDEV_PORT_ATTR_SET staging: fsl-dpaa2: ethsw: Handle SWITCHDEV_PORT_ATTR_SET net: switchdev: Replace port attr set SDO with a notification net: Remove switchdev_ops .../net/ethernet/mellanox/mlxsw/spectrum.c| 12 -- .../net/ethernet/mellanox/mlxsw/spectrum.h| 2 - .../mellanox/mlxsw/spectrum_switchdev.c | 24 +-- drivers/net/ethernet/mscc/ocelot.c| 32 +++- drivers/net/ethernet/mscc/ocelot.h| 1 + drivers/net/ethernet/mscc/ocelot_board.c | 2 + drivers/net/ethernet/rocker/rocker_main.c | 23 ++- drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 26 +++- include/linux/netdevice.h | 3 - include/net/switchdev.h | 42 ++--- net/dsa/slave.c | 23 ++- net/switchdev/switchdev.c | 147 +- 12 files changed, 200 insertions(+), 137 deletions(-) -- 2.17.1
[PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field
from all clients, which were migrated to use switchdev notification in
the previous patches.
Add a new function switchdev_port_attr_notify() that sends the switchdev
notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on
SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking
(atomic) notifier chain accordingly.
Drop __switchdev_port_attr_set() and update switchdev_port_attr_set()
likewise.
Signed-off-by: Florian Fainelli
---
net/switchdev/switchdev.c | 96 +++
1 file changed, 26 insertions(+), 70 deletions(-)
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 94400f5b8e07..a1f16836ef89 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device
*dev,
return 0;
}
-/**
- * switchdev_port_attr_get - Get port attribute
- *
- * @dev: port device
- * @attr: attribute to get
- */
-int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr
*attr)
+static int switchdev_port_attr_notify(enum switchdev_notifier_type nt,
+ struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans)
{
- const struct switchdev_ops *ops = dev->switchdev_ops;
- struct net_device *lower_dev;
- struct list_head *iter;
- struct switchdev_attr first = {
- .id = SWITCHDEV_ATTR_ID_UNDEFINED
- };
- int err = -EOPNOTSUPP;
+ int err;
+ int rc;
- if (ops && ops->switchdev_port_attr_get)
- return ops->switchdev_port_attr_get(dev, attr);
+ struct switchdev_notifier_port_attr_info attr_info = {
+ .attr = attr,
+ .trans = trans,
+ .handled = false,
+ };
- if (attr->flags & SWITCHDEV_F_NO_RECURSE)
+ if (attr & SWITCHDEV_F_DEFER)
+ rc = call_switchdev_blocking_notifiers(nt, dev,
+ _info.info, NULL);
+ else
+ rc = call_switchdev_notifiers(nt, dev, _info.info, NULL);
+ err = notifier_to_errno(rc);
+ if (err) {
+ WARN_ON(!attr_info.handled);
return err;
-
- /* Switch device port(s) may be stacked under
-* bond/team/vlan dev, so recurse down to get attr on
-* each port. Return -ENODATA if attr values don't
-* compare across ports.
-*/
-
- netdev_for_each_lower_dev(dev, lower_dev, iter) {
- err = switchdev_port_attr_get(lower_dev, attr);
- if (err)
- break;
- if (first.id == SWITCHDEV_ATTR_ID_UNDEFINED)
- first = *attr;
- else if (memcmp(, attr, sizeof(*attr)))
- return -ENODATA;
}
- return err;
-}
-EXPORT_SYMBOL_GPL(switchdev_port_attr_get);
-
-static int __switchdev_port_attr_set(struct net_device *dev,
-const struct switchdev_attr *attr,
-struct switchdev_trans *trans)
-{
- const struct switchdev_ops *ops = dev->switchdev_ops;
- struct net_device *lower_dev;
- struct list_head *iter;
- int err = -EOPNOTSUPP;
-
- if (ops && ops->switchdev_port_attr_set) {
- err = ops->switchdev_port_attr_set(dev, attr, trans);
- goto done;
- }
-
- if (attr->flags & SWITCHDEV_F_NO_RECURSE)
- goto done;
-
- /* Switch device port(s) may be stacked under
-* bond/team/vlan dev, so recurse down to set attr on
-* each port.
-*/
-
- netdev_for_each_lower_dev(dev, lower_dev, iter) {
- err = __switchdev_port_attr_set(lower_dev, attr, trans);
- if (err)
- break;
- }
-
-done:
- if (err == -EOPNOTSUPP && attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP)
- err = 0;
+ if (!attr_info.handled)
+ return -EOPNOTSUPP;
- return err;
+ return 0;
}
static int switchdev_port_attr_set_now(struct net_device *dev,
@@ -267,7 +221,8 @@ static int switchdev_port_attr_set_now(struct net_device
*dev,
*/
trans.ph_prepare = true;
- err = __switchdev_port_attr_set(dev, attr, );
+ err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr,
+);
if (err) {
/* Prepare phase failed: abort the transaction. Any
* resources reserved in the prepare phase are
@@ -286,7 +241,8 @@ static int switchdev_port_attr_set_now(struct net_device
*dev,
*/
trans.ph_prepare = false;
- err =
[PATCH net-next 5/8] net: mscc: ocelot: Handle SWITCHDEV_PORT_ATTR_SET
Following patches will change the way we communicate setting a port's
attribute and use notifiers to perform those tasks.
Ocelot does not currently have an atomic notifier registered for
switchdev events, so we need to register one in order to deal with
atomic context SWITCHDEV_PORT_ATTR_SET events.
Signed-off-by: Florian Fainelli
---
drivers/net/ethernet/mscc/ocelot.c | 27
drivers/net/ethernet/mscc/ocelot.h | 1 +
drivers/net/ethernet/mscc/ocelot_board.c | 2 ++
3 files changed, 30 insertions(+)
diff --git a/drivers/net/ethernet/mscc/ocelot.c
b/drivers/net/ethernet/mscc/ocelot.c
index 195306d05bcd..83a678b11757 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1582,6 +1582,28 @@ struct notifier_block ocelot_netdevice_nb __read_mostly
= {
};
EXPORT_SYMBOL(ocelot_netdevice_nb);
+static int ocelot_switchdev_event(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+ struct net_device *dev = switchdev_notifier_info_to_dev(ptr);
+ int err;
+
+ switch (event) {
+ case SWITCHDEV_PORT_ATTR_SET:
+ err = switchdev_handle_port_attr_set(dev, ptr,
+ocelot_netdevice_dev_check,
+ocelot_port_attr_set);
+ return notifier_from_errno(err);
+ }
+
+ return NOTIFY_DONE;
+}
+
+struct notifier_block ocelot_switchdev_nb __read_mostly = {
+ .notifier_call = ocelot_switchdev_event,
+};
+EXPORT_SYMBOL(ocelot_switchdev_nb);
+
static int ocelot_switchdev_blocking_event(struct notifier_block *unused,
unsigned long event, void *ptr)
{
@@ -1600,6 +1622,11 @@ static int ocelot_switchdev_blocking_event(struct
notifier_block *unused,
ocelot_netdevice_dev_check,
ocelot_port_obj_del);
return notifier_from_errno(err);
+ case SWITCHDEV_PORT_ATTR_SET:
+ err = switchdev_handle_port_attr_set(dev, ptr,
+ocelot_netdevice_dev_check,
+ocelot_port_attr_set);
+ return notifier_from_errno(err);
}
return NOTIFY_DONE;
diff --git a/drivers/net/ethernet/mscc/ocelot.h
b/drivers/net/ethernet/mscc/ocelot.h
index 086775f7b52f..ba3b3380b4d0 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -499,6 +499,7 @@ int ocelot_probe_port(struct ocelot *ocelot, u8 port,
struct phy_device *phy);
extern struct notifier_block ocelot_netdevice_nb;
+extern struct notifier_block ocelot_switchdev_nb;
extern struct notifier_block ocelot_switchdev_blocking_nb;
#endif
diff --git a/drivers/net/ethernet/mscc/ocelot_board.c
b/drivers/net/ethernet/mscc/ocelot_board.c
index ca3ea2fbfcd0..2c1121d86edf 100644
--- a/drivers/net/ethernet/mscc/ocelot_board.c
+++ b/drivers/net/ethernet/mscc/ocelot_board.c
@@ -329,6 +329,7 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
}
register_netdevice_notifier(_netdevice_nb);
+ register_switchdev_notifier(_switchdev_nb);
register_switchdev_blocking_notifier(_switchdev_blocking_nb);
dev_info(>dev, "Ocelot switch probed\n");
@@ -345,6 +346,7 @@ static int mscc_ocelot_remove(struct platform_device *pdev)
ocelot_deinit(ocelot);
unregister_switchdev_blocking_notifier(_switchdev_blocking_nb);
+ unregister_switchdev_notifier(_switchdev_nb);
unregister_netdevice_notifier(_netdevice_nb);
return 0;
--
2.17.1
[PATCH net-next 4/8] mlxsw: spectrum_switchdev: Handle SWITCHDEV_PORT_ATTR_SET
Following patches will change the way we communicate setting a port's
attribute and use a notifier to perform those tasks.
Prepare mlxsw to support receiving notifier events targeting
SWITCHDEV_PORT_ATTR_SET and utilize the switchdev_handle_port_attr_set()
to handle stacking of devices.
Signed-off-by: Florian Fainelli
---
.../net/ethernet/mellanox/mlxsw/spectrum_switchdev.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index 766f5b5f1cf5..c1aedfea3a31 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -3123,6 +3123,13 @@ static int mlxsw_sp_switchdev_event(struct
notifier_block *unused,
struct net_device *br_dev;
int err;
+ if (event == SWITCHDEV_PORT_ATTR_SET) {
+ err = switchdev_handle_port_attr_set(dev, ptr,
+mlxsw_sp_port_dev_check,
+mlxsw_sp_port_attr_set);
+ return notifier_from_errno(err);
+ }
+
/* Tunnel devices are not our uppers, so check their master instead */
br_dev = netdev_master_upper_dev_get_rcu(dev);
if (!br_dev)
@@ -3446,6 +3453,11 @@ static int mlxsw_sp_switchdev_blocking_event(struct
notifier_block *unused,
mlxsw_sp_port_dev_check,
mlxsw_sp_port_obj_del);
return notifier_from_errno(err);
+ case SWITCHDEV_PORT_ATTR_SET:
+ err = switchdev_handle_port_attr_set(dev, ptr,
+mlxsw_sp_port_dev_check,
+mlxsw_sp_port_attr_set);
+ return notifier_from_errno(err);
}
return NOTIFY_DONE;
--
2.17.1
[PATCH net-next 6/8] staging: fsl-dpaa2: ethsw: Handle SWITCHDEV_PORT_ATTR_SET
Following patches will change the way we communicate setting a port's
attribute and use a blocking notifier to perform those tasks.
Prepare ethsw to support receiving notifier events targeting
SWITCHDEV_PORT_ATTR_SET and simply translate that into the existing
swdev_port_attr_set() call.
Signed-off-by: Florian Fainelli
---
drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index 018399ee8731..73efc2a5fd91 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -1047,6 +1047,12 @@ static int port_switchdev_event(struct notifier_block
*unused,
struct ethsw_switchdev_event_work *switchdev_work;
struct switchdev_notifier_fdb_info *fdb_info = ptr;
+ if (!ethsw_port_dev_check(dev))
+ return NOTIFY_DONE;
+
+ if (event == SWITCHDEV_PORT_ATTR_SET)
+ return ethsw_switchdev_port_attr_set_event(dev, ptr);
+
switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
if (!switchdev_work)
return NOTIFY_BAD;
@@ -1103,6 +1109,19 @@ ethsw_switchdev_port_obj_event(unsigned long event,
struct net_device *netdev,
return notifier_from_errno(err);
}
+static int
+ethsw_switchdev_port_attr_set_event(struct net_device *netdev,
+ struct switchdev_notifier_port_attr_info *port_attr_info)
+{
+ int err;
+
+ err = swdev_port_attr_set(netdev, port_attr_info->attr,
+ port_attr_info->trans);
+
+ port_attr_info->handled = true;
+ return notifier_from_errno(err);
+}
+
static int port_switchdev_blocking_event(struct notifier_block *unused,
unsigned long event, void *ptr)
{
@@ -1115,6 +1134,8 @@ static int port_switchdev_blocking_event(struct
notifier_block *unused,
case SWITCHDEV_PORT_OBJ_ADD: /* fall through */
case SWITCHDEV_PORT_OBJ_DEL:
return ethsw_switchdev_port_obj_event(event, dev, ptr);
+ case SWITCHDEV_PORT_ATTR_SET:
+ return ethsw_switchdev_port_attr_set_event(dev, ptr);
}
return NOTIFY_DONE;
--
2.17.1
[PATCH net-next 8/8] net: Remove switchdev_ops
Now that we have converted all possible callers to using a switchdev
notifier for attributes we do not have a need for implementing
switchdev_ops anymore, and this can be removed from all drivers the
net_device structure.
Signed-off-by: Florian Fainelli
---
.../net/ethernet/mellanox/mlxsw/spectrum.c| 12 ---
.../net/ethernet/mellanox/mlxsw/spectrum.h| 2 --
.../mellanox/mlxsw/spectrum_switchdev.c | 12 ---
drivers/net/ethernet/mscc/ocelot.c| 5 -
drivers/net/ethernet/rocker/rocker_main.c | 5 -
drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 5 -
include/linux/netdevice.h | 3 ---
include/net/switchdev.h | 21 ---
net/dsa/slave.c | 5 -
9 files changed, 70 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index f018b0607dac..5d998517653a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3215,7 +3215,6 @@ static int mlxsw_sp_port_create(struct mlxsw_sp
*mlxsw_sp, u8 local_port,
}
mlxsw_sp_port->default_vlan = mlxsw_sp_port_vlan;
- mlxsw_sp_port_switchdev_init(mlxsw_sp_port);
mlxsw_sp->ports[local_port] = mlxsw_sp_port;
err = register_netdev(dev);
if (err) {
@@ -3232,7 +3231,6 @@ static int mlxsw_sp_port_create(struct mlxsw_sp
*mlxsw_sp, u8 local_port,
err_register_netdev:
mlxsw_sp->ports[local_port] = NULL;
- mlxsw_sp_port_switchdev_fini(mlxsw_sp_port);
mlxsw_sp_port_vlan_destroy(mlxsw_sp_port_vlan);
err_port_vlan_create:
err_port_pvid_set:
@@ -3275,7 +3273,6 @@ static void mlxsw_sp_port_remove(struct mlxsw_sp
*mlxsw_sp, u8 local_port)
mlxsw_core_port_clear(mlxsw_sp->core, local_port, mlxsw_sp);
unregister_netdev(mlxsw_sp_port->dev); /* This calls ndo_stop */
mlxsw_sp->ports[local_port] = NULL;
- mlxsw_sp_port_switchdev_fini(mlxsw_sp_port);
mlxsw_sp_port_vlan_flush(mlxsw_sp_port, true);
mlxsw_sp_port_nve_fini(mlxsw_sp_port);
mlxsw_sp_tc_qdisc_fini(mlxsw_sp_port);
@@ -3996,12 +3993,6 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
goto err_span_init;
}
- err = mlxsw_sp_switchdev_init(mlxsw_sp);
- if (err) {
- dev_err(mlxsw_sp->bus_info->dev, "Failed to initialize
switchdev\n");
- goto err_switchdev_init;
- }
-
err = mlxsw_sp_counter_pool_init(mlxsw_sp);
if (err) {
dev_err(mlxsw_sp->bus_info->dev, "Failed to init counter
pool\n");
@@ -4072,8 +4063,6 @@ static int mlxsw_sp_init(struct mlxsw_core *mlxsw_core,
err_afa_init:
mlxsw_sp_counter_pool_fini(mlxsw_sp);
err_counter_pool_init:
- mlxsw_sp_switchdev_fini(mlxsw_sp);
-err_switchdev_init:
mlxsw_sp_span_fini(mlxsw_sp);
err_span_init:
mlxsw_sp_lag_fini(mlxsw_sp);
@@ -4138,7 +4127,6 @@ static void mlxsw_sp_fini(struct mlxsw_core *mlxsw_core)
mlxsw_sp_nve_fini(mlxsw_sp);
mlxsw_sp_afa_fini(mlxsw_sp);
mlxsw_sp_counter_pool_fini(mlxsw_sp);
- mlxsw_sp_switchdev_fini(mlxsw_sp);
mlxsw_sp_span_fini(mlxsw_sp);
mlxsw_sp_lag_fini(mlxsw_sp);
mlxsw_sp_buffers_fini(mlxsw_sp);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 8bb83d0facc2..b1342fe9ca48 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -381,8 +381,6 @@ extern const struct mlxsw_sp_sb_vals mlxsw_sp2_sb_vals;
/* spectrum_switchdev.c */
int mlxsw_sp_switchdev_init(struct mlxsw_sp *mlxsw_sp);
void mlxsw_sp_switchdev_fini(struct mlxsw_sp *mlxsw_sp);
-void mlxsw_sp_port_switchdev_init(struct mlxsw_sp_port *mlxsw_sp_port);
-void mlxsw_sp_port_switchdev_fini(struct mlxsw_sp_port *mlxsw_sp_port);
int mlxsw_sp_rif_fdb_op(struct mlxsw_sp *mlxsw_sp, const char *mac, u16 fid,
bool adding);
void
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
index c1aedfea3a31..f6ce386c3036 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_switchdev.c
@@ -1938,10 +1938,6 @@ static struct mlxsw_sp_port
*mlxsw_sp_lag_rep_port(struct mlxsw_sp *mlxsw_sp,
return NULL;
}
-static const struct switchdev_ops mlxsw_sp_port_switchdev_ops = {
- .switchdev_port_attr_set= mlxsw_sp_port_attr_set,
-};
-
static int
mlxsw_sp_bridge_8021q_port_join(struct mlxsw_sp_bridge_device *bridge_device,
struct mlxsw_sp_bridge_port *bridge_port,
@@ -3545,11 +3541,3 @@ void mlxsw_sp_switchdev_fini(struct mlxsw_sp *mlxsw_sp)
kfree(mlxsw_sp->bridge);
}
-void
[PATCH net-next 2/8] rocker: Handle SWITCHDEV_PORT_ATTR_SET
Following patches will change the way we communicate setting a port's
attribute and use notifiers towards that goal.
Prepare rocker to support receiving notifier events targeting
SWITCHDEV_PORT_ATTR_SET from both atomic and process context and use a
small helper to translate the event notifier into something that
rocker_port_attr_set() can process.
Signed-off-by: Florian Fainelli
---
drivers/net/ethernet/rocker/rocker_main.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/net/ethernet/rocker/rocker_main.c
b/drivers/net/ethernet/rocker/rocker_main.c
index 309a6bf9130c..fc772cf079cc 100644
--- a/drivers/net/ethernet/rocker/rocker_main.c
+++ b/drivers/net/ethernet/rocker/rocker_main.c
@@ -2710,6 +2710,19 @@ static bool rocker_port_dev_check(const struct
net_device *dev)
return dev->netdev_ops == _port_netdev_ops;
}
+static int
+rocker_switchdev_port_attr_set_event(struct net_device *netdev,
+ struct switchdev_notifier_port_attr_info *port_attr_info)
+{
+ int err;
+
+ err = rocker_port_attr_set(netdev, port_attr_info->attr,
+ port_attr_info->trans);
+
+ port_attr_info->handled = true;
+ return notifier_from_errno(err);
+}
+
struct rocker_switchdev_event_work {
struct work_struct work;
struct switchdev_notifier_fdb_info fdb_info;
@@ -2779,6 +2792,9 @@ static int rocker_switchdev_event(struct notifier_block
*unused,
if (!rocker_port_dev_check(dev))
return NOTIFY_DONE;
+ if (event == SWITCHDEV_PORT_ATTR_SET)
+ return rocker_switchdev_port_attr_set_event(dev, ptr);
+
rocker_port = netdev_priv(dev);
switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
if (WARN_ON(!switchdev_work))
@@ -2841,6 +2857,8 @@ static int rocker_switchdev_blocking_event(struct
notifier_block *unused,
case SWITCHDEV_PORT_OBJ_ADD:
case SWITCHDEV_PORT_OBJ_DEL:
return rocker_switchdev_port_obj_event(event, dev, ptr);
+ case SWITCHDEV_PORT_ATTR_SET:
+ return rocker_switchdev_port_attr_set_event(dev, ptr);
}
return NOTIFY_DONE;
--
2.17.1
[PATCH net-next 3/8] net: dsa: Handle SWITCHDEV_PORT_ATTR_SET
Following patches will change the way we communicate setting a port's
attribute and use notifiers towards that goal.
Prepare DSA to support receiving notifier events targeting
SWITCHDEV_PORT_ATTR_SET from both atomic and process context and use a
small helper to translate the event notifier into something that
dsa_slave_port_attr_set() can process.
Signed-off-by: Florian Fainelli
---
net/dsa/slave.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a78b2bba0332..49a5b29fe884 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1422,6 +1422,19 @@ static int dsa_slave_netdevice_event(struct
notifier_block *nb,
return NOTIFY_DONE;
}
+static int
+dsa_slave_switchdev_port_attr_set_event(struct net_device *netdev,
+ struct switchdev_notifier_port_attr_info *port_attr_info)
+{
+ int err;
+
+ err = dsa_slave_port_attr_set(netdev, port_attr_info->attr,
+ port_attr_info->trans);
+
+ port_attr_info->handled = true;
+ return notifier_from_errno(err);
+}
+
struct dsa_switchdev_event_work {
struct work_struct work;
struct switchdev_notifier_fdb_info fdb_info;
@@ -1500,6 +1513,9 @@ static int dsa_slave_switchdev_event(struct
notifier_block *unused,
if (!dsa_slave_dev_check(dev))
return NOTIFY_DONE;
+ if (event == SWITCHDEV_PORT_ATTR_SET)
+ return dsa_slave_switchdev_port_attr_set_event(dev, ptr);
+
switchdev_work = kzalloc(sizeof(*switchdev_work), GFP_ATOMIC);
if (!switchdev_work)
return NOTIFY_BAD;
@@ -1562,6 +1578,8 @@ static int dsa_slave_switchdev_blocking_event(struct
notifier_block *unused,
case SWITCHDEV_PORT_OBJ_ADD: /* fall through */
case SWITCHDEV_PORT_OBJ_DEL:
return dsa_slave_switchdev_port_obj_event(event, dev, ptr);
+ case SWITCHDEV_PORT_ATTR_SET:
+ return dsa_slave_switchdev_port_attr_set_event(dev, ptr);
}
return NOTIFY_DONE;
--
2.17.1
[PATCH net-next 1/8] switchdev: Add SWITCHDEV_PORT_ATTR_SET
In preparation for allowing switchdev enabled drivers to veto specific
attribute settings from within the context of the caller, introduce a
new switchdev notifier type for port attributes.
Suggested-by: Ido Schimmel
Signed-off-by: Florian Fainelli
---
include/net/switchdev.h | 27 +
net/switchdev/switchdev.c | 51 +++
2 files changed, 78 insertions(+)
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index 45310ddf2d7e..ca56b7487540 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -136,6 +136,7 @@ enum switchdev_notifier_type {
SWITCHDEV_PORT_OBJ_ADD, /* Blocking. */
SWITCHDEV_PORT_OBJ_DEL, /* Blocking. */
+ SWITCHDEV_PORT_ATTR_SET, /* May be blocking . */
SWITCHDEV_VXLAN_FDB_ADD_TO_BRIDGE,
SWITCHDEV_VXLAN_FDB_DEL_TO_BRIDGE,
@@ -164,6 +165,13 @@ struct switchdev_notifier_port_obj_info {
bool handled;
};
+struct switchdev_notifier_port_attr_info {
+ struct switchdev_notifier_info info; /* must be first */
+ const struct switchdev_attr *attr;
+ struct switchdev_trans *trans;
+ bool handled;
+};
+
static inline struct net_device *
switchdev_notifier_info_to_dev(const struct switchdev_notifier_info *info)
{
@@ -216,7 +224,15 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
int (*del_cb)(struct net_device *dev,
const struct switchdev_obj *obj));
+int switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans));
+
#define SWITCHDEV_SET_OPS(netdev, ops) ((netdev)->switchdev_ops = (ops))
+
#else
static inline void switchdev_deferred_process(void)
@@ -303,6 +319,17 @@ switchdev_handle_port_obj_del(struct net_device *dev,
return 0;
}
+static inline int
+switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans))
+{
+ return 0;
+}
+
#define SWITCHDEV_SET_OPS(netdev, ops) do {} while (0)
#endif
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index 7e1357db33d7..94400f5b8e07 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -697,3 +697,54 @@ int switchdev_handle_port_obj_del(struct net_device *dev,
return err;
}
EXPORT_SYMBOL_GPL(switchdev_handle_port_obj_del);
+
+static int __switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans))
+{
+ struct net_device *lower_dev;
+ struct list_head *iter;
+ int err = -EOPNOTSUPP;
+
+ if (check_cb(dev)) {
+ port_attr_info->handled = true;
+ return set_cb(dev, port_attr_info->attr,
+ port_attr_info->trans);
+ }
+
+ /* Switch ports might be stacked under e.g. a LAG. Ignore the
+* unsupported devices, another driver might be able to handle them. But
+* propagate to the callers any hard errors.
+*
+* If the driver does its own bookkeeping of stacked ports, it's not
+* necessary to go through this helper.
+*/
+ netdev_for_each_lower_dev(dev, lower_dev, iter) {
+ err = __switchdev_handle_port_attr_set(lower_dev,
port_attr_info,
+ check_cb, set_cb);
+ if (err && err != -EOPNOTSUPP)
+ return err;
+ }
+
+ return err;
+}
+
+int switchdev_handle_port_attr_set(struct net_device *dev,
+ struct switchdev_notifier_port_attr_info
*port_attr_info,
+ bool (*check_cb)(const struct net_device *dev),
+ int (*set_cb)(struct net_device *dev,
+ const struct switchdev_attr *attr,
+ struct switchdev_trans *trans))
+{
+ int err;
+
+ err = __switchdev_handle_port_attr_set(dev, port_attr_info, check_cb,
+
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, Feb 22, 2019 at 3:02 PM Jann Horn wrote: > > On Fri, Feb 22, 2019 at 11:39 PM Nadav Amit wrote: > > > On Feb 22, 2019, at 2:21 PM, Nadav Amit wrote: > > > > > >> On Feb 22, 2019, at 2:17 PM, Jann Horn wrote: > > >> > > >> On Fri, Feb 22, 2019 at 11:08 PM Nadav Amit wrote: > > On Feb 22, 2019, at 1:43 PM, Jann Horn wrote: > > > > (adding some people from the text_poke series to the thread, removing > > stable@) > > > > On Fri, Feb 22, 2019 at 8:55 PM Andy Lutomirski > > wrote: > > >> On Feb 22, 2019, at 11:34 AM, Alexei Starovoitov > > >> wrote: > > >>> On Fri, Feb 22, 2019 at 02:30:26PM -0500, Steven Rostedt wrote: > > >>> On Fri, 22 Feb 2019 11:27:05 -0800 > > >>> Alexei Starovoitov wrote: > > >>> > > > On Fri, Feb 22, 2019 at 09:43:14AM -0800, Linus Torvalds wrote: > > > > > > Then we should still probably fix up "__probe_kernel_read()" to > > > not > > > allow user accesses. The easiest way to do that is actually > > > likely to > > > use the "unsafe_get_user()" functions *without* doing a > > > uaccess_begin(), which will mean that modern CPU's will simply > > > fault > > > on a kernel access to user space. > > > > On bpf side the bpf_probe_read() helper just calls > > probe_kernel_read() > > and users pass both user and kernel addresses into it and expect > > that the helper will actually try to read from that address. > > > > If __probe_kernel_read will suddenly start failing on all user > > addresses > > it will break the expectations. > > How do we solve it in bpf_probe_read? > > Call probe_kernel_read and if that fails call unsafe_get_user > > byte-by-byte > > in the loop? > > That's doable, but people already complain that bpf_probe_read() > > is slow > > and shows up in their perf report. > > >>> > > >>> We're changing kprobes to add a specific flag to say that we want to > > >>> differentiate between kernel or user reads. Can this be done with > > >>> bpf_probe_read()? If it's showing up in perf report, I doubt a > > >>> single > > >> > > >> so you're saying you will break existing kprobe scripts? > > >> I don't think it's a good idea. > > >> It's not acceptable to break bpf_probe_read uapi. > > > > > > If so, the uapi is wrong: a long-sized number does not reliably > > > identify an address if you don’t separately know whether it’s a user > > > or kernel address. s390x and 4G:4G x86_32 are the notable exceptions. > > > I have lobbied for RISC-V and future x86_64 to join the crowd. I > > > don’t know whether I’ll win this fight, but the uapi will probably > > > have to change for at least s390x. > > > > > > What to do about existing scripts is a different question. > > > > This lack of logical separation between user and kernel addresses > > might interact interestingly with the text_poke series, specifically > > "[PATCH v3 05/20] x86/alternative: Initialize temporary mm for > > patching" > > (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-6-rick.p.edgecombe%40intel.com%2Fdata=02%7C01%7Cnamit%40vmware.com%7Cf2513009ef734ecd6b0d08d69913a5ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864707020821793sdata=HAbnDcrBne64JyPuVUMKmM7nQk67F%2BFvjuXEn8TmHeo%3Dreserved=0) > > and "[PATCH v3 06/20] x86/alternative: Use temporary mm for text > > poking" > > (https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20190221234451.17632-7-rick.p.edgecombe%40intel.com%2Fdata=02%7C01%7Cnamit%40vmware.com%7Cf2513009ef734ecd6b0d08d69913a5ae%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636864707020821793sdata=vNRIMKtFDy%2F3z5FlTwDiJY6VGEV%2FMHgQPTdFSFtCo4s%3Dreserved=0), > > right? If someone manages to get a tracing BPF program to trigger in a > > task that has switched to the patching mm, could they use > > bpf_probe_write_user() - which uses probe_kernel_write() after > > checking that KERNEL_DS isn't active and that access_ok() passes - to > > overwrite kernel text that is mapped writable in the patching mm? > > >>> > > >>> Yes, this is a good point. I guess text_poke() should be defined with > > >>> “__kprobes” and open-code memcpy. > > >>> > > >>> Does it sound reasonable? > > >> > > >> Doesn't __text_poke() as implemented in the proposed patch use a > > >> couple other kernel functions, too? Like switch_mm_irqs_off() and > > >> pte_clear() (which can be a call into a separate function on paravirt > > >> kernels)? > > > > > > I will move the pte_clear() to be done after the poking mm was unloaded. > > > Give me a few minutes to send a sketch of what I
Re: [PATCH v2 08/15] clock: milbeaut: Add Milbeaut M10V clock controller
Quoting Sugaya Taichi (2019-02-08 04:27:17)
> diff --git a/drivers/clk/clk-milbeaut.c b/drivers/clk/clk-milbeaut.c
> new file mode 100644
> index 000..f798939
> --- /dev/null
> +++ b/drivers/clk/clk-milbeaut.c
> @@ -0,0 +1,626 @@
[]
> +struct m10v_clk_div_fixed_data {
> + const char *name;
> + const char *parent_name;
> + u8 div;
> + u8 mult;
> + int onecell_idx;
> +};
> +struct m10v_clk_mux_factors {
> + const char *name;
> + const char * const *parent_names;
> + u8 num_parents;
> + u32 offset;
> + u8 shift;
> + u8 mask;
> + u32 *table;
> + unsigned long mux_flags;
> + int onecell_idx;
> +};
Please add newlines between struct definitions. It also wouldn't hurt to
have kernel-doc on these.
> +
> +static const struct clk_div_table emmcclk_table[] = {
> + { .val = 0, .div = 8 },
> + { .val = 1, .div = 9 },
> + { .val = 2, .div = 10 },
> + { .val = 3, .div = 15 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table mclk400_table[] = {
> + { .val = 1, .div = 2 },
> + { .val = 3, .div = 4 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table mclk200_table[] = {
> + { .val = 3, .div = 4 },
> + { .val = 7, .div = 8 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table aclk400_table[] = {
> + { .val = 1, .div = 2 },
> + { .val = 3, .div = 4 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table aclk300_table[] = {
> + { .val = 0, .div = 2 },
> + { .val = 1, .div = 3 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table aclk_table[] = {
> + { .val = 3, .div = 4 },
> + { .val = 7, .div = 8 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table aclkexs_table[] = {
> + { .val = 3, .div = 4 },
> + { .val = 4, .div = 5 },
> + { .val = 5, .div = 6 },
> + { .val = 7, .div = 8 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table hclk_table[] = {
> + { .val = 7, .div = 8 },
> + { .val = 15, .div = 16 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table hclkbmh_table[] = {
> + { .val = 3, .div = 4 },
> + { .val = 7, .div = 8 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table pclk_table[] = {
> + { .val = 15, .div = 16 },
> + { .val = 31, .div = 32 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table rclk_table[] = {
> + { .val = 0, .div = 8 },
> + { .val = 1, .div = 16 },
> + { .val = 2, .div = 24 },
> + { .val = 3, .div = 32 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table uhs1clk0_table[] = {
> + { .val = 0, .div = 2 },
> + { .val = 1, .div = 3 },
> + { .val = 2, .div = 4 },
> + { .val = 3, .div = 8 },
> + { .val = 4, .div = 16 },
> + { .div = 0 },
> +};
> +static const struct clk_div_table uhs2clk_table[] = {
> + { .val = 0, .div = 9 },
> + { .val = 1, .div = 10 },
> + { .val = 2, .div = 11 },
> + { .val = 3, .div = 12 },
> + { .val = 4, .div = 13 },
> + { .val = 5, .div = 14 },
> + { .val = 6, .div = 16 },
> + { .val = 7, .div = 18 },
> + { .div = 0 },
> +};
Same comment applies here. Newlines between tables please.
> +
> +static u32 spi_mux_table[] = {0, 1, 2};
> +static const char * const spi_mux_names[] = {
> + M10V_SPI_PARENT0, M10V_SPI_PARENT1, M10V_SPI_PARENT2
> +};
> +
> +static u32 uhs1clk2_mux_table[] = {2, 3, 4, 8};
> +static const char * const uhs1clk2_mux_names[] = {
> + M10V_UHS1CLK2_PARENT0, M10V_UHS1CLK2_PARENT1,
> + M10V_UHS1CLK2_PARENT2, M10V_PLL6DIV2
> +};
> +
> +static u32 uhs1clk1_mux_table[] = {3, 4, 8};
> +static const char * const uhs1clk1_mux_names[] = {
> + M10V_UHS1CLK1_PARENT0, M10V_UHS1CLK1_PARENT1, M10V_PLL6DIV2
> +};
> +
[...]
> +
> +static const struct m10v_clk_mux_factors m10v_mux_factor_data[] = {
> + {"spi", spi_mux_names, ARRAY_SIZE(spi_mux_names),
> + CLKSEL(8), 3, 7, spi_mux_table, 0, M10V_SPICLK_ID},
> + {"uhs1clk2", uhs1clk2_mux_names, ARRAY_SIZE(uhs1clk2_mux_names),
> + CLKSEL(1), 13, 31, uhs1clk2_mux_table, 0, -1},
> + {"uhs1clk1", uhs1clk1_mux_names, ARRAY_SIZE(uhs1clk1_mux_names),
> + CLKSEL(1), 8, 31, uhs1clk1_mux_table, 0, -1},
> + {"nfclk", nfclk_mux_names, ARRAY_SIZE(nfclk_mux_names),
> + CLKSEL(1), 22, 127, nfclk_mux_table, 0, M10V_NFCLK_ID},
> +};
> +
> +static u8 m10v_mux_get_parent(struct clk_hw *hw)
> +{
> + struct clk_mux *mux = to_clk_mux(hw);
> + u32 val;
> +
> + val = clk_readl(mux->reg) >> mux->shift;
Please don't use
Re: [PATCH 1/2 v2] kprobe: Do not use uaccess functions to access kernel memory that can fault
On Fri, Feb 22, 2019 at 03:16:35PM -0800, Linus Torvalds wrote:
>
> So a kernel pointer value of 0x12345678 could be a value kernel
> pointer pointing to some random kmalloc'ed kernel memory, and a user
> pointer value of 0x12345678 could be a valid _user_ pointer pointing
> to some user mapping.
>
> See?
>
> If you access a user pointer, you need to use a user accessor function
> (eg "get_user()"), while if you access a kernel pointer you need to
> just dereference it directly (unless you can't trust it, in which case
> you need to use a _different_ accessor function).
that was clear already.
Reading 0x12345678 via probe_kernel_read can return valid value
and via get_user() can return another valid value on _some_ architectures.
> The fact that user and kernel pointers happen to be distinct on x86-64
> (right now) is just a random implementation detail.
yes and my point that people already rely on this implementation detail.
Say we implement
int bpf_probe_read(void *val, void *unsafe_ptr)
{
if (probe_kernel_read(val, unsafe_ptr) == OK) {
return 0;
} else (get_user(val, unsafe_ptr) == OK) {
return 0;
} else {
*val = 0;
return -EFAULT;
}
}
It will preserve existing bpf_probe_read() behavior on x86.
If x86 implementation changes tomorrow then progs that read user
addresses may start failing randomly because first probe_kernel_read()
will be returning random values from kernel memory and that's no good,
but at least we won't be breaking them today, so we have time to
introduce bpf_user_read and bpf_kernel_read and folks have time to adopt them.
Imo that's much better than making current bpf_probe_read() fail
on user addresses today and not providing a non disruptive path forward.
Re: [RFC][PATCH] objtool: STAC/CLAC validation
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra wrote:
>
> On Fri, Feb 22, 2019 at 07:10:34PM +0100, Thomas Gleixner wrote:
>
> > But correct :)
>
> > I agree, that a function which is doing the actual copy should be callable,
> > but random other functions? NO!
>
> So find the below patch -- which spotted fail in ptrace.c
>
> It has an AC_SAFE(func) annotation which allows marking specific
> functions as safe to call. The patch includes 2 instances which were
> required to make arch/x86 'build':
>
> arch/x86/ia32/ia32_signal.o: warning: objtool:
> ia32_restore_sigcontext()+0x3d: call to native_load_gs_index() with AC set
> arch/x86/kernel/ptrace.o: warning: objtool: genregs_get()+0x8e: call to
> getreg() with AC set
>
> It also screams (provided one has CONFIG_FUNCTION_TRACE=y) about the
> lack of notrace annotations on functions marked AC_SAFE():
>
> arch/x86/kernel/ptrace.o: warning: objtool: getreg()+0x0: call to
> __fentry__() with AC set
>
> It builds arch/x86 relatively clean; it only complains about some
> redundant CLACs in entry_64.S because it doesn't understand interrupts
> and I've not bothered creating an annotation for them yet.
>
> arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x4d:
> redundant CLAC
> arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0x5a:
> redundant CLAC
> ...
> arch/x86/entry/entry_64.o: warning: objtool: .altinstr_replacement+0xb1:
> redundant CLAC
Does objtool understand altinstr? If it understands that this is an
altinstr associated with a not-actually-a-fuction (i.e. END not
ENDPROC) piece of code, it could suppress this warning.
>
> -static unsigned long getreg(struct task_struct *task, unsigned long offset)
> +static notrace unsigned long getreg(struct task_struct *task, unsigned long
> offset)
> {
> switch (offset) {
> case offsetof(struct user_regs_struct, cs):
> @@ -444,6 +444,7 @@ static unsigned long getreg(struct task_struct *task,
> unsigned long offset)
>
> return *pt_regs_access(task_pt_regs(task), offset);
> }
> +AC_SAFE(getreg);
This takes the address and could plausibly suppress some optimizations.
>
> +#if defined(CONFIG_STACK_VALIDATION) && !defined(BUILD_VDSO) &&
> !defined(BUILD_VDSO32)
> +/*
> + * This macro marks functions as AC-safe, that is, it is safe to call from an
> + * EFLAGS.AC enabled region (typically user_access_begin() /
> + * user_access_end()).
> + *
> + * These functions in turn will only call AC-safe functions themselves (which
> + * precludes tracing, including __fentry__ and scheduling, including
> + * preempt_enable).
> + *
> + * AC-safe functions will obviously also not change EFLAGS.AC themselves.
> + *
> + * Since STAC/CLAC are OPL-0 only, this is all irrelevant for VDSO builds
> + * (and the generated symbol reference will in fact cause link failures).
> + */
> +#define AC_SAFE(func) \
> + static void __used __section(.discard.ac_safe) \
> + *__func_ac_safe_##func = func
Are we okay with annotating function *declarations* in a way that
their addresses get taken whenever the declaration is processed? It
would be nice if we could avoid this.
I'm wondering if we can just change the code that does getreg() and
load_gs_index() so it doesn't do it with AC set. Also, what about
paravirt kernels? They'll call into PV code for load_gs_index() with
AC set.
--Andy
Re: block: be more careful about status in __bio_chain_endio
On Fri, Feb 22 2019 at 5:46pm -0500,
Jens Axboe wrote:
> On 2/22/19 2:10 PM, Mike Snitzer wrote:
> > On Thu, Feb 15 2018 at 4:09am -0500,
> > NeilBrown wrote:
> >
> >>
> >> If two bios are chained under the one parent (with bio_chain())
> >> it is possible that one will succeed and the other will fail.
> >> __bio_chain_endio must ensure that the failure error status
> >> is reported for the whole, rather than the success.
> >>
> >> It currently tries to be careful, but this test is racy.
> >> If both children finish at the same time, they might both see that
> >> parent->bi_status as zero, and so will assign their own status.
> >> If the assignment to parent->bi_status by the successful bio happens
> >> last, the error status will be lost which can lead to silent data
> >> corruption.
> >>
> >> Instead, __bio_chain_endio should only assign a non-zero status
> >> to parent->bi_status. There is then no need to test the current
> >> value of parent->bi_status - a test that would be racy anyway.
> >>
> >> Note that this bug hasn't been seen in practice. It was only discovered
> >> by examination after a similar bug was found in dm.c
> >>
> >> Signed-off-by: NeilBrown
> >> ---
> >> block/bio.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/block/bio.c b/block/bio.c
> >> index e1708db48258..ad77140edc6f 100644
> >> --- a/block/bio.c
> >> +++ b/block/bio.c
> >> @@ -312,7 +312,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
> >> {
> >>struct bio *parent = bio->bi_private;
> >>
> >> - if (!parent->bi_status)
> >> + if (bio->bi_status)
> >>parent->bi_status = bio->bi_status;
> >>bio_put(bio);
> >>return parent;
> >> --
> >> 2.14.0.rc0.dirty
> >>
> >
> > Reviewed-by: Mike Snitzer
> >
> > Jens, this one slipped through the crack just over a year ago.
> > It is available in patchwork here:
> > https://patchwork.kernel.org/patch/10220727/
>
> Should this be:
>
> if (!parent->bi_status && bio->bi_status)
> parent->bi_status = bio->bi_status;
>
> perhaps?
Yeap, even better. Not seeing any reason to have the last error win,
the first in the chain is likely the most important.
Re: [PATCH v2 3/9] serial: core: add rs485-rts-delay-us devicetree property for RS485
On Thu, Feb 21, 2019 at 06:17:52PM +0100, Martin Kepplinger wrote: > struct serial_rs485 now optionally holds the rts delay values in > microseconds. Users can set these delays in their devicetree descriptions, > so this adds the microseconds-option with the "rs485-rts-delay-us" boolean > property. If it has a value, it's not boolean. Should the old prop be deprecated? > > Signed-off-by: Martin Kepplinger > --- > Documentation/devicetree/bindings/serial/rs485.txt | 1 + > drivers/tty/serial/serial_core.c | 11 +++ > 2 files changed, 12 insertions(+) > > diff --git a/Documentation/devicetree/bindings/serial/rs485.txt > b/Documentation/devicetree/bindings/serial/rs485.txt > index b92592dff6dd..77396c62b383 100644 > --- a/Documentation/devicetree/bindings/serial/rs485.txt > +++ b/Documentation/devicetree/bindings/serial/rs485.txt > @@ -12,6 +12,7 @@ Optional properties: >* b is the delay between end of data sent and rts signal in milliseconds >it corresponds to the delay after sending data and actual release of > the line. >If this property is not specified, <0 0> is assumed. > +- rs485-rts-delay-us: the same as rs485-rts-delay, but in microseconds. > - rs485-rts-active-low: drive RTS low when sending (default is high). > - linux,rs485-enabled-at-boot-time: empty property telling to enable the > rs485 >feature at boot time. It can be disabled later with proper ioctl.
Re: [PATCH v2 02/10] iio: document bindings for mounting matrices
On Thu, 21 Feb 2019 18:02:47 +0100, "H. Nikolaus Schaller" wrote:
> From: Linus Walleij
>
> The mounting matrix for sensors was introduced in
> commit dfc57732ad38 ("iio:core: mounting matrix support")
>
> However the device tree bindings are very terse and since this is
> a widely applicable property, we need a proper binding for it
> that the other bindings can reference. This will also be useful
> for other operating systems and sensor engineering at large.
>
> I think all 3D sensors should support it, the current situation
> is probably that the mounting information is confined in magic
> userspace components rather than using the mounting matrix, which
> is not good for portability and reuse.
>
> Cc: Linus Walleij
> Cc: Gregor Boirie
> Cc: Sebastian Reichel
> Cc: Samu Onkalo
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Linus Walleij
> Signed-off-by: H. Nikolaus Schaller
> ---
> .../devicetree/bindings/iio/mount-matrix.txt | 204 ++
> 1 file changed, 204 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt
>
Reviewed-by: Rob Herring
Re: [RFC][PATCH] objtool: STAC/CLAC validation
On Fri, Feb 22, 2019 at 2:26 PM Peter Zijlstra wrote: > > So find the below patch -- which spotted fail in ptrace.c > > It has an AC_SAFE(func) annotation which allows marking specific > functions as safe to call. The patch includes 2 instances which were > required to make arch/x86 'build': Looks sane enough to me. Can you make it do DF too while at it? I really think AC and DF are the same in this context. If you call an arbitrary function with DF set, things will very quickly go sideways (even if it might work in practice as long as the function just doesn't do a memcpy or similar) Linus
