Linux Documentation
 help / color / mirror / Atom feed
* Re: [PATCH v10 2/5] arm64: KVM: export the capability to set guest SError syndrome
From: gengdongjiu @ 2018-03-18  7:33 UTC (permalink / raw)
  To: James Morse
  Cc: rkrcmar@redhat.com, corbet@lwn.net, christoffer.dall@linaro.org,
	marc.zyngier@arm.com, linux@armlinux.org.uk,
	catalin.marinas@arm.com, rjw@rjwysocki.net, bp@alien8.de,
	lenb@kernel.org, kvm@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, linux-acpi@vger.kernel.org,
	devel@acpica.org, Huangshaoyu (Shawn), Wuquanming

Hi James,
   Thanks for your time to review and give comments.

[...]
> > +
> > +8.14 KVM_CAP_ARM_SET_SERROR_ESR
> > +
> > +Architectures: arm, arm64
> > +
> > +This capability indicates that userspace can specify syndrome value
> > +reported to guest OS when guest takes a virtual SError interrupt exception.
> 
> "when userspace triggers a virtual SError"... how?

In the user space(QEMU), it will call kvm_arch_put_registers() or kvm_arch_get_registers() to set or get KVM registers through KVM_SET_ONE_REG/ KVM_GET_ONE_REG IOCTL, at the same time the two functions will separately call kvm_arm_vcpu_get_events() and kvm_arm_vcpu_set_events() to get/set vcpu events. If user space want to trigger a virtual SError with specified ESR, it only need to setup the kvm_vcpu_events struct(exception.serror_pending = 1; exception.serror_has_esr=1; serror_esr=xxxxx), then KVM will trigger this virtual SError.

userspace can trigger it at any time, for example, for debug purpose.  Or simulate a SError after recording a CPER for guest. But before triggering a virtual SError, it needs to know whether KVM has such capability, so KVM needs to export this capability to user space. If has this capability, User space will call kvm_arm_vcpu_set_events() to trigger a virtual SError.

> 
> 
> > +If KVM has this capability, userspace can only specify the ISS field
> > +for the ESR syndrome, can not specify the EC field which is not under control by KVM.
> 
> Where do I put the ESR?
> If you re-order this after the patch that adds the API, you can describe how this can be used.

Ok, thank a lot for your suggestion.

> 
> 
> Thanks,
> 
> James
> 
> 
> 
> > +If this virtual SError is taken to EL1 using AArch64, this value will
> > +be reported into ISS filed of ESR_EL1.
> > diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c index
> > 3256b92..38c8a64 100644
> > --- a/arch/arm64/kvm/reset.c
> > +++ b/arch/arm64/kvm/reset.c
> > @@ -77,6 +77,9 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
> >  	case KVM_CAP_ARM_PMU_V3:
> >  		r = kvm_arm_support_pmu_v3();
> >  		break;
> > +	case KVM_CAP_ARM_INJECT_SERROR_ESR:
> > +		r = cpus_have_const_cap(ARM64_HAS_RAS_EXTN);
> > +		break;
> >  	case KVM_CAP_SET_GUEST_DEBUG:
> >  	case KVM_CAP_VCPU_ATTRIBUTES:
> >  		r = 1;
> > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index
> > 8fb90a0..3587b33 100644
> > --- a/include/uapi/linux/kvm.h
> > +++ b/include/uapi/linux/kvm.h
> > @@ -934,6 +934,7 @@ struct kvm_ppc_resize_hpt {  #define
> > KVM_CAP_S390_AIS_MIGRATION 150  #define KVM_CAP_PPC_GET_CPU_CHAR 151
> > #define KVM_CAP_S390_BPB 152
> > +#define KVM_CAP_ARM_INJECT_SERROR_ESR 153
> >
> >  #ifdef KVM_CAP_IRQ_ROUTING
> >
> >

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 1/6] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From: Rob Herring @ 2018-03-18 12:52 UTC (permalink / raw)
  To: Karthikeyan Ramasubramanian
  Cc: corbet, andy.gross, david.brown, mark.rutland, wsa, gregkh,
	linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
	jslaby, evgreen, acourbot, swboyd, Sagar Dharia, Girish Mahadevan
In-Reply-To: <1521071931-9294-2-git-send-email-kramasub@codeaurora.org>

On Wed, Mar 14, 2018 at 05:58:46PM -0600, Karthikeyan Ramasubramanian wrote:
> Add device tree binding support for the QCOM GENI SE driver.
> 
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> ---
>  .../devicetree/bindings/soc/qcom/qcom,geni-se.txt  | 123 +++++++++++++++++++++
>  1 file changed, 123 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> new file mode 100644
> index 0000000..b71b5df
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> @@ -0,0 +1,123 @@
> +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
> +
> +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP) wrapper
> +is a programmable module for supporting a wide range of serial interfaces
> +like UART, SPI, I2C, I3C, etc. A single QUP module can provide upto 8 Serial
> +Interfaces, using its internal Serial Engines. The GENI Serial Engine QUP
> +Wrapper controller is modeled as a node with zero or more child nodes each
> +representing a serial engine.
> +
> +Required properties:
> +- compatible:		Must be "qcom,geni-se-qup".
> +- reg:			Must contain QUP register address and length.
> +- clock-names:		Must contain "m-ahb" and "s-ahb".
> +- clocks:		AHB clocks needed by the device.
> +
> +Required properties if child node exists:
> +- #address-cells: 	Must be <1> for Serial Engine Address
> +- #size-cells: 		Must be <1> for Serial Engine Address Size
> +- ranges: 		Must be present
> +
> +Properties for children:
> +
> +A GENI based QUP wrapper controller node can contain 0 or more child nodes
> +representing serial devices.  These serial devices can be a QCOM UART, I2C
> +controller, SPI controller, or some combination of aforementioned devices.
> +Please refer below the child node definitions for the supported serial
> +interface protocols.
> +
> +Qualcomm Technologies Inc. GENI Serial Engine based I2C Controller
> +
> +Required properties:
> +- compatible:		Must be "qcom,geni-i2c".
> +- reg: 			Must contain QUP register address and length.
> +- interrupts: 		Must contain I2C interrupt.
> +- clock-names: 		Must contain "se".
> +- clocks: 		Serial engine core clock needed by the device.
> +- #address-cells:	Must be <1> for I2C device address.
> +- #size-cells:		Must be <0> as I2C addresses have no size component.
> +
> +Optional property:
> +- clock-frequency:	Desired I2C bus clock frequency in Hz.
> +			When missing default to 400000Hz.
> +
> +Child nodes should conform to I2C bus binding as described in i2c.txt.
> +
> +Qualcomm Technologies Inc. GENI Serial Engine based UART Controller
> +
> +Required properties:
> +- compatible:		Must be "qcom,geni-debug-uart".
> +- reg: 			Must contain UART register location and length.
> +- interrupts: 		Must contain UART core interrupts.
> +- clock-names:		Must contain "se".
> +- clocks:		Serial engine core clock needed by the device.
> +
> +Qualcomm Technologies Inc. GENI Serial Engine based SPI Controller
> +
> +Required properties:
> +- compatible:		Must contain "qcom,geni-spi".
> +- reg:			Must contain SPI register location and length.
> +- interrupts:		Must contain SPI controller interrupts.
> +- clock-names:		Must contain "se".
> +- clocks:		Serial engine core clock needed by the device.
> +- spi-max-frequency:	Specifies maximum SPI clock frequency, units - Hz.
> +- #address-cells:	Must be <1> to define a chip select address on
> +			the SPI bus.
> +- #size-cells:		Must be <0>.
> +
> +Optional property:
> +- qcom,rt:		Indicates if the framework worker thread for this
> +			controller device should have real-time priority.

Sounds like a Linux issue, not a DT property. Given you have no driver 
yet, I'd drop this for now at least. With that,

Reviewed-by: Rob Herring <robh@kernel.org>

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 2/2] iio: chemical: sgp30: Support Sensirion SGPxx sensors
From: Rob Herring @ 2018-03-18 12:48 UTC (permalink / raw)
  To: Andreas Brauchli
  Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Mark Rutland, Jonathan Corbet, linux-iio,
	linux-kernel, devicetree, linux-doc
In-Reply-To: <58a6dcdc4097d60e7bcee61ec1d0cbdf8ad3ae49.camel@elementarea.net>

