Re: [PATCH] nvdimm/pmem: Set dax flag for all 'PFN_MAP' cases
On Wed, Jul 31, 2024 at 08:25:30PM +0800, Zhihao Cheng wrote:
> The dax is only supportted on pfn type pmem devices since commit
> f467fee48da4 ("block: move the dax flag to queue_limits"), fix it
> by adding dax flag setting for the missed case.
s/supportted/supported
How about adding failure messages like this:
Trying to mount DAX filesystem fails with this error:
mount: : wrong fs type, bad option, bad superblock on /dev/pmem7, missing
codepage or helper program, or other error.
dmesg(1) may have more information after failed mount system call.
dmesg: EXT4-fs (pmem7): DAX unsupported by block device.
Tested-by: Alison Schofield
>
> Fixes: f467fee48da4 ("block: move the dax flag to queue_limits")
> Signed-off-by: Zhihao Cheng
> ---
> drivers/nvdimm/pmem.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 1ae8b2351654..210fb77f51ba 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -498,7 +498,7 @@ static int pmem_attach_disk(struct device *dev,
> }
> if (fua)
> lim.features |= BLK_FEAT_FUA;
> - if (is_nd_pfn(dev))
> + if (is_nd_pfn(dev) || pmem_should_map_pages(dev))
> lim.features |= BLK_FEAT_DAX;
>
> if (!devm_request_mem_region(dev, res->start, resource_size(res),
> --
> 2.39.2
>
Re: [PATCH][next] dax: remove redundant assignment to variable rc
On Mon, Apr 15, 2024 at 11:19:28AM +0100, Colin Ian King wrote: > The variable rc is being assigned an value and then is being re-assigned > a new value in the next statement. The assignment is redundant and can > be removed. > > Cleans up clang scan build warning: > drivers/dax/bus.c:1207:2: warning: Value stored to 'rc' is never > read [deadcode.DeadStores] > > Signed-off-by: Colin Ian King Reviewed-by: Alison Schofield > --- > drivers/dax/bus.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index 797e1ebff299..f758afbf8f09 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -1204,7 +1204,6 @@ static ssize_t mapping_store(struct device *dev, struct > device_attribute *attr, > if (rc) > return rc; > > - rc = -ENXIO; > rc = down_write_killable(&dax_region_rwsem); > if (rc) > return rc; > -- > 2.39.2 > >
Re: [PATCH] tracing: Add __string_src() helper to help compilers not to get confused
On Thu, Mar 14, 2024 at 11:27:54PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)"
>
> The __string() helper macro of the TRACE_EVENT() macro is used to
> determine how much of the ring buffer needs to be allocated to fit the
> given source string. Some trace events have a string that is dependent on
> another variable that could be NULL, and in those cases the string is
> passed in to be NULL.
>
> The __string() macro can handle being passed in a NULL pointer for which
> it will turn it into "(null)". It does that with:
>
> strlen((src) ? (const char *)(src) : "(null)") + 1
>
> But if src itself has the same conditional type it can confuse the
> compiler. That is:
>
> __string(r ? dev(r)->name : NULL)
>
> Would turn into:
>
> strlen((r ? dev(r)->name : NULL) ? (r ? dev(r)->name : NULL) : "(null)" + 1
>
> For which the compiler thinks that NULL is being passed to strlen() and
> gives this kind of warning:
>
> ./include/trace/stages/stage5_get_offsets.h:50:21: warning: argument 1 null
> where non-null expected [-Wnonnull]
>50 | strlen((src) ? (const char *)(src) : "(null)") +
> 1)
>
> Instead, create a static inline function that takes the src string and
> will return the string if it is not NULL and will return "(null)" if it
> is. This will then make the strlen() line:
>
> strlen(__string_src(src)) + 1
>
> Where the compiler can see that strlen() will not end up with NULL and
> does not warn about it.
>
> Note that this depends on commit 51270d573a8d ("tracing/net_sched: Fix
> tracepoints that save qdisc_dev() as a string") being applied, as passing
> the qdisc_dev() into __string_src() will give an error.
>
> Link: https://lore.kernel.org/all/ZfNmfCmgCs4Nc+EH@aschofie-mobl2/
Reviewed-by: Alison Schofield
>
> Reported-by: Alison Schofield
> Signed-off-by: Steven Rostedt (Google)
> ---
> include/trace/stages/stage5_get_offsets.h | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/trace/stages/stage5_get_offsets.h
> b/include/trace/stages/stage5_get_offsets.h
> index e6b96608f452..c6a62dfb18ef 100644
> --- a/include/trace/stages/stage5_get_offsets.h
> +++ b/include/trace/stages/stage5_get_offsets.h
> @@ -9,6 +9,16 @@
> #undef __entry
> #define __entry entry
>
> +#ifndef __STAGE5_STRING_SRC_H
> +#define __STAGE5_STRING_SRC_H
> +static inline const char *__string_src(const char *str)
> +{
> + if (!str)
> +return EVENT_NULL_STR;
> + return str;
> +}
> +#endif /* __STAGE5_STRING_SRC_H */
> +
> /*
> * Fields should never declare an array: i.e. __field(int, arr[5])
> * If they do, it will cause issues in parsing and possibly corrupt the
> @@ -47,7 +57,7 @@
>
> #undef __string
> #define __string(item, src) __dynamic_array(char, item,
> \
> - strlen((const char *)(src) ? : EVENT_NULL_STR) + 1) \
> + strlen(__string_src(src)) + 1) \
> __data_offsets->item##_ptr_ = src;
>
> #undef __string_len
> @@ -70,7 +80,7 @@
>
> #undef __rel_string
> #define __rel_string(item, src) __rel_dynamic_array(char, item,
> \
> - strlen((const char *)(src) ? : EVENT_NULL_STR) + 1) \
> + strlen(__string_src(src)) + 1) \
> __data_offsets->item##_ptr_ = src;
>
> #undef __rel_string_len
> --
> 2.43.0
>
Re: [PATCH v7 5/5] dax: add a sysfs knob to control memmap_on_memory behavior
On Wed, Jan 24, 2024 at 12:03:50PM -0800, Vishal Verma wrote:
> Add a sysfs knob for dax devices to control the memmap_on_memory setting
> if the dax device were to be hotplugged as system memory.
>
> The default memmap_on_memory setting for dax devices originating via
> pmem or hmem is set to 'false' - i.e. no memmap_on_memory semantics, to
> preserve legacy behavior. For dax devices via CXL, the default is on.
> The sysfs control allows the administrator to override the above
> defaults if needed.
Reviewed-by: Alison Schofield
>
> Cc: David Hildenbrand
> Cc: Dan Williams
> Cc: Dave Jiang
> Cc: Dave Hansen
> Cc: Huang Ying
> Tested-by: Li Zhijian
> Reviewed-by: Jonathan Cameron
> Reviewed-by: David Hildenbrand
> Reviewed-by: Huang, Ying
> Signed-off-by: Vishal Verma
> ---
> drivers/dax/bus.c | 43
> +
> Documentation/ABI/testing/sysfs-bus-dax | 17 +
> 2 files changed, 60 insertions(+)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 0fd948a4443e..27c86d0ca711 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -1349,6 +1349,48 @@ static ssize_t numa_node_show(struct device *dev,
> }
> static DEVICE_ATTR_RO(numa_node);
>
> +static ssize_t memmap_on_memory_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct dev_dax *dev_dax = to_dev_dax(dev);
> +
> + return sysfs_emit(buf, "%d\n", dev_dax->memmap_on_memory);
> +}
> +
> +static ssize_t memmap_on_memory_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct dev_dax *dev_dax = to_dev_dax(dev);
> + bool val;
> + int rc;
> +
> + rc = kstrtobool(buf, &val);
> + if (rc)
> + return rc;
> +
> + if (val == true && !mhp_supports_memmap_on_memory()) {
> + dev_dbg(dev, "memmap_on_memory is not available\n");
> + return -EOPNOTSUPP;
> + }
> +
> + rc = down_write_killable(&dax_dev_rwsem);
> + if (rc)
> + return rc;
> +
> + if (dev_dax->memmap_on_memory != val && dev->driver &&
> + to_dax_drv(dev->driver)->type == DAXDRV_KMEM_TYPE) {
> + up_write(&dax_dev_rwsem);
> + return -EBUSY;
> + }
> +
> + dev_dax->memmap_on_memory = val;
> + up_write(&dax_dev_rwsem);
> +
> + return len;
> +}
> +static DEVICE_ATTR_RW(memmap_on_memory);
> +
> static umode_t dev_dax_visible(struct kobject *kobj, struct attribute *a,
> int n)
> {
> struct device *dev = container_of(kobj, struct device, kobj);
> @@ -1375,6 +1417,7 @@ static struct attribute *dev_dax_attributes[] = {
> &dev_attr_align.attr,
> &dev_attr_resource.attr,
> &dev_attr_numa_node.attr,
> + &dev_attr_memmap_on_memory.attr,
> NULL,
> };
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dax
> b/Documentation/ABI/testing/sysfs-bus-dax
> index 6359f7bc9bf4..b34266bfae49 100644
> --- a/Documentation/ABI/testing/sysfs-bus-dax
> +++ b/Documentation/ABI/testing/sysfs-bus-dax
> @@ -134,3 +134,20 @@ KernelVersion: v5.1
> Contact: nvd...@lists.linux.dev
> Description:
> (RO) The id attribute indicates the region id of a dax region.
> +
> +What:/sys/bus/dax/devices/daxX.Y/memmap_on_memory
> +Date:January, 2024
> +KernelVersion: v6.8
> +Contact: nvd...@lists.linux.dev
> +Description:
> + (RW) Control the memmap_on_memory setting if the dax device
> + were to be hotplugged as system memory. This determines whether
> + the 'altmap' for the hotplugged memory will be placed on the
> + device being hotplugged (memmap_on_memory=1) or if it will be
> + placed on regular memory (memmap_on_memory=0). This attribute
> + must be set before the device is handed over to the 'kmem'
> + driver (i.e. hotplugged into system-ram). Additionally, this
> + depends on CONFIG_MHP_MEMMAP_ON_MEMORY, and a globally enabled
> + memmap_on_memory parameter for memory_hotplug. This is
> + typically set on the kernel command line -
> + memory_hotplug.memmap_on_memory set to 'true' or 'force'."
>
> --
> 2.43.0
>
>
Re: [PATCH v7 2/5] dax/bus.c: replace several sprintf() with sysfs_emit()
On Wed, Jan 24, 2024 at 12:03:47PM -0800, Vishal Verma wrote:
> There were several places where drivers/dax/bus.c uses 'sprintf' to
> print sysfs data. Since a sysfs_emit() helper is available specifically
> for this purpose, replace all the sprintf() usage for sysfs with
> sysfs_emit() in this file.
>
Reviewed-by: Alison Schofield
> Cc: Dan Williams
> Reported-by: Greg Kroah-Hartman
> Signed-off-by: Vishal Verma
> ---
> drivers/dax/bus.c | 32
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index cb148f74ceda..0fd948a4443e 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -269,7 +269,7 @@ static ssize_t id_show(struct device *dev,
> {
> struct dax_region *dax_region = dev_get_drvdata(dev);
>
> - return sprintf(buf, "%d\n", dax_region->id);
> + return sysfs_emit(buf, "%d\n", dax_region->id);
> }
> static DEVICE_ATTR_RO(id);
>
> @@ -278,8 +278,8 @@ static ssize_t region_size_show(struct device *dev,
> {
> struct dax_region *dax_region = dev_get_drvdata(dev);
>
> - return sprintf(buf, "%llu\n", (unsigned long long)
> - resource_size(&dax_region->res));
> + return sysfs_emit(buf, "%llu\n",
> + (unsigned long long)resource_size(&dax_region->res));
> }
> static struct device_attribute dev_attr_region_size = __ATTR(size, 0444,
> region_size_show, NULL);
> @@ -289,7 +289,7 @@ static ssize_t region_align_show(struct device *dev,
> {
> struct dax_region *dax_region = dev_get_drvdata(dev);
>
> - return sprintf(buf, "%u\n", dax_region->align);
> + return sysfs_emit(buf, "%u\n", dax_region->align);
> }
> static struct device_attribute dev_attr_region_align =
> __ATTR(align, 0400, region_align_show, NULL);
> @@ -322,7 +322,7 @@ static ssize_t available_size_show(struct device *dev,
> size = dax_region_avail_size(dax_region);
> up_read(&dax_region_rwsem);
>
> - return sprintf(buf, "%llu\n", size);
> + return sysfs_emit(buf, "%llu\n", size);
> }
> static DEVICE_ATTR_RO(available_size);
>
> @@ -340,7 +340,7 @@ static ssize_t seed_show(struct device *dev,
> if (rc)
> return rc;
> seed = dax_region->seed;
> - rc = sprintf(buf, "%s\n", seed ? dev_name(seed) : "");
> + rc = sysfs_emit(buf, "%s\n", seed ? dev_name(seed) : "");
> up_read(&dax_region_rwsem);
>
> return rc;
> @@ -361,7 +361,7 @@ static ssize_t create_show(struct device *dev,
> if (rc)
> return rc;
> youngest = dax_region->youngest;
> - rc = sprintf(buf, "%s\n", youngest ? dev_name(youngest) : "");
> + rc = sysfs_emit(buf, "%s\n", youngest ? dev_name(youngest) : "");
> up_read(&dax_region_rwsem);
>
> return rc;
> @@ -763,7 +763,7 @@ static ssize_t start_show(struct device *dev,
> dax_range = get_dax_range(dev);
> if (!dax_range)
> return -ENXIO;
> - rc = sprintf(buf, "%#llx\n", dax_range->range.start);
> + rc = sysfs_emit(buf, "%#llx\n", dax_range->range.start);
> put_dax_range();
>
> return rc;
> @@ -779,7 +779,7 @@ static ssize_t end_show(struct device *dev,
> dax_range = get_dax_range(dev);
> if (!dax_range)
> return -ENXIO;
> - rc = sprintf(buf, "%#llx\n", dax_range->range.end);
> + rc = sysfs_emit(buf, "%#llx\n", dax_range->range.end);
> put_dax_range();
>
> return rc;
> @@ -795,7 +795,7 @@ static ssize_t pgoff_show(struct device *dev,
> dax_range = get_dax_range(dev);
> if (!dax_range)
> return -ENXIO;
> - rc = sprintf(buf, "%#lx\n", dax_range->pgoff);
> + rc = sysfs_emit(buf, "%#lx\n", dax_range->pgoff);
> put_dax_range();
>
> return rc;
> @@ -969,7 +969,7 @@ static ssize_t size_show(struct device *dev,
> size = dev_dax_size(dev_dax);
> up_write(&dax_dev_rwsem);
>
> - return sprintf(buf, "%llu\n", size);
> + return sysfs_emit(buf, "%llu\n", size);
> }
>
> static bool alloc_is_aligned(struct dev_dax *dev_dax, resource_size_t size)
> @@ -1233,7 +1233,7 @@ static ssize_t align_show(struct device *dev,
> {
> struct dev_dax *dev_dax = to_dev_dax(dev);
>
> - return sprintf
Re: [PATCH v7 1/5] dax/bus.c: replace driver-core lock usage by a local rwsem
On Wed, Jan 24, 2024 at 12:03:46PM -0800, Vishal Verma wrote:
> The dax driver incorrectly used driver-core device locks to protect
> internal dax region and dax device configuration structures. Replace the
> device lock usage with a local rwsem, one each for dax region
> configuration and dax device configuration. As a result of this
> conversion, no device_lock() usage remains in dax/bus.c.
>
Reviewed-by: Alison Schofield
> Cc: Dan Williams
> Reported-by: Greg Kroah-Hartman
> Signed-off-by: Vishal Verma
> ---
> drivers/dax/bus.c | 220
> ++
> 1 file changed, 157 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 1ff1ab5fa105..cb148f74ceda 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -12,6 +12,18 @@
>
> static DEFINE_MUTEX(dax_bus_lock);
>
> +/*
> + * All changes to the dax region configuration occur with this lock held
> + * for write.
> + */
> +DECLARE_RWSEM(dax_region_rwsem);
> +
> +/*
> + * All changes to the dax device configuration occur with this lock held
> + * for write.
> + */
> +DECLARE_RWSEM(dax_dev_rwsem);
> +
> #define DAX_NAME_LEN 30
> struct dax_id {
> struct list_head list;
> @@ -180,7 +192,7 @@ static u64 dev_dax_size(struct dev_dax *dev_dax)
> u64 size = 0;
> int i;
>
> - device_lock_assert(&dev_dax->dev);
> + WARN_ON_ONCE(!rwsem_is_locked(&dax_dev_rwsem));
>
> for (i = 0; i < dev_dax->nr_range; i++)
> size += range_len(&dev_dax->ranges[i].range);
> @@ -194,8 +206,15 @@ static int dax_bus_probe(struct device *dev)
> struct dev_dax *dev_dax = to_dev_dax(dev);
> struct dax_region *dax_region = dev_dax->region;
> int rc;
> + u64 size;
>
> - if (dev_dax_size(dev_dax) == 0 || dev_dax->id < 0)
> + rc = down_read_interruptible(&dax_dev_rwsem);
> + if (rc)
> + return rc;
> + size = dev_dax_size(dev_dax);
> + up_read(&dax_dev_rwsem);
> +
> + if (size == 0 || dev_dax->id < 0)
> return -ENXIO;
>
> rc = dax_drv->probe(dev_dax);
> @@ -283,7 +302,7 @@ static unsigned long long dax_region_avail_size(struct
> dax_region *dax_region)
> resource_size_t size = resource_size(&dax_region->res);
> struct resource *res;
>
> - device_lock_assert(dax_region->dev);
> + WARN_ON_ONCE(!rwsem_is_locked(&dax_region_rwsem));
>
> for_each_dax_region_resource(dax_region, res)
> size -= resource_size(res);
> @@ -295,10 +314,13 @@ static ssize_t available_size_show(struct device *dev,
> {
> struct dax_region *dax_region = dev_get_drvdata(dev);
> unsigned long long size;
> + int rc;
>
> - device_lock(dev);
> + rc = down_read_interruptible(&dax_region_rwsem);
> + if (rc)
> + return rc;
> size = dax_region_avail_size(dax_region);
> - device_unlock(dev);
> + up_read(&dax_region_rwsem);
>
> return sprintf(buf, "%llu\n", size);
> }
> @@ -314,10 +336,12 @@ static ssize_t seed_show(struct device *dev,
> if (is_static(dax_region))
> return -EINVAL;
>
> - device_lock(dev);
> + rc = down_read_interruptible(&dax_region_rwsem);
> + if (rc)
> + return rc;
> seed = dax_region->seed;
> rc = sprintf(buf, "%s\n", seed ? dev_name(seed) : "");
> - device_unlock(dev);
> + up_read(&dax_region_rwsem);
>
> return rc;
> }
> @@ -333,14 +357,18 @@ static ssize_t create_show(struct device *dev,
> if (is_static(dax_region))
> return -EINVAL;
>
> - device_lock(dev);
> + rc = down_read_interruptible(&dax_region_rwsem);
> + if (rc)
> + return rc;
> youngest = dax_region->youngest;
> rc = sprintf(buf, "%s\n", youngest ? dev_name(youngest) : "");
> - device_unlock(dev);
> + up_read(&dax_region_rwsem);
>
> return rc;
> }
>
> +static struct dev_dax *__devm_create_dev_dax(struct dev_dax_data *data);
> +
> static ssize_t create_store(struct device *dev, struct device_attribute
> *attr,
> const char *buf, size_t len)
> {
> @@ -358,7 +386,9 @@ static ssize_t create_store(struct device *dev, struct
> device_attribute *attr,
> if (val != 1)
> return -EINVAL;
>
> - device_lock(dev);
> + rc = down_write_killable(&dax_region_rwsem);
> + if (rc)
> +
Re: [PATCH] ACPI: NFIT: add helper to_nfit_mem() to take device to nfit_mem
On Mon, Jul 03, 2023 at 02:17:29PM +0100, Ben Dooks wrote:
> Add a quick helper to just do struct device to the struct nfit_mem
> field it should be referencing. This reduces the number of code
> lines in some of the followgn code as it removes the intermediate
> struct nvdimm.
>
Hi Ben,
This a useful cleanup. Minor comments below.
Alison
> Signed-off-by: Ben Dooks
> ---
> drivers/acpi/nfit/core.c | 27 +--
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 0fcc247fdfac..9213b426b125 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -1361,18 +1361,22 @@ static const struct attribute_group
> *acpi_nfit_attribute_groups[] = {
> NULL,
> };
>
> -static struct acpi_nfit_memory_map *to_nfit_memdev(struct device *dev)
> +static struct nfit_mem *to_nfit_mem(struct device *dev)
> {
> struct nvdimm *nvdimm = to_nvdimm(dev);
> - struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
> + return nvdimm_provider_data(nvdimm);
I was going to say add a space before that return, but checkpatch
beat me to it, with a warning. Please fix that up.
WARNING: Missing a blank line after declarations
> +}
> +
> +static struct acpi_nfit_memory_map *to_nfit_memdev(struct device *dev)
> +{
> + struct nfit_mem *nfit_mem = to_nfit_mem(dev);
>
> return __to_nfit_memdev(nfit_mem);
> }
I was a bit puzzled why the diff looked awkward because when I applied
the patch it fell nicely in order with the new helper function first.
It all merges the same in the end but it's easier on reviewers eyes
when well presented. Consider using diff.algorithm=patience (output
shown below) when things scramble needlessly.
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -1361,18 +1361,22 @@ static const struct attribute_group
*acpi_nfit_attribute_groups[] = {
NULL,
};
+static struct nfit_mem *to_nfit_mem(struct device *dev)
+{
+ struct nvdimm *nvdimm = to_nvdimm(dev);
+ return nvdimm_provider_data(nvdimm);
+}
+
static struct acpi_nfit_memory_map *to_nfit_memdev(struct device *dev)
{
- struct nvdimm *nvdimm = to_nvdimm(dev);
- struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+ struct nfit_mem *nfit_mem = to_nfit_mem(dev);
return __to_nfit_memdev(nfit_mem);
}
snip
>
Re: [PATCH] dax/kmem: Pass valid argument to memory_group_register_static
On Tue, Jun 20, 2023 at 07:33:32PM +0530, Tarun Sahu wrote: > memory_group_register_static takes maximum number of pages as the argument > while dev_dax_kmem_probe passes total_len (in bytes) as the argument. This sounds like a fix. An explanation of the impact and a fixes tag may be needed. Also, wondering how you found it. Alison > > Signed-off-by: Tarun Sahu > --- > drivers/dax/kmem.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dax/kmem.c b/drivers/dax/kmem.c > index 7b36db6f1cbd..898ca9505754 100644 > --- a/drivers/dax/kmem.c > +++ b/drivers/dax/kmem.c > @@ -99,7 +99,7 @@ static int dev_dax_kmem_probe(struct dev_dax *dev_dax) > if (!data->res_name) > goto err_res_name; > > - rc = memory_group_register_static(numa_node, total_len); > + rc = memory_group_register_static(numa_node, PFN_UP(total_len)); > if (rc < 0) > goto err_reg_mgid; > data->mgid = rc; > -- > 2.31.1 >
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
On Fri, Apr 14, 2023 at 06:50:59PM +0200, Markus Elfring wrote: > >> The address of a data structure member was determined before > >> a corresponding null pointer check in the implementation of > >> the function “nd_pfn_validate”. > >> > >> Thus avoid the risk for undefined behaviour by replacing the usage of > >> the local variable “parent_uuid” by a direct function call within > >> a later condition check. > > > > Hi Markus, > > > > I think I understand what you are saying above, but I don't follow > > how that applies here. This change seems to be a nice simplification, > > parent_uuid, is used once, just grab it when needed. > > Thanks for your positive feedback. Hi Markus, FYI - I'm a tiny bit taken aback that in response to me applying, and providing feedback, on your patch, you respond with 2 links for me to follow and cut off a chunk of my feedback. Seems like it would taken the same amount of time to just answer my two questions directly. Was this part of a larger patch set? Andy's comment seems to indicate that. Would have been nice to be CC'd on the cover letter. More below... > > > > What is the risk of undefined behavior? > > See also: > https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504137#comment-405504137 Where is the NULL pointer dereference here? > > > >> This issue was detected by using the Coccinelle software. > > Which cocci script? > > See also: > Reconsidering pointer dereferences before null pointer checks (with SmPL) > https://lore.kernel.org/cocci/1a11455f-ab57-dce0-1677-6beb8492a...@web.de/ > https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00021.html > The cocci script linked above does not seem to apply here. > > How do you think about to review and improve any similarly affected software > components? > > Regards, > Markus >
Re: [PATCH] nvdimm: Replace the usage of a variable by a direct function call in nd_pfn_validate()
On Fri, Apr 14, 2023 at 12:12:37PM +0200, Markus Elfring wrote:
> Date: Fri, 14 Apr 2023 12:01:15 +0200
>
> The address of a data structure member was determined before
> a corresponding null pointer check in the implementation of
> the function “nd_pfn_validate”.
>
> Thus avoid the risk for undefined behaviour by replacing the usage of
> the local variable “parent_uuid” by a direct function call within
> a later condition check.
Hi Markus,
I think I understand what you are saying above, but I don't follow
how that applies here. This change seems to be a nice simplification,
parent_uuid, is used once, just grab it when needed.
What is the risk of undefined behavior?
>
> This issue was detected by using the Coccinelle software.
Which cocci script?
>
> Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add uuid
> helpers")
This fixes tag seems to be the wrong tag. It is a tag from when the
uuid helpers were introduce, not where parent_uuid was first introduced
and used. I'm not clear this warrants a Fixes tag anyway. Is there
really a bug here? Perhaps I'm missing something in the previous
explanation of risk.
checkpatch is WARNING on the tag format:
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("")' - ie: 'Fixes: d1c6e08e7503 ("libnvdimm/labels: Add uuid helpers")'
#17:
Fixes: d1c6e08e7503649e4a4f3f9e700e2c05300b6379 ("libnvdimm/labels: Add
uuid helpers")
checkpatch is also WARNING on the commit msg:
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per
line)
#5:
nvdimm: Replace the usage of a variable by a direct function call in
nd_pfn_validate()
Also, possible only my pet peeve, the long commit message spoils my
pretty 80 column view. Please trim it to not wrap here:
$git log --oneline pfn_devs.c
52b639e56a46 nvdimm: Replace the usage of a variable by a direct function call
in nd_pfn_validate()
c91d71363084 nvdimm: Support sizeof(struct page) > MAX_STRUCT_PAGE_SIZE
6e9f05dc66f9 libnvdimm/pfn_dev: increase MAX_STRUCT_PAGE_SIZE
81beea55cb74 nvdimm: Drop nd_device_lock()
4a0079bc7aae nvdimm: Replace lockdep_mutex with local lock classes
322cbb50de71 block: remove genhd.h
Alison
> Signed-off-by: Markus Elfring
> ---
> drivers/nvdimm/pfn_devs.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index af7d9301520c..f14cbfa500ed 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -456,7 +456,6 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char
> *sig)
> unsigned long align, start_pad;
> struct nd_pfn_sb *pfn_sb = nd_pfn->pfn_sb;
> struct nd_namespace_common *ndns = nd_pfn->ndns;
> - const uuid_t *parent_uuid = nd_dev_to_uuid(&ndns->dev);
>
> if (!pfn_sb || !ndns)
> return -ENODEV;
> @@ -476,7 +475,7 @@ int nd_pfn_validate(struct nd_pfn *nd_pfn, const char
> *sig)
> return -ENODEV;
> pfn_sb->checksum = cpu_to_le64(checksum);
>
> - if (memcmp(pfn_sb->parent_uuid, parent_uuid, 16) != 0)
> + if (memcmp(pfn_sb->parent_uuid, nd_dev_to_uuid(&ndns->dev), 16) != 0)
> return -ENODEV;
>
> if (__le16_to_cpu(pfn_sb->version_minor) < 1) {
> --
> 2.40.0
>
Re: [PATCH] nvdimm: check for null return of devm_kmalloc in nd_pfn_probe
On Sun, Feb 26, 2023 at 01:56:15PM +0800, Kang Chen wrote:
> devm_kmalloc may fails, pfn_sb might be null and will cause
> null pointer dereference later.
>
> Signed-off-by: Kang Chen
> ---
> drivers/nvdimm/pfn_devs.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index af7d93015..d24fad175 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -640,6 +640,8 @@ int nd_pfn_probe(struct device *dev, struct
> nd_namespace_common *ndns)
> if (!pfn_dev)
> return -ENOMEM;
> pfn_sb = devm_kmalloc(dev, sizeof(*pfn_sb), GFP_KERNEL);
> + if (!pfn_sb)
> + return -ENOMEM;
> nd_pfn = to_nd_pfn(pfn_dev);
> nd_pfn->pfn_sb = pfn_sb;
> rc = nd_pfn_validate(nd_pfn, PFN_SIG);
Hi Kang,
I too, think the code is clearer if the failure to alloc is addressed
immediately. In this case, it seems we can't just return -ENOMEM.
The original code is detecting that NULL pfn_sb in nd_pfn_validate(),
and then doing this cleanup upon return:
if (rc < 0) {
nd_detach_ndns(pfn_dev, &nd_pfn->ndns);
put_device(pfn_dev);
Perhaps refactor a bit to go right to the cleanup, as opposed to calling
nd_pfn_validate() when !pfn_sb.
Alison
> --
> 2.34.1
>
>
[tip: x86/core] x86, sched: Treat Intel SNC topology as default, COD as exception
The following commit has been merged into the x86/core branch of tip:
Commit-ID: 2c88d45edbb89029c1190bb3b136d2602f057c98
Gitweb:
https://git.kernel.org/tip/2c88d45edbb89029c1190bb3b136d2602f057c98
Author:Alison Schofield
AuthorDate:Wed, 10 Mar 2021 11:02:33 -08:00
Committer: Peter Zijlstra
CommitterDate: Thu, 15 Apr 2021 18:34:20 +02:00
x86, sched: Treat Intel SNC topology as default, COD as exception
Commit 1340ccfa9a9a ("x86,sched: Allow topologies where NUMA nodes
share an LLC") added a vendor and model specific check to never
call topology_sane() for Intel Skylake Server systems where NUMA
nodes share an LLC.
Intel Ice Lake and Sapphire Rapids CPUs also enumerate an LLC that is
shared by multiple NUMA nodes. The LLC on these CPUs is shared for
off-package data access but private to the NUMA node for on-package
access. Rather than managing a list of allowable SNC topologies, make
this SNC topology the default, and treat Intel's Cluster-On-Die (COD)
topology as the exception.
In SNC mode, Sky Lake, Ice Lake, and Sapphire Rapids servers do not
emit this warning:
sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0].
Ignoring dependency.
Suggested-by: Peter Zijlstra (Intel)
Signed-off-by: Alison Schofield
Signed-off-by: Peter Zijlstra (Intel)
Acked-by: Dave Hansen
Cc: sta...@vger.kernel.org
Link:
https://lkml.kernel.org/r/20210310190233.31752-1-alison.schofi...@intel.com
---
arch/x86/kernel/smpboot.c | 90 +++---
1 file changed, 46 insertions(+), 44 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 02813a7..147b2f3 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -458,29 +458,52 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
return false;
}
+static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+ if (c->phys_proc_id == o->phys_proc_id &&
+ c->cpu_die_id == o->cpu_die_id)
+ return true;
+ return false;
+}
+
/*
- * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
+ * Unlike the other levels, we do not enforce keeping a
+ * multicore group inside a NUMA node. If this happens, we will
+ * discard the MC level of the topology later.
+ */
+static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+ if (c->phys_proc_id == o->phys_proc_id)
+ return true;
+ return false;
+}
+
+/*
+ * Define intel_cod_cpu[] for Intel COD (Cluster-on-Die) CPUs.
*
- * These are Intel CPUs that enumerate an LLC that is shared by
- * multiple NUMA nodes. The LLC on these systems is shared for
- * off-package data access but private to the NUMA node (half
- * of the package) for on-package access.
+ * Any Intel CPU that has multiple nodes per package and does not
+ * match intel_cod_cpu[] has the SNC (Sub-NUMA Cluster) topology.
*
- * CPUID (the source of the information about the LLC) can only
- * enumerate the cache as being shared *or* unshared, but not
- * this particular configuration. The CPU in this case enumerates
- * the cache to be shared across the entire package (spanning both
- * NUMA nodes).
+ * When in SNC mode, these CPUs enumerate an LLC that is shared
+ * by multiple NUMA nodes. The LLC is shared for off-package data
+ * access but private to the NUMA node (half of the package) for
+ * on-package access. CPUID (the source of the information about
+ * the LLC) can only enumerate the cache as shared or unshared,
+ * but not this particular configuration.
*/
-static const struct x86_cpu_id snc_cpu[] = {
- X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
+static const struct x86_cpu_id intel_cod_cpu[] = {
+ X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, 0), /* COD */
+ X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, 0), /* COD */
+ X86_MATCH_INTEL_FAM6_MODEL(ANY, 1), /* SNC */
{}
};
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
+ const struct x86_cpu_id *id = x86_match_cpu(intel_cod_cpu);
int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+ bool intel_snc = id && id->driver_data;
/* Do not match if we do not have a valid APICID for cpu: */
if (per_cpu(cpu_llc_id, cpu1) == BAD_APICID)
@@ -495,32 +518,12 @@ static bool match_llc(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
* means 'c' does not share the LLC of 'o'. This will be
* reflected to userspace.
*/
- if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
+ if (match_pkg(c, o) && !topology_same_node(c, o) && intel_snc)
return false;
return topology_sane(c, o, "llc");
}
-/*
- * Unlike the other levels, we do not enforce keeping a
- * multicore group inside a NUMA node. If this happens, we will
Re: [PATCH v3] x86, sched: Treat Intel SNC topology as default, COD as exception
Ping - any feedback?
Thanks!
On Wed, Mar 10, 2021 at 11:02:33AM -0800, Alison Schofield wrote:
> Commit 1340ccfa9a9a ("x86,sched: Allow topologies where NUMA nodes
> share an LLC") added a vendor and model specific check to never
> call topology_sane() for Intel Skylake Server systems where NUMA
> nodes share an LLC.
>
> Intel Ice Lake and Sapphire Rapids CPUs also enumerate an LLC that is
> shared by multiple NUMA nodes. The LLC on these CPUs is shared for
> off-package data access but private to the NUMA node for on-package
> access. Rather than managing a list of allowable SNC topologies, make
> this SNC topology the default, and treat Intel's Cluster-On-Die (COD)
> topology as the exception.
>
> In SNC mode, Sky Lake, Ice Lake, and Sapphire Rapids servers do not
> emit this warning:
>
> sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0].
> Ignoring dependency.
>
> Acked-by: Dave Hansen
> Suggested-by: Peter Zijlstra (Intel)
> Signed-off-by: Alison Schofield
> Cc: sta...@vger.kernel.org
> Cc: Dave Hansen
> Cc: Tony Luck
> Cc: Tim Chen
> Cc: "H. Peter Anvin"
> Cc: Borislav Petkov
> Cc: Peter Zijlstra (Intel)
> Cc: David Rientjes
> Cc: Igor Mammedov
> Cc: Prarit Bhargava
> Cc: brice.gog...@gmail.com
> ---
>
> Changes v2->v3:
> - This is a v3 of this patch:
> https://lore.kernel.org/lkml/20210216195804.24204-1-alison.schofi...@intel.com/
> - Implemented PeterZ suggestion, and his code, to check for COD instead
> of SNC.
> - Updated commit message and log.
> - Added 'Cc stable.
>
> Changes v1->v2:
> - Implemented the minimal required change of adding the new models to
> the existing vendor and model specific check.
>
> - Side effect of going minimalist: no longer labelled an X86_BUG (TonyL)
>
> - Considered PeterZ suggestion of checking for COD CPUs, rather than
> SNC CPUs. That meant this snc_cpu list would go away, and so it never
> needs updating. That ups the stakes for this patch wrt LOC changed
> and testing needed. It actually drove me back to this simplest soln.
>
> - Considered DaveH suggestion to remove the check altogether and recognize
> these topologies as sane. Not running with that further here. This patch
> is what is needed now. The broader discussion of sane topologies can
> carry on independent of this update.
>
> - Updated commit message and log.
>
>
> arch/x86/kernel/smpboot.c | 90 ---
> 1 file changed, 46 insertions(+), 44 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 02813a7f3a7c..147b2f3a2a09 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -458,29 +458,52 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
> cpuinfo_x86 *o)
> return false;
> }
>
> +static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
> +{
> + if (c->phys_proc_id == o->phys_proc_id &&
> + c->cpu_die_id == o->cpu_die_id)
> + return true;
> + return false;
> +}
> +
> +/*
> + * Unlike the other levels, we do not enforce keeping a
> + * multicore group inside a NUMA node. If this happens, we will
> + * discard the MC level of the topology later.
> + */
> +static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
> +{
> + if (c->phys_proc_id == o->phys_proc_id)
> + return true;
> + return false;
> +}
> +
> /*
> - * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
> + * Define intel_cod_cpu[] for Intel COD (Cluster-on-Die) CPUs.
> *
> - * These are Intel CPUs that enumerate an LLC that is shared by
> - * multiple NUMA nodes. The LLC on these systems is shared for
> - * off-package data access but private to the NUMA node (half
> - * of the package) for on-package access.
> + * Any Intel CPU that has multiple nodes per package and does not
> + * match intel_cod_cpu[] has the SNC (Sub-NUMA Cluster) topology.
> *
> - * CPUID (the source of the information about the LLC) can only
> - * enumerate the cache as being shared *or* unshared, but not
> - * this particular configuration. The CPU in this case enumerates
> - * the cache to be shared across the entire package (spanning both
> - * NUMA nodes).
> + * When in SNC mode, these CPUs enumerate an LLC that is shared
> + * by multiple NUMA nodes. The LLC is shared for off-package data
> + * access but private to the NUMA node (half of the package) for
> + * on-package access. CPUID (the source of the information about
> + * the LLC) can only enumerate the cache as shared or unshar
[PATCH v3] x86, sched: Treat Intel SNC topology as default, COD as exception
Commit 1340ccfa9a9a ("x86,sched: Allow topologies where NUMA nodes
share an LLC") added a vendor and model specific check to never
call topology_sane() for Intel Skylake Server systems where NUMA
nodes share an LLC.
Intel Ice Lake and Sapphire Rapids CPUs also enumerate an LLC that is
shared by multiple NUMA nodes. The LLC on these CPUs is shared for
off-package data access but private to the NUMA node for on-package
access. Rather than managing a list of allowable SNC topologies, make
this SNC topology the default, and treat Intel's Cluster-On-Die (COD)
topology as the exception.
In SNC mode, Sky Lake, Ice Lake, and Sapphire Rapids servers do not
emit this warning:
sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0].
Ignoring dependency.
Acked-by: Dave Hansen
Suggested-by: Peter Zijlstra (Intel)
Signed-off-by: Alison Schofield
Cc: sta...@vger.kernel.org
Cc: Dave Hansen
Cc: Tony Luck
Cc: Tim Chen
Cc: "H. Peter Anvin"
Cc: Borislav Petkov
Cc: Peter Zijlstra (Intel)
Cc: David Rientjes
Cc: Igor Mammedov
Cc: Prarit Bhargava
Cc: brice.gog...@gmail.com
---
Changes v2->v3:
- This is a v3 of this patch:
https://lore.kernel.org/lkml/20210216195804.24204-1-alison.schofi...@intel.com/
- Implemented PeterZ suggestion, and his code, to check for COD instead
of SNC.
- Updated commit message and log.
- Added 'Cc stable.
Changes v1->v2:
- Implemented the minimal required change of adding the new models to
the existing vendor and model specific check.
- Side effect of going minimalist: no longer labelled an X86_BUG (TonyL)
- Considered PeterZ suggestion of checking for COD CPUs, rather than
SNC CPUs. That meant this snc_cpu list would go away, and so it never
needs updating. That ups the stakes for this patch wrt LOC changed
and testing needed. It actually drove me back to this simplest soln.
- Considered DaveH suggestion to remove the check altogether and recognize
these topologies as sane. Not running with that further here. This patch
is what is needed now. The broader discussion of sane topologies can
carry on independent of this update.
- Updated commit message and log.
arch/x86/kernel/smpboot.c | 90 ---
1 file changed, 46 insertions(+), 44 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 02813a7f3a7c..147b2f3a2a09 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -458,29 +458,52 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
return false;
}
+static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+ if (c->phys_proc_id == o->phys_proc_id &&
+ c->cpu_die_id == o->cpu_die_id)
+ return true;
+ return false;
+}
+
+/*
+ * Unlike the other levels, we do not enforce keeping a
+ * multicore group inside a NUMA node. If this happens, we will
+ * discard the MC level of the topology later.
+ */
+static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
+{
+ if (c->phys_proc_id == o->phys_proc_id)
+ return true;
+ return false;
+}
+
/*
- * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
+ * Define intel_cod_cpu[] for Intel COD (Cluster-on-Die) CPUs.
*
- * These are Intel CPUs that enumerate an LLC that is shared by
- * multiple NUMA nodes. The LLC on these systems is shared for
- * off-package data access but private to the NUMA node (half
- * of the package) for on-package access.
+ * Any Intel CPU that has multiple nodes per package and does not
+ * match intel_cod_cpu[] has the SNC (Sub-NUMA Cluster) topology.
*
- * CPUID (the source of the information about the LLC) can only
- * enumerate the cache as being shared *or* unshared, but not
- * this particular configuration. The CPU in this case enumerates
- * the cache to be shared across the entire package (spanning both
- * NUMA nodes).
+ * When in SNC mode, these CPUs enumerate an LLC that is shared
+ * by multiple NUMA nodes. The LLC is shared for off-package data
+ * access but private to the NUMA node (half of the package) for
+ * on-package access. CPUID (the source of the information about
+ * the LLC) can only enumerate the cache as shared or unshared,
+ * but not this particular configuration.
*/
-static const struct x86_cpu_id snc_cpu[] = {
- X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
+static const struct x86_cpu_id intel_cod_cpu[] = {
+ X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, 0), /* COD */
+ X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, 0), /* COD */
+ X86_MATCH_INTEL_FAM6_MODEL(ANY, 1), /* SNC */
{}
};
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
+ const struct x86_cpu_id *id = x86_match_cpu(intel_cod_cpu);
int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
+ bool intel_snc = id && id->driver_dat
[PATCH v2] x86,sched: Update the Intel SNC CPU list that allows shared LLCs
Commit 1340ccfa9a9a ("x86,sched: Allow topologies where NUMA nodes
share an LLC") added a vendor and model specific check to never
call topology_sane() for systems where NUMA nodes share an LLC.
Intel's Ice Lake and Sapphire Rapids CPUs exhibit this same topology.
They enumerate an LLC that is shared by multiple NUMA nodes. The
LLC on these CPUs is shared for off-package data access but private
to the NUMA node for on-package access. Since its CPUID can only
enumerate the cache as shared or unshared, add these CPUs to the
list of allowable topologies (snc_cpu[]).
In SNC mode, Ice Lake and Sapphire Rapids servers will no longer emit
this warning:
sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0].
Ignoring dependency.
Acked-by: Dave Hansen
Signed-off-by: Alison Schofield
Cc: Dave Hansen
Cc: Tony Luck
Cc: Tim Chen
Cc: "H. Peter Anvin"
Cc: Borislav Petkov
Cc: Peter Zijlstra (Intel)
Cc: David Rientjes
Cc: Igor Mammedov
Cc: Prarit Bhargava
Cc: brice.gog...@gmail.com
---
Changes v1->v2:
- Implemented the minimal required change of adding the new models to
the existing vendor and model specific check.
- Side effect of going minimalist: no longer labelled an X86_BUG (TonyL)
- Considered PeterZ suggestion of checking for COD CPUs, rather than
SNC CPUs. That meant this snc_cpu list would go away, and so it never
needs updating. That ups the stakes for this patch wrt LOC changed
and testing needed. It actually drove me back to this simplest soln.
- Considered DaveH suggestion to remove the check altogether and recognize
these topologies as sane. Not running with that further here. This patch
is what is needed now. The broader discussion of sane topologies can
carry on independent of this update.
- Updated commit message and log.
arch/x86/kernel/smpboot.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 02813a7f3a7c..de8c598dc3b9 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -475,6 +475,8 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
static const struct x86_cpu_id snc_cpu[] = {
X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
+ X86_MATCH_INTEL_FAM6_MODEL(ICELAKE_X, NULL),
+ X86_MATCH_INTEL_FAM6_MODEL(SAPPHIRERAPIDS_X, NULL),
{}
};
--
2.20.1
Re: [PATCH] x86, sched: Allow NUMA nodes to share an LLC on Intel platforms
On Tue, Feb 16, 2021 at 12:29:02PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 10, 2021 at 02:11:34PM -0800, Alison Schofield wrote:
>
> > This is equivalent to determining if x86_has_numa_in_package.
> > Do you think there is an opportunity to set x86_has_numa_in_package
> > earlier, and use it here and in set_cpu_sibling_map()?
>
> Sure. Not sure that's actually clearer though.
>
> > With that additional info (match_pkg()) how about -
> >
> > Instead of this:
> > - if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
> > + if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu) &&
> > match_pkg(c, o))
> >
> > Do this:
> >
> > - if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
> > + if (!topology_same_node(c, o) && match_pkg(c, o))
> >
> >
> > Looking at Commit 316ad248307f ("sched/x86: Rewrite set_cpu_sibling_map())
> > which reworked topology WARNINGs, the intent was to "make sure to
> > only warn when the check changes the end result"
> >
> > This check doesn't change the end result. It returns false directly
> > and if it were bypassed completely, it would still return false with
> > a WARNING.
>
> I'm not following the argument, we explicitly *do* modify the end result
> for those SNC caches. Also, by doing what you propose, we fail to get a
> warning if/when AMD decides to do 'funny' things.
This might be beating a dead horse (me==dead horse) but how do we
modify the end result? That is, I see false returned either way.
Am I missing another code path that considers this info?
>
> Suppose AMD too thinks this is a swell idea, but they have subtly
> different cache behaviour (just for giggles), then it all goes
> undetected, which would be bad.
>
Understood.
> > If we add that additional match_pkg() check is removing the WARNING for
> > all cases possible?
>
> How many parts had that Intel Cluster-on-Die thing? Seeing how all the
> new parts have the SNC crud, that seems like a finite list.
>
> Wikipedia seems to suggest haswell and broadwell were the only onces
> with COD on, skylake and later has the SNC.
>
> How's something like this then (needs splitting into multiple patches I
> suppose):
>
This is interesting. Flipping to check COD, instead of SNC would mean
we don't have to revisit this for future SNC CPUs. I don't think it's
worth the risk now (more code changes, more platforms to test).
The list exists. We can add to the list and it makes us think about the
SNC topologies as each new CPU is announced. That is a good nuisance.
Thanks for showing how the COD case would work.
> ---
> arch/x86/kernel/smpboot.c | 76
> +--
> 1 file changed, 41 insertions(+), 35 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 117e24fbfd8a..cfe23badf9a3 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -458,8 +458,31 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
> cpuinfo_x86 *o)
> return false;
> }
>
> +static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
> +{
> + if ((c->phys_proc_id == o->phys_proc_id) &&
> + (c->cpu_die_id == o->cpu_die_id))
> + return true;
> + return false;
> +}
> +
> +/*
> + * Unlike the other levels, we do not enforce keeping a
> + * multicore group inside a NUMA node. If this happens, we will
> + * discard the MC level of the topology later.
> + */
> +static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
> +{
> + if (c->phys_proc_id == o->phys_proc_id)
> + return true;
> + return false;
> +}
> +
> /*
> - * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
> + * Define intel_cod_cpu[] for Intel COD (Cluster-on-Die) CPUs.
> + *
> + * Any Intel CPU that has multiple nodes per package and doesn't match this
> + * will have the newer SNC (Sub-NUMA Cluster).
> *
> * These are Intel CPUs that enumerate an LLC that is shared by
> * multiple NUMA nodes. The LLC on these systems is shared for
> @@ -473,14 +496,18 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
> cpuinfo_x86 *o)
> * NUMA nodes).
> */
>
> -static const struct x86_cpu_id snc_cpu[] = {
> - X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
> +static const struct x86_cpu_id intel_cod_cpu[] = {
> + X86_MATCH_INTEL_FAM6_MODEL(HASWELL_X, 0), /* COD */
> + X86_MATCH_INTEL_FAM6_MODEL(BROADWELL_X, 0), /* COD */
> + X86
Re: [PATCH] x86, sched: Allow NUMA nodes to share an LLC on Intel platforms
On Wed, Feb 10, 2021 at 08:38:42PM +0100, Peter Zijlstra wrote:
> On Wed, Feb 10, 2021 at 07:22:03AM -0800, Dave Hansen wrote:
> > On 2/10/21 12:05 AM, Peter Zijlstra wrote:
> > >> +if (IS_ENABLED(CONFIG_NUMA))
> > >> +set_cpu_bug(c, X86_BUG_NUMA_SHARES_LLC);
> > >> }
> > > This seens wrong too, it shouldn't be allowed pre SKX. And ideally only
> > > be allowed when SNC is enabled.
> >
> > Originally, this just added a few more models to the list of CPUs with
> > SNC. I was hoping for something a bit more durable that we wouldn't
> > have to go back and poke at every year or two.
>
> It's not like we don't have to update a gazillion FMS tables for each
> new instance anyway :-(
>
> > > Please make this more specific than: all Intel CPUs. Ofcourse, since you
> > > all knew this was an issue, you could've made it discoverable
> > > _somewhere_ :-(
> >
> > You're totally right, of course. The hardware could enumerate SNC as a
> > feature explicitly somewhere. But, that's a little silly because all of
> > the information that it's enumerating about the CPU caches and NUMA
> > nodes present and correct is *correct*. The secondary information would
> > only be for the CPU to say, "yeah, I'm really sure about that other stuff".
> >
> > I think this sanity check has outlived its usefulness.
>
> Maybe BIOS monkeys got better, but I'm not sure I trust it all.
>
> So SNC is all on-package, do all those nodes have the same pkg id? That
> is, I'm trying to find something to restrict topological madness.
>
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 88cd0064d1f8..de1010dd0bba 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -458,6 +458,26 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
> cpuinfo_x86 *o)
> return false;
> }
>
> +static bool match_die(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
> +{
> + if ((c->phys_proc_id == o->phys_proc_id) &&
> + (c->cpu_die_id == o->cpu_die_id))
> + return true;
> + return false;
> +}
> +
> +/*
> + * Unlike the other levels, we do not enforce keeping a
> + * multicore group inside a NUMA node. If this happens, we will
> + * discard the MC level of the topology later.
> + */
> +static bool match_pkg(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
> +{
> + if (c->phys_proc_id == o->phys_proc_id)
> + return true;
> + return false;
> +}
> +
> /*
> * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
> *
> @@ -495,33 +515,12 @@ static bool match_llc(struct cpuinfo_x86 *c, struct
> cpuinfo_x86 *o)
>* means 'c' does not share the LLC of 'o'. This will be
>* reflected to userspace.
>*/
> - if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
> + if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu) && match_pkg(c,
> o))
> return false;
>
> return topology_sane(c, o, "llc");
> }
>
This is equivalent to determining if x86_has_numa_in_package.
Do you think there is an opportunity to set x86_has_numa_in_package
earlier, and use it here and in set_cpu_sibling_map()?
With that additional info (match_pkg()) how about -
Instead of this:
- if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
+ if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu) && match_pkg(c,
o))
Do this:
- if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
+ if (!topology_same_node(c, o) && match_pkg(c, o))
Looking at Commit 316ad248307f ("sched/x86: Rewrite set_cpu_sibling_map())
which reworked topology WARNINGs, the intent was to "make sure to
only warn when the check changes the end result"
This check doesn't change the end result. It returns false directly
and if it were bypassed completely, it would still return false with
a WARNING.
If we add that additional match_pkg() check is removing the WARNING for
all cases possible?
-snip
[PATCH] x86, sched: Allow NUMA nodes to share an LLC on Intel platforms
Commit 1340ccfa9a9a ("x86,sched: Allow topologies where NUMA nodes
share an LLC") added a vendor and model specific check to skip the
topology_sane() check for Intel's Sky Lake Server CPUs where NUMA
nodes shared an LLC.
This topology is no longer a quirk for Intel CPUs as Ice Lake and
Sapphire Rapids CPUs exhibit the same topology. Rather than maintain
the quirk list, define a synthetic flag that directs the scheduler
to allow this topology without warning for all Intel CPUs when NUMA
is configured.
Acked-by: Dave Hansen
Signed-off-by: Alison Schofield
Cc: Dave Hansen
Cc: Tony Luck
Cc: Tim Chen
Cc: "H. Peter Anvin"
Cc: Borislav Petkov
Cc: Peter Zijlstra (Intel)
Cc: David Rientjes
Cc: Igor Mammedov
Cc: Prarit Bhargava
Cc: brice.gog...@gmail.com
---
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/kernel/cpu/intel.c| 15 +++
arch/x86/kernel/smpboot.c | 23 ++-
3 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/arch/x86/include/asm/cpufeatures.h
b/arch/x86/include/asm/cpufeatures.h
index 84b887825f12..bec74b90d3d6 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -421,5 +421,6 @@
#define X86_BUG_TAAX86_BUG(22) /* CPU is affected by TSX
Async Abort(TAA) */
#define X86_BUG_ITLB_MULTIHIT X86_BUG(23) /* CPU may incur MCE during
certain page attribute changes */
#define X86_BUG_SRBDS X86_BUG(24) /* CPU may leak RNG bits if
not mitigated */
+#define X86_BUG_NUMA_SHARES_LLCX86_BUG(25) /* CPU may
enumerate an LLC shared by multiple NUMA nodes */
#endif /* _ASM_X86_CPUFEATURES_H */
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 816fdbec795a..027348261080 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -719,6 +719,21 @@ static void init_intel(struct cpuinfo_x86 *c)
tsx_disable();
split_lock_init();
+
+ /*
+* Set X86_BUG_NUMA_SHARES_LLC to allow topologies where NUMA
+* nodes share an LLC. In Sub-NUMA Clustering mode Intel CPUs
+* may enumerate an LLC as shared by multiple NUMA nodes. The
+* LLC is shared for off-package data access but private to
+* the NUMA node for on-package access. This topology first
+* appeared in SKYLAKE_X. It was treated as a quirk and allowed.
+* This topology reappeared in ICELAKE_X and SAPPHIRERAPIDS_X.
+* Rather than maintain a list of quirk CPUS, allow this topology
+* on all Intel CPUs with NUMA configured. When this X86_BUG is
+* set, the scheduler accepts this topology without warning.
+*/
+ if (IS_ENABLED(CONFIG_NUMA))
+ set_cpu_bug(c, X86_BUG_NUMA_SHARES_LLC);
}
#ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 117e24fbfd8a..7d05c3552795 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -458,26 +458,6 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
return false;
}
-/*
- * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
- *
- * These are Intel CPUs that enumerate an LLC that is shared by
- * multiple NUMA nodes. The LLC on these systems is shared for
- * off-package data access but private to the NUMA node (half
- * of the package) for on-package access.
- *
- * CPUID (the source of the information about the LLC) can only
- * enumerate the cache as being shared *or* unshared, but not
- * this particular configuration. The CPU in this case enumerates
- * the cache to be shared across the entire package (spanning both
- * NUMA nodes).
- */
-
-static const struct x86_cpu_id snc_cpu[] = {
- X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, NULL),
- {}
-};
-
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
@@ -495,7 +475,8 @@ static bool match_llc(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
* means 'c' does not share the LLC of 'o'. This will be
* reflected to userspace.
*/
- if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu))
+ if (!topology_same_node(c, o) &&
+ boot_cpu_has_bug(X86_BUG_NUMA_SHARES_LLC))
return false;
return topology_sane(c, o, "llc");
--
2.20.1
Re: [PATCH] Ability to read the MKTME status from userspace (patch v2)
On Thu, Jun 25, 2020 at 06:10:29PM -0300, Daniel Gutson wrote:
> The intent of this patch is to provide visibility of the
> MKTME status to userspace. This is an important factor for
> firmware security related applilcations.
>
> Changes since v1:
> * Documentation/ABI/stable/securityfs-mktme-status (new file)
> * include/uapi/misc/mktme_status.h (new file)
> * Fixed MAINTAINER title.
> * cpu.h: added values to the enumerands
> * Renamed the function from get_mktme_status to mktme_get_status
> * Improved Kconfig help
> * Removed unnecessary license-related lines since there is a SPDX line
> * Moved pr_fmt macro before the includes
> * Turned global variables (mktme_dir, mktme_file) as static
> * Removed BUFFER_SIZE macro
> * No longer returning -1 for error, but using the previously error.
> * No more pr_info for the normal behavior.
> * Renamed this from a kernel driver to a kernel module.
>
> Signed-off-by: Daniel Gutson
> ---
> .../ABI/stable/securityfs-mktme-status| 12
> MAINTAINERS | 5 ++
> arch/x86/include/asm/cpu.h| 8 +++
> arch/x86/kernel/cpu/intel.c | 12 ++--
> drivers/misc/Kconfig | 15 +
> drivers/misc/Makefile | 1 +
> drivers/misc/mktme_status.c | 67 +++
> include/uapi/misc/mktme_status.h | 12
> 8 files changed, 127 insertions(+), 5 deletions(-)
> create mode 100644 Documentation/ABI/stable/securityfs-mktme-status
> create mode 100644 drivers/misc/mktme_status.c
> create mode 100644 include/uapi/misc/mktme_status.h
>
> diff --git a/Documentation/ABI/stable/securityfs-mktme-status
> b/Documentation/ABI/stable/securityfs-mktme-status
> new file mode 100644
> index ..a791c6ef2c15
> --- /dev/null
> +++ b/Documentation/ABI/stable/securityfs-mktme-status
> @@ -0,0 +1,12 @@
> +What:/securityfs/mktme/status
> +Date:24-Jun-2020
> +KernelVersion: v5.7
> +Contact: daniel.gut...@eclypsium.com
> +Description: The mktme securityfs interface exports the BIOS status
> + of the MKTME.
> +Values: For possible values see arch/x86/include/asm/cpu.h.
> +
> + Currently these values are:
> + 0 Enabled by BIOS
> + 1 Disabled by BIOS
> + 2 Uninitialized
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b5ffd646c6b..e034e8ab6fe1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11477,6 +11477,11 @@ W: https://linuxtv.org
> T: git git://linuxtv.org/media_tree.git
> F: drivers/media/radio/radio-miropcm20*
>
> +MKTME STATUS READING SUPPORT
> +M: Daniel Gutson
> +S: Supported
> +F: drivers/misc/mktme_status.c
> +
> MMP SUPPORT
> R: Lubomir Rintel
> L: linux-arm-ker...@lists.infradead.org (moderated for non-subscribers)
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index dd17c2da1af5..80d2896a3f43 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -26,6 +26,14 @@ struct x86_cpu {
> struct cpu cpu;
> };
>
> +enum mktme_status_type {
> + MKTME_ENABLED = 0,
> + MKTME_DISABLED = 1,
> + MKTME_UNINITIALIZED = 2
> +};
> +
> +extern enum mktme_status_type mktme_get_status(void);
> +
> #ifdef CONFIG_HOTPLUG_CPU
> extern int arch_register_cpu(int num);
> extern void arch_unregister_cpu(int);
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index c25a67a34bd3..134a88685bc3 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -495,11 +495,7 @@ static void srat_detect_node(struct cpuinfo_x86 *c)
> #define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0x)/* Bits 63:48 */
> #define TME_ACTIVATE_CRYPTO_AES_XTS_128 1
>
> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED0
> -#define MKTME_DISABLED 1
> -#define MKTME_UNINITIALIZED 2
> -static int mktme_status = MKTME_UNINITIALIZED;
> +static enum mktme_status_type mktme_status = MKTME_UNINITIALIZED;
>
> static void detect_tme(struct cpuinfo_x86 *c)
> {
> @@ -1114,6 +1110,12 @@ bool handle_user_split_lock(struct pt_regs *regs, long
> error_code)
> return true;
> }
>
> +enum mktme_status_type mktme_get_status(void)
> +{
> + return mktme_status;
> +}
> +EXPORT_SYMBOL_GPL(mktme_get_status);
> +
Hi Daniel,
It's not clear this code is getting what it says its getting.
(putting aside for the moment the other feedback)
I think you said you want to know if TME is activated in BIOS. I get that.
You don't want the system to be up and the customer can't tell if they
actually turned on TME or not.
If you only look for MKTME enabled status, you may miss the TME enabled
status. We can have TME on and MKTME off. (Can't have opposite)
Here are the boot message I observe based on BIOS selections:
Se
Re: [PATCHv2 57/59] x86/mktme: Document the MKTME Key Service API
On Mon, Aug 05, 2019 at 07:58:37AM -0400, Ben Boeckel wrote:
> On Wed, Jul 31, 2019 at 18:08:11 +0300, Kirill A. Shutemov wrote:
> > + key = add_key("mktme", "name", "no-encrypt", strlen(options_CPU),
> > + KEY_SPEC_THREAD_KEYRING);
>
> Should this be `type=no-encrypt` here? Also, seems like copy/paste from
> the `type=cpu` case for the `strlen` call.
>
> --Ben
Yes. Fixed up as follows:
Add a "no-encrypt' type key::
char \*options_NOENCRYPT = "type=no-encrypt";
key = add_key("mktme", "name", options_NOENCRYPT,
strlen(options_NOENCRYPT), KEY_SPEC_THREAD_KEYRING);
Thanks for the review,
Alison
Re: [PATCHv2 25/59] keys/mktme: Preparse the MKTME key payload
On Mon, Aug 05, 2019 at 07:58:19AM -0400, Ben Boeckel wrote:
> On Wed, Jul 31, 2019 at 18:07:39 +0300, Kirill A. Shutemov wrote:
> > From: Alison Schofield
> > +/* Make sure arguments are correct for the TYPE of key requested */
> > +static int mktme_check_options(u32 *payload, unsigned long token_mask,
> > + enum mktme_type type, enum mktme_alg alg)
> > +{
> > + if (!token_mask)
> > + return -EINVAL;
> > +
> > + switch (type) {
> > + case MKTME_TYPE_CPU:
> > + if (test_bit(OPT_ALGORITHM, &token_mask))
> > + *payload |= (1 << alg) << 8;
> > + else
> > + return -EINVAL;
> > +
> > + *payload |= MKTME_KEYID_SET_KEY_RANDOM;
> > + break;
> > +
> > + case MKTME_TYPE_NO_ENCRYPT:
if (test_bit(OPT_ALGORITHM, &token_mask))
return -EINVAL;
> > + *payload |= MKTME_KEYID_NO_ENCRYPT;
> > + break;
>
> The documentation states that for `type=no-encrypt`, algorithm must not
> be specified at all. Where is that checked?
>
> --Ben
It's not currently checked, but should be.
I'll add it as shown above.
Thanks for the review,
Alison
Re: [PATCH, RFC 45/62] mm: Add the encrypt_mprotect() system call for MKTME
On Fri, Jun 14, 2019 at 01:51:37PM +0200, Peter Zijlstra wrote:
> On Wed, May 08, 2019 at 05:44:05PM +0300, Kirill A. Shutemov wrote:
snip
> > /*
> > - * When pkey==NO_KEY we get legacy mprotect behavior here.
> > + * do_mprotect_ext() supports the legacy mprotect behavior plus extensions
> > + * for Protection Keys and Memory Encryption Keys. These extensions are
> > + * mutually exclusive and the behavior is:
> > + * (pkey==NO_KEY && keyid==NO_KEY) ==> legacy mprotect
> > + * (pkey is valid) ==> legacy mprotect plus Protection Key extensions
> > + * (keyid is valid) ==> legacy mprotect plus Encryption Key extensions
> > */
> > static int do_mprotect_ext(unsigned long start, size_t len,
> > - unsigned long prot, int pkey)
> > + unsigned long prot, int pkey, int keyid)
> > {
snip
>
> I've missed the part where pkey && keyid results in a WARN or error or
> whatever.
>
I wasn't so sure about that since do_mprotect_ext()
is the call 'behind' the system calls.
legacy mprotect always calls with: NO_KEY, NO_KEY
pkey_mprotect always calls with: pkey, NO_KEY
encrypt_mprotect always calls with NO_KEY, keyid
Would a check on those arguments be debug only
to future proof this?
Re: [PATCH, RFC 47/62] mm: Restrict MKTME memory encryption to anonymous VMAs
On Fri, Jun 14, 2019 at 01:55:20PM +0200, Peter Zijlstra wrote: > On Wed, May 08, 2019 at 05:44:07PM +0300, Kirill A. Shutemov wrote: > > From: Alison Schofield > > > > Memory encryption is only supported for mappings that are ANONYMOUS. > > Test the VMA's in an encrypt_mprotect() request to make sure they all > > meet that requirement before encrypting any. > > > > The encrypt_mprotect syscall will return -EINVAL and will not encrypt > > any VMA's if this check fails. > > > > Signed-off-by: Alison Schofield > > Signed-off-by: Kirill A. Shutemov > > This should be folded back into the initial implemention, methinks. It is part of the initial implementation. I looked for places to split the implementation into smaller, reviewable patches, hence this split. None of it gets built until the CONFIG_X86_INTEL_MKTME is introduced in a later patch. The encrypt_mprotect() patchset is ordered like this: 1) generalize mprotect to support the mktme extension 2) wire up encrypt_mprotect() 3) implement encrypt_mprotect() 4) keep reference counts on encryption keys (was VMAs) 5) (this patch) restrict to anonymous VMAs. I thought Patch 5) was a small, but meaningful split. It accentuates the fact that MKTME is restricted to anonymous memory. Alas, I want to make it logical to review, so I'll move it.
Re: [PATCH, RFC 44/62] x86/mm: Set KeyIDs in encrypted VMAs for MKTME
On Fri, Jun 14, 2019 at 11:26:10AM -0700, Dave Hansen wrote: > On 6/14/19 10:33 AM, Alison Schofield wrote: > > Preserving the data across encryption key changes has not > > been a requirement. I'm not clear if it was ever considered > > and rejected. I believe that copying in order to preserve > > the data was never considered. > > We could preserve the data pretty easily. It's just annoying, though. > Right now, our only KeyID conversions happen in the page allocator. If > we were to convert in-place, we'd need something along the lines of: > > 1. Allocate a scratch page > 2. Unmap target page, or at least make it entirely read-only > 3. Copy plaintext into scratch page > 4. Do cache KeyID conversion of page being converted: > Flush caches, change page_ext metadata > 5. Copy plaintext back into target page from scratch area > 6. Re-establish PTEs with new KeyID Seems like the 'Copy plaintext' steps might disappoint the user, as much as the 'we don't preserve your data' design. Would users be happy w the plain text steps ? Alison > > #2 is *really* hard. It's similar to the problems that the poor > filesystem guys are having with RDMA these days when RDMA is doing writes. > > What we have here (destroying existing data) is certainly the _simplest_ > semantic. We can certainly give it a different name, or even non-PROT_* > semantics where it shares none of mprotect()'s functionality. > > Doesn't really matter to me at all.
Re: [PATCH, RFC 46/62] x86/mm: Keep reference counts on encrypted VMAs for MKTME
On Fri, Jun 14, 2019 at 01:54:24PM +0200, Peter Zijlstra wrote: > On Wed, May 08, 2019 at 05:44:06PM +0300, Kirill A. Shutemov wrote: > > From: Alison Schofield > > > > The MKTME (Multi-Key Total Memory Encryption) Key Service needs > > a reference count on encrypted VMAs. This reference count is used > > to determine when a hardware encryption KeyID is no longer in use > > and can be freed and reassigned to another Userspace Key. > > > > The MKTME Key service does the percpu_ref_init and _kill, so > > these gets/puts on encrypted VMA's can be considered the > > intermediaries in the lifetime of the key. > > > > Increment/decrement the reference count during encrypt_mprotect() > > system call for initial or updated encryption on a VMA. > > > > Piggy back on the vm_area_dup/free() helpers. If the VMAs being > > duplicated, or freed are encrypted, adjust the reference count. > > That all talks about VMAs, but... > > > @@ -102,6 +115,22 @@ void __prep_encrypted_page(struct page *page, int > > order, int keyid, bool zero) > > > > page++; > > } > > + > > + /* > > +* Make sure the KeyID cannot be freed until the last page that > > +* uses the KeyID is gone. > > +* > > +* This is required because the page may live longer than VMA it > > +* is mapped into (i.e. in get_user_pages() case) and having > > +* refcounting per-VMA is not enough. > > +* > > +* Taking a reference per-4K helps in case if the page will be > > +* split after the allocation. free_encrypted_page() will balance > > +* out the refcount even if the page was split and freed as bunch > > +* of 4K pages. > > +*/ > > + > > + percpu_ref_get_many(&encrypt_count[keyid], 1 << order); > > } snip > > counts pages, what gives? Yeah. Comments are confusing. We implemented the refcounting w VMA's in mind, and then added the page counting. I'll update the comments and dig around some more based on your overall concerns about the refcounting you mentioned in the cover letter.
Re: [PATCH, RFC 45/62] mm: Add the encrypt_mprotect() system call for MKTME
On Fri, Jun 14, 2019 at 01:47:32PM +0200, Peter Zijlstra wrote: > On Wed, May 08, 2019 at 05:44:05PM +0300, Kirill A. Shutemov wrote: > > diff --git a/fs/exec.c b/fs/exec.c > > index 2e0033348d8e..695c121b34b3 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -755,8 +755,8 @@ int setup_arg_pages(struct linux_binprm *bprm, > > vm_flags |= mm->def_flags; > > vm_flags |= VM_STACK_INCOMPLETE_SETUP; > > > > - ret = mprotect_fixup(vma, &prev, vma->vm_start, vma->vm_end, > > - vm_flags); > > + ret = mprotect_fixup(vma, &prev, vma->vm_start, vma->vm_end, vm_flags, > > +-1); > > You added a nice NO_KEY helper a few patches back, maybe use it? Sure, done. (I hesitated to define NO_KEY in mm.h initially. Put it there now. We'll see how that looks it next round.) > > > if (ret) > > goto out_unlock; > > BUG_ON(prev != vma);
Re: [PATCH, RFC 44/62] x86/mm: Set KeyIDs in encrypted VMAs for MKTME
On Fri, Jun 14, 2019 at 01:44:08PM +0200, Peter Zijlstra wrote: > On Wed, May 08, 2019 at 05:44:04PM +0300, Kirill A. Shutemov wrote: > > From: Alison Schofield > > > > MKTME architecture requires the KeyID to be placed in PTE bits 51:46. > > To create an encrypted VMA, place the KeyID in the upper bits of > > vm_page_prot that matches the position of those PTE bits. > > > > When the VMA is assigned a KeyID it is always considered a KeyID > > change. The VMA is either going from not encrypted to encrypted, > > or from encrypted with any KeyID to encrypted with any other KeyID. > > To make the change safely, remove the user pages held by the VMA > > and unlink the VMA's anonymous chain. > > This does not look like a transformation that preserves content; is > mprotect() still a suitable name? Data is not preserved across KeyID changes, by design. Background: We chose to implement encrypt_mprotect() as an extension to the legacy mprotect so that memory allocated in any method could be encrypted. ie. we didn't want to be tied to mmap. As an mprotect extension, encrypt_mprotect also supports the changing of access flags. The usage we suggest is: 1) alloc the memory w PROT_NONE to prevent any usage before encryption 2) use encrypt_mprotect() to add the key and change the access to PROT_WRITE|PROT_READ. Preserving the data across encryption key changes has not been a requirement. I'm not clear if it was ever considered and rejected. I believe that copying in order to preserve the data was never considered. Back to your naming question: Since it is an mprotect extension, it seems we need to keep the mprotect in the name. Thanks for bringing it up. It would be good to hear more thoughts on this.
Re: [PATCH, RFC 26/62] keys/mktme: Move the MKTME payload into a cache aligned structure
On Fri, Jun 14, 2019 at 01:35:23PM +0200, Peter Zijlstra wrote:
> On Wed, May 08, 2019 at 05:43:46PM +0300, Kirill A. Shutemov wrote:
>
> > +/* Copy the payload to the HW programming structure and program this KeyID
> > */
> > +static int mktme_program_keyid(int keyid, struct mktme_payload *payload)
> > +{
> > + struct mktme_key_program *kprog = NULL;
> > + int ret;
> > +
> > + kprog = kmem_cache_zalloc(mktme_prog_cache, GFP_ATOMIC);
>
> Why GFP_ATOMIC, afaict neither of the usage is with a spinlock held.
Got it. GFP_ATOMIC not needed.
That said, this is an artifact of reworking the locking, and that
locking may need to change again. If it does, will try to pre-allocate
rather than depend on GFP_ATOMIC here.
>
> > + if (!kprog)
> > + return -ENOMEM;
> > +
> > + /* Hardware programming requires cached aligned struct */
> > + kprog->keyid = keyid;
> > + kprog->keyid_ctrl = payload->keyid_ctrl;
> > + memcpy(kprog->key_field_1, payload->data_key, MKTME_AES_XTS_SIZE);
> > + memcpy(kprog->key_field_2, payload->tweak_key, MKTME_AES_XTS_SIZE);
> > +
> > + ret = MKTME_PROG_SUCCESS; /* Future programming call */
> > + kmem_cache_free(mktme_prog_cache, kprog);
> > + return ret;
> > +}
Re: [PATCH, RFC 00/62] Intel MKTME enabling
On Wed, May 29, 2019 at 10:30:07AM +0300, Mike Rapoport wrote: > On Wed, May 08, 2019 at 05:43:20PM +0300, Kirill A. Shutemov wrote: > > = Intro = > > > > The patchset brings enabling of Intel Multi-Key Total Memory Encryption. > > It consists of changes into multiple subsystems: > > > > * Core MM: infrastructure for allocation pages, dealing with encrypted VMAs > >and providing API setup encrypted mappings. > > * arch/x86: feature enumeration, program keys into hardware, setup > >page table entries for encrypted pages and more. > > * Key management service: setup and management of encryption keys. > > * DMA/IOMMU: dealing with encrypted memory on IO side. > > * KVM: interaction with virtualization side. > > * Documentation: description of APIs and usage examples. > > > > The patchset is huge. This submission aims to give view to the full picture > > and > > get feedback on the overall design. The patchset will be split into more > > digestible pieces later. > > > > Please review. Any feedback is welcome. > > It would be nice to have a brief usage description in cover letter rather > than in the last patches in the series ;-) > Thanks for making it all the way to the last patches in the set ;) Yes, we will certainly include that usage model in the cover letters of future patchsets. Alison
Re: [PATCH, RFC 57/62] x86/mktme: Overview of Multi-Key Total Memory Encryption
On Wed, May 29, 2019 at 10:21:48AM +0300, Mike Rapoport wrote: > On Wed, May 08, 2019 at 05:44:17PM +0300, Kirill A. Shutemov wrote: > > From: Alison Schofield > > > > Provide an overview of MKTME on Intel Platforms. > > > > Signed-off-by: Alison Schofield > > Signed-off-by: Kirill A. Shutemov > > --- > > Documentation/x86/mktme/index.rst | 8 +++ > > Documentation/x86/mktme/mktme_overview.rst | 57 ++ > > I'd expect addition of mktme docs to Documentation/x86/index.rst Got it. Thanks. Alison
Re: [PATCH, RFC 43/62] syscall/x86: Wire up a system call for MKTME encryption keys
On Wed, May 29, 2019 at 10:21:37AM +0300, Mike Rapoport wrote: > On Wed, May 08, 2019 at 05:44:03PM +0300, Kirill A. Shutemov wrote: > > From: Alison Schofield > > > > encrypt_mprotect() is a new system call to support memory encryption. > > > > It takes the same parameters as legacy mprotect, plus an additional > > key serial number that is mapped to an encryption keyid. > > Shouldn't this patch be after the encrypt_mprotect() is added? COND_SYSCALL(encrypt_mprotect) defined in kernel/sys_ni.c, allowed it to build in this order, but the order is not logical. Thanks for pointing it out. I will reorder the two patches. Alison
Re: [PATCH, RFC 48/62] selftests/x86/mktme: Test the MKTME APIs
Please ignore this patch. It includes an outdated draft from early testing. Other than showing our intent to deliver selftests, it is not out for review. Alison
Re: [PATCH] acpi/hmat: Update acpi_hmat_type enum with ACPI_HMAT_TYPE_PROXIMITY
On Thu, Apr 18, 2019 at 05:07:12PM +0200, Rafael J. Wysocki wrote: > On Thu, Apr 18, 2019 at 5:02 PM Keith Busch wrote: > > > > On Wed, Apr 17, 2019 at 11:13:10AM -0700, Alison Schofield wrote: > > > ACPI 6.3 changed the subtable "Memory Subsystem Address Range Structure" > > > to "Memory Proximity Domain Attributes Structure". > > > > > > Updating and renaming of the structure was included in commit: > > > ACPICA: ACPI 6.3: HMAT updates (9a8d961f1ef835b0d338fbe13da03cb424e87ae5) > > > > I was not really happy with that HMAT update. Platforms implementing > > 6.2's HMAT continue to exist even if 6.3 isn't backward compatible. We > > just lost the original subtable definition. > > Well, that's true, sadly, but the question is what to do in the kernel. > > Definitely, the 6.3 format needs to be supported, but if the 6.2 ships > anywhere in practice, that will need to be supported too. So, what's the usual practice when ACPI tables are updated? Do we define separate 6.2 and 6.3 versions of this subtable and let the kernel figure out which one its looking at?
[PATCH] acpi/hmat: Update acpi_hmat_type enum with ACPI_HMAT_TYPE_PROXIMITY
ACPI 6.3 changed the subtable "Memory Subsystem Address Range Structure"
to "Memory Proximity Domain Attributes Structure".
Updating and renaming of the structure was included in commit:
ACPICA: ACPI 6.3: HMAT updates (9a8d961f1ef835b0d338fbe13da03cb424e87ae5)
Rename the enum type to match the subtable and structure naming.
Signed-off-by: Alison Schofield
---
drivers/acpi/hmat/hmat.c | 4 ++--
include/acpi/actbl1.h| 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c
index b7824a0309f7..3e32120e2dab 100644
--- a/drivers/acpi/hmat/hmat.c
+++ b/drivers/acpi/hmat/hmat.c
@@ -411,7 +411,7 @@ static int __init hmat_parse_subtable(union
acpi_subtable_headers *header,
return -EINVAL;
switch (hdr->type) {
- case ACPI_HMAT_TYPE_ADDRESS_RANGE:
+ case ACPI_HMAT_TYPE_PROXIMITY:
return hmat_parse_proximity_domain(header, end);
case ACPI_HMAT_TYPE_LOCALITY:
return hmat_parse_locality(header, end);
@@ -649,7 +649,7 @@ static __init int hmat_init(void)
goto out_put;
}
- for (i = ACPI_HMAT_TYPE_ADDRESS_RANGE; i < ACPI_HMAT_TYPE_RESERVED;
i++) {
+ for (i = ACPI_HMAT_TYPE_PROXIMITY; i < ACPI_HMAT_TYPE_RESERVED; i++) {
if (acpi_table_parse_entries(ACPI_SIG_HMAT,
sizeof(struct acpi_table_hmat), i,
hmat_parse_subtable, 0) < 0) {
diff --git a/include/acpi/actbl1.h b/include/acpi/actbl1.h
index d14037ddf108..22c039ebc6c5 100644
--- a/include/acpi/actbl1.h
+++ b/include/acpi/actbl1.h
@@ -1395,7 +1395,7 @@ struct acpi_table_hmat {
/* Values for HMAT structure types */
enum acpi_hmat_type {
- ACPI_HMAT_TYPE_ADDRESS_RANGE = 0, /* Memory subsystem address
range */
+ ACPI_HMAT_TYPE_PROXIMITY = 0, /* Memory proximity domain attributes */
ACPI_HMAT_TYPE_LOCALITY = 1,/* System locality latency and
bandwidth information */
ACPI_HMAT_TYPE_CACHE = 2, /* Memory side cache information */
ACPI_HMAT_TYPE_RESERVED = 3 /* 3 and greater are reserved */
--
2.14.1
[PATCH] selftests/vm/gup_benchmark.c: match gup struct to kernel
An expansion field was added to the kernel copy of this structure for
future use. See mm/gup_benchmark.c.
Add the same expansion field here, so that the IOCTL command decodes
correctly. Otherwise, it fails with EINVAL.
Signed-off-by: Alison Schofield
---
tools/testing/selftests/vm/gup_benchmark.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/vm/gup_benchmark.c
b/tools/testing/selftests/vm/gup_benchmark.c
index 880b96fc80d4..c0534e298b51 100644
--- a/tools/testing/selftests/vm/gup_benchmark.c
+++ b/tools/testing/selftests/vm/gup_benchmark.c
@@ -25,6 +25,7 @@ struct gup_benchmark {
__u64 size;
__u32 nr_pages_per_call;
__u32 flags;
+ __u64 expansion[10];/* For future use */
};
int main(int argc, char **argv)
--
2.14.1
Re: [PATCHv5 10/19] x86/mm: Implement page_keyid() using page_ext
On Mon, Jul 23, 2018 at 12:45:17PM +0300, Kirill A. Shutemov wrote: > On Wed, Jul 18, 2018 at 04:38:02PM -0700, Dave Hansen wrote: > > On 07/17/2018 04:20 AM, Kirill A. Shutemov wrote: > > > Store KeyID in bits 31:16 of extended page flags. These bits are unused. > > > > I'd love a two sentence remind of what page_ext is and why you chose to > > use it. Yes, you need this. No, not everybody that you want to review > > this patch set knows what it is or why you chose it. > > Okay. > > > > page_keyid() returns zero until page_ext is ready. > > > > Is there any implication of this? Or does it not matter because we > > don't run userspace until after page_ext initialization is done? > > It matters in sense that we shouldn't reference page_ext before it's > initialized otherwise we will get garbage and crash. > > > > page_ext initializer enables static branch to indicate that > > > > "enables a static branch" > > > > > page_keyid() can use page_ext. The same static branch will gate MKTME > > > readiness in general. > > > > Can you elaborate on this a bit? It would also be a nice place to hint > > to the folks working hard on the APIs to ensure she checks this. > > Okay. At API init time we can check if (MKTME_ENABLED && mktme_nr_keyids > 0) Sounds like this is another dependency we need to check and 'wait' on? It happens after MKTME_ENABLED is set? Let me know. > > > > We don't yet set KeyID for the page. It will come in the following > > > patch that implements prep_encrypted_page(). All pages have KeyID-0 for > > > now. > > > > It also wouldn't hurt to mention why you don't use an X86_FEATURE_* for > > this rather than an explicit static branch. I'm sure the x86 > > maintainers will be curious. > > Sure. > > -- > Kirill A. Shutemov
[tip:x86/urgent] x86,sched: Allow topologies where NUMA nodes share an LLC
Commit-ID: 1340ccfa9a9afefdbab90d7935d4ed19817e37c2
Gitweb: https://git.kernel.org/tip/1340ccfa9a9afefdbab90d7935d4ed19817e37c2
Author: Alison Schofield
AuthorDate: Fri, 6 Apr 2018 17:21:30 -0700
Committer: Thomas Gleixner
CommitDate: Tue, 17 Apr 2018 15:39:55 +0200
x86,sched: Allow topologies where NUMA nodes share an LLC
Intel's Skylake Server CPUs have a different LLC topology than previous
generations. When in Sub-NUMA-Clustering (SNC) mode, the package is divided
into two "slices", each containing half the cores, half the LLC, and one
memory controller and each slice is enumerated to Linux as a NUMA
node. This is similar to how the cores and LLC were arranged for the
Cluster-On-Die (CoD) feature.
CoD allowed the same cache line to be present in each half of the LLC.
But, with SNC, each line is only ever present in *one* slice. This means
that the portion of the LLC *available* to a CPU depends on the data being
accessed:
Remote socket: entire package LLC is shared
Local socket->local slice: data goes into local slice LLC
Local socket->remote slice: data goes into remote-slice LLC. Slightly
higher latency than local slice LLC.
The biggest implication from this is that a process accessing all
NUMA-local memory only sees half the LLC capacity.
The CPU describes its cache hierarchy with the CPUID instruction. One of
the CPUID leaves enumerates the "logical processors sharing this
cache". This information is used for scheduling decisions so that tasks
move more freely between CPUs sharing the cache.
But, the CPUID for the SNC configuration discussed above enumerates the LLC
as being shared by the entire package. This is not 100% precise because the
entire cache is not usable by all accesses. But, it *is* the way the
hardware enumerates itself, and this is not likely to change.
The userspace visible impact of all the above is that the sysfs info
reports the entire LLC as being available to the entire package. As noted
above, this is not true for local socket accesses. This patch does not
correct the sysfs info. It is the same, pre and post patch.
The current code emits the following warning:
sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0].
Ignoring dependency.
The warning is coming from the topology_sane() check in smpboot.c because
the topology is not matching the expectations of the model for obvious
reasons.
To fix this, add a vendor and model specific check to never call
topology_sane() for these systems. Also, just like "Cluster-on-Die" disable
the "coregroup" sched_domain_topology_level and use NUMA information from
the SRAT alone.
This is OK at least on the hardware we are immediately concerned about
because the LLC sharing happens at both the slice and at the package level,
which are also NUMA boundaries.
Signed-off-by: Alison Schofield
Signed-off-by: Thomas Gleixner
Reviewed-by: Borislav Petkov
Cc: Prarit Bhargava
Cc: Tony Luck
Cc: Peter Zijlstra (Intel)
Cc: brice.gog...@gmail.com
Cc: Dave Hansen
Cc: Borislav Petkov
Cc: David Rientjes
Cc: Igor Mammedov
Cc: "H. Peter Anvin"
Cc: Tim Chen
Link: https://lkml.kernel.org/r/20180407002130.ga18...@alison-desk.jf.intel.com
---
arch/x86/kernel/smpboot.c | 45 -
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ff99e2b6fc54..45175b81dd5b 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,8 @@
#include
#include
#include
+#include
+#include
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -390,15 +392,47 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
return false;
}
+/*
+ * Define snc_cpu[] for SNC (Sub-NUMA Cluster) CPUs.
+ *
+ * These are Intel CPUs that enumerate an LLC that is shared by
+ * multiple NUMA nodes. The LLC on these systems is shared for
+ * off-package data access but private to the NUMA node (half
+ * of the package) for on-package access.
+ *
+ * CPUID (the source of the information about the LLC) can only
+ * enumerate the cache as being shared *or* unshared, but not
+ * this particular configuration. The CPU in this case enumerates
+ * the cache to be shared across the entire package (spanning both
+ * NUMA nodes).
+ */
+
+static const struct x86_cpu_id snc_cpu[] = {
+ { X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_X },
+ {}
+};
+
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
- if (per_cpu(cpu_llc_id, cpu1) != BAD_APICID &&
- per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2))
- return topology_sane(c, o, "llc");
+ /* Do not match if we do not have a valid APICID for cpu: */
+ if (per
[PATCH v5] x86,sched: allow topologies where NUMA nodes share an LLC
From: Alison Schofield Intel's Skylake Server CPUs have a different LLC topology than previous generations. When in Sub-NUMA-Clustering (SNC) mode, the package is divided into two "slices", each containing half the cores, half the LLC, and one memory controller and each slice is enumerated to Linux as a NUMA node. This is similar to how the cores and LLC were arranged for the Cluster-On-Die (CoD) feature. CoD allowed the same cache line to be present in each half of the LLC. But, with SNC, each line is only ever present in *one* slice. This means that the portion of the LLC *available* to a CPU depends on the data being accessed: Remote socket: entire package LLC is shared Local socket->local slice: data goes into local slice LLC Local socket->remote slice: data goes into remote-slice LLC. Slightly higher latency than local slice LLC. The biggest implication from this is that a process accessing all NUMA-local memory only sees half the LLC capacity. The CPU describes its cache hierarchy with the CPUID instruction. One of the CPUID leaves enumerates the "logical processors sharing this cache". This information is used for scheduling decisions so that tasks move more freely between CPUs sharing the cache. But, the CPUID for the SNC configuration discussed above enumerates the LLC as being shared by the entire package. This is not 100% precise because the entire cache is not usable by all accesses. But, it *is* the way the hardware enumerates itself, and this is not likely to change. The userspace visible impact of all the above is that the sysfs info reports the entire LLC as being available to the entire package. As noted above, this is not true for local socket accesses. This patch does not correct the sysfs info. It is the same, pre and post patch. This patch continues to allow this SNC topology and it does so without complaint. It eliminates a warning that looks like this: sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0]. Ignoring dependency. The warning is coming from the sane_topology check() in smpboot.c. To fix this, add a vendor and model specific check to never call topology_sane() for these systems. Also, just like "Cluster-on-Die" we throw out the "coregroup" sched_domain_topology_level and use NUMA information from the SRAT alone. This is OK at least on the hardware we are immediately concerned about because the LLC sharing happens at both the slice and at the package level, which are also NUMA boundaries. Signed-off-by: Alison Schofield Cc: Dave Hansen Cc: Tony Luck Cc: Tim Chen Cc: "H. Peter Anvin" Cc: Borislav Petkov Cc: Peter Zijlstra (Intel) Cc: David Rientjes Cc: Igor Mammedov Cc: Prarit Bhargava Cc: brice.gog...@gmail.com Cc: Ingo Molnar --- Changes in v5: * Removed the redundant setting of boolean x86_has_numa_in_package. Related comments were put back in their pre-patch locations. Changes in v4: * Added this to the patch description above: The userspace visible impact of all the above is that the sysfs info reports the entire LLC as being available to the entire package. As noted above, this is not true for local socket accesses. This patch does not correct the sysfs info. It is the same, pre and post patch. This patch continues to allow this SNC topology and it does so without complaint. It eliminates a warning that looks like this: * Changed the code comment per PeterZ/DaveH discussion wrt bypassing that topology_sane() check in match_llc() /* * false means 'c' does not share the LLC of 'o'. * Note: this decision gets reflected all the way * out to userspace */ This message hopes to clarify what happens when we return false. Note that returning false is *not* new to this patch. Without this patch we always returned false - with a warning. This avoids that warning and returns false directly. * Remove __initconst from snc_cpu[] declaration that I had added in v3. This is not an init time only path. * Do not deal with the wrong sysfs info. It was wrong before this patch and it will be the exact same 'wrongness' after this patch. We can address the sysfs reporting separately. Here are some options: 1) Change the way the LLC-size is reported. Enumerate two separate, half-sized LLCs shared only by the slice when SNC mode is on. 2) Do not export the sysfs info that is wrong. Prevents userspace from making bad decisions based on inaccurate info. Changes in v3: * Use x86_match_cpu() for vendor & model check and moved related comments to the array define. (Still just one system model) * Updated the comments surrounding the topology_sane() check. Changes in v2: * Add vendor check (Intel) where we only had a model check (Skylake_X). Considered the sugg
Re: [PATCH v4] x86,sched: allow topologies where NUMA nodes share an LLC
On Wed, Apr 04, 2018 at 12:00:45PM -0700, Alison Schofield wrote:
> On Wed, Apr 04, 2018 at 11:42:11AM -0700, Tim Chen wrote:
> > On 04/04/2018 10:38 AM, Alison Schofield wrote:
> > > On Wed, Apr 04, 2018 at 10:24:49AM -0700, Tim Chen wrote:
> > >> On 04/03/2018 02:12 PM, Alison Schofield wrote:
> > >>
> > >>> +
> > >>> + /*
> > >>> +* topology_sane() considers LLCs that span NUMA nodes to be
> > >>> +* insane and will display a warning message. Bypass the call
> > >>> +* to topology_sane() for snc_cpu's to avoid that warning.
> > >>> +*/
> > >>> +
> > >>> + if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu)) {
> > >>> + /* Indicate that package has NUMA nodes inside: */
> > >>> + x86_has_numa_in_package = true;
> > >>
> > >> Why does the x86_has_numa_in_package has to be set here when it would
> > >> have
> > >> been done later in set_cpu_sibling_map?
> > >
> > > Tim,
> > > I had that same thought when you commented on it previously. After
> > > discussing w DaveH, decided that match_llc() and match_die(c,0)
> > > could be different and chose to be (cautiously) redundant.
> > > alisons
> >
> > If it is redundant, I suggest it be removed, and only added if
> > there is truly a case where the current logic
> >
> > if (match_die(c, o) && !topology_same_node(c, o))
> > x86_has_numa_in_package = true;
> >
> > fails. And also the modification of this logic should be at the
> > same place for easy code maintenance.
>
> That makes good sense. I'll look to define the difference or remove
> the redundancy.
>
> alisons
I found not reason for the redundancy via experimentation w my Skylake,
nor through code examination. I've removed it in v5. I'll see if
anyone claims theoretical case.
alisons
>
> >
> > Tim
> >
> > >
> > >
> > >
> > >>
> > >>> +
> > >>> + /*
> > >>> +* false means 'c' does not share the LLC of 'o'.
> > >>> +* Note: this decision gets reflected all the way
> > >>> +* out to userspace.
> > >>> +*/
> > >>> +
> > >>> + return false;
> > >>
> > >> Thanks.
> > >>
> > >> Tim
> >
Re: [PATCH v4] x86,sched: allow topologies where NUMA nodes share an LLC
On Wed, Apr 04, 2018 at 11:42:11AM -0700, Tim Chen wrote:
> On 04/04/2018 10:38 AM, Alison Schofield wrote:
> > On Wed, Apr 04, 2018 at 10:24:49AM -0700, Tim Chen wrote:
> >> On 04/03/2018 02:12 PM, Alison Schofield wrote:
> >>
> >>> +
> >>> + /*
> >>> + * topology_sane() considers LLCs that span NUMA nodes to be
> >>> + * insane and will display a warning message. Bypass the call
> >>> + * to topology_sane() for snc_cpu's to avoid that warning.
> >>> + */
> >>> +
> >>> + if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu)) {
> >>> + /* Indicate that package has NUMA nodes inside: */
> >>> + x86_has_numa_in_package = true;
> >>
> >> Why does the x86_has_numa_in_package has to be set here when it would have
> >> been done later in set_cpu_sibling_map?
> >
> > Tim,
> > I had that same thought when you commented on it previously. After
> > discussing w DaveH, decided that match_llc() and match_die(c,0)
> > could be different and chose to be (cautiously) redundant.
> > alisons
>
> If it is redundant, I suggest it be removed, and only added if
> there is truly a case where the current logic
>
> if (match_die(c, o) && !topology_same_node(c, o))
> x86_has_numa_in_package = true;
>
> fails. And also the modification of this logic should be at the
> same place for easy code maintenance.
That makes good sense. I'll look to define the difference or remove
the redundancy.
alisons
>
> Tim
>
> >
> >
> >
> >>
> >>> +
> >>> + /*
> >>> + * false means 'c' does not share the LLC of 'o'.
> >>> + * Note: this decision gets reflected all the way
> >>> + * out to userspace.
> >>> + */
> >>> +
> >>> + return false;
> >>
> >> Thanks.
> >>
> >> Tim
>
Re: [PATCH v4] x86,sched: allow topologies where NUMA nodes share an LLC
On Wed, Apr 04, 2018 at 10:24:49AM -0700, Tim Chen wrote:
> On 04/03/2018 02:12 PM, Alison Schofield wrote:
>
> > +
> > + /*
> > +* topology_sane() considers LLCs that span NUMA nodes to be
> > +* insane and will display a warning message. Bypass the call
> > +* to topology_sane() for snc_cpu's to avoid that warning.
> > +*/
> > +
> > + if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu)) {
> > + /* Indicate that package has NUMA nodes inside: */
> > + x86_has_numa_in_package = true;
>
> Why does the x86_has_numa_in_package has to be set here when it would have
> been done later in set_cpu_sibling_map?
Tim,
I had that same thought when you commented on it previously. After
discussing w DaveH, decided that match_llc() and match_die(c,0)
could be different and chose to be (cautiously) redundant.
alisons
>
> > +
> > + /*
> > +* false means 'c' does not share the LLC of 'o'.
> > +* Note: this decision gets reflected all the way
> > +* out to userspace.
> > +*/
> > +
> > + return false;
>
> Thanks.
>
> Tim
[PATCH v4] x86,sched: allow topologies where NUMA nodes share an LLC
From: Alison Schofield Intel's Skylake Server CPUs have a different LLC topology than previous generations. When in Sub-NUMA-Clustering (SNC) mode, the package is divided into two "slices", each containing half the cores, half the LLC, and one memory controller and each slice is enumerated to Linux as a NUMA node. This is similar to how the cores and LLC were arranged for the Cluster-On-Die (CoD) feature. CoD allowed the same cache line to be present in each half of the LLC. But, with SNC, each line is only ever present in *one* slice. This means that the portion of the LLC *available* to a CPU depends on the data being accessed: Remote socket: entire package LLC is shared Local socket->local slice: data goes into local slice LLC Local socket->remote slice: data goes into remote-slice LLC. Slightly higher latency than local slice LLC. The biggest implication from this is that a process accessing all NUMA-local memory only sees half the LLC capacity. The CPU describes its cache hierarchy with the CPUID instruction. One of the CPUID leaves enumerates the "logical processors sharing this cache". This information is used for scheduling decisions so that tasks move more freely between CPUs sharing the cache. But, the CPUID for the SNC configuration discussed above enumerates the LLC as being shared by the entire package. This is not 100% precise because the entire cache is not usable by all accesses. But, it *is* the way the hardware enumerates itself, and this is not likely to change. The userspace visible impact of all the above is that the sysfs info reports the entire LLC as being available to the entire package. As noted above, this is not true for local socket accesses. This patch does not correct the sysfs info. It is the same, pre and post patch. This patch continues to allow this SNC topology and it does so without complaint. It eliminates a warning that looks like this: sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != 0]. Ignoring dependency. The warning is coming from the sane_topology check() in smpboot.c. To fix this, add a vendor and model specific check to never call topology_sane() for these systems. Also, just like "Cluster-on-Die" we throw out the "coregroup" sched_domain_topology_level and use NUMA information from the SRAT alone. This is OK at least on the hardware we are immediately concerned about because the LLC sharing happens at both the slice and at the package level, which are also NUMA boundaries. Signed-off-by: Alison Schofield Cc: Dave Hansen Cc: Tony Luck Cc: Tim Chen Cc: "H. Peter Anvin" Cc: Borislav Petkov Cc: Peter Zijlstra (Intel) Cc: David Rientjes Cc: Igor Mammedov Cc: Prarit Bhargava Cc: brice.gog...@gmail.com Cc: Ingo Molnar --- Changes in v4: * Added this to the patch description above: The userspace visible impact of all the above is that the sysfs info reports the entire LLC as being available to the entire package. As noted above, this is not true for local socket accesses. This patch does not correct the sysfs info. It is the same, pre and post patch. This patch continues to allow this SNC topology and it does so without complaint. It eliminates a warning that looks like this: * Changed the code comment per PeterZ/DaveH discussion wrt bypassing that topology_sane() check in match_llc() /* * false means 'c' does not share the LLC of 'o'. * Note: this decision gets reflected all the way * out to userspace */ This message hopes to clarify what happens when we return false. Note that returning false is *not* new to this patch. Without this patch we always returned false - with a warning. This avoids that warning and returns false directly. * Remove __initconst from snc_cpu[] declaration that I had added in v3. This is not an init time only path. * Do not deal with the wrong sysfs info. It was wrong before this patch and it will be the exact same 'wrongness' after this patch. We can address the sysfs reporting separately. Here are some options: 1) Change the way the LLC-size is reported. Enumerate two separate, half-sized LLCs shared only by the slice when SNC mode is on. 2) Do not export the sysfs info that is wrong. Prevents userspace from making bad decisions based on inaccurate info. Changes in v3: * Use x86_match_cpu() for vendor & model check and moved related comments to the array define. (Still just one system model) * Updated the comments surrounding the topology_sane() check. Changes in v2: * Add vendor check (Intel) where we only had a model check (Skylake_X). Considered the suggestion of adding a new flag here but thought that to be overkill for this usage. * Return false, instead of true, from match_llc() per reviewer
Re: [PATCH v3] x86,sched: allow topologies where NUMA nodes share an LLC
On Wed, Mar 28, 2018 at 05:00:24PM -0700, Alison Schofield wrote: > From: Alison Schofield > > Intel's Skylake Server CPUs have a different LLC topology than previous > generations. When in Sub-NUMA-Clustering (SNC) mode, the package is > divided into two "slices", each containing half the cores, half the LLC, > and one memory controller and each slice is enumerated to Linux as a > NUMA node. This is similar to how the cores and LLC were arranged > for the Cluster-On-Die (CoD) feature. > > CoD allowed the same cache line to be present in each half of the LLC. > But, with SNC, each line is only ever present in *one* slice. This > means that the portion of the LLC *available* to a CPU depends on the > data being accessed: > > Remote socket: entire package LLC is shared > Local socket->local slice: data goes into local slice LLC > Local socket->remote slice: data goes into remote-slice LLC. Slightly > higher latency than local slice LLC. > > The biggest implication from this is that a process accessing all > NUMA-local memory only sees half the LLC capacity. > > The CPU describes its cache hierarchy with the CPUID instruction. One > of the CPUID leaves enumerates the "logical processors sharing this > cache". This information is used for scheduling decisions so that tasks > move more freely between CPUs sharing the cache. > > But, the CPUID for the SNC configuration discussed above enumerates > the LLC as being shared by the entire package. This is not 100% > precise because the entire cache is not usable by all accesses. But, > it *is* the way the hardware enumerates itself, and this is not likely > to change. > > This breaks the sane_topology() check in the smpboot.c code because > this topology is considered not-sane. To fix this, add a vendor and > model specific check to never call topology_sane() for these systems. > Also, just like "Cluster-on-Die" we throw out the "coregroup" > sched_domain_topology_level and use NUMA information from the SRAT > alone. > > This is OK at least on the hardware we are immediately concerned about > because the LLC sharing happens at both the slice and at the package > level, which are also NUMA boundaries. > > This patch eliminates a warning that looks like this: > > sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 != > 0]. Ignoring dependency. > Let's see if I'm getting a better grasp of this: My goal here is to suppress that WARNING message from topology_sane(). (We have a customer who is seeing the WARNING and would like it to go away) The sysfs exported info for SNC's is 'not precise'. It reports the entire LLC as available. This 'non precise' data existed before this patch and exists after this patch. This is a problem, agreed. PeterZ: At first I thought you were saying that this patch itself broke the sysfs info. I experimented with that and found no differences in sysfs info before/after the patch and with SNC on/off. That makes me think you are saying that we should not say this topology is 'Allowed' when the sysfs data is wrong. (ie. That WARNING serves a purpose) If you did indeed mean that the patch breaks the sysfs data, please point me real close! ie. How does it change the cache-mask as exposed to userspace? All: Here are 3 alternatives: 1) Keep patch code basically the same and improve the comments & commit, being very explicit about the sysfs info issue. 2) Change the way the LLC-size is reported. Enumerate "two separate, half-sized LLCs shared only by the slice when SNC mode is on." 3) Do not export the sysfs info that is wrong. Userspace cannot make bad decisions based upon it. Can we do 1) now and then follow with 2) or 3)? Thanks for all the reviews/comments, alisons
[PATCH v3] x86,sched: allow topologies where NUMA nodes share an LLC
From: Alison Schofield
Intel's Skylake Server CPUs have a different LLC topology than previous
generations. When in Sub-NUMA-Clustering (SNC) mode, the package is
divided into two "slices", each containing half the cores, half the LLC,
and one memory controller and each slice is enumerated to Linux as a
NUMA node. This is similar to how the cores and LLC were arranged
for the Cluster-On-Die (CoD) feature.
CoD allowed the same cache line to be present in each half of the LLC.
But, with SNC, each line is only ever present in *one* slice. This
means that the portion of the LLC *available* to a CPU depends on the
data being accessed:
Remote socket: entire package LLC is shared
Local socket->local slice: data goes into local slice LLC
Local socket->remote slice: data goes into remote-slice LLC. Slightly
higher latency than local slice LLC.
The biggest implication from this is that a process accessing all
NUMA-local memory only sees half the LLC capacity.
The CPU describes its cache hierarchy with the CPUID instruction. One
of the CPUID leaves enumerates the "logical processors sharing this
cache". This information is used for scheduling decisions so that tasks
move more freely between CPUs sharing the cache.
But, the CPUID for the SNC configuration discussed above enumerates
the LLC as being shared by the entire package. This is not 100%
precise because the entire cache is not usable by all accesses. But,
it *is* the way the hardware enumerates itself, and this is not likely
to change.
This breaks the sane_topology() check in the smpboot.c code because
this topology is considered not-sane. To fix this, add a vendor and
model specific check to never call topology_sane() for these systems.
Also, just like "Cluster-on-Die" we throw out the "coregroup"
sched_domain_topology_level and use NUMA information from the SRAT
alone.
This is OK at least on the hardware we are immediately concerned about
because the LLC sharing happens at both the slice and at the package
level, which are also NUMA boundaries.
This patch eliminates a warning that looks like this:
sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 !=
0]. Ignoring dependency.
Signed-off-by: Alison Schofield
Cc: Dave Hansen
Cc: Luck, Tony
Cc: Tim Chen
Cc: "H. Peter Anvin"
Cc: Borislav Petkov
Cc: Peter Zijlstra (Intel)
Cc: David Rientjes
Cc: Igor Mammedov
Cc: Prarit Bhargava
Cc: brice.gog...@gmail.com
Cc: Ingo Molnar
---
Changes in v3:
* Use x86_match_cpu() for vendor & model check and moved related
comments to the array define. (Still just one system model)
* Updated the comments surrounding the topology_sane() check.
Changes in v2:
* Add vendor check (Intel) where we only had a model check (Skylake_X).
Considered the suggestion of adding a new flag here but thought that
to be overkill for this usage.
* Return false, instead of true, from match_llc() per reviewer suggestion.
That also cleaned up a topology broken bug message in sched_domain().
* Updated the comments around the match_llc() return value, continuing to
note that the return value doesn't actually matter because we are throwing
away coregroups for scheduling.
* Changed submitter. I'm following up on this patch originally submitted
by Dave Hansen.
arch/x86/kernel/smpboot.c | 62 +++
1 file changed, 52 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ff99e2b..92f0fe0 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,8 @@
#include
#include
#include
+#include
+#include
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -390,15 +392,61 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
return false;
}
+/*
+ * Set if a package/die has multiple NUMA nodes inside.
+ * AMD Magny-Cours, Intel Cluster-on-Die, and Intel
+ * Sub-NUMA Clustering have this.
+ */
+
+static bool x86_has_numa_in_package;
+
+/*
+ * Define SNC (Sub-NUMA Cluster) CPUs.
+ *
+ * These are Intel CPUs that enumerate an LLC that is shared by
+ * multiple NUMA nodes. The LLC on these systems is shared for
+ * off-package data access but private to the NUMA node (half
+ * of the package) for on-package access.
+ *
+ * CPUID (the source of the information about the LLC) can only
+ * enumerate the cache as being shared *or* unshared, but not
+ * this particular configuration. The CPU in this case enumerates
+ * the cache to be shared across the entire package (spanning both
+ * NUMA nodes).
+ */
+
+static const struct x86_cpu_id snc_cpu[] __initconst = {
+ { X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_X },
+ {}
+};
+
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
int cpu1 = c->cp
Re: [PATCH v2] x86,sched: allow topologies where NUMA nodes share an LLC
On Thu, Mar 22, 2018 at 04:30:29PM -0700, Luck, Tony wrote:
> On Thu, Mar 22, 2018 at 01:49:22PM -0700, Alison Schofield wrote:
> > + if (!topology_same_node(c, o) &&
> > + (c->x86_vendor == X86_VENDOR_INTEL &&
> > +c->x86_model == INTEL_FAM6_SKYLAKE_X)) {
>
> Maybe make life easier in the future to add more models
> to the list by using x86_match_cpu() here?
>
> -Tony
Tony -
Am I on the right track below?
Define like this:
static const __initconst struct x86_cpu_id snc_cpu[] = {
{ X86_VENDOR_INTEL, 6, INTEL_FAM6_SKYLAKE_X },
{}
};
Use like this:
if (!topology_same_node(c, o) && x86_match_cpu(snc_cpu)) {
Thanks,
alisons
Re: [PATCH v2] x86,sched: allow topologies where NUMA nodes share an LLC
On Thu, Mar 22, 2018 at 05:42:41PM -0700, Tim Chen wrote:
> On 03/22/2018 01:49 PM, Alison Schofield wrote:
> >
> > +*/
> > + if (!topology_same_node(c, o) &&
> > + (c->x86_vendor == X86_VENDOR_INTEL &&
> > +c->x86_model == INTEL_FAM6_SKYLAKE_X)) {
> > + /* Use NUMA instead of coregroups for scheduling: */
> > + x86_has_numa_in_package = true;
>
> x86_has_numa_in_package will only be set true for SKYLAKE in the above?
>
> This boolean probably should be set for (!topology_same_node(c, o) &&
> match_die(c, o)) and not
> dependent on cpu family. Only the return value should depend on cpu family.
>
> Tim
Tim,
x86_has_numa_in_package is being set in set_cpu_sibling_map() with the
same criteria you describe: (!topology_same_node(c, o) && match_die(c,
o))
Skylake in SNC mode takes that path and it gets set correctly, so I'm
thinking and seeing that the setting of it in match_llc() is redundant.
The intent of the patch is to skip the topology_sane() check to avoid
the warning message it spits out at boot time.
This has me wondering if, aside from it being redundant, if it may be
incorrect? Should we set that boolean based on vendor/model alone in
match_llc(). I guess I need to understand what happens w a Skylake that
doesn't have SNC turned on. I can try that configuration next.
Thanks for reviewing!
alisons
>
>
> > +
> > + /*
> > +* Return value doesn't actually matter because we
> > +* are throwing away coregroups for scheduling anyway.
> > +* Return false to bypass topology broken bug messages
> > +* and fixups in sched_domain().
> > +*/
> > + return false;
> > + }
> > +
> > + return topology_sane(c, o, "llc");
> > }
> >
> > /*
> > @@ -454,12 +492,6 @@ static struct sched_domain_topology_level
> > x86_topology[] = {
> > { NULL, },
> > };
> >
> > -/*
> > - * Set if a package/die has multiple NUMA nodes inside.
> > - * AMD Magny-Cours and Intel Cluster-on-Die have this.
> > - */
> > -static bool x86_has_numa_in_package;
> > -
> > void set_cpu_sibling_map(int cpu)
> > {
> > bool has_smt = smp_num_siblings > 1;
> >
>
[PATCH v2] x86,sched: allow topologies where NUMA nodes share an LLC
From: Alison Schofield
Intel's Skylake Server CPUs have a different LLC topology than previous
generations. When in Sub-NUMA-Clustering (SNC) mode, the package is
divided into two "slices", each containing half the cores, half the LLC,
and one memory controller and each slice is enumerated to Linux as a
NUMA node. This is similar to how the cores and LLC were arranged
for the Cluster-On-Die (CoD) feature.
CoD allowed the same cache line to be present in each half of the LLC.
But, with SNC, each line is only ever present in *one* slice. This
means that the portion of the LLC *available* to a CPU depends on the
data being accessed:
Remote socket: entire package LLC is shared
Local socket->local slice: data goes into local slice LLC
Local socket->remote slice: data goes into remote-slice LLC. Slightly
higher latency than local slice LLC.
The biggest implication from this is that a process accessing all
NUMA-local memory only sees half the LLC capacity.
The CPU describes its cache hierarchy with the CPUID instruction. One
of the CPUID leaves enumerates the "logical processors sharing this
cache". This information is used for scheduling decisions so that tasks
move more freely between CPUs sharing the cache.
But, the CPUID for the SNC configuration discussed above enumerates
the LLC as being shared by the entire package. This is not 100%
precise because the entire cache is not usable by all accesses. But,
it *is* the way the hardware enumerates itself, and this is not likely
to change.
This breaks the sane_topology() check in the smpboot.c code because
this topology is considered not-sane. To fix this, add a vendor and
model specifc check to never call topology_sane() for these systems.
Also, just like "Cluster-on-Die" we throw out the "coregroup"
sched_domain_topology_level and use NUMA information from the SRAT
alone.
This is OK at least on the hardware we are immediately concerned about
because the LLC sharing happens at both the slice and at the package
level, which are also NUMA boundaries.
This patch eliminates a warning that looks like this:
sched: CPU #3's llc-sibling CPU #0 is not on the same node! [node: 1 !=
0]. Ignoring dependency.
Signed-off-by: Alison Schofield
Cc: Dave Hansen
Cc: Luck, Tony
Cc: Tim Chen
Cc: "H. Peter Anvin"
Cc: Borislav Petkov
Cc: Peter Zijlstra (Intel)
Cc: David Rientjes
Cc: Igor Mammedov
Cc: Prarit Bhargava
Cc: brice.gog...@gmail.com
Cc: Ingo Molnar
---
Changes in v2:
* Add vendor check (Intel) where we only had a model check (Skylake_X).
Considered the suggestion of adding a new flag here but thought that
to be overkill for this usage.
* Return false, instead of true, from match_llc() per reviewer suggestion.
That also cleaned up a topology broken bug message in sched_domain().
* Updated the comments around the match_llc() return value, continuing to
note that the return value doesn't actually matter because we are throwing
away coregroups for scheduling.
* Changed submitter. I'm following up on this patch originally submitted
by Dave Hansen
arch/x86/kernel/smpboot.c | 52 ++-
1 file changed, 42 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index ff99e2b..cffd181 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -77,6 +77,7 @@
#include
#include
#include
+#include
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
@@ -390,15 +391,52 @@ static bool match_smt(struct cpuinfo_x86 *c, struct
cpuinfo_x86 *o)
return false;
}
+/*
+ * Set if a package/die has multiple NUMA nodes inside.
+ * AMD Magny-Cours, Intel Cluster-on-Die, and Intel
+ * Sub-NUMA Clustering have this.
+ */
+static bool x86_has_numa_in_package;
+
static bool match_llc(struct cpuinfo_x86 *c, struct cpuinfo_x86 *o)
{
int cpu1 = c->cpu_index, cpu2 = o->cpu_index;
- if (per_cpu(cpu_llc_id, cpu1) != BAD_APICID &&
- per_cpu(cpu_llc_id, cpu1) == per_cpu(cpu_llc_id, cpu2))
- return topology_sane(c, o, "llc");
+ /* Do not match if we do not have a valid APICID for cpu: */
+ if (per_cpu(cpu_llc_id, cpu1) == BAD_APICID)
+ return false;
- return false;
+ /* Do not match if LLC id does not match: */
+ if (per_cpu(cpu_llc_id, cpu1) != per_cpu(cpu_llc_id, cpu2))
+ return false;
+
+ /*
+* Some Intel CPUs enumerate an LLC that is shared by
+* multiple NUMA nodes. The LLC on these systems is
+* shared for off-package data access but private to the
+* NUMA node (half of the package) for on-package access.
+*
+* CPUID can only enumerate the cache as being shared *or*
+* unshared, but not this particular configurat
Re: [Outreachy kernel] [PATCH] iio: adc: replace comma with a semicolon
On Thu, Mar 30, 2017 at 06:16:03PM +0530, Arushi Singhal wrote: > Replace a comma between expression statements by a semicolon. This > changes the semantics of the code, but given the current indentation > appears to be what is intended. Hi Arushi, Well, you've gotten a lot of comments on this one, and I too, couldn't resist ;) My comment is to test things like this out. Of course, you're not going to go off and test the max11100 driver (BTW - driver name in Subject line please), but you can write a simple C program to verify the behavior of commas vs semi-colons in assignment statements. That'll prove it to yourself, and then you can write the changelog with confidence. (That this is cosmetic ;)) It's a useful habit to pick up. When you see a code segment that makes you wonder, you can often pluck it out of context and try it in a simple c program. alisons > A simplified version of the Coccinelle semantic patch that performs this > transformation is as follows: > > // > @r@ > expression e1,e2; > @@ > > e1 > -, > +; > e2; > // > > Signed-off-by: Arushi Singhal > --- > drivers/iio/adc/max11100.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/max11100.c b/drivers/iio/adc/max11100.c > index 23c060e1b663..1180bcc22ff1 100644 > --- a/drivers/iio/adc/max11100.c > +++ b/drivers/iio/adc/max11100.c > @@ -124,8 +124,8 @@ static int max11100_probe(struct spi_device *spi) > indio_dev->name = "max11100"; > indio_dev->info = &max11100_info; > indio_dev->modes = INDIO_DIRECT_MODE; > - indio_dev->channels = max11100_channels, > - indio_dev->num_channels = ARRAY_SIZE(max11100_channels), > + indio_dev->channels = max11100_channels; > + indio_dev->num_channels = ARRAY_SIZE(max11100_channels); > > state->vref_reg = devm_regulator_get(&spi->dev, "vref"); > if (IS_ERR(state->vref_reg)) > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170330124603.GA29301%40arushi-HP-Pavilion-Notebook. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH 0/4] drivers: iio: Replace ternary operator with min macro
On Wed, Mar 29, 2017 at 06:03:07PM +0530, simran singhal wrote: > Use macro min() to get the minimum of two values for brevity and readability. > > simran singhal (4): > iio: common: st_sensors: Replace ternary operator with min macro > iio: imu: st_lsm6dsx: Replace ternary operator with min macro > iio: imu: st_lsm6dsx: Replace ternary operator with min macro > iio: light: si1145: Replace ternary operator with min macro Hi Simran, I'm guessing these were found by Coccinelle. Please reference that in the changelog. FYI: 'Mileage will vary' on cleanup patches sent upstream. Their acceptance varies by subsystem and the change. Consider sending one such patch and await feedback. Another option is to send one and be direct. Ask below the line --- if such cleanup patches are welcome. Sometimes they create too much churn and are only taken as part of a larger update to the particular driver. Just my thoughts for future patches as Outreachy window closes :( alisons > > drivers/iio/common/st_sensors/st_sensors_i2c.c | 2 +- > drivers/iio/humidity/hts221_core.c | 2 +- > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +- > drivers/iio/light/si1145.c | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/1490790791-5694-1-git-send-email-singhalsimran0%40gmail.com. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] iio: accel: bma180: Set up buffer timestamps for non-zero values
On Wed, Mar 29, 2017 at 08:16:58AM -0700, Alison Schofield wrote:
> On Wed, Mar 29, 2017 at 07:41:31PM +0530, simran singhal wrote:
> > Use the iio_pollfunc_store_time parameter during triggered buffer set-up
> > to get valid timestamps.
> >
> > Signed-off-by: simran singhal
>
> Hi Simran,
> I guess you knew I'd recognize this one ;)
>
> My first thought were - "Wow that was a short and sweet description!
> Is that all I said when I made similar patches?" So, I went back
> in history and sure enough - that is all I said in the changelog.
>
> But I knew I had thought more about it than that changelog showed.
> So, I went back to the mailing list and found the patchset cover
> sheet where I had placed the greater explanation and options.
>
> Note to myself: don't bury too much stuff in cover letters!
>
> Note to you: Defects do repeat themselves so your method of hunting
> is fine. If you haven't done it already, add the step where you go
> find the mailing list discussion of the patch. That can be helpful.
>
> Here's a link to the patchset on linux-iio:
> http://marc.info/?l=linux-iio&m=146825066822274&w=2
>
> The piece of interest is:
> Alternative change would be to use iio_get_time_ns() directly in the
> push_to_buffers call in the handler. We can do this if you are not
> one of those "devices (who) need a timestamp grabbed as soon as possible
> after the trigger" (from iio_poll_func definition)
Looked at driver. It's already doing the above mentioned call
to iio_get_time_ns() in the trigger handler. So, it seems this
change is not needed.
alisons
>
> This is all just FYI for you now.
>
> Leave the patch for the reviewers to comment.
>
> Thanks,
> alisons
>
> > ---
> > drivers/iio/accel/bma180.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> > index efc6773..4ae5c80 100644
> > --- a/drivers/iio/accel/bma180.c
> > +++ b/drivers/iio/accel/bma180.c
> > @@ -762,7 +762,7 @@ static int bma180_probe(struct i2c_client *client,
> > goto err_trigger_free;
> > }
> >
> > - ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > + ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> > bma180_trigger_handler, NULL);
> > if (ret < 0) {
> > dev_err(&client->dev, "unable to setup iio triggered buffer\n");
> > --
> > 2.7.4
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to outreachy-kernel+unsubscr...@googlegroups.com.
> > To post to this group, send email to outreachy-ker...@googlegroups.com.
> > To view this discussion on the web visit
> > https://groups.google.com/d/msgid/outreachy-kernel/20170329141131.GA23646%40singhal-Inspiron-5558.
> > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] iio: accel: bma180: Set up buffer timestamps for non-zero values
On Wed, Mar 29, 2017 at 07:41:31PM +0530, simran singhal wrote:
> Use the iio_pollfunc_store_time parameter during triggered buffer set-up
> to get valid timestamps.
>
> Signed-off-by: simran singhal
Hi Simran,
I guess you knew I'd recognize this one ;)
My first thought were - "Wow that was a short and sweet description!
Is that all I said when I made similar patches?" So, I went back
in history and sure enough - that is all I said in the changelog.
But I knew I had thought more about it than that changelog showed.
So, I went back to the mailing list and found the patchset cover
sheet where I had placed the greater explanation and options.
Note to myself: don't bury too much stuff in cover letters!
Note to you: Defects do repeat themselves so your method of hunting
is fine. If you haven't done it already, add the step where you go
find the mailing list discussion of the patch. That can be helpful.
Here's a link to the patchset on linux-iio:
http://marc.info/?l=linux-iio&m=146825066822274&w=2
The piece of interest is:
Alternative change would be to use iio_get_time_ns() directly in the
push_to_buffers call in the handler. We can do this if you are not
one of those "devices (who) need a timestamp grabbed as soon as possible
after the trigger" (from iio_poll_func definition)
This is all just FYI for you now.
Leave the patch for the reviewers to comment.
Thanks,
alisons
> ---
> drivers/iio/accel/bma180.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c
> index efc6773..4ae5c80 100644
> --- a/drivers/iio/accel/bma180.c
> +++ b/drivers/iio/accel/bma180.c
> @@ -762,7 +762,7 @@ static int bma180_probe(struct i2c_client *client,
> goto err_trigger_free;
> }
>
> - ret = iio_triggered_buffer_setup(indio_dev, NULL,
> + ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> bma180_trigger_handler, NULL);
> if (ret < 0) {
> dev_err(&client->dev, "unable to setup iio triggered buffer\n");
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170329141131.GA23646%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock
On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote:
> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield
> wrote:
> > On Fri, Mar 24, 2017 at 12:05:20AM +0530, simran singhal wrote:
> >> The IIO subsystem is redefining iio_dev->mlock to be used by
> >> the IIO core only for protecting device operating mode changes.
> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> >>
> >> In this driver, mlock was being used to protect hardware state
> >> changes. Replace it with a lock in the devices global data.
> >
> > Hi Simran,
> >
> > Please post all revision histories below the --- not just the most
> > recent.
> >
> Sorry, will not repeat this.
>
> > Does this lock enforce the needed "atomicity" in the write_frequency
> > function? I read Jonathans comment on a previous revision about
> > "ensuring the spi bus frequency and sampling frequency of the device
> > are changed in an atomic fashion"
> >
>
> By introducing another lock I am protecting read_modify_write and
> in this way also protecting the designated register that we are about
> to write.
I see it protecting this path from being re-entered. My uncertainty
is about other paths to read/write.
>
> > Is it possible for another spi bus transaction (read or write) to
> > occur between the read and write in write_frequency?
> >
>
> Gargi has also come up with a solution.
> https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8
>
> Should I do like her as her's also seem correct or go ahead with this.
My suggestion would be to wait for feedback on Gargi's patch.
(See the Outreachy log about creating similar solutions.)
We will not be able to close on this set of patches during the
Outreachy application window. You can continue to push for closure
beyond the March 30th date as your time allows :)
Thanks,
alisons
>
> > alisons
> >>
> >> Signed-off-by: simran singhal
> >> ---
> >>
> >> v4:
> >>-Add mutex_init
> >>
> >> drivers/staging/iio/meter/ade7753.c | 7 +--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/meter/ade7753.c
> >> b/drivers/staging/iio/meter/ade7753.c
> >> index b71fbd3..30aebaf 100644
> >> --- a/drivers/staging/iio/meter/ade7753.c
> >> +++ b/drivers/staging/iio/meter/ade7753.c
> >> @@ -80,11 +80,13 @@
> >> * @us: actual spi_device
> >> * @tx: transmit buffer
> >> * @rx: receive buffer
> >> + * @lock: protect sensor state
> >> * @buf_lock: mutex to protect tx and rx
> >> **/
> >> struct ade7753_state {
> >> struct spi_device *us;
> >> struct mutexbuf_lock;
> >> + struct mutexlock; /* protect sensor state */
> >> u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> >> u8 rx[ADE7753_MAX_RX];
> >> };
> >> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> if (!val)
> >> return -EINVAL;
> >>
> >> - mutex_lock(&indio_dev->mlock);
> >> + mutex_lock(&st->lock);
> >>
> >> t = 27900 / val;
> >> if (t > 0)
> >> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
> >>
> >> out:
> >> - mutex_unlock(&indio_dev->mlock);
> >> + mutex_unlock(&st->lock);
> >>
> >> return ret ? ret : len;
> >> }
> >> @@ -581,6 +583,7 @@ static int ade7753_probe(struct spi_device *spi)
> >> st = iio_priv(indio_dev);
> >> st->us = spi;
> >> mutex_init(&st->buf_lock);
> >> + mutex_init(&st->lock);
> >>
> >> indio_dev->name = spi->dev.driver->name;
> >> indio_dev->dev.parent = &spi->dev;
> >> --
> >> 2.7.4
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >> email to outreachy-kernel+unsubscr...@googlegroups.com.
> >> To post to this group, send email to outreachy-ker...@googlegroups.com.
> >> To view this discussion on the web visit
> >> https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
> >> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock
On Fri, Mar 24, 2017 at 12:05:20AM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
Hi Simran,
Please post all revision histories below the --- not just the most
recent.
Does this lock enforce the needed "atomicity" in the write_frequency
function? I read Jonathans comment on a previous revision about
"ensuring the spi bus frequency and sampling frequency of the device
are changed in an atomic fashion"
Is it possible for another spi bus transaction (read or write) to
occur between the read and write in write_frequency?
alisons
>
> Signed-off-by: simran singhal
> ---
>
> v4:
>-Add mutex_init
>
> drivers/staging/iio/meter/ade7753.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index b71fbd3..30aebaf 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -80,11 +80,13 @@
> * @us: actual spi_device
> * @tx: transmit buffer
> * @rx: receive buffer
> + * @lock: protect sensor state
> * @buf_lock: mutex to protect tx and rx
> **/
> struct ade7753_state {
> struct spi_device *us;
> struct mutexbuf_lock;
> + struct mutexlock; /* protect sensor state */
> u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> u8 rx[ADE7753_MAX_RX];
> };
> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> if (!val)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
>
> t = 27900 / val;
> if (t > 0)
> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret ? ret : len;
> }
> @@ -581,6 +583,7 @@ static int ade7753_probe(struct spi_device *spi)
> st = iio_priv(indio_dev);
> st->us = spi;
> mutex_init(&st->buf_lock);
> + mutex_init(&st->lock);
>
> indio_dev->name = spi->dev.driver->name;
> indio_dev->dev.parent = &spi->dev;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v3 0/2] Replace mlock with private lock and delete whitespaces
On Tue, Mar 21, 2017 at 11:33:54PM +0530, simran singhal wrote: > The patch series replaces mlock with a private lock for driver ad9834 and > Fix coding style issues related to white spaces. Hi Simran, I'm getting lost. Patchset Subject Line needs subsystem and driver. The comment above says ad9834 but the patches below say ade7753. Can we drive adis16060 through ACK and then come back to this one? (ie. applyling lessons learned) thanks, alisons > > v3: > -Using new private "lock" instead of using "buf_lock" >as it can cause deadlock. > -Sending it as a series of two patches. > > v2: > -Using the existing buf_lock instead of lock. > > > simran singhal (2): > staging: iio: ade7753: Remove trailing whitespaces > staging: iio: ade7753: Replace mlock with driver private lock > > drivers/staging/iio/meter/ade7753.c | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) > > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/1490119436-20042-1-git-send-email-singhalsimran0%40gmail.com. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH 0/2] IIO coding tasks
On Tue, Mar 21, 2017 at 10:52:46PM +0530, Arushi Singhal wrote: > On Tue, Mar 21, 2017 at 10:32 PM, Alison Schofield > wrote: > > > On Tue, Mar 21, 2017 at 05:39:38PM +0100, Julia Lawall wrote: > > > > > > > > > On Tue, 21 Mar 2017, Arushi Singhal wrote: > > > > > > > On Tue, Mar 21, 2017 at 9:33 PM, Alison Schofield < > > amsfiel...@gmail.com> > > > > wrote: > > > > On Tue, Mar 21, 2017 at 07:00:17PM +0530, Arushi Singhal wrote: > > > > > Patchseries of IIO coding tasks > > > > > > > > This wouldn't be a patchset. Although they touch the same > > > > driver, the changes are unrelated. See more below... > > > > > > > > This I have send as a Patchset because as you have mentioned below > > that the > > > > [PATCH 2/2] was already done before by someone but I think so it is > > not yet > > > > applied in the Greg's tree yet.So I have done both the changes and as > > they > > > > should be applied one after other so that's why I have send them as > > > > Patchset. > > > > > > For the IIO patches, it is better to work on the IIO tree, not Greg's. > > > Greg manages staging, not IIO. The IIO patches should appear in Greg's > > > tree eventually. > > > > > > julia > > > > We didn't direct applicants to create an iio tree. At this point, > > it seems more than is necessary. They can follow the directions in the > > task descriptions and avoid the collisions. > > > > Of course, they are welcome to create a tree to iio/testing. > > > > (IMHO it's more overhead/busy work and maybe not the best use > > of time in the home stretch of the application period.) > > > > alisons > > > > Hi Alison > As you have mentioned that my [PATCH 2/2] is already being done someone. So > how can I make the changes of [PATCH 1/2] on top of it as [PATCH 2/2] is > not yet applied on the staging tree. > Please suggest me. > Thanks > Arushi Arushi - I don't see anything needing to be done by you! Your ad7759 mlock patch is awaiting review. It's not your issue to keep up with all changes going into the file. When it gets applied will most likely merge with no merge issue at all. alisons > > > > > > > > > > > > > > Arushi Singhal (2): > > > > > staging: ad7759: Replace mlock with driver private lock > > > > > > > > This one is already been submitted. If you have a v2 for it, > > > > then v2 > > > > the original patch. > > > > > > > > Is it submitted by me only before? And this is not the v2. > > > > I have just resed it. > > > > > staging: iio: ade7759: Move header content to implementation > > > > file > > > > > > > > This patch is done and applied already. See the Coding Task #1 > > > > notes > > > > in the IIO Tasks page. > > > > > > > > Not at applied I think so. > > > > Thanks > > > > Arushi > > > > > > > > alisons > > > > > > > > > > > > > > drivers/staging/iio/meter/ade7759.c | 56 > > > > +-- > > > > > drivers/staging/iio/meter/ade7759.h | 53 > > > > - > > > > > 2 files changed, 54 insertions(+), 57 deletions(-) > > > > > > > > > > -- > > > > > 2.11.0 > > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > > > > Groups "outreachy-kernel" group. > > > > > To unsubscribe from this group and stop receiving emails from it, > > > > send an email to outreachy-kernel+unsubscr...@googlegroups.com. > > > > > To post to this group, send email to > > > > outreachy-ker...@googlegroups.com. > > > > > To view this discussion on the web visithttps://groups.google. > > com/d/msgid/outreachy-kernel/20170321133021.6737-1-ar > > > > ushisinghal19971997%40gmail.com. > > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > > > > > -- > > > > You received this message because you are subscribed to the Google > > Groups > > > > "outreachy-kernel" group. > > > > To unsubscribe from this group and stop receiving emails from it, send > > an > > > > email to outreachy-kernel+unsubscr...@googlegroups.com. > > > > To post to this group, send email to outreachy-ker...@googlegroups.com > > . > > > > To view this discussion on the web visithttps://groups.google. > > com/d/msgid/outreachy-kernel/CA%2BXqjF9dVy33Dsv0H2z8x > > > > taNeMOW7SQgr4qa4wLwz6xFNVTsUA%40mail.gmail.com. > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > > > > >
Re: [Outreachy kernel] [PATCH v6] staging: Use buf_lock instead of mlock and Refactor code
On Tue, Mar 21, 2017 at 10:34:01PM +0530, SIMRAN SINGHAL wrote:
> On Tue, Mar 21, 2017 at 10:18 PM, Alison Schofield
> wrote:
> > On Mon, Mar 20, 2017 at 01:36:21AM +0530, simran singhal wrote:
> >
> > Hi Simran,
> >
> > I going to ask for a v7 without looking at the code ;)
> > Subject line needs subsystem and driver.
> > Subject and log message can be improved.
>
> Hi Alison,
> I have already sent v7 with changed subject.
Simran,
I see v7. Needs subsystem (iio) and to nitpick, driver name
is "adis16060" ;) Other comments still apply.
Please append all version histories below the --- for review.
v7:
v6:
.
.
v2:
thanks,
alisons
>
> >
> >> The IIO subsystem is redefining iio_dev->mlock to be used by
> >> the IIO core only for protecting device operating mode changes.
> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> >>
> >> In this driver, mlock was being used to protect hardware state
> >> changes. Replace it with buf_lock in the devices global data.
> >^^^ this was not done
> >>
> >> As buf_lock protects both the adis16060_spi_write() and
> >> adis16060_spi_read() functions and both are always called in
> >> pair. First write, then read. Thus, refactor the code to have
> >> one single function adis16060_spi_write_than_read() which is
> >> protected by the existing buf_lock.
> > This was done. So, you were able to obsolete the need for mlock
> > by creating the paired function.
> >
> >>
> >> Removed nested locks as the function adis16060_read_raw call
> >> a lock on &st->buf_lock and then calls the function
> >> adis16060_spi_write which again tries to get hold
> >> of the same lock.
> > this was not done. Yes, you avoided nested locks through
> > proper coding, but we don't want to give the impression in the
> > log message that there was a pre-existing nested lock issue.
> >
> > I did checkpatch & compile it...but looked no further yet.
> >
> > alisons
> >>
> >> Signed-off-by: simran singhal
> >> ---
> >>
> >> v6:
> >>-Change commit message
> >>-Remove nested lock
> >>
> >> drivers/staging/iio/gyro/adis16060_core.c | 40
> >> ++-
> >> 1 file changed, 13 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> >> b/drivers/staging/iio/gyro/adis16060_core.c
> >> index c9d46e7..1c6de46 100644
> >> --- a/drivers/staging/iio/gyro/adis16060_core.c
> >> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> >> @@ -40,25 +40,17 @@ struct adis16060_state {
> >>
> >> static struct iio_dev *adis16060_iio_dev;
> >>
> >> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> >> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> >> + u8 conf, u16 *val)
> >> {
> >> int ret;
> >> struct adis16060_state *st = iio_priv(indio_dev);
> >>
> >> - mutex_lock(&st->buf_lock);
> >> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
> >> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
> >> ret = spi_write(st->us_w, st->buf, 3);
> >> - mutex_unlock(&st->buf_lock);
> >> -
> >> - return ret;
> >> -}
> >> -
> >> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> >> -{
> >> - int ret;
> >> - struct adis16060_state *st = iio_priv(indio_dev);
> >>
> >> - mutex_lock(&st->buf_lock);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >> ret = spi_read(st->us_r, st->buf, 3);
> >>
> >> @@ -69,8 +61,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev,
> >> u16 *val)
> >>*/
> >> if (!ret)
> >> *val = ((st->buf[0] & 0x3) << 12) |
> >> - (st->buf[1] << 4) |
> >> - ((st->buf[2] >> 4) & 0xF);
> >> + (st->buf[1] << 4) |
> >> + ((st->buf[2] >> 4) & 0xF);
> >> mutex_unlock(&st->buf_lock);
> >>
> >> return ret;
> >> @@ -83,20 +75,18 @@ stat
Re: [Outreachy kernel] [PATCH 0/2] IIO coding tasks
On Tue, Mar 21, 2017 at 05:39:38PM +0100, Julia Lawall wrote: > > > On Tue, 21 Mar 2017, Arushi Singhal wrote: > > > On Tue, Mar 21, 2017 at 9:33 PM, Alison Schofield > > wrote: > > On Tue, Mar 21, 2017 at 07:00:17PM +0530, Arushi Singhal wrote: > > > Patchseries of IIO coding tasks > > > > This wouldn't be a patchset. Although they touch the same > > driver, the changes are unrelated. See more below... > > > > This I have send as a Patchset because as you have mentioned below that the > > [PATCH 2/2] was already done before by someone but I think so it is not yet > > applied in the Greg's tree yet.So I have done both the changes and as they > > should be applied one after other so that's why I have send them as > > Patchset. > > For the IIO patches, it is better to work on the IIO tree, not Greg's. > Greg manages staging, not IIO. The IIO patches should appear in Greg's > tree eventually. > > julia We didn't direct applicants to create an iio tree. At this point, it seems more than is necessary. They can follow the directions in the task descriptions and avoid the collisions. Of course, they are welcome to create a tree to iio/testing. (IMHO it's more overhead/busy work and maybe not the best use of time in the home stretch of the application period.) alisons > > > > > > > Arushi Singhal (2): > > > staging: ad7759: Replace mlock with driver private lock > > > > This one is already been submitted. If you have a v2 for it, > > then v2 > > the original patch. > > > > Is it submitted by me only before? And this is not the v2. > > I have just resed it. > > > staging: iio: ade7759: Move header content to implementation > > file > > > > This patch is done and applied already. See the Coding Task #1 > > notes > > in the IIO Tasks page. > > > > Not at applied I think so. > > Thanks > > Arushi > > > > alisons > > > > > > > > drivers/staging/iio/meter/ade7759.c | 56 > > +-- > > > drivers/staging/iio/meter/ade7759.h | 53 > > - > > > 2 files changed, 54 insertions(+), 57 deletions(-) > > > > > > -- > > > 2.11.0 > > > > > > -- > > > You received this message because you are subscribed to the Google > > Groups "outreachy-kernel" group. > > > To unsubscribe from this group and stop receiving emails from it, > > send an email to outreachy-kernel+unsubscr...@googlegroups.com. > > > To post to this group, send email to > > outreachy-ker...@googlegroups.com. > > > To view this discussion on the web > > > visithttps://groups.google.com/d/msgid/outreachy-kernel/20170321133021.6737-1-ar > > ushisinghal19971997%40gmail.com. > > > For more options, visit https://groups.google.com/d/optout. > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to outreachy-kernel+unsubscr...@googlegroups.com. > > To post to this group, send email to outreachy-ker...@googlegroups.com. > > To view this discussion on the web > > visithttps://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF9dVy33Dsv0H2z8x > > taNeMOW7SQgr4qa4wLwz6xFNVTsUA%40mail.gmail.com. > > For more options, visit https://groups.google.com/d/optout. > > > >
Re: [Outreachy kernel] [PATCH v6] staging: Use buf_lock instead of mlock and Refactor code
On Mon, Mar 20, 2017 at 01:36:21AM +0530, simran singhal wrote:
Hi Simran,
I going to ask for a v7 without looking at the code ;)
Subject line needs subsystem and driver.
Subject and log message can be improved.
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with buf_lock in the devices global data.
^^^ this was not done
>
> As buf_lock protects both the adis16060_spi_write() and
> adis16060_spi_read() functions and both are always called in
> pair. First write, then read. Thus, refactor the code to have
> one single function adis16060_spi_write_than_read() which is
> protected by the existing buf_lock.
This was done. So, you were able to obsolete the need for mlock
by creating the paired function.
>
> Removed nested locks as the function adis16060_read_raw call
> a lock on &st->buf_lock and then calls the function
> adis16060_spi_write which again tries to get hold
> of the same lock.
this was not done. Yes, you avoided nested locks through
proper coding, but we don't want to give the impression in the
log message that there was a pre-existing nested lock issue.
I did checkpatch & compile it...but looked no further yet.
alisons
>
> Signed-off-by: simran singhal
> ---
>
> v6:
>-Change commit message
>-Remove nested lock
>
> drivers/staging/iio/gyro/adis16060_core.c | 40
> ++-
> 1 file changed, 13 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..1c6de46 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -40,25 +40,17 @@ struct adis16060_state {
>
> static struct iio_dev *adis16060_iio_dev;
>
> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> + u8 conf, u16 *val)
> {
> int ret;
> struct adis16060_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->buf_lock);
> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
> ret = spi_write(st->us_w, st->buf, 3);
> - mutex_unlock(&st->buf_lock);
> -
> - return ret;
> -}
> -
> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> -{
> - int ret;
> - struct adis16060_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->buf_lock);
> + if (ret < 0)
> + return ret;
>
> ret = spi_read(st->us_r, st->buf, 3);
>
> @@ -69,8 +61,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev,
> u16 *val)
>*/
> if (!ret)
> *val = ((st->buf[0] & 0x3) << 12) |
> - (st->buf[1] << 4) |
> - ((st->buf[2] >> 4) & 0xF);
> + (st->buf[1] << 4) |
> + ((st->buf[2] >> 4) & 0xF);
> mutex_unlock(&st->buf_lock);
>
> return ret;
> @@ -83,20 +75,18 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> {
> u16 tval = 0;
> int ret;
> + struct adis16060_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> - ret = adis16060_spi_write(indio_dev, chan->address);
> + mutex_lock(&st->buf_lock);
> + ret = adis16060_spi_write_than_read(indio_dev,
> + chan->address, &tval);
> + mutex_unlock(&st->buf_lock);
> if (ret < 0)
> - goto out_unlock;
> + return ret;
>
> - ret = adis16060_spi_read(indio_dev, &tval);
> - if (ret < 0)
> - goto out_unlock;
> -
> - mutex_unlock(&indio_dev->mlock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -110,10 +100,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> }
>
> return -EINVAL;
> -
> -out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> - return ret;
> }
>
> static const struct iio_info adis16060_info = {
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgi
Re: [Outreachy kernel] [PATCH 0/2] IIO coding tasks
On Tue, Mar 21, 2017 at 07:00:17PM +0530, Arushi Singhal wrote: > Patchseries of IIO coding tasks This wouldn't be a patchset. Although they touch the same driver, the changes are unrelated. See more below... > > Arushi Singhal (2): > staging: ad7759: Replace mlock with driver private lock This one is already been submitted. If you have a v2 for it, then v2 the original patch. > staging: iio: ade7759: Move header content to implementation file This patch is done and applied already. See the Coding Task #1 notes in the IIO Tasks page. alisons > > drivers/staging/iio/meter/ade7759.c | 56 > +-- > drivers/staging/iio/meter/ade7759.h | 53 - > 2 files changed, 54 insertions(+), 57 deletions(-) > > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170321133021.6737-1-arushisinghal19971997%40gmail.com. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v2] staging: iio: ade7753: replace mlock with driver private lock
On Mon, Mar 13, 2017 at 10:01:07PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Fix some coding style issues related to white space also.
>
> Signed-off-by: simran singhal
> ---
>
> v2:
>-Removed new lock to reuse the existing lock
Simran,
The good news is that you have 2 patches that have similar
challenges! I'll suggest picking one, drive it to completion,
then do the other.
This has the nested locking issue that Lars warned about.
Need to refactor to avoid. Check back on his review comments.
I suggest dropping those whitespace changes - they appear
out of place in this patch since you are no longer actually
touching those lines of code.
alisons
>
> drivers/staging/iio/meter/ade7753.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..d88eaa3 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -83,10 +83,10 @@
> * @buf_lock: mutex to protect tx and rx
> **/
> struct ade7753_state {
> - struct spi_device *us;
> - struct mutexbuf_lock;
> - u8 tx[ADE7753_MAX_TX]
> cacheline_aligned;
> - u8 rx[ADE7753_MAX_RX];
> + struct spi_device *us;
> + struct mutexbuf_lock;
> + u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> + u8 rx[ADE7753_MAX_RX];
> };
>
> static int ade7753_spi_write_reg_8(struct device *dev,
> @@ -484,7 +484,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> if (!val)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->buf_lock);
>
> t = 27900 / val;
> if (t > 0)
> @@ -505,7 +505,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
>
> return ret ? ret : len;
> }
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170313163107.GA31496%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v3] staging: adis16060_core: Use private driver lock instead of mlock
On Mon, Mar 13, 2017 at 11:41:30PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: simran singhal
> ---
>
> v3:
>-Removed new lock to reuse the existing lock
Hi Simran,
Check Lars review comments about refactoring into a
write_then_read function protected by same buf_lock.
As it is now, it'll deadlock on buf_lock. It locks in
_read_raw and then calls _spi_write where an attempt
is made to grab the same lock.
alisons
>
> drivers/staging/iio/gyro/adis16060_core.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..602ec53 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -83,11 +83,12 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> {
> u16 tval = 0;
> int ret;
> + struct adis16060_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->buf_lock);
> ret = adis16060_spi_write(indio_dev, chan->address);
> if (ret < 0)
> goto out_unlock;
> @@ -96,7 +97,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out_unlock;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +113,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170313181130.GA27111%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock
On Mon, Mar 13, 2017 at 09:28:34AM +0530, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 12:03 AM, Alison Schofield
> wrote:
> > On Sun, Mar 12, 2017 at 07:02:50PM +0530, simran singhal wrote:
> >> The IIO subsystem is redefining iio_dev->mlock to be used by
> >> the IIO core only for protecting device operating mode changes.
> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> >>
> >> In this driver, mlock was being used to protect hardware state
> >> changes. Replace it with a lock in the devices global data.
> >>
> >> Fix some coding style issues related to white space also.
> >>
> >> Signed-off-by: simran singhal
> >
> > Hi Simran, This looks good to me. Let's see what the
> > reviewers say. I think the white space stuff is ok,
> > since it was right where you were editing.
> > alisons
> >
> Alison, so sending this patch here on outreachy mailing list is fine.
> Still confuse :P
You are OK. You sent it to everyone suggested in the Task Description.
This patch was sent before I posted the Task Description. I'm assuming
that since then you've found the posted Task:
https://kernelnewbies.org/IIO_tasks
Find Coding Task 2 --> "PATCHES need to be sent to all of:"
>
> >> ---
> >> drivers/staging/iio/meter/ade7753.c | 14 --
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/meter/ade7753.c
> >> b/drivers/staging/iio/meter/ade7753.c
> >> index dfd8b71..ca99d82 100644
> >> --- a/drivers/staging/iio/meter/ade7753.c
> >> +++ b/drivers/staging/iio/meter/ade7753.c
> >> @@ -81,12 +81,14 @@
> >> * @tx: transmit buffer
> >> * @rx: receive buffer
> >> * @buf_lock: mutex to protect tx and rx
> >> + * @lock:protect sensor state
> >> **/
> >> struct ade7753_state {
> >> - struct spi_device *us;
> >> - struct mutexbuf_lock;
> >> - u8 tx[ADE7753_MAX_TX]
> >> cacheline_aligned;
> >> - u8 rx[ADE7753_MAX_RX];
> >> + struct spi_device *us;
> >> + struct mutexbuf_lock;
> >> + struct mutexlock; /* protect sensor state */
> >> + u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> >> + u8 rx[ADE7753_MAX_RX];
> >> };
> >>
> >> static int ade7753_spi_write_reg_8(struct device *dev,
> >> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> if (!val)
> >> return -EINVAL;
> >>
> >> - mutex_lock(&indio_dev->mlock);
> >> + mutex_lock(&st->lock);
> >>
> >> t = 27900 / val;
> >> if (t > 0)
> >> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
> >>
> >> out:
> >> - mutex_unlock(&indio_dev->mlock);
> >> + mutex_unlock(&st->lock);
> >>
> >> return ret ? ret : len;
> >> }
> >> --
> >> 2.7.4
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >> email to outreachy-kernel+unsubscr...@googlegroups.com.
> >> To post to this group, send email to outreachy-ker...@googlegroups.com.
> >> To view this discussion on the web visit
> >> https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
> >> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock
On Sun, Mar 12, 2017 at 07:02:50PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Fix some coding style issues related to white space also.
>
> Signed-off-by: simran singhal
Hi Simran, This looks good to me. Let's see what the
reviewers say. I think the white space stuff is ok,
since it was right where you were editing.
alisons
> ---
> drivers/staging/iio/meter/ade7753.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..ca99d82 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -81,12 +81,14 @@
> * @tx: transmit buffer
> * @rx: receive buffer
> * @buf_lock: mutex to protect tx and rx
> + * @lock:protect sensor state
> **/
> struct ade7753_state {
> - struct spi_device *us;
> - struct mutexbuf_lock;
> - u8 tx[ADE7753_MAX_TX]
> cacheline_aligned;
> - u8 rx[ADE7753_MAX_RX];
> + struct spi_device *us;
> + struct mutexbuf_lock;
> + struct mutexlock; /* protect sensor state */
> + u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> + u8 rx[ADE7753_MAX_RX];
> };
>
> static int ade7753_spi_write_reg_8(struct device *dev,
> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> if (!val)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
>
> t = 27900 / val;
> if (t > 0)
> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret ? ret : len;
> }
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] staging: adis16060_core: Use private driver lock instead of mlock
On Sun, Mar 12, 2017 at 06:40:52PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: simran singhal
This does not compile.
alisons
> ---
> drivers/staging/iio/gyro/adis16060_core.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..90a3a18 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -29,11 +29,13 @@
> * @us_r:actual spi_device to read back data
> * @buf: transmit or receive buffer
> * @buf_lock:mutex to protect tx and rx
> + * @lock:protect sensor state
> **/
> struct adis16060_state {
> struct spi_device *us_w;
> struct spi_device *us_r;
> struct mutexbuf_lock;
> + struct mutexlock;/* protect sensor state */
>
> u8 buf[3] cacheline_aligned;
> };
> @@ -87,7 +89,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = adis16060_spi_write(indio_dev, chan->address);
> if (ret < 0)
> goto out_unlock;
> @@ -96,7 +98,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out_unlock;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +114,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170312131052.GA21816%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] staging: adis16060_core: Use private driver lock instead of mlock
On Sun, Mar 12, 2017 at 06:40:52PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: simran singhal
Hi Simran,
These are now IIO tasks for this round of Outreachy. Please
follow the directions on the IIO task page w.r.t. claiming,
sending, and all.
Thanks,
alisons
> ---
> drivers/staging/iio/gyro/adis16060_core.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..90a3a18 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -29,11 +29,13 @@
> * @us_r:actual spi_device to read back data
> * @buf: transmit or receive buffer
> * @buf_lock:mutex to protect tx and rx
> + * @lock:protect sensor state
> **/
> struct adis16060_state {
> struct spi_device *us_w;
> struct spi_device *us_r;
> struct mutexbuf_lock;
> + struct mutexlock;/* protect sensor state */
>
> u8 buf[3] cacheline_aligned;
> };
> @@ -87,7 +89,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = adis16060_spi_write(indio_dev, chan->address);
> if (ret < 0)
> goto out_unlock;
> @@ -96,7 +98,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out_unlock;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +114,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170312131052.GA21816%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH] staging: media: Remove unnecessary function and its call
On Sun, Mar 05, 2017 at 12:17:21PM +0530, simran singhal wrote:
> The function atomisp_set_stop_timeout on being called, simply returns
> back. The function hasn't been mentioned in the TODO and doesn't have
> FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
> removed.
>
> Signed-off-by: simran singhal
> ---
Hi Simran,
It's helpful to state right in the subject line what you removed.
ie. remove unused function atomisp_set_stop_timeout()
If you do that, scan's or grep'ing the git log pretty oneline's can
easily see this without having to dig into the log.
(gitpretty='git log --pretty=oneline --abbrev-commit')
Can you share to Outreachy group how you found this? By inspection
or otherwise??
Thanks,
alisons
alisons
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 1 -
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h | 1 -
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c | 5 -
> 3 files changed, 7 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index e99f7b8..66299dd 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -1700,7 +1700,6 @@ void atomisp_wdt_work(struct work_struct *work)
> }
> }
> #endif
> - atomisp_set_stop_timeout(ATOMISP_CSS_STOP_TIMEOUT_US);
> dev_err(isp->dev, "timeout recovery handling done\n");
> atomic_set(&isp->wdt_work_queued, 0);
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> index 5a404e4..0b9ced5 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> @@ -660,7 +660,6 @@ int atomisp_css_set_acc_parameters(struct atomisp_acc_fw
> *acc_fw);
> int atomisp_css_isr_thread(struct atomisp_device *isp,
> bool *frame_done_found,
> bool *css_pipe_done);
> -void atomisp_set_stop_timeout(unsigned int timeout);
>
> bool atomisp_css_valid_sof(struct atomisp_device *isp);
>
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> index 6697d72..cfa0ad4 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> @@ -4699,11 +4699,6 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
> return 0;
> }
>
> -void atomisp_set_stop_timeout(unsigned int timeout)
> -{
> - return;
> -}
> -
> bool atomisp_css_valid_sof(struct atomisp_device *isp)
> {
> unsigned int i, j;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170305064721.GA22548%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v3 4/6] staging: fbtft: Fix sparse warnings of incorrect type in assignment
On Sun, Mar 05, 2017 at 10:35:33AM +0530, simran singhal wrote: > This patch fixes the following sparse warnings: > > drivers/staging/fbtft/fbtft-io.c:74:29: warning: incorrect type in assignment > (different base types) > drivers/staging/fbtft/fbtft-io.c:74:29:expected unsigned long long > [unsigned] [long] [long long] [usertype] > drivers/staging/fbtft/fbtft-io.c:74:29:got restricted __be64 [usertype] > > > Signed-off-by: simran singhal > --- > > v3: >-Change commit message >-Drop the changes did in fbtft-bus.c Hi Simran, Sorry, I didn't mean to drop the broken part of fbtft, but rather to fix it. Let's tidy up a few things: - we can't send out one patch labelled as part of a patchset without resending the entire set. That's why I was suggesting just abandoning the patchset, and beginning to rework this anew with only the fbtft driver in a single patch. - I suggest keeping the -bus and -io file changes together. - Look to correct what is wrong with fbtft-bus.c before sending a new patch. - Patch Subject would be something like: use __be64 types for endian correctness (if this is what you end up doing - hint) So, next step, 1) read up on endianess and propose how to change -bus.c. OK to check that idea/code snippet here without re-rolling the patch. https://kernelnewbies.org/EndianIssues and Vaishali also gave a good link to lwn.net article and to see some example fixes (in iio subsystem) do this: git log --pretty=oneline --abbrev-commit | grep iio | grep endian 2) build the patch. At this point I say make it a first version and start totally fresh. However, below the line --- where you usually put the version comments, you can add a comment stating that this was previously part of a patchset, but is now a single patch. alisons > > drivers/staging/fbtft/fbtft-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fbtft-io.c > b/drivers/staging/fbtft/fbtft-io.c > index d868405..ffb9a3b 100644 > --- a/drivers/staging/fbtft/fbtft-io.c > +++ b/drivers/staging/fbtft/fbtft-io.c > @@ -71,7 +71,7 @@ int fbtft_write_spi_emulate_9(struct fbtft_par *par, void > *buf, size_t len) > src++; > } > tmp |= ((*src & 0x0100) ? 1 : 0); > - *(u64 *)dst = cpu_to_be64(tmp); > + *(__be64 *)dst = cpu_to_be64(tmp); > dst += 8; > *dst++ = (u8)(*src++ & 0x00FF); > added++; > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170305050533.GA10169%40singhal-Inspiron-5558. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] Re: [PATCH v2 4/6] staging: fbtft: Fix sparse warnings of incorrect type in assignment
On Thu, Mar 02, 2017 at 02:26:37PM +0100, Noralf Trønnes wrote: > > Den 02.03.2017 14.04, skrev simran singhal: > >This patch fixes the following sparse warnings: > > > >drivers/staging/fbtft/fbtft-bus.c:166:36: warning: incorrect type in > >assignment (different base types) > >drivers/staging/fbtft/fbtft-bus.c:166:36:expected unsigned short > >[unsigned] [short] [usertype] > >drivers/staging/fbtft/fbtft-bus.c:166:36:got restricted __be16 > >[usertype] > > > >drivers/staging/fbtft/fbtft-io.c:74:29: warning: incorrect type in > >assignment (different base types) > >drivers/staging/fbtft/fbtft-io.c:74:29:expected unsigned long long > >[unsigned] [long] [long long] [usertype] > >drivers/staging/fbtft/fbtft-io.c:74:29:got restricted __be64 [usertype] > > > > > >Signed-off-by: simran singhal > >--- > > v2: > >-changed commit message > > > > drivers/staging/fbtft/fbtft-bus.c | 2 +- > > drivers/staging/fbtft/fbtft-io.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/staging/fbtft/fbtft-bus.c > >b/drivers/staging/fbtft/fbtft-bus.c > >index ec45043..df2223e 100644 > >--- a/drivers/staging/fbtft/fbtft-bus.c > >+++ b/drivers/staging/fbtft/fbtft-bus.c > >@@ -163,7 +163,7 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, > >size_t offset, size_t len) > > to_copy, remain - to_copy); > > for (i = 0; i < to_copy; i++) > >-txbuf16[i] = cpu_to_be16(vmem16[i]); > >+txbuf16[i] = vmem16[i]; > > This change breaks functionality on little endian machines like > the Raspberry Pi. > > > Noralf. > Hi Simran, It's probably good to get back to this one while we have Noralf's attention. While the change above - in fbtft-bus.c is a problem, the change below in fbtft-io.c looks ok. So, compare what you did. You can't *not* do the endian conversion. This is a cleanup change, a cosmetic change. There is no underlying bug, so you just need to get the typing correct w/out affecting behavior. BTW: I think it's OK to pull this one out and send a v3 of this alone. That would mean abandoning the patchset and doing them one at a time. They are all in different drivers anyway. alisons > > vmem16 = vmem16 + to_copy; > > ret = par->fbtftops.write(par, par->txbuf.buf, > >diff --git a/drivers/staging/fbtft/fbtft-io.c > >b/drivers/staging/fbtft/fbtft-io.c > >index d868405..ffb9a3b 100644 > >--- a/drivers/staging/fbtft/fbtft-io.c > >+++ b/drivers/staging/fbtft/fbtft-io.c > >@@ -71,7 +71,7 @@ int fbtft_write_spi_emulate_9(struct fbtft_par *par, void > >*buf, size_t len) > > src++; > > } > > tmp |= ((*src & 0x0100) ? 1 : 0); > >-*(u64 *)dst = cpu_to_be64(tmp); > >+*(__be64 *)dst = cpu_to_be64(tmp); > > dst += 8; > > *dst++ = (u8)(*src++ & 0x00FF); > > added++; > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/fe8d6a85-3d4e-8019-937b-22389b942da3%40tronnes.org. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v2] staging: speakup: Comparison to NULL could be written
On Thu, Mar 02, 2017 at 08:37:21AM -0800, Alison Schofield wrote:
> On Thu, Mar 02, 2017 at 02:13:02PM +0530, Arushi Singhal wrote:
> > Fixed coding style for null comparisons in speakup driver to be more
> > consistant with the rest of the kernel coding style.
> >
> > Signed-off-by: Arushi Singhal
> > ---
> > changes in v2
> > - fixed coding style error and upto the coding style.
>
> Hi Arushi,
>
> Take another look at Joe's message about checkpatch on the patch
> file. Looks like you missed that.
Arushi,
Looking further, seems you only missed the spelling warning:
WARNING: 'consistant' may be misspelled - perhaps 'consistent'?
You would be missing this if you are running checkpatch on your
.c file, but not doing the checkpatch "hook" from the firstpatch
tutorial. That hook would catch this. And, you can just catch
it by running checkpatch on the patch file after it's built.
(The hook is a painless failsafe...recommend using it.)
alisons
>
> Here's some tips on styling your commit and log messages:
>
> You've probably seen feedback of putting the message into
> the "imperative". ie...it's as if you are directing/commanding
> what is to happen. Best way I've found to get that to sink in
> is to look at your predecessors...and look at more than one
> because poor messages do sneak in occasionally.
>
> For this one, I might do this:
>
> > staging/speakup$ gitpretty *.c | grep NULL
> > (my alias: alias gitpretty='git log --pretty=oneline --abbrev-commit')
> >
> > d3da1cb staging: speakup: (coding style) Rewrite comparisons to NULL
> > a90624c Staging: speakup: kobjects.c: Remove explicit NULL comparison
> > 114885e Staging: speakup: serialio.c: Remove explicit NULL comparison
> > 562c4798 Staging: speakup: devsynth.c: Remove explicit NULL comparison
> > ff52fc3 Staging: speakup: varhandlers.c: Remove explicit NULL comparison
> >
> Then, go further into one that looks like it might match your change:
>
> > staging/speakup$ git log a90624c
> > commit a90624cf253cc74e9464b42d54aa4825575edefe
> > Author: Shraddha Barke
> > Date: Fri Sep 11 11:32:28 2015 +0530
> >
> > Staging: speakup: kobjects.c: Remove explicit NULL comparison
> > >
> > Remove explicit NULL comparison and write it in its simpler form.
> >
> >
>
> This would give me confidence in the commit message and log.
> And, since I tend toward the obsessive ;), if the next day I do this
> fix in another directory, I would repeat this process on that directory
> and file because styles can vary by driver/subsystem. Perhaps not on
> such a simple fix, but more so as you dive deeper.
>
> alisons
>
> >
> > drivers/staging/speakup/fakekey.c | 2 +-
> > drivers/staging/speakup/kobjects.c | 2 +-
> > drivers/staging/speakup/main.c | 38
> > +++---
> > 3 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/speakup/fakekey.c
> > b/drivers/staging/speakup/fakekey.c
> > index d76da0a1382c..294c74b47224 100644
> > --- a/drivers/staging/speakup/fakekey.c
> > +++ b/drivers/staging/speakup/fakekey.c
> > @@ -56,7 +56,7 @@ int speakup_add_virtual_keyboard(void)
> >
> > void speakup_remove_virtual_keyboard(void)
> > {
> > - if (virt_keyboard != NULL) {
> > + if (virt_keyboard) {
> > input_unregister_device(virt_keyboard);
> > virt_keyboard = NULL;
> > }
> > diff --git a/drivers/staging/speakup/kobjects.c
> > b/drivers/staging/speakup/kobjects.c
> > index 5d871ec3693c..2fef55569bfd 100644
> > --- a/drivers/staging/speakup/kobjects.c
> > +++ b/drivers/staging/speakup/kobjects.c
> > @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > len--;
> > new_synth_name[len] = '\0';
> > spk_strlwr(new_synth_name);
> > - if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
> > + if (synth && !strcmp(new_synth_name, synth->name)) {
> > pr_warn("%s already in use\n", new_synth_name);
> > } else if (synth_init(new_synth_name) != 0) {
> > pr_warn("failed to init synth %s\n", new_synth_name);
> > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > index c2f70ef5b9b3..a12ec2b061fe 100644
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -299,7 +299
Re: [Outreachy kernel] [PATCH 1/6] staging: wlan-ng: Fix sparse warning of restricted __le16
On Thu, Mar 02, 2017 at 12:14:40PM +0100, Julia Lawall wrote: > > > On Thu, 2 Mar 2017, SIMRAN SINGHAL wrote: > > > On Thu, Mar 2, 2017 at 3:20 PM, Julia Lawall wrote: > > > > > > > > > On Thu, 2 Mar 2017, Julia Lawall wrote: > > > > > >> > > >> > > >> On Thu, 2 Mar 2017, simran singhal wrote: > > >> > > >> > This patch fixes the following sparse warning: > > >> > warning: cast to restricted __le16 > > >> > > >> You commit message should not say just fix X. What have you done to > > >> carry > > >> out the fix and why is this the correct approach? > > > > > > This comment applies to all of the patches in the series. > > > > > > julia > > > > > The changes and sparse warnings for them seems reasonable to me as > > after doing the > > change sparse was not showing error like cast to __le16 or something like > > this. > > Also it compiled perfectly. > > This doesn't mean that it actually works. The function calls at least may > have been put there for a reason. Maysbe you can find other patches that > relate to this call and see what they do. > > julia Hi Simran, Just following on Julia's feedback and I see you've gotten a message that this breaks little endian machines. Good for you for trying to go deeper. I'd suggest taking this one at a time and figure out what is going on with one of them. As Julia says, with this kind of change, you will have to prove why it is correct. (You might not be able to change any of these - I don't know, but I know you'll learn something!) Look here - and then pick one to trace through - https://kernelnewbies.org/EndianIssues Generally - probably best to test the waters with something of this type, with a single patch. Get that nailed, then move onto a patchset, if you have a pattern. Heads down & good luck! alisons > > > > >> > > >> julia > > >> > > >> > > > >> > Signed-off-by: simran singhal > > >> > --- > > >> > drivers/staging/wlan-ng/prism2sta.c | 51 > > >> > ++--- > > >> > 1 file changed, 25 insertions(+), 26 deletions(-) > > >> > > > >> > diff --git a/drivers/staging/wlan-ng/prism2sta.c > > >> > b/drivers/staging/wlan-ng/prism2sta.c > > >> > index 984804b..82d3a70 100644 > > >> > --- a/drivers/staging/wlan-ng/prism2sta.c > > >> > +++ b/drivers/staging/wlan-ng/prism2sta.c > > >> > @@ -372,10 +372,9 @@ static int prism2sta_mlmerequest(struct > > >> > wlandevice *wlandev, > > >> > qualmsg->noise.status = > > >> > P80211ENUM_msgitem_status_data_ok; > > >> > > > >> > - qualmsg->link.data = > > >> > le16_to_cpu(hw->qual.cq_curr_bss); > > >> > - qualmsg->level.data = > > >> > - le16_to_cpu(hw->qual.asl_curr_bss); > > >> > - qualmsg->noise.data = > > >> > le16_to_cpu(hw->qual.anl_curr_fc); > > >> > + qualmsg->link.data = hw->qual.cq_curr_bss; > > >> > + qualmsg->level.data = hw->qual.asl_curr_bss; > > >> > + qualmsg->noise.data = hw->qual.anl_curr_fc; > > >> > qualmsg->txrate.data = hw->txrate; > > >> > > > >> > break; > > >> > @@ -603,10 +602,10 @@ static int prism2sta_getcardinfo(struct > > >> > wlandevice *wlandev) > > >> > } > > >> > > > >> > /* get all the nic id fields in host byte order */ > > >> > - hw->ident_nic.id = le16_to_cpu(hw->ident_nic.id); > > >> > - hw->ident_nic.variant = le16_to_cpu(hw->ident_nic.variant); > > >> > - hw->ident_nic.major = le16_to_cpu(hw->ident_nic.major); > > >> > - hw->ident_nic.minor = le16_to_cpu(hw->ident_nic.minor); > > >> > + hw->ident_nic.id = hw->ident_nic.id; > > >> > + hw->ident_nic.variant = hw->ident_nic.variant; > > >> > + hw->ident_nic.major = hw->ident_nic.major; > > >> > + hw->ident_nic.minor = hw->ident_nic.minor; > > >> > > > >> > netdev_info(wlandev->netdev, "ident: nic h/w: id=0x%02x > > >> > %d.%d.%d\n", > > >> > hw->ident_nic.id, hw->ident_nic.major, > > >> > @@ -622,10 +621,10 @@ static int prism2sta_getcardinfo(struct > > >> > wlandevice *wlandev) > > >> > } > > >> > > > >> > /* get all the private fw id fields in host byte order */ > > >> > - hw->ident_pri_fw.id = le16_to_cpu(hw->ident_pri_fw.id); > > >> > - hw->ident_pri_fw.variant = le16_to_cpu(hw->ident_pri_fw.variant); > > >> > - hw->ident_pri_fw.major = le16_to_cpu(hw->ident_pri_fw.major); > > >> > - hw->ident_pri_fw.minor = le16_to_cpu(hw->ident_pri_fw.minor); > > >> > + hw->ident_pri_fw.id = hw->ident_pri_fw.id; > > >> > + hw->ident_pri_fw.variant = hw->ident_pri_fw.variant; > > >> > + hw->ident_pri_fw.major = hw->ident_pri_fw.major; > > >> > + hw->ident_pri_fw.minor = hw->ident_pri_fw.minor; > > >> > > > >> > netdev_info(wlandev->netdev, "ident: pri f/w: id=0x%02x > > >> > %d.%d.%d\n", > > >> > hw->ident_pri_fw.id, hw->ident_pri_fw.major, > > >> > @@ -648,10 +647,10 @@ static int prism2sta_
Re: [Outreachy kernel] [PATCH 1/4] staging: speakup: Placed Logical on the previous line
On Thu, Mar 02, 2017 at 09:05:57PM +0530, Arushi Singhal wrote: > Placed Logical continuations on the previous line as reported by > checkpatch.pl. > > Signed-off-by: Arushi Singhal Hi Arushi, I'm not seeing the patch cover letter for this one. That would be your [PATCH 0/4] and would come first and then 1 through 4 follow threaded as your've done. Stating in imperative might look like this: place logical continuation on previous line insert spaces around operator use tabs for indentation align open parenthesis I didn't look into the changes themselves. alisons > --- > drivers/staging/speakup/main.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c > index a12ec2b061fe..25acebb9311f 100644 > --- a/drivers/staging/speakup/main.c > +++ b/drivers/staging/speakup/main.c > @@ -2144,10 +2144,10 @@ speakup_key(struct vc_data *vc, int shift_state, int > keycode, u_short keysym, > if (up_flag || spk_killed || type == KT_SHIFT) > goto out; > spk_shut_up &= 0xfe; > - kh = (value == KVAL(K_DOWN)) > - || (value == KVAL(K_UP)) > - || (value == KVAL(K_LEFT)) > - || (value == KVAL(K_RIGHT)); > + kh = (value == KVAL(K_DOWN)) || > + (value == KVAL(K_UP)) || > + (value == KVAL(K_LEFT)) || > + (value == KVAL(K_RIGHT)); > if ((cursor_track != read_all_mode) || !kh) > if (!spk_no_intr) > spk_do_flush(); > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170302153600.13803-1-arushisinghal19971997%40gmail.com. > For more options, visit https://groups.google.com/d/optout.
Re: [Outreachy kernel] [PATCH v2] staging: speakup: Comparison to NULL could be written
On Thu, Mar 02, 2017 at 02:13:02PM +0530, Arushi Singhal wrote:
> Fixed coding style for null comparisons in speakup driver to be more
> consistant with the rest of the kernel coding style.
>
> Signed-off-by: Arushi Singhal
> ---
> changes in v2
> - fixed coding style error and upto the coding style.
Hi Arushi,
Take another look at Joe's message about checkpatch on the patch
file. Looks like you missed that.
Here's some tips on styling your commit and log messages:
You've probably seen feedback of putting the message into
the "imperative". ie...it's as if you are directing/commanding
what is to happen. Best way I've found to get that to sink in
is to look at your predecessors...and look at more than one
because poor messages do sneak in occasionally.
For this one, I might do this:
> staging/speakup$ gitpretty *.c | grep NULL
> (my alias: alias gitpretty='git log --pretty=oneline --abbrev-commit')
>
> d3da1cb staging: speakup: (coding style) Rewrite comparisons to NULL
> a90624c Staging: speakup: kobjects.c: Remove explicit NULL comparison
> 114885e Staging: speakup: serialio.c: Remove explicit NULL comparison
> 562c4798 Staging: speakup: devsynth.c: Remove explicit NULL comparison
> ff52fc3 Staging: speakup: varhandlers.c: Remove explicit NULL comparison
>
Then, go further into one that looks like it might match your change:
> staging/speakup$ git log a90624c
> commit a90624cf253cc74e9464b42d54aa4825575edefe
> Author: Shraddha Barke
> Date: Fri Sep 11 11:32:28 2015 +0530
>
> Staging: speakup: kobjects.c: Remove explicit NULL comparison
> >
> Remove explicit NULL comparison and write it in its simpler form.
>
>
This would give me confidence in the commit message and log.
And, since I tend toward the obsessive ;), if the next day I do this
fix in another directory, I would repeat this process on that directory
and file because styles can vary by driver/subsystem. Perhaps not on
such a simple fix, but more so as you dive deeper.
alisons
>
> drivers/staging/speakup/fakekey.c | 2 +-
> drivers/staging/speakup/kobjects.c | 2 +-
> drivers/staging/speakup/main.c | 38
> +++---
> 3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/speakup/fakekey.c
> b/drivers/staging/speakup/fakekey.c
> index d76da0a1382c..294c74b47224 100644
> --- a/drivers/staging/speakup/fakekey.c
> +++ b/drivers/staging/speakup/fakekey.c
> @@ -56,7 +56,7 @@ int speakup_add_virtual_keyboard(void)
>
> void speakup_remove_virtual_keyboard(void)
> {
> - if (virt_keyboard != NULL) {
> + if (virt_keyboard) {
> input_unregister_device(virt_keyboard);
> virt_keyboard = NULL;
> }
> diff --git a/drivers/staging/speakup/kobjects.c
> b/drivers/staging/speakup/kobjects.c
> index 5d871ec3693c..2fef55569bfd 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> len--;
> new_synth_name[len] = '\0';
> spk_strlwr(new_synth_name);
> - if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
> + if (synth && !strcmp(new_synth_name, synth->name)) {
> pr_warn("%s already in use\n", new_synth_name);
> } else if (synth_init(new_synth_name) != 0) {
> pr_warn("failed to init synth %s\n", new_synth_name);
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index c2f70ef5b9b3..a12ec2b061fe 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -299,7 +299,7 @@ static void speakup_shut_up(struct vc_data *vc)
> spk_shut_up |= 0x01;
> spk_parked &= 0xfe;
> speakup_date(vc);
> - if (synth != NULL)
> + if (synth)
> spk_do_flush();
> }
>
> @@ -441,7 +441,7 @@ static void speak_char(u_char ch)
> synth_printf("%s", spk_str_caps_stop);
> return;
> }
> - if (cp == NULL) {
> + if (!cp) {
> pr_info("speak_char: cp == NULL!\n");
> return;
> }
> @@ -1157,7 +1157,7 @@ static void do_handle_shift(struct vc_data *vc, u_char
> value, char up_flag)
> {
> unsigned long flags;
>
> - if (synth == NULL || up_flag || spk_killed)
> + if (!synth || up_flag || spk_killed)
> return;
> spin_lock_irqsave(&speakup_info.spinlock, flags);
> if (cursor_track == read_all_mode) {
> @@ -1195,7 +1195,7 @@ static void do_handle_latin(struct vc_data *vc, u_char
> value, char up_flag)
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> return;
> }
> - if (synth == NULL || spk_killed) {
> + if (!synth || spk_killed) {
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> return;
> }
> @@ -1279,7 +1279,7 @@ void spk_reset_def
[PATCH] iio: trigger: close race condition in acquiring trigger reference
In iio_trigger_write_current() we find the trigger we want while
holding mutex on the list of triggers, but we don't actually do a
get on it while holding mutex. We wait until further validations
are completed and we're sure it's the one we want. Race condition
is that it could be freed by the time we do the get.
Solution is to grab the trigger (iio_trigger_get) as soon as we
find it while holding mutex on the list of triggers. If later
we decide it's not the right one, put it back. (iio_trigger_put).
Signed-off-by: Alison Schofield
Suggested-by: Lars-Peter Clausen
---
Not directly related to this patch, but wondering about the
behavior when userspace tries to set an invalid current_trigger.
If the given trigger name is not found on the list, we don't
simply quit, we proceed. Net result is the old trigger is
removed, the current trigger is set to NULL, and we return
success to userspace. This looks like desired behavior
since it is very intentionally coded. Perhaps it is the
method used to remove a trigger?
Is that what we want?
drivers/iio/industrialio-trigger.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/industrialio-trigger.c
b/drivers/iio/industrialio-trigger.c
index 978729f..d0d869e 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -147,8 +147,7 @@ static struct iio_trigger *__iio_trigger_find_by_name(const
char *name)
return NULL;
}
-static struct iio_trigger *iio_trigger_find_by_name(const char *name,
- size_t len)
+static struct iio_trigger *iio_trigger_acquire_by_name(const char *name)
{
struct iio_trigger *trig = NULL, *iter;
@@ -156,6 +155,7 @@ static struct iio_trigger *iio_trigger_find_by_name(const
char *name,
list_for_each_entry(iter, &iio_trigger_list, list)
if (sysfs_streq(iter->name, name)) {
trig = iter;
+ iio_trigger_get(trig);
break;
}
mutex_unlock(&iio_trigger_list_lock);
@@ -416,20 +416,22 @@ static ssize_t iio_trigger_write_current(struct device
*dev,
}
mutex_unlock(&indio_dev->mlock);
- trig = iio_trigger_find_by_name(buf, len);
- if (oldtrig == trig)
- return len;
+ trig = iio_trigger_acquire_by_name(buf);
+ if (oldtrig == trig) {
+ ret = len;
+ goto out_trigger_put;
+ }
if (trig && indio_dev->info->validate_trigger) {
ret = indio_dev->info->validate_trigger(indio_dev, trig);
if (ret)
- return ret;
+ goto out_trigger_put;
}
if (trig && trig->ops->validate_device) {
ret = trig->ops->validate_device(trig, indio_dev);
if (ret)
- return ret;
+ goto out_trigger_put;
}
indio_dev->trig = trig;
@@ -441,13 +443,16 @@ static ssize_t iio_trigger_write_current(struct device
*dev,
iio_trigger_put(oldtrig);
}
if (indio_dev->trig) {
- iio_trigger_get(indio_dev->trig);
if (indio_dev->modes & INDIO_EVENT_TRIGGERED)
iio_trigger_attach_poll_func(indio_dev->trig,
indio_dev->pollfunc_event);
}
return len;
+
+out_trigger_put:
+ iio_trigger_put(trig);
+ return ret;
}
static DEVICE_ATTR(current_trigger, S_IRUGO | S_IWUSR,
--
2.1.4
[PATCH] iio: proximity: sx9500: claim direct mode during raw proximity reads
Driver was checking for direct mode but not locking it. Use the
claim/release helper functions to guarantee the device stays in
direct mode during raw reads of proximity data.
Signed-off-by: Alison Schofield
---
drivers/iio/proximity/sx9500.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/proximity/sx9500.c b/drivers/iio/proximity/sx9500.c
index 1f06282..9ea147f 100644
--- a/drivers/iio/proximity/sx9500.c
+++ b/drivers/iio/proximity/sx9500.c
@@ -387,14 +387,18 @@ static int sx9500_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct sx9500_data *data = iio_priv(indio_dev);
+ int ret;
switch (chan->type) {
case IIO_PROXIMITY:
switch (mask) {
case IIO_CHAN_INFO_RAW:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
- return sx9500_read_proximity(data, chan, val);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+ ret = sx9500_read_proximity(data, chan, val);
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9500_read_samp_freq(data, val, val2);
default:
--
2.1.4
[PATCH] iio: magnetometer: mag3110: claim direct mode during raw writes
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during raw writes.
Signed-off-by: Alison Schofield
---
drivers/iio/magnetometer/mag3110.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/magnetometer/mag3110.c
b/drivers/iio/magnetometer/mag3110.c
index f2b3bd7..b4f643f 100644
--- a/drivers/iio/magnetometer/mag3110.c
+++ b/drivers/iio/magnetometer/mag3110.c
@@ -222,29 +222,39 @@ static int mag3110_write_raw(struct iio_dev *indio_dev,
int val, int val2, long mask)
{
struct mag3110_data *data = iio_priv(indio_dev);
- int rate;
+ int rate, ret;
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (mask) {
case IIO_CHAN_INFO_SAMP_FREQ:
rate = mag3110_get_samp_freq_index(data, val, val2);
- if (rate < 0)
- return -EINVAL;
+ if (rate < 0) {
+ ret = -EINVAL;
+ break;
+ }
data->ctrl_reg1 &= ~MAG3110_CTRL_DR_MASK;
data->ctrl_reg1 |= rate << MAG3110_CTRL_DR_SHIFT;
- return i2c_smbus_write_byte_data(data->client,
+ ret = i2c_smbus_write_byte_data(data->client,
MAG3110_CTRL_REG1, data->ctrl_reg1);
+ break;
case IIO_CHAN_INFO_CALIBBIAS:
- if (val < -1 || val > 1)
- return -EINVAL;
- return i2c_smbus_write_word_swapped(data->client,
+ if (val < -1 || val > 1) {
+ ret = -EINVAL;
+ break;
+ }
+ ret = i2c_smbus_write_word_swapped(data->client,
MAG3110_OFF_X + 2 * chan->scan_index, val << 1);
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
}
static irqreturn_t mag3110_trigger_handler(int irq, void *p)
--
2.1.4
[PATCH] iio: pressure: ms5611: claim direct mode during oversampling changes
Driver was checking for direct mode before changing oversampling
ratios, but was not locking it. Use the claim/release helper
functions to guarantee the device stays in direct mode while the
oversampling ratios are being updated. Continue to use the drivers
private state lock to protect against conflicting direct mode access
of the state data.
Signed-off-by: Alison Schofield
---
drivers/iio/pressure/ms5611_core.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/pressure/ms5611_core.c
b/drivers/iio/pressure/ms5611_core.c
index 6bd53e7..2a77a2f 100644
--- a/drivers/iio/pressure/ms5611_core.c
+++ b/drivers/iio/pressure/ms5611_core.c
@@ -308,6 +308,7 @@ static int ms5611_write_raw(struct iio_dev *indio_dev,
{
struct ms5611_state *st = iio_priv(indio_dev);
const struct ms5611_osr *osr = NULL;
+ int ret;
if (mask != IIO_CHAN_INFO_OVERSAMPLING_RATIO)
return -EINVAL;
@@ -321,12 +322,11 @@ static int ms5611_write_raw(struct iio_dev *indio_dev,
if (!osr)
return -EINVAL;
- mutex_lock(&st->lock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&st->lock);
- return -EBUSY;
- }
+ mutex_lock(&st->lock);
if (chan->type == IIO_TEMP)
st->temp_osr = osr;
@@ -334,6 +334,8 @@ static int ms5611_write_raw(struct iio_dev *indio_dev,
st->pressure_osr = osr;
mutex_unlock(&st->lock);
+ iio_device_release_direct_mode(indio_dev);
+
return 0;
}
--
2.1.4
[PATCH v2] iio: trigger: free trigger resource correctly
These stand-alone trigger drivers were using iio_trigger_put()
where they should have been using iio_trigger_free(). The
iio_trigger_put() adds a module_put which is bad since they
never did a module_get.
In the sysfs driver, module_get/put's are used as triggers are
added & removed. This extra module_put() occurs on an error path
in the probe routine (probably rare).
In the bfin-timer & interrupt trigger drivers, the module resources
are not explicitly managed, so it's doing a put on something that
was never get'd. It occurs on the probe error path and on the
remove path (not so rare).
Tested with the sysfs trigger driver.
The bfin & interrupt drivers were build tested & inspected only.
Signed-off-by: Alison Schofield
---
Changes in v2:
Renamed probe jump label to say free instead of put.
Added further explanation to commit log.
Note from v1:
Patches to use devm_* funcs are ready to follow this for
the interrupt & bfin-timer triggers.
drivers/iio/trigger/iio-trig-interrupt.c | 8
drivers/iio/trigger/iio-trig-sysfs.c | 2 +-
drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/trigger/iio-trig-interrupt.c
b/drivers/iio/trigger/iio-trig-interrupt.c
index 572bc6f..e18f12b 100644
--- a/drivers/iio/trigger/iio-trig-interrupt.c
+++ b/drivers/iio/trigger/iio-trig-interrupt.c
@@ -58,7 +58,7 @@ static int iio_interrupt_trigger_probe(struct platform_device
*pdev)
trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
if (!trig_info) {
ret = -ENOMEM;
- goto error_put_trigger;
+ goto error_free_trigger;
}
iio_trigger_set_drvdata(trig, trig_info);
trig_info->irq = irq;
@@ -83,8 +83,8 @@ static int iio_interrupt_trigger_probe(struct platform_device
*pdev)
free_irq(irq, trig);
error_free_trig_info:
kfree(trig_info);
-error_put_trigger:
- iio_trigger_put(trig);
+error_free_trigger:
+ iio_trigger_free(trig);
error_ret:
return ret;
}
@@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct
platform_device *pdev)
iio_trigger_unregister(trig);
free_irq(trig_info->irq, trig);
kfree(trig_info);
- iio_trigger_put(trig);
+ iio_trigger_free(trig);
return 0;
}
diff --git a/drivers/iio/trigger/iio-trig-sysfs.c
b/drivers/iio/trigger/iio-trig-sysfs.c
index 3dfab2b..202e8b8 100644
--- a/drivers/iio/trigger/iio-trig-sysfs.c
+++ b/drivers/iio/trigger/iio-trig-sysfs.c
@@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id)
return 0;
out2:
- iio_trigger_put(t->trig);
+ iio_trigger_free(t->trig);
free_t:
kfree(t);
out1:
diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
index 9658f20..4e0b4ee 100644
--- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
+++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
@@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct
platform_device *pdev)
out1:
iio_trigger_unregister(st->trig);
out:
- iio_trigger_put(st->trig);
+ iio_trigger_free(st->trig);
return ret;
}
@@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct
platform_device *pdev)
peripheral_free(st->t->pin);
free_irq(st->irq, st);
iio_trigger_unregister(st->trig);
- iio_trigger_put(st->trig);
+ iio_trigger_free(st->trig);
return 0;
}
--
2.1.4
Re: [PATCH] iio: trigger: free trigger resource correctly
On Wed, Jan 18, 2017 at 12:56:29PM +0300, Dan Carpenter wrote: > On Tue, Jan 17, 2017 at 07:00:28PM -0800, Alison Schofield wrote: > > Using iio_trigger_put() to free a trigger leads to release of > > a resource we never held. Replace with iio_trigger_free(). > > They're basically the same except iio_trigger_put() puts the module and > the device and free only puts the device. > > I've looked at this briefly, but I can't figure out how iio_trigger_get/ > iio_trigger_put is supposed to be used. There isn't any documentation. > I'm trying to review this code, but I can't figure out where we *are* > supposed to be doing the put. > > For example, iio_device_unregister_trigger_consumer() takes a put, but > iio_device_register_trigger_consumer() doesn't do a get... It's all > very confusing. > > You seem like you know what's going on. Can we get some documentation? > Dan, Although I'm comfortable within the bounds of this fix (pair the alloc & free's and do not _put the module resource we never _get'd) beyond that, not so much. I'm just figuring it out myself. linux-iio experts...pointers, help? alisons > > > > Signed-off-by: Alison Schofield > > --- > > Patches to use devm_* funcs are ready to follow this for > > the interrupt & bfin-timer triggers. > > > > drivers/iio/trigger/iio-trig-interrupt.c | 4 ++-- > > drivers/iio/trigger/iio-trig-sysfs.c | 2 +- > > drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++-- > > 3 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/trigger/iio-trig-interrupt.c > > b/drivers/iio/trigger/iio-trig-interrupt.c > > index 572bc6f..b18e50d 100644 > > --- a/drivers/iio/trigger/iio-trig-interrupt.c > > +++ b/drivers/iio/trigger/iio-trig-interrupt.c > > @@ -84,7 +84,7 @@ static int iio_interrupt_trigger_probe(struct > > platform_device *pdev) > > error_free_trig_info: > > kfree(trig_info); > > error_put_trigger: > > - iio_trigger_put(trig); > > + iio_trigger_free(trig); > > > We could rename this label. > > regards, > dan carpenter >
[PATCH] iio: trigger: free trigger resource correctly
Using iio_trigger_put() to free a trigger leads to release of a resource we never held. Replace with iio_trigger_free(). Signed-off-by: Alison Schofield --- Patches to use devm_* funcs are ready to follow this for the interrupt & bfin-timer triggers. drivers/iio/trigger/iio-trig-interrupt.c | 4 ++-- drivers/iio/trigger/iio-trig-sysfs.c | 2 +- drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c index 572bc6f..b18e50d 100644 --- a/drivers/iio/trigger/iio-trig-interrupt.c +++ b/drivers/iio/trigger/iio-trig-interrupt.c @@ -84,7 +84,7 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev) error_free_trig_info: kfree(trig_info); error_put_trigger: - iio_trigger_put(trig); + iio_trigger_free(trig); error_ret: return ret; } @@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev) iio_trigger_unregister(trig); free_irq(trig_info->irq, trig); kfree(trig_info); - iio_trigger_put(trig); + iio_trigger_free(trig); return 0; } diff --git a/drivers/iio/trigger/iio-trig-sysfs.c b/drivers/iio/trigger/iio-trig-sysfs.c index 3dfab2b..202e8b8 100644 --- a/drivers/iio/trigger/iio-trig-sysfs.c +++ b/drivers/iio/trigger/iio-trig-sysfs.c @@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id) return 0; out2: - iio_trigger_put(t->trig); + iio_trigger_free(t->trig); free_t: kfree(t); out1: diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c index 9658f20..4e0b4ee 100644 --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c @@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) out1: iio_trigger_unregister(st->trig); out: - iio_trigger_put(st->trig); + iio_trigger_free(st->trig); return ret; } @@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev) peripheral_free(st->t->pin); free_irq(st->irq, st); iio_trigger_unregister(st->trig); - iio_trigger_put(st->trig); + iio_trigger_free(st->trig); return 0; } -- 2.1.4
[PATCH] iio: adc: palmas_gpadc: retrieve a valid iio_dev in suspend/resume
The suspend/resume functions were using dev_to_iio_dev() to get
the iio_dev. That only works on IIO dev's. Use dev_get_drvdata()
for a platform device to get the correct iio_dev.
Signed-off-by: Alison Schofield
---
drivers/iio/adc/palmas_gpadc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
index 2bbf0c5..7d61b56 100644
--- a/drivers/iio/adc/palmas_gpadc.c
+++ b/drivers/iio/adc/palmas_gpadc.c
@@ -775,7 +775,7 @@ static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc)
static int palmas_gpadc_suspend(struct device *dev)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct palmas_gpadc *adc = iio_priv(indio_dev);
int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
int ret;
@@ -798,7 +798,7 @@ static int palmas_gpadc_suspend(struct device *dev)
static int palmas_gpadc_resume(struct device *dev)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
struct palmas_gpadc *adc = iio_priv(indio_dev);
int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
int ret;
--
2.1.4
Re: [PATCH] iio: health: afe4403: retrieve a valid iio_dev in suspend/resume
On Mon, Jan 16, 2017 at 12:22:03PM -0600, Andrew F. Davis wrote:
> On 01/16/2017 12:10 PM, Alison Schofield wrote:
> > On Mon, Jan 16, 2017 at 10:38:27AM -0600, Andrew F. Davis wrote:
> >> On 01/14/2017 09:51 PM, Alison Schofield wrote:
> >>> The suspend/resume functions were using dev_to_iio_dev() to get
> >>> the iio_dev. That only works on IIO dev's. Replace it with spi
> >>> functions to get the correct iio_dev.
> >>>
> >>> Signed-off-by: Alison Schofield
> >>
> >> Was this found with an automated tool? If not, it might be nice to have
> >> a Coccinelle style check for this. Anyway for this and the afe4404
> >> version patch:
> >>
> >> Acked-by: Andrew F. Davis
> >>
> > Hi Andrew,
> >
> > Just caught my eye while looking at these drivers for another reason:
> > they popped up in a cocci scan looking for drivers with regmap and dev
> > struct in global data. The dev struct may be redundant since it
> > can be retrieved from regmap. I'm going to look at that a bit further
> > and send a patch if appropriate.
> >
>
> I have seen these patches before, and I always NACK them when I can. My
> stance is that just because we can find creative ways to recover
> information doesn't mean we should. It doesn't and anything and it is
> much more clear to me to just keep a copy of *dev to reference when
> needed, rather than some round-about method of fetching it through other
> entities.
Thanks for the heads up...removed from queue.
alisons
>
> > Back to dev_to_iio_dev...it's a case of history repeating itself, but
> > not so often anymore. I grep'd one more that I'll patch. So, although
> > it appears close to extinction, a cocci check would be great. It would
> > be even more valuble if worked with a more general check of the IIO
> > conversion/naming functions that are ripe for misuse.
> >
>
> I'm not a huge fan of all these macro tricks to fetch types for this
> reason here, we lose visibility into the containing types. Like here we
> have a function (dev_to_iio_dev) that goes from dev to iio_dev with no
> real compile time way to know if it is the *right* dev type...
>
> > I'll propose this as an intern task.
> >
> > If anyone has read this far and wants to chime in with their favorite
> > misused funcs to add to the todo list...please do.
> >
> > thanks,
> > alisons
> >
> >>> ---
> >>> drivers/iio/health/afe4403.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
> >>> index 9a08146..6bb23a4 100644
> >>> --- a/drivers/iio/health/afe4403.c
> >>> +++ b/drivers/iio/health/afe4403.c
> >>> @@ -422,7 +422,7 @@ MODULE_DEVICE_TABLE(of, afe4403_of_match);
> >>>
> >>> static int __maybe_unused afe4403_suspend(struct device *dev)
> >>> {
> >>> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> + struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
> >>> struct afe4403_data *afe = iio_priv(indio_dev);
> >>> int ret;
> >>>
> >>> @@ -443,7 +443,7 @@ static int __maybe_unused afe4403_suspend(struct
> >>> device *dev)
> >>>
> >>> static int __maybe_unused afe4403_resume(struct device *dev)
> >>> {
> >>> - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> >>> + struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
> >>> struct afe4403_data *afe = iio_priv(indio_dev);
> >>> int ret;
> >>>
> >>>
Re: [PATCH] iio: health: afe4403: retrieve a valid iio_dev in suspend/resume
On Mon, Jan 16, 2017 at 10:38:27AM -0600, Andrew F. Davis wrote:
> On 01/14/2017 09:51 PM, Alison Schofield wrote:
> > The suspend/resume functions were using dev_to_iio_dev() to get
> > the iio_dev. That only works on IIO dev's. Replace it with spi
> > functions to get the correct iio_dev.
> >
> > Signed-off-by: Alison Schofield
>
> Was this found with an automated tool? If not, it might be nice to have
> a Coccinelle style check for this. Anyway for this and the afe4404
> version patch:
>
> Acked-by: Andrew F. Davis
>
Hi Andrew,
Just caught my eye while looking at these drivers for another reason:
they popped up in a cocci scan looking for drivers with regmap and dev
struct in global data. The dev struct may be redundant since it
can be retrieved from regmap. I'm going to look at that a bit further
and send a patch if appropriate.
Back to dev_to_iio_dev...it's a case of history repeating itself, but
not so often anymore. I grep'd one more that I'll patch. So, although
it appears close to extinction, a cocci check would be great. It would
be even more valuble if worked with a more general check of the IIO
conversion/naming functions that are ripe for misuse.
I'll propose this as an intern task.
If anyone has read this far and wants to chime in with their favorite
misused funcs to add to the todo list...please do.
thanks,
alisons
> > ---
> > drivers/iio/health/afe4403.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
> > index 9a08146..6bb23a4 100644
> > --- a/drivers/iio/health/afe4403.c
> > +++ b/drivers/iio/health/afe4403.c
> > @@ -422,7 +422,7 @@ MODULE_DEVICE_TABLE(of, afe4403_of_match);
> >
> > static int __maybe_unused afe4403_suspend(struct device *dev)
> > {
> > - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
> > struct afe4403_data *afe = iio_priv(indio_dev);
> > int ret;
> >
> > @@ -443,7 +443,7 @@ static int __maybe_unused afe4403_suspend(struct device
> > *dev)
> >
> > static int __maybe_unused afe4403_resume(struct device *dev)
> > {
> > - struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > + struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
> > struct afe4403_data *afe = iio_priv(indio_dev);
> > int ret;
> >
> >
[PATCH] iio: bmi160: use variable names for sizeof() operator
Replace the types with the actual variable names when using the
sizeof() operator. This is kernel preferred style as it protects
against future changes to variable type.
Signed-off-by: Alison Schofield
---
drivers/iio/imu/bmi160/bmi160_core.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c
b/drivers/iio/imu/bmi160/bmi160_core.c
index 5355507..1e7c88b 100644
--- a/drivers/iio/imu/bmi160/bmi160_core.c
+++ b/drivers/iio/imu/bmi160/bmi160_core.c
@@ -338,9 +338,9 @@ static int bmi160_get_data(struct bmi160_data *data, int
chan_type,
__le16 sample;
enum bmi160_sensor_type t = bmi160_to_sensor(chan_type);
- reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(__le16);
+ reg = bmi160_regs[t].data + (axis - IIO_MOD_X) * sizeof(sample);
- ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(__le16));
+ ret = regmap_bulk_read(data->regmap, reg, &sample, sizeof(sample));
if (ret < 0)
return ret;
@@ -405,8 +405,8 @@ static irqreturn_t bmi160_trigger_handler(int irq, void *p)
for_each_set_bit(i, indio_dev->active_scan_mask,
indio_dev->masklength) {
- ret = regmap_bulk_read(data->regmap, base + i * sizeof(__le16),
- &sample, sizeof(__le16));
+ ret = regmap_bulk_read(data->regmap, base + i * sizeof(sample),
+ &sample, sizeof(sample));
if (ret < 0)
goto done;
buf[j++] = sample;
--
2.1.4
[PATCH] iio: health: afe4404: retrieve a valid iio_dev in suspend/resume
The suspend/resume functions were using dev_to_iio_dev() to get
the iio_dev. That only works on IIO dev's. Replace it with i2c
functions to get the correct iio_dev.
Signed-off-by: Alison Schofield
---
drivers/iio/health/afe4404.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/health/afe4404.c b/drivers/iio/health/afe4404.c
index 4526640..964f523 100644
--- a/drivers/iio/health/afe4404.c
+++ b/drivers/iio/health/afe4404.c
@@ -428,7 +428,7 @@ MODULE_DEVICE_TABLE(of, afe4404_of_match);
static int __maybe_unused afe4404_suspend(struct device *dev)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
struct afe4404_data *afe = iio_priv(indio_dev);
int ret;
@@ -449,7 +449,7 @@ static int __maybe_unused afe4404_suspend(struct device
*dev)
static int __maybe_unused afe4404_resume(struct device *dev)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
struct afe4404_data *afe = iio_priv(indio_dev);
int ret;
--
2.1.4
[PATCH] iio: health: afe4403: retrieve a valid iio_dev in suspend/resume
The suspend/resume functions were using dev_to_iio_dev() to get
the iio_dev. That only works on IIO dev's. Replace it with spi
functions to get the correct iio_dev.
Signed-off-by: Alison Schofield
---
drivers/iio/health/afe4403.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/health/afe4403.c b/drivers/iio/health/afe4403.c
index 9a08146..6bb23a4 100644
--- a/drivers/iio/health/afe4403.c
+++ b/drivers/iio/health/afe4403.c
@@ -422,7 +422,7 @@ MODULE_DEVICE_TABLE(of, afe4403_of_match);
static int __maybe_unused afe4403_suspend(struct device *dev)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
struct afe4403_data *afe = iio_priv(indio_dev);
int ret;
@@ -443,7 +443,7 @@ static int __maybe_unused afe4403_suspend(struct device
*dev)
static int __maybe_unused afe4403_resume(struct device *dev)
{
- struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev *indio_dev = spi_get_drvdata(to_spi_device(dev));
struct afe4403_data *afe = iio_priv(indio_dev);
int ret;
--
2.1.4
Re: [PATCH v4] iio: humidity: hdc100x: add triggered buffer support for HDC100X
On Sat, Sep 03, 2016 at 08:37:27PM +0100, Jonathan Cameron wrote:
> On 02/09/16 18:23, Alison Schofield wrote:
> > Triggered buffer support uses the HDC100X's dual acquisition mode
> > to read both humidity and temperature in one shot.
> >
> > This patch depends on
> > 447136effbf4 ("iio: humidity: hdc100x: fix sensor data reads of
> > temp and humidity")
> >
> > Signed-off-by: Alison Schofield
> > Cc: Daniel Baluta
> Looks good. I'll have to hold it until the dependency has worked it's way
> into my togreg branch in a week or so.
>
> Feel free to remind me!
>
> Jonathan
It's safe to take this one now. Thanks!
alisons
> > ---
> >
> > Changes in v4:
> > - unlock private lock if claim_direct_mode fails (kbuild robot reported)
> > - move unlock to err: out path in trigger handler (kbuild robot inspired)
> >
> > Changes in v3:
> > Thanks for the review Jonathan
> > - This patch now depends on
> >447136effbf4 ("iio: humidity: hdc100x: fix sensor data reads of temp
> >and humidity")
> > - remove i2c functionality check, done in dependent patch
> > - remove unused define HDC100X_ALL_CHANNEL_MASK
> > - define and use available_scan_masks to demux between temp &/or humid
> >data in buffered mode
> > - changed timestamp channel scan_index from 3 to 2.
> > - put back private data lock in read_raw
> >(we'll lock private data lock and claim_direct_mode on raw reads)
> > - remove unneeded NULL parameters in iio_buffer_setup_ops
> > - add private data lock on buffered read
> >We are going to allow configuration changes while the sensor is in
> >buffered mode. We need to allow the toggling of the Heater Enable bit
> >because the heater only functions during measurements. The lock will
> >ensure that the config change and buffer read transaction do not collide.
> > - blank lines added for readability per reviewer
> >
> > Changes in v2:
> > Thanks for the review Peter
> > - switched endianness from IIO_CPU to IIO_BE
> > - use only one buffer in the handler
> > - simplify selection of temp &/or humidity to push to buffer
> >
> >
> > drivers/iio/humidity/Kconfig | 2 +
> > drivers/iio/humidity/hdc100x.c | 130
> > -
> > 2 files changed, 130 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> > index f155386..5a25569 100644
> > --- a/drivers/iio/humidity/Kconfig
> > +++ b/drivers/iio/humidity/Kconfig
> > @@ -25,6 +25,8 @@ config DHT11
> > config HDC100X
> > tristate "TI HDC100x relative humidity and temperature sensor"
> > depends on I2C
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > help
> > Say yes here to build support for the Texas Instruments
> > HDC1000 and HDC1008 relative humidity and temperature sensors.
> > diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
> > index e0c9c70..265c34d 100644
> > --- a/drivers/iio/humidity/hdc100x.c
> > +++ b/drivers/iio/humidity/hdc100x.c
> > @@ -22,11 +22,15 @@
> >
> > #include
> > #include
> > +#include
> > +#include
> > +#include
> >
> > #define HDC100X_REG_TEMP 0x00
> > #define HDC100X_REG_HUMIDITY 0x01
> >
> > #define HDC100X_REG_CONFIG 0x02
> > +#define HDC100X_REG_CONFIG_ACQ_MODEBIT(12)
> > #define HDC100X_REG_CONFIG_HEATER_EN BIT(13)
> >
> > struct hdc100x_data {
> > @@ -87,22 +91,40 @@ static const struct iio_chan_spec hdc100x_channels[] = {
> > BIT(IIO_CHAN_INFO_SCALE) |
> > BIT(IIO_CHAN_INFO_INT_TIME) |
> > BIT(IIO_CHAN_INFO_OFFSET),
> > + .scan_index = 0,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 16,
> > + .storagebits = 16,
> > + .endianness = IIO_BE,
> > + },
> > },
> > {
> > .type = IIO_HUMIDITYRELATIVE,
> > .address = HDC100X_REG_HUMIDITY,
> > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > BIT(IIO_CHAN_INFO_SCALE) |
> > - BIT(IIO_CHAN_INFO_INT_TIME)
> > +
[PATCH v2] iio: light: ltr501: claim direct mode during raw writes
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during all raw write operations.
Signed-off-by: Alison Schofield
---
Changes in v2:
Replaced 'goto release' statements with breaks.
The release helper function remains in the same place as in version
one of patch, but now break statements control the flow rather than
jumping out with goto's.
I may have 'break'd more than needed at tail end of nested switch.
Tried to follow official c language definition.
drivers/iio/light/ltr501.c | 81 +-
1 file changed, 51 insertions(+), 30 deletions(-)
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 3afc53a..8f9d5cf 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -729,8 +729,9 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
int i, ret, freq_val, freq_val2;
struct ltr501_chip_info *info = data->chip_info;
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -739,85 +740,105 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
i = ltr501_get_gain_index(info->als_gain,
info->als_gain_tbl_size,
val, val2);
- if (i < 0)
- return -EINVAL;
+ if (i < 0) {
+ ret = -EINVAL;
+ break;
+ }
data->als_contr &= ~info->als_gain_mask;
data->als_contr |= i << info->als_gain_shift;
- return regmap_write(data->regmap, LTR501_ALS_CONTR,
- data->als_contr);
+ ret = regmap_write(data->regmap, LTR501_ALS_CONTR,
+ data->als_contr);
+ break;
case IIO_PROXIMITY:
i = ltr501_get_gain_index(info->ps_gain,
info->ps_gain_tbl_size,
val, val2);
- if (i < 0)
- return -EINVAL;
+ if (i < 0) {
+ ret = -EINVAL;
+ break;
+ }
data->ps_contr &= ~LTR501_CONTR_PS_GAIN_MASK;
data->ps_contr |= i << LTR501_CONTR_PS_GAIN_SHIFT;
- return regmap_write(data->regmap, LTR501_PS_CONTR,
- data->ps_contr);
+ ret = regmap_write(data->regmap, LTR501_PS_CONTR,
+ data->ps_contr);
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+ break;
+
case IIO_CHAN_INFO_INT_TIME:
switch (chan->type) {
case IIO_INTENSITY:
- if (val != 0)
- return -EINVAL;
+ if (val != 0) {
+ ret = -EINVAL;
+ break;
+ }
mutex_lock(&data->lock_als);
- i = ltr501_set_it_time(data, val2);
+ ret = ltr501_set_it_time(data, val2);
mutex_unlock(&data->lock_als);
- return i;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+ break;
+
case IIO_CHAN_INFO_SAMP_FREQ:
switch (chan->type) {
case IIO_INTENSITY:
ret = ltr501_als_read_samp_freq(data, &freq_val,
&freq_val2);
if (ret < 0)
- return ret;
+ break;
ret = ltr501_als_write_samp_freq(data, val, val2);
if (ret < 0)
- return ret;
+ break;
/* update persistence count when changing frequency */
ret = ltr50
[PATCH v2] iio: light: ltr501: claim direct mode during select raw reads
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during required raw read cases.
Signed-off-by: Alison Schofield
---
Changes in v2:
Reworked IIO_CHAN_INFO_RAW case so claim and release are
executed at same level in the switch statements. (rather
than claims on top level and releases nested in cases)
drivers/iio/light/ltr501.c | 30 --
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 3afc53a..d577477 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -631,14 +631,16 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
-
switch (chan->type) {
case IIO_LIGHT:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
mutex_lock(&data->lock_als);
ret = ltr501_read_als(data, buf);
mutex_unlock(&data->lock_als);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
*val = ltr501_calculate_lux(le16_to_cpu(buf[1]),
@@ -648,8 +650,9 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
case IIO_CHAN_INFO_RAW:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (chan->type) {
case IIO_INTENSITY:
@@ -657,21 +660,28 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
ret = ltr501_read_als(data, buf);
mutex_unlock(&data->lock_als);
if (ret < 0)
- return ret;
+ break;
*val = le16_to_cpu(chan->address == LTR501_ALS_DATA1 ?
buf[0] : buf[1]);
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
+ break;
case IIO_PROXIMITY:
mutex_lock(&data->lock_ps);
ret = ltr501_read_ps(data);
mutex_unlock(&data->lock_ps);
if (ret < 0)
- return ret;
+ break;
*val = ret & LTR501_PS_DATA_MASK;
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
+ break;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ break;
}
+
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
+
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_INTENSITY:
--
2.1.4
Re: [PATCH] iio: accel: mma8452: claim direct mode during write raw
On Sat, Oct 15, 2016 at 04:05:24PM +0100, Jonathan Cameron wrote:
> On 15/10/16 15:55, Jonathan Cameron wrote:
> > On 11/10/16 20:32, Alison Schofield wrote:
> >> Driver was checking for direct mode but not locking it. Use
> >> claim/release helper functions to guarantee the device stays
> >> in direct mode during all write raw operations.
> >>
> >> Signed-off-by: Alison Schofield
> > Well you were consistent. Forgot breaks that needed adding everywhere ;)
> >
> > I fixed up and applied to the togreg branch of iio.git - initially
> > pushed out as testing for the autobuilders to play with it.
> >
> > Please sanity check I didn't mess it up!
> Just noticed, that break would work better than goto here as well
> so have made that change and pushed out.
>
> Let me know if i messed it up.
Thanks Jonathan. I've looked at the ones you've repaired this
morning and see the errors in my usage of return/goto/break in
the case statements. Also sanity checked your fix-ups in
iio.git and they look right, of course :)
I'll fix up the ltr501 drivers and resend.
alisons
>
> Jonathan
> >
> > Thanks,
> >
> > Jonathan
> >> ---
> >> drivers/iio/accel/mma8452.c | 53
> >> +++--
> >> 1 file changed, 32 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> >> index 7951def..cbbc746 100644
> >> --- a/drivers/iio/accel/mma8452.c
> >> +++ b/drivers/iio/accel/mma8452.c
> >> @@ -666,37 +666,43 @@ static int mma8452_write_raw(struct iio_dev
> >> *indio_dev,
> >>struct mma8452_data *data = iio_priv(indio_dev);
> >>int i, ret;
> >>
> >> - if (iio_buffer_enabled(indio_dev))
> >> - return -EBUSY;
> >> + ret = iio_device_claim_direct_mode(indio_dev);
> >> + if (ret)
> >> + return ret;
> >>
> >>switch (mask) {
> >>case IIO_CHAN_INFO_SAMP_FREQ:
> >>i = mma8452_get_samp_freq_index(data, val, val2);
> >> - if (i < 0)
> >> - return i;
> >> -
> >> + if (i < 0) {
> >> + ret = i;
> >> + goto release;
> >> + }
> >>data->ctrl_reg1 &= ~MMA8452_CTRL_DR_MASK;
> >>data->ctrl_reg1 |= i << MMA8452_CTRL_DR_SHIFT;
> >>
> >> - return mma8452_change_config(data, MMA8452_CTRL_REG1,
> >> - data->ctrl_reg1);
> >> + ret = mma8452_change_config(data, MMA8452_CTRL_REG1,
> >> + data->ctrl_reg1);
> > break?
> >>case IIO_CHAN_INFO_SCALE:
> >>i = mma8452_get_scale_index(data, val, val2);
> >> - if (i < 0)
> >> - return i;
> >> + if (i < 0) {
> >> + ret = i;
> >> + goto release;
> >> + }
> >>
> >>data->data_cfg &= ~MMA8452_DATA_CFG_FS_MASK;
> >>data->data_cfg |= i;
> >>
> >> - return mma8452_change_config(data, MMA8452_DATA_CFG,
> >> - data->data_cfg);
> >> + ret = mma8452_change_config(data, MMA8452_DATA_CFG,
> >> + data->data_cfg);
> > break?
> >>case IIO_CHAN_INFO_CALIBBIAS:
> >> - if (val < -128 || val > 127)
> >> - return -EINVAL;
> >> + if (val < -128 || val > 127) {
> >> + ret = -EINVAL;
> >> + goto release;
> >> + }
> >>
> >> - return mma8452_change_config(data,
> >> - MMA8452_OFF_X + chan->scan_index,
> >> - val);
> >> + ret = mma8452_change_config(data,
> >> + MMA8452_OFF_X + chan->scan_index,
> >> + val);
> > break?
> >>
> >>case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> >>if (val == 0 && val2 == 0) {
> >> @@ -705,23 +711,28 @@ static int mma8452_write_raw(struct iio_dev
> >> *indio_dev,
> >>
[PATCH] iio: accel: mma8452: claim direct mode during write raw
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during all write raw operations.
Signed-off-by: Alison Schofield
---
drivers/iio/accel/mma8452.c | 53 +++--
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7951def..cbbc746 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -666,37 +666,43 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
struct mma8452_data *data = iio_priv(indio_dev);
int i, ret;
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (mask) {
case IIO_CHAN_INFO_SAMP_FREQ:
i = mma8452_get_samp_freq_index(data, val, val2);
- if (i < 0)
- return i;
-
+ if (i < 0) {
+ ret = i;
+ goto release;
+ }
data->ctrl_reg1 &= ~MMA8452_CTRL_DR_MASK;
data->ctrl_reg1 |= i << MMA8452_CTRL_DR_SHIFT;
- return mma8452_change_config(data, MMA8452_CTRL_REG1,
-data->ctrl_reg1);
+ ret = mma8452_change_config(data, MMA8452_CTRL_REG1,
+ data->ctrl_reg1);
case IIO_CHAN_INFO_SCALE:
i = mma8452_get_scale_index(data, val, val2);
- if (i < 0)
- return i;
+ if (i < 0) {
+ ret = i;
+ goto release;
+ }
data->data_cfg &= ~MMA8452_DATA_CFG_FS_MASK;
data->data_cfg |= i;
- return mma8452_change_config(data, MMA8452_DATA_CFG,
-data->data_cfg);
+ ret = mma8452_change_config(data, MMA8452_DATA_CFG,
+ data->data_cfg);
case IIO_CHAN_INFO_CALIBBIAS:
- if (val < -128 || val > 127)
- return -EINVAL;
+ if (val < -128 || val > 127) {
+ ret = -EINVAL;
+ goto release;
+ }
- return mma8452_change_config(data,
-MMA8452_OFF_X + chan->scan_index,
-val);
+ ret = mma8452_change_config(data,
+ MMA8452_OFF_X + chan->scan_index,
+ val);
case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
if (val == 0 && val2 == 0) {
@@ -705,23 +711,28 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
data->data_cfg |= MMA8452_DATA_CFG_HPF_MASK;
ret = mma8452_set_hp_filter_frequency(data, val, val2);
if (ret < 0)
- return ret;
+ goto release;
}
- return mma8452_change_config(data, MMA8452_DATA_CFG,
+ ret = mma8452_change_config(data, MMA8452_DATA_CFG,
data->data_cfg);
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
ret = mma8452_get_odr_index(data);
for (i = 0; i < ARRAY_SIZE(mma8452_os_ratio); i++) {
- if (mma8452_os_ratio[i][ret] == val)
- return mma8452_set_power_mode(data, i);
+ if (mma8452_os_ratio[i][ret] == val) {
+ ret = mma8452_set_power_mode(data, i);
+ goto release;
+ }
}
default:
- return -EINVAL;
+ ret = -EINVAL;
}
+release:
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
}
static int mma8452_read_thresh(struct iio_dev *indio_dev,
--
2.1.4
[PATCH] iio: light: ltr501: claim direct mode during raw writes
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during all raw write operations.
Signed-off-by: Alison Schofield
---
drivers/iio/light/ltr501.c | 71 +++---
1 file changed, 42 insertions(+), 29 deletions(-)
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 3afc53a..0f1032d 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -729,8 +729,9 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
int i, ret, freq_val, freq_val2;
struct ltr501_chip_info *info = data->chip_info;
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -739,39 +740,50 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
i = ltr501_get_gain_index(info->als_gain,
info->als_gain_tbl_size,
val, val2);
- if (i < 0)
- return -EINVAL;
+ if (i < 0) {
+ ret = -EINVAL;
+ goto release;
+ }
data->als_contr &= ~info->als_gain_mask;
data->als_contr |= i << info->als_gain_shift;
- return regmap_write(data->regmap, LTR501_ALS_CONTR,
- data->als_contr);
+ ret = regmap_write(data->regmap, LTR501_ALS_CONTR,
+ data->als_contr);
+ goto release;
case IIO_PROXIMITY:
i = ltr501_get_gain_index(info->ps_gain,
info->ps_gain_tbl_size,
val, val2);
- if (i < 0)
- return -EINVAL;
+ if (i < 0) {
+ ret = -EINVAL;
+ goto release;
+ }
data->ps_contr &= ~LTR501_CONTR_PS_GAIN_MASK;
data->ps_contr |= i << LTR501_CONTR_PS_GAIN_SHIFT;
- return regmap_write(data->regmap, LTR501_PS_CONTR,
- data->ps_contr);
+ ret = regmap_write(data->regmap, LTR501_PS_CONTR,
+ data->ps_contr);
+ goto release;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ goto release;
}
case IIO_CHAN_INFO_INT_TIME:
switch (chan->type) {
case IIO_INTENSITY:
- if (val != 0)
- return -EINVAL;
+ if (val != 0) {
+ ret = -EINVAL;
+ goto release;
+ }
mutex_lock(&data->lock_als);
i = ltr501_set_it_time(data, val2);
mutex_unlock(&data->lock_als);
- return i;
+ ret = i;
+ goto release;
default:
- return -EINVAL;
+ ret = -EINVAL;
+ goto release;
}
case IIO_CHAN_INFO_SAMP_FREQ:
switch (chan->type) {
@@ -779,45 +791,46 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
ret = ltr501_als_read_samp_freq(data, &freq_val,
&freq_val2);
if (ret < 0)
- return ret;
+ goto release;
ret = ltr501_als_write_samp_freq(data, val, val2);
if (ret < 0)
- return ret;
+ goto release;
/* update persistence count when changing frequency */
ret = ltr501_write_intr_prst(data, chan->type,
0, data->als_period);
if (ret < 0)
- return ltr501_als_write_samp_freq(data,
- freq_val,
-
[PATCH] iio: light: ltr501: claim direct mode during select raw reads
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during required raw read cases.
Signed-off-by: Alison Schofield
---
drivers/iio/light/ltr501.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 3afc53a..c2ac3cd 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -631,14 +631,16 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
-
switch (chan->type) {
case IIO_LIGHT:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
mutex_lock(&data->lock_als);
ret = ltr501_read_als(data, buf);
mutex_unlock(&data->lock_als);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
*val = ltr501_calculate_lux(le16_to_cpu(buf[1]),
@@ -648,14 +650,16 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
case IIO_CHAN_INFO_RAW:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (chan->type) {
case IIO_INTENSITY:
mutex_lock(&data->lock_als);
ret = ltr501_read_als(data, buf);
mutex_unlock(&data->lock_als);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
*val = le16_to_cpu(chan->address == LTR501_ALS_DATA1 ?
@@ -665,11 +669,13 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
mutex_lock(&data->lock_ps);
ret = ltr501_read_ps(data);
mutex_unlock(&data->lock_ps);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
*val = ret & LTR501_PS_DATA_MASK;
return IIO_VAL_INT;
default:
+ iio_device_release_direct_mode(indio_dev);
return -EINVAL;
}
case IIO_CHAN_INFO_SCALE:
--
2.1.4
[PATCH] iio: pressure: mpl3115: claim direct mode during raw reads
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during raw reads.
Signed-off-by: Alison Schofield
---
drivers/iio/pressure/mpl3115.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
index 6392d7b..fb36f7d 100644
--- a/drivers/iio/pressure/mpl3115.c
+++ b/drivers/iio/pressure/mpl3115.c
@@ -82,8 +82,9 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
switch (chan->type) {
case IIO_PRESSURE: /* in 0.25 pascal / LSB */
@@ -91,31 +92,35 @@ static int mpl3115_read_raw(struct iio_dev *indio_dev,
ret = mpl3115_request(data);
if (ret < 0) {
mutex_unlock(&data->lock);
- return ret;
+ goto release;
}
ret = i2c_smbus_read_i2c_block_data(data->client,
MPL3115_OUT_PRESS, 3, (u8 *) &tmp);
mutex_unlock(&data->lock);
if (ret < 0)
- return ret;
+ goto release;
*val = be32_to_cpu(tmp) >> 12;
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
case IIO_TEMP: /* in 0.0625 celsius / LSB */
mutex_lock(&data->lock);
ret = mpl3115_request(data);
if (ret < 0) {
mutex_unlock(&data->lock);
- return ret;
+ goto release;
}
ret = i2c_smbus_read_i2c_block_data(data->client,
MPL3115_OUT_TEMP, 2, (u8 *) &tmp);
mutex_unlock(&data->lock);
if (ret < 0)
- return ret;
+ goto release;
*val = sign_extend32(be32_to_cpu(tmp) >> 20, 11);
- return IIO_VAL_INT;
+ ret = IIO_VAL_INT;
default:
- return -EINVAL;
+ ret = -EINVAL;
+
+release:
+ iio_device_release_direct_mode(indio_dev);
+ return ret;
}
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
--
2.1.4
[PATCH] iio: accel: mma8452: claim direct mode during raw reads
Driver was checking for direct mode but not locking it. Use
claim/release helper functions to guarantee the device stays
in direct mode during raw reads.
Signed-off-by: Alison Schofield
---
drivers/iio/accel/mma8452.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 6e82e02..cbbc746 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -459,12 +459,14 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
mutex_lock(&data->lock);
ret = mma8452_read(data, buffer);
mutex_unlock(&data->lock);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
--
2.1.4
[PATCH] iio: adc: ina2xx: remove unused debug field from chip global data
commit 1961bce76452 "iio: ina2xx: Remove trace_printk debug
statements" removed the code that used the chip->prev_ns field.
This patch cleans it up further by removing the unused field
and assignments.
Signed-off-by: Alison Schofield
Cc: Daniel Baluta
---
drivers/iio/adc/ina2xx-adc.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 955f3fd..59b7d76 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -114,7 +114,6 @@ struct ina2xx_chip_info {
struct mutex state_lock;
unsigned int shunt_resistor;
int avg;
- s64 prev_ns; /* track buffer capture time, check for underruns */
int int_time_vbus; /* Bus voltage integration time uS */
int int_time_vshunt; /* Shunt voltage integration time uS */
bool allow_async_readout;
@@ -509,8 +508,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
iio_push_to_buffers_with_timestamp(indio_dev,
(unsigned int *)data, time_a);
- chip->prev_ns = time_a;
-
return (unsigned long)(time_b - time_a) / 1000;
};
@@ -554,8 +551,6 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev)
dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
chip->allow_async_readout);
- chip->prev_ns = iio_get_time_ns(indio_dev);
-
chip->task = kthread_run(ina2xx_capture_thread, (void *)indio_dev,
"%s:%d-%uus", indio_dev->name, indio_dev->id,
sampling_us);
--
2.1.4
Re: [PATCH v2] iio: adc: ltc2485: add support for Linear Technology LTC2485 ADC
On Mon, Aug 29, 2016 at 05:01:14PM +0100, Jonathan Cameron wrote: > On 25/08/16 06:48, Alison Schofield wrote: > > Adds basic support for the LTC2485 ADC - a delta-sigma analog-to-digital > > converter with an I2C interface that operates in single shot conversion > > mode. > > > > The driver supports an on board 5V reference and the power-on default > > configuration which rejects both 50hz & 60hz line frequencies and > > operates in 1x speed mode. > > > > Signed-off-by: Alison Schofield > > Cc: Daniel Baluta > > --- > > Changes in v2: > > - changed the wait conversion time function to use monotonic rather than > > real time and to use milliseconds rather than nanoseconds. > > - made conv time a constant int. > > > > Same notes as in v1: > > Reviewers: In addition to commenting on what is present, please comment > > on what is absent and the priority in which you would like to see > > additional features added to this driver, or needed in this first submittal. > None of the below are needed for initially submital. > All 'value' add features at the end of the day. Job one: get reading from > ADC done :) > > I made a few really small white space changes and applied this to the > togreg branch of iio.git. Will be initially pushed out as testing > in a few minutes time to check the autobuilders can't find anything. > > Thanks, > > Jonathan > > > > > Not supported: > > 1. External Vref > > 2. Additional Configuration Modes > > Temperature Sensor Mode: report temp data (not voltage). > > > > Line Frequency Rejection Mode: select to reject 50Hz or 60Hz > > or both. (both is default) > This is interesting... Will need some new ABI. It's a filter so we probably > want to try and fit it in with the existing filter ABI. > > > > Speed Mode: Default speed mode (1x) performs two conversions each > > cycle and combines the results. 2x speed mode only does one > > conversion each cycle. > Really simple case of oversampling. So this one is easy ;) > > > 3. Power management > > 4. DT Bindings > > 5. Triggered buffers > Device is really slow, so no real rush on this one. Nice to have for > the convenience of using standard interfaces in libiio etc, but > at 7sps reading sysfs will work fine for all usecases! > > > 6. What else? > Other power supply regulators.. > (allows complete power disabling if there is an appropriate controllable > regulator on the board). > Thanks Jonathan! I need more direction on the power supply regulators. When you say power supply regulator, is that the same as reference voltage? The driver uses onboard vref now. I see how I can add support for an external vref. But, it seems like maybe you are commenting that we can have both, an external vref with an onboard vref as a fallback? Thanks, alisons
Re: A potential race in drivers/iio/adc/vf610_adc.ko
On Fri, Sep 02, 2016 at 11:05:09AM +0300, Pavel Andrianov wrote: > > Hi! > > There is a potential race in drivers/iio/adc/vf610_adc.ko. > Handlers vf610_set_conversion_mode and vf610_write_raw are called via > device_attibute interface, but they are related to different attributes, so > may be executed in parallel. vf610_set_conversion_mode acquires the mutex > indio_dev->mlock, and vf610_write_raw does not. Thus updating the structure > 'info' may be performed simultaneously. > > Should vf610_write_raw also acquire the same mutex indio_dev->mlock? > Hi Pavel, I'm not familiar with the conversion_mode interface, so I'll leave your specific question for someone with that knowledge. Just wanted to point out that if you're going to update the locking in the driver, there are 2 things to consider: 1) Use iio_device_claim_direct_mode() helper functions instead of checking iio_buffer_enabled and grabbing mlock. 2) Any other uses of indio_dev->mlock are best moved to a private data lock. We want to return that mlock to an INTERNAL (core) use only. alisons > -- > Pavel Andrianov > Linux Verification Center, ISPRAS > web: http://linuxtesting.org > e-mail: andria...@ispras.ru > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4] iio: humidity: hdc100x: add triggered buffer support for HDC100X
Triggered buffer support uses the HDC100X's dual acquisition mode
to read both humidity and temperature in one shot.
This patch depends on
447136effbf4 ("iio: humidity: hdc100x: fix sensor data reads of
temp and humidity")
Signed-off-by: Alison Schofield
Cc: Daniel Baluta
---
Changes in v4:
- unlock private lock if claim_direct_mode fails (kbuild robot reported)
- move unlock to err: out path in trigger handler (kbuild robot inspired)
Changes in v3:
Thanks for the review Jonathan
- This patch now depends on
447136effbf4 ("iio: humidity: hdc100x: fix sensor data reads of temp
and humidity")
- remove i2c functionality check, done in dependent patch
- remove unused define HDC100X_ALL_CHANNEL_MASK
- define and use available_scan_masks to demux between temp &/or humid
data in buffered mode
- changed timestamp channel scan_index from 3 to 2.
- put back private data lock in read_raw
(we'll lock private data lock and claim_direct_mode on raw reads)
- remove unneeded NULL parameters in iio_buffer_setup_ops
- add private data lock on buffered read
We are going to allow configuration changes while the sensor is in
buffered mode. We need to allow the toggling of the Heater Enable bit
because the heater only functions during measurements. The lock will
ensure that the config change and buffer read transaction do not collide.
- blank lines added for readability per reviewer
Changes in v2:
Thanks for the review Peter
- switched endianness from IIO_CPU to IIO_BE
- use only one buffer in the handler
- simplify selection of temp &/or humidity to push to buffer
drivers/iio/humidity/Kconfig | 2 +
drivers/iio/humidity/hdc100x.c | 130 -
2 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
index f155386..5a25569 100644
--- a/drivers/iio/humidity/Kconfig
+++ b/drivers/iio/humidity/Kconfig
@@ -25,6 +25,8 @@ config DHT11
config HDC100X
tristate "TI HDC100x relative humidity and temperature sensor"
depends on I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
help
Say yes here to build support for the Texas Instruments
HDC1000 and HDC1008 relative humidity and temperature sensors.
diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index e0c9c70..265c34d 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -22,11 +22,15 @@
#include
#include
+#include
+#include
+#include
#define HDC100X_REG_TEMP 0x00
#define HDC100X_REG_HUMIDITY 0x01
#define HDC100X_REG_CONFIG 0x02
+#define HDC100X_REG_CONFIG_ACQ_MODEBIT(12)
#define HDC100X_REG_CONFIG_HEATER_EN BIT(13)
struct hdc100x_data {
@@ -87,22 +91,40 @@ static const struct iio_chan_spec hdc100x_channels[] = {
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_INT_TIME) |
BIT(IIO_CHAN_INFO_OFFSET),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
},
{
.type = IIO_HUMIDITYRELATIVE,
.address = HDC100X_REG_HUMIDITY,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
- BIT(IIO_CHAN_INFO_INT_TIME)
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
},
{
.type = IIO_CURRENT,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
.extend_name = "heater",
.output = 1,
+ .scan_index = -1,
},
+ IIO_CHAN_SOFT_TIMESTAMP(2),
};
+static const unsigned long hdc100x_scan_masks[] = {0x3, 0};
+
static int hdc100x_update_config(struct hdc100x_data *data, int mask, int val)
{
int tmp = (~mask & data->config) | val;
@@ -183,7 +205,14 @@ static int hdc100x_read_raw(struct iio_dev *indio_dev,
*val = hdc100x_get_heater_status(data);
ret = IIO_VAL_INT;
} else {
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret) {
+ mutex_unlock(&data->lock);
+ return ret;
Re: [PATCH v3] iio: humidity: hdc100x: add triggered buffer support for HDC100X
On Fri, Sep 02, 2016 at 07:30:38AM +0100, Julia Lawall wrote:
> Please check on the lines marked with @. An unlock may be needed at line
> 210.
>
> julia
Yes, needs unlock. Thanks!
alisons
>
> -- Forwarded message --
> Date: Fri, 2 Sep 2016 07:19:23 +0800
> From: kbuild test robot
> To: kbu...@01.org
> Cc: Julia Lawall
> Subject: Re: [PATCH v3] iio: humidity: hdc100x: add triggered buffer support
> for
> HDC100X
>
> Hi Alison,
>
> [auto build test WARNING on next-20160825]
> [also build test WARNING on v4.8-rc4]
> [cannot apply to iio/togreg v4.8-rc4 v4.8-rc3 v4.8-rc2]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for
> convenience) to record what (public, well-known) commit your patch series was
> built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
>
> url:
> https://github.com/0day-ci/linux/commits/Alison-Schofield/iio-humidity-hdc100x-add-triggered-buffer-support-for-HDC100X/20160902-055115
> :: branch date: 88 minutes ago
> :: commit date: 88 minutes ago
>
> >> drivers/iio/humidity/hdc100x.c:210:4-10: preceding lock on line 203
>drivers/iio/humidity/hdc100x.c:345:1-7: preceding lock on line 325
>
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout f6862a537522f8d668f7daec0aa0817f87706017
> vim +210 drivers/iio/humidity/hdc100x.c
>
> 4839367d9 Matt Ranostay2015-09-06 197struct hdc100x_data *data =
> iio_priv(indio_dev);
> 4839367d9 Matt Ranostay2015-09-06 198
> 4839367d9 Matt Ranostay2015-09-06 199switch (mask) {
> 4839367d9 Matt Ranostay2015-09-06 200case IIO_CHAN_INFO_RAW: {
> 4839367d9 Matt Ranostay2015-09-06 201int ret;
> 4839367d9 Matt Ranostay2015-09-06 202
> 4839367d9 Matt Ranostay2015-09-06 @203mutex_lock(&data->lock);
> 4839367d9 Matt Ranostay2015-09-06 204if (chan->type ==
> IIO_CURRENT) {
> 4839367d9 Matt Ranostay2015-09-06 205*val =
> hdc100x_get_heater_status(data);
> 4839367d9 Matt Ranostay2015-09-06 206ret =
> IIO_VAL_INT;
> 4839367d9 Matt Ranostay 2015-09-06 207} else {
> f6862a537 Alison Schofield 2016-09-01 208ret =
> iio_device_claim_direct_mode(indio_dev);
> f6862a537 Alison Schofield 2016-09-01 209if (ret)
> f6862a537 Alison Schofield 2016-09-01 @210 return
> ret;
> f6862a537 Alison Schofield 2016-09-01 211
> 4839367d9 Matt Ranostay2015-09-06 212ret =
> hdc100x_get_measurement(data, chan);
> f6862a537 Alison Schofield 2016-09-01 213
> iio_device_release_direct_mode(indio_dev);
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH v3] iio: humidity: hdc100x: add triggered buffer support for HDC100X
Triggered buffer support uses the HDC100X's dual acquisition mode
to read both humidity and temperature in one shot.
This patch depends on
447136effbf4 ("iio: humidity: hdc100x: fix sensor data reads of
temp and humidity")
Signed-off-by: Alison Schofield
Cc: Daniel Baluta
---
Changes in v3:
Thanks for the review Jonathan
- This patch now depends on
447136effbf4 ("iio: humidity: hdc100x: fix sensor data reads of temp
and humidity")
- remove i2c functionality check, done in dependent patch
- remove unused define HDC100X_ALL_CHANNEL_MASK
- define and use available_scan_masks to demux between temp &/or humid
data in buffered mode
- changed timestamp channel scan_index from 3 to 2.
- put back private data lock in read_raw
(we'll lock private data lock and claim_direct_mode on raw reads)
- remove unneeded NULL parameters in iio_buffer_setup_ops
- add private data lock on buffered read
We are going to allow configuration changes while the sensor is in
buffered mode. We need to allow the toggling of the Heater Enable bit
because the heater only functions during measurements. The lock will
ensure that the config change and buffer read transaction do not collide.
- blank lines added for readability per reviewer
Changes in v2:
Thanks for the review Peter
- switched endianness from IIO_CPU to IIO_BE
- use only one buffer in the handler
- simplify selection of temp &/or humidity to push to buffer
drivers/iio/humidity/Kconfig | 2 +
drivers/iio/humidity/hdc100x.c | 128 -
2 files changed, 128 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
index f155386..5a25569 100644
--- a/drivers/iio/humidity/Kconfig
+++ b/drivers/iio/humidity/Kconfig
@@ -25,6 +25,8 @@ config DHT11
config HDC100X
tristate "TI HDC100x relative humidity and temperature sensor"
depends on I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
help
Say yes here to build support for the Texas Instruments
HDC1000 and HDC1008 relative humidity and temperature sensors.
diff --git a/drivers/iio/humidity/hdc100x.c b/drivers/iio/humidity/hdc100x.c
index e0c9c70..7508c26 100644
--- a/drivers/iio/humidity/hdc100x.c
+++ b/drivers/iio/humidity/hdc100x.c
@@ -22,11 +22,15 @@
#include
#include
+#include
+#include
+#include
#define HDC100X_REG_TEMP 0x00
#define HDC100X_REG_HUMIDITY 0x01
#define HDC100X_REG_CONFIG 0x02
+#define HDC100X_REG_CONFIG_ACQ_MODEBIT(12)
#define HDC100X_REG_CONFIG_HEATER_EN BIT(13)
struct hdc100x_data {
@@ -87,22 +91,40 @@ static const struct iio_chan_spec hdc100x_channels[] = {
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_INT_TIME) |
BIT(IIO_CHAN_INFO_OFFSET),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
},
{
.type = IIO_HUMIDITYRELATIVE,
.address = HDC100X_REG_HUMIDITY,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
- BIT(IIO_CHAN_INFO_INT_TIME)
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
},
{
.type = IIO_CURRENT,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
.extend_name = "heater",
.output = 1,
+ .scan_index = -1,
},
+ IIO_CHAN_SOFT_TIMESTAMP(2),
};
+static const unsigned long hdc100x_scan_masks[] = {0x3, 0};
+
static int hdc100x_update_config(struct hdc100x_data *data, int mask, int val)
{
int tmp = (~mask & data->config) | val;
@@ -183,7 +205,12 @@ static int hdc100x_read_raw(struct iio_dev *indio_dev,
*val = hdc100x_get_heater_status(data);
ret = IIO_VAL_INT;
} else {
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
ret = hdc100x_get_measurement(data, chan);
+ iio_device_release_direct_mode(indio_dev);
if (ret >= 0) {
*val = ret;
ret =
