Re: [PATCH] powerpc/pseries: Fix of_node_put() underflow during pseries remove
On 07/24/2017 09:47 PM, Michael Ellerman wrote:
> Tyrel Datwyler writes:
>
>> On 07/24/2017 03:42 AM, Michael Ellerman wrote:
>>> Laurent Vivier writes:
>>>
>>>> As for commit 68baf692c435 ("powerpc/pseries: Fix of_node_put()
>>>> underflow during DLPAR remove"), the call to of_node_put()
>>>> must be removed from pSeries_reconfig_remove_node().
>>>>
>>>> dlpar_detach_node() and pSeries_reconfig_remove_node() call
>>>> of_detach_node(), and thus the node should not be released
>>>> in this case too.
>>>>
>>>> Signed-off-by: Laurent Vivier
>>>> ---
>>>> arch/powerpc/platforms/pseries/reconfig.c | 1 -
>>>> 1 file changed, 1 deletion(-)
>>>
>>> Thanks. I'll spare you the swearing about why we have the same bug in
>>> two places.
>>
>> That's probably my bad. I must have failed to test with older powerpc-util
>> tooling where
>> drmgr uses the /proc/ofdt interface for device tree modification.
>
> OK. Really we should have automated tests of the various cases, I've
> just never had time to write any.
Agreed, some better CI is warranted.
>
> Mainly the thing that bugs me is that we still have the two separate
> paths. Or if we must maintain both they could at least share more code,
> the two functions do basically the same thing AFAICS.
Yeah, I think that is where I dropped the ball. I wrongly assumed by not
looking close
enough that code was shared in those two paths. Definitely some code
de-duplication work
that can be done.
-Tyrel
>
> cheers
>
Re: [PATCH] PCI: Convert to using %pOF instead of full_name
On 07/18/2017 02:43 PM, Rob Herring wrote: > Now that we have a custom printf format specifier, convert users of > full_name to use %pOF instead. This is preparation to remove storing > of the full path string for each node. > > Signed-off-by: Rob Herring > Cc: Thomas Petazzoni > Cc: Jason Cooper > Cc: Bjorn Helgaas > Cc: Thierry Reding > Cc: Jonathan Hunter > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Michael Ellerman > Cc: linux-...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-te...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > --- > drivers/pci/host/pci-mvebu.c| 8 > drivers/pci/host/pci-tegra.c| 3 +-- > drivers/pci/hotplug/pnv_php.c | 4 ++-- > drivers/pci/hotplug/rpadlpar_core.c | 4 ++-- > drivers/pci/hotplug/rpaphp_core.c | 2 +- > drivers/pci/hotplug/rpaphp_pci.c| 4 ++-- > drivers/pci/hotplug/rpaphp_slot.c | 4 ++-- > drivers/pci/pci-sysfs.c | 4 ++-- > drivers/pci/pci.c | 4 ++-- > 9 files changed, 18 insertions(+), 19 deletions(-) > Reviewed-by: Tyrel Datwyler
Re: [PATCH 4/4] axonram: Delete an unnecessary variable initialisation in axon_ram_probe()
On 08/03/2017 12:17 PM, SF Markus Elfring wrote: > From: Markus Elfring > Date: Thu, 3 Aug 2017 20:34:00 +0200 > > The local variable "rc" will eventually be set only to an error code. > Thus omit the explicit initialisation at the beginning. > > Signed-off-by: Markus Elfring > --- > arch/powerpc/sysdev/axonram.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/powerpc/sysdev/axonram.c b/arch/powerpc/sysdev/axonram.c > index 93cc902350db..5677f3371e30 100644 > --- a/arch/powerpc/sysdev/axonram.c > +++ b/arch/powerpc/sysdev/axonram.c > @@ -184,7 +184,6 @@ static int axon_ram_probe(struct platform_device *device) > static int axon_ram_bank_id = -1; > struct axon_ram_bank *bank; > struct resource resource; > - int rc = 0; You've completely removed the decleration of "rc" instead of removing the "= 0" initialization. I would expect a compilation test to have turned up an undeclared use error for "rc". -Tyrel > > axon_ram_bank_id++; >
Re: [PATCH v3] powerpc/powernv: Use darn instr for random_seed on p9
On 08/03/2017 06:12 PM, Matt Brown wrote:
> This adds the powernv_get_random_darn function which utilises the darn
> instruction, introduced in POWER9. The powernv_get_random_darn function
> is used as the ppc_md.get_random_seed on P9.
>
> The DARN instruction can potentially throw an error, so we attempt to
> register the powernv_get_random_darn function up to 10 times before
> failing.
>
> Signed-off-by: Matt Brown
> ---
> v3:
> - add repeat attempts to register the ppc_md.get_random_seed
> - fixed the PPC_DARN macro
> - move DARN_ERR definition
> - fixed commit message
> v2:
> - remove repeat darn attempts
> - move hook to rng_init
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 4
> arch/powerpc/platforms/powernv/rng.c | 35
> ++-
> 2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h
> b/arch/powerpc/include/asm/ppc-opcode.h
> index c4ced1d..aabd150 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -134,6 +134,7 @@
> #define PPC_INST_COPY0x7c00060c
> #define PPC_INST_COPY_FIRST 0x7c20060c
> #define PPC_INST_CP_ABORT0x7c00068c
> +#define PPC_INST_DARN0x7c0005e6
> #define PPC_INST_DCBA0x7c0005ec
> #define PPC_INST_DCBA_MASK 0xfc0007fe
> #define PPC_INST_DCBAL 0x7c2005ec
> @@ -325,6 +326,9 @@
>
> /* Deal with instructions that older assemblers aren't aware of */
> #define PPC_CP_ABORTstringify_in_c(.long PPC_INST_CP_ABORT)
> +#define PPC_DARN(t, l) stringify_in_c(.long PPC_INST_DARN | \
> + ___PPC_RT(t) | \
> + (((l) & 0x3) << 16))
> #define PPC_DCBAL(a, b) stringify_in_c(.long PPC_INST_DCBAL | \
> __PPC_RA(a) | __PPC_RB(b))
> #define PPC_DCBZL(a, b) stringify_in_c(.long PPC_INST_DCBZL | \
> diff --git a/arch/powerpc/platforms/powernv/rng.c
> b/arch/powerpc/platforms/powernv/rng.c
> index 5dcbdea..83b925c 100644
> --- a/arch/powerpc/platforms/powernv/rng.c
> +++ b/arch/powerpc/platforms/powernv/rng.c
> @@ -16,11 +16,13 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
>
> +#define DARN_ERR 0xul
>
> struct powernv_rng {
> void __iomem *regs;
> @@ -67,6 +69,21 @@ int powernv_get_random_real_mode(unsigned long *v)
> return 1;
> }
>
> +int powernv_get_random_darn(unsigned long *v)
> +{
> + unsigned long val;
> +
> + /* Using DARN with L=1 - 64-bit conditioned random number */
> + asm volatile(PPC_DARN(%0, 1) : "=r"(val));
> +
> + if (val == DARN_ERR)
> + return 0;
> +
> + *v = val;
> +
> + return 1;
> +}
> +
> int powernv_get_random_long(unsigned long *v)
> {
> struct powernv_rng *rng;
> @@ -135,8 +152,9 @@ static __init int rng_create(struct device_node *dn)
>
> static __init int rng_init(void)
> {
> + unsigned long darn_test;
> struct device_node *dn;
> - int rc;
> + int rc, i;
>
> for_each_compatible_node(dn, NULL, "ibm,power-rng") {
> rc = rng_create(dn);
> @@ -150,6 +168,21 @@ static __init int rng_init(void)
> of_platform_device_create(dn, NULL, NULL);
> }
>
> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> + for (i = 0; i < 10; i++) {
> + if (powernv_get_random_darn(&darn_test)) {
> + ppc_md.get_random_seed =
> + powernv_get_random_darn;
> + break;
If you return directly here you can avoid the (i == 9) conditional every
iteration of the
loop by moving the pr_warn to just outside the loop.
-Tyrel
> + }
> +
> + if (i == 9) {
> + pr_warn("Failed to use powernv_get_random_darn"\
> + "as get_random_seed");
> + }
> + }
> + }
> +
> return 0;
> }
> machine_subsys_initcall(powernv, rng_init);
>
Re: [PATCH] ibmvnic: Fix unused variable warning
On 08/09/2017 04:16 AM, Michal Suchanek wrote:
> Fixes: a248878d7a1d ("ibmvnic: Check for transport event on driver resume")
>
> Signed-off-by: Michal Suchanek
> ---
Reviewed-by: Tyrel Datwyler
Re: [bug report] powerpc/mm/radix: Avoid flushing the PWC on every flush_tlb_range
On 08/11/2017 01:15 PM, Dan Carpenter wrote:
> Hello Benjamin Herrenschmidt,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 424de9c6e3f8: "powerpc/mm/radix: Avoid flushing the PWC on
> every flush_tlb_range" from Jul 19, 2017, leads to the following
> Smatch complaint:
>
> arch/powerpc/mm/tlb-radix.c:368 radix__flush_tlb_collapsed_pmd()
>error: we previously assumed 'mm' could be null (see line 362)
>
> arch/powerpc/mm/tlb-radix.c
>361
>362pid = mm ? mm->context.id : 0;
> ^^
> Check for NULL.
>
>363if (unlikely(pid == MMU_NO_CONTEXT))
>364goto no_context;
>365
>366/* 4k page size, just blow the world */
>367if (PAGE_SIZE == 0x1000) {
>368radix__flush_all_mm(mm);
> ^^
> Unchecked dereference.
Appears to be a false positive. MMU_NO_CONTEXT I believe is defined as "0". So,
maybe it
would be clearer that we take the goto branch if this line read:
362 pid = mm ? mm->context.id : MMU_NO_CONTEXT;
-Tyrel
>
>369return;
>370}
>
> regards,
> dan carpenter
>
Re: [PATCH] tty: hvcs: make ktermios const
On 08/28/2017 11:30 AM, Bhumika Goyal wrote: > Make this const as it is not modified anywhere. > > Signed-off-by: Bhumika Goyal Reviewed-by: Tyrel Datwyler
Re: [PATCH 19/31] timer: Remove open-coded casts for .data and .function
On 08/31/2017 04:29 PM, Kees Cook wrote: > This standardizes the callback and data prototypes in several places that > perform casting, in an effort to remove more open-coded .data and > .function uses in favor of setup_timer(). > > Cc: Samuel Ortiz > Cc: Tyrel Datwyler > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Michael Ellerman > Cc: "James E.J. Bottomley" > Cc: "Martin K. Petersen" > Cc: net...@vger.kernel.org > Cc: linux-s...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Kees Cook > --- > drivers/net/irda/bfin_sir.c | 5 +++-- > drivers/scsi/ibmvscsi/ibmvfc.c | 14 ++ > drivers/scsi/ibmvscsi/ibmvscsi.c | 8 > 3 files changed, 13 insertions(+), 14 deletions(-) For ibmvfc & ibmvscsi portions: Acked-by: Tyrel Datwyler
Re: [PATCH 19/31] timer: Remove open-coded casts for .data and .function
On 08/31/2017 04:29 PM, Kees Cook wrote: > This standardizes the callback and data prototypes in several places that > perform casting, in an effort to remove more open-coded .data and > .function uses in favor of setup_timer(). > > Cc: Samuel Ortiz > Cc: Tyrel Datwyler > Cc: Benjamin Herrenschmidt > Cc: Paul Mackerras > Cc: Michael Ellerman > Cc: "James E.J. Bottomley" > Cc: "Martin K. Petersen" > Cc: net...@vger.kernel.org > Cc: linux-s...@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Kees Cook > --- > drivers/net/irda/bfin_sir.c | 5 +++-- > drivers/scsi/ibmvscsi/ibmvfc.c | 14 ++ > drivers/scsi/ibmvscsi/ibmvscsi.c | 8 > 3 files changed, 13 insertions(+), 14 deletions(-) Resending from correct email address. For ibmvfc & ibmvscsi portions: Acked-by: Tyrel Datwyler
Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()
On 5/5/22 12:37, Rob Herring wrote:
> On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
>> Add function which allows to dynamically allocate and free properties.
>> Use this function internally for all code that used the same logic
>> (mainly __of_prop_dup()).
>>
>> Signed-off-by: Clément Léger
>> ---
>> drivers/of/dynamic.c | 101 ++-
>> include/linux/of.h | 16 +++
>> 2 files changed, 88 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index cd3821a6444f..e8700e509d2e 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -313,9 +313,7 @@ static void property_list_free(struct property
>> *prop_list)
>>
>> for (prop = prop_list; prop != NULL; prop = next) {
>> next = prop->next;
>> -kfree(prop->name);
>> -kfree(prop->value);
>> -kfree(prop);
>> +of_property_free(prop);
>> }
>> }
>>
>> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
>> }
>>
>> /**
>> - * __of_prop_dup - Copy a property dynamically.
>> - * @prop: Property to copy
>> + * of_property_free - Free a property allocated dynamically.
>> + * @prop: Property to be freed
>> + */
>> +void of_property_free(const struct property *prop)
>> +{
>> +kfree(prop->value);
>> +kfree(prop->name);
>> +kfree(prop);
>> +}
>> +EXPORT_SYMBOL(of_property_free);
>> +
>> +/**
>> + * of_property_alloc - Allocate a property dynamically.
>> + * @name: Name of the new property
>> + * @value: Value that will be copied into the new property value
>> + * @value_len: length of @value to be copied into the new property
>> value
>> + * @len:Length of new property value, must be greater than @value_len
>
> What's the usecase for the lengths being different? That doesn't seem
> like a common case, so perhaps handle it with a NULL value and
> non-zero length. Then the caller has to deal with populating
> prop->value.
>
>> * @allocflags: Allocation flags (typically pass GFP_KERNEL)
>> *
>> - * Copy a property by dynamically allocating the memory of both the
>> + * Create a property by dynamically allocating the memory of both the
>> * property structure and the property name & contents. The property's
>> * flags have the OF_DYNAMIC bit set so that we can differentiate between
>> * dynamically allocated properties and not.
>> *
>> * Return: The newly allocated property or NULL on out of memory error.
>> */
>> -struct property *__of_prop_dup(const struct property *prop, gfp_t
>> allocflags)
>> +struct property *of_property_alloc(const char *name, const void *value,
>> + int value_len, int len, gfp_t allocflags)
>> {
>> -struct property *new;
>> +int alloc_len = len;
>> +struct property *prop;
>> +
>> +if (len < value_len)
>> +return NULL;
>>
>> -new = kzalloc(sizeof(*new), allocflags);
>> -if (!new)
>> +prop = kzalloc(sizeof(*prop), allocflags);
>> +if (!prop)
>> return NULL;
>>
>> +prop->name = kstrdup(name, allocflags);
>> +if (!prop->name)
>> +goto out_err;
>> +
>> /*
>> - * NOTE: There is no check for zero length value.
>> - * In case of a boolean property, this will allocate a value
>> - * of zero bytes. We do this to work around the use
>> - * of of_get_property() calls on boolean values.
>> + * Even if the property has no value, it must be set to a
>> + * non-null value since of_get_property() is used to check
>> + * some values that might or not have a values (ranges for
>> + * instance). Moreover, when the node is released, prop->value
>> + * is kfreed so the memory must come from kmalloc.
>
> Allowing for NULL value didn't turn out well...
>
> We know that we can do the kfree because OF_DYNAMIC is set IIRC...
>
> If we do 1 allocation for prop and value, then we can test
> for "prop->value == prop + 1" to determine if we need to free or not.
If its a single allocation do we even need a test? Doesn't kfree(prop) take care
of the property and the trailing memory allocated for the value?
-Tyrel
>
>> */
>> -new->name = kstrdup(prop->name, allocflags);
>> -new->value = kmemdup(prop->value, prop->length, allocflags);
>> -new->length = prop->length;
>> -if (!new->name || !new->value)
>> -goto err_free;
>> +if (!alloc_len)
>> +alloc_len = 1;
>>
>> -/* mark the property as dynamic */
>> -of_property_set_flag(new, OF_DYNAMIC);
>> +prop->value = kzalloc(alloc_len, allocflags);
>> +if (!prop->value)
>> +goto out_err;
>>
>> -return new;
>> +if (value)
>> +memcpy(prop->value, value, value_len);
>> +
>> +prop->length = len;
>> +of_property_set_flag(prop, OF_DYNAMIC);
>> +
>> +return prop;
>> +
>> +out_err:
>> +of_property_free(prop);
>>
>>
Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()
On 6/1/22 01:17, Clément Léger wrote:
> Add function which allows to dynamically allocate and free properties.
> Use this function internally for all code that used the same logic
> (mainly __of_prop_dup()).
>
> Signed-off-by: Clément Léger
> ---
> drivers/of/dynamic.c| 82 -
> drivers/of/of_private.h | 21 ++-
> include/linux/of.h | 14 +++
> 3 files changed, 82 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index cd3821a6444f..c0dcbea31d28 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c
> @@ -313,9 +313,7 @@ static void property_list_free(struct property *prop_list)
>
> for (prop = prop_list; prop != NULL; prop = next) {
> next = prop->next;
> - kfree(prop->name);
> - kfree(prop->value);
> - kfree(prop);
> + of_property_free(prop);
> }
> }
>
> @@ -367,48 +365,66 @@ void of_node_release(struct kobject *kobj)
> }
>
> /**
> - * __of_prop_dup - Copy a property dynamically.
> - * @prop:Property to copy
> + * of_property_free - Free a property allocated dynamically.
> + * @prop:Property to be freed
> + */
> +void of_property_free(const struct property *prop)
> +{
> + if (!of_property_check_flag(prop, OF_DYNAMIC))
> + return;
> +
This looks wrong to me. From what I understand the value data is allocated as
trailing memory that is part of the property allocation itself. (ie. prop =
kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
of the trailing value data. Calling kfree(prop->value) is bogus since
prop->value wasn't dynamically allocated on its own.
Also, this condition will always fail. You explicitly set prop->value = prop + 1
in alloc.
Maybe I need to go back and look at v1 again.
-Tyrel
> + if (prop->value != prop + 1)
> + kfree(prop->value);
> +
> + kfree(prop->name);
> + kfree(prop);
> +}
> +EXPORT_SYMBOL(of_property_free);
> +
> +/**
> + * of_property_alloc - Allocate a property dynamically.
> + * @name:Name of the new property
> + * @value: Value that will be copied into the new property value or NULL
> + * if only @len allocation is needed.
> + * @len: Length of new property value and if @value is provided, the
> + * length of the value to be copied
> * @allocflags: Allocation flags (typically pass GFP_KERNEL)
> *
> - * Copy a property by dynamically allocating the memory of both the
> + * Create a property by dynamically allocating the memory of both the
> * property structure and the property name & contents. The property's
> * flags have the OF_DYNAMIC bit set so that we can differentiate between
> * dynamically allocated properties and not.
> *
> * Return: The newly allocated property or NULL on out of memory error.
> */
> -struct property *__of_prop_dup(const struct property *prop, gfp_t allocflags)
> +struct property *of_property_alloc(const char *name, const void *value,
> +size_t len, gfp_t allocflags)
> {
> - struct property *new;
> + struct property *prop;
>
> - new = kzalloc(sizeof(*new), allocflags);
> - if (!new)
> + prop = kzalloc(sizeof(*prop) + len, allocflags);
> + if (!prop)
> return NULL;
>
> - /*
> - * NOTE: There is no check for zero length value.
> - * In case of a boolean property, this will allocate a value
> - * of zero bytes. We do this to work around the use
> - * of of_get_property() calls on boolean values.
> - */
> - new->name = kstrdup(prop->name, allocflags);
> - new->value = kmemdup(prop->value, prop->length, allocflags);
> - new->length = prop->length;
> - if (!new->name || !new->value)
> - goto err_free;
> -
> - /* mark the property as dynamic */
> - of_property_set_flag(new, OF_DYNAMIC);
> -
> - return new;
> -
> - err_free:
> - kfree(new->name);
> - kfree(new->value);
> - kfree(new);
> + prop->name = kstrdup(name, allocflags);
> + if (!prop->name)
> + goto out_err;
> +
> + prop->value = prop + 1;
> + if (value)
> + memcpy(prop->value, value, len);
> +
> + prop->length = len;
> + of_property_set_flag(prop, OF_DYNAMIC);
> +
> + return prop;
> +
> +out_err:
> + of_property_free(prop);
> +
> return NULL;
> }
> +EXPORT_SYMBOL(of_property_alloc);
>
> /**
> * __of_node_dup() - Duplicate or create an empty device node dynamically.
> @@ -447,9 +463,7 @@ struct device_node *__of_node_dup(const struct
> device_node *np,
> if (!new_pp)
> goto err_prop;
> if (__of_add_property(node, new_pp)) {
> - kfree(new_pp->name);
> - kfree(new_pp->value);
> - kfree(new_pp);
> +
Re: [PATCH 1/3] of: dynamic: add of_property_alloc() and of_property_free()
On 6/2/22 07:06, Rob Herring wrote:
> On Wed, Jun 1, 2022 at 5:31 PM Tyrel Datwyler wrote:
>>
>> On 5/5/22 12:37, Rob Herring wrote:
>>> On Wed, May 04, 2022 at 05:40:31PM +0200, Clément Léger wrote:
>>>> Add function which allows to dynamically allocate and free properties.
>>>> Use this function internally for all code that used the same logic
>>>> (mainly __of_prop_dup()).
>>>>
>>>> Signed-off-by: Clément Léger
>>>> ---
>>>> drivers/of/dynamic.c | 101 ++-
>>>> include/linux/of.h | 16 +++
>>>> 2 files changed, 88 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>>>> index cd3821a6444f..e8700e509d2e 100644
>>>> --- a/drivers/of/dynamic.c
>>>> +++ b/drivers/of/dynamic.c
>>>> @@ -313,9 +313,7 @@ static void property_list_free(struct property
>>>> *prop_list)
>>>>
>>>> for (prop = prop_list; prop != NULL; prop = next) {
>>>> next = prop->next;
>>>> -kfree(prop->name);
>>>> -kfree(prop->value);
>>>> -kfree(prop);
>>>> +of_property_free(prop);
>>>> }
>>>> }
>>>>
>>>> @@ -367,48 +365,95 @@ void of_node_release(struct kobject *kobj)
>>>> }
>>>>
>>>> /**
>>>> - * __of_prop_dup - Copy a property dynamically.
>>>> - * @prop: Property to copy
>>>> + * of_property_free - Free a property allocated dynamically.
>>>> + * @prop: Property to be freed
>>>> + */
>>>> +void of_property_free(const struct property *prop)
>>>> +{
>>>> +kfree(prop->value);
>>>> +kfree(prop->name);
>>>> +kfree(prop);
>>>> +}
>>>> +EXPORT_SYMBOL(of_property_free);
>>>> +
>>>> +/**
>>>> + * of_property_alloc - Allocate a property dynamically.
>>>> + * @name: Name of the new property
>>>> + * @value: Value that will be copied into the new property value
>>>> + * @value_len: length of @value to be copied into the new property
>>>> value
>>>> + * @len:Length of new property value, must be greater than @value_len
>>>
>>> What's the usecase for the lengths being different? That doesn't seem
>>> like a common case, so perhaps handle it with a NULL value and
>>> non-zero length. Then the caller has to deal with populating
>>> prop->value.
>>>
>>>> * @allocflags: Allocation flags (typically pass GFP_KERNEL)
>>>> *
>>>> - * Copy a property by dynamically allocating the memory of both the
>>>> + * Create a property by dynamically allocating the memory of both the
>>>> * property structure and the property name & contents. The property's
>>>> * flags have the OF_DYNAMIC bit set so that we can differentiate between
>>>> * dynamically allocated properties and not.
>>>> *
>>>> * Return: The newly allocated property or NULL on out of memory error.
>>>> */
>>>> -struct property *__of_prop_dup(const struct property *prop, gfp_t
>>>> allocflags)
>>>> +struct property *of_property_alloc(const char *name, const void *value,
>>>> + int value_len, int len, gfp_t allocflags)
>>>> {
>>>> -struct property *new;
>>>> +int alloc_len = len;
>>>> +struct property *prop;
>>>> +
>>>> +if (len < value_len)
>>>> +return NULL;
>>>>
>>>> -new = kzalloc(sizeof(*new), allocflags);
>>>> -if (!new)
>>>> +prop = kzalloc(sizeof(*prop), allocflags);
>>>> +if (!prop)
>>>> return NULL;
>>>>
>>>> +prop->name = kstrdup(name, allocflags);
>>>> +if (!prop->name)
>>>> +goto out_err;
>>>> +
>>>> /*
>>>> - * NOTE: There is no check for zero length value.
>>>> - * In case of a boolean property, this will allocate a value
>>>> - * of zero bytes. We do this to work around the use
>>>> - * of of_get_property() calls on boolean values.
>>>> + * Even if the property has no value, it must be set to a
>>>> + * non-null value since of_get_property() is used to check
>>>> + * some values that might or not have a values (ranges for
>>>> + * instance). Moreover, when the node is released, prop->value
>>>> + * is kfreed so the memory must come from kmalloc.
>>>
>>> Allowing for NULL value didn't turn out well...
>>>
>>> We know that we can do the kfree because OF_DYNAMIC is set IIRC...
>>>
>>> If we do 1 allocation for prop and value, then we can test
>>> for "prop->value == prop + 1" to determine if we need to free or not.
>>
>> If its a single allocation do we even need a test? Doesn't kfree(prop) take
>> care
>> of the property and the trailing memory allocated for the value?
>
> Yes, it does when it's a single alloc, but it's testing for when
> prop->value is not a single allocation because we could have either.
>
Ok, that is the part I was missing. Thanks for the clarification.
-Tyrel
Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property_free()
On 6/1/22 23:58, Clément Léger wrote:
> Le Wed, 1 Jun 2022 15:32:29 -0700,
> Tyrel Datwyler a écrit :
>
>>> /**
>>> - * __of_prop_dup - Copy a property dynamically.
>>> - * @prop: Property to copy
>>> + * of_property_free - Free a property allocated dynamically.
>>> + * @prop: Property to be freed
>>> + */
>>> +void of_property_free(const struct property *prop)
>>> +{
>>> + if (!of_property_check_flag(prop, OF_DYNAMIC))
>>> + return;
>>> +
>>
>> This looks wrong to me. From what I understand the value data is allocated as
>> trailing memory that is part of the property allocation itself. (ie. prop =
>> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take
>> care
>> of the trailing value data. Calling kfree(prop->value) is bogus since
>> prop->value wasn't dynamically allocated on its own.
>
> kfree(prop->value) is only called if the value is not the trailing data
> of the property so I don't see what is wrong there. In that case, only
> kfree(prop) is called.
Right, Rob clarified for me in the v1 patch.
>
>>
>> Also, this condition will always fail. You explicitly set prop->value = prop
>> + 1
>> in alloc.
>
> The user that did allocated the property might want to provide its own
> "value". In that case, prop->value would be overwritten by the user
> allocated value and thus the check would be true, hence calling
> kfree(prop->value).
So, that was the part I was missing. I think a comment would be helpful so its
clear value can be either trailing or user assigned.
-Tyrel
>
>>
>> Maybe I need to go back and look at v1 again.
>>
>> -Tyrel
>>
>>> + if (prop->value != prop + 1)
>>> + kfree(prop->value);
>>> +
>>> + kfree(prop->name);
>>> + kfree(prop);
>>> +}
>>> +EXPORT_SYMBOL(of_property_free);
>>> +
>
>
Re: [PATCH] powerpc/pci: Add config option for using OF 'reg' for PCI domain
On 6/9/22 13:21, Guilherme G. Piccoli wrote: > First of all, thanks for looping me Bjorn! Much appreciated. > I'm also CCing Ben and Gavin, that know a lot of PPC PCI stuff. > > > On 09/06/2022 16:34, Bjorn Helgaas wrote: >> [...] >>> Upgrading powerpc kernels from LTS 4.4 version (which does not >>> contain mentioned commit) to new LTS versions brings a >>> regression in domain assignment. >> >> Can you elaborate why it is a regression ? >> 63a72284b159 That commit says 'no functionnal changes', I'm >> having hard time understanding how a nochange can be a >> regression. > > It is not 'no functional change'. That commit completely changed > PCI domain assignment in a way that is incompatible with other > architectures and also incompatible with the way how it was done > prior that commit. I agree that the "no functional change" statement is incorrect. However, for most powerpc platforms it ended up being simply a cosmetic behavior change. As far as I can tell there is nothing requiring domain ids to increase montonically from zero or that each architecture is required to use the same domain numbering scheme. > > Strongly agree here in both points: first, this was not a "no functional > change" thing, and I apologize for adding this in the commit message. > What I meant is that: despite changing the numbering, (as Tyrel said) > nothing should require increasing monotonic mutable PCI domains. At > least, I'm not aware of such requirement in any spec or even in the > kernel and adjacent tooling. > > >>> [...] We could properly limit it to powernv and pseries by using ibm,fw-phb-id instead of reg property in the look up that follows a failed ibm,opal-phbid lookup. I think this is acceptable as long as no other powerpc platforms have started using this behavior for persistent naming. >>> >>> And what about setting that new config option to enabled by default >>> for those series? > > I don't remember all the details about PPC dt, but it should already be > restricted to pseries/powernv, right? At least, the commit has a comment: > > /* If not pseries nor powernv, or if fixed PHB numbering tried to add > * the same PHB number twice, then fallback to dynamic PHB numbering.*/ > > If this is *not* restricted to these 2 platforms, I agree with Pali's > approach, although I'd consider the correct is to keep the persistent > domain scheme for both pnv and pseries, as it's working like this for 5 > years and counting, and this *does* prevent a lot of issues with PCI > hotplugging in PPC servers. I mentioned this in a previous post, but it is clear the Author's intent was for this only to apply to powernv and pseries platforms. However, it only really checks for powernv, and if that fails it does a read the reg property for the domain which works for and PPC platform. If we really only want this on powernv and pseries and revert all other PPC platforms back we can fix this with a pseries check instead of a config property. Using ibm,fw-phb-id instead of reg property if ibm,opal-phbid lookup fails does the trick. -Tyrel > > >>> [...] I forgot to ask before about the actual regression here. The commit log says domain numbers are different, but I don't know the connection from there to something failing. I assume there's some script or config file that depends on specific domain numbers? And that dependency is (hopefully) powerpc-specific? >>> >>> You assume correct. For example this is the way how OpenWRT handles PCI >>> devices (but not only OpenWRT). This OpenWRT case is not >>> powerpc-specific but generic to all architectures. This is just one >>> example. >> >> So basically everybody uses D/b/d/f for persistent names. That's ... >> well, somewhat stable for things soldered down or in a motherboard >> slot, but a terrible idea for things that can be hot-plugged. >> >> Even for more core things, it's possible for firmware to change bus >> numbering between boots. For example, if a complicated hierarchy is >> cold-plugged into one slot, firmware is likely to assign different bus >> numbers on the next boot to make room for it. Obviously this can also >> happen as a hot-add, and Linux needs the flexibility to do similar >> renumbering then, although we don't support it yet. >> >> It looks like 63a72284b159 was intended to make domain numbers *more* >> consistent, so it's ironic that this actually broke something by >> changing domain numbers. Maybe there's a way to limit the scope of >> 63a72284b159 so it avoids the breakage. I don't know enough about the >> powerpc landscape to even guess at how. > > I don't considereit breaks the userspace since this is definitely no > stable ABI (or if it is, I'm not aware and my apologies). If scripts > rely on
Re: [PATCH] powerpc/rtas: Allow ibm,platform-dump RTAS call with null buffer address
On 6/14/22 06:49, Andrew Donnellan wrote:
> Add a special case to block_rtas_call() to allow the ibm,platform-dump RTAS
> call through the RTAS filter if the buffer address is 0.
>
> According to PAPR, ibm,platform-dump is called with a null buffer address
> to notify the platform firmware that processing of a particular dump is
> finished.
>
> Without this, on a pseries machine with CONFIG_PPC_RTAS_FILTER enabled, an
> application such as rtas_errd that is attempting to retrieve a dump will
> encounter an error at the end of the retrieval process.
>
> Fixes: bd59380c5ba4 ("powerpc/rtas: Restrict RTAS requests from userspace")
> Cc: sta...@vger.kernel.org
> Reported-by: Sathvika Vasireddy
> Signed-off-by: Andrew Donnellan
Similar to what is done for ibm,configure-connector with idx_buf2 and a NULL
address.
Reviewed-by: Tyrel Datwyler
> ---
> arch/powerpc/kernel/rtas.c | 11 ++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index a6fce3106e02..693133972294 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -1071,7 +1071,7 @@ static struct rtas_filter rtas_filters[]
> __ro_after_init = {
> { "get-time-of-day", -1, -1, -1, -1, -1 },
> { "ibm,get-vpd", -1, 0, -1, 1, 2 },
> { "ibm,lpar-perftools", -1, 2, 3, -1, -1 },
> - { "ibm,platform-dump", -1, 4, 5, -1, -1 },
> + { "ibm,platform-dump", -1, 4, 5, -1, -1 }, /* Special
> cased */
> { "ibm,read-slot-reset-state", -1, -1, -1, -1, -1 },
> { "ibm,scan-log-dump", -1, 0, 1, -1, -1 },
> { "ibm,set-dynamic-indicator", -1, 2, -1, -1, -1 },
> @@ -1120,6 +1120,15 @@ static bool block_rtas_call(int token, int nargs,
> size = 1;
>
> end = base + size - 1;
> +
> + /*
> + * Special case for ibm,platform-dump - NULL buffer
> + * address is used to indicate end of dump processing
> + */
> + if (!strcmp(f->name, "ibm,platform-dump") &&
> + base == 0)
> + return false;
> +
> if (!in_rmo_buf(base, end))
> goto err;
> }
Re: [PATCH v2 0/4] pseries-wdt: initial support for H_WATCHDOG-based watchdog timers
On 6/15/22 18:43, Daniel Henrique Barboza wrote: > Hi, > > I tried this series out with mainline QEMU built with Alexey's patch [1] > and I wasn't able to get it to work. I'm using a simple QEMU command line > booting a fedora36 guest in a Power9 boston host: I would assume the H_WATCHDOG hypercall is not implemented by the qemu pseries machine type. It is a very new hypercall. -Tyrel > > sudo ./qemu-system-ppc64 \ > -M > pseries,cap-cfpc=broken,cap-sbbc=broken,cap-ibs=broken,cap-ccf-assist=off,ic-mode=dual > \ > -m 4G -accel kvm -cpu POWER9 -smp 1,maxcpus=1,threads=1,cores=1,sockets=1 \ > -device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x2 \ > -drive > file=/home/danielhb/fedora36.qcow2,if=none,id=drive-scsi0-0-0-0,format=qcow2,cache=none > \ > -device > scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2 > \ > -device qemu-xhci,id=usb,bus=pci.0,addr=0x4 -nographic -display none > > > Guest is running v5.19-rc2 with this series applied. Kernel config consists of > 'pseries_le_defconfig' plus the following 'watchdog' related changes: > > [root@fedora ~]# cat linux/.config | grep PSERIES_WDT > CONFIG_PSERIES_WDT=y > > [root@fedora ~]# cat linux/.config | grep -i watchdog > CONFIG_PPC_WATCHDOG=y > CONFIG_HAVE_NMI_WATCHDOG=y > CONFIG_WATCHDOG=y > CONFIG_WATCHDOG_CORE=y > CONFIG_WATCHDOG_NOWAYOUT=y > CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=y > CONFIG_WATCHDOG_OPEN_TIMEOUT=0 > # CONFIG_WATCHDOG_SYSFS is not set > # CONFIG_WATCHDOG_HRTIMER_PRETIMEOUT is not set > # Watchdog Pretimeout Governors > # CONFIG_WATCHDOG_PRETIMEOUT_GOV is not set > # Watchdog Device Drivers > # CONFIG_SOFT_WATCHDOG is not set > # CONFIG_XILINX_WATCHDOG is not set > # CONFIG_ZIIRAVE_WATCHDOG is not set > # CONFIG_CADENCE_WATCHDOG is not set > # CONFIG_DW_WATCHDOG is not set > # CONFIG_MAX63XX_WATCHDOG is not set > CONFIG_WATCHDOG_RTAS=y > # PCI-based Watchdog Cards > # CONFIG_PCIPCWATCHDOG is not set > # USB-based Watchdog Cards > # CONFIG_USBPCWATCHDOG is not set > # CONFIG_WQ_WATCHDOG is not set > [root@fedora ~]# > > > > Kernel command line: > > [root@fedora ~]# cat /proc/cmdline > BOOT_IMAGE=(ieee1275/disk,msdos2)/vmlinuz-5.19.0-rc2-00054-g12ede8ffb103 \ > root=/dev/mapper/fedora_fedora-root ro rd.lvm.lv=fedora_fedora/root \ > pseries-wdt.timeout=60 pseries-wdt.nowayout=1 pseries-wdt.action=2 > > > With all that, executing > > echo V > /dev/watchdog0 > > Does nothing. dmesg is clean and the guest doesn't reboot after the 60 sec > timeout. I also tried with PSERIES_WDT being compiled as a module instead > of built-in. Same results. > > > What am I missing? > > > [1] > https://patchwork.ozlabs.org/project/qemu-ppc/patch/20220608030153.1862335-1-...@ozlabs.ru/ > > > > > Thanks, > > > Daniel > > > > > On 6/2/22 14:53, Scott Cheloha wrote: >> PAPR v2.12 defines a new hypercall, H_WATCHDOG. This patch series >> adds support for this hypercall to powerpc/pseries kernels and >> introduces a new watchdog driver, "pseries-wdt", for the virtual >> timers exposed by the hypercall. >> >> This series is preceded by the following: >> >> RFC v1: >> https://lore.kernel.org/linux-watchdog/20220413165104.179144-1-chel...@linux.ibm.com/ >> >> RFC v2: >> https://lore.kernel.org/linux-watchdog/20220509174357.5448-1-chel...@linux.ibm.com/ >> >> PATCH v1: >> https://lore.kernel.org/linux-watchdog/20220520183552.33426-1-chel...@linux.ibm.com/ >> >> >> Changes of note from PATCH v1: >> >> - Trim down the large comment documenting the H_WATCHDOG hypercall. >> The comment is likely to rot, so remove anything we aren't using >> and anything overly obvious. >> >> - Remove any preprocessor definitions not actually used in the module >> right now. If we want to use other features offered by the hypercall >> we can add them in later. They're just clutter until then. >> >> - Simplify the "action" module parameter. The value is now an index >> into an array of possible timeoutAction values. This design removes >> the need for the custom get/set methods used in PATCH v1. >> >> Now we merely need to check that the "action" value is a valid >> index during pseries_wdt_probe(). Easy. >> >> - Make the timeoutAction a member of pseries_wdt, "action". This >> eliminates the use of a global variable during pseries_wdt_start(). >> >> - Use watchdog_init_timeout() idiomatically. Check its return value >> and error out of pseries_wdt_probe() if it fails. >> >>
[PATCH] ibmvfc: multiqueue bug fixes
Fixes for a couple observed crashes of the ibmvfc driver when in MQ mode. Tyrel Datwyler (2): ibmvfc: store vhost pointer during subcrq allocation ibmvfc: alloc/free queue resource only during probe/remove drivers/scsi/ibmvscsi/ibmvfc.c | 82 ++ drivers/scsi/ibmvscsi/ibmvfc.h | 2 +- 2 files changed, 65 insertions(+), 19 deletions(-) -- 2.35.3
[PATCH] ibmvfc: store vhost pointer during subcrq allocation
Currently the back pointer from a queue to the vhost adapter isn't set
until after subcrq interrupt registration. The value is available when a queue
is first allocated and can/should be also set for primary and async queues as
well as subcrq's.
This fixes a crash observed during kexec/kdump on Power 9 with legacy XICS
interrupt controller where a pending subcrq interrupt from the previous kernel
can be replayed immediately upon IRQ registration resulting in dereference of a
garbage backpointer in ibmvfc_interrupt_scsi.
Kernel attempted to read user page (58) - exploit attempt? (uid: 0)
BUG: Kernel NULL pointer dereference on read at 0x0058
Faulting instruction address: 0xc00803216a08
Oops: Kernel access of bad area, sig: 11 [#1]
...
NIP [c00803216a08] ibmvfc_interrupt_scsi+0x40/0xb0 [ibmvfc]
LR [c82079e8] __handle_irq_event_percpu+0x98/0x270
Call Trace:
[c00047fa3d80] [c000123e6180] 0xc000123e6180 (unreliable)
[c00047fa3df0] [c82079e8] __handle_irq_event_percpu+0x98/0x270
[c00047fa3ea0] [c8207d18] handle_irq_event+0x98/0x188
[c00047fa3ef0] [c820f564] handle_fasteoi_irq+0xc4/0x310
[c00047fa3f40] [c8205c60] generic_handle_irq+0x50/0x80
[c00047fa3f60] [c8015c40] __do_irq+0x70/0x1a0
[c00047fa3f90] [c8016d7c] __do_IRQ+0x9c/0x130
[c00014622f60] [2000] 0x2000
[c00014622ff0] [c8016e50] do_IRQ+0x40/0xa0
[c00014623020] [c8017044] replay_soft_interrupts+0x194/0x2f0
[c00014623210] [c80172a8] arch_local_irq_restore+0x108/0x170
[c00014623240] [c8eb1008] _raw_spin_unlock_irqrestore+0x58/0xb0
[c00014623270] [c820b12c] __setup_irq+0x49c/0x9f0
[c00014623310] [c820b7c0] request_threaded_irq+0x140/0x230
[c00014623380] [c00803212a50] ibmvfc_register_scsi_channel+0x1e8/0x2f0
[ibmvfc]
[c00014623450] [c00803213d1c] ibmvfc_init_sub_crqs+0xc4/0x1f0 [ibmvfc]
[c000146234d0] [c008032145a8] ibmvfc_reset_crq+0x150/0x210 [ibmvfc]
[c00014623550] [c008032147c8] ibmvfc_init_crq+0x160/0x280 [ibmvfc]
[c000146235f0] [c0080321a9cc] ibmvfc_probe+0x2a4/0x530 [ibmvfc]
Fixes: 3034ebe263897 ("scsi: ibmvfc: Add alloc/dealloc routines for SCSI
Sub-CRQ Channels")
Cc: sta...@vger.kernel.org
Signed-off-by: Tyrel Datwyler
Reviewed-by: Brian King
---
drivers/scsi/ibmvscsi/ibmvfc.c | 3 ++-
drivers/scsi/ibmvscsi/ibmvfc.h | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index d0eab5700dc5..9fc0ffda6ae8 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -5682,6 +5682,8 @@ static int ibmvfc_alloc_queue(struct ibmvfc_host *vhost,
queue->cur = 0;
queue->fmt = fmt;
queue->size = PAGE_SIZE / fmt_size;
+
+ queue->vhost = vhost;
return 0;
}
@@ -5790,7 +5792,6 @@ static int ibmvfc_register_scsi_channel(struct
ibmvfc_host *vhost,
}
scrq->hwq_id = index;
- scrq->vhost = vhost;
LEAVE;
return 0;
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index 3718406e0988..c39a245f43d0 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -789,6 +789,7 @@ struct ibmvfc_queue {
spinlock_t _lock;
spinlock_t *q_lock;
+ struct ibmvfc_host *vhost;
struct ibmvfc_event_pool evt_pool;
struct list_head sent;
struct list_head free;
@@ -797,7 +798,6 @@ struct ibmvfc_queue {
union ibmvfc_iu cancel_rsp;
/* Sub-CRQ fields */
- struct ibmvfc_host *vhost;
unsigned long cookie;
unsigned long vios_cookie;
unsigned long hw_irq;
--
2.35.3
[PATCH] ibmvfc: alloc/free queue resource only during probe/remove
Currently, the sub-queues and event pool resources are alloc/free'd
for every CRQ connection event such as reset and LPM. This exposes the driver to
a couple issues. First the inefficiency of freeing and reallocating memory that
can simply be resued after being sanitized. Further, a system under memory
pressue runs the risk of allocation failures that could result in a cripled
driver. Finally, there is a race window where command submission/compeletion can
try to pull/return elements from/to an event pool that is being deleted or
already has been deleted due to the lack of host state around freeing/allocating
resources. The following is an example of list corruption following a live
partition migration (LPM):
Oops: Exception in kernel mode, sig: 5 [#1]
LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
Modules linked in: vfat fat isofs cdrom ext4 mbcache jbd2 nft_counter
nft_compat nf_tables nfnetlink rpadlpar_io rpaphp xsk_diag nfsv3 nfs_acl nfs
lockd grace fscache netfs rfkill bonding tls sunrpc pseries_rng drm
drm_panel_orientation_quirks xfs libcrc32c dm_service_time sd_mod t10_pi sg
ibmvfc scsi_transport_fc ibmveth vmx_crypto dm_multipath dm_mirror
dm_region_hash dm_log dm_mod ipmi_devintf ipmi_msghandler fuse
CPU: 0 PID: 2108 Comm: ibmvfc_0 Kdump: loaded Not tainted
5.14.0-70.9.1.el9_0.ppc64le #1
NIP: c07c4bb0 LR: c07c4bac CTR: 005b9a10
REGS: c0025c10b760 TRAP: 0700 Not tainted (5.14.0-70.9.1.el9_0.ppc64le)
MSR: 8282b033 CR: 2800028f XER:
000f
CFAR: c01f55bc IRQMASK: 0
GPR00: c07c4bac c0025c10ba00 c2a47c00
004e
GPR04: c031e3006f88 c031e308bd00 c0025c10b768
0027
GPR08: c031e3009dc0 0031e0eb
GPR12: c031e2a8 c2dd c0187108
c0020fcee2c0
GPR16:
GPR20:
c00802f81300
GPR24: 5deadbeef100 5deadbeef122 c00263ba6910
c0024cc88000
GPR28: 003c c002430a c002430ac300
c300
NIP [c07c4bb0] __list_del_entry_valid+0x90/0x100
LR [c07c4bac] __list_del_entry_valid+0x8c/0x100
Call Trace:
[c0025c10ba00] [c07c4bac] __list_del_entry_valid+0x8c/0x100
(unreliable)
[c0025c10ba60] [c00802f42284] ibmvfc_free_queue+0xec/0x210 [ibmvfc]
[c0025c10bb10] [c00802f4246c] ibmvfc_deregister_scsi_channel+0xc4/0x160
[ibmvfc]
[c0025c10bba0] [c00802f42580] ibmvfc_release_sub_crqs+0x78/0x130
[ibmvfc]
[c0025c10bc20] [c00802f4f6cc] ibmvfc_do_work+0x5c4/0xc70 [ibmvfc]
[c0025c10bce0] [c00802f4fdec] ibmvfc_work+0x74/0x1e8 [ibmvfc]
[c0025c10bda0] [c01872b8] kthread+0x1b8/0x1c0
[c0025c10be10] [c000cd64] ret_from_kernel_thread+0x5c/0x64
Instruction dump:
40820034 3861 38210060 4e800020 7c0802a6 7c641b78 3c62fe7a 7d254b78
3863b590 f8010070 4ba309cd 6000 <0fe0> 7c0802a6 3c62fe7a 3863b640
---[ end trace 11a2b65a92f8b66c ]---
ibmvfc 3003: Send warning. Receive queue closed, will retry.
Add registration/deregistartion helpers that are called instead during
connection resets to sanitize and reconfigure the queues.
Fixes: 3034ebe26389 ("scsi: ibmvfc: Add alloc/dealloc routines for SCSI Sub-CRQ
Channels")
Cc: sta...@vger.kernel.org
Signed-off-by: Tyrel Datwyler
Reviewed-by: Brian King
---
drivers/scsi/ibmvscsi/ibmvfc.c | 79 ++
1 file changed, 62 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 9fc0ffda6ae8..00684e11976b 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -160,8 +160,8 @@ static void ibmvfc_npiv_logout(struct ibmvfc_host *);
static void ibmvfc_tgt_implicit_logout_and_del(struct ibmvfc_target *);
static void ibmvfc_tgt_move_login(struct ibmvfc_target *);
-static void ibmvfc_release_sub_crqs(struct ibmvfc_host *);
-static void ibmvfc_init_sub_crqs(struct ibmvfc_host *);
+static void ibmvfc_dereg_sub_crqs(struct ibmvfc_host *);
+static void ibmvfc_reg_sub_crqs(struct ibmvfc_host *);
static const char *unknown_error = "unknown error";
@@ -917,7 +917,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host
*vhost)
struct vio_dev *vdev = to_vio_dev(vhost->dev);
unsigned long flags;
- ibmvfc_release_sub_crqs(vhost);
+ ibmvfc_dereg_sub_crqs(vhost);
/* Re-enable the CRQ */
do {
@@ -936,7 +936,7 @@ static int ibmvfc_reenable_crq_queue(struct ibmvfc_host
*vhost)
spin_unlock(vhost->crq.q_lock);
spin_unlock_irqrestore(vhost->host->host_lock, flags);
- ibmvfc_init_sub_crqs(vhost);
+ ibmvfc_reg_sub_crqs(vhost);
return rc;
}
@@ -955,7 +
Re: [PATCH] powerpc: kernel: pci_dn: Add missing of_node_put() for of_get_xx API
On 7/1/22 06:17, Liang He wrote:
> In pci_add_device_node_info(), we should use of_node_put() for the
> reference 'parent' returned by of_get_parent() to keep refcount
> balance.
>
> Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn")
> Co-authored-by: Miaoqian Lin
> Signed-off-by: Liang He
> ---
> arch/powerpc/kernel/pci_dn.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index 938ab8838ab5..aa221958007e 100644
> --- a/arch/powerpc/kernel/pci_dn.c
> +++ b/arch/powerpc/kernel/pci_dn.c
> @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct
> pci_controller *hose,
> INIT_LIST_HEAD(&pdn->list);
> parent = of_get_parent(dn);
> pdn->parent = parent ? PCI_DN(parent) : NULL;
NACK
pdn->parent is now a long term reference so we should not do a put on parent
until we pdn->parent is no longer valid.
-Tyrel
> + of_node_put(parent);
> if (pdn->parent)
> list_add_tail(&pdn->list, &pdn->parent->child_list);
>
Re: [PATCH] powerpc: kernel: pci_dn: Add missing of_node_put() for of_get_xx API
On 7/1/22 12:47, Tyrel Datwyler wrote:
> On 7/1/22 06:17, Liang He wrote:
>> In pci_add_device_node_info(), we should use of_node_put() for the
>> reference 'parent' returned by of_get_parent() to keep refcount
>> balance.
>>
>> Fixes: cca87d303c85 ("powerpc/pci: Refactor pci_dn")
>> Co-authored-by: Miaoqian Lin
>> Signed-off-by: Liang He
>> ---
>> arch/powerpc/kernel/pci_dn.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
>> index 938ab8838ab5..aa221958007e 100644
>> --- a/arch/powerpc/kernel/pci_dn.c
>> +++ b/arch/powerpc/kernel/pci_dn.c
>> @@ -330,6 +330,7 @@ struct pci_dn *pci_add_device_node_info(struct
>> pci_controller *hose,
>> INIT_LIST_HEAD(&pdn->list);
>> parent = of_get_parent(dn);
>> pdn->parent = parent ? PCI_DN(parent) : NULL;
> NACK
>
> pdn->parent is now a long term reference so we should not do a put on parent
> until we pdn->parent is no longer valid.
I withdraw my NACK. On closer inspection pdn->parent is a reference to the
parent struct pci_dn and not a reference to a parent struct device_node.
Reviewed-by: Tyrel Datwyler
>
> -Tyrel
>
>> +of_node_put(parent);
>> if (pdn->parent)
>> list_add_tail(&pdn->list, &pdn->parent->child_list);
>>
>