On Sat, Mar 10, 2018 at 11:07:30PM +0100, Andreas Brauchli wrote:
> Support Sensirion SGP30 and SGPC3 multi-pixel I2C gas sensors
> 
> Supported Features:
> 
> * Indoor Air Quality (IAQ) concentrations for
>   - tVOC (in_concentration_voc_input)
>   - CO2eq (in_concentration_co2_input) - SGP30 only
>   IAQ concentrations are periodically read out by a background thread
>   to allow the sensor to maintain its internal baseline.
> * Baseline support for IAQ (in_iaq_baseline, set_iaq_baseline)
> * Gas concentration signals
>   - Ethanol (in_concentration_ethanol_raw)
>   - H2 (in_concentration_h2_raw) - SGP30 only
> * On-chip self test (in_selftest)
> * Sensor interface version (in_feature_set_version)
> * Sensor serial number (in_serial_id)
> * Humidity compensation
>   With the help of an external humidity signal, the gas signals can be
>   humidity-compensated. The sensor performs the compensation on-chip.
> * Power mode switching between low power mode (1/2Hz) and
>   ultra low power mode (1/30Hz) sampling - SGPC3 only
> * Checksummed I2C communication
> 
> For all features, refer to the data sheet or the documentation in
> Documentation/iio/chemical/sgp30.txt for more details.
> 
> The ABI is documented in
> Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
> 
> Signed-off-by: Andreas Brauchli <andreas.brauchli@sensirion.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-chemical-sgp30       |   62 ++
>  .../bindings/iio/chemical/sensirion,sgp30.txt      |   15 +

Please split bindings to separate patch.

>  Documentation/iio/chemical/sgp30.txt               |   97 ++
>  drivers/iio/chemical/Kconfig                       |   13 +
>  drivers/iio/chemical/Makefile                      |    1 +
>  drivers/iio/chemical/sgp30.c                       | 1120 ++++++++++++++++++++
>  6 files changed, 1308 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-chemical-sgp30
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,sgp30.txt
>  create mode 100644 Documentation/iio/chemical/sgp30.txt
>  create mode 100644 drivers/iio/chemical/sgp30.c
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v16 06/13] task_isolation: userspace hard isolation from kernel
From: Yury Norov @ 2018-03-18 14:22 UTC (permalink / raw)
  To: Chris Metcalf
  Cc: Steven Rostedt, Ingo Molnar, Peter Zijlstra, Andrew Morton,
	Rik van Riel, Tejun Heo, Frederic Weisbecker, Thomas Gleixner,
	Paul E. McKenney, Christoph Lameter, Viresh Kumar,
	Catalin Marinas, Will Deacon, Andy Lutomirski, Michal Hocko,
	Jonathan Corbet, linux-mm, linux-doc, linux-api, linux-kernel
In-Reply-To: <1509728692-10460-7-git-send-email-cmetcalf@mellanox.com>

Hi Chris,

On Fri, Nov 03, 2017 at 01:04:45PM -0400, Chris Metcalf wrote:
> The existing nohz_full mode is designed as a "soft" isolation mode
> that makes tradeoffs to minimize userspace interruptions while
> still attempting to avoid overheads in the kernel entry/exit path,
> to provide 100% kernel semantics, etc.
> 
> However, some applications require a "hard" commitment from the
> kernel to avoid interruptions, in particular userspace device driver
> style applications, such as high-speed networking code.
> 
> This change introduces a framework to allow applications
> to elect to have the "hard" semantics as needed, specifying
> prctl(PR_TASK_ISOLATION, PR_TASK_ISOLATION_ENABLE) to do so.
> 
> The kernel must be built with the new TASK_ISOLATION Kconfig flag
> to enable this mode, and the kernel booted with an appropriate
> "nohz_full=CPULIST isolcpus=CPULIST" boot argument to enable
> nohz_full and isolcpus.  The "task_isolation" state is then indicated
> by setting a new task struct field, task_isolation_flag, to the
> value passed by prctl(), and also setting a TIF_TASK_ISOLATION
> bit in the thread_info flags.  When the kernel is returning to
> userspace from the prctl() call and sees TIF_TASK_ISOLATION set,
> it calls the new task_isolation_start() routine to arrange for
> the task to avoid being interrupted in the future.
> 
> With interrupts disabled, task_isolation_start() ensures that kernel
> subsystems that might cause a future interrupt are quiesced.  If it
> doesn't succeed, it adjusts the syscall return value to indicate that
> fact, and userspace can retry as desired.  In addition to stopping
> the scheduler tick, the code takes any actions that might avoid
> a future interrupt to the core, such as a worker thread being
> scheduled that could be quiesced now (e.g. the vmstat worker)
> or a future IPI to the core to clean up some state that could be
> cleaned up now (e.g. the mm lru per-cpu cache).
> 
> Once the task has returned to userspace after issuing the prctl(),
> if it enters the kernel again via system call, page fault, or any
> other exception or irq, the kernel will kill it with SIGKILL.
> In addition to sending a signal, the code supports a kernel
> command-line "task_isolation_debug" flag which causes a stack
> backtrace to be generated whenever a task loses isolation.
> 
> To allow the state to be entered and exited, the syscall checking
> test ignores the prctl(PR_TASK_ISOLATION) syscall so that we can
> clear the bit again later, and ignores exit/exit_group to allow
> exiting the task without a pointless signal being delivered.
> 
> The prctl() API allows for specifying a signal number to use instead
> of the default SIGKILL, to allow for catching the notification
> signal; for example, in a production environment, it might be
> helpful to log information to the application logging mechanism
> before exiting.  Or, the signal handler might choose to reset the
> program counter back to the code segment intended to be run isolated
> via prctl() to continue execution.
> 
> In a number of cases we can tell on a remote cpu that we are
> going to be interrupting the cpu, e.g. via an IPI or a TLB flush.
> In that case we generate the diagnostic (and optional stack dump)
> on the remote core to be able to deliver better diagnostics.
> If the interrupt is not something caught by Linux (e.g. a
> hypervisor interrupt) we can also request a reschedule IPI to
> be sent to the remote core so it can be sure to generate a
> signal to notify the process.
> 
> Separate patches that follow provide these changes for x86, tile,
> arm, and arm64.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@mellanox.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |   6 +
>  include/linux/isolation.h                       | 175 +++++++++++
>  include/linux/sched.h                           |   4 +
>  include/uapi/linux/prctl.h                      |   6 +
>  init/Kconfig                                    |  28 ++
>  kernel/Makefile                                 |   1 +
>  kernel/context_tracking.c                       |   2 +
>  kernel/isolation.c                              | 402 ++++++++++++++++++++++++
>  kernel/signal.c                                 |   2 +
>  kernel/sys.c                                    |   6 +
>  10 files changed, 631 insertions(+)
>  create mode 100644 include/linux/isolation.h
>  create mode 100644 kernel/isolation.c
 
[...]

> + * This routine is called from syscall entry, prevents most syscalls
> + * from executing, and if needed raises a signal to notify the process.
> + *
> + * Note that we have to stop isolation before we even print a message
> + * here, since otherwise we might end up reporting an interrupt due to
> + * kicking the printk handling code, rather than reporting the true
> + * cause of interrupt here.
> + */
> +int task_isolation_syscall(int syscall)
> +{

All callers of this function call it like this:
        if (work & _TIF_TASK_ISOLATION) {
                if (task_isolation_syscall(regs->syscallno) ==
                                -1)
                        return -1;
        }

Would it make sense to move check of _TIF_TASK_ISOLATION flag inside
the function?

> +	struct task_struct *task = current;
> +
> +	if (is_acceptable_syscall(syscall)) {
> +		stop_isolation(task);
> +		return 0;
> +	}
> +
> +	send_isolation_signal(task);
> +
> +	pr_warn("%s/%d (cpu %d): task_isolation lost due to syscall %d\n",
> +		task->comm, task->pid, smp_processor_id(), syscall);
> +	debug_dump_stack();
> +
> +	syscall_set_return_value(task, current_pt_regs(), -ERESTARTNOINTR, -1);
> +	return -1;
> +}

