Re: [git pull] drm for v4.7
On Mon, May 23, 2016 at 11:59 AM, Linus Torvaldswrote: > > I'll test this out and look what happens, but I hate getting different > results than what I'm told to expect. Hmm. I also get a lot of ./usr/include/drm/amdgpu_drm.h:38: userspace cannot reference function or variable defined in the kernel ./usr/include/drm/drm.h:63: userspace cannot reference function or variable defined in the kernel ./usr/include/drm/drm.h:699: userspace cannot reference function or variable defined in the kernel ... warnings with my allmodconfig build. They do seem to be due to checkpatch not really grokking the #if defined(__cplusplus) extern "C" { #endif and thinking that's a sign of a kernel function or variable declaration being exported to user space, but it's a bit annoying. Is there a patch pending for this that I'm not aware of, or is it just that nobody but me hates spurious warnings? Didn't this show up in linux-next? And if it _did_ show up in linux-next, why was the pull request not talking about it? Linus
Re: [git pull] drm for v4.7
On Mon, May 23, 2016 at 11:59 AM, Linus Torvalds wrote: > > I'll test this out and look what happens, but I hate getting different > results than what I'm told to expect. Hmm. I also get a lot of ./usr/include/drm/amdgpu_drm.h:38: userspace cannot reference function or variable defined in the kernel ./usr/include/drm/drm.h:63: userspace cannot reference function or variable defined in the kernel ./usr/include/drm/drm.h:699: userspace cannot reference function or variable defined in the kernel ... warnings with my allmodconfig build. They do seem to be due to checkpatch not really grokking the #if defined(__cplusplus) extern "C" { #endif and thinking that's a sign of a kernel function or variable declaration being exported to user space, but it's a bit annoying. Is there a patch pending for this that I'm not aware of, or is it just that nobody but me hates spurious warnings? Didn't this show up in linux-next? And if it _did_ show up in linux-next, why was the pull request not talking about it? Linus
Re: [PATCH] Fix RC5 decoding with Fintek CIR chipset
On Sat, May 14, 2016 at 06:01:26PM +0100, Jonathan McDowell wrote: >Fix RC5 decoding with Fintek CIR chipset > >Commit e87b540be2dd02552fb9244d50ae8b4e4619a34b tightened up the RC5 >decoding by adding a check for trailing silence to ensure a valid RC5 >command had been received. Unfortunately the trailer length checked was >10 units and the Fintek CIR device does not want to provide details of a >space longer than 6350us. This meant that RC5 remotes working on a >Fintek setup on 3.16 failed on 3.17 and later. Fix this by shortening >the trailer check to 6 units (allowing for a previous space in the >received remote command). > >Signed-off-by: Jonathan McDowellSigned-off-by: David Härdeman >Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=117221 >Cc: sta...@vger.kernel.org > >- >diff --git a/drivers/media/rc/ir-rc5-decoder.c >b/drivers/media/rc/ir-rc5-decoder.c >index 6ffe776..a0fd4e6 100644 >--- a/drivers/media/rc/ir-rc5-decoder.c >+++ b/drivers/media/rc/ir-rc5-decoder.c >@@ -29,7 +29,7 @@ > #define RC5_BIT_START (1 * RC5_UNIT) > #define RC5_BIT_END (1 * RC5_UNIT) > #define RC5X_SPACE(4 * RC5_UNIT) >-#define RC5_TRAILER (10 * RC5_UNIT) /* In reality, approx 100 */ >+#define RC5_TRAILER (6 * RC5_UNIT) /* In reality, approx 100 */ > > enum rc5_state { > STATE_INACTIVE, >- > >J. > >-- >What did the first punk rock girl wear to your school? > -- David Härdeman
Re: [PATCH] Fix RC5 decoding with Fintek CIR chipset
On Sat, May 14, 2016 at 06:01:26PM +0100, Jonathan McDowell wrote: >Fix RC5 decoding with Fintek CIR chipset > >Commit e87b540be2dd02552fb9244d50ae8b4e4619a34b tightened up the RC5 >decoding by adding a check for trailing silence to ensure a valid RC5 >command had been received. Unfortunately the trailer length checked was >10 units and the Fintek CIR device does not want to provide details of a >space longer than 6350us. This meant that RC5 remotes working on a >Fintek setup on 3.16 failed on 3.17 and later. Fix this by shortening >the trailer check to 6 units (allowing for a previous space in the >received remote command). > >Signed-off-by: Jonathan McDowell Signed-off-by: David Härdeman >Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=117221 >Cc: sta...@vger.kernel.org > >- >diff --git a/drivers/media/rc/ir-rc5-decoder.c >b/drivers/media/rc/ir-rc5-decoder.c >index 6ffe776..a0fd4e6 100644 >--- a/drivers/media/rc/ir-rc5-decoder.c >+++ b/drivers/media/rc/ir-rc5-decoder.c >@@ -29,7 +29,7 @@ > #define RC5_BIT_START (1 * RC5_UNIT) > #define RC5_BIT_END (1 * RC5_UNIT) > #define RC5X_SPACE(4 * RC5_UNIT) >-#define RC5_TRAILER (10 * RC5_UNIT) /* In reality, approx 100 */ >+#define RC5_TRAILER (6 * RC5_UNIT) /* In reality, approx 100 */ > > enum rc5_state { > STATE_INACTIVE, >- > >J. > >-- >What did the first punk rock girl wear to your school? > -- David Härdeman
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon, May 23, 2016 at 02:49:09PM -0400, Rik van Riel wrote: > On Mon, 2016-05-23 at 20:42 +0200, Michal Hocko wrote: > > On Mon 23-05-16 20:14:11, Ebru Akagunduz wrote: > > > > > > Currently khugepaged makes swapin readahead under > > > down_write. This patch supplies to make swapin > > > readahead under down_read instead of down_write. > > You are still keeping down_write. Can we do without it altogether? > > Blocking mmap_sem of a remote proces for write is certainly not nice. > > Maybe Andrea can explain why khugepaged requires > a down_write of mmap_sem? > > If it were possible to have just down_read that > would make the code a lot simpler. You need a down_write() to retract page table. We need to make sure that nobody sees the page table before we can replace it with huge pmd. -- Kirill A. Shutemov
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon, May 23, 2016 at 02:49:09PM -0400, Rik van Riel wrote: > On Mon, 2016-05-23 at 20:42 +0200, Michal Hocko wrote: > > On Mon 23-05-16 20:14:11, Ebru Akagunduz wrote: > > > > > > Currently khugepaged makes swapin readahead under > > > down_write. This patch supplies to make swapin > > > readahead under down_read instead of down_write. > > You are still keeping down_write. Can we do without it altogether? > > Blocking mmap_sem of a remote proces for write is certainly not nice. > > Maybe Andrea can explain why khugepaged requires > a down_write of mmap_sem? > > If it were possible to have just down_read that > would make the code a lot simpler. You need a down_write() to retract page table. We need to make sure that nobody sees the page table before we can replace it with huge pmd. -- Kirill A. Shutemov
Re: [git pull] drm for v4.7
On Sun, May 22, 2016 at 11:41 PM, Dave Airliewrote: > > Here's the main drm pull request for 4.7, it's been > a busy one, and I've been a bit more distracted in > real life this merge window. Hmm. I pulled this, but I think I'll have to unpull again. Neither the diffstat not the shortlog match what you sent me. There's four extra commits at the top that aren't mentioned: Dave Airlie (3): drm/edid: move displayid tiled block parsing into separate function. drm/edid: move displayid validation to it's own function. drm/edid: add displayid detailed 1 timings to the modelist. (v1.1) Tomas Bzatek (1): drm/displayid: Iterate over all DisplayID blocks was that intentional? What happened? Are those commits meant to be merged, or are they wrong? They _look_ ok, but dammit, according to your message they shouldn't be there. I'll test this out and look what happens, but I hate getting different results than what I'm told to expect. This is one reason I much prefer getting explicit tags rather than a random branch. Did you update the branch on purpose and wanted me to get the new state, or did you update the branch just because you happened to do development on that branch and pushed it out? With an explicit tag, there's a much more _intentional_ "push this to Linus" thing going on, and it's less ambiguous in cases like this. Linus
Re: [git pull] drm for v4.7
On Sun, May 22, 2016 at 11:41 PM, Dave Airlie wrote: > > Here's the main drm pull request for 4.7, it's been > a busy one, and I've been a bit more distracted in > real life this merge window. Hmm. I pulled this, but I think I'll have to unpull again. Neither the diffstat not the shortlog match what you sent me. There's four extra commits at the top that aren't mentioned: Dave Airlie (3): drm/edid: move displayid tiled block parsing into separate function. drm/edid: move displayid validation to it's own function. drm/edid: add displayid detailed 1 timings to the modelist. (v1.1) Tomas Bzatek (1): drm/displayid: Iterate over all DisplayID blocks was that intentional? What happened? Are those commits meant to be merged, or are they wrong? They _look_ ok, but dammit, according to your message they shouldn't be there. I'll test this out and look what happens, but I hate getting different results than what I'm told to expect. This is one reason I much prefer getting explicit tags rather than a random branch. Did you update the branch on purpose and wanted me to get the new state, or did you update the branch just because you happened to do development on that branch and pushed it out? With an explicit tag, there's a much more _intentional_ "push this to Linus" thing going on, and it's less ambiguous in cases like this. Linus
[PATCH] drm/i915/ilk: Disable SSC for DPLLs if we're not using it
Thanks to Ville Syrjälä for pointing me towards the cause of this issue. Unfortunately one of the sideaffects of having the refclk for a DPLL set to SSC is that as long as it's set to SSC, the GPU will prevent us from powering down any of the pipes or transcoders using it. A couple of BIOSes, despite the fact they don't actually need it, enable SSC both in PCH_DREF_CONTROL and on the DPLL configurations. This causes issues on the first modeset, since we don't expect SSC to be left on and as a result, can't successfully power down the pipes or the transcoders using it. Here's an example from this Dell OptiPlex 990: [drm:intel_modeset_init] SSC enabled by BIOS, overriding VBT which says disabled [drm:intel_modeset_init] 2 display pipes available. [drm:intel_update_cdclk] Current CD clock rate: 40 kHz [drm:intel_update_max_cdclk] Max CD clock rate: 40 kHz [drm:intel_update_max_cdclk] Max dotclock rate: 36 kHz vgaarb: device changed decodes: PCI::00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem [drm:intel_crt_reset] crt adpa set to 0xf4 [drm:intel_dp_init_connector] Adding DP connector on port C [drm:intel_dp_aux_init] registering DPDDC-C bus for card0-DP-1 [drm:ironlake_init_pch_refclk] has_panel 0 has_lvds 0 has_ck505 0 [drm:ironlake_init_pch_refclk] Disabling SSC entirely … later we try committing the first modeset … [drm:intel_dump_pipe_config] [CRTC:26][modeset] config 88041b02e800 for pipe A [drm:intel_dump_pipe_config] cpu_transcoder: A … [drm:intel_dump_pipe_config] dpll_hw_state: dpll: 0xc4016001, dpll_md: 0x0, fp0: 0x20e08, fp1: 0x30d07 [drm:intel_dump_pipe_config] planes on this crtc [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0 idx: 0 enabled [drm:intel_dump_pipe_config] FB:42, fb = 800x600 format = 0x34325258 [drm:intel_dump_pipe_config] scaler:0 src (0, 0) 800x600 dst (0, 0) 800x600 [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx: 1 disabled, scaler_id = 0 [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1 idx: 2 disabled, scaler_id = 0 [drm:intel_get_shared_dpll] CRTC:26 allocated PCH DPLL A [drm:intel_get_shared_dpll] using PCH DPLL A for pipe A [drm:ilk_audio_codec_disable] Disable audio codec on port C, pipe A [drm:intel_disable_pipe] disabling pipe A [ cut here ] WARNING: CPU: 1 PID: 130 at drivers/gpu/drm/i915/intel_display.c:1146 intel_disable_pipe+0x297/0x2d0 [i915] pipe_off wait timed out … ---[ end trace 94fc8aa03ae139e8 ]--- [drm:intel_dp_link_down] [drm:ironlake_crtc_disable [i915]] *ERROR* failed to disable transcoder A Later modesets succeed since they reset the DPLL's configuration anyway, but this is enough to get stuck with a big fat warning in dmesg. Normally we'd need to add some sort of ref counting mechanism to the CRTCs so we avoid accidentally trying to shut off a shared resource, but since ignore what the BIOS does anyway and we shut off the refclks before any modesets, this workaround should suffice for now. Cc: sta...@vger.kernel.org Signed-off-by: Lyude--- drivers/gpu/drm/i915/intel_display.c | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d500e6f..3fb6025 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8230,7 +8230,8 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_encoder *encoder; - u32 val, final; + int i; + u32 val, temp, final; bool has_lvds = false; bool has_cpu_edp = false; bool has_panel = false; @@ -8369,6 +8370,22 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) I915_WRITE(PCH_DREF_CONTROL, val); POSTING_READ(PCH_DREF_CONTROL); udelay(200); + + /* Unset SSC as the refclk for all of the DPLLs */ + for (i = 0; i < dev_priv->num_shared_dpll; i++) { + temp = I915_READ(PCH_DPLL(i)); + + if (!(temp & PLLB_REF_INPUT_SPREADSPECTRUMIN)) + continue; + + DRM_DEBUG_KMS("Disabling SSC refclk for %s\n", + dev_priv->shared_dplls[i].name); + + /* Change refclk to DREFCLK */ + temp &= ~PLL_REF_INPUT_MASK; + + I915_WRITE(PCH_DPLL(i), temp); + } } BUG_ON(val != final); -- 2.5.5
[PATCH] drm/i915/ilk: Disable SSC for DPLLs if we're not using it
Thanks to Ville Syrjälä for pointing me towards the cause of this issue. Unfortunately one of the sideaffects of having the refclk for a DPLL set to SSC is that as long as it's set to SSC, the GPU will prevent us from powering down any of the pipes or transcoders using it. A couple of BIOSes, despite the fact they don't actually need it, enable SSC both in PCH_DREF_CONTROL and on the DPLL configurations. This causes issues on the first modeset, since we don't expect SSC to be left on and as a result, can't successfully power down the pipes or the transcoders using it. Here's an example from this Dell OptiPlex 990: [drm:intel_modeset_init] SSC enabled by BIOS, overriding VBT which says disabled [drm:intel_modeset_init] 2 display pipes available. [drm:intel_update_cdclk] Current CD clock rate: 40 kHz [drm:intel_update_max_cdclk] Max CD clock rate: 40 kHz [drm:intel_update_max_cdclk] Max dotclock rate: 36 kHz vgaarb: device changed decodes: PCI::00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem [drm:intel_crt_reset] crt adpa set to 0xf4 [drm:intel_dp_init_connector] Adding DP connector on port C [drm:intel_dp_aux_init] registering DPDDC-C bus for card0-DP-1 [drm:ironlake_init_pch_refclk] has_panel 0 has_lvds 0 has_ck505 0 [drm:ironlake_init_pch_refclk] Disabling SSC entirely … later we try committing the first modeset … [drm:intel_dump_pipe_config] [CRTC:26][modeset] config 88041b02e800 for pipe A [drm:intel_dump_pipe_config] cpu_transcoder: A … [drm:intel_dump_pipe_config] dpll_hw_state: dpll: 0xc4016001, dpll_md: 0x0, fp0: 0x20e08, fp1: 0x30d07 [drm:intel_dump_pipe_config] planes on this crtc [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0 idx: 0 enabled [drm:intel_dump_pipe_config] FB:42, fb = 800x600 format = 0x34325258 [drm:intel_dump_pipe_config] scaler:0 src (0, 0) 800x600 dst (0, 0) 800x600 [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx: 1 disabled, scaler_id = 0 [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1 idx: 2 disabled, scaler_id = 0 [drm:intel_get_shared_dpll] CRTC:26 allocated PCH DPLL A [drm:intel_get_shared_dpll] using PCH DPLL A for pipe A [drm:ilk_audio_codec_disable] Disable audio codec on port C, pipe A [drm:intel_disable_pipe] disabling pipe A [ cut here ] WARNING: CPU: 1 PID: 130 at drivers/gpu/drm/i915/intel_display.c:1146 intel_disable_pipe+0x297/0x2d0 [i915] pipe_off wait timed out … ---[ end trace 94fc8aa03ae139e8 ]--- [drm:intel_dp_link_down] [drm:ironlake_crtc_disable [i915]] *ERROR* failed to disable transcoder A Later modesets succeed since they reset the DPLL's configuration anyway, but this is enough to get stuck with a big fat warning in dmesg. Normally we'd need to add some sort of ref counting mechanism to the CRTCs so we avoid accidentally trying to shut off a shared resource, but since ignore what the BIOS does anyway and we shut off the refclks before any modesets, this workaround should suffice for now. Cc: sta...@vger.kernel.org Signed-off-by: Lyude --- drivers/gpu/drm/i915/intel_display.c | 19 ++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d500e6f..3fb6025 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8230,7 +8230,8 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_encoder *encoder; - u32 val, final; + int i; + u32 val, temp, final; bool has_lvds = false; bool has_cpu_edp = false; bool has_panel = false; @@ -8369,6 +8370,22 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) I915_WRITE(PCH_DREF_CONTROL, val); POSTING_READ(PCH_DREF_CONTROL); udelay(200); + + /* Unset SSC as the refclk for all of the DPLLs */ + for (i = 0; i < dev_priv->num_shared_dpll; i++) { + temp = I915_READ(PCH_DPLL(i)); + + if (!(temp & PLLB_REF_INPUT_SPREADSPECTRUMIN)) + continue; + + DRM_DEBUG_KMS("Disabling SSC refclk for %s\n", + dev_priv->shared_dplls[i].name); + + /* Change refclk to DREFCLK */ + temp &= ~PLL_REF_INPUT_MASK; + + I915_WRITE(PCH_DPLL(i), temp); + } } BUG_ON(val != final); -- 2.5.5
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon, 2016-05-23 at 20:42 +0200, Michal Hocko wrote: > On Mon 23-05-16 20:14:11, Ebru Akagunduz wrote: > > > > Currently khugepaged makes swapin readahead under > > down_write. This patch supplies to make swapin > > readahead under down_read instead of down_write. > You are still keeping down_write. Can we do without it altogether? > Blocking mmap_sem of a remote proces for write is certainly not nice. Maybe Andrea can explain why khugepaged requires a down_write of mmap_sem? If it were possible to have just down_read that would make the code a lot simpler. -- All Rights Reversed. signature.asc Description: This is a digitally signed message part
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon, 2016-05-23 at 20:42 +0200, Michal Hocko wrote: > On Mon 23-05-16 20:14:11, Ebru Akagunduz wrote: > > > > Currently khugepaged makes swapin readahead under > > down_write. This patch supplies to make swapin > > readahead under down_read instead of down_write. > You are still keeping down_write. Can we do without it altogether? > Blocking mmap_sem of a remote proces for write is certainly not nice. Maybe Andrea can explain why khugepaged requires a down_write of mmap_sem? If it were possible to have just down_read that would make the code a lot simpler. -- All Rights Reversed. signature.asc Description: This is a digitally signed message part
Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE
On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote: > On 05/18/2016 12:58 PM, Jason Low wrote: > > It should be fine to use the standard READ_ONCE here, even if it's just > > for documentation, as it's probably not going to cost anything in > > practice. It would be better to avoid adding any special macros for this > > which may just add more complexity. > > See, I don't understand this line of reasoning at all. > > I read this as "it's ok to be non-optimal here where were spinning CPU > time but not ok to be non-optimal generally elsewhere where it's > way less important like at init time". So I think there is a difference between using it during init time and using it here where we're spinning. During init time, initializing the owner field locklessly is normal. No other thread should be concurrently be writing to the field, since the structure is just getting initialized, so there are no surprises there. Our access of the owner field in this function is special in that we're using a bit of "lockless magic" to read and write to a field that gets concurrently accessed without any serialization. Since we're not taking the wait_lock in a scenario where we'd normally would take a lock, it would be good to have this documented. > And by the way, it's not just "here" but _everywhere_. > What about reading ->on_cpu locklessly? Sure, we could also use READ_ONCE when reading ->on_cpu :)
Re: [PATCH v4 2/5] locking/rwsem: Protect all writes to owner by WRITE_ONCE
On Sat, 2016-05-21 at 09:04 -0700, Peter Hurley wrote: > On 05/18/2016 12:58 PM, Jason Low wrote: > > It should be fine to use the standard READ_ONCE here, even if it's just > > for documentation, as it's probably not going to cost anything in > > practice. It would be better to avoid adding any special macros for this > > which may just add more complexity. > > See, I don't understand this line of reasoning at all. > > I read this as "it's ok to be non-optimal here where were spinning CPU > time but not ok to be non-optimal generally elsewhere where it's > way less important like at init time". So I think there is a difference between using it during init time and using it here where we're spinning. During init time, initializing the owner field locklessly is normal. No other thread should be concurrently be writing to the field, since the structure is just getting initialized, so there are no surprises there. Our access of the owner field in this function is special in that we're using a bit of "lockless magic" to read and write to a field that gets concurrently accessed without any serialization. Since we're not taking the wait_lock in a scenario where we'd normally would take a lock, it would be good to have this documented. > And by the way, it's not just "here" but _everywhere_. > What about reading ->on_cpu locklessly? Sure, we could also use READ_ONCE when reading ->on_cpu :)
[PATCH] misc: at24: Fix typo in at24 header file
This commit fixes a simple typo s/mvmem/nvmem in the example. Signed-off-by: Moritz Fischer--- include/linux/platform_data/at24.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h index dc9a13e..be830b1 100644 --- a/include/linux/platform_data/at24.h +++ b/include/linux/platform_data/at24.h @@ -26,7 +26,7 @@ * * An example in pseudo code for a setup() callback: * - * void get_mac_addr(struct mvmem_device *nvmem, void *context) + * void get_mac_addr(struct nvmem_device *nvmem, void *context) * { * u8 *mac_addr = ethernet_pdata->mac_addr; * off_t offset = context; -- 2.5.5
[PATCH] misc: at24: Fix typo in at24 header file
This commit fixes a simple typo s/mvmem/nvmem in the example. Signed-off-by: Moritz Fischer --- include/linux/platform_data/at24.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h index dc9a13e..be830b1 100644 --- a/include/linux/platform_data/at24.h +++ b/include/linux/platform_data/at24.h @@ -26,7 +26,7 @@ * * An example in pseudo code for a setup() callback: * - * void get_mac_addr(struct mvmem_device *nvmem, void *context) + * void get_mac_addr(struct nvmem_device *nvmem, void *context) * { * u8 *mac_addr = ethernet_pdata->mac_addr; * off_t offset = context; -- 2.5.5
RE: livepatch: change to a per-task consistency model
On Mon, 23 May 2016, David Laight wrote: > Related, please can we have a flag for the sleep and/or process so that > an uninterruptible sleep doesn't trigger the 'hung task' detector TASK_KILLABLE > and also stops the process counting towards the 'load average'. TASK_NOLOAD -- Jiri Kosina SUSE Labs
RE: livepatch: change to a per-task consistency model
On Mon, 23 May 2016, David Laight wrote: > Related, please can we have a flag for the sleep and/or process so that > an uninterruptible sleep doesn't trigger the 'hung task' detector TASK_KILLABLE > and also stops the process counting towards the 'load average'. TASK_NOLOAD -- Jiri Kosina SUSE Labs
Re: [PATCH 3/4] x86: Rewrite switch_to() code
On Mon, May 23, 2016 at 10:03:54AM -0700, Andy Lutomirski wrote: > On Mon, May 23, 2016 at 9:46 AM, Josh Poimboeufwrote: > > On Mon, May 23, 2016 at 06:49:03AM -0500, Josh Poimboeuf wrote: > >> On Mon, May 23, 2016 at 06:47:22AM -0500, Josh Poimboeuf wrote: > >> > On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote: > >> > > On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf > >> > > wrote: > >> > > > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote: > >> > > >> cc: Josh Poimboeuf: do you care about the exact stack layout of the > >> > > >> bottom of the stack of an inactive task? > >> > > > > >> > > > So there's one minor issue with this patch, relating to unwinding the > >> > > > stack of a newly forked task. For detecting reliable stacks, the > >> > > > unwinder needs to unwind all the way to the syscall pt_regs to make > >> > > > sure > >> > > > the stack is sane. But for newly forked tasks, that won't be > >> > > > possible > >> > > > here because the unwinding will stop at the fork_frame instead. > >> > > > > >> > > > So from an unwinder standpoint it might be nice for > >> > > > copy_thread_tls() to > >> > > > place a frame pointer on the stack next to the ret_from_fork return > >> > > > address, so that it would resemble an actual stack frame. The frame > >> > > > pointer could probably just be hard-coded to zero. And then the > >> > > > first > >> > > > bp in fork_frame would need to be a pointer to it instead of zero. > >> > > > That > >> > > > would make it nicely resemble the stack of any other task. > >> > > > > >> > > > Alternatively I could teach the unwinder that if the unwinding > >> > > > starts at > >> > > > the fork_frame offset from the end of the stack page, and the saved > >> > > > rbp > >> > > > is zero, it can assume that it's a newly forked task. But that > >> > > > seems a > >> > > > little more brittle to me, as it requires the unwinder to understand > >> > > > more of the internal workings of the fork code. > >> > > > > >> > > > But overall I think this patch is a really nice cleanup, and other > >> > > > than > >> > > > the above minor issue it should be fine with my reliable unwinder, > >> > > > since > >> > > > rbp is still at the top of the stack. > >> > > > >> > > Ok, how about if it pushed RBP first, then we teach get_wchan() to add > >> > > the fixed offset from thread.sp to get bp? that way it don't have to > >> > > push it twice. > >> > > >> > In theory I like the idea, and it would work: the unwinder could just > >> > use the inactive_task_frame struct (as Andy suggested) to find the frame > >> > pointer. > >> > > >> > But I suspect it would break all existing unwinders, both in-tree and > >> > out-of-tree. The only out-of-tree one I know of is crash, not sure if > >> > there are more out there. > >> > >> I should mention it would only affect those unwinders which know how to > >> do sleeping kernel tasks. So generic tools like gdb wouldn't be > >> affected. > > > > [continuing my conversation with myself...] > > > > To clarify, I still think we should do it. The stack format of a > > sleeping task isn't exactly an ABI, and I wouldn't expect many tools to > > rely on it. I can help with the fixing of in-tree unwinders if needed. > > > > Or I could even do the moving of the frame pointer as a separate patch > > on top of this one, since it might cause breakage elsewhere. > > Do you have any understanding of why there are so many unwinder > implementations? Your reliable unwinder seems to be yet another copy > of more or less the same code. Yeah, there are way too many instantations of stacktrace_ops and there's definitely a lot of room for consolidation and simplification. There are different requirements needed by all the different codes relying on dump_trace(): - starting with a given pt_regs - starting with a given task - whether to skip sched code functions - whether to skip the random '?' ktext addresses found on the stack - whether frame pointers are enabled - whether the stack is reliable - output to an arch-independent "struct stack_trace" array So everybody implements their own callbacks for dump_trace(). It's kind of a big mess. > I'd like to see a single, high-quality unwinder implemented as a state > machine, along the lines of: > > struct unwind_state state; > unwind_start_inactive_task(, ...); or > unwind_start_pt_regs(, regs); or whatever. > unwind_next_frame(); > > where, after unwind_next_frame, state encodes whatever registers are > known (at least bp and ip, but all the GPRs would be nice and are > probably mandatory for DWARF) and an indication of whether this is a > real frame or a guessed frame (the things that currently show up as > '?'). I like the idea of a state machine. I'll probably end up doing something like that before introducing the DWARF unwinder. -- Josh
Re: [PATCH 3/4] x86: Rewrite switch_to() code
On Mon, May 23, 2016 at 10:03:54AM -0700, Andy Lutomirski wrote: > On Mon, May 23, 2016 at 9:46 AM, Josh Poimboeuf wrote: > > On Mon, May 23, 2016 at 06:49:03AM -0500, Josh Poimboeuf wrote: > >> On Mon, May 23, 2016 at 06:47:22AM -0500, Josh Poimboeuf wrote: > >> > On Mon, May 23, 2016 at 07:14:14AM -0400, Brian Gerst wrote: > >> > > On Sun, May 22, 2016 at 10:34 PM, Josh Poimboeuf > >> > > wrote: > >> > > > On Sun, May 22, 2016 at 10:59:38AM -0700, Andy Lutomirski wrote: > >> > > >> cc: Josh Poimboeuf: do you care about the exact stack layout of the > >> > > >> bottom of the stack of an inactive task? > >> > > > > >> > > > So there's one minor issue with this patch, relating to unwinding the > >> > > > stack of a newly forked task. For detecting reliable stacks, the > >> > > > unwinder needs to unwind all the way to the syscall pt_regs to make > >> > > > sure > >> > > > the stack is sane. But for newly forked tasks, that won't be > >> > > > possible > >> > > > here because the unwinding will stop at the fork_frame instead. > >> > > > > >> > > > So from an unwinder standpoint it might be nice for > >> > > > copy_thread_tls() to > >> > > > place a frame pointer on the stack next to the ret_from_fork return > >> > > > address, so that it would resemble an actual stack frame. The frame > >> > > > pointer could probably just be hard-coded to zero. And then the > >> > > > first > >> > > > bp in fork_frame would need to be a pointer to it instead of zero. > >> > > > That > >> > > > would make it nicely resemble the stack of any other task. > >> > > > > >> > > > Alternatively I could teach the unwinder that if the unwinding > >> > > > starts at > >> > > > the fork_frame offset from the end of the stack page, and the saved > >> > > > rbp > >> > > > is zero, it can assume that it's a newly forked task. But that > >> > > > seems a > >> > > > little more brittle to me, as it requires the unwinder to understand > >> > > > more of the internal workings of the fork code. > >> > > > > >> > > > But overall I think this patch is a really nice cleanup, and other > >> > > > than > >> > > > the above minor issue it should be fine with my reliable unwinder, > >> > > > since > >> > > > rbp is still at the top of the stack. > >> > > > >> > > Ok, how about if it pushed RBP first, then we teach get_wchan() to add > >> > > the fixed offset from thread.sp to get bp? that way it don't have to > >> > > push it twice. > >> > > >> > In theory I like the idea, and it would work: the unwinder could just > >> > use the inactive_task_frame struct (as Andy suggested) to find the frame > >> > pointer. > >> > > >> > But I suspect it would break all existing unwinders, both in-tree and > >> > out-of-tree. The only out-of-tree one I know of is crash, not sure if > >> > there are more out there. > >> > >> I should mention it would only affect those unwinders which know how to > >> do sleeping kernel tasks. So generic tools like gdb wouldn't be > >> affected. > > > > [continuing my conversation with myself...] > > > > To clarify, I still think we should do it. The stack format of a > > sleeping task isn't exactly an ABI, and I wouldn't expect many tools to > > rely on it. I can help with the fixing of in-tree unwinders if needed. > > > > Or I could even do the moving of the frame pointer as a separate patch > > on top of this one, since it might cause breakage elsewhere. > > Do you have any understanding of why there are so many unwinder > implementations? Your reliable unwinder seems to be yet another copy > of more or less the same code. Yeah, there are way too many instantations of stacktrace_ops and there's definitely a lot of room for consolidation and simplification. There are different requirements needed by all the different codes relying on dump_trace(): - starting with a given pt_regs - starting with a given task - whether to skip sched code functions - whether to skip the random '?' ktext addresses found on the stack - whether frame pointers are enabled - whether the stack is reliable - output to an arch-independent "struct stack_trace" array So everybody implements their own callbacks for dump_trace(). It's kind of a big mess. > I'd like to see a single, high-quality unwinder implemented as a state > machine, along the lines of: > > struct unwind_state state; > unwind_start_inactive_task(, ...); or > unwind_start_pt_regs(, regs); or whatever. > unwind_next_frame(); > > where, after unwind_next_frame, state encodes whatever registers are > known (at least bp and ip, but all the GPRs would be nice and are > probably mandatory for DWARF) and an indication of whether this is a > real frame or a guessed frame (the things that currently show up as > '?'). I like the idea of a state machine. I'll probably end up doing something like that before introducing the DWARF unwinder. -- Josh
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon 23-05-16 20:14:11, Ebru Akagunduz wrote: > Currently khugepaged makes swapin readahead under > down_write. This patch supplies to make swapin > readahead under down_read instead of down_write. You are still keeping down_write. Can we do without it altogether? Blocking mmap_sem of a remote proces for write is certainly not nice. -- Michal Hocko SUSE Labs
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon 23-05-16 20:14:11, Ebru Akagunduz wrote: > Currently khugepaged makes swapin readahead under > down_write. This patch supplies to make swapin > readahead under down_read instead of down_write. You are still keeping down_write. Can we do without it altogether? Blocking mmap_sem of a remote proces for write is certainly not nice. -- Michal Hocko SUSE Labs
[v2 PATCH] mm: make CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on !FLATMEM explicitly
Per the suggestion from Michal Hocko [1], DEFERRED_STRUCT_PAGE_INIT requires some ordering wrt other initialization operations, e.g. page_ext_init has to happen after the whole memmap is initialized properly. For SPARSEMEM this requires to wait for page_alloc_init_late. Other memory models (e.g. flatmem) might have different initialization layouts (page_ext_init_flatmem). Currently DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which in turn depends on SPARSEMEM || X86_64_ACPI_NUMA depends on ARCH_ENABLE_MEMORY_HOTPLUG and X86_64_ACPI_NUMA depends on NUMA which in turn disable FLATMEM memory model: config ARCH_FLATMEM_ENABLE def_bool y depends on X86_32 && !NUMA so FLATMEM is ruled out via dependency maze. Be explicit and disable FLATMEM for DEFERRED_STRUCT_PAGE_INIT so that we do not reintroduce subtle initialization bugs [1] http://lkml.kernel.org/r/20160523073157.gd2...@dhcp22.suse.cz CC: Michal HockoSigned-off-by: Yang Shi --- v2: Adopted Michal's comments for the commit log mm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/Kconfig b/mm/Kconfig index 2664c11..22fa818 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -649,6 +649,7 @@ config DEFERRED_STRUCT_PAGE_INIT default n depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG + depends on !FLATMEM help Ordinarily all struct pages are initialised during early boot in a single thread. On very large machines this can take a considerable -- 2.0.2
[v2 PATCH] mm: make CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on !FLATMEM explicitly
Per the suggestion from Michal Hocko [1], DEFERRED_STRUCT_PAGE_INIT requires some ordering wrt other initialization operations, e.g. page_ext_init has to happen after the whole memmap is initialized properly. For SPARSEMEM this requires to wait for page_alloc_init_late. Other memory models (e.g. flatmem) might have different initialization layouts (page_ext_init_flatmem). Currently DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which in turn depends on SPARSEMEM || X86_64_ACPI_NUMA depends on ARCH_ENABLE_MEMORY_HOTPLUG and X86_64_ACPI_NUMA depends on NUMA which in turn disable FLATMEM memory model: config ARCH_FLATMEM_ENABLE def_bool y depends on X86_32 && !NUMA so FLATMEM is ruled out via dependency maze. Be explicit and disable FLATMEM for DEFERRED_STRUCT_PAGE_INIT so that we do not reintroduce subtle initialization bugs [1] http://lkml.kernel.org/r/20160523073157.gd2...@dhcp22.suse.cz CC: Michal Hocko Signed-off-by: Yang Shi --- v2: Adopted Michal's comments for the commit log mm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/Kconfig b/mm/Kconfig index 2664c11..22fa818 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -649,6 +649,7 @@ config DEFERRED_STRUCT_PAGE_INIT default n depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG + depends on !FLATMEM help Ordinarily all struct pages are initialised during early boot in a single thread. On very large machines this can take a considerable -- 2.0.2
[RFC 4/7] iio: Add current_trigger_id alternative
This allows controlling the current trigger by numeric ID rather than name. Signed-off-by: Crestez Dan Leonard--- Documentation/ABI/testing/sysfs-bus-iio | 9 +++ Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/industrialio-trigger.c | 115 3 files changed, 98 insertions(+), 30 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio index df44998..e276032 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio +++ b/Documentation/ABI/testing/sysfs-bus-iio @@ -1038,6 +1038,15 @@ Description: The name of the trigger source being used, as per string given in /sys/class/iio/triggerY/name. +What: /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger_id +KernelVersion: 4.7 +Contact: linux-...@vger.kernel.org +Description: + The id of the trigger source being used. This is the Y from + /sys/class/iio/triggerY. This is an alternative to + /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger. + Write a negative number to clear. + What: /sys/bus/iio/devices/iio:deviceX/buffer/length KernelVersion: 2.6.35 Contact: linux-...@vger.kernel.org diff --git a/Documentation/DocBook/iio.tmpl b/Documentation/DocBook/iio.tmpl index f525bf5..599eff0 100644 --- a/Documentation/DocBook/iio.tmpl +++ b/Documentation/DocBook/iio.tmpl @@ -523,7 +523,9 @@ /sys/bus/iio/devices/iio:deviceX/trigger/, this directory is created once the device supports a triggered buffer. We can associate a trigger with our device by writing - the trigger's name in the current_trigger file. + the trigger's name in the current_trigger + file. Alternatively we can write the numeric id Y from + triggerY into current_trigger_id diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index ae2806a..e79c64c 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -121,6 +121,21 @@ static struct iio_trigger *iio_trigger_find_by_name(const char *name, return trig; } +static struct iio_trigger *iio_trigger_find_by_id(int id) +{ + struct iio_trigger *trig = NULL, *iter; + + mutex_lock(_trigger_list_lock); + list_for_each_entry(iter, _trigger_list, list) + if (iter->id == id) { + trig = iter; + break; + } + mutex_unlock(_trigger_list_lock); + + return trig; +} + void iio_trigger_poll(struct iio_trigger *trig) { int i; @@ -294,7 +309,7 @@ void iio_dealloc_pollfunc(struct iio_poll_func *pf) EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc); /** - * iio_trigger_read_current() - trigger consumer sysfs query current trigger + * iio_trigger_read_current_name() - trigger consumer sysfs query current trigger * @dev: device associated with an industrial I/O device * @attr: pointer to the device_attribute structure that * is being processed @@ -306,9 +321,9 @@ EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc); * Return: a negative number on failure, the number of characters written *on success or 0 if no trigger is available */ -static ssize_t iio_trigger_read_current(struct device *dev, - struct device_attribute *attr, - char *buf) +static ssize_t iio_trigger_read_current_name(struct device *dev, +struct device_attribute *attr, +char *buf) { struct iio_dev *indio_dev = dev_to_iio_dev(dev); @@ -317,28 +332,22 @@ static ssize_t iio_trigger_read_current(struct device *dev, return 0; } -/** - * iio_trigger_write_current() - trigger consumer sysfs set current trigger - * @dev: device associated with an industrial I/O device - * @attr: device attribute that is being processed - * @buf: string buffer that holds the name of the trigger - * @len: length of the trigger name held by buf - * - * For trigger consumers the current_trigger interface allows the trigger - * used for this device to be specified at run time based on the trigger's - * name. - * - * Return: negative error code on failure or length of the buffer - *on success - */ -static ssize_t iio_trigger_write_current(struct device *dev, -struct device_attribute *attr, -const char *buf, -size_t len) +static ssize_t iio_trigger_read_current_id(struct device *dev, + struct device_attribute *attr, + char *buf) { struct iio_dev *indio_dev =
[RFC 6/7] iio: Refuse to register triggers with duplicate names
The trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the 'current_trigger' interface unusable. Signed-off-by: Crestez Dan Leonard--- drivers/iio/industrialio-trigger.c | 21 + 1 file changed, 21 insertions(+) diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index e79c64c..e77503c 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = { }; ATTRIBUTE_GROUPS(iio_trig_dev); +static struct iio_trigger *__iio_trigger_find_by_name(const char *name); + int iio_trigger_register(struct iio_trigger *trig_info) { int ret; @@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info) /* Add to list of available triggers held by the IIO core */ mutex_lock(_trigger_list_lock); + if (__iio_trigger_find_by_name(trig_info->name)) { + pr_err("Duplicate trigger name '%s'\n", trig_info->name); + ret = -EEXIST; + goto error_device_del; + } list_add_tail(_info->list, _trigger_list); mutex_unlock(_trigger_list_lock); return 0; +error_device_del: + device_del(_info->dev); error_unregister_id: ida_simple_remove(_trigger_ida, trig_info->id); return ret; @@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger *trig_info) } EXPORT_SYMBOL(iio_trigger_unregister); +/* Search for trigger by name, assuming iio_trigger_list_lock held */ +static struct iio_trigger *__iio_trigger_find_by_name(const char *name) +{ + struct iio_trigger *iter; + + list_for_each_entry(iter, _trigger_list, list) + if (!strcmp(iter->name, name)) + return iter; + + return NULL; +} + static struct iio_trigger *iio_trigger_find_by_name(const char *name, size_t len) { -- 2.5.5
[RFC 4/7] iio: Add current_trigger_id alternative
This allows controlling the current trigger by numeric ID rather than name. Signed-off-by: Crestez Dan Leonard --- Documentation/ABI/testing/sysfs-bus-iio | 9 +++ Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/industrialio-trigger.c | 115 3 files changed, 98 insertions(+), 30 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio index df44998..e276032 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio +++ b/Documentation/ABI/testing/sysfs-bus-iio @@ -1038,6 +1038,15 @@ Description: The name of the trigger source being used, as per string given in /sys/class/iio/triggerY/name. +What: /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger_id +KernelVersion: 4.7 +Contact: linux-...@vger.kernel.org +Description: + The id of the trigger source being used. This is the Y from + /sys/class/iio/triggerY. This is an alternative to + /sys/bus/iio/devices/iio:deviceX/trigger/current_trigger. + Write a negative number to clear. + What: /sys/bus/iio/devices/iio:deviceX/buffer/length KernelVersion: 2.6.35 Contact: linux-...@vger.kernel.org diff --git a/Documentation/DocBook/iio.tmpl b/Documentation/DocBook/iio.tmpl index f525bf5..599eff0 100644 --- a/Documentation/DocBook/iio.tmpl +++ b/Documentation/DocBook/iio.tmpl @@ -523,7 +523,9 @@ /sys/bus/iio/devices/iio:deviceX/trigger/, this directory is created once the device supports a triggered buffer. We can associate a trigger with our device by writing - the trigger's name in the current_trigger file. + the trigger's name in the current_trigger + file. Alternatively we can write the numeric id Y from + triggerY into current_trigger_id diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index ae2806a..e79c64c 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -121,6 +121,21 @@ static struct iio_trigger *iio_trigger_find_by_name(const char *name, return trig; } +static struct iio_trigger *iio_trigger_find_by_id(int id) +{ + struct iio_trigger *trig = NULL, *iter; + + mutex_lock(_trigger_list_lock); + list_for_each_entry(iter, _trigger_list, list) + if (iter->id == id) { + trig = iter; + break; + } + mutex_unlock(_trigger_list_lock); + + return trig; +} + void iio_trigger_poll(struct iio_trigger *trig) { int i; @@ -294,7 +309,7 @@ void iio_dealloc_pollfunc(struct iio_poll_func *pf) EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc); /** - * iio_trigger_read_current() - trigger consumer sysfs query current trigger + * iio_trigger_read_current_name() - trigger consumer sysfs query current trigger * @dev: device associated with an industrial I/O device * @attr: pointer to the device_attribute structure that * is being processed @@ -306,9 +321,9 @@ EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc); * Return: a negative number on failure, the number of characters written *on success or 0 if no trigger is available */ -static ssize_t iio_trigger_read_current(struct device *dev, - struct device_attribute *attr, - char *buf) +static ssize_t iio_trigger_read_current_name(struct device *dev, +struct device_attribute *attr, +char *buf) { struct iio_dev *indio_dev = dev_to_iio_dev(dev); @@ -317,28 +332,22 @@ static ssize_t iio_trigger_read_current(struct device *dev, return 0; } -/** - * iio_trigger_write_current() - trigger consumer sysfs set current trigger - * @dev: device associated with an industrial I/O device - * @attr: device attribute that is being processed - * @buf: string buffer that holds the name of the trigger - * @len: length of the trigger name held by buf - * - * For trigger consumers the current_trigger interface allows the trigger - * used for this device to be specified at run time based on the trigger's - * name. - * - * Return: negative error code on failure or length of the buffer - *on success - */ -static ssize_t iio_trigger_write_current(struct device *dev, -struct device_attribute *attr, -const char *buf, -size_t len) +static ssize_t iio_trigger_read_current_id(struct device *dev, + struct device_attribute *attr, + char *buf) { struct iio_dev *indio_dev = dev_to_iio_dev(dev); + +
[RFC 6/7] iio: Refuse to register triggers with duplicate names
The trigger name is documented as unique but drivers are currently allowed to register triggers with duplicate names. This should be considered a bug since it makes the 'current_trigger' interface unusable. Signed-off-by: Crestez Dan Leonard --- drivers/iio/industrialio-trigger.c | 21 + 1 file changed, 21 insertions(+) diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c index e79c64c..e77503c 100644 --- a/drivers/iio/industrialio-trigger.c +++ b/drivers/iio/industrialio-trigger.c @@ -64,6 +64,8 @@ static struct attribute *iio_trig_dev_attrs[] = { }; ATTRIBUTE_GROUPS(iio_trig_dev); +static struct iio_trigger *__iio_trigger_find_by_name(const char *name); + int iio_trigger_register(struct iio_trigger *trig_info) { int ret; @@ -82,11 +84,18 @@ int iio_trigger_register(struct iio_trigger *trig_info) /* Add to list of available triggers held by the IIO core */ mutex_lock(_trigger_list_lock); + if (__iio_trigger_find_by_name(trig_info->name)) { + pr_err("Duplicate trigger name '%s'\n", trig_info->name); + ret = -EEXIST; + goto error_device_del; + } list_add_tail(_info->list, _trigger_list); mutex_unlock(_trigger_list_lock); return 0; +error_device_del: + device_del(_info->dev); error_unregister_id: ida_simple_remove(_trigger_ida, trig_info->id); return ret; @@ -105,6 +114,18 @@ void iio_trigger_unregister(struct iio_trigger *trig_info) } EXPORT_SYMBOL(iio_trigger_unregister); +/* Search for trigger by name, assuming iio_trigger_list_lock held */ +static struct iio_trigger *__iio_trigger_find_by_name(const char *name) +{ + struct iio_trigger *iter; + + list_for_each_entry(iter, _trigger_list, list) + if (!strcmp(iter->name, name)) + return iter; + + return NULL; +} + static struct iio_trigger *iio_trigger_find_by_name(const char *name, size_t len) { -- 2.5.5
[PATCHv3 3/7] iio: generic_buffer: Add --trigger-num option
Signed-off-by: Crestez Dan Leonard--- tools/iio/generic_buffer.c | 34 +- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index 3f16e9f..e8c3052 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -254,7 +254,9 @@ void print_usage(void) " --device-name -n \n" " --device-num -N \n" "Set device by name or number (mandatory)\n" - " -t Set trigger name\n" + " --trigger-name -t \n" + " --trigger-num -T \n" + "Set trigger by name or number\n" " -w Set delay between reads in us (event-less mode)\n"); } @@ -320,6 +322,8 @@ void register_cleanup(void) static const struct option longopts[] = { { "device-name",1, 0, 'n' }, { "device-num", 1, 0, 'N' }, + { "trigger-name", 1, 0, 't' }, + { "trigger-num",1, 0, 'T' }, { }, }; @@ -348,7 +352,7 @@ int main(int argc, char **argv) register_cleanup(); - while ((c = getopt_long(argc, argv, "ac:egl:n:N:t:w:", longopts, NULL)) != -1) { + while ((c = getopt_long(argc, argv, "ac:egl:n:N:t:T:w:", longopts, NULL)) != -1) { switch (c) { case 'a': autochannels = AUTOCHANNELS_ENABLED; @@ -391,6 +395,12 @@ int main(int argc, char **argv) case 't': trigger_name = strdup(optarg); break; + case 'T': + errno = 0; + trig_num = strtoul(optarg, , 10); + if (errno) + return -errno; + break; case 'w': errno = 0; timedelay = strtoul(optarg, , 10); @@ -444,7 +454,23 @@ int main(int argc, char **argv) } } - if (!notrigger) { + if (notrigger) { + printf("trigger-less mode selected\n"); + } if (trig_num > 0) { + char *trig_dev_name; + ret = asprintf(_dev_name, "%strigger%d", iio_dir, trig_num); + if (ret < 0) { + return -ENOMEM; + } + trigger_name = malloc(IIO_MAX_NAME_LENGTH); + ret = read_sysfs_string("name", trig_dev_name, trigger_name); + free(trig_dev_name); + if (ret < 0) { + fprintf(stderr, "Failed to read trigger%d name from\n", trig_num); + return ret; + } + printf("iio trigger number being used is %d\n", trig_num); + } else { if (!trigger_name) { /* * Build the trigger name. If it is device associated @@ -481,8 +507,6 @@ int main(int argc, char **argv) } printf("iio trigger number being used is %d\n", trig_num); - } else { - printf("trigger-less mode selected\n"); } /* -- 2.5.5
[RFC 0/7] Deal with iio trigger names
IIO documents that trigger names are unique but does not actually guarantee this. You can easily create a software trigger with a duplicate name if you enable CONFIG_IIO_HRTIMER_TRIGGER: mkdir /sys/kernel/config/iio/triggers/hrtimer/\ `cat /sys/bus/iio/devices/trigger0/name` You can also get in this situation if you connect two identical st_sensors. My attempt to fix this is by adding a new 'current_trigger_id' ABI which is a numeric ID (X from triggerX) rather than a non-unique string. I tested that this works as expected when connecting two LIS3DH to the same system. This is in patches 4,5. In theory the current_trigger ABI could be overloaded to looks for something matching "trigger[0-9]+" but this would be nastier. There would be a need to prevent registering triggers that match that expression in order to prevent stuff like: mkdir /sys/kernel/config/iio/triggers/hrtimer/trigger1 Such an overload wouldn't work on read and make it impossible to unambigiously determine the currently selected trigger. Patch 6 disallows registering duplicate trigger names. This makes drivers which currently do that break at probe time. Patch 7 attempts to ensure that all drivers include indio_dev->id when calling iio_trigger_alloc. This obviously changes trigger names. Either 4,5 or 6,7 fix this issue, but 4,5 causes fewer compat issues. Patches 1,2,3 are just minor features in tools/generic_buffer. They supersede the v2 I posted earlier: https://www.spinics.net/lists/linux-iio/msg24867.html Crestez Dan Leonard (7): iio: generic_buffer: Cleanup when receiving signals iio: generic_buffer: Add --device-num option iio: generic_buffer: Add --trigger-num option iio: Add current_trigger_id alternative iio: generic_buffer: Use current_trigger_id iio: Refuse to register triggers with duplicate names iio: Make trigger names unique Documentation/ABI/testing/sysfs-bus-iio| 9 + Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/adc/max1027.c | 4 +- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/industrialio-trigger.c | 136 --- drivers/iio/light/gp2ap020a00f.c | 4 +- tools/iio/generic_buffer.c | 269 ++--- 7 files changed, 309 insertions(+), 119 deletions(-) -- 2.5.5
[PATCHv3 3/7] iio: generic_buffer: Add --trigger-num option
Signed-off-by: Crestez Dan Leonard --- tools/iio/generic_buffer.c | 34 +- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index 3f16e9f..e8c3052 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -254,7 +254,9 @@ void print_usage(void) " --device-name -n \n" " --device-num -N \n" "Set device by name or number (mandatory)\n" - " -t Set trigger name\n" + " --trigger-name -t \n" + " --trigger-num -T \n" + "Set trigger by name or number\n" " -w Set delay between reads in us (event-less mode)\n"); } @@ -320,6 +322,8 @@ void register_cleanup(void) static const struct option longopts[] = { { "device-name",1, 0, 'n' }, { "device-num", 1, 0, 'N' }, + { "trigger-name", 1, 0, 't' }, + { "trigger-num",1, 0, 'T' }, { }, }; @@ -348,7 +352,7 @@ int main(int argc, char **argv) register_cleanup(); - while ((c = getopt_long(argc, argv, "ac:egl:n:N:t:w:", longopts, NULL)) != -1) { + while ((c = getopt_long(argc, argv, "ac:egl:n:N:t:T:w:", longopts, NULL)) != -1) { switch (c) { case 'a': autochannels = AUTOCHANNELS_ENABLED; @@ -391,6 +395,12 @@ int main(int argc, char **argv) case 't': trigger_name = strdup(optarg); break; + case 'T': + errno = 0; + trig_num = strtoul(optarg, , 10); + if (errno) + return -errno; + break; case 'w': errno = 0; timedelay = strtoul(optarg, , 10); @@ -444,7 +454,23 @@ int main(int argc, char **argv) } } - if (!notrigger) { + if (notrigger) { + printf("trigger-less mode selected\n"); + } if (trig_num > 0) { + char *trig_dev_name; + ret = asprintf(_dev_name, "%strigger%d", iio_dir, trig_num); + if (ret < 0) { + return -ENOMEM; + } + trigger_name = malloc(IIO_MAX_NAME_LENGTH); + ret = read_sysfs_string("name", trig_dev_name, trigger_name); + free(trig_dev_name); + if (ret < 0) { + fprintf(stderr, "Failed to read trigger%d name from\n", trig_num); + return ret; + } + printf("iio trigger number being used is %d\n", trig_num); + } else { if (!trigger_name) { /* * Build the trigger name. If it is device associated @@ -481,8 +507,6 @@ int main(int argc, char **argv) } printf("iio trigger number being used is %d\n", trig_num); - } else { - printf("trigger-less mode selected\n"); } /* -- 2.5.5
[RFC 0/7] Deal with iio trigger names
IIO documents that trigger names are unique but does not actually guarantee this. You can easily create a software trigger with a duplicate name if you enable CONFIG_IIO_HRTIMER_TRIGGER: mkdir /sys/kernel/config/iio/triggers/hrtimer/\ `cat /sys/bus/iio/devices/trigger0/name` You can also get in this situation if you connect two identical st_sensors. My attempt to fix this is by adding a new 'current_trigger_id' ABI which is a numeric ID (X from triggerX) rather than a non-unique string. I tested that this works as expected when connecting two LIS3DH to the same system. This is in patches 4,5. In theory the current_trigger ABI could be overloaded to looks for something matching "trigger[0-9]+" but this would be nastier. There would be a need to prevent registering triggers that match that expression in order to prevent stuff like: mkdir /sys/kernel/config/iio/triggers/hrtimer/trigger1 Such an overload wouldn't work on read and make it impossible to unambigiously determine the currently selected trigger. Patch 6 disallows registering duplicate trigger names. This makes drivers which currently do that break at probe time. Patch 7 attempts to ensure that all drivers include indio_dev->id when calling iio_trigger_alloc. This obviously changes trigger names. Either 4,5 or 6,7 fix this issue, but 4,5 causes fewer compat issues. Patches 1,2,3 are just minor features in tools/generic_buffer. They supersede the v2 I posted earlier: https://www.spinics.net/lists/linux-iio/msg24867.html Crestez Dan Leonard (7): iio: generic_buffer: Cleanup when receiving signals iio: generic_buffer: Add --device-num option iio: generic_buffer: Add --trigger-num option iio: Add current_trigger_id alternative iio: generic_buffer: Use current_trigger_id iio: Refuse to register triggers with duplicate names iio: Make trigger names unique Documentation/ABI/testing/sysfs-bus-iio| 9 + Documentation/DocBook/iio.tmpl | 4 +- drivers/iio/adc/max1027.c | 4 +- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/industrialio-trigger.c | 136 --- drivers/iio/light/gp2ap020a00f.c | 4 +- tools/iio/generic_buffer.c | 269 ++--- 7 files changed, 309 insertions(+), 119 deletions(-) -- 2.5.5
[RFC 7/7] iio: Make trigger names unique
This changes the format of trigger names for some drivers so that the result always includes indio_dev->id and is thus unique. Signed-off-by: Crestez Dan Leonard--- drivers/iio/adc/max1027.c | 4 ++-- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/light/gp2ap020a00f.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index 41d495c..4020257 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -447,8 +447,8 @@ static int max1027_probe(struct spi_device *spi) return ret; } - st->trig = devm_iio_trigger_alloc(>dev, "%s-trigger", - indio_dev->name); + st->trig = devm_iio_trigger_alloc(>dev, "%s-dev%d", + indio_dev->name, indio_dev->iod); if (st->trig == NULL) { ret = -ENOMEM; dev_err(_dev->dev, "Failed to allocate iio trigger\n"); diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c index da72279..9d3e9ab 100644 --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c @@ -24,7 +24,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, struct st_sensor_data *sdata = iio_priv(indio_dev); unsigned long irq_trig; - sdata->trig = iio_trigger_alloc("%s-trigger", indio_dev->name); + sdata->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id); if (sdata->trig == NULL) { dev_err(_dev->dev, "failed to allocate iio trigger.\n"); return -ENOMEM; diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c index 6d41086..e2efaeb 100644 --- a/drivers/iio/light/gp2ap020a00f.c +++ b/drivers/iio/light/gp2ap020a00f.c @@ -1553,8 +1553,8 @@ static int gp2ap020a00f_probe(struct i2c_client *client, goto error_regulator_disable; /* Allocate trigger */ - data->trig = devm_iio_trigger_alloc(>dev, "%s-trigger", - indio_dev->name); + data->trig = devm_iio_trigger_alloc(>dev, "%s-dev%d", + indio_dev->name, indio_dev->id); if (data->trig == NULL) { err = -ENOMEM; dev_err(_dev->dev, "Failed to allocate iio trigger.\n"); -- 2.5.5
[PATCHv3 1/7] iio: generic_buffer: Cleanup when receiving signals
This will clean (disable buffer/trigger/channels) when doing something like a CTRL-C. Otherwise restarting generic_buffer requires a manual echo 0 > buffer/enable This also cleanup up all the code freeing string buffers at the end of main. We initialize all pointers to NULL so that cleanup can all be done under a single error label. Signed-off-by: Crestez Dan Leonard--- Changes since v2: Initialize data to NULL to prevent crash on free. tools/iio/generic_buffer.c | 172 - 1 file changed, 108 insertions(+), 64 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index 2429c78..972f400 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -32,6 +32,8 @@ #include #include #include +#include +#include #include "iio_utils.h" /** @@ -254,6 +256,65 @@ void print_usage(void) " -w Set delay between reads in us (event-less mode)\n"); } +enum autochan autochannels = AUTOCHANNELS_DISABLED; +char *dev_dir_name = NULL; +char *buf_dir_name = NULL; +bool current_trigger_set = false; + +void cleanup(void) +{ + int ret; + + /* Disable trigger */ + if (dev_dir_name && current_trigger_set) { + /* Disconnect the trigger - just write a dummy name. */ + ret = write_sysfs_string("trigger/current_trigger", +dev_dir_name, "NULL"); + if (ret < 0) + fprintf(stderr, "Failed to disable trigger: %s\n", + strerror(-ret)); + current_trigger_set = false; + } + + /* Disable buffer */ + if (buf_dir_name) { + ret = write_sysfs_int("enable", buf_dir_name, 0); + if (ret < 0) + fprintf(stderr, "Failed to disable buffer: %s\n", + strerror(-ret)); + } + + /* Disable channels if auto-enabled */ + if (dev_dir_name && autochannels == AUTOCHANNELS_ACTIVE) { + ret = enable_disable_all_channels(dev_dir_name, 0); + if (ret) + fprintf(stderr, "Failed to disable all channels\n"); + autochannels = AUTOCHANNELS_DISABLED; + } +} + +void sig_handler(int signum) +{ + fprintf(stderr, "Caught signal %d\n", signum); + cleanup(); + exit(-signum); +} + +void register_cleanup(void) +{ + struct sigaction sa = { .sa_handler = sig_handler }; + const int signums[] = { SIGINT, SIGTERM, SIGABRT }; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(signums); ++i) { + ret = sigaction(signums[i], , NULL); + if (ret) { + perror("Failed to register signal handler"); + exit(-1); + } + } +} + int main(int argc, char **argv) { unsigned long num_loops = 2; @@ -261,25 +322,24 @@ int main(int argc, char **argv) unsigned long buf_len = 128; int ret, c, i, j, toread; - int fp; + int fp = -1; - int num_channels; + int num_channels = 0; char *trigger_name = NULL, *device_name = NULL; - char *dev_dir_name, *buf_dir_name; - int datardytrigger = 1; - char *data; + char *data = NULL; ssize_t read_size; int dev_num, trig_num; - char *buffer_access; + char *buffer_access = NULL; int scan_size; int noevents = 0; int notrigger = 0; - enum autochan autochannels = AUTOCHANNELS_DISABLED; char *dummy; struct iio_channel_info *channels; + register_cleanup(); + while ((c = getopt(argc, argv, "ac:egl:n:t:w:")) != -1) { switch (c) { case 'a': @@ -288,8 +348,10 @@ int main(int argc, char **argv) case 'c': errno = 0; num_loops = strtoul(optarg, , 10); - if (errno) - return -errno; + if (errno) { + ret = -errno; + goto error; + } break; case 'e': @@ -301,26 +363,30 @@ int main(int argc, char **argv) case 'l': errno = 0; buf_len = strtoul(optarg, , 10); - if (errno) - return -errno; + if (errno) { + ret = -errno; + goto error; + } break; case 'n': device_name = optarg; break; case 't': - trigger_name = optarg; - datardytrigger = 0; +
[PATCHv3 2/7] iio: generic_buffer: Add --device-num option
This makes it possible to distinguish between iio devices with the same name. Signed-off-by: Crestez Dan Leonard--- tools/iio/generic_buffer.c | 69 ++ 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index 972f400..3f16e9f 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -251,7 +251,9 @@ void print_usage(void) " -e Disable wait for event (new data)\n" " -g Use trigger-less mode\n" " -l Set buffer length to n samples\n" - " -n Set device name (mandatory)\n" + " --device-name -n \n" + " --device-num -N \n" + "Set device by name or number (mandatory)\n" " -t Set trigger name\n" " -w Set delay between reads in us (event-less mode)\n"); } @@ -315,6 +317,12 @@ void register_cleanup(void) } } +static const struct option longopts[] = { + { "device-name",1, 0, 'n' }, + { "device-num", 1, 0, 'N' }, + { }, +}; + int main(int argc, char **argv) { unsigned long num_loops = 2; @@ -329,7 +337,7 @@ int main(int argc, char **argv) char *data = NULL; ssize_t read_size; - int dev_num, trig_num; + int dev_num = -1, trig_num; char *buffer_access = NULL; int scan_size; int noevents = 0; @@ -340,7 +348,7 @@ int main(int argc, char **argv) register_cleanup(); - while ((c = getopt(argc, argv, "ac:egl:n:t:w:")) != -1) { + while ((c = getopt_long(argc, argv, "ac:egl:n:N:t:w:", longopts, NULL)) != -1) { switch (c) { case 'a': autochannels = AUTOCHANNELS_ENABLED; @@ -370,7 +378,15 @@ int main(int argc, char **argv) break; case 'n': - device_name = optarg; + device_name = strdup(optarg); + break; + case 'N': + errno = 0; + dev_num = strtoul(optarg, , 10); + if (errno) { + ret = -errno; + goto error; + } break; case 't': trigger_name = strdup(optarg); @@ -390,26 +406,42 @@ int main(int argc, char **argv) } } - if (!device_name) { - fprintf(stderr, "Device name not set\n"); - print_usage(); - return -1; - } - /* Find the device requested */ - dev_num = find_type_by_name(device_name, "iio:device"); - if (dev_num < 0) { - fprintf(stderr, "Failed to find the %s\n", device_name); - ret = dev_num; + if (dev_num < 0 && !device_name) { + fprintf(stderr, "Device not set\n"); + print_usage(); + ret = -1; + goto error; + } else if (dev_num >= 0 && device_name) { + fprintf(stderr, "Only one of --device-num or --device-name needs to be set\n"); + print_usage(); + ret = -1; goto error; + } else if (dev_num < 0) { + dev_num = find_type_by_name(device_name, "iio:device"); + if (dev_num < 0) { + fprintf(stderr, "Failed to find the %s\n", device_name); + ret = dev_num; + goto error; + } } - printf("iio device number being used is %d\n", dev_num); ret = asprintf(_dir_name, "%siio:device%d", iio_dir, dev_num); - if (ret < 0) { - ret = -ENOMEM; - goto error; + if (ret < 0) + return -ENOMEM; + /* Fetch device_name if specified by number */ + if (!device_name) { + device_name = malloc(IIO_MAX_NAME_LENGTH); + if (!device_name) { + ret = -ENOMEM; + goto error; + } + ret = read_sysfs_string("name", dev_dir_name, device_name); + if (ret < 0) { + fprintf(stderr, "Failed to read name of device %d\n", dev_num); + goto error; + } } if (!notrigger) { @@ -619,6 +651,7 @@ error: } free(channels); free(trigger_name); + free(device_name); free(dev_dir_name); return ret; -- 2.5.5
[RFC 5/7] iio: generic_buffer: Use current_trigger_id
This is a preferred alternative to 'current_trigger'. Signed-off-by: Crestez Dan Leonard--- tools/iio/generic_buffer.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index e8c3052..b23371a 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -582,12 +582,12 @@ int main(int argc, char **argv) * Set the device trigger to be the data ready trigger found * above */ - ret = write_sysfs_string_and_verify("trigger/current_trigger", - dev_dir_name, - trigger_name); + ret = write_sysfs_int_and_verify("trigger/current_trigger_id", +dev_dir_name, +trig_num); if (ret < 0) { fprintf(stderr, - "Failed to write current_trigger file\n"); + "Failed to write current_trigger_id file\n"); goto error; } } -- 2.5.5
[RFC 7/7] iio: Make trigger names unique
This changes the format of trigger names for some drivers so that the result always includes indio_dev->id and is thus unique. Signed-off-by: Crestez Dan Leonard --- drivers/iio/adc/max1027.c | 4 ++-- drivers/iio/common/st_sensors/st_sensors_trigger.c | 2 +- drivers/iio/light/gp2ap020a00f.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c index 41d495c..4020257 100644 --- a/drivers/iio/adc/max1027.c +++ b/drivers/iio/adc/max1027.c @@ -447,8 +447,8 @@ static int max1027_probe(struct spi_device *spi) return ret; } - st->trig = devm_iio_trigger_alloc(>dev, "%s-trigger", - indio_dev->name); + st->trig = devm_iio_trigger_alloc(>dev, "%s-dev%d", + indio_dev->name, indio_dev->iod); if (st->trig == NULL) { ret = -ENOMEM; dev_err(_dev->dev, "Failed to allocate iio trigger\n"); diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c index da72279..9d3e9ab 100644 --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c @@ -24,7 +24,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev, struct st_sensor_data *sdata = iio_priv(indio_dev); unsigned long irq_trig; - sdata->trig = iio_trigger_alloc("%s-trigger", indio_dev->name); + sdata->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id); if (sdata->trig == NULL) { dev_err(_dev->dev, "failed to allocate iio trigger.\n"); return -ENOMEM; diff --git a/drivers/iio/light/gp2ap020a00f.c b/drivers/iio/light/gp2ap020a00f.c index 6d41086..e2efaeb 100644 --- a/drivers/iio/light/gp2ap020a00f.c +++ b/drivers/iio/light/gp2ap020a00f.c @@ -1553,8 +1553,8 @@ static int gp2ap020a00f_probe(struct i2c_client *client, goto error_regulator_disable; /* Allocate trigger */ - data->trig = devm_iio_trigger_alloc(>dev, "%s-trigger", - indio_dev->name); + data->trig = devm_iio_trigger_alloc(>dev, "%s-dev%d", + indio_dev->name, indio_dev->id); if (data->trig == NULL) { err = -ENOMEM; dev_err(_dev->dev, "Failed to allocate iio trigger.\n"); -- 2.5.5
[PATCHv3 1/7] iio: generic_buffer: Cleanup when receiving signals
This will clean (disable buffer/trigger/channels) when doing something like a CTRL-C. Otherwise restarting generic_buffer requires a manual echo 0 > buffer/enable This also cleanup up all the code freeing string buffers at the end of main. We initialize all pointers to NULL so that cleanup can all be done under a single error label. Signed-off-by: Crestez Dan Leonard --- Changes since v2: Initialize data to NULL to prevent crash on free. tools/iio/generic_buffer.c | 172 - 1 file changed, 108 insertions(+), 64 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index 2429c78..972f400 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -32,6 +32,8 @@ #include #include #include +#include +#include #include "iio_utils.h" /** @@ -254,6 +256,65 @@ void print_usage(void) " -w Set delay between reads in us (event-less mode)\n"); } +enum autochan autochannels = AUTOCHANNELS_DISABLED; +char *dev_dir_name = NULL; +char *buf_dir_name = NULL; +bool current_trigger_set = false; + +void cleanup(void) +{ + int ret; + + /* Disable trigger */ + if (dev_dir_name && current_trigger_set) { + /* Disconnect the trigger - just write a dummy name. */ + ret = write_sysfs_string("trigger/current_trigger", +dev_dir_name, "NULL"); + if (ret < 0) + fprintf(stderr, "Failed to disable trigger: %s\n", + strerror(-ret)); + current_trigger_set = false; + } + + /* Disable buffer */ + if (buf_dir_name) { + ret = write_sysfs_int("enable", buf_dir_name, 0); + if (ret < 0) + fprintf(stderr, "Failed to disable buffer: %s\n", + strerror(-ret)); + } + + /* Disable channels if auto-enabled */ + if (dev_dir_name && autochannels == AUTOCHANNELS_ACTIVE) { + ret = enable_disable_all_channels(dev_dir_name, 0); + if (ret) + fprintf(stderr, "Failed to disable all channels\n"); + autochannels = AUTOCHANNELS_DISABLED; + } +} + +void sig_handler(int signum) +{ + fprintf(stderr, "Caught signal %d\n", signum); + cleanup(); + exit(-signum); +} + +void register_cleanup(void) +{ + struct sigaction sa = { .sa_handler = sig_handler }; + const int signums[] = { SIGINT, SIGTERM, SIGABRT }; + int ret, i; + + for (i = 0; i < ARRAY_SIZE(signums); ++i) { + ret = sigaction(signums[i], , NULL); + if (ret) { + perror("Failed to register signal handler"); + exit(-1); + } + } +} + int main(int argc, char **argv) { unsigned long num_loops = 2; @@ -261,25 +322,24 @@ int main(int argc, char **argv) unsigned long buf_len = 128; int ret, c, i, j, toread; - int fp; + int fp = -1; - int num_channels; + int num_channels = 0; char *trigger_name = NULL, *device_name = NULL; - char *dev_dir_name, *buf_dir_name; - int datardytrigger = 1; - char *data; + char *data = NULL; ssize_t read_size; int dev_num, trig_num; - char *buffer_access; + char *buffer_access = NULL; int scan_size; int noevents = 0; int notrigger = 0; - enum autochan autochannels = AUTOCHANNELS_DISABLED; char *dummy; struct iio_channel_info *channels; + register_cleanup(); + while ((c = getopt(argc, argv, "ac:egl:n:t:w:")) != -1) { switch (c) { case 'a': @@ -288,8 +348,10 @@ int main(int argc, char **argv) case 'c': errno = 0; num_loops = strtoul(optarg, , 10); - if (errno) - return -errno; + if (errno) { + ret = -errno; + goto error; + } break; case 'e': @@ -301,26 +363,30 @@ int main(int argc, char **argv) case 'l': errno = 0; buf_len = strtoul(optarg, , 10); - if (errno) - return -errno; + if (errno) { + ret = -errno; + goto error; + } break; case 'n': device_name = optarg; break; case 't': - trigger_name = optarg; - datardytrigger = 0; + trigger_name =
[PATCHv3 2/7] iio: generic_buffer: Add --device-num option
This makes it possible to distinguish between iio devices with the same name. Signed-off-by: Crestez Dan Leonard --- tools/iio/generic_buffer.c | 69 ++ 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index 972f400..3f16e9f 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -251,7 +251,9 @@ void print_usage(void) " -e Disable wait for event (new data)\n" " -g Use trigger-less mode\n" " -l Set buffer length to n samples\n" - " -n Set device name (mandatory)\n" + " --device-name -n \n" + " --device-num -N \n" + "Set device by name or number (mandatory)\n" " -t Set trigger name\n" " -w Set delay between reads in us (event-less mode)\n"); } @@ -315,6 +317,12 @@ void register_cleanup(void) } } +static const struct option longopts[] = { + { "device-name",1, 0, 'n' }, + { "device-num", 1, 0, 'N' }, + { }, +}; + int main(int argc, char **argv) { unsigned long num_loops = 2; @@ -329,7 +337,7 @@ int main(int argc, char **argv) char *data = NULL; ssize_t read_size; - int dev_num, trig_num; + int dev_num = -1, trig_num; char *buffer_access = NULL; int scan_size; int noevents = 0; @@ -340,7 +348,7 @@ int main(int argc, char **argv) register_cleanup(); - while ((c = getopt(argc, argv, "ac:egl:n:t:w:")) != -1) { + while ((c = getopt_long(argc, argv, "ac:egl:n:N:t:w:", longopts, NULL)) != -1) { switch (c) { case 'a': autochannels = AUTOCHANNELS_ENABLED; @@ -370,7 +378,15 @@ int main(int argc, char **argv) break; case 'n': - device_name = optarg; + device_name = strdup(optarg); + break; + case 'N': + errno = 0; + dev_num = strtoul(optarg, , 10); + if (errno) { + ret = -errno; + goto error; + } break; case 't': trigger_name = strdup(optarg); @@ -390,26 +406,42 @@ int main(int argc, char **argv) } } - if (!device_name) { - fprintf(stderr, "Device name not set\n"); - print_usage(); - return -1; - } - /* Find the device requested */ - dev_num = find_type_by_name(device_name, "iio:device"); - if (dev_num < 0) { - fprintf(stderr, "Failed to find the %s\n", device_name); - ret = dev_num; + if (dev_num < 0 && !device_name) { + fprintf(stderr, "Device not set\n"); + print_usage(); + ret = -1; + goto error; + } else if (dev_num >= 0 && device_name) { + fprintf(stderr, "Only one of --device-num or --device-name needs to be set\n"); + print_usage(); + ret = -1; goto error; + } else if (dev_num < 0) { + dev_num = find_type_by_name(device_name, "iio:device"); + if (dev_num < 0) { + fprintf(stderr, "Failed to find the %s\n", device_name); + ret = dev_num; + goto error; + } } - printf("iio device number being used is %d\n", dev_num); ret = asprintf(_dir_name, "%siio:device%d", iio_dir, dev_num); - if (ret < 0) { - ret = -ENOMEM; - goto error; + if (ret < 0) + return -ENOMEM; + /* Fetch device_name if specified by number */ + if (!device_name) { + device_name = malloc(IIO_MAX_NAME_LENGTH); + if (!device_name) { + ret = -ENOMEM; + goto error; + } + ret = read_sysfs_string("name", dev_dir_name, device_name); + if (ret < 0) { + fprintf(stderr, "Failed to read name of device %d\n", dev_num); + goto error; + } } if (!notrigger) { @@ -619,6 +651,7 @@ error: } free(channels); free(trigger_name); + free(device_name); free(dev_dir_name); return ret; -- 2.5.5
[RFC 5/7] iio: generic_buffer: Use current_trigger_id
This is a preferred alternative to 'current_trigger'. Signed-off-by: Crestez Dan Leonard --- tools/iio/generic_buffer.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/iio/generic_buffer.c b/tools/iio/generic_buffer.c index e8c3052..b23371a 100644 --- a/tools/iio/generic_buffer.c +++ b/tools/iio/generic_buffer.c @@ -582,12 +582,12 @@ int main(int argc, char **argv) * Set the device trigger to be the data ready trigger found * above */ - ret = write_sysfs_string_and_verify("trigger/current_trigger", - dev_dir_name, - trigger_name); + ret = write_sysfs_int_and_verify("trigger/current_trigger_id", +dev_dir_name, +trig_num); if (ret < 0) { fprintf(stderr, - "Failed to write current_trigger file\n"); + "Failed to write current_trigger_id file\n"); goto error; } } -- 2.5.5
Re: powerpc/pseries: start rtasd before PCI probing
On Mon, 23 May 2016 20:23:19 +0200 Thomas Huthwrote: > On 23.05.2016 10:28, Greg Kurz wrote: > > A strange behaviour is observed when comparing PCI hotplug in QEMU, between > > x86 and pseries. If you consider the following steps: > > - start a VM > > - add a PCI device via the QEMU monitor before the rtasd has started (for > > example starting the VM in paused state, or hotplug during FW or boot > > loader) > > - resume the VM execution > > > > The x86 kernel detects the PCI device, but the pseries one does not. > > > > This happens because the rtasd kernel worker is currently started under > > device_initcall, while PCI probing happens earlier under subsys_initcall. > > > > As a consequence, if we have a pending RTAS event at boot time, a message > > is printed and the event is dropped. > > > > This patch moves all the initialization of rtasd to arch_initcall, which is > > run before subsys_call: this way, logging_enabled is true when the RTAS > > event pops up and it is not lost anymore. > > > > The proc fs bits stay at device_initcall because they cannot be run before > > fs_initcall. > > > > Signed-off-by: Greg Kurz > > --- > > arch/powerpc/kernel/rtasd.c | 19 ++- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > By the way, same is true for device UNplugging: When unplugging devices > in QEMU while the firmware is still running, they are never properly > removed from the guest. I've checked it, and your patch fixes this > problem as well! Great :-) > > Tested-by: Thomas Huth > Indeed, all pending RTAS events were lost... now rtasd will log them, up to 64 events, but I did not test that far :) Thanks for testing ! Cheers. -- Greg
Re: powerpc/pseries: start rtasd before PCI probing
On Mon, 23 May 2016 20:23:19 +0200 Thomas Huth wrote: > On 23.05.2016 10:28, Greg Kurz wrote: > > A strange behaviour is observed when comparing PCI hotplug in QEMU, between > > x86 and pseries. If you consider the following steps: > > - start a VM > > - add a PCI device via the QEMU monitor before the rtasd has started (for > > example starting the VM in paused state, or hotplug during FW or boot > > loader) > > - resume the VM execution > > > > The x86 kernel detects the PCI device, but the pseries one does not. > > > > This happens because the rtasd kernel worker is currently started under > > device_initcall, while PCI probing happens earlier under subsys_initcall. > > > > As a consequence, if we have a pending RTAS event at boot time, a message > > is printed and the event is dropped. > > > > This patch moves all the initialization of rtasd to arch_initcall, which is > > run before subsys_call: this way, logging_enabled is true when the RTAS > > event pops up and it is not lost anymore. > > > > The proc fs bits stay at device_initcall because they cannot be run before > > fs_initcall. > > > > Signed-off-by: Greg Kurz > > --- > > arch/powerpc/kernel/rtasd.c | 19 ++- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > By the way, same is true for device UNplugging: When unplugging devices > in QEMU while the firmware is still running, they are never properly > removed from the guest. I've checked it, and your patch fixes this > problem as well! Great :-) > > Tested-by: Thomas Huth > Indeed, all pending RTAS events were lost... now rtasd will log them, up to 64 events, but I did not test that far :) Thanks for testing ! Cheers. -- Greg
Re: [RFC PATCH 03/15] Provide atomic_t functions implemented with ISO-C++11 atomics
On Fri, May 20, 2016 at 07:32:09PM +1000, Michael Ellerman wrote: > On Thu, 2016-05-19 at 08:00 -0700, Paul E. McKenney wrote: > > On Thu, May 19, 2016 at 04:41:17PM +0200, Peter Zijlstra wrote: > > > On Thu, May 19, 2016 at 07:22:52AM -0700, Paul E. McKenney wrote: > > > > Agreed, these sorts of instruction sequences make a lot of sense. > > > > Of course, if you stuff too many intructions and cache misses between > > > > the LL and the SC, the SC success probability starts dropping, but short > > > > seqeunces of non-memory-reference instructions like the above should be > > > > just fine. > > > > > > In fact, pretty much every single LL/SC arch I've looked at doesn't > > > allow _any_ loads or stores inside and will guarantee SC failure (or > > > worse) if you do. > > > > Last I know, PowerPC allowed memory-reference instructions inside, but > > the more of them you have, the less likely your reservation is to survive. > > But perhaps I missed some fine print somewhere. And in any case, > > omitting them is certainly better. > > There's nothing in the architecture AFAIK. > > Also I don't see anything to indicate that doing more unrelated accesses makes > the reservation more likely to be lost. Other than it causes you to hold the > reservation for longer, which increases the chance of some other CPU accessing > the variable. And also more likely to hit cache-geometry limitations. > Having said that, the architecture is written to provide maximum wiggle room > for implementations. So the list of things that may cause the reservation to > be > lost includes "Implementation-specific characteristics of the coherence > mechanism", ie. anything. > > > > This immediately disqualifies things like calls/traps/etc.. because > > > those implicitly issue stores. > > > > Traps for sure. Not so sure about calls on PowerPC. > > Actually no, exceptions (aka interrupts/traps) are explicitly defined to *not* > clear the reservation. And function calls are just branches so should also be > fine. But don't most interrupt/trap handlers clear the reservation in software? Thanx, Paul
Re: [RFC PATCH 03/15] Provide atomic_t functions implemented with ISO-C++11 atomics
On Fri, May 20, 2016 at 07:32:09PM +1000, Michael Ellerman wrote: > On Thu, 2016-05-19 at 08:00 -0700, Paul E. McKenney wrote: > > On Thu, May 19, 2016 at 04:41:17PM +0200, Peter Zijlstra wrote: > > > On Thu, May 19, 2016 at 07:22:52AM -0700, Paul E. McKenney wrote: > > > > Agreed, these sorts of instruction sequences make a lot of sense. > > > > Of course, if you stuff too many intructions and cache misses between > > > > the LL and the SC, the SC success probability starts dropping, but short > > > > seqeunces of non-memory-reference instructions like the above should be > > > > just fine. > > > > > > In fact, pretty much every single LL/SC arch I've looked at doesn't > > > allow _any_ loads or stores inside and will guarantee SC failure (or > > > worse) if you do. > > > > Last I know, PowerPC allowed memory-reference instructions inside, but > > the more of them you have, the less likely your reservation is to survive. > > But perhaps I missed some fine print somewhere. And in any case, > > omitting them is certainly better. > > There's nothing in the architecture AFAIK. > > Also I don't see anything to indicate that doing more unrelated accesses makes > the reservation more likely to be lost. Other than it causes you to hold the > reservation for longer, which increases the chance of some other CPU accessing > the variable. And also more likely to hit cache-geometry limitations. > Having said that, the architecture is written to provide maximum wiggle room > for implementations. So the list of things that may cause the reservation to > be > lost includes "Implementation-specific characteristics of the coherence > mechanism", ie. anything. > > > > This immediately disqualifies things like calls/traps/etc.. because > > > those implicitly issue stores. > > > > Traps for sure. Not so sure about calls on PowerPC. > > Actually no, exceptions (aka interrupts/traps) are explicitly defined to *not* > clear the reservation. And function calls are just branches so should also be > fine. But don't most interrupt/trap handlers clear the reservation in software? Thanx, Paul
Re: [PATCH] mm: make CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on !FLATMEM explicitly
On 5/23/2016 11:22 AM, Michal Hocko wrote: On Mon 23-05-16 09:54:31, Yang Shi wrote: Per the suggestion from Michal Hocko [1], CONFIG_DEFERRED_STRUCT_PAGE_INIT should be incompatible with FLATMEM, make this explicitly in Kconfig. I guess the changelog could benefit from some clarification. What do you think about the following: " DEFERRED_STRUCT_PAGE_INIT requires some ordering wrt other initialization operations, e.g. page_ext_init has to happen after the whole memmap is initialized properly. For SPARSEMEM this requires to wait for page_alloc_init_late. Other memory models (e.g. flatmem) might have different initialization layouts (page_ext_init_flatmem). Currently DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which in turn depends on SPARSEMEM || X86_64_ACPI_NUMA depends on ARCH_ENABLE_MEMORY_HOTPLUG and X86_64_ACPI_NUMA depends on NUMA which in turn disable FLATMEM memory model: config ARCH_FLATMEM_ENABLE def_bool y depends on X86_32 && !NUMA so FLATMEM is ruled out via dependency maze. Be explicit and disable FLATMEM for DEFERRED_STRUCT_PAGE_INIT so that we do not reintroduce subtle initialization bugs " Thanks a lot. It sounds way better. Will address in V2. Yang [1] http://lkml.kernel.org/r/20160523073157.gd2...@dhcp22.suse.cz Signed-off-by: Yang Shi--- mm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/Kconfig b/mm/Kconfig index 2664c11..22fa818 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -649,6 +649,7 @@ config DEFERRED_STRUCT_PAGE_INIT default n depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG + depends on !FLATMEM help Ordinarily all struct pages are initialised during early boot in a single thread. On very large machines this can take a considerable -- 2.0.2
Re: [PATCH] mm: make CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on !FLATMEM explicitly
On 5/23/2016 11:22 AM, Michal Hocko wrote: On Mon 23-05-16 09:54:31, Yang Shi wrote: Per the suggestion from Michal Hocko [1], CONFIG_DEFERRED_STRUCT_PAGE_INIT should be incompatible with FLATMEM, make this explicitly in Kconfig. I guess the changelog could benefit from some clarification. What do you think about the following: " DEFERRED_STRUCT_PAGE_INIT requires some ordering wrt other initialization operations, e.g. page_ext_init has to happen after the whole memmap is initialized properly. For SPARSEMEM this requires to wait for page_alloc_init_late. Other memory models (e.g. flatmem) might have different initialization layouts (page_ext_init_flatmem). Currently DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which in turn depends on SPARSEMEM || X86_64_ACPI_NUMA depends on ARCH_ENABLE_MEMORY_HOTPLUG and X86_64_ACPI_NUMA depends on NUMA which in turn disable FLATMEM memory model: config ARCH_FLATMEM_ENABLE def_bool y depends on X86_32 && !NUMA so FLATMEM is ruled out via dependency maze. Be explicit and disable FLATMEM for DEFERRED_STRUCT_PAGE_INIT so that we do not reintroduce subtle initialization bugs " Thanks a lot. It sounds way better. Will address in V2. Yang [1] http://lkml.kernel.org/r/20160523073157.gd2...@dhcp22.suse.cz Signed-off-by: Yang Shi --- mm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/mm/Kconfig b/mm/Kconfig index 2664c11..22fa818 100644 --- a/mm/Kconfig +++ b/mm/Kconfig @@ -649,6 +649,7 @@ config DEFERRED_STRUCT_PAGE_INIT default n depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG + depends on !FLATMEM help Ordinarily all struct pages are initialised during early boot in a single thread. On very large machines this can take a considerable -- 2.0.2
Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE
On 05/23/2016 01:56 PM, Lorenzo Pieralisi wrote: > On Tue, May 17, 2016 at 12:48:53PM -0400, Jon Masters wrote: >> On 05/17/2016 12:44 PM, Jon Masters wrote: >> >>> 1). During development of a platform, it is much easier to debug >>> problems with tables if you can test replacement ones without having to >>> respin the firmware. In the server world, you usually don't have the >>> firmware source code, so to get it respun could be days-weeks even if >>> you are working with the authors closely. We have practically used this >>> feature on a number of platforms already and it will continue. >> >> For example, on one platform we were unable to fully boot RHEL(SA) due >> to a bug in one of the ACPI tables. But I was able to boot the system to >> a ramdisk containing a uuencode library and then write out the content >> of the tables over the serial port, then decompile/patch/recompile, and >> override replacement tables on the system. Then we beat the vendor up >> with the fixes and the official firmware was corrected. > > Can you explain to me please why you can't do it with GRUB ? It's doable with GRUB (if you rebuild GRUB modules to include it) > I am using mainline GRUB and its acpi command all the time to update > static ACPI tables for testing new features (ie IORT) and it works > just fine for me (and you can still override the DSDT, which is > likely to be the main source of bugs, through the CONFIG_ACPI_CUSTOM_DSDT > config option, that works on ARM in mainline with no changes required). > > I just want to understand if there is really a compelling reason > for adding this stuff when we can easily implement its features > through something that is usable today without any kernel changes. We're looking for x86 feature parity in the base platform. Every time there's a gratuitous differentiation it's a waste of time for folks trying to figure out what is different on an ARM system. So if we completely remove the ACPI override feature from the kernel and make everyone use GRUB instead, then fine, but ARM shouldn't be special. Jon. -- Computer Architect | Sent from my Fedora powered laptop
Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE
On 05/23/2016 01:56 PM, Lorenzo Pieralisi wrote: > On Tue, May 17, 2016 at 12:48:53PM -0400, Jon Masters wrote: >> On 05/17/2016 12:44 PM, Jon Masters wrote: >> >>> 1). During development of a platform, it is much easier to debug >>> problems with tables if you can test replacement ones without having to >>> respin the firmware. In the server world, you usually don't have the >>> firmware source code, so to get it respun could be days-weeks even if >>> you are working with the authors closely. We have practically used this >>> feature on a number of platforms already and it will continue. >> >> For example, on one platform we were unable to fully boot RHEL(SA) due >> to a bug in one of the ACPI tables. But I was able to boot the system to >> a ramdisk containing a uuencode library and then write out the content >> of the tables over the serial port, then decompile/patch/recompile, and >> override replacement tables on the system. Then we beat the vendor up >> with the fixes and the official firmware was corrected. > > Can you explain to me please why you can't do it with GRUB ? It's doable with GRUB (if you rebuild GRUB modules to include it) > I am using mainline GRUB and its acpi command all the time to update > static ACPI tables for testing new features (ie IORT) and it works > just fine for me (and you can still override the DSDT, which is > likely to be the main source of bugs, through the CONFIG_ACPI_CUSTOM_DSDT > config option, that works on ARM in mainline with no changes required). > > I just want to understand if there is really a compelling reason > for adding this stuff when we can easily implement its features > through something that is usable today without any kernel changes. We're looking for x86 feature parity in the base platform. Every time there's a gratuitous differentiation it's a waste of time for folks trying to figure out what is different on an ARM system. So if we completely remove the ACPI override feature from the kernel and make everyone use GRUB instead, then fine, but ARM shouldn't be special. Jon. -- Computer Architect | Sent from my Fedora powered laptop
Re: powerpc/pseries: start rtasd before PCI probing
On 23.05.2016 10:28, Greg Kurz wrote: > A strange behaviour is observed when comparing PCI hotplug in QEMU, between > x86 and pseries. If you consider the following steps: > - start a VM > - add a PCI device via the QEMU monitor before the rtasd has started (for > example starting the VM in paused state, or hotplug during FW or boot > loader) > - resume the VM execution > > The x86 kernel detects the PCI device, but the pseries one does not. > > This happens because the rtasd kernel worker is currently started under > device_initcall, while PCI probing happens earlier under subsys_initcall. > > As a consequence, if we have a pending RTAS event at boot time, a message > is printed and the event is dropped. > > This patch moves all the initialization of rtasd to arch_initcall, which is > run before subsys_call: this way, logging_enabled is true when the RTAS > event pops up and it is not lost anymore. > > The proc fs bits stay at device_initcall because they cannot be run before > fs_initcall. > > Signed-off-by: Greg Kurz> --- > arch/powerpc/kernel/rtasd.c | 19 ++- > 1 file changed, 14 insertions(+), 5 deletions(-) By the way, same is true for device UNplugging: When unplugging devices in QEMU while the firmware is still running, they are never properly removed from the guest. I've checked it, and your patch fixes this problem as well! Great :-) Tested-by: Thomas Huth
Re: powerpc/pseries: start rtasd before PCI probing
On 23.05.2016 10:28, Greg Kurz wrote: > A strange behaviour is observed when comparing PCI hotplug in QEMU, between > x86 and pseries. If you consider the following steps: > - start a VM > - add a PCI device via the QEMU monitor before the rtasd has started (for > example starting the VM in paused state, or hotplug during FW or boot > loader) > - resume the VM execution > > The x86 kernel detects the PCI device, but the pseries one does not. > > This happens because the rtasd kernel worker is currently started under > device_initcall, while PCI probing happens earlier under subsys_initcall. > > As a consequence, if we have a pending RTAS event at boot time, a message > is printed and the event is dropped. > > This patch moves all the initialization of rtasd to arch_initcall, which is > run before subsys_call: this way, logging_enabled is true when the RTAS > event pops up and it is not lost anymore. > > The proc fs bits stay at device_initcall because they cannot be run before > fs_initcall. > > Signed-off-by: Greg Kurz > --- > arch/powerpc/kernel/rtasd.c | 19 ++- > 1 file changed, 14 insertions(+), 5 deletions(-) By the way, same is true for device UNplugging: When unplugging devices in QEMU while the firmware is still running, they are never properly removed from the guest. I've checked it, and your patch fixes this problem as well! Great :-) Tested-by: Thomas Huth
Re: [PATCH] mm: make CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on !FLATMEM explicitly
On Mon 23-05-16 09:54:31, Yang Shi wrote: > Per the suggestion from Michal Hocko [1], CONFIG_DEFERRED_STRUCT_PAGE_INIT > should be incompatible with FLATMEM, make this explicitly in Kconfig. I guess the changelog could benefit from some clarification. What do you think about the following: " DEFERRED_STRUCT_PAGE_INIT requires some ordering wrt other initialization operations, e.g. page_ext_init has to happen after the whole memmap is initialized properly. For SPARSEMEM this requires to wait for page_alloc_init_late. Other memory models (e.g. flatmem) might have different initialization layouts (page_ext_init_flatmem). Currently DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which in turn depends on SPARSEMEM || X86_64_ACPI_NUMA depends on ARCH_ENABLE_MEMORY_HOTPLUG and X86_64_ACPI_NUMA depends on NUMA which in turn disable FLATMEM memory model: config ARCH_FLATMEM_ENABLE def_bool y depends on X86_32 && !NUMA so FLATMEM is ruled out via dependency maze. Be explicit and disable FLATMEM for DEFERRED_STRUCT_PAGE_INIT so that we do not reintroduce subtle initialization bugs " > > [1] http://lkml.kernel.org/r/20160523073157.gd2...@dhcp22.suse.cz > > Signed-off-by: Yang Shi> --- > mm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/Kconfig b/mm/Kconfig > index 2664c11..22fa818 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -649,6 +649,7 @@ config DEFERRED_STRUCT_PAGE_INIT > default n > depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT > depends on MEMORY_HOTPLUG > + depends on !FLATMEM > help > Ordinarily all struct pages are initialised during early boot in a > single thread. On very large machines this can take a considerable > -- > 2.0.2 -- Michal Hocko SUSE Labs
Re: [PATCH] mm: make CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on !FLATMEM explicitly
On Mon 23-05-16 09:54:31, Yang Shi wrote: > Per the suggestion from Michal Hocko [1], CONFIG_DEFERRED_STRUCT_PAGE_INIT > should be incompatible with FLATMEM, make this explicitly in Kconfig. I guess the changelog could benefit from some clarification. What do you think about the following: " DEFERRED_STRUCT_PAGE_INIT requires some ordering wrt other initialization operations, e.g. page_ext_init has to happen after the whole memmap is initialized properly. For SPARSEMEM this requires to wait for page_alloc_init_late. Other memory models (e.g. flatmem) might have different initialization layouts (page_ext_init_flatmem). Currently DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which in turn depends on SPARSEMEM || X86_64_ACPI_NUMA depends on ARCH_ENABLE_MEMORY_HOTPLUG and X86_64_ACPI_NUMA depends on NUMA which in turn disable FLATMEM memory model: config ARCH_FLATMEM_ENABLE def_bool y depends on X86_32 && !NUMA so FLATMEM is ruled out via dependency maze. Be explicit and disable FLATMEM for DEFERRED_STRUCT_PAGE_INIT so that we do not reintroduce subtle initialization bugs " > > [1] http://lkml.kernel.org/r/20160523073157.gd2...@dhcp22.suse.cz > > Signed-off-by: Yang Shi > --- > mm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/Kconfig b/mm/Kconfig > index 2664c11..22fa818 100644 > --- a/mm/Kconfig > +++ b/mm/Kconfig > @@ -649,6 +649,7 @@ config DEFERRED_STRUCT_PAGE_INIT > default n > depends on ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT > depends on MEMORY_HOTPLUG > + depends on !FLATMEM > help > Ordinarily all struct pages are initialised during early boot in a > single thread. On very large machines this can take a considerable > -- > 2.0.2 -- Michal Hocko SUSE Labs
Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
On 5/23/2016 01:25 PM, Mark Rutland wrote: > On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote: >> L2 registers are accessed using a select register and data >> register pair. To prevent multiple concurrent writes to the >> select register by independent drivers, the write to the >> select register and the associated access of the data register >> are protected with a lock. All drivers accessing the L2 >> registers use the set and get functions provided by >> l2-accessors to ensure correct reads and writes to L2 registers. > > What will this be used for? (i.e. which drivers want to touch the L2 > registers?). > > Generally we expect FW to configure the caches and interconnect > appropriately. The primary use is in the L2 PMU driver, which will be posted shortly. > >> Signed-off-by: Neil Leeder>> --- >> drivers/soc/qcom/Kconfig | 9 + >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/l2-accessors.c | 66 >> +++ >> include/linux/soc/qcom/l2-accessors.h | 27 ++ >> 4 files changed, 103 insertions(+) >> create mode 100644 drivers/soc/qcom/l2-accessors.c >> create mode 100644 include/linux/soc/qcom/l2-accessors.h > > These are awfully generic file names (and function names). Which SoCs > does this apply to? > > It would be good to give these more specific names. It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs. Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd prefer not to add redundancy by adding another qcom in there. >> diff --git a/include/linux/soc/qcom/l2-accessors.h >> b/include/linux/soc/qcom/l2-accessors.h >> new file mode 100644 >> index 000..563c114 >> --- /dev/null >> +++ b/include/linux/soc/qcom/l2-accessors.h >> @@ -0,0 +1,27 @@ >> +/* >> + * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + > >> +#ifndef __QCOM_L2_ACCESSORS_H >> +#define __QCOM_L2_ACCESSORS_H >> + >> +#ifdef CONFIG_QCOM_L2_ACCESSORS >> +void set_l2_indirect_reg(u64 reg_addr, u64 val); >> +u64 get_l2_indirect_reg(u64 reg_addr); >> +#else >> +static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {} >> +static inline u64 get_l2_indirect_reg(u64 reg_addr) >> +{ >> +return 0; >> +} > > Surely it would be better to error out on any unintentional use of these > at build time? This allows building code which is common to ARM SoCs and QCOM SoCs without having to ifdef out the QCOM-specific pieces. > > Thanks, > Mark. > Thanks, Neil -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
On 5/23/2016 01:25 PM, Mark Rutland wrote: > On Fri, May 20, 2016 at 03:13:07PM -0400, Neil Leeder wrote: >> L2 registers are accessed using a select register and data >> register pair. To prevent multiple concurrent writes to the >> select register by independent drivers, the write to the >> select register and the associated access of the data register >> are protected with a lock. All drivers accessing the L2 >> registers use the set and get functions provided by >> l2-accessors to ensure correct reads and writes to L2 registers. > > What will this be used for? (i.e. which drivers want to touch the L2 > registers?). > > Generally we expect FW to configure the caches and interconnect > appropriately. The primary use is in the L2 PMU driver, which will be posted shortly. > >> Signed-off-by: Neil Leeder >> --- >> drivers/soc/qcom/Kconfig | 9 + >> drivers/soc/qcom/Makefile | 1 + >> drivers/soc/qcom/l2-accessors.c | 66 >> +++ >> include/linux/soc/qcom/l2-accessors.h | 27 ++ >> 4 files changed, 103 insertions(+) >> create mode 100644 drivers/soc/qcom/l2-accessors.c >> create mode 100644 include/linux/soc/qcom/l2-accessors.h > > These are awfully generic file names (and function names). Which SoCs > does this apply to? > > It would be good to give these more specific names. It's under soc/qcom, and dependent on ARCH_QCOM and (in v2) also on ARM64. It applies to all QCOM ARM64 SoCs. Given that it can only be used in a QCOM driver, and the include path has qcom in it, I'd prefer not to add redundancy by adding another qcom in there. >> diff --git a/include/linux/soc/qcom/l2-accessors.h >> b/include/linux/soc/qcom/l2-accessors.h >> new file mode 100644 >> index 000..563c114 >> --- /dev/null >> +++ b/include/linux/soc/qcom/l2-accessors.h >> @@ -0,0 +1,27 @@ >> +/* >> + * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + > >> +#ifndef __QCOM_L2_ACCESSORS_H >> +#define __QCOM_L2_ACCESSORS_H >> + >> +#ifdef CONFIG_QCOM_L2_ACCESSORS >> +void set_l2_indirect_reg(u64 reg_addr, u64 val); >> +u64 get_l2_indirect_reg(u64 reg_addr); >> +#else >> +static inline void set_l2_indirect_reg(u64 reg_addr, u64 val) {} >> +static inline u64 get_l2_indirect_reg(u64 reg_addr) >> +{ >> +return 0; >> +} > > Surely it would be better to error out on any unintentional use of these > at build time? This allows building code which is common to ARM SoCs and QCOM SoCs without having to ifdef out the QCOM-specific pieces. > > Thanks, > Mark. > Thanks, Neil -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
[GIT PULL] ftracetest: Use proper logic to find process PID
Linus, Reviewing the selftest I recently submitted, I realize that the second part of it uses my old hack to get the PID of the spawned background tasks, which doesn't work for all shells, instead of the common use of $!. Please pull the latest trace-v4.7-3 tree, which can be found at: git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git trace-v4.7-3 Tag SHA1: 773c674f8cf1e96253bef9889e5e26f0f3e8dcbb Head SHA1: 97f8827a8c7963756ae7d3ee898675b4667eca73 Steven Rostedt (Red Hat) (1): ftracetest: Use proper logic to find process PID .../selftests/ftrace/test.d/instances/instance-event.tc | 15 +-- 1 file changed, 5 insertions(+), 10 deletions(-) --- commit 97f8827a8c7963756ae7d3ee898675b4667eca73 Author: Steven Rostedt (Red Hat)Date: Mon May 23 10:04:46 2016 -0400 ftracetest: Use proper logic to find process PID Half of the test in instance-event.tc was updated to use $! to find the PID of the previous background process that was launched, but the second part of the test still used the parsing of "jobs", which does not work on all shells like $! does. Signed-off-by: Steven Rostedt diff --git a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc index 5f2abd03f16b..4c5a061a5b4e 100644 --- a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc +++ b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc @@ -92,28 +92,23 @@ instance_slam() { } instance_slam & -x=`jobs -l` -p1=`echo $x | cut -d' ' -f2` +p1=$! echo $p1 instance_slam & -x=`jobs -l | tail -1` -p2=`echo $x | cut -d' ' -f2` +p2=$! echo $p2 instance_slam & -x=`jobs -l | tail -1` -p3=`echo $x | cut -d' ' -f2` +p3=$! echo $p3 instance_slam & -x=`jobs -l | tail -1` -p4=`echo $x | cut -d' ' -f2` +p4=$! echo $p4 instance_slam & -x=`jobs -l | tail -1` -p5=`echo $x | cut -d' ' -f2` +p5=$! echo $p5 ls -lR >/dev/null
[GIT PULL] ftracetest: Use proper logic to find process PID
Linus, Reviewing the selftest I recently submitted, I realize that the second part of it uses my old hack to get the PID of the spawned background tasks, which doesn't work for all shells, instead of the common use of $!. Please pull the latest trace-v4.7-3 tree, which can be found at: git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git trace-v4.7-3 Tag SHA1: 773c674f8cf1e96253bef9889e5e26f0f3e8dcbb Head SHA1: 97f8827a8c7963756ae7d3ee898675b4667eca73 Steven Rostedt (Red Hat) (1): ftracetest: Use proper logic to find process PID .../selftests/ftrace/test.d/instances/instance-event.tc | 15 +-- 1 file changed, 5 insertions(+), 10 deletions(-) --- commit 97f8827a8c7963756ae7d3ee898675b4667eca73 Author: Steven Rostedt (Red Hat) Date: Mon May 23 10:04:46 2016 -0400 ftracetest: Use proper logic to find process PID Half of the test in instance-event.tc was updated to use $! to find the PID of the previous background process that was launched, but the second part of the test still used the parsing of "jobs", which does not work on all shells like $! does. Signed-off-by: Steven Rostedt diff --git a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc index 5f2abd03f16b..4c5a061a5b4e 100644 --- a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc +++ b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc @@ -92,28 +92,23 @@ instance_slam() { } instance_slam & -x=`jobs -l` -p1=`echo $x | cut -d' ' -f2` +p1=$! echo $p1 instance_slam & -x=`jobs -l | tail -1` -p2=`echo $x | cut -d' ' -f2` +p2=$! echo $p2 instance_slam & -x=`jobs -l | tail -1` -p3=`echo $x | cut -d' ' -f2` +p3=$! echo $p3 instance_slam & -x=`jobs -l | tail -1` -p4=`echo $x | cut -d' ' -f2` +p4=$! echo $p4 instance_slam & -x=`jobs -l | tail -1` -p5=`echo $x | cut -d' ' -f2` +p5=$! echo $p5 ls -lR >/dev/null
Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
On 5/23/2016 01:04 PM, Stephen Boyd wrote: > On 05/23/2016 08:43 AM, Neil Leeder wrote: >> >> On 5/20/2016 05:19 PM, Stephen Boyd wrote: >> >>> >>> Is there a patch to add sysreg.h to arch/arm? It would be nice to use >>> one l2 accessor API on arm64 and arm. >>> >> Sounds like a good thing for the next person who submits a krait L2 patch to >> consider. >> > > Heh, ok. If it isn't supported in this patch then we need to make the > config depend on ARM64 so that this doesn't fail to compile on ARM. > Ok, I'll add that in v2 too. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH] soc: qcom: provide mechanism for drivers to access L2 registers
On 5/23/2016 01:04 PM, Stephen Boyd wrote: > On 05/23/2016 08:43 AM, Neil Leeder wrote: >> >> On 5/20/2016 05:19 PM, Stephen Boyd wrote: >> >>> >>> Is there a patch to add sysreg.h to arch/arm? It would be nice to use >>> one l2 accessor API on arm64 and arm. >>> >> Sounds like a good thing for the next person who submits a krait L2 patch to >> consider. >> > > Heh, ok. If it isn't supported in this patch then we need to make the > config depend on ARM64 so that this doesn't fail to compile on ARM. > Ok, I'll add that in v2 too. -- Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [v2 PATCH] mm: move page_ext_init after all struct pages are initialized
On Mon 23-05-16 09:42:00, Shi, Yang wrote: > On 5/23/2016 12:31 AM, Michal Hocko wrote: > > On Fri 20-05-16 08:41:09, Shi, Yang wrote: > > > On 5/20/2016 6:16 AM, Michal Hocko wrote: > > > > On Thu 19-05-16 15:13:26, Yang Shi wrote: > > > > [...] > > > > > diff --git a/init/main.c b/init/main.c > > > > > index b3c6e36..2075faf 100644 > > > > > --- a/init/main.c > > > > > +++ b/init/main.c > > > > > @@ -606,7 +606,6 @@ asmlinkage __visible void __init > > > > > start_kernel(void) > > > > > initrd_start = 0; > > > > > } > > > > > #endif > > > > > - page_ext_init(); > > > > > debug_objects_mem_init(); > > > > > kmemleak_init(); > > > > > setup_per_cpu_pageset(); > > > > > @@ -1004,6 +1003,8 @@ static noinline void __init > > > > > kernel_init_freeable(void) > > > > > sched_init_smp(); > > > > > > > > > > page_alloc_init_late(); > > > > > + /* Initialize page ext after all struct pages are initializaed > > > > > */ > > > > > + page_ext_init(); > > > > > > > > > > do_basic_setup(); > > > > > > > > I might be missing something but don't we have the same problem with > > > > CONFIG_FLATMEM? page_ext_init_flatmem is called way earlier. Or > > > > CONFIG_DEFERRED_STRUCT_PAGE_INIT is never enabled for CONFIG_FLATMEM? > > > > > > Yes, CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which > > > depends on SPARSEMEM. So, this config is not valid for FLATMEM at all. > > > > Well > > config MEMORY_HOTPLUG > > bool "Allow for memory hot-add" > > depends on SPARSEMEM || X86_64_ACPI_NUMA > > depends on ARCH_ENABLE_MEMORY_HOTPLUG > > > > I wasn't really sure about X86_64_ACPI_NUMA dependency branch which > > depends on X86_64 && NUMA && ACPI && PCI and that didn't sound like > > SPARSEMEM only. If the FLATMEM shouldn't exist with > > Actually, FLATMEMT depends on !NUMA. Ahh, OK, you are right! Thanks for the clarification. -- Michal Hocko SUSE Labs
Re: [v2 PATCH] mm: move page_ext_init after all struct pages are initialized
On Mon 23-05-16 09:42:00, Shi, Yang wrote: > On 5/23/2016 12:31 AM, Michal Hocko wrote: > > On Fri 20-05-16 08:41:09, Shi, Yang wrote: > > > On 5/20/2016 6:16 AM, Michal Hocko wrote: > > > > On Thu 19-05-16 15:13:26, Yang Shi wrote: > > > > [...] > > > > > diff --git a/init/main.c b/init/main.c > > > > > index b3c6e36..2075faf 100644 > > > > > --- a/init/main.c > > > > > +++ b/init/main.c > > > > > @@ -606,7 +606,6 @@ asmlinkage __visible void __init > > > > > start_kernel(void) > > > > > initrd_start = 0; > > > > > } > > > > > #endif > > > > > - page_ext_init(); > > > > > debug_objects_mem_init(); > > > > > kmemleak_init(); > > > > > setup_per_cpu_pageset(); > > > > > @@ -1004,6 +1003,8 @@ static noinline void __init > > > > > kernel_init_freeable(void) > > > > > sched_init_smp(); > > > > > > > > > > page_alloc_init_late(); > > > > > + /* Initialize page ext after all struct pages are initializaed > > > > > */ > > > > > + page_ext_init(); > > > > > > > > > > do_basic_setup(); > > > > > > > > I might be missing something but don't we have the same problem with > > > > CONFIG_FLATMEM? page_ext_init_flatmem is called way earlier. Or > > > > CONFIG_DEFERRED_STRUCT_PAGE_INIT is never enabled for CONFIG_FLATMEM? > > > > > > Yes, CONFIG_DEFERRED_STRUCT_PAGE_INIT depends on MEMORY_HOTPLUG which > > > depends on SPARSEMEM. So, this config is not valid for FLATMEM at all. > > > > Well > > config MEMORY_HOTPLUG > > bool "Allow for memory hot-add" > > depends on SPARSEMEM || X86_64_ACPI_NUMA > > depends on ARCH_ENABLE_MEMORY_HOTPLUG > > > > I wasn't really sure about X86_64_ACPI_NUMA dependency branch which > > depends on X86_64 && NUMA && ACPI && PCI and that didn't sound like > > SPARSEMEM only. If the FLATMEM shouldn't exist with > > Actually, FLATMEMT depends on !NUMA. Ahh, OK, you are right! Thanks for the clarification. -- Michal Hocko SUSE Labs
Re: [PATCH v2] KVM: halt-polling: poll if emulated lapic timer will fire soon
On Sun, May 22, 2016 at 6:26 PM, Yang Zhangwrote: > On 2016/5/21 2:37, David Matlack wrote: >> >> It's not obvious to me why polling for a timer interrupt would improve >> context switch latency. Can you explain a bit more? > > > We have a workload which using high resolution timer(less than 1ms) inside > guest. It rely on the timer to wakeup itself. Sometimes the timer is > expected to fired just after the VCPU is blocked due to execute halt > instruction. But the thread who is running in the CPU will turn off the > hardware interrupt for long time due to disk access. This will cause the > timer interrupt been blocked until the interrupt is re-open. Does this happen on the idle thread (swapper)? If not, halt-polling may not help; it only polls if there are no other runnable threads. > For optimization, we let VCPU to poll for a while if the next timer will > arrive soon before schedule out. And the result shows good when running > several workloads inside guest. Thanks for the explanation, I appreciate it. > > -- > best regards > yang
Re: [PATCH v2] KVM: halt-polling: poll if emulated lapic timer will fire soon
On Sun, May 22, 2016 at 6:26 PM, Yang Zhang wrote: > On 2016/5/21 2:37, David Matlack wrote: >> >> It's not obvious to me why polling for a timer interrupt would improve >> context switch latency. Can you explain a bit more? > > > We have a workload which using high resolution timer(less than 1ms) inside > guest. It rely on the timer to wakeup itself. Sometimes the timer is > expected to fired just after the VCPU is blocked due to execute halt > instruction. But the thread who is running in the CPU will turn off the > hardware interrupt for long time due to disk access. This will cause the > timer interrupt been blocked until the interrupt is re-open. Does this happen on the idle thread (swapper)? If not, halt-polling may not help; it only polls if there are no other runnable threads. > For optimization, we let VCPU to poll for a while if the next timer will > arrive soon before schedule out. And the result shows good when running > several workloads inside guest. Thanks for the explanation, I appreciate it. > > -- > best regards > yang
Re: [PATCH 1/4] isa: Allow ISA-style drivers on modern systems
On Mon, May 23, 2016 at 7:58 AM, William Breathitt Graywrote: > > For now, the ISA_BUS Kconfig option is only be available on X86 > architectures. Support for other architectures may be added as required. So I'd prefer to see that > +config ISA_BUS_API > + def_bool ISA part in arch/Kconfig. Why? Because other architectures _already_ define that ISA symbol, and we want the "ISA_BUS_API" to be a complete superset of ISA. So whenever ISA is enabled, ISA_BUS_API should be enabled. And the way you did that, that's not true. Now, if you were to enable ISA on ARM, you'd not get ISA_BUS_API. And that sounds insane to me. It also sounds *wrong* because it effectively changes the meaning of this: --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -10,7 +10,7 @@ obj-$(CONFIG_DMA_CMA) += dma-contiguous.o -obj-$(CONFIG_ISA) += isa.o +obj-$(CONFIG_ISA_BUS_API) += isa.o where now that "isa.c" file gets built only on x86, whereas it *used* to get built whenever ISA was enabled. So the reason I suggested a separate ISA_BUS_API config option (that then a particular architecture can choose to enable, in this case the x86 choice of selecting ISA_BUS) was _exactly_ this issue. The plain "ISA" config variable is not limited to x86, and the new subset of it (the ISA_BUS_API) thus also must not be limited to just x86. Linus
Re: [PATCH 1/4] isa: Allow ISA-style drivers on modern systems
On Mon, May 23, 2016 at 7:58 AM, William Breathitt Gray wrote: > > For now, the ISA_BUS Kconfig option is only be available on X86 > architectures. Support for other architectures may be added as required. So I'd prefer to see that > +config ISA_BUS_API > + def_bool ISA part in arch/Kconfig. Why? Because other architectures _already_ define that ISA symbol, and we want the "ISA_BUS_API" to be a complete superset of ISA. So whenever ISA is enabled, ISA_BUS_API should be enabled. And the way you did that, that's not true. Now, if you were to enable ISA on ARM, you'd not get ISA_BUS_API. And that sounds insane to me. It also sounds *wrong* because it effectively changes the meaning of this: --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -10,7 +10,7 @@ obj-$(CONFIG_DMA_CMA) += dma-contiguous.o -obj-$(CONFIG_ISA) += isa.o +obj-$(CONFIG_ISA_BUS_API) += isa.o where now that "isa.c" file gets built only on x86, whereas it *used* to get built whenever ISA was enabled. So the reason I suggested a separate ISA_BUS_API config option (that then a particular architecture can choose to enable, in this case the x86 choice of selecting ISA_BUS) was _exactly_ this issue. The plain "ISA" config variable is not limited to x86, and the new subset of it (the ISA_BUS_API) thus also must not be limited to just x86. Linus
Re: [PATCH v3] KVM: halt-polling: poll if emulated lapic timer will fire soon
On Sun, May 22, 2016 at 5:42 PM, Wanpeng Liwrote: > From: Wanpeng Li I'm ok with this patch, but I'd like to better understand the target workloads. What type of workloads do you expect to benefit from this? > > If an emulated lapic timer will fire soon(in the scope of 10us the > base of dynamic halt-polling, lower-end of message passing workload > latency TCP_RR's poll time < 10us) we can treat it as a short halt, > and poll to wait it fire, the fire callback apic_timer_fn() will set > KVM_REQ_PENDING_TIMER, and this flag will be check during busy poll. > This can avoid context switch overhead and the latency which we wake > up vCPU. > > This feature is slightly different from current advance expiration > way. Advance expiration rely on the vCPU is running(do polling before > vmentry). But in some cases, the timer interrupt may be blocked by > other thread(i.e., IF bit is clear) and vCPU cannot be scheduled to > run immediately. So even advance the timer early, vCPU may still see > the latency. But polling is different, it ensures the vCPU to aware > the timer expiration before schedule out. > > iperf TCP get ~6% bandwidth improvement. I think my question got lost in the previous thread :). Can you explain why TCP bandwidth improves with this patch? > > Cc: Paolo Bonzini > Cc: Radim Krčmář > Cc: David Matlack > Cc: Christian Borntraeger > Cc: Yang Zhang > Signed-off-by: Wanpeng Li > --- > v2 -> v3: > * add Yang's statement to patch description > v1 -> v2: > * add return statement to non-x86 archs > * capture never expire case for x86 (hrtimer is not started) > > arch/arm/include/asm/kvm_host.h | 4 > arch/arm64/include/asm/kvm_host.h | 4 > arch/mips/include/asm/kvm_host.h| 4 > arch/powerpc/include/asm/kvm_host.h | 4 > arch/s390/include/asm/kvm_host.h| 4 > arch/x86/kvm/lapic.c| 11 +++ > arch/x86/kvm/lapic.h| 1 + > arch/x86/kvm/x86.c | 5 + > include/linux/kvm_host.h| 1 + > virt/kvm/kvm_main.c | 14 ++ > 10 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4cd8732..a5fd858 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -284,6 +284,10 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) > {} > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > > static inline void kvm_arm_init_debug(void) {} > static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index d49399d..94e227a 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -359,6 +359,10 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) > {} > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > > void kvm_arm_init_debug(void); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > diff --git a/arch/mips/include/asm/kvm_host.h > b/arch/mips/include/asm/kvm_host.h > index 9a37a10..456bc42 100644 > --- a/arch/mips/include/asm/kvm_host.h > +++ b/arch/mips/include/asm/kvm_host.h > @@ -813,6 +813,10 @@ static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu > *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > > #endif /* __MIPS_KVM_HOST_H__ */ > diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > index ec35af3..5986c79 100644 > --- a/arch/powerpc/include/asm/kvm_host.h > +++ b/arch/powerpc/include/asm/kvm_host.h > @@ -729,5 +729,9 @@ static inline void kvm_arch_exit(void) {} > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > +static
Re: [PATCH v3] KVM: halt-polling: poll if emulated lapic timer will fire soon
On Sun, May 22, 2016 at 5:42 PM, Wanpeng Li wrote: > From: Wanpeng Li I'm ok with this patch, but I'd like to better understand the target workloads. What type of workloads do you expect to benefit from this? > > If an emulated lapic timer will fire soon(in the scope of 10us the > base of dynamic halt-polling, lower-end of message passing workload > latency TCP_RR's poll time < 10us) we can treat it as a short halt, > and poll to wait it fire, the fire callback apic_timer_fn() will set > KVM_REQ_PENDING_TIMER, and this flag will be check during busy poll. > This can avoid context switch overhead and the latency which we wake > up vCPU. > > This feature is slightly different from current advance expiration > way. Advance expiration rely on the vCPU is running(do polling before > vmentry). But in some cases, the timer interrupt may be blocked by > other thread(i.e., IF bit is clear) and vCPU cannot be scheduled to > run immediately. So even advance the timer early, vCPU may still see > the latency. But polling is different, it ensures the vCPU to aware > the timer expiration before schedule out. > > iperf TCP get ~6% bandwidth improvement. I think my question got lost in the previous thread :). Can you explain why TCP bandwidth improves with this patch? > > Cc: Paolo Bonzini > Cc: Radim Krčmář > Cc: David Matlack > Cc: Christian Borntraeger > Cc: Yang Zhang > Signed-off-by: Wanpeng Li > --- > v2 -> v3: > * add Yang's statement to patch description > v1 -> v2: > * add return statement to non-x86 archs > * capture never expire case for x86 (hrtimer is not started) > > arch/arm/include/asm/kvm_host.h | 4 > arch/arm64/include/asm/kvm_host.h | 4 > arch/mips/include/asm/kvm_host.h| 4 > arch/powerpc/include/asm/kvm_host.h | 4 > arch/s390/include/asm/kvm_host.h| 4 > arch/x86/kvm/lapic.c| 11 +++ > arch/x86/kvm/lapic.h| 1 + > arch/x86/kvm/x86.c | 5 + > include/linux/kvm_host.h| 1 + > virt/kvm/kvm_main.c | 14 ++ > 10 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4cd8732..a5fd858 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -284,6 +284,10 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) > {} > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > > static inline void kvm_arm_init_debug(void) {} > static inline void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) {} > diff --git a/arch/arm64/include/asm/kvm_host.h > b/arch/arm64/include/asm/kvm_host.h > index d49399d..94e227a 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -359,6 +359,10 @@ static inline void kvm_arch_sync_events(struct kvm *kvm) > {} > static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > > void kvm_arm_init_debug(void); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > diff --git a/arch/mips/include/asm/kvm_host.h > b/arch/mips/include/asm/kvm_host.h > index 9a37a10..456bc42 100644 > --- a/arch/mips/include/asm/kvm_host.h > +++ b/arch/mips/include/asm/kvm_host.h > @@ -813,6 +813,10 @@ static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu > *vcpu) {} > static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > > #endif /* __MIPS_KVM_HOST_H__ */ > diff --git a/arch/powerpc/include/asm/kvm_host.h > b/arch/powerpc/include/asm/kvm_host.h > index ec35af3..5986c79 100644 > --- a/arch/powerpc/include/asm/kvm_host.h > +++ b/arch/powerpc/include/asm/kvm_host.h > @@ -729,5 +729,9 @@ static inline void kvm_arch_exit(void) {} > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu) {} > +static inline u64 kvm_arch_timer_remaining(struct kvm_vcpu *vcpu) > +{ > + return -1ULL; > +} > > #endif /* __POWERPC_KVM_HOST_H__ */ > diff --git a/arch/s390/include/asm/kvm_host.h
Re: [PATCH] Input: wacom_w8001 - Ignore bogus idx values in interrupt
On Mon, May 23, 2016 at 9:52 AM, Dmitry Torokhovwrote: > On Sun, May 22, 2016 at 10:21:45PM -0700, Ping Cheng wrote: >> Hi Chris, >> >> On Sun, May 22, 2016 at 6:42 PM, Chris J Arges >> wrote: >> > I've noticed crashes when using my x60t using a coreboot bios. When using >> > the pen I can produce a crash simply by tapping a few times. This >> > generates an event which has an idx of 0xc. This in turn crashes the >> > machine because the array access is greater than W8001_MAX_LENGTH. This >> > patch checks for bogus values and filters them in order to prevent crashes. >> >> Thank you for submitting a patch in addition to reporting the issue. >> >> > Signed-off-by: Chris J Arges >> > --- >> > drivers/input/touchscreen/wacom_w8001.c | 9 + >> > 1 file changed, 9 insertions(+) >> > >> > diff --git a/drivers/input/touchscreen/wacom_w8001.c >> > b/drivers/input/touchscreen/wacom_w8001.c >> > index bab3c6a..c858200 100644 >> > --- a/drivers/input/touchscreen/wacom_w8001.c >> > +++ b/drivers/input/touchscreen/wacom_w8001.c >> > @@ -283,6 +283,15 @@ static irqreturn_t w8001_interrupt(struct serio >> > *serio, >> > unsigned char tmp; >> > >> > w8001->data[w8001->idx] = data; >> > + >> > + /* ignore bogus idx values */ >> > + if (w8001->idx >= W8001_MAX_LENGTH) { >> > + pr_info("w8001: ignored interrupt: data 0x%02x idx %d\n", >> > data, >> > + w8001->idx); >> > + w8001->idx = 0; >> > + return IRQ_HANDLED; >> > + } >> > + >> >> I don't have an x60t system to test with. I wonder if your system >> supports two finger touch or not. We at least have a bug in the code >> since W8001_MAX_LENGTH should be 13 instead of 11. How come no one had >> encountered that issue before? >> >> I'm going to email a patch to the list. Please test it and let us know >> your result. Maybe we still need your patch if your device doesn't >> support two finger touch or the idx=0xc can't be fixed by >> W8001_MAX_LENGTH=13. > > Just so we are clear this version of the patch is buggy as we check the > index only after [potentially] writing past the array bounds of > w8001->data[]. Thanks for the heads up. I noticed that last night. Since it breaks two-finger touch, we won't use it anyway. My other patch is still necessary though. You'll need to change: From: wacom to From: Ping Cheng I made it on a brand new system, which I didn't setup the environment properly. I can update the patch if that's what you like... Ping
Re: [PATCH] Input: wacom_w8001 - Ignore bogus idx values in interrupt
On Mon, May 23, 2016 at 9:52 AM, Dmitry Torokhov wrote: > On Sun, May 22, 2016 at 10:21:45PM -0700, Ping Cheng wrote: >> Hi Chris, >> >> On Sun, May 22, 2016 at 6:42 PM, Chris J Arges >> wrote: >> > I've noticed crashes when using my x60t using a coreboot bios. When using >> > the pen I can produce a crash simply by tapping a few times. This >> > generates an event which has an idx of 0xc. This in turn crashes the >> > machine because the array access is greater than W8001_MAX_LENGTH. This >> > patch checks for bogus values and filters them in order to prevent crashes. >> >> Thank you for submitting a patch in addition to reporting the issue. >> >> > Signed-off-by: Chris J Arges >> > --- >> > drivers/input/touchscreen/wacom_w8001.c | 9 + >> > 1 file changed, 9 insertions(+) >> > >> > diff --git a/drivers/input/touchscreen/wacom_w8001.c >> > b/drivers/input/touchscreen/wacom_w8001.c >> > index bab3c6a..c858200 100644 >> > --- a/drivers/input/touchscreen/wacom_w8001.c >> > +++ b/drivers/input/touchscreen/wacom_w8001.c >> > @@ -283,6 +283,15 @@ static irqreturn_t w8001_interrupt(struct serio >> > *serio, >> > unsigned char tmp; >> > >> > w8001->data[w8001->idx] = data; >> > + >> > + /* ignore bogus idx values */ >> > + if (w8001->idx >= W8001_MAX_LENGTH) { >> > + pr_info("w8001: ignored interrupt: data 0x%02x idx %d\n", >> > data, >> > + w8001->idx); >> > + w8001->idx = 0; >> > + return IRQ_HANDLED; >> > + } >> > + >> >> I don't have an x60t system to test with. I wonder if your system >> supports two finger touch or not. We at least have a bug in the code >> since W8001_MAX_LENGTH should be 13 instead of 11. How come no one had >> encountered that issue before? >> >> I'm going to email a patch to the list. Please test it and let us know >> your result. Maybe we still need your patch if your device doesn't >> support two finger touch or the idx=0xc can't be fixed by >> W8001_MAX_LENGTH=13. > > Just so we are clear this version of the patch is buggy as we check the > index only after [potentially] writing past the array bounds of > w8001->data[]. Thanks for the heads up. I noticed that last night. Since it breaks two-finger touch, we won't use it anyway. My other patch is still necessary though. You'll need to change: From: wacom to From: Ping Cheng I made it on a brand new system, which I didn't setup the environment properly. I can update the patch if that's what you like... Ping
Re: [PATCH] kernel/kcov: unproxify debugfs file's fops
On Mon, May 23, 2016 at 6:45 AM, Nicolai Stange <nicsta...@gmail.com> wrote: > Since commit 49d200deaa68 ("debugfs: prevent access to removed files' > private data"), a debugfs file's file_operations methods get proxied > through lifetime aware wrappers. > > However, only a certain subset of the file_operations members is supported > by debugfs and ->mmap isn't among them -- it appears to be NULL from the > VFS layer's perspective. > > This behaviour breaks the /sys/kernel/debug/kcov file introduced > concurrently with commit 5c9a8750a640 ("kernel: add kcov code coverage"). > > Since that file never gets removed, there is no file removal race and thus, > a lifetime checking proxy isn't needed. > > Avoid the proxying for /sys/kernel/debug/kcov by creating it via > debugfs_create_file_unsafe() rather than debugfs_create_file(). > > Fixes: 49d200deaa68 ("debugfs: prevent access to removed files' private > data") > Fixes: 5c9a8750a640 ("kernel: add kcov code coverage") > Signed-off-by: Nicolai Stange <nicsta...@gmail.com> > --- > Applicable to linux-next 20160523. > In particular, it depends on > - c64688081490 ("debugfs: add support for self-protecting attribute file > fops") > - 5c9a8750a640 ("kernel: add kcov code coverage") > > This issue has been debugged and reported by > Sasha Levin <sasha.le...@oracle.com>: > http://lkml.kernel.org/g/573f4200.3080...@oracle.com > > kernel/kcov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/kcov.c b/kernel/kcov.c > index a02f2dd..4c349dd 100644 > --- a/kernel/kcov.c > +++ b/kernel/kcov.c > @@ -264,7 +264,7 @@ static const struct file_operations kcov_fops = { > > static int __init kcov_init(void) > { > - if (!debugfs_create_file("kcov", 0600, NULL, NULL, _fops)) { > + if (!debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, > _fops)) { It might make sense to add a comment above this to describe why "unsafe" is not unsafe in this case. -Kees > pr_err("failed to create kcov in debugfs\n"); > return -ENOMEM; > } > -- > 2.8.2 > -- Kees Cook Chrome OS & Brillo Security
Re: [PATCH] kernel/kcov: unproxify debugfs file's fops
On Mon, May 23, 2016 at 6:45 AM, Nicolai Stange wrote: > Since commit 49d200deaa68 ("debugfs: prevent access to removed files' > private data"), a debugfs file's file_operations methods get proxied > through lifetime aware wrappers. > > However, only a certain subset of the file_operations members is supported > by debugfs and ->mmap isn't among them -- it appears to be NULL from the > VFS layer's perspective. > > This behaviour breaks the /sys/kernel/debug/kcov file introduced > concurrently with commit 5c9a8750a640 ("kernel: add kcov code coverage"). > > Since that file never gets removed, there is no file removal race and thus, > a lifetime checking proxy isn't needed. > > Avoid the proxying for /sys/kernel/debug/kcov by creating it via > debugfs_create_file_unsafe() rather than debugfs_create_file(). > > Fixes: 49d200deaa68 ("debugfs: prevent access to removed files' private > data") > Fixes: 5c9a8750a640 ("kernel: add kcov code coverage") > Signed-off-by: Nicolai Stange > --- > Applicable to linux-next 20160523. > In particular, it depends on > - c64688081490 ("debugfs: add support for self-protecting attribute file > fops") > - 5c9a8750a640 ("kernel: add kcov code coverage") > > This issue has been debugged and reported by > Sasha Levin : > http://lkml.kernel.org/g/573f4200.3080...@oracle.com > > kernel/kcov.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/kcov.c b/kernel/kcov.c > index a02f2dd..4c349dd 100644 > --- a/kernel/kcov.c > +++ b/kernel/kcov.c > @@ -264,7 +264,7 @@ static const struct file_operations kcov_fops = { > > static int __init kcov_init(void) > { > - if (!debugfs_create_file("kcov", 0600, NULL, NULL, _fops)) { > + if (!debugfs_create_file_unsafe("kcov", 0600, NULL, NULL, > _fops)) { It might make sense to add a comment above this to describe why "unsafe" is not unsafe in this case. -Kees > pr_err("failed to create kcov in debugfs\n"); > return -ENOMEM; > } > -- > 2.8.2 > -- Kees Cook Chrome OS & Brillo Security
Applied "regulator: mt6397: Constify struct regulator_ops" to the regulator tree
The patch regulator: mt6397: Constify struct regulator_ops has been applied to the regulator tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 7e3fc9960169ec5eab0ec6810fa24518ea3f3608 Mon Sep 17 00:00:00 2001 From: Henry ChenDate: Mon, 23 May 2016 15:30:16 +0800 Subject: [PATCH] regulator: mt6397: Constify struct regulator_ops Consitify the structure of regulator operations. Signed-off-by: Henry Chen Signed-off-by: Mark Brown --- drivers/regulator/mt6397-regulator.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/regulator/mt6397-regulator.c b/drivers/regulator/mt6397-regulator.c index 17a5b6c2d6a9..1c45abb94409 100644 --- a/drivers/regulator/mt6397-regulator.c +++ b/drivers/regulator/mt6397-regulator.c @@ -160,7 +160,7 @@ static int mt6397_get_status(struct regulator_dev *rdev) return (regval & info->qi) ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF; } -static struct regulator_ops mt6397_volt_range_ops = { +static const struct regulator_ops mt6397_volt_range_ops = { .list_voltage = regulator_list_voltage_linear_range, .map_voltage = regulator_map_voltage_linear_range, .set_voltage_sel = regulator_set_voltage_sel_regmap, @@ -172,7 +172,7 @@ static struct regulator_ops mt6397_volt_range_ops = { .get_status = mt6397_get_status, }; -static struct regulator_ops mt6397_volt_table_ops = { +static const struct regulator_ops mt6397_volt_table_ops = { .list_voltage = regulator_list_voltage_table, .map_voltage = regulator_map_voltage_iterate, .set_voltage_sel = regulator_set_voltage_sel_regmap, @@ -184,7 +184,7 @@ static struct regulator_ops mt6397_volt_table_ops = { .get_status = mt6397_get_status, }; -static struct regulator_ops mt6397_volt_fixed_ops = { +static const struct regulator_ops mt6397_volt_fixed_ops = { .list_voltage = regulator_list_voltage_linear, .enable = regulator_enable_regmap, .disable = regulator_disable_regmap, -- 2.8.1
Applied "spi: Add file patterns for spi device tree bindings" to the spi tree
The patch spi: Add file patterns for spi device tree bindings has been applied to the spi tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From c876fec76e3719734f8a055b799c8c4c1930fcfc Mon Sep 17 00:00:00 2001 From: Geert UytterhoevenDate: Sun, 22 May 2016 11:06:21 +0200 Subject: [PATCH] spi: Add file patterns for spi device tree bindings Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Signed-off-by: Mark Brown --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9c567a431d8d..3aaea9f66eae 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10511,6 +10511,7 @@ L: linux-...@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git Q: http://patchwork.kernel.org/project/spi-devel-general/list/ S: Maintained +F: Documentation/devicetree/bindings/spi/ F: Documentation/spi/ F: drivers/spi/ F: include/linux/spi/ -- 2.8.1
Re: sem_lock() vs qspinlocks
On Mon, May 23, 2016 at 5:25 AM, Peter Zijlstrawrote: > > Paul has smp_mb__after_unlock_lock() for the RCpc 'upgrade'. How about > something like: > > smp_mb__after_lock() I'd much rather make the naming be higher level. It's not necessarily going to be a "mb", and while the problem is about smp, the primitives it is synchronizing aren't actually smp-specific (ie you're synchronizing a lock that is relevant on UP too). So I'd just call it something like spin_lock_sync_after_lock(); because different locks might have different levels of serialization (ie maybe a spinlock needs one thing, and a mutex needs another - if we start worrying about ordering between spin_lock and mutex_is_locked(), for example, or between mutex_lock() and spin_is_locked()). Hmm? Linus
Applied "regulator: mt6397: Constify struct regulator_ops" to the regulator tree
The patch regulator: mt6397: Constify struct regulator_ops has been applied to the regulator tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 7e3fc9960169ec5eab0ec6810fa24518ea3f3608 Mon Sep 17 00:00:00 2001 From: Henry Chen Date: Mon, 23 May 2016 15:30:16 +0800 Subject: [PATCH] regulator: mt6397: Constify struct regulator_ops Consitify the structure of regulator operations. Signed-off-by: Henry Chen Signed-off-by: Mark Brown --- drivers/regulator/mt6397-regulator.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/regulator/mt6397-regulator.c b/drivers/regulator/mt6397-regulator.c index 17a5b6c2d6a9..1c45abb94409 100644 --- a/drivers/regulator/mt6397-regulator.c +++ b/drivers/regulator/mt6397-regulator.c @@ -160,7 +160,7 @@ static int mt6397_get_status(struct regulator_dev *rdev) return (regval & info->qi) ? REGULATOR_STATUS_ON : REGULATOR_STATUS_OFF; } -static struct regulator_ops mt6397_volt_range_ops = { +static const struct regulator_ops mt6397_volt_range_ops = { .list_voltage = regulator_list_voltage_linear_range, .map_voltage = regulator_map_voltage_linear_range, .set_voltage_sel = regulator_set_voltage_sel_regmap, @@ -172,7 +172,7 @@ static struct regulator_ops mt6397_volt_range_ops = { .get_status = mt6397_get_status, }; -static struct regulator_ops mt6397_volt_table_ops = { +static const struct regulator_ops mt6397_volt_table_ops = { .list_voltage = regulator_list_voltage_table, .map_voltage = regulator_map_voltage_iterate, .set_voltage_sel = regulator_set_voltage_sel_regmap, @@ -184,7 +184,7 @@ static struct regulator_ops mt6397_volt_table_ops = { .get_status = mt6397_get_status, }; -static struct regulator_ops mt6397_volt_fixed_ops = { +static const struct regulator_ops mt6397_volt_fixed_ops = { .list_voltage = regulator_list_voltage_linear, .enable = regulator_enable_regmap, .disable = regulator_disable_regmap, -- 2.8.1
Applied "spi: Add file patterns for spi device tree bindings" to the spi tree
The patch spi: Add file patterns for spi device tree bindings has been applied to the spi tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From c876fec76e3719734f8a055b799c8c4c1930fcfc Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Sun, 22 May 2016 11:06:21 +0200 Subject: [PATCH] spi: Add file patterns for spi device tree bindings Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Signed-off-by: Mark Brown --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 9c567a431d8d..3aaea9f66eae 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10511,6 +10511,7 @@ L: linux-...@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git Q: http://patchwork.kernel.org/project/spi-devel-general/list/ S: Maintained +F: Documentation/devicetree/bindings/spi/ F: Documentation/spi/ F: drivers/spi/ F: include/linux/spi/ -- 2.8.1
Re: sem_lock() vs qspinlocks
On Mon, May 23, 2016 at 5:25 AM, Peter Zijlstra wrote: > > Paul has smp_mb__after_unlock_lock() for the RCpc 'upgrade'. How about > something like: > > smp_mb__after_lock() I'd much rather make the naming be higher level. It's not necessarily going to be a "mb", and while the problem is about smp, the primitives it is synchronizing aren't actually smp-specific (ie you're synchronizing a lock that is relevant on UP too). So I'd just call it something like spin_lock_sync_after_lock(); because different locks might have different levels of serialization (ie maybe a spinlock needs one thing, and a mutex needs another - if we start worrying about ordering between spin_lock and mutex_is_locked(), for example, or between mutex_lock() and spin_is_locked()). Hmm? Linus
Applied "regulator: pv880x0: Clean up unnecessary header inclusion" to the regulator tree
The patch regulator: pv880x0: Clean up unnecessary header inclusion has been applied to the regulator tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From f88112de5a7f3da9ac436ae149c9095c2cb0f4c7 Mon Sep 17 00:00:00 2001 From: Axel LinDate: Mon, 23 May 2016 22:03:23 +0800 Subject: [PATCH] regulator: pv880x0: Clean up unnecessary header inclusion Signed-off-by: Axel Lin Signed-off-by: Mark Brown --- drivers/regulator/pv88060-regulator.c | 3 --- drivers/regulator/pv88080-regulator.c | 3 --- drivers/regulator/pv88090-regulator.c | 3 --- 3 files changed, 9 deletions(-) diff --git a/drivers/regulator/pv88060-regulator.c b/drivers/regulator/pv88060-regulator.c index c448b727f5f8..6c4afc73ecac 100644 --- a/drivers/regulator/pv88060-regulator.c +++ b/drivers/regulator/pv88060-regulator.c @@ -14,7 +14,6 @@ */ #include -#include #include #include #include @@ -25,8 +24,6 @@ #include #include #include -#include -#include #include "pv88060-regulator.h" #define PV88060_MAX_REGULATORS 14 diff --git a/drivers/regulator/pv88080-regulator.c b/drivers/regulator/pv88080-regulator.c index d7107566c429..81950bdb1cc4 100644 --- a/drivers/regulator/pv88080-regulator.c +++ b/drivers/regulator/pv88080-regulator.c @@ -14,7 +14,6 @@ */ #include -#include #include #include #include @@ -25,8 +24,6 @@ #include #include #include -#include -#include #include "pv88080-regulator.h" #define PV88080_MAX_REGULATORS 3 diff --git a/drivers/regulator/pv88090-regulator.c b/drivers/regulator/pv88090-regulator.c index 0057c6740d6f..421641175352 100644 --- a/drivers/regulator/pv88090-regulator.c +++ b/drivers/regulator/pv88090-regulator.c @@ -14,7 +14,6 @@ */ #include -#include #include #include #include @@ -25,8 +24,6 @@ #include #include #include -#include -#include #include "pv88090-regulator.h" #define PV88090_MAX_REGULATORS 5 -- 2.8.1
Applied "regulator: pv880x0: Clean up unnecessary header inclusion" to the regulator tree
The patch regulator: pv880x0: Clean up unnecessary header inclusion has been applied to the regulator tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From f88112de5a7f3da9ac436ae149c9095c2cb0f4c7 Mon Sep 17 00:00:00 2001 From: Axel Lin Date: Mon, 23 May 2016 22:03:23 +0800 Subject: [PATCH] regulator: pv880x0: Clean up unnecessary header inclusion Signed-off-by: Axel Lin Signed-off-by: Mark Brown --- drivers/regulator/pv88060-regulator.c | 3 --- drivers/regulator/pv88080-regulator.c | 3 --- drivers/regulator/pv88090-regulator.c | 3 --- 3 files changed, 9 deletions(-) diff --git a/drivers/regulator/pv88060-regulator.c b/drivers/regulator/pv88060-regulator.c index c448b727f5f8..6c4afc73ecac 100644 --- a/drivers/regulator/pv88060-regulator.c +++ b/drivers/regulator/pv88060-regulator.c @@ -14,7 +14,6 @@ */ #include -#include #include #include #include @@ -25,8 +24,6 @@ #include #include #include -#include -#include #include "pv88060-regulator.h" #define PV88060_MAX_REGULATORS 14 diff --git a/drivers/regulator/pv88080-regulator.c b/drivers/regulator/pv88080-regulator.c index d7107566c429..81950bdb1cc4 100644 --- a/drivers/regulator/pv88080-regulator.c +++ b/drivers/regulator/pv88080-regulator.c @@ -14,7 +14,6 @@ */ #include -#include #include #include #include @@ -25,8 +24,6 @@ #include #include #include -#include -#include #include "pv88080-regulator.h" #define PV88080_MAX_REGULATORS 3 diff --git a/drivers/regulator/pv88090-regulator.c b/drivers/regulator/pv88090-regulator.c index 0057c6740d6f..421641175352 100644 --- a/drivers/regulator/pv88090-regulator.c +++ b/drivers/regulator/pv88090-regulator.c @@ -14,7 +14,6 @@ */ #include -#include #include #include #include @@ -25,8 +24,6 @@ #include #include #include -#include -#include #include "pv88090-regulator.h" #define PV88090_MAX_REGULATORS 5 -- 2.8.1
Applied "regulator: mt6397: Add buck change mode regulator interface for mt6397" to the regulator tree
The patch regulator: mt6397: Add buck change mode regulator interface for mt6397 has been applied to the regulator tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 6d26507bd45bf09374d3744ee07fabebc87266f5 Mon Sep 17 00:00:00 2001 From: Henry ChenDate: Mon, 23 May 2016 15:13:31 +0800 Subject: [PATCH] regulator: mt6397: Add buck change mode regulator interface for mt6397 BUCKs of mt6397 have auto mode and pwm mode. User can use regulator interfaces to control modes Signed-off-by: Henry Chen Signed-off-by: Mark Brown --- drivers/regulator/mt6397-regulator.c | 89 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/drivers/regulator/mt6397-regulator.c b/drivers/regulator/mt6397-regulator.c index 1c45abb94409..c6c6aa85e4e8 100644 --- a/drivers/regulator/mt6397-regulator.c +++ b/drivers/regulator/mt6397-regulator.c @@ -23,6 +23,9 @@ #include #include +#define MT6397_BUCK_MODE_AUTO 0 +#define MT6397_BUCK_MODE_FORCE_PWM 1 + /* * MT6397 regulators' information * @@ -38,10 +41,14 @@ struct mt6397_regulator_info { u32 vselon_reg; u32 vselctrl_reg; u32 vselctrl_mask; + u32 modeset_reg; + u32 modeset_mask; + u32 modeset_shift; }; #define MT6397_BUCK(match, vreg, min, max, step, volt_ranges, enreg, \ - vosel, vosel_mask, voselon, vosel_ctrl) \ + vosel, vosel_mask, voselon, vosel_ctrl, _modeset_reg, \ + _modeset_shift) \ [MT6397_ID_##vreg] = { \ .desc = { \ .name = #vreg, \ @@ -62,6 +69,9 @@ struct mt6397_regulator_info { .vselon_reg = voselon, \ .vselctrl_reg = vosel_ctrl, \ .vselctrl_mask = BIT(1),\ + .modeset_reg = _modeset_reg,\ + .modeset_mask = BIT(_modeset_shift),\ + .modeset_shift = _modeset_shift \ } #define MT6397_LDO(match, vreg, ldo_volt_table, enreg, enbit, vosel, \ @@ -145,6 +155,63 @@ static const u32 ldo_volt_table7[] = { 130, 150, 180, 200, 250, 280, 300, 330, }; +static int mt6397_regulator_set_mode(struct regulator_dev *rdev, +unsigned int mode) +{ + struct mt6397_regulator_info *info = rdev_get_drvdata(rdev); + int ret, val; + + switch (mode) { + case REGULATOR_MODE_FAST: + val = MT6397_BUCK_MODE_FORCE_PWM; + break; + case REGULATOR_MODE_NORMAL: + val = MT6397_BUCK_MODE_AUTO; + break; + default: + ret = -EINVAL; + goto err_mode; + } + + dev_dbg(>dev, "mt6397 buck set_mode %#x, %#x, %#x, %#x\n", + info->modeset_reg, info->modeset_mask, + info->modeset_shift, val); + + val <<= info->modeset_shift; + ret = regmap_update_bits(rdev->regmap, info->modeset_reg, +info->modeset_mask, val); +err_mode: + if (ret != 0) { + dev_err(>dev, + "Failed to set mt6397 buck mode: %d\n", ret); + return ret; + } + + return 0; +} + +static unsigned int mt6397_regulator_get_mode(struct regulator_dev *rdev) +{ + struct mt6397_regulator_info *info = rdev_get_drvdata(rdev); + int ret, regval; + + ret = regmap_read(rdev->regmap, info->modeset_reg, ); + if (ret != 0) { + dev_err(>dev, + "Failed to get mt6397 buck mode: %d\n", ret); + return ret; + } + + switch ((regval & info->modeset_mask) >> info->modeset_shift) { + case MT6397_BUCK_MODE_AUTO: + return REGULATOR_MODE_NORMAL; +
Applied "regulator: mt6397: Add buck change mode regulator interface for mt6397" to the regulator tree
The patch regulator: mt6397: Add buck change mode regulator interface for mt6397 has been applied to the regulator tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 6d26507bd45bf09374d3744ee07fabebc87266f5 Mon Sep 17 00:00:00 2001 From: Henry Chen Date: Mon, 23 May 2016 15:13:31 +0800 Subject: [PATCH] regulator: mt6397: Add buck change mode regulator interface for mt6397 BUCKs of mt6397 have auto mode and pwm mode. User can use regulator interfaces to control modes Signed-off-by: Henry Chen Signed-off-by: Mark Brown --- drivers/regulator/mt6397-regulator.c | 89 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/drivers/regulator/mt6397-regulator.c b/drivers/regulator/mt6397-regulator.c index 1c45abb94409..c6c6aa85e4e8 100644 --- a/drivers/regulator/mt6397-regulator.c +++ b/drivers/regulator/mt6397-regulator.c @@ -23,6 +23,9 @@ #include #include +#define MT6397_BUCK_MODE_AUTO 0 +#define MT6397_BUCK_MODE_FORCE_PWM 1 + /* * MT6397 regulators' information * @@ -38,10 +41,14 @@ struct mt6397_regulator_info { u32 vselon_reg; u32 vselctrl_reg; u32 vselctrl_mask; + u32 modeset_reg; + u32 modeset_mask; + u32 modeset_shift; }; #define MT6397_BUCK(match, vreg, min, max, step, volt_ranges, enreg, \ - vosel, vosel_mask, voselon, vosel_ctrl) \ + vosel, vosel_mask, voselon, vosel_ctrl, _modeset_reg, \ + _modeset_shift) \ [MT6397_ID_##vreg] = { \ .desc = { \ .name = #vreg, \ @@ -62,6 +69,9 @@ struct mt6397_regulator_info { .vselon_reg = voselon, \ .vselctrl_reg = vosel_ctrl, \ .vselctrl_mask = BIT(1),\ + .modeset_reg = _modeset_reg,\ + .modeset_mask = BIT(_modeset_shift),\ + .modeset_shift = _modeset_shift \ } #define MT6397_LDO(match, vreg, ldo_volt_table, enreg, enbit, vosel, \ @@ -145,6 +155,63 @@ static const u32 ldo_volt_table7[] = { 130, 150, 180, 200, 250, 280, 300, 330, }; +static int mt6397_regulator_set_mode(struct regulator_dev *rdev, +unsigned int mode) +{ + struct mt6397_regulator_info *info = rdev_get_drvdata(rdev); + int ret, val; + + switch (mode) { + case REGULATOR_MODE_FAST: + val = MT6397_BUCK_MODE_FORCE_PWM; + break; + case REGULATOR_MODE_NORMAL: + val = MT6397_BUCK_MODE_AUTO; + break; + default: + ret = -EINVAL; + goto err_mode; + } + + dev_dbg(>dev, "mt6397 buck set_mode %#x, %#x, %#x, %#x\n", + info->modeset_reg, info->modeset_mask, + info->modeset_shift, val); + + val <<= info->modeset_shift; + ret = regmap_update_bits(rdev->regmap, info->modeset_reg, +info->modeset_mask, val); +err_mode: + if (ret != 0) { + dev_err(>dev, + "Failed to set mt6397 buck mode: %d\n", ret); + return ret; + } + + return 0; +} + +static unsigned int mt6397_regulator_get_mode(struct regulator_dev *rdev) +{ + struct mt6397_regulator_info *info = rdev_get_drvdata(rdev); + int ret, regval; + + ret = regmap_read(rdev->regmap, info->modeset_reg, ); + if (ret != 0) { + dev_err(>dev, + "Failed to get mt6397 buck mode: %d\n", ret); + return ret; + } + + switch ((regval & info->modeset_mask) >> info->modeset_shift) { + case MT6397_BUCK_MODE_AUTO: + return REGULATOR_MODE_NORMAL; + case MT6397_BUCK_MODE_FORCE_PWM: + return
Applied "ASoC: Add file patterns for sound device tree bindings" to the asoc tree
The patch ASoC: Add file patterns for sound device tree bindings has been applied to the asoc tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From afc9fb456dd0bc16633757c62e9857de588fdf57 Mon Sep 17 00:00:00 2001 From: Geert UytterhoevenDate: Sun, 22 May 2016 11:06:20 +0200 Subject: [PATCH] ASoC: Add file patterns for sound device tree bindings Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Signed-off-by: Mark Brown --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 09a9cf1e0a8a..9d8795ed25c2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10435,6 +10435,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git L: alsa-de...@alsa-project.org (moderated for non-subscribers) W: http://alsa-project.org/main/index.php/ASoC S: Supported +F: Documentation/devicetree/bindings/sound/ F: Documentation/sound/alsa/soc/ F: sound/soc/ F: include/sound/soc* -- 2.8.1
Applied "ASoC: kirkwood: fix build failure" to the asoc tree
The patch ASoC: kirkwood: fix build failure has been applied to the asoc tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From b01518ca88410195ace38ce755c1206588e5c167 Mon Sep 17 00:00:00 2001 From: Sudip MukherjeeDate: Mon, 23 May 2016 16:00:54 +0530 Subject: [PATCH] ASoC: kirkwood: fix build failure While building m32r allmodconfig the build failed with: ERROR: "bad_dma_ops" [sound/soc/kirkwood/snd-soc-kirkwood.ko] undefined! To satisfy the dependency CONFIG_SND_KIRKWOOD_SOC should depend on HAS_DMA. Signed-off-by: Sudip Mukherjee Signed-off-by: Mark Brown --- sound/soc/kirkwood/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/soc/kirkwood/Kconfig b/sound/soc/kirkwood/Kconfig index 132bb83f8e99..bc3c7b5ac752 100644 --- a/sound/soc/kirkwood/Kconfig +++ b/sound/soc/kirkwood/Kconfig @@ -1,6 +1,7 @@ config SND_KIRKWOOD_SOC tristate "SoC Audio for the Marvell Kirkwood and Dove chips" depends on ARCH_DOVE || ARCH_MVEBU || COMPILE_TEST + depends on HAS_DMA help Say Y or M if you want to add support for codecs attached to the Kirkwood I2S interface. You will also need to select the -- 2.8.1
Applied "ASoC: Add file patterns for sound device tree bindings" to the asoc tree
The patch ASoC: Add file patterns for sound device tree bindings has been applied to the asoc tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From afc9fb456dd0bc16633757c62e9857de588fdf57 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Sun, 22 May 2016 11:06:20 +0200 Subject: [PATCH] ASoC: Add file patterns for sound device tree bindings Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Signed-off-by: Mark Brown --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 09a9cf1e0a8a..9d8795ed25c2 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10435,6 +10435,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git L: alsa-de...@alsa-project.org (moderated for non-subscribers) W: http://alsa-project.org/main/index.php/ASoC S: Supported +F: Documentation/devicetree/bindings/sound/ F: Documentation/sound/alsa/soc/ F: sound/soc/ F: include/sound/soc* -- 2.8.1
Applied "ASoC: kirkwood: fix build failure" to the asoc tree
The patch ASoC: kirkwood: fix build failure has been applied to the asoc tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From b01518ca88410195ace38ce755c1206588e5c167 Mon Sep 17 00:00:00 2001 From: Sudip Mukherjee Date: Mon, 23 May 2016 16:00:54 +0530 Subject: [PATCH] ASoC: kirkwood: fix build failure While building m32r allmodconfig the build failed with: ERROR: "bad_dma_ops" [sound/soc/kirkwood/snd-soc-kirkwood.ko] undefined! To satisfy the dependency CONFIG_SND_KIRKWOOD_SOC should depend on HAS_DMA. Signed-off-by: Sudip Mukherjee Signed-off-by: Mark Brown --- sound/soc/kirkwood/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/sound/soc/kirkwood/Kconfig b/sound/soc/kirkwood/Kconfig index 132bb83f8e99..bc3c7b5ac752 100644 --- a/sound/soc/kirkwood/Kconfig +++ b/sound/soc/kirkwood/Kconfig @@ -1,6 +1,7 @@ config SND_KIRKWOOD_SOC tristate "SoC Audio for the Marvell Kirkwood and Dove chips" depends on ARCH_DOVE || ARCH_MVEBU || COMPILE_TEST + depends on HAS_DMA help Say Y or M if you want to add support for codecs attached to the Kirkwood I2S interface. You will also need to select the -- 2.8.1
Applied "regmap: Add file patterns for regmap device tree bindings" to the regmap tree
The patch regmap: Add file patterns for regmap device tree bindings has been applied to the regmap tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 9398a639c8b175460828ee7c45e6b7a4c3f586b9 Mon Sep 17 00:00:00 2001 From: Geert UytterhoevenDate: Sun, 22 May 2016 11:06:17 +0200 Subject: [PATCH] regmap: Add file patterns for regmap device tree bindings Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Signed-off-by: Mark Brown --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index a727d9959ecd..94a9af5d7f7d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9318,6 +9318,7 @@ M:Mark Brown L: linux-kernel@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git S: Supported +F: Documentation/devicetree/bindings/regmap/ F: drivers/base/regmap/ F: include/linux/regmap.h -- 2.8.1
Applied "regmap: Add file patterns for regmap device tree bindings" to the regmap tree
The patch regmap: Add file patterns for regmap device tree bindings has been applied to the regmap tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From 9398a639c8b175460828ee7c45e6b7a4c3f586b9 Mon Sep 17 00:00:00 2001 From: Geert Uytterhoeven Date: Sun, 22 May 2016 11:06:17 +0200 Subject: [PATCH] regmap: Add file patterns for regmap device tree bindings Submitters of device tree binding documentation may forget to CC the subsystem maintainer if this is missing. Signed-off-by: Geert Uytterhoeven Signed-off-by: Mark Brown --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index a727d9959ecd..94a9af5d7f7d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9318,6 +9318,7 @@ M:Mark Brown L: linux-kernel@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git S: Supported +F: Documentation/devicetree/bindings/regmap/ F: drivers/base/regmap/ F: include/linux/regmap.h -- 2.8.1
Re: [PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup
Hi Matthias, Le 23/05/2016 18:32, Matthias Schiffer a écrit : > On 05/23/2016 04:01 PM, Cyrille Pitchen wrote: >> Hi Matthias, >> >> Le 18/05/2016 15:32, Matthias Schiffer a écrit : >>> This patch has been tested in OpenWrt for a few months and seems to work >>> correctly. >>> >>> Signed-off-by: Felix Fietkau>>> Signed-off-by: Matthias Schiffer >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 157841d..d681003 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -1304,6 +1304,7 @@ int spi_nor_scan(struct spi_nor *nor, const char >>> *name, enum read_mode mode) >>> >>> if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || >>> JEDEC_MFR(info) == SNOR_MFR_INTEL || >>> + JEDEC_MFR(info) == SNOR_MFR_MACRONIX || >>> JEDEC_MFR(info) == SNOR_MFR_SST || >>> info->flags & SPI_NOR_HAS_LOCK) { >>> write_enable(nor); >>> >> The line following this patch chunk is "write_sr(nor, 0);" however, if I >> refer >> to the Macronix mx25l25673g datasheet about the Status Register, bits[5:2] >> (BP0, BP1, BP2 and BP3) are non-volatile and define the protected area. >> Also bit6 (Quad Enable) is also non-volatile and is used to reassign #WP and >> #Hold pins to IO2 and IO3 functions needed by Quad SPI protocols on many >> other >> Macronix memories (indeed on the 73g part, the Quad Enable bit is always 1). >> >> So you should not write 0 directly into the Status Register if you only want >> to >> clear bit7 (Status Register Write Disable): use a read, modify, write >> sequence >> instead. >> >> >> Best regards, >> >> Cyrille >> > > Hi, > clearing the protected area (BP0..BP3) is exactly what this code is > supposed to do; it was already doing this for Atmel, Intel and SST flash, > and this patch series makes it behave the same for Macronix and Winbond. > > I see that the mx25l25673g defaults to unprotected according to the > datasheet, but removing the protection again should not hurt. If there is > actually a problem with writing 0 to the Quad Enable bit, we can of course > make this conditional and/or just clear the relevant bits. > > I've observed the mx25l6405d coming up with protection bits set (the > datasheet doesn't seem to specify the initial state of the bits). It's also > possible that the software protection is enabled by the bootloader, which I > can't replace; but I'd argue that the kernel should try to get the flash > chip into a known state on boot, thus always removing the software protection. > > Most of the setup of the flash chip is done after the "write_sr(nor, 0);" > in spi_nor_scan(). Do you suspect that "write_sr(nor, 0);" might cause > problems anyways? Yes I do: it's likely to prevent us from adding support to the Macronix QPI mode which use the SPI 4-4-4 protocol. As all other Quad SPI protocols on Macronix memories, it requires the Quad Enable bit to be set so the #Write Protect, #Hold, and #Reset functions are disabled and the associated pins are reassigned to IO2 and IO3 data lines. Adding proper support of the QPI mode would allow us to handle the case where the Macronix memory has already entered its QPI mode before spi_nor_scan() is called. So previous commands (Read JEDEC ID 0xAF, ...) already use the SPI 4-4-4 protocol. Then if you clear the Quad Enable bit in the Status Register: 1 - I don't whether it safely makes the Macronix exit its QPI mode 2 - the spi-nor framework is already configured to use the SPI 4-4-4 but once the QE bit is cleared you could no longer use any Quad SPI protocol until you set this bit again. Also let forget the QPI mode for now, the Quad Enable bit is non-volatile: if you clear it then a spurious reset occurs before the spi-nor framework sets the bit back, some bootloaders might find the Macronix memory in a unexpected state where Quad SPI protocols don't work. Besides, if you just want to clear the BPx bits, there is no need to also clear the SRWD non-volatile bit at the same time. I don't think it's good practice to modify non-volatile bits when not needed. > > Regards, > Matthias > Regards, Cyrille
Re: [PATCH 1/2] mtd: spi-nor: disable software protection for Macronix flash at startup
Hi Matthias, Le 23/05/2016 18:32, Matthias Schiffer a écrit : > On 05/23/2016 04:01 PM, Cyrille Pitchen wrote: >> Hi Matthias, >> >> Le 18/05/2016 15:32, Matthias Schiffer a écrit : >>> This patch has been tested in OpenWrt for a few months and seems to work >>> correctly. >>> >>> Signed-off-by: Felix Fietkau >>> Signed-off-by: Matthias Schiffer >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >>> index 157841d..d681003 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -1304,6 +1304,7 @@ int spi_nor_scan(struct spi_nor *nor, const char >>> *name, enum read_mode mode) >>> >>> if (JEDEC_MFR(info) == SNOR_MFR_ATMEL || >>> JEDEC_MFR(info) == SNOR_MFR_INTEL || >>> + JEDEC_MFR(info) == SNOR_MFR_MACRONIX || >>> JEDEC_MFR(info) == SNOR_MFR_SST || >>> info->flags & SPI_NOR_HAS_LOCK) { >>> write_enable(nor); >>> >> The line following this patch chunk is "write_sr(nor, 0);" however, if I >> refer >> to the Macronix mx25l25673g datasheet about the Status Register, bits[5:2] >> (BP0, BP1, BP2 and BP3) are non-volatile and define the protected area. >> Also bit6 (Quad Enable) is also non-volatile and is used to reassign #WP and >> #Hold pins to IO2 and IO3 functions needed by Quad SPI protocols on many >> other >> Macronix memories (indeed on the 73g part, the Quad Enable bit is always 1). >> >> So you should not write 0 directly into the Status Register if you only want >> to >> clear bit7 (Status Register Write Disable): use a read, modify, write >> sequence >> instead. >> >> >> Best regards, >> >> Cyrille >> > > Hi, > clearing the protected area (BP0..BP3) is exactly what this code is > supposed to do; it was already doing this for Atmel, Intel and SST flash, > and this patch series makes it behave the same for Macronix and Winbond. > > I see that the mx25l25673g defaults to unprotected according to the > datasheet, but removing the protection again should not hurt. If there is > actually a problem with writing 0 to the Quad Enable bit, we can of course > make this conditional and/or just clear the relevant bits. > > I've observed the mx25l6405d coming up with protection bits set (the > datasheet doesn't seem to specify the initial state of the bits). It's also > possible that the software protection is enabled by the bootloader, which I > can't replace; but I'd argue that the kernel should try to get the flash > chip into a known state on boot, thus always removing the software protection. > > Most of the setup of the flash chip is done after the "write_sr(nor, 0);" > in spi_nor_scan(). Do you suspect that "write_sr(nor, 0);" might cause > problems anyways? Yes I do: it's likely to prevent us from adding support to the Macronix QPI mode which use the SPI 4-4-4 protocol. As all other Quad SPI protocols on Macronix memories, it requires the Quad Enable bit to be set so the #Write Protect, #Hold, and #Reset functions are disabled and the associated pins are reassigned to IO2 and IO3 data lines. Adding proper support of the QPI mode would allow us to handle the case where the Macronix memory has already entered its QPI mode before spi_nor_scan() is called. So previous commands (Read JEDEC ID 0xAF, ...) already use the SPI 4-4-4 protocol. Then if you clear the Quad Enable bit in the Status Register: 1 - I don't whether it safely makes the Macronix exit its QPI mode 2 - the spi-nor framework is already configured to use the SPI 4-4-4 but once the QE bit is cleared you could no longer use any Quad SPI protocol until you set this bit again. Also let forget the QPI mode for now, the Quad Enable bit is non-volatile: if you clear it then a spurious reset occurs before the spi-nor framework sets the bit back, some bootloaders might find the Macronix memory in a unexpected state where Quad SPI protocols don't work. Besides, if you just want to clear the BPx bits, there is no need to also clear the SRWD non-volatile bit at the same time. I don't think it's good practice to modify non-volatile bits when not needed. > > Regards, > Matthias > Regards, Cyrille
Applied "ASoC: dwc: Add helper functions to disable/enable irqs" to the asoc tree
The patch ASoC: dwc: Add helper functions to disable/enable irqs has been applied to the asoc tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From df4e7be1832a1ca9fccfb2ed8771a17da6f7fa57 Mon Sep 17 00:00:00 2001 From: Jose AbreuDate: Mon, 23 May 2016 11:02:22 +0100 Subject: [PATCH] ASoC: dwc: Add helper functions to disable/enable irqs Helper functions to disable and enable the I2S interrupts were added. Only the interrupts of the used channels are enabled. Also, there is no need to enable irqs at dw_i2s_config(), they are already enabled at startup. Signed-off-by: Jose Abreu Cc: Carlos Palminha Cc: Mark Brown Cc: Liam Girdwood Cc: Jaroslav Kysela Cc: Takashi Iwai Cc: Rob Herring Cc: Alexey Brodkin Cc: linux-snps-...@lists.infradead.org Cc: alsa-de...@alsa-project.org Cc: devicet...@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Mark Brown --- sound/soc/dwc/designware_i2s.c | 68 +- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c index 0db69b7e9617..4c4f0dc24f10 100644 --- a/sound/soc/dwc/designware_i2s.c +++ b/sound/soc/dwc/designware_i2s.c @@ -145,26 +145,54 @@ static inline void i2s_clear_irqs(struct dw_i2s_dev *dev, u32 stream) } } -static void i2s_start(struct dw_i2s_dev *dev, - struct snd_pcm_substream *substream) +static inline void i2s_disable_irqs(struct dw_i2s_dev *dev, u32 stream, + int chan_nr) { - struct i2s_clk_config_data *config = >config; u32 i, irq; - i2s_write_reg(dev->i2s_base, IER, 1); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - for (i = 0; i < (config->chan_nr / 2); i++) { + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + for (i = 0; i < (chan_nr / 2); i++) { + irq = i2s_read_reg(dev->i2s_base, IMR(i)); + i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30); + } + } else { + for (i = 0; i < (chan_nr / 2); i++) { + irq = i2s_read_reg(dev->i2s_base, IMR(i)); + i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03); + } + } +} + +static inline void i2s_enable_irqs(struct dw_i2s_dev *dev, u32 stream, + int chan_nr) +{ + u32 i, irq; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + for (i = 0; i < (chan_nr / 2); i++) { irq = i2s_read_reg(dev->i2s_base, IMR(i)); i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x30); } - i2s_write_reg(dev->i2s_base, ITER, 1); } else { - for (i = 0; i < (config->chan_nr / 2); i++) { + for (i = 0; i < (chan_nr / 2); i++) { irq = i2s_read_reg(dev->i2s_base, IMR(i)); i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x03); } - i2s_write_reg(dev->i2s_base, IRER, 1); } +} + +static void i2s_start(struct dw_i2s_dev *dev, + struct snd_pcm_substream *substream) +{ + struct i2s_clk_config_data *config = >config; + + i2s_write_reg(dev->i2s_base, IER, 1); + i2s_enable_irqs(dev, substream->stream, config->chan_nr); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + i2s_write_reg(dev->i2s_base, ITER, 1); + else + i2s_write_reg(dev->i2s_base, IRER, 1); i2s_write_reg(dev->i2s_base, CER, 1); } @@ -172,24 +200,14 @@ static void i2s_start(struct dw_i2s_dev *dev, static void i2s_stop(struct dw_i2s_dev *dev, struct snd_pcm_substream *substream) { - u32 i = 0, irq; i2s_clear_irqs(dev, substream->stream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + if (substream->stream ==
Applied "ASoC: dwc: Add helper functions to disable/enable irqs" to the asoc tree
The patch ASoC: dwc: Add helper functions to disable/enable irqs has been applied to the asoc tree at git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark >From df4e7be1832a1ca9fccfb2ed8771a17da6f7fa57 Mon Sep 17 00:00:00 2001 From: Jose Abreu Date: Mon, 23 May 2016 11:02:22 +0100 Subject: [PATCH] ASoC: dwc: Add helper functions to disable/enable irqs Helper functions to disable and enable the I2S interrupts were added. Only the interrupts of the used channels are enabled. Also, there is no need to enable irqs at dw_i2s_config(), they are already enabled at startup. Signed-off-by: Jose Abreu Cc: Carlos Palminha Cc: Mark Brown Cc: Liam Girdwood Cc: Jaroslav Kysela Cc: Takashi Iwai Cc: Rob Herring Cc: Alexey Brodkin Cc: linux-snps-...@lists.infradead.org Cc: alsa-de...@alsa-project.org Cc: devicet...@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Mark Brown --- sound/soc/dwc/designware_i2s.c | 68 +- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c index 0db69b7e9617..4c4f0dc24f10 100644 --- a/sound/soc/dwc/designware_i2s.c +++ b/sound/soc/dwc/designware_i2s.c @@ -145,26 +145,54 @@ static inline void i2s_clear_irqs(struct dw_i2s_dev *dev, u32 stream) } } -static void i2s_start(struct dw_i2s_dev *dev, - struct snd_pcm_substream *substream) +static inline void i2s_disable_irqs(struct dw_i2s_dev *dev, u32 stream, + int chan_nr) { - struct i2s_clk_config_data *config = >config; u32 i, irq; - i2s_write_reg(dev->i2s_base, IER, 1); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - for (i = 0; i < (config->chan_nr / 2); i++) { + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + for (i = 0; i < (chan_nr / 2); i++) { + irq = i2s_read_reg(dev->i2s_base, IMR(i)); + i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x30); + } + } else { + for (i = 0; i < (chan_nr / 2); i++) { + irq = i2s_read_reg(dev->i2s_base, IMR(i)); + i2s_write_reg(dev->i2s_base, IMR(i), irq | 0x03); + } + } +} + +static inline void i2s_enable_irqs(struct dw_i2s_dev *dev, u32 stream, + int chan_nr) +{ + u32 i, irq; + + if (stream == SNDRV_PCM_STREAM_PLAYBACK) { + for (i = 0; i < (chan_nr / 2); i++) { irq = i2s_read_reg(dev->i2s_base, IMR(i)); i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x30); } - i2s_write_reg(dev->i2s_base, ITER, 1); } else { - for (i = 0; i < (config->chan_nr / 2); i++) { + for (i = 0; i < (chan_nr / 2); i++) { irq = i2s_read_reg(dev->i2s_base, IMR(i)); i2s_write_reg(dev->i2s_base, IMR(i), irq & ~0x03); } - i2s_write_reg(dev->i2s_base, IRER, 1); } +} + +static void i2s_start(struct dw_i2s_dev *dev, + struct snd_pcm_substream *substream) +{ + struct i2s_clk_config_data *config = >config; + + i2s_write_reg(dev->i2s_base, IER, 1); + i2s_enable_irqs(dev, substream->stream, config->chan_nr); + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + i2s_write_reg(dev->i2s_base, ITER, 1); + else + i2s_write_reg(dev->i2s_base, IRER, 1); i2s_write_reg(dev->i2s_base, CER, 1); } @@ -172,24 +200,14 @@ static void i2s_start(struct dw_i2s_dev *dev, static void i2s_stop(struct dw_i2s_dev *dev, struct snd_pcm_substream *substream) { - u32 i = 0, irq; i2s_clear_irqs(dev, substream->stream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) i2s_write_reg(dev->i2s_base, ITER, 0); - - for (i = 0; i < 4; i++) { - irq = i2s_read_reg(dev->i2s_base, IMR(i)); -
Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE
On Tue, May 17, 2016 at 12:48:53PM -0400, Jon Masters wrote: > On 05/17/2016 12:44 PM, Jon Masters wrote: > > > 1). During development of a platform, it is much easier to debug > > problems with tables if you can test replacement ones without having to > > respin the firmware. In the server world, you usually don't have the > > firmware source code, so to get it respun could be days-weeks even if > > you are working with the authors closely. We have practically used this > > feature on a number of platforms already and it will continue. > > For example, on one platform we were unable to fully boot RHEL(SA) due > to a bug in one of the ACPI tables. But I was able to boot the system to > a ramdisk containing a uuencode library and then write out the content > of the tables over the serial port, then decompile/patch/recompile, and > override replacement tables on the system. Then we beat the vendor up > with the fixes and the official firmware was corrected. Can you explain to me please why you can't do it with GRUB ? I am using mainline GRUB and its acpi command all the time to update static ACPI tables for testing new features (ie IORT) and it works just fine for me (and you can still override the DSDT, which is likely to be the main source of bugs, through the CONFIG_ACPI_CUSTOM_DSDT config option, that works on ARM in mainline with no changes required). I just want to understand if there is really a compelling reason for adding this stuff when we can easily implement its features through something that is usable today without any kernel changes. Thanks, Lorenzo
Re: [PATCH 3/3] ACPI: ARM64: support for ACPI_TABLE_UPGRADE
On Tue, May 17, 2016 at 12:48:53PM -0400, Jon Masters wrote: > On 05/17/2016 12:44 PM, Jon Masters wrote: > > > 1). During development of a platform, it is much easier to debug > > problems with tables if you can test replacement ones without having to > > respin the firmware. In the server world, you usually don't have the > > firmware source code, so to get it respun could be days-weeks even if > > you are working with the authors closely. We have practically used this > > feature on a number of platforms already and it will continue. > > For example, on one platform we were unable to fully boot RHEL(SA) due > to a bug in one of the ACPI tables. But I was able to boot the system to > a ramdisk containing a uuencode library and then write out the content > of the tables over the serial port, then decompile/patch/recompile, and > override replacement tables on the system. Then we beat the vendor up > with the fixes and the official firmware was corrected. Can you explain to me please why you can't do it with GRUB ? I am using mainline GRUB and its acpi command all the time to update static ACPI tables for testing new features (ie IORT) and it works just fine for me (and you can still override the DSDT, which is likely to be the main source of bugs, through the CONFIG_ACPI_CUSTOM_DSDT config option, that works on ARM in mainline with no changes required). I just want to understand if there is really a compelling reason for adding this stuff when we can easily implement its features through something that is usable today without any kernel changes. Thanks, Lorenzo
Re: [PATCH] x86: fix potential memleak in do_error_trap
On Mon 23-05-16 17:33:55, Oleg Nesterov wrote: > On 05/23, Michal Hocko wrote: > > > > @@ -271,6 +271,7 @@ static void do_error_trap(struct pt_regs *regs, long > > error_code, char *str, > > > > if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) != > > NOTIFY_STOP) { > > + memset(, 0, sizeof(info)); > > cond_local_irq_enable(regs); > > do_trap(trapnr, signr, str, regs, error_code, > > fill_trap_info(regs, signr, trapnr, )); > > at first glance fill_trap_info() initializes everything we will copy > to user-space in copy_siginfo_to_user(__SI_FAULT). Ohh, you are right. Dunno, how I managed to miss it. Sorry about the noise. > But even if not, shuldn't we change fill_trap_info() instead ? Yes that would be the proper place. -- Michal Hocko SUSE Labs
Re: [PATCH] x86: fix potential memleak in do_error_trap
On Mon 23-05-16 17:33:55, Oleg Nesterov wrote: > On 05/23, Michal Hocko wrote: > > > > @@ -271,6 +271,7 @@ static void do_error_trap(struct pt_regs *regs, long > > error_code, char *str, > > > > if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) != > > NOTIFY_STOP) { > > + memset(, 0, sizeof(info)); > > cond_local_irq_enable(regs); > > do_trap(trapnr, signr, str, regs, error_code, > > fill_trap_info(regs, signr, trapnr, )); > > at first glance fill_trap_info() initializes everything we will copy > to user-space in copy_siginfo_to_user(__SI_FAULT). Ohh, you are right. Dunno, how I managed to miss it. Sorry about the noise. > But even if not, shuldn't we change fill_trap_info() instead ? Yes that would be the proper place. -- Michal Hocko SUSE Labs
Re: [PATCH v2 2/2] i2c: qup: support SMBus block read
On 5/20/2016 2:31 AM, Sricharan wrote: Hi, @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg) goto err; qup->blk.pos++; + + /* Handle SMBus block read length */ + if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) { + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) { + ret = -EPROTO; + goto err; + } + msg->len += msg->buf[0]; + qup->pos = 0; + qup_i2c_set_read_mode_v2(qup, msg->len); + qup_i2c_issue_xfer_v2(qup, msg); + ret = qup_i2c_wait_for_complete(qup, msg); + if (ret) + goto err; Is the issue_xfer_v2 needed inside this here ? No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data that is indicated by the length we read earlier. So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop above this check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data inside this check, will not be enough ? Regards, Sricharan In testing, removing the call to qup_i2c_issue_xfer_v2() within the conditional statement for SMBus length causes failures. That transfer request is needed to read that block of data. Thanks, Austin
Re: [PATCH v2 2/2] i2c: qup: support SMBus block read
On 5/20/2016 2:31 AM, Sricharan wrote: Hi, @@ -1128,6 +1173,22 @@ static int qup_i2c_read_one_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg) goto err; qup->blk.pos++; + + /* Handle SMBus block read length */ + if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) { + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) { + ret = -EPROTO; + goto err; + } + msg->len += msg->buf[0]; + qup->pos = 0; + qup_i2c_set_read_mode_v2(qup, msg->len); + qup_i2c_issue_xfer_v2(qup, msg); + ret = qup_i2c_wait_for_complete(qup, msg); + if (ret) + goto err; Is the issue_xfer_v2 needed inside this here ? No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data that is indicated by the length we read earlier. So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop above this check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data inside this check, will not be enough ? Regards, Sricharan In testing, removing the call to qup_i2c_issue_xfer_v2() within the conditional statement for SMBus length causes failures. That transfer request is needed to read that block of data. Thanks, Austin
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon, May 23, 2016 at 08:14:11PM +0300, Ebru Akagunduz wrote: > Currently khugepaged makes swapin readahead under > down_write. This patch supplies to make swapin > readahead under down_read instead of down_write. > > The patch was tested with a test program that allocates > 800MB of memory, writes to it, and then sleeps. The system > was forced to swap out all. Afterwards, the test program > touches the area by writing, it skips a page in each > 20 pages of the area. > > Signed-off-by: Ebru Akagunduz> --- > mm/huge_memory.c | 33 +++-- > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index feee44c..668bc07 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2386,13 +2386,14 @@ static bool hugepage_vma_check(struct vm_area_struct > *vma) > * but with mmap_sem held to protect against vma changes. > */ > > -static void __collapse_huge_page_swapin(struct mm_struct *mm, > +static bool __collapse_huge_page_swapin(struct mm_struct *mm, > struct vm_area_struct *vma, > unsigned long address, pmd_t *pmd) > { > unsigned long _address; > pte_t *pte, pteval; > int swapped_in = 0, ret = 0; > + struct vm_area_struct *vma_orig = vma; > > pte = pte_offset_map(pmd, address); > for (_address = address; _address < address + HPAGE_PMD_NR*PAGE_SIZE; > @@ -2402,11 +2403,19 @@ static void __collapse_huge_page_swapin(struct > mm_struct *mm, > continue; > swapped_in++; > ret = do_swap_page(mm, vma, _address, pte, pmd, > - > FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_RETRY_NOWAIT, > +FAULT_FLAG_ALLOW_RETRY, > pteval); > + /* do_swap_page returns VM_FAULT_RETRY with released mmap_sem */ > + if (ret & VM_FAULT_RETRY) { > + down_read(>mmap_sem); > + vma = find_vma(mm, address); > + /* vma is no longer available, don't continue to swapin > */ > + if (vma != vma_orig) > + return false; > + } > if (ret & VM_FAULT_ERROR) { > trace_mm_collapse_huge_page_swapin(mm, swapped_in, 0); > - return; > + return false; > } > /* pte is unmapped now, we need to map it */ > pte = pte_offset_map(pmd, _address); > @@ -2414,6 +2423,7 @@ static void __collapse_huge_page_swapin(struct > mm_struct *mm, > pte--; > pte_unmap(pte); > trace_mm_collapse_huge_page_swapin(mm, swapped_in, 1); > + return true; > } > > static void collapse_huge_page(struct mm_struct *mm, > @@ -2459,7 +2469,7 @@ static void collapse_huge_page(struct mm_struct *mm, >* gup_fast later hanlded by the ptep_clear_flush and the VM >* handled by the anon_vma lock + PG_lock. >*/ > - down_write(>mmap_sem); > + down_read(>mmap_sem); > if (unlikely(khugepaged_test_exit(mm))) { > result = SCAN_ANY_PROCESS; > goto out; > @@ -2490,9 +2500,20 @@ static void collapse_huge_page(struct mm_struct *mm, >* Don't perform swapin readahead when the system is under pressure, >* to avoid unnecessary resource consumption. >*/ > - if (allocstall == curr_allocstall && swap != 0) > - __collapse_huge_page_swapin(mm, vma, address, pmd); > + if (allocstall == curr_allocstall && swap != 0) { > + /* > + * __collapse_huge_page_swapin always returns with mmap_sem > + * locked. If it fails, release mmap_sem and jump directly > + * label out. Continuing to collapse causes inconsistency. > + */ > + if (!__collapse_huge_page_swapin(mm, vma, address, pmd)) { > + up_read(>mmap_sem); > + goto out; > + } > + } > > + up_read(>mmap_sem); > + down_write(>mmap_sem); That's the critical point. How do you guarantee that the vma will not be destroyed (or changed) between up_read() and down_write()? You need at least find_vma() again. > anon_vma_lock_write(vma->anon_vma); > > pte = pte_offset_map(pmd, address); > -- > 1.9.1 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majord...@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"d...@kvack.org;> em...@kvack.org -- Kirill A. Shutemov
Re: [PATCH 3/3] mm, thp: make swapin readahead under down_read of mmap_sem
On Mon, May 23, 2016 at 08:14:11PM +0300, Ebru Akagunduz wrote: > Currently khugepaged makes swapin readahead under > down_write. This patch supplies to make swapin > readahead under down_read instead of down_write. > > The patch was tested with a test program that allocates > 800MB of memory, writes to it, and then sleeps. The system > was forced to swap out all. Afterwards, the test program > touches the area by writing, it skips a page in each > 20 pages of the area. > > Signed-off-by: Ebru Akagunduz > --- > mm/huge_memory.c | 33 +++-- > 1 file changed, 27 insertions(+), 6 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index feee44c..668bc07 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2386,13 +2386,14 @@ static bool hugepage_vma_check(struct vm_area_struct > *vma) > * but with mmap_sem held to protect against vma changes. > */ > > -static void __collapse_huge_page_swapin(struct mm_struct *mm, > +static bool __collapse_huge_page_swapin(struct mm_struct *mm, > struct vm_area_struct *vma, > unsigned long address, pmd_t *pmd) > { > unsigned long _address; > pte_t *pte, pteval; > int swapped_in = 0, ret = 0; > + struct vm_area_struct *vma_orig = vma; > > pte = pte_offset_map(pmd, address); > for (_address = address; _address < address + HPAGE_PMD_NR*PAGE_SIZE; > @@ -2402,11 +2403,19 @@ static void __collapse_huge_page_swapin(struct > mm_struct *mm, > continue; > swapped_in++; > ret = do_swap_page(mm, vma, _address, pte, pmd, > - > FAULT_FLAG_ALLOW_RETRY|FAULT_FLAG_RETRY_NOWAIT, > +FAULT_FLAG_ALLOW_RETRY, > pteval); > + /* do_swap_page returns VM_FAULT_RETRY with released mmap_sem */ > + if (ret & VM_FAULT_RETRY) { > + down_read(>mmap_sem); > + vma = find_vma(mm, address); > + /* vma is no longer available, don't continue to swapin > */ > + if (vma != vma_orig) > + return false; > + } > if (ret & VM_FAULT_ERROR) { > trace_mm_collapse_huge_page_swapin(mm, swapped_in, 0); > - return; > + return false; > } > /* pte is unmapped now, we need to map it */ > pte = pte_offset_map(pmd, _address); > @@ -2414,6 +2423,7 @@ static void __collapse_huge_page_swapin(struct > mm_struct *mm, > pte--; > pte_unmap(pte); > trace_mm_collapse_huge_page_swapin(mm, swapped_in, 1); > + return true; > } > > static void collapse_huge_page(struct mm_struct *mm, > @@ -2459,7 +2469,7 @@ static void collapse_huge_page(struct mm_struct *mm, >* gup_fast later hanlded by the ptep_clear_flush and the VM >* handled by the anon_vma lock + PG_lock. >*/ > - down_write(>mmap_sem); > + down_read(>mmap_sem); > if (unlikely(khugepaged_test_exit(mm))) { > result = SCAN_ANY_PROCESS; > goto out; > @@ -2490,9 +2500,20 @@ static void collapse_huge_page(struct mm_struct *mm, >* Don't perform swapin readahead when the system is under pressure, >* to avoid unnecessary resource consumption. >*/ > - if (allocstall == curr_allocstall && swap != 0) > - __collapse_huge_page_swapin(mm, vma, address, pmd); > + if (allocstall == curr_allocstall && swap != 0) { > + /* > + * __collapse_huge_page_swapin always returns with mmap_sem > + * locked. If it fails, release mmap_sem and jump directly > + * label out. Continuing to collapse causes inconsistency. > + */ > + if (!__collapse_huge_page_swapin(mm, vma, address, pmd)) { > + up_read(>mmap_sem); > + goto out; > + } > + } > > + up_read(>mmap_sem); > + down_write(>mmap_sem); That's the critical point. How do you guarantee that the vma will not be destroyed (or changed) between up_read() and down_write()? You need at least find_vma() again. > anon_vma_lock_write(vma->anon_vma); > > pte = pte_offset_map(pmd, address); > -- > 1.9.1 > > -- > To unsubscribe, send a message with 'unsubscribe linux-mm' in > the body to majord...@kvack.org. For more info on Linux MM, > see: http://www.linux-mm.org/ . > Don't email: mailto:"d...@kvack.org;> em...@kvack.org -- Kirill A. Shutemov
Re: [PATCH] mm: memcontrol: fix possible css ref leak on oom
On Mon 23-05-16 19:02:10, Vladimir Davydov wrote: > mem_cgroup_oom may be invoked multiple times while a process is handling > a page fault, in which case current->memcg_in_oom will be overwritten > leaking the previously taken css reference. Have you seen this happening? I was under impression that the page fault paths that have oom enabled will not retry allocations. > Signed-off-by: Vladimir DavydovThat being said I do not have anything against the patch. It is a good safety net I am just not sure this might happen right now and so the patch is not stable candidate. After clarification Acked-by: Michal Hocko > --- > mm/memcontrol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 5b48cd25951b..ef8797d34039 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1608,7 +1608,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg) > > static void mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order) > { > - if (!current->memcg_may_oom) > + if (!current->memcg_may_oom || current->memcg_in_oom) > return; > /* >* We are in the middle of the charge context here, so we > -- > 2.1.4 -- Michal Hocko SUSE Labs
Re: [PATCH] mm: memcontrol: fix possible css ref leak on oom
On Mon 23-05-16 19:02:10, Vladimir Davydov wrote: > mem_cgroup_oom may be invoked multiple times while a process is handling > a page fault, in which case current->memcg_in_oom will be overwritten > leaking the previously taken css reference. Have you seen this happening? I was under impression that the page fault paths that have oom enabled will not retry allocations. > Signed-off-by: Vladimir Davydov That being said I do not have anything against the patch. It is a good safety net I am just not sure this might happen right now and so the patch is not stable candidate. After clarification Acked-by: Michal Hocko > --- > mm/memcontrol.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 5b48cd25951b..ef8797d34039 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1608,7 +1608,7 @@ static void memcg_oom_recover(struct mem_cgroup *memcg) > > static void mem_cgroup_oom(struct mem_cgroup *memcg, gfp_t mask, int order) > { > - if (!current->memcg_may_oom) > + if (!current->memcg_may_oom || current->memcg_in_oom) > return; > /* >* We are in the middle of the charge context here, so we > -- > 2.1.4 -- Michal Hocko SUSE Labs