Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer
On Fri, Apr 16, 2021 at 01:08:06PM -0700, Dan Williams wrote: > [ add Erik ] > > On Fri, Apr 16, 2021 at 10:36 AM Andy Shevchenko > wrote: > > > > On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > > > Strictly speaking the comparison between guid_t and raw buffer > > > is not correct. Return to plain memcmp() since the data structures > > > haven't changed to use uuid_t / guid_t the current state of affairs > > > is inconsistent. Either it should be changed altogether or left > > > as is. > > > > Dan, please review this one as well. I think here you may agree with me. > > You know, this is all a problem because ACPICA is using a raw buffer. And this is fine. It might be any other representation as well. > Erik, would it be possible to use the guid_t type in ACPICA? That > would allow NFIT to drop some ugly casts. guid_t is internal kernel type. If we ever decide to deviate from the current representation it wouldn't be possible in case a 3rd party is using it 1:1 (via typedef or so). -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer
On Thu, Apr 15, 2021 at 05:37:54PM +0300, Andy Shevchenko wrote: > Strictly speaking the comparison between guid_t and raw buffer > is not correct. Return to plain memcmp() since the data structures > haven't changed to use uuid_t / guid_t the current state of affairs > is inconsistent. Either it should be changed altogether or left > as is. Dan, please review this one as well. I think here you may agree with me. -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v1 1/1] ACPI: NFIT: Import GUID before use
On Fri, Apr 16, 2021 at 09:15:34AM -0700, Dan Williams wrote: > On Fri, Apr 16, 2021 at 1:58 AM Andy Shevchenko > wrote: > > On Fri, Apr 16, 2021 at 8:28 AM Dan Williams > > wrote: > > > On Thu, Apr 15, 2021 at 6:59 AM Andy Shevchenko > > > wrote: > > > > > > > > Strictly speaking the comparison between guid_t and raw buffer > > > > is not correct. Import GUID to variable of guid_t type and then > > > > compare. > > > > > > Hmm, what about something like the following instead, because it adds > > > safety. Any concerns about evaluating x twice in a macro should be > > > alleviated by the fact that ARRAY_SIZE() will fail the build if (x) is > > > not an array. > > > > ARRAY_SIZE doesn't check type. > > See __must_be_array. > > > I don't like hiding ugly casts like this. > > See PTR_ERR, ERR_PTR, ERR_CAST. It's special, i.e. error pointer case. We don't handle such here. > There's nothing broken about the way the code currently stands, so I'd > rather try to find something to move the implementation forward than > sideways. Submit a patch then. I rest my case b/c I consider that ugly castings worse than additional API call, although it's not ideal. -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH v1 1/1] ACPI: NFIT: Import GUID before use
On Fri, Apr 16, 2021 at 8:28 AM Dan Williams wrote: > > On Thu, Apr 15, 2021 at 6:59 AM Andy Shevchenko > wrote: > > > > Strictly speaking the comparison between guid_t and raw buffer > > is not correct. Import GUID to variable of guid_t type and then > > compare. > > Hmm, what about something like the following instead, because it adds > safety. Any concerns about evaluating x twice in a macro should be > alleviated by the fact that ARRAY_SIZE() will fail the build if (x) is > not an array. ARRAY_SIZE doesn't check type. I don't like hiding ugly casts like this. -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH v1 1/1] libnvdimm: Don't use GUID APIs against raw buffer
Strictly speaking the comparison between guid_t and raw buffer
is not correct. Return to plain memcmp() since the data structures
haven't changed to use uuid_t / guid_t the current state of affairs
is inconsistent. Either it should be changed altogether or left
as is.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/btt_devs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/nvdimm/btt_devs.c b/drivers/nvdimm/btt_devs.c
index 05feb97e11ce..82bcd2e86a18 100644
--- a/drivers/nvdimm/btt_devs.c
+++ b/drivers/nvdimm/btt_devs.c
@@ -244,13 +244,14 @@ struct device *nd_btt_create(struct nd_region *nd_region)
*/
bool nd_btt_arena_is_valid(struct nd_btt *nd_btt, struct btt_sb *super)
{
+ static const u8 null_uuid[16];
const u8 *parent_uuid = nd_dev_to_uuid(&nd_btt->ndns->dev);
u64 checksum;
if (memcmp(super->signature, BTT_SIG, BTT_SIG_LEN) != 0)
return false;
- if (!guid_is_null((guid_t *)&super->parent_uuid))
+ if (memcmp(super->parent_uuid, null_uuid, 16) != 0)
if (memcmp(super->parent_uuid, parent_uuid, 16) != 0)
return false;
--
2.30.2
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH v1 1/1] ACPI: NFIT: Import GUID before use
Strictly speaking the comparison between guid_t and raw buffer
is not correct. Import GUID to variable of guid_t type and then
compare.
Signed-off-by: Andy Shevchenko
---
drivers/acpi/nfit/core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 958aaac869e8..6d8a1a93636a 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -678,10 +678,12 @@ static const char *spa_type_name(u16 type)
int nfit_spa_type(struct acpi_nfit_system_address *spa)
{
+ guid_t guid;
int i;
+ import_guid(&guid, spa->range_guid);
for (i = 0; i < NFIT_UUID_MAX; i++)
- if (guid_equal(to_nfit_uuid(i), (guid_t *)&spa->range_guid))
+ if (guid_equal(to_nfit_uuid(i), &guid))
return i;
return -1;
}
--
2.30.2
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [RFT][PATCH v2 2/4] ACPI: OSL: Add support for deferred unmapping of ACPI memory
On Mon, Jun 22, 2020 at 6:28 PM Rafael J. Wysocki wrote: > On Mon, Jun 22, 2020 at 4:56 PM Andy Shevchenko > wrote: > > On Mon, Jun 22, 2020 at 5:06 PM Rafael J. Wysocki > > wrote: ... > > > + return true; > > > + } > > > + > > > + return false; > > > > A nit: > > > > Effectively it returns a value of defer. > > > > return defer; > > > > > } > > Do you mean that one line of code could be saved? Yes, it could. Yes. The question here would it make a cleaner way for the reader to understand the returned value? (For the rest, nevermind, choose whatever suits better in your opinion) -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [RFT][PATCH v2 2/4] ACPI: OSL: Add support for deferred unmapping of ACPI memory
On Mon, Jun 22, 2020 at 5:06 PM Rafael J. Wysocki wrote:
>
> From: "Rafael J. Wysocki"
>
> Implement acpi_os_unmap_deferred() and
> acpi_os_release_unused_mappings() and set ACPI_USE_DEFERRED_UNMAPPING
> to allow ACPICA to use deferred unmapping of memory in
> acpi_ex_system_memory_space_handler() so as to avoid RCU-related
> performance issues with memory opregions.
...
> +static bool acpi_os_drop_map_ref(struct acpi_ioremap *map, bool defer)
> {
> - unsigned long refcount = --map->refcount;
> + if (--map->track.refcount)
> + return true;
>
> - if (!refcount)
> - list_del_rcu(&map->list);
> - return refcount;
> + list_del_rcu(&map->list);
> +
> + if (defer) {
> + INIT_LIST_HEAD(&map->track.gc);
> + list_add_tail(&map->track.gc, &unused_mappings);
> + return true;
> + }
> +
> + return false;
A nit:
Effectively it returns a value of defer.
return defer;
> }
...
> @@ -416,26 +421,102 @@ void __ref acpi_os_unmap_iomem(void __iomem *virt,
> acpi_size size)
> }
>
> mutex_lock(&acpi_ioremap_lock);
> +
> map = acpi_map_lookup_virt(virt, size);
A nit: should it be somewhere else (I mean in another patch)?
> if (!map) {
...
> + /* Release the unused mappings in the list. */
> + while (!list_empty(&list)) {
> + struct acpi_ioremap *map;
> +
> + map = list_entry(list.next, struct acpi_ioremap, track.gc);
A nt: if __acpi_os_map_cleanup() (actually acpi_unmap() according to
the code) has no side effects, can we use list_for_each_entry_safe()
here?
> + list_del(&map->track.gc);
> + __acpi_os_map_cleanup(map);
> + }
> +}
...
> @@ -472,16 +552,18 @@ void acpi_os_unmap_generic_address(struct
> acpi_generic_address *gas)
> return;
>
> mutex_lock(&acpi_ioremap_lock);
> +
> map = acpi_map_lookup(addr, gas->bit_width / 8);
A nit: should it be somewhere else (I mean in another patch)?
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [RFT][PATCH] ACPI: OSL: Use rwlock instead of RCU for memory management
On Fri, Jun 5, 2020 at 5:11 PM Rafael J. Wysocki wrote:
...
> + if (!refcount) {
> + write_lock_irq(&acpi_ioremaps_list_lock);
> +
> + list_del(&map->list);
> +
> + write_unlock_irq(&acpi_ioremaps_list_lock);
> + }
> return refcount;
It seems we can decrease indentation level at the same time:
if (refcount)
return refcount;
...
return 0;
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
Re: [PATCH] ACPI: Drop rcu usage for MMIO mappings
On Thu, May 7, 2020 at 3:21 AM Dan Williams wrote:
>
> Recently a performance problem was reported for a process invoking a
> non-trival ASL program. The method call in this case ends up
> repetitively triggering a call path like:
>
> acpi_ex_store
> acpi_ex_store_object_to_node
> acpi_ex_write_data_to_field
> acpi_ex_insert_into_field
> acpi_ex_write_with_update_rule
> acpi_ex_field_datum_io
> acpi_ex_access_region
> acpi_ev_address_space_dispatch
> acpi_ex_system_memory_space_handler
> acpi_os_map_cleanup.part.14
> _synchronize_rcu_expedited.constprop.89
> schedule
>
> The end result of frequent synchronize_rcu_expedited() invocation is
> tiny sub-millisecond spurts of execution where the scheduler freely
> migrates this apparently sleepy task. The overhead of frequent scheduler
> invocation multiplies the execution time by a factor of 2-3X.
>
> For example, performance improves from 16 minutes to 7 minutes for a
> firmware update procedure across 24 devices.
>
> Perhaps the rcu usage was intended to allow for not taking a sleeping
> lock in the acpi_os_{read,write}_memory() path which ostensibly could be
> called from an APEI NMI error interrupt? Neither rcu_read_lock() nor
> ioremap() are interrupt safe, so add a WARN_ONCE() to validate that rcu
> was not serving as a mechanism to avoid direct calls to ioremap(). Even
> the original implementation had a spin_lock_irqsave(), but that is not
> NMI safe.
>
> APEI itself already has some concept of avoiding ioremap() from
> interrupt context (see erst_exec_move_data()), if the new warning
> triggers it means that APEI either needs more instrumentation like that
> to pre-emptively fail, or more infrastructure to arrange for pre-mapping
> the resources it needs in NMI context.
...
> +static void __iomem *acpi_os_rw_map(acpi_physical_address phys_addr,
> + unsigned int size, bool *did_fallback)
> +{
> + void __iomem *virt_addr = NULL;
Assignment is not needed as far as I can see.
> + if (WARN_ONCE(in_interrupt(), "ioremap in interrupt context\n"))
> + return NULL;
> +
> + /* Try to use a cached mapping and fallback otherwise */
> + *did_fallback = false;
> + mutex_lock(&acpi_ioremap_lock);
> + virt_addr = acpi_map_vaddr_lookup(phys_addr, size);
> + if (virt_addr)
> + return virt_addr;
> + mutex_unlock(&acpi_ioremap_lock);
> +
> + virt_addr = acpi_os_ioremap(phys_addr, size);
> + *did_fallback = true;
> +
> + return virt_addr;
> +}
I'm wondering if Sparse is okay with this...
> +static void acpi_os_rw_unmap(void __iomem *virt_addr, bool did_fallback)
> +{
> + if (did_fallback) {
> + /* in the fallback case no lock is held */
> + iounmap(virt_addr);
> + return;
> + }
> +
> + mutex_unlock(&acpi_ioremap_lock);
> +}
...and this functions from locking perspective.
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH v1] libnvdimm: Replace guid_copy() with import_guid() where it makes sense
There is a specific API to treat raw data as GUID, i.e. import_guid(). Use it instead of guid_copy() with explicit casting. Signed-off-by: Andy Shevchenko --- drivers/acpi/nfit/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index fa4500f9cfd13..7c138a4edc03e 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -2293,7 +2293,7 @@ static int acpi_nfit_init_interleave_set(struct acpi_nfit_desc *acpi_desc, nd_set = devm_kzalloc(dev, sizeof(*nd_set), GFP_KERNEL); if (!nd_set) return -ENOMEM; - guid_copy(&nd_set->type_guid, (guid_t *) spa->range_guid); + import_guid(&nd_set->type_guid, spa->range_guid); info = devm_kzalloc(dev, sizeof_nfit_set_info(nr), GFP_KERNEL); if (!info) -- 2.26.1 ___ Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org To unsubscribe send an email to linux-nvdimm-le...@lists.01.org
[PATCH v1] libnvdimm, namespace: Drop uuid_t implementation detail
There is no need for caller to know how uuid_t type is constructed. Thus,
whenever we use it the implementation details are not needed. Drop it for good.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index a434a5964cb9..2d8d7e554877 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1822,8 +1822,8 @@ static bool has_uuid_at_pos(struct nd_region *nd_region,
u8 *uuid,
&& !guid_equal(&nd_set->type_guid,
&nd_label->type_guid)) {
dev_dbg(ndd->dev, "expect type_guid %pUb got
%pUb\n",
- nd_set->type_guid.b,
- nd_label->type_guid.b);
+ &nd_set->type_guid,
+ &nd_label->type_guid);
continue;
}
@@ -2227,8 +2227,8 @@ static struct device *create_namespace_blk(struct
nd_region *nd_region,
if (namespace_label_has(ndd, type_guid)) {
if (!guid_equal(&nd_set->type_guid, &nd_label->type_guid)) {
dev_dbg(ndd->dev, "expect type_guid %pUb got %pUb\n",
- nd_set->type_guid.b,
- nd_label->type_guid.b);
+ &nd_set->type_guid,
+ &nd_label->type_guid);
return ERR_PTR(-EAGAIN);
}
--
2.20.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf
On Mon, Mar 25, 2019 at 05:13:00PM +0200, Sakari Ailus wrote: > All other invalid pointer conversion specifiers currently result into a > warning only. I see that as an orthogonal change to this set. I found > another issue in checkpatch.pl that may require some discussion; would you > be ok with addressing this in another set? If it looks better that way, I have no objection. -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf
On Sun, Mar 24, 2019 at 11:10:08PM +0200, Sakari Ailus wrote: > On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote: > > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote: > > > > > Porting a patch > > > forward should have no issues either as checkpatch.pl has been complaining > > > of the use of %pf and %pF for a while now. > > > > And that's exactly the reason why I think instead of removing warning on > > checkpatch, it makes sense to convert to an error for a while. People are > > tending read documentation on internet and thus might have outdated one. And > > yes, the compiler doesn't tell a thing about it. > > > > P.S. Though, if majority of people will tell that I'm wrong, then it's okay > > to > > remove. > > I wonder if you wrote this before seeing my other patchset. Yes, I wrote it before seeing another series. > What I think could be done is to warn of plain %pf (without following "w") > in checkpatch.pl, and %pf that is not followed by "w" in the kernel. > Although we didn't have such checks to begin with. The case is still a > little bit different as %pf used to be a valid conversion specifier whereas > %pO likely has never existed. > > So, how about adding such checks in the other set? I can retain %p[fF] check > here, too, if you like. Consistency tells me that the warning->error transformation in checkpatch.pl belongs this series. -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf
On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote: > Porting a patch > forward should have no issues either as checkpatch.pl has been complaining > of the use of %pf and %pF for a while now. And that's exactly the reason why I think instead of removing warning on checkpatch, it makes sense to convert to an error for a while. People are tending read documentation on internet and thus might have outdated one. And yes, the compiler doesn't tell a thing about it. P.S. Though, if majority of people will tell that I'm wrong, then it's okay to remove. -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v1] libnvdimm, namespace: Drop uuid_t implementation detail
There is no need for caller to know how uuid_t type is constructed. Thus,
whenever we use it the implementation details are not needed. Drop it for good.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 4b077555ac70..47485373c5cd 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -1811,8 +1811,8 @@ static bool has_uuid_at_pos(struct nd_region *nd_region,
u8 *uuid,
&& !guid_equal(&nd_set->type_guid,
&nd_label->type_guid)) {
dev_dbg(ndd->dev, "expect type_guid %pUb got
%pUb\n",
- nd_set->type_guid.b,
- nd_label->type_guid.b);
+ &nd_set->type_guid,
+ &nd_label->type_guid);
continue;
}
@@ -2216,8 +2216,8 @@ static struct device *create_namespace_blk(struct
nd_region *nd_region,
if (namespace_label_has(ndd, type_guid)) {
if (!guid_equal(&nd_set->type_guid, &nd_label->type_guid)) {
dev_dbg(ndd->dev, "expect type_guid %pUb got %pUb\n",
- nd_set->type_guid.b,
- nd_label->type_guid.b);
+ &nd_set->type_guid,
+ &nd_label->type_guid);
return ERR_PTR(-EAGAIN);
}
--
2.20.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v1] libnvdimm, namespace: Replace kmemdup() with kstrndup()
On Mon, Jun 11, 2018 at 04:47:21PM +0300, Andy Shevchenko wrote:
> kstrndup() takes care of '\0' terminator for the strings.
>
> Use it here instead of kmemdup() + explicit terminating the input string.
>
Any comments on this?
> Signed-off-by: Andy Shevchenko
> ---
> drivers/nvdimm/namespace_devs.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
> index 28afdd668905..19525f025539 100644
> --- a/drivers/nvdimm/namespace_devs.c
> +++ b/drivers/nvdimm/namespace_devs.c
> @@ -270,11 +270,10 @@ static ssize_t __alt_name_store(struct device *dev,
> const char *buf,
> if (dev->driver || to_ndns(dev)->claim)
> return -EBUSY;
>
> - input = kmemdup(buf, len + 1, GFP_KERNEL);
> + input = kstrndup(buf, len, GFP_KERNEL);
> if (!input)
> return -ENOMEM;
>
> - input[len] = '\0';
> pos = strim(input);
> if (strlen(pos) + 1 > NSLABEL_NAME_LEN) {
> rc = -EINVAL;
> --
> 2.17.1
>
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v1] libnvdimm, label: Switch to bitmap_zalloc()
Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/label.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
index 1d28cd656536..53159262c85d 100644
--- a/drivers/nvdimm/label.c
+++ b/drivers/nvdimm/label.c
@@ -814,8 +814,7 @@ static int __blk_label_update(struct nd_region *nd_region,
victims = 0;
if (old_num_resources) {
/* convert old local-label-map to dimm-slot victim-map */
- victim_map = kcalloc(BITS_TO_LONGS(nslot), sizeof(long),
- GFP_KERNEL);
+ victim_map = bitmap_zalloc(nslot, GFP_KERNEL);
if (!victim_map)
return -ENOMEM;
@@ -838,7 +837,7 @@ static int __blk_label_update(struct nd_region *nd_region,
/* don't allow updates that consume the last label */
if (nfree - alloc < 0 || nfree - alloc + victims < 1) {
dev_info(&nsblk->common.dev, "insufficient label space\n");
- kfree(victim_map);
+ bitmap_free(victim_map);
return -ENOSPC;
}
/* from here on we need to abort on error */
@@ -1010,7 +1009,7 @@ static int __blk_label_update(struct nd_region *nd_region,
out:
kfree(old_res_list);
- kfree(victim_map);
+ bitmap_free(victim_map);
return rc;
abort:
--
2.18.0
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v7 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Wed, Jul 18, 2018 at 5:49 AM, Baoquan He wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.
Some minor stuff.
> +/**
> + * reparent_resources - reparent resource children of parent that res covers
> + * @parent: parent resource descriptor
> + * @res: resource descriptor desired by caller
> + *
> + * Returns 0 on success, -ENOTSUPP if child resource is not completely
> + * contained by 'res', -ECANCELED if no any conflicting entry found.
'res' -> @res
> + *
> + * Reparent resource children of 'parent' that conflict with 'res'
Ditto + 'parent' -> @parent
> + * under 'res', and make 'res' replace those children.
Ditto.
> + */
> +int reparent_resources(struct resource *parent, struct resource *res)
> +{
> + struct resource *p, **pp;
> + struct resource **firstpp = NULL;
> +
> + for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> + if (p->end < res->start)
> + continue;
> + if (res->end < p->start)
> + break;
> + if (p->start < res->start || p->end > res->end)
> + return -ENOTSUPP; /* not completely contained */
> + if (firstpp == NULL)
> + firstpp = pp;
> + }
> + if (firstpp == NULL)
> + return -ECANCELED; /* didn't find any conflicting entries? */
> + res->parent = parent;
> + res->child = *firstpp;
> + res->sibling = *pp;
> + *firstpp = res;
> + *pp = NULL;
> + for (p = res->child; p != NULL; p = p->sibling) {
> + p->parent = res;
> + pr_debug("PCI: Reparented %s %pR under %s\n",
> +p->name, p, res->name);
Now, PCI is a bit confusing here.
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL(reparent_resources);
> +
> static void __init __reserve_region_with_split(struct resource *root,
> resource_size_t start, resource_size_t end,
> const char *name)
> --
> 2.13.6
>
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v6 2/4] resource: Use list_head to link sibling resource
On Sun, Jul 8, 2018 at 5:59 AM, Baoquan He wrote:
> On 07/05/18 at 01:00am, kbuild test robot wrote:
> However, I didn't find below branch. And tried to open it in web
> broswer, also failed.
While this is kinda valid point...
> Could you help have a look at this?
...isn't obvious that you didn't change the file mentioned in a report?
Just take latest linux-next and you will see.
>> All error/warnings (new ones prefixed by >>):
>>
>> >> arch/mips/pci/pci-rc32434.c:57:11: error: initialization from
>> >> incompatible pointer type [-Werror=incompatible-pointer-types]
>> .child = &rc32434_res_pci_mem2
>> ^
>>arch/mips/pci/pci-rc32434.c:57:11: note: (near initialization for
>> 'rc32434_res_pci_mem1.child.next')
>> >> arch/mips/pci/pci-rc32434.c:51:47: warning: missing braces around
>> >> initializer [-Wmissing-braces]
>> static struct resource rc32434_res_pci_mem1 = {
>> ^
>>arch/mips/pci/pci-rc32434.c:60:47: warning: missing braces around
>> initializer [-Wmissing-braces]
>> static struct resource rc32434_res_pci_mem2 = {
>> ^
>>cc1: some warnings being treated as errors
>>
>> vim +57 arch/mips/pci/pci-rc32434.c
>>
>> 73b4390f Ralf Baechle 2008-07-16 50
>> 73b4390f Ralf Baechle 2008-07-16 @51 static struct resource
>> rc32434_res_pci_mem1 = {
>> 73b4390f Ralf Baechle 2008-07-16 52 .name = "PCI MEM1",
>> 73b4390f Ralf Baechle 2008-07-16 53 .start = 0x5000,
>> 73b4390f Ralf Baechle 2008-07-16 54 .end = 0x5FFF,
>> 73b4390f Ralf Baechle 2008-07-16 55 .flags = IORESOURCE_MEM,
>> 73b4390f Ralf Baechle 2008-07-16 56 .sibling = NULL,
>> 73b4390f Ralf Baechle 2008-07-16 @57 .child = &rc32434_res_pci_mem2
>> 73b4390f Ralf Baechle 2008-07-16 58 };
>> 73b4390f Ralf Baechle 2008-07-16 59
>>
>> :: The code at line 57 was first introduced by commit
>> :: 73b4390fb23456964201abda79f1210fe337d01a [MIPS] Routerboard 532:
>> Support for base system
>>
>> :: TO: Ralf Baechle
>> :: CC: Ralf Baechle
>>
>> ---
>> 0-DAY kernel test infrastructureOpen Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
>
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v6 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Wed, Jul 4, 2018 at 7:10 AM, Baoquan He wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared.
With couple of comments below,
Reviewed-by: Andy Shevchenko
P.S. In some commit message in this series you used 'likt' instead of 'like'.
>
> Signed-off-by: Baoquan He
> ---
> arch/microblaze/pci/pci-common.c | 37 -
> arch/powerpc/kernel/pci-common.c | 35 ---
> include/linux/ioport.h | 1 +
> kernel/resource.c| 39 +++
> 4 files changed, 40 insertions(+), 72 deletions(-)
>
> diff --git a/arch/microblaze/pci/pci-common.c
> b/arch/microblaze/pci/pci-common.c
> index f34346d56095..7899bafab064 100644
> --- a/arch/microblaze/pci/pci-common.c
> +++ b/arch/microblaze/pci/pci-common.c
> @@ -619,43 +619,6 @@ int pcibios_add_device(struct pci_dev *dev)
> EXPORT_SYMBOL(pcibios_add_device);
>
> /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int __init reparent_resources(struct resource *parent,
> -struct resource *res)
> -{
> - struct resource *p, **pp;
> - struct resource **firstpp = NULL;
> -
> - for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> - if (p->end < res->start)
> - continue;
> - if (res->end < p->start)
> - break;
> - if (p->start < res->start || p->end > res->end)
> - return -1; /* not completely contained */
> - if (firstpp == NULL)
> - firstpp = pp;
> - }
> - if (firstpp == NULL)
> - return -1; /* didn't find any conflicting entries? */
> - res->parent = parent;
> - res->child = *firstpp;
> - res->sibling = *pp;
> - *firstpp = res;
> - *pp = NULL;
> - for (p = res->child; p != NULL; p = p->sibling) {
> - p->parent = res;
> - pr_debug("PCI: Reparented %s [%llx..%llx] under %s\n",
> -p->name,
> -(unsigned long long)p->start,
> -(unsigned long long)p->end, res->name);
> - }
> - return 0;
> -}
> -
> -/*
> * Handle resources of PCI devices. If the world were perfect, we could
> * just allocate all the resource regions and do nothing more. It isn't.
> * On the other hand, we cannot just re-allocate all devices, as it would
> diff --git a/arch/powerpc/kernel/pci-common.c
> b/arch/powerpc/kernel/pci-common.c
> index fe9733aa..926035bb378d 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1088,41 +1088,6 @@ resource_size_t pcibios_align_resource(void *data,
> const struct resource *res,
> EXPORT_SYMBOL(pcibios_align_resource);
>
> /*
> - * Reparent resource children of pr that conflict with res
> - * under res, and make res replace those children.
> - */
> -static int reparent_resources(struct resource *parent,
> -struct resource *res)
> -{
> - struct resource *p, **pp;
> - struct resource **firstpp = NULL;
> -
> - for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> - if (p->end < res->start)
> - continue;
> - if (res->end < p->start)
> - break;
> - if (p->start < res->start || p->end > res->end)
> - return -1; /* not completely contained */
> - if (firstpp == NULL)
> - firstpp = pp;
> - }
> - if (firstpp == NULL)
> - return -1; /* didn't find any conflicting entries? */
> - res->parent = parent;
> - res->child = *firstpp;
> - res->sibling = *pp;
> - *firstpp = res;
> - *pp = NULL;
> - for (p = res->child; p != NULL; p = p->sibling) {
> - p->parent = res;
> - pr_debug("PCI: Reparented %s %pR under %s\n",
> -p->name, p, res->name);
> - }
> - return 0;
> -}
> -
> -/*
> * Handle resources of PCI devices. If the world were perfect, we could
&g
Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Tue, Jul 3, 2018 at 5:55 PM, Baoquan He wrote: > On 06/12/18 at 05:24pm, Andy Shevchenko wrote: >> On Tue, Jun 12, 2018 at 5:20 PM, Andy Shevchenko >> wrote: >> > I briefly looked at the code and error codes we have, so, my proposal >> > is one of the following >> >> > - use -ECANCELED (not the best choice for first occurrence here, >> > though I can't find better) >> >> Actually -ENOTSUPP might suit the first case (although the actual >> would be something like -EOVERLAP, which we don't have) > > Sorry for late reply, and many thanks for your great suggestion. > > I am fine to use -ENOTSUPP as the first returned value, and -ECANCELED > for the 2nd one. I have no strong opinion, but I like (slightly better) this approach ^^^ > Or define an enum as you suggested inside the function > or in header file. > > Or use -EBUSY for the first case because existing resource is > overlapping but not fully contained by 'res'; and -EINVAL for > the 2nd case since didn't find any one resources which is contained by > 'res', means we passed in a invalid resource. > > All is fine to me, I can repost with each of them. >> > - use positive integers (or enum), like >> > #define RES_REPARENTED 0 >> > #define RES_OVERLAPPED 1 >> > #define RES_NOCONFLICT 2 -- With Best Regards, Andy Shevchenko ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Tue, Jun 12, 2018 at 5:20 PM, Andy Shevchenko
wrote:
> On Tue, Jun 12, 2018 at 12:38 PM, Baoquan He wrote:
>> On 06/12/18 at 11:29am, Andy Shevchenko wrote:
>>> On Tue, Jun 12, 2018 at 6:28 AM, Baoquan He wrote:
>
>>> > +{
>>>
>>> > + for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
>>> > + if (p->end < res->start)
>>> > + continue;
>>> > + if (res->end < p->start)
>>> > + break;
>>>
>>> > + if (p->start < res->start || p->end > res->end)
>>> > + return -1; /* not completely contained */
>>>
>>> Usually we are expecting real eeror codes.
>>
>> Hmm, I just copied it from arch/powerpc/kernel/pci-common.c. The
>> function interface expects an integer returned value, not sure what a
>> real error codes look like, could you give more hints? Will change
>> accordingly.
>
> I briefly looked at the code and error codes we have, so, my proposal
> is one of the following
> - use -ECANCELED (not the best choice for first occurrence here,
> though I can't find better)
Actually -ENOTSUPP might suit the first case (although the actual
would be something like -EOVERLAP, which we don't have)
> - use positive integers (or enum), like
> #define RES_REPARENTED 0
> #define RES_OVERLAPPED 1
> #define RES_NOCONFLICT 2
>
>
>>> > + if (firstpp == NULL)
>>> > + firstpp = pp;
>>> > + }
>>>
>>> > + if (firstpp == NULL)
>>> > + return -1; /* didn't find any conflicting entries? */
>>>
>>> Ditto.
>
> Ditto.
>
>>>
>>> > +}
>>> > +EXPORT_SYMBOL(reparent_resources);
>
> --
> With Best Regards,
> Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Tue, Jun 12, 2018 at 12:38 PM, Baoquan He wrote:
> On 06/12/18 at 11:29am, Andy Shevchenko wrote:
>> On Tue, Jun 12, 2018 at 6:28 AM, Baoquan He wrote:
>> > +{
>>
>> > + for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
>> > + if (p->end < res->start)
>> > + continue;
>> > + if (res->end < p->start)
>> > + break;
>>
>> > + if (p->start < res->start || p->end > res->end)
>> > + return -1; /* not completely contained */
>>
>> Usually we are expecting real eeror codes.
>
> Hmm, I just copied it from arch/powerpc/kernel/pci-common.c. The
> function interface expects an integer returned value, not sure what a
> real error codes look like, could you give more hints? Will change
> accordingly.
I briefly looked at the code and error codes we have, so, my proposal
is one of the following
- use -ECANCELED (not the best choice for first occurrence here,
though I can't find better)
- use positive integers (or enum), like
#define RES_REPARENTED 0
#define RES_OVERLAPPED 1
#define RES_NOCONFLICT 2
>> > + if (firstpp == NULL)
>> > + firstpp = pp;
>> > + }
>>
>> > + if (firstpp == NULL)
>> > + return -1; /* didn't find any conflicting entries? */
>>
>> Ditto.
Ditto.
>>
>> > +}
>> > +EXPORT_SYMBOL(reparent_resources);
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH v5 1/4] resource: Move reparent_resources() to kernel/resource.c and make it public
On Tue, Jun 12, 2018 at 6:28 AM, Baoquan He wrote:
> reparent_resources() is duplicated in arch/microblaze/pci/pci-common.c
> and arch/powerpc/kernel/pci-common.c, so move it to kernel/resource.c
> so that it's shared. Later its code also need be updated using list_head
> to replace singly linked list.
While this is a good deduplication of the code, some requirements for
public functions would be good to satisfy.
> +/*
> + * Reparent resource children of pr that conflict with res
> + * under res, and make res replace those children.
> + */
kernel doc format, though...
> +static int reparent_resources(struct resource *parent,
> +struct resource *res)
...is it really public with static keyword?!
> +{
> + for (pp = &parent->child; (p = *pp) != NULL; pp = &p->sibling) {
> + if (p->end < res->start)
> + continue;
> + if (res->end < p->start)
> + break;
> + if (p->start < res->start || p->end > res->end)
> + return -1; /* not completely contained */
Usually we are expecting real eeror codes.
> + if (firstpp == NULL)
> + firstpp = pp;
> + }
> + if (firstpp == NULL)
> + return -1; /* didn't find any conflicting entries? */
Ditto.
> +}
> +EXPORT_SYMBOL(reparent_resources);
--
With Best Regards,
Andy Shevchenko
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
[PATCH v1] libnvdimm, namespace: Replace kmemdup() with kstrndup()
kstrndup() takes care of '\0' terminator for the strings.
Use it here instead of kmemdup() + explicit terminating the input string.
Signed-off-by: Andy Shevchenko
---
drivers/nvdimm/namespace_devs.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
index 28afdd668905..19525f025539 100644
--- a/drivers/nvdimm/namespace_devs.c
+++ b/drivers/nvdimm/namespace_devs.c
@@ -270,11 +270,10 @@ static ssize_t __alt_name_store(struct device *dev, const
char *buf,
if (dev->driver || to_ndns(dev)->claim)
return -EBUSY;
- input = kmemdup(buf, len + 1, GFP_KERNEL);
+ input = kstrndup(buf, len, GFP_KERNEL);
if (!input)
return -ENOMEM;
- input[len] = '\0';
pos = strim(input);
if (strlen(pos) + 1 > NSLABEL_NAME_LEN) {
rc = -EINVAL;
--
2.17.1
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 15/24] block: remove blk_part_pack_uuid
On Wed, 2017-05-31 at 18:18 +0200, Christoph Hellwig wrote: > This helper was only used by IMA of all things, which would get > spurious > errors if CONFIG_BLOCK is disabled. Just opencode the call there. > - result = blk_part_pack_uuid(args[0].from, > - entry->fsuuid); > + result = uuid_to_bin(args[0].from, (uuid_t > *)&entry->fsuuid); > uuid_parse() ? -- Andy Shevchenko Intel Finland Oy ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 08/24] uuid: rename uuid_to_bin to uuid_parse
On Wed, 2017-05-31 at 18:18 +0200, Christoph Hellwig wrote:
> This matches the userspace version of it, and describes the
> functionality
> much better. Also do the same for the guid version.
>
No objections for renaming, though I'm pretty sure it should be squashed
to patch 6.
> Signed-off-by: Christoph Hellwig
> ---
> include/linux/uuid.h | 8
> lib/test_uuid.c | 8
> lib/uuid.c | 14 +++---
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/uuid.h b/include/linux/uuid.h
> index a9d0fdba5404..82b165b579f5 100644
> --- a/include/linux/uuid.h
> +++ b/include/linux/uuid.h
> @@ -45,8 +45,8 @@ bool __must_check uuid_is_valid(const char *uuid);
> extern const u8 guid_index[16];
> extern const u8 uuid_index[16];
>
> -int guid_to_bin(const char *uuid, guid_t *u);
> -int uuid_to_bin(const char *uuid, uuid_t *u);
> +int guid_parse(const char *uuid, guid_t *u);
> +int uuid_parse(const char *uuid, uuid_t *u);
>
> /* backwards compatibility, don't use in new code */
> typedef uuid_t uuid_be;
> @@ -58,8 +58,8 @@ typedef uuid_t uuid_be;
>
> #define uuid_le_gen(u) guid_gen(u)
> #define uuid_be_gen(u) uuid_gen(u)
> -#define uuid_le_to_bin(guid, u) guid_to_bin(guid, u)
> -#define uuid_be_to_bin(uuid, u) uuid_to_bin(uuid, u)
> +#define uuid_le_to_bin(guid, u) guid_parse(guid, u)
> +#define uuid_be_to_bin(uuid, u) uuid_parse(uuid, u)
>
> static inline int uuid_le_cmp(const guid_t u1, const guid_t u2)
> {
> diff --git a/lib/test_uuid.c b/lib/test_uuid.c
> index 9cad846fd805..edda536a7b45 100644
> --- a/lib/test_uuid.c
> +++ b/lib/test_uuid.c
> @@ -67,7 +67,7 @@ static void __init test_uuid_test(const struct
> test_uuid_data *data)
>
> /* LE */
> total_tests++;
> - if (guid_to_bin(data->uuid, &le))
> + if (guid_parse(data->uuid, &le))
> test_uuid_failed("conversion", false, false, data-
> >uuid, NULL);
>
> total_tests++;
> @@ -78,7 +78,7 @@ static void __init test_uuid_test(const struct
> test_uuid_data *data)
>
> /* BE */
> total_tests++;
> - if (uuid_to_bin(data->uuid, &be))
> + if (uuid_parse(data->uuid, &be))
> test_uuid_failed("conversion", false, true, data-
> >uuid, NULL);
>
> total_tests++;
> @@ -95,12 +95,12 @@ static void __init test_uuid_wrong(const char
> *data)
>
> /* LE */
> total_tests++;
> - if (!guid_to_bin(data, &le))
> + if (!guid_parse(data, &le))
> test_uuid_failed("negative", true, false, data,
> NULL);
>
> /* BE */
> total_tests++;
> - if (!uuid_to_bin(data, &be))
> + if (!uuid_parse(data, &be))
> test_uuid_failed("negative", true, true, data, NULL);
> }
>
> diff --git a/lib/uuid.c b/lib/uuid.c
> index f80dc63f6ca8..90bee73f7bd7 100644
> --- a/lib/uuid.c
> +++ b/lib/uuid.c
> @@ -97,7 +97,7 @@ bool uuid_is_valid(const char *uuid)
> }
> EXPORT_SYMBOL(uuid_is_valid);
>
> -static int __uuid_to_bin(const char *uuid, __u8 b[16], const u8
> ei[16])
> +static int __uuid_parse(const char *uuid, __u8 b[16], const u8
> ei[16])
> {
> static const u8 si[16] =
> {0,2,4,6,9,11,14,16,19,21,24,26,28,30,32,34};
> unsigned int i;
> @@ -115,14 +115,14 @@ static int __uuid_to_bin(const char *uuid, __u8
> b[16], const u8 ei[16])
> return 0;
> }
>
> -int guid_to_bin(const char *uuid, guid_t *u)
> +int guid_parse(const char *uuid, guid_t *u)
> {
> - return __uuid_to_bin(uuid, u->b, guid_index);
> + return __uuid_parse(uuid, u->b, guid_index);
> }
> -EXPORT_SYMBOL(guid_to_bin);
> +EXPORT_SYMBOL(guid_parse);
>
> -int uuid_to_bin(const char *uuid, uuid_t *u)
> +int uuid_parse(const char *uuid, uuid_t *u)
> {
> - return __uuid_to_bin(uuid, u->b, uuid_index);
> + return __uuid_parse(uuid, u->b, uuid_index);
> }
> -EXPORT_SYMBOL(uuid_to_bin);
> +EXPORT_SYMBOL(uuid_parse);
--
Andy Shevchenko
Intel Finland Oy
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 10/23] afs: switch to use uuid_t and uuid_gen
On Thu, 2017-05-25 at 15:00 +0200, Christoph Hellwig wrote: > On Tue, May 23, 2017 at 04:11:39PM +0300, Andy Shevchenko wrote: > > Since we introduced a union it's possible that we might access the > > member which wasn't last modified one. So, my comment is to give an > > attention on such possibility and avoid if there is an aliasing > > happened. > > We do for AFS (and XFS for fs fsid). My preference would be to > not have the v1 struct defintion but instead provide a few > helpers in uuid.h that use get_unaligned_be* if needed: > > uuid_v1_time_low() > uuid_v1_time_mid() > uuid_v1_time_time_hi_and_version().. > > From his previously reply it seems like Dave doesn't like that idea > too much, in which case I suspect moving struct uuid_v1 back into > afs and living with cast in it is the way to go. Personally I don't like that union stuff, so, definitely my vote to get rid of AFS stuff in generic helpers. OTOH if there will be more users of such API then something like you proposed would be sufficient without introducing a union. -- Andy Shevchenko Intel Finland Oy ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 10/23] afs: switch to use uuid_t and uuid_gen
On Tue, 2017-05-23 at 10:49 +0200, Christoph Hellwig wrote:
> On Mon, May 22, 2017 at 09:49:17PM +0300, Andy Shevchenko wrote:
> > > struct afs_call *call = container_of(work, struct
> > > afs_call,
> > > work);
> > > - struct uuid_v1 *r = call->request;
> > > + uuid_t *r = call->request;
> > >
> > > struct {
> > > __be32 match;
> > >
> >
> > Just to double check that this doesn't create a union aliasing.
>
> What do you mean with that?
Since we introduced a union it's possible that we might access the
member which wasn't last modified one. So, my comment is to give an
attention on such possibility and avoid if there is an aliasing
happened.
--
Andy Shevchenko
Intel Finland Oy
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 23/23] uuid: remove uuid_be
On Thu, 2017-05-18 at 10:56 +0200, Christoph Hellwig wrote: > On Thu, May 18, 2017 at 10:57:24AM +0300, Amir Goldstein wrote: > > I reviewed the entire series. You may add > > Reviewed-by: Amir Goldstein > > > > to any of the patches as you see fit. > > Thanks, done! Similar from my side, FWIW: Reviewed-by: Andy Shevchenko > > > Now let's talk about how this is going to be merged. > > Do you intend to send Linus a pull request? > > Yes, that might be the best idea. I'm also contemplating listing > me plus anyone volunteering (you?, Andy) as maintaines for the uuid > code. I would agree to be a designated reviewer for now (too many stuff to follow as a (co-)maintainer). > > The reason I am asking is because this last removal patch should > > probably > > be applied near the end of the merge window (?). > > Because maintainers cannot apply patches with code that uses the new > > uuid_t to linux-next branches and we don't want linux-next build to > > fail > > with new code that uses uuid_be... > > Yeah, we can probably defer that one for now. I would also like to append some of patches from my side, though they are WIP. -- Andy Shevchenko Intel Finland Oy ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 13/23] md: namespace private helper names
On Thu, 2017-05-18 at 08:26 +0200, Christoph Hellwig wrote:
> From: Amir Goldstein
>
> The md private helper uuid_equal() collides with a generic helper
> of the same name.
>
> Rename the md private helper to md_uuid_equal() and do the same for
> md_sb_equal().
>
While patch is good, shouldn't it go before we introduce those helpers?
> Cc: Shaohua Li
> Signed-off-by: Amir Goldstein
> Signed-off-by: Christoph Hellwig
> ---
> drivers/md/md.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 82f798be964f..65795cc4cb7d 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -825,7 +825,7 @@ static int read_disk_sb(struct md_rdev *rdev, int
> size)
> return -EINVAL;
> }
>
> -static int uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> +static int md_uuid_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> {
> return sb1->set_uuid0 == sb2->set_uuid0 &&
> sb1->set_uuid1 == sb2->set_uuid1 &&
> @@ -833,7 +833,7 @@ static int uuid_equal(mdp_super_t *sb1,
> mdp_super_t *sb2)
> sb1->set_uuid3 == sb2->set_uuid3;
> }
>
> -static int sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> +static int md_sb_equal(mdp_super_t *sb1, mdp_super_t *sb2)
> {
> int ret;
> mdp_super_t *tmp1, *tmp2;
> @@ -1025,12 +1025,12 @@ static int super_90_load(struct md_rdev *rdev,
> struct md_rdev *refdev, int minor
> } else {
> __u64 ev1, ev2;
> mdp_super_t *refsb = page_address(refdev->sb_page);
> - if (!uuid_equal(refsb, sb)) {
> + if (!md_uuid_equal(refsb, sb)) {
> pr_warn("md: %s has different UUID to %s\n",
> b, bdevname(refdev->bdev,b2));
> goto abort;
> }
> - if (!sb_equal(refsb, sb)) {
> + if (!md_sb_equal(refsb, sb)) {
> pr_warn("md: %s has same UUID but different
> superblock to %s\n",
> b, bdevname(refdev->bdev, b2));
> goto abort;
--
Andy Shevchenko
Intel Finland Oy
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 10/23] afs: switch to use uuid_t and uuid_gen
On Thu, 2017-05-18 at 08:26 +0200, Christoph Hellwig wrote:
Changelog?
> Signed-off-by: Christoph Hellwig
> Reviewed-by: David Howells
> @@ -453,7 +453,7 @@ static int afs_deliver_cb_probe(struct afs_call
> *call)
> static void SRXAFSCB_ProbeUuid(struct work_struct *work)
> {
> struct afs_call *call = container_of(work, struct afs_call,
> work);
> - struct uuid_v1 *r = call->request;
> + uuid_t *r = call->request;
>
> struct {
> __be32 match;
>
Just to double check that this doesn't create a union aliasing.
--
Andy Shevchenko
Intel Finland Oy
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 05/16] uuid: add the v1 layout to uuid_t
On Wed, 2017-05-10 at 20:02 +0200, Christoph Hellwig wrote:
> Turn the content of uuid_t into a union and add the fields for the v1
> interpretation to it.
In the branch it has fix for UUID() wrt union change, but there missed
similar for GUID().
+#define UUID(a, _b, c, d0, d1, d2, d3, d4, d5, d6,
d7) \
+{ .b = { \
^^^ This should be done for GUID() as well.
While here, can we indent \ to be the same as of the rest lines in
macro(s) ?
--
Andy Shevchenko
Intel Finland Oy
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 03/16] uuid: rename uuid types
On Wed, 2017-05-10 at 19:20 +0100, David Howells wrote: > Christoph Hellwig wrote: > > > -#define NULL_UUID_LE > > \ > > - UUID_LE(0x, 0x, 0x, 0x00, 0x00, 0x00, 0x00, > > \ > > - 0x00, 0x00, 0x00, 0x00) > > +#define NULL_GUID > > \ > > + GUID(0x, 0x, 0x, 0x00, 0x00, 0x00, 0x00, > > \ > > + 0x00, 0x00, 0x00, 0x00) > > > > -#define NULL_UUID_BE > > \ > > - UUID_BE(0x, 0x, 0x, 0x00, 0x00, 0x00, 0x00, > > \ > > - 0x00, 0x00, 0x00, 0x00) > > +#define NULL_UUID > > \ > > + UUID(0x, 0x, 0x, 0x00, 0x00, 0x00, 0x00, > > \ > > + 0x00, 0x00, 0x00, 0x00) > > These are UAPI and ought not to be renamed. > Good point. I see in the branch it's already fixed. -- Andy Shevchenko Intel Finland Oy ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