Yury
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v12 00/11] Application Data Integrity feature introduced by SPARC M7
From: David Miller @ 2018-03-18 15:08 UTC (permalink / raw)
  To: khalid.aziz
  Cc: dave.hansen, aarcange, akpm, allen.pais, aneesh.kumar,
	anthony.yznaga, arnd, benh, bob.picco, bsingharora, corbet,
	dan.j.williams, dave.jiang, david.j.aldridge, dwindsor, ebiederm,
	elena.reshetova, gregkh, hannes, henry.willard, hpa, hughd,
	imbrenda, jack, jag.raman, jane.chu, jglisse, jroedel, khalid,
	khandual, kirill.shutemov, kstewart, ktkhai, liam.merwick,
	linux-arch, linux-doc, linux-kernel, linux-mm, linuxppc-dev,
	linuxram, linux, me, mgorman, mgorman, mhocko, mike.kravetz,
	minchan, mingo
In-Reply-To: <cover.1519227112.git.khalid.aziz@oracle.com>

From: Khalid Aziz <khalid.aziz@oracle.com>
Date: Wed, 21 Feb 2018 10:15:42 -0700

> V12 changes:
> This series is same as v10 and v11 and was simply rebased on 4.16-rc2
> kernel and patch 11 was added to update signal delivery code to use the
> new helper functions added by Eric Biederman. Can mm maintainers please
> review patches 2, 7, 8 and 9 which are arch independent, and
> include/linux/mm.h and mm/ksm.c changes in patch 10 and ack these if
> everything looks good? 

Khalid I've applied this series to sparc-next, thank you!

But one thing has to be fixed up.

In uapi/asm/auxvec.h you conditionalize the ADI aux vectors on
CONFIG_SPARC64.

That's not correct, you should never control user facing definitions
based upon kernel configuration.

Also, both 32-bit and 64-bit applications running on ADI capable
machines want to compile against and use this informaiton.

So please remove these CPP checks.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 0/8] Ingenic JZ47xx Timer/Counter Unit drivers
From: Daniel Lezcano @ 2018-03-18 22:13 UTC (permalink / raw)
  To: Paul Cercueil, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Lee Jones, Ralf Baechle, Rob Herring, Jonathan Corbet,
	Mark Rutland
  Cc: James Hogan, Maarten ter Huurne, linux-clk, devicetree,
	linux-kernel, linux-mips, linux-doc
In-Reply-To: <20180317232901.14129-1-paul@crapouillou.net>

On 18/03/2018 00:28, Paul Cercueil wrote:
> Hi,
> 
> This is the 4th version of my TCU patchset.
> 
> The major change is a greatly improved documentation, both in-code
> and as separate text files, to describe how the hardware works and
> how the devicetree bindings should be used.
> 
> There are also cosmetic changes in the irqchip driver, and the
> clocksource driver will now use as timers all TCU channels not
> requested by the TCU PWM driver.

Hi Paul,

I don't know why but you series appears in reply to [PATCH v3 2/9]. Not
sure if it is my mailer or how you are sending the patches but if it is
the latter can you in the future, when resending a new version, not use
the in-reply-to option. It will be easier to follow the versions.

Thanks.

 -- Daniel



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* another DECLARE transform needed
From: Randy Dunlap @ 2018-03-19  3:34 UTC (permalink / raw)
  To: linux-doc@vger.kernel.org

Hey,

I've been struggling with this for awhile so I might as well just ask for help.

../include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
../include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'

This comes from:

struct phylink_link_state {
	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
	__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising);
	phy_interface_t interface;
	int speed;
	int duplex;
	int pause;
	unsigned int link:1;
	unsigned int an_enabled:1;
	unsigned int an_complete:1;
};

where __ETHTOOL_DECLARE_LINK_MODE_MASK() is basically a DECLARE_BITMAP():
(from include/linux/ethtool.h:)

/* number of link mode bits/ulongs handled internally by kernel */
#define __ETHTOOL_LINK_MODE_MASK_NBITS			\
	(__ETHTOOL_LINK_MODE_LAST + 1)

/* declare a link mode bitmap */
#define __ETHTOOL_DECLARE_LINK_MODE_MASK(name)		\
	DECLARE_BITMAP(name, __ETHTOOL_LINK_MODE_MASK_NBITS)

I have tried multiple string substitutions but they have all failed, so if anyone
can make it work, please do so.

thanks.
-- 
Here's my current patch:
---
From: Randy Dunlap <rdunlap@infradead.org>

Add support for __ETHTOOL_DECLARE_LINK_MODE_MASK() as used in
<linux/phylink.h>.

../include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
../include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'

Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
---
 scripts/kernel-doc |    2 ++
 1 file changed, 2 insertions(+)

--- lnx-416-rc6.orig/scripts/kernel-doc
+++ lnx-416-rc6/scripts/kernel-doc
@@ -1020,6 +1020,8 @@ sub dump_struct($$) {
 	$members =~ s/\s*CRYPTO_MINALIGN_ATTR//gos;
 	# replace DECLARE_BITMAP
 	$members =~ s/DECLARE_BITMAP\s*\(([^,)]+),\s*([^,)]+)\)/unsigned long $1\[BITS_TO_LONGS($2)\]/gos;
+	# replace __ETHTOOL_DECLARE_LINK_MODE_MASK
+	$members =~ s/__ETHTOOL_DECLARE_LINK_MODE_MASK\s*\(([a-zA-Z0-9_]+)\)/unsigned long $1\[BITS_TO_LONGS\(__ETHTOOL_LINK_MODE_MASK_NBITS\)\]/gos;
 	# replace DECLARE_HASHTABLE
 	$members =~ s/DECLARE_HASHTABLE\s*\(([^,)]+),\s*([^,)]+)\)/unsigned long $1\[1 << (($2) - 1)\]/gos;
 	# replace DECLARE_KFIFO


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 5/8] trace_uprobe: Support SDT markers having reference count (semaphore)
From: Ravi Bangoria @ 2018-03-19  4:28 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: mhiramat, peterz, srikar, acme, ananth, akpm, alexander.shishkin,
	alexis.berlemont, corbet, dan.j.williams, gregkh, huawei.libin,
	hughd, jack, jglisse, jolsa, kan.liang, kirill.shutemov, kjlx,
	kstewart, linux-doc, linux-kernel, linux-mm, mhocko, milian.wolff,
	mingo, namhyung, naveen.n.rao, pc, pombredanne, rostedt, tglx,
	tmricht, willy, yao.jin, fengguang.wu, Ravi Bangoria
In-Reply-To: <20180314165943.GA5948@redhat.com>

Hi Oleg,

On 03/14/2018 10:29 PM, Oleg Nesterov wrote:
> On 03/13, Ravi Bangoria wrote:
>> +static bool sdt_valid_vma(struct trace_uprobe *tu, struct vm_area_struct *vma)
>> +{
>> +	unsigned long vaddr = vma_offset_to_vaddr(vma, tu->ref_ctr_offset);
>> +
>> +	return tu->ref_ctr_offset &&
>> +		vma->vm_file &&
>> +		file_inode(vma->vm_file) == tu->inode &&
>> +		vma->vm_flags & VM_WRITE &&
>> +		vma->vm_start <= vaddr &&
>> +		vma->vm_end > vaddr;
>> +}
> Perhaps in this case a simple
>
> 		ref_ctr_offset < vma->vm_end - vma->vm_start
>
> check without vma_offset_to_vaddr() makes more sense, but I won't insist.
>

I still don't get this. This seems a comparison between file offset and size
of the vma. Shouldn't we need to consider pg_off here?

Thanks,
Ravi

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] crypto: doc - clarify hash callbacks state machine
From: Horia Geantă @ 2018-03-19  6:39 UTC (permalink / raw)
  To: Herbert Xu
  Cc: David S. Miller, Jonathan Corbet, linux-crypto@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20180316151642.GA6606@gondor.apana.org.au>

On 3/16/2018 5:16 PM, Herbert Xu wrote:
> On Mon, Mar 05, 2018 at 12:39:45PM +0200, Horia Geantă wrote:
>> Even though it doesn't make too much sense, it is perfectly legal to:
>> - call .init() and then (as many times) .update()
>> - subseqently _not_ call any of .final(), .finup() or .export()
> 
> Actually it makes perfect sense, because there can be an arbitrary
> number of requests for a given tfm.  There is no requirement that
> you must finalise the first request before submitting new ones.
> 
> IOW there can be an arbitrary number of outstanding requests even
> without the user intentionally abandoning any hash request.
> 
The fact that there can be multiple requests in parallel (for a given tfm) is a
different topic.
Each request object has its state in its own state machine, independent from the
other request objects.
I assume this is clear enough.

Why I wanted to underline is that "abandoning" a hash request is allowed (even
though doing this is at least questionable), thus implementations must take
special care not to leak resources in this case.

If you think the commit message should be updated, then probably so should the
documentation update.

Thanks,
Horia
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 6/8] trace_uprobe/sdt: Fix multiple update of same reference counter
From: Ravi Bangoria @ 2018-03-19  9:18 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: mhiramat, peterz, srikar, acme, ananth, akpm, alexander.shishkin,
	alexis.berlemont, corbet, dan.j.williams, gregkh, huawei.libin,
	hughd, jack, jglisse, jolsa, kan.liang, kirill.shutemov, kjlx,
	kstewart, linux-doc, linux-kernel, linux-mm, mhocko, milian.wolff,
	mingo, namhyung, naveen.n.rao, pc, pombredanne, rostedt, tglx,
	tmricht, willy, yao.jin, fengguang.wu, Ravi Bangoria
In-Reply-To: <20180316175030.GA28770@redhat.com>

Hi Oleg,

On 03/16/2018 11:20 PM, Oleg Nesterov wrote:
> On 03/16, Ravi Bangoria wrote:
>> On 03/15/2018 08:19 PM, Oleg Nesterov wrote:
>>> On 03/13, Ravi Bangoria wrote:
>>>> For tiny binaries/libraries, different mmap regions points to the
>>>> same file portion. In such cases, we may increment reference counter
>>>> multiple times.
>>> Yes,
>>>
>>>> But while de-registration, reference counter will get
>>>> decremented only by once
>>> could you explain why this happens? sdt_increment_ref_ctr() and
>>> sdt_decrement_ref_ctr() look symmetrical, _decrement_ should see
>>> the same mappings?
> ...
>
>>     # strace -o out python
>>       mmap(NULL, 2738968, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7fff92460000
>>       mmap(0x7fff926a0000, 327680, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x230000) = 0x7fff926a0000
>>       mprotect(0x7fff926a0000, 65536, PROT_READ) = 0
> Ah, in this case everything is clear, thanks.
>
> I was confused by the changelog, I misinterpreted it as if inc/dec are not
> balanced in case of multiple mappings even if the application doesn't play
> with mmap/mprotect/etc.
>
> And it seems that you are trying to confuse yourself, not only me ;) Just
> suppose that an application does mmap+munmap in a loop and the mapped region
> contains uprobe but not the counter.

this is fine because ...

>
> And this all makes me think that we should do something else. Ideally,
> install_breakpoint() and remove_breakpoint() should inc/dec the counter
> if they do not fail...

The whole point of adding this logic in trace_uprobe is we wanted to
decouple the counter inc/dec logic from uprobe patching. If user is just
doing mmap+munmap region in a loop which contains uprobe, the
instruction will be patched by the core uprobe infrastructure. Whenever
application mmap the region that holds to counter, it will be incremented.

Our initial design was to increment counter in install_breakpoint() but
uprobed instruction gets patched in a very early stage of binary loading
and vma that holds the counter may not be mapped yet.

>
> Btw, why do we need a counter, not a boolean? Who else can modify it?
> Or different uprobes can share the same counter?

Yes, multiple SDT markers can share the counter. Ex, there can be multiple
implementation of same function and thus each individual implementation
may contain marker which share the same counter. From mysql,

  # readelf -n /usr/lib64/mysql/libmysqlclient.so.18.0.0 | grep -A2 Provider
    Provider: mysql
    Name: net__write__start
    Location: 0x000000000003caa0, ..., Semaphore: 0x0000000000333532
  --
    Provider: mysql
    Name: net__write__start
    Location: 0x000000000003cd5c, ..., Semaphore: 0x0000000000333532

Here, both the markers has same name, but different location. Also they
share the counter (semaphore).

Apart from that, counter allows multiple tracers to trace on a single marker,
which is difficult with boolean flag.

Thanks,
Ravi

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] crypto: doc - clarify hash callbacks state machine
From: Herbert Xu @ 2018-03-19  9:24 UTC (permalink / raw)
  To: Horia Geantă
  Cc: David S. Miller, Jonathan Corbet, linux-crypto@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <AM3PR04MB45284A83B66E00785872DE698D40@AM3PR04MB452.eurprd04.prod.outlook.com>

On Mon, Mar 19, 2018 at 06:39:50AM +0000, Horia Geantă wrote:
>
> The fact that there can be multiple requests in parallel (for a given tfm) is a
> different topic.
> Each request object has its state in its own state machine, independent from the
> other request objects.
> I assume this is clear enough.

My point is that all of the state associated with a request needs
to be stored in the request object.  If you're start storing things
in the driver/hardware, then things will get ugly one way or another.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] crypto: doc - clarify hash callbacks state machine
From: Horia Geantă @ 2018-03-19 11:04 UTC (permalink / raw)
  To: Herbert Xu
  Cc: David S. Miller, Jonathan Corbet, linux-crypto@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20180319092458.GB12302@gondor.apana.org.au>

On 3/19/2018 11:25 AM, Herbert Xu wrote:
> On Mon, Mar 19, 2018 at 06:39:50AM +0000, Horia Geantă wrote:
>>
>> The fact that there can be multiple requests in parallel (for a given tfm) is a
>> different topic.
>> Each request object has its state in its own state machine, independent from the
>> other request objects.
>> I assume this is clear enough.
> 
> My point is that all of the state associated with a request needs
> to be stored in the request object.  If you're start storing things
> in the driver/hardware, then things will get ugly one way or another.
> 
Agree, the request state should be stored in the request object; I am not
debating that.

Still there are limitations even when keeping state in the request object.
For e.g. an implementation cannot DMA map a buffer for the entire lifetime of a
request object, because this lifetime is unknown - user can "abandon" the object
after a few .update() calls, or even after .init(). By "abandon" I mean not call
_ever_ any of .final(), .finup() or .export() on the object.

The only solution to avoid leaks in this case is to repeatedly DMA map & unmap
the buffer.
IOW, if one wants to load/save HW state in a buffer after an .update() and to
instruct the crypto engine to do this operation, the following steps are involved:
-gpp: DMA map the buffer, get its IOVA
-gpp: program the crypto engine with IOVA, wait for crypto engine's signal
-crypto engine: load HW state from buffer, perform the partial hash, save HW
state in buffer, signal gpp
-gpp: DMA unmap the buffer

I'd say this is pretty inefficient, yet I don't see an alternative.

Good or bad, the documentation should reflect this limitation - hence this patch.

Thanks,
Horia
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 6/8] trace_uprobe/sdt: Fix multiple update of same reference counter
From: Oleg Nesterov @ 2018-03-19 13:40 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: mhiramat, peterz, srikar, acme, ananth, akpm, alexander.shishkin,
	alexis.berlemont, corbet, dan.j.williams, gregkh, huawei.libin,
	hughd, jack, jglisse, jolsa, kan.liang, kirill.shutemov, kjlx,
	kstewart, linux-doc, linux-kernel, linux-mm, mhocko, milian.wolff,
	mingo, namhyung, naveen.n.rao, pc, pombredanne, rostedt, tglx,
	tmricht, willy, yao.jin, fengguang.wu
In-Reply-To: <4b337afd-fc5e-6110-888b-d4fa36a797ee@linux.vnet.ibm.com>

Hi Ravi,

On 03/19, Ravi Bangoria wrote:
>
> On 03/16/2018 11:20 PM, Oleg Nesterov wrote:
> >
> > And it seems that you are trying to confuse yourself, not only me ;) Just
> > suppose that an application does mmap+munmap in a loop and the mapped region
> > contains uprobe but not the counter.
>
> this is fine because ...

Yes, I guess I tried to say "counter but not uprobe" but possibly I was actually
confused.

> Our initial design was to increment counter in install_breakpoint() but
> uprobed instruction gets patched in a very early stage of binary loading
> and vma that holds the counter may not be mapped yet.

Yes, yes, I understand this is not that simple...

> > Btw, why do we need a counter, not a boolean? Who else can modify it?
> > Or different uprobes can share the same counter?
>
> Yes, multiple SDT markers can share the counter.

OK, thanks.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 5/8] trace_uprobe: Support SDT markers having reference count (semaphore)
From: Oleg Nesterov @ 2018-03-19 13:46 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: mhiramat, peterz, srikar, acme, ananth, akpm, alexander.shishkin,
	alexis.berlemont, corbet, dan.j.williams, gregkh, huawei.libin,
	hughd, jack, jglisse, jolsa, kan.liang, kirill.shutemov, kjlx,
	kstewart, linux-doc, linux-kernel, linux-mm, mhocko, milian.wolff,
	mingo, namhyung, naveen.n.rao, pc, pombredanne, rostedt, tglx,
	tmricht, willy, yao.jin, fengguang.wu
In-Reply-To: <9cb068f7-0996-6e24-a95b-771006559318@linux.vnet.ibm.com>

On 03/19, Ravi Bangoria wrote:
>
> Hi Oleg,
> 
> On 03/14/2018 10:29 PM, Oleg Nesterov wrote:
> > On 03/13, Ravi Bangoria wrote:
> >> +static bool sdt_valid_vma(struct trace_uprobe *tu, struct vm_area_struct *vma)
> >> +{
> >> +	unsigned long vaddr = vma_offset_to_vaddr(vma, tu->ref_ctr_offset);
> >> +
> >> +	return tu->ref_ctr_offset &&
> >> +		vma->vm_file &&
> >> +		file_inode(vma->vm_file) == tu->inode &&
> >> +		vma->vm_flags & VM_WRITE &&
> >> +		vma->vm_start <= vaddr &&
> >> +		vma->vm_end > vaddr;
> >> +}
> > Perhaps in this case a simple
> >
> > 		ref_ctr_offset < vma->vm_end - vma->vm_start
> >
> > check without vma_offset_to_vaddr() makes more sense, but I won't insist.
> >
> 
> I still don't get this. This seems a comparison between file offset and size
> of the vma. Shouldn't we need to consider pg_off here?

Indeed, I am stupid ;)

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v12 00/11] Application Data Integrity feature introduced by SPARC M7
From: Khalid Aziz @ 2018-03-19 15:19 UTC (permalink / raw)
  To: David Miller
  Cc: dave.hansen, aarcange, akpm, allen.pais, aneesh.kumar,
	anthony.yznaga, arnd, benh, bob.picco, bsingharora, corbet,
	dan.j.williams, dave.jiang, david.j.aldridge, dwindsor, ebiederm,
	elena.reshetova, gregkh, hannes, henry.willard, hpa, hughd,
	imbrenda, jack, jag.raman, jane.chu, jglisse, jroedel, khalid,
	khandual, kirill.shutemov, kstewart, ktkhai, liam.merwick,
	linux-arch, linux-doc, linux-kernel, linux-mm, linuxppc-dev,
	linuxram, linux, me, mgorman, mgorman, mhocko, mike.kravetz,
	minchan, mingo
In-Reply-To: <20180318.110857.1660518649370410007.davem@davemloft.net>

On 03/18/2018 09:08 AM, David Miller wrote:
> In uapi/asm/auxvec.h you conditionalize the ADI aux vectors on
> CONFIG_SPARC64.
> 
> That's not correct, you should never control user facing definitions
> based upon kernel configuration.
> 
> Also, both 32-bit and 64-bit applications running on ADI capable
> machines want to compile against and use this informaiton.
> 
> So please remove these CPP checks.
> 

Hi Dave,

That makes sense. I will send a patch to remove these.

Thanks,
Khalid
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Tejun Heo @ 2018-03-19 15:34 UTC (permalink / raw)
  To: Mike Galbraith
  Cc: Waiman Long, Peter Zijlstra, Li Zefan, Johannes Weiner,
	Ingo Molnar, cgroups, linux-kernel, linux-doc, kernel-team, pjt,
	luto, torvalds, Roman Gushchin
In-Reply-To: <1521082141.7100.1.camel@gmx.de>

Hello, Mike.

On Thu, Mar 15, 2018 at 03:49:01AM +0100, Mike Galbraith wrote:
> Under the hood v2 details are entirely up to you.  My input ends at
> please don't leave dynamic partitioning standing at the dock when v2
> sails.

So, this isn't about implementation details but about what the
interface achieves - ie, what's the actual function?  The only thing I
can see is blocking the entity which is configuring the hierarchy from
making certain configs.  While that might be useful in some specific
use cases, it seems to miss the bar for becoming its own kernel
feature.  After all, nothing prevents the same entity from clearing
the exlusive bit and making the said changes.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5 2/9] proc/sysctl: Provide additional ctl_table.flags checks
From: Waiman Long @ 2018-03-19 15:35 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: Kees Cook, linux-kernel, linux-fsdevel, linux-doc,
	Jonathan Corbet, Andrew Morton, Al Viro, Matthew Wilcox,
	Eric W. Biederman
In-Reply-To: <20180317005458.GA4449@wotan.suse.de>

On 03/16/2018 08:54 PM, Luis R. Rodriguez wrote:
> On Fri, Mar 16, 2018 at 02:13:43PM -0400, Waiman Long wrote:
>> Checking code is added to provide the following additional
>> ctl_table.flags checks:
>>
>>  1) No unknown flag is allowed.
>>  2) Minimum of a range cannot be larger than the maximum value.
>>  3) The signed and unsigned flags are mutually exclusive.
>>  4) The proc_handler should be consistent with the signed or unsigned
>>     flags.
>>
>> Two new flags are added to indicate if the min/max values are signed
>> or unsigned - CTL_FLAGS_SIGNED_RANGE & CTL_FLAGS_UNSIGNED_RANGE.
>> These 2 flags can be optionally enabled for range checking purpose.
>> But either one of them must be set with CTL_FLAGS_CLAMP_RANGE.
>>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index e446e1f..088f032 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -134,14 +134,26 @@ struct ctl_table
>>   *	the input value. No lower bound or upper bound checking will be
>>   *	done if the corresponding minimum or maximum value isn't provided.
>>   *
>> + * @CTL_FLAGS_SIGNED_RANGE: Set to indicate that the extra1 and extra2
>> + *	fields are pointers to minimum and maximum signed values of
>> + *	an allowable range.
>> + *
>> + * @CTL_FLAGS_UNSIGNED_RANGE: Set to indicate that the extra1 and extra2
>> + *	fields are pointers to minimum and maximum unsigned values of
>> + *	an allowable range.
>> + *
>>   * At most 16 different flags are allowed.
>>   */
>>  enum ctl_table_flags {
>>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
>> -	__CTL_FLAGS_MAX			= BIT(1),
>> +	CTL_FLAGS_SIGNED_RANGE		= BIT(1),
>> +	CTL_FLAGS_UNSIGNED_RANGE	= BIT(2),
>> +	__CTL_FLAGS_MAX			= BIT(3),
>>  };
> You are adding new flags which the user can set, and yet these are used
> internally.
>
> It would be best if internal flags are just that, not flags that a user can set.
>
> This patch should be folded with the first one.
>
> I'm starting to loose hope on these patch sets.
>
>   Luis

In order to do the correct min > max check, I need to know if the
quantity is signed or not. Just looking at the proc_handler alone is not
a reliable indicator if it is signed or unsigned.

Yes, I can put the signed bit into the previous patch.

-Longman


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5 1/9] sysctl: Add flags to support min/max range clamping
From: Waiman Long @ 2018-03-19 15:39 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: Kees Cook, linux-kernel, linux-fsdevel, linux-doc,
	Jonathan Corbet, Andrew Morton, Al Viro, Matthew Wilcox,
	Eric W. Biederman
In-Reply-To: <20180317011021.GB4449@wotan.suse.de>

On 03/16/2018 09:10 PM, Luis R. Rodriguez wrote:
> On Fri, Mar 16, 2018 at 02:13:42PM -0400, Waiman Long wrote:
>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table
>> entry, any update from the userspace will be clamped to the given
>> range without error if either the proc_dointvec_minmax() or the
>> proc_douintvec_minmax() handlers is used.
> I don't get it.  Why define a generic range flag when we can be mores specific and
> you do that in your next patch. What's the point of this flag then?
>
>   Luis

I was thinking about using the signed/unsigned bits as just annotations
for ranges for future extension. For the purpose of this patchset alone,
I can merge the three bits into just two.

Cheers,
Longman


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5 2/2] cpuset: Add cpuset.flags control knob to v2
From: Tejun Heo @ 2018-03-19 16:26 UTC (permalink / raw)
  To: Waiman Long
  Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
	linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
	Roman Gushchin
In-Reply-To: <1521148842-15486-3-git-send-email-longman@redhat.com>

Hello, Waiman.

On Thu, Mar 15, 2018 at 05:20:42PM -0400, Waiman Long wrote:
> +	The currently supported flag is:
> +
> +	  sched_load_balance
> +		When it is not set, there will be no load balancing
> +		among CPUs on this cpuset.  Tasks will stay in the
> +		CPUs they are running on and will not be moved to
> +		other CPUs.
> +
> +		When it is set, tasks within this cpuset will be
> +		load-balanced by the kernel scheduler.  Tasks will be
> +		moved from CPUs with high load to other CPUs within
> +		the same cpuset with less load periodically.

Hmm... looks like this is something which can be decided by the cgroup
itself and should be made delegatable.  Given that different flags
might need different delegation settings and the precedence of
memory.oom_group, I think it'd be better to make the flags separate
bool files - ie. cpuset.sched_load_balance which contains 0/1 and
marked delegatable.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5 2/2] cpuset: Add cpuset.flags control knob to v2
From: Waiman Long @ 2018-03-19 16:33 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
	linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
	Roman Gushchin
In-Reply-To: <20180319162654.GR2943022@devbig577.frc2.facebook.com>

On 03/19/2018 12:26 PM, Tejun Heo wrote:
> Hello, Waiman.
>
> On Thu, Mar 15, 2018 at 05:20:42PM -0400, Waiman Long wrote:
>> +	The currently supported flag is:
>> +
>> +	  sched_load_balance
>> +		When it is not set, there will be no load balancing
>> +		among CPUs on this cpuset.  Tasks will stay in the
>> +		CPUs they are running on and will not be moved to
>> +		other CPUs.
>> +
>> +		When it is set, tasks within this cpuset will be
>> +		load-balanced by the kernel scheduler.  Tasks will be
>> +		moved from CPUs with high load to other CPUs within
>> +		the same cpuset with less load periodically.
> Hmm... looks like this is something which can be decided by the cgroup
> itself and should be made delegatable.  Given that different flags
> might need different delegation settings and the precedence of
> memory.oom_group, I think it'd be better to make the flags separate
> bool files - ie. cpuset.sched_load_balance which contains 0/1 and
> marked delegatable.
>
> Thanks.
>
Sure. Will do that.

-Longman

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v7] usb: core: Add "quirks" parameter for usbcore
From: Kai-Heng Feng @ 2018-03-19 16:26 UTC (permalink / raw)
  To: gregkh; +Cc: oneukum, willy, linux-doc, linux-kernel, linux-usb, Kai-Heng Feng

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v7: Update documentation to specify the quirks parameter can XOR the
    built-in quirks.

v6: Add missed header file <linux/moduleparam.h>.

v5: Use module_param_cb() to get notified when parameter changes.
    Replace linked list with array.

v4: Use kmalloc() to reduce memory usage.
    Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
    Use XOR for dynamic quirks.
    Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  56 ++++++++
 drivers/usb/core/quirks.c                       | 178 +++++++++++++++++++++++-
 drivers/usb/core/usb.c                          |   1 +
 drivers/usb/core/usb.h                          |   1 +
 4 files changed, 231 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..e00cdd313dc2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,62 @@
 
 	usbcore.nousb	[USB] Disable the USB subsystem
 
+	usbcore.quirks=
+			[USB] A list of quirk entries to augment the built-in
+			usb core quirk list. List entries are separated by
+			commas. Each entry has the form
+			VendorID:ProductID:Flags. The IDs are 4-digit hex
+			numbers and Flags is a set of letters. Each letter
+			will change the built-in quirk; setting it if it is
+			clear and clearing it if it is set. The letters have
+			the following meanings:
+				a = USB_QUIRK_STRING_FETCH_255 (string
+					descriptors must not be fetched using
+					a 255-byte read);
+				b = USB_QUIRK_RESET_RESUME (device can't resume
+					correctly so reset it instead);
+				c = USB_QUIRK_NO_SET_INTF (device can't handle
+					Set-Interface requests);
+				d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+					handle its Configuration or Interface
+					strings);
+				e = USB_QUIRK_RESET (device can't be reset
+					(e.g morph devices), don't use reset);
+				f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+					more interface descriptions than the
+					bNumInterfaces count, and can't handle
+					talking to these interfaces);
+				g = USB_QUIRK_DELAY_INIT (device needs a pause
+					during initialization, after we read
+					the device descriptor);
+				h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+					high speed and super speed interrupt
+					endpoints, the USB 2.0 and USB 3.0 spec
+					require the interval in microframes (1
+					microframe = 125 microseconds) to be
+					calculated as interval = 2 ^
+					(bInterval-1).
+					Devices with this quirk report their
+					bInterval as the result of this
+					calculation instead of the exponent
+					variable used in the calculation);
+				i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+					handle device_qualifier descriptor
+					requests);
+				j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+					generates spurious wakeup, ignore
+					remote wakeup capability);
+				k = USB_QUIRK_NO_LPM (device can't handle Link
+					Power Management);
+				l = USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL
+					(Device reports its bInterval as linear
+					frames instead of the USB 2.0
+					calculation);
+				m = USB_QUIRK_DISCONNECT_SUSPEND (Device needs
+					to be disconnected before suspend to
+					prevent spurious wakeup)
+			Example: quirks=0781:5580:bk,0a5c:5834:gij
+
 	usbhid.mousepoll=
 			[USBHID] The interval which mice are to be polled at.
 
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 54b019e267c5..6fb8d5433268 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -6,11 +6,149 @@
  * Copyright (c) 2007 Greg Kroah-Hartman <gregkh@suse.de>
  */
 
+#include <linux/moduleparam.h>
 #include <linux/usb.h>
 #include <linux/usb/quirks.h>
 #include <linux/usb/hcd.h>
 #include "usb.h"
 
+struct quirk_entry {
+	u16 vid;
+	u16 pid;
+	u32 flags;
+};
+
+static DEFINE_MUTEX(quirk_mutex);
+
+static struct quirk_entry *quirk_list;
+static unsigned int quirk_count;
+
+static char quirks_param[128];
+
+static int quirks_param_set(const char *val, const struct kernel_param *kp)
+{
+	char *p, *field;
+	u16 vid, pid;
+	u32 flags;
+	size_t i;
+
+	mutex_lock(&quirk_mutex);
+
+	if (!val || !*val) {
+		quirk_count = 0;
+		kfree(quirk_list);
+		quirk_list = NULL;
+		goto unlock;
+	}
+
+	for (quirk_count = 1, i = 0; val[i]; i++)
+		if (val[i] == ',')
+			quirk_count++;
+
+	if (quirk_list) {
+		kfree(quirk_list);
+		quirk_list = NULL;
+	}
+
+	quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry),
+			     GFP_KERNEL);
+	if (!quirk_list) {
+		mutex_unlock(&quirk_mutex);
+		return -ENOMEM;
+	}
+
+	for (i = 0, p = (char *)val; p && *p;) {
+		/* Each entry consists of VID:PID:flags */
+		field = strsep(&p, ":");
+		if (!field)
+			break;
+
+		if (kstrtou16(field, 16, &vid))
+			break;
+
+		field = strsep(&p, ":");
+		if (!field)
+			break;
+
+		if (kstrtou16(field, 16, &pid))
+			break;
+
+		field = strsep(&p, ",");
+		if (!field || !*field)
+			break;
+
+		/* Collect the flags */
+		for (flags = 0; *field; field++) {
+			switch (*field) {
+			case 'a':
+				flags |= USB_QUIRK_STRING_FETCH_255;
+				break;
+			case 'b':
+				flags |= USB_QUIRK_RESET_RESUME;
+				break;
+			case 'c':
+				flags |= USB_QUIRK_NO_SET_INTF;
+				break;
+			case 'd':
+				flags |= USB_QUIRK_CONFIG_INTF_STRINGS;
+				break;
+			case 'e':
+				flags |= USB_QUIRK_RESET;
+				break;
+			case 'f':
+				flags |= USB_QUIRK_HONOR_BNUMINTERFACES;
+				break;
+			case 'g':
+				flags |= USB_QUIRK_DELAY_INIT;
+				break;
+			case 'h':
+				flags |= USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL;
+				break;
+			case 'i':
+				flags |= USB_QUIRK_DEVICE_QUALIFIER;
+				break;
+			case 'j':
+				flags |= USB_QUIRK_IGNORE_REMOTE_WAKEUP;
+				break;
+			case 'k':
+				flags |= USB_QUIRK_NO_LPM;
+				break;
+			case 'l':
+				flags |= USB_QUIRK_LINEAR_FRAME_INTR_BINTERVAL;
+				break;
+			case 'm':
+				flags |= USB_QUIRK_DISCONNECT_SUSPEND;
+				break;
+			/* Ignore unrecognized flag characters */
+			}
+		}
+
+		quirk_list[i++] = (struct quirk_entry)
+			{ .vid = vid, .pid = pid, .flags = flags };
+	}
+
+	if (i < quirk_count)
+		quirk_count = i;
+
+unlock:
+	mutex_unlock(&quirk_mutex);
+
+	return param_set_copystring(val, kp);
+}
+
+static const struct kernel_param_ops quirks_param_ops = {
+	.set = quirks_param_set,
+	.get = param_get_string,
+};
+
+static struct kparam_string quirks_param_string = {
+	.maxlen = sizeof(quirks_param),
+	.string = quirks_param,
+};
+
+module_param_cb(quirks, &quirks_param_ops, &quirks_param_string, 0644);
+MODULE_PARM_DESC(quirks, "Add/modify USB quirks by specifying quirks=vendorID:productID:quirks");
+
 /* Lists of quirky USB devices, split in device quirks and interface quirks.
  * Device quirks are applied at the very beginning of the enumeration process,
  * right after reading the device descriptor. They can thus only match on device
@@ -321,8 +459,8 @@ static int usb_amd_resume_quirk(struct usb_device *udev)
 	return 0;
 }
 
-static u32 __usb_detect_quirks(struct usb_device *udev,
-			       const struct usb_device_id *id)
+static u32 usb_detect_static_quirks(struct usb_device *udev,
+				    const struct usb_device_id *id)
 {
 	u32 quirks = 0;
 
@@ -340,21 +478,43 @@ static u32 __usb_detect_quirks(struct usb_device *udev,
 	return quirks;
 }
 
+static u32 usb_detect_dynamic_quirks(struct usb_device *udev)
+{
+	u16 vid = le16_to_cpu(udev->descriptor.idVendor);
+	u16 pid = le16_to_cpu(udev->descriptor.idProduct);
+	int i, flags = 0;
+
+	mutex_lock(&quirk_mutex);
+
+	for (i = 0; i < quirk_count; i++) {
+		if (vid == quirk_list[i].vid && pid == quirk_list[i].pid) {
+			flags = quirk_list[i].flags;
+			break;
+		}
+	}
+
+	mutex_unlock(&quirk_mutex);
+
+	return flags;
+}
+
 /*
  * Detect any quirks the device has, and do any housekeeping for it if needed.
  */
 void usb_detect_quirks(struct usb_device *udev)
 {
-	udev->quirks = __usb_detect_quirks(udev, usb_quirk_list);
+	udev->quirks = usb_detect_static_quirks(udev, usb_quirk_list);
 
 	/*
 	 * Pixart-based mice would trigger remote wakeup issue on AMD
 	 * Yangtze chipset, so set them as RESET_RESUME flag.
 	 */
 	if (usb_amd_resume_quirk(udev))
-		udev->quirks |= __usb_detect_quirks(udev,
+		udev->quirks |= usb_detect_static_quirks(udev,
 				usb_amd_resume_quirk_list);
 
+	udev->quirks ^= usb_detect_dynamic_quirks(udev);
+
 	if (udev->quirks)
 		dev_dbg(&udev->dev, "USB quirks for this device: %x\n",
 			udev->quirks);
@@ -373,7 +533,7 @@ void usb_detect_interface_quirks(struct usb_device *udev)
 {
 	u32 quirks;
 
-	quirks = __usb_detect_quirks(udev, usb_interface_quirk_list);
+	quirks = usb_detect_static_quirks(udev, usb_interface_quirk_list);
 	if (quirks == 0)
 		return;
 
@@ -381,3 +541,11 @@ void usb_detect_interface_quirks(struct usb_device *udev)
 		quirks);
 	udev->quirks |= quirks;
 }
+
+void usb_release_quirk_list(void)
+{
+	mutex_lock(&quirk_mutex);
+	kfree(quirk_list);
+	quirk_list = NULL;
+	mutex_unlock(&quirk_mutex);
+}
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 2f5fbc56a9dd..0adb6345ff2e 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -1259,6 +1259,7 @@ static void __exit usb_exit(void)
 	if (usb_disabled())
 		return;
 
+	usb_release_quirk_list();
 	usb_deregister_device_driver(&usb_generic_driver);
 	usb_major_cleanup();
 	usb_deregister(&usbfs_driver);
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index 149cc7480971..546a2219454b 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -36,6 +36,7 @@ extern void usb_deauthorize_interface(struct usb_interface *);
 extern void usb_authorize_interface(struct usb_interface *);
 extern void usb_detect_quirks(struct usb_device *udev);
 extern void usb_detect_interface_quirks(struct usb_device *udev);
+extern void usb_release_quirk_list(void);
 extern int usb_remove_device(struct usb_device *udev);
 
 extern int usb_get_device_descriptor(struct usb_device *dev,
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy
From: Mike Galbraith @ 2018-03-19 20:49 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Waiman Long, Peter Zijlstra, Li Zefan, Johannes Weiner,
	Ingo Molnar, cgroups, linux-kernel, linux-doc, kernel-team, pjt,
	luto, torvalds, Roman Gushchin
In-Reply-To: <20180319153413.GO2943022@devbig577.frc2.facebook.com>

On Mon, 2018-03-19 at 08:34 -0700, Tejun Heo wrote:
> Hello, Mike.
> 
> On Thu, Mar 15, 2018 at 03:49:01AM +0100, Mike Galbraith wrote:
> > Under the hood v2 details are entirely up to you.  My input ends at
> > please don't leave dynamic partitioning standing at the dock when v2
> > sails.
> 
> So, this isn't about implementation details but about what the
> interface achieves - ie, what's the actual function?  The only thing I
> can see is blocking the entity which is configuring the hierarchy from
> making certain configs.  While that might be useful in some specific
> use cases, it seems to miss the bar for becoming its own kernel
> feature.  After all, nothing prevents the same entity from clearing
> the exlusive bit and making the said changes.

Yes, privileged contexts can maliciously or stupidly step all over one
other no matter what you do (finite resource), but oxymoron creation
(CPUs simultaneously balanced and isolated) should be handled.  If one
context can allocate a set overlapping a set another context intends to
or already has detached from scheduler domains, both are screwed.

	-Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 3/6] i2c: i2c-qcom-geni: Add bus driver for the Qualcomm GENI I2C controller
From: Doug Anderson @ 2018-03-19 21:08 UTC (permalink / raw)
  To: Karthikeyan Ramasubramanian
  Cc: Jonathan Corbet, Andy Gross, David Brown, Rob Herring,
	Mark Rutland, Wolfram Sang, Greg Kroah-Hartman, linux-doc,
	linux-arm-msm, devicetree, linux-i2c, linux-serial, Jiri Slaby,
	evgreen, acourbot, swboyd, Sagar Dharia, Girish Mahadevan
In-Reply-To: <1521071931-9294-4-git-send-email-kramasub@codeaurora.org>

Hi,

On Wed, Mar 14, 2018 at 4:58 PM, Karthikeyan Ramasubramanian
<kramasub@codeaurora.org> wrote:
> This bus driver supports the GENI based i2c hardware controller in the
> Qualcomm SOCs. The Qualcomm Generic Interface (GENI) is a programmable
> module supporting a wide range of serial interfaces including I2C. The
> driver supports FIFO mode and DMA mode of transfer and switches modes
> dynamically depending on the size of the transfer.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> ---
>  drivers/i2c/busses/Kconfig         |  13 +
>  drivers/i2c/busses/Makefile        |   1 +
>  drivers/i2c/busses/i2c-qcom-geni.c | 648 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 662 insertions(+)

Since I reviewed v3 of the i2c patch, can you make sure that you CC me
on future patches?  Thanks!  :)

Overall this looks pretty nice to me now.  A few minor comments...


> +/*
> + * Hardware uses the underlying formula to calculate time periods of
> + * SCL clock cycle.
> + *
> + * time of high period of SCL: t_high = (t_high_cnt * clk_div) / source_clock
> + * time of low period of SCL: t_low = (t_low_cnt * clk_div) / source_clock
> + * time of full period of SCL: t_cycle = (t_cycle_cnt * clk_div) / source_clock
> + * clk_freq_out = t / t_cycle
> + * source_clock = 19.2 MHz
> + */
> +static const struct geni_i2c_clk_fld geni_i2c_clk_map[] = {
> +       {KHZ(100), 7, 10, 11, 26},
> +       {KHZ(400), 2,  5, 12, 24},
> +       {KHZ(1000), 1, 3,  9, 18},
> +};

Thanks for the docs.  ...though if these docs are indeed correct and
there's no extra magic fudge factor I'm still a bit baffled why the
frequency isn't out of spec for 100 kHz and 1 MHz.  I know you said
hardware validated it, but are you really certain they validated 100
kHz and 1 MHz?

>>> 19200. / (1 * 18)
1066.6666666666667

>>> 19200. / (2 * 24)
400.0

>>> 19200. / (7 * 26)
105.49450549450549

Specifically: either the docs you wrote above are wrong (and there's a
magic fudge factor that you didn't document) or your hardware guys are
wrong.  See the I2C spec at
<https://www.nxp.com/docs/en/user-guide/UM10204.pdf>.  Table 10.
("Characteristics of the SDA and SCL bus lines for Standard, Fast, and
Fast-mode Plus I2C-bus devices") says fSCL Max is 100 kHz, 400 kHz, or
1000 kHz.


I could believe that 99.9% of all devices that support 100 kHz also
support 105.5 kHz and that 99.9% of all devices that support 1000 kHz
also support 1066.7 kHz.  However, it's still not in spec.  If you
want to enable a turbo boost mode that runs 5% faster (really?) then I
suppose you could add that as an optional feature, but this shouldn't
be the default.


> +static void geni_i2c_abort_xfer(struct geni_i2c_dev *gi2c)
> +{
> +       u32 val;
> +       unsigned long time_left = ABORT_TIMEOUT;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gi2c->lock, flags);
> +       geni_i2c_err(gi2c, GENI_TIMEOUT);
> +       gi2c->cur = NULL;
> +       geni_se_abort_m_cmd(&gi2c->se);
> +       spin_unlock_irqrestore(&gi2c->lock, flags);
> +       do {
> +               time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> +               val = readl_relaxed(gi2c->se.base + SE_GENI_M_IRQ_STATUS);
> +       } while (!(val & M_CMD_ABORT_EN) && time_left);
> +
> +       if (!(val & M_CMD_ABORT_EN) && !time_left)

Why do you need to check !time_left?  Just "if (!(val & M_CMD_ABORT_EN))".


> +               dev_err(gi2c->se.dev, "Timeout abort_m_cmd\n");
> +}
> +
> +static void geni_i2c_rx_fsm_rst(struct geni_i2c_dev *gi2c)
> +{
> +       u32 val;
> +       unsigned long time_left = RST_TIMEOUT;
> +
> +       writel_relaxed(1, gi2c->se.base + SE_DMA_RX_FSM_RST);
> +       do {
> +               time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> +               val = readl_relaxed(gi2c->se.base + SE_DMA_RX_IRQ_STAT);
> +       } while (!(val & RX_RESET_DONE) && time_left);
> +
> +       if (!(val & RX_RESET_DONE) && !time_left)

Similar.  Don't need "&& !time_left"


> +               dev_err(gi2c->se.dev, "Timeout resetting RX_FSM\n");
> +}
> +
> +static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
> +{
> +       u32 val;
> +       unsigned long time_left = RST_TIMEOUT;
> +
> +       writel_relaxed(1, gi2c->se.base + SE_DMA_TX_FSM_RST);
> +       do {
> +               time_left = wait_for_completion_timeout(&gi2c->done, time_left);
> +               val = readl_relaxed(gi2c->se.base + SE_DMA_TX_IRQ_STAT);
> +       } while (!(val & TX_RESET_DONE) && time_left);
> +
> +       if (!(val & TX_RESET_DONE) && !time_left)

Similar.  Don't need "&& !time_left"


> +static int geni_i2c_probe(struct platform_device *pdev)
> +{
> +       struct geni_i2c_dev *gi2c;
> +       struct resource *res;
> +       u32 proto, tx_depth;
> +       int ret;
> +
> +       gi2c = devm_kzalloc(&pdev->dev, sizeof(*gi2c), GFP_KERNEL);
> +       if (!gi2c)
> +               return -ENOMEM;
> +
> +       gi2c->se.dev = &pdev->dev;
> +       gi2c->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       gi2c->se.base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(gi2c->se.base))
> +               return PTR_ERR(gi2c->se.base);
> +
> +       gi2c->se.clk = devm_clk_get(&pdev->dev, "se");
> +       if (IS_ERR(gi2c->se.clk)) {
> +               ret = PTR_ERR(gi2c->se.clk);
> +               dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> +               return ret;
> +       }
> +
> +       ret = device_property_read_u32(&pdev->dev, "clock-frequency",
> +                                                       &gi2c->clk_freq_out);
> +       if (ret) {
> +               /* All GENI I2C slaves support 400kHz. So default to 400kHz. */

Can you explain this comment?  Are you saying that GENI only supports
talking to I2C devices (devices are also known as "slaves" and GENI
should be the I2C master) that talk 400 kHz I2C or better?  Why do you
even have 100 kHz in your table above then?  I don't think this is
what you meant...

Perhaps you meant to say "All GENI I2C masters support at least 400
kHz, so default ot 400 kHz"

...however, even if you changed the comment as I suggested I'm still
not a fan.  As I said in my review of the prevous version:

> I feel like it should default to 100KHz.  i2c_parse_fw_timings()
> defaults to this and to me the wording "New drivers almost always
> should use the defaults" makes me feel this should be the defaults.

I would also say that even if all GENI I2C masters support at least
400 kHz that doesn't mean that all I2C devices on the bus support 400
kHz.  You could certainly imagine someone sticking something on this
bus that was super low cost and supported only 100 kHz I2C.


-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 2/8] dt-bindings: ingenic: Add DT bindings for TCU clocks
From: Stephen Boyd @ 2018-03-19 21:27 UTC (permalink / raw)
  To: Daniel Lezcano, Jason Cooper, Jonathan Corbet, Lee Jones,
	Marc Zyngier, Mark Rutland, Paul Cercueil, Ralf Baechle,
	Rob Herring, Thomas Gleixner
  Cc: James Hogan, Maarten ter Huurne, linux-clk, devicetree,
	linux-kernel, linux-mips, linux-doc, Paul Cercueil
In-Reply-To: <20180317232901.14129-3-paul@crapouillou.net>

Quoting Paul Cercueil (2018-03-17 16:28:55)
> This header provides clock numbers for the ingenic,tcu
> DT binding.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v5 1/2] cpuset: Enable cpuset controller in default hierarchy
From: Tejun Heo @ 2018-03-19 15:59 UTC (permalink / raw)
  To: Waiman Long
  Cc: Li Zefan, Johannes Weiner, Peter Zijlstra, Ingo Molnar, cgroups,
	linux-kernel, linux-doc, kernel-team, pjt, luto, efault, torvalds,
	Roman Gushchin
In-Reply-To: <1521148842-15486-2-git-send-email-longman@redhat.com>

Hello, Waiman.

This looks great.  A couple nitpicks below.

> +     5-3. Cpuset
> +       5.3-1. Cpuset Interface Files

Can we put cpuset below pid?  It feels weird to break up cpu, memory
and io as they represent the three major resources and are in a
similar fashion.

> +  cpuset.effective_cpus
> +	A read-only multiple values file which exists on non-root
> +	cgroups.
> +
> +	It lists the onlined CPUs that are actually allowed to be
> +	used by tasks within the current cgroup. It is a subset of
> +	"cpuset.cpus".  Its value will be affected by CPU hotplug
> +	events.

Can we do cpuset.cpus.availble which lists the cpus available to the
cgroup instead of the eventual computed mask for the cgroup?  That'd
be more useful as it doesn't lose the information by and'ing what's
available with the cgroup's mask and it's trivial to determine the
effective from the two masks.

> +  cpuset.effective_mems
> +	A read-only multiple values file which exists on non-root
> +	cgroups.
> +
> +	It lists the onlined memory nodes that are actually allowed
> +	to be used by tasks within the current cgroup.  It is a subset
> +	of "cpuset.mems".  Its value will be affected by memory nodes
> +	hotplug events.

Ditto.

> +static struct cftype dfl_files[] = {
> +	{
> +		.name = "cpus",
> +		.seq_show = cpuset_common_seq_show,
> +		.write = cpuset_write_resmask,
> +		.max_write_len = (100U + 6 * NR_CPUS),
> +		.private = FILE_CPULIST,
> +	},

Is it missing CFTYPE_NOT_ON_ROOT?  Other files too.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox